From 83a407e3e5b766b86f7dbc33a0b791cb4bdc12a6 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Fri, 19 Nov 2021 16:13:49 +0000 Subject: [PATCH] Bug 29483: Further improve performance of script This patch adds a few tests to cover more cases, and to highlight current functionality. The script only allows renewal if all outstanding holds can be filled by available items. This means we can return as soon as we have determined that not all holds can be filled. I add FIXME and some explanatory comments - I will file a follow-up bug for those, but I feel we can accept these improvements to the performance and deal with the issues of how it 'should' work versus how it does work on another report. To test: 1 - prove -v t/db_dependent/Circulation.t --- C4/Circulation.pm | 33 +++++++++++++++++------ t/db_dependent/Circulation.t | 52 +++++++++++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 9 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 646fe5c0f2..0f3d01624e 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2811,6 +2811,8 @@ sub CanBookBeRenewed { return ( 0, $auto_renew ) if $auto_renew =~ 'auto_too_much_oweing'; } + # 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. @@ -2835,34 +2837,48 @@ sub CanBookBeRenewed { 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(); - # If the count of the union of the lists of reservable items for each borrower - # is equal or greater than the number of borrowers, we know that all reserves - # can be filled with available items. We can get the union of the sets simply - # by pushing all the elements onto an array and removing the duplicates. - my @reservable; + 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 $matched_items{$other_item->itemnumber} == 1; + 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->borrowernumber,$other_item->itemnumber,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; - push @reservable, $other_item->itemnumber; - if (@reservable >= @borrowernumbers) { + # 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; } @@ -2870,6 +2886,7 @@ sub CanBookBeRenewed { last; } $items->reset; + $seen++; } } } diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 7b9a56a196..c22976d826 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -1574,7 +1574,7 @@ subtest "Bug 13841 - Do not create new 0 amount fines" => sub { }; subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { - plan tests => 9; + plan tests => 13; my $biblio = $builder->build_sample_biblio(); my $item_1 = $builder->build_sample_item( { @@ -1731,12 +1731,62 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); is( $renewokay, 0, 'Verify the borrower cannot renew with 2 holds on the record, but only one of those holds can be filled when AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and two other items on record' ); + Koha::CirculationRules->set_rules( + { + categorycode => $patron_category_2->{categorycode}, + itemtype => $item_1->effective_itemtype, + branchcode => undef, + rules => { + reservesallowed => 25, + } + } + ); + # Setting item not checked out to be not for loan but holdable $item_2->notforloan(-1)->store; ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); is( $renewokay, 0, 'Bug 14337 - Verify the borrower can not renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled but the only available item is notforloan' ); + my $mock_circ = Test::MockModule->new("C4::Circulation"); + $mock_circ->mock( CanItemBeReserved => sub { + warn "Checked"; + return { status => 'no' } + } ); + + $item_2->notforloan(0)->store; + $item_3->delete(); + # Two items total, one item available, one issued, two holds on record + + warnings_are{ + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + } [], "CanItemBeReserved not called when there are more possible holds than available items"; + is( $renewokay, 0, 'Borrower cannot renew when there are more holds than available items' ); + + $item_3 = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + itype => $item_1->effective_itemtype, + } + ); + + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => $item_1->effective_itemtype, + branchcode => undef, + rules => { + reservesallowed => 0, + } + } + ); + + warnings_are{ + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + } ["Checked","Checked"], "CanItemBeReserved only called once per available item if it returns a negative result for all items for a borrower"; + is( $renewokay, 0, 'Borrower cannot renew when there are more holds than available items' ); + }; { -- 2.20.1