fix(place-order): keep the button disabled until privacy & terms are accepted - #825
Merged
Merged
Conversation
…accepted
Selecting a payment method could enable `PlaceOrderButton` while the privacy &
terms checkbox was still unchecked.
Acceptance travelled from `<PrivacyAndTermsCheckbox>` to `PlaceOrderButton`
through `localStorage["privacy-terms"]`, and the write that happens at *mount*
notified nobody. A `"true"` left behind by an earlier visit — the unmount
cleanup only runs on a React unmount, never on a tab close or a hard navigation
— was read by `PlaceOrderContainer` as `isPermitted: true`. The checkbox then
mounted and correctly wrote `"false"`, but none of the container's effect
dependencies changed, so `isPermitted` stayed stale. The button's own effect
does re-run on `paymentSource?.id`, i.e. exactly when a payment method is
picked, and it read that stale value.
Acceptance now lives in a module-level store keyed by order id, which notifies
its subscribers. It is deliberately not persisted: a reload starts from
"not accepted", so what the shopper sees can no longer diverge from what gates
the button.
- add `termsAcceptanceStore`: in-memory, per-order, notifying
- add the public `useTermsAndConditions()` hook, so a custom consent control has
a supported channel now that `localStorage` is gone
- `placeOrderPermitted` takes `termsAccepted` as a parameter instead of reading
a global, and reports `termsBlocking`
- warn in development when acceptance is required but no control collects it,
from an effect so a late-mounting checkbox cannot raise a false alarm
- drop `PLACE_ORDER_RECHECK_EVENT`, made redundant by the store
- drop a dead `isFree && !isPermitted` line: `setNotPermitted` is a state
setter, so the branches below always overwrote it in the same effect pass
Two existing tests claimed to cover this and covered nothing: the suite mocks
`getCardDetails` to `{ brand: "" }`, which already falsifies the first factor of
the enabling condition, so they passed on the wrong factor and would have passed
with the gate deleted. Both now run with the condition live.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selecting a Drop-in method that needs no input — Klarna, PayPal — enabled `PlaceOrderButton` while privacy & terms were still unaccepted, undoing the gate added in 819280f. `onSelect` and `onChange` answered `isValid` by writing `placeOrderButtonRef.current.disabled = false` straight onto the DOM node, which skips `isPermitted` — where the terms check lives — entirely. React never repaired it either: its own `disabled` prop had not changed, so no re-render reconciled the node. Observed on a live checkout as a fiber saying `disabled: true` over a DOM saying `disabled: false`, with the reducer reporting `isPermitted: false, termsBlocking: true`. Both writes were redundant as well as harmful. Each sits right after `ref.current.onsubmit = …` and `setPaymentRef({ ref })`, and `onsubmit` is in the button's own effect dependencies — so the button already re-runs and enables itself, but through `&& isPermitted`, which respects the terms. The two remaining writes in the post-authorization paths are left alone: they precede a programmatic `.click()` and belong to a different flow, worth revisiting on its own terms. - add regression coverage for both entry points, asserting on the DOM node's `disabled` rather than on React state — the blind spot that let the vacuous `place-order.spec.tsx` "disabled" tests pass on the wrong factor - capture the Drop-in's options in the test double, so `onSelect` is reachable Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
commit: |
The two remaining `placeOrderButtonRef.current.disabled = false` writes look like leftovers of the pattern just removed from the `isValid` handlers, and the obvious next cleanup is to delete them for symmetry or swap the `.click()` for `setPlaceOrder`. Both would break paying by redirect. By the time these run Adyen has already authorized the payment and only the order is left to place. Terms acceptance lives in memory and does not survive the reload a redirect method (Klarna, iDEAL) causes, so the button is legitimately disabled on the way back — and `.click()` on a disabled button is a no-op, which would leave the shopper charged for an order that is never placed. The click cannot become `setPlaceOrder` either: `handleClick` additionally guards against already-placed and draft orders, drives the loading state, and fires the integrator's `onClick`. The Apple/Google Pay branch alongside does call `setPlaceOrder` directly, but express payments bypass that logic on purpose. Comments only — no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`placeOrderPermitted` arms the gate on `privacyUrl && termsUrl`, so an order carrying exactly one of the two silently requires no acceptance: the checkbox is never rendered and the button enables with nothing ticked. Nothing distinguishes that from a deliberate opt-out, which makes it the last remaining way to reach a live place-order button without accepting anything. The gate itself is left alone. Requiring acceptance on a single URL would be the worse trade: `<PrivacyAndTermsCheckbox>` renders a link for each, so one of them would point nowhere — and it would start blocking checkouts that integrators have had working. The ambiguity is surfaced instead, following the `useMissingTermsCheckboxWarning` precedent: development-only, from an effect, and silent in production. Both URLs absent stays quiet — that is the opt-out, not a mistake. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pfferrari
approved these changes
Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #824.
The bug
Selecting a payment method could enable
PlaceOrderButtonwhile the privacy & terms checkbox was still unchecked.Root cause
Acceptance travelled from
<PrivacyAndTermsCheckbox>toPlaceOrderButtonthroughlocalStorage["privacy-terms"], and the write that happens at mount notified nobody — the checkbox only signalled from insidehandleChange."true"left behind by an earlier visit survives: the cleanup that doesremoveItemruns only on a React unmount, never on a tab close or a hard navigation.PlaceOrderContainerreads that stale"true"and computesisPermitted: true."false"— but none of the container's effect dependencies ([order, include, includeLoaded, organizationConfig]) changed, soisPermittedstays stale.paymentSource?.idandcurrentPaymentMethodType— i.e. exactly when a payment method is picked — reads the staleisPermitted: true, seescard.brand, and enables.Because it depends on mount order, it does not reproduce every time.
The fix
Acceptance now lives in a module-level store keyed by order id, following the
usePrices/useSkusidiom already in this repo. The store notifies its subscribers, which is the thing that was missing. It is deliberately not persisted: a reload starts from "not accepted", so what the shopper sees can no longer diverge from what gates the button. Keying by order id also stops consent given on one order from leaking into another.termsAcceptanceStore— in-memory, per-order, notifyinguseTermsAndConditions()hook: withlocalStoragegone, a custom consent control needs a supported channel rather than an unofficial oneplaceOrderPermittedtakestermsAcceptedas a parameter instead of reading a global, and reportstermsBlockingPLACE_ORDER_RECHECK_EVENT, made redundant by the storeisFree && !isPermittedline:setNotPermittedis a state setter, so the branches below always overwrote it within the same effect pass. Verified by removing it against the full suite — no observable behaviour changesTwo tests that covered nothing
The suite mocks
getCardDetailsto{ brand: "" }and never providescurrentPaymentMethodRef, which already falsifies the first factor of the enabling condition((isFree && isPermitted) || onsubmit || card.brand) && isPermitted. Sostays disabled when paymentMethodErrors clear but privacy/terms checkbox is not checkedandis enabled for free order when permittedpassed on the wrong factor and would have passed with the gate deleted entirely. Both now run with the condition live, and the free-order describe asserts a real matrix (missing billing address, missing shipping address, unaccepted terms).Verification
termsAcceptedfrom the container's deps fails 2; removing the diagnostic's count guard fails 2dist/: nolocalStorageaccess for privacy/terms remains; the input'sname="privacy-terms"attribute is unchangedOne mutation is not caught: flipping the
termsAccepted = falsedefault totrue. It is unreachable because every caller passes the value explicitly — left atfalseas the safe direction, but no test protects it.Breaking changes
PLACE_ORDER_RECHECK_EVENTremovedlocalStorage["privacy-terms"]no longer read or writtenNeither had consumers in this repo outside its own tests.
mfe-checkoutneeds no change — it usesPlaceOrderContainer+PrivacyAndTermsCheckbox, both covered — but it pinspkg.pr.new@5884410, so it will not pick this up until published, andcore-components/react-hooks-componentsare pinned to the same commit.🤖 Generated with Claude Code