From b14d9499f8528e0797e801411e357bc95a7a0155 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Tue, 10 Feb 2015 11:34:01 +0100 Subject: [PATCH] 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 --- C4/Reserves.pm | 18 +++++++++++++++++- opac/opac-reserve.pl | 12 +----------- reserve/request.pl | 10 +--------- t/db_dependent/Holds.t | 42 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 60 insertions(+), 22 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index f405262..d5cded1 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -119,7 +119,7 @@ BEGIN { &CheckReserves &CanBookBeReserved - &CanItemBeReserved + &CanItemBeReserved &CanReserveBeCanceledFromOpac &CancelReserve &CancelExpiredReserves @@ -486,6 +486,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 @@ -577,6 +578,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 b714368..8806ed5 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -515,17 +515,7 @@ foreach my $biblioNum (@biblionumbers) { # If there is no loan, return and transfer, we show a checkbox. $itemLoopIter->{notforloan} = $itemLoopIter->{notforloan} || 0; - my $branch = GetReservesControlBranch( $itemInfo, $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; - } - - if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and CanItemBeReserved($borrowernumber,$itemNum) eq 'OK' and ($itemLoopIter->{already_reserved} ne 1)) { + if (IsAvailableForItemLevelRequest($itemNum) and CanItemBeReserved($borrowernumber,$itemNum) eq 'OK' and ($itemLoopIter->{already_reserved} ne 1)) { $itemLoopIter->{available} = 1; $numCopiesAvailable++; } diff --git a/reserve/request.pl b/reserve/request.pl index ce0e70f..01b8bda 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -411,19 +411,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($itemnumber) && CanItemBeReserved( $borrowerinfo->{borrowernumber}, $itemnumber diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 4aee01b..5a480d8 100755 --- 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); -- 2.1.0