Bug 27249 - Using the calendar to 'close' a library can create an infinite loop during renewals
Summary: Using the calendar to 'close' a library can create an infinite loop during re...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P2 major (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-16 11:24 UTC by Nick Clemens (kidclamp)
Modified: 2023-11-13 13:45 UTC (History)
9 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:
23.11.00,23.05.05,22.11.12


Attachments
Bug 27249: Prevent infinite loop when searching for an open day (8.41 KB, patch)
2023-07-18 10:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27249: Prevent infinite loop when searching for an open day (8.45 KB, patch)
2023-07-19 17:24 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 27249: Catch NoOpenDays exception while checking out an item (3.39 KB, patch)
2023-08-24 07:28 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27249: Prevent infinite loop when searching for an open day (8.50 KB, patch)
2023-08-29 20:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 27249: Catch NoOpenDays exception while checking out an item (3.44 KB, patch)
2023-08-29 20:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 27249: Fix code style (3.53 KB, patch)
2023-10-10 11:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27249: Add missing POD for Koha::Exceptions::Calendar (857 bytes, patch)
2023-10-10 11:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27249: Replace DateTime->now by dt_from_string (1.42 KB, patch)
2023-10-10 11:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27249: Prevent infinite loop when searching for an open day (8.55 KB, patch)
2023-10-14 18:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27249: Catch NoOpenDays exception while checking out an item (3.50 KB, patch)
2023-10-14 18:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27249: Fix code style (3.59 KB, patch)
2023-10-14 18:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27249: Add missing POD for Koha::Exceptions::Calendar (914 bytes, patch)
2023-10-14 18:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 27249: Replace DateTime->now by dt_from_string (1.47 KB, patch)
2023-10-14 18:57 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 (kidclamp) 2020-12-16 11:24:51 UTC
When calculating due date using holidays we don't account for the case where everyday is a holiday until the end of time

To recreate:
1 - Set  OverdueNoticeCalendar  to 'Use calendar' and useDaysMode to 'Use the calendar to push'
2 - Browse to Tools->Calendar
3 - Set every day of the week to "Holiday repeated every same day of the week"
4 - Issue an item to a patron
5 - Check the box and select 'Renew selected items'
6 - Watch the spinner spin...and spin...and spin
Comment 1 Mathieu Saby 2023-07-06 07:43:34 UTC
Still valid

Our koha was **completely** unavailable this week for several hours due to this bug. The level of severity of this bug must be raised!
Comment 2 Mathieu Saby 2023-07-09 17:08:01 UTC
If the library is never open in the future, and usedaymode uses calendar to define the new due date, the correct behavior should be that Koha does not change the original due date.
Comment 3 Julian Maurice 2023-07-18 10:18:23 UTC
Created attachment 153585 [details] [review]
Bug 27249: Prevent infinite loop when searching for an open day

Calendars can be configured in a way that all days are closed.
The simplest way to do that is to configure a repeatable holiday for
every day of the week.
With such calendars, searching for an open day will literally take
forever.

This patch sets a hard limit on how many iterations are allowed before
giving up. This limit is set to the arbitrary value of 5000, which
should be large enough to be able to consider there is no open days if
we haven't found any with that many iterations, and small enough to
allow the loop to end quickly

Test plan:
1. Set system preference 'useDaysMode' to 'Use the calendar to push the
   due date to the next open day' ('Datedue'). Make sure the existing
   circulation rules do not conflict with that setting.
2. Browse to Tools » Calendar
3. Set every day of the week to "Holiday repeated every same day of the
   week"
4. Issue an item to a patron
5. Check the box and select 'Renew selected items'
6. The renewal should fail pretty quickly
Comment 4 Julian Maurice 2023-07-18 10:23:21 UTC
For testers: Since bug 28653, the checkouts table is automatically refreshed after a renew/checkin, so you won't be able to see the error message (and from what I have seen, the error handling code is buggy anyway). I will open a new bug report for that.
Comment 5 Julian Maurice 2023-07-18 10:35:25 UTC
(In reply to Julian Maurice from comment #4)
> I will open a new bug report for that.

Bug 34302
Comment 6 Sam Lau 2023-07-19 17:24:43 UTC
Created attachment 153682 [details] [review]
Bug 27249: Prevent infinite loop when searching for an open day

Calendars can be configured in a way that all days are closed.
The simplest way to do that is to configure a repeatable holiday for
every day of the week.
With such calendars, searching for an open day will literally take
forever.

This patch sets a hard limit on how many iterations are allowed before
giving up. This limit is set to the arbitrary value of 5000, which
should be large enough to be able to consider there is no open days if
we haven't found any with that many iterations, and small enough to
allow the loop to end quickly

Test plan:
1. Set system preference 'useDaysMode' to 'Use the calendar to push the
   due date to the next open day' ('Datedue'). Make sure the existing
   circulation rules do not conflict with that setting.
2. Browse to Tools » Calendar
3. Set every day of the week to "Holiday repeated every same day of the
   week"
4. Issue an item to a patron
5. Check the box and select 'Renew selected items'
6. The renewal should fail pretty quickly

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 7 Jonathan Druart 2023-07-21 12:54:57 UTC
I don't understand, what about the issue?

I am getting a 500 at step 4:

Exception 'Koha::Exceptions::Calendar::NoOpenDays' thrown 'Unable to find an open day for library CPL'
 at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 8 Julian Maurice 2023-07-21 13:08:23 UTC
Maybe there are other sysprefs/circ rules that make Koha the calendar checks different when checking out or renewing an item...
Without the patch, do you get the infinite loop when checking out the item ?
Comment 9 Jonathan Druart 2023-07-21 13:53:57 UTC
(In reply to Julian Maurice from comment #8)
> Maybe there are other sysprefs/circ rules that make Koha the calendar checks
> different when checking out or renewing an item...

I am using the default values.

> Without the patch, do you get the infinite loop when checking out the item ?

Yes.
Comment 10 Julian Maurice 2023-08-24 07:28:22 UTC
Created attachment 154728 [details] [review]
Bug 27249: Catch NoOpenDays exception while checking out an item
Comment 11 Julian Maurice 2023-08-24 07:29:02 UTC
The last patch fixes the issue mentioned in comment 7
Comment 12 David Nind 2023-08-29 20:29:05 UTC
Created attachment 154920 [details] [review]
Bug 27249: Prevent infinite loop when searching for an open day

Calendars can be configured in a way that all days are closed.
The simplest way to do that is to configure a repeatable holiday for
every day of the week.
With such calendars, searching for an open day will literally take
forever.

This patch sets a hard limit on how many iterations are allowed before
giving up. This limit is set to the arbitrary value of 5000, which
should be large enough to be able to consider there is no open days if
we haven't found any with that many iterations, and small enough to
allow the loop to end quickly

Test plan:
1. Set system preference 'useDaysMode' to 'Use the calendar to push the
   due date to the next open day' ('Datedue'). Make sure the existing
   circulation rules do not conflict with that setting.
2. Browse to Tools » Calendar
3. Set every day of the week to "Holiday repeated every same day of the
   week"
4. Issue an item to a patron
5. Check the box and select 'Renew selected items'
6. The renewal should fail pretty quickly

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2023-08-29 20:29:08 UTC
Created attachment 154921 [details] [review]
Bug 27249: Catch NoOpenDays exception while checking out an item

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2023-08-29 21:13:58 UTC
Testing notes (using KTD):

1. I'm not sure if I tested this correctly, but this is what I did:
   . Set 'useDaysMode' to 'Use the calendar to push the due date to the 
     next open day'
   . In the calendar, for Monday to Sunday, chose 'Holiday repeated every same day of the week' and ticked 'Copy to all libraries'
   . Before the patch: Can't issue anything - does nothing (stays sitting on the page with the browser loading button/indicator in the tab going round)
   . After patch: message is 'No open days found while calculating the due date. Check the library calendar.'
   . Removed holiday for Saturday and Sunday - can then issue something.
   . For the renewing items (Patron record > Details > tick renew box then 'Renew or check in selected items') - screen refreshes, no renewal, don't see any messages or errors.

2. I've signed off anyway, as this definitely fixes the issue of it sitting on the page doing nothing.
Comment 15 Marcel de Rooy 2023-09-01 08:03:53 UTC
 FAIL   Koha/Exceptions/Calendar.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 0, now: 1)

 WARN   circ/circulation.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 244, now: 253)
   OK     valid

 FAIL   t/db_dependent/Holidays.t
   FAIL   forbidden patterns
                forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 292)
                forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 318)
