NEW Validation of fields when inline saving #1150

emteknetnz · 2024-02-26T05:55:15Z

Issue #329

Requires silverstripe/silverstripe-admin#1685 to be functional

Requires silverstripe/silverstripe-frameworktest#169 for behat to pass

Use the following test block

<?php

use DNADesign\Elemental\Models\BaseElement;
use SilverStripe\Forms\CompositeValidator;
use SilverStripe\Forms\RequiredFields;
use SilverStripe\Forms\UrlField;

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

    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'])]);
    }

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

src/Controllers/ElementalAreaController.php

GuySartorelli · 2024-03-26T21:35:19Z

I think I need a second opinion on this before I okay it - it feels like the flow is too convoluted and will cause trouble for us down the line, which I've mentioned in more detail in comments above.
@maxime-rainville @silverstripe/core-team does anyone else have opinions about this?

emteknetnz · 2024-03-26T23:10:33Z

In terms of refactoring to make this less convoluted, as mentioned earlier the bottle neck is really the use of the GraphQL HOC on the publish button. So in order to refactor I can think of a couple of options:

a) have way to use apollo/graphql so you can do an "inline" query rather than as a HOC (I don't know how to do this) - then we could use the mutation from Element.js
b) A possibly easier option (easier in that we don't have 2 different ways to do graphql on top of all the other multiple ways of doing things), is to just not use GraphQL and create a little publish endpoint on ElementalAreaController and send a basic fetch() request from Element.js. That's fairly trivial to implement.

GuySartorelli · 2024-03-26T23:17:15Z

Another possibly easier option (so we don't have 2 different ways to do graphql on top of all the other multiple ways of doing things), is to just not bother with graphql and create a little publish endpoint on ElementalAreaController and send a basic fetch() request from Element.js. That's fairly trivial.

That sounds preferable to me.
Bonus points (but not a requirement) if it's something that can be handled via formschema so we can reuse that scenario for any formschema-based forms which have versioned records going forward.

emteknetnz · 2024-03-27T00:27:28Z

Publishing has nothing to do with formschema. This would just be method on a controller that calls $element->publishRecursive(); and some others things like permission checks and returned the correct https status code

I did a POC a while back of using regular controllers in elemental so I'd largely just copy that

maxime-rainville

To the extent that all the other actions on the block (archive, duplicate, unpublish, etc) are still going through GraphQL, I would question the wisdom of moving publication off GraphQL.

I do think the way we write GraphQL request is dumb and unduly complicated. I would be in favour of coming up with a better standard for creating those GraphQL requests.

I think the terraformers have done some customisation to an important project in that space. Might be worth validating with them if that's going to bit them in the ass.

maxime-rainville · 2024-04-01T09:00:43Z

client/src/components/ElementEditor/Element.js

+import * as toastsActions from 'state/toasts/ToastsActions';
+import { addFormChanged, removeFormChanged } from 'state/unsavedForms/UnsavedFormsActions';
+
+export const ElementContext = createContext(null);


This probably should be in its own file.

Will be addressed in follow up card #1164

maxime-rainville · 2024-04-01T09:09:20Z

client/src/components/ElementEditor/Element.js

@@ -248,6 +423,19 @@ class Element extends Component {
      this.getVersionedStateClassName()
    );

+    // eslint-disable-next-line react/jsx-no-constructed-context-values
+    const providerValue = {


I think this is abusing the useContext hook. Context normally passes a simple value down the virtual dom. This is passing down values and a bunch of handlers to update things.

Seems like the component is already heavily using redux anyway, so layering context on top of it seems to be adding complexity.

If we're not going to put this in redux, then we should minimally be extremely clear about what each entry will is doing by putting the context in a dedicated file with clear documentation of what each key is doing.

Will be addressed in follow up card #1164

maxime-rainville · 2024-04-01T09:10:58Z

client/src/components/ElementEditor/Element.js

We're already rewriting most of the component ... seems like this would have been a good opportunity to refactor has a functional component.

Considering how long we've been working on this I think that should be explicitly out of scope.

Will be addressed in follow up card #1164

maxime-rainville · 2024-04-01T09:20:48Z

client/src/components/ElementEditor/Element.js

+    }
+  }
+
+  componentDidUpdate() {


Triggering HTTP requests by updating the state and picking up the state update in componentDidUpdate seems incredibly weird and questionable.

It's the only way I could work out how to do this.

Essentially we need to ensure that several dependent occurred in a sequence sending a publish request, e.g. when publishing

The form has rendered before sending a request so that validation can occur - it won't have rendered if the element has been toggled open

If the form is dirty, save the form first to trigger validation. If it's valid then publish.

Have we tried using Promises?

The way I would approach this is:

Define a waitForFormRender method. The method would return a Promise.

If the form as already rendered, then Promise.resolve is return.

If the form as not resolved a new promise is returned that will be resolved when handleFormInit is called.

Update handlePublishButtonClick to call waitForFormRender and trigger whatever request is needed when the promise resolves.

Will be addressed in follow up card #1164

maxime-rainville · 2024-04-01T09:28:26Z

client/src/components/ElementEditor/InlineEditForm.scss

+
+  // Render the form off the screen so that it's not visible
+  // Note that using display:none will mean the form is not rendered at all and cannot be submitted
+  &--rendered-not-visible {


Why are we rendering the form off the screen?

Won't this cause accessibility problems?

There's an aria-hidden (or whatever the actual thing is) to avoid this causing problems for screen-reader users.
It needs to be rendered so the form can submit with the correct values. Otherwise, the formschema form submission has no values to submit.

It's an unfortunate quirk of using formschema in an element that can be collapsed.

That's good context. This seems like a very important point that should be document in the component using this class rather than the style sheet.

Have put a note about this in Content.js which is where this css class is added

maxime-rainville · 2024-04-01T09:34:49Z

client/src/components/ElementActions/PublishAction.js

@@ -132,6 +46,19 @@ const PublishAction = (MenuComponent) => (props) => {
    toggle: props.toggle,
  };

+  useEffect(() => {
+    if (formHasRendered && doPublishElement) {


We are triggering an action based on a state update we are getting from somewhere else. This seems very bad.

Will be addressed in follow up card #1164

maxime-rainville · 2024-04-01T09:42:50Z

client/src/components/ElementEditor/Element.js

+  refetchElementalArea() {
+    // This will trigger a graphql readOneElementalArea request that will cause this
+    // element to re-render including any updated title and versioned badge
+    window.ss.apolloClient.queryManager.reFetchObservableQueries();


Surely there's a better way to refresh a single query than force every other query to re-run?

Will be addressed in follow up card #1164

michalkleiner · 2024-04-01T10:41:14Z

To the extent that all the other actions on the block (archive, duplicate, unpublish, etc) are still going through GraphQL, I would question the wisdom of moving publication off GraphQL.

☝️

I do think the way we write GraphQL request is dumb and unduly complicated. I would be in favour of coming up with a better standard for creating those GraphQL requests.

👍

emteknetnz · 2024-04-01T21:10:27Z

Based on Max's reponse it seems like the plan is basically

Redo the graphql publish query in a non-hoc fashion
Move save + publish logic to Element.js
Possibly move ElementContext to its own file (though not sure there will be much left after step 2)

Like has Guy has already replied, I'm not a fan of "refactor this to become a functional component" alongside an existing complex PR because that increase in scope makes things FAR harder to peer review.

However to achieve 1) it's a requirement because apollo now uses hooks - https://www.apollographql.com/docs/react/data/mutations

The existing apollo hoc method is deprecated https://www.apollographql.com/docs/react/api/react/hoc/

Given that this PR has been open and worked on for an extremely long time, can I propose that we just merge this as is and then open a new card to refactor this to a functional component and implement 1-3?

maxime-rainville

Given that this PR has been open and worked on for an extremely long time, can I propose that we just merge this as is and then open a new card to refactor this to a functional component and implement 1-3?

I would be keen to see the timeout, weird state management logic and the ElementContext solve in this card.

Given that GraphQL bits require coming up with new and improve way of doing GraphQL queries, I'm fine with putting that in a different card.

maxime-rainville · 2024-04-04T22:10:44Z

client/src/components/ElementEditor/InlineEditForm.scss

+
+  // Render the form off the screen so that it's not visible
+  // Note that using display:none will mean the form is not rendered at all and cannot be submitted
+  &--rendered-not-visible {


That's good context. This seems like a very important point that should be document in the component using this class rather than the style sheet.

maxime-rainville · 2024-04-04T22:21:53Z

client/src/components/ElementEditor/Element.js

+    }
+  }
+
+  componentDidUpdate() {


Have we tried using Promises?

The way I would approach this is:

Define a waitForFormRender method. The method would return a Promise.

If the form as already rendered, then Promise.resolve is return.

If the form as not resolved a new promise is returned that will be resolved when handleFormInit is called.

Update handlePublishButtonClick to call waitForFormRender and trigger whatever request is needed when the promise resolves.

maxime-rainville · 2024-04-04T22:55:23Z

client/src/components/ElementEditor/Element.js

+    // time under certain conditions, specifically during a behat test when trying to publish a closed
+    // block when presumably the apollo cache is empty (or something like that). This happens late and
+    // there are no hooks/callbacks available after this happens the input onchange handlers are fired
+    setTimeout(() => {


If this is waiting for a HTTP request to finish, it's entirely possible it will take more than .5 sec to complete.

If we know what action in redux form is triggering this behaviour, we could add our own logic in our reducer to handle it and trigger our own follow up action.

Will be addressed in follow up card #1164

emteknetnz · 2024-04-05T03:56:38Z

I would be keen to see the timeout, weird state management logic and the ElementContext solve in this card.
Given that GraphQL bits require coming up with new and improve way of doing GraphQL queries, I'm fine with putting that in a different card.

Most of the weird state management logic + ElementContext goes away if we resolve the GraphQL query. Hence why I suggested merging this as is and opening a new card. No point refactoring stuff as part of this peer review only to get rid of it almost straight away.

This card has been in peer review for over a month :-/ Are people OK with my suggestion to just merge this as (I believe it works functionally just fine as is) and do any additional refactoring work in a new card?

maxime-rainville · 2024-04-08T09:12:41Z

I'm really not sure what the value of merging something that introduces a lot of bad construct is if we are going to immediately have a follow up card.

I'll just take all my grievances and convert them to ACs on the new card. You'll end up with something like:

Simplify the GraphQL publication call.
Remove abusive context usage on PublishAction.
Don't trigger action on state change.
Don't use setTimeout in Element.

emteknetnz · 2024-04-08T21:11:39Z

I've created a follow up card for refactoring and put into refinement #1164

maxime-rainville

I tested the following scenarios:

Validation for not inline block ✔️
Validation for errors thrown by the validate method on a block when manually saving a block ✔️
Validation for errors thrown by the validate method on a block when saving the page ⚠️
Field level validation (putting a non-numeric value in a numeric field) ❌

Scenario 3 sort of works, but the validation message is attached to the page instead. I take it this is covered by #1155

Scenario 4 doesn't work either on block save or page save. It does work for non-inline-block. Are we splitting this up as well?

emteknetnz · 2024-04-09T04:44:23Z

Yes 3 is will be covered by #1155

Re 4 - it's likely that the rendered <input type="number"> is letting you enter non-numeric e.g. "abc" though it won't actually submit the value, either via inline save or page save e.g. network panel XHR request shows a blank value for the numeric field

Try using UrlField instead - you should see that validation is working correctly for that.

Also for NumericField I do have this setup on my local, which does trigger for values for '1' on inline save:

// app/src/MyNumbericField.php
use SilverStripe\Forms\NumericField;

class MyNumbericField extends NumericField
{
    private static $extensions = [
        MyNumericFieldExtension::class
    ];
}

// app/src/MyNumericFieldExtension.php
use SilverStripe\Core\Extension;
use SilverStripe\Forms\NumericField;
use SilverStripe\Forms\Validator;

/**
 * @extends Extension<NumericField>
 */
class MyNumericFieldExtension extends Extension
{
    public function updateValidationResult($result, Validator $validator)
    {
        if ($this->owner->value == 1) {
            $validator->validationError($this->owner->getName(), 'This field cannot be 1');
        }
    }
}

// app/src/MyBlock.php
// ...
class MyBlock extends BaseElement
{
    private static $db = [
        'MyInt' => 'Int',
    ];
    // ...
    public function getCMSFields()
    {
        // ...        
        $fields->removeByName('MyInt');
        $fields->addFieldToTab('Root.Main', MyNumbericField::create('MyInt', 'My Int'));
    }
}

maxime-rainville · 2024-04-09T06:10:43Z

Still doesn't work for me

validation-not-triggering-2024-04-09_17.59.22.webm

Here's the sample block I was using

<?php

use DNADesign\Elemental\Models\BaseElement;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\NumericField;
use SilverStripe\Forms\UrlField;

class ValidationBlock extends BaseElement
{
    private static $db = [
        'Integer' => 'Int',
        'Url' => 'Varchar(255)'
    ];

    private static $inline_editable = true;

    private static $singular_name = 'validation block';

    private static $plural_name = 'validation blocks';

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

    private static $table_name = 'S_EB_ValidiationBlock';

    public function getCMSFields()
    {
        $this->beforeUpdateCMSFields(function (FieldList $fields) {
            $fields->addFieldToTab(
                'Root.Main',
                NumericField::create('Integer'),
                UrlField::create('Url')
            );

        });

        return parent::getCMSFields();
    }

    public function getType()
    {
        return _t(__CLASS__ . '.BlockType', 'Validation');
    }

    public function getSummary()
    {
        return '';
    }

    public function validate()
    {
        $result = parent::validate();

        // This will add a field specific error to the ValidationResult
        if ($this->Integer > 10) {
            $result->addFieldError('Integer', 'Integer must be less than 11');
        }

        return $result;
    }
}

class FormValidationBlock extends ValidationBlock
{
    private static $inline_editable = false;

    private static $singular_name = 'not inline validation block';

    private static $plural_name = 'not inline validation blocks';

    public function getType()
    {
        return _t(__CLASS__ . '.BlockType', 'Not inline Validation block');
    }

}

The problem might be with how I'm using UrlField.

I tried hacking my numeric field to always fail. The page save went through, but the other scenarios got blocked.

validation-not-triggering-2024-04-09_18.09.14.webm

emteknetnz · 2024-04-09T07:26:59Z

Re numeric field - the inline validation was working correctly in your video. This PR is only for inline validation. It does not cover non-inline dataobject validation / page validation. Those concerns are being handled on #1155.

Re the UrlField - that's really weird. I've copied your way of adding UrlField to my local using $this->beforeUpdateCMSFields() and validation works correctly for me when inline saving. Could you have a look in your network panel to see the XHR request that's being returned when you inline save a non-url in that field? For me I'm getting a formschema response with an errors node

Perhaps try dumping this into your project, it works for me

<?php

use DNADesign\Elemental\Models\BaseElement;
use SilverStripe\Forms\UrlField;
use SilverStripe\Forms\FieldList;

class MrBlock extends BaseElement
{
    private static $db = [
        'MyUrlOne' => 'Varchar',
        'MyUrlTwo' => 'Varchar',
    ];

    private static $table_name = 'MrBlock';

    private static $singular_name = 'Mr Block';

    private static $plural_name = 'Mr Blocks';

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

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

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

    private static $inline_editable = true;
 
    public function getCMSFields()
    {
        $this->beforeUpdateCMSFields(function (FieldList $fields) {
            $fields->addFieldToTab(
                'Root.Main',
                UrlField::create('MyUrlTwo', 'My Url Two')
            );
        });

        $fields = parent::getCMSFields();
        $fields->removeByName('MyUrlOne');
        $fields->addFieldToTab('Root.Main', UrlField::create('MyUrlOne', 'My Url One'));
        return $fields;
    }
}

If that's still not working, please confirm you have the latest hash of this PR as well as this PR

maxime-rainville · 2024-04-14T09:31:19Z

I'm an idiot.

addFieldsToTab() expects its second parameter to be an array of fields. If you give it a field without wrapping it in an array, it will still work however.

However, if you try passing another form field to addFieldsToTab()'s third parameter, nothing will happen and it will be ignored.

So my UrlField wasn't doing anything and the URL field was being scaffolded as a plain text field.

I created an issue to strongly type FieldList in CMS 6 silverstripe/silverstripe-framework#11198

maxime-rainville

Begrudgingly approved with the understanding that #1155 and #1164 will follow up

GuySartorelli · 2024-04-16T23:11:07Z

Merging on Max's behalf

We've decided to merge as-is with followup work to address my concerns

emteknetnz mentioned this pull request Feb 26, 2024

SPIKE Inline validation #1148

Closed

emteknetnz force-pushed the pulls/5/inline-validation branch 2 times, most recently from 577ca62 to 3f8cb66 Compare February 26, 2024 06:00

emteknetnz mentioned this pull request Feb 26, 2024

Implement validation method for inline editable form #329

Closed

emteknetnz force-pushed the pulls/5/inline-validation branch 15 times, most recently from 9ea8753 to bb23f17 Compare February 29, 2024 04:39

emteknetnz mentioned this pull request Feb 29, 2024

FormBuilder validation message does not show for TextCheckboxGroupField #1154

Closed

emteknetnz force-pushed the pulls/5/inline-validation branch from bb23f17 to 4837d2b Compare February 29, 2024 20:56

emteknetnz commented Feb 29, 2024

View reviewed changes

src/Controllers/ElementalAreaController.php Show resolved Hide resolved

emteknetnz force-pushed the pulls/5/inline-validation branch 2 times, most recently from 22aab0d to 9bb7edb Compare February 29, 2024 22:49

emteknetnz changed the title ~~ENH Inline validation~~ NEW Validation of fields when inline saving Feb 29, 2024

emteknetnz force-pushed the pulls/5/inline-validation branch 5 times, most recently from 048c625 to a4ffd80 Compare March 1, 2024 00:05

emteknetnz force-pushed the pulls/5/inline-validation branch from e387931 to 4419ecb Compare March 26, 2024 03:09

maxime-rainville reviewed Apr 1, 2024

View reviewed changes

maxime-rainville reviewed Apr 4, 2024

View reviewed changes

emteknetnz mentioned this pull request Apr 8, 2024

Refactor inline saving and publishing #1164

Closed

8 tasks

emteknetnz force-pushed the pulls/5/inline-validation branch 2 times, most recently from b76fda4 to badbb6b Compare April 8, 2024 21:57

maxime-rainville reviewed Apr 9, 2024

View reviewed changes

maxime-rainville approved these changes Apr 14, 2024

View reviewed changes

emteknetnz force-pushed the pulls/5/inline-validation branch 2 times, most recently from 65d6bb2 to 1193414 Compare April 16, 2024 05:35

NEW Validation of fields when inline saving

4789b34

emteknetnz force-pushed the pulls/5/inline-validation branch from 1193414 to 4789b34 Compare April 16, 2024 07:00

GuySartorelli merged commit e1b6a3b into silverstripe:5 Apr 16, 2024
13 checks passed

emteknetnz mentioned this pull request Apr 18, 2024

Unable to inline save related data #1170

Closed

emteknetnz deleted the pulls/5/inline-validation branch April 22, 2024 04:06

NEW Validation of fields when inline saving #1150

NEW Validation of fields when inline saving #1150

Conversation

emteknetnz commented Feb 26, 2024 • edited by GuySartorelli Loading

GuySartorelli commented Mar 26, 2024

emteknetnz commented Mar 26, 2024 • edited Loading

GuySartorelli commented Mar 26, 2024 • edited Loading

emteknetnz commented Mar 27, 2024 • edited Loading

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkleiner commented Apr 1, 2024

emteknetnz commented Apr 1, 2024 • edited Loading

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz commented Apr 5, 2024 • edited Loading

maxime-rainville commented Apr 8, 2024

emteknetnz commented Apr 8, 2024

maxime-rainville left a comment • edited Loading

Choose a reason for hiding this comment

emteknetnz commented Apr 9, 2024 • edited Loading

maxime-rainville commented Apr 9, 2024

emteknetnz commented Apr 9, 2024

maxime-rainville commented Apr 14, 2024 • edited Loading

maxime-rainville left a comment

Choose a reason for hiding this comment

GuySartorelli commented Apr 16, 2024

emteknetnz commented Feb 26, 2024 •

edited by GuySartorelli

Loading

emteknetnz commented Mar 26, 2024 •

edited

Loading

GuySartorelli commented Mar 26, 2024 •

edited

Loading

emteknetnz commented Mar 27, 2024 •

edited

Loading

emteknetnz commented Apr 1, 2024 •

edited

Loading

emteknetnz commented Apr 5, 2024 •

edited

Loading

maxime-rainville left a comment •

edited

Loading

emteknetnz commented Apr 9, 2024 •

edited

Loading

maxime-rainville commented Apr 14, 2024 •

edited

Loading