[$1000] Workspace -User needs to tap ‘Back’ twice to return to previous screen from ‘Connect BA' offline

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open Expensify App / iOS
2. Log in with Expensifail account
3. Create or go to any workspace
4. Go to 'Settings'
5. Go OFFline
6. Go to any screen with 'Connect bank account' feature (e.q. 'Cards')
7. Tap to 'Connect bank account'
8. User moves to screen with info 'Your WS currency set to different currency then USD'
9. Tap 'Back'

Expected Result:

User moves to previous screen

Actual Result:

The user remained on the same screen. To go to the previous screen user needs tap 'Back' again

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.30.3

Reproducible in staging?: Yes

Reproducible in production?: No

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug6102064_WS-offline-back-2times.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bb0f281e371868b5
• Upwork Job ID: 1671972100571131904
• Last Price Increase: 2023-09-27
• Automatic offers:
• bernhardoj | Contributor | 27049536

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @hayata-suenaga (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[hayata-suenaga]

was able to reproduce

RPReplay_Final1687390272.MP4

[hayata-suenaga]

so weird only happens when the device is offline...

[hayata-suenaga]

I edited the Action Performed section of OP because the selection of current doesn't matter for reproduction of the issue

here is the video without the currency warning. the same issue is present even when the current is a default one.

RPReplay_Final1687391044.MP4

[hayata-suenaga]

the issue is also present on web

Screen.Recording.2023-06-21.at.4.47.24.PM.mov

[melvin-bot]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[hayata-suenaga]

The bug doesn't prevent the usage of NewDot, and the problem only happens offline.

I don't believe this is a deploy blocker. Removing DeployBlockerCash label and assigning bug label.

I'll keep assigning myself for the internal engineer assignment when the proposal is accepted.

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We need to back twice from Connect Bank Account if we open it while offline.

What is the root cause of that problem?

When we navigate back, the page will be re-rendered, even though technically no props/state is changed. After digging around and with the help of WDYR, I found that withPolicy HOC triggers the rerender when we navigate back. It's because the useNavigationState hook is triggered when the navigation happens. Because withPolicy rerenders, the child, in this case, ReimbursementAccountPage also gets re-rendered.

App/src/pages/workspace/withPolicy.js

Lines 87 to 103 in 6183609

	function WithPolicy(props) {
	const currentRoute = _.last(useNavigationState((state) => state.routes || []));
	const policyID = getPolicyIDFromRoute(currentRoute);
	if (_.isString(policyID) && !_.isEmpty(policyID)) {
	Policy.updateLastAccessedWorkspace(policyID);
	}
	const rest = _.omit(props, ['forwardedRef']);
	return (
	<WrappedComponent
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...rest}
	ref={props.forwardedRef}
	/>
	);
	}

What happens when the page is re-rendered? componentDidUpdate is called and this part of the code is responsible to bring back the user to the same bank account page.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 156 to 171 in a777544

	const currentStepRouteParam = this.getStepToOpenFromRouteParams();
	if (currentStepRouteParam === currentStep) {
	// The route is showing the correct step, no need to update the route param or clear errors.
	return;
	}
	if (currentStepRouteParam !== '') {
	// When we click "Connect bank account", we load the page without the current step param, if there
	// was an error when we tried to disconnect or start over, we want the user to be able to see the error,
	// so we don't clear it. We only want to clear the errors if we are moving between steps.
	BankAccounts.hideBankAccountErrors();
	}
	// 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')));

As you can notice, we already prevent it if currentStepRouteParam === currentStep.
currentStepRouteParam is from param (stepToOpen), which it's initial value is an empty string.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 202 to 219 in a777544

	getStepToOpenFromRouteParams() {
	switch (lodashGet(this.props.route, ['params', 'stepToOpen'])) {
	case 'new':
	return CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT;
	case 'company':
	return CONST.BANK_ACCOUNT.STEP.COMPANY;
	case 'personal-information':
	return CONST.BANK_ACCOUNT.STEP.REQUESTOR;
	case 'contract':
	return CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT;
	case 'validate':
	return CONST.BANK_ACCOUNT.STEP.VALIDATION;
	case 'enable':
	return CONST.BANK_ACCOUNT.STEP.ENABLE;
	default:
	return '';
	}
	}

App/src/ROUTES.js

Line 21 in a777544

getBankAccountRoute: (stepToOpen = '', policyID = '') => `bank-account/${stepToOpen}?policyID=${policyID}`,

currentStep is from reimbursementAccount.achData.currentStep. We get this from the API and fallback to BankAccountStep.

Because '' !== 'BankAccountStep', the navigation happens and it will now navigate with stepToOpen = new.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 225 to 241 in a777544

	getRouteForCurrentStep(currentStep) {
	switch (currentStep) {
	case CONST.BANK_ACCOUNT.STEP.COMPANY:
	return 'company';
	case CONST.BANK_ACCOUNT.STEP.REQUESTOR:
	return 'personal-information';
	case CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT:
	return 'contract';
	case CONST.BANK_ACCOUNT.STEP.VALIDATION:
	return 'validate';
	case CONST.BANK_ACCOUNT.STEP.ENABLE:
	return 'enable';
	case CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT:
	default:
	return 'new';
	}
	}

That's why we can close it the 2nd time because currentStepRouteParam value is now BankAccountStep.

Why it doesn't happen while online?

currentStepRouteParam initial value is also empty. The difference is, while online we are doing an API requestand componentDidUpdate will be triggered which will execute the navigation code above. This will update currentStepRouteParam to BankAccountStep. So, when we close the page, currentStepRouteParam === currentStep is true and an early return happens.

---

In summary,
If currentStepRouteParam is not equal to currentStep, we will navigate to the bank account page
while offline, currentStepRouteParam is empty and currentStep is BankAccountStep, navigate back will trigger above
while online, currentStepRouteParam is initially empty, but then updated after the API starts and now has a value of BankAccountStep. currentStep is also BankAccountStep. No navigation happens when going back.

What changes do you think we should make in order to solve the problem?

We should prevent the re-rendering when going back. As explained in the root cause, the re-render happens because the navigation state changes.

App/src/pages/workspace/withPolicy.js

Lines 87 to 103 in 6183609

	function WithPolicy(props) {
	const currentRoute = _.last(useNavigationState((state) => state.routes || []));
	const policyID = getPolicyIDFromRoute(currentRoute);
	if (_.isString(policyID) && !_.isEmpty(policyID)) {
	Policy.updateLastAccessedWorkspace(policyID);
	}
	const rest = _.omit(props, ['forwardedRef']);
	return (
	<WrappedComponent
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...rest}
	ref={props.forwardedRef}
	/>
	);
	}

If we look at the code, the purpose of the navigation state is to retrieve the policyID from route params. There are 2 concerns with this:

1. When the navigation state changes (for example navigating to another page), the code will re-run with the wrong route.
2. It's overkill to get route params from the navigation state. We can easily get the route param from useRoute hook

So, I suggest reworking the logic:
We want policyID from route params, but we actually don't need it from route because WithPolicy component is wrapped with withOnyx that provides the policy object.

App/src/pages/workspace/withPolicy.js

Lines 116 to 123 in 6183609

	return withOnyx({
	policy: {
	key: (props) => `${ONYXKEYS.COLLECTION.POLICY}${getPolicyIDFromRoute(props.route)}`,
	},
	policyMembers: {
	key: (props) => `${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${getPolicyIDFromRoute(props.route)}`,
	},
	})(withPolicy);

This means we can simply get the policy id through props.policy.id and remove useNavigationState.

const policyID = props.policy.id;
if (_.isString(policyID) && !_.isEmpty(policyID)) {
    Policy.updateLastAccessedWorkspace(policyID);
}

What alternative solutions did you explore? (Optional)

I put this as an alternative because looks like it's intentional to have an empty stepToOpen initially. If you have completed the first 2 steps of Connect bank account, the page will show the Continue with setup and Start over buttons. On this page, the stepToOpen is an empty string

Instead of empty stepToOpen, we can pass new.

App/src/libs/actions/ReimbursementAccount/navigation.js

Lines 21 to 23 in a777544

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

App/src/libs/ReportUtils.js

Lines 373 to 375 in a777544

	function getBankAccountRoute(report) {
	return isPolicyExpenseChat(report) ? ROUTES.getBankAccountRoute('', report.policyID) : ROUTES.SETTINGS_ADD_BANK_ACCOUNT;
	}

However, I don't know if it's intended to pass an empty stepToOpen because the value is being used in API params.

App/src/libs/actions/BankAccounts.js

Lines 294 to 300 in a777544

	const param = {
	stepToOpen,
	subStep,
	localCurrentStep,
	};
	return API.read('OpenReimbursementAccountPage', param, onyxData);

When we open the page, it will call the API with empty stepToOpen.

2. In componentDidUpdate of ReimbursementAccountPage, if we are offline, we immediately return preventing any other logic to run.

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[hayata-suenaga]

This issue is reproducible. making this issue external cc: @adelekennedy

@bernhardoj thank you so much for the detailed review. If you have time, can you check if this ongoing navigation refactoring will affect your solution?

We might need to put a hold on this cc: @Santhosh-Sellavel

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01bb0f281e371868b5

[melvin-bot]

Triggered auto assignment to @isabelastisser (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[hayata-suenaga]

@ArekChr do we have a proposal that we can go with?

[adelekennedy]

It looks like this is still reproducible, @ArekChr do you have an opinion on any of the proposals above

[hayata-suenaga]

@ArekChr kindly bumping 🙇

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[hayata-suenaga]

bumpbed @ArekChr in the #contributor-plus channel

[narefyev91]

Proposal from @bernhardoj looks good to me #21277 (comment)
The main issue that during offline we will not get correct step to go to - and returning back to the same one.
I will suggest to work on alternative solution - in case making changing for usePolicy make cause a huge amount of regressions
🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @hayata-suenaga is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[adelekennedy]

@hayata-suenaga will review the proposal above (please Melvin, it was the weekend!)

[melvin-bot]

@ArekChr, @adelekennedy, @hayata-suenaga Whoops! This issue is 2 days overdue. Let's get this updated quick!

[hayata-suenaga]

ah that's a very simple solution if we can get the ID from the Onyx data we already subscribe. Let's go with @bernhardoj 🚀

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

Now I'm unable to reproduce it anymore after this PR. Do we still want to fix the withPolicy? If not, then I think we can close this.

@hayata-suenaga

[hayata-suenaga]

if the issue has been fixed, I don't think we need to create a PR for this.

@adelekennedy please check if the issue reporter needs to be paid 🙇 Otherwise this PR can be closed

[peterdbarkerUK]

Hey hey - I made a mistake in paying a reporter bounty of $50. It was for this other issue, but was tied to this job in upwork. No action required, just leaving a paper trail.

[adelekennedy]

This was applause internal so no payments due - @peterdbarkerUK @bernhardoj I'm going to end the contract but will not request a refund (I think the $50 doesn't require a refund here)