silverstripe/silverstripe-elemental

SPIKE Come up with an approach to validate inline blocks

Closed this issue · 11 comments

maxime-rainville commented

We have a gnarly card to implement validation for inline blocks.

TODO

  • Document the various ways in which inline blocks are saved.
  • Review possible a ways to validate form schemas ond other potential form schema improvements that could help with Elemental
  • Identify potential approaches to implement validation for inline blocks. Ideally those could be implemented in a minor.

Timebox

2 days (with potential for extension if need be)

Parent card

#329

PRs

emteknetnz commented

tl;dr - IMO there is only one "correct" way to handle inline validation which is to utilise the existing FormSchema.php + FormBuilder.js functionality to do everything in relation to inline XHR saves and validation.

IMO there aren't any good viable alternatives to doing this, so I haven't provide any

What's happening right now is we use the existing functionality for rendering the inline form, but not for validating/saving the data in it.

I've got a draft PR that is the net result of my investigation. This was very slow and painful trying to work out what's currently happening at trying to move it toward the state it should have always been in. As usual everything is hard with incomplete implementations, things are undocumented and there's lots of jumping around the place.

Going forward I think we should simply continue with the draft PR and work on that until it's all wired together. This may require some work in silverstripe/admin FormBuilder.js as well. We could also consider if we want a more centralised FormSchema API controller, though the work here could and possibly should happen independently of that for now and we just migrate to that later.

The following is a bit of a dump of various things:

GETing elemental data:

  • Frontend uses FormBuilderLoader.js to request a FormSchema
  • GET FormSchema request is made to /admin/elemental-area/schema/elementForm/3
  • There is a controllers ElementalAreaController.php extends CMSMain (which extends LeftAndMain) which has a $url_segment = 'elemental-area'
  • Routing goes to LeftAndMain::schema() with params $FormName = elementForm and $ItemID = 3
  • In LeftAndMain::schema() .. $formMethod = "get{$formName}" .. $form = $this->{$formMethod}($ItemID); which translates to ElementalAreaController::getElementForm($elementID): Form;
  • In ElementalAreaController::getElementForm() .. $form = (EditFormFactory extends DefaultFormFactory)::getForm(). EditFormFactory does the elemental namespacing which prefixing Elements with "PageElements__" because you can edit multiple elements in an ElementalArea at a time.
  • This works, though it's pretty confusing routing because there's a lot of jumping around the place, and it uses LeftAndMain which has loads of stuff on it. If we do the "centralise FormSchema/API endpoints" then we could come up with something more intuitive to generate the FormSchema.
  • Note GraphQL is only used for read operations to render the list of elements and do write operations to update the sort order on drag and drop sort. GraphQL is NOT used to render the inline edit form or submit form data

POSTing elemental data

  • This is the bit that is currently the furthest away from validation on inline save.
  • When the inline save button is clicked, and standalone XHR request is made from the inline save button. The POST body is a JSON representation of the form field names as keys and corresponding values, same as what you'd expect for a REST request.
  • Creating a standalone XHR request is wrong since it's not tied to the form that was created using the FormSchema. There's no way to get validation errors put back on to the form fields without doing jquery hacks.
  • What should happen instead is that the form is submitted and a FormSchema with the errors node populated is returned. The FormSchema is then used FormBuilder.js to rebuild the form. This should then show inline validation errors, presuming that there is existing logic in FormBuilder.js to actually do this.
  • When submitting the form it'll send form urlencoded data rather than wrapping it in JSON. This is fine IMO since it's not REST and ther'es no need to json_decode() on the server, you simply read $request->postVars().
  • I got the server to return the FormSchema with the error node populated, though I wasn't able to get it to replace existing form. Should be able to get this working with some addtional effort. Was getting stuck in FormAlert.js (toast notification?) where the error node object was being passed instead of a string error message.
  • Note the existing LeftAndMain::jsonError() only supports a single validation error, I had to override it on ElementalAreaController::jsonError() to support multiple validation errors() - https://github.com/silverstripe/silverstripe-elemental/pull/1113/files#diff-942115e4b8f6030e4d72ebc2b3a0772ec65e5dfcd08fbd0e677c70d1231daf28R248
  • I've updated FormBuildController::apiSaveForm() to return a FormSchema response and to call both Form::validate() and Element::validate() which is used to populate the error node of the FormSchema response - https://github.com/silverstripe/silverstripe-elemental/pull/1113/files#diff-942115e4b8f6030e4d72ebc2b3a0772ec65e5dfcd08fbd0e677c70d1231daf28R122

Frontend validation as you type

GuySartorelli commented

Frontend validation as you type

I think that's a bit overkill - we don't provide that anywhere else in the CMS. For now lets just focus on getting validation working at all, and then we can look at our validation across the board and decide if and where and how we want to apply validation as you type.

GuySartorelli commented

@emteknetnz There's currently two ways to save inline elemental forms - through the save button on the block itself, and by saving the whole page.
It's a bit hard to tell in your comment whether you've accounted for both or just one of these. Can you please clarify that for me?

emteknetnz commented

Only the inline save, I've paid no attention to the page save. I'm kind of making an assumption that later on we'd want to drop the page save method altogether, and only save inline blocks via their own XHR requests that would be triggered when clicking the page save button. I'd like to not have 2 ways to do the same thing, it adds a massive amount of overhead effort to maintain, just ends up wasting time.

Frontend validation as you type ... I think that's a bit overkill

FormBuilder.js supports this right now and we should be making use of it if possible IMO. It will block submitting the redux-form which is really nice.

emteknetnz commented

There's probably very little to peer review here, which makes sense since it's a SPIKE

The conclusion I came to was basically "There is this existing FormSchema + FormBuilder functionality that we should be using because it supports validation, so let's use it"

