Merge branch 'improve_jira_issue_closing' into 'master'

Improve JIRA issue closing

- Zendesk ticket: https://gitlab.zendesk.com/agent/tickets/11034

### Expected behavior

When a merge request has 'Fixes JIRA-1234' in the description the issue should not only be transitioned but also record a comment (regardless of workflow type in JIRA).

### Observed behavior

Depending on the configured workflow a comment may or may not be allowed while transitioning to a new state. In my testing the newest JIRA default workflow did not allow comments while transitioning to 'Done', for example. The 'classic' workflow does, though. 

This merge request splits the transition and comment in to two API calls. The advantage is that the comment will be recorded 100% of the time instead of it being dependent on the workflow configuration.

Also, JIRA returns a '204' response code when the transition is successful. I added '204' to the success conditional because we were generating an error in the current configuration even though the transition was successful.

See merge request !45

CHANGELOG-EE

@@ -4,6 +4,7 @@ v 8.2.0
   - When someone as marked as a required approver for a merge request, an email should be sent
   - Allow configuring the Jira API path (Alex Lossent)
   - Fix "Rebase onto master"
+  - Ensure a comment is properly recorded in JIRA when a merge request is accepted
 
 v 8.1.0 (unreleased)
   - added an issues template (Hannes Rosenögger)

app/models/project_services/jira_service.rb

@@ -130,7 +130,7 @@ class JiraService < IssueTrackerService
 
     case result.code
     when 201, 200
-      Rails.logger.info("#{self.class.name} SUCCESS #{result.code}: Sucessfully connected to #{api_url}.")
+      Rails.logger.info("#{self.class.name} SUCCESS #{result.code}: Successfully connected to #{api_url}.")
       true
     else
       Rails.logger.info("#{self.class.name} ERROR #{result.code}: #{result.parsed_response}")
@@ -161,25 +161,36 @@ class JiraService < IssueTrackerService
     self.jira_issue_transition_id ||= "2"
   end
 
-  def close_issue(commit, issue)
-    url = close_issue_url(issue.iid)
-
-    commit_url = build_entity_url(:commit, commit.id)
+  def close_issue(entity, issue)
+    commit_id = if entity.is_a?(Commit)
+                  entity.id
+                elsif entity.is_a?(MergeRequest)
+                  entity.last_commit.id
+                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. Split the operation in to two calls so the
+    # comment always works.
+    transition_issue(issue)
+    add_issue_solved_comment(issue, commit_id, commit_url)
+  end
 
+  def transition_issue(issue)
     message = {
-      update: {
-        comment: [{
-          add: {
-            body: "Issue solved with [#{commit.id}|#{commit_url}]."
-          }
-        }]
-      },
       transition: {
         id: jira_issue_transition_id
       }
-    }.to_json
+    }
+    send_message(close_issue_url(issue.iid), message.to_json)
+  end
+
+  def add_issue_solved_comment(issue, commit_id, commit_url)
+    comment = {
+      body: "Issue solved with [#{commit_id}|#{commit_url}]."
+    }
 
-    send_message(url, message)
+    send_message(comment_url(issue.iid), comment.to_json)
   end
 
   def add_comment(data, issue_name)
@@ -216,8 +227,8 @@ class JiraService < IssueTrackerService
     )
 
     message = case result.code
-              when 201, 200
-                "#{self.class.name} SUCCESS #{result.code}: Sucessfully posted to #{url}."
+              when 201, 200, 204
+                "#{self.class.name} SUCCESS #{result.code}: Successfully posted to #{url}."
               when 401
                 "#{self.class.name} ERROR 401: Unauthorized. Check the #{self.username} credentials and JIRA access permissions and try again."
               else

spec/models/project_services/jira_service_spec.rb

@@ -31,6 +31,7 @@ describe JiraService do
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }
+    let(:merge_request) { create(:merge_request) }
 
     before do
       @jira_service = JiraService.new
@@ -46,20 +47,22 @@ describe JiraService do
       @sample_data = Gitlab::PushDataBuilder.build_sample(project, user)
       # https://github.com/bblimke/webmock#request-with-basic-authentication
       @api_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'
 
       WebMock.stub_request(:post, @api_url)
+      WebMock.stub_request(:post, @comment_url)
     end
 
     it "should call JIRA API" do
-      @jira_service.execute(sample_commit, JiraIssue.new("JIRA-123", project))
-      expect(WebMock).to have_requested(:post, @api_url).with(
+      @jira_service.execute(merge_request, JiraIssue.new("JIRA-123", project))
+      expect(WebMock).to have_requested(:post, @comment_url).with(
         body: /Issue solved with/
       ).once
     end
 
     it "calls the api with jira_issue_transition_id" do
       @jira_service.jira_issue_transition_id = 'this-is-a-custom-id'
-      @jira_service.execute(sample_commit, JiraIssue.new("JIRA-123", project))
+      @jira_service.execute(merge_request, JiraIssue.new("JIRA-123", project))
       expect(WebMock).to have_requested(:post, @api_url).with(
         body: /this-is-a-custom-id/
       ).once

spec/services/git_push_service_spec.rb

@@ -310,14 +310,7 @@ describe GitPushService do
         let(:message) { "this is some work.\n\ncloses JIRA-1" }
 
         it "should initiate one api call to jira server to close the issue" do
-          body = {
-            update: {
-              comment: [{
-                add: {
-                  body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]."
-                }
-              }]
-            },
+          transition_body = {
             transition: {
               id: '2'
             }
@@ -325,7 +318,18 @@ describe GitPushService do
 
           service.execute(project, user, @oldrev, @newrev, @ref)
           expect(WebMock).to have_requested(:post, jira_api_transition_url).with(
-            body: body
+            body: transition_body
+          ).once
+        end
+
+        it "should initiate one api call to jira server to comment on the issue" do
+          comment_body = {
+            body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]."
+          }.to_json
+
+          service.execute(project, user, @oldrev, @newrev, @ref)
+          expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
+            body: comment_body
           ).once
         end
       end

spec/support/jira_service_helper.rb

@@ -13,7 +13,7 @@ module JiraServiceHelper
   end
 
   def jira_status_message
-    "JiraService SUCCESS 200: Sucessfully posted to #{jira_api_comment_url}."
+    "JiraService SUCCESS 200: Successfully posted to #{jira_api_comment_url}."
   end
 
   def jira_issue_comments