Bug 30802 - numReturnedItemsToShow doesn't show more than 20 items on the return screen
Summary: numReturnedItemsToShow doesn't show more than 20 items on the return screen
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Shi Yao Wang
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-18 22:13 UTC by Caroline Cyr La Rose
Modified: 2023-06-08 22:26 UTC (History)
9 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:
22.11.00


Attachments
Bug 30802: Make numReturnedItemsToShow function properly (2.20 KB, patch)
2022-05-19 15:18 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30802: Make numReturnedItemsToShow function properly (2.26 KB, patch)
2022-09-11 12:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30802: Make numReturnedItemsToShow function properly (2.26 KB, patch)
2022-09-11 12:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30802: Make numReturnedItemsToShow function properly (2.36 KB, patch)
2022-09-13 13:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30802: (QA follow-up) Simplify a ternary (1.07 KB, patch)
2022-09-13 13:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2022-05-18 22:13:30 UTC
The system preference numReturnedItemsToShow says "show the XX last returned items on the checkin screen". The default value is 20. If you put a value < 20, it affects the returns.pl table. But if you put a value > 20 in the syspref, the table on returns.pl shows a maximum of 21 items.

To test:
1- Make lots of checkouts, at least like 40 (I used the batchCheckouts feature)
2- Go to Circulation > Check in
3- Return 21 items
   The last 20 items returned will be displayed 
4- Change numReturnedItemsToShow to 50
5- Return a couple more items
   Only the last 21 returned items are displayed
6- Change numReturnedItemsToShow to 10
7- Return a couple more items
   Only the last 10 returned items are displayed
Comment 1 Jonathan Druart 2022-05-19 05:03:55 UTC
Indeed, there is a 20 hardcoded somewhere else...

A possible fix could be

diff --git a/circ/returns.pl b/circ/returns.pl
index e2d2317a37a..ee874864d05 100755
--- a/circ/returns.pl
+++ b/circ/returns.pl
@@ -94,6 +94,7 @@ my $userenv_branch = $userenv->{'branch'} // '';
 my $forgivemanualholdsexpire = $query->param('forgivemanualholdsexpire');
 
 my $overduecharges = (C4::Context->preference('finesMode') && C4::Context->preference('finesMode') eq 'production');
+my $numReturnedItemsToShow = ( C4::Context->preference('numReturnedItemsToShow') ) ? C4::Context->preference('numReturnedItemsToShow') : 8;
 
 # Set up the item stack ....
 my %returneditems;
@@ -104,7 +105,7 @@ foreach ( $query->param ) {
     my $counter;
     if (/ri-(\d*)/) {
         $counter = $1;
-        if ($counter > 20) {
+        if ($counter > $numReturnedItemsToShow) {
             next;
         }
     }
@@ -645,14 +646,13 @@ foreach my $code ( keys %$messages ) {
 $template->param( errmsgloop => \@errmsgloop );
 
 #set up so only the last 8 returned items display (make for faster loading pages)
-my $returned_counter = ( C4::Context->preference('numReturnedItemsToShow') ) ? C4::Context->preference('numReturnedItemsToShow') : 8;
 my $count = 0;
 my @riloop;
 my $shelflocations =
   { map { $_->{authorised_value} => $_->{lib} } Koha::AuthorisedValues->get_descriptions_by_koha_field( { frameworkcode => '', kohafield => 'items.location' } ) };
 foreach ( sort { $a <=> $b } keys %returneditems ) {
     my %ri;
-    if ( $count++ < $returned_counter ) {
+    if ( $count++ < $numReturnedItemsToShow ) {
         my $bar_code = $returneditems{$_};
         if ($riduedate{$_}) {
             my $duedate = dt_from_string( $riduedate{$_}, 'sql');
Comment 2 Caroline Cyr La Rose 2022-05-19 12:23:34 UTC
I tested in a sandbox in case it was my installation that was acting up, and the sandbox is limited to 14, completely ignoring the value in numReturnedItemsToShow (which was set to 20 by default). Even after I changed the pref to 50, the returns.pl page only shows 14.
Comment 3 Shi Yao Wang 2022-05-19 15:18:17 UTC
Created attachment 135206 [details] [review]
Bug 30802: Make numReturnedItemsToShow function properly

Replaced the hardcoded 20 by the value of numReturnedItemsToShow to
display the right number of items in check in.

Test plan:
1-  Make lots of checkouts, at least like 40 (I used the batchCheckouts feature)
2-  Go to Circulation > Check in
3-  Return 21 items
    The last 20 items returned will be displayed
4-  Change numReturnedItemsToShow to 50
5-  Return a couple more items
    Only the last 20 returned items are displayed
6-  Change numReturnedItemsToShow to 10
7-  Return a couple more items
    Only the last 10 returned items are displayed
8-  Apply the patch
9-  Change numReturnedItemsToShow back to 20
10- Do steps 1 to 7 again, but this time step 5 returns the right amount
of items
Comment 4 Owen Leonard 2022-05-20 17:00:57 UTC
With  numReturnedItemsToShow set to 50 I seem to be limited to seeing 36 checkins.
Comment 5 Shi Yao Wang 2022-05-20 20:38:39 UTC
(In reply to Owen Leonard from comment #4)
> With  numReturnedItemsToShow set to 50 I seem to be limited to seeing 36
> checkins.

I tested again and set numReturnedItemsToShow to 50 and checked in 51 items and it shows 50 checkins for me.

Can someone else confirm the behaviour?

Putting this back to Needs Signoff for others to test.
Comment 6 Sally 2022-07-20 10:06:52 UTC
(In reply to Caroline Cyr La Rose from comment #2)
> I tested in a sandbox in case it was my installation that was acting up, and
> the sandbox is limited to 14, completely ignoring the value in
> numReturnedItemsToShow (which was set to 20 by default). Even after I
> changed the pref to 50, the returns.pl page only shows 14.

I found the same whilst testing on a sandbox.
Comment 7 Shi Yao Wang 2022-09-09 17:20:36 UTC
I do not see this problem on my local koha installation.

For those who tested on a sandbox, does it display a random number of returned items before the patch?
For example: only displays 14 items of the 25 items returned when the default limit is 20.

If so, then it is not caused by this patch and is probably an issue within the sandboxes' setup. (Which I think is really weird because there is no reason why it should be limited to 14 or 36 displayed checkins unless specified in syspref config or hardcoded like the 20 mentioned in Comment 1. Also, 14 + 36 = 50. Coincidence?)
Comment 8 Katrin Fischer 2022-09-11 12:49:38 UTC
Hm this is a bit of an odd one:

What I did:
* Created items with barcodes 1-30 using the multi-add feature
* Checked all of them out to one patron using the batch checkout
* Set numReturnedItemsToShow to 23
* First item vanished when I checked in number 20?
* Not ok...

... trying with the patch next.

And it works! The change also looks logical to me. Signing off.

I wonder if returns of items not checked out or similar could throw the counter in testing?
Comment 9 Katrin Fischer 2022-09-11 12:52:29 UTC
Created attachment 140392 [details] [review]
Bug 30802: Make numReturnedItemsToShow function properly

Replaced the hardcoded 20 by the value of numReturnedItemsToShow to
display the right number of items in check in.

Test plan:
1-  Make lots of checkouts, at least like 40 (I used the batchCheckouts feature)
2-  Go to Circulation > Check in
3-  Return 21 items
    The last 20 items returned will be displayed
4-  Change numReturnedItemsToShow to 50
5-  Return a couple more items
    Only the last 20 returned items are displayed
6-  Change numReturnedItemsToShow to 10
7-  Return a couple more items
    Only the last 10 returned items are displayed
8-  Apply the patch
9-  Change numReturnedItemsToShow back to 20
10- Do steps 1 to 7 again, but this time step 5 returns the right amount
of items

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2022-09-11 12:56:17 UTC
Created attachment 140393 [details] [review]
Bug 30802: Make numReturnedItemsToShow function properly

Replaced the hardcoded 20 by the value of numReturnedItemsToShow to
display the right number of items in check in.

Test plan:
1-  Make lots of checkouts, at least like 40 (I used the batchCheckouts feature)
2-  Go to Circulation > Check in
3-  Return 21 items
    The last 20 items returned will be displayed
4-  Change numReturnedItemsToShow to 50
5-  Return a couple more items
    Only the last 20 returned items are displayed
6-  Change numReturnedItemsToShow to 10
7-  Return a couple more items
    Only the last 10 returned items are displayed
8-  Apply the patch
9-  Change numReturnedItemsToShow back to 20
10- Do steps 1 to 7 again, but this time step 5 returns the right amount
of items

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Marcel de Rooy 2022-09-13 13:27:42 UTC
Created attachment 140573 [details] [review]
Bug 30802: Make numReturnedItemsToShow function properly

Replaced the hardcoded 20 by the value of numReturnedItemsToShow to
display the right number of items in check in.

Test plan:
1-  Make lots of checkouts, at least like 40 (I used the batchCheckouts feature)
2-  Go to Circulation > Check in
3-  Return 21 items
    The last 20 items returned will be displayed
4-  Change numReturnedItemsToShow to 50
5-  Return a couple more items
    Only the last 20 returned items are displayed
6-  Change numReturnedItemsToShow to 10
7-  Return a couple more items
    Only the last 10 returned items are displayed
8-  Apply the patch
9-  Change numReturnedItemsToShow back to 20
10- Do steps 1 to 7 again, but this time step 5 returns the right amount
of items

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2022-09-13 13:27:47 UTC
Created attachment 140574 [details] [review]
Bug 30802: (QA follow-up) Simplify a ternary

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2022-09-13 13:29:11 UTC
Jay. My first qa from Debian 11 master container ;) With a working git-bz still needing python2..
Comment 14 Tomás Cohen Arazi 2022-09-23 12:32:29 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 15 Lucas Gass 2022-11-01 22:07:30 UTC
Enhancement will not be backported to 22.05.x series