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

Use `targetSchema` of JSON Hyper Schema to communicate sticky action #6529

Merged
merged 7 commits into from May 7, 2018

Conversation

Projects
None yet
7 participants
@danielbachhuber
Member

danielbachhuber commented May 1, 2018

Description

Updates the post-sticky check to use presence of wp:action-sticky in _links to determine whether the sticky UI should display. This system is called targetSchema. It allows us to compute whether a user can perform an action server-side, and then communicate it to the client.

See #6361 (comment)

How has this been tested?

For editors and above, the "Stick to the Front Page" UI should display:

image

For authors and below, it should not:

image

User Switching is a helpful plugin for quickly testing this.

Checklist:

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

@danielbachhuber danielbachhuber added this to the 2.8 milestone May 1, 2018

@danielbachhuber danielbachhuber requested review from pento and WordPress/gutenberg-core May 1, 2018

<PostStickyCheck postType="page" post={ {
_links: {
self: {
href: 'https://w.org/wp-json/wp/v2/posts/5',

This comment has been minimized.

@kadamwhite

kadamwhite May 2, 2018

Contributor

Should this be /page/5 if postType="page"? (Doesn't change test result, admittedly)

This comment has been minimized.

@danielbachhuber
},
'wp:action-sticky': {
href: 'https://w.org/wp-json/wp/v2/posts/5',
},

This comment has been minimized.

@kadamwhite

kadamwhite May 2, 2018

Contributor

Once again the test works as-is because of what we're testing for, but all of the keys on the _links object hold arrays of objects, instead of plain objects
(e.g. wp:action-sticky': [ { /* ... */ } ]).

This comment has been minimized.

@danielbachhuber
),
),
),
),

This comment has been minimized.

@kadamwhite

kadamwhite May 2, 2018

Contributor

We have this issue all over the API, but this is a lot of data to be piling into the response just to indicate whether the UI can take some action. Do we really want to be repeating all of this:
image
for each post every time we're in a collection where we might possibly want to sticky something? Our _links object is getting quite heavy.

I don't see a better option so I'm 👍 on the implementation, but I think we'll need to come back to that at some point.

This comment has been minimized.

@TimothyBJacobs

TimothyBJacobs May 2, 2018

Contributor

If we ever land generating a URL to access a schema, the targetSchema section could be replaced with a $ref for the property.

This comment has been minimized.

@TimothyBJacobs

TimothyBJacobs May 2, 2018

Contributor

We could also remove the description property but it'd make the link less helpful for clients.

@@ -27,14 +23,8 @@ export function PostStickyCheck( { postType, children, user } ) {
export default compose( [
withSelect( ( select ) => {
return {
post: select( 'core/editor' ).getCurrentPost(),

This comment has been minimized.

@gziolo

gziolo May 2, 2018

Member

It would be better to return only:

hasStickyAction: get( post, [ '_links', 'wp:action-sticky' ], false )`

from withSelect to avoid unnecessary rerenders when the current post changes after unrelated modifications.

In general, it is recommended to pass down as props only what is really used inside the component.

This comment has been minimized.

@danielbachhuber

danielbachhuber May 2, 2018

Member

Good catch, fixed in 3e591c5

Use `targetSchema` of JSON Hyper Schema to communicate sticky action
Because WordPress' capabilities system is filterable, we need to execute
the capabilities before we know whether the current user can perform the
action. Fortunately, `targetSchema` supports exactly this use-case.

@danielbachhuber danielbachhuber force-pushed the 6361-post-sticky branch from 18e815f to e2bf660 May 2, 2018

@danielbachhuber danielbachhuber requested a review from gziolo May 2, 2018

// Only Posts can be sticky.
if ( 'post' === $post->post_type ) {
if ( current_user_can( $post_type->cap->edit_others_posts )
&& current_user_can( $post_type->cap->publish_posts ) ) {

This comment has been minimized.

@felixarntz

felixarntz May 3, 2018

Member

Why is the capability for publishing posts required to make a post sticky?

More importantly though, this should use the meta capabilities for the specified post and rely on map_meta_cap() instead of handling the logic manually:

if ( 'post' === $post->post_type ) {
    if ( current_user_can( 'edit_post', $post->ID ) && current_user_can( 'publish_post', $post->ID ) {
        // Add link.
    }
}

This comment has been minimized.

@danielbachhuber

danielbachhuber May 3, 2018

Member

Why is the capability for publishing posts required to make a post sticky?

I'm using the check within edit_post() as my point of reference: https://github.com/WordPress/WordPress/blob/3266b10d04ad43b9d983e0b55d4d6247be33e18a/wp-admin/includes/post.php#L401

More importantly though, this should use the meta capabilities for the specified post and rely on map_meta_cap() instead of handling the logic manually.

I'm not sure I follow. Can you explain why?

This comment has been minimized.

@felixarntz

felixarntz May 3, 2018

Member

I'm using the check within edit_post() as my point of reference: https://github.com/WordPress/WordPress/blob/3266b10d04ad43b9d983e0b55d4d6247be33e18a/wp-admin/includes/post.php#L401

Okay, I didn't know this was previously being used for it. I'm not sure that check makes sense though even in this area.
Basically, any action you perform to a specific item (like a post) should go through a meta capability check (using a singular noun, like edit_post or publish_post). Logic to resolve these to their primitive required capabilities is in place in the map_meta_cap() function, and developers rely on this functionality, for example to revoke that permission from a user (for example for one specific post).
Therefore I think the check in edit_post() is not accurate either. For this very case, there's no appropriate capability yet, so I'd recommend to introduce stick_post and unstick_post meta capabilities and add a clause for them in map_meta_cap(). We can then resolve it to the primitive capabilities we want to in there (if those are decided to be what currently happens in edit_post(), that's alright I guess). The benefit is that we then don't have to memorize all over what to check for when making a post sticky or unsticky, we can simply use intuitive stick_post / unstick_post, both in the current backend, the REST API, anywhere.

This comment has been minimized.

@danielbachhuber

danielbachhuber May 3, 2018

Member

Therefore I think the check in edit_post() is not accurate either. For this very case, there's no appropriate capability yet, so I'd recommend to introduce stick_post and unstick_post meta capabilities and add a clause for them in map_meta_cap(). We can then resolve it to the primitive capabilities we want to in there (if those are decided to be what currently happens in edit_post(), that's alright I guess). The benefit is that we then don't have to memorize all over what to check for when making a post sticky or unsticky, we can simply use intuitive stick_post / unstick_post, both in the current backend, the REST API, anywhere.

Does what you describe need to be solved for in this pull request?

This comment has been minimized.

@pento

pento May 4, 2018

Member

Given that only the post post type can be sticky, I don't think there's a need to add these special meta capabilities.

@aduth aduth modified the milestones: 2.8, 2.9 May 3, 2018

@pento

I've been letting this tumble around for a bit, and I have a few concerns.

First off, I like the idea of providing information in the form of "this is an action the current user can perform, and here are the properties that action relates to". This is useful information to provide.

As has been mentioned, this solution seems quite verbose. I guess a major part of that is the description being on the post object, instead of in the schema, where all the other descriptions are. It's also duplicating the description of the sticky property, rather than documenting what action-sticky is for.

On that note, the naming is quite obtuse, too, it took me way too long to understand how the data is supposed to be used. There's nothing to indicate that "the presence of wp:action-sticky mean you should render the Sticky Post UI".

I think this would be better in a sibling property to _links, _actions, or something like that. _links is entirely used as a list of shortcuts to related information about the current object, adding in "here is an action the current user can perform" is confusing.

// Only Posts can be sticky.
if ( 'post' === $post->post_type ) {
if ( current_user_can( $post_type->cap->edit_others_posts )
&& current_user_can( $post_type->cap->publish_posts ) ) {

This comment has been minimized.

@pento

pento May 4, 2018

Member

Given that only the post post type can be sticky, I don't think there's a need to add these special meta capabilities.

@kadamwhite

This comment has been minimized.

Contributor

kadamwhite commented May 4, 2018

I think this would be better in a sibling property to _links, _actions, or something like that. _links is entirely used as a list of shortcuts to related information about the current object, adding in "here is an action the current user can perform" is confusing.

This would have the added benefit that such information could be omitted using _fields when the client does not need to know about actions

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 4, 2018

I think this would be better in a sibling property to _links, _actions, or something like that. _links is entirely used as a list of shortcuts to related information about the current object, adding in "here is an action the current user can perform" is confusing.

Can you point me to the existing spec where this is documented?

@pento

This comment has been minimized.

Member

pento commented May 4, 2018

Can you point me to the existing spec where this is documented?

It isn't documented as far as I'm aware, this is just how _links seems to be used in the core REST API. It's suggested by the name, too: a link is something you click (or in HTTP verbs, something you GET), it's not a thing that you POST.

As far as adhering to the JSON API spec goes, I'm fine with using something that exists within the spec, if that fits our requirements. If not, I'm 💯 in favour of doing something that works, rather than something that's shoe-horned into a vaguely related structure.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 4, 2018

It isn't documented as far as I'm aware, this is just how _links seems to be used in the core REST API.

_links is an implementation of HAL. targetSchema is a part of the current JSON Hyper-Schema spec.

If not, I'm 💯 in favour of doing something that works, rather than something that's shoe-horned into a vaguely related structure.

I'm not particularly interested in spending a ton of time bikeshedding an alternative spec. Can you describe the alternative implementation you'd accept?

cc @TimothyBJacobs for any thoughts he has.

@pento

This comment has been minimized.

Member

pento commented May 4, 2018

Sure, here's a rough idea of something I think would be extensible, would clearly show the difference between _links and _actions.

In the schema, _actions would be defined as an array of all the possible actions, with a description of what they're for, and what they're related to, like so:

{
	"routes": {
		"/wp/v2/posts/(?P<id>[\\d]+)": {
			"endpoints": [
				{
					"methods": [ "POST", "PUT", "PATCH" ],
					"args": {
						// ...
					},
					"_actions": {
						"wp:sticky": {
							"title": "Sticky Post",
							"description": "Whether the current user can sticky the post or not",
							"targetSchema": {
								// Something in here about being related to the "sticky" property.
							}
						}
					}
				},
			],
		},
	},
}

Then, in the post object, the _actions property would define whether each action is available or not:

{
	"id": 1,
	// ...
	"_actions": {
		"wp:sticky": true,
	},
}

Alternatively, _actions could be an array of available actions, the difference seems to mostly be a choice of style:

{
	"id": 1,
	// ...
	"_actions": [
		"wp:sticky",
	],
}
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 4, 2018

Again, I'm strongly opposed to inventing a new spec when the existing spec is sufficient. In inventing a new spec, we'll need to go through multiple iterations of relearning past mistake/decision cycles. This doesn't seem like a judicious use of our time.

To address response size concerns, what if we only included these action links if context=edit?

@TimothyBJacobs

This comment has been minimized.

Contributor

TimothyBJacobs commented May 4, 2018

I agree with @danielbachhuber. I don't see why including them in a separate "magic" field makes much sense. As I mentioned earlier, we can definitely omit the description fields which would considerably reduce the response size.

We've talked in #core-restapi a few times about adding links to the actual schemas. We could revisit this and try to land it for 5.0. This would allow us to replace the entire property description with a $ref like "$ref": "https://example.org/wp-json/wp/v2/?_method=OPTIONS&context=schema#/properties/sticky"

I also think including the links only if the context=edit makes a lot of sense as well. However, these can be added to the $schema document like this:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "title": "post",
  "type": "object",
  "properties": {
    "date": {
      "description": "The date the object was published, in the site's timezone.",
      "type": "string",
      "format": "date-time",
      "context": [
        "view",
        "edit",
        "embed"
      ]
    }
  },
  "links": [
    {
      "rel": "https://api.w.org/action-sticky",
      "href": "https://actualwebsite.com/wp-json/wp/v2/posts/{id}",
      "targetSchema": {
        "type": "object",
        "properties": {
          "sticky": {
            "$ref": "#/properties/sticky"
          }
        }
      }
    }
  ]
}

Our response could possibly then look something like.

{
  "_links": {
    "self": [],
    "wp:action-sticky": [
      {
        "href": "https://w.org/wp-json/wp/v2/posts/5"
      }
    ]
  }
}

However, we are not currently using this pattern anywhere else and we are introducing concepts like URI Templates and JSON Pointers, which while I'd be in favor of the API supporting, are not things we have looked at before.

@pento

This comment has been minimized.

Member

pento commented May 5, 2018

To address response size concerns, what if we only included these action links if context=edit?

Yah, that'd be fine. I don't think there's a need for them in other contexts, we can always add it to them if there is. That's not really the issue, though.

The specific concern I have is that, when reading these new links, there's nothing to suggest what they mean, or how you use them. The same problem exists in @TimothyBJacobs' example: there's nothing to suggest what wp:action-sticky means, someone might guess it's related to the https://api.w.org/action-sticky thing in the schema, but I wouldn't rely on that. Even if they do, there's still nothing to tell them what it actually means.

The problem we need to solve is that there are pieces of UI that should only be displayed when a certain set of conditions are met: we want to be calculating those conditions on the server, rather than leaving it up to the client. targetSchema is nice, but it's a super complex way making available what are essentially a list of flags.

So, here's what I would like to see:

  • The post object returning a list of these flags.
  • The schema containing a description of the list, and of each flag.

_actions happens to be the name I gave that list in my earlier example, but I have no attachment to that: if there's a spec that more readily fits this, then use that. If targetSchema can be adjusted to do those things, then use it.

If there isn't a spec, then we can just make something that works, and iterate it on it later if needed.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 5, 2018

@pento I'm not sure I follow your specific recommendation. What do you think about cce57b8? Or, if there's other changes you'd like to make, can you communicate them in a pull request against this branch?

@TimothyBJacobs

This comment has been minimized.

Contributor

TimothyBJacobs commented May 5, 2018

I disagree. If you understand what targetSchema means, then it is immediately obvious what is going on. Gutenberg is filled with dozens of these technical hurdles that aren't immediately obvious to WordPress developers until you understand what a HoC is, or Slot & Fill, etc... The same thing is true with a number of patterns in WordPress core as well.

I also disagree that it is super complex. Its a fairly pattern of representing what the target resource looks like and is certainly no more complex than many other G7g patterns that are necessary to fulfill a complex requirement.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 5, 2018

To clarify my own position: I'm open to being convinced there is an implementation superior to that proposed on this pull request. However, at this point, I'm unclear as to what the alternative implementation is, and how it's functionally better.

From my perspective, the targetSchema implementation on this pull request is correct because:

  1. It conveys the information needed by the client.
  2. It follows an existing, documented spec that much of the REST API already follows.
@TimothyBJacobs

This comment has been minimized.

Contributor

TimothyBJacobs commented May 5, 2018

In case it wasn’t clear, I of course have no issue with some other mechanism being used to indicate what actions are available, but I very much do not want to see WordPress adopt another standard in a weird way that isn’t really applicable and break the purpose of it being standard without a very good reason. And I don’t think that targetSchema is complicated enough to warrant that breakage.

@pento pento referenced this pull request May 7, 2018

Merged

Tweak targetSchema Response #6613

@pento

This comment has been minimized.

Member

pento commented May 7, 2018

Thank you for the discussion, @danielbachhuber and @TimothyBJacobs. cce57b8 is a step in a good direction. I've opened #6613 with some small tweaks, but I think we can go with this.

We're not introducing meta caps at this point.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 7, 2018

Thanks @pento.

@danielbachhuber danielbachhuber merged commit 4dc4c7b into master May 7, 2018

2 checks passed

codecov/project 44.19% (+0.23%) compared to 63cf515
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the 6361-post-sticky branch May 7, 2018

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