From 9699acfd562beb8b224691fcd7f458b6b53969da 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: Remove unnecessary if-clause To test: 1) Please check manually that the logic stays the same, use git's -w command line parameter to ignore whitespace changes in the diff output. 2) prove t/db_dependent/Circulation.t Signed-off-by: Sally Signed-off-by: Nick Clemens --- C4/Circulation.pm | 114 +++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 61 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 7b028bed0f..6ccaf225c2 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2961,68 +2961,60 @@ sub CanBookBeRenewed { # 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') ) + && C4::Context->preference('AllowRenewalIfOtherItemsAvailable') && !Koha::Holds->search( { itemnumber => $itemnumber, found => undef } )->count() ) { - my $item_holds = Koha::Holds->search( { itemnumber => $itemnumber, found => undef } )->count(); - if ($item_holds) { - # There is an item level hold on this item, no other item can fill the hold - $resfound = 1; - } - else { - - # 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}, - 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; - 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 ) { - 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'; - # 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; - } - $matched_items{$other_item->itemnumber} = 1; - last; - } - $items->reset; - $seen++; - } - } + # 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}, + 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; + 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 ) { + 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'; + # 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; + } + $matched_items{$other_item->itemnumber} = 1; + last; + } + $items->reset; + $seen++; + } } return ( 0, "on_reserve" ) if $resfound; # '' when no hold was found -- 2.30.2