Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send all fields when transmitting an autosave. #7092

Merged
merged 5 commits into from
Jun 4, 2018
Merged

Conversation

adamsilverstein
Copy link
Member

Description

merges the post object with toSend before sending the autosave, ensuring that unedited fields are sent with their current values.

Fixes #7078

How has this been tested?

Create a post and publish it.
Change the content of the post and wait 10 seconds for the autosave to fire.
Check the autosave REST request - note that the title is present in the request. Check the database and note the autosave title is present in the database.

Changes

I found that altering the original object sets up an infinite save loop, to avoid this I create a new object to send to autosaves and leave toSend untouched.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Object.assign( autosaveData,
{
title: post.title,
content: post.content,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike title and excerpt, toSend will always have an assigned value for content, and thus will always override. This line will never take effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wondering: Since isEditedPostAutosaveable uses property values from state.autosave to consider a field as having been changed, should we be using those here as well as the original value to fall back to? Maybe with a getCurrentAutosave selector?

toSend = {
	...getCurrentAutosave(),
	...toSend,
	parent: post.id
};

@@ -118,12 +118,23 @@ export default {

let request;
if ( isAutosave ) {
toSend.parent = post.id;
const autosaveData = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: If we use let instead of const to declare toSend, with spread syntax we could simplify this a bit:

toSend = {
	title: post.title,
	excerpt: post.excerpt,
	...toSend,
	parent: post.id
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if we want to take it a step further with Lodash's _.pick:

toSend = {
	...pick( post, [ 'title', 'excerpt' ] ),
	...toSend,
	parent: post.id
};

@aduth aduth added this to the 3.0 milestone Jun 4, 2018
@aduth
Copy link
Member

aduth commented Jun 4, 2018

@adamsilverstein I just pushed d10a64e which accounts for all of my feedback. The request body accounts for both the fields from the current post, but preferring values from state.autosave. I'm a bit unclear about what value we're expecting state.autosave to hold over time: For example, if I have an autosave, but then proceed to save/publish the post, should the value revert back to null (reflecting the fact that there is no autosave?).

Otherwise there may be some edge cases where e.g. post is autosaved (state.autosave assigned), then published (state.autosave currently not assigned), then edits made: Values from state.autosave would incorrectly take precedent over the last saved post values.

I don't know that that needs to be addressed as part of this pull request specifically.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@youknowriad youknowriad merged commit f897235 into master Jun 4, 2018
@youknowriad youknowriad deleted the fix/issue-7078 branch June 4, 2018 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants