Show approval status (green background for LGTMs) on Issues page

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/

[techtonik, 2011-09-10 21:57:19]

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.

[Andi Albrecht, 2011-09-11 07:36:27]

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...

[techtonik, 2011-09-11 08:14:04]

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.

[Andi Albrecht, 2011-09-11 09:49:30]

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/
>

[techtonik, 2011-09-11 13:06:16]

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. =)

[paul.rademacher, 2011-09-12 08:18:46]

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/>
>

[techtonik, 2011-09-15 09:35:01]

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.

[techtonik, 2011-10-25 20:59:39]

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.