Reviewer Admin

[obenland]

Enhancements to the standard WordPress plugin to enable plugin reviews.

• Create two Reviewer roles: Plugin Reviewer, Plugin Admin.
• Meta boxes with SVN/Trac Links.
• Internal notes, only viewable to Plugin Admins.

Plugin reviewers can only suggest to accept a plugin, Plugin Admins can change status.

[obenland]

Got started today on internal notes and experimented a little bit there. Should have something presentable by tomorrow.

[dd32]

In 2655:

Plugin Directory: Add several metaboxes for the Plugin Review/Admin.
This change includes switching to custom taxonomies, a custom publish metabox, a custom tags metabox, and the start of a plugin-reviewer metabox.

See #1570, #1584

[obenland]

In 2659:

Plugin Directory: Add support for internal notes.

Needs some more permission checks.

See #1570.

[obenland]

@ipstenu, what should a Plugin Reviewer be able to do vs. a Plugin Admin?

[Ipstenu]

Admin: Approve, Reject, Open, Close, Disable, Contact developer from 'interface' (plus everything a reviewer can do)

Reviewer: Download plugin (have link to it, whichever), leave private review for admin to act on, report plugin to admins for shenanigans.

[obenland]

So a Reviewer would be able to edit_plugins and edit_others_plugins. An Admin could additionally publish_plugins and edit_published_plugins.

In terms of statuses, we currently have pending, rejected, published, disabled, and closed.

Does that sound sane?

[Ipstenu]

I take it edit_plugins doesn't mean they can edit published ones?

Would Reviewers be able to see internal notes? I'm trying to see envision how we'd flag a plugin as reviewed but not approved... pending-not-reviewed, pending-reviewed?

[samuelsidler]

Replying to obenland:

In terms of statuses, we currently have pending, rejected, published, disabled, and closed.

What's the difference between disabled and closed?

[Ipstenu]

• disabled still serves updates but cannot be found in search
• closed is fully closed.

Closed is more common. Disabled is for things like we want to push an update to secure a plugin but it's been abandoned.

​https://wordpress.org/plugins/custom-content-type-manager/ should probably be disabled, for example.

[obenland]

Is hidden a better term for disabled maybe?

[Ipstenu]

Yeah, probably. :)

[obenland]

Replying to Ipstenu:

I'm trying to see envision how we'd flag a plugin as reviewed but not approved... pending-not-reviewed, pending-reviewed?

We could use the built-in draft and pending stati.

[obenland]

In 2735:

Plugin Directory: Use a custom comment type for internal notes.

See #1570, #1603.

[obenland]

Used by @ipstenu when managing plugins:

• Link to SVN repo.
• Revision log.
• Controlling permitted committers.
• Primary author.

I believe all of these are already covered.

[obenland]

In 2763:

Plugin Directory: Move list table classes into their own directory.

See #1570, #1571.

[obenland]

In 2764:

Plugin Directory: After [2763], update @package docs.

See #1570, #1571.

[obenland]

It doesn’t look like edit-comment.js can be used for more than once comment box in the edit screen. So for the custom comment type for plugin author communication, I’m looking at either rewriting edit-comments or starting from scratch :/

[Ipstenu]

Bah.

Okay so comments would be useful MOST for plugin reviewers with the plugin authors, hands down. Keeping the official communique in one place wins.

If it comes down to brass tacks, we can use the P2 for reviewers to leave reviews.

• link to plugin
• What they see wrong

They can then flag it un-resolved and an admin/approved reviewer can swoop in and send the official notice.

It's a little back and forth, but in a perfect world would lead to more admins ;)

[obenland]

In 2788:

Plugin Directory: Don't let Plugin Committers and Reviewers quickedit plugins.

Only PLugin Admins and above have the necessary privileges to change the things
quickedit allows users to change.

See #1570, #1571.

[obenland]

In 2789:

Plugin Directory: Make plugin post available in global scope.

\List_Table\Committers::single_row() needs it to be available globally, so
that permission checks for removing a committer work.

See #1570.

[obenland]

In 2790:

Plugin Directory: Don't show review tools to Plugin Committers.

See #1570, #1571.

[obenland]

In 2793:

Plugin Directory: Don't link to non-existent plugin previews.

See #1570, #1571.

[obenland]

In 2798:

Plugin Directory: Handle list view links in Plugin_Posts class.

Overwrites WP_Posts_List_Table's get_views() method, taking plugins into
account that the current user might not have authored, but has commit access to.

See #1570, #1571.

[obenland]

In 2830:

Plugin Directory: Capability improvements.

• Fill built-in roles with custom caps.
• Let Plugin Reviewers be able to manage their own plugins like regular authors.

See #1570, #1571.

[obenland]

In 2831:

Plugin Directory: Let Reviewers see all pending plugins.

See #1570.

[obenland]

In 2834:

Plugin Directory: Don't show pending plugins in Mine view.

See #1570.

[obenland]

In 2836:

Plugin Directory: Cache commit access data to avoid multible db calls.

Reduces DB calls on edit.php from 127 to 11.

See #1570, #1571.

[obenland]

For the review workflow, see 7 and following.

• Original author should be notified on approval/disapproval.
• We should think about what a re-submit process could look like after a plugin was not approved

I can't think of other actions right now that need to take place during the review, what am I missing?

[dd32]

The review process currently is

• Submit
• Review
  • Reject
  • Approve
  • Work with the author to fix issues (resubmit)

The last case there is currently done by keeping the plugin in the pending state, with the author emailing updated zip files to plugins@ in response to the feedback they've received.

My first thought was multiple statuses as part of a Workflow:

• Submitted, awaiting review
• Reviewed, waiting for developer feedback
• Reviewed, waiting for admin feedback (If plugin reviewers need their reviews reviewed by an admin)
• Hold - Reviewed, not waiting for Developer feedback, admin feedback, nor rejected/approved. Basically "This is being discussed".
• Rejected
• Approved

Reviewed, waiting for admin feedback and Hold could be merged together to form a Second Opinion status - basically some way of saying "This has been looked at, and someone else needs to look at it".

@Ipstenu Does the above sound right to you?

Finally, If a plugin author has uploaded a ZIP originally, Ideally, plugin authors would update that ZIP which the reviewers are reviewing, so plugin owners probably need access to the plugin admin screen before approval.

[Ipstenu]

Hold and 'Reviewed waiting for more admins' can be combined, yes. That works for me :)

Finally, If a plugin author has uploaded a ZIP originally, Ideally, plugin authors would update that ZIP which the reviewers are reviewing, so plugin owners probably need access to the plugin admin screen before approval.

Have we decided then that we're going with uploading zips? I'm neither for it nor against it, but yes, if they're going to be re-uploading, we'll need that.

[coffee2code]

Is there any plan with regards to the primary existing pain point when having multiple plugin reviewers: the potential for multiple plugin reviewers to step on each other's toes by inadvertently reviewing the same plugin at the same time?

A solution might suggest needing:

• Another post status ("under-review")
• A simple means for a reviewer to claim a plugin (action link in post table and/or button on edit page) and to unclaim the plugin (should they decide not to proceed with review).
• To store reviewer as post meta
• A post column to show reviewer
• A post table filter link to easily take a reviewer to plugins they have under-review. (Though we could limit reviewers to only claiming one at a time, in which case this view wouldn't be necessary and we could just have an admin notice above the plugin posts table saying something like You are currently reviewing <a href="">Plugin Name</a>.)
• To separately store who approved the plugin, again as post meta. (The person may differ from reviewer and recording this may be beneficial for future auditing)
• Possible safeguards to only permit the user who claimed a plugin for review (and admins, of course) to act as the plugin's reviewer (such as being able to mark a plugin as some form of reviewed).
• Possible ability to assign a plugin to another reviewer/admin for review.

In such a setup, reviewers should only proceed with reviews of plugins that they've successfully claimed.

[obenland]

Replying to coffee2code:

Is there any plan with regards to the primary existing pain point when having multiple plugin reviewers: the potential for multiple plugin reviewers to step on each other's toes by inadvertently reviewing the same plugin at the same time?

We'll be using the built-in ​post lock. That and a status signaling that that plugin has been reviewed.

[DrewAPicture]

Replying to obenland:

Replying to coffee2code:

Is there any plan with regards to the primary existing pain point when having multiple plugin reviewers: the potential for multiple plugin reviewers to step on each other's toes by inadvertently reviewing the same plugin at the same time?

We'll be using the built-in ​post lock. That and a status signaling that that plugin has been reviewed.

@coffee2code can correct me if I'm wrong, but I get the impression he's alluding to the fact that a plugin review might not be finished in a single session (a la post locking), thus needing a way to mark it as "In Progress by X reviewer".

[coffee2code]

Replying to DrewAPicture:

@coffee2code can correct me if I'm wrong, but I get the impression he's alluding to the fact that a plugin review might not be finished in a single session (a la post locking), thus needing a way to mark it as "In Progress by X reviewer".

Yep, what @DrewAPicture said is what I was thinking.

Unlike when actively editing a post (for which a post lock is appropriate), a plugin review happens outside of wp-admin. Once a reviewer has obtained the code (whether it was uploaded or available via a link) they only really need to return to the post to progress the status to the next state. In the meantime, the review is being performed elsewhere and may not occur in a single session.

As well, a reviewer may wish to preemptively claim a few plugins without actually sitting down to do the review(s) right that second. Or their machine or browser may crash. Or they're about to board a flight and would like to review a handful of plugins without having an internet connection. Or we may want to eventually allow assigning reviewers (such as cases requiring deeper security review or to deal with particularly complex code). All of which are not well handled (if at all) by post locking.

[Ipstenu]

So you want an assignment dropdown?

[obenland]

How about they just write a internal note?

[coffee2code]

Replying to Ipstenu:

So you want an assignment dropdown?

For the reviewer assignment feature part of it, sure (though I wasn't concerned at this stage about a specific implementation).

My main belief is that we should have a way of explicitly having a reviewer be able to say "I will review this plugin" and make that clear to other reviewers.

That could certainly be done with a dropdown when editing the post (and on quick/bulk edit) whereby reviewers could assign it to themselves. (...which then populates a post meta field which is then used as the data source for a post column that clearly shows the assigned reviewer.)

[coffee2code]

Replying to obenland:

How about they just write a internal note?

It's not apparent at a glance what plugins are currently being reviewed.
You'd have to visit the edit page for any/all plugin(s) in the queue depending on what you wanted to do or know. If I wanted to review a plugin, I wouldn't relish going down the list and loading each post one by one until I found a plugin that doesn't have a reviewer. (Even if you can tell if a post has internal notes or not, you can't infer a reviewer has claimed it without reading the internal notes of a plugin. And you'd have to read them all, as someone could claim it but after a number of other notes decide not to proceed with the review.)

It's not easy to determine which plugins are specifically assigned to me (or anyone else).
Maybe I've forgotten which one(s) I said I'd review. Or I got sick and asked someone else to take on my reviews. Or a reviewer quit or got kicked off the review team and now their claimed plugins need to be unclaimed/reassigned.

It's not easy to programmatically determine the reviewer for a plugin.
Thus you can't really do any filtering (e.g. list only the plugins I'm reviewing, list only plugins needing a reviewer, list all plugins reviewed by UserX and approved (perhaps they all warrant a re-review), etc) or stats (percentage of plugin queue currently under review; number of reviews Ipstenu has performed last month) or analysis.

[Ipstenu]

Having an assignment drop down would fulfill that. If something is assigned to a person, much like themes, it's THEIR responsibility. The only time it might get chancy is if something is assigned (like Pippin assigns a review to Aaron) and we forget to tell the assignee.

Email: Plugin review x has been assigned to you by Bob.

A custom meta value could do it? Maybe... They're not very sortable as I've learned recently.

[obenland]

In 2926:

Plugin Directory: Display custom post stati in list table.

See #1570.

[obenland]

In 2927:

Plugin Directory: Make sure pagination in plugin list works.

See #1570.

[obenland]

In 2929:

Plugin Directory: Allow committer mangament only for active plugins.

Plugins that are not yet live or are deprecated/hidden, should not have
committers added to them.

See #1570.

[obenland]

In 2930:

Plugin Directory: Initial setup for post status transitions.

Provides a place to handle post status transitions for plugins.

See #1570.

[obenland]

@dd32: Should we make Tools\SVN closer to Automattic_SVN? What were your plans for it going forward?

@Ipstenu: Are there any actions we need to execute when post statuses change? I added some for when a plugin gets published/rejected. Is there anything we need to do when one gets hidden, closed etc.

[Ipstenu]

I almost said "Gee wouldn't it be nice if it automatically posted in the P2 and emailed..." except sometimes we close posts and then check to make sure we should have. So ... No nothing needs to be triggered unless I'm forgetting something ( @otto42 @mordauk ?)

[mordauk]

Nothing that I can think of immediately.

[dd32]

Replying to obenland:

@dd32: Should we make Tools\SVN closer to Automattic_SVN? What were your plans for it going forward?

I was planning to just add wrapper methods for the core functionalities needed (as we need them), rather than the kitchen sink & abstraction that Automattic_SVN offers. I haven't seen a need for extra methods there yet - have I missed something?

[obenland]

In 2975:

Plugins Directory: Avoid a missing namespace warning.

See #1570.

[obenland]

In 2976:

Plugins Directory: Fix namespace typo after [2975].

See #1570.

[obenland]

In 2978:

Plugins Directory: Always show link to zip file if available.

Restricting it to only pending does not reflect the review flow anymore.

See #1570.

[obenland]

In 2979:

Plugins Directory: Smoothen out capability set up.

Only adds caps to built-in roles if they still exist.
Sets the default role to what plugin authors/committers will have.

See #1570, #1571.

[obenland]

In 2980:

Plugin Directory: Base post status access on user level.

Restricts plugin reviewers access to only set plugins to reviewed ('pending')
and doesn't let them publish a plugin. This will need more cap checks when
saving a plugin.

See #1570.

[obenland]

In 2981:

Plugin Directory: Attachments are not 0-based but ID-based.

Makes sure that the submitted zip file gets read and removed when publishing
a new plugin.

See #1570.

[obenland]

In 2982:

Plugin Directory: Add plugin count to dashboard widget.

Not high priority, but I had time on a flight and thought it would be neat.

See #1570.

[obenland]

In 2983:

Plugin Directory: Add meta data on plugin submission.

Adds readme meta data to the plugin post after uploading a new plugin.

See #1570, #1571.

[obenland]

Replying to dd32:

I was planning to just add wrapper methods for the core functionalities needed (as we need them), rather than the kitchen sink & abstraction that Automattic_SVN offers. I haven't seen a need for extra methods there yet - have I missed something?

Makes sense. We'll need a way to create a new repo for uploaded plugins once they're approved.

[obenland]

In 2995:

Plugin Directory: Don't process publish transition twice.

Bails if the plugin has been published before.

See #1570.

[obenland]

In 2996:

Plugin Directory: Make sure post_status changes are kosher.

Adds a cap check to make sure everyone can only set plugins to the status they
are supposed to:

• Plugin Admins: Any post status.
• Plugin Reviewers: Draft and Pending.
• Everyone else: No changes allowed.

See #1570.

[dd32]

Replying to obenland:

Replying to dd32:

I was planning to just add wrapper methods for the core functionalities needed (as we need them), rather than the kitchen sink & abstraction that Automattic_SVN offers. I haven't seen a need for extra methods there yet - have I missed something?

Makes sense. We'll need a way to create a new repo for uploaded plugins once they're approved.

Yep, SVN::mkdir( $remote_paths ) and SVN::import( $url, $local_path ) will be needed then.

Will need to check how mkdir deals with making */{trunk,tags,branches} and if it can be done in a single remote operation.
We could also get away with using just an import command for /plugin-name/ which includes trunk, branches, tags in the structure already.

[obenland]

In 3026:

Plugin Directory: Import plugin files to SVN upon approval.

We need to create an SVN repo anyway, adding the approved files also gives us
a nice baseline to compare commits to. And it saves plugin authors one more
step in the process of initially publishing the plugin.

See #1570.

[Ipstenu]

The ONLY catch with this is some people do not want plugins to be active right away. They want to be approved and have the code ready and waiting until they make some public declaration on their websites. Remember we get companies who need to time releases :)

