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.

Sign up for GitHub

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

Plaid popup continue to open from resume app #29927

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Plaid popup continue to open from resume app #29927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81c8c58
plaid modal opne
pradeepmdk Oct 18, 2023
60799d9
add const values
pradeepmdk Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@ const CONST = {
ERROR: {
TOO_MANY_ATTEMPTS: 'Too many attempts',
},
EVENTS_NAME: {
OPEN: 'OPEN',
EXIT: 'EXIT',
},
},
ERROR: {
MISSING_ROUTING_NUMBER: '402 Missing routingNumber',
Expand Down
3 changes: 3 additions & 0 deletions src/ONYXKEYS.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ const ONYXKEYS = {
/** Token needed to initialize Plaid link */
PLAID_LINK_TOKEN: 'plaidLinkToken',

/** Capture Plaid event */
PLAID_CURRENT_EVENT: 'plaidCurrentEvent',

/** Token needed to initialize Onfido */
ONFIDO_TOKEN: 'onfidoToken',

Expand Down
1 change: 1 addition & 0 deletions src/components/AddPlaidBankAccount.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ function AddPlaidBankAccount({
Log.hmmm('[PlaidLink] Error: ', error.message);
}}
onEvent={(event, metadata) => {
BankAccounts.setPlaidEvent(event);
// Handle Plaid login errors (will potentially reset plaid token and item depending on the error)
if (event === 'ERROR') {
Log.hmmm('[PlaidLink] Error: ', metadata);
Expand Down
8 changes: 7 additions & 1 deletion src/components/PlaidLink/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {useEffect} from 'react';
import {openLink, useDeepLinkRedirector, usePlaidEmitter} from 'react-native-plaid-link-sdk';
import {openLink, useDeepLinkRedirector, usePlaidEmitter, dismissLink} from 'react-native-plaid-link-sdk';
import Log from '../../libs/Log';
import {plaidLinkPropTypes, plaidLinkDefaultProps} from './plaidLinkPropTypes';
import CONST from '../../CONST';

function PlaidLink(props) {
useDeepLinkRedirector();
Expand All @@ -10,6 +11,7 @@ function PlaidLink(props) {
props.onEvent(event.eventName, event.metadata);
});
useEffect(() => {
props.onEvent(CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.OPEN, {});
openLink({
tokenConfig: {
token: props.token,
Expand All @@ -23,6 +25,10 @@ function PlaidLink(props) {
},
});

return () => {
dismissLink();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is beyond the proposal. What does this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2023-10-19 at 9 19 15 PM Already there in my proposal. when a component is destroyed we need to dismiss the link when onexit event trigger plaid component will destroy based on this `subStep` https://github.com/Expensify/App/blob/c77814fd5e643cba78e8c8294370d78f633a59c5/src/pages/ReimbursementAccount/BankAccountPlaidStep.js#L105

Copy link
Contributor

@situchan situchan Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, but what does this fix exactly?
What issue do you have currently in production app? (without calling dismissLink on unmount)

This is good fix but just to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the component is destroyed if the plaid popup opens means it will close the popup.
in the current issue was popup is opened repeatedly after resuming the app

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so with this fix only, duplicated popup bug is replaced with repeatedly close & open popup 2 times.
So only 1 instance exists, which is a little better than original bug. But still we should fix this for better user experience which is the reason for introducing new Onyx key
Thanks for the explanation.

Screen.Recording.2023-10-19.at.11.24.10.PM.mov

};

// We generally do not need to include the token as a dependency here as it is only provided once via props and should not change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Expand Down
7 changes: 6 additions & 1 deletion src/libs/actions/BankAccounts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,18 @@ type ReimbursementAccountSubStep = BankAccountSubStep | '';

function clearPlaid(): Promise<void> {
Onyx.set(ONYXKEYS.PLAID_LINK_TOKEN, '');

Onyx.set(ONYXKEYS.PLAID_CURRENT_EVENT, null);
return Onyx.set(ONYXKEYS.PLAID_DATA, PlaidDataProps.plaidDataDefaultProps);
}

function openPlaidView() {
clearPlaid().then(() => ReimbursementAccount.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID));
}

function setPlaidEvent(eventName: string) {
Onyx.set(ONYXKEYS.PLAID_CURRENT_EVENT, eventName);
}

/**
* Open the personal bank account setup flow, with an optional exitReportID to redirect to once the flow is finished.
*/
Expand Down Expand Up @@ -426,6 +430,7 @@ export {
clearOnfidoToken,
clearPersonalBankAccount,
clearPlaid,
setPlaidEvent,
openPlaidView,
connectBankAccountManually,
connectBankAccountWithPlaid,
Expand Down
9 changes: 7 additions & 2 deletions src/pages/ReimbursementAccount/ReimbursementAccountPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ class ReimbursementAccountPage extends React.Component {
}
if (subStep) {
BankAccounts.setBankAccountSubStep(null);
BankAccounts.setPlaidEvent(null);
} else {
Navigation.goBack(backTo);
}
Expand Down Expand Up @@ -396,8 +397,9 @@ class ReimbursementAccountPage extends React.Component {
</ScreenWrapper>
);
}

const isLoading = this.props.isLoadingReportData || this.props.account.isLoading || this.props.reimbursementAccount.isLoading;
const isLoading =
(this.props.isLoadingReportData || this.props.account.isLoading || this.props.reimbursementAccount.isLoading) &&
(!this.props.plaidCurrentEvent || this.props.plaidCurrentEvent === CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.EXIT);

// Prevent the full-page blocking offline view from being displayed for these steps if the device goes offline.
const shouldShowOfflineLoader = !(
Expand Down Expand Up @@ -551,6 +553,9 @@ export default compose(
plaidLinkToken: {
key: ONYXKEYS.PLAID_LINK_TOKEN,
},
plaidCurrentEvent: {
key: ONYXKEYS.PLAID_CURRENT_EVENT,
},
onfidoToken: {
key: ONYXKEYS.ONFIDO_TOKEN,
},
Expand Down
Loading