In the 18.05 documentation, finesMode is described thus: Default: Calculate (but only for mailing to the admin) Asks: ___ fines Values: - Calculate (but only for mailing to the admin) [test] - Calculate and charge [production] - Don’t calculate [off] https://koha-community.org/manual/18.05/en/html/systempreferences.html?highlight=finesmode#finesmode I have interpreted this as "If fines.pl is run and finesMode is set to 'test', fines will be generated in a CSV(ish) file and the file will be emailed to the address in KohaAdminEmailAddress". I have tested this a bit now, and no email with the file seems to be sent. Reading the code, I can not see any signs of an email being sent either. The 3.0 docs has this to say about finesMode: Description: This preference establishes how Koha will handle fines being accrued by patrons. There are three values to choose from: “Off”, Test, and Production. If the “Off” value is put into effect no fines will be calculated. The Test value prompts Koha to email fine notifications. The Production value is selected to allow Koha to start calculating overdue fines. It is important to know that extra programming will be involved to make this preference functional and a cronjob will need to be put into effect. A cronjob is a programming function that issues commands at specific dates and times. https://koha-community.org/files/2010/02/wayne-state-university-koha-3.0-reference-manual.pdf So: "The Test value prompts Koha to email fine notifications." But: "It is important to know that extra programming will be involved to make this preference functional." I am starting to believe that the emailing of the file was never really implemented. Maybe we should just remove references to emailing from the docs, the comments in the fines.pl script etc? Or was the intended meaning that fines.pl produces a file that could somehow be sent to the admin, but the "somehow" is left to the reader to implement? Or am I missing something?
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