Merge pull request #52 from uber/fix-commenting

Fix regression in posting comments
uber-archive · Jul 28, 2015 · 261f56c · 261f56c
2 parents 93089a9 + 383023a
commit 261f56c
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 27 deletions.
diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorBuildWrapper.java
@@ -100,7 +100,7 @@ public Environment setUp(AbstractBuild build,
 
                 // Post a silent notification if option is enabled
                 if (showBuildStartedMessage) {
-                    diffClient.postComment(diff.getBuildStartedMessage(environment));
+                    diffClient.postComment(diff.getRevisionID(false), diff.getBuildStartedMessage(environment));
                 }
             } catch (ArcanistUsageException e) {
                 logger.println("[arcanist] unable to apply patch");

diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java
@@ -38,8 +38,6 @@
 import hudson.plugins.cobertura.targets.CoverageResult;
 import hudson.tasks.BuildStepMonitor;
 import hudson.tasks.Notifier;
-import net.sf.json.JSONNull;
-import net.sf.json.JSONObject;
 import org.kohsuke.stapler.DataBoundConstructor;
 
 import java.io.IOException;
@@ -177,7 +175,7 @@ public final boolean perform(final AbstractBuild<?, ?> build, final Launcher lau
                 commenter.addBuildLink();
             }
 
-            new PostCommentTask(logger, diffClient, commenter.getComment(), commentAction).run();
+            new PostCommentTask(logger, diffClient, diff.getRevisionID(false), commenter.getComment(), commentAction).run();
         }
 
         return true;

diff --git a/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java b/src/main/java/com/uber/jenkins/phabricator/conduit/DifferentialClient.java
@@ -46,13 +46,14 @@ public DifferentialClient(String diffID, LauncherFactory launcher, String condui
 
     /**
      * Posts a comment to a differential
+     * @param revisionID the revision ID (e.g. "D1234" without the "D")
      * @param message
      * @param silent whether or not to trigger an email
      * @param action phabricator comment action, e.g. 'resign', 'reject', 'none'
      */
-    public JSONObject postComment(String message, boolean silent, String action) throws IOException, InterruptedException, ArcanistUsageException {
+    public JSONObject postComment(String revisionID, String message, boolean silent, String action) throws IOException, InterruptedException, ArcanistUsageException {
         Map params = new HashMap<String, String>();
-        params.put("revision_id", this.diffID);
+        params.put("revision_id", revisionID);
         params.put("action", action);
         params.put("message", message);
         params.put("silent", silent);
@@ -102,14 +103,15 @@ public JSONObject sendHarbormasterMessage(String phid, boolean pass) throws IOEx
 
     /**
      * Post a comment on the differential
+     * @param revisionID the revision ID (e.g. "D1234" without the "D")
      * @param message the string message to post
      * @return
      * @throws IOException
      * @throws InterruptedException
      * @throws ArcanistUsageException
      */
-    public JSONObject postComment(String message) throws IOException, InterruptedException, ArcanistUsageException {
-        return postComment(message, true, "none");
+    public JSONObject postComment(String revisionID, String message) throws IOException, InterruptedException, ArcanistUsageException {
+        return postComment(revisionID, message, true, "none");
     }
 
     protected JSONObject callConduit(String methodName, Map<String, String> params) throws IOException, InterruptedException, ArcanistUsageException {

diff --git a/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java b/src/main/java/com/uber/jenkins/phabricator/tasks/PostCommentTask.java
@@ -36,19 +36,21 @@ public class PostCommentTask extends Task {
     private static final boolean SILENT = false;
     private static final String DEFAULT_COMMENT_ACTION = "none";
 
-    private DifferentialClient differentialClient;
-    private String commentAction;
-    private String comment;
+    private final DifferentialClient differentialClient;
+    private final String revisionID;
+    private final String comment;
+    private final String commentAction;
 
     /**
      * PostCommentTask constructor.
      * @param logger The logger.
      */
     public PostCommentTask(Logger logger, DifferentialClient differentialClient,
-                           String comment, String commentAction) {
+                           String revisionID, String comment, String commentAction) {
         super(logger);
 
         this.differentialClient = differentialClient;
+        this.revisionID = revisionID;
         this.comment = comment;
         this.commentAction = commentAction;
     }
@@ -98,8 +100,8 @@ protected void tearDown() {
 
     private JSONObject postDifferentialComment(String message, boolean silent, String action) {
         try {
-            JSONObject postDifferentialCommentResult = differentialClient.postComment(message,
-                    silent, action);
+            JSONObject postDifferentialCommentResult = differentialClient.postComment(revisionID,
+                    message, silent, action);
             result = Result.SUCCESS;
             return postDifferentialCommentResult;
         } catch (ArcanistUsageException e) {

diff --git a/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java b/src/test/java/com/uber/jenkins/phabricator/conduit/DifferentialClientTest.java
@@ -20,7 +20,6 @@
 
 package com.uber.jenkins.phabricator.conduit;
 
-import com.uber.jenkins.phabricator.LauncherFactory;
 import com.uber.jenkins.phabricator.utils.TestUtils;
 import net.sf.json.JSONObject;
 import org.junit.Before;
@@ -32,7 +31,7 @@
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.anyMap;
 import static org.mockito.Matchers.anyString;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.doReturn;
 
 public class DifferentialClientTest {
 
@@ -50,7 +49,7 @@ public void testPostComment() throws Exception {
 
         mockConduitResponse(differentialClient, sentinel);
 
-        JSONObject response = differentialClient.postComment("hello", true, "none");
+        JSONObject response = differentialClient.postComment("anything", "hello", true, "none");
         assertEquals(sentinel, response);
     }
 
@@ -61,7 +60,7 @@ public void testPostCommentSingleArgument() throws Exception {
 
         mockConduitResponse(differentialClient, sentinel);
 
-        JSONObject response = differentialClient.postComment("hello");
+        JSONObject response = differentialClient.postComment("anything", "hello");
         assertEquals(response, sentinel);
     }
 

diff --git a/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java b/src/test/java/com/uber/jenkins/phabricator/tasks/PostCommentTaskTest.java
@@ -20,19 +20,18 @@
 
 package com.uber.jenkins.phabricator.tasks;
 
-import com.uber.jenkins.phabricator.CodeCoverageMetrics;
 import com.uber.jenkins.phabricator.conduit.ArcanistUsageException;
 import com.uber.jenkins.phabricator.conduit.DifferentialClient;
-import com.uber.jenkins.phabricator.uberalls.UberallsClient;
 import com.uber.jenkins.phabricator.utils.Logger;
 import com.uber.jenkins.phabricator.utils.TestUtils;
 import net.sf.json.JSONObject;
 import org.junit.Before;
 import org.junit.Test;
 
-import static org.junit.Assert.assertEquals;
-import static org.mockito.Matchers.*;
-import static org.mockito.Mockito.*;
+import static org.mockito.Matchers.anyBoolean;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
 
 public class PostCommentTaskTest {
 
@@ -41,6 +40,7 @@ public class PostCommentTaskTest {
 
     private String TEST_COMMENT = "They are selling like hotcakes!";
     private String TEST_COMMENT_ACTION = "none";
+    private String TEST_REVISION_ID = "something";
 
     @Before
     public void setup() {
@@ -51,26 +51,28 @@ public void setup() {
     @Test
     public void testPostDifferentialFailed() throws Exception {
         doThrow(new ArcanistUsageException("")).when(differentialClient).postComment(
+                anyString(),
                 anyString(),
                 anyBoolean(),
                 anyString()
         );
 
-        PostCommentTask postCommentTask = new PostCommentTask(logger, differentialClient, TEST_COMMENT,
-                TEST_COMMENT_ACTION);
+        PostCommentTask postCommentTask = new PostCommentTask(logger, differentialClient,
+                TEST_REVISION_ID, TEST_COMMENT, TEST_COMMENT_ACTION);
         Task.Result result = postCommentTask.run();
         assert result == Task.Result.FAILURE;
     }
 
     @Test
     public void testPostDifferentialSuccess() throws Exception {
         doReturn(new JSONObject()).when(differentialClient).postComment(
+                anyString(),
                 anyString(),
                 anyBoolean(),
                 anyString()
         );
 
-        assert new PostCommentTask(logger, differentialClient, TEST_COMMENT,
-                TEST_COMMENT_ACTION).run() == Task.Result.SUCCESS;
+        assert new PostCommentTask(logger, differentialClient, TEST_REVISION_ID,
+                TEST_COMMENT, TEST_COMMENT_ACTION).run() == Task.Result.SUCCESS;
     }
 }