Bug 29570 - Unable to sort summary column of pending_orders table on parcel.pl by summary column
Summary: Unable to sort summary column of pending_orders table on parcel.pl by summary...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 20212 29464
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-24 15:02 UTC by Lucas Gass
Modified: 2022-02-21 08:58 UTC (History)
7 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.11


Attachments
Bug 29570: Regression tests (1.91 KB, patch)
2022-02-02 20:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29570: Fix sorting orders by biblioitems columns (2.13 KB, patch)
2022-02-02 20:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29570: Enable sorting orders by summary (2.10 KB, patch)
2022-02-02 20:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29570: Regression tests (1.96 KB, patch)
2022-02-02 20:51 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29570: Fix sorting orders by biblioitems columns (2.18 KB, patch)
2022-02-02 20:51 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29570: Enable sorting orders by summary (2.16 KB, patch)
2022-02-02 20:51 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 29570: Regression tests (2.04 KB, patch)
2022-02-08 16:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29570: Fix sorting orders by biblioitems columns (2.25 KB, patch)
2022-02-08 16:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29570: Enable sorting orders by summary (2.23 KB, patch)
2022-02-08 16:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29570: Simplify for loop (1.19 KB, patch)
2022-02-08 16:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29570: Simplify for loop (1.24 KB, patch)
2022-02-08 18:02 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 Lucas Gass 2021-11-24 15:02:42 UTC
Have a pending order and go to:

/cgi-bin/koha/acqui/parcel.pl?invoiceid=X

Most of the columns you are able to sort but they summary column you cannot, it has the 'sorting_disabled' class.

Librarians with large orders need to be able to sort by Summary.
Comment 1 Jonathan Druart 2021-11-25 13:11:00 UTC

*** This bug has been marked as a duplicate of bug 29464 ***
Comment 2 Tomás Cohen Arazi 2022-01-03 12:02:01 UTC

*** This bug has been marked as a duplicate of bug 29464 ***
Comment 3 Tomás Cohen Arazi 2022-01-03 12:03:11 UTC
I forgot where this was at, sorry :-D
Comment 4 Catherine E. Ingram 2022-01-05 17:39:24 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 5 Katrin Fischer 2022-01-08 21:48:25 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> I forgot where this was at, sorry :-D

1) Create a basket
2) Create orders
3) Close basket
4) Receive shipment
5) Enter invoice information
6) You should see your pending orders
7) Verify the 'Summary' column is no longer sortable.
Comment 6 Tomás Cohen Arazi 2022-02-02 20:24:38 UTC
Created attachment 130122 [details] [review]
Bug 29570: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2022-02-02 20:24:42 UTC
Created attachment 130123 [details] [review]
Bug 29570: Fix sorting orders by biblioitems columns

