From fe86de90479a4f6a6a52277490a83cf8e133dbba Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 21 May 2020 13:41:23 +0000 Subject: [PATCH] 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 Signed-off-by: Martin Renvoize (cherry picked from commit 96a871035043c7ffd93c61e756ee1ff89e3da0f0) Signed-off-by: Lucas Gass (cherry picked from commit d59536e289692537289460a9225cc0c0bce80a57) Signed-off-by: Aleisha Amohia --- C4/Circulation.pm | 2 +- C4/Reserves.pm | 29 +++++++++++++-------- t/db_dependent/DecreaseLoanHighHolds.t | 26 +++++++++++++++--- t/db_dependent/Reserves/MultiplePerRecord.t | 7 +++-- 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index ff51e4438f..618bb00489 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1223,7 +1223,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 2a9296f76f..2fc184a64e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -281,15 +281,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 @@ -300,7 +302,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; @@ -308,9 +310,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. @@ -324,7 +330,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 @@ -387,12 +393,13 @@ sub CanItemBeReserved { $ruleitemtype = '*'; } - 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 ( $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 9128aae215..ffdc0b8a71 100755 --- a/t/db_dependent/DecreaseLoanHighHolds.t +++ b/t/db_dependent/DecreaseLoanHighHolds.t @@ -29,7 +29,7 @@ use Koha::Hold; 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(); @@ -72,6 +72,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( @@ -83,6 +84,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( @@ -94,8 +96,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 }); $builder->build( { @@ -130,7 +133,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" ); @@ -143,6 +146,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( @@ -154,9 +159,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" ); @@ -165,6 +173,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" ); @@ -172,6 +181,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" ); @@ -179,6 +189,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" ); @@ -186,8 +197,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 80c1c3ecd1..1f30a341d9 100755 --- 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; @@ -271,7 +271,7 @@ $rule = $rules_rs->new( categorycode => '*', itemtype => '*', branchcode => '*', - reservesallowed => 2, + reservesallowed => 3, holds_per_record => 2, } )->insert(); @@ -293,4 +293,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; -- 2.39.5