fix: ensure draft changes #3306

barthc · 2020-02-23T14:32:28Z

Now that we are using the diff API to pull workflow changes, it makes sense to make sure that they are actual draft changes before saving by comparing the current entry draft with unpublished or published Entry.

The changes also take care of the relation widget unsaved changes bug #725.

erezrokah · 2020-02-25T18:35:26Z

I like the approach in this PR, will try to finalize the review soon.

erezrokah · 2020-02-25T18:52:19Z

Realized this is similar to #3305, will go over both of them

erezrokah · 2020-02-26T08:23:10Z

I'm still able to reproduce the bug on https://bitbucket.org/erezro/netlify-cms-reproductions/branch/netlify_cms/issue_3256:

PR: https://bitbucket.org/erezro/netlify-cms-reproductions/pull-requests/5/update-post-test/diff

erezrokah · 2020-02-26T08:45:00Z

Seems to be related to jonschlinkert/gray-matter#96.
A new line is always added at the end of the file here:
https://github.com/netlify/netlify-cms/blob/53365b73701c872948a1e65da00a655e0718e8cb/packages/netlify-cms-core/src/formats/frontmatter.js#L87

barthc · 2020-02-26T09:03:42Z

I think it's the markdown widget, the rich text mode is inserting a tab at the beginning of the string on edit. But the markdown mode is good. Is it a bug ?

erezrokah · 2020-02-26T09:37:27Z

Can you share a way to get that tab inserted? I couldn't reproduce it.

I was able to fix my issue by removing the extra line break inserted by gray-matter.
See here 411d986.

This might have some non-intuitive results as our markdown widget seems to strip line breaks at the end. Notice in the following GIF, it won't report a change until a non line break character is inserted:

Looking into that now

barthc · 2020-02-26T09:48:23Z

Here is the markdown mode:

erezrokah · 2020-02-26T09:56:27Z

Markdown mode doesn't process anything so I think most of the issues won't come from there.
The rich text editor parses the raw markdown into an abstract tree and then reports it back as raw markdown. In that process it does some schema validations and other transformations that caused a few 'Unsaved Changes' issues in the past.

bharrisau · 2020-02-26T10:20:28Z

packages/netlify-cms-core/src/reducers/entryDraft.js

+      let newState = state.withMutations(state => {
        state.setIn(['entry', 'data', action.payload.field], action.payload.value);
        state.mergeDeepIn(['fieldsMetaData'], fromJS(action.payload.metadata));
        state.set('hasChanged', true);
      });
+      const newStateData = newState.getIn(['entry', 'data']);
+
+      (newStateData.equals(unpublishedEntry.get('data')) ||
+        newStateData.equals(publishedEntry.get('data'))) &&
+        (newState = newState.set('hasChanged', false));

+      return newState;


Can this be done with only a single Immutable node?

return state.withMutations(state => { state.setIn(['entry', 'data', action.payload.field], action.payload.value); state.mergeDeepIn(['fieldsMetaData'], fromJS(action.payload.metadata)); const newData = state.getIn(['entry', 'data']); const matchesLastSaved = newData.equals(unpublishedEntry.get('data')) || newData.equals(publishedEntry.get('data')) state.set('hasChanged', !matchesLastSaved); });

I changed that in my branch 411d986#diff-9409717c5b67d95c17fad5f0b8165dd8R106

bharrisau · 2020-02-26T10:25:49Z

Couple of comments/questions

Is .equals going to give the expected equality when matching maps? Immutable provides .is that gives good deep matching, but I'm only a flythrough contributor so I'm not clear enough on the codebase to know if .equals will do the same.
Is it worth adding a test spec for this behavior?
Is it possible to consolidate unpublishedEntry and publishedEntry into just the one lastSavedEntry or something? I'm not clear on why there are two possible states that can set hasChanged to false. As a user, there would only be one last saved version of the document right? so only one state that would remove the save button?

bharrisau · 2020-02-26T10:27:19Z

I prefer this approach over the simple mod I proposed in #3305 - this looks like if you change something and then change it back it would recognize that the data matches the previous version.

barthc · 2020-02-26T10:32:38Z

Is it possible to consolidate unpublishedEntry and publishedEntry into just the one lastSavedEntry or something? I'm not clear on why there are two possible states that can set hasChanged to false. As a user, there would only be one last saved version of the document right? so only one state that would remove the save button?

