Block currency changes when workspace has partially setup bank account#86075
Block currency changes when workspace has partially setup bank account#86075
Conversation
…ccount When a non-US bank account is partially set up via the Corpay flow, the backend correctly rejects currency changes. The frontend now also blocks currency changes in the UI by checking for partially setup bank accounts using the existing doesPolicyHavePartiallySetupBankAccount utility. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Fixed the failing ESLint check: replaced |
|
The failing check (test job 8) is unrelated to this PR's changes. Analysis: The job failed during the "Set up job" phase while downloading GitHub Actions dependencies ( Evidence: This is a transient GitHub Actions infrastructure/network issue. Re-running the failed job should resolve it. |
|
The failing checks are unrelated to this PR's code changes.
Re-running these checks should resolve both failures. |
|
@MelvinBot since the logic is duplicated, let's refactor the logic into a hook for the shouldBlockCurrencyChange |
Extract the duplicated shouldBlockCurrencyChange logic from WorkspaceOverviewCurrencyPage and WorkspaceOverviewPage into a new useShouldBlockCurrencyChange hook. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Refactored the duplicated The hook encapsulates:
Both |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Fixed the failing Prettier check: sorted the |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@MelvinBot since hasVBA from WorkspacePageWithSections is not being used anywhere, let's remove it. |
The hasVBA parameter is no longer consumed by any child component after the shouldBlockCurrencyChange hook refactor. Remove it from the children type signature, computation, and the one remaining consumer (WorkspaceInvoicesPage). Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Removed the unused
|
| modals={modals} | ||
| > | ||
| {(hasVBA?: boolean) => ( | ||
| {() => ( |
There was a problem hiding this comment.
@MelvinBot I think we can remove the function style here (line 559). We can just render the component directly
Now that hasVBA is no longer consumed from the render prop, render the children directly instead of using function style. Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Removed the function-style render prop wrapper. Since |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Fixed the failing Prettier check: the children inside the |
|
The failing check ( Analysis: This is a
TypeScript files are bundled by Metro, not compiled by xcodebuild. The native iOS build failure cannot be caused by these changes. Evidence: The same |
|
@MelvinBot Mark this PR as ready |
|
@bernhardoj 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] |
|
The failing check ( Analysis: The iOS build fails with Swift compilation errors in Evidence: This PR only modifies TypeScript files ( Recommendation: Re-run the failing check. |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
@bernhardoj there are no screenshots/videos in the PR, is Melvin supposed to add them?
|
The failing check ( Analysis: The failure is in the Evidence: The same |
|
@joekaufmanexpensify I don't think Melvin can add them |
|
@MelvinBot can you merge main into this branch? |
|
I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me merge main into this branch, the PR assignee ( |
|
The failing check is unrelated to this PR. Failing check: Analysis: This PR only modifies TypeScript files ( Evidence: The same Re-running the failed check should resolve this. |
|
@bernhardoj Could you add them then? I am interested to understand more about what this change will look like. |
|
@joekaufmanexpensify I put the rec on my checklist. Here is the rec: web.16.10.53.mp4 |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Looks good to me 👍
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Fixed the failing perf-tests check. The issue was a duplicate |
|
@bernhardoj can you merge main to handle the conflict? |
|
@MelvinBot please merge with |
…olve conflict Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Merged Conflict details: |
|
@nkuoch done |
|
🚧 @nkuoch 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/nkuoch in version: 9.3.52-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. Why: This PR extends an existing currency-lock behavior (previously triggered only by a fully verified bank account) to also cover partially-setup bank accounts. The UI hint text is unchanged — The relevant help site articles are unaffected:
There is no standalone New Expensify article for changing workspace currency. The in-app hint text adequately explains the restriction to users who encounter it. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.52-9 🚀
|
Explanation of Change
When a workspace has a non-US bank account that is only partially set up (via the Corpay flow), the backend correctly rejects currency changes with error "You cannot change the currency because this policy is linked to a business bank account." However, the frontend was not blocking the currency field, allowing users to attempt a change that would silently revert.
This PR uses the existing
doesPolicyHavePartiallySetupBankAccountutility to also disable the currency field and block the currency selection page when the workspace has a partially-setup bank account (state isSETUP,VERIFYING, orPENDING).Changes:
WorkspaceOverviewCurrencyPage.tsx: Block the page viaAccessOrNotFoundWrapperwhen a partially-setup bank account exists for the policyWorkspaceOverviewPage.tsx: Disable the currency menu item (hide right icon, disable interaction, show disabled hint text) when a partially-setup bank account existsFixed Issues
$ #84535
PROPOSAL: #84535 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari