Automatically check CQ bit after LGTM

Automatically check CQ bit after LGTM and uncheck it for 'not LGTM'.

Changes are staged here:
https://skia-codereview-staging2.appspot.com/490001/

[rmistry, 2014-02-20 18:08:45]

This is what I had in mind, what do you guys think?

[iannucci, 2014-02-20 20:27:38]

seems generally OK to me for current-rietveld.

In the new model, this logic should be fired by an event triggered by the
Message being added to the Issue, so it can live in a 'commit_queue' extension,
and not in the core model logic.

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py#newc...
codereview/models.py:327: self.commit = False
This logic should be in the loop above. No reason to loop over msgs again I
don't think?

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium...
codereview/views_chromium.py:397: if request.user.email().lower() ==
request.issue.owner.email():
no action comment: bleh. This is what I'm talking about when I say that the
authorization code is splattered all over the place. This sort of check should
live (IMO) in the model (and it does in codereviewv2, see the can_* methods on
the models).

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium...
codereview/views_chromium.py:400: request.issue.put()
no action comment: This is another anti-pattern. We should only ever put objects
once per request (and transactionally, natch).

[jrobbins (corp), 2014-02-20 23:34:40]

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py#newc...
codereview/models.py:104: lgtm_auto_commit = db.BooleanProperty(default=False)
I'd like to see a one-or-two-line descriptive comment of every field.  I know
that the existing code does not have that, but I think it makes sense to start
by adding ones for commit and lgtm_auto_commit so that the difference between
them is clear.

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py#newc...
codereview/models.py:327: self.commit = False
On 2014/02/20 20:27:39, iannucci wrote:
> This logic should be in the loop above. No reason to loop over msgs again I
> don't think?

Adding this feature here (rather than in a new method) stretches the purpose of
calculate_updates_for() beyond what is in the method docstring.  So, I'd like to
see the docstring updated, or a separate method defined.

Also, this seems to look for just the last non-owner message that has any
approval or disapproval.  I think it needs to be all(approval_dict.values()) to
check that all reviewers have approved and no one has jumped in to disapprove.  
This is what you called an AND mode in your email.  It seems better to start
conservative because you don't want to have unintended auto-commits that require
rollbacks or too many followup CLs.

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium...
codereview/views_chromium.py:396: request.issue.lgtm_auto_commit =
form.cleaned_data['lgtm_auto_commit']
Why not delete this line and just keep the assignment a few lines down where
lgtm_auto_commit is set after checking that the requester is the issue owner?

https://codereview.appspot.com/66340044/diff/100001/templates/patchset.html
File templates/patchset.html (right):

https://codereview.appspot.com/66340044/diff/100001/templates/patchset.html#n...
templates/patchset.html:154: Automatically check CQ box after LGTM:
I think this feature should start conservative with the AND semantics, so the
description would be:
"Commit after all reviewers give LGTM:".

https://codereview.appspot.com/66340044/diff/100001/templates/patchset.html#n...
templates/patchset.html:165: Commit: <input type="checkbox" id="commit"
name="commit"
Since we now have two similar options, I think we need to clarify the difference
between them in the UI.  So, the old "Commit" checkbox should have a more
precise label.  E.g., something like "Commit now:".

[rmistry, 2014-02-21 15:55:10]

Addressed comments and also did the following:
* Disable the new checkbox for non owners.
* Uncheck the new checkbox when the Commit checkbox is unchecked.

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py#newc...
codereview/models.py:104: lgtm_auto_commit = db.BooleanProperty(default=False)
On 2014/02/20 23:34:40, jrobbins1 wrote:
> I'd like to see a one-or-two-line descriptive comment of every field.  I know
> that the existing code does not have that, but I think it makes sense to start
> by adding ones for commit and lgtm_auto_commit so that the difference between
> them is clear. 

Done.

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py#newc...
codereview/models.py:327: self.commit = False
On 2014/02/20 20:27:39, iannucci wrote:
> This logic should be in the loop above. No reason to loop over msgs again I
> don't think?

Removed the loop due to Jason's below comment.

https://codereview.appspot.com/66340044/diff/100001/codereview/models.py#newc...
codereview/models.py:327: self.commit = False
On 2014/02/20 23:34:40, jrobbins1 wrote:
> On 2014/02/20 20:27:39, iannucci wrote:
> > This logic should be in the loop above. No reason to loop over msgs again I
> > don't think?
> 
> Adding this feature here (rather than in a new method) stretches the purpose
of
> calculate_updates_for() beyond what is in the method docstring.  So, I'd like
to
> see the docstring updated, or a separate method defined.

Updated the docstring.

> 
> Also, this seems to look for just the last non-owner message that has any
> approval or disapproval.  I think it needs to be all(approval_dict.values())
to
> check that all reviewers have approved and no one has jumped in to disapprove.
 
> This is what you called an AND mode in your email.  It seems better to start
> conservative because you don't want to have unintended auto-commits that
require
> rollbacks or too many followup CLs.

I think this is a fantastic suggestion. One day when Rietveld handles AND / OR
reviewers this logic can similarly change.
Done.

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium...
codereview/views_chromium.py:396: request.issue.lgtm_auto_commit =
form.cleaned_data['lgtm_auto_commit']
On 2014/02/20 23:34:40, jrobbins1 wrote:
> Why not delete this line and just keep the assignment a few lines down where
> lgtm_auto_commit is set after checking that the requester is the issue owner?

Oops. Deleted, this line should not have been here.

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium...
codereview/views_chromium.py:397: if request.user.email().lower() ==
request.issue.owner.email():
On 2014/02/20 20:27:39, iannucci wrote:
> no action comment: bleh. This is what I'm talking about when I say that the
> authorization code is splattered all over the place. This sort of check should
> live (IMO) in the model (and it does in codereviewv2, see the can_* methods on
> the models).

Acknowledged.

https://codereview.appspot.com/66340044/diff/100001/codereview/views_chromium...
codereview/views_chromium.py:400: request.issue.put()
On 2014/02/20 20:27:39, iannucci wrote:
> no action comment: This is another anti-pattern. We should only ever put
objects
> once per request (and transactionally, natch).

Acknowledged.

Wouldn't it be great if Reitveld had
'Reply Reply_with_quote Done Ack'
instead of
'Reply Done'

Maybe I'll do that in my next CL.

https://codereview.appspot.com/66340044/diff/100001/templates/patchset.html
File templates/patchset.html (right):

https://codereview.appspot.com/66340044/diff/100001/templates/patchset.html#n...
templates/patchset.html:154: Automatically check CQ box after LGTM:
On 2014/02/20 23:34:40, jrobbins1 wrote:
> I think this feature should start conservative with the AND semantics, so the
> description would be:
> "Commit after all reviewers give LGTM:".

Done.

https://codereview.appspot.com/66340044/diff/100001/templates/patchset.html#n...
templates/patchset.html:165: Commit: <input type="checkbox" id="commit"
name="commit"
On 2014/02/20 23:34:40, jrobbins1 wrote:
> Since we now have two similar options, I think we need to clarify the
difference
> between them in the UI.  So, the old "Commit" checkbox should have a more
> precise label.  E.g., something like "Commit now:".

This SGTM. Done.

[jrobbins (corp), 2014-02-21 20:35:01]

not lgtm.  I'm just saying that because the message text itself contains the
string l-g-t-m.

I think this is close to being ready. But now that I see the UI change and can
imagine using it, I wonder if we should just change the meaning of a single
checkbox rather than offering two checkboxes in the UI?  (I think I understand
why we need 2 booleans in the DB).

https://codereview.appspot.com/66340044/diff/120001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

https://codereview.appspot.com/66340044/diff/120001/codereview/views_chromium...
codereview/views_chromium.py:413: request.issue.lgtm_auto_commit = (False if not
request.issue.commit
This seems confusing. I think it would be more clear with the if-statement
controlling the assignment:

if not request.issue.commit:
  request.issue.lgtm_auto_commit = False

Also, I was concerned that every time the user submits the form that this logic
might undo their setting for the other checkbox, which would be mysterious and
frustrating.  Luckily I remembered from the template that we actually have two
distinct forms each of which has one checkbox and not the other.    I think you
could just document that here.  

Also, it's a little counter-intuitive that only the issue owner can check the
auto-commit checkbox, but anyone can effectively uncheck it by toggling the
commit checkbox twice.  I see why that might be needed, but a comment would help
future maintainers understand why it needs to be that way.  

The UI might be confusing too, because if the auto-commit was checked and the
commit was checked, and then I uncheck the commit box: I will be looking at a
page that still has the auto-commit box checked, even though that bit has been
cleared in the database.  If you use JS to uncheck the auto-commit box, you will
likely trigger another AJAX request, but I guess it would be a no-op.  Would it
be better to just offer the auto-commit checkbox alone?  Will anyone still need
the regular commit checkbox?

[iannucci, 2014-02-21 21:23:51]

On 2014/02/21 20:35:01, jrobbins1 wrote:
> not lgtm.  I'm just saying that because the message text itself contains the
> string l-g-t-m.
> 

Haha 'surprise!' :)

We should probably prefix the code snippet lines with a > or something. Or have
the lgtm logic only look at the comment bodies.

> I think this is close to being ready. But now that I see the UI change and can
> imagine using it, I wonder if we should just change the meaning of a single
> checkbox rather than offering two checkboxes in the UI?  (I think I understand
> why we need 2 booleans in the DB).
> 
>
https://codereview.appspot.com/66340044/diff/120001/codereview/views_chromium.py
> File codereview/views_chromium.py (right):
> 
>
https://codereview.appspot.com/66340044/diff/120001/codereview/views_chromium...
> codereview/views_chromium.py:413: request.issue.lgtm_auto_commit = (False if
not
> request.issue.commit
> This seems confusing. I think it would be more clear with the if-statement
> controlling the assignment:
> 
> if not request.issue.commit:
>   request.issue.lgtm_auto_commit = False
> 
> Also, I was concerned that every time the user submits the form that this
logic
> might undo their setting for the other checkbox, which would be mysterious and
> frustrating.  Luckily I remembered from the template that we actually have two
> distinct forms each of which has one checkbox and not the other.    I think
you
> could just document that here.  
> 
> Also, it's a little counter-intuitive that only the issue owner can check the
> auto-commit checkbox, but anyone can effectively uncheck it by toggling the
> commit checkbox twice.  I see why that might be needed, but a comment would
help
> future maintainers understand why it needs to be that way.

I'm not actually sure I understand why only the owner can set the new box? The
reviewers can already tick the CQ button, and we even have a 'lgtm+cq' button in
the publish comments section.
  
> 
> The UI might be confusing too, because if the auto-commit was checked and the
> commit was checked, and then I uncheck the commit box: I will be looking at a
> page that still has the auto-commit box checked, even though that bit has been
> cleared in the database.  If you use JS to uncheck the auto-commit box, you
will
> likely trigger another AJAX request, but I guess it would be a no-op.  Would
it
> be better to just offer the auto-commit checkbox alone?  Will anyone still
need
> the regular commit checkbox?

[iannucci, 2014-02-21 21:24:44]

ack, busted! I did the same thing you were trying to avoid :( 'not lgtm'

[rmistry, 2014-02-21 21:26:00]

I did not make any code changes, I think we should first flesh out the design.
BTW thanks for thinking through this, I was not 100% sure of the flow and I am
glad we are talking about it.

On 2014/02/21 20:35:01, jrobbins1 wrote:
> not lgtm.  I'm just saying that because the message text itself contains the
> string l-g-t-m.
> 
> I think this is close to being ready. But now that I see the UI change and can
> imagine using it, I wonder if we should just change the meaning of a single
> checkbox rather than offering two checkboxes in the UI?  (I think I understand
> why we need 2 booleans in the DB).

We should keep the existing checkbox because it is useful for other usecases,
like one-click revert that does a commit through the CQ using 'TBR=' without
caring about the reviewer list.


> 
>
https://codereview.appspot.com/66340044/diff/120001/codereview/views_chromium.py
> File codereview/views_chromium.py (right):
> 
>
https://codereview.appspot.com/66340044/diff/120001/codereview/views_chromium...
> codereview/views_chromium.py:413: request.issue.lgtm_auto_commit = (False if
not
> request.issue.commit
> This seems confusing. I think it would be more clear with the if-statement
> controlling the assignment:
> 
> if not request.issue.commit:
>   request.issue.lgtm_auto_commit = False
> 
> Also, I was concerned that every time the user submits the form that this
logic
> might undo their setting for the other checkbox, which would be mysterious and
> frustrating.  Luckily I remembered from the template that we actually have two
> distinct forms each of which has one checkbox and not the other.    I think
you
> could just document that here.  
> 
> Also, it's a little counter-intuitive that only the issue owner can check the
> auto-commit checkbox, but anyone can effectively uncheck it by toggling the
> commit checkbox twice.  I see why that might be needed, but a comment would
help
> future maintainers understand why it needs to be that way.  

I completely agree. Here is another idea:
Make it so that the new checkbox can be clicked by anybody, this is the behavior
right now of the 'Commit' checkbox as well, so we will just be preserving
existing behavior.


> 
> The UI might be confusing too, because if the auto-commit was checked and the
> commit was checked, and then I uncheck the commit box: I will be looking at a
> page that still has the auto-commit box checked, even though that bit has been
> cleared in the database.  If you use JS to uncheck the auto-commit box, you
will
> likely trigger another AJAX request, but I guess it would be a no-op.  Would
it
> be better to just offer the auto-commit checkbox alone?  Will anyone still
need
> the regular commit checkbox?

This is true. I originally left the auto-commit checkbox alone, but then what
happened is if you uncheck the commit checkbox it posts a msg in the Issue
saying 'unchecked by xyz' this then triggers the check of reviewer lgtms and
then it checked the commit checkbox again. Thus, I put in logic to uncheck the
checkbox but this causes the problem of the page showing stale data.

[jrobbins (corp), 2014-02-21 22:24:30]

OK, so it sounds like we are thinking:
1. We go with two checkboxes
2. Anyone can check/uncheck either one
3. We need to prevent the "commit now" box from getting checked again when
you are trying to uncheck it.
4. (separately, we need to make the l-g-t-m detection have fewer false
positives)

Regarding 3, what about using JS on the "commit now" box to uncheck the
"auto commit" box?  That way it will uncheck both in the UI and the info
displayed will match what is in the DB.  Just watch out for double POSTs if
they might cause race conditions.

BTW, good thing that you did not take my all(approval_dict.values())
suggestion literally, that would be True when there are zero reviewers.

Thanks,
jason!



On Fri, Feb 21, 2014 at 1:26 PM, <rmistry@google.com> wrote:

> I did not make any code changes, I think we should first flesh out the
> design.
> BTW thanks for thinking through this, I was not 100% sure of the flow
> and I am glad we are talking about it.
>
>
> On 2014/02/21 20:35:01, jrobbins1 wrote:
>
>> not lgtm.  I'm just saying that because the message text itself
>>
> contains the
>
>> string l-g-t-m.
>>
>
>  I think this is close to being ready. But now that I see the UI change
>>
> and can
>
>> imagine using it, I wonder if we should just change the meaning of a
>>
> single
>
>> checkbox rather than offering two checkboxes in the UI?  (I think I
>>
> understand
>
>> why we need 2 booleans in the DB).
>>
>
> We should keep the existing checkbox because it is useful for other
> usecases, like one-click revert that does a commit through the CQ using
> 'TBR=' without caring about the reviewer list.
>
>
>
>
>
> https://codereview.appspot.com/66340044/diff/120001/
> codereview/views_chromium.py
>
>> File codereview/views_chromium.py (right):
>>
>
>
> https://codereview.appspot.com/66340044/diff/120001/
> codereview/views_chromium.py#newcode413
>
>> codereview/views_chromium.py:413: request.issue.lgtm_auto_commit =
>>
> (False if not
>
>> request.issue.commit
>> This seems confusing. I think it would be more clear with the
>>
> if-statement
>
>> controlling the assignment:
>>
>
>  if not request.issue.commit:
>>    request.issue.lgtm_auto_commit = False
>>
>
>  Also, I was concerned that every time the user submits the form that
>>
> this logic
>
>> might undo their setting for the other checkbox, which would be
>>
> mysterious and
>
>> frustrating.  Luckily I remembered from the template that we actually
>>
> have two
>
>> distinct forms each of which has one checkbox and not the other.    I
>>
> think you
>
>> could just document that here.
>>
>
>  Also, it's a little counter-intuitive that only the issue owner can
>>
> check the
>
>> auto-commit checkbox, but anyone can effectively uncheck it by
>>
> toggling the
>
>> commit checkbox twice.  I see why that might be needed, but a comment
>>
> would help
>
>> future maintainers understand why it needs to be that way.
>>
>
> I completely agree. Here is another idea:
> Make it so that the new checkbox can be clicked by anybody, this is the
> behavior right now of the 'Commit' checkbox as well, so we will just be
> preserving existing behavior.
>
>
>
>
>  The UI might be confusing too, because if the auto-commit was checked
>>
> and the
>
>> commit was checked, and then I uncheck the commit box: I will be
>>
> looking at a
>
>> page that still has the auto-commit box checked, even though that bit
>>
> has been
>
>> cleared in the database.  If you use JS to uncheck the auto-commit
>>
> box, you will
>
>> likely trigger another AJAX request, but I guess it would be a no-op.
>>
> Would it
>
>> be better to just offer the auto-commit checkbox alone?  Will anyone
>>
> still need
>
>> the regular commit checkbox?
>>
>
> This is true. I originally left the auto-commit checkbox alone, but then
> what happened is if you uncheck the commit checkbox it posts a msg in
> the Issue saying 'unchecked by xyz' this then triggers the check of
> reviewer lgtms and then it checked the commit checkbox again. Thus, I
> put in logic to uncheck the checkbox but this causes the problem of the
> page showing stale data.
>
>
> https://codereview.appspot.com/66340044/
>

[jrobbins (corp), 2014-02-21 22:37:44]

Oh, and regarding the use case where the reviewer writes "l-g-t-m with the
following nits".  One way to deal with that might be to show the checkboxes
on the publish form.  Also, "Quick L-G-T-M and CQ" button could be removed.
   If the reviewer has nits, s/he could see and uncheck the auto-commit
checkbox.  If the reviewer is excited about the change and "Commit now" is
not checked, s/he would check it and press "quick l-g-t-m" (two clicks
instead of the current one click button).

Thanks,
jason!


On Fri, Feb 21, 2014 at 2:24 PM, Jason Robbins <jrobbins@google.com> wrote:

> OK, so it sounds like we are thinking:
> 1. We go with two checkboxes
> 2. Anyone can check/uncheck either one
> 3. We need to prevent the "commit now" box from getting checked again when
> you are trying to uncheck it.
> 4. (separately, we need to make the l-g-t-m detection have fewer false
> positives)
>
> Regarding 3, what about using JS on the "commit now" box to uncheck the
> "auto commit" box?  That way it will uncheck both in the UI and the info
> displayed will match what is in the DB.  Just watch out for double POSTs if
> they might cause race conditions.
>
> BTW, good thing that you did not take my all(approval_dict.values())
> suggestion literally, that would be True when there are zero reviewers.
>
> Thanks,
> jason!
>
>
>
> On Fri, Feb 21, 2014 at 1:26 PM, <rmistry@google.com> wrote:
>
>> I did not make any code changes, I think we should first flesh out the
>> design.
>> BTW thanks for thinking through this, I was not 100% sure of the flow
>> and I am glad we are talking about it.
>>
>>
>> On 2014/02/21 20:35:01, jrobbins1 wrote:
>>
>>> not lgtm.  I'm just saying that because the message text itself
>>>
>> contains the
>>
>>> string l-g-t-m.
>>>
>>
>>  I think this is close to being ready. But now that I see the UI change
>>>
>> and can
>>
>>> imagine using it, I wonder if we should just change the meaning of a
>>>
>> single
>>
>>> checkbox rather than offering two checkboxes in the UI?  (I think I
>>>
>> understand
>>
>>> why we need 2 booleans in the DB).
>>>
>>
>> We should keep the existing checkbox because it is useful for other
>> usecases, like one-click revert that does a commit through the CQ using
>> 'TBR=' without caring about the reviewer list.
>>
>>
>>
>>
>>
>> https://codereview.appspot.com/66340044/diff/120001/
>> codereview/views_chromium.py
>>
>>> File codereview/views_chromium.py (right):
>>>
>>
>>
>> https://codereview.appspot.com/66340044/diff/120001/
>> codereview/views_chromium.py#newcode413
>>
>>> codereview/views_chromium.py:413: request.issue.lgtm_auto_commit =
>>>
>> (False if not
>>
>>> request.issue.commit
>>> This seems confusing. I think it would be more clear with the
>>>
>> if-statement
>>
>>> controlling the assignment:
>>>
>>
>>  if not request.issue.commit:
>>>    request.issue.lgtm_auto_commit = False
>>>
>>
>>  Also, I was concerned that every time the user submits the form that
>>>
>> this logic
>>
>>> might undo their setting for the other checkbox, which would be
>>>
>> mysterious and
>>
>>> frustrating.  Luckily I remembered from the template that we actually
>>>
>> have two
>>
>>> distinct forms each of which has one checkbox and not the other.    I
>>>
>> think you
>>
>>> could just document that here.
>>>
>>
>>  Also, it's a little counter-intuitive that only the issue owner can
>>>
>> check the
>>
>>> auto-commit checkbox, but anyone can effectively uncheck it by
>>>
>> toggling the
>>
>>> commit checkbox twice.  I see why that might be needed, but a comment
>>>
>> would help
>>
>>> future maintainers understand why it needs to be that way.
>>>
>>
>> I completely agree. Here is another idea:
>> Make it so that the new checkbox can be clicked by anybody, this is the
>> behavior right now of the 'Commit' checkbox as well, so we will just be
>> preserving existing behavior.
>>
>>
>>
>>
>>  The UI might be confusing too, because if the auto-commit was checked
>>>
>> and the
>>
>>> commit was checked, and then I uncheck the commit box: I will be
>>>
>> looking at a
>>
>>> page that still has the auto-commit box checked, even though that bit
>>>
>> has been
>>
>>> cleared in the database.  If you use JS to uncheck the auto-commit
>>>
>> box, you will
>>
>>> likely trigger another AJAX request, but I guess it would be a no-op.
>>>
>> Would it
>>
>>> be better to just offer the auto-commit checkbox alone?  Will anyone
>>>
>> still need
>>
>>> the regular commit checkbox?
>>>
>>
>> This is true. I originally left the auto-commit checkbox alone, but then
>> what happened is if you uncheck the commit checkbox it posts a msg in
>> the Issue saying 'unchecked by xyz' this then triggers the check of
>> reviewer lgtms and then it checked the commit checkbox again. Thus, I
>> put in logic to uncheck the checkbox but this causes the problem of the
>> page showing stale data.
>>
>>
>> https://codereview.appspot.com/66340044/
>>
>
>

[rmistry, 2014-02-27 14:41:43]

Alright, ready for review @PatchSet10.

On 2014/02/21 22:24:30, jrobbins (corp) wrote:
> OK, so it sounds like we are thinking:
> 1. We go with two checkboxes

Done.

> 2. Anyone can check/uncheck either one

Done.

> 3. We need to prevent the "commit now" box from getting checked again when
> you are trying to uncheck it.

Done. Used javascript to uncheck the auto-commit checkbox and unsetting it in
views_chromium.py as well.

> 4. (separately, we need to make the l-g-t-m detection have fewer false
> positives)
> 
> Regarding 3, what about using JS on the "commit now" box to uncheck the
> "auto commit" box?  That way it will uncheck both in the UI and the info
> displayed will match what is in the DB.  Just watch out for double POSTs if
> they might cause race conditions.
> 
> BTW, good thing that you did not take my all(approval_dict.values())
> suggestion literally, that would be True when there are zero reviewers.
> 
> Thanks,
> jason!
> 
> 
> 
> On Fri, Feb 21, 2014 at 1:26 PM, <mailto:rmistry@google.com> wrote:
> 
> > I did not make any code changes, I think we should first flesh out the
> > design.
> > BTW thanks for thinking through this, I was not 100% sure of the flow
> > and I am glad we are talking about it.
> >
> >
> > On 2014/02/21 20:35:01, jrobbins1 wrote:
> >
> >> not lgtm.  I'm just saying that because the message text itself
> >>
> > contains the
> >
> >> string l-g-t-m.
> >>
> >
> >  I think this is close to being ready. But now that I see the UI change
> >>
> > and can
> >
> >> imagine using it, I wonder if we should just change the meaning of a
> >>
> > single
> >
> >> checkbox rather than offering two checkboxes in the UI?  (I think I
> >>
> > understand
> >
> >> why we need 2 booleans in the DB).
> >>
> >
> > We should keep the existing checkbox because it is useful for other
> > usecases, like one-click revert that does a commit through the CQ using
> > 'TBR=' without caring about the reviewer list.
> >
> >
> >
> >
> >
> > https://codereview.appspot.com/66340044/diff/120001/
> > codereview/views_chromium.py
> >
> >> File codereview/views_chromium.py (right):
> >>
> >
> >
> > https://codereview.appspot.com/66340044/diff/120001/
> > codereview/views_chromium.py#newcode413
> >
> >> codereview/views_chromium.py:413: request.issue.lgtm_auto_commit =
> >>
> > (False if not
> >
> >> request.issue.commit
> >> This seems confusing. I think it would be more clear with the
> >>
> > if-statement
> >
> >> controlling the assignment:
> >>
> >
> >  if not request.issue.commit:
> >>    request.issue.lgtm_auto_commit = False
> >>
> >
> >  Also, I was concerned that every time the user submits the form that
> >>
> > this logic
> >
> >> might undo their setting for the other checkbox, which would be
> >>
> > mysterious and
> >
> >> frustrating.  Luckily I remembered from the template that we actually
> >>
> > have two
> >
> >> distinct forms each of which has one checkbox and not the other.    I
> >>
> > think you
> >
> >> could just document that here.
> >>
> >
> >  Also, it's a little counter-intuitive that only the issue owner can
> >>
> > check the
> >
> >> auto-commit checkbox, but anyone can effectively uncheck it by
> >>
> > toggling the
> >
> >> commit checkbox twice.  I see why that might be needed, but a comment
> >>
> > would help
> >
> >> future maintainers understand why it needs to be that way.
> >>
> >
> > I completely agree. Here is another idea:
> > Make it so that the new checkbox can be clicked by anybody, this is the
> > behavior right now of the 'Commit' checkbox as well, so we will just be
> > preserving existing behavior.
> >
> >
> >
> >
> >  The UI might be confusing too, because if the auto-commit was checked
> >>
> > and the
> >
> >> commit was checked, and then I uncheck the commit box: I will be
> >>
> > looking at a
> >
> >> page that still has the auto-commit box checked, even though that bit
> >>
> > has been
> >
> >> cleared in the database.  If you use JS to uncheck the auto-commit
> >>
> > box, you will
> >
> >> likely trigger another AJAX request, but I guess it would be a no-op.
> >>
> > Would it
> >
> >> be better to just offer the auto-commit checkbox alone?  Will anyone
> >>
> > still need
> >
> >> the regular commit checkbox?
> >>
> >
> > This is true. I originally left the auto-commit checkbox alone, but then
> > what happened is if you uncheck the commit checkbox it posts a msg in
> > the Issue saying 'unchecked by xyz' this then triggers the check of
> > reviewer lgtms and then it checked the commit checkbox again. Thus, I
> > put in logic to uncheck the checkbox but this causes the problem of the
> > page showing stale data.
> >
> >
> > https://codereview.appspot.com/66340044/
> >

[rmistry, 2014-02-27 14:43:29]

On 2014/02/21 22:37:44, jrobbins (corp) wrote:
> Oh, and regarding the use case where the reviewer writes "l-g-t-m with the
> following nits".  One way to deal with that might be to show the checkboxes
> on the publish form.  Also, "Quick L-G-T-M and CQ" button could be removed.
>    If the reviewer has nits, s/he could see and uncheck the auto-commit
> checkbox.  If the reviewer is excited about the change and "Commit now" is
> not checked, s/he would check it and press "quick l-g-t-m" (two clicks
> instead of the current one click button).
> 
> Thanks,
> jason!


I added a msg on the publish page that says (if cq bit is not set and auto cq
bit is set):

Note: The 'Commit after a reviewer gives LGTM' bit is turned on for this CL.
Saying 'LGTM with nits' will send it to the CQ.

Try it out here:
https://skia-codereview-staging2.appspot.com/420005/




> 
> 
> On Fri, Feb 21, 2014 at 2:24 PM, Jason Robbins <mailto:jrobbins@google.com>
wrote:
> 
> > OK, so it sounds like we are thinking:
> > 1. We go with two checkboxes
> > 2. Anyone can check/uncheck either one
> > 3. We need to prevent the "commit now" box from getting checked again when
> > you are trying to uncheck it.
> > 4. (separately, we need to make the l-g-t-m detection have fewer false
> > positives)
> >
> > Regarding 3, what about using JS on the "commit now" box to uncheck the
> > "auto commit" box?  That way it will uncheck both in the UI and the info
> > displayed will match what is in the DB.  Just watch out for double POSTs if
> > they might cause race conditions.
> >
> > BTW, good thing that you did not take my all(approval_dict.values())
> > suggestion literally, that would be True when there are zero reviewers.
> >
> > Thanks,
> > jason!
> >
> >
> >
> > On Fri, Feb 21, 2014 at 1:26 PM, <mailto:rmistry@google.com> wrote:
> >
> >> I did not make any code changes, I think we should first flesh out the
> >> design.
> >> BTW thanks for thinking through this, I was not 100% sure of the flow
> >> and I am glad we are talking about it.
> >>
> >>
> >> On 2014/02/21 20:35:01, jrobbins1 wrote:
> >>
> >>> not lgtm.  I'm just saying that because the message text itself
> >>>
> >> contains the
> >>
> >>> string l-g-t-m.
> >>>
> >>
> >>  I think this is close to being ready. But now that I see the UI change
> >>>
> >> and can
> >>
> >>> imagine using it, I wonder if we should just change the meaning of a
> >>>
> >> single
> >>
> >>> checkbox rather than offering two checkboxes in the UI?  (I think I
> >>>
> >> understand
> >>
> >>> why we need 2 booleans in the DB).
> >>>
> >>
> >> We should keep the existing checkbox because it is useful for other
> >> usecases, like one-click revert that does a commit through the CQ using
> >> 'TBR=' without caring about the reviewer list.
> >>
> >>
> >>
> >>
> >>
> >> https://codereview.appspot.com/66340044/diff/120001/
> >> codereview/views_chromium.py
> >>
> >>> File codereview/views_chromium.py (right):
> >>>
> >>
> >>
> >> https://codereview.appspot.com/66340044/diff/120001/
> >> codereview/views_chromium.py#newcode413
> >>
> >>> codereview/views_chromium.py:413: request.issue.lgtm_auto_commit =
> >>>
> >> (False if not
> >>
> >>> request.issue.commit
> >>> This seems confusing. I think it would be more clear with the
> >>>
> >> if-statement
> >>
> >>> controlling the assignment:
> >>>
> >>
> >>  if not request.issue.commit:
> >>>    request.issue.lgtm_auto_commit = False
> >>>
> >>
> >>  Also, I was concerned that every time the user submits the form that
> >>>
> >> this logic
> >>
> >>> might undo their setting for the other checkbox, which would be
> >>>
> >> mysterious and
> >>
> >>> frustrating.  Luckily I remembered from the template that we actually
> >>>
> >> have two
> >>
> >>> distinct forms each of which has one checkbox and not the other.    I
> >>>
> >> think you
> >>
> >>> could just document that here.
> >>>
> >>
> >>  Also, it's a little counter-intuitive that only the issue owner can
> >>>
> >> check the
> >>
> >>> auto-commit checkbox, but anyone can effectively uncheck it by
> >>>
> >> toggling the
> >>
> >>> commit checkbox twice.  I see why that might be needed, but a comment
> >>>
> >> would help
> >>
> >>> future maintainers understand why it needs to be that way.
> >>>
> >>
> >> I completely agree. Here is another idea:
> >> Make it so that the new checkbox can be clicked by anybody, this is the
> >> behavior right now of the 'Commit' checkbox as well, so we will just be
> >> preserving existing behavior.
> >>
> >>
> >>
> >>
> >>  The UI might be confusing too, because if the auto-commit was checked
> >>>
> >> and the
> >>
> >>> commit was checked, and then I uncheck the commit box: I will be
> >>>
> >> looking at a
> >>
> >>> page that still has the auto-commit box checked, even though that bit
> >>>
> >> has been
> >>
> >>> cleared in the database.  If you use JS to uncheck the auto-commit
> >>>
> >> box, you will
> >>
> >>> likely trigger another AJAX request, but I guess it would be a no-op.
> >>>
> >> Would it
> >>
> >>> be better to just offer the auto-commit checkbox alone?  Will anyone
> >>>
> >> still need
> >>
> >>> the regular commit checkbox?
> >>>
> >>
> >> This is true. I originally left the auto-commit checkbox alone, but then
> >> what happened is if you uncheck the commit checkbox it posts a msg in
> >> the Issue saying 'unchecked by xyz' this then triggers the check of
> >> reviewer lgtms and then it checked the commit checkbox again. Thus, I
> >> put in logic to uncheck the checkbox but this causes the problem of the
> >> page showing stale data.
> >>
> >>
> >> https://codereview.appspot.com/66340044/
> >>
> >
> >

[rmistry, 2014-02-27 14:54:52]

I made one change to the agreed upon design after talking to a few users to get
their opinion.

The new checkbox no longer checks for approvals from ALL reviewers and instead
just goes with the last approval (or disapproval).
The reason for this change is the way CQ works right now:  any LGTM (from a
committer) is respected, the concept of checking for LGTMs from every reviewer
is something new and is not intuitive for current users.
I propose that the new checkbox respects the last approval or the last
disapproval (similar to the CQ), we can add the OR / AND reviewers logic once
that feature is added to Rietveld.

[reed1, 2014-02-27 16:00:27]

On 2014/02/27 14:54:52, rmistry wrote:
> I made one change to the agreed upon design after talking to a few users to
get
> their opinion.
> 
> The new checkbox no longer checks for approvals from ALL reviewers and instead
> just goes with the last approval (or disapproval).
> The reason for this change is the way CQ works right now:  any LGTM (from a
> committer) is respected, the concept of checking for LGTMs from every reviewer
> is something new and is not intuitive for current users.
> I propose that the new checkbox respects the last approval or the last
> disapproval (similar to the CQ), we can add the OR / AND reviewers logic once
> that feature is added to Rietveld.

+1 thanks for that change, I think this is better.

[jrobbins (corp), 2014-02-27 19:01:24]

lgtm.  I am still a little concerned about the possibility of a CL going to the
CQ before it has had a full review, but I guess the way that the options is
worded now informs people of that risk.  I was also concerned about random users
being able to set this bit, but then again, they can already set the commit bit,
so this is not adding a new risk.

[reed1, 2014-02-27 19:03:33]

On 2014/02/27 19:01:24, jrobbins (corp) wrote:
> lgtm.  I am still a little concerned about the possibility of a CL going to
the
> CQ before it has had a full review, but I guess the way that the options is
> worded now informs people of that risk.  I was also concerned about random
users
> being able to set this bit, but then again, they can already set the commit
bit,
> so this is not adding a new risk.

Agreed.

I am in favor of the (other) idea of formalizing the AND or OR ness of multiple
reviewers, but I think that can be treated orthogonally to this feature.

[rmistry, 2014-02-27 19:29:44]

On 2014/02/27 19:03:33, reed1 wrote:
> On 2014/02/27 19:01:24, jrobbins (corp) wrote:
> > lgtm.  I am still a little concerned about the possibility of a CL going to
> the
> > CQ before it has had a full review, but I guess the way that the options is
> > worded now informs people of that risk.  I was also concerned about random
> users
> > being able to set this bit, but then again, they can already set the commit
> bit,
> > so this is not adding a new risk.
> 
> Agreed.
> 
> I am in favor of the (other) idea of formalizing the AND or OR ness of
multiple
> reviewers, but I think that can be treated orthogonally to this feature.

Thanks for the review Jason!
Robbie, could you also please take a quick look, I will submit after your LGTM.

[rmistry, 2014-03-05 21:53:39]

On 2014/02/27 19:29:44, rmistry wrote:
> On 2014/02/27 19:03:33, reed1 wrote:
> > On 2014/02/27 19:01:24, jrobbins (corp) wrote:
> > > lgtm.  I am still a little concerned about the possibility of a CL going
to
> > the
> > > CQ before it has had a full review, but I guess the way that the options
is
> > > worded now informs people of that risk.  I was also concerned about random
> > users
> > > being able to set this bit, but then again, they can already set the
commit
> > bit,
> > > so this is not adding a new risk.
> > 
> > Agreed.
> > 
> > I am in favor of the (other) idea of formalizing the AND or OR ness of
> multiple
> > reviewers, but I think that can be treated orthogonally to this feature.
> 
> Thanks for the review Jason!
> Robbie, could you also please take a quick look, I will submit after your
LGTM.

Ping for Robbie.

[iannucci, 2014-03-06 01:07:19]

lgtm. +pawel for CQ interaction change FYI.

https://codereview.appspot.com/66340044/diff/180001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/66340044/diff/180001/codereview/models.py#newc...
codereview/models.py:325: self.commit = False
nice touch :)

https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium...
codereview/views_chromium.py:402: # Add as reviewer if setting, not clearing.
Hm. If someone unchecks it, then shouldn't they still be added? Otherwise I can
ghost-uncheck the box for people.

[rmistry, 2014-03-06 16:17:31]

https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium.py
File codereview/views_chromium.py (right):

https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium...
codereview/views_chromium.py:402: # Add as reviewer if setting, not clearing.
On 2014/03/06 01:07:20, iannucci wrote:
> Hm. If someone unchecks it, then shouldn't they still be added? Otherwise I
can
> ghost-uncheck the box for people.

This is the same behavior that the CQ checkbox currently does, it only adds
users as reviewers while setting and not clearing.
I do not know what the original rational was behind doing that, but I am
assuming there was a good one :)

And about ghost-unchecking, all actions taken on the checkbox are recorded in
the msgs section of the issue, so issue owners will know who changed the bit.

[rmistry, 2014-03-06 19:48:42]

On 2014/03/06 16:17:31, rmistry wrote:
>
https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium.py
> File codereview/views_chromium.py (right):
> 
>
https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium...
> codereview/views_chromium.py:402: # Add as reviewer if setting, not clearing.
> On 2014/03/06 01:07:20, iannucci wrote:
> > Hm. If someone unchecks it, then shouldn't they still be added? Otherwise I
> can
> > ghost-uncheck the box for people.
> 
> This is the same behavior that the CQ checkbox currently does, it only adds
> users as reviewers while setting and not clearing.
> I do not know what the original rational was behind doing that, but I am
> assuming there was a good one :)
> 
> And about ghost-unchecking, all actions taken on the checkbox are recorded in
> the msgs section of the issue, so issue owners will know who changed the bit.

This has been committed:
https://code.google.com/p/rietveld/source/detail?r=10dc0231d90c02727da9004894...

I deployed it to rietveld (without making it the default version to test it):
https://1237-10dc0231d90c.chromiumcodereview-hr.appspot.com/

I see a bug, when apply patch fails and the CQ unchecks the 'Commit' checkbox it
does not seem to uncheck the new checkbox as well. It works when we do it by
hand but for some reason does not work when the CQ does it.
Starting to investigate, will create a new CL when I figure out how to fix it.

[rmistry, 2014-03-07 13:55:05]

On 2014/03/06 19:48:42, rmistry wrote:
> On 2014/03/06 16:17:31, rmistry wrote:
> >
>
https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium.py
> > File codereview/views_chromium.py (right):
> > 
> >
>
https://codereview.appspot.com/66340044/diff/180001/codereview/views_chromium...
> > codereview/views_chromium.py:402: # Add as reviewer if setting, not
clearing.
> > On 2014/03/06 01:07:20, iannucci wrote:
> > > Hm. If someone unchecks it, then shouldn't they still be added? Otherwise
I
> > can
> > > ghost-uncheck the box for people.
> > 
> > This is the same behavior that the CQ checkbox currently does, it only adds
> > users as reviewers while setting and not clearing.
> > I do not know what the original rational was behind doing that, but I am
> > assuming there was a good one :)
> > 
> > And about ghost-unchecking, all actions taken on the checkbox are recorded
in
> > the msgs section of the issue, so issue owners will know who changed the
bit.
> 
> This has been committed:
>
https://code.google.com/p/rietveld/source/detail?r=10dc0231d90c02727da9004894...
> 
> I deployed it to rietveld (without making it the default version to test it):
> https://1237-10dc0231d90c.chromiumcodereview-hr.appspot.com/
> 
> I see a bug, when apply patch fails and the CQ unchecks the 'Commit' checkbox
it
> does not seem to uncheck the new checkbox as well. It works when we do it by
> hand but for some reason does not work when the CQ does it.
> Starting to investigate, will create a new CL when I figure out how to fix it.

Jason, needed to land and deploy an emergency change into Rietveld yesterday, I
asked him to revert this change so that it does not go live before this is
completely ready.
I will reattach to this issue when I merge in his changes and do some further
testing.

[rmistry, 2014-03-07 17:54:01]

Reopened this CL.

It contains both:
https://code.google.com/p/rietveld/source/detail?r=10dc0231d90c02727da9004894...
and
https://codereview.appspot.com/72040046/

All code here has been LGTM'ed previously. I will land it once jrobbins@ gives
me the go-ahead.

[rmistry, 2014-03-07 18:03:11]

On 2014/03/07 17:54:01, rmistry wrote:
> Reopened this CL.
> 
> It contains both:
>
https://code.google.com/p/rietveld/source/detail?r=10dc0231d90c02727da9004894...
> and
> https://codereview.appspot.com/72040046/
> 
> All code here has been LGTM'ed previously. I will land it once jrobbins@ gives
> me the go-ahead.


Tested the following here https://skia-codereview-staging2.appspot.com/490001/ -
* Checked both checkboxes and then closed the issue. Both checkboxes are then
unchecked and disabled.
* Checked the 'Commit after a reviewer gives LGTM' checkbox and uploaded a new
patchset. The checkbox is unchecked for the new patchset.