[Bug 12529] New: Overdue notices do not respect holidays
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Bug ID: 12529 Summary: Overdue notices do not respect holidays Change sponsored?: Sponsored Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Notices Assignee: koha-bugs@lists.koha-community.org Reporter: chris@bigballofwax.co.nz QA Contact: testopia@bugs.koha-community.org If you have holidays defined, and the syspref set, fines will not acrue for overdues when its a holiday. Notices however do not take holidays into account, so they will be sent even if holidays have meant the item is not actually overdue yet This will introduce a syspref, to make the overdue notices take the calendar into account -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #1 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 29507 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29507&action=edit Bug 12529: Add a syspref to make overdue notices respect holidays Test Plan Set up some overdue triggers, for example 5,10,15 Set up some holidays Create some items that are past due (one due 5 days, 10 days ago etc) Run the overdue notices script (misc/cronjobs/overdue_notices.pl) Notice holidays are ignored Apply the patch, Switch the OverdueNoticeCalendar syspref to Use calendar Run the overdue notices again Notice holidays are now taken into account Sponsored-by: BSZ -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11120 --- Comment #2 from Jonathan Druart <jonathan.druart@biblibre.com> --- Hi Chris, Did you see bug 11120? It seems you impacted the same code with this patch. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #3 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 29956 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=29956&action=edit Documentation of some test cases As BSZ has sponsored this feature I already got to test it quite a bit :) Basically we are trying to copy the logic that is used to calculate fines when finescalendar is set to "not including days the library is closed". With the new pref turned to on: No overdue notice will be sent out on a holiday. Also, if the library is closed, the overdue notice will be delayed accordingly. The overdue notice delay can be read as 'days the library is open' instead of 'calendar days'. This way the patron always has the same amount of days he actually can return the items before the next overdue notice is sent. With the new pref off, the current behaviour won't be changed at all. == Example test plan == So for testing I added a fines configuration that would result in my overdue notices being triggered the same day the fine would go up according to the calendar. This way I could check the logic was the same. I tested without the patch and with both possible settings for the new preference. = Configuration = - multiple items on multiple records with the itemtype "Books" - circulation rule: * fine amount: 1.00 * charging interval: 5 * fine grace period: 0 * overdues fines cap: 3.00 - calendar: the library is closed on Saturday and Sunday * Note: Add more holidays later and rerun tests - add 3 different overdue notices, so you can easily see which one has been triggered, I used: * ODUE1, Overdue 1 * ODUE2, Overdue 2 * ODUE3, Overdue 3 - Overdue notice triggers * Start with for the patron category you want to test with: * ODUE1, delay 5 * ODUE2, delay 10 * ODUE3, delay 15 * Note: Change settings later and rerun tests - System preferences * finesCalendar - not including days the library is closed * OverdueNoticeCalendar - ignore calendar (previous behaviour) * Note: compare this to the behaviour without the patch being applied and then swith the pref to confirm the new behaviour * CalculateFinesOnReturn - Don't (for easier without having to waive fines) = Test = - Check out items to a patron with the chosen category. Due dates should be in the past. Note: For some of my tests I checked out 1 item for each day the last 3 weeks or similar. Sometimes it can also be useful to only check out 1 item and see which due dates will trigger the overdue notice exactly. - run fines.pl to create the fines - run overdue_notices.pl -t [-v] - If you want to send the overdue notices, you can also run: process_messagequeue.pl or you can check the generated overdue notices in the patron account (notices tab) or in the database (message_queue table). I ran the scripts 3 times for each scenario I created, once on a branch without the patch and then with the patch and both settings for the new system preference 'OverdueNoticeCalendar'. After each test I truncated the accountlines and message_queue and reran the scripts. I cleaned up and translated some of my test cases, adding those as attachement. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #4 from Jonathan Druart <jonathan.druart@biblibre.com> --- IMO, this patch should be based on top of bug 11120 (waiting for a signoff for 8 months now) , especially if it does not fix the --date parameter. This patch is more than an enh than a bugfix and the importance of --date should be considered. This parameter is the *only* way to recover unsent overdue notifications (for instance the script has crashed, the server was down, you forgot to add the cronjob, etc.). Actually it is not the only way, you can do it manually. But trust me, this parameter is very useful and it could help anyone at one point. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #5 from Jonathan Druart <jonathan.druart@biblibre.com> --- I have written bz-splitter to avoid this kind of conflicts. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #29507|0 |1 is obsolete| | --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 30225 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30225&action=edit Bug 12529: Add a syspref to make overdue notices respect holidays Test Plan Set up some overdue triggers, for example 5,10,15 Set up some holidays Create some items that are past due (one due 5 days, 10 days ago etc) Run the overdue notices script (misc/cronjobs/overdue_notices.pl) Notice holidays are ignored Apply the patch, Switch the OverdueNoticeCalendar syspref to Use calendar Run the overdue notices again Notice holidays are now taken into account Sponsored-by: BSZ -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #30225|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 30226 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30226&action=edit Bug 12529: Add a syspref to make overdue notices respect holidays Test Plan Set up some overdue triggers, for example 5,10,15 Set up some holidays Create some items that are past due (one due 5 days, 10 days ago etc) Run the overdue notices script (misc/cronjobs/overdue_notices.pl) Notice holidays are ignored Apply the patch, Switch the OverdueNoticeCalendar syspref to Use calendar Run the overdue notices again Notice holidays are now taken into account Sponsored-by: BSZ -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 30227 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30227&action=edit Bug 12529 [QA Followup] - Remove trailing whitespace -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #30226|0 |1 is obsolete| | Attachment #30227|0 |1 is obsolete| | --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 30449 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30449&action=edit Bug 12529: Add a syspref to make overdue notices respect holidays Test Plan Set up some overdue triggers, for example 5,10,15 Set up some holidays Create some items that are past due (one due 5 days, 10 days ago etc) Run the overdue notices script (misc/cronjobs/overdue_notices.pl) Notice holidays are ignored Apply the patch, Switch the OverdueNoticeCalendar syspref to Use calendar Run the overdue notices again Notice holidays are now taken into account Sponsored-by: BSZ Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 30450 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30450&action=edit Bug 12529 [QA Followup] - Remove trailing whitespace -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 30622 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30622&action=edit [PASSED QA] Bug 12529: Add a syspref to make overdue notices respect holidays Test Plan Set up some overdue triggers, for example 5,10,15 Set up some holidays Create some items that are past due (one due 5 days, 10 days ago etc) Run the overdue notices script (misc/cronjobs/overdue_notices.pl) Notice holidays are ignored Apply the patch, Switch the OverdueNoticeCalendar syspref to Use calendar Run the overdue notices again Notice holidays are now taken into account Sponsored-by: BSZ Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #30450|0 |1 is obsolete| | CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #30449|0 |1 is obsolete| | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Happy to pass, meets guidelines and does what it says on the tin. I'd like at some point to see such scripts re-factored to move code out of the script and into a module in the Koha namespace. It worries me that things like this have no unit or regression tests :(. Still, it passes the current muster, so it can't be a fail for this reason. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patch pushed to master. Thanks Chris! Note: I agree with Martin that this script should be refactored soon, so we are able to properly have regression tests. Everytime this script is changed people fear some use case is broken (as it happens to C4::Auth, C4::Search... etc). But its friday, and we don't care until monday. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #14 from Jonathan Druart <jonathan.druart@biblibre.com> --- Martin and Tomas, did you see my comment 4 ? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Marjorie Barry-Vila <marjorie.barry-vila@ccsr.qc.ca> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |marjorie.barry-vila@ccsr.qc | |.ca -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13468 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529
--- Comment #15 from Jonathan Druart <jonathan.druart@biblibre.com> ---
I am under the impression there is a big issue introduced by this patch,
MariaDB [koha]> select itemnumber,date_due from issues where
borrowernumber=8408;
+------------+---------------------+
| itemnumber | date_due |
+------------+---------------------+
| 27127 | 2015-03-03 23:59:00 |
| 27143 | 2015-01-01 14:31:00 |
| 27754 | 2014-12-22 14:31:00 |
+------------+---------------------+
Overdue rules are 1:7, 2:15, 3:21
On master, the following diff
diff --git a/misc/cronjobs/overdue_notices.pl
b/misc/cronjobs/overdue_notices.pl
index 4d05ff7..d4dd153 100755
--- a/misc/cronjobs/overdue_notices.pl
+++ b/misc/cronjobs/overdue_notices.pl
@@ -627,6 +627,9 @@ END_SQL
}
}
else {
+ if ( $borrowernumber == 8408 ) {
+ warn "$item_info->{itemnumber} : $days_between
>= $mindays && $days_between <= $maxdays";
+ }
unless ( $days_between >= $mindays
&& $days_between <= $maxdays )
{
output:
27127 : 25 >= 21 && 25 <= 90 at misc/cronjobs/overdue_notices.pl line 631.
27143 : 36 >= 21 && 36 <= 90 at misc/cronjobs/overdue_notices.pl line 631.
27754 : 46 >= 21 && 46 <= 90 at misc/cronjobs/overdue_notices.pl line 631.
The itemnumber 27127 should not be embedded in the notice (it is).
The problem comes from the $MAX=90 and the $date_between.
Note that I launched the script without the -t option, but I am pretty sure
some cases can cause problem even with -t
Don't have the time to investigate more.
Could someone confirm?
--
You are receiving this mail because:
You are the assignee for the bug.
You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13679 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #16 from Jonathan Druart <jonathan.druart@biblibre.com> --- Thanks Chris for the fix (bug 13679). It looks easier than I expected :) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Sophie MEYNIEUX <sophie.meynieux@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sophie.meynieux@biblibre.co | |m --- Comment #17 from Sophie MEYNIEUX <sophie.meynieux@biblibre.com> --- When using overdue_notices.pl with -t parameter, if a notice should be sent on a closed date, it will never be sent ? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hm, did you test? I think the notice should be send out the next opening day, as the number of days the library is open (the delay) remains the same until we hit a day the library is open again. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Sophie MEYNIEUX <sophie.meynieux@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=15240 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #19 from Sophie MEYNIEUX <sophie.meynieux@biblibre.com> --- Performance problem found, see bug 15240 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Sophie MEYNIEUX <sophie.meynieux@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15240 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15240 [Bug 15240] Performance issue running overdue_notices.pl -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 30622 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30622 [PASSED QA] Bug 12529: Add a syspref to make overdue notices respect holidays Review of attachment 30622: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=12529&attachment=30622) ----------------------------------------------------------------- ::: misc/cronjobs/overdue_notices.pl @@ +422,5 @@
foreach my $branchcode (@branches) { + if ( C4::Context->preference('OverdueNoticeCalendar') ) { + my $calendar = Koha::Calendar->new( branchcode => $branchcode ); + if ( $calendar->is_holiday($date_to_run) ) { + next;
Why do we skip the library is today is holiday? It seems that should only be done if --triggered|-t is given. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- s/is/if -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #22 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I think the idea is that you don't want to have a fine set on a holiday - but making it so that it depends on -t would work for us. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- For me it's a bug, I don't plan to fix it if I am the only one to think it's a bug :) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12529 Janet McGowan <janet.mcgowan@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |janet.mcgowan@ptfs-europe.c | |om -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org