Editor: Reset Post Edits when switching posts in the post editor #10177

youknowriad · 2016-12-20T12:57:10Z

If we create a new post, switch to a draft from the sidebar without saving and coming back to /post/new again, some initial post edits were kept instead of having a fresh new post. Currently, we can experience this for the "sticky" post flag for example.

While we continue reduxifying the editor, this behaviour will be more and more troublesome. (see #8814 )

This PR resets the postEdits while switching posts on the PostEditor Component.

Testing instructions

Start new post
Click on the "sticky" post icon
Before post saves, open drafts drawer and select another post
Accept warning that changes will be lost
Observe draft loads correctly
Start new post
The "sticky" post icon should not be selected, we should have a fresh post

matticbot · 2016-12-20T12:57:11Z

Test live: https://calypso.live/?branch=fix/editor/reset-edits

aduth

In my testing, this resolves the specific case I'd encountered 👍 One effect of this is that resets are fired after saving a new draft (no ID to saved ID) and when switching to a draft in the draft drawer, but it doesn't seem that this would problematic.

Another case which doesn't quite behave as I'd expect:

Start a new post
Toggle sticky
Click Write in master bar and select same site

Expected: Sticky should be reset?
Actual: Sticky not reset

I'm not sure whether this would be expected or not. If it were, maybe we need to fire the reset from the new post controller.

aduth · 2016-12-20T13:49:43Z

client/post-editor/post-editor.jsx

@@ -156,6 +156,12 @@ export const PostEditor = React.createClass( {

 	componentWillReceiveProps: function( nextProps ) {
 		const { siteId, postId } = this.props;
+
+		// When switching posts, reset post edits for the post we're leaving
+		if ( nextProps.postId !== postId ) {


A very edge case that might occur is switching from editing posts of the same ID on two different sites. Should we also check whether the site ID has changed?

Yeah, I think we should. I though these IDs were unique between sites.

youknowriad · 2016-12-20T14:39:52Z

@aduth The use-case you mentioned is probably worth fixing since the title/content is reset if we trigger a new Post from the master bar. Fixed in f38d7ba

aduth · 2016-12-20T14:59:01Z

client/post-editor/controller.js

@@ -115,6 +115,7 @@ module.exports = {

 		function startEditing( siteId ) {
 			context.store.dispatch( setEditorPostId( postID ) );


Just a thought - should the reducer for state.posts.edits be listening for the EDITOR_POST_ID_SET action type and reset itself internally, rather than us calling the reset action explicitly? A possible complication from this is that we might not know that the ID is different, though ideally we're only dispatching this action in cases where the ID has in-fact changed. Or we could track ID in edits itself when EDITOR_POST_ID_SET is dispatched and compare against it in the reducer.

After taking a look at where this action is used now, It looks like it's a good fit, but I'm wondering if it's not risky. One could use this action elsewhere and not knowing about the consequences. Should we rename it to something like startPostEditor( siteID, postID ). I don't think actions are meant to be "setters".

Also, does this mean, we should drop the current resetPostEdits calls (componentWillUnmount, componentWillReceiveProps, onSaveSuccess)? I would say yes, but maybe this would change the behavior a bit.

I agree that a more meaningfully named action creator might help avoid accidents. Do you think it would be used in addition to or in place of the existing setEditorPostId ? If in addition to, would we treat it as a totally separate action type or could we extend the existing action type with additional details / meta to describe it as being a "new" editing session?

Also, does this mean, we should drop the current resetPostEdits calls (componentWillUnmount, componentWillReceiveProps, onSaveSuccess)? I would say yes, but maybe this would change the behavior a bit.

I think it'd be nice to allow the editor component to not worry about these details. If I were to guess, I suspect we reset upon unmounting as a sort of clean-up task, so the state doesn't hang around after leaving the editor. As long as we properly reset when revisiting the editor, I don't know that there will be any problems. Otherwise we could fire some new stopEditing action creator upon unmounting / leaving the editor that has similar effects of resetting edit state.

Otherwise we could fire some new stopEditing action creator upon unmounting / leaving the editor that has similar effects of resetting edit state.

This is now ringing bells that we'd previously reset edits on a page.exit handler but changed away from that approach for some reason. Let me see if I can dig that up.

Do you think it would be used in addition to or in place of the existing setEditorPostId ?

I was thinking as a replacement on the controller, but I'm still wondering if I should keep the setEditorPostId for the specific usecase when saving a new post with ID yet, we'll have to update the ID without "starting a new editing session".

Let me try to put that in place, to have more context for discussion

Yes, so I added the startPostEditor action in 4c6278d but I kept the cleaning on unmount, because I was experiencing the same bug in #4539. SO any thoughts about the current implementation? Is this new Action worth adding? It looks ok, but the initial change was 4 lines of code :P

Thought, this seems more "logic"

Also just a small remark, I'm wondering why the lingering edits change the behavior of the "New" links. They seem unrelated to me. Maybe a bug on these links instead no?

Also just a small remark, I'm wondering why the lingering edits change the behavior of the "New" links. They seem unrelated to me. Maybe a bug on these links instead no?

There's an expanded explanation for the behavior on #4539

I think what you proposed: getNewEditorPath selector, is a good approach to solve this definitely.

Anyway, Should we merge the current approach of this branch?

aduth · 2016-12-21T16:12:38Z

Brain-dumping here, since I'm concerned we have too many ways of changing the ID of the edited post and too many places where we're resetting edits. Taking a step back and trying to think through expectations: If we've changed posts or navigated away from the editor, it makes sense to drop unsaved edits for the post we had been editing. The main complication here is when a new draft is saved, since we transition from an empty post ID to the draft's new saved ID.

I would like to think in that case that it would be fine to reset edits for the post, but I worry that if a user continues to make edits while a save request is in flight, those edits need to be preserved until next we save, so we can't simply reset the edits upon the save completing.

With this in mind, perhaps it is best that the setEditorPostId doesn't have any effect on the edits state. That's where action creators triggered upon starting an editing session or leaving an editing session might be more appropriate, like your proposed startPostEditor.

But then, should those edits be reset when starting or ending? I mentioned we don't want lingering state hanging around after leaving the editor, but we similarly need to reset edits when changing between posts while still in the editor (drafts drawer, master bar "Write").

As an aside, we already have some logic which needs to run when starting a post to assign the post's type, so this could fit well within an action creator which signals that we've started editing a post.

With these changes, what we have is resetting edits both when starting a post and when ending, but implemented in different ways. The resetting upon starting occurs in the controller by calling startPostEditor and when leaving in the component's componentWillUnmount calling resetPostEdits.

Could we make this more consistent? Perhaps your solution in f38d7ba is best in simply calling resetPostEdits in the controller explicitly.

Another idea is to have special-purpose startEditingPost and stopEditingPost, both in the controller, when entering and leaving the route respectively.

Or maybe we don't care if edits linger after leaving the editor, as long as we fix the bugs observed in #4539. That way we don't need to reset anything when leaving the editor.

If we do keep the new action creator, do you think it could simply extend setEditorPostId, something like:

export function startEditingPost( siteId, postId ) {
	return {
		...setEditorPostId( postId ),
		clearEdits: true
	};
}

... since almost all of the behavior is the same except the reducer logic for resetting? There we could just check for the presence of this new key.

This was long, sorry 😄 Just expressing some inner turmoil being uncertain what's the clearest solution.

youknowriad · 2016-12-22T08:40:27Z

Yeah difficult problem to solve and I see two way of thinking here:

1- Should we continue our Redux setters approach, which is simpler for this particular case (but maybe not for the longer term). In this case the solution would be:

Call resetPostEdits where needed: Unmounting and switching site while still on the editor like in f38d7ba And we could resolve the edgecase you mentioned about resetting edits for new posts being saved by checking that the old id should not be null before calling the reset combined with a state variable maybe (I have to look deeper to find the correct test to add)

2- The second solution is I think a more "redux" way of thinking but It requires bigger changes (and I'm not sure it's worth it). In this solution, we have to rethink our actions as a whole.

Having startEditingPost, stopEditingPost both in the controller as you said.
Having a savePost which I assume already exists (and we can use to reset the edits as well). We need a way in this action to tell, that we're saved a new draft (without id), or we updated an existing post.

With this solution, all resets should be handled on the reducers, and the component should not worry about "resetting" the edits explicitly.

That said, I'm more keen to try the second.

youknowriad · 2016-12-22T10:01:59Z

@aduth I tried the new approach (pure redux actions) in bcfc667 and I'm pretty satisfied with the results. What do you think?

aduth · 2017-01-02T20:14:22Z

client/post-editor/post-editor.jsx

-			this.props.setEditorPostId( post.ID );
-		}
+		// 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, {} );


Only by a bit of luck does this not cause a notice to be shown (which would be non-ideal, at least for now, given that it's redundant with the editor's own notice). See:

wp-calypso/client/state/notices/middleware.js

Lines 78 to 93 in 795cd4d

export function onPostSaveSuccess( dispatch, action ) {

let text;

switch ( action.post.status ) {

case 'trash':

text = translate( 'Post successfully moved to trash' );

break;

case 'publish':

text = translate( 'Post successfully published' );

break;

}

if ( text ) {

dispatch( successNotice( text ) );

}

}

(it would show a notice if {} instead included a status property).

Arguably this is an argument against the lack of discoverability of middlewares 😄

Yeah, I noticed these notices as well :). I'm starting to think, we should avoid using the middleware to automatically trigger notices on request success. I had to revert my own save settings global notices here #10278 to avoid double notices. I think we should rely on componentWillReceiveProps to trigger these notices.

Alternatively we could create separate extended action creators for a more specific variations if we only want a notice to be shown in some cases, e.g. adding a meta flag that the middleware checks the presence of.

aduth · 2017-01-02T20:16:38Z

client/state/posts/reducer.js

+			} );
+
+		case EDITOR_STOP:
+		case POST_SAVE_SUCCESS:


I wondered if clearing edits on success would have a negative impact where we currently depend on type to be always present, but realized that this should gracefully fall back to the saved post's type property. Nice 👍

About that, I think we should move the type out of the edits state. I don't think it should be stored there since it's not something editable. I would store this on the ui subtree, the same way we store the current edited ID. Maybe another PR

It is an edit in that it needs to be sent with the initial payload for a new draft of non-post types. Though you're right it's not really editable for saved items and posts of type "post".

aduth · 2017-01-02T20:19:04Z

client/state/ui/editor/reducer.js

 			return action.postId;
+		case POST_SAVE_SUCCESS:
+			return state === action.postId ? action.savedPost.ID : state;


I guess we need this condition since other posts could potentially be saved in the background that aren't related to the one we're editing? Seems okay enough, since we're effectively ensuring that the ID is kept in sync after a save (especially for draft -> saved post).

Also, in theory, we could switch the editor's post before the SAVE_SUCCESS is dispatched. but very unlikely to happen.

aduth · 2017-01-02T20:21:18Z

client/state/ui/editor/actions.js

+ * @param  {String}  postType Post Type
+ * @return {Object}           Action object
+ */
+export function startEditingPost( siteId, postId, postType ) {


Do you think we should default postType = 'post'? Fine to not, though "post" is something of a default in WordPress (see wp_insert_post, WP_Query post_type arguments).

Yeah, I'm all for more consistency between APIs.

aduth

This tests well for me across a variety of types, statuses, and flows. I'm happier with the recent changes; also considering whether this would be a more reliable mechanism for the "Resume Editing" feature (we do similar resetting for this after a save completes).

Code looks good too 👍

If we create a new post, switch to a draft from the sidebar without saving and coming back to /post/new again, some initial post edits were kept instead of having a fresh new post

aduth · 2017-01-10T16:33:54Z

client/post-editor/controller.js

@@ -177,6 +175,15 @@ module.exports = {
 		renderEditor( context, postType );
 	},

+	exitPost: function( context, next ) {
+		const postId = getPostID( context );
+		const siteId = getSelectedSiteId( context.store.getState() );


I'm seeing a few errors here on the Calypso errors dashboard:

Cannot read property 'getState' of undefined

It's not clear to me when or why context.store would be ever be undefined.

Maybe it's because https://github.com/Automattic/wp-calypso/blob/master/client/state/initial-state.js#L90 is async. It seems that the context is initialized on the callback of this function https://github.com/Automattic/wp-calypso/blob/master/client/boot/index.js#L260

Is this the only place we have this kind of errors? seems surprising to me

@youknowriad Following the logic from boot, I don't think we render anything until the initial state has finished loading. It may have something to do with it being a page.exit handler, which we don't use in very many (any?) other parts of Calypso.

youknowriad added [Feature] Post/Page Editor The editor for editing posts and pages. [Type] Bug labels Dec 20, 2016

youknowriad self-assigned this Dec 20, 2016

youknowriad requested a review from aduth December 20, 2016 12:57

youknowriad mentioned this pull request Dec 20, 2016

Editor: Reduxify The EditorTitle component #8814

Merged

aduth reviewed Dec 20, 2016

View reviewed changes

youknowriad force-pushed the fix/editor/reset-edits branch from dbf8b02 to 4c6278d Compare December 20, 2016 16:03

hoverduck added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 21, 2016

aduth reviewed Jan 2, 2017

View reviewed changes

aduth approved these changes Jan 2, 2017

View reviewed changes

aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 2, 2017

youknowriad added 5 commits January 3, 2017 09:00

Editor: Reset Post Edits when switching posts in the post editor

b641420

If we create a new post, switch to a draft from the sidebar without saving and coming back to /post/new again, some initial post edits were kept instead of having a fresh new post

Changes per review

d029c25

Adding editor start action

31c9c58

Trying a new approach

de3074e

Changes per review

f8f19dd

youknowriad force-pushed the fix/editor/reset-edits branch from bcfc667 to f8f19dd Compare January 3, 2017 08:03

youknowriad merged commit baa395a into master Jan 3, 2017

youknowriad deleted the fix/editor/reset-edits branch January 3, 2017 08:09

aduth reviewed Jan 10, 2017

View reviewed changes

lancewillett removed the [Status] Ready to Merge label Jan 18, 2017

youknowriad mentioned this pull request Mar 3, 2017

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Reset Post Edits when switching posts in the post editor #10177

Editor: Reset Post Edits when switching posts in the post editor #10177

youknowriad commented Dec 20, 2016 •

edited by artpi

Loading

matticbot commented Dec 20, 2016

aduth left a comment

aduth Dec 20, 2016

youknowriad Dec 20, 2016

youknowriad commented Dec 20, 2016

aduth Dec 20, 2016

youknowriad Dec 20, 2016 •

edited

Loading

aduth Dec 20, 2016

aduth Dec 20, 2016

youknowriad Dec 20, 2016 •

edited

Loading

youknowriad Dec 20, 2016

youknowriad Dec 20, 2016

youknowriad Dec 20, 2016

aduth Dec 20, 2016

youknowriad Dec 21, 2016

aduth commented Dec 21, 2016

youknowriad commented Dec 22, 2016

youknowriad commented Dec 22, 2016

aduth Jan 2, 2017

youknowriad Jan 3, 2017

aduth Jan 3, 2017

aduth Jan 2, 2017

youknowriad Jan 3, 2017

aduth Jan 3, 2017

aduth Jan 2, 2017 •

edited

Loading

youknowriad Jan 3, 2017

aduth Jan 2, 2017

youknowriad Jan 3, 2017

aduth left a comment

aduth Jan 10, 2017

youknowriad Jan 10, 2017 •

edited

Loading

aduth Jan 11, 2017

		@@ -115,6 +115,7 @@ module.exports = {

		function startEditing( siteId ) {
		context.store.dispatch( setEditorPostId( postID ) );

	export function onPostSaveSuccess( dispatch, action ) {
	let text;
	switch ( action.post.status ) {
	case 'trash':
	text = translate( 'Post successfully moved to trash' );
	break;

	case 'publish':
	text = translate( 'Post successfully published' );
	break;
	}

	if ( text ) {
	dispatch( successNotice( text ) );
	}
	}

Editor: Reset Post Edits when switching posts in the post editor #10177

Editor: Reset Post Edits when switching posts in the post editor #10177

Conversation

youknowriad commented Dec 20, 2016 • edited by artpi Loading

matticbot commented Dec 20, 2016

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Dec 20, 2016

Choose a reason for hiding this comment

youknowriad Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Dec 20, 2016 • edited Loading

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

aduth commented Dec 21, 2016

youknowriad commented Dec 22, 2016

youknowriad commented Dec 22, 2016

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

aduth Jan 2, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Dec 20, 2016 •

edited by artpi

Loading

youknowriad Dec 20, 2016 •

edited

Loading

youknowriad Dec 20, 2016 •

edited

Loading

aduth Jan 2, 2017 •

edited

Loading

youknowriad Jan 10, 2017 •

edited

Loading