From 5dde0536c323d14fef2327a8d553b5f8a8a7b2d0 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 26 Oct 2017 17:49:07 +0200 Subject: Free up `avatar`, `group_members` and `milestones` as paths --- config/routes/group.rb | 31 +++++++------- lib/gitlab/path_regex.rb | 3 -- spec/lib/gitlab/path_regex_spec.rb | 14 ------- spec/routing/group_routing_spec.rb | 82 +++++++++++++++++++++++++++++++++----- spec/routing/routing_spec.rb | 30 -------------- 5 files changed, 87 insertions(+), 73 deletions(-) diff --git a/config/routes/group.rb b/config/routes/group.rb index ccd4ea7a8dd..11124eaec51 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -20,20 +20,6 @@ constraints(GroupUrlConstrainer.new) do module: :groups, as: :group, constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do - resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do - post :resend_invite, on: :member - delete :leave, on: :collection - end - - resource :avatar, only: [:destroy] - resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do - member do - get :merge_requests - get :participants - get :labels - end - end - scope path: '-' do namespace :settings do resource :ci_cd, only: [:show], controller: 'ci_cd' @@ -46,6 +32,21 @@ constraints(GroupUrlConstrainer.new) do resources :labels, except: [:show] do post :toggle_subscription, on: :member end + + resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :edit, :update, :new, :create] do + member do + get :merge_requests + get :participants + get :labels + end + end + + resource :avatar, only: [:destroy] + + resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do + post :resend_invite, on: :member + delete :leave, on: :collection + end end end @@ -62,6 +63,6 @@ constraints(GroupUrlConstrainer.new) do # Legacy paths should be defined last, so they would be ignored if routes with # one of the previously reserved words exist. scope(path: 'groups/*group_id') do - Gitlab::Routing.redirect_legacy_paths(self, :labels) + Gitlab::Routing.redirect_legacy_paths(self, :labels, :milestones, :group_members) end end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 5b9c60f945c..12e54013be1 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -115,15 +115,12 @@ module Gitlab activity analytics audit_events - avatar edit - group_members hooks issues ldap ldap_group_links merge_requests - milestones notification_setting pipeline_quota projects diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb index ee63c9338c5..c76084114cb 100644 --- a/spec/lib/gitlab/path_regex_spec.rb +++ b/spec/lib/gitlab/path_regex_spec.rb @@ -225,8 +225,6 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/') - expect(subject).to match('group_members/') - expect(subject).to match('labels/') end it 'is not case sensitive' do @@ -258,8 +256,6 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/') - expect(subject).to match('group_members/') - expect(subject).to match('labels/') end end @@ -280,8 +276,6 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/more/') - expect(subject).to match('group_members/more/') - expect(subject).to match('labels/more/') end end end @@ -304,8 +298,6 @@ describe Gitlab::PathRegex do it 'rejects group routes' do expect(subject).not_to match('root/activity/') - expect(subject).not_to match('root/group_members/') - expect(subject).not_to match('root/labels/') end end @@ -326,8 +318,6 @@ describe Gitlab::PathRegex do it 'rejects group routes' do expect(subject).not_to match('root/activity/more/') - expect(subject).not_to match('root/group_members/more/') - expect(subject).not_to match('root/labels/more/') end end end @@ -361,8 +351,6 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/') - expect(subject).to match('group_members/') - expect(subject).to match('labels/') end it 'is not case sensitive' do @@ -394,8 +382,6 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('root/activity/') - expect(subject).to match('root/group_members/') - expect(subject).to match('root/labels/') end it 'is not case sensitive' do diff --git a/spec/routing/group_routing_spec.rb b/spec/routing/group_routing_spec.rb index 39fbf082b4d..b3e55f5e3c9 100644 --- a/spec/routing/group_routing_spec.rb +++ b/spec/routing/group_routing_spec.rb @@ -1,18 +1,78 @@ require 'spec_helper' -describe 'group routing' do - let!(:existing_group) { create(:group, parent: create(:group, path: 'gitlab-org'), path: 'infra') } - - describe 'GET #labels' do - it 'routes to the correct controller' do - expect(get('/groups/gitlab-org/infra/-/labels')) - .to route_to(group_id: 'gitlab-org/infra', - controller: 'groups/labels', - action: 'index') +describe "Groups", "routing" do + let(:group_path) { 'complex.group-namegit' } + let!(:group) { create(:group, path: group_path) } + + it "to #show" do + expect(get("/groups/#{group_path}")).to route_to('groups#show', id: group_path) + end + + it "also supports nested groups" do + nested_group = create(:group, parent: group) + expect(get("/#{group_path}/#{nested_group.path}")).to route_to('groups#show', id: "#{group_path}/#{nested_group.path}") + end + + it "also display group#show on the short path" do + expect(get("/#{group_path}")).to route_to('groups#show', id: group_path) + end + + it "to #activity" do + expect(get("/groups/#{group_path}/activity")).to route_to('groups#activity', id: group_path) + end + + it "to #issues" do + expect(get("/groups/#{group_path}/issues")).to route_to('groups#issues', id: group_path) + end + + it "to #members" do + expect(get("/groups/#{group_path}/-/group_members")).to route_to('groups/group_members#index', group_id: group_path) + end + + describe 'legacy redirection' do + shared_examples 'canonical groups route' do |path| + it "#{path} routes to the correct controller" do + 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 + let(:resource) { create(:group, parent: group, path: path) } + end + end + + describe 'labels' do + it_behaves_like 'canonical groups route', 'labels' + end + + describe 'group_members' do + it_behaves_like 'canonical groups route', 'group_members' end - it_behaves_like 'redirecting a legacy path', '/groups/gitlab-org/infra/labels', '/groups/gitlab-org/infra/-/labels' do - let(:resource) { create(:group, parent: existing_group, path: 'labels') } + describe 'avatar' do + it 'routes to the avatars controller' do + expect(delete("/groups/#{group_path}/-/avatar")) + .to route_to(group_id: group_path, + controller: 'groups/avatars', + action: 'destroy') + end + end + + describe 'milestones' do + it_behaves_like 'canonical groups route', 'milestones' + + context 'nested routes' do + include RSpec::Rails::RequestExampleGroup + + let(:milestone) { create(:milestone, group: group) } + + it 'redirects the nested routes' do + request = get("/groups/#{group_path}/milestones/#{milestone.id}/merge_requests") + expect(request).to redirect_to("/groups/#{group_path}/-/milestones/#{milestone.id}/merge_requests") + end + end end end end diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index 609481603af..32aa6e5ad52 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -278,36 +278,6 @@ describe "Authentication", "routing" do end end -describe "Groups", "routing" do - let(:name) { 'complex.group-namegit' } - let!(:group) { create(:group, name: name) } - - it "to #show" do - expect(get("/groups/#{name}")).to route_to('groups#show', id: name) - end - - it "also supports nested groups" do - nested_group = create(:group, parent: group) - expect(get("/#{name}/#{nested_group.name}")).to route_to('groups#show', id: "#{name}/#{nested_group.name}") - end - - it "also display group#show on the short path" do - expect(get("/#{name}")).to route_to('groups#show', id: name) - end - - it "to #activity" do - expect(get("/groups/#{name}/activity")).to route_to('groups#activity', id: name) - end - - it "to #issues" do - expect(get("/groups/#{name}/issues")).to route_to('groups#issues', id: name) - end - - it "to #members" do - expect(get("/groups/#{name}/group_members")).to route_to('groups/group_members#index', group_id: name) - end -end - describe HealthCheckController, 'routing' do it 'to #index' do expect(get('/health_check')).to route_to('health_check#index') -- cgit v1.2.1