Skip to content
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

Closed
1 of 6 tasks
lanitochka17 opened this issue Dec 1, 2023 · 49 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Dec 1, 2023

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: 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:

  1. Go to https://staging.new.expensify.com/
  2. Tap profile icon
  3. Tap Workspaces
  4. Tap on a Workspace
  5. Tap Bank account
  6. Tap connect online with plaid
  7. Tap continue and Select Regions Bank
  8. Enter credentials and submit
  9. Tap continue
  10. Tap allow

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?

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6296635_1701380765722.connect.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012a029613cdb55a03
  • Upwork Job ID: 1730598635612438528
  • Last Price Increase: 2024-03-08
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 1, 2023
@melvin-bot melvin-bot bot changed the title Bank account - "Connect online with plaid" page is shown first instead of "connect Bank account" [$500] Bank account - "Connect online with plaid" page is shown first instead of "connect Bank account" Dec 1, 2023
Copy link

melvin-bot bot commented Dec 1, 2023

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

Copy link

melvin-bot bot commented Dec 1, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 1, 2023
Copy link

melvin-bot bot commented Dec 1, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Copy link

melvin-bot bot commented Dec 1, 2023

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

@bernhardoj
Copy link
Contributor

bernhardoj commented Dec 1, 2023

Proposal

Please 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 Connect online with Plaid, the subStep data is set to plaid. The connect option page will be shown if the subStep is empty.

if (subStep === CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL) {
return (
<BankAccountManualStep
reimbursementAccount={props.reimbursementAccount}
reimbursementAccountDraft={props.reimbursementAccountDraft}
onBackButtonPress={props.onBackButtonPress}
getDefaultStateForField={props.getDefaultStateForField}
/>
);
}
if (subStep === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID) {
return (
<BankAccountPlaidStep
reimbursementAccount={props.reimbursementAccount}
reimbursementAccountDraft={props.reimbursementAccountDraft}
onBackButtonPress={props.onBackButtonPress}
getDefaultStateForField={props.getDefaultStateForField}
/>
);
}
return (
<ScreenWrapper
includeSafeAreaPaddingBottom={false}
testID={BankAccountStep.displayName}
>
<View style={[styles.flex1, styles.justifyContentBetween]}>
<HeaderWithBackButton
title={props.translate('workspace.common.connectBankAccount')}
subtitle={props.policyName}
stepCounter={subStep ? {step: 1, total: 5} : undefined}
onBackButtonPress={props.onBackButtonPress}
shouldShowGetAssistanceButton
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_BANK_ACCOUNT}
/>
<ScrollView style={[styles.flex1]}>
<Section
icon={Illustrations.MoneyWings}
title={props.translate('workspace.bankAccount.streamlinePayments')}
>
<View style={[styles.mv3]}>
<Text>{props.translate('bankAccount.toGetStarted')}</Text>
</View>
{Boolean(plaidDesktopMessage) && (
<View style={[styles.mv3, styles.flexRow, styles.justifyContentBetween]}>
<TextLink href={bankAccountRoute}>{props.translate(plaidDesktopMessage)}</TextLink>
</View>
)}
<Button
icon={Expensicons.Bank}
text={props.translate('bankAccount.connectOnlineWithPlaid')}
onPress={() => {
if (props.isPlaidDisabled || !props.user.validated) {
return;
}
BankAccounts.openPlaidView();
}}
isDisabled={props.isPlaidDisabled || !props.user.validated}
style={[styles.mt4]}
iconStyles={[styles.buttonCTAIcon]}
shouldShowRightIcon
success
large
/>
{Boolean(props.error) && <Text style={[styles.formError, styles.mh5]}>{props.error}</Text>}
<View style={[styles.mv3]}>
<MenuItem
icon={Expensicons.Connect}
title={props.translate('bankAccount.connectManually')}
disabled={!props.user.validated}
onPress={() => BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL)}
shouldShowRightIcon
wrapperStyle={[styles.cardMenuItem]}
/>
</View>
</Section>
{!props.user.validated && (
<View style={[styles.flexRow, styles.alignItemsCenter, styles.m4]}>
<Icon
src={Expensicons.Exclamation}
fill={theme.danger}
/>
<Text style={[styles.mutedTextLabel, styles.ml4, styles.flex1]}>{props.translate('bankAccount.validateAccountError')}</Text>
</View>
)}
<View style={[styles.mv0, styles.mh5, styles.flexRow, styles.justifyContentBetween]}>
<TextLink href="https://use.expensify.com/privacy">{props.translate('common.privacy')}</TextLink>
<PressableWithoutFeedback
onPress={() => Link.openExternalLink('https://community.expensify.com/discussion/5677/deep-dive-how-expensify-protects-your-information/')}
style={[styles.flexRow, styles.alignItemsCenter]}
accessibilityLabel={props.translate('bankAccount.yourDataIsSecure')}
>
<TextLink href="https://community.expensify.com/discussion/5677/deep-dive-how-expensify-protects-your-information/">
{props.translate('bankAccount.yourDataIsSecure')}
</TextLink>
<View style={[styles.ml1]}>
<Icon
src={Expensicons.Lock}
fill={theme.link}
/>
</View>
</PressableWithoutFeedback>
</View>
</ScrollView>
</View>
</ScreenWrapper>
);

