New issue
Advanced search Search tips
Starred by 42 users
Status: Released
Owner: ----
Closed: Oct 2014



Sign in to add a comment
Gerrit should warn when try to submit someone else's patch
Project Member Reported by omakoto@google.com, Feb 20 2010 Back to list
Affected Version: 2.1.1.1

Currently these "Publish Comments" and "Submit Patch Set" buttons are next 
to each other, and it's very easy to mistakenly submit someone else's patch 
when actually trying to publish comments.


Changing button layout would be great, but I think it'd also be great if 
gerrit shows a big warning screen when trying to submit someone else's 
patch.
 
Project Member Comment 1 by shane...@gmail.com, Feb 20 2010
The button layout seems fine to me, and I'd rather not have to click through a
warning every time I submit a change.

More often than not for us, someone submits a change, it gets automatically QAed,
then someone else will approve it and submit it.

Its rare that someone submits their own change.
Comment 2 by sop@google.com, Feb 25 2010
Unfortunately I'm with ShaneMcC on this one.  I don't think it
makes sense to add an extra check just to submit a change.  But
I know folks within Google use a workflow where the user submits
their own change and (rarely if ever) submits someone else's.  So
maybe this should be a permission thing?  Instead of blanket turn
on the submit button we turn it on only if its your change?
Comment 3 by omakoto@google.com, Feb 25 2010
That'd be fine for me, but might break someone else's workflow...
Please fix this.

I ran into this last week, submitting my coworker's change by mistake. And today, I saw somebody else 
submit a change accidentally. It's extremely annoying because submitting a change is so hard to undo! 
Particularly with automerges etc. It's exactly like the "ejector seat" button here:
  http://www.codinghorror.com/blog/2010/03/the-opposite-of-fitts-law.html

The fix could be as simple as picking a different color for the submit button. Or moving the buttons further 
apart. 
Comment 5 by bdc@google.com, Apr 21 2010
Just saw another accidental patch submit by a reviewer...

The android workflow seems to be submit your own changes. Are the alternate workflows 
on other projects? perhaps it should just be a configuration option?
Comment 6 by sop@google.com, Jun 16 2010
Status: AwaitingInformation
I still have no idea how to implement this effectively.

Maybe the feature is to add a user-account level preference?

 [ X ]  Prompt before submitting another user's change

And enable it by default?  Power users who frequently submit other changes can
disable it, but for workflows like many of the folks above, it would help you to
avoid submitting something that isn't yours.

There may be some annoyance on a site like review.source.android.com where
in one project you frequently submit other people's contributions, as you are
the maintainer, but for the rest of the projects you submit only your own work
and never want to submit someone else's.
Could you just make the button red? Or move it somewhere else on the page? Putting a non-reversible action like submit next to the the regular review button is what I find most troublesome.
Yes, what's wrong with putting it right aligned on the page?
Project Member Comment 9 by shane...@gmail.com, Jun 16 2010
On a 1920x1200 monitor it would be very out of the way if it was at the far right.

Also, in our project its pretty much always someone else who submits the code, as its often the person who gives the final +2

I've never accidentally clicked submit, and it would look ugly to have the button red or right aligned.

I guess the setting is the best way, but I wouldn't want it to default to on.
Comment 10 Deleted
Comment 11 by bdc@google.com, Jun 16 2010
Changing color and alignment were just quick fix suggestions, but the confirmation seems best. having an all or nothing personal settings seems like it might not be a good fit. does it make sense to be per site or even per branch or ??? Because I'd say it should default to on for me and my peers and I don't want people to only discover there was an option after the first mistake (or perhaps never even after mistakes). 
Comment 12 by sop@google.com, Jun 16 2010
I'm going to space the buttons out more, so there is a
bigger gutter between them in case your mouse slips too
close to the edge of your target.

But coloring the button yellow/red just draws massive
attention to it, making me want to click it even more,
so that's a non-starter.

I think that for 2.1.3 we'll just space these out a bit
and revisit confirmations in a later release.
Comment 13 by sop@google.com, Jun 16 2010
Status: Accepted
Comment 14 by tyanh@google.com, Jun 30 2010
I accidentally submitted someone else's change by mistake today. My fingers move faster than my brain even though there's plenty of space between two buttons. It'd be nice if there's a confirmation. Better yet, an "undo" link like in gmail to cancel the action would work best for both work flows.
Comment 15 by sop@google.com, Aug 23 2010
 Issue 679  has been merged into this issue.