The API representation of Koha::Biblio objects includes the
biblioitems.* columns too. This proved problematic as queries had to get
translated so they work (i.e. if a query passes q={"biblio.ean":"123%"}
then the query needs to be tweaked so biblio.ean is translated into
biblio.biblioitem.ean. This is solved, locally, in the controller.

But sorting needs the same kind of tweak, and it was missing. This patch
solves that by doing a similar conversion.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail, searching on a 'biblioitems' column generates a 500
error.
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! We can sort on those fields
5. Sign off :-D

Note: you will notice the tests only cover sorting by ISBN. I consider
it enough as sorting is a DB problem, and we only want to know if the
generated ORDER BY is valid for the underlying query, and MySQL would
complain if it wasn't the case.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2022-02-02 20:24:47 UTC
Created attachment 130124 [details] [review]
Bug 29570: Enable sorting orders by summary

This patch enables (on receiving) sorting orders by the summary column.
It needs the previous patches to work. Title is given more precedence as
expressed by users on the bug report. Which, by the way, makes sense.

To test:
1. Create a basket
2. Create orders
3. Close basket
4. Receive shipment
5. Enter invoice information
=> SUCCESS: You should see your pending orders
=> FAIL: Verify the 'Summary' column is not sortable
7. Apply this patches
8. Restart Plack
9. Reload the page
=> SUCCESS: Can sort by the summary column
10. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Lucas Gass 2022-02-02 20:51:50 UTC
Created attachment 130125 [details] [review]
Bug 29570: Regression tests

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

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 10 Lucas Gass 2022-02-02 20:51:54 UTC
Created attachment 130126 [details] [review]
Bug 29570: Fix sorting orders by biblioitems columns

The API representation of Koha::Biblio objects includes the
biblioitems.* columns too. This proved problematic as queries had to get
translated so they work (i.e. if a query passes q={"biblio.ean":"123%"}
then the query needs to be tweaked so biblio.ean is translated into
biblio.biblioitem.ean. This is solved, locally, in the controller.

But sorting needs the same kind of tweak, and it was missing. This patch
solves that by doing a similar conversion.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail, searching on a 'biblioitems' column generates a 500
error.
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! We can sort on those fields
5. Sign off :-D

Note: you will notice the tests only cover sorting by ISBN. I consider
it enough as sorting is a DB problem, and we only want to know if the
generated ORDER BY is valid for the underlying query, and MySQL would
complain if it wasn't the case.

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

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 11 Lucas Gass 2022-02-02 20:51:58 UTC
Created attachment 130127 [details] [review]
Bug 29570: Enable sorting orders by summary

This patch enables (on receiving) sorting orders by the summary column.
It needs the previous patches to work. Title is given more precedence as
expressed by users on the bug report. Which, by the way, makes sense.

To test:
1. Create a basket
2. Create orders
3. Close basket
4. Receive shipment
5. Enter invoice information
=> SUCCESS: You should see your pending orders
=> FAIL: Verify the 'Summary' column is not sortable
7. Apply this patches
8. Restart Plack
9. Reload the page
=> SUCCESS: Can sort by the summary column
10. Sign off :-D

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

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 12 Jonathan Druart 2022-02-08 08:53:13 UTC
Tomas, shouldn't we deal with all biblioitem's attributes?
Comment 13 Jonathan Druart 2022-02-08 08:54:19 UTC
Comment on attachment 130126 [details] [review]
Bug 29570: Fix sorting orders by biblioitems columns

Review of attachment 130126 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Acquisitions/Orders.pm
@@ +76,5 @@
>          }
>  
> +        if ( exists $reserved_params->{_order_by} ) {
> +            # _order_by passed, fix if required
> +            for ( my $i = 0; $i < scalar @{$reserved_params->{_order_by}}; $i++ ) {

You could write it in perl instead of javascript ;)
Comment 14 Tomás Cohen Arazi 2022-02-08 10:28:08 UTC
(In reply to Jonathan Druart from comment #12)
> Tomas, shouldn't we deal with all biblioitem's attributes?

It is your fault :P

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115963
Comment 15 Jonathan Druart 2022-02-08 10:33:32 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> (In reply to Jonathan Druart from comment #12)
> > Tomas, shouldn't we deal with all biblioitem's attributes?
> 
> It is your fault :P
> 
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115963

Yes, do it like that :)
Comment 16 Tomás Cohen Arazi 2022-02-08 14:21:23 UTC
(In reply to Jonathan Druart from comment #13)
> Comment on attachment 130126 [details] [review] [review]
> Bug 29570: Fix sorting orders by biblioitems columns
> 
> Review of attachment 130126 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Acquisitions/Orders.pm
> @@ +76,5 @@
> >          }
> >  
> > +        if ( exists $reserved_params->{_order_by} ) {
> > +            # _order_by passed, fix if required
> > +            for ( my $i = 0; $i < scalar @{$reserved_params->{_order_by}}; $i++ ) {
> 
> You could write it in perl instead of javascript ;)

This is more C-ish
Comment 17 Jonathan Druart 2022-02-08 16:31:56 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> (In reply to Jonathan Druart from comment #12)
> > Tomas, shouldn't we deal with all biblioitem's attributes?
> 
> It is your fault :P
> 
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115963

Sorry, misunderstood what you were referring to.
Fair enough then!
Comment 18 Jonathan Druart 2022-02-08 16:35:55 UTC
Created attachment 130308 [details] [review]
Bug 29570: Regression tests

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

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2022-02-08 16:36:00 UTC
Created attachment 130309 [details] [review]
Bug 29570: Fix sorting orders by biblioitems columns

The API representation of Koha::Biblio objects includes the
biblioitems.* columns too. This proved problematic as queries had to get
translated so they work (i.e. if a query passes q={"biblio.ean":"123%"}
then the query needs to be tweaked so biblio.ean is translated into
biblio.biblioitem.ean. This is solved, locally, in the controller.

But sorting needs the same kind of tweak, and it was missing. This patch
solves that by doing a similar conversion.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail, searching on a 'biblioitems' column generates a 500
error.
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! We can sort on those fields
5. Sign off :-D

Note: you will notice the tests only cover sorting by ISBN. I consider
it enough as sorting is a DB problem, and we only want to know if the
generated ORDER BY is valid for the underlying query, and MySQL would
complain if it wasn't the case.

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

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2022-02-08 16:36:05 UTC
Created attachment 130310 [details] [review]
Bug 29570: Enable sorting orders by summary

This patch enables (on receiving) sorting orders by the summary column.
It needs the previous patches to work. Title is given more precedence as
expressed by users on the bug report. Which, by the way, makes sense.

To test:
1. Create a basket
2. Create orders
3. Close basket
4. Receive shipment
5. Enter invoice information
=> SUCCESS: You should see your pending orders
=> FAIL: Verify the 'Summary' column is not sortable
7. Apply this patches
8. Restart Plack
9. Reload the page
=> SUCCESS: Can sort by the summary column
10. Sign off :-D

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

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2022-02-08 16:36:10 UTC
Created attachment 130311 [details] [review]
Bug 29570: Simplify for loop

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Tomás Cohen Arazi 2022-02-08 18:02:54 UTC
Created attachment 130336 [details] [review]
Bug 29570: Simplify for loop

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Fridolin Somers 2022-02-15 07:42:41 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 24 Kyle M Hall 2022-02-16 18:39:14 UTC
Pushed to 21.11.x for 21.11.03
Comment 25 Andrew Fuerste-Henry 2022-02-17 13:59:00 UTC
Pushed to 21.05.x for 21.05.11
Comment 26 Victor Grousset/tuxayo 2022-02-21 08:58:46 UTC
Missing dependencies for 20.11.x, it shouldn't be affected, no backport.