Merge branch 'issue_13232' into 'master'

Add JIRA remotelinks and prevent duplicated closing messages

implements #13232  
closes #20479   

See merge request !7413

app/models/project_services/jira_service.rb

@@ -70,7 +70,7 @@ class JiraService < IssueTrackerService
   end
 
   def jira_project
-    @jira_project ||= client.Project.find(project_key)
+    @jira_project ||= jira_request { client.Project.find(project_key) }
   end
 
   def help
@@ -128,12 +128,19 @@ class JiraService < IssueTrackerService
       # we just want to test settings
       test_settings
     else
-      close_issue(push, issue)
+      jira_issue = jira_request { client.Issue.find(issue.iid) }
+
+      return false unless jira_issue.present?
+
+      close_issue(push, jira_issue)
     end
   end
 
   def create_cross_reference_note(mentioned, noteable, author)
-    issue_key = mentioned.id
+    jira_issue = jira_request { client.Issue.find(mentioned.id) }
+
+    return false unless jira_issue.present?
+
     project = self.project
     noteable_name = noteable.class.name.underscore.downcase
     noteable_id = if noteable.is_a?(Commit)
@@ -160,7 +167,7 @@ class JiraService < IssueTrackerService
       }
     }
 
-    add_comment(data, issue_key)
+    add_comment(data, jira_issue)
   end
 
   # reason why service cannot be tested
@@ -181,16 +188,14 @@ class JiraService < IssueTrackerService
   def test_settings
     return unless url.present?
     # Test settings by getting the project
-    jira_project
-
-  rescue Errno::ECONNREFUSED, JIRA::HTTPError => e
-    Rails.logger.info "#{self.class.name} ERROR: #{e.message}. API URL: #{url}."
-    false
+    jira_request { jira_project.present? }
   end
 
   private
 
   def close_issue(entity, issue)
+    return if issue.nil? || issue.resolution.present?
+
     commit_id = if entity.is_a?(Commit)
                   entity.id
                 elsif entity.is_a?(MergeRequest)
@@ -200,23 +205,24 @@ class JiraService < IssueTrackerService
     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)
+    # 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 = client.Issue.find(issue.iid)
     issue.transitions.build.save(transition: { id: jira_issue_transition_id })
   end
 
   def add_issue_solved_comment(issue, commit_id, commit_url)
+    link_title   = "GitLab: Solved by commit #{commit_id}."
     comment      = "Issue solved with [#{commit_id}|#{commit_url}]."
-    send_message(issue.iid, comment)
+    link_props   = build_remote_link_props(url: commit_url, title: link_title, resolved: true)
+    send_message(issue, comment, link_props)
   end
 
-  def add_comment(data, issue_key)
+  def add_comment(data, issue)
     user_name    = data[:user][:name]
     user_url     = data[:user][:url]
     entity_name  = data[:entity][:name]
@@ -225,30 +231,59 @@ class JiraService < IssueTrackerService
     project_name = data[:project][:name]
 
     message      = "[#{user_name}|#{user_url}] mentioned this issue in [a #{entity_name} of #{project_name}|#{entity_url}]:\n'#{entity_title}'"
+    link_title   = "GitLab: Mentioned on #{entity_name} - #{entity_title}"
+    link_props   = build_remote_link_props(url: entity_url, title: link_title)
 
-    unless comment_exists?(issue_key, message)
-      send_message(issue_key, message)
+    unless comment_exists?(issue, message)
+      send_message(issue, message, link_props)
     end
   end
 
-  def comment_exists?(issue_key, message)
-    comments = client.Issue.find(issue_key).comments
-    comments.map { |comment| comment.body.include?(message) }.any?
+  def comment_exists?(issue, message)
+    comments = jira_request { issue.comments }
+
+    comments.present? && comments.any? { |comment| comment.body.include?(message) }
   end
 
-  def send_message(issue_key, message)
+  def send_message(issue, message, remote_link_props)
     return unless url.present?
 
-    issue = client.Issue.find(issue_key)
-
+    jira_request do
       if issue.comments.build.save!(body: message)
+        remote_link = issue.remotelink.build
+        remote_link.save!(remote_link_props)
         result_message = "#{self.class.name} SUCCESS: Successfully posted to #{url}."
       end
 
       Rails.logger.info(result_message)
       result_message
-  rescue URI::InvalidURIError, Errno::ECONNREFUSED, JIRA::HTTPError => e
-    Rails.logger.info "#{self.class.name} Send message ERROR: #{url} - #{e.message}"
+    end
+  end
+
+  # Build remote link on JIRA properties
+  # Icons here must be available on WEB so JIRA can read the URL
+  # We are using a open word graphics icon which have LGPL license
+  def build_remote_link_props(url:, title:, resolved: false)
+    status = {
+      resolved: resolved
+    }
+
+    if resolved
+      status[:icon] = {
+        title: 'Closed',
+        url16x16: 'http://www.openwebgraphics.com/resources/data/1768/16x16_apply.png'
+      }
+    end
+
+    {
+      GlobalID: 'GitLab',
+      object: {
+        url: url,
+        title: title,
+        status: status,
+        icon: { title: 'GitLab', url16x16: 'https://gitlab.com/favicon.ico' }
+      }
+    }
   end
 
   def resource_url(resource)
@@ -266,4 +301,13 @@ class JiraService < IssueTrackerService
       host: Settings.gitlab.base_url
     )
   end
+
+  # Handle errors when doing JIRA API calls
+  def jira_request
+    yield
+
+  rescue Timeout::Error, Errno::EINVAL, Errno::ECONNRESET, Errno::ECONNREFUSED, URI::InvalidURIError, JIRA::HTTPError => e
+    Rails.logger.info "#{self.class.name} Send message ERROR: #{url} - #{e.message}"
+    nil
+  end
 end

changelogs/unreleased/issue_13232.yml

+---
+title: Add JIRA remotelinks and prevent duplicated closing messages
+merge_request: 
+author: 

spec/models/project_services/jira_service_spec.rb

@@ -86,17 +86,30 @@ describe JiraService, models: true do
         project_key: 'GitLabProject'
       )
 
+      # These stubs are needed to test JiraService#close_issue.
+      # We close the issue then do another request to API to check if it got closed.
+      # Here is stubbed the API return with a closed and an opened issues.
+      open_issue   = JIRA::Resource::Issue.new(@jira_service.client, attrs: { "id" => "JIRA-123" })
+      closed_issue = open_issue.dup
+      allow(open_issue).to receive(:resolution).and_return(false)
+      allow(closed_issue).to receive(:resolution).and_return(true)
+      allow(JIRA::Resource::Issue).to receive(:find).and_return(open_issue, closed_issue)
+
+      allow_any_instance_of(JIRA::Resource::Issue).to receive(:key).and_return("JIRA-123")
+
       @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)
+      WebMock.stub_request(:post, @remote_link_url)
     end
 
     it "calls JIRA API" do
@@ -107,6 +120,37 @@ describe JiraService, models: true do
       ).once
     end
 
+    # 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))
+
+      # Creates comment
+      expect(WebMock).to have_requested(:post, @comment_url)
+
+      # Creates Remote Link in JIRA issue fields
+      expect(WebMock).to have_requested(:post, @remote_link_url).with(
+        body: hash_including(
+          GlobalID: "GitLab",
+          object: {
+            url: "#{Gitlab.config.gitlab.url}/#{project.path_with_namespace}/commit/#{merge_request.diff_head_sha}",
+            title: "GitLab: Solved by commit #{merge_request.diff_head_sha}.",
+            icon: { title: "GitLab", url16x16: "https://gitlab.com/favicon.ico" },
+            status: { resolved: true, icon: { url16x16: "http://www.openwebgraphics.com/resources/data/1768/16x16_apply.png", title: "Closed" } }
+          }
+        )
+      ).once
+    end
+
+    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))
+
+      expect(WebMock).not_to have_requested(:post, @comment_url)
+      expect(WebMock).not_to have_requested(:post, @remote_link_url)
+    end
+
     it "references the GitLab commit/merge request" do
       stub_config_setting(base_url: custom_base_url)
 

spec/services/git_push_service_spec.rb

@@ -453,6 +453,16 @@ describe GitPushService, services: true do
         let(:message)         { "this is some work.\n\ncloses JIRA-1" }
         let(:comment_body)    { { body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json }
 
+        before do
+          open_issue   = JIRA::Resource::Issue.new(jira_tracker.client, attrs: { "id" => "JIRA-1" })
+          closed_issue = open_issue.dup
+          allow(open_issue).to receive(:resolution).and_return(false)
+          allow(closed_issue).to receive(:resolution).and_return(true)
+          allow(JIRA::Resource::Issue).to receive(:find).and_return(open_issue, closed_issue)
+
+          allow_any_instance_of(JIRA::Resource::Issue).to receive(:key).and_return("JIRA-1")
+        end
+
         context "using right markdown" do
           it "initiates one api call to jira server to close the issue" do
             execute_service(project, commit_author, @oldrev, @newrev, @ref )

spec/services/merge_requests/merge_service_spec.rb

@@ -67,17 +67,19 @@ describe MergeRequests::MergeService, services: true do
 
         it 'closes issues on JIRA issue tracker' do
           jira_issue = ExternalIssue.new('JIRA-123', project)
+          stub_jira_urls(jira_issue)
           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, jira_issue).once
