From fd55ad0ef712d07c385df8f35fce9b7d36499801 Mon Sep 17 00:00:00 2001
From: Prem Sichanugrist <prem@thoughtbot.com>
Date: Wed, 18 Mar 2015 15:40:11 -0400
Subject: [PATCH] Do not send status update if user disable it

We forgot to check if user has turned on the `update_status` flag or not
before we sends a pending status to GitHub.

This commit includes refactoring, hopefully to cleanup the confusion on
conditionals.
---
 lib/cc/services/github_pull_requests.rb | 80 ++++++++++++-------------
 test/github_pull_requests_test.rb       | 42 ++++++++++++-
 2 files changed, 79 insertions(+), 43 deletions(-)

diff --git a/lib/cc/services/github_pull_requests.rb b/lib/cc/services/github_pull_requests.rb
index 74a1ab7..f2c2b27 100644
--- a/lib/cc/services/github_pull_requests.rb
+++ b/lib/cc/services/github_pull_requests.rb
@@ -40,30 +40,15 @@ def receive_test
 
   def receive_pull_request
     setup_http
+    state = @payload["state"]
 
-    case @payload["state"]
-    when "pending"
-      update_status_pending
-    when "success"
-      if config.update_status && config.add_comment
-        update_status_success
-        add_comment
-      elsif config.update_status
-        update_status_success
-      elsif config.add_comment
-        add_comment
-      else
-        simple_failure("Nothing happened")
-      end
-    when "skipped"
-      if config.update_status
-        update_status_skipped
-      end
-    when "error"
-      update_status_error
+    if %w(pending success skipped error).include?(state)
+      send("update_status_#{state}")
     else
-      simple_failure("Unknown state")
+      @response = simple_failure("Unknown state")
     end
+
+    response
   end
 
 private
@@ -72,19 +57,27 @@ def simple_failure(message)
     { ok: false, message: message }
   end
 
+  def response
+    @response || simple_failure("Nothing happened")
+  end
+
   def update_status_skipped
-    update_status("success", "Code Climate has skipped analysis of this commit.")
+    update_status(
+      "success",
+      "Code Climate has skipped analysis of this commit."
+    )
   end
 
   def update_status_success
     update_status("success", "Code Climate has analyzed this pull request.")
+    add_comment
   end
 
   def update_status_error
     update_status(
       "error",
       "Code Climate encountered an error while attempting to analyze this " +
-      "pull request."
+        "pull request."
     )
   end
 
@@ -93,27 +86,34 @@ def update_status_pending
   end
 
   def update_status(state, description)
-    params = {
-      state:       state,
-      description: description,
-      target_url:  @payload["details_url"],
-      context:     "codeclimate"
-    }
-    service_post(status_url, params.to_json)
+    if config.update_status
+      params = {
+        state:       state,
+        description: description,
+        target_url:  @payload["details_url"],
+        context:     "codeclimate"
+      }
+      @response = service_post(status_url, params.to_json)
+    end
   end
 
   def add_comment
-    if !comment_present?
-      body = {
-        body: COMMENT_BODY % @payload["compare_url"]
-      }.to_json
-
-      service_post(comments_url, body) do |response|
-        doc = JSON.parse(response.body)
-        { id: doc["id"] }
+    if config.add_comment
+      if !comment_present?
+        body = {
+          body: COMMENT_BODY % @payload["compare_url"]
+        }.to_json
+
+        @response = service_post(comments_url, body) do |response|
+          doc = JSON.parse(response.body)
+          { id: doc["id"] }
+        end
+      else
+        @response = {
+          ok: false,
+          message: "Comment already present"
+        }
       end
-    else
-      { ok: false, message: "Comment already present" }
     end
   end
 
diff --git a/test/github_pull_requests_test.rb b/test/github_pull_requests_test.rb
index 027a694..ecbe552 100644
--- a/test/github_pull_requests_test.rb
+++ b/test/github_pull_requests_test.rb
@@ -63,6 +63,27 @@ def test_no_status_update_for_skips_when_update_status_config_is_falsey
     })
   end
 
+  def test_no_status_update_for_pending_when_update_status_config_is_falsey
+    # With no POST expectation, test will fail if request is made.
+
+    receive_pull_request({}, {
+      github_slug: "pbrisbin/foo",
+      commit_sha:  "abc123",
+      state:       "pending",
+    })
+  end
+
+  def test_no_status_update_for_error_when_update_status_config_is_falsey
+    # With no POST expectation, test will fail if request is made.
+
+    receive_pull_request({}, {
+      github_slug: "pbrisbin/foo",
+      commit_sha:  "abc123",
+      state:       "error",
+    })
+  end
+
+
   def test_no_comment_for_skips_regardless_of_add_comment_config
     # With no POST expectation, test will fail if request is made.
 
@@ -147,13 +168,28 @@ def test_pull_request_comment_already_present
 
     # With no POST expectation, test will fail if request is made.
 
-    rsp = receive_pull_request({ add_comment: true, update_status: false }, {
+    response = receive_pull_request({
+      add_comment: true,
+      update_status: false
+    }, {
       github_slug: "pbrisbin/foo",
       number:      1,
       state:       "success",
     })
-    assert_not_nil rsp
-    assert_false rsp[:ok]
+
+    assert_equal({ ok: false, message: "Comment already present" }, response)
+  end
+
+  def test_pull_request_unknown_state
+    response = receive_pull_request({}, { state: "unknown" })
+
+    assert_equal({ ok: false, message: "Unknown state" }, response)
+  end
+
+  def test_pull_request_nothing_happened
+    response = receive_pull_request({}, { state: "success" })
+
+    assert_equal({ ok: false, message: "Nothing happened" }, response)
   end
 
 private