[$1,000] [Form Refactor] RequestorStep

[luacmartins]

Coming from the New Expensify Forms design doc, we should refactor RequestorStep to use the new form component, follow the guidelines below:

Here's an example of a Form refactor: https://github.com/Expensify/App/pull/9056

Guidelines

1. Replace the form component with Form.js.
2. Create a unique Onyx key in ONYXKEYS.FORM and pass it as the formID prop to Form.
3. Pass a validate callback prop.
4. Pass an onSubmit callback prop that calls the API via an action.
5. Update all inputs wrapped by Form, following the guidelines in Refactor inputs.
6. 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[bot]]

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

[NicMendonca]

Job post -- https://www.upwork.com/jobs/~012c4ed44ee1b967a1

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[Murodjon000]

Hi 👋 @neil-marcellini, may i update RequsterStep, i checked the details, the steps that i will do using Form.js component inside RequestorStep component properly refactoring the code. In old one clicking the fix the errors is not working as expected, While refactoring i will stick to the instructions above.

[luacmartins]

This issue is being put on hold due to push to page discussions, as per this comment.

[luacmartins]

@Murodjon000 thanks for the interest. However, this issue is now on hold and we won't assign anyone to it until the hold is removed.

[neil-marcellini]

On HOLD, but we said we would discuss push to page this week, have we?

[neil-marcellini]

^ Still on hold unless the push to page discussion is complete.

[neil-marcellini]

Holding

[neil-marcellini]

We're discussing removing the HOLD on this in the BankAccountStep refactor issue. Push to page discussions are still ongoing here.

[luacmartins]

Hold removed! looking for contributors!

[neil-marcellini]

@Murodjon000 would you like to submit a proposal now that the issue is off of HOLD?

[neil-marcellini]

I'm going to ping in #expensify-open-source for more visibility.

[mateusbra]

I think I could take this one, if possible

[neil-marcellini]

Please submit a proposal following the guidelines here and if it looks good we'll hire you for the job! Thanks.

[mateusbra]

Nvm, I think I won't have time for this one right now, I'll let it for another contributor.

[neil-marcellini]

Still waiting for proposals...

[NicMendonca]

Doubled price: https://www.upwork.com/jobs/~012c4ed44ee1b967a1

[Santhosh-Sellavel]

@NicMendonca lets update title also.

[NicMendonca]

doubled: https://www.upwork.com/jobs/~012c4ed44ee1b967a1

[mollfpr]

Proposal

Here are the changes that I made so far for refactoring the RequestorStep. It might be not finalized yet since I'm unable to access the form guidelines.

diff --git a/src/ONYXKEYS.js b/src/ONYXKEYS.js
index 4885b4995..f597017e7 100755
--- a/src/ONYXKEYS.js
+++ b/src/ONYXKEYS.js
@@ -178,5 +178,8 @@ export default {
     FORMS: {
         ADD_DEBIT_CARD_FORM: 'addDebitCardForm',
         REQUEST_CALL_FORM: 'requestCallForm',
+        REQUESTOR_STEP_FORM: 'requestorStepForm',
     },

ePicker/datepickerPropTypes.js b/src/components/DatePicker/datepickerPropTypes.js
index 4a681ec90..d8668de5c 100644
--- a/src/components/DatePicker/datepickerPropTypes.js
+++ b/src/components/DatePicker/datepickerPropTypes.js
@@ -21,12 +21,15 @@ const propTypes = {

     /* Restricts for selectable max date range for the picker */
     maximumDate: PropTypes.instanceOf(Date),
+
+    inputID: PropTypes.string,
 };

 const defaultProps = {
     ...defaultFieldPropTypes,
     value: undefined,
     maximumDate: undefined,
+    inputID: '',
 };

 export {propTypes, defaultProps};
diff --git a/src/components/DatePicker/index.js b/src/components/DatePicker/index.js
index 0077e4cf9..9a3cb5e42 100644
--- a/src/components/DatePicker/index.js
+++ b/src/components/DatePicker/index.js
@@ -86,6 +86,7 @@ class DatePicker extends React.Component {
                 containerStyles={this.props.containerStyles}
                 disabled={this.props.disabled}
                 onBlur={this.props.onBlur}
+                inputID={this.props.inputID}
             />
         );
     }