+          expect_any_instance_of(JiraService).to receive(:close_issue).with(merge_request, an_instance_of(JIRA::Resource::Issue)).once
 
           service.execute(merge_request)
         end
 
         context "wrong issue markdown" do
           it 'does not close issues on JIRA issue tracker' do
-            jira_issue = ExternalIssue.new('#123', project)
+            jira_issue = ExternalIssue.new('#JIRA-123', project)
+            stub_jira_urls(jira_issue)
             commit = double('commit', safe_message: "Fixes #{jira_issue.to_reference}")
             allow(merge_request).to receive(:commits).and_return([commit])
 

spec/services/system_note_service_spec.rb

 require 'spec_helper'
 
 describe SystemNoteService, services: true do
+  include Gitlab::Routing.url_helpers
+
   let(:project)  { create(:project) }
   let(:author)   { create(:user) }
   let(:noteable) { create(:issue, project: project) }
@@ -543,23 +545,55 @@ describe SystemNoteService, services: true do
 
     before { stub_jira_urls(jira_issue.id) }
 
-    context 'in JIRA issue tracker' do
+    context 'in issue' do
       before { jira_service_settings }
 
       describe "new reference" do
         subject { described_class.cross_reference(jira_issue, commit, author) }
 
         it { is_expected.to eq(success_message) }
+
+        it "creates remote link" do
+          subject
+
+          expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
+            body: hash_including(
+              GlobalID: "GitLab",
+              object: {
+                url: namespace_project_commit_url(project.namespace, project, commit),
+                title: "GitLab: Mentioned on commit - #{commit.title}",
+                icon: { title: "GitLab", url16x16: "https://gitlab.com/favicon.ico" },
+                status: { resolved: false }
+              }
+            )
+          ).once
+        end
       end
     end
 
-    context 'issue from an issue' do
+    context 'in commit' do
       context 'in JIRA issue tracker' do
         before { jira_service_settings }
 
         subject { described_class.cross_reference(jira_issue, issue, author) }
 
         it { is_expected.to eq(success_message) }
+
+        it "creates remote link" do
+          subject
+
+          expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
+            body: hash_including(
+              GlobalID: "GitLab",
+              object: {
+                url: namespace_project_issue_url(project.namespace, project, issue),
+                title: "GitLab: Mentioned on issue - #{issue.title}",
+                icon: { title: "GitLab", url16x16: "https://gitlab.com/favicon.ico" },
+                status: { resolved: false }
+              }
+            )
+          ).once
+        end
       end
     end
 
@@ -572,6 +606,13 @@ describe SystemNoteService, services: true do
       subject { described_class.cross_reference(jira_issue, commit, author) }
 
       it { is_expected.not_to eq(success_message) }
+
+      it 'does not try to create comment and remote link' do
+        subject
+
+        expect(WebMock).not_to have_requested(:post, jira_api_comment_url(jira_issue))
+        expect(WebMock).not_to have_requested(:post, jira_api_remote_link_url(jira_issue))
+      end
     end
   end
 end

spec/support/jira_service_helper.rb

@@ -57,6 +57,10 @@ module JiraServiceHelper
     JIRA_API + "/issue/#{issue_id}/comment"
   end
 
+  def jira_api_remote_link_url(issue_id)
+    JIRA_API + "/issue/#{issue_id}/remotelink"
+  end
+
   def jira_api_transition_url(issue_id)
     JIRA_API + "/issue/#{issue_id}/transitions"
   end
@@ -75,6 +79,7 @@ module JiraServiceHelper
     WebMock.stub_request(:get, jira_issue_url(issue_id))
     WebMock.stub_request(:get, jira_api_test_url)
     WebMock.stub_request(:post, jira_api_comment_url(issue_id))
+    WebMock.stub_request(:post, jira_api_remote_link_url(issue_id))
     WebMock.stub_request(:post, jira_api_transition_url(issue_id))
   end
 end