Validation messages on composite fields

[emteknetnz]

Split off from peer review comment https://github.com/silverstripe/silverstripe-elemental/pull/1178/files#r1585863104

Composite field validation messages do not show correctly (at least for one tested field in elemental) because the validation message is attached to a child field, not the parent composite field that has logic in the template to show validation messages

Acceptance criteria

• Add the logic in https://github.com/silverstripe/silverstripe-elemental/pull/1178/files#r1585863104 from elemental to composite field
• Remove the logic from elemental
• If practical handle multiple validation errors i.e. if both fields in a composite field have validation errors, then show both the errors at once

Notes

• Messages have three methods
  • one to get the message text
  • one to get the message casting
  • one to get the message type
• The above methods all assume each field has only a single message - so we may need to massage the API a little for CompositeField, so each child field can have a message with a different cast and type which are all displayed correctly in the CompositeField context.

[emteknetnz]

I don't think we need to do this card, composite fields seem like they're generally fine as is. They are happy to show multiple validation errors

A quick summary of things that are not working right now and could be fixed on different cards:

• ToggleCompositeField doesn't pop open when there are validation errors on children
• MoneyField doesn't work with RequiredFields (though possible shouldn't)
• There are a bunch of missing react components for composite fields

Details

I looked around for composite fields and found the following:

framework

• FieldGroup
• SelectionGroup + SelectionGroup_Item
• ToggleCompositeField
• MoneyField (doesn't extend CompositeField though uses DBMoney extends DBComposite, means that multiple validation warnings are practical)

framework, though not applicable for validation

• Tab + TabSet

Elemental

• TextCheckboxGroupField - validation handled in this elemental PR

I used the following test class:

<?php

use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\FieldGroup;
use SilverStripe\Forms\RequiredFields;
use SilverStripe\Forms\TextField;
use SilverStripe\Forms\SelectionGroup;
use SilverStripe\Forms\SelectionGroup_Item;
use SilverStripe\Forms\LiteralField;
use SilverStripe\Forms\MoneyField;
use SilverStripe\Forms\ToggleCompositeField;

class Page extends SiteTree
{
    private static $db = [
        'FieldOne' => 'Varchar',
        'FieldTwo' => 'Varchar',
        'FieldThree' => 'Varchar',
        'FieldFour' => 'Varchar',
        'FieldFive' => 'Varchar',
        'FieldSix' => 'Money',
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->addFieldsToTab(
            'Root.Main',
            [
                # FieldGroup
                new FieldGroup(
                    new FieldGroup(
                        new TextField('FieldOne')
                    ),
                    new FieldGroup(
                        new TextField('FieldTwo')
                    )
                ),
                # SelectionGroup
                (new SelectionGroup('FieldThree', [
                    new SelectionGroup_Item(
                        'x',
                        new LiteralField('nx', 'Option x'),
                    ),
                    new SelectionGroup_Item(
                        'y',
                        new LiteralField('ny', 'Option y'),
                    ),
                ]))->setRightTitle('FieldThree'),
                # ToggleCompositeField
                new ToggleCompositeField('nested', 'FieldFour and FieldFive', [
                    new TextField('FieldFour'),
                    new TextField('FieldFive'),
                ]),
                # MoneyField
                new MoneyField('FieldSix'),
            ]
        );
        $fields->removeByName('Content');
        return $fields;
    }

    public function validate()
    {
        $result = parent::validate();
        if ($this->FieldOne == 'x') {
            $result->addFieldError('FieldOne', 'FieldOne cannot be x');
        }
        if ($this->FieldTwo == 'x') {
            $result->addFieldError('FieldTwo', 'FieldTwo cannot be x');
        }
        if ($this->FieldThree == 'x') {
            $result->addFieldError('FieldThree', 'FieldThree cannot be x');
        }
        if ($this->FieldFour == 'x') {
            $result->addFieldError('FieldFour', 'FieldFour cannot be x');
        }
        if ($this->FieldFive == 'x') {
            $result->addFieldError('FieldFive', 'FieldFive cannot be x');
        }
        if ($this->FieldSix->getCurrency() == 'x') {
            $result->addFieldError('FieldSix', 'FieldSix currency cannot be x');
        }
        return $result;
    }

    public function getCMSCompositeValidator(): CompositeValidator
    {
        return CompositeValidator::create([
            RequiredFields::create(['FieldOne', 'FieldTwo', 'FieldThree', 'FieldFour', 'FieldFive', 'FieldSix'])
        ]);
    }
}

Save a new page, RequiredFields give the following validation warnings:

Putting in x's, give the following validation warnings:

Note that the ToggleFields will be closed and not show the validation warnings until you open it for both RequiredFields and for validate(), which isn't great UX, though is also basically unrelated to card

Other then that, RequiredFields isn't great for MoneyField, though again that would be a seperate card, and honestly a pretty low priority one given you can just (and debatable should) use validate().

[emteknetnz]

To test how this work in elemental, I used the following test code:

<?php

use DNADesign\Elemental\Models\BaseElement;
use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\FieldGroup;
use SilverStripe\Forms\RequiredFields;
use SilverStripe\Forms\TextField;
use SilverStripe\Forms\SelectionGroup;
use SilverStripe\Forms\SelectionGroup_Item;
use SilverStripe\Forms\LiteralField;
use SilverStripe\Forms\MoneyField;
use SilverStripe\Forms\ToggleCompositeField;

class MyBlock extends BaseElement
{
    private static $db = [
        'FieldOne' => 'Varchar',
        'FieldTwo' => 'Varchar',
        'FieldThree' => 'Varchar',
        'FieldFour' => 'Varchar',
        'FieldFive' => 'Varchar',
        'FieldSix' => 'Money',
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->removeByName('Title');
        $fields->removeByName('FieldOne');
        $fields->removeByName('FieldTwo');
        $fields->removeByName('FieldThree');
        $fields->removeByName('FieldFour');
        $fields->removeByName('FieldFive');
        $fields->removeByName('FieldSix');
        $fields->addFieldsToTab(
            'Root.Main',
            [
                # FieldGroup
                new FieldGroup(
                    new FieldGroup(
                        new TextField('FieldOne')
                    ),
                    new FieldGroup(
                        new TextField('FieldTwo')
                    )
                ),
                # SelectionGroup
                (new SelectionGroup('FieldThree', [
                    new SelectionGroup_Item(
                        'x',
                        new LiteralField('nx', 'Option x'),
                    ),
                    new SelectionGroup_Item(
                        'y',
                        new LiteralField('ny', 'Option y'),
                    ),
                ]))->setRightTitle('FieldThree'),
                # ToggleCompositeField
                new ToggleCompositeField('nested', 'FieldFour and FieldFive', [
                    new TextField('FieldFour'),
                    new TextField('FieldFive'),
                ]),
                # MoneyField
                new MoneyField('FieldSix'),
            ]
        );
        $fields->removeByName('Content');
        return $fields;
    }

    public function validate()
    {
        $result = parent::validate();
        if ($this->FieldOne == 'x') {
            $result->addFieldError('FieldOne', 'FieldOne cannot be x');
        }
        if ($this->FieldTwo == 'x') {
            $result->addFieldError('FieldTwo', 'FieldTwo cannot be x');
        }
        if ($this->FieldThree == 'x') {
            $result->addFieldError('FieldThree', 'FieldThree cannot be x');
        }
        if ($this->FieldFour == 'x') {
            $result->addFieldError('FieldFour', 'FieldFour cannot be x');
        }
        if ($this->FieldFive == 'x') {
            $result->addFieldError('FieldFive', 'FieldFive cannot be x');
        }
        if ($this->FieldSix->getCurrency() == 'x') {
            $result->addFieldError('FieldSix', 'FieldSix currency cannot be x');
        }
        return $result;
    }

    public function getCMSCompositeValidator(): CompositeValidator
    {
        return CompositeValidator::create([
            RequiredFields::create(['FieldOne', 'FieldTwo', 'FieldThree', 'FieldFour', 'FieldFive', 'FieldSix'])
        ]);
    }
}

<?php

class MyNonInlineBlock extends MyBlock
{
    private static $inline_editable = false;

    private static $table_name = 'MyNonInlineBlock';

    private static $singular_name = 'My non-inline Block';

    private static $plural_name = 'My non-inline Blocks';

    private static $description = 'This is my non-inline block';

    private static $icon = 'font-icon-block-content';

    public function getType()
    {
        return 'My non-inline Block';
    }
}

Non-inline edit elements, edited as regular DataObjects, worked exactly the same as in a Page context

Inilne edit elements were terrible as they didn't appear to have react components :-/ That's obviously outside the scope of this card

[GuySartorelli]

@emteknetnz Above you have said "TextCheckboxGroupField - validation handled in this elemental PR" - but there's no link to a PR?

[GuySartorelli]

Also the note about handling multiple validation messages was moved here specifically because TextCheckboxGroupField can't handle those, so can you please confirm if TextCheckboxGroupField has been updated to handle multiple validation messages?

[emteknetnz]

Have updated link

please confirm if TextCheckboxGroupField has been updated to handle multiple validation messages?

At a practical level it simply doesn't need to support multiple validation messages. TextCheckboxGroupField is a combination of Title (TextField) and ShowTitle (Checkbox)

For this to require multiple validation messages a developer would need to put a validation rule on ShowTitle that somehow fails i.e. the value isn't a 0 or a 1. No one should put a validation rule like that on a checkbox

[GuySartorelli]

At a practical level it simply doesn't need to support multiple validation messages.

Yes, it does. We allow developers to apply validation for both child fields. That validation will be processed, and both validation messages will exist at the same time. They should both be displayed at the same time. This is exactly what you have demonstrated above that all other composite fields do already. From the screenshots it seems clear that the messages are concatenated together with a comma inbetween.

That's trivial to do, and is correct to do. I only merged the PR for TextCheckboxGroupField as it was because this AC was moved into this card. I wouldn't have merged it otherwise. Please make the change.

[emteknetnz]

Discussed offline will close

[emteknetnz]

Note re raw CompositeField (not subclass) - works correct - tested on Page

# CompositeField
new CompositeField(
    new TextField('FieldSeven'),
    new TextField('FieldEight')
)

// + adding 'x' validate() and adding to RequiredFields