From 8eb34586110c5861cbf4f518234908aa99f10e29 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Thu, 9 Sep 2021 16:52:16 -0700 Subject: [PATCH 1/6] Handle the subStep directly from Onyx The subState in the VBA flow was being copied from Onyx into a local state in the BankAccountStep component and it was read from this local state. This caused that when we update the subState from BankAccounts.js in Onyx, the component BankAccountStep missed the update because it was reading it from the local state. --- .../ReimbursementAccount/BankAccountStep.js | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/pages/ReimbursementAccount/BankAccountStep.js b/src/pages/ReimbursementAccount/BankAccountStep.js index 93458d8a7cae..d816daf2fd16 100644 --- a/src/pages/ReimbursementAccount/BankAccountStep.js +++ b/src/pages/ReimbursementAccount/BankAccountStep.js @@ -3,7 +3,7 @@ import lodashGet from 'lodash/get'; import React from 'react'; import {View, Image} from 'react-native'; import PropTypes from 'prop-types'; -import {withOnyx} from 'react-native-onyx'; +import Onyx, {withOnyx} from 'react-native-onyx'; import HeaderWithCloseButton from '../../components/HeaderWithCloseButton'; import MenuItem from '../../components/MenuItem'; import { @@ -49,7 +49,6 @@ class BankAccountStep extends React.Component { this.addPlaidAccount = this.addPlaidAccount.bind(this); this.state = { // One of CONST.BANK_ACCOUNT.SETUP_TYPE - bankAccountAddMethod: props.achData.subStep || undefined, hasAcceptedTerms: props.achData.acceptTerms || true, routingNumber: props.achData.routingNumber || '', accountNumber: props.achData.accountNumber || '', @@ -78,6 +77,16 @@ class BankAccountStep extends React.Component { return errors[inputKey] ? this.props.translate(this.errorTranslationKeys[inputKey]) : ''; } + setSubStep(subStep) { + console.debug('setSubStep', subStep); + if (!subStep) { + // undefined seems to be ignored by Onyx + Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {achData: {subStep: null}}); + } else { + Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {achData: {subStep}}); + } + } + toggleTerms() { this.setState(prevState => ({ hasAcceptedTerms: !prevState.hasAcceptedTerms, @@ -185,15 +194,16 @@ class BankAccountStep extends React.Component { // Disable bank account fields once they've been added in db so they can't be changed const isFromPlaid = this.props.achData.setupType === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID; const shouldDisableInputs = Boolean(this.props.achData.bankAccountID) || isFromPlaid; + const {subStep} = this.props.achData; return ( this.setState({bankAccountAddMethod: undefined})} - shouldShowBackButton={!_.isUndefined(this.state.bankAccountAddMethod)} + onBackButtonPress={() => this.setSubStep(undefined)} + shouldShowBackButton={!_.isUndefined(subStep)} /> - {!this.state.bankAccountAddMethod && ( + {!subStep && ( <> @@ -202,9 +212,7 @@ class BankAccountStep extends React.Component { { - this.setState({bankAccountAddMethod: CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID}); - }} + onPress={() => this.setSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID)} disabled={this.props.isPlaidDisabled} shouldShowRightIcon /> @@ -216,9 +224,7 @@ class BankAccountStep extends React.Component { { - this.setState({bankAccountAddMethod: CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL}); - }} + onPress={() => this.setSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL)} shouldShowRightIcon /> @@ -240,16 +246,15 @@ class BankAccountStep extends React.Component { )} - {this.state.bankAccountAddMethod === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID && ( + {subStep === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID && ( { - this.setState({bankAccountAddMethod: undefined}); - }} + onExitPlaid={() => this.setSubStep(undefined)} + /> )} - {this.state.bankAccountAddMethod === CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL && ( + {subStep === CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL && ( <> From 7c5866e6ae4b76215f541e49a9d058cd6a3d1e42 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Thu, 9 Sep 2021 18:39:29 -0700 Subject: [PATCH 2/6] Avoid double-fetching the withdrawal account data Withdrawal account data was being loaded from AuthScreens component, and then again from ReiumbursementAccountPage on mount. Moved the code handling the step from route to componentDidUpdate in ReimbursementAccountPage and is not fetching anymore. --- src/libs/actions/BankAccounts.js | 7 +++++-- .../ReimbursementAccount/BankAccountStep.js | 2 +- .../ReimbursementAccountPage.js | 16 ++++++++++------ 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/libs/actions/BankAccounts.js b/src/libs/actions/BankAccounts.js index 1704d1f19d04..2847f22fb28e 100644 --- a/src/libs/actions/BankAccounts.js +++ b/src/libs/actions/BankAccounts.js @@ -345,9 +345,9 @@ function fetchFreePlanVerifiedBankAccount(stepToOpen) { // We are using set here since we will rely on data from the server (not local data) to populate the VBA flow // and determine which step to navigate to. Onyx.set(ONYXKEYS.REIMBURSEMENT_ACCOUNT, { - loading: true, + loading: true, // shows loading indicator error: '', - + fetching: true, // only used when fetching // We temporarily keep the achData state to prevent UI changes while fetching. achData: {state: lodashGet(reimbursementAccountInSetup, 'state', '')}, }); @@ -505,6 +505,7 @@ function fetchFreePlanVerifiedBankAccount(stepToOpen) { .finally(() => { const dataToMerge = { loading: false, + fetching: false, }; // If we didn't get a routingNumber and accountNumber from the response and we have previously saved @@ -581,6 +582,7 @@ function setFreePlanVerifiedBankAccountID(bankAccountID) { * @param {String} validateCode */ function validateBankAccount(bankAccountID, validateCode) { + console.log('validateBankAccount'); Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: true}); API.BankAccount_Validate({bankAccountID, validateCode}) @@ -653,6 +655,7 @@ function showBankAccountFormValidationError(error) { * @param {Object} [data] */ function setupWithdrawalAccount(data) { + console.log('setupWithdrawalAccount', data); let nextStep; Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: true}); diff --git a/src/pages/ReimbursementAccount/BankAccountStep.js b/src/pages/ReimbursementAccount/BankAccountStep.js index d816daf2fd16..f420c4f5c880 100644 --- a/src/pages/ReimbursementAccount/BankAccountStep.js +++ b/src/pages/ReimbursementAccount/BankAccountStep.js @@ -201,7 +201,7 @@ class BankAccountStep extends React.Component { title={this.props.translate('bankAccount.addBankAccount')} onCloseButtonPress={Navigation.dismissModal} onBackButtonPress={() => this.setSubStep(undefined)} - shouldShowBackButton={!_.isUndefined(subStep)} + shouldShowBackButton={Boolean(subStep)} /> {!subStep && ( <> diff --git a/src/pages/ReimbursementAccount/ReimbursementAccountPage.js b/src/pages/ReimbursementAccount/ReimbursementAccountPage.js index 7e4f7fa8a779..b174473d2d09 100644 --- a/src/pages/ReimbursementAccount/ReimbursementAccountPage.js +++ b/src/pages/ReimbursementAccount/ReimbursementAccountPage.js @@ -7,7 +7,7 @@ import {View} from 'react-native'; import PropTypes from 'prop-types'; import ScreenWrapper from '../../components/ScreenWrapper'; import { - fetchFreePlanVerifiedBankAccount, + goToWithdrawalAccountSetupStep, hideBankAccountErrorModal, hideBankAccountErrors, } from '../../libs/actions/BankAccounts'; @@ -87,11 +87,6 @@ const defaultProps = { }; class ReimbursementAccountPage extends React.Component { - componentDidMount() { - // We can specify a step to navigate to by using route params when the component mounts. - fetchFreePlanVerifiedBankAccount(this.getStepToOpenFromRouteParams()); - } - componentDidUpdate(prevProps) { const currentStep = lodashGet( this.props, @@ -104,6 +99,15 @@ class ReimbursementAccountPage extends React.Component { CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT, ); + if (prevProps.reimbursementAccount.fetching && !this.props.reimbursementAccount.fetching) { + // We can specify a step to navigate to by using route params when finish fetching the bank account data. + const routeStep = this.getStepToOpenFromRouteParams(); + if (this.props.reimbursementAccount.achData.currentStep !== routeStep) { + goToWithdrawalAccountSetupStep(routeStep); + return; + } + } + if (currentStep === previousStep) { return; } From ce5357ee1a6b1d61c9dadda610c3b5e09e625b0e Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Mon, 13 Sep 2021 17:42:06 -0700 Subject: [PATCH 3/6] Revert Onyx merge separate tick and double fetch --- src/libs/actions/BankAccounts.js | 19 +++++-------------- .../ReimbursementAccountPage.js | 16 ++++++---------- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/src/libs/actions/BankAccounts.js b/src/libs/actions/BankAccounts.js index 94dd3c43a2fc..8abcf4338f58 100644 --- a/src/libs/actions/BankAccounts.js +++ b/src/libs/actions/BankAccounts.js @@ -339,9 +339,9 @@ function fetchFreePlanVerifiedBankAccount(stepToOpen) { // We are using set here since we will rely on data from the server (not local data) to populate the VBA flow // and determine which step to navigate to. Onyx.set(ONYXKEYS.REIMBURSEMENT_ACCOUNT, { - loading: true, // shows loading indicator + loading: true, error: '', - fetching: true, // only used when fetching + // We temporarily keep the achData state to prevent UI changes while fetching. achData: {state: lodashGet(reimbursementAccountInSetup, 'state', '')}, }); @@ -471,7 +471,7 @@ function fetchFreePlanVerifiedBankAccount(stepToOpen) { goToWithdrawalAccountSetupStep(currentStep, achData); }) .finally(() => { - Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, fetching: false}); + Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false}); }); }); } @@ -644,18 +644,9 @@ function setupWithdrawalAccount(data) { } API.BankAccount_SetupWithdrawal(newACHData) - /* eslint-disable arrow-body-style */ - .then((response) => { - // Without this block, we can call merge again with the achData before this merge finishes, resulting in - // the original achData overwriting the data we're trying to set here. With this block, we ensure that the - // newACHData is set in Onyx before we call merge on the reimbursementAccount key again. - return Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, { - loading: false, - achData: {...newACHData}, - }) - .then(() => Promise.resolve(response)); - }) .then((response) => { + Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, achData: {...newACHData}}); + const currentStep = newACHData.currentStep; let achData = lodashGet(response, 'achData', {}); let error = lodashGet(achData, CONST.BANK_ACCOUNT.VERIFICATIONS.ERROR_MESSAGE); diff --git a/src/pages/ReimbursementAccount/ReimbursementAccountPage.js b/src/pages/ReimbursementAccount/ReimbursementAccountPage.js index b174473d2d09..7e4f7fa8a779 100644 --- a/src/pages/ReimbursementAccount/ReimbursementAccountPage.js +++ b/src/pages/ReimbursementAccount/ReimbursementAccountPage.js @@ -7,7 +7,7 @@ import {View} from 'react-native'; import PropTypes from 'prop-types'; import ScreenWrapper from '../../components/ScreenWrapper'; import { - goToWithdrawalAccountSetupStep, + fetchFreePlanVerifiedBankAccount, hideBankAccountErrorModal, hideBankAccountErrors, } from '../../libs/actions/BankAccounts'; @@ -87,6 +87,11 @@ const defaultProps = { }; class ReimbursementAccountPage extends React.Component { + componentDidMount() { + // We can specify a step to navigate to by using route params when the component mounts. + fetchFreePlanVerifiedBankAccount(this.getStepToOpenFromRouteParams()); + } + componentDidUpdate(prevProps) { const currentStep = lodashGet( this.props, @@ -99,15 +104,6 @@ class ReimbursementAccountPage extends React.Component { CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT, ); - if (prevProps.reimbursementAccount.fetching && !this.props.reimbursementAccount.fetching) { - // We can specify a step to navigate to by using route params when finish fetching the bank account data. - const routeStep = this.getStepToOpenFromRouteParams(); - if (this.props.reimbursementAccount.achData.currentStep !== routeStep) { - goToWithdrawalAccountSetupStep(routeStep); - return; - } - } - if (currentStep === previousStep) { return; } From abb39df2e3f843e4d13a6947969f61f0d083fa7e Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Mon, 13 Sep 2021 17:54:36 -0700 Subject: [PATCH 4/6] Move Onyx.merge to actions --- src/libs/actions/BankAccounts.js | 12 ++++++++++- .../ReimbursementAccount/BankAccountStep.js | 21 ++++++------------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/libs/actions/BankAccounts.js b/src/libs/actions/BankAccounts.js index 8abcf4338f58..8c60a9682986 100644 --- a/src/libs/actions/BankAccounts.js +++ b/src/libs/actions/BankAccounts.js @@ -72,7 +72,7 @@ function goToWithdrawalAccountSetupStep(stepID, achData) { // When going back to the BankAccountStep from the Company Step, show the manual form instead of Plaid if (newACHData.currentStep === CONST.BANK_ACCOUNT.STEP.COMPANY && stepID === CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT) { - newACHData.subStep = 'manual'; + newACHData.subStep = CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL; } Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {achData: {...newACHData, ...achData, currentStep: stepID}}); @@ -605,6 +605,15 @@ function showBankAccountFormValidationError(error) { Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {error}); } +/** + * Set the current sub step in first step of adding withdrawal bank account + * + * @param {String} subStep - One of {CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL, CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID, null} + */ +function setBankAccountSubStep(subStep) { + Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {achData: {subStep}}); +} + /** * Create or update the bank account in db with the updated data. * @@ -815,5 +824,6 @@ export { showBankAccountFormValidationError, setBankAccountFormValidationErrors, setWorkspaceIDForReimbursementAccount, + setBankAccountSubStep, updateReimbursementAccountDraft, }; diff --git a/src/pages/ReimbursementAccount/BankAccountStep.js b/src/pages/ReimbursementAccount/BankAccountStep.js index eb2b651ea027..35bfac628fca 100644 --- a/src/pages/ReimbursementAccount/BankAccountStep.js +++ b/src/pages/ReimbursementAccount/BankAccountStep.js @@ -3,7 +3,7 @@ import lodashGet from 'lodash/get'; import React from 'react'; import {View, Image} from 'react-native'; import PropTypes from 'prop-types'; -import Onyx, {withOnyx} from 'react-native-onyx'; +import {withOnyx} from 'react-native-onyx'; import HeaderWithCloseButton from '../../components/HeaderWithCloseButton'; import MenuItem from '../../components/MenuItem'; import { @@ -24,6 +24,7 @@ import Text from '../../components/Text'; import ExpensiTextInput from '../../components/ExpensiTextInput'; import { setBankAccountFormValidationErrors, + setBankAccountSubStep, setupWithdrawalAccount, showBankAccountErrorModal, updateReimbursementAccountDraft, @@ -79,16 +80,6 @@ class BankAccountStep extends React.Component { return errors[inputKey] ? this.props.translate(this.errorTranslationKeys[inputKey]) : ''; } - setSubStep(subStep) { - console.debug('setSubStep', subStep); - if (!subStep) { - // undefined seems to be ignored by Onyx - Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {achData: {subStep: null}}); - } else { - Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {achData: {subStep}}); - } - } - toggleTerms() { this.setState((prevState) => { const hasAcceptedTerms = !prevState.hasAcceptedTerms; @@ -206,7 +197,7 @@ class BankAccountStep extends React.Component { this.setSubStep(undefined)} + onBackButtonPress={() => setBankAccountSubStep(null)} shouldShowBackButton={Boolean(subStep)} /> {!subStep && ( @@ -218,7 +209,7 @@ class BankAccountStep extends React.Component { this.setSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID)} + onPress={() => setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID)} disabled={this.props.isPlaidDisabled} shouldShowRightIcon /> @@ -230,7 +221,7 @@ class BankAccountStep extends React.Component { this.setSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL)} + onPress={() => setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL)} shouldShowRightIcon /> @@ -256,7 +247,7 @@ class BankAccountStep extends React.Component { this.setSubStep(undefined)} + onExitPlaid={() => setBankAccountSubStep(null)} /> )} From 0137eeb75929266434cd12e35576a986fdad18c1 Mon Sep 17 00:00:00 2001 From: Aldo Canepa Date: Mon, 13 Sep 2021 17:56:44 -0700 Subject: [PATCH 5/6] Remove unwanted destructuring --- src/pages/ReimbursementAccount/BankAccountStep.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/ReimbursementAccount/BankAccountStep.js b/src/pages/ReimbursementAccount/BankAccountStep.js index 35bfac628fca..e691b22fb0ec 100644 --- a/src/pages/ReimbursementAccount/BankAccountStep.js +++ b/src/pages/ReimbursementAccount/BankAccountStep.js @@ -191,7 +191,7 @@ class BankAccountStep extends React.Component { // Disable bank account fields once they've been added in db so they can't be changed const isFromPlaid = this.props.achData.setupType === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID; const shouldDisableInputs = Boolean(this.props.achData.bankAccountID) || isFromPlaid; - const {subStep} = this.props.achData; + const subStep = this.props.achData.subStep; return ( Date: Mon, 13 Sep 2021 17:57:36 -0700 Subject: [PATCH 6/6] Remove empty line --- src/libs/actions/BankAccounts.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/actions/BankAccounts.js b/src/libs/actions/BankAccounts.js index 8c60a9682986..84572a285277 100644 --- a/src/libs/actions/BankAccounts.js +++ b/src/libs/actions/BankAccounts.js @@ -655,7 +655,6 @@ function setupWithdrawalAccount(data) { API.BankAccount_SetupWithdrawal(newACHData) .then((response) => { Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, achData: {...newACHData}}); - const currentStep = newACHData.currentStep; let achData = lodashGet(response, 'achData', {}); let error = lodashGet(achData, CONST.BANK_ACCOUNT.VERIFICATIONS.ERROR_MESSAGE);