From 901d4d2ca54d173f9c6b1f39c7548ef7fc9e8cd7 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Mon, 1 Aug 2016 18:23:12 -0700
Subject: [PATCH] Remove `url_for_new_issue` helper
Now we link to the standard `IssuesController#new` action, and let it
redirect if we're using an external tracker.
---
app/controllers/projects/issues_controller.rb | 12 +++--
app/helpers/issues_helper.rb | 16 -------
.../projects/buttons/_dropdown.html.haml | 2 +-
.../projects/issues_controller_spec.rb | 14 ++++++
spec/helpers/issues_helper_spec.rb | 46 -------------------
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
--
GitLab