Fix non-usd flow api error#84904
Conversation
|
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!
|
melvin-bot
bot
requested review from
thesahindia
and removed request for
a team
|
@thesahindia 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] |
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| isLoading?: boolean; | ||
|
|
||
| /** Optional content rendered below the radio buttons */ | ||
| footerContent?: React.ReactNode; |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
Adding a named ReactNode optional prop (footerContent) whose sole purpose is to render UI in a specific position within the component is a configuration pattern (Case 2). The prop is rendered directly as {footerContent} inside FormProvider and serves no other purpose. This couples the YesNoStep component to consumer-specific UI needs and requires modifying its interface every time a new consumer needs different footer content.
Instead, consider using composition via children or a compound component pattern. For example, YesNoStep could accept children that render after the radio buttons, or the error message UI in SignerInfo could be composed alongside YesNoStep rather than injected into it:
// Option 1: Use children
<YesNoStep title={...} description={...} defaultValue={...} onSelectedValue={...}>
{showNoPolicyError && (
<View style={[styles.flex1, styles.justifyContentEnd]}>
<DotIndicatorMessage ... />
</View>
)}
</YesNoStep>Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`); | ||
| const currency = policy?.outputCurrency ?? reimbursementAccountDraft?.currency ?? ''; | ||
| const country = reimbursementAccountDraft?.[INPUT_IDS.ADDITIONAL_DATA.COUNTRY] ?? reimbursementAccount?.achData?.[INPUT_IDS.ADDITIONAL_DATA.COUNTRY] ?? ''; | ||
| const currency = policy?.outputCurrency ?? reimbursementAccountDraft?.currency ?? CONST.BBA_COUNTRY_CURRENCY_MAP[country] ?? ''; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The currency resolution pattern policy?.outputCurrency ?? reimbursementAccountDraft?.currency ?? CONST.BBA_COUNTRY_CURRENCY_MAP[country] ?? '' (along with the country resolution pattern just above it) is duplicated across 11 files in this PR: BankInfo.tsx, BusinessInfo.tsx, AverageReimbursement.tsx, PaymentVolume.tsx, Website.tsx, EnterEmail.tsx, SignerInfo/index.tsx, SignerInfo/Confirmation.tsx, SignerInfo/UploadDocuments.tsx, BeneficialOwnerInfo/Confirmation.tsx, and BeneficialOwnerInfo/Documents.tsx.
Extract this into a shared utility function or custom hook to follow the DRY principle:
// e.g., in src/pages/ReimbursementAccount/NonUSD/utils/getCurrencyForNonUSDBankAccount.ts
function getCurrencyForNonUSDBankAccount(
policy: OnyxEntry<Policy>,
reimbursementAccountDraft: OnyxEntry<ReimbursementAccountFormDraft>,
reimbursementAccount: OnyxEntry<ReimbursementAccount>,
): string {
const country = reimbursementAccountDraft?.[INPUT_IDS.ADDITIONAL_DATA.COUNTRY]
?? reimbursementAccount?.achData?.[INPUT_IDS.ADDITIONAL_DATA.COUNTRY] ?? '';
return policy?.outputCurrency ?? reimbursementAccountDraft?.currency
?? CONST.BBA_COUNTRY_CURRENCY_MAP[country] ?? '';
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: ca521f9050
ℹ️ 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".
| if (!value && !policyID) { | ||
| setShowNoPolicyError(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This guard only blocks the No branch, but for wallet onboarding in Australia (country=AU, so currency falls back to AUD) selecting Yes still proceeds without a policy and later reaches handleEmailSubmit, which sends policyID: String(policyID) as "undefined" to ASK_FOR_CORPAY_SIGNER_INFORMATION. That recreates the same API-error class this change is addressing, just on the Yes path instead of No, and blocks completion for non-workspace AU accounts.
Useful? React with 👍 / 👎.
|
🚧 @joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
joekaufmanexpensify
requested review from
dominictb
and removed request for
thesahindia
|
🚧 @joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
BUG: showing Not here error after deleting workspace with bank account linked to itI got this after creating a workspace and linking bank account to it to resolve the director error, then proceeding one step past where we throw the director error in the bank account flow, and then deleting the workspace. I am guessing we need the workspace to actually exist the entire time this director thing is ongoing, until the bank account is fully verified. Is that right? 2026-03-12_14-53-43.mp4 |
|
Triggering a new AdHoc build |
|
🚧 @srikarparsi has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
I just tried it on the build and for some reason I'm still seeing the hang tight screen. But when I test it locally, it correctly reverts to the director step. Locally: |
|
@koko57 could you try merging main into this PR and I'll test again |
|
@joekaufmanexpensify @srikarparsi I confirm - when we delete the workspace, we're kicked back to the Signer Info step. Thank you @srikarparsi! Screen.Recording.2026-04-01.at.09.10.21.mp4So no changes on the FE necessary. |
|
🚧 @joekaufmanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Works well for me too. Good from my perspective!
|
Nice! @dominictb do you have time to review this? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-04-02.at.15.13.28.mov |
src/languages/en.ts
Outdated
| connectToWorkspacePrefix: 'Please connect this bank account to a', | ||
| connectToWorkspaceLink: 'workspace', | ||
| connectToWorkspaceSuffix: 'to invite a director to sign.', |
There was a problem hiding this comment.
Don't break these terms, use anchors <a href>
Lines 3373 to 3374 in 9a57c32
|
|
||
| const handleIsDirectorSelected = useCallback( | ||
| (value: boolean) => { | ||
| if (!policyID && (!value || currency === CONST.CURRENCY.AUD)) { |
There was a problem hiding this comment.
AUD has a slightly different logic - for AUD we always require an email
after the latest change |
|
All you @srikarparsi ! |
|
🚧 @srikarparsi has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/srikarparsi in version: 9.3.52-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This PR adds client-side error handling for an edge case: when a user adds a non-USD business bank account from the Wallet (without a workspace) and reaches the signer info step where they need to invite a director. The new error message ("Please connect this bank account to a workspace to invite a director to sign.") guides users to the correct flow. The existing help site articles already document the correct workspace-based flow:
Since no new user-facing features or flows are introduced — only a better error message for an unsupported path — the current documentation remains accurate and complete. |
|
Deploy Blocker #87022 was identified to be related to this PR. |
7 participants
Explanation of Change
Fixed Issues
$ #84804
PROPOSAL: -
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Screen.Recording.2026-03-11.at.18.00.20.mp4
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
Screen.Recording.2026-03-11.at.18.06.03.mp4
MacOS: Chrome / Safari
Screen.Recording.2026-03-11.at.16.54.31.mp4