Bug 14715 - Results per page setting for catalog search in staff client and OPAC
Summary: Results per page setting for catalog search in staff client and OPAC
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact:
URL:
Keywords:
: 7260 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-08-24 19:30 UTC by Martin Stenberg
Modified: 2020-05-20 09:06 UTC (History)
23 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
This enhancement adds a 'results per page' dropdown list to catalog search results pages in the OPAC and staff interface. This lets you set the number of results to show (20, 40, 60, 80, and so on). This is enabled by two new system preferences: `OPACnumSearchResultsDropdown` for the OPAC, and `numSearchResultsDropdown` for the staff interface. The default number of search results is set using existing system preferences: `OPACnumSearchResults` for the OPAC, and `numSearchResults` for the staff interface. This enhancement works for both Zebra and Elasticsearch search engines. **New system preferences**: `OPACnumSearchResultsDropdown` and `numSearchResultsDropdown` both default to disabled.
Version(s) released in:
20.05.00


Attachments
Bug 14715: results per page setting for catalog search (11.17 KB, patch)
2015-08-25 18:24 UTC, Martin Stenberg
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (11.23 KB, patch)
2015-08-27 03:34 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: syspref for results per page options (9.58 KB, patch)
2015-09-21 16:51 UTC, Martin Stenberg
Details | Diff | Splinter Review
Bug 14715: database updates (1.18 KB, patch)
2015-09-21 16:51 UTC, Martin Stenberg
Details | Diff | Splinter Review
Bug 14715: Fixing merge conflicts etc. (1.80 KB, patch)
2016-10-04 03:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (11.07 KB, patch)
2016-10-04 03:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: syspref for results per page options (2.62 KB, patch)
2016-10-04 03:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: database updates (1.18 KB, patch)
2016-10-04 03:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (11.08 KB, patch)
2016-10-17 03:10 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: syspref for results per page options (1.59 KB, patch)
2016-10-17 03:10 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: database updates (1.19 KB, patch)
2016-10-17 03:10 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (11.31 KB, patch)
2016-10-18 00:34 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: syspref for results per page options (1.62 KB, patch)
2016-10-18 00:34 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: database updates (1.18 KB, patch)
2016-10-18 00:34 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (13.25 KB, patch)
2016-12-02 00:19 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (12.35 KB, patch)
2017-03-30 00:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues (8.20 KB, patch)
2017-06-13 04:51 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (12.37 KB, patch)
2017-10-10 22:06 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues (4.46 KB, patch)
2017-10-10 22:25 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (12.42 KB, patch)
2017-10-12 13:17 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues (4.50 KB, patch)
2017-10-12 13:17 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues (5.88 KB, patch)
2017-10-20 00:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues (5.94 KB, patch)
2017-10-20 15:47 UTC, Simon Pouchol
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (13.11 KB, patch)
2018-06-10 23:47 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues (3.11 KB, patch)
2018-06-10 23:47 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues (5.40 KB, patch)
2018-06-10 23:47 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Rebase and fix up (11.63 KB, patch)
2018-06-11 02:55 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Fix results not showing (1.86 KB, patch)
2018-08-28 21:47 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (13.20 KB, patch)
2018-08-30 04:10 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues (3.16 KB, patch)
2018-08-30 04:10 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues (5.47 KB, patch)
2018-08-30 04:10 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Rebase and fix up (11.71 KB, patch)
2018-08-30 04:10 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14715: [FOLLOW-UP] Fix results not showing (1.92 KB, patch)
2018-08-30 04:11 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14715: (follow-up) some code changes (4.46 KB, patch)
2018-10-02 00:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (follow-up) some code changes (3.19 KB, patch)
2018-10-02 00:55 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (follow-up) Note about elasticsearch (1.48 KB, patch)
2018-10-05 02:03 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (follow-up) Edits for ElasticSearch (4.21 KB, patch)
2018-10-25 22:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (13.30 KB, patch)
2019-04-16 19:24 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (13.30 KB, patch)
2019-04-16 19:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: [QA Follow up] Fixing qa-tools issues (3.21 KB, patch)
2019-04-16 19:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: [QA Follow up] Removing 'all' option, fixing QA tools issues (5.51 KB, patch)
2019-04-16 19:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: [QA Follow up] Rebase and fix up (11.76 KB, patch)
2019-04-16 19:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: (QA Follow up) Fix results not showing (1.97 KB, patch)
2019-04-16 19:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: (QA follow-up) some code changes (3.25 KB, patch)
2019-04-16 19:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: (QA follow-up) Edits for ElasticSearch (4.28 KB, patch)
2019-04-16 19:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: (QA Follow up) More QA tool cleanup (6.79 KB, patch)
2019-04-16 19:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: (follow-up) Fixing database updates (5.53 KB, patch)
2019-04-17 01:46 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (follow-up) Fixing database updates (5.60 KB, patch)
2019-04-17 14:17 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14715: results per page setting for catalog search (14.93 KB, patch)
2019-04-26 02:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [QA Follow up] Fixing qa-tools issues (3.20 KB, patch)
2019-04-26 02:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [QA Follow up] Removing 'all' option, fixing QA tools issues (5.49 KB, patch)
2019-04-26 02:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: [QA Follow up] Rebase and fix up (11.69 KB, patch)
2019-04-26 02:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (QA Follow up) Fix results not showing (1.95 KB, patch)
2019-04-26 02:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (QA follow-up) some code changes (3.23 KB, patch)
2019-04-26 02:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (QA follow-up) Edits for ElasticSearch (4.26 KB, patch)
2019-04-26 02:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (QA Follow up) More QA tool cleanup (6.77 KB, patch)
2019-04-26 02:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (follow-up) Fixing database updates (5.58 KB, patch)
2019-04-26 02:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: Results per page dropdown for catalogue search (20.57 KB, patch)
2019-11-05 03:01 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: Results per page dropdown for catalogue search (19.59 KB, patch)
2020-01-08 15:08 UTC, Gabriel DeCarufel
Details | Diff | Splinter Review
Bug 14715: Results per page dropdown for catalogue search (19.68 KB, patch)
2020-02-04 09:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results (28.49 KB, patch)
2020-02-25 20:44 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results (28.75 KB, patch)
2020-02-26 22:05 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14715: Results per page dropdown for catalogue search (19.73 KB, patch)
2020-02-27 07:58 UTC, David Nind
Details | Diff | Splinter Review
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results (28.83 KB, patch)
2020-02-27 07:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 14715: Results per page dropdown for catalogue search (19.79 KB, patch)
2020-03-25 21:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results (28.89 KB, patch)
2020-03-25 21:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14715: (QA follow-up) Add missing filters (1.62 KB, patch)
2020-03-25 21:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14715: (QA follow-up) Remove limit on Zebra (3.74 KB, patch)
2020-03-25 21:41 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Stenberg 2015-08-24 19:30:41 UTC
Add a select field to the search results page (both in staff client and opac) where user can select how many results per page should be shown. For instance: 20/40/60/80/100/all
Comment 1 Martin Stenberg 2015-08-25 18:24:54 UTC Comment hidden (obsolete)
Comment 2 Aleisha Amohia 2015-08-27 03:34:48 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-08-27 09:22:50 UTC
I would prefer to see a version of this patch using sessionStorage instead of cookies.
Comment 4 Martin Stenberg 2015-09-21 12:32:51 UTC
I have now investigated the option of using sessionStorage instead of a cookie. However this would, what I can see, either:

require to send the sessionStorage variable to the server for every search, which means modifying all scripts requesting the (opac-)search.pl files. In this case it would not be much different from using a cookie.

or 

require the (opac-)search.pl scripts to get the maximum number of search results ("all", or perhaps limit to 100) and then filter them on client side. This would require a lot of extra unnecessary work for the server.

Neither option seems very attractiv to me. Perhaps there's a third option which I'm missing?
Comment 5 Jonathan Druart 2015-09-21 14:37:22 UTC
Martin,

I am really sorry, this was a stupid suggestion!

I had a look again at your patch and I have just two concerns:
1/ If the number of results is large (say more than 1000), we don't want to allow the user to display all of the results.
The solution would be to add a syspref, to let the choice to the library, or not display this entry.
2/ The value of the [OPAC]numSearchResults pref should appear in the options.

Marked as Failed QA.
Comment 6 Martin Stenberg 2015-09-21 16:51:24 UTC Comment hidden (obsolete)
Comment 7 Martin Stenberg 2015-09-21 16:51:59 UTC Comment hidden (obsolete)
Comment 8 Aleisha Amohia 2016-10-04 03:08:01 UTC Comment hidden (obsolete)
Comment 9 Aleisha Amohia 2016-10-04 03:38:39 UTC Comment hidden (obsolete)
Comment 10 Aleisha Amohia 2016-10-04 03:38:47 UTC Comment hidden (obsolete)
Comment 11 Aleisha Amohia 2016-10-04 03:38:53 UTC Comment hidden (obsolete)
Comment 12 Aleisha Amohia 2016-10-04 03:40:51 UTC
These patches should now apply cleanly and are ready for testing.

To test:
1) Do a catalog search in the intranet and OPAC
2) Confirm that results automatically update upon selection of results
per page to show
3) Test for all options (20, 40, 60, 80, 100, all)
Comment 13 Jokin Perez de Viñaspre 2016-10-06 09:55:08 UTC
Hi!

If the results of the search is bigger than the  page size, the pagination appears but it doesnt work.
Comment 14 Marc Véron 2016-10-06 13:43:14 UTC
Wanted to test, but sorry, does not apply.
Comment 15 Aleisha Amohia 2016-10-17 03:10:19 UTC Comment hidden (obsolete)
Comment 16 Aleisha Amohia 2016-10-17 03:10:28 UTC Comment hidden (obsolete)
Comment 17 Aleisha Amohia 2016-10-17 03:10:36 UTC Comment hidden (obsolete)
Comment 18 Jokin Perez de Viñaspre 2016-10-17 08:42:40 UTC
Hi!

The pagination doesnt work if the result of the search is bigger than the results_per_page.
Comment 19 Aleisha Amohia 2016-10-18 00:34:25 UTC Comment hidden (obsolete)
Comment 20 Aleisha Amohia 2016-10-18 00:34:32 UTC Comment hidden (obsolete)
Comment 21 Aleisha Amohia 2016-10-18 00:34:39 UTC Comment hidden (obsolete)
Comment 22 Aleisha Amohia 2016-10-18 00:35:44 UTC
Patches reattached to fix merge conflicts.

(In reply to Jokin Perez de Viñaspre from comment #18)
> Hi!
> 
> The pagination doesnt work if the result of the search is bigger than the
> results_per_page.

Are you running updatedatabase.pl ?
Comment 23 Jokin Perez de Viñaspre 2016-10-25 09:02:52 UTC
Hi!

Yes, I runned updatedatabase.pl

To solve the problem of pagination I change one line in a/opac/opac-search.pl(line 527) and a/catalogue/search.pl(line 456) of your patch

my $offset = $results_per_page != 'all' ? ($page-1)*$results_per_page : 0;
to: 
my $offset = $params->{'offset'} || 0;

With that change works fine.
Comment 24 Aleisha Amohia 2016-12-02 00:19:15 UTC Comment hidden (obsolete)
Comment 25 julien.comte 2017-03-20 15:59:00 UTC
I made two searchs. It worked just fine for both of them.
Comment 26 Jonathan Druart 2017-03-22 18:33:44 UTC
I do not think it is a good idea to provide 'all' at the OPAC. With only one query you can kill a server for several minutes :)
Comment 27 Jonathan Druart 2017-03-22 18:36:04 UTC
From logs:
Argument "all" isn't numeric in numeric eq (==) at /home/vagrant/kohaclone/opac/opac-search.pl line 637.
Comment 28 Jonathan Druart 2017-03-29 19:15:40 UTC
Failing for comment 27, please answer to comment 26.
Comment 29 Aleisha Amohia 2017-03-29 23:55:15 UTC
(In reply to Jonathan Druart from comment #28)
> Failing for comment 27, please answer to comment 26.

I think you're probably right about this - I will remove the 'all' option for the OPAC.
Comment 30 Aleisha Amohia 2017-03-30 00:27:04 UTC Comment hidden (obsolete)
Comment 31 Marc Véron 2017-04-07 19:44:58 UTC
Sorry, QA tools complain:

FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
   FAIL	  tt_valid
		lines 368, 369, 370, 371, 372, 373

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
   FAIL	  tt_valid
		lines 151, 152, 153, 154, 155


Suppose it is because of:
https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML1:_Template_Toolkit_markup_inside_HTML
Comment 32 Aleisha Amohia 2017-06-13 04:51:24 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2017-10-07 23:51:31 UTC
Patch doesn't apply anymore, but this sounds like an interesting feature. Can someone rebase?


Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14715: results per page setting for catalog search
Using index info to reconstruct a base tree...
M	C4/Search.pm
M	catalogue/search.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
M	opac/opac-search.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
CONFLICT (content): Merge conflict in opac/opac-search.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
Auto-merging catalogue/search.pl
CONFLICT (content): Merge conflict in catalogue/search.pl
Auto-merging C4/Search.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14715: results per page setting for catalog search
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14715-results-per-page-setting-for-catalog-sea-nZTnir.patch
Comment 34 Aleisha Amohia 2017-10-10 22:06:15 UTC Comment hidden (obsolete)
Comment 35 Aleisha Amohia 2017-10-10 22:25:25 UTC Comment hidden (obsolete)
Comment 36 Aleisha Amohia 2017-10-10 22:27:06 UTC
Patches rebased, ready for testing.
Comment 37 Owen Leonard 2017-10-12 13:17:38 UTC Comment hidden (obsolete)
Comment 38 Owen Leonard 2017-10-12 13:17:43 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2017-10-13 09:10:54 UTC
(In reply to Jonathan Druart from comment #26)
> I do not think it is a good idea to provide 'all' at the OPAC. With only one
> query you can kill a server for several minutes :)

For the same reason I would prefer to remove it for Staff too.
Comment 40 Marcel de Rooy 2017-10-13 09:11:26 UTC
FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
   OK     forbidden patterns
   OK     git manipulation
   SKIP   spelling
   FAIL   tt_valid
                lines 151, 152, 153, 154, 155
   OK     valid_template
Comment 41 Aleisha Amohia 2017-10-20 00:27:27 UTC Comment hidden (obsolete)
Comment 42 Simon Pouchol 2017-10-20 15:47:50 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2017-10-23 08:33:23 UTC
QA Comment:

Why do you add in catalogue/search.pl:
+use String::Random;

+@results_per_page_options = do { my %seen; grep { !$seen{$_}++ } @results_per_page_options };
Could you 'do' this without a do statement? Do you need uniq from List::More ?
Is it really needed btw? Do you expect duplicate numbers in this pref..

results.tt
Why are you changing the lines with respect to list counts? What is the relation with this patch?

opac-search.pl
-$offset = ($page-1)*$results_per_page if $page>1;
Why do you remove it ?

-        ($error, $results_hashref, $facets) = C4::Search::pazGetRecords($query,$simple_query,\@sort_by,\@servers,$results_per_page,$offset,$expanded_facet,undef,$query_type,$scan);
+        ($error, $results_hashref, $facets) = C4::Search::pazGetRecords($query,$simple_query,\@sort_by,\@servers,$results_per_page,$offset,$expanded_facet,$query_type,$scan);
You remove a undef parameter.
Why? Is this a bug on its own? What is the relation with this report?

Please clarify or adjust.
Comment 44 Aleisha Amohia 2018-06-10 23:47:32 UTC
Created attachment 75938 [details] [review]
Bug 14715: results per page setting for catalog search

Add new select field to catalog search results pages which allows user to set number
of results to be shown per page. Setting saved in cookie "results_per_page".

To test:
1) Do a catalog search in the intranet and OPAC
2) Confirm that results automatically update upon selection of results
per page to show
3) Test for all options (20, 40, 60, 80, 100, all)

Bug 14715: syspref for results per page options

Remove the "all" option.

Add syspref [OPAC]numSearchResultsPerPage for specifying options for numbers of
results per page including [OPAC]numSearchResults.

Bug 14715: database updates

Bug 14715: Fixing pagination problems

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 45 Aleisha Amohia 2018-06-10 23:47:38 UTC
Created attachment 75939 [details] [review]
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues

and merge conflicts

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 46 Aleisha Amohia 2018-06-10 23:47:45 UTC
Created attachment 75940 [details] [review]
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>
Comment 47 Aleisha Amohia 2018-06-11 02:55:07 UTC
Created attachment 75941 [details] [review]
Bug 14715: [FOLLOW-UP] Rebase and fix up

I noticed that the two sysprefs initially created with this patch
weren't really being used, so I removed them. Instead I've added two new
sysprefs:
numSearchResultsDropdown
OPACnumSearchResultsDropdown
that control whether the dropdown shows or not. Otherwise the defaults
in numSearchResults and OPACnumSearchResults are used to decide number
of search results per page.

When testing:
Run updatedatabase.pl
Test with values in numSearchResults and OPACnumSearchResults. Use
original test plan in Comment 12 but test with the dropdown sysprefs off
and on.

