Bug 11144

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 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.

Back to the drawing board... :)

A new attempt at this.

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.

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.

Thx for checking Mark, I have uploaded a hopefully fixed patch.

Rebased on current master.

Calculating fines and then renewing an item can make patrons unhappy, updating severity a little bit.

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

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?

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?

(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 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.

> 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.

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.

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.

> - 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).

(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.

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?

(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

For our libraries, I'd say yes - because the overdues sent would be wrong (wrong fines, overdues that shouldn't be overdue, but renewed).

(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.

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.

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.

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.

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.

*** Bug 16798 has been marked as a duplicate of this bug. ***

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.

(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.

Can this go to Passed QA then?

(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?

(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.

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>

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).

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.

Pushed to master for 16.11, thanks for the followup Katrin!

Pushed in 16.05. Will be in 16.05.04.

Pushed to 3.22.x, will be in 3.22.11