Bug 25566: Add option to ignore found holds and use it when checking high holds

To test:
 1 - Find or create a record with 10 items
 2 - Set sysprefs:
     decreaseLoanHighHolds - enable
     decreaseLoanHighHoldsDuration - 2
     decreaseLoanHighHoldsValue - 2
     decreaseLoanHighHoldsControl  - 'over the number of holdable items'/dynamic
 3 - Set circ rules to allow 1 hold per record on the relevant record
 4 - Place 3 holds on the record
 5 - Check one item in and confirm hold to set to waiting
 6 - Issue to the patron with the waiting hold
 7 - Get a notice that loan period is decreased
 8 - Don't confirm the checkout
 9 - Apply patch
10 - Restart all the things
11 - Repeat checkout, no decrease this time!

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 67a986b..2fe1dcd 100644 (file)
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1225,7 +1225,7 @@ sub checkHighHolds {
             }
 
             # Remove any items that are not holdable for this patron
-            @items = grep { CanItemBeReserved( $borrower->{borrowernumber}, $_->itemnumber )->{status} eq 'OK' } @items;
+            @items = grep { CanItemBeReserved( $borrower->{borrowernumber}, $_->itemnumber, undef, { ignore_found_holds => 1 } )->{status} eq 'OK' } @items;
 
             my $items_count = scalar @items;
 

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index 2c17d53..979b3de 100644 (file)
--- a/C4/Reserves.pm
+++ b/C4/Reserves.pm
@@ -301,15 +301,17 @@ sub AddReserve {
 
 =head2 CanBookBeReserved
 
-  $canReserve = &CanBookBeReserved($borrowernumber, $biblionumber, $branchcode)
+  $canReserve = &CanBookBeReserved($borrowernumber, $biblionumber, $branchcode, $params)
   if ($canReserve eq 'OK') { #We can reserve this Item! }
 
+  $params are passed directly through to CanItemBeReserved
+
 See CanItemBeReserved() for possible return values.
 
 =cut
 
 sub CanBookBeReserved{
-    my ($borrowernumber, $biblionumber, $pickup_branchcode) = @_;
+    my ($borrowernumber, $biblionumber, $pickup_branchcode, $params) = @_;
 
     my @itemnumbers = Koha::Items->search({ biblionumber => $biblionumber})->get_column("itemnumber");
     #get items linked via host records
@@ -320,7 +322,7 @@ sub CanBookBeReserved{
 
     my $canReserve = { status => '' };
     foreach my $itemnumber (@itemnumbers) {
-        $canReserve = CanItemBeReserved( $borrowernumber, $itemnumber, $pickup_branchcode );
+        $canReserve = CanItemBeReserved( $borrowernumber, $itemnumber, $pickup_branchcode, $params );
         return { status => 'OK' } if $canReserve->{status} eq 'OK';
     }
     return $canReserve;
@@ -328,9 +330,13 @@ sub CanBookBeReserved{
 
 =head2 CanItemBeReserved
 
-  $canReserve = &CanItemBeReserved($borrowernumber, $itemnumber, $branchcode)
+  $canReserve = &CanItemBeReserved($borrowernumber, $itemnumber, $branchcode, $params)
   if ($canReserve->{status} eq 'OK') { #We can reserve this Item! }
 
+  current params are 'ignore_found_holds' - if true holds that have been trapped are not counted
+  toward the patron limit, used by checkHighHolds to avoid counting the hold we will fill with the
+  current checkout against the high holds threshold
+
 @RETURNS { status => OK },              if the Item can be reserved.
          { status => ageRestricted },   if the Item is age restricted for this borrower.
          { status => damaged },         if the Item is damaged.
@@ -346,7 +352,7 @@ sub CanBookBeReserved{
 =cut
 
 sub CanItemBeReserved {
-    my ( $borrowernumber, $itemnumber, $pickup_branchcode ) = @_;
+    my ( $borrowernumber, $itemnumber, $pickup_branchcode, $params ) = @_;
 
     my $dbh = C4::Context->dbh;
     my $ruleitemtype;    # itemtype of the matching issuing rule
@@ -409,12 +415,13 @@ sub CanItemBeReserved {
         $ruleitemtype = undef;
     }
 
-    my $holds = Koha::Holds->search(
-        {
-            borrowernumber => $borrowernumber,
-            biblionumber   => $item->biblionumber,
-        }
-    );
+    my $search_params = {
+        borrowernumber => $borrowernumber,
+        biblionumber   => $item->biblionumber,
+    };
+    $search_params->{found} = undef if $params->{ignore_found_holds};
+
+    my $holds = Koha::Holds->search($search_params);
     if (   defined $holds_per_record && $holds_per_record ne ''
         && $holds->count() >= $holds_per_record ) {
         return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record };

diff --git a/t/db_dependent/DecreaseLoanHighHolds.t b/t/db_dependent/DecreaseLoanHighHolds.t
index f0bf9b6..9228f32 100755 (executable)
--- 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 => 17;
+use Test::More tests => 19;
 
 my $dbh    = C4::Context->dbh;
 my $schema = Koha::Database->new()->schema();
@@ -71,6 +71,7 @@ for my $i ( 1 .. 20 ) {
 
 my $biblio = $builder->build_sample_biblio();
 
+# The biblio gets 10 items
 my @items;
 for my $i ( 1 .. 10 ) {
     my $item = $builder->build_sample_item(
@@ -82,6 +83,7 @@ for my $i ( 1 .. 10 ) {
     push( @items, $item );
 }
 
+# Place 6 holds, patrons 0,1,2,3,4,5
 for my $i ( 0 .. 5 ) {
     my $patron = $patrons[$i];
     my $hold   = Koha::Hold->new(
@@ -93,8 +95,9 @@ for my $i ( 0 .. 5 ) {
     )->store();
 }
 
-my $item   = pop(@items);
-my $patron = pop(@patrons);
+my $item   = shift(@items);
+my $patron = shift(@patrons);
+my $patron_hold = Koha::Holds->find({ borrowernumber => $patron->borrowernumber, biblionumber => $item->biblionumber });
 
 Koha::CirculationRules->set_rules(
     {
@@ -129,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 5 outstanding holds" );
+is( $data->{outstanding},     6,          "Should have 6 outstanding holds" );
 is( $data->{duration},        1,          "Should have duration of 1" );
 is( ref( $data->{due_date} ), 'DateTime', "due_date should be a DateTime object" );
 
@@ -142,6 +145,8 @@ t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsControl', 'dynamic' );
 $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 ) {
     my $patron = $patrons[$i];
     my $hold   = Koha::Hold->new(
@@ -153,9 +158,12 @@ for my $i ( 5 .. 10 ) {
     )->store();
 }
 
+# 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" );
+is( $data->{outstanding}, 12, "Should exceed threshold of 1" );
 
+# 12 holds, threshold is 2 over 10 holdable items = 12 (equal is okay)
 t::lib::Mocks::mock_preference( 'decreaseLoanHighHoldsValue', 2 );
 $data = C4::Circulation::checkHighHolds( $item_hr, $patron_hr );
 is( $data->{exceeded}, 0, "Should not exceed threshold of 2" );
@@ -164,6 +172,7 @@ my $unholdable = pop(@items);
 $unholdable->damaged(-1);
 $unholdable->store();
 
+# 12 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 damaged item" );
 
@@ -171,6 +180,7 @@ $unholdable->damaged(0);
 $unholdable->itemlost(-1);
 $unholdable->store();
 
+# 12 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 lost item" );
 
@@ -178,6 +188,7 @@ $unholdable->itemlost(0);
 $unholdable->notforloan(-1);
 $unholdable->store();
 
+# 12 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 notforloan item" );
 
@@ -185,8 +196,15 @@ $unholdable->notforloan(0);
 $unholdable->withdrawn(-1);
 $unholdable->store();
 
+# 12 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('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');
 

diff --git a/t/db_dependent/Reserves/MultiplePerRecord.t b/t/db_dependent/Reserves/MultiplePerRecord.t
index 6fb431d..1a46a2f 100755 (executable)
--- a/t/db_dependent/Reserves/MultiplePerRecord.t
+++ b/t/db_dependent/Reserves/MultiplePerRecord.t
@@ -19,7 +19,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 39;
+use Test::More tests => 40;
 use t::lib::TestBuilder;
 use t::lib::Mocks;
 
@@ -277,7 +277,7 @@ Koha::CirculationRules->set_rules(
         itemtype     => undef,
         branchcode   => undef,
         rules        => {
-            reservesallowed  => 2,
+            reservesallowed  => 3,
             holds_per_record => 2,
         }
     }
@@ -314,4 +314,7 @@ Koha::Holds->find($hold_id)->found("W")->store;
 $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber});
 is( $can->{status}, 'tooManyHoldsForThisRecord', 'Third hold exceeds limit of holds per record' );
 
+$can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber}, undef, { ignore_found_holds => 1 });
+is( $can->{status}, 'OK', 'Third hold is allowed when ignoring waiting holds' );
+
 $schema->storage->txn_rollback;