Bug 11144 - Fix sequence of cronjobs: automatic renewal - fines - overdue notices
Summary: Fix sequence of cronjobs: automatic renewal - fines - overdue notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Katrin Fischer
QA Contact: Marcel de Rooy
URL:
Keywords:
: 16798 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-10-27 03:46 UTC by Katrin Fischer
Modified: 2017-12-07 22:17 UTC (History)
12 users (show)

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


Attachments
Bug 11144: Run fines cronjob before overdues cronjob (2.16 KB, patch)
2013-12-04 15:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices (3.88 KB, patch)
2015-06-09 21:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices (3.85 KB, patch)
2015-07-09 02:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices (4.06 KB, patch)
2015-11-01 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices (4.12 KB, patch)
2016-05-09 21:01 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices (4.17 KB, patch)
2016-06-24 14:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11144 [QA Followup] - Let each script run in sequence even if one fails (1.09 KB, patch)
2016-06-24 15:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices (4.28 KB, patch)
2016-08-25 11:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11144 [QA Followup] - Let each script run in sequence even if one fails (1.56 KB, patch)
2016-08-25 11:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2013-10-27 03:46:25 UTC
Printing the correct fine in the overdues letters will only work correctly, when the fines.pl cronjob runs before overdue_notices.pl, else the fine is not there when the letter is created and can not be printed.
Comment 1 Katrin Fischer 2013-12-04 15:01:00 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2013-12-04 15:09:21 UTC
This is a simple patch, but not trivial to test if you want to make sure it works as before. What you could do:

Add fines and notices configuration to your installation
- Add circulation condition: 
    fine inverval 1, 
    fine 1.50, 
    grace period 0, 
    fines cap 3.00
- Checkout an item with due date yesterday
- Add an overdue notice letter that includes <<items.fine>>
- Add notice triggers with a delay matching your overdue item - 1
- Make sure finesmode is set to calculate
- Make sure patron has right patron category and email address

Edit crontab example to make it run not shortly after 1, but more suitable to your testing time. [This is cheating a bit, but not sure how to do this in a better way]

Wait until it has run.
- Check the fine is there
- Check the overdue letter for the fine
Before the patch, the fine is not printed.
- Update your crontab using the new example
- [Change time for it to be run]
- Repeat test. Fine should be printed now.

For installations using the packages a similar test can be done.
Comment 3 Galen Charlton 2013-12-18 23:04:32 UTC
Comment on attachment 23300 [details] [review]
Bug 11144: Run fines cronjob before overdues cronjob

Review of attachment 23300 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/crontab.example
@@ +48,5 @@
>  # FINES
> +0    1 * * *  __KOHA_USER__  $KOHA_CRON_PATH/fines.pl
> +
> +# OVERDUE NOTICES
> +5    1 * * *  __KOHA_USER__  $KOHA_CRON_PATH/overdue_notices.pl -t

There's a race condition here if the database is so large that fines.pl needs more than five minutes to run.  If that's the case, in theory some overdue notices could include the fine amount while others do not because fines.pl hasn't gotten to those patrons yet.

One way to fix this would be to have a single cron entry that first run fines.pl, then overdue_notices.pl.

Note that this doesn't affect koha-common.cron.daily since it runs the jobs in order, waiting for each one to finish.
Comment 4 Katrin Fischer 2013-12-19 06:53:46 UTC
Back to the drawing board... :)
Comment 5 Katrin Fischer 2015-06-09 21:21:54 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2015-06-09 21:22:10 UTC
A new attempt at this.
Comment 7 M. Tompsett 2015-06-18 00:17:47 UTC
Comment on attachment 40044 [details] [review]
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices

Review of attachment 40044 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/crontab.example
@@ +49,4 @@
>  # FINES
> +# OVERDUE NOTICES
> +# Sequence is important - renew what can be renewed, charge fines for the rest, print fine amount into overdue notices
> +5    1 * * *  __KOHA_USER__ $KOHA_CRON_PATH/automatic_renewals.pl && __KOHA_USER__  $KOHA_CRON_PATH/fines.pl && __KOHA_USER__  $KOHA_CRON_PATH/overdue_notices.pl -t

Not quite. __KOHA_USER__ the first time, sure. But those others seem wrong to me.

@@ -86,5 @@
>  # delete old purchase suggestions weekly. Replace XX with a number to define the age of suggestions to delete.
>  @weekly	__KOHA_USER__  $KOHA_CRON_PATH/purge_suggestions.pl --days XX > /dev/null 2>&1
> -
> -# every day at 3AM renew all issues scheduled for automatic renewal
> -0 3 * * *  __KOHA_USER__ $KOHA_CRON_PATH/automatic_renewals.pl

Do we want to shift renewals to 1:05am, or should we shift overdues and fines to 3:00am? I see no reason one way or another. Just wanted to point this out.
Comment 8 M. Tompsett 2015-06-18 00:20:43 UTC
The first __KOHA_USER__ tells the OS to run the command as that user. However, the second and third are like trying to run: ls & mtompset ls & mtompset ls.
Comment 9 Katrin Fischer 2015-07-09 02:09:17 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2015-07-09 02:09:49 UTC
Thx for checking Mark, I have uploaded a hopefully fixed patch.
Comment 11 Katrin Fischer 2015-11-01 20:10:37 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2015-11-01 20:11:16 UTC
Rebased on current master.
Comment 13 Katrin Fischer 2015-11-01 20:13:04 UTC
Calculating fines and then renewing an item can make patrons unhappy, updating severity a little bit.
Comment 14 M. Tompsett 2016-01-02 08:54:59 UTC
Comment on attachment 44283 [details] [review]
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices

Review of attachment 44283 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/crontab.example
@@ +48,4 @@
>  
> +# AUTOMATIC RENEWAL + FINES + OVERDUE NOTICES
> +# Sequence is important - renew what can be renewed, charge fines for the rest, print fine amount into overdue notices
> +5    1 * * *  __KOHA_USER__ $KOHA_CRON_PATH/automatic_renewals.pl && $KOHA_CRON_PATH/fines.pl && $KOHA_CRON_PATH/overdue_notices.pl -t

I'm pretty sure you want semi-colons. You want to run them sequentially in this order regardless of the success or failure of script.

echo one; false; echo three
echo one; true; echo three
echo one && false && echo three
echo one && true && echo three
echo one || false || echo three
echo one || true || echo three

Only the first two print one and three for both false and true cases
Comment 15 Chris Cormack 2016-01-16 02:31:06 UTC
Hmmm or maybe we do want them &&

Don't put fines on, if the automatic renewals script failed to run, or we will be fining people that we shouldn't

A case could be made for it either way

What do you think?
Comment 16 Katrin Fischer 2016-01-18 22:38:35 UTC
I think I agree with rangi - better to have it fail early, then to have to clean up all the patron accounts after figuring out what went wrong. 

What do others think?
Comment 17 M. Tompsett 2016-01-20 20:17:18 UTC
(In reply to Katrin Fischer from comment #16)
> I think I agree with rangi - better to have it fail early, then to have to
> clean up all the patron accounts after figuring out what went wrong. 
> 
> What do others think?

My concern is failure to automatically renew would fail to produce overdue notices with &&.
"You owe $100 in fines."
"But I never got an overdue notice!"

(R && F) ; O?
The fines won't accidentally charge on automatic renewal failures, but the overdues will run after the automatic renewals.
Does this look like a good solution?
Comment 18 M. Tompsett 2016-01-20 20:21:17 UTC
Comment on attachment 44283 [details] [review]
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices

Review of attachment 44283 [details] [review]:
-----------------------------------------------------------------

::: debian/koha-common.cron.daily
@@ +19,1 @@
>  koha-foreach --enabled /usr/share/koha/bin/cronjobs/fines.pl

Our discussion does raise a problem with this.
This runs the fines regardless of whether the renewals succeed or not.

@@ +19,2 @@
>  koha-foreach --enabled /usr/share/koha/bin/cronjobs/fines.pl
> +koha-foreach --enabled --email /usr/share/koha/bin/cronjobs/overdue_notices.pl -t

These three lines are the equivalent of semi-colons.
Comment 19 Katrin Fischer 2016-01-21 08:39:13 UTC
> My concern is failure to automatically renew would fail to produce overdue
> notices with &&.
> "You owe $100 in fines."
> "But I never got an overdue notice!"

Libraries print the fine amount for each item into the overdue notice - so if you send the overdue notices without generating the fines first, it will list 0,00 or a wrong amount, which is what started this bug report :) 
> 
> (R && F) ; O?
> The fines won't accidentally charge on automatic renewal failures, but the
> overdues will run after the automatic renewals.
> Does this look like a good solution?

see above

I'd also like to point out that we should try and decide something here - I am happy to make the changes, but currently this is really a bug with the sequence being totally wrong.
Comment 20 M. Tompsett 2016-01-21 13:39:59 UTC
So what is really needed is a way to:
- automatically renew
   output => list of failures
- generate fines
   input  => list of failures to exclude
   output => same or larger list of failures
- overdues
    input  => same or larger list of failures to exclude
    output => potentially bigger list of failures
