Bugzilla – Attachment 193376 Details for
Bug 41887
Booking::store runs clash detection on terminal status transition causing 500 on checkout
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41887: Skip clash detection on terminal status transition
Bug-41887-Skip-clash-detection-on-terminal-status-.patch (text/plain), 7.44 KB, created by
Paul Derscheid
on 2026-02-19 11:29:52 UTC
(
hide
)
Description:
Bug 41887: Skip clash detection on terminal status transition
Filename:
MIME Type:
Creator:
Paul Derscheid
Created:
2026-02-19 11:29:52 UTC
Size:
7.44 KB
patch
obsolete
>From c36a79ea19bfa5648f1ba87c3313430e93be567d Mon Sep 17 00:00:00 2001 >From: Paul Derscheid <paul.derscheid@lmscloud.de> >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
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 41887
:
193376
|
193531