[$500] IdentityForm

[neil-marcellini]

Coming from the New Expensify Forms design doc, we should refactor the inputs within the IdentityForm to be compatible with the new Form component, following the guidelines below:

Here's an example of a Form refactor: #9056
Here is a component that had its inputs refactored to be compatible with Form.js.

Guidelines

Contributors should first read FORMS.md.
The refactored inputs should be functional when wrapped by Form.js and when not.

1. Add an optional inputID prop.
2. Add an optional shouldSaveDraft prop that defaults to false.
3. Make the value prop optional.
4. Change any onChange prop, e.g. onTextChange to onChange.
5. In the input’s onBlur method, call props.onBlur().
6. In the input’s onChange method, call props.onChange() .
7. Remove the hasError prop.
8. Update the error message to display if errorText is truthy.
9. Make sure that props.ref is attached to the appropriate DOM node. This could involve forwarding the ref to a child component.
10. Make sure the ref is exposing both value and focus().
11. Add an optional maxLength prop to text inputs and UI to display the current character count out of the limit.
12. Add a hint prop and display the text under the input if there is no existing error.
13. Remove any unused code.

Testing

Verify that:

• UI looks as it did before the refactor
• Values can be added and edited
• Errors are highlighted correctly (input border)
• Error messages show up correctly
• Draft values are saved properly
• Form alerts are displayed correctly
• Clicking the fix the errors link focuses the first input with error
• No duplicate submission of the form occurs (when it's already submitting)

[melvin-bot]

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

[neil-marcellini]

It might be a good idea to price this issue at $500+ right away since it's a precursor to the RequestorStep refactor.

[mollfpr]

@neil-marcellini May I know the plan for deploying this form? Is this form will be deployed before refactoring ACHContractStep and RequestStep?

I also think the proposal should include refactoring AddressForm (since it's been used in IdentityForm).

[neil-marcellini]

Great point about the AddressForm. I created this issue to refactor it #10738. I also updated this description because this refactor will only involve updating the inputs to be compatible with Form.js. Please submit your proposal for AddressForm. Thanks.

[neil-marcellini]

deployed before refactoring ACHContractStep and RequestStep?

Yes. We will start with AddressForm, then IdentityForm, and then RequestorStep. Then eventually ACHContractStep.

[mollfpr]

@neil-marcellini Got it! I'm on my way to send the proposal for AddressForm.

[JmillsExpensify]

Just created an Upwork job here: https://www.upwork.com/jobs/~01604cc324bbf955b6. I went ahead and started the posting at $500. cc @neil-marcellini

[melvin-bot]

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

[melvin-bot]

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

[JmillsExpensify]

Proposals happening in #10738. We're still on hold until that one goes through.

[luacmartins]

I'm ooo until Wednesday. Will come back to this when I'm back!

[melvin-bot]

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

• Refactor identity form #12252
• Refactor identity form #12252

If no regressions arise, payment will be issued on 2022-11-16. 🎊

[JmillsExpensify]

Nice work getting this merged! I'll circle back in the coming days when the regression period has ended.

[Santhosh-Sellavel]

@mollfpr I've just tested a proposal there it's not working on this form.

@mollfpr bump

[mollfpr]

@Santhosh-Sellavel

#12400 (comment) in this test you using CompanyStep where we haven't refactored this form to use Form.js.

App/src/pages/ReimbursementAccount/CompanyStep.js

Line 193 in 9e3a5ab

<ReimbursementAccountForm onSubmit={this.submit}>

App/src/pages/ReimbursementAccount/ReimbursementAccountForm.js

Lines 65 to 67 in 9e3a5ab

	onFixTheErrorsLinkPressed={() => {
	this.form.scrollTo({y: 0, animated: true});
	}}

While this #12400 issue happens in BankAccountManualStep where we use Form.js. You should try to test it in any form that uses Form.js.

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Lines 79 to 85 in 9e3a5ab

	<Form
	formID={ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM}
	onSubmit={this.submit}
	validate={this.validate}
	submitButtonText={this.props.translate('common.saveAndContinue')}
	style={[styles.mh5, styles.flexGrow1]}
	>

App/src/components/Form.js

Lines 230 to 232 in 9e3a5ab

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

[Santhosh-Sellavel]

@mollfpr How to test this IdentityForm against the same issue, Should we have to wait until RequesterStep is complete?

[mollfpr]

@Santhosh-Sellavel IMO we shouldn't. This refactoring doesn't impact the inputRefs state of Form.js.

The current implementation of onFixTheErrorsLinkPressed is to get the input refs from the order of key in this.state.errors.

This issue #12400 occurred because on the validation function of BankAccountManualStep the first to validate is the accountNumber value. So that's why the focus input is the account number field. If we change the order of that validation the routingNumber is the first value to validate, the focus input will be correct to the routing number input.

App/src/pages/ReimbursementAccount/BankAccountManualStep.js

Lines 36 to 54 in 9e3a5ab

	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;
	}

It's already explained in this proposal.

[Santhosh-Sellavel]

@mollfpr I believe you miss understood, I'm not saying to put 12400 on hold until.

I am asking, to test this 'IdentityForm' we should wait until 'RequestorStep' to test whether this works as expected or not against the issue as we didn't complete RequesterStep which uses Identity Form.

[mollfpr]

@Santhosh-Sellavel Ahh sorry!

[Santhosh-Sellavel]

@mollfpr So we have to wait right?

[mollfpr]

@Santhosh-Sellavel Let’s ask @neil-marcellini and @luacmartins

[neil-marcellini]

My understanding is that to test a solution to this issue #12400, we need a form that uses IdentityForm. Is that indeed the problem in question? Could we use Storybook to test IdentityForm in isolation? If not then it sounds like we will have to hold #12400 on #9581.

[mollfpr]

I could create a storybook component for IdentityForm that use the Form.js but holding this until #9581 passed is a good idea.

[JmillsExpensify]

@Santhosh-Sellavel @neil-marcellini The regression period has passed. At this point, this PR is merged and IndentityForm is "done." What's our plan for testing this in conjunction with related issues? Should that happen in a separate issue when #12400 and #9581 are ready?

[Santhosh-Sellavel]

@JmillsExpensify When #9581 is ready, this should work!

[JmillsExpensify]

Ok, I'm going to go ahead and issue payment. Per the process, the PR is merged and has been for 7 days. I don't feel great about waiting on a related issue that is not yet merged. That part is on Expensify.

[JmillsExpensify]

Alright, all contributors paid out. I removed the Awaiting Payment label and I'm also updating the hold as well. We can come back to this and test when #9581 is ready.

[JmillsExpensify]

Review of the linked/holding PR is in progress! I'll have to check back in the coming days.

[JmillsExpensify]

#9581 is merged and on staging at this point, so I think we should be able to remove the hold and move forward with next steps. Does that sound right @Santhosh-Sellavel @neil-marcellini?

[Santhosh-Sellavel]

We can close this now or along with #9581.This is already merged, we were waiting for test a particular test case which is passing now. @JmillsExpensify

[JmillsExpensify]

Awesome, sounds like a plan. As for regression tests, I think we are already covered via this test. Accordingly, I'm going to close. cc @luacmartins please re-open if you disagree and I'll make sure any additional cases are added to that existing test.