Skip to content
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

Group membership requests and invitations update. #206

Merged
merged 8 commits into from Aug 14, 2019

Conversation

@dcavins
Copy link
Contributor

commented Aug 5, 2019

Here's an updated PR with changes to the group membership requests and invitations endpoints.

@renatonascalves renatonascalves added this to the before-bp-merge milestone Aug 5, 2019

@renatonascalves renatonascalves requested a review from imath Aug 5, 2019

@renatonascalves
Copy link
Member

left a comment

A few changes are needed here.

  • I hosnetly don't like how the permissions checks were done. Mainly the way it was put in place. The linear way used in other components, here is an example, follows a more logic path. Making it easier to see which errors might be returned first. The current implementation works but, in my opinion, it is harder to read.

  • There are a few places in the permissions checks where the errors are being returned too early. I called out some of them in my review.

  • There are still a few places where fallback values are being returned inline instead of the collections/params.

  • I'd recommend creating a helper method for this as you seem to use that a lot:

$memreqs = groups_get_requests( array( 'id' => $request['request_id'] ) );
$memreq = current( $memreqs );
  • And other minor changes.
Show resolved Hide resolved includes/bp-groups/classes/class-bp-rest-group-invites-endpoint.php Outdated
* @return WP_REST_Response|WP_Error
*/
public function create_item( $request ) {
$inviter_id_arg = $request['inviter_id'] ? $request['inviter_id'] : bp_loggedin_user_id();

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 5, 2019

Member

This fallback should be added via the params/collections as a default value.

This comment has been minimized.

Copy link
@dcavins

dcavins Aug 6, 2019

Author Contributor

The fallback only applies to the create() method. Does the WP REST API allow for defaults in one verb only?

This comment has been minimized.

Copy link
@renatonascalves
@@ -271,14 +430,69 @@ public function create_item( $request ) {
* @return bool|WP_Error
*/
public function create_item_permissions_check( $request ) {
$retval = $this->get_items_permissions_check( $request );
$inviter_id_arg = $request['inviter_id'] ? $request['inviter_id'] : bp_loggedin_user_id();

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 5, 2019

Member

This fallback should be added via the params/collections as a default value.

This comment has been minimized.

Copy link
@dcavins

dcavins Aug 6, 2019

Author Contributor

Same as above.

This comment has been minimized.

Copy link
@renatonascalves
Show resolved Hide resolved includes/bp-groups/classes/class-bp-rest-group-invites-endpoint.php Outdated
Show resolved Hide resolved includes/bp-groups/classes/class-bp-rest-group-invites-endpoint.php Outdated
Show resolved Hide resolved includes/bp-groups/classes/class-bp-rest-group-invites-endpoint.php Outdated
Show resolved Hide resolved ...es/bp-groups/classes/class-bp-rest-group-membership-request-endpoint.php Outdated
Show resolved Hide resolved ...es/bp-groups/classes/class-bp-rest-group-membership-request-endpoint.php
$group_member->is_confirmed = 0;
if ( ! $group_member->save() ) {
$user_id_arg = $request['user_id'] ? $request['user_id'] : bp_loggedin_user_id();

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 5, 2019

Member

Default values should be passed via the params/collections.

Show resolved Hide resolved ...es/bp-groups/classes/class-bp-rest-group-membership-request-endpoint.php Outdated
@dcavins

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Hi @renatonascalves- Thanks again for the review. I'm sorry you don't like the permissions check. I went away from the true && checks because I found them confusing and hard to read, unlike an if/else string which is easy to follow. However, if you prefer the other method, I can rework the checks again, since the REST API is really your baby. :) Thanks again!

@renatonascalves

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@dcavins If you feel strongly, keep it! Maybe only I have trouble with it! I can bet others will like the if/else pattern.

@dcavins

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Nope! These endpoints should match the code style of the rest of the REST API. :)

@renatonascalves
Copy link
Member

left a comment

Can I ask you to update all hooks that are using bp_rest_group_membership_request into bp_rest_group_membership_requests?

Also, a few more changes. :)

@renatonascalves
Copy link
Member

left a comment

Good for me! :)

