|
Description
Julian Maurice
2013-09-26 10:51:04 UTC
Created attachment 21482 [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. Increasing severity to major as its more a bugfix than an enhancement (cf bug 10807) 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. Created attachment 21550 [details] [review] [SIGNED-OFF] 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: 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 Created attachment 22133 [details] [review] [PASSED QA] 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: 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. 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. Created attachment 23618 [details] [review] [PASSED QA] 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. Created attachment 23619 [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 1st patch rebased on master. 2nd patch needs signoff (or maybe only QA since it modifies only comments and unit tests) (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. Created attachment 23755 [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. Created attachment 23756 [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 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
Hm, and now I can't reproduce it a second time... could someone else give this a try please? Ok, giving this another go... 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 :( 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. Created attachment 23927 [details] [review] Bug 10952: Always flush session after deletion This is recommended in CGI::Session documentation. I couldn't reproduce this either, so would like someone who can to sign off 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. Patch tested with a sandbox, by Charlene Criton <charlene.criton@univ-lyon2.fr> Created attachment 24137 [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> Created attachment 24138 [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> Created attachment 24139 [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> 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> 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> 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> 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! Patches pushed to 3.14.x, will be in 3.14.4. A nice work on this really ugly bug. |