Bug 28864

Summary: The patron search results in the patron card creator doesn't seem to use PatronsPerPage syspref
Product: Koha Reporter: Michael Hafen <michael.hafen>
Component: Staff interfaceAssignee: Michael Hafen <michael.hafen>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, lucas, sally.healey, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,,22.05.06,21.11.12
Attachments: Proposed patch
Proposed patch
Bug 28864: Have patron card creator use PatronsPerPage
Bug 28864: Have patron search use PatronsPerPage better.
Bug 28864: Have patron search use PatronsPerPage better.
Bug 31120: Items will renew for zero ( 0 ) days if renewalperiod is blank/empty value
Bug 28864: Have patron search use PatronsPerPage better.
Bug 28864: Have patron card creator use PatronsPerPage
Bug 28864: Have patron card creator use PatronsPerPage

Description Michael Hafen 2021-08-13 22:50:25 UTC
The patron search results in the patron card creator card batch doesn't include the value of the PatronsPerPage system preference in the per page drop down, which means it isn't using the system preference.
Also, on the Patrons page, the patron search includes the PatronsPerPage value, but doesn't use it as the default.
Comment 1 Michael Hafen 2021-08-13 22:51:10 UTC
Created attachment 123848 [details] [review]
Proposed patch
Comment 2 Michael Hafen 2021-09-10 22:43:33 UTC
Created attachment 124787 [details] [review]
Proposed patch
Comment 3 Lucas Gass 2021-09-13 21:58:59 UTC
Micheal,

I'm happy to test this if you could add a test plan.
Comment 4 Michael Hafen 2021-09-14 16:21:49 UTC
The test plan is in the patch, but it isn't complete.
Here it is for you.

1.  Set the PatronsPerPage system preference to any integer other than 20.

2.  Go to the Patrons page and search for patrons.
3.  Observe that the per page drop down defaults to 20.

4.  Go to Tools -> Patron card creator.
5.  Start a new Card Batch.
6.  Click the 'Add patron(s)' button to open the patron search window.
7.  Do a search with more then 20 results.
8.  Observe that the per page drop down defaults to 20.
8b. If the PatronsPerPage was set to a value other than 10, 20, 50, or 100
      observe that that value is not in the per page drop down.
9.  Add patrons to the batch.
10. Close patron search window.
11. Click the 'Add patron(s)' button again to add the patrons to the batch.
      Observe that the per page defaults to 20.
12. Delete the card batch.

13. Apply patch.
14. Start a new card batch.  Click the 'Add patron(s)' button again to open the
      patron search window.
15. Do a search.
16. Observe that the per page drop down includes the value of PatronsPerPage
      and that value is selected by default.
17. Add patrons to batch and close the search window.
18. Click the 'Add patron(s)' button again and observe that the per page
      drop down includes and defaults to the PatronsPerPage value.
19. Delete the card batch.
17. Go back to the Patrons page and search for patrons.
18. Observe that the per page drop down defaults to the value of the
     PatronsPerPage system preference.
Comment 5 Michael Hafen 2022-04-26 23:55:11 UTC
Created attachment 134004 [details] [review]
Bug 28864: Have patron card creator use PatronsPerPage

Rebase after update to how patron search works elsewhere.

Add aLengthMenu, pageLength, and iDisplayLength to patroncards/edit-batch

Test plan:
1. Set the PatronsPerPage system preference to any integer other than 20.

2. Go to Tools -> Patron card creator.
3. Start a new Card Batch.
4. Click the 'Add patron(s)' button to open the patron search window.
5. Do a search.
6. Add more than 20 patrons to the batch and close the search window.
7. Observe that the per page drop down defaults to 20.
8. If the PatronsPerPage was set to a value other than 10, 20, 50, or 100
      observe that that value is not in the per page drop down.

9. Apply patch.
10. Reload the Patron Card Batch
11. Observe that the per page drop down includes the value of PatronsPerPage
     and that value is selected by default.
Comment 6 Sally 2022-07-25 09:45:25 UTC
I think this has since been fixed; I can't replicate the reported issue on the current branch or on 20.11.
Comment 7 Michael Hafen 2022-07-27 18:00:18 UTC
My test plan missed hitting the 'add patrons' button the second time.  The patron table in the search window that open the first time you hit the 'add patrons' button is fine, it honors the PatronsPerPage setting.  After adding the patrons to the batch though, the patron table that appears on the main windows does not honor the setting.
I just loaded the patron card creator on my workstation, which is running 22.05.3-1, and checked.  My PatronsPerPage setting is 50.  I created a batch with more than 50 patrons.  In the patrons table in the card batch after adding patrons to the batch, the default per page selected is 20, not 50 as it should be.
Comment 8 ByWater Sandboxes 2022-07-28 08:40:39 UTC
Created attachment 138194 [details] [review]
Bug 28864: Have patron search use PatronsPerPage better.

Add pageLength to members/member.tt
Add aLengthMenu and pageLength to
 patroncards/add_user_search (common/patron_search.tt).
Add aLengthMenu, pageLength, and iDisplayLength to patroncards/edit-batch

Test plan:
1. Set the PatronsPerPage system preference to any integer other than 20.

2. Go to the Patrons page and search for patrons.
3. Observe that the per page drop down defaults to 20.

4. Go to Tools -> Patron card creator.
5. Start a new Card Batch.
6. Click the 'Add patron(s)' button to open the patron search window.
7. Do a search.
8. Observe that the per page drop down defaults to 20.
8b.  If the PatronsPerPage was set to a value other than 10, 20, 50, or 100
      observe that that value is not in the per page drop down.
9. Close patron search window.

10. Apply patch.
11. Click the 'Add patron(s)' button again to open the patron search window.
12. Do a search.
13. Observe that the per page drop down includes the value of PatronsPerPage
     and that value is selected by default.

14. Go back to the Patrons page and search for patrons.
15. Observe that the per page drop down defaults to the value of the
     PatronsPerPage system preference.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 9 ByWater Sandboxes 2022-07-28 08:41:40 UTC
Created attachment 138195 [details] [review]
Bug 28864: Have patron search use PatronsPerPage better.

Add pageLength to members/member.tt
Add aLengthMenu and pageLength to
 patroncards/add_user_search (common/patron_search.tt).
Add aLengthMenu, pageLength, and iDisplayLength to patroncards/edit-batch

Test plan:
1. Set the PatronsPerPage system preference to any integer other than 20.

2. Go to the Patrons page and search for patrons.
3. Observe that the per page drop down defaults to 20.

4. Go to Tools -> Patron card creator.
5. Start a new Card Batch.
6. Click the 'Add patron(s)' button to open the patron search window.
7. Do a search.
8. Observe that the per page drop down defaults to 20.
8b.  If the PatronsPerPage was set to a value other than 10, 20, 50, or 100
      observe that that value is not in the per page drop down.
9. Close patron search window.

10. Apply patch.
11. Click the 'Add patron(s)' button again to open the patron search window.
12. Do a search.
13. Observe that the per page drop down includes the value of PatronsPerPage
     and that value is selected by default.

14. Go back to the Patrons page and search for patrons.
15. Observe that the per page drop down defaults to the value of the
     PatronsPerPage system preference.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 10 ByWater Sandboxes 2022-07-28 08:42:23 UTC
Created attachment 138196 [details] [review]
Bug 31120: Items will renew for zero ( 0 ) days if renewalperiod is blank/empty value

If the rule renewalperiod is the blank empty string instead of being null/undefined or non-existant, Koha will interpret the renewal period as being zero days instead of falling back to the issuelength rule.

It makes sense to me that a literal 0 here should make it renew for zero days even though that is nonsensical.

Test Plan:
1) Delete all your rules
2) Create an all/all/all rules with an empty string for renewal base
   period
3) Note that renewing an item does nothing
4) Apply this patch
5) Restart all the things!
6) Renew again
7) Note the renewal uses the issuelength rule as intended

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 11 ByWater Sandboxes 2022-07-28 08:42:26 UTC
Created attachment 138197 [details] [review]
Bug 28864: Have patron search use PatronsPerPage better.

Add pageLength to members/member.tt
Add aLengthMenu and pageLength to
 patroncards/add_user_search (common/patron_search.tt).
Add aLengthMenu, pageLength, and iDisplayLength to patroncards/edit-batch

Test plan:
1. Set the PatronsPerPage system preference to any integer other than 20.

2. Go to the Patrons page and search for patrons.
3. Observe that the per page drop down defaults to 20.

4. Go to Tools -> Patron card creator.
5. Start a new Card Batch.
6. Click the 'Add patron(s)' button to open the patron search window.
7. Do a search.
8. Observe that the per page drop down defaults to 20.
8b.  If the PatronsPerPage was set to a value other than 10, 20, 50, or 100
      observe that that value is not in the per page drop down.
9. Close patron search window.

10. Apply patch.
11. Click the 'Add patron(s)' button again to open the patron search window.
12. Do a search.
13. Observe that the per page drop down includes the value of PatronsPerPage
     and that value is selected by default.

14. Go back to the Patrons page and search for patrons.
15. Observe that the per page drop down defaults to the value of the
     PatronsPerPage system preference.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 12 Sally 2022-07-28 08:45:47 UTC
(In reply to Michael Hafen from comment #7)
> My test plan missed hitting the 'add patrons' button the second time.  

Great, thank you.

Patch works as described with the updated test plan - so I've signed off.

Unfortunately, the sandbox is playing havoc at the moment and won't sign off both of the patches.  After signing one off twice, it has now added a patch from a different bug - rather than signing off the two patches Michael added.  Nightmare.  :(

But the two patches from Michael both work here.
Comment 13 Victor Grousset/tuxayo 2022-07-28 20:11:59 UTC
Comment on attachment 138196 [details] [review]
Bug 31120: Items will renew for zero ( 0 ) days if renewalperiod is blank/empty value

A patch for Bug 31120 was attached by mistake here.
Comment 14 Katrin Fischer 2022-07-28 20:32:51 UTC
I have tested without applying the patch and this works nicely on master. I believe this might be because of the standardizing of patron search that has taken place since this patch was written.
Comment 15 Michael Hafen 2022-07-28 20:40:52 UTC
(In reply to Katrin Fischer from comment #14)
> I have tested without applying the patch and this works nicely on master. I
> believe this might be because of the standardizing of patron search that has
> taken place since this patch was written.

The test plan, since the standardizing of the patron search, is wrong.  This is still an issue on the edit-batch page.  The contents of the patch reflect this though I neglected to update the test plan.

Here is the new test plan:

Test plan:
1.  Set the PatronsPerPage system preference to any integer other than 20.

2.  Go to Tools -> Patron card creator.
3.  Start a new Card Batch.
4.  Click the 'Add patron(s)' button to open the patron search window.
5.  Do a search.
6.  Add more than 20 patrons to the batch.
7.  Close patron search window.
8.  Click the 'Add patron(s)' button to add the selected patrons to the batch.
9.  Observe that the table of patrons is has 20 as the default per page setting.

10. Apply patch.
11. Reload the batch.
12. Observe that the per page drop down includes the value of PatronsPerPage
     and that value is selected by default.
Comment 16 Victor Grousset/tuxayo 2022-07-28 20:59:19 UTC
The commits
Bug 28864: Have patron card creator use PatronsPerPage 
Bug 28864: Have patron search use PatronsPerPage better.
are duplicate, which the is the right one?

Thanks Michael for the correct test plan. Which patch name? With this, we will be able to cleanup this.

The name and test plan was changed on the 1st signoff via a sandbox. I don't know how is that possible to change a commit message via a sandbox.
Comment 17 Victor Grousset/tuxayo 2022-07-28 21:03:04 UTC
Email sent to Sally to ask if their testing matches the new test plan. To know if the signoff is still valid.
Comment 18 Michael Hafen 2022-07-28 21:36:07 UTC
(In reply to Victor Grousset/tuxayo from comment #16)
> The commits
> Bug 28864: Have patron card creator use PatronsPerPage 
> Bug 28864: Have patron search use PatronsPerPage better.
> are duplicate, which the is the right one?
> 
> Thanks Michael for the correct test plan. Which patch name? With this, we
> will be able to cleanup this.
> 
> The name and test plan was changed on the 1st signoff via a sandbox. I don't
> know how is that possible to change a commit message via a sandbox.

The line numbers are the only difference.  It seems that the commit "Bug 28864: Have patron search use PatronsPerPage better." is the more correct, looking at the line numbers (this is the only attachment that isn't marked obsolete); though I like the commit message of the other commit better, but if you can't change the commit message, then that's ok.
Comment 19 Sally 2022-07-29 07:43:28 UTC
(In reply to Victor Grousset/tuxayo from comment #17)
> Email sent to Sally to ask if their testing matches the new test plan. To
> know if the signoff is still valid.

Yes, it's valid. 

I used the new test plan and checked the bug was valid on master.

Then I used the new test plan against the patch, and it worked perfectly - so that's why I added the sign off.  I'll change the status back.
Comment 20 Katrin Fischer 2022-07-29 08:21:52 UTC
Michael, please don't forget to assign yourself!
Comment 21 Victor Grousset/tuxayo 2022-07-29 22:10:11 UTC
Created attachment 138379 [details] [review]
Bug 28864: Have patron card creator use PatronsPerPage

Add pageLength to members/member.tt
Add aLengthMenu and pageLength to
 patroncards/add_user_search (common/patron_search.tt).
Add aLengthMenu, pageLength, and iDisplayLength to patroncards/edit-batch

Test plan:
1.  Set the PatronsPerPage system preference to any integer other than 20.

2.  Go to Tools -> Patron card creator.
3.  Start a new Card Batch.
4.  Click the 'Add patron(s)' button to open the patron search window.
5.  Do a search.
6.  Add more than 20 patrons to the batch.
7.  Close patron search window.
8.  Click the 'Add patron(s)' button to add the selected patrons to the batch.
9.  Observe that the table of patrons is has 20 as the default per page setting.

10. Apply patch.
11. Reload the batch.
12. Observe that the per page drop down includes the value of PatronsPerPage
     and that value is selected by default.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 22 Victor Grousset/tuxayo 2022-07-29 22:13:15 UTC
(In reply to Michael Hafen from comment #18)
> though I like the commit message of the other commit better, but
> if you can't change the commit message, then that's ok.

Ok I changed to "Bug 28864: Have patron card creator use PatronsPerPage"
and added test plan from Comment 15
Comment 23 Katrin Fischer 2022-07-30 14:59:38 UTC
Thanks for clearing that up!
Comment 24 Katrin Fischer 2022-07-30 15:00:22 UTC
Created attachment 138388 [details] [review]
Bug 28864: Have patron card creator use PatronsPerPage

Add pageLength to members/member.tt
Add aLengthMenu and pageLength to
 patroncards/add_user_search (common/patron_search.tt).
Add aLengthMenu, pageLength, and iDisplayLength to patroncards/edit-batch

Test plan:
1.  Set the PatronsPerPage system preference to any integer other than 20.

2.  Go to Tools -> Patron card creator.
3.  Start a new Card Batch.
4.  Click the 'Add patron(s)' button to open the patron search window.
5.  Do a search.
6.  Add more than 20 patrons to the batch.
7.  Close patron search window.
8.  Click the 'Add patron(s)' button to add the selected patrons to the batch.
9.  Observe that the table of patrons is has 20 as the default per page setting.

10. Apply patch.
11. Reload the batch.
12. Observe that the per page drop down includes the value of PatronsPerPage
     and that value is selected by default.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Tomás Cohen Arazi 2022-08-01 13:02:53 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 26 Lucas Gass 2022-09-29 15:37:10 UTC
Backported to 22.05.x for 22.05.06
Comment 27 Victor Grousset/tuxayo 2022-10-13 16:08:49 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 28 Arthur Suzuki 2022-10-22 22:07:21 UTC
Thanks!

Pushed to 21.11 for 21.11.12