Locking an order for checkout is meant to freeze its lines. Removing is not refused at all.

Correction to the first version of this summary. It claimed that adding was unguarded too. That is wrong: holdGroup() takes a new line into an order only while it is empty or pending (BookingManager.php:180-193), and its comment covers "a late hold (a stale cart, a request racing a payment)". I had read only the validation at the top of the method. The add side needs no change.

Removing. BookingManager::release() checks only that the line is held and cancel() only that it is confirmed. Neither looks at the owning order, so a held line can be taken out of an order that is mid-payment: the per-line operations in the admin UI (BookingOperationController), the bulk release and cancel actions on the bookings tab, the API client, the seat map's re-seating, and the quantity stepper on the slot booking page all reach it. The amount the visitor is paying was computed from those lines.

The visitor's own surfaces are the ones least exposed, because they reach their order through BookingCart::current(), which counts only the empty and pending states as editable. That is each surface remembering to do the right thing rather than an invariant, and LocalBookingClient takes ids straight from its caller.

The wrinkle: the guard cannot simply live in release() unconditionally, because the checkout's own settlement legitimately releases a locked order's lines. DefaultSettlementSubscriber::onFailed() releases them when the payment fails, BookingTerminationReleaser when the workflow instance ends, and the order-wide release and cancel paths do the same. What has to be refused is an edit, not a settlement.

The rule also has to be narrower than the one for adding. Cancelling one ticket of a confirmed order is ordinary operator work, and cancel() only accepts a confirmed line, which lives in a settled order by definition, so refusing everything outside empty and pending would break it. Only the window the checkout owns needs closing.

Proposal: one guard in the booking manager, so every surface inherits it, refusing a removal while the order is in checkout (locked or placed); the settlement paths opt out by name. Companion to #3614424: The checkout amount is computed before the order is locked, so the basket can still change between the two, the ordering half of the same invariant: the checkout amount is computed before the lock is taken.

Issue fork yoyaku-3614426

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mably created an issue. See original summary.

mably’s picture

Title: Nothing refuses to add a line to a locked order, so the checkout lock is advisory » Nothing stops a locked order's lines being added to or removed, so the checkout lock is advisory
Issue summary: View changes

mably’s picture

Title: Nothing stops a locked order's lines being added to or removed, so the checkout lock is advisory » A line can be taken out of an order the checkout owns, so the lock is advisory
Issue summary: View changes
Status: Active » Needs review

mably changed the visibility of the branch 3614426-nothing-refuses-to to hidden.

  • mably committed 33b52095 on 1.x
    fix: #3614426 A line can be taken out of an order the checkout owns, so...
mably’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.