It has to compare both entries, for example, if a user makes workflow changes and on subquest edits revert the workflow changes back to the published entry, we will still have the diff issue,

bharrisau · 2020-02-26T10:37:52Z

Is it possible to consolidate unpublishedEntry and publishedEntry into just the one lastSavedEntry or something? I'm not clear on why there are two possible states that can set hasChanged to false. As a user, there would only be one last saved version of the document right? so only one state that would remove the save button?

It has to compare both entries, for example, if a user makes workflow changes and on subquest edits revert the workflow changes back to the published entry, we will still have the diff issue,

Might be a silly question (again, not familiar with the codebase) - but can picking between those two be the responsibility of the Editor.js when it is calling changeDraftField?

Alternatively, can you just pass through an array of entries that should be compared against? [unpublishedEntry, publishedEntry].filter(remove nulls)
Then the reducer API wouldn't need to be changed in the future if there were more states to compare against?

Probably bikeshedding now - but I see APIs where multiple fields are treated identically and I start asking if there needs to be multiple fields.

erezrokah · 2020-02-26T10:37:55Z

our markdown widget seems to strip line breaks

This happens here. Tried removing it, but created other issues when parsing loading the saved markdown.

barthc · 2020-02-26T10:43:32Z

Alternatively, can you just pass through an array of entries that should be compared against? [unpublishedEntry, publishedEntry].filter(remove nulls)
Then the reducer API wouldn't need to be changed in the future if there were more states to compare against?

~~So the possible values for unpublishedEntry, publishedEntry at any time could be empty Map() for both, one empty and the other with some value, and then both have values.~~

I see your point, yes we can do that.

barthc · 2020-02-26T10:45:45Z

Is .equals going to give the expected equality when matching maps? Immutable provides .is that gives good deep matching, but I'm only a flythrough contributor so I'm not clear enough on the codebase to know if .equals will do the same.

Looks like equals is calling is under the hood https://immutable-js.github.io/immutable-js/docs/#/Collection/equals

bharrisau · 2020-02-26T11:00:49Z

Looks like equals is calling is under the hood https://immutable-js.github.io/immutable-js/docs/#/Collection/equals

No worries. And don't feel like you need to change anything above (unpublished/published) - I just make comments about possible alternatives in reviews because that's how I like others to review my stuff.

barthc · 2020-02-26T15:49:11Z

@erezrokah any luck with the markdown ?

erezrokah · 2020-02-26T15:57:03Z

Not yet and we might want to handle some of those issues in another PR.
We should at least make sure gray-matter doesn't add that line break as at the moment it seems we will always add it on save, but never report it in the value of the change event.
I'll keep looking

erezrokah · 2020-02-27T15:37:30Z

packages/netlify-cms-core/src/formats/frontmatter.js

@@ -72,7 +72,7 @@ class FrontmatterFormatter {
    const result = matter(content, { engines: parsers, ...format });
    return {
      ...result.data,
-      body: result.content,
+      ...(result.content.trim() && { body: result.content }),


@barthc do you mind sharing the reason this is required?

It's the same line break issue at the end of the file. So when the file is parsed, it includes a body key with line break as value, this would occur for config fields without the top-level markdown body widget that resolves to the body of the markdown.

Thanks I think I get it now.
We pass in an empty body here:
https://github.com/netlify/netlify-cms/blob/3e873f3e0277bca8f5dee619e931a66d47779c3d/packages/netlify-cms-core/src/formats/frontmatter.js#L80

when it doesn't exist, so we have to remove that property on parse.

erezrokah · 2020-02-27T17:47:38Z

@barthc are you good with me adding the other line break fix (411d986#diff-ac1a295de180688364c35e08b03a343fR89)?

We can fix the behaviour in #3306 (comment) in another PR.
#3256 and #725 seem more important to have fixed.

WDYT?

barthc · 2020-02-27T17:53:09Z

Yes please do, thanks.

erezrokah · 2020-02-27T18:29:08Z

looking into the tests failures

barthc requested a review from a team February 23, 2020 14:32

erezrokah self-requested a review February 24, 2020 11:13

erezrokah mentioned this pull request Feb 25, 2020

fix!: ignore metadata when setting entry unsaved flag #3305

Closed

bharrisau reviewed Feb 26, 2020

View reviewed changes

erezrokah reviewed Feb 27, 2020

View reviewed changes

barthc and others added 4 commits February 27, 2020 19:29

fix: ensure draft changes

8c4e253

fix: feedback changes

6079168

fix: strip line break added by gray matter

0f4aefb

test(e2e): fix recorded test data

f5f9c35

erezrokah force-pushed the fix/ensure-draft-changes branch from 8c6a78e to f5f9c35 Compare February 28, 2020 09:59

test(e2e): fix recorded test data

f74ee03

erezrokah merged commit cf81f58 into master Feb 28, 2020

erezrokah deleted the fix/ensure-draft-changes branch February 28, 2020 10:40

barthc mentioned this pull request Feb 28, 2020

New entries with a Date field show changed status #725

Open

erezrokah mentioned this pull request Mar 12, 2020

fix(backend-github): don't create new commits on empty diff when rebasing #3411

Merged

barthc mentioned this pull request Mar 13, 2020

fix: call createDraftFromEntry after entry is loaded instead in Editor #3418

Merged

vladdu pushed a commit to vladdu/netlify-cms that referenced this pull request Jan 26, 2021

fix: ensure draft changes (decaporg#3306)

e7e04bb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure draft changes #3306

fix: ensure draft changes #3306

barthc commented Feb 23, 2020

erezrokah commented Feb 25, 2020

erezrokah commented Feb 25, 2020

erezrokah commented Feb 26, 2020

erezrokah commented Feb 26, 2020

barthc commented Feb 26, 2020 •

edited

Loading

erezrokah commented Feb 26, 2020 •

edited

Loading

barthc commented Feb 26, 2020

erezrokah commented Feb 26, 2020 •

edited

Loading

bharrisau Feb 26, 2020

erezrokah Feb 26, 2020

bharrisau commented Feb 26, 2020

bharrisau commented Feb 26, 2020

barthc commented Feb 26, 2020

bharrisau commented Feb 26, 2020

erezrokah commented Feb 26, 2020 •

edited

Loading

barthc commented Feb 26, 2020 •

edited

Loading

barthc commented Feb 26, 2020

bharrisau commented Feb 26, 2020

barthc commented Feb 26, 2020

erezrokah commented Feb 26, 2020 •

edited

Loading

erezrokah Feb 27, 2020

barthc Feb 27, 2020

erezrokah Feb 27, 2020

erezrokah commented Feb 27, 2020

barthc commented Feb 27, 2020

erezrokah commented Feb 27, 2020

fix: ensure draft changes #3306

fix: ensure draft changes #3306

Conversation

barthc commented Feb 23, 2020

erezrokah commented Feb 25, 2020

erezrokah commented Feb 25, 2020

erezrokah commented Feb 26, 2020

erezrokah commented Feb 26, 2020

barthc commented Feb 26, 2020 • edited Loading

erezrokah commented Feb 26, 2020 • edited Loading

barthc commented Feb 26, 2020

erezrokah commented Feb 26, 2020 • edited Loading

bharrisau Feb 26, 2020

Choose a reason for hiding this comment

erezrokah Feb 26, 2020

Choose a reason for hiding this comment

bharrisau commented Feb 26, 2020

bharrisau commented Feb 26, 2020

barthc commented Feb 26, 2020

bharrisau commented Feb 26, 2020

erezrokah commented Feb 26, 2020 • edited Loading

barthc commented Feb 26, 2020 • edited Loading

barthc commented Feb 26, 2020

bharrisau commented Feb 26, 2020

barthc commented Feb 26, 2020

erezrokah commented Feb 26, 2020 • edited Loading

erezrokah Feb 27, 2020

Choose a reason for hiding this comment

barthc Feb 27, 2020

Choose a reason for hiding this comment

erezrokah Feb 27, 2020

Choose a reason for hiding this comment

erezrokah commented Feb 27, 2020

barthc commented Feb 27, 2020

erezrokah commented Feb 27, 2020

barthc commented Feb 26, 2020 •

edited

Loading

erezrokah commented Feb 26, 2020 •

edited

Loading

erezrokah commented Feb 26, 2020 •

edited

Loading

erezrokah commented Feb 26, 2020 •

edited

Loading

barthc commented Feb 26, 2020 •

edited

Loading

erezrokah commented Feb 26, 2020 •

edited

Loading