Merge branch 'add_comment_when_it_is_new' into 'master'

Check if comment exists before sending a reference to Jira

Currently every time an issue is mentioned, Jira service sends a reference to Jira.
To prevent problems like described in #265 , add another query to the process to retrieve all comments for the issue so we can see if it contains the reference we want to add.

See merge request !377

CHANGELOG-EE

-v 7.10.0 (unreleased)
+v 7.11.1
+  - Check if comment exists in Jira before sending a reference
+
+v 7.10.0
   - Improve UI for next pages: Group LDAP sync, Project git hooks, Project share with groups, Admin -> Appearance settigns
   - Default git hooks for new projects
   - Fix LDAP group links page by using new group members route.
   - Skip email confirmation when updated via LDAP.
 
-v 7.9.0 (unreleased)
+v 7.9.0
   - Strip prefixes and suffixes from synced SSH keys:
     `SSHKey:ssh-rsa keykeykey` and `ssh-rsa keykeykey (SSH key)` will now work
   - Check if LDAP admin group exists before querying for user membership

app/models/project_services/jira_service.rb

@@ -70,7 +70,13 @@ class JiraService < IssueTrackerService
   end
 
   def execute(push, issue = nil)
-    close_issue(push, issue) if issue
+    if issue.nil?
+      # No specific issue, that means
+      # we just want to test settings
+      test_settings
+    else
+      close_issue(push, issue)
+    end
   end
 
   def create_cross_reference_note(mentioned, noteable, author)
@@ -103,6 +109,28 @@ class JiraService < IssueTrackerService
     add_comment(data, issue_name)
   end
 
+  def test_settings
+    result = JiraService.get(
+      jira_project_url,
+      headers: {
+        'Content-Type' => 'application/json',
+        'Authorization' => "Basic #{auth}"
+      }
+    )
+
+    case result.code
+    when 201, 200
+      Rails.logger.info("#{self.class.name} SUCCESS #{result.code}: Sucessfully connected to #{server_url}.")
+      true
+    else
+      Rails.logger.info("#{self.class.name} ERROR #{result.code}: #{result.parsed_response}")
+      false
+    end
+  rescue Errno::ECONNREFUSED => e
+    Rails.logger.info "#{self.class.name} ERROR: #{e.message}. Hostname: #{server_url}."
+    false
+  end
+
   private
 
 
@@ -136,7 +164,7 @@ class JiraService < IssueTrackerService
   end
 
   def add_comment(data, issue_name)
-    url = add_comment_url(issue_name)
+    url = comment_url(issue_name)
     user_name = data[:user][:name]
     user_url = data[:user][:url]
     entity_name = data[:entity][:name]
@@ -147,9 +175,11 @@ class JiraService < IssueTrackerService
 
     message = {
       body: "[#{user_name}|#{user_url}] mentioned this issue in [a #{entity_name} of #{project_name}|#{entity_url}]."
-    }.to_json
+    }
 
-    send_message(url, message)
+    unless existing_comment?(issue_name, message[:body])
+      send_message(url, message.to_json)
+    end
   end
 
 
@@ -179,10 +209,33 @@ class JiraService < IssueTrackerService
 
     Rails.logger.info(message)
     message
-  rescue URI::InvalidURIError => e
+  rescue URI::InvalidURIError, Errno::ECONNREFUSED => e
     Rails.logger.info "#{self.class.name} ERROR: #{e.message}. Hostname: #{url}."
   end
 
+  def existing_comment?(issue_name, new_comment)
+    result = JiraService.get(
+      comment_url(issue_name),
+      headers: {
+        'Content-Type' => 'application/json',
+        'Authorization' => "Basic #{auth}"
+      }
+    )
+
+    case result.code
+    when 201, 200
+      existing_comments = JSON.parse(result.body)['comments']
+
+      if existing_comments.present?
+        return existing_comments.map { |comment| comment['body'].include?(new_comment) }.any?
+      end
+    end
+
+    false
+  rescue JSON::ParserError
+    false
+  end
+
   def server_url
     server = URI(project_url)
     default_ports = [80, 443].include?(server.port)
