From 2c25a7ae3453e72ad6cab504255e327c17df0a95 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 11 Oct 2017 18:27:53 +0200 Subject: [PATCH] Nest the group_children_path inside the canonical group path --- app/controllers/groups/children_controller.rb | 39 +++ app/controllers/groups_controller.rb | 32 -- config/routes/group.rb | 6 +- .../groups/children_controller_spec.rb | 277 ++++++++++++++++++ spec/controllers/groups_controller_spec.rb | 270 ----------------- 5 files changed, 318 insertions(+), 306 deletions(-) create mode 100644 app/controllers/groups/children_controller.rb create mode 100644 spec/controllers/groups/children_controller_spec.rb diff --git a/app/controllers/groups/children_controller.rb b/app/controllers/groups/children_controller.rb new file mode 100644 index 00000000000..b474f5d15ee --- /dev/null +++ b/app/controllers/groups/children_controller.rb @@ -0,0 +1,39 @@ +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 diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index a24a0056a30..62987b404a7 100644 --- a/app/controllers/groups_controller.rb +++ b/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]) diff --git a/config/routes/group.rb b/config/routes/group.rb index 514f756a45f..2f5de5bed5a 100644 --- a/config/routes/group.rb +++ b/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 diff --git a/spec/controllers/groups/children_controller_spec.rb b/spec/controllers/groups/children_controller_spec.rb new file mode 100644 index 00000000000..f15a12ef7fd --- /dev/null +++ b/spec/controllers/groups/children_controller_spec.rb @@ -0,0 +1,277 @@ +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 diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 827c4cd3d19..e7631d4d709 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/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) }