Implement mail dispatcher app.

reviewbot/handlers/policy_checklist.py

+# Copyright (c) 2013 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import re
+
+import model.app_config
+import util
+
+
+POLICY_TEMPLATES_FILE = 'chrome/app/policy/policy_templates.json'
+ID_LINE_COMMENT = 'For your editing convenience: highest ID currently used:'
+CONTEXT_THRESHOLD = 12

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

Should we make a separate config file specifically for strings like these?

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

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.

+PROPERTY_NAME_RE = re.compile('\'(\\w+)\'\\s*:')

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

may be worth using a raw string to reduce the backslash madness

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

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

Good idea, done.

+MAX_LINE = 1000000
+
+REVIEW_MESSAGE = '''
+Thanks for improving Chromium enterprise features! In general,
+enterprise policies should work across all platforms and need to remain
+backward compatible across several Chrome versions. To help avoid common
+pitfalls, policy review bot has analyzed your change and added comments
+with checklists of things to watch out for.
+
+Please direct comments and complaints about this service to
+chrome-enterprise@google.com and/or mnissler@chromium.org.
+'''
+
+ADDITION_COMMENT = '''
+When adding a new policy setting, please consider the following:
+
+- Ideally, your new policy should be expressing the intention of an
+  administrator and/or cover a well-scoped user-visible behavior of the
+  browser as opposed to an implementation-specific parameter. Consider
+  that your implementation may change in the future, and you might have
+  to re-implement your policy on top of the new implementation.

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

I'm assuming these strings have all been vetted by the appropriate whomever?

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

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

+- Make sure your policy definition has proper supported_on declarations
+  specifying the platform and versions this policy is supported on.
+- Make sure feature flags are correct. In particular:
+  - dynamic_refresh - whether your feature can adjust to changes in
+    policy at run time. You typically use PrefChangeRegistrar to do so.
+  - per_profile - whether your policy is browser-global or can be set
+    independently for each Chrome Profile. This is usually true if you
+    read the policy value from the Profile's PrefService and false if
+    you read it from the local_state PrefService.
+  - can_be_recommended - whether your feature supports admin-supplied
+    default values that the user can override.
+- Make sure you put a helpful policy description:
+  - Describe the effect of setting the policy in a way that makes sense
+    to somebody who is not intimately familiar with your feature, such
+    as administrators and other Chromium developers.
+  - Do mention behavior for the cases where the policy gets ignored
+    (i.e. not configured, booleans only one value is effective). It's
+    nice for completeness and admins have been asking specifically for
+    this piece of information in the past.
+- Write a browser_test for you new policy. The ideal test would fire up
+  the browser with the policy set and check whether the policy affects
+  user-visible behavior in the intended way. See
+  chrome/browser/policy/policy_browsertest.cc for examples.
+- If you're adding a device policy for Chrome OS, be sure to update
+  chrome/browser/chromeos/policy/device_policy_decoder_chromeos.{h,cc}
+  so the policy shows up on the chrome://policy page.

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

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.

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

Good point. Done.

+'''
+
+MODIFICATION_COMMENT = '''
+When modifying an existing policy setting, please consider the following:
+
+- Make sure the policy meta data is up-to-date, in particular
+  supported_on, and the feature flags.
+- In general, don't change policy semantics in a way that is
+  incompatible (as determined by user/admin-visible behavior) with
+  previous semantics. In particular, consider that existing policy
+  deployments may affect both old and new browser versions, and both
+  should behave according to the admin's intentions.
+- It is OK to expand semantics of policy values as long as the previous
+  policy description is compatible with the new behavior.
+- It is OK to update feature implementations and the policy description
+  when Chrome changes as long as the intended effect of the policy
+  remains intact.
+- The process for removing policies is to deprecate them first, wait a
+  few releases (if possible) and then remove them. Make sure you put the
+  deprecated flag if you deprecate a policy.
+'''
+
+
+def nmin(*args):
+  """Calculates the minimum of |args|, ignoring None entries."""
+  try:
+    return min(v for v in args if v is not None)
+  except ValueError:

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

This seems sketchy. Why ValueError?

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

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

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

+    return None
+
+
+def nmax(*args):
+  """Calculates the maximum of |args|, ignoring None entries."""
+  try:
+    return max(v for v in args if v is not None)
+  except ValueError:
+    return None
+
+
+def nsub(a, b):
+  """Calculates a - b, returning None if either a or b is None"""
+  return None if (a is None or b is None) else a - b
+
+
+def indent(line):
+  """Returns the indent level (number of leading spaces) for |line|."""
+  nspaces = len(line) - len(line.lstrip(' '))
+  return None if nspaces == 0 else nspaces

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

You may want to take a look at the textwrap module

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

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

+
+
+class PolicyChangeParser(object):

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

Needs docs, code is hard to read without context/examples.

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

I've added documentation and some illustrative unit tests.

+  def __init__(self, lines):
+    self.lines = lines
+    self.chunks_list = []
+    self.reset()
+
+  def run(self):
+    self.chunks_list = []
+    self.last_change = [None, None]
+    cursor = [None, None]
+    self.reset()
+    for (a_line, b_line, line) in self.lines:
+      # Skip comment lines.
+      if line.startswith('#'):
+        continue
+
+      # See whether the current line has a JSON property.
+      keyword = None
+      match = PROPERTY_NAME_RE.search(line)
+      if match:
+        keyword = match.group(1).lower()
+
+
+      # Check whether the current block closes.
+      line_indent = indent(line)
+      if (self.block_indent is not None and
+            line_indent is not None and
+            line_indent < self.block_indent):
+        self.block_closed = True
+
+      if (keyword == 'name' and self.block_closed):
+        # If we see the 'name' property, that likely indicates the start of a
+        # new policy. Start a new chunk.
+        self.flush_chunk()
+
+      # Update various cursors.
+      cursor = [nmax(a_line, cursor[0]), nmax(b_line, cursor[1])]
+      offset = nmin(nsub(cursor[0], self.last_change[0]),
+                    nsub(cursor[1], self.last_change[1]))
+
+      if a_line > 0 and b_line == 0:
+        self.removals = True
+        self.last_change[0] = a_line
+      elif a_line == 0 and b_line > 0:
+        self.additions = True
+        self.last_change[1] = b_line
+
+      if (offset is not None and
+          (offset > CONTEXT_THRESHOLD or
+           (offset > 1 and self.block_closed))):
+        # If the last chunk is too far away, assume a new one starts.
+        self.flush_chunk()
+
+      # Try to figure out block indent from properties exclusively used for
+      # policy definitions.
+      if keyword in ('id', 'schema', 'future', 'items', 'features',
+                     'supported_on', 'example_value', 'deprecated'):
+        self.block_indent = line_indent
+
+      # Prefer the comment on the policy name property if we see it fly by.
+      if keyword == 'name':
+        # Attempt to filter out name labels on enum items.
+        if self.block_indent is not None and self.block_indent != line_indent:
+          pass
+        elif a_line > 0 and b_line == 0:
+          self.comment_pos[0] = a_line
+        elif a_line == 0 and b_line > 0:
+          self.comment_pos[1] = b_line
+
+      self.chunk_start = [nmin(self.last_change[0], self.chunk_start[0]),
+                          nmin(self.last_change[1], self.chunk_start[1])]
+
+    # Flush the last chunk.
+    if self.chunk_start != [None, None]:
+      self.flush_chunk()
+
+  @property
+  def chunks(self):
+    return self.chunks_list
+
+  def flush_chunk(self):
+    self.comment_pos = [nmax(self.chunk_start[0], self.comment_pos[0]),
+                        nmax(self.chunk_start[1], self.comment_pos[1])]
+    self.chunks_list.append(
+        util.ObjectDict(
+            { 'start': self.chunk_start,
+              'end': self.last_change,
+              'comment_pos': self.comment_pos,
+              'additions': self.additions,
+              'removals': self.removals }))
+    self.reset()
+
+  def reset(self):
+    # This is called from __init__.
+    # pylint: disable=W0201
+    self.chunk_start = [None, None]
+    self.last_change = [None, None]
+    self.comment_pos = [None, None]
+    self.block_indent = None
+    self.block_closed = False
+    self.additions = False
+    self.removals = False
+
+
+def process(addr, message, review, rietveld):
+  """Handles reviews for chrome/app/policy/policy_templates.json.
+
+  This looks at the patch to identify additions/modifications to policy
+  definitions and posts comments with a checklist intended for the author and
+  reviewer to go through in order to catch common mistakes.
+  """
+
+  if POLICY_TEMPLATES_FILE not in review.latest_patchset.files:
+    return
+
+  # Only process the change if the mail is directly to us or we haven't
+  # processed this review yet.
+  client_id = model.app_config.get().client_id
+  if (not addr in util.get_emails(getattr(message, 'to', '')) and
+      client_id in [m.sender for m in review.issue_data.messages]):
+    return

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

So the idea here is that you could add reviewbot as a reviewer?

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

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.

+
+  # Don't process reverts.
+  if 'revert' in review.issue_data.description.lower():
+    return
+
+  # Parse the patch, look at the chunks and generate inline comments.
+  parser = PolicyChangeParser(

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

why not just make PolicyChangeParser a function returning a list of chunks?

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

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.

+      review.latest_patchset.files[POLICY_TEMPLATES_FILE].patch.lines)
+  parser.run()
+  for chunk in parser.chunks:
+    if chunk.additions and not chunk.removals:
+      message = ADDITION_COMMENT
+    else:
+      message = MODIFICATION_COMMENT
+
+    if chunk.comment_pos[1] is not None:
+      line, side = chunk.comment_pos[1], 'b'
+    elif chunk.comment_pos[0] is not None:
+      line, side = chunk.comment_pos[0], 'a'
+    else:
+      # No suitable position?
+      continue
+
+    rietveld.add_inline_comment(
+        review.issue_id, review.latest_patchset.patchset,
+        review.latest_patchset.files[POLICY_TEMPLATES_FILE].id,
+        line, side, message)

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

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?

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

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.

+
+  # Finally, post all inline comments.
+  if len(parser.chunks) > 0:
+    rietveld.post_comment(review.issue_id, REVIEW_MESSAGE, True)