From 53fce3007ec22b3dbbac6c980b6ca90b5ec2fe68 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 23 Dec 2021 16:49:34 +0000 Subject: [PATCH] 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 --- C4/Circulation.pm | 16 ++++-- t/db_dependent/DecreaseLoanHighHolds.t | 72 ++++++++++++++++++++++++-- 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index a18864dea7..c1782abfa7 100644 --- a/C4/Circulation.pm +++ b/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 ); diff --git a/t/db_dependent/DecreaseLoanHighHolds.t b/t/db_dependent/DecreaseLoanHighHolds.t index e4df3def21..ed93a21c33 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 => 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(); -- 2.30.2