Bug 28872 - AcquisitionLog, NewsLog, NoticesLog should use 1/0 for their values
Summary: AcquisitionLog, NewsLog, NoticesLog should use 1/0 for their values
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Andrew Fuerste-Henry
QA Contact: Testopia
URL:
Keywords:
Depends on: 14233 23971 26205
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-17 17:13 UTC by Andrew Fuerste-Henry
Modified: 2021-08-27 19:30 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.03


Attachments
Bug 28872: make AcquisitionLog, NewsLog, NoticesLog use 1/0 for their values (1.94 KB, patch)
2021-08-17 17:29 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28872: make AcquisitionLog, NewsLog, NoticesLog use 1/0 for their values (1.99 KB, patch)
2021-08-17 20:09 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 28872: Atomicupdate to correct existing values (1.32 KB, patch)
2021-08-17 23:59 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28872: (QA follow-up) print a message describing what has happened (1.06 KB, patch)
2021-08-18 11:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28872: (QA follow-up) Update printed messaged, fix file permissions (1.18 KB, patch)
2021-08-18 11:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28872: Fix QA issues on atomicupdate (1.49 KB, patch)
2021-08-18 12:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28872: make AcquisitionLog, NewsLog, NoticesLog use 1/0 for their values (2.05 KB, patch)
2021-08-18 12:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28872: Atomicupdate to correct existing values (1.38 KB, patch)
2021-08-18 12:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28872: Fix QA issues on atomicupdate (1.55 KB, patch)
2021-08-18 12:47 UTC, Katrin Fischer
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-08-17 17:13:09 UTC
In system preferences, AcquisitionLog, NewsLog,  and NoticesLog are all saving values of "on" and "off" when they should be "1" and "0."

That means if you change any of these prefs and save new values you end up with errors on the About page:
Warning	System preference 'AcquisitionLog' must be '0' or '1', but is 'on'.
Warning	System preference 'NewsLog' must be '0' or '1', but is 'on'.
Warning	System preference 'NoticesLog' must be '0' or '1', but is 'on'.

In a little preliminary testing, this seems to mean we default to ALWAYS logging acquisitions actions, even if it's set to "Don't log." I didn't check whether the other two are ending up on or off.
Comment 1 Andrew Fuerste-Henry 2021-08-17 17:29:27 UTC
Created attachment 123880 [details] [review]
Bug 28872: make AcquisitionLog, NewsLog, NoticesLog use 1/0 for their values

To test:
1 - edit AcquisitionLog, NewsLog, NoticesLog to change their values
2 - in About Koha, see the errors noted above
3 - apply patch, restart services
4 - re-edit AcquisitionLog, NewsLog, NoticesLog to change their values again
5 - reload About, see errors are cleared
6 - confirm that actions are logged as expected when logs are on, not logged when logs are off
Comment 2 Andrew Fuerste-Henry 2021-08-17 18:27:28 UTC
A note on my test plan, the NewsLog is currently broken (bug 28717), so don't expect it to log anything.
Comment 3 sandboxes@biblibre.com 2021-08-17 20:09:16 UTC
Created attachment 123911 [details] [review]
Bug 28872: make AcquisitionLog, NewsLog, NoticesLog use 1/0 for their values

To test:
1 - edit AcquisitionLog, NewsLog, NoticesLog to change their values
2 - in About Koha, see the errors noted above
3 - apply patch, restart services
4 - re-edit AcquisitionLog, NewsLog, NoticesLog to change their values again
5 - reload About, see errors are cleared
6 - confirm that actions are logged as expected when logs are on, not logged when logs are off

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 4 Katrin Fischer 2021-08-17 21:30:08 UTC
Hm, I think we should add a database update here, to set the logs to 0 or 1 according to the current setting to fit what was actually selected.
Comment 5 Andrew Fuerste-Henry 2021-08-17 23:59:09 UTC
Created attachment 123921 [details] [review]
Bug 28872: Atomicupdate to correct existing values
Comment 6 Andrew Fuerste-Henry 2021-08-18 00:00:01 UTC
Good call! I've added an atomicupdate to correct existing values. That structure has changed a little since the last one I did, so please let me know if I've done something wrong!
Comment 7 Katrin Fischer 2021-08-18 10:24:54 UTC
Also my first! There are some problems noted by the QA script:

 FAIL	installer/data/mysql/atomicupdate/bug_28872_update_log_values.pl
   OK	  critic
   FAIL	  file permissions
		File must have the exec flag
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		Useless use of anonymous hash ({}) in void context 

