Bug 29138 - LoadSearchHistoryToTheFirstLoggedUser should save 0 instead of "no"
Summary: LoadSearchHistoryToTheFirstLoggedUser should save 0 instead of "no"
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Andrew Fuerste-Henry
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 22824
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-29 19:13 UTC by Andrew Fuerste-Henry
Modified: 2022-06-06 20:24 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser (2.20 KB, patch)
2021-09-29 19:26 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser (2.25 KB, patch)
2021-10-05 01:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser (2.34 KB, patch)
2021-10-07 06:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser (2.30 KB, patch)
2021-10-07 14:17 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser (2.34 KB, patch)
2021-10-08 06:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29138: (QA follow-up) Changes to UPDATE (1.37 KB, patch)
2021-10-08 06:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29138: (QA follow-up) Changes to UPDATE (1.40 KB, patch)
2021-10-08 06:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29138: (QA follow-up) Changes to UPDATE (1.40 KB, patch)
2021-10-08 06:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29138: (QA follow-up) Changes to UPDATE (1.52 KB, patch)
2021-10-09 14:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2021-09-29 19:13:36 UTC
The syspref LoadSearchHistoryToTheFirstLoggedUser saves a 1 when turned on, but a "no" when turned off. It should be a 0 when turned off. When a "no" is saved, the syspref behaves as if it's turned on.
Comment 1 Andrew Fuerste-Henry 2021-09-29 19:26:22 UTC
Created attachment 125462 [details] [review]
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser

To test:
- apply patch
- set LoadSearchHistoryToTheFirstLoggedUser to 'Don't add'
- confirm via sql that value=0
- confirm on OPAC that search history is not loaded to account on login
Comment 2 David Nind 2021-10-05 01:35:24 UTC
Created attachment 125717 [details] [review]
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser

To test:
- apply patch
- set LoadSearchHistoryToTheFirstLoggedUser to 'Don't add'
- confirm via sql that value=0
- confirm on OPAC that search history is not loaded to account on login

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2021-10-05 01:38:45 UTC
Testing notes (koha-testing-docker):

1. Check that LoadSearchHistoryToTheFirstLoggedUser is not working as expected when set to 'Don't add':
   a. Set LoadSearchHistoryToTheFirstLoggedUser to 'Add'
   b. In the OPAC make sure you are logged out
   c. Perform some searches
   d. Check search history is recorded by selecting "Search history" in the top-right navigation menu
   e. Log in as a user
   f. Check search history recorded (From the top-right navigation menu select Welcome, [username] > Search history)
   g. Search history should be populated with searches from when not logged in
   h. Select all then select 'Remove selected searches'
   i. Change LoadSearchHistoryToTheFirstLoggedUser to 'Don't add'
   j. Repeat from b-g
   k. Search history will still be shown, even though it should be empty
2. Check database to see what value is recorded:
   a. koha-mysql kohadev
   b. SQL query: select * from systempreferences where variable='LoadSearchHistoryToTheFirstLoggedUser';
   c. When LoadSearchHistoryToTheFirstLoggedUser is set to 'Don't add' value = "no" 
3. Apply the patch.
4. Update the database (updatedatabase) and restart services (restart_all)
5. Check value in database again from step 2 - value should now be 0
6. Repeat the steps from 1 - this time it should work as expected, that is:
   . When LoadSearchHistoryToTheFirstLoggedUser is set to 'Don't add'
   . Search history should be empty when you log in
7. Sign-off!
Comment 4 Marcel de Rooy 2021-10-07 06:26:44 UTC
Created attachment 125856 [details] [review]
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser

To test:
- apply patch
- set LoadSearchHistoryToTheFirstLoggedUser to 'Don't add'
- confirm via sql that value=0
- confirm on OPAC that search history is not loaded to account on login

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Jonathan Druart 2021-10-07 09:25:06 UTC
Caused by
  commit 1fe1b704f9a81172f2a6b86367b314572a854d18
  Bug 22824: Fix yes/no vs 1/0
Comment 6 Jonathan Druart 2021-10-07 13:43:27 UTC
 UPDATE systempreferences SET value= IF(value='no',0,1)