Comment 16 by tomo@google.com, Mar 23 2011
A lot of us have been hit by this.  Let's stop debating fixes and just implement one.  Even just using a different color for the button would be a big improvement on the current situation.
Comment 17 by bdc@google.com, Mar 23 2011
Happened to me on CL 100000 because a voyeur trying to comment on the CL accidentally submitted it: https://android-git.corp.google.com/g/#change,100000
I think there should in general be more control who may submit a change. For example, we would want to have a policy that the reviewer of a change may not submit it (unless someone else has reviewed it +2, too). 
Comment 19 by sop@google.com, Jul 6 2011
Labels: Milestone-2.2.2
Once my intern's pending change for rule inheritance is submitted, we should be able to write a configuration for a 2.2.2 server that says only the change author can click the submit button. We can also do what comment #18 asks, to require that the submitter not be a reviewer unless there is also another reviewer on the change.
Comment 20 by care...@gmail.com, Jan 31 2012
considering that 2.2.2 has been released already, could someone comment on the configuration/rule that would be needed for enabling this?
Project Member Comment 21 by mf...@codeaurora.org, Feb 23 2012
Well, the submit button seems to be the only one which does not bring up another screen or popup before applying its action (I haven't checked the new Publish, or Delete buttons, sorry).  So, perhaps all we need to do here is to add a comment popup to the submit action.  I hate to slow things down with a "confirm" button, but adding the ability to comment on a submit action might even make things more consistent (you can comment on it via ssh) and solve the confirm problem too.

Another approach might be to eventually do something like some of my trading accounts do, put some of the actions in a pulldown menu instead of in buttons (but I would leave review as a button or a link perhaps).  In the pulldown we could put Submit, Publish, Delete, Abandon, Restore and any new actions which come up soon (Rebase)...  That would also serve the dual purpose of making these actions take less space on the screen, something that has recently started to concern me since I have many ideas for new actions. :)
Comment 22 by tomo@google.com, Feb 24 2012
I guess any of these suggestions would work for me, thanks.  One nice thing about the popup is that it could have a red warning banner if you're not the author.
Project Member Comment 23 by edwin.ke...@gmail.com, Mar 5 2012
I like Martin's suggestion with having a pulldown for most of the actions on the change screen. We are really starting to have too many buttons here. It makes sense to keep the most important actions (review) as button or link. I also think it is useful to being able to provide a comment on submit.

Regarding the 'Publish' and 'Delete' buttons for drafts there is currently no popup, but the actions are just performed.

I'm not so sure about having a red warning if you are about to submit a change of somebody else. We actually follow mostly the rule that you should not submit your own changes. So for us it would make more sense if you get a red warning when you are about to submit your own change. So maybe we should just stay without warning to not imply any processes here.
Comment 24 by bdc@google.com, Mar 5 2012
I think having a button to combine two operations into one (publish and submit) is already favoring one process over another (that reviewers submit others changes rather than the authors themselves). The fact that many of are having to install chrome extensions to remove this button because we all accidentally hit it when we shouldn't seems to indicate that this should be seen as more than a minor issue. accidentally submitting someone else's changes and breaking the build is not minor.
Comment 25 by tomo@google.com, Mar 5 2012
I concur with b...@google.com.  The advantage of having the author submit their own changes is that they're in the best position to run the tests before submitting.  My guess is that the "don't submit your own CL" process that edwin.ke...@gmail.com follows is relatively rare.

But if you want to make accidental self-submissions more difficult too I guess that's OK with me.
Project Member Comment 26 by bklar...@gmail.com, Mar 5 2012
Not that it really matters, but I think 'only submit your own CL' is only really popular inside Google.  Especially in the open-source world (Android, jgit/egit, gerrit, etc), the pattern is to upload patches and have them reviewed/tested/merged by the experts.
Comment 27 by tomo@google.com, Mar 5 2012
Well it matters in that you have been able to reduce my ignorance on the subject. ;-)
Thanks!
At my company we also use the pattern 'only submit your own CL' so I think there is a general pattern where companies typically use 'only submit your own CL' whereas open source projects typically do NOT use 'only submit your own CL'
Comment 29 by di...@google.com, Mar 5 2012
I'm not sure if it's a company vs open source community difference, seems to me to be more of a "change authored by team member" vs "change authored by outside party" issue. For the former case you generally want the author to submit, for the later, they probably don't have submit rights anyways (inside companies, submit right restrictions may be more lax and so the "team member" relation may extend to all employees in that case).
I agree with di...@google.com's assessment in comment 29.  In our organization, everyone who is able to submit changes for review, is also able to submit that review *after* it has the necessary +2 approvals.  Not all of our developers have ±2 access, but everyone has ±1 access.  And only our QA department has +1 Verified access.  So, it's conceivable that a reviewer would +2 the change first from a code perspective, before QA has gone through it to verify that the change does what it's meant to do.

So in general (talking about how it works in *our* organization only), there are 3 parties involved:  the author, the reviewer(s), and the QA tester.  QA can +1 Verified, but cannot submit and cannot review.  Reviewers can +1 and/or +2 to approve the code, but cannot +1 Verify the change.  The author can't really vote at all because it's difficult to be objective regarding *your own* change;  therefore the only job remaining is that the author can submit the change, after reviewer and tester have approved.

In our organization, the author is the only one who is totally involved from start to finish with the change.  QA only tests the product, and reviewers only look at the code.  If I review the change but QA hasn't looked at it yet, I don't want to have to sit around watching my Gerrit dashboard, waiting until QA touches some product that I'm not really even involved in.  It should be the author.  Actually, Gerrit seems to be built for that environment anyway, because if you're a reviewer on the change, and you haven't voted yet, it shows up bold in your dashboard -- until you vote.  After you vote, it's not bold anymore, and therefore you have no reason to go back and look at it.

That's just my argument for it.  It's also annoying when someone who actually doesn't know anything about the project jumps in and submits the patch before I was ready for it to be submitted.  I know the change, and I know what to do after it's submitted, and what requirements must be met before it can be submitted, so I want to be the one to submit it.
Project Member Comment 31 by edwin.ke...@gmail.com, Mar 9 2012
For such use cases there should be maybe a prolog rule that enables the submit only for the change author.
Comment 32 by jbjo...@gmail.com, Oct 26 2012
I think we could introduce a new prolog category "warn" in addition to the existing need/may/ok/impossible.

warn() could take a text string that will be shown in a popup when the user presses the submit button.

warn("Your organization has a policy that only the code-author should submit changes, are you sure you want to continue?")
See : http://code.google.com/p/gerrit/issues/detail?id=891 for example on how to only allow the author to submit.


Comment 33 by org...@gmail.com, Oct 29 2013
Adding a warning would be great
Comment 34 by tomo@google.com, Oct 29 2013
This has been open for 3 1/2 years, and is probably the biggest usability issue in gerrit. At this point *any* improvement would be welcome, even if it's something as simple as changing the color of the submit button. What exactly is preventing some kind of fix here?
Project Member Comment 35 by edwin.ke...@gmail.com, Oct 29 2013
For most people the normal workflow is that you do not self submit your own changes, but that the submit is done by another person. By this it is ensured that at least two persons agreed to the change (the owner and the submitter). For this workflow you don't want to have a warning when you submit a change of somebody else. Still enabling this warning could be configurable per project. If you have a workflow that requires this warning, you are welcome to implement this and contribute this feature. It's unlikely that somebody who doesn't have a need for such a warning, will spend the effort to implement this for you.
Project Member Comment 36 by david.os...@gmail.com, Sep 19 2014
On new change screen JavaScript API was added to enable plugins to define a hooks on different UI actions, among other "onSubmit". Cookbook-plugin has an example how to do that. Should be easy to do now. I haven't checked if plugin can easily figure out if the current user is the owner of the change or not.
Project Member Comment 37 by david.os...@gmail.com, Sep 21 2014
Status: ChangeUnderReview
https://gerrit-review.googlesource.com/60248
Project Member Comment 38 by david.os...@gmail.com, Oct 1 2014
Labels: -Milestone-2.2.2 FixedIn-2.11
Status: Submitted
Example how to do it in plugin: [1].

[1] https://gerrit-review.googlesource.com/60470
Status: Released
Sign in to add a comment