fix bank account ending shows incorrect BA after reselect#87152
fix bank account ending shows incorrect BA after reselect#87152apeyada wants to merge 2 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04df0132f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (bankAccountID) { | ||
| deletePaymentBankAccount(bankAccountID, undefined); | ||
| } |
There was a problem hiding this comment.
Delete pending bank account only when Plaid account changed
deletePaymentBankAccount() is called on every Plaid submit whenever bankAccountID exists, but there is no check that the user actually switched to a different Plaid account. In the common back-navigation case (user returns to Bank Info and presses Next with the same selection), this deletes the in-progress VBBA and can reset/discard data entered in later steps (requestor/company/owners), forcing the flow to restart unexpectedly. The delete should be gated on an actual account change (e.g., comparing previous vs newly selected plaidAccountID).
Useful? React with 👍 / 👎.
|
The review comment raises a valid point — Proposed fix: Gate the delete on an actual account change by comparing } else if (setupType === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID) {
- if (bankAccountID) {
+ const previousPlaidAccountID = reimbursementAccount?.achData?.plaidAccountID;
+ const newPlaidAccountID = data[BANK_INFO_STEP_KEYS.PLAID_ACCOUNT_ID];
+ if (bankAccountID && previousPlaidAccountID && previousPlaidAccountID !== newPlaidAccountID) {
deletePaymentBankAccount(bankAccountID, undefined);
}This way
If the user navigates back and re-submits the same selection, the existing VBBA and any data entered in later steps (requestor/company/owners) are preserved. |
PR ReviewOverall: The changes look correct and well-guarded. Two observations: 1. Sequencing of delete → connect (low risk, but worth noting)
2. The
|
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
Explanation of Change
bankAccountIDinCONNECT_BANK_ACCOUNT_WITH_PLAIDapi as it's automatically set in backend..Fixed Issues
$ #81946
PROPOSAL: #81946 (comment)
Tests
Precondition: User with a workspace that has the Invoices feature enabled.
Offline tests
Same as Tests
QA Steps
Same as Tests
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov