It appears that people have asked me to review all merge proposals I look at

[Björn Tillenius]

Having the name of the currently logged-in user appear in the reviewer table looks confusing. I think having the "[Review]" link outside the table would be better, or maybe have the link where it is, but don't add my name to the table, if no one has requested my to review the branch.

When I look at a merge proposal, my name appears in the reviewer table. Thus it looks like someone requested me to review that branch, but since it is like this on every merge proposal I look like, I assume this is not the case? My name has a different color, but it looks simply like a visited link.

[Tim Penhey (thumper)]

On Sat, 15 Nov 2008 22:37:50 Björn Tillenius wrote:
> ** Summary changed:
>
> - It appears that people have asked me to review all merge proposals
> + It appears that people have asked me to review all merge proposals
> I look at

Well, the date requested is also empty...

I actually quite like this.

[Björn Tillenius (bjornt)]

On Mon, Nov 17, 2008 at 05:29:13AM -0000, Tim Penhey wrote:
> On Sat, 15 Nov 2008 22:37:50 Björn Tillenius wrote:
> > ** Summary changed:
> >
> > - It appears that people have asked me to review all merge proposals
> > + It appears that people have asked me to review all merge proposals
> > I look at
>
> Well, the date requested is also empty...

Well, you forget that people actually don't read everything on the page
;) Since someone had already reviewed the branch, I assumed that date
was 'date reviewed'. That's the most logical thing to put there if the
branch has already been reviewed, IMHO.

Now that you have informed me how things work, I won't believe someone
has requested me to review the branch (although I will probably still
believe that sometimes for a very short while, before I take a closer
look).

