Bug 21633 - Did finesMode = test ever send email?
Summary: Did finesMode = test ever send email?
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Documentation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 24475
  Show dependency treegraph
 
Reported: 2018-10-23 09:00 UTC by Magnus Enger
Modified: 2020-03-13 00:37 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04


Attachments
Bug 21633: Remove finesMode 'test' (4.94 KB, patch)
2020-01-03 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21633: Remove finesMode 'test' (4.90 KB, patch)
2020-01-06 13:05 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 21633: Remove finesMode 'test' (4.94 KB, patch)
2020-01-23 12:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21633: (follow-up) Cleanup other values for finesMode (1.07 KB, patch)
2020-01-23 12:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21633: Remove finesMode 'test' (5.01 KB, patch)
2020-02-18 19:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21633: (follow-up) Cleanup other values for finesMode (1.17 KB, patch)
2020-02-18 19:58 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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