Opened 7 years ago
Closed 2 years ago
#7614 closed enhancement (fixed)
Group member count routine is bad
Reported by: | boonebgorges | Owned by: | imath |
---|---|---|---|
Milestone: | 11.0.0 | Priority: | normal |
Severity: | normal | Version: | 1.2.3 |
Component: | Groups | Keywords: | has-patch has-unit-tests commit |
Cc: | dcavins |
Description
There are in fact two bad things going on here, but they're closely related, so I'm lumping them together.
- 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.
- The method used to query the group member count is
groups_get_total_member_count()
, which callsBP_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 (seeuser_status
orspam
in thewp_users
table, it ignores whether the users in question even exist inwp_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.
Attachments (9)
Change History (44)
#1
@
3 years ago
- Milestone changed from Awaiting Contributions to 10.0.0
- Owner set to espellcaste
- Status changed from new to assigned
#2
@
3 years ago
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
- Version set to 1.2.3
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 usegroups_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.
This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.
3 years ago
#4
@
3 years ago
- Keywords dev-reviewed needs-refresh added; good-first-bug has-patch removed
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; }
- I'm wondering how the
$group
variable can be numeric as thebp_get_group()
function is returning a Group object orfalse
. - 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. - 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 usingbp_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.
This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.
3 years ago
#6
@
3 years ago
@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. :)
#7
@
3 years ago
- Keywords 2nd-opinion added; needs-refresh removed
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.
This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.
3 years ago
#12
@
3 years ago
- Keywords needs-patch added; has-unit-tests dev-reviewed 2nd-opinion removed
- Milestone changed from 10.0.0 to 11.0.0
- Resolution fixed deleted
- Status changed from closed to reopened
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.
#13
@
2 years ago
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.
- Create a new user via WP Admin e.g. username testuser (do not login within them)
- Create a group
- Edit the group in WP Admin
- Add 'testuser' to the group in the 'Add New Members' search field.
- Click 'Save Changes'
- Click 'Edit' under newly added member (testuser) within 'Manage Members'
- 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)
- Click 'Remove' under the user which created the group.
- Click 'Save Changes'
The group member count will be zero when there is a group member.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
2 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
2 years ago
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
2 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
2 years ago
#19
@
2 years ago
- Owner changed from espellcaste to imath
- Status changed from reopened to assigned
@imath I'm reassigning the ticket since based on the last dev chat, you are taking care of it.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
2 years ago
#21
@
2 years ago
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.
#22
follow-up:
↓ 23
@
2 years ago
@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.
#23
in reply to:
↑ 22
@
2 years ago
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.)
#24
@
2 years ago
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.
#25
@
2 years ago
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 ?
#26
@
2 years ago
@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.
#27
@
2 years ago
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…
#28
@
2 years ago
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 🤔
#29
@
2 years ago
Here's a patch that solves the initial problem: recounting groups when a user logs in for the first time. :)
@
2 years ago
Update group member counts for groups that a user belongs to when that user logs in for the first time.
#30
@
2 years ago
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'
This ticket was mentioned in PR #33 on buddypress/buddypress by @imath.
2 years ago
#31
- Keywords has-patch has-unit-tests added; needs-patch removed
Adds a bp_moderate
capability check before eventually remove inactive users from groups members count.
Trac ticket: https://buddypress.trac.wordpress.org/ticket/7614
#32
follow-up:
↓ 33
@
2 years ago
- Keywords dev-feedback added
@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 ?
#33
in reply to:
↑ 32
@
2 years ago
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.)
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.