Bug 7607 - Advanced search: Index and search term don't match when leaving fields empty
Summary: Advanced search: Index and search term don't match when leaving fields empty
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks: 9234 25548
  Show dependency treegraph
 
Reported: 2012-02-26 17:05 UTC by Katrin Fischer
Modified: 2020-12-11 16:40 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07


Attachments
Bug 7607: Fix adv search when leaving fields empty (2.68 KB, patch)
2020-08-07 13:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7607: Fix adv search when leaving fields empty (2.64 KB, patch)
2020-09-28 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7607: Fix adv search when leaving fields empty (2.70 KB, patch)
2020-09-28 11:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 7607: Fix adv search when leaving fields empty (2.80 KB, patch)
2020-10-02 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7607: (QA follow-up) Add comment line to new submit action (1.79 KB, patch)
2020-10-02 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7607: [Alternate] Remove empty inputs when submitting search form (2.78 KB, patch)
2020-11-03 11:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 7607: [Alternate] Remove empty inputs when submitting search form (2.83 KB, patch)
2020-11-03 23:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 7607: [Alternate] Remove empty inputs when submitting search form (2.94 KB, patch)
2020-11-03 23:30 UTC, David Cook
Details | Diff | Splinter Review
Bug 7607: Remove empty inputs when submitting search form (2.92 KB, patch)
2020-11-05 19:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 7607: (follow-up) Disable rather than remove inputs (1.57 KB, patch)
2020-11-05 19:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 7607: Remove empty inputs when submitting search form (2.91 KB, patch)
2020-11-16 19:16 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 7607: (follow-up) Disable rather than remove inputs (1.62 KB, patch)
2020-11-16 19:16 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 7607: Remove empty inputs when submitting search form (2.98 KB, patch)
2020-11-17 09:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 7607: (follow-up) Disable rather than remove inputs (1.68 KB, patch)
2020-11-17 09:32 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 7607: (follow-up) Address OPAC and limits (3.59 KB, patch)
2020-11-18 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 7607: Remove empty inputs when submitting search form (3.03 KB, patch)
2020-11-20 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7607: (follow-up) Disable rather than remove inputs (1.74 KB, patch)
2020-11-20 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7607: (follow-up) Address OPAC and limits (3.63 KB, patch)
2020-11-20 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2012-02-26 17:05:44 UTC
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.
Comment 1 Koha Team University Lyon 3 2013-03-15 15:51:06 UTC
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
Comment 2 Katrin Fischer 2013-03-15 17:08:49 UTC
Adding Jared in CC as I think he mentioned his search rewrite work might have a fix for this :)
Comment 3 Fridolin Somers 2013-12-31 09:07:16 UTC
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.
Comment 4 Jonathan Druart 2020-08-07 13:27:35 UTC
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
Comment 5 David Nind 2020-09-18 09:37:07 UTC
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
Comment 6 Jonathan Druart 2020-09-28 10:54:12 UTC
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
Comment 7 David Nind 2020-09-28 11:24:34 UTC
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>
Comment 8 Marcel de Rooy 2020-10-02 07:28:18 UTC
QAing
Comment 9 Marcel de Rooy 2020-10-02 08:04:46 UTC
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>
Comment 10 Marcel de Rooy 2020-10-02 08:04:56 UTC
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>
Comment 11 Marcel de Rooy 2020-10-02 08:05:47 UTC
QA Comment: Works, but kind of tweaky ;)
Comment 12 Marcel de Rooy 2020-10-02 08:08:02 UTC
Should this really be marked as Major btw ?
Comment 13 Jonathan Druart 2020-10-08 08:17:08 UTC
(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 :)
Comment 14 Jonathan Druart 2020-10-08 09:22:17 UTC
Blocked by bug 25548 for now.
Comment 15 Katrin Fischer 2020-10-08 10:28:04 UTC
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 ?
Comment 16 Jonathan Druart 2020-10-12 08:33:05 UTC
If we have the change from bug 25548, this issue will get fixed.
Comment 17 David Cook 2020-11-03 04:10:13 UTC
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.
Comment 18 David Cook 2020-11-03 04:15:17 UTC
(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.
Comment 19 David Cook 2020-11-03 04:17:45 UTC
(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.
Comment 20 David Cook 2020-11-03 04:30:13 UTC
(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.
Comment 21 Nick Clemens 2020-11-03 11:33:48 UTC
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
Comment 22 Nick Clemens 2020-11-03 11:35:42 UTC
hopefully helping and not muddying, adding an alternate patch to remove the whole search (index, query, operator) if there is no value provided
Comment 23 Katrin Fischer 2020-11-03 12:14:16 UTC
(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 24 David Cook 2020-11-03 23:03:58 UTC
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?
Comment 25 David Nind 2020-11-03 23:04:38 UTC
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>
Comment 26 David Cook 2020-11-03 23:27:56 UTC
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!
Comment 27 David Cook 2020-11-03 23:30:33 UTC
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
Comment 28 Jonathan Druart 2020-11-05 08:28:21 UTC
To prevent the values to be sent maybe we could disable the elements instead of removing them from the DOM. What do you think?
Comment 29 Nick Clemens 2020-11-05 19:01:31 UTC
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
Comment 30 Nick Clemens 2020-11-05 19:01:34 UTC
Created attachment 113134 [details] [review]
Bug 7607: (follow-up) Disable rather than remove inputs
Comment 31 Nick Clemens 2020-11-05 19:03:37 UTC
(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
Comment 32 David Cook 2020-11-05 22:42:17 UTC
(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.
Comment 33 David Cook 2020-11-05 22:43:28 UTC
(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.)
Comment 34 Nick Clemens 2020-11-06 14:30:14 UTC
(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
Comment 35 David Cook 2020-11-09 22:56:40 UTC
(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.
Comment 36 Victor Grousset/tuxayo 2020-11-16 19:16:13 UTC
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>
Comment 37 Victor Grousset/tuxayo 2020-11-16 19:16:17 UTC
Created attachment 113683 [details] [review]
Bug 7607: (follow-up) Disable rather than remove inputs

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 38 Victor Grousset/tuxayo 2020-11-16 19:24:55 UTC
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$
Comment 39 Josef Moravec 2020-11-17 09:32:05 UTC
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>
Comment 40 Josef Moravec 2020-11-17 09:32:15 UTC
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>
Comment 41 Josef Moravec 2020-11-17 09:35:08 UTC
I do really like to see this bug fixed.

- QA tools passed.
- Behaviour is correct.
Comment 42 Jonathan Druart 2020-11-17 11:25:21 UTC
1. Those patches don't affect the OPAC
2. Same problem exists with the "limit" parameter.
Comment 43 David Cook 2020-11-17 22:46:53 UTC
(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.
Comment 44 Jonathan Druart 2020-11-18 08:17:18 UTC
(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)
Comment 45 Nick Clemens 2020-11-18 13:19:53 UTC
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
Comment 46 David Cook 2020-11-19 01:05:22 UTC
(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...
Comment 47 Jonathan Druart 2020-11-19 09:23:32 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2020-11-19 09:36:59 UTC
(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)
Comment 49 Martin Renvoize 2020-11-20 11:15:06 UTC
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>
Comment 50 Martin Renvoize 2020-11-20 11:15:10 UTC
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>
Comment 51 Martin Renvoize 2020-11-20 11:15:14 UTC
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>
Comment 52 Martin Renvoize 2020-11-20 11:15:54 UTC
This works really well, and I can't replicate any issues with using the back button either.

Passing QA
Comment 53 Jonathan Druart 2020-11-20 14:42:43 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 54 Andrew Fuerste-Henry 2020-12-10 22:04:05 UTC
backported to 20.05.x for 20.05.07
Comment 55 Victor Grousset/tuxayo 2020-12-11 16:40:43 UTC
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)