[wp-meta] [Making WordPress.org] #4875: Add bulk suspension and reinstating admin tool for themes

[Making WordPress.org]

#4875: Add bulk suspension and reinstating admin tool for themes
------------------------------------------------+---------------------
 Reporter:  dingo_d                             |       Owner:  (none)
     Type:  enhancement                         |      Status:  new
 Priority:  high                                |   Milestone:
Component:  Theme Directory                     |  Resolution:
 Keywords:  has-patch dev-feedback 2nd-opinion  |
------------------------------------------------+---------------------

Comment (by dingo_d):

 > There's a lot to work through here, which makes it incredibly hard to
 review, smaller chunks or even a Git commit log may have made it easier to
 review.

 I agree I'll redo the work in smaller commits when I'll submit an updated
 patch.

 > `set_transient()` shouldn't be used for data-storage, it's backed by
 memcache on wp.org and may not be retained

 Ok, I can replace this with `get_options` since they are not touching
 cache (usually). I'm placing info with the expired dates and themes that
 should have their suspension removed in there so that reps don't forget to
 reinstate the theme.

 > `suspend_theme_on_trac()` queries for tickets by keyword, but we
 actually have the list of tickets stored within postmeta, which is much
 more accurate than the keywords field.

 That's even better, I can use `ticket_update()` method with the ticket ID
 directly 👍

 It was hard to work with this since I don't have all the data, so I was
 kinda working blindly. I realized that I could add my own trac username
 and password and do some simple things like getting data from trac, but I
 cannot create tickets since I'm not an admin, and don't have the
 `TICKET_CREATE` capability that the `themetracbot` has.

 > Does a singular-set-to-suspended still trigger
 wporg_themes_remove_wpthemescom()? it looks like it's now only set through
 bulk suspension? Without testing it it's hard to understand what the new
 flows are.

 Yes, since the singular-set-to-suspend will bring you to the bulk
 suspension messages screen, and that triggers the
 `wporg_themes_remove_wpthemescom()` function that will remove the preview.

 I'll create UML diagrams when I'll work on updating this ticket so that
 it's clearer what is done.

 > Is there any reason why this is an entirely new screen? Any reason why
 it couldn't have just been a post-list filter? Seems like it'd have saved
 a lot of code with either an extra column or extra row callback added.

 When looking at it, there is no reason for the suspended themes list. This
 was the first screen I worked on, as we (theme review team reps) were
 talking that it's hard to see what themes are suspended, by who and when.
 Looking at it now, I guess I could just add new columns to the themes
 list, and that way when the regular filters are applied, only suspended
 themes will be shown. Plus I can make some columns sortable (using
 `pre_get_posts`filter).

 > What's the purpose of having suspensions that expire like this? This
 seems like some kind of penalty box setup that doesn't really belong here?
 I initially thought it was to give the "Pending Suspension, upload changes
 by x date" functionality but it doesn't appear so?

 TRT reps were discussing how to handle suspensions and issues with theme
 authors we are having. So the plan was, roughly to do it like this:

 Warning (7 days) -> Suspension (2 weeks) -> Suspension (1 month) -> Ban
 (they didn't do anything or did bad things)

 Depending on what the theme author did, and if they updated the theme or
 not. Your `Pending Suspension, upload changes by x date` doesn't sound
 bad, but I'd need to see how to implement this with the flow we had
 (@poena, @kafleg, @williampatton, @acalfieri, @aristath).
 The idea of this ticket is to have this suspension information in one
 place since having it across various excel sheets seems super
 inconvenient.

 > It seems like you're dancing around the problem of bad authors without
 actually dealing with bad authors?

 Bad authors should be banned. Which is also a thing I'll probably want to
 add - `type of suspension` - permanent or for X amount of time. For others
 who failed to comply with the requirements, there is suspension, until
 they fix the issue (if they were warned but didn't act on it).

 > `WPorg_Themes_List_Table` contains a loooot of code that feels like it's
 probably already offered by the `WP_List_Table` - it feels like this is
 reinventing the wheel so it can be displayed on a certain screen? Is
 `WPorg_Themes_List_Table::comments_bubble()` even needed for example?

 I tried extending the default `WP_List_Table` in the
 `WPorg_Themes_Suspended_List_Table` but got errors, so I copied it to
 `WPorg_Themes_List_Table` and extended that. That's the reason for loads
 of unused code.
 But since I can add this data to the original `Packages` list (with
 filters etc.), I guess I can remove this code altogether.

 > This seems like several changes that should've been made in small
 discreet incremental changes, Suspension notes/reasons, bulk
 suspension/reinstate, and finally the penalty box setup, and none of those
 should've really required this much code IMHO.

 All this suspension code, notifications etc, are dependant on each other.
 I didn't see a way to separate it all in smaller tickets that would work
 independently, which is why I made one ticket.

 I'll re-work it, make smaller commits and see if this will be better.

 Also, regarding trac statuses, can those be added from the code? If I were
 to add the `suspended` or `suspended-under-review` status, I've just added
 those in the `ticket_update`, will this be reflected on the trac? Or
 should this be in the `$stati` property in the `Trac_Sync` class?

 Thank you again for constructive criticism, it helps when writing code for
 wordpress.org, where I don't have insight into what is happening 'under
 the hood' 🙂

 P.S. Sorry for the wall of text 😅

-- 
Ticket URL: <https://meta.trac.wordpress.org/ticket/4875#comment:4>
Making WordPress.org <https://meta.trac.wordpress.org/>
Making WordPress.org