Response 2xx even if executor fails

ikikko · Feb 7, 2015 · ea06ef8 · ea06ef8
1 parent f29d99f
commit ea06ef8
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 17 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/typetalk/webhookaction/ResponseParameter.java b/src/main/java/org/jenkinsci/plugins/typetalk/webhookaction/ResponseParameter.java
@@ -23,10 +23,10 @@ public String getMessage() {
         return message;
     }
 
-    private TypetalkMessage.Emoji emoji = TypetalkMessage.Emoji.SMILEY;
+    private TypetalkMessage.Emoji emoji;
 
     public TypetalkMessage.Emoji getEmoji() {
-        return emoji;
+        return emoji != null ? emoji : TypetalkMessage.Emoji.SMILEY;
     }
 
     public void setEmoji(TypetalkMessage.Emoji emoji) {

diff --git a/src/main/java/org/jenkinsci/plugins/typetalk/webhookaction/WebhookExecutor.java b/src/main/java/org/jenkinsci/plugins/typetalk/webhookaction/WebhookExecutor.java
@@ -26,23 +26,20 @@ protected WebhookExecutor(WebhookRequest req, StaplerResponse rsp, String comman
     public abstract void execute();
 
     protected void output(ResponseParameter parameter) {
-        outputInternal(Level.INFO, HttpServletResponse.SC_OK, parameter);
+        outputInternal(Level.INFO, parameter);
     }
 
     protected void outputError(ResponseParameter parameter) {
-        outputError(parameter, HttpServletResponse.SC_BAD_REQUEST);
+        parameter.setEmoji(TypetalkMessage.Emoji.CRY);
+        outputInternal(Level.WARNING, parameter);
     }
 
-    protected void outputError(ResponseParameter parameter, int status) {
-        outputInternal(Level.WARNING, status, parameter);
-    }
-
-    private void outputInternal(Level level, int status, ResponseParameter parameter) {
+    private void outputInternal(Level level, ResponseParameter parameter) {
         try {
             logger.log(level, parameter.getDescription());
 
             rsp.setContentType("application/json");
-            rsp.setStatus(status);
+            rsp.setStatus(HttpServletResponse.SC_OK);
             rsp.getWriter().println(buildResponseMessage(parameter));
         } catch (IOException e) {
             throw new IllegalStateException(e);

diff --git a/src/main/java/org/jenkinsci/plugins/typetalk/webhookaction/executorimpl/BuildExecutor.java b/src/main/java/org/jenkinsci/plugins/typetalk/webhookaction/executorimpl/BuildExecutor.java
@@ -7,7 +7,6 @@
 import org.jenkinsci.plugins.typetalk.webhookaction.WebhookRequest;
 import org.kohsuke.stapler.StaplerResponse;
 
-import javax.servlet.http.HttpServletResponse;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -54,7 +53,7 @@ public void execute() {
 
         if (!project.hasPermission(Item.BUILD)) {
             String name = Jenkins.getAuthentication().getName();
-            outputError(new ResponseParameter(String.format("Project [ %s ] cannot be built by '%s'", job, name)), HttpServletResponse.SC_FORBIDDEN);
+            outputError(new ResponseParameter(String.format("Project [ %s ] cannot be built by '%s'", job, name)));
             return;
         }
 

diff --git a/...groovy/org/jenkinsci/plugins/typetalk/webhookaction/executorimpl/BuildExecutorSpec.groovy b/...groovy/org/jenkinsci/plugins/typetalk/webhookaction/executorimpl/BuildExecutorSpec.groovy
@@ -46,7 +46,8 @@ class BuildExecutorSpec extends Specification {
         executor.execute()
 
         then:
-        1 * res.setStatus(HttpServletResponse.SC_BAD_REQUEST)
+        1 * res.setStatus(HttpServletResponse.SC_OK)
+        !isBuilt()
    }
 
     def "execute : no parameter"() {
@@ -57,9 +58,11 @@ class BuildExecutorSpec extends Specification {
 
         when:
         executor.execute()
+        buildStarted.block()
 
         then:
         1 * res.setStatus(HttpServletResponse.SC_OK)
+        isBuilt()
     }
 
     def "execute : parameter without key"() {
@@ -182,17 +185,19 @@ class BuildExecutorSpec extends Specification {
 
         when:
         executor.execute()
+        if (isBuilt) buildStarted.block()
 
         then:
-        1 * res.setStatus(result)
+        1 * res.setStatus(HttpServletResponse.SC_OK)
+        isBuilt() == isBuilt
 
         cleanup:
         SecurityContextHolder.setContext(old);
 
         where:
-        username         || result
-        "authorized"     || HttpServletResponse.SC_OK
-        "not authorized" || HttpServletResponse.SC_FORBIDDEN
+        username         || isBuilt
+        "authorized"     || true
+        "not authorized" || false
     }
 
     // --- helper method ---
@@ -215,6 +220,10 @@ class BuildExecutorSpec extends Specification {
         })
     }
 
+    def isBuilt() {
+        project?.lastBuild != null
+    }
+
     def getBuildParameter(key) {
         project.lastBuild.getAction(ParametersAction.class).getParameter(key).value
     }