Add tests for group invitations and membership request functions

[dcavins]

As the first step toward developing a new invitations API, I want to track down the actual and expected behavior of functions related to group invitations and membership requests.

I've started with the delete/reject functions, because they don't behave as I would expect. For example, using groups_delete_membership_request() or groups_reject_membership_request() will also remove outstanding invitations and confirmed group memberships for that user id. Using groups_reject_invite() or groups_uninvite_user() will also remove any membership requests or confirmed group memberships for that user id.

These oddities can be fixed pretty easily by using a more scoped method like BP_Groups_Member::delete_invite()or BP_Groups_Member::delete_request() rather than the more burn-notice-like BP_Groups_Member::delete().

I will finish writing tests for the other invite/request functions very soon. Please offer any thoughts or feedback.

[dcavins]

Test coverage for groups_delete_membership_request(), groups_reject_membership_request(), groups_reject_invite(), groups_uninvite_user(), groups_delete_invite()

[boonebgorges]

dcavins - The tests look generally good. Some feedback:

• Use two separate @group annotations, not two function names separated by 'and'. These group annotations can then be used at the command line: $ phpunit --group foo
• Each of your existing methods should be separated into threeish methods. Then make the method names more descriptive: test_bp_groups_uninvite_user_should_leave_confirmed_memberships_intact(). Don't worry if you need to do more setup to make this work - it's more important to have the methods be as narrow as possible.
• Go ahead and separate out the tests that are currently passing, and commit them. Commit the failing tests at the same time as the issue is fixed. #6223

[dcavins]

In 6209-working-tests.02.patch, I've added passing behavior checks on bp_groups_reject_membership_request(), bp_groups_delete_membership_request(), bp_groups_reject_invite(), bp_groups_delete_invite() and bp_groups_uninvite_user().

In 6209-correct-delete-behavior.03.patch I've added tests that I think should describe the correct behavior of those functions, and modified groups_uninvite_user(), groups_reject_invite() and groups_delete_membership_request() so that they only do what they say they're going to do. Unfortunately, we were relying on the loose cannon behavior of groups_uninvite_user() to remove a confirmed member from a group in groups_leave_group() from a variety of contexts, so that fails with my tightening up of groups_uninvite_user(). (All uses of the other functions are targeted to invites or requests, so should not be affected.) I had to also modify groups_remove_member() so that it will work as expected when called in groups_leave_group(). (My wrangling of bp_is_item_admin() in groups_remove_member() seems like a hack. Suggestions welcome for how to improve that.) Finally, I've added tests to check the behavior of groups_leave_group() and groups_join_group() because of the changes to groups_leave_group().

Kind of a mess, after all.

Thanks in advance for your comments.

[dcavins]

Add tests for group membership

[dcavins]

Correct invite/request delete behavior

[boonebgorges]

6209-working-tests.02.patch looks good. I'd go ahead and commit it.

Comments on the second patch:

• Instead of doing the $u1_has_request_before and after dance - which could mistakenly pass if both were accidentally false instead of both true - you should verify truth values directly. $this->assertTrue( groups_check_for_membership_request() ) etc.
• It seems like a bug that groups_join_group() returns true when a user is already a member, but I guess this can't be changed for backward compatibility reasons.
• For clarity's sake, when committed, it would be nice if standalone fixes (and the tests that demonstrate them) could go in as parts of separate changesets. Eg, it looks like a subset of the patch is devoted to correcting a bug in groups_remove_member() having to do with calling from outside a group context. That fix should be separate from the delete_invite switches.
• Do you have thoughts about backward compatibility with existing plugins that are expecting a certain behavior from functions like groups_delete_membership_request()? It looks like currently this family of functions all uses groups_uninvite_user() internally - you call it a "loose cannon", I'd call it a "sledge hammer". Making these functions more targeted may be more semantic, but it's also going to change existing behavior. Are there plugins expecting the current behavior? How badly will they break? It may be a good idea to look through the WP plugin repo to see if you can find any examples.
• On a related note: even if we can't find any specific examples of plugins breaking, we still ought to put something on bpdevel or elsewhere that describes the changes, and warns plugin authors about potential breakage.

[dcavins]

Thanks again for the advice, Boone. I've slightly corrected the working tests patch per our conversation about writing tests. The new version is 6209-working-tests.04.patch.

I've also checked the WordPress plugin repo, and the functions that I'd like to modify are used as follows:

groups_uninvite_user()

BP Group Control uses groups_uninvite_user() to delete members from groups
Last Updated: 5 years ago, Compatible up to: BP 1.2.4.1, WP 2.9
/bp-group-control/bpgc-classes.php:92
/bp-group-control/bpgc-core.php:239

BuddyPress uses groups_uninvite_user() to remove invites, and in one place even specifically checks for existing requests before using the function:
/buddypress/bp-templates/bp-legacy/buddypress-functions.php:1188

Buddypress Backwards Compatibility uses groups_uninvite_user() to remove invites
/buddypress-backwards-compatibility/bp-themes/bp-sn-parent/_inc/ajax.php:99

CommentPress uses groups_uninvite_user() to remove invites
/commentpress-core/commentpress-core/assets/includes/bp-compat/ajax.php:669

Genesis Connect for BuddyPress uses groups_uninvite_user() to remove invites
/genesis-connect-for-buddypress/lib/ajax.php:458

Invite Anyone uses groups_uninvite_user() to remove invites
/invite-anyone/group-invites/group-invites.php:407

groups_reject_invite()

Only used by BuddyPress to delete invites
/buddypress/bp-groups/bp-groups-functions.php:1012
/buddypress/bp-groups/bp-groups-screens.php:79

groups_remove_member()

BP Groups Civi crm Sync would like to use groups_remove_member() but have trouble with the is_item_admin check
/bp-groups-civicrm-sync/bp-groups-civicrm-sync-bp.php:480:
"We cannot use 'groups_remove_member()' because the logged in user may not pass the 'bp_is_item_admin()' check in that function."

groups_delete_membership_request()

BP Group Management uses as expected.
/bp-group-management/bp-group-management-aux.php:87

BuddyForms Attach Posts to Groups Extension uses as expected.
/buddyforms-attach-posts-to-groups-extension/includes/group-control.php:128

Sort of surprisingly, it looks like the functions are being used to do what their names would suggest. I don't think we'd break anything other than the really out-of-date BP Group Control. There's no way to know how many custom plugins we'd break, though. Given what I found, do you think it's worth changing the behavior of the various function or would you prefer I don't pursue that?

[boonebgorges]

dcavins - Thanks for doing this research. Based on your findings, I'd go ahead and break them. Make sure the internal documentation is clear, then write a blog post about breaking changes with instructions on how to fix, and put it up at bpdevel (I'll give you access if you don't have it). Feel free to call out specific plugins (like BP Group Control).

[dcavins]

Changes groups_uninvite_user(), groups_reject_invite(), groups_delete_membership_request() to only remove the membership type that the function name refers to.

[dcavins]

Change groups_leave_group() to use groups_remove_member(). Change groups_remove_member() to work in more cases.

[dcavins]

Test coverage for groups_get_invites_for_user(), groups_get_invite_count_for_user(), groups_invite_user(), groups_accept_invite(), groups_send_invites(), groups_delete_all_group_invites(), groups_send_membership_request(), groups_accept_membership_request(), groups_accept_all_pending_membership_requests()

[dcavins]

Documentation updates for groups_get_invites_for_user (), groups_check_user_has_invite() & groups_check_for_membership_request()

[dcavins]

I'm refreshing the patches. Current patches begin with .09.patch

[dcavins]

Tests for groups_reject_membership_request(), groups_delete_membership_request(), groups_reject_invite(), groups_delete_invite(), groups_uninvite_user(), groups_join_group(), groups_leave_group(), groups_get_invites_for_user(), groups_get_invite_count_for_user(), groups_send_invites(), groups_accept_invite(), groups_delete_all_group_invites(), groups_send_membership_request(), groups_accept_membership_request(), groups_accept_all_pending_membership_requests()

[dcavins]

Change behavior of groups_uninvite_user(), groups_reject_invite(), groups_delete_membership_request(), to only affect the membership types referred to by function name. Adds tests to support the new behavior.

[dcavins]

Use groups_remove_member() in groups_leave_group(). Also change groups_remove_member() to allow it to work in more situations.

[dcavins]

Documentation changes for group invitation functions.

[dcavins]

In 9810:

Limit scope of group invitation delete functions.

Change behavior of groups_uninvite_user(),
groups_reject_invite() and
groups_delete_membership_request(),
to affect only the membership types referred to
by the function name. Adds tests to support the
new behavior.

See #6209.

[dcavins]

In 9811:

Use groups_remove_member in groups_leave_group.

Also change groups_remove_member() to allow it
to work in situations where bp_is_item_admin
may not be true.

See #6209.

[johnjamesjacoby]

Bumping this to 2.4, pending confirmation of completeness from David.