[Workspace Feeds] [$250] Card - "Account ending in" message appears after adding bank account for the card

m-natarajan commented 1 month 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!

Issue found when validating #45388 Version Number: 9.0.9-1 Reproducible in staging?: Y Reproducible in production?: N 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:

Navigate to https://staging.new.expensify.com/
Log in with a new expensifail account
Create a workspace and set default currency to USD
Navigate to Workspace settings - More features
Enable Expensify Card
Navigate to Expensify Card
Click on "Issue new card" button
Click on "Connect online with Plaid"
Complete BA flow with Regions bank
Click on "Issue new card" button
Expected Result:

The account holders name and the accounts last four number should be showing.

Actual Result:

"Account ending in" message appears after adding bank account for the card. You have to relog for the correct info to load.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/6ad8a5a7-6e4c-48ba-91ff-c29784f21626

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01622f31aad5cbea6e
Upwork Job ID: 1814111023366877998
Last Price Increase: 2024-07-26

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

m-natarajan commented 1 month ago

@kadiealexander FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

m-natarajan commented 1 month ago

We think that this bug might be related to #wave-collect - Release 2

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~01622f31aad5cbea6e

melvin-bot[bot] commented 1 month ago

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

nyomanjyotisa commented 1 month ago

Proposal

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

"Account ending in" message appears after adding bank account for the card

What is the root cause of that problem?

After adding new bank account from Expensify Card page, the bankAccounts onyx data given by the API not formatted properly and we didn't fetch the properly formatted bankAccounts onyx data as we do in the wallet page

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

Add the following code after AcceptACHContractForBankAccount API Write here to make sure we got the properly formatted bankAccounts onyx data after adding new bank account from Expensify Card page

PaymentMethods.openWalletPage();

OR

API.read(READ_COMMANDS.OPEN_PAYMENTS_PAGE, null);

And need to update here to properly get the right bankName and bankAccountOwnerName, to make sure we displayed the bank icon

        return eligibleBankAccounts.map((bankAccount) => {
            const bankName = bankAccount.accountData?.additionalData?.bankName;
            const bankAccountOwnerName = bankAccount.accountData?.addressName;
            const formattedBankAccountNumber = bankAccount.accountData?.accountNumber
                ? `${translate('workspace.expensifyCard.accountEndingIn')} ${bankAccount.accountData?.accountNumber.slice(-4)}`
                : '';

            const {icon, iconSize, iconStyles} = getBankIcon({bankName, styles});

            return (
                <MenuItem
                    title={bankAccountOwnerName}
                    description={formattedBankAccountNumber}
                    onPress={handleSelectBankAccount}
                    icon={icon}
                    iconHeight={iconSize}
                    iconWidth={iconSize}
                    iconStyles={iconStyles}
                    shouldShowRightIcon
                    displayInDefaultIconColor
                />
            );
        });

What alternative solutions did you explore? (Optional)

Fix the AcceptACHContractForBankAccount API response on BE to return the properly formatted bankAccountList onyx data
And also need to do the same as main solution to display the bank icon properly

RESULT

VasylMartyniv commented 1 month ago

Proposal

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

Account details not being correctly displayed after adding new account. The account's data is not being passed properly to the item in a list.

What is the root cause of that problem?

The eligibleBankAccounts mapping function that maps bankAccountList to menu items, is trying to use some values from bankAccount.accountData which are not present in that object after adding the account.

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

For displaying the data in menu item we can use the values from bankAccountList item iteself. So the proposed changes are next:

For the display of Name and Account number use the data from bankAccount object, and not from bankAccount.accountData.
- Use bankAccount.description.slice(-4) for the bankAccountNumber variable
- Use bankAccount.title for the Account Holder name in the menu item
Because the bankName property is present directly in the accountData object after adding a new account, we can add a conditional that will use the bankName variable from accountData.bankName, and if that is missing then use a value from accountData.additionalData.bankName.
Adjust "AccountData" type, as the property bankName can be present both in AccountData type or the BankAccountAdditionalData type, depending on the response that provides bankAccountList.

All of the above variables are already accessible in bankAccountList items after adding a new account, so no additional fetching needed.

What alternative solutions did you explore? (Optional)

Additional fetching of the missing data after adding new card
Adjusting the back-end responses to have a consistent bankAccount object structure that they return.

Result of proposed changes:

melvin-bot[bot] commented 1 month ago

📣 @VasylMartyniv! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

VasylMartyniv commented 1 month ago

Contributor details Your Expensify account email: vasyl.martyniv@perfsol.tech Upwork Profile Link: https://www.upwork.com/freelancers/~018d00e8b30180eee9

melvin-bot[bot] commented 1 month ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

s77rt commented 1 month ago

@nyomanjyotisa

the bankAccounts onyx data given by the API not formatted properly

Can you elaborate how not formatted? (include returned data vs expected data)

s77rt commented 1 month ago

@VasylMartyniv

trying to use some values from bankAccount.accountData which are not present in that object after adding the account

Why do we have missing fields? And are the rest of the fields returned by the backend or populated optimistically?

melvin-bot[bot] commented 1 month ago

@s77rt, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

s77rt commented 1 month ago

Waiting on proposals

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@s77rt, @kadiealexander Eep! 4 days overdue now. Issues have feelings too...

s77rt commented 1 month ago

Same ^

koko57 commented 1 month ago

@s77rt I am still working on Expensify Card (Workspace Feed) project and I was working on the adding bank account logic. Maybe I could work on that?

s77rt commented 1 month ago

🎀 👀 🎀 Please assign @koko57

melvin-bot[bot] commented 1 month ago

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

iwiznia commented 1 month ago

Shouldn't there be a proposal?

s77rt commented 1 month ago

@iwiznia agency-employees are assigned immediately then a proposal is submitted

melvin-bot[bot] commented 1 month ago

@iwiznia @s77rt @kadiealexander this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

s77rt commented 4 weeks ago

Not overdue. Waiting on proposal

koko57 commented 4 weeks ago

I will be working on this today

koko57 commented 4 weeks ago

I'm trying to reproduce the bug, but even with providing the data to skip the Verification wit Onfido, or trying to verify I can't get my account added to the bankAccount list. But I agree with previous proposals - it would be good to check why the bankAccount is not returned after adding it the same way the same bank account is then returned with openWalletPage

melvin-bot[bot] commented 3 weeks ago

@iwiznia, @s77rt, @koko57, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

s77rt commented 3 weeks ago

No update yet. Still having issues reproducing this due to verification flow

koko57 commented 3 weeks ago

I will investingate it today

koko57 commented 3 weeks ago

posted my findings here: https://github.com/Expensify/App/issues/47239#issuecomment-2286312613

melvin-bot[bot] commented 3 weeks ago

@iwiznia, @s77rt, @koko57, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

s77rt commented 3 weeks ago

Not overdue. Waiting on https://github.com/Expensify/App/issues/47239

melvin-bot[bot] commented 2 weeks ago

@iwiznia @s77rt @koko57 @kadiealexander this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

s77rt commented 2 weeks ago

Same ^

melvin-bot[bot] commented 2 weeks ago

@iwiznia, @s77rt, @koko57, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

s77rt commented 2 weeks ago

Same ^

koko57 commented 2 weeks ago

@s77rt more explaination here: https://github.com/Expensify/App/issues/47239#issuecomment-2298836350

The issue with the data not being sent the same way is solved, but we still need to make one fix here. We need to change bankAccount.accountData?.accountNumber for bankAccount.accountData?.additionalData?.mask

https://github.com/Expensify/App/blob/a32da40e1017d2136389f05735bace7bc307e098/src/pages/workspace/expensifyCard/WorkspaceExpensifyCardBankAccounts.tsx#L51

and use the mask without additionally using getLastFourDigits

https://github.com/Expensify/App/blob/a32da40e1017d2136389f05735bace7bc307e098/src/pages/workspace/expensifyCard/WorkspaceExpensifyCardBankAccounts.tsx#L59

s77rt commented 2 weeks ago

@koko57 Thanks! Let's wait for @mountiny on this one, for consistency I think we may want the accountNumber

koko57 commented 1 week ago

@mountiny waiting for your decision here 🙂 Personally I would go with changing this to mask regardless if the accountNumber is added or not, because we could get rid of getLastFourDigits calculation and mask is something that should be returned for every card

mountiny commented 1 week ago

We should work on unifying how we send the data from those two commands, I created a task to do that here https://github.com/Expensify/App/issues/47944

I got a quick PR to add the masked accountNumber to the response here too so we can keep the behaviour unified.

koko57 commented 1 week ago

@mountiny should I make this change with mask that I proposed anyway? or are we going with just the backend fix?

mountiny commented 1 week ago

Lets just wait for the backend fix so we do not handle this differently in two places

melvin-bot[bot] commented 1 week ago

@iwiznia, @s77rt, @koko57, @mountiny, @kadiealexander Huh... This is 4 days overdue. Who can take care of this?

mountiny commented 1 week ago

@koko57 the fix is deployed, can you retest it?

koko57 commented 6 days ago

the same as in https://github.com/Expensify/App/issues/47239#issuecomment-2312597635

kadiealexander commented 6 days ago

Not overdue

mountiny commented 6 days ago

Follow up backend PR merged

kadiealexander commented 1 day ago

@mountiny is there anything else left to complete on this issue?

mountiny commented 1 day ago

@koko57 We had to make one small backend change and that should have been deployed. Can you please confirm if there is anything left to do for this one? Thank you 🙇

koko57 commented 1 day ago

it's working now - ending in number is displayed

cc @mountiny

Expensify / App