Bug 12063: Change date calculation for reserve expiration to skip all holiday

This patch makes koha automatically set expiration date when reserves become
waitting. Also it adds a new syspref "ExcludeHolidaysFromMaxPickUpDelay" that allows to
take holidays into account while calculating expiration date.

Test plan:

  - Install this patch and run updatedatabase.pl script,
  - allow ExpireReservesMaxPickUpDelay in system preferences,
  - set ReservesMaxPickUpDelay to 5.

  - Place an hold on a checked out item and check in this item:
    The hold's expiration date should be today + 5.

  - Allow ExcludeHolidaysFromMaxPickUpDelay in system preferences,
  - add holiday during this pickup delay period,
  - Create a new hold and make it comes waitting:
    The hold's expiration date should be today + 5 + number of closed
    day(s).

Also:
  - Check that ExpireReservesOnHolidays syspref works again
    without ExcludeHolidaysFromMaxPickUpDelay.
  - Check that cancel fees apply again if wanted.

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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

C4/Reserves.pm

@@ -899,48 +899,28 @@ Cancels all reserves with an expiration date from before today.
 =cut
 
 sub CancelExpiredReserves {
+    my $today = dt_from_string();
+    my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays');
 
-    # Cancel reserves that have passed their expiration date.
     my $dbh = C4::Context->dbh;
     my $sth = $dbh->prepare( "
         SELECT * FROM reserves WHERE DATE(expirationdate) < DATE( CURDATE() )
         AND expirationdate IS NOT NULL
-        AND found IS NULL
     " );
     $sth->execute();
 
     while ( my $res = $sth->fetchrow_hashref() ) {
-        CancelReserve({ reserve_id => $res->{'reserve_id'} });
-    }
+        my $calendar = Koha::Calendar->new( branchcode => $res->{'branchcode'} );
+        my $cancel_params = { reserve_id => $res->{'reserve_id'} };
 
-    # Cancel reserves that have been waiting too long
-    if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) {
-        my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay");
-        my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays');
+        next if !$cancel_on_holidays && $calendar->is_holiday( $today );
 
-        my $today = dt_from_string();
-
-        my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS( waitingdate ) > ? AND found = 'W' AND priority = 0";
-        $sth = $dbh->prepare( $query );
-        $sth->execute( $max_pickup_delay );
-
-        while ( my $res = $sth->fetchrow_hashref ) {
-            my $do_cancel = 1;
-            unless ( $cancel_on_holidays ) {
-                my $calendar = Koha::Calendar->new( branchcode => $res->{'branchcode'} );
-                my $is_holiday = $calendar->is_holiday( $today );
-
-                if ( $is_holiday ) {
-                    $do_cancel = 0;
-                }
-            }
-
-            if ( $do_cancel ) {
-                CancelReserve({ reserve_id => $res->{'reserve_id'}, charge_cancel_fee => 1 });
-            }
+        if ( $res->{found} eq 'W' ) {
+            $cancel_params->{charge_cancel_fee} = 1;
         }
-    }
 
+        CancelReserve($cancel_params);
+    }
 }
 
 =head2 AutoUnsuspendReserves
