Update formatting to be more readable in email

Summary:
The `{icon}` formatting uses FontAwesome web fonts and CSS, which don't
get rendered in email.  Switch to splitting the sections up more
visually using `NOTE` and `IMPORTANT` to give blue/red banners for the
passing/failing test sections.

Test Plan: Existing tests

Reviewers: anupc, josiah

Reviewed By: josiah

Subscribers: tony, changesbot, kylec

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

changes/listeners/phabricator_listener.py

@@ -193,25 +193,27 @@ def build_finished_handler(build_id, **kwargs):
 
     context = build_context_lib.get_collection_context(builds)
 
-    message = '\n\n'.join([_get_message_for_build_context(x) for x in context.builds])
+    good_builds = [b for b in context.builds if b['build'].result == Result.passed]
+    bad_builds = [b for b in context.builds if b['build'].result != Result.passed]
+
+    message = ""
+    if bad_builds:
+        message += '(IMPORTANT) Failing builds!\n'
+        message += '\n'.join([_get_message_for_build_context(x) for x in bad_builds])
+    if good_builds:
+        if bad_builds:
+            message += '\n\n'
+        message += '(NOTE) Passing builds:\n'
+        message += '\n'.join([_get_message_for_build_context(x) for x in good_builds])
 
     post_comment(target, message, phab)
 
 
 def _get_message_for_build_context(build_context):
     build = build_context['build']
-    result = build.result
-    if result == Result.passed:
-        result_image = '{icon check, color=green}'
-    elif result == Result.failed:
-        result_image = '{icon times, color=red}'
-    else:
-        result_image = '{icon question, color=orange}'
     safe_slug = urllib.quote_plus(build.project.slug)
-    message = u'{project} build {result} {image} ([results]({link})).'.format(
+    message = u' - [{project}]({link}).'.format(
         project=build.project.name,
-        image=result_image,
-        result=unicode(build.result),
         link=build_web_uri('/find_build/{0}/'.format(build.id.hex))
     )
 

tests/changes/listeners/test_phabricator.py

@@ -74,7 +74,7 @@ def test_whitelisted_project(self, get_options, post):
 
         get_options.assert_called_once_with(project.id)
         build_link = build_web_uri('/find_build/{0}/'.format(build.id.hex))
-        expected_msg = "test build Failed {{icon times, color=red}} ([results]({0})).".format(
+        expected_msg = "(IMPORTANT) Failing builds!\n - [test]({0}).".format(
             build_link
         )
 
@@ -113,7 +113,7 @@ def test_build_failure_with_tests_and_no_base_build(self, get_options, post):
             testcase.id.hex
         ))
         test_desc = "[test_foo](%s)" % test_link
-        expected_msg = """Server build Failed {{icon times, color=red}} ([results]({0})). There were a total of 1 [test failures]({1}), but we could not determine if any of these tests were previously failing.
+        expected_msg = """(IMPORTANT) Failing builds!\n - [Server]({0}). There were a total of 1 [test failures]({1}), but we could not determine if any of these tests were previously failing.
 
 **All failures (1):**
 |Test Name | Package|
@@ -159,7 +159,7 @@ def test_build_failure_with_tests_and_no_base_job(self, get_options, post):
             testcase.id.hex,
         ))
         test_desc = "[test_foo](%s)" % test_link
-        expected_msg = """Server build Failed {{icon times, color=red}} ([results]({0})). There were a total of 1 [test failures]({1}), but we could not determine if any of these tests were previously failing.
+        expected_msg = """(IMPORTANT) Failing builds!\n - [Server]({0}). There were a total of 1 [test failures]({1}), but we could not determine if any of these tests were previously failing.
 
 **All failures (1):**
 |Test Name | Package|
@@ -204,7 +204,7 @@ def test_build_failure_with_tests(self, get_options, post):
             testcase.id.hex,
         ))
         test_desc = "[test_foo](%s)" % test_link
-        expected_msg = """Server build Failed {{icon times, color=red}} ([results]({0})). There were 1 new [test failures]({1})
+        expected_msg = """(IMPORTANT) Failing builds!\n - [Server]({0}). There were 1 new [test failures]({1})
 
 **New failures (1):**
 |Test Name | Package|
@@ -246,7 +246,7 @@ def test_no_new_failures(self, get_options, post, get_base_failures):
             testcase.id.hex,
         ))
         test_desc = "[test_foo](%s)" % test_link
-        expected_msg = """Server build Failed {{icon times, color=red}} ([results]({0})). There were 0 new [test failures]({1})
+        expected_msg = """(IMPORTANT) Failing builds!\n - [Server]({0}). There were 0 new [test failures]({1})
 
 **Failures in parent revision (1):**
 |Test Name | Package|
@@ -298,7 +298,7 @@ def get_test_desc(build, testcase, test_name):
 
         test_desc = get_test_desc(build, testcase, 'test_foo')
         test_desc2 = get_test_desc(build, testcase2, 'test_foo2')
-        expected_msg = """Server build Failed {{icon times, color=red}} ([results]({0})). There were 1 new [test failures]({1})
+        expected_msg = """(IMPORTANT) Failing builds!\n - [Server]({0}). There were 1 new [test failures]({1})
 
 **New failures (1):**
 |Test Name | Package|
@@ -354,7 +354,7 @@ def test_max_shown_build_failures(self, get_options, post):
             if test_link in comment:
                 shown_test_count += 1
         assert shown_test_count == max_shown
-        assert 'Server build Failed {{icon times, color=red}} ([results]({0})). There were {2} new [test failures]({1})'.format(build_link, failure_link, total_test_count)
+        assert '[Server]({0}). There were a total of {2} [test failures]({1})'.format(build_link, failure_link, total_test_count) in comment
         assert '|...more...|...|' in comment
 
     @mock.patch('changes.utils.phabricator_utils.post_diff_comment')
@@ -402,14 +402,15 @@ def create_build(result, project):
             testcase1.id.hex,
         ))
         test_desc = "[test_foo](%s)" % test_link
-        expected_msg = """Server build Failed {{icon times, color=red}} ([results]({0})). There were 1 new [test failures]({1})
+        expected_msg = """(IMPORTANT) Failing builds!\n - [Server]({0}). There were 1 new [test failures]({1})
 
 **New failures (1):**
 |Test Name | Package|
 |--|--|
 |{2}|test.group.ClassName|
 
-Server2 build Passed {{icon check, color=green}} ([results]({3}))."""
+(NOTE) Passing builds:
+ - [Server2]({3})."""
 
         post.assert_called_once_with('1', expected_msg.format(build_link, failure_link, test_desc, build2_link), mock.ANY)
 
@@ -511,7 +512,7 @@ def test_slug_escape(self, get_options, post):
         get_options.assert_called_once_with(project.id)
         build_link = build_web_uri('/find_build/{0}/'.format(build.id.hex))
 
-        expected_msg = 'Server build Passed {{icon check, color=green}} ([results]({0})).'
+        expected_msg = '(NOTE) Passing builds:\n - [Server]({0}).'
         post.assert_called_once_with('1', expected_msg.format(build_link), mock.ANY)
 
     @mock.patch('changes.utils.phabricator_utils.post_diff_comment')