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 unexpected back navigation in Bank Connect flow #24480

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion src/ROUTES.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ export default {
BANK_ACCOUNT_NEW: 'bank-account/new',
BANK_ACCOUNT_WITH_STEP_TO_OPEN: 'bank-account/:stepToOpen?',
BANK_ACCOUNT_PERSONAL: 'bank-account/personal',
getBankAccountRoute: (stepToOpen = '', policyID = '') => `bank-account/${stepToOpen}?policyID=${policyID}`,
getBankAccountRoute: (stepToOpen = '', policyID = '', backTo = '') => {
const backToParam = backTo ? `&backTo=${encodeURIComponent(backTo)}` : '';
return `bank-account/${stepToOpen}?policyID=${policyID}${backToParam}`;
},
HOME: '',
SETTINGS: 'settings',
SETTINGS_PROFILE: 'settings/profile',
Expand Down
4 changes: 3 additions & 1 deletion src/components/ConnectBankAccountButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import compose from '../libs/compose';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import networkPropTypes from './networkPropTypes';
import Text from './Text';
import Navigation from '../libs/Navigation/Navigation';

const propTypes = {
...withLocalizePropTypes,
Expand All @@ -29,14 +30,15 @@ const defaultProps = {
};

function ConnectBankAccountButton(props) {
const activeRoute = Navigation.getActiveRoute().replace(/\?.*/, '');
return props.network.isOffline ? (
<View style={props.style}>
<Text>{`${props.translate('common.youAppearToBeOffline')} ${props.translate('common.thisFeatureRequiresInternet')}`}</Text>
</View>
) : (
<Button
text={props.translate('workspace.common.connectBankAccount')}
onPress={() => ReimbursementAccount.navigateToBankAccountRoute(props.policyID)}
onPress={() => ReimbursementAccount.navigateToBankAccountRoute(props.policyID, activeRoute)}
icon={Expensicons.Bank}
style={props.style}
iconStyles={[styles.buttonCTAIcon]}
Expand Down
7 changes: 4 additions & 3 deletions src/libs/actions/ReimbursementAccount/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ function goToWithdrawalAccountSetupStep(stepID, newAchData) {
/**
* Navigate to the correct bank account route based on the bank account state and type
*
* @param {String} policyId
* @param {string} policyId - The policy ID associated with the bank account.
* @param {string} [backTo=''] - An optional return path. If provided, it will be URL-encoded and appended to the resulting URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's good we have the backTo property described here, but now this JSDoc is inconsistent because the policyId has no description - could we add it?

*/
function navigateToBankAccountRoute(policyId) {
Navigation.navigate(ROUTES.getBankAccountRoute('', policyId));
function navigateToBankAccountRoute(policyId, backTo) {
Navigation.navigate(ROUTES.getBankAccountRoute('', policyId, backTo));
}

export {goToWithdrawalAccountSetupStep, navigateToBankAccountRoute};
4 changes: 4 additions & 0 deletions src/pages/ReimbursementAccount/ContinueBankAccountSetup.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ const propTypes = {
/* The workspace name */
policyName: PropTypes.string,

/** Goes to the previous step */
onBackButtonPress: PropTypes.func.isRequired,

...withLocalizePropTypes,
};

Expand All @@ -43,6 +46,7 @@ function ContinueBankAccountSetup(props) {
title={props.translate('workspace.common.connectBankAccount')}
subtitle={props.policyName}
shouldShowGetAssistanceButton
onBackButtonPress={props.onBackButtonPress}
Copy link
Contributor

@burczu burczu Aug 16, 2023

Choose a reason for hiding this comment

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

The onBackButtonPress prop is missing in propTypes of this component

guidesCallTaskID={CONST.GUIDES_CALL_TASK_IDS.WORKSPACE_BANK_ACCOUNT}
/>
<ScrollView style={styles.flex1}>
Expand Down
14 changes: 10 additions & 4 deletions src/pages/ReimbursementAccount/ReimbursementAccountPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,9 @@ class ReimbursementAccountPage extends React.Component {
// When the step changes we will navigate to update the route params. This is mostly cosmetic as we only use
// the route params when the component first mounts to jump to a specific route instead of picking up where the
// user left off in the flow.
Navigation.navigate(ROUTES.getBankAccountRoute(this.getRouteForCurrentStep(currentStep), lodashGet(this.props.route.params, 'policyID')));
const backTo = lodashGet(this.props.route.params, 'backTo');
const policyId = lodashGet(this.props.route.params, 'policyID');
Navigation.navigate(ROUTES.getBankAccountRoute(this.getRouteForCurrentStep(currentStep), policyId, backTo));
}

/*
Expand Down Expand Up @@ -280,6 +282,7 @@ class ReimbursementAccountPage extends React.Component {
const currentStep = achData.currentStep || CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT;
const subStep = achData.subStep;
const shouldShowOnfido = this.props.onfidoToken && !achData.isOnfidoSetupComplete;
const backTo = lodashGet(this.props.route.params, 'backTo');
switch (currentStep) {
case CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT:
if (this.hasInProgressVBBA()) {
Expand All @@ -288,7 +291,7 @@ class ReimbursementAccountPage extends React.Component {
if (subStep) {
BankAccounts.setBankAccountSubStep(null);
} else {
Navigation.goBack();
Navigation.goBack(backTo);
}
break;
case CONST.BANK_ACCOUNT.STEP.COMPANY:
Expand All @@ -313,11 +316,11 @@ class ReimbursementAccountPage extends React.Component {
shouldShowContinueSetupButton: true,
});
} else {
Navigation.goBack();
Navigation.goBack(backTo);
}
break;
default:
Navigation.goBack();
Navigation.goBack(backTo);
}
}

Expand Down Expand Up @@ -399,6 +402,9 @@ class ReimbursementAccountPage extends React.Component {
reimbursementAccount={this.props.reimbursementAccount}
continue={this.continue}
policyName={policyName}
onBackButtonPress={() => {
Navigation.goBack(lodashGet(this.props.route.params, 'backTo'));
}}
/>
);
}
Expand Down
5 changes: 4 additions & 1 deletion src/pages/workspace/WorkspaceInitialPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,10 @@ function WorkspaceInitialPage(props) {
{
translationKey: 'workspace.common.bankAccount',
icon: Expensicons.Bank,
action: () => (policy.outputCurrency === CONST.CURRENCY.USD ? ReimbursementAccount.navigateToBankAccountRoute(policy.id) : setIsCurrencyModalOpen(true)),
action: () =>
policy.outputCurrency === CONST.CURRENCY.USD
? ReimbursementAccount.navigateToBankAccountRoute(policy.id, Navigation.getActiveRoute().replace(/\?.*/, ''))
: setIsCurrencyModalOpen(true),
brickRoadIndicator: !_.isEmpty(props.reimbursementAccount.errors) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '',
},
];
Expand Down