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: 2021-04-21 13:54 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
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 ***
Comment 10 Gérard Simon Voyer 2021-04-21 13:44:13 UTC
Sadly I'll have to bump this one as still open.

On our side it seems like a regression from 19.05, but it could come from our weird branch setup and my misunderstanding of Koha.

Here's an example from our dev server (excuse my bad knowledge of procedures, I don't think I've seen anyone sharing direct links to their servers) :

1 -- http://dev.koha.collecto.ca/cgi-bin/koha/opac-search.pl?advsearch=1&idx=kw&q=&op=and&idx=kw&q=&op=and&idx=kw&q=&limit-yr=1900-1901&limit=&limit=&sort_by=pubdate_dsc&limit=&limit=&limit=&limit=

2 -- Sort the results Title A-Z (or any option actually)

3 -- Anything query-related is gone from the URL, except for the new sort_by parameter and a weird &addto=Add+to... that seems to be taken from the results box.

Any pointers would be appreciated. At this point I feel pretty defeated, as I'm a new developer fresh outta Internet school where they teach you JS and Python. Perl seems to me like a huge monster trying to eat me alive every day.
Comment 11 Gérard Simon Voyer 2021-04-21 13:54:38 UTC
Ok, news update! It does come from our branches setup. Carry on with your lives, I'll make sure to use Koha's stable version instead of the fixed version we have with our «home-made» Koha branch.