Attachment 139492 Details for Bug 31112 – Bug 31112: Remove unnecessary if-clause

[patch] Bug 31112: Remove unnecessary if-clause

Bug-31112-Remove-unnecessary-if-clause.patch (text/plain), 7.23 KB, created by Nick Clemens (kidclamp) on 2022-08-19 13:43:06 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2022-08-19 13:43:06 UTC

Size: 7.23 KB

patch

obsolete

>From 9699acfd562beb8b224691fcd7f458b6b53969da Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= <joonas.kylmala@iki.fi>
>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 <sally.healey@cheshiresharedservices.gov.uk>
>
>Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 31112: 137826 | 137827 | 137828 | 137856 | 137857 | 137858 | 139492 | 139493 | 139494 | 139495 | 139622 | 139623 | 139729 | 139730 | 139731 | 139732 | 139733 | 139734 | 139735