Bug 13687: Move hold policy check into CanItemBeReserved

This way ILS-DI HoldItem and HoldTitle services also benefit from this
check

Test plan:

1/ Define some default holds policies by item type in
/admin/smart-rules.pl
2/ Use ILS-DI HoldItem service and check that those rules are respected
3/ Check that staff and opac hold behaviour is unchanged regarding
these rules.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes tests and QA script. No regressions found,
improves the ILS-DI HoldItem response.
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 5874260..9c760b6 100644 (file)
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -119,7 +119,7 @@ BEGIN {
 
         &CheckReserves
         &CanBookBeReserved
-       &CanItemBeReserved
+        &CanItemBeReserved
         &CanReserveBeCanceledFromOpac
         &CancelReserve
         &CancelExpiredReserves
@@ -488,6 +488,7 @@ sub CanBookBeReserved{
          damaged,         if the Item is damaged.
          cannotReserveFromOtherBranches, if syspref 'canreservefromotherbranches' is OK.
          tooManyReserves, if the borrower has exceeded his maximum reserve amount.
+         notReservable,   if holds on this item are not allowed
 
 =cut
 
@@ -578,6 +579,21 @@ sub CanItemBeReserved{
         return 'tooManyReserves';
     }
 
+    my $circ_control_branch = C4::Circulation::_GetCircControlBranch($item,
+        $borrower);
+    my $branchitemrule = C4::Circulation::GetBranchItemRule($circ_control_branch,
+        $item->{itype});
+
+    if ( $branchitemrule->{holdallowed} == 0 ) {
+        return 'notReservable';
+    }
+
+    if (   $branchitemrule->{holdallowed} == 1
+        && $borrower->{branchcode} ne $item->{homebranch} )
+    {
+          return 'cannotReserveFromOtherBranches';
+    }
+
     # If reservecount is ok, we check item branch if IndependentBranches is ON
     # and canreservefromotherbranches is OFF
     if ( C4::Context->preference('IndependentBranches')

diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index 5ab4c34..920916e 100755 (executable)
--- a/opac/opac-reserve.pl
+++ b/opac/opac-reserve.pl
@@ -526,16 +526,6 @@ foreach my $biblioNum (@biblionumbers) {
         my $branch = GetReservesControlBranch( $itemInfo, $borr );
 
         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
-            }
-        }
         $policy_holdallowed &&=
             IsAvailableForItemLevelRequest($itemInfo,$borr) &&
             CanItemBeReserved($borrowernumber,$itemNum) eq 'OK';

diff --git a/reserve/request.pl b/reserve/request.pl
index c30fb21..62f82a8 100755 (executable)
--- a/reserve/request.pl
+++ b/reserve/request.pl
@@ -417,19 +417,11 @@ foreach my $biblionumber (@biblionumbers) {
             my $branch = C4::Circulation::_GetCircControlBranch($item, $borrowerinfo);
 
             my $branchitemrule = GetBranchItemRule( $branch, $item->{'itype'} );
-            my $policy_holdallowed = 1;
 
             $item->{'holdallowed'} = $branchitemrule->{'holdallowed'};
 
-            if ( $branchitemrule->{'holdallowed'} == 0 ||
-                 ( $branchitemrule->{'holdallowed'} == 1 &&
-                     $borrowerinfo->{'branchcode'} ne $item->{'homebranch'} ) ) {
-                $policy_holdallowed = 0;
-            }
-            
             if (
-                   $policy_holdallowed
-                && !$item->{cantreserve}
+                   !$item->{cantreserve}
                 && IsAvailableForItemLevelRequest($item, $borrowerinfo)
                 && CanItemBeReserved(
                     $borrowerinfo->{borrowernumber}, $itemnumber

diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t
index 4aee01b..5a480d8 100755 (executable)
--- a/t/db_dependent/Holds.t
+++ b/t/db_dependent/Holds.t
@@ -6,7 +6,7 @@ use t::lib::Mocks;
 use C4::Context;
 use C4::Branch;
 
-use Test::More tests => 38;
+use Test::More tests => 41;
 use MARC::Record;
 use C4::Biblio;
 use C4::Items;
@@ -329,6 +329,46 @@ ok(
     "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)"
 );
 
+
+# Test branch item rules
+
+$dbh->do('DELETE FROM issuingrules');
+$dbh->do(
+    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed)
+      VALUES (?, ?, ?, ?)},
+    {},
+    '*', '*', '*', 25
+);
+$dbh->do('DELETE FROM branch_item_rules');
+$dbh->do('DELETE FROM default_branch_circ_rules');
+$dbh->do('DELETE FROM default_branch_item_rules');
+$dbh->do('DELETE FROM default_circ_rules');
+$dbh->do(q{
+    INSERT INTO branch_item_rules (branchcode, itemtype, holdallowed, returnbranch)
+    VALUES (?, ?, ?, ?)
+}, {}, 'CPL', 'CANNOT', 0, 'homebranch');
+$dbh->do(q{
+    INSERT INTO branch_item_rules (branchcode, itemtype, holdallowed, returnbranch)
+    VALUES (?, ?, ?, ?)
+}, {}, 'CPL', 'CAN', 1, 'homebranch');
+($bibnum, $title, $bibitemnum) = create_helper_biblio('CANNOT');
+($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem(
+    { homebranch => 'CPL', holdingbranch => 'CPL', itype => 'CANNOT' } , $bibnum);
+is(CanItemBeReserved($borrowernumbers[0], $itemnumber), 'notReservable',
+    "CanItemBeReserved should returns 'notReservable'");
+
+($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem(
+    { homebranch => 'MPL', holdingbranch => 'CPL', itype => 'CAN' } , $bibnum);
+is(CanItemBeReserved($borrowernumbers[0], $itemnumber),
+    'cannotReserveFromOtherBranches',
+    "CanItemBeReserved should returns 'cannotReserveFromOtherBranches'");
+
+($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem(
+    { homebranch => 'CPL', holdingbranch => 'CPL', itype => 'CAN' } , $bibnum);
+is(CanItemBeReserved($borrowernumbers[0], $itemnumber), 'OK',
+    "CanItemBeReserved should returns 'OK'");
+
+
 # Test CancelExpiredReserves
 C4::Context->set_preference('ExpireReservesMaxPickUpDelay', 1);
 C4::Context->set_preference('ReservesMaxPickUpDelay', 1);