Bug 20114 - Build better $query_cgi
Summary: Build better $query_cgi
Status: RESOLVED DUPLICATE of bug 22413
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-31 14:58 UTC by David Gustafsson
Modified: 2019-06-10 14:52 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
Bug 20114: Build better $query_cgi (4.00 KB, patch)
2018-01-31 15:12 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20114: (follow-up) Don't add sort in query cgi (1.12 KB, patch)
2018-03-16 11:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Alternative solution (8.36 KB, patch)
2018-10-11 13:23 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2018-01-31 14:58:53 UTC
The current code for building $query_cgi in build_query_compat generates an invalid query string for many searches (advanced search, clicking a search link in biblio details etc). When the $query_cgi is incorrect, for example using pagination results in a search error. Have tried to fix this, and while there might still be issues, now works considerably better than before.
Comment 1 David Gustafsson 2018-01-31 15:12:32 UTC
Created attachment 71089 [details] [review]
Bug 20114: Build better $query_cgi

Build a more complete $query_cgi fixing issues with pagination after
performing either an advanced search or visiting a search link from
biblio details.

To test:
1) Go to the details page for a biblio.
2) Click a link that will produce enough hits to trigger pagination.
3) Try going to next page.
4) This will produce a search error.
5) Apply patch.
6) Repeat steps 1 to 3.
7) Pagination should now work as expected.

Sponsored-by: Gothenburg University Library
Comment 2 David Gustafsson 2018-03-01 12:38:26 UTC
Discoverd a double-encoding issue, and problem with sort_by being appended multiple times. So probably best to avoid reviewing this issue until this is fixed.
Comment 3 Nick Clemens 2018-03-16 11:43:25 UTC
Created attachment 73025 [details] [review]
Bug 20114: (follow-up) Don't add sort in query cgi

To test:
1 - Do a search for"*:*"
2 - Try to limit using a facet
3 - Get a search error
4 - Note the URL says kw=*:*
5 - Apply patch
6 - Repeate search
7 - Facet should work
8 - Note URL is better formed
9 - Test with advanced searches as well
Comment 4 Nicolas Legrand 2018-03-16 13:25:29 UTC
Works great till 9, where it sent lots of idx=kw:

localhost:9999/cgi-bin/koha/catalogue/search.pl?idx=kw&q=*%3A*&op=and&idx=kw&q=&op=and&idx=kw&q=&limit-yr=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&limit=&sort_by=relevance

and get:

Error: Unable to perform your search. Please try again.
Comment 5 Joonas Kylmälä 2018-10-11 13:23:21 UTC
Created attachment 80410 [details] [review]
Alternative solution

How would you like the solution in the attachment (it is made for older version of koha so it won't apply)? I.e. completely forego the recreation of search query. Actually the recreation code is used in just a couple other places (addbooks.pl, neworderbiblio.pl, opac-search.pl) in Koha so maybe it could be somehow replaced completely.
Comment 6 David Cook 2019-02-15 00:13:12 UTC
Comment on attachment 71089 [details] [review]
Bug 20114: Build better $query_cgi

Review of attachment 71089 [details] [review]:
-----------------------------------------------------------------

::: Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
@@ +45,4 @@
>  use List::MoreUtils qw/ each_array /;
>  use Modern::Perl;
>  use URI::Escape;
> +use URI::QueryParam;

Is this a new dependency?

@@ +269,4 @@
>      );
>  }
>  
> +sub _build_query_query_string {

Can we get a more descriptive name or some comments to clarify what this means?
Comment 7 David Gustafsson 2019-05-22 15:29:03 UTC
I have more or less given up on this one for the current being since the query parsing for the search page CGI is so fubar and probably needs a larger overhaul and refactoring than the approach in this patch. If I remember correctly I tried fixing the current issues of the patch but each fix tended to break other things.
Comment 8 Joonas Kylmälä 2019-05-23 20:20:59 UTC
Hi,

this might be fixed by the bug #22413 but I have not yet tested but it looks very similar to this bug.
Comment 9 Nick Clemens 2019-06-10 14:52:38 UTC
I think David has some points, we could do more here in the future.

As he has abandoned this code and 22413 seems to fix the issues caused by this bug I think we can close

*** This bug has been marked as a duplicate of bug 22413 ***