-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Ensure only latest bank account shows default badge #51437
Conversation
@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] |
I will review it in a few hours! |
When I am adding the second account I am getting an error. |
@thesahindia, I tested now, I could add the 2nd account. |
I did the same. Trying again. |
Screen.Recording.2024-10-28.at.6.46.24.PM.movWhen 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. |
I think @NikkiWines might be able to answer this. |
|
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. |
I noticed something during testing:
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. |
Sorry for the delay, I can confirm that when re-adding a deleted account, the original |
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. |
@Shahidullah-Muffakir, any progress on the new solution? |
@thesahindia, will push the changes in few hours. |
@thesahindia Changes are complete. Let me know if anything else is needed, Thank you. |
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.
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 App/src/libs/actions/PaymentMethods.ts Line 110 in 3023d60
|
@NikkiWines , Just to confirm, should the key be named
|
I think you could just use |
Currently, when we add a new payment method with
Given the current issue, relying on these alone won’t work. So, considering this, we have a couple of options:
|
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. |
Okay, that sounds great, this change should work then. Thanks
Got it, thanks for clarifying. |
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 |
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. |
I added changes to use |
Hmm yeah the failing test does seem unrelated, try merging main again @Shahidullah-Muffakir? |
@NikkiWines, Thank you, Merging main fixed the issue. |
Backend changes are live! You should be getting the updated |
Yes, I tested it with the new backend changes, and it’s working well. Thanks. |
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.
Looks great, thanks for the perserverence here 🙇
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/NikkiWines in version: 9.0.63-1 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.0.63-3 🚀
|
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
Offline tests
This feature requires an active internet connection.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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