Nest the group_children_path inside the canonical group path

app/controllers/groups/children_controller.rb

+module Groups
+  class ChildrenController < Groups::ApplicationController
+    before_action :group
+
+    def index
+      parent = if params[:parent_id].present?
+                 GroupFinder.new(current_user).execute(id: params[:parent_id])
+               else
+                 @group
+               end
+
+      if parent.nil?
+        render_404
+        return
+      end
+
+      setup_children(parent)
+
+      respond_to do |format|
+        format.json do
+          serializer = GroupChildSerializer
+                         .new(current_user: current_user)
+                         .with_pagination(request, response)
+          serializer.expand_hierarchy(parent) if params[:filter].present?
+          render json: serializer.represent(@children)
+        end
+      end
+    end
+
+    protected
+
+    def setup_children(parent)
+      @children = GroupDescendantsFinder.new(current_user: current_user,
+                                             parent_group: parent,
+                                             params: params).execute
+      @children = @children.page(params[:page])
+    end
+  end
+end

app/controllers/groups_controller.rb

@@ -59,31 +59,6 @@ class GroupsController < Groups::ApplicationController
     end
   end
 
-  def children
-    parent = if params[:parent_id].present?
-               GroupFinder.new(current_user).execute(id: params[:parent_id])
-             else
-               @group
-             end
-
-    if parent.nil?
-      render_404
-      return
-    end
-
-    setup_children(parent)
-
-    respond_to do |format|
-      format.json do
-        serializer = GroupChildSerializer
-                       .new(current_user: current_user)
-                       .with_pagination(request, response)
-        serializer.expand_hierarchy(parent) if params[:filter].present?
-        render json: serializer.represent(@children)
-      end
-    end
-  end
-
   def activity
     respond_to do |format|
       format.html
@@ -120,13 +95,6 @@ class GroupsController < Groups::ApplicationController
 
   protected
 
-  def setup_children(parent)
-    @children = GroupDescendantsFinder.new(current_user: current_user,
-                                           parent_group: parent,
-                                           params: params).execute
-    @children = @children.page(params[:page])
-  end
-
   def authorize_create_group!
     allowed = if params[:parent_id].present?
                 parent = Group.find_by(id: params[:parent_id])

config/routes/group.rb

@@ -30,6 +30,8 @@ scope(path: 'groups/*group_id',
     end
 
     resources :variables, only: [:index, :show, :update, :create, :destroy]
+
+    resources :children, only: [:index]
   end
 end
 
@@ -53,9 +55,5 @@ constraints(GroupUrlConstrainer.new) do
     patch '/', action: :update
     put '/', action: :update
     delete '/', action: :destroy
-
-    scope(path: '-') do
-      get :children
-    end
   end
 end

spec/controllers/groups/children_controller_spec.rb

+require 'spec_helper'
+
+describe Groups::ChildrenController do
+  let(:group) { create(:group, :public) }
+  let(:user) { create(:user) }
+  let!(:group_member) { create(:group_member, group: group, user: user) }
+
+  describe 'GET #index' do
+    context 'for projects' do
+      let!(:public_project) { create(:project, :public, namespace: group) }
+      let!(:private_project) { create(:project, :private, namespace: group) }
+
+      context 'as a user' do
+        before do
+          sign_in(user)
+        end
+
+        it 'shows all children' do
+          get :index, group_id: group.to_param, format: :json
+
+          expect(assigns(:children)).to contain_exactly(public_project, private_project)
+        end
+
+        context 'being member of private subgroup' do
+          it 'shows public and private children the user is member of' do
+            group_member.destroy!
+            private_project.add_guest(user)
+
+            get :index, group_id: group.to_param, format: :json
+
+            expect(assigns(:children)).to contain_exactly(public_project, private_project)
+          end
+        end
+      end
+
+      context 'as a guest' do
+        it 'shows the public children' do
+          get :index, group_id: group.to_param, format: :json
+
+          expect(assigns(:children)).to contain_exactly(public_project)
+        end
+      end
+    end
+
+    context 'for subgroups', :nested_groups do
+      let!(:public_subgroup) { create(:group, :public, parent: group) }
+      let!(:private_subgroup) { create(:group, :private, parent: group) }
+      let!(:public_project) { create(:project, :public, namespace: group) }
+      let!(:private_project) { create(:project, :private, namespace: group) }
+
+      context 'as a user' do
+        before do
+          sign_in(user)
+        end
+
+        it 'shows all children' do
+          get :index, group_id: group.to_param, format: :json
+
+          expect(assigns(:children)).to contain_exactly(public_subgroup, private_subgroup, public_project, private_project)
+        end
+
+        context 'being member of private subgroup' do
+          it 'shows public and private children the user is member of' do
+            group_member.destroy!
+            private_subgroup.add_guest(user)
+            private_project.add_guest(user)
+
+            get :index, group_id: group.to_param, format: :json
+
+            expect(assigns(:children)).to contain_exactly(public_subgroup, private_subgroup, public_project, private_project)
+          end
+        end
+      end
+
+      context 'as a guest' do
+        it 'shows the public children' do
+          get :index, group_id: group.to_param, format: :json
+
+          expect(assigns(:children)).to contain_exactly(public_subgroup, public_project)
+        end
+      end
+
+      context 'filtering children' do
+        it 'expands the tree for matching projects' do
+          project = create(:project, :public, namespace: public_subgroup, name: 'filterme')
+
+          get :index, group_id: group.to_param, filter: 'filter', format: :json
+
+          group_json = json_response.first
+          project_json = group_json['children'].first
+
+          expect(group_json['id']).to eq(public_subgroup.id)
+          expect(project_json['id']).to eq(project.id)
+        end
+
+        it 'expands the tree for matching subgroups' do
+          matched_group = create(:group, :public, parent: public_subgroup, name: 'filterme')
+
+          get :index, group_id: group.to_param, filter: 'filter', format: :json
+
+          group_json = json_response.first
+          matched_group_json = group_json['children'].first
+
+          expect(group_json['id']).to eq(public_subgroup.id)
+          expect(matched_group_json['id']).to eq(matched_group.id)
+        end
+
+        it 'merges the trees correctly' do
+          shared_subgroup = create(:group, :public, parent: group, path: 'hardware')
+          matched_project_1 = create(:project, :public, namespace: shared_subgroup, name: 'mobile-soc')
+
+          l2_subgroup = create(:group, :public, parent: shared_subgroup, path: 'broadcom')
+          l3_subgroup = create(:group, :public,  parent: l2_subgroup, path: 'wifi-group')
+          matched_project_2 = create(:project, :public, namespace: l3_subgroup, name: 'mobile')
+
+          get :index, group_id: group.to_param, filter: 'mobile', format: :json
+
+          shared_group_json = json_response.first
+          expect(shared_group_json['id']).to eq(shared_subgroup.id)
+
+          matched_project_1_json = shared_group_json['children'].detect { |child| child['type'] == 'project' }
+          expect(matched_project_1_json['id']).to eq(matched_project_1.id)
+
+          l2_subgroup_json = shared_group_json['children'].detect { |child| child['type'] == 'group' }
+          expect(l2_subgroup_json['id']).to eq(l2_subgroup.id)
+
+          l3_subgroup_json = l2_subgroup_json['children'].first
+          expect(l3_subgroup_json['id']).to eq(l3_subgroup.id)
+
+          matched_project_2_json = l3_subgroup_json['children'].first
+          expect(matched_project_2_json['id']).to eq(matched_project_2.id)
+        end
+
+        it 'expands the tree upto a specified parent' do
+          subgroup = create(:group, :public, parent: group)
+          l2_subgroup = create(:group, :public, parent: subgroup)
+          create(:project, :public, namespace: l2_subgroup, name: 'test')
+
+          get :index, group_id: subgroup.to_param, filter: 'test', format: :json
+
+          expect(response).to have_http_status(200)
+        end
+
+        it 'returns an empty array when there are no search results' do
+          subgroup = create(:group, :public, parent: group)
+          l2_subgroup = create(:group, :public, parent: subgroup)
+          create(:project, :public, namespace: l2_subgroup, name: 'no-match')
+
+          get :index, group_id: subgroup.to_param, filter: 'test', format: :json
+
+          expect(json_response).to eq([])
+        end
+
+        it 'includes pagination headers' do
+          2.times { |i| create(:group, :public, parent: public_subgroup, name: "filterme#{i}") }
+
+          get :index, group_id: group.to_param, filter: 'filter', per_page: 1, format: :json
+
+          expect(response).to include_pagination_headers
+        end
+      end
+
+      context 'queries per rendered element', :request_store do
+        # We need to make sure the following counts are preloaded
+        # otherwise they will cause an extra query
+        # 1. Count of visible projects in the element
+        # 2. Count of visible subgroups in the element
+        # 3. Count of members of a group
+        let(:expected_queries_per_group) { 0 }
+        let(:expected_queries_per_project) { 0 }
+
+        def get_list
+          get :index, group_id: group.to_param, format: :json
+        end
+
+        it 'queries the expected amount for a group row' do
+          control = ActiveRecord::QueryRecorder.new { get_list }
+
+          _new_group = create(:group, :public, parent: group)
+
+          expect { get_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group)
+        end
+
+        it 'queries the expected amount for a project row' do
+          control = ActiveRecord::QueryRecorder.new { get_list }
+          _new_project = create(:project, :public, namespace: group)
+
+          expect { get_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_project)
+        end
+
+        context 'when rendering hierarchies' do
+          # When loading hierarchies we load the all the ancestors for matched projects
+          # in 1 separate query
+          let(:extra_queries_for_hierarchies) { 1 }
+
+          def get_filtered_list
+            get :index, group_id: group.to_param, filter: 'filter', format: :json
+          end
+
+          it 'queries the expected amount when nested rows are increased for a group' do
+            matched_group = create(:group, :public, parent: group, name: 'filterme')
+
+            control = ActiveRecord::QueryRecorder.new { get_filtered_list }
+
+            matched_group.update!(parent: public_subgroup)
+
+            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
+          end
+
+          it 'queries the expected amount when a new group match is added' do
+            create(:group, :public, parent: public_subgroup, name: 'filterme')
+
+            control = ActiveRecord::QueryRecorder.new { get_filtered_list }
+
+            create(:group, :public, parent: public_subgroup, name: 'filterme2')
+            create(:group, :public, parent: public_subgroup, name: 'filterme3')
+
+            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
+          end
+
+          it 'queries the expected amount when nested rows are increased for a project' do
+            matched_project = create(:project, :public, namespace: group, name: 'filterme')
+
+            control = ActiveRecord::QueryRecorder.new { get_filtered_list }
+
+            matched_project.update!(namespace: public_subgroup)
+
+            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
+          end
+        end
+      end
+    end
+
+    context 'pagination' do
+      let(:per_page) { 3 }
+
+      before do
+        allow(Kaminari.config).to receive(:default_per_page).and_return(per_page)
+      end
+
+      context 'with only projects' do
+        let!(:other_project) { create(:project, :public, namespace: group) }
+        let!(:first_page_projects) { create_list(:project, per_page, :public, namespace: group ) }
+
+        it 'has projects on the first page' do
+          get :index, group_id: group.to_param, sort: 'id_desc', format: :json
+
+          expect(assigns(:children)).to contain_exactly(*first_page_projects)
+        end
+
+        it 'has projects on the second page' do
+          get :index, group_id: group.to_param, sort: 'id_desc', page: 2, format: :json
+
+          expect(assigns(:children)).to contain_exactly(other_project)
+        end
+      end
+
+      context 'with subgroups and projects', :nested_groups do
+        let!(:first_page_subgroups) { create_list(:group,  per_page, :public,  parent: group) }
+        let!(:other_subgroup) { create(:group, :public, parent: group) }
+        let!(:next_page_projects) { create_list(:project, per_page, :public, namespace: group) }
+
+        it 'contains all subgroups' do
+          get :index, group_id: group.to_param, sort: 'id_asc', format: :json
+
+          expect(assigns(:children)).to contain_exactly(*first_page_subgroups)
+        end
+
+        it 'contains the project and group on the second page' do
+          get :index, group_id: group.to_param, sort: 'id_asc', page: 2, format: :json
+
+          expect(assigns(:children)).to contain_exactly(other_subgroup, *next_page_projects.take(per_page - 1))
+        end
+      end
+    end
+  end
+end

spec/controllers/groups_controller_spec.rb

@@ -150,276 +150,6 @@ describe GroupsController do
     end
   end
 
-  describe 'GET #children' do
-    context 'for projects' do
-      let!(:public_project) { create(:project, :public, namespace: group) }
-      let!(:private_project) { create(:project, :private, namespace: group) }
-
-      context 'as a user' do
-        before do
-          sign_in(user)
-        end
-
-        it 'shows all children' do
-          get :children, id: group.to_param, format: :json
-
-          expect(assigns(:children)).to contain_exactly(public_project, private_project)
-        end
-
-        context 'being member of private subgroup' do
-          it 'shows public and private children the user is member of' do
-            group_member.destroy!
-            private_project.add_guest(user)
-
-            get :children, id: group.to_param, format: :json
-
-            expect(assigns(:children)).to contain_exactly(public_project, private_project)
-          end
-        end
-      end
-
-      context 'as a guest' do
-        it 'shows the public children' do
-          get :children, id: group.to_param, format: :json
-
-          expect(assigns(:children)).to contain_exactly(public_project)
-        end
-      end
-    end
-
-    context 'for subgroups', :nested_groups do
-      let!(:public_subgroup) { create(:group, :public, parent: group) }
-      let!(:private_subgroup) { create(:group, :private, parent: group) }
-      let!(:public_project) { create(:project, :public, namespace: group) }
-      let!(:private_project) { create(:project, :private, namespace: group) }
-
-      context 'as a user' do
-        before do
-          sign_in(user)
-        end
-
-        it 'shows all children' do
-          get :children, id: group.to_param, format: :json
-
-          expect(assigns(:children)).to contain_exactly(public_subgroup, private_subgroup, public_project, private_project)
-        end
-
-        context 'being member of private subgroup' do
-          it 'shows public and private children the user is member of' do
-            group_member.destroy!
-            private_subgroup.add_guest(user)
-            private_project.add_guest(user)
-
-            get :children, id: group.to_param, format: :json
-
-            expect(assigns(:children)).to contain_exactly(public_subgroup, private_subgroup, public_project, private_project)
-          end
-        end
-      end
-
-      context 'as a guest' do
-        it 'shows the public children' do
-          get :children, id: group.to_param, format: :json
-
-          expect(assigns(:children)).to contain_exactly(public_subgroup, public_project)
-        end
-      end
-
-      context 'filtering children' do
-        it 'expands the tree for matching projects' do
-          project = create(:project, :public, namespace: public_subgroup, name: 'filterme')
-
-          get :children, id: group.to_param, filter: 'filter', format: :json
-
-          group_json = json_response.first
-          project_json = group_json['children'].first
-
-          expect(group_json['id']).to eq(public_subgroup.id)
-          expect(project_json['id']).to eq(project.id)
-        end
-
-        it 'expands the tree for matching subgroups' do
-          matched_group = create(:group, :public, parent: public_subgroup, name: 'filterme')
-
-          get :children, id: group.to_param, filter: 'filter', format: :json
-
-          group_json = json_response.first
-          matched_group_json = group_json['children'].first
-
-          expect(group_json['id']).to eq(public_subgroup.id)
-          expect(matched_group_json['id']).to eq(matched_group.id)
-        end
-
-        it 'merges the trees correctly' do
-          shared_subgroup = create(:group, :public, parent: group, path: 'hardware')
-          matched_project_1 = create(:project, :public, namespace: shared_subgroup, name: 'mobile-soc')
-
-          l2_subgroup = create(:group, :public, parent: shared_subgroup, path: 'broadcom')
-          l3_subgroup = create(:group, :public,  parent: l2_subgroup, path: 'wifi-group')
-          matched_project_2 = create(:project, :public, namespace: l3_subgroup, name: 'mobile')
-
-          get :children, id: group.to_param, filter: 'mobile', format: :json
-
-          shared_group_json = json_response.first
-          expect(shared_group_json['id']).to eq(shared_subgroup.id)
-
-          matched_project_1_json = shared_group_json['children'].detect { |child| child['type'] == 'project' }
-          expect(matched_project_1_json['id']).to eq(matched_project_1.id)
-
-          l2_subgroup_json = shared_group_json['children'].detect { |child| child['type'] == 'group' }
-          expect(l2_subgroup_json['id']).to eq(l2_subgroup.id)
-
-          l3_subgroup_json = l2_subgroup_json['children'].first
-          expect(l3_subgroup_json['id']).to eq(l3_subgroup.id)
-
-          matched_project_2_json = l3_subgroup_json['children'].first
-          expect(matched_project_2_json['id']).to eq(matched_project_2.id)
-        end
-
-        it 'expands the tree upto a specified parent' do
-          subgroup = create(:group, :public, parent: group)
-          l2_subgroup = create(:group, :public, parent: subgroup)
-          create(:project, :public, namespace: l2_subgroup, name: 'test')
-
-          get :children, id: subgroup.to_param, filter: 'test', format: :json
-
-          expect(response).to have_http_status(200)
-        end
-
-        it 'returns an empty array when there are no search results' do
-          subgroup = create(:group, :public, parent: group)
-          l2_subgroup = create(:group, :public, parent: subgroup)
-          create(:project, :public, namespace: l2_subgroup, name: 'no-match')
-
-          get :children, id: subgroup.to_param, filter: 'test', format: :json
-
-          expect(json_response).to eq([])
-        end
-
-        it 'includes pagination headers' do
-          2.times { |i| create(:group, :public, parent: public_subgroup, name: "filterme#{i}") }
-
-          get :children, id: group.to_param, filter: 'filter', per_page: 1, format: :json
-
-          expect(response).to include_pagination_headers
-        end
-      end
-
-      context 'queries per rendered element', :request_store do
-        # We need to make sure the following counts are preloaded
-        # otherwise they will cause an extra query
-        # 1. Count of visible projects in the element
-        # 2. Count of visible subgroups in the element
-        # 3. Count of members of a group
-        let(:expected_queries_per_group) { 0 }
-        let(:expected_queries_per_project) { 0 }
-
-        def get_list
-          get :children, id: group.to_param, format: :json
-        end
-
-        it 'queries the expected amount for a group row' do
-          control = ActiveRecord::QueryRecorder.new { get_list }
-
-          _new_group = create(:group, :public, parent: group)
-
-          expect { get_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group)
-        end
-
-        it 'queries the expected amount for a project row' do
-          control = ActiveRecord::QueryRecorder.new { get_list }
-          _new_project = create(:project, :public, namespace: group)
-
-          expect { get_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_project)
-        end
-
-        context 'when rendering hierarchies' do
-          # When loading hierarchies we load the all the ancestors for matched projects
-          # in 1 separate query
-          let(:extra_queries_for_hierarchies) { 1 }
-
-          def get_filtered_list
-            get :children, id: group.to_param, filter: 'filter', format: :json
-          end
-
-          it 'queries the expected amount when nested rows are increased for a group' do
-            matched_group = create(:group, :public, parent: group, name: 'filterme')
-
-            control = ActiveRecord::QueryRecorder.new { get_filtered_list }
-
-            matched_group.update!(parent: public_subgroup)
-
-            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
-          end
-
-          it 'queries the expected amount when a new group match is added' do
-            create(:group, :public, parent: public_subgroup, name: 'filterme')
-
-            control = ActiveRecord::QueryRecorder.new { get_filtered_list }
-
-            create(:group, :public, parent: public_subgroup, name: 'filterme2')
-            create(:group, :public, parent: public_subgroup, name: 'filterme3')
-
-            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
-          end
-
-          it 'queries the expected amount when nested rows are increased for a project' do
-            matched_project = create(:project, :public, namespace: group, name: 'filterme')
-
-            control = ActiveRecord::QueryRecorder.new { get_filtered_list }
-
-            matched_project.update!(namespace: public_subgroup)
-
-            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
-          end
-        end
-      end
-    end
-
-    context 'pagination' do
-      let(:per_page) { 3 }
-
-      before do
-        allow(Kaminari.config).to receive(:default_per_page).and_return(per_page)
-      end
-
-      context 'with only projects' do
-        let!(:other_project) { create(:project, :public, namespace: group) }
-        let!(:first_page_projects) { create_list(:project, per_page, :public, namespace: group ) }
-
-        it 'has projects on the first page' do
-          get :children, id: group.to_param, sort: 'id_desc', format: :json
-
-          expect(assigns(:children)).to contain_exactly(*first_page_projects)
-        end
-
-        it 'has projects on the second page' do
-          get :children, id: group.to_param, sort: 'id_desc', page: 2, format: :json
-
-          expect(assigns(:children)).to contain_exactly(other_project)
-        end
-      end
-
-      context 'with subgroups and projects', :nested_groups do
-        let!(:first_page_subgroups) { create_list(:group,  per_page, :public,  parent: group) }
-        let!(:other_subgroup) { create(:group, :public, parent: group) }
-        let!(:next_page_projects) { create_list(:project, per_page, :public, namespace: group) }
-
-        it 'contains all subgroups' do
-          get :children, id: group.to_param, sort: 'id_asc', format: :json
-
-          expect(assigns(:children)).to contain_exactly(*first_page_subgroups)
-        end
-
-        it 'contains the project and group on the second page' do
-          get :children, id: group.to_param, sort: 'id_asc', page: 2, format: :json
-
-          expect(assigns(:children)).to contain_exactly(other_subgroup, *next_page_projects.take(per_page - 1))
-        end
-      end
-    end
-  end
-
   describe 'GET #issues' do
     let(:issue_1) { create(:issue, project: project) }
     let(:issue_2) { create(:issue, project: project) }