Skip to content

Add check capability endpoint. #883

Merged
merged 2 commits into from Jul 24, 2014

6 participants

@lezama
lezama commented Jul 23, 2014

@enejb could you give it a check to the logic?
@justinshreve I am not sure what should be the path for this endpoint. What do you suggest?

@lezama lezama Add check capability endpoint.
+ Move the capability check inside `validate_call` to the
`check_capability` method.
b629eee
@georgestephanis
Automattic member

@justinshreve Also, whether this would be useful to have for wpcom sites as well, and not merely Jetpack sites. It may be useful for the API to randomly check what a user can do, so it can tailor UIs to the user.

@enejb
Automattic member
enejb commented Jul 23, 2014

I checked the logic and it makes sense. Should we check that we have at least one must pass condition ?

Also I still need to test it.

@justinshreve

@georgestephanis I think this would be good to have for both WordPress.com & Jetpack sites.

Not 100% sure what the endpoint should be...
/sites/%s/capabilities or /sites/%s/capabilities/mine ?

@georgestephanis
Automattic member

/mine matches the existing structure already for a lot of endpoints, so I'd think that ... but is there any time someone would need to query for another user's caps?

@lezama
lezama commented Jul 23, 2014

is there any time someone would need to query for another user's caps?

I can't imagine a good use case but we could allow it just for users of the same blog just in case.

@enejb
Automattic member
enejb commented Jul 23, 2014

I think an endpoint like current_user_can make sense to me. I would expect to get a list of capabilities if I went to the capabilities endpoint.

@lezama
lezama commented Jul 23, 2014

Wouldn't be more interesting to return the list of capabilities instead of doing the check endpoint side?
In order to avoid multiple requests, and easier to cache.

@georgestephanis
Automattic member
@lezama
lezama commented Jul 23, 2014

👍 good point, no need to rethink that part.

@georgestephanis
Automattic member

@lezama: What's the point of the 'must pass' number?

I think it'd make more sense if the in/out looked something like ..

in:

[
  'capability_1',
  'capability_2',
  'capability_3',
  'capability_4',
  'capability_5'
]

out:

{
  'capability_1' : true,
  'capability_2' : true,
  'capability_3' : false,
  'capability_4' : false,
  'capability_5' : true
}
@enejb
Automattic member
enejb commented Jul 23, 2014

@georgestephanis The point was that this way you can easier pass in an or condition. So out of all of these conditions we need to pass at least one. If you need to pass all of them you just leave it out.

What you are proposing sounds good. Except for that a non empty array would eventuate as true even if no conditions where passed which might be a cause of errors.

@lezama
lezama commented Jul 23, 2014

It's true that it makes harder to use the endpoint to cache individual capabilities in one request.

@georgestephanis
Automattic member

@enejb I'm actually fine with that. If you pass an array in, you should expect an array back, not just a boolean.

If you pass in just a string for a single cap, then I'm fine returning a simple boolean.

@justinshreve

but is there any time someone would need to query for another user's caps

Nope.. so I'm not sure what the root level would do if we went with /mine.

@blobaugh

Leaves room for possible future expansion

@lezama lezama REST API: Return an array of capabilities.
The endpoint can be called passing a single capability or an array of
capabilities.
In the first case, It returns a boolean indicating if the current user
has that capability.
In the second case, an array with the capabilities as keys and
current_user_can( key ) as values.
056d864
@georgestephanis
Automattic member

@lezama Looks good, but can you move it to the shared endpoints, so it can be used on wpcom as well, as per @justinshreve? Then hit that awesome Merge button! 👍

@justinshreve

I think the /me/* thing was an interesting idea. Nice. I think that should probably be /me/capabilities though (see the FieldGuide page for naming guidelines).

In addition to moving it to shared endpoints - I would change the group to 'sites'. I would also like to see a test or two for this in the current test suite.

@georgestephanis
Automattic member

Just so I can get the strings into master and the pot file, I'm going to merge this, then any further tweaks can be done in master.

@georgestephanis georgestephanis merged commit 657871f into master Jul 24, 2014

2 checks passed

Details ci/scrutinizer Scrutinizer: 1 new issues, 5 updated code elements — Tests: passed
Details continuous-integration/travis-ci The Travis CI build passed
@georgestephanis
Automattic member

cc: @lezama @justinshreve ^^ -- we can also leave it just in Jetpack until we have all the tests for wpcom correctly in place.

@georgestephanis georgestephanis deleted the add-check-capability-endpoint branch Jul 24, 2014
@justinshreve

I'd like to see tests for JP anyway though :).

@georgestephanis
Automattic member

Oh, agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.