Bug 39302 - Checkins can disappear from checkin list if transfer modal is triggered
Summary: Checkins can disappear from checkin list if transfer modal is triggered
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: Baptiste Wojtkowski (bwoj)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-03-11 11:11 UTC by Jonathan Druart
Modified: 2025-03-13 09:30 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39302: Fix logic flaw in the display count of checkins (39.54 KB, patch)
2025-03-11 11:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39302: Fix logic flaw in the display count of checkins (39.61 KB, patch)
2025-03-11 14:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Behaviour with patch (89.92 KB, image/png)
2025-03-12 10:23 UTC, Baptiste Wojtkowski (bwoj)
Details
Behaviour without patch (81.16 KB, image/png)
2025-03-12 10:23 UTC, Baptiste Wojtkowski (bwoj)
Details
Bug 39302: Restore item's info (634 bytes, patch)
2025-03-12 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39302: Fix logic flaw in the display count of checkins (39.67 KB, patch)
2025-03-12 15:44 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 39302: Restore item's info (698 bytes, patch)
2025-03-12 15:44 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-03-11 11:11:55 UTC
There is a flaw in the logic of the checkin code: we keep 20 (default value for numReturnedItemsToShow) checkins in the table, but there can be shift in the "counter" logic when the transfer modal is triggered.

There are actually several other problems in the logic of this controller but we are going to try and focus on only this one.
Comment 1 Jonathan Druart 2025-03-11 11:30:45 UTC
Created attachment 179160 [details] [review]
Bug 39302: Fix logic flaw in the display count of checkins

Checkins can disappear from the checkin list if transfer modal is
triggered.

There is a flaw in the logic of the checkin code: we keep 20 (default
value for numReturnedItemsToShow) checkins in the table, but there can
be shift in the "counter" logic when the transfer modal is triggered.

This patch is removing the different hashes and use a single @checkins
array to pass to the template.
When a item is checked in, it is inserted at the beginning of the list
(unshift). Koha::Objects are passed for patron, item and biblio to
improve the readability (and maintenance cost) of the code.

