Revisions #5: Add support for page postTypes

[bperson]

Note: This builds on top of #14927, so it's not merge-able yet

5th chunk coming from my WIP PR to introduce post revisions in calypso: #13367. This simply adds support for pages. It's mostly code to hit a different endpoint based on the postType.

testing

• The unit tests for the data-layer part have been updated to test this use case as well
• Manual testing: Tap on "x revisions" in the post editor sidebar of a page (shown only on pages with multiple revisions)

[aduth]

Does the revisions endpoint not support custom post types? Ideally we'd not be tied to posts and pages only. These are mistakes we'd made early in both the REST API and Calypso that would be good to avoid moving forward. Would GET /sites/%s/posts/%d/revisions return anything if the ID passed was one for a custom post type?

[aduth]

Typo: ressourceName -> resourceName

[bperson]

french sneaking in, it should be fixed ;)

[bperson]

Does the revisions endpoint not support custom post types? Ideally we'd not be tied to posts and pages only.

it looks like there is support for some post types.

Would GET /sites/%s/posts/%d/revisions return anything if the ID passed was one for a custom post type?

Nope, but it does work (for some post types) if you use the normalised post type resource name [0]: /sites/%s/<normalised post type>/%d/revisions. For instance, it works with jetpack-testimonial but not with media (it's the normalised post type resource name for the attachment post type).

[0] I can't find logic in calypso that would do what's done in the REST API to compute the resource name though:

$this->parent_base = ! empty( $post_type_object->rest_base ) ? $post_type_object->rest_base : $post_type_object->name;

The issue comes from the fact we populate postTypes in Redux from old endpoints which won't return rest_base which is used to compute the resource name.

I ideally would like to not upgrade the code fetching post types to v2, as it sounds like a deep rabbit hole, so there are 2 options I think:
1- disable the revisions UI for custom post types
2- play it risky: const resourceName = get( { page: pages, post: posts }, postType, postType )

[aduth]

Interesting observations. I wonder how common it is to rename rest_base. Regardless, you're right to point out that it's something we'd want to account for. Throwing out a few other ideas:

• Include optional flag on <QueryPostTypes /> that invokes an additional request to the wp/v2 post types endpoint to determine rest_base
  • State entity would include data from multiple sources; in fairness we don't want to tie entities to particular API responses anyways. Multiple API requests, complex logic to manage
• Include rest_base in the WordPress.com post types response
  • Might require Jetpack changes and therefore version minimum, awkward to have the .com endpoints returning information about the .org equivalents
• "play it risky", but at least account for the failure case gracefully
  • Detect the 404 and disable revisions UI when encountered?

Porting post types to wp/v2 would indeed be a rabbit hole, since the .com endpoints use a different criteria for determining whether a post type should be made available on API responses.

Enabling the UI only for posts and pages is an option, though not ideal per my previous comment.

[bperson]

Detect the 404 and disable revisions UI when encountered?

Right now, with #14927, we fetch the revisions when the UI gets rendered which makes it impossible to disable the revision UI as a result of a 404, at best, we can display a button redirecting to wp-admin or an error message.

That said, I think it's safer for now to only enable it for posts/pages, leave a NOTE/TODO comment (or a GH issue?) to revisit it with a bit more mileage on the revisions UI. It makes the UX consistent for all custom types, without having to explain in an error message why some are failing and redirecting the user to wp-admin due to an hidden parameter.

[aduth]

Even if we don't support custom post types quite yet, is there a way we could express this to be less tied to posts and pages? If we hypothetically had access to rest_base, how would we want this data handler and the associated action creator to be written?

[bperson]

• associated action creator: nothing? postType is the most suited parameter to transmit that data in the action imho: using the rest_base in the action would leak a "data-layer" concept + postType is widely used in the codebase.

• data handler:

export const fetchPostRevisions = ( { dispatch, getState }, action ) => {
	const { siteId, postId } = action;
	const { siteId, postId, postType } = action;
	const resourceName = getPostType( getState(), siteId, postType ).rest_base
	(...)
} );

[bperson]

Now that #14943 is merged, CircleCI is green, it's also rebased on the latest master

[aduth]

Minor note, but this is looking good.

[aduth]

Passing this as a prop is arguably a bit redundant, because siteId and postId props should be sufficient for the component to be able to look up this information from state on its own via connect. EditorDocumentHead is similar here (though more extreme in that it doesn't accept any props) where it determines the type using the getEditedPostValue selector.

[bperson]

interesting, it's updated (I also updated #14927 to load siteId and postId from state).

[matticbot]

@bperson This PR needs a rebase

[matticbot]

@bperson This PR needs a rebase