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.