The default should still be 'not active' and need some sanity check from the users, since we're changing a decade of expectation, basically!

[obenland]

If the trunk readme has a stable tag defined that is other than trunk, does it still fall back to trunk?

It also occurred to me that when a plugin is approved in the directory, it shouldn't show up in the directory until there is a "tagged" version.

[obenland]

In 3027:

Plugin Directory: Remove mac resources fork files after unzipping.

See #1570.

[Ipstenu]

If the trunk readme has a stable tag defined that is other than trunk, does it still fall back to trunk?

I believe it does - @otto42 would know for sure.

[Otto42]

If the Stable Tag line is missing entirely, then "trunk" is assumed.

If Stable Tag is set to trunk, then trunk it is.

If Stable Tag is set to XYZ, and /tags/XYZ exists, then that is used.

If Stable Tag is set to ABC, and /tags/ABC does not exist, then "trunk" is assumed.

[Otto42]

Note: If the Stable Tag line exists, but it is left blank, then it tries to use the /tags directory alone as containing the plugin. This is a bug, which does not come up a lot. It should use trunk in such a case.

[Otto42]

it shouldn't show up in the directory until there is a "tagged" version.

Simple plugins (quite of lot of them, probably the majority) only use trunk, not tagged versions. These should work too, so possibly simply putting the plugin into /trunk alone would work for the initial commit.

