Bug 41887

Summary: Booking::store runs clash detection on terminal status transition causing 500 on checkout
Product: Koha Reporter: Paul Derscheid <paul.derscheid>
Component: CirculationAssignee: Paul Derscheid <paul.derscheid>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: david, gmcharlt, kyle
Version: Main   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function: Bookings
Bug Depends on: 29002    
Bug Blocks:    
Attachments: Bug 41887: Skip clash detection on terminal status transition
Bug 41887: Skip clash detection on terminal status transition

Description Paul Derscheid 2026-02-19 11:18:30 UTC
- 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 these checks
- Combined with false positives from Biblio::check_booking (e.g. checkouts on non-bookable sibling items inflating the unavailable count, see Bug XXXXX), this throws Koha::Exceptions::Booking::Clash resulting in an unhandled 500 error
- Even without false positives, running clash detection on a terminal transition (completed/cancelled) is unnecessary and should be skipped

Steps to reproduce:
- Have a biblio with two items: one bookable, one not (bookable = 0).
- Create a booking on the bookable item.
- Check out the non-bookable item to a patron.
- Check out the bookable item to the booking patron.
- Expected: Checkout succeeds, booking transitions to completed.
- Actual: 500 internal server error from Koha::Exceptions::Booking::Clash.
Comment 1 Paul Derscheid 2026-02-19 11:29:52 UTC
Created attachment 193376 [details] [review]
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.
Comment 2 David Nind 2026-02-20 12:12:39 UTC
Created attachment 193531 [details] [review]
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.

Signed-off-by: David Nind <david@davidnind.com>