Making WordPress.org

Opened 10 years ago

Closed 9 years ago

#762 closed enhancement (fixed)

Themes trac: Magic button to automatically assign reviewers the next available ticket (theme) to review if they don't already have a ticket

Reported by: samuelsidler's profile samuelsidler Owned by: nacin's profile nacin
Milestone: Priority: high
Component: Theme Review Keywords:
Cc:

Description

From make/meta (karmatosed):

At the theme review team we’d would like to increase the automation of our processes and try and make things a little easier for new members to become reviewers. In light of this we have a tool request for a new reviewer assignment button.

Spec of an assignment button:
A button that when clicked takes the wordpress.org username logged in as and assigns the next ticket in the #4 queue. This will only ever assign one theme, if you have one not approved but in review you can’t get another this way – you must do one by one. It will also cc in any admins we want – primarily thinking Emil and me.

Placement of button and extras:
Along with this, there would be a report we could look at that showed any of these themes. This button will live as a menu item and also on the handbook in the ‘How to Join’. This will remove the request post requirement forever. This post creation each season is a bit of a pain point and people ask a lot where it is, this will solve that. It will also mean at contribution days people can get a theme without an admin being around. That should have a knock on effect of increasing reviewers – which is great.

Design:
As far as the term button goes, this could be a link to a page. The idea is though it is obvious. Perhaps button styling would achieve this, but we’re flexible about that. The goal is it’s obvious though and easy to use.

Any questions I’d be happy to answer.

Attachments (1)

site-themes.php (5.2 KB) - added by nacin 10 years ago.

Download all attachments as: .zip

Change History (26)

#1 @samuelsidler
10 years ago

  • Owner set to al_the_x
  • Status changed from new to assigned

#2 @al_the_x
10 years ago

Based on discussion with Sam, recommended approach:

  • Install the XML-RPC plugin to the Themes Trac
  • Expose an authenticated XML-RPC / JSON-RPC method for assigning the next open ticket to the currently authenticated user.
  • Write a jQuery plugin that will fire that RPC method when clicked (via XHR, presumably?)

Feedback appreciated...

#3 @al_the_x
10 years ago

Some sample code from the Database API for Trac 1.x page for reference:

from trac.env import open_environment
def myFunc():
    env = open_environment() ## omitting the path uses $ENV[TRAC_ENV]
    with env.db_transaction as db:
        cursor = db.cursor()
        # Execute some SQL statements

Workflow:

  • return failure if find ticket.id assigned to authenticated user
  • fetch ticket.id:
    • owner is none
    • join ticket_enum on type to where ticket_enum.name is "open"...?
    • order by time
    • limit 1
  • use trac.ticket.model.Ticket to update the owner field to the authenticated user
Last edited 10 years ago by al_the_x (previous) (diff)

#4 @al_the_x
10 years ago

Writing the code to fetch and update the Ticket is nowhere near as complicated as exposing the XML-RPC method for triggering it... And the XML-RPC module is not very well documented. Might need to ping some folks on the Trac mailing list / chat room.

#5 @al_the_x
10 years ago

Couple examples of existing XML-RPC implementations:

Gonna have to register the method as a Trac Component (like usual plugin development), implement the IXMLRPCHandler interface, and implement xmlrpc_namespace and xmlrpc_methods, which yield's a number of tuples describing the methods exposed by the handler. Each method receives the Request instance, which will have the authname key: either None or a valid Trac username.

From trac.ticket.model.Ticket.save_changes, I don't believe that the author is _required_, so we should be able to save the ticket via the system and bypass the regular permissions system.

#7 @al_the_x
10 years ago

Question: Do users in themes.trac have TICKET_CREATE permissions, just not TICKET_CHGPROP or some other TracPermissions...?

#8 @Otto42
10 years ago

The themes trac already has the XML-RPC plugin. It's how the uploader on w.org/themes creates the ticket.

No, users do not have ticket create permissions. That permission is reserved to the "themetracbot" user, which creates the ticket, or modifies the existing ticket, at the time of the theme upload.

#9 @Otto42
10 years ago

Also, we already have a relatively small PHP library that has the ability to create and update theme tickets, via XML-RPC, in place.

Having themetracbot update a ticket to change the owner field is just a few lines of code. The only hard part is working out all the logic. Assume that you can call $themes_trac->ticket_update() with a few parameters and have it just do it. Because, well, you can.

From the library:

function ticket_update( $id, $comment, $attr = array(), $notify = false )

Setting the 'owner' in the $attr array would set who it's assigned to.

There's also ticket_query to do searches, and ticket_get to get tickets by ID number.

#10 @al_the_x
10 years ago

Check out my gist... I'm flying blind, as I don't have a Trac instance running in since... Well, forever. Eyes on code is appreciated.

#12 follow-up: @Otto42
10 years ago

Why Python? I'm thinking this is something we'd do in PHP, on the w.org server, in the themes directory somewhere. Not something we'd need to modify Trac for.

#13 in reply to: ↑ 12 @samuelsidler
10 years ago

Replying to Otto42:

