Implement mail dispatcher app.

Implement mail dispatcher app.

This adds the mail python app that handles incoming mail and dispatches it to
handlers, as well as a handler implementing automated commenting for changes to
chrome/app/policy/policy_templates.json.

R=agable@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219005

[Mattias Nissler (ping if slow), 2013-08-05 09:45:31]

And here's the app, finally :)

[Mattias Nissler (ping if slow), 2013-08-05 18:16:52]

cc agable

[Mattias Nissler (ping if slow), 2013-08-09 15:14:23]

OK guys, this is the main app and the last review in the series. Can you take a
look?

[iannucci, 2013-08-15 03:48:05]

Sorry for the lag on this.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
File reviewbot/handlers/policy_checklist.py (right):

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:13: CONTEXT_THRESHOLD = 12
Should we make a separate config file specifically for strings like these?

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:14: PROPERTY_NAME_RE =
re.compile('\'(\\w+)\'\\s*:')
may be worth using a raw string to reduce the backslash madness

r'\'(\w+)\'\s*:'

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:35: to re-implement your policy on top of
the new implementation.
I'm assuming these strings have all been vetted by the appropriate whomever?

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:61: so the policy shows up on the
chrome://policy page.
It may make sense to actually turn these strings into templates. I suspect that
will need to happen at some near point in the future, it may be easier to do it
now.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:89: except ValueError:
This seems sketchy. Why ValueError?

Also, I think this will throw a TypeError if a = [] or a = [1]

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:109: return None if nspaces == 0 else
nspaces
You may want to take a look at the textwrap module

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:112: class PolicyChangeParser(object):
Needs docs, code is hard to read without context/examples.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:232: return
So the idea here is that you could add reviewbot as a reviewer?

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:239: parser = PolicyChangeParser(
why not just make PolicyChangeParser a function returning a list of chunks?

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:259: line, side, message)
Maybe I'm unfamiliar with the types of changes here, but this could end up
posting a lot of (fairly large) comments. Maybe it should just post a max of one
of each message type?

https://codereview.chromium.org/20518002/diff/4001/reviewbot/mail_dispatcher.py
File reviewbot/mail_dispatcher.py (right):

https://codereview.chromium.org/20518002/diff/4001/reviewbot/mail_dispatcher....
reviewbot/mail_dispatcher.py:44: match = re.search(r'\(issue *(?P<id>\d+)\)$',
message.subject)
Ick... this is definitely fragile. Is there a way we could encode the issue as
an X header in the email? That would be more portable going forward.

[Mattias Nissler (ping if slow), 2013-08-16 12:49:22]

Thanks for the comments, I think the new version I uploaded is indeed much
better.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
File reviewbot/handlers/policy_checklist.py (right):

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:13: CONTEXT_THRESHOLD = 12
On 2013/08/15 03:48:05, iannucci wrote:
> Should we make a separate config file specifically for strings like these?

I'd expect that if we need changes, not only the constants but also the parser
code will need to be adjusted. Thus, keeping them in a single place seems
preferable to me.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:14: PROPERTY_NAME_RE =
re.compile('\'(\\w+)\'\\s*:')
On 2013/08/15 03:48:05, iannucci wrote:
> may be worth using a raw string to reduce the backslash madness
> 
> r'\'(\w+)\'\s*:'

Good idea, done.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:35: to re-implement your policy on top of
the new implementation.
On 2013/08/15 03:48:05, iannucci wrote:
> I'm assuming these strings have all been vetted by the appropriate whomever?

Yes, I've discussed them in a doc with the chrome/app/policy/OWNERS.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:61: so the policy shows up on the
chrome://policy page.
On 2013/08/15 03:48:05, iannucci wrote:
> It may make sense to actually turn these strings into templates. I suspect
that
> will need to happen at some near point in the future, it may be easier to do
it
> now.

Good point. Done.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:89: except ValueError:
On 2013/08/15 03:48:05, iannucci wrote:
> This seems sketchy. Why ValueError?
> 
> Also, I think this will throw a TypeError if a = [] or a = [1]

min raises ValueError if the sequence is empty. I've updated the code to handle
that situation explicitly now, which is easier to follow.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:109: return None if nspaces == 0 else
nspaces
On 2013/08/15 03:48:05, iannucci wrote:
> You may want to take a look at the textwrap module

This actually determines the indent from a given line of text. I updated the
function name to be clearer.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:112: class PolicyChangeParser(object):
On 2013/08/15 03:48:05, iannucci wrote:
> Needs docs, code is hard to read without context/examples.

I've added documentation and some illustrative unit tests.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:232: return
On 2013/08/15 03:48:05, iannucci wrote:
> So the idea here is that you could add reviewbot as a reviewer?

Correct. Once we've done so a couple times and convinced ourselves it's working,
the intention is to add the bot to an appropriate WATCHLISTS entry.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:239: parser = PolicyChangeParser(
On 2013/08/15 03:48:05, iannucci wrote:
> why not just make PolicyChangeParser a function returning a list of chunks?

It's got all this state to carry around, which is easier to handle within an
object. I've now created a wrapper function that makes it easier to use.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
reviewbot/handlers/policy_checklist.py:259: line, side, message)
On 2013/08/15 03:48:05, iannucci wrote:
> Maybe I'm unfamiliar with the types of changes here, but this could end up
> posting a lot of (fairly large) comments. Maybe it should just post a max of
one
> of each message type?

I've tried it on GIT history and it usually posted 1-4 comments on changes that
were affected, with occasional outliers.

It's definitely a good idea to put a hard limit here, so I did that.

https://codereview.chromium.org/20518002/diff/4001/reviewbot/mail_dispatcher.py
File reviewbot/mail_dispatcher.py (right):

https://codereview.chromium.org/20518002/diff/4001/reviewbot/mail_dispatcher....
reviewbot/mail_dispatcher.py:44: match = re.search(r'\(issue *(?P<id>\d+)\)$',
message.subject)
On 2013/08/15 03:48:05, iannucci wrote:
> Ick... this is definitely fragile. Is there a way we could encode the issue as
> an X header in the email? That would be more portable going forward.

Yes it is... but it is the exact way that rietveld uses itself to figure out
which review the mail belongs to :)

See here:
https://code.google.com/p/rietveld/source/browse/codereview/views.py#4224

Added a comment to that effect.

[iannucci, 2013-08-20 00:45:31]

On 2013/08/16 12:49:22, Mattias Nissler wrote:
> Thanks for the comments, I think the new version I uploaded is indeed much
> better.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> File reviewbot/handlers/policy_checklist.py (right):
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:13: CONTEXT_THRESHOLD = 12
> On 2013/08/15 03:48:05, iannucci wrote:
> > Should we make a separate config file specifically for strings like these?
> 
> I'd expect that if we need changes, not only the constants but also the parser
> code will need to be adjusted. Thus, keeping them in a single place seems
> preferable to me.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:14: PROPERTY_NAME_RE =
> re.compile('\'(\\w+)\'\\s*:')
> On 2013/08/15 03:48:05, iannucci wrote:
> > may be worth using a raw string to reduce the backslash madness
> > 
> > r'\'(\w+)\'\s*:'
> 
> Good idea, done.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:35: to re-implement your policy on top
of
> the new implementation.
> On 2013/08/15 03:48:05, iannucci wrote:
> > I'm assuming these strings have all been vetted by the appropriate whomever?
> 
> Yes, I've discussed them in a doc with the chrome/app/policy/OWNERS.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:61: so the policy shows up on the
> chrome://policy page.
> On 2013/08/15 03:48:05, iannucci wrote:
> > It may make sense to actually turn these strings into templates. I suspect
> that
> > will need to happen at some near point in the future, it may be easier to do
> it
> > now.
> 
> Good point. Done.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:89: except ValueError:
> On 2013/08/15 03:48:05, iannucci wrote:
> > This seems sketchy. Why ValueError?
> > 
> > Also, I think this will throw a TypeError if a = [] or a = [1]
> 
> min raises ValueError if the sequence is empty. I've updated the code to
handle
> that situation explicitly now, which is easier to follow.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:109: return None if nspaces == 0 else
> nspaces
> On 2013/08/15 03:48:05, iannucci wrote:
> > You may want to take a look at the textwrap module
> 
> This actually determines the indent from a given line of text. I updated the
> function name to be clearer.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:112: class PolicyChangeParser(object):
> On 2013/08/15 03:48:05, iannucci wrote:
> > Needs docs, code is hard to read without context/examples.
> 
> I've added documentation and some illustrative unit tests.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:232: return
> On 2013/08/15 03:48:05, iannucci wrote:
> > So the idea here is that you could add reviewbot as a reviewer?
> 
> Correct. Once we've done so a couple times and convinced ourselves it's
working,
> the intention is to add the bot to an appropriate WATCHLISTS entry.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:239: parser = PolicyChangeParser(
> On 2013/08/15 03:48:05, iannucci wrote:
> > why not just make PolicyChangeParser a function returning a list of chunks?
> 
> It's got all this state to carry around, which is easier to handle within an
> object. I've now created a wrapper function that makes it easier to use.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/handlers/policy_...
> reviewbot/handlers/policy_checklist.py:259: line, side, message)
> On 2013/08/15 03:48:05, iannucci wrote:
> > Maybe I'm unfamiliar with the types of changes here, but this could end up
> > posting a lot of (fairly large) comments. Maybe it should just post a max of
> one
> > of each message type?
> 
> I've tried it on GIT history and it usually posted 1-4 comments on changes
that
> were affected, with occasional outliers.
> 
> It's definitely a good idea to put a hard limit here, so I did that.
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/mail_dispatcher.py
> File reviewbot/mail_dispatcher.py (right):
> 
>
https://codereview.chromium.org/20518002/diff/4001/reviewbot/mail_dispatcher....
> reviewbot/mail_dispatcher.py:44: match = re.search(r'\(issue *(?P<id>\d+)\)$',
> message.subject)
> On 2013/08/15 03:48:05, iannucci wrote:
> > Ick... this is definitely fragile. Is there a way we could encode the issue
as
> > an X header in the email? That would be more portable going forward.
> 
> Yes it is... but it is the exact way that rietveld uses itself to figure out
> which review the mail belongs to :)
> 
> See here:
> https://code.google.com/p/rietveld/source/browse/codereview/views.py#4224
> 
> Added a comment to that effect.

