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.
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
A note on my test plan, the NewsLog is currently broken (bug 28717), so don't expect it to log anything.
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>
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.
Created attachment 123921 [details] [review] Bug 28872: Atomicupdate to correct existing values
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!
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.
Created attachment 123940 [details] [review] Bug 28872: (QA follow-up) print a message describing what has happened
Created attachment 123941 [details] [review] Bug 28872: (QA follow-up) Update printed messaged, fix file permissions
Created attachment 123942 [details] [review] Bug 28872: Fix QA issues on atomicupdate
There was a problem in the skeleton file, I've fixed it in master and adjusted the atomic update here.
Sorry, I got a collision a couple of hours ago when I submitted the patch and didn't realize until now.
Thanks Jonathan and Kyle!
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>
Created attachment 123957 [details] [review] Bug 28872: Atomicupdate to correct existing values Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 123958 [details] [review] Bug 28872: Fix QA issues on atomicupdate Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Why do I not find any references to NewsLog outside of the log viewer tt?
(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
(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!
(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.
(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 :)
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.03
Those preferences are recent : AcquisitionLog Bug 23971 NewsLogBug Bug 26205 NoticesLog Bug 14233
None are in 20.11.x