Bug 37333 - Search filters using OR are not correctly grouped
Summary: Search filters using OR are not correctly grouped
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-07-11 19:40 UTC by Nick Clemens (kidclamp)
Modified: 2024-12-06 08:51 UTC (History)
4 users (show)

See Also:
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 (4.33 KB, patch)
2024-07-11 19:45 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37333: Group search terms in a saved filter (4.40 KB, patch)
2024-07-12 11:53 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 37333: Group search terms in a saved filter (4.45 KB, patch)
2024-07-17 16:07 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37333: (follow-up) Update existing tests (4.85 KB, patch)
2024-07-22 14:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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