Ensure only latest bank account shows default badge #51437

Shahidullah-Muffakir · 2024-10-24T19:04:32Z

Details

Updated logic to ensure only the latest added bank account is marked as default, preventing two accounts from being shown as default without needing a page refresh.

Fixed Issues

$ #50829
PROPOSAL: #50829 (comment)

Tests

Log in to the account that has no bank accounts.
Navigate to Settings > Wallet and click on Add Bank Account > Personal Bank Account.
On the Plaid modal, search and select Wells Fargo bank, enter credentials (e.g., user_good / pass_good), and complete the process.
Verify that the added bank account is not marked as the default(because we are not showing default badge if there is only one bank account)
Repeat the process to add a second bank account by selecting Chase bank.
Verify that only the newly added bank account is marked as default, and the previous account does not show the default badge.
Refresh the page and confirm that the correct default account is still displayed.

Verify that no errors appear in the JS console

Offline tests

This feature requires an active internet connection.

QA Steps

Log in to the account that has no bank accounts.
Navigate to Settings > Wallet and click on Add Bank Account > Personal Bank Account.
On the Plaid modal, search and select Wells Fargo bank, enter credentials (e.g., user_good / pass_good), and complete the process.
Verify that the added bank account is not marked as the default(because we are not showing default badge if there is only one bank account)
Repeat the process to add a second bank account by selecting Chase bank.
Verify that only the newly added bank account is marked as default, and the previous account does not show the default badge.
Refresh the page and confirm that the correct default account is still displayed.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android.20native.mp4

Android: mWeb Chrome

Android.20mweb.20chrome.mp4

iOS: Native

iOS.20native.mp4

iOS: mWeb Safari

ios.20mweb.20safari.mp4

MacOS: Chrome / Safari

macos.20safari.mp4

MacOS: Desktop

macos.20desktop.mp4

…ple accounts

melvin-bot · 2024-10-24T19:04:37Z

@thesahindia 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]

…ean() with git status

thesahindia · 2024-10-26T15:10:59Z

I will review it in a few hours!

thesahindia · 2024-10-26T21:06:10Z

When I am adding the second account I am getting an error.
@Shahidullah-Muffakir, could you please check if you are getting same error on main?

Shahidullah-Muffakir · 2024-10-26T21:38:39Z

@thesahindia, I tested now, I could add the 2nd account.
It seems we need to test it with different banks, one time CHASE bank and for the 2nd account with WELLS FARGO.
Thank you.

thesahindia · 2024-10-28T12:37:44Z

@thesahindia, I tested now, I could add the 2nd account. It seems we need to test it with different banks, one time CHASE bank and for the 2nd account with WELLS FARGO. Thank you.

I did the same. Trying again.
Edit: It was something account specific. I can't repro on a different account.

thesahindia · 2024-10-28T13:23:10Z

Screen.Recording.2024-10-28.at.6.46.24.PM.mov

When I add the second account the default badge is appearing on the previously added bank account. @Shahidullah-Muffakir, could you please retest your changes?

Shahidullah-Muffakir · 2024-10-28T20:05:25Z

Screen.Recording.2024-10-28.at.6.46.24.PM.mov

When I add the second account the default badge is appearing on the previously added bank account. @Shahidullah-Muffakir, could you please retest your changes?

@thesahindia, Now I noticed an issue with the created date for each account. I just created two new accounts, but one shows a created date from several months ago, even as far back as 2023.Since we’re using this date to show the default badge on the latest added account, it’s causing incorrect badge assignments.
Could you clarify what the created date represents? Is it the date the user added their bank account to Expensify, or the date the account was initially opened at the bank? Thank you.

First account:

Second account:

thesahindia · 2024-10-29T15:39:25Z

Now I noticed an issue with the created date for each account. I just created two new accounts, but one shows a created date from several months ago, even as far back as 2023.Since we’re using this date to show the default badge on the latest added account, it’s causing incorrect badge assignments.
Could you clarify what the created date represents? Is it the date the user added their bank account to Expensify, or the date the account was initially opened at the bank? Thank you.

I think @NikkiWines might be able to answer this.

NikkiWines · 2024-10-29T18:12:28Z

created should be the date the bank account was added into the DB... so it should reflect the order the accounts were added. If those accounts are the testing plaid accounts there might be some weirdness with the created dates since they're re-used regularly 🤔

Shahidullah-Muffakir · 2024-10-29T18:22:47Z

created should be the date the bank account was added into the DB... so it should reflect the order the accounts were added. If those accounts are the testing plaid accounts there might be some weirdness with the created dates since they're re-used regularly 🤔

Thanks for explaining! If the created date is the date the bank account was added to the database, then that should work fine for what we need. I also think that the issue might be because of the test accounts. I noticed that when I re-added the same test account, it showed the correct created date as today’s date and time. So, it seems like the old dates we’re seeing could be because these test accounts.

Shahidullah-Muffakir · 2024-10-31T14:40:37Z

I noticed something during testing:

I added a Chase Bank account (“Plaid Checking”).
Then, I added another account, WELLS FARGO (“Plaid Saving”).
I deleted the Chase Bank account from Step 1.
I re-added the Chase Bank account from Step 1.

When I re-added the account in Step 4, I saw that it kept the original created date from Step 1, rather than updating to the current date. It seems like we aren’t updating the created date if a user re-adds a deleted payment method.

@NikkiWines , do we update the created date for real payment methods in production when a user re-adds a deleted method, or is this just an issue with test accounts? If we can confirm this, I think we will be good to proceed with our PR.

@thesahindia , based on this, we can test our approach by using a new user account. If we add two accounts with a new user, the most recently added should correctly show as the default.

Thank you.

NikkiWines · 2024-11-01T20:40:48Z

@NikkiWines , do we update the created date for real payment methods in production when a user re-adds a deleted method, or is this just an issue with test accounts? If we can confirm this, I think we will be good to proceed with our PR.

Sorry for the delay, I can confirm that when re-adding a deleted account, the original created is used - it's not updated to reflect the most recent time the account was added (for both test and non-test accounts)

Shahidullah-Muffakir · 2024-11-01T23:15:58Z

@NikkiWines , do we update the created date for real payment methods in production when a user re-adds a deleted method, or is this just an issue with test accounts? If we can confirm this, I think we will be good to proceed with our PR.

Sorry for the delay, I can confirm that when re-adding a deleted account, the original created is used - it's not updated to reflect the most recent time the account was added (for both test and non-test accounts)

Thanks for confirming! Considering this, our current approach won’t work as expected. We’ll need to find another solution, maybe we can store the ID of the latest payment method in Onyx and use that ID to determine which account shows the default badge.

thesahindia · 2024-11-06T17:39:17Z

@Shahidullah-Muffakir, any progress on the new solution?

Shahidullah-Muffakir · 2024-11-06T17:49:47Z

@Shahidullah-Muffakir, any progress on the new solution?

@thesahindia, will push the changes in few hours.

Shahidullah-Muffakir · 2024-11-06T20:57:47Z

@thesahindia Changes are complete. Let me know if anything else is needed, Thank you.

thesahindia · 2024-11-11T15:15:55Z

Looking into it!

thesahindia · 2024-11-11T21:51:21Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Screen.Recording.2024-11-12.at.3.06.18.AM.mov

Android: mWeb Chrome

iOS: Native

Screen.Recording.2024-11-11.at.10.44.23.PM.mov

iOS: mWeb Safari

Screen.Recording.2024-11-11.at.10.52.31.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-11-11.at.10.14.08.PM.mov

MacOS: Desktop

Screen.Recording.2024-11-12.at.1.07.57.AM.mov

thesahindia

Tests well!

NikkiWines

Sorry to do this, since I know there's been a fair amount of back and forth on this issue already... But after doing some digging, I think we actually need to set the default badge according to the bankAccountID saved in userWallet.walletLinkedAccountID, as that's the value we reference on the backend to determine the default account.

Additionally, some bankAccounts are not added via plaid, so relying on that value may b unreliable.

Sorry!! Let me know if that doesn't make sense

Shahidullah-Muffakir · 2024-11-12T22:24:45Z

Sorry to do this, since I know there's been a fair amount of back and forth on this issue already... But after doing some digging, I think we actually need to set the default badge according to the bankAccountID saved in userWallet.walletLinkedAccountID, as that's the value we reference on the backend to determine the default account.

Additionally, some bankAccounts are not added via plaid, so relying on that value may b unreliable.

Sorry!! Let me know if that doesn't make sense

No issue, I’ll make the update. Thanks for explaining! We’ll use bankAccountID || fundID since that’s what’s stored in walletLinkedAccountID here:

App/src/libs/actions/PaymentMethods.ts

Line 110 in 3023d60

walletLinkedAccountID: bankAccountID || fundID,

Shahidullah-Muffakir · 2024-11-12T22:37:18Z

Sorry to do this, since I know there's been a fair amount of back and forth on this issue already... But after doing some digging, I think we actually need to set the default badge according to the bankAccountID saved in userWallet.walletLinkedAccountID, as that's the value we reference on the backend to determine the default account.

Additionally, some bankAccounts are not added via plaid, so relying on that value may b unreliable.

Sorry!! Let me know if that doesn't make sense

