WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 7 months ago

#22917 closed enhancement (fixed)

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

Reported by: vmaia Owned by: 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 16 months ago.
22917-2.diff (2.6 KB) - added by adamsilverstein 15 months ago.
new patch adds filter to turn on live update
22915-unittest.diff (2.0 KB) - added by adamsilverstein 15 months ago.
22917.unittest.diff (2.0 KB) - added by adamsilverstein 14 months ago.
updated/cleaned up unit test for 22917
22917.3.diff (2.6 KB) - added by SergeyBiryukov 14 months ago.
Same as 22917-2.diff, with minor formatting fixes
22917.diff (2.6 KB) - added by adamsilverstein 13 months ago.
refresh
22917.2.diff (2.6 KB) - added by adamsilverstein 13 months ago.
refresh named properly
22917.4.diff (2.9 KB) - added by adamsilverstein 12 months ago.
uses wp_is_large_network to set live count update defaults
22917.5.diff (5.1 KB) - added by jeremyfelt 7 months ago.
22917.6.diff (6.3 KB) - added by jeremyfelt 7 months ago.
22917.7.diff (6.5 KB) - added by nacin 7 months ago.

Download all attachments as: .zip

Change History (41)

comment:1 Ipstenu16 months ago

  • Cc ipstenu@… added

comment:2 nacin16 months ago

  • Milestone changed from Awaiting Review to 3.5.1

comment:3 Ipstenu16 months 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.

comment:4 ocean9016 months 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

comment:5 wpmuguru16 months 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.

comment:6 follow-up: vmaia16 months ago

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

comment:7 in reply to: ↑ 6 wpmuguru16 months 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.

comment:8 nacin16 months 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.

comment:9 adamsilverstein16 months 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.

comment:10 adamsilverstein16 months 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.

comment:11 follow-up: wpmuguru16 months 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 16 months ago by wpmuguru (previous) (diff)

comment:12 in reply to: ↑ 11 ; follow-up: adamsilverstein16 months 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?

adamsilverstein15 months ago

new patch adds filter to turn on live update

comment:13 adamsilverstein15 months ago

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

comment:14 follow-up: wpmuguru15 months ago

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

comment:15 in reply to: ↑ 14 adamsilverstein15 months 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.

comment:16 adamsilverstein15 months ago

  • Keywords dev-feedback added

comment:17 adamsilverstein14 months 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

adamsilverstein14 months ago

updated/cleaned up unit test for 22917

SergeyBiryukov14 months ago

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

comment:18 in reply to: ↑ 12 ; follow-up: SergeyBiryukov14 months 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.

adamsilverstein13 months ago

refresh

adamsilverstein13 months ago

refresh named properly

adamsilverstein12 months ago

uses wp_is_large_network to set live count update defaults

comment:19 in reply to: ↑ 18 adamsilverstein12 months 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 :)

comment:20 adamsilverstein12 months ago

  • Keywords needs-testing added

comment:21 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

comment:22 jeremyfelt8 months ago

  • Milestone changed from Future Release to 3.7

Related #21341

comment:23 follow-up: nacin8 months 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.

jeremyfelt7 months ago

comment:24 in reply to: ↑ 23 ; follow-up: jeremyfelt7 months 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.

comment:25 in reply to: ↑ 24 adamsilverstein7 months 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 :)

jeremyfelt7 months ago

comment:26 jeremyfelt7 months 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().

nacin7 months ago

comment:27 follow-ups: nacin7 months 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.

comment:28 in reply to: ↑ 27 adamsilverstein7 months 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.

comment:29 in reply to: ↑ 27 jeremyfelt7 months 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.

comment:30 nacin7 months 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.

Note: See TracTickets for help on using tickets.