From 2b58f4d89ca2d71c8979e4d87759fc45d63795f4 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Tue, 18 Jul 2023 10:26:03 +0200 Subject: [PATCH] Bug 27249: Prevent infinite loop when searching for an open day MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: David Nind Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- Koha/Calendar.pm | 28 ++++++++- Koha/Exceptions/Calendar.pm | 17 ++++++ koha-tmpl/intranet-tmpl/prog/js/checkouts.js | 2 + svc/renew | 2 + t/db_dependent/Holidays.t | 61 +++++++++++++++++++- 5 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 Koha/Exceptions/Calendar.pm diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index 5ffebb47ab..645b538700 100644 --- a/Koha/Calendar.pm +++ b/Koha/Calendar.pm @@ -8,6 +8,15 @@ use DateTime::Duration; use C4::Context; use Koha::Caches; use Koha::Exceptions; +use Koha::Exceptions::Calendar; + +# This limit avoids an infinite loop when searching for an open day in an +# always closed library +# The value is arbitrary, but it 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 +# See next_open_days and prev_open_days +use constant OPEN_DAYS_SEARCH_MAX_ITERATIONS => 5000; sub new { my ( $classname, %options ) = @_; @@ -261,10 +270,18 @@ sub next_open_days { my $base_date = $dt->clone(); $base_date->add(days => $to_add); - while ($self->is_holiday($base_date)) { + my $i = 0; + while ( $self->is_holiday($base_date) && $i < OPEN_DAYS_SEARCH_MAX_ITERATIONS ) { my $add_next = $self->get_push_amt($base_date); $base_date->add(days => $add_next); + ++$i; + } + + if ( $self->is_holiday($base_date) ) { + Koha::Exceptions::Calendar::NoOpenDays->throw( + sprintf( 'Unable to find an open day for library %s', $self->{branchcode} ) ); } + return $base_date; } @@ -282,11 +299,18 @@ sub prev_open_days { $base_date->add(days => $to_sub); - while ($self->is_holiday($base_date)) { + my $i = 0; + while ( $self->is_holiday($base_date) && $i < OPEN_DAYS_SEARCH_MAX_ITERATIONS ) { my $sub_next = $self->get_push_amt($base_date); # Ensure we're subtracting when we need to be $sub_next = $sub_next > 0 ? 0 - $sub_next : $sub_next; $base_date->add(days => $sub_next); + ++$i; + } + + if ( $self->is_holiday($base_date) ) { + Koha::Exceptions::Calendar::NoOpenDays->throw( + sprintf( 'Unable to find an open day for library %s', $self->{branchcode} ) ); } return $base_date; diff --git a/Koha/Exceptions/Calendar.pm b/Koha/Exceptions/Calendar.pm new file mode 100644 index 0000000000..3891c3bced --- /dev/null +++ b/Koha/Exceptions/Calendar.pm @@ -0,0 +1,17 @@ +package Koha::Exceptions::Calendar; + +use Modern::Perl; + +use Koha::Exception; + +use Exception::Class ( + 'Koha::Exceptions::Calendar' => { + isa => 'Koha::Exception', + }, + 'Koha::Exceptions::Calendar::NoOpenDays' => { + isa => 'Koha::Exceptions::Calendar', + description => 'Library has no open days', + }, +); + +1; diff --git a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js index e750767310..fb72b8b80a 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js +++ b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js @@ -203,6 +203,8 @@ $(document).ready(function() { content += __("Not allowed: patron restricted"); } else if ( data.error == "overdue" ) { content += __("Not allowed: overdue"); + } else if ( data.error == 'no_open_days' ) { + content += __('Unable to find an open day'); } else if ( data.error ) { content += data.error; } else { diff --git a/svc/renew b/svc/renew index ce026cfc46..eca18e647f 100755 --- a/svc/renew +++ b/svc/renew @@ -87,6 +87,8 @@ if ( $data->{renew_okay} || ( $seen && $data->{error} eq 'too_unseen') ) { } catch { if ( ref($_) eq 'Koha::Exceptions::Checkout::FailedRenewal' ) { $data->{error} = 'renewal_failed'; + } elsif ( ref($_) eq 'Koha::Exceptions::Calendar::NoOpenDays' ) { + $data->{error} = 'no_open_days'; } else { $_->rethrow; } diff --git a/t/db_dependent/Holidays.t b/t/db_dependent/Holidays.t index 607906a169..42c2ce604e 100755 --- a/t/db_dependent/Holidays.t +++ b/t/db_dependent/Holidays.t @@ -17,7 +17,8 @@ use Modern::Perl; -use Test::More tests => 12; +use Test::More tests => 14; +use Test::Exception; use DateTime; use DateTime::TimeZone; @@ -277,5 +278,63 @@ subtest 'copy_to_branch' => sub { }; +subtest 'with a library that is never open' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; + my $calendar = C4::Calendar->new( branchcode => $branchcode ); + foreach my $weekday ( 0 .. 6 ) { + $calendar->insert_week_day_holiday( weekday => $weekday, title => '', description => '' ); + } + + my $now = DateTime->now; + + subtest 'next_open_days should throw an exception' => sub { + my $kcalendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' ); + throws_ok { $kcalendar->next_open_days( $now, 1 ) } 'Koha::Exceptions::Calendar::NoOpenDays'; + }; + + subtest 'prev_open_days should throw an exception' => sub { + my $kcalendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' ); + throws_ok { $kcalendar->prev_open_days( $now, 1 ) } 'Koha::Exceptions::Calendar::NoOpenDays'; + }; + + $schema->storage->txn_rollback; +}; + +subtest 'with a library that is *almost* never open' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $branchcode = $builder->build( { source => 'Branch' } )->{branchcode}; + my $calendar = C4::Calendar->new( branchcode => $branchcode ); + foreach my $weekday ( 0 .. 6 ) { + $calendar->insert_week_day_holiday( weekday => $weekday, title => '', description => '' ); + } + + my $now = DateTime->now; + my $open_day_in_the_future = $now->clone()->add( years => 1 ); + my $open_day_in_the_past = $now->clone()->subtract( years => 1 ); + $calendar->insert_exception_holiday( date => $open_day_in_the_future->ymd, title => '', description => '' ); + $calendar->insert_exception_holiday( date => $open_day_in_the_past->ymd, title => '', description => '' ); + + subtest 'next_open_days should find the open day' => sub { + my $kcalendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' ); + my $next_open_day = $kcalendar->next_open_days( $now, 1 ); + is( $next_open_day->ymd, $open_day_in_the_future->ymd ); + }; + + subtest 'prev_open_days should find the open day' => sub { + my $kcalendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Calendar' ); + my $prev_open_day = $kcalendar->prev_open_days( $now, 1 ); + is( $prev_open_day->ymd, $open_day_in_the_past->ymd ); + }; + + $schema->storage->txn_rollback; +}; + # Clear cache Koha::Caches->get_instance->flush_all; -- 2.39.5