Skip to content

fix: pickup orders keep and charge stale delivery totals rows - #137

Open
iamnothardcoded wants to merge 1 commit into
tastyigniter:4.xfrom
iamnothardcoded:fix/stale-order-totals
Open

fix: pickup orders keep and charge stale delivery totals rows#137
iamnothardcoded wants to merge 1 commit into
tastyigniter:4.xfrom
iamnothardcoded:fix/stale-order-totals

Conversation

@iamnothardcoded

Copy link
Copy Markdown

The bug

A customer who reaches checkout with the default delivery order type, then switches to pickup and places the order, gets a collection order that still carries a delivery totals row — and calculateTotals() sums that row into order_total, so the pickup customer is silently charged the delivery fee. We hit this in production (caught it on a printed receipt; every display surface hides the row, see below).

Two coupled defects produce this:

1. CartCondition::apply() leaves stale state behind. When beforeApply() returns false, apply() early-returns without resetting passed or calculatedValue. Both keep the values from an earlier pass, so Cart::conditions() — which re-applies and then filters with ->applied() (an isValid() filter) — still reports the condition as applied, and OrderManager::getCartTotals() persists a row with the stale value. The applied() filter exists precisely to exclude these rows; the stale flag defeats it.

2. addOrderTotals() never deletes, but totals are written more than once per order. loadOrder() writes totals already when the order row is first created — which happens when the checkout page is rendered, under whatever the session order type is at that moment (Location::orderType() defaults to delivery for a visitor who never chose one). addOrderTotals() only updateOrCreates per code (unlike addOrderMenus() directly above it, which deletes first), so once the customer switches to pickup and confirms, the later write correctly excludes the delivery condition — but the earlier row is orphaned forever and still summed by calculateTotals().

The symptom appears to be known: the admin order view works around it by hiding delivery rows on collection orders (resources/views/_partials/orders/order_menus.blade.php):

@continue($model->isCollectionType() && $total->code == 'delivery')

…while the hidden row keeps being summed into what the customer pays. The success page and order mail hide it the same way, which is why the bug survives unnoticed unless a delivery fee is configured and someone checks the math.

Reproduce

  1. Fresh session (never touch the order-type toggle — it defaults to delivery), location with a delivery fee configured.
  2. Add an item to the cart, open the checkout page. → order row + totals (incl. delivery) are persisted here.
  3. Switch the order type to pickup, complete checkout.
  4. orders.order_type = collection, but order_totals still contains the delivery row and order_total includes the fee.

The fix

  • CartCondition::apply() resets passed = false and calculatedValue = 0 in the beforeApply() === false branch, so validity flags always describe the current pass. This alone makes the existing applied() filter work and stops new stale-value rows.
  • Order::syncOrderTotals() (new, in ManagesOrderItems) prunes totals rows whose code is absent from the written set, then delegates to addOrderTotals(). The two checkout write sites (loadOrder(), saveOrder()) now use it, so a row from an earlier checkout pass cannot outlive the pass that dropped its condition. addOrderTotals() itself keeps its upsert-only behaviour — other callers (e.g. the API orders resource) may legitimately pass partial sets.

Tests

  • it resets applied state when beforeApply fails on a later pass — unit test for defect 1.
  • it syncs order totals removing rows absent from the given set — unit test for syncOrderTotals().
  • it removes stale totals rows when the order is saved after a condition stops applying — end-to-end regression mirroring the delivery→pickup switch; it fails if either half of the fix is removed.

Full suite run locally: no new failures (+3 passing).

🤖 Generated with Claude Code

A cart condition whose beforeApply() returns false keeps the passed flag
and calculatedValue from an earlier apply pass, so the applied() filter
still reports it as valid and getCartTotals() persists a totals row with
the stale value. On top of that, addOrderTotals() only upserts and never
deletes, while loadOrder() already writes totals when the order row is
first created on checkout page render — under the session order type of
that moment, which defaults to delivery.

Combined real-world effect: a customer who reaches checkout with the
default delivery order type, then switches to pickup and places the
order, ends up with a collection order carrying an orphaned delivery
totals row, and calculateTotals() sums the delivery fee into
order_total — the pickup customer is silently charged the delivery fee.
The admin order view already works around the symptom by hiding
delivery rows on collection orders instead of the data being fixed.

- reset passed/calculatedValue in CartCondition::apply() when
  beforeApply() returns false, so validity flags stay truthful
- add Order::syncOrderTotals(), which prunes totals rows absent from
  the written set, and use it at the two checkout write sites;
  addOrderTotals() keeps its upsert-only behaviour for other callers
  that pass partial sets

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
iamnothardcoded added a commit to iamnothardcoded/tastyrhodos that referenced this pull request Aug 2, 2026
Second variant of the stale-condition totals bug (elgrecomarl order 15,
first real El Greco order): addOrderTotals() upserts per code and never
deletes, and loadOrder() writes totals already at checkout page render —
under the session state of that moment, where the order type DEFAULTS to
delivery for a visitor who never touched the pill. Render checkout →
delivery row persisted → switch to Abholen → place order → the
collection pass correctly excludes delivery from the new set, but the
old row is orphaned and calculateTotals() re-sums it into order_total.
Dev repro: collection order billed 7,50 = 5,00 subtotal + 2,50 stale
delivery fee. Fleet audit: 11 orphaned rows across all 3 tenants, all
0,00 — no customer was ever charged (no fee configured at the time).

Fix: getCartTotals() memoizes the codes of the row set it produced;
saveOrder() deletes rows the current pass did not produce and re-runs
calculateTotals(). Filed upstream as tastyigniter/ti-ext-cart#137 —
drop FixedOrderManager entirely once that is merged and released.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant