-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Use invoiceTransferBankAccountID to determine default badge for a payment method on Invoices page #54619
Conversation
…n the invoice page
|
@jjcoffee PR is ready for your review. There are some ESLint errors, but they are not related to the changes made in this PR. Should we address them here, or handle them separately? |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-app-2025-01-14_15.55.34.mp4Android: mWeb Chromeandroid-chrome-2025-01-14_16.14.28.mp4iOS: Nativeios-app-2025-01-14_16.53.58.mp4iOS: mWeb Safariios-safari-2025-01-14_16.58.40.mp4MacOS: Chrome / Safaridesktop-chrome-2025-01-14_15.43.59.mp4MacOS: Desktopdesktop-app-2025-01-14_15.51.33.mp4 |
@Shahidullah-Muffakir Can you add a screen recording for Android native? |
@Shahidullah-Muffakir Should we add a test to for the wallet page too? I'm not sure where the discussion ended up, but I think the two pages aren't necessarily meant to be in-sync now, right? |
I'm facing some issues with the Android build. I'll upload the recording once it's resolved. |
Sounds good, I actually tested that as well, but I’ll add the tests for it in the PR.
Yes, you’re right, the two pages aren’t necessarily meant to be in-sync now. |
@Shahidullah-Muffakir I was thinking for the test steps more something like testing that the default doesn't match on the invoices pages if e.g. you manually set the first bank account as default on the invoices page. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@jjcoffee Since the BE doesn't update the default payment method when a payment method is deleted on the Invoices page (unlike on the Wallet page), we should still show the "Make the payment method default" option. This ensures that if the user deletes the default method, they can still set a new default for the remaining payment method. |
@Shahidullah-Muffakir This is inconsistent with the wallet page though and I think it makes sense that they should work the same way. I think the point is that the option should be hidden if there's only one bank account, it doesn't necessarily need to reflect whether or not the BE has updated the default payment method. cc @nkuoch in case you disagree. |
That's strange, I wasn't able to reproduce the issue. One thing to note: while testing, it seems we need to use one savings and one checking account. It looks like testing accounts from different banks (like Wells Fargo and Chase) have the same account number for the same type. Screen.20Recording.202025-01-14.20at.209.mp4 |
@Shahidullah-Muffakir Your video shows the behaviour I described - the newly added bank account is marked as default. |
@jjcoffee I see your point. It would be a bit different from the Wallet page, but if a payment method is not the default, why should we hide the "Make the payment method default" option for that payment method? Even if there's only one payment method, users should still have the option to make it the default if needed. |
Ah, then that’s the expected behavior. The newly added bank account should be the default one. @nkuoch confirmed this with the new BE changes. |
@Shahidullah-Muffakir Because it's inconsistent and may cause confusion for the user (why do they need to set a default if there's only one bank account?). It's also the expected result in the issue:
|
@jjcoffee Even in the wallet the newly added bank account is default |
@nkuoch based on the discussion from here, the newly added payment method should be set as the default The current behaviour:
Considering this, I guess if the user manually made a payment method default by using 'Make payment method default', then adding a new payment method shouldn't override it? |
Back end side, if a bank account is set as the default one, we can't know if it's because the user explicitly set it or because we did it when the new bank account was added. |
Oh okay, that makes sense and I don't think it's a big deal from a UX perspective, it's more the inconsistency between wallet/invoice. I do wonder how the wallet page is keeping the manually set default though? |
@nkuoch Also, sorry to bug you with so many things at once, but I feel like we've gone in circles a bit on this and wonder if you have any thoughts on this issue? Based on the issue's expected results, I would think we never show the |
On the invoice page, we do want to let the user specify the default explicitly (either by clicking on the default button, or by adding the bank account from the invoice page). So, if it's not the policy default one. No matter how many bank accounts we display, we want to display "Make default payment method". On the wallet page too, but the case shouldn't occur, as back end side when deleting the bank account, we auto set the default no matter what. |
It looks like |
I think we're nearly (finally!) done here, just to summarise where we're at:
|
@jjcoffee I've added the updated Tests and Android recording. |
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.
LGTM! Assuming my understanding here is correct, regarding the "bugs" I found.
I think we can probably leave the lint fixes as they weren't introduced by this PR, and it'd be good to just get this one finished.
@nkuoch looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
eslint errors weren't introduced by this PR |
🚀 Deployed to staging by https://github.com/nkuoch in version: 9.0.87-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.0.87-3 🚀
|
Explanation of Change
Default Badge on Invoices: Show it only if
invoiceTransferBankAccountID
exists for that policy.Set Default Payment Method: When adding a new payment method from invoices, send
policyID
andsource = 'invoice'
toAddPersonalBankAccount
to make it default for that policy.Fixed Issues
$ #53693
PROPOSAL: #53693 (comment)
Tests
Test1: (Invoices page)
Add the First Bank Account:
user_good
/pass_good
) and complete the process.Add a Second Bank Account:
Verify "Make default payment method" is Available with One Account:
Verify Defaults are Independent Between Pages:
Test2: (Wallet page)
Add the First Bank Account:
user_good
/pass_good
) and complete the process.Add a Second Bank Account:
Verify Defaults are Independent Between Pages:
Offline tests
Internet connection needed.
QA Steps
Test1: (Invoices page)
Add the First Bank Account:
user_good
/pass_good
) and complete the process.Add a Second Bank Account:
Verify "Make default payment method" is Available with One Account:
Verify Defaults are Independent Between Pages:
Test2: (Wallet page)
Add the First Bank Account:
user_good
/pass_good
) and complete the process.Add a Second Bank Account:
Verify Defaults are Independent Between Pages:
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
Screen.20Recording.202025-01-16.20at.202.mp4
Android: mWeb Chrome
android.20web-2.mp4
iOS: Native
ios.20native-3.mp4
iOS: mWeb Safari
ios.20safari-3.mp4
MacOS: Chrome / Safari
macosafari.mp4
MacOS: Desktop
Screen.20Recording.202025-01-14.20at.203.mp4