Release: develop -> main - #194
Merged
Merged
Conversation
* feat(bank): add a receive-IBAN check to the bank client and hook The support form asks a customer which account their missing transfer went to. Today that field is 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 own receiving IBAN there and has to pick an account they never transferred to. The field is becoming free text, and the API gained PUT /bank/receive-iban so the frontend can tell the customer straight away whether what they typed is an IBAN DFX receives money on. This exposes that call: BankUrl.receiveIban, the ReceiveIbanStatus enum with its four wire values, the ReceiveIbanCheck response type, BankApi.checkReceiveIban on the client, and checkReceiveIban on the useBank hook. NotMatched deliberately does not claim the IBAN is unknown to DFX - personal IBANs of other accounts are never checked, and an account merge leaves personal IBANs behind on the former account, so a customer's own older IBAN can land there too. The comment on the enum says so, because the wording a frontend shows depends on it. Unlike the neighbouring list(), the new client method does not pass token: false. The endpoint distinguishes an authenticated check from LoginRequired, so the bearer token has to travel with the request whenever one exists. No existing API changes: getBanks, BankUrl.get and the Bank type are untouched. * test(core): pin the receive-IBAN call shape and document the status enum Review found two gaps in the previous commit. The client method had no test, although CONTRIBUTING asks for tests when functionality is added to core, and the pattern was there all along - auth-api.test.ts asserts the exact call shape of a client method, token flag included. The relevant invariant here is the absence of token: false, since dropping the bearer token would make LoginRequired unresolvable and silently disable the personal-IBAN half of the check. bank-api.test.ts now pins the URL, method and body, asserts that no token key is present, and keeps list() asserted with token: false so the deliberate asymmetry between the two is written down. The enum comments were line comments. tsc strips those from the declaration output, and a consumer only ever sees dist/ - so the warning that NotMatched makes no claim about DFX ownership would never have reached the developer writing the customer-facing wording, who is exactly who it was written for. All four members are JSDoc now and reach dist/definitions/bank.d.ts, verified on the built artifact rather than the source. DfxIban is phrased as ownership rather than active receiving, because most rows in the bank registry are decommissioned accounts and a present-tense claim would be untrue. * docs(core): correct the receive-IBAN test name and document both DfxIban branches Review found the test title stating the inverse of the invariant it guards. The API answers LoginRequired precisely when no token arrives, so naming the token-preserving test after that status said the opposite of what it pins, and mocking LoginRequired made the wrong reading look deliberate. Title and mock now agree with the assertion and its comment; the assertion itself is unchanged. The DfxIban documentation named only the collective-account branch. The other one - the requesting account's own personal deposit IBAN - is the case the endpoint exists for, and with the enum documented as the source of truth for the customer-facing wording, leaving the positive case unstated was the worst half to omit. * refactor(core): use a camelCase route for the receive-IBAN check The API repository documents camelCase for URL routes, and the closest sibling for this very concept - GET /v1/buy/personalIban - follows it, as do the two read-with-a-body precedents this endpoint was modelled on. The hyphenated form was an unjustified deviation. Doing it now is free: the endpoint is unreleased and has no consumer in production. Once this package ships the call, the same rename would break everyone using it. Only the route value changes. The BankUrl key, the enum values, the method and type names and the documentation all stay as they are; BankApi and the hook reference the constant symbolically and follow automatically. The test keeps a literal rather than the constant, so an unintended change to the route still fails a test. * docs(core): put the usage notes where a consumer sees them and fix three status claims Two independent reviews found that the enum documentation had been copied from the API repository without the corrections made there afterwards. Three statements were therefore still wrong here. LoginRequired described itself as "the caller is not logged in". That excludes the case it also covers: an authenticated token that carries no account-scoped identity. It now names the cause instead of the login state. DfxIban claimed that many bank rows are decommissioned. That quantifies over the contents of the production table, cannot be checked from this repository, and was never the point - what a consumer needs is that a match does not imply the account still accepts funds. NotMatched explained itself by naming a function in the API repository. A reader of this package cannot open it, and the statement can go stale there without anything failing here. The consequence a consumer actually needs survives without the mechanism: an IBAN the same customer used earlier can land in this state, so it must never be phrased as "this IBAN does not belong to DFX". The concurrency and throttling requirements lived only in the pull request description, which nobody sees who installs the package. They are now JSDoc on both public methods and verified present in the built declarations. For the React side that meant annotating the interface member rather than the function inside the hook: useBank emits as returning BankInterface, so a comment on the implementation would have produced no declaration at all and the note would have been invisible for the third time in this repository. * docs(core,react): keep the receive-IBAN docs to what a consumer can verify This documentation ships as .d.ts to consumers who have only the installed package in front of them. Anything it asserts about the service behind the call is unverifiable there and goes stale without a single test turning red. Removed accordingly: that the endpoint is throttled and what HTTP 429 means, that many DFX accounts are decommissioned, that an IBAN the same customer used earlier can end up unmatched, that another customer's personal IBAN is never checked, and which steps the check skips when no identity is present. What replaces them is either verifiable in this repository or phrased as guidance rather than fact: any non-2xx response is raised as an ApiException by the client in this package, and a rejected call means the IBAN could not be checked rather than that it was rejected. The wording rule for NotMatched stays - it is an instruction to the consumer, not a claim about the service. The test comment loses its assertion about how the service distinguishes two statuses and states what the assertion actually pins: unlike list(), this call does not opt out of the token. * fix(core,react): make the failure wording true for both packages and the test bite The previous wording said any non-2xx response is raised. That holds for the core client, but not for the hook: on HTTP 401 the shared api hook retries once with the current token when the stored token changed while the call was in flight, and a successful retry resolves normally. Both places now say what is true in either package - a failure never arrives as a status, and a rejection is an ApiException - and keep the instruction to read that as "could not check". The endpoint test asserted the sent IBAN against the same constant it passed in, so an implementation that always sent one hardcoded IBAN passed. It now runs over two distinct example IBANs, which fails that implementation on the second case. The second test's title claimed what the service can answer. It now states what the assertion pins: unlike list(), this call does not set token: false.
* fix(react): refresh the cached user after updateMail updateMail was a bare passthrough to the API hook: unlike updateUser, verifyMail and every other sibling in this context, it neither refreshed the cached user nor toggled isUserUpdating. The endpoint answers with an empty body, so the refreshed user has to be fetched explicitly. Consumers therefore kept a user whose mail was still null after a successful update, re-rendered the same "enter your email" step and re-submitted the same address — which the API answers with 403 TFA_REQUIRED once the account carries a mail. The missing isUserUpdating also left callers unable to disable their submit button, so the confirm action stayed clickable while a request was in flight. Both were lost in the switch from changeMail to updateMail in #473; changeMail delegated to updateUser and got the refresh and the flag for free. * fix(react): keep a failed user refresh from failing the mail update The refresh added in the previous commit rejected updateMail when the follow-up getUser() failed, even though the mail had already been changed server-side. Consumers would then present a successful change as an error, the user would re-submit the same address, and the account now carrying a mail would answer 403 TFA_REQUIRED — the exact loop this change set exists to remove, just on a narrower path. The refresh is now best-effort; the update result itself still propagates. * fix(react): keep the user identity stable when the mail update did not apply Review found that the refresh could drive a consumer into an endless update loop. PUT /v2/user/mail only persists the address when the account has none; once one exists it answers 202 and holds the new address until the emailed code is verified, so the refetched user still carries the OLD address. A consumer whose effect watches the user object and compares it against a target address — which is exactly what the services widget does for its mail parameter — then sees a new object identity with the condition still unmet, calls updateMail again, and repeats. Each pass sends another verification mail and invalidates the previous code, so the flow can never be completed. The refresh now keeps the previous object when the address did not move, so an update that changed nothing cannot re-trigger those effects. * fix(react): do not clear the cached user when the refresh yields nothing The identity guard adopted the refetched value whenever the address differed, which included the case where the refetch resolved undefined: an OK response whose body does not parse resolves that way, and the cached user was then wiped even though the mail update itself had succeeded. That contradicted the best-effort contract of this refresh, under which a failed refetch leaves the user untouched. The previous value is now kept unless a real user came back. * docs(react): condense the comment on the updateMail refresh Keep the two non-obvious constraints, best-effort and stable identity when the address did not move, without restating the chain below. * fix(react): keep a post-logout refresh from restoring the user The refresh callback treated a cleared state as "no previous value to compare against" and wrote the fetched user back. A getUser() response arriving after the session ended therefore put the signed-out user into the context again, without a valid token behind it. updateMail is the only mutation in this file that lacks the `if (!user) return;` guard its siblings carry, because it was a bare passthrough until this branch gave it a state write. --------- Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
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
2 participants
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.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist