Attachment 127885 Details for Bug 29483 – Bug 29483: Further improve performance of script

[patch] Bug 29483: Further improve performance of script

Bug-29483-Further-improve-performance-of-script.patch (text/plain), 8.22 KB, created by Nick Clemens (kidclamp) on 2021-11-19 16:19:38 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2021-11-19 16:19:38 UTC

Size: 8.22 KB

patch

obsolete

>From 83a407e3e5b766b86f7dbc33a0b791cb4bdc12a6 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>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

Actions: View | Diff | Splinter Review

Attachments on bug 29483: 127656 | 127819 | 127820 | 127883 | 127884 | 127885 | 129450 | 129451 | 129452 | 129453 | 129454 | 129499 | 129500 | 129501 | 132094 | 132095 | 132096 | 132113 | 133389 | 133390 | 133391 | 134723