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 <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

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
+            # Skip clash detection when the status transition cannot
-            # status (cancelled or completed); the checks are irrelevant
+            # introduce a new clash: terminal statuses (cancelled,
-            # at those points and can produce false positives.
+            # completed) and collection (issued) leave the booked
-            unless ( $self->_is_final_status_transition ) {
+            # 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
+Returns true when the booking is being transitioned to a status for which
-clash detection is no longer meaningful: C<cancelled> or C<completed>.
+clash detection is not meaningful: the terminal statuses C<cancelled> and
+C<completed>, or C<issued> (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<issued> only skips the
+checks while the booked period and item remain unchanged, and only from an
+active (C<new> or C<issued>) 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;
 }
 

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(
         {

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 {
+subtest 'store() skips clash detection on lifecycle status transitions' => sub {
-    plan tests => 4;
+    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
+    # A checkout on the non-bookable sibling inflates the unavailable count
-    # checkout inflates the unavailable count in
+    # in Biblio::check_booking (see Bug 41886), and a checkout on the booked
-    # Biblio::check_booking (see Bug 41886) and causes a
+    # item itself registers as a clash in Item::check_booking; neither may
-    # false clash when there is only one bookable item.
+    # 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() }