Bug 29138

Summary: LoadSearchHistoryToTheFirstLoggedUser should save 0 instead of "no"
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: SearchingAssignee: Andrew Fuerste-Henry <andrewfh>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: david, jonathan.druart, kyle, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00
Bug Depends on: 22824    
Bug Blocks:    
Attachments: Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser
Bug 29138: Use zero instead of no in LoadSearchHistoryToTheFirstLoggedUser
Bug 29138: (QA follow-up) Changes to UPDATE
Bug 29138: (QA follow-up) Changes to UPDATE
Bug 29138: (QA follow-up) Changes to UPDATE
Bug 29138: (QA follow-up) Changes to UPDATE

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!