Bug 28847 - Branch limits while searching should be expanded in query building and not in CGI
Summary: Branch limits while searching should be expanded in query building and not in...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
: 28295 (view as bug list)
Depends on: 21249 28845
Blocks: 22605
  Show dependency treegraph
 
Reported: 2021-08-12 11:06 UTC by Nick Clemens
Modified: 2022-12-12 21:23 UTC (History)
5 users (show)

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


Attachments
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules (8.40 KB, patch)
2021-08-12 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Unit tests (11.93 KB, patch)
2021-08-12 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT (9.30 KB, patch)
2021-08-12 11:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT (9.30 KB, patch)
2021-08-13 16:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT (9.52 KB, patch)
2021-08-13 16:34 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT (10.47 KB, patch)
2021-08-13 17:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules (8.47 KB, patch)
2021-08-13 17:17 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28847: Unit tests (11.99 KB, patch)
2021-08-13 17:17 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT (10.53 KB, patch)
2021-08-13 17:17 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28847: Only apply JS to branch/multibranch selection if there are search groups (2.07 KB, patch)
2021-08-13 19:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules (8.47 KB, patch)
2021-09-07 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Unit tests (11.99 KB, patch)
2021-09-07 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT (10.34 KB, patch)
2021-09-07 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Only apply JS to branch/multibranch selection if there are search groups (2.07 KB, patch)
2021-09-07 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: (follow-up) Handle branch_group_limit (1.58 KB, patch)
2021-09-11 15:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Remove dead code and make regex consistent (2.29 KB, patch)
2021-09-13 11:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules (8.61 KB, patch)
2021-09-18 08:15 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28847: Unit tests (12.14 KB, patch)
2021-09-18 08:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT (10.49 KB, patch)
2021-09-18 08:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28847: Only apply JS to branch/multibranch selection if there are search groups (2.21 KB, patch)
2021-09-18 08:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28847: (follow-up) Handle branch_group_limit (1.73 KB, patch)
2021-09-18 08:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28847: Remove dead code and make regex consistent (2.44 KB, patch)
2021-09-18 08:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28847: Remove diag statements (4.55 KB, patch)
2021-10-25 13:03 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-08-12 11:06:02 UTC
Bug 21249 introduced the SearchLimitLibrary system preference and added logic to the search scripts to account for it.

Moving the code to modules allows us to cover the settings with tests.

Additionally we should only expand the query passed to the search engine, the CGI param should remain brief
Comment 1 Nick Clemens 2021-08-12 11:35:20 UTC
Created attachment 123807 [details] [review]
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules

This patch removes the code from the search scripts into QueryBuilder
modules.

To test:
1 - Have a library group defined as a search group for both staff and opac
2 - Search on staff client and opac with that group limit and a single branch limit
3 - Note your results/counts
4 - Note the visuals of the search description
5 - Apply patch
6 - Repeat searches
7 - All should work as before
Comment 2 Nick Clemens 2021-08-12 11:35:23 UTC
Created attachment 123808 [details] [review]
Bug 28847: Unit tests

Note: tests are very similar between ES and Zebra, however, ES requires the uppercase
OR and doesn't use '=' in the same way. I feel having test coverage in each module is fair and more future
proof in case of changes to search engine
Comment 3 Nick Clemens 2021-08-12 11:35:26 UTC
Created attachment 123809 [details] [review]
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT

The opac had 'branch_group_limit' parameters which can be simplified to more
closely match intranet code.

Adjust C4::Auth for chaneg above to ensure dropdowns correctly populate

Expand JS to prevent selection of single and multibranch limits

