Bugzilla – Attachment 194383 Details for
Bug 41977
Hold fee not charged for title-level holds when all items have negative notforloan status
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41977: Hold fee not charged for title-level holds when all items are on order
b1c2d98.patch (text/plain), 4.95 KB, created by
Martin Renvoize (ashimema)
on 2026-03-03 14:03:32 UTC
(
hide
)
Description:
Bug 41977: Hold fee not charged for title-level holds when all items are on order
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-03 14:03:32 UTC
Size:
4.95 KB
patch
obsolete
>From b1c2d986151713aa140481b0d31638638417bb74 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 <https://openfifth.co.uk> >--- > 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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 41977
:
194383
|
194384