From b1c2d986151713aa140481b0d31638638417bb74 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 3 Mar 2026 13:55:43 +0000 Subject: [PATCH] Bug 41977: Hold fee not charged for title-level holds when all items are on order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Koha's convention is that notforloan <= 0 means "holdable" and notforloan > 0 means "not holdable". This is documented in both IsAvailableForItemLevelRequest() and CheckReserves(): "item with negative or zero notforloan value is holdable" _calculate_title_hold_fee() was only searching for items with notforloan = 0 or IS NULL, silently excluding items with negative notforloan values (e.g. -1 = "Ordered"). When all items on a title had a negative notforloan status, no items were found, @fees was empty, and the method returned 0 — causing no fee to be charged. Fix the filter to use <= 0 so it matches the same holdability rule used consistently elsewhere in the codebase. Sponsored-by: OpenFifth --- Koha/Hold.pm | 2 +- t/db_dependent/Koha/Hold.t | 30 ++++++++++++++++++++++-------- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/Koha/Hold.pm b/Koha/Hold.pm index d886f7d8b55..17ee4f5fdc0 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -1355,7 +1355,7 @@ sub _calculate_title_hold_fee { my @holdable_items = $biblio->items->search( { -or => [ - { 'me.notforloan' => 0 }, + { 'me.notforloan' => { '<=', 0 } }, { 'me.notforloan' => undef } ] } diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index 9ba3a0caa2e..11e8a691a64 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -2224,7 +2224,7 @@ subtest '_Findgroupreserve in the context of hold groups' => sub { }; subtest 'calculate_hold_fee() tests' => sub { - plan tests => 12; + plan tests => 13; $schema->storage->txn_begin; @@ -2372,17 +2372,31 @@ subtest 'calculate_hold_fee() tests' => sub { $fee = $title_hold->calculate_hold_fee(); is( $fee, 3.00, 'Title-level hold: empty strategy preference defaults to highest fee (3.00)' ); - # Test 8: No holdable items returns 0 - # Make all items not holdable + # Test 8: No holdable items (positive notforloan, e.g. "Staff Only") returns 0 + # Positive notforloan values block both loaning and holding $item1->notforloan(1)->store; $item2->notforloan(1)->store; $item3->notforloan(1)->store; $item4->notforloan(1)->store; $fee = $title_hold->calculate_hold_fee(); - is( $fee, 0, 'Title-level hold: no holdable items returns 0 fee' ); + is( $fee, 0, 'Title-level hold: all items have positive notforloan (not holdable) returns 0 fee' ); + + # Test 9 (new): Items with negative notforloan (e.g. "Ordered") are holdable per + # Koha convention and should have their fee included in the calculation. + # notforloan < 0 means "not for loan but holdable" (see IsAvailableForItemLevelRequest). + $item2->notforloan(-1)->store; # Fee: 3.00, on order but holdable + $item3->notforloan(-1)->store; # Fee: 2.00, on order but holdable + + t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'highest' ); + $fee = $title_hold->calculate_hold_fee(); + is( $fee, 3.00, 'Title-level hold: items with negative notforloan (on order, holdable) included in fee calculation' ); + + # Restore for remaining tests + $item2->notforloan(1)->store; + $item3->notforloan(1)->store; - # Test 9: Mix of holdable and non-holdable items (highest) + # Test 10: Mix of holdable and non-holdable items (highest) # Make some items holdable again $item2->notforloan(0)->store; # Fee: 3.00 $item3->notforloan(0)->store; # Fee: 2.00 @@ -2391,12 +2405,12 @@ subtest 'calculate_hold_fee() tests' => sub { $fee = $title_hold->calculate_hold_fee(); is( $fee, 3.00, 'Title-level hold: highest strategy with mixed holdable items returns 3.00' ); - # Test 10: Mix of holdable and non-holdable items (lowest) + # Test 11: Mix of holdable and non-holdable items (lowest) t::lib::Mocks::mock_preference( 'TitleHoldFeeStrategy', 'lowest' ); $fee = $title_hold->calculate_hold_fee(); is( $fee, 2.00, 'Title-level hold: lowest strategy with mixed holdable items returns 2.00' ); - # Test 11: Items with 0 fee + # Test 12 (was 11): Items with 0 fee my $itemtype_free = $builder->build( { source => 'Itemtype' } ); Koha::CirculationRules->set_rules( { @@ -2419,7 +2433,7 @@ subtest 'calculate_hold_fee() tests' => sub { $fee = $title_hold->calculate_hold_fee(); is( $fee, 0, 'Title-level hold: lowest strategy with free item returns 0' ); - # Test 12: All items have same fee + # Test 13 (was 12): All items have same fee # Set all holdable items to same fee Koha::CirculationRules->set_rules( { -- 2.53.0