Bug 21633

Summary: Did finesMode = test ever send email?
Product: Koha Reporter: Magnus Enger <magnus>
Component: DocumentationAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, jonathan.druart, katrin.fischer, 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
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
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?
Comment 1 Magnus Enger 2018-10-23 09:50:43 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.
Comment 2 Katrin Fischer 2018-10-27 10:33:23 UTC
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.
Comment 3 Nick Clemens 2019-10-24 11:38:53 UTC
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
Comment 4 Katrin Fischer 2020-01-03 06:58:55 UTC
I think if we can't fix it easily (there is not code yet...) then we should finally remove the option to end confusion.
Comment 5 Magnus Enger 2020-01-03 08:02:53 UTC
(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.
Comment 6 Nick Clemens 2020-01-03 14:57:29 UTC Comment hidden (obsolete)
Comment 7 Magnus Enger 2020-01-06 13:05:08 UTC
Created attachment 96835 [details] [review]
Bug 21633: Remove finesMode 'test'
Comment 8 Jonathan Druart 2020-01-10 13:23:51 UTC
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');
Comment 9 Jonathan Druart 2020-01-10 13:26:04 UTC
Also there are 2 NULL and 136 "", maybe we should deal with them here.
Comment 10 Nick Clemens 2020-01-10 15:02:37 UTC
(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
Comment 11 Jonathan Druart 2020-01-21 14:34:30 UTC
(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?
Comment 12 Nick Clemens 2020-01-23 12:05:06 UTC
Created attachment 97809 [details] [review]
Bug 21633: Remove finesMode 'test'
Comment 13 Nick Clemens 2020-01-23 12:06:37 UTC
Created attachment 97810 [details] [review]
Bug 21633: (follow-up) Cleanup other values for finesMode
Comment 14 Martin Renvoize 2020-02-18 19:57:57 UTC
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>
Comment 15 Martin Renvoize 2020-02-18 19:58:00 UTC
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>
Comment 16 Martin Renvoize 2020-02-18 19:58:45 UTC
Nice bit of clean up.. thanks guys.

Passing QA
Comment 17 Martin Renvoize 2020-02-19 11:34:11 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 18 Joy Nelson 2020-03-13 00:37:10 UTC
Pushed to 19.11.x branch for 19.11.04