Bug 37333

Summary: Search filters using OR are not correctly grouped
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: SearchingAssignee: Nick Clemens (kidclamp) <nick>
Status: Pushed to oldoldstable --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jake.deery, januszop, lucas
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:
24.11.00,24.05.06,23.11.11
Circulation function:
Attachments: Bug 37333: Group search terms in a saved filter
Bug 37333: Group search terms in a saved filter
Bug 37333: Group search terms in a saved filter
Bug 37333: (follow-up) Update existing tests

Description Nick Clemens (kidclamp) 2024-07-11 19:40:17 UTC
When a search filter contains terms joined by OR:
subject:programming OR subject:internet

The search filter is added to a search like:
title:perl AND subject:programming OR subject:internet

This negates the full effect of the filter as it is not considered as a single part
Comment 1 Nick Clemens (kidclamp) 2024-07-11 19:45:39 UTC
Created attachment 168864 [details] [review]
Bug 37333: Group search terms in a saved filter

This patch surrounds the expanded query of a search filter in parnetheses to correctly
group the terms when searching

To test:
 1 - Enable system preference SavedSearchFilters
 2 - In staff client, perform an advanced search for:
     subject:internet OR subject:programming
 3 - Note results
 4 - Save search as filter, enabled in staff client and opac
 5 - Search for 'a'
 6 - Apply filter from top of facets on left hand side
 7 - Note results didn't change
 8 - Apply patch, restart all
 9 - Reload search, it now limits correctly
10 - Test on OPAC
11 - Test on both Zebra and Elastic
12 - Sign off :-)
Comment 2 Jake Deery 2024-07-12 11:53:28 UTC
Created attachment 168894 [details] [review]
Bug 37333: Group search terms in a saved filter

This patch surrounds the expanded query of a search filter in parnetheses to correctly
group the terms when searching

To test:
 1 - Enable system preference SavedSearchFilters
 2 - In staff client, perform an advanced search for:
     subject:internet OR subject:programming
 3 - Note results
 4 - Save search as filter, enabled in staff client and opac
 5 - Search for 'a'
 6 - Apply filter from top of facets on left hand side
 7 - Note results didn't change
 8 - Apply patch, restart all
 9 - Reload search, it now limits correctly
10 - Test on OPAC
11 - Test on both Zebra and Elastic
12 - Sign off :-)

Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
Comment 3 Jake Deery 2024-07-12 11:55:27 UTC
Hi Nick,

All looks good to me. I can see before vs. after on the results page:

-  11 result(s) found for 'kw,wrdl: a' with limit(s): 'and su=(internet) OR su=(programming)'.
+  11 result(s) found for 'kw,wrdl: a' with limit(s): 'and (su=(internet) OR su=(programming))'.

… and the results do change, now closer to what I think is correct, and definitely in a more concice order.

Probably need a search nerd to QA this, but for now, here is my sign-off, with thanks!

Jake
Comment 4 Martin Renvoize (ashimema) 2024-07-17 16:07:24 UTC
Created attachment 169099 [details] [review]
Bug 37333: Group search terms in a saved filter

This patch surrounds the expanded query of a search filter in parnetheses to correctly
group the terms when searching

To test:
 1 - Enable system preference SavedSearchFilters
 2 - In staff client, perform an advanced search for:
     subject:internet OR subject:programming
 3 - Note results
 4 - Save search as filter, enabled in staff client and opac
 5 - Search for 'a'
 6 - Apply filter from top of facets on left hand side
 7 - Note results didn't change
 8 - Apply patch, restart all
 9 - Reload search, it now limits correctly
10 - Test on OPAC
11 - Test on both Zebra and Elastic
12 - Sign off :-)

Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize (ashimema) 2024-07-17 16:08:08 UTC
Thanks Nick, patch I see is trivial, but it's great to see the mammoth set to unit tests to go with it.

Passing QA
Comment 6 Katrin Fischer 2024-07-18 14:49:38 UTC
Unit tests! \o/
Comment 7 Katrin Fischer 2024-07-18 15:57:27 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 8 Janusz Kaczmarek 2024-07-22 11:26:43 UTC
It seems that after applying this patch the test:

t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t

fails :(

The patch adds an additional level of parentheses which in turn makes "Handle search filters" section fail in 4 subtests.

Could you confirm this?
Comment 9 Martin Renvoize (ashimema) 2024-07-22 14:06:43 UTC
Correct; I've already asked Nick to take a look.

My hunch is that it's OK and we just need to update the tests, but i wanted him to confirm that before I went ahead and did that.
Comment 10 Janusz Kaczmarek 2024-07-22 14:54:53 UTC
(In reply to Martin Renvoize from comment #9)
> My hunch is that it's OK and we just need to update the tests, but i wanted
> him to confirm that before I went ahead and did that.

Yes, I agree, this is probably just about updating the test patterns.  Now this stops my playing around with QueryBuilder.t ;)
Comment 11 Martin Renvoize (ashimema) 2024-07-22 14:55:29 UTC
Created attachment 169308 [details] [review]
Bug 37333: (follow-up) Update existing tests

We add a level of bracket nesting. I checked with Nick that this
shouldn't cause any issues in the queries and we agreed it made sense so
I've gone ahead and updated the tests to reflect the change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Lucas Gass (lukeg) 2024-11-18 18:23:05 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 13 Fridolin Somers 2024-12-06 08:51:15 UTC
Pushed to 23.11.x for 23.11.11