|
Description
Nick Clemens (kidclamp)
2020-12-16 11:24:51 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! 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. 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 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. (In reply to Julian Maurice from comment #4) > I will open a new bug report for that. Bug 34302 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> 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 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 ? (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. Created attachment 154728 [details] [review] Bug 27249: Catch NoOpenDays exception while checking out an item The last patch fixes the issue mentioned in comment 7 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> Created attachment 154921 [details] [review] Bug 27249: Catch NoOpenDays exception while checking out an item Signed-off-by: David Nind <david@davidnind.com> 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.
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)
Created attachment 156786 [details] [review] Bug 27249: Fix code style Created attachment 156787 [details] [review] Bug 27249: Add missing POD for Koha::Exceptions::Calendar Created attachment 156788 [details] [review] Bug 27249: Replace DateTime->now by dt_from_string 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> 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> Created attachment 157165 [details] [review] Bug 27249: Fix code style Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 157166 [details] [review] Bug 27249: Add missing POD for Koha::Exceptions::Calendar Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 157167 [details] [review] Bug 27249: Replace DateTime->now by dt_from_string Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 23.11. Nice work everyone, thanks! Pushed to 23.05.x for 23.05.05 Nice work everyone! Pushed to oldstable for 22.11.x |