[Koha-patches] [PATCH] bug_5786, bug_5787, bug_5788: Holds rewrite

Srdjan srdjan at catalyst.net.nz
Wed Sep 19 02:18:01 CEST 2012


bug_5786: moved AllowOnShelfHolds to issuingrules
bug_5787: moved OPACItemHolds to issuingrules
bug_5788: Added reservesmaxpickupdelay to issuingrules

C4::Reserves:
* Added OnShelfHoldsAllowed() to check issuingrules
* Added OPACItemHoldsAllowed() to check issuingrules
* IsAvailableForItemLevelRequest() changed interface, now takes
  $item_record,$borrower_record; calls OnShelfHoldsAllowed()

opac/opac-reserve.pl and opac/opac-search.pl:
* rewrote hold allowed rule to use OPACItemHoldsAllowed()
* also use OnShelfHoldsAllowed() through
* IsAvailableForItemLevelRequest()

templates:
* Removed AllowOnShelfHolds and OPACItemHolds global flags, they now
  only have meaning per item type

Signed-off-by: Nicole C. Engard <nengard at bywatersolutions.com>

I have tested this patch left, right and upside down for the last
several months. All tests have passed.

Signed-off-by: Kyle M Hall <kyle at bywatersolutions.com>
---
 C4/Auth.pm                                         |    1 -
 C4/ILSDI/Services.pm                               |    2 +-
 C4/Items.pm                                        |    2 +
 C4/Reserves.pm                                     |  341 ++++++++++++--------
 C4/VirtualShelves/Page.pm                          |    1 -
 admin/smart-rules.pl                               |   19 +-
 admin/systempreferences.pl                         |    2 -
 .../mysql/it-IT/necessari/system_preferences.sql   |    1 -
 installer/data/mysql/kohastructure.sql             |    3 +
 installer/data/mysql/sysprefs.sql                  |    2 -
 installer/data/mysql/updatedatabase.pl             |   36 ++-
 installer/html-template-to-template-toolkit.pl     |    2 +-
 .../en/modules/admin/preferences/circulation.pref  |    6 -
 .../prog/en/modules/admin/preferences/opac.pref    |    6 -
 .../prog/en/modules/admin/smart-rules.tt           |   94 +++---
 .../prog/en/includes/opac-detail-sidebar.inc       |    8 +-
 .../opac-tmpl/prog/en/modules/opac-reserve.tt      |  177 +++++-----
 .../prog/en/modules/opac-results-grouped.tt        |   18 +-
 .../opac-tmpl/prog/en/modules/opac-results.tt      |    6 +-
 .../opac-tmpl/prog/en/modules/opac-shelves.tt      |    2 +-
 opac/opac-ISBDdetail.pl                            |    9 +-
 opac/opac-MARCdetail.pl                            |    8 +-
 opac/opac-detail.pl                                |    2 -
 opac/opac-reserve.pl                               |   43 +--
 opac/opac-search.pl                                |   19 +-
 reserve/request.pl                                 |    2 +-
 26 files changed, 439 insertions(+), 373 deletions(-)

diff --git a/C4/Auth.pm b/C4/Auth.pm
index d4ad292..787772b 100644
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -397,7 +397,6 @@ sub get_template_and_user {
             OPACAmazonCoverImages     => C4::Context->preference("OPACAmazonCoverImages"),
             OPACFRBRizeEditions       => C4::Context->preference("OPACFRBRizeEditions"),
             OpacHighlightedWords       => C4::Context->preference("OpacHighlightedWords"),
-            OPACItemHolds             => C4::Context->preference("OPACItemHolds"),
             OPACShelfBrowser          => "". C4::Context->preference("OPACShelfBrowser"),
             OpacShowRecentComments    => C4::Context->preference("OpacShowRecentComments"),
             OPACURLOpenInNewWindow    => "" . C4::Context->preference("OPACURLOpenInNewWindow"),
diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm
index d685e6c..14d92c8 100644
--- a/C4/ILSDI/Services.pm
+++ b/C4/ILSDI/Services.pm
@@ -489,7 +489,7 @@ sub GetServices {
     my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber );
     if ($canbookbereserved) {
         push @availablefor, 'title level hold';
-        my $canitembereserved = IsAvailableForItemLevelRequest($itemnumber);
+        my $canitembereserved = IsAvailableForItemLevelRequest($item, $borrower);
         if ($canitembereserved) {
             push @availablefor, 'item level hold';
         }
diff --git a/C4/Items.pm b/C4/Items.pm
index e9af3dd..8e33896 100644
--- a/C4/Items.pm
+++ b/C4/Items.pm
@@ -164,6 +164,8 @@ sub GetItem {
         ($data->{'serialseq'} , $data->{'publisheddate'}) = $ssth->fetchrow_array();
     }
 	#if we don't have an items.itype, use biblioitems.itemtype.
+    # FIXME this should respect the itypes systempreference
+    # if (C4::Context->preference('item-level_itypes')) {
 	if( ! $data->{'itype'} ) {
 		my $sth = $dbh->prepare("SELECT itemtype FROM biblioitems  WHERE biblionumber = ?");
 		$sth->execute($data->{'biblionumber'});
diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 585d1e5..a59b445 100644
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -68,13 +68,13 @@ This modules provides somes functions to deal with reservations.
   The complete workflow is :
   ==== 1st use case ====
   patron request a document, 1st available :                      P >0, F=NULL, I=NULL
-  a library having it run "transfertodo", and clic on the list    
+  a library having it run "transfertodo", and clic on the list
          if there is no transfer to do, the reserve waiting
-         patron can pick it up                                    P =0, F=W,    I=filled 
+         patron can pick it up                                    P =0, F=W,    I=filled
          if there is a transfer to do, write in branchtransfer    P =0, F=T,    I=filled
            The pickup library recieve the book, it check in       P =0, F=W,    I=filled
   The patron borrow the book                                      P =0, F=F,    I=filled
-  
+
   ==== 2nd use case ====
   patron requests a document, a given item,
     If pickup is holding branch                                   P =0, F=W,   I=filled
@@ -93,7 +93,7 @@ BEGIN {
     @ISA = qw(Exporter);
     @EXPORT = qw(
         &AddReserve
-  
+
         &GetReservesFromItemnumber
         &GetReservesFromBiblionumber
         &GetReservesFromBorrowernumber
@@ -103,9 +103,9 @@ BEGIN {
         &GetReserveFee
         &GetReserveInfo
         &GetReserveStatus
-        
+
         &GetOtherReserves
-        
+
         &ModReserveFill
         &ModReserveAffect
         &ModReserve
@@ -113,7 +113,7 @@ BEGIN {
         &ModReserveCancelAll
         &ModReserveMinusPriority
         &MoveReserve
-        
+
         &CheckReserves
         &CanBookBeReserved
 	&CanItemBeReserved
@@ -123,7 +123,9 @@ BEGIN {
         &AutoUnsuspendReserves
 
         &IsAvailableForItemLevelRequest
-        
+
+        &OPACItemHoldsAllowed
+
         &AlterPriority
         &ToggleLowestPriority
 
@@ -132,7 +134,7 @@ BEGIN {
         &SuspendAll
     );
     @EXPORT_OK = qw( MergeHolds );
-}    
+}
 
 =head2 AddReserve
 
@@ -240,7 +242,7 @@ sub AddReserve {
     foreach (@$bibitems) {
         $sth->execute($borrowernumber, $biblionumber, $resdate, $_);
     }
-        
+
     return;     # FIXME: why not have a useful return value?
 }
 
@@ -302,7 +304,7 @@ sub GetReservesFromBiblionumber {
                 push( @bibitemno, $bibitemnos );    # FIXME: inefficient: use fetchall_arrayref
             }
             my $count = scalar @bibitemno;
-    
+
             # if we have two or more different specific itemtypes
             # reserved by same person on same day
             my $bdata;
@@ -417,30 +419,30 @@ This function return 1 if an item can be issued by this borrower.
 
 sub CanItemBeReserved{
     my ($borrowernumber, $itemnumber) = @_;
-    
+
     my $dbh             = C4::Context->dbh;
     my $allowedreserves = 0;
-            
+
     my $controlbranch = C4::Context->preference('ReservesControlBranch');
     my $itype         = C4::Context->preference('item-level_itypes') ? "itype" : "itemtype";
 
     # we retrieve borrowers and items informations #
     my $item     = GetItem($itemnumber);
-    my $borrower = C4::Members::GetMember('borrowernumber'=>$borrowernumber);     
-    
+    my $borrower = C4::Members::GetMember('borrowernumber'=>$borrowernumber);
+
     # we retrieve user rights on this itemtype and branchcode
-    my $sth = $dbh->prepare("SELECT categorycode, itemtype, branchcode, reservesallowed 
-                             FROM issuingrules 
-                             WHERE (categorycode in (?,'*') ) 
-                             AND (itemtype IN (?,'*')) 
-                             AND (branchcode IN (?,'*')) 
-                             ORDER BY 
-                               categorycode DESC, 
-                               itemtype     DESC, 
+    my $sth = $dbh->prepare("SELECT categorycode, itemtype, branchcode, reservesallowed
+                             FROM issuingrules
+                             WHERE (categorycode in (?,'*') )
+                             AND (itemtype IN (?,'*'))
+                             AND (branchcode IN (?,'*'))
+                             ORDER BY
+                               categorycode DESC,
+                               itemtype     DESC,
                                branchcode   DESC;"
                            );
-                           
-    my $querycount ="SELECT 
+
+    my $querycount ="SELECT
                             count(*) as count
                             FROM reserves
                                 LEFT JOIN items USING (itemnumber)
@@ -448,13 +450,13 @@ sub CanItemBeReserved{
                                 LEFT JOIN borrowers USING (borrowernumber)
                             WHERE borrowernumber = ?
                                 ";
-    
-    
+
+
     my $itemtype     = $item->{$itype};
     my $categorycode = $borrower->{categorycode};
     my $branchcode   = "";
     my $branchfield  = "reserves.branchcode";
-    
+
     if( $controlbranch eq "ItemHomeLibrary" ){
         $branchfield = "items.homebranch";
         $branchcode = $item->{homebranch};
@@ -462,34 +464,33 @@ sub CanItemBeReserved{
         $branchfield = "borrowers.branchcode";
         $branchcode = $borrower->{branchcode};
     }
-    
-    # we retrieve rights 
+
+    # we retrieve rights
     $sth->execute($categorycode, $itemtype, $branchcode);
     if(my $rights = $sth->fetchrow_hashref()){
         $itemtype        = $rights->{itemtype};
-        $allowedreserves = $rights->{reservesallowed}; 
+        $allowedreserves = $rights->{reservesallowed};
     }else{
         $itemtype = '*';
     }
-    
+
     # we retrieve count
-    
+
     $querycount .= "AND $branchfield = ?";
-    
+
     $querycount .= " AND $itype = ?" if ($itemtype ne "*");
     my $sthcount = $dbh->prepare($querycount);
-    
+
     if($itemtype eq "*"){
         $sthcount->execute($borrowernumber, $branchcode);
     }else{
         $sthcount->execute($borrowernumber, $branchcode, $itemtype);
     }
-    
+
     my $reservecount = "0";
     if(my $rowcount = $sthcount->fetchrow_hashref()){
         $reservecount = $rowcount->{count};
     }
-    
     # we check if it's ok or not
     if( $reservecount < $allowedreserves ){
         return 1;
@@ -688,8 +689,8 @@ sub GetReservesToBranch {
     my $dbh = C4::Context->dbh;
     my $sth = $dbh->prepare(
         "SELECT borrowernumber,reservedate,itemnumber,timestamp
-         FROM reserves 
-         WHERE priority='0' 
+         FROM reserves
+         WHERE priority='0'
            AND branchcode=?"
     );
     $sth->execute( $frombranch );
@@ -712,7 +713,7 @@ sub GetReservesForBranch {
     my ($frombranch) = @_;
     my $dbh          = C4::Context->dbh;
 	my $query        = "SELECT borrowernumber,reservedate,itemnumber,waitingdate
-        FROM   reserves 
+        FROM   reserves
         WHERE   priority='0'
             AND found='W' ";
     if ($frombranch){
@@ -737,11 +738,11 @@ sub GetReservesForBranch {
 
 sub GetReserveStatus {
     my ($itemnumber) = @_;
-    
+
     my $dbh = C4::Context->dbh;
-    
+
     my $itemstatus = $dbh->prepare("SELECT found FROM reserves WHERE itemnumber = ?");
-    
+
     $itemstatus->execute($itemnumber);
     my ($found) = $itemstatus->fetchrow_array;
     return $found;
@@ -803,7 +804,7 @@ sub CheckReserves {
            LEFT JOIN itemtypes   ON biblioitems.itemtype   = itemtypes.itemtype
         ";
     }
-   
+
     if ($item) {
         $sth = $dbh->prepare("$select WHERE itemnumber = ?");
         $sth->execute($item);
@@ -818,7 +819,7 @@ sub CheckReserves {
     return ( '' ) unless $itemnumber; # bail if we got nothing.
 
     # if item is not for loan it cannot be reserved either.....
-    #    execpt where items.notforloan < 0 :  This indicates the item is holdable. 
+    #    execpt where items.notforloan < 0 :  This indicates the item is holdable.
     return ( '' ) if  ( $notforloan_per_item > 0 ) or $notforloan_per_itemtype;
 
     # Find this item in the reserves
@@ -873,7 +874,7 @@ sub CancelExpiredReserves {
     # Cancel reserves that have passed their expiration date.
     my $dbh = C4::Context->dbh;
     my $sth = $dbh->prepare( "
-        SELECT * FROM reserves WHERE DATE(expirationdate) < DATE( CURDATE() ) 
+        SELECT * FROM reserves WHERE DATE(expirationdate) < DATE( CURDATE() )
         AND expirationdate IS NOT NULL
         AND found IS NULL
     " );
@@ -882,7 +883,7 @@ sub CancelExpiredReserves {
     while ( my $res = $sth->fetchrow_hashref() ) {
         CancelReserve( $res->{'biblionumber'}, '', $res->{'borrowernumber'} );
     }
-  
+
     # Cancel reserves that have been waiting too long
     if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") ) {
         my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay");
@@ -1038,12 +1039,12 @@ If C<$rank> is 'del', the hold request is cancelled.
 
 If C<$rank> is an integer greater than zero, the priority of
 the request is set to that value.  Since priority != 0 means
-that the item is not waiting on the hold shelf, setting the 
+that the item is not waiting on the hold shelf, setting the
 priority to a non-zero value also sets the request's found
-status and waiting date to NULL. 
+status and waiting date to NULL.
 
 The optional C<$itemnumber> parameter is used only when
-C<$rank> is a non-zero integer; if supplied, the itemnumber 
+C<$rank> is a non-zero integer; if supplied, the itemnumber
 of the hold request is set accordingly; if omitted, the itemnumber
 is cleared.
 
@@ -1073,20 +1074,20 @@ sub ModReserve {
         $query = qq/
             INSERT INTO old_reserves
             SELECT *
-            FROM   reserves 
+            FROM   reserves
             WHERE  biblionumber   = ?
              AND   borrowernumber = ?
         /;
         $sth = $dbh->prepare($query);
         $sth->execute( $biblio, $borrower );
         $query = qq/
-            DELETE FROM reserves 
+            DELETE FROM reserves
             WHERE  biblionumber   = ?
              AND   borrowernumber = ?
         /;
         $sth = $dbh->prepare($query);
         $sth->execute( $biblio, $borrower );
-        
+
     }
     elsif ($rank =~ /^\d+/ and $rank > 0) {
         my $query = "
@@ -1171,7 +1172,7 @@ sub ModReserveFill {
                 ";
     $sth = $dbh->prepare($query);
     $sth->execute( $biblionumber, $resdate, $borrowernumber );
-    
+
     # now fix the priority on the others (if the priority wasn't
     # already sorted!)....
     unless ( $priority == 0 ) {
@@ -1216,7 +1217,7 @@ with the biblionumber & the borrowernumber, we can affect the itemnumber
 to the correct reserve.
 
 if $transferToDo is not set, then the status is set to "Waiting" as well.
-otherwise, a transfer is on the way, and the end of the transfer will 
+otherwise, a transfer is on the way, and the end of the transfer will
 take care of the waiting status
 
 =cut
@@ -1297,7 +1298,7 @@ sub ModReserveCancelAll {
 
   &ModReserveMinusPriority($itemnumber,$borrowernumber,$biblionumber)
 
-Reduce the values of queuded list     
+Reduce the values of queuded list
 
 =cut
 
@@ -1308,7 +1309,7 @@ sub ModReserveMinusPriority {
     my $dbh   = C4::Context->dbh;
     my $query = "
         UPDATE reserves
-        SET    priority = 0 , itemnumber = ? 
+        SET    priority = 0 , itemnumber = ?
         WHERE  borrowernumber=?
           AND  biblionumber=?
     ";
@@ -1330,42 +1331,42 @@ Current implementation this query should have a single result.
 sub GetReserveInfo {
 	my ( $borrowernumber, $biblionumber ) = @_;
     my $dbh = C4::Context->dbh;
-	my $strsth="SELECT 
-	               reservedate, 
-	               reservenotes, 
+	my $strsth="SELECT
+	               reservedate,
+	               reservenotes,
 	               reserves.borrowernumber,
-				   reserves.biblionumber, 
+				   reserves.biblionumber,
 				   reserves.branchcode,
 				   reserves.waitingdate,
-				   notificationdate, 
-				   reminderdate, 
-				   priority, 
+				   notificationdate,
+				   reminderdate,
+				   priority,
 				   found,
-				   firstname, 
-				   surname, 
-				   phone, 
-				   email, 
-				   address, 
+				   firstname,
+				   surname,
+				   phone,
+				   email,
+				   address,
 				   address2,
-				   cardnumber, 
-				   city, 
+				   cardnumber,
+				   city,
 				   zipcode,
-				   biblio.title, 
+				   biblio.title,
 				   biblio.author,
-				   items.holdingbranch, 
-				   items.itemcallnumber, 
+				   items.holdingbranch,
+				   items.itemcallnumber,
 				   items.itemnumber,
-				   items.location, 
-				   barcode, 
+				   items.location,
+				   barcode,
 				   notes
-			FROM reserves 
-			 LEFT JOIN items USING(itemnumber) 
+			FROM reserves
+			 LEFT JOIN items USING(itemnumber)
 		     LEFT JOIN borrowers USING(borrowernumber)
-		     LEFT JOIN biblio ON  (reserves.biblionumber=biblio.biblionumber) 
-			WHERE 
+		     LEFT JOIN biblio ON  (reserves.biblionumber=biblio.biblionumber)
+			WHERE
 				reserves.borrowernumber=?
 				AND reserves.biblionumber=?";
-	my $sth = $dbh->prepare($strsth); 
+	my $sth = $dbh->prepare($strsth);
 	$sth->execute($borrowernumber,$biblionumber);
 
 	my $data = $sth->fetchrow_hashref;
@@ -1375,22 +1376,18 @@ sub GetReserveInfo {
 
 =head2 IsAvailableForItemLevelRequest
 
-  my $is_available = IsAvailableForItemLevelRequest($itemnumber);
+  my $is_available = IsAvailableForItemLevelRequest($item_record,$borrower_record);
 
 Checks whether a given item record is available for an
 item-level hold request.  An item is available if
 
-* it is not lost AND 
-* it is not damaged AND 
-* it is not withdrawn AND 
+* it is not lost AND
+* it is not damaged AND
+* it is not withdrawn AND
 * does not have a not for loan value > 0
 
-Whether or not the item is currently on loan is 
-also checked - if the AllowOnShelfHolds system preference
-is ON, an item can be requested even if it is currently
-on loan to somebody else.  If the system preference
-is OFF, an item that is currently checked out cannot
-be the target of an item-level hold request.
+Need to check the issuingrules onshelfholds column,
+if this is set items on the shelf can be placed on hold
 
 Note that IsAvailableForItemLevelRequest() does not
 check if the staff operator is authorized to place
@@ -1401,50 +1398,82 @@ and canreservefromotherbranches.
 =cut
 
 sub IsAvailableForItemLevelRequest {
-    my $itemnumber = shift;
-   
-    my $item = GetItem($itemnumber);
+    my $item = shift;
+    my $borrower = shift;
 
+    my $dbh = C4::Context->dbh;
     # must check the notforloan setting of the itemtype
     # FIXME - a lot of places in the code do this
     #         or something similar - need to be
     #         consolidated
-    my $dbh = C4::Context->dbh;
-    my $notforloan_query;
+    my $itype;
     if (C4::Context->preference('item-level_itypes')) {
-        $notforloan_query = "SELECT itemtypes.notforloan
-                             FROM items
-                             JOIN itemtypes ON (itemtypes.itemtype = items.itype)
-                             WHERE itemnumber = ?";
-    } else {
-        $notforloan_query = "SELECT itemtypes.notforloan
-                             FROM items
-                             JOIN biblioitems USING (biblioitemnumber)
-                             JOIN itemtypes USING (itemtype)
-                             WHERE itemnumber = ?";
+        # We cant trust GetItem to honour the syspref, so safest to do it ourselves
+        # When GetItem is fixed, we can remove this
+        $itype = $item->{itype};
     }
-    my $sth = $dbh->prepare($notforloan_query);
-    $sth->execute($itemnumber);
-    my $notforloan_per_itemtype = 0;
-    if (my ($notforloan) = $sth->fetchrow_array) {
-        $notforloan_per_itemtype = 1 if $notforloan;
+    else {
+        # XXX This is a bit dodgy. It relies on biblio itemtype column having different name.
+        # So if we already have a biblioitems join when calling this function,
+        # we don't need to access the database again
+        $itype = $item->{itemtype};
+    }
+    unless ($itype) {
+        my $query = "SELECT itemtype FROM biblioitems WHERE biblioitemnumber = ? ";
+        my $sth = $dbh->prepare($query);
+        $sth->execute($item->{biblioitemnumber});
+        if (my $data = $sth->fetchrow_hashref()){
+            $itype = $data->{itemtype};
+        }
     }
 
-    my $available_per_item = 1;
-    $available_per_item = 0 if $item->{itemlost} or
-                               ( $item->{notforloan} > 0 ) or
-                               ($item->{damaged} and not C4::Context->preference('AllowHoldsOnDamagedItems')) or
-                               $item->{wthdrawn} or
-                               $notforloan_per_itemtype;
+    my $notforloan_per_itemtype
+      = $dbh->selectrow_array("SELECT notforloan FROM itemtypes WHERE itemtype = ?",
+                              undef, $itype);
+
+    return 0 if
+        $notforloan_per_itemtype ||
+        $item->{itemlost}        ||
+        $item->{notforloan} > 0  ||
+        $item->{wthdrawn}        ||
+        ($item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems'));
 
 
-    if (C4::Context->preference('AllowOnShelfHolds')) {
-        return $available_per_item;
+    if (OnShelfHoldsAllowed($itype,$borrower->{categorycode},$item->{holdingbranch})) {
+        return 1;
     } else {
-        return ($available_per_item and ($item->{onloan} or GetReserveStatus($itemnumber) eq "W")); 
+        return $item->{onloan} || GetReserveStatus($item->{itemnumber}) eq "W";
     }
 }
 
+=head2 OnShelfHoldsAllowed
+
+  OnShelfHoldsAllowed($itemtype,$borrowercategory,$branchcode);
+
+Checks issuingrules, using the borrowers categorycode, the itemtype, and branchcode to see if onshelf
+holds are allowed, returns true if so.
+
+=cut
+
+sub OnShelfHoldsAllowed {
+    my ($itype,$borrowercategory,$branchcode) = @_;
+
+    my $query = "SELECT onshelfholds FROM issuingrules WHERE
+          (issuingrules.categorycode = ? OR issuingrules.categorycode = '*')
+        AND
+          (issuingrules.itemtype = ? OR issuingrules.itemtype = '*')
+        AND
+          (issuingrules.branchcode = ? OR issuingrules.branchcode = '*')
+        ORDER BY
+          issuingrules.categorycode desc,
+          issuingrules.itemtype desc,
+          issuingrules.branchcode desc
+       LIMIT 1";
+    my $dbh = C4::Context->dbh;
+    my $onshelfholds = $dbh->selectrow_array($query, undef, $borrowercategory,$itype,$branchcode);
+    return $onshelfholds;
+}
+
 =head2 AlterPriority
 
   AlterPriority( $where, $borrowernumber, $biblionumber, $reservedate );
@@ -1466,8 +1495,8 @@ sub AlterPriority {
     $sth->finish();
 
     if ( $where eq 'up' || $where eq 'down' ) {
-    
-      my $priority = $reserve->{'priority'};        
+
+      my $priority = $reserve->{'priority'};
       $priority = $where eq 'up' ? $priority - 1 : $priority + 1;
       _FixPriority( $biblionumber, $borrowernumber, $priority )
 
@@ -1505,7 +1534,7 @@ sub ToggleLowestPriority {
         $borrowernumber,
     );
     $sth->finish;
-    
+
     _FixPriority( $biblionumber, $borrowernumber, '999999' );
 }
 
@@ -1612,7 +1641,7 @@ the array index (+1 as array starts from 0)
 and if $rank is supplied will splice item from the array and splice it back in again
 in new priority rank
 
-=cut 
+=cut
 
 sub _FixPriority {
     my ( $biblio, $borrowernumber, $rank, $ignoreSetLowestRank ) = @_;
@@ -1691,10 +1720,10 @@ sub _FixPriority {
         );
         $sth->finish;
     }
-    
+
     $sth = $dbh->prepare( "SELECT borrowernumber FROM reserves WHERE lowestPriority = 1 ORDER BY priority" );
     $sth->execute();
-    
+
     unless ( $ignoreSetLowestRank ) {
       while ( my $res = $sth->fetchrow_hashref() ) {
         _FixPriority( $biblio, $res->{'borrowernumber'}, '999999', 1 );
@@ -1754,7 +1783,7 @@ sub _Findgroupreserve {
         push( @results, $data );
     }
     return @results if @results;
-    
+
     # check for title-level targetted match
     my $title_level_target_query = qq/
         SELECT reserves.biblionumber        AS biblionumber,
@@ -1833,7 +1862,7 @@ sub _koha_notify_reserve {
 
     my $dbh = C4::Context->dbh;
     my $borrower = C4::Members::GetMember(borrowernumber => $borrowernumber);
-    
+
     # Try to get the borrower's email address
     my $to_address;
     my $which_address = C4::Context->preference('AutoEmailPrimaryAddress');
@@ -1843,7 +1872,7 @@ sub _koha_notify_reserve {
     } else {
         $to_address = $borrower->{$which_address};
     }
-    
+
     my $letter_code;
     my $print_mode = 0;
     my $messagingprefs;
@@ -1888,7 +1917,7 @@ sub _koha_notify_reserve {
             borrowernumber => $borrowernumber,
             message_transport_type => 'print',
         } );
-        
+
         return;
     }
 
@@ -1967,6 +1996,58 @@ sub _ShiftPriorityByDateAndPriority {
     return $new_priority;  # so the caller knows what priority they wind up receiving
 }
 
+=head2 OPACItemHoldsAllowed
+
+  OPACItemHoldsAllowed($item_record,$borrower_record);
+
+Checks issuingrules, using the borrowers categorycode, the itemtype, and branchcode to see
+if specific item holds are allowed, returns true if so.
+
+=cut
+
+sub OPACItemHoldsAllowed {
+    my ($item,$borrower) = @_;
+
+    my $branchcode = $item->{homebranch} or die "No homebranch";
+    my $itype;
+    my $dbh = C4::Context->dbh;
+    if (C4::Context->preference('item-level_itypes')) {
+       # We cant trust GetItem to honour the syspref, so safest to do it ourselves
+       # When GetItem is fixed, we can remove this
+       $itype = $item->{itype};
+    }
+    else {
+       my $query = "SELECT itemtype FROM biblioitems WHERE biblioitemnumber = ? ";
+       my $sth = $dbh->prepare($query);
+       $sth->execute($item->{biblioitemnumber});
+       if (my $data = $sth->fetchrow_hashref()){
+           $itype = $data->{itemtype};
+       }
+    }
+
+    my $query = "SELECT opacitemholds,categorycode,itemtype,branchcode FROM issuingrules WHERE
+          (issuingrules.categorycode = ? OR issuingrules.categorycode = '*')
+        AND
+          (issuingrules.itemtype = ? OR issuingrules.itemtype = '*')
+        AND
+          (issuingrules.branchcode = ? OR issuingrules.branchcode = '*')
+        ORDER BY
+          issuingrules.categorycode desc,
+          issuingrules.itemtype desc,
+          issuingrules.branchcode desc
+       LIMIT 1";
+    my $dbh = C4::Context->dbh;
+    my $sth = $dbh->prepare($query);
+    $sth->execute($borrower->{categorycode},$itype,$branchcode);
+    my $data = $sth->fetchrow_hashref;
+    if ($data->{opacitemholds}){
+       return 1;
+    }
+    else {
+       return 0;
+    }
+}
+
 =head2 MoveReserve
 
   MoveReserve( $itemnumber, $borrowernumber, $cancelreserve )
diff --git a/C4/VirtualShelves/Page.pm b/C4/VirtualShelves/Page.pm
index 988244c..1d984ed 100644
--- a/C4/VirtualShelves/Page.pm
+++ b/C4/VirtualShelves/Page.pm
@@ -238,7 +238,6 @@ sub shelfpage {
             # explicitly fetch this shelf
             my ($shelfnumber2,$shelfname,$owner,$category,$sorton) = GetShelf($shelfnumber);
 
-            $template->param( 'AllowOnShelfHolds' => C4::Context->preference('AllowOnShelfHolds') );
             if (C4::Context->preference('TagsEnabled')) {
                 $template->param(TagsEnabled => 1);
                     foreach (qw(TagsShowOnList TagsInputOnList)) {
diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl
index f45ea43..faf9781 100755
--- a/admin/smart-rules.pl
+++ b/admin/smart-rules.pl
@@ -101,12 +101,12 @@ elsif ($op eq 'delete-branch-item') {
 # save the values entered
 elsif ($op eq 'add') {
     my $sth_search = $dbh->prepare('SELECT COUNT(*) AS total FROM issuingrules WHERE branchcode=? AND categorycode=? AND itemtype=?');
-    my $sth_insert = $dbh->prepare('INSERT INTO issuingrules (branchcode, categorycode, itemtype, maxissueqty, renewalsallowed, reservesallowed, issuelength, lengthunit, hardduedate, hardduedatecompare, fine, finedays, firstremind, chargeperiod,rentaldiscount, overduefinescap) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)');
-    my $sth_update=$dbh->prepare("UPDATE issuingrules SET fine=?, finedays=?, firstremind=?, chargeperiod=?, maxissueqty=?, renewalsallowed=?, reservesallowed=?, issuelength=?, lengthunit = ?, hardduedate=?, hardduedatecompare=?, rentaldiscount=?, overduefinescap=?  WHERE branchcode=? AND categorycode=? AND itemtype=?");
+    my $sth_insert = $dbh->prepare('INSERT INTO issuingrules (branchcode, categorycode, itemtype, maxissueqty, renewalsallowed, reservesallowed, issuelength, lengthunit, hardduedate, hardduedatecompare, fine, finedays, firstremind, chargeperiod,rentaldiscount, onshelfholds, opacitemholds, reservesmaxpickupdelay, overduefinescap) VALUES(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)');
+    my $sth_update=$dbh->prepare("UPDATE issuingrules SET fine=?, finedays=?, firstremind=?, chargeperiod=?, maxissueqty=?, renewalsallowed=?, reservesallowed=?, issuelength=?, lengthunit=?, hardduedate=?, hardduedatecompare=?, rentaldiscount=?, onshelfholds=?, opacitemholds=?, reservesmaxpickupdelay=?, overduefinescap=? WHERE branchcode=? AND categorycode=? AND itemtype=?");
     
     my $br = $branch; # branch
     my $bor  = $input->param('categorycode'); # borrower category
-    my $cat  = $input->param('itemtype');     # item type
+    my $itemtype  = $input->param('itemtype');     # item type
     my $fine = $input->param('fine');
     my $finedays     = $input->param('finedays');
     my $firstremind  = $input->param('firstremind');
@@ -114,6 +114,7 @@ elsif ($op eq 'add') {
     my $maxissueqty  = $input->param('maxissueqty');
     my $renewalsallowed  = $input->param('renewalsallowed');
     my $reservesallowed  = $input->param('reservesallowed');
+    my $onshelfholds     = $input->param('onshelfholds') || 0;
     $maxissueqty =~ s/\s//g;
     $maxissueqty = undef if $maxissueqty !~ /^\d+/;
     my $issuelength  = $input->param('issuelength');
@@ -122,15 +123,17 @@ elsif ($op eq 'add') {
     $hardduedate = format_date_in_iso($hardduedate);
     my $hardduedatecompare = $input->param('hardduedatecompare');
     my $rentaldiscount = $input->param('rentaldiscount');
+    my $reservesmaxpickupdelay = $input->param('reservesmaxpickupdelay');
+    my $opacitemholds = $input->param('opacitemholds') || 0;
     my $overduefinescap = $input->param('overduefinescap') || undef;
-    $debug and warn "Adding $br, $bor, $cat, $fine, $maxissueqty";
+    $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty";
 
-    $sth_search->execute($br,$bor,$cat);
+    $sth_search->execute($br,$bor,$itemtype);
     my $res = $sth_search->fetchrow_hashref();
     if ($res->{total}) {
-        $sth_update->execute($fine, $finedays,$firstremind, $chargeperiod, $maxissueqty, $renewalsallowed,$reservesallowed, $issuelength,$lengthunit, $hardduedate,$hardduedatecompare,$rentaldiscount,$overduefinescap, $br,$bor,$cat);
+        $sth_update->execute($fine, $finedays,$firstremind, $chargeperiod, $maxissueqty, $renewalsallowed,$reservesallowed, $issuelength,$lengthunit, $hardduedate,$hardduedatecompare,$rentaldiscount, $onshelfholds, $opacitemholds, $reservesmaxpickupdelay, $overduefinescap, $br,$bor,$itemtype);
     } else {
-        $sth_insert->execute($br,$bor,$cat,$maxissueqty,$renewalsallowed,$reservesallowed,$issuelength,$lengthunit,$hardduedate,$hardduedatecompare,$fine,$finedays,$firstremind,$chargeperiod,$rentaldiscount,$overduefinescap);
+        $sth_insert->execute($br,$bor,$itemtype,$maxissueqty,$renewalsallowed,$reservesallowed,$issuelength,$lengthunit,$hardduedate,$hardduedatecompare,$fine,$finedays,$firstremind,$chargeperiod,$rentaldiscount,$onshelfholds,$opacitemholds,$reservesmaxpickupdelay,$overduefinescap);
     }
 } 
 elsif ($op eq "set-branch-defaults") {
@@ -385,7 +388,7 @@ while (my $row = $sth2->fetchrow_hashref) {
     $row->{'humancategorycode'} ||= $row->{'categorycode'};
     $row->{'default_humancategorycode'} = 1 if $row->{'humancategorycode'} eq '*';
     $row->{'fine'} = sprintf('%.2f', $row->{'fine'});
-    if ($row->{'hardduedate'} ne '0000-00-00') {
+    if ($row->{'hardduedate'} && $row->{'hardduedate'} ne '0000-00-00') {
        $row->{'hardduedate'} = format_date( $row->{'hardduedate'});
        $row->{'hardduedatebefore'} = 1 if ($row->{'hardduedatecompare'} == -1);
        $row->{'hardduedateexact'} = 1 if ($row->{'hardduedatecompare'} ==  0);
diff --git a/admin/systempreferences.pl b/admin/systempreferences.pl
index 175f6a0..6686904 100755
--- a/admin/systempreferences.pl
+++ b/admin/systempreferences.pl
@@ -186,7 +186,6 @@ $tabsysprefs{HomeOrHoldingBranch}            = "Circulation";
 $tabsysprefs{HomeOrHoldingBranchReturn}      = "Circulation";
 $tabsysprefs{RandomizeHoldsQueueWeight}      = "Circulation";
 $tabsysprefs{StaticHoldsQueueWeight}         = "Circulation";
-$tabsysprefs{AllowOnShelfHolds}              = "Circulation";
 $tabsysprefs{AllowHoldsOnDamagedItems}       = "Circulation";
 $tabsysprefs{UseBranchTransferLimits}        = "Circulation";
 $tabsysprefs{AllowHoldPolicyOverride}        = "Circulation";
@@ -367,7 +366,6 @@ $tabsysprefs{suggestion}           = "OPAC";
 $tabsysprefs{OpacTopissue}         = "OPAC";
 $tabsysprefs{OpacBrowser}          = "OPAC";
 $tabsysprefs{OpacRenewalAllowed}   = "OPAC";
-$tabsysprefs{OPACItemHolds}        = "OPAC";
 $tabsysprefs{OPACGroupResults}     = "OPAC";
 $tabsysprefs{XSLTDetailsDisplay}   = "OPAC";
 $tabsysprefs{XSLTResultsDisplay}   = "OPAC";
diff --git a/installer/data/mysql/it-IT/necessari/system_preferences.sql b/installer/data/mysql/it-IT/necessari/system_preferences.sql
index b4ddfa0..7aaa066 100644
--- a/installer/data/mysql/it-IT/necessari/system_preferences.sql
+++ b/installer/data/mysql/it-IT/necessari/system_preferences.sql
@@ -17,7 +17,6 @@
 -- 51 Franklin Street' WHERE variable = ' Fifth Floor' WHERE variable = ' Boston' WHERE variable = ' MA 02110-1301 USA.
 
 UPDATE systempreferences SET value = 'cataloguing' WHERE variable = 'AcqCreateItem';
-UPDATE systempreferences SET value = '1' WHERE variable = 'AllowOnShelfHolds';
 UPDATE systempreferences SET value = '1' WHERE variable = 'AllowRenewalLimitOverride';
 UPDATE systempreferences SET value = 'annual' WHERE variable = 'autoBarcode';
 UPDATE systempreferences SET value = 'email' WHERE variable = 'AutoEmailPrimaryAddress';
diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql
index 8fd20a4..d213545 100644
--- a/installer/data/mysql/kohastructure.sql
+++ b/installer/data/mysql/kohastructure.sql
@@ -1001,6 +1001,9 @@ CREATE TABLE `issuingrules` ( -- circulation and fine rules
   `reservesallowed` smallint(6) NOT NULL default "0", -- how many holds are allowed
   `branchcode` varchar(10) NOT NULL default '', -- the branch this rule is for (branches.branchcode)
   overduefinescap decimal default NULL, -- the maximum amount of an overdue fine
+  onshelfholds tinyint(1) NOT NULL default 0, -- allow holds for items that are on shelf
+  opacitemholds tinyint(1) NOT NULL default "0", -- allow opac users to place specific items on hold
+  reservesmaxpickupdelay smallint(6) default NULL, -- max pickup delay
   PRIMARY KEY  (`branchcode`,`categorycode`,`itemtype`),
   KEY `categorycode` (`categorycode`),
   KEY `itemtype` (`itemtype`)
diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql
index 7b33ce6..be0f802 100644
--- a/installer/data/mysql/sysprefs.sql
+++ b/installer/data/mysql/sysprefs.sql
@@ -187,7 +187,6 @@ INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES
 INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('OAI-PMH:archiveID','KOHA-OAI-TEST','OAI-PMH archive identification',NULL,'Free');
 INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('OAI-PMH:MaxCount','50','OAI-PMH maximum number of records by answer to ListRecords and ListIdentifiers queries',NULL,'Integer');
 INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('OAI-PMH:ConfFile','','If empty, Koha OAI Server operates in normal mode, otherwise it operates in extended mode.',NULL,'File');
-INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('OPACItemHolds','1','Allow OPAC users to place hold on specific items. If OFF, users can only request next available copy.','','YesNo');
 
 INSERT INTO `systempreferences` (variable, value,options,type, explanation) VALUES ('AddPatronLists','categorycode','categorycode|category_type','Choice','Allow user to choose what list to pick up from when adding patrons');
 INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('ExtendedPatronAttributes','0','Use extended patron IDs and attributes',NULL,'YesNo');
@@ -220,7 +219,6 @@ INSERT INTO systempreferences (variable,value,options,explanation,type) VALUES
 ('XSLTDetailsDisplay','','','Enable XSL stylesheet control over details page display on intranet','Free'),
 ('XSLTResultsDisplay','','','Enable XSL stylesheet control over results page display on intranet','Free');
 INSERT INTO `systempreferences` (variable,value,options,explanation,type) VALUES('AdvancedSearchTypes','itemtypes','itemtypes|ccode','Select which set of fields comprise the Type limit in the advanced search','Choice');
-INSERT INTO `systempreferences` (variable,value,options,explanation,type) VALUES('AllowOnShelfHolds', '0', '', 'Allow hold requests to be placed on items that are not on loan', 'YesNo');
 INSERT INTO `systempreferences` (variable,value,options,explanation,type) VALUES('AllowHoldsOnDamagedItems', '1', '', 'Allow hold requests to be placed on damaged items', 'YesNo');
 INSERT INTO `systempreferences` (variable,value,options,explanation,type) VALUES('OpacSuppression', '0', '', 'Turn ON the OPAC Suppression feature, requires further setup, ask your system administrator for details', 'YesNo');
 -- FIXME: add FrameworksLoaded, noOPACUserLogin?
diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl
index 5d6f7db..bc11d4d 100755
--- a/installer/data/mysql/updatedatabase.pl
+++ b/installer/data/mysql/updatedatabase.pl
@@ -2092,7 +2092,6 @@ if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
     $dbh->do("UPDATE `systempreferences` SET options='10' WHERE variable='globalDueDate'");
     $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE variable='numSearchResults'");
     $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE variable='OPACnumSearchResults'");
-    $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE variable='ReservesMaxPickupDelay'");
     $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE variable='TransfersMaxDaysWarning'");
     $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE variable='StaticHoldsQueueWeight'");
     $dbh->do("UPDATE `systempreferences` SET type='Integer' WHERE variable='holdCancelLength'");
@@ -5859,6 +5858,41 @@ if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
     SetVersion($DBversion);
 }
 
+
+
+$DBversion = '3.09.00.XXX';
+if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
+    # First create the column
+    $dbh->do("ALTER TABLE issuingrules ADD onshelfholds tinyint(1) default 0");
+    # Now update the column
+    if (C4::Context->preference("AllowOnShelfHolds")){
+        # Pref is on, set allow for all rules
+        $dbh->do("UPDATE issuingrules SET onshelfholds=1");
+    } else {
+        # If the preference is not set, leave off
+        $dbh->do("UPDATE issuingrules SET onshelfholds=0");
+    }
+    $dbh->do("ALTER TABLE issuingrules MODIFY onshelfholds tinyint(1) default 0 NOT NULL");
+    # Remove from the systempreferences table
+    $dbh->do("DELETE FROM systempreferences WHERE variable = 'AllowOnShelfHolds'");
+
+    # First create the column
+    $dbh->do("ALTER TABLE issuingrules ADD opacitemholds tinyint(1) DEFAULT 0");
+    # Now update the column
+    if (C4::Context->preference("OPACItemHolds")){
+       # Pref is on, set allow for all rules
+       $dbh->do("UPDATE issuingrules SET opacitemholds=1");
+    }
+    # If the preference is not set, leave off
+    # Remove from the systempreferences table
+    $dbh->do("DELETE FROM systempreferences WHERE variable = 'OPACItemHolds'");
+
+    $dbh->do("ALTER TABLE issuingrules ADD reservesmaxpickupdelay smallint(6) DEFAULT NULL");
+
+    print "Upgrade to $DBversion done (Move AllowOnShelfHolds to circulation matrix; Move OPACItemHolds system preference to circulation matrix; ReservesMaxPickupDelay circulation rule)\n";
+    SetVersion ($DBversion);
+}
+
 =head1 FUNCTIONS
 
 =head2 TableExists($table)
diff --git a/installer/html-template-to-template-toolkit.pl b/installer/html-template-to-template-toolkit.pl
index e99b195..5f0e0ac 100755
--- a/installer/html-template-to-template-toolkit.pl
+++ b/installer/html-template-to-template-toolkit.pl
@@ -32,7 +32,7 @@ my @globals = ("themelang","JacketImages","OPACAmazonCoverImages","GoogleJackets
 "SyndeticsEnabled", "OpacRenewalAllowed", "item_level_itypes","noItemTypeImages",
 "virtualshelves", "RequestOnOpac", "COinSinOPACResults", "OPACXSLTResultsDisplay",
 "OPACItemsResultsDisplay", "LibraryThingForLibrariesID", "opacuserlogin", "TagsEnabled",
-"TagsShowOnList", "TagsInputOnList","loggedinusername","AllowOnShelfHolds","opacbookbag",
+"TagsShowOnList", "TagsInputOnList","loggedinusername","opacbookbag",
 "OPACAmazonEnabled", "SyndeticsCoverImages","using_https");
 
 # Arguments:
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 531b878..103b8b9 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
@@ -299,12 +299,6 @@ Circulation:
                   no: "Don't allow"
             - hold requests to be placed on damaged items.
         -
-            - pref: AllowOnShelfHolds
-              choices:
-                  yes: Allow
-                  no: "Don't allow"
-            - hold requests to be placed on items that are not checked out.
-        -
             - pref: AllowHoldDateInFuture
               choices:
                   yes: Allow
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
index e645785..6386213 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ -401,12 +401,6 @@ OPAC:
 #              choices:
 #            - If ON, enables subject cloud on OPAC
         -
-            - pref: OPACItemHolds
-              choices:
-                  yes: Allow
-                  no: "Don't allow"
-            - patrons to place holds on specific items in the OPAC. If this is disabled, users can only put a hold on the next available item.
-        -
             - pref: OpacRenewalAllowed
               choices:
                   yes: Allow
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
index b2f62f7..429f33d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
@@ -76,57 +76,46 @@ for="tobranch"><strong>Clone these rules to:</strong></label> <input type="hidde
                 <th>Suspension in days (day)</th>
                 <th>Renewals allowed (count)</th>
                 <th>Holds allowed (count)</th>
-		<th>Rental discount (%)</th>
-				<th>&nbsp;</th>
+                <th>On Shelf Holds Allowed</th>
+                <th>Allow OPAC Users to place Holds on Items</th>
+                <th>Holds Max Pickup Delay (day)</th>
+                <th>Rental discount (%)</th>
+                <th>&nbsp;</th>
             </tr>
-				[% FOREACH rule IN rules %]
-					[% UNLESS ( loop.odd ) %]
-					<tr class="highlight">
-					[% ELSE %]
-					<tr>
-					[% END %]
-							<td>[% IF ( rule.default_humancategorycode ) %]
-									<em>All</em>
-								[% ELSE %]
-									[% rule.humancategorycode %]
-								[% END %]
-							</td>
-							<td>[% IF ( rule.default_humanitemtype ) %]
-									<em>All</em>
-								[% ELSE %]
-									[% rule.humanitemtype %]
-								[% END %]
-							</td>
-							<td>[% IF ( rule.unlimited_maxissueqty ) %]
-									Unlimited
-								[% ELSE %]
-									[% rule.maxissueqty %]
-								[% END %]
-							</td>
-							<td>[% rule.issuelength %]</td>
-							<td>
-							    [% rule.lengthunit %]
-							</td>
-                                                        <td>[% IF ( rule.hardduedate ) %]
-                                                               [% IF ( rule.hardduedatebefore ) %]before [% rule.hardduedate %]</td>
-                                                               [% ELSE %][% IF ( rule.hardduedateexact ) %]on [% rule.hardduedate %]</td>
-                                                                                 [% ELSE %][% IF ( rule.hardduedateafter ) %]after [% rule.hardduedate %]</td>[% END %]
-                                                                                 [% END %]
-                                                               [% END %]
-                                                            [% ELSE %]None defined[% END %]   
-							<td>[% rule.fine %]</td>
-							<td>[% rule.chargeperiod %]</td>
-							<td>[% rule.firstremind %]</td>
-                            <td>[% rule.overduefinescap FILTER format("%.2f") %]</td>
-							<td>[% rule.finedays %]</td>
-							<td>[% rule.renewalsallowed %]</td>
-							<td>[% rule.reservesallowed %]</td>
-							<td>[% rule.rentaldiscount %]</td>
-							<td>
-								<a class="button" href="/cgi-bin/koha/admin/smart-rules.pl?op=delete&amp;itemtype=[% rule.itemtype %]&amp;categorycode=[% rule.categorycode %]&amp;branch=[% rule.current_branch %]">Delete</a>
-							</td>
-                	</tr>
-            	[% END %]
+        [% FOREACH rule IN rules %]
+            [% IF ( loop.odd ) %] <tr> [% ELSE %] <tr class="highlight"> [% END %]
+                <td>[% IF ( rule.default_humancategorycode ) %]<em>All</em>[% ELSE %][% rule.humancategorycode %][% END %]</td>
+                <td>[% IF ( rule.default_humanitemtype ) %]<em>All</em>[% ELSE %][% rule.humanitemtype %][% END %]</td>
+                <td>[% IF ( rule.unlimited_maxissueqty ) %] Unlimited [% ELSE %][% rule.maxissueqty %][% END %]</td>
+                <td>[% rule.issuelength %]</td>
+                <td>[% rule.lengthunit %]</td>
+                <td>
+                [% IF ( rule.hardduedate ) %]
+                    [% IF ( rule.hardduedatebefore ) %]
+                        before [% rule.hardduedate %]
+                    [% ELSIF ( rule.hardduedateexact ) %]
+                        on [% rule.hardduedate %]
+                    [% ELSIF ( rule.hardduedateafter ) %]
+                        after [% rule.hardduedate %]
+                    [% END %]
+                [% ELSE %]
+                    None defined
+                [% END %]   
+                </td>
+                <td>[% rule.fine %]</td>
+                <td>[% rule.chargeperiod %]</td>
+                <td>[% rule.firstremind %]</td>
+                <td>[% rule.overduefinescap FILTER format("%.2f") %]</td>
+                <td>[% rule.finedays %]</td>
+                <td>[% rule.renewalsallowed %]</td>
+                <td>[% rule.reservesallowed %]</td>
+                <td>[% IF rule.onshelfholds %]Yes[% ELSE %]No[% END %]</td>
+                <td>[% IF rule.opacitemholds %]Yes[% ELSE %]No[% END %]</td>
+                <td>[% rule.reservesmaxpickupdelay %]</td>
+                <td>[% rule.rentaldiscount %]</td>
+                <td> <a class="button" href="/cgi-bin/koha/admin/smart-rules.pl?op=delete&amp;itemtype=[% rule.itemtype %]&amp;categorycode=[% rule.categorycode %]&amp;branch=[% rule.current_branch %]">Delete</a> </td>
+             </tr>
+        [% END %]
                 <tr>
                     <td>
                         <select name="categorycode">
@@ -167,7 +156,10 @@ for="tobranch"><strong>Clone these rules to:</strong></label> <input type="hidde
                     <td><input name="finedays" size="3" /> </td>
                     <td><input name="renewalsallowed" size="2" /></td>
                     <td><input name="reservesallowed" size="2" /></td>
-		    <td><input name="rentaldiscount" size="2" /></td>
+                    <td><input type="checkbox" name="onshelfholds" value="1" /></td>
+                    <td><input type="checkbox" name="opacitemholds" value="1" /></td>
+                    <td><input name="reservesmaxpickupdelay" size="2" /></td>
+                    <td><input name="rentaldiscount" size="2" /></td>
                     <td><input type="hidden" name="branch" value="[% current_branch %]"/><input type="submit" value="Add" class="submit" /></td>
                 </tr>
             </table>
diff --git a/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc b/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc
index 0f16f3a..3e79d46 100644
--- a/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc
+++ b/koha-tmpl/opac-tmpl/prog/en/includes/opac-detail-sidebar.inc
@@ -2,13 +2,7 @@
     [% UNLESS ( norequests ) %]
         [% IF ( opacuserlogin ) %]
             [% IF ( RequestOnOpac ) %]
-                [% IF ( AllowOnShelfHolds ) %]
-                    <li><a class="reserve" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% biblionumber %]">Place hold</a></li>
-                [% ELSE %]
-                    [% IF ( ItemsIssued ) %]
-                        <li><a class="reserve" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% biblionumber %]">Place hold</a></li>
-                    [% END %]
-                [% END %]
+                <li><a class="reserve" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% biblionumber %]">Place hold</a></li>
             [% END %]
         [% END %]
     [% END %]
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 a5f3602..00f7c75 100644
--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt
@@ -209,23 +209,22 @@
             [% IF ( bad_data ) %]
               <div id="bad_data" class="dialog alert">ERROR: Internal error: incomplete hold request.</div>
             [% END %]
-          [% ELSE %]
-            [% IF ( none_available ) %]
+          [% ELSIF ( none_available ) %]
                 <div id="none_available" class="dialog alert"><strong>Sorry</strong>, none of these items can be placed on hold.
                 </div>
-              [% 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) %]
+                    </h3>
+          [% IF (RESERVE_CHARGE) %]
 	      <div class="dialog alert" id="reserve_fee">
 	        There is a charge of [% RESERVE_CHARGE %] for placing this hold
 	      </div>
-	      [% END %]
+          [% END %]
+      [% END %][% END %]
 
             <form action="/cgi-bin/koha/opac-reserve.pl" method="post" id="hold-request-form">
             <input type="hidden" name="place_reserve" value="1"/>
@@ -234,38 +233,47 @@
             <input type="hidden" name="biblionumbers" id="biblionumbers"/>
             <input type="hidden" name="selecteditems" id="selections"/>
             <div id="bigloop">
+            [% extra_cols = 2 %]
               <table id="bibitemloop">
-                [% UNLESS ( none_available ) %]<tr>
+                <tr>
+                [% UNLESS ( none_available ) %]
                   <th>Hold</th>
+                [% END %]
                   <th>Title</th>
-                  [% UNLESS ( item_level_itypes ) %]
+                [% UNLESS ( item_level_itypes ) %]
                     <th>Item type</th>
-                  [% END %]
+                [% END %]
+                [% UNLESS ( none_available ) %]
                   [% IF showholds && showpriority %]
+                    [% extra_cols = extra_cols + 1 %]
                   <th>Holds and priority</th>
                   [% ELSIF showholds %]
+                    [% extra_cols = extra_cols + 1 %]
                   <th>Holds</th>
                   [% ELSIF showpriority %]
+                    [% extra_cols = extra_cols + 1 %]
                   <th>Priority</th>
                   [% END %]
 		  [% IF ( reserve_in_future ) %]
+                    [% extra_cols = extra_cols + 1 %]
         <th>Hold starts on date</th>
 		  [% END %]
         <th>Hold not needed after</th>
-                  [% IF ( OPACItemHolds ) %]
                     <th id="place_on_hdr" style="display:none">Place on</th>
-                  [% END %]
                   [% UNLESS ( singleBranchMode ) %]
 		    [% IF ( choose_branch ) %]
+                      [% extra_cols = extra_cols + 1 %]
                         <th>Pickup location</th>
 		    [% END %]
                   [% END %]
-                </tr>[% ELSE %]<tr><th colspan="5">Title</th></tr>[% END %]
+                [% END %]
+                </tr>
 
                 [% FOREACH bibitemloo IN bibitemloop %]
-                  <tr>
-                      [% IF ( bibitemloo.holdable ) %]
-                                    <td class="hold">
+                <tr>
+                    [% UNLESS none_available %]
+                    <td class="hold">
+                        [% IF ( bibitemloo.holdable ) %]
                       <input class="reserve_mode" name="reserve_mode" type="hidden" value="single"/>
                       <input class="single_bib" name="single_bib" type="hidden" value="[% bibitemloo.biblionumber %]"/>
                         <span class="confirmjs_hold" title="[% bibitemloo.biblionumber %]"></span>
@@ -276,38 +284,35 @@
                                  value="any" />
                           <label class="confirm_label" for="[% bibitemloo.checkitem_bib %]">Next available copy</label>
                         </span>
-					</td>
-                      [% ELSE %]
-                                      [% UNLESS ( none_available ) %]<td class="hold">&nbsp;</td>[% END %]
-                      [% END %]
-                    [% IF ( bibitemloo.holdable ) %]<td class="title">[% ELSE %]<td class="title" colspan="5">[% END %]
+                        [% ELSE %]
+                        &nbsp;
+                        [% END %]
+                    </td>
+                    [% END %]
+                    <td class="title">
                       <a 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 %]
 
                       [% UNLESS ( bibitemloo.holdable ) %]
-
+                          <div class="bibmessage">
                         [% IF ( bibitemloo.already_reserved ) %]
-                          <div class="bibmessage">You have already requested this title.</div>
+                            You have already requested this title.
+                        [% ELSIF ( bibitemloo.bib_available ) %]
+                            No available items.
                         [% ELSE %]
-                          [% UNLESS ( bibitemloo.bib_available ) %]
-                            <div class="bibmessage">No available items.</div>
-                          [% ELSE %]
-                            <div class="bibmessage">This title cannot be requested.</div>
-                          [% END %]
-                        [% END %]
-
-
+                            This title cannot be requested.
                         [% END %]
-
+                          </div>
+                      [% END %]
                     </td>
-                    [% IF ( bibitemloo.holdable ) %]
-            <!-- HOLDABLE -->
                         [% UNLESS ( item_level_itypes ) %]
                         <td class="itype">
                             [% IF ( bibitemloo.imageurl ) %]<img src="[% bibitemloo.imageurl %]" alt="" />[% END %]
                             [% bibitemloo.description %]
                         </td>
                         [% END %]
+                    [% IF ( bibitemloo.holdable ) %]
+            <!-- HOLDABLE -->
                         [% IF showholds || showpriority %]
                         <td class="priority">
                         [% IF showpriority %] [% bibitemloo.rank %] [% END %]
@@ -332,68 +337,36 @@
         <input name="expiration_date_[% bibitemloo.biblionumber %]" id="to" size="10" readonly="readonly" class="datepickerto" />
       <p style="margin:.3em 2em;">
       <a href="#" style="font-size:85%;text-decoration:none;" onclick="document.getElementById('expiration_date_[% bibitemloo.biblionumber %]').value='';return false;">Clear date</a></p>
-    </td>[% END %]
+    </td>
 
-                    [% IF ( bibitemloo.holdable ) %]
-		    <!-- HOLD ABLE -->
-		    [% IF ( OPACItemHolds ) %]
+                        <td class="place_on_type" style="display:none">
 		    <!-- ITEM HOLDS -->
-                                          <td class="place_on_type" style="display:none">
-                                            <ul>
-                                                <li>
-                                                  [% UNLESS ( bibitemloo.holdable ) %]
-                                                    <input type="radio" name="reqtype_[% bibitemloo.biblionumber %]"
-                                                           id="reqany_[% bibitemloo.biblionumber %]"
-                                                           class="selectany"
-                                                           value="Any"
-                                                           disabled="disabled"
-                                                    />
-                                                  [% ELSE %]
-                                                    <input type="radio" name="reqtype_[% bibitemloo.biblionumber %]"
-                                                           id="reqany_[% bibitemloo.biblionumber %]"
-                                                           class="selectany"
-                                                           value="Any"
-                                                           checked="checked"
-                                                    />
-                                                  [% END %]
-                                                  <label for="reqany_[% bibitemloo.biblionumber %]">Next available copy</label>
-                                                </li>
-                                                <li>
-                                                  [% UNLESS ( bibitemloo.holdable ) %]
-                                                    <input type="radio" name="reqtype_[% bibitemloo.biblionumber %]"
-                                                           id="reqspecific_[% bibitemloo.biblionumber %]"
-                                                           class="selectspecific"
-                                                           disabled="disabled"
-                                                           value="Specific"
-                                                    />
-                                                  [% ELSE %]
-                                                    <input type="radio" name="reqtype_[% bibitemloo.biblionumber %]"
-                                                           id="reqspecific_[% bibitemloo.biblionumber %]"
-                                                           class="selectspecific"
-                                                           value="Specific"
-                                                    />
-                                                  [% END %]
-                                                  <label for="reqspecific_[% bibitemloo.biblionumber %]">A specific copy</label>
-                                                </li>
-                                            </ul>
-                                          </td>
-                                        [% END %][% END %]
-
-                    [% UNLESS ( singleBranchMode ) %]
-                        [% IF ( bibitemloo.holdable ) %]
+                          <ul>
+                              <li>
+                                  <input type="radio" name="reqtype_[% bibitemloo.biblionumber %]"
+                                         id="reqany_[% bibitemloo.biblionumber %]"
+                                         class="selectany"
+                                         value="Any"
+                                         checked="checked"
+                                  />
+                                <label for="reqany_[% bibitemloo.biblionumber %]">Next available copy</label>
+                              </li>
+                        [% IF ( bibitemloo.itemholdable ) %]
+                              <li>
+                                  <input type="radio" name="reqtype_[% bibitemloo.biblionumber %]"
+                                         id="reqspecific_[% bibitemloo.biblionumber %]"
+                                         class="selectspecific"
+                                         value="Specific"
+                                  />
+                                <label for="reqspecific_[% bibitemloo.biblionumber %]">A specific copy</label>
+                              </li>
+                        [% END %]
+                          </ul>
+                        </td>
+
+                        [% UNLESS ( singleBranchMode ) %]
 			    [% IF ( choose_branch ) %]
-                                          <td class="branch">
-                         [% UNLESS ( bibitemloo.holdable ) %]
-                            <select name="branch" id="branch_[% bibitemloo.biblionumber %]" disabled="disabled">
-                              [% FOREACH branchChoicesLoo IN bibitemloo.branchChoicesLoop %]
-                                [% IF ( branchChoicesLoo.selected ) %]
-                                  <option value="[% branchChoicesLoo.value %]" selected="selected">[% branchChoicesLoo.branchname %]</option>
-                                [% ELSE %]
-                                  <option value="[% branchChoicesLoo.value %]">[% branchChoicesLoo.branchname %]</option>
-                                [% END %]
-                              [% END %]
-                          </select>
-                          [% ELSE %]
+                        <td class="branch">
                             <select name="branch" id="branch_[% bibitemloo.biblionumber %]">
                               [% FOREACH branchChoicesLoo IN bibitemloo.branchChoicesLoop %]
                                 [% IF ( branchChoicesLoo.selected ) %]
@@ -403,15 +376,15 @@
                                 [% END %]
                               [% END %]
                             </select>
-                          [% END %]
                        </td>
-			    [% END %]
-		        [% END %]
-                    [% END %]
+                            [% END %]
+                        [% END %]
+                    [% ELSIF NOT none_available %]
+                       <td colspan="[% extra_cols %]">&nbsp;</td>
+                    [% END # holdable%]
                   </tr>
 
-                  [% IF ( OPACItemHolds ) %]
-                  [% IF ( bibitemloo.holdable ) %]
+                    [% IF ( bibitemloo.itemholdable ) %]
                     <tr class="copiesrow" id="copiesrow_[% bibitemloo.biblionumber %]">
                       <td>&nbsp;</td>
                       <td colspan="[% itemtable_colspan %]">
@@ -490,11 +463,9 @@
                         </table>
                       </td>
                     </tr>
-                  [% END %]<!-- bib_available -->
-                  [% END %]<!-- OPACItemHolds -->
-                [% END %]
-              </table><!-- bibitemloop -->
-              [% END %] <!-- if message -->
+                    [% END # itemholdable%]
+                [% END %]<!-- bibitemloop -->
+              </table>
             </div><!-- bigloop -->
 
             [% UNLESS ( message ) %]
diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt
index 86e1a08..85db58b 100644
--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt
+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt
@@ -269,18 +269,14 @@ function highlightOn() {
 
 				<p>
                                 [% IF ( RequestOnOpac ) %]
-					[% UNLESS ( GROUP_RESULT.norequests ) %]
-						[% IF ( opacuserlogin ) %]
-							[% IF ( AllowOnShelfHolds ) %]
+                                    [% UNLESS ( GROUP_RESULT.norequests ) %]
+                                        [% IF ( opacuserlogin ) %]
+                                            [% IF ( GROUP_RESULT.holdable ) %]
                                 <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% GROUP_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue-->
-							[% ELSE %]
-								[% IF ( GROUP_RESULT.itemsissued ) %]
-                                    <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% GROUP_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue-->
-								[% END %]
-							[% END %]
-						[% END %]
-					[% END %]
-				[% END %]
+                                            [% END %]
+                                        [% END %]
+                                    [% END %]
+                                [% END %]
 
 				[% IF ( opacbookbag || virtualshelves ) %]<input type="checkbox" name="biblionumber" value="[% GROUP_RESULT.biblionumber %]" title="Click to add to cart" /> <label for="bib[% GROUP_RESULT.biblionumber %]">[% END %]<img src="[% themelang %]/images/[% GROUP_RESULT.itemtype %].gif" alt="[% GROUP_RESULT.ccode %]" title="[% GROUP_RESULT.ccode %]" />[% IF ( opacbookbag || virtualshelves ) %]</label>[% END %] [% IF ( GROUP_RESULT.classification ) %]
                                     <a href="/cgi-bin/koha/opac-search.pl?q=callnum:[% GROUP_RESULT.classification |url %]">
diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
index e5238f7..1d04cec 100644
--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
@@ -587,12 +587,8 @@ $(document).ready(function(){
                 [% IF ( RequestOnOpac ) %]
                     [% UNLESS ( SEARCH_RESULT.norequests ) %]
                         [% IF ( opacuserlogin ) %]
-                            [% IF ( AllowOnShelfHolds ) %]
+                            [% IF ( SEARCH_RESULT.holdable ) %]
                                 <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% SEARCH_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue-->
-                            [% ELSE %]
-                                [% IF ( SEARCH_RESULT.itemsissued ) %]
-                                    <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% SEARCH_RESULT.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue-->
-                                [% END %]
                             [% END %]
                         [% END %]
                     [% END %]
diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt
index 9e782fb..ed139ca 100644
--- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt
+++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt
@@ -384,7 +384,7 @@ $(function() {
       [% IF ( RequestOnOpac ) %]
           [% UNLESS ( itemsloo.norequests ) %]
             [% IF ( opacuserlogin ) %]
-              [% IF ( AllowOnShelfHolds ) %]
+              [% IF ( itemsloo.allow_onshelf_holds ) %]
                 <a class="hold" href="/cgi-bin/koha/opac-reserve.pl?biblionumber=[% itemsloo.biblionumber %]">Place hold</a><!-- add back when available 0 holds in queue-->
               [% ELSE %]
                 [% IF ( itemsloo.itemsissued ) %]
diff --git a/opac/opac-ISBDdetail.pl b/opac/opac-ISBDdetail.pl
index 773ed51..cfa149b 100755
--- a/opac/opac-ISBDdetail.pl
+++ b/opac/opac-ISBDdetail.pl
@@ -69,17 +69,13 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
 my $biblionumber = $query->param('biblionumber');
 
 # get biblionumbers stored in the cart
-my @cart_list;
-
-if($query->cookie("bib_list")){
-    my $cart_list = $query->cookie("bib_list");
-    @cart_list = split(/\//, $cart_list);
+if(my $cart_list = $query->cookie("bib_list")){
+    my @cart_list = split(/\//, $cart_list);
     if ( grep {$_ eq $biblionumber} @cart_list) {
         $template->param( incart => 1 );
     }
 }
 
-$template->param( 'AllowOnShelfHolds' => C4::Context->preference('AllowOnShelfHolds') );
 $template->param( 'ItemsIssued' => CountItemsIssued( $biblionumber ) );
 
 my $marcflavour      = C4::Context->preference("marcflavour");
@@ -162,7 +158,6 @@ foreach ( @$reviews ) {
 
 $template->param(
     RequestOnOpac       => C4::Context->preference("RequestOnOpac"),
-    AllowOnShelfHolds   => C4::Context->preference('AllowOnShelfHolds'),
     norequests   => $norequests,
     ISBD         => $res,
     biblionumber => $biblionumber,
diff --git a/opac/opac-MARCdetail.pl b/opac/opac-MARCdetail.pl
index 2d9dc25..34c9125 100755
--- a/opac/opac-MARCdetail.pl
+++ b/opac/opac-MARCdetail.pl
@@ -82,17 +82,13 @@ $template->param(
 );
 
 # get biblionumbers stored in the cart
-my @cart_list;
-
-if($query->cookie("bib_list")){
-    my $cart_list = $query->cookie("bib_list");
-    @cart_list = split(/\//, $cart_list);
+if(my $cart_list = $query->cookie("bib_list")){
+    my @cart_list = split(/\//, $cart_list);
     if ( grep {$_ eq $biblionumber} @cart_list) {
         $template->param( incart => 1 );
     }
 }
 
-$template->param( 'AllowOnShelfHolds' => C4::Context->preference('AllowOnShelfHolds') );
 $template->param( 'ItemsIssued' => CountItemsIssued( $biblionumber ) );
 
 # adding the $RequestOnOpac param
diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl
index cdb4a5a..f9a64e2 100755
--- a/opac/opac-detail.pl
+++ b/opac/opac-detail.pl
@@ -386,8 +386,6 @@ if ($session->param('busc')) {
 }
 
 
-
-$template->param( 'AllowOnShelfHolds' => C4::Context->preference('AllowOnShelfHolds') );
 $template->param( 'ItemsIssued' => CountItemsIssued( $biblionumber ) );
 
 
diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index bb23752..7bf505a 100755
--- a/opac/opac-reserve.pl
+++ b/opac/opac-reserve.pl
@@ -400,6 +400,7 @@ foreach my $biblioNum (@biblionumbers) {
 
     $biblioLoopIter{itemLoop} = [];
     my $numCopiesAvailable = 0;
+    my $numCopiesOPACAvailable = 0;
     foreach my $itemInfo (@{$biblioData->{itemInfos}}) {
         my $itemNum = $itemInfo->{itemnumber};
         my $itemLoopIter = {};
@@ -492,16 +493,26 @@ foreach my $biblioNum (@biblionumbers) {
 
         my $branch = C4::Circulation::_GetCircControlBranch($itemLoopIter, $borr);
 
-        my $branchitemrule = GetBranchItemRule( $branch, $itemInfo->{'itype'} );
-        my $policy_holdallowed = 1;
-
-        if ( $branchitemrule->{'holdallowed'} == 0 ||
-                ( $branchitemrule->{'holdallowed'} == 1 && $borr->{'branchcode'} ne $itemInfo->{'homebranch'} ) ) {
-            $policy_holdallowed = 0;
+        my $policy_holdallowed = !$itemLoopIter->{already_reserved};
+        if ($policy_holdallowed) {
+            if (my $branchitemrule = GetBranchItemRule( $branch, $itemInfo->{'itype'} )) {
+                $policy_holdallowed =
+                  ($branchitemrule->{'holdallowed'} == 2) ||
+                  ($branchitemrule->{'holdallowed'} == 1
+                      && $borr->{'branchcode'} eq $itemInfo->{'homebranch'});
+            } else {
+                $policy_holdallowed = 0; # No rule - not allowed
+            }
         }
-
-        if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and CanItemBeReserved($borrowernumber,$itemNum) and ($itemLoopIter->{already_reserved} ne 1)) {
-            $itemLoopIter->{available} = 1;
+        $policy_holdallowed &&=
+            IsAvailableForItemLevelRequest($itemInfo,$borr) &&
+            CanItemBeReserved($borrowernumber,$itemNum);
+
+        if ($policy_holdallowed) {
+            if ( OPACItemHoldsAllowed( $itemInfo, $borr ) ) {
+                $itemLoopIter->{available} = 1;
+                $numCopiesOPACAvailable++;
+            }
             $numCopiesAvailable++;
         }
 
@@ -527,18 +538,17 @@ foreach my $biblioNum (@biblionumbers) {
         $numBibsAvailable++;
         $biblioLoopIter{bib_available} = 1;
         $biblioLoopIter{holdable} = 1;
-        $anyholdable = 1;
+        $biblioLoopIter{itemholdable} = 1 if $numCopiesOPACAvailable;
     }
     if ($biblioLoopIter{already_reserved}) {
         $biblioLoopIter{holdable} = undef;
-        $anyholdable = undef;
-    }
-    if(not CanBookBeReserved($borrowernumber,$biblioNum)){
-        $biblioLoopIter{holdable} = undef;
-        $anyholdable = undef;
+        $biblioLoopIter{itemholdable} = undef;
     }
+    $biblioLoopIter{holdable} &&= CanBookBeReserved($borrowernumber,$biblioNum);
 
     push @$biblioLoop, \%biblioLoopIter;
+
+    $anyholdable = 1 if $biblioLoopIter{holdable};
 }
 
 if ( $numBibsAvailable == 0 || !$anyholdable) {
@@ -546,9 +556,6 @@ if ( $numBibsAvailable == 0 || !$anyholdable) {
 }
 
 my $itemTableColspan = 7;
-if (! $template->{VARS}->{'OPACItemHolds'}) {
-    $itemTableColspan--;
-}
 if (! $template->{VARS}->{'singleBranchMode'}) {
     $itemTableColspan--;
 }
diff --git a/opac/opac-search.pl b/opac/opac-search.pl
index 6415a29..cbbfefe 100755
--- a/opac/opac-search.pl
+++ b/opac/opac-search.pl
@@ -51,6 +51,8 @@ use C4::Tags qw(get_tags);
 use C4::Branch; # GetBranches
 use C4::SocialData;
 use C4::Ratings;
+use C4::Members;
+use C4::Reserves;
 
 use POSIX qw(ceil floor strftime);
 use URI::Escape;
@@ -124,7 +126,7 @@ if (C4::Context->preference("marcflavour") eq "UNIMARC" ) {
 elsif (C4::Context->preference("marcflavour") eq "MARC21" ) {
     $template->param('usmarc' => 1);
 }
-$template->param( 'AllowOnShelfHolds' => C4::Context->preference('AllowOnShelfHolds') );
+
 $template->param( 'OPACNoResultsFound' => C4::Context->preference('OPACNoResultsFound') );
 
 $template->param(
@@ -512,8 +514,11 @@ if ($@ || $error) {
     exit;
 }
 
+my $borrower = $borrowernumber ? GetMember( borrowernumber => $borrowernumber ) : undef;
+
 # At this point, each server has given us a result set
 # now we build that set for template display
+my %allow_onshelf_holds;
 my @sup_results_array;
 for (my $i=0;$i<@servers;$i++) {
     my $server = $servers[$i];
@@ -528,11 +533,23 @@ for (my $i=0;$i<@servers;$i++) {
                 # we need to set the offset parameter of searchResults to 0
                 my @group_results = searchResults( 'opac', $query_desc, $group->{'group_count'},$results_per_page, 0, $scan,
                                                    $group->{"RECORDS"});
+                if ($borrower) {
+                    $_->{holdable} =
+                        IsAvailableForItemLevelRequest($_, $borrower) &&
+                        OPACItemHoldsAllowed($_, $borrower)
+                      foreach @group_results;
+                }
                 push @newresults, { group_label => $group->{'group_label'}, GROUP_RESULTS => \@group_results };
             }
         } else {
             @newresults = searchResults('opac', $query_desc, $hits, $results_per_page, $offset, $scan,
                                         $results_hashref->{$server}->{"RECORDS"});
+            if ($borrower) {
+                $_->{holdable} =
+                    IsAvailableForItemLevelRequest($_, $borrower) &&
+                    OPACItemHoldsAllowed($_, $borrower)
+                  foreach @newresults;
+            }
         }
 
         # must define a value for size if not present in DB
diff --git a/reserve/request.pl b/reserve/request.pl
index c34a614..a922c8f 100755
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -456,7 +456,7 @@ foreach my $biblionumber (@biblionumbers) {
             if (
                    $policy_holdallowed
                 && !$item->{cantreserve}
-                && IsAvailableForItemLevelRequest($itemnumber)
+                && IsAvailableForItemLevelRequest($item, $borrowerinfo)
                 && CanItemBeReserved(
                     $borrowerinfo->{borrowernumber}, $itemnumber
                 )
-- 
1.7.9.5



More information about the Koha-patches mailing list