Comment 16 Julian Maurice 2023-10-10 11:07:31 UTC
Created attachment 156786 [details] [review]
Bug 27249: Fix code style
Comment 17 Julian Maurice 2023-10-10 11:07:34 UTC
Created attachment 156787 [details] [review]
Bug 27249: Add missing POD for Koha::Exceptions::Calendar
Comment 18 Julian Maurice 2023-10-10 11:07:38 UTC
Created attachment 156788 [details] [review]
Bug 27249: Replace DateTime->now by dt_from_string
Comment 19 Katrin Fischer 2023-10-14 18:57:17 UTC
Created attachment 157163 [details] [review]
Bug 27249: Prevent infinite loop when searching for an open day

Calendars can be configured in a way that all days are closed.
The simplest way to do that is to configure a repeatable holiday for
every day of the week.
With such calendars, searching for an open day will literally take
forever.

This patch sets a hard limit on how many iterations are allowed before
giving up. This limit is set to the arbitrary value of 5000, which
should be large enough to be able to consider there is no open days if
we haven't found any with that many iterations, and small enough to
allow the loop to end quickly

Test plan:
1. Set system preference 'useDaysMode' to 'Use the calendar to push the
   due date to the next open day' ('Datedue'). Make sure the existing
   circulation rules do not conflict with that setting.
2. Browse to Tools » Calendar
3. Set every day of the week to "Holiday repeated every same day of the
   week"
4. Issue an item to a patron
5. Check the box and select 'Renew selected items'
6. The renewal should fail pretty quickly

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2023-10-14 18:57:21 UTC
Created attachment 157164 [details] [review]
Bug 27249: Catch NoOpenDays exception while checking out an item

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2023-10-14 18:57:24 UTC
Created attachment 157165 [details] [review]
Bug 27249: Fix code style

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2023-10-14 18:57:27 UTC
Created attachment 157166 [details] [review]
Bug 27249: Add missing POD for Koha::Exceptions::Calendar

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2023-10-14 18:57:31 UTC
Created attachment 157167 [details] [review]
Bug 27249: Replace DateTime->now by dt_from_string

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Tomás Cohen Arazi 2023-10-17 17:47:47 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 25 Fridolin Somers 2023-10-25 06:28:03 UTC
Pushed to 23.05.x for 23.05.05
Comment 26 Matt Blenkinsop 2023-11-13 13:45:55 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x