Launchpad reviews (was Re: Patch Pilot report)

[Martin Pool]

2009/11/24 Aaron Bentley <aaron at aaronbentley.com>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Martin Pool wrote:
>> Let me add a few more things:
>>
>> The criticism is intended to be constructive; the page is already
>> quite useful but it has the germ of an amazing system.
>>
>> The "wants review from/claim review" system is interesting, but not
>> fully realized.  It should be clear from the list "needs review from
>> me in particular"
>
> 'Reviews I have to do'

I guess this heading is absent because there are none of them?
Understandable, but it makes the categorization maybe less
discoverable.

>> "needs review from someone on the team",
>
> 'Requested reviews I can do'
>
>> "doesn't
>> need more reviewers".
>
> 'Other reviews I am not actively reviewing'
>
> It's not clear to me whether you're critiquing our terminology or didn't
> understand this.

I don't think the terminology is ideal, and I have filed a bug to that
effect, and I think also one asking that you should document what they
do mean.  But more importantly the emergent behaviour does not align
with the way you expect it to work: for example
<https://code.edge.launchpad.net/~vila/bzr/releasing-clarified/+merge/10854>
is shown in "reviews I can do" but in fact there is nothing useful for
me to do there.

>> When someone works on an mp the system should be asking them "what's
>> the next action?" much more clearly than at present.
>
> It sounds like you're suggesting a wizard-style approach.  That's quite
> different from most areas of Launchpad.  For example, updating or
> commenting on a bug report doesn't propose further actions.

Updating a bug does naturally lead you into changing its state and
priority, which determine what happens to it next.  (The recent ajax
changes have weakened this link, which I think is a problem, much as I
like them in other regards.)

I actually think the most highly evolved workflow tool in Launchpad is
the Answers system and that does very directly invite you to change
state when commenting: "Thanks, that answered my question" etc.
Taking ideas from that would be great.

>>  * I will do the changes and land it
>
> I think that would be best expressed by creating your own branch and
> creating a new merge proposal that supersedes this one.  The 'supersede'
> part isn't something we currently support.
>
>>  * I want someone to do the changes and land it, but not me
>>  * I want the submitter to do the changes and land it (not sure what
>> status this is)
>
> "needs-fixing"

These three cases have quite different implications for the next
action, and I think the fact they're conflated causes some churn.

>>  * I want the submitter to do the changes and resubmit it (I guess
>> this is 'needs fixing'?)
>
> It is "resubmit".

Perhaps this changed, but 'resubmit' used to have the special
behaviour of creating a new mp, not asking someone to resubmit.

I've been involved with this for something over four years and I've
done probably over a hundred reviews in the new system.  I'm not being
intentionally bloody minded.  So if I'm not sure what the statuses
mean, and apparently most other bzr core team members don't either, I
think there is a real problem.

>>  * I need the submitter to do something other than to the code, eg to
>> sign the contributor agreement or confirm that they tested the
>> performance; nobody can progress this until then
>
> Perhaps a review of "needs info", and resetting the status to "work in
> progress".  Certainly, confirming that they tested the performance is a
> case of "needs info".
>
>>  * I need to come back and finish reviewing it
>
> Claim review.  Optionally, add a comment.
>
>>  * We need to discuss and agree on whether this is even a good idea
>
> Sounds like a special form of "request review".
>
>> These are kind of captured, but not optimally clearly or easily, and
>> they're not clearly displayed on the overview page.
>
> The code review system is not intended to enforce a particular workflow;
> it's meant to provide enough flexibility to enable teams to use it as
> they see fit.

Let's be clear, I'm not asking for you to enforce a particular
workflow or to remove any generality.  I don't dispute that it's
technically possible to use it.  It's also possible to do reviews on
the mailing list or by attaching them to bugs.  The whole point of the
review system is to help with workflow, so user feedback that it does
not support their workflow well is pretty important.

The specific changes I'd like to see are:

 * document the headings in the overview (either in help or inline)
 * document the votes and statuses
 * address the problem of people creating reviews as themselves when
they want to review on behalf of a team
 * add a field to the review form to set the new status of the mp

Those are pretty small and then we could see what effect they have on
actual use.

-- 
Martin <http://launchpad.net/~mbp/>