Bug 30133

Summary: Pagination broken on pickup_locations routes when AllowHoldPolicyOverride=1
Product: Koha Reporter: Nick Clemens <nick>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: andrewfh, katrin.fischer, kyle, martin.renvoize, tomascohen, victor
Version: Main   
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.05.00,21.11.03,21.05.13
Attachments: Bug 30133: Avoid calling c->objects->search twice
Bug 30133: Avoid calling c->objects->search twice
Bug 30133: Avoid calling c->objects->search twice
Bug 30133: Regression tests
Bug 30133: Avoid calling c->objects->search twice
Bug 30133: Simplify code
Bug 30133: Regression tests
Bug 30133: Avoid calling c->objects->search twice
Bug 30133: Simplify code
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations
Bug 30133: Regression tests
Bug 30133: Avoid calling c->objects->search twice
Bug 30133: Simplify code
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations
Bug 30133: Regression tests
Bug 30133: Avoid calling c->objects->search twice
Bug 30133: Simplify code
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations
Bug 30133: (follow-up) Update objects.search POD

Description Nick Clemens 2022-02-18 14:24:51 UTC
To recreate:
1 - Set AllowHoldPolicyOverride to allow
2 - Set RESTdefaultPageSize = 5
3 - Try to place a hold
4 - Note list is truncated and marked as needing override
Comment 1 Nick Clemens 2022-02-18 14:26:57 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2022-02-18 14:29:16 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2022-02-18 14:31:40 UTC
Created attachment 130855 [details] [review]
Bug 30133: Avoid calling c->objects->search twice

Calling this twice sets double pagination headers, which messes up the page for the requests and
retrieves on ly the end of the list

This patch simply avoids a doulbe call

TO DO: must implement same for Biblio->pickup_locations and cover with tests
this also returns all as needing override
Comment 4 Nick Clemens 2022-02-18 15:27:26 UTC
*** Bug 30126 has been marked as a duplicate of this bug. ***
Comment 5 Tomás Cohen Arazi 2022-02-18 18:59:27 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2022-02-18 18:59:31 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2022-02-18 18:59:36 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2022-02-18 19:11:44 UTC
Created attachment 130886 [details] [review]
Bug 30133: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2022-02-18 19:11:48 UTC
Created attachment 130887 [details] [review]
Bug 30133: Avoid calling c->objects->search twice

Calling this twice sets double pagination headers, which messes up the page for the requests and
retrieves on ly the end of the list

This patch simply avoids a doulbe call

TO DO: must implement same for Biblio->pickup_locations and cover with tests
this also returns all as needing override

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2022-02-18 19:11:52 UTC
Created attachment 130888 [details] [review]
Bug 30133: Simplify code

This patch simplifies the code, and also fixes a problem with branchcode
comparisson failing, highlighted when running the tests

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/items.t
=> FAIL: Tests fail. Notably:
- Link header pagination is wrong
- X-*Total-Count headers have two values (dupicated)
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 11 Tomás Cohen Arazi 2022-02-18 19:11:57 UTC
Created attachment 130889 [details] [review]
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations

This patch does the same thing we did for items on the previous patch,
but for biblios. The tests are almost the same as well.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Andrew Fuerste-Henry 2022-02-18 19:20:50 UTC
Created attachment 130890 [details] [review]
Bug 30133: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=30113
Comment 13 Andrew Fuerste-Henry 2022-02-18 19:20:54 UTC
Created attachment 130891 [details] [review]
Bug 30133: Avoid calling c->objects->search twice

Calling this twice sets double pagination headers, which messes up the page for the requests and
retrieves on ly the end of the list

This patch simply avoids a doulbe call

TO DO: must implement same for Biblio->pickup_locations and cover with tests
this also returns all as needing override

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=30113
Comment 14 Andrew Fuerste-Henry 2022-02-18 19:20:58 UTC
Created attachment 130892 [details] [review]
Bug 30133: Simplify code

This patch simplifies the code, and also fixes a problem with branchcode
comparisson failing, highlighted when running the tests

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/items.t
=> FAIL: Tests fail. Notably:
- Link header pagination is wrong
- X-*Total-Count headers have two values (dupicated)
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=30113
Comment 15 Andrew Fuerste-Henry 2022-02-18 19:21:02 UTC
Created attachment 130893 [details] [review]
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations

This patch does the same thing we did for items on the previous patch,
but for biblios. The tests are almost the same as well.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=30113
Comment 16 Jonathan Druart 2022-02-21 11:51:42 UTC
Created attachment 130953 [details] [review]
Bug 30133: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2022-02-21 11:51:47 UTC
Created attachment 130954 [details] [review]
Bug 30133: Avoid calling c->objects->search twice

Calling this twice sets double pagination headers, which messes up the page for the requests and
retrieves on ly the end of the list

This patch simply avoids a doulbe call

TO DO: must implement same for Biblio->pickup_locations and cover with tests
this also returns all as needing override

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2022-02-21 11:51:52 UTC
Created attachment 130955 [details] [review]
Bug 30133: Simplify code

This patch simplifies the code, and also fixes a problem with branchcode
comparisson failing, highlighted when running the tests

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/items.t
=> FAIL: Tests fail. Notably:
- Link header pagination is wrong
- X-*Total-Count headers have two values (dupicated)
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2022-02-21 11:51:56 UTC
Created attachment 130956 [details] [review]
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations

This patch does the same thing we did for items on the previous patch,
but for biblios. The tests are almost the same as well.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/biblios.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Tomás Cohen Arazi 2022-02-21 12:00:01 UTC
Created attachment 130957 [details] [review]
Bug 30133: (follow-up) Update objects.search POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Fridolin Somers 2022-02-25 00:36:50 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 22 Tomás Cohen Arazi 2022-02-25 00:39:08 UTC
(In reply to Fridolin Somers from comment #21)
> Pushed to master for 22.05, thanks to everybody involved 🦄

\o/
Comment 23 Kyle M Hall 2022-02-25 15:43:08 UTC
Pushed to 21.11.x for 21.11.03
Comment 24 Andrew Fuerste-Henry 2022-03-20 14:43:22 UTC
Pushed to 21.05.x for 21.05.13
Comment 25 Victor Grousset/tuxayo 2022-03-21 00:02:49 UTC
About backporting to 20.11.x, this depend on bug 27931, right? 20.11.x doesn't seem to have the route used in the tests so it shouldn't be affected at all.
Comment 26 Tomás Cohen Arazi 2022-03-21 11:59:12 UTC
(In reply to Victor Grousset/tuxayo from comment #25)
> About backporting to 20.11.x, this depend on bug 27931, right? 20.11.x
> doesn't seem to have the route used in the tests so it shouldn't be affected
> at all.

That's right!