Bug 6510 - "Sort by" in intranet search alters search and number of hits
Summary: "Sort by" in intranet search alters search and number of hits
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Magnus Enger
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-16 09:18 UTC by Magnus Enger
Modified: 2012-10-25 23:10 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (1.07 KB, patch)
2011-06-16 10:51 UTC, Magnus Enger
Details | Diff | Splinter Review
Revised patch (1.11 KB, patch)
2011-06-16 11:55 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 6510 - [REVISED] Sort by in intranet search alters search and number of hits (1.15 KB, patch)
2011-06-16 13:36 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2011-06-16 09:18:56 UTC
An example: 

* Simple search for "jones" in the intranet gives: 

search.pl?q=jones
55 result(s) found for 'kw,wrdl: jones' in Koha Catalog.

Reordering by e.g. "Title (A-Z)" gives: 

search.pl?x=kw&q=jones&sort_by=title_az
55 result(s) found for 'kw,wrdl: jones' in Koha Catalog.

* Advanced search for "jones" in the default field ("Keyword") gives: 

search.pl?idx=kw&q=jones&idx=kw&idx=kw&sort_by=relevance
55 result(s) found for 'kw,wrdl: jones' in SKSK Catalog.

Reordering by "Title (A-Z)" gives: 

search.pl?x=kw&q=jones&sort_by=title_az
55 result(s) found for 'kw,wrdl: jones' in SKSK Catalog.

idx=kw is turned into x=kw, but we still get the same number of results. 

* Choosing "Author" as the index to search, and then searching for "jones" gives:

search.pl?idx=au&q=jones&idx=kw&idx=kw&sort_by=relevance
41 result(s) found for 'au,wrdl: jones' in SKSK Catalog. 

And here is the biggie, reordering by "Title (A-Z)" gives:

search.pl?x=au&q=jones&sort_by=title_az
55 result(s) found for 'kw,wrdl: jones' in SKSK Catalog.

Choosing a different sort criteria results in a different number of hits, presumably because idx=au has been turned into x=au.

Searching for "jones" as "Author" and choosing to sort by "Title (A-Z)" on the advanced search page does gives the expected result: 

search.pl?idx=au&q=jones&idx=kw&idx=kw&sort_by=title_az
41 result(s) found for 'au,wrdl: jones' in SKSK Catalog.

The same thing seems to hapen no matter what index you choose to limit your search to, and what criteria you choose to sort on. So why does re-sorting turn idx into x?
Comment 1 Magnus Enger 2011-06-16 10:37:20 UTC
After some more digging, it looks like the problem stems from the regex on line 474 of catalogue/search.pl: 
 
472 for my $this_cgi ( split('&',$query_cgi) ) {
473     next unless $this_cgi;
474     $this_cgi =~ m/(.?)=(.*)/;
475     my $input_name = $1;
476     my $input_value = $2;
477     push @query_inputs, { input_name => $input_name, input_value => $input_value };
478         if ($input_name eq 'idx') {
479         $scan_index_to_use = $input_value; # unless $scan_index_to_use;
480         }
481 }
482 $template->param ( QUERY_INPUTS => \@query_inputs,
483                    scan_index_to_use => $scan_index_to_use );

.? is non-greedy and matches the minimum number of characters, which is the "x" part of "idx", resulting in the behaviour described earlier. A patch proposing to change that line to this: 

$this_cgi =~ m/(.*)=(.*)/;

is coming up...
Comment 2 Magnus Enger 2011-06-16 10:51:16 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2011-06-16 10:57:50 UTC
The problem is not present in the OPAC, where this code takes care of the parsing: 

 368 sub _input_cgi_parse ($) { 
 369     my @elements;
 370     for my $this_cgi ( split('&',shift) ) {
 371         next unless $this_cgi;
 372         $this_cgi =~ /(.*?)=(.*)/;
 373         push @elements, { input_name => $1, input_value => $2 };
 374     }
 375     return @elements;
 376 }

Not sure if there is any practical difference between my proposed fix:

$this_cgi =~ /(.*)=(.*)/;

and the one used in opac/opac-search.pl: 

$this_cgi =~ /(.*?)=(.*)/;

?
Comment 4 MJ Ray (software.coop) 2011-06-16 11:42:06 UTC
I think the correct fix would be the one from the OPAC.

I think:
.? matches 0 or 1 characters which seems incorrect;
.*= matches any characters, potentially up to the last =, which seems incorrect;
.*?= matches any characters up to the first =.

I'm not QA so I'm putting this back from "needs signoff" to "---".  Hope that's OK.
Comment 5 Magnus Enger 2011-06-16 11:55:19 UTC Comment hidden (obsolete)
Comment 6 MJ Ray (software.coop) 2011-06-16 13:36:17 UTC
Created attachment 4499 [details] [review]
Bug 6510 - [REVISED] Sort by in intranet search alters search and number of hits

This patch uses .*? instead of .*

To test:
- In advanced search in the intranet choose Author as the search index
- Do a search for an author, check the number of hits
- Choose another value than the default from Sort by
- Check that the number of hits is the same as for the original search, once
  the hits have been re-ordered

Signed-off-by: MJ Ray <mjr@phonecoop.coop>
Comment 7 Chris Cormack 2011-06-25 20:29:09 UTC
Pushed please test
Comment 8 Maxime Pelletier 2011-08-26 20:28:58 UTC
Tested and the patch applied in 3.4.2 fixes the problem.