Use m['approval'] instead of looking at the text for 'lgtm'.

This makes it coherent with the commit queue by keeping the lgtm at a single
place.

This simplifies owners check code.

R=dpranke@chromium.org
BUG=
TEST=

Review URL: http://codereview.chromium.org/7082029

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@87251 0039d316-1c4b-4281-b951-d872f2087c98

presubmit_canned_checks.py

@@ -761,60 +761,18 @@ def _RietveldOwnerAndApprovers(input_api, email_regexp):
   if not host.startswith('http://') and not host.startswith('https://'):
     host = 'http://' + host
   url = '%s/api/%s?messages=true' % (host, input_api.change.issue)
-
-  f = input_api.urllib2.urlopen(url)
-  issue_props = input_api.json.load(f)
-  messages = issue_props.get('messages', [])
+  issue_props = input_api.json.load(input_api.urllib2.urlopen(url))
   owner_email = issue_props['owner_email']
-  owner_regexp = input_api.re.compile(input_api.re.escape(owner_email))
-  approvers = _GetApprovers(messages, email_regexp, owner_regexp)
-
-  return (owner_email, set(approvers))
-
-
-def _IsApprovingComment(text):
-  """Implements the logic for parsing a change comment for approval."""
-
-  # Any comment that contains a non-quoted line containing an 'lgtm' is an
-  # approval.
-  #
-  # TODO(dpranke): this differs from the logic used inside Google in a few
-  # ways. Inside Google,
-  #
-  # 1) the approving phrase must appear at the beginning of the first non
-  #    quoted-line in the comment.'
-  # 2) "LG", "Looks Good", and "Looks Good to Me" are also acceptable.
-  # 3) Subsequent comments from the reviewer can rescind approval, unless
-  #    the phrase "LGTM++" was used.
-  # We should consider implementing some or all of this here.
-  for l in text.splitlines():
-    l = l.strip().lower()
-    if l.startswith('>'):
-      continue
-
-    if 'lgtm' in l:
-      return True
-  return False
-
 
-def _GetApprovers(messages, email_regexp, owner_regexp):
-  """Returns the set of approvers for a change given the owner and messages.
+  def match_reviewer(r):
+    return email_regexp.match(r) and r != owner_email
 
-  Messages should be a list of dicts containing 'sender' and 'text' keys."""
+  messages = issue_props.get('messages', [])
+  approvers = set(
+      m['sender'] for m in messages
+      if m.get('approval') and match_reviewer(m['sender']))
 
-  # TODO(dpranke): This mimics the logic in
-  # /tools/commit-queue/verifiers/reviewer_lgtm.py
-  # We should share the code and/or remove the check there where it is
-  # redundant (since the commit queue also enforces the presubmit checks).
-  def match_reviewer(r):
-    return email_regexp.match(r) and not owner_regexp.match(r)
-
-  approvers = []
-  for m in messages:
-    sender = m['sender']
-    if _IsApprovingComment(m['text']) and match_reviewer(sender):
-      approvers.append(sender)
-  return set(approvers)
+  return owner_email, approvers
 
 
 def _CheckConstNSObject(input_api, output_api, source_file_filter):

tests/presubmit_unittest.py

@@ -2029,11 +2029,12 @@ mac|success|blew
           expected_host = host_url
 
       owner_email = 'john@example.com'
-      messages = list('{"sender": "' + a + '","text": "lgtm"}' for
-                      a in approvers)
       if not rietveld_response:
-        rietveld_response = ('{"owner_email": "' + owner_email + '",'
-                            '"messages": [' + ','.join(messages) + ']}')
+        messages = (
+          '{"sender": "%s", "text": "I approve", "approval": true}' % a
+          for a in approvers)
+        rietveld_response = '{"owner_email": "%s", "messages": [%s]}' % (
+            owner_email, ','.join(messages))
       input_api.urllib2.urlopen(
           expected_host + '/api/1?messages=true').AndReturn(
           StringIO.StringIO(rietveld_response))
@@ -2050,33 +2051,21 @@ mac|success|blew
     self.assertEquals(output.getvalue(), expected_output)
 
   def testCannedCheckOwners_LGTMPhrases(self):
-    def phrase_test(phrase, approvers=None, expected_output=''):
+    def phrase_test(approval, approvers=None, expected_output=''):
       if approvers is None:
         approvers = set(['ben@example.com'])
       self.AssertOwnersWorks(approvers=approvers,
-          rietveld_response='{"owner_email": "john@example.com",' +
-                            '"messages": [{"sender": "ben@example.com",' +
-                                          '"text": "' + phrase + '"}]}',
+          rietveld_response=(
+            '{"owner_email": "john@example.com",'
+            '"messages": [{"sender": "ben@example.com",'
+            '  "text": "foo", "approval": %s}]}') % approval,
           expected_output=expected_output)
 
-    phrase_test('LGTM')
-    phrase_test('\\nlgtm')
-    phrase_test('> foo\\n> bar\\nlgtm\\n')
-    phrase_test('> LGTM', approvers=set(),
+    phrase_test('true')
+    phrase_test('false', approvers=set(),
                 expected_output='Missing LGTM from someone other than '
                                 'john@example.com\n')
 
-    # TODO(dpranke): these probably should pass.
-    phrase_test('Looks Good To Me', approvers=set(),
-                expected_output='Missing LGTM from someone other than '
-                                'john@example.com\n')
-    phrase_test('looks good to me', approvers=set(),
-                expected_output='Missing LGTM from someone other than '
-                                'john@example.com\n')
-
-    # TODO(dpranke): this probably shouldn't pass.
-    phrase_test('no lgtm for you')
-
   def testCannedCheckOwners_HTTPS_HostURL(self):
     self.AssertOwnersWorks(approvers=set(['ben@example.com']),
                            host_url='https://localhost')