Bug 12529 - Overdue notices do not respect holidays
Summary: Overdue notices do not respect holidays
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13468 13679 15240
  Show dependency treegraph
 
Reported: 2014-07-03 21:09 UTC by Chris Cormack
Modified: 2017-06-14 22:10 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12529: Add a syspref to make overdue notices respect holidays (12.06 KB, patch)
2014-07-03 21:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Documentation of some test cases (320.00 KB, application/force-download)
2014-07-22 16:05 UTC, Katrin Fischer
Details
Bug 12529: Add a syspref to make overdue notices respect holidays (12.08 KB, patch)
2014-07-28 16:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12529: Add a syspref to make overdue notices respect holidays (12.08 KB, patch)
2014-07-28 17:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12529 [QA Followup] - Remove trailing whitespace (2.23 KB, patch)
2014-07-28 17:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12529: Add a syspref to make overdue notices respect holidays (12.13 KB, patch)
2014-08-01 11:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12529 [QA Followup] - Remove trailing whitespace (2.23 KB, patch)
2014-08-01 11:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12529: Add a syspref to make overdue notices respect holidays (12.17 KB, patch)
2014-08-08 11:50 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2014-07-03 21:09:51 UTC
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
Comment 1 Chris Cormack 2014-07-03 21:16:03 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-07-22 14:49:42 UTC
Hi Chris,
Did you see bug 11120? It seems you impacted the same code with this patch.
Comment 3 Katrin Fischer 2014-07-22 16:05:08 UTC
Created attachment 29956 [details]
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.
Comment 4 Jonathan Druart 2014-07-22 16:46:55 UTC
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.
Comment 5 Jonathan Druart 2014-07-22 16:48:10 UTC
I have written bz-splitter to avoid this kind of conflicts.
Comment 6 Kyle M Hall 2014-07-28 16:43:05 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2014-07-28 17:14:53 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2014-07-28 17:15:03 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2014-08-01 11:10:02 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2014-08-01 11:10:13 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2014-08-08 11:50:48 UTC
Created attachment 30622 [details] [review]
[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>
Comment 12 Martin Renvoize 2014-08-08 11:54:05 UTC
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.
Comment 13 Tomás Cohen Arazi 2014-08-15 14:14:38 UTC
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.
Comment 14 Jonathan Druart 2014-08-18 08:11:34 UTC
Martin and Tomas, did you see my comment 4 ?
Comment 15 Jonathan Druart 2015-02-06 09:30:06 UTC
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?
Comment 16 Jonathan Druart 2015-02-10 08:35:56 UTC
Thanks Chris for the fix (bug 13679). It looks easier than I expected :)
Comment 17 Sophie MEYNIEUX 2015-07-21 15:46:37 UTC
When using overdue_notices.pl with -t parameter, if a notice should be sent on a closed date, it will never be sent ?
Comment 18 Katrin Fischer 2015-07-21 20:50:26 UTC
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.
Comment 19 Sophie MEYNIEUX 2015-11-23 13:31:48 UTC
Performance problem found, see bug 15240
Comment 20 Jonathan Druart 2015-12-09 11:33:32 UTC
Comment on attachment 30622 [details] [review]
[PASSED QA] Bug 12529: Add a syspref to make overdue notices respect holidays

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

::: 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.
Comment 21 Jonathan Druart 2015-12-09 11:33:59 UTC
s/is/if
Comment 22 Katrin Fischer 2015-12-09 11:35:39 UTC
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.
Comment 23 Jonathan Druart 2015-12-09 15:44:30 UTC
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 :)