Post Editor: when saving post, overwrite the local edits with post version from server #26247

jsnajdr · 2018-07-23T08:55:08Z

This PR seeks to fix the dirty-after-save issues reported in #9833, namely the following incarnations:

element style with space by @jblz in Editor: prompts to restore/discard changes when there are no changes #9833 (comment)
some cases (but not all) of [code] escaping by @alisterscott in Editor: prompts to restore/discard changes when there are no changes #9833 (comment) and Editor: prompts to restore/discard changes when there are no changes #9833 (comment)
This doesn't fix any of the Markdown issues mentioned by @lancewillett in Editor: prompts to restore/discard changes when there are no changes #9833 (comment) -- I can't reproduce the Markdown issue at all -- I never see the AYS dialog. Maybe someone can provide a reliable STR?

What changed is the algorithm how post edits are stored and discarded on save.

Before this patch:
Save with server-transformed content, without race condition:

Typing content, edits in state.posts.edits become { content: 'Hello' }
Saving content sends a POST save request with { content: 'Hello' }
Server returns transformed content { content '<p>Hello</p>' }
We merge the server changes into state.posts.edits and determine that content wasn't saved yet. state.posts.edits is still { content: 'Hello' } and the post is dirty. This is BAD!

Save without server-transformed content, but with race condition:

Typing content, edits in state.posts.edits become { content: 'Hello' }
Saving content sends a POST save request with { content: 'Hello' }
While saving, we continue typing and state.posts.edits become { content: 'Hello world' }
Server returns untransformed content { content 'Hello' }
We merge the server changes into state.posts.edits and determine that content wasn't saved yet. state.posts.edits is still { content: 'Hello world' } and the post is dirty. This is GOOD!

The same algorithm provides a bad result for transformed content, but a good result for the race condition.

This patch changes the edit algorithm to cover both cases correctly.

After the patch:
Save with server-transformed content, without race condition:

Typing content, edits in state.posts.edits are a log (i.e., array) rather that a single edits object: [ { content: 'Hello' } ]
Just before saving the post, we append a "save marker" string to the edits log: [ { content: 'Hello' }, 'save-marker-1' ]
Saving content sends a POST save request with { content: 'Hello' } and the server returns transformed content { content '<p>Hello</p>' }
On save success, we truncate the state.posts.edits by removing all edits before the save marker. The state.posts.edits log therefore becomes empty. The post is no longer dirty. That's GOOD

Save without server-transformed content, but with race condition:

Typing content, edits log in state.posts.edits becomes [ { content: 'Hello' } ]
Saving content appends a save marker to the edits log and sends a POST save request with { content: 'Hello' }
While saving, we continue typing and state.posts.edits becomes [ { content: 'Hello', 'save-marker-2', { content: 'Hello world' } ]
Server returns untransformed content { content 'Hello' }, we update the post in state.posts and truncate the edits log to [ { content: 'Hello world' } ]
The edits log still has the unsaved edit (it was not lost) and the post is dirty. This is GOOD!

The key changes to the code are:

mergePostEdits function now takes arbitrary number of arguments and we pass it the spread edits log. It will merge all edits together, skipping save markers.
function appendToPostEditsLog appends a new edit to the log, merging the last two edits if there is no marker at the end. This prevents the log from getting too long.
there is a new EDITOR_SAVE action that inserts the save marker
the POST_SAVE_SUCCESS action will remove the marker
all code that works with state.posts.edits is updated to work with an array of edits instead of just one edit.

There's also a very useful new test suite in client/state/ui/editor/test/edit-save-flow.js that tests the whole flow described above. It uses a realistic Redux store, mocks the network requests with nock, dispatches the saveEdited action and checks the results.

`mergePostEdits` now takes a variable number of arguments and merges them all into one object. Also supports the "save marker" concept: string markers in the edits log that are skipped when merging.

The new function appends new edits to an existing log, merging with the last one if it's not a save marker.

It will be used by the `saveEdited` action to insert a save marker to edit log when save is initiated.

…markers Updates the `edits` reducer to work with the new shape of post edits: edits log instead of just one edits object. Also adds support for adding a save marker on `EDITOR_SAVE` and removing it on `POST_SAVE_SUCCESS`.

Test the `saveEdited` flow with the real reducer and mocked network requests. Verifies that the post edits log gets properly updated when server returns a transformed content.

….posts.edits shape

matticbot · 2018-07-23T08:55:10Z

Test live: https://calypso.live/?branch=fix/edits-with-transformed-content

jblz · 2018-07-23T19:44:28Z

This is working brilliantly for me. All attributes I've tried (including page slugs) seem to only trigger the dialog at appropriate times on this branch.

There's still an issue with switching between editor modes & inline styles, but it's much more solvable with this in place. Thanks!

jsnajdr · 2018-07-24T06:32:10Z

This is working brilliantly for me. All attributes I've tried (including page slugs) seem to only trigger the dialog at appropriate times on this branch.

I'm glad to hear that @jblz! I added a proper PR description and now it's ready for review and more testing.

There's still an issue with switching between editor modes & inline styles, but it's much more solvable with this in place. Thanks!

Switching between editor modes is a different beast that will require a different fix. On every switch, functions wpautop and removep transform the HTML content and sometimes introduce unwanted changes.

Also, the [code] shortcode is special. In wp-admin, one can register pre- and post- hooks for the wpautop and removep functions that do custom transforms. The syntax highlighting plugin does exactly that to avoid breaking code formatting. But the hooks are not present in Calypso and [code] breaks all the time.

alisterscott · 2018-07-24T06:39:18Z

I'm still seeing some issues with [code] shortcodes and switching editor modes but I otherwise can't trigger the AYS prompt - so if this PR makes the editor code cleaner and easier to maintain it's great 🎉 Thank you

alisterscott

LGTM from a testing perspective

spen

I tried to reproduce on WP.com (staged) with the instructions inn the description and didn't see the notice. Possibly I wasn't reproducing correctly though as I'm sure I've seen it under those circumstances before.

Either way this does work as described and the method you describe sounds sensible and smart :)

