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: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Stenberg
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-24 19:30 UTC by Martin Stenberg
Modified: 2018-08-07 14:07 UTC (History)
16 users (show)

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


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

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.