Bug 27180 - Fines cronjob does not update fines on holidays when finesCalendar is set to ignore
Summary: Fines cronjob does not update fines on holidays when finesCalendar is set to ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 27835
  Show dependency treegraph
 
Reported: 2020-12-09 15:34 UTC by Nick Clemens
Modified: 2021-12-13 21:09 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Prior to this patchset the fines cronjob would not run on holidays. This was to ensure that dropbox mode correctly decremented fines. Dropbox mode has been rewritten and we can now correctly calculate and charge fines on holidays (or not) following the calendar and system preferences.
Version(s) released in:
21.05.00,20.11.02,20.05.08


Attachments
Bug 27180: Update fines on holidays (2.46 KB, patch)
2020-12-09 15:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27180: Update fines on holidays (2.52 KB, patch)
2020-12-15 08:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27180: (follow-up) Remove unused %is_holiday (759 bytes, patch)
2020-12-26 14:23 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27180: Update fines on holidays (2.58 KB, patch)
2021-01-17 17:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27180: (follow-up) Remove unused %is_holiday (816 bytes, patch)
2021-01-17 17:20 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-12-09 15:34:21 UTC
The fines cronjob currently skips updating fines if the date it is run is a holiday for the applicable library.

This is problematic when an item is marked lost the day following a holiday as we can end up missing a day of fines. Additionally it confuses things for libraries who do not expect the calendar to affect fines.

This seems to have been done when dropbox mode used calculations on the 'last update' of a fine to determine how much to refund.

Now that dropbox mode simply calculates the fine with a different return date this code is no longer needed.
Comment 1 Nick Clemens 2020-12-09 15:43:20 UTC
Created attachment 114287 [details] [review]
Bug 27180: Update fines on holidays

This patch removes code that prevents fines from being update on holidays.
The fines are already being calculated, and that takes the calendar into account if
needed, so fines won't be assessed on holidays if they shouldn't.

To test:
 1 - set finesCalendar to 'ignore'
 2 - Checkout an item due yesterday
 3 - Ensure circ rules have a fine amount set
 4 - Make today a holiday
 5 - run fines.pl
 6 - No fine assessed
 7 - Apply patch
 8 - run fines.pl
 9 - Fine assessed!
10 - set finesCalendar to 'use'
11 - checkout an item, due yesterday
12 - run fines.pl
13 - no fine assessed
Comment 2 Martin Renvoize 2020-12-15 08:43:59 UTC
Created attachment 114404 [details] [review]
Bug 27180: Update fines on holidays

This patch removes code that prevents fines from being update on holidays.
The fines are already being calculated, and that takes the calendar into account if
needed, so fines won't be assessed on holidays if they shouldn't.

To test:
 1 - set finesCalendar to 'ignore'
 2 - Checkout an item due yesterday
 3 - Ensure circ rules have a fine amount set
 4 - Make today a holiday
 5 - run fines.pl
 6 - No fine assessed
 7 - Apply patch
 8 - run fines.pl
 9 - Fine assessed!
10 - set finesCalendar to 'use'
11 - checkout an item, due yesterday
12 - run fines.pl
13 - no fine assessed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2020-12-15 08:44:35 UTC
This makes allot of sense and cleans up the code... Works as expected, signing off.
Comment 4 Victor Grousset/tuxayo 2020-12-26 14:23:25 UTC
Created attachment 114706 [details] [review]
Bug 27180: (follow-up) Remove unused %is_holiday
Comment 5 Martin Renvoize 2021-01-11 12:36:36 UTC
I think you could probably set this to PQA Victor.. I'd call your followup a 'QA follow-up) as it's trivial and works well..
Comment 6 Katrin Fischer 2021-01-17 17:20:10 UTC
Created attachment 115228 [details] [review]
Bug 27180: Update fines on holidays

This patch removes code that prevents fines from being update on holidays.
The fines are already being calculated, and that takes the calendar into account if
needed, so fines won't be assessed on holidays if they shouldn't.

To test:
 1 - set finesCalendar to 'ignore'
 2 - Checkout an item due yesterday
 3 - Ensure circ rules have a fine amount set
 4 - Make today a holiday
 5 - run fines.pl
 6 - No fine assessed
 7 - Apply patch
 8 - run fines.pl
 9 - Fine assessed!
10 - set finesCalendar to 'use'
11 - checkout an item, due yesterday
12 - run fines.pl
13 - no fine assessed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2021-01-17 17:20:13 UTC
Created attachment 115229 [details] [review]
Bug 27180: (follow-up) Remove unused %is_holiday

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Jonathan Druart 2021-01-19 15:24:12 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 9 Fridolin Somers 2021-01-20 08:58:15 UTC
Pushed to 20.11.x for 20.11.02
Comment 10 Andrew Fuerste-Henry 2021-01-22 20:46:01 UTC
Pushed to 20.05.x for 20.05.08
Comment 11 Victor Grousset/tuxayo 2021-01-22 21:29:28 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.
Comment 12 Jonathan Druart 2021-02-09 11:01:50 UTC
Nick, please fill the release notes.
Comment 13 Katrin Fischer 2021-03-10 13:45:51 UTC
It appears like now we are charging on closed days with this bug... see bug 27835. For our use case that is definitely unwanted behaviour and we need to fix it asap. 

I know I QA'd that... but I was under the assumtpion it would only affect finescalendar = ignore... which it doesn't.