Bug 18064 - Long overdue cronjob does not follow the library's calendar and skip closed days
Summary: Long overdue cronjob does not follow the library's calendar and skip closed days
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P4 enhancement with 50 votes (vote)
Assignee: Thibaud Guillot
QA Contact: Testopia
URL:
Keywords:
: 22380 (view as bug list)
Depends on: 9596
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-06 15:11 UTC by Jessie Zairo
Modified: 2023-11-17 15:14 UTC (History)
19 users (show)

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


Attachments
Bug 18064 : Add syspref to launch longOverdue script with calendar support (8.78 KB, patch)
2022-02-09 14:18 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 18064: (follow-up) Get libraries as a list (879 bytes, patch)
2022-06-09 14:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18064 : Add syspref to launch longOverdue script with calendar support (8.77 KB, patch)
2022-06-16 22:24 UTC, Michal Urban
Details | Diff | Splinter Review
Bug 18064: (follow-up) Get libraries as a list (2.22 KB, patch)
2022-06-16 22:24 UTC, Michal Urban
Details | Diff | Splinter Review
Bug 18064: (QA follow-up) Clean up code (2.49 KB, patch)
2022-06-17 13:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 18064: (follow-up) Remove statement for summary & reorder sysprefs (3.77 KB, patch)
2022-08-30 10:27 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 18064: (follow up) rebase on master and reorder syspref (7.58 KB, patch)
2023-02-01 08:21 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 18064: (follow-up) Fix var type 'holidays_by_branch' (920 bytes, patch)
2023-02-28 15:29 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 18064: (follow-up) Fix var type 'holidays_by_branch' (920 bytes, patch)
2023-02-28 15:30 UTC, Thibaud Guillot
Details | Diff | Splinter Review
Bug 18064: Add syspref to launch longOverdue script with calendar support (9.33 KB, patch)
2023-11-17 14:41 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Zairo 2017-02-06 15:11:26 UTC
The cronjob that sets the Long overdue does not follow the library's calendar and skip closed days.

However, the cronjob that sends the email (overdue notices) does follow the library's calendar.

So as an example on a weekend where Monday is a holiday, the long over due may get set and chanrge the patrons account, but the email may be sent a day or 2 later. 

It would be good to allow the long overdue cron to follow the library calendar and skip closed days.
Comment 1 Nick Clemens 2017-02-06 15:16:57 UTC
Just a thought on this onem it owuld be nice to add a fourth notice with options for:
'mark returned'
'charge'
'lost value'

And allow the fourth notice (or any other if we add options to all) to call the relevant code from longoverdue.pl

