Bug 21405 - Pagination in authorities search broken for Zebra and broken for 10000+ results in ES
Summary: Pagination in authorities search broken for Zebra and broken for 10000+ resul...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 21527 21636 (view as bug list)
Depends on: 20261
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-25 08:08 UTC by Hugo Agud
Modified: 2018-12-19 13:35 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21405: Fix authorities pagination for Zebra and ES (3.56 KB, patch)
2018-10-04 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21405: (follow-up) C4::Auth is imported twice (691 bytes, patch)
2018-10-06 07:58 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 21405: Fix authorities pagination for Zebra and ES (3.71 KB, patch)
2018-10-06 09:21 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 21405: (follow-up) C4::Auth is imported twice (834 bytes, patch)
2018-10-06 09:21 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 21405 - Fix auhtorities pagination for Zebra and ES (2.51 KB, patch)
2018-12-03 13:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21405 - Fix authorities pagination for Zebra and ES (2.54 KB, patch)
2018-12-04 06:02 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21405 - Fix authorities pagination for Zebra and ES (2.99 KB, patch)
2018-12-04 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21405 - Fix authorities pagination for Zebra and ES (3.05 KB, patch)
2018-12-04 14:13 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21405 - Fix authorities pagination for Zebra and ES (3.13 KB, patch)
2018-12-07 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Hugo Agud 2018-09-25 08:08:19 UTC
we have checked that at staff interface when you try to broswe authorities, you always get the same result list, except one for the last result.

we have checked at 18.05.01 and 18.05.03 from our servers and demo servers, and same result.

test plan
1. login into staff interface
2. Go to authorities
3. Search full authotities (get more than one page results)
4. go to second page... the same results stills there
Comment 1 Nick Clemens 2018-10-04 13:28:50 UTC
Created attachment 79948 [details] [review]
Bug 21405: Fix authorities pagination for Zebra and ES

This patch corrects a problem introduced by bug 20261

The underlying problem still exists - the two search_auth_compat methods
don't expect the same information. Using the 'options' parameter allows
us to work around this for ES.

Additionally code is added to avoid asking for more results form ES than
are returned.

To test:
 1 - Perform an authorities search with 1+ pages using Zebra
 2 - Page through results, note you are only offseeting by 1 each time
 3 - Perform an authorities search with 10,000+ results in ES
 4 - Click on the last page and get an error
 5 - Apply patch
 6 - Retry Zebra search
 7 - Results should paginate correctly
 8 - Clicking on last page should return last results
 9 - Retry ES results
