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

FIX Improve non-inline validation #1178

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Apr 29, 2024

Issue #1155

Need to merge silverstripe/silverstripe-framework#11213 first to get the has_one RequiredFields validation working correctly and silverstripe/silverstripe-frameworktest#172 to get behat to pass.

There is a mutli-pr CI run linked on the issue

Note that this PR does not touch inline-saving

Scenarios:

  1. DataObject validate() addFieldError()
  2. DataObject validate() arrError()
  3. UrlField validation
  4. RequiredFields on default Title field
  5. has_one relation to File and RequiredFields
  6. has_one relation to SiteTree and RequiredFields

Non-inline editable block:

  1. Already worked
  2. Already worked
  3. Already worked
  4. Fixed in this PR - by default it's a composite field and validation works but doesn't show message on front-end
  5. Already worked
  6. Fixed in framework PR

Inline-editable block page save

  1. Already worked - UX is bad not fixed in this PR - issue
  2. Already worked - UX is bad not fixed in this PR - issue
  3. Fixed in this PR
  4. Fixed in this PR
  5. Already worked
  6. Fixed in framework PR

I used the following blocks to tests these scenarios

<?php

use DNADesign\Elemental\Models\BaseElement;
use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\RequiredFields;
use SilverStripe\Forms\UrlField;
use SilverStripe\Assets\Image;
use SilverStripe\CMS\Model\SiteTree;

class MyBlock extends BaseElement
{
    private static $db = [
        'MyField' => 'Varchar',
        'MyHTML' => 'HTMLText',
        'MyUrl' => 'Varchar',
    ];

    private static $has_one = [
        'MyImage' => Image::class,
        'MyPage' => SiteTree::class,
    ];

    private static $table_name = 'MyBlock';

    private static $singular_name = 'My Block';

    private static $plural_name = 'My Blocks';

    private static $description = 'This is my block';

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

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

    public function validate()
    {
        $result = parent::validate();
        if ($this->Title == 'x') {
            $result->addFieldError('Title', 'Title cannot be x', 'validation');
        }
        if ($this->MyField == 'x') {
            $result->addFieldError('MyField', 'MyField cannot be x', 'validation');
        }
        $c = trim(strip_tags($this->MyHTML ?? ''));
        if ($c == 'x') {
            $result->addFieldError('MyHTML', 'MyHTML cannot be x', 'validation');
        }
        if ($this->MyField == 'z' && $c == 'z') {
            $result->addError('This is a general error message');
        }
        return $result;
    }

    public function getCMSCompositeValidator(): CompositeValidator
    {
        return CompositeValidator::create([RequiredFields::create(['Title', 'MyImage', 'MyPageID'])]);
    }

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->dataFieldByName('MyHTML')->setRows(5);
        $fields->removeByName('MyUrl');
        // URLfield contains its own validate() methods
        $fields->addFieldToTab('Root.Main', UrlField::create('MyUrl', 'My URL'));
        return $fields;
    }
}
<?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';
    }
}

@emteknetnz emteknetnz force-pushed the pulls/5/non-inline-validation branch from 49c532b to 4045901 Compare April 30, 2024 00:12
* This will perform FormField validation
* DataObject validation will happen in saveInto() as part of $element->write()
*/
public function validate($validator)
Copy link
Member Author

@emteknetnz emteknetnz Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously FormField validation wasn't happening on fields on elements e.g. $db = [ 'MyUrl' => UrlField::class ] when saving the page (as opposed to editing the element directly)

@emteknetnz emteknetnz force-pushed the pulls/5/non-inline-validation branch 2 times, most recently from bc3ff5a to 31201ec Compare April 30, 2024 03:59
@emteknetnz emteknetnz marked this pull request as ready for review April 30, 2024 04:08
@emteknetnz emteknetnz force-pushed the pulls/5/non-inline-validation branch 2 times, most recently from 4488a86 to e1674f8 Compare May 1, 2024 23:58
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I save the page without expanding the block, none of the validation seems to happen. e.g. with your test blocks from the description, I add the block and immediately save the page - it should trigger validation errors for the missing required fields but it doesn't.

tests/Behat/features/inline-block-validation.feature Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Forms/TextCheckboxGroupField.php Outdated Show resolved Hide resolved
tests/Behat/features/non-inline-block-validation.feature Outdated Show resolved Hide resolved
tests/Behat/features/non-inline-block-validation.feature Outdated Show resolved Hide resolved
src/Forms/TextCheckboxGroupField.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Forms/ElementalAreaField.php Outdated Show resolved Hide resolved
src/Forms/ElementalAreaField.php Outdated Show resolved Hide resolved
src/Forms/ElementalAreaField.php Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/5/non-inline-validation branch 2 times, most recently from 3c23c58 to f1ad55d Compare May 2, 2024 02:37
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem described in #1178 (review) has not been resolved

