gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 41977: Hold fee not charged for title-level holds when all items are on order 

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>
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Roger fredricks <roger.fredricks@westsussex.gov.uk>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Martin Renvoize 2026-03-03 13:55:43 +00:00 committed by Lucas Gass
parent a4bbf98dbf
commit dee69bb7bf
No known key found for this signature in database
GPG Key ID: 60B447398B0D36CA
2 changed files with 23 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
Koha/Hold.pm
View File

@ -1383,7 +1383,7 @@ sub _calculate_title_hold_fee {
my @holdable_items = $biblio->items->search(
{
-or => [
{ 'me.notforloan' => 0 },
{ 'me.notforloan' => { '<=', 0 } },
{ 'me.notforloan' => undef }
]
}

30
t/db_dependent/Koha/Hold.t
View File

@ -2270,7 +2270,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;
@ -2418,17 +2418,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: Mix of holdable and non-holdable items (highest)
# 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 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
@ -2437,12 +2451,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(
{
@ -2465,7 +2479,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(
{