|
Description
Magnus Enger
2018-10-23 09:00:46 UTC
Looking some more at this, it seems that finesMode = test really does not do anything. In order to make fines.pl write output to a file, either the --log command line option (for writing to the default location) or --out (for writing to a specific directory) needs to be added to the standard cronjob. In a multi-tenant setup (think Debian packages) this is less than ideal, since it means turning on logging to files for all instances, which might not be desirable in light of GDPR. I am pretty sure there was an old bug somewhere about the wrong third option, but I just can't find it. I am pretty sure you are correct in the test mode being non-functional. I looked through the code - I cannot find any place where we check finesMode for 'test' - we check for 'production' and take not action if it isn't set I think if we can't fix it easily (there is not code yet...) then we should finally remove the option to end confusion. (In reply to Katrin Fischer from comment #4) > I think if we can't fix it easily (there is not code yet...) then we should > finally remove the option to end confusion. Agreed. Created attachment 96794 [details] [review] Bug 21633: Remove finesMode 'test' Created attachment 96835 [details] [review] Bug 21633: Remove finesMode 'test' We have a problem with the update DB, Hea says that "test" is the most used option, and moving test to off will introduce changes in behavior:
97 my $overduecharges = (C4::Context->preference('finesMode') && C4::Context->preference('finesMode') ne 'off');
Also there are 2 NULL and 136 "", maybe we should deal with them here. (In reply to Jonathan Druart from comment #8) > We have a problem with the update DB, Hea says that "test" is the most used > option, and moving test to off will introduce changes in behavior: > > 97 my $overduecharges = (C4::Context->preference('finesMode') && > C4::Context->preference('finesMode') ne 'off'); This just controls whether 'Forgive overdue charges' checkbox is displayed - if 'test' it had not effect, removing it should not change behaviour (In reply to Jonathan Druart from comment #9) > Also there are 2 NULL and 136 "", maybe we should deal with them here. Dealing with that here or somewhere else? Created attachment 97809 [details] [review] Bug 21633: Remove finesMode 'test' Created attachment 97810 [details] [review] Bug 21633: (follow-up) Cleanup other values for finesMode Created attachment 99225 [details] [review] Bug 21633: Remove finesMode 'test' Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 99226 [details] [review] Bug 21633: (follow-up) Cleanup other values for finesMode Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice bit of clean up.. thanks guys. Passing QA Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.x branch for 19.11.04 |