From 1db26eb9c165a5a0b99f0984fc9b2b1ac2c97e06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Mon, 18 Jul 2022 16:29:06 +0000 Subject: [PATCH] Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio If a single patron had more than 1 hold to a biblio and there was only one available item we allowed incorrectly renewing the checkout when AllowRenewalIfOtherItemsAvailable was set to "Allow". This changes CanBookBeRenewed so that it makes sure all the holds are filled and not just one per patron. To test: 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. Signed-off-by: Sally Signed-off-by: Nick Clemens --- C4/Circulation.pm | 85 ++++++++++++------------------------ t/db_dependent/Circulation.t | 21 ++++++++- 2 files changed, 47 insertions(+), 59 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 6ccaf225c2..85fa276028 100644 --- a/C4/Circulation.pm +++ b/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() ){ diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 3c69d684a0..6c53bbf0b1 100755 --- a/t/db_dependent/Circulation.t +++ b/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( { -- 2.30.2