Bug 14464: Add ability to cancel waiting holds from checkin screen

Test plan: 1. Ensure that ExpireReservesMaxPickUpDelayCharge is set to 0. 2. Place a hold (doesn't matter whether it's a bib/item-level hold), then confirm the hold by checking it in. 3. Check in the item again, and hit Cancel. 4. The reserve in question should be cancelled. 5. Repeat steps 2-4 twice, once after setting ExpireReservesMaxPickUpDelayCharge to a nonzero value and again after clicking the "Forgive fees for manually expired holds" checkbox. A fine should only be applied when the syspref is enabled and the checkbox is not checked. Also, the checkbox should only appear after enabling the syspref. And finally, the checkbox should remember whether it is checked across multiple checkins, same as the "Forgive overdue charges" and "Book drop mode" checkboxes. Signed-off-by: Jason Burds <[email protected]> Signed-off-by: Jonathan Druart <[email protected]> Amended patch: Removed 2 debugging lines. Signed-off-by: Tomas Cohen Arazi <[email protected]>
minusdavid · Aug 21, 2015 · bb6277f · bb6277f
1 parent 3cbed1e
commit bb6277f
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 13 deletions.
diff --git a/C4/Reserves.pm b/C4/Reserves.pm
@@ -1077,7 +1077,6 @@ sub CancelExpiredReserves {
     # Cancel reserves that have been waiting too long
     if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) {
         my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay");
-        my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
         my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays');
 
         my $today = dt_from_string();
@@ -1098,11 +1097,7 @@ sub CancelExpiredReserves {
             }
 
             if ( $do_cancel ) {
-                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'}, charge_cancel_fee => 1 });
             }
         }
     }
@@ -1129,17 +1124,19 @@ 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, ] [ charge_cancel_fee => 1 ] });
 
-Cancels a reserve.
+Cancels a reserve. If C<charge_cancel_fee> is passed and the C<ExpireReservesMaxPickUpDelayCharge> syspref is set, charge that fee to the patron's account.
 
 =cut
 
 sub CancelReserve {
     my ( $params ) = @_;
 
     my $reserve_id = $params->{'reserve_id'};
-    $reserve_id = GetReserveId( $params ) unless ( $reserve_id );
+    # Filter out only the desired keys; this will insert undefined values for elements missing in
+    # \%params, but GetReserveId filters them out anyway.
+    $reserve_id = GetReserveId( { biblionumber => $params->{'biblionumber'}, borrowernumber => $params->{'borrowernumber'}, itemnumber => $params->{'itemnumber'} } ) unless ( $reserve_id );
 
     return unless ( $reserve_id );
 
@@ -1174,6 +1171,12 @@ sub CancelReserve {
 
         # now fix the priority on the others....
         _FixPriority({ biblionumber => $reserve->{biblionumber} });
+
+        # and, if desired, charge a cancel fee
+        my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
+        if ( $charge && $params->{'charge_cancel_fee'} ) {
+            manualinvoice($reserve->{'borrowernumber'}, $reserve->{'itemnumber'}, 'Hold waiting too long', 'F', $charge);
+        }
     }
 
     return $reserve;

diff --git a/circ/returns.pl b/circ/returns.pl
@@ -30,6 +30,9 @@ =head1 returns.pl
 use strict;
 use warnings;
 
+use Carp 'verbose';
+$SIG{ __DIE__ } = sub { Carp::confess( @_ ) };
+
 use CGI qw ( -utf8 );
 use DateTime;
 use C4::Context;
@@ -84,6 +87,7 @@ =head1 returns.pl
 my $userenv = C4::Context->userenv;
 my $userenv_branch = $userenv->{'branch'} // '';
 my $printer = $userenv->{'branchprinter'} // '';
+my $forgivemanualholdsexpire = $query->param('forgivemanualholdsexpire');
 
 my $overduecharges = (C4::Context->preference('finesMode') && C4::Context->preference('finesMode') ne 'off');
 #
@@ -144,12 +148,18 @@ =head1 returns.pl
     my $resbarcode     = $query->param('resbarcode');
     my $diffBranchReturned = $query->param('diffBranch');
     my $iteminfo   = GetBiblioFromItemNumber($item);
+    my $cancel_reserve = $query->param('cancel_reserve');
     # fix up item type for display
     $iteminfo->{'itemtype'} = C4::Context->preference('item-level_itypes') ? $iteminfo->{'itype'} : $iteminfo->{'itemtype'};
-    my $diffBranchSend = ($userenv_branch ne $diffBranchReturned) ? $diffBranchReturned : undef;
-# diffBranchSend tells ModReserveAffect whether document is expected in this library or not,
-# i.e., whether to apply waiting status
-    ModReserveAffect( $item, $borrowernumber, $diffBranchSend);
+
+    if ( $cancel_reserve ) {
+        CancelReserve({ borrowernumber => $borrowernumber, itemnumber => $item, charge_cancel_fee => !$forgivemanualholdsexpire });
+    } else {
+        my $diffBranchSend = ($userenv_branch ne $diffBranchReturned) ? $diffBranchReturned : undef;
+        # diffBranchSend tells ModReserveAffect whether document is expected in this library or not,
+        # i.e., whether to apply waiting status
+        ModReserveAffect( $item, $borrowernumber, $diffBranchSend);
+    }
 #   check if we have other reserves for this document, if we have a return send the message of transfer
     my ( $messages, $nextreservinfo ) = GetOtherReserves($item);
 
@@ -613,6 +623,7 @@ =head1 returns.pl
     exemptfine     => $exemptfine,
     dropboxmode    => $dropboxmode,
     dropboxdate    => output_pref($dropboxdate),
+    forgivemanualholdsexpire => $forgivemanualholdsexpire,
     overduecharges => $overduecharges,
     soundon        => C4::Context->preference("SoundOn"),
     BlockReturnOfWithdrawnItems => C4::Context->preference("BlockReturnOfWithdrawnItems"),

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
@@ -79,6 +79,16 @@ $(document).ready(function () {
         }
         $("#barcode").focus();
     });
