Merge branch 'use_url_helpers_for_internal_issues' into 'master'

Use internal issue url unless external tracker

See merge request gitlab-org/gitlab!32330

app/helpers/issues_helper.rb

@@ -9,40 +9,6 @@ module IssuesHelper
     classes.join(' ')
   end
 
-  def url_for_issue(issue_iid, project = @project, options = {})
-    return '' if project.nil?
-
-    url =
-      if options[:internal]
-        url_for_internal_issue(issue_iid, project, options)
-      else
-        url_for_tracker_issue(issue_iid, project, options)
-      end
-
-    # Ensure we return a valid URL to prevent possible XSS.
-    URI.parse(url).to_s
-  rescue URI::InvalidURIError
-    ''
-  end
-
-  def url_for_tracker_issue(issue_iid, project, options)
-    if options[:only_path]
-      project.issues_tracker.issue_path(issue_iid)
-    else
-      project.issues_tracker.issue_url(issue_iid)
-    end
-  end
-
-  def url_for_internal_issue(issue_iid, project = @project, options = {})
-    helpers = Gitlab::Routing.url_helpers
-
-    if options[:only_path]
-      helpers.namespace_project_issue_path(namespace_id: project.namespace, project_id: project, id: issue_iid)
-    else
-      helpers.namespace_project_issue_url(namespace_id: project.namespace, project_id: project, id: issue_iid)
-    end
-  end
-
   def status_box_class(item)
     if item.try(:expired?)
       'status-box-expired'
@@ -168,11 +134,6 @@ module IssuesHelper
   def show_moved_service_desk_issue_warning?(issue)
     false
   end
-
-  # Required for Banzai::Filter::IssueReferenceFilter
-  module_function :url_for_issue
-  module_function :url_for_internal_issue
-  module_function :url_for_tracker_issue
 end
 
 IssuesHelper.prepend_if_ee('EE::IssuesHelper')

ee/spec/services/epic_issues/create_service_spec.rb

@@ -153,7 +153,7 @@ describe EpicIssues::CreateService do
           end
 
           context 'when an issue link is given' do
-            subject { assign_issue([IssuesHelper.url_for_issue(issue.iid, issue.project)]) }
+            subject { assign_issue([Gitlab::Routing.url_helpers.namespace_project_issue_url(namespace_id: issue.project.namespace, project_id: issue.project, id: issue.iid)])}
 
             include_examples 'returns success'
           end
@@ -163,7 +163,7 @@ describe EpicIssues::CreateService do
             let(:project2) { create(:project, group: subgroup) }
             let(:issue) { create(:issue, project: project2) }
 
-            subject { assign_issue([IssuesHelper.url_for_issue(issue.iid, issue.project)]) }
+            subject { assign_issue([Gitlab::Routing.url_helpers.namespace_project_issue_url(namespace_id: issue.project.namespace, project_id: issue.project, id: issue.iid)])}
 
             include_examples 'returns success'
           end

lib/banzai/filter/external_issue_reference_filter.rb

@@ -66,7 +66,7 @@ module Banzai
       # links have `gfm` and `gfm-issue` class names attached for styling.
       def issue_link_filter(text, link_content: nil)
         self.class.references_in(text, issue_reference_pattern) do |match, id|
-          url = url_for_issue(id, project, only_path: context[:only_path])
+          url = url_for_issue(id)
           klass = reference_class(:issue)
           data  = data_attribute(project: project.id, external_issue: id)
           content = link_content || match
@@ -77,8 +77,19 @@ module Banzai
         end
       end
 
-      def url_for_issue(*args)
-        IssuesHelper.url_for_issue(*args)
+      def url_for_issue(issue_id)
+        return '' if project.nil?
+
+        url = if only_path?
+                project.external_issue_tracker.issue_path(issue_id)
+              else
+                project.external_issue_tracker.issue_url(issue_id)
+              end
+
+        # Ensure we return a valid URL to prevent possible XSS.
+        URI.parse(url).to_s
+      rescue URI::InvalidURIError
+        ''
       end
 
       def default_issues_tracker?

lib/banzai/filter/issue_reference_filter.rb

@@ -18,7 +18,9 @@ module Banzai
       end
 
       def url_for_object(issue, project)
-        IssuesHelper.url_for_issue(issue.iid, project, only_path: context[:only_path], internal: true)
+        return issue_path(issue, project) if only_path?
+
+        issue_url(issue, project)
       end
 
       def projects_relation_for_paths(paths)
@@ -35,6 +37,14 @@ module Banzai
 
       private
 
+      def issue_path(issue, project)
+        Gitlab::Routing.url_helpers.namespace_project_issue_path(namespace_id: project.namespace, project_id: project, id: issue.iid)
+      end
+
+      def issue_url(issue, project)
+        Gitlab::Routing.url_helpers.namespace_project_issue_url(namespace_id: project.namespace, project_id: project, id: issue.iid)
+      end
+
       def design_link_extras(issue, path)
         if path == '/designs' && read_designs?(issue)
           ['designs']

lib/banzai/filter/reference_filter.rb

@@ -142,6 +142,12 @@ module Banzai
       def element_node?(node)
         node.is_a?(Nokogiri::XML::Element)
       end
+
+      private
+
+      def only_path?
+        context[:only_path]
+      end
     end
   end
 end

spec/helpers/issues_helper_spec.rb

@@ -7,59 +7,6 @@ describe IssuesHelper do
   let(:issue) { create :issue, project: project }
   let(:ext_project) { create :redmine_project }
 
-  describe "url_for_issue" do
-    let(:issues_url) { ext_project.external_issue_tracker.issues_url}
-    let(:ext_expected) { issues_url.gsub(':id', issue.iid.to_s).gsub(':project_id', ext_project.id.to_s) }
-    let(:int_expected) { polymorphic_path([@project.namespace, @project, issue]) }
-
-    it "returns internal path if used internal tracker" do
-      @project = project
-
-      expect(url_for_issue(issue.iid)).to match(int_expected)
-    end
-
-    it "returns path to external tracker" do
-      @project = ext_project
-
-      expect(url_for_issue(issue.iid)).to match(ext_expected)
-    end
-
-    it "returns path to internal issue when internal option passed" do
-      @project = ext_project
-
-      expect(url_for_issue(issue.iid, ext_project, internal: true)).to match(int_expected)
-    end
-
-    it "returns empty string if project nil" do
-      @project = nil
-
-      expect(url_for_issue(issue.iid)).to eq ""
-    end
-
-    it 'returns an empty string if issue_url is invalid' do
-      expect(project).to receive_message_chain('issues_tracker.issue_url') { 'javascript:alert("foo");' }
-
-      expect(url_for_issue(issue.iid, project)).to eq ''
-    end
-
-    it 'returns an empty string if issue_path is invalid' do
-      expect(project).to receive_message_chain('issues_tracker.issue_path') { 'javascript:alert("foo");' }
-
-      expect(url_for_issue(issue.iid, project, only_path: true)).to eq ''
-    end
-
-    describe "when external tracker was enabled and then config removed" do
-      before do
-        @project = ext_project
-        allow(Gitlab.config).to receive(:issues_tracker).and_return(nil)
-      end
-
-      it "returns external path" do
-        expect(url_for_issue(issue.iid)).to match(ext_expected)
-      end
-    end
-  end
-
   describe '#award_user_list' do
     it "returns a comma-separated list of the first X users" do
       user = build_stubbed(:user, name: 'Joe')

spec/lib/banzai/filter/external_issue_reference_filter_spec.rb

@@ -5,10 +5,6 @@ require 'spec_helper'
 describe Banzai::Filter::ExternalIssueReferenceFilter do
   include FilterSpecHelper
 
-  def helper
-    IssuesHelper
-  end
-
   shared_examples_for "external issue tracker" do
     it_behaves_like 'a reference containing an element node'
 
@@ -36,7 +32,7 @@ describe Banzai::Filter::ExternalIssueReferenceFilter do
       issue_id = doc.css('a').first.attr("data-external-issue")
 
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(issue_id, project)
+        .to eq project.external_issue_tracker.issue_url(issue_id)
     end
 
     it 'links to the external tracker' do
@@ -45,7 +41,7 @@ describe Banzai::Filter::ExternalIssueReferenceFilter do
       link = doc.css('a').first.attr('href')
       issue_id = doc.css('a').first.attr("data-external-issue")
 
-      expect(link).to eq(helper.url_for_issue(issue_id, project))
+      expect(link).to eq(project.external_issue_tracker.issue_url(issue_id))
     end
 
     it 'links with adjacent text' do
@@ -78,7 +74,25 @@ describe Banzai::Filter::ExternalIssueReferenceFilter do
       link = doc.css('a').first.attr('href')
       issue_id = doc.css('a').first["data-external-issue"]
 
