[HOLD for payment 2024-03-26] [$500] BA - Not able to add Regions BA, modal redirects to initial Bank account page

izarutskaya commented 2 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: v1.4.50-2 Reproducible in staging?: Y Reproducible in production?: Unable to check If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4413762 Email or phone of affected tester (no customers): applausetester+vd_mweb031124@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Pre-requisite: user must be logged in on mWeb/Safari (iPhone) and must have created a Workspace.

Go to Workspace > Bank account.
Select Connect online with Plaid.
Select Regions bank and use the testing credentials.

Expected Result:

The page to confirm the account should be displayed.

Actual Result:

The modal redirects to the initial add bank account page.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/5dc83ec7-74ff-4f7f-886c-c99a656f869c

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01a531e07d9e5f1c52
Upwork Job ID: 1767269083257348096
Last Price Increase: 2024-03-11
Automatic offers:

shubham1206agra | Reviewer | 0
abzokhattab | Contributor | 0

melvin-bot[bot] commented 2 months ago

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

github-actions[bot] commented 2 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @chiragsalian (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

izarutskaya commented 2 months ago

@chiragsalian 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.

dylanexpensify commented 2 months ago

commented here

chiragsalian commented 2 months ago

I'm pretty sure this is external since it's happening only on mWeb. Assigning external label.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

abzokhattab commented 2 months ago

Proposal

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

Not able to add Regions BA, modal redirects to initial Bank account page

What is the root cause of that problem?

The PlaidLink opens the connection multiple times because of the following useEffect, since the onError is a function that doesn't have dependencies causing it to rerender: https://github.com/Expensify/App/blob/8aec07c55df3f0fefc6bed76c991183cf6b97f7a/src/components/PlaidLink/index.tsx#L39-L54

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

there is no need to add the onError to the dependency list of the useEffect since we already have the error object inside the dependencies list so it should call the useEffect in case the error changes:

    // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [ready, error, isPlaidLoaded, open]);

this way the Plaid component behaviour will be consistent and the useEffect will not depend on whether the incoming onError prop has a dependencies list or not

also, we can remove the open function from the dependencies as well since we don't know what the dependencies of this function but this step is optional in solving our issue

POC

https://github.com/Expensify/App/assets/59809993/33c90dcb-aeee-4db6-8c89-711796318853

shubham1206agra commented 2 months ago

@abzokhattab Can you mark the offending PR as this is deploy blocker?

abzokhattab commented 2 months ago

Does that mean its not occurring on production? As far as i can see we were not able to test it on prod

shubham1206agra commented 2 months ago

Yes, as per https://expensify.slack.com/archives/C01GTK53T8Q/p1710187880234419?thread_ts=1710182030.177439&cid=C01GTK53T8Q

abzokhattab commented 2 months ago

Thanks, honestly I am not able to find it i just checked the differences between staging and production and I see that all the changes are not related to the bank setup.

https://github.com/Expensify/App/compare/production...staging

let me know if you are able to find any

shubham1206agra commented 2 months ago

Can you confirm the repro of the issue on staging? And on https://36676.pr-testing.expensify.com/

abzokhattab commented 2 months ago

Yes, the issue is reproducible on both:

https://github.com/Expensify/App/assets/59809993/b3f559fc-fb67-42d5-973c-e85f71bcd771

https://github.com/Expensify/App/assets/59809993/e332d60c-dd1b-4bce-b771-da4c75abbf8a

shubham1206agra commented 2 months ago

Lets go with @abzokhattab's proposal.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

chiragsalian commented 2 months ago

Proposal LGTM. Feel free to create the PR for this @abzokhattab. Try to prioritize this since we'll have to try to get it reviewed/merged and CP'd soon since the issue is a blocker.

melvin-bot[bot] commented 2 months ago

📣 @shubham1206agra 🎉 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 2 months ago

📣 @abzokhattab 🎉 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 📖

abzokhattab commented 2 months ago

Thanks!! the PR is ready.

luacmartins commented 2 months ago

We're demoting this to NAB since we could reproduce the same issue on this adboc build that was deployed to prod 5 days ago. We think this is a Plaid issue on their sandbox API.

bernhardoj commented 2 months ago

Most likely related to https://github.com/Expensify/App/issues/32356

shubham1206agra commented 2 months ago

Would you like to post a proposal here?

bernhardoj commented 2 months ago

I already have a proposal there

shubham1206agra commented 2 months ago

@bernhardoj Please create a test branch to confirm the proposal fixes this issue too. Thanks

dylanexpensify commented 2 months ago

Moving along!

abzokhattab commented 2 months ago

Proposal is updated

shubham1206agra commented 2 months ago

@dylanexpensify Can you unassign @abzokhattab here and reopen this for proposals?

bernhardoj commented 2 months ago

@shubham1206agra I don't know if it's the same issue or not because, in https://github.com/Expensify/App/issues/32356, the connect option page is shown for a few seconds before showing the correct step,

but in the OP video, looks like the correct step is never shown and I can't reproduce that.

You can test it here: https://github.com/bernhardoj/Expensify/tree/fix/plaid-reopening-on-rerender

Swor71 commented 2 months ago

Hey, I've been looking into this issue as it relates to Callstack's recent VBBA refactor to see if it's a regression issue.

It seems the problem is with the unstable ref for PlaidLink’s onError prop and actually @bernhardoj's suggestion here describes the problem well and it was posted way back in December last year so kudos to him. In my testing this solves the issue (tried both, Regions and CitiBank)

cc @dylanexpensify @shubham1206agra @luacmartins

shubham1206agra commented 2 months ago

Can confirm, https://github.com/bernhardoj/Expensify/tree/fix/plaid-reopening-on-rerender fixed the issue.

@chiragsalian @luacmartins Can we assign @bernhardoj to fix the issue here as other PR has no activity for quite some time?

luacmartins commented 2 months ago

That sounds good to me, but I'll let @chiragsalian handle this since he's the assigned engineer on this issue

chiragsalian commented 2 months ago

Yup that sounds good to me. Feel free to create a PR @bernhardoj.

melvin-bot[bot] commented 2 months ago

❌ There was an error making the offer to @bernhardoj for the Contributor role. The BZ member will need to manually hire the contributor.

bernhardoj commented 2 months ago

PR is ready

cc: @shubham1206agra

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.54-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/38330

If no regressions arise, payment will be issued on 2024-03-26. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@bernhardoj requires payment (Needs manual offer from BZ)
@shubham1206agra requires payment automatic offer (Reviewer)

melvin-bot[bot] commented 1 month ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@shubham1206agra] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@shubham1206agra] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@shubham1206agra] Determine if we should create a regression test for this bug.
[ ] [@shubham1206agra] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

dylanexpensify commented 1 month ago

Payment coming up!

dylanexpensify commented 1 month ago

paying out today!

shubham1206agra commented 1 month ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[x] [@shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:https://github.com/Expensify/App/pull/2746
[x] [@shubham1206agra] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/2746/files#r1540955125
[x] [@shubham1206agra] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not required
[x] [@shubham1206agra] Determine if we should create a regression test for this bug. No
[x] [@shubham1206agra] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

shubham1206agra commented 1 month ago

@dylanexpensify I have discussed this internally here. You may close this issue when you issue the payment to other contributor as I am keeping track of payment internally and will ask to pay once the issue is resolved. Just write in the payment summary that I have not been paid yet.

dylanexpensify commented 1 month ago

Payment summary:

Contributor: @bernhardoj $500
Contributor+: @shubham1206agra $500

Please apply/request!

dylanexpensify commented 1 month ago

@bernhardoj please accept offer when ready and I'll pay it out!

bernhardoj commented 1 month ago

@dylanexpensify the offer is broken.

dylanexpensify commented 1 month ago

oh weird! Let me send a new one then!

dylanexpensify commented 1 month ago

@bernhardoj sent an invite to apply!

bernhardoj commented 1 month ago

@dylanexpensify applied

dylanexpensify commented 1 month ago

@bernhardoj sent!

Expensify / App