Merge branch 'fix-post-receive-external-tracker' into 'master'

Fix post-receive errors on a push when an external issue tracker is configured

### What does this MR do?

This MR improves upon !766, fixing errors upon a git push that occur only when an external issue tracker (not JIRA) is used. This MR takes into account that external issue trackers, such as JIRA, may close issues. Disabling the processing of post-receive commits when an external issue tracker is configured seems like the wrong behavior.

### Why was this MR needed?

When a user adds an issue reference, the refactoring in 8f8a8ab and a6defd157 caused `project.get_issue` to be called, causing `ExternalIssue` to be returned when an external issue tracker was configured. This object does not have a `close` method, as needed in `CloseService`. Nor does it make sense to associate a `SystemNote` with this object.

GitLab EE uses a [special case for JIRA](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/services/git_push_service.rb#L87). I would recommend moving this logic into `CloseService` too.

### What are the relevant issue numbers?

* Closes #1700
* Closes #1720

See merge request !804

CHANGELOG

@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.12.0 (unreleased)
+  - Fix post-receive errors on a push when an external issue tracker is configured (Stan Hu)
   - Update oauth button logos for Twitter and Google to recommended assets
   - Update browser gem to version 0.8.0 for IE11 support (Stan Hu)
   - Fix timeout when rendering file with thousands of lines.

app/services/git_push_service.rb

@@ -88,18 +88,24 @@ class GitPushService
         end
       end
 
-      # Create cross-reference notes for any other references. Omit any issues that were referenced in an
-      # issue-closing phrase, or have already been mentioned from this commit (probably from this commit
-      # being pushed to a different branch).
-      refs = commit.references(project, user) - issues_to_close
-      refs.reject! { |r| commit.has_mentioned?(r) }
+      if project.default_issues_tracker?
+        create_cross_reference_notes(commit, issues_to_close)
+      end
+    end
+  end
 
-      if refs.present?
-        author ||= commit_user(commit)
+  def create_cross_reference_notes(commit, issues_to_close)
+    # Create cross-reference notes for any other references. Omit any issues that were referenced in an
+    # issue-closing phrase, or have already been mentioned from this commit (probably from this commit
+    # being pushed to a different branch).
+    refs = commit.references(project, user) - issues_to_close
+    refs.reject! { |r| commit.has_mentioned?(r) }
 
-        refs.each do |r|
-          Note.create_cross_reference_note(r, commit, author)
-        end
+    if refs.present?
+      author ||= commit_user(commit)
+
+      refs.each do |r|
+        Note.create_cross_reference_note(r, commit, author)
       end
     end
   end

app/services/issues/close_service.rb

@@ -1,7 +1,7 @@
 module Issues
   class CloseService < Issues::BaseService
     def execute(issue, commit = nil)
-      if issue.close
+      if project.default_issues_tracker? && issue.close
         event_service.close_issue(issue, current_user)
         create_note(issue, commit)
         notification_service.close_issue(issue, current_user)

spec/services/git_push_service_spec.rb

@@ -233,6 +233,15 @@ describe GitPushService do
 
       expect(Issue.find(issue.id)).to be_opened
     end
+
+    it "doesn't close issues when external issue tracker is in use" do
+      allow(project).to receive(:default_issues_tracker?).and_return(false)
+
+      # The push still shouldn't create cross-reference notes.
+      expect {
+        service.execute(project, user, @oldrev, @newrev, 'refs/heads/hurf')
+      }.not_to change { Note.where(project_id: project.id, system: true).count }
+    end
   end
 
   describe "empty project" do

spec/services/issues/close_service_spec.rb

@@ -31,5 +31,15 @@ describe Issues::CloseService do
         expect(note.note).to include "Status changed to closed"
       end
     end
+
+    context "external issue tracker" do
+      before do
+        allow(project).to receive(:default_issues_tracker?).and_return(false)
+        @issue = Issues::CloseService.new(project, user, {}).execute(issue)
+      end
+
+      it { expect(@issue).to be_valid }
+      it { expect(@issue).to be_opened }
+    end
   end
 end