Commit 58d1d6a5 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Free up some group reserved words

parent 5dde0536
...@@ -8,45 +8,46 @@ constraints(GroupUrlConstrainer.new) do ...@@ -8,45 +8,46 @@ constraints(GroupUrlConstrainer.new) do
scope(path: 'groups/*id', scope(path: 'groups/*id',
controller: :groups, controller: :groups,
constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) do constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) do
get :edit, as: :edit_group scope(path: '-') do
get :issues, as: :issues_group get :edit, as: :edit_group
get :merge_requests, as: :merge_requests_group get :issues, as: :issues_group
get :projects, as: :projects_group get :merge_requests, as: :merge_requests_group
get :activity, as: :activity_group get :projects, as: :projects_group
get :activity, as: :activity_group
end
get '/', action: :show, as: :group_canonical get '/', action: :show, as: :group_canonical
end end
scope(path: 'groups/*group_id', scope(path: 'groups/*group_id/-',
module: :groups, module: :groups,
as: :group, as: :group,
constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do
scope path: '-' do namespace :settings do
namespace :settings do resource :ci_cd, only: [:show], controller: 'ci_cd'
resource :ci_cd, only: [:show], controller: 'ci_cd' end
end
resources :variables, only: [:index, :show, :update, :create, :destroy] resources :variables, only: [:index, :show, :update, :create, :destroy]
resources :children, only: [:index] resources :children, only: [:index]
resources :labels, except: [:show] do resources :labels, except: [:show] do
post :toggle_subscription, on: :member post :toggle_subscription, on: :member
end end
resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do
member do member do
get :merge_requests get :merge_requests
get :participants get :participants
get :labels get :labels
end
end end
end
resource :avatar, only: [:destroy] resource :avatar, only: [:destroy]
resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
post :resend_invite, on: :member post :resend_invite, on: :member
delete :leave, on: :collection delete :leave, on: :collection
end
end end
end end
...@@ -63,6 +64,8 @@ constraints(GroupUrlConstrainer.new) do ...@@ -63,6 +64,8 @@ constraints(GroupUrlConstrainer.new) do
# Legacy paths should be defined last, so they would be ignored if routes with # Legacy paths should be defined last, so they would be ignored if routes with
# one of the previously reserved words exist. # one of the previously reserved words exist.
scope(path: 'groups/*group_id') do scope(path: 'groups/*group_id') do
Gitlab::Routing.redirect_legacy_paths(self, :labels, :milestones, :group_members) Gitlab::Routing.redirect_legacy_paths(self, :labels, :milestones, :group_members,
:edit, :issues, :merge_requests, :projects,
:activity)
end end
end end
...@@ -112,18 +112,13 @@ module Gitlab ...@@ -112,18 +112,13 @@ module Gitlab
# this would map to the activity-page of its parent. # this would map to the activity-page of its parent.
GROUP_ROUTES = %w[ GROUP_ROUTES = %w[
- -
activity
analytics analytics
audit_events audit_events
edit
hooks hooks
issues
ldap ldap
ldap_group_links ldap_group_links
merge_requests
notification_setting notification_setting
pipeline_quota pipeline_quota
projects
].freeze ].freeze
ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES
......
...@@ -42,10 +42,18 @@ module Gitlab ...@@ -42,10 +42,18 @@ module Gitlab
end end
def self.redirect_legacy_paths(router, *paths) def self.redirect_legacy_paths(router, *paths)
build_redirect_path = lambda do |request, _params, path|
# Only replace the last occurence of `path`.
path = request.fullpath.sub(%r{/#{path}/*(?!.*#{path})}, "/-/#{path}/")
path << request.query_string if request.query_string.present?
path
end
paths.each do |path| paths.each do |path|
router.match "/#{path}(/*rest)", router.match "/#{path}(/*rest)",
via: [:get, :post, :patch, :delete], via: [:get, :post, :patch, :delete],
to: router.redirect { |_params, request| request.fullpath.gsub(%r{/#{path}/*}, "/-/#{path}/") }, to: router.redirect { |params, request| build_redirect_path.call(request, params, path) },
as: "legacy_#{path}_redirect" as: "legacy_#{path}_redirect"
end end
end end
......
...@@ -24,7 +24,7 @@ describe LabelsHelper do ...@@ -24,7 +24,7 @@ describe LabelsHelper do
let(:group) { build(:group, name: 'bar') } let(:group) { build(:group, name: 'bar') }
it 'links to group issues page' do it 'links to group issues page' do
expect(link_to_label(label, subject: group)).to match %r{<a href="/groups/bar/issues\?label_name%5B%5D=#{label.name}">.*</a>} expect(link_to_label(label, subject: group)).to match %r{<a href="/groups/bar/-/issues\?label_name%5B%5D=#{label.name}">.*</a>}
end end
end end
......
...@@ -297,7 +297,7 @@ describe Gitlab::PathRegex do ...@@ -297,7 +297,7 @@ describe Gitlab::PathRegex do
end end
it 'rejects group routes' do it 'rejects group routes' do
expect(subject).not_to match('root/activity/') expect(subject).not_to match('root/analytics/')
end end
end end
...@@ -317,7 +317,7 @@ describe Gitlab::PathRegex do ...@@ -317,7 +317,7 @@ describe Gitlab::PathRegex do
end end
it 'rejects group routes' do it 'rejects group routes' do
expect(subject).not_to match('root/activity/more/') expect(subject).not_to match('root/analytics/more/')
end end
end end
end end
...@@ -350,7 +350,7 @@ describe Gitlab::PathRegex do ...@@ -350,7 +350,7 @@ describe Gitlab::PathRegex do
end end
it 'accepts group routes' do it 'accepts group routes' do
expect(subject).to match('activity/') expect(subject).to match('analytics/')
end end
it 'is not case sensitive' do it 'is not case sensitive' do
...@@ -381,7 +381,7 @@ describe Gitlab::PathRegex do ...@@ -381,7 +381,7 @@ describe Gitlab::PathRegex do
end end
it 'accepts group routes' do it 'accepts group routes' do
expect(subject).to match('root/activity/') expect(subject).to match('root/analytics/')
end end
it 'is not case sensitive' do it 'is not case sensitive' do
......
...@@ -65,12 +65,6 @@ describe Group do ...@@ -65,12 +65,6 @@ describe Group do
expect(group).not_to be_valid expect(group).not_to be_valid
end end
it 'rejects reserved group paths' do
group = build(:group, path: 'activity', parent: create(:group))
expect(group).not_to be_valid
end
end end
describe '#visibility_level_allowed_by_parent' do describe '#visibility_level_allowed_by_parent' do
......
...@@ -18,37 +18,36 @@ describe "Groups", "routing" do ...@@ -18,37 +18,36 @@ describe "Groups", "routing" do
end end
it "to #activity" do it "to #activity" do
expect(get("/groups/#{group_path}/activity")).to route_to('groups#activity', id: group_path) expect(get("/groups/#{group_path}/-/activity")).to route_to('groups#activity', id: group_path)
end end
it "to #issues" do it "to #issues" do
expect(get("/groups/#{group_path}/issues")).to route_to('groups#issues', id: group_path) expect(get("/groups/#{group_path}/-/issues")).to route_to('groups#issues', id: group_path)
end end
it "to #members" do it "to #members" do
expect(get("/groups/#{group_path}/-/group_members")).to route_to('groups/group_members#index', group_id: group_path) expect(get("/groups/#{group_path}/-/group_members")).to route_to('groups/group_members#index', group_id: group_path)
end end
describe 'legacy redirection' do it "to #labels" do
shared_examples 'canonical groups route' do |path| expect(get("/groups/#{group_path}/-/labels")).to route_to('groups/labels#index', group_id: group_path)
it "#{path} routes to the correct controller" do end
expect(get("/groups/#{group_path}/-/#{path}"))
.to route_to(group_id: group_path,
controller: "groups/#{path}",
action: 'index')
end
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/#{path}", "/groups/complex.group-namegit/-/#{path}/" do it "to #milestones" do
let(:resource) { create(:group, parent: group, path: path) } expect(get("/groups/#{group_path}/-/milestones")).to route_to('groups/milestones#index', group_id: group_path)
end end
end
describe 'legacy redirection' do
describe 'labels' do describe 'labels' do
it_behaves_like 'canonical groups route', 'labels' it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/labels", "/groups/complex.group-namegit/-/labels/" do
let(:resource) { create(:group, parent: group, path: 'labels') }
end
end end
describe 'group_members' do describe 'group_members' do
it_behaves_like 'canonical groups route', 'group_members' it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/group_members", "/groups/complex.group-namegit/-/group_members/" do
let(:resource) { create(:group, parent: group, path: 'group_members') }
end
end end
describe 'avatar' do describe 'avatar' do
...@@ -61,7 +60,9 @@ describe "Groups", "routing" do ...@@ -61,7 +60,9 @@ describe "Groups", "routing" do
end end
describe 'milestones' do describe 'milestones' do
it_behaves_like 'canonical groups route', 'milestones' it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/milestones", "/groups/complex.group-namegit/-/milestones/" do
let(:resource) { create(:group, parent: group, path: 'milestones') }
end
context 'nested routes' do context 'nested routes' do
include RSpec::Rails::RequestExampleGroup include RSpec::Rails::RequestExampleGroup
...@@ -74,5 +75,43 @@ describe "Groups", "routing" do ...@@ -74,5 +75,43 @@ describe "Groups", "routing" do
end end
end end
end end
describe 'edit' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/edit", "/groups/complex.group-namegit/-/edit/" do
let(:resource) do
pending('still rejected because of the wildcard reserved word')
create(:group, parent: group, path: 'edit')
end
end
end
describe 'issues' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/issues", "/groups/complex.group-namegit/-/issues/" do
let(:resource) { create(:group, parent: group, path: 'issues') }
end
end
describe 'merge_requests' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/merge_requests", "/groups/complex.group-namegit/-/merge_requests/" do
let(:resource) { create(:group, parent: group, path: 'merge_requests') }
end
end
describe 'projects' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/projects", "/groups/complex.group-namegit/-/projects/" do
let(:resource) { create(:group, parent: group, path: 'projects') }
end
end
describe 'activity' do
it_behaves_like 'redirecting a legacy path', "/groups/complex.group-namegit/activity", "/groups/complex.group-namegit/-/activity/" do
let(:resource) { create(:group, parent: group, path: 'activity') }
end
it_behaves_like 'redirecting a legacy path', "/groups/activity/activity", "/groups/activity/-/activity/" do
let!(:parent) { create(:group, path: 'activity') }
let(:resource) { create(:group, parent: parent, path: 'activity') }
end
end
end end
end end
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