@@ -1219,33 +1199,10 @@ sub ModReserveAffect {
 
     return unless $hold;
 
-    $reserve_id = $hold->id();
-
     my $already_on_shelf = $hold->found && $hold->found eq 'W';
 
-    # If we affect a reserve that has to be transferred, don't set to Waiting
-    my $query;
-    if ($transferToDo) {
-        $hold->set(
-            {
-                priority   => 0,
-                itemnumber => $itemnumber,
-                found      => 'T',
-            }
-        );
-    }
-    else {
-        # affect the reserve to Waiting as well.
-        $hold->set(
-            {
-                priority    => 0,
-                itemnumber  => $itemnumber,
-                found       => 'W',
-                waitingdate => dt_from_string(),
-            }
-        );
-    }
-    $hold->store();
+    $hold->itemnumber($itemnumber);
+    $hold->set_waiting($transferToDo);
 
     _koha_notify_reserve( $hold->reserve_id )
       if ( !$transferToDo && !$already_on_shelf );

Koha/Calendar.pm

@@ -296,6 +296,20 @@ sub prev_open_day {
     return $base_date;
 }
 
+sub days_forward {
+    my $self     = shift;
+    my $start_dt = shift;
+    my $num_days = shift;
+
+    my $base_dt = $start_dt->clone();
+
+    while ($num_days--) {
+        $base_dt = $self->next_open_day($base_dt);
+    }
+
+    return $base_dt;
+}
+
 sub days_between {
     my $self     = shift;
     my $start_dt = shift;

Koha/Hold.pm

@@ -25,7 +25,7 @@ use Data::Dumper qw(Dumper);
 use C4::Context qw(preference);
 use C4::Log;
 
-use Koha::DateUtils qw(dt_from_string);
+use Koha::DateUtils qw(dt_from_string output_pref);
 use Koha::Patrons;
 use Koha::Biblios;
 use Koha::Items;
@@ -131,6 +131,46 @@ sub waiting_expires_on {
     return $dt;
 }
 
+=head3 set_waiting
+
+=cut
+
+sub set_waiting {
+    my ( $self, $transferToDo ) = @_;
+
+    $self->priority(0);
+
+    if ($transferToDo) {
+        $self->found('T')->store();
+        return $self;
+    }
+
+    my $today = dt_from_string();
+    my $values = {
+        found => 'W',
+        waitingdate => $today->ymd,
+    };
+
+    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 );
+        }
+
+        $values->{expirationdate} = $expirationdate->ymd;
+    }
+
+    $self->set($values)->store();
+
+    return $self;
+}
+
 =head3 is_found
 
 Returns true if hold is a waiting or in transit

installer/data/mysql/atomicupdate/bug_12063-add_excludeholidaysfrommaxpickupdelay_syspref.sql

@@ -0,0 +1 @@
+INSERT IGNORE INTO systempreferences (variable,value,explanation,options,type) VALUES ('ExcludeHolidaysFromMaxPickUpDelay', '0', 'If ON, reserves max pickup delay takes into account the closed days.', NULL, 'Integer');

installer/data/mysql/sysprefs.sql

@@ -155,6 +155,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
 ('ExpireReservesMaxPickUpDelay','0','','Enabling this allows holds to expire automatically if they have not been picked by within the time period specified in ReservesMaxPickUpDelay','YesNo'),
 ('ExpireReservesMaxPickUpDelayCharge','0',NULL,'If ExpireReservesMaxPickUpDelay is enabled, and this field has a non-zero value, than a borrower whose waiting hold has expired will be charged this amount.','free'),
 ('ExpireReservesOnHolidays', '1', NULL, 'If false, reserves at a library will not be canceled on days the library is not open.', 'YesNo'),
+('ExcludeHolidaysFromMaxPickUpDelay', '0', NULL, 'If ON, reserves max pickup delay takes into accountthe closed days.', 'YesNo'),
 ('ExportCircHistory', 0, NULL, "Display the export circulation options",  'YesNo' ),
 ('ExportRemoveFields','',NULL,'List of fields for non export in circulation.pl (separated by a space)','Free'),
 ('ExtendedPatronAttributes','0',NULL,'Use extended patron IDs and attributes','YesNo'),

koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref

@@ -623,6 +623,12 @@ Circulation:
                   yes: Allow
                   no: "Don't allow"
             - expired holds to be canceled on days the library is closed.
+        -
+            - pref: ExcludeHolidaysFromMaxPickUpDelay
+              choices:
+                  yes: Allow
+                  no: "Don't allow"
+            - Closed days to be taken into account in reserves max pickup delay.
         -
             - pref: decreaseLoanHighHolds
               choices:

t/db_dependent/Holds.t

@@ -7,7 +7,7 @@ use t::lib::TestBuilder;
 
 use C4::Context;
 
-use Test::More tests => 61;
+use Test::More tests => 58;
 use MARC::Record;
 use C4::Biblio;
 use C4::Items;