@@ -213,7 +266,11 @@ class JiraService < IssueTrackerService
     "#{server_url}/rest/api/#{self.api_version}/issue/#{issue_name}/transitions"
   end
 
-  def add_comment_url(issue_name)
+  def comment_url(issue_name)
     "#{server_url}/rest/api/#{self.api_version}/issue/#{issue_name}/comment"
   end
+
+  def jira_project_url
+    "#{server_url}/rest/api/#{self.api_version}/project"
+  end
 end

spec/models/note_spec.rb

@@ -20,6 +20,8 @@
 require 'spec_helper'
 
 describe Note do
+  include JiraServiceHelper
+
   describe "Associations" do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:noteable) }
@@ -332,7 +334,6 @@ describe Note do
     let(:commit)     { project.repository.commit }
     let(:jira_issue) { JiraIssue.new("JIRA-1", project)}
     let(:jira_tracker) { project.create_jira_service if project.jira_service.nil? }
-    let(:api_mention_url) { 'http://jira.example/rest/api/2/issue/JIRA-1/comment' }
 
     # Test all of {issue, merge request, commit} in both the referenced and referencing
     # roles, to ensure that the correct information can be inferred from any argument.
@@ -368,7 +369,9 @@ describe Note do
       context 'in JIRA issue tracker' do
         before do
           jira_service_settings
-          WebMock.stub_request(:post, api_mention_url)
+          WebMock.stub_request(:post, jira_api_comment_url)
+          WebMock.stub_request(:get, jira_api_comment_url).
+            to_return(:body => jira_issue_comments)
         end
 
         after do
@@ -402,16 +405,34 @@ describe Note do
       context 'in JIRA issue tracker' do
         before do
           jira_service_settings
-          WebMock.stub_request(:post, api_mention_url)
+          WebMock.stub_request(:post, jira_api_comment_url)
         end
 
         after do
           jira_tracker.destroy!
         end
 
-        subject { Note.create_cross_reference_note(jira_issue, commit, author, project) }
+        describe "new reference" do
+          before do
+            WebMock.stub_request(:get, jira_api_comment_url).
+              to_return(:body => jira_issue_comments)
+          end
 
-        it { is_expected.to eq(jira_status_message) }
+          subject { Note.create_cross_reference_note(jira_issue, commit, author, project) }
+
+          it { is_expected.to eq(jira_status_message) }
+        end
+
+        describe "existing reference" do
+          before do
+            message = "[#{author.name}|http://localhost/u/#{author.username}] mentioned this issue in [a commit of #{project.path_with_namespace}|http://localhost/#{project.path_with_namespace}/commit/#{commit.id}]."
+            WebMock.stub_request(:get, jira_api_comment_url).
+              to_return(:body => "{\"comments\":[{\"body\":\"#{message}\"}]}")
+          end
+
+          subject { Note.create_cross_reference_note(jira_issue, commit, author, project) }
+          it { is_expected.not_to eq(jira_status_message) }
+        end
       end
     end
 
@@ -419,7 +440,9 @@ describe Note do
       context 'in JIRA issue tracker' do
         before do
           jira_service_settings
-          WebMock.stub_request(:post, api_mention_url)
+          WebMock.stub_request(:post, jira_api_comment_url)
+          WebMock.stub_request(:get, jira_api_comment_url).
+            to_return(:body => jira_issue_comments)
         end
 
         after do
@@ -690,19 +713,4 @@ describe Note do
     let(:backref_text) { issue.gfm_reference }
     let(:set_mentionable_text) { ->(txt) { subject.note = txt } }
   end
