Bug 29464 - GET /acquisitions/orders doesn't honour sorting
Summary: GET /acquisitions/orders doesn't honour sorting
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major with 6 votes (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 20212
Blocks: 29570
  Show dependency treegraph
 
Reported: 2021-11-12 11:00 UTC by Nick Clemens
Modified: 2022-12-12 21:24 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.05.09,21.11.03


Attachments
Bug 29464: Regression tests (3.05 KB, patch)
2021-12-10 13:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29464: Make GET /acquisitions/orders honour sorting (1.79 KB, patch)
2021-12-10 13:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29464: Regression tests (3.10 KB, patch)
2021-12-10 14:27 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29464: Make GET /acquisitions/orders honour sorting (1.85 KB, patch)
2021-12-10 14:27 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29464: Regression tests (3.17 KB, patch)
2022-01-07 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29464: Make GET /acquisitions/orders honour sorting (1.92 KB, patch)
2022-01-07 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-11-12 11:00:27 UTC
This seems to be from bug 20212 - the sort options in the headers of the pending_orders table no longer function.

Additionally, the 'Summary' column is now marked as 'orderable: false' - it was possible to sort on this column before those patches
Comment 1 Katrin Fischer 2021-11-19 14:45:13 UTC
Confirmed the issue :(
Comment 2 Jonathan Druart 2021-11-25 13:11:00 UTC
*** Bug 29570 has been marked as a duplicate of this bug. ***
Comment 3 Tomás Cohen Arazi 2021-12-10 12:14:56 UTC
This was certainly working on bug 20212. Will try to check what else changed after it was pushed.
Comment 4 Tomás Cohen Arazi 2021-12-10 13:24:31 UTC
Created attachment 128395 [details] [review]
Bug 29464: Regression tests
Comment 5 Tomás Cohen Arazi 2021-12-10 13:24:36 UTC
Created attachment 128396 [details] [review]
Bug 29464: Make GET /acquisitions/orders honour sorting

When bug 20212 was pushed, we lost (didn't dig enough to find out how)
the sorting feature for this route.

This is mainly because biblio+biblioitems filtering prevents the route
to use plain $c->objects->search, so it has baked a custom version of
it. In this case, it missed the call to the dbic_merge_sorting helper,
that takes care of reading the query parameters and inject the sorting
attributes to the resultset.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail, sorting doesn't actually work
3. Apply this patch
4. Repeat 2
=> SUCCESS: Sorting works!
5. Test sorting the pending orders on parcel.tt
6. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 6 Tomás Cohen Arazi 2021-12-10 13:31:23 UTC
(In reply to Nick Clemens from comment #0)
> This seems to be from bug 20212 - the sort options in the headers of the
> pending_orders table no longer function.

Done.

> Additionally, the 'Summary' column is now marked as 'orderable: false' - it
> was possible to sort on this column before those patches

Will handle this on bug 29570.
Comment 7 Lucas Gass 2021-12-10 14:27:53 UTC
Created attachment 128397 [details] [review]
Bug 29464: Regression tests

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 8 Lucas Gass 2021-12-10 14:27:57 UTC
Created attachment 128398 [details] [review]
Bug 29464: Make GET /acquisitions/orders honour sorting

When bug 20212 was pushed, we lost (didn't dig enough to find out how)
the sorting feature for this route.

This is mainly because biblio+biblioitems filtering prevents the route
to use plain $c->objects->search, so it has baked a custom version of
it. In this case, it missed the call to the dbic_merge_sorting helper,
that takes care of reading the query parameters and inject the sorting
attributes to the resultset.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail, sorting doesn't actually work
3. Apply this patch
4. Repeat 2
=> SUCCESS: Sorting works!
5. Test sorting the pending orders on parcel.tt
6. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 9 Lucas Gass 2021-12-10 14:29:21 UTC
Tests pass and sorting works so I signed off. Making the Summary/title sortable again will happen in Bug 29570?
Comment 10 Tomás Cohen Arazi 2021-12-10 15:08:20 UTC
(In reply to Lucas Gass from comment #9)
> Tests pass and sorting works so I signed off. Making the Summary/title
> sortable again will happen in Bug 29570?

Yes, that's much more complex to solve.
Comment 11 Tomás Cohen Arazi 2022-01-03 12:02:01 UTC
*** Bug 29570 has been marked as a duplicate of this bug. ***
Comment 12 Catherine E. Ingram 2022-01-05 17:38:51 UTC
This is a bug that is costing my staff time every day. 
If we receive an order of 30 books and have to go into Acquisitions and click "received" on each title - it is much easy to find the titles when they are in alphabetical order. Otherwise it is a hunt and seek when the titles are listed in random order. So it might take anywhere from a few extra minutes for a small order to an extra 10 minutes for a larger order.
Thank you,
Catherine Ingram
Comment 13 Barbara Johnson 2022-01-05 18:43:57 UTC
+1 It has negatively impacted our workflow as well.
Comment 14 Jonathan Druart 2022-01-07 11:03:00 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> (In reply to Lucas Gass from comment #9)
> > Tests pass and sorting works so I signed off. Making the Summary/title
> > sortable again will happen in Bug 29570?
> 
> Yes, that's much more complex to solve.

(In reply to Tomás Cohen Arazi from comment #11)
> *** Bug 29570 has been marked as a duplicate of this bug. ***

Why is this marked as duplicate? We are not solving the problem for the summary column (and so the sort by title)
Comment 15 Jonathan Druart 2022-01-07 11:05:51 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Tomás Cohen Arazi from comment #10)
> > (In reply to Lucas Gass from comment #9)
> > > Tests pass and sorting works so I signed off. Making the Summary/title
> > > sortable again will happen in Bug 29570?
> > 
> > Yes, that's much more complex to solve.
> 
> (In reply to Tomás Cohen Arazi from comment #11)
> > *** Bug 29570 has been marked as a duplicate of this bug. ***
> 
> Why is this marked as duplicate? We are not solving the problem for the
> summary column (and so the sort by title)

duplicate status has been reverted, I missed that.
Comment 16 Jonathan Druart 2022-01-07 11:15:13 UTC
Created attachment 129136 [details] [review]
Bug 29464: Regression tests

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2022-01-07 11:15:20 UTC
Created attachment 129137 [details] [review]
Bug 29464: Make GET /acquisitions/orders honour sorting

When bug 20212 was pushed, we lost (didn't dig enough to find out how)
the sorting feature for this route.

This is mainly because biblio+biblioitems filtering prevents the route
to use plain $c->objects->search, so it has baked a custom version of
it. In this case, it missed the call to the dbic_merge_sorting helper,
that takes care of reading the query parameters and inject the sorting
attributes to the resultset.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail, sorting doesn't actually work
3. Apply this patch
4. Repeat 2
=> SUCCESS: Sorting works!
5. Test sorting the pending orders on parcel.tt
6. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Fridolin Somers 2022-01-19 07:20:46 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 19 Lucas Gass 2022-01-20 17:30:56 UTC
This is quite a pain point for many libraries, is there a chance to get this one backported?
Comment 20 Katrin Fischer 2022-01-20 20:19:41 UTC
Updating severity a little and also +1 for backporting :)
Comment 21 Kyle M Hall 2022-02-04 19:21:56 UTC
Pushed to 21.11.x for 21.11.03
Comment 22 Andrew Fuerste-Henry 2022-02-04 19:22:57 UTC
Pushed to 21.05.x for 21.05.09
Comment 23 Victor Grousset/tuxayo 2022-02-11 22:10:38 UTC
Missing dependencies for 20.11.x, it shouldn't be affected, no backport.