Bug 19564 - Fix extraction of sort order from sort condition name
Summary: Fix extraction of sort order from sort condition name
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Gustafsson
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-02 13:55 UTC by David Gustafsson
Modified: 2018-12-03 20:04 UTC (History)
3 users (show)

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


Attachments
Bug 19564: Fix extraction of sort order from sort condition name (1.11 KB, patch)
2017-11-02 14:07 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19564: Fix extraction of sort order from sort condition name (1.09 KB, patch)
2017-11-07 14:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19564: Add tests for _convert_sort_fields (1.67 KB, patch)
2017-12-22 18:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19564: Fix extraction of sort order from sort condition name (1.14 KB, patch)
2018-03-16 11:25 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19564: Add tests for _convert_sort_fields (1.72 KB, patch)
2018-03-16 11:26 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 19564: Fix extraction of sort order from sort condition name (1.19 KB, patch)
2018-03-28 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19564: Add tests for _convert_sort_fields (1.78 KB, patch)
2018-03-28 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19564: (QA follow-up) Make commented out tests pass (2.67 KB, patch)
2018-03-28 12:47 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2017-11-02 13:55:05 UTC
Currently sort order is extracted from sort condition with "my ( $f, $d ) = split /_/;"
So for example with the value "call_number_asc", you get $f (field) "call", and $d (order) "number", when expecting to get "call_number" and "asc". This should be changes using a regular expression instead.
Comment 1 David Gustafsson 2017-11-02 14:07:05 UTC
Created attachment 68892 [details] [review]
Bug 19564: Fix extraction of sort order from sort condition name

Currently sort order is extracted from sort condition by splitting the
field, instead use regular expression to extract the last part preceded
by underscore.
Comment 2 David Gustafsson 2017-11-02 14:10:41 UTC
I did not add a test-case since sorting does currently work anyway because of other unrelated bugs (which I can link to once having created issues and submitted fixes f). But this Perl code can be executed to illustrate the issue (and fix):

my ( $f, $d ) = "call_number_asc" =~ /(.+)_(.+)/;
print "$f $d\n";
( $f, $d ) = split /_/, "call_number_asc";
print "$f $d\n";
Comment 3 David Gustafsson 2017-11-07 14:41:57 UTC
Replaced the wrong line in previous patch, will fix.
Comment 4 David Gustafsson 2017-11-07 14:42:49 UTC
Created attachment 68998 [details] [review]
Bug 19564: Fix extraction of sort order from sort condition name

Currently sort order is extracted from sort condition by splitting the
field, instead use regular expression to extract the last part preceded
by underscore.
Comment 5 Nick Clemens 2017-11-17 15:18:32 UTC
This works, I mean we need a callnum index, but it changes form not having a sort to having a sort on an undefined index :-) let me know what other bugs you are working on
Comment 6 David Gustafsson 2017-11-20 10:43:18 UTC
Yes, although I think it's a good idea to fix the code even though it does not solve the issue with the current code base, I think that sorting should work with this fix together with https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19563
Comment 7 Nick Clemens 2017-11-21 15:23:21 UTC
(In reply to Nick Clemens from comment #5)
> This works, I mean we need a callnum index, but it changes form not having a
> sort to having a sort on an undefined index :-) let me know what other bugs
> you are working on

I think I didn't finish my thought here, we need unit tests here to verify the change, if you can provide those I will sign off. If you need any help with them let me know.
Comment 8 Jonathan Druart 2017-12-22 18:23:33 UTC
Created attachment 70137 [details] [review]
Bug 19564: Add tests for _convert_sort_fields
Comment 9 Jonathan Druart 2017-12-22 18:24:16 UTC
Nice ascii art
Comment 10 David Gustafsson 2018-01-03 14:52:53 UTC
:) Thanks for the tests! Will apply and test this tomorrow.
Comment 11 David Gustafsson 2018-01-03 15:14:36 UTC
I had time to do so today. Don't know if I am allowed to sign this off, but can vouch for that the tests are working.
Comment 12 Nicolas Legrand 2018-03-16 11:25:59 UTC
Created attachment 73022 [details] [review]
Bug 19564: Fix extraction of sort order from sort condition name

Currently sort order is extracted from sort condition by splitting the
field, instead use regular expression to extract the last part preceded
by underscore.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 13 Nicolas Legrand 2018-03-16 11:26:02 UTC
Created attachment 73023 [details] [review]
Bug 19564: Add tests for _convert_sort_fields

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Comment 14 Nick Clemens 2018-03-28 12:47:15 UTC
Created attachment 73385 [details] [review]
Bug 19564: Fix extraction of sort order from sort condition name

Currently sort order is extracted from sort condition by splitting the
field, instead use regular expression to extract the last part preceded
by underscore.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2018-03-28 12:47:17 UTC
Created attachment 73386 [details] [review]
Bug 19564: Add tests for _convert_sort_fields

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2018-03-28 12:47:20 UTC
Created attachment 73387 [details] [review]
Bug 19564: (QA follow-up) Make commented out tests pass

We likely shoudln't pass through an uncoverted sort order for now, but
it does allow us to look ahead to implementing the orders directly so
seems a good option to have.

Either this patch should be used, or the commented out tests should be
removed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Jonathan Druart 2018-03-29 15:02:54 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 18 Nick Clemens 2018-05-14 21:24:35 UTC
Pushed to stable for 17.11.06

Awesome work all!
Comment 19 Fridolin Somers 2018-05-22 09:31:31 UTC
Not pushed to 17.05.x