Launchpad reviews (was Re: Patch Pilot report)

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> You don't seem very open to the possibility that the Launchpad
> review app is not yet perfect.  Do you really think so?

I accepted some of the points in your previous email.  I pointed a way
that we could improve Launchpad's handling of your case 1.  I don't
think that's consistent with a view that Code Review is perfect.  I only
have to look at the bug list to see that it's not perfect.

> 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.

You have had some really prime opportunities to get rid of any confusion
you may have had.  I could have answered any questions you had at the
sprint, for example.

> 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.

Asking me personally is not a good answer in the general case, but since
you know me personally, and as project leader have an interest in Bazaar
getting the most out of the code review system, I'm surprised that you
haven't done so.  You could also ask Tim Penhey, who is in a similar
timezone, and who you see at team lead meetings.

Documentation updates can certainly be done.  You mentioned several bugs:

- - confusion about the meanings of the votes
- - the terminology of the activereviews headings being less-than-ideal
- - documenting the meaning of the activereviews headings

I haven't been able to find them.  Could you point me at them, please?

> 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.

As a frequent user of the system, I'm pretty confident that it's not
crippled.  Of course, that doesn't mean it's simple or user-friendly.

> I'm quite concerned if you're going to just block this all
> out as user error.

I can accept that you experience problems, and that they need some kind
of solution.  But how can I accept your suggestions of what we should do
if you don't understand what we're already doing?

> 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.

I don't understand what this statement means.  Where did Ian make it?
Is it about speed or page space, or ...?

>> 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

Ah, I see.  Well, I've already proposed how to separate 1, though it
will require some enhancement of the "resubmit" action.

I guess 3 could be handled in a similar way, by creating a new branch
owned by ~bzr, and submitting a merge proposal that supersedes the
current one.

The issue I'm grappling with is that the merge proposal refers to a
particular branch.  If you don't merge that branch, you haven't really
performed the requested merge.  But the branch encodes the ownership
information, which in turn implies that the branch owner must must fix
the issues raised in code review.  It's not really possible to assign
the task to someone else without changing the source branch.

>> 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!

You should understand that votes and statuses are separate.  They were
separate in Bundle Buggy too.

>> 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.

Well, ideally the process of educating individuals would lead to better
documentation and UI, reducing the need to educate individuals.  I'll
never know by myself what needs to be documented and in what detail.

>>> 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.

Well, I responded to 'When someone works on an mp the system should be
asking them "what's the next action?" much more clearly than at
present.'  I'm glad you didn't mean it literally, but I don't understand
what you meant metaphorically.

>>>  * address the problem of people creating reviews as themselves when
>>> they want to review on behalf of a team
>> 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?
> 
> 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.

Okay, my best guess is that the review request is that there was an
invisible difference between the code review type of the review request
and the code review type of the review.  Something like None vs ''.

I experienced that bug when I initially reviewed it, but when I deleted
it and recreated it, the automatic claiming did behave as expected:
https://code.staging.launchpad.net/~vila/bzr/releasing-clarified/+merge/15180

> 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?

Yes, that what its current state indicates, but it should not have
entered that state.

>> 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?

A computer can tell that vote: approve and status: disapprove is
inconsistent.  It can't tell that vote: approve and "your code sucks and
I hate you" are inconsistent.

So quite possibly when the user chooses "approve" as a vote, we'll
reveal a checkbox that lets them also select "approved" as the status.

> 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.)

It's not always on a different page.  You can also review via the
comment box at the bottom.

> 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 agree.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAksMgaEACgkQ0F+nu1YWqI0pBACgh1GVB1lZjKvyFaXzcSN7SFpK
SEUAnA/WDz0P8lZccyFxtCwm+H0RjJDN
=dJ0F
-----END PGP SIGNATURE-----