Add bulk suspension and reinstating admin tool for themes

[dingo_d]

I've been working on this on and off in my spare time for some week or two.

Theme review reps need better tools when working on suspending themes and reinstating them. Currently, we have a big issue if we find an author violating the rules and not fixing them in a timely manner so we need to suspend all of their themes. At the moment this is a lengthy process where you need to find all the themes, go and suspend one by one, then go to trac and send a message that the theme has been suspended for violating rules.

The proposed patch adds a way to suspend themes in bulk and also reinstate them.

The workflow goes like this:

All the themes that need to be suspended are found, selected and with a bulk action can be suspended. The theme rep will then be brought to a new suspension message screen where they can add the suspension reason and the duration of the suspension.

This information is stored in a theme meta and can be visible in the theme edit screen. This also helps with keeping track of who suspended a theme and when in the past.

In addition to that, a cron job is added which will be ran twice a day that will check all the suspended themes, and those that have the necessary metadata will be checked if they need to be reinstated (unsuspended). A permanent ban can be set with a super high suspension date (+1000 years).

There is also Suspended Packages screen which holds all the suspended themes, and assorted data with them. Those can also be reinstated in bulk if necessary.

Now I've added a call that would update the ticket on trac for the theme that is suspended. But this is where I need help from meta team with trac experience.

Ideally, we'd have 2 new statuses for the themes: suspended and suspended-under-review. When a theme is suspended, the last trac ticket should be changed to suspended and the author shouldn't be able to upload themes.
Once suspension passes and we reinstate the themes, on reinstating the theme the new status should be suspended-under-review so that the changes can be updated, but the theme won't be set live. I think for this we might need a new post status in the admin so that we can differentiate those.

Once the themes have been reviewed and fixed we'd approve them and set them live.

The things that 've changed are:

	modified:   wp-content/plugins/theme-directory/admin-edit.php
	new file:   wp-content/plugins/theme-directory/assets/suspend-form.css
	modified:   wp-content/plugins/theme-directory/class-wporg-themes-repo-package.php
	new file:   wp-content/plugins/theme-directory/class-wporg-themes-suspended-repo-packages.php
	new file:   wp-content/plugins/theme-directory/inc/class-wporg-themes-list-table.php
	new file:   wp-content/plugins/theme-directory/inc/class-wporg-themes-suspended-list-table.php
	modified:   wp-content/plugins/theme-directory/jobs/class-trac-sync.php
	modified:   wp-content/plugins/theme-directory/theme-directory.php

The details are in the patch I'll attach.

There is still some work to be done, but I need meta's help with this one, as it's quite a big change, but would help theme review reps considerably.

[dingo_d]

Main packages screen

[dingo_d]

Suspension message screen - when bulk themes are selected their names are in the notice

[dingo_d]

Suspended packages listing

[dingo_d]

Suspension metabox history

[dd32]

Thanks @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..

At a glance through it, there's only a few things that stood out:

• set_transient() shouldn't be used for data-storage, it's backed by memcache on wp.org and may not be retained
• 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.
• 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.

Other questions:

• 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.
• 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? It seems like you're dancing around the problem of bad authors without actually dealing with bad authors?
• 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?
• 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.

[dingo_d]

Thanks for your input. I was super busy yesterday and I have things to do today, but I'll go through the comments and get back to you :)

[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_postsfilter).

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 😅

[dingo_d]

An updated patch for bulk suspension.

[dingo_d]

I've added a new patch, still WIP but I've consolidated all in one class, and it's easier to follow what is happening and what the intention is.
I've removed the extra screens, which weren't necessary. I've also made changes so that extra columns are shown only when a 'suspended' filter is selected. This I think I'll need to add some guard clauses, I've only noticed now that there are some notices being thrown.

Also, I'm pasting the gitlog here:

commit 555f1419e8a8d9942198ef78a439fa63015e8880 (HEAD -> feature/bulk-suspension-admin-tool)
Author: Denis Žoljom <denis.zoljom@gmail.com>
Date:   Sat Dec 7 18:36:48 2019 +0100

    Update suspension and reinstating bulk options, styling fix

    This commit should have been broken down per hook added, I noticed this when it was too late. I have added some styling to the subscription form, I've also added bulk reinstating and suspension options, as well as what happens during bulk suspension.
    I've left the default post row actions for now, as I'm not 100% sure what else is going on when one clicks that.
    I'll see how to add checks for the cron, to notify theme reps which themes needs reinstating, and which do not.

commit 5d693dd82a3ee14c18e5ec219391f7f139c883ff
Author: Denis Žoljom <denis.zoljom@gmail.com>
Date:   Sat Dec 7 18:35:23 2019 +0100

    Add assets

    CSS for the suspension form

commit 6d1055aa1452a37cb40be6adff05e8e6e94de5de
Author: Denis Žoljom <denis.zoljom@gmail.com>
Date:   Sun Dec 1 18:02:57 2019 +0100

    Add bulk suspension screen

commit 7008c92c9b4140e87f833cd7703afffdb000f863
Author: Denis Žoljom <denis.zoljom@gmail.com>
Date:   Sun Dec 1 17:44:41 2019 +0100

    Add bulk suspension redirect actions

commit e95157fc00d34962c76d70a473c0d2202b88cdbf
Author: Denis Žoljom <denis.zoljom@gmail.com>
Date:   Sun Dec 1 16:59:43 2019 +0100

    Add content to the suspended columns and make some of them sortable

commit 7cda096b14a60245fcf9d7666a6a37ebca45d962
Author: Denis Žoljom <denis.zoljom@gmail.com>
Date:   Sun Dec 1 11:42:32 2019 +0100

    Add new suspension columns

    Columns will be added only if the status of the theme is suspended. This way there is no need for additional screens, and there is less clutter. Also theme review team reps can see all the necessary information in one place.

commit 844561821e63bd74967cf438bc9d137c854a7703
Author: Denis Žoljom <denis.zoljom@gmail.com>
Date:   Sun Dec 1 11:40:28 2019 +0100

    Add new class responsible for the suspension functionality