@@ -432,43 +432,6 @@ is(CanItemBeReserved($borrowernumbers[0], $itemnumber),
 is(CanItemBeReserved($borrowernumbers[0], $itemnumber), 'OK',
     "CanItemBeReserved should returns 'OK'");
 
-
-# Test CancelExpiredReserves
-t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelay', 1);
-t::lib::Mocks::mock_preference('ReservesMaxPickUpDelay', 1);
-
-my ( $sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $isdst ) = localtime(time);
-$year += 1900;
-$mon += 1;
-my $reserves = $dbh->selectall_arrayref('SELECT * FROM reserves', { Slice => {} });
-$reserve = $reserves->[0];
-my $calendar = C4::Calendar->new(branchcode => $reserve->{branchcode});
-$calendar->insert_single_holiday(
-    day         => $mday,
-    month       => $mon,
-    year        => $year,
-    title       => 'Test',
-    description => 'Test',
-);
-$reserve_id = $reserve->{reserve_id};
-$dbh->do("UPDATE reserves SET waitingdate = DATE_SUB( NOW(), INTERVAL 5 DAY ), found = 'W', priority = 0 WHERE reserve_id = ?", undef, $reserve_id );
-t::lib::Mocks::mock_preference('ExpireReservesOnHolidays', 0);
-CancelExpiredReserves();
-my $count = $dbh->selectrow_array("SELECT COUNT(*) FROM reserves WHERE reserve_id = ?", undef, $reserve_id );
-is( $count, 1, "Waiting reserve beyond max pickup delay *not* canceled on holiday" );
-t::lib::Mocks::mock_preference('ExpireReservesOnHolidays', 1);
-CancelExpiredReserves();
-$count = $dbh->selectrow_array("SELECT COUNT(*) FROM reserves WHERE reserve_id = ?", undef, $reserve_id );
-is( $count, 0, "Waiting reserve beyond max pickup delay canceled on holiday" );
-
-# Test expirationdate
-$reserve = $reserves->[1];
-$reserve_id = $reserve->{reserve_id};
-$dbh->do("UPDATE reserves SET expirationdate = DATE_SUB( NOW(), INTERVAL 1 DAY ) WHERE reserve_id = ?", undef, $reserve_id );
-CancelExpiredReserves();
-$count = $dbh->selectrow_array("SELECT COUNT(*) FROM reserves WHERE reserve_id = ?", undef, $reserve_id );
-is( $count, 0, "Reserve with manual expiration date canceled correctly" );
-
 # Bug 12632
 t::lib::Mocks::mock_preference( 'item-level_itypes',     1 );
 t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'PatronLibrary' );

t/db_dependent/Holds/CancelReserves.t

@@ -0,0 +1,101 @@
+#!/usr/bin/perl
+
+use Modern::Perl;
+
+use C4::Reserves;
+use Koha::DateUtils;
+
+use t::lib::Mocks;
+use t::lib::TestBuilder;
+
+use Test::More tests => 5;
+
+use_ok('C4::Reserves');
+
+my $schema  = Koha::Database->new->schema;
+$schema->storage->txn_begin;
+
+my $dbh = C4::Context->dbh;
+$dbh->{AutoCommit} = 0;
+$dbh->{RaiseError} = 1;
+
+my $builder = t::lib::TestBuilder->new();
+
+t::lib::Mocks::mock_preference('ExpireReservesOnHolidays', 0);
+
+my $today = dt_from_string();
+my $reserve_reservedate = $today->clone;
+$reserve_reservedate->subtract(days => 30);
+
+my $reserve1_expirationdate = $today->clone;
+$reserve1_expirationdate->add(days => 1);
+
+# Reserve not expired
+my $reserve1 = $builder->build({
+    source => 'Reserve',
+    value => {
+        reservedate => $reserve_reservedate,
+        expirationdate => $reserve1_expirationdate,
+        cancellationdate => undef,
+        priority => 0,
+        found => 'W',
+    },
+});
+
+CancelExpiredReserves();
+my $r1 = Koha::Holds->find($reserve1->{reserve_id});
+ok($r1, 'Reserve 1 should not be canceled.');
+
+my $reserve2_expirationdate = $today->clone;
+$reserve2_expirationdate->subtract(days => 1);
+
+# Reserve expired
+my $reserve2 = $builder->build({
+    source => 'Reserve',
+    value => {
+        reservedate => $reserve_reservedate,
+        expirationdate => $reserve2_expirationdate,
+        cancellationdate => undef,
+        priority => 0,
+        found => 'W',
+    },
+});
+
+CancelExpiredReserves();
+my $r2 = Koha::Holds->find($reserve2->{reserve_id});
+is($r2, undef,'Reserve 2 should be canceled.');
+
+# Reserve expired on holiday
+my $reserve3 = $builder->build({
+    source => 'Reserve',
+    value => {
+        reservedate => $reserve_reservedate,
+        expirationdate => $reserve2_expirationdate,
+        branchcode => 'LIB1',
+        cancellationdate => undef,
+        priority => 0,
+        found => 'W',
+    },
+});
+
+Koha::Cache->get_instance()->flush_all();
+my $holiday = $builder->build({
+    source => 'SpecialHoliday',
+    value => {
+        branchcode => 'LIB1',
+        day => $today->day,
+        month => $today->month,
+        year => $today->year,
+        title => 'My holiday',
+        isexception => 0
+    },
+});
+
+CancelExpiredReserves();
+my $r3 = Koha::Holds->find($reserve3->{reserve_id});
+ok($r3,'Reserve 3 should not be canceled.');
+
+t::lib::Mocks::mock_preference('ExpireReservesOnHolidays', 1);
+CancelExpiredReserves();
+$r3 = Koha::Holds->find($reserve3->{reserve_id});
+is($r3, undef,'Reserve 3 should be canceled.');

t/db_dependent/Holds/WaitingReserves.t

