Bug 41887 - Booking::store runs clash detection on terminal status transition causing 500 on checkout
Summary: Booking::store runs clash detection on terminal status transition causing 500...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Paul Derscheid
QA Contact: Testopia
URL:
Keywords:
Depends on: 29002
Blocks:
  Show dependency treegraph
 
Reported: 2026-02-19 11:18 UTC by Paul Derscheid
Modified: 2026-02-20 12:14 UTC (History)
3 users (show)

See Also:
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


Attachments
Bug 41887: Skip clash detection on terminal status transition (7.44 KB, patch)
2026-02-19 11:29 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 41887: Skip clash detection on terminal status transition (7.48 KB, patch)
2026-02-20 12:12 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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>