From 3b3f352a8957d89e8aad83074e959c6d1ceb440d 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 Signed-off-by: Katrin Fischer Passes tests and QA script. No regressions found, improves the ILS-DI HoldItem response. --- C4/Reserves.pm | 18 +++++++++++++++++- opac/opac-reserve.pl | 10 ---------- reserve/request.pl | 10 +--------- t/db_dependent/Holds.t | 42 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 59 insertions(+), 21 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index b82387f..763b22d 100644 --- 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 c30ee08..bfd0a30 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -519,16 +519,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 b4b749e..5978d11 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -408,19 +408,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 --- 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); -- 1.9.1