What if it's 0 already? :)
Comment 7 Marcel de Rooy 2021-10-07 13:54:57 UTC
(In reply to Jonathan Druart from comment #6)
>  UPDATE systempreferences SET value= IF(value='no',0,1)
> 
> What if it's 0 already? :)

Good catch!
Comment 8 Andrew Fuerste-Henry 2021-10-07 14:17:21 UTC
Created attachment 125886 [details] [review]
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser

To test:
- apply patch
- set LoadSearchHistoryToTheFirstLoggedUser to 'Don't add'
- confirm via sql that value=0
- confirm on OPAC that search history is not loaded to account on login

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Andrew Fuerste-Henry 2021-10-07 14:18:05 UTC
Updated to:
UPDATE systempreferences SET value= IF(value in (0,'no'),0,1) WHERE variable = 'LoadSearchHistoryToTheFirstLoggedUser';
Comment 10 Jonathan Druart 2021-10-07 14:24:51 UTC
(In reply to Andrew Fuerste-Henry from comment #9)
> Updated to:
> UPDATE systempreferences SET value= IF(value in (0,'no'),0,1) WHERE variable
> = 'LoadSearchHistoryToTheFirstLoggedUser';

Does it work?

MariaDB [koha_kohadev]> select value from systempreferences where variable="LoadSearchHistoryToTheFirstLoggedUser";
+-------+
| value |
+-------+
| no    |
+-------+
1 row in set (0.001 sec)

MariaDB [koha_kohadev]> UPDATE systempreferences SET value=IF(value in (0,'no'),0,1) WHERE variable = 'LoadSearchHistoryToTheFirstLoggedUser';
ERROR 1292 (22007): Truncated incorrect DOUBLE value: 'no'
Comment 11 Andrew Fuerste-Henry 2021-10-07 15:05:06 UTC
Huh, I didn't get that error:
MariaDB [koha_kohadev]> select value from systempreferences where variable="LoadSearchHistoryToTheFirstLoggedUser";
+-------+
| value |
+-------+
| no    |
+-------+
1 row in set (0.000 sec)

MariaDB [koha_kohadev]> UPDATE systempreferences SET value= IF(value in (0,'no'),0,1) WHERE variable = 'LoadSearchHistoryToTheFirstLoggedUser';
Query OK, 1 row affected, 1 warning (0.003 sec)
Rows matched: 1  Changed: 1  Warnings: 1

MariaDB [koha_kohadev]> select value from systempreferences where variable="LoadSearchHistoryToTheFirstLoggedUser";
+-------+
| value |
+-------+
| 0     |
+-------+
1 row in set (0.001 sec)
Comment 12 Marcel de Rooy 2021-10-08 06:34:06 UTC
Created attachment 125922 [details] [review]
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser

To test:
- apply patch
- set LoadSearchHistoryToTheFirstLoggedUser to 'Don't add'
- confirm via sql that value=0
- confirm on OPAC that search history is not loaded to account on login

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2021-10-08 06:34:10 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2021-10-08 06:38:01 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2021-10-08 06:38:58 UTC
Created attachment 125925 [details] [review]
Bug 29138: (QA follow-up) Changes to UPDATE

Move the condition to the WHERE clause.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested on NULL, 0, 1, 2, no, yes. (2 and yes become 0 too. Fine.)
Replaced IFNULL by COALESCE.
Comment 16 Jonathan Druart 2021-10-08 14:57:18 UTC
Still not working for me

ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Truncated incorrect DOUBLE value: 'no' at /kohadevbox/koha/C4/Installer.pm line 736

10.5.12-MariaDB-1

Why not simply:

UPDATE systempreferences SET value=0 WHERE variable='LoadSearchHistoryToTheFirstLoggedUser' and value="no";

?
Comment 17 Marcel de Rooy 2021-10-09 14:48:37 UTC
Created attachment 125992 [details] [review]
Bug 29138: (QA follow-up) Changes to UPDATE

Move the condition to the WHERE clause, using COALESCE to
prevent mysqlism (IFNULL) and strings for strict SQL.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested on NULL, 0, 1, 2, no, yes. (2 and yes become 0 too. Fine.)
Tested strict sql mode: no truncated incorrect DOUBLE value-error anymore.
Comment 18 Marcel de Rooy 2021-10-09 14:50:17 UTC
(In reply to Jonathan Druart from comment #16)
> Still not working for me
> 
> ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Truncated
> incorrect DOUBLE value: 'no' at /kohadevbox/koha/C4/Installer.pm line 736
> 
> 10.5.12-MariaDB-1

Obscure message, caused by strict SQL.

> Why not simply:
> 
> UPDATE systempreferences SET value=0 WHERE
> variable='LoadSearchHistoryToTheFirstLoggedUser' and value="no";

Current condition catches everything outside '1'
Comment 19 Jonathan Druart 2021-10-12 08:57:14 UTC
(In reply to Marcel de Rooy from comment #18)
> (In reply to Jonathan Druart from comment #16)
> > Still not working for me
> > 
> > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Truncated
> > incorrect DOUBLE value: 'no' at /kohadevbox/koha/C4/Installer.pm line 736
> > 
> > 10.5.12-MariaDB-1
> 
> Obscure message, caused by strict SQL.
> 
> > Why not simply:
> > 
> > UPDATE systempreferences SET value=0 WHERE
> > variable='LoadSearchHistoryToTheFirstLoggedUser' and value="no";
> 
> Current condition catches everything outside '1'

Yes, and so it catches "yes". I won't block the patch because of that, but I don't understand how it can be more correct than setting to "0" when was "no".

The problem was with "no":
-                   no : "Don't add"
+                   0: "Don't add"
Comment 20 Jonathan Druart 2021-10-12 09:13:10 UTC
Changing my mind, the bug title and DBrev description are "Use a zero instead of a 'no' in LoadSearchHistoryToTheFirstLoggedUser". It's what we must do, nothing else.
Comment 21 Jonathan Druart 2021-10-12 09:14:16 UTC
DB rev will be changed to:

  UPDATE systempreferences SET value='0'
  WHERE variable='LoadSearchHistoryToTheFirstLoggedUser' AND value='no';
Comment 22 Jonathan Druart 2021-10-12 09:19:09 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 23 Andrew Fuerste-Henry 2021-10-12 11:20:04 UTC
Thanks, Jonathan and everybody!