From c36a79ea19bfa5648f1ba87c3313430e93be567d Mon Sep 17 00:00:00 2001 From: Paul Derscheid Date: Thu, 19 Feb 2026 10:40:07 +0100 Subject: [PATCH] Bug 41887: Skip clash detection on terminal status transition - Booking::store runs item-level and biblio-level clash detection unconditionally on every call - During checkout, C4::Circulation sets the booking status to 'completed' and calls ->store(), which triggers clash checks - Combined with false positives from Biblio::check_booking (e.g. checkouts on non-bookable sibling items inflating the unavailable count), this throws Koha::Exceptions::Booking::Clash resulting in a 500 error - Running clash detection on a terminal transition is unnecessary regardless; skip it for cancelled/completed - Add _is_final_status_transition() helper that inspects the dirty columns to detect completed/cancelled transitions - Guard both clash detection blocks with the new helper To test: 1. Have a biblio with two items: one bookable, one not (bookable = 0). 2. Create a booking on the bookable item. 3. Check out the non-bookable item to a patron. 4. Check out the bookable item to the booking patron (this triggers the completed transition via C4::Circulation). 5. Without the patch: 500 error from Koha::Exceptions::Booking::Clash. 6. Apply the patch. 7. Repeat steps 2-4. 8. Verify the checkout succeeds and the booking transitions to completed. 9. Run: prove t/db_dependent/Koha/Booking.t and confirm the new subtest "store() skips clash detection on terminal status transition" passes. --- Koha/Booking.pm | 69 +++++++++++++++++++++++++---------- t/db_dependent/Koha/Booking.t | 49 ++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 20 deletions(-) diff --git a/Koha/Booking.pm b/Koha/Booking.pm index 4e62da2a73e..2bd609b4e66 100644 --- a/Koha/Booking.pm +++ b/Koha/Booking.pm @@ -173,25 +173,33 @@ sub store { value => $self->biblio_id, ) unless ( $self->biblio ); - # Throw exception for item level booking clash - Koha::Exceptions::Booking::Clash->throw() - if $self->item_id && !$self->item->check_booking( - { - start_date => $self->start_date, - end_date => $self->end_date, - booking_id => $self->in_storage ? $self->booking_id : undef - } - ); - - # Throw exception for biblio level booking clash - Koha::Exceptions::Booking::Clash->throw() - if !$self->biblio->check_booking( - { - start_date => $self->start_date, - end_date => $self->end_date, - booking_id => $self->in_storage ? $self->booking_id : undef - } - ); + # Skip clash detection when transitioning to a final + # status. During checkout C4::Circulation sets status + # to 'completed' and calls ->store; the clash checks + # are irrelevant at that point and can produce false + # positives. + unless ( $self->_is_final_status_transition ) { + + # Throw exception for item level booking clash + Koha::Exceptions::Booking::Clash->throw() + if $self->item_id && !$self->item->check_booking( + { + start_date => $self->start_date, + end_date => $self->end_date, + booking_id => $self->in_storage ? $self->booking_id : undef + } + ); + + # Throw exception for biblio level booking clash + Koha::Exceptions::Booking::Clash->throw() + if !$self->biblio->check_booking( + { + start_date => $self->start_date, + end_date => $self->end_date, + booking_id => $self->in_storage ? $self->booking_id : undef + } + ); + } # FIXME: We should be able to combine the above two functions into one @@ -336,6 +344,29 @@ sub to_api_mapping { =head2 Internal methods +=head3 _is_final_status_transition + + my $bool = $self->_is_final_status_transition; + +Returns true when the booking is being transitioned to a final status +(cancelled or completed). Used to skip clash detection that is only +meaningful for active bookings. + +=cut + +sub _is_final_status_transition { + my ($self) = @_; + + return 0 unless $self->in_storage; + + my $updated_columns = { $self->_result->get_dirty_columns }; + my $new_status = $updated_columns->{status}; + + return 0 unless $new_status; + return 1 if any { $_ eq $new_status } qw( cancelled completed ); + return 0; +} + =head3 _select_optimal_item my $item_id = $self->_select_optimal_item($available_items); diff --git a/t/db_dependent/Koha/Booking.t b/t/db_dependent/Koha/Booking.t index be9f05f9fc3..77358ae5e50 100755 --- a/t/db_dependent/Koha/Booking.t +++ b/t/db_dependent/Koha/Booking.t @@ -20,7 +20,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 6; +use Test::More tests => 7; use Test::NoWarnings; use Test::Warn; @@ -1180,3 +1180,50 @@ subtest 'Integration test: Full optimal selection workflow' => sub { $schema->storage->txn_rollback; }; + +subtest 'store() skips clash detection on terminal status transition' => sub { + plan tests => 3; + $schema->storage->txn_begin; + + my $patron = $builder->build_object( { class => "Koha::Patrons" } ); + t::lib::Mocks::mock_userenv( { patron => $patron } ); + + my $biblio = $builder->build_sample_biblio(); + my $bookable_item = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, bookable => 1 } ); + my $non_bookable_item = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, bookable => 0 } ); + + my $start = dt_from_string()->truncate( to => 'day' ); + my $end = $start->clone->add( days => 7 ); + + # Create booking first, before any checkouts exist so + # store() succeeds without interference from Bug 41886. + my $booking = Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $biblio->biblionumber, + item_id => $bookable_item->itemnumber, + pickup_library_id => $bookable_item->homebranch, + start_date => $start, + end_date => $end, + } + )->store(); + ok( $booking->in_storage, 'Booking on bookable item stored OK' ); + + # Now check out the non-bookable sibling item. This + # checkout inflates the unavailable count in + # Biblio::check_booking (see Bug 41886) and causes a + # false clash when there is only one bookable item. + C4::Circulation::AddIssue( $patron, $non_bookable_item->barcode ); + + # Without the fix, transitioning to 'completed' runs clash + # detection which sees the non-bookable checkout and throws + # Koha::Exceptions::Booking::Clash → 500 error. + lives_ok { $booking->status('completed')->store() } + 'Transition to completed skips clash detection'; + + # Cancellation from completed is also a terminal transition + lives_ok { $booking->status('cancelled')->store() } + 'Transition to cancelled skips clash detection'; + + $schema->storage->txn_rollback; +}; -- 2.39.5