Bug 24236 - Using quotes in a cataloging search, resulting in multiple pages, will not allow you to advance page
Summary: Using quotes in a cataloging search, resulting in multiple pages, will not al...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-12 23:17 UTC by Todd Goatley
Modified: 2020-06-04 20:37 UTC (History)
9 users (show)

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


Attachments
Bug 24236: Fix pagination to use query_cgi (2.68 KB, patch)
2019-12-13 16:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24236: Fix pagination to use query_cgi (2.74 KB, patch)
2020-01-15 18:43 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24236: Fix pagination to use query_cgi (2.79 KB, patch)
2020-01-15 19:23 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24236: Fix pagination to use query_cgi (2.86 KB, patch)
2020-01-21 12:22 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Todd Goatley 2019-12-12 23:17:08 UTC
When you do a search that has multiple page results in cataloging, if you do that search in quotations it will not allow you to advance the page.
Comment 1 Kyle M Hall 2019-12-13 12:26:29 UTC
Using double quotes, subsequent pages were 'blank'. Singles quotes did not break the paging, but did make the reservoir search come up empty.

https://monosnap.com/file/H3n9b1Xncd7UAGhTrkcOzv0XgSc2sH
Comment 2 Nick Clemens 2019-12-13 16:12:42 UTC
Created attachment 96264 [details] [review]
Bug 24236: Fix pagination to use query_cgi

This is still wrong, we should use page-numbers.inc, but that is a larger refactoring
This patch fixes non-QueryParser searches by simple getting an using the escaped query (query_cgi)

For QueryParser, we do what the QP code does, just uri escape the query - the way that SimpleSearch does the query parsing instead of the usual buildQuery suggest to me that we should probably switch all of this to use the general Zebra search.

The issues with reservoir should be their own bug

To test:
1 - Search for cat in cataloging search
2 - Get results and see you can paginate
3 - Search for "cat"
4 - Get results but subsequent pages empty
5 - Apply patch
6 - Repeat search for "cat"
7 - You can paginate
8 - Enable UseQueryParser syspref (don't worry about installing)
9 - Confirm can still search with quotes and paginate
Comment 3 Myka Kennedy Stephens 2020-01-15 00:49:20 UTC
I cannot replicate the problem. Please provide further direction for testing.

Testing system = koha-testing-docker
Versions tested = master, 19.11.x, 19.05.x
Search engine = Zebra
Search query = cat >> can page through results
Search query = "cat" >> can page through results
Comment 4 Nick Clemens 2020-01-15 13:35:28 UTC
(In reply to Myka Kennedy Stephens from comment #3)
> I cannot replicate the problem. Please provide further direction for testing.
> 
> Testing system = koha-testing-docker
> Versions tested = master, 19.11.x, 19.05.x
> Search engine = Zebra
> Search query = cat >> can page through results
> Search query = "cat" >> can page through results

I I try  search for "a" I end up at this URL:
http://localhost:8081/cgi-bin/koha/cataloguing/addbooks.pl?q=%22a%22

I cannot page forward from there, I get blank pages

If i hit:
http://localhost:8081/cgi-bin/koha/cataloguing/addbooks.pl?q=a

then I can page
Comment 5 ByWater Sandboxes 2020-01-15 18:43:24 UTC
Created attachment 97407 [details] [review]
Bug 24236: Fix pagination to use query_cgi

This is still wrong, we should use page-numbers.inc, but that is a larger refactoring
This patch fixes non-QueryParser searches by simple getting an using the escaped query (query_cgi)

For QueryParser, we do what the QP code does, just uri escape the query - the way that SimpleSearch does the query parsing instead of the usual buildQuery suggest to me that we should probably switch all of this to use the general Zebra search.

The issues with reservoir should be their own bug

To test:
1 - Search for cat in cataloging search
2 - Get results and see you can paginate
3 - Search for "cat"
4 - Get results but subsequent pages empty
5 - Apply patch
6 - Repeat search for "cat"
7 - You can paginate
8 - Enable UseQueryParser syspref (don't worry about installing)
9 - Confirm can still search with quotes and paginate

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 ByWater Sandboxes 2020-01-15 19:23:44 UTC
Created attachment 97413 [details] [review]
Bug 24236: Fix pagination to use query_cgi

This is still wrong, we should use page-numbers.inc, but that is a larger refactoring
This patch fixes non-QueryParser searches by simple getting an using the escaped query (query_cgi)

For QueryParser, we do what the QP code does, just uri escape the query - the way that SimpleSearch does the query parsing instead of the usual buildQuery suggest to me that we should probably switch all of this to use the general Zebra search.

The issues with reservoir should be their own bug

To test:
1 - Search for cat in cataloging search
2 - Get results and see you can paginate
3 - Search for "cat"
4 - Get results but subsequent pages empty
5 - Apply patch
6 - Repeat search for "cat"
7 - You can paginate
8 - Enable UseQueryParser syspref (don't worry about installing)
9 - Confirm can still search with quotes and paginate

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: George Williams <george@nekls.org>
Comment 7 Jonathan Druart 2020-01-21 12:22:48 UTC
Created attachment 97657 [details] [review]
Bug 24236: Fix pagination to use query_cgi

This is still wrong, we should use page-numbers.inc, but that is a larger refactoring
This patch fixes non-QueryParser searches by simple getting an using the escaped query (query_cgi)

For QueryParser, we do what the QP code does, just uri escape the query - the way that SimpleSearch does the query parsing instead of the usual buildQuery suggest to me that we should probably switch all of this to use the general Zebra search.

The issues with reservoir should be their own bug

To test:
1 - Search for cat in cataloging search
2 - Get results and see you can paginate
3 - Search for "cat"
4 - Get results but subsequent pages empty
5 - Apply patch
6 - Repeat search for "cat"
7 - You can paginate
8 - Enable UseQueryParser syspref (don't worry about installing)
9 - Confirm can still search with quotes and paginate

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: George Williams <george@nekls.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Martin Renvoize 2020-01-23 09:10:47 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Joy Nelson 2020-02-06 22:55:04 UTC
Pushed to 19.11.x branch for 19.11.03
Comment 10 Lucas Gass 2020-02-13 20:15:52 UTC
backported to 19.05.x for 19.05.08
Comment 11 Hayley Mapley 2020-02-14 03:09:09 UTC
Is someone able to also test 18.11.x for the presence of this bug?

I have tested, and I am able to paginate with results still coming up even when searching with "cat".
Comment 12 Jonathan Druart 2020-02-14 08:25:17 UTC
(In reply to Hayley Mapley from comment #11)
> Is someone able to also test 18.11.x for the presence of this bug?
> 
> I have tested, and I am able to paginate with results still coming up even
> when searching with "cat".

I do recreate with "cat" with the double quote :)

Page 2 link is /cgi-bin/koha/cataloguing/addbooks.pl?q=

With the patch (it applies correctly on 18.11.x), I get
/cgi-bin/koha/cataloguing/addbooks.pl?idx=kw&q=%22cat%22&page=2