From 4863cb3eef1fe31fdbe1da70890eb6a457badea6 Mon Sep 17 00:00:00 2001 From: Alex Arnaud Date: Mon, 20 Jun 2016 11:52:37 +0200 Subject: [PATCH] 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 --- C4/Reserves.pm | 72 ++----- Koha/Calendar.pm | 14 ++ Koha/Hold.pm | 42 +++- ...d_excludeholidaysfrommaxpickupdelay_syspref.sql | 1 + installer/data/mysql/sysprefs.sql | 1 + .../en/modules/admin/preferences/circulation.pref | 6 + t/db_dependent/Holds.t | 39 +--- t/db_dependent/Holds/CancelReserves.t | 101 ++++++++++ t/db_dependent/Holds/WaitingReserves.t | 208 ++++++++++++++++++++ 9 files changed, 388 insertions(+), 96 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_12063-add_excludeholidaysfrommaxpickupdelay_syspref.sql create mode 100644 t/db_dependent/Holds/CancelReserves.t create mode 100644 t/db_dependent/Holds/WaitingReserves.t diff --git a/C4/Reserves.pm b/C4/Reserves.pm index b83f8e1..e51dac9 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -988,48 +988,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'} }); - } - - # 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'); - - 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 ); + my $calendar = Koha::Calendar->new( branchcode => $res->{'branchcode'} ); + my $cancel_params = { reserve_id => $res->{'reserve_id'} }; - if ( $is_holiday ) { - $do_cancel = 0; - } - } + next if !$cancel_on_holidays && $calendar->is_holiday( $today ); - 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 @@ -1319,35 +1299,13 @@ sub ModReserveAffect { biblionumber => $biblionumber, }); return unless defined $reserve_id; - my $request = GetReserveInfo($reserve_id); - my $already_on_shelf = ($request && $request->{found} eq 'W') ? 1 : 0; - # If we affect a reserve that has to be transferred, don't set to Waiting - my $query; - if ($transferToDo) { - $query = " - UPDATE reserves - SET priority = 0, - itemnumber = ?, - found = 'T' - WHERE borrowernumber = ? - AND biblionumber = ? - "; - } - else { - # affect the reserve to Waiting as well. - $query = " - UPDATE reserves - SET priority = 0, - found = 'W', - waitingdate = NOW(), - itemnumber = ? - WHERE borrowernumber = ? - AND biblionumber = ? - "; - } - $sth = $dbh->prepare($query); - $sth->execute( $itemnumber, $borrowernumber,$biblionumber); + my $reserve = Koha::Holds->find($reserve_id); + my $already_on_shelf = ($reserve && $reserve->found eq 'W') ? 1 : 0; + + $reserve->itemnumber($itemnumber); + $reserve->set_waiting($transferToDo); + _koha_notify_reserve( $itemnumber, $borrowernumber, $biblionumber ) if ( !$transferToDo && !$already_on_shelf ); _FixPriority( { biblionumber => $biblionumber } ); if ( C4::Context->preference("ReturnToShelvingCart") ) { diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index 1321621..998a861 100644 --- a/Koha/Calendar.pm +++ b/Koha/Calendar.pm @@ -294,6 +294,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; diff --git a/Koha/Hold.pm b/Koha/Hold.pm index f4d541a..aa231f7 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -23,7 +23,7 @@ use Carp; use C4::Context qw(preference); -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; @@ -106,6 +106,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 diff --git a/installer/data/mysql/atomicupdate/bug_12063-add_excludeholidaysfrommaxpickupdelay_syspref.sql b/installer/data/mysql/atomicupdate/bug_12063-add_excludeholidaysfrommaxpickupdelay_syspref.sql new file mode 100644 index 0000000..c19cc5a --- /dev/null +++ b/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'); \ No newline at end of file diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index af42f66..d977303 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -143,6 +143,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'), ('ExtendedPatronAttributes','0',NULL,'Use extended patron IDs and attributes','YesNo'), ('FacetLabelTruncationLength','20',NULL,'Specify the facet max length in OPAC','Integer'), ('FacetMaxCount','20',NULL,'Specify the max facet count for each category','Integer'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index 9a37608..37d9c93 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -592,6 +592,12 @@ Circulation: 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: yes: Enable diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index f9ddc46..63e8ec7 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -8,7 +8,7 @@ use t::lib::TestBuilder; use C4::Context; use C4::Branch; -use Test::More tests => 60; +use Test::More tests => 57; use MARC::Record; use C4::Biblio; use C4::Items; @@ -409,43 +409,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; -$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' ); diff --git a/t/db_dependent/Holds/CancelReserves.t b/t/db_dependent/Holds/CancelReserves.t new file mode 100644 index 0000000..deee1a9 --- /dev/null +++ b/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.'); diff --git a/t/db_dependent/Holds/WaitingReserves.t b/t/db_dependent/Holds/WaitingReserves.t new file mode 100644 index 0000000..6167f43 --- /dev/null +++ b/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; \ No newline at end of file -- 1.7.10.4