Refactor JiraService by moving code out of JiraService#execute method

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.

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

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

changelogs/unreleased/clean-up-jira-service.yml

+---
+title: Refactor JiraService by moving code out of JiraService#execute method
+merge_request: 7756
+author:

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
 

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