uestorStep.js b/src/pages/ReimbursementAccount/RequestorStep.js
index 0f947beef..8ce4f6199 100644
--- a/src/pages/ReimbursementAccount/RequestorStep.js
+++ b/src/pages/ReimbursementAccount/RequestorStep.js
@@ -3,7 +3,7 @@ import lodashGet from 'lodash/get';
 import {ScrollView, View} from 'react-native';
 import {withOnyx} from 'react-native-onyx';
 import _ from 'underscore';
-import moment from 'moment';
+import PropTypes from 'prop-types';
 import styles from '../../styles/styles';
 import withLocalize, {withLocalizePropTypes} from '../../components/withLocalize';
 import HeaderWithCloseButton from '../../components/HeaderWithCloseButton';
@@ -13,30 +13,46 @@ import Navigation from '../../libs/Navigation/Navigation';
 import CheckboxWithLabel from '../../components/CheckboxWithLabel';
 import Text from '../../components/Text';
 import * as BankAccounts from '../../libs/actions/BankAccounts';
-import IdentityForm from './IdentityForm';
 import * as ValidationUtils from '../../libs/ValidationUtils';
+import * as ReimbursementAccountUtils from '../../libs/ReimbursementAccountUtils';
 import Onfido from '../../components/Onfido';
-import compose from '../../libs/compose';
 import ONYXKEYS from '../../ONYXKEYS';
-import * as ReimbursementAccountUtils from '../../libs/ReimbursementAccountUtils';
 import Growl from '../../libs/Growl';
-import reimbursementAccountPropTypes from './reimbursementAccountPropTypes';
-import ReimbursementAccountForm from './ReimbursementAccountForm';
 import * as Link from '../../libs/actions/Link';
+import Form from '../../components/Form';
+import TextInput from '../../components/TextInput';
+import DatePicker from '../../components/DatePicker';
+import AddressSearch from '../../components/AddressSearch';
+import StatePicker from '../../components/StatePicker';
+import compose from '../../libs/compose';

 const propTypes = {
-    /** Bank account currently in setup */
-    reimbursementAccount: reimbursementAccountPropTypes.isRequired,
+    achData: PropTypes.shape({
+
+        /** Step of the setup flow that we are on. Determines which view is presented. */
+        currentStep: PropTypes.string,
+
+        /** Bank account state */
+        state: PropTypes.string,
+
+        /** Bank account ID of the VBA that we are validating is required */
+        bankAccountID: PropTypes.number,
+
+    }),

     ...withLocalizePropTypes,
 };

