From 1f45a43f1dae9a321d39be38471a0daed7e08ef5 Mon Sep 17 00:00:00 2001 From: Paul Derscheid Date: Fri, 24 Jul 2026 08:37:41 +0200 Subject: [PATCH] Bug 41898: (QA follow-up) Skip clash detection when collecting a booking Transitioning a booking to 'issued' no longer skipped clash detection, and Item::check_booking treats any live checkout on the item as a clash. Switching an on-site checkout of a booked item to a regular checkout (SwitchOnSiteCheckouts) therefore crashed AddIssue with an unhandled Koha::Exceptions::Booking::Clash; collection could fail the same way whenever the biblio's bookable item pool had shrunk since the booking was placed. - short-circuit no-op stores before validation; circulation issues redundant status writes that must not re-trigger the checks - rename _is_final_status_transition to _skip_clash_detection and also skip when transitioning to 'issued' from an active status, as long as the booked period and item are unchanged - keep validating period/item changes and reactivations of terminal bookings - cover the on-site switch in Circulation.t and the store() semantics in Booking.t Test plan: 1. Enable OnSiteCheckouts and SwitchOnSiteCheckouts 2. Place a booking starting today for a patron 3. Check the booked item out to that patron from Circulation > Check out with 'On-site checkout' ticked. Do NOT use the bookings page's 'Transform to checkout' button: it submits a fixed field set that does not carry the on-site flag (Bug 36789), so the checkout would silently be a regular one and step 4 could not reproduce the switch 4. The booking shows 'Issued'. Scan the same barcode for the same patron again to switch the on-site checkout to a regular one; this crashed with Koha::Exceptions::Booking::Clash before the patch and now succeeds with the booking still 'Issued' 5. prove t/db_dependent/Koha/Booking.t t/db_dependent/Circulation.t Signed-off-by: Paul Derscheid Signed-off-by: Pedro Amorim --- Koha/Booking.pm | 42 ++++++++++++++++++++------- t/db_dependent/Circulation.t | 37 +++++++++++++++++++++++- t/db_dependent/Koha/Booking.t | 54 ++++++++++++++++++++++++++++------- 3 files changed, 112 insertions(+), 21 deletions(-) diff --git a/Koha/Booking.pm b/Koha/Booking.pm index 2d0e9dd7f3..bab2c9af4a 100644 --- a/Koha/Booking.pm +++ b/Koha/Booking.pm @@ -150,6 +150,13 @@ sub store { $self->_result->result_source->schema->txn_do( sub { + # Nothing changed, nothing to validate; circulation may issue + # redundant status writes (e.g. when switching an on-site + # checkout) that must not re-trigger clash detection + if ( $self->in_storage && !( () = $self->_result->get_dirty_columns ) ) { + return $self->SUPER::store; + } + if ( $self->item_id ) { Koha::Exceptions::Object::FKConstraint->throw( broken_fk => 'item_id', @@ -173,10 +180,12 @@ sub store { value => $self->biblio_id, ) unless ( $self->biblio ); - # Skip clash detection when transitioning to a final/terminal - # status (cancelled or completed); the checks are irrelevant - # at those points and can produce false positives. - unless ( $self->_is_final_status_transition ) { + # Skip clash detection when the status transition cannot + # introduce a new clash: terminal statuses (cancelled, + # completed) and collection (issued) leave the booked + # period untouched, and the checks can produce false + # positives (e.g. against the booking's own checkout). + unless ( $self->_skip_clash_detection ) { # Throw exception for item level booking clash Koha::Exceptions::Booking::Clash->throw() @@ -212,7 +221,6 @@ sub store { $self->_send_notice( { notice => 'BOOKING_CONFIRMATION' } ); } else { my %updated_columns = $self->_result->get_dirty_columns; - return $self->SUPER::store unless %updated_columns; my $old_booking = $self->get_from_storage; $self->SUPER::store; @@ -342,16 +350,22 @@ sub to_api_mapping { =head2 Internal methods -=head3 _is_final_status_transition +=head3 _skip_clash_detection - my $bool = $self->_is_final_status_transition; + my $bool = $self->_skip_clash_detection; -Returns true when the booking is being transitioned to a terminal status where -clash detection is no longer meaningful: C or C. +Returns true when the booking is being transitioned to a status for which +clash detection is not meaningful: the terminal statuses C and +C, or C (collection). A booking being collected already +occupies the period being validated, and the item's own checkout would +register as a false clash. The transition to C only skips the +checks while the booked period and item remain unchanged, and only from an +active (C or C) stored status; reactivating a terminal booking +is still validated. =cut -sub _is_final_status_transition { +sub _skip_clash_detection { my ($self) = @_; return 0 unless $self->in_storage; @@ -361,6 +375,14 @@ sub _is_final_status_transition { return 0 unless $new_status; return 1 if any { $_ eq $new_status } qw( cancelled completed ); + + if ( $new_status eq 'issued' ) { + return 0 if any { exists $updated_columns->{$_} } qw( start_date end_date item_id biblio_id ); + + my $stored_status = $self->get_from_storage->status; + return 1 if any { $_ eq $stored_status } qw( new issued ); + } + return 0; } diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 6b99440cf5..086676a290 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -7260,7 +7260,7 @@ subtest 'AddIssue records staff who checked out item if appropriate' => sub { }; subtest 'AddIssue/AddReturn | booking status handling' => sub { - plan tests => 6; + plan tests => 9; my $schema = Koha::Database->schema; $schema->storage->txn_begin; @@ -7294,6 +7294,41 @@ subtest 'AddIssue/AddReturn | booking status handling' => sub { $booking1->delete(); + # Test 1b: Onsite checkout of a booked item, then switched to a regular checkout - booking stays issued + t::lib::Mocks::mock_preference( 'OnSiteCheckouts', 1 ); + t::lib::Mocks::mock_preference( 'SwitchOnSiteCheckouts', 1 ); + + my $booking1b = Koha::Booking->new( + { + patron_id => $patron1->borrowernumber, + pickup_library_id => $pickup_library->branchcode, + item_id => $item->itemnumber, + biblio_id => $item->biblio->biblionumber, + start_date => dt_from_string(), + end_date => dt_from_string()->add( days => 7 ), + } + )->store(); + + AddIssue( + $patron1, $item->barcode, dt_from_string()->add( days => 7 ), undef, undef, undef, + { onsite_checkout => 1 } + ); + $booking1b->discard_changes(); + is( $booking1b->status, 'issued', "Booking marked as issued on onsite checkout" ); + + lives_ok { + AddIssue( + $patron1, $item->barcode, dt_from_string()->add( days => 7 ), undef, undef, undef, + { switch_onsite_checkout => 1 } + ) + } + 'Switching an onsite checkout to a regular checkout does not trigger clash detection'; + $booking1b->discard_changes(); + is( $booking1b->status, 'issued', "Booking still issued after switching to a regular checkout" ); + + AddReturn( $item->barcode, $pickup_library->branchcode ); + $booking1b->delete(); + # Test 2: Another patron checks out item with different patron's booking overlapping actual booking period - should cancel booking my $booking2 = Koha::Booking->new( { diff --git a/t/db_dependent/Koha/Booking.t b/t/db_dependent/Koha/Booking.t index e1f19495c3..04f8388f92 100755 --- a/t/db_dependent/Koha/Booking.t +++ b/t/db_dependent/Koha/Booking.t @@ -1177,8 +1177,8 @@ subtest 'Integration test: Full optimal selection workflow' => sub { $schema->storage->txn_rollback; }; -subtest 'store() skips clash detection on terminal status transition' => sub { - plan tests => 4; +subtest 'store() skips clash detection on lifecycle status transitions' => sub { + plan tests => 6; $schema->storage->txn_begin; my $patron = $builder->build_object( { class => "Koha::Patrons" } ); @@ -1205,17 +1205,51 @@ subtest 'store() skips clash detection on terminal status transition' => sub { )->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. + # A checkout on the non-bookable sibling inflates the unavailable count + # in Biblio::check_booking (see Bug 41886), and a checkout on the booked + # item itself registers as a clash in Item::check_booking; neither may + # block a lifecycle status transition. C4::Circulation::AddIssue( $patron, $non_bookable_item->barcode ); + $builder->build_object( + { + class => 'Koha::Checkouts', + value => { + itemnumber => $bookable_item->itemnumber, + borrowernumber => $patron->borrowernumber, + date_due => $end, + returndate => undef, + onsite_checkout => 0, + } + } + ); - # Without the fix, transitioning to 'issued' runs clash - # detection which sees the non-bookable checkout and throws - # Koha::Exceptions::Booking::Clash → 500 error. lives_ok { $booking->status('issued')->store() } - 'Transition to issued skips clash detection'; + 'Transition to issued skips clash detection despite a live checkout on the booked item'; + + lives_ok { $booking->status('issued')->store() } + 'Redundant issued store is a no-op and skips clash detection'; + + # A change to the booked period must still be validated, whatever the status + my $booking2 = Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $biblio->biblionumber, + item_id => $bookable_item->itemnumber, + pickup_library_id => $bookable_item->homebranch, + start_date => $end->clone->add( days => 1 ), + end_date => $end->clone->add( days => 3 ), + } + )->store(); + throws_ok { + $booking->set( + { + start_date => $booking2->start_date, + end_date => $booking2->end_date, + } + )->store() + } + 'Koha::Exceptions::Booking::Clash', 'Changing the booked period still runs clash detection'; + $booking->discard_changes; # Transition to completed (item returned) also skips clash detection lives_ok { $booking->status('completed')->store() }