Attachment 139493 Details for Bug 31112 – Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio

[patch] Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio

Bug-31112-CanBookBeRenewed-take-into-account-patro.patch (text/plain), 8.57 KB, created by Nick Clemens (kidclamp) on 2022-08-19 13:43:09 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

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

Size: 8.57 KB

patch

obsolete

>From 1db26eb9c165a5a0b99f0984fc9b2b1ac2c97e06 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: CanBookBeRenewed: take into account patrons with
> more than 1 hold to a biblio
>
>If a single patron had more than 1 hold to a biblio and there was only one
>available item we allowed incorrectly renewing the checkout when
>AllowRenewalIfOtherItemsAvailable was set to "Allow". This
>changes CanBookBeRenewed so that it makes sure all the holds are
>filled and not just one per patron.
>
>To test:
> 1) prove t/db_dependent/Circulation.t
> 2) (Optional, as unit test is provided)
>    - Set AllowRenewalIfOtherItemsAvailable = Allow
>    - Create biblio with three items
>    - Checkout one item to patron A
>    - Add two biblio-level holds for patron B
>    - Try to renew patron A's checkout with and without this patch.
>    - Notice that without this patch the renewal succeeds even though we
>      one unfilled hold left. After applying the patch the renewal should
>      fail.
>
>Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
>
>Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
>---
> C4/Circulation.pm            | 85 ++++++++++++------------------------
> t/db_dependent/Circulation.t | 21 ++++++++-
> 2 files changed, 47 insertions(+), 59 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 6ccaf225c2..85fa276028 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -2945,79 +2945,48 @@ sub CanBookBeRenewed {
>         }
>     }
> 
>-    # 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.
>-    if ( $resfound && $resrec->{non_priority} ) {
>-        $resfound = Koha::Holds->search(
>-            { biblionumber => $resrec->{biblionumber}, non_priority => 0 } )
>-          ->count > 0;
>-    }
>-
>-
>-
>-    # 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') && !Koha::Holds->search( { itemnumber => $itemnumber, found => undef } )->count() )
>+    if ( C4::Context->preference('AllowRenewalIfOtherItemsAvailable') && !Koha::Holds->search( { itemnumber => $itemnumber, found => undef } )->count()
>+ )
>     {
>+        my $biblio = Koha::Biblios->find($item->biblionumber);
>+        my @possible_holds = $biblio->current_holds->unfilled->search({non_priority => 0})->as_list;
>+
>         # 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},
>+        my @other_items = Koha::Items->search({
>+            biblionumber => $biblio->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;
>+            -not         => { itemnumber => $itemnumber } })->as_list;
>+
>         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 ) {
>+        foreach my $possible_hold (@possible_holds) {
>+            my $fillable = 0;
>+            my $patron_with_reserve = Koha::Patrons->find($possible_hold->borrowernumber);
>+            my $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron_with_reserve });
>+
>+            # FIXME: We are not checking whether the item we are renewing can fill the hold
>+
>+            foreach my $other_item (@other_items) {
>               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';
>+              next unless IsAvailableForItemLevelRequest($other_item, $patron_with_reserve, undef, $items_any_available);
>+              next unless CanItemBeReserved($patron_with_reserve,$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;
>-              }
>+              $fillable = 1;
>               $matched_items{$other_item->itemnumber} = 1;
>               last;
>-          }
>-          $items->reset;
>-          $seen++;
>-      }
>+            }
>+            return ( 0, "on_reserve" ) unless $fillable;
>+        }
>+
>+    } else {
>+        my ($status, $matched_reserve, $possible_reserves) = CheckReserves($itemnumber);
>+        return ( 0, "on_reserve" ) if $matched_reserve;
>     }
> 
>-    return ( 0, "on_reserve" ) if $resfound;    # '' when no hold was found
>     return ( 0, $auto_renew, { soonest_renew_date => $soonest } ) if $auto_renew =~ 'too_soon';#$auto_renew ne "no" && $auto_renew ne "ok";
>     $soonest = GetSoonestRenewDate($borrowernumber, $itemnumber);
>     if ( $soonest > dt_from_string() ){
>diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
>index 3c69d684a0..6c53bbf0b1 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -420,7 +420,7 @@ subtest "GetIssuingCharges tests" => sub {
> 
> my ( $reused_itemnumber_1, $reused_itemnumber_2 );
> subtest "CanBookBeRenewed tests" => sub {
>-    plan tests => 104;
>+    plan tests => 105;
> 
>     C4::Context->set_preference('ItemsDeniedRenewal','');
>     # Generate test biblio
>@@ -570,6 +570,25 @@ subtest "CanBookBeRenewed tests" => sub {
>     ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $item_2->itemnumber);
>     is( $renewokay, 1, 'Bug 11634 - Allow renewal of item with unfilled holds if other available items can fill those holds');
> 
>+
>+    # Second biblio-level hold
>+    my $reserve_id = AddReserve(
>+        {
>+            branchcode       => $branch,
>+            borrowernumber   => $reserving_borrowernumber,
>+            biblionumber     => $biblio->biblionumber,
>+            priority         => $priority,
>+            reservation_date => $resdate,
>+            expiration_date  => $expdate,
>+            notes            => $notes,
>+            itemnumber       => $checkitem,
>+            found            => $found,
>+        }
>+    );
>+    ( $renewokay, $error ) = CanBookBeRenewed($renewing_borrowernumber, $item_1->itemnumber);
>+    is( $renewokay, 0, 'Renewal not possible when single patron\'s holds exceed the number of available items');
>+    Koha::Holds->find($reserve_id)->delete;
>+
>     # Now let's add an item level hold, we should no longer be able to renew the item
>     my $hold = Koha::Database->new()->schema()->resultset('Reserve')->create(
>         {
>-- 
>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