[obenland]

Replying to Otto42:
Thanks!

So should we refrain from importing the entire plugin to account for authors who'd like to wait with the first release?

[obenland]

Replying to obenland:

So should we refrain from importing the entire plugin to account for authors who'd like to wait with the first release?

And if the answer is Yes, how should we handle the discrepancy between a published plugin and a "truly" published plugin, one that also has a stable version in the repo.

[obenland]

In 3029:

Plugin Directory: Promote authors without a role.

Gives authors access to manage their plugin(s) if they don't have that already.

See #1570, #1571.

[dd32]

Replying to obenland:

In 3027:

Plugin Directory: Remove mac resources fork files after unzipping.

See #1570.

@obenland You should use the -delete flag here instead of -exec rm :)

[obenland]

Replying to dd32:

@obenland You should use the -delete flag here instead of -exec rm :)

Thanks for taking a look! I tried, but for some reason it doesn't actually remove it. I tried:

exec( "find {$esc_directory} -name '__MACOSX' -delete" );

Am I missing something?

[dd32]

Replying to obenland:

Replying to dd32:

@obenland You should use the -delete flag here instead of -exec rm :)

Thanks for taking a look! I tried, but for some reason it doesn't actually remove it. I tried:

exec( "find {$esc_directory} -name '__MACOSX' -delete" );

Am I missing something?

Doesn't seem like it, It looks like on linux it doesn't remove non-empty directories
This should work, but I don't think it needs changing, I'll take my recommendation back :)

exec( "find {$esc_directory} -path '*/__MACOSX*' -delete" );

You might also want to look for .DS_Store files from windows-based plugin authors.

[mfalhi_155]

Replying to obenland:

Enhancements to the standard WordPress plugin to enable plugin reviews.

• Create two Reviewer roles: Plugin Reviewer, Plugin Admin.
• Meta boxes with SVN/Trac Links.
• Internal notes, only viewable to Plugin Admins.

Plugin reviewers can only suggest to accept a plugin, Plugin Admins can change status.

[Ipstenu]

@mfalhi_155 - What's that a question about? :)

[obenland]

In 3033:

Plugin Directory: Also remove .DS_Store files from plugin zip.

H/t dd32.
See #1570.

[obenland]

In 3034:

Plugin Directory: Allow authors to update their plugin submissions.

