Free up `avatar`, `group_members` and `milestones` as paths

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')