Add support to rietveld for filtering emails sent.

Add support to rietveld for filtering emails sent.

This allows messages which are publish/'d to be flagged as automated and/or advisory. Users may then select in Settings to exclude receiving these messages, varied by their involvement with the issue. 

Additionally, I've added a toggle which will cause rietveld to not send emails to you if you're the one who generated them. 

Features:
  * Uses the google apps email normalization steps to apply rules to email addresses in the CC field which may be isometric to an actual user account.
  * Human generated content always goes through.
  * Message UI is augmented slightly to show which messages are automated and/or advisory in nature (addition of two small icons in the Message header area).
  * Defaults to the current email workflow (i.e. No filters).

[iannucci, 2013-02-26 05:20:56]

PTAL.

Outstanding questions:
  * This changes the 'recipients' Message field to accurately reflect who got
the message. I can't tell if this is correct or not.
  * I know that the 'never_self_mail' setting was optional as part of this
feature, however:
    * It was (extremely) trivial to add
    * It will improve my own quality of life, and I imagine it would do so for
others :)
    * That said, if it's highly detested, I can plan to roll it out as a
separate patch...

Please note that this change will not currently flag any messages.  Changes to
gcl/git-cl/cq are required to enable this (I already have these changes in my
working copy).

[M-A, 2013-02-28 17:50:36]

Is this planned specifically for the chromium branch or also for the default
branch?

I think the default value for "automated" should be True and then have the
/publish form set automated=False so changing the upload.py tool is not
required.

https://codereview.appspot.com/7388060/diff/7001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7388060/diff/7001/codereview/models.py#newcode357
codereview/models.py:357: flags = db.StringListProperty()
I'm not super comfortable with free form strings when it's really just 2
booleans. I don't think this list will increase indefinitely.

https://codereview.appspot.com/7388060/diff/7001/codereview/models.py#newcode...
codereview/models.py:1059: return filter_flags > (filter_flags & flags)
This would be simpler?
return bool(filter_flags - flags)

https://codereview.appspot.com/7388060/diff/7001/templates/issue.html
File templates/issue.html (right):

https://codereview.appspot.com/7388060/diff/7001/templates/issue.html#newcode121
templates/issue.html:121: <div class="message{%if message.issue_was_closed%}
issue_was_closed{%endif%}{%if message.approval%} approval{%endif%}{%if
message.disapproval%} disapproval{%endif%}{%for flag in message.flags%}
flag_{{flag}}{%endfor%}"
Did you use an automatic formatting tool or you did it manually? Because that
changes the generated html.

[iannucci, 2013-03-01 00:05:01]

I think that messages generated from upload.py (i.e. the blank message from
--send-mail) actually counts as non-automated, since the only way it occurs is
if a human asks upload to send it, and the email content includes the
description+patch, which are human-generated content.

https://codereview.appspot.com/7388060/diff/7001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7388060/diff/7001/codereview/models.py#newcode357
codereview/models.py:357: flags = db.StringListProperty()
On 2013/02/28 17:50:36, M-A wrote:
> I'm not super comfortable with free form strings when it's really just 2
> booleans. I don't think this list will increase indefinitely.

Yeah I went back and forth on this. The amount of code to support the flags (and
any new flags) goes up if it's bools, but the string list is only enforced at
the submit point (publish).

I can make it bools.

https://codereview.appspot.com/7388060/diff/7001/codereview/models.py#newcode...
codereview/models.py:1059: return filter_flags > (filter_flags & flags)
On 2013/02/28 17:50:36, M-A wrote:
> This would be simpler?
> return bool(filter_flags - flags)

Yeah, I think I was tired... Will fix.

https://codereview.appspot.com/7388060/diff/7001/templates/issue.html
File templates/issue.html (right):

https://codereview.appspot.com/7388060/diff/7001/templates/issue.html#newcode121
templates/issue.html:121: <div class="message{%if message.issue_was_closed%}
issue_was_closed{%endif%}{%if message.approval%} approval{%endif%}{%if
message.disapproval%} disapproval{%endif%}{%for flag in message.flags%}
flag_{{flag}}{%endfor%}"
On 2013/02/28 17:50:36, M-A wrote:
> Did you use an automatic formatting tool or you did it manually? Because that
> changes the generated html.

I did this on purpose. It cleans up some weird extra spacing in the output html.

[iannucci, 2013-03-01 03:41:48]

PTA(nother)L.

