From c95ffe0e6f9301500a00a90111e873a7fc469c83 Mon Sep 17 00:00:00 2001
From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Date: Tue, 15 Oct 2013 13:35:01 +0300
Subject: [PATCH] Bug 10993 - Transfer limits should be checked when placing a
 hold/reservation in OPAC.

Depends on bug 11005, 11020

Prevents a user from placing holds on material whose branch transfer is denied. Displays clear messages that items are
unavailable and re-checks in the business layer in case of UI blocks failing.
Enables choosing another pickup location.

Manual test plan included.
---
 .../opac-tmpl/prog/en/modules/opac-reserve.tt      | 85 +++++++++++++------
 opac/opac-reserve.pl                               | 95 ++++++++++++++--------
 2 files changed, 121 insertions(+), 59 deletions(-)

diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
index bf334fa..21d2ae4 100644
--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
@@ -5,6 +5,7 @@
 <script type="text/javascript">
 // <![CDATA[
  var MSG_NO_COPY_SELECTED = _("Expecting a specific copy selection.");
+ var MSG_NO_TRANSFERRABLE_ITEMS = _("No item can be transferred to your pickup location. Please try another pickup location for ");
  var ForceHoldNotesReasons=new Array(
     _("This title consists of multiple volumes/parts. Please indicate which part you need. Clicking on specific copy information may be helpful."),
     "*** Add a new reason above this line ***" ); // NOTE: Do not renumber reasons; this will affect use of existing ones.
@@ -21,16 +22,31 @@
  $(document).ready(function() {
     $('#hold-request-form').preventDoubleFormSubmit();
 
+    $("#hold-request-form").find("select[name='branch']").change( function() {
+            var newSelection = $(this).val();
+
+            //Make sure that when changing the pickup location by resubmitting the form,
+            // user doesn't accidentally confirm his reservations.
+            var place_reserveInput = $("#hold-request-form").find('[name="place_reserve"]');
+            place_reserveInput.attr("value", 0);
+
+            //Make sure the pickup location is changed to the first select as well.
+            // The branch selection uses only the first given pickuplocations branch value.
+            $("#hold-request-form").find("select[name='branch']").val(newSelection);
+
+            $("#hold-request-form").submit();
+    });
+
     var copiesRowId = null;
     var wasSpecific = false;
     var lastCopiesRowId = null;
-	$(".checkitem").parent().click(function(e){
-		if(e.target.tagName.toLowerCase() == 'td'){
+    $(".checkitem").parent().click(function(e){
+        if(e.target.tagName.toLowerCase() == 'td'){
            $(this).find("input.checkitem").each( function() {
                $(this).attr('checked', !$(this).attr('checked'));
-			});
-		}
-	});
+            });
+        }
+    });
     // Hides all 'specific copy' table rows on load.
     $(".copiesrow").hide();
 
@@ -138,6 +154,21 @@
             return false;
         }
 
+        //Check for each biblio if there are no available Items that can be transferred to the pickup location,
+        // fail the hold request and alert the user.
+        var anotherPickupLocationSuggesters = $('#hold-request-form').find('div[id^="suggestAnotherPickupLocation_"]');
+        if ( $(anotherPickupLocationSuggesters).size() > 0 ) {
+            anotherPickupLocationSuggesters.each( function() {
+                    //Extract the biblionumber from the id and fetch the Biblio title
+                    var idString = $(this).attr('id');
+                    var bibnumber = idString.replace('suggestAnotherPickupLocation_', '');
+                    var title = $('#title_'+bibnumber).text();
+
+                    alert(MSG_NO_TRANSFERRABLE_ITEMS + title);
+            });
+            return false;
+        }
+
         // Find the items with the 'Hold' box checked
         var badBib = null;
         $(".confirmjs:checked").each(function() {
@@ -189,7 +220,6 @@
 
 
 
-
 [% FOREACH bibitemloo IN bibitemloop %]
     [% IF ( bibitemloo.holdable ) %]
         // http://jqueryui.com/demos/datepicker/#date-range
@@ -220,7 +250,7 @@
   <div id="doc3" class="yui-t7">
     <div id="bd">
     [% INCLUDE 'masthead.inc' %]
-	  <div id="yui-g">
+      <div id="yui-g">
         <div id="holds" class="container">
         [% IF ( message ) %]
             [% IF ( GNA ) %]
@@ -273,25 +303,26 @@
             [% IF ( none_available ) %]
                 <div id="none_available" class="dialog alert"><strong>Sorry</strong>, none of these items can be placed on hold.
                 </div>
-              [% END %]
+            [% END %]
           [% END %]<!-- NAME="message" -->
 
-      [% UNLESS ( message ) %][% UNLESS ( none_available ) %]<h3>Confirm holds for:
-                      [% FOREACH USER_INF IN USER_INFO %]
-                        [% USER_INF.firstname %] [% USER_INF.surname %] ([% USER_INF.cardnumber %])
-                      [% END %]
-                    </h3>[% END %]
-	      [% IF (RESERVE_CHARGE) %]
-	      <div class="dialog alert" id="reserve_fee">
-	        There is a charge of [% RESERVE_CHARGE %] for placing this hold
-	      </div>
-	      [% END %]
+        [% UNLESS ( message ) %][% IF ( 1 ) %]
+            <h3>Confirm holds for:
+                [% FOREACH USER_INF IN USER_INFO %]
+                    [% USER_INF.firstname %] [% USER_INF.surname %] ([% USER_INF.cardnumber %])
+                [% END %]
+            </h3>[% END %]
+            [% IF (RESERVE_CHARGE) %]
+                <div class="dialog alert" id="reserve_fee">
+                    There is a charge of [% RESERVE_CHARGE %] for placing this hold
+                </div>
+            [% END %]
 
             <form action="/cgi-bin/koha/opac-reserve.pl" method="post" id="hold-request-form">
             <input type="hidden" name="place_reserve" value="1"/>
 
-            <!-- These values are set dynamically by js -->
-            <input type="hidden" name="biblionumbers" id="biblionumbers"/>
+            <!-- These values are set dynamically by js. Biblionumbers is needed for the page reload when changing pickup branch -->
+            <input type="hidden" name="biblionumbers" id="biblionumbers" value="[% biblionumbers %]"/>
             <input type="hidden" name="selecteditems" id="selections"/>
             <div id="bigloop">
 
@@ -314,7 +345,7 @@
                         [% ELSE %]
                         [% END %]
 
-                            <a class="title" href="/cgi-bin/koha/opac-detail.pl?biblionumber=[% bibitemloo.biblionumber %]">[% bibitemloo.title |html %][% IF ( bibitemloo.subtitle ) %] [% FOREACH subtitl IN bibitemloo.subtitle %][% subtitl.subfield %][% END %][% END %]</a>
+                            <a class="title" id="title_[% bibitemloo.biblionumber %]" href="/cgi-bin/koha/opac-detail.pl?biblionumber=[% bibitemloo.biblionumber %]">[% bibitemloo.title |html %][% IF ( bibitemloo.subtitle ) %] [% FOREACH subtitl IN bibitemloo.subtitle %][% subtitl.subfield %][% END %][% END %]</a>
                                             [% IF ( bibitemloo.author ) %],  by [% bibitemloo.author %][% END %]</p>
 
 
@@ -359,6 +390,10 @@
 
 
                             [% UNLESS ( singleBranchMode ) %]
+                                [% IF ( bibitemloo.suggestAnotherPickupLocation ) %]
+                                    <div id="suggestAnotherPickupLocation_[% bibitemloo.biblionumber %]" class="dialog alert">None of the available items can be transferred to your pickup location. You could try another pickup location.
+                                    </div>
+                                [% END %]
                                 [% IF ( bibitemloo.holdable ) %]
                                     [% IF ( choose_branch ) %]
                                         <li class="branch">
@@ -398,8 +433,8 @@
                                     <span class="date-format from" data-biblionumber="[% bibitemloo.biblionumber %]">[% INCLUDE 'date-format.inc' %]</span>
                                 </li>
                             [% END %]
+                        [% END %][%#End of IF ( bibitemloo.holdable ) %]
 
-                        [% END %]
                         [% IF ( bibitemloo.holdable ) %]
                             <li>
                                 <label for="to[% bibitemloo.biblionumber %]">Hold not needed after:</label>
@@ -440,8 +475,8 @@
                                            checked="checked"
                                     />
                                   [% END %]
-
                                   <label for="reqany_[% bibitemloo.biblionumber %]">Next available copy</label>
+
                                   [% UNLESS ( bibitemloo.holdable ) %]
                                     <input type="radio" name="reqtype_[% bibitemloo.biblionumber %]"
                                            id="reqspecific_[% bibitemloo.biblionumber %]"
@@ -500,7 +535,7 @@
                                   [% UNLESS ( noItemTypeImages ) %]
                                   [% IF ( itemLoo.imageurl ) %]<img src="[% itemLoo.imageurl %]" alt="" />[% END %]
                                   [% END %]
-								[% itemLoo.description %]
+                                [% itemLoo.description %]
                                 </td>
                               [% END %]
                               <td class="barcode">[% itemLoo.barcode %]</td>
@@ -551,10 +586,8 @@
             </div><!-- bigloop -->
 
             [% UNLESS ( message ) %]
-            [% UNLESS ( none_available ) %]
             <input type="submit" value="Place hold" class="placehold" />
             [% END %]
-            [% END %]
 
             </form>
 
diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index 4bba9a6..2dcb75e 100755
--- a/opac/opac-reserve.pl
+++ b/opac/opac-reserve.pl
@@ -58,8 +58,8 @@ for ( C4::Context->preference("OPACShowHoldQueueDetails") ) {
 }
 
 sub get_out {
-	output_html_with_http_headers(shift,shift,shift); # $query, $cookie, $template->output;
-	exit;
+    output_html_with_http_headers(shift,shift,shift); # $query, $cookie, $template->output;
+    exit;
 }
 
 # get borrower information ....
@@ -91,7 +91,8 @@ if ((! $biblionumbers) && (! $query->param('place_reserve'))) {
 }
 
 # Pass the numbers to the page so they can be fed back
-# when the hold is confirmed. TODO: Not necessary?
+# when the hold is confirmed. Biblionumbers are also needed if the
+# pickup branch changes and the page needs to be reloaded.
 $template->param( biblionumbers => $biblionumbers );
 
 # Each biblio number is suffixed with '/', e.g. "1/2/3/"
@@ -103,12 +104,12 @@ if (($#biblionumbers < 0) && (! $query->param('place_reserve'))) {
 }
 
 # pass the pickup branch along....
-my $branch = $query->param('branch') || $borr->{'branchcode'} || C4::Context->userenv->{branch} || '' ;
-($branches->{$branch}) or $branch = "";     # Confirm branch is real
-$template->param( branch => $branch );
+my $pickupBranch = $query->param('branch') || $borr->{'branchcode'} || C4::Context->userenv->{branch} || '' ;
+($branches->{$pickupBranch}) or $pickupBranch = "";     # Confirm branch is real
+$template->param( branch => $pickupBranch );
 
 # make branch selection options...
-my $branchloop = GetBranchesLoop($branch);
+my $branchloop = GetBranchesLoop($pickupBranch);
 
 # Is the person allowed to choose their branch
 my $OPACChooseBranch = (C4::Context->preference("OPACAllowUserToChooseBranch")) ? 1 : 0;
@@ -202,19 +203,19 @@ if ( $query->param('place_reserve') ) {
     }
 
     while (@selectedItems) {
-        my $biblioNum = shift(@selectedItems);
-        my $itemNum   = shift(@selectedItems);
-        my $branch    = shift(@selectedItems);    # i.e., branch code, not name
+        my $biblioNum 		= shift(@selectedItems);
+        my $itemNum   		= shift(@selectedItems);
+        my $pickupLocation  = shift(@selectedItems);    # i.e., branch code, not name,
 
         my $canreserve = 0;
 
         my $singleBranchMode = C4::Context->preference("singleBranchMode");
         if ( $singleBranchMode || !$OPACChooseBranch )
         {    # single branch mode or disabled user choosing
-            $branch = $borr->{'branchcode'};
+            $pickupLocation = $borr->{'branchcode'};
         }
 
-#item may belong to a host biblio, if yes change biblioNum to hosts bilbionumber
+        #item may belong to a host biblio, if yes change biblioNum to hosts bilbionumber
         if ( $itemNum ne '' ) {
             my $hostbiblioNum = GetBiblionumberFromItemnumber($itemNum);
             if ( $hostbiblioNum ne $biblioNum ) {
@@ -235,20 +236,26 @@ if ( $query->param('place_reserve') ) {
 
         my $expiration_date = $query->param("expiration_date_$biblioNum");
 
-      # If a specific item was selected and the pickup branch is the same as the
-      # holdingbranch, force the value $rank and $found.
+        # If a specific item was selected and the pickup branch is the same as the
+        # holdingbranch, force the value $rank and $found.
         my $rank = $biblioData->{rank};
         if ( $itemNum ne '' ) {
+            my $item = GetItem($itemNum);
             $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum );
             $rank = '0' unless C4::Context->preference('ReservesNeedReturns');
-            my $item = GetItem($itemNum);
-            if ( $item->{'holdingbranch'} eq $branch ) {
+            if ( $item->{'holdingbranch'} eq $pickupLocation ) {
                 $found = 'W'
                   unless C4::Context->preference('ReservesNeedReturns');
             }
+
+            # UseBranchTransferLimits checking.
+            my ($transferOk, $message) = CheckBranchTransferAllowed( $pickupLocation, $item->{'holdingbranch'}, $item, undef );
+            if (! $transferOk) {
+                $canreserve = 0;
+            }
         }
         else {
-            $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum );
+            $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum, $pickupLocation );
 
             # Inserts a null into the 'itemnumber' field of 'reserves' table.
             $itemNum = undef;
@@ -264,7 +271,7 @@ if ( $query->param('place_reserve') ) {
         # Here we actually do the reserveration. Stage 3.
         if ($canreserve) {
             AddReserve(
-                $branch,      $borrowernumber,
+                $pickupLocation,      $borrowernumber,
                 $biblioNum,   'a',
                 [$biblioNum], $rank,
                 $startdate,   $expiration_date,
@@ -398,6 +405,10 @@ foreach my $biblioNum (@biblionumbers) {
         }
     }
 
+    #Collect the amout of items that pass the CheckBranchTransferAllowed-check. This is needed to tell
+    #  the user if some or all Items cannot be transferred to the pickup location.
+    my $branchTransferableItemsCount = 0;
+
     $biblioLoopIter{itemLoop} = [];
     my $numCopiesAvailable = 0;
     foreach my $itemInfo (@{$biblioData->{itemInfos}}) {
@@ -434,10 +445,10 @@ foreach my $biblioNum (@biblionumbers) {
         my ($reservedate,$reservedfor,$expectedAt) = GetReservesFromItemnumber($itemNum);
         my $ItemBorrowerReserveInfo = GetMemberDetails( $reservedfor, 0);
 
-	# the item could be reserved for this borrower vi a host record, flag this
-	if ($reservedfor eq $borrowernumber){
-		$itemLoopIter->{already_reserved} = 1;
-	}
+        # the item could be reserved for this borrower vi a host record, flag this
+        if ($reservedfor eq $borrowernumber){
+            $itemLoopIter->{already_reserved} = 1;
+        }
 
         if ( defined $reservedate ) {
             $itemLoopIter->{backgroundcolor} = 'reserved';
@@ -480,12 +491,12 @@ foreach my $biblioNum (@biblionumbers) {
             $itemLoopIter->{nocancel} = 1;
         }
 
-	# if the items belongs to a host record, show link to host record
-	if ($itemInfo->{biblionumber} ne $biblioNum){
-		$biblioLoopIter{hostitemsflag} = 1;
-		$itemLoopIter->{hostbiblionumber} = $itemInfo->{biblionumber};
-		$itemLoopIter->{hosttitle} = GetBiblioData($itemInfo->{biblionumber})->{title};
-	}
+        # if the items belongs to a host record, show link to host record
+        if ($itemInfo->{biblionumber} ne $biblioNum){
+            $biblioLoopIter{hostitemsflag} = 1;
+            $itemLoopIter->{hostbiblionumber} = $itemInfo->{biblionumber};
+            $itemLoopIter->{hosttitle} = GetBiblioData($itemInfo->{biblionumber})->{title};
+        }
 
         # If there is no loan, return and transfer, we show a checkbox.
         $itemLoopIter->{notforloan} = $itemLoopIter->{notforloan} || 0;
@@ -501,20 +512,32 @@ foreach my $biblioNum (@biblionumbers) {
         }
 
         if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and CanItemBeReserved($borrowernumber,$itemNum) and ($itemLoopIter->{already_reserved} ne 1)) {
+
             $itemLoopIter->{available} = 1;
             $numCopiesAvailable++;
+
+            #Check for UseBranchTransferLimit. $numCopiesAvailable is incremented because this Item
+            #  could still be available from another pickup location
+            my ($transferOk, $errorMsg) = CheckBranchTransferAllowed( $pickupBranch, undef, GetItem($itemNum), undef );
+            if (! $transferOk) {
+                $itemLoopIter->{available} = 0;
+                $itemLoopIter->{branchTransferBlocked} = 1;
+            }
+            else {
+                $branchTransferableItemsCount++;
+            }
         }
 
-	# FIXME: move this to a pm
+        # FIXME: move this to a pm
         my $dbh = C4::Context->dbh;
         my $sth2 = $dbh->prepare("SELECT * FROM reserves WHERE borrowernumber=? AND itemnumber=? AND found='W'");
         $sth2->execute($itemLoopIter->{ReservedForBorrowernumber}, $itemNum);
         while (my $wait_hashref = $sth2->fetchrow_hashref) {
             $itemLoopIter->{waitingdate} = format_date($wait_hashref->{waitingdate});
         }
-	$itemLoopIter->{imageurl} = getitemtypeimagelocation( 'opac', $itemTypes->{ $itemInfo->{itype} }{imageurl} );
+        $itemLoopIter->{imageurl} = getitemtypeimagelocation( 'opac', $itemTypes->{ $itemInfo->{itype} }{imageurl} );
 
-    # Show serial enumeration when needed
+        # Show serial enumeration when needed
         if ($itemLoopIter->{enumchron}) {
             $itemdata_enumchron = 1;
         }
@@ -523,6 +546,7 @@ foreach my $biblioNum (@biblionumbers) {
     }
     $template->param( itemdata_enumchron => $itemdata_enumchron );
 
+    ## Set the behaviour flags for the template
     if ($numCopiesAvailable > 0) {
         $numBibsAvailable++;
         $biblioLoopIter{bib_available} = 1;
@@ -538,13 +562,18 @@ foreach my $biblioNum (@biblionumbers) {
         $biblioLoopIter{holdable} = undef;
         $biblioLoopIter{already_patron_possession} = 1;
     }
+    if ($branchTransferableItemsCount == 0) {
+        #We can tell our Borrowers that they can try another pickup location if they don't find what they need.
+        $biblioLoopIter{suggestAnotherPickupLocation} = 1 ;
+    }
+
 
     if( $biblioLoopIter{holdable} ){ $anyholdable++; }
 
     push @$biblioLoop, \%biblioLoopIter;
 }
 
-if ( $numBibsAvailable == 0 || $anyholdable == 0) {
+if ( $numBibsAvailable == 0 || $anyholdable == 0 ) {
     $template->param( none_available => 1 );
 }
 
@@ -571,7 +600,7 @@ if (
     C4::Context->preference( 'OPACAllowHoldDateInFuture' )
     ) {
     $template->param(
-	    reserve_in_future         => 1,
+        reserve_in_future         => 1,
     );
 }
 
-- 
1.8.1.2