This would allow us to remove that script - have the calendar taken into account, and allow configuration at the library level by the library
Comment 2 Barton Chittenden 2018-02-06 06:42:49 UTC
I think this is a bug, not an enhancement -- this is causes confusing and seemingly inconsistent behavior of overdue_notices.pl and fines.pl (when longoverdue.pl runs before the third overdue notice, because the longoverdue delay stays the same, but notices are delayed, or when fines seemingly stop prematurely because items are marked longoverdue).
Comment 3 Katrin Fischer 2018-02-06 06:47:29 UTC
If overdue_notices and fines ignore the calendar or use it is determined by system preferences (OverdueNoticeCalendar, finescalendar). So I think we might either tie this to one of them or have another? Otherwise this might be inconsistent for people not using the calendar.
Comment 4 Barton Chittenden 2018-02-06 16:52:49 UTC
(In reply to Katrin Fischer from comment #3)
> If overdue_notices and fines ignore the calendar or use it is determined by
> system preferences (OverdueNoticeCalendar, finescalendar). So I think we
> might either tie this to one of them or have another? Otherwise this might
> be inconsistent for people not using the calendar.

You're absolutely right; I think this should follow OverdueNoticeCalendar.
Comment 5 Andrew Fuerste-Henry 2019-01-18 17:39:45 UTC
Giving this bug a bump as this continues to be a source of confusion and frustration for any library that wants their overdues to follow the calendar and their 3rd overdue to line up with their longoverduelost behavior.
Comment 6 Joy Nelson 2019-03-04 23:20:46 UTC
In order to preserve the current functionality of the long overdue cronjob not following the 'calendar/closed days', we will need a new syspref in the Long Overdue area.  

DefaultLongOverdueCalendar: Use Calendar/Ignore Calendar when working out the period for setting items to long overdue.
Comment 7 Marjorie Barry-Vila 2019-03-27 14:04:39 UTC
*** Bug 22380 has been marked as a duplicate of this bug. ***
Comment 8 David Cook 2019-05-21 02:55:00 UTC
I'm also bumping this one, as I've just been bitten by this issue. 

I'm not currently working on a fix for this, but I'd be happy to test/sign off someone else's patch.
Comment 9 David Cook 2019-05-21 02:57:17 UTC
(In reply to Joy Nelson from comment #6)
> In order to preserve the current functionality of the long overdue cronjob
> not following the 'calendar/closed days', we will need a new syspref in the
> Long Overdue area.  
> 
> DefaultLongOverdueCalendar: Use Calendar/Ignore Calendar when working out
> the period for setting items to long overdue.

That probably makes sense. We could add a warning in the "System information" in About Koha too if DefaultLongOverdueCalendar and OverdueNoticeCalendar are out of sync. 

(I've actually been thinking how it would be nice to have some validation of system preferences within the system preference editor, so that administrators could get immediate feedback about their combinations of system preferences, but haven't really thought through the idea completely yet.)
Comment 10 Andrew Fuerste-Henry 2020-01-28 17:34:22 UTC
I'm still very much in favor of making this change, but:
The patron emailer cron introduced in bug 16149 can be used to send a daily email to all patrons with items that have gone to Long Overdue in the past day, thereby sidestepping the regular overdues process and this issue.
But I imagine those libraries that prefer not to send overdues when the library is closed would still prefer not to set overdue items to lost and bill patrons on days the library is closed.
Comment 11 Andrew Fuerste-Henry 2020-02-05 20:37:43 UTC
Bug 24203 brings up the fact that as the longoverdue cron is now able to differ based on itype while overdues are not. This moves the two processes further apart. This pushes me further into the belief that we should abandon the idea of making overdue notices work for alerting patrons to long overdue charges and should instead work to accomplish that via the patron emailer cron. I just put together a plan for doing so: https://bywatersolutions.com/education/sending-notices-for-long-overdue-items
Comment 12 Marjorie Barry-Vila 2020-10-13 13:20:38 UTC
+1
Comment 13 Harold 2021-01-15 20:58:38 UTC
+1
Comment 14 Christofer Zorn 2021-04-21 17:24:28 UTC
We experienced the same issue ... could we either add an option to the one system pref similar to the "follow or ignore the calendar"? Thanks!
Comment 15 Thibaud Guillot 2022-02-09 14:18:09 UTC
Created attachment 130363 [details] [review]
Bug 18064 : Add syspref to launch longOverdue script with calendar support

The problem here is that the longOverdue script does not support calendars
and when libraries set their holidays, long overdues do not take this into account.
For example, during the covid pandemic, some libraries were closed for days, maybe months,
but longOverdue didn't care and set "lost" or other values on the items.

This fix / enhancement based on taking calendars into account to test it, regardless of your
rules related to longOverdue, I propose this test plan:

Test plan :

1) For clearer vision on what the patch does, have at least "2" libraries
2) Go to Tools/Calendar and place holidays, on each of the libraries for example, of any kind (special, repeated, as you want)
3) Manually run the misc/cronjobs/longoverdue.pl script with the -v (verbose) argument to get as much information as possible
4) You will see a common "block", with today's date + 366 days (by default if you don't pass any arguments to the script),
holidays are not taken into account in this date range
5) Apply my patch
6) Launch the updatedatabase
7) Set the new system preference "LongOverdueNoticeCalendar" to "use calendar"
8) Repeat step 3
9) You will see in output one line per library and according to the number of days off in the interval of dates,
the start date of the analysis will have been postponed by as many days in order to take these holidays into account.

If something is not completely clear, we can of course discuss it
Comment 16 Andrew Fuerste-Henry 2022-06-09 11:12:01 UTC
I'm getting this error when running the cron with  LongOverdueNoticeCalendar  set to use the calendar:

root@kohadevbox:koha(master)$ perl misc/cronjobs/longoverdue.pl -l 5=2 -v
### TEST MODE -- NO ACTIONS TAKEN ###
The method Koha::Libraries->branchcode is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Objects.pm line 572
Koha::Objects::AUTOLOAD('Koha::Libraries=HASH(0x563384af1b98)') called at misc/cronjobs/longoverdue.pl line 333
main::get_number_of_holidays('2021-06-08', '2022-06-04') called at misc/cronjobs/longoverdue.pl line 424
Comment 17 Nick Clemens 2022-06-09 14:26:59 UTC
Created attachment 135888 [details] [review]
Bug 18064: (follow-up) Get libraries as a list
Comment 18 Michal Urban 2022-06-16 22:24:17 UTC
Created attachment 136213 [details] [review]
Bug 18064 : Add syspref to launch longOverdue script with calendar support

The problem here is that the longOverdue script does not support calendars
and when libraries set their holidays, long overdues do not take this into account.
For example, during the covid pandemic, some libraries were closed for days, maybe months,
but longOverdue didn't care and set "lost" or other values on the items.

This fix / enhancement based on taking calendars into account to test it, regardless of your
rules related to longOverdue, I propose this test plan:

Test plan :

1) For clearer vision on what the patch does, have at least "2" libraries
2) Go to Tools/Calendar and place holidays, on each of the libraries for example, of any kind (special, repeated, as you want)
3) Manually run the misc/cronjobs/longoverdue.pl script with the -v (verbose) argument to get as much information as possible
4) You will see a common "block", with today's date + 366 days (by default if you don't pass any arguments to the script),
holidays are not taken into account in this date range
5) Apply my patch
6) Launch the updatedatabase
7) Set the new system preference "LongOverdueNoticeCalendar" to "use calendar"
8) Repeat step 3
9) You will see in output one line per library and according to the number of days off in the interval of dates,
the start date of the analysis will have been postponed by as many days in order to take these holidays into account.

If something is not completely clear, we can of course discuss it
Comment 19 Michal Urban 2022-06-16 22:24:21 UTC
Created attachment 136214 [details] [review]
Bug 18064: (follow-up) Get libraries as a list

Output: One line per library as expected, along with the date + 366 days
for libraries without set holidays, and start dates postponed by 366 days
+ the amount of days corresponding to library holidays.

Added random holidays to 3 different libraries, of different variety,
(Holiday Repeating Yearly, Unique Holiday, etc.) and ran longoverdue.pl,
getting the same start days, then ran with patch applied and returned
above result as expected.

Test plan :

1) For clearer vision on what the patch does, have at least "2" libraries
2) Go to Tools/Calendar and place holidays, on each of the libraries
 for example, of any kind (special, repeated, as you want)
3) Manually run the misc/cronjobs/longoverdue.pl script
 with the -v (verbose) argument to get as much information as possible
4) You will see a common "block", with today's date + 366 days
 (by default if you don't pass any arguments to the script),
holidays are not taken into account in this date range
5) Apply my patch
6) Launch the updatedatabase
7) Set the new system preference "LongOverdueNoticeCalendar" to "use calendar"
8) Repeat step 3
9) You will see in output one line per library and according to the
 number of days off in the interval of dates,
the start date of the analysis will have been postponed
 by as many days in order to take these holidays into account.

Signed-off-by: Michal Urban <michalurban177@gmail.com>
Comment 20 Kyle M Hall 2022-06-17 13:32:57 UTC
Created attachment 136263 [details] [review]
Bug 18064: (QA follow-up) Clean up code
Comment 21 Kyle M Hall 2022-06-17 13:35:16 UTC
Comment on attachment 136213 [details] [review]
Bug 18064 : Add syspref to launch longOverdue script with calendar support

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

::: misc/cronjobs/longoverdue.pl
@@ +492,4 @@
>      }
>  }
>  
> +if (!( C4::Context->preference( 'LongOverdueNoticeCalendar' ) )) {

Why is this here? I don't understand the utility of suppressing the summary if using the calendar.
Comment 22 Katrin Fischer 2022-08-21 11:22:40 UTC
Hi Thibaud, can you please take a look at Kyle's question in comment#21?
Comment 23 Thibaud Guillot 2022-08-30 10:27:20 UTC
Created attachment 139978 [details] [review]
Bug 18064: (follow-up) Remove statement for summary & reorder sysprefs
Comment 24 Thibaud Guillot 2022-08-30 10:33:13 UTC
(In reply to Kyle M Hall from comment #21)
> Comment on attachment 136213 [details] [review] [review]
> Bug 18064 : Add syspref to launch longOverdue script with calendar support
> 
> Review of attachment 136213 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: misc/cronjobs/longoverdue.pl
> @@ +492,4 @@
> >      }
> >  }
> >  
> > +if (!( C4::Context->preference( 'LongOverdueNoticeCalendar' ) )) {
> 
> Why is this here? I don't understand the utility of suppressing the summary
> if using the calendar.

Yes you're right. I worked on this script again recently and realized that I had misunderstood this part. We need this output to get the summary, it's my mistake.
Comment 25 Thibaud Guillot 2022-08-30 10:35:21 UTC
If you are interested in this script, I have also opened a BZ (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31487) which can be useful if someone can give me their opinion.
Comment 26 Lucas Gass 2023-01-30 23:08:11 UTC
It appears like this one no longer applies in current master. I think the fix is an easy one though in installer/data/mysql/mandatory/sysprefs.sql.
Comment 27 Thibaud Guillot 2023-02-01 08:21:39 UTC
Created attachment 145920 [details] [review]
Bug 18064: (follow up) rebase on master and reorder syspref
Comment 28 Thibaud Guillot 2023-02-01 08:22:24 UTC
(In reply to Lucas Gass from comment #26)
> It appears like this one no longer applies in current master. I think the
> fix is an easy one though in installer/data/mysql/mandatory/sysprefs.sql.

Thanks for your feedback, patch actually rebased on fresh master :)
Comment 29 Thibaud Guillot 2023-02-28 15:29:08 UTC
Created attachment 147496 [details] [review]
Bug 18064: (follow-up) Fix var type 'holidays_by_branch'
Comment 30 Thibaud Guillot 2023-02-28 15:30:11 UTC
Created attachment 147497 [details] [review]
Bug 18064: (follow-up) Fix var type 'holidays_by_branch'
Comment 31 Thibaud Guillot 2023-02-28 15:31:07 UTC
Actually rebased this patch on master. Try to go forward with this :)
Comment 32 Lucas Gass 2023-03-14 14:10:59 UTC
Needs rebased again due to changes in sysprefs.sql
Comment 33 Katrin Fischer 2023-09-10 12:16:33 UTC
I fixed the first conflict in sysprefs.sql, but then sadly got stuck a bit later again:

git bz apply --continue
Applying: Bug 18064 : Add syspref to launch longOverdue script with calendar support
Applying: Bug 18064: (follow-up) Get libraries as a list
Applying: Bug 18064: (QA follow-up) Clean up code
Applying: Bug 18064: (follow-up) Remove statement for summary & reorder sysprefs
error: sha1 information is lacking or useless (installer/data/mysql/mandatory/sysprefs.sql).
error: could not build fake ancestor
Patch failed at 0001 Bug 18064: (follow-up) Remove statement for summary & reorder sysprefs
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-18064-follow-up-Remove-statement-for-summary---jkamgi1_.patch
Comment 34 Suzanne 2023-09-19 22:36:37 UTC
I would love to see this get resolved! It is very tricky to be using two different calendars when trying to set up overdue notices and lost items. Thanks for working on it!
Comment 35 Nick Clemens 2023-11-17 14:41:40 UTC
Created attachment 159088 [details] [review]
Bug 18064: Add syspref to launch longOverdue script with calendar support

The problem here is that the longOverdue script does not support calendars
and when libraries set their holidays, long overdues do not take this into account.
For example, during the covid pandemic, some libraries were closed for days, maybe months,
but longOverdue didn't care and set "lost" or other values on the items.

This fix / enhancement based on taking calendars into account to test it, regardless of your
rules related to longOverdue, I propose this test plan:

Test plan :

1) For clearer vision on what the patch does, have at least "2" libraries
2) Go to Tools/Calendar and place holidays, on each of the libraries for example, of any kind (special, repeated, as you want)
3) Manually run the misc/cronjobs/longoverdue.pl script with the -v (verbose) argument to get as much information as possible
4) You will see a common "block", with today's date + 366 days (by default if you don't pass any arguments to the script),
holidays are not taken into account in this date range
5) Apply my patch
6) Launch the updatedatabase
7) Set the new system preference "LongOverdueNoticeCalendar" to "use calendar"
8) Repeat step 3
9) You will see in output one line per library and according to the number of days off in the interval of dates,
the start date of the analysis will have been postponed by as many days in order to take these holidays into account.

If something is not completely clear, we can of course discuss it
Comment 36 Nick Clemens 2023-11-17 14:43:53 UTC
Rebased and squashed.

As far as I can tell, this correctly fetches the calendar per branch, however, the items queries do not take branches into account - so we run a query per branch with different date ranges, but don't limit the items by branch.

I think we also need a switch here to control how we will decide which branch to apply - is it the patron's branch? the item's home/holding branch? I think this could follow CircControl - but needs some logic

Moving to Failed QA.
Comment 37 Andrew Fuerste-Henry 2023-11-17 15:14:44 UTC
(In reply to Nick Clemens from comment #36)
> Rebased and squashed.
> 
> As far as I can tell, this correctly fetches the calendar per branch,
> however, the items queries do not take branches into account - so we run a
> query per branch with different date ranges, but don't limit the items by
> branch.
> 
> I think we also need a switch here to control how we will decide which
> branch to apply - is it the patron's branch? the item's home/holding branch?
> I think this could follow CircControl - but needs some logic
> 
> Moving to Failed QA.

That's a great point, though I'm not at all happy about it. Longoverdue does not currently have any concept of branch-specificity (see BZ 9596). There is no branch-agnostic calendar. This one can't move forward without BZ 9596.