Free up some group reserved words

7 files changed, 100 insertions, 61 deletions

diff --git a/config/routes/group.rb b/config/routes/group.rb
index 11124eaec51..db99e10bb9a 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -8,45 +8,46 @@ constraints(GroupUrlConstrainer.new) do
   scope(path: 'groups/*id',
         controller: :groups,
         constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) do
-    get :edit, as: :edit_group
-    get :issues, as: :issues_group
-    get :merge_requests, as: :merge_requests_group
-    get :projects, as: :projects_group
-    get :activity, as: :activity_group
+    scope(path: '-') do
+      get :edit, as: :edit_group
+      get :issues, as: :issues_group
+      get :merge_requests, as: :merge_requests_group
+      get :projects, as: :projects_group
+      get :activity, as: :activity_group
+    end
+
     get '/', action: :show, as: :group_canonical
   end
 
-  scope(path: 'groups/*group_id',
+  scope(path: 'groups/*group_id/-',
         module: :groups,
         as: :group,
         constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do
-    scope path: '-' do
-      namespace :settings do
-        resource :ci_cd, only: [:show], controller: 'ci_cd'
-      end
+    namespace :settings do
+      resource :ci_cd, only: [:show], controller: 'ci_cd'
+    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
-        post :toggle_subscription, on: :member
-      end
+    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
+    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]
+    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
+    resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
+      post :resend_invite, on: :member
+      delete :leave, on: :collection
     end
   end
 
@@ -63,6 +64,8 @@ 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, :milestones, :group_members)
+    Gitlab::Routing.redirect_legacy_paths(self, :labels, :milestones, :group_members,
+                                          :edit, :issues, :merge_requests, :projects,
+                                          :activity)
   end
 end
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index 12e54013be1..6b8a4442ebd 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -112,18 +112,13 @@ module Gitlab
     # this would map to the activity-page of its parent.
     GROUP_ROUTES = %w[
       -
-      activity
       analytics
       audit_events
-      edit
       hooks
-      issues
       ldap
       ldap_group_links
-      merge_requests
       notification_setting
       pipeline_quota
-      projects
     ].freeze
 
     ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES
diff --git a/lib/gitlab/routing.rb b/lib/gitlab/routing.rb
index abfd413b7ea..defb47663cb 100644
--- a/lib/gitlab/routing.rb
+++ b/lib/gitlab/routing.rb
@@ -42,10 +42,18 @@ module Gitlab
     end
 
     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|
         router.match "/#{path}(/*rest)",
                      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"
       end
     end
diff --git a/spec/helpers/labels_helper_spec.rb b/spec/helpers/labels_helper_spec.rb
index 36d6e495ed0..4ac4302adfd 100644
--- a/spec/helpers/labels_helper_spec.rb
+++ b/spec/helpers/labels_helper_spec.rb
@@ -24,7 +24,7 @@ describe LabelsHelper do
       let(:group) { build(:group, name: 'bar') }
 
       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
 
diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb
index c76084114cb..98fcc84c416 100644
--- a/spec/lib/gitlab/path_regex_spec.rb
+++ b/spec/lib/gitlab/path_regex_spec.rb
@@ -297,7 +297,7 @@ describe Gitlab::PathRegex do
         end
 
         it 'rejects group routes' do
-          expect(subject).not_to match('root/activity/')
+          expect(subject).not_to match('root/analytics/')
         end
       end
 
@@ -317,7 +317,7 @@ describe Gitlab::PathRegex do
         end
 
         it 'rejects group routes' do
-          expect(subject).not_to match('root/activity/more/')
+          expect(subject).not_to match('root/analytics/more/')
         end
       end
     end
@@ -350,7 +350,7 @@ describe Gitlab::PathRegex do
     end
 
     it 'accepts group routes' do
-      expect(subject).to match('activity/')
+      expect(subject).to match('analytics/')
     end
 
     it 'is not case sensitive' do
@@ -381,7 +381,7 @@ describe Gitlab::PathRegex do
     end
 
     it 'accepts group routes' do
-      expect(subject).to match('root/activity/')
+      expect(subject).to match('root/analytics/')
     end
 
     it 'is not case sensitive' do
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index c8caa11b8b0..d4052a64570 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -65,12 +65,6 @@ describe Group do
 
         expect(group).not_to be_valid
       end
-
-      it 'rejects reserved group paths' do
-        group = build(:group, path: 'activity', parent: create(:group))
-
-        expect(group).not_to be_valid
-      end
     end
 
     describe '#visibility_level_allowed_by_parent' do
diff --git a/spec/routing/group_routing_spec.rb b/spec/routing/group_routing_spec.rb
index b3e55f5e3c9..3e56d34bc9d 100644
--- a/spec/routing/group_routing_spec.rb
+++ b/spec/routing/group_routing_spec.rb
@@ -18,37 +18,36 @@ describe "Groups", "routing" do
   end
 
   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
 
   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
 
   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 "to #labels" do
+    expect(get("/groups/#{group_path}/-/labels")).to route_to('groups/labels#index', group_id: group_path)
+  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
+  it "to #milestones" do
+    expect(get("/groups/#{group_path}/-/milestones")).to route_to('groups/milestones#index', group_id: group_path)
+  end
 
+  describe 'legacy redirection' 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
 
     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
 
     describe 'avatar' do
@@ -61,7 +60,9 @@ describe "Groups", "routing" do
     end
 
     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
         include RSpec::Rails::RequestExampleGroup
@@ -74,5 +75,43 @@ describe "Groups", "routing" do
         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