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

Don't error if autosave runs and there are no changes to save #7347

Merged
merged 2 commits into from
Jun 18, 2018

Conversation

notnownikki
Copy link
Member

Description

If autosave runs and there are no changes, the user gets an error banner. This happens when there's an autosave newer than the post content you're editing, and autosave runs again.

errorerror

This change sets the status code of the error to 200, because the fact that there are no changes since the last autosave shouldn't be an error we alert users to.

Types of changes

Bug fix

@@ -309,7 +309,7 @@ public function create_post_autosave( $post_data ) {

if ( ! $autosave_is_different ) {
wp_delete_post_revision( $old_autosave->ID );
return new WP_Error( 'rest_autosave_no_changes', __( 'There is nothing to save. The autosave and the post content are the same.', 'gutenberg' ) );
return new WP_Error( 'rest_autosave_no_changes', __( 'There is nothing to save. The autosave and the post content are the same.', 'gutenberg' ), array( 'status' => 200 ) );
Copy link
Member

Choose a reason for hiding this comment

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

I think 400 is more appropriate here than 200. We should return 400 from the REST API, and then have special handling in Gutenberg for a 400 from the autosaves endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this to 400, and handled this specific error in the REQUEST_POST_UPDATE_FAILURE effect.

@danielbachhuber danielbachhuber added this to the 3.1 milestone Jun 18, 2018
@danielbachhuber danielbachhuber added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jun 18, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

If autosave runs and there are no changes, the user gets an error banner.

Ideally, we should never be sending an autosave request in the first place if there are no changes to be saved. As currently implemented, I could imagine there may be cases where the user makes a change and manually resets it back to the original value where autosave could happen (particularly for post content). And regardless it does seem reasonable we don't want to surface/alarm the user on this type of error.

Would this apply for any autosave error (i.e. never show warnings on failed autosave)? I'd guess depending on the error type, we would want to show a notice, so fine as-is.

const { post, edits } = action;
const { post, edits, error } = action;

if ( error && 'rest_autosave_no_changes' === error.code ) {
Copy link
Member

Choose a reason for hiding this comment

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

In what situations will we expect error to be empty such that we'd want to check its truthiness? If it's expected, we should have a separate unit test for this circumstance as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure here. While REQUEST_POST_UPDATE does include the error in the dispatched action, REQUEST_POST_UPDATE_FAILURE didn't use it until this change, and several tests didn't include an error either, so I felt it was safer to include this check until someone who knows things better could say for certain if the error would always be there or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants