Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task done
boonebgorges opened this issue Jan 10, 2024 · 3 comments · May be fixed by #1179
Open
1 task done
Assignees
Labels
type:bug Something isn't working.
Milestone

Comments

@boonebgorges
Copy link

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

  • I agree to follow this project's Code of Conduct
@boonebgorges boonebgorges added the type:bug Something isn't working. label Jan 10, 2024
@jeffpaul
Copy link
Member

@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
Copy link
Author

@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
Copy link
Collaborator

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.

@jeffpaul jeffpaul added this to the 2.1.0 milestone Jan 16, 2024
@jeffpaul jeffpaul moved this from To Do to In Review in Open Source Practice Jan 16, 2024
@jeffpaul jeffpaul moved this from Code Review to In Progress in Open Source Practice Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working.
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

3 participants