From 593eade29dfc044913504357b1da99e782ac929b 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 Content-Type: text/plain; charset=utf-8 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 Signed-off-by: Marcel de Rooy --- C4/Reserves.pm | 65 ++----- 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, 384 insertions(+), 93 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 3192d50..9851181 100644 --- a/C4/Reserves.pm +++ b/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'} }); - } - - # 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 @@ -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 ); diff --git a/Koha/Calendar.pm b/Koha/Calendar.pm index f6cb58c..3c04fbe 100644 --- a/Koha/Calendar.pm +++ b/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; diff --git a/Koha/Hold.pm b/Koha/Hold.pm index ba75a52..3b2216e 100644 --- a/Koha/Hold.pm +++ b/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 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 3e0f93b..5a3d2ba 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/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'), 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 a583f2b..254c6e8 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 @@ -624,6 +624,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 1b4f1df..b00a6da 100755 --- a/t/db_dependent/Holds.t +++ b/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' ); 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 -- 2.1.4