10up / distributor

Share content between your websites.
https://distributorplugin.com
GNU General Public License v2.0
628 stars 155 forks source link

Method for assembling list of internal connections does not work well on large installations #1176

Open boonebgorges opened 8 months ago

boonebgorges commented 8 months ago

Describe the bug

The mechanism used by Distributor to assemble a list of internal connections (used on the 'Pull Content' dropdown and elsewhere) uses get_sites( [ 'number' => 1000 ] ) to pull a list of all sites on the network, then iterates through to see whether the current user has posting permissions on any of those sites. This strategy does not work well on the sorts of installations where there are thousands of sites, since any site beyond the initial 1000 will never appear in the list. (This came up in the context of a client site where the network has many thousands of sites, but the vast majority of users are members of only a few handful of them.)

By contrast, if one uses get_blogs_of_user() or some other WP mechanism that only identifies the sites on which a user has a role, the list will contain all of a user's sites. (As a bonus, it performs far better.)

This change would mean, however, that a super admin would not see sites where the super admin does not have a role. This might not work well for some use cases of the plugin.

For the time being, I've effected the get_blogs_of_user() change using the 'dt_pre_get_authorized_sites' filter. But I thought I'd open the ticket, because I suspect that the use case I described (large networks where users are members of many sites) is perhaps more common than the situation best served by the current strategy (a super admin who needs access to all sites). In any case, I would argue that a dropdown containing 1000 items is not particularly useful, and in the case where wp_is_large_network() (or something similar) this interface ought to be an AJAX-powered type-ahead or something like that.

Thanks for the plugin :-D

Steps to Reproduce

  1. Have a large network
  2. Log in as a user who owns at least two sites. At least one of them (site A) should have ID > 1000
  3. Go to Site B > Dashboard > Distributor > Pull Content
  4. Site A will not be in the dropdown

Screenshots, screen recording, code snippet

No response

Environment information

No response

WordPress information

No response

Code of Conduct

jeffpaul commented 7 months ago

@boonebgorges perhaps a conditional there based on role such that super admins on particularly large installs maybe have things under perform but other users get the discrete listing of their sites more quickly? Though the AJAX approach probably suffices for cases of particularly long listings.

boonebgorges commented 7 months ago

@jeffpaul Thanks for the quick response and for the thoughts. I think a pure AJAX solution is probably the one that will produce the fewest problems for the most people. However, this kind of interface is suboptimal for the kind of users I describe. If I'm a member of four or five sites, I'd probably prefer to see a simple dropdown with a list of my sites - this is easier than remembering the list and having to type. So this suggests two different kinds of strategy:

  1. Use a library that allows you to provide a pre-populated list in the dropdown. You'd then have the ability to type, which would first try to select from the pre-populated options. If the client knows that there are more items than what's been pre-populated (because you're a super-admin, or because a preflight check showed that you're a member of too many sites), then an AJAX search is performed and the dropdown updated with results. I've built something like this in the distant past using Select2 and more recently using a React dropdown library. This is a nice solution but is a decent-sized design and development lift.
  2. Do something more like what you've suggested: On the server, if we detect that you are a super admin and the network is large, give you an AJAX-powered typeahead. Otherwise, generate a dropdown using get_blogs_of_user() or similar.
peterwilsoncc commented 7 months ago

I've created #1179 as an initial step.

It replaces the get_sites() query for non-super-admins with get_blogs_of_user(). I've currently got it in a draft state as I am unsure if anything is missing.