-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: Enable report payment for admins with business bank account access in New Expensify #78794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: Enable report payment for admins with business bank account access in New Expensify #78794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an issue where admins with business bank account access could not pay reports in New Expensify, even though they could in Expensify Classic. The fix enables the payment action for workspace admins who have access to the same business bank account as the reimburser, even if they are not the designated reimburser themselves.
Key Changes:
- Modified the
isPayerfunction to check if an admin has bank account access via theshareesfield in addition to checking if they are the reimburser - Threaded the
bankAccountListparameter through multiple utility functions and React components to enable bank account access validation - Added test coverage for the new bank account access logic
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/libs/ReportUtils.ts |
Core logic change: Updated isPayer function to accept bankAccountList parameter and check if admin has bank account access via sharees |
src/libs/ReportSecondaryActionUtils.ts |
Updated function signatures to accept and pass bankAccountList for secondary actions (cancel payment, export, mark as exported) |
src/libs/ReportPrimaryActionUtils.ts |
Updated isPrimaryPayAction and getReportPrimaryAction to accept and pass bankAccountList parameter |
src/libs/ReportPreviewActionUtils.ts |
Updated canPay and getReportPreviewAction to accept and pass bankAccountList parameter |
src/libs/NextStepUtils.ts |
Updated next step calculation functions to accept and pass bankAccountList for correct payer determination |
src/hooks/useTodos.ts |
Added bankAccountList hook and passed it to isPrimaryPayAction for todo list calculations |
src/components/MoneyReportHeader.tsx |
Added bankAccountList hook and passed it through primary/secondary action calculations |
src/components/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx |
Added bankAccountList hook and included it in report preview action calculations |
tests/unit/ReportUtilsTest.ts |
Added test case validating that admins with bank account access via sharees can pay reports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
|
|
Failing checks are not related to our changes. |
@samranahm This missing |
|
WIP |
|
@samranahm also, please merge with main to see if the lint is fixed |
JS00001
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, one comment thats not critical
|
@samranahm please fix the conflicts and also this lint error
|
|
On it, will fix them ASAP |
|
Looks like prettier is failing |
|
@bernhardoj All yours. |
|
@bernhardoj can you re-review it today? It's a feature disparity gap we'd like to get fixed ASAP. 👍 |
|
@samranahm the |
|
Ahh, I’ll make them required here too and review all the changes again. |
|
@samranahm let us know when this is ready for another review! |
|
I’m reviewing it one more time and will ping you shortly. |
|
@bernhardoj @JS00001 PR ready, please take a look. |
src/libs/actions/IOU/index.ts
Outdated
| }); | ||
|
|
||
| let allBankAccountList: OnyxEntry<OnyxTypes.BankAccountList> = {}; | ||
| Onyx.connectWithoutView({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to add a new Onyx.connectWithoutView usage in this file. Let's pass it all from useOnyx hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I see that we use it mostly for hasOutstandingChildRequest(). @JS00001 When the non-payer approver can pay (bcs the bank account is shared), should the hasOutstandingChildRequest be true or not?
If not, then we can just pass undefined to hasOutstandingChildRequest()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should get confirmation here. Do we want to show the GBR for admins with bank account access, or should it only be displayed for the actual payer? I think passing undefined to hasOutstandingChildRequest makes sense. @JS00001 what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to add a new Onyx.connectWithoutView usage in this file. Let's pass it all from useOnyx hook.
Agree, lets remove this
Yes, we should get confirmation here. Do we want to show the GBR for admins with bank account access, or should it only be displayed for the actual payer?
I think it should show for only the payer, not all admins, cc @trjExpensify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, agreed. 👍
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Code LGTM. @JS00001 can we have an ad-hoc build so @joekaufmanexpensify can help us test this? (for ref) |


Explanation of Change
Fixed Issues
$ #78247
PROPOSAL: #78247 (comment)
Tests
Preconditions:
Test:
Offline tests
QA Steps
Same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
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.This PR will be tested by an internal engineer due to inability to add a real business bank account for testing. #78247 (comment)
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari