@@ -, +, @@ more than 1 hold to a biblio 1) prove t/db_dependent/Circulation.t 2) (Optional, as unit test is provided) - Set AllowRenewalIfOtherItemsAvailable = Allow - Create biblio with three items - Checkout one item to patron A - Add two biblio-level holds for patron B - Try to renew patron A's checkout with and without this patch. - Notice that without this patch the renewal succeeds even though we one unfilled hold left. After applying the patch the renewal should fail. --- C4/Circulation.pm | 85 ++++++++++++------------------------ t/db_dependent/Circulation.t | 21 ++++++++- 2 files changed, 47 insertions(+), 59 deletions(-) --- a/C4/Circulation.pm +++ a/C4/Circulation.pm @@ -2945,79 +2945,48 @@ sub CanBookBeRenewed { } } - # Note: possible_reserves will contain all title level holds on this bib and item level - # holds on the checked out item - my ( $resfound, $resrec, $possible_reserves ) = C4::Reserves::CheckReserves($itemnumber); - - # If next hold is non priority, then check if any hold with priority (non_priority = 0) exists for the same biblionumber. - if ( $resfound && $resrec->{non_priority} ) { - $resfound = Koha::Holds->search( - { biblionumber => $resrec->{biblionumber}, non_priority => 0 } ) - ->count > 0; - } - - - - # This item can fill one or more unfilled reserve, can those unfilled reserves - # all be filled by other available items? - if ( $resfound - && C4::Context->preference('AllowRenewalIfOtherItemsAvailable') && !Koha::Holds->search( { itemnumber => $itemnumber, found => undef } )->count() ) + if ( C4::Context->preference('AllowRenewalIfOtherItemsAvailable') && !Koha::Holds->search( { itemnumber => $itemnumber, found => undef } )->count() + ) { + my $biblio = Koha::Biblios->find($item->biblionumber); + my @possible_holds = $biblio->current_holds->unfilled->search({non_priority => 0})->as_list; + # Get all other items that could possibly fill reserves # FIXME We could join reserves (or more tables) here to eliminate some checks later - my $items = Koha::Items->search({ - biblionumber => $resrec->{biblionumber}, + my @other_items = Koha::Items->search({ + biblionumber => $biblio->biblionumber, onloan => undef, notforloan => 0, - -not => { itemnumber => $itemnumber } - }); - my $item_count = $items->count(); - - # Get all other reserves that could have been filled by this item - my @borrowernumbers = map { $_->{borrowernumber} } @$possible_reserves; - # Note: fetching the patrons in this manner means that a patron with 2 holds will - # not block renewal if one reserve can be satisfied i.e. each patron is checked once - my $patrons = Koha::Patrons->search({ - borrowernumber => { -in => \@borrowernumbers } - }); - my $patron_count = $patrons->count(); - - return ( 0, "on_reserve" ) if ($patron_count > $item_count); - # We cannot possibly fill all reserves if we don't have enough items - - # If we can fill each hold that has been found with the available items on the record - # then the patron can renew. If we cannot, they cannot renew. - # FIXME This code does not check whether the item we are renewing can fill - # any of the existing reserves. - my $reservable = 0; + -not => { itemnumber => $itemnumber } })->as_list; + my %matched_items; - my $seen = 0; - PATRON: while ( my $patron = $patrons->next ) { - # If there is a reserve that cannot be filled we are done - return ( 0, "on_reserve" ) if ( $seen > $reservable ); - my $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron }); - while ( my $other_item = $items->next ) { + foreach my $possible_hold (@possible_holds) { + my $fillable = 0; + my $patron_with_reserve = Koha::Patrons->find($possible_hold->borrowernumber); + my $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron_with_reserve }); + + # FIXME: We are not checking whether the item we are renewing can fill the hold + + foreach my $other_item (@other_items) { next if defined $matched_items{$other_item->itemnumber}; next if IsItemOnHoldAndFound( $other_item->itemnumber ); - next unless IsAvailableForItemLevelRequest($other_item, $patron, undef, $items_any_available); - next unless CanItemBeReserved($patron,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; + next unless IsAvailableForItemLevelRequest($other_item, $patron_with_reserve, undef, $items_any_available); + next unless CanItemBeReserved($patron_with_reserve,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' # CanItemBeReserved checks 'rules' and 'policies' which means # items will fill holds at checkin that are rejected here - $reservable++; - if ($reservable >= $patron_count) { - $resfound = 0; - last PATRON; - } + $fillable = 1; $matched_items{$other_item->itemnumber} = 1; last; - } - $items->reset; - $seen++; - } + } + return ( 0, "on_reserve" ) unless $fillable; + } + + } else { + my ($status, $matched_reserve, $possible_reserves) = CheckReserves($itemnumber); + return ( 0, "on_reserve" ) if $matched_reserve; } - return ( 0, "on_reserve" ) if $resfound; # '' when no hold was found return ( 0, $auto_renew, { soonest_renew_date => $soonest } ) if $auto_renew =~ 'too_soon';#$auto_renew ne "no" && $auto_renew ne "ok"; $soonest = GetSoonestRenewDate($borrowernumber, $itemnumber); if ( $soonest > dt_from_string() ){ --- a/t/db_dependent/Circulation.t +++ a/t/db_dependent/Circulation.t @@ -420,7 +420,7 @@ subtest "GetIssuingCharges tests" => sub { my ( $reused_itemnumber_1, $reused_itemnumber_2 ); subtest "CanBookBeRenewed tests" => sub { - plan tests => 104; + plan tests => 105; C4::Context->set_preference('ItemsDeniedRenewal',''); # Generate test biblio @@ -570,6 +570,25 @@ subtest "CanBookBeRenewed tests" => sub { ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $item_2->itemnumber); is( $renewokay, 1, 'Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds'); + + # Second biblio-level hold + my $reserve_id = AddReserve( + { + branchcode => $branch, + borrowernumber => $reserving_borrowernumber, + biblionumber => $biblio->biblionumber, + priority => $priority, + reservation_date => $resdate, + expiration_date => $expdate, + notes => $notes, + itemnumber => $checkitem, + found => $found, + } + ); + ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $item_1->itemnumber); + is( $renewokay, 0, 'Renewal not possible when single patron\'s holds exceed the number of available items'); + Koha::Holds->find($reserve_id)->delete; + # Now let's add an item level hold, we should no longer be able to renew the item my $hold = Koha::Database->new()->schema()->resultset('Reserve')->create( { --