Give Rietveld git-cl-status the 'not lgtm' state

While working on fixing git-cl-status for gerrit, I realized
it would be really easy to bring the Rietveld version up to
parity and simplify it at the same time.

Bug: 706460
Change-Id: Icff32b532fa29f8869205111cd117176e0d34b8f
Reviewed-on: https://chromium-review.googlesource.com/470448
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

git_cl.py

@@ -1730,9 +1730,6 @@ class Changelist(object):
     """Get owner from codereview, which may differ from this checkout."""
     return self._codereview_impl.GetIssueOwner()
 
-  def GetApprovingReviewers(self):
-    return self._codereview_impl.GetApprovingReviewers()
-
   def GetMostRecentPatchset(self):
     return self._codereview_impl.GetMostRecentPatchset()
 
@@ -1826,13 +1823,6 @@ class _ChangelistCodereviewBase(object):
     """Closes the issue."""
     raise NotImplementedError()
 
-  def GetApprovingReviewers(self):
-    """Returns a list of reviewers approving the change.
-
-    Note: not necessarily committers.
-    """
-    raise NotImplementedError()
-
   def GetMostRecentPatchset(self):
     """Returns the most recent patchset number from the codereview site."""
     raise NotImplementedError()
@@ -1995,9 +1985,6 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
       'rietveld': self.GetCodereviewServer(),
     }
 
-  def GetApprovingReviewers(self):
-    return get_approving_reviewers(self.GetIssueProperties())
-
   def GetIssueOwner(self):
     return (self.GetIssueProperties() or {}).get('owner_email')
 
@@ -2026,6 +2013,7 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
       * 'unsent'   - not sent for review
       * 'waiting'  - waiting for review
       * 'reply'    - waiting for owner to reply to review
+      * 'not lgtm' - Code-Review label has been set negatively
       * 'lgtm'     - LGTM from at least one approved reviewer
       * 'commit'   - in the commit queue
       * 'closed'   - closed
@@ -2045,16 +2033,15 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
       # Issue is in the commit queue.
       return 'commit'
 
-    try:
+    messages = props.get('messages') or []
-      reviewers = self.GetApprovingReviewers()
+    if not messages:
-    except urllib2.HTTPError:
+      # No message was sent.
-      return 'error'
+      return 'unsent'
 
-    if reviewers:
+    if get_approving_reviewers(props):
-      # Was LGTM'ed.
       return 'lgtm'
-
+    elif get_approving_reviewers(props, disapproval=True):
-    messages = props.get('messages') or []
+      return 'not lgtm'
 
     # Skip CQ messages that don't require owner's action.
     while messages and messages[-1]['sender'] == COMMIT_BOT_EMAIL:
@@ -2068,9 +2055,6 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
         # This is probably a CQ messages warranting user attention.
         break
 
-    if not messages:
-      # No message was sent.
-      return 'unsent'
     if messages[-1]['sender'] != props.get('owner_email'):
       # Non-LGTM reply from non-owner and not CQ bot.
       return 'reply'
@@ -2617,13 +2601,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
   def CloseIssue(self):
     gerrit_util.AbandonChange(self._GetGerritHost(), self.GetIssue(), msg='')
 
-  def GetApprovingReviewers(self):
-    """Returns a list of reviewers approving the change.
-
-    Note: not necessarily committers.
-    """
-    raise NotImplementedError()
-
   def SubmitIssue(self, wait_for_merge=True):
     gerrit_util.SubmitChange(self._GetGerritHost(), self.GetIssue(),
                              wait_for_merge=wait_for_merge)
@@ -3428,18 +3405,21 @@ class ChangeDescription(object):
     self._description_lines.extend('%s: %s' % kv for kv in parsed_footers)
 
 
-def get_approving_reviewers(props):
+def get_approving_reviewers(props, disapproval=False):
   """Retrieves the reviewers that approved a CL from the issue properties with
   messages.
 
   Note that the list may contain reviewers that are not committer, thus are not
   considered by the CQ.
+
+  If disapproval is True, instead returns reviewers who 'not lgtm'd the CL.
   """
+  approval_type = 'disapproval' if disapproval else 'approval'
   return sorted(
       set(
         message['sender']
         for message in props['messages']
-        if message['approval'] and message['sender'] in props['reviewers']
+        if message[approval_type] and message['sender'] in props['reviewers']
       )
   )
 
@@ -3942,7 +3922,7 @@ def get_cl_statuses(changes, fine_grained, max_processes=None):
 
   if not fine_grained:
     # Fast path which doesn't involve querying codereview servers.
-    # Do not use GetApprovingReviewers(), since it requires an HTTP request.
+    # Do not use get_approving_reviewers(), since it requires an HTTP request.
     for cl in changes:
       yield (cl, 'waiting' if cl.GetIssueURL() else 'error')
     return
@@ -4909,7 +4889,8 @@ def CMDland(parser, args):
   # contains the link to the Rietveld issue, while the Rietveld message contains
   # the commit viewvc url.
   if cl.GetIssue():
-    change_desc.update_reviewers(cl.GetApprovingReviewers())
+    change_desc.update_reviewers(
+        get_approving_reviewers(cl.GetIssueProperties()))
 
   commit_desc = ChangeDescription(change_desc.description)
   if cl.GetIssue():

tests/git_cl_test.py

@@ -68,6 +68,7 @@ class RietveldMock(object):
       'messages': [
         {
           'approval': True,
+          'disapproval': False,
           'sender': 'john@chromium.org',
         },
       ],