Commit 1e518c04 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'clean-up-jira-service' into 'master'

Clean up JiraService

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/24967

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/24777

See merge request !7756
parents c2be86b5 11055973
...@@ -9,6 +9,9 @@ class JiraService < IssueTrackerService ...@@ -9,6 +9,9 @@ class JiraService < IssueTrackerService
before_update :reset_password before_update :reset_password
# This is confusing, but JiraService does not really support these events.
# The values here are required to display correct options in the service
# configuration screen.
def supported_events def supported_events
%w(commit merge_request) %w(commit merge_request)
end end
...@@ -105,18 +108,29 @@ class JiraService < IssueTrackerService ...@@ -105,18 +108,29 @@ class JiraService < IssueTrackerService
"#{url}/secure/CreateIssue.jspa" "#{url}/secure/CreateIssue.jspa"
end end
def execute(push, issue = nil) def execute(push)
if issue.nil? # This method is a no-op, because currently JiraService does not
# No specific issue, that means # support any events.
# we just want to test settings end
test_settings
else
jira_issue = jira_request { client.Issue.find(issue.iid) }
return false unless jira_issue.present? def close_issue(entity, external_issue)
issue = jira_request { client.Issue.find(external_issue.iid) }
close_issue(push, jira_issue) return if issue.nil? || issue.resolution.present? || !jira_issue_transition_id.present?
commit_id = if entity.is_a?(Commit)
entity.id
elsif entity.is_a?(MergeRequest)
entity.diff_head_sha
end end
commit_url = build_entity_url(:commit, commit_id)
# Depending on the JIRA project's workflow, a comment during transition
# may or may not be allowed. Refresh the issue after transition and check
# if it is closed, so we don't have one comment for every commit.
issue = jira_request { client.Issue.find(issue.key) } if transition_issue(issue)
add_issue_solved_comment(issue, commit_id, commit_url) if issue.resolution
end end
def create_cross_reference_note(mentioned, noteable, author) def create_cross_reference_note(mentioned, noteable, author)
...@@ -156,6 +170,11 @@ class JiraService < IssueTrackerService ...@@ -156,6 +170,11 @@ class JiraService < IssueTrackerService
"Please fill in Password and Username." "Please fill in Password and Username."
end end
def test(_)
result = test_settings
{ success: result.present?, result: result }
end
def can_test? def can_test?
username.present? && password.present? username.present? && password.present?
end end
...@@ -182,24 +201,6 @@ class JiraService < IssueTrackerService ...@@ -182,24 +201,6 @@ class JiraService < IssueTrackerService
end end
end end
def close_issue(entity, issue)
return if issue.nil? || issue.resolution.present? || !jira_issue_transition_id.present?
commit_id = if entity.is_a?(Commit)
entity.id
elsif entity.is_a?(MergeRequest)
entity.diff_head_sha
end
commit_url = build_entity_url(:commit, commit_id)
# Depending on the JIRA project's workflow, a comment during transition
# may or may not be allowed. Refresh the issue after transition and check
# if it is closed, so we don't have one comment for every commit.
issue = jira_request { client.Issue.find(issue.key) } if transition_issue(issue)
add_issue_solved_comment(issue, commit_id, commit_url) if issue.resolution
end
def transition_issue(issue) def transition_issue(issue)
issue.transitions.build.save(transition: { id: jira_issue_transition_id }) issue.transitions.build.save(transition: { id: jira_issue_transition_id })
end end
......
...@@ -17,7 +17,7 @@ module Issues ...@@ -17,7 +17,7 @@ module Issues
# allowed to close the given issue. # allowed to close the given issue.
def close_issue(issue, commit: nil, notifications: true, system_note: true) def close_issue(issue, commit: nil, notifications: true, system_note: true)
if project.jira_tracker? && project.jira_service.active if project.jira_tracker? && project.jira_service.active
project.jira_service.execute(commit, issue) project.jira_service.close_issue(commit, issue)
todo_service.close_issue(issue, current_user) todo_service.close_issue(issue, current_user)
return issue return issue
end end
......
---
title: Refactor JiraService by moving code out of JiraService#execute method
merge_request: 7756
author:
...@@ -68,7 +68,7 @@ describe JiraService, models: true do ...@@ -68,7 +68,7 @@ describe JiraService, models: true do
end end
end end
describe "Execute" do describe '#close_issue' do
let(:custom_base_url) { 'http://custom_url' } let(:custom_base_url) { 'http://custom_url' }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
...@@ -101,12 +101,10 @@ describe JiraService, models: true do ...@@ -101,12 +101,10 @@ describe JiraService, models: true do
@jira_service.save @jira_service.save
project_issues_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123' project_issues_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123'
@project_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/project/GitLabProject'
@transitions_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123/transitions' @transitions_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123/transitions'
@comment_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123/comment' @comment_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123/comment'
@remote_link_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123/remotelink' @remote_link_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123/remotelink'
WebMock.stub_request(:get, @project_url)
WebMock.stub_request(:get, project_issues_url) WebMock.stub_request(:get, project_issues_url)
WebMock.stub_request(:post, @transitions_url) WebMock.stub_request(:post, @transitions_url)
WebMock.stub_request(:post, @comment_url) WebMock.stub_request(:post, @comment_url)
...@@ -114,7 +112,7 @@ describe JiraService, models: true do ...@@ -114,7 +112,7 @@ describe JiraService, models: true do
end end
it "calls JIRA API" do it "calls JIRA API" do
@jira_service.execute(merge_request, ExternalIssue.new("JIRA-123", project)) @jira_service.close_issue(merge_request, ExternalIssue.new("JIRA-123", project))
expect(WebMock).to have_requested(:post, @comment_url).with( expect(WebMock).to have_requested(:post, @comment_url).with(
body: /Issue solved with/ body: /Issue solved with/
...@@ -124,7 +122,7 @@ describe JiraService, models: true do ...@@ -124,7 +122,7 @@ describe JiraService, models: true do
# Check https://developer.atlassian.com/jiradev/jira-platform/guides/other/guide-jira-remote-issue-links/fields-in-remote-issue-links # Check https://developer.atlassian.com/jiradev/jira-platform/guides/other/guide-jira-remote-issue-links/fields-in-remote-issue-links
# for more information # for more information
it "creates Remote Link reference in JIRA for comment" do it "creates Remote Link reference in JIRA for comment" do
@jira_service.execute(merge_request, ExternalIssue.new("JIRA-123", project)) @jira_service.close_issue(merge_request, ExternalIssue.new("JIRA-123", project))
# Creates comment # Creates comment
expect(WebMock).to have_requested(:post, @comment_url) expect(WebMock).to have_requested(:post, @comment_url)
...@@ -146,7 +144,7 @@ describe JiraService, models: true do ...@@ -146,7 +144,7 @@ describe JiraService, models: true do
it "does not send comment or remote links to issues already closed" do it "does not send comment or remote links to issues already closed" do
allow_any_instance_of(JIRA::Resource::Issue).to receive(:resolution).and_return(true) allow_any_instance_of(JIRA::Resource::Issue).to receive(:resolution).and_return(true)
@jira_service.execute(merge_request, ExternalIssue.new("JIRA-123", project)) @jira_service.close_issue(merge_request, ExternalIssue.new("JIRA-123", project))
expect(WebMock).not_to have_requested(:post, @comment_url) expect(WebMock).not_to have_requested(:post, @comment_url)
expect(WebMock).not_to have_requested(:post, @remote_link_url) expect(WebMock).not_to have_requested(:post, @remote_link_url)
...@@ -155,7 +153,7 @@ describe JiraService, models: true do ...@@ -155,7 +153,7 @@ describe JiraService, models: true do
it "references the GitLab commit/merge request" do it "references the GitLab commit/merge request" do
stub_config_setting(base_url: custom_base_url) stub_config_setting(base_url: custom_base_url)
@jira_service.execute(merge_request, ExternalIssue.new("JIRA-123", project)) @jira_service.close_issue(merge_request, ExternalIssue.new("JIRA-123", project))
expect(WebMock).to have_requested(:post, @comment_url).with( expect(WebMock).to have_requested(:post, @comment_url).with(
body: /#{custom_base_url}\/#{project.path_with_namespace}\/commit\/#{merge_request.diff_head_sha}/ body: /#{custom_base_url}\/#{project.path_with_namespace}\/commit\/#{merge_request.diff_head_sha}/
...@@ -170,7 +168,7 @@ describe JiraService, models: true do ...@@ -170,7 +168,7 @@ describe JiraService, models: true do
{ script_name: '/gitlab' } { script_name: '/gitlab' }
end end
@jira_service.execute(merge_request, ExternalIssue.new("JIRA-123", project)) @jira_service.close_issue(merge_request, ExternalIssue.new("JIRA-123", project))
expect(WebMock).to have_requested(:post, @comment_url).with( expect(WebMock).to have_requested(:post, @comment_url).with(
body: /#{Gitlab.config.gitlab.url}\/#{project.path_with_namespace}\/commit\/#{merge_request.diff_head_sha}/ body: /#{Gitlab.config.gitlab.url}\/#{project.path_with_namespace}\/commit\/#{merge_request.diff_head_sha}/
...@@ -178,19 +176,33 @@ describe JiraService, models: true do ...@@ -178,19 +176,33 @@ describe JiraService, models: true do
end end
it "calls the api with jira_issue_transition_id" do it "calls the api with jira_issue_transition_id" do
@jira_service.execute(merge_request, ExternalIssue.new("JIRA-123", project)) @jira_service.close_issue(merge_request, ExternalIssue.new("JIRA-123", project))
expect(WebMock).to have_requested(:post, @transitions_url).with( expect(WebMock).to have_requested(:post, @transitions_url).with(
body: /custom-id/ body: /custom-id/
).once ).once
end end
end
context "when testing" do describe '#test_settings' do
it "tries to get jira project" do let(:jira_service) do
@jira_service.execute(nil) described_class.new(
url: 'http://jira.example.com',
username: 'gitlab_jira_username',
password: 'gitlab_jira_password',
project_key: 'GitLabProject'
)
end
let(:project_url) { 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/project/GitLabProject' }
expect(WebMock).to have_requested(:get, @project_url) before do
WebMock.stub_request(:get, project_url)
end end
it 'tries to get JIRA project' do
jira_service.test_settings
expect(WebMock).to have_requested(:get, project_url)
end end
end end
......
...@@ -75,7 +75,7 @@ describe MergeRequests::MergeService, services: true do ...@@ -75,7 +75,7 @@ describe MergeRequests::MergeService, services: true do
commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}") commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}")
allow(merge_request).to receive(:commits).and_return([commit]) allow(merge_request).to receive(:commits).and_return([commit])
expect_any_instance_of(JiraService).to receive(:close_issue).with(merge_request, an_instance_of(JIRA::Resource::Issue)).once expect_any_instance_of(JiraService).to receive(:close_issue).with(merge_request, jira_issue).once
service.execute(merge_request) service.execute(merge_request)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment