@@ -, +, @@ skip all holiday - 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). - Check that ExpireReservesOnHolidays syspref works again without ExcludeHolidaysFromMaxPickUpDelay. - Check that cancel fees apply again if wanted. --- 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 --- a/C4/Reserves.pm +++ a/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") ) { --- a/Koha/Calendar.pm +++ a/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; --- a/Koha/Hold.pm +++ a/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 --- a/installer/data/mysql/atomicupdate/bug_12063-add_excludeholidaysfrommaxpickupdelay_syspref.sql +++ a/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'); --- a/installer/data/mysql/sysprefs.sql +++ a/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'), --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ a/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 --- a/t/db_dependent/Holds.t +++ a/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; @@ -413,43 +413,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' ); --- a/t/db_dependent/Holds/CancelReserves.t +++ a/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.'); --- a/t/db_dependent/Holds/WaitingReserves.t +++ a/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; --