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
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
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
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
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
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
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
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
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>
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>
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>
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.
Created attachment 123841 [details] [review] Bug 28847: Only apply JS to branch/multibranch selection if there are search groups
(confirmed library limits are showing up properly on the OPAC now)
*** Bug 28295 has been marked as a duplicate of this bug. ***
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>
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>
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>
Created attachment 124577 [details] [review] Bug 28847: Only apply JS to branch/multibranch selection if there are search groups
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 ) {
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
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.
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
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>
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>
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>
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>
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>
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>
Pushed to master for 21.11, thanks to everybody involved!
Created attachment 126827 [details] [review] Bug 28847: Remove diag statements
(In reply to Jonathan Druart from comment #30) > Created attachment 126827 [details] [review] [review] > Bug 28847: Remove diag statements Pushed to master.
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'
Pushed to 21.05.x for 21.05.05
Depends on bug 21249 not in 20.11.x