From d59536e289692537289460a9225cc0c0bce80a57 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 --- 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 7a3b21b903..733c0f17e9 100644 --- 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 2c17d53235..979b3de244 100644 --- 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 f0bf9b61d0..9228f32c93 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 => 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 6fb431da09..1a46a2f353 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; @@ -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; -- 2.39.5