Bug 8010 - Search history can be added to the wrong patron
Summary: Search history can be added to the wrong patron
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: Baptiste
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-25 21:11 UTC by Anthony Laquerre
Modified: 2017-04-04 07:21 UTC (History)
7 users (show)

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


Attachments
Added a new syspref to Load history to the next user (11.75 KB, patch)
2017-02-13 15:30 UTC, Baptiste
Details | Diff | Splinter Review
Added a new syspref to Load history to the next user (11.73 KB, patch)
2017-02-14 10:51 UTC, Baptiste
Details | Diff | Splinter Review
Added a new syspref to Load history to the next user (11.68 KB, patch)
2017-02-14 11:04 UTC, Baptiste
Details | Diff | Splinter Review
BUG 8010: Correct a few syntax errors, and moved tests (7.56 KB, patch)
2017-03-17 16:15 UTC, Baptiste
Details | Diff | Splinter Review
BUG 8010: Removed Auth2.t (5.09 KB, patch)
2017-03-20 08:39 UTC, Baptiste
Details | Diff | Splinter Review
Added a new syspref to Load history to the next user (11.86 KB, patch)
2017-03-20 10:10 UTC, Marc Véron
Details | Diff | Splinter Review
BUG 8010: Correct a few syntax errors, and moved tests (7.69 KB, patch)
2017-03-20 10:11 UTC, Marc Véron
Details | Diff | Splinter Review
BUG 8010: Removed Auth2.t (5.22 KB, patch)
2017-03-20 10:13 UTC, Marc Véron
Details | Diff | Splinter Review
Added a new syspref to Load history to the next user (11.92 KB, patch)
2017-03-20 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
BUG 8010: Correct a few syntax errors, and moved tests (7.75 KB, patch)
2017-03-20 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
BUG 8010: Removed Auth2.t (5.29 KB, patch)
2017-03-20 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Anthony Laquerre 2012-04-25 21:11:22 UTC
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.
Comment 1 Owen Leonard 2012-05-09 18:23:29 UTC
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.
Comment 2 Marc Véron 2015-12-20 17:07:08 UTC
Still valid?
Comment 3 Marc Véron 2016-10-07 16:20:53 UTC
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.
Comment 4 Katrin Fischer 2016-10-16 13:49:49 UTC
What solution to this would you suggest?
Comment 5 Jonathan Druart 2016-10-18 08:09:58 UTC
What you describe is the expected behavior.
We could imagine a pref to disable the search history for non-logged in user.
Comment 6 Baptiste 2017-02-13 15:30:35 UTC
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.
Comment 7 Baptiste 2017-02-14 10:51:30 UTC
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.
Comment 8 Baptiste 2017-02-14 11:04:24 UTC
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.
Comment 9 Marc Véron 2017-03-15 16:10:46 UTC
Small typo in the sql files:
....get the last seaches....
Comment 10 Marc Véron 2017-03-15 16:24:25 UTC
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
Comment 11 Jonathan Druart 2017-03-15 19:06:32 UTC
And tests must be added to t/db_dependent/Search/History.t, in a new subtest.
Comment 12 Baptiste 2017-03-17 16:15:59 UTC
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
Comment 13 Marc Véron 2017-03-17 16:59:22 UTC
applied both paches, then 
git grep FirstLoggedUser
Result: 
both variants of LoadSearchHistoryToTheFirstLoggedUser rsp. addSearchHistoryToTheFirstLoggedUser
Comment 14 Baptiste 2017-03-20 08:39:58 UTC
Created attachment 61251 [details] [review]
BUG 8010: Removed Auth2.t
Comment 15 Marc Véron 2017-03-20 10:10:56 UTC
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>
Comment 16 Marc Véron 2017-03-20 10:11:50 UTC
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>
Comment 17 Marc Véron 2017-03-20 10:13:24 UTC
Created attachment 61258 [details] [review]
BUG 8010: Removed Auth2.t

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 18 Martin Renvoize 2017-03-20 12:14:12 UTC
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>
Comment 19 Martin Renvoize 2017-03-20 12:14:16 UTC
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>
Comment 20 Martin Renvoize 2017-03-20 12:14:19 UTC
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>
Comment 21 Martin Renvoize 2017-03-20 12:15:11 UTC
All works as expected and passes qa script checks.  Looks good to me.
Comment 22 Brendan Gallagher 2017-03-23 10:04:34 UTC
Pushed to Master - Should be in the 17.05 release.  Thanks!
Comment 23 Katrin Fischer 2017-03-26 20:43:55 UTC
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?
Comment 24 Katrin Fischer 2017-04-02 16:35:22 UTC
Please see last comment!
Comment 25 Baptiste 2017-04-03 13:26:54 UTC
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 ?
Comment 26 Katrin Fischer 2017-04-04 06:24:45 UTC
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. :)
Comment 27 Claire Gravely 2017-04-04 07:21:37 UTC
maybe:

SaveSearchHistoryToNextLoggedInUser

Save/don't save unlogged search history to the next logged in user's account

Add/don't add could also work.