Test plan:
Generate several checkouts (see below)
Some should generate the transfer modal
Check them in
=> Without this patch notice that at some point there will be a problem in the list,
some checkins will be missing.
=> With this patch applied the checkin list always contain the last 20
checkins
(or whatever is the value of numReturnedItemsToShow, or "8" if pref is
empty, don't ask why 8!)

Code to generate 50 checkouts (all overdue):
```
use C4::Circulation qw( AddIssue );
use Koha::Items;
use Koha::Patrons;
use t::lib::Mocks;
use Koha::DateUtils qw(dt_from_string);
my $patron = Koha::Patrons->find(51);
t::lib::Mocks::mock_userenv({patron => $patron});
my $items = Koha::Items->search;
my $patron_unblessed = $patron->unblessed;
my $i;
while ( my $item = $items->next ) {
    my $due = dt_from_string->subtract(days => 45 + rand(45))->set_hour(23)->set_minute(59)->set_second(00);
    AddIssue($patron, $item->barcode, $due);
    last if ++$i >= 50;
}
```
Then retrieve the barcodes:
> SELECT items.barcode FROM issues LEFT JOIN items ON items.itemnumber=issues.itemnumber;

QA:
1. Do not be afraid by the length of this patch, most of the changes are
variable renames.
2. return_overdue is now only calculated once per checkin (per page
   load, could be improved)
3. date formatting is done template-side
Comment 2 Owen Leonard 2025-03-11 14:55:48 UTC
Created attachment 179169 [details] [review]
Bug 39302: Fix logic flaw in the display count of checkins

Checkins can disappear from the checkin list if transfer modal is
triggered.

There is a flaw in the logic of the checkin code: we keep 20 (default
value for numReturnedItemsToShow) checkins in the table, but there can
be shift in the "counter" logic when the transfer modal is triggered.

This patch is removing the different hashes and use a single @checkins
array to pass to the template.
When a item is checked in, it is inserted at the beginning of the list
(unshift). Koha::Objects are passed for patron, item and biblio to
improve the readability (and maintenance cost) of the code.

Test plan:
Generate several checkouts (see below)
Some should generate the transfer modal
Check them in
 => Without this patch notice that at some point there will be a problem
    in the list, some checkins will be missing.
 => With this patch applied the checkin list always contain the last 20
    checkins (or whatever is the value of numReturnedItemsToShow, or "8"
    if pref is empty, don't ask why 8!)

Code to generate 50 checkouts (all overdue):
```
use C4::Circulation qw( AddIssue );
use Koha::Items;
use Koha::Patrons;
use t::lib::Mocks;
use Koha::DateUtils qw(dt_from_string);
my $patron = Koha::Patrons->find(51);
t::lib::Mocks::mock_userenv({patron => $patron});
my $items = Koha::Items->search;
my $patron_unblessed = $patron->unblessed;
my $i;
while ( my $item = $items->next ) {
    my $due = dt_from_string->subtract(days => 45 + rand(45))->set_hour(23)->set_minute(59)->set_second(00);
    AddIssue($patron, $item->barcode, $due);
    last if ++$i >= 50;
}
```
Then retrieve the barcodes:
> SELECT items.barcode FROM issues LEFT JOIN items ON items.itemnumber=issues.itemnumber;

QA:
1. Do not be afraid by the length of this patch, most of the changes are
   variable renames.
2. return_overdue is now only calculated once per checkin (per page
   load, could be improved)
3. date formatting is done template-side

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Baptiste Wojtkowski (bwoj) 2025-03-12 10:23:00 UTC
I'm losing the transfer information I previously had in the array (see attachments).
Bug looks fixed otherwise
Comment 4 Baptiste Wojtkowski (bwoj) 2025-03-12 10:23:26 UTC
Created attachment 179197 [details]
Behaviour with patch
Comment 5 Baptiste Wojtkowski (bwoj) 2025-03-12 10:23:41 UTC
Created attachment 179198 [details]
Behaviour without patch
Comment 6 Jonathan Druart 2025-03-12 13:59:27 UTC
Created attachment 179207 [details] [review]
Bug 39302: Restore item's info
Comment 7 Jonathan Druart 2025-03-12 13:59:41 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #3)
> I'm losing the transfer information I previously had in the array (see
> attachments).
> Bug looks fixed otherwise

Oops, good catch. All item's info were actually missing!
Comment 8 Baptiste Wojtkowski (bwoj) 2025-03-12 15:44:52 UTC
Created attachment 179219 [details] [review]
Bug 39302: Fix logic flaw in the display count of checkins

Checkins can disappear from the checkin list if transfer modal is
triggered.

There is a flaw in the logic of the checkin code: we keep 20 (default
value for numReturnedItemsToShow) checkins in the table, but there can
be shift in the "counter" logic when the transfer modal is triggered.

This patch is removing the different hashes and use a single @checkins
array to pass to the template.
When a item is checked in, it is inserted at the beginning of the list
(unshift). Koha::Objects are passed for patron, item and biblio to
improve the readability (and maintenance cost) of the code.

Test plan:
Generate several checkouts (see below)
Some should generate the transfer modal
Check them in
 => Without this patch notice that at some point there will be a problem
    in the list, some checkins will be missing.
 => With this patch applied the checkin list always contain the last 20
    checkins (or whatever is the value of numReturnedItemsToShow, or "8"
    if pref is empty, don't ask why 8!)

Code to generate 50 checkouts (all overdue):
```
use C4::Circulation qw( AddIssue );
use Koha::Items;
use Koha::Patrons;
use t::lib::Mocks;
use Koha::DateUtils qw(dt_from_string);
my $patron = Koha::Patrons->find(51);
t::lib::Mocks::mock_userenv({patron => $patron});
my $items = Koha::Items->search;
my $patron_unblessed = $patron->unblessed;
my $i;
while ( my $item = $items->next ) {
    my $due = dt_from_string->subtract(days => 45 + rand(45))->set_hour(23)->set_minute(59)->set_second(00);
    AddIssue($patron, $item->barcode, $due);
    last if ++$i >= 50;
}
```
Then retrieve the barcodes:
> SELECT items.barcode FROM issues LEFT JOIN items ON items.itemnumber=issues.itemnumber;

QA:
1. Do not be afraid by the length of this patch, most of the changes are
   variable renames.
2. return_overdue is now only calculated once per checkin (per page
   load, could be improved)
3. date formatting is done template-side

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 9 Baptiste Wojtkowski (bwoj) 2025-03-12 15:44:55 UTC
Created attachment 179220 [details] [review]
Bug 39302: Restore item's info

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 10 Katrin Fischer 2025-03-13 09:30:24 UTC
Pushed for 25.05!

Well done everyone, thank you!