Merge branch 'ee-3662-board_routes_prefix_with_dash' into 'master'

Move group boards routes under '-' and remove "boards" from reserved paths

Closes #3662

See merge request gitlab-org/gitlab-ee!3078

changelogs/unreleased-ee/ee-3662-board_routes_prefix_with_dash.yml

+---
+title: Move group boards routes under - and remove "boards" from reserved paths
+merge_request:
+author:
+type: fixed

config/routes/group.rb

@@ -2,84 +2,87 @@ require 'constraints/group_url_constrainer'
 
 resources :groups, only: [:index, :new, :create]
 
-scope(path: 'groups/*group_id',
-      module: :groups,
-      as: :group,
-      constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do
-
-  ## EE-specific
-  resource :analytics, only: [:show]
-  resource :ldap, only: [] do
-    member do
-      put :sync
-    end
+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
+    get :subgroups, as: :subgroups_group
+    get '/', action: :show, as: :group_canonical
   end
 
-  resources :ldap_group_links, only: [:index, :create, :destroy]
-  ## EE-specific
-
-  resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
-    post :resend_invite, on: :member
-    delete :leave, on: :collection
+  scope(path: 'groups/*group_id',
+        module: :groups,
+        as: :group,
+        constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do
 
     ## EE-specific
-    patch :override, on: :member
+    resource :analytics, only: [:show]
+    resource :ldap, only: [] do
+      member do
+        put :sync
+      end
+    end
+
+    resources :ldap_group_links, only: [:index, :create, :destroy]
     ## EE-specific
-  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
+    resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
+      post :resend_invite, on: :member
+      delete :leave, on: :collection
 
-  ## EE-specific
-  resource :notification_setting, only: [:update]
-  resources :audit_events, only: [:index]
-  resources :pipeline_quota, only: [:index]
-  ## EE-specific
+      ## EE-specific
+      patch :override, on: :member
+      ## EE-specific
+    end
 
-  ## EE-specific
-  resources :hooks, only: [:index, :create, :destroy], constraints: { id: /\d+/ } do
-    member do
-      get :test
+    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
-  end
-  ## EE-specific
 
-  resources :labels, except: [:show] do
-    post :toggle_subscription, on: :member
-  end
+    ## EE-specific
+    resource :notification_setting, only: [:update]
+    resources :audit_events, only: [:index]
+    resources :pipeline_quota, only: [:index]
+    ## EE-specific
 
-  scope path: '-' do
-    namespace :settings do
-      resource :ci_cd, only: [:show], controller: 'ci_cd'
+    ## EE-specific
+    resources :hooks, only: [:index, :create, :destroy], constraints: { id: /\d+/ } do
+      member do
+        get :test
+      end
     end
+    ## EE-specific
 
-    resources :variables, only: [:index, :show, :update, :create, :destroy]
-    resources :billings, only: [:index]
-  end
+    resources :labels, except: [:show] do
+      post :toggle_subscription, on: :member
+    end
 
-  ## EE-specific
-  resources :boards, only: [:index, :show, :create, :update, :destroy]
-end
+    scope path: '-' do
+      namespace :settings do
+        resource :ci_cd, only: [:show], controller: 'ci_cd'
+      end
 
-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
-  get :subgroups, as: :subgroups_group
-  get '/', action: :show, as: :group_canonical
-end
+      resources :variables, only: [:index, :show, :update, :create, :destroy]
+      resources :billings, only: [:index]
+
+      ## EE-specific
+      resources :boards, only: [:index, :show, :create, :update, :destroy]
+    end
+
+    ## EE-specific
+    get :boards, to: redirect('/groups/%{group_id}/-/boards')
+  end
 
-constraints(GroupUrlConstrainer.new) do
   scope(path: '*id',
         as: :group,
         constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ },

lib/gitlab/path_regex.rb

@@ -120,7 +120,6 @@ module Gitlab
       analytics
       audit_events
       avatar
-      boards
       edit
       group_members
       hooks

spec/lib/gitlab/path_regex_spec.rb

@@ -72,7 +72,14 @@ describe Gitlab::PathRegex do
     route_set = Rails.application.routes
     routes_collection = route_set.routes
     routes_array = routes_collection.routes
-    routes_array.map { |route| route.path.spec.to_s }
+    non_deprecated_redirect_routes = routes_array.reject do |route|
+      app = route.app
+      # `app.app` is either another app, or `self`. We want to find the final app.
+      app = app.app while app.try(:app) && app.app != app
+
+      app.is_a?(ActionDispatch::Routing::PathRedirect) && app.block.include?('/-/')
+    end
+    non_deprecated_redirect_routes.map { |route| route.path.spec.to_s }
   end
 
   let(:routes_without_format) { all_routes.map { |path| without_format(path) } }

spec/requests/group_routing_spec.rb

+require 'spec_helper'
+
+describe 'Deprecated boards paths' do
+  it 'redirects to boards page' do
+    group = create :group, name: 'gitlabhq'
+
+    get('/groups/gitlabhq/boards')
+
+    expect(response).to redirect_to(group_boards_path(group))
+  end
+end

spec/routing/group_routing_spec.rb

+require 'spec_helper'
+
+describe 'Group routing' do
+  describe 'subgroup "boards"' do
+    it 'shows group show page' do
+      allow(Group).to receive(:find_by_full_path).with('gitlabhq/boards', any_args).and_return(true)
+
+      expect(get('/groups/gitlabhq/boards')).to route_to('groups#show', id: 'gitlabhq/boards')
+    end
+
+    it 'shows boards index page' do
+      allow(Group).to receive(:find_by_full_path).with('gitlabhq', any_args).and_return(true)
+
+      expect(get('/groups/gitlabhq/-/boards')).to route_to('groups/boards#index', group_id: 'gitlabhq')
+    end
+  end
+end

spec/routing/routing_spec.rb

@@ -285,17 +285,15 @@ end
 
 describe "Groups", "routing" do
   let(:name) { 'complex.group-namegit' }
-
-  before do
-    allow_any_instance_of(GroupUrlConstrainer).to receive(:matches?).and_return(true)
-  end
+  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
-    expect(get("/#{name}/#{name}")).to route_to('groups#show', id: "#{name}/#{name}")
+    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
@@ -313,10 +311,6 @@ describe "Groups", "routing" do
   it "to #members" do
     expect(get("/groups/#{name}/group_members")).to route_to('groups/group_members#index', group_id: name)
   end
-
-  it "also display group#show with slash in the path" do
-    expect(get('/group/subgroup')).to route_to('groups#show', id: 'group/subgroup')
-  end
 end
 
 describe HealthCheckController, 'routing' do