I'm not sure if we want this in mainline reitveld? Do you think it'll be
desired?

I'm still inclined to default both automated and advisory to False (leaving
upload.py alone on the assumptions that those mails are always triggered by a
human explicitly), unless you have strong objections.

[M-A, 2013-03-07 19:22:58]

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcode37
codereview/models.py:37: FILTER_CHOICES = (
I think this should go in views.py since it's not used here, unlike
CONTEXT_CHOICES.

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
codereview/models.py:1054: """Returns a Boolean which indicates if a message
with the given flags
s/Boolean/bool/ since it's the name of the python type.

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
codereview/models.py:1057: Specifically, they don't want the mail if all the
flags set on this user
This sentence is hard to parse but I don't see a better way to state it.

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
codereview/models.py:1061: # Default to ret to True in case this user has NO
filter conditions.
Shorter and orthogonal, unlike your version:

filter_property = 'email_filter_%s_' % role
return (
  (not automated or not getattr(self, filter_property + 'automated')) and
  (not advisory or not getattr(self, filter_property + 'advisory')))

It's not exactly what you are doing but I think it's better.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode349
codereview/views.py:349: automated = forms.BooleanField(required=False,
widget=forms.HiddenInput())
defaut=True

then have all the templates have it set to False (0).

And fix upload.py too.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode373
codereview/views.py:373: automated = forms.BooleanField(required=False,
widget=forms.HiddenInput())
default=True

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3463: def _normalize_email(addr):
Since that's only true for gmail, make the function name imply it is
gmail-specific, because it is.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3470: local, domain = addr.split('@')
addr.split('@', 1)

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3471: local = local.lower()
local = local.lower().replace('.', '').split('+', 1)[0]

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3487: for norm_addr, addr in ((_normalize_email(a), a) for a
in emails):
I think it's easier to read with:

for addr in emails:
  norm_addr = _normalize_email(addr)
...

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3495: if not acnt.should_send_email(role, automated,
advisory):
should_send_email() should check never_self_mail too.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:4022: 'nickname': nickname,
While at it, sort the keys.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:4029: for role in ('owner', 'reviewer', 'cc'):
Can you make that a model property so you can then do:
initial.update(account.filters)
or something like that

[iannucci, 2013-03-08 00:17:10]

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcode37
codereview/models.py:37: FILTER_CHOICES = (
On 2013/03/07 19:22:58, M-A wrote:
> I think this should go in views.py since it's not used here, unlike
> CONTEXT_CHOICES.

Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
codereview/models.py:1054: """Returns a Boolean which indicates if a message
with the given flags
On 2013/03/07 19:22:58, M-A wrote:
> s/Boolean/bool/ since it's the name of the python type.

Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
codereview/models.py:1057: Specifically, they don't want the mail if all the
flags set on this user
On 2013/03/07 19:22:58, M-A wrote:
> This sentence is hard to parse but I don't see a better way to state it.

Yeah I know... I went through 3-4 more complex versions of this sentence
previously :/

https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
codereview/models.py:1061: # Default to ret to True in case this user has NO
filter conditions.
On 2013/03/07 19:22:58, M-A wrote:
> Shorter and orthogonal, unlike your version:
> 
> filter_property = 'email_filter_%s_' % role
> return (
>   (not automated or not getattr(self, filter_property + 'automated')) and
>   (not advisory or not getattr(self, filter_property + 'advisory')))
> 
> It's not exactly what you are doing but I think it's better.

Hm, ok. I like this. Done. :)

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode349
codereview/views.py:349: automated = forms.BooleanField(required=False,
widget=forms.HiddenInput())
On 2013/03/07 19:22:58, M-A wrote:
> defaut=True
> 
> then have all the templates have it set to False (0).
> 
> And fix upload.py too.

Ok. I'll make publish form set this False. Upload.py doesn't use publish, so it
uses the defaults for _make_message which set automated and advisory to False.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode373
codereview/views.py:373: automated = forms.BooleanField(required=False,
widget=forms.HiddenInput())
On 2013/03/07 19:22:58, M-A wrote:
> default=True

Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3463: def _normalize_email(addr):
On 2013/03/07 19:22:58, M-A wrote:
> Since that's only true for gmail, make the function name imply it is
> gmail-specific, because it is.

Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3470: local, domain = addr.split('@')
On 2013/03/07 19:22:58, M-A wrote:
> addr.split('@', 1)

Good catch, Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3471: local = local.lower()
On 2013/03/07 19:22:58, M-A wrote:
> local = local.lower().replace('.', '').split('+', 1)[0]

Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3487: for norm_addr, addr in ((_normalize_email(a), a) for a
in emails):
On 2013/03/07 19:22:58, M-A wrote:
> I think it's easier to read with:
> 
> for addr in emails:
>   norm_addr = _normalize_email(addr)
> ...

Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:3495: if not acnt.should_send_email(role, automated,
advisory):
On 2013/03/07 19:22:58, M-A wrote:
> should_send_email() should check never_self_mail too.

Done (and I DeMorgan'd the should_send_email logic :))

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:4022: 'nickname': nickname,
On 2013/03/07 19:22:58, M-A wrote:
> While at it, sort the keys.

Done.

https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
codereview/views.py:4029: for role in ('owner', 'reviewer', 'cc'):
On 2013/03/07 19:22:58, M-A wrote:
> Can you make that a model property so you can then do:
> initial.update(account.filters)
> or something like that

Done.

[iannucci, 2013-03-11 21:04:18]

On 2013/03/08 00:17:10, iannucci wrote:
> https://codereview.appspot.com/7388060/diff/17001/codereview/models.py
> File codereview/models.py (right):
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcode37
> codereview/models.py:37: FILTER_CHOICES = (
> On 2013/03/07 19:22:58, M-A wrote:
> > I think this should go in views.py since it's not used here, unlike
> > CONTEXT_CHOICES.
> 
> Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
> codereview/models.py:1054: """Returns a Boolean which indicates if a message
> with the given flags
> On 2013/03/07 19:22:58, M-A wrote:
> > s/Boolean/bool/ since it's the name of the python type.
> 
> Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
> codereview/models.py:1057: Specifically, they don't want the mail if all the
> flags set on this user
> On 2013/03/07 19:22:58, M-A wrote:
> > This sentence is hard to parse but I don't see a better way to state it.
> 
> Yeah I know... I went through 3-4 more complex versions of this sentence
> previously :/
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/models.py#newcod...
> codereview/models.py:1061: # Default to ret to True in case this user has NO
> filter conditions.
> On 2013/03/07 19:22:58, M-A wrote:
> > Shorter and orthogonal, unlike your version:
> > 
> > filter_property = 'email_filter_%s_' % role
> > return (
> >   (not automated or not getattr(self, filter_property + 'automated')) and
> >   (not advisory or not getattr(self, filter_property + 'advisory')))
> > 
> > It's not exactly what you are doing but I think it's better.
> 
> Hm, ok. I like this. Done. :)
> 
> https://codereview.appspot.com/7388060/diff/17001/codereview/views.py
> File codereview/views.py (right):
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode349
> codereview/views.py:349: automated = forms.BooleanField(required=False,
> widget=forms.HiddenInput())
> On 2013/03/07 19:22:58, M-A wrote:
> > defaut=True
> > 
> > then have all the templates have it set to False (0).
> > 
> > And fix upload.py too.
> 
> Ok. I'll make publish form set this False. Upload.py doesn't use publish, so
it
> uses the defaults for _make_message which set automated and advisory to False.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode373
> codereview/views.py:373: automated = forms.BooleanField(required=False,
> widget=forms.HiddenInput())
> On 2013/03/07 19:22:58, M-A wrote:
> > default=True
> 
> Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
> codereview/views.py:3463: def _normalize_email(addr):
> On 2013/03/07 19:22:58, M-A wrote:
> > Since that's only true for gmail, make the function name imply it is
> > gmail-specific, because it is.
> 
> Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
> codereview/views.py:3470: local, domain = addr.split('@')
> On 2013/03/07 19:22:58, M-A wrote:
> > addr.split('@', 1)
> 
> Good catch, Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
> codereview/views.py:3471: local = local.lower()
> On 2013/03/07 19:22:58, M-A wrote:
> > local = local.lower().replace('.', '').split('+', 1)[0]
> 
> Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
> codereview/views.py:3487: for norm_addr, addr in ((_normalize_email(a), a) for
a
> in emails):
> On 2013/03/07 19:22:58, M-A wrote:
> > I think it's easier to read with:
> > 
> > for addr in emails:
> >   norm_addr = _normalize_email(addr)
> > ...
> 
> Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
> codereview/views.py:3495: if not acnt.should_send_email(role, automated,
> advisory):
> On 2013/03/07 19:22:58, M-A wrote:
> > should_send_email() should check never_self_mail too.
> 
> Done (and I DeMorgan'd the should_send_email logic :))
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
> codereview/views.py:4022: 'nickname': nickname,
> On 2013/03/07 19:22:58, M-A wrote:
> > While at it, sort the keys.
> 
> Done.
> 
>
https://codereview.appspot.com/7388060/diff/17001/codereview/views.py#newcode...
> codereview/views.py:4029: for role in ('owner', 'reviewer', 'cc'):
> On 2013/03/07 19:22:58, M-A wrote:
> > Can you make that a model property so you can then do:
> > initial.update(account.filters)
> > or something like that
> 
> Done.

Ping

[M-A, 2013-03-11 23:23:56]

Make sure the code works before committing. Feel free to test  a live instance
on chromiumcodereview-hr.

https://codereview.appspot.com/7388060/diff/25001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7388060/diff/25001/codereview/models.py#newcod...
codereview/models.py:823: never_self_mail = db.BooleanProperty()
Do you expect someone to want that feature?

https://codereview.appspot.com/7388060/diff/25001/codereview/models.py#newcod...
codereview/models.py:845: ret['email_filter_'+role] = ",".join(items)
ret['email_filter_'+role] = ",".join(
    flag for flag in ('automated', 'advisory')
    if getattr(self, 'email_filter_%s_%s' % (role, flag)))

would save 2 line :)

https://codereview.appspot.com/7388060/diff/25001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/7388060/diff/25001/codereview/views.py#newcode375
codereview/views.py:375: defualt=True)
default
Need more testing :)

https://codereview.appspot.com/7388060/diff/25001/codereview/views.py#newcode...
codereview/views.py:4056: flags = [] if not flags_raw else flags_raw.split(',')
flags = (flags_raw or '').split(',')

[iannucci, 2013-03-11 23:34:11]

Of course it works :). I meant to stand up a fake-codereview instance over the
weekend, but I didn't get to. I'll fix your suggestions and stand one up so we
can play with it. It's all working on the dev_appserver though (complete with
fake-sending emails).

[iannucci, 2013-03-12 02:14:37]

https://codereview.appspot.com/7388060/diff/25001/codereview/models.py
File codereview/models.py (right):

https://codereview.appspot.com/7388060/diff/25001/codereview/models.py#newcod...
codereview/models.py:823: never_self_mail = db.BooleanProperty()
On 2013/03/11 23:23:56, M-A wrote:
> Do you expect someone to want that feature?

Yes... I know I want that feature :) I imagine it would be useful for others
(but I didn't take a poll).

https://codereview.appspot.com/7388060/diff/25001/codereview/models.py#newcod...
codereview/models.py:845: ret['email_filter_'+role] = ",".join(items)
On 2013/03/11 23:23:56, M-A wrote:
> ret['email_filter_'+role] = ",".join(
>     flag for flag in ('automated', 'advisory')
>     if getattr(self, 'email_filter_%s_%s' % (role, flag)))
> 
> would save 2 line :)

lol. done.

https://codereview.appspot.com/7388060/diff/25001/codereview/views.py
File codereview/views.py (right):

https://codereview.appspot.com/7388060/diff/25001/codereview/views.py#newcode375
codereview/views.py:375: defualt=True)
On 2013/03/11 23:23:56, M-A wrote:
> default
> Need more testing :)

Done.

https://codereview.appspot.com/7388060/diff/25001/codereview/views.py#newcode...
codereview/views.py:4056: flags = [] if not flags_raw else flags_raw.split(',')
On 2013/03/11 23:23:56, M-A wrote:
> flags = (flags_raw or '').split(',')

Done.

[iannucci, 2013-03-12 02:27:31]

Demo app is up at https://testfoo-codereview.appspot.com

[M-A, 2013-03-12 20:45:55]

lgtm but did you mean to only commit it to chromium branch? I don't see anything
chromium specific, so it'd be worth committing it to default. If so, add
codereview-discuss@googlegroups.com.

[iannucci, 2013-03-12 22:24:55]

On 2013/03/12 20:45:55, M-A wrote:
> lgtm but did you mean to only commit it to chromium branch? I don't see
anything
> chromium specific, so it'd be worth committing it to default. If so, add
> mailto:codereview-discuss@googlegroups.com.

+codereview-discuss@googlegroups.com .

I tried this patch against default and it looks pretty sane (only some minor
conflicts). I've uploaded the resolved diff as patchset #8.