Attachment 140970 Details for Bug 29102 – Bug 29102: Do not count patron's own hold against limits

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

Bug-29102-Do-not-count-patrons-own-hold-against-li.patch (text/plain), 8.50 KB, created by David Nind on 2022-09-25 05:40:17 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2022-09-25 05:40:17 UTC

Size: 8.50 KB

patch

obsolete

>From 0051e507e6d5c80a1333ef8008a77c490d121ac4 Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>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
>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> 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 6faf53add9..ffe11487ad 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -1352,7 +1352,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();
>@@ -1367,8 +1372,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;
>             }
>         }
>@@ -1385,7 +1390,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;
> 
>@@ -1537,6 +1544,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

Actions: View | Diff | Splinter Review

Attachments on bug 29102: 128901 | 128902 | 128903 | 137722 | 137723 | 137745 | 137746 | 140970 | 140971 | 142990 | 142991