State: Trigger autosave as standard save for draft by current user

[aduth]

Closes #7124

This pull request seeks to change an autosave request to occur as a standard save when the saved post is a draft or auto-draft, and the request is issued by the author of the post (i.e. current user is author).

This resolves two issues:

• Autosave a draft had shown that unsaved changes still exist (including prompt).
• Post preview would not work for drafts, unless first doing a full save
  • Note: Published post preview does not work and has never been implemented (Preview doesn't work after modifying on published posts #4099)

Implementation notes:

Note: This is a stub pull-request while I think through the implementation.

We currently do not track the current user ID in Gutenberg. As a short-term fix, this introduces access on the window.userSettings.uid value. Obviously this value may or may not be present, depending on where Gutenberg is implemented, but is assumed to exist in wp-admin.

Alternatively, we could trigger the autosave request, and allow the server to perform the necessary logic, testing that newPost.id !== post.id to see whether it was effectively saved as autosave. There are two caveats to this:

• Change detection (dirty prompt) resets on UPDATE_POST which only occurs for full saves
• The autosave endpoint returns entity in a slightly different shape than a post schema entity, so we can't simply resetPost with the autosave response value.

[jorgefilipecosta]

Are there any predictable non-desirable side effects of using normal save for an autosave? I feel the number of revisions has increased e.g: before for each autosave of a new not previously saved post we had no revisions now I think we have one revision on each autosave.

[aduth]

@jorgefilipecosta Part of the problem is that this is already how the server is treating it under these conditions, so we're simply not respecting (or taking advantage of) that.

[jorgefilipecosta]

Sorry @aduth it seems the number of revisions created is the same in both cases, I did something wrong in the first test cases.
Edited: It seems I found a consistent way to reproduce a revisions difference between this branch and master.
Create a new post.
never save it, write things until the autosave.
continue to write after the autosave until another autosave, repeat it until the autosave executes more than 3 times.
never manually save the post just allow auto save to save it.

When we execute this actions in master the autosave contains the last version and in the classic editor, there are no revisions available. If we execute this tests on this branch and we go to the classic editor we have the post as expected but we have one revision each autosave.

[aduth]

Nice sleuthing @jorgefilipecosta . Digging further, this is related to the constant that the endpoint defines when performing as an autosave, even if it ultimately just ends up calling the same wp_update_post.

gutenberg/lib/class-wp-rest-autosaves-controller.php

Lines 168 to 170 in 7141ce6

	if ( ! defined( 'DOING_AUTOSAVE' ) ) {
	define( 'DOING_AUTOSAVE', true );
	}

This is considered in preventing the creation of a revision in wp_save_post_revision:

https://github.com/WordPress/wordpress-develop/blob/f4580c122b7d0d2d66d22f806c6fe6e11023c6f0/src/wp-includes/revision.php#L112-L114

With this in mind, I'd be inclined that it might be better to follow the other option mentioned in my original comment about not trying to vary this in the client before the save, but rather calling the autosave endpoint and inferring after the fact whether it was saved with parent. There are a few complications to this which might require some broader refactoring to the save process.

[aduth]

I attempted to change the logic such that we infer after-the-fact whether the autosave was treated as a regular save, but encountered an issue: We can't reset the post dirty state after the save completes, because the user may have made edits while the request was in-flight that need to be respected as dirtying the post.

Instead, what I opted for in c36ad21 was to keep the logic which determines from the client whether the server will treat the request as a regular save, considering it as a regular save for purposes of resetting change detection (before request is issued) and in resetting some post fields, but importantly still using the /autosaves endpoint to perform the save.

I still think there's room for improvement here:

• That the autosaves endpoint returns a response in a similar-but-not-identical shape to that of the regular post save endpoint requires awkward field picking. Ideally these were aligned so that it could be received verbatim, or maybe we need to revise how we track posts in state to an application-specific format and create a pattern to map received entities values, whether from the post endpoint or the revisions endpoints.
• Still have the issue where we're relying on globals to determine that the request is being issued on behalf of the author of the post. I'd think we'd want this to be made available from core-data, e.g. select( 'core' ).getCurrentUserId()

The effect handler is quite complex, and I'm not super comfortable with all of the conditions necessary to reconcile saving and autosaving therein. Some of this may be unavoidable, which is why I'm going to try to work on some more end-to-end tests here. There's a new one already, though I also want to ensure that previewing works correctly, which is also meant to be addressed by these changes.

[aduth]

I took a different approach in 4df1bb6, leveraging redux-optimist not just for handling failure, but for consolidating the behavior of UPDATE_POST to optimistically assume the autosave will be applied as a full save, and then reverting that assumption for both (a) if a failure occurs (has already existed) and (b) if it turns out that a revision is actually created (introduced in 4df1bb6). This way, we don't need to duplicate the logic in the client for detecting whether the request will be applied as a revision.

This required tinkering with the reset and ignore types of withChangeDetection, as previously we would reset dirty state on UPDATE_POST. Since we want to apply cherry-picked properties from the saved revision to the post if it's applied as a full save, we want to take advantage of the same reducer logic for the action type but not reset change detection. This handles the case where the user made more edits while the request was in-flight. To me, it also seems reasonable that it is the begin of the save transaction that the reset is expected to occur which can be later reverted via redux-optimist on failure or revision creation (e.g. published post autosave should still prompt as being dirty).

One issue that came up in discussing this with @youknowriad are the fields we expect to update in an autosave request which falls under the condition of "same user draft". These are applied as full saves, but do we send just the title, content, and excerpt? What about other fields like status or terms? We can't treat the post as not having unsaved changes unless we can be certain that these are all updated on the server. Currently we do send them all, but is it expected that the server would save all these values, or just those relevant for a revision?

I got to thinking that since state.editor.edits unsets properties once they become considered the canonical value, we could enhance withChangeDetection to allow custom logic to determine state as becoming changed. Something like:

withChangeDetection( {
	isChanging( state, nextState ) {
		return state !== nextState || ! isEmpty( nextState.edits );
	},
} );

With above, we could assure that if only a subset of properties are updated, leaving remaining unsaved changes, they would be accounted for in change detection (the dirty post prompt).

cc @adamsilverstein @youknowriad @danielbachhuber

[jorgefilipecosta]

If the server returns a different excerpt (or title, content, etc... ) on the autosave endpoint ( because of a filter, for example) this revert is going to revert the changes made by the server, and they are not going to be reflected by UI right? This is something that I'm not sure if it is even possible but I may be missing something, and it may be helpful for me to understand the changes.
Edited: I think I understand the idea now, the revert is only going to revert the update post action with the redux optimistic id the other actions e.g: resetAutosave don't have the redux optimistic id and will stay untouched. So if the endpoint returns something different we are going to reflect it :)

[aduth]

If the server returns a different excerpt (or title, content, etc... ) on the autosave endpoint ( because of a filter, for example) this revert is going to revert the changes made by the server, and they are not going to be reflected by UI right? [...] I think I understand the idea now, the revert is only going to revert the update post action with the redux optimistic id [...] So if the endpoint returns something different we are going to reflect it

Yes and no, but it's an important point to raise, one which is partially considered in #7124 but which this pull request doesn't address entirely: A resetPost called after the save completes will update the canonical post values and remove edits from state.editor.edits. A revision, if created, is a separate entity from the post and shouldn't update state.currentPost. But: Draft autosaves by same author are full saves, not separate revisions. These should update title, content, excerpt for state.currentPost (not sure about other fields). This doesn't exist yet, but probably should. The UI shouldn't be impacted though, since the edits reducer does a diff on UPDATE_POST and RESET_POST: the edits values will remain. The one exception is currentPost.status which needs to be updated by autosave received since it's considered for the isEditedPostNew selector. Ideally this would be generalized to receive all post attributes which we know to have been updated by an autosave to the root post.

[jorgefilipecosta]

We know that the server does not save all the fields during autosave should we pick here the fields we send to the server, in order not have unused bytes sent to the server? Or they are being sent all the time because the server may threat autosave as normal save and in that case even during an autosave all the edits may get saved?

[aduth]

We know that the server does not save all the fields during autosave

Do we know this? I sincerely don't know and would be curious to know the answer. In theory it seems it should only need to save these fields, but in the implementation it's just calling wp_update_post and doesn't have its own implementation of prepare_item_for_database, falling back instead to WP_REST_Revisions_Controller::prepare_item_for_database. It's hard to follow if these fields might be dropped by part of a schema definition in one of the autosaves, revisions, or post controller classes.

gutenberg/lib/class-wp-rest-autosaves-controller.php

Line 185 in 102445b

$autosave_id = wp_update_post( wp_slash( (array) $prepared_post ), true );

That said, to the previous conversation, it might make things simpler if we could assume that at most, only title, excerpt, and content would be updated in order to know how to update state.currentPost.

[aduth]

Do we know this? I sincerely don't know and would be curious to know the answer.

I tested this, and for a new post, if I change author and hit preview (i.e. autosave), the author is updated.

[adamsilverstein]

We know that the server does not save all the fields during autosave

Nearly all fields (at least date, date_gmt, post_title, post_content, post_excerpt, post_modified, post_modified_gmt), are saved to the autosave as a new wp_posts row linked to the original post by a post_parent column. some fields get a special value: post_name (usually the slug) is stored as [POST_ID]-autosave-v1, post_status is inherit, post_type is revision.

I'm not certain about to_ping, pinged, post_password, ping_status, comment_count, menu_order, comment_status - I think these are saved. guid is a unique id pointing to the autosave.

related - attached meta fields are currently saved to the original post a longstanding core bug, see https://core.trac.wordpress.org/ticket/20299 - we recently fixed this for featured image selection, however a more complete implementation would need to support revisioning of meta, since we need to attach the saved meta to the autosave for previewing. see https://github.com/adamsilverstein/wp-post-meta-revisions and https://core.trac.wordpress.org/ticket/20564

[aduth]

Related (extended by): #7189

[jorgefilipecosta]

Instead of a revert would the following idea work:?
Save the post the autosave logic (but send flag) and maybe pick the fields to send, and then when we receive the return value from the server we compare the fields against the edits. In the case, a field was present in the edits and not returned by the server we use an action e.g: RESTORE_EDITS to restore those edits. This would make the post dirty again because some fields edits are not saved.

[aduth]

Instead of a revert would the following idea work:?

This reminds me of another goal: For autosaving a new draft post, ideally the user should not be prompted about unsaved changes after the autosave completes. This only works if we know that all fields are transmitted and considered in the save request.

Rather than a standalone action, something like the isChanging implementation in my comment above might serve the same purpose to what you describe.

[youknowriad]

It looks like there's the issue I was concerned about:

• Create a new post
• Add some content
• Add a tag to this post
• Let it autosave
• Refresh the page
• Everything is fine but the tag disappeared.

[aduth]

It looks like there's the issue I was concerned about:

Thanks for testing @youknowriad . That's disappointing, and I can reproduce, but maybe highlights to an opportunity to enhance as described in #7130 (comment) with a more aware change detection which considers the non-empty edits, where only if we're confident that the post updated includes all sent properties do we reset the change detection (not just blindly on REQUEST_POST_UPDATE_START).

[aduth]

I think I'll plan to merge #7189 into this branch just so I'm not managing related changes in two separate branches / pull requests.

[adamsilverstein]

Everything is fine but the tag disappeared.

@youknowriad can you clarify, did you publish the post, or did it remain a draft?

Please note this matches the existing behavior in the classic editor for a published post (for draft posts the autosave action is the same as the save draft action and does save the tags). Perhaps for drafts we should avoid using the autosave endpoint, just do a regular draft save (only difference vs clicking the save draft button: update the draft without creating a revision?)

[aduth]

This has turned into something of a rabbit hole, of all things impacted largely by shared blocks. Below is a brain dump of the problem:

We can know whether an autosave will have lingering unsaved changes (e.g. tags) by picking only those fields we expect the endpoint to handle as part of the payload, and in the UPDATE_POST action. This requires changing the behavior of dirty detection to more closely inspect state than to rely on specific app lifecycle. In other words, we can't simply assume the post is persisted after a save.

The editor.edits state already unsets values when they become the saved value, so at its most basic, we could say that the editor has unsaved changes if a property ever exists here. This is made complicated by the manipulation of blocks, which are not kept directly in sync with edits (i.e. the content field).

Thus, determining whether a post has unsaved changes is a two-step process: Check whether there are any properties in editor.edits, and whether the state of blockOrder and blocksByUID has changed since either the initialization of the editor or the last save.

There's a few ways we could go about doing this. Ultimately I landed on refactoring blocksByUID and blockOrder into a new object shape blocks.byUID and blocks.order where the top-level blocks reducer could have an independent withChangeDetection. Then the selector for isEditedPostDirty becomes:

export function isEditedPostDirty( state ) {
	return (
		state.editor.present.blocks.isDirty ||
		! isEmpty( getPostEdits( state ) ) ||
		inSomeHistory( state, isEditedPostDirty )
	);
}

Now enters shared blocks: In order to have consistent treatment of a block regardless of whether it is shared or not, as part of #5228 I moved shared blocks data into editor.blocks. Thus the problem: By simply receiving shared blocks when the inserter is shown, the state value changes, and the editor will start treating itself as though there are unsaved changes.

We work around this in master by setting an ignoreTypes for the withChangeDetection to not consider RECEIVE_BLOCKS as dirtying state. And technically now that I consider it, that might be the most direct solution here. But in considering more closely what it means for an editor to be in a dirty state, it's become increasingly odd to me that we include shared blocks within the editor state itself, which otherwise accounts for the current unsaved change progress for an editor session (something which shared blocks have nothing to do with).

This also surfaces itself in some interesting ways, because we do assume that blocks in editor are actually blocks present in the editor. Case in point: If you have a shared Heading block, every post will report in its Document Overview as including a heading, even if it doesn't.

Because the getGlobalBlockCount selector operates on state.editor.present.blocksByUID, it will include shared blocks (Note: May only appear if having opened the inserter at some point to trigger fetch for shared blocks):

gutenberg/editor/store/selectors.js

Lines 565 to 579 in 3504ae7

	export const getGlobalBlockCount = createSelector(
	( state, blockName ) => {
	if ( ! blockName ) {
	return size( state.editor.present.blocksByUID );
	}
	return reduce(
	state.editor.present.blocksByUID,
	( count, block ) => block.name === blockName ? count + 1 : count,
	0
	);
	},
	( state ) => [
	state.editor.present.blocksByUID,
	]
	);

How then do we solve this? We could consider shared blocks data separate from the editor state, and even retain consistency in doing so by extracting a common blocks reducer which is used for both editor and sharedBlocks. But when and why do we want to consider blocks as including both the set present the editor and those which aren't present but in shared blocks state? For getGlobalBlockCount, we don't want to consider shared blocks. For getBlock, we might. This could result in some unfortunate logic for checking in both places:

const block = (
	state.editor.present.blocks.byUID[ uid ] ||
	state.sharedBlocks.blocks.byUID[ uid ]
);

Let alone the confusion caused by mixed messaging in expecting shared blocks to sometimes be referenced by a selector.

Where do we go from here?

• As I type this, I realize ignoreTypes on the withChangeDetection for avoiding consideration of received shared blocks may be an easy direct solution for the short-term. It does not solve the bigger problem though.
• Separately, we should explore extracting shared blocks data out of the editor state and discussion where and why it makes sense to consider shared blocks as part of the block getter selectors
• Or put effort toward simplifying understanding of a post being dirty as exclusively operating on the edits, where blocks are either kept directly in sync with the content property or at least assign a temporary value to be considered at save time (for performance's sake, direct syncing may be unadvisable). Then it wouldn't matter so much if we group shared blocks and editor blocks together, at least for the sake of change detection. We'd still need to solve issues like getGlobalBlockCount considering blocks outside the set included in the current editor's content.

cc @noisysocks (having experience with shared blocks)

[youknowriad]

@aduth Related #7119

I do think shared blocks shouldn't live in the editor state at all. I tried doing this in #7080 but to handle nested blocks in shared blocks we need to refactor how to edit shared blocks (how to show the block in edit mode)

I'm proposing some ideas in #7119 maybe the way to go is

We can also try to load another Gutenberg instance :man-shrugging: when clicking the “edit” button of a reusable block, but this obviously requires a lot of work to make Gutenberg reusable this way (which should benefit things like multiple Gutenberg instances in a page) and this also means we’d need to figure out where to show the inspector of this inner instance, should it be shown above the outer instance’s inspector.

No matter the solution adopted, It will require a lot of work IMO

[azaozz]

It looks like there's the issue I was concerned about:
...
Everything is fine but the tag disappeared.

Yeah, "historically" some post boxes on the Edit Post screen save "instantly" (with AJAX), other require a form submission/page reload. Autosave only saves title, content, excerpt.

However there are two preview buttons: one to preview a draft post, another for published posts:

• Clicking Preview on a draft saves the draft as usual, then redirects to the front-end and loads the draft post there.
• Clicking "Preview Changes" on a saved post, triggers an autosave (hence saves only title, content, excerpt) and then "patches" the actual published post while displaying it, replacing these three fields with the data from the latest autosave. This is done so the published post is not modified before the Update button is used.

Thinking this logic makes sense. We shouldn't update a published post or its settings from autosave or when previewing it.

Also thinking that in Gutenberg autosaves can be "full" saves for drafts, but partial saves for published posts, following the above logic. That will remove the long-standing problem with some post boxes being "instant" and others needing the user to click "save".

[aduth]

Also thinking that in Gutenberg autosaves can be "full" saves for drafts, but partial saves for published posts, following the above logic.

This is the assumption I've been operating under, but the testing instructions in #7130 (comment) also apply for draft posts as well (i.e. not saving tags, even though they are included in the payload). I suppose it makes some sense that the endpoint have a consistent schema for the parameters it accepts regardless of published vs. draft, but it requires some additional handling in the client since we can't assume all fields are saved, even if, as a draft, those fields which are saved are applied to the original post (contrasted with a published revision).

[noisysocks]

My gut feeling is we should get shared block state out of editor and explore a different approach to supporting shared nested blocks. As mentioned, the approach we went with in #5228 has come with some issues: what Andrew described above, #7119, and also #5754.

I understand #5228 was an alternative to #5010 so maybe we can revive that approach?

Another reason for getting shared blocks out of editor is that it might make it easier to use a resolver for managing shared blocks which is something I'd like to explore.

[aduth]

@noisysocks I have a work-in-progress branch which renders shared blocks as an embedded editor, similar to suggested idea and subsequent discussion in #7119. It works fairly well, though is a significant refactor. Most all of the behaviors I have working cleanly, aside from some few remaining items like shared block deletion and converting to/from shared/static.

Given that this bug has persisted much longer than I would have liked, my current plan is to see if I can find a more direct solution here, likely to manually trigger a dirtying change to state after a RESET_POST if there are any values remaining in state.editor.edits.

Separately then we can work toward the refactoring of shared blocks and editor dirtiness as a condition of state.editor.edits + change detection on blocks state. For the latter, I found yet another complication with our treatment of "initial edits". For new posts, we populate edits with some values which are not expected to be considered as dirtying (e.g. "auto-draft" -> "draft" status). My plan here is to formalize the idea of the overrides to be considered a separate (non-change-detected) state value which is merged with state.editor.edits, and is populated from the overridePost we currently assign from EditPostEditor:

gutenberg/edit-post/editor.js

Line 25 in c6cbfb0

<EditorProvider settings={ editorSettings } post={ { ...post, ...overridePost } } { ...props }>

[aduth]

I've rebased and pushed changes which account for all known flows:

• When autosaving (previewing) a draft post by current user...
  • If edits only include title, content, excerpt, the user will not be prompted about unsaved changes
  • Otherwise, the user is prompted about unsaved changes
• For all other instances, autosaving (previewing) will prompt the user about changes being unsaved

There's some unfortunate complexity in managing this with the autosaves endpoint (related to supported fields and auto-draft status), which I've documented in the relevant Trac ticket.

The current implementation for ensuring that unsaved edits (e.g. tags, sticky) are presented in a warning prompt after a draft/auto-draft autosave is intended to be a temporary resolution while a better (bigger) refactor can be achieved (#7409).

[aduth]

Re: #7130 (comment) , The idea for an isChanging option for withChangeDetection was not viable because of its internal implementation in returning the previous state value:

gutenberg/editor/utils/with-change-detection/index.js

Lines 31 to 34 in 657c368

	// If not intending to update dirty flag, return early and avoid clone.
	if ( ! isChanging && ! isReset ) {
	return state;
	}

Since with #7130 (comment) the assumption would be that state changes over time, but the isDirty flag only alters on certain conditions, this would wrongly not respect changes. The only workaround to allow nextState to be returned from the implementation would require that we create a shallow clone on every single dispatch, to ensure that the isDirty flag is always included after the original reducer is run. This would certainly have an undesirable performance impact (since the state value would always be referentially unequal even if the original reducer had not returned a new value).

[aduth]

I had also attempted to introduce a withSnapshot higher-order reducer to replace withChangeDetection, so that we could compare blocks state to an earlier point in time (post start or last post saved):

isShallowEqual( [ state.editor.blocks.byUID, state.editor.blocks.byUID ], snapshot );

This is viable, but only once we refactor blocks state to omit shared blocks, since with RECEIVE_BLOCKS, the state value does change, even if the only thing that has actually changed is the receiving of shared blocks (not technically a dirtying change).

[youknowriad]

Thanks for the awesome work on this PR. I tested all the use-cases I can think of and it works as expected for me.

Thanks for the e2e tests

[aduth]

There are still some lingering edge case issues with autosave, particularly around non-content edits when an autosave exists for a post, which I've documented in #7416. These exist on master as well. I had started down the path of resolving them here before deciding that this pull request is already large enough as it is, and works well enough for the majority case.