git cl comment: refactor fetching comments for Rietveld and add tests.

R=agable@chromium.org,machenbach@chromium.org
BUG=698236

Change-Id: I04e264130952e3e270540fae8381db544cf40e7c
Reviewed-on: https://chromium-review.googlesource.com/456697
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>

git_cl.py

@@ -14,6 +14,7 @@ from multiprocessing.pool import ThreadPool
 import base64
 import collections
 import contextlib
+import datetime
 import fnmatch
 import httplib
 import itertools
@@ -1090,6 +1091,12 @@ class GerritChangeNotExists(Exception):
         self.issue, self.url)
 
 
+_CommentSummary = collections.namedtuple(
+    '_CommentSummary', ['date', 'message', 'sender',
+                        # TODO(tandrii): these two aren't known in Gerrit.
+                        'approval', 'disapproval'])
+
+
 class Changelist(object):
   """Changelist works with one changelist in local branch.
 
@@ -1657,6 +1664,14 @@ class Changelist(object):
   def AddComment(self, message):
     return self._codereview_impl.AddComment(message)
 
+  def GetCommentsSummary(self):
+    """Returns list of _CommentSummary for each comment.
+
+    Note: comments per file or per line are not included,
+    only top-level comments are returned.
+    """
+    return self._codereview_impl.GetCommentsSummary()
+
   def CloseIssue(self):
     return self._codereview_impl.CloseIssue()
 
@@ -1759,6 +1774,9 @@ class _ChangelistCodereviewBase(object):
     """Posts a comment to the codereview site."""
     raise NotImplementedError()
 
+  def GetCommentsSummary(self):
+    raise NotImplementedError()
+
   def CloseIssue(self):
     """Closes the issue."""
     raise NotImplementedError()
@@ -1934,6 +1952,19 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
   def AddComment(self, message):
     return self.RpcServer().add_comment(self.GetIssue(), message)
 
+  def GetCommentsSummary(self):
+    summary = []
+    for message in self.GetIssueProperties().get('messages', []):
+      date = datetime.datetime.strptime(message['date'], '%Y-%m-%d %H:%M:%S.%f')
+      summary.append(_CommentSummary(
+        date=date,
+        disapproval=bool(message['disapproval']),
+        approval=bool(message['approval']),
+        sender=message['sender'],
+        message=message['text'],
+      ))
+    return summary
+
   def GetStatus(self):
     """Apply a rough heuristic to give a simple summary of an issue's review
     or CQ status, assuming adherence to a common workflow.
@@ -2499,6 +2530,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     gerrit_util.SetReview(self._GetGerritHost(), self.GetIssue(),
                           msg=message)
 
+  def GetCommentsSummary(self):
+    # TODO(tandrii): implement in follow up CL (http://crbug.com/698236).
+    raise NotImplementedError()
+
   def CloseIssue(self):
     gerrit_util.AbandonChange(self._GetGerritHost(), self.GetIssue(), msg='')
 
@@ -4234,34 +4269,30 @@ def CMDcomments(parser, args):
     cl.AddComment(options.comment)
     return 0
 
-  data = cl.GetIssueProperties()
-  summary = []
-  for message in sorted(data.get('messages', []), key=lambda x: x['date']):
-    summary.append({
-        'date': message['date'],
-        'lgtm': False,
-        'message': message['text'],
-        'not_lgtm': False,
-        'sender': message['sender'],
-    })
-    if message['disapproval']:
+  summary = sorted(cl.GetCommentsSummary(), key=lambda c: c.date)
+  for comment in summary:
+    if comment.disapproval:
       color = Fore.RED
-      summary[-1]['not lgtm'] = True
-    elif message['approval']:
+    elif comment.approval:
       color = Fore.GREEN
-      summary[-1]['lgtm'] = True
-    elif message['sender'] == data['owner_email']:
+    elif comment.sender == cl.GetIssueOwner():
       color = Fore.MAGENTA
     else:
       color = Fore.BLUE
-    print('\n%s%s  %s%s' % (
-        color, message['date'].split('.', 1)[0], message['sender'],
-        Fore.RESET))
-    if message['text'].strip():
-      print('\n'.join('  ' + l for l in message['text'].splitlines()))
+    print('\n%s%s   %s%s\n%s' % (
+      color,
+      comment.date.strftime('%Y-%m-%d %H:%M:%S UTC'),
+      comment.sender,
+      Fore.RESET,
+      '\n'.join('  ' + l for l in comment.message.strip().splitlines())))
+
   if options.json_file:
+    def pre_serialize(c):
+      dct = c.__dict__.copy()
+      dct['date'] = dct['date'].strftime('%Y-%m-%d %H:%M:%S.%f')
+      return dct
     with open(options.json_file, 'wb') as f:
-      json.dump(summary, f)
+      json.dump(map(pre_serialize, summary), f)
   return 0
 
 

tests/git_cl_test.py

@@ -6,6 +6,7 @@
 """Unit tests for git_cl.py."""
 
 import contextlib
+import datetime
 import json
 import logging
 import os
@@ -3117,6 +3118,59 @@ class TestGitCl(TestCase):
     self.assertEqual(0, git_cl.main(['comment', '--gerrit', '-i', '10',
                                      '-a', 'msg']))
 
+  def test_git_cl_comments_fetch_rietveld(self):
+    self.mock(sys, 'stdout', StringIO.StringIO())
+    self.calls = [
+      ((['git', 'config', 'rietveld.autoupdate'],), CERR1),
+      ((['git', 'config', 'rietveld.server'],), 'codereview.chromium.org'),
+    ] * 2
+    self.mock(git_cl._RietveldChangelistImpl, 'GetIssueProperties', lambda _: {
+      'messages': [
+        {'text': 'lgtm', 'date': '2017-03-13 20:49:34.515270',
+         'disapproval': False, 'approval': True, 'sender': 'r@example.com'},
+        {'text': 'not lgtm', 'date': '2017-03-13 21:50:34.515270',
+         'disapproval': True, 'approval': False, 'sender': 'r2@example.com'},
+        # Intentionally wrong order here.
+        {'text': 'PTAL', 'date': '2000-03-13 20:49:34.515270',
+         'disapproval': False, 'approval': False,
+         'sender': 'owner@example.com'},
+      ],
+      'owner_email': 'owner@example.com',
+    })
+    expected_comments_summary = [
+      git_cl._CommentSummary(
+        message='lgtm',
+        date=datetime.datetime(2017, 3, 13, 20, 49, 34, 515270),
+        disapproval=False, approval=True, sender='r@example.com'),
+      git_cl._CommentSummary(
+        message='not lgtm',
+        date=datetime.datetime(2017, 3, 13, 21, 50, 34, 515270),
+        disapproval=True, approval=False, sender='r2@example.com'),
+      # Note: same order as in whatever Rietveld returns.
+      git_cl._CommentSummary(
+        message='PTAL',
+        date=datetime.datetime(2000, 3, 13, 20, 49, 34, 515270),
+        disapproval=False, approval=False, sender='owner@example.com'),
+    ]
+    cl = git_cl.Changelist(codereview='rietveld', issue=1)
+    self.assertEqual(cl.GetCommentsSummary(), expected_comments_summary)
+
+    with git_cl.gclient_utils.temporary_directory() as tempdir:
+      out_file = os.path.abspath(os.path.join(tempdir, 'out.json'))
+      self.assertEqual(0, git_cl.main(['comment', '--rietveld', '-i', '10',
+                                       '-j', out_file]))
+      with open(out_file) as f:
+        read = json.load(f)
+      self.assertEqual(len(read), 3)
+      self.assertEqual(read[0], {
+          'date': '2000-03-13 20:49:34.515270',
+          'message': 'PTAL',
+          'approval': False,
+          'disapproval': False,
+          'sender': 'owner@example.com'})
+      self.assertEqual(read[1]['date'], '2017-03-13 20:49:34.515270')
+      self.assertEqual(read[2]['date'], '2017-03-13 21:50:34.515270')
+
 
 if __name__ == '__main__':
   logging.basicConfig(