Skip to content
This repository has been archived by the owner on Apr 4, 2020. It is now read-only.

Added User Meta Endpoints #3

Merged
merged 11 commits into from
Feb 25, 2016

Conversation

kjbenk
Copy link
Contributor

@kjbenk kjbenk commented Feb 22, 2016

I wanted to learn more about the Rest API so I decided to take a crack at adding the User meta endpoints based on this issue #1. I love how you made the WP_REST_Meta_Controller class by the way.

One thing that I was not sure about how to do was the check_read_permission authorization check. This is just a different use case from posts I think since we are displaying user data rather than post data, so wouldn't that data never be shown to logged out users? @danielbachhuber...or am I just overthinking it :)

For now the check_read_permission function is commented out with @todo placed above it. I have not added in the unit tests yet.

* ADDED: User meta endpoints
@kjbenk
Copy link
Contributor Author

kjbenk commented Feb 23, 2016

I noticed that I made a simple mistake in this PR with the permissions check function. I will be posting an update later today to fix it.

* FIXED: The check for user permissions to access or edit the user has been fixed.
/**
* Associated object type.
*
* @var string Type slug ("post", "user", or "comment")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment shouldn't need the "("post", "user", or "comment")"

* ADDED: Basic units tests for the user meta endpoint
* FIXED: Changed user meta permission check caps
$request = new WP_REST_Request( 'GET', sprintf( '/wp/v2/users/%d/meta', $user_id ) );
$response = $this->server->dispatch( $request );

$this->assertEquals( 200, $response->get_status() );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line that is failing to pass in travis. I am not 100% sure why it is failing since it is passing on my local install via VVV.

Failed: https://travis-ci.org/WP-API/wp-api-meta-endpoints/jobs/111276918#L300

@danielbachhuber thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

am not 100% sure why it is failing since it is passing on my local install via VVV.

How would it be passing locally?

In this test, you're requesting user meta in an unauthenticated request. I'd expect the test to fail with a 403 response, which it does locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see why it would be passing locally. For some reason when I use the $this->factory->user->create() function I create a new temp user with the ID of 2, but my local admin user has an ID of 2. I am not 100% sure if this is effecting it but I just looked over the way you test in the wp-api repo and I think I understand how to authenticate properly now.

https://github.com/WP-API/WP-API/blob/develop/tests/test-rest-users-controller.php#L74

* FIXED: Unit testing did not perform the user authentication correctly
@kjbenk kjbenk mentioned this pull request Feb 23, 2016
if ( ! $this->parent_controller->check_read_permission( $parent ) ) {
return new WP_Error( 'rest_forbidden', __( 'Sorry, you cannot view this user.' ), array( 'status' => rest_authorization_required_code() ) );
}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this commented code?

@danielbachhuber
Copy link
Member

Couple of small comments. Looks good otherwise though.

@danielbachhuber danielbachhuber added this to the 0.2.0 milestone Feb 24, 2016
danielbachhuber added a commit that referenced this pull request Feb 25, 2016
@danielbachhuber danielbachhuber merged commit ba7d709 into WP-API:master Feb 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants