Remove `url_for_new_issue` helperrs-external-issue-tracker-redirect

Now we link to the standard `IssuesController#new` action, and let it
redirect if we're using an external tracker.

5 files changed, 24 insertions, 66 deletions

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index cb1e514c60e..660e0eba06f 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -5,7 +5,7 @@ class Projects::IssuesController < Projects::ApplicationController
   include ToggleAwardEmoji
   include IssuableCollections
 
-  before_action :redirect_to_external_issue_tracker, only: [:index]
+  before_action :redirect_to_external_issue_tracker, only: [:index, :new]
   before_action :module_enabled
   before_action :issue, only: [:edit, :update, :show, :referenced_merge_requests,
                                :related_branches, :can_create_branch]
@@ -203,9 +203,15 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def redirect_to_external_issue_tracker
-    return unless @project.external_issue_tracker
+    external = @project.external_issue_tracker
 
-    redirect_to @project.external_issue_tracker.issues_url
+    return unless external
+
+    if action_name == 'new'
+      redirect_to external.new_issue_path
+    else
+      redirect_to external.issues_url
+    end
   end
 
   # Since iids are implemented only in 6.1
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 5061ccb93a4..2e82b44437b 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_new_issue(project = @project, options = {})
-    return '' if project.nil?
-
-    url =
-      if options[:only_path]
-        project.issues_tracker.new_issue_path
-      else
-        project.issues_tracker.new_issue_url
-      end
-
-    # Ensure we return a valid URL to prevent possible XSS.
-    URI.parse(url).to_s
-  rescue URI::InvalidURIError
-    ''
-  end
-
   def url_for_issue(issue_iid, project = @project, options = {})
     return '' if project.nil?
 
diff --git a/app/views/projects/buttons/_dropdown.html.haml b/app/views/projects/buttons/_dropdown.html.haml
index 16b8e1cca91..ca907077c2b 100644
--- a/app/views/projects/buttons/_dropdown.html.haml
+++ b/app/views/projects/buttons/_dropdown.html.haml
@@ -9,7 +9,7 @@
 
       - if can_create_issue
         %li
-          = link_to url_for_new_issue(@project, only_path: true) do
+          = link_to new_namespace_project_issue_path(@project.namespace, @project) do
             = icon('exclamation-circle fw')
             New issue
 
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index ed31f689d3d..ec820de3d09 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -54,6 +54,20 @@ describe Projects::IssuesController do
     end
   end
 
+  describe 'GET #new' do
+    context 'external issue tracker' do
+      it 'redirects to the external issue tracker' do
+        external = double(new_issue_path: 'https://example.com/issues/new')
+        allow(project).to receive(:external_issue_tracker).and_return(external)
+        controller.instance_variable_set(:@project, project)
+
+        get :new, namespace_id: project.namespace.path, project_id: project
+
+        expect(response).to redirect_to('https://example.com/issues/new')
+      end
+    end
+  end
+
   describe 'PUT #update' do
     context 'when moving issue to another private project' do
       let(:another_project) { create(:project, :private) }
diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb
index ca4aea47413..9ee46dd2508 100644
--- a/spec/helpers/issues_helper_spec.rb
+++ b/spec/helpers/issues_helper_spec.rb
@@ -51,52 +51,6 @@ describe IssuesHelper do
     end
   end
 
-  describe 'url_for_new_issue' do
-    let(:issues_url) { ext_project.external_issue_tracker.new_issue_url }
-    let(:ext_expected) { issues_url.gsub(':project_id', ext_project.id.to_s) }
-    let(:int_expected) { new_namespace_project_issue_path(project.namespace, project) }
-
-    it "should return internal path if used internal tracker" do
-      @project = project
-      expect(url_for_new_issue).to match(int_expected)
-    end
-
-    it "should return path to external tracker" do
-      @project = ext_project
-
-      expect(url_for_new_issue).to match(ext_expected)
-    end
-
-    it "should return empty string if project nil" do
-      @project = nil
-
-      expect(url_for_new_issue).to eq ""
-    end
-
-    it 'returns an empty string if issue_url is invalid' do
-      expect(project).to receive_message_chain('issues_tracker.new_issue_url') { 'javascript:alert("foo");' }
-
-      expect(url_for_new_issue(project)).to eq ''
-    end
-
-    it 'returns an empty string if issue_path is invalid' do
-      expect(project).to receive_message_chain('issues_tracker.new_issue_path') { 'javascript:alert("foo");' }
-
-      expect(url_for_new_issue(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 internal path" do
-        expect(url_for_new_issue).to match(ext_expected)
-      end
-    end
-  end
-
   describe "merge_requests_sentence" do
     subject { merge_requests_sentence(merge_requests)}
     let(:merge_requests) do