| Summary: | Fix extraction of sort order from sort condition name | ||
|---|---|---|---|
| Product: | Koha | Reporter: | David Gustafsson <glasklas> |
| Component: | Searching - Elasticsearch | Assignee: | David Gustafsson <glasklas> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | fridolin.somers, jonathan.druart, nick |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Bug 19564: Fix extraction of sort order from sort condition name
Bug 19564: Fix extraction of sort order from sort condition name Bug 19564: Add tests for _convert_sort_fields Bug 19564: Fix extraction of sort order from sort condition name Bug 19564: Add tests for _convert_sort_fields Bug 19564: Fix extraction of sort order from sort condition name Bug 19564: Add tests for _convert_sort_fields Bug 19564: (QA follow-up) Make commented out tests pass |
||
|
Description
David Gustafsson
2017-11-02 13:55: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. 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"; Replaced the wrong line in previous patch, will fix. 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. 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 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 (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. Created attachment 70137 [details] [review] Bug 19564: Add tests for _convert_sort_fields Nice ascii art :) Thanks for the tests! Will apply and test this tomorrow. 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. 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> Created attachment 73023 [details] [review] Bug 19564: Add tests for _convert_sort_fields Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> 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> 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> 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> Pushed to master for 18.05, thanks to everybody involved! Pushed to stable for 17.11.06 Awesome work all! Not pushed to 17.05.x |