[HOLD for payment 2022-11-29] [$250] First field is not focused after clicking the fix the errors link on Connect Manually page reported by @parasharrajat

[kavimuru]

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 on web.
2. Go to Settings.
3. Click on any workspace(if there are not workspace create new).
4. Go to Connect Bank Account menu.
5. Select Connect Manually.
6. Press save & Continue without entering any information on any field.
7. fix the errors shows up above the save & continue button.
8. Click on the link.
9. Observe that all the fields are red but focus is set to Account number Field even if there is a error field above it.

Expected Result:

Focus should be set on Routing Number field.

Actual Result:

When all fields are errored, focus is not set on the first field called Routing Number.

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.22-3
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/199597236-7faabc85-1baf-4b22-ba6c-05bf4aa06abc.mp4
Expensify/Expensify Issue URL:
Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665921536612629

View all open jobs on GitHub

[melvin-bot]

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

[gadhiyamanan]

Proposal
we need to rearrange errorFields as below. add routingNumber validation before accountNumber validation

Before :

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Lines 36 to 54 in 25c3060

	validate(values) {
	const errorFields = {};
	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 = this.props.translate('bankAccount.error.accountNumber');
	}
	if (!routingNumber || !CONST.BANK_ACCOUNT.REGEX.SWIFT_BIC.test(routingNumber) || !ValidationUtils.isValidRoutingNumber(routingNumber)) {
	errorFields.routingNumber = this.props.translate('bankAccount.error.routingNumber');
	}
	if (!values.acceptedTerms) {
	errorFields.acceptedTerms = this.props.translate('common.error.acceptedTerms');
	}
	return errorFields;
	}

After:

    validate(values) {
        const errorFields = {};
        const routingNumber = values.routingNumber && values.routingNumber.trim();

        if (!routingNumber || !CONST.BANK_ACCOUNT.REGEX.SWIFT_BIC.test(routingNumber) || !ValidationUtils.isValidRoutingNumber(routingNumber)) {
            errorFields.routingNumber = this.props.translate('bankAccount.error.routingNumber');
        }
        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 = this.props.translate('bankAccount.error.accountNumber');
        }
        if (!values.acceptedTerms) {
            errorFields.acceptedTerms = this.props.translate('common.error.acceptedTerms');
        }

        return errorFields;
    }

it's because we have set focus on the first errorfield in From.js

App/src/components/Form.js

Lines 230 to 232 in 25c3060

	onFixTheErrorsLinkPressed={() => {
	this.inputRefs[_.first(_.keys(this.state.errors))].focus();
	}}

AFfter Fix :

Simulator.Screen.Recording.-.iPhone.13.mini.-.2022-11-03.at.17.07.59.mp4

[zanyrenney]

Sorry @hungvu193 i'm not clear if you are saying this bug Will be fixed as part of that proposal above, or if that proposal above is a fix for this specific bug?

[hungvu193]

@zanyrenney my bad. I misunderstood the description, I deleted it since there was a good proposal above.

[rushatgabhane]

@gadhiyamanan @hungvu193 can we have a solution that doesn't need rearrangement of the if conditions?

Because it's really easy to make this bug happen again.

[hungvu193]

@rushatgabhane Yeah, we can update the way we focus on field in:

App/src/components/Form.js

Lines 230 to 232 in 25c3060

	onFixTheErrorsLinkPressed={() => {
	this.inputRefs[_.first(_.keys(this.state.errors))].focus();
	}}

 onFixTheErrorsLinkPressed={() => { 
-      this.inputRefs[_.first(_.keys(this.state.errors))].focus(); 
+     this.findFirstError()
 }} 

 findFirstError = () => {
        const errorArrays = _.keys(this.state.errors);
        const inputArrays = _.keys(this.inputRefs);
        let currentFocusKey = null;
        _.every(inputArrays, (currentFieldKey) => {
            const errorIndex = _.indexOf(errorArrays, currentFieldKey);
            if (errorIndex !== -1) {
                currentFocusKey = currentFieldKey;
                return false;
            }
            return true;
        });
        if (currentFocusKey) {
            this.inputRefs[currentFocusKey].focus();
        }
    }

Screen.Recording.2022-11-06.at.17.29.05.mov

[rushatgabhane]

Fantastic, that's a great solution!

[hungvu193]

There are similar error reported here: #12436
That because we are using different Form components for different page. And each form has different way to handle press Fix Error. We should avoid that.

[0xmiros]

Proposal

Root cause:

App/src/components/Form.js

Line 231 in 0724337

this.inputRefs[_.first(_.keys(this.state.errors))].focus();

This picks first item of errors json. This means focus priority depends on keys sorting in errors. This is incorrect.
Focus priority should depend on keys sorting in this.inputRefs <=> views order (from top to bottom)
i.e. on Connect bank account page, these are priority order in view: Routing number, Account number, Terms

Solution:
find inputRef by looping this.inputRefs from first to last which has the error

                            onFixTheErrorsLinkPressed={() => {
-                               this.inputRefs[_.first(_.keys(this.state.errors))].focus();
+                               const focusKey = _.find(_.keys(this.inputRefs), key => _.keys(this.state.errors).includes(key));
+                               this.inputRefs[focusKey].focus();
                            }}

[parasharrajat]

@0xmiroslav Nice one. Exactly the way I was thinking (mount order).

[melvin-bot]

@zanyrenney Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[zanyrenney]

Looks like we have a proposal - @ctkochan22 let me know what you think! Upwork posting is here:https://www.upwork.com/jobs/~01a2007b1a50c1fece

[Santhosh-Sellavel]

@hungvu193 @0xmiroslav I tested both of your proposals both are working for the connect manually page. But not working for other pages
example for company setup page,

Screen.Recording.2022-11-09.at.6.31.15.AM.mov

[hungvu193]

There are similar error reported here: #12436
That because we are using different Form components for different page. And each form has different way to handle press Fix Error. We should avoid that.

As I mentioned here, they are using different Form component, I think we have a PR to refactor that already

[0xmiros]

@Santhosh-Sellavel I think we don't need care about components that don't use Form yet.

There are several form refactoring for components work in progress:
ACHContractStep
CompanyStep
RequestorStep
IdentityForm

These are done:
RequestCallPage
AddPersonalBankAccountPage
BankAccountStep
AddressForm
WorkspaceSettingsPage
so you can test proposal on these components

[Santhosh-Sellavel]

Sorry for the delay, @ctkochan22.
Both proposals are working!
But I like @0xmiroslav's proposals, here!
C+ Reviewed
🎀 👀 🎀

[zanyrenney]

Hmm, I don't see the regression test linked - @ctkochan22 @Santhosh-Sellavel any idea where I'd find that?

[Santhosh-Sellavel]

@ctkochan22 bump!

[ctkochan22]

Added

[luacmartins]

I'm not sure that the offending PR is correct. The feature was introduced in the linked PR and worked well. AFAIK we had quite a few regressions on this functionality along the way, e.g. #11424 and #11729

[Santhosh-Sellavel]

@zanyrenney This is due for payment, also the PR is eligible for %50 bonus as it was merged in 3 biz days!

[parasharrajat]

Thanks for the ping. I almost forgot about it. I think when this issue was solved, %50 bonus was not active. But it could be.

[Santhosh-Sellavel]

@parasharrajat Bonus was active on issue hires made after Nov 4. As we can see they are hired on Nov 11 Friday. Merged within the next 3 business days i/e by Nov 16. So it's eligible for a bonus!

[ctkochan22]

Sorry, was ooo last week on a family emergency. Looks like we need to add a regression test. Looking up how to do that now

[zanyrenney]

Paid and included Bonus @0xmiroslav

[zanyrenney]

oops, missed some payments here. Reopening!

[zanyrenney]

https://www.upwork.com/jobs/~01a2007b1a50c1fece - i need you to apply for the job @parasharrajat @Santhosh-Sellavel so I can pay you.

[0xmiros]

@zanyrenney I think you paid $50 more. Can I consider and reduce it in next payment for any job?
or shall I refund here? (I don't suggest this)

[zanyrenney]

thanks for the catch @0xmiroslav - i'm chatting to my teammate about how best to do this.

[zanyrenney]

@0xmiroslav our policy is to request the refund through Upwork so we've done that! Lmk any issues or questions please.

[0xmiros]

@michaelhaxhiu @zanyrenney refunded

[zanyrenney]

@Santhosh-Sellavel @parasharrajat - paid!