Opened 5 years ago
Last modified 5 years ago
#4875 new enhancement
Add bulk suspension and reinstating admin tool for themes
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | high | |
Component: | Theme Directory | Keywords: | has-patch dev-feedback 2nd-opinion |
Cc: |
Description
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.
Attachments (6)
Change History (12)
@
5 years ago
Suspension message screen - when bulk themes are selected their names are in the notice
#1
@
5 years ago
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 retainedsuspend_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 theWP_List_Table
- it feels like this is reinventing the wheel so it can be displayed on a certain screen? IsWPorg_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.
This ticket was mentioned in Slack in #themereview by kafleg. View the logs.
5 years ago
#3
@
5 years ago
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 :)
#4
@
5 years ago
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 theWP_List_Table
- it feels like this is reinventing the wheel so it can be displayed on a certain screen? IsWPorg_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 😅
#5
@
5 years ago
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
Main packages screen