From f31ce74c404bd77d43704e802090aa39683cf4d2 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Wed, 26 Mar 2014 10:51:59 +0100 Subject: [PATCH] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) This patch makes CanBookBeReserved and CanItemBeReserved to return a another variable, a hashref that describes why it cannot be reserved. It changes all calls to these functions and removes code in Perl scripts that is not needed anymore. The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. --- C4/ILSDI/Services.pm | 9 ++--- C4/Reserves.pm | 61 ++++++++++++++++++++++++++------ opac/opac-reserve.pl | 31 ++++++++--------- reserve/request.pl | 36 +++++++++---------- t/db_dependent/Holds.t | 91 +++++++++++++++++++++++++++++++++++------------- 5 files changed, 155 insertions(+), 73 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index bd201d3..60661c7 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -488,7 +488,7 @@ sub GetServices { # Reserve level management my $biblionumber = $item->{'biblionumber'}; - my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber ); + my ($canbookbereserved) = CanBookBeReserved( $borrowernumber, $biblionumber ); if ($canbookbereserved) { push @availablefor, 'title level hold'; my $canitembereserved = IsAvailableForItemLevelRequest($itemnumber); @@ -609,7 +609,8 @@ sub HoldTitle { my $title = $$biblio{title}; # Check if the biblio can be reserved - return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber ); + my ($canbookbereserved) = CanBookBeReserved($borrowernumber, $biblionumber); + return { code => 'NotHoldable' } unless $canbookbereserved; my $branch; @@ -685,8 +686,8 @@ sub HoldItem { return { code => 'RecordNotFound' } if $$item{biblionumber} ne $$biblio{biblionumber}; # Check for item disponibility - my $canitembereserved = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber ); - my $canbookbereserved = C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber ); + my ($canitembereserved) = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber ); + my ($canbookbereserved) = C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber ); return { code => 'NotHoldable' } unless $canbookbereserved and $canitembereserved; # Pickup branch management diff --git a/C4/Reserves.pm b/C4/Reserves.pm index d399ee0..08c3ca7 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -431,13 +431,33 @@ sub GetReservesFromBorrowernumber { #------------------------------------------------------------------------------------- =head2 CanBookBeReserved - $error = &CanBookBeReserved($borrowernumber, $biblionumber) + ($can_be_reserved, $reasons) = &CanBookBeReserved($borrowernumber, $biblionumber) + +$can_be_reserved is true if book can be reserved, false otherwise. + +$reasons is a hashref where keys are reasons why a book cannot be reserved +(undef if $can_be_reserved is true) +$reasons keys can be: alreadyreserved, maxreserves or noholdableitems. =cut sub CanBookBeReserved{ my ($borrowernumber, $biblionumber) = @_; + # Check if borrower already has reserved the same biblio. + my @reserves = GetReservesFromBorrowernumber($borrowernumber); + foreach my $reserve (@reserves) { + if ($reserve->{biblionumber} == $biblionumber) { + return (0, {alreadyreserved => 1}); + } + } + + # Check if borrower has reached the maximum number of holds allowed + my $maxreserves = C4::Context->preference('maxreserves'); + if ($maxreserves && scalar(@reserves) >= $maxreserves) { + return (0, {maxreserves => 1}); + } + my $items = GetItemnumbersForBiblio($biblionumber); #get items linked via host records my @hostitems = get_hostitemnumbers_of($biblionumber); @@ -446,16 +466,21 @@ sub CanBookBeReserved{ } foreach my $item (@$items){ - return 1 if CanItemBeReserved($borrowernumber, $item); + my ($canitembereserved) = CanItemBeReserved($borrowernumber, $item); + return 1 if $canitembereserved; } - return 0; + return (0, {noholdableitems => 1}); } =head2 CanItemBeReserved - $error = &CanItemBeReserved($borrowernumber, $itemnumber) + ($can_be_reserved, $reasons) = &CanItemBeReserved($borrowernumber, $itemnumber) -This function return 1 if an item can be issued by this borrower. +$can_be_reserved is true if book can be reserved, false otherwise. + +$reasons is a hashref where keys are reasons why an item cannot be reserved +(undef if $can_be_reserved is true) +$reasons keys can be: damaged, alreadyreserved, maxreserves or wrongbranch. =cut @@ -465,16 +490,32 @@ sub CanItemBeReserved{ my $dbh = C4::Context->dbh; my $ruleitemtype; # itemtype of the matching issuing rule my $allowedreserves = 0; - + # we retrieve borrowers and items informations # # item->{itype} will come for biblioitems if necessery my $item = GetItem($itemnumber); # If an item is damaged and we don't allow holds on damaged items, we can stop right here - return 0 if ( $item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems') ); + return (0, {damaged => 1}) if ( $item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems') ); my $borrower = C4::Members::GetMember('borrowernumber'=>$borrowernumber); - + + # Check if borrower already has reserved the same item or biblio. + my @reserves = GetReservesFromBorrowernumber($borrowernumber); + foreach my $reserve (@reserves) { + if ( $reserve->{itemnumber} == $itemnumber + or $reserve->{biblionumber} == $item->{biblionumber} ) + { + return (0, {alreadyreserved => 1}); + } + } + + # Check if borrower has reached the maximum number of holds allowed + my $maxreserves = C4::Context->preference('maxreserves'); + if ($maxreserves && scalar(@reserves) >= $maxreserves) { + return (0, {maxreserves => 1}); + } + my $controlbranch = C4::Context->preference('ReservesControlBranch'); my $itemtypefield = C4::Context->preference('item-level_itypes') ? "itype" : "itemtype"; @@ -540,7 +581,7 @@ sub CanItemBeReserved{ # we check if it's ok or not if( $reservecount >= $allowedreserves ){ - return 0; + return (0, {maxreserves => 1}); } # If reservecount is ok, we check item branch if IndependentBranches is ON @@ -550,7 +591,7 @@ sub CanItemBeReserved{ { my $itembranch = $item->{homebranch}; if ($itembranch ne $borrower->{branchcode}) { - return 0; + return (0, {wrongbranch => 1}); } } diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index a842869..06af530 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -38,8 +38,6 @@ use Koha::DateUtils; use Date::Calc qw/Today Date_to_Days/; # use Data::Dumper; -my $MAXIMUM_NUMBER_OF_RESERVES = C4::Context->preference("maxreserves"); - my $query = new CGI; my ( $template, $borrowernumber, $cookie ) = get_template_and_user( { @@ -181,11 +179,9 @@ foreach my $biblioNumber (@biblionumbers) { # with a specific item for each biblionumber. # # +my $maxreserves = 0; if ( $query->param('place_reserve') ) { my $reserve_cnt = 0; - if ($MAXIMUM_NUMBER_OF_RESERVES) { - $reserve_cnt = GetReservesFromBorrowernumber( $borrowernumber ); - } # List is composed of alternating biblio/item/branch my $selectedItems = $query->param('selecteditems'); @@ -250,8 +246,9 @@ if ( $query->param('place_reserve') ) { # If a specific item was selected and the pickup branch is the same as the # holdingbranch, force the value $rank and $found. my $rank = $biblioData->{rank}; + my $reasons; if ( $itemNum ne '' ) { - $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum ); + ($canreserve, $reasons) = CanItemBeReserved( $borrowernumber, $itemNum ); $rank = '0' unless C4::Context->preference('ReservesNeedReturns'); my $item = GetItem($itemNum); if ( $item->{'holdingbranch'} eq $branch ) { @@ -260,18 +257,13 @@ if ( $query->param('place_reserve') ) { } } else { - $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum ); + ($canreserve, $reasons) = CanBookBeReserved( $borrowernumber, $biblioNum ); # Inserts a null into the 'itemnumber' field of 'reserves' table. $itemNum = undef; } - my $notes = $query->param('notes_'.$biblioNum)||''; - if ( $MAXIMUM_NUMBER_OF_RESERVES - && $reserve_cnt >= $MAXIMUM_NUMBER_OF_RESERVES ) - { - $canreserve = 0; - } + my $notes = $query->param('notes_'.$biblioNum)||''; # Here we actually do the reserveration. Stage 3. if ($canreserve) { @@ -283,7 +275,10 @@ if ( $query->param('place_reserve') ) { $notes, $biblioData->{title}, $itemNum, $found ); - ++$reserve_cnt; + } else { + if ( $reasons->{maxreserves} ) { + $maxreserves = 1; + } } } @@ -329,7 +324,7 @@ if ( $borr->{'debarred'} ) { my @reserves = GetReservesFromBorrowernumber( $borrowernumber ); $template->param( RESERVES => \@reserves ); -if ( $MAXIMUM_NUMBER_OF_RESERVES && (scalar(@reserves) >= $MAXIMUM_NUMBER_OF_RESERVES) ) { +if ($maxreserves) { $template->param( message => 1 ); $noreserves = 1; $template->param( too_many_reserves => scalar(@reserves)); @@ -514,7 +509,8 @@ foreach my $biblioNum (@biblionumbers) { $policy_holdallowed = 0; } - if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and CanItemBeReserved($borrowernumber,$itemNum) and ($itemLoopIter->{already_reserved} ne 1)) { + my ($canitembereserved) = CanItemBeReserved($borrowernumber, $itemNum); + if (IsAvailableForItemLevelRequest($itemNum) and $policy_holdallowed and $canitembereserved and ($itemLoopIter->{already_reserved} ne 1)) { $itemLoopIter->{available} = 1; $numCopiesAvailable++; } @@ -538,7 +534,8 @@ foreach my $biblioNum (@biblionumbers) { if ($biblioLoopIter{already_reserved}) { $biblioLoopIter{holdable} = undef; } - if(not CanBookBeReserved($borrowernumber,$biblioNum)){ + my ($canbookbereserved) = CanBookBeReserved($borrowernumber, $biblioNum); + if(not $canbookbereserved){ $biblioLoopIter{holdable} = undef; } if(not C4::Context->preference('AllowHoldsOnPatronsPossessions') and CheckIfIssuedToPatron($borrowernumber,$biblioNum)) { diff --git a/reserve/request.pl b/reserve/request.pl index a86e0e7..e1f8bc2 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -186,8 +186,19 @@ foreach my $biblionumber (@biblionumbers) { my $dat = GetBiblioData($biblionumber); - unless ( CanBookBeReserved($borrowerinfo->{borrowernumber}, $biblionumber) ) { - $maxreserves = 1; + my $alreadyreserved = 0; + + my ($can_book_be_reserved, $reasons) = + CanBookBeReserved( $borrowerinfo->{borrowernumber}, $biblionumber ); + + unless ($can_book_be_reserved) { + if ($reasons->{alreadyreserved}) { + $alreadyreserved = 1; + $biblioloopiter{warn} = 1; + $biblioloopiter{alreadyres} = 1; + } else { + $maxreserves = 1; + } } my $alreadypossession; @@ -199,23 +210,11 @@ foreach my $biblionumber (@biblionumbers) { my $reserves = GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 }); my $count = scalar( @$reserves ); my $totalcount = $count; - my $holds_count = 0; - my $alreadyreserved = 0; foreach my $res (@$reserves) { if ( defined $res->{found} && $res->{found} eq 'W' ) { $count--; } - - if ( defined $borrowerinfo && defined($borrowerinfo->{borrowernumber}) && ($borrowerinfo->{borrowernumber} eq $res->{borrowernumber}) ) { - $holds_count++; - } - } - - if ( $holds_count ) { - $alreadyreserved = 1; - $biblioloopiter{warn} = 1; - $biblioloopiter{alreadyres} = 1; } $template->param( @@ -403,14 +402,15 @@ foreach my $biblionumber (@biblionumbers) { $borrowerinfo->{'branchcode'} ne $item->{'homebranch'} ) ) { $policy_holdallowed = 0; } - + + my ($canitembereserved) = + CanItemBeReserved( $borrowerinfo->{borrowernumber}, $itemnumber ); + if ( $policy_holdallowed && !$item->{cantreserve} && IsAvailableForItemLevelRequest($itemnumber) - && CanItemBeReserved( - $borrowerinfo->{borrowernumber}, $itemnumber - ) + && $canitembereserved ) { $item->{available} = 1; diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index d51dfdb..90bb7d4 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 => 32; +use Test::More tests => 43; use MARC::Record; use C4::Biblio; use C4::Items; @@ -54,8 +54,8 @@ foreach (1..$borrowers_count) { my $biblionumber = $bibnum; -my @branches = GetBranchesLoop(); -my $branch = $branches[0][0]{value}; +my $branches = GetBranchesLoop(); +my $branch = $branches->[0]{value}; # Create five item level holds foreach my $borrowernumber ( @borrowernumbers ) { @@ -205,25 +205,31 @@ t::lib::Mocks::mock_preference('item-level_itypes', 1); # if IndependentBranches is OFF, a CPL patron can reserve an MPL item t::lib::Mocks::mock_preference('IndependentBranches', 0); +my ($canitembereserved, $reasons) = CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber); ok( - CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber), + $canitembereserved, 'CPL patron allowed to reserve MPL item with IndependentBranches OFF (bug 2394)' ); +is($reasons, undef); # if IndependentBranches is OFF, a CPL patron cannot reserve an MPL item t::lib::Mocks::mock_preference('IndependentBranches', 1); t::lib::Mocks::mock_preference('canreservefromotherbranches', 0); +($canitembereserved, $reasons) = CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber); ok( - ! CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber), + ! $canitembereserved, 'CPL patron NOT allowed to reserve MPL item with IndependentBranches ON ... (bug 2394)' ); +ok($reasons->{wrongbranch}); # ... unless canreservefromotherbranches is ON t::lib::Mocks::mock_preference('canreservefromotherbranches', 1); +($canitembereserved, $reasons) = CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber); ok( - CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber), + $canitembereserved, '... unless canreservefromotherbranches is ON (bug 2394)' ); +is($reasons, undef); # Regression test for bug 11336 ($bibnum, $title, $bibitemnum) = create_helper_biblio(); @@ -288,17 +294,36 @@ ModReserve({ reserve_id => $reserveid2, rank => 'del' }); $reserve3 = GetReserve( $reserveid3 ); is( $reserve3->{priority}, 1, "After ModReserve, the 3rd reserve becomes the first on the waiting list" ); -ModItem({ damaged => 1 }, $item_bibnum, $itemnumber); C4::Context->set_preference( 'AllowHoldsOnDamagedItems', 1 ); -ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber), "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" ); + +CancelReserve({reserve_id => $reserveid3}); +ModItem({ damaged => 1 }, $item_bibnum, $itemnumber); +($canitembereserved, $reasons) = CanItemBeReserved($borrowernumbers[0], $itemnumber); +ok( $canitembereserved, "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); +is($reasons, undef); + C4::Context->set_preference( 'AllowHoldsOnDamagedItems', 0 ); -ok( !CanItemBeReserved( $borrowernumbers[0], $itemnumber), "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" ); +($canitembereserved, $reasons) = CanItemBeReserved($borrowernumbers[0], $itemnumber); +ok( !$canitembereserved, "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" ); +ok($reasons->{damaged}); ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for damaged item with AllowHoldsOnDamagedItems disabled" ); # Regression test for bug 9532 ($bibnum, $title, $bibitemnum) = create_helper_biblio('CANNOT'); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => 'CPL', holdingbranch => 'CPL', itype => 'CANNOT' } , $bibnum); +($canitembereserved) = CanItemBeReserved( $borrowernumbers[0], $itemnumber); +ok( + !$canitembereserved, + "cannot request item if policy that matches on item-level item type forbids it" +); +ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber); +($canitembereserved, $reasons) = CanItemBeReserved( $borrowernumbers[0], $itemnumber); +ok( + $canitembereserved, + "can request item if policy that matches on item type allows it" +); + AddReserve( $branch, $borrowernumbers[0], @@ -307,32 +332,50 @@ AddReserve( '', 1, ); -ok( - !CanItemBeReserved( $borrowernumbers[0], $itemnumber), - "cannot request item if policy that matches on item-level item type forbids it" -); -ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber); -ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber), - "can request item if policy that matches on item type allows it" -); t::lib::Mocks::mock_preference('item-level_itypes', 0); ModItem({ itype => undef }, $item_bibnum, $itemnumber); +($canitembereserved) = CanItemBeReserved( $borrowernumbers[0], $itemnumber); ok( - !CanItemBeReserved( $borrowernumbers[0], $itemnumber), + !$canitembereserved, "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)" ); +my $canbookbereserved; +($canbookbereserved, $reasons) = CanBookBeReserved($borrowernumbers[0], $bibnum); +ok(!$canbookbereserved); +ok($reasons->{alreadyreserved}); + +C4::Context->set_preference('maxreserves', 1); +($canbookbereserved, $reasons) = CanBookBeReserved($borrowernumbers[0], $biblionumber); +ok(!$canbookbereserved); +ok($reasons->{maxreserves}); + +C4::Context->set_preference('maxreserves', 0); +t::lib::Mocks::mock_preference('IndependentBranches', 1); +t::lib::Mocks::mock_preference('canreservefromotherbranches', 0); +($canbookbereserved, $reasons) = CanBookBeReserved($borrowernumbers[0], $foreign_bibnum); +ok(!$canbookbereserved); +ok($reasons->{noholdableitems}); + + # Helper method to set up a Biblio. sub create_helper_biblio { my $itemtype = shift; my $bib = MARC::Record->new(); my $title = 'Silence in the library'; - $bib->append_fields( - MARC::Field->new('100', ' ', ' ', a => 'Moffat, Steven'), - MARC::Field->new('245', ' ', ' ', a => $title), - MARC::Field->new('942', ' ', ' ', c => $itemtype), - ); + if (C4::Context->preference('marcflavour') eq 'UNIMARC') { + $bib->append_fields( + MARC::Field->new('600', '', '1', a => 'Moffat, Steven'), + MARC::Field->new('200', '', '', a => $title), + MARC::Field->new('099', '', '', t => $itemtype), + ); + } else { + $bib->append_fields( + MARC::Field->new('100', ' ', ' ', a => 'Moffat, Steven'), + MARC::Field->new('245', ' ', ' ', a => $title), + MARC::Field->new('942', ' ', ' ', c => $itemtype), + ); + } return ($bibnum, $title, $bibitemnum) = AddBiblio($bib, ''); } -- 1.7.10.4