Commit b69cc523 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'rs-external-issue-tracker-redirect' into 'master'

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! 🎉

See merge request !5608
parents 8c8bdb79 901d4d2c
...@@ -5,6 +5,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -5,6 +5,7 @@ class Projects::IssuesController < Projects::ApplicationController
include ToggleAwardEmoji include ToggleAwardEmoji
include IssuableCollections include IssuableCollections
before_action :redirect_to_external_issue_tracker, only: [:index, :new]
before_action :module_enabled before_action :module_enabled
before_action :issue, only: [:edit, :update, :show, :referenced_merge_requests, before_action :issue, only: [:edit, :update, :show, :referenced_merge_requests,
:related_branches, :can_create_branch] :related_branches, :can_create_branch]
...@@ -201,6 +202,18 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -201,6 +202,18 @@ class Projects::IssuesController < Projects::ApplicationController
return render_404 unless @project.issues_enabled && @project.default_issues_tracker? return render_404 unless @project.issues_enabled && @project.default_issues_tracker?
end end
def redirect_to_external_issue_tracker
external = @project.external_issue_tracker
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 # Since iids are implemented only in 6.1
# user may navigate to issue page using old global ids. # user may navigate to issue page using old global ids.
# #
......
...@@ -13,38 +13,6 @@ module IssuesHelper ...@@ -13,38 +13,6 @@ module IssuesHelper
OpenStruct.new(id: 0, title: 'None (backlog)', name: 'Unassigned') OpenStruct.new(id: 0, title: 'None (backlog)', name: 'Unassigned')
end 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?
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 = {}) def url_for_issue(issue_iid, project = @project, options = {})
return '' if project.nil? return '' if project.nil?
......
...@@ -66,7 +66,7 @@ ...@@ -66,7 +66,7 @@
- if project_nav_tab? :issues - if project_nav_tab? :issues
= nav_link(controller: [:issues, :labels, :milestones]) do = 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 %span
Issues Issues
- if @project.default_issues_tracker? - if @project.default_issues_tracker?
......
...@@ -9,7 +9,7 @@ ...@@ -9,7 +9,7 @@
- if can_create_issue - if can_create_issue
%li %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') = icon('exclamation-circle fw')
New issue New issue
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
%ul{ class: (container_class) } %ul{ class: (container_class) }
- if project_nav_tab?(:issues) && !current_controller?(:merge_requests) - if project_nav_tab?(:issues) && !current_controller?(:merge_requests)
= nav_link(controller: :issues) do = 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 %span
Issues Issues
......
...@@ -6,37 +6,65 @@ describe Projects::IssuesController do ...@@ -6,37 +6,65 @@ describe Projects::IssuesController do
let(:issue) { create(:issue, project: project) } let(:issue) { create(:issue, project: project) }
describe "GET #index" do describe "GET #index" do
before do context 'external issue tracker' do
sign_in(user) it 'redirects to the external issue tracker' do
project.team << [user, :developer] external = double(issues_url: 'https://example.com/issues')
end 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
get :index, namespace_id: project.namespace.path, project_id: project.path
expect(response).to have_http_status(200) expect(response).to redirect_to('https://example.com/issues')
end
end end
it "return 301 if request path doesn't match project path" do context 'internal issue tracker' do
get :index, namespace_id: project.namespace.path, project_id: project.path.upcase before do
sign_in(user)
project.team << [user, :developer]
end
expect(response).to redirect_to(namespace_project_issues_path(project.namespace, project)) it "returns index" do
end get :index, namespace_id: project.namespace.path, project_id: project.path
expect(response).to have_http_status(200)
end
it "returns 404 when issues are disabled" do it "return 301 if request path doesn't match project path" do
project.issues_enabled = false get :index, namespace_id: project.namespace.path, project_id: project.path.upcase
project.save
get :index, namespace_id: project.namespace.path, project_id: project.path expect(response).to redirect_to(namespace_project_issues_path(project.namespace, project))
expect(response).to have_http_status(404) end
it "returns 404 when issues are disabled" do
project.issues_enabled = false
project.save
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)
end
end 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)
it "returns 404 when external issue tracker is enabled" do get :new, namespace_id: project.namespace.path, project_id: project
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 redirect_to('https://example.com/issues/new')
expect(response).to have_http_status(404) end
end end
end end
......
...@@ -5,52 +5,6 @@ describe IssuesHelper do ...@@ -5,52 +5,6 @@ describe IssuesHelper do
let(:issue) { create :issue, project: project } let(:issue) { create :issue, project: project }
let(:ext_project) { create :redmine_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 describe "url_for_issue" do
let(:issues_url) { ext_project.external_issue_tracker.issues_url} 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(:ext_expected) { issues_url.gsub(':id', issue.iid.to_s).gsub(':project_id', ext_project.id.to_s) }
...@@ -97,52 +51,6 @@ describe IssuesHelper do ...@@ -97,52 +51,6 @@ describe IssuesHelper do
end end
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 describe "merge_requests_sentence" do
subject { merge_requests_sentence(merge_requests)} subject { merge_requests_sentence(merge_requests)}
let(:merge_requests) do let(:merge_requests) do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment