chore: sync staging → main - #341
Open
two-inc[bot] wants to merge 498 commits into
Open
Conversation
…al-entry row Adversarial review round 1 (Han) found two real gaps: - the observer created by attachManualEntryRow() was only disconnected on select2:close/dispose, not on the re-bind path enableCompanySearch() takes on every re-render — a re-render while the picker is open can replace the select2 instance without emitting select2:close first, orphaning the previous observer's detached results list. - clearCompany() tore the select2 instance down synchronously from inside select2's own select2:selecting dispatch; deferred the teardown a tick so select2's own post-preventDefault bookkeeping for that event finishes first. Tests pin both: the re-bind path now asserted to call detachManualEntryObserver before re-init, and the selecting handler asserted to defer clearCompany() via setTimeout rather than call it inline.
…ield Ports the reference company-search UX: the payment tile's company-name field now shows the selected company's registry id in light-grey text to its right, cleared with the companyId observable. Cosmetic only — getData() keeps reading companyId() directly, nothing new writes it. Also hides the address step's separate "Company Number" field purely via CSS (new .two-company-id-hidden class), rather than flipping its `visible` config to false: address-autocomplete.js resolves the field through uiRegistry.get(), which a visible:false would remove from the registry entirely. The DOM node and its submitted value are unaffected.
Review flagged that the comment described the address-step fix as already landed; #305 is still open/unmerged. Reworded to reflect that this PR tackles the payment-tile picker independently.
TWO-25279/feat(surcharge): require a merchant tax rule for the surcharge tax treatment
…entry The "Search for company" link only re-bound the picker and hid itself, leaving the buyer on a closed dropdown they had to click a second time before they could type. It now asks the fresh widget to open, which runs the existing `select2:open` handler and puts the caret in the search box. The request is a one-shot local, not a component flag: the bind runs inside a `$.async` MutationObserver that fires again on every re-render, so a persistent flag would pop the dropdown open under a buyer who has moved on. The initial render passes no option and is unchanged. Test/Js/company-search-return-to-search.test.js drives the real journey on both pickers and asserts the real DOM — an open container, one live `.select2-search__field`, and `document.activeElement` being that input. The select2 double throws on `open` without an instance, so opening before the re-bind cannot pass. amd-harness gains an optional globals argument so the module can see jsdom's document instead of the inert stub. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A limit of exactly 0 survived the grid: validateValue only rejected
negatives, and only an empty cell deleted the config row. Zero is
never what an admin means by it.
The limit bounds the WHOLE fee line - the percentage part and the
fixed amount together, not the percentage alone - so a limit of 0
silently wipes a configured fixed amount too, with nothing in the
admin copy saying so. And the intent it gets mistaken for ("charge
nothing on this term") is already expressible directly, by entering 0
in the fixed and percentage cells.
So: refuse it at entry, and say what to do instead. An EMPTY limit
stays valid and still means "no limit" - absence and zero are
different values, and only the explicit zero is refused.
- Model/Config/Backend/SurchargeGrid: reject `limit` === 0 (the
authority; reachable from the admin UI, from `config:set`, and from
app:config:import alike).
- surcharge-grid.phtml / surcharge-grid.js: the same refusal in the
browser, as a registered rule rather than Magento's own
validate-greater-than-zero, which is locale-blind - parseFloat('0,5')
is 0 for a Dutch admin and would reject a legitimate half-unit
limit. Applied to rendered rows and to rows the JS adds live.
- Admin copy: state that a limit of 0 is not allowed, and - on
fixed_and_percentage, the only mode where it is load-bearing - that
the limit applies to the whole fee rather than the percentage alone.
Tests: the pre-existing SurchargeGridTestable stub reimplements the
save flow, so it cannot pin a validation RULE - breaking the
production check could not turn a stub-only test red. The three new
tests therefore invoke the production validateValue directly.
Verified by mutation: neutering the check reddens
testProductionValidatorRefusesAZeroLimit only; widening it to `>= 0`
reddens testProductionValidatorAcceptsAPositiveLimit. Three existing
fixtures carried `'limit' => '0'` incidentally and would have started
passing for the wrong reason - emptied.
Refs TWO-25289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…, sv_SE) Append-only, kept as its own commit so it rebases cleanly behind the other i18n work in flight on this repo. Four new rows per locale: the save-time error, the browser-side rule message, and the two admin-copy sentences. Terminology follows the rows already in each file - Limit = Grense / Limiet / Grans, Fixed Amount = Fast belop / Vast bedrag / Fast belopp - rather than being translated afresh. Refs TWO-25289. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e-contradict the guidance Two things, both fallout of refusing a zero limit at the admin boundary. 1. `convertAmount()` sent its result unrounded. The API refuses monetary values finer than two decimal places rather than rounding them, so an FX conversion landing on >2dp (349 * 0.0872 = 30.4328) was rejected upstream and surfaced to the buyer as the generic "temporarily unavailable" error. Both `cap` and `surcharge` are now rounded, on the no-conversion path too - an admin can type more precision than the API accepts. Plain half-up rounding, deliberately. Sub-cent caps, away-from-zero rounding and zero-decimal currencies are all out of scope: the one value where the rounding direction would have mattered, an explicit cap of 0, is refused by the grid instead. A sub-cent cap rounding to 0.00 is pinned by its own test so it reads as a decision rather than a surprise. 2. The prose merged with the TWO-25269 revert told the next reader not to add admin validation rejecting a typed zero - correct as a statement about the RUNTIME relay, wrong as guidance, and sitting in the first file an implementer of this change would open. Rewritten in both places (the inline comment and AGENTS.md) to hold both rules and say why they do not conflict: the runtime relays a zero cap faithfully because the API bounds the fee at zero rather than uncapping it, AND the admin refuses zero because a merchant wanting no fee says so with 0% and 0 fixed, while the sibling plugins were turning a zero cap into "absent" and relaying it genuinely uncapped. The test that pinned the unrounded pass-through was pinning the bug - and asserted nothing, so it was also risky-flagged. Replaced with two that pin the rounding. Verified by mutation: dropping either round() reddens exactly the test for that path. Refs TWO-25289. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nt limits, non-numeric input
Adversarial review found one blocker and five real defects. Fixes:
BLOCKER - a limit of 0 stored while it was still legal made the whole
Two payment section UNSAVEABLE. The grid JS hides the Limit column for
a fixed-only surcharge type, but a hidden input still posts, and
client-side validation skips hidden fields, so the stored 0 arrived
server-side and threw - naming a cell the admin can neither see nor
clear. Now an inapplicable limit is DELETED rather than validated,
which also retires the legacy row instead of leaving it to resurface.
Same reasoning the funding-partner cap check already carries: failing
a save over an invisible cell is a dead end.
The type gate reads the POSTED surcharge type, not the stored one: type
and grid are saved in the same request, so the stored value is the
previous one and would misjudge the merchant switching type - which is
exactly the case that decides whether their save is blocked.
A SUB-CENT limit (0.001) passed validation and then became a hard cap
of 0.00 once rounded for the wire - the very outcome being refused,
one step later. The check is now round($value, 2) === 0.0, so the
rounding direction cannot decide whether a configured cap survives.
Both prose claims to that effect were false as written; corrected
rather than deleted, because the FX-conversion case genuinely remains
and is pinned as accepted.
NON-NUMERIC input had no server-side check at all. Cast to float,
'abc' is 0.0, so it was rejected as "a limit of 0 is not allowed" -
wrong and unactionable. validateValue() now takes the RAW string so it
can tell the two apart, with its own message.
A stale docblock paragraph directly contradicted the new one ("the
result is returned unrounded"); deleted. The MONEY_DECIMALS comment
claimed every monetary value is rounded, when gross_amount and
rounding.step are not; scoped to what it does.
The registered JS rule had three defects: $t() around a
`+`-concatenated literal, which Magento's phrase collector cannot
harvest, so the three new i18n rows were dead and the message would
have stayed English; the message resolved at define time, before the
dictionary is guaranteed registered; and an `if ($.validator && ...)`
guard that would silently skip registration while the rendered
data-validate attribute still named the rule, making jquery.validate
throw on submit and kill validation of the whole form. One unbroken
literal, lazy function, no guard.
Two of round 1's new tests were exercising the test stub rather than
production code - the exact can't-fail pattern this PR's own commit
message called out. Both moved onto the real methods via reflection.
Mutations verified: dropping is_numeric, inverting the type gate, and
reading the stored type instead of the posted one each redden exactly
one test.
Refs TWO-25289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…, and test the rule
Jest went red on the surcharge-grid smoke load: dropping the
`if ($.validator && ...)` guard exposed that the AMD harness's jQuery
mock carries neither `$.validator` nor `$.mage.parseNumber`, both of
which `mage/validation` provides in the browser. A jQuery without them
is a shape no browser presents, so the failure was testing the mock,
not the module - the guard had been hiding a harness gap as well as a
real defect. Mock fixed rather than guard restored.
The mock's parseNumber reproduces the comma-decimal behaviour
deliberately: parseFloat('0,5') is 0, and that difference is the entire
reason this rule exists instead of Magento's locale-blind
validate-greater-than-zero. A mock that used parseFloat would let the
locale bug through unnoticed.
With the registry mocked, the rule itself is now directly testable, so
it is tested rather than merely smoke-loaded: zero in every spelling,
sub-cent values, empty, comma decimals, non-numeric left to the
sibling rule, and the message being a lazily-resolved function.
Mutations verified - reverting the sub-cent rounding, and swapping
parseNumber for parseFloat, each redden one case.
Refs TWO-25289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p rather than delete, non-finite limits
Round 1's fix for the unsaveable-grid dead end had three problems of its
own, all found by round 2.
DELETING an inapplicable limit was wrong twice over. It discarded a
VALID limit on any save made while the surcharge was fixed-only or off -
a normal round trip, and one that kept the equally inapplicable
percentage cell. And at a non-default scope, deleting an override does
not retire a value at all: it re-exposes the parent's, so a legacy zero
at default scope produced an unsaveable loop at store scope - reject the
posted inherited zero, clear it, the override is deleted, the next
render shows the inherited zero again, and the error's "leave the limit
empty" was actively wrong there. The column-visibility gate now
suppresses only the ZERO RULE; the cell is stored either way, and a
legacy zero resurfaces when the column comes back into view, which is
where the admin can act on it.
The TYPE READ was unscoped. The config fallback is not an edge case: a
type field left on "Use Default Value" renders its select disabled, and
browsers do not submit disabled inputs, so nothing is posted for it -
which makes the fallback the normal path at a non-default scope, where
an unscoped read returns the DEFAULT scope's value. Default fixed +
website percentage would have silently deleted a store's real limits;
the reverse would have blocked its whole section save. Resolved at the
saving scope now, as the sibling render block already does.
is_numeric('1e400') is true and the cast is INF. Limit is the one column
with no upper bound, so INF was stored and then failed the pricing
request at serialisation time, a long way from the cause.
Test debt from round 1, which the review was right to call a recurrence:
the delete-path test exercised only the stub, so removing the production
wiring kept the suite green. The rule and the type gate are both pinned
against production code now, including the scoped fallback (via a
reflection-injected scope config), and the stub keeps only the flow it
is honest about. MONEY_DECIMALS is duplicated across the grid and the
calculator and the whole correctness argument depends on the two
agreeing, so a test now pins that.
Also: the Jest parseNumber mock only handled a single comma, so
'1.234,56' parsed as 1.234 - the mock diverging from the real parser in
exactly the locale behaviour it exists to cover, which is false
confidence rather than coverage. It implements last-separator-wins now,
with a test.
Copy: the zero-limit sentence loses its possessive apostrophe, matching
the sibling plugins. PrestaShop keys its translations on the md5 of the
BACKSLASH-ESCAPED source, so an apostrophe there silently yields a dead
key; sidestepping it in the shared wording beats getting the escaping
right in one plugin and wrong in the next.
Mutations verified: dropping the visibility gate, unscoping the fallback
read, dropping is_finite, and breaking MONEY_DECIMALS parity each redden
exactly the intended test.
Refs TWO-25289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… pin the visibility wiring, de-stale the doc Three round-3 findings. 1. The stored surcharge limit was cast blindly on read, so a value that never came through the admin grid decided the buyer's fee. A hand-edited row, a config:set or an import could store `abc`, which cast to a hard cap of 0.0 and suppressed the fee outright, or `-10`, which was relayed as a negative cap and refused upstream so the buyer saw a generic failure. Non-scalar, empty, whitespace-only, non-numeric, non-finite and negative now all resolve to NULL, i.e. absent, i.e. no cap. A genuine 0 is still relayed verbatim: a zero cap clamps the fee to zero, which is a different instruction from absence. Same shape as the sibling plugins. 2. Round 2's limit-column visibility wiring was untested. validateValue() defaults the flag to true, so deleting the argument at the call site — or dropping the term from the rule — compiled and left the suite green while reintroducing the failed-section-save regression round 2 removed. Both directions are now pinned against the REAL afterSave(), which needed afterSave() and an explicit setData() on the test stubs before the production method was callable at all. 3. AGENTS.md still described the hidden Limit column as deleted rather than validated. Round 2 changed that to skipped-not-deleted, so the doc was instructing a future reader to reintroduce the regression. Corrected, with the reason deleting is wrong, and a new section for the read-path guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nostic, not the result The array case asserted only that the limit read as absent, which held with or without the guard: the string cast yields "Array", which is non-numeric and lands on null anyway. The cast is a PHP warning rather than an error, so the assertion that actually distinguishes the two is that no diagnostic is raised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…manual-entry-affordance # Conflicts: # Test/Js/amd-harness.js # view/frontend/web/js/model/company-search.js
…-hint-magento TWO-25288: grey inline company-id hint + CSS-hide address-step field
TWO-25289/fix(surcharge): refuse a limit of 0 in the grid, round relayed amounts to 2dp
…staleness Round 2 adversarial review (Han, Vader, both independently) found that the setTimeout(0) deferral added in the previous fix had no staleness check: every other deferred/async call in this handler chain (onSearching, onUnavailable, clearSearchChrome, attachManualEntryRow) gates on the bind staying current, but the manual-entry teardown acted unconditionally through `self`. A checkout re-render landing in the zero-delay gap between the buyer's click and the timer firing would tear down whatever widget is live NOW rather than no-op for the stale bind, and the eagerly-resolved `$searchForCompany` reference from the same gap could operate on a detached node. Gates the deferred callback on companySearch.getSearchFieldContainer() staying non-empty for the bind token, and moves `$searchForCompany` resolution inside the deferred body instead of before it (Yoda, round 2). New regression test proves reverting the staleness check goes RED.
feat(TWO-25288): open and focus company search on return from manual entry
…ffordance fix(company-search): move payment-tile manual-entry affordance inside the results listbox
TWO-25288. Four cases in company-search-return-to-search.test.js fail on
staging, on nobody's branch. The payment surface's manual-entry exit stopped
being a "#billing_enter_details_manually" link below the results and became a
cancellable row inside them, but that landed AFTER this file did, so the
helper's click found no node and every case that routes through manual entry
died in the fixture.
Drive the real route instead: trigger the preventable select2:selecting
pre-event with a sentinel row, then run the deferred teardown. Two things the
straightforward version of that gets wrong:
- jest.useFakeTimers() cannot reach the deferral. The AMD harness copies the
real setTimeout into its vm sandbox when the module loads, so a later timer
swap patches a binding the module never sees. The fixture supplies a
queueing setTimeout through extraGlobals and flushes it explicitly, which
also keeps the defer visible rather than collapsing it to a synchronous
call.
- the teardown is gated on the bind still being current, which the default
company-search mock answers with null. getSearchFieldContainer now reports
a live bind, alongside the sentinel-aware isManualEntryOption the shipping
surface already needed.
The flush reports how many callbacks it ran and the helper refuses a run that
deferred none, so a future regression that stops reaching the handler fails
loudly instead of asserting against a widget nothing asked to tear down.
Verified against origin/staging with only this file changed: 249/249, from
4 failing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Supersedes the removal of the payment tile's company-number input. The reason
it was removed holds — a hand-typed organisation number is not an accepted
source; it produced poor data quality and genuine buyers receiving invoices for
orders they never placed — but removing the field also stopped the buyer seeing
what the order would be invoiced against. Read-only keeps the first and
restores the second, on every capture mode.
The number field is back as a labelled input, `readonly` as a STATIC attribute
(there is no mode in which it may be typed), bound to `value: companyId`, and
deliberately carrying no `name` and no `required`:
- no `name`, so it adds no second carrier for a value getData() reads off the
observable. The observable stays the only thing that ships.
- not `required`, because a read-only empty required field fails validation
with no way for the buyer to satisfy it. Model/Two.php::authorize() remains
the only enforcement of the number's presence, unchanged by this commit.
- `readonly`, not `disabled`: a disabled control is dropped from the
submitted form and skipped by jQuery Validation's `elements()`.
The name field's `readonly` is BOUND, to sole-trader mode only, and the
asymmetry is load-bearing. Sole trader is the one mode where that node is a
plain text box holding a captured name the buyer must not edit. In search mode
select2 replaces it with its own chrome; in manual-entry mode the buyer MUST be
able to type, and a static `readonly` would brick that mode outright.
clearCompany() now blanks `companyId()`. While the tile had no number field the
stale observable was invisible and was flagged as out of scope; with the number
displayed, leaving it set would show the buyer the ABANDONED company's registry
number, uneditable, beside a name they are being asked to retype — and submit
that number under the new name. `companyName()` is deliberately NOT cleared: it
is read after the call on the sole-trader path (getAutofillData() prefills the
signup popup from it) and by the intent-approved notice.
The grey inline hint that briefly stood in for the field is gone, markup and
CSS together — a labelled field supersedes it, and both would have shown the
buyer the same number twice. Its address-step half, the CSS-only hide, is
untouched and keeps its own pins.
Tests read the `value:` and `readonly:` binding targets out of the template and
evaluate them against a renderer driven through each real capture flow, so a
field bound to the wrong observable fails even with correct-looking markup.
Every read-only assertion in the first draft passed against a template with the
attribute DELETED, because `\breadonly\b` matches inside the class name
`two-company-id-readonly`; attribute matching is now anchored on whitespace.
Caught by mutation, and each of five mutations is confirmed to fail the suite:
dropping the static attribute (5), removing the field (6), making the name
statically read-only (2), unclearing companyId in clearCompany (1), and
rebinding the number to companyName (5).
npm run test:js: 21 suites, 253 tests, all passing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e escape hatch Two real findings from an independent adversarial round on the diff. MAJOR — the name field could be blank, `readonly` and `required` at once, which is a validation error no buyer action clears. jQuery Validation enforces `required` on a `[readonly]` field; its `elements()` skips `:disabled` only. Keying `readonly` on sole-trader mode alone hit that: enterSoleTraderUi() blanks the input, and the autofill only refills it when the prefetch matched a buyer — fillCompanyData() early-returns unless BOTH name and number are non-empty. On the unmatched branch the buyer is sent to signup and may abandon it, and any autofill buyer with an empty organisation number lands there too. Before this change the buyer could retype; the first draft took that away. Now bound to isCompanyNameReadOnly(), which is `showSoleTrader() && !!companyId()` — lock only once a number has actually been captured, so every state in which the field is empty is a state the buyer can type into. A plain method rather than a computed: the template calls it inside the ko `attr` binding, so ko tracks both observables as dependencies of that binding, there is no subscription to dispose, and it exists on renderers the unit tests load without booting. MAJOR — the comments this change edited (and inherited) asserted that the address step is "the sole hand-typed route" for the organisation number. It is not a route at all: that field carries `two-company-id-hidden` from Plugin/Model/Checkout/LayoutProcessorPlugin.php and `display: none` from the stylesheet, unconditionally, with nothing anywhere removing the class. So no buyer can hand-type an organisation number ANYWHERE in the plugin, and an identifier-less company is a dead end refused by Model/Two.php::authorize(). Corrected in all three places rather than left as an escape hatch a future reader would go looking for. Whether that field should be unhidden when needsManualCompanyId() is a separate decision, not taken here. MINOR — the manual-entry helper's flush guard accepted any queued callback, and the injected setTimeout catches every timer the module sets (showErrorMessage's dismissal is the other). Tightened to exactly one. Two new cases pin the MAJOR directly: sole-trader mode entered with nothing captured, and a captured company abandoned by the mode switch. Both assert the field is NOT locked. Mutation-verified, six mutants, all killed: name readonly keyed on mode alone (2 — the bug above), always true (4), always false (2), static readonly dropped from the number (5), clearCompany not clearing companyId (2), a `name` attribute added to the number (1). npm run test:js: 21 suites, 255 tests, all passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pany-fields feat(TWO-25288): show the captured company name and number read-only in the tile
Both the shipping-step and payment-step "Search for company" controls rendered as a plain <div class="search_for_company"> with only a click handler — no tabindex, role, or keydown binding, so keyboard users could not reach or activate them at all. Add role="button", tabindex="0", and Enter/Space keydown handling to both address-autocomplete.js and gateway_method.js, matching the pattern already correct on Hyvä (companyName.phtml) and shipped on WooCommerce. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nt tile gateway_method-csp-js.phtml renders "Company number:" and "Company name:" via __() but these two exact strings (with the trailing colon) had no rows in nb_NO/nl_NL/sv_SE, so they silently fell back to English. Add both, reusing the wording already shipped for "Company Number"/"Company Name" elsewhere in the same dictionaries plus a trailing colon (matching the existing "Merchant ID:" pattern). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adversarial review (Vader) flagged that some assistive-tech/browser combinations forward a synthetic click in addition to the Enter keydown for a role="button" div (as opposed to a native <button>). Without a guard, a second call to activateSearchForCompany() would re-open a dropdown the buyer already opened. Early-return once the control is hidden. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fix(a11y): keyboard-reachable Search for company control
…bels fix(i18n): add missing Company number/name label translations
…country order Three live bugs Doug found testing Luma checkout this morning (#30.x.11): - Company search never got the CSS side of last night's WC/PS keyboard- reachability work: text-transform scoping, select2 vertical alignment, and a visible focus indicator on the "Search for company" control. PR #311 already fixed the JS keyboard behaviour (role/tabindex/Enter-Space); the matching CSS never landed alongside it. - The payment tile showed the captured company number as a second, separate read-only input. Canonical design across all four platforms (Doug's ruling): a plain text label immediately below the company-name field, right-aligned to it, visible only once a number has actually been captured. - Country rendered after street/company. checkout_index_index.xml already declares country_id's sortOrder as a static 50, which does win over Magento core's EAV default — but company/street carry no override of their own, so their position depends entirely on the store's Customer Address Attributes admin configuration, and this staging store's config pushes them below 50. LayoutProcessorPlugin now reads company's actual resolved sortOrder back out of the array (after core's own merge) and forces country immediately before it, mirroring PrestaShop's CustomerAddressFormatter::moveFieldBefore('id_country', 'company') — dynamic relative positioning instead of a static number. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d a11y caption, id-fragile CSS Three real defects from an independent adversarial round (Han/Vader/Yoda), all confirmed before fixing rather than taken on faith: - moveCountryBeforeCompany() anchored to company's sortOrder alone and rewrote country_id unconditionally on every render — the live bug was reported as "country after STREET", which the fix never checked, and an unconditional rewrite could push country past some other field even on a store that never had the bug. Now anchors to min(company, street) and only acts when country is not already ordered correctly; added an is_array guard on a malformed non-array country_id. - The payment-tile label removed the entire "Company Number" caption along with the old input, leaving a bare number with no indication of what it was — a real regression for sighted buyers and assistive tech. Restored as a static caption span alongside the number span (no `for`/id association, since there's no control left to associate with). - The address-step company-search dropdown CSS was scoped by `#select2-company-container`/`#select2-company-results`, assuming select2 derives those ids from the field's own `id="company"`. Verified live: the actual id is a Magento-generated uid (`id="S0FR5JB"`), not "company" — those selectors were dead CSS. Re-scoped the selection box via the adjacent-sibling selector already used elsewhere (id-independent), and dropdown rows via `dropdownCssClass: 'two-company-search-dropdown'` set on both select2 inits — select2's own supported hook for this, immune to its id-generation fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Retargets the five sole-trader suites from renderer delegates onto the component-owned flow, and re-pins the behaviours carried over from the previous attempt: tokens minted on availability rather than on click, a synchronous click-to-window.open() path, the 30-minute refresh skipped while a round trip is outstanding, autoselect=false on both re-signup routes, a blocked popup's retry reusing the blocked launch's options, abandonment deferring to the handshake, and the flight settling only once the identity write has landed. Two get stronger proofs than before. The absence of a passive buyer probe is pinned in source — exactly one `fetchBuyer` call site, inside the ACCEPTED branch — because it is a security property and a behavioural fixture alone would not catch its return. The phone write is proven against the real search model and a real form: applyAddress() handed a record carrying a phone number leaves telephone untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
Both were live before the component took over the mount, and both died because the new mount did not pass what the old call sites did. The company field showed nothing when empty: select2 renders its own display node, so the placeholder belongs to templateSelectionFallback and not to the input underneath. The msgid was still sitting in three catalogues with nothing referencing it. The "Search for company" link was built, hidden, and never shown again — manual entry is what used to reveal it. A buyer who has just typed into that field is looking at it rather than at the chips, so the in-field route back earns its place beside the chip that does the same thing. Test suites for company search retargeted onto the single mount. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
TWO-24867 was broken end to end: `_lastCountry` was written only inside onCountryChanged(), so the first switch after page load read as the first resolution and kept the company. A buyer could capture a GB organisation number, move to Spain, and place the order with it. The old renderer compared against an observable the quote seeded on load; the component had no counterpart until now. Remaining suites retargeted onto the single mount, and the control's docblock no longer argues for the two-mount design it used to have. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
Country invalidation had exactly one trigger and it was the wrong one. Only a `change` on an address-form select reached onCountryChanged(), and it read the country through a shipping-first document scan. A saved address in another country fired no event at all; a separate billing country resolved to the unchanged shipping one and was dropped. Both left a registry number standing under an address from a different registry, which is the failure TWO-24867 exists to prevent. The quote now drives it too, and the handler reads the select the buyer actually touched. A tile replaced under an unchanged selector left the control bound to a detached node. The node keeps its select2 data, so the control reports itself bound and the mount refresh skipped — the buyer got a plain input with no picker, permanently, because the selector never changes again. The guard now compares node identity, which is the only thing that can tell a replaced node from the one still on the page. A sole trader with no registry number of their own inherited the number of whatever company was captured before them, because the adoption write was not authoritative. Their name went out attached to another company's organisation number. The lifecycle suite claimed to cover the second of these and did not: its double reported itself bound independently of any node, so removing the guard entirely left the suite green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
The previous commit stopped a numberless sole trader inheriting the previous company's registry number, but left the mirror of it: the name was written only when truthy, whatever the caller asked for, so a sole trader with a number and no trading name of their own kept the earlier company's NAME against their own number. The same mismatch, the other field. An authoritative write now replaces both halves, empty ones included — which is what the callers already believed they were asking for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
…y-search refactor(TWO-25503): page-level company-capture component
Revert page-level company-capture component (TWO-25503)
`bind()` registered a fresh MutationObserver on every call and nothing ever disconnected one, so observers accumulated for the life of the page. Each observer's callback rebuilds select2, which mutates the DOM, which re-fires every observer registered so far — on a checkout that re-binds per totals change that compounds until the renderer stops responding. One observer per selector is enough: it already re-fires when the node is replaced, which is the case re-binding exists for. A caller asking to re-bind while one is watching gets the widget rebuilt immediately rather than waiting for the next mutation. The harness's `$.async` stub ran the callback once and never again, so stacking could not be expressed and no test could have caught this. It now records registrations and replays them the way a mutation does. Against the unfixed control the new suite fails five of eight, reporting one observer per bind and ten re-initialisations for a single mutation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
fix(TWO-25503): register one $.async observer per selector
Re-lands the component reverted in #368, on top of the observer-leak fix in #370, with the two changes that revert made necessary. `isMountLive()` is gone. It existed because the control reported itself bound off a node that could be detached, so a replaced tile left the buyer a dead input. With one `$.async` observer per selector guaranteed, that observer re-initialises on the replacement itself and the case is handled a layer down. The guard is what re-bound per totals change and stacked observers until the renderer stopped responding — it produced three of the five defects the previous attempt's review found, so it is removed rather than patched again. `onCountryChanged()` returns early without a brand config. The boot hook calls it on every address and totals change, on every checkout, and `start()` bails leaving no flow to tell and no registry to ask — so on a merchant with no Two-family method it made a registry call it had no business making and then dereferenced a null flow. Both are mutation-checked: without the guard the country change throws `Cannot read properties of null (reading 'forgetAdoptions')`, and the tile-replacement case now pins that re-binding is the control's job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
refactor(TWO-25503): page-level company-capture component
start() runs from the checkout sidebar, before the address form exists and before the quote carries a billing address, so the mount selector and country both resolved empty and nothing re-drove them. Watch for either candidate host via $.async and mount when it arrives. The harness now installs the $.async simulation over real jQuery too, so suites using real DOM do not have to know a module waits for a node.
TWO-25503 mount company search when its host node appears
Measured on staging: the company input lands 44ms before the country select, so the mount watcher asks for a country nothing can answer yet and sole-trader availability early-returns. The select then arrives already holding the default, so the `change` that would ask again never fires and the sole-trader chip stays hidden for every buyer who keeps their default country. Watch for the select itself and re-resolve through countryCode(), so an appearing billing form cannot impose its own default over the quote. The harness delivered every registration on fireAll(), letting one watcher stand in for another and hiding a missing one; it now delivers a node once, as an observer does.
…n-appear TWO-25503 resolve the country once a form can answer for it
…lude ABN-489: shadow .worktrees/ in docker install/test mounts
Replace the select2 picker with a self-owned panel anchored to the company field, holding the query input, the results and the mode chips as one control. select2 appends its dropdown to <body> and rewrites it on every open, so the chips could only ever be a sibling it drew over — which is what hid them exactly when the buyer opened the thing that offers them. Matches PrestaShop's TwoCompanySearch structure.
…p note The suites were written against select2's own DOM and its transport contract; they now assert the same guarantees against the panel and searchCompanies(). Also fixes a real bug the chip move introduced: the blocked-popup fallback note anchored itself after the chips, which now live inside the popover that entering sole-trader mode closes — so the buyer's only route forward rendered inside something they could not see. Anchored outside the popover instead.
- one $.async observer per selector EVER, not per selector transition, and the observer for an abandoned mount no longer drags the panel back to it - a re-render no longer wipes a manually typed company name or re-arms the field as a search trigger while manual entry owns it - leaving a mount unwraps it, so a second wrapper cannot clone the sole-trader fallback note - manual entry writes through to Knockout, so the quote stops carrying the company the buyer just abandoned - IME composition and paste reach the query field: seeding moved from keydown, which reports no printable key for either, to input - combobox semantics moved onto the field the keyboard actually reaches - closing aborts the search still on the wire - "the search is down" is styled apart from "your company is not here" again - per-panel result-row ids, so aria-activedescendant cannot resolve into a sibling panel - destroy() is final against observers it cannot disconnect
…wice The open/close/teardown/keyboard contract had no suite of its own — the neighbouring ones each reach through the panel at something else — so a dozen guarantees were passing vacuously. Each new case is pinned by a mutation that turns the suite red. The too-short hint now renders once: the query field's placeholder carries it for an untouched field, and the message line takes over only when the buyer is actually short of the threshold. e2e drives the popover rather than the deleted select2 widget.
- the too-short hint reads "Enter 3 or more characters", matching PrestaShop - the signup popup comes down when focus returns to the CHECKOUT page, and only then: focus leaving for a mail client to fetch an OTP leaves it up. Clicking the Sole trader chip is the one exception and raises it instead - manual entry renders the "Search for company" link again, below the field and right-aligned — without it manual entry is a dead end - clicking the company field lands the caret in the query box, rather than waiting for the first keystroke: the mousedown's own default action was taking focus straight back out - the query row shows in registered-company mode alone; a sole trader is enrolled through the hosted signup and a manual name is typed into the company field, so a query box answers for neither
The chips have to stay on screen while the buyer is in the hosted signup, or the only route back to the Sole trader chip is the company field — and clicking that reads as "focus is back on checkout", which takes the signup down a moment before they can reach the chip. The popup now comes down only when focus returns to checkout AND settles outside this control, so a click landing inside the popover is the buyer reaching for the signup rather than away from it.
…panel open() returned early when the panel was already up, which is exactly the state coming back from sole-trader mode leaves: the popover stays open behind the signup popup with the query row hidden, so the buyer landed on a search box nothing had put the caret in. It now re-syncs and re-focuses whether or not the panel was already open, with the sync ordered first so the query row is showing before the caret is asked to go there.
It was held open only so the Sole trader chip stayed reachable while the hosted signup was up. Once an identity is adopted the company is in the field and there is nothing left in the popover to act on.
TWO-25503: one unified company-capture popover
The Hyva extension carries its own parallel popover implementation, and that duplication is why the two checkouts keep drifting apart. It cannot reuse this one while the file depends on jQuery, mage/translate and a company-search module that reaches Magento_Checkout's Knockout quote. Pure refactor: same DOM, same order, same copy, same behaviour. Everything platform-shaped is now injected - `search` (the six-member transport, which Magento passes as the company-search module verbatim), `translate` and `observe` - and the UMD tail lets a host with no RequireJS load the same deployed file as a plain script. Test changes are delivery-mechanism only, no assertion touched: jQuery's .trigger() walks its own handler store and calls elem[type](), which does not exist for `input` or `mousedown`, so those call sites now dispatch the same event natively. The teardown suite counts document listeners by intercepting addEventListener rather than reading jQuery's namespace store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
jQuery's `.empty()` dropped the handler data for everything it removed; the vanilla port recorded only exact-target matches, so every chip rebuild - one per totals change on a re-rendering checkout - left an entry behind for the page's lifetime. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JheYXJZ6LdrKmr2XfPwcEm
TWO-25503: extract the company popover into a framework-free module
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.
Automated rolling sync PR opened by
.github/workflows/auto-pr.yml.Merges everything currently on
stagingintomain. Auto-updates as new commits land onstaging. Close manually if you need to skip a sync window.