Report Phabrictor revision URL to Analytics

Summary:
This makes it much easier for builds to be grouped by the actual change and for
commit builds to be correlated with diff builds in Analytics.

Test Plan: Unit

Reviewers: jukka

Reviewed By: jukka

Subscribers: changesbot, wwu

Differential Revision: https://tails.corp.dropbox.com/D97125

changes/listeners/analytics_notifier.py

@@ -7,6 +7,7 @@
 
 import logging
 import json
+import re
 import requests
 from datetime import datetime
 
@@ -22,6 +23,30 @@ def _datetime_to_timestamp(dt):
     return int((dt - datetime.utcfromtimestamp(0)).total_seconds())
 
 
+_REV_URL_RE = re.compile(r'^\s*Differential Revision:\s+(http.*/D[0-9]+)\s*$', re.MULTILINE)
+
+
+def _get_phabricator_revision_url(build):
+    """Returns the Phabricator Revision URL for a Build.
+
+    Args:
+      build (Build): The Build.
+
+    Returns:
+      A str with the Phabricator Revision URL, or None if we couldn't find one (or found
+      multiple).
+    """
+    source_data = build.source.data or {}
+    rev_url = source_data.get('phabricator.revisionURL')
+    if rev_url:
+        return rev_url
+    matches = _REV_URL_RE.findall(build.message)
+    # only return if there's a clear choice.
+    if matches and len(matches) == 1:
+        return matches[0]
+    return None
+
+
 def build_finished_handler(build_id, **kwargs):
     url = current_app.config.get('ANALYTICS_POST_URL')
     if not url:
@@ -47,6 +72,10 @@ def maybe_ts(dt):
         'date_created': maybe_ts(build.date_created),
         'date_started': maybe_ts(build.date_started),
         'date_finished': maybe_ts(build.date_finished),
+        # Revision URL rather than just revision id because the URL should
+        # be globally unique, whereas the id is only certain to be unique for
+        # a single Phabricator instance.
+        'phab_revision_url': _get_phabricator_revision_url(build),
     }
     if build.author:
         data['author'] = build.author.email

tests/changes/listeners/test_analytics_notifier.py

@@ -7,7 +7,7 @@
 
 from changes.constants import Result
 from changes.testutils import TestCase
-from changes.listeners.analytics_notifier import build_finished_handler
+from changes.listeners.analytics_notifier import build_finished_handler, _get_phabricator_revision_url
 
 
 class AnalyticsNotifierTest(TestCase):
@@ -44,7 +44,9 @@ def ts_to_datetime(ts):
                                   label='Some sweet diff', duration=duration,
                                   date_created=ts_to_datetime(created), date_started=ts_to_datetime(started),
                                   date_finished=ts_to_datetime(finished))
-        build_finished_handler(build_id=build.id.hex)
+        with mock.patch('changes.listeners.analytics_notifier._get_phabricator_revision_url') as mock_get_phab:
+            mock_get_phab.return_value = 'https://example.com/D1'
+            build_finished_handler(build_id=build.id.hex)
 
         expected_data = {
             'build_id': build.id.hex,
@@ -58,5 +60,61 @@ def ts_to_datetime(ts):
             'date_created': created,
             'date_started': started,
             'date_finished': finished,
+            'phab_revision_url': 'https://example.com/D1',
         }
         post_fn.assert_called_once_with(URL, expected_data)
+
+    def test_get_phab_revision_url_diff(self):
+        project = self.create_project(name='test', slug='test')
+        source_data = {'phabricator.revisionURL': 'https://tails.corp.dropbox.com/D6789'}
+        source = self.create_source(project, data=source_data)
+        build = self.create_build(project, result=Result.failed, source=source, message='Some commit')
+        self.assertEquals(_get_phabricator_revision_url(build), 'https://tails.corp.dropbox.com/D6789')
+
+    def test_get_phab_revision_url_commit(self):
+        project = self.create_project(name='test', slug='test')
+        source_data = {}
+        source = self.create_source(project, data=source_data)
+        msg = """
+        Some fancy commit.
+
+        Summary: Fixes T33417.
+
+        Test Plan: Added tests.
+
+        Reviewers: mickey
+
+        Reviewed By: mickey
+
+        Subscribers: changesbot
+
+        Maniphest Tasks: T33417
+
+        Differential Revision: https://tails.corp.dropbox.com/D6789"""
+        build = self.create_build(project, result=Result.failed, source=source, message=msg)
+        self.assertEquals(_get_phabricator_revision_url(build), 'https://tails.corp.dropbox.com/D6789')
+
+    def test_get_phab_revision_url_commit_conflict(self):
+        project = self.create_project(name='test', slug='test')
+        source_data = {}
+        source = self.create_source(project, data=source_data)
+        msg = """
+        Some fancy commit.
+
+        Summary: Fixes T33417.
+          Adds messages like:
+             Differential Revision: https://tails.corp.dropbox.com/D1234
+
+        Test Plan: Added tests.
+
+        Reviewers: mickey
+
+        Reviewed By: mickey
+
+        Subscribers: changesbot
+
+        Maniphest Tasks: T33417
+
+        Differential Revision: https://tails.corp.dropbox.com/D6789"""
+        build = self.create_build(project, result=Result.failed, source=source, message=msg)
+        self.assertEquals(_get_phabricator_revision_url(build), None)