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

Jump to bottom

Add a loading functionality to eliminate race condition in ResetFreeBankAccount #12112

Closed
wants to merge 2 commits into from
Closed

Add a loading functionality to eliminate race condition in ResetFreeBankAccount #12112

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
e87a1fb
initial fix
ctkochan22 Oct 24, 2022
6343713
fix lint
ctkochan22 Oct 25, 2022
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
38 changes: 24 additions & 14 deletions src/libs/actions/ReimbursementAccount/resetFreePlanBankAccount.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ import lodashGet from 'lodash/get';
import ONYXKEYS from '../../../ONYXKEYS';
import CONST from '../../../CONST';
import * as store from './store';
import Navigation from '../../Navigation/Navigation';
import ROUTES from '../../../ROUTES';
import * as API from '../../API';

/**
Expand All @@ -18,33 +16,45 @@ function resetFreePlanBankAccount() {
throw new Error('Missing credentials when attempting to reset free plan bank account');
}

const achData = {
useOnfido: true,
policyID: '',
isInSetup: true,
domainLimit: 0,
currentStep: CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT,
};

API.write('RestartBankAccountSetup',
{
bankAccountID,
ownerEmail: store.getCredentials().login,
},
{
optimisticData: [{
onyxMethod: 'merge',
onyxMethod: 'set',
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
value: {achData, shouldShowResetModal: false},
value: {
achData: {},
shouldShowResetModal: false,
isResettingReimbursementAccount: true,
},
},
{
onyxMethod: 'set',
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT_DRAFT,
value: null,
}],
successData: [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
value: {
isResettingReimbursementAccount: false,
},
},
],
failureData: [
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
value: {
isResettingReimbursementAccount: false,
},
},
],
});

Navigation.navigate(ROUTES.getBankAccountRoute());
}

export default resetFreePlanBankAccount;
65 changes: 39 additions & 26 deletions src/pages/workspace/WorkspaceBankAccountPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import withPolicy from './withPolicy';
import Button from '../../components/Button';
import MenuItem from '../../components/MenuItem';
import FullPageNotFoundView from '../../components/BlockingViews/FullPageNotFoundView';
import FullScreenLoadingIndicator from '../../components/FullscreenLoadingIndicator';

const propTypes = {
/** ACH data for the withdrawal account actively being set up */
Expand Down Expand Up @@ -58,6 +59,14 @@ class WorkspaceBankAccountPage extends React.Component {
this.navigateToBankAccountRoute();
}

componentDidUpdate(prevProps) {
Copy link
Contributor

@nkuoch nkuoch Oct 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm ....

So, you're doing this because you want to wait for the RestartBankAccountSetup call to finish (which deletes the free plan bank account) before calling navigateToBankAccountRoute (which calls fetchFreePlanVerifiedBankAccount), so it doesn't end up fetching a deleted bank account.

Personally, I would prefer calling API.makeRequestWithSideEffects('RestartBankAccountSetup' than doing this even though I know we want to avoid it.

Or another alternative would be to somehow pass a param to navigateToBankAccountRoute to tell it not to call fetchFreePlanVerifiedBankAccount in this case, as we should just be able to rely on the optimistic data that we put (that resets REIMBURSEMENT_ACCOUNT). Can you try it see if it would work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that second option better of passing a route parameter to avoid the fetch. Another alternative to that would be to have two components:

  1. Does the fetch on mounting (this is the one users normally hit in the flow)
  2. Doesn't do the fetch (you can only get to this component by being redirected to it from resetFreePlanBankAccount()

It's sorta the same thing as a route parameter but implemented in a slightly different way.

Copy link
Contributor Author

@ctkochan22 ctkochan22 Oct 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thats interesting. I wonder if we can do a bit of both. There are actually two problems here:

  1. User resets VBBA and then we immediately do a fetch
  2. When user logs in for the first time, we don't do any sort of fetch before WorkspaceBankAccountPage, so we skip this step entirely. Which is a bit strange
    image

We could:

  1. Always fetch VBBA upon getting to WorkspaceBankAccountPage component
  2. If we navigate to /bank-account from this component, we always skip fetching (since we just did one)
  3. If user chooses to "Start Over", we call the API. Skip the fetch, so we have no issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing in a param to navigate is trickier than I thought. Still working

const wasResetting = lodashGet(prevProps, 'reimbursementAccount.isResettingReimbursementAccount', false);
const isResetting = lodashGet(this.props, 'reimbursementAccount.isResettingReimbursementAccount', false);
if (wasResetting && !isResetting) {
this.navigateToBankAccountRoute();
}
}

/**
* If we have an open bank account or no bank account at all then we will immediately redirect the user to /bank-account to display the next step
*
Expand Down Expand Up @@ -88,32 +97,36 @@ class WorkspaceBankAccountPage extends React.Component {
guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_BANK_ACCOUNT}
shouldShowBackButton
/>
<ScrollView style={styles.flex1}>
<Section
title={this.props.translate('workspace.bankAccount.almostDone')}
icon={Illustrations.BankArrowPink}
>
<Text>
{this.props.translate('workspace.bankAccount.youreAlmostDone')}
</Text>
</Section>
<Button
text={this.props.translate('workspace.bankAccount.continueWithSetup')}
onPress={this.navigateToBankAccountRoute}
icon={Expensicons.Bank}
style={[styles.mt2, styles.buttonCTA]}
iconStyles={[styles.buttonCTAIcon]}
shouldShowRightIcon
large
success
/>
<MenuItem
title={this.props.translate('workspace.bankAccount.startOver')}
icon={Expensicons.RotateLeft}
onPress={BankAccounts.requestResetFreePlanBankAccount}
shouldShowRightIcon
/>
</ScrollView>
{this.props.reimbursementAccount.isResettingReimbursementAccount ? (
<FullScreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />
) : (
<ScrollView style={styles.flex1}>
<Section
title={this.props.translate('workspace.bankAccount.almostDone')}
icon={Illustrations.BankArrowPink}
>
<Text>
{this.props.translate('workspace.bankAccount.youreAlmostDone')}
</Text>
</Section>
<Button
text={this.props.translate('workspace.bankAccount.continueWithSetup')}
onPress={this.navigateToBankAccountRoute}
icon={Expensicons.Bank}
style={[styles.mt2, styles.buttonCTA]}
iconStyles={[styles.buttonCTAIcon]}
shouldShowRightIcon
large
success
/>
<MenuItem
title={this.props.translate('workspace.bankAccount.startOver')}
icon={Expensicons.RotateLeft}
onPress={BankAccounts.requestResetFreePlanBankAccount}
shouldShowRightIcon
/>
</ScrollView>
)}
<WorkspaceResetBankAccountModal />
</FullPageNotFoundView>
</ScreenWrapper>
Expand Down