From e1cec51c8a5b866ba8cff6c510acd77a848749fa Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 18 Jun 2013 10:17:33 +0200 Subject: [PATCH] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay This patch removes the ReservesMaxPickUpDelay syspref. The update database script will update the issuingrules table with the correct value before removing it. 1 new column: reserves.maxpickupdate (and for old_reserves). It contains the waitingdate + the corresponding max pickup delay. Each time the waitingdate is modified, this value will be modified too. To test: 1/ Play with the reserve workflow (Add, remove, change priority, etc.) and check there are no regression. 2/ Update the new column 'Holds pickup delay' in your issuing rules. According to a library and an item type, the maxpickupdate value will be equal to the waiting date + the pickup delay defined. Signed-off-by: Kyle M Hall --- C4/Letters.pm | 3 +- C4/Reserves.pm | 136 ++++++++++++-------- circ/circulation.pl | 5 +- circ/waitingreserves.pl | 5 +- installer/data/mysql/kohastructure.sql | 2 + installer/data/mysql/updatedatabase.pl | 24 +++- .../en/modules/admin/preferences/circulation.pref | 7 +- .../prog/en/modules/help/circ/waitingreserves.tt | 4 +- .../prog/en/modules/members/moremember.tt | 2 +- members/moremember.pl | 2 +- .../thirdparty/TalkingTech_itiva_outbound.pl | 5 +- opac/opac-user.pl | 1 - 12 files changed, 115 insertions(+), 81 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index 0349efe..28bdeb0 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -610,8 +610,7 @@ sub _parseletter { if ( $table eq 'reserves' && $values->{'waitingdate'} ) { my @waitingdate = split /-/, $values->{'waitingdate'}; - my $dt = dt_from_string(); - $dt->add( days => C4::Context->preference('ReservesMaxPickUpDelay') ); + my $dt = dt_from_string($values->{maxpickupdate}); $values->{'expirationdate'} = output_pref( $dt, undef, 1 ); $values->{'waitingdate'} = output_pref( dt_from_string( $values->{'waitingdate'} ), undef, 1 ); diff --git a/C4/Reserves.pm b/C4/Reserves.pm index f86e380..b91044e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -131,8 +131,6 @@ BEGIN { &ReserveSlip &ToggleSuspend &SuspendAll - &GetMaxPickupDelay - &GetMaxPickupDate &GetReservesControlBranch ); @EXPORT_OK = qw( MergeHolds ); @@ -173,35 +171,41 @@ sub AddReserve { $waitingdate = $resdate; } + my $maxpickupdate = GetMaxPickupDate({ + waitingdate => $waitingdate, + itemnumber => $checkitem, + borrowernumber => $borrowernumber + }); + #eval { # updates take place here if ( $fee > 0 ) { my $nextacctno = &getnextacctno( $borrowernumber ); - my $query = qq/ + my $query = q{ INSERT INTO accountlines (borrowernumber,accountno,date,amount,description,accounttype,amountoutstanding) VALUES (?,?,now(),?,?,'Res',?) - /; + }; my $usth = $dbh->prepare($query); $usth->execute( $borrowernumber, $nextacctno, $fee, "Reserve Charge - $title", $fee ); } #if ($const eq 'a'){ - my $query = qq/ + my $query = q{ INSERT INTO reserves (borrowernumber,biblionumber,reservedate,branchcode,constrainttype, - priority,reservenotes,itemnumber,found,waitingdate,expirationdate) + priority,reservenotes,itemnumber,found,waitingdate,expirationdate,maxpickupdate) VALUES (?,?,?,?,?, - ?,?,?,?,?,?) - /; + ?,?,?,?,?,?,?) + }; my $sth = $dbh->prepare($query); $sth->execute( $borrowernumber, $biblionumber, $resdate, $branch, $const, $priority, $notes, $checkitem, - $found, $waitingdate, $expdate + $found, $waitingdate, $expdate, $maxpickupdate ); # Send e-mail to librarian if syspref is active @@ -502,58 +506,55 @@ sub CanItemBeReserved{ } } -=item GetMaxPickupDelay +=head2 GetMaxPickupDate -$resallowed = &GetMaxPickupDelay($borrowernumber, $item) +$maxpickupdate = &GetMaxPickupDate($reserve [, $item]); +$reserve->{waitingdate} is a string or a DateTime. -this function return the max delay for an hold +this function returns the max pickup date (DateTime format). +(e.g. : the date after which the hold will be considered cancelled) =cut -sub GetMaxPickupDelay { - my ( $borrowernumber, $item ) = @_; - - my $dbh = C4::Context->dbh; - my $allowedreserves = 0; - - my $borrower = C4::Members::GetMember( 'borrowernumber' => $borrowernumber ); - - my $controlbranch = GetReservesControlBranch( $borrower, $item ); +sub GetMaxPickupDate { + my ( $reserve, $item ) = @_; - my $issuingrule = C4::Circulation::GetIssuingRule( $borrower->{categorycode}, $item->{itype}, $controlbranch ); + if ( not defined $reserve and not defined $item->{itemnumber} ) { + warn "ERROR: GetMaxPickupDate is called without reserve and without itemnumber"; + return; + } - return $issuingrule->{holdspickupdelay} - if defined($issuingrule) - and defined $issuingrule->{holdspickupdelay}; + if ( defined $reserve and not defined $item ) { + $item = C4::Items::GetItem( $reserve->{itemnumber} ); + } - # if nothing found, return the syspref value - return C4::Context->preference("ReservesMaxPickUpDelay") || 0; -} + unless ( defined $reserve ) { + my $reserve = GetReservesFromItemnumber( $item->{itemnumber} ); + } + return unless $reserve->{waitingdate}; -=item GetMaxPickupDate + my $borrower = C4::Members::GetMember( 'borrowernumber' => $reserve->{borrowernumber} ); -$maxpickupdate = &GetMaxPickupDate($item, $borrowernumber); -$item->{waitingdate} is a string or a DateTime. + my $controlbranch = GetReservesControlBranch( $borrower, $item ); -this function returns the max pickup date (DateTime format). -(e.g. : the date after which the hold will be considered cancelled) + my $issuingrule = C4::Circulation::GetIssuingRule( $borrower->{categorycode}, $item->{itype}, $controlbranch ); -=cut + my $date = ref $reserve->{waitingdate} eq 'DateTime' + ? $reserve->{waitingdate} + : dt_from_string $reserve->{waitingdate}; -sub GetMaxPickupDate { - my ( $item, $borrowernumber ) = @_; - return unless $item->{waitingdate}; + my $holdspickupdelay = 0; + if ( defined($issuingrule) + and defined $issuingrule->{holdspickupdelay} ) { + $holdspickupdelay = $issuingrule->{holdspickupdelay} + } - my $date = ref $item->{waitingdate} eq 'DateTime' - ? $item->{waitingdate} - : dt_from_string $item->{waitingdate}; + $date->add( days => $holdspickupdelay ); - my $delay = GetMaxPickupDelay( $borrowernumber, $item ); - $date->add( days => $delay ); return $date; } -=item GetReservesControlBranch +=head2 GetReservesControlBranch $branchcode = &GetReservesControlBranch($borrower, $item) @@ -788,7 +789,7 @@ sub GetReservesToBranch { sub GetReservesForBranch { my ($frombranch) = @_; my $dbh = C4::Context->dbh; - my $query = "SELECT borrowernumber,reservedate,itemnumber,waitingdate + my $query = "SELECT borrowernumber,reservedate,itemnumber,waitingdate,maxpickupdate FROM reserves WHERE priority='0' AND found='W' "; @@ -985,15 +986,12 @@ sub CancelExpiredReserves { } # Cancel reserves that have been waiting too long - # FIXME The only way I see to do this job with the issuingrules.holdspickupdelay value would be to - # get all reserves and to get the related borrower and item. So we could call GetMaxPickupDelay for each reserve. if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) { - my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); - my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS( waitingdate ) > ? AND found = 'W' AND priority = 0"; + my $query = "SELECT * FROM reserves WHERE NOW() > maxpickupdate AND found = 'W' AND priority = 0"; $sth = $dbh->prepare( $query ); - $sth->execute( $max_pickup_delay ); + $sth->execute(); while (my $res = $sth->fetchrow_hashref ) { if ( $charge ) { @@ -1300,9 +1298,28 @@ sub ModReserveStatus { my ($itemnumber, $newstatus) = @_; my $dbh = C4::Context->dbh; - my $query = "UPDATE reserves SET found = ?, waitingdate = NOW() WHERE itemnumber = ? AND found IS NULL AND priority = 0"; + my $now = dt_from_string; + my $reserve = $dbh->selectrow_hashref(q{ + SELECT * + FROM reserves + WHERE itemnumber = ? + AND found IS NULL + AND priority = 0 + }, {}, $itemnumber); + return unless $reserve; + + my $maxpickupdate = GetMaxPickupDate( $reserve ); + my $query = q{ + UPDATE reserves + SET found = ?, + waitingdate = ?, + maxpickupdate = ? + WHERE itemnumber = ? + AND found IS NULL + AND priority = 0 + }; my $sth_set = $dbh->prepare($query); - $sth_set->execute( $newstatus, $itemnumber ); + $sth_set->execute( $newstatus, $now, $maxpickupdate, $itemnumber ); if ( C4::Context->preference("ReturnToShelvingCart") && $newstatus ) { CartToShelf( $itemnumber ); @@ -1350,21 +1367,29 @@ sub ModReserveAffect { WHERE borrowernumber = ? AND biblionumber = ? "; + $sth = $dbh->prepare($query); + $sth->execute( $itemnumber, $borrowernumber,$biblionumber); } else { # affect the reserve to Waiting as well. + my $maxpickupdate = GetMaxPickupDate({ + waitingdate => dt_from_string, + itemnumber => $itemnumber, + borrowernumber => $borrowernumber + }); $query = " UPDATE reserves SET priority = 0, found = 'W', waitingdate = NOW(), + maxpickupdate = ?, itemnumber = ? WHERE borrowernumber = ? AND biblionumber = ? "; + $sth = $dbh->prepare($query); + $sth->execute( $maxpickupdate, $itemnumber, $borrowernumber,$biblionumber); } - $sth = $dbh->prepare($query); - $sth->execute( $itemnumber, $borrowernumber,$biblionumber); _koha_notify_reserve( $itemnumber, $borrowernumber, $biblionumber ) if ( !$transferToDo && !$already_on_shelf ); if ( C4::Context->preference("ReturnToShelvingCart") ) { @@ -1440,6 +1465,7 @@ sub GetReserveInfo { reserves.biblionumber, reserves.branchcode, reserves.waitingdate, + reserves.maxpickupdate, notificationdate, reminderdate, priority, @@ -1894,6 +1920,7 @@ sub _Findgroupreserve { reserves.borrowernumber AS borrowernumber, reserves.reservedate AS reservedate, reserves.waitingdate AS waitingdate, + reserves.maxpickupdate AS maxpickupdate, reserves.branchcode AS branchcode, reserves.cancellationdate AS cancellationdate, reserves.found AS found, @@ -2209,7 +2236,8 @@ sub RevertWaitingStatus { SET priority = 1, found = NULL, - waitingdate = NULL + waitingdate = NULL, + maxpickupdate = NULL, WHERE reserve_id = ? "; diff --git a/circ/circulation.pl b/circ/circulation.pl index 65b379c..de656dc 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -373,9 +373,8 @@ if ($borrowernumber) { if ( $num_res->{'found'} && $num_res->{'found'} eq 'W' ) { $getreserv{color} = 'reserved'; $getreserv{waiting} = 1; - my $maxpickupdate = GetMaxPickupDate( $num_res, $borrowernumber ); - $getWaitingReserveInfo{maxpickupdate} = $maxpickupdate; - $getreserv{maxpickupdate} = $maxpickupdate; + $getWaitingReserveInfo{maxpickupdate} = $num_res->{maxpickupdate}; + $getreserv{maxpickupdate} = $num_res->{maxpickupdate}; # genarate information displaying only waiting reserves $getWaitingReserveInfo{title} = $getiteminfo->{'title'}; diff --git a/circ/waitingreserves.pl b/circ/waitingreserves.pl index 7a35d39..cb4d7b3 100755 --- a/circ/waitingreserves.pl +++ b/circ/waitingreserves.pl @@ -104,9 +104,9 @@ foreach my $num (@getreserves) { my $itemtypeinfo = getitemtypeinfo( $gettitle->{'itemtype'} ); # using the fixed up itype/itemtype if ( $num->{waitingdate} ) { - my $maxpickupdate = GetMaxPickupDate( $num, $borrowernumber ); + my $maxpickupdate = dt_from_string($num->{maxpickupdate}); $getreserv{waitingdate} = $num->{waitingdate}; - $getreserv{maxpickupdate} = $maxpickupdate; + $getreserv{maxpickupdate} = $num->{maxpickupdate}; if ( DateTime->compare( $today, $maxpickupdate ) == 1 ) { if ($cancelall) { my $res = cancel( $itemnumber, $borrowernumber, $holdingbranch, $homebranch, !$transfer_when_cancel_all ); @@ -150,7 +150,6 @@ $template->param( overloop => \@overloop, overcount => $overcount, show_date => format_date(C4::Dates->today('iso')), - dateformat => C4::Context->preference("dateformat"), ); if ($cancelall) { diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 06f7c29..a6fe3f7 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1591,6 +1591,7 @@ CREATE TABLE `old_reserves` ( -- this table holds all holds/reserves that have b `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) + `maxpickupdate` date NULL DEFAULT NULL, -- the max pickup date for this reserves PRIMARY KEY (`reserve_id`), KEY `old_reserves_borrowernumber` (`borrowernumber`), KEY `old_reserves_biblionumber` (`biblionumber`), @@ -1790,6 +1791,7 @@ CREATE TABLE `reserves` ( -- information related to holds/reserves in Koha `lowestPriority` tinyint(1) NOT NULL, `suspend` BOOLEAN NOT NULL DEFAULT 0, `suspend_until` DATETIME NULL DEFAULT NULL, + `maxpickupdate` date NULL DEFAULT NULL, -- the max pickup date for this reserves PRIMARY KEY (`reserve_id`), KEY priorityfoundidx (priority,found), KEY `borrowernumber` (`borrowernumber`), diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 6800efb..5978d18 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -7011,19 +7011,33 @@ CREATE TABLE IF NOT EXISTS borrower_files ( } - - - $DBversion = "3.11.00.XXX"; if ( CheckVersion($DBversion) ) { + my $maxpickupdelay = C4::Context->preference('ReservesMaxPickUpDelay') || 0; + $dbh->do(q{ + DELETE FROM systempreferences WHERE variable='ReservesMaxPickUpDelay'; + }); $dbh->do(qq{ ALTER TABLE issuingrules ADD COLUMN holdspickupdelay INT(11) NULL default NULL AFTER reservesallowed; }); - print "Upgrade to $DBversion done (8367: Add colum issuingrules.holdspickupdelay)\n"; + my $sth = $dbh->prepare(q{ + UPDATE issuingrules SET holdspickupdelay = ? + }); + $sth->execute( $maxpickupdelay ); + $dbh->do(q{ + ALTER TABLE reserves ADD COLUMN maxpickupdate DATE NULL default NULL AFTER suspend_until; + }); + $sth = $dbh->prepare(q{ + UPDATE reserves SET maxpickupdate = ADDDATE(waitingdate, INTERVAL ? DAY); + }); + $sth->execute( $maxpickupdelay ); + $dbh->do(q{ + ALTER TABLE old_reserves ADD COLUMN maxpickupdate DATE NULL default NULL AFTER suspend_until; + }); + print "Upgrade to $DBversion done (8367: Add colum issuingrules.holdspickupdelay and reserves.maxpickupdate. Delete the ReservesMaxPickUpDelay syspref)\n"; SetVersion($DBversion); } - =head1 FUNCTIONS =head2 TableExists($table) 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 64cf338..8b589c5 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 @@ -365,16 +365,11 @@ Circulation: PatronLibrary: "patron's home library" - to see if the patron can place a hold on the item. - - - Mark a hold as problematic if it has been waiting for more than - - pref: ReservesMaxPickUpDelay - class: integer - - days. - - - pref: ExpireReservesMaxPickUpDelay choices: yes: Allow no: "Don't allow" - - "holds to expire automatically if they have not been picked by within the time period specified in ReservesMaxPickUpDelay" + - "holds to expire automatically if they have not been picked by within the time period specified in hold pickup delay defined in the issuing rules" - - If using ExpireReservesMaxPickUpDelay, charge a borrower who allows his or her waiting hold to expire a fee of - pref: ExpireReservesMaxPickUpDelayCharge diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/help/circ/waitingreserves.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/help/circ/waitingreserves.tt index be0e509..8ce7b9c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/help/circ/waitingreserves.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/help/circ/waitingreserves.tt @@ -4,8 +4,8 @@

This report will show all of the holds that are waiting for patrons to pick them up.

-

Items that have been on the hold shelf longer than you normally allow (based on the ReservesMaxPickUpDelay preference value) will appear on the 'Holds Over' tab, they will not automatically be cancelled unless you have set the cron job to do that for you, but you can cancel all holds using the button at the top of the list.

+

Items that have been on the hold shelf longer than you normally allow (based on the holds pickup delay defined in the issuing rules) will appear on the 'Holds Over' tab, they will not automatically be cancelled.

See the full documentation for Holds Awaiting Pickup in the manual (online).

-[% INCLUDE 'help-bottom.inc' %] \ No newline at end of file +[% INCLUDE 'help-bottom.inc' %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt index 3b1de36..05f9570 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ -637,7 +637,7 @@ function validate1(date) { [% reservloo.itemcallnumber %] [% IF ( reservloo.waiting ) %] - Item is waiting until [% reservloo.maxpickupdate %] + Item is waiting[% IF reservloo.maxpickupdate %] until [% reservloo.maxpickupdate %][% END %] [% END %] [% IF ( reservloo.transfered ) %] Item in transit from diff --git a/members/moremember.pl b/members/moremember.pl index 79493e0..1310b03 100755 --- a/members/moremember.pl +++ b/members/moremember.pl @@ -285,7 +285,7 @@ if ($borrowernumber) { if ( $num_res->{'found'} eq 'W' ) { $getreserv{color} = 'reserved'; $getreserv{waiting} = 1; - $getreserv{maxpickupdate} = GetMaxPickupDate( $num_res, $borrowernumber ); + $getreserv{maxpickupdate} = $num_res->{maxpickupdate}; } # check transfers with the itemnumber foud in th reservation loop diff --git a/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl b/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl index b4d4e9c..a052e31 100755 --- a/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl +++ b/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl @@ -284,7 +284,7 @@ sub GetPredueIssues { sub GetWaitingHolds { my $query = "SELECT borrowers.borrowernumber, borrowers.cardnumber, borrowers.title as patron_title, borrowers.firstname, borrowers.surname, - borrowers.phone, borrowers.email, borrowers.branchcode, biblio.biblionumber, biblio.title, items.barcode, reserves.waitingdate, + borrowers.phone, borrowers.email, borrowers.branchcode, biblio.biblionumber, biblio.title, items.barcode, reserves.waitingdate,reserves.maxpickupdate, reserves.branchcode AS site, branches.branchname AS site_name, TO_DAYS(NOW())-TO_DAYS(reserves.waitingdate) AS days_since_waiting FROM borrowers JOIN reserves USING (borrowernumber) @@ -298,7 +298,6 @@ sub GetWaitingHolds { AND message_transport_type = 'phone' AND message_name = 'Hold_Filled' "; - my $pickupdelay = C4::Context->preference("ReservesMaxPickUpDelay"); my $sth = $dbh->prepare($query); $sth->execute(); my @results; @@ -306,7 +305,7 @@ sub GetWaitingHolds { my @waitingdate = split( /-/, $issue->{'waitingdate'} ); my @date_due = Add_Delta_Days( $waitingdate[0], $waitingdate[1], $waitingdate[2], - $pickupdelay ); + $issue->{maxpickupdate} ); $issue->{'date_due'} = sprintf( "%04d-%02d-%02d", $date_due[0], $date_due[1], $date_due[2] ); $issue->{'level'} = 1; # only one level for Hold Waiting notifications diff --git a/opac/opac-user.pl b/opac/opac-user.pl index af770e8..6fb2f7b 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -261,7 +261,6 @@ foreach my $res (@reserves) { $res->{$publictype} = 1; if ( $res->{found} eq 'W' ) { $res->{waiting} = 1; - $res->{maxpickupdate} = GetMaxPickupDate( $res, $borrowernumber ); } $res->{'branch'} = $branches->{ $res->{'branchcode'} }->{'branchname'}; my $biblioData = GetBiblioData($res->{'biblionumber'}); -- 1.7.10.4