Hide Payer row during incomplete bank account setup#90538
Conversation
The Payer button on the Workflows page was visible during incomplete bank account setup, but clicking it led to a "Not here" page because the Payer page requires reimbursementChoice === REIMBURSEMENT_YES. Adding an isBankAccountFullySetup guard to the Payer row visibility ensures it only appears when the bank account has reached OPEN or LOCKED state. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@MelvinBot implement your 2nd approach in your original proposal. Do not implement other proposal. |
…tFullySetup Align the Payer row visibility with the Payer page guard by checking reimbursementChoice === REIMBURSEMENT_YES in shouldShowBankAccount, rather than gating on isBankAccountFullySetup. This allows the Payer row to appear during partial bank setup when direct reimbursement is enabled, while still hiding it when reimbursementChoice is not YES. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Updated to use the 2nd approach from the original proposal:
This aligns the Workflows page with the Payer page's |
|
|
||
| const shouldShowBankAccount = (!!isBankAccountFullySetup || !!bankAccountConnectedToWorkspace) && policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO; | ||
| const shouldShowBankAccount = | ||
| (!!isBankAccountFullySetup || !!bankAccountConnectedToWorkspace) && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES; |
There was a problem hiding this comment.
@MelvinBot do NOT change shouldShowBankAccount logic.
Only tighten payer visibility logic here:
- {shouldShowBankAccount && (
+ {shouldShowBankAccount && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES && (Add reimbursementChoice === REIMBURSEMENT_YES check to the Payer row rendering condition, aligning it with the Payer page's FullPageNotFoundView guard. shouldShowBankAccount logic is unchanged. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Updated per your feedback. The change is now a single-line addition on the Payer row only — - {shouldShowBankAccount && (
+ {shouldShowBankAccount && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES && ( |
|
@MelvinBot complete author checklist |
|
Completed the author checklist. Most items are N/A for this one-line conditional change. Left unchecked the items that require human action:
|
|
@MelvinBot complete author checklist |
| /> | ||
| )} | ||
| {shouldShowBankAccount && ( | ||
| {shouldShowBankAccount && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES && ( |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Completed the author checklist. This is a single-line condition change with no new UI, files, CSS, assets, copy, or code patterns — most N/A items are checked off. Still needs human co-author:
|
|
@MelvinBot check off all items as I already verified |
Tests verify that the Payer row: - Shows when reimbursementChoice is REIMBURSEMENT_YES with a bank account - Hides when reimbursementChoice is REIMBURSEMENT_MANUAL - Shows during partial bank setup when reimbursementChoice is REIMBURSEMENT_YES Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Added unit tests in
|
|
Done — all checklist items have been checked off. |
Add missing required properties (accountNumber, routingNumber) to ACHAccount test data and (bankCurrency, bankCountry) to BankAccountList test data. Use CONST.BANK_NAMES.GENERIC_BANK instead of a freeform string for bankName in additionalData. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed the failing typecheck: added missing required properties ( |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot fix failing lint |
- Use getByText instead of queryByText for presence assertions - Use computed property for numeric bank account ID key Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed the failing ESLint check — 3 errors in |
|
@MelvinBot add Tests steps and mark as ready for review |
|
@mkhutornyi 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] |
|
Added test steps (Tests, Offline tests, QA Steps) and marked the PR as ready for review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb1.movweb2.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #88263 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
There's infinite loop in author checklist. @nkuoch can you please remove this manually?
|
trjExpensify
left a comment
There was a problem hiding this comment.
I was just in a discussion with @arosiclair in #migrate about a bunch of workspaces that have direct reimbursement fully set up, but seemingly no reimbursementChoice stored on the workspace. I expect this is some crusty Classic data or something, but I fear that solely relying on reimbursementChoice here will cause the Payer row to be hidden when it shouldn't be.
Is there another policy attribute we could use in addition? Like, achAccount.state or something?
|
App/src/pages/workspace/workflows/WorkspaceWorkflowsPayerPage.tsx Lines 250 to 251 in d65ec27 In production, we're showing not found page when click on payer row if |
|
Right, but I'm pointing out that we've found that sometimes there is no reimbursementChoice in the policyData at all despite the business being set up fully for reimbursement. So by looking at reimbursementChoice alone, we risk hiding the |
If we still show @MelvinBot thoughts on #90538 (review)? |
|
I agree with
If the goal is to support legacy workspaces where |
| /> | ||
| )} | ||
| {shouldShowBankAccount && ( | ||
| {shouldShowBankAccount && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES && ( |
There was a problem hiding this comment.
@MelvinBot I think some policies might just not have a reimbursementChoice set, but still have a bank account. If that's the case, we should probably still show payer row. So, instead, maybe we should show if reimbursementChoice NOT IN (manual, no)?
|
That's a solid approach — using a negated check ( However, the Payer page guard at
|
yes, do that @MelvinBot |
Switch both the Payer row (WorkspaceWorkflowsPage) and Payer page guard (WorkspaceWorkflowsPayerPage) from checking reimbursementChoice === REIMBURSEMENT_YES to checking reimbursementChoice !== REIMBURSEMENT_NO && !== REIMBURSEMENT_MANUAL. This ensures legacy workspaces with undefined reimbursementChoice (but fully set up reimbursement) still show the Payer row and page correctly. Co-authored-by: mkhutornyi <58530508+mkhutornyi@users.noreply.github.com> Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@mkhutornyi 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] |
|
Updated both guards to use the negated approach: Row ( Page ( Added a unit test for the legacy workspace scenario (undefined |
| </OfflineWithFeedback> | ||
| )} | ||
| {shouldShowBankAccount && | ||
| policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO && |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The check policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO on this line is redundant. shouldShowBankAccount (defined on line 304) already includes this exact condition:
const shouldShowBankAccount = (!!isBankAccountFullySetup || !!bankAccountConnectedToWorkspace) && policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO;If shouldShowBankAccount is true, then reimbursementChoice is guaranteed to not be REIMBURSEMENT_NO. Remove this redundant condition:
{shouldShowBankAccount &&
policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL && (Reviewed at: ed119e8 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good finding. @MelvinBot address #90538 (comment)
There was a problem hiding this comment.
Good catch — removed the redundant != REIMBURSEMENT_NO check since shouldShowBankAccount already guarantees that. Pushed in 1442a66.
shouldShowBankAccount already includes this condition, so repeating it on the Payer row guard is redundant. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent the Workflows page from showing the Payer row when the workspace is not in a direct reimbursement state, avoiding navigation to an unsupported Payer page during incomplete bank account setup.
Changes:
- Adds reimbursement-choice gating around the Payer row on the Workflows page.
- Adjusts the Payer page blocking condition.
- Adds UI tests for Payer row visibility across reimbursement states.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx |
Adds reimbursement-choice checks before rendering the Payer row. |
src/pages/workspace/workflows/WorkspaceWorkflowsPayerPage.tsx |
Updates the Payer page not-found guard for reimbursement choices. |
tests/ui/WorkspaceWorkflowsPayerRowTest.tsx |
Adds tests for Payer row visibility with bank account and reimbursement-choice combinations. |
Comments suppressed due to low confidence (1)
tests/ui/WorkspaceWorkflowsPayerRowTest.tsx:166
- This test codifies the unsafe case as expected behavior: an undefined
reimbursementChoiceis still “notREIMBURSEMENT_YES”, so showing the Payer row can reproduce the Not here/unsupported Payer page flow when a partial bank account entry exists. The regression coverage should assert that partial setup with a missing/non-YES choice hides the row instead.
it('shows the Payer row when reimbursementChoice is undefined (legacy workspaces)', async () => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /> | ||
| )} | ||
| {shouldShowBankAccount && ( | ||
| {shouldShowBankAccount && policy?.reimbursementChoice !== CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL && ( |
| policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO || | ||
| policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL; |
| it('hides the Payer row when reimbursementChoice is REIMBURSEMENT_MANUAL', async () => { | ||
| await TestHelper.signInWithTestUser(); | ||
| await act(async () => { | ||
| await Onyx.merge( | ||
| `${ONYXKEYS.COLLECTION.POLICY}${POLICY_ID}`, | ||
| buildPolicy({ | ||
| reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL, | ||
| achAccount: { | ||
| reimburser: 'test@user.com', | ||
| bankAccountID: 123456, | ||
| accountNumber: '1234567890', | ||
| routingNumber: '011000015', | ||
| bankName: 'Test Bank', | ||
| addressName: 'Test Address', | ||
| state: CONST.BANK_ACCOUNT.STATE.OPEN, | ||
| }, | ||
| }), | ||
| ); |

Explanation of Change
When a workspace admin starts but doesn't complete bank account setup, a partial bank account entry exists that makes
shouldShowBankAccounttruthy. This caused the Payer button to appear on the Workflows page. However, clicking it navigated to the Payer page which requiresreimbursementChoice === REIMBURSEMENT_YES, resulting in a "Not here" (FullPageNotFoundView) screen.This adds a
reimbursementChoice === REIMBURSEMENT_YESguard directly on the Payer row atWorkspaceWorkflowsPage.tsx:582, aligning it with the Payer page'sFullPageNotFoundViewcondition. TheshouldShowBankAccountlogic is unchanged — the bank account row and other elements it controls remain unaffected.Fixed Issues
$ #88263
PROPOSAL: #88263 (comment)
Tests
reimbursementChoiceis notREIMBURSEMENT_YESduring partial setup)OPENstate andreimbursementChoicebecomesREIMBURSEMENT_YESOffline tests
reimbursementChoiceisREIMBURSEMENT_YES)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
Screenshots/Videosundefined