Bug 25882 - Elasticsearch - Advanced search itemtype limits are being double quoted
Summary: Elasticsearch - Advanced search itemtype limits are being double quoted
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 26110 (view as bug list)
Depends on: 23521
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-26 13:19 UTC by Nick Clemens
Modified: 2020-09-20 14:37 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09


Attachments
Bug 25882: Unit test (1.54 KB, patch)
2020-06-26 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25882: Remove phr indicator from limits when processing (1.67 KB, patch)
2020-06-26 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25882: Remove phr indicator from limits when processing (1.72 KB, patch)
2020-06-26 14:57 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25882: Unit test (1.59 KB, patch)
2020-06-26 14:58 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25882: Remove phr indicator from limits when processing (1.72 KB, patch)
2020-06-26 14:58 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25882: Unit test (1.64 KB, patch)
2020-08-11 13:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25882: Remove phr indicator from limits when processing (1.78 KB, patch)
2020-08-11 13:43 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 Nick Clemens 2020-06-26 13:19:03 UTC
To test:
 1 - Have koha using ES
 2 - Go to advanced search
 3 - Limit by a single itemtype that exists
 4 - Get some results
 5 - Limit by a different itemtype that exists
 6 - Get some results
 7 - Limit by both itemtypes
 8 - Get only the results for the second itemtype
 9 - Enable DumpTemplateVarsIntranet and DumpSearchQueryTemplate
10 - Repeat search
11 - View page source and find 'search_query'
12 - See limit looks like itype:("("BK")" OR "("CR")")
Comment 1 Nick Clemens 2020-06-26 13:22:42 UTC
Created attachment 106338 [details] [review]
Bug 25882: Unit test
Comment 2 Nick Clemens 2020-06-26 13:22:45 UTC
Created attachment 106339 [details] [review]
Bug 25882: Remove phr indicator from limits when processing

We assume all limits from advanced search to be a phrase and quote them
when doing this we should remove the phrase marker to avoid doulbe quoting

To test:
 1 - Have koha using ES
 2 - Go to advanced search
 3 - Limit by a single itemtype that exists
 4 - Get some results
 5 - Limit by a different itemtype that exists
 6 - Get some results
 7 - Limit by both itemtypes
 8 - Get only the results for the second itemtype
 9 - Enable DumpTemplateVarsIntranet and DumpSearchQueryTemplate
10 - Repeat search
11 - View page source and find 'search_query'
12 - See limit looks like itype:("("BK")" OR "("CR")")
13 - Apply patches
14 - Restart all the things
15 - Repeat search for both itemtypes
16 - Note results now include both types
Comment 3 Alex Arnaud 2020-06-26 14:56:39 UTC
Tested with es-5 and es-6, ok.
Signing off
Comment 4 Alex Arnaud 2020-06-26 14:57:26 UTC Comment hidden (obsolete)
Comment 5 Alex Arnaud 2020-06-26 14:58:51 UTC
Created attachment 106343 [details] [review]
Bug 25882: Unit test

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 6 Alex Arnaud 2020-06-26 14:58:54 UTC
Created attachment 106344 [details] [review]
Bug 25882: Remove phr indicator from limits when processing

We assume all limits from advanced search to be a phrase and quote them
when doing this we should remove the phrase marker to avoid doulbe quoting

To test:
 1 - Have koha using ES
 2 - Go to advanced search
 3 - Limit by a single itemtype that exists
 4 - Get some results
 5 - Limit by a different itemtype that exists
 6 - Get some results
 7 - Limit by both itemtypes
 8 - Get only the results for the second itemtype
 9 - Enable DumpTemplateVarsIntranet and DumpSearchQueryTemplate
10 - Repeat search
11 - View page source and find 'search_query'
12 - See limit looks like itype:("("BK")" OR "("CR")")
13 - Apply patches
14 - Restart all the things
15 - Repeat search for both itemtypes
16 - Note results now include both types

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 7 Nick Clemens 2020-07-31 09:42:29 UTC
*** Bug 26110 has been marked as a duplicate of this bug. ***
Comment 8 Tomás Cohen Arazi 2020-08-11 13:39:56 UTC
I had issues with the test plan, as using ES 6.8.8 (the default with KTD) I cannot even filter by one itemtype on the advanced search. Applying this patches , restarting and reloading the 'no results' page showed this actually fixes that also.
Comment 9 Tomás Cohen Arazi 2020-08-11 13:43:03 UTC
Created attachment 108067 [details] [review]
Bug 25882: Unit test

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2020-08-11 13:43:12 UTC
Created attachment 108068 [details] [review]
Bug 25882: Remove phr indicator from limits when processing

We assume all limits from advanced search to be a phrase and quote them
when doing this we should remove the phrase marker to avoid doulbe quoting

To test:
 1 - Have koha using ES
 2 - Go to advanced search
 3 - Limit by a single itemtype that exists
 4 - Get some results
 5 - Limit by a different itemtype that exists
 6 - Get some results
 7 - Limit by both itemtypes
 8 - Get only the results for the second itemtype
 9 - Enable DumpTemplateVarsIntranet and DumpSearchQueryTemplate
10 - Repeat search
11 - View page source and find 'search_query'
12 - See limit looks like itype:("("BK")" OR "("CR")")
13 - Apply patches
14 - Restart all the things
15 - Repeat search for both itemtypes
16 - Note results now include both types

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Jonathan Druart 2020-08-13 05:57:29 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 12 Lucas Gass 2020-08-17 14:28:42 UTC
backported to 20.05.x for 20.05.03
Comment 13 Aleisha Amohia 2020-08-18 01:47:28 UTC
backported to 19.11.x for 19.11.09
Comment 14 Victor Grousset/tuxayo 2020-09-20 14:37:46 UTC
Missing dependencies for 19.05.x, can't backport.