If the initial version of a plugin does not satisfy review requirements, plugin
authors need a way to update their plugin for further review.
This now adds subsequent zip files by the same author to existing tickets and
adjusts logic and UI in a few places to handle multiple zip files.

See #1570.

[obenland]

In 3035:

Plugin Directory: Document zip file prefix.

See #1570, [3034].

[obenland]

In 3036:

Plugin Directory: Introduce an approved post status.

Approving a plugin creates the SVN repo and commits the approved plugin files
to that repo. It also add the plugin author to the list of authorised
committers and sends out a congratulatory email.

A plugin will require an initial SVN commit by the post author to then be moved
to publish, making it visible in the directory.

See ​https://wordpress.slack.com/archives/meta/p1461805883000448
See #1570.

[obenland]

In 3037:

Plugin Directory: Add a display state for approved plugins.

See #1570, [3036].

[obenland]

In 3038:

Plugin Directory: Make sure approved plugins stay approved on update.

Makes sure the status dropdown is populated with the right statuses.
Otherwise an approved plugin would be published after updating the post.

See #1570, [3036].

[Ipstenu]

FYI, Reviewer Admin is broken right now.

I can view a page, but I cannot edit anything and I have no access to leave comments etc as part of a review.

All I can do is see all posts and edit them and save changes, but I can't make any changes. :|

[obenland]

Replying to Ipstenu:

FYI, Reviewer Admin is broken right now.

Apologies, there was an error in how I added caps to existing roles. I didn't notice that because I've only been testing the custom roles directly. You should be good to go now.

[obenland]

Major programming operations on the Reviewer Admin have ended. In the battle of the Reviewer Admin, the Meta team and our allies have prevailed.

[obenland]

In 3078:

Plugin Directory: Delete zip files on rejection.

After a plugin was rejected there is no use for its zip files anymore.

See #1570.

[obenland]

In 3079:

Plugin Directory: Properly add caps to existing user roles.

Probably not needed in the future, but for now let's make sure existing roles
can test the plugin directory as well.

See #1570.

[obenland]

In 3080:

Plugin Directory: Allow Plugin Admins to assign plugin categories.

See #1570.

[dd32]

In 3082:

Plugin Directory: Move the SVN authentication handlers into the SVN class.
This also fixes an issue where the commit message was being prefixed with a = due to invalid arguement creation.

See [3026]
See #1570

[obenland]

In 3093:

Plugin Directory: Allow Committers and Reviewers to edit_others_posts.

WordPress seems to require users to have that capability globally for a post
type, in order to make changes to others posts, even if it's only certain
others posts.

This switches to using plugin_review and plugin_approve capabilities to
control Reviewers and Committers access to certain plugins. It also improves
the logic for displaying links to the various post status views in the plugins
list table.

See #1570.

[dd32]

Replying to obenland:

In 3093:

Plugin Directory: Allow Committers and Reviewers to edit_others_posts.

WordPress seems to require users to have that capability globally for a post
type, in order to make changes to others posts, even if it's only certain
others posts.

This switches to using plugin_review and plugin_approve capabilities to
control Reviewers and Committers access to certain plugins. It also improves
the logic for displaying links to the various post status views in the plugins
list table.

See #1570.

This works, but causes unexpected extra privileges that committers shouldn't have. There's a core ticket that needs fixing to fix the issue you were probably seeing.

[obenland]

In 3157:

Plugin Directory: Add a way to bulk reject plugins.

Probably most useful in draft or pending views.

See #1570.

[obenland]

@dd32 could you let me know what you think of 1570.diff​ when you get a chance?

Short of a custom query, can you think of a better way to get meta values for a set of posts?

[dd32]

@obenland looks good to me, the only change I'd suggest is to move the import changes to the start where the post is created rather than always performing it. (If that's not possible, it's just another thing to change at switch over, no biggie)

[obenland]

In 3331:

Plugin Directory: Give Reviewers the ability to filter plugins by author IP.

See #1570.

[samuelsidler]

Milestone Plugin Directory v3 - M1 deleted