@emteknetnz
Copy link
Member Author

If I save the page without expanding the block, none of the validation seems to happen. e.g. with your test blocks from the description, I add the block and immediately save the page - it should trigger validation errors for the missing required fields but it doesn't.

That would be hard to solve.

In the case of inline blocks, currently if they haven't been opened and the page is saved, then then because the child FormBuilder form hasn't been rendered then the element data will not be sent as part of the page form submission. However if the inline-element is opened then page save will submit element data with the rest of the page, and closing the element then saving the page will also send the element data. So to solve this we'd need to render the FormBuilder forms for ALL unopened elements on page save

In the case of non-inline blocks, I really don't know what can be done since their forms are never rendered on the page and and thus their data is never sent.

The fact this doesn't work doesn't actually matter given the AC is Saving/publishing a page will trigger validation on its child block if they are in a a dirty state. - the elements in question aren't dirty, they're just created in an invalid state.

We could split this off as a seperate card, though I honestly don't know how or even if we should attempt a fix here

@GuySartorelli
Copy link
Member

GuySartorelli commented May 5, 2024

We could split this off as a seperate card, though I honestly don't know how or even if we should attempt a fix here

I think that's worth looking into. There's a tradeoff to be made between:

  1. always having the form rendered (and just hidden until it's expanded) - which means we never have to do workarounds like explicitly render it when hitting the save button on the block
  2. potential performance loss from eagerly getting the required data to render the form for all blocks on a given page

I'll open a separate card about this for discussion.

Edit: #1183

@emteknetnz emteknetnz force-pushed the pulls/5/non-inline-validation branch from f1ad55d to 9539290 Compare May 5, 2024 23:29
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one thing left and then we're good, I think.

src/Forms/TextCheckboxGroupField.php Show resolved Hide resolved
@GuySartorelli
Copy link
Member

GuySartorelli commented May 6, 2024

Reran CI after merging frameworktest. Confirmed they are running with the correct commit:
silverstripe/frameworktest 1.x-dev d12463a
silverstripe/silverstripe-frameworktest@d12463a

@GuySartorelli
Copy link
Member

behat failing

@emteknetnz emteknetnz force-pushed the pulls/5/non-inline-validation branch 2 times, most recently from 8516a9e to eba37f1 Compare May 6, 2024 21:37
@emteknetnz
Copy link
Member Author

I've added an extra code change that's from a different PR that's blocked on this one which is required to get the inline-block-validation.feature behat tests to pass

@@ -139,7 +139,7 @@ public function save(array $data, Form $form): HTTPResponse
$dataWithoutNamespaces = static::removeNamespacesFromFields($data, $element->ID);

// Update and write the data object which will trigger model validation
$element->update($dataWithoutNamespaces);
$element->updateFromFormData($dataWithoutNamespaces);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this code change in from a different PR that's blocked on this one which is required to get the inline-block-validation.feature behat tests to pass

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I didn't notice this before, but for inline-editable blocks when you save the page, only one required field validation message appears at a time.

So if I don't enter any data I only get told about the title, then I add a title and I'm told about the image, then I add an image and I'm told about the page.

The HTTP response does have all three validation errors at the same time - at this stage I don't know if getting them to display is part of this card or not so if there's another card to handle this please just link to it and I'll be okay with that.

@GuySartorelli
Copy link
Member

GuySartorelli commented May 7, 2024

Rerunning failed behat tests now that the framework PR is merged, but regardless of whether that goes green I need at least a response to my above comment.

@emteknetnz emteknetnz force-pushed the pulls/5/non-inline-validation branch from eba37f1 to e2f742b Compare May 7, 2024 02:48
@GuySartorelli
Copy link
Member

Still need to respond to #1178 (review)

@emteknetnz
Copy link
Member Author

Split of as separate card that can be refined

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the understanding that there's still a few issues left to be done before this functionality is considered 100% working 😅

@GuySartorelli GuySartorelli merged commit 4e0ba30 into silverstripe:5 May 7, 2024
13 checks passed
@GuySartorelli GuySartorelli deleted the pulls/5/non-inline-validation branch May 7, 2024 04:30
emteknetnz added a commit to creative-commoners/silverstripe-elemental that referenced this pull request May 7, 2024
…non-inline-validation

FIX Improve non-inline validation
emteknetnz added a commit to creative-commoners/silverstripe-elemental that referenced this pull request May 7, 2024
…non-inline-validation

FIX Improve non-inline validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants