[HOLD for Payment 2024-10-08][$250] Bank account - Double RHP animation when clicking on "connect bank accont"

[IuliiaHerets]

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

---

Version Number: 9.0.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com with an Expensifail account
2. Create a workspace and enable workflows
3. Go to workflow
4. Click on connect to bank account

Expected Result:

RHP animation gets shown only once

Actual Result:

RHP animation gets shown twice

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6608862_1726749444034.bandicam_2024-09-19_15-31-14-322.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021837132579404847952
• Upwork Job ID: 1837132579404847952
• Last Price Increase: 2024-09-20

[melvin-bot]

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[Krishna2323]

Proposal


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

Bank account - Double RHP animation when clicking on "connect bank accont"

What is the root cause of that problem?

• Due to the useEffect below double navigation is happening.
• currentStep is CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT by default.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx

Line 218 in 25450d9

const currentStep = achData?.currentStep || CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT;

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx

Lines 312 to 322 in 25450d9

	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();
	}
	const policyID = route.params.policyID;
	const backTo = route.params.backTo;
	Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute(getRouteForCurrentStep(currentStep), policyID, backTo));

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


We should prevent calling the navigation in the useEffect if the backTo route is backTo === ROUTES.WORKSPACE_WORKFLOWS.getRoute(policyID ?? '')

What alternative solutions did you explore? (Optional)

Result

[nkdengineer]

Proposal

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

RHP animation gets shown twice

What is the root cause of that problem?

After we navigate to the route BANK_ACCOUNT_WITH_STEP_TO_OPEN here, we are setting stepToOpen to an empty string. Then, we navigate a second time with stepToOpen set to getRouteForCurrentStep(currentStep) in this useEffect, which causes a double RHP animation.

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

Lines 20 to 22 in 25450d9

	function navigateToBankAccountRoute(policyID: string, backTo?: string) {
	Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute('', policyID, backTo));
	}

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

We should modify stepToOpen when we call the navigateToBankAccountRoute function, and I think it should default to ROUTE_NAMES.NEW as we are doing here.

function navigateToBankAccountRoute(policyID: string, backTo?: string) {
    Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute(ROUTE_NAMES.NEW, policyID, backTo));
}

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

Lines 20 to 22 in 25450d9

	function navigateToBankAccountRoute(policyID: string, backTo?: string) {
	Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute('', policyID, backTo));
	}

What alternative solutions did you explore? (Optional)

[cretadn22]

Proposal

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

Double RHP animation when clicking on "connect bank account"

What is the root cause of that problem?

On the Workflow page, we navigate to the ReimbursementAccountPage with an empty string for stepToOpen.

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Line 236 in 25450d9

navigateToBankAccountRoute(route.params.policyID, ROUTES.WORKSPACE_WORKFLOWS.getRoute(route.params.policyID));

Then, another navigation is triggered here because the currentStep in Onyx and the step in the route are different.

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

In the navigateToBankAccountRoute function, we introduced a new parameter stepToOpen

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

Line 20 in 25450d9

function navigateToBankAccountRoute(policyID: string, backTo?: string) {

function navigateToBankAccountRoute(stepToOpen: string, policyID: string, backTo?: string) {
    Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute(stepToOpen ?? '', policyID, backTo));
}

we pass stepToOpen as ROUTE_NAMES.NEW here, we also need to update other areas that use this function.

Or even we could eliminate the navigateToBankAccountRoute function and use Navigation.navigate directly with the correct params

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@garrettmknight, @mollfpr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[bernhardoj]

Proposal

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

The connect bank account page slide animation isn't smooth.

What is the root cause of that problem?

On the bank account page, we have a navigation code that will be called whenever the step is changed.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx

Line 322 in a7a408f

Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute(getRouteForCurrentStep(currentStep), policyID, backTo));

When we open the bank account page, the step is initially empty and will be updated to /new which is mapped to BankAccountStep that shows whether to connect manually or with Plaid. If there is an existing progress, then the step won't be updated to /new.

The navigation code is there to update the step params of the bank account page.

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

Instead of using navigate to update the params, we can use setParams.

Navigation.setParams({stepToOpen: getRouteForCurrentStep(currentStep)});

[mollfpr]

We should prevent calling the navigation in the useEffect if the backTo route is backTo === ROUTES.WORKSPACE_WORKFLOWS.getRoute(policyID ?? '')

@Krishna2323 The comparison doesn't make sense to me, we could have different backTo values in the future.

---

We should modify stepToOpen when we call the navigateToBankAccountRoute function, and I think it should default to ROUTE_NAMES.NEW as we are doing here.

@nkdengineer When the user navigates to connect a bank account it shows the continue setup or start over, there's no step decided yet, so it makes sense to start with an empty string.

@cretadn22 It Seems your solution is similar but has a different implementation.

---

I think the proposal from @bernhardoj makes sense here and the double animation is not happening. We still have the correct route set up.

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[dangrous]

Works for me!

[bernhardoj]

PR is ready

cc: @mollfpr

[garrettmknight]

PR is on staging

[dangrous]

on prod 14 hours ago. Is there like a serious issue with automations right now?

EDIT: yes, but it's being fixed!

[garrettmknight]

Payment Summary:

• Contributor: @bernhardoj $250
• Reviewer: @mollfpr $250

[garrettmknight]

@mollfpr can you complete the checklist please?

[garrettmknight]

Not overdue?

[bernhardoj]

Requested in ND.

[mollfpr]

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

I couldn't determine the offending PR.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

1. Open a workspace and enable workflows (if disabled)
2. Go to workflow
3. Click on connect to bank account
4. Verify there is no double RHP slide animation
5. 👍 or 👎

[JmillsExpensify]

Confirming this is the correct payment summary:

• Contributor: @bernhardoj $250
• Reviewer: @mollfpr $250

[mallenexpensify]

@garrettmknight , for auditing purposes, so that we know if a contributor is paid/due and where they've been paid, can you use the below format, as per the main payment SO

Contributor: @ paid $ via Upwork
Contributor+: @ due $ via NewDot

Thx

[garrettmknight]

$250 approved for @mollfpr

[JmillsExpensify]

$250 approved for @bernhardoj based on this summary.