|
|
|
Created:
14 years, 4 months ago by paul.rademacher Modified:
14 years, 2 months ago CC:
codereview-discuss_googlegroups.com Visibility:
Public. |
This change displays the "approved" status for an issue, by coloring
the issue green on the main Issues page.
Before this, approval (an "lgtm" in a reviewer message) was only
displayed on the message header inside each issue, but approval state
could not be seen from the main Issues page.
Demo at http://codereview-rademacher-demo.appspot.com/
Patch Set 1 #
Total comments: 1
Total messages: 9
|
paul.rademacher
|
14 years, 4 months ago (2011年09月10日 20:52:55 UTC) #1 | ||||||||||||||||||||||||||||
Some projects (Chromium?) require at least 3 independent LGTMs before issue can be deemed approved, and we need to expose this through API. With boolean field it is impossible.
I think this change is ok. We don't have a feature request for more advanced approvals yet. And we already highlight a single LGTM message on the issue page, so showing it on the issue list too looks consistent to me. BTW, it seems that for the Chromium project a single LGTM is ok too: http://codereview.chromium.org/7840021/ http://codereview.appspot.com/4996047/diff/1/codereview/views.py File codereview/views.py (right): http://codereview.appspot.com/4996047/diff/1/codereview/views.py#newcode3668 codereview/views.py:3668: if msg.approval: The fact that we have to do this in two places doesn't look right. I'm hesitating a bit if this is could be a problem or not...
On 2011年09月11日 07:36:27, Andi Albrecht wrote: > I think this change is ok. We don't have a feature request for more advanced > approvals yet. I have. =) I need to get a list of people who approved commit to match them against the list of project members for automated commit management. I'd like to see the field approvers as a list of emails in API which is filled when a new message arrives with LGTM inside.
On Sunday, September 11, 2011, <techtonik@gmail.com> wrote: > On 2011年09月11日 07:36:27, Andi Albrecht wrote: >> >> I think this change is ok. We don't have a feature request for more > > advanced >> >> approvals yet. > > I have. =) I need to get a list of people who approved commit to match > them against the list of project members for automated commit > management. I'd like to see the field approvers as a list of emails in > API which is filled when a new message arrives with LGTM inside. Ok. Do you have a suggestion how to set Issue.approved according to project specific rules? OTOH, do you have a patch for adding "projects"? That'd be a starter for some other feature requests? :) -Andi > > http://codereview.appspot.com/4996047/ >
On 2011年09月11日 09:49:30, Andi Albrecht wrote: > Ok. Do you have a suggestion how to set Issue.approved according to project > specific rules? No. For now it is fine if we highlight issues with at least one approval. I want to avoid model migration in future, so I'd like to change the logic to add approver's email to the list of approvers when message is saved. > OTOH, do you have a patch for adding "projects"? That'd be a starter for > some other feature requests? :) For a starter I had a patch for adding unique ids to Repositories, but it was lost and this killed my further enthusiasm. =)
Thanks for the discussion, guys. I'll send a revised patchset with: - issue.approvals: list of emails instead of a single "approved" boolean - Exposure of approvals email list in API - Possibly displaying the approval emails on the issues page (though that'd be an extra column and might crowd the UI). Just a thought. - A response to Andi's comment on having to update issue in two places. Cheers, Paul On Sun, Sep 11, 2011 at 6:06 AM, <techtonik@gmail.com> wrote: > On 2011年09月11日 09:49:30, Andi Albrecht wrote: > >> Ok. Do you have a suggestion how to set Issue.approved according to >> > project > >> specific rules? >> > > No. For now it is fine if we highlight issues with at least one > approval. I want to avoid model migration in future, so I'd like to > change the logic to add approver's email to the list of approvers when > message is saved. > > > OTOH, do you have a patch for adding "projects"? That'd be a starter >> > for > >> some other feature requests? :) >> > > For a starter I had a patch for adding unique ids to Repositories, but > it was lost and this killed my further enthusiasm. =) > > > > http://codereview.appspot.com/**4996047/<http://codereview.appspot.com/4996047/> >
On 2011年09月12日 08:18:46, paul.rademacher wrote: > Thanks for the discussion, guys. I'll send a revised patchset with: > > - issue.approvals: list of emails instead of a single "approved" boolean > - Exposure of approvals email list in API > - Possibly displaying the approval emails on the issues page (though > that'd be an extra column and might crowd the UI). Just a thought. > - A response to Andi's comment on having to update issue in two places. It is easier to review several logically complete issues. For example, API can be added later, and list of approval emails is a feature-creep.
On 2011年09月12日 08:18:46, paul.rademacher wrote: > Thanks for the discussion, guys. I'll send a revised patchset with: > > - issue.approvals: list of emails instead of a single "approved" boolean Hi, Paul. How is the progress? Do you experience any troubles with that part? > - Exposure of approvals email list in API > - Possibly displaying the approval emails on the issues page (though > that'd be an extra column and might crowd the UI). Just a thought. This stuff can be added later. > - A response to Andi's comment on having to update issue in two places.