lanitochka17 commented 8 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.27-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Open Expensify App
Log in
Navigate to Settings - Wallet
Add a bank account for example Chasebank
Repeat step 4 with another bank account for example Fidelity

Expected Result:

Only one Default tagged bank account should exist

Actual Result:

Default tagged 2 bank account exist after adding the second bank account unless navigate to other page and return

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/a41bce4e-72ba-43f4-b65e-f67e050cf87a

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01df86e2c207d235f0
Upwork Job ID: 1748142042333646848
Last Price Increase: 2024-01-19
Automatic offers:

s77rt | Reviewer | 28129024
brunovjk | Contributor | 28129025

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 8 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01df86e2c207d235f0

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

brunovjk commented 7 months ago

I think this is a backend issue. When add a bank account, isDeault is always true:

But seem to don't have a Pusher to return the updated values of isDeault, like we get when we navigate to other page and return

ikevin127 commented 7 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue

2 bank accounts with Default badge can be seen after adding the second bank account until the user navigates away then opens the Wallet page again or performs refresh on web.

What is the root cause of that problem?

When we add the first account (eg. Plaid Saving) this has the isDefault: true but it is not shown in the UI because of this function:

https://github.com/Expensify/App/blob/2a03320a054f0d9a28cffc1ee96fdde78700218a/src/pages/settings/Wallet/PaymentMethodList.js#L158

which filters the paymentMethods based on if there's any method with accountType bankAccount or debitCard then returns true if the lenght of the filtered array is > 1. This means that after we add the first account (eg. Plaid Saving) even though it has isDefault: true the badge won't be shown.

When we add the second account (eg. Plaid Checking), this will also be added with isDefault: true and because of the function mentioned above, both of them will display the Default badge since both have isDefault: true and now the filtered array length will be > 1 when called for each item.

What changes do you think we should make in order to solve the problem?

Within PaymentMethodList.js we have the filteredPaymentMethods variable which contains the array of payment methods we list. Within this function we have the PaymentUtils.formatPaymentMethods utils function which takes both bankAccount and filteredCardList and returns the combinedPaymentMethods.

Add the following changes to the PaymentUtils.formatPaymentMethods utils function:

find the first payment method by created date (when it was added) using

const firstMethodByDate = _.min(bankAccountList, (method) => new Date(method.accountData?.created ?? ''));

if the payment method is found, alter the isDefault to only be true for that payment method using