+    $("#forgivemanualholdsexpire").change(function () {
+        if (this.checked == true) {
+            $("#barcode").addClass("alert");
+            $("#forgivemanualholdsexpire-alert").show();
+        } else {
+            $("#barcode").removeClass("alert");
+            $("#forgivemanualholdsexpire-alert").hide();
+        }
+        $("#barcode").focus();
+    });
     [% IF(overduecharges) %] $("#barcode").focus(function () {
         if (($("#exemptcheck").attr("checked") == true) || ($("#dropboxcheck").attr("checked") == true)) {
             $("#barcode").addClass("alert");
@@ -211,6 +221,9 @@ $(document).ready(function () {
 		<h4><strong>Hold at</strong> [% destbranchname %]</h4>
         [% END %]
         <form method="post" action="returns.pl" class="confirm">
+            <input type="hidden" name="cancel_reserve" value="0" />
+            <input type="submit" class="deny" value="Cancel" onclick="this.form.cancel_reserve.value = 1; this.form.submit();" />
+
             <input type="submit" class="approve" value="Confirm" />
 
             <input type="hidden" name="print_slip" value="0" />
@@ -227,6 +240,7 @@ $(document).ready(function () {
                 <input type="hidden" name="diffBranch" value="[% destbranch %]" />
                 <input type="hidden" name="exemptfine" value="[% exemptfine %]" />
                 <input type="hidden" name="dropboxmode" value="[% dropboxmode %]" />
+                <input type="hidden" name="forgivemanualholdsexpire" value="[% forgivemanualholdsexpire %]" />
 
                 <input type="hidden" name="return_date_override" value="[% return_date_override %]" />
                 <input type="hidden" name="return_date_override_remember" value="[% return_date_override_remember %]" />
@@ -272,6 +286,7 @@ $(document).ready(function () {
             <input type="hidden" name="diffBranch" value="[% destbranch %]" />
             <input type="hidden" name="exemptfine" value="[% exemptfine %]" />
             <input type="hidden" name="dropboxmode" value="[% dropboxmode %]" />
+            <input type="hidden" name="forgivemanualholdsexpire" value="[% forgivemanualholdsexpire %]" />
             <input type="hidden" name="barcode" value="0" />
 
             <input type="hidden" name="return_date_override" value="[% return_date_override %]" />
@@ -307,6 +322,7 @@ $(document).ready(function () {
 	<input type="hidden" name="transferitem" value="[% itemnumber %]" />
         <input type="hidden" name="exemptfine" value="[% exemptfine %]" />
         <input type="hidden" name="dropboxmode" value="[% dropboxmode %]" />
+        <input type="hidden" name="forgivemanualholdsexpire" value="[% forgivemanualholdsexpire %]" />
 	[% FOREACH inputloo IN inputloop %]
 	<input type="hidden" name="ri-[% inputloo.counter %]" value="[% inputloo.barcode %]" />
 	<input type="hidden" name="dd-[% inputloo.counter %]" value="[% inputloo.duedate %]" />
@@ -398,6 +414,7 @@ $(document).ready(function () {
             <input type="hidden" name="diffBranch" value="[% destbranch %]" />
             <input type="hidden" name="exemptfine" value="[% exemptfine %]" />
             <input type="hidden" name="dropboxmode" value="[% dropboxmode %]" />
+            <input type="hidden" name="forgivemanualholdsexpire" value="[% forgivemanualholdsexpire %]" />
             <input type="hidden" name="return_date_override" value="[% return_date_override %]" />
             <input type="hidden" name="return_date_override_remember" value="[% return_date_override_remember %]" />
         </form>
@@ -488,6 +505,9 @@ $(document).ready(function () {
     <div id="exemptfines" class="dialog message" style="display:none;">
         <p>Fines for returned items are forgiven.</p>
     </div>
+    <div id="forgivemanualholdsexpire-alert" class="dialog message" style="display:none;">
+        <p>Fines are not charged for manually cancelled holds.</p>
+    </div>
     <div id="dropboxmode" class="dialog message" style="display:none;">
         <p>Book drop mode.  (Effective checkin date is [% dropboxdate %] ).</p>
     </div>
@@ -553,6 +573,16 @@ $(document).ready(function () {
                         [% END %]
                         <label for="dropboxcheck">Book drop mode</label>
                     </p>
+                    [% IF Koha.Preference('ExpireReservesMaxPickUpDelayCharge') %]
+                    <p>
+                        [% IF ( forgivemanualholdsexpire ) %]
+                        <input type="checkbox" id="forgivemanualholdsexpire" name="forgivemanualholdsexpire" value="forgivemanualholdsexpire" checked="checked" />
+                        [% ELSE %]
+                        <input type="checkbox" id="forgivemanualholdsexpire" name="forgivemanualholdsexpire" value="forgivemanualholdsexpire" />
+                        [% END %]
+                        <label for="forgivemanualholdsexpire">Forgive fees for manually expired holds</label>
+                    </p>
+                    [% END %] <!-- overduecharges -->
             </fieldset>
         </div>
     </form>