+const defaultProps = {
+    achData: {},
+};
+
 class RequestorStep extends React.Component {
     constructor(props) {
         super(props);

+        this.validate = this.validate.bind(this);
         this.submit = this.submit.bind(this);
-        this.clearErrorsAndSetValues = this.clearErrorsAndSetValues.bind(this);

         this.state = {
             firstName: ReimbursementAccountUtils.getDefaultStateForField(props, 'firstName'),
@@ -51,95 +67,55 @@ class RequestorStep extends React.Component {
             onfidoData: lodashGet(props, ['achData', 'onfidoData'], ''),
             isOnfidoSetupComplete: lodashGet(props, ['achData', 'isOnfidoSetupComplete'], false),
         };
+    }

-        // Required fields not validated by `validateIdentity`
-        this.requiredFields = [
-            'isControllingOfficer',
-        ];
+    validate(values) {
+        const errors = {};

-        // Map a field to the key of the error's translation
-        this.errorTranslationKeys = {
-            firstName: 'bankAccount.error.firstName',
-            lastName: 'bankAccount.error.lastName',
-            isControllingOfficer: 'requestorStep.isControllingOfficerError',
-        };
+        if (!values.firstName) {
+            errors.firstName = this.props.translate('bankAccount.error.firstName');
+        }

-        this.clearError = inputKey => ReimbursementAccountUtils.clearError(this.props, inputKey);
-        this.clearErrors = inputKeys => ReimbursementAccountUtils.clearErrors(this.props, inputKeys);
-        this.getErrors = () => ReimbursementAccountUtils.getErrors(this.props);
-    }
+        if (!values.lastName) {
+            errors.lastName = this.props.translate('bankAccount.error.lastName');
+        }

-    /**
-     * Clear the errors associated to keys in values if found and store the new values in the state.
-     *
-     * @param {Object} values
-     */
-    clearErrorsAndSetValues(values) {
-        const renamedFields = {
-            street: 'requestorAddressStreet',
-            city: 'requestorAddressCity',
-            state: 'requestorAddressState',
-            zipCode: 'requestorAddressZipCode',
-        };
-        const newState = {};
-        _.each(values, (value, inputKey) => {
-            const renamedInputKey = lodashGet(renamedFields, inputKey, inputKey);
-            newState[renamedInputKey] = value;
-        });
-        this.setState(newState);
-        BankAccounts.updateReimbursementAccountDraft(newState);
+        if (!values.requestorAddressStreet || !ValidationUtils.isValidAddress(values.requestorAddressStreet)) {
+            errors.requestorAddressStreet = this.props.translate('bankAccount.error.addressStreet');
+        }

-        // Prepare inputKeys for clearing errors
-        const inputKeys = _.keys(values);
+        if (!values.requestorAddressCity) {
+            errors.requestorAddressCity = this.props.translate('bankAccount.error.addressState');
+        }

-        // dob field has multiple validations/errors, we are handling it temporarily like this.
-        if (_.contains(inputKeys, 'dob')) {
-            inputKeys.push('dobAge');
+        if (!values.requestorAddressState) {
+            errors.requestorAddressState = this.props.translate('bankAccount.error.addressCity');
         }
-        this.clearErrors(inputKeys);
-    }

-    /**
-     * @returns {Boolean}
-     */
-    validate() {
-        const errors = ValidationUtils.validateIdentity({
-            firstName: this.state.firstName,
-            lastName: this.state.lastName,
-            street: this.state.requestorAddressStreet,
-            state: this.state.requestorAddressState,
-            city: this.state.requestorAddressCity,
-            zipCode: this.state.requestorAddressZipCode,
-            dob: this.state.dob,
-            ssnLast4: this.state.ssnLast4,
-        });
+        if (!values.requestorAddressZipCode || !ValidationUtils.isValidZipCode(values.requestorAddressZipCode)) {
+            errors.requestorAddressZipCode = this.props.translate('bankAccount.error.zipCode');
+        }

-        _.each(this.requiredFields, (inputKey) => {
-            if (ValidationUtils.isRequiredFulfilled(this.state[inputKey])) {
-                return;
-            }
+        if (!values.dob || !ValidationUtils.isValidDate(values.dob)) {
+            errors.dob = this.props.translate('bankAccount.error.dob');
+        }

-            errors[inputKey] = true;
-        });
-        if (_.size(errors)) {
-            BankAccounts.setBankAccountFormValidationErrors(errors);
-            BankAccounts.showBankAccountErrorModal();
-            return false;
+        if (values.dob && !ValidationUtils.meetsAgeRequirements(values.dob)) {
+            errors.dob = this.props.translate('bankAccount.error.age');
         }
-        return true;
-    }

-    submit() {
-        if (!this.validate()) {
-            return;
+        if (!values.ssnLast4 || !ValidationUtils.isValidSSNLastFour(values.ssnLast4)) {
+            errors.ssnLast4 = this.props.translate('bankAccount.error.ssnLast4');
         }

-        const payload = {
-            ...this.state,
-            dob: moment(this.state.dob).format(CONST.DATE.MOMENT_FORMAT_STRING),
-        };
+        return errors;
+    }

-        BankAccounts.setupWithdrawalAccount(payload);
+    submit(values) {
+        BankAccounts.setupWithdrawalAccount({
+            ...this.state,
+            ...values,
+        });
     }

     render() {
@@ -176,9 +152,13 @@ class RequestorStep extends React.Component {
                         />
                     </ScrollView>
                 ) : (
-                    <ReimbursementAccountForm
-                        reimbursementAccount={this.props.reimbursementAccount}
+                    <Form
+        });
     }

     render() {
@@ -176,9 +152,13 @@ class RequestorStep extends React.Component {
                         />
                     </ScrollView>
                 ) : (
-                    <ReimbursementAccountForm
-                        reimbursementAccount={this.props.reimbursementAccount}
+                    <Form
+                        formID={ONYXKEYS.FORMS.REQUESTOR_STEP_FORM}
+                        validate={this.validate}
                         onSubmit={this.submit}
+                        submitButtonText={this.props.translate('common.saveAndContinue')}
+                        style={[styles.mh5, styles.flexGrow1]}
+                        draftValues={_.omit(this.state, ['onfidoData', 'isOnfidoSetupComplete'])}
                     >
                         <Text>{this.props.translate('requestorStep.subtitle')}</Text>
                         <View style={[styles.mb5, styles.mt1, styles.dFlex, styles.flexRow]}>
@@ -198,30 +178,75 @@ class RequestorStep extends React.Component {
                                 {`${this.props.translate('requestorStep.isMyDataSafe')}`}
                             </TextLink>
                         </View>
-                        <IdentityForm
-                            onFieldChange={this.clearErrorsAndSetValues}
-                            values={{
-                                firstName: this.state.firstName,
-                                lastName: this.state.lastName,
-                                street: this.state.requestorAddressStreet,
-                                state: this.state.requestorAddressState,
-                                city: this.state.requestorAddressCity,
-                                zipCode: this.state.requestorAddressZipCode,
-                                dob: this.state.dob,
-                                ssnLast4: this.state.ssnLast4,
-                            }}
-                            errors={this.props.reimbursementAccount.errors}
+
+                        <View style={[styles.flexRow]}>
+                            <View style={[styles.flex2, styles.mr2]}>
+                                <TextInput
+                                    inputID="firstName"
+                                    label={`${this.props.translate('common.firstName')}`}
+                                />
+                            </View>
+                            <View style={[styles.flex2]}>
+                                <TextInput
+                                    inputID="lastName"
+                                    label={`${this.props.translate('common.lastName')}`}
+                                />
+                            </View>
+                        </View>
+
+                        <DatePicker
+                            inputID="dob"
+                            containerStyles={[styles.mt4]}
+                            label={`${this.props.translate('common.dob')}`}
+                            placeholder={this.props.translate('common.dateFormat')}
+                            maximumDate={new Date()}
                         />
+
+                        <TextInput
+                            inputID="ssnLast4"
+                            containerStyles={[styles.mt4]}
+                            label={`${this.props.translate('common.ssnLast4')}`}
+                            keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
+                            maxLength={CONST.BANK_ACCOUNT.MAX_LENGTH.SSN}
+                        />
+
+                        <View>
+                            <AddressSearch
+                                inputID="requestorAddressStreet"
+                                label={this.props.translate('common.personalAddress')}
+                                containerStyles={[styles.mt4]}
+                                hint={this.props.translate('common.noPO')}
+                                renamedInputKeys={{
+                                    street: 'requestorAddressStreet',
+                                    city: 'requestorAddressCity',
+                                    state: 'requestorAddressState',
+                                    zipCode: 'requestorAddressZipCode',
+                                }}
+                            />
+                        </View>
+
+                        <View style={[styles.flexRow, styles.mt4]}>
+                            <View style={[styles.flex2, styles.mr2]}>
+                                <TextInput
+                                    inputID="requestorAddressCity"
+                                    label={this.props.translate('common.city')}
+                                />
+                            </View>
+                            <View style={[styles.flex1]}>
+                                <StatePicker inputID="requestorAddressState" />
+                            </View>
+                        </View>
+
+                        <TextInput
+                            inputID="requestorAddressZipCode"
+                            label={this.props.translate('common.zip')}
+                            containerStyles={[styles.mt4]}
+                            keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
+                            maxLength={CONST.BANK_ACCOUNT.MAX_LENGTH.ZIP_CODE}
+                        />
+
                         <CheckboxWithLabel
-                            isChecked={this.state.isControllingOfficer}
-                            onInputChange={() => {
-                                this.setState((prevState) => {
-                                    const newState = {isControllingOfficer: !prevState.isControllingOfficer};
-                                    BankAccounts.updateReimbursementAccountDraft(newState);
-                                    return newState;
-                                });
-                                this.clearError('isControllingOfficer');
-                            }}
+                            inputID="isControllingOfficer"
                             LabelComponent={() => (
                                 <View style={[styles.flex1, styles.pr1]}>
                                     <Text>
@@ -230,8 +255,8 @@ class RequestorStep extends React.Component {
                                 </View>
                             )}
                             style={[styles.mt4]}
-                            errorText={this.getErrors().isControllingOfficer ? this.props.translate('requestorStep.isControllingOfficerError') : ''}
                         />
+
                         <Text style={[styles.mt3, styles.textMicroSupporting]}>
                             {this.props.translate('requestorStep.onFidoConditions')}
                             <Text
@@ -258,7 +283,7 @@ class RequestorStep extends React.Component {
                                 {`${this.props.translate('common.termsOfService')}`}
                             </Text>
                         </Text>
-                    </ReimbursementAccountForm>
+                    </Form>
-                    </ReimbursementAccountForm>
+                    </Form>
                 )}
             </>
         );
@@ -266,6 +291,7 @@ class RequestorStep extends React.Component {
 }

 RequestorStep.propTypes = propTypes;
+RequestorStep.defaultProps = defaultProps;

 export default compose(
     withLocalize,

Result

https://user-images.githubusercontent.com/25520267/187083051-f0277035-5df7-4aaf-b189-2d0d79c8467c.mp4

[melvin-bot[bot]]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[smrutiparida]

Proposal

Here is a proposal for the Form changes. This a quick prototype and form styles can only be refined after going through the new design document. Please review.

https://user-images.githubusercontent.com/5945400/187087227-f61e2afa-7795-4be4-a3f9-88b4d5d6d8f8.mov

Test Result without style changes looks like this

[Santhosh-Sellavel]

@neil-marcellini @mollfpr's proposal looks good to me.

🎀 👀 🎀 C+ Reviewed

[neil-marcellini]

Overall @mollfpr's proposal is looking very good. Here are some of my initial thoughts. I will ask some questions to the team and based on their response decide how to move forward. I will let you know once we are ready for you to create a PR.

Since you can't access the linked doc, check out FORMS.md if you haven't already.

• With the validation for the requestor street address the same error message is shown if it's empty or invalid. That's not quite right, but I see it done in the example so I'll ask about it.
• I don't think it's a good idea to pass all of the state into the function called by onSubmit.
• I don't think we want to keep all of those state values since we will now be using uncontrolled inputs which will store their own values.
• Refactor the inputs to take a defaultValue and let the form handle the draft values. Remove all uses of getDefaultStateForField. https://github.com/Expensify/App/blob/b81ff69197e091727b33f8dd53fbc429fdd616b9/src/pages/ReimbursementAccount/RequestorStep.js#L42
• The draft values should be "An object containing saved draft values for each form input and passed by withOnyx() subscribing to the ${props.formID}DraftValues key."
• Hmm, do we have to refactor IdentityForm now? Should that be done first? I will ask about that.

[mollfpr]

Thanks @Santhosh-Sellavel @neil-marcellini I’ll read the FORMS.md carefully.

[neil-marcellini]

We decided that it would be a good idea to refactor the IdentityForm first, since this form uses it. I created https://github.com/Expensify/App/issues/10729. @mollfpr please put up a proposal for that issue if you would like. I will give you priority since it's related to this one. I'm going to put this issue on HOLD until that one is complete.

[mollfpr]

Okay, thanks. I’ll write the proposal for that issue ASAP.

[NicMendonca]

^ still holding?