Bug 12063 - Fix QA failures

  - 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 <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/Letters.pm b/C4/Letters.pm
index de7e04a..419f272 100644 (file)
--- 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} =~ /<<today>>/) {

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 9851181..e1e355a 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (executable)
--- 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 (file)
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 (file)
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 <http://www.gnu.org/licenses>.
+
+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 (file)
--- 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({