I filed this bug because that was my first impression. This is the kind
of issue you would have found if you would have done user testing with
my as a subject. I think other people will have the same impression as I
had. The phrase "Don't make me think" comes to mind. I think the current
UI really makes you think. you have to analyze it before you understand
it the first time (e.g., "oh, that date is date requested, and it's
empty, so therefore I'm not reqeusted to review it, I only have the
possibility to do it" as compared to "oh, my name isn't in the table, so
I'm not requested to review it"). Of course, this is just my opinion,
and maybe my brain works differently than everyone else's.

[Tim Penhey (thumper)]

On Mon, 17 Nov 2008 21:33:14 Björn Tillenius wrote:
> On Mon, Nov 17, 2008 at 05:29:13AM -0000, Tim Penhey wrote:
> > On Sat, 15 Nov 2008 22:37:50 Björn Tillenius wrote:
> > > ** Summary changed:
> > >
> > > - It appears that people have asked me to review all merge
> > > proposals + It appears that people have asked me to review all
> > > merge proposals I look at
> >
> > Well, the date requested is also empty...
>
> Well, you forget that people actually don't read everything on the
> page ;) Since someone had already reviewed the branch, I assumed that
> date was 'date reviewed'. That's the most logical thing to put there
> if the branch has already been reviewed, IMHO.
>
> Now that you have informed me how things work, I won't believe
> someone has requested me to review the branch (although I will
> probably still believe that sometimes for a very short while, before
> I take a closer look).
>
> I filed this bug because that was my first impression. This is the
> kind of issue you would have found if you would have done user
> testing with my as a subject. I think other people will have the same
> impression as I had. The phrase "Don't make me think" comes to mind.
> I think the current UI really makes you think. you have to analyze it
> before you understand it the first time (e.g., "oh, that date is date
> requested, and it's empty, so therefore I'm not reqeusted to review
> it, I only have the possibility to do it" as compared to "oh, my name
> isn't in the table, so I'm not requested to review it"). Of course,
> this is just my opinion, and maybe my brain works differently than
> everyone else's.

Well... part of the purpose of it was to make it easier to review.

The idea that since you are a valid reviewer you see a greyed out link
where you would appear if you did actually review it. It provides a
simple location for any reviewer to click on a link to review it whether
they had been asked or not.

[Martin Albisetti (beuno)]

What do you guys think about changing the wording of the links to:

(Team Name) [Claim Review]
(My grayed Name) [Add my review]

[Björn Tillenius (bjornt)]

On Tue, Nov 18, 2008 at 07:16:50PM -0000, Martin Albisetti wrote:
> What do you guys think about changing the wording of the links to:
>
> (Team Name) [Claim Review]
> (My grayed Name) [Add my review]

If someone had requested a review from my, I assume that it would look
exactly the same as above, but with the name not greyed out? IMHO that's
equally bad as now, since (as I mentioned), the currently greyed out
link looks like a visited link.

[Martin Albisetti (beuno)]

If someone requests a review from you, your name is not grayed out.
You only get your name grayed out when nobody has requested a review from you explicitly, but you have permissions to do so anyway.
Would the above text help clarify that?
The fact that it's visited is arguable I think and, to some extent, it's something that shouldn't be prominent, it's just a short cut so you don't have to request a review from yourself and then review, like it was before.

[Björn Tillenius (bjornt)]

On Tue, Nov 18, 2008 at 07:49:18PM -0000, Martin Albisetti wrote:
> If someone requests a review from you, your name is not grayed out.
> You only get your name grayed out when nobody has requested a review from you explicitly, but you have permissions to do so anyway.
> Would the above text help clarify that?

Sorry, I don't think the text would clarify it. It's not that much
difference between "claiming" and "adding" a review. If someone requests
a review from you, I'd say you'd "add" a review.

> The fact that it's visited is arguable I think and, to some extent,
> it's something that shouldn't be prominent, it's just a short cut so you
> don't have to request a review from yourself and then review, like it
> was before.

Agreed. The fact that it looks visited is just my opinion, maybe I'm
alone in thinking that. I do think that you can have a shortcut, by
having a link outside the table, or (as I think I mentioned), keep the
[Review] link inside the table, but remove my name. I.e., have an empty
row, with just a [Review] link. That would serve the same purpose, but
it wouldn't be that confusing.

[Martin Albisetti (beuno)]

> Sorry, I don't think the text would clarify it. It's not that much
> difference between "claiming" and "adding" a review. If someone requests
> a review from you, I'd say you'd "add" a review.

Maybe it would help clarify it in general, even though not in this
specific case.

> I do think that you can have a shortcut, by
> having a link outside the table, or (as I think I mentioned), keep the
> [Review] link inside the table, but remove my name. I.e., have an empty
> row, with just a [Review] link. That would serve the same purpose, but
> it wouldn't be that confusing.

That actually sounds like a good idea.
Tim, could you try that to see how it would look?

My guess is that we still need some sort of distinction between these
non-requested reviews and team reviews, because I think most people
aren't claiming them, when we actually do want them to.

Thoughts?

[Jonathan Lange (jml)]

On Wed, Nov 19, 2008 at 7:33 AM, Martin Albisetti <email address hidden> wrote:
> My guess is that we still need some sort of distinction between these
> non-requested reviews and team reviews, because I think most people
> aren't claiming them, when we actually do want them to.
>

Well, for team reviews you could actually render the words "Someone
from <team-name>". That'd be pretty clear.

jml

[Martin Albisetti (beuno)]

On Tue, Nov 18, 2008 at 6:57 PM, Jonathan Lange <email address hidden> wrote:
> Well, for team reviews you could actually render the words "Someone
> from <team-name>". That'd be pretty clear.

Would that be clear enough that it can me *you*?

[Martin Pool (mbp)]

This seems pretty complicated.

I think the normal case is that review will just be wanted from one or
two persons from the project team. In that case, having the concept
of multiple review invitations on the branch is just a distraction.

It seems like you only need to present this when the person could do a
review using one of several hats: are they expressing an opinion as
just a member of the team, or as the db guru? In that case, I'd
suggest just having a field on the review form saying in what capacity
they're writing this review.

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