Improve support for external issue referencesadam-external-issue-references-spike

22 files changed, 87 insertions, 118 deletions

diff --git a/app/models/concerns/mentionable/reference_regexes.rb b/app/models/concerns/mentionable/reference_regexes.rb
index 1848230ec7e..2d86a70c395 100644
--- a/app/models/concerns/mentionable/reference_regexes.rb
+++ b/app/models/concerns/mentionable/reference_regexes.rb
@@ -14,7 +14,7 @@ module Mentionable
     end
 
     EXTERNAL_PATTERN = begin
-      issue_pattern = ExternalIssue.reference_pattern
+      issue_pattern = IssueTrackerService.reference_pattern
       link_patterns = URI.regexp(%w(http https))
       reference_pattern(link_patterns, issue_pattern)
     end
diff --git a/app/models/external_issue.rb b/app/models/external_issue.rb
index e63f89a9f85..0bf18e529f0 100644
--- a/app/models/external_issue.rb
+++ b/app/models/external_issue.rb
@@ -38,11 +38,6 @@ class ExternalIssue
     @project.id
   end
 
-  # Pattern used to extract `JIRA-123` issue references from text
-  def self.reference_pattern
-    @reference_pattern ||= %r{(?<issue>\b([A-Z][A-Z0-9_]+-)\d+)}
-  end
-
   def to_reference(_from_project = nil, full: nil)
     id
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index a75c5209955..8140ed3763f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -727,8 +727,8 @@ class Project < ActiveRecord::Base
     end
   end
 
-  def issue_reference_pattern
-    issues_tracker.reference_pattern
+  def external_issue_reference_pattern
+    external_issue_tracker.class.reference_pattern
   end
 
   def default_issues_tracker?
diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb
index ff138b9066d..fcc7c4bec06 100644
--- a/app/models/project_services/issue_tracker_service.rb
+++ b/app/models/project_services/issue_tracker_service.rb
@@ -5,7 +5,10 @@ class IssueTrackerService < Service
 
   # Pattern used to extract links from comments
   # Override this method on services that uses different patterns
-  def reference_pattern
+  # This pattern does not support cross-project references
+  # The other code assumes that this pattern is a superset of all
+  # overriden patterns. See ReferenceRegexes::EXTERNAL_PATTERN
+  def self.reference_pattern
     @reference_pattern ||= %r{(\b[A-Z][A-Z0-9_]+-|#{Issue.reference_prefix})(?<issue>\d+)}
   end
 
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 2450fb43212..00328892b4a 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -18,7 +18,7 @@ class JiraService < IssueTrackerService
   end
 
   # {PROJECT-KEY}-{NUMBER} Examples: JIRA-1, PROJECT-1
-  def reference_pattern
+  def self.reference_pattern
     @reference_pattern ||= %r{(?<issue>\b([A-Z][A-Z0-9_]+-)\d+)}
   end
 
diff --git a/changelogs/unreleased/adam-external-issue-references-spike.yml b/changelogs/unreleased/adam-external-issue-references-spike.yml
new file mode 100644
index 00000000000..aeec6688425
--- /dev/null
+++ b/changelogs/unreleased/adam-external-issue-references-spike.yml
@@ -0,0 +1,4 @@
+---
+title: Improve support for external issue references
+merge_request: 12485
+author:
diff --git a/doc/integration/external-issue-tracker.md b/doc/integration/external-issue-tracker.md
index 265c891cf83..2dd9b33273c 100644
--- a/doc/integration/external-issue-tracker.md
+++ b/doc/integration/external-issue-tracker.md
@@ -8,6 +8,9 @@ you to do the following:
   issue index of the external tracker
 - clicking **New issue** on the project dashboard creates a new issue on the
   external tracker
+- you can reference these external issues inside GitLab interface
+  (merge requests, commits, comments) and they will be automatically converted
+  into links
 
 ## Configuration
 
diff --git a/doc/user/project/integrations/bugzilla.md b/doc/user/project/integrations/bugzilla.md
index 0b219e84478..6a040516231 100644
--- a/doc/user/project/integrations/bugzilla.md
+++ b/doc/user/project/integrations/bugzilla.md
@@ -16,3 +16,14 @@ Once you have configured and enabled Bugzilla:
 - the **Issues** link on the GitLab project pages takes you to the appropriate
   Bugzilla product page
 - clicking **New issue** on the project dashboard takes you to Bugzilla for entering a new issue
+
+## Referencing issues in Bugzilla
+
+Issues in Bugzilla can be referenced in two alternative ways:
+1. `#<ID>` where `<ID>` is a number (example `#143`)
+2. `<PROJECT>-<ID>` where `<PROJECT>` starts with a capital letter which is
+  then followed by capital letters, numbers or underscores, and `<ID>` is
+  a number (example `API_32-143`).
+
+Please note that `<PROJECT>` part is ignored and links always point to the
+address specified in `issues_url`.
diff --git a/doc/user/project/integrations/redmine.md b/doc/user/project/integrations/redmine.md
index 89c0312d3c2..8026f1f57bc 100644
--- a/doc/user/project/integrations/redmine.md
+++ b/doc/user/project/integrations/redmine.md
@@ -21,3 +21,14 @@ Once you have configured and enabled Redmine:
 As an example, below is a configuration for a project named gitlab-ci.
 
 ![Redmine configuration](img/redmine_configuration.png)
+
+## Referencing issues in Redmine
+
+Issues in Redmine can be referenced in two alternative ways:
+1. `#<ID>` where `<ID>` is a number (example `#143`)
+2. `<PROJECT>-<ID>` where `<PROJECT>` starts with a capital letter which is
+  then followed by capital letters, numbers or underscores, and `<ID>` is
+  a number (example `API_32-143`).
+
+Please note that `<PROJECT>` part is ignored and links always point to the
+address specified in `issues_url`.
diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb
index 8bc2dd18bda..7a262dd025c 100644
--- a/lib/banzai/filter/abstract_reference_filter.rb
+++ b/lib/banzai/filter/abstract_reference_filter.rb
@@ -216,12 +216,7 @@ module Banzai
         @references_per_project ||= begin
           refs = Hash.new { |hash, key| hash[key] = Set.new }
 
-          regex =
-            if uses_reference_pattern?
-              Regexp.union(object_class.reference_pattern, object_class.link_reference_pattern)
-            else
-              object_class.link_reference_pattern
-            end
+          regex = Regexp.union(object_class.reference_pattern, object_class.link_reference_pattern)
 
           nodes.each do |node|
             node.to_html.scan(regex) do
@@ -323,14 +318,6 @@ module Banzai
           value
         end
       end
-
-      # There might be special cases like filters
-      # that should ignore reference pattern
-      # eg: IssueReferenceFilter when using a external issues tracker
-      # In those cases this method should be overridden on the filter subclass
-      def uses_reference_pattern?
-        true
-      end
     end
   end
 end
diff --git a/lib/banzai/filter/external_issue_reference_filter.rb b/lib/banzai/filter/external_issue_reference_filter.rb
index dce4de3ceaf..53a229256a5 100644
--- a/lib/banzai/filter/external_issue_reference_filter.rb
+++ b/lib/banzai/filter/external_issue_reference_filter.rb
@@ -3,6 +3,8 @@ module Banzai
     # HTML filter that replaces external issue tracker references with links.
     # References are ignored if the project doesn't use an external issue
     # tracker.
+    #
+    # This filter does not support cross-project references.
     class ExternalIssueReferenceFilter < ReferenceFilter
       self.reference_type = :external_issue
 
@@ -87,7 +89,7 @@ module Banzai
       end
 
       def issue_reference_pattern
-        external_issues_cached(:issue_reference_pattern)
+        external_issues_cached(:external_issue_reference_pattern)
       end
 
       private
diff --git a/lib/banzai/filter/issue_reference_filter.rb b/lib/banzai/filter/issue_reference_filter.rb
index 044d18ff824..ba1a5ac84b3 100644
--- a/lib/banzai/filter/issue_reference_filter.rb
+++ b/lib/banzai/filter/issue_reference_filter.rb
@@ -15,10 +15,6 @@ module Banzai
         Issue
       end
 
-      def uses_reference_pattern?
-        context[:project].default_issues_tracker?
-      end
-
       def find_object(project, iid)
         issues_per_project[project][iid]
       end
@@ -38,13 +34,7 @@ module Banzai
 
           projects_per_reference.each do |path, project|
             issue_ids = references_per_project[path]
-
-            issues =
-              if project.default_issues_tracker?
-                project.issues.where(iid: issue_ids.to_a)
-              else
-                issue_ids.map { |id| ExternalIssue.new(id, project) }
-              end
+            issues = project.issues.where(iid: issue_ids.to_a)
 
             issues.each do |issue|
               hash[project][issue.iid.to_i] = issue
@@ -55,26 +45,6 @@ module Banzai
         end
       end
 
-      def object_link_title(object)
-        if object.is_a?(ExternalIssue)
-          "Issue in #{object.project.external_issue_tracker.title}"
-        else
-          super
-        end
-      end
-
-      def data_attributes_for(text, project, object, link: false)
-        if object.is_a?(ExternalIssue)
-          data_attribute(
-            project: project.id,
-            external_issue: object.id,
-            reference_type: ExternalIssueReferenceFilter.reference_type
-          )
-        else
-          super
-        end
-      end
-
       def projects_relation_for_paths(paths)
         super(paths).includes(:gitlab_issue_tracker_service)
       end
diff --git a/lib/banzai/reference_parser/issue_parser.rb b/lib/banzai/reference_parser/issue_parser.rb
index 9fd4bd68d43..a65bbe23958 100644
--- a/lib/banzai/reference_parser/issue_parser.rb
+++ b/lib/banzai/reference_parser/issue_parser.rb
@@ -4,9 +4,6 @@ module Banzai
       self.reference_type = :issue
 
       def nodes_visible_to_user(user, nodes)
-        # It is not possible to check access rights for external issue trackers
-        return nodes if project && project.external_issue_tracker
-
         issues = issues_for_nodes(nodes)
 
         readable_issues = Ability
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index aef1c17a239..1bb2db11e7f 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -220,7 +220,7 @@ FactoryGirl.define do
         active: true,
         properties: {
           'project_url' => 'http://redmine/projects/project_name_in_redmine',
-          'issues_url' => "http://redmine/#{project.id}/project_name_in_redmine/:id",
+          'issues_url' => 'http://redmine/projects/project_name_in_redmine/issues/:id',
           'new_issue_url' => 'http://redmine/projects/project_name_in_redmine/issues/new'
         }
       )
diff --git a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb
index a4bb043f8f1..b7d82c36ddd 100644
--- a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb
@@ -88,12 +88,12 @@ describe Banzai::Filter::ExternalIssueReferenceFilter, lib: true do
 
       it 'queries the collection on the first call' do
         expect_any_instance_of(Project).to receive(:default_issues_tracker?).once.and_call_original
-        expect_any_instance_of(Project).to receive(:issue_reference_pattern).once.and_call_original
+        expect_any_instance_of(Project).to receive(:external_issue_reference_pattern).once.and_call_original
 
         not_cached = reference_filter.call("look for #{reference}", { project: project })
 
         expect_any_instance_of(Project).not_to receive(:default_issues_tracker?)
-        expect_any_instance_of(Project).not_to receive(:issue_reference_pattern)
+        expect_any_instance_of(Project).not_to receive(:external_issue_reference_pattern)
 
         cached = reference_filter.call("look for #{reference}", { project: project })
 
diff --git a/spec/lib/banzai/filter/issue_reference_filter_spec.rb b/spec/lib/banzai/filter/issue_reference_filter_spec.rb
index e5c1deb338b..a79d365d6c5 100644
--- a/spec/lib/banzai/filter/issue_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/issue_reference_filter_spec.rb
@@ -39,13 +39,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
 
     let(:reference) { "##{issue.iid}" }
 
-    it 'ignores valid references when using non-default tracker' do
-      allow(project).to receive(:default_issues_tracker?).and_return(false)
-
-      exp = act = "Issue #{reference}"
-      expect(reference_filter(act).to_html).to eq exp
-    end
-
     it 'links to a valid reference' do
       doc = reference_filter("Fixed #{reference}")
 
@@ -340,24 +333,6 @@ describe Banzai::Filter::IssueReferenceFilter, lib: true do
           .to eq({ project => { issue.iid => issue } })
       end
     end
-
-    context 'using an external issue tracker' do
-      it 'returns a Hash containing the issues per project' do
-        doc = Nokogiri::HTML.fragment('')
-        filter = described_class.new(doc, project: project)
-
-        expect(project).to receive(:default_issues_tracker?).and_return(false)
-
-        expect(filter).to receive(:projects_per_reference)
-          .and_return({ project.path_with_namespace => project })
-
-        expect(filter).to receive(:references_per_project)
-          .and_return({ project.path_with_namespace => Set.new([1]) })
-
-        expect(filter.issues_per_project[project][1])
-          .to be_an_instance_of(ExternalIssue)
-      end
-    end
   end
 
   describe '.references_in' do
diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
new file mode 100644
index 00000000000..2b8c76f2bb8
--- /dev/null
+++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
@@ -0,0 +1,33 @@
+require 'rails_helper'
+
+describe Banzai::Pipeline::GfmPipeline do
+  describe 'integration between parsing regular and external issue references' do
+    let(:project) { create(:redmine_project, :public) }
+
+    it 'allows to use shorthand external reference syntax for Redmine' do
+      markdown = '#12'
+
+      result = described_class.call(markdown, project: project)[:output]
+      link = result.css('a').first
+
+      expect(link['href']).to eq 'http://redmine/projects/project_name_in_redmine/issues/12'
+    end
+
+    it 'parses cross-project references to regular issues' do
+      other_project = create(:empty_project, :public)
+      issue = create(:issue, project: other_project)
+      markdown = issue.to_reference(project, full: true)
+
+      result = described_class.call(markdown, project: project)[:output]
+      link = result.css('a').first
+
+      expect(link['href']).to eq(
+        Gitlab::Routing.url_helpers.namespace_project_issue_path(
+          other_project.namespace,
+          other_project,
+          issue
+        )
+      )
+    end
+  end
+end
diff --git a/spec/lib/banzai/reference_parser/issue_parser_spec.rb b/spec/lib/banzai/reference_parser/issue_parser_spec.rb
index 58e1a0c1bc1..acdd23f81f3 100644
--- a/spec/lib/banzai/reference_parser/issue_parser_spec.rb
+++ b/spec/lib/banzai/reference_parser/issue_parser_spec.rb
@@ -39,16 +39,6 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do
         expect(subject.nodes_visible_to_user(user, [link])).to eq([])
       end
     end
-
-    context 'when the project uses an external issue tracker' do
-      it 'returns all nodes' do
-        link = double(:link)
-
-        expect(project).to receive(:external_issue_tracker).and_return(true)
-
-        expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
-      end
-    end
   end
 
   describe '#referenced_by' do
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index c86f56c55eb..4a1de76f099 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -64,12 +64,12 @@ describe JiraService, models: true do
     end
   end
 
-  describe '#reference_pattern' do
+  describe '.reference_pattern' do
     it_behaves_like 'allows project key on reference pattern'
 
     it 'does not allow # on the code' do
-      expect(subject.reference_pattern.match('#123')).to be_nil
-      expect(subject.reference_pattern.match('1#23#12')).to be_nil
+      expect(described_class.reference_pattern.match('#123')).to be_nil
+      expect(described_class.reference_pattern.match('1#23#12')).to be_nil
     end
   end
 
diff --git a/spec/models/project_services/redmine_service_spec.rb b/spec/models/project_services/redmine_service_spec.rb
index 6631d9040b1..441b3f896ca 100644
--- a/spec/models/project_services/redmine_service_spec.rb
+++ b/spec/models/project_services/redmine_service_spec.rb
@@ -31,11 +31,11 @@ describe RedmineService, models: true do
     end
   end
 
-  describe '#reference_pattern' do
+  describe '.reference_pattern' do
     it_behaves_like 'allows project key on reference pattern'
 
     it 'does allow # on the reference' do
-      expect(subject.reference_pattern.match('#123')[:issue]).to eq('123')
+      expect(described_class.reference_pattern.match('#123')[:issue]).to eq('123')
     end
   end
 end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index ca827fc0f39..8e8816870e1 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -401,18 +401,6 @@ describe GitPushService, services: true do
         expect(SystemNoteService).not_to receive(:cross_reference)
         execute_service(project, commit_author, @oldrev, @newrev, @ref )
       end
-
-      it "doesn't close issues when external issue tracker is in use" do
-        allow_any_instance_of(Project).to receive(:default_issues_tracker?)
-          .and_return(false)
-        external_issue_tracker = double(title: 'My Tracker', issue_path: issue.iid, reference_pattern: project.issue_reference_pattern)
-        allow_any_instance_of(Project).to receive(:external_issue_tracker).and_return(external_issue_tracker)
-
-        # The push still shouldn't create cross-reference notes.
-        expect do
-          execute_service(project, commit_author, @oldrev, @newrev,  'refs/heads/hurf' )
-        end.not_to change { Note.where(project_id: project.id, system: true).count }
-      end
     end
 
     context "to non-default branches" do
diff --git a/spec/support/issue_tracker_service_shared_example.rb b/spec/support/issue_tracker_service_shared_example.rb
index e70b3963d9d..a6ab03cb808 100644
--- a/spec/support/issue_tracker_service_shared_example.rb
+++ b/spec/support/issue_tracker_service_shared_example.rb
@@ -8,15 +8,15 @@ end
 
 RSpec.shared_examples 'allows project key on reference pattern' do |url_attr|
   it 'allows underscores in the project name' do
-    expect(subject.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
+    expect(described_class.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
   end
 
   it 'allows numbers in the project name' do
-    expect(subject.reference_pattern.match('EXT3_EXT-1234')[0]).to eq 'EXT3_EXT-1234'
+    expect(described_class.reference_pattern.match('EXT3_EXT-1234')[0]).to eq 'EXT3_EXT-1234'
   end
 
   it 'requires the project name to begin with A-Z' do
-    expect(subject.reference_pattern.match('3EXT_EXT-1234')).to eq nil
-    expect(subject.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
+    expect(described_class.reference_pattern.match('3EXT_EXT-1234')).to eq nil
+    expect(described_class.reference_pattern.match('EXT_EXT-1234')[0]).to eq 'EXT_EXT-1234'
   end
 end