On advanced search in OPAC or staff: - leave the first pulldown on keyword and the search term empty - select author or title from the second pull down and enter a search term What will happen is, that Koha performs a keyword search with the search term from the second pull down. This means: Keyword: empty Title: <search term> Keyword: empty is equal to Keyword: <search term> Keyword: empty Keyword: empty but not the same as: Title : <search term> Keyword: empty Keyword: empty I wonder if this is related to bug 3264? But not sure how to fix it.
Same observation at Lyon3 : the problem comes from what Paul Poulain pointed out in last comment of bug 3264. (But a recent test in Chrome got the same wrong result as in Firefox). As a work around I put the following function in opacuserjs syspref but I guess it would be better to find a more orthodox way : $(document).ready(function(){ $("input[name='do']").click(function(){ $("input[name='q']").each(function(i){ if (!$(this).val()){ $("select[name='idx']").eq(i).append('<option value="" selected="selected"></option>'); } }); }); }); and in intranetuserjs : $(document).ready(function() { $("#submit1,#submit2").click(function(){ $("input[name='q']").each(function(i){ if (!$(this).val()){ $("select[name='idx']").eq(i).append('<option value="" selected="selected"></option>'); } }); }); }); Olivier Crouzet
Adding Jared in CC as I think he mentioned his search rewrite work might have a fix for this :)
This might be caused by URL-rewriting (in apache configuration). Because when Keyword is selected, the value of select is empty and it generates in URL "&idx=" which is removed by URL-rewriting.
Created attachment 107963 [details] [review] Bug 7607: Fix adv search when leaving fields empty We have a rewrite rule in our apache config that remove empty parameters: RewriteCond %{QUERY_STRING} (.*?)(?:[A-Za-z0-9_-]+)=&(.*) That causes the advanced search to search on the wrong index if an input field is left empty. Test plan (staff and OPAC): - leave the first pulldown on keyword and the search term empty - select author or title from the second pull down and enter a search term
Patch no longer applies 8-( : root@kohadevbox:koha(bz7607)$ git bz apply 7607 Bug 7607 - Advanced search: Index and search term don't match when leaving fields empty 107963 - Bug 7607: Fix adv search when leaving fields empty Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 7607: Fix adv search when leaving fields empty Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/advsearch.tt M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/advsearch.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/advsearch.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 7607: Fix adv search when leaving fields empty The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-7607-Fix-adv-search-when-leaving-fields-empty-P2nq_2.patch
Created attachment 110845 [details] [review] Bug 7607: Fix adv search when leaving fields empty We have a rewrite rule in our apache config that remove empty parameters: RewriteCond %{QUERY_STRING} (.*?)(?:[A-Za-z0-9_-]+)=&(.*) That causes the advanced search to search on the wrong index if an input field is left empty. Test plan (staff and OPAC): - leave the first pulldown on keyword and the search term empty - select author or title from the second pull down and enter a search term
Created attachment 110849 [details] [review] Bug 7607: Fix adv search when leaving fields empty We have a rewrite rule in our apache config that remove empty parameters: RewriteCond %{QUERY_STRING} (.*?)(?:[A-Za-z0-9_-]+)=&(.*) That causes the advanced search to search on the wrong index if an input field is left empty. Test plan (staff and OPAC): - leave the first pulldown on keyword and the search term empty - select author or title from the second pull down and enter a search term Signed-off-by: David Nind <david@davidnind.com>
QAing
Created attachment 111064 [details] [review] Bug 7607: Fix adv search when leaving fields empty We have a rewrite rule in our apache config that remove empty parameters: RewriteCond %{QUERY_STRING} (.*?)(?:[A-Za-z0-9_-]+)=&(.*) That causes the advanced search to search on the wrong index if an input field is left empty. Test plan (staff and OPAC): - leave the first pulldown on keyword and the search term empty - select author or title from the second pull down and enter a search term Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 111065 [details] [review] Bug 7607: (QA follow-up) Add comment line to new submit action The solution here is a workaround, but we need to document why we are doing this. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Works, but kind of tweaky ;)
Should this really be marked as Major btw ?
(In reply to Marcel de Rooy from comment #12) > Should this really be marked as Major btw ? I fixed it because it was marked as major :)
Blocked by bug 25548 for now.
I believe major is justified as this is an issue we've run into multiple times over the years and it affects the OPAC search and so is confusing to users. I am not sure how it relates to bug 25548 ?
If we have the change from bug 25548, this issue will get fixed.
Actually, we have a customization that does something like this patch, and it actually causes the problem to be worse. If you set the idx element value to '', the idx select elements don't get sent, but the empty q input elements do. So you wind up with a mismatched number of parameters.
(In reply to Jonathan Druart from comment #16) > If we have the change from bug 25548, this issue will get fixed. I agree. Bug 25548 would resolve Bug 7607. And if people are worried about ugly URLs, then I think they should open a new bug report to remove unused search boxes in advanced search from the DOM so they don't get submitted with the HTML form.
(In reply to David Cook from comment #18) > And if people are worried about ugly URLs, then I think they should open a > new bug report to remove unused search boxes in advanced search from the DOM > so they don't get submitted with the HTML form. Alternatively, we could only show 1 search box on the advanced search page, and make people add more search boxes as needed.
(In reply to David Cook from comment #17) > Actually, we have a customization that does something like this patch, and > it actually causes the problem to be worse. > > If you set the idx element value to '', the idx select elements don't get > sent, but the empty q input elements do. So you wind up with a mismatched > number of parameters. This isn't true *if* you provide an option in the select list with a value of ''. So adding that option element in the list would help, if we want to use the Javascript in the patches.
Created attachment 112912 [details] [review] Bug 7607: [Alternate] Remove empty inputs when submitting search form This patch surrounds the operator, index, and query fields on the advanced search page wtih a 'search_set' class. This allows us to process them as a group and confirm the query is populated before submitting the other parts to the form TO test: 1 - Go to advanced search 2 - Use search boxes to search like: Title: (leave blank) Author: clinton keyword: shuffle 3 - Perform search 4 - Note no results as you searched for title:clinton etc. 5 - apply patch 6 - refresh and repeat 7 - search is now correct 8 - Try vaeious searches with 'more options' and selecting different operators
hopefully helping and not muddying, adding an alternate patch to remove the whole search (index, query, operator) if there is no value provided
(In reply to Nick Clemens from comment #22) > hopefully helping and not muddying, adding an alternate patch to remove the > whole search (index, query, operator) if there is no value provided Thx Nick, I was going to comment that we don't need to change the form itself!
Comment on attachment 112912 [details] [review] Bug 7607: [Alternate] Remove empty inputs when submitting search form Review of attachment 112912 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/js/staff-global.js @@ +140,5 @@ > + if( $(this).find('input[name="q"]').val() == "" ){ > + $(this).remove(); > + removedPrior = true; > + } > + }); I haven't tested the code but what about the idx? Wouldn't this code exacerbate the problem?
Created attachment 112963 [details] [review] Bug 7607: [Alternate] Remove empty inputs when submitting search form This patch surrounds the operator, index, and query fields on the advanced search page wtih a 'search_set' class. This allows us to process them as a group and confirm the query is populated before submitting the other parts to the form TO test: 1 - Go to advanced search 2 - Use search boxes to search like: Title: (leave blank) Author: clinton keyword: shuffle 3 - Perform search 4 - Note no results as you searched for title:clinton etc. 5 - apply patch 6 - refresh and repeat 7 - search is now correct 8 - Try vaeious searches with 'more options' and selecting different operators Signed-off-by: David Nind <david@davidnind.com>
Ok this issue is really annoying me, so let's get it sorted... Test plan: 0. No patch 1. Go to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?expanded_options=1 2. Change 2nd "Keyword" and 3rd "Keyword" to "Author" 3. Change 2nd search box to "Martin" and 3rd search box to "Humble" 4. Change 2nd op box (between Martin and Humble) to "or" 5. Click "Search" 6. Note the following URL: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?advsearch=1&idx=kw&op=and&idx=au&q=Martin&op=or&idx=au&q=Humble&sort_by=relevance 7. Note the HTML response: No results match your search for 'kw,wrdl: Martin and au,wrdl: Humble'. Test plan: 0. Apply Nick's patch and "koha-plack --restart kohadev" 1. Go to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?expanded_options=1 2. Change 2nd "Keyword" and 3rd "Keyword" to "Author" 3. Change 2nd search box to "Martin" and 3rd search box to "Humble" 4. Change 2nd op box (between Martin and Humble) to "or" 5. Click "Search" 5b. If you have a quick eye, you'll see the empty search box was removed by Javascript. Yay! 6. Note the following URL: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?advsearch=1&idx=au&q=Martin&op=or&idx=au&q=Humble&sort_by=relevance 7. Note the HTML response: 12 result(s) found for 'au,wrdl: Martin or au,wrdl: Humble'. Great patch, Nick!
Created attachment 112964 [details] [review] Bug 7607: [Alternate] Remove empty inputs when submitting search form This patch surrounds the operator, index, and query fields on the advanced search page wtih a 'search_set' class. This allows us to process them as a group and confirm the query is populated before submitting the other parts to the form TO test: 1 - Go to advanced search 2 - Use search boxes to search like: Title: (leave blank) Author: clinton keyword: shuffle 3 - Perform search 4 - Note no results as you searched for title:clinton etc. 5 - apply patch 6 - refresh and repeat 7 - search is now correct 8 - Try vaeious searches with 'more options' and selecting different operators Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> https://bugs.koha-community.org/show_bug.cgi?id=7606
To prevent the values to be sent maybe we could disable the elements instead of removing them from the DOM. What do you think?
Created attachment 113133 [details] [review] Bug 7607: Remove empty inputs when submitting search form This patch surrounds the operator, index, and query fields on the advanced search page wtih a 'search_set' class. This allows us to process them as a group and confirm the query is populated before submitting the other parts to the form TO test: 1 - Go to advanced search 2 - Use search boxes to search like: Title: (leave blank) Author: clinton keyword: shuffle 3 - Perform search 4 - Note no results as you searched for title:clinton etc. 5 - apply patch 6 - refresh and repeat 7 - search is now correct 8 - Try vaeious searches with 'more options' and selecting different operators Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> https://bugs.koha-community.org/show_bug.cgi?id=7606
Created attachment 113134 [details] [review] Bug 7607: (follow-up) Disable rather than remove inputs
(In reply to Jonathan Druart from comment #28) > To prevent the values to be sent maybe we could disable the elements instead > of removing them from the DOM. What do you think? It seems to work either way - I am not sure if there is any advantage in one method over the other, but I trust the RM
(In reply to Nick Clemens from comment #29) > Signed-off-by: David Nind <david@davidnind.com> > > Signed-off-by: David Cook <dcook@prosentient.com.au> > > https://bugs.koha-community.org/show_bug.cgi?id=7606 Ooops I think I introduced an extra mistyped line to the bottom of the commit message. Hope someone erases that later.
(In reply to Nick Clemens from comment #31) > (In reply to Jonathan Druart from comment #28) > > To prevent the values to be sent maybe we could disable the elements instead > > of removing them from the DOM. What do you think? > > It seems to work either way - I am not sure if there is any advantage in one > method over the other, but I trust the RM What's the effect when hitting the "Back" button in the browser? I have a feeling that removing the elements might work better than disabling them in that context, but I'm not 100% sure off the top of my head. (Just thinking of different scenarios.)
(In reply to David Cook from comment #33) > (In reply to Nick Clemens from comment #31) > > (In reply to Jonathan Druart from comment #28) > > > To prevent the values to be sent maybe we could disable the elements instead > > > of removing them from the DOM. What do you think? > > > > It seems to work either way - I am not sure if there is any advantage in one > > method over the other, but I trust the RM > > What's the effect when hitting the "Back" button in the browser? I have a > feeling that removing the elements might work better than disabling them in > that context, but I'm not 100% sure off the top of my head. (Just thinking > of different scenarios.) In chrome and firefox the field is enabled again when hitting back. visually it is probably less jarring to see the field disabled versus removed so I think it can move forward like this
(In reply to Nick Clemens from comment #34) > In chrome and firefox the field is enabled again when hitting back. visually > it is probably less jarring to see the field disabled versus removed so I > think it can move forward like this Sounds good! I think the input reset type doesn't remove the disabled attribute but I don't think that we ever use that in Koha, so no worries.
Created attachment 113682 [details] [review] Bug 7607: Remove empty inputs when submitting search form This patch surrounds the operator, index, and query fields on the advanced search page wtih a 'search_set' class. This allows us to process them as a group and confirm the query is populated before submitting the other parts to the form TO test: 1 - Go to advanced search 2 - Use search boxes to search like: Title: (leave blank) Author: clinton keyword: shuffle 3 - Perform search 4 - Note no results as you searched for title:clinton etc. 5 - apply patch 6 - refresh and repeat 7 - search is now correct 8 - Try vaeious searches with 'more options' and selecting different operators Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 113683 [details] [review] Bug 7607: (follow-up) Disable rather than remove inputs Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
As a step to help QA, here is another signoff. However I'm having a crash of the qa-test-tools. Maybe it's my setup, however it works on other previous commits from master. From a koha-testing-docker with up to date qa-test-tools: kohadev-koha@kohadevbox:/kohadevbox/koha$ /kohadevbox/qa-test-tools/koha-qa.pl -v 2 -c 2 testing 2 commit(s) (applied to 55835cc 'af Bug 27021: Make chaining ->empty a') Processing files before patches sh: 1: Syntax error: "(" unexpected100.00%) Processing files after patches |========================>| 2 / 2 (100.00%) OK koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/advsearch.tt OK filters OK forbidden patterns OK git manipulation OK js_in_body OK spelling OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/js/staff-global.js OK ES template literals OK git manipulation Processing additional checks OK! sh: 1: Syntax error: "(" unexpected kohadev-koha@kohadevbox:/kohadevbox/koha$
Created attachment 113691 [details] [review] Bug 7607: Remove empty inputs when submitting search form This patch surrounds the operator, index, and query fields on the advanced search page wtih a 'search_set' class. This allows us to process them as a group and confirm the query is populated before submitting the other parts to the form TO test: 1 - Go to advanced search 2 - Use search boxes to search like: Title: (leave blank) Author: clinton keyword: shuffle 3 - Perform search 4 - Note no results as you searched for title:clinton etc. 5 - apply patch 6 - refresh and repeat 7 - search is now correct 8 - Try vaeious searches with 'more options' and selecting different operators Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 113692 [details] [review] Bug 7607: (follow-up) Disable rather than remove inputs Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
I do really like to see this bug fixed. - QA tools passed. - Behaviour is correct.
1. Those patches don't affect the OPAC 2. Same problem exists with the "limit" parameter.
(In reply to Jonathan Druart from comment #42) > 1. Those patches don't affect the OPAC That's a good point but I don't know that it's worth failing it? > 2. Same problem exists with the "limit" parameter. Can you provide steps to reproduce that problem and more explanation? I have not experienced any problems with the "limit" parameter.
(In reply to David Cook from comment #43) > (In reply to Jonathan Druart from comment #42) > > 1. Those patches don't affect the OPAC > > That's a good point but I don't know that it's worth failing it? The first patch dealt with OPAC, and the code is similar. I don't see any good reasons to not fix it as well. > > 2. Same problem exists with the "limit" parameter. > > Can you provide steps to reproduce that problem and more explanation? I have > not experienced any problems with the "limit" parameter. Nothing special... Adv search: /cgi-bin/koha/catalogue/search.pl?advsearch=1&idx=kw&q=d&limit=&limit=&limit=&limit=&limit=&sort_by=relevance (On top of bug 25548)
Created attachment 113781 [details] [review] Bug 7607: (follow-up) Address OPAC and limits This patch adds the same function to the OPAC and adds support for limits I adjust the class on staff side to match the one already existing on OPAC On the OPAC when you click the back button the fields are not enabled - on the staff side they are, I leave this problem for someone else to solve
(In reply to Jonathan Druart from comment #44) Thanks for clarifying, Jonathan! Makes sense. (In reply to Nick Clemens from comment #45) > On the OPAC when you click the back button the fields are not enabled - on > the staff side they are, > I leave this problem for someone else to solve Hmm bizarre...
(In reply to Nick Clemens from comment #45) > Created attachment 113781 [details] [review] [review] > On the OPAC when you click the back button the fields are not enabled - on > the staff side they are, > I leave this problem for someone else to solve Looks of for me on Firefox (68.12.0esr) and Chromium (Version 83.0.4103.116)
(In reply to Nick Clemens from comment #45) > Created attachment 113781 [details] [review] [review] [review] > On the OPAC when you click the back button the fields are not enabled - on > the staff side they are, > I leave this problem for someone else to solve Looks ok for me on Firefox (68.12.0esr) and Chromium (Version 83.0.4103.116)
Created attachment 113872 [details] [review] Bug 7607: Remove empty inputs when submitting search form This patch surrounds the operator, index, and query fields on the advanced search page wtih a 'search_set' class. This allows us to process them as a group and confirm the query is populated before submitting the other parts to the form TO test: 1 - Go to advanced search 2 - Use search boxes to search like: Title: (leave blank) Author: clinton keyword: shuffle 3 - Perform search 4 - Note no results as you searched for title:clinton etc. 5 - apply patch 6 - refresh and repeat 7 - search is now correct 8 - Try vaeious searches with 'more options' and selecting different operators Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 113873 [details] [review] Bug 7607: (follow-up) Disable rather than remove inputs Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 113874 [details] [review] Bug 7607: (follow-up) Address OPAC and limits This patch adds the same function to the OPAC and adds support for limits I adjust the class on staff side to match the one already existing on OPAC On the OPAC when you click the back button the fields are not enabled - on the staff side they are, I leave this problem for someone else to solve Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This works really well, and I can't replicate any issues with using the back button either. Passing QA
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.07
Conflict in koha-tmpl/intranet-tmpl/prog/js/staff-global.js Can't resolve and can't backport to 19.11.x <<<<<<< HEAD if (typeof $.cookie("lastborrowernumber") === "undefined" || ($("#hiddenborrowernumber").val() != $.cookie("lastborrowernumber") && $.cookie("currentborrowernumber") != $("#hiddenborrowernumber").val())) { $.cookie("lastborrowernumber", $("#hiddenborrowernumber").val(), { path: "/" }); $.cookie("lastborrowername", $("#hiddenborrowername").val(), { path: "/" }); $.cookie("lastborrowercard", $("#hiddenborrowercard").val(), { path: "/" }); } $.cookie("currentborrowernumber", $("#hiddenborrowernumber").val(), { path: "/" }); ||||||| parent of b460a630ef (Bug 7607: Remove empty inputs when submitting search form) /* Search results browsing */ /* forms with action leading to search */ $("form[action*='search.pl']").submit(function(){ resetSearchContext(); }); /* any link to launch a search except navigation links */ $("[href*='search.pl?']").not(".nav").not('.searchwithcontext').click(function(){ resetSearchContext(); }); /* any link to a detail page from the results page. */ $("#bookbag_form a[href*='detail.pl?']").click(function(){ resetSearchContext(); }); ======= /* Search results browsing */ /* forms with action leading to search */ $("form[action*='search.pl']").submit(function(){ var removedPrior = false; $(".search_set").each(function(){ if( removedPrior ){ $(this).find('select[name="op"]').remove(); removedPrior = false; } if( $(this).find('input[name="q"]').val() == "" ){ $(this).remove(); removedPrior = true; } }); resetSearchContext(); }); /* any link to launch a search except navigation links */ $("[href*='search.pl?']").not(".nav").not('.searchwithcontext').click(function(){ resetSearchContext(); }); /* any link to a detail page from the results page. */ $("#bookbag_form a[href*='detail.pl?']").click(function(){ resetSearchContext(); }); >>>>>>> b460a630ef (Bug 7607: Remove empty inputs when submitting search form)