The first one would be an easy fix, but not sure if strictly necessary. updatedatabase still works correctly and the file will be removed.

The second one is a bit puzzling to me.
Comment 8 Kyle M Hall 2021-08-18 11:33:22 UTC
Created attachment 123940 [details] [review]
Bug 28872: (QA follow-up) print a message describing what has happened
Comment 9 Kyle M Hall 2021-08-18 11:50:29 UTC
Created attachment 123941 [details] [review]
Bug 28872: (QA follow-up) Update printed messaged, fix file permissions
Comment 10 Jonathan Druart 2021-08-18 12:17:20 UTC
Created attachment 123942 [details] [review]
Bug 28872: Fix QA issues on atomicupdate
Comment 11 Jonathan Druart 2021-08-18 12:17:34 UTC
There was a problem in the skeleton file, I've fixed it in master and adjusted the atomic update here.
Comment 12 Jonathan Druart 2021-08-18 12:18:14 UTC
Sorry, I got a collision a couple of hours ago when I submitted the patch and didn't realize until now.
Comment 13 Andrew Fuerste-Henry 2021-08-18 12:18:55 UTC
Thanks Jonathan and Kyle!
Comment 14 Katrin Fischer 2021-08-18 12:47:26 UTC
Created attachment 123956 [details] [review]
Bug 28872: make AcquisitionLog, NewsLog, NoticesLog use 1/0 for their values

To test:
1 - edit AcquisitionLog, NewsLog, NoticesLog to change their values
2 - in About Koha, see the errors noted above
3 - apply patch, restart services
4 - re-edit AcquisitionLog, NewsLog, NoticesLog to change their values again
5 - reload About, see errors are cleared
6 - confirm that actions are logged as expected when logs are on, not logged when logs are off

Signed-off-by: Kelly <kelly@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2021-08-18 12:47:31 UTC
Created attachment 123957 [details] [review]
Bug 28872: Atomicupdate to correct existing values

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2021-08-18 12:47:35 UTC
Created attachment 123958 [details] [review]
Bug 28872: Fix QA issues on atomicupdate

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2021-08-18 12:50:06 UTC
Why do I not find any references to NewsLog outside of the log viewer tt?
Comment 18 Kyle M Hall 2021-08-18 12:55:50 UTC
(In reply to Katrin Fischer from comment #17)
> Why do I not find any references to NewsLog outside of the log viewer tt?

It's from Bug 26205. You are right, something has happened
Comment 19 Kyle M Hall 2021-08-18 12:57:44 UTC
(In reply to Kyle M Hall from comment #18)
> (In reply to Katrin Fischer from comment #17)
> > Why do I not find any references to NewsLog outside of the log viewer tt?
> 
> It's from Bug 26205. You are right, something has happened

Removed by Bug 22544, looks like you caught a regression!
Comment 20 Kyle M Hall 2021-08-18 12:59:29 UTC
(In reply to Kyle M Hall from comment #19)
> (In reply to Kyle M Hall from comment #18)
> > (In reply to Katrin Fischer from comment #17)
> > > Why do I not find any references to NewsLog outside of the log viewer tt?
> > 
> > It's from Bug 26205. You are right, something has happened
> 
> Removed by Bug 22544, looks like you caught a regression!

Looks like Bug 28717 is the fix.
Comment 21 Katrin Fischer 2021-08-19 07:55:27 UTC
(In reply to Kyle M Hall from comment #20)
> (In reply to Kyle M Hall from comment #19)
> > (In reply to Kyle M Hall from comment #18)
> > > (In reply to Katrin Fischer from comment #17)
> > > > Why do I not find any references to NewsLog outside of the log viewer tt?
> > > 
> > > It's from Bug 26205. You are right, something has happened
> > 
> > Removed by Bug 22544, looks like you caught a regression!
> 
> Looks like Bug 28717 is the fix.

Thx for poking this one some more - I had to run off yesterday shortly after leaving the references comment :)
Comment 22 Jonathan Druart 2021-08-19 07:59:59 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 23 Kyle M Hall 2021-08-20 18:10:05 UTC
Pushed to 21.05.x for 21.05.03
Comment 24 Fridolin Somers 2021-08-27 19:29:45 UTC
Those preferences are recent :
AcquisitionLog Bug 23971
NewsLogBug Bug 26205
NoticesLog Bug 14233
Comment 25 Fridolin Somers 2021-08-27 19:30:09 UTC
None are in 20.11.x