From 8d3eba6a297f4fd993ddf237b6cd66cfda38ff95 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 23 Apr 2018 15:23:00 -0300 Subject: [PATCH] Bug 19204: Make the debarment date calculation depends on finesCalendar This patchs adds the ability to calculate the end of the suspension date (debarment date) using the finesCalendar syspref. Prior to this patch it was never calculating without taking into account the calendar. calculated without taking holidays into account. This was a problem because the restriction could end in the middle of a period the library is closed. Test plan: - Set finescalendar to 'not including days the library is closed' - Set a circulation condition with no fine/maxfine, but fine days and max fine days instead - Check out an item with a due date in the past - Check the item in and verify the restriction date - Clean the restriction - Add holidays to your calendar on the calculated restriction date - Check the item out again with the same due date in the past - Check in the item again - Verify the calculated restriction end date has changed, it's set to the day after the holiday. Fines in days restriction calculation is correctly taking calendar into account. Sponsored-by: Goethe-Institut Signed-off-by: Claire Gravely Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart --- C4/Circulation.pm | 14 ++++++- Koha/Calendar.pm | 2 +- t/db_dependent/Circulation.t | 77 +++++++++++++++++++++++++++++++++++- 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index c6dc6afacd..2deb3678a1 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2246,8 +2246,18 @@ sub _debar_user_on_return { ); } - my $new_debar_dt = - $return_date->clone()->add_duration( $suspension_days ); + my $new_debar_dt; + # Use the calendar or not to calculate the debarment date + if ( C4::Context->preference('finesCalendar') eq 'noFinesWhenClosed' ) { + my $calendar = Koha::Calendar->new( + branchcode => $branchcode, + days_mode => 'Calendar' + ); + $new_debar_dt = $calendar->addDate( $return_date, $suspension_days ); + } + else { + $new_debar_dt = $return_date->clone()->add_duration($suspension_days); + } Koha::Patron::Debarments::AddUniqueDebarment({ borrowernumber => $borrower->{borrowernumber}, diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index c66ab45092..fc63572b28 100644 --- a/Koha/Calendar.pm +++ b/Koha/Calendar.pm @@ -47,7 +47,7 @@ sub _init { 1; } - $self->{days_mode} = C4::Context->preference('useDaysMode'); + $self->{days_mode} ||= C4::Context->preference('useDaysMode'); $self->{test} = 0; return; } diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 468fdfac37..b7f9d4fba9 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -24,6 +24,7 @@ use POSIX qw( floor ); use t::lib::Mocks; use t::lib::TestBuilder; +use C4::Calendar; use C4::Circulation; use C4::Biblio; use C4::Items; @@ -48,6 +49,11 @@ my $dbh = C4::Context->dbh; # Start transaction $dbh->{RaiseError} = 1; +my $cache = Koha::Caches->get_instance(); +$dbh->do(q|DELETE FROM special_holidays|); +$dbh->do(q|DELETE FROM repeatable_holidays|); +$cache->clear_from_cache('single_holidays'); + # Start with a clean slate $dbh->do('DELETE FROM issues'); $dbh->do('DELETE FROM borrowers'); @@ -1758,7 +1764,7 @@ subtest 'AddReturn + CumulativeRestrictionPeriods' => sub { }; subtest 'AddReturn + suspension_chargeperiod' => sub { - plan tests => 6; + plan tests => 8; my $library = $builder->build( { source => 'Branch' } ); my $patron = $builder->build( { source => 'Borrower', value => { categorycode => $patron_category->{categorycode} } } ); @@ -1863,9 +1869,72 @@ subtest 'AddReturn + suspension_chargeperiod' => sub { Koha::Patron::Debarments::DelUniqueDebarment( { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); -}; + + # We want to charge 2 days every days, with 0 day of grace (to not burn brains) + $rule->finedays(2)->store; + $rule->suspension_chargeperiod(1)->store; + $rule->firstremind(0)->store; + t::lib::Mocks::mock_preference('finesCalendar', 'noFinesWhenClosed'); + + # Adding a holiday 2 days ago + my $calendar = C4::Calendar->new(branchcode => $library->{branchcode}); + my $two_days_ago = dt_from_string->subtract( days => 2 ); + $calendar->insert_single_holiday( + day => $two_days_ago->day, + month => $two_days_ago->month, + year => $two_days_ago->year, + title => 'holidayTest-2d', + description => 'holidayDesc 2 days ago' + ); + + # With 5 days of overdue, only 4 (x finedays=2) days must charged (one was an holiday) + AddIssue( $patron, $item_1->{barcode}, $five_days_ago ); # Add an overdue + + AddReturn( $item_1->{barcode}, $library->{branchcode}, + undef, undef, dt_from_string ); + $debarments = Koha::Patron::Debarments::GetDebarments( + { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); + $expected_expiration = output_pref( + { + dt => dt_from_string->add( days => floor( ( ( 5 - 0 - 1 ) / 1 ) * 2 ) ), + dateformat => 'sql', + dateonly => 1 + } + ); + is( $debarments->[0]->{expiration}, $expected_expiration ); + Koha::Patron::Debarments::DelUniqueDebarment( + { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); + my $two_days_ahead = dt_from_string->add( days => 2 ); + $calendar->insert_single_holiday( + day => $two_days_ahead->day, + month => $two_days_ahead->month, + year => $two_days_ahead->year, + title => 'holidayTest+2d', + description => 'holidayDesc 2 days ahead' + ); + + # Same as above, but we should skip D+2 + AddIssue( $patron, $item_1->{barcode}, $five_days_ago ); # Add an overdue + + AddReturn( $item_1->{barcode}, $library->{branchcode}, + undef, undef, dt_from_string ); + $debarments = Koha::Patron::Debarments::GetDebarments( + { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); + $expected_expiration = output_pref( + { + dt => dt_from_string->add( days => floor( ( ( 5 - 0 - 1 ) / 1 ) * 2 ) + 1 ), + dateformat => 'sql', + dateonly => 1 + } + ); + is( $debarments->[0]->{expiration}, $expected_expiration ); + Koha::Patron::Debarments::DelUniqueDebarment( + { borrowernumber => $patron->{borrowernumber}, type => 'SUSPENSION' } ); + +}; + subtest 'AddReturn | is_overdue' => sub { plan tests => 5; @@ -2163,6 +2232,10 @@ subtest 'CanBookBeIssued | is_overdue' => sub { }; + +$schema->storage->txn_rollback; +$cache->clear_from_cache('single_holidays'); + sub set_userenv { my ( $library ) = @_; C4::Context->set_userenv(0,0,0,'firstname','surname', $library->{branchcode}, $library->{branchname}, '', '', ''); -- 2.39.5