Awesome work @jsnajdr 💚

spen · 2018-07-24T13:44:05Z

client/state/posts/reducer.js

@@ -415,7 +418,10 @@ export function edits( state = {}, action ) {
 					// Receive a new version of a post object, in most cases returned in the POST
 					// response after a successful save. Removes the edits that have been applied
 					// and leaves only the ones that are not noops.
-					const postEdits = get( memoState, [ post.site_ID, post.ID ] );
+					const postEditsLog = get( memoState, [ post.site_ID, post.ID ] ) || [];


tiny nit, [] default could be given as the 3rd argument here, get will return that as the default if the path (2nd argument) isn't found. FFTI though, this is inconsequential.

No no, that would actually be a horrible bug 😉 The value in state can be null in case when there are no active edits on the post. And get only uses the default value for undefined, nothing else.

Ahh ofc! I should have known there'd be good reason! Thanks for clarifying :D

Caused by changes introduced by #26261 (dependency on `getPodcastingCategoryId` selector). The tests in #26247 were authored before that PR was merged and it introduces new Redux dependencies: the test store needs also `sites` and `siteSettings` reducers now.

alisterscott · 2018-07-25T07:05:52Z

🤔 it looks like this PR introduced a race condition regression in the editor accordions where if you have the categories open and you save or an autosave happens the accordion is now closed:

#26293

Interestingly the e2e tests have started picking this up on Production, however they didn't find it on this PR when the tests were run - I think this is because of a race condition where if the post is saved before you open the category accordion then it remains open on subsequent saves

I pinpointed it to this PR by git checkout 0a433af46276a9c424f0180bcd16e6b1de4e6c2d after this PR which includes it, and git checkout c478a402b6a34b93ac34e082213506f175489e7f before this PR which doesn't include the race condition

jsnajdr added 8 commits July 23, 2018 08:55

Post Editor State: improve mergePostEdits to merge multiple objects

92b8a9f

`mergePostEdits` now takes a variable number of arguments and merges them all into one object. Also supports the "save marker" concept: string markers in the edits log that are skipped when merging.

Post Editor State: add new function appendToPostEditsLog

175faa9

The new function appends new edits to an existing log, merging with the last one if it's not a save marker.

Add editorSave action

4df6da3

It will be used by the `saveEdited` action to insert a save marker to edit log when save is initiated.

Make getPostEdits process array from state and return merged edits

2de267e

Make the saveEdited action insert a save marker and remove it on success

527b25d

Post Editor: add tests for the saveEdited flow

d48665a

Test the `saveEdited` flow with the real reducer and mocked network requests. Verifies that the post edits log gets properly updated when server returns a transformed content.

Update state/posts unit tests, mostly to reflect the changes of state…

be52cfb

….posts.edits shape

jsnajdr added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs e2e Testing labels Jul 23, 2018

jsnajdr self-assigned this Jul 23, 2018

alisterscott mentioned this pull request Jul 24, 2018

Editor: prompts to restore/discard changes when there are no changes #9833

Closed

jsnajdr requested review from a team, spen, jblz, alisterscott and nylen July 24, 2018 06:32

alisterscott approved these changes Jul 24, 2018

View reviewed changes

spen approved these changes Jul 24, 2018

View reviewed changes

jsnajdr merged commit fda8c0b into master Jul 24, 2018

jsnajdr deleted the fix/edits-with-transformed-content branch July 24, 2018 14:32

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 24, 2018

jsnajdr mentioned this pull request Jul 24, 2018

Fix Post Editor unit tests #26277

Merged

alisterscott mentioned this pull request Jul 25, 2018

Editor: Categories Accordion closes when autosave takes place #26293

Closed

jsnajdr mentioned this pull request Jul 26, 2018

Fix editor bug where the edited post is null for a moment while saving #26335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post Editor: when saving post, overwrite the local edits with post version from server #26247

Post Editor: when saving post, overwrite the local edits with post version from server #26247

jsnajdr commented Jul 23, 2018 •

edited

Loading

matticbot commented Jul 23, 2018

jblz commented Jul 23, 2018 •

edited

Loading

jsnajdr commented Jul 24, 2018

alisterscott commented Jul 24, 2018

alisterscott left a comment

spen left a comment

spen Jul 24, 2018

jsnajdr Jul 24, 2018

spen Jul 24, 2018

alisterscott commented Jul 25, 2018

Post Editor: when saving post, overwrite the local edits with post version from server #26247

Post Editor: when saving post, overwrite the local edits with post version from server #26247

Conversation

jsnajdr commented Jul 23, 2018 • edited Loading

matticbot commented Jul 23, 2018

jblz commented Jul 23, 2018 • edited Loading

jsnajdr commented Jul 24, 2018

alisterscott commented Jul 24, 2018

alisterscott left a comment

Choose a reason for hiding this comment

spen left a comment

Choose a reason for hiding this comment

spen Jul 24, 2018

Choose a reason for hiding this comment

jsnajdr Jul 24, 2018

Choose a reason for hiding this comment

spen Jul 24, 2018

Choose a reason for hiding this comment

alisterscott commented Jul 25, 2018

jsnajdr commented Jul 23, 2018 •

edited

Loading

jblz commented Jul 23, 2018 •

edited

Loading