Page MenuHomePhabricator

Set up volunteer code review queue
Open, Needs TriagePublic

Description

The MediaWiki / Wikimedia developer community is doing poorly at volunteer retention, and there is wide agreement that the difficulty of getting code review is one of the main problems (possibly the main problem) behind this. There has long been interest in improving the situation (e.g. T78768: Agree on and implement actions to prioritize code review of patches submitted by volunteers) but not a lot of concrete actions happened.

One of those was T128371: Set up Code Review office hours, which did not really work out, maybe in part due to lack of organization / effort, but mainly because IRC wasn't a great channel for code review (except for very simple patches). Per T173770#4076361, that effort has been abandoned; let's find a replacement that works better.

The aspect of code review office hours were authors were able to put their patches in a central place to get reviewers' attention was IMO valuable; but then coordinating that everyone be online at the same time was an unnecessary complication, and the poor usability of events in Phabricator made the patches hard to find.

Let's have a code review queue instead: a wiki page or Phab task where people can nominate their patches (and maybe patches by new volunteers get auto-nominated), they get announced to wikitech-l once a week, and patches which get a review are removed from the queue.

Event Timeline

Tgr created this task.Aug 2 2018, 9:09 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 2 2018, 9:09 AM

What kind of review does this refer to? Regular (in-depth) code review for +1, +2 for merging, or perhaps both?

Tgr added a comment.Aug 2 2018, 12:33 PM

I'd think both - they are not that separate in practice.

Hmm. Why is this supposed to work better than IRC code review hours, apart from not having to be around at a certain time?

Middle term: We have a list of last open (means: not merged && not abandoned) patchsets not created by bots and not created by WMF or WMDE folks at https://wikimedia.biterg.io/goto/2a7e85ff75feb0d3c8b21515392dac1d . However this list ignores CR±1/CR-2 labels (that's https://github.com/chaoss/grimoirelab/issues/96 ) so I assume it's not too useful?

Long term: Folks who'd like to see their code reviewed should not have to manually add their patch somewhere when their patch is already in a code review system.

IRC code review hours, apart from not having to be around at a certain time?

Do those [still] exist? I have hardly ever succeeded to get my code reviewed via IRC (timezone, busy people, no interest, etc.) so using a list of patches you'd like someone to review would be better in my experience if, and only if, there are people interested in volunteering for this. I'd like to thank WMDE for their Technical Advice meetings where I could get some of my patches reviewed and merged, and to James, Kunal and Tgr that keep reviewing my code (although I feel somewhat guilty that I have to ping the same set of people everytime).

Do those [still] exist?

See T128371, T173770 linked below the task description.

Code Review Office Hours were disbanded some time ago. It was not until recently that the Phabricator calendar event displaying that they were still taking place was disabled; thus causing volunteers such as myself to still continue to list patches for review (and no one noticing them). Thank you.

Can we please just set something up here? I would like to add patches to a queue like that for sure.

Can we please just set something up here? I would like to add patches to a queue like that for sure.

We already have www.mediawiki.org and phabricator.wikimedia.org. I suggest editing a wiki page or filing some tasks here, either of which could easily serve as a queue. The trickier part is finding qualified reviewers.

Krenair added a subscriber: Krenair.Sep 6 2018, 2:16 AM

Gerrit searches can be good for finding stuff to review but I've found some groups simply don't watch it.

Can we please just set something up here?

@MGChecker: Nobody stops you from doing that, I'd say, but that's not a solution... The harder part is finding [the right] folks, making [the right] folks look at it, perform code review. If there's no concept, it's creating false hopes: Folks might waste time adding an item to some list and nothing will happen afterwards.

Tgr added a comment.Sep 6 2018, 12:38 PM

Hmm. Why is this supposed to work better than IRC code review hours, apart from not having to be around at a certain time?

That and not pretending that code review can be done within an hour.

Middle term: We have a list of last open (means: not merged && not abandoned) patchsets not created by bots and not created by WMF or WMDE folks at https://wikimedia.biterg.io/goto/2a7e85ff75feb0d3c8b21515392dac1d . However this list ignores CR±1/CR-2 labels (that's https://github.com/chaoss/grimoirelab/issues/96 ) so I assume it's not too useful?

CR+1 is not super useful, but yeah -1/-2 should be filtered out. Probably V-2 as well.
And Kibana is not a great interface for something like this IMO anyway.
(Also, I get a HTTP 500 for that link. I guess it expired?)

Long term: Folks who'd like to see their code reviewed should not have to manually add their patch somewhere when their patch is already in a code review system.

There is a workflow issue and a capacity issue here. Obviously it would be great to fix the capacity issue but I don't think that will happen without support from higher on in the org. As long as there's limited code review capacity, having an explicit method for pushing stuff for review is not a terrible way to filter for more important patches.