@NikkiWines , Just to confirm, should the key be named LATEST_ADDED_WALLET_LINKED_ACCOUNT_ID, or do you prefer something else? Thanks!

/** The ID of the latest added wallet linked account */
   LATEST_ADDED_WALLET_LINKED_ACCOUNT_ID: 'latestAddedWalletLinkedAccountID',

NikkiWines · 2024-11-12T22:41:19Z

I think you could just use walletLinkedAccountID which is what we use elsewhere in the product, but you shouldn't need a new onyx key for this. You should be able to get the value from the USER_WALLET onyx key.

Shahidullah-Muffakir · 2024-11-13T00:56:30Z

I think you could just use walletLinkedAccountID which is what we use elsewhere in the product, but you shouldn't need a new onyx key for this. You should be able to get the value from the USER_WALLET onyx key.

Currently, when we add a new payment method with addPersonalBankAccount, the userWallet.walletLinkedAccountID is not updated. walletLinkedAccountID only gets updated in two cases:

when a user manually sets a payment method as default by clicking 'Make default payment method,'
or when the OpenPaymentsPage API runs (only triggered when the Wallet page opens).

Given the current issue, relying on these alone won’t work.

So, considering this, we have a couple of options:

When we add a new payment method via addPersonalBankAccount, the backend could save userWallet.walletLinkedAccountID as the latest added account if isDefault is true. This would be similar to how OpenPaymentsPage returns userWallet.walletLinkedAccountID . Then, we could use this to show the default.
we will still need to add the same changes, as I added in the last approach in the shouldShowDefaultBadge function.

Alternatively, a more straightforward approach would be to call the OpenPaymentsPage endpoint right after a user adds a new payment method. Since OpenPaymentsPage returns only one isDefault true.
We need to add bankAccountList in deps array of this useEffect:

App/src/pages/settings/Wallet/WalletPage/WalletPage.tsx

Lines 267 to 272 in 7256ad6

    
           useEffect(() => { 
        
               if (network.isOffline) { 
        
                   return; 
        
               } 
        
               PaymentMethods.openWalletPage(); 
        
           }, [network.isOffline]);

If two bank accounts have isDefault: true, we could show the default badge only for the account whose ID does not match walletLinkedAccountID, since that ID hasn't updated yet.
I think this third approach could work best.

Shahidullah-Muffakir · 2024-11-13T01:28:12Z

Additionally, some bankAccounts are not added via plaid, so relying on that value may b unreliable.

When I click on 'Add Bank Account,' it directly opens the Plaid modal, and I couldn’t find any other way to add a bank account. Could you clarify how bank accounts might be added without Plaid. Thank you.

NikkiWines · 2024-11-13T03:16:57Z

Currently, when we add a new payment method with addPersonalBankAccount, the userWallet.walletLinkedAccountID is not updated. walletLinkedAccountID only gets updated in two cases:

Oh, hmm. We do update it on the backend but the onyx key isn't appropriately updated currently. I can push up a change that will fix that.

When I click on 'Add Bank Account,' it directly opens the Plaid modal, and I couldn’t find any other way to add a bank account. Could you clarify how bank accounts might be added without Plaid. Thank you.

Users can add personal bank accouns without plaid via Expensify Classic.

Shahidullah-Muffakir · 2024-11-13T03:30:20Z

Oh, hmm. We do update it on the backend but the onyx key isn't appropriately updated currently. I can push up a change that will fix that.

Okay, that sounds great, this change should work then. Thanks

Users can add personal bank accouns without plaid via Expensify Classic.

Got it, thanks for clarifying.

NikkiWines · 2024-11-13T04:33:10Z

PR is up to resolve this, I'll update here when it's live but for now you can proceed as though we'll use the walletLinkedAccountID from the USER_WALLET key to determine the default

NikkiWines · 2024-11-13T17:01:35Z

Backend PR has been merged, should be on production by thursday

Shahidullah-Muffakir · 2024-11-13T17:06:00Z

Backend PR has been merged, should be on production by thursday

Thank you! I’ll test it with the backend changes on Thursday and push the updates afterward.

…r latest added payment method

Shahidullah-Muffakir · 2024-11-14T09:54:12Z

I added changes to use userWallet.walletLinkedAccountID.
Not sure if the failing test is related, as it seems unrelated to the changes made.

NikkiWines · 2024-11-14T16:18:37Z

Hmm yeah the failing test does seem unrelated, try merging main again @Shahidullah-Muffakir?

Shahidullah-Muffakir · 2024-11-14T19:26:41Z

Hmm yeah the failing test does seem unrelated, try merging main again @Shahidullah-Muffakir?

