Launchpad reviews (was Re: Patch Pilot report)

[Martin Pool]

Aaron, you seem to be being pretty defensive about this, by
attributing all this feedback only to lack of understanding on my
part.  You don't seem very open to the possibility that the Launchpad
review app is not yet perfect.  Do you really think so?

I've seen members of the Launchpad team confused about how code
reviews work, so I really don't think it's just me being thick.  John,
Ian, Robert and Andrew have made similar comments.  Ian said today it
works well up to a few mps but falls over at 20, which seems to be
true.  The UI has O(n**2) behaviour.

It's reasonable to expect that a web app should be understandable and
usable from within its own gui, or at the very most by reading the
hypertext help.  It's true that I could have asked you personally, or
read the code, but that's not an adequate answer.

I really am trying to give constructive feedback on this.  I think the
code review feature is important and I want to help it.  The feedback,
in a nutshell, is that the workflow aspects are potentially good but
crippled by usability problems that could potentially be quite easily
fixed.   I'm quite concerned if you're going to just block this all
out as user error.

> Which three cases are conflated?

1- I am assuming responsibility for landing this
2- I am putting the responsibility back onto the submitter
3- Someone from ~bzr, not necessarily me, will be responsible

This determines "what next" as far as workflow.  In the first case,
"finish the patch" is something I can & should do; in the second case
it's not something I need to worry about at all; in the last case
something I can do.  This is important to guiding the patch pilot's
work, for example, but it's not really handled at the moment.

>> Perhaps this changed, but 'resubmit' used to have the special
>> behaviour of creating a new mp, not asking someone to resubmit.
>
> The "resubmit" vote has never had that effect.  There used to be a
> "resubmit" entry in the status selector, but that has been turned into
> the "resubmit proposal" action.

I'd forgotten you had two controls with the same name and different
undocumented effects!

> But the significance of that feedback also depends on the degree of
> understanding demonstrated by the user.  The critique of a user who does
> not thoroughly understand the system may simply indicate a need for
> documentation or education.

The first two points on my list were asking for documentation, but
documentation is a poor substitute for an understandable interface,
especially for a web app, and you educating people individually will
not scale.

>> The specific changes I'd like to see are:
>
> (These are a lot more palatable than the "suggest a next action out of
> these 10 or so" thing that I was responding to)

I did actually specifically say that I was listing the user
intentions, not how it should be technically implemented.

>>  * 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
>
> I'm not sure what you mean.  You mean that when there is a review
> request for bzr-core, and you perform a review, that review request
> isn't converted into a review request for Martin Pool?  Or do you mean
> that when you perform such a review, you want it to be attributed to the
> team?  As well as yourself?

I just tested that in
<https://code.staging.launchpad.net/~vila/bzr/releasing-clarified/+merge/10854>
and yes, that seems to be true that it's not converted.  It has a
review from me, but still says "bzr-core: Pending", but now the "claim
review" button has gone.  Does this now mean that the mp wants another
review from the bzr-core team, as well as from me?  That would explain
the case I mentioned above.  That's not what I wanted to have happen:
I was doing the review on behalf of ~bzr-core.  At any rate this mp
does not need another review from them.

>>  * add a field to the review form to set the new status of the mp
>
> I don't think we want something that general.  We wouldn't want people
> voting "disapprove" and selecting "approve" as the status, for example.

That's a somewhat silly counterexample because people could also write
"your code sucks and I hate you" and then vote approve.  Or they can
vote approve and then set the status to disapprove through separate
web forms, or in a single email.  So what?

Anyhow, the general case is this: the conceptual review causes a
change in the status of the proposal, and most status changes happen
because of reviews.  We don't want to hardcode policy about how that
link happens.

At the moment you must change the status separately.  At best, this is
another roundtrip.  (I see it's ajaxy now, but it's still in a
different page and visually separate.)  At worst, people don't
remember/discover they need to do it, leading to mps left in "needs
review" state, which means the workflow features don't work well.  My
speculation is that if you tied together "what's your vote" with "what
happens next?" then you would get better data therefore better
workflow guidance.

I gave these examples of possible improvements to make it concrete and
to show some small changes must help.  If you can think of better
things, that's great.

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