From 7c44efa7cf23168fe93c3af471203bc9d88d61cf Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Wed, 28 Oct 2015 18:37:26 +0200 Subject: [PATCH] Bug 10744 - ExpireReservesMaxPickUpDelay works with hold(s) over report Hold(s) over -report at circ/waitingreserves.pl doesn't work because ExpireReservesMaxPickup removes reserves from the koha.reserves-table to the koha.old_reserves-table. This patch adds a new column koha.reserves.pickupexpired telling when/if the reserve has got it's pickup duration expired. This date is used to pull old reserves to the waitingreserves-view. syspref 'PickupExpiredHoldsOverReportDuration' controls how many days after expiration old reserves are displayed on the report, and the values respect Koha::Calendar holidays To test: 1) Apply patch. 2) Enable 'ExpireReservesMaxPickUpDelay'. 3) Set some value to 'ReservesMaxPickUpDelay' and 'PickupExpiredHoldsOverReportDuration' (e.g. 2 and 7). 4) Find or create hold(s) waiting pickup and set their expiration date so it exceeds 'ReservesMaxPickUpDelay' but not 'PickupExpiredHoldsOverReportDuration'. 5) Confirm hold(s) are moved to 'Holds waiting over ...' list. 6) Run misc/cronjobs/holds/cancel_expired_holds.pl. 7) Confirm hold(s) are still displayed on the list. 8) Confirm from db that hold(s) have 'pickupexpired' set same as 'expirationdate'. 9) Set new value to 'PickupExpiredHoldsOverReportDuration' so it no longer covers hold(s) expiration date(s). 10) Confirm that hold(s) are no longer displayed in waiting list Also prove t/db_depentent/Koha/Old.t --- C4/Reserves.pm | 3 + Koha/Hold.pm | 11 ++ Koha/Old/Holds.pm | 59 ++++++++++ Koha/Schema/Result/OldReserve.pm | 12 +- Koha/Schema/Result/Reserve.pm | 12 +- circ/waitingreserves.pl | 9 +- ...re_reserves_conflict_hold_over_report.perl | 16 +++ installer/data/mysql/kohastructure.sql | 4 + installer/data/mysql/sysprefs.sql | 1 + .../admin/preferences/circulation.pref | 4 + t/db_dependent/Koha/Old.t | 103 +++++++++++++++++- 11 files changed, 228 insertions(+), 6 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 2c17d53235..7026dae85d 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -907,6 +907,9 @@ sub CancelExpiredReserves { if ( $hold->found eq 'W' ) { $cancel_params->{charge_cancel_fee} = 1; } + + $cancel_params->{pickupexpired} = dt_from_string($hold->expirationdate); + $hold->cancel( $cancel_params ); } } diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 8969e32839..0829fe04fd 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -35,6 +35,7 @@ use Koha::Old::Holds; use Koha::Calendar; use Koha::Exceptions::Hold; +use Koha::Exceptions::BadParameter; use base qw(Koha::Object); @@ -363,6 +364,16 @@ sub cancel { my ( $self, $params ) = @_; $self->_result->result_source->schema->txn_do( sub { + + my $pickupexpired = $params->{pickupexpired}; + if ($pickupexpired) { + unless ($pickupexpired && $pickupexpired->isa('DateTime')) { + Koha::Exceptions::BadParameter->throw(error => "cancel():> Parameter 'pickupexpired' is not a DateTime-object or undef!"); + } else { + $self->pickupexpired($pickupexpired->ymd()); + } + } + $self->cancellationdate( dt_from_string->strftime( '%Y-%m-%d %H:%M:%S' ) ); $self->priority(0); $self->_move_to_old; diff --git a/Koha/Old/Holds.pm b/Koha/Old/Holds.pm index e12e1446b2..74dccba297 100644 --- a/Koha/Old/Holds.pm +++ b/Koha/Old/Holds.pm @@ -20,8 +20,10 @@ package Koha::Old::Holds; use Modern::Perl; use Carp; +use Scalar::Util qw(blessed); use Koha::Database; +use Koha::Exceptions::BadParameter; use Koha::Old::Hold; @@ -39,6 +41,63 @@ This object represents a set of holds that have been filled or canceled =cut +=head3 + +my $expired_holds = Koha::Old::Holds->expired(); + +Returns all holds that have already expired and moved to old_reserves during period +of time given in 'PickupExpiredHoldsOverReportDuration'. + +If branchcode is given returns expired holds for that branch and take into account +its holidays. + +=cut + +sub expired { + my ($self, $params) = @_; + + my $pickupExpiredHoldsOverReportDuration = C4::Context->preference('PickupExpiredHoldsOverReportDuration'); + return undef unless $pickupExpiredHoldsOverReportDuration; + + my $branchcode = $params->{branchcode}; + if ($params->{from}) { + unless (blessed($params->{from}) && $params->{from}->isa('DateTime')) { + Koha::Exceptions::BadParameter->throw(error => "expired():> Parameter 'from' is not a DateTime-object or undef!"); + } + } + if ($params->{to}) { + unless (blessed($params->{from}) && $params->{from}->isa('DateTime')) { + Koha::Exceptions::BadParameter->throw(error => "expired():> Parameter 'from' is not a DateTime-object or undef!"); + } + } + + #Calculate the days for which we get the expired reserves. + my $fromDate = $params->{from}; + my $toDate = $params->{to} || DateTime->now(time_zone => C4::Context->tz()); + + unless ($fromDate) { + $fromDate = DateTime->now( time_zone => C4::Context->tz() ); + #Look for previous open days + if ($branchcode) { + my $calendar = Koha::Calendar->new( branchcode => $branchcode, days_mode => 'Default'); + foreach my $i (1..$pickupExpiredHoldsOverReportDuration) { + $fromDate = $calendar->prev_open_days($fromDate, 1); + } + } + #If no branch has been specified we cannot use a calendar, so simply just go back in time. + else { + $fromDate = DateTime->now(time_zone => C4::Context->tz())->subtract(days => $pickupExpiredHoldsOverReportDuration); + } + } + + my $search_params; + $search_params->{priority} = 0; + $search_params->{pickupexpired} = { -between => [ $fromDate->ymd(), $toDate->ymd() ] }; + $search_params->{branchcode} = $branchcode if defined $branchcode; + + return $self->search( $search_params, { order_by => 'waitingdate' } ); +} + =head3 type =cut diff --git a/Koha/Schema/Result/OldReserve.pm b/Koha/Schema/Result/OldReserve.pm index 3df98cc4e5..f09146b4f3 100644 --- a/Koha/Schema/Result/OldReserve.pm +++ b/Koha/Schema/Result/OldReserve.pm @@ -112,6 +112,12 @@ __PACKAGE__->table("old_reserves"); datetime_undef_if_invalid: 1 is_nullable: 1 +=head2 pickupexpired + + data_type: 'date' + datetime_undef_if_invalid: 1 + is_nullable: 1 + =head2 lowestPriority accessor: 'lowest_priority' @@ -182,6 +188,8 @@ __PACKAGE__->add_columns( { data_type => "date", datetime_undef_if_invalid => 1, is_nullable => 1 }, "expirationdate", { data_type => "date", datetime_undef_if_invalid => 1, is_nullable => 1 }, + "pickupexpired", + { data_type => "date", datetime_undef_if_invalid => 1, is_nullable => 1 }, "lowestPriority", { accessor => "lowest_priority", @@ -298,8 +306,8 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07046 @ 2020-03-18 12:43:15 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:4vMUC/1kSr3vgQ7n0Pmuug +# Created by DBIx::Class::Schema::Loader v0.07049 @ 2020-04-28 12:23:24 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:AAKfhZfJaya1XUWTqeVMgA __PACKAGE__->add_columns( '+item_level_hold' => { is_boolean => 1 }, diff --git a/Koha/Schema/Result/Reserve.pm b/Koha/Schema/Result/Reserve.pm index c5460e34da..050c7a5a53 100644 --- a/Koha/Schema/Result/Reserve.pm +++ b/Koha/Schema/Result/Reserve.pm @@ -116,6 +116,12 @@ __PACKAGE__->table("reserves"); datetime_undef_if_invalid: 1 is_nullable: 1 +=head2 pickupexpired + + data_type: 'date' + datetime_undef_if_invalid: 1 + is_nullable: 1 + =head2 lowestPriority accessor: 'lowest_priority' @@ -196,6 +202,8 @@ __PACKAGE__->add_columns( { data_type => "date", datetime_undef_if_invalid => 1, is_nullable => 1 }, "expirationdate", { data_type => "date", datetime_undef_if_invalid => 1, is_nullable => 1 }, + "pickupexpired", + { data_type => "date", datetime_undef_if_invalid => 1, is_nullable => 1 }, "lowestPriority", { accessor => "lowest_priority", @@ -337,8 +345,8 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07046 @ 2020-03-18 12:43:15 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:VH7h5kYo9WhlGobXb3N3Jg +# Created by DBIx::Class::Schema::Loader v0.07049 @ 2020-04-28 12:23:24 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:lCAjQKeOZPYLaMstOD0oiw __PACKAGE__->belongs_to( "item", diff --git a/circ/waitingreserves.pl b/circ/waitingreserves.pl index abefc095e5..cb0c625622 100755 --- a/circ/waitingreserves.pl +++ b/circ/waitingreserves.pl @@ -39,6 +39,7 @@ use Koha::BiblioFrameworks; use Koha::Items; use Koha::ItemTypes; use Koha::Patrons; +use Koha::Old::Holds; my $input = new CGI; @@ -84,7 +85,7 @@ $template->param( all_branches => 1 ) if $all_branches; my (@reserve_loop, @over_loop); # FIXME - Is priority => 0 useful? If yes it must be moved to waiting, otherwise we need to remove it from here. my $holds = Koha::Holds->waiting->search({ priority => 0, ( $all_branches ? () : ( branchcode => $default ) ) }, { order_by => ['waitingdate'] }); - +my $expired_holds = $all_branches ? Koha::Old::Holds->expired() : Koha::Old::Holds->expired({branchcode => $default}); # get reserves for the branch we are logged into, or for all branches my $today = Date_to_Days(&Today); @@ -109,6 +110,12 @@ while ( my $hold = $holds->next ) { } +if($expired_holds){ + while (my $expired_hold = $expired_holds->next){ + push @over_loop, $expired_hold; + } +} + $template->param(cancel_result => \@cancel_result) if @cancel_result; $template->param( diff --git a/installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl b/installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl new file mode 100644 index 0000000000..71753eec9f --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_10744-expire_reserves_conflict_hold_over_report.perl @@ -0,0 +1,16 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + # you can use $dbh here like: + # $dbh->do( "ALTER TABLE biblio ADD COLUMN badtaste int" ); + + $dbh->do("ALTER TABLE reserves ADD `pickupexpired` DATE DEFAULT NULL AFTER `expirationdate`"); + $dbh->do("ALTER TABLE reserves ADD KEY `reserves_pickupexpired` (`pickupexpired`)"); + $dbh->do("ALTER TABLE old_reserves ADD `pickupexpired` DATE DEFAULT NULL AFTER `expirationdate`"); + $dbh->do("ALTER TABLE old_reserves ADD KEY `old_reserves_pickupexpired` (`pickupexpired`)"); + + $dbh->do("INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('PickupExpiredHoldsOverReportDuration','1',NULL,\"For how many days holds expired by the 'ExpireReservesMaxPickUpDelay'-syspref are visible in the 'Hold Over'-tab in /circ/waitingreserves.pl ?\",'Integer')"); + + # Always end with this (adjust the bug info) + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 10744 - ExpireReservesMaxPickUpDelay works with hold(s) over report)\n"; +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 4f6a9c399d..5ce5e7c993 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1759,6 +1759,7 @@ CREATE TABLE `reserves` ( -- information related to holds/reserves in Koha `itemnumber` int(11) default NULL, -- foreign key from the items table defining the specific item the patron has placed on hold or the item this hold was filled with `waitingdate` date default NULL, -- the date the item was marked as waiting for the patron at the library `expirationdate` DATE DEFAULT NULL, -- the date the hold expires (usually the date entered by the patron to say they don't need the hold after a certain date) + `pickupexpired` DATE DEFAULT NULL, -- if hold has been waiting but it expired before it was picked up, the expiration date is set here `lowestPriority` tinyint(1) NOT NULL DEFAULT 0, `suspend` tinyint(1) NOT NULL DEFAULT 0, `suspend_until` DATETIME NULL DEFAULT NULL, @@ -1770,6 +1771,7 @@ CREATE TABLE `reserves` ( -- information related to holds/reserves in Koha KEY `biblionumber` (`biblionumber`), KEY `itemnumber` (`itemnumber`), KEY `branchcode` (`branchcode`), + KEY `reserves_pickupexpired` (`pickupexpired`), KEY `itemtype` (`itemtype`), CONSTRAINT `reserves_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT `reserves_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE, @@ -1799,6 +1801,7 @@ CREATE TABLE `old_reserves` ( -- this table holds all holds/reserves that have b `itemnumber` int(11) default NULL, -- foreign key from the items table defining the specific item the patron has placed on hold or the item this hold was filled with `waitingdate` date default NULL, -- the date the item was marked as waiting for the patron at the library `expirationdate` DATE DEFAULT NULL, -- the date the hold expires (usually the date entered by the patron to say they don't need the hold after a certain date) + `pickupexpired` DATE DEFAULT NULL, -- if hold has been waiting but it expired before it was picked up, the expiration date is set here `lowestPriority` tinyint(1) NOT NULL DEFAULT 0, -- has this hold been pinned to the lowest priority in the holds queue (1 for yes, 0 for no) `suspend` tinyint(1) NOT NULL DEFAULT 0, -- in this hold suspended (1 for yes, 0 for no) `suspend_until` DATETIME NULL DEFAULT NULL, -- the date this hold is suspended until (NULL for infinitely) @@ -1809,6 +1812,7 @@ CREATE TABLE `old_reserves` ( -- this table holds all holds/reserves that have b KEY `old_reserves_biblionumber` (`biblionumber`), KEY `old_reserves_itemnumber` (`itemnumber`), KEY `old_reserves_branchcode` (`branchcode`), + KEY `old_reserves_pickupexpired` (`pickupexpired`), KEY `old_reserves_itemtype` (`itemtype`), CONSTRAINT `old_reserves_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL, diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index d67a02b367..f2d965cb82 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -185,6 +185,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('EnhancedMessagingPreferencesOPAC', '1', NULL, 'If ON, show patrons messaging setting on the OPAC.', 'YesNo'), ('expandedSearchOption','0',NULL,'If ON, set advanced search to be expanded by default','YesNo'), ('ExpireReservesMaxPickUpDelay','0','','Enabling this allows holds to expire automatically if they have not been picked by within the time period specified in ReservesMaxPickUpDelay','YesNo'), +('PickupExpiredHoldsOverReportDuration','1',NULL,"For how many days holds expired by the 'ExpireReservesMaxPickUpDelay'-syspref are visible in the 'Hold Over'-tab in /circ/waitingreserves.pl ?",'Integer'), ('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'), 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 f2379e797e..c2c02154df 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 @@ -698,6 +698,10 @@ Circulation: - If using ExpireReservesMaxPickUpDelay, charge a borrower who allows their waiting hold to expire a fee of - pref: ExpireReservesMaxPickUpDelayCharge class: currency + - + - pref: PickupExpiredHoldsOverReportDuration + class: integer + - "For how many days holds expired by the 'ExpireReservesMaxPickUpDelay'-syspref are visible in the 'Hold Over'-tab in /circ/waitingreserves.pl ?" - - Satisfy holds using items from the libraries - pref: StaticHoldsQueueWeight diff --git a/t/db_dependent/Koha/Old.t b/t/db_dependent/Koha/Old.t index de2b0ba69f..3d2ef43d82 100644 --- a/t/db_dependent/Koha/Old.t +++ b/t/db_dependent/Koha/Old.t @@ -19,18 +19,28 @@ use Modern::Perl; -use Test::More tests => 2; +use Test::More tests => 3; +use DateTime; +use DateTime::TimeZone; + +use C4::Reserves; use Koha::Database; use Koha::Old::Patrons; use Koha::Old::Biblios; use Koha::Old::Items; +use Koha::Old::Holds; +use Koha::Calendar; use t::lib::TestBuilder; +use t::lib::Mocks; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; +#since we use calendar here, flush caches +Koha::Caches->get_instance()->flush_all(); + my $builder = t::lib::TestBuilder->new; subtest 'Koha::Old::Patrons' => sub { @@ -54,4 +64,95 @@ subtest 'Koha::Old::Biblios and Koha::Old::Items' => sub { # Cannot be tested in a meaningful way so far ok(1); }; + $schema->storage->txn_rollback; + +subtest 'Koha::Old::Holds' => sub { + + $schema->storage->txn_begin; + + plan tests => 5; + + my $branch = $builder->build_object({ class => 'Koha::Libraries' }); + my $calendar = Koha::Calendar->new( branchcode => $branch->branchcode ); + my $holiday_date = DateTime->now(time_zone => C4::Context->tz())->subtract(days => 2); + my $holiday = $builder->build({ + source => 'SpecialHoliday', + value => { + branchcode => $branch->branchcode, + day => $holiday_date->day, + month => $holiday_date->month, + year => $holiday_date->year, + }, + }); + + t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelay', 1); + + t::lib::Mocks::mock_preference('ReservesMaxPickUpDelay', 6); + t::lib::Mocks::mock_preference('PickupExpiredHoldsOverReportDuration', 2); + + my $hold_1 = $builder->build_object({ + class => 'Koha::Holds', + value => { + branchcode => $branch->branchcode, + expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 3)->iso8601(), + } + }); + my $hold_2 = $builder->build_object({ + class => 'Koha::Holds', + value => { + branchcode => $branch->branchcode, + expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 2)->iso8601(), + } + }); + my $hold_3 = $builder->build_object({ + class => 'Koha::Holds', + value => { + branchcode => $branch->branchcode, + expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->iso8601(), + } + }); + my $hold_4 = $builder->build_object({ + class => 'Koha::Holds', + value => { + branchcode => $branch->branchcode, + expirationdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->iso8601(), + } + }); + my $hold_5 = $builder->build_object({ + class => 'Koha::Holds', + value => { + branchcode => $branch->branchcode, + expirationdate => DateTime->now(time_zone => C4::Context->tz()), + } + }); + my $hold_6 = $builder->build_object({ + class => 'Koha::Holds', + value => { + branchcode => $branch->branchcode, + expirationdate => DateTime->now(time_zone => C4::Context->tz())->add(days => 1)->iso8601(), + } + }); + + C4::Reserves::CancelExpiredReserves(); + + my $expired_holds = Koha::Old::Holds->expired({ branchcode => $branch->branchcode }); + is($expired_holds->count(), 3, 'expired() returns 3 holds'); + + my $expired_hold_1 = $expired_holds->find($hold_1->id); + my $expired_hold_2 = $expired_holds->find($hold_2->id); + my $expired_hold_3 = $expired_holds->find($hold_3->id); + my $expired_hold_4 = $expired_holds->find($hold_4->id); + my $expired_hold_5 = $expired_holds->find($hold_5->id); + my $expired_hold_6 = $expired_holds->find($hold_6->id); + + is($expired_hold_1, undef, "Hold 1 not found with expired()."); + ok($expired_hold_2->pickupexpired eq DateTime->now(time_zone => C4::Context->tz())->subtract(days => 2)->ymd() + , "Pick up for hold 2 expired 2 days ago."); + ok($expired_hold_3->pickupexpired && $expired_hold_4->pickupexpired eq DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->ymd() + , "Pick up for holds 3 and 4 expired yesterday."); + is($expired_hold_5 && $expired_hold_6, undef, "Holds 5 and 6 not expired."); + + $schema->storage->txn_rollback; + +}; -- 2.17.1