[$250] BA - All - User is prompted to manually enter bank account and routing number after connecting account via Plaid

IuliiaHerets 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!

Version Number: 9.0.37-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: https://expensify.testrail.io/index.php?/tests/view/4977692 Email or phone of affected tester (no customers): applausetester+091724dr009@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Go to staging.new.expensify.com or open Expensify App
Log in with a applause.expensifail account
Go to existing workspace or create new one
Navigate to Workspace settings > More features > Workflows > Connect bank account
Select the "Connect with plaid" method to add a bank account
Select "Chase" bank, enter the credentials "user_good / pass_good" and follow the flow
Checkmark "Plaid Saving" account
After returning from Plaid to ND select the bank account ending in 1111 ~9. On page "Manually add your bank account" enter incorrect account - 1111222233334444~ ~10. Click/tap "Next"~

Expected Result:

After returning from Plaid to ND and selecting the bank account ending in 1111 (action 8 above), user should be taken to step 2 in the bank account setup flow.

Actual Result:

After returning from Plaid to ND and selecting the bank account ending in 1111, user is taken to "Step 1 - Manually add your bank account" and prompted to enter their account number and routing number manually.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/92046543-c68b-49e2-9acd-bdb328de5dbd

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021836806526771494788
Upwork Job ID: 1836806526771494788
Last Price Increase: 2024-10-31

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @sakluger (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.

IuliiaHerets commented 1 month ago

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

abzokhattab commented 1 month ago

Proposal

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

No error message is displayed when entering an incorrect account number in the connect with plaid bank flow.

What is the root cause of that problem?

In the validation function of the bank info step, we do not verify if the mask from the draft state matches the last 4 characters of the current input: https://github.com/Expensify/App/blob/e9b2107423491ff5ebc8bab620bd7255c88fd9dd/src/pages/ReimbursementAccount/BankInfo/substeps/Manual.tsx#L36-L56

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

We should use the draft state to retrieve the mask of the Plaid connection:

const [draftReimbursementAccount] = useOnyx(ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM_DRAFT);

Then, modify the invalid account number error condition to include a check if the mask doesn't match. Alternatively, we could create a new error message for this specific case:

if (
            values.accountNumber &&
            !CONST.BANK_ACCOUNT.REGEX.MASKED_US_ACCOUNT_NUMBER.test(values.accountNumber.trim()) &&
            (!CONST.BANK_ACCOUNT.REGEX.US_ACCOUNT_NUMBER.test(values.accountNumber.trim()) ||
                (draftReimbursementAccount && draftReimbursementAccount?.mask !== values.accountNumber.trim()?.slice(-4)))
        ) {
            errors.accountNumber = translate('bankAccount.error.accountNumber');

optionally, we could add shouldValidateOnChange={false} to the FormProvider so that the validation function only executes when the input is blurred, preventing users from getting annoyed by constant validation while typing

POC

https://github.com/user-attachments/assets/d9bbc70b-b83f-410f-863e-0f0fd86edc66

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

sakluger commented 1 month ago

Hey @allroundexperts, what do you think of the proposal from last week?

allroundexperts commented 1 month ago

Thanks for your proposal @abzokhattab. I think your proposal would work only if the last 4 characters are different. Isn't there a way to get the full bank account number?

allroundexperts commented 1 month ago

I'm also wondering on why does the user have to fill in the bank account number again, when he selected a bank account in the previous step. @sakluger @IuliiaHerets Maybe you know?

sakluger commented 1 month ago

I'm also wondering on why does the user have to fill in the bank account number again, when he selected a bank account in the previous step.

Good catch. They shouldn't have to do both - if Plaid connects successfully, then they shouldn't have to manually enter the account and routing numbers. Manual entry is just a fallback option if they can't connect via Plaid. I wonder if this is unique to Staging/Dev, since I know adding a bank account is different on staging & dev.

abzokhattab commented 1 month ago

as far as i see, all we get from plaid after entering the credentials is the following object:

so the account and the route numbers are empty, maybe that is why we are requested to enter them.

I think, the mask is the only relevant data in this case

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

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

sakluger commented 1 month ago

Right, we'll generally never return a full account number, only the last four digits for privacy/security reasons.

abzokhattab commented 1 month ago

Also one more thing we can do is

Given that the length of the bank account number is between 8 and 12 digits, we can restrict the validation of the last 4 digits to only be executed if the length is 8 or more

sakluger commented 1 month ago

I don't know if there's a minimum length for bank accounts - I feel like I've seen some pretty short bank account numbers.

melvin-bot[bot] commented 1 month ago

@sakluger, @allroundexperts 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

sakluger commented 1 month ago

I'm not 100% on next steps for this one. @allroundexperts is there anything I can do to help move this one forward?

melvin-bot[bot] commented 1 month ago

@sakluger @allroundexperts 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!

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? 💸

sakluger commented 1 month ago

@allroundexperts still not quite sure how to move this one forward. What do you think we should do here?

allroundexperts commented 1 month ago

Posted about this in slack here. Let's see if anyone knows why user is being asked to manually add there bank details.

sakluger commented 1 month ago

No updates.

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? 💸

sakluger commented 3 weeks ago

Still no updates.

melvin-bot[bot] commented 3 weeks ago

@sakluger @allroundexperts 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!

melvin-bot[bot] commented 3 weeks 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 3 weeks ago

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

sakluger commented 3 weeks ago

I'm pretty confident that users shouldn't have to manually enter account & routing number after connecting via Plaid, even in Staging. Once they connect via Plaid, we would pull their account & routing number from Plaid.

@allroundexperts what are next steps assuming that what I described above is the expected behavior?

allroundexperts commented 2 weeks ago

@allroundexperts what are next steps assuming that what I described above is the expected behavior?

@sakluger Given that's the case, we'd need to create a new issue for it, and investigate why its not pulling information from Plaid. We can put this one on hold till that is resolved.

sakluger commented 2 weeks ago

Can we just repurpose this issue for that investigation? I feel like this exising issue becomes irrelevant since the user would no longer be asked to enter an account and routing number after connecting via Plaid.

melvin-bot[bot] commented 2 weeks ago

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

sakluger commented 2 weeks ago

No updates.

melvin-bot[bot] commented 1 week ago

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

allroundexperts commented 1 week ago

Still awaiting proposals.

@sakluger Maybe we can ask someone from the expert agency to look at this?

abzokhattab commented 1 week ago

so the new requirement is to investigate why the user is being prompted to enter their account number, even though the connection to Plaid was successful, and to attempt to fix that.

i will check that tomorrow

melvin-bot[bot] commented 1 week ago

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

sakluger commented 1 week ago

Hey @abzokhattab, were you able to test based on the new requirement?

sakluger commented 1 week ago

I updated the GH issue title and the Expected Results and Actual Results in the OP.

I'm also labeling as #quality, but I also asked for confirmation in Slack that it's under the right project.

tgolen commented 1 week ago

I did some debugging on this today and put my findings in this slack thread.

tgolen commented 1 week ago

I got confirmation in that Slack thread from @nkuoch that this is working as intended and is not a bug. I'm going to close this out.

Expensify / App