feat(soletrader): write the buyer's registered address on adoption (TWO-25461) - #344
Conversation
…WO-25461) §1(a.3): the payment tile resolved its country through three shipping-biased feeds — a customer-data section written from the shipping form, whichever quote address fired last, and a DOM fallback that tries the shipping form first. The tile has no address fields of its own, so its country is the BILLING/invoice-role address's. searchCountryCode() now reads quote.billingAddress() first — the same source the autofill payload, getData() and placeOrderIntent() already use — and keeps the existing feeds as fallbacks. §5: fetchBuyer() has always returned the buyer's address beside their organisation number and company name, and all three call sites read the two identity fields and discarded the address, so a buyer who had just enrolled retyped an address the plugin was holding. The three sites now share one adoptSoleTraderBuyer(), which writes the address and then the identity. Deliberately NOT gated on either address-lookup switch: both are legitimately off wherever company search is not mounted in the address area, which is exactly where the sole-trader entry point lives. applyAddress() gains the field routing the write-back needs, shared with an ordinary company-search selection rather than special-cased: building/apartment take line 1 and move the street to line 2, a street alone takes line 1 and leaves line 2 untouched, no de-duplication between the lines, and a region goes to the region select (matched on option name or code), else a visible free-text region field, else onto the city with a comma. Line 2 and the region join the revert's field list so a country switch can still take back everything the write reaches. The country is never written.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No critical issues found. Frontend-only change (JS models/view + tests, no PHP/DI/schema touched); the deliberate bypass of the address-lookup switches on the sole-trader write-back is well-documented and pinned by a test that asserts the flags are never read on that path. Field routing, region matching, and revert-list symmetry are covered by thorough parameterized tests.
Ready for human approval ✅
…(TWO-25461) Adversarial review round 1 findings on the write-back. Scope. applyAddress()'s selectors were document-wide, which was tolerable while a buyer's own search pick was the only trigger and is not now that a passive prefetch fires it unattended: the payment step keeps the shipping form in the DOM and renders a billing form per payment method, all with the same field names, so one write reached every one of them — and a region id resolved against one form's option list was written into all of the others, where it is not an option at all. applyAddress() now takes an optional scope, and the tile passes the billing-role form (§1(a.3)) while the address-area picker keeps its existing behaviour. Replays. All three adoption paths can fire for the same buyer, and a replay overwrote a correction the buyer had since made. The address is now written once per identity, re-armed when sole-trader mode is left — which also now takes the address back, the missing half of a discard that already dropped the identity. The prefetch chain carries a generation and its own minted token, so a chain superseded by an email edit neither records its buyer nor acts on it. The popup's message listener is detached on dispose(), so a re-rendered checkout does not stack one live writer per render. A throw in the address write can no longer take the identity fill with it. Payload handling. A key the payload omits is no longer written as an empty string — that blanked fields the buyer had filled — while an explicit empty string still is. Conversely a field this module wrote for a previous selection is retracted rather than left standing, so an address cannot end up assembled from two companies. On a shop configured for one street line the locator and street are joined onto it instead of the street being written to a field that does not exist. `change` fires only once every value has landed, so no listener sees the city without its appended region. Which region control is in play is decided by whether the select carries real options rather than by CSS visibility, which also answers "hidden" for a form that is merely on the other step. The return-value plumbing is gone: it was always truthy, so nothing it reported was real. Tests grew the cases those defects needed, the spy jQuery double gained a real attribute store (an inert attr() getter made every marker assertion vacuous), and two brittle source-greps were replaced by behavioural cases — the switches are now read through throwing getters, which covers a gate added in any helper.
There was a problem hiding this comment.
No critical issues found on the current head (d91b9d3), which supersedes the commit the last review covered.
The second commit ("scope the address write and make it survive replays") addresses exactly the class of issues that would have been critical here: applyAddress()'s writes were document-wide and could land in the wrong address form (a region id resolved for one form's option list written into another form where it isn't an option), and a replayed adoption could overwrite a buyer's correction. Both are now fixed — the write is scoped to the billing-role form, gated on a once-per-identity guard, and the prefetch race is handled with a generation counter plus its own minted token so a superseded chain can't record or act on a stale buyer. The popup listener is detached on dispose(), closing a listener-stacking leak on re-rendered checkouts (Amasty/Fire Checkout).
Frontend-only change (JS models/view + tests, no PHP/DI/schema touched). The deliberate bypass of the two address-lookup switches on the sole-trader write-back is well-documented and pinned by a test that asserts the flags are never even read on that path. Field routing, region matching, and revert-list symmetry are covered by thorough parameterized tests against real jsdom markup.
Ready for human approval ✅
…e (TWO-25461) Adversarial review round 2, on the round-1 fixes themselves. The revert read the marker off a SET and cleared every element in it. jQuery answers .attr()/.val() from the first match only, so with the write now scoped to the billing form, the shipping form's unmarked field decided the answer for every field — the revert on leaving sole-trader mode, and the one on a country change, both silently did nothing on the payment step. It also meant one form's marker could authorise clearing a line the buyer had typed in another. Now checked and cleared per element. Scope selection: `#billing-new-address-form` went through jQuery's getElementById fast path and returned at most ONE element, so the loop meant to pick the selected payment method's form among several could never see them. Attribute selector instead. Role now beats visibility too — a hidden billing form is still the invoice address — with visibility only choosing between forms matching the same selector, and the shipping form kept as the last resort because core renders no billing form at all when billing and shipping are the same. The once-per-identity guard was armed before the write, so a throw or a checkout with no address form yet consumed the single chance and the address was never written. It is now recorded only after a write actually happens, keyed on the email where a buyer has no organisation number, and held at module scope: a per-instance record is re-armed by the very re-render it exists to survive. A completed signup now supersedes a prefetch still in flight — its pre-auth answer would disagree with the identity the buyer just authenticated and revert both the mode and the address. The tile's ordinary company-search selection is scoped the same way as the write-back; it had the same document-wide reach for the same reason. A region is written to the free-text control only when that control is enabled, since Magento disables the one the country does not use and drops its value on post. And the region is appended to the city only when this payload is writing the city too: appending to a city the buyer typed marked their own text as ours, which the next revert would then delete. Tests: the round-1 mutation survivors are all covered now. Supersession is exercised after the tokens are minted rather than before, so each guard is reached individually instead of the first one hiding the rest; the buyer lookup's token threading runs the real fetchBuyer; and the "nothing sees a half-written form" invariant is asserted by snapshotting every field at each change rather than by checking the event sequence, which could not tell the two apart. The comment that justified it with a now-impossible city case is corrected.
There was a problem hiding this comment.
No critical issues found on the current head (36a5862), which supersedes d91b9d3 covered by the last review.
The third commit ("per-element revert, role-first scope, retryable write") fixes exactly the class of bug that would have been critical: revertFields() was checking the autofill marker on the first element of a jQuery set, so once the write was scoped to the billing form, an unmarked shipping-form field decided the answer for the whole set — silently no-opping the revert on both a country switch and on leaving sole-trader mode, and letting one form's marker license clearing a field in another. It's now checked and cleared per element. billingRoleFormRoot()'s selector also moved from #id to an attribute selector — the bare #id form goes through jQuery's getElementById fast path and returns at most one node, silently defeating the loop meant to pick the right form among several billing forms (one per payment method) sharing that id. The once-per-identity write guard is now recorded only after writeSoleTraderAddress() actually returns true, so a throw or a checkout with no address form yet no longer permanently consumes the single write attempt.
Frontend-only change (JS models/view + tests, no PHP/DI/schema touched). The module-scope adoptedSoleTraderIds Set (replacing the old per-instance flag) is a deliberate fix for Amasty/Fire Checkout re-rendering the renderer instance — well-reasoned and consistent with registeredOrganisationMode() clearing it wholesale on mode exit. Field routing, region matching, and the revert/scope symmetry are all covered by thorough parameterized tests against real jsdom markup, including the per-element regression this commit fixes.
Ready for human approval ✅
…461) Adversarial review round 3. The billing-form selector matched nothing. Core renders the billing fieldset with `data-form="billing-new-address"` and no id at all — there is no `billing-new-address-form` in core on any version, and core's own tests select it by that attribute — so the scope resolution fell through to the shipping form every time. The sole-trader write-back landed in the hidden shipping form on the exact flow it exists for (guest, physical cart, billing different from shipping), and the tile's ordinary company-search autofill had been narrowed from document-wide to a form the buyer was not looking at. Verified against Magento_Checkout's billing-address/form.html and shipping-address/form.html in a 2.4.6 install; the id is kept after it for third-party checkouts that render one, and the test fixtures now use core's markup rather than markup the previous fixtures invented. The region rule tested `disabled`, which core never sets: its region element HIDES the control the country does not use (Magento_Ui region.js hideRegion → setVisible / toggleInput), so the check was inert and the comment justifying it was wrong on both halves. Back to a visibility test on the free-text control, with the select's option count — the thing the country actually drives — still deciding which control is in play. The once-per-identity record is now keyed per brand: a checkout offering two Two-family brands renders a tile and a billing form for each, and an adoption in one says nothing about the other. It is also re-armed on a country change, where the address step reverts the address it describes.
Self-review summary (TWO-25461 — Magento sole-trader country resolution + address write-back)This branch already carries three rounds of adversarial self-review, each fixing a real defect found in the round before it (see commit messages: selector matched nothing in core markup, revert cleared the wrong form via Checked and satisfied:
Verification this round:
No further findings. Clean round. (review by Claude) |
…ento-soletrader-writeback # Conflicts: # Test/Js/company-search-address-lookup.test.js # view/frontend/web/js/model/company-search.js
…ento-soletrader-writeback # Conflicts: # view/frontend/web/js/view/payment/method-renderer/gateway_method.js
…ento-soletrader-writeback
There was a problem hiding this comment.
No critical issues found on the current head (b83b583), which supersedes 36a5862 covered by the last bot review.
The intervening commit (9ee8ac7, "match the billing form core actually renders") fixes two real defects rather than adding new surface: ADDRESS_FORM_ROOT_SELECTORS now matches [data-form="billing-new-address"] — the attribute core's own billing-address/form.html actually renders — ahead of the #billing-new-address-form id that doesn't exist anywhere in core, so billingRoleFormRoot() would previously have fallen through to the shipping form on stock Luma/core checkouts. And the free-text region control's applicability check moved from .prop('disabled') to .is(':visible'), matching how Magento_Ui's region.js actually hides the unused region control (visibility, not the disabled attribute) — the old check would have silently accepted a hidden field as "in play" and written a region value the buyer could never see. Both are pinned by the updated company-search-address-field-routing.test.js, which also now models the real data-form fieldset markup instead of a synthetic id.
Everything else in this diff (scoped write-back, per-element revert, once-per-identity guard now keyed per-brand, prefetch generation guard, popup listener detach on dispose()) is unchanged from what the last three reviews already covered and approved. Frontend-only change (JS models/view + tests, no PHP/DI/schema touched).
No unresolved review threads on this PR.
Ready for human approval ✅
Self-review round 4 — confirmed clean, merged staging's PHPStan fixPicking up the interrupted round 4 above: CI had gone red on this PR after that comment was posted, but the failure ( Merged current
No new findings. Clean round. (review by Claude) |
1 participant
Closes the two remaining TWO-25461 gaps from the sole-trader porting guide: §1(a.3) country resolution and §5 address write-back.
§1(a.3) — the tile resolves the billing-role country
The payment tile has no address fields of its own, so the country it searches and gates sole-trader availability on is whichever address plays the billing/invoice role. It was resolving through three shipping-biased feeds instead: a customer-data section written only from the shipping form,
updateAddress()off whichever quote address fired last, and a DOM fallback whose first selector is#shipping-new-address-form. With a shipping country differing from the billing one, any of them can answer wrong.searchCountryCode()now consultsquote.billingAddress()first — the same source the autofill payload,getData()andplaceOrderIntent()already read, so this is one resolution reused rather than a fourth mirror — and keeps every existing feed as a fallback for the states where the quote has no billing address yet (which is what the Fire Checkout fix in TWO-25326 added them for).Country only feeds an availability-gate lookup and the search URL; there is no security dimension to it, as already established for the sibling plugins.
§5 — the write-back actually writes the address
fetchBuyer()(/autofill/v1/buyer/current) has always answered with the buyer's address beside their organisation number and company name. All three call sites read the two identity fields and discarded the address, so a buyer who had just enrolled was asked to retype an address the plugin was holding. Those sites — the passive prefetch, the chip click against an already-prefetched buyer, and the popup's post-signupACCEPTEDmessage — now share oneadoptSoleTraderBuyer().applyAddress()carries no gate of its own (it lives one level up inlookupCompanyAddress()), so calling it directly is the bypass.fillCompanyData()is the order-intent trigger andplaceOrderIntent()readsquote.billingAddress(). No new intent trigger is introduced — the trigger and its_orderIntentInFlightForguard are untouched, so a replayed adoption starts no second intent.fillCompanyData()refuses a nameless pair, and the address write is deliberately outside that refusal.Field routing, shared not special-cased
applyAddress()wrote city, postcode and street line 1 only, offstreet_addressalone. It now routes per §2.6, for a company-detail response and a buyer record alike:building/apartmentpresent → line 1 (joined most-specific-first), street moves to line 2;region→ the region<select>when an option matches on name or code, else a visible free-text region field, else appended to the city with a comma;Line 2 and both region controls join
revertAutofilledAddress()'s field list, so a country switch can still take back everything the write can reach.changenow fires per field rather than once for a combined selector, from the same statement as the value and its provenance marker.Tests
npx jest --config Test/Js/jest.config.js→ 34 suites, 465 tests, all passing.Two new suites, parameterised:
company-search-address-field-routing.test.js(routing table + region table + revert coverage, against real jsdom markup so the option-label match is genuinely exercised) andgateway-method-sole-trader-address-writeback.test.js(all three adoption paths, the switch-off cases, awkward buyer records, re-entrancy).company-search-tile-country-sourcing.test.jsgains a billing-vs-shipping table for §1(a.3).