@@ -0,0 +1,208 @@
+#!/usr/bin/perl
+
+use Modern::Perl;
+
+use C4::Reserves;
+use Koha::DateUtils;
+
+use t::lib::Mocks;
+use t::lib::TestBuilder;
+
+use Test::More tests => 10;
+
+use_ok('C4::Reserves');
+
+my $schema  = Koha::Database->new->schema;
+$schema->storage->txn_begin;
+
+my $dbh = C4::Context->dbh;
+$dbh->{AutoCommit} = 0;
+$dbh->{RaiseError} = 1;
+$dbh->do(q{DELETE FROM special_holidays});
+
+my $builder = t::lib::TestBuilder->new();
+
+# Category, branch and patrons
+$builder->build({
+    source => 'Category',
+    value  => {
+        categorycode => 'XYZ1',
+    },
+});
+$builder->build({
+    source => 'Branch',
+    value  => {
+        branchcode => 'LIB1',
+    },
+});
+
+$builder->build({
+    source => 'Branch',
+    value  => {
+        branchcode => 'LIB2',
+    },
+});
+
+my $patron1 = $builder->build({
+    source => 'Borrower',
+    value  => {
+        categorycode => 'XYZ1',
+        branchcode => 'LIB1',
+    },
+});
+
+my $patron2 = $builder->build({
+    source => 'Borrower',
+    value  => {
+        categorycode => 'XYZ1',
+        branchcode => 'LIB2',
+    },
+});
+
+my $biblio = $builder->build({
+    source => 'Biblio',
+    value  => {
+        title => 'Title 1',    },
+});
+
+my $biblio2 = $builder->build({
+    source => 'Biblio',
+    value  => {
+        title => 'Title 2',    },
+});
+
+my $biblio3 = $builder->build({
+    source => 'Biblio',
+    value  => {
+        title => 'Title 3',    },
+});
+
+my $item1 = $builder->build({
+    source => 'Item',
+    value  => {
+        biblionumber => $biblio->{biblionumber},
+    },
+});
+
+my $item2 = $builder->build({
+    source => 'Item',
+    value  => {
+        biblionumber => $biblio2->{biblionumber},
+    },
+});
+
+my $item3 = $builder->build({
+    source => 'Item',
+    value  => {
+        biblionumber => $biblio3->{biblionumber},
+    },
+});
+
+my $today = dt_from_string();
+
+my $reserve1_reservedate = $today->clone;
+$reserve1_reservedate->subtract(days => 20);
+
+my $reserve1_expirationdate = $today->clone;
+$reserve1_expirationdate->add(days => 6);
+
+my $reserve1 = $builder->build({
+    source => 'Reserve',
+    value => {
+        borrowernumber => $patron1->{borrowernumber},
+        reservedate => $reserve1_reservedate->ymd,
+        expirationdate => undef,
+        biblionumber => $biblio->{biblionumber},
+        branchcode => 'LIB1',
+        priority => 1,
+        found => '',
+    },
+});
+
+t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelay', 1);
+t::lib::Mocks::mock_preference('ReservesMaxPickUpDelay', 6);
+
+ModReserveAffect( $item1->{itemnumber}, $patron1->{borrowernumber}, 0);
+my $r = Koha::Holds->find($reserve1->{reserve_id});
+
+is($r->waitingdate, $today->ymd, 'Waiting date should be set to today' );
+is($r->expirationdate, $reserve1_expirationdate->ymd, 'Expiration date should be set to today + 6' );
+is($r->found, 'W', 'Reserve status is now "waiting"' );
+is($r->priority, 0, 'Priority should be 0' );
+is($r->itemnumber, $item1->{itemnumber}, 'Item number should be set correctly' );
+
+my $reserve2 = $builder->build({
+    source => 'Reserve',
+    value => {
+        borrowernumber => $patron2->{borrowernumber},
+        reservedate => $reserve1_reservedate->ymd,
+        expirationdate => undef,
+        biblionumber => $biblio2->{biblionumber},
+        branchcode => 'LIB1',
+        priority => 1,
+        found => '',
+    },
+});
+
+ModReserveAffect( $item2->{itemnumber}, $patron2->{borrowernumber}, 1);
+my $r2 = Koha::Holds->find($reserve2->{reserve_id});
+
+is($r2->found, 'T', '2nd reserve - Reserve status is now "To transfer"' );
+is($r2->priority, 0, '2nd reserve - Priority should be 0' );
+is($r2->itemnumber, $item2->{itemnumber}, '2nd reserve - Item number should be set correctly' );
+
+my $reserve3 = $builder->build({
+    source => 'Reserve',
+    value => {
+        borrowernumber => $patron2->{borrowernumber},
+        reservedate => $reserve1_reservedate->ymd,
+        expirationdate => undef,
+        biblionumber => $biblio3->{biblionumber},
+        branchcode => 'LIB1',
+        priority => 1,
+        found => '',
+    },
+});
+
+my $special_holiday1_dt = $today->clone;
+$special_holiday1_dt->add(days => 2);
+
+Koha::Cache->get_instance()->flush_all();
+my $holiday = $builder->build({
+    source => 'SpecialHoliday',
+    value => {
+        branchcode => 'LIB1',
+        day => $special_holiday1_dt->day,
+        month => $special_holiday1_dt->month,
+        year => $special_holiday1_dt->year,
+        title => 'My special holiday',
+        isexception => 0
+    },
+});
+
+my $special_holiday2_dt = $today->clone;
+$special_holiday2_dt->add(days => 4);
+
+my $holiday2 = $builder->build({
+    source => 'SpecialHoliday',
+    value => {
+        branchcode => 'LIB1',
+        day => $special_holiday2_dt->day,
+        month => $special_holiday2_dt->month,
+        year => $special_holiday2_dt->year,
+        title => 'My special holiday 2',
+        isexception => 0
+    },
+});
+
+t::lib::Mocks::mock_preference('ExcludeHolidaysFromMaxPickUpDelay', 1);
+ModReserveAffect( $item3->{itemnumber}, $patron2->{borrowernumber}, 0);
+
+# Add 6 days of pickup delay + 1 day of holiday.
+my $expected_expiration = $today->clone;
+$expected_expiration->add(days => 8);
+
+my $r3 = Koha::Holds->find($reserve3->{reserve_id});
+is($r3->expirationdate, $expected_expiration->ymd, 'Expiration date should be set to today + 7' );
+
+$dbh->rollback;