Issue metadata
Sign in to add a comment
|
Buttons should be grayed out instead of missing if a user has no permission |
||||||||||||||||||||||
Issue descriptionCurrently, a button like "Submit to CQ" doesn't exist for non-committers. The same goes for "Code review +1". This often leads to confusion and miscommunication. How about keeping all (or at least most) buttons, but disabling them if a user has no permission for a button. When the user hovers over the button, the UI could show a short explanation, why the button isn't usable. Examples how this goes wrong: - I experienced 5+ times different non-committer reviewers pressing CQ +1 (aka dry run), because they didn't have a CR +1 button and they had the impression they needed to do something (their intention was to say lgtm). - Hangout chats can be confusing. The committer author might say which button to press, but the other person doesn't see the same buttons.
,
Jan 27 2017
In general I *totally* agree with this, and it's something that I'd like to improve. There are a few complications, however: 1) Some buttons (like Code-Review +1) are generated by Gerrit itself, not by our plugins. This makes them harder to manipulate, but not impossible. 2) Some buttons (like Code-Review +1) are displayed dynamically based on what labels Gerrit thinks need to be set for the CL to get into the "submittable" state. This means that displaying but grey-ing out all of those possible buttons would result in a *lot* of buttons being displayed. 3) If we greyed out the buttons created by the commit queue plugin, but didn't do anything to change the behavior of the buttons created by Gerrit itself, it would be an inconsistent user experience (they would think that a button being missing is somehow more meaningful than being present but greyed out), which could be even worse than the current status quo. 4) Gerrit just implemented an overflow menu for buttons. Greying out items in an overflow menu is a really crappy user experience, and the js API to do so isn't even in place yet as far as I know. So that new feature has complicated the design of this feature. 5) Rietveld didn't grey out buttons, it just didn't display them. So technically we're currently closer to parity than we would be if we changed that behavior. So yeah I totally agree that having the buttons just be missing is not great. But I'm not sure that just naively greying out the ones we have control over is the best solution either. I'm going to put this in the afterglow milestone, because it's not clear to me how best to fix it, we should get more feedback, and it's not a regression.
,
Jan 29 2017
That's fair, thanks for investigating!
,
May 4 2017
I think my experienced 5+ from the description is 20+ by now. I'll start collecting some examples and dump them here to get a feeling about quantity. E.g.: https://chromium-review.googlesource.com/c/494488/#message-1bd343690b4aa3ae058cb16b702c1a011dd94601
,
May 4 2017
,
May 4 2017
The message that you just linked to is not related to this bug. That person doesn't have permission to CR+1 because they're reviewing a CL in an infra repo. They're a chromium committer, not an infra committer. This is unrelated to the buttons being greyed out vs missing.
,
May 4 2017
I think the point was that the UI is confusing. It really is. Please listen to your users' feedback ;) We're all reasonable people trying to do our jobs. In this case, the tool could be more helpful, and we're trying to tell you that.
,
May 4 2017
We are listening. I don't want users to be confused. I really want to improve PolyGerrit so that users aren't confused when they can't apply labels. This bug, however, is specifically about the buttons being greyed out vs missing. And unfortunately, my statements in comment 2 still stand. I don't think that changing the greyed out state will be a helpful fix, and it wouldn't have fixed the particular situation that machenbach mentioned in comment 6. If you have ideas about how to improve that particular case, I'd be more than happy to listen, especially in a separate bug thread dedicated to that issue.
,
May 5 2017
I think we have two problems when having a non-committer as reviewer: 1) The author doesn't know the reviewer's permissions -> orthogonal issue 683842 2) The reviewer is confused how to respond I'd like to take away some of this confusion. When chromium switches to gerrit, there will be more confused people. Instead of graying out the CR+1 button, I'd actually strongly support using Cr+1, Cr+2 buttons. This'd take away this confusion, the non-committer has a formal way to respond (without the response drowning in the message history), and committer approval has still the same formality as before. Graying out a button is basically a light version of this tri-state system. The problem is probably larger for people who do a lot of cross-repo work.
,
May 5 2017
Thanks for filing that parallel issue, Michael, I think that's a fantastic way to reduce a lot of confusion here. My main point about the greying out is that it wouldn't have worked in this case -- the user (as they note in their comment on the CL) was presented with the message "You do not have permission to set the Code-Review label". I'm not sure how showing the buttons but having them greyed out would be more clear/obvious than that. Personally, I'd love to have CR+1 and CR+2, with anyone able to set +1 and only committers allowed to set +2. In fact, we'll be headed that way shortly after launch. But so far feedback has indicated that making that change would be more confusing on the whole, so we aren't doing that immediately at launch.
,
Jan 3 2018
Removing Milestone-Afterglow, as it has ceased to have meaning. More refined milestones may be added back in the near future.
,
Mar 27 2018
At this point, we're not going to address this issue in the way this bug wants. Gerrit doesn't have a practice of greying out buttons, so our plugins won't either. We've done a lot of other work (especially in the Reply dialog) to make it clear whether or not you have permission to set certain labels, and I'm not hearing complaints about this particular flavor of confusion anymore. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by andyb...@chromium.org
, Jan 26 2017Status: Assigned (was: Untriaged)