WordPress Network Admin Users unspam after BuddyPress user's profile capabilities spam doesn't unspam user

[imath]

while testing #5114 patch, i've noticed that in a Multisite config if i mark a user as spammer from the front end (site.url/members/user/settings/capabilities/) and i unspam the user from the WordPress Network Administration screen for users using the bulk action then the user seems to be unspammed from this screen.
But logging in with the user is forbidden as he's still consider as a spammer. The SuperAdmin needs to go to user's profile on front end (site.url/members/user/settings/capabilities/) to unmark user as a spammer to finally let him log in.

• When marking a user as spammer
  • from Front end : $wpdb->users fields 'user_status' and 'spam' are set to 1
  • from WordPress Network Admin users list : only the field 'spam' is set to 1 in $wpdb->users
• When unmarking a user as spammer
  • from Front end : $wpdb->users fields 'user_status' and 'spam' are set to 0
  • from WordPress Network Admin users list : only the field 'spam' is set to 0 in $wpdb->users

At the beginning, I thought as WordPress doesn't seem to use this field at all, in a Multisite config we shouldn't update at all this user_status field, but i realized that doing so could cause other bugs, as there must be a lot of spammed users with a user_status field set to 1 and a spam field set to 1 in the different Multisite configs using BuddyPress, and in a case of a Multisite to single switch, spammed users might become unspammed..

So i think the best solution in bp_core_process_spammer_status() is to run the user_status update query not only if !is_admin but always. This way the behavior will be the same in the two contexts. See attached diff.

[imath]

I forgot to mention : i think in bp_core_get_active_member_count() user_status is "doubled" so i've replaced one with spam

[r-a-y]

Thanks for the thorough analysis, imath.

in a Multisite config we shouldn't update at all this user_status field, but i realized that doing so could cause other bugs, as there must be a lot of spammed users with a user_status field set to 1 and a spam field set to 1 in the different Multisite configs using BuddyPress, and in a case of a Multisite to single switch, spammed users might become unspammed..

Same with switching from a single-site to multisite install. I think your patch is an improvement. Though, there might be some reasons as to why the current code is the way it is. Need another core dev to chime in.

If we're thinking of the current codebase, the problem lies in the bp_is_user_spammer() function and the way we're checking the spam and user_status fields.

I've attached an initial patch that addresses the problem without needing imath's patch. However, we should definitely look at imath's patch as well.

[boonebgorges]

I'm pretty sure that our system for using the user_status column for spamming on non-MS is a sort of hack for the fact that the concept of "spam user" doesn't really exist natively in non-MS. It looks like we're probably being inconsistent in the way that we've implemented it.

Moving to 2.0 so this doesn't get lost in the shuffle.

[DJPaul]

Where are we with this? Do we need to re-evaluate the approach, or is it just a case of tidying up the patch?

[r-a-y]

Where are we with this? Do we need to re-evaluate the approach, or is it just a case of tidying up the patch?

I think both patches have merit.

imath's patch needs to be reviewed a bit based on the WPMU 'wp_signups' abstraction that was added awhile back, but other that, the approach is solid.

As for my patch, bp_core_boot_spammer() should be using bp_is_user_spammer() to do checks instead of another inline-based algorithm.

[imath]

Replying to r-a-y:

imath's patch needs to be reviewed a bit based on the WPMU 'wp_signups' abstraction

Thanks r-a-y for reminding me this. Actually, the only problem i can see in this area would be that an admin could display the signup account in front end using site.url/members/signup_account for instance. And then go to the settings screen and spam the user. In this case it might be problematic as the status would no more be '2' but '1'. So to prevent this case, i think we should check the user_status before setting the displayed user_id.

That's what i first did in 5275.02.patch.

Then, i've added unit tests and could check that if we only run
$wpdb->update( $wpdb->users, array( 'user_status' => $is_spam ), array( 'ID' => $user_id ) ); on single site, then unspaming the user from a bulk action of the network admin will fail. So i think the fix is to move this update query to be sure it will be played no matter the config as i did in 5275.02.patch.

This patch will contain the fix + unit tests. I'm also adding 5275.solotests.patch if you want to check that there's an issue in the test test_bp_core_process_spammer_status_ms_bulk_ham() on multisite configs without applying 5275.02.patch.

[imath]

Just updated the patch (.03.patch) to latest trunk, if no objections, i will commit it tomorrow.

[DJPaul]

Why do the tests need to call clean_user_cache? I would normally expect functions that modify the state of the user to clear any caches as relevant.

[imath]

Thanks for your feedback DJPaul, you're absolutely right.

This is due to the fact, we're actually not cleaning user's cache in bp_core_process_spammer_status() once we updated the user_status field of the wp_users table.

I've look at bp_core_process_spammer_status() more deeply and came to the conclusion some code were not used anymore : i'm talking about the $is_admin check. It's now possible to manage the spam status of the user on front end or in administration screens whatever the multisite config is.

Now i think the $do_wp_cleanup part only concerns multisite configs. This variable is false only if WordPress already did the cleanup, which means it's a multisite config. So if true, if think we only have to do the multisite clean up : it's happening when i spam a user from front end or from the wp-admin/user extended profile (since 2.0) or from the spam row action since (2.1 if i remember well). So i moved the "BuddyPress" specific logic (user_status field set to 1) for spamming a user out of this part because it needs to happen whatever the config is.

I've also added clean_user_cache( $user_id ); just after the user_status field of the wp_users table has been updated so that we make sure bp_core_process_spammer_status() is actually cleaning the user's cache.

I've removed the clean_user_cache() from every tests.

I'd be very thankful if you could take a look at 5275.04.patch the sooner you can because i think we should have this fixed for 2.2.

[DJPaul]

I think this is OK.

[imath]

5275.05.patch is updated to trunk revision 9397 and i moved the tests into the members directory as i've found some tests about bp_core_process_spammer_status() there, so i thought it was best to have it all in one place.

[imath]

In 9398:

On multisite configs, make sure unspamming the user from the Users Network Administration screen is updating the .

When a user is marked as spam from the front end, then marked as ham from the network-users administration screen using the 'unspam' bulk action, we need to be sure the field of the users table is updated so that BuddyPress effectively takes in account the user has been unspammed.

This commit also makes sure the user's cache has been cleared.

Fixes #5275

[johnjamesjacoby]

Replying to imath:

I forgot to mention : i think in bp_core_get_active_member_count() user_status is "doubled" so i've replaced one with spam

See #6155. I just found this on my own last night.