groups_send_invites() should allow us to omit sending to users that have already received an invite

[r-a-y]

<?php

groups_send_invites() is the function that handles sending group invites.

In that function, there is a call to groups_get_invites_for_group(). This function determines what users to send an invite to.

However, groups_get_invites_for_group() doesn't allow us to check if we have already sent an invite to that user due to a lack of check for invite_sent = 1.

Attached patch allows for this and includes unit tests.

Note: I have not changed the default logic of groups_send_invites() to omit sending to those we have already sent invites to. However, we can if we want to.

[boonebgorges]

Thanks for opening this ticket, @r-a-y.

Under what circumstances would one want $omit_sent = false? I assume you added this param to maintain compatibility with the current behavior. But the current behavior may not be worth preserving. Here's what happens in the loop:

	for ( $i = 0, $count = count( $invited_users ); $i < $count; ++$i ) {
		$member = new BP_Groups_Member( $invited_users[$i], $group_id );

		// Send the actual invite.
		groups_notification_group_invites( $group, $member, $user_id );

		$member->invite_sent = 1;
		$member->save();
	}

Members with invite_sent=1 will be passed to groups_notification_group_invites(), but that function bails immediately for memberships with invite_sent=1. Setting $member->invite_sent = 1 obviously does nothing. So the only things that actually happen for these members is that (a) the BP_Group_Member::save() hooks fire, and (b) the already-invited users are passed to the groups_send_invites filter.

What if we simply do something like this:

	for ( $i = 0, $count = count( $invited_users ); $i < $count; ++$i ) {
		$member = new BP_Groups_Member( $invited_users[$i], $group_id );

                if ( $member->invite_sent ) {
                    continue;
                }

                // ...
	}

This preserves behavior (b) (the values passed to the filter). It changes (a), but (a) is sorta semantically incorrect anyway - no change has been made, so why fire the save actions? - and it's hard to imagine any real-world compatibility breaks.

What do you think?

[r-a-y]

I assume you added this param to maintain compatibility with the current behavior.

You assume correctly!

Members with invite_sent=1 will be passed to groups_notification_group_invites(), but that function bails immediately for memberships with invite_sent=1.

Good catch. Was writing this patch and ticket late at night and I completely missed this.

---

What do you think about using your continue approach? And also taking parts of the groups_get_invites_for_group() and BP_Groups_Group::get_invites() mods from 01.patch?

[boonebgorges]

What do you think about using your continue approach? And also taking parts of the groups_get_invites_for_group() and BP_Groups_Group::get_invites() mods from 01.patch?

Sure, that seems reasonable. The changes from 01.patch wouldn't be used in this specific fix, but they could definitely be useful for developers down the road.

[dcavins]

The other use case is being able to resend invitations in specific instances. In some old patches, I went the other way and added a force_send parameter to groups_notification_group_invites().

I think @r-a-y's suggested parameter to filter invitations by sent status is a useful improvement to the fetch functions.

[r-a-y]

In 11603:

Groups: Skip the member save routine if we've already sent an invite to the group member in groups_send_invites().

Props boonebgorges.

Fixes #7397.

[r-a-y]

In 11604:

Groups: Add fine-grained querying of invite sent status in groups_get_invites_for_group().

This commit introduces a new $sent parameter to groups_get_invites_for_group()
and BP_Groups_Group::get_invites(). With this parameter, developers can
now query group invites by sent status or not.

See #7397.