Create New Invitations API

[dcavins]

Create a flexible API to create/retrieve invitations across components.

Examples:

• invite to a group
• invite to the site
• invite to a blog
• invite to edit a doc

Tasks:

• Write tests for group invitations. (See #6209)
• Write core invitations API code.
• Switch group invitations over to using invitations API.
• improve group invitations experience
• Add new features.

---

Data structure

On one hand, it makes sense to use a dedicated table for this like we do with notifications, but it could easily be built using a new custom post type and post meta (since Boone’s been souping up WP_Query's meta queries...)

I would really appreciate some feedback on which way BuddyPress wants to go with future data and the positives of each approach.

If a table, here are the fields I’m imagining:
id - primary key
user_id - ID of invited user, maybe 0 if invitee is not a site member
inviter_id - ID of user that created invite, maybe 0 if a request for membership
invitee_email - used when invitee is not yet a member of the site
component_name - e.g., groups, similar to activity/notification
component_action - to cover components that need several invitations
item_id - e.g., group id in the case of a group invitation
secondary_item_id - flexibility for components
content - store extra information provided by inviter/requester
date_modified - date created or date invite sent
invite_sent - 0 if draft, 1 if sent.
opt_out - Has this user asked to receive no more invitations from a component?

Group Memberships

Currently, outstanding group memberships are stored in the group members table and identified via a combination of statuses. I’m thinking that users who have been invited or have requested membership would exist only in the new invitations table.

Areas for improvement/Adding new features

• Allow multiple invitations to a group from different users, as imath proposed—this helps with transparency.
• Allow site admins to decide if members can only invite friends to groups (current behavior), or if users can invite any site member.
• Apply the new suggestions API to the group invitation pane. (Can it be extended to display status messages like “Violet Vance — already a member”?)
• Allow invitation of non-site members to the site and groups, like Invite Anyone.
• Maybe provide centralized invitations pane for user with status. Could be a new tab, or maybe should be under some other tab—there are a lot of navigation options on the user’s profile already—could notifications about invites contain actions for responding to invitations/requests?
• Improvements to invitations UI so that it’s clear what status each invitation has—draft, sent (and when)—and the ability to resend.
• Improved invitation management for group admins and site admins (via wp-admin groups management pane).

[johnjamesjacoby]

Awesome!

[DJPaul]

Looking at this at a high level, why is this a Component? The only current feature that would use an Invitations Component are Groups. You list possible cases such as "invite to site", "invite to edit a doc", but all those would rely on third-party functionality.

My initial reaction is that I'm unsure why promoting this specific piece of Group functionality to its own Component would make sense. (I don't think every plugin for BuddyPress is going to require user invitations)

[boonebgorges]

I agree with DJPaul that there's no need for an Invites component, but I also think that dcavins never suggested one :-D

[DJPaul]

Maybe I'm just being too pedantic; one of the top lines in the patch says "Classes used for the Invitations component." ;)

[johnjamesjacoby]

Replying to boonebgorges:

I agree with DJPaul that there's no need for an Invites component, but I also think that dcavins never suggested one :-D

The patch is setup as src/bp-invitiations.

I would love for this to be some kind of "common" library (similar to what we are envisioning attachments to be?) with a universal approach towards inviting members to do things.

• Join groups
• Join a site/network
• Join a private message thread
• Become friends
• Third party components like events, docs, chat, etc...

If that means making it a full-fledged component, I'm fine with that approach, but we will want unit test coverage on existing invitations code to ensure nothing breaks.

If we went the component route, we would want to default it to on for upgrades, and let each component continue to work as before, and decide whether or not new installations should be "invitation" friendly networks, or whether that's worth allowing someone to turn on.

[dcavins]

I've been thinking about the component-ness of this ever since talking with @jjj about it, and I still don't think it needs to be a component (as I understand components to be--core code that includes critical user-facing functionality and is on a par with "activity" or "groups").

However, once you add the ability to invite someone to the site or to manage a variety of invitations in a central interface, that probably happens via the user profile (like Invite Anyone). Since "add new features" is pretty far down the list (ha ha), I'm still considering this as a library, not a component, with the thought that some critical and awesome user-facing functionality to be named later could cause it to grow into component-ness.

I'm really building a helper library at the moment, although I'm taking many code design cues from the notifications component. I will also be taking cues from @imath's work on the non-component attachments API.

Also, trac, why don't you email the ticket creator when the ticket is updated? :)

[r-a-y]

Maybe we can think of invites as a pending signup in the wp_signups table.

wp_signups already has a meta column, but it's pretty much ugly. Perhaps all that is needed is a wp_signups_meta table?

That way, we can attach information about what groups, friends, etc. that this signup user should be a part of once the user successfully registers.

[dcavins]

Punting to next version. I'm going to scale back what I'm trying to do to get the core changes in place and then add new features. I was taking too big of a bite at one time. :(

[dcavins]

It's so sad to punt this (again). :(

[dcavins]

Adds nw invitations table. Switches group invites to use new invitations API.

[dcavins]

I've just added a new "progress" patch. In it, I've worked out many of the details of saving, updating and retrieving invitations from the new invitations table and switched BP groups to use the new core functions for their invitations and requests. There are plenty of places I see to make improvements, like letting the group invites rely on default core invite behavior more.

The biggest questions mark for me is that this change removes pending members from the groups_members table. It seems like a nice improvement, but it really messes with BP_Groups_Member_Query. I'd love some feedback on how this could work more smoothly--I hacked something together but I have doubts. Many tests are failing because of how group invites are created in the tests (new BP_Group_Member( $args )), which could be fixed in the tests, but, geez, are people in the wild using the BP Group Member class in that way? If so, this idea just got harder. :)

Also feedback on my caching mechanisms would be very helpful (especially the bits where I'm filtering lists of invitations to avoid making more database calls). Of course, any feedback is helpful--this is a big lump of code changes.

Moving to a new table will also require an upgrade method, which is still to be worked out.

Thanks for your comments!

[dcavins]

Now that I have an understanding of how the business parts of invitations could work, I was thinking about how to make it more like an API. The two approaches that came to mind were:

• handling it like the BP_Group_Extension class or the BP_Attachment class with stub functions that new implementations should include to specify the behavior they need.

class BP_Invitations_API {
public function send_invitation() {}
public function verify_invitation() {}
public function accept_invitation() {}
...
}

• handling it more like the class WP_Customize_Setting, by declaring callbacks via some helper function, like

bp_register_invitation_set( array(
	'send_invitation'   => 'my_send_invitation_callback',
	'verify_invitation' => 'my_verify_invitation_callback',
	'accept_invitation' => 'my_accept_invitation_callback',	
) );

Does BP have a preference for what style we'd like to use?

[johnjamesjacoby]

Other than the _API suffix (which I assume was just paraphrasing code) I think this type of code clarity is exactly what BuddyPress needs more of, specifically for Invitations, and to wrap around the otherwise relatively complex and procedural logic involved with site invitations in WordPress.

I also think the array of arguments that gets passed into any registration function will require more scrutinization, but I think you have the right idea, so keep running with it. I imagine @boonebgorges will have deeper examples of existing patterns that could be useful as starting off points.

[DJPaul]

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see ​https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: ​https://make.wordpress.org/chat/

[dcavins]

I've written a new patch for this and updated group invitations to use the new logic. It went better than I expected, though there are still some question marks, because group memmberships and invitations are so intertwined currently.

I was able to update nearly all phpunit tests to verify that existing group invitation behavior works as expected. (The failing tests show that bp_get_user_groups() is going to be a sticking point.)

I'd appreciate feedback on the core setup (introducing a BP_Invitiations_Invitations class that represents an invitation and contains the SQL-query-building logic, a base logic class BP_Invitations that is meant to be extended to adapt the behavior to specific components, and an extension class BP_Groups_Invitations to handle the management for group invitations).

If the basic structure makes sense, I'll work on writing the network and site invitation extension classes, which seem pretty doable.

Thanks for your feedback!

[dcavins]

Add core invitations classes and groups invitations extension class. Update tests.

[boonebgorges]

Thanks for bringing this one back to life, @dcavins !

I've done a casual review and I have some questions and feedback. In no particular order:

• Can you explain how component_name and component_action work? Specifically, it feels like component_name is enough to differentiate between, eg, group invitations and site invitations.
• Regarding naming: I'm not a huge fan of BP_Invitations_Invitation. IMO it's better to go with BP_Invitation or BP_Core_Invitation. And BP_Invitations is not very clear to me. Maybe something like BP_Invitation_Manager, BP_Group_Invitation_Manager, etc? ​https://stackoverflow.com/questions/1866794/naming-classes-how-to-avoid-calling-everything-a-whatevermanager
• Otherwise, the general pattern of having a base manager class and then extending it per component seems OK.
• Because run_send_action() and run_acceptance_action() seem to be the critical integration points for specific components, perhaps it'd make sense (from the point of view of self-documentation) to make the base class abstract and to make these two methods abstract.
• I see that, in various places, you're deprecating the use of membership_id in the case of acceptance/rejection. Obviously, I understand why you have to do this, and some level of compatibility breakage is unavoidable in this kind of change. But I'm concerned that it's going to cause backward compatibility concerns of an awkward type. For example, in bp_group_request_accept_link() you are swapping membership_id with user_id in the existing link format. If a third-party plugin is manually building a link of this sort, and accidentally puts in a "membership id" that matches up with a valid user_id, it could result in a link that accepts the wrong invitation. This is bad. In cases like this, perhaps it's better to break those old links altogether, and change the format: admin/membership-requests/accept-request/ or something like that. More generally, it'll be necessary to have a high-level summary of compatibility breaks, so we can share with BP devs.

[dcavins]

Thanks Boone, for your review. That's exactly the kind of feedback I was hoping for. Comments below.

Replying to boonebgorges:

Thanks for bringing this one back to life, @dcavins !

I've done a casual review and I have some questions and feedback. In no particular order:

• Can you explain how component_name and component_action work? Specifically, it feels like component_name is enough to differentiate between, eg, group invitations and site invitations.

I'm not using component_action at the moment for group invitations, but added it for flexibility, because I can imagine that a single component could need multiple invitation types. For example, if some "sites" invitation setup was needed to be able to invite people to subscribe to update emails or to join the site, then you could identify your different invitations as sites/subscribe or sites/join. Of course, two new extension classes could be used to arrive at the same end result.

• Regarding naming: I'm not a huge fan of BP_Invitations_Invitation. IMO it's better to go with BP_Invitation or BP_Core_Invitation. And BP_Invitations is not very clear to me. Maybe something like BP_Invitation_Manager, BP_Group_Invitation_Manager, etc? ​https://stackoverflow.com/questions/1866794/naming-classes-how-to-avoid-calling-everything-a-whatevermanager

This is one of the biggest doubts I had. I named BP_Invitations_Invitation in a sort-of-BP pattern (BP_Notifications_Notification), but think it's awkward. I'll happily rename the classes.

• Otherwise, the general pattern of having a base manager class and then extending it per component seems OK.
• Because run_send_action() and run_acceptance_action() seem to be the critical integration points for specific components, perhaps it'd make sense (from the point of view of self-documentation) to make the base class abstract and to make these two methods abstract.

Yes, I started out with the base class being abstract, but thought there might be some high-level use in being able to invoke it directly to do mass invitation clean-ups without having to work through the extension classes. This seems like an edge-case use to me, though, since you could work directly with the invitations object class to do mass deletions, for instance. If you think the base class should be abstract, with certain key methods being marked abstract, I think it's a good idea.

• I see that, in various places, you're deprecating the use of membership_id in the case of acceptance/rejection. Obviously, I understand why you have to do this, and some level of compatibility breakage is unavoidable in this kind of change. But I'm concerned that it's going to cause backward compatibility concerns of an awkward type. For example, in bp_group_request_accept_link() you are swapping membership_id with user_id in the existing link format. If a third-party plugin is manually building a link of this sort, and accidentally puts in a "membership id" that matches up with a valid user_id, it could result in a link that accepts the wrong invitation. This is bad. In cases like this, perhaps it's better to break those old links altogether, and change the format: admin/membership-requests/accept-request/ or something like that. More generally, it'll be necessary to have a high-level summary of compatibility breaks, so we can share with BP devs.

Yes, the intertwining of group memberships and invitations causes some problems, because group invitations and requests are cached and referred to by their ID in the group_membership table. So any time the streams get crossed with the new table IDs, it causes an issue, like in bp_get_user_groups(). I was able to support backwards compatibility in many cases, but, for example, if a dev is issuing invitations by using the BP_Groups_Member class directly (we were doing that in our tests), I'm not sure how to prevent a problem. I agree with you that making a clean break where we have to is probably better than creating a situation where it fails occasionally, and frustratingly.

I was hoping for very much like 100% backwards compatibility, but that isn't possible, so having a plan on how to break compatibility in sensible ways will be needed.

Thanks again for taking a look! This is quite a big change to be making, so I was happy that there were many tests to cover various aspects of group membership and find problems, but having your input is much more valuable.

[boonebgorges]

I'm not using component_action at the moment for group invitations, but added it for flexibility

I figured as much. I wonder if it might be more transparent (though less BP-ish?) to simply have a type column. group_member would be one type. account might be another type. site_membership (ie a role on a WP site in a MS network) might be another. Then, do away with the component columns. They'd still be registered within components - so bp-groups/classes/... and so on - but without the conceptual overhead of name and action, which almost-but-not-quite fits here.

This seems like an edge-case use to me, though, since you could work directly with the invitations object class to do mass deletions, for instance.

Yeah, let's go with abstract. Someone doing this kind of thing ought to have to set up their own extending class, as a way of making intentional that they want certain actions to no-op.

I agree with you that making a clean break where we have to is probably better than creating a situation where it fails occasionally, and frustratingly.

Agree 100%. Let's make sure that these breaks are properly documented, along with some suggestions about how to migrate to the new system.

Speaking of migration, we'll soon have to discuss how migration will work.

[dcavins]

I'm attaching a new patch, that makes the following changes to the previous patch:

• Rename logic classes and invitation object class:
  • BP_Invitation is the invitation object
  • BP_Invitation_Manager is the base logic class
  • BP_Groups_Invitation_Manager is the extension for group invitations.

• Mark the base class and the methods within it, run_send_action() and run_acceptance_action(), abstract. You gotta bring your own business logic if you wanna play.

• Remove component_name and component_action fields. I'm using the (sanitized) name of the extension class in place of this previous identifier, which I think makes sense. For example, a group invitation is stored like this:

id: 9
user_id: 3
inviter_id: 1
invitee_email:
class: bp_groups_invitation_manager
item_id: 8
secondary_item_id: 0
type: invite
content: We would love it if you would join our group!
date_modified: 2019-03-13 21:37:44
invite_sent: 1
accepted: 0

Let me know if that's a bad idea for some reason. It seems simple to me, and also implies that that field will be unique per extension class, because a duplicated class would throw an error for php reasons. Of course, if you change the name of your extension class, the invitations in the database will be left disconnected. (The same would be true if you changed the component_name property with the earlier patch.)

• Break the accept/reject membership request links and acceptance logic, which previously keyed on the membership_id of the request, using query arguments instead of bp_action_variable positions. New links look like: ​http://bptest.local/groups/group-six/admin/membership-requests/?_wpnonce=f8b7e7a866&user_id=2&action=accept

• Update tests, adding a test invitation extension (now that the base class is abstract).

Thanks in advance for your feedback!

[dcavins]

Change names of classes and other logic changes.

[boonebgorges]

Thanks for your work here, @dcavins ! These changes seem good to me.

The class_name trick looks fine. I prefer it to the name/action juggling from your previous patch. One small thing I'd mention is that, since you're silently translating the class name using sanitize_key() at the time that the invite is saved, you may also want to do the same thing to the 'class' parameter when *querying* for invitations. This way, if someone naively tries passing in something like HardG\MyPlugin\InvitationManager, the query will work.

What's the plan for the bp_get_user_groups() breaks? I haven't looked into the root cause.

[dcavins]

Hi Boone,

Thanks for your message. As long as you are creating or fetching invitations via a manager class, the class name should always be sanitized:
​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation-manager.php#L364

The property class_name is set (and the sanitization occurs) in the construct method:
​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation-manager.php#L33

Are you thinking that you'd like to see the sanitizing happen not in the manager class but down in the invitation object class?

Yes, I've been hoping for some blast of inspiration on how to deal with bp_get_user_groups(). The sticky bit is that bp_get_user_groups caches by the ID in the membership table. The best workaround I've thought of so far is to prefix the "ID" of invitations and requests with the name of the invitations table or similar, to avoid cache collisions. Then, I'd have to add some logic to bp_get_user_groups that checks the ID to see where the membership data should come from. Of course, blending IDs like that will cause an order by ID sort to fail, but I guess it should, since they're no longer apples to apples.

I'll write up a quick fix, and we can see how gross it really is.

Thanks again for reading over the patch!

[boonebgorges]

Are you thinking that you'd like to see the sanitizing happen not in the manager class but down in the invitation object class?

No, I mean you should also sanitize when querying using BP_Invitation::get(). ​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation.php#L39

[dcavins]

Replying to boonebgorges:

Are you thinking that you'd like to see the sanitizing happen not in the manager class but down in the invitation object class?

No, I mean you should also sanitize when querying using BP_Invitation::get(). ​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation.php#L39

Sorry, that's what I was trying to say by "down in the invitation object class".

I could add the sanitization to the populate method (which would effectively be a pre-save sanitization):
​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation.php#L230

and the get method as you suggest:
​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation.php#L704

Then the sanitization could be removed from the manager class:
​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation-manager.php#L41

And it would be enforced whether you were using a manager class to manage invitations (like we expect), or if you were working directly with the invitation object via methods like BP_Invitation::get(), which is a bit more dangerous, but surely people will do.

Whichever you prefer is fine with me.

Thanks!

[boonebgorges]

Oh, I see what you mean about the sanitization happening higher up. I guess my preference would be to do it closer to the database, which is to say at ​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation.php#L230 and at ​https://github.com/dcavins/bp-svn-bporg/blob/6210-Aug2018/src/bp-core/classes/class-bp-invitation.php#L395. But it doesn't matter too much, as long as it's consistent on the way in as well as on the way out.

[dcavins]

I'm attaching two new files. The first is an updated complete patch. It contains the previous work, plus some fixes for bp_get_user_groups(), caching issues, and action points, and adds a migration function.

The other is just the commit that deals with the (ugly) changes to bp_get_user_groups() that arise from having to avoid cache id collisions when data is stored in two different tables.

Thanks in advance for your feedback!

[dcavins]

Complete patch; adds migration, fixes for bp_get_user_groups(), moves the class name sanitization down into the Invitation object class, test updates.

[dcavins]

Snapshot of changes required to make bp_get_user_groups() work with new table.

[boonebgorges]

@dcavins I wonder if we could avoid much of the calisthenics in bp_get_user_groups() by using a different cache group, and handling invites and memberships totally separately. So:

$membership_ids = wp_cache_get( $user_id, 'bp_groups_memberships_for_user' );
// prime caches, etc

$invitation_ids = array();
if ( invites need to be included ) {
  $invitation_ids = wp_cache_get( $user_id, 'bp_groups_invitations_for_user' );
  // prime caches, etc
}

// Loop through memberships as currently happens in the function, and assemble membership objects
foreach ( $membership_ids as $membership_id ) {
    // ...
    $groups[ $group_id ] = $membership;
}

// Then loop through the invitation objects and fake them as "memberships"
foreach ( $invitation_ids as $invitation ) {
  $invitation = wp_cache_get( ... );
  
  $invite_membership_obj = new stdClass();
  ...
  
  $groups[ $group_id ] = $invite_membership_obj;
}

No extra non_cached_keys() overhead; we only fetch the invitations when they're actually requested (I think this is just is_confirmed=false?); we keep the cache keys easier to understand and track; and the logic of bp_get_user_groups() is easier to follow.

I started working on a patch, but before putting the work in, I wanted to check whether there was a reason for not going this way instead.

Migration routine looks fine - I think the numbers will be small enough that this'll work.

[dcavins]

Replying to boonebgorges:

@dcavins I wonder if we could avoid much of the calisthenics in bp_get_user_groups() by using a different cache group, and handling invites and memberships totally separately. So:

$membership_ids = wp_cache_get( $user_id, 'bp_groups_memberships_for_user' );
// prime caches, etc

$invitation_ids = array();
if ( invites need to be included ) {
  $invitation_ids = wp_cache_get( $user_id, 'bp_groups_invitations_for_user' );
  // prime caches, etc
}

// Loop through memberships as currently happens in the function, and assemble membership objects
foreach ( $membership_ids as $membership_id ) {
    // ...
    $groups[ $group_id ] = $membership;
}

// Then loop through the invitation objects and fake them as "memberships"
foreach ( $invitation_ids as $invitation ) {
  $invitation = wp_cache_get( ... );
  
  $invite_membership_obj = new stdClass();
  ...
  
  $groups[ $group_id ] = $invite_membership_obj;
}

No extra non_cached_keys() overhead; we only fetch the invitations when they're actually requested (I think this is just is_confirmed=false?); we keep the cache keys easier to understand and track; and the logic of bp_get_user_groups() is easier to follow.

I started working on a patch, but before putting the work in, I wanted to check whether there was a reason for not going this way instead.

Migration routine looks fine - I think the numbers will be small enough that this'll work.

Hi Boone, yes, I have another branch that took that approach, so can easily verify that it works and share it. Essentially, invitations and requests are only needed if is_confirmed = false || null.

I'm not sure that adding a special cache group bp_groups_invitations_for_user is needed, given that invitations are individually cached, and the SQL query results are cached using an incrementor. So doing a groups_get_invites() call should always hit the cache first anyway. The only advantage is that you wouldn't have to reformat the cached invitation objects as fake memberships, but I'm assuming that's a pretty low overhead operation.

Thanks for looking it over!

[boonebgorges]

I'm not sure that adding a special cache group bp_groups_invitations_for_user is needed, given that invitations are individually cached, and the SQL query results are cached using an incrementor.

IMHO, it's needed just for clarity's sake. The driving force behind this ticket is that requests and invitations are NOT like memberships - they're a different kind of entity. You're now storing them separately in the database; you have different representations for them in the codebase (the new BP_Invitation classes); you have different query methods; and so on. Naively, this suggests that we shouldn't be forcing them into the same cache group either, especially since doing so requires us to concatenate keys, etc.

For backward compatibility, you do have to continue to pretend that they're the same thing in the context of bp_get_user_groups(). But, for ease of comprehension, we should minimize this.

[dcavins]

Thanks for your feedback on the changes to bp_get_user_groups(). I'm attaching an alternative logic branch that I had abandoned but was able to simplify after reading your initial feedback, which was very helpful. I'll also attach another complete patch, including the alternative logic in bp_get_user_groups().

There is one other wrinkle between the new invitations logic and bp_get_user_groups(). bp_get_user_groups() formats the response as an array, keyed by the group ID. However, it's possible to have more than one entry per group, so the results may not be 100% accurate. In current BP, I think it would be possible for an unsent invitation and a later request to the same group to exist, so the last record encountered would set the status for the user in that group. In both cases, the user is not confirmed, but determining status between "pending" and "invited" from bp_get_user_groups() could be incorrect.

In the new invitations scheme, multiple users are allowed to extend an invitation to the same user, so it would be conceivable, if an unsent invitation is the last record encountered, the user may not seem invited, though an earlier record was a sent invitation.

Because I've altered groups_is_user_invited(), which used to rely on bp_get_user_groups() to determine the status, the chance for error is minimized, but there is a slim chance that if devs are currently using bp_get_user_groups() to fetch invitations, they could run into the occasional problem.

This key conflict just occurred to me while updating the patch. I hope my explanation makes sense.

Thanks!

[dcavins]

Updated complete patch; includes alternative logic for bp_get_user_groups().

[dcavins]

Alternative approach to bp_get_user_groups() as suggested by Boone.

[dcavins]

@boonebgorges Are you generally OK with the approach I've taken here? If so, I'll begin work on the network invitations extension. (I expect that working on it will help me find limitations in the base classes.)

About "network/site" language, should we call invitations to a BP site "network invitations" and invitations to sites on a network "site invitations". (I'm working on the assumption that BP should be run as the base site on a WP network, though I don't know firsthand how that works in a multi-network setup.)

Thanks again for your feedback.

[boonebgorges]

Approach looks better to me - thanks for humoring me.

There's no great solution to the language problem, but what you've described here seems good to me :)

[dcavins]

Rebased version of the patch.

[imath]

Hi @dcavins

First, thanks a lot for your work on this ticket and these patches. I can imagine the huge time you invested in it so far 💪

In 6210-imath-suggestions.patch, you’ll find:

• some minor code formatting suggestions,
• some “highlighted” @todos so that you don’t forget to remove the one that has been achieved,
• I also think it’s important to check the Groups component is active before doing the migrate task.

5 more sensitive issues needing fixes:

1. Two tests are failing about the Personal Data export feature:

→ BP_Tests_Groups_Functions::test_bp_groups_pending_sent_invitations_personal_data_exporter
→ BP_Tests_Groups_Functions::test_bp_groups_pending_received_invitations_personal_data_exporter

2. 7 tests are failing in the BP REST plugin.

I must say I was fearing this considering the subject of the patch. As we announced 5.0.0 is focused on the REST API, the existing endpoints need to be updated to this new API, maybe @espellcaste can give you a hand about this.
Here is the list of the tests:

→ BP_Test_REST_Group_Invites_Endpoint::test_get_items
→ BP_Test_REST_Group_Invites_Endpoint::test_create_item
→ BP_Test_REST_Group_Invites_Endpoint::test_mods_can_create_item
→ BP_Test_REST_Group_Membership_Request_Endpoint::test_get_items
→ BP_Test_REST_Group_Membership_Request_Endpoint::test_create_item
→ BP_Test_REST_Group_Membership_Request_Endpoint::test_update_item
→ BP_Test_REST_Group_Membership_Request_Endpoint::test_delete_item

3. BP Nouveau is the new default Template pack, and one feature is broken 😬. In BP Nouveau, invites can be sent even if the friends component is not active. Having your patch applied: it’s no more possible. See the following screen capture:

Moreover, some deprecated messages are thrown: it shouldn’t be the case imho for a template we directly maintain ;)

4. BP Legacy: it’s not possible to invite a user even if the friends component is active 😳. As soon as you click on the checkbox the Ajax query returns 0 and the page is reloaded to this:

5. Although I’m invited to join a group, the Groups Admin Bar submenu tells me there’s no Pending Invites 🤔

More generally, I understand that the goal of this first patch is to improve the way invitations (& requests?) are managed: which is already great. I would suggest to try to include a little feature to show how it has the potential to improve the BuddyPress experience. It can be allowing inviting any member of a site -- as in Legacy the Group’s Settings screen is misleading showing ​this part even if the friends component is not active -- for instance : it shouldn’t take long.

imho, I’d leave the ticket on the up/next milestone, changing its priority (eg: high) so that you can commit it early on the 6.0.0 milestone. But if you feel 100% confident about your patch and about fixing the 5 issues above before 5.0.0-beta, then let’s try to have it in 😉

[imath]

Following our latest dev-chat: let's try to make this happen for 5.0.0 :)

[imath]

Hi @dcavins,

I've refreshed the patch, but I've noticed your latest patch (6210-06182019.diff) wasn't including the latest improvements you brought into this branch: ​https://github.com/dcavins/bp-svn-bporg/commits/6210-Mar2019. So I was a bit surprised to find some bugs we already discussed about :)

Could you refresh the patch here using the branch I'm talking about ? I think it would be best to test the improvements to the BP REST API you're suggesting in ​https://github.com/buddypress/BP-REST/pull/197

Oh and I almost forgot I've found a deprecated notice when sending invites using BP Legacy, please check how groups_send_invites() is used in groups_screen_group_invite() 😇

Thanks in advance ;)

[dcavins]

Hi @imath-

Thanks for you feedback. I've rebased and updated the patch, which I'll upload shortly. Thanks for catching the notice on groups_send_invites(); I've updated the function everywhere. :)

[dcavins]

Rebased version.

[imath]

Thanks a lot for your work on it. You're almost arrived at the finish line! Congrats 💪

I confirm points 1, 3 and 4 of ​comment #42 are fixed.
Point 2 is being dealt in ​PR #197

About point 5, it's actually a bug we currently have! But let's fix it here 😁: Could you edit line 797 of src/bp-groups/classes/class-bp-groups-component.php replacing if ( ! empty( $count['total'] ) ) { by if ( $count ) {

Here are some last advices/concerns:

• When a group invite is removed for a user the corresponding notification is not.
• About bp_groups_migrate_invitations() I'm a little worried if there are a lot of group invites, pending requests to migrate : is there a risk of time out ?
• The code in src/bp-core/bp-core-invitations-cache.php could be added to src/bp-core/bp-core-cache.php that would avoid to add/load a new file for only one function, If you apply this then don't forget to remove the include part in class-buddypress.php ☺️
• in src/bp-core/classes/class-bp-invitation.php there are extra line feeds at line 2, 89 & 135
• in src/bp-core/classes/class-bp-invitation-manager.php there's an extra line feed at line 562
• What if a plugin is checking the Group members table for is_confirmed = 0 or invites_sent = 1 ? I can imagine it's difficult to manage, do you plan to write a dev note (on bpdevel for instance) to anticipate/inform about this new & great API?

[dcavins]

Hi @imath-

Thanks for your comments. Other than the notifications comment, I've integrated your suggestions into the most recent patch.

In the most recent patch, I also updated many tests that were adding "unconfirmed" users to the groups_members table directly, so were not fully testing the somewhat complicated code that adds invitations table info to the BP_Group_Member_Query results.

What if a plugin is checking the Group members table for is_confirmed = 0 or invites_sent = 1 ? I can imagine it's difficult to manage, do you plan to write a dev note (on bpdevel for instance) to anticipate/inform about this new & great API?

Yes, direct checks of the groups_members table will no longer yield the right answers. Similarly, manipulating the BP_Groups_Member object directly will no longer work (though it never triggered the necessary hooks, so never really "worked"). Hopefully, most devs have been using BP_Group_Member_Query and/or the invitation/request related functions instead of direct table lookups. I will write up a how-to on updating code to avoid deprecation errors and such.

Regarding the notifications comment, I doubt they ever were removed (since the delete/reject hooks are all still in place, and any removal would have been tied to those hooks), but this seems like a good time to update that behavior. I haven't worked with notifications for a while, so I'll have to remember how to find the appropriate notifications to delete.

Thanks again for the thorough review and feedback!

[imath]

@dcavins

Just tested 6210-0730.diff: all good for me 🏁👏

About notification, you're right: I've checked there are not remove when a user is uninvited. As I was looking at it, I've quickly built a patch (6210-groups-uninvite-user.patch), it will save you some time 😉

Happy committing 🎉

[dcavins]

Hi @imath-

Thanks for your notifications patch! I also revisited this question this morning:

About bp_groups_migrate_invitations() I'm a little worried if there are a lot of group invites, pending requests to migrate : is there a risk of time out ?

I tested on a site with 12,000 invitations to migrate and it was able to finish the migration successfully. It took 25s and used 140MB of memory.

It is possible to manually build a bulk insert:

<?php
/**
 * Migrate invitations and requests from pre-5.0 group_members table to invitations table.
 *
 * @since 5.0.0
 */
function bp_groups_migrate_invitations() {
        global $wpdb;
        $bp = buddypress();

        $records = $wpdb->get_results( "SELECT id, group_id, user_id, inviter_id, date_modified, comments, invite_sent FROM {$bp->groups->table_name_members} WHERE is_confirmed = 0 AND is_banned = 0" );

        $processed = array();
        $values = array();
        foreach ( $records as $record ) {
                $values[] = $wpdb->prepare(
                        "(%d, %d, %s, %s, %d, %d, %s, %s, %s, %d, %d)",
                        (int) $record->user_id,
                        (int) $record->inviter_id,
                        '',
                        'bp_groups_invitation_manager',
                        (int) $record->group_id,
                        0,
                        ( 0 === (int) $record->inviter_id ) ? 'request' : 'invite',
                        $record->comments,
                        $record->date_modified,
                        (int) $record->invite_sent,
                        0
                );
                $processed[] = (int) $record->id;
        }

        $table_name = BP_Invitation_Manager::get_table_name();
        $query = "INSERT INTO {$table_name} (user_id, inviter_id, invitee_email, class, item_id, secondary_item_id, type, content, date_modified, invite_sent, accepted) VALUES ";
        $query .= implode(', ', $values );
        $query .= ';';
        $wpdb->query( $query );

        $ids_to_delete = implode( ',', $processed );
        if ( $ids_to_delete ) {
                $wpdb->query( "DELETE FROM {$bp->groups->table_name_members} WHERE ID IN ($ids_to_delete)" );
        }
}

The bulk insert is much faster and more efficient (9s and 35MB memory for the same 12,000 invitations). I'm not sure that I'm building it in the safest/most reliable way, though, so any feedback would be welcome.

[imath]

Thanks for looking at it 😍. I’d take the 9s! 😊

[dcavins]

Adds Imath's notifications cleanup and move migration to bulk insert.

[dcavins]

In 12428:

Introduce BP_Invitation and BP_Invitation_Manager.

• Add BP_Invitation, a new class describing generic invitation objects with methods for adding, updating and deleting invitations and membership requests.
• Add a creation routine for the new table that will contain invitations data.
• Add BP_Invitation_Manager, a new class that offers helper functions for managing BP_Invitation objects. For most use cases, this class will be extended for new invitation types, like group invitations.
• Add unit tests for the new classes.

See #6210.

[dcavins]

In 12429:

Introduce BP_Groups_Invitation_Manager.

• Add BP_Groups_Invitation_Manager, a new class that extends BP_Invitation_Manager for the purpose of using the new API to manage group invitations and requests.
• Migrate group invitations to the new table on update, via bp_groups_migrate_invitations().
• Update group invitation function logic to make use of BP_Groups_Invitation_Manager.

See #6210.

[dcavins]

In 12430:

Update BP_Groups_Group for Invitation API compatibility.

• Update logic in BP_Groups_Group to avoid direct table lookups and instead use request and invitation functions to fetch and delete invitations.

See #6210.

[dcavins]

In 12431:

Update BP_Groups_Member for Invitation API compatibility.

• Update logic in BP_Groups_Member to avoid direct table lookups and instead use request and invitation functions to fetch and delete invitations.

See #6210.

[dcavins]

In 12432:

Update BP_Group_Member_Query for Invitation API compatibility.

• Update logic in BP_Group_Member_Query to account for the migration of unconfirmed members to the bp_invitations table.

See #6210.

[dcavins]

In 12433:

Update invitation/request functions calls.

Update calls to group invitation and management functions in tests and functional code to take advantage of new functionality (especially by replacing workaround code in tests) and to avoid deprecation notices caused by calling invitations functions with out-of-date parameters.

See #6210.

[dcavins]

In 12434:

Invitations: Add support for messages.

• Add the optional user message to the email output notifying group admins and invitees that a request has been extended to them.
• In BP Nouveau, capture any passed message and insert it as part of the invitation creation process.

See #6210.

[dcavins]

In 12435:

Update BP_Nouveau_Group_Invite_Query for Invitations API compatibility.

See #6210.

[dcavins]

In 12436:

Invitations: Update groups_screen_group_admin_requests().

Update groups_screen_group_admin_requests() to work with new Invitations API.

See #6210.

[dcavins]

In 12437:

Invitations: Clean up notifications on acceptance.

Clean up stray notifications when an invitation is revoked.

Props imath.

See #6210.

[dcavins]

Closing as core purpose of this ticket has been accomplished. :)