To test:
1 - Enable OpacAddMastheadLibraryPulldown system preference
2 - Ensure branches and groups show as before patch
3 - Ensure single and multibranch limits from masthead apply as expected
4 - Test advanced search page, ensure you cannot select both single and multibranch limit
5 - Follow test plan on 28845 - ensure multibranch limit still correctly pre-selected
Comment 4 Andrew Fuerste-Henry 2021-08-13 16:01:53 UTC
I stumbled into a problem:
- create a library group for use for OPAC searching BUT NOT staff searching
- go to advanced search in the intranet
- confirm you can select an individual library but no groups are shown
- apply patch
- reload adv search on the intranet
- individual library selector is disabled (incorrect), groups selector doesn't show (correct)
- edit your group, make it usable on the intranet
- reload adv search
- the group picker is now showing and the libraries picker is enabled
Comment 5 Nick Clemens 2021-08-13 16:17:58 UTC
Created attachment 123830 [details] [review]
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT

The opac had 'branch_group_limit' parameters which can be simplified to more
closely match intranet code.

Adjust C4::Auth for chaneg above to ensure dropdowns correctly populate

Expand JS to prevent selection of single and multibranch limits

To test:
1 - Enable OpacAddMastheadLibraryPulldown system preference
2 - Ensure branches and groups show as before patch
3 - Ensure single and multibranch limits from masthead apply as expected
4 - Test advanced search page, ensure you cannot select both single and multibranch limit
5 - Follow test plan on 28845 - ensure multibranch limit still correctly pre-selected
Comment 6 Nick Clemens 2021-08-13 16:34:44 UTC
Created attachment 123832 [details] [review]
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT

The opac had 'branch_group_limit' parameters which can be simplified to more
closely match intranet code.

Adjust C4::Auth for chaneg above to ensure dropdowns correctly populate

Expand JS to prevent selection of single and multibranch limits

To test:
1 - Enable OpacAddMastheadLibraryPulldown system preference
2 - Ensure branches and groups show as before patch
3 - Ensure single and multibranch limits from masthead apply as expected
4 - Test advanced search page, ensure you cannot select both single and multibranch limit
5 - Follow test plan on 28845 - ensure multibranch limit still correctly pre-selected
Comment 7 Nick Clemens 2021-08-13 17:11:24 UTC
Created attachment 123833 [details] [review]
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT

The opac had 'branch_group_limit' parameters which can be simplified to more
closely match intranet code.

Adjust C4::Auth for chaneg above to ensure dropdowns correctly populate

Expand JS to prevent selection of single and multibranch limits

To test:
1 - Enable OpacAddMastheadLibraryPulldown system preference
2 - Ensure branches and groups show as before patch
3 - Ensure single and multibranch limits from masthead apply as expected
4 - Test advanced search page, ensure you cannot select both single and multibranch limit
5 - Follow test plan on 28845 - ensure multibranch limit still correctly pre-selected
Comment 8 Andrew Fuerste-Henry 2021-08-13 17:17:27 UTC
Created attachment 123835 [details] [review]
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules

This patch removes the code from the search scripts into QueryBuilder
modules.

To test:
1 - Have a library group defined as a search group for both staff and opac
2 - Search on staff client and opac with that group limit and a single branch limit
3 - Note your results/counts
4 - Note the visuals of the search description
5 - Apply patch
6 - Repeat searches
7 - All should work as before

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 9 Andrew Fuerste-Henry 2021-08-13 17:17:30 UTC
Created attachment 123836 [details] [review]
Bug 28847: Unit tests

Note: tests are very similar between ES and Zebra, however, ES requires the uppercase
OR and doesn't use '=' in the same way. I feel having test coverage in each module is fair and more future
proof in case of changes to search engine

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 10 Andrew Fuerste-Henry 2021-08-13 17:17:34 UTC
Created attachment 123837 [details] [review]
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT

The opac had 'branch_group_limit' parameters which can be simplified to more
closely match intranet code.

Adjust C4::Auth for chaneg above to ensure dropdowns correctly populate

Expand JS to prevent selection of single and multibranch limits

