From e5c6b37ef2bd19155cafeb6a5b157be772e9566d Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 24 Feb 2026 19:21:51 +0000 Subject: [PATCH] Bug 41917: (QA follow-up) Fix effective_bookable item-level override The previous implementation used the // (defined-or) operator then checked !$bookable, which could not distinguish between an explicit item-level bookable=0 and an inherited itemtype bookable=0. Both triggered the parent itemtype fallback, allowing a bookable parent to override an item explicitly marked as not bookable. Fix by returning item->bookable immediately when it is defined (0 or 1), so the parent fallback is only reached when item->bookable is NULL. Also caches $self->itemtype in a local variable, consistent with the effective_not_for_loan_status pattern. Adds a missing test: item bookable=0 with a bookable parent itemtype should still return 0. --- Koha/Item.pm | 24 ++++++++++++++---------- t/db_dependent/Koha/Item.t | 11 +++++++++-- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/Koha/Item.pm b/Koha/Item.pm index 530f1d07d25..aed9bf8bdc0 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -179,7 +179,7 @@ sub store { # If the field has changed otherwise, we much update # the 'on' field - elsif (exists $updated_columns{$field} + elsif ( exists $updated_columns{$field} && $updated_columns{$field} && !$pre_mod_item->$field ) { @@ -381,7 +381,7 @@ sub safe_to_delete { { itemnumber => undef, } - )->count; + )->count; if ($error) { return Koha::Result::Boolean->new(0)->add_message( { message => $error } ); @@ -1563,8 +1563,8 @@ sub columns_to_str { $subfield ? $subfield->{authorised_value} ? C4::Biblio::GetAuthorisedValueDesc( - $itemtagfield, - $subfield->{tagsubfield}, $value, '', $tagslib + $itemtagfield, + $subfield->{tagsubfield}, $value, '', $tagslib ) : $value : $value; @@ -1788,7 +1788,8 @@ sub _set_found_trigger { if ($lost_fee_payment) { my $today = dt_from_string(); my $payment_age_in_days = - dt_from_string( $lost_fee_payment->created_on )->delta_days($today) + dt_from_string( $lost_fee_payment->created_on ) + ->delta_days($today) ->in_units('days'); if ( $payment_age_in_days > $no_refund_if_lost_fee_paid_age ) { $self->add_message( @@ -2174,12 +2175,15 @@ Returns the effective bookability of the current item, be that item or itemtype sub effective_bookable { my ($self) = @_; - my $bookable = $self->bookable // $self->itemtype->bookable; - if ( !$bookable && $self->itemtype && $self->itemtype->parent ) { - $bookable = $self->itemtype->parent->bookable; - } + return $self->bookable if defined $self->bookable; + + my $itype = $self->itemtype; + return 0 unless $itype; + + return $itype->bookable if $itype->bookable; - return $bookable; + my $parent = $itype->parent; + return $parent ? $parent->bookable : 0; } =head3 orders diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index 506f4303313..3c4c1a9ded0 100755 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -2886,7 +2886,7 @@ subtest 'store() tests' => sub { { borrowernumber => $patron->id, date => '1970-01-01 14:00:01', - amountoutstanding => 0, + amountoutstanding => 0, amount => -5, interface => 'commandline', credit_type_code => 'PAYMENT' @@ -3869,7 +3869,7 @@ subtest 'effective_not_for_loan_status() tests' => sub { subtest 'effective_bookable() tests' => sub { - plan tests => 9; + plan tests => 10; $schema->storage->txn_begin; @@ -3950,6 +3950,13 @@ subtest 'effective_bookable() tests' => sub { '->effective_bookable uses item-level bookable over parent fallback' ); + $child_item->bookable(0)->store(); + $child_item->discard_changes; + is( + $child_item->effective_bookable, 0, + '->effective_bookable item-level bookable=0 is not overridden by bookable parent' + ); + my $orphan_itype = $builder->build_object( { class => 'Koha::ItemTypes', value => { bookable => 0, parent_type => undef } } ); my $orphan_item = $builder->build_sample_item( -- 2.53.0