summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/project_services/jira_service.rb57
-rw-r--r--app/services/issues/close_service.rb2
-rw-r--r--app/views/shared/issuable/_form.html.haml25
-rw-r--r--app/views/shared/issuable/form/_branch_chooser.html.haml30
-rw-r--r--changelogs/unreleased/clean-up-jira-service.yml4
-rw-r--r--lib/gitlab/o_auth/user.rb2
-rw-r--r--spec/finders/labels_finder_spec.rb2
-rw-r--r--spec/models/project_services/jira_service_spec.rb40
-rw-r--r--spec/services/merge_requests/merge_service_spec.rb2
9 files changed, 94 insertions, 70 deletions
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 70bbbbcda85..894315a8593 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -9,6 +9,9 @@ class JiraService < IssueTrackerService
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
%w(commit merge_request)
end
@@ -105,18 +108,29 @@ class JiraService < IssueTrackerService
"#{url}/secure/CreateIssue.jspa"
end
- def execute(push, issue = nil)
- if issue.nil?
- # No specific issue, that means
- # we just want to test settings
- test_settings
- else
- jira_issue = jira_request { client.Issue.find(issue.iid) }
+ def execute(push)
+ # This method is a no-op, because currently JiraService does not
+ # support any events.
+ end
- 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)
- end
+ 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 create_cross_reference_note(mentioned, noteable, author)
@@ -156,6 +170,11 @@ class JiraService < IssueTrackerService
"Please fill in Password and Username."
end
+ def test(_)
+ result = test_settings
+ { success: result.present?, result: result }
+ end
+
def can_test?
username.present? && password.present?
end
@@ -182,24 +201,6 @@ class JiraService < IssueTrackerService
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)
issue.transitions.build.save(transition: { id: jira_issue_transition_id })
end
diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb
index ab4c51386a4..f1030912c68 100644
--- a/app/services/issues/close_service.rb
+++ b/app/services/issues/close_service.rb
@@ -17,7 +17,7 @@ module Issues
# allowed to close the given issue.
def close_issue(issue, commit: nil, notifications: true, system_note: true)
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)
return issue
end
diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml
index 3d515a05d46..2f05093f435 100644
--- a/app/views/shared/issuable/_form.html.haml
+++ b/app/views/shared/issuable/_form.html.haml
@@ -40,30 +40,7 @@
title: 'Moving an issue will copy the discussion to a different project and close it here. All participants will be notified of the new location.' }
= icon('question-circle')
-- if issuable.is_a?(MergeRequest) && !issuable.closed_without_fork?
- %hr
- - if @merge_request.new_record?
- .form-group
- = form.label :source_branch, class: 'control-label'
- .col-sm-10
- .issuable-form-select-holder
- = form.select(:source_branch, [@merge_request.source_branch], { }, { class: 'source_branch select2 span2', disabled: true })
- .form-group
- = form.label :target_branch, class: 'control-label'
- .col-sm-10
- .issuable-form-select-holder
- = form.select(:target_branch, @merge_request.target_branches, { include_blank: true }, { class: 'target_branch select2 span2', disabled: @merge_request.new_record?, data: {placeholder: "Select branch"} })
- - if @merge_request.new_record?
- &nbsp;
- = link_to 'Change branches', mr_change_branches_path(@merge_request)
- - if @merge_request.can_remove_source_branch?(current_user)
- .form-group
- .col-sm-10.col-sm-offset-2
- .checkbox
- = label_tag 'merge_request[force_remove_source_branch]' do
- = hidden_field_tag 'merge_request[force_remove_source_branch]', '0', id: nil
- = check_box_tag 'merge_request[force_remove_source_branch]', '1', @merge_request.force_remove_source_branch?
- Remove source branch when merge request is accepted.
+= render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form
- is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
.row-content-block{class: (is_footer ? "footer-block" : "middle-block")}
diff --git a/app/views/shared/issuable/form/_branch_chooser.html.haml b/app/views/shared/issuable/form/_branch_chooser.html.haml
new file mode 100644
index 00000000000..b757893ea04
--- /dev/null
+++ b/app/views/shared/issuable/form/_branch_chooser.html.haml
@@ -0,0 +1,30 @@
+- issuable = local_assigns.fetch(:issuable)
+- form = local_assigns.fetch(:form)
+
+- return unless issuable.is_a?(MergeRequest)
+- return if issuable.closed_without_fork?
+
+%hr
+- if issuable.new_record?
+ .form-group
+ = form.label :source_branch, class: 'control-label'
+ .col-sm-10
+ .issuable-form-select-holder
+ = form.select(:source_branch, [issuable.source_branch], {}, { class: 'source_branch select2 span2', disabled: true })
+.form-group
+ = form.label :target_branch, class: 'control-label'
+ .col-sm-10
+ .issuable-form-select-holder
+ = form.select(:target_branch, issuable.target_branches, { include_blank: true }, { class: 'target_branch select2 span2', disabled: issuable.new_record?, data: { placeholder: "Select branch" }})
+ - if issuable.new_record?
+ &nbsp;
+ = link_to 'Change branches', mr_change_branches_path(issuable)
+
+- if issuable.can_remove_source_branch?(current_user)
+ .form-group
+ .col-sm-10.col-sm-offset-2
+ .checkbox
+ = label_tag 'merge_request[force_remove_source_branch]' do
+ = hidden_field_tag 'merge_request[force_remove_source_branch]', '0', id: nil
+ = check_box_tag 'merge_request[force_remove_source_branch]', '1', issuable.force_remove_source_branch?
+ Remove source branch when merge request is accepted.
diff --git a/changelogs/unreleased/clean-up-jira-service.yml b/changelogs/unreleased/clean-up-jira-service.yml
new file mode 100644
index 00000000000..a309cb57532
--- /dev/null
+++ b/changelogs/unreleased/clean-up-jira-service.yml
@@ -0,0 +1,4 @@
+---
+title: Refactor JiraService by moving code out of JiraService#execute method
+merge_request: 7756
+author:
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index a8b4dc2a83f..96ed20af918 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -39,7 +39,7 @@ module Gitlab
log.info "(#{provider}) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}"
gl_user
rescue ActiveRecord::RecordInvalid => e
- log.info "(#{provider}) Error saving user: #{gl_user.errors.full_messages}"
+ log.info "(#{provider}) Error saving user #{auth_hash.uid} (#{auth_hash.email}): #{gl_user.errors.full_messages}"
return self, e.record.errors
end
diff --git a/spec/finders/labels_finder_spec.rb b/spec/finders/labels_finder_spec.rb
index 9085cc8debf..1724cdba830 100644
--- a/spec/finders/labels_finder_spec.rb
+++ b/spec/finders/labels_finder_spec.rb
@@ -17,7 +17,7 @@ describe LabelsFinder do
let!(:project_label_4) { create(:label, project: project_4, title: 'Label 4') }
let!(:project_label_5) { create(:label, project: project_5, title: 'Label 5') }
- let!(:group_label_1) { create(:group_label, group: group_1, title: 'Label 1') }
+ let!(:group_label_1) { create(:group_label, group: group_1, title: 'Label 1 (group)') }
let!(:group_label_2) { create(:group_label, group: group_1, title: 'Group Label 2') }
let!(:group_label_3) { create(:group_label, group: group_2, title: 'Group Label 3') }
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index f5da967cd14..862e3a72a73 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -68,7 +68,7 @@ describe JiraService, models: true do
end
end
- describe "Execute" do
+ describe '#close_issue' do
let(:custom_base_url) { 'http://custom_url' }
let(:user) { create(:user) }
let(:project) { create(:project) }
@@ -101,12 +101,10 @@ describe JiraService, models: true do
@jira_service.save
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'
@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'
- WebMock.stub_request(:get, @project_url)
WebMock.stub_request(:get, project_issues_url)
WebMock.stub_request(:post, @transitions_url)
WebMock.stub_request(:post, @comment_url)
@@ -114,7 +112,7 @@ describe JiraService, models: true do
end
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(
body: /Issue solved with/
@@ -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
# for more information
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
expect(WebMock).to have_requested(:post, @comment_url)
@@ -146,7 +144,7 @@ describe JiraService, models: true 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)
- @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, @remote_link_url)
@@ -155,7 +153,7 @@ describe JiraService, models: true do
it "references the GitLab commit/merge request" do
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(
body: /#{custom_base_url}\/#{project.path_with_namespace}\/commit\/#{merge_request.diff_head_sha}/
@@ -170,7 +168,7 @@ describe JiraService, models: true do
{ script_name: '/gitlab' }
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(
body: /#{Gitlab.config.gitlab.url}\/#{project.path_with_namespace}\/commit\/#{merge_request.diff_head_sha}/
@@ -178,19 +176,33 @@ describe JiraService, models: true do
end
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(
body: /custom-id/
).once
end
+ end
- context "when testing" do
- it "tries to get jira project" do
- @jira_service.execute(nil)
+ describe '#test_settings' do
+ let(:jira_service) do
+ 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)
- end
+ before do
+ WebMock.stub_request(:get, project_url)
+ end
+
+ it 'tries to get JIRA project' do
+ jira_service.test_settings
+
+ expect(WebMock).to have_requested(:get, project_url)
end
end
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index dff1781d2aa..5a89acc96a4 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -75,7 +75,7 @@ describe MergeRequests::MergeService, services: true do
commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}")
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)
end
View Lorry Controller Status