Introduce a dedicated autosaves endpoint for handling autosave behavior by adamsilverstein · Pull Request #6257 · WordPress/gutenberg

adamsilverstein · 2018-04-18T17:35:35Z

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.

…hed posts

…f autosave status

# Conflicts: # editor/store/effects.js

adamsilverstein · 2018-04-18T18:31:56Z

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

danielbachhuber

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

danielbachhuber · 2018-04-23T12:08:18Z

editor/store/actions.js

 }

-export function savePost() {
+export function savePost( options ) {


Do these options merit a doc block?

danielbachhuber · 2018-04-23T12:09:28Z

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

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


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

changed to GB_REST_Autosaves_Controller

danielbachhuber · 2018-04-23T12:12:18Z

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

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


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

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

danielbachhuber · 2018-04-23T12:15:30Z

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

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


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.

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.

danielbachhuber · 2018-04-23T12:17:01Z

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

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


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

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

Was this addressed @danielbachhuber ?

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

danielbachhuber · 2018-04-23T12:18:29Z

lib/register.php

+	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 );


Can we include the PHPUnit tests for this controller too?

sure assuming its straightforward to bring them over from core.

danielbachhuber · 2018-04-23T13:08:03Z

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)?

…o avoid conflict with core class

Also eliminate possible race condition on autosave effecting selector prior to resolved evaluation.

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.

Only those used in isEditedPostAutosaveable implementation

Since already checking isEditedPostSaveable, which returns false if save (including autosave) is in progress

Consolidate to getEditedPostAttribute as canonical source of fields values

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 · 2018-05-25T22:14:36Z

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:

Consolidate "saving" and "autosaving" behaviors, both in effect handling and in representation by state / selectors (where autosave is reflected as a superset of the condition of saving).
Restore (and resolve failings of) immediate autosave dispatch in end-to-end tests.
- I did observe a test failure which resolved itself after restarting build. Related: Intermittent e2e test failures #6956
Simplify representation of autosave object in state. It appears we only ever consider its title, content, and excerpt, so we don't need to track the entire post object.
Add missing tests.
Fixes.
- Prior to these changes it was possible to Cmd+S to save a new post, which would fail. (End-to-end test case added)
- Prior to these changes it was possible to Cmd+S to save a post which already had been saved, which would succeed but is not an intended supported behavior (and did not exist previously). (End-to-end test case added)
Marked TODO where I observed issues but did not introduce changes (to avoid scope creep)
Open questions:
- What's the relationship between autosave and saving meta box values? This has not been changed from its previous logic, where metabox updates trigger in response to the start of a save request (including autosave).

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

#6257 (comment)

adamsilverstein · 2018-05-29T13:42:16Z

@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 · 2018-05-31T00:17:39Z

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

Fixed in 0bec4ea

These ended up on the wrong side of the merge conflict 0bec4ea

danielbachhuber · 2018-05-31T01:10:57Z

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

adamsilverstein · 2018-05-31T03:58:16Z

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

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

aduth · 2018-05-31T16:56:28Z

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

Introduce a dedicated autosaves endpoint for handling autosave behavior #6257 (comment)

danielbachhuber · 2018-05-31T16:59:55Z

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 · 2018-06-04T09:17:59Z

@adamsilverstein thanks for driving this one!

adamsilverstein added 15 commits April 17, 2018 08:28

autosaves with rest controller, first pass

9b41d7c

use newer title call

004c830

autosave: compare raw attributes for change check

9b0a3a4

Clean up autosave effects

b4ad672

pull isAutosaving from props in PostSavedState

aba8548

Merge branch 'master' of github.com:WordPress/gutenberg

c6d5cce

Merge branch 'master' of github.com:WordPress/gutenberg

345a508

from .14

bb99f4c

effects, improve toggle autosave timing, enable autosaving for publis…

5e85a53

…hed posts

Disable ‘save draft’ after draft is autosaved

491ce55

revert post saved state changes, keep save button active regardless o…

17a67f6

…f autosave status

remove unrelated showAutosaveAlert

81ca0de

fixes for eslint

4da3ef1

leverage withChangeDetection for autosaves

b189a3c

Merge branch 'master' into fix/autosaves

7769231

# Conflicts: # editor/store/effects.js

adamsilverstein requested review from aduth and azaozz April 18, 2018 17:40

fixes for php slint

d4400a9

pento mentioned this pull request Apr 19, 2018

Preview page randomly wont display #4625

Closed

danielbachhuber self-requested a review April 19, 2018 21:43

pento mentioned this pull request Apr 21, 2018

Preview for published posts not working when editing #6331

Closed

Merge branch 'master' of github.com:WordPress/gutenberg

ab44c63

danielbachhuber reviewed Apr 23, 2018

View reviewed changes

adamsilverstein added 5 commits April 23, 2018 16:23

Merge branch 'master' of github.com:WordPress/gutenberg

5b0e36d

Merge branch 'master' into fix/autosaves

949557e

rename WP_REST_Autosaves_Controller -> GB_REST_Autosaves_Controller t…

e4d20ff

…o avoid conflict with core class

Add a docblock for savePost action

0833e15

check state.autosave exists before using

6b07e56

aduth added 6 commits May 25, 2018 17:13

Testing: Evaluate autosave completion by class presence

8d5dcf0

Also eliminate possible race condition on autosave effecting selector prior to resolved evaluation.

Keyboard Shortcuts: Disallow non-dirty save requests

83ad219

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

4821f49

Only those used in isEditedPostAutosaveable implementation

State: Remove redundant condition from autosaveable

2dfc666

Since already checking isEditedPostSaveable, which returns false if save (including autosave) is in progress

State: Simplify isEditedPostAutosaveable as Array#some of fields

102b944

Consolidate to getEditedPostAttribute as canonical source of fields values

aduth mentioned this pull request May 30, 2018

Data Module: Add support for action creators as generators #6999

Closed

Merge branch 'master' into fix/autosaves

0bec4ea

danielbachhuber added 2 commits May 30, 2018 17:50

Remove tests originally removed in bdcbf71

f3e564a

These ended up on the wrong side of the merge conflict 0bec4ea

Fix linting

11f9930

danielbachhuber merged commit 5a6c24e into master May 31, 2018

danielbachhuber deleted the fix/autosaves branch May 31, 2018 01:11

This was referenced May 31, 2018

Editing a post in Gutenberg generates too many revisions #3656

Closed

State: Avoid URL redirect for published post autosave #7067

Merged

adamsilverstein mentioned this pull request Jun 1, 2018

Autosave has a blank title #7078

Closed

aduth mentioned this pull request Jun 6, 2018

Preview: Enable published posts preview #7189

Merged

danielbachhuber mentioned this pull request Jun 20, 2018

Possibility to filter $editor_settings in /lib/client-assets.php #7400

Closed

aduth mentioned this pull request Jun 25, 2018

Only save metaboxes when it's not an autosave #7502

Merged

4 tasks

ellatrix mentioned this pull request Oct 9, 2018

Performance: save components are called for *every* block on *every* keystroke #10427

Closed

aduth mentioned this pull request Oct 10, 2018

Remove autosaveable check from autosave monitor #10431

Closed

4 tasks

aduth mentioned this pull request Nov 12, 2018

REST API: Avoid resetting parent field in draft update #11513

Merged

Conversation

adamsilverstein commented Apr 18, 2018 • edited by danielbachhuber Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

adamsilverstein commented Apr 18, 2018

Uh oh!

danielbachhuber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielbachhuber commented Apr 23, 2018

Uh oh!

aduth commented May 25, 2018

Uh oh!

adamsilverstein commented May 29, 2018

Uh oh!

danielbachhuber commented May 31, 2018

Uh oh!

danielbachhuber commented May 31, 2018

Uh oh!

adamsilverstein commented May 31, 2018

Uh oh!

aduth commented May 31, 2018

Uh oh!

danielbachhuber commented May 31, 2018

Uh oh!

mtias commented Jun 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adamsilverstein commented Apr 18, 2018 •

edited by danielbachhuber

Loading