-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: restart the flow for another policy #49687
Changes from all commits
ae7a704
d7d0411
e88d940
98b1256
90b1454
e1524d7
29623ee
67d7d99
626cea8
7956396
d4ca888
e461db1
20165ec
e56d462
dfd1ab5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import {useOnyx} from 'react-native-onyx'; | |
import type {ValueOf} from 'type-fest'; | ||
import FullPageNotFoundView from '@components/BlockingViews/FullPageNotFoundView'; | ||
import HeaderWithBackButton from '@components/HeaderWithBackButton'; | ||
import {useSession} from '@components/OnyxProvider'; | ||
import ReimbursementAccountLoadingIndicator from '@components/ReimbursementAccountLoadingIndicator'; | ||
import ScreenWrapper from '@components/ScreenWrapper'; | ||
import Text from '@components/Text'; | ||
|
@@ -24,6 +25,7 @@ import shouldReopenOnfido from '@libs/shouldReopenOnfido'; | |
import type {WithPolicyOnyxProps} from '@pages/workspace/withPolicy'; | ||
import withPolicy from '@pages/workspace/withPolicy'; | ||
import * as BankAccounts from '@userActions/BankAccounts'; | ||
import * as ReimbursementAccount from '@userActions/ReimbursementAccount'; | ||
import CONST from '@src/CONST'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import type SCREENS from '@src/SCREENS'; | ||
|
@@ -96,64 +98,87 @@ function getRouteForCurrentStep(currentStep: TBankAccountStep): ValueOf<typeof R | |
} | ||
} | ||
|
||
/** | ||
* Returns selected bank account fields based on field names provided. | ||
*/ | ||
function getFieldsForStep(step: TBankAccountStep): InputID[] { | ||
switch (step) { | ||
case CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT: | ||
return ['routingNumber', 'accountNumber', 'bankName', 'plaidAccountID', 'plaidAccessToken', 'isSavings']; | ||
case CONST.BANK_ACCOUNT.STEP.COMPANY: | ||
return [ | ||
'companyName', | ||
'addressStreet', | ||
'addressZipCode', | ||
'addressCity', | ||
'addressState', | ||
'companyPhone', | ||
'website', | ||
'companyTaxID', | ||
'incorporationType', | ||
'incorporationDate', | ||
'incorporationState', | ||
]; | ||
case CONST.BANK_ACCOUNT.STEP.REQUESTOR: | ||
return ['firstName', 'lastName', 'dob', 'ssnLast4', 'requestorAddressStreet', 'requestorAddressCity', 'requestorAddressState', 'requestorAddressZipCode']; | ||
default: | ||
return []; | ||
} | ||
} | ||
|
||
function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps) { | ||
const session = useSession(); | ||
const [reimbursementAccount] = useOnyx(ONYXKEYS.REIMBURSEMENT_ACCOUNT); | ||
const [session] = useOnyx(ONYXKEYS.SESSION); | ||
const [plaidLinkToken = ''] = useOnyx(ONYXKEYS.PLAID_LINK_TOKEN); | ||
const [plaidCurrentEvent = ''] = useOnyx(ONYXKEYS.PLAID_CURRENT_EVENT); | ||
const [onfidoToken = ''] = useOnyx(ONYXKEYS.ONFIDO_TOKEN); | ||
const [isLoadingApp = false] = useOnyx(ONYXKEYS.IS_LOADING_APP); | ||
const [account] = useOnyx(ONYXKEYS.ACCOUNT); | ||
|
||
const policyName = policy?.name ?? ''; | ||
const policyIDParam = route.params?.policyID ?? '-1'; | ||
const styles = useThemeStyles(); | ||
const {translate} = useLocalize(); | ||
const {isOffline} = useNetwork(); | ||
const requestorStepRef = useRef(null); | ||
const prevReimbursementAccount = usePrevious(reimbursementAccount); | ||
const prevIsOffline = usePrevious(isOffline); | ||
|
||
/** | ||
The SetupWithdrawalAccount flow allows us to continue the flow from various points depending on where the | ||
user left off. This view will refer to the achData as the single source of truth to determine which route to | ||
display. We can also specify a specific route to navigate to via route params when the component first | ||
mounts which will set the achData.currentStep after the account data is fetched and overwrite the logical | ||
next step. | ||
*/ | ||
The SetupWithdrawalAccount flow allows us to continue the flow from various points depending on where the | ||
user left off. This view will refer to the achData as the single source of truth to determine which route to | ||
display. We can also specify a specific route to navigate to via route params when the component first | ||
mounts which will set the achData.currentStep after the account data is fetched and overwrite the logical | ||
next step. | ||
*/ | ||
const achData = reimbursementAccount?.achData; | ||
const isPreviousPolicy = policyIDParam === achData?.policyID; | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
const currentStep = !isPreviousPolicy ? CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT : achData?.currentStep || CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT; | ||
koko57 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
When this page is first opened, `reimbursementAccount` prop might not yet be fully loaded from Onyx. | ||
Calculating `shouldShowContinueSetupButton` immediately on initial render doesn't make sense as | ||
it relies on incomplete data. Thus, we should wait to calculate it until we have received | ||
the full `reimbursementAccount` data from the server. This logic is handled within the useEffect hook, | ||
which acts similarly to `componentDidUpdate` when the `reimbursementAccount` dependency changes. | ||
*/ | ||
const [hasACHDataBeenLoaded, setHasACHDataBeenLoaded] = useState(reimbursementAccount !== CONST.REIMBURSEMENT_ACCOUNT.DEFAULT_DATA && isPreviousPolicy); | ||
const [shouldShowContinueSetupButton, setShouldShowContinueSetupButton] = useState(getShouldShowContinueSetupButtonInitialValue()); | ||
|
||
function getBankAccountFields<T extends InputID>(fieldNames: T[]): Pick<ACHDataReimbursementAccount, T> { | ||
return { | ||
...lodashPick(reimbursementAccount?.achData, ...fieldNames), | ||
}; | ||
} | ||
|
||
/** | ||
* Returns selected bank account fields based on field names provided. | ||
*/ | ||
function getFieldsForStep(step: TBankAccountStep): InputID[] { | ||
switch (step) { | ||
case CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT: | ||
return ['routingNumber', 'accountNumber', 'bankName', 'plaidAccountID', 'plaidAccessToken', 'isSavings']; | ||
case CONST.BANK_ACCOUNT.STEP.COMPANY: | ||
return [ | ||
'companyName', | ||
'addressStreet', | ||
'addressZipCode', | ||
'addressCity', | ||
'addressState', | ||
'companyPhone', | ||
'website', | ||
'companyTaxID', | ||
'incorporationType', | ||
'incorporationDate', | ||
'incorporationState', | ||
]; | ||
case CONST.BANK_ACCOUNT.STEP.REQUESTOR: | ||
return ['firstName', 'lastName', 'dob', 'ssnLast4', 'requestorAddressStreet', 'requestorAddressCity', 'requestorAddressState', 'requestorAddressZipCode']; | ||
default: | ||
return []; | ||
} | ||
} | ||
|
||
/** | ||
* Returns true if a VBBA exists in any state other than OPEN or LOCKED | ||
*/ | ||
function hasInProgressVBBA(): boolean { | ||
return !!achData?.bankAccountID && achData?.state !== BankAccount.STATE.OPEN && achData?.state !== BankAccount.STATE.LOCKED; | ||
return !!achData?.bankAccountID && !!achData?.state && achData?.state !== BankAccount.STATE.OPEN && achData?.state !== BankAccount.STATE.LOCKED; | ||
} | ||
|
||
/* | ||
* Calculates the state used to show the "Continue with setup" view. If a bank account setup is already in progress and | ||
* no specific further step was passed in the url we'll show the workspace bank account reset modal if the user wishes to start over | ||
|
@@ -166,75 +191,36 @@ function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps | |
return achData?.state === BankAccount.STATE.PENDING || [CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT, ''].includes(getStepToOpenFromRouteParams(route)); | ||
} | ||
|
||
/** | ||
When this page is first opened, `reimbursementAccount` prop might not yet be fully loaded from Onyx. | ||
Calculating `shouldShowContinueSetupButton` immediately on initial render doesn't make sense as | ||
it relies on incomplete data. Thus, we should wait to calculate it until we have received | ||
the full `reimbursementAccount` data from the server. This logic is handled within the useEffect hook, | ||
which acts similarly to `componentDidUpdate` when the `reimbursementAccount` dependency changes. | ||
*/ | ||
const [hasACHDataBeenLoaded, setHasACHDataBeenLoaded] = useState(reimbursementAccount !== CONST.REIMBURSEMENT_ACCOUNT.DEFAULT_DATA); | ||
|
||
const [shouldShowContinueSetupButton, setShouldShowContinueSetupButton] = useState(hasACHDataBeenLoaded ? getShouldShowContinueSetupButtonInitialValue() : false); | ||
const [isReimbursementAccountLoading, setIsReimbursementAccountLoading] = useState(() => { | ||
// By default return true (loading), if there are already loaded data we can skip the loading state | ||
if (hasACHDataBeenLoaded && typeof reimbursementAccount?.isLoading === 'boolean' && !reimbursementAccount?.isLoading) { | ||
return false; | ||
} | ||
return true; | ||
}); | ||
|
||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
const currentStep = achData?.currentStep || CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT; | ||
const policyName = policy?.name ?? ''; | ||
const policyIDParam = route.params?.policyID ?? '-1'; | ||
const styles = useThemeStyles(); | ||
const {translate} = useLocalize(); | ||
const {isOffline} = useNetwork(); | ||
const requestorStepRef = useRef(null); | ||
const prevIsReimbursementAccountLoading = usePrevious(reimbursementAccount?.isLoading); | ||
const prevReimbursementAccount = usePrevious(reimbursementAccount); | ||
const prevIsOffline = usePrevious(isOffline); | ||
|
||
/** | ||
* Retrieve verified business bank account currently being set up. | ||
* @param ignoreLocalCurrentStep Pass true if you want the last "updated" view (from db), not the last "viewed" view (from onyx). | ||
* @param ignoreLocalSubStep Pass true if you want the last "updated" view (from db), not the last "viewed" view (from onyx). | ||
*/ | ||
function fetchData(ignoreLocalCurrentStep?: boolean, ignoreLocalSubStep?: boolean) { | ||
// Show loader right away, as optimisticData might be set only later in case multiple calls are in the queue | ||
BankAccounts.setReimbursementAccountLoading(true); | ||
|
||
function fetchData() { | ||
// We can specify a step to navigate to by using route params when the component mounts. | ||
// We want to use the same stepToOpen variable when the network state changes because we can be redirected to a different step when the account refreshes. | ||
const stepToOpen = getStepToOpenFromRouteParams(route); | ||
const subStep = achData?.subStep ?? ''; | ||
const localCurrentStep = achData?.currentStep ?? ''; | ||
BankAccounts.openReimbursementAccountPage(stepToOpen, ignoreLocalSubStep ? '' : subStep, ignoreLocalCurrentStep ? '' : localCurrentStep, policyIDParam); | ||
const subStep = isPreviousPolicy ? achData?.subStep ?? '' : ''; | ||
const localCurrentStep = isPreviousPolicy ? achData?.currentStep ?? '' : ''; | ||
BankAccounts.openReimbursementAccountPage(stepToOpen, subStep, localCurrentStep, policyIDParam); | ||
} | ||
|
||
useEffect(() => { | ||
if (!isReimbursementAccountLoading) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koko57 Why do you remove this check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we no longer use isReimbursementAccountLoading here. I removed the local state we're just taking the info about the loading from Onyx. We don't need it locally There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Night owl 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koko57 I mean should we replace with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. temporarily different timezone 😃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koko57 Bump on this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but here I'm changing the logic a bit. This runs only when component mounts. As we no longer set the state like this:
we don't need to check if it's true or false and skip the content of the hook. We also don't need to check for reimbursementAccount?.isLoading, we set it true later and we actually want the whole hook to run on mount. We only don't want to fetch the data if it's the same policy as we already have the data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it |
||
if (isPreviousPolicy) { | ||
return; | ||
} | ||
|
||
BankAccounts.setReimbursementAccountLoading(true); | ||
ReimbursementAccount.clearReimbursementAccountDraft(); | ||
|
||
// If the step to open is empty, we want to clear the sub step, so the connect option view is shown to the user | ||
const isStepToOpenEmpty = getStepToOpenFromRouteParams(route) === ''; | ||
if (isStepToOpenEmpty) { | ||
BankAccounts.setBankAccountSubStep(null); | ||
BankAccounts.setPlaidEvent(null); | ||
} | ||
fetchData(false, isStepToOpenEmpty); | ||
fetchData(); | ||
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||
}, []); // The empty dependency array ensures this runs only once after the component mounts. | ||
|
||
useEffect(() => { | ||
if (typeof reimbursementAccount?.isLoading !== 'boolean' || reimbursementAccount.isLoading === prevIsReimbursementAccountLoading) { | ||
return; | ||
} | ||
setIsReimbursementAccountLoading(reimbursementAccount.isLoading); | ||
}, [prevIsReimbursementAccountLoading, reimbursementAccount?.isLoading]); | ||
|
||
useEffect( | ||
() => { | ||
// Check for network change from offline to online | ||
|
@@ -243,8 +229,7 @@ function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps | |
} | ||
|
||
if (!hasACHDataBeenLoaded) { | ||
if (reimbursementAccount !== CONST.REIMBURSEMENT_ACCOUNT.DEFAULT_DATA && isReimbursementAccountLoading === false) { | ||
setShouldShowContinueSetupButton(getShouldShowContinueSetupButtonInitialValue()); | ||
if (reimbursementAccount !== CONST.REIMBURSEMENT_ACCOUNT.DEFAULT_DATA && reimbursementAccount?.isLoading === false) { | ||
setHasACHDataBeenLoaded(true); | ||
} | ||
return; | ||
|
@@ -296,7 +281,6 @@ function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps | |
BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL).then(() => { | ||
setShouldShowContinueSetupButton(false); | ||
}); | ||
fetchData(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koko57 same here, Why do you remove this line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't need to fetch the data here. Besides, when I was working on that that call to API seemed not to be sent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koko57 To safe, If we remove this, I think we need to understand why we added it before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This flow is quite complicated. So we should be careful about this change to avoid regression. Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DylanDylann as we fetchData when the component mounts we don't need to fetch it once again when we choose manual flow. And as I said - when working on it I've never saw this request being sent (not sure why). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DylanDylann yeah it's complicates 😅 I've tried to simplify it as much as possible. |
||
}; | ||
|
||
const goBack = () => { | ||
|
@@ -352,7 +336,7 @@ function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps | |
} | ||
}; | ||
|
||
const isLoading = (!!isLoadingApp || !!account?.isLoading || isReimbursementAccountLoading) && (!plaidCurrentEvent || plaidCurrentEvent === CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.EXIT); | ||
const isLoading = (!!isLoadingApp || !!account?.isLoading || reimbursementAccount?.isLoading) && (!plaidCurrentEvent || plaidCurrentEvent === CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.EXIT); | ||
|
||
const shouldShowOfflineLoader = !( | ||
isOffline && | ||
|
@@ -424,53 +408,43 @@ function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps | |
); | ||
} | ||
|
||
if (currentStep === CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT) { | ||
return ( | ||
<BankAccountStep | ||
reimbursementAccount={reimbursementAccount} | ||
onBackButtonPress={goBack} | ||
receivedRedirectURI={getPlaidOAuthReceivedRedirectURI()} | ||
plaidLinkOAuthToken={plaidLinkToken} | ||
policyName={policyName} | ||
policyID={policyIDParam} | ||
/> | ||
); | ||
} | ||
|
||
if (currentStep === CONST.BANK_ACCOUNT.STEP.COMPANY) { | ||
return <CompanyStep onBackButtonPress={goBack} />; | ||
} | ||
|
||
if (currentStep === CONST.BANK_ACCOUNT.STEP.REQUESTOR) { | ||
const shouldShowOnfido = onfidoToken && !achData?.isOnfidoSetupComplete; | ||
return ( | ||
<RequestorStep | ||
ref={requestorStepRef} | ||
shouldShowOnfido={!!shouldShowOnfido} | ||
onBackButtonPress={goBack} | ||
/> | ||
); | ||
} | ||
|
||
if (currentStep === CONST.BANK_ACCOUNT.STEP.BENEFICIAL_OWNERS) { | ||
return <BeneficialOwnersStep onBackButtonPress={goBack} />; | ||
} | ||
|
||
if (currentStep === CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT) { | ||
return <ACHContractStep onBackButtonPress={goBack} />; | ||
} | ||
|
||
if (currentStep === CONST.BANK_ACCOUNT.STEP.VALIDATION) { | ||
return <ConnectBankAccount onBackButtonPress={goBack} />; | ||
} | ||
|
||
if (currentStep === CONST.BANK_ACCOUNT.STEP.ENABLE) { | ||
return ( | ||
<EnableBankAccount | ||
reimbursementAccount={reimbursementAccount} | ||
onBackButtonPress={goBack} | ||
/> | ||
); | ||
switch (currentStep) { | ||
case CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT: | ||
return ( | ||
<BankAccountStep | ||
reimbursementAccount={reimbursementAccount} | ||
onBackButtonPress={goBack} | ||
receivedRedirectURI={getPlaidOAuthReceivedRedirectURI()} | ||
plaidLinkOAuthToken={plaidLinkToken} | ||
policyName={policyName} | ||
policyID={policyIDParam} | ||
/> | ||
); | ||
case CONST.BANK_ACCOUNT.STEP.REQUESTOR: | ||
return ( | ||
<RequestorStep | ||
ref={requestorStepRef} | ||
shouldShowOnfido={!!(onfidoToken && !achData?.isOnfidoSetupComplete)} | ||
onBackButtonPress={goBack} | ||
/> | ||
); | ||
case CONST.BANK_ACCOUNT.STEP.COMPANY: | ||
return <CompanyStep onBackButtonPress={goBack} />; | ||
case CONST.BANK_ACCOUNT.STEP.BENEFICIAL_OWNERS: | ||
return <BeneficialOwnersStep onBackButtonPress={goBack} />; | ||
case CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT: | ||
return <ACHContractStep onBackButtonPress={goBack} />; | ||
case CONST.BANK_ACCOUNT.STEP.VALIDATION: | ||
return <ConnectBankAccount onBackButtonPress={goBack} />; | ||
case CONST.BANK_ACCOUNT.STEP.ENABLE: | ||
return ( | ||
<EnableBankAccount | ||
reimbursementAccount={reimbursementAccount} | ||
onBackButtonPress={goBack} | ||
/> | ||
); | ||
default: | ||
return null; | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason these aren't also CONST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, it was implemented like that before, I've just taken this function out of the component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not a blocker for now, we can add consts in future PRs if we think it would be cleaner