Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.

Do not send status update if user disable it #57

Merged
merged 1 commit into from
Mar 19, 2015
Merged

Do not send status update if user disable it #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 40 additions & 40 deletions lib/cc/services/github_pull_requests.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,30 +40,15 @@ def receive_test

def receive_pull_request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is getting pretty long and hard to read. Ideas on breaking it down? De-compose the method?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@noahd1 - I think the new #receive_pull_request is easier to understand. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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
Expand All @@ -72,19 +57,27 @@ def simple_failure(message)
{ ok: false, message: message }
end

def response
@response || simple_failure("Nothing happened")
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goodbye cruel conditionals!

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in love with having status mixed in with commenting, but it's a larger issue with this class in general (that it does two things). Commenting should go away soon, so I'm ok with this.

However -- I think we swap the order of these lines for now. We're not capturing both results in the response back (just the last one), but the PR one is the more important one right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This class is doing too much.

I'm fine with the swap, but that means we're actually change the functionality of this (and its return value). Currently, we return the status of add comment only. Do you think we should swap it anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. Perhaps a separate PR then is appropriate for this change. I think it's in scope however, because we're now capturing the result of these calls -- and the most important result is the one that comes back from posting a status to a PR (not the comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Let's do that swap in another PR. 🥚

end

def update_status_error
update_status(
"error",
"Code Climate encountered an error while attempting to analyze this " +
"pull request."
"pull request."
)
end

Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a pre-existing problem but I don't think this branch should respond with ok: false -- it should be ok: true.

My understanding is that an ok: false has user facing implications: it's a red stop sign meaning the integration isn't working properly. However, the comment existing on a PR if we've already commented is expected. It happens if you open a PR and then push more commits (so a lot).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also the current behavior. If we're to change it, let's do it in another PR/card.

But yeah, I agree with you. It used to be the case where the return value may/may not matter, but now it does matter.

message: "Comment already present"
}
end
else
{ ok: false, message: "Comment already present" }
end
end

Expand Down
42 changes: 39 additions & 3 deletions test/github_pull_requests_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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
Expand Down