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

NEW Inline save all rendered element forms on parent form submit #1214

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jun 24, 2024

Issue #1215

Requires silverstripe/silverstripe-admin#1792 and silverstripe/silverstripe-behat-extension#277 and silverstripe/silverstripe-frameworktest#193 and silverstripe/silverstripe-framework#11301

What this PR does

  • When clicking the page save/publish button, a promise is created which waits for all dirty elemental forms to be submitted.
  • If any of the inline forms responses fails validation then the promise will resolve(false) and the page form will not submit.
  • If the element forms are valid then the promise will resolve (true) then the page form will submit as usual
  • The promise is checked in LeftAndMain in this PR
  • A lot of logic has has been added to let the parent ElementList component know the current state of its child elements if there are unsaved changes, and the last validation result. This determines when and the value the promise will resolve with.

@emteknetnz emteknetnz force-pushed the pulls/5/always-inline-submit-elements branch from 6a8f776 to 15c9006 Compare July 4, 2024 05:16
@@ -1,52 +0,0 @@
@javascript
Feature: Don't lose content when page or block is invalid
Copy link
Member Author

@emteknetnz emteknetnz Jul 4, 2024

Choose a reason for hiding this comment

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

This whole file was testing validation messages after page save. Since we've removed that functionality entirely this feature file is no longer relevant

@emteknetnz emteknetnz force-pushed the pulls/5/always-inline-submit-elements branch from 15c9006 to fffa9ba Compare July 4, 2024 06:36
@emteknetnz emteknetnz marked this pull request as ready for review July 4, 2024 06:41
@emteknetnz emteknetnz force-pushed the pulls/5/always-inline-submit-elements branch from fffa9ba to 123be88 Compare July 4, 2024 23:48
@GuySartorelli
Copy link
Member

Problems encountered when testing locally:

There's no feedback

  1. Open a block
  2. Make an edit which makes the block invalid
  3. Close the block
  4. Click "Save" on the page

There's nothing at all indicating that something went wrong. No toast, and the block doesn't open. It looks like the page save is just broken but in reality there's a validation error I'm not being told about.

There should definitely be a toast message, and ideally the block should open so that I can immediately see the validation error message without opening all the blocks to try find it.

This is partially #1177 which is meant to be fixed by this PR.

Blocks get double saved - which turns them back to "modified"

  1. Have some valid blocks.
  2. Click "Publish" on the page. The blocks should all be in a published state.
  3. Open one of the blocks. Made no modifications.
  4. Click "Save" on the page

The block is now "Modified" even though I didn't make any changes! Note that the page itself isn't "modified", so this isn't just how the CMS treats a save that happens after a publish.

validate() method validation still gets confused with fields on the page

  1. Implement a validate() method on a block which adds this: $result->addFieldError('Title', 'Nah bro');
  2. Do not open any blocks
  3. Click "Publish" on the page (doesn't happen with save)

The "Nah bro" message appears underneath the page title field. This is #1183 which is meant to be fixed by this PR.

@emteknetnz emteknetnz force-pushed the pulls/5/always-inline-submit-elements branch 3 times, most recently from 183ea5c to f29fcd9 Compare July 11, 2024 06:02
Copy link
Member Author

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Need to set a composer requirement of silverstripe/admin ^2.3

@GuySartorelli
Copy link
Member

Note that I still have the "no feedback" and "Blocks get double saved - which turns them back to "modified"" issues as described above.

Note that the "no feedback" problem only happens with form validation (i.e. getCMSCompositeValidator() - I specifically add a RequiredFields validator for the Title field). Using the validate() method I do get a toast and the block does open as expected.

@emteknetnz
Copy link
Member Author

emteknetnz commented Jul 18, 2024

I've solved the "Blocks get double saved - which turns them back to "modified" issue - change here

For the "no feedback" issue with RequiredFields ... I don't know how to resolve this one without globally removing client-side validation (which admittedly would simplify things, but we probably shouldn't. Though I also won't really mind :-).

The issue is that RequiredFields has client-side (pure js) validation which prevents an XHR request from being sent to the server in the first place. The 'feedback' of popping opening the element happens as part of the XHR response which is formSchema formatted and contains the validation errors.

However with the client-side validation there's nothing "returned" to trigger opening the element. If we did globally remove client-side validation then it'll make an XHR request and fail validation on the server and behave the same UX wise as DataObject->validate() failing

I think the client-side validation is passed in as a callback into redux-form ... somehow (this maybe?). I don't think there's anyway to somehow extract a 'response' from it, particularly as we're remote-submitting the form which does not return a promise

So I guess, options are:
a) Globally disable client-side validation
b) Hack is something kind of nasty that emits an event from Validator.js and we listen for it in Element.js .. somehow ..
c) Just live with the bad UX

@GuySartorelli
Copy link
Member

GuySartorelli commented Jul 18, 2024

Just live with the bad UX

I don't think that's really an option, since RequiredFields is the only validator we offer out-of-the-box so it's presumably used a lot. Not having any feedback at all about why submitting a form did nothing is really bad IMO. If I was a content author and clicking "save" seemed to do literally nothing, I'd be contacting my vendor and reporting a critical bug. If there was either a toast or the block popped open, it wouldn't be so bad.

It's interesting to note that a RequiredFields validator for any field on the page directly doesn't have this same behaviour - in that case a toast is triggered.

Is that front-end validation JS able to trigger a toast? That would resolve the problem well enough for now, I think. Better if the block opens, but at least if there's a toast it prompts the content author to start looking for a validation error message as opposed to just thinking something's not working the way it should be.

@emteknetnz
Copy link
Member Author

emteknetnz commented Jul 18, 2024

I've the validation fixed it by hardcoding something into admin to disable client-side validation for elemental specifically - https://github.com/silverstripe/silverstripe-admin/pull/1792/files#diff-cc40afd32326a7c9ddea47afd5623f5dd54abfe99db517c1f6e72ca43337a4cbR73

It's obviously not the prettiest, though the UX experience is now much better.

Note that I'm pretty sure this change will require updating a behat test that expects client side validation, so rerun tests for this is the admin PR is merged

@emteknetnz emteknetnz force-pushed the pulls/5/always-inline-submit-elements branch 4 times, most recently from 36ec955 to 41060e0 Compare July 22, 2024 23:30
@emteknetnz emteknetnz force-pushed the pulls/5/always-inline-submit-elements branch 2 times, most recently from fa0710b to e2a7828 Compare July 24, 2024 02:16
@emteknetnz emteknetnz force-pushed the pulls/5/always-inline-submit-elements branch from e2a7828 to 823cc53 Compare July 24, 2024 06:02
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 - will merge the other PRs then rerun CI.

@GuySartorelli
Copy link
Member

Behat failing

@emteknetnz
Copy link
Member Author

Needed to tag 5.4.0 of behat-extension, have done that an re-run, working now

@GuySartorelli GuySartorelli merged commit fbbca6a into silverstripe:5 Jul 24, 2024
25 checks passed
@GuySartorelli GuySartorelli deleted the pulls/5/always-inline-submit-elements branch July 24, 2024 22:17
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.

2 participants