feat(checkout): unify the order-intent loading state and verdict styling (TWO-25326) - #451
Merged
Conversation
…rms (TWO-25326)
The order-intent check looked different on all four plugin checkouts. This
brings WooCommerce onto the shared target agreed 2026-08-04:
- while the check runs: the shared spinner GIF plus the VISIBLE words
"Checking availability". This checkout showed three pulsing dots and no
words at all — the only one of the four saying nothing about what it was
doing. The GIF is byte-identical to the one the Magento front ends ship
and is already in this tree for the company-search field, so this is a
second rule pointing at an existing asset, not a new one;
- the verdict: a coloured, bordered box — green for approved, red for the
two failure boxes — matching the PrestaShop module's palette. All three
boxes previously carried no styling at all beyond overflow-wrap, so a
"not available for this order" verdict read as ordinary tile copy. Message
only, no title.
The .twoinc-dots rule stays: the term-chip fee quote still uses it. The
.twoinc-sr-only rule goes, with the hidden sentence it existed for.
Catalogued and translated for nb_NO/nl_NL/sv_SE — the sentence is visible copy
now, so an untranslated catalogue is a visible English string on those shops.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rts (TWO-25326)
The buyer changed their selected company and the tile kept showing the OLD
company's verdict — "<old company> is not available for this order" — for the
whole of the new check, because the only thing that cleared it was the new
result arriving.
Fixed at getApproval(), the one choke point every route into a check passes
through. Four of the five routes (setSoleTraderCompany, onCompanyInputBlur,
onRepresentativeInputBlur, onCountryChange) cleared nothing of their own, and
the fifth — the company picker's select2:select — still left a second of stale
verdict on screen because the check is armed on a 1s interval. Entering the
loading state there both clears the old verdict and shows the spinner
immediately, and a route added later inherits it.
Two supporting changes fall out of that:
- updateElements() now runs its hide-every-pay-box reset BEFORE arming the
approval pass. In the old order the reset wiped the loader the pass had
just shown, and the buyer saw a blank second;
- a check abandoned at the tick (a required field emptied in the intervening
second) now takes the loader down with it, rather than leaving "Checking
availability" on screen with nothing running behind it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🖌 Pre-commit success 🏆DetailsExit code: 0 Author ✍️@dgjlindsay |
… (TWO-25326)
Round 1 found that showing the loader when a check is ARMED, rather than when
its request goes out, exposed four paths that ended a check without saying
anything about the UI. Invisible before; a permanent "Checking availability"
after. Every one of them now goes through one `abandonOrderIntentCheck()`:
- the cached-verdict branch returned with the interval still armed, which
left `pendingCheck` permanently true (the guard sets it whenever a timer
exists and nothing could clear it) and had the 3s poller re-entering
getApproval() forever — a loader/verdict flicker on a ~3s cycle with no
request behind it. It now disarms; the answer is already in hand;
- `#place_order` and `checkout_error` disarmed the timer only. The second is
the worse one: it does not trigger `updated_checkout`, so nothing
re-rendered the tile and the spinner sat beside the validation errors for
the rest of the page;
- the wait for a readable cart total retried indefinitely. A 100%-discounted
cart reads 0 — falsy every tick — and a theme whose totals markup cannot be
read never yields one at all. Bounded to ten ticks.
Two response paths threw before reaching the only code that renders a verdict
and takes the loader down, so both stranded it:
- `status >= 400` with no `responseJSON` (a proxy 502 carrying an HTML error
page) — `"error_details" in undefined` is a TypeError;
- `invalidFields.append()` — Array has no `append`, so the one route to the
phone-number box has always thrown and that box has never been on screen.
Which would have made this PR's new red border styling for unreachable UI.
Accessibility: the three verdict boxes are now announced — role="status" on the
approval, role="alert" on the two failures. The loader had a role and the
verdicts did not, so a screen-reader buyer heard that a check had started and
never heard how it ended, and the colour this pass adds is no help to them.
Tests: the CSS assertions move to jsdom's real cascade (injectStylesheet +
getComputedStyle). Round 1 proved the raw-text greps blind three ways — to a
commented-out declaration, to a later overriding rule, and to an at-rule-wrapped
copy — and all three now fail. Round 1 also found one vacuous JS assertion (no
positive control, so a selector typo passed), one line with no test at all, and
a catalogue check that searched for msgid and msgstr independently rather than
asserting the pairing. Suite: 396 JS tests, 194 PHP cases.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s cached (TWO-25326)
Round 1 established that the loading state must always resolve. Round 2 found
the one wait it had not bounded, plus three defects the earlier fixes exposed.
- the wait that holds a verdict back until WooCommerce is not mid-re-render
was unbounded, and it is the only code that takes the loading state down —
so an overlay that never clears meant a permanent spinner, exactly the
defect the cart-total wait was bounded for. Capped at ten ticks;
- that wait was also held in a local, unreachable from
abandonOrderIntentCheck(). A Place Order click reset the tile and an orphan
copy of the wait then painted a verdict back onto a checkout already
mid-submit, gateway radio already deselected. It now lives on
orderIntentCheck and is cancelled with everything else;
- the request hash lived in a single `lastCheckHash` slot, and the interval is
disarmed BEFORE the request goes out — so a second check armed while the
first was in flight overwrote it and the first response was filed under the
second body. With the cached branch now disarming, that mis-filed entry
would be served forever with no request ever issued again. The hash is
passed to processOrderIntentResponse() instead;
- a transport failure is no longer cached. `status` 0 (a dropped connection)
and a 5xx both landed as "not available" and were cached, which declined
that cart and company for the rest of the page — permanently, for the same
reason. A 4xx business decline still is cached, and is pinned so the guard
cannot be widened into "never cache";
- abandonOrderIntentCheck() only resets the tile when something was actually
in flight. `#place_order` fires on clicks that never submit and
`checkout_error` fires for a missing postcode; neither fires
`updated_checkout`, so resetting unconditionally wiped a good verdict with
nothing to bring it back.
Accessibility: round 1's roles announced nothing. A live region only announces a
content change made while it is IN the accessibility tree, and the first line of
togglePaySubtitleDesc() hides every box — so writing the sentence and then
revealing the box mutated a region that was not in the tree, then revealed one
whose content had not changed. Reveal now precedes the text, both in one task,
which is one announcement rather than two.
The loader also gets its own two-class hiding rule, following the idiom the term
chips and the sole-trader toggle already use here. Its hiding rested entirely on
the blanket `.hidden` being `!important`, since `display: flex` is a
same-specificity declaration later in the file — and nothing asserted that,
because jsdom does not honour `!important` from an earlier rule. Which is also
why round 1's loader layout assertion was measuring a state that never existed.
Tests: 410 JS, 193 PHP cases. Twenty-eight mutations checked, each killing at
least one test; two negative controls (an equivalent `rgb()` border notation, a
CRLF catalogue) confirmed NOT to fail. The .po pairing check is now an entry
parser rather than a regex — a `msgctxt`-scoped entry sharing the msgid defeated
every regex form of it — and the .mo substring assertion is dropped in favour of
check-catalogues.sh, which decodes and diffs rather than searching binary.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-25326)
Round 2's `wasRunning` gate had a hole of its own making, and it points at the
real missing mechanism: this request had no supersession token, unlike the
registry address lookup in the same file. Three defects hung off that, and all
three are closed by `seq`/`inFlightSeq`:
- the interval is disarmed BEFORE the request goes out, so two checks can
overlap. Arriving in reverse order the OLDER verdict won, and the buyer read
an answer about a company or cart they had already moved on from. Round 2
fixed only where that answer was filed, not that it was acted on;
- a response arriving after the check was abandoned deselected the gateway and
painted a verdict onto a checkout already mid-submit;
- across the whole duration of the XHR every flag `wasRunning` consults reads
falsy, so an abandon in that window skipped the reset and left the loader on
screen for the rest of the page — the exact defect that gate was added to
avoid, reintroduced through its own condition.
The request is orphaned rather than aborted: an abort still runs `.fail`, which
deselects the gateway and paints a decline, and doing that to a checkout the
buyer has just submitted is worse than the request completing unheard.
- `timeout: 30000`, matching the company-search transport. A request that
never settles calls neither handler, and both the loader coming down and the
verdict appearing hang off them;
- which jQuery callback a response came from is now passed, not sniffed. jQuery
hands `.done` the parsed response BODY, so a field called `status` in a good
200 was read as an HTTP status — routing a declining 200 to the
phone-number box and marking `billing_phone` invalid;
- 401, 403, 408 and 429 are no longer cached. They mean "ask again", and a
cached answer is permanent for the page because the cached branch issues no
request;
- the render give-up no longer calls abandonOrderIntentCheck(). That also bumps
the supersession counter, which would silently orphan a newer check armed
while the blocked paint was waiting. It cancels its own timer and resets the
tile, nothing else;
- a repeated verdict is no longer re-announced. `.text()` replaces the child
text node whether or not the string differs, and that mutation inside an
assertive live region had it repeating "not available for this order" on
every field blur — worse than the silence rounds 1-2 fixed.
Dead `lastCheckHash` removed; nothing has written it since round 2.
Tests: 423 JS, 193 PHP cases. Thirty-nine mutations, each killing at least one
test — one survivor was found this way rather than by review (dropping the
`isFailure` gate failed nothing) and is now covered. Suite also verified under
`jest --randomize`. The round-2 test asserting two overlapping checks each cache
a verdict is rewritten: with supersession the correct behaviour is that the stale
response has no effect at all.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ngs held (TWO-25326)
Round 4's two reviewers converged on one defect this branch introduced, plus a
set of coverage holes that mutation — not reading — found. Fixed here are only the
items that are self-contained and belong to code this PR added:
- the stuck-overlay give-up blanked the tile even when a NEWER check was in
flight, so the tile sat blank until that response landed. Round 3 stopped the
give-up ORPHANING a newer check but not it BLANKING one, and the round-3 test
asserted the blank as correct — so the branch shipped a defect its own test
enshrined. It now hands the tile back to whatever is still running, and both
sides of that branch are pinned;
- `setPayBoxText` compared the whole jQuery set, whose `.text()` is the
CONCATENATION of its elements — so two live copies of a verdict box rewrote
both (re-announcing on the first). `.first()` was tried and was also wrong: it
skipped the second copy, leaving it visibly empty. Element-wise walk; all
three shapes are now pinned;
- dead `lastCheckOk` removed. It sat inside the non-approved branch, so it could
only ever be assigned false, and nothing in the repo read it.
Test-only, all in tests this PR added:
- `expect(record.aborted).toBe(false)` could not fail — the harness initialises
it false and no production path calls `abort()`, so blanking
abandonOrderIntentCheck() entirely passed it. Replaced with the observable
mechanism (the counter moving past the in-flight request);
- the "timed out" test was a duplicate of the transport-failure test —
production's `.fail` handler never reads `textStatus`, so the two are
indistinguishable. Merged, and the timeout test renamed to what it actually
asserts (the config, not the behaviour);
- the four retryable-status tests' `getApproval()` preamble issued a real request
under a different hash than the one under test. Removed;
- the supersession test's "different hash" premise was decorative and is now
labelled as such;
- new coverage for five things nothing pinned: `stillCurrent()`'s `inFlightSeq`
release (left set, the abandon gate reads as permanently running and the next
non-submitting Place Order click wipes a good verdict), the pre-arm paint
cancel, that an approved verdict is deliberately NOT cached, and the
tracking-id write;
- the catalogue parser now skips `#, fuzzy` entries. msgfmt drops them from the
.mo, so the shop renders English while a naive read of the .po says
translated — and check-catalogues.sh cannot catch it, because msgfmt drops
them from both sides of its diff. Latent today, which is when to close it.
Tests: 428 JS, 193 PHP cases. Fifty-four mutations, each killing at least one
test; suite green under `--randomize`.
DELIBERATELY NOT FIXED HERE — see the PR comment. Round 4 also found six
order-intent LIFECYCLE findings (missing `seq` bumps on the readiness early-return
and the country-change path, the cached branch not superseding, concurrent
requests now that the window is 30s, `#place_order` firing on non-submitting
clicks, and clearSelectedCompany's unguarded 3s reset). Every one is a consequence
of extending the loader's lifetime beyond the request's, which is the round-1
change this branch made, and each round's fix has generated the next round's
findings. That is an oscillating loop and a scope well beyond this ticket, so it
stops here and goes to Doug rather than into another patch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
Adversarial review: four rounds, and why I am stopping rather than starting a fifthTwo reviewers per round (runtime correctness; test vacuity + integration), each round reading the pushed ref, each finding verified by mutation. Round 4 was not clean, and I am not merging. What the ticket asked for — done and stableThese four have not been touched since the first commit. No review round has found anything against them:
What the rounds actually found — and the patternEverything below round 1 is about the order-intent request lifecycle, not about wording or styling. Round 1 — showing the loader when a check is armed rather than when its request is issued extended the loader's lifetime beyond the request's, and exposed four paths that ended a check while saying nothing about the UI: the cached-verdict branch left the interval armed (so Round 2 — the verdict-paint wait was unbounded and held in a local unreachable from the abandon path; the request hash was one shared slot, so overlapping checks mis-filed each other's verdicts; transport failures were cached (one dropped connection declined that cart permanently); the abandon reset fired on clicks that never submit. Round 1's ARIA roles announced nothing, because the reveal came after the text write. Round 3 — round 2's Round 4 — the That is the pattern: rounds 2, 3 and 4 each found defects created by the preceding round's fix. An oscillating loop is a stop-and-report condition, not a merge condition. Fixed in this PREverything from rounds 1-3, plus the round-4 items that are self-contained and belong to code this PR added: the give-up blanking a newer loader, 428 JS tests, 193 PHP cases. 54 mutations, each killing at least one test. Two negative controls confirmed not to fail (an equivalent Held for a ruling — the recommendationSix round-4 lifecycle findings are not fixed: missing Every one traces to a single decision I made in round 1: moving the loader from "shown when the request is issued" to "shown when a check is armed". That is what decoupled the loader's lifetime from the request's, and it is what rounds 2-4 have been patching. Two ways forward:
I recommend (1). Two things worth a look either way, where matching PrestaShop and this repo's own conventions pull apart — both kept on the PrestaShop side deliberately, since converging the four checkouts is the point: by Claude |
…WO-25326)
Reverts this branch's round-1 decision to show the loader when a check is ARMED
rather than when its request is ISSUED, and fixes the six lifecycle findings
round 4 left open.
The revert is the point. Extending the loading state's lifetime beyond the
request's is what generated rounds 2, 3 and 4 — each round's fix adding an
invariant the next round attacked: a gate that reintroduced the defect it was
added to prevent, a supersession counter that then needed bumping in four more
places, a give-up that blanked a newer check's spinner and a test that asserted
the blank as correct. Tied to the request, "the loader is up exactly while a
request is outstanding" holds by construction rather than by patching every exit.
`getApproval()` now CLEARS the previous verdict and nothing else, which is all
TWO-25326 asked of it. New `clearIntentVerdicts()` takes the three verdict boxes
down and leaves the loader alone — the blanket hide it replaces also blinked the
spinner off on every `updated_checkout`, so a shipping-method change or a coupon
blanked the tile mid-request. The visible cost of the revert is a gap between the
old verdict going and the spinner appearing; it is pinned by a test so nobody
"fixes" it back.
The six lifecycle findings:
- a form that goes incomplete mid-request retires that request. Its answer
describes a form the buyer no longer has, and the loader would otherwise run
until it arrived;
- one request at a time. The interval is disarmed before a request goes out, so
nothing stopped a second check POSTing while the first was outstanding — at
one per second against a 30s timeout, up to thirty in flight, all but the last
already superseded. Arming a new check now retires the previous one, abort
included;
- the abort is safe only because the counter moves FIRST: jQuery runs `.fail`
synchronously for an abort, and that handler deselects the gateway and paints
a decline. Both orderings are pinned;
- a cache hit retires anything in flight for an earlier body, whose answer would
otherwise land afterwards and paint over it;
- `checkout_error` re-arms. It does NOT fire `updated_checkout`, so nothing else
ran another check and the tile sat blank — no verdict, no spinner — while the
buyer corrected a field. `#place_order` deliberately does not re-arm: the
buyer is leaving;
- `clearSelectedCompany()`'s 3s deferred re-read is guarded by the
company-search counter. Three seconds is long enough to pick a company, and
the closure overwrote `customerCompany` from the DOM and undid the capture.
An explicit supersede on the country-change path was written and then DELETED: no
mutation could kill it, because `clearSelectedCompany()` empties the record and
the following `getApproval()` retires the request through its own readiness guard.
The test asserts the outcome instead.
Tests: 437 JS, 193 PHP cases. Sixty-seven mutations, each killing at least one
test; green under `--randomize`. Eleven tests that asserted loader-on-arming are
rewritten rather than deleted — the behaviour they described is gone, the
properties underneath them are not.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… blanket hides (TWO-25326)
Round 5 reviewed the reverted design. None of what it found was caused by the
revert; the revert made most of it VISIBLE, by giving the codebase a
verdicts-only clear to compare each blanket hide against.
- the verdict named the WRONG COMPANY. These sentences carry the captured
company (§7.3) and were built by re-reading the DOM when the verdict was
painted — but supersession only begins when the next request is issued, up to
a second after the buyer changes company. A response for company A landing in
that window painted A's verdict with B's name and number substituted in: a
decline, or an approval, attributed to the wrong company. The label is now
snapshotted when the request goes out. The live read stays for callers that
re-render rather than report a response, where the DOM IS the truth;
- four sites blanket-hid every pay-box where they meant to clear the verdict —
the picker's `select2:select`, the `#billing_company` change handler, the
container change handler, and clearSelectedCompany()'s 3s deferred re-read.
Each took the spinner down for a request still in flight, and the deferred one
fires three seconds late so nothing re-armed after it. The container handler
was also bound by reference, so jQuery passed it an Event as its `action` — it
did the right thing only by accident;
- a pending verdict paint could outlive its own check. Neither the issue path
nor the cached branch clears `renderInterval`, so a paint left over from an
earlier response fired afterwards and put a stale verdict over the newer
check's loader. Reachable with an `updated_checkout` in the second between a
response and its paint. Guarded by the check's own seq;
- the cart-total give-up called the blanket reset. No loading state is up during
the price wait, so there was nothing of that check's to take down and it
erased whatever else was on screen — an earlier request's verdict — with
nothing left to re-arm. It disarms quietly now, and leaves an outstanding
request alone;
- the readiness guard now counts a PENDING PAINT as well as a request in flight.
Once stillCurrent() has banked the response only the paint is left, and letting
it land writes a verdict about a form the buyer has since emptied.
Five comments still asserted the reverted design as their live rationale, which is
the shape that sends the next reader chasing a defect that no longer exists. One
docblock had drifted onto the wrong function. Both fixed. The `#place_order`
handler now aborts an outstanding POST where it used to only disarm; that is
recorded in place, with why it cannot cost a tracking id.
Tests: 449 JS, 193 PHP cases. Eighty mutations, each killing at least one test.
Five of round 5's findings came from mutation rather than review — `updateElements()`'s
own clear was vacuous on a complete form, no test abandoned with `pendingCheck`
true, the deferred re-read's guard and its tile call were both unpinned, and
`inFlightXhr`'s release was unpinned. The harness gains
`record.abortedWhilePending`: `aborted` flips even for an abort of an
already-settled deferred, where a real jqXHR does nothing, so it proved only that
the call was made.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pany (TWO-25326)
Round 6's exhaustive sweep of the three properties that matter came back clean:
no path shows the loader without taking it down, none takes it down while a
request is outstanding, and — after this commit — none paints a verdict that is
wrong for the current form state.
- `resolveCompanyLabel()` honoured an EMPTY snapshot as though it were a
company's label. It is not the same fact as "this company has no label": it
means the DOM read at request time came back blank, which happens while
WooCommerce is mid-replacement of the billing fields — `readCapturedCompany()`
reads the inputs, and those go empty for an instant even though
`customerCompany`, which `isReadyApprovalCheck()` has already proved complete,
does not. The result was the served no-company fallback sentence printed over
a verdict for a company the buyer certainly had. A blank snapshot carries no
information, so it now falls through to the live read at paint time;
- `clearIntentVerdicts()` listed the three verdict classes by name. Same result
today — those three plus the loader are all this plugin renders — but a brand
overlay or a later ticket adding a fourth verdict box would silently not be
cleared, and one stale box surviving every clear is a symptom a long way from
that cause. It now says what it means: every pay-box except the loading state.
Tests: 452 JS, 193 PHP cases. Eighty-three mutations, each killing at least one
test. Both halves of the selector change are pinned — a synthetic fourth box IS
cleared, and the loader is NOT.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd-6 change (TWO-25326)
Round 6's mutation sweep (64 new mutations) found 24 survivors. Reading had
flagged none of them. Also reverts a change made earlier in round 6 on a
misdiagnosis.
REVERTED — resolveCompanyLabel's empty-snapshot handling. It was changed to fall
back to a live DOM read when the snapshot is "", on the reasoning that a blank
snapshot carries no information. That is wrong, and reintroduces the exact defect
the snapshot exists to prevent: by paint time the buyer may have moved to another
company, so the fallback substitutes a name unrelated to the verdict. "" means the
capture read blank when the request went out, and the served no-company sentence
is the right output for that. `typeof`, not truthiness, and now tested.
Coverage holes closed:
- the APPROVED branch's company snapshot was unpinned — the only wrong-company
test used `approved: false`. An approval naming the wrong company is the more
damaging direction of the two and it was uncovered;
- BOTH company-field change handlers had no test at all. Swapping either to the
blanket hide, and deleting either's clear outright, all survived. One of them
is `#billing_company`, the manual-entry path a buyer types into. Round 5's
commit claimed four blanket hides fixed; only two were pinned;
- poTranslation() — the .po parser rounds 1-4 built to replace a raw regex — had
EIGHT of eight mutations survive, so every safety property in its 30-line
docblock was unverifiable, including the fuzzy case the docblock itself called
"latent today". Direct cases now: fuzzy alone and in a multi-flag list, a
non-fuzzy flag that must NOT reject, msgctxt, CRLF across an ENTRY BOUNDARY (a
single-entry fixture leaves the entry splitter unexercised — one mutation
proved exactly that), plurals, first-occurrence-wins, escaped quotes both
ways, and a shorter msgid not matching a longer one;
- the quiet cart-total give-up's `pendingCheck = false` and its
deliberately-not-superseding were both asserted vacuously: `pendingCheck` was
already false and the request already settled by the time the give-up ran;
- the paintSeq guard's `clearInterval` was unpinned — nulling the handle alone
leaks a 1s interval per superseded paint, forever;
- `record.abortedWhilePending`, added in round 5 to be stronger than `aborted`,
was provably indistinguishable from it: no test asserted it FALSE, and all
three mutations of the `settled` guard behind it survived. It now has its own
suite, since no production path aborts a settled request and the distinction is
otherwise unreachable;
- the select2:select test's two clear-assertions ran against a box that was never
shown. New `revealVerdictBox()` stages a verdict without disturbing the loader,
which `togglePaySubtitleDesc()` cannot do.
Two survivors are left deliberately and recorded in place as EQUIVALENT mutations
rather than gaps: the redundant `blankToEmpty()` on the company name
(`formatCompanyLabel()` applies it again downstream), and widening
`resolveCompanyLabel`'s `typeof` test to truthiness, which is now behaviour-
preserving.
Tests: 460 JS across 15 suites, 194 PHP cases. 101 mutations verified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e boundaries (TWO-25326)
Two MAJOR findings, both in code earlier rounds added, and both of which the
code's own comments had already described as failures while still causing them.
- the paint give-up handed the tile to a check that was only ARMED — no request
issued — and raised the loader for it. That check could then die in the
cart-total give-up, which disarms with NO UI touch precisely because it is
entitled to assume no loading state of its own is up. Nothing could lower it
afterwards: `clearIntentVerdicts()` excludes the loader by construction, and
`abandonOrderIntentCheck()`'s gate then reads false on every flag, so
`#place_order` and `checkout_error` skipped their reset too. On exactly the
carts the bound exists for — a 0 total, totals markup `getPrice()` cannot read
— the spinner was up for the rest of the page.
Fixed by DELETING the hand-back rather than narrowing it again. This branch had
been rewritten three times (rounds 3, 4, 7) and a mutation replacing it with
`if (false)` survived the whole suite, which is what exposed it as unreachable:
getting there requires `paintSeq === seq`, and an outstanding request implies
`paintSeq !== seq`. It is an unconditional reset now, and the behaviour the
hand-back was reaching for is delivered by the `paintSeq` guard, separately
pinned;
- `clearSelectedCompany()`'s 3s deferred closure wiped a correct, current verdict.
Its comment named this exact failure — "wipe a verdict painted a second and a
half ago, with nothing left to re-arm" — as the reason a blanket hide was wrong,
then kept the clear for the verdict half. Country change -> the handler's own
`getApproval()` arms a check -> request at ~1s -> verdict at ~1.5-2s -> the
closure wipes it at 3s, and neither a country change nor `checkout_error` fires
`updated_checkout`, so nothing repaints. The closure now touches the tile not at
all; the synchronous reset above already retired the outgoing verdict at t=0,
the only moment it was the stale one.
- MINOR: `checkout_error` re-armed unconditionally, so `getApproval()`'s own clear
wiped a good verdict the abandon had just been careful to leave alone — with no
repaint for at least a second, or at all quickly for an approval, which is never
cached. Every failed submit for an unrelated reason flickered the box.
`abandonOrderIntentCheck()` now returns whether it stopped anything.
Coverage, from 33 new mutations:
- the cache window's boundaries were unpinned in both directions. 400 added to the
retryable list, `>= 400` narrowed to `> 400`, and `< 500` widened to `<= 500`
all survived, because the only cacheable control was 422 and the 5xx cases used
502 and 503. Now parametrised over 400/422/499 cacheable and 500/503 not;
- `getUnsecuredHash` returning `inp.length` survived everything, so the cache had
only ever been exercised on bodies of differing length. A buyer swapping one
8-digit org number for another would have been served the previous company's
verdict. Pinned as a unit and end to end;
- a cacheable verdict with a falsy request hash could be filed under a blank key.
Tests: 471 JS across 15 suites, 194 PHP cases. 144 mutations verified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…TWO-25326)
Two reviewers, each in its own worktree this time (round 7's correctness reviewer
could not run the suites because both shared one and the mutation sweep kept it
dirty). 52 mutations, 14 survivors.
Production:
- a 200 whose JSON body parses to `null` made every read of the response a
TypeError — thrown after `stillCurrent()` had released `inFlightSeq`/`inFlightXhr`
and before the paint was armed, so the loader was up for the rest of the page
with nothing able to reset it. Same class as the `responseJSON` and
`Array.append` throws round 1 fixed, but on the SUCCESS path, which those guards
never covered. Normalised to `{}`, which reads an unusable body as not approved;
- the verdict's company label now comes from `customerCompany` — the same record
the request BODY is built from — instead of `#billing_company`/`#company_id`.
Those two diverge, and `clearCompanyIfCountryStale()` exists because of it and
documents the case (a number typed with no blur); divergent, the sentence named
a company the API was never asked about. One source for both now, and a
consequence worth having: `isReadyApprovalCheck()` refuses to issue unless every
field of the record is non-empty, so a request in flight always has a company.
Reverted, on its own merits: a `wasShowing`/`wasRunning` split in
abandonOrderIntentCheck(). The verdict-wipe it was meant to prevent cannot happen —
every route that arms a check calls `clearIntentVerdicts()` in the same breath, so
"armed" already implies "nothing of ours on screen". Same reasoning as round 7's
deletion of the paint give-up's hand-back.
Coverage:
- the `enable_order_intent` gate was entirely unpinned — deleting it and loosening
it to a truthiness test both survived, because no suite anywhere set the flag to
anything but "yes". It is the merchant switch for this whole ticket's UI;
- the `isAnyElementEmpty` arm of `isReadyApprovalCheck()` was unpinned: the
organisation-number guard above it is tested, so nothing covered a company with a
number but no name, which is the only reason the array check exists;
- a zero cart total is now tested. The give-up tests had claimed that case in their
comments since round 1 while only ever exercising MISSING markup, and those
comments are corrected. Narrowing `!gross_amount` to `=== undefined` turns out to
be an EQUIVALENT mutation, not a gap — `getPrice()` cannot return 0, because
`getPriceRecursively()` gates recursion on `if (val)` and discards a "0.00" text
node as falsy (verified directly). The real, pre-existing behaviour that leaves is
now pinned and flagged: a fully-discounted order can never obtain a verdict;
- the catalogue test's glob is asserted to have VISITED every locale. Narrowing it
to one hardcoded filename passed identically, which is the exact failure the glob
was introduced to prevent;
- the spinner's `flex-shrink: 0`, which keeps it from being squashed beside a longer
translated sentence.
Tests: 477 JS across 15 suites, 194 PHP cases. 190 mutations verified.
Pre-existing findings NOT addressed here, for their own tickets: `getPrice()` is
untested against real WooCommerce price markup (`<bdi>`, a currency-symbol span) and
against `price_thousand_separator`/`price_decimal_separator`, which no fixture
configures — a cart over 1000 in a comma-thousands locale reads as no total at all;
the `tax_amount = 0` fallback and the request body's own fields (currency,
invoice_type, tax_rate, tax_class_name) are unasserted. None is touched by this diff.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
Review: eight rounds. Stopping here, with one item that needs your ruling.Supersedes my earlier comment. Two reviewers per round — runtime correctness, and test vacuity driven by mutation — each round reading the pushed ref. 190 mutations verified, every one killing at least one test. I am not merging. One open question below is a product decision, not a defect I should keep patching. The ticket — done, and stable since the first commitNo review round has found anything against these four. Everything after round 1 was the order-intent request lifecycle underneath them.
The one thing I got structurally wrong, and how it was fixedRound 1, I moved the loading state from "shown when the request is issued" to "shown when a check is armed". That decoupled the spinner's lifetime from the request's, and rounds 2, 3 and 4 each found defects created by the previous round's fix — a gate that reintroduced the very defect it was added to prevent, a supersession counter that then needed bumping in four more places, a give-up that blanked a newer check's spinner with a test of mine asserting the blank as correct. Round 5 reverted that one decision. The spinner is tied to the request again — pre-existing, already-shipped behaviour — and Twice more the same lesson landed: round 7 deleted the paint give-up's hand-back after a mutation proved it unreachable (it had been rewritten three times), and round 8 added then reverted a Real bugs found along the way
Open — needs your rulingA response can still act on the checkout for up to a second after the buyer changes company. Supersession begins when the next request is issued, so in the arming window the previous company's response is still current: it deselects the gateway, writes Round 5 chose "name the right company" over "don't paint at all", and Two smaller judgement calls, both kept on the PrestaShop side because converging the four checkouts is the point: Follow-up tickets (pre-existing, untouched by this diff)
Verification477 JS tests across 15 suites · 194 PHP cases · CSS is asserted through jsdom's real cascade, not a source grep — three mutations that defeat a grep fail against it. One documented jsdom gap: it does not honour by Claude |
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
1 participant
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.
What
Brings this checkout's order-intent UI onto the shared target agreed 2026-08-04 for TWO-25326, so all four plugin checkouts look and read the same during and after the availability check. The other three platforms are being brought onto the same spec in parallel.
1. While the check runs
The shared spinner GIF beside the visible words "Checking availability".
Before: three pulsing dots and no words at all — the only one of the four checkouts saying nothing about what it was doing. The sentence existed, but only inside a screen-reader-only span.
The GIF is byte-identical to the one the Magento front ends ship, and it is already in this tree for the company-search field, so this is a second rule pointing at an existing asset rather than a new one.
.twoinc-dotsstays in the stylesheet — the term-chip fee quote still uses it..twoinc-sr-onlygoes, with the hidden sentence it existed for.2. The verdict
A coloured, bordered box: green for approved, red for the two failure boxes, palette matched to the PrestaShop module as the reference implementation. Message only, no title.
Before: all three boxes carried no styling at all beyond
overflow-wrap, so a "not available for this order" verdict rendered as ordinary tile copy in the theme's text colour.WooCommerce core's own
.woocommerce-message/.woocommerce-errorconventions were deliberately not reused: they are top-border-only bars whose accent colour is themeable, so a themed shop would render a decline in the theme's brand colour, and they are laid out for full-width page notices rather than a box inside the payment tile.3. A new check clears the previous verdict
The reported bug: the buyer changed their selected company and the tile kept showing the old company's verdict for the whole of the new check, because the only thing that cleared it was the new result arriving.
Fixed at
getApproval(), the one choke point every route into a check passes through. Four of the five routes (setSoleTraderCompany,onCompanyInputBlur,onRepresentativeInputBlur,onCountryChange) cleared nothing of their own; the fifth — the company picker'sselect2:select— still left a second of stale verdict on screen, because the check is armed on a 1s interval. Entering the loading state there both clears the old verdict and shows the spinner immediately, and any route added later inherits it.Two supporting changes fall out of that:
updateElements()now runs its hide-every-pay-box reset before arming the approval pass. In the old order the reset wiped the loader the pass had just shown, and the buyer saw a blank second.4. The loading state always resolves (review round 1)
Showing the loader when a check is armed rather than when its request goes out exposed four paths that ended a check without saying anything about the UI. Invisible before, a permanent "Checking availability" after. All four now go through one
abandonOrderIntentCheck():pendingCheckpermanently true — the guard sets it whenever a timer exists and nothing could ever clear it — so the 3s poller re-enteredgetApproval()forever. A loader/verdict flicker on a ~3s cycle with no request behind it. It now disarms; the answer is already in hand.#place_orderandcheckout_errordisarmed the timer only. The second is the worse one: it does not triggerupdated_checkout, so nothing re-renders the tile and the spinner would sit beside the validation errors for the rest of the page.0— falsy on every tick — and a theme whose totals markupgetPrice()cannot read never yields one at all. Bounded to ten ticks.Two response paths threw before reaching the only code that renders a verdict and takes the loader down, so both stranded it. Pre-existing, but this PR made the loader the thing that clears the previous verdict, so a stranded loader is now the whole tile:
status >= 400with noresponseJSON— a proxy 502 carrying an HTML error page — where"error_details" in undefinedis aTypeError;invalidFields.append().Arrayhas noappend, so the only route to the phone-number box has always thrown and that box has never once been on screen. Without this fix, the red border added above would have been styling unreachable UI.5. Accessibility (review round 1)
The three verdict boxes are now announced:
role="status"on the approval,role="alert"on the two failures. The loader carried a role and the verdicts carried nothing, so a screen-reader buyer heard that a check had started and never heard how it ended — and the colour this pass adds is no help to them at all. Polite for the approval, assertive for the failures, which have just deselected the payment method under the buyer.Copy and translations
Checking availability, catalogued and translated for the three locales this plugin ships:The sentence is visible copy now rather than screen-reader-only, so an untranslated catalogue is a visible English string on those shops.
.mofiles recompiled;check-catalogues.shagrees.Tests
New suite
tests/js/intent-loading-state.test.js(26 tests): the clearing behaviour, thependingCheckpath, the two cases where nothing should change (incomplete company; a brand with the notice suppressed and so no loader element at all), every way a check can end without a response, the cached-verdict branch, the two throwing response paths, and the stylesheet. Driven throughgetApproval()rather than per-caller, deliberately — a test per route would have missed exactly the four routes that were broken.The stylesheet is asserted through jsdom's real cascade (
injectStylesheet()+getComputedStyle). Round 1 started with raw-text greps over the CSS and proved them blind three ways — to a commented-out declaration, to a later overriding rule, and to an at-rule-wrapped copy — all three of which now fail.tests/unit/run.php: the loader-markup test rewritten as one composed string (three independent substring checks passed however the nodes were ordered), a bare-sentence test covering all three boxes rather than one, an ARIA-role test per box, and a catalogue test that parses the.poto assert the msgid→msgstr pairing — searching for the two independently passed when the translation was attached to a different msgid, which is a shop rendering the wrong sentence.Mutation-verified: 16 mutations, each killing at least one test.
assets/js/twoinc.js— removing the clearing fromgetApproval()fails 7; re-swappingupdateElements()'s two calls fails 1; dropping the abandoned-tick reset fails 1; leaving the cached branch armed fails 2; removing the price-wait bound fails 1;push→appendfails 1; removing theresponseJSONguard fails 1; reverting both abandon handlers to their inline disarm fails 2; deleting the tick's re-asserted loading state fails 1.assets/css/twoinc.css— all six of the mutations named above.class/WC_Twoinc.php— dropping either role, injecting an<h4>into a verdict box, and swapping the loader's two spans. Plus a catalogue mis-pairing on the.po.Verification
npm run test:js— 396 passed, 14 suitesphp tests/unit/run.php— all passed (194 cases)pre-commit run --all-files— passedphpcs— 0 errorsphpstan analyse— no errors.github/scripts/check-catalogues.sh— all catalogues agreeOpen for a ruling
Two deliberate choices where matching PrestaShop and following this repo's own conventions pull in opposite directions. Both kept on the PrestaShop side, because converging the four checkouts is the point of the pass:
font-size: 14pxon the verdict boxes is absolute, where every other buyer-facing size in this stylesheet isem. It overrides theme typography and does not follow a rem-based theme's user font scaling.#28a745against a#d4eddafill — 2.53:1, below WCAG 1.4.11's 3:1 for non-text. Not a hard failure (the sentence, not the border, carries the meaning) but it is the one number in the palette that misses.#1e7e34would clear it, at the cost of diverging from the other plugin.The sentence also differs from PrestaShop's by an ellipsis: PrestaShop ships
Checking availability..., this shipsChecking availability. The latter is the agreed target string.Not covered
Whether the spinner actually animates, and how the boxes look, are both beyond jsdom — the computed
background-image, box metrics and colours are asserted, the rendered paint is not. Needs a look on a real shop.