@NikkiWines, Thank you, Merging main fixed the issue.

NikkiWines · 2024-11-14T20:53:57Z

Backend changes are live! You should be getting the updated walletLinkedAccountID now

Shahidullah-Muffakir · 2024-11-14T21:00:02Z

Yes, I tested it with the new backend changes, and it’s working well. Thanks.

NikkiWines

Looks great, thanks for the perserverence here 🙇

OSBotify · 2024-11-14T21:36:47Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Ensure only latest bank account shows default badge when adding multi…

839e379

…ple accounts

Shahidullah-Muffakir requested a review from a team as a code owner October 24, 2024 19:04

melvin-bot bot requested review from thesahindia and removed request for a team October 24, 2024 19:04

Shahidullah-Muffakir added 4 commits October 25, 2024 00:41

Resolve linter errors by moving default parameters and replacing Bool…

5a2b5c5

…ean() with git status

fixed type errors

88e26ec

Use optional chaining and non-null assertions in payment method

5a63808

address linting issues

1551d76

Shahidullah-Muffakir mentioned this pull request Oct 24, 2024

[$250] Mweb/Chrome - Wallet - Adding 2 Payment methods, Both are shown as default #50829

Open

6 tasks

Shahidullah-Muffakir added 5 commits November 7, 2024 01:41

Store ID of latest added payment method in Onyx

9fd23d1

Merge branch 'main' of github.com:Expensify/App into fix/50829

92c399d

Fix linting issues

3dd07e9

Fix Prettier formatting issues

c18e9da

Fix linting issue

d2c4f01

thesahindia approved these changes Nov 11, 2024

View reviewed changes

melvin-bot bot requested a review from NikkiWines November 11, 2024 21:51

NikkiWines requested changes Nov 12, 2024

View reviewed changes

Merge branch 'main' of github.com:Expensify/App into fix/50829

e32242a

Shahidullah-Muffakir added 4 commits November 14, 2024 14:32

merge main into fix/50829

1e0ebb2

using userWallet.walletLinkedAccountID show the Default badge only fo…

65237d3

…r latest added payment method

ESLint: remove unnecessary 'else' after 'return'

66fbabf

Update comment for clarity

f529168

Merge branch 'main' of github.com:Expensify/App into fix/50829

c11ac70

NikkiWines approved these changes Nov 14, 2024

View reviewed changes

NikkiWines merged commit b0d785c into Expensify:main Nov 14, 2024
16 checks passed

Ensure only latest bank account shows default badge #51437

Ensure only latest bank account shows default badge #51437

Conversation

Shahidullah-Muffakir commented Oct 24, 2024

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Oct 24, 2024

thesahindia commented Oct 26, 2024

thesahindia commented Oct 26, 2024

Shahidullah-Muffakir commented Oct 26, 2024

thesahindia commented Oct 28, 2024 • edited Loading

thesahindia commented Oct 28, 2024

Shahidullah-Muffakir commented Oct 28, 2024

thesahindia commented Oct 29, 2024

NikkiWines commented Oct 29, 2024

Shahidullah-Muffakir commented Oct 29, 2024

Shahidullah-Muffakir commented Oct 31, 2024

NikkiWines commented Nov 1, 2024 • edited Loading

Shahidullah-Muffakir commented Nov 1, 2024

thesahindia commented Nov 6, 2024

Shahidullah-Muffakir commented Nov 6, 2024

Shahidullah-Muffakir commented Nov 6, 2024

thesahindia commented Nov 11, 2024

thesahindia commented Nov 11, 2024

Reviewer Checklist

Screenshots/Videos

thesahindia left a comment

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

Shahidullah-Muffakir commented Nov 12, 2024

Shahidullah-Muffakir commented Nov 12, 2024

NikkiWines commented Nov 12, 2024

Shahidullah-Muffakir commented Nov 13, 2024 • edited Loading

Shahidullah-Muffakir commented Nov 13, 2024

NikkiWines commented Nov 13, 2024

Shahidullah-Muffakir commented Nov 13, 2024

NikkiWines commented Nov 13, 2024

NikkiWines commented Nov 13, 2024

Shahidullah-Muffakir commented Nov 13, 2024

Shahidullah-Muffakir commented Nov 14, 2024

NikkiWines commented Nov 14, 2024

Shahidullah-Muffakir commented Nov 14, 2024

NikkiWines commented Nov 14, 2024

Shahidullah-Muffakir commented Nov 14, 2024

NikkiWines left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 14, 2024

thesahindia commented Oct 28, 2024 •

edited

Loading

NikkiWines commented Nov 1, 2024 •

edited

Loading

Shahidullah-Muffakir commented Nov 13, 2024 •

edited

Loading