Refactor Form: Support `errors` and `errorFields` returned from the backend, and add possibility to hide the submit the button

[youssef-lr]

Problem

We recently started returning an errors object in onyxData from the API that contain the latest error related to a form submission. The current Form component does not take into account the new key and no server error message gets displayed when we submit incorrect data.

There are also some cases where we need to hide the submit button but this is currently not possible as it is abstracted away from outside components. Example: when adding a bank account from Plaid, we need to hide the button while Plaid is loading and when the user hasn't selected a Plaid account yet from the picker.

Since there are a few ongoing PRs that depend on the Form component, I figured it might be a good idea to create a separate PR for this and have the PRs merge with main.

cc @luacmartins @marcaaron

Solution

• Add a getErrorMessage function to Form that extracts the latest error message from the errors object. For API calls that still return an error key, this should still work as the function below checks for both keys.

getErrorMessage() {
    const latestErrorMessage = ErrorUtils.getLatestErrorMessage(this.props.formState);
    return this.props.formState.error || (typeof latestErrorMessage === 'string' ? latestErrorMessage : '');
}

• Rename setErrorMessage in actions/FormActions.js to setErrors.

• Add an optional prop isSubmitButtonVisible to Form that controls the visibility of the submit button. This prop should have a default value of true to not break any existing components that use Form.

• Handle field-specific errors coming from the backend and display them on the inputs that need attention.

• If there are any errorFields, display the errors on the input and display the fixTheErrors.

childrenWrapperWithProps(children) {
    return React.Children.map(children, (child) => {
        ...
        const errorFields = lodashGet(this.props.formState, 'errorFields', {});
        const fieldErrorMessage = _.chain(errorFields[inputID])
            .keys()
            .sortBy()
            .reverse()
            .map(key => errorFields[inputID][key])
            .first()
            .value();

         return React.cloneElement(child, {
             ref: node => this.inputRefs[inputID] = node,
             defaultValue,
             errorText: this.state.errors[inputID] || fieldErrorMessage,
             ...
        }
    }
}

/** Controls the submit button's visibility */
isSubmitButtonVisible: PropTypes.bool,
...

const defaultProps = {
    isSubmitButtonVisible: true,
    ...
};
...

{this.props.isSubmitButtonVisible && (
<FormAlertWithSubmitButton
    buttonText={this.props.submitButtonText}
    isAlertVisible={_.size(this.state.errors) > 0 || Boolean(this.getErrorMessage())}
    isLoading={this.props.formState.isLoading}
    message={_.isEmpty(this.props.formState.errorFields) ? this.getErrorMessage() : null}

    onSubmit={this.submit}
    onFixTheErrorsLinkPressed={() => {
        this.inputRefs[_.first(_.keys(this.state.errors))].focus();
    }}
    containerStyles={[styles.mh0, styles.mt5]}
/>
)}

[melvin-bot[bot]]

Triggered auto assignment to @sophiepintoraetz (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[youssef-lr]

Not overdue. Working on making this work properly with some new changes in the API, then I'll update the issue with the final changes that must be added to Form.

[marcaaron]

Add a getErrorMessage function to Form that extracts the latest error message from the errors object.

Should we be showing all possible errors messages from the errors object?

Rename setErrorMessage in actions/FormActions.js to setErrors.

👍

Add an optional prop isSubmitButtonVisible to Form that controls the visibility of the submit button. This prop should have a default value of true to not break any existing components that use Form.

This is probably good for the Form case. I think sometimes an alternative can be to figure out how to pass an element you would otherwise toggle with a boolean prop in as a child or prop and then have the parent control it's visibility by either passing in the button or not. That is sometimes more flexible than a boolean prop - but probably not be worth worrying about in this case (and would be a lot more changes).

[marcaaron]

@youssef-lr I'm assuming you are also actively working on this - if so, we can add the Internal label

[youssef-lr]

Should we be showing all possible errors messages from the errors object?

Is there is any discussion I can look at about why we made the switch from an error key to the errors object key? As right now, I checked the code for occurrences of the errors Onyx key, and could not find where we include more than one item in the array (please correct me if I missed something). So I'm not sure yet how we would handle displaying all of the errors because the current format right now only sends out a single error.

There's also a new addition to passing errors from the backend which is errorFields which should contain any server side errors related to specific inputs. I'm working on including it as well in Form.

[luacmartins]

Is there is any discussion I can look at about why we made the switch from an error key to the errors object key?

IIRC this is where we discussed it, not sure if there were other discussions around it though.

As right now, I checked the code for occurrences of the errors Onyx key, and could not find where we include more than one item in the array (please correct me if I missed something). So I'm not sure yet how we would handle displaying all of the errors because the current format right now only sends out a single error.

That may be the case now, but our OfflineWithFeedback component does allow multiple errors to be passed and displayed here. So we should have the same behavior for Forms.

There's also a new addition to passing errors from the backend which is errorFields which should contain any server side errors related to specific inputs. I'm working on including it as well in Form.

👍 IIRC errorFields was explicitly added for Form errors, so we should support that in Form.

[youssef-lr]

Update: I have a draft PR, should be ready for review today.

[youssef-lr]

Issue updated and PR ready to review! There's one thing I haven't implemented yet which is displaying all of the errors, I think we'll need to check with the design team about how they should look on top of the submit button?

[melvin-bot[bot]]

@youssef-lr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[youssef-lr]

Not overdue.

[youssef-lr]

bump! @marcaaron and @luacmartins! Would love your input on the use of errorFields in Form in the PR linked here.

[luacmartins]

@youssef-lr what exactly did you want input on? Is it whether or not we should use errorFields in Form? Or how to display those errors, or something else?

[youssef-lr]

@luacmartins sorry I completely missed your comment. The PR basically handles displaying errorFields that are coming from the backend, which isn't currently supported, but isn't widely used in the backend either.

[luacmartins]

@youssef-lr I think it would be a good idea to support errorFields. We always want to show the error in the field that caused it, but in the past the API didn't return the error field, but I think that we should incorporate that going forward.

[youssef-lr]

I'll be updating the PR today and removing WIP, it looks like we have some more interest in having this implemented.

[trjExpensify]

Totally agree with doing this. A great example of us running into this is here.

[JmillsExpensify]

Agreed, this is a great one to make sure we get right. Putting this one in WAQ.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@youssef-lr Huh... This is 4 days overdue. Who can take care of this?

[youssef-lr]

This has been deployed to production.