Bug 10952 - Store anonymous search history in session
Summary: Store anonymous search history in session
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 10857
  Show dependency treegraph
 
Reported: 2013-09-26 10:51 UTC by Julian Maurice
Modified: 2015-06-04 23:23 UTC (History)
10 users (show)

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


Attachments
Bug 10952: Store anonymous search history in session (11.03 KB, patch)
2013-09-26 10:52 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10952: Store anonymous search history in session (11.33 KB, patch)
2013-09-28 18:55 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 10952: Store anonymous search history in session (11.44 KB, patch)
2013-10-20 23:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10952: Store anonymous search history in session (11.04 KB, patch)
2013-12-18 10:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 10952: comments fixes and unit tests (6.56 KB, patch)
2013-12-18 10:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 10952: Store anonymous search history in session (11.54 KB, patch)
2013-12-22 20:52 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10952: comments fixes and unit tests (6.65 KB, patch)
2013-12-22 20:52 UTC, Chris Cormack
Details | Diff | Splinter Review
Screenshot showing doubled searches in search historx (21.80 KB, image/png)
2013-12-23 13:31 UTC, Katrin Fischer
Details
Bug 10952: Always flush session after deletion (4.15 KB, patch)
2014-01-06 09:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 10952: Store anonymous search history in session (11.60 KB, patch)
2014-01-10 14:57 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10952: comments fixes and unit tests (6.72 KB, patch)
2014-01-10 14:57 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10952: Always flush session after deletion (4.21 KB, patch)
2014-01-10 14:57 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10952: Store anonymous search history in session (11.66 KB, patch)
2014-01-10 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10952: comments fixes and unit tests (6.77 KB, patch)
2014-01-10 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10952: Always flush session after deletion (4.26 KB, patch)
2014-01-10 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2013-09-26 10:51:04 UTC
Storing search history into cookie can cause problems, due to the size limitation of 4KB.  The solution here is to store search history into the CGI::Session object, so there is no size limitation
Comment 1 Julian Maurice 2013-09-26 10:52:11 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2013-09-26 14:28:08 UTC
Increasing severity to major as its more a bugfix than an enhancement (cf bug 10807)
Comment 3 Galen Charlton 2013-09-26 16:39:58 UTC
Haven't looked at the patch in detail, but +1 to the notion of storing it in the session.  I do wonder if there should be a limit on the number of searches kept though -- while there is no specific limit on the amount of data that can be stored in a session, consider the case of a public OPAC station that doesn't get reset often.  In principle, it could end up with a session that carries hundreds of searches, which might have a notable impact on performance.
Comment 4 Bernardo Gonzalez Kriegel 2013-09-28 18:55:23 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2013-10-20 23:20:10 UTC Comment hidden (obsolete)
Comment 6 Galen Charlton 2013-11-14 16:28:11 UTC
This patch needs more work.  Most obviously, it breaks the tests in t/db_dependent/Auth_ParseSearchHistoryCookie.t.  Of course, ParseSearchHistoryCookie is removed by the patch, but the tests should be rewritten so that they exercise session-based search history.

Also, there are a number of comments referring to the search history cookie that should be removed or revised.
Comment 7 Julian Maurice 2013-12-18 10:01:50 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2013-12-18 10:01:56 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2013-12-18 10:05:57 UTC
1st patch rebased on master.
2nd patch needs signoff (or maybe only QA since it modifies only comments and unit tests)
Comment 10 Chris Cormack 2013-12-22 20:49:26 UTC
(In reply to Julian Maurice from comment #9)
> 1st patch rebased on master.

When you did the rebase, you lost the sign offs on the commit, I have readded them and the comments.

Please try to be careful to apply the signed off patch then rebase it.
Comment 11 Chris Cormack 2013-12-22 20:52:27 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2013-12-22 20:52:47 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2013-12-23 13:31:25 UTC
Created attachment 23783 [details]
Screenshot showing doubled searches in search historx

I am sorry, but I think I found a problem:

- logged out of Koha
- did 2 searches (Any: der and Title: der und)
- logged into Koha
- 4 searches (each doubled up) are added to my user's search history
Comment 14 Katrin Fischer 2013-12-23 13:41:57 UTC
Hm, and now I can't reproduce it a second time... could someone else give this a try please?
Comment 15 Katrin Fischer 2013-12-29 13:12:38 UTC
Ok, giving this another go...
Comment 16 Katrin Fischer 2013-12-29 13:22:20 UTC
Tested again in Chromium and Bootstrap theme:
- Logged out.
- Closed browser.
- Reopened browser.
- Did 2 simple searches.
- Logged in using the link on top.
- Doubled up entries in search history.
- Logged out.
- Did another serach.
- Logged back in.
- Doubled up entries in search history.

I can't pass this :(
Comment 17 Julian Maurice 2014-01-06 09:07:29 UTC
I was not able to reproduce the problem, but I suspect that the session is not properly deleted when it should.
The following patch ensure that by flushing session after each call to $session->delete(). I hope this will fix the problem.
Comment 18 Julian Maurice 2014-01-06 09:07:45 UTC Comment hidden (obsolete)
Comment 19 Chris Cormack 2014-01-08 05:58:59 UTC
I couldn't reproduce this either, so would like someone who can to sign off
Comment 20 jmbroust 2014-01-10 14:56:53 UTC
Tested on sandbox 16 biblibre
This patch works well ! Tests with Firefox and Internet Explorer.

We wonder if it's not a problem for public computers without identification. For example, in our library, lot of research are performed with computers only catalog access. If a patron log after several anonymous searches, he would have the all precedent anonymous searches in his search history.
Comment 21 sandboxes@biblibre.com 2014-01-10 14:57:11 UTC
Patch tested with a sandbox, by Charlene Criton <charlene.criton@univ-lyon2.fr>
Comment 22 sandboxes@biblibre.com 2014-01-10 14:57:49 UTC Comment hidden (obsolete)
Comment 23 sandboxes@biblibre.com 2014-01-10 14:57:53 UTC Comment hidden (obsolete)
Comment 24 sandboxes@biblibre.com 2014-01-10 14:57:57 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2014-01-10 15:51:05 UTC
Created attachment 24156 [details] [review]
Bug 10952: Store anonymous search history in session

Storing search history into cookie can cause problems, due to the size
limitation of 4KB.
The solution here is to store search history into the CGI::Session
object, so there is no size limitation (but anonymous search history
still remember up to 15 requests max.)

Test plan:
- Go to OPAC in anonymous mode.
- Check that the "Search history" link is *not* shown in the top right
  corner of the page
- Make some searches on /cgi-bin/koha/opac-search.pl
- The "Search history" link must appear. Click.
- Your search history must be displayed.
- Try to log in with invalid username/password
- Go back to search history, it's still there
- Now log in with valid username/password
- Your anonymous search history must be saved into your own search
  history.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Restoring original sign offs and comments below

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described. No koha-qa errors

Well, search history saving is similar before and after patch.
i.e. anonmymous search is saved when user logs in, but cookie
KohaOpacRecentSearches is empty.
Shows current an previous session searches

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
All tests and QA script pass, works as described.

Signed-off-by: Charlene Criton <charlene.criton@univ-lyon2.fr>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2014-01-10 15:51:10 UTC
Created attachment 24157 [details] [review]
Bug 10952: comments fixes and unit tests

- Remove unit tests for ParseSearchHistoryCookie, which doesn't exist
  anymore
- Add unit tests for ParseSearchHistorySession and
  SetSearchHistorySession
- Remove/Modify comments about search history cookie

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Tests fixed and moved, and comments tidied up

Signed-off-by: Charlene Criton <charlene.criton@univ-lyon2.fr>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2014-01-10 15:51:15 UTC
Created attachment 24158 [details] [review]
Bug 10952: Always flush session after deletion

This is recommended in CGI::Session documentation.

Signed-off-by: Charlene Criton <charlene.criton@univ-lyon2.fr>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Galen Charlton 2014-01-10 16:56:41 UTC
Pushed to master, along with a follow-up that fixes the doubled (and tripled, etc.) search history issue that Katrin found, and a follow-up that removes that last remaining reference to the KohaOpacRecentSearches cookie.

Thanks, Julian!
Comment 29 Fridolin SOMERS 2014-02-18 09:18:29 UTC
Patches pushed to 3.14.x, will be in 3.14.4.
A nice work on this really ugly bug.