Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement validation method for inline editable form #329

Closed
brynwhyman opened this issue Aug 21, 2018 · 17 comments
Closed

Implement validation method for inline editable form #329

brynwhyman opened this issue Aug 21, 2018 · 17 comments

Comments

@brynwhyman
Copy link

brynwhyman commented Aug 21, 2018

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

SPIKE

There was a spike to investigate this (which includes a POC PR):

New issues created

PRs

Multi-PR CI run

@clarkepaul
Copy link

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

@ScopeyNZ
Copy link
Contributor

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.

@chrispenny
Copy link
Contributor

chrispenny commented Nov 19, 2020

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

@brynwhyman
Copy link
Author

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

@chrispenny
Copy link
Contributor

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).

@brynwhyman
Copy link
Author

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.

@GuySartorelli
Copy link
Member

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;
}

@GuySartorelli
Copy link
Member

GuySartorelli commented Jun 11, 2023

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)

@maxime-rainville
Copy link
Contributor

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

public function getElementForm($elementID)
{
$scaffolder = Injector::inst()->get(EditFormFactory::class);
$element = BaseElement::get()->byID($elementID);
if (!$element) {
return null;
}
/** @var Form $form */
$form = $scaffolder->getForm(
$this,
sprintf(static::FORM_NAME_TEMPLATE ?? '', $elementID),
['Record' => $element]
);
if (!$element->canEdit()) {
$form->makeReadonly();
}
$form->addExtraClass('element-editor-editform__form');
return $form;
}

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.

@GuySartorelli
Copy link
Member

GuySartorelli commented Jun 13, 2023

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?

@maxime-rainville
Copy link
Contributor

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

@GuySartorelli
Copy link
Member

Merged green PRs - reassigning to Steve to do whatever needs doing with the draft PR

@GuySartorelli
Copy link
Member

PRs merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants