Summary: | Did finesMode = test ever send email? | ||
---|---|---|---|
Product: | Koha | Reporter: | Magnus Enger <magnus> |
Component: | Documentation | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | 1joynelson, jonathan.druart, martin.renvoize, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24475 | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
20.05.00, 19.11.04
|
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 24475 | ||
Attachments: |
Bug 21633: Remove finesMode 'test'
Bug 21633: Remove finesMode 'test' Bug 21633: Remove finesMode 'test' Bug 21633: (follow-up) Cleanup other values for finesMode Bug 21633: Remove finesMode 'test' Bug 21633: (follow-up) Cleanup other values for finesMode |
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 |