Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#22917 closed enhancement (fixed)

Allow Live Updating of User & Site number in Multisite network dashboard 'Right Now' meta box

Reported by: vmaia's profile vmaia Owned by: nacin's profile nacin
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.5
Component: Multisite Keywords: has-patch commit
Focuses: Cc:

Description

Wordpress installation don’t show the correct number of users and sites correctly registered in the site.

Please see images for reference:

Attachments (11)

right-now-live.diff (2.3 KB) - added by adamsilverstein 11 years ago.
22917-2.diff (2.6 KB) - added by adamsilverstein 11 years ago.
new patch adds filter to turn on live update
22915-unittest.diff (2.0 KB) - added by adamsilverstein 11 years ago.
22917.unittest.diff (2.0 KB) - added by adamsilverstein 11 years ago.
updated/cleaned up unit test for 22917
22917.3.diff (2.6 KB) - added by SergeyBiryukov 11 years ago.
Same as 22917-2.diff, with minor formatting fixes
22917.diff (2.6 KB) - added by adamsilverstein 11 years ago.
refresh
22917.2.diff (2.6 KB) - added by adamsilverstein 11 years ago.
refresh named properly
22917.4.diff (2.9 KB) - added by adamsilverstein 11 years ago.
uses wp_is_large_network to set live count update defaults
22917.5.diff (5.1 KB) - added by jeremyfelt 11 years ago.
22917.6.diff (6.3 KB) - added by jeremyfelt 10 years ago.
22917.7.diff (6.5 KB) - added by nacin 10 years ago.

Download all attachments as: .zip

Change History (42)

#1 @Ipstenu
11 years ago

  • Cc ipstenu@… added

#2 @nacin
11 years ago

  • Milestone changed from Awaiting Review to 3.5.1

#3 @Ipstenu
11 years ago

I am 99.999% sure I saw this on 3.4.2 as well as 3.5 with a guy at DreamHost, but the 3.5 upgrade fixed him.

See http://wordpress.org/support/topic/missing-user-site-number-in-wp-adminnetworkindexphp?replies=8 for previous work done.

#4 @ocean90
11 years ago

Have in mind, that these numbers are cached.

From get_user_count(): The count is cached and updated twice daily. This is not a live count.

#16367, #15567

#5 @wpmuguru
11 years ago

This count depends on cron. If cron is not working correctly or the site has no page views then the counts will not be updated. Only one cron action is executed with each page view.

#6 follow-up: @vmaia
11 years ago

I tested in different servers, including dedicated and shared (Hostgator), and have the same result. No count.

#7 in reply to: ↑ 6 @wpmuguru
11 years ago

Replying to vmaia:

I tested in different servers, including dedicated and shared (Hostgator), and have the same result. No count.

You can use http://wordpress.org/extend/plugins/cron-debug-log/ to test and see if your cron is working. It's very rudimentary but it will show if you are having errors on cron.

#8 @nacin
11 years ago

  • Milestone changed from 3.5.1 to 3.6

If this is a bug, I highly doubt it is a regression, as these counts have been counting the same way since around 3.1.

#9 @adamsilverstein
11 years ago

  • Cc ADAMSILVERSTEIN@… added

i tried testing this bug, i added a few sites to a network site setup and the user/site count was correct once i ran the wp_update_network_counts() function from core control.

on my setups it looks like this gets updated the next time the cron ran, maybe hostgator isn't firing the cron correctly?

any reason not to update those values right after changing the number of users or sites when these are added/removed? too intensive on a large network?

seems a little confusing for network admin that after they add/remove site/user (s), the count is out of sync. this would also remove the reliance on the cron firing. also, since a user can be added independently of a blog, it would make sense to have a separate function for each count. anyway this doesn't seem like a bug unless there is a new issue with how cron gets fired.

#10 @adamsilverstein
11 years ago

  • Keywords has-patch added

patch summary:

in ms-function.php:

  • split wp_update_network_counts into separate functions for users, blogs. that way both potentially heavy queries don't have to be run when only the count of users or blogs has changed. both queries are still run by the original function, called by cron, and when a site is added along with a user (the normal case)
  • added recount calls when a user or blog is added

in ms.php

  • added recount calls when a user or blog is deleted

notes:

i'm not really sure i placed the calls in the right place, feel free to correct me. i think i mentioned, i'm new here.

I did some testing on my local multisite install and the updates are now correct immediately after adding or removing a user (or users) from the network admin or from a site dashboard. also adding a site or removing site(s) updates the count immediately.

i did not test my patch against a non-multisite config, but assume it works.

#11 follow-up: @wpmuguru
11 years ago

right-now-liv.diff would render the caching on this mostly worthless for large networks with frequent user/site creation. Someone who has a small network that wants to have live stats can easily accomplish this with a small plugin

add_action( 'wpmu_new_blog', 'wp_update_network_counts' );
add_action( 'wpmu_new_user', 'wp_update_network_counts' );
// etc.

recommend wontfix.

Last edited 11 years ago by wpmuguru (previous) (diff)

#12 in reply to: ↑ 11 ; follow-up: @adamsilverstein
11 years ago

Replying to wpmuguru:

right-now-liv.diff would render the caching on this mostly worthless for large networks with frequent user/site creation. Someone who has a small network that wants to have live stats can easily accomplish this with a small plugin

add_action( 'wpmu_new_blog', 'wp_update_network_counts' );
add_action( 'wpmu_new_user', 'wp_update_network_counts' );
// etc.

recommend wontfix.

we could add a check for large sites before running the update, but i agree my patch renders caching useless. still the current behavior is confusing to novice users, and for small networks without frequent user/site creation i think the caching is superfluous.

would you accept a patch that, instead of recalculating the entire count, simply pulled and updated the current count - incremented/decremented based on the current action? no query would be required, just the option updated. same locations as my existing changes i believe.

caching is great, but in this case its confusing the end user. it almost sounds like you are saying the ticket itself should be thrown out, that the current behavior is fine as is.

do you agree with the ticket idea that the widget should show the live count, or do you think the caching is more important and things should stay as they are? could we use wp_is_large_network() to split up the cases?

@adamsilverstein
11 years ago

new patch adds filter to turn on live update

#13 @adamsilverstein
11 years ago

new patch adds hooks to turn on live updating - off by default; also added unit test.

#14 follow-up: @wpmuguru
11 years ago

22917-2.diff looks fine to me and is a good compromise.

#15 in reply to: ↑ 14 @adamsilverstein
11 years ago

Replying to wpmuguru:

22917-2.diff looks fine to me and is a good compromise.

glad you like it, this also gives us an easy way to turn on live updating automatically for small networks if we decide thats a good idea.

also we could add a message next to the site/user count in the 'right now' meta box to indicate the count isn't live (unless it is), which is point of the initial confusion.

#16 @adamsilverstein
11 years ago

  • Keywords dev-feedback added

#17 @adamsilverstein
11 years ago

  • Component changed from General to Multisite
  • Summary changed from Missing User & Site number in Multisite to Allow Live Updating of User & Site number in Multisite network dashboard 'Right Now' meta box
  • Type changed from defect (bug) to enhancement

@adamsilverstein
11 years ago

updated/cleaned up unit test for 22917

@SergeyBiryukov
11 years ago

Same as 22917-2.diff, with minor formatting fixes

#18 in reply to: ↑ 12 ; follow-up: @SergeyBiryukov
11 years ago

Replying to adamsilverstein:

could we use wp_is_large_network() to split up the cases?

I guess we could use it to set the default value for the filter. It needs to be moved to ms-functions.php in that case though, see #23683.

@adamsilverstein
11 years ago

refresh

@adamsilverstein
11 years ago

refresh named properly

@adamsilverstein
11 years ago

uses wp_is_large_network to set live count update defaults

#19 in reply to: ↑ 18 @adamsilverstein
11 years ago

Replying to SergeyBiryukov:

Replying to adamsilverstein:

could we use wp_is_large_network() to split up the cases?

I guess we could use it to set the default value for the filter. It needs to be moved to ms-functions.php in that case though, see #23683.

22917.4.diff​ adds wp_is_large_network calls to set defaults for live updating; haven't tested yet :)

#20 @adamsilverstein
11 years ago

  • Keywords needs-testing added

#21 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#22 @jeremyfelt
11 years ago

  • Milestone changed from Future Release to 3.7

Related #21341

#23 follow-up: @nacin
11 years ago

Could use a tidying but the patch looks good. One option could be to have a maybe_update_*-style function that then runs the filter, to consolidate the logic. It might also make sense for it to be the same filter — enable_live_network_counts, with context passed for 'users' or 'sites'. Just tossing out ideas, feel free to bat them down.

@jeremyfelt
11 years ago

#24 in reply to: ↑ 23 ; follow-up: @jeremyfelt
11 years ago

  • Keywords dev-feedback needs-testing removed

22917.5.diff is a refresh against trunk. The patch combines tests and src into one and cleans things up a bit.

Replying to nacin:

One option could be to have a maybe_update_*-style function that then runs the filter, to consolidate the logic.

Skipped this for now. Open to the idea, as the opposites generated through the ! wp_is_large_network() stuff are a bit strange feeling.

It might also make sense for it to be the same filter — enable_live_network_counts, with context passed for 'users' or 'sites'.

Went with this in the updated approach. Should be nicer to add this filter as a dev and react on context. Likely the return will be the same for both anyway.

Also updated the tests a bit. By moving to one filter name, it highlighted the necessity to use remove_filter() for both tests to avoid conflicts.

The new enable_live_network_counts filter has docs in this patch as well.

#25 in reply to: ↑ 24 @adamsilverstein
11 years ago

Replying to jeremyfelt:

22917.5.diff is a refresh against trunk. The patch combines tests and src into one and cleans things up a bit.

Replying to nacin:

One option could be to have a maybe_update_*-style function that then runs the filter, to consolidate the logic.

Skipped this for now. Open to the idea, as the opposites generated through the ! wp_is_large_network() stuff are a bit strange feeling.

It might also make sense for it to be the same filter — enable_live_network_counts, with context passed for 'users' or 'sites'.

Went with this in the updated approach. Should be nicer to add this filter as a dev and react on context. Likely the return will be the same for both anyway.

Also updated the tests a bit. By moving to one filter name, it highlighted the necessity to use remove_filter() for both tests to avoid conflicts.

The new enable_live_network_counts filter has docs in this patch as well.

This looks good, thanks! I like your improvement by combining the filters. Cheers :)

@jeremyfelt
10 years ago

#26 @jeremyfelt
10 years ago

22917.6.diff changes a bit from the last approach:

  • Introduce wp_update_network_counts_maybe_update() to check a single occurrence of the enable_live_network_counts filter rather than quadruplicating efforts. There's not much prior art for the naming of that function, so I followed the new wp_auto_updates_maybe_update() as an example.
  • Moves the possible count update for users from wpmu_create_user() to wp_insert_user()
  • Moves the possible count update for sites from install_blog() to insert_blog()
  • Updates tests for get_user_count() now that the count is refreshed by default in wp_insert_user().

@nacin
10 years ago

#27 follow-ups: @nacin
10 years ago

  • Keywords commit added

Based on the queries, these counts also need to change whenever a site is (un)deleted, (un)archived, (un)spammed, and whenever a user is (un)spammed. (Also whenever a user's deleted bit is flipped, but I can't actually find that occurring anywhere in core, nor is there an action.)

I chatted with jeremyfelt in IRC about how we could and should use hooks for this — except for in insert_blog(). 22917.7.diff is what I was thinking. I've tested it out for the different situations and it works well. It's also a simpler diff, obviously.

#28 in reply to: ↑ 27 @adamsilverstein
10 years ago

Replying to nacin:

Based on the queries, these counts also need to change whenever a site is (un)deleted, (un)archived, (un)spammed, and whenever a user is (un)spammed. (Also whenever a user's deleted bit is flipped, but I can't actually find that occurring anywhere in core, nor is there an action.)

I chatted with jeremyfelt in IRC about how we could and should use hooks for this — except for in insert_blog(). 22917.7.diff is what I was thinking. I've tested it out for the different situations and it works well. It's also a simpler diff, obviously.

Brilliant polish! Appreciate how the query keyed you into the other contexts and also how this ticket evolved into such a broad yet concise solution.

The only thing I can think to add would be more tests verifying the other (un)(delete|archived|spammed) actions and count updates, ensuring these continue working properly.

#29 in reply to: ↑ 27 @jeremyfelt
10 years ago

Replying to nacin:

I chatted with jeremyfelt in IRC about how we could and should use hooks for this — except for in insert_blog(). 22917.7.diff is what I was thinking. I've tested it out for the different situations and it works well. It's also a simpler diff, obviously.

Ahh, I see what you meant by using the action now. Dig it.

#30 @nacin
10 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 25621:

Live network counts of users and sites for small networks.

props adamsilverstein, jeremyfelt.
fixes #22917.

#31 @SergeyBiryukov
10 years ago

#25583 was marked as a duplicate.

Note: See TracTickets for help on using tickets.