-
-  def jira_service_settings
-    properties = {
-      "title"=>"JIRA tracker",
-      "project_url"=>"http://jira.example/issues/?jql=project=A",
-      "issues_url"=>"http://jira.example/browse/JIRA-1",
-      "new_issue_url"=>"http://jira.example/secure/CreateIssue.jspa"
-    }
-
-    jira_tracker.update_attributes(properties: properties, active: true)
-  end
-
-  def jira_status_message
-    "JiraService SUCCESS 200: Sucessfully posted to #{api_mention_url}."
-  end
 end

spec/services/git_push_service_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe GitPushService do
   include RepoHelpers
+  include JiraServiceHelper
 
   let (:user)          { create :user }
   let (:project)       { create :project }
@@ -237,21 +238,16 @@ describe GitPushService do
     end
 
     context "for jira issue tracker" do
-      let(:api_transition_url) { 'http://jira.example/rest/api/2/issue/JIRA-1/transitions' }
-      let(:api_mention_url) { 'http://jira.example/rest/api/2/issue/JIRA-1/comment' }
       let(:jira_tracker) { project.create_jira_service if project.jira_service.nil? }
 
       before do
-        properties = {
-          "title"=>"JIRA tracker",
-          "project_url"=>"http://jira.example/issues/?jql=project=A",
-          "issues_url"=>"http://jira.example/browse/JIRA-1",
-          "new_issue_url"=>"http://jira.example/secure/CreateIssue.jspa"
-        }
-        jira_tracker.update_attributes(properties: properties, active: true)
+        jira_service_settings
 
-        WebMock.stub_request(:post, api_transition_url)
-        WebMock.stub_request(:post, api_mention_url)
+        WebMock.stub_request(:post, jira_api_transition_url)
+        WebMock.stub_request(:post, jira_api_comment_url)
+        WebMock.stub_request(:get, jira_api_comment_url).
+            to_return(:body => jira_issue_comments)
+        WebMock.stub_request(:get, jira_api_project_url)
 
         closing_commit.stub({
           issue_closing_regex: Regexp.new(Gitlab.config.gitlab.issue_closing_pattern),
@@ -282,7 +278,7 @@ describe GitPushService do
         }.to_json
 
         service.execute(project, user, @oldrev, @newrev, @ref)
-        WebMock.should have_requested(:post, api_transition_url).with(
+        WebMock.should have_requested(:post, jira_api_transition_url).with(
           body: message
         ).once
       end
@@ -290,7 +286,7 @@ describe GitPushService do
       it "should initiate one api call to jira server to mention the issue" do
         service.execute(project, user, @oldrev, @newrev, @ref)
 
-        WebMock.should have_requested(:post, api_mention_url).with(
+        WebMock.should have_requested(:post, jira_api_comment_url).with(
           body: /mentioned this issue in/
         ).once
       end

spec/support/jira_service_helper.rb

+module JiraServiceHelper
+
+  def jira_service_settings
+    properties = {
+      "title"=>"JIRA tracker",
+      "project_url"=>"http://jira.example/issues/?jql=project=A",
+      "issues_url"=>"http://jira.example/browse/JIRA-1",
+      "new_issue_url"=>"http://jira.example/secure/CreateIssue.jspa"
+    }
+
+    jira_tracker.update_attributes(properties: properties, active: true)
+  end
+
+  def jira_status_message
+    "JiraService SUCCESS 200: Sucessfully posted to #{jira_api_comment_url}."
+  end
+
+  def jira_issue_comments
+    "{\"startAt\":0,\"maxResults\":11,\"total\":11,
+      \"comments\":[{\"self\":\"http://0.0.0.0:4567/rest/api/2/issue/10002/comment/10609\",
+      \"id\":\"10609\",\"author\":{\"self\":\"http://0.0.0.0:4567/rest/api/2/user?username=gitlab\",
+      \"name\":\"gitlab\",\"emailAddress\":\"gitlab@example.com\",
+      \"avatarUrls\":{\"16x16\":\"http://0.0.0.0:4567/secure/useravatar?size=xsmall&avatarId=10122\",
+      \"24x24\":\"http://0.0.0.0:4567/secure/useravatar?size=small&avatarId=10122\",
+      \"32x32\":\"http://0.0.0.0:4567/secure/useravatar?size=medium&avatarId=10122\",
+      \"48x48\":\"http://0.0.0.0:4567/secure/useravatar?avatarId=10122\"},
+      \"displayName\":\"GitLab\",\"active\":true},
+      \"body\":\"[Administrator|http://localhost:3000/u/root] mentioned JIRA-1 in Merge request of [gitlab-org/gitlab-test|http://localhost:3000/gitlab-org/gitlab-test/merge_requests/2].\",
+      \"updateAuthor\":{\"self\":\"http://0.0.0.0:4567/rest/api/2/user?username=gitlab\",\"name\":\"gitlab\",\"emailAddress\":\"gitlab@example.com\",
+      \"avatarUrls\":{\"16x16\":\"http://0.0.0.0:4567/secure/useravatar?size=xsmall&avatarId=10122\",
+      \"24x24\":\"http://0.0.0.0:4567/secure/useravatar?size=small&avatarId=10122\",
+      \"32x32\":\"http://0.0.0.0:4567/secure/useravatar?size=medium&avatarId=10122\",
+      \"48x48\":\"http://0.0.0.0:4567/secure/useravatar?avatarId=10122\"},\"displayName\":\"GitLab\",\"active\":true},
+      \"created\":\"2015-02-12T22:47:07.826+0100\",
+      \"updated\":\"2015-02-12T22:47:07.826+0100\"},
+     {\"self\":\"http://0.0.0.0:4567/rest/api/2/issue/10002/comment/10700\",
+        \"id\":\"10700\",\"author\":{\"self\":\"http://0.0.0.0:4567/rest/api/2/user?username=gitlab\",
+        \"name\":\"gitlab\",\"emailAddress\":\"gitlab@example.com\",
+        \"avatarUrls\":{\"16x16\":\"http://0.0.0.0:4567/secure/useravatar?size=xsmall&avatarId=10122\",
+        \"24x24\":\"http://0.0.0.0:4567/secure/useravatar?size=small&avatarId=10122\",
+        \"32x32\":\"http://0.0.0.0:4567/secure/useravatar?size=medium&avatarId=10122\",
+        \"48x48\":\"http://0.0.0.0:4567/secure/useravatar?avatarId=10122\"},\"displayName\":\"GitLab\",\"active\":true},
+        \"body\":\"[Administrator|http://localhost:3000/u/root] mentioned this issue in [a commit of h5bp/html5-boilerplate|http://localhost:3000/h5bp/html5-boilerplate/commit/2439f77897122fbeee3bfd9bb692d3608848433e].\",
+        \"updateAuthor\":{\"self\":\"http://0.0.0.0:4567/rest/api/2/user?username=gitlab\",\"name\":\"gitlab\",\"emailAddress\":\"gitlab@example.com\",
+        \"avatarUrls\":{\"16x16\":\"http://0.0.0.0:4567/secure/useravatar?size=xsmall&avatarId=10122\",
+        \"24x24\":\"http://0.0.0.0:4567/secure/useravatar?size=small&avatarId=10122\",
+        \"32x32\":\"http://0.0.0.0:4567/secure/useravatar?size=medium&avatarId=10122\",
+        \"48x48\":\"http://0.0.0.0:4567/secure/useravatar?avatarId=10122\"},\"displayName\":\"GitLab\",\"active\":true},
+        \"created\":\"2015-04-01T03:45:55.667+0200\",
+        \"updated\":\"2015-04-01T03:45:55.667+0200\"
+      }
+      ]}"
+  end
+
+  def jira_api_comment_url
+    'http://jira.example/rest/api/2/issue/JIRA-1/comment'
+  end
+
+  def jira_api_transition_url
+    'http://jira.example/rest/api/2/issue/JIRA-1/transitions'
+  end
+
+  def jira_api_project_url
+    'http://jira.example/rest/api/2/project'
+  end
+end