Redirect to external issue tracker from `/issues`

Prior, in order to display the correct link to "Issues" in the project
navigation, we were performing a check against the project to see if it
used an external issue tracker, and if so, we used that URL. This was
inefficient.

Now, we simply _always_ link to `namespace_project_issues_path`, and
then in the controller we redirect to the external tracker if it's
present.

This also removes the need for the url_for_issue helper. Bonus! :tada:

6 files changed, 45 insertions, 86 deletions

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 7f5c3ff3d6a..cb1e514c60e 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -5,6 +5,7 @@ class Projects::IssuesController < Projects::ApplicationController
   include ToggleAwardEmoji
   include IssuableCollections
 
+  before_action :redirect_to_external_issue_tracker, only: [:index]
   before_action :module_enabled
   before_action :issue, only: [:edit, :update, :show, :referenced_merge_requests,
                                :related_branches, :can_create_branch]
@@ -201,6 +202,12 @@ class Projects::IssuesController < Projects::ApplicationController
     return render_404 unless @project.issues_enabled && @project.default_issues_tracker?
   end
 
+  def redirect_to_external_issue_tracker
+    return unless @project.external_issue_tracker
+
+    redirect_to @project.external_issue_tracker.issues_url
+  end
+
   # Since iids are implemented only in 6.1
   # user may navigate to issue page using old global ids.
   #
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 2b0defd1dda..5061ccb93a4 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -13,22 +13,6 @@ module IssuesHelper
     OpenStruct.new(id: 0, title: 'None (backlog)', name: 'Unassigned')
   end
 
-  def url_for_project_issues(project = @project, options = {})
-    return '' if project.nil?
-
-    url =
-      if options[:only_path]
-        project.issues_tracker.project_path
-      else
-        project.issues_tracker.project_url
-      end
-
-    # Ensure we return a valid URL to prevent possible XSS.
-    URI.parse(url).to_s
-  rescue URI::InvalidURIError
-    ''
-  end
-
   def url_for_new_issue(project = @project, options = {})
     return '' if project.nil?
 
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index 9e65d94186b..1d3b8fc3683 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -66,7 +66,7 @@
 
     - if project_nav_tab? :issues
       = nav_link(controller: [:issues, :labels, :milestones]) do
-        = link_to url_for_project_issues(@project, only_path: true), title: 'Issues', class: 'shortcuts-issues' do
+        = link_to namespace_project_issues_path(@project.namespace, @project), title: 'Issues', class: 'shortcuts-issues' do
           %span
             Issues
             - if @project.default_issues_tracker?
diff --git a/app/views/projects/issues/_head.html.haml b/app/views/projects/issues/_head.html.haml
index 403adb7426b..60b45115b73 100644
--- a/app/views/projects/issues/_head.html.haml
+++ b/app/views/projects/issues/_head.html.haml
@@ -2,7 +2,7 @@
   %ul{ class: (container_class) }
     - if project_nav_tab?(:issues) && !current_controller?(:merge_requests)
       = nav_link(controller: :issues) do
-        = link_to url_for_project_issues(@project, only_path: true), title: 'Issues' do
+        = link_to namespace_project_issues_path(@project.namespace, @project), title: 'Issues' do
           %span
             Issues
 
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 77f65057f71..ed31f689d3d 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -6,37 +6,51 @@ describe Projects::IssuesController do
   let(:issue)   { create(:issue, project: project) }
 
   describe "GET #index" do
-    before do
-      sign_in(user)
-      project.team << [user, :developer]
-    end
+    context 'external issue tracker' do
+      it 'redirects to the external issue tracker' do
+        external = double(issues_url: 'https://example.com/issues')
+        allow(project).to receive(:external_issue_tracker).and_return(external)
+        controller.instance_variable_set(:@project, project)
 
-    it "returns index" do
-      get :index, namespace_id: project.namespace.path, project_id: project.path
+        get :index, namespace_id: project.namespace.path, project_id: project
 
-      expect(response).to have_http_status(200)
+        expect(response).to redirect_to('https://example.com/issues')
+      end
     end
 
-    it "return 301 if request path doesn't match project path" do
-      get :index, namespace_id: project.namespace.path, project_id: project.path.upcase
+    context 'internal issue tracker' do
+      before do
+        sign_in(user)
+        project.team << [user, :developer]
+      end
 
-      expect(response).to redirect_to(namespace_project_issues_path(project.namespace, project))
-    end
+      it "returns index" do
+        get :index, namespace_id: project.namespace.path, project_id: project.path
 
-    it "returns 404 when issues are disabled" do
-      project.issues_enabled = false
-      project.save
+        expect(response).to have_http_status(200)
+      end
 
-      get :index, namespace_id: project.namespace.path, project_id: project.path
-      expect(response).to have_http_status(404)
-    end
+      it "return 301 if request path doesn't match project path" do
+        get :index, namespace_id: project.namespace.path, project_id: project.path.upcase
+
+        expect(response).to redirect_to(namespace_project_issues_path(project.namespace, project))
+      end
+
+      it "returns 404 when issues are disabled" do
+        project.issues_enabled = false
+        project.save
 
-    it "returns 404 when external issue tracker is enabled" do
-      controller.instance_variable_set(:@project, project)
-      allow(project).to receive(:default_issues_tracker?).and_return(false)
+        get :index, namespace_id: project.namespace.path, project_id: project.path
+        expect(response).to have_http_status(404)
+      end
+
+      it "returns 404 when external issue tracker is enabled" do
+        controller.instance_variable_set(:@project, project)
+        allow(project).to receive(:default_issues_tracker?).and_return(false)
 
-      get :index, namespace_id: project.namespace.path, project_id: project.path
-      expect(response).to have_http_status(404)
+        get :index, namespace_id: project.namespace.path, project_id: project.path
+        expect(response).to have_http_status(404)
+      end
     end
   end
 
diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb
index 831ae7fb69c..ca4aea47413 100644
--- a/spec/helpers/issues_helper_spec.rb
+++ b/spec/helpers/issues_helper_spec.rb
@@ -5,52 +5,6 @@ describe IssuesHelper do
   let(:issue) { create :issue, project: project }
   let(:ext_project) { create :redmine_project }
 
-  describe "url_for_project_issues" do
-    let(:project_url) { ext_project.external_issue_tracker.project_url }
-    let(:ext_expected) { project_url.gsub(':project_id', ext_project.id.to_s) }
-    let(:int_expected) { polymorphic_path([@project.namespace, project]) }
-
-    it "should return internal path if used internal tracker" do
-      @project = project
-      expect(url_for_project_issues).to match(int_expected)
-    end
-
-    it "should return path to external tracker" do
-      @project = ext_project
-
-      expect(url_for_project_issues).to match(ext_expected)
-    end
-
-    it "should return empty string if project nil" do
-      @project = nil
-
-      expect(url_for_project_issues).to eq ""
-    end
-
-    it 'returns an empty string if project_url is invalid' do
-      expect(project).to receive_message_chain('issues_tracker.project_url') { 'javascript:alert("foo");' }
-
-      expect(url_for_project_issues(project)).to eq ''
-    end
-
-    it 'returns an empty string if project_path is invalid' do
-      expect(project).to receive_message_chain('issues_tracker.project_path') { 'javascript:alert("foo");' }
-
-      expect(url_for_project_issues(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 "should return path to external tracker" do
-        expect(url_for_project_issues).to match(ext_expected)
-      end
-    end
-  end
-
   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) }