SPIKE: Validation not compatible with adding blocks inline in 4.0

[brynwhyman]

Overview

Given the investigation required, we'll be spiking the effort and implementation detail required in introducing standard SilverStripe validation with inline-editable blocks.

The following issue gives the best detail: #584

Notes

• Given validation isn't possible at the moment, on introducing it, we should consider a release note or documentation update to make it clear to developers that this is something that can be implemented.

[robbieaverill]

I've fixed the original bug in #584, and a JavaScript error that happens when you haven't loaded any inline editable element forms yet and try to save the page - cannot convert undefined or null to object.

The state of play now (using the example code from #584) is:

• If you open an element edit form and save the page, you get an error at the top of the page. It should really be targeting the elements that have the error instead:
  
• If you haven't opened any element edit forms yet, and you save the page, it will save successfully. This is because it doesn't write any element information so it doesn't call its validator. I'm in two minds about this but I think it's correct behaviour, particularly for performance reasons.
• If you open an element edit form and save the element itself, you get a toast message saying there was an error. Ideally this would tell you the validation error if there was one.
  

[robbieaverill]

I guess the ideal situation is that the field itself shows the error rather than it being a toast message, e.g. something like this:

This would require some adjustment to the ElementalAreaController to return the errors and identify which fields they apply to, and to the SaveAction to propagate those errors back into the Redux state in a way that the element editor form can detect and update to show them. We could then do away with the toast message when there's an error, leaving the success toast message when it saves successfully.

Thoughts @silverstripe/creative-commoners?

[robbieaverill]

I've tested the same patterns in asset admin:

• addError($message) doesn't work - issue raised at ValidationResult::addError() doesn't show up anywhere in the UI silverstripe-asset-admin#967
• addFieldError($fieldName, $message) works - the field identifies its error correctly

In Elemental we "namespace" the element field names, so it might be a little tricky to support addFieldError() but I'll have a snoop and see what happens.

[robbieaverill]

What I've done

• Refactor the controller to use an MFA style jsonResponse() method, and provide validation error messages in responses
• Refactor the SaveAction to use fetch() rather than the backend fetcher from admin

What I get

• Inline editing validation errors now show in a toast message
  
• Saving the page with an inline edit form open targets the validation error at the page's fields rather than the element (because of the form namespacing we add)
  
• Saving the page without an inline edit form open does not show any element related validation errors, because their forms were never loaded. The save request succeeds (see below).

Ways to save content

• Via the page, without any edit forms open
• Via the page, with at least one edit form open
• Via the element's edit form

Things to consider

• If you save the page, the inline edit form is going to be closed again. If there's a validation error inside it then how do we handle that? Do we show a validation error indicator on the element title? Do we automatically expand the element forms that have errors so that they can have inline validation errors shown? When you save at the page level it doesn't have a relevant form to target the errors at yet so the information won't be immediately surfaced. This might need PUX/designs.
• We want the API developers use for this to be straight forward, as in the example issue above. This would probably mean some logic in BaseElement to add namespacing to validation field errors so they don't need to concern themselves with the namespace for each form.
• When saving an element inline, the error should be pushed into redux so it can show as an inline form validation error - I haven't investigated doing this yet, but it shouldn't be too much work.
• When saving a page without having opened any element edit forms yet, there are no validation errors, because none of the forms with validators attached have been loaded yet. This means that the page can be saved successfully, which might be a little misleading for content authors given that the content in their page hasn't been validated at all yet. I can think of two options to work around this:
  • We load all element forms when you load the page (performance hit)
  • We manually call validate() on every element on the page when a page is being saved to ensure they're valid, and if not we attach the appropriate validation error to each form and auto-load them when the page loads (as mentioned above)
• When using $validator->addFieldError('Title', 'Title is required') I'd expect it to be shown as an inline form field error state on the relevant elements (there could be multiple).

    public function validate()
    {
        $validator = parent::validate();
        if ($this->isInDB() && empty($this->Title)) {
            $validator->addFieldError('Title', 'Title is required');
        }
        return $validator;
    }

• When using $validator->addError('You missed something in the element, but I don\'t have a specific field to point you at') then I'd expect the error to show on the relevant element(s) edit form. As above, this won't be expanded by default if you save at the page level, so we may need to auto-expand them on page save when errors occur.

    public function validate()
    {
        $validator = parent::validate();
        if ($this->isInDB() && empty($this->Title)) {
            $validator->addError('A non-field specific error occurred during validation');
        }
        return $validator;
    }

Is this enough to go on for the spike @brynwhyman?

I've pushed my WIP to pulls/4.1/validation-refactor for reference.

[ScopeyNZ]

• If you save the page, the inline edit form is going to be closed again. If there's a validation error inside it then how do we handle that? Do we show a validation error indicator on the element title? Do we automatically expand the element forms that have errors so that they can have inline validation errors shown? When you save at the page level it doesn't have a relevant form to target the errors at yet so the information won't be immediately surfaced. This might need PUX/designs.

Yeah I can imagine we'll need to have some validation payload to put into redux so the form can know about it when it's mounted. It looks like components like fieldHolder have some validation state logic in them. Marking a block as unsaved is already designed for but it definitely would need something to indicate it's unsaved due to validation.

• When saving an element inline, the error should be pushed into redux so it can show as an inline form validation error - I haven't investigated doing this yet, but it shouldn't be too much work.

Yeah hopefully if we write something server-side to generate something to put into redux then we can share this with the page-level saving.

• When saving a page without having opened any element edit forms yet, there are no validation errors, because none of the forms with validators attached have been loaded yet. This means that the page can be saved successfully, which might be a little misleading for content authors given that the content in their page hasn't been validated at all yet. I can think of two options to work around this:

  • We load all element forms when you load the page (performance hit)
  • We manually call validate() on every element on the page when a page is being saved to ensure they're valid, and if not we attach the appropriate validation error to each form and auto-load them when the page loads (as mentioned above)

Option 3; we don't bother. We don't attempt to save any blocks that haven't been opened when you do a page level save so the validation shouldn't trigger? If a block has become invalid without a content author editing it then that's just general $model->validate() weirdness that we don't have to support?

• When using $validator->addFieldError('Title', 'Title is required') I'd expect it to be shown as an inline form field error state on the relevant elements (there could be multiple).

    public function validate()
    {
        $validator = parent::validate();
        if ($this->isInDB() && empty($this->Title)) {
            $validator->addFieldError('Title', 'Title is required');
        }
        return $validator;
    }

Agreed. I guess that there's some way this should work with FormBuilder but doesn't yet?

• When using $validator->addError('You missed something in the element, but I don\'t have a specific field to point you at') then I'd expect the error to show on the relevant element(s) edit form. As above, this won't be expanded by default if you save at the page level, so we may need to auto-expand them on page save when errors occur.

    public function validate()
    {
        $validator = parent::validate();
        if ($this->isInDB() && empty($this->Title)) {
            $validator->addError('A non-field specific error occurred during validation');
        }
        return $validator;
    }

Perhaps this is also something that PUX can weigh in on

I'm a little worried about validation needing to show on blocks that aren't expanded. I think that means we'll have to loop the blocks and run ->validate() on all of them to gather some detail about errors they may have? Preferably we'd only do this when generating a response for a page save.

Great investigation though, this is probably enough to go on. I see that we'll probably need to:

• Figure out some way to map that state to FormBuilder so that we can get field level errors showing where relevant
• Include some component to show element level errors on the in-line forms
• Get some designs from pux for invalid blocks in their collapsed state

Then we just need to figure out how to get validation responses to the client-side. Here is a crazy idea that I've just hatched after chatting with @unclecheese: Toss validation errors from a page level save into data-schema and use it when rendering the list of blocks again.

Currently we're stuck in a weird half-way case (again) where we're partially using REST/pjax and GraphQL. It makes sense to return validation responses with mutations, but not so much with queries. We can do some magic though where we can add validation errors to "schema data" on the ElementalAreaField when saving, and after passing that detail through to the props of the ElementalEditor component we can tag blocks as invalid.

Hopefully individual block saves aren't so painful here.

[clarkepaul]

One approach to fixing hidden validation errors is to open any blocks with errors so they can be seen. We could apply an additional style to highlight those blocks (e.g. red border on left) if we want to keep them closed—design would be required) for this.

[ScopeyNZ]

I've raised an issue to do the work outlined in this issue:

#699

I'll close this now (as done) but we can continue discussion.

One approach to fixing hidden validation errors is to open any blocks with errors so they can be seen. We could apply an additional style to highlight those blocks (e.g. red border on left) if we want to keep them closed—design would be required) for this.

Yeah - the only concern about that is performance - when you open a block you have to request the form from the server. Doing that for ~5 blocks would be slow and annoying.