-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[$500] mWeb - "Connect online with plaid" page is shown first instead of "connect Bank account" #32356
Comments
Triggered auto assignment to @MitchExpensify ( |
Job added to Upwork: https://www.upwork.com/jobs/~012a029613cdb55a03 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.After user complete the plaid setup, the connect option page is shown briefly before the bank account selection page is shown. What is the root cause of that problem?When we select App/src/pages/ReimbursementAccount/BankAccountStep.js Lines 81 to 189 in 48b5a29
After we complete the plaid setup, the plaid is closed and
Based on the plaid doc, App/src/components/AddPlaidBankAccount.js Lines 222 to 224 in 48b5a29
So, there is something wrong with our code. After doing further investigation, we tried to open the plaid multiple times unintended in a App/src/components/PlaidLink/index.tsx Lines 33 to 48 in 54f3daa
Why App/src/components/AddPlaidBankAccount.js Lines 204 to 206 in 54f3daa
What changes do you think we should make in order to solve the problem?Use
|
ProposalPlease re-state the problem that we are trying to solve in this issue.After submitting bank account details, the "Connect Bank Account" options page is shown first, before select bank account page. This can be seen on web too. What is the root cause of that problem?This happens because the AddPlaidBankAccount here is rerendered and exited right after submission of valid bank credentials. As can be seen in the last lines of the log: The process should stop after the But there is another OPEN event and subsequent The This extra render is caused by the non-memoized What changes do you think we should make in order to solve the problem?We should memoise both functions using const onExitPlaid = useCallback(() => {
BankAccounts.setBankAccountSubStep(null);
}, []); const onSelect = useCallback((plaidAccountID) => {
ReimbursementAccount.updateReimbursementAccountDraft({plaidAccountID});
}, []) Then use these functions in the component: onSelect={onSelect}
onExitPlaid={onExitPlaid} Result:Before Changes: Before.Changes.MacOS.Chrome.movAfter Changes: Plaid.mov |
@eVoloshchak, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Lets get this fixed! |
Triggered auto assignment to @grgia ( |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@eVoloshchak, @MitchExpensify, @grgia Eep! 4 days overdue now. Issues have feelings too... |
How do these proposals look to you @eVoloshchak ? |
@neonbhai proposal: Screen.Recording.2023-12-13.at.16.12.06.mov@bernhardoj's proposal does resolve the issue and the general approach is better, since we're fixing it in one place for the whole app. However, I think we need to dig a bit deeper to figure out exactly why this happens. Is this a problem with our codebase or something that should be addressed in |
@eVoloshchak I did see some issues on the report about the iframe is not destroyed, but I think our issue here is different. The iframe is properly destroyed. The issue we have is the I can't find the plaid link core source code, so my guess is that because we have n |
Still on hold for #30442 |
of HOLD |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@dylanexpensify Is this wave 5 worthy? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Checking in with @dylanexpensify to see if wave 5 worthy, not sure how this might fit into the quarterly release plan but that seems like a good first step |
Seems like a LOW wave collect issue, what d'you reckon @trjExpensify ? |
It would go in the
|
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Can't reproduce on Android using Browserstack - @lanitochka17 can you please let me know if you are still seeing this on Android? mWeb on Browsterstack is also not playing nice for me. Can't log in:
Web is proving weird for me too - I'm getting booted from the VBA flow altogether on Safari v1.4.49-4 VBA.bugging.out.movSame on Chrome v1.4.49-4 VBA.bugging.out.Chrome.mov@lanitochka17 Can you let me know what you are seeing when trying to take the reproduction steps on web too please? |
Here is how it looks for me, the issue still persists. The plaid page is flashing and the connect step is shown for a few seconds. Screen.Recording.2024-03-11.at.13.17.50.mov
Maybe try re-login to have fresh data. |
Posting my proposal from #38069 (comment) here as well: ProposalPlease 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 App/src/components/PlaidLink/index.tsx Lines 39 to 54 in 8aec07c
What changes do you think we should make in order to solve the problem?there is no need to add the // eslint-disable-next-line react-hooks/exhaustive-deps
}, [ready, error, isPlaidLoaded, open]); this way the also, we can remove the POCScreen.Recording.2024-03-13.at.12.56.53.PM.mov |
Oh wait.. is this issue a dupe of #38069 (comment) @abzokhattab / @eVoloshchak ? |
@MitchExpensify This is fixed via #38330. You can close this issue. |
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.7-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:
Expected Result:
"Connect online with plaid" page must not be shown and "connect Bank account" page must be displayed
Actual Result:
Instead of showing "connect Bank account" page, "Connect online with plaid" page is shown for a second and then only "connect Bank account" page is displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6296635_1701380765722.connect.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: