Refactor JiraService by moving code out of JiraService#execute methodclean-up-jira-service

The implicit interface of project services states that the "execute"
method is meant to be called when project hooks are executed.
Currently JiraService does not support any project events even though
JiraService#supported_events says that "commit" and "merge_request"
are supported. They are only used to render correct options in
JIRA configuration screen, but they are not supported.
Because of that, this commit makes "execute" method a no-op.

5 files changed, 61 insertions, 44 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/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/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