Bug 34068 - Dropdown selector when changing pickup library should not depend on RESTdefaultPageSize
Summary: Dropdown selector when changing pickup library should not depend on RESTdefau...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Pedro Amorim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-20 14:54 UTC by Laura Escamilla
Modified: 2025-01-28 01:35 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the dropdown list for holds pickup locations in the staff interface - it now shows the complete list of libraries. Previously, in some circumstances, it was not showing the complete list of pickup locations (for example, with RESTdefaultPageSize = 5 and AllowHoldPolicyOverride = Allow, it would only show the final page of libraries instead of the full list of libraries).
Version(s) released in:
25.05.00
Circulation function: holds


Attachments
Buggy dropdown selector (15.89 KB, image/png)
2023-06-20 14:54 UTC, Laura Escamilla
Details
Bug 34068: Fix pagination for hold pickup locations (1.84 KB, patch)
2024-09-26 12:45 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34068: Fix pagination for hold pickup locations (1.90 KB, patch)
2024-09-26 13:08 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 34068: Fix pagination for hold pickup locations (1.97 KB, patch)
2024-09-26 13:17 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34068: Add tests (7.11 KB, patch)
2024-11-06 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34068: Fix tests (1.94 KB, patch)
2024-11-06 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34068: Mock RESTdefaultPageSize (978 bytes, patch)
2024-11-07 09:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34068: Fix pagination for hold pickup locations (2.00 KB, patch)
2025-01-03 01:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 34068: Add tests (7.15 KB, patch)
2025-01-03 01:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 34068: Fix tests (1.98 KB, patch)
2025-01-03 01:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 34068: Mock RESTdefaultPageSize (1.00 KB, patch)
2025-01-03 01:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 34068: Fix pagination for hold pickup locations (2.06 KB, patch)
2025-01-10 13:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34068: Add tests (7.21 KB, patch)
2025-01-10 13:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34068: Fix tests (2.04 KB, patch)
2025-01-10 13:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34068: Mock RESTdefaultPageSize (1.06 KB, patch)
2025-01-10 13:58 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Laura Escamilla 2023-06-20 14:54:30 UTC
Created attachment 152486 [details]
Buggy dropdown selector

To recreate:
1. Have a library that has more than 30 pickup branches. 
2. Place a hold on an item. 
3. Go to the patron account and try to change the pickup location. The dropdown selector will keep repeating the last 10 libraries.
Comment 1 Mikko Liimatainen 2023-06-28 08:17:54 UTC
Had the same issue with 60 pickup branches where the last 20 libraries kept repeating. Turns out this was caused by RESTdefaultPageSize being set to 20. Increasing the setting to 80 solved the problem.
Comment 2 Laura Escamilla 2023-06-28 12:57:46 UTC
Mikko's answer solved the issue. Thanks!
Comment 3 Katrin Fischer 2023-06-30 19:56:45 UTC
It does solve it, but I think the dependency is not good - we should keep this open and look into changing the behaviour.
Comment 4 Lucas Gass (lukeg) 2023-07-12 15:11:18 UTC
+1
Comment 5 Matt Blenkinsop 2024-09-26 12:45:44 UTC
Created attachment 171964 [details] [review]
Bug 34068: Fix pagination for hold pickup locations

This patch addresses an issue where pagination filters are set incorrectly for the pickup locations endpoint. Only the final page is returned rather than the expected full list of results

Test plan:
1) In KTD, set the syspref RESTdefaultPageSize to 5
2) Set AllowHoldPolicyOverride to Allow
3) Create a hold on an item for a patron
4) Navigate to the patron's page and click on the holds tab
5) Click the dropdown in pickup locations
6) There will be two results, repeated three times
7) Apply patch and restart_all
8) Repeat step 5, there should now be a full list of 12 libraries
Comment 6 Laura Escamilla 2024-09-26 13:08:24 UTC
Created attachment 171969 [details] [review]
Bug 34068: Fix pagination for hold pickup locations

This patch addresses an issue where pagination filters are set incorrectly for the pickup locations endpoint. Only the final page is returned rather than the expected full list of results

Test plan:
1) In KTD, set the syspref RESTdefaultPageSize to 5
2) Set AllowHoldPolicyOverride to Allow
3) Create a hold on an item for a patron
4) Navigate to the patron's page and click on the holds tab
5) Click the dropdown in pickup locations
6) There will be two results, repeated three times
7) Apply patch and restart_all
8) Repeat step 5, there should now be a full list of 12 libraries

Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Comment 7 Matt Blenkinsop 2024-09-26 13:17:12 UTC
Created attachment 171970 [details] [review]
Bug 34068: Fix pagination for hold pickup locations

This patch addresses an issue where pagination filters are set incorrectly for the pickup locations endpoint. Only the final page is returned rather than the expected full list of results

Test plan:
1) In KTD, set the syspref RESTdefaultPageSize to 5
2) Set AllowHoldPolicyOverride to Allow
3) Create a hold on an item for a patron
4) Navigate to the patron's page and click on the holds tab
5) Click the dropdown in pickup locations
6) There will be two results, repeated three times
7) Apply patch and restart_all
8) Repeat step 5, there should now be a full list of 12 libraries

Co-authored-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Comment 8 Jonathan Druart 2024-11-06 11:00:23 UTC
Created attachment 174071 [details] [review]
Bug 34068: Add tests
Comment 9 Jonathan Druart 2024-11-06 11:00:26 UTC
Created attachment 174072 [details] [review]
Bug 34068: Fix tests

Several wrong things in the previous attempt:
* as_list returns an array
* as_list returns objects, ->{library_id} does not exist
* as_list does not returns api object, you need to call ->branchcode to
  get the id
* $pickup_locations defined for both situations was confusing
Comment 10 Jonathan Druart 2024-11-06 11:01:10 UTC
Please retest with and without AllowHoldPolicyOverride.
Comment 11 Lucas Gass (lukeg) 2024-11-06 17:12:25 UTC
Tests are failing here for me now, not always, but it seems that if I set  RESTdefaultPageSize to something other than 20 they fail:



    #   Failed test 'exact match for JSON Pointer ""'
    #   at /kohadevbox/koha/t/db_dependent/api/v1/holds.t line 908.
    #     Structures begin differing at:
    #          $got->[5] = Does not exist
    #     $expected->[5] = HASH(0x5ed8c2627ed8)
    ok 13 - Patron passed correctly
    ok 14 - GET //tomasito:thePassword123@/api/v1/holds/157/pickup_locations?_order_by=marc_org_code&_page=2
    not ok 15 - exact match for JSON Pointer ""

    #   Failed test 'exact match for JSON Pointer ""'
    #   at /kohadevbox/koha/t/db_dependent/api/v1/holds.t line 912.
    #     Structures begin differing at:
    #          $got->[0]{address3} = 'gftd3B_'
    #     $expected->[0]{address3} = 'YtBCoGC'
    # Looks like you failed 2 tests of 15.
Comment 12 Jonathan Druart 2024-11-07 09:03:44 UTC
Created attachment 174106 [details] [review]
Bug 34068: Mock RESTdefaultPageSize
Comment 13 David Nind 2025-01-03 01:50:57 UTC
Created attachment 176110 [details] [review]
Bug 34068: Fix pagination for hold pickup locations

This patch addresses an issue where pagination filters are set incorrectly for the pickup locations endpoint. Only the final page is returned rather than the expected full list of results

Test plan:
1) In KTD, set the syspref RESTdefaultPageSize to 5
2) Set AllowHoldPolicyOverride to Allow
3) Create a hold on an item for a patron
4) Navigate to the patron's page and click on the holds tab
5) Click the dropdown in pickup locations
6) There will be two results, repeated three times
7) Apply patch and restart_all
8) Repeat step 5, there should now be a full list of 12 libraries

Co-authored-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2025-01-03 01:51:01 UTC
Created attachment 176111 [details] [review]
Bug 34068: Add tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2025-01-03 01:51:04 UTC
Created attachment 176112 [details] [review]
Bug 34068: Fix tests

Several wrong things in the previous attempt:
* as_list returns an array
* as_list returns objects, ->{library_id} does not exist
* as_list does not returns api object, you need to call ->branchcode to
  get the id
* $pickup_locations defined for both situations was confusing

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2025-01-03 01:51:07 UTC
Created attachment 176113 [details] [review]
Bug 34068: Mock RESTdefaultPageSize

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 David Nind 2025-01-03 01:52:16 UTC
I've changed this to a normal bug, instead of an enhancement.

I'm not sure who the assignee should be 8-), so haven't updated that.

The tests pass: prove t/db_dependent/api/v1/holds.t
Comment 18 Pedro Amorim 2025-01-10 13:58:24 UTC
Created attachment 176331 [details] [review]
Bug 34068: Fix pagination for hold pickup locations

This patch addresses an issue where pagination filters are set incorrectly for the pickup locations endpoint. Only the final page is returned rather than the expected full list of results

Test plan:
1) In KTD, set the syspref RESTdefaultPageSize to 5
2) Set AllowHoldPolicyOverride to Allow
3) Create a hold on an item for a patron
4) Navigate to the patron's page and click on the holds tab
5) Click the dropdown in pickup locations
6) There will be two results, repeated three times
7) Apply patch and restart_all
8) Repeat step 5, there should now be a full list of 12 libraries

Co-authored-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 19 Pedro Amorim 2025-01-10 13:58:27 UTC
Created attachment 176332 [details] [review]
Bug 34068: Add tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 20 Pedro Amorim 2025-01-10 13:58:31 UTC
Created attachment 176333 [details] [review]
Bug 34068: Fix tests

Several wrong things in the previous attempt:
* as_list returns an array
* as_list returns objects, ->{library_id} does not exist
* as_list does not returns api object, you need to call ->branchcode to
  get the id
* $pickup_locations defined for both situations was confusing

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 21 Pedro Amorim 2025-01-10 13:58:34 UTC
Created attachment 176334 [details] [review]
Bug 34068: Mock RESTdefaultPageSize

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 22 Pedro Amorim 2025-01-10 13:59:58 UTC
Thanks for improving upon the initial patch, Joubu, and all the additional work here.
Comment 23 Katrin Fischer 2025-01-10 18:20:42 UTC
Pushed for 25.05!

Well done everyone, thank you!