Bug 30133 - Pagination broken on pickup_locations routes when AllowHoldPolicyOverride=1
Summary: Pagination broken on pickup_locations routes when AllowHoldPolicyOverride=1
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
: 30126 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-02-18 14:24 UTC by Nick Clemens
Modified: 2022-12-12 21:24 UTC (History)
6 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.05.00,21.11.03,21.05.13


Attachments
Bug 30133: Avoid calling c->objects->search twice (1.87 KB, patch)
2022-02-18 14:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30133: Avoid calling c->objects->search twice (1.69 KB, patch)
2022-02-18 14:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30133: Avoid calling c->objects->search twice (1.74 KB, patch)
2022-02-18 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30133: Regression tests (3.78 KB, patch)
2022-02-18 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30133: Avoid calling c->objects->search twice (1.79 KB, patch)
2022-02-18 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30133: Simplify code (2.25 KB, patch)
2022-02-18 18:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30133: Regression tests (3.78 KB, patch)
2022-02-18 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30133: Avoid calling c->objects->search twice (1.79 KB, patch)
2022-02-18 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30133: Simplify code (2.23 KB, patch)
2022-02-18 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations (5.76 KB, patch)
2022-02-18 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30133: Regression tests (3.90 KB, patch)
2022-02-18 19:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30133: Avoid calling c->objects->search twice (1.91 KB, patch)
2022-02-18 19:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30133: Simplify code (2.35 KB, patch)
2022-02-18 19:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations (5.88 KB, patch)
2022-02-18 19:21 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 30133: Regression tests (3.92 KB, patch)
2022-02-21 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30133: Avoid calling c->objects->search twice (1.93 KB, patch)
2022-02-21 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30133: Simplify code (2.37 KB, patch)
2022-02-21 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30133: Fix pagination in /biblios/:bibio_id/pickup_locations (5.90 KB, patch)
2022-02-21 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30133: (follow-up) Update objects.search POD (875 bytes, patch)
2022-02-21 12:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!