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

Introduce a dedicated autosaves endpoint for handling autosave behavior #6257

Merged
merged 124 commits into from May 31, 2018

Conversation

Projects
None yet
7 participants
@adamsilverstein
Contributor

adamsilverstein commented Apr 18, 2018

Description

Based on previous work from #4156.

Leverages the final version of the Autosaves controller from @azaozz in Trac, https://core.trac.wordpress.org/ticket/43316#comment:76

See #4112.

Does not implement the autosave alert when an autosave exists and the deletion of stale autosaves on load, these are handled in #4218

How Has This Been Tested?

Local testing, db monitoring. Verified behavior as expected, matches core.

  • Create a new post, don't type anything
  • Check the database, there will be a single auto-draft
  • type a title and some content, pause for 10 seconds
  • note the message 'Autosaving' ... 'Saved' ... 'Save draft'
  • check the database, not that there is a single row for the post, with a draft status
  • Click the 'save draft' button
  • note the message 'Saving' ... 'Saved' - button stays in saved state
  • check the database, note a revision has been created because of the manual save action (not an autosave)
  • publish the post
  • check the database, note the post status is now published
  • make some more changes and wait 10 seconds
  • note the message 'Autosaving' ... then disappears (not save draft option) - @todo maybe should show saved briefly here?
  • check the database, note an autosave revision has been created
  • make some more changes and wait again for the autosave
  • check the database and note the autosave is updated, and that there is only one autosave
  • note that the 'update button' is available, click it
  • note that 'update' is disabled (until you change the post)
  • check the database, note that a new revision was created
  • repeat the last three steps
  • exit the post edit screen and log in as a different user, open the same post
  • edit the post and wait for an autosave
  • check the database, note that there are now two autosaves for the post, one for each user.

Types of changes

  • New WP_REST_Autosaves_Controller
  • New REQUEST_POST_AUTOSAVE effect, autosavePost action
  • Tracks isPostAutosavable separately from isEditedPostSaveable
  • Leverage withChangeDetection to track autosave state (this needs more testing)

Checklist:

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

@adamsilverstein adamsilverstein requested review from aduth and azaozz Apr 18, 2018

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented Apr 18, 2018

I see some tests are failing, I'll get that fixed.

@danielbachhuber

Excited to try this out! Left a few comments to start.

@@ -353,9 +353,10 @@ export function editPost( edits ) {
};
}
export function savePost() {
export function savePost( options ) {

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 23, 2018

Member

Do these options merit a doc block?

This comment has been minimized.

@adamsilverstein
*
* @see WP_REST_Controller
*/
class WP_REST_Autosaves_Controller extends WP_REST_Revisions_Controller {

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 23, 2018

Member

Does the WP_REST_Autosaves_Controller class need to be named something else to avoid conflict with core?

This comment has been minimized.

@adamsilverstein

adamsilverstein Apr 23, 2018

Contributor

changed to GB_REST_Autosaves_Controller

array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_items' ),
'permission_callback' => array( $this->revision_controller, 'get_items_permissions_check' ),

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 23, 2018

Member

Why do we need to instantiate a $this->revision_controller instance if we're subclassing the revisions controller?

This comment has been minimized.

@adamsilverstein

adamsilverstein Apr 23, 2018

Contributor

instantiating the parent sets it up correctly for the subsequent method calls.

return new WP_Error( 'rest_post_invalid_id', __( 'Invalid item ID.', 'gutenberg' ), array( 'status' => 404 ) );
}
return $this->parent_controller->update_item_permissions_check( $request );

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 23, 2018

Member

It's probably better to copy the contents of $this->parent_controller->update_item_permissions_check() into here.

I'd be concerned about a point in the future where $this->parent_controller->update_item_permissions_check() changed and caused an unexpected change in behavior in this controller. If we want to formalize its behavior into an explicit API, we can do so, but we should make sure we're intentional about it.

This comment has been minimized.

@adamsilverstein

adamsilverstein Apr 23, 2018

Contributor

the parent controller here is the post this autosave belongs to, and the permissions of the autosave decend directly from the parent post - this is similar to the approach we use on the revisions class.

define( 'DOING_AUTOSAVE', true );
}
$post = get_post( $request->get_param( 'id' ) );

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 23, 2018

Member

We should rename id to post. id is typically a read-only value generated by the database.

This comment has been minimized.

@aduth

aduth May 24, 2018

Member

We should rename id to post. id is typically a read-only value generated by the database.

Was this addressed @danielbachhuber ?

This comment has been minimized.

@danielbachhuber

danielbachhuber May 24, 2018

Member

I think we need to land this and iterate. I don't see my comment as blocking for the PR.

foreach ( get_post_types( array(), 'objects' ) as $post_type_object ) {
if ( ! empty( $post_type_object->rest_base ) ) {
if ( post_type_supports( $post_type_object->name, 'revisions' ) ) {
$autosaves = new WP_REST_Autosaves_Controller( $post_type_object->name );

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 23, 2018

Member

Can we include the PHPUnit tests for this controller too?

This comment has been minimized.

@adamsilverstein

adamsilverstein Apr 23, 2018

Contributor

sure assuming its straightforward to bring them over from core.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Apr 23, 2018

  • note the message 'Autosaving' ... 'Saved' ... 'Save draft'
  • note the message 'Autosaving' ... then disappears (not save draft option) - @todo maybe should show saved briefly here?

See also #4644

Maybe we can steal behavior from elsewhere (Calypso, Google Docs)?

aduth added some commits May 25, 2018

Keyboard Shortcuts: Disallow non-dirty save requests
UI restriction previously accounted for by fact that `AUTOSAVE` handler had aborted when post not dirty. This will need to be a future iteration on `savePost` effect.
State: Track autosave object as subset of received properties
Only those used in isEditedPostAutosaveable implementation
State: Remove redundant condition from autosaveable
Since already checking isEditedPostSaveable, which returns false if save (including autosave) is in progress
State: Simplify isEditedPostAutosaveable as Array#some of fields
Consolidate to getEditedPostAttribute as canonical source of fields values
State: Deprecate getEditedPostExcerpt selector
Consolidate to getEditedPostAttribute. Unlike content, there is no special behavior for excerpt. Even if there were, getEditedPostAttribute should be the entry point, which like content, would only then defer its implementation to getEditedPostExcerpt.
@aduth

This comment has been minimized.

Member

aduth commented May 25, 2018

I've pushed several commits in response to my own prior feedback. Most include extended commit descriptions with more explanation.

At a high-level, the goals were:

I will dismiss my own review. If all appears well to you, feel free to merge.

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented May 29, 2018

@aduth Thanks for all your work here, I will give this a test to verify the autosaves are still working as expected. I see there is a merge conflict now, I'll see if I can resolve that.

What's the relationship between autosave and saving meta box values?

in the current core implementation, metabox data is not included in autosaves, however the heartbeat transport mechanism is hookable in a way that enables developers to add meta box data to autosaves on their own if they want. for regular (draft/update) saves, all of the metabox data is sent as part of the <form submission and developers can then store/respond on the PHP side. To match that here, we can probably exclude that triggering for autosaves.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 31, 2018

I see there is a merge conflict now, I'll see if I can resolve that.

Fixed in 0bec4ea

danielbachhuber added some commits May 31, 2018

Remove tests originally removed in bdcbf71
These ended up on the wrong side of the merge conflict 0bec4ea
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 31, 2018

Green means go! @pento said he'd clean up any fallout 😝

@danielbachhuber danielbachhuber merged commit 5a6c24e into master May 31, 2018

2 checks passed

codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +4.58% compared to eb4fc18
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the fix/autosaves branch May 31, 2018

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented May 31, 2018

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

Thanks @danielbachhuber - I'll also pick up any issues!

@aduth

This comment has been minimized.

Member

aduth commented May 31, 2018

@danielbachhuber Can you please direct me to the issue and/or pull request where the following promised iterations are addressed?

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 31, 2018

Can you please direct me to the issue and/or pull request where the following promised iterations are addressed?

https://core.trac.wordpress.org/ticket/43316#comment:82

We'll make sure it's polished top to bottom before it lands in core.

@mtias

This comment has been minimized.

Contributor

mtias commented Jun 4, 2018

@adamsilverstein thanks for driving this one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment