From c94dd86dee04fe09f8168a112869ee95d3ae71d3 Mon Sep 17 00:00:00 2001
From: Nick Clemens <nick@bywatersolutions.com>
Date: Thu, 23 Dec 2021 17:05:59 +0000
Subject: [PATCH] Bug 29102: Do not count patron's own hold against limits

This patch makes two changes:
1 - The borrower's own holds are not counted towards HighHolds limit
2 - We exclude all hold counts from CanItemBeReserved

Previously a patron's hold could put the count over the threshhold, and
if the patron is only allowed 1 hold per record, and the hold wasn't found before
the checkout, it would make all items unholdable, thus lowering the theshhold for
dynamic HighHolds

To test:
1 - Set sysaprefs:
     decreaseLoanHighHolds  - enable
     decreaseLoanHighHoldsDuration - 1
     decreaseLoanHighHoldsValue - 1
     decreaseLoanHighHoldsControl - dynamic
     decreaseLoanHighHoldsIgnoreStatuses - blank
2 - Set circ rules to allow 1 hold per record and loan period of 5
3 - Find/create a record with 3 items
4 - Place a title level hold for a patron
5 - Attempt to checkout item - note warning about high holds
6 - Cancel checkout
7 - Set decreaseLoanHighHoldsControl - static
8 - Attempt checkout - note warning about high holds
9 - Apply patrch
10 - Checkout item - no warning
11 - checkin item, replace hold
12 - Set decreaseLoanHighHoldsControl - dynamic
13 - Checkout item - no warning
---
 C4/Circulation.pm                      | 11 +++++++++--
 t/db_dependent/DecreaseLoanHighHolds.t | 16 ++++++----------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index c2edf81f94..dac5bb4f0e 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1289,7 +1289,11 @@ sub checkHighHolds {
         due_date    => undef,
     };
 
-    my $holds = Koha::Holds->search( { biblionumber => $item->{'biblionumber'} } );
+    # Count holds on this record, ignoring the borrowers own holds as they would be filled by the checkout
+    my $holds = Koha::Holds->search({
+        biblionumber => $item->{'biblionumber'},
+        borrowernumber => { '!=' => $borrower->{borrowernumber} }
+    });
 
     if ( $holds->count() ) {
         $return_data->{outstanding} = $holds->count();
@@ -1322,7 +1326,9 @@ sub checkHighHolds {
             }
 
             # Remove any items that are not holdable for this patron
-            @items = grep { CanItemBeReserved( $borrower->{borrowernumber}, $_->itemnumber, undef, { ignore_found_holds => 1 } )->{status} eq 'OK' } @items;
+            # We need to ignore hold counts as the borrower's own hold that will be filled by the checkout
+            # could prevent them from placing further holds
+            @items = grep { CanItemBeReserved( $borrower->{borrowernumber}, $_->itemnumber, undef, { ignore_hold_counts => 1 } )->{status} eq 'OK' } @items;
 
             my $items_count = scalar @items;
 
@@ -1468,6 +1474,7 @@ sub AddIssue {
             );
         }
         else {
+
             unless ($datedue) {
                 my $itype = $item_object->effective_itemtype;
                 $datedue = CalcDateDue( $issuedate, $itype, $branchcode, $borrower );
diff --git a/t/db_dependent/DecreaseLoanHighHolds.t b/t/db_dependent/DecreaseLoanHighHolds.t
index e981e32aee..9998910d4e 100755
--- a/t/db_dependent/DecreaseLoanHighHolds.t
+++ b/t/db_dependent/DecreaseLoanHighHolds.t
@@ -30,7 +30,7 @@ use Koha::CirculationRules;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
-use Test::More tests => 22;
+use Test::More tests => 23;
 
 my $dbh    = C4::Context->dbh;
 my $schema = Koha::Database->new()->schema();
@@ -132,7 +132,7 @@ my $patron_hr = { borrowernumber => $patron->id, branchcode => $library->{branch
 
 my $data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr );
 is( $data->{exceeded},        1,          "Static mode should exceed threshold" );
-is( $data->{outstanding},     6,          "Should have 6 outstanding holds" );
+is( $data->{outstanding},     5,          "Should have 5 outstanding holds" );
 is( $data->{duration},        0,          "Should have duration of 0 because of specific circulation rules" );
 is( ref( $data->{due_date} ), 'DateTime', "due_date should be a DateTime object" );
 
@@ -164,8 +164,8 @@ $data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr );
 is( $data->{exceeded}, 0, "Should not exceed threshold" );
 
 
-# Place 6 more holds - patrons 5,6,7,8,9,10
-for my $i ( 5 .. 10 ) {
+# Place 7 more holds - patrons 5,6,7,8,9,10,11
+for my $i ( 5 .. 11 ) {
     my $patron = $patrons[$i];
     my $hold   = Koha::Hold->new(
         {
@@ -176,6 +176,8 @@ for my $i ( 5 .. 10 ) {
     )->store();
 }
 
+# Note in counts below, patron's own hold is not counted
+
 # 12 holds, threshold is 1 over 10 holdable items = 11
 $data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr );
 is( $data->{exceeded}, 1, "Should exceed threshold of 1" );
@@ -218,12 +220,6 @@ $unholdable->store();
 $data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr );
 is( $data->{exceeded}, 1, "Should exceed threshold with one withdrawn item" );
 
-$patron_hold->found('F')->store;
-# 11 holds, threshold is 2 over 9 holdable items = 11
-$data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr );
-is( $data->{exceeded}, 1, "Should exceed threshold with one withdrawn item" );
-$patron_hold->found(undef)->store;
-
 t::lib::Mocks::mock_preference('CircControl', 'PatronLibrary');
 
 my $patron_object = Koha::Patrons->find( $patron_hr->{borrowernumber} );
-- 
2.30.2