Improve shipment error handling - #97252
Conversation
SetPersonalDetailsAndShipExpensifyCards returns 200 and still saves the personal
details even when a card fails to ship, reporting per-card failures in a new
cardShipmentErrors response array ({cardID, error}).
- Register the command as a side-effect request and type cardShipmentErrors on Response.
- updatePersonalDetailsAndShipExpensifyCards now returns a promise: it writes each
failed card's reason onto that card (cardList[cardID].errors) so it shows an RBR in
the wallet, and rejects with the reason only when the card the user is completing
details for is among the failures — other cards' failures don't block the flow.
- MissingPersonalDetailsMagicCodePage shows the same "Unable to ship Expensify Card:
<reason>" message on the magic-code modal when the target card fails.
- Add the cardPage.shipCardError copy (en.ts); locale files are placeholders that the
generateTranslations bot refines on the PR.
When a card's shipment failure has no user-actionable reason, the backend returns the generic "Card could not be shipped". Treat that (and an empty reason) as no reason so both the magic-code modal and the wallet RBR show a bare "Unable to ship Expensify Card" instead of a redundant tail. - Add CONST.EXPENSIFY_CARD.GENERIC_SHIPMENT_ERROR and normalize it to an empty reason in updatePersonalDetailsAndShipExpensifyCards (RBR merge and the target-card rejection). - Distinguish request failures (isRequestError) from card-shipment failures so only the former fall back to the generic cardPage.unexpectedError copy. - cardPage.shipCardError now omits the ": <reason>" when no reason is given (all locales).
The dismissal keyed off areAllExpensifyCardsShipped, so a different Expensify card that failed to ship kept the modal open even after the card this flow is for had shipped, leaving the user stuck. Key the dismissal off the target card's state instead, so the modal closes once that card ships regardless of other cards' outcomes; a failure on the target card still keeps the modal open to show its error.
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
CardShipmentError is only referenced within Response.ts (by the cardShipmentErrors field), so exporting it left an unused export that knip flagged. Keep it as a local type declaration.
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a3565099a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
makeRequestWithSideEffects resolves API-level failures as responses rather than throwing, so a non-success jsonCode (e.g. an incorrect magic code) reached the shipment-error handler, found no cardShipmentErrors, and resolved as if the submission succeeded — the validate-code page never surfaced the error. Check response.jsonCode before processing cardShipmentErrors: reject incorrect magic codes with the incorrect-security-code message and any other non-success response as a generic request error, matching the sibling reveal-card flow.
…ession - Clear the target card's errors in optimisticData so a successful retry no longer keeps a stale "Unable to ship Expensify Card" RBR on the wallet/workspace card row; the .then re-sets it only if the card fails again (addresses P2 review). - Add a justification above the @typescript-eslint/no-deprecated suppression explaining that the shipment error is a pre-formatted message stored verbatim, not a translation key (CONSISTENCY-5).
|
Looks like you have merge conflicts |
| ...buildSetPersonalDetailsAndShipExpensifyCardsParams(values, countryCode), | ||
| validateCode, | ||
| }; | ||
| function updatePersonalDetailsAndShipExpensifyCards( |
There was a problem hiding this comment.
@heekinho I wonder why we need to handle the error this way. The error information isn't confidential data, so I think it's fine to save it to Onyx and have the component read it from Onyx to display
cc @rafecolton
There was a problem hiding this comment.
My suggestion is that the BE should return the error message directly in the Onyx object. On the FE side, we'd no longer need to extract the error message from the JSON code like this
if (response?.jsonCode !== CONST.JSON_CODE.SUCCESS) {
if (response?.jsonCode === CONST.JSON_CODE.INCORRECT_VALIDATE_CODE) {
// eslint-disable-next-line prefer-promise-reject-errors
reject({isIncorrectMagicCode: true});
return;
}
// eslint-disable-next-line prefer-promise-reject-errors
reject({isRequestError: true});
return;
}
We'd only need to get it from Onyx to use. I believe this approach would make more sense
There was a problem hiding this comment.
Great call, @DylanDylann! Adjusted.
Please lemme know if it's better and further adjustments needed 😄
The backend now persists each failed card's shipment error to Onyx as an RBR on the card (cardList[cardID].errors) and returns an incorrect-magic-code failure as a normal command failure. Read both from Onyx instead of inspecting the response inline: - updatePersonalDetailsAndShipExpensifyCards goes back to API.write. The incorrect-magic-code error is written to the validate-code action's errorFields via failureData, and the optimistic data clears any stale RBR and magic-code error from a previous attempt. - MissingPersonalDetailsValidateCodePage reads the target card's shipment RBR from Onyx and surfaces it inline, and clears it via clearCardListErrors. The promise .catch, the local ship-error state, and the manual response parsing are gone. Remove the client-side scaffolding this makes dead: the SetPersonalDetailsAndShipExpensifyCards side-effect command registration, the cardShipmentErrors response types, the GENERIC_SHIPMENT_ERROR constant, and the shipCardError translations (the backend now builds that message).
3 participants
Explanation of Change
Adjusts in conjunction with backend changes to surface known errors instead of letting the user in the dark.
This PR specifically:
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/664259
PROPOSAL:
Tests
Scenario 1
Scenario 2
This.card.ships.and.some.cards.get.RBR.mov
This.card.does.not.ship.but.others.do.ship.mov
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari