-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix add business bank account opens personal bank account flow in invoice room #75530
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?
Conversation
|
@mananjadhav 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] |
| const isInvoiceReceiverPolicyCurrencySupported = isCurrencySupportedForDirectReimbursement(invoiceReceiverPolicy?.outputCurrency ?? ''); | ||
|
|
||
| const canUseActivePolicy = hasActivePolicyAsAdmin && isActivePolicyCurrencySupported; | ||
| const isPolicyCurrencySupported = invoiceReceiverPolicy ? isInvoiceReceiverPolicyCurrencySupported : canUseActivePolicy || isUserCurrencySupported; |
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.
❌ Missing Error Handling
The createWorkspace() function is called within a click handler without any error handling or loading state management.
Why this matters:
- Workspace creation is an async operation that can fail (network issues, server errors, etc.)
- Users get no feedback while the workspace is being created
- If creation fails, navigation to
navigateToBankAccountRoute(policyID)may use an invalid policyID
Suggested fix: Add proper async handling:
onSelected: async () => {
if (payAsBusiness) {
if (chatReport?.invoiceReceiver?.type === CONST.REPORT.INVOICE_RECEIVER_TYPE.BUSINESS) {
navigateToBankAccountRoute(chatReport?.invoiceReceiver?.policyID);
} else {
if (!canUseActivePolicy) {
try {
const {policyID} = await createWorkspace();
navigateToBankAccountRoute(policyID);
} catch (error) {
// Handle error appropriately
console.error('Failed to create workspace:', error);
}
} else {
navigateToBankAccountRoute(activePolicy.id);
}
}
} else {
Navigation.navigate(ROUTES.SETTINGS_ADD_BANK_ACCOUNT.route);
}
},Check if createWorkspace() returns a promise and handle it appropriately.
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.
@bernhardoj Can you check this 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.
I left this unresolved since I'm not sure how to handle this. We create the workspace optimistically and navigate to the bank account page. Should we show not found page if the workspace is failed to be created?
App/src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx
Lines 426 to 435 in cb4506d
| if ((!isLoading && (isEmptyObject(policy) || !isPolicyAdmin(policy))) || isPendingDeletePolicy(policy)) { | |
| return ( | |
| <ScreenWrapper testID={ReimbursementAccountPage.displayName}> | |
| <FullPageNotFoundView | |
| shouldShow | |
| onBackButtonPress={goBackFromInvalidPolicy} | |
| onLinkPress={goBackFromInvalidPolicy} | |
| subtitleKey={isEmptyObject(policy) || isPendingDeletePolicy(policy) ? undefined : 'workspace.common.notAuthorized'} | |
| /> | |
| </ScreenWrapper> |
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.
|
|
Recording of the other cases: Case 1: 1.mp4Case 2: 2.mp4Case 3: 4.mp4Case 4: 3.mp4Case 5: 5.mp4 |
…rsonal-bank-account-flow
heyjennahay
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.
No concern with product change
|
Reviewed the code, but requires more testing. @bernhardoj Can you please resolve conflicts? |
…rsonal-bank-account-flow
|
Fixed the conflict |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-business-account-flow.movAndroid: mWeb Chromemweb-chrome-business-account-flow.moviOS: HybridAppios-business-account-flow.moviOS: mWeb Safarimweb-safari-business-account-flow.movMacOS: Chrome / Safariweb-business-account-flow.mov |
|
@bernhardoj Can you please resolve the conflicts? |
…rsonal-bank-account-flow
|
@mananjadhav fixed |
Explanation of Change
In invoice room, when we choose Pay as business > Add bank account, it'll open the add personal bank account page. This PR navigates the user to the correct add business bank account page.
Fixed Issues
$ #73763
PROPOSAL: #73763 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Main test:
Prerequisite: Account B has a default workspace (as admin) and the WS currency is USD
Another case:
Case 1, Account B has no default workspace (as admin), user currency not USD
Case 2, Account B has a default workspace (as admin), both workspace and user currency are not USD
Case 3, Account B has no default workspace (as admin), user currency is USD
Case 4, Account B has a default workspace (as admin), WS currency is not USD, but user currency is USD.
Case 5, invoice room with business type, Account B has a default workspace (as admin) and the WS currency is USD
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
android.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4