Search history is added to the next person that log in. *EnableOpacSearchHistory is activated. 1- Go on your OPAC. 2- Dont log in. 3- Do couple of search. 4- Login your account (opac) 5- Click on search history. If you really login yourself that's okay. But for public computer, this is a big problem since it's not necessary you that will login next.. A patron ask us why there was so many search in his account. He was sure it was a security problem where a student was able to hack his account/the system. Tested on On ByWater- Koha demo site : 3.07.00.044 + our site : 3.6.0 Maybe the search history should be cleared once someone login ? Let me know if you need more infos.
What you describe is a potential problem for public computers. It's a feature for non-public computers. A user on their home computer might have a different workflow: 1- Search the library's catalog on their computer 2- Find something they want to place a hold on, log in 3- Click on search history and find that their previous searches didn't get lost Since search history is tied to the user's session cookie, an improvement might be to get a fix for Bug 1627 and set up a lower timeout on OPAC sessions.
Still valid?
Tested on current master. I can reproduce the scenario from initial comment. The behavior seems to be intended, see C4/Auth.pm lines 334 ff: # And if there are searches performed when the user was not logged in, # we add them to the logged-in search history On public computer rsp. a computer used by several persons, previous searches will be added to the (private!) search history of the patron who logs in. That should not happen! Changing status to "major" because patron data is arbitrarily changed.
What solution to this would you suggest?
What you describe is the expected behavior. We could imagine a pref to disable the search history for non-logged in user.
Created attachment 60158 [details] [review] Added a new syspref to Load history to the next user Added a syspref LoadHistory addSearchHistoryToTheFirstLoggedUser to select if you want the system to add the history of searches performed without session when you log in as registered user. TEST PLAN 1 - Search in the catalogue, check you are not logged 2 - Log in : your last history should appear 4 - Log out 5 - Apply the patch 6 - Repeat 1 and 2 7 - Desactivate the syspref addSearchHistoryToTheFirstLoggedUser 8 - Repeat 1 and 2 : your last history shouldn't appear The Unit test doesn't rollback but delete the added lines : the function get_template_and_user allway sets the autocommit to 1.
Created attachment 60183 [details] [review] Added a new syspref to Load history to the next user Added a syspref LoadHistory addSearchHistoryToTheFirstLoggedUser to select if you want the system to add the history of searches performed without session when you log in as registered user. TEST PLAN 1 - Search in the catalogue, check you are not logged 2 - Log in : your last history should appear 4 - Log out 5 - Apply the patch 6 - Repeat 1 and 2 7 - Desactivate the syspref addSearchHistoryToTheFirstLoggedUser 8 - Repeat 1 and 2 : your last history shouldn't appear The Unit test doesn't rollback but delete the added lines : the function get_template_and_user allway sets the autocommit to 1.
Created attachment 60187 [details] [review] Added a new syspref to Load history to the next user Added a syspref LoadHistory addSearchHistoryToTheFirstLoggedUser to select if you want the system to add the history of searches performed without session when you log in as registered user. TEST PLAN 1 - Search in the catalogue, check you are not logged 2 - Log in : your last history should appear 4 - Log out 5 - Apply the patch 6 - Repeat 1 and 2 7 - Desactivate the syspref addSearchHistoryToTheFirstLoggedUser 8 - Repeat 1 and 2 : your last history shouldn't appear The Unit test doesn't rollback but delete the added lines : the function get_template_and_user allway sets the autocommit to 1.
Small typo in the sql files: ....get the last seaches....
I think there is a small error with the logic: If I set LoadSearchHistoryToTheFirstLoggedUserto Load, it does not load, if I set it to Don't load, it loads. BTW: Testplan refers to addSearchHistoryToTheFirstLoggedUser, not LoadSearchHistoryToTheFirstLoggedUserto
And tests must be added to t/db_dependent/Search/History.t, in a new subtest.
Created attachment 61223 [details] [review] BUG 8010: Correct a few syntax errors, and moved tests - Tests are now in t/db_dependent/Search/History.t - There were 2 differents sysprefs in sysprefs.sql and in atomicupdate => fixed
applied both paches, then git grep FirstLoggedUser Result: both variants of LoadSearchHistoryToTheFirstLoggedUser rsp. addSearchHistoryToTheFirstLoggedUser
Created attachment 61251 [details] [review] BUG 8010: Removed Auth2.t
Created attachment 61256 [details] [review] Added a new syspref to Load history to the next user Added a syspref LoadHistory addSearchHistoryToTheFirstLoggedUser to select if you want the system to add the history of searches performed without session when you log in as registered user. TEST PLAN 1 - Search in the catalogue, check you are not logged 2 - Log in : your last history should appear 4 - Log out 5 - Apply the patch 6 - Repeat 1 and 2 7 - Desactivate the syspref addSearchHistoryToTheFirstLoggedUser 8 - Repeat 1 and 2 : your last history shouldn't appear The Unit test doesn't rollback but delete the added lines : the function get_template_and_user allway sets the autocommit to 1. https://bugs.koha-community.org/show_bug.cgi?id=8010 Tested 3 patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61257 [details] [review] BUG 8010: Correct a few syntax errors, and moved tests - Tests are now in t/db_dependent/Search/History.t - There were 2 differents sysprefs in sysprefs.sql and in atomicupdate => fixed Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61258 [details] [review] BUG 8010: Removed Auth2.t Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61288 [details] [review] Added a new syspref to Load history to the next user Added a syspref LoadHistory addSearchHistoryToTheFirstLoggedUser to select if you want the system to add the history of searches performed without session when you log in as registered user. TEST PLAN 1 - Search in the catalogue, check you are not logged 2 - Log in : your last history should appear 4 - Log out 5 - Apply the patch 6 - Repeat 1 and 2 7 - Desactivate the syspref addSearchHistoryToTheFirstLoggedUser 8 - Repeat 1 and 2 : your last history shouldn't appear The Unit test doesn't rollback but delete the added lines : the function get_template_and_user allway sets the autocommit to 1. https://bugs.koha-community.org/show_bug.cgi?id=8010 Tested 3 patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61289 [details] [review] BUG 8010: Correct a few syntax errors, and moved tests - Tests are now in t/db_dependent/Search/History.t - There were 2 differents sysprefs in sysprefs.sql and in atomicupdate => fixed Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 61290 [details] [review] BUG 8010: Removed Auth2.t Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All works as expected and passes qa script checks. Looks good to me.
Pushed to Master - Should be in the 17.05 release. Thanks!
This contains strings, so will have to wait to be considered next month. I am not sure about the pref description: + - Load the unlogged history to the next user. + - pref : LoadSearchHistoryToTheFirstLoggedUser + default: 0 + choices: + yes: "Load" + no : "Don't load" + - history to the next client. Maybe we could rephrase that a bit to make it clearer? We'd also want to make it INSERT IGNORE if we want this in 16.11.x in master. The sypsref name got a bit long :) Why Load and not Add?
Please see last comment!
Hi ! - "This contains strings, so will have to wait to be considered next month." I don't quite understand what you mean.. - "I am not sure about the pref description Maybe we could rephrase that a bit to make it clearer?" What do you think about: "When a user log in [load/don't load] on his own history all searches performed while no user were logged in" - "We'd also want to make it INSERT IGNORE if we want this in 16.11.x in master." You mean in "bug_8010-add_LoadSearchHistoryToTheFirstLoggedUser_syspref.sql" ? - "The sypsref name got a bit long :) Why Load and not Add?" Will one character change anything ? I couldn't find shorter and still.. hum.. let's say explicit. Any idea ?
A week before release of the maintenance release (usually around 15th, release on 22nd) we go into 'string freeze'. I try not to push any more patches to my branch that contain strings. It's to give translators time to finish translations and bring them up to 100% before release. When I looked at this, it was in 'string freeze' - so was planning to come back for the next release. bug_8010-add_LoadSearchHistoryToTheFirstLoggedUser_syspref.sql was moved to updatedatabase.sql in master. When I decide to push this to 16.11.x people upgrading from 16.11.x to 17.05 will get an ugly warning. We can prevent that by adding the IGNORE to updatedatabase.pl in master. My thinking was that the 'load onto' or 'load' sounded not quite right to me in English - but best would be to get a native speaker on this. :)
maybe: SaveSearchHistoryToNextLoggedInUser Save/don't save unlogged search history to the next logged in user's account Add/don't add could also work.