Bug 27933 - Order patron search broken (dateofbirth, cardnumber, expirationdate)
Summary: Order patron search broken (dateofbirth, cardnumber, expirationdate)
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 27715
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-11 15:34 UTC by Martin Renvoize
Modified: 2021-03-23 23:30 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10,19.11.16


Attachments
Bug 27933: Fix patron search result ordering (2.88 KB, patch)
2021-03-17 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27933: [ALTERNATIVE PATCH] Fix patron search result ordering (1.98 KB, patch)
2021-03-17 09:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27933: Fix patron search result ordering (2.97 KB, patch)
2021-03-17 15:43 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27933: Fix patron search result ordering (3.00 KB, patch)
2021-03-18 10:54 UTC, Martin Renvoize
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-03-11 15:34:12 UTC
Since bug 15400 the date of birth sorting is broken for the members results and guarantors results tables.

See bug 27926 for further details.
Comment 1 Martin Renvoize 2021-03-11 15:35:11 UTC
I had a go at fixing this as part of 27926 but came to the conclusion they should be treated separately as they work in rather different ways.

Eventually, I think this table should be moved to the modern REST API datatables wrapper approach.
Comment 2 Jonathan Druart 2021-03-11 15:48:26 UTC
(In reply to Martin Renvoize from comment #1)
> Eventually, I think this table should be moved to the modern REST API
> datatables wrapper approach.

That's not trivial at all, there are some specific stuffs done with the filters. I'd would recommend to start with the list first (I have started a bit already but haven't submitted yet).
Comment 3 Martin Renvoize 2021-03-12 14:06:45 UTC
Indeed.. I took a look at it as I submitted this bug... it does not look trivial in the slightest.. there's some really interesting code constructing that table.

What do you mean by start with the list?
Comment 4 Jonathan Druart 2021-03-12 14:16:52 UTC
(In reply to Martin Renvoize from comment #3)
> What do you mean by start with the list?

% ls C4/Utils/DataTables/
Members.pm         TablesSettings.pm  VirtualShelves.pm

=> VirtualShelves.pm will be easier to move to the REST API than Members.pm
Comment 5 Jonathan Druart 2021-03-17 08:38:57 UTC
I think this is different from bug 27926 and is broken since bug 27715.
Comment 6 Jonathan Druart 2021-03-17 09:17:06 UTC
Created attachment 118359 [details] [review]
Bug 27933: Fix patron search result ordering

On bug 27715 we restrict the order by dt params for security reasons.
However in some cases the param passed is "columnname" instead of
"table.columnname".
We should make sure the table is part of the sort fieldname.

Test plan:
Do a "normal" patron search (from the patrons home page) and another
patron search (guarantor for instance).
Sort by cardnumber, date of birth, expiration date, asc, desc and
confirm it works as expected.
Comment 7 Jonathan Druart 2021-03-17 09:18:10 UTC
This is a regression for stable versions.
Comment 8 Jonathan Druart 2021-03-17 09:18:53 UTC
Created attachment 118360 [details] [review]
Bug 27933: [ALTERNATIVE PATCH] Fix patron search result ordering
Comment 9 Jonathan Druart 2021-03-17 09:19:27 UTC
(In reply to Jonathan Druart from comment #8)
> Created attachment 118360 [details] [review] [review]
> Bug 27933: [ALTERNATIVE PATCH] Fix patron search result ordering

I first wrote that version, but I think we should go with the other one.
Tests must be done without this patch.
Comment 10 Owen Leonard 2021-03-17 15:43:54 UTC
Created attachment 118413 [details] [review]
Bug 27933: Fix patron search result ordering

On bug 27715 we restrict the order by dt params for security reasons.
However in some cases the param passed is "columnname" instead of
"table.columnname".
We should make sure the table is part of the sort fieldname.

Test plan:
Do a "normal" patron search (from the patrons home page) and another
patron search (guarantor for instance).
Sort by cardnumber, date of birth, expiration date, asc, desc and
confirm it works as expected.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Martin Renvoize 2021-03-18 10:54:47 UTC
Created attachment 118457 [details] [review]
Bug 27933: Fix patron search result ordering

On bug 27715 we restrict the order by dt params for security reasons.
However in some cases the param passed is "columnname" instead of
"table.columnname".
We should make sure the table is part of the sort fieldname.

Test plan:
Do a "normal" patron search (from the patrons home page) and another
patron search (guarantor for instance).
Sort by cardnumber, date of birth, expiration date, asc, desc and
confirm it works as expected.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-03-18 10:55:19 UTC
Trivial patch, works as expected.. Thanks Jonathan.

PQA
Comment 13 Jonathan Druart 2021-03-18 14:06:58 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Fridolin Somers 2021-03-22 09:48:08 UTC
Pushed to 20.11.x for 20.11.04
Comment 15 Victor Grousset/tuxayo 2021-03-23 23:29:39 UTC
Was pushed to oldstable (20.05.x) for v20.05.10
Comment 16 Victor Grousset/tuxayo 2021-03-23 23:30:00 UTC
Backported: Pushed to 19.11.x branch for 19.11.16