[HOLD for payment 2024-03-07] [$500] Bank account - Selected bank account is not checked in bank account selection step

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: v1.4.44-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal team

Action Performed:

1. Go to staging.new.expensify.com.
2. Go to workspace settings.
3. Select a workspace.
4. Go to Bank account.
5. Click Connect online with Paid.
6. Log in with Chase account.
7. Back in bank account selection page, select any bank account.

Expected Result:

The selected bank account is checked in the checkbox.

Actual Result:

The selected bank account is not checked in the checkbox.
This issue also occurs with the checkbox in Company owner steps.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6393828_1709031318790.20240227_074738.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f67808490772984d
• Upwork Job ID: 1762476893193760768
• Last Price Increase: 2024-02-27
• Automatic offers:
• jjcoffee | Reviewer | 0
• ZhenjaHorbach | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

👋 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 @flodnv (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #vip-bills
CC @davidcardoza

[eucool]

Proposal

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

Selected bank account is not checked in bank account selection step

What is the root cause of that problem?

We forget to pass isChecked prop:

App/src/components/RadioButtons.tsx

Line 58 in cfa0ae3

isChecked={item.value === checkedValue}

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

Pass isChecked prop below:

App/src/components/AddPlaidBankAccount.js

Lines 298 to 304 in cfa0ae3

	<Text style={[styles.textLabelSupporting]}>{`${translate('bankAccount.chooseAnAccountBelow')}:`}</Text>
	<RadioButtons
	items={options}
	defaultCheckedValue={defaultSelectedPlaidAccountID}
	onPress={handleSelectingPlaidAccount}
	radioButtonStyle={[styles.mb6]}
	/>

What alternative solutions did you explore? (Optional)

N/A

[flodnv]

Probably good, do you agree with the fix @jjcoffee @roryabraham @danieldoglas ?

[danieldoglas]

That sounds good to me as well, I noticed that happening on #37271 as well, but the error logged is different there.

[ZhenjaHorbach]

Actually problem related with this PR
#34925

Proposal

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

Bank account - Selected bank account is not checked in bank account selection step

What is the root cause of that problem?

Since we added useEffect in this PR
#34925

App/src/components/RadioButtons.tsx

Lines 42 to 47 in 22b6390

	useEffect(() => {
	if (value === checkedValue) {
	return;
	}
	setCheckedValue(value ?? '');
	}, [checkedValue, value]);

but don't pass value for RadioButtons like prop

As result every time when we change checkedValue we set checkedValue as empty line

Because for all cases value(undefined) is not equal checkedValue

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

To fix this issue we can don't change checkedValue inside useEffect if value is undefined

App/src/components/RadioButtons.tsx

Lines 42 to 47 in 22b6390

	useEffect(() => {
	if (value === checkedValue) {
	return;
	}
	setCheckedValue(value ?? '');
	}, [checkedValue, value]);

And as additional information
Probably we are expected to pass value inside AddPlaidBankAccount using selectedPlaidAccountID

App/src/components/AddPlaidBankAccount.js

Line 89 in 4059b7f

selectedPlaidAccountID,

App/src/components/AddPlaidBankAccount.js

Lines 299 to 304 in 4059b7f

	<RadioButtons
	items={options}
	defaultCheckedValue={defaultSelectedPlaidAccountID}
	onPress={handleSelectingPlaidAccount}
	radioButtonStyle={[styles.mb6]}
	/>

What alternative solutions did you explore? (Optional)

As alternative, we can add value like prop for all places where we use RadioButtons

Plus I think we need refactor this component and don't use defaultCheckedValue
Because defaultCheckedValue and value needed for change current state from parent component

And to me it looks like duplication of props

[danieldoglas]

@ZhenjaHorbach hmm thanks for checking that.

Yeah, thanks for pointing out that pull request.

I think checking if the value exists makes sense since most of the forms are not using value outside of the form like it's the case with the exit survey and that property was just implemented. @roryabraham @mollfpr what do you think?

[jjcoffee]

@ZhenjaHorbach's proposal LGTM and makes sense that it comes from #34925 given this is only happening on staging. Also agree that checking if value is undefined makes sense here as value is an optional prop that's meant to only be used if using the component as a controlled input (i.e. it should work as expected without it!).

App/src/components/RadioButtons.tsx

Lines 35 to 36 in 22b6390

	/** The checked value, if you're using this component as a controlled input. */
	value?: string;

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @flodnv is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[flodnv]

Leaving this to you @roryabraham since you originally wrote the code

[roryabraham]

whoops, my mistake. I made RadioButtons a controlled component without refactoring other usages to be controlled. I like @ZhenjaHorbach's proposal to make it work as controlled or uncontrolled, but I think that we can leave defaultCheckedValue as it still has a utility for parent components using RadioButtons as an uncontrolled component

[melvin-bot]

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

Offer link
Upwork job

[melvin-bot]

📣 @ZhenjaHorbach 🎉 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 📖

[roryabraham]

btw @ZhenjaHorbach, are you in the #expensify-open-source slack room?

[ZhenjaHorbach]

btw @ZhenjaHorbach, are you in the #expensify-open-source slack room?

Yeah, sure )

[roryabraham]

what's your Slack handle?

[ZhenjaHorbach]

📣 @ZhenjaHorbach 🎉 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 📖

Pr will be ready today

[ZhenjaHorbach]

what's your Slack handle?

HorbachY

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.45-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix bug with Selected bank account is not checked in bank account selection step #37342

If no regressions arise, payment will be issued on 2024-03-07. 🎊

For reference, here are some details about the assignees on this issue:

• @jjcoffee requires payment automatic offer (Reviewer)
• @ZhenjaHorbach requires payment automatic offer (Contributor)

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: Mandatory exit survey for users going back to OldDot #34925
• 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: https://github.com/Expensify/App/pull/34925/files#r1515894840
• 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: N/A
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression Test Proposal

1. Go to workspace settings
2. Select a workspace
3. Go to Bank account
4. Click Connect online with Paid
5. Log in with Chase test account details
6. Back in bank account selection page, select any bank account
7. Verify that selected bank account is checked in the checkbox

Do we agree 👍 or 👎

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @Christinadobrzyn you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[Christinadobrzyn]

Payouts due:

Contributor: $500 @ZhenjaHorbach (paid in Upwork - https://www.upwork.com/nx/wm/workroom/36490106/overview)
Contributor+: $500 @jjcoffee (paid in Upwork - https://www.upwork.com/nx/wm/workroom/36497814/overview

Upwork job is here.

Regression test here - https://github.com/Expensify/Expensify/issues/376594

closing