Editor: Fix over-riding of local title edits on auto-save. #11729

timmyc · 2017-03-03T01:33:58Z

This branch fixes #11567 which is causing local edits of the title field to be lost when the changes are made while an auto-save is in flight.

There is a video of the bug in #11567 - but to see the bug in action, add some content to a post ( draft ) you are editing, then focus on the title field. Watch the ground control area, and as soon as an auto save fires, start changing the title, and once the API returns, your local title changes will disappear 💥

I traced the issue back to onSaveSuccess in <PostEditor /> which was calling savePostSuccess which clears out any local edits in the state tree. onSaveSuccess also calls receivePost which iterates over the local edits, and compares it against the saved post from the API, and only deletes edits that match - so it seems like the call to recievePost should be enough here ( though /cc @aduth for your thoughts ).

matticbot · 2017-03-03T01:33:59Z

Test live: https://calypso.live/?branch=fix/editor/11567

youknowriad · 2017-03-03T07:47:36Z

client/post-editor/post-editor.jsx

@@ -713,9 +713,6 @@ export const PostEditor = React.createClass( {
 			this.props.resetEditorLastDraft();
 		}

-		// Remove this when the editor is completely reduxified ( When using Redux actions for all post saving requests )
-		this.props.savePostSuccess( post.site_ID, this.props.postId, post, {} );


This was introduced here #10177 and the idea IIRC was to update the editedPostId in the state when we're creating a new post. If we drop this, the editedPostId on the state will not reflect the saved post id. I'm not sure What's the impact yet.

One other thing to think about. Currently, since the editor is not reduxified, saving a post is called using the old Actions but when we reduxify everything, we would probably call the savePost action which could create the same bug. So maybe the fix for this would be to clear the edits when triggering the request instead of when receiving the response. The downside of this approach is when the save fails.

Alternatively, could we update the reducer for edits to migrate them to the updated post ID when saving a new post (i.e. going from null to valid postId)?

In my testing I haven't found any issues with removing this code. The same logic is repeated by exitPost which seems to clear up the original issue of lingering draft edits when leaving the editor.

So maybe the fix for this would be to clear the edits when triggering the request instead of when receiving the response.

This seems like a logical approach too. The action would have to clear edits specific for autoSave vs full save events since autoSave only sends Title, Excerpt and content so it should not clear out edits of other attributes.

Would you be willing to allow me to explore that approach in a subsequent PR @youknowriad and at least get this fix out to stop the loss of data? I hit this bug a few times yesterday while testing the new chrome and it really does take ones trust away from the save operations.

If we remove this, we'll want to remove the prop from mapDispatchToProps since this is the only place it's used. Just a small observation, still doing a more in-depth review...

aduth

I'm finding this to be a bit problematic in its current form. Saving a new draft with just a title doesn't update the URL, doesn't change from "New Post", doesn't hide the "Save" button, and the post remains dirty (attempting to navigate away prompts). From what existed prior to baa395a, it appears we at least need to update state to reflect that the post ID has changed when saving a new draft.

timmyc · 2017-03-03T21:16:37Z

Thanks for finding that issue @aduth - I will work on a solution.

timmyc · 2017-03-03T23:09:21Z

The issues were due to state/ui/editor/reducer still needing to have the POST_SAVE_SUCCESS action triggered in order to maintain a correct state. So I set about a fix to make the POST_SAVE_SUCCESS reducer respect any local changes in the edits subtree... but then realized the same logic is already happening in POSTS_RECEIVE so we could safely remove the logic around POST_SAVE_SUCCESS in the edits reducer entirely ( which was what my original fix effectively did ).

At some point we will need to revisit this when all is in redux, but for now this seems to be a logical fix. @aduth thoughts?

aduth · 2017-03-03T23:40:57Z

With the latest changes, it appears to revert back to the previous issue of losing changes made while the save request is in-flight. At least it does when writing a title for a new draft. I have a feeling that it's preserving the edits under the empty (new post) '' key, but since the post ID is updated to a numeric value, it's no longer associated with the post being edited.

gcorne · 2017-03-04T02:03:44Z

Just a random drive-by from someone from another era, but I fixed this issue back in the day by keeping a queue of changes that occurred while a request was in-flight and then reapplying them. I imagine that subtle aspect got lost during reduxification. See https://github.com/Automattic/wp-calypso/blob/master/client/lib/posts/post-edit-store.js#L98

gcorne · 2017-03-04T02:21:51Z

To add a tiny bit of additional color, the reason that the queue was necesary was to properly set the dirty state after one or more edit actions has occurred (perhaps mutating different properties of the post) while the save request is in-flight.

aduth · 2017-03-06T15:56:08Z

Hi @gcorne 😄 Hope you've been well!

With the current Redux state being capable of tracking multiple posts and edit values for multiple posts, I think what had previously been reflected in the queue could be imitated by tracking edits in the empty (new) post key and, at the time the save completes, simply replace (or copy values from) the key with the new post ID. Maybe a new action which notifies that the edited post's ID has changed, in this case from "" to 281 (or whatever the ID ends up being).

timmyc · 2017-03-06T16:31:12Z

👋 @gcorne - great to see you!

timmyc · 2017-03-06T21:03:34Z

@aduth in b7c5ec9I have added logic that when the prior save operation had a null postId copy over any edits from '' to the new savedPost.ID

aduth · 2017-03-06T21:06:06Z

client/state/posts/reducer.js

+			const { postId, siteId, savedPost } = action;
+
+			if ( postId ) {
+				return state;


Should we be consistent with how we exit early from the switch, between this and the break; above which achieve the same goal?

Also, should we make this so specific to the empty postId case? Presumably if it were ever the case that the ID from the savedPost differs from the postId we sent with the save, we'd want to re-key?

Not entirely sure I follow here, if the prior props.postId was set, and it differs from savedPost.ID? I hope that doesn't happen, but is that what you are talking about?

aduth · 2017-03-06T21:11:23Z

client/state/posts/reducer.js

+
+			// if postId is null, copy over any edits
+			return Object.assign( {}, state, {
+				[ siteId ]: {


The way we're assigning here will delete any other post IDs for the same site. While in practice this will probably never occur, and conveniently removes the '' (empty) key for us, we might want to preserve these either with _.merge or a nested Object.assign:

return { ...state, [ siteId ]: { ...state[ siteId ], [ savedPost.ID ]: state[ siteId ][ '' ] } };

Of course at that point we'd need to assume responsibility for removing the empty key:

const nextState = { ...state, [ siteId ]: { ...state[ siteId ], [ savedPost.ID ]: state[ siteId ][ '' ] } }; delete nextState[ siteId ][ '' ]; return nextState;

Alternatively we could use _.mapKeys to reassign the empty key:

return { ...state, [ siteId ]: mapKeys( state[ siteId ], ( value, key ) => ( '' === key ? savedPost.ID : key ) ) };

aduth · 2017-03-06T21:13:10Z

client/state/posts/reducer.js

@@ -282,6 +281,23 @@ export function edits( state = {}, action ) {
 				[ action.siteId ]: omit( state[ action.siteId ], action.postId || '' )
 			} );

+		case POST_SAVE_SUCCESS:
+			if ( ! state.hasOwnProperty( action.siteId ) || ! action.savedPost ) {


A little safety doesn't hurt, but is there ever a case where action.savedPost would be falsey?

Probably not, but one seems okay to err on the side of safe here.

timmyc · 2017-03-06T22:26:10Z

Went with the mapKeys approach in 940d479

aduth · 2017-03-07T13:23:12Z

Not entirely sure I follow here, if the prior props.postId was set, and it differs from savedPost.ID? I hope that doesn't happen, but is that what you are talking about?

Right, so the thinking is when we call savePost with a post ID, we include that original post ID in the final payload for POST_SAVE_SUCCESS, so if in the odd chance the ID for the post we assumed we were saving differs from what we receive in the response, we'd still want to reflect that by migrating the edits. Or at the very least, we needn't go out of our way to avoid it by adding conditions to check whether the postId is truthy.

But you're right, this shouldn't ever happen, and may very well have other implications if it were to, so I'm not too concerned about it.

aduth

This looks good and is working nicely now in my testing, both for a new draft and existing draft 👍

aduth · 2017-03-07T13:17:50Z

client/state/posts/reducer.js

+			if ( ! state.hasOwnProperty( action.siteId ) || ! action.savedPost || action.postId ) {
+				break;
+			}
+			const { siteId, savedPost } = action;


Minor, not worth another round of reviews: Could move destructuring above the condition to make the condition slightly shorter:

const { siteId, savedPost, postId } = action; if ( ! state.hasOwnProperty( siteId ) || ! savedPost || postId ) {

Editor: Fix over-riding of local title edits on auto-save.

f913d12

timmyc 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. labels Mar 3, 2017

matticbot added the [Size] S Small sized issue label Mar 3, 2017

youknowriad reviewed Mar 3, 2017

View reviewed changes

aduth suggested changes Mar 3, 2017

View reviewed changes

aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 3, 2017

Remove POST_SAVE_SUCCESS from edits reducer.

a7962c4

matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Mar 3, 2017

Add reducer logic to move edits to post.ID

b7c5ec9

matticbot added the [Size] M Medium sized issue label Mar 6, 2017

aduth reviewed Mar 6, 2017

View reviewed changes

Updates per feedback.

940d479

matticbot added the [Size] M Medium sized issue label Mar 6, 2017

aduth approved these changes Mar 7, 2017

View reviewed changes

aduth added [Status] Ready to Merge and removed [Status] Needs Author Reply labels Mar 7, 2017

timmyc merged commit f3a0d58 into master Mar 7, 2017

matticbot removed the [Status] Ready to Merge label Mar 7, 2017

mtias deleted the fix/editor/11567 branch March 7, 2017 17:31

timmyc restored the fix/editor/11567 branch March 8, 2017 02:57

This was referenced Mar 8, 2017

Revert 11729 #11878

Merged

Editor: Properly clear local state tree edits on POST_SAVE_SUCCESS #11907

Merged

alisterscott deleted the fix/editor/11567 branch October 16, 2017 03:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Fix over-riding of local title edits on auto-save. #11729

Editor: Fix over-riding of local title edits on auto-save. #11729

timmyc commented Mar 3, 2017

matticbot commented Mar 3, 2017

youknowriad Mar 3, 2017

youknowriad Mar 3, 2017

aduth Mar 3, 2017

timmyc Mar 3, 2017

aduth Mar 3, 2017

aduth left a comment •

edited

Loading

timmyc commented Mar 3, 2017

timmyc commented Mar 3, 2017

aduth commented Mar 3, 2017

gcorne commented Mar 4, 2017 •

edited

Loading

gcorne commented Mar 4, 2017

aduth commented Mar 6, 2017

timmyc commented Mar 6, 2017

timmyc commented Mar 6, 2017

aduth Mar 6, 2017

aduth Mar 6, 2017

timmyc Mar 6, 2017

aduth Mar 6, 2017

aduth Mar 6, 2017

timmyc Mar 6, 2017

timmyc commented Mar 6, 2017

aduth commented Mar 7, 2017

aduth left a comment

aduth Mar 7, 2017

Editor: Fix over-riding of local title edits on auto-save. #11729

Editor: Fix over-riding of local title edits on auto-save. #11729

Conversation

timmyc commented Mar 3, 2017

matticbot commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment • edited Loading

Choose a reason for hiding this comment

timmyc commented Mar 3, 2017

timmyc commented Mar 3, 2017

aduth commented Mar 3, 2017

gcorne commented Mar 4, 2017 • edited Loading

gcorne commented Mar 4, 2017

aduth commented Mar 6, 2017

timmyc commented Mar 6, 2017

timmyc commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmyc commented Mar 6, 2017

aduth commented Mar 7, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment •

edited

Loading

gcorne commented Mar 4, 2017 •

edited

Loading