combinedPaymentMethods.push({
    ...bankAccount,
    ...(typeof firstMethodByDate !== 'number' && {isDefault: bankAccount.methodID === firstMethodByDate.methodID}),
    // rest of the code

Implementing this change front-end wise ensures that when we add the second account, only one will have the Default badge and that one is the same one that will have the Default badge after Wallet re-fetch.

Videos

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/56457735/6cbade7d-c761-49a5-ae02-00362c929b12

I also found some redundant code within `WalletPage.js` while debugging this issue:

    useEffect(() => {
        PaymentMethods.openWalletPage();
    }, []);

    // and lower in the component

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

and because we have both of them, when we navigate to the Wallet the 'OpenPaymentsPage' is called twice.

OpenPaymentsPage being called twice

https://github.com/Expensify/App/assets/56457735/420a6640-df00-49b0-8149-2a2ad158ec0b

The first useEffect can be removed safely since the second one will do the job just fine when online.

What alternative solutions did you explore? (Optional)

The alternative solution for this issue would be to trigger a pusher event when 'AddPersonalBankAccount' endpoint is called that will send the array with the correctly assigned isDefault: true as it happens when the Wallet is re-fetched by navigating away then returning to Wallet or by refreshing on web.

MitchExpensify commented 7 months ago

Hmm I'm running into a "Missing translation" error when choosing the same Fidelity test account. Did you reproduce successfully @ikevin127 ?

ikevin127 commented 7 months ago

@MitchExpensify Yes I did, both on staging and local dev w/ staging api sandbox toggled on from Preferences.

It works w/ any account (I used gmail) using the sandbox credentials. Fidelity can be replaced w/ City Bank or any other.

If OP video is followed you'll get consistent reproduction of the issue.

MitchExpensify commented 7 months ago

IMG_1209B675D831-1

MitchExpensify commented 7 months ago

I get this no matter what bank I try with

ikevin127 commented 7 months ago

That's weird - most likely it's a generic BE error that doesn't have translation. Try to reproduce on web maybe you'll have more luck - that's where I did it.

s77rt commented 7 months ago

@MitchExpensify The missing translation message is something expected for now, it only occurs for expensify.com domain users. But it will be fixed (ref: https://expensify.slack.com/archives/C049HHMV9SM/p1705523289822429?thread_ts=1705435804.140879&cid=C049HHMV9SM)

s77rt commented 7 months ago

This is a backend issue. Currently the BE always sets the default to the last added bank. If we add 2 banks, first request's response will have isDefault since it's the only one and second request's response will also have isDefault since it's the last added one. I don't know if this is intended, if it's not we can just set isDefault only for the first bank.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @stitesExpensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

stitesExpensify commented 7 months ago

I think we should be using the server as the source of truth here. If the bank account comes back as the isDefault then all other bank account should not be the default. So I think the solution is to set isDefault to false for all other bank accounts in onyx if our response has isDefault as true.

The only solution from the backend would be to always return all bank accounts which seems pretty inefficient

s77rt commented 7 months ago

@stitesExpensify Why is the server returning isDefault = true for newly added banks. We can have the server check if the user already have a default account then we should not set isDefault for the new one. That shouldn't be much work.

brunovjk commented 7 months ago

Hi @s77rt and @stitesExpensify, I understand your concerns. I've wrote a proposal "to set isDefault to false for all other bank accounts in onyx if our response has isDefault as true." I'd love to hear your thoughts on this. Im still woudering about a Pusher, I dont know if make sense here.

brunovjk commented 7 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Presence of two default tagged bank accounts after adding the second one in the Wallet section of the Expensify App.

What is the root cause of that problem?

The root cause of the problem lies in the backend implementation. Currently, when adding a bank account, the backend always sets the default to the last added bank. This leads to discrepancies in the default tagging of bank accounts.

What changes do you think we should make in order to solve the problem?

To address this issue, we propose modifying the AddPersonalBankAccountPage and addPersonalBankAccount to update the onyx store by setting isDefault to false for all other bank accounts if the response from the server indicates that the added bank account should be the default one.

Update AddPersonalBankAccountPage to fetch a bankAccountList from the onyx and pass it here, something like:
```
BankAccounts.addPersonalBankAccount(selectedPlaidBankAccount, bankAccountList);
```

Update BankAccounts.addPersonalBankAccount to update the onyx, something like:

        // ... Existing optimistic data
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.BANK_ACCOUNT_LIST,
            value: {
                ..._.map(bankAccountList, () => ({ isDefault: false })),
               [parameters.plaidAccountID]: { isDefault: true },
            },
        },

What alternative solutions did you explore? (Optional)

stitesExpensify commented 7 months ago

Why is the server returning isDefault = true for newly added banks

This is intended behavior and has been this way for many years :)

The idea is that if you're adding an entirely new bank account, you probably switched banks and want to use the new one for payments/transfers

s77rt commented 7 months ago

@ikevin127 Thanks for the proposal. Your RCA is correct. However the solution is s a workaround as Onyx will still have isDefault set to true for both accounts.

s77rt commented 7 months ago

@brunovjk Thanks for the proposal. Your RCA is correct. Overall the solution looks good to me.

🎀 👀 🎀 C+ reviewed Link to proposal

melvin-bot[bot] commented 7 months ago

Current assignee @stitesExpensify is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

ikevin127 commented 7 months ago

Thanks for the review! 🚀

@brunovjk gg, make sure to handle the double OpenPaymentsPage call mentioned in my proposal 👍

brunovjk commented 7 months ago

@s77rt Thanks for the review, I expect to have the PR ready for review by 02/01, we can discuss further there.

@ikevin127 Nice catch, should we create another PR for it?

ikevin127 commented 7 months ago

I don't see any issue including it in your PR since I was going to do the same♻️

As long as it's cool w/ the team handling this issue!🚀

s77rt commented 7 months ago

@brunovjk Please don't raise a PR until being assigned

brunovjk commented 7 months ago

ok :D thanks

melvin-bot[bot] commented 7 months ago

📣 @s77rt 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 7 months ago

📣 @brunovjk 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

mvtglobally commented 7 months ago

Issue not reproducible during KI retests. (First week)

brunovjk commented 7 months ago

PR Ready for Review

melvin-bot[bot] commented 6 months ago

Current assignee @s77rt is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 6 months ago

This issue has not been updated in over 15 days. @s77rt, @MitchExpensify, @stitesExpensify, @brunovjk eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

MitchExpensify commented 6 months ago

How is the PR review coming along, @s77rt ?

s77rt commented 6 months ago

@MitchExpensify The PR is closed. This requires a backend fix. @stitesExpensify will be working on this.

brunovjk commented 6 months ago

PR closed since we are going with a backend solution. It will also fix another issue

MitchExpensify commented 6 months ago

Ok thanks, and is there any payment due here from your perspective @s77rt ?

s77rt commented 6 months ago

@MitchExpensify No, no payment is due here

brunovjk commented 5 months ago

I can still reproduce this issue in staging.

Sorry for the inconvenience @stitesExpensify but how are we with the solution on the backend? The issue similar to this was closed after an internal discussion in Slack, I don't have access, did you discuss anything about this issue here as well? If not, I wonder if it would be a good idea to continue working with @s77rt to implement a solution in FE. Thank you very much.

stitesExpensify commented 5 months ago

Yes. Just like that issue, unfortunately I don't think that this can be prioritized right now. Do you agree @MitchExpensify?

MitchExpensify commented 4 months ago

Agree this isn't important to fix, happy to close

Expensify / App

[$500] Wallet - Two default tagged bank account exist after adding the second one #34774

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

Please re-state the problem that we are trying to solve in this issue

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Videos

I also found some redundant code within `WalletPage.js` while debugging this issue:

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

Expensify / App

[$500] Wallet - Two default tagged bank account exist after adding the second one #34774

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

Please re-state the problem that we are trying to solve in this issue

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Videos

I also found some redundant code within WalletPage.js while debugging this issue:

What alternative solutions did you explore? (Optional)

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

What alternative solutions did you explore? (Optional)

I also found some redundant code within `WalletPage.js` while debugging this issue: