We have configured our calendar to have closed days on Saturdays and Sundays but the fines job executes on Saturdays, but not on Mondays. It seems like it offsets incorrectly with one day. I think the problem started after updating to Koha version 20.05.09.
Can you check that your server time is correctly set?
I ran `timedatectl` and got `Local time: Fri 2021-03-05 09:14:07 CET` which is correct. It would surprise me if the clock was off by a whole day. But since no one else has reported this I guess it has to be some configuration issue on my part. Otherwise more libraries would have had this problem.
It would be a bit scary if we broke that, so I am interested to see if we can figure it out. What are your settings for fines? - fine amount - max fine - interval - charge at beginning/end of interval? - finescalendar system preference If you turn on the cronjobs log in the system preferences you can see when the fine job runs. Also the fines log will show when exactly things were charged. Actually it would be a little odd if it charged on a holiday, as this is the first thing that is checked... could it be a missing or wrong setting in a branch calendar?
Thanks for taking an interest in our problem. When I check the calendar we have closed on Saturdays and Sundays. Weekly - Repeatable holidays Day of week Title Description Saturdays Biblioteket stängt pga Crona Sundays BIblioteket stängt pga corona Saturdays and Sundays are also marked as yellow in the gui. (We have two branches but the other is only used for ILL requests.) When I check the log, many borrowers got a fine on Saturday at 11, which corresponds with the cron job configuration, but Saturdays are closed in the calendar. The con job also ran on Sunday, but no fines where added, and on Monday and than - which is really strange - no fines where added. Monday is an open day in the calendar. Here is the information you required: - fine amount it's complicated. different patron types have different amount on different item types. But for one of our most common combination it's 10 SEK a day. for day loans it's 100 SEK a day. - max fine it differs between 100 SEK, 150 SEK to 200 SEK - interval fine charging interval = 1 - charge at beginning/end of interval? end of interval - finescalendar system preference "Use the calendar when calculating the period for fines." Hope this information gives you any clues in further investigation.
What happened on Tuesday? Says we have the following situation: March 2021 Su Mo Tu We Th Fr Sa 1 2 3 4 5 6 7 8 9 *10* 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 Wednesdays are closed. Date due is Mon 8. 2€ each day late (fine amount=2, interval=1) Cronjob run: on Monday: no fine is generated on Tuesday: fine of 2€ is generated (Monday) on Wednesday: additional fine of 2€ is generated (Monday + Tuesday) on Thursday: no additional fine is generated (Wed was closed) The cronjob must run early in the morning! Isn't it how it is supposed to work?
(In reply to Jonathan Druart from comment #5) > What happened on Tuesday? > > Says we have the following situation: > > March 2021 > Su Mo Tu We Th Fr Sa > 1 2 3 4 5 6 > 7 8 9 *10* 11 12 13 > 14 15 16 17 18 19 20 > 21 22 23 24 25 26 27 > 28 29 30 31 > > Wednesdays are closed. > Date due is Mon 8. > > 2€ each day late (fine amount=2, interval=1) > > Cronjob run: > on Monday: no fine is generated > on Tuesday: fine of 2€ is generated (Monday) > on Wednesday: additional fine of 2€ is generated (Monday + Tuesday) > on Thursday: no additional fine is generated (Wed was closed) > > The cronjob must run early in the morning! Correct. > Isn't it how it is supposed to work? I think not. It should be: > Cronjob run: > on Monday: no fine is generated (usually you can return until 23:59) > on Tuesday: fine of 2€ is generated (for Tuesday) > on Wednesday: no fine is generated - library is closed! > on Thursday: an additional 2€ is generated (for Thursday) In the past, Wednesday would not generate a fine. The script would exit when "today" was a holiday. I am not sure if we have changed that - but I hope not as our use cases requires it looks like above. Here when the library is closed, nothing on the account should "move". Could settings for fine interval (charge at beginning or end) and finescalendar have different effects here?
Maybe it was working like that, and I don't know if the behaviour changed, but I am not convinced it's the expected behaviour. If you return on a closed day (using a box), aren't you still 1 day late? Maybe it's related to bug 25850? (In reply to Stefan Norinder from comment #0) > I think the problem started after updating to Koha version 20.05.09. From which version?
Ha found it, it's coming from bug 27180.
(In reply to Jonathan Druart from comment #7) > Maybe it was working like that, and I don't know if the behaviour changed, > but I am not convinced it's the expected behaviour. > > If you return on a closed day (using a box), aren't you still 1 day late? So if you returned on the closed Wednesday, it will be backdated to thelast opening day usually. You'd pay the 2 charged on Tuesday, but not more.
(In reply to Jonathan Druart from comment #8) > Ha found it, it's coming from bug 27180. I understand it changes the logic here, but I think the end results are the same. In comment 4: previous: borrower was charged late on monday morning when the cron ran - if borrower returns on monday they are charged 1 day current: borrower was charged late on saturday morning when the cron ran - if borrower returns on monday they are charged 1 day The two cases highlighted later: > Cronjob run: > on Monday: no fine is generated > on Tuesday: fine of 2€ is generated (Monday) > on Wednesday: additional fine of 2€ is generated (Monday + Tuesday) > on Thursday: no additional fine is generated (Wed was closed) VS > Cronjob run: > on Monday: no fine is generated (usually you can return until 23:59) > on Tuesday: fine of 2€ is generated (for Tuesday) > on Wednesday: no fine is generated - library is closed! > on Thursday: an additional 2€ is generated (for Thursday) Patron is charged 2 days if returned thursday (after cron run) for either The change shouldn't affect the amounts owed, just the time when they are calculated
(In reply to Nick Clemens from comment #10) > (In reply to Jonathan Druart from comment #8) > > Ha found it, it's coming from bug 27180. > > I understand it changes the logic here, but I think the end results are the > same. > > In comment 4: > previous: borrower was charged late on monday morning when the cron ran - if > borrower returns on monday they are charged 1 day > current: borrower was charged late on saturday morning when the cron ran - > if borrower returns on monday they are charged 1 day > > The two cases highlighted later: > > Cronjob run: > > on Monday: no fine is generated > > on Tuesday: fine of 2€ is generated (Monday) > > on Wednesday: additional fine of 2€ is generated (Monday + Tuesday) > > on Thursday: no additional fine is generated (Wed was closed) > VS > > Cronjob run: > > on Monday: no fine is generated (usually you can return until 23:59) > > on Tuesday: fine of 2€ is generated (for Tuesday) > > on Wednesday: no fine is generated - library is closed! > > on Thursday: an additional 2€ is generated (for Thursday) > > Patron is charged 2 days if returned thursday (after cron run) for either > > The change shouldn't affect the amounts owed, just the time when they are > calculated It doesn't affect the amounts owed, but the timing, We depend on the timing. We need to be able to send the overdue notices on the same day the fines are charged. Not a day after. We need this regression to be fixed.
Created attachment 118032 [details] [review] Bug 27835: Do not count closed day if holiday when calculating fines Bug 27180 attempted to allow calculating fines on holidays when the calendar was ignored but it affected the day when an item would be charged when following the calendar for fines The difference seems to be that get_chargeable_units returns 1 if the start date is yesterday and today is a holiday. If finesCalendar is set to 'noFinesWhenClosed' then there should be 0 chargable units To test: 1 - Checkout an item to a patron, due yesterday 2 - Confirm the circ rules are set to charge .1 per 1 day 3 - perl misc/cronjobs/fines.pl -v 4 - Patron is charged .1 5 - Clear the checkout and fine and recreate 6 - Apply patch 7 - perl misc/cronjobs/fines.pl -v 8 - Patron is charged 0 9 - prove -v t/db_dependent/Circulation/CalcFine.t
Created attachment 118033 [details] [review] Bug 27835: Fix unit test Without this patch the unit tests fail. This highlights a potential issue When ignoring holidays we get chargeable units as: $date_returned->delta_days( $date_due ); When excluding holidays: $calendar->days_between( $date_due, $date_returned ); days_between does a loop: 328 while( $start_dt->compare($end_dt) < 1 ) { 329 $delta_days-- if $self->is_holiday($start_dt); 330 $start_dt->add( days => 1 ); 331 } Form docs https://metacpan.org/pod/DateTime#DateTime-%3Ecompare(-$dt1,-$dt2-),-DateTime-%3Ecompare_ignore_floating(-$dt1,-$dt2-) This method compare two DateTime objects. The semantics are compatible with Perl's sort function; it returns -1 if $dt1 < $dt2, 0 if $dt1 == $dt2, 1 if $dt1 > $dt2. So '2000-01-30 00:00:00' compares to itself as '0' and is less than 1 Is that correct? Or should the loop use "> -1"
Aren't we mixing 2 different behaviours? noFinesWhenClosed and "don't generate fines on close days"?
(In reply to Jonathan Druart from comment #14) > Aren't we mixing 2 different behaviours? > > noFinesWhenClosed and "don't generate fines on close days"? Can you explain that a bit more? Where is noFinesWhenClosed coming from? Before the change the behaviour for the fines.pl cronjob was that no fines would be generated on the days the library was closed and also not for closed days. The first bit was changed and needs to be fixed.
(In reply to Katrin Fischer from comment #15) > (In reply to Jonathan Druart from comment #14) > > Aren't we mixing 2 different behaviours? > > > > noFinesWhenClosed and "don't generate fines on close days"? > > Can you explain that a bit more? Where is noFinesWhenClosed coming from? OK, I figured it out: internal setting for finecalendar = Use the calendar. > Before the change the behaviour for the fines.pl cronjob was that no fines > would be generated on the days the library was closed and also not for > closed days. The first bit was changed not only for the 'ignore' setting, as the patch claimed, but also for the noFinesWhenClosed setting and this needs to be fixed. The total fine on a day the library is open should not change by this. It's about the timing of increasing a charge.
Maybe the best would be to revert bug 27180 for now and then restart discussions? Or limit it to finescalendar = ignore.
(In reply to Katrin Fischer from comment #17) > Maybe the best would be to revert bug 27180 for now and then restart > discussions? Or limit it to finescalendar = ignore. Do the new patches solve the issue?
I think I finally understood what Joubu meant during the weekend. The change Nick suggests here makes a change for the holidays, but when calculating the difference, so I think it won't stop charging on holidays in all circumstances. I'd need to do more testing, but I believe it doesn't fix the issue or only fixes the issue for some constellations. We don't want to charge on a holiday, so the check shoudl be done higher up.
Created attachment 118288 [details] [review] Bug 27835: Revert "Bug 27180: (follow-up) Remove unused %is_holiday" This reverts commit a26d529a2bc7faee3743b4cd1e603ba29411a0b9. Revert "Bug 27180: Update fines on holidays" This reverts commit 80e1b4e66f01a07cad9bf37625d879f649edf52a.
Created attachment 118289 [details] [review] Bug 27835: Add new syspref ChargeFinesOnCloseDay
Created attachment 118290 [details] [review] Bug 27835: Add a syspref to control fines calculation on closed days Bug 27180 added a patch to not update fines on holidays if finesCalendar was set to ignore. At least it's what it advertised, but not what it did. It seems that we need to add a new pref to control the calculation of fines on closed days to answer the different use cases. Test plan: With this patch applied you can run the fines.pl cronjob and play with the new pref ChargeFinesOnCloseDay to generate fines on close days.
Nick, Katrin, I had a look at this one and it seems that the only things we need is to: 1. Revert 27180 2. Add a pref to control the behaviour Those 3 patches are an alternative approach and must be applied separately.
Created attachment 118319 [details] [review] Bug 27835: Revert "Bug 27180: (follow-up) Remove unused %is_holiday" This reverts commit a26d529a2bc7faee3743b4cd1e603ba29411a0b9. Revert "Bug 27180: Update fines on holidays" This reverts commit 80e1b4e66f01a07cad9bf37625d879f649edf52a. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 118320 [details] [review] Bug 27835: Add new syspref ChargeFinesOnCloseDay Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 118321 [details] [review] Bug 27835: Add a syspref to control fines calculation on closed days Bug 27180 added a patch to not update fines on holidays if finesCalendar was set to ignore. At least it's what it advertised, but not what it did. It seems that we need to add a new pref to control the calculation of fines on closed days to answer the different use cases. Test plan: With this patch applied you can run the fines.pl cronjob and play with the new pref ChargeFinesOnCloseDay to generate fines on close days. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 118322 [details] [review] Bug 27835: (follow-up) Update language, remove warn, add feedback This patch: - Updates lanugage to be clearer 'ChargeFinesOnClosedDays' vs 'ChargeFinesOnCloseDay' - Removes a stary warn - Add an 'updated' counter and provides feedback in fines.pl Test plan: 0 - Apply patches, updatedatabase 1 - set finesCalendar to 'ignore', ChargeFinesOnClosedDays to "Don't charge" 2 - Checkout an item due yesterday 3 - Ensure circ rules have a fine amount and interval set 4 - Make today a holiday 5 - perl misc/cronjobs/fines.pl -v 6 - 0 updated 7 - ChargeFinesOnClosedDays to "Charge" 8 - perl misc/cronjobs/fines.pl -v 9 - 1 updated 10 - set fines calendar to 'use' 11 - perl misc/cronjobs/fines.pl -v 12 - 1 updated (NOTE: This is wrong maybe, but handle on another bug) 13 - set ChargeFinesOnClosedDays to "Don't charge" 14 - perl misc/cronjobs/fines.pl -v 12 - 0 updated Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I have looked at the code and I believe these patches will fix the issue, but sadly couldn't test them properly :( Only thing. I believe we should restore the former behaviour so there is no behaviour change for most people on update. I think this would mean changing the default for the pref to 0 = Don't charge on closed days. +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + + $dbh->do(q| + INSERT IGNORE INTO systempreferences (variable,value,options,explanation,type) + VALUES ('ChargeFinesOnCloseDay', '1', NULL, 'Charge fines on close day.', 'YesNo') + |); + + NewVersion( $DBversion, 27835, "Add new system preference ChargeFinesOnCloseDay"); +}
Created attachment 118361 [details] [review] Bug 27835: Turn off by default
Created attachment 118414 [details] [review] Bug 27835: Revert "Bug 27180: (follow-up) Remove unused %is_holiday" This reverts commit a26d529a2bc7faee3743b4cd1e603ba29411a0b9. Revert "Bug 27180: Update fines on holidays" This reverts commit 80e1b4e66f01a07cad9bf37625d879f649edf52a. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118415 [details] [review] Bug 27835: Add new syspref ChargeFinesOnCloseDay Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118416 [details] [review] Bug 27835: Add a syspref to control fines calculation on closed days Bug 27180 added a patch to not update fines on holidays if finesCalendar was set to ignore. At least it's what it advertised, but not what it did. It seems that we need to add a new pref to control the calculation of fines on closed days to answer the different use cases. Test plan: With this patch applied you can run the fines.pl cronjob and play with the new pref ChargeFinesOnCloseDay to generate fines on close days. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118417 [details] [review] Bug 27835: (follow-up) Update language, remove warn, add feedback This patch: - Updates lanugage to be clearer 'ChargeFinesOnClosedDays' vs 'ChargeFinesOnCloseDay' - Removes a stary warn - Add an 'updated' counter and provides feedback in fines.pl Test plan: 0 - Apply patches, updatedatabase 1 - set finesCalendar to 'ignore', ChargeFinesOnClosedDays to "Don't charge" 2 - Checkout an item due yesterday 3 - Ensure circ rules have a fine amount and interval set 4 - Make today a holiday 5 - perl misc/cronjobs/fines.pl -v 6 - 0 updated 7 - ChargeFinesOnClosedDays to "Charge" 8 - perl misc/cronjobs/fines.pl -v 9 - 1 updated 10 - set fines calendar to 'use' 11 - perl misc/cronjobs/fines.pl -v 12 - 1 updated (NOTE: This is wrong maybe, but handle on another bug) 13 - set ChargeFinesOnClosedDays to "Don't charge" 14 - perl misc/cronjobs/fines.pl -v 12 - 0 updated Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118418 [details] [review] Bug 27835: Turn off by default Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Hi Tomas, thx for the QA and Joubu for the follow-up!
Pushed to master for 21.05, thanks to everybody involved!
Please push to the stable releases as well!
Pushed to 20.11.x for 20.11.04
Pushed to 20.05.x for 20.05.10
Missing dependencies for 19.11.x, it shouldn't be affected, no backport.