10 - Results should paginate correctly
11 - Clicking on last page hsould return last results
Comment 2 Ere Maijala 2018-10-04 13:43:02 UTC
Nick, bug 19365 includes a fix for search_auth_compat in ES.
Comment 3 Hugo Agud 2018-10-04 13:50:56 UTC
I have tested with zebra and it works fine, but I am not able to test on ES.. waitting for other tester
Comment 4 Nick Clemens 2018-10-04 13:52:10 UTC
(In reply to Ere Maijala from comment #2)
> Nick, bug 19365 includes a fix for search_auth_compat in ES.

Indeed - it looks like there is no conflict - this patch fixes it for now and still works after your patch

We still need this to restore passing the offset directly and handling 10000+ results - whichever gets pushed first will need to rebase on the other
Comment 5 Andreas Roussos 2018-10-06 07:58:35 UTC
Created attachment 80157 [details] [review]
Bug 21405: (follow-up) C4::Auth is imported twice

In authorities/authorities-home.pl C4::Auth is imported twice.
Comment 6 Séverine Queune 2018-10-06 09:21:32 UTC
Created attachment 80158 [details] [review]
Bug 21405: Fix authorities pagination for Zebra and ES

This patch corrects a problem introduced by bug 20261

The underlying problem still exists - the two search_auth_compat methods
don't expect the same information. Using the 'options' parameter allows
us to work around this for ES.

Additionally code is added to avoid asking for more results form ES than
are returned.

To test:
 1 - Perform an authorities search with 1+ pages using Zebra
 2 - Page through results, note you are only offseeting by 1 each time
 3 - Perform an authorities search with 10,000+ results in ES
 4 - Click on the last page and get an error
 5 - Apply patch
 6 - Retry Zebra search
 7 - Results should paginate correctly
 8 - Clicking on last page should return last results
 9 - Retry ES results
10 - Results should paginate correctly
11 - Clicking on last page hsould return last results

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 7 Séverine Queune 2018-10-06 09:21:36 UTC
Created attachment 80159 [details] [review]
Bug 21405: (follow-up) C4::Auth is imported twice

In authorities/authorities-home.pl C4::Auth is imported twice.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 8 Séverine Queune 2018-10-06 09:22:29 UTC
(In reply to Hugo Agud from comment #3)
> I have tested with zebra and it works fine, but I am not able to test on
> ES.. waitting for other tester

Works fine with ES !
Comment 9 Martin Renvoize 2018-10-08 17:00:13 UTC
Should there not be a test accompanying this for Koha/SearchEngine/Elasticsearch/Search.pm search_auth_compat to ensure we don't get future regressions?
Comment 10 Josef Moravec 2018-10-10 13:21:19 UTC
*** Bug 21527 has been marked as a duplicate of this bug. ***
Comment 11 Pablo Jones 2018-10-24 13:57:50 UTC
*** Bug 21636 has been marked as a duplicate of this bug. ***
Comment 12 Heather 2018-11-29 20:28:26 UTC
Hi!  I just wanted to chime in that as a cataloger, I want to thank everyone who is working on this bug!  This bug is causing me a lot of extra work: I have to do all my authority searching in the public OPAC, then switch back to the staff client to search the authority record(s) in ways that I know they will be retrieved in order to edit, update, etc., or even just look at the MARC record in an efficient way.  If I don't take all that time & forget to do all my searching in the public OPAC, then I run the risk of importing duplicate authority records, creating duplicate authority records, etc.  My workflow to clean up our headings & authority file is not just impacted, but my workflow for every single title that I catalog is impacted, since every, single authority controlled access point requires searching the authority file in the OPAC...switching to staff client...etc., etc., etc.

So my hearty thanks to all of you working to fix this bug--I'm cheering you on and really appreciate your efforts!!

--Grateful librarian h2.
Comment 13 Nick Clemens 2018-12-03 13:10:45 UTC
Created attachment 82824 [details] [review]
Bug 21405 - Fix auhtorities pagination for Zebra and ES

Bug 20261 introduced an error because search_auth_compat responded
differently for ES and Zebra, now bug 19365 fixed the underlying
difference. This patch restores previous code to fix pagination.

Additionally we add a fix for 10000+ results in ES and remove a double
import of C4::Auth

To test:
 1 - Perform an authorities search with 1+ pages using Zebra
 2 - Page through results, note you are only offseeting by 1 each time
 3 - Perform an authorities search with 10,000+ results in ES
 4 - Click on the last page and get an error
 5 - Apply patch
 6 - Retry Zebra search
 7 - Results should paginate correctly
 8 - Clicking on last page should return last results
 9 - Retry ES results
10 - Results should paginate correct
11 - Clicking on last page should return last results
Comment 14 Nick Clemens 2018-12-03 13:12:09 UTC
(In reply to Martin Renvoize from comment #9)
> Should there not be a test accompanying this for
> Koha/SearchEngine/Elasticsearch/Search.pm search_auth_compat to ensure we
> don't get future regressions?

Rebased on top of 19365 - that added some basic tests, I have talked with Ere and we agree we need to work towrds actual search testing, then we can add selenium for this, but for now I think we need to fix this issue asap
Comment 15 Martin Renvoize 2018-12-03 14:20:42 UTC
I pretty much agree with the above.. perhaps a more concerted effort for increasing search test coverage elsewhere and get this particular bug pushed through.. I'm happy to go with that, and defer to Ere as ES Topic Expert (and of course the RM, Nick)
Comment 16 Ere Maijala 2018-12-04 06:02:10 UTC
Created attachment 82830 [details] [review]
Bug 21405 - Fix authorities pagination for Zebra and ES

Bug 20261 introduced an error because search_auth_compat responded
differently for ES and Zebra, now bug 19365 fixed the underlying
difference. This patch restores previous code to fix pagination.

Additionally we add a fix for 10000+ results in ES and remove a double
import of C4::Auth

To test:
 1 - Perform an authorities search with 1+ pages using Zebra
 2 - Page through results, note you are only offseeting by 1 each time
 3 - Perform an authorities search with 10,000+ results in ES
 4 - Click on the last page and get an error
 5 - Apply patch
 6 - Retry Zebra search
 7 - Results should paginate correctly
 8 - Clicking on last page should return last results
 9 - Retry ES results
10 - Results should paginate correct
11 - Clicking on last page should return last results

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 17 Ere Maijala 2018-12-04 06:04:24 UTC
I took the liberty of removing addition of "use POSIX qw(ceil);" since it wasn't actually used.
Comment 18 Ere Maijala 2018-12-04 06:55:08 UTC
Oops, I didn't test the pagination fix properly. It's not working. I get 1822 pages of results and trying to go to the last page causes an error. It seems that $hits_to_paginate is not being used.
Comment 19 Nick Clemens 2018-12-04 13:45:21 UTC
Created attachment 82838 [details] [review]
Bug 21405 - Fix authorities pagination for Zebra and ES

Bug 20261 introduced an error because search_auth_compat responded
differently for ES and Zebra, now bug 19365 fixed the underlying
difference. This patch restores previous code to fix pagination.

Additionally we add a fix for 10000+ results in ES and remove a double
import of C4::Auth

To test:
 1 - Perform an authorities search with 1+ pages using Zebra
 2 - Page through results, note you are only offseeting by 1 each time
 3 - Perform an authorities search with 10,000+ results in ES
 4 - Click on the last page and get an error
 5 - Apply patch
 6 - Retry Zebra search
 7 - Results should paginate correctly
 8 - Clicking on last page should return last results
 9 - Retry ES results
10 - Results should paginate correct
11 - Clicking on last page should return last results
Comment 20 Ere Maijala 2018-12-04 14:13:35 UTC
Created attachment 82839 [details] [review]
Bug 21405 - Fix authorities pagination for Zebra and ES

Bug 20261 introduced an error because search_auth_compat responded
differently for ES and Zebra, now bug 19365 fixed the underlying
difference. This patch restores previous code to fix pagination.

Additionally we add a fix for 10000+ results in ES and remove a double
import of C4::Auth

To test:
 1 - Perform an authorities search with 1+ pages using Zebra
 2 - Page through results, note you are only offseeting by 1 each time
 3 - Perform an authorities search with 10,000+ results in ES
 4 - Click on the last page and get an error
 5 - Apply patch
 6 - Retry Zebra search
 7 - Results should paginate correctly
 8 - Clicking on last page should return last results
 9 - Retry ES results
10 - Results should paginate correct
11 - Clicking on last page should return last results

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 21 Marcel de Rooy 2018-12-07 10:45:43 UTC
Created attachment 82960 [details] [review]
Bug 21405 - Fix authorities pagination for Zebra and ES

Bug 20261 introduced an error because search_auth_compat responded
differently for ES and Zebra, now bug 19365 fixed the underlying
difference. This patch restores previous code to fix pagination.

Additionally we add a fix for 10000+ results in ES and remove a double
import of C4::Auth

To test:
 1 - Perform an authorities search with 1+ pages using Zebra
 2 - Page through results, note you are only offseeting by 1 each time
 3 - Perform an authorities search with 10,000+ results in ES
 4 - Click on the last page and get an error
 5 - Apply patch
 6 - Retry Zebra search
 7 - Results should paginate correctly
 8 - Clicking on last page should return last results
 9 - Retry ES results
10 - Results should paginate correct
11 - Clicking on last page should return last results

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2018-12-07 10:46:39 UTC
Forgot this one, Nick:
* Commit title does not start with 'Bug XXXXX: ' - 736809b
Comment 23 Nick Clemens 2018-12-11 20:55:39 UTC
Awesome work all!

Pushed to master for 19.05
Comment 24 Martin Renvoize 2018-12-12 10:50:36 UTC
Pushed to 18.11.x for 18.11.01
Comment 25 Jesse Maseto 2018-12-18 19:24:52 UTC
Pushed to 18.05.x for 18.05.07
Comment 26 Fridolin SOMERS 2018-12-19 13:35:58 UTC
Depends on Bug 20261 not in 17.11.x