- email potentially bigger list of failures to administrator to deal with.

If this list of failures could be passed from script to script to script, then semi-colons would work.
Comment 21 Katrin Fischer 2016-01-21 13:46:15 UTC
Can we have a simple fix first?

For your suggestion we'd need to touch a lot of scripts, and also:
- Auto-renewal - can be repeated without change for already renewed
- fines.pl - can be repeated, without problems
- overdue_notices - will generate a new notice if repeated. So you'd have to pass a positive list to avoid sending double notices.
Comment 22 Katrin Fischer 2016-01-21 13:52:10 UTC
> - overdue_notices - will generate a new notice if repeated. So you'd have to
> pass a positive list to avoid sending double notices.

Actually... I don't think that would work.

Why is it so bad if they all don't run, if there is really a bigger problem with one of them? That's how it is now and it doesn't seem to be a problem that occurs frequently. We have been running our installations with a changed cronjob sequence since the very beginning (fines before overdues).
Comment 23 M. Tompsett 2016-01-22 14:22:48 UTC
(In reply to Katrin Fischer from comment #22)
> > - overdue_notices - will generate a new notice if repeated. So you'd have to
> > pass a positive list to avoid sending double notices.
> 
> Actually... I don't think that would work.
> 
> Why is it so bad if they all don't run, if there is really a bigger problem
> with one of them? That's how it is now and it doesn't seem to be a problem
> that occurs frequently. We have been running our installations with a
> changed cronjob sequence since the very beginning (fines before overdues).

If overdues don't run, then people don't get emailed reminders, and then they get angry when they have a larger fine to pay. I would rather be reminded with a footnote stating that the amounts may be incorrect if an automatic renewals failed to run, than get hit with a whack load of fines. Tweak the notice and use semi-colons. That's my suggestion, because the current koha-foreach logic is effectively that.
Comment 24 Chris Cormack 2016-05-09 21:01:49 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2016-05-11 09:51:48 UTC
Just wondering:
With that change, if automatic_renewals.pl and/or fines.pl fail, overdue_notices.pl will not be launched. Is it really what we expect?
Comment 26 M. Tompsett 2016-05-11 10:53:57 UTC
(In reply to Jonathan Druart from comment #25)
> Just wondering:
> With that change, if automatic_renewals.pl and/or fines.pl fail,
> overdue_notices.pl will not be launched. Is it really what we expect?

I still think comment #17 is better: (R && F) ; O
Comment 27 Katrin Fischer 2016-05-11 10:58:13 UTC
For our libraries, I'd say yes - because the overdues sent would be wrong (wrong fines, overdues that shouldn't be overdue, but renewed).
Comment 28 Jonathan Druart 2016-05-11 11:16:57 UTC
(In reply to Katrin Fischer from comment #27)
> For our libraries, I'd say yes - because the overdues sent would be wrong
> (wrong fines, overdues that shouldn't be overdue, but renewed).

Yes but the letters will not be generated at all and the patron will not be notified.
Comment 29 Katrin Fischer 2016-05-11 11:52:35 UTC
I'd rather fix the first and have correct overdues sent, then having to sent another batch of emails to "correct" - but that's probably a problem mostly for libraries charging fines. 

Please note: This whole discussion is about the file for non-package setups. I think the behaviour for packages is different anyway. As we are using packages I am fine with any solution. We use debian/koha-common.cron.daily as changed in this patch for several versions now.
Comment 30 Katrin Fischer 2016-05-19 20:54:53 UTC
I still think that sending out wrong overdue notices is a bad bad thing. It's hard to communicate to patrons why you charge them a fine later, that was not listed in the notice they received - or why the items have been renewed the next day. Or why they receive a second overdue notice with different information once you have fixed things. I think overdue notices should only be generated if the first jobs finish correctly and ideally, if one of the jobs fails for whatever reason, we should have a way of notifying the library. 

But anyway, this has taken long now, we have a fix in production. I am ready to make changes as suggested, but I need some clear decision to do so.
Comment 31 Jonathan Druart 2016-06-12 08:31:41 UTC
Katrin,
I may be a bit lost here, but with this patch, the behavior changes between package and non-package installs.

(In reply to Katrin Fischer from comment #29)
> Please note: This whole discussion is about the file for non-package setups.
> I think the behaviour for packages is different anyway. As we are using
> packages I am fine with any solution. We use debian/koha-common.cron.daily
> as changed in this patch for several versions now.

The discussion is for both setups, as we want to have the same behavior :)
What I understand is that, using packages, the overdue notices will be generated even if the fines are not generated correctly. And it's not what you want apparently.
Comment 32 Jonathan Druart 2016-06-12 08:34:20 UTC
As this is a major one, I'd suggest to move the automatic_renewals script before the fines calculation, and then open a new bug report for the error handling problem.
Comment 33 Katrin Fischer 2016-06-22 20:34:04 UTC
*** Bug 16798 has been marked as a duplicate of this bug. ***
Comment 34 Kyle M Hall 2016-06-24 14:14:08 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-06-24 15:19:48 UTC Comment hidden (obsolete)
Comment 36 M. Tompsett 2016-06-24 15:47:44 UTC
From what I can read in these two patches, both generate the exact same ordering. And while it doesn't necessarily match desired perfection, I think they are an improvement. Do I need to sign off to get this to QA? Or is QA still in discussion about whether we are going to process these like comment 32 suggests? That is, push this to master, and open a new bug to work out the error cases / situations that Katrin and/or others might be concerned about.
Comment 37 Kyle M Hall 2016-06-27 13:52:08 UTC
(In reply to M. Tompsett from comment #36)
> From what I can read in these two patches, both generate the exact same
> ordering. And while it doesn't necessarily match desired perfection, I think
> they are an improvement. Do I need to sign off to get this to QA? Or is QA
> still in discussion about whether we are going to process these like comment
> 32 suggests? That is, push this to master, and open a new bug to work out
> the error cases / situations that Katrin and/or others might be concerned
> about.

As far as I understand, my followup takes care of the error handling problem. If you'd be willing to test and sign off I think this can go back to Passed QA.
Comment 38 Mirko Tietgen 2016-08-18 10:40:27 UTC
Can this go to Passed QA then?
Comment 39 Marcel de Rooy 2016-08-22 13:02:52 UTC
(In reply to Kyle M Hall from comment #37)
> As far as I understand, my followup takes care of the error handling
> problem. 

Not sure about that.
You just run them one after another without looking at the exit code of the previous script as initially proposed.
Since this keeps package and non-package more in sync, I would agree though.

I think we c/should try to resolve the effects of a failing cronjob on other cronjobs in another report. Some implicit consensus about that in the former comments. Sounds like a larger thing btw than just an exit code.
And now fix the obvious order.
[ Anyone still willing to write the koha-cron.pl that does it all (moving all code to modules)? .. ]

Can we merge both patches and have these cronjobs on separate lines again?
Comment 40 Marcel de Rooy 2016-08-25 10:47:25 UTC
(In reply to Marcel de Rooy from comment #39)
> Can we merge both patches and have these cronjobs on separate lines again?
Actually, better leave them as-is.
Comment 41 Marcel de Rooy 2016-08-25 11:53:04 UTC
Created attachment 54851 [details] [review]
Bug 11144: Fix sequence of cronjobs: automatic renewal - fines - overdue notices

The patch changes the sequence of cronjobs in the crontab example
file and in the cron.daily file of the packages.

This is why:
1) Renew automatically
   ... only when we can't renew, we want to
2) Calculate fines
   ... once the fine are calculated and charged
   we can print the amount into the
3) Overdue notices

Before the change it could happen that you'd charge for an item,
that would then be renewed. Or that you'd try to print fine
amounts into the overdue notices, when they would only be
charged moments later.

To test:
- configure your system so you have items that should
  - be charged with fines
  - renew automatically
- configure your crontabs according to the example file
  or switch the cron.daily in your package installation with
  the new one
- configure your overdue notices so that one should be generated
  <<items.fine>>
- Wait for the cronjobs or schedule them to run earlier
- Verify all is well and as it should be

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2016-08-25 11:53:14 UTC
Created attachment 54852 [details] [review]
Bug 11144 [QA Followup] - Let each script run in sequence even if one fails

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Tested the A;B;C variant here. If A fails, B will run. Since we can safely
assume that A (or B) will not fail on a daily basis, this seems to be better
than running them in the wrong order every day.

As the comments on Bugzilla show, several people support this improved
(reordered) scheme and look forward to improved error handling on another
report (obviously not that simple).
Comment 43 Marcel de Rooy 2016-08-25 11:56:01 UTC
Stirring up the discussion again in this final phase(?).
I do not see further objections to pass QA on this one.
Let's solve the "snowball errors" on new reports.
Comment 44 Kyle M Hall 2016-09-02 14:51:28 UTC
Pushed to master for 16.11, thanks for the followup Katrin!
Comment 45 Frédéric Demians 2016-09-08 19:18:17 UTC
Pushed in 16.05. Will be in 16.05.04.
Comment 46 Julian Maurice 2016-09-16 06:27:02 UTC
Pushed to 3.22.x, will be in 3.22.11