From a3505da3079505848e6d729a54fad859b804338c Mon Sep 17 00:00:00 2001 From: Alex Arnaud Date: Thu, 30 Mar 2017 09:53:10 +0000 Subject: [PATCH] Bug 12063 - Fix QA failures Content-Type: text/plain; charset=utf-8 - Remove expiration date calculation in C4::Letter since it's done when setting the reserve waiting, - remove expiration date calculation in circ/waitingreserves.pl. Use the one in DB, - add a new atomic update that calculate expiration date for waiting reserves, - add tests for days_foward function and fix the infinite loop. Signed-off-by: Marcel de Rooy --- C4/Letters.pm | 10 ---- C4/Reserves.pm | 4 +- Koha/Calendar.pm | 2 + Koha/Hold.pm | 30 +++++----- circ/waitingreserves.pl | 9 +-- ...efine_expirationdate_for_waitting_reserves.perl | 30 ++++++++++ t/db_dependent/Calendar.t | 69 ++++++++++++++++++++++ t/db_dependent/Holds/CancelReserves.t | 9 ++- 8 files changed, 127 insertions(+), 36 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_12063-define_expirationdate_for_waitting_reserves.perl create mode 100644 t/db_dependent/Calendar.t diff --git a/C4/Letters.pm b/C4/Letters.pm index de7e04a..419f272 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -871,17 +871,7 @@ sub _parseletter { } if ( $table eq 'reserves' && $values->{'waitingdate'} ) { - my @waitingdate = split /-/, $values->{'waitingdate'}; - - $values->{'expirationdate'} = ''; - if ( C4::Context->preference('ReservesMaxPickUpDelay') ) { - my $dt = dt_from_string(); - $dt->add( days => C4::Context->preference('ReservesMaxPickUpDelay') ); - $values->{'expirationdate'} = output_pref( { dt => $dt, dateonly => 1 } ); - } - $values->{'waitingdate'} = output_pref({ dt => dt_from_string( $values->{'waitingdate'} ), dateonly => 1 }); - } if ($letter->{content} && $letter->{content} =~ /<>/) { diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 9851181..e1e355a 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -680,7 +680,7 @@ sub GetReservesForBranch { my $dbh = C4::Context->dbh; my $query = " - SELECT reserve_id,borrowernumber,reservedate,itemnumber,waitingdate + SELECT reserve_id,borrowernumber,reservedate,itemnumber,waitingdate, expirationdate FROM reserves WHERE priority='0' AND found='W' @@ -899,6 +899,8 @@ Cancels all reserves with an expiration date from before today. =cut sub CancelExpiredReserves { + return unless C4::Context->preference("ExpireReservesMaxPickUpDelay"); + my $today = dt_from_string(); my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index 3c04fbe..67d2a6b 100644 --- a/Koha/Calendar.pm +++ b/Koha/Calendar.pm @@ -301,6 +301,8 @@ sub days_forward { my $start_dt = shift; my $num_days = shift; + return $start_dt unless $num_days > 0; + my $base_dt = $start_dt->clone(); while ($num_days--) { diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 1e2e730..030e90a 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -132,24 +132,22 @@ sub set_waiting { $requested_expiration = dt_from_string($self->expirationdate); } - if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) { - my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); - my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); - my $calendar = Koha::Calendar->new( branchcode => $self->branchcode ); - - my $expirationdate = $today->clone; - $expirationdate->add(days => $max_pickup_delay); - - if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) { - $expirationdate = $calendar->days_forward( dt_from_string($self->waitingdate), $max_pickup_delay ); - } - - # If patron's requested expiration date is prior to the - # calculated one, we keep the patron's one. - my $cmp = $requested_expiration ? DateTime->compare($requested_expiration, $expirationdate) : 0; - $values->{expirationdate} = $cmp == -1 ? $requested_expiration->ymd : $expirationdate->ymd; + my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); + my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); + my $calendar = Koha::Calendar->new( branchcode => $self->branchcode ); + + my $expirationdate = $today->clone; + $expirationdate->add(days => $max_pickup_delay); + + if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) { + $expirationdate = $calendar->days_forward( dt_from_string($self->waitingdate), $max_pickup_delay ); } + # If patron's requested expiration date is prior to the + # calculated one, we keep the patron's one. + my $cmp = $requested_expiration ? DateTime->compare($requested_expiration, $expirationdate) : 0; + $values->{expirationdate} = $cmp == -1 ? $requested_expiration->ymd : $expirationdate->ymd; + $self->set($values)->store(); return $self; diff --git a/circ/waitingreserves.pl b/circ/waitingreserves.pl index d0b3942..af41394 100755 --- a/circ/waitingreserves.pl +++ b/circ/waitingreserves.pl @@ -86,7 +86,6 @@ my @getreserves = $all_branches ? GetReservesForBranch() : GetReservesForBranch( my $today = Date_to_Days(&Today); my $max_pickup_delay = C4::Context->preference('ReservesMaxPickUpDelay'); -$max_pickup_delay-- if C4::Context->preference('ExpireReservesMaxPickUpDelay'); foreach my $num (@getreserves) { next unless ($num->{'waitingdate'} && $num->{'waitingdate'} ne '0000-00-00'); @@ -107,12 +106,8 @@ foreach my $num (@getreserves) { my $getborrower = GetMember(borrowernumber => $num->{'borrowernumber'}); my $itemtypeinfo = getitemtypeinfo( $gettitle->{'itemtype'} ); # using the fixed up itype/itemtype $getreserv{'waitingdate'} = $num->{'waitingdate'}; - my ( $waiting_year, $waiting_month, $waiting_day ) = split (/-/, $num->{'waitingdate'}); - - ( $waiting_year, $waiting_month, $waiting_day ) = - Add_Delta_Days( $waiting_year, $waiting_month, $waiting_day, - $max_pickup_delay); - my $calcDate = Date_to_Days( $waiting_year, $waiting_month, $waiting_day ); + my ( $expire_year, $expire_month, $expire_day ) = split (/-/, $num->{'expirationdate'}); + my $calcDate = Date_to_Days( $expire_year, $expire_month, $expire_day ); $getreserv{'itemtype'} = $itemtypeinfo->{'description'}; $getreserv{'title'} = $gettitle->{'title'}; diff --git a/installer/data/mysql/atomicupdate/bug_12063-define_expirationdate_for_waitting_reserves.perl b/installer/data/mysql/atomicupdate/bug_12063-define_expirationdate_for_waitting_reserves.perl new file mode 100644 index 0000000..3a5d232 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_12063-define_expirationdate_for_waitting_reserves.perl @@ -0,0 +1,30 @@ +use C4::Context; + +use Koha::Holds; +use Koha::DateUtils; +use Koha::Calendar; + +my $waiting_holds = Koha::Holds->search({ found => 'W', priority => 0 }); +while ( my $hold = $waiting_holds->next ) { + + my $requested_expiration; + if ($hold->expirationdate) { + $requested_expiration = dt_from_string($hold->expirationdate); + } + + if ( my $waitingdate = dt_from_string($hold->waitingdate) ) { + my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); + my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); + my $calendar = Koha::Calendar->new( branchcode => $hold->branchcode ); + + my $expirationdate = $waitingdate->clone; + $expirationdate->add(days => $max_pickup_delay); + + if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) { + $expirationdate = $calendar->days_forward( dt_from_string($hold->waitingdate), $max_pickup_delay ); + } + + my $cmp = $requested_expiration ? DateTime->compare($requested_expiration, $expirationdate) : 0; + $hold->expirationdate($cmp == -1 ? $requested_expiration->ymd : $expirationdate->ymd)->store; + } +} \ No newline at end of file diff --git a/t/db_dependent/Calendar.t b/t/db_dependent/Calendar.t new file mode 100644 index 0000000..d443f11 --- /dev/null +++ b/t/db_dependent/Calendar.t @@ -0,0 +1,69 @@ +#!/usr/bin/perl + +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 5; +use t::lib::TestBuilder; + +use DateTime; +use Koha::Caches; +use Koha::DateUtils; + +use_ok('Koha::Calendar'); + +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; + +my $today = dt_from_string(); +my $holiday_dt = $today->clone; +$holiday_dt->add(days => 15); + +Koha::Caches->get_instance()->flush_all(); + +my $builder = t::lib::TestBuilder->new(); +my $holiday = $builder->build({ + source => 'SpecialHoliday', + value => { + branchcode => 'LIB1', + day => $holiday_dt->day, + month => $holiday_dt->month, + year => $holiday_dt->year, + title => 'My holiday', + isexception => 0 + }, +}); + +my $calendar = Koha::Calendar->new( branchcode => 'LIB1'); +my $forwarded_dt = $calendar->days_forward($today, 10); + +my $expected = $today->clone; +$expected->add(days => 10); +is($forwarded_dt->ymd, $expected->ymd, 'With no holiday on the perioddays_forward should add 10 days'); + +$forwarded_dt = $calendar->days_forward($today, 20); + +$expected->add(days => 11); +is($forwarded_dt->ymd, $expected->ymd, 'With holiday on the perioddays_forward should add 20 days + 1 day for holiday'); + +$forwarded_dt = $calendar->days_forward($today, 0); +is($forwarded_dt->ymd, $today->ymd, '0 day should return start dt'); + +$forwarded_dt = $calendar->days_forward($today, -2); +is($forwarded_dt->ymd, $today->ymd, 'negative day should return start dt'); + +$schema->storage->txn_rollback(); diff --git a/t/db_dependent/Holds/CancelReserves.t b/t/db_dependent/Holds/CancelReserves.t index b6d216f..3330a1c 100644 --- a/t/db_dependent/Holds/CancelReserves.t +++ b/t/db_dependent/Holds/CancelReserves.t @@ -8,7 +8,7 @@ use Koha::DateUtils; use t::lib::Mocks; use t::lib::TestBuilder; -use Test::More tests => 5; +use Test::More tests => 6; use_ok('C4::Reserves'); @@ -63,7 +63,12 @@ my $reserve2 = $builder->build({ CancelExpiredReserves(); my $r2 = Koha::Holds->find($reserve2->{reserve_id}); -is($r2, undef,'Reserve 2 should be canceled.'); +ok($r2, 'Without ExpireReservesMaxPickUpDelay, reserve 2 should not be canceled.'); + +t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelay', 1); +CancelExpiredReserves(); +$r2 = Koha::Holds->find($reserve2->{reserve_id}); +is($r2, undef,'With ExpireReservesMaxPickUpDelay, reserve 2 should be canceled.'); # Reserve expired on holiday my $reserve3 = $builder->build({ -- 2.1.4