From d02d183556d4f7dcd3c720512ab0389410a761c0 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 has minor workflow conflicts with hold(s) over report Initial commit. --- C4/Reserves.pm | 86 +++++++++++++- circ/waitingreserves.pl | 4 +- ...Bug10744ExpireReservesConflictHoldOverReport.pl | 36 ++++++ installer/data/mysql/kohastructure.sql | 4 + installer/data/mysql/sysprefs.sql | 1 + .../en/modules/admin/preferences/circulation.pref | 4 + .../pickupExpiredHoldsOverReportDuration.t | 121 ++++++++++++++++++++ 7 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/Bug10744ExpireReservesConflictHoldOverReport.pl create mode 100644 t/db_dependent/Reserves/pickupExpiredHoldsOverReportDuration.t diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 7630292..a66a97b 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -43,6 +43,7 @@ use Koha::Calendar; use DateTime; use List::MoreUtils qw( firstidx ); +use Scalar::Util qw(blessed); use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); @@ -791,6 +792,59 @@ sub GetReservesForBranch { return (@transreserv); } +=head GetExpiredReserves + + my $expiredReserves = C4::Reserves::GetExpiredReserves({branchcode => 'CPL', + from => DateTime->new(...), + to => DateTime->now(), + }); + +@RETURNS ARRAYRef of expired reserves from the given duration. Defaults to this day. +=cut + +sub GetExpiredReserves { + my ($params) = @_; + + my $pickupExpiredHoldsOverReportDuration = C4::Context->preference('PickupExpiredHoldsOverReportDuration'); + return [] unless $pickupExpiredHoldsOverReportDuration; + + my $branchcode = $params->{branchcode}; + if ($params->{from}) { + unless (blessed($params->{from}) && $params->{from}->isa('DateTime')) { + Koha::Exception::BadParameter->throw(error => "GetExpiredReserves():> Parameter 'from' is not a DateTime-object or undef!"); + } + } + if ($params->{to}) { + unless (blessed($params->{from}) && $params->{from}->isa('DateTime')) { + Koha::Exception::BadParameter->throw(error => "GetExpiredReserves():> Parameter 'from' is not a DateTime-object or undef!"); + } + } + + my $fromDate = $params->{from} || DateTime->now(time_zone => C4::Context->tz())->subtract(days => $pickupExpiredHoldsOverReportDuration); + my $toDate = $params->{to} || DateTime->now(time_zone => C4::Context->tz()); + + my $dbh = C4::Context->dbh; + + my @params = ($fromDate->ymd(), $toDate->ymd()); + my $query = " + SELECT * + FROM old_reserves + WHERE priority='0' + AND pickupexpired BETWEEN ? AND ? + "; + if ( $branchcode ) { + push @params, $branchcode; + $query .= " AND branchcode=? "; + } + $query .= "ORDER BY waitingdate" ; + + my $sth = $dbh->prepare($query); + $sth->execute(@params); + + my $data = $sth->fetchall_arrayref({}); + return ($data) ? $data : []; +} + =head2 GetReserveStatus $reservestatus = GetReserveStatus($itemnumber, $biblionumber); @@ -1021,7 +1075,9 @@ sub CancelExpiredReserves { if ( $charge ) { manualinvoice($res->{'borrowernumber'}, $res->{'itemnumber'}, 'Hold waiting too long', 'F', $charge); } - CancelReserve({ reserve_id => $res->{'reserve_id'} }); + CancelReserve({ reserve_id => $res->{'reserve_id'}, + pickupexpired => $expiration, + }); push @sb, printReserve($res,'tab',['reserve_id','borrowernumber','branchcode','waitingdate']).sprintf("% 14s",substr($expiration,0,10))."| past lastpickupdate.\n" if $verbose; } elsif($verbose > 1) { @@ -1064,7 +1120,12 @@ sub AutoUnsuspendReserves { =head2 CancelReserve - CancelReserve({ reserve_id => $reserve_id, [ biblionumber => $biblionumber, borrowernumber => $borrrowernumber, itemnumber => $itemnumber ] }); + CancelReserve({ reserve_id => $reserve_id, + [ biblionumber => $biblionumber, + borrowernumber => $borrrowernumber, + itemnumber => $itemnumber ], + pickupexpired => DateTime->new(year => 2015, ...), #If the reserve was waiting for pickup, set the date the pickup wait period expired. + }); Cancels a reserve. @@ -1074,6 +1135,13 @@ sub CancelReserve { my ( $params ) = @_; my $reserve_id = $params->{'reserve_id'}; + my $pickupexpired = $params->{pickupexpired}; + if ($pickupexpired) { + unless (blessed($pickupexpired) && $pickupexpired->isa('DateTime')) { + Koha::Exception::BadParameter->throw(error => "CancelReserve():> Parameter 'pickupexpired' is not a DateTime-object or undef!"); + } + } + $reserve_id = GetReserveId( $params ) unless ( $reserve_id ); return unless ( $reserve_id ); @@ -1082,15 +1150,25 @@ sub CancelReserve { my $reserve = GetReserve( $reserve_id ); + my @params; my $query = " UPDATE reserves - SET cancellationdate = now(), + SET cancellationdate = DATE(NOW()), found = Null, + "; + if ($pickupexpired) { + push @params, $pickupexpired->ymd(); + $query .= " + pickupexpired = ?, + "; + } + push @params, $reserve_id; + $query .= " priority = 0 WHERE reserve_id = ? "; my $sth = $dbh->prepare($query); - $sth->execute( $reserve_id ); + $sth->execute( @params ); $query = " INSERT INTO old_reserves diff --git a/circ/waitingreserves.pl b/circ/waitingreserves.pl index f4a93f0..6e6ded2 100755 --- a/circ/waitingreserves.pl +++ b/circ/waitingreserves.pl @@ -80,8 +80,10 @@ $template->param( all_branches => 1 ) if $all_branches; my (@reservloop, @overloop); my ($reservcount, $overcount); -my @getreserves = $all_branches ? GetReservesForBranch() : GetReservesForBranch($default); # get reserves for the branch we are logged into, or for all branches +my @getreserves = $all_branches ? GetReservesForBranch() : GetReservesForBranch($default); +my $expiredReserves = $all_branches ? C4::Reserves::GetExpiredReserves() : C4::Reserves::GetExpiredReserves({branchcode => $default}); +push @getreserves, @$expiredReserves; my $today = DateTime->now(); foreach my $num (@getreserves) { diff --git a/installer/data/mysql/atomicupdate/Bug10744ExpireReservesConflictHoldOverReport.pl b/installer/data/mysql/atomicupdate/Bug10744ExpireReservesConflictHoldOverReport.pl new file mode 100644 index 0000000..89cf4e2 --- /dev/null +++ b/installer/data/mysql/atomicupdate/Bug10744ExpireReservesConflictHoldOverReport.pl @@ -0,0 +1,36 @@ +#!/usr/bin/perl + +# Copyright Open Source Freedom Fighters +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use C4::Context; +use Koha::AtomicUpdater; + +my $dbh = C4::Context->dbh(); +my $atomicUpdater = Koha::AtomicUpdater->new(); + +unless($atomicUpdater->find('Bug10744')) { + + $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')"); + + print "Upgrade done (Bug 10744 - ExpireReservesMaxPickUpDelay has minor workflow conflicts with hold(s) over report)\n"; +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 076473f..e9d836c 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1706,6 +1706,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, -- has this hold been pinned to the lowest priority in the holds queue (1 for yes, 0 for no) `suspend` BOOLEAN 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) @@ -1714,6 +1715,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`), CONSTRAINT `old_reserves_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL, CONSTRAINT `old_reserves_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) @@ -1940,6 +1942,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, `suspend` BOOLEAN NOT NULL DEFAULT 0, `suspend_until` DATETIME NULL DEFAULT NULL, @@ -1949,6 +1952,7 @@ CREATE TABLE `reserves` ( -- information related to holds/reserves in Koha KEY `biblionumber` (`biblionumber`), KEY `itemnumber` (`itemnumber`), KEY `branchcode` (`branchcode`), + KEY `reserves_pickupexpired` (`pickupexpired`), 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, CONSTRAINT `reserves_ibfk_3` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE CASCADE ON UPDATE CASCADE, diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index 00e070d..341cf8d 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -118,6 +118,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('EnhancedMessagingPreferences','0','','If ON, allows patrons to select to receive additional messages about items due or nearly due.','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'), ('ExtendedPatronAttributes','0',NULL,'Use extended patron IDs and attributes','YesNo'), ('FacetLabelTruncationLength','20',NULL,'Specify the facet max length in OPAC','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 a205e24..3619211 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 @@ -416,6 +416,10 @@ Circulation: - 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 from the libraries - pref: StaticHoldsQueueWeight class: multi diff --git a/t/db_dependent/Reserves/pickupExpiredHoldsOverReportDuration.t b/t/db_dependent/Reserves/pickupExpiredHoldsOverReportDuration.t new file mode 100644 index 0000000..d46dc58 --- /dev/null +++ b/t/db_dependent/Reserves/pickupExpiredHoldsOverReportDuration.t @@ -0,0 +1,121 @@ +#!/usr/bin/perl + +# Copyright 2015 KohaSuomi +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; +use Test::More; +use Try::Tiny; +use Scalar::Util qw(blessed); + +use t::lib::TestObjects::ObjectFactory; +use t::lib::TestObjects::HoldFactory; +use t::lib::TestObjects::SystemPreferenceFactory; + +##Setting up the test context +my $testContext = {}; + +t::lib::TestObjects::SystemPreferenceFactory->createTestGroup([{preference => 'PickupExpiredHoldsOverReportDuration', + value => 2, + }, + {preference => 'ExpireReservesMaxPickUpDelay', + value => 1, + }, + {preference => 'ReservesMaxPickUpDelay', + value => 6, + }, + ], undef, $testContext); + +my $holds = t::lib::TestObjects::HoldFactory->createTestGroup([ + {cardnumber => '1A01', + isbn => '987Kivi', + barcode => '1N01', + branchcode => 'CPL', + waitingdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 9)->iso8601(), + reservenotes => 'expire3daysAgo', + }, + {cardnumber => '1A01', + isbn => '987Kivi', + barcode => '1N02', + branchcode => 'CPL', + waitingdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 8)->iso8601(), + reservenotes => 'expire2daysAgo', + }, + {cardnumber => '1A02', + isbn => '987Kivi', + barcode => '1N03', + branchcode => 'CPL', + waitingdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 7)->iso8601(), + reservenotes => 'expire1dayAgo1', + }, + {cardnumber => '1A03', + isbn => '987Kivi', + barcode => '1N04', + branchcode => 'CPL', + waitingdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 7)->iso8601(), + reservenotes => 'expire1dayAgo2', + }, + {cardnumber => '1A04', + isbn => '987Kivi', + barcode => '1N05', + branchcode => 'CPL', + waitingdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 6)->iso8601(), + reservenotes => 'expiresToday', + }, + {cardnumber => '1A05', + isbn => '987Kivi', + barcode => '1N06', + branchcode => 'CPL', + waitingdate => DateTime->now(time_zone => C4::Context->tz())->subtract(days => 5)->iso8601(), + reservenotes => 'expiresTomorrow', + }, + ], undef, $testContext); + + + +##Test context set, starting testing: +subtest "Expiring holds and getting old_reserves" => \&expiringHoldsAndOld_reserves; +sub expiringHoldsAndOld_reserves { + eval { #run in a eval-block so we don't die without tearing down the test context + C4::Reserves::CancelExpiredReserves(); + my $expiredReserves = C4::Reserves::GetExpiredReserves({}); + ok($expiredReserves->[0]->{reserve_id} == $holds->{'expire2daysAgo'}->{reserve_id} && + $expiredReserves->[0]->{pickupexpired} eq DateTime->now(time_zone => C4::Context->tz())->subtract(days => 2)->ymd() + , "Hold for Item 1N02 expired yesterday"); + ok($expiredReserves->[1]->{reserve_id} == $holds->{'expire1dayAgo1'}->{reserve_id} && + $expiredReserves->[1]->{pickupexpired} eq DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->ymd() + , "Hold for Item 1N03 expired today"); + ok($expiredReserves->[2]->{reserve_id} == $holds->{'expire1dayAgo2'}->{reserve_id} && + $expiredReserves->[2]->{pickupexpired} eq DateTime->now(time_zone => C4::Context->tz())->subtract(days => 1)->ymd() + , "Hold for Item 1N04 expired today"); + is($expiredReserves->[3], undef, + "Holds for Items 1N05 and 1N06 not expired."); + }; + if ($@) { #Catch all leaking errors and gracefully terminate. + warn $@; + tearDown(); + exit 1; + } +} + +##All tests done, tear down test context +tearDown(); +done_testing; + +sub tearDown { + t::lib::TestObjects::ObjectFactory->tearDownTestContext($testContext); +} \ No newline at end of file -- 1.7.9.5