Implement validation method for inline editable form
Closed this issue · 17 comments
Validation messaging should be handled within the custom form React component as opposed to at a page level.
There are currently two ways to submit content for validation - you can save the page, or you can save an individual block.
Acceptance criteria
- In-line editable content block are validated with form schema as outline in the SPIKE.
Validation occurs both on page save and on individual block save.(new issue created)- API for enforcing validation of content block is inline with the one to validate pages.
Validation works both for inline block and "not inline" block.(new issue created)- Official elemental doc provides clear guidance on how to validate elemental block.
Note
- Need to create a follow up UX card for handling block that are broken and general display of validation errors.
- @GuySartorelli has some good questions about what the UI should do with validation errors.
- Can we constrained this card to validating a block that is currently being edited?
- Related card: #1179
SPIKE
There was a spike to investigate this (which includes a POC PR):
New issues created
PRs
- #1150
- silverstripe/silverstripe-admin#1685
- silverstripe/developer-docs#466
- silverstripe/developer-docs#467
- silverstripe/silverstripe-frameworktest#169
- #1169
Multi-PR CI run
We have some initial designs for the notification placement but we haven't gone through each scenario to figure out what the notifications should say in each case. Please refer to the Style Guide for guidance https://projects.invisionapp.com/dsm/silver-stripe/silver-stripe
Note at this point we'll ideally have completed silverstripe/silverstripe-admin#327 which implements a custom React form component. With this (and the messages that are a part of FormBuilderLoader) we should be able to expose the messages we get back and ship them up to the parent component.
Hi team,
Does this issue also cover adding support for getCMSValidator
or (the new) getCMSCompositeValidator
with inline edited Elements?
At the moment, it seems like the only way to have validation results for Elements (that are inline edited) is to use the validate()
extension point. But, even these are not displayed in the inline editor, and it has a pretty bad side effect, which is already tracked here:
#764
Loosely related:
#840
Sorry this issue is light on detail. It needs a new description to cover how we'd intend on this being implemented, which I'd need some help with.
While looking into silverstripe/silverstripe-admin#840 is was clear that there's a number of loosely related issues, so @Cheddam proposed we step back to look at this holistically: SPIKE Broken form state in EditForms following validation errors
Do we have any sort of time frame that we could expect Validation messages to be support by Inline Editing?
Currently we end up needing to disable this feature quite regularly, as our Blocks have required fields. As such, Inline Editing is a bit of underused feature at the moment (for me, at least).
Hi @chrispenny this would be blocked until we get some time to dig deeper in to the broader issues with inline block state management in silverstripe/silverstripe-admin#1131
At this stage I don't have a time frame for when our team might look into this but I'll push it up our backlog and add an update to this issue in a month or so.
If we add the following to ElementalAreaField
, saving the page will also run validation against form validators in each elemental block (i.e. returned from getCMSCompositeValidator()
or getCMSValidator()
)
public function validate($validator)
{
$valid = parent::validate($validator);
$elementData = $this->Value();
if (!$elementData) {
return $valid;
}
// Ensure CMSCompositeValidator validation is run for each inline-editable element.
$idPrefixLength = strlen(sprintf(ElementalAreaController::FORM_NAME_TEMPLATE ?? '', ''));
foreach ($elementData as $form => $data) {
// Extract the ID
$elementId = (int) substr($form ?? '', $idPrefixLength ?? 0);
/** @var BaseElement $element */
$element = $this->getArea()->Elements()->byID($elementId);
if (!$element) {
// Ignore broken elements
continue;
}
$form = ElementalAreaController::singleton()->getElementFormForValidation($elementId);
$form->loadDataFrom(ElementalAreaController::removeNamespacesFromFields($data, $elementId));
$result = $form->getValidator()->validate();
if (!$result->isValid()) {
$valid = false;
$fields = $this->mapFieldNamesToNamespace($data, $elementId);
foreach ($result->getMessages() as $metadata) {
// TODO: Not all messages are field errors
$validator->validationError($fields[$metadata['fieldName']], $metadata['message']);
}
}
}
return $valid;
}
private function mapFieldNamesToNamespace(array $data, $elementID): array
{
$output = [];
$template = sprintf(EditFormFactory::FIELD_NAMESPACE_TEMPLATE, $elementID, '');
foreach (array_keys($data) as $namespacedField) {
// Only look at fields which belong to this element
if (substr($namespacedField, 0, strlen($template)) !== $template) {
continue;
}
$fieldName = substr($namespacedField, strlen($template));
$output[$fieldName] = $namespacedField;
}
return $output;
}
There are some UX considerations we need to consider before implementing this, such as:
- Do invalid blocks get expanded automatically?
- Do invalid blocks get highlighted e.g. with a red border?
- How do we display validation errors that apply for a block but aren't for a given field?
- How do we deal with validation errors in a separate tab on of the block (e.g. settings tab on the block)?
- What about validation errors for the ElementalArea field itself? Is that in scope?
- Should those be displayed above (non-standard but means those messages are easier to see and relate to the elemental area) or below (more standard but content authors won't notice them until they scroll past the elemental area)?
- Should we display a summary of all validation errors in some central place? IIRC that's what you're meant to do for accessibility - though that should probably be for the form (or at least the current tab of the form) as a whole, not just for the elemental area?)
- Presumably when saving a block from the ... menu on the block itself, its validation error status will change independently from all other blocks (i.e. other blocks which already had visible validation errors will still have those visible validation errors)
Following up from our refinement session yesterday, I wanted to validate how the form for each inline block was being fetch.
When you start editing a block, it fires a request to /admin/elemental-area/schema/elementForm/2
, where 2
is the ID of the block you are editing. This returns a form schema response.
The schema is being generated at
silverstripe-elemental/src/Controllers/ElementalAreaController.php
Lines 84 to 107 in 5dfe7e2
There's a separate endpoint for saving the inline form.
I'm thinking maybe the way to approach this is to come up with a convention for deciding how to validate data entered in a form schema and/or formalise how form schema should be used.
How the form is provided to be rendered is almost irrelevant, unless that's being provided completely fresh after a form submission and after saving a block, and there's a clean way to correctly pass validation messages through that pipe.
What matters more IMO is what happens at submission time, i.e.
- How is the data submitted?
- How is that submission handled?
- What sort of response is given to that submission, and how do we send validation details down that line?
The first two questions there are already answered in this issue's description - and part of the problem is that there's two different types of form submission depending on whether a whole page or just one block is being saved.
Yes, standardisation around how forms are handled across the board would be awesome. I don't know if that's something we can do within the lifetime of CMS 5 though. Do we want to hold this back until CMS 6?
We've been talking about this for a bit and can't come up with an obvious way to do this upfront. So we've schedule a SPIKE for it #1068
Re the ACs, I've intentionally not done them all:
Done
- In-line editable content block are validated with form schema as outline in the SPIKE.
- API for enforcing validation of content block is inline with the one to validate pages.
- Official elemental doc provides clear guidance on how to validate elemental block.
Not done
I've spun these off on to a separate card
Validation occurs both on page save and on individual block save. (sort of, detailed below)
Validation now works works significantly better for individual block save then it does for page save
Page save is somewhat busted. MyBlock::validate()
style validation does work .. however the validation message will show at the top of the page rather than on the element as described in this bug
MyBlock::getCMSCompositeValidator()
aka RequiredFields
validation doesn't seem to work, nor does FormField::validate()
- I tried putting a UrlField on my block which has its own validate()
method though I could save invalid values.
Because the main PR on this issue is already too large I think we need to treat this separately
Validation works both for inline block and "not inline" block.
I assume that "not inline" block means setting private static $inline_editable = false;
It's just a regular DataObject EditForm at this point. It work much better than blocks on the Page EditForm, though RequiredFields
is still a bit busted because instead of a nice red error message you get a red toast that says "validation error" and no indication of what's invalid.
Same issue with a toast showing when adding a message via Model::validate()
using addError()
, as opposed to addFieldError()
which does work OK
Again, this needs to be treated on a separate card
@emteknetnz After making changes from my review, please do an installer CI run to show that the three PRs work nicely together.
@emteknetnz your installer CI run doesn't include elemental tests?
Have updated CI run linked in description to only include behat for elemental
Merged green PRs - reassigning to Steve to do whatever needs doing with the draft PR
PRs merged