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

Invites update #197

Closed
wants to merge 22 commits into from

Conversation

@dcavins
Copy link
Contributor

commented Jul 25, 2019

Hello all-

I had to do some updates to make the REST API not throw errors with the new BP_Invitations changes I'm proposing. In doing so, I realized that the endpoints could use some expansion to cover the typical use cases:
Group admin wants to view outstanding invitations to a specific group.
User wants to view/accept/reject his incoming group invitations (to any group).
User wants to view/create/delete his sent group invitations (from any group).

Group admins want to view/approve/reject pending membership requests (for a specific group).
User wants to view/create/delete membership requests (to any group).

Add tests cases to cover each of these scenarios.

The commits do accomplish those changes. I changed the endpoints pretty significantly, though, so I hope the changes will meet with your approval.

One format thing that appears to be normal across REST endpoints, but seemed odd to me is that you can fetch a single invitation at GET:buddypress/v1/groups/invites/{invite_id} but when you change the verb to PUT or DELETE that {invite_id} position becomes {user_id}. I'd really prefer it stay {invite_id} for every verb, and allow deletion or acceptance by invite_id or by specifying parameters group_id, user_id, and optionally inviter_id. It would be some work to revisit the logic, but the new classes make finding invitations to work with much easier, and it's way easier to change the incoming parameters before it has been released to the public. :)

Please let me know what you think of my changes and also what you think of my suggestion. I'd be happy to make the changes.

Note that these changes require a BP installation that has my latest changeset from #6210 applied.

Thanks!

@imath

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Thanks @dcavins I've tried to test your PR but it seems you haven't rebased it regularly to latest version of master during your work. You missed these 26 commits:

Could you rebase it to latest master and force the push of the PR ?

I've done it locally so I'll be able to test, but if we want to merge I think it would be better.

@imath
Copy link
Contributor

left a comment

Thanks a lot for your hard work on the PR @dcavins

I've added some questions into the code you're suggesting. And I advise you to run the full test suite because there are 3 failing tests:

There were 3 failures:

1) BP_Test_REST_Group_Invites_Endpoint::test_create_item
Failed asserting that false is true.

/pathto/bp-rest/tests/groups/test-group-invites-controller.php:347

2) BP_Test_REST_Group_Invites_Endpoint::test_create_item_as_group_admin
Failed asserting that false is true.

/pathto/bp-rest/tests/groups/test-group-invites-controller.php:372

3) BP_Test_REST_Group_Invites_Endpoint::test_create_item_invalid_group_id
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'bp_rest_group_invalid_id'
+'bp_rest_authorization_required'

/pathto/tests/phpunit/includes/testcase-rest-api.php:11
/pathto/bp-rest/tests/groups/test-group-invites-controller.php:459
@imath

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

About your suggestion, when you say:

It would be some work to revisit the logic, but the new classes make finding invitations to work with much easier, and it's way easier to change the incoming parameters before it has been released to the public

Do you think it can be done before our next dev-chat (August 7th) ?

dcavins added some commits Jun 27, 2019

Update logic for new group invites API.
BP_REST_Group_Invites_Endpoint is not found when BP_REST_Group_Membership_Request_Endpoint is being instantiated?
Change group membership requests endpoint.
Change membership requests logic to ensure:
• Users can create membership requests.
• The requester and the target group's site admins can delete the request.
• Group admins can accept requests.
Change group invitations endpoint.
Change invitation logic to ensure:
• Users can invite user to group.
• Invitee or inviter can delete the invitation.
• Invitee can accept (update) the invitation.

@dcavins dcavins force-pushed the dcavins:invites-update branch from 02fbc84 to 8b56dc6 Jul 29, 2019

Incorporate imath's code review suggestions.
• Remove old commented out test code.
• Fix incorrect messages.
@dcavins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Hi @imath-

Thanks very much for your code review. 🥇 I've rebased from master and added your suggested changes. Git made it look like the messages had been changed, but I actually added the get_item() routine (and its associated permissions check) after working on the membership requests side, so it was copypasta.

I'm not having any failing tests (in BP or in this REST repo), so I'm guessing the failures you're seeing are due to my not providing up-to-date patches for both sides. I'm updating the BP patch now and will post. Thanks again!

@imath

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Awesome! Thanks for updating the PR and the patch on Trac-6210. I'll check it asap 👍

dcavins added some commits Jul 30, 2019

Update group invites endpoint format.
Follow the WP Posts endpoint approach:
• GET and CREATE are handled by passing parameters to /
• GET (single invite), UPDATE, and DELETE are handled by accessing `/{invite_id}`.
More updates to the invite endpoint.
• Correct links.
• Set current user as default inviter if not specified in create().
• In get_items(), set current user as default invitee if not specified (and not a site admin and not viewing invites for a group).
Update group memberships endpoint.
Follow the WP Posts endpoint approach:
• GET and CREATE are handled by passing parameters to /
• GET (single invite), UPDATE, and DELETE are handled by accessing `/{request_id}`.
• Correct links preparation.
• Ensure that passed "messages" are handled correctly if passed as part of CREATE.
Improve convenience of memberships endpoint.
• Set current user as default requestor if not specified in create().
• In get_items(), set current user as default requestor if not specified (and not a site admin and not viewing requests for a group).
@dcavins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

I've updated the group invites and memberships endpoints to behave more like the WP posts endpoint. Examples:

Fetch invitations where the current user is the invitee:
GET http://bptest.local/wp-json/buddypress/v1/groups/invites/

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

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

Fetch invitations from a specific user (must be the user or site admin):
GET http://bptest.local/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 http://bptest.local/wp-json/buddypress/v1/groups/invites/?group_id=3&user_id=2
GET http://bptest.local/wp-json/buddypress/v1/groups/invites/?group_id=3&inviter_id=6
GET http://bptest.local/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 http://bptest.local/wp-json/buddypress/v1/groups/invites/?group_id=3&user_id=2

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

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

Delete an invitation (must be invitee, inviter, group admin or site admin ):
DELETE http://bptest.local/wp-json/buddypress/v1/groups/invites/{invite_id}

Fetch requests where the current user is the requestor:
GET http://bptest.local/wp-json/buddypress/v1/groups/membership-requests/

Other requests endpoints: same patterns as the invites endpoint, just no inviter_id parameter.

And it looks like it needs a rebase! Ha ha. Anyway, thanks for looking.

dcavins added some commits Jun 27, 2019

Update logic for new group invites API.
BP_REST_Group_Invites_Endpoint is not found when BP_REST_Group_Membership_Request_Endpoint is being instantiated?
Change group membership requests endpoint.
Change membership requests logic to ensure:
• Users can create membership requests.
• The requester and the target group's site admins can delete the request.
• Group admins can accept requests.
Change group invitations endpoint.
Change invitation logic to ensure:
• Users can invite user to group.
• Invitee or inviter can delete the invitation.
• Invitee can accept (update) the invitation.
Incorporate imath's code review suggestions.
• Remove old commented out test code.
• Fix incorrect messages.
Update group invites endpoint format.
Follow the WP Posts endpoint approach:
• GET and CREATE are handled by passing parameters to /
• GET (single invite), UPDATE, and DELETE are handled by accessing `/{invite_id}`.
More updates to the invite endpoint.
• Correct links.
• Set current user as default inviter if not specified in create().
• In get_items(), set current user as default invitee if not specified (and not a site admin and not viewing invites for a group).
Update group memberships endpoint.
Follow the WP Posts endpoint approach:
• GET and CREATE are handled by passing parameters to /
• GET (single invite), UPDATE, and DELETE are handled by accessing `/{request_id}`.
• Correct links preparation.
• Ensure that passed "messages" are handled correctly if passed as part of CREATE.
Improve convenience of memberships endpoint.
• Set current user as default requestor if not specified in create().
• In get_items(), set current user as default requestor if not specified (and not a site admin and not viewing requests for a group).
@imath

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Cool! Thanks a lot @dcavins I'll look at it asap ! Just gave a quick look: it looks promising 💪

@dcavins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Cool! Thanks a lot @dcavins I'll look at it asap ! Just gave a quick look: it looks promising 💪

Reformatting it to a posts-style endpoint was much simpler than the previous approach. However, the new style wasn't possible with the way invites and requests worked before the changes in 6210. So it was a catch 22! :)

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

@renatonascalves
Copy link
Member

left a comment

Gave a quick peek and here are some of changes that could be made:

  • Try to keep the same structure on the permissions checks. $retval instead of $allow.
  • Use the correct error code for not allowed permissions. As far as I know, not allowed without permissions should be a 500 error. rest_authorization_required_code is for users not logged in. If you see otherwise in the code base, it is most likely a mistake of mine.
  • Fallbacks should be added/passed in the schema or in the get_collection_params.
  • Places where a group or a user id is being used, they should have those ids checked for existence. Usually, I'm doing that on the permissions checks. Mainly because when the code is run, they are fired first and the user would get an error sooner. Also to improve the error code/message as a rest_authorization_required_code error brings no value when the actual error is because the group doesn't exist.
  • A helper method to get an invitation would be useful and would remove a few lines of code.

Overall, the changes are good!

$args = array(
'group_id' => $group->id,
'is_confirmed' => $request['is_confirmed'],
'item_id' => isset( $request['group_id'] ) ? $request['group_id'] : false,

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member

Those fallbacks should not be necessary as the get_collection_params already have the default values as their default.

This comment has been minimized.

Copy link
@dcavins

dcavins Aug 1, 2019

Author Contributor

👍

array(
'status' => 404,
)
);
}
if ( true === $retval && ! $this->can_see( $group->id ) ) {
$retval = new WP_Error(
$this->invite = current( $invites );

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member

Any reason why this was assigned to this property?

This comment has been minimized.

Copy link
@dcavins

dcavins Aug 1, 2019

Author Contributor

I'm not sure--that's how it was done in the code base I started with, so maybe there was some thought of making the invite persist from the permission check to the get() or similar, but the invitations are all cached independently, so it shouldn't be needed.

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member

I agree! As it is right now, I don't see why it is the way it is.

'bp_rest_authorization_required',
__( 'Sorry, you are not allowed to fetch an invitation.', 'buddypress' ),
array(
'status' => rest_authorization_required_code(),

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member
Suggested change
'status' => rest_authorization_required_code(),
'status' => 500,
'type' => $invite->type,
'message' => array(
'raw' => $invite->content,
'rendered' => apply_filters( 'the_content', $invite->content ),

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member

Are you sure this is the same filter used on BuddyPress core?

This comment has been minimized.

Copy link
@dcavins

dcavins Aug 1, 2019

Author Contributor

The invite content is not yet displayed anywhere in a BP template, but I'd guess that the_content is probably the right filter. If there is a better filter, please let me know.

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member

K! I don't know of a better one. If there is no other, I'd agree this is the best one. :)

@@ -455,10 +705,17 @@ public function delete_item_permissions_check( $request ) {
*/
public function prepare_item_for_response( $invite, $request ) {
$data = array(
'user_id' => $invite->user_id,

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member

This method first variable doc is wrong. Should be BP_Invitation.

@@ -490,7 +747,7 @@ public function prepare_item_for_response( $invite, $request ) {
*/
protected function prepare_links( $invite ) {

This comment has been minimized.

Copy link
@renatonascalves

renatonascalves Aug 1, 2019

Member

PHPDoc wrong: BP_Invitation is the correct one.

@imath imath force-pushed the dcavins:invites-update branch from f069034 to 8d09674 Aug 1, 2019

@imath

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@dcavins for your info, I've just rebased your branch :) There were twice the same commits so I've cherry picked only one of them.

I've run the phpunit tests using latest patch on BP6210 and I confirm the tests are all ok. I'm going to explore it a little more.

@renatonascalves Thanks a lot for your review 👍

@imath imath self-requested a review Aug 1, 2019

@imath

imath approved these changes Aug 1, 2019

@imath imath self-requested a review Aug 1, 2019

@imath
Copy link
Contributor

left a comment

Thanks a lot for your work @dcavins 💪 here are some small details I suggest you to fix.

$user = bp_rest_get_user( $this->invite->user_id );
$group = $this->groups_endpoint->get_group_object( $this->invite->item_id );
$deleted = groups_delete_invite( $this->invite->user_id, $this->invite->item_id, $this->invite->inviter_id );

This comment has been minimized.

Copy link
@imath

imath Aug 1, 2019

Contributor

Working on 6210, I've noticed groups_uninvite_user() was used into template packs Ajax functions. I wonder what's the difference between groups_delete_invite() && groups_uninvite_user() 🤔

This comment has been minimized.

Copy link
@dcavins

dcavins Aug 1, 2019

Author Contributor

The difference is in what hooks are called (and the same goes for groups_reject_invite()), but it's confusing to me, too. From the function names, I guess you'd think:
uninvite -> when the inviter or site admin want to delete an invite to a user
reject -> when the invitee wants to delete an invite
delete -> no idea what intent there could be here.

It's a mystery to me, and something I've wonderd about in the past.

Start integrating suggestions from review.
Have introduced some problems, now.

@dcavins dcavins force-pushed the dcavins:invites-update branch from 8d09674 to 3288ee4 Aug 1, 2019

@renatonascalves renatonascalves added this to In Development in BP REST - V1 Aug 3, 2019

@dcavins

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

I'm closing this PR in favor of a fresh PR with a much simpler commit history. See you there!

@dcavins dcavins closed this Aug 5, 2019

@renatonascalves renatonascalves moved this from In Development 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.