That said, using the code review system for the code review queue might be nicer than using wiki pages or whatnot. Now that we have hashtags in Gerrit it should be simple enough to set up. E.g., here's a search for patches with the please-review tag (I added it to the last code-review-hours nominee), with no negative reviews or test errors, which the current user has not reviewed. (The query is hashtag:please-review status:open -label:Verified=-2 -label:Code-Review<=-1 -is:wip -reviewedby:self.) If you think that's a reasonable approach, we could just get it documented somewhere, announce on wikitech-l, then mail a list of open patches to wikitech-l every week.

Mailing could be done by hand, or it's not hard to automate either; here's a script for fetching the relevant data in bash:

curl -s 'https://gerrit.wikimedia.org/r/changes/?q=hashtag:please-review+status:open+-label:Verified=-2+-label:Code-Review<=-1+-is:wip&o=DETAILED_ACCOUNTS&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=TRACKING_IDS&o=DETAILED_LABELS' | tail -n+2 | jq '.[] | { id: ._number, change_id: .change_id, project: .project, url: "https://gerrit.wikimedia.org/r/c/\(.project)/+/\(._number)", subject: .subject, owner: "\(.owner.name) <\(.owner.email)>", committer: "\(.revisions[].uploader.name) <\(.revisions[].uploader.email)>", message: .revisions[].commit.message | split("\n\n") [1:-1] | join(""), phab: [.tracking_ids[]] | map(select(.system=="Phab")|.id), created: .created[:10], updated: .updated[:10], cr_labels: .labels."Code-Review".all | map(select(.value!=0) | {key: "\(.name) <\(.email)>", value: .value}) | from_entries, unresolved_comment_count: .unresolved_comment_count }'
Tgr added a subscriber: Bmueller.Sep 6 2018, 12:40 PM

@Bmueller is this something volunteer dev support people at WMDE are interested in? (Tried to find the right project for that but got lost. Is it WMDE-Tech-Communication?)

[nitpick] There's no Verified-2 label; only V+1/+2/-1. Thanks.

@Bmueller is this something volunteer dev support people at WMDE are interested in? (Tried to find the right project for that but got lost. Is it WMDE-Tech-Communication?)

Hey @Tgr , generally interested, but currently we have no resources on top of the work we're already doing :-(. We don't have a project for all our activities around volunteer dev support, as it happens more on a project basis. For example there is Season of RevisionSlider for our current volunteer dev team project.

I'd like to remove the Developer-Advocacy tag from this task but which other tag to add?
I do not see Developer-Advocacy planning to work on this and I'm rather against working on this.
Every 2 or 3 weeks I send an email "Patchsets by new Gerrit contributors waiting for code review and/or merge" to wikitech-l@. It's something like a queue, it could create "awareness" in theory, and it is a "central place". And for the various reasons why code review does not happen, a number of proposed patchsets will also show in the edition afterwards. No additional manual queue will fix that. Better overall public data and public dashboards might though, in the long run.

mmodell added a subscriber: mmodell.Aug 6 2019, 2:49 PM
Ainali added a subscriber: Ainali.Sep 27 2019, 11:14 AM
kostajh added a subscriber: kostajh.Oct 8 2019, 2:23 PM

Hmm. Why is this supposed to work better than IRC code review hours, apart from not having to be around at a certain time?

That and not pretending that code review can be done within an hour.

Middle term: We have a list of last open (means: not merged && not abandoned) patchsets not created by bots and not created by WMF or WMDE folks at https://wikimedia.biterg.io/goto/2a7e85ff75feb0d3c8b21515392dac1d . However this list ignores CR±1/CR-2 labels (that's https://github.com/chaoss/grimoirelab/issues/96 ) so I assume it's not too useful?

CR+1 is not super useful, but yeah -1/-2 should be filtered out. Probably V-2 as well.
And Kibana is not a great interface for something like this IMO anyway.
(Also, I get a HTTP 500 for that link. I guess it expired?)

The equivalent gerrit search: https://gerrit.wikimedia.org/r/q/-ownerin:ldap/wmf+-ownerin:ldap/wmde+-label:Code-Review%253C0+is:open+-is:wip+-label:Verified%253C0+-is:assigned+-is:reviewed

Could also do the same search for small changes; i.e., less than 10 lines:

https://gerrit.wikimedia.org/r/q/-ownerin:ldap/wmf+-ownerin:ldap/wmde+-label:Code-Review%253C0+is:open+-is:wip+-label:Verified%253C0+-is:assigned+-is:reviewed+delta:%253C10

Might be worth it to make a dashboard of these folks could add to their menu

Dzahn added a subscriber: Dzahn.Tue, Aug 4, 7:58 PM