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

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

1. Open Expensify App
2. Log in
3. Navigate to Settings - Wallet
4. Add a bank account for example Chasebank
5. 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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6347080_1705613704650.mobizen_20240118_220604.mp4

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]

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

[melvin-bot]

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

[melvin-bot]

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

[brunovjk]

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]

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:

App/src/pages/settings/Wallet/PaymentMethodList.js

Line 158 in 2a03320

function shouldShowDefaultBadge(filteredPaymentMethods, isDefault = false) {

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

Screen.Recording.2024-01-19.at.16.36.32.mov

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

Screen.Recording.2024-01-19.at.16.57.15.mov

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]

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

[ikevin127]

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

[MitchExpensify]

I get this no matter what bank I try with

[ikevin127]

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]

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

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]

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

[stitesExpensify]

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]

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

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]

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]

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]

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

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

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

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

[ikevin127]

Thanks for the review! 🚀

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

[s77rt]

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

[brunovjk]

ok :D thanks

[melvin-bot]

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

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

Issue not reproducible during KI retests. (First week)

[brunovjk]

PR Ready for Review

[melvin-bot]

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

[melvin-bot]

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]

How is the PR review coming along, @s77rt ?

[s77rt]

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

[brunovjk]

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

[MitchExpensify]

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

[s77rt]

@MitchExpensify No, no payment is due here

[brunovjk]

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]

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

[MitchExpensify]

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