GuySartorelli commented

Only the inline save, I've paid no attention to the page save. I'm kind of making an assumption that later on we'd want to drop the page save method altogether, and only save inline blocks via their own XHR requests that would be triggered when clicking the page save button. I'd like to not have 2 ways to do the same thing, it adds a massive amount of overhead effort to maintain, just ends up wasting time.

Fair enough. I'd say we want the validation experience to be the same regardless of which button people are clicking to save their data, so we should do both at once IMO. Specifically:

I'm kind of making an assumption that later on we'd want to drop the page save method altogether, and only save inline blocks via their own XHR requests that would be triggered when clicking the page save button

That sounds great, and IMO we should do that right away if that more easily lends itself to validating in a consistent way. At the end of the day the content authors don't care whether it's a separate XHR request triggered when clicking the save button, or if the data is all shoved through together with the page data. They just care that when they click the button, they have validation errors if there are validation errors and stuff is saved otherwise.

FormBuilder.js supports this right now and we should be making use of it if possible IMO. It will block submitting the redux-form which is really nice.

I'm not saying "lets never do that" - I just think that there's two concerns here. One is getting validation working at a base level, consistent with the rest of the CMS. We can then look at as-you-type validation as a separate concern IMO and try to get that implemented in a consistent way across the board for a clear and consistent validation experience. It would be a weird CMS Author experience if sometimes stuff gets validated as you type, but sometimes it doesn't. Authors shouldn't have to care that this comes from this module so it does things this way, and that comes from that module so it does things that way.

The conclusion I came to was basically "There is this existing FormSchema + FormBuilder functionality that we should be using because it supports validation, so let's use it"

That sounds sensible to me.
I think we're at the stage where we get into a room, make sure we're all on the same page, and create some clear acceptance criteria.

emteknetnz commented

That sounds great, and IMO we should do that right away if that more easily lends itself to validating in a consistent way.

Yeah if we want a consistent UX experience I think it's borderline a requirement to get rid of the "save element content in the same request as saving the page content" functionality, which is actually pretty similar to what linkfield does now.

I think we're at the stage where we get into a room

I suspect this is going to be significantly easier to get going on linkfield first because it's simpler and we've already agreed to a new major version making it easier to change things, so I'd be inclined to see how that goes first before tackling this one. We'd be able to have a more informed discussion if we've already got that under our belt.

  • 👍1
blueo commented

just want to throw my 2c in here re the saving mechanic. I'm all for a single way to do things but there's a common pattern I've seen on projects to disable the publish on individual blocks and instead have only a page level publish. This is because content authors usually do not want a partial update to the page (or at least its an easier mental model). If we're saving blocks individually it would be good to make sure it is compatible with preventing a page publish (or save for that matter) when a validation error occurs. On the face of it this appears to require some 'waiting' for blocks to save before allowing a page save.

emteknetnz commented

This would only be for saving blocks, not for publishing blocks

In my head, on page save click all the elements would, if in a changed state, fire off their own XHR's to validate/save and the page save button would wait for all of those to resolve first. If there's any validation errors then there this would prevent the page form from submitting and all of the relevant blocks with validation errors would pop open and show the validation errors on the relevant fields. Ditto clicking the page publish button, though again it would inline save blocks, not inline publish blocks.

For actually publishing the blocks there's no need to XHR everything because we just rely on ElementalPageExtension.$owns = ['ElementalArea'] and ElementalArea.$owns = ['Elements'] to cascade publish the blocks when the page is published

  • 👍2
maxime-rainville commented
  • In ElementalAreaController::getElementForm() .. $form = (EditFormFactory extends DefaultFormFactory)::getForm(). EditFormFactory does the elemental namespacing which prefixing Elements with "PageElements__" because you can edit multiple elements in an ElementalArea at a time.
  • This works, though it's pretty confusing routing because there's a lot of jumping around the place, and it uses LeftAndMain which has loads of stuff on it. If we do the "centralise FormSchema/API endpoints" then we could come up with something more intuitive to generate the FormSchema.

That was a massive pain in the but to getting AnyField working with Elemental block. That definitely give some pretty hackish vibe.

When submitting the form it'll send form urlencoded data rather than wrapping it in JSON. This is fine IMO since it's not REST and ther'es no need to json_decode() on the server, you simply read $request->postVars().

That sounds fine to me. I guess there's a wider question about whether we want form schema to support JSON data submission or facilitate it. But that doesn't need to be answered here.

I got the server to return the FormSchema with the error node populated, though I wasn't able to get it to replace existing form. Should be able to get this working with some addtional effort. Was getting stuck in FormAlert.js (toast notification?) where the error node object was being passed instead of a string error message.

Campaign admin seems to have figured out the "Validation" part of form schema. I don't know if it's a proper fix or a hack thought.

Frontend validation as you type

I think that's a bit overkill - we don't provide that anywhere else in the CMS. For now lets just focus on getting validation working at all, and then we can look at our validation across the board and decide if and where and how we want to apply validation as you type.

I wouldn't block the card on it. But I think it's something worth investigating as part of our wider form schema work.

I'm thinking it might actually be easier to validate the elemental block form as you go rather than the parent form + multiple elemental block all at once.

Saving/Publishing individually or via the page

I would be very concerned if we forced content author to save their block individually without giving them the option to save them all at once through the parent page. I would want us to sit down with a bunch of content authors before making such a big change to their workflow.

Would triggering the individual "block save" when saving the page, before triggering the main page save be an option here?

Disabling individual block publish does sound like something quite sensible however.

maxime-rainville commented

We think we've got enough info to proceed. We're currently doing similar work on LinkField. We'll draw some lesson from that and apply them to elemental validation.