feat: Update option missing for Personal Bank Accounts in ND compared to Classic #80020
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team and
trjExpensify
|
@dukenv0307 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04a4154be9
ℹ️ 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".
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.
|
src/libs/BankAccountUtils.ts
Outdated
| * This is used to show "Action required" badge for existing accounts that need updates | ||
| * to enable global reimbursement payments. | ||
| */ | ||
| function isPersonalBankAccountMissingInfo(accountData: AccountData | undefined, privatePersonalDetails: OnyxEntry<OnyxTypes.PrivatePersonalDetails>): boolean { |
There was a problem hiding this comment.
Checking privatePersonalDetails here is incorrect. OldDot shows the update button when it's missing fields on the bankAccount additionalData specifically. See
https://github.com/Expensify/Web-Expensify/blob/main/site/class/bankAccount.js#L240
There was a problem hiding this comment.
@chuckdries Thanks for the feedback! I don't have access to Web-Expensify repo.
In NewDot, BankAccountAdditionalData doesn't have personal info fields (legalFirstName, addressStreet, phoneNumber) - these only exist on PrivatePersonalDetails.
From the issue discussion, @joekaufmanexpensify mentioned that "completing one account saves info to all (stored in Profile)" - which aligns with checking privatePersonalDetails.
Could you clarify:
- What specific fields does OldDot check on bankAccount.additionalData or share the source code for that section?
- Should we add these fields to BankAccountAdditionalData type, or is checking privatePersonalDetails the correct approach for NewDot?
Happy to adjust the implementation based on your guidance.
There was a problem hiding this comment.
Ah, I forgot that repo is private, sorry about that! The BankAccountAdditionalData interface is incomplete. I'll look up what it should be and get back to you tomorrow :)
There was a problem hiding this comment.
So, turns out there's actually a bunch of fields that can be on that object that I don't think are relevant. Here's a diff you can apply that'll add the relevant fields to BankAccount
diff --git a/src/types/onyx/BankAccount.ts b/src/types/onyx/BankAccount.ts
index 54581f3a071..bb807e7b663 100644
--- a/src/types/onyx/BankAccount.ts
+++ b/src/types/onyx/BankAccount.ts
@@ -38,6 +38,21 @@ type BankAccountAdditionalData = {
/** Powerform files */
achAuthorizationForm?: FileObject[];
};
+
+ /** City of the business's address */
+ addressCity?: string;
+
+ /** State of the business's address */
+ addressState?: string;
+
+ /** Business's street address */
+ addressStreet?: string;
+
+ /** Zip code of the business's address */
+ addressZipCode?: string;
+
+ /** Phone number of the company */
+ companyPhone?: string;
};
/** Model of bank account */
There was a problem hiding this comment.
@chuckdries can you please take a look at the comment above? Thanks
There was a problem hiding this comment.
Sorry for the delay on this all, I will check tomorrow
There was a problem hiding this comment.
Hi @TaduJR, thanks again for your patience. I wanted to make sure I was right before I replied this time :)
- the owner's name is stored in the
firstNameandlastNamefields on the BankAccountAdditionalData object - the owner's personal phone number is stored in the
companyPhonefield - you're right about the comments being confusing, I was tripped up reading the existing code because we reuse this interface for personal and business bank accounts. We can just change the comments to be "State of the bank account owner's address", etc.
I just noticed that UpdatePersonalBankAccountInfo does not take a phone number parameter. I'll file a ticket to get that added
There was a problem hiding this comment.
Thanks @chuckdries! I've updated the code based on your feedback:
- Added
firstNameandlastNametoBankAccountAdditionalDatatype - Updated
isPersonalBankAccountMissingInfoto checkaccountData.additionalDatafields (firstName,lastName,addressStreet,addressCity,addressState,addressZipCode,companyPhone) instead ofprivatePersonalDetails
Let me know if you need anything else!
There was a problem hiding this comment.
Sounds great, thanks! The required changes to UpdatePersonalBankAccountInfo were just merged moments ago, they'll become available on staging in the next few days. I will ping you here when that happens 👍
|
@TaduJR Any update here? |
|
kindly bump @TaduJR |
|
Bump @TaduJR |
Hi @dukenv0307 Already replied 3 days ago. We are awaiting for clarifications on this #80020 (comment) |
|
@TaduJR Ah I see now. Thank you |
…-option-missing-for-Personal-Bank-Accounts-in-ND-compared-to-Classic
Add missing fields to BankAccountAdditionalData type: - addressCity, addressState, addressStreet, addressZipCode - companyPhone
…-option-missing-for-Personal-Bank-Accounts-in-ND-compared-to-Classic
…-option-missing-for-Personal-Bank-Accounts-in-ND-compared-to-Classic
…missing bank account info Per reviewer feedback, isPersonalBankAccountMissingInfo now checks accountData.additionalData fields (firstName, lastName, address, companyPhone) instead of privatePersonalDetails, matching OldDot behavior.
3 participants
Explanation of Change
In Expensify Classic, users with US personal bank accounts can update their name, address, and phone number when this info is missing from the bank account. This is required for global reimbursement payments (e.g., GBP → USD) to succeed. NewDot was missing this flow entirely.
What this PR adds:
Detection:
isPersonalBankAccountMissingInfo()checksbankAccount.additionalDatafor missingfirstName,lastName, address fields, orcompanyPhoneon US personal bank accounts in OPEN stateVisual indicator: Shows "Action required" badge with brick road indicator on affected bank account rows in Wallet (matching the existing partially-setup pattern). Three dots menu still works independently for
deletion
Update flow: Tapping the row navigates to a new
UpdatePersonalBankAccountPagethat reuses theLegalNameStep,AddressStep, andPhoneNumberStepsubsteps from PR Change for personal bank account setup #75179. Steps with info already in the user's profile are automatically skippedConfirmation & success: Custom
UpdatePersonalInfoConfirmationshows a review screen (without bank routing/account numbers). On submit, a success screen with Fireworks animation confirms the updateupdatePersonalBankAccountInfocallsUpdatePersonalBankAccountInfoand optimistically updatesBANK_ACCOUNT_LISTadditionalData for all affected accounts +PRIVATE_PERSONAL_DETAILS. Form draft ispreserved on failure so the user can retry
Translations: All 5 new keys added to all 10 language files
Fixed Issues
$ #76053
PROPOSAL: #76053 (comment)
Tests
Prerequisite: User with a US personal bank account missing personal info (name, address, or phone)
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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