From 489dab1038965a2a741a6c2cf86a886116278c9d Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Mon, 15 Nov 2021 13:54:00 +0000 Subject: [PATCH] Bug 29483: [21.11.x] Check ItemsAnyAvailableAndNotRestricted once per patron ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. We can check it once per patron, however, this requires us to flip the loops. Since an item can only be used once, we now add a check to see if this item has already been assigned to a borrower. To test: 1 - Find or create a biblio with 100 items 2 - Place ten 'Next available' holds on a biblio 3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' Set circ rules 'On shelf holds allowed' to 'If any unavailable' 4 - Checkout one of the items to a patron, backdated to be overdue 5 - Note a long loading time for the patron's checkouts 6 - Apply patch, restart_all 7 - Patron loads much faster Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Martin Renvoize Bug 29483: Unit tests This patch updates the AllowRenewalIfOtherItemsAvailable tests to remove deletion of all data, and create specific circ rules for this test. It adjust several other tests that were relying on the rules from this test, so thy too create their opwn specific rules. Additionally, we add tests to cover the case of mutliple items on the record, and some items cannot fill some reserves. What is uncovered here is that the same patron is checked twice, so two holds can be filled, but they only satisfy a single patron Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Martin Renvoize 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 Signed-off-by: Andrew Fuerste-Henry Signed-off-by: Martin Renvoize --- C4/Circulation.pm | 51 ++++++--- t/db_dependent/Circulation.t | 211 +++++++++++++++++++++++++++++------ 2 files changed, 211 insertions(+), 51 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index a4d6929e31..543524feb4 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -27,7 +27,7 @@ use Encode; use Koha::DateUtils qw( dt_from_string output_pref ); use C4::Context; use C4::Stats qw( UpdateStats ); -use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ); +use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ItemsAnyAvailableAndNotRestricted ); use C4::Biblio qw( UpdateTotalIssues ); use C4::Items qw( ModItemTransfer ModDateLastSeen CartToShelf ); use C4::Accounts; @@ -2835,37 +2835,56 @@ 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 } }); - - # 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; - ITEM: while ( my $item = $items->next ) { - next if IsItemOnHoldAndFound( $item->itemnumber ); - while ( my $patron = $patrons->next ) { - next unless IsAvailableForItemLevelRequest($item, $patron); - next unless CanItemBeReserved($patron->borrowernumber,$item->itemnumber,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; - push @reservable, $item->itemnumber; - if (@reservable >= @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->borrowernumber,$other_item->itemnumber,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 ITEM; + last PATRON; } + $matched_items{$other_item->itemnumber} = 1; last; } - $patrons->reset; + $items->reset; + $seen++; } } } diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 249e8aee1c..de4637de9b 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -1556,16 +1556,30 @@ subtest "Bug 13841 - Do not create new 0 amount fines" => sub { }; subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { - $dbh->do('DELETE FROM issues'); - $dbh->do('DELETE FROM items'); - $dbh->do('DELETE FROM circulation_rules'); + plan tests => 13; + my $biblio = $builder->build_sample_biblio(); + my $item_1 = $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + } + ); + my $item_2= $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + itype => $item_1->effective_itemtype, + } + ); + Koha::CirculationRules->set_rules( { categorycode => undef, - itemtype => undef, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { reservesallowed => 25, + holds_per_record => 25, issuelength => 14, lengthunit => 'days', renewalsallowed => 1, @@ -1578,23 +1592,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { } } ); - my $biblio = $builder->build_sample_biblio(); - my $item_1 = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library2->{branchcode}, - itype => $itemtype, - } - ); - - my $item_2= $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library2->{branchcode}, - itype => $itemtype, - } - ); my $borrowernumber1 = Koha::Patron->new({ firstname => 'Kyle', @@ -1608,6 +1606,22 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { categorycode => $patron_category->{categorycode}, branchcode => $library2->{branchcode}, })->store->borrowernumber; + my $patron_category_2 = $builder->build( + { + source => 'Category', + value => { + category_type => 'P', + enrolmentfee => 0, + BlockExpiredPatronOpacActions => -1, # Pick the pref value + } + } + ); + my $borrowernumber3 = Koha::Patron->new({ + firstname => 'Carnegie', + surname => 'Hall', + categorycode => $patron_category_2->{categorycode}, + branchcode => $library2->{branchcode}, + })->store->borrowernumber; my $borrower1 = Koha::Patrons->find( $borrowernumber1 )->unblessed; my $borrower2 = Koha::Patrons->find( $borrowernumber2 )->unblessed; @@ -1629,7 +1643,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { Koha::CirculationRules->set_rules( { categorycode => undef, - itemtype => undef, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { onshelfholds => 0, @@ -1640,53 +1654,121 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { ( $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' ); + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); + ( $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 is enabled and onshelfholds is disabled' ); + Koha::CirculationRules->set_rules( { categorycode => undef, - itemtype => undef, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { - onshelfholds => 0, + onshelfholds => 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 is disabled and onshelfhold is enabled' ); + t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); ( $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 is enabled and onshelfholds is disabled' ); + is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled' ); + + AddReserve( + { + branchcode => $library2->{branchcode}, + borrowernumber => $borrowernumber3, + biblionumber => $biblio->biblionumber, + priority => 1, + } + ); + + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + is( $renewokay, 0, 'Verify the borrower cannot renew with 2 holds on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and one other item on record' ); + + my $item_3= $builder->build_sample_item( + { + biblionumber => $biblio->biblionumber, + library => $library2->{branchcode}, + itype => $item_1->effective_itemtype, + } + ); + + ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); + is( $renewokay, 1, 'Verify the borrower cannot renew with 2 holds on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and two other items on record' ); Koha::CirculationRules->set_rules( { - categorycode => undef, - itemtype => undef, + categorycode => $patron_category_2->{categorycode}, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { - onshelfholds => 1, + reservesallowed => 0, } } ); - 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 is disabled and onshelfhold is enabled' ); + 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 => undef, - itemtype => undef, + categorycode => $patron_category_2->{categorycode}, + itemtype => $item_1->effective_itemtype, branchcode => undef, rules => { - onshelfholds => 1, + reservesallowed => 25, } } ); - t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); - ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); - is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled' ); # 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' ); + }; { @@ -1771,6 +1853,25 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub { holdingbranch => $holdingbranch->{branchcode}, } ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => $item->effective_itemtype, + branchcode => undef, + rules => { + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + maxissueqty => 20 + } + } + ); set_userenv($holdingbranch); @@ -1924,6 +2025,26 @@ subtest 'CanBookBeIssued + Koha::Patron->is_debarred|has_overdues' => sub { library => $library->{branchcode}, } ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => $library->{branchcode}, + rules => { + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + maxissueqty => 20 + } + } + ); + my ( $error, $question, $alerts ); @@ -2121,6 +2242,26 @@ subtest 'CanBookBeIssued + AllowMultipleIssuesOnABiblio' => sub { } ); + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => $library->{branchcode}, + rules => { + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, + norenewalbefore => undef, + auto_renew => 0, + fine => .10, + chargeperiod => 1, + maxissueqty => 20 + } + } + ); + my ( $error, $question, $alerts ); my $issue = AddIssue( $patron->unblessed, $item_1->barcode, dt_from_string->add( days => 1 ) ); -- 2.30.2