my_activity.py: Use gerrit new REST API.

my_activity.py

Index: my_activity.py
diff --git a/my_activity.py b/my_activity.py
index 16d3f7dba4770c620b381233818d5651d2bc88e8..4c0fe9d48b64540b994aac556615f1eef4e34380 100755
--- a/my_activity.py
+++ b/my_activity.py
@@ -116,14 +116,22 @@ rietveld_instances = [
 
 gerrit_instances = [
   {
-    'url': 'gerrit.chromium.org',
-    'port': 29418,
+    'url': 'chromium-review.googlesource.com',
     'shorturl': 'crosreview.com',
   },
+  # TODO: chrome-internal-review requires login credentials. Enable once login
+  # support is added to this client. See crbug.com/281695.
+  #{
+  #  'url': 'chrome-internal-review.googlesource.com',
+  #  'shorturl': 'crosreview.com/i',
+  #},
+  {
+    'host': 'gerrit.chromium.org',
+    'port': 29418,
+  },
   {
-    'url': 'gerrit-int.chromium.org',
+    'host': 'gerrit-int.chromium.org',
     'port': 29419,
-    'shorturl': 'crosreview.com/i',
   },
 ]
 
@@ -239,6 +247,10 @@ def get_yes_or_no(msg):
       return False
 
 
+def datetime_from_gerrit(date_string):
+  return datetime.strptime(date_string, '%Y-%m-%d %H:%M:%S.%f000')
+
+
 def datetime_from_rietveld(date_string):
   return datetime.strptime(date_string, '%Y-%m-%d %H:%M:%S.%f')
 
@@ -376,33 +388,69 @@ class MyActivity(object):
       ret.append(r)
     return ret
 
-  def gerrit_search(self, instance, owner=None, reviewer=None):
-    max_age = datetime.today() - self.modified_after
-    max_age = max_age.days * 24 * 3600 + max_age.seconds
-
+  @staticmethod
+  def gerrit_changes_over_ssh(instance, filters):
     # See https://review.openstack.org/Documentation/cmd-query.html
     # Gerrit doesn't allow filtering by created time, only modified time.
-    user_filter = 'owner:%s' % owner if owner else 'reviewer:%s' % reviewer
-    gquery_cmd = ['ssh', '-p', str(instance['port']), instance['url'],
+    gquery_cmd = ['ssh', '-p', str(instance['port']), instance['host'],
                   'gerrit', 'query',
                   '--format', 'JSON',
                   '--comments',
-                  '--',
-                  '-age:%ss' % str(max_age),
-                  user_filter]
+                  '--'] + filters
     [stdout, _] = subprocess.Popen(gquery_cmd, stdout=subprocess.PIPE,

[M-A Ruel, 2013/09/11 13:47:26]

(stdout, _)

[deymo, 2013/09/11 21:14:35]

Done.

                                    stderr=subprocess.PIPE).communicate()

[M-A Ruel, 2013/09/11 13:47:26]

Failure will be hard to handle since you discard both the exit code and stderr.

[deymo, 2013/09/11 21:14:35]

This part of the code is using the old gerrit interface and was like that on the
codebase. I think the idea is to ignore errors on my_activity.py (for example,
if you don't use gerrit and don't have permission or a user to access it). I'd
keep this as it is.

     issues = str(stdout).split('\n')[:-2]

[M-A Ruel, 2013/09/11 13:47:26]

splitlines()

stdout is already a str(), not need to convert explicitly.

[deymo, 2013/09/11 21:14:35]

Done.

-    issues = map(json.loads, issues)
+    return map(json.loads, issues)
+
+  @staticmethod
+  def gerrit_changes_over_rest(instance, filters):
+    # See https://gerrit-review.googlesource.com/Documentation/rest-api.html
+    # Gerrit doesn't allow filtering by created time, only modified time.
+    args = urllib.urlencode([
+        ('q', ' '.join(filters)),
+        ('o', 'MESSAGES'),
+        ('o', 'LABELS')])
+    rest_url = 'https://%s/changes/?%s' % (instance['url'], args)
+
+    req = urllib2.Request(rest_url, headers={'Accept': 'text/plain'})
+    try:
+      response = urllib2.urlopen(req)
+      stdout = response.read()
+    except urllib2.HTTPError, e:
+      print 'ERROR: Looking up %r: %s' % (rest_url, e)
+      return []
+
+    # Check that the returned JSON starts with the right marker.

[M-A Ruel, 2013/09/11 13:47:26]

s/starts/ends/

[deymo, 2013/09/11 21:14:35]

hmmm... I think that "starts" is the right option here.

+    if stdout[:5] != ")]}'\n":
+      print 'ERROR: Marker not found on REST API response: %r' % stdout[:5]
+      return []
+    return json.loads(stdout[5:])

[M-A Ruel, 2013/09/11 13:47:26]

What are you skipping exactly?

[deymo, 2013/09/11 21:14:35]

I'm skipping the string ")]}'\n" from the beginning of the HTTP response and
checking that the skipped part is what I'm expecting to be (in the if statement
right before this return).

The REST API responds with this 5 chars and then a totally valid JSON string. I
don't know why, but that's what the examples on the documentation show (and what
the server is actually sending).

+
+  def gerrit_search(self, instance, owner=None, reviewer=None):
+    max_age = datetime.today() - self.modified_after
+    max_age = max_age.days * 24 * 3600 + max_age.seconds
+    user_filter = 'owner:%s' % owner if owner else 'reviewer:%s' % reviewer
+    filters = ['-age:%ss' % max_age, user_filter]
+
+    # Determine the gerrit interface to use: SSH or REST API:
+    if 'host' in instance:
+      issues = self.gerrit_changes_over_ssh(instance, filters)
+      issues = [self.process_gerrit_ssh_issue(instance, issue)
+          for issue in issues]
+    elif 'url' in instance:
+      issues = self.gerrit_changes_over_rest(instance, filters)
+      issues = [self.process_gerrit_rest_issue(instance, issue)
+          for issue in issues]
+    else:
+      raise Exception("Invalid gerrit_instances configuration.")

[M-A Ruel, 2013/09/11 13:47:26]

resto f the file use single quotes, stay consistent.

[deymo, 2013/09/11 21:14:35]

Done.

 
     # TODO(cjhopman): should we filter abandoned changes?
-    issues = [self.process_gerrit_issue(instance, issue) for issue in issues]
     issues = filter(self.filter_issue, issues)
     issues = sorted(issues, key=lambda i: i['modified'], reverse=True)
 
     return issues
 
-  def process_gerrit_issue(self, instance, issue):
+  def process_gerrit_ssh_issue(self, instance, issue):
     ret = {}
     ret['review_url'] = issue['url']
     if 'shorturl' in instance:
@@ -414,7 +462,7 @@ class MyActivity(object):
     ret['created'] = datetime.fromtimestamp(issue['createdOn'])
     ret['modified'] = datetime.fromtimestamp(issue['lastUpdated'])
     if 'comments' in issue:
-      ret['replies'] = self.process_gerrit_issue_replies(issue['comments'])
+      ret['replies'] = self.process_gerrit_ssh_issue_replies(issue['comments'])
     else:
       ret['replies'] = []
     ret['reviewers'] = set()
@@ -424,7 +472,7 @@ class MyActivity(object):
     return ret
 
   @staticmethod
-  def process_gerrit_issue_replies(replies):
+  def process_gerrit_ssh_issue_replies(replies):
     ret = []
     replies = filter(lambda r: 'email' in r['reviewer'], replies)
     for reply in replies:
@@ -435,6 +483,40 @@ class MyActivity(object):
       ret.append(r)
     return ret
 
+  def process_gerrit_rest_issue(self, instance, issue):
+    ret = {}
+    ret['review_url'] = 'https://%s/%s' % (instance['url'], issue['_number'])
+    if 'shorturl' in instance:
+      # TODO: Move this short link to https once crosreview.com supports it.

[M-A Ruel, 2013/09/11 13:47:26]

TODO(deymo)

[deymo, 2013/09/11 21:14:35]

Done.

+      ret['review_url'] = 'http://%s/%s' % (instance['shorturl'],
+                                            issue['_number'])
+    ret['header'] = issue['subject']
+    ret['owner'] = issue['owner']['email']
+    ret['author'] = ret['owner']
+    ret['created'] = datetime_from_gerrit(issue['created'])
+    ret['modified'] = datetime_from_gerrit(issue['updated'])
+    if 'messages' in issue:
+      ret['replies'] = self.process_gerrit_rest_issue_replies(issue['messages'])
+    else:
+      ret['replies'] = []
+    ret['reviewers'] = set()

[M-A Ruel, 2013/09/11 13:47:26]

(optional) Could be done as a one liner: 
ret['reviewers'] = set(
    r['author'] for r in ret['replies'] if r['author'] != ret['author'])

or:
ret['reviewers'] = set(r['author'] for r in ret['replies'])
ret['reviewers'].discard(ret['author'])

[deymo, 2013/09/11 21:14:35]

Changed in both functions.

On 2013/09/11 13:47:26, M-A Ruel wrote:
Done.

+    for reply in ret['replies']:
+      if reply['author'] != ret['author']:
+        ret['reviewers'].add(reply['author'])
+    return ret
+
+  @staticmethod
+  def process_gerrit_rest_issue_replies(replies):
+    ret = []
+    replies = filter(lambda r: 'email' in r['author'], replies)
+    for reply in replies:
+      r = {}

[M-A Ruel, 2013/09/11 13:47:26]

ret.append(
    {
      'author': reply['author']['email'],
      ...
    })

or better and shorter:
return [
  {
    'author': reply['author']['email'],
    ...
  }
  for reply in replies
]

[deymo, 2013/09/11 21:14:35]

This code is essentially the same as the previously existing
process_gerrit_issue_replies. I replaced both, the new and the old function.

On 2013/09/11 13:47:26, M-A Ruel wrote:

+      r['author'] = reply['author']['email']
+      r['created'] = datetime_from_gerrit(reply['date'])
+      r['content'] = reply['message']
+      ret.append(r)
+    return ret
+
   def google_code_issue_search(self, instance):
     time_format = '%Y-%m-%dT%T'
     # See http://code.google.com/p/support/wiki/IssueTrackerAPI