Sponsored-by: Region Halland
Comment 48 Aleisha Amohia 2018-06-11 02:56:38 UTC
(In reply to Marcel de Rooy from comment #43)
> QA Comment:
> 
> Why do you add in catalogue/search.pl:
> +use String::Random;
> 
> +@results_per_page_options = do { my %seen; grep { !$seen{$_}++ }
> @results_per_page_options };
> Could you 'do' this without a do statement? Do you need uniq from List::More
> ?
> Is it really needed btw? Do you expect duplicate numbers in this pref..
> 

This is now gone in latest follow-up.

> results.tt
> Why are you changing the lines with respect to list counts? What is the
> relation with this patch?
> 

This seems unrelated to this patch but is no longer affected in the follow-up.

> opac-search.pl
> -$offset = ($page-1)*$results_per_page if $page>1;
> Why do you remove it ?
> 

Offset is dealt with later.

> -        ($error, $results_hashref, $facets) =
> C4::Search::pazGetRecords($query,$simple_query,\@sort_by,\@servers,
> $results_per_page,$offset,$expanded_facet,undef,$query_type,$scan);
> +        ($error, $results_hashref, $facets) =
> C4::Search::pazGetRecords($query,$simple_query,\@sort_by,\@servers,
> $results_per_page,$offset,$expanded_facet,$query_type,$scan);
> You remove a undef parameter.
> Why? Is this a bug on its own? What is the relation with this report?
> 
> Please clarify or adjust.

Have put undef param back. Not sure why this was removed initially.

Ready for testing.
Comment 49 Cab Vinton 2018-08-05 12:59:35 UTC
Tested in Biblibre Sandbox 05.

Results appear correctly in the intranet when set *manually* on the results page to 20, 40, et.

No results displayed initially, however. This is true whether numSearchResults preference is blank or whether it is set to 20.

Unable to test in the OPAC as all searches lead to "No results found!"
Comment 50 Owen Leonard 2018-08-07 14:07:04 UTC
My tests turned out exactly the same as Cab's in comment 49.
Comment 51 Aleisha Amohia 2018-08-28 21:47:19 UTC
Created attachment 78230 [details] [review]
Bug 14715: [FOLLOW-UP] Fix results not showing

This fixes the display issue for me. Can someone please test?
Comment 52 PTFS Europe Sandboxes 2018-08-30 04:09:16 UTC
Patch tested with a sandbox, by Cab Vinton <bibliwho@gmail.com>
Comment 53 PTFS Europe Sandboxes 2018-08-30 04:10:39 UTC
Created attachment 78298 [details] [review]
Bug 14715: results per page setting for catalog search

Add new select field to catalog search results pages which allows user to set number
of results to be shown per page. Setting saved in cookie "results_per_page".

To test:
1) Do a catalog search in the intranet and OPAC
2) Confirm that results automatically update upon selection of results
per page to show
3) Test for all options (20, 40, 60, 80, 100, all)

Bug 14715: syspref for results per page options

Remove the "all" option.

Add syspref [OPAC]numSearchResultsPerPage for specifying options for numbers of
results per page including [OPAC]numSearchResults.

Bug 14715: database updates

Bug 14715: Fixing pagination problems

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>
Comment 54 PTFS Europe Sandboxes 2018-08-30 04:10:44 UTC
Created attachment 78299 [details] [review]
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues

and merge conflicts

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>
Comment 55 PTFS Europe Sandboxes 2018-08-30 04:10:50 UTC
Created attachment 78300 [details] [review]
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>
Comment 56 PTFS Europe Sandboxes 2018-08-30 04:10:55 UTC
Created attachment 78301 [details] [review]
Bug 14715: [FOLLOW-UP] Rebase and fix up

I noticed that the two sysprefs initially created with this patch
weren't really being used, so I removed them. Instead I've added two new
sysprefs:
numSearchResultsDropdown
OPACnumSearchResultsDropdown
that control whether the dropdown shows or not. Otherwise the defaults
in numSearchResults and OPACnumSearchResults are used to decide number
of search results per page.

When testing:
Run updatedatabase.pl
Test with values in numSearchResults and OPACnumSearchResults. Use
original test plan in Comment 12 but test with the dropdown sysprefs off
and on.

Sponsored-by: Region Halland

Signed-off-by: Cab Vinton <bibliwho@gmail.com>
Comment 57 PTFS Europe Sandboxes 2018-08-30 04:11:01 UTC
Created attachment 78302 [details] [review]
Bug 14715: [FOLLOW-UP] Fix results not showing

This fixes the display issue for me. Can someone please test?

Signed-off-by: Cab Vinton <bibliwho@gmail.com>
Comment 58 Cab Vinton 2018-08-30 04:16:39 UTC
Signed off in PTFS Sandbox 0. But note that the dropdown setting appears to control.

In other words, once the dropdown is displayed, that setting sticks, even if the preference setting is changed or if the user logs out of the staff client or OPAC. This is also true if the dropdown is subsequently hidden.

So, once the dropdown has been displayed, that is where the Results per page must be set & the system preference does not control.
Comment 59 Katrin Fischer 2018-09-25 06:23:37 UTC
Hi Aleisha, 

1) some complaints from the QA script:

 FAIL	installer/data/mysql/atomicupdate/bug_14715-searchresults_syspref.sql
   FAIL	  git manipulation
		The file has been added and deleted in the same patchset

 FAIL	installer/data/mysql/atomicupdate/bug_14715_-_results_per_page_dropdown_sysprefs.sql
   OK	  git manipulation
   FAIL	  semicolon
		simicolon found instead of comma at line 3
   FAIL	  sysprefs_order
		Not blocker: Sysprefs numSearchResultsDropdown is bad placed (see bug 10610)

It appears the change to sysprefs.sql is missing?

2) I think I would just remove the display of 'library default' in the OPAC and staff. I think we don't need the extra entry in the pull down and it's more confusing as the difference between Library default:20 and 20 is not clear.

3) It appears to break when Elasticsearch is activated in a non-good way. Switching the number of results made the OPAC load forever for me showing a white page. Then it started to work, but the number of pages doesn't change.
Can you please check? If we can't make this work, we should at least add a note to the pref and keep it from showing up with Elasticsearch active. 

Note for later:
New cookie needs to be documented in the wiki, once this has been pushed:
https://wiki.koha-community.org/wiki/Use_of_Cookies
Comment 60 Aleisha Amohia 2018-10-02 00:30:48 UTC
Created attachment 79796 [details] [review]
Bug 14715: (follow-up) some code changes

1) adds sysprefs to sysprefs.sql
2) removes a merge conflict error that should not have been added
Comment 61 Aleisha Amohia 2018-10-02 00:55:06 UTC
Created attachment 79797 [details] [review]
Bug 14715: (follow-up) some code changes

1) adds sysprefs to sysprefs.sql
2) removes a merge conflict error that should not have been added
Comment 62 Aleisha Amohia 2018-10-05 02:03:15 UTC
Created attachment 79988 [details] [review]
Bug 14715: (follow-up) Note about elasticsearch

I'm not sure how to hide the sysprefs if ElasticSearch is active so I've
added some comments to the preferences.

Ready to test
Comment 63 Séverine Queune 2018-10-24 12:30:05 UTC
Tested with Zebra : works fine !

Tested with Elasticsearch :  works fine except for "60 results per page" (UI + OPAC) : each time there is more than 10000 results, using "Last" button failed : "Error: Unable to perform your search. Please try again."

As it's not supposed to work with ES, I don't know if I should change the status to "Failed" or sign the patch...
Comment 64 Aleisha Amohia 2018-10-25 22:05:04 UTC
Created attachment 81203 [details] [review]
Bug 14715: (follow-up) Edits for ElasticSearch

Note added to preferences that the dropdown should not be used with
ElasticSearch activated.
Dropdowns do not show on search results on staff side or OPAC side if
Elasticsearch enabled.

Ready to test
Comment 65 Aleisha Amohia 2018-10-25 22:06:06 UTC
(In reply to Séverine Queune from comment #63)
> Tested with Zebra : works fine !
> 
> Tested with Elasticsearch :  works fine except for "60 results per page" (UI
> + OPAC) : each time there is more than 10000 results, using "Last" button
> failed : "Error: Unable to perform your search. Please try again."
> 
> As it's not supposed to work with ES, I don't know if I should change the
> status to "Failed" or sign the patch...

Hi there! Can you please test again - i have hidden the dropdown if Elasticsearch is enabled to avoid this error.
Comment 66 Liz Rea 2019-04-16 19:23:46 UTC
I'm going to sign off on this despite these two things: 

 FAIL	installer/data/mysql/atomicupdate/bug_14715-searchresults_syspref.sql
   FAIL	  git manipulation
		The file has been added and deleted in the same patchset

^^ I couldn't figure out how to resolve that, someone share plz.

 FAIL	installer/data/mysql/atomicupdate/bug_14715_-_results_per_page_dropdown_sysprefs.sql
   OK	  git manipulation
   OK	  semicolon
   FAIL	  sysprefs_order
		Not blocker: Sysprefs numSearchResultsDropdown is bad placed (see bug 10610)

^^ isn't a blocker so I'm not letting it block me.
Comment 67 Liz Rea 2019-04-16 19:24:50 UTC
Created attachment 88097 [details] [review]
Bug 14715: results per page setting for catalog search

Add new select field to catalog search results pages which allows user to set number
of results to be shown per page. Setting saved in cookie "results_per_page".

To test:
1) Do a catalog search in the intranet and OPAC
2) Confirm that results automatically update upon selection of results
per page to show
3) Test for all options (20, 40, 60, 80, 100, all)

Bug 14715: syspref for results per page options

Remove the "all" option.

Add syspref [OPAC]numSearchResultsPerPage for specifying options for numbers of
results per page including [OPAC]numSearchResults.

Bug 14715: database updates

Bug 14715: Fixing pagination problems

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 68 Liz Rea 2019-04-16 19:30:09 UTC
Created attachment 88102 [details] [review]
Bug 14715: results per page setting for catalog search

Add new select field to catalog search results pages which allows user to set number
of results to be shown per page. Setting saved in cookie "results_per_page".

To test:
1) Do a catalog search in the intranet and OPAC
2) Confirm that results automatically update upon selection of results
per page to show
3) Test for all options (20, 40, 60, 80, 100, all)

Bug 14715: syspref for results per page options

Remove the "all" option.

Add syspref [OPAC]numSearchResultsPerPage for specifying options for numbers of
results per page including [OPAC]numSearchResults.

Bug 14715: database updates

Bug 14715: Fixing pagination problems

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 69 Liz Rea 2019-04-16 19:30:38 UTC
Created attachment 88103 [details] [review]
Bug 14715: [QA Follow up] Fixing qa-tools issues

and merge conflicts

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 70 Liz Rea 2019-04-16 19:31:34 UTC
Created attachment 88104 [details] [review]
Bug 14715: [QA Follow up] Removing 'all' option, fixing QA tools issues

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 71 Liz Rea 2019-04-16 19:31:39 UTC
Created attachment 88105 [details] [review]
Bug 14715: [QA Follow up] Rebase and fix up

I noticed that the two sysprefs initially created with this patch
weren't really being used, so I removed them. Instead I've added two new
sysprefs:
numSearchResultsDropdown
OPACnumSearchResultsDropdown
that control whether the dropdown shows or not. Otherwise the defaults
in numSearchResults and OPACnumSearchResults are used to decide number
of search results per page.

When testing:
Run updatedatabase.pl
Test with values in numSearchResults and OPACnumSearchResults. Use
original test plan in Comment 12 but test with the dropdown sysprefs off
and on.

Sponsored-by: Region Halland

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 72 Liz Rea 2019-04-16 19:31:44 UTC
Created attachment 88106 [details] [review]
Bug 14715: (QA Follow up) Fix results not showing

This fixes the display issue for me. Can someone please test?

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 73 Liz Rea 2019-04-16 19:31:49 UTC
Created attachment 88107 [details] [review]
Bug 14715: (QA follow-up) some code changes

1) adds sysprefs to sysprefs.sql
2) removes a merge conflict error that should not have been added

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 74 Liz Rea 2019-04-16 19:31:53 UTC
Created attachment 88108 [details] [review]
Bug 14715: (QA follow-up) Edits for ElasticSearch

Note added to preferences that the dropdown should not be used with
ElasticSearch activated.
Dropdowns do not show on search results on staff side or OPAC side if
Elasticsearch enabled.

Ready to test

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 75 Liz Rea 2019-04-16 19:31:58 UTC
Created attachment 88109 [details] [review]
Bug 14715: (QA Follow up) More QA tool cleanup

Tools are picking up more stuff now, so a few more things to fix!

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 76 Katrin Fischer 2019-04-16 23:01:07 UTC
System preference names don't match up:

Database updates add 4 different prefs:
OPACnumSearchResultsPerPage
numSearchResultsPerPage
OPACnumSearchResultsDropdown
numSearchResultsDropdown

syspref.sql adds only 2:
numSearchResultsPerPage
OPACnumSearchResultsPerPage

yaml file has:
OPACnumSearchResultsDropdown
numSearchResultsDropdown

Code appears to use the Dropdown version.

Please, when cleaning this up: Database update SQL files should be merged and moved into a .perl file - this will make the RM much happier.

For later:
New cookie results_per_page needs documentation on the wiki.
Comment 77 Aleisha Amohia 2019-04-17 01:46:32 UTC
Created attachment 88126 [details] [review]
Bug 14715: (follow-up) Fixing database updates

This patch removes the atomic update that adds the 'perPage' system
preferences and keeps the 'dropdown' system preferences.

Also sql file is moved into perl file.
Comment 78 Liz Rea 2019-04-17 14:17:28 UTC
Created attachment 88195 [details] [review]
Bug 14715: (follow-up) Fixing database updates

This patch removes the atomic update that adds the 'perPage' system
preferences and keeps the 'dropdown' system preferences.

Also sql file is moved into perl file.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 79 Liz Rea 2019-04-17 14:19:01 UTC
we might want to rebase out the old .sql files, but I don't want to mess it up so I will leave that bit. That's the only QA fail I can see, the newest patch resolves Katrin's concerns, near as I can tell.
Comment 80 Katrin Fischer 2019-04-25 06:19:10 UTC
Hi Aleisha, I am sorry, can you please rebase?
Comment 81 Aleisha Amohia 2019-04-26 02:38:16 UTC
Created attachment 88831 [details] [review]
Bug 14715: results per page setting for catalog search

Add new select field to catalog search results pages which allows user to set number
of results to be shown per page. Setting saved in cookie "results_per_page".

To test:
1) Do a catalog search in the intranet and OPAC
2) Confirm that results automatically update upon selection of results
per page to show
3) Test for all options (20, 40, 60, 80, 100, all)

Bug 14715: syspref for results per page options

Remove the "all" option.

Add syspref [OPAC]numSearchResultsPerPage for specifying options for numbers of
results per page including [OPAC]numSearchResults.

Bug 14715: database updates

Bug 14715: Fixing pagination problems

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 82 Aleisha Amohia 2019-04-26 02:38:24 UTC
Created attachment 88832 [details] [review]
Bug 14715: [QA Follow up] Fixing qa-tools issues

and merge conflicts

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 83 Aleisha Amohia 2019-04-26 02:38:31 UTC
Created attachment 88833 [details] [review]
Bug 14715: [QA Follow up] Removing 'all' option, fixing QA tools issues

Signed-off-by: Simon Pouchol <simon.pouchol@biblibre.com>

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 84 Aleisha Amohia 2019-04-26 02:38:38 UTC
Created attachment 88834 [details] [review]
Bug 14715: [QA Follow up] Rebase and fix up

I noticed that the two sysprefs initially created with this patch
weren't really being used, so I removed them. Instead I've added two new
sysprefs:
numSearchResultsDropdown
OPACnumSearchResultsDropdown
that control whether the dropdown shows or not. Otherwise the defaults
in numSearchResults and OPACnumSearchResults are used to decide number
of search results per page.

When testing:
Run updatedatabase.pl
Test with values in numSearchResults and OPACnumSearchResults. Use
original test plan in Comment 12 but test with the dropdown sysprefs off
and on.

Sponsored-by: Region Halland

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 85 Aleisha Amohia 2019-04-26 02:38:45 UTC
Created attachment 88835 [details] [review]
Bug 14715: (QA Follow up) Fix results not showing

This fixes the display issue for me. Can someone please test?

Signed-off-by: Cab Vinton <bibliwho@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 86 Aleisha Amohia 2019-04-26 02:38:52 UTC
Created attachment 88836 [details] [review]
Bug 14715: (QA follow-up) some code changes

1) adds sysprefs to sysprefs.sql
2) removes a merge conflict error that should not have been added

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 87 Aleisha Amohia 2019-04-26 02:38:59 UTC
Created attachment 88837 [details] [review]
Bug 14715: (QA follow-up) Edits for ElasticSearch

Note added to preferences that the dropdown should not be used with
ElasticSearch activated.
Dropdowns do not show on search results on staff side or OPAC side if
Elasticsearch enabled.

Ready to test

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 88 Aleisha Amohia 2019-04-26 02:39:07 UTC
Created attachment 88838 [details] [review]
Bug 14715: (QA Follow up) More QA tool cleanup

Tools are picking up more stuff now, so a few more things to fix!

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 89 Aleisha Amohia 2019-04-26 02:39:14 UTC
Created attachment 88839 [details] [review]
Bug 14715: (follow-up) Fixing database updates

This patch removes the atomic update that adds the 'perPage' system
preferences and keeps the 'dropdown' system preferences.

Also sql file is moved into perl file.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 90 Katrin Fischer 2019-04-28 22:27:10 UTC
*** Bug 7260 has been marked as a duplicate of this bug. ***
Comment 91 Séverine Queune 2019-10-02 14:56:37 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 14715:
<h1>Something went wrong !</h1>Applying: Bug 14715: results per page setting for catalog search
Using index info to reconstruct a base tree...
M       C4/Search.pm
M       catalogue/search.pl
M       koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
M       koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
M       opac/opac-search.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
CONFLICT (content): Merge conflict in opac/opac-search.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
Auto-merging catalogue/search.pl
CONFLICT (content): Merge conflict in catalogue/search.pl
Auto-merging C4/Search.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14715: results per page setting for catalog search
Comment 92 Aleisha Amohia 2019-11-05 03:01:55 UTC
Created attachment 95038 [details] [review]
Bug 14715: Results per page dropdown for catalogue search

This patch utilises an unused search facet to handle results shown per
page.

To test:

1) Enable patch and update database to apply atomic updates
2) Go to system preferences and set both 'numSearchResultsDropdown' and
'OPACnumSearchResultsDropdown' to 'Show'
3) Set both 'numSearchResults' and 'OPACnumSearchResults' system
preferences to 10 so that it will be clear to see how the patch works
4) Do a catalogue search in the intranet using a search term that will
bring lots of results
5) The results per page dropdown should show with the selected option as
the library default, the value of numSearchResults. The number of
results shown on the page should match numSearchResults.
6) Choose a different number of results per page in the dropdown. The
search results will automatically refresh
7) Confirm that the correct number of results per page shows
8) Confirm that changing pages does not revert the number of results
shown per page to the default value
9) Confirm that you are still able to use other search facets as
expected
10) Repeat steps 4-9 on the OPAC (note OPAC uses OPACnumSearchResults as
    the default value)

Sponsored-by: Region Halland
Comment 93 Gabriel DeCarufel 2020-01-08 15:08:07 UTC
Created attachment 97004 [details] [review]
Bug 14715: Results per page dropdown for catalogue search

This patch utilises an unused search facet to handle results shown per
page.

Sponsored-by: Region Halland
Signed-off-by: Gabriel DeCarufel <gabriel@inlibro.com>
Comment 94 David Nind 2020-02-04 09:08:36 UTC
Created attachment 98365 [details] [review]
Bug 14715: Results per page dropdown for catalogue search

This patch utilises an unused search facet to handle results shown per
page.

To test:

1) Enable patch and update database to apply atomic updates
2) Go to system preferences and set both 'numSearchResultsDropdown' and
'OPACnumSearchResultsDropdown' to 'Show'
3) Set both 'numSearchResults' and 'OPACnumSearchResults' system
preferences to 10 so that it will be clear to see how the patch works
4) Do a catalogue search in the intranet using a search term that will
bring lots of results
5) The results per page dropdown should show with the selected option as
the library default, the value of numSearchResults. The number of
results shown on the page should match numSearchResults.
6) Choose a different number of results per page in the dropdown. The
search results will automatically refresh
7) Confirm that the correct number of results per page shows
8) Confirm that changing pages does not revert the number of results
shown per page to the default value
9) Confirm that you are still able to use other search facets as
expected
10) Repeat steps 4-9 on the OPAC (note OPAC uses OPACnumSearchResults as
    the default value)

Sponsored-by: Region Halland
Signed-off-by: Gabriel DeCarufel <gabriel@inlibro.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 95 David Nind 2020-02-04 09:14:36 UTC
A couple of things I noticed during testing, not sure whether they are show stoppers or not:

- OPAC and staff interface: the number of results resets to the default if you select any of the refine search options/facets

- Staff interface: if you choose any option from the sort drop down this also resets the number of search results to the default

Otherwise, it works very nicely!
Comment 96 Katrin Fischer 2020-02-19 22:58:40 UTC
Hi Aleisha, 

can you please take a look at David'd comment#95?

I also got a lot of fails with the QA tools for missing filters :(

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/page-numbers.inc
   FAIL	  filters
		missing_filter at line 4 (        <li><a class="nav" href="/cgi-bin/koha/catalogue/search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by |url %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]"><i class="fa fa-fw fa-angle-double-left"></i> First</a></li>)
		missing_filter at line 6 (        <li><a class="nav" href="/cgi-bin/koha/catalogue/search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% previous_page_offset | html %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by |url %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]"><i class="fa fa-fw fa-angle-left"></i> Previous</a></li>)
		missing_filter at line 12 (            <li><a class="nav" href="/cgi-bin/koha/catalogue/search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% PAGE_NUMBER.offset | html %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by |url %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">[% PAGE_NUMBER.pg | html %]</a></li>)
		missing_filter at line 16 (        <li><a class="nav" href="/cgi-bin/koha/catalogue/search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% next_page_offset | html %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by |url %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">Next <i class="fa fa-fw fa-angle-right"></i></a></li>)
		missing_filter at line 17 (        <li><a class="nav" href="/cgi-bin/koha/catalogue/search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% last_page_offset | html %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by |url %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">Last <i class="fa fa-fw fa-angle-double-right"></i></a></li>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
   FAIL	  filters
		missing_filter at line 274 (                                        Results per page: [% results_per_page %] <span class="caret"></span>)
		missing_filter at line 281 (                                                [% END %] Library default: [% Koha.Preference('numSearchResults') %])
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/includes/page-numbers.inc
   FAIL	  filters
		wrong_html_filter at line 6 (                <li class="page-first"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]"><i class="fa fa-fw fa-angle-double-left"></i>  First</a></li>)
		missing_filter at line 6 (                <li class="page-first"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]"><i class="fa fa-fw fa-angle-double-left"></i>  First</a></li>)
		wrong_html_filter at line 7 (                <li class="page-prev"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% previous_page_offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]"> <i class="fa fa-fw fa-angle-left"></i>  Previous</a></li>)
		missing_filter at line 7 (                <li class="page-prev"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% previous_page_offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]"> <i class="fa fa-fw fa-angle-left"></i>  Previous</a></li>)
		wrong_html_filter at line 13 (                    <li class="page-num"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% PAGE_NUMBER.offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">[% PAGE_NUMBER.pg | html %]</a></li>)
		missing_filter at line 13 (                    <li class="page-num"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% PAGE_NUMBER.offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">[% PAGE_NUMBER.pg | html %]</a></li>)
		wrong_html_filter at line 17 (                <li class="page-next"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% next_page_offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">Next <i class="fa fa-fw fa-angle-double-right"></i></a></li>)
		missing_filter at line 17 (                <li class="page-next"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% next_page_offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">Next <i class="fa fa-fw fa-angle-double-right"></i></a></li>)
		wrong_html_filter at line 18 (                <li class="page-last"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% last_page_offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">Last <i class="fa fa-fw fa-angle-double-right"></i></a></li>)
		missing_filter at line 18 (                <li class="page-last"><a href="/cgi-bin/koha/opac-search.pl?[% query_cgi | $raw %][% limit_cgi | $raw %]&amp;offset=[% last_page_offset | uri %][% IF ( sort_by ) %]&amp;sort_by=[% sort_by | uri %][% END %][% IF results_per_page %]&amp;count=[% results_per_page %][% END %]">Last <i class="fa fa-fw fa-angle-double-right"></i></a></li>)
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
   FAIL	  filters
		missing_filter at line 172 (                                                                <option value="[% Koha.Preference('OPACnumSearchResults') %]" selected="selected">Library default: [% Koha.Preference('OPACnumSearchResults') %]</option>)
		missing_filter at line 172 (                                                                <option value="[% Koha.Preference('OPACnumSearchResults') %]" selected="selected">Library default: [% Koha.Preference('OPACnumSearchResults') %]</option>)
		missing_filter at line 174 (                                                                <option value="[% Koha.Preference('OPACnumSearchResults') %]">Library default: [% Koha.Preference('OPACnumSearchResults') %]</option>)
		missing_filter at line 174 (                                                                <option value="[% Koha.Preference('OPACnumSearchResults') %]">Library default: [% Koha.Preference('OPACnumSearchResults') %]</option>)
Comment 97 Jonathan Druart 2020-02-21 11:01:45 UTC
Missing filters can be added easily with misc/devel/add_missing_filters.pl
Comment 98 Aleisha Amohia 2020-02-25 20:44:25 UTC
Created attachment 99607 [details] [review]
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results

Also adding missing filters
Comment 99 Jonathan Druart 2020-02-26 08:41:40 UTC
Aleisha, what about David's comment?
Comment 100 Jonathan Druart 2020-02-26 08:42:21 UTC
Aleisha, what about David's comment? can you please take a look at David'd comment#95?(In reply to Jonathan Druart from comment #99)
> Aleisha, what about David's comment?

Scratch that! I thought you only added the filters!
Comment 101 David Nind 2020-02-26 09:23:22 UTC
Thanks for all the changes Aleisha! When changing facets it now remembers the setting.

However, the only thing thing that isn't working is on the staff interface.

When you try to go back to the default (Library default: 10) nothing happens, example URL - the number doesn't change, the page doesn't automatically refredh and a # is added at the end of the URL:
http://127.0.0.1:8081/cgi-bin/koha/catalogue/search.pl?count=20&idx=kw&q=cat#
Comment 102 Aleisha Amohia 2020-02-26 22:05:03 UTC
Created attachment 99677 [details] [review]
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results

Also adding missing filters

ALSO ensuring you can switch back to library default on staff side
Comment 103 David Nind 2020-02-27 07:58:56 UTC
Created attachment 99684 [details] [review]
Bug 14715: Results per page dropdown for catalogue search

This patch utilises an unused search facet to handle results shown per
page.

Sponsored-by: Region Halland
Signed-off-by: Gabriel DeCarufel <gabriel@inlibro.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 104 David Nind 2020-02-27 07:59:02 UTC
Created attachment 99685 [details] [review]
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results

Also adding missing filters

ALSO ensuring you can switch back to library default on staff side

Signed-off-by: David Nind <david@davidnind.com>
Comment 105 Jonathan Druart 2020-02-27 09:03:26 UTC
What is the problem with elasticsearch? I have tried (removing the condition in the template), and it works as expected.

I do not think we should continue to push Zebra-only features nowadays, especially if it works with elastic :)
Comment 106 Katrin Fischer 2020-03-25 21:38:16 UTC
>3) It appears to break when Elasticsearch is activated in a non-good way. >Switching the number of results made the OPAC load forever for me showing a >white page. Then it started to work, but the number of pages doesn't change.
>Can you please check? If we can't make this work, we should at least add a note >to the pref and keep it from showing up with Elasticsearch active. 

I have retested and can't find any issues with Elasticsearch - I think the problems I found earlier were probably an issue in my installation. I am proposing a PQA with a follow-up removing the code that makes this Zebra only.
Comment 107 Katrin Fischer 2020-03-25 21:40:44 UTC
Created attachment 101775 [details] [review]
Bug 14715: Results per page dropdown for catalogue search

This patch utilises an unused search facet to handle results shown per
page.

Sponsored-by: Region Halland
Signed-off-by: Gabriel DeCarufel <gabriel@inlibro.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 108 Katrin Fischer 2020-03-25 21:40:54 UTC
Created attachment 101776 [details] [review]
Bug 14715: (follow-up) Keep results per page setting when selecting facets and sorting results

Also adding missing filters

ALSO ensuring you can switch back to library default on staff side

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 109 Katrin Fischer 2020-03-25 21:41:01 UTC
Created attachment 101777 [details] [review]
Bug 14715: (QA follow-up) Add missing filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 110 Katrin Fischer 2020-03-25 21:41:08 UTC
Created attachment 101778 [details] [review]
Bug 14715: (QA follow-up) Remove limit on Zebra

Found in testing that this appears to work well for Elasticsearch -
so removing everything that made this Zebra only.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 111 Martin Renvoize 2020-03-26 12:02:50 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 112 Martin Renvoize 2020-03-26 12:03:55 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 113 Joy Nelson 2020-04-03 22:35:47 UTC
enhancement not backported to 19.11.x
Comment 114 Katrin Fischer 2020-05-10 12:29:33 UTC
Hm, was looking to add information on the use of the cookie mentioned to https://wiki.koha-community.org/wiki/Use_of_Cookies, but didn't spot the code for it. Can you help?
Comment 115 Katrin Fischer 2020-05-11 10:31:09 UTC
Verified with Joubu - there is no cookie :)