Bug 27807 - API DataTables Wrapper fails for ordered on multiple columns
Summary: API DataTables Wrapper fails for ordered on multiple columns
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 27680
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-26 13:48 UTC by Martin Renvoize
Modified: 2021-12-13 21:10 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11


Attachments
Bug 27807: Add multi-column ordering support to datatables wrapper (2.33 KB, patch)
2021-03-02 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27807: (QA follow-up) Unique values only (1.63 KB, patch)
2021-03-02 11:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27807: (QA follow-up) Unique values only (1.65 KB, patch)
2021-03-02 11:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27807: Add multi-column ordering support to datatables wrapper (2.40 KB, patch)
2021-03-02 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27807: (QA follow-up) Unique values only (1.72 KB, patch)
2021-03-02 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27807: Add multi-column ordering support to datatables wrapper (2.46 KB, patch)
2021-03-25 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27807: (QA follow-up) Unique values only (1.78 KB, patch)
2021-03-25 16:18 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 Martin Renvoize 2021-02-26 13:48:15 UTC
One cannot currently sort on more than one datatables column at present.
Comment 1 Martin Renvoize 2021-03-02 10:39:53 UTC
Created attachment 117502 [details] [review]
Bug 27807: Add multi-column ordering support to datatables wrapper

This patch add mutli-column ordering support to the datatables api
wrapper.

Test plan
1/ Add a series of cities to the system with various combinations of
city and state such that you can distinguish sorting on city + state in
city ascending + state ascending, city ascending + state descending etc.
2/ Attempt to sort on the two fields in the table (Click the 'City'
heading to sort on city name, then Shift Click on 'State' to add 'state'
ordering on top.. Shift click again on state to reverse the 'state'
ordering but maintain the 'City' ordering.
3/ Confirm the various ordering comes out correctly.
4/ Signoff
Comment 2 Jonathan Druart 2021-03-02 10:47:50 UTC
I've applied "Bug 27680: [DO NOT PUSH] Example patch on cities" and I managed to generate queries like:

  _order_by=+me.name,+me.state,+me.name,+me.city_id
or even:
  _order_by=+me.name,+me.state,+me.name,-me.city_id

I know it is not an usual case, and certainly only hypothetical, but maybe we should unique orderArray?
Comment 3 Martin Renvoize 2021-03-02 10:58:59 UTC
Good call, I didn't think about the uniqueness challenge
Comment 4 Martin Renvoize 2021-03-02 11:21:19 UTC
Created attachment 117504 [details] [review]
Bug 27807: (QA follow-up) Unique values only

This patch filters the final _order_by clause such that we only have
unique data fields present and we take the first occurence of a field.
Comment 5 Tomás Cohen Arazi 2021-03-02 11:39:52 UTC
(In reply to Jonathan Druart from comment #2)
> I've applied "Bug 27680: [DO NOT PUSH] Example patch on cities" and I
> managed to generate queries like:
> 
>   _order_by=+me.name,+me.state,+me.name,+me.city_id
> or even:
>   _order_by=+me.name,+me.state,+me.name,-me.city_id
> 
> I know it is not an usual case, and certainly only hypothetical, but maybe
> we should unique orderArray?

Good catch.
Comment 6 Martin Renvoize 2021-03-02 11:52:58 UTC
Created attachment 117506 [details] [review]
Bug 27807: (QA follow-up) Unique values only

This patch filters the final _order_by clause such that we only have
unique data fields present and we take the first occurence of a field.
Comment 7 Martin Renvoize 2021-03-02 11:54:50 UTC
Updated the patch.. I was pushing an array onto an array.. effectively `[ thing2, [ thing2, thing3] ]`

I've updated the patch to perform an array merge to we get a single level array back and thus the unique filtering works as expected.
Comment 8 Jonathan Druart 2021-03-02 13:58:30 UTC
Created attachment 117514 [details] [review]
Bug 27807: Add multi-column ordering support to datatables wrapper

This patch add mutli-column ordering support to the datatables api
wrapper.

Test plan
1/ Add a series of cities to the system with various combinations of
city and state such that you can distinguish sorting on city + state in
city ascending + state ascending, city ascending + state descending etc.
2/ Attempt to sort on the two fields in the table (Click the 'City'
heading to sort on city name, then Shift Click on 'State' to add 'state'
ordering on top.. Shift click again on state to reverse the 'state'
ordering but maintain the 'City' ordering.
3/ Confirm the various ordering comes out correctly.
4/ Signoff

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2021-03-02 13:58:34 UTC
Created attachment 117515 [details] [review]
Bug 27807: (QA follow-up) Unique values only

This patch filters the final _order_by clause such that we only have
unique data fields present and we take the first occurence of a field.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Tomás Cohen Arazi 2021-03-25 16:18:24 UTC
Created attachment 118825 [details] [review]
Bug 27807: Add multi-column ordering support to datatables wrapper

This patch add mutli-column ordering support to the datatables api
wrapper.

Test plan
1/ Add a series of cities to the system with various combinations of
city and state such that you can distinguish sorting on city + state in
city ascending + state ascending, city ascending + state descending etc.
2/ Attempt to sort on the two fields in the table (Click the 'City'
heading to sort on city name, then Shift Click on 'State' to add 'state'
ordering on top.. Shift click again on state to reverse the 'state'
ordering but maintain the 'City' ordering.
3/ Confirm the various ordering comes out correctly.
4/ Signoff

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2021-03-25 16:18:33 UTC
Created attachment 118826 [details] [review]
Bug 27807: (QA follow-up) Unique values only

This patch filters the final _order_by clause such that we only have
unique data fields present and we take the first occurence of a field.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Jonathan Druart 2021-04-01 16:54:17 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 13 Fridolin Somers 2021-04-13 14:49:33 UTC
Pushed to 20.11.x for 20.11.05
Comment 14 Andrew Fuerste-Henry 2021-04-24 13:46:28 UTC
Pushed to 20.05.x for 20.05.11
Comment 15 Victor Grousset/tuxayo 2021-04-24 16:49:55 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.