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

fix: restart the flow for another policy #49687

14 changes: 13 additions & 1 deletion src/libs/actions/ReimbursementAccount/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ function updateReimbursementAccountDraft(bankAccountData: Partial<ReimbursementA
Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {draftStep: undefined});
}

function clearReimbursementAccountDraft() {
Onyx.set(ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM_DRAFT, {});
}

/**
* Triggers a modal to open allowing the user to reset their bank account
*/
Expand All @@ -40,4 +44,12 @@ function cancelResetFreePlanBankAccount() {
Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {shouldShowResetModal: false});
}

export {resetFreePlanBankAccount, setBankAccountSubStep, hideBankAccountErrors, updateReimbursementAccountDraft, requestResetFreePlanBankAccount, cancelResetFreePlanBankAccount};
export {
resetFreePlanBankAccount,
setBankAccountSubStep,
hideBankAccountErrors,
updateReimbursementAccountDraft,
requestResetFreePlanBankAccount,
cancelResetFreePlanBankAccount,
clearReimbursementAccountDraft,
};
4 changes: 4 additions & 0 deletions src/pages/ReimbursementAccount/BankInfo/substeps/Manual.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ function Manual({onNext}: ManualProps) {
[BANK_INFO_STEP_KEYS.ACCOUNT_NUMBER]: reimbursementAccount?.achData?.[BANK_INFO_STEP_KEYS.ACCOUNT_NUMBER] ?? '',
};

const shouldBeReadOnlyInput = reimbursementAccount?.achData?.setupType === CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID;

const validate = useCallback(
(values: FormOnyxValues<typeof ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM> => {
const errors = ValidationUtils.getFieldRequiredErrors(values, STEP_FIELDS);
Expand Down Expand Up @@ -86,6 +88,7 @@ function Manual({onNext}: ManualProps) {
inputMode={CONST.INPUT_MODE.NUMERIC}
shouldSaveDraft
shouldUseDefaultValue={hasBankAccountData}
disabled={shouldBeReadOnlyInput}
/>
<InputWrapper
InputComponent={TextInput}
Expand All @@ -98,6 +101,7 @@ function Manual({onNext}: ManualProps) {
inputMode={CONST.INPUT_MODE.NUMERIC}
shouldSaveDraft
shouldUseDefaultValue={hasBankAccountData}
disabled={shouldBeReadOnlyInput}
/>
</FormProvider>
);
Expand Down
244 changes: 109 additions & 135 deletions src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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'];
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

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
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@koko57 Why do you remove this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Night owl 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@koko57 I mean should we replace with reimbursementAccount?.isLoading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

temporarily different timezone 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

@koko57 Bump on this

Copy link
Contributor Author

@koko57 koko57 Oct 2, 2024

Choose a reason for hiding this comment

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

    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;
    });

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand All @@ -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;
Expand Down Expand Up @@ -296,7 +281,6 @@ function ReimbursementAccountPage({route, policy}: ReimbursementAccountPageProps
BankAccounts.setBankAccountSubStep(CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL).then(() => {
setShouldShowContinueSetupButton(false);
});
fetchData(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

@koko57 same here, Why do you remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 = () => {
Expand Down Expand Up @@ -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 &&
Expand Down Expand Up @@ -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;
}
}

Expand Down
Loading
Loading