feat(support): let customers type the receiver IBAN instead of picking one - #1191
Conversation
…g one When a customer reports that a transfer never arrived, the form asks which account it went to. That field was a required dropdown filled from GET /bank, which lists only the collective accounts. A customer who deposits through a personal IBAN can never find their actual receiver in that list, and because the field is required they had to pick an account they never transferred to. The wrong IBAN then sits in the ticket and reads like a statement from the customer - in exactly the situation where someone needs help. There are more than three thousand personal IBANs in use, so this is the normal case rather than an edge case. The dropdown becomes a free-text field, and the API checks what was typed. The hint below the field says whether the IBAN was recognised, whether it does not look like an IBAN at all, whether it could not be attributed, or that a login is needed before personal IBANs can be checked. The check is advisory and never blocks submission. Any failure, rate limiting included, shows a neutral "could not check right now" and leaves the form submittable - a support ticket must never fail because an IBAN was not recognised. The field stays required in the sense that it may not be empty. Three things the client library does not handle, so the screen does: the input is debounced by 500 ms, the value used elsewhere in the codebase, rather than checked on every keystroke; a request counter discards answers that arrive after a newer one, so a slow answer for a prefix cannot overwrite the answer for the complete IBAN; and the counter is also bumped when the field is cleared, so an in-flight request cannot bring a hint back. The wording for a non-attributable IBAN deliberately avoids claiming that it is not a DFX IBAN. The status does not carry that meaning: another customer's personal IBAN is never checked, and after an account merge personal IBANs stay with the former account, so a customer's own older IBAN can land there too. getBanks is no longer used on this screen and is removed along with its state and effect. The sender IBAN field, which comes from the customer's own bank accounts, is untouched.
🤖 PR Review Bot❌ TypeScript: 3 errorsThis is an automated review. Please address the issues above. |
…the hint Review found three things the first version got wrong. The check fired on every typing pause, and the API answers incomplete input with InvalidIban by design. A customer copying an IBAN off a bank statement and glancing up mid-way was told their IBAN looks invalid, with the form jumping as the hint appeared and vanished. For the customers this change exists for that is the normal way of filling the field, not an edge case, and every pause also spent a request from a limit shared by everyone behind the same network. The call now waits until the normalized value reaches fifteen characters, the shortest IBAN in use. A real typo is still caught, because the customer stops at twenty-one or twenty-two characters and the checksum fails there. The hint for an unattributable IBAN asked the customer to check whether it is correct. The status does not support that: another customer's personal IBAN is never checked, and after an account merge personal IBANs stay with the former account. So the one customer this feature was built for - the one whose personal IBAN cannot be matched - was being told to correct an IBAN that was already right, and replacing it with a guess would restore exactly the wrong-IBAN-in-the-ticket problem this change removes. The hint now says only that we could not assign it, and that the request can be submitted anyway. A blank-only entry passed the required rule, sanitized to an empty string server-side and produced a ticket with no receiver IBAN at all - something the dropdown made impossible. An emptiness check now rejects it. It is a check on presence, never on the IBAN itself, so an unrecognized value is still accepted.
…field Two independent reviews showed the length threshold does not do what it was added for. Fifteen characters is the shortest IBAN in existence and a Swiss one needs twenty-one, so a customer pausing at sixteen still triggered a request and still read "this does not look like a valid IBAN" while typing a correct one. The threshold moved the problem, it did not solve it, and a country-length table in the frontend would only add a second authority on IBAN validity next to the API. What actually matters is which hint can do harm, and that is only the negative one. A positive confirmation can occur only for a complete IBAN that matched, so it still appears immediately; every other verdict now waits until the field loses focus. That also keeps the form from jumping while someone is still typing. The threshold itself was measuring the wrong string: it stripped only ASCII spaces while the API strips every non-alphanumeric character, so a dash-grouped entry counted separators towards the length and was checked too early. Normalization now matches the API exactly and is the single source for the threshold, the request and the equality guard. The debounce sits on the normalized value, so retyping a space or a dash costs no request. The check could also start for a debounced value that was no longer in the field, if both updates landed in one commit - the response then wrote a hint under the new or empty value. Rather than reason about batching, the effect now refuses to run unless the debounced value is still the current one. And the unmatched wording is more careful again: the status does not establish that DFX does not own the IBAN, since another customer's personal IBAN is never checked and neither is one left behind by an account merge. The text now says we could not recognize it with the information available to us.
|
This diff went through four review passes before two independent reviews came back with no findings. What they caught: The length threshold did not do what it was added for. Fifteen characters is the shortest IBAN in existence and a Swiss one needs twenty-one, so a customer pausing at sixteen still triggered a check and still read "this does not look like a valid IBAN" while typing a correct one — the very behaviour the threshold was meant to prevent. It moved the problem rather than solving it. A country-length table in the frontend was considered and rejected: it would put a second authority on IBAN validity next to the API. Instead, display is now gated on focus. Only the positive confirmation appears while the field is focused, because it can occur only for a complete IBAN that matched; every other verdict waits until the field is left. The threshold was measuring the wrong string. It stripped only ASCII spaces while the API strips every non-alphanumeric character, so a dash-grouped entry counted its separators towards the length and was checked far too early. Normalization now matches the API exactly and is the single source for the threshold, the request and the equality guard. A check could start for a value no longer in the field. If the debounce timer queued a value while the field changed, both updates could land in one commit and the response would write a hint under the new or empty value. Rather than reason about batching, the effect now refuses to run unless the debounced value is still the current one. The unmatched wording overstated twice. It first asked the customer to check whether the IBAN was correct — which imputes a mistake in exactly the cases the API documents as not mistakes: another customer's personal IBAN is never checked, and neither is one left behind by an account merge. Telling the customer this change was built for to re-check a correct IBAN would push them back to guessing. The text now says only that we could not recognize it with the information available to us, and that the request can be submitted anyway. Both reviews independently verified the assurance that matters most here: the check never enters form validation, never disables submission, and the ticket endpoint accepts any string. An unrecognized IBAN cannot stop a customer from reporting a problem.
This PR stays in draft, and its CI will fail until the library release lands. See the ordering section above — the lockfile has to be updated in this branch after the release, which is a required step and not an automatic one. |
The advisory receiver IBAN check had no test at all. These tests pin the two properties the feature depends on: the check never blocks, delays or alters a support request, and a hint never belongs to a value that has left the field. The component tests render the screen and cover field visibility per reason, the length threshold at both its boundaries, the normalization the request shares with the API, debouncing, the fact that a formatting-only edit costs no request, the reset that clears hint, spinner and unavailable state on every edit, discarding a late answer for a superseded value whether it resolves or rejects, the focus gating that holds back every non-positive verdict until the field is left, the four hint texts and their colour, the unavailable path for a failed or rate-limited check, the spinner, the requirement that the field be non-empty, and submits that actually go through while a check is in flight and after one has failed, proving the raw text the customer typed is what gets sent. Each behaviour was checked by breaking the screen on purpose and confirming the named test turns red. One constant cannot be pinned this way: setting ReceiverIbanCheckDelay to 0 changes nothing, because useDebounce computes delay || 500 and a falsy 0 becomes 500 again. The language files get their own test for the five new keys, pinning the exact text per language so a swapped or hollowed-out translation fails. It is scoped to those keys: the three files already differ on unrelated entries, so full parity would fail on gaps this change did not create. The Playwright spec captures the six visible states of the field for review. Its baselines are not part of this commit: they are platform-bound, and CONTRIBUTING.md asks for them to be generated on the same platform as the existing ones. @dfx.swiss/react and @dfx.swiss/react-components are mocked rather than loaded, because their published ESM does not load under this repo's test command. The mocks reproduce the rule merging and validation shapes the screen relies on and wire the input through react-hook-form, so focus, validation and the watched value behave as they do in the app. Form deliberately renders no submit handler of its own, matching the real component, so the button's own handler stays the only path a submit test can take.
…check CI installs from the lockfile, so the range alone does not move the build; the pin has to ship with the change. This is the prerelease line, taken knowingly: the stable release of the same version is not out yet, and the screen cannot compile without checkReceiveIban. The range is ^1.7.0-beta.0, which the stable 1.7.0 also satisfies, so a later install moves off the prerelease without another range edit. @dfx.swiss/core follows transitively to 0.6.0-beta.0; it is not a direct dependency here. Verified against the published package rather than a local build: build:dev, the stricter widget:dev, lint, and the full test suite at 465 tests.
The hint was cleared in an effect on the normalized value, and effects run after the render. On the first render after an edit the previous verdict was therefore still in state and was shown under the new value - most visibly the positive confirmation, which is deliberately shown while the field still has focus. The result now carries the IBAN it was computed for, and the hint is derived only when that value still matches the field. Clearing it in the effect becomes unnecessary and is gone, along with the same clearing in the check effect's guard branch; the reset effect keeps only the request counter and the spinner. Two tests did not hold what their names promised, both shown by mutating the screen: Validations.Iban was mocked as always passing, so adding real IBAN validation to the receiver field left the suite green - while in the app it would reject a free-text entry and block a support request, which is the one thing this screen must never do. The mock now fails, and a test keeps a complete form submittable with a non-IBAN value in the field. The translate mock returns its key, so removing the translation call around the hint also left the suite green, and every non-English customer would have seen English text. A test now pins that the hint goes through translate under the screens/support namespace. The visual spec gained the LoginRequired state, which the docblock promised but did not record, and now forces lang=en: it asserts English text while authenticating as a real account, and the language of that account would otherwise decide whether the selectors match.
The button mock dropped isLoading, while the real component disables itself on disabled || isLoading. Putting the submit button into its loading state while a check is in flight would therefore have blocked the very thing this screen promises never to block, and the suite would have stayed green. The mock now mirrors the real component, and the two in-flight tests fail against that change. Four comments claimed things a reader cannot check here: how the service answers incomplete input and how its rate limit is shared, how the input component handles focus handlers internally, that the tested translation keys contain colons - they contain periods - and that the visual baselines are deterministic and free of production data, when only the check endpoint is intercepted and everything else comes from whatever stack the run points at. Each now says what holds in this repository, and the reasoning they carried is kept where it was about this code.
…gative one Submission was proven unaffected for an unrecognized IBAN, for a check still in flight and for one that failed - but not for the positive verdict. Blocking the button or normalizing the submitted value specifically when the IBAN was recognized therefore left the suite green, and the customer who gets the friendliest answer is the one whose request would have been swallowed. The submission test now runs over all four statuses, so any verdict-specific special case fails it.
…g comments The non-IBAN test checked that the field carries no error and the button stays enabled, but it never pressed the button. An onSubmit that quietly returned for a receiver value below the check threshold therefore passed, while in the app the customer's request would simply never have been created. The test now clicks and asserts the request carries the raw value. Two comments were wrong rather than merely unverifiable. One said the hints appear only after the field is left - the positive confirmation deliberately appears immediately, which is the whole reason focus gating is selective. The other justified the missing autocomplete by saying the receiver IBAN is not the customer's own; with a personal deposit IBAN it is exactly theirs, which is the case this screen was rewritten for. The remaining reason, that the sender field above already claims the IBAN autofill, is true and stays. A third comment pointed into node_modules for its evidence and now describes what the mock does and why.
…imiting The guarantee that a failed check never becomes a form error was pinned against rate limiting only. Reporting every other failure through the screen's error banner therefore passed, and a customer whose check answered with a server error would have seen a form error under an advisory field. The failure case now runs over two kinds of rejection, and each of them also submits and asserts the raw value, so the two former single-status tests are covered by one parameterized case rather than duplicated.
|
Six review passes were needed before both independent reviews came back clean. The screen itself carried one real defect, and most of the rest were tests that did not hold what their names promised. The defect. The check result was cleared in an effect on the normalized value, and effects run after the render. On the first render after an edit the previous verdict was therefore still in state and was shown under the new value — most visibly the positive confirmation, which is deliberately shown while the field still has focus. The result now carries the IBAN it was computed for, and a hint is derived only while the field still holds that value. Clearing it in the effect became unnecessary and is gone, together with the duplicate clearing in the check effect's guard branch. Mocks that were plausible but not faithful. Three of them, each found by mutating the screen and watching the suite stay green:
Guarantees that were pinned for one case only. Submission was proven unaffected for an unrecognized IBAN, but not for the positive verdict, not for a non-IBAN entry, and not for a failure other than rate limiting. Each of those is now covered, so a verdict-specific or error-specific special case fails a test. The translation wiring is pinned too: removing the Two reported points were rejected with reasons rather than applied. Retrying anonymously on HTTP 401 cannot occur here, because the endpoint's guard treats an unusable token as an anonymous request rather than rejecting it. And reproducing the button's The PR description was brought to the end state along the way: the ordering note now reflects that the API and library changes have landed, and the dependency pin is described for what it is. |
The spec drove /support/issue with a freshly registered account. The screen sits behind useKycLevelGuard, so the app redirected to contact data and the Receiver IBAN field never rendered: all seven tests timed out waiting for a field that was not there. The route setup now pins kyc.level in the GET /v2/user response, next to the receiveIban mock the tests already install. Nothing changes on the server, so the account other specs share keeps the state they rely on, and the screenshots no longer depend on whatever level the local account happens to have.
The spec captures the field in seven states: empty, check in flight, recognized, not matched, invalid, check unavailable and login required. Its baselines were missing, so the spec had nothing to compare against. Taken on macOS against a local API stack, as CONTRIBUTING requires, and confirmed by a second run without --update-snapshots. No other baseline is touched.
|
The seven missing baselines for The spec could not work as committed. How the images were produced and checked
One thing worth knowing. The Date of the transaction field renders Playwright does not run in CI by design (CONTRIBUTING), so these files cannot affect the checks; CI is green. |
1 participant
Why
When a customer reports that a transfer never arrived, the support form asks which account it went to. For
TRANSACTION_MISSINGthat field was a required dropdown fed byGET /bank, which lists only the collective accounts.A customer who deposits through a personal IBAN could never find their real receiver there — and because the field is required, they had to pick an account that is not theirs. That wrong value lands in the ticket and reads to support like a statement from the customer. There are over 3,000 personal IBANs in production, so this is the normal case, not the edge case.
What
The field is now free text, backed by
checkReceiveIbanfromuseBank().The check is advisory and never blocks submission — a support ticket must never fail on an unrecognized IBAN. The field stays required only in the sense of "must not be empty"; a blank-only entry is rejected as empty, which the dropdown made impossible before.
The hints, all phrased without blaming the customer:
NotMatchedclaims no more than the status supports. Per the library's JSDoc it is not a statement that DFX does not own the IBAN: another customer's personal IBAN is never checked, and one left behind by an account merge is not either. So the text says we could not recognize it with the information available to us — not that it could not be assigned, and certainly not that the customer mistyped. Telling the very customer this change was built for to re-check an IBAN that is already correct would push them back to guessing, the exact failure this removes.Timing of the check, and of the hints. The call is debounced by 500 ms and only fires once the value reaches 15 characters after normalization — the endpoint is rate-limited and the limit is shared by everyone behind the same network, and the API answers incomplete input with
InvalidIbanby design. Normalization strips every non-alphanumeric character, matching what the API does before it looks an IBAN up, so a grouped or dash-separated entry is measured and sent exactly as the server will read it. The debounce sits on that normalized value, so retyping a space costs no request.Because 15 characters is the shortest IBAN in existence and a Swiss one needs 21, the threshold proves length but not completeness. Display is therefore gated on focus: while the field is focused only the positive confirmation appears — it can occur only for a complete IBAN that matched — and every other verdict waits until the field is left. That keeps a wrong "this does not look like a valid IBAN" off the screen mid-typing and stops the form from jumping. Submitting by mouse still shows the hint, because focus leaves the input on
mousedown, before the click is dispatched; submitting with Enter from inside the field shows none, which is acceptable for a check that blocks nothing.Stale responses are discarded through a request counter, and the check refuses to run at all when the debounced value is no longer the value in the field, so no hint can appear under a value it was not computed for.
getBanks()and its state and effect are gone from this screen —bankshad no other use here, and the error banner that used to appear when the bank list failed to load goes with it. The sender IBAN, date and transaction fields are untouched.Dependency — the prerelease is the intended pin
DFXswiss/api#4391 and DFXswiss/packages#192 are both merged. The client call arrives here
through
@dfx.swiss/react, pinned to1.7.0-beta.0;@dfx.swiss/corefollows transitively to0.6.0-beta.0and is not a direct dependency here.The prerelease is the deliberate choice for this change, not a placeholder waiting on something
else: no release of the library to
mainis planned for it. Nothing about this PR is blocked on astable version, and reviewers should not hold it for one. Should a stable
1.7.0be publishedlater, the range
^1.7.0-beta.0accepts it, so an ordinary install picks it up without a secondrange edit.
CI installs from the lockfile rather than the range, which is why the pin has to ship as part of
this change rather than only as a range bump.
What was verified locally, and how
These were run against the published package, not a local build of it.
npm run lintnpm run build:dev(the pipeline's real TS compile)npm run widget:dev(the stricter build)CI=true npm test -- --watchAll=falseprettier --checkon every changed and added file@dfx.swiss/react1.7.0-beta.0,@dfx.swiss/core0.6.0-beta.0 transitivelyeslinton the added test filesNote for reviewers:
npm run lintglobs{src,apps,libs,test}/**/*.tsand therefore does not cover.tsx— the only changed code file. It was linted explicitly and is clean, but a green lint job alone says nothing about this change.Tests
The check had no coverage at all. Two things are pinned, because everything else about
this feature is cosmetic next to them: the check never blocks, delays or alters a
support request, and a hint never belongs to a value that has left the field.
src/__tests__/support-issue-receiver-iban.test.tsxrenders the screen and covers:TRANSACTION_MISSINGTRANSACTION_MISSINGclears the state and starts no further checkEach of these was checked by breaking the screen on purpose and confirming the named
test turns red — the guards, the threshold, the gating, the normalization, the
required rule, the failure path and the submit handler were each removed in turn.
Two things resist a single-line mutation, and neither is a gap in the tests.
ReceiverIbanCheckDelayset to0changes nothing, becauseuseDebouncecomputesdelay || 500and a falsy0becomes 500 again. And the reset effect shares itsthree
setStatecalls with the check effect's own guard branch, which runs on thesame dependency; removing a reset from one place leaves the other to compensate, so
only removing it from both turns the test red.
src/__tests__/support-issue-translations.test.tspins the five new keys in de, fr andit. It is deliberately scoped to those keys: the three files already differ on unrelated
entries, so a full parity test would fail on other people's gaps.
@dfx.swiss/reactand@dfx.swiss/react-componentsare mocked rather than loaded —their published ESM does not load under this repo's test command. The mocks reproduce
the rule merging and validation shapes the screen relies on and wire the input through
react-hook-form, so focus, validation and the watched value behave as in the app.
Visual regression baselines
e2e/support-issue-receiver-iban.spec.tscaptures the six visible states of the field:empty, checking, recognized, not recognized, invalid, and check unavailable. It follows
the pattern of
e2e/realunit-support.spec.ts— real customer auth, synthetic fixtures,and
page.routeintercepting onlyPUT /v1/bank/receiveIbanwhile everything else goesto the API. Type and reason come from the screen's own
issue-typeandreasonURLparameters, so no dropdown interaction is needed.
The baseline images are not in this commit.
playwright.config.tsputs the platformin the snapshot path and all existing baselines are
*-chromium-darwin.png;CONTRIBUTING.mdasks for new ones to be generated on the same platform as the existingones. Generating them here would have produced
*-chromium-linux.pngwith different fontrendering — different files sitting next to the existing set, which is worse than none.
To add them, on macOS with the local API stack up and a
@dfx.swiss/reactthat hascheckReceiveIbaninstalled:This writes all six baselines in one pass. Per
CONTRIBUTING.mdthese tests are areview aid and do not run in CI, so their absence blocks nothing.