Bug 29102: Do not count patron's own hold against limits

This patch makes three changes:
1 - The borrower's own holds are not counted towards HighHolds limit
2 - We exclude all hold counts from CanItemBeReserved
3 - Static mode should only decrease hold when over the decreaseLoanHighHoldsValue, not when equal

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 - "over the number of holdable items on the record" / 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 two different patrons
 5 - Attempt to checkout item - note warning about high holds
 6 - Cancel checkout
 7 - Set decreaseLoanHighHoldsControl - "on the record" / static
 8 - Attempt checkout - note warning about high holds
 9 - Apply patch
10 - Checkout item - no warning
11 - checkin item, replace hold
12 - Set decreaseLoanHighHoldsControl - "over the number of holdable items on the record" / dynamic
13 - Checkout item - no warning
14 - prove t/db_dependent/DecreaseLoanHighHolds.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

C4/Circulation.pm

@@ -1354,7 +1354,12 @@ 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 => { '!=' => $patron->borrowernumber }
+    });
 
     if ( $holds->count() ) {
         $return_data->{outstanding} = $holds->count();
@@ -1369,8 +1374,8 @@ sub checkHighHolds {
 
             # static means just more than a given number of holds on the record
 
-            # If the number of holds is less than the threshold, we can stop here
-            if ( $holds->count() < $decreaseLoanHighHoldsValue ) {
+            # If the number of holds is not above the threshold, we can stop here
+            if ( $holds->count() <= $decreaseLoanHighHoldsValue ) {
                 return $return_data;
             }
         }
@@ -1387,7 +1392,9 @@ sub checkHighHolds {
             }
 
             # Remove any items that are not holdable for this patron
-            @items = grep { CanItemBeReserved( $patron , $_, 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( $patron, $_, undef, { ignore_hold_counts => 1 } )->{status} eq 'OK' } @items;
 
             my $items_count = scalar @items;
 
@@ -1539,6 +1546,7 @@ sub AddIssue {
             );
         }
         else {
+
             unless ($datedue) {
                 my $itype = $item_object->effective_itemtype;
                 $datedue = CalcDateDue( $issuedate, $itype, $branchcode, $borrower );

t/db_dependent/DecreaseLoanHighHolds.t

@@ -30,7 +30,7 @@ use Koha::CirculationRules;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
-use Test::More tests => 21;
+use Test::More tests => 26;
 
 my $dbh    = C4::Context->dbh;
 my $schema = Koha::Database->new()->schema();
@@ -129,10 +129,18 @@ t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsIgnoreStatuses', 'damaged,
 
 my $data = C4::Circulation::checkHighHolds( $item, $patron );
 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" );
 
+t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsValue',          5 );
+$data = C4::Circulation::checkHighHolds( $item, $patron );
+is( $data->{exceeded},        0,          "Static mode should not exceed threshold when it equals outstanding holds" );
+is( $data->{outstanding},     5,          "Should have 5 outstanding holds" );
+is( $data->{duration},        0,          "Should have duration of 0 because decrease not calculated" );
+is( $data->{due_date},     undef,         "duedate undefined as not decreasing loan period" );
+t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsValue',          1 );
+
 Koha::CirculationRules->set_rules(
     {
         branchcode   => undef,
@@ -161,8 +169,8 @@ $data = C4::Circulation::checkHighHolds( $item, $patron );
 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(
         {
@@ -173,6 +181,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, $patron );
 is( $data->{exceeded}, 1, "Should exceed threshold of 1" );
@@ -242,4 +252,58 @@ Koha::CirculationRules->set_rule(
 $data = C4::Circulation::checkHighHolds( $item, $patron );
 is( $data->{duration}, 2, "Circulation rules override system preferences" );
 
+
+subtest "Test patron's own holds do not count towards HighHolds count" => sub {
+
+    plan tests => 2;
+
+    my $item = $builder->build_sample_item();
+    my $item2 = $builder->build_sample_item({ biblionumber => $item->biblionumber });
+    my $item3 = $builder->build_sample_item({ biblionumber => $item->biblionumber });
+
+    my $patron = $builder->build_object({
+        class => 'Koha::Patrons',
+        value => {
+            branchcode => $item->homebranch
+        }
+    });
+    my $hold = $builder->build_object({
+        class => 'Koha::Holds',
+        value => {
+            biblionumber => $item->biblionumber,
+            borrowernumber => $patron->id,
+            suspend => 0,
+            found => undef
+        }
+    });
+
+    Koha::CirculationRules->set_rules(
+        {
+            branchcode   => $item->homebranch,
+            categorycode => undef,
+            itemtype     => $item->itype,
+            rules        => {
+                issuelength     => '14',
+                lengthunit      => 'days',
+                reservesallowed => '99',
+                holds_per_record => '1',
+            }
+        }
+    );
+
+    t::lib::Mocks::mock_preference( 'decreaseLoanHighHolds',               1 );
+    t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsDuration',       1 );
+    t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsValue',          1 );
+    t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsControl',        'static' );
+    t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsIgnoreStatuses', 'damaged,itemlost,notforloan,withdrawn' );
+
+    my $data = C4::Circulation::checkHighHolds( $item , $patron );
+    ok( !$data->{exceeded}, "Patron's hold on the record does not limit their own circulation if static decrease");
+    t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsControl',        'dynamic' );
+    # 3 items on record, patron has 1 hold
+    $data = C4::Circulation::checkHighHolds( $item, $patron );
+    ok( !$data->{exceeded}, "Patron's hold on the record does not limit their own circulation if dynamic decrease");
+
+};
+
 $schema->storage->txn_rollback();