@imath
Copy link
Contributor

left a comment

Ok! Thanks a lot for your work on this @dcavins and @renatonascalves 💪

I just woke up and think we are forgetting some cases and we should think about what verb perform what action.

See my comments about groups_join_group() and groups_reject_invite() etc.. Let's talk about it during dev-chat 😉

@renatonascalves

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Rerun Travis and tests are passing now. :)

@imath

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Thanks ! I think we should rebase & merge and improve it if we need to later. Any objections if I do so in the coming hours?

@renatonascalves

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@imath not at all! As soon as it is in, the better! :)

dcavins added some commits Aug 5, 2019

Update Group Invitation endpoint for Invites API.
The new Invitations API in BuddyPress enabled some simplfication and expansion of the group invitations endpoint. Here are the possible variations of this endpoint:

Fetch invitations to a specific group (must be group admin or site admin):
GET /wp-json/buddypress/v1/groups/invites/?group_id=3

Fetch invitations to a specific user (must be the user or site admin):
GET /wp-json/buddypress/v1/groups/invites/?user_id=3

Fetch invitations from a specific user (must be the user or site admin):
GET /wp-json/buddypress/v1/groups/invites/?inviter_id=3

Find an invitation by a mix of its parameters (must be the inviter, invitee, group admin or site admin):
GET /wp-json/buddypress/v1/groups/invites/?group_id=3&user_id=2
GET /wp-json/buddypress/v1/groups/invites/?group_id=3&inviter_id=6
GET /wp-json/buddypress/v1/groups/invites/?group_id=3&user_id=2&inviter_id=6

Send an invitation to a user from a group, setting the current user as the inviter:
POST /wp-json/buddypress/v1/groups/invites/?group_id=3&user_id=2

Fetch an invitation by ID:
GET /wp-json/buddypress/v1/groups/invites/{invite_id}

Accept an invitation (must be invitee or site admin):
PUT /wp-json/buddypress/v1/groups/invites/{invite_id}

Delete an invitation (must be invitee, inviter, group admin or site admin ):
DELETE /wp-json/buddypress/v1/groups/invites/{invite_id}
Update Group Membership Request endpoint for Invites API.
The new Invitations API in BuddyPress enabled some simplfication and expansion of the group membership requests endpoint. Here are the possible variations of this endpoint:

Fetch membership requests to a specific group (must be group admin or site admin):
GET /wp-json/buddypress/v1/groups/membership-requests/?group_id=3

Fetch membership requests from a specific user (must be the user or site admin):
GET /wp-json/buddypress/v1/groups/membership-requests/?user_id=3

Find a membership request by a mix of its parameters (must be the requestor, group admin or site admin):
GET /wp-json/buddypress/v1/groups/membership-requests/?group_id=3&user_id=2

Send a membership request to a group:
POST /wp-json/buddypress/v1/groups/membership-requests/?group_id=3&user_id=2

Fetch a membership request by ID:
GET /wp-json/buddypress/v1/groups/membership-requests/{invite_id}

Accept a membership request (must be an admin of the group or a site admin):
PUT /wp-json/buddypress/v1/groups/membership-requests/{invite_id}

Delete an invitation (must be requestor, group admin or site admin ):
DELETE /wp-json/buddypress/v1/groups/membership-requests/{invite_id}
Invites/requests endpoint: Updates from Renato's review.
• Use `bp_rest_group_membership_requests` not `bp_rest_group_membership_request` in hook and error IDs.
• Replace `$memreq` with variable names `$group_requests` for group membership requests.
• Change signature of helper function fetch_single_invite() to accept an ID.
• Fix documentation errors.

@imath imath force-pushed the dcavins:invites-aug4 branch from c50fc62 to ede55bd Aug 14, 2019

@imath imath merged commit a1106a8 into buddypress:master Aug 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@renatonascalves renatonascalves added this to Done in BP REST - V1 Aug 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.