Fix vendor bill showing credit card accounts in per-card export#90355
Fix vendor bill showing credit card accounts in per-card export#90355MelvinBot wants to merge 4 commits into
Conversation
Remove VENDOR_BILL from the credit card switch fall-through in both QBO and QBD branches of getExportMenuItem, so vendor bill now correctly hits the default case (shouldShowMenuItem: false, data: []). Add a useEffect guard in WorkspaceCompanyCardAccountSelectCardPage that navigates back to card details when shouldShowMenuItem becomes false, preventing stale screen rendering after export type changes. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| switch (qboConfig) { | ||
| case CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY: | ||
| case CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.CHECK: | ||
| case CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.VENDOR_BILL: |
There was a problem hiding this comment.
@MelvinBot no need this change. shouldShowMenuItem is already set as false here:
There was a problem hiding this comment.
You're right — shouldShowMenuItem is already set to false for VENDOR_BILL before the switch (line 64), so removing it from the fall-through was unnecessary. Reverted in d51148d.
| switch (qbdConfig) { | ||
| case CONST.QUICKBOOKS_DESKTOP_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY: | ||
| case CONST.QUICKBOOKS_DESKTOP_REIMBURSABLE_ACCOUNT_TYPE.CHECK: | ||
| case CONST.QUICKBOOKS_DESKTOP_REIMBURSABLE_ACCOUNT_TYPE.VENDOR_BILL: |
There was a problem hiding this comment.
Reverted in d51148d. Same reasoning — shouldShowMenuItem already handles VENDOR_BILL at lines 310-312.
shouldShowMenuItem is already set to false for VENDOR_BILL before the switch statement in both QBO (line 64) and QBD (lines 310-312), so removing it from the fall-through was unnecessary. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
| if (exportMenuItem?.shouldShowMenuItem !== false) { | ||
| return; | ||
| } | ||
| Navigation.goBack(ROUTES.WORKSPACE_COMPANY_CARD_DETAILS.getRoute(policyID, feed, cardID, backTo)); |
There was a problem hiding this comment.
@MelvinBot after selecting Vendor Bill, it should go back to Export company card expenses as page.
But because of this goBack logic, it directly goes to Card details page as WorkspaceCompanyCardAccountSelectCardPage component is still mounted.
There was a problem hiding this comment.
You're right — the useEffect with goBack was too aggressive. When the user selects Vendor Bill, the normal navigation should return to the "Export company card expenses as" page, but the useEffect was firing and sending them directly to Card Details. Reverted in f363296.
The useEffect that navigated back when shouldShowMenuItem became false was interfering with the normal flow: when selecting Vendor Bill, it should go back to the "Export company card expenses as" page, but the goBack was sending the user directly to Card Details since the component was still mounted. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
@MelvinBot how about removing |
… false Use Navigation.removeScreenFromNavigationState instead of goBack to cleanly remove the screen from the stack when the export menu item should not be shown. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Done in e27f8ef — updated |
Explanation of Change
Two bugs caused vendor bill export to incorrectly show credit card accounts in the per-card company card export flow:
getExportMenuItemfall-through bug (QBO + QBD):VENDOR_BILLwas grouped into the same switch case asCREDIT_CARD, so when the export destination was vendor bill, the function returned credit card account data instead of treating it as a hidden menu item. This PR removesVENDOR_BILLfrom the credit card fall-through in both the QBO and QBD branches, so it now correctly hits thedefaultcase (shouldShowMenuItem: false,data: []).Stale screen on back-navigation:
WorkspaceCompanyCardAccountSelectCardPagenever checkedshouldShowMenuItem, so if the user changed the export type to vendor bill from within the account select flow and navigated back, the stale screen remained mounted showing wrong data. This PR adds auseEffectthat navigates back to the card details page whenshouldShowMenuItembecomesfalse.Fixed Issues
$ #89148
PROPOSAL: #89148 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
// TODO: The human co-author must fill out offline test steps
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
AI Tests
npm run lint-changednpx eslint(changed files)npm run typecheck-tsgonpx prettier --write(changed files)npm test -- --testPathPattern="companyCards"npm run react-compiler-compliance-check check-changed