@@ -, +, @@ tests --- C4/Circulation.pm | 1 + C4/Reserves.pm | 5 +-- t/db_dependent/Circulation.t | 15 +++++-- t/db_dependent/Holds.t | 13 +++--- t/db_dependent/Reserves.t | 100 +++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 116 insertions(+), 18 deletions(-) --- a/C4/Circulation.pm +++ a/C4/Circulation.pm @@ -2060,6 +2060,7 @@ sub AddReturn { my ($resfound, $resrec); my $lookahead= C4::Context->preference('ConfirmFutureHolds'); #number of days to look for future holds ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->itemnumber, undef, $lookahead ) unless ( $item->withdrawn ); + if ($resfound) { $resrec->{'ResFound'} = $resfound; $messages->{'ResFound'} = $resrec; --- a/C4/Reserves.pm +++ a/C4/Reserves.pm @@ -214,7 +214,6 @@ sub AddReserve { } )->store(); $hold->set_waiting() if $found eq 'W'; - logaction( 'HOLDS', 'CREATE', $hold->id, Dumper($hold->unblessed) ) if C4::Context->preference('HoldsLog'); @@ -378,6 +377,7 @@ sub CanItemBeReserved { found => undef, # Found holds don't count against a patron's holds limit } ); + if ( $holds->count() >= $holds_per_record ) { return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; } @@ -758,7 +758,6 @@ sub CheckReserves { my $priority = 10000000; foreach my $res (@reserves) { - #Before allocating to bib-level hold check if reserve respects hold rule, i.e. can patron category/item type combination allow reserves my $checkreserve = Koha::Hold->check_if_existing_hold_matches_issuingrules( $res->{'borrowernumber'}, $itemnumber ); next unless ($checkreserve eq 'OK'); @@ -810,14 +809,12 @@ sub CheckReserves { } } } - # If we get this far, then no exact match was found. # We return the most important (i.e. next) reservation. if ($highest) { $highest->{'itemnumber'} = $item; return ( "Reserved", $highest, \@reserves ); } - return ( '' ); } --- a/t/db_dependent/Circulation.t +++ a/t/db_dependent/Circulation.t @@ -259,12 +259,15 @@ $dbh->do( issuelength, lengthunit, renewalsallowed, renewalperiod, norenewalbefore, auto_renew, - fine, chargeperiod) + fine, chargeperiod, + holds_per_day, holds_per_record + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, - ?, ? + ?, ?, + ?,? ) }, {}, @@ -272,7 +275,8 @@ $dbh->do( 14, 'days', 1, 7, undef, 0, - .10, 1 + .10, 1, + 12,1 ); my ( $reused_itemnumber_1, $reused_itemnumber_2 ); @@ -1193,7 +1197,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { undef, undef, undef ); - C4::Context->dbh->do("UPDATE issuingrules SET onshelfholds = 0"); + C4::Context->dbh->do("UPDATE issuingrules SET onshelfholds = 0, holds_per_day = 1"); t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 0 ); ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); is( $renewokay, 0, 'Bug 14337 - Verify the borrower cannot renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfholds are disabled' ); @@ -2661,6 +2665,8 @@ subtest 'Set waiting flag' => sub { my $biblio = $builder->build( { source => 'Biblio' } ); my $biblioitem = $builder->build( { source => 'Biblioitem', value => { biblionumber => $biblio->{biblionumber} } } ); + C4::Context->dbh->do("UPDATE issuingrules SET reservesallowed = 1, holds_per_day = 1, holds_per_record = 1"); + my $item = $builder->build( { source => 'Item', @@ -2688,6 +2694,7 @@ subtest 'Set waiting flag' => sub { my $hold = Koha::Holds->find( $reserve_id ); is( $hold->found, 'T', 'Hold is in transit' ); + my ( $status ) = CheckReserves($item->{itemnumber}); is( $status, 'Reserved', 'Hold is not waiting yet'); --- a/t/db_dependent/Holds.t +++ a/t/db_dependent/Holds.t @@ -245,16 +245,16 @@ my ($foreign_item_bibnum, $foreign_item_bibitemnum, $foreign_itemnumber) = AddItem({ homebranch => $branch_2, holdingbranch => $branch_2 } , $foreign_biblio->biblionumber); $dbh->do('DELETE FROM issuingrules'); $dbh->do( - q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) - VALUES (?, ?, ?, ?, ?)}, + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record, holds_per_day) + VALUES (?, ?, ?, ?, ?, ?)}, {}, - '*', '*', '*', 25, 99 + '*', '*', '*', 25, 99, 99 ); $dbh->do( - q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record) - VALUES (?, ?, ?, ?, ?)}, + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record, holds_per_day) + VALUES (?, ?, ?, ?, ?, ?)}, {}, - '*', '*', 'CANNOT', 0, 99 + '*', '*', 'CANNOT', 1, 99, 99 ); # make sure some basic sysprefs are set @@ -331,6 +331,7 @@ AddReserve( '', 1, ); + is( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'tooManyReserves', "cannot request item if policy that matches on item-level item type forbids it" --- a/t/db_dependent/Reserves.t +++ a/t/db_dependent/Reserves.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 62; +use Test::More tests => 65; use Test::MockModule; use Test::Warn; @@ -192,10 +192,10 @@ $requesters{$branch_3} = Koha::Patron->new({ $dbh->do('DELETE FROM issuingrules'); $dbh->do( - q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed) - VALUES (?, ?, ?, ?)}, + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record, holds_per_day) + VALUES (?, ?, ?, ?, ?, ?)}, {}, - '*', '*', '*', 25 + '*', '*', '*', 25, 1, 1 ); # CPL allows only its own patrons to request its items @@ -290,6 +290,7 @@ my $messages; # the one placed by the CPL patron, as the other two patron's hold # requests cannot be filled by that item per policy. (undef, $messages, undef, undef) = AddReturn('bug10272_CPL', $branch_2); + is( $messages->{ResFound}->{borrowernumber}, $requesters{$branch_1}, 'restrictive library\'s items only fill requests by own patrons (bug 10272)'); @@ -596,6 +597,97 @@ MoveReserve( $itemnumber, $borrowernumber ); is( $status, 'Reserved', 'MoveReserve did not fill future hold of 3 days'); $dbh->do('DELETE FROM reserves', undef, ($bibnum)); + + + + + +#Test bug 23172. Check when returning an item that any patron category/item type combinations that forbid reservation are skipped over +#when allocating the item to a bib-level hold +$dbh->do('DELETE FROM issuingrules'); + +#Create two new patrons +my $patronnumcat1 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_1})->store->borrowernumber; +my $patronnumcat2 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_2})->store->borrowernumber; + +#Create a new biblio record +my $bibnum3 = $builder->build_sample_biblio({frameworkcode => $frameworkcode})->biblionumber; + +#Create a second item type +my $itemtype2 = $builder->build({ source => 'Itemtype', value => { notforloan => undef } } )->{itemtype}; + +# Create an item from new itemtype +my ( $item_bibnum2, $item_bibitemnum2, $itemnumber2 ) = AddItem( + { homebranch => $branch_1, + holdingbranch => $branch_1, + itype => $itemtype2, + barcode => 'bug23172_CPL' + }, + $bibnum3 +); + +#Make a general All patron category/all item type rule +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + '*', '*', '*', 25, 1, 1 +); + +#When patron category = $category_1 and itemtype = $itemtype1 then reservesallowed, holds_per_day and holds_per_record are 0 +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + $category_1, $branch_1, $itemtype2, 0, 1, 1 +); + +#When patron category = $category_2 and itemtype = $itemtype2 then reservesallowed, holds_per_day and holds_per_record are 1 +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + $category_2, $branch_1, $itemtype2, 1, 1, 1 +); + +#Remove existing reserves +$dbh->do("DELETE FROM reserves"); + +#Create Bib-level hold for borrower who has a patron category of $category_2 +my $reserveid2 = AddReserve($branch_1, $patronnumcat2, $bibnum3); + +#Bib-level hold for borrower who has a patron category of $category_1 +my $reserveid1 = AddReserve($branch_1, $patronnumcat1, $bibnum3 ); + +#Check bib-level hold priority and confirm hold made by patronnumcat1 (who fits into issuingrule with reserveallowed = 0) has first priority +my $reserve1 = Koha::Holds->find($reserveid1); +is( $reserve1->priority, 1, 'patronnumcat1 reserve is first priority'); + +my $reserve2 = Koha::Holds->find($reserveid2); +is( $reserve2->priority, 2, 'patronnumcat2 reserve is second priority'); + +#Return the item +(undef, $messages, undef, undef) = AddReturn('bug23172_CPL', $branch_1); + +#First priority hold is skipped as issuingrule is checked and the first hold does not respect issuingrules (as one of the three fields: reservesallowed, holds_per_day is 0) therefore not allocating item to this bib-level hold +is( $messages->{ResFound}->{borrowernumber}, $patronnumcat2,'Skip allocating item to first priority hold as it does not respect issuingrules instead allocate to next valid hold (bug 23172)'); + +#Remove and re-add the issuing rule that prevented patronnumcat1 bib-level hold from being allocated upon item return making reservesallowed, holds_per_day and holds_per_record > 0 +$dbh->do(q{DELETE FROM issuingrules WHERE categorycode = ? AND branchcode = ? AND itemtype = ?}, + {}, + $category_1, $branch_1, $itemtype2 +); +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + $category_1, $branch_1, $itemtype2, 1, 1, 1 +); + +#Return the item again and notice this time it is allocated to the bib-level hold made by patronnumcat1 +(undef, $messages, undef, undef) = AddReturn('bug23172_CPL', $branch_1); +is( $messages->{ResFound}->{borrowernumber}, $patronnumcat1,'Skip allocating item to first priority hold as it does not respect issuingrules instead allocate to next valid hold (bug 23172)'); + $cache->clear_from_cache("MarcStructure-0-$frameworkcode"); $cache->clear_from_cache("MarcStructure-1-$frameworkcode"); $cache->clear_from_cache("default_value_for_mod_marc-"); --