Issue 1383: Display reviewers and existing votes on the Publish Comments page
Status:  Released
Owner: ----
Closed:  Jun 2012
Reported by jhans...@myyearbook.com, May 14, 2012
Currently from the main change screen, you can see the list of reviewers on the change, and whatever vote(s) each reviewer has given (displaying the vote as a number, a green checkmark, or a red X, as determined by the vote's approval/rejection status).

However, many people may be stepping through files instead of going back to the main change screen, and only use the "R" keyboard shortcut from the last file to take them -- to the Publish Comments page -- in order to make their vote and publish.

The problem, at least in our organization, is that in the event a change requires two or more people to approve it, existing votes might influence whether I give a +1 or +2 on the change.  For example, if I'm the first to review it, but I think the change is big enough that I'd like to have another reviewer give the final +2, I may only give it a +1.  If it's small and I know it is an acceptable patch, I may give it a +2.  Or if it's a large change and someone else has already given it a +1, I may give it a +2 as final approval.

Right now what I end up doing is getting to the last file in the review, hitting "R", and then I realize that I'm not sure if I should give it a +1 or +2.  So before I submit my vote, I have to click Cancel to take me back to the main change screen, in order to see what other votes are already there.  Other times I may give it a +1, and then I land back on the main change screen, only to see that someone else also already gave it a +1, so I have to go *back* in to the review page and change my vote from +1 to +2.

Instead, if on the Publish Comments screen, I see the same list of Reviewers that I would see on the main change overview screen, then I would know immediately what the other votes are, and then I can simply submit my vote from that screen.

It does not necessarily need to have the ability to add/remove reviewers from the change on that screen, but I think that could be equally useful, especially if that makes the feature easier to implement.  E.g., if the author only added me as the single reviewer, but I want to defer final approval to someone in particular who I know has had more experience with that section of code, then I might be inclined to give my "+1" vote, add a comment saying "LGTM, but would like to have Joe Smith take a look as well, for final approval", and then add Joe Smith directly from that page before publishing my comments.

I'll look into supplying a patch for this.  Just wanted to have this open in case someone else already has it, or is able to knock it out quickly (I'm not familiar with the code yet, so it would take me a little while to get up to speed)
May 16, 2012
#1 jhans...@myyearbook.com
A patch has been uploaded at https://gerrit-review.googlesource.com/35141
Jun 17, 2012
#2 sop@google.com
(No comment was entered for this change.)
Status: Submitted
Labels: FixedIn-2.5
Jun 18, 2012
#3 sop@google.com
Change had to be backed out, buggy implementation with display.
See https://gerrit-review.googlesource.com/36200
Status: Accepted
Labels: -FixedIn-2.5
Jun 18, 2012
#4 jhans...@myyearbook.com
@shawn: I'll take a look at this.  I didn't have this problem during development.  What userbase source causes this behavior?  OpenID?  I was testing with LDAP and gerrit-internal.
Jun 18, 2012
#5 sop@google.com
The auth.type is that weird custom_extension one that nobody except me uses. Its not fully functional in the open source server either, you have to bind in a bunch of your own implementations of interfaces and servlets that are normally supplied by OpenID or LDAP alternates but are skipped when custom_extension is used.

I left a comment on your original change, I think the bug is an ordering issue.
Jun 18, 2012
#6 madco...@gmail.com
Awesome, I got your message.  It makes perfect sense.  I didn't notice it during testing, for the same reason you didn't.  But I will make that change, and will try to throw in a unit test to make sure it still works.

Thanks for taking the time to investigate!  If "nobody except [you]" uses it, it makes me feel less bad, but still should have noticed it..  I thought I tested with LDAP, but apparently I did not.
Jun 19, 2012
#7 sop@google.com
(No comment was entered for this change.)
Status: Submitted
Labels: FixedIn-2.5
Oct 31, 2012
Project Member #8 edwin.ke...@gmail.com
(No comment was entered for this change.)
Status: Released