Skip to:
Content

Opened 4 months ago

Last modified 3 weeks ago

#7397 new enhancement

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

Reported by: r-a-y Owned by:
Milestone: 2.9 Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch
Cc:

Description (last modified by boonebgorges)

<?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.

Attachments (1)

7397.01.patch (6.1 KB) - added by r-a-y 4 months ago.

Download all attachments as: .zip

Change History (8)

@r-a-y
4 months ago

#1 @r-a-y
4 months ago

  • Description modified (diff)

#2 @boonebgorges
4 months ago

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?

#3 @r-a-y
4 months ago

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?

#4 @boonebgorges
4 months ago

  • Description modified (diff)

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.

#5 @dcavins
4 months ago

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.

#6 @DJPaul
3 months ago

  • Milestone changed from Awaiting Review to 2.9

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


3 weeks ago

Note: See TracTickets for help on using tickets.