Merge pull request #74 from uber/escape-summary

Fix escaping on Summary Action for builds
uber-archive · Aug 13, 2015 · 01f1435 · 01f1435
2 parents e1403cf + ea68133
commit 01f1435
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 43 deletions.
diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorPostbuildSummaryAction.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorPostbuildSummaryAction.java
@@ -30,10 +30,17 @@
 @ExportedBean(defaultVisibility = 2)
 public class PhabricatorPostbuildSummaryAction implements Action {
     private final String iconPath;
-    private final StringBuilder textBuilder = new StringBuilder();
+    private final String url;
+    private final String revisionID;
+    private final String authorName;
+    private final String authorEmail;
 
-    public PhabricatorPostbuildSummaryAction(String iconPath) {
-        this.iconPath = PhabricatorPlugin.getIconPath(iconPath);
+    public PhabricatorPostbuildSummaryAction(String iconPath, String phabricatorLink, String revisionID, String authorName, String authorEmail) {
+        this.iconPath = iconPath;
+        this.url = phabricatorLink;
+        this.revisionID = revisionID;
+        this.authorName = authorName;
+        this.authorEmail = authorEmail;
     }
 
     /* Action methods */
@@ -50,14 +57,22 @@ public String getIconFileName() {
     }
 
     @Exported public String getIconPath() {
-        return iconPath;
+        return PhabricatorPlugin.getIconPath(iconPath);
     }
 
-    @Exported public String getText() {
-        return textBuilder.toString();
+    @Exported public String getUrl() {
+        return url;
     }
 
-    public void appendText(String text) {
-        textBuilder.append(text);
+    @Exported public String getRevisionID() {
+        return revisionID;
+    }
+
+    @Exported public String getAuthorName() {
+        return authorName;
+    }
+
+    @Exported public String getAuthorEmail() {
+        return authorEmail;
     }
 }
diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java b/src/main/java/com/uber/jenkins/phabricator/conduit/Differential.java
@@ -48,18 +48,6 @@ public String getRevisionID(boolean formatted) {
         return rawRevisionId;
     }
 
-    /**
-     * Get the summary message
-     * @param phabricatorURL the root of the Phabricator URL for hyperlinks
-     * @return the summary message to display
-     */
-    public String getSummaryMessage(String phabricatorURL) {
-        return String.format("This was a build of <a href=\"%s\">%s</a> by %s &lt;%s&gt;",
-                this.getPhabricatorLink(phabricatorURL),
-                this.getRevisionID(true),
-                this.rawJSON.get("authorName"), this.rawJSON.get("authorEmail"));
-    }
-
     public String getPhabricatorLink(String phabricatorURL) {
         String revisionID = this.getRevisionID(true);
         try {
@@ -76,13 +64,19 @@ public void decorate(AbstractBuild build, String phabricatorURL) {
                 this.getRevisionID(true),
                 this.getPhabricatorLink(phabricatorURL)));
         // Add some long-form text
-        this.createSummary(build).appendText(this.getSummaryMessage(phabricatorURL));
+        PhabricatorPostbuildSummaryAction summary = createSummary(phabricatorURL);
+        build.addAction(summary);
+
     }
 
-    private PhabricatorPostbuildSummaryAction createSummary(final AbstractBuild build) {
-        PhabricatorPostbuildSummaryAction action = new PhabricatorPostbuildSummaryAction("phabricator.png");
-        build.addAction(action);
-        return action;
+    public PhabricatorPostbuildSummaryAction createSummary(String phabricatorURL) {
+        return new PhabricatorPostbuildSummaryAction(
+                "phabricator.png",
+                getPhabricatorLink(phabricatorURL),
+                getRevisionID(true),
+                rawJSON.getString("authorName"),
+                rawJSON.getString("authorEmail")
+        );
     }
 
     /**

diff --git a/src/main/resources/com/uber/jenkins/phabricator/PhabricatorNotifier/config.jelly b/src/main/resources/com/uber/jenkins/phabricator/PhabricatorNotifier/config.jelly
@@ -1,5 +1,5 @@
 <?jelly escape-by-default='true'?>
-<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
   <!--
     This jelly script is used for per-project configuration.
 

diff --git a/src/main/resources/com/uber/jenkins/phabricator/PhabricatorPostbuildAction/badge.jelly b/src/main/resources/com/uber/jenkins/phabricator/PhabricatorPostbuildAction/badge.jelly
@@ -1,5 +1,5 @@
 <?jelly escape-by-default='true'?>
-<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+<j:jelly xmlns:j="jelly:core">
     <j:if test="${it.iconPath != null &amp;&amp; it.link == null}">
         <img src="${rootURL}${it.iconPath}" width="16" height="16" alt="${it.text}" title="${it.text}" onclick="javascript:alert('${it.text}')" />
     </j:if>

diff --git a/...in/resources/com/uber/jenkins/phabricator/PhabricatorPostbuildSummaryAction/summary.jelly b/...in/resources/com/uber/jenkins/phabricator/PhabricatorPostbuildSummaryAction/summary.jelly
@@ -1,13 +1,6 @@
 <?jelly escape-by-default='true'?>
-<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
-  <j:if test="${it.iconPath != null}">
-    <t:summary icon="${it.iconPath}">
-      ${it.text}
-    </t:summary>
-  </j:if>
-  <j:if test="${it.iconPath == null}">
-    <div>
-      ${it.text}
-    </div>
-  </j:if>
+<j:jelly xmlns:j="jelly:core" xmlns:t="/lib/hudson">
+  <t:summary icon="${it.iconPath}">
+    This was a build of <a href="${it.url}">${it.revisionID}</a> by ${it.authorName} (${it.authorEmail})
+  </t:summary>
 </j:jelly>
diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialTest.java
@@ -20,6 +20,7 @@
 
 package com.uber.jenkins.phabricator.conduit;
 
+import com.uber.jenkins.phabricator.PhabricatorPostbuildSummaryAction;
 import com.uber.jenkins.phabricator.utils.TestUtils;
 import hudson.EnvVars;
 import junit.framework.TestCase;
@@ -87,10 +88,11 @@ public void testGetBaseCommit() throws Exception {
     }
 
     @Test
-    public  void testGetSummaryMessage() throws Exception {
-        String message = differential.getSummaryMessage("http://example.com");
-        assertTrue(message.contains("ai@uber.com"));
-        assertTrue(message.contains("http://example.com"));
-        assertTrue(message.contains("aiden"));
+    public void testGetSummaryMessage() throws Exception {
+        PhabricatorPostbuildSummaryAction summary = differential.createSummary("http://example.com");
+        assertEquals("http://example.com/Dnot-a-real-id", summary.getUrl());
+        assertEquals("Dnot-a-real-id", summary.getRevisionID());
+        assertEquals("aiden", summary.getAuthorName());
+        assertEquals("ai@uber.com", summary.getAuthorEmail());
     }
 }