From 6ce7de432831291382f3028f0e637d65c16d344f Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 1 Mar 2018 15:46:22 +0000 Subject: [PATCH] Bug 19532 - Improve and refine recalls functionality Only allow 1 recall per item When checking in only check for holds if no reserves found Add a T/transferring status for recalls, only confirm them once arrived Improve message when checking in recall Fix clashing ids on recall-history Check if patron can hold item before allowing recall Prevent multiple recalls when pressing back button Select/fill only active recall when issuing To test: 1 - Repeat major test plan - verify all points 2 - Attempt to place multiple recallsusing back button, should fail 3 - Set all item level holds to not allowed, should not be able to place recall 4 - Check-in recall at branch other than destination, should be in transit 5 - Confirm at correct branch, should send message and get expiration date etc. 6 - Return to another branch, should eb in transit again 7 - Play with circ rules and verify only allowed patrons can place recall Signed-off-by: Aleisha Amohia --- C4/Circulation.pm | 43 ++++++++------- catalogue/detail.pl | 2 +- circ/returns.pl | 61 ++++++++++++---------- .../atomicupdate/bug_19532_-_add_recalls_table.sql | 2 +- installer/data/mysql/kohastructure.sql | 2 +- .../prog/en/modules/circ/recalls_waiting.tt | 6 +-- .../intranet-tmpl/prog/en/modules/circ/returns.tt | 13 +++-- .../opac-tmpl/bootstrap/en/modules/opac-recall.tt | 6 ++- .../opac-tmpl/bootstrap/en/modules/opac-recalls.tt | 4 +- .../opac-tmpl/bootstrap/en/modules/opac-user.tt | 4 +- opac/opac-detail.pl | 6 ++- opac/opac-recall.pl | 25 ++++++--- opac/opac-user.pl | 2 +- 13 files changed, 104 insertions(+), 72 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 4362159880..b9f4cd1c6c 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -346,19 +346,8 @@ sub transferbook { $messages->{'WasReturned'} = $issue->borrowernumber; } - # find reserves..... - # That'll save a database query. - my ( $resfound, $resrec, undef ) = - CheckReserves( $itemnumber ); - if ( $resfound and not $ignoreRs ) { - $resrec->{'ResFound'} = $resfound; - - # $messages->{'ResFound'} = $resrec; - $dotransfer = 1; - } - # check if item has been recalled. do a transfer if the recall branch is different to the item holding branch, otherwise don't do a transfer - my $recall = Koha::Recalls->find({ itemnumber => $item->itemnumber, status => 'R' }); + my $recall = Koha::Recalls->find({ itemnumber => $item->itemnumber, old => undef }); if (defined $recall){ if ($recall->branchcode eq $fbr){ $dotransfer = 0; @@ -366,6 +355,15 @@ sub transferbook { } else { $dotransfer = 1; } + } else { #recalls take priority over holds, only check holds if no recalls + # find reserves..... + my ( $resfound, $resrec, undef ) = + CheckReserves( $itemnumber ); + if ( $resfound and not $ignoreRs ) { + $resrec->{'ResFound'} = $resfound; +# $messages->{'ResFound'} = $resrec; + $dotransfer = 1; + } } #actually do the transfer.... @@ -1380,7 +1378,7 @@ sub AddIssue { # Check if a recall if ( C4::Context->preference('UseRecalls') ) { - my $recall = Koha::Recalls->find({ itemnumber => $item->{'itemnumber'}, borrowernumber => $borrower->{'borrowernumber'} }); + my $recall = Koha::Recalls->find({ itemnumber => $item->{'itemnumber'}, borrowernumber => $borrower->{'borrowernumber'}, old => undef }); if (defined $recall) { $recall->update({ status => "F", old => 1 }); } @@ -2034,21 +2032,22 @@ sub AddReturn { } } - # find reserves..... - # if we don't have a reserve with the status W, we launch the Checkreserves routine my ($resfound, $resrec); - my $lookahead= C4::Context->preference('ConfirmFutureHolds'); #number of days to look for future holds - ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->{'itemnumber'}, undef, $lookahead ) unless ( $item->{'withdrawn'} ); - if ($resfound) { - $resrec->{'ResFound'} = $resfound; - $messages->{'ResFound'} = $resrec; - } # find recalls..... - my $recall = Koha::Recalls->find({ itemnumber => $item->{'itemnumber'} }); + my $recall = Koha::Recalls->find({ itemnumber => $item->{'itemnumber'}, old => undef }); if (defined $recall){ $messages->{'RecallFound'} = 1; $messages->{'Recall'} = $recall; + } else { + # find reserves..... + # if we don't have a reserve with the status W, we launch the Checkreserves routine + my $lookahead= C4::Context->preference('ConfirmFutureHolds'); #number of days to look for future holds + ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->{'itemnumber'}, undef, $lookahead ) unless ( $item->{'withdrawn'} ); + if ($resfound) { + $resrec->{'ResFound'} = $resfound; + $messages->{'ResFound'} = $resrec; + } } # Record the fact that this book was returned. diff --git a/catalogue/detail.pl b/catalogue/detail.pl index be601bf874..deae6b47a4 100755 --- a/catalogue/detail.pl +++ b/catalogue/detail.pl @@ -334,7 +334,7 @@ foreach my $item (@items) { } } - my $recall = Koha::Recalls->find({ itemnumber => $item->{itemnumber} }); + my $recall = Koha::Recalls->find({ itemnumber => $item->{itemnumber}, old => undef }); if (defined $recall){ $item->{recalled} = 1; $item->{recall} = $recall; diff --git a/circ/returns.pl b/circ/returns.pl index 3d9cecd0a5..dcf9785ced 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -190,29 +190,35 @@ if ( $query->param('reserve_id') ) { } if ( $query->param('recall_id') ){ - my $recall = Koha::Recalls->find(scalar $query->param('recall_id')); + my $recall = Koha::Recalls->find( $query->param('recall_id') ); my $recall_borrower = $recall->patron; my $item = Koha::Items->find($recall->itemnumber); my $biblio = Koha::Biblios->find($item->biblionumber); - - my $issuing_rule = Koha::IssuingRules->get_effective_issuing_rule({ categorycode => $recall_borrower->categorycode, itemtype => $item->itype, branchcode => $item->holdingbranch }); - my $shelf_time = $issuing_rule->recall_shelf_time || C4::Context->preference('RecallsMaxPickUpDelay'); - my $expirationdate = dt_from_string()->add( $issuing_rule->lengthunit => $shelf_time ); - $recall->update({ status => 'W', waitingdate => dt_from_string(), expirationdate => $expirationdate }); - - # send notice to user who requested recall to pick up item - my $letter = C4::Letters::GetPreparedLetter ( - module => 'circulation', - letter_code => 'PICKUP_RECALLED_ITEM', - branchcode => $recall->branchcode, - tables => { - 'biblio', $biblio->biblionumber, - 'borrowers', $recall_borrower->borrowernumber, - 'items', $item->itemnumber, - 'recalls', $recall->recall_id, - }, - ); - C4::Message->enqueue($letter, $recall_borrower->unblessed, 'email'); + if( $query->param("transfer_recall") ){ + $recall->update({ status => 'T', waitingdate => undef, expirationdate => undef }); + } else { + my $issuing_rule = Koha::IssuingRules->get_effective_issuing_rule({ categorycode => $recall_borrower->categorycode, itemtype => $item->itype, branchcode => $item->holdingbranch }); + my $shelf_time = $issuing_rule->recall_shelf_time || C4::Context->preference('RecallsMaxPickUpDelay'); + my $expirationdate = dt_from_string()->add( $issuing_rule->lengthunit => $shelf_time ); + $recall->update({ status => 'W', waitingdate => dt_from_string(), expirationdate => $expirationdate }); + + # send notice to user who requested recall to pick up item + my $letter = C4::Letters::GetPreparedLetter ( + module => 'circulation', + letter_code => 'PICKUP_RECALLED_ITEM', + branchcode => $recall->branchcode, + tables => { + 'biblio', + $biblio->biblionumber, + 'borrowers', + $recall_borrower->borrowernumber, + 'items', $item->itemnumber, + 'recalls', + $recall->recall_id, + }, + ); + C4::Message->enqueue($letter, $recall_borrower->unblessed, 'email'); + } } my $borrower; @@ -513,13 +519,14 @@ if ( $messages->{'RecallFound'} ){ found => 1, recalled => 1, recall => $recall, - address => $patron->address, - address2 => $patron->address2, - streetnumber => $patron->streetnumber, - city => $patron->city, - zipcode => $patron->zipcode, - state => $patron->state, - country => $patron->country, + transfer_recall => ($userenv_branch eq $recall->branchcode ? 1 : 0 ) +# address => $patron->address, +# address2 => $patron->address2, +# streetnumber => $patron->streetnumber, +# city => $patron->city, +# zipcode => $patron->zipcode, +# state => $patron->state, +# country => $patron->country, ); } diff --git a/installer/data/mysql/atomicupdate/bug_19532_-_add_recalls_table.sql b/installer/data/mysql/atomicupdate/bug_19532_-_add_recalls_table.sql index 29730e4051..d37e891d97 100644 --- a/installer/data/mysql/atomicupdate/bug_19532_-_add_recalls_table.sql +++ b/installer/data/mysql/atomicupdate/bug_19532_-_add_recalls_table.sql @@ -9,7 +9,7 @@ CREATE TABLE `recalls` ( -- information related to recalls in Koha `cancellationdate` date default NULL, -- the date this recall was cancelled `recallnotes` mediumtext, -- notes related to this recall `priority` smallint(6) default NULL, -- where in the queue the patron sits - `status` varchar(1) default NULL, -- a one letter code defining what the status is of the recall is after it has been requested. R=requested, O=overdue, W=awaiting pickup, E=expired, C=cancelled, F=finished/completed + `status` varchar(1) default NULL, -- a one letter code defining what the status is of the recall is after it has been requested. R=requested, O=overdue, W=awaiting pickup, E=expired, C=cancelled, F=finished/completed, T=transferring `timestamp` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP, -- the date and time this recall was last updated `itemnumber` int(11) default NULL, -- foreign key from the items table defining the specific item the recall request was placed on `waitingdate` date default NULL, -- the date the item was marked as waiting for the patron at the library diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index a1b178b8cd..f3da1e4630 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1871,7 +1871,7 @@ CREATE TABLE `recalls` ( -- information related to recalls in Koha `cancellationdate` date default NULL, -- the date this recall was cancelled `recallnotes` mediumtext, -- notes related to this recall `priority` smallint(6) default NULL, -- where in the queue the patron sits - `found` varchar(1) default NULL, -- a one letter code defining what the status is of the recall is after it has been requested. R=requested, O=overdue, W=awaiting pickup, E=expired, F=finished/completed + `found` varchar(1) default NULL, -- a one letter code defining what the status is of the recall is after it has been requested. R=requested, O=overdue, W=awaiting pickup, T=transferring, E=expired, F=finished/completed `timestamp` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP, -- the date and time this recall was last updated `itemnumber` int(11) default NULL, -- foreign key from the items table defining the specific item the recall request was placed on `waitingdate` date default NULL, -- the date the item was marked as waiting for the patron at the library diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/recalls_waiting.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/recalls_waiting.tt index 8c700d6933..0ab3a6eb6d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/recalls_waiting.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/recalls_waiting.tt @@ -45,8 +45,8 @@
@@ -97,7 +97,7 @@
- [% IF ( over.count ) %] + [% IF ( over.size > 0 ) %] [% IF ( Koha.Preference('RecallsMaxPickUpDelay') ) %]

Recalls listed here have been awaiting pickup for more than [% Koha.Preference('RecallsMaxPickUpDelay') %] days.

[% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt index 3ab1a55650..d9ffc24faf 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ -515,7 +515,7 @@
-

Recall found:
+

Recall found [% IF transfer_recall %] needing transfer to [% Branches.GetName( recall.branchcode ) %][% END %]:
[% itembarcode |html %]: [% title |html %]

@@ -549,18 +549,21 @@
  • Patron's address is in doubt
    • [% END %] -

    Wait for pickup at [% recall.library.branchname %]

    - +

    + [% IF transfer_recall %]Transfer to [% Branches.GetName( recall.branchcode ) %] + [% ELSIF recall.status == 'W' %]Hold for pickup (already waiting) at [% recall.library.branchname %] + [% ELSE %]Hold for pickup at [% recall.library.branchname %][% END %] +

    - +
    diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt index f1ed15f694..db76aeb099 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt @@ -848,10 +848,12 @@ Requested [% ELSIF ( RECALL.is_waiting ) %] Ready for pickup + [% ELSIF ( RECALL.status == 'T' ) %] + In-transit to pickup library [% END %]
    @@ -115,7 +117,7 @@ - + [% END %] - [% IF ( !RECALL.cancellationdate && ( RECALL.is_requested || RECALL.is_overdue ) ) %] + [% IF ( !RECALL.cancellationdate && ( RECALL.is_requested || RECALL.is_overdue || RECALL.status == 'T' ) ) %]
    diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index c7f1743b3e..d9c4312be5 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -709,7 +709,11 @@ if ( not $viewallitems and @items > $max_items_to_display ) { if grep /^$itm->{itemnumber}$/, @itemnumbers_on_order; } - if ( $itm->{datedue} && $borrowernumber && $itm->{borrowernumber} != $borrowernumber && C4::Context->preference('UseRecalls') ) { + my $recall = Koha::Recalls->find({ itemnumber => $itm->{itemnumber}, old => undef }); + if ( !$recall && $itm->{datedue} && $borrowernumber && $itm->{borrowernumber} != $borrowernumber + && C4::Context->preference('UseRecalls') + && Koha::IssuingRules->get_opacitemholds_policy({ item => $item, patron => $patron }) ne 'N' ) + { $itm->{avail_for_recall} = 1; } diff --git a/opac/opac-recall.pl b/opac/opac-recall.pl index ff0a19144c..ce3811377a 100755 --- a/opac/opac-recall.pl +++ b/opac/opac-recall.pl @@ -1,4 +1,5 @@ #!/usr/bin/perl +#pinio # This file is part of Koha. # @@ -46,15 +47,25 @@ my $op = $query->param('op') || ''; my $itemnumber = $query->param('itemnumber'); my $item = Koha::Items->find($itemnumber); my $biblio = Koha::Biblios->find($item->biblionumber); -my $recalled = Koha::Recalls->find({ itemnumber => $itemnumber }); +my $patron = Koha::Patrons->find($borrowernumber); +my $recalled = Koha::Recalls->find({ itemnumber => $itemnumber, old => undef, borrowernumber => $borrowernumber }); my $error; -if ( defined $recalled && $recalled->borrowernumber == $borrowernumber && ($recalled->is_requested || $recalled->is_waiting || $recalled->is_overdue) ){ +if ( defined $recalled ){ # can't place a recall on an item that this borrower has already recalled # if recall is expired or cancelled, user may recall again $error = 'duplicate'; } +my $checkout = Koha::Checkouts->find({ itemnumber => $itemnumber }); +unless ( defined $checkout ){ + # can't recall an item not checked out + # checking in case they force the itemnumber + $error = 'not_issued'; +} + +$error = 'not_allowed' unless Koha::IssuingRules->get_opacitemholds_policy({ item => $item, patron => $patron }) ne 'N'; + if ($op eq 'request'){ if (!defined $borrowernumber){ # can't place recall if not logged in @@ -62,9 +73,8 @@ if ($op eq 'request'){ print $query->redirect('/cgi-bin/koha/opac-detail.pl?biblionumber='.$item->biblionumber); } elsif ( !defined $error ){ # can recall - my $borrower = Koha::Patrons->find($borrowernumber); - my $issuing_rule = Koha::IssuingRules->get_effective_issuing_rule({ categorycode => $borrower->categorycode, itemtype => $item->itype, branchcode => $item->holdingbranch }); - my $recall_request = Koha::Recall->new({ + my $issuing_rule = Koha::IssuingRules->get_effective_issuing_rule({ categorycode => $patron->categorycode, itemtype => $item->itype, branchcode => $item->holdingbranch }); + my $recall_request = Koha::Recall->new({ borrowernumber => $borrowernumber, recalldate => dt_from_string(), biblionumber => $biblio->biblionumber, @@ -78,8 +88,9 @@ if ($op eq 'request'){ # updating due date on checkout my $timestamp = dt_from_string($recall->timestamp); my $due_date = $timestamp->add( $issuing_rule->lengthunit => $issuing_rule->recall_due_date_interval ); - my $checkout = Koha::Checkouts->find({ itemnumber => $itemnumber })->update({ date_due => $due_date }); - my $checkout_borrower = Koha::Patrons->find($checkout->borrowernumber->borrowernumber); + + $checkout->update({ date_due => $due_date }); + my $checkout_borrower = $checkout->patron; # send notice to user with recalled item checked out my $letter = C4::Letters::GetPreparedLetter ( diff --git a/opac/opac-user.pl b/opac/opac-user.pl index 2b65857016..9c86e9c01f 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -305,7 +305,7 @@ $template->param( show_barcode => 1 ) if $show_barcode; # now the reserved items.... my $reserves = Koha::Holds->search( { borrowernumber => $borrowernumber } ); -my $recalls = Koha::Recalls->search( { borrowernumber => $borrowernumber, status => { '=', ['R', 'W'] } } ); +my $recalls = Koha::Recalls->search( { borrowernumber => $borrowernumber, old => undef } ); $template->param( RESERVES => $reserves, -- 2.11.0