Why Python? I'm thinking this is something we'd do in PHP, on the w.org server, in the themes directory somewhere. Not something we'd need to modify Trac for.

Mostly because I didn't realize such a script existed. Given that this was a request for "trac," I asked @al_the_x to help out. @Otto42, can you use the work here and the preexisting PHP script to code this up?

#14 @karmatosed
10 years ago

  • Cc tammie@… added

I'm super excited to see movement on this. It's really important for the team and contribution days.

As a little additional request, once we have everything in place it would it be possible to have it also assign a mentor? We have a list of people who are mentors. There is actually a request for a new role in trac and field for that:

https://meta.trac.wordpress.org/ticket/708

#15 @al_the_x
10 years ago

Like @samuelsidler said, the original thinking was to add an XML-RPC method to Trac that would enforce this workflow... And he had no idea that there was an existing integration with wp.org written in PHP. You're right though, @otto42: the existing XML-RPC interface for Trac has all the parts and pieces required to execute the desired workflow. If you enforced the same in PHP, you'd additionally need to expose a URL on wp.org that the "Magic Button" could trigger. To determine the authenticated user in Trac, I believe you could employ $USER in the request.

@nacin
10 years ago

#16 @nacin
10 years ago

  • Owner changed from al_the_x to nacin
  • Status changed from assigned to accepted

This is implemented. karmatosed, you'll notice there is an inactive widget ready for you to drag it to the sidebar make/themes. You can add instructions in the text field that will be displayed above the button. I'd place it at the top of the sidebar. Once the widget is live, it'll all work.

How it works:

  • If you already have a theme currently assigned to you and "reviewing" (which thus excludes "reopened" and "approved", also "not-approved" — please tell me if anything should change here), then upon clicking the button, you're told to go to your report of open tickets assigned to you.
  • It grabs the ticket off this query, assigns it to you, and redirects you to the ticket so you can immediately start to review it: https://themes.trac.wordpress.org/query?priority=new+theme&priority=previously+reviewed&status=new&keywords=!~buddypress&order=time&max=1&owner=. (Note: That first ticket is awaiting a reply for the reporter, and would be bad if we assigned it to someone random — does that happen often?)
  • If the queue is empty (hey, it's happened before), it'll recognize that. It also handles errors fairly gracefully and accounts for race conditions where two people press the button at the same time.
  • I'll be able to write a report of all tickets assigned using the button, once it actually starts to get used. It doesn't CC individuals. Please confirm this is what you want.

Due to directory structures (which are in the process of changing), it was easier to commit this code privately for the moment. So for those curious, I've uploaded the plugin.

#17 @karmatosed
10 years ago

Oh wow this is so cool - thanks a lot :)

I popped it in the sidebar. I did a test but I got this message:

"Something went wrong; the system can᾿t assign you a review right now."

I assume that is the first case you mention - maybe? Or do you need to not be an admin to test?:
"If you already have a theme currently assigned to you and "reviewing" (which thus excludes "reopened" and "approved", also "not-approved" — please tell me if anything should change here), then upon clicking the button, you're told to go to your report of open tickets assigned to you."

The only other thing I would love to have would be to cc in mentors. Maybe we can look at that for a later version? Currently we have to manually using a spreadsheet assign mentors.

#18 @nacin
10 years ago

From what list of mentors should it pull from, and how should it assign them to a ticket? On a rotating basis?

I'm not sure why it was failing. It works for me. I just assigned https://themes.trac.wordpress.org/ticket/21573 to me (and then re-assigned it to you). It should give you the following notice: You have reviews assigned to you that you need to complete first.

Could you try clicking it again, then give me the URL it sends you back to? It should include theme-assign=failed&assign-error=... I'd need to know the assign-error. It would either be 'active', 'max-tries', 'queue'. Or, it would be theme-assigned=existing, at which point you'd get the notice above.

#19 @karmatosed
10 years ago

On trying today I get the expected message:

"You have reviews assigned to you that you need to complete first.".

So, sorry maybe that was a false alarm as it does appear to be working now.

We have a list on a spreadsheet of mentors, which isn't a robust approach. The idea is it would be on a rotating basis through that list. I'm unsure what you'd recommend for that. My one thought is maybe it could be a role in trac that has no extra privaledges but we assign to people and is used to rotate through.

Would that be a good procedure? We need some method that allows automation but also lets us add to the list (and take away).

#20 @nacin
10 years ago

There was some undeployed code I was running that was allowing it to work for me.

Does someone's mentor change with each review, or is it a single mentor for all of their reviews?

#21 @karmatosed
10 years ago

The mentor stays with them as long as they want it, for every review.

#22 @nacin
10 years ago

So it's not just a matter of rotating through, it's also a matter of ensuring they're getting the same mentor each time. That's a bit more complicated, OK.

#23 @karmatosed
10 years ago

Yes it is. Our current method is me assigning from a spreadsheet, so anything is an improvement on that :)

#24 @nacin
9 years ago

  • Component changed from Trac to Theme Review

#25 @Otto42
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

Button exists and has existed quite a while. Fixes need a new ticket.

Note: See TracTickets for help on using tickets.