WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 6 days ago

#37392 assigned enhancement

Multisite "Sites" screen: Add links to filter websites by status

Reported by: thomaswm Owned by: mnelson4
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch needs-unit-tests
Focuses: multisite Cc:

Description

I'd like to see a list of all archived sites in my multisite network. Currently, I have to go through all sites on the "Sites" screen. There's no possibility to filter them by status.

My suggestion is to add a list of links ("Archived", "Deativated", "Spam" etc.) to the top of the "Sites" screen. Clicking one of them would result in only archived/deleted/spam websites being displayed.

Most other screens in wp-admin have those links, for example to filter posts by post status.

Attachments (5)

37392.patch (4.5 KB) - added by mnelson4 4 months ago.
1st implementation of this feature in a git patch
37392.2.patch (5.3 KB) - added by mnelson4 3 months ago.
First Revision of changes
37392.3.patch (5.3 KB) - added by mnelson4 3 months ago.
same as previous patch, but the last one had some spaces instead of tabs
37392.4.patch (4.9 KB) - added by mnelson4 3 months ago.
Same as previous, except no unnecessary removal of line in ms-functions.php
37392.5.patch (4.4 KB) - added by mnelson4 6 weeks ago.
Patch uploaded in response to https://core.trac.wordpress.org/ticket/37392#comment:16

Download all attachments as: .zip

Change History (27)

#1 @flixos90
21 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Thanks for the ticket @thomaswm!

Although yours has been around a little longer, there's #37684 now which has seen a little more progress lately, so let's continue the discussion there.

This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.


6 months ago

#3 @flixos90
6 months ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Per https://core.trac.wordpress.org/ticket/37684#comment:11, this is actually not a duplicate, only a related ticket. This one here is about adding filters for site states, while the other one deals with display site states in the list table.

#4 @flixos90
6 months ago

  • Owner set to mnelson4
  • Status changed from reopened to assigned

#5 @mnelson4
5 months ago

I saw a post somewhere (sorry I can't find it, I thought it was on make.wordpress.org) mentioning how we're going to want to add sites REST API endpoints, and use them from the admin. If that's the case, it would probably be a waste to try adding this in the old style, because we'll be wanting to rewrite it anyways right away. Right?

#6 @flixos90
5 months ago

@mnelson4 It's true that parts of the admin will be rewritten once the multisite API endpoints are in place, however this will be happening over the course of time, and also reasonably. If a change doesn't really improve anything other than using AJAX and the REST API, there's not much of a point.

I think this particular problem here would benefit from a solution based on current circumstances, as there are more pressing issues in multisite UX when it comes to using the REST API that would need to be handled first.

@mnelson4
4 months ago

1st implementation of this feature in a git patch

#7 @mnelson4
4 months ago

  • Keywords has-patch needs-unit-tests 2nd-opinion added

Ok I added a patch that adds this feature. I based it off the users list table.

Some issues I wasn't sure about:

  • what would be the best name for the new function, which for now is just called wp_count_sites(). Also just took a guess that ms-functions.php would be a good place for it
  • the translatable strings which I basically copied from class-wp-users-list-table.php has HTML in it (I thought its best to have placeholders, because we don't want to burden translators with understanding HTML, and we certainly don't want translators to translate the HTML or the classes etc!). Eg __( '%1$s <span class="count">(%2$s)</span>' ) is one. I just left the HTML in there for now, but it seems it would be best to put placeholders in there instead.... although that would leave the string as just "%1$s %2$s(%3$s)%4$s" which is so vague that maybe we should just NOT translate it at all. Thoughts?
  • I don't totally understand what the "public" flag on a blog row means. When I set that to false for a blog, I could still visit it on the front-end (even when not logged in), and its mostly absent from the UI (except if you click to "edit" a page). Perhaps we should exclude that view?
  • wp_count_sites() returns an stdClass with the properties all_blogs, public, archived, mature, spam and deleted. Do we like those names? Would it be better as an array?

Lastly, I should probably add a unit test or two for wp_count_sites(), but I'd rather do that once I have confirmation from someone that that functionality looks good.

This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.


4 months ago

#10 @flixos90
4 months ago

Thanks for the patch @mnelson4, generally looking good! A few observations here:

class-wp-ms-sites-list-table.php

  • Typo deletected in line 69
  • The if clause in lines 62 to 73 can all go into one line, per coding standards. The list of statuses is short enough to be on a single line.
  • The list of statuses in lines 225 to 229 should use __() instead of esc_html__() --> always escape as late as possible (usually when outputting); so here that should happen in lines 251 and 259.
  • Blank line 222 at method beginning is not needed, instead a few more blank lines inside the method code would help with readability. Also look out for having = signs similarly indented if there isn't a blank line between them.
  • Instead of concatenating the long strings from line 235-245 and 254-260, rather use sprintf(), like <a href="%1$s"%2$s>%3$s</a> (make sure to not put additional spaces where not needed). You can look at other core uses of sprintf() to see correct indentation in case of longer content. In the first case here that would mean you end up with an sprintf() inside another sprintf(), so in that case, put the inner one first and store it in a variable, to not overcomplicate readability there.
  • Instead of having a single label for each site status, rather following the pattern similar like post statuses do (see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/post.php#L234 and https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-posts-list-table.php#L356 for how it works there).

ms-functions.php

  • I agree with the name wp_count_sites(), as it matches the existing wp_count_posts() for example. I think it should be part of ms-blogs.php though since that one houses the basic site API functions related to querying.
  • The query in wp_count_sites() currently doesn't take the network into account. It should only count sites of the current network. Even better would be: Add an optional $network_id parameter that uses get_current_network_id() if nothing provided, and use that then.
  • The query should definitely be cached. It's kind of tricky to find a suitable cache group and key here. Generally, those things are stored in the counts group, but that group is per site, and the sites count is global, so we can't use that. I'd suggest going with a group of sites now (that one houses all other site-related data), and the key could be counts-{$network_id}. @jeremyfelt What do you think about naming here? Which cache key and group would be suitable? Anyway, you can probably just use the above for now. See wp_count_posts() for an example how it handles the caching. In addition, we'd need to clear the cache in clean_blog_cache() for the network the site is part of.
  • Maybe it would make sense to build the SQL string by iterating through the different site statuses (you could put those in an array, just like in your get_views()).
  • The return value of an object is fine as it matches wp_count_posts(). In the docblock, rather say object instead of stdClass though.

General

  • Very minor, but you can probably update the versions from 4.9.3 to 5.0, as this will likely not go in as part of a minor release.
  • Check spacing and blank lines for docblocks and make sure all docblocks have a description.

#11 @mnelson4
4 months ago

Ok thanks for the great feedback Felix, I'll make a new patch

@mnelson4
3 months ago

First Revision of changes

@mnelson4
3 months ago

same as previous patch, but the last one had some spaces instead of tabs

@mnelson4
3 months ago

Same as previous, except no unnecessary removal of line in ms-functions.php

#12 @mnelson4
3 months ago

  • Keywords 2nd-opinion removed

Ok I think the latest patch applies your suggestions @flixos90 (the previous ones did too, but had formatting mistakes).

The only one I didn't apply was

Maybe it would make sense to build the SQL string by iterating through the different site statuses (you could put those in an array, just like in your get_views()).

That could be good, but it would have beena bit ugly because the first selection (where it selects "all") would be an exception; I don't think we'll be adding new statuses soon; and IMO leaving it as simple SQL makes it easier to immediately understand what it's doing. However, if you feel strongly about it, I'm happy to add the loop like you suggested.

Let me know if I missed something!

If this looks good, what else is needed to get this ready?

This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.


3 months ago

#14 @SergeyBiryukov
3 months ago

  • Milestone set to Awaiting Review

This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.


3 months ago

#16 @flixos90
2 months ago

Thanks for the updated patch @mnelson4, sorry it took me so long to review.

This looks great for the most part! A few observations:

class-wp-ms-sites-list-table.php

  • all should probably not be part of the $slug_to_label array, as it's not a status and its link should have a different structure (it shouldn't do ?status=all, but instead should not add any status query arg). See how https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-posts-list-table.php#L323 does that. The array should then also receive a more clear name such as $statuses, and the keys in the loop could be $status ($this_status looks weird as it can easily be confused with something like $this->status).

ms-functions.php

  • In wp_count_sites(), try to be less strict about the provided value. ! $network_id should be sufficient to fall back to get_current_network_id(). Before that you might wanna cast it to an integer.
  • I agree with your comment regarding the SQL query. Leave it as is in this patch.

Please also have another look at spacing and blank lines inside of doc blocks. There are a number of missing blank lines there, and some parts are incorrectly indented.

Regarding general approach: I think the approach we're taking is fine for now, but we should consider other approaches than how core has handled this previously too, just in case. Particularly, site statuses are different from post statuses because a site can have multiple statuses at the same time. The current approach doesn't, while maybe being sufficient, doesn't reflect that, for example it wouldn't allow to select all sites that are "archived and mature", or "deleted and not mature". That could only be covered by something like a select multiple, multiple checkboxes or dropdowns - again, not sure we should do this, but we should think about it further before proceeding with this just because core has done that before (in a slightly different scenario).

#17 @mnelson4
2 months ago

Thanks for the updated patch @mnelson4, sorry it took me so long to review.

No worries, thanks for the review!

we should consider other approaches than how core has handled this previously too... like a select multiple, multiple checkboxes

In my usage of multisite, it's rare for a site to have multiple statuses (actually, we mostly just mark sites as spam, and don't really use the other statuses much). So I wouldn't prioritize this, but if you think it's important we can attempt it.

In order to have parity with what WP_Site_Query can handle, where you can provide any of the statuses and a value of either true or false (and not including the status means "whatever"), I think we would need to list each status, and have a dropdown for it with options like "Yes", "No", or "Either", and have a button saying "Filter" or "Search".

To query for "archived and mature", the would set archived to "Yes", and mature to "Yes", and leave all the others at "Either". For "deleted and not mature" they would set deleted to "Yes", mature to "No", and leave all others on "Either". Thoughts?

Also, it's maybe this patch should just add actions and filters to WP core, and then create a plugin for this instead. Then we can see what feedback there is on that plugin, before committing to a decision for WP core. Thoughts?

This ticket was mentioned in Slack in #core-multisite by mnelson4. View the logs.


8 weeks ago

#19 @mnelson4
8 weeks ago

We discussed this in slack, and we're going to stick with this UI initially (ie, we won't initially support filtering by multiples statuses).

I'll add some screenshots of the current UI, and work on the code tweaks @flixos90 suggested.

I think @flixos90 and @jeremyfelt will discuss where best to cache the query, but for now I'm going to leave it where it is.

#21 follow-up: @mnelson4
6 weeks ago

all should probably not be part of the $slug_to_label array, as it's not a status and its link should have a different structure (it shouldn't do ?status=all, but instead should not add any status query arg). See how https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-posts-list-table.php#L323 does that.

I hear you that there shouldn't be a link with ?status=all, so I made an adjustment that avoids that, but I do prefer how having all be in the array is avoids quite a bit of repeated code. If you still prefer not having all in the array, I can make another patch that does that.

here are a number of missing blank lines there

I'm not sure where there should be blank lines. I double-checked the WP coding standards and don't see anything about where there should be blank lines. Could you point me to some documentation or help me see where blank lines are needed?

some parts are incorrectly indented.

I double-checked that it's using real tabs instead of spaces for indentation and don't see where the indentation is wrong, sorry!

I hope this is a bit better though

#22 in reply to: ↑ 21 @flixos90
6 days ago

Replying to mnelson4:

I hear you that there shouldn't be a link with ?status=all, so I made an adjustment that avoids that, but I do prefer how having all be in the array is avoids quite a bit of repeated code. If you still prefer not having all in the array, I can make another patch that does that.

That's fine with me. One minor detail, please always use strict comparisons, there's a non-strict comparison in line 232.

I'm not sure where there should be blank lines. I double-checked the WP coding standards and don't see anything about where there should be blank lines. Could you point me to some documentation or help me see where blank lines are needed?

Between docblock descriptions, since descriptions, param/return descriptions there should always be a blank line (blank meaning of course still put the asterisk there, but only that). For example between lines 207, 208 and 209 in class-wp-ms-sites-list-table.php or lines 1544 and 1545 in ms-blogs.php.

I double-checked that it's using real tabs instead of spaces for indentation and don't see where the indentation is wrong, sorry!

For example line 1545 of ms-blogs.php there are several extra spaces between the variable name and description.

Note: See TracTickets for help on using tickets.