Merge branch 'jprovazn-fix-references' into 'master'

Fix reference filters in group context

See merge request gitlab-org/gitlab-ce!18340

changelogs/unreleased/fix-references-in-group-context.yml

+---
+title: Ignore project internal references in group context
+merge_request:
+author:
+type: fixed

lib/banzai/filter/abstract_reference_filter.rb

@@ -56,29 +56,29 @@ module Banzai
       # Implement in child class
       # Example: project.merge_requests.find
-      def find_object(project, id)
+      def find_object(parent_object, id)
       end
       # Override if the link reference pattern produces a different ID (global
       # ID vs internal ID, for instance) to the regular reference pattern.
-      def find_object_from_link(project, id)
-        find_object(project, id)
+      def find_object_from_link(parent_object, id)
+        find_object(parent_object, id)
       end
       # Implement in child class
       # Example: project_merge_request_url
-      def url_for_object(object, project)
+      def url_for_object(object, parent_object)
       end
-      def find_object_cached(project, id)
-        cached_call(:banzai_find_object, id, path: [object_class, project.id]) do
-          find_object(project, id)
+      def find_object_cached(parent_object, id)
+        cached_call(:banzai_find_object, id, path: [object_class, parent_object.id]) do
+          find_object(parent_object, id)
         end
       end
-      def find_object_from_link_cached(project, id)
-        cached_call(:banzai_find_object_from_link, id, path: [object_class, project.id]) do
-          find_object_from_link(project, id)
+      def find_object_from_link_cached(parent_object, id)
+        cached_call(:banzai_find_object_from_link, id, path: [object_class, parent_object.id]) do
+          find_object_from_link(parent_object, id)
         end
       end
@@ -88,9 +88,9 @@ module Banzai
         end
       end
-      def url_for_object_cached(object, project)
-        cached_call(:banzai_url_for_object, object, path: [object_class, project.id]) do
-          url_for_object(object, project)
+      def url_for_object_cached(object, parent_object)
+        cached_call(:banzai_url_for_object, object, path: [object_class, parent_object.id]) do
+          url_for_object(object, parent_object)
         end
       end

lib/banzai/filter/commit_range_reference_filter.rb

@@ -23,6 +23,8 @@ module Banzai
       end
       def find_object(project, id)
+        return unless project.is_a?(Project)
+
         range = CommitRange.new(id, project)
         range.valid_commits? ? range : nil

lib/banzai/filter/commit_reference_filter.rb

@@ -17,6 +17,8 @@ module Banzai
       end
       def find_object(project, id)
+        return unless project.is_a?(Project)
+
         if project && project.valid_repo?
           # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/43894
           Gitlab::GitalyClient.allow_n_plus_1_calls { project.commit(id) }

lib/banzai/filter/label_reference_filter.rb

@@ -8,8 +8,8 @@ module Banzai
         Label
       end
-      def find_object(project, id)
-        find_labels(project).find(id)
+      def find_object(parent_object, id)
+        find_labels(parent_object).find(id)
       end
       def self.references_in(text, pattern = Label.reference_pattern)

lib/banzai/filter/milestone_reference_filter.rb

@@ -12,10 +12,14 @@ module Banzai
       # 'regular' references, we need to use the global ID to disambiguate
       # between group and project milestones.
       def find_object(project, id)
+        return unless project.is_a?(Project)
+
         find_milestone_with_finder(project, id: id)
       end
       def find_object_from_link(project, iid)
+        return unless project.is_a?(Project)
+
         find_milestone_with_finder(project, iid: iid)
       end
@@ -40,7 +44,7 @@ module Banzai
         project_path = full_project_path(namespace_ref, project_ref)
         project = parent_from_ref(project_path)
-        return unless project
+        return unless project && project.is_a?(Project)
         milestone_params = milestone_params(milestone_id, milestone_name)

lib/banzai/filter/snippet_reference_filter.rb

@@ -12,6 +12,8 @@ module Banzai
       end
       def find_object(project, id)
+        return unless project.is_a?(Project)
+
         project.snippets.find_by(id: id)
       end

lib/banzai/reference_parser/commit_range_parser.rb

@@ -29,6 +29,8 @@ module Banzai
       end
       def find_object(project, id)
+        return unless project.is_a?(Project)
+
         range = CommitRange.new(id, project)
         range.valid_commits? ? range : nil

spec/lib/banzai/filter/commit_range_reference_filter_spec.rb

@@ -233,4 +233,20 @@ describe Banzai::Filter::CommitRangeReferenceFilter do
       expect(reference_filter(act).to_html).to eq exp
     end
   end
+
+  context 'group context' do
+    let(:context) { { project: nil, group: create(:group) } }
+
+    it 'ignores internal references' do
+      exp = act = "See #{range.to_reference}"
+
+      expect(reference_filter(act, context).to_html).to eq exp
+    end
+
+    it 'links to a full-path reference' do
+      reference = "#{project.full_path}@#{commit1.short_id}...#{commit2.short_id}"
+
+      expect(reference_filter("See #{reference}", context).css('a').first.text).to eql(reference)
+    end
+  end
 end

spec/lib/banzai/filter/commit_reference_filter_spec.rb

@@ -238,4 +238,20 @@ describe Banzai::Filter::CommitReferenceFilter do
       expect(doc.text).to eq("See (#{commit.reference_link_text(project)} (builds).patch)")
     end
   end
+
+  context 'group context' do
+    let(:context) { { project: nil, group: create(:group) } }
+
+    it 'ignores internal references' do
+      exp = act = "See #{commit.id}"
+
+      expect(reference_filter(act, context).to_html).to eq exp
+    end
+
+    it 'links to a valid reference' do
+      act = "See #{project.full_path}@#{commit.id}"
+
+      expect(reference_filter(act, context).css('a').first.text).to eql("#{project.full_path}@#{commit.short_id}")
+    end
+  end
 end

spec/lib/banzai/filter/milestone_reference_filter_spec.rb

@@ -343,14 +343,22 @@ describe Banzai::Filter::MilestoneReferenceFilter do
   end
   context 'group context' do
+    let(:context) { { project: nil, group: create(:group) } }
+    let(:milestone) { create(:milestone, project: project) }
+
     it 'links to a valid reference' do
-      milestone = create(:milestone, project: project)
       reference = "#{project.full_path}%#{milestone.iid}"
-      result = reference_filter("See #{reference}", { project: nil, group: create(:group) } )
+      result = reference_filter("See #{reference}", context)
       expect(result.css('a').first.attr('href')).to eq(urls.milestone_url(milestone))
     end
+
+    it 'ignores internal references' do
+      exp = act = "See %#{milestone.iid}"
+
+      expect(reference_filter(act, context).to_html).to eq exp
+    end
   end
   context 'when milestone is open' do

spec/lib/banzai/filter/snippet_reference_filter_spec.rb

@@ -210,5 +210,11 @@ describe Banzai::Filter::SnippetReferenceFilter do
       expect(result.css('a').first.attr('href')).to eq(urls.project_snippet_url(project, snippet))
     end
+
+    it 'ignores internal references' do
+      exp = act = "See $#{snippet.id}"
+
+      expect(reference_filter(act, project: nil, group: create(:group)).to_html).to eq exp
+    end
   end
 end

spec/lib/banzai/reference_parser/commit_range_parser_spec.rb

@@ -107,12 +107,9 @@ describe Banzai::ReferenceParser::CommitRangeParser do
   describe '#find_object' do
     let(:range) { double(:range) }
-    before do
-      expect(CommitRange).to receive(:new).and_return(range)
-    end
-
     context 'when the range has valid commits' do
       it 'returns the commit range' do
+        expect(CommitRange).to receive(:new).and_return(range)
         expect(range).to receive(:valid_commits?).and_return(true)
         expect(subject.find_object(project, '123..456')).to eq(range)
@@ -121,10 +118,19 @@ describe Banzai::ReferenceParser::CommitRangeParser do
     context 'when the range does not have any valid commits' do
       it 'returns nil' do
+        expect(CommitRange).to receive(:new).and_return(range)
         expect(range).to receive(:valid_commits?).and_return(false)
         expect(subject.find_object(project, '123..456')).to be_nil
       end
     end
+
+    context 'group context' do
+      it 'returns nil' do
+        group = create(:group)
+
+        expect(subject.find_object(group, '123..456')).to be_nil
+      end
+    end
   end
 end