Fix a few issues with saving post titles, dirty handling, etc.

[nylen]

I originally started this PR to fix the following bug:

When loading the example post content in the editor and clicking "Save", the post is saved with an empty title.

In the process it became a bit sprawly because I found some other related issues to fix.

The original bug is fixed by sending an EDIT_POST action when the editor is initialized with the new post content. This currently contains title and status. Adding status here also allowed me to eliminate the distinction the PublishButton currently makes between onSaveDraft and onUpdate, unifying these into a single onSave callback.

I also discovered that editing post visiblility or other properties was not registered as a change by the "dirty" logic. Adding EDIT_POST to the list of "dirtying" actions (except for the initial EDIT_POST action) fixes this.

Finally, it seems useful to have another flag that determines the information SavedState displays: whether the current post is new or existing. If the current post has not yet been saved, we should indicate this here. This component now has 4 states (the first 2 are new):

• New post, no changes: "edit" icon with text "New post"
• New post, changes: "warning" icon with text "New post with changes"
• Existing post, no changes: "saved" icon with text "Saved"
• Existing post, changes: "warning" icon with text "Unsaved changes"

To test

Test various combinations of loading new and existing posts, modifying them or not, then saving them. For example:

• Load the Gutenberg editor as a new post. Verify that the SavedState indicator displays an "editing" icon and the text "New post".
• Save the post. Verify that the title is saved correctly with the post content (fixed in this PR). Verify that the SavedState indicator displays "Saved".
• Refresh the page. Verify that the post has the correct title and the SavedState indicator still displays "Saved".

Load a new post. Make a change to the visibility under "Post settings" and verify that the SavedState indicator changes to "New post with changes" (fixed in this PR).

Load an existing post. Verify that the SavedState indicator switches between "Saved" and "Unsaved changes" when edits are made, Undo/Redo is clicked, and the post is saved (as appropriate). Fixed in this PR for updates like post visibility.

Questions

It seems like we need a way to initialize the edits state with properties that should be sent to save a new post. The need to initialize the title this way will go away when we remove post-content.js; however, there will probably be other properties we should set this way (status for example). Currently this PR accomplishes the task using an EDIT_POST action with shouldMarkDirty: false - but this feels like a bit of a hack to me. Can you think of a better way?

This PR introduces a new selector: isEditedPostNew. It feels to me like maybe we should be using this in the publish button and the savePost action as well, but it's not entirely clear to me how, because we do still need the actual post ID here to send it to the API. How can this be improved?

[youknowriad]

Trying to think broadly. I see edits as an object of attributes explicitly modified by the user, IMO I don't think we should do this because it creates confusion on what the edit reducer contains.

That said, I've already noticed the bug and as you said, the need for this may disappear when we'll drop the post-content file. An alternative could be to merge the edits with currentPost in the savePost action instead.

[nylen]

I see edits as more like "the attributes of the post that need to be saved, but haven't yet". status: 'draft' is a good one, along with whatever other defaults we choose for things like visibility. I think this might keep our code a bit cleaner because we can use the same logic to read the current state of the post, for our controls UI.

I don't think we should send the entire currentPost output with each save request. I'm aware of at one core bug that we would hit if we did this: https://core.trac.wordpress.org/ticket/39996 - and I suspect there are probably still a few more.

[nylen]

Copy/pasting #848 (review) here because it is still relevant, let's move this discussion into the main thread here.

@youknowriad

Trying to think broadly. I see edits as an object of attributes explicitly modified by the user, IMO I don't think we should do this because it creates confusion on what the edit reducer contains.

That said, I've already noticed the bug and as you said, the need for this may disappear when we'll drop the post-content file. An alternative could be to merge the edits with currentPost in the savePost action instead.

Me

I see edits as more like "the attributes of the post that need to be saved, but haven't yet". status: 'draft' is a good one, along with whatever other defaults we choose for things like visibility. I think this might keep our code a bit cleaner because we can use the same logic to read the current state of the post, for our controls UI.

I don't think we should send the entire currentPost output with each save request. I'm aware of at one core bug that we would hit if we did this: https://core.trac.wordpress.org/ticket/39996 - and I suspect there are probably still a few more.

[nylen]

e8113a8 does a bit to address my first question from the original post. Instead of an EDIT_POST action with shouldMarkDirty: false, this is handled by a separate action type SETUP_POST. I think I like this better except for the naming.

[youknowriad]

I see edits as more like "the attributes of the post that need to be saved, but haven't yet".

Me too (just phrased differently) and that's exactly why we shouldn't do this ...omit( post, 'title', 'content' ),. AFAIK this is also called while editing posts and we shouldn't include all the post attributes there.

status: 'draft' is a good one, along with whatever other defaults we choose for things like visibility.

Setting those sounds good to me (only for the new posts though, not while editing saved posts). But I don't think we should include the title, I don't mind leaving the bug specific to post-content right now. Or maybe find a way to make this call only when using the default content.

I don't think we should send the entire currentPost output with each save request.

Fair enough!

[nylen]

Or maybe find a way to make this call only when using the default content.

Ah, you are totally right! I forgot this would also be called when initializing the editor using an existing post. Addressed in 9fd008d.

[youknowriad]

@nylen

How do you expect us to provide the "default" edits for newly created posts? Should these be included in the post object provided to the editor (like we do now with post-content), or maybe it's better to include these defaults values inside the editor itself. (and maybe make the post object nullable)?

By this question, I'm trying to anticipate the behaviour of the editor when we'll drop the default post-content. And maybe separate the default edits we'll always have and the title (which is a special case needed only because we have a default post content).

[nylen]

How do you expect us to provide the "default" edits for newly created posts? Should these be included in the post object provided to the editor (like we do now with post-content), or maybe it's better to include these defaults values inside the editor itself. (and maybe make the post object nullable)?

I'm imagining something like this, yes. Or maybe the default values could be part of the reducer that handles SETUP_NEW_POST? In any case, I would prefer to avoid fragmenting the "default" state across multiple places, and I think where it lives now is close to the right place given how post-content.js works.

By this question, I'm trying to anticipate the behaviour of the editor when we'll drop the default post-content. And maybe separate the default edits we'll always have and the title (which is a special case needed only because we have a default post content).

This part isn't totally clear to me either. title is a special case; do you agree that there's value in having a set of "default" edits beyond that? Which other properties do you think we'll need?

I'd like to avoid a lot of logic like post.status || 'draft' spread throughout various places in the codebase, by providing a way to explicitly set some default values. I agree that these are "edits" in the sense of "properties that need to be saved with the post", but not in the sense of "changes the user has made".

I'll think about it some more over the weekend, hopefully I will get some more ideas while sleeping 😴

[youknowriad]

I've worked on something similar in Calypso and IIRC we don't have default edits aside the post type. But like you, I do think it's a good thing to have SETUP_NEW_POST and avoid fallback logic.

I was thinking these default values should be part of the editor itself (reducer or a constant in editor/index) (as opposed to an argument when initializing the editor), makes the editor more reusable IMO because for example, if we don't enforce the status to be set inside the editor, we wouldn't guarantee that post.status is filled and thus avoid the fallback. (But I'm not 100% settled on this)

What default edits I'm expecting, I don't know, maybe status, postType, defaultCategory

And Enjoy your weekend James 🙂

[aduth]

Some of this defaulting need assumes we'll stick with a flow where savePost decides what to do based on presence of an ID. Won't we have separate "Save/Revert as Draft" and "Publish" actions which can assign the status when invoked? Alternatively, maybe the default values could be assigned in the initial state of the reducer, and unset for non-new posts.

const DEFAULT_EDITS = { status: 'draft' };

function edits( state = DEFAULT_EDITS, action ) {
	switch ( action.type ) {
		case 'RESET_BLOCKS':
			if ( action.post.id ) {
				return {};
			}

			return state;
	}

	return state;
}

What are the cases we'd need to know the default values? Is it just dirty detection, or do we think it'd be useful to say getPostValue( state, postId, 'status' ) and return "draft" even if not explicitly assigned?

In Calypso we only use it for the former (but could easily do both), comparing against a DEFAULT_NEW_POST_VALUES constant in the selector call. There, default values aren't stored in state:

https://github.com/Automattic/wp-calypso/blob/c7ea1a5/client/state/posts/constants.js#L16-L26
https://github.com/Automattic/wp-calypso/blob/c7ea1a5/client/state/posts/selectors.js#L357-L358

This is another reason I'd like to see "dirtiness" determined by a selector, not tracked in state itself.

[nylen]

Some of this defaulting need assumes we'll stick with a flow where savePost decides what to do based on presence of an ID. Won't we have separate "Save/Revert as Draft" and "Publish" actions which can assign the status when invoked?

I don't follow this bit... I tried to remove a lot of this logic where savePost decides what to do based on whether or not we have an ID. Here are the only things that should be different based on whether we have an ID:

• Which API method do we call (POST /wp/v2/posts or PUT /wp/v2/posts/:id)?
• How do we know the post's current status? If it's a newly created post, we need to initialize it somewhere. This shouldn't be relevant to savePost.

function edits( state = DEFAULT_EDITS, action ) {

+1 on this approach, but it's not clear to me whether or not we should do that now, with post-content.js in place. Should we go ahead and set up the DEFAULT_EDITS constant and merge its values with the stuff from post-content.js, or should we leave the "default edits" where they are for now, with the TODO comment pointing to this thread?

What are the cases we'd need to know the default values? Is it just dirty detection, or do we think it'd be useful to say getPostValue( state, postId, 'status' ) and return "draft" even if not explicitly assigned?

As currently structured, the default edits don't matter for dirty detection.

I do think this will also be useful to avoid a lot of logic like getPostValue( state, postId, 'status' ) || 'draft'.

This is another reason I'd like to see "dirtiness" determined by a selector, not tracked in state itself.

I'm still sort of worried about this, because of the issues we've seen in the past with dirty detection in Calypso.

In theory, editing a block's content and then changing it back should not be a "dirty" state. However, this means that dirty detection has to rely on the block serialization logic. It seems preferable to me to keep this simple and explicit rather than having it depend on a lot of other complicated behavior.

At least, because dirty is attached to the undoable part of the state tree, undoing all edits is guaranteed to be a non-dirty state. Personally that is good enough for me.

[aduth]

I'm still sort of worried about this, because of the issues we've seen in the past with dirty detection in Calypso.

Is this referring to false positives on dirtiness when switching between modes? What is it about the current direction here that will address this problem better?

[nylen]

Is this referring to false positives on dirtiness when switching between modes?

This, and things like the editor starting as dirty when opening a new page.

What is it about the current direction here that will address this problem better?

For both of the cases above, we take a simple approach based on an explicit list of user-generated actions. Catching all the different (broad) types of user actions, and deciding for each action whether it represents a "dirtying" change, seems easier and simpler to me than serializing all content and post state and comparing it for changes.

However I would think redesigning the dirty detection is better addressed in a separate issue/PR.

[nylen]

Any objections to me merging this in the next day or so?

[youknowriad]

LGTM 👍