Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

FIX Make change tracking work again for inline editable blocks #1319

Merged
merged 1 commit into from
Feb 27, 2025
Merged

FIX Make change tracking work again for inline editable blocks #1319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions client/dist/js/bundle.js
View file
Open in desktop

Large diffs are not rendered by default.

65 changes: 11 additions & 54 deletions client/src/components/ElementEditor/Element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);

Expand All @@ -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);
Comment on lines -42 to -43
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to separately keep track of hasUnsavedChanges because we can just rely on formDirty instead.
No need to use doDispatchAddFormChanged because redux form keeps track of that for us automatically.

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);
}
Comment on lines -49 to -56
Copy link
Member Author

Choose a reason for hiding this comment

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

That's not how formDirty works now - it's just a boolean same as it used to be in 5.2.

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) {
Expand All @@ -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);
Expand All @@ -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 }
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -349,7 +314,6 @@ const Element = (props) => {
return;
}
// Form is valid
setHasUnsavedChanges(false);
setNewTitle(title);
if (doPublishElementAfterSave) {
setDoPublishElementAfterSave(false);
Expand Down Expand Up @@ -435,6 +399,7 @@ const Element = (props) => {
broken={type.broken}
onFormSchemaSubmitResponse={handleFormSchemaSubmitResponse}
onFormInit={() => handleFormInit(activeTab)}
formDirty={formDirty}
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 assume this was originally removed with the intention of using the new context in the Content component - but that's a class component so it can't use hooks. Using the prop that already exists is easier and more suitable as a patch compared with completely refactoring that component to a functional one.

/>
</ElementContext.Provider>
</div>);
Expand Down Expand Up @@ -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);
Comment on lines -465 to +432
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just swapping us back to how this was in CMS 5.2. This was changed in 5.3 but it's unclear why.
Dirty state tracking worked in 5.2 so I've just reverted this back to how it was then.


// Find name of the active tab in the tab set
// Only defined once an element form is expanded for the first time
Expand Down Expand Up @@ -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}`));
},
Comment on lines -493 to -502
Copy link
Member Author

Choose a reason for hiding this comment

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

Just rely on the existing formdirty data from redux.

actions: {
toasts: bindActionCreators(toastsActions, dispatch),
},
Expand Down
11 changes: 8 additions & 3 deletions client/src/components/ElementEditor/ElementList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment on lines +42 to +48
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed because otherwise we retain the old "invalid" state. As noted in the code comment, resetting the validation state is already done when saving a block with the ... menu, so makes sense to reset it when submitting via the page save button as well.

// 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) {
Expand Down Expand Up @@ -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];
Comment on lines -157 to +164
Copy link
Member Author

Choose a reason for hiding this comment

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

We should attempt to save the block regardless of the validation state it was in prior to trying to save the block.

return <div key={element.id}>
<ElementComponent
element={element}
Expand Down
47 changes: 47 additions & 0 deletions tests/Behat/features/change-tracking.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
@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
# For some reason, using the usual 'When I fill in "changed" for "Title" for block 1'
# it fails to recognise the change in CI even though it works locally for some people.
When I focus on the "#Form_ElementForm_1_Title" element
And I type "changed" in the field
# 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 "" for "Title" for block 1
When I focus on the "#Form_ElementForm_1_Title" element
And I type "My title" in the field
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

43 changes: 43 additions & 0 deletions tests/Behat/features/file-upload.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member Author

Choose a reason for hiding this comment

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

If we add a file, save, remove the file and try to add the same file back, since it's the same file the form isn't dirty so we can't save.

The dirty state didn't change before, but now that it does we need to test this.
Anything with a file2 is for testing that scenario.

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
Expand All @@ -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
Expand Down
Loading