-      expect(link).to eq helper.url_for_issue(issue_id, project, only_path: true)
+      expect(link).to eq project.external_issue_tracker.issue_path(issue_id)
+    end
+
+    it 'has an empty link if issue_url is invalid' do
+      expect_any_instance_of(project.external_issue_tracker.class).to receive(:issue_url) { 'javascript:alert("foo");' }
+
+      doc = filter("Issue #{reference}")
+      link = doc.css('a').first.attr('href')
+
+      expect(link).to eq ''
+    end
+
+    it 'has an empty link if issue_path is invalid' do
+      expect_any_instance_of(project.external_issue_tracker.class).to receive(:issue_path) { 'javascript:alert("foo");' }
+
+      doc = filter("Issue #{reference}", only_path: true)
+      link = doc.css('a').first.attr('href')
+
+      expect(link).to eq ''
     end
 
     context 'with RequestStore enabled', :request_store do

spec/lib/banzai/filter/issue_reference_filter_spec.rb

@@ -11,7 +11,9 @@ describe Banzai::Filter::IssueReferenceFilter do
   end
 
   let(:project) { create(:project, :public) }
-  let(:issue)  { create(:issue, project: project) }
+  let(:issue) { create(:issue, project: project) }
+  let(:issue_path) { "/#{issue.project.namespace.path}/#{issue.project.path}/-/issues/#{issue.iid}" }
+  let(:issue_url) { "http://#{Gitlab.config.gitlab.host}#{issue_path}" }
 
   it 'requires project context' do
     expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
@@ -46,7 +48,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       doc = reference_filter("Fixed #{reference}")
 
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(issue.iid, project)
+        .to eq issue_url
     end
 
     it 'links with adjacent text' do
@@ -113,7 +115,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       link = doc.css('a').first.attr('href')
 
       expect(link).not_to match %r(https?://)
-      expect(link).to eq helper.url_for_issue(issue.iid, project, only_path: true)
+      expect(link).to eq issue_path
     end
 
     it 'does not process links containing issue numbers followed by text' do
@@ -145,7 +147,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       doc = reference_filter("See #{reference}")
 
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(issue.iid, project2)
+        .to eq issue_url
     end
 
     it 'link has valid text' do
@@ -195,7 +197,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       doc = reference_filter("See #{reference}")
 
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(issue.iid, project2)
+        .to eq issue_url
     end
 
     it 'link has valid text' do
@@ -245,7 +247,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       doc = reference_filter("See #{reference}")
 
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(issue.iid, project2)
+        .to eq issue_url
     end
 
     it 'link has valid text' do
@@ -279,7 +281,7 @@ describe Banzai::Filter::IssueReferenceFilter do
     let(:namespace) { create(:namespace, name: 'cross-reference') }
     let(:project2)  { create(:project, :public, namespace: namespace) }
     let(:issue)     { create(:issue, project: project2) }
-    let(:reference) { helper.url_for_issue(issue.iid, project2) + "#note_123" }
+    let(:reference) { issue_url + "#note_123" }
 
     it 'links to a valid reference' do
       doc = reference_filter("See #{reference}")
@@ -314,7 +316,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       doc = reference_filter("See #{reference_link}")
 
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(issue.iid, project2)
+        .to eq issue_url
     end
 
     it 'links with adjacent text' do
@@ -336,14 +338,14 @@ describe Banzai::Filter::IssueReferenceFilter do
     let(:namespace) { create(:namespace, name: 'cross-reference') }
     let(:project2)  { create(:project, :public, namespace: namespace) }
     let(:issue)     { create(:issue, project: project2) }
-    let(:reference) { "#{helper.url_for_issue(issue.iid, project2) + "#note_123"}" }
+    let(:reference) { "#{issue_url + "#note_123"}" }
     let(:reference_link) { %{<a href="#{reference}">Reference</a>} }
 
     it 'links to a valid reference' do
       doc = reference_filter("See #{reference_link}")
 
       expect(doc.css('a').first.attr('href'))
-        .to eq helper.url_for_issue(issue.iid, project2) + "#note_123"
+        .to eq issue_url + "#note_123"
     end
 
     it 'links with adjacent text' do
@@ -413,7 +415,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       doc = reference_filter("See #{reference}", context)
 
       link = doc.css('a').first
-      expect(link.attr('href')).to eq(helper.url_for_issue(issue.iid, project))
+      expect(link.attr('href')).to eq(issue_url)
       expect(link.text).to include("#{project.full_path}##{issue.iid}")
     end
 
@@ -425,23 +427,23 @@ describe Banzai::Filter::IssueReferenceFilter do
     end
 
     it 'links to a valid reference for url cross-reference' do
-      reference = helper.url_for_issue(issue.iid, project) + "#note_123"
+      reference = issue_url + "#note_123"
 
       doc = reference_filter("See #{reference}", context)
 
       link = doc.css('a').first
-      expect(link.attr('href')).to eq(helper.url_for_issue(issue.iid, project) + "#note_123")
+      expect(link.attr('href')).to eq(issue_url + "#note_123")
       expect(link.text).to include("#{project.full_path}##{issue.iid}")
     end
 
     it 'links to a valid reference for cross-reference in link href' do
-      reference = "#{helper.url_for_issue(issue.iid, project) + "#note_123"}"
+      reference = "#{issue_url + "#note_123"}"
       reference_link = %{<a href="#{reference}">Reference</a>}
 
       doc = reference_filter("See #{reference_link}", context)
 
       link = doc.css('a').first
-      expect(link.attr('href')).to eq(helper.url_for_issue(issue.iid, project) + "#note_123")
+      expect(link.attr('href')).to eq(issue_url + "#note_123")
       expect(link.text).to include('Reference')
     end
 
@@ -451,7 +453,7 @@ describe Banzai::Filter::IssueReferenceFilter do
       doc = reference_filter("See #{reference_link}", context)
 
       link = doc.css('a').first
-      expect(link.attr('href')).to eq(helper.url_for_issue(issue.iid, project))
+      expect(link.attr('href')).to eq(issue_url)
       expect(link.text).to include('Reference')
     end
   end

spec/requests/api/markdown_spec.rb

@@ -52,6 +52,7 @@ describe API::Markdown do
     context "when arguments are valid" do
       let_it_be(:project) { create(:project) }
       let_it_be(:issue) { create(:issue, project: project) }
+      let(:issue_url) { "http://#{Gitlab.config.gitlab.host}/#{issue.project.namespace.path}/#{issue.project.path}/-/issues/#{issue.iid}" }
       let(:text) { ":tada: Hello world! :100: #{issue.to_reference}" }
 
       context "when not using gfm" do
@@ -88,7 +89,7 @@ describe API::Markdown do
                                         .and include('data-name="tada"')
                                         .and include('data-name="100"')
                                         .and include("#1")
-                                        .and exclude("<a href=\"#{IssuesHelper.url_for_issue(issue.iid, project)}\"")
+                                        .and exclude("<a href=\"#{issue_url}\"")
                                         .and exclude("#1</a>")
           end
         end
@@ -104,16 +105,16 @@ describe API::Markdown do
             expect(json_response["html"]).to include("Hello world!")
                                         .and include('data-name="tada"')
                                         .and include('data-name="100"')
-                                        .and include("<a href=\"#{IssuesHelper.url_for_issue(issue.iid, project)}\"")
+                                        .and include("<a href=\"#{issue_url}\"")
                                         .and include("#1</a>")
           end
         end
 
         context 'with a public project and confidential issue' do
-          let(:public_project)     { create(:project, :public) }
-          let(:confidential_issue) { create(:issue, :confidential, project: public_project, title: 'Confidential title') }
+          let(:public_project) { create(:project, :public) }
+          let(:issue) { create(:issue, :confidential, project: public_project, title: 'Confidential title') }
 
-          let(:text)   { ":tada: Hello world! :100: #{confidential_issue.to_reference}" }
+          let(:text)   { ":tada: Hello world! :100: #{issue.to_reference}" }
           let(:params) { { text: text, gfm: true, project: public_project.full_path } }
 
           shared_examples 'user without proper access' do
@@ -141,7 +142,7 @@ describe API::Markdown do
           end
 
           context 'when logged in as author' do
-            let(:user) { confidential_issue.author }
+            let(:user) { issue.author }
 
             it 'renders the title or link' do
               expect(response).to have_gitlab_http_status(:created)
@@ -149,7 +150,7 @@ describe API::Markdown do
               expect(json_response["html"]).to include('Hello world!')
                                           .and include('data-name="tada"')
                                           .and include('data-name="100"')
-                                          .and include("<a href=\"#{IssuesHelper.url_for_issue(confidential_issue.iid, public_project)}\"")
+                                          .and include("<a href=\"#{issue_url}\"")
                                           .and include("#1</a>")
             end
           end