FIX Make change tracking work again for inline editable blocks

client/src/components/ElementEditor/Element.js

@@ -10,7 +10,7 @@ import { inject } from 'lib/Injector';
 import i18n from 'i18n';
 import classNames from 'classnames';
 import { connect } from 'react-redux';
-import { submit } from 'redux-form';
+import { submit, isDirty } from 'redux-form';
 import { loadElementFormStateName } from 'state/editor/loadElementFormStateName';
 import { loadElementSchemaValue } from 'state/editor/loadElementSchemaValue';
 import { publishBlockMutation } from 'state/editor/publishBlockMutation';
@@ -20,7 +20,7 @@ import { DragSource, DropTarget } from 'react-dnd';
 import { getEmptyImage } from 'react-dnd-html5-backend';
 import { elementDragSource, isOverTop } from 'lib/dragHelpers';
 import * as toastsActions from 'state/toasts/ToastsActions';
-import { addFormChanged, removeFormChanged } from 'state/unsavedForms/UnsavedFormsActions';
+import getFormState from 'lib/getFormState';
 
 export const ElementContext = createContext(null);
 
@@ -39,32 +39,19 @@ const Element = (props) => {
   const [doPublishElementAfterSave, setDoPublishElementAfterSave] = useState(false);
   const [ensureFormRendered, setEnsureFormRendered] = useState(false);
   const [formHasRendered, setFormHasRendered] = useState(false);
-  const [doDispatchAddFormChanged, setDoDispatchAddFormChanged] = useState(false);
-  const [hasUnsavedChanges, setHasUnsavedChanges] = useState(false);
   const [publishBlock] = useMutation(publishBlockMutation);
 
   const formRenderedIfNeeded = formHasRendered || !props.type.inlineEditable;
 
   useEffect(() => {
-    // Note that formDirty from redux can be set to undefined after failed validation
-    // which is confusing as the block still has unsaved changes, hence why we create
-    // this state variable to track this instead
-    // props.formDirty is either undefined (when pristine) or an object (when dirty)
-    const formDirty = typeof props.formDirty !== 'undefined';
-    if (formDirty && !hasUnsavedChanges) {
-      setHasUnsavedChanges(true);
-    }
+    props.onChangeHasUnsavedChanges(props.formDirty);
   }, [props.formDirty]);
 
   useEffect(() => {
-    props.onChangeHasUnsavedChanges(hasUnsavedChanges);
-  }, [hasUnsavedChanges]);
-
-  useEffect(() => {
-    if (props.saveElement && hasUnsavedChanges && !doSaveElement) {
+    if (props.saveElement && props.formDirty && !doSaveElement) {
       setDoSaveElement(true);
     }
-  }, [props.saveElement, hasUnsavedChanges, props.increment]);
+  }, [props.saveElement, props.formDirty, props.increment]);
 
   useEffect(() => {
     if (props.connectDragPreview) {
@@ -81,7 +68,7 @@ const Element = (props) => {
   useEffect(() => {
     if (justClickedPublishButton && formRenderedIfNeeded) {
       setJustClickedPublishButton(false);
-      if (hasUnsavedChanges) {
+      if (props.formDirty) {
         // Save the element first before publishing, which may trigger validation errors
         props.submitForm();
         setDoPublishElementAfterSave(true);
@@ -92,13 +79,6 @@ const Element = (props) => {
     }
   }, [justClickedPublishButton, formHasRendered]);
 
-  useEffect(() => {
-    if (doDispatchAddFormChanged) {
-      setDoDispatchAddFormChanged(false);
-      props.dispatchAddFormChanged();
-    }
-  }, [doDispatchAddFormChanged]);
-
   const getNoTitle = () => i18n.inject(
     i18n._t('ElementHeader.NOTITLE', 'Untitled {type} block'),
     { type: props.type.title }
@@ -143,18 +123,7 @@ const Element = (props) => {
     showPublishedElementToast(wasError);
     setDoPublishElement(false);
     setDoPublishElementAfterSave(false);
-    // Ensure that formDirty becomes falsey after publishing
-    // We need to call at a later render rather than straight away or redux-form may override this
-    // and set the form state to dirty under certain conditions
-    // setTimeout is a hackish way to do this, though I'm not sure how else we can do this
-    // The core issue is that redux-form will detect changes when a form is hydrated for the first
-    // 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
-    Promise.all(refetchElementalArea())
-      .then(() => {
-        setTimeout(() => props.dispatchRemoveFormChanged(), 250);
-      });
+    refetchElementalArea();
   };
 
   // Save action
@@ -337,10 +306,6 @@ const Element = (props) => {
       if (props.type.inlineEditable) {
         setPreviewExpanded(true);
       }
-      // Ensure that formDirty remains truthy
-      // Note we need to call props.dispatchAddFormChanged() on the next render rather than straight away
-      // or it will get unset by code somewhere else, probably redux-form
-      setDoDispatchAddFormChanged(true);
       // Don't accidentally auto publish the element once validation errors are fixed
       if (doPublishElementAfterSave) {
         setDoPublishElementAfterSave(false);
@@ -349,7 +314,6 @@ const Element = (props) => {
       return;
     }
     // Form is valid
-    setHasUnsavedChanges(false);
     setNewTitle(title);
     if (doPublishElementAfterSave) {
       setDoPublishElementAfterSave(false);
@@ -435,6 +399,7 @@ const Element = (props) => {
         broken={type.broken}
         onFormSchemaSubmitResponse={handleFormSchemaSubmitResponse}
         onFormInit={() => handleFormInit(activeTab)}
+        formDirty={formDirty}
       />
     </ElementContext.Provider>
   </div>);
@@ -462,7 +427,9 @@ function mapStateToProps(state, ownProps) {
 
   const tabSetName = tabSet && tabSet.id;
   const uniqueFieldId = `element.${elementName}__${tabSetName}`;
-  const formDirty = state.unsavedForms.find((unsaved) => unsaved.name === `element.${elementName}`);
+
+  const formName = loadElementFormStateName(ownProps.element.id);
+  const formDirty = isDirty(`element.${formName}`, getFormState)(state);
 
   // Find name of the active tab in the tab set
   // Only defined once an element form is expanded for the first time
@@ -490,16 +457,6 @@ function mapDispatchToProps(dispatch, ownProps) {
       // Perform a redux-form remote-submit
       dispatch(submit(`element.${elementName}`));
     },
-    dispatchAddFormChanged() {
-      // Ensures the form identifier is in unsavedForms in the global redux state
-      // This is used to derive the formDirty prop in mapStateToProps
-      dispatch(addFormChanged(`element.${elementName}`));
-    },
-    dispatchRemoveFormChanged() {
-      // Removes the form identifier from unsavedForms in the global redux store
-      // Opposite of beheaviour of dispatchAddFormChanged()
-      dispatch(removeFormChanged(`element.${elementName}`));
-    },
     actions: {
       toasts: bindActionCreators(toastsActions, dispatch),
     },

client/src/components/ElementEditor/ElementList.js

@@ -39,6 +39,13 @@ class ElementList extends Component {
       this.resetState(prevState, false);
       return;
     }
+    // Scenario we've just clicked "save" or "submit" on the form
+    if (this.state.saveAllElements && !prevState.saveAllElements) {
+      // Reset the validation state of all blocks.
+      // This mirrors handleBeforeSubmitForm() for individual blocks.
+      this.resetState(prevState, false);
+      return;
+    }
     // Scenario Saving all elements and state has just updated because of a formSchema response from
     // an inline save - see Element.js handleFormSchemaSubmitResponse()
     if (this.state.saveAllElements) {
@@ -154,9 +161,7 @@ class ElementList extends Component {
     }
 
     let output = blocks.map(element => {
-      const saveElement = this.state.saveAllElements
-        && this.state.hasUnsavedChangesBlockIDs[element.id]
-        && this.state.validBlockIDs[element.id] === null;
+      const saveElement = this.state.saveAllElements && this.state.hasUnsavedChangesBlockIDs[element.id];
       return <div key={element.id}>
         <ElementComponent
           element={element}

src/Models/ElementContent.php

@@ -50,4 +50,13 @@ public function getType()
     {
         return _t(__CLASS__ . '.BlockType', 'Content');
     }
+
+    public function validate()
+    {
+        $result = parent::validate();
+        if ($this->Title === 'zyx') {
+            $result->addFieldError('Title', 'Title must not be "zyx"');
+        }
+        return $result;
+    }
 }

tests/Behat/features/change-tracking.feature

@@ -0,0 +1,41 @@
+@retry @job5
+Feature: Unsaved changes are identified by changetracker
+  As a CMS user
+  I want to know when I have unsaved changes
+
+  Background:
+    Given I add an extension "DNADesign\Elemental\Extensions\ElementalPageExtension" to the "Page" class
+    And a "page" "Blocks Page" with a "My title" content element with "<p>My content</p>" content
+    And the "group" "EDITOR" has permissions "Access to 'Pages' section"
+    And I am logged in as a member of "EDITOR" group
+    And I go to "/admin/pages"
+    And I follow "Blocks Page"
+
+  Scenario: Change tracking for text and HTML fields
+    # Note we can't just check for "Save" vs "Saved" because one is a subset of the other,
+    # so `I should not see a "Save" button` will always fail when the "Saved" button is present.
+    # Instead, font-icon-tick is a CSS class on the "Saved" button, and font-icon-save is on the "Save" button
+    Then I should see the "button.font-icon-tick" element
+    Then I should not see the "button.font-icon-save" element
+    # Just opening the block doesn't get seen as a "change"
+    When I click on the caret button for block 1
+    Then I should see the "button.font-icon-tick" element
+    Then I should not see the "button.font-icon-save" element
+    # Update the title
+    When I fill in "changed" for "Title" for block 1
+    Then I should not see the "button.font-icon-tick" element
+    Then I should see the "button.font-icon-save" element
+    # Change it back
+    When I fill in "My title" for "Title" for block 1
+    Then I should see the "button.font-icon-tick" element
+    Then I should not see the "button.font-icon-save" element
+    # Update the HTML content
+    When I fill in "<p>New sample content</p>" for "Content" for block 1
+    Then I should not see the "button.font-icon-tick" element
+    Then I should see the "button.font-icon-save" element
+    # Change it back
+    When I fill in "<p>My content</p>" for "Content" for block 1
+    Then I should see the "button.font-icon-tick" element
+    Then I should not see the "button.font-icon-save" element
+    # Don't click save at the end - there should be no alert because there were no changes
+

tests/Behat/features/file-upload.feature

@@ -8,12 +8,54 @@ Feature: Files can be saved in and removed from elemental blocks
     And I add an extension "SilverStripe\FrameworkTest\Elemental\Extension\FileElementalExtension" to the "DNADesign\Elemental\Models\ElementContent" class
     And I go to "/dev/build?flush"
     And a "image" "file1.jpg"
+    And a "image" "file2.jpg"
     And a "page" "Blocks Page" with a "My title" content element with "My content" content
     And the "group" "EDITOR" has permissions "Access to 'Pages' section"
     And I am logged in as a member of "EDITOR" group
     And I go to "/admin/pages"
     And I follow "Blocks Page"
 
+  Scenario: Add a file and save the block, then remove it and add a different one
+    # Add a file to the block
+    Given I click on the caret button for block 1
+    Then I should not see "file1"
+    When I click "Choose existing" in the "#Form_ElementForm_1 .uploadfield" element
+    And I press the "Back" HTML field button
+    And I click on the file named "file1" in the gallery
+    And I press the "Insert" button
+    And I press the "View actions" button
+    And I click on the ".element-editor__actions-save" element
+    Then I should see a "Saved 'My title' successfully" success toast
+    # Check we see the file both in the current page load (react state is correct) and after reloading the form
+    Then I should see "file1"
+    When I go to "/admin/pages"
+    And I follow "Blocks Page"
+    And I click on the caret button for block 1
+    Then I should see "file1"
+    # Then remove the file from the block
+    And I click on the "#Form_ElementForm_1 .uploadfield-item__remove-btn" element
+    Then I should not see "file1"
+    # Try adding the same file back
+    When I click "Choose existing" in the "#Form_ElementForm_1 .uploadfield" element
+    And I press the "Back" HTML field button
+    And I click on the file named "file1" in the gallery
+    And I press the "Insert" button
+    And I press the "View actions" button
+    # same file, so we shouldn't see the button
+    Then I should not see the save button for block 1
+    # Add a different file
+    And I click on the "#Form_ElementForm_1 .uploadfield-item__remove-btn" element
+    When I click "Choose existing" in the "#Form_ElementForm_1 .uploadfield" element
+    # Note we don't have to press "Back" here because react knows what folder we were just in before
+    And I click on the file named "file2" in the gallery
+    And I press the "Insert" button
+    And I press the "View actions" button
+    Then I should see the save button for block 1
+    And I click on the ".element-editor__actions-save" element
+    Then I should see a "Saved 'My title' successfully" success toast
+    And I should see "file2"
+    And I should not see "file1"
+
   Scenario: Add a file and save the block, then remove the file and save the block
     # Add a file to the block
     Given I click on the caret button for block 1
@@ -33,6 +75,7 @@ Feature: Files can be saved in and removed from elemental blocks
     Then I should see "file1"
     # Then remove the file from the block
     And I click on the "#Form_ElementForm_1 .uploadfield-item__remove-btn" element
+    Then I should not see "file1"
     And I press the "View actions" button
     And I click on the ".element-editor__actions-save" element
     Then I should see a "Saved 'My title' successfully" success toast