react-redux-form icon indicating copy to clipboard operation
react-redux-form copied to clipboard

Handling on-submit validation correctly

Open thomasboyt opened this issue 7 years ago • 5 comments

I've got a fairly simple payment form in my app, with the usual fields (card number, CVV, expiration date, and zip code). The form has frontend validation for the simple things (Luhn10, ensuring MM/YY expiration is valid, ensuring zip and CVV are numbers), but, of course, most of the errors encountered in payment are things that can only be checked on submission (e.g. incorrect CVV, declined card).

I was hoping to use React-Redux-Form's built-in errors and validity to handle these on-submit errors, but had a lot of trouble doing so.

Async Field Errors on Submit

First, I was hoping that, in the case of an error being tied directly to a field (e.g. a failed CVV check is tied to the CVV field), I could highlight the field in question. I figured the easiest way to do this would be to simply use setValidity from the submission error handler:

if (error.type === 'card_error') {
  if (error.param === 'cvv') {
    dispatch(rrfActions.setValidity('forms.addPayment.cvv', {stripe: false}, {async: true});
  }
}

The obvious problem here is that, now, my fields are permanently invalid! To try to solve this, I attempted to have my form reset errors on change:

resetStripeErrors() {
    this.props.dispatch(actions.resetValidity(MODEL_NAME, ['stripe']));
}

render() {
    return (
        <Form
            model={MODEL_NAME}
            className="form"
            onSubmit={(values) => this.handleSubmit(values)}
            onChange={() => this.resetStripeErrors()}>
            {/* ... */}
        </Form>
    )
}

However, when I add this onChange handler to the form, the entire form and all of its fields are made valid on initial render for reasons I do not understand.

#596 appears to sort of track this issue. I'm extremely confused about how "async" validations are supposed to work, but if the answer here is to consider these validations "async" and have a way to clear them specifically, that could help.

"Validity" of a Form With Submission Errors

The other major question I had was around using the errors of a model to track generic errors returned on submission. Not all errors correspond one to one with a field (for example, a login form might have a throttle). To model this use case, a form should be able to have "errors" without being "invalid."

I'm really unclear whether this is actually possible or not. #567 makes me think it should be, but this seems to be specific to the use case of ensuring the onSubmit() handler works. My particular problem is that if I set an error as recommended:

dispatch(rrfActions.setErrors('forms.addBillingProfile', 'Unknown error', {async: true}));

The model is marked as "invalid" until a blur event happens. This is a problem because my submit button is tied to the valid state of the form model:

    renderSubmit() {
        const getClassName = ({fieldValue}) => classNames('form--submit', 'button', '-large', '-primary', {
            '_disabled': !fieldValue.valid,
        });

        return (
            <Control.button model="." type="submit"
                mapProps={{className: getClassName}} disabled={{valid: false}}>
                Save Card
            </Control.button>
        );
    }

I would like for errors from submissions to not cause the form to be marked as "invalid" (unless they are tied to a specific field), so that a user can resubmit.

My current workaround for both of these problems has been to store submission errors in another reducer, ensuring that the validity of the form is never changed by submission. This, unfortunately, means:

  • I'm not able to highlight specific fields when a submission error is tied to a specific field
  • I have to duplicate my error state in another reducer, and carefully manage ensuring that it is properly reset on form submission and when the form is mounted/unmounted

I'm not actually sure whether the problems I've described are bugs, missing features, or if there's better way to handle my use cases. If either of the problems I've described are not intended behavior, I'd be happy to come up with code samples reproducing them :)

thomasboyt avatar Mar 02 '17 16:03 thomasboyt

I think both issues can be solved by adding a new form property: .syncValid (and maybe .asyncValid). The current behavior is that you should be able to submit a form if only the sync validation is valid - that is, RRF ignores async validation when determining if a form could be submitted. The "is sync valid" state is just not exposed (yet). What do you think?

davidkpiano avatar Mar 07 '17 14:03 davidkpiano

Bump: @thomasboyt do you think your use case will be solved with .syncValid and .asyncValid?

davidkpiano avatar Mar 11 '17 16:03 davidkpiano

@davidkpiano I believe so. I'm not 100% convinced that "sync" and "async" are the right concepts for this - I guess in my head it's more "submission state" rather than "async state," but maybe I'm just overthinking things. Whatever the terminology, exposing "sync validity" seems like the right move.

thomasboyt avatar Mar 13 '17 14:03 thomasboyt

Every time I revisit react-redux-form, I struggle with this very issue of mixing client side and server side validation errors.

I find myself regularly wanting to do something like:

class MyForm extends React.Component {
  constructor(props) {
    super(props)
    this.state = {
      isValid: true,
      error: null
    }
  }

  handleFormUpdate(form) {
    this.setState({ isValid: form.$form.valid })
  }

  handleSubmit(model) {
    this.setState({ error: null })
    const response = saveModelToServer(model)

    /* response shape
    {
      model: { ... },
      errors: [
        { key: 'email', messages: ['has already been take'] },
        ...
      ] 
    }
    */

    if (!response.errors.length) return

    response.errors.map( error => {
      if (error.key) {
        this.formDispatch(actions.setErrors(`local.${error.key}`, { serverError: error.messages.join(', ') }))
      } else {
        this.setState({ error: error.messages.join(', ') })
      }
    })
  }

  attachDispatch(dispatch) {
    this.formDispatch = dispatch
  }

  render() {
    const { isValid, error } = this.state
    const { model, children } = this.props
    let renderedError

    if (error) {
      renderedError = <Alert>{ error }</Alert>
    }

    return (
      <LocalForm
        initialState={ model }
        onUpdate={ this.handleFormUpdate.bind(this) }
        onSubmit={ this.handleSubmit.bind(this) }
        getDispatch={ this.attachDispatch.bind(this) }
      >
        { renderedError }
        { children }
        <button type='submit' disabled={ !isValid }>Save</button>
      </LocalForm>
    )
  }
}

It feels clunky to have to import actions from the library, and attach dispatch to my component via a callback just for the purposes of setting errors. To me it'd feel more intuitive to pass errors to <LocalForm /> via a prop. Where the value passed would conform to same shape as the value passed to initialState, just with the error messages as the values.

Failing something along those lines, then a official example/pattern of how to get something like this working.

As it is, the above doesn't work since isValid stays false after the server responds. The only work around I can think of is:

handleFormChange(model) {
  if (!this.formDispatch) return
  Object.keys(model).map( k => {
     this.formDispatch(actions.setErrors(`local.${k}`, { serverError: false }))
  })
}

...

<LocalForm onChange={ this.handleFormChange.bind(this)  }>
  ...
</LocalForm>

But this is terribly hacky, especially as it loops through the model on every key press :( And it resets all "server errors" when any one field is touched.

If exposing .syncValid, and modifying the above to code to be setError( ... , { async: true }) does the trick, then great. But like @thomasboyt said, "async" doesn't feel quite like the right concept here. And would nice to a have a cleaner more straightforward API, especially when dealing with <LocalForm />.

cipater avatar Apr 24 '17 07:04 cipater

Is there any news with this issue? I'm stacked with handling server error with RRF, as I also need to enable/disable "Next" button according to form state. syncValid & asyncValid does make sense to me

NNSTH avatar May 25 '20 06:05 NNSTH