After we complete the plaid setup, the plaid is closed and onExit callback of the plaid is called. Currently, every time onExit is called, we clear the subStep which will show the connect option page briefly (before the API response returns the correct subStep).

onExitPlaid={() => BankAccounts.setBankAccountSubStep(null)}

Based on the plaid doc, onExit will be called if the user closes the plaid without completing it.

// User prematurely exited the Plaid flow
// eslint-disable-next-line react/jsx-props-no-multi-spaces
onExit={onExitPlaid}

So, there is something wrong with our code. After doing further investigation, we tried to open the plaid multiple times unintended in a useEffect, and somehow this resulted in a buggy behavior.

useEffect(() => {
if (error) {
onError(error);
return;
}
if (!ready) {
return;
}
if (!isPlaidLoaded) {
return;
}
open();
}, [ready, error, isPlaidLoaded, open, onError]);

Why open is called multiple times? It's because of the onError props. When the plaid component re-renders, onError props instance is always recreated. It's because we pass an inline function.

onError={(error) => {
Log.hmmm('[PlaidLink] Error: ', error.message);
}}

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

Use useCallback to wrap the onError function to have a stable ref of the function.

const onError = useCallback((error) => {
    Log.hmmm('[PlaidLink] Error: ', error.message);
}, []);

onError={onError}

@neonbhai
Copy link
Contributor

neonbhai commented Dec 1, 2023

Proposal

Please 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:

Screenshot 2023-12-01 at 9 17 54 PM

The process should stop after the HANDOFF event.

But there is another OPEN event and subsequent onExit call.

The onExit should only be called when when the user exits prematurely. This function makes the app reset to the connect options page. But, an extra render and instant exit after submission causes underlying options page to be visible while the bank account is processing.

This extra render is caused by the non-memoized onSelect and onExitPlaid functions here and here. As can be seen by the why-did-you-render outputs:

Screenshot 2023-12-01 at 10 30 56 PM

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

We should memoise both functions using useCallback.

    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.mov

After Changes:

Plaid.mov

@melvin-bot melvin-bot bot added the Overdue label Dec 4, 2023
Copy link

melvin-bot bot commented Dec 4, 2023

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

@MitchExpensify
Copy link
Contributor

Lets get this fixed!

Copy link

melvin-bot bot commented Dec 6, 2023

Triggered auto assignment to @grgia (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 6, 2023
Copy link

melvin-bot bot commented Dec 8, 2023

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

@kbecciv kbecciv changed the title [$500] Bank account - "Connect online with plaid" page is shown first instead of "connect Bank account" [$500] mWeb - "Connect online with plaid" page is shown first instead of "connect Bank account" Dec 8, 2023
Copy link

melvin-bot bot commented Dec 11, 2023

@eVoloshchak, @MitchExpensify, @grgia Eep! 4 days overdue now. Issues have feelings too...

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Dec 12, 2023

How do these proposals look to you @eVoloshchak ?

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2023
@eVoloshchak
Copy link
Contributor

@neonbhai proposal:
doesn't resolve the issue for me, the behavior can still be observed on web, could you double-check please?

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 react-plaid-link?
Also, this might be partially related to plaid/react-plaid-link#325

@bernhardoj
Copy link
Contributor

@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 onExit callback is called even though we complete the setup which is caused by multiple plaid link open calls.

I can't find the plaid link core source code, so my guess is that because we have n open calls and we complete it once, the n-1 "opened" plaid is exited prematurely so onExit is called.

@melvin-bot melvin-bot bot removed the Overdue label Feb 6, 2024
@melvin-bot melvin-bot bot added the Overdue label Feb 15, 2024
@eVoloshchak
Copy link
Contributor

Still on hold for #30442

@melvin-bot melvin-bot bot removed the Overdue label Feb 15, 2024
@mountiny mountiny changed the title [HOLD VBBA refactor] [$500] mWeb - "Connect online with plaid" page is shown first instead of "connect Bank account" [$500] mWeb - "Connect online with plaid" page is shown first instead of "connect Bank account" Feb 16, 2024
@mountiny
Copy link
Contributor

of HOLD

Copy link

melvin-bot bot commented Feb 16, 2024

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

@MitchExpensify
Copy link
Contributor

Copy link

melvin-bot bot commented Feb 23, 2024

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

@MitchExpensify
Copy link
Contributor

@dylanexpensify Is this wave 5 worthy?

Copy link

melvin-bot bot commented Mar 1, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Mar 5, 2024
@MitchExpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Mar 5, 2024
@MitchExpensify
Copy link
Contributor

Seems like a LOW wave collect issue, what d'you reckon @trjExpensify ?

@trjExpensify
Copy link
Contributor

It would go in the Polish section, yeah. Quick questions though:

  1. When was this last tested? It was on hold for the VBBA refactor, came off hold, but I can't see if it has been retested and if it stills occurs?
  2. I don't think it's mWeb specific judging by this proposal showing it happen on Web?

Copy link

melvin-bot bot commented Mar 7, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

Copy link

melvin-bot bot commented Mar 8, 2024

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

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Mar 10, 2024

  1. When was this last tested? It was on hold for the VBBA refactor, came off hold, but I can't see if it has been retested and if it stills occurs?

Can't reproduce on Android using Browserstack - @lanitochka17 can you please let me know if you are still seeing this on Android?

image

mWeb on Browsterstack is also not playing nice for me. Can't log in:

image
  1. I don't think it's mWeb specific judging by this proposal showing it happen on Web?

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.mov

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

@bernhardoj
Copy link
Contributor

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

Web is proving weird for me too - I'm getting booted from the VBA flow altogether

Maybe try re-login to have fresh data.

@abzokhattab
Copy link
Contributor

Posting my proposal from #38069 (comment) here as well:

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:

useEffect(() => {
if (error) {
onError(error);
return;
}
if (!ready) {
return;
}
if (!isPlaidLoaded) {
return;
}
open();
}, [ready, error, isPlaidLoaded, open, onError]);

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

Screen.Recording.2024-03-13.at.12.56.53.PM.mov

@MitchExpensify
Copy link
Contributor

Oh wait.. is this issue a dupe of #38069 (comment) @abzokhattab / @eVoloshchak ?

@shubham1206agra
Copy link
Contributor

@MitchExpensify This is fixed via #38330. You can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants