Skip to:
Content

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#6209 closed enhancement (fixed)

Add tests for group invitations and membership request functions

Reported by: dcavins Owned by: dcavins
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch
Cc:

Description

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.

Attachments (12)

6209-invites-requests.01.patch (11.4 KB) - added by dcavins 3 years ago.
Test coverage for groups_delete_membership_request(), groups_reject_membership_request(), groups_reject_invite(), groups_uninvite_user(), groups_delete_invite()
6209-working-tests.02.patch (4.7 KB) - added by dcavins 3 years ago.
Add tests for group membership
6209-correct-delete-behavior.03.patch (17.7 KB) - added by dcavins 3 years ago.
Correct invite/request delete behavior
6209-working-tests.04.patch (4.5 KB) - added by dcavins 3 years ago.
6209-behavior-change.05.patch (8.5 KB) - added by dcavins 3 years ago.
Changes groups_uninvite_user(), groups_reject_invite(), groups_delete_membership_request() to only remove the membership type that the function name refers to.
6209-groups_leave_group.06.patch (8.5 KB) - added by dcavins 3 years ago.
Change groups_leave_group() to use groups_remove_member(). Change groups_remove_member() to work in more cases.
6209-more-invite-tests.07.patch (12.9 KB) - added by dcavins 3 years ago.
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()
6209-invite-docs.08.patch (2.2 KB) - added by dcavins 3 years ago.
Documentation updates for groups_get_invites_for_user (), groups_check_user_has_invite() & groups_check_for_membership_request()
6209-passing-tests.09.patch (24.2 KB) - added by dcavins 3 years ago.
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()
6209-correct-behavior.10.patch (7.5 KB) - added by dcavins 3 years ago.
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.
6209-groups-leave-group.11.patch (1.8 KB) - added by dcavins 3 years ago.
Use groups_remove_member() in groups_leave_group(). Also change groups_remove_member() to allow it to work in more situations.
6209-docs-updates.12.patch (2.2 KB) - added by dcavins 3 years ago.
Documentation changes for group invitation functions.

Download all attachments as: .zip

Change History (27)

@dcavins
3 years ago

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

#1 @boonebgorges
3 years ago

  • Milestone changed from Awaiting Review to 2.3

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

#2 @dcavins
3 years ago

  • Owner set to dcavins
  • Status changed from new to assigned

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


3 years ago

#4 @dcavins
3 years ago

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
3 years ago

Add tests for group membership

@dcavins
3 years ago

Correct invite/request delete behavior

#5 @boonebgorges
3 years ago

  • Keywords needs-unit-tests removed

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.

#6 @dcavins
3 years ago

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?

#7 @boonebgorges
3 years ago

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
3 years ago

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

@dcavins
3 years ago

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

@dcavins
3 years ago

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
3 years ago

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

#8 @dcavins
3 years ago

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

@dcavins
3 years ago

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
3 years ago

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
3 years ago

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

@dcavins
3 years ago

Documentation changes for group invitation functions.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


3 years ago

#10 @dcavins
3 years ago

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.

#11 @dcavins
3 years ago

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.

This ticket was mentioned in Slack in #buddypress by jjj. View the logs.


3 years ago

#13 @johnjamesjacoby
3 years ago

  • Milestone changed from 2.3 to 2.4

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

#14 @dcavins
3 years ago

  • Milestone changed from 2.4 to 2.3
  • Resolution set to fixed
  • Status changed from assigned to closed

#15 @DJPaul
2 years ago

  • Type changed from task to enhancement
Note: See TracTickets for help on using tickets.