To test:
1 - Enable OpacAddMastheadLibraryPulldown system preference
2 - Ensure branches and groups show as before patch
3 - Ensure single and multibranch limits from masthead apply as expected
4 - Test advanced search page, ensure you cannot select both single and multibranch limit
5 - Follow test plan on 28845 - ensure multibranch limit still correctly pre-selected

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Andrew Fuerste-Henry 2021-08-13 18:21:22 UTC
Sorry, just noticed the same issue exists on the OPAC. If there is no search group in place for use on the OPAC, the Libraries dropdown is erroneously disabled. Kicking back to FQA.
Comment 12 Nick Clemens 2021-08-13 19:52:59 UTC
Created attachment 123841 [details] [review]
Bug 28847: Only apply JS to branch/multibranch selection if there are search groups
Comment 13 Andrew Fuerste-Henry 2021-08-18 16:03:44 UTC
(confirmed library limits are showing up properly on the OPAC now)
Comment 14 Jonathan Druart 2021-08-25 16:16:08 UTC
*** Bug 28295 has been marked as a duplicate of this bug. ***
Comment 15 Nick Clemens 2021-09-07 12:02:04 UTC
Created attachment 124574 [details] [review]
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules

This patch removes the code from the search scripts into QueryBuilder
modules.

To test:
1 - Have a library group defined as a search group for both staff and opac
2 - Search on staff client and opac with that group limit and a single branch limit
3 - Note your results/counts
4 - Note the visuals of the search description
5 - Apply patch
6 - Repeat searches
7 - All should work as before

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 16 Nick Clemens 2021-09-07 12:02:08 UTC
Created attachment 124575 [details] [review]
Bug 28847: Unit tests

Note: tests are very similar between ES and Zebra, however, ES requires the uppercase
OR and doesn't use '=' in the same way. I feel having test coverage in each module is fair and more future
proof in case of changes to search engine

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 17 Nick Clemens 2021-09-07 12:02:11 UTC
Created attachment 124576 [details] [review]
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT

The opac had 'branch_group_limit' parameters which can be simplified to more
closely match intranet code.

Adjust C4::Auth for chaneg above to ensure dropdowns correctly populate

Expand JS to prevent selection of single and multibranch limits

To test:
1 - Enable OpacAddMastheadLibraryPulldown system preference
2 - Ensure branches and groups show as before patch
3 - Ensure single and multibranch limits from masthead apply as expected
4 - Test advanced search page, ensure you cannot select both single and multibranch limit
5 - Follow test plan on 28845 - ensure multibranch limit still correctly pre-selected

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 18 Nick Clemens 2021-09-07 12:02:14 UTC
Created attachment 124577 [details] [review]
Bug 28847: Only apply JS to branch/multibranch selection if there are search groups
Comment 19 Joonas Kylmälä 2021-09-11 10:25:42 UTC
This breaks the existing links in the format:

localhost:8080/cgi-bin/koha/opac-search.pl?idx=&q=&branch_group_limit=multibranchlimit-1&weight_search=1

i.e. where branch_group_limit is used. Let's not break the links suddenly as they might be someone's bookmarks or shared online. So please fix the code in opac/opac-search.pl to not produce multibranch query params from branch_group_limit param and leave a note that branch_group_limit is deprecated and should not be used.

The code I'm talking about is in the following if block:

> my $branch_group_limit = $cgi->param("branch_group_limit");
> if ( $branch_group_limit ) {
Comment 20 Nick Clemens 2021-09-11 15:16:22 UTC
Created attachment 124797 [details] [review]
Bug 28847: (follow-up) Handle branch_group_limit

To test:
1 - Create a library group as an OPAC search group
2 - Enable OpacAddMastheadLibraryPulldown
3 - Browse to:
    http://localhost:8080/cgi-bin/koha/opac-search.pl?idx=&q=t&branch_group_limit=multibranchlimit-1&weight_search=1
4 - Confirm search works and returns correct results
5 - Confirm dropdown is correctly populated
Comment 21 Joonas Kylmälä 2021-09-12 12:36:53 UTC
Sorry for not doing very good job the first time reviewing, I found now additional issue:

The buildQuery function in C4/Search.pm is modified by "Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules", it however leaves dead code in the else section "if ($this_limit =~ /^branch:(.+)/) {". That should be completely removed, and also we can see from that that the new regex condition added doesn't match as carefully the limit parameter.
Comment 22 Nick Clemens 2021-09-13 11:55:43 UTC
Created attachment 124831 [details] [review]
Bug 28847: Remove dead code and make regex consistent

This updates the regex code to match throughout the routine and removes
code that would no longer be reached.

Note that the code to update library name in the query descruiption was
broken before this patch, so removal does not change behaviour
Comment 23 Joonas Kylmälä 2021-09-18 08:15:58 UTC
Created attachment 125017 [details] [review]
Bug 28847: Move SearchLimitLibrary code to QueryBuilder Modules

This patch removes the code from the search scripts into QueryBuilder
modules.

To test:
1 - Have a library group defined as a search group for both staff and opac
2 - Search on staff client and opac with that group limit and a single branch limit
3 - Note your results/counts
4 - Note the visuals of the search description
5 - Apply patch
6 - Repeat searches
7 - All should work as before

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 24 Joonas Kylmälä 2021-09-18 08:16:02 UTC
Created attachment 125018 [details] [review]
Bug 28847: Unit tests

Note: tests are very similar between ES and Zebra, however, ES requires the uppercase
OR and doesn't use '=' in the same way. I feel having test coverage in each module is fair and more future
proof in case of changes to search engine

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 25 Joonas Kylmälä 2021-09-18 08:16:07 UTC
Created attachment 125019 [details] [review]
Bug 28847: Cleanup of branch limitsand fix OPAC_SEARCH_LIMIT

The opac had 'branch_group_limit' parameters which can be simplified to more
closely match intranet code.

Adjust C4::Auth for chaneg above to ensure dropdowns correctly populate

Expand JS to prevent selection of single and multibranch limits

To test:
1 - Enable OpacAddMastheadLibraryPulldown system preference
2 - Ensure branches and groups show as before patch
3 - Ensure single and multibranch limits from masthead apply as expected
4 - Test advanced search page, ensure you cannot select both single and multibranch limit
5 - Follow test plan on 28845 - ensure multibranch limit still correctly pre-selected

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 26 Joonas Kylmälä 2021-09-18 08:16:11 UTC
Created attachment 125020 [details] [review]
Bug 28847: Only apply JS to branch/multibranch selection if there are search groups

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 27 Joonas Kylmälä 2021-09-18 08:16:15 UTC
Created attachment 125021 [details] [review]
Bug 28847: (follow-up) Handle branch_group_limit

To test:
1 - Create a library group as an OPAC search group
2 - Enable OpacAddMastheadLibraryPulldown
3 - Browse to:
    http://localhost:8080/cgi-bin/koha/opac-search.pl?idx=&q=t&branch_group_limit=multibranchlimit-1&weight_search=1
4 - Confirm search works and returns correct results
5 - Confirm dropdown is correctly populated

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 28 Joonas Kylmälä 2021-09-18 08:16:19 UTC
Created attachment 125022 [details] [review]
Bug 28847: Remove dead code and make regex consistent

This updates the regex code to match throughout the routine and removes
code that would no longer be reached.

Note that the code to update library name in the query descruiption was
broken before this patch, so removal does not change behaviour

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 29 Jonathan Druart 2021-09-20 12:47:56 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 30 Jonathan Druart 2021-10-25 13:03:13 UTC
Created attachment 126827 [details] [review]
Bug 28847: Remove diag statements
Comment 31 Jonathan Druart 2021-10-25 13:59:08 UTC
(In reply to Jonathan Druart from comment #30)
> Created attachment 126827 [details] [review] [review]
> Bug 28847: Remove diag statements

Pushed to master.
Comment 32 Nick Clemens 2021-11-22 18:36:51 UTC
This should be backported to 21.05 - it fixes a bug introduced by 21249 

The code in the scripts joins the branches by 'or' - for ES this must be 'OR'
Comment 33 Kyle M Hall 2021-11-22 18:39:04 UTC
Pushed to 21.05.x for 21.05.05
Comment 34 Fridolin Somers 2021-11-23 07:11:35 UTC
Depends on bug 21249 not in 20.11.x