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: Signed Off
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: 2017-10-21 23:15 UTC (History)
14 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

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
Created attachment 67912 [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
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
Created attachment 68000 [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 38 Owen Leonard 2017-10-12 13:17:43 UTC
Created attachment 68001 [details] [review]
Bug 14715: [FOLLOW-UP] Fixing qa-tools issues

and merge conflicts

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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
Created attachment 68305 [details] [review]
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues
Comment 42 Simon Pouchol 2017-10-20 15:47:50 UTC
Created attachment 68327 [details] [review]
Bug 14715: [FOLLOW-UP] Removing 'all' option, fixing QA tools issues

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