diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb new file mode 100644 index 00000000000..6d5682ff769 --- /dev/null +++ b/app/controllers/concerns/group_tree.rb @@ -0,0 +1,19 @@ +module GroupTree + def render_group_tree(groups) + # Only show root groups if no parent-id is given + @groups = groups.where(parent_id: params[:parent_id]) + @groups = @groups.search(params[:filter]) if params[:filter].present? + @groups = @groups.includes(:route) + @groups = @groups.sort(@sort = params[:sort]) + @groups = @groups.page(params[:page]) + + respond_to do |format| + format.html + format.json do + serializer = GroupChildSerializer.new(current_user: current_user) + .with_pagination(request, response) + render json: serializer.represent(@groups) + end + end + end +end diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb index 6488e0b6c09..025769f512a 100644 --- a/app/controllers/dashboard/groups_controller.rb +++ b/app/controllers/dashboard/groups_controller.rb @@ -1,20 +1,8 @@ class Dashboard::GroupsController < Dashboard::ApplicationController - def index - @groups = GroupsFinder.new(current_user, all_available: false).execute - # Only show root groups if no parent-id is given - @groups = @groups.where(parent_id: params[:parent_id]) - @groups = @groups.search(params[:filter]) if params[:filter].present? - @groups = @groups.includes(:route) - @groups = @groups.sort(@sort = params[:sort]) - @groups = @groups.page(params[:page]) + include GroupTree - respond_to do |format| - format.html - format.json do - serializer = GroupChildSerializer.new(current_user: current_user) - .with_pagination(request, response) - render json: serializer.represent(@groups) - end - end + def index + groups = GroupsFinder.new(current_user, all_available: false).execute + render_group_tree(groups) end end diff --git a/app/controllers/explore/groups_controller.rb b/app/controllers/explore/groups_controller.rb index 81883c543ba..fa0a0f68fbc 100644 --- a/app/controllers/explore/groups_controller.rb +++ b/app/controllers/explore/groups_controller.rb @@ -1,17 +1,7 @@ class Explore::GroupsController < Explore::ApplicationController - def index - @groups = GroupsFinder.new(current_user).execute - @groups = @groups.search(params[:filter_groups]) if params[:filter_groups].present? - @groups = @groups.sort(@sort = params[:sort]) - @groups = @groups.page(params[:page]) + include GroupTree - respond_to do |format| - format.html - format.json do - render json: { - html: view_to_html_string("explore/groups/_groups", locals: { groups: @groups }) - } - end - end + def index + render_group_tree GroupsFinder.new(current_user).execute end end diff --git a/spec/controllers/concerns/group_tree_spec.rb b/spec/controllers/concerns/group_tree_spec.rb new file mode 100644 index 00000000000..19387f2d271 --- /dev/null +++ b/spec/controllers/concerns/group_tree_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe GroupTree do + let(:group) { create(:group, :public) } + let(:user) { create(:user) } + + controller(ApplicationController) do + include GroupTree # rubocop:disable Rspec/DescribedClass + + def index + render_group_tree Group.all + end + end + + before do + group.add_owner(user) + sign_in(user) + end + + describe 'GET #index' do + it 'filters groups' do + other_group = create(:group, name: 'filter') + other_group.add_owner(user) + + get :index, filter: 'filt', format: :json + + expect(assigns(:groups)).to contain_exactly(other_group) + end + + context 'for subgroups', :nested_groups do + it 'only renders root groups when no parent was given' do + create(:group, :public, parent: group) + + get :index, format: :json + + expect(assigns(:groups)).to contain_exactly(group) + end + + it 'contains only the subgroup when a parent was given' do + subgroup = create(:group, :public, parent: group) + + get :index, parent_id: group.id, format: :json + + expect(assigns(:groups)).to contain_exactly(subgroup) + end + end + + context 'json content' do + it 'shows groups as json' do + get :index, format: :json + + expect(json_response.first['id']).to eq(group.id) + end + end + end +end diff --git a/spec/controllers/dashboard/groups_controller_spec.rb b/spec/controllers/dashboard/groups_controller_spec.rb index 89a16c233d8..fb9d3efbac0 100644 --- a/spec/controllers/dashboard/groups_controller_spec.rb +++ b/spec/controllers/dashboard/groups_controller_spec.rb @@ -1,29 +1,23 @@ require 'spec_helper' describe Dashboard::GroupsController do - let(:group) { create(:group, :public) } let(:user) { create(:user) } before do - group.add_owner(user) sign_in(user) end - describe 'GET #index' do - it 'shows child groups as json' do - get :index, format: :json + it 'renders group trees' do + expect(described_class).to include(GroupTree) + end - expect(json_response.first['id']).to eq(group.id) - end + it 'only includes projects the user is a member of' do + member_of_group = create(:group) + member_of_group.add_developer(user) + create(:group, :public) - it 'filters groups' do - other_group = create(:group, name: 'filter') - other_group.add_owner(user) + get :index - get :index, filter: 'filt', format: :json - all_ids = json_response.map { |group_json| group_json['id'] } - - expect(all_ids).to contain_exactly(other_group.id) - end + expect(assigns(:groups)).to contain_exactly(member_of_group) end end diff --git a/spec/controllers/explore/groups_controller_spec.rb b/spec/controllers/explore/groups_controller_spec.rb new file mode 100644 index 00000000000..1923d054e95 --- /dev/null +++ b/spec/controllers/explore/groups_controller_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe Explore::GroupsController do + let(:user) { create(:user) } + + before do + sign_in(user) + end + + it 'renders group trees' do + expect(described_class).to include(GroupTree) + end + + it 'includes public projects' do + member_of_group = create(:group) + member_of_group.add_developer(user) + public_group = create(:group, :public) + + get :index + + expect(assigns(:groups)).to contain_exactly(member_of_group, public_group) + end + +end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 25778cc2b59..18f9d707e18 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -1,4 +1,4 @@ -require 'rails_helper' +require 'spec_helper' describe GroupsController do let(:user) { create(:user) } @@ -150,6 +150,45 @@ describe GroupsController do end end + describe 'GET #show' do + context 'pagination' do + context 'with only projects' do + let!(:other_project) { create(:project, :public, namespace: group) } + let!(:first_page_projects) { create_list(:project, Kaminari.config.default_per_page, :public, namespace: group ) } + + it 'has projects on the first page' do + get :show, id: group.to_param, sort: 'id_desc' + + expect(assigns(:children)).to contain_exactly(*first_page_projects) + end + + it 'has projects on the second page' do + get :show, id: group.to_param, sort: 'id_desc', page: 2 + + expect(assigns(:children)).to contain_exactly(other_project) + end + end + + context 'with subgroups and projects', :nested_groups do + let!(:other_subgroup) { create(:group, :public, parent: group) } + let!(:project) { create(:project, :public, namespace: group) } + let!(:first_page_subgroups) { create_list(:group, Kaminari.config.default_per_page, parent: group) } + + it 'contains all subgroups' do + get :children, id: group.to_param, sort: 'id_desc', 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_desc', page: 2, format: :json + + expect(assigns(:children)).to contain_exactly(other_subgroup, project) + end + end + end + end + describe 'GET #children' do context 'for projects' do let!(:public_project) { create(:project, :public, namespace: group) } @@ -251,12 +290,14 @@ describe GroupsController do end end - context 'queries per rendered element' do + context 'queries per rendered element', :request_store do # The expected extra queries for the rendered group are: # 1. Count of memberships of the group # 2. Count of visible projects in the element # 3. Count of visible subgroups in the element - let(:expected_queries_per_group) { 3 } + # 4. Every parent + # 5. The route for a parent + let(:expected_queries_per_group) { 5 } let(:expected_queries_per_project) { 0 } def get_list @@ -265,7 +306,6 @@ describe GroupsController do it 'queries the expected amount for a group row' do control_count = ActiveRecord::QueryRecorder.new { get_list }.count - _new_group = create(:group, :public, parent: group) expect { get_list }.not_to exceed_query_limit(control_count + expected_queries_per_group) @@ -273,7 +313,6 @@ describe GroupsController do it 'queries the expected amount for a project row' do control_count = ActiveRecord::QueryRecorder.new { get_list }.count - _new_project = create(:project, :public, namespace: group) expect { get_list }.not_to exceed_query_limit(control_count + expected_queries_per_project) @@ -288,7 +327,6 @@ describe GroupsController do matched_group = create(:group, :public, parent: public_subgroup, name: 'filterme') control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count - nested_group = create(:group, :public, parent: public_subgroup) matched_group.update!(parent: nested_group) @@ -299,7 +337,6 @@ describe GroupsController do create(:group, :public, parent: public_subgroup, name: 'filterme') control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count - create(:group, :public, parent: public_subgroup, name: 'filterme2') expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_group) @@ -570,79 +607,61 @@ describe GroupsController do end end - context 'pagination' do - let!(:other_subgroup) { create(:group, :public, parent: group) } - let!(:project) { create(:project, :public, namespace: group) } - let!(:first_page_subgroups) { create_list(:group, Kaminari.config.default_per_page, parent: group) } + context 'for a POST request' do + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + post :update, id: group.to_param.upcase, group: { path: 'new_path' } - it 'contains all subgroups' do - get :children, id: group.to_param, sort: 'id', format: :json + expect(response).not_to have_http_status(404) + end - expect(assigns(:children)).to contain_exactly(*first_page_subgroups) + it 'does not redirect to the correct casing' do + post :update, id: group.to_param.upcase, group: { path: 'new_path' } + + expect(response).not_to have_http_status(301) + end end - it 'contains the project and group on the second page' do - get :children, id: group.to_param, sort: 'id', page: 2, format: :json + context 'when requesting a redirected path' do + let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } - expect(assigns(:children)).to contain_exactly(other_subgroup, project) + it 'returns not found' do + post :update, id: redirect_route.path, group: { path: 'new_path' } + + expect(response).to have_http_status(404) + end + end + end + + context 'for a DELETE request' do + context 'when requesting the canonical path with different casing' do + it 'does not 404' do + delete :destroy, id: group.to_param.upcase + + expect(response).not_to have_http_status(404) + end + + it 'does not redirect to the correct casing' do + delete :destroy, id: group.to_param.upcase + + expect(response).not_to have_http_status(301) + end + end + + context 'when requesting a redirected path' do + let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + + it 'returns not found' do + delete :destroy, id: redirect_route.path + + expect(response).to have_http_status(404) + end end end end - context 'for a POST request' do - context 'when requesting the canonical path with different casing' do - it 'does not 404' do - post :update, id: group.to_param.upcase, group: { path: 'new_path' } - - expect(response).not_to have_http_status(404) - end - - it 'does not redirect to the correct casing' do - post :update, id: group.to_param.upcase, group: { path: 'new_path' } - - expect(response).not_to have_http_status(301) - end - end - - context 'when requesting a redirected path' do - let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } - - it 'returns not found' do - post :update, id: redirect_route.path, group: { path: 'new_path' } - - expect(response).to have_http_status(404) - end - end + def group_moved_message(redirect_route, group) + "Group '#{redirect_route.path}' was moved to '#{group.full_path}'. Please update any links and bookmarks that may still have the old path." end - - context 'for a DELETE request' do - context 'when requesting the canonical path with different casing' do - it 'does not 404' do - delete :destroy, id: group.to_param.upcase - - expect(response).not_to have_http_status(404) - end - - it 'does not redirect to the correct casing' do - delete :destroy, id: group.to_param.upcase - - expect(response).not_to have_http_status(301) - end - end - - context 'when requesting a redirected path' do - let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } - - it 'returns not found' do - delete :destroy, id: redirect_route.path - - expect(response).to have_http_status(404) - end - end - end - end - - def group_moved_message(redirect_route, group) - "Group '#{redirect_route.path}' was moved to '#{group.full_path}'. Please update any links and bookmarks that may still have the old path." end end diff --git a/spec/features/dashboard/groups_list_spec.rb b/spec/features/dashboard/groups_list_spec.rb index 533df7a325c..227550e62be 100644 --- a/spec/features/dashboard/groups_list_spec.rb +++ b/spec/features/dashboard/groups_list_spec.rb @@ -6,6 +6,10 @@ feature 'Dashboard Groups page', :js do let!(:nested_group) { create(:group, :nested) } let!(:another_group) { create(:group) } + before do + pending('Update for new group tree') + end + it 'shows groups user is member of' do group.add_owner(user) nested_group.add_owner(user) diff --git a/spec/features/explore/groups_list_spec.rb b/spec/features/explore/groups_list_spec.rb index b5325301968..fa3d8f97a09 100644 --- a/spec/features/explore/groups_list_spec.rb +++ b/spec/features/explore/groups_list_spec.rb @@ -8,6 +8,8 @@ describe 'Explore Groups page', :js do let!(:empty_project) { create(:project, group: public_group) } before do + pending('Update for new group tree') + group.add_owner(user) sign_in(user)