agable@, can you do the final review for this (b/c OOO)?

[agable, 2013-08-20 08:42:41]

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
File reviewbot/handlers/policy_checklist/__init__.py (right):

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/__init__.py:1: # Copyright (c) 2013 The
Chromium Authors. All rights reserved.
I dislike having this much logic in __init__ (as opposed to just having classes
and imports to surface logic from other modules in the package) but this is
probably fine.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
File reviewbot/handlers/policy_checklist/parser.py (right):

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/parser.py:11: PROPERTY_NAME_RE =
re.compile(r"'(\w+)\'\s*:")
Don't need to escape the single quote in the middle.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/parser.py:86: match =
PROPERTY_NAME_RE.search(line)
Can the json property happen anywhere in the line, or only at the beginning?
re.match is more efficient than re.search, if the latter.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/parser.py:129: # Attempt to filter out name
labels on enum items.
Please include a test for this behavior.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/mail_dispatcher.py
File reviewbot/mail_dispatcher.py (right):

https://codereview.chromium.org/20518002/diff/18001/reviewbot/mail_dispatcher...
reviewbot/mail_dispatcher.py:25: # addresses the app receives email on is used
to as a key into the handler map.
nit: ...is used as a key...

https://codereview.chromium.org/20518002/diff/18001/reviewbot/mail_dispatcher...
reviewbot/mail_dispatcher.py:44: # up-to-date with the mechanism rietveld use
the tools don't confuse issues.
nit: ...uses so the tools...

[Mattias Nissler (ping if slow), 2013-08-22 12:44:22]

Adressed your comments, ready for another round.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
File reviewbot/handlers/policy_checklist/__init__.py (right):

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/__init__.py:1: # Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/08/20 08:42:41, Aaron Gable wrote:
> I dislike having this much logic in __init__ (as opposed to just having
classes
> and imports to surface logic from other modules in the package) but this is
> probably fine.

Fair enough, moved the code out to handler.py

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
File reviewbot/handlers/policy_checklist/parser.py (right):

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/parser.py:11: PROPERTY_NAME_RE =
re.compile(r"'(\w+)\'\s*:")
On 2013/08/20 08:42:41, Aaron Gable wrote:
> Don't need to escape the single quote in the middle.

Done.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/parser.py:86: match =
PROPERTY_NAME_RE.search(line)
On 2013/08/20 08:42:41, Aaron Gable wrote:
> Can the json property happen anywhere in the line, or only at the beginning?
> re.match is more efficient than re.search, if the latter.

There should only be one, prefixed by whitespace. Updated the regex to that
effect.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/handlers/policy...
reviewbot/handlers/policy_checklist/parser.py:129: # Attempt to filter out name
labels on enum items.
On 2013/08/20 08:42:41, Aaron Gable wrote:
> Please include a test for this behavior.

Done.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/mail_dispatcher.py
File reviewbot/mail_dispatcher.py (right):

https://codereview.chromium.org/20518002/diff/18001/reviewbot/mail_dispatcher...
reviewbot/mail_dispatcher.py:25: # addresses the app receives email on is used
to as a key into the handler map.
On 2013/08/20 08:42:41, Aaron Gable wrote:
> nit: ...is used as a key...

Done.

https://codereview.chromium.org/20518002/diff/18001/reviewbot/mail_dispatcher...
reviewbot/mail_dispatcher.py:44: # up-to-date with the mechanism rietveld use
the tools don't confuse issues.
On 2013/08/20 08:42:41, Aaron Gable wrote:
> nit: ...uses so the tools...

Done.

[agable, 2013-08-22 12:48:38]

Thanks, LGTM!

[Mattias Nissler (ping if slow), 2013-08-22 12:57:59]

Committed patchset #4 manually as r219005 (presubmit successful).