Group member count routine is bad

[boonebgorges]

There are in fact two bad things going on here, but they're closely related, so I'm lumping them together.

1. Group member count is stored in groupmeta for performance. This count is refreshed every time a group's Members page is viewed. ​https://buddypress.trac.wordpress.org/browser/tags/2.9.1/src/bp-groups/bp-groups-screens.php?marks=585#L573 [2858] This is bad. This cached value only needs updating when a member joins or leaves.

2. The method used to query the group member count is groups_get_total_member_count(), which calls BP_Groups_Group::get_total_member_count(), which makes a direct SQL query. This query doesn't take into account whether users are activated in WP (see user_status or spam in the wp_users table, it ignores whether the users in question even exist in wp_users, it is not filterable, and it is not cached. We should not use it. This should be switched to a regular group member query, with 'count' functionality added to that query class if it's not already there.

[espellcaste]

Good points and I noticed that as well when working on the REST API endpoints. There are actually other examples in the code base where direct DB queries are performed without cache.

[espellcaste]

Here is a patch which does a few things:

• Updates groups_get_total_member_count to grab from group cache if available
• Fallbacks to BP_Groups_Group::get_total_member_count
• BP_Groups_Group::get_total_member_count was updated to use groups_get_group_members so that only active users are considered (banned users are excluded)
• The "Refresh the group member count meta." on the group member template was removed. This is not necessary since there is already code to update this cache when a user leaves/join/remove from a group or when a user is deleted.
• Ticket version was updated to 1.2.3, this is when the groups_get_total_member_count function was introduced.
• Also, minor tweaks were added.

[imath]

Hi @espellcaste

Thanks a lot for your work on this. The first thing I noticed was the following code you are using :

	$bp    = buddypress();
	$group = bp_get_group( $group );

	if (
		empty( $group->id )
		&& (
			! empty( $bp->groups->current_group )
			&& is_numeric( $group ) // **This cannot be true.**
			&& $group === $bp->groups->current_group->id
		)
	) {
		$group = $bp->groups->current_group;
	}

1. I'm wondering how the $group variable can be numeric as the bp_get_group() function is returning a Group object or false.
2. I believe checking the current group should happen before using the bp_get_group() function to potentially avoid a DB query if it matches the group id.
3. I was a bit confused to see changes about things that don't relate directly to the subject of the ticket. But let's keep these improvements and maybe make sure to split the changes in two commits: one to improve groups_get_slug() (<- side note: why this one is not using bp_get_group()?), groups_leave_group(), groups_join_group(), groups_update_last_activity() ; and one other for what's directly about the Group member count routine.

Instead of the above code, I'd probably do something like this:

	$group_id      = 0;
	$current_group = null;

	if ( is_numeric( $group ) ) {
		$group_id      = (int) $group;
		$current_group = groups_get_current_group();

		if ( $current_group instanceof BP_Groups_Group && (int) $current_group->id === $group_id ) {
			$group = $current_group;
		}
	}

	if ( ! isset( $group->id )  ) {
		$group = bp_get_group( $group );
	}

Remember BuddyPress can still be used with PHP 5.6 😉: the null coalescing operator (??) is not present in PHP version 5.6 or earlier. (I advise you to run grunt commit to see the phpcompat warning.)

I confirm the PHP Unit tests are OK (with or without my suggestion).

I believe we shouldn't change the kind of returned value for refresh_total_member_count_for_group() we just need to know if the count was refreshed, not the new member count.

[espellcaste]

@imath

1 - Good catch! The lack of test for this use case made me miss it.
2 - Your suggestion is good. Going to apply it.
3 - I understand the confusion and have no trouble in adding two different commits.
4 - Another good catch!
5 - Just to note it was already returning total count before. So it was not a new change but I can certainly remove that.

I'll send two new patches with the suggestions. :)

[imath]

Hi @espellcaste

Once I've seen the changes about checking the current group, I realized we were duplicating 3 times the same code. Sorry for realizing it late. I believe we should improve bp_get_group() instead. That's mainly what I'm suggesting into ​7614.4.recommandations.patch. I've moved the template's global check earlier to always try to use a group object we already got before requesting it via the DB. I've noticed some template functions were using 0 as the parameter, eg: bp_core_get_iso8601_date( bp_get_group_last_active( 0, array( 'relative' => false ) ) ) In this case 0 is numeric and the template's global was not used with the previous code of bp_get_group().

You might wonder why I'm introducing the 'bp_groups_set_current_group' hook. That's to:

• take in account early BP Rewrites need (the current group is set later during bp_parse_request. So it's a way to avoid the BP Rewrites plugin to generate doing it wrong notices.
• make sure the current group had a chance to be set before checking it.

Otherwise, the rest of the patch looks good. That's why I applied your patch and generated the .recommandations.patch after

You'll probably need to reference #6749 if you agree with these changes.

[espellcaste]

In 13103:

Improving the group member count routine and the function helper.

The group member count routine was updated to avoid direct, uncached, SQL query and unnecessary cache refresh when a group's Members page was viewed.

is now being used to get the group member count which takes into account users' existence in the site,

the query is now cached and filterable.

was also updated to get the current group from if available.

Props imath
Fixes #7614 and see #6749

[espellcaste]

In 13104:

Update group-related functions so that they use the new bp_get_group helper.

groups_get_slug, groups_leave_group, groups_join_group, and groups_update_last_activity were updated
to get a group using the recently added function bp_get_group, allowing them to be fetched with more than just IDs.

See #7614

[imath]

In 13129:

Avoid using legacy args with groups_get_group_members()

Instead of using the 'exclude_banned' and/or 'exclude_admins_mods' args, it's best to use the 'group_role' argument & pass the list of roles to include in the count (in this case all roles except the banned one).

See #7614

[imath]

We need to reopen this ticket and take some more time about it. Here are a few examples where the "only update a group count on members join/leave" strategy can generate a wrong count.

An admin creates a user and add it to a group, the group members count won't take this user in account because the user is not active yet. When the user activate their account, as they are already member, the count stays wrong.

[sjregan]

Here is another example (in v10.3.0) of how a group member count can end up incorrect. I am not sure if this is the same issue described above or a variation.

1. Create a new user via WP Admin e.g. username testuser (do not login within them)
2. Create a group
3. Edit the group in WP Admin
4. Add 'testuser' to the group in the 'Add New Members' search field.
5. Click 'Save Changes'
6. Click 'Edit' under newly added member (testuser) within 'Manage Members'
7. Change their role to Administrator (wait for AJAX to complete, otherwise you get an 'No route was found matching the URL and request method.' error unrelated to this issue)
8. Click 'Remove' under the user which created the group.
9. Click 'Save Changes'

The group member count will be zero when there is a group member.

[espellcaste]

@imath I'm reassigning the ticket since based on the last ​dev chat, you are taking care of it.

[dcavins]

The "not active" user belonging to a group is an interesting problem. It seems the only way to add a not-yet-active user to a group is via the form at WP Admin > Groups > Edit Group. If you add a user like this, then you see odd mismatches in the groups UI like

The fix we talked about in the dev chat was updating the group counts on the bp_first_activity_for_member action hook. Another possibility is limiting the "add user" interface to only use "active" site members. That would avoid problems in the groups UI, but would be a change.

[dcavins]

Out-of-sync membership lsit.

[dcavins]

WP Admin Group edit UI

[sjregan]

@dcavins In regards to your comment 'Another possibility is limiting the "add user" interface to only use "active" site members.' I would like to share a common scenario for the communities we build for clients.

Often, the communities are for businesses or organisations where one or two staff members will setup private groups, and invite other staff members to join specific groups based on their role within the business. The invitees receive the invitation email, complete their profile and immediately have the access to their required groups for their responsibilities within the business.

So while it may seem odd that people would want to add inactive users to a group, it is useful reason for some.

[dcavins]

Replying to sjregan:

So while it may seem odd that people would want to add inactive users to a group, it is useful reason for some.

Thanks for your feedback! I always assume that a behavior that seems odd to me has a use case somewhere in the BP universe. :)

If we are ok with adding not-yet-active users to groups and showing them in the members list, maybe the simplest (and most logical) answer is to just include those users in the count? The exclusion happens in BP_Group_Member_Query: ​https://github.com/buddypress/buddypress/blob/master/src/bp-groups/classes/class-bp-group-member-query.php#L544-L564

If the count parameter is removed from BP_Groups_Group::get_total_member_count() then this validation step is skipped and the count returns all group members. ​https://github.com/buddypress/buddypress/blob/master/src/bp-groups/classes/class-bp-groups-group.php#L1781-L1817

And, with that change, I can verify that when a non-active user is added, the count is incremented. (And if that user is deleted before logging in, the group count is decremented.)

[dcavins]

Change BP_Groups_Group::get_total_member_count() behavior.

[dcavins]

Another way that we could resolve the logic mismatch (of seeing not-yet-active members in a group's list) is to rename the count parameter to exclude_ghosts or exclude_inactive (something that actually explains what it does) and use it by default in groups_get_group_members() (but make it a param so that the behavior could be overriden). This could be expensive, though, since it fetches every active user to use as a filter comb.

[imath]

Hi @dcavins & @sjregan

Thanks a lot for your contributions. It's a complex issue. I need to point you to #8688 to add some context about why I've reopened this ticket. I've introduced the count parameter at this time to avoid populating group member extra data when it's not needed (like when counting members) as it caused performance issues on profile.wordpress.org.

So I'm not very comfortable with your suggestion @dcavins ( using 'count' => false ), because we'd probably be back to a poor performance situation when a group has many members like on profiles.wordpress.org.

Although I understand @sjregan's need, I'd rather take "the restrict Admin UI to active users only" direction. By the way, reading your comment, I don't see why restricting the Admin UI has an impact about

The invitees receive the invitation email, complete their profile and immediately have the access to their required groups for their responsibilities within the business

If the above process is automatic, why do you need the Admin UI for ?

[sjregan]

@imath The sending of invitations is automated, the process of placing (inactive) users into groups is a manual process.

If the client can't add an inactive user to a group, then they will end up with a poor workflow:

• Invite X number of users to site.
• Wait until a user accepts invitation and becomes 'active'.
• (Custom code will need to be written to:) Send an email to the client notifying them a user is now active.
• Client will login to site, look up internal documentation as to which groups a user should belong to, assign the user to their groups
• Repeat every time a user accepts invitation

Between an invitee accepting an invitation and the client logging in and assigning them to groups, the invitee will not have access to any of the resources they should.

[imath]

Thanks for your feedback @sjregan. Let’s try something else @dcavins could you check what happens if instead of removing the count parameter in BP_Groups_Group::get_total_member_count() you add the type parameter and set it to 'alphabetical' If my intuition is right, then it should skip the active user check and include the users added by the Administrator from the Admin UI. Some PHPUnit tests should fail, so we’d probably have to edit these…

[imath]

Ah made some quick test, I guess it was a wrong intuition, I’ll give it to another look. I made the commit 6 months ago and need to refresh my memory 🤔

[dcavins]

Here's a patch that solves the initial problem: recounting groups when a user logs in for the first time. :)

[dcavins]

Update group member counts for groups that a user belongs to when that user logs in for the first time.

[dcavins]

To a longer-term solution, I wonder if tracking those who have never logged in would be more efficient than tracking those who have logged in. On a well-maintained system, I'd hope that there were more real users than cruft. Can we ask the profiles team for example, to compare the total number of User IDs/signups to the result of the following query?

SELECT count(distinct `user_id`)
FROM `wp_bp_activity`
where `type` = 'last_activity'

Adds a bp_moderate capability check before eventually remove inactive users from groups members count.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/7614

[imath]

@dcavins Thanks a lot for your interesting patch. I'm pretty sure the logic you used will be useful to repair some out of sync user data. I just added a PR, I believe is enough to fix this ticket. I finally understood what I did 6 months ago. Sorry it took me this long and that I made you work on this direction.

What the PR does: it simply doesn't apply the inactive users neutralization from count when origin was an Admin action.

What do you think of it @dcavins ?

[dcavins]

Replying to imath:

@dcavins Thanks a lot for your interesting patch. I'm pretty sure the logic you used will be useful to repair some out of sync user data. I just added a PR, I believe is enough to fix this ticket. I finally understood what I did 6 months ago. Sorry it took me this long and that I made you work on this direction.

What the PR does: it simply doesn't apply the inactive users neutralization from count when origin was an Admin action.

What do you think of it @dcavins ?

I think your patch is fine. I was a little wary because it adds some hard-to-explain logic, but it seems OK. Any accidental overreach of the logic is so minimal as to not worth bothering about. (We are only talking about the group count here, so if we become aware of an edge case that blows it up, well, we can fix it.)

[imath]

Thanks a lot for your feedback @dcavins Let's have this in! I'll commit the PR asap.

[imath]

In 13357:

Eventually include inactive users in group's members count

When the Community Site Administrator decides to add an inactive user
as a member of a group from the Group's WP Admin screen, we need to
include them into the group's members count to remain consistent with the
fact:

• an activity is created inside the group to inform this inactive user

joined the group

• the group's members list is including this user.

Props dcavins, sjregan, espellcaste

Closes ​https://github.com/buddypress/buddypress/pull/33
Fixes #7614