[$1000] Web - App allows to move to next step even when we keep account no and routing number same

[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!

---

Action Performed:

1. Open the app
2. Open settings->workspaces->any workspace->bank account->connect manually
3. Enter routing number and account number (keep it different for now) and click continue
4. Fill in details on step 2 and continue
5. Copy link of step 3, go back to step 1 and make account number same as routing number and click on left hand side to close right hand menu (ensure that error is not displayed when we close the right hand menu)
6. Paste and open copied link, fill out step 3 fields and click continue
7. Observe that it allows us to move to next step even though account no and routing number is same, to cross check, use app back button to revisit step 1

Expected Result:

App should not allow to save and move to next step if account no and routing no are same

Actual Result:

App allows to save and move to next step even if account no and routing no are same if we open step 3 by deep link and if we close the right hand menu before app triggers the error on step 1

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: v1.3.43-6
Reproducible in staging?: y
Reproducible in production?: y
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

save.to.next.step.even.on.same.routing.account.no.mp4

Recording.3836.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689931285240149

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012c191ec47caa63f3
• Upwork Job ID: 1683438280951832576
• Last Price Increase: 2023-07-31

[melvin-bot]

Triggered auto assignment to @laurenreidexpensify (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

[samh-nl]

Proposal

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

The routing number and account number can currently be provided as the same values when connecting a BA.

What is the root cause of that problem?

The validation function in the BankAccountManualStep component checks whether the routing number and the account number are the same, however it uses the translated error key translate('bankAccount.error.routingAndAccountNumberCannotBeSame'); instead of the error key itself.

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Lines 37 to 46 in cd851d0

	const routingNumber = values.routingNumber && values.routingNumber.trim();
	if (
	!values.accountNumber ||
	(!CONST.BANK_ACCOUNT.REGEX.US_ACCOUNT_NUMBER.test(values.accountNumber.trim()) && !CONST.BANK_ACCOUNT.REGEX.MASKED_US_ACCOUNT_NUMBER.test(values.accountNumber.trim()))
	) {
	errorFields.accountNumber = 'bankAccount.error.accountNumber';
	} else if (values.accountNumber === routingNumber) {
	errorFields.accountNumber = translate('bankAccount.error.routingAndAccountNumberCannotBeSame');
	}

In addition, we compare the trimmed routing number with the as-entered account number, this inconsistency can lead to comparison problems.

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

1. Return the error key bankAccount.error.routingAndAccountNumberCannotBeSame and not the translated value.
2. Trim both values when comparing for consistency (or alternatively, trim none and compare raw values).

Regarding the latter change, it should be noted that Form already passes trimmed string values to the validator, so trimming seems redundant here:

App/src/components/Form.js

Lines 111 to 120 in cd851d0

	const onValidate = useCallback(
	(values) => {
	const trimmedStringValues = {};
	_.each(values, (inputValue, inputID) => {
	if (_.isString(inputValue)) {
	trimmedStringValues[inputID] = inputValue.trim();
	} else {
	trimmedStringValues[inputID] = inputValue;
	}
	});

What alternative solutions did you explore? (Optional)

N/A

[rayane-d]

Proposal

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

The app currently allows moving to the next step even when the account number and routing number are the same. This behavior is not desired as it can lead to incorrect data being saved.

What is the root cause of that problem?

The submission validation logic in the app's BankAccountManualStep component does not include a check for the account number and routing number being the same. As a result, the form can be submitted without displaying an error message in this scenario.

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

To solve this problem, we propose making the following changes:

In the BankAccountManualStep.js file Inside the "submit" function, add a check to compare the account number and routing number. If they are the same, return an error object with the corresponding error message.
Update the error object to include the error message for the account number and routing number being the same.

Proposed changes:

const submit = useCallback(
    (values) => {
        if (values.accountNumber === values.routingNumber) {
            // Display an error message to the user
            showBankAccountFormValidationError('Account number and routing number cannot be the same.');
            return;
        }

        BankAccounts.connectBankAccountManually(
            lodashGet(reimbursementAccount, ['achData.bankAccountID']) || 0,
            values.accountNumber,
            values.routingNumber,
            lodashGet(reimbursementAccountDraft, ['plaidMask']),
        );
    },
    [reimbursementAccount, reimbursementAccountDraft],
);

By implementing this change, the app will prevent the form from being submitted and display an error message to the user if the account number and routing number are the same. This will ensure that correct data is entered and saved in the app.

What alternative solutions did you explore? (Optional)

None.

[bernhardoj]

Proposal

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

After filling in a valid routing and account number and completing step 2, we can go back to step 1 and type an invalid routing and account number, in this case, the routing and account number are the same. The invalid routing and account number are actually just a draft because we are not pressing Continue to submit it, so technically it won't be used when creating the bank account, but it is still confusing to the user.

However, the real problem here is that we can edit the routing and account number after completing the 2nd step. Previously, we are not able to edit both the routing and account numbers after completing 2nd step.

What is the root cause of that problem?

This is a regression from #22245. On the bank account page, we check if the reimbursement account data contains a bankAccountID, and we disable both routing and account numbers. bankAccountID is available as soon as we complete step 2.

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Line 71 in cd851d0

const shouldDisableInputs = Boolean(lodashGet(reimbursementAccount, ['achData.bankAccountID']));

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Lines 98 to 104 in cd851d0

	inputID="routingNumber"
	label={translate('bankAccount.routingNumber')}
	accessibilityLabel={translate('bankAccount.routingNumber')}
	accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
	defaultValue={props.getDefaultStateForField('routingNumber', '')}
	keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
	disabled={shouldDisableInputs}

But as you can notice, the way the bankAccountID is retrieved using lodash is wrong.

In lodash, if we use [], then we should separate each property with a comma, for example ['achData', 'bankAccountID'].

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

We just need to revert the lodash usage to the previous version
https://github.com/Expensify/App/pull/22245/files#diff-ae92657199968d4150382ec7b55291c76fa9824f40f373848899c8c7eb84afb6L68

which does not use the array.
lodashGet(reimbursementAccount, 'achData.bankAccountID')

[melvin-bot]

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

[melvin-bot]

Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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