From a1b3cd40647e8f7768b6db0bc64179e60f5d5937 Mon Sep 17 00:00:00 2001 From: Mircea Danila Dumitrescu Date: Mon, 2 Oct 2017 20:32:36 +0000 Subject: namespace should be lowercased in kubernetes. This is also true for the scenario where the namespace is generated from the project group-name. --- app/models/project_services/kubernetes_service.rb | 12 +++++++++++- changelogs/unreleased/mr-14642.yml | 6 ++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/mr-14642.yml diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index 8ba07173c74..45a544e3674 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -153,7 +153,17 @@ class KubernetesService < DeploymentService end def default_namespace - "#{project.path}-#{project.id}" if project.present? + return unless project + + # 1. lowercase + # 2. replace non kubernetes characters with dash + # 3. trim dash from the beginning and end + + slugified = "#{project.path}-#{project.id}" + slugified.downcase! + slugified.gsub!(/[^a-z0-9]/, '-') + slugified.gsub!(/^-+|-+$/, '') + slugified end def build_kubeclient!(api_path: 'api', api_version: 'v1') diff --git a/changelogs/unreleased/mr-14642.yml b/changelogs/unreleased/mr-14642.yml new file mode 100644 index 00000000000..048cc79e323 --- /dev/null +++ b/changelogs/unreleased/mr-14642.yml @@ -0,0 +1,6 @@ +--- +title: Auto Devops kubernetes default namespace is now correctly built out of gitlab + project group-name +merge_request: 14642 +author: Mircea Danila Dumitrescu +type: fixed -- cgit v1.2.1 From 063b5312111ccea62f84fa9f68a2262dc1f66e64 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 4 Sep 2017 16:23:55 +0200 Subject: Add a separate finder for collecting children of groups --- app/finders/group_children_finder.rb | 54 +++++++++++++++++++++++ spec/finders/group_children_finder_spec.rb | 71 ++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 app/finders/group_children_finder.rb create mode 100644 spec/finders/group_children_finder_spec.rb diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb new file mode 100644 index 00000000000..d95dfa2a877 --- /dev/null +++ b/app/finders/group_children_finder.rb @@ -0,0 +1,54 @@ +class GroupChildrenFinder + include Gitlab::Allowable + + attr_reader :current_user, :parent_group, :params + + def initialize(current_user = nil, parent_group:, params: {}) + @current_user = current_user + @parent_group = parent_group + @params = params + end + + def execute + Kaminari.paginate_array(children) + end + + # This allows us to fetch only the count without loading the objects. Unless + # the objects were already loaded. + def total_count + @total_count ||= if defined?(@children) + children.size + else + child_groups.count + projects.count + end + end + + private + + def children + @children ||= child_groups + projects + end + + def child_groups + return Group.none unless Group.supports_nested_groups? + return Group.none unless can?(current_user, :read_group, parent_group) + + groups = GroupsFinder.new(current_user, + parent: parent_group, + all_available: true, + all_children_for_parent: params[:filter_groups].present?).execute + + groups = groups.search(params[:filter]) if params[:filter].present? + groups = groups.includes(:route).includes(:children) + groups.sort(params[:sort]) + end + + def projects + return Project.none unless can?(current_user, :read_group, parent_group) + + projects = GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute + projects = projects.includes(:route) + projects = projects.search(params[:filter]) if params[:filter].present? + projects.sort(params[:sort]) + end +end diff --git a/spec/finders/group_children_finder_spec.rb b/spec/finders/group_children_finder_spec.rb new file mode 100644 index 00000000000..afd96e27a1d --- /dev/null +++ b/spec/finders/group_children_finder_spec.rb @@ -0,0 +1,71 @@ +require 'spec_helper' + +describe GroupChildrenFinder do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:params) { {} } + subject(:finder) { described_class.new(user, parent_group: group, params: params) } + + before do + group.add_owner(user) + end + + describe '#execute' do + it 'includes projects' do + project = create(:project, namespace: group) + + expect(finder.execute).to contain_exactly(project) + end + + context 'with a filter' do + let(:params) { { filter: 'test' } } + + it 'includes only projects matching the filter' do + _other_project = create(:project, namespace: group) + matching_project = create(:project, namespace: group, name: 'testproject') + + expect(finder.execute).to contain_exactly(matching_project) + end + end + end + + context 'with nested groups', :nested_groups do + let!(:project) { create(:project, namespace: group) } + let!(:subgroup) { create(:group, parent: group) } + + describe '#execute' do + it 'contains projects and subgroups' do + expect(finder.execute).to contain_exactly(subgroup, project) + end + + context 'with a filter' do + let(:params) { { filter: 'test' } } + + it 'contains only matching projects and subgroups' do + matching_project = create(:project, namespace: group, name: 'Testproject') + matching_subgroup = create(:group, name: 'testgroup', parent: group) + + expect(finder.execute).to contain_exactly(matching_subgroup, matching_project) + end + end + end + + describe '#total_count' do + it 'counts the array children were already loaded' do + finder.instance_variable_set(:@children, [double]) + + expect(finder).not_to receive(:child_groups) + expect(finder).not_to receive(:projects) + + expect(finder.total_count).to eq(1) + end + + it 'performs a count without loading children when they are not loaded yet' do + expect(finder).to receive(:child_groups).and_call_original + expect(finder).to receive(:projects).and_call_original + + expect(finder.total_count).to eq(2) + end + end + end +end -- cgit v1.2.1 From ca538899b66a6a82582d2d590297cfef1d310dcf Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 4 Sep 2017 18:04:33 +0200 Subject: Add a `WithPagination` concern to reuse across serializers --- app/serializers/concerns/with_pagination.rb | 20 ++++++++++++++++++++ app/serializers/environment_serializer.rb | 12 ++---------- app/serializers/group_serializer.rb | 18 ++---------------- app/serializers/pipeline_serializer.rb | 10 ++-------- config/application.rb | 1 + 5 files changed, 27 insertions(+), 34 deletions(-) create mode 100644 app/serializers/concerns/with_pagination.rb diff --git a/app/serializers/concerns/with_pagination.rb b/app/serializers/concerns/with_pagination.rb new file mode 100644 index 00000000000..484c6855f7c --- /dev/null +++ b/app/serializers/concerns/with_pagination.rb @@ -0,0 +1,20 @@ +module WithPagination + def with_pagination(request, response) + tap { @paginator = Gitlab::Serializer::Pagination.new(request, response) } + end + + def paginated? + @paginator.present? + end + + # super is `BaseSerializer#represent` here. + # + # we shouldn't try to paginate single resources + def represent(resource, opts = {}) + if paginated? && resource.respond_to?(:page) + super(@paginator.paginate(resource), opts) + else + super(resource, opts) + end + end +end diff --git a/app/serializers/environment_serializer.rb b/app/serializers/environment_serializer.rb index 88842a9aa75..84722f33f59 100644 --- a/app/serializers/environment_serializer.rb +++ b/app/serializers/environment_serializer.rb @@ -1,4 +1,6 @@ class EnvironmentSerializer < BaseSerializer + include WithPagination + Item = Struct.new(:name, :size, :latest) entity EnvironmentEntity @@ -7,18 +9,10 @@ class EnvironmentSerializer < BaseSerializer tap { @itemize = true } end - def with_pagination(request, response) - tap { @paginator = Gitlab::Serializer::Pagination.new(request, response) } - end - def itemized? @itemize end - def paginated? - @paginator.present? - end - def represent(resource, opts = {}) if itemized? itemize(resource).map do |item| @@ -27,8 +21,6 @@ class EnvironmentSerializer < BaseSerializer latest: super(item.latest, opts) } end else - resource = @paginator.paginate(resource) if paginated? - super(resource, opts) end end diff --git a/app/serializers/group_serializer.rb b/app/serializers/group_serializer.rb index 26e8566828b..8cf7eb63bcf 100644 --- a/app/serializers/group_serializer.rb +++ b/app/serializers/group_serializer.rb @@ -1,19 +1,5 @@ class GroupSerializer < BaseSerializer - entity GroupEntity - - def with_pagination(request, response) - tap { @paginator = Gitlab::Serializer::Pagination.new(request, response) } - end + include WithPagination - def paginated? - @paginator.present? - end - - def represent(resource, opts = {}) - if paginated? - super(@paginator.paginate(resource), opts) - else - super(resource, opts) - end - end + entity GroupEntity end diff --git a/app/serializers/pipeline_serializer.rb b/app/serializers/pipeline_serializer.rb index 661bf17983c..7181f8a6b04 100644 --- a/app/serializers/pipeline_serializer.rb +++ b/app/serializers/pipeline_serializer.rb @@ -1,16 +1,10 @@ class PipelineSerializer < BaseSerializer + include WithPagination + InvalidResourceError = Class.new(StandardError) entity PipelineDetailsEntity - def with_pagination(request, response) - tap { @paginator = Gitlab::Serializer::Pagination.new(request, response) } - end - - def paginated? - @paginator.present? - end - def represent(resource, opts = {}) if resource.is_a?(ActiveRecord::Relation) diff --git a/config/application.rb b/config/application.rb index ca2ab83becc..bcb9f59ae3e 100644 --- a/config/application.rb +++ b/config/application.rb @@ -29,6 +29,7 @@ module Gitlab #{config.root}/app/models/project_services #{config.root}/app/workers/concerns #{config.root}/app/services/concerns + #{config.root}/app/serializers/concerns #{config.root}/app/finders/concerns]) config.generators.templates.push("#{config.root}/generator_templates") -- cgit v1.2.1 From 2eac1537ad907f2f7e628788cf980cb7e48d3f56 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 4 Sep 2017 20:01:58 +0200 Subject: Fetch children using new finder for the `show` of a group. --- app/controllers/groups_controller.rb | 11 ++++------- app/finders/group_children_finder.rb | 29 ++++++++++++++++++++--------- app/views/groups/_children.html.haml | 4 ++++ app/views/groups/_show_nav.html.haml | 8 -------- app/views/groups/show.html.haml | 3 +-- spec/finders/group_children_finder_spec.rb | 6 +++--- 6 files changed, 32 insertions(+), 29 deletions(-) create mode 100644 app/views/groups/_children.html.haml delete mode 100644 app/views/groups/_show_nav.html.haml diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 3769a2cde33..588995ab5a8 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -45,18 +45,15 @@ class GroupsController < Groups::ApplicationController end def show - setup_projects + @children = GroupChildrenFinder.new(current_user, parent_group: @group, params: params).execute + + @children = @children.page(params[:page]) respond_to do |format| format.html - format.json do - render json: { - html: view_to_html_string("dashboard/projects/_projects", locals: { projects: @projects }) - } - end - format.atom do + setup_projects load_events render layout: 'xml.atom' end diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index d95dfa2a877..71bef7b0ce2 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -16,27 +16,38 @@ class GroupChildrenFinder # This allows us to fetch only the count without loading the objects. Unless # the objects were already loaded. def total_count - @total_count ||= if defined?(@children) - children.size - else - child_groups.count + projects.count - end + @total_count ||= subgroup_count + project_count + end + + def subgroup_count + @subgroup_count ||= if defined?(@children) + children.count { |child| child.is_a?(Group) } + else + subgroups.count + end + end + + def project_count + @project_count ||= if defined?(@children) + children.count { |child| child.is_a?(Project) } + else + projects.count + end end private def children - @children ||= child_groups + projects + @children ||= subgroups + projects end - def child_groups + def subgroups return Group.none unless Group.supports_nested_groups? return Group.none unless can?(current_user, :read_group, parent_group) groups = GroupsFinder.new(current_user, parent: parent_group, - all_available: true, - all_children_for_parent: params[:filter_groups].present?).execute + all_available: true).execute groups = groups.search(params[:filter]) if params[:filter].present? groups = groups.includes(:route).includes(:children) diff --git a/app/views/groups/_children.html.haml b/app/views/groups/_children.html.haml new file mode 100644 index 00000000000..e22d9cc6013 --- /dev/null +++ b/app/views/groups/_children.html.haml @@ -0,0 +1,4 @@ +- if children.any? + render children here +- else + .nothing-here-block No children found diff --git a/app/views/groups/_show_nav.html.haml b/app/views/groups/_show_nav.html.haml deleted file mode 100644 index 35b75bc0923..00000000000 --- a/app/views/groups/_show_nav.html.haml +++ /dev/null @@ -1,8 +0,0 @@ -%ul.nav-links - = nav_link(page: group_path(@group)) do - = link_to group_path(@group) do - Projects - - if Group.supports_nested_groups? - = nav_link(page: subgroups_group_path(@group)) do - = link_to subgroups_group_path(@group) do - Subgroups diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 3ca63f9c3e0..a8842596dbd 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -8,7 +8,6 @@ .groups-header{ class: container_class } .top-area - = render 'groups/show_nav' .nav-controls = render 'shared/projects/search_form' = render 'shared/projects/dropdown' @@ -16,4 +15,4 @@ = link_to new_project_path(namespace_id: @group.id), class: 'btn btn-new pull-right' do New Project - = render "projects", projects: @projects + = render "children", children: @children diff --git a/spec/finders/group_children_finder_spec.rb b/spec/finders/group_children_finder_spec.rb index afd96e27a1d..a2a24b2a12e 100644 --- a/spec/finders/group_children_finder_spec.rb +++ b/spec/finders/group_children_finder_spec.rb @@ -52,16 +52,16 @@ describe GroupChildrenFinder do describe '#total_count' do it 'counts the array children were already loaded' do - finder.instance_variable_set(:@children, [double]) + finder.instance_variable_set(:@children, [build(:project)]) - expect(finder).not_to receive(:child_groups) + expect(finder).not_to receive(:subgroups) expect(finder).not_to receive(:projects) expect(finder.total_count).to eq(1) end it 'performs a count without loading children when they are not loaded yet' do - expect(finder).to receive(:child_groups).and_call_original + expect(finder).to receive(:subgroups).and_call_original expect(finder).to receive(:projects).and_call_original expect(finder.total_count).to eq(2) -- cgit v1.2.1 From 376a8c66c1ca8ee2a95255d21c9d55ce006ab655 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 5 Sep 2017 10:03:43 +0200 Subject: Remove the subgroups path on a group --- app/controllers/groups_controller.rb | 4 ---- app/views/groups/subgroups.html.haml | 21 --------------------- config/routes/group.rb | 1 - spec/controllers/groups_controller_spec.rb | 1 + spec/features/groups_spec.rb | 22 +++++++++------------- 5 files changed, 10 insertions(+), 39 deletions(-) delete mode 100644 app/views/groups/subgroups.html.haml diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 588995ab5a8..b8def9f5812 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -60,11 +60,7 @@ class GroupsController < Groups::ApplicationController end end - def subgroups - return not_found unless Group.supports_nested_groups? - @nested_groups = GroupsFinder.new(current_user, parent: group).execute - @nested_groups = @nested_groups.search(params[:filter_groups]) if params[:filter_groups].present? end def activity diff --git a/app/views/groups/subgroups.html.haml b/app/views/groups/subgroups.html.haml deleted file mode 100644 index 869b3b243c6..00000000000 --- a/app/views/groups/subgroups.html.haml +++ /dev/null @@ -1,21 +0,0 @@ -- breadcrumb_title "Details" -- @no_container = true - -= render 'groups/home_panel' - -.groups-header{ class: container_class } - .top-area - = render 'groups/show_nav' - .nav-controls - = form_tag request.path, method: :get do |f| - = search_field_tag :filter_groups, params[:filter_groups], placeholder: 'Filter by name', class: 'form-control', spellcheck: false - - if can?(current_user, :create_subgroup, @group) - = link_to new_group_path(parent_id: @group.id), class: 'btn btn-new pull-right' do - New Subgroup - - - if @nested_groups.present? - %ul.content-list - = render partial: 'shared/groups/group', collection: @nested_groups, locals: { full_name: false } - - else - .nothing-here-block - There are no subgroups to show. diff --git a/config/routes/group.rb b/config/routes/group.rb index 23052a6c6dc..e71a3c3b190 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -41,7 +41,6 @@ scope(path: 'groups/*id', 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 diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index b0564e27a68..83a2e82d78c 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -157,6 +157,7 @@ describe GroupsController do context 'as a user' do before do sign_in(user) + pending('spec the children path instead') end it 'shows all subgroups' do diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index 4ec2e7e6012..493dd551d25 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -90,7 +90,10 @@ feature 'Group' do context 'as admin' do before do - visit subgroups_group_path(group) + visit group_path(group) + + pending('use the new subgroup button') + click_link 'New Subgroup' end @@ -111,7 +114,10 @@ feature 'Group' do sign_out(:user) sign_in(user) - visit subgroups_group_path(group) + visit group_path(group) + + pending('use the new subgroup button') + click_link 'New Subgroup' fill_in 'Group path', with: 'bar' click_button 'Create group' @@ -120,16 +126,6 @@ feature 'Group' do expect(page).to have_content("Group 'bar' was successfully created.") end end - - context 'when nested group feature is disabled' do - it 'renders 404' do - allow(Group).to receive(:supports_nested_groups?).and_return(false) - - visit subgroups_group_path(group) - - expect(page.status_code).to eq(404) - end - end end it 'checks permissions to avoid exposing groups by parent_id' do @@ -213,8 +209,8 @@ feature 'Group' do let!(:path) { group_path(group) } it 'has nested groups tab with nested groups inside' do + pending('the child should be visible on the show page') visit path - click_link 'Subgroups' expect(page).to have_content(nested_group.name) end -- cgit v1.2.1 From d33e15574b064e38ceadf8aafb47af985d1a7a7a Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 5 Sep 2017 11:30:16 +0200 Subject: Add serializer for group children --- app/controllers/groups_controller.rb | 15 +++++++++++++++ app/serializers/group_child_entity.rb | 20 ++++++++++++++++++++ app/serializers/group_child_serializer.rb | 5 +++++ spec/serializers/group_child_entity_spec.rb | 15 +++++++++++++++ 4 files changed, 55 insertions(+) create mode 100644 app/serializers/group_child_entity.rb create mode 100644 app/serializers/group_child_serializer.rb create mode 100644 spec/serializers/group_child_entity_spec.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index b8def9f5812..17d40cedd5d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -60,7 +60,22 @@ class GroupsController < Groups::ApplicationController end end + def children + parent = Group.find_by(parent_id: params[:parent_id]) || @group + if parent.nil? || !can?(current_user, :read_group, parent) + render_404 + end + + @children = GroupChildrenFinder.new(current_user, parent_group: parent, params: params).execute + respond_to do |format| + format.json do + render json: GroupChildrenSerializer + .new(current_user: current_user) + .with_pagination(request, response) + .represent(@children) + end + end end def activity diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb new file mode 100644 index 00000000000..0d2052fe6f3 --- /dev/null +++ b/app/serializers/group_child_entity.rb @@ -0,0 +1,20 @@ +class GroupChildEntity < Grape::Entity + include ActionView::Helpers::NumberHelper + include RequestAwareEntity + + expose :id, :name, :description, :visibility, :full_name, :full_path, :web_url, + :created_at, :updated_at, :star_count, :can_edit, :type, :parent_id, + :children_count, :leave_path, :edit_path, :number_projects_with_delimiter, + :number_users_with_delimiter, :permissions, :star_count + + def type + object.class.name.downcase + end + + def can_edit + return false unless request.respond_to?(:current_user) + + can?(request.current_user, "edit_{type}", object) + end + expose +end diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb new file mode 100644 index 00000000000..fbf4a6783b9 --- /dev/null +++ b/app/serializers/group_child_serializer.rb @@ -0,0 +1,5 @@ +class GroupChildSerializer < BaseSerializer + include WithPagination + + entity GroupChildEntity +end diff --git a/spec/serializers/group_child_entity_spec.rb b/spec/serializers/group_child_entity_spec.rb new file mode 100644 index 00000000000..1c4cdc2a5fb --- /dev/null +++ b/spec/serializers/group_child_entity_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe GroupChildEntity do + let(:request) { double('request') } + let(:entity) { described_class.new(object, request: request) } + subject(:json) { entity.as_json } + + describe 'for a project' do + let(:object) { build_stubbed(:project) } + + it 'has the correct type' do + expect(json[:type]).to eq('project') + end + end +end -- cgit v1.2.1 From 648c082a23f51bdf7151b6c5f716e74c4fe6a5bd Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 5 Sep 2017 16:18:24 +0200 Subject: Render group children using the same entity --- app/serializers/group_child_entity.rb | 70 ++++++++++++++++++++++++--- spec/serializers/group_child_entity_spec.rb | 75 ++++++++++++++++++++++++++++- 2 files changed, 138 insertions(+), 7 deletions(-) diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index 0d2052fe6f3..bc870541795 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -2,10 +2,12 @@ class GroupChildEntity < Grape::Entity include ActionView::Helpers::NumberHelper include RequestAwareEntity - expose :id, :name, :description, :visibility, :full_name, :full_path, :web_url, - :created_at, :updated_at, :star_count, :can_edit, :type, :parent_id, - :children_count, :leave_path, :edit_path, :number_projects_with_delimiter, - :number_users_with_delimiter, :permissions, :star_count + expose :id, :name, :path, :description, :visibility, :full_name, :full_path, :web_url, + :created_at, :updated_at, :can_edit, :type, :avatar_url, :permission, :edit_path + + def project? + object.is_a?(Project) + end def type object.class.name.downcase @@ -14,7 +16,63 @@ class GroupChildEntity < Grape::Entity def can_edit return false unless request.respond_to?(:current_user) - can?(request.current_user, "edit_{type}", object) + if project? + can?(request.current_user, :admin_project, object) + else + can?(request.current_user, :admin_group, object) + end + end + + def edit_path + if project? + edit_project_path(object) + else + edit_group_path(object) + end + end + + def permission + if project? + object.project_members.find_by(user_id: request.current_user)&.human_access + else + object.group_members.find_by(user_id: request.current_user)&.human_access + end + end + + # Project only attributes + expose :star_count, + if: lambda { |_instance, _options| project? } + + # Group only attributes + expose :children_count, :leave_path, :parent_id, :number_projects_with_delimiter, + :number_users_with_delimiter, :project_count, :subgroup_count, + unless: lambda { |_instance, _options| project? } + + def children_finder + @children_finder ||= GroupChildrenFinder.new(request.current_user, parent_group: object) + end + + def children_count + @children_count ||= children_finder.total_count + end + + def project_count + @project_count ||= children_finder.project_count + end + + def subgroup_count + @subgroup_count ||= children_finder.subgroup_count + end + + def leave_path + leave_group_group_members_path(object) + end + + def number_projects_with_delimiter + number_with_delimiter(project_count) + end + + def number_users_with_delimiter + number_with_delimiter(object.users.count) end - expose end diff --git a/spec/serializers/group_child_entity_spec.rb b/spec/serializers/group_child_entity_spec.rb index 1c4cdc2a5fb..3a7d2205f53 100644 --- a/spec/serializers/group_child_entity_spec.rb +++ b/spec/serializers/group_child_entity_spec.rb @@ -1,15 +1,88 @@ require 'spec_helper' describe GroupChildEntity do + let(:user) { create(:user) } let(:request) { double('request') } let(:entity) { described_class.new(object, request: request) } subject(:json) { entity.as_json } + before do + allow(request).to receive(:current_user).and_return(user) + end + + shared_examples 'group child json' do + it 'renders json' do + is_expected.not_to be_nil + end + + %w[id + path + full_name + full_path + avatar_url + name + description + web_url + visibility + type + can_edit + visibility + edit_path + permission].each do |attribute| + it "includes #{attribute}" do + expect(json[attribute.to_sym]).to be_present + end + end + end + describe 'for a project' do - let(:object) { build_stubbed(:project) } + set(:object) do + create(:project, :with_avatar, + description: 'Awesomeness') + end + + before do + object.add_master(user) + end it 'has the correct type' do expect(json[:type]).to eq('project') end + + it 'includes the star count' do + expect(json[:star_count]).to be_present + end + + it_behaves_like 'group child json' + end + + describe 'for a group', :nested_groups do + set(:object) do + create(:group, :nested, :with_avatar, + description: 'Awesomeness') + end + + before do + object.add_owner(user) + end + + it 'has the correct type' do + expect(json[:type]).to eq('group') + end + + it 'counts projects and subgroups as children' do + create(:project, namespace: object) + create(:group, parent: object) + + expect(json[:children_count]).to eq(2) + end + + %w[children_count leave_path parent_id number_projects_with_delimiter number_users_with_delimiter project_count subgroup_count].each do |attribute| + it "includes #{attribute}" do + expect(json[attribute.to_sym]).to be_present + end + end + + it_behaves_like 'group child json' end end -- cgit v1.2.1 From 80780018a931ce41047ab62ed7dd6c5f1e28f08b Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 5 Sep 2017 16:57:46 +0200 Subject: Update `children` route to handle projects and groups --- app/controllers/groups_controller.rb | 8 ++- config/routes/group.rb | 3 ++ lib/gitlab/path_regex.rb | 1 - spec/controllers/groups_controller_spec.rb | 82 ++++++++++++++++++++++-------- spec/lib/gitlab/path_regex_spec.rb | 14 ++--- 5 files changed, 77 insertions(+), 31 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 17d40cedd5d..c967488fff4 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -61,7 +61,11 @@ class GroupsController < Groups::ApplicationController end def children - parent = Group.find_by(parent_id: params[:parent_id]) || @group + parent = if params[:parent_id].present? + Group.find(params[:parent_id]) + else + @group + end if parent.nil? || !can?(current_user, :read_group, parent) render_404 end @@ -70,7 +74,7 @@ class GroupsController < Groups::ApplicationController respond_to do |format| format.json do - render json: GroupChildrenSerializer + render json: GroupChildSerializer .new(current_user: current_user) .with_pagination(request, response) .represent(@children) diff --git a/config/routes/group.rb b/config/routes/group.rb index e71a3c3b190..0f0ece61a38 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -41,6 +41,9 @@ scope(path: 'groups/*id', get :merge_requests, as: :merge_requests_group get :projects, as: :projects_group get :activity, as: :activity_group + scope(path: '-') do + get :children, as: :group_children + end get '/', action: :show, as: :group_canonical end diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 7c02c9c5c48..e2fbcefdb74 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -129,7 +129,6 @@ module Gitlab notification_setting pipeline_quota projects - subgroups ].freeze ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 83a2e82d78c..c96a44d6186 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -150,39 +150,79 @@ describe GroupsController do end end - describe 'GET #subgroups', :nested_groups do - let!(:public_subgroup) { create(:group, :public, parent: group) } - let!(:private_subgroup) { create(:group, :private, parent: group) } + 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) - pending('spec the children path instead') + 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 - it 'shows all subgroups' do - get :subgroups, id: group.to_param + context 'as a guest' do + it 'shows the public children' do + get :children, id: group.to_param, format: :json - expect(assigns(:nested_groups)).to contain_exactly(public_subgroup, private_subgroup) + expect(assigns(:children)).to contain_exactly(public_project) + end end + end - context 'being member of private subgroup' do - it 'shows public and private subgroups the user is member of' do - group_member.destroy! - private_subgroup.add_guest(user) + 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) } - get :subgroups, id: group.to_param + context 'as a user' do + before do + sign_in(user) + end - expect(assigns(:nested_groups)).to contain_exactly(public_subgroup, private_subgroup) + 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 - end - context 'as a guest' do - it 'shows the public subgroups' do - get :subgroups, id: group.to_param + context 'as a guest' do + it 'shows the public children' do + get :children, id: group.to_param, format: :json - expect(assigns(:nested_groups)).to contain_exactly(public_subgroup) + expect(assigns(:children)).to contain_exactly(public_subgroup, public_project) + end end end end diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb index 1f1c48ee9b5..f1f188cbfb5 100644 --- a/spec/lib/gitlab/path_regex_spec.rb +++ b/spec/lib/gitlab/path_regex_spec.rb @@ -213,7 +213,7 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/') expect(subject).to match('group_members/') - expect(subject).to match('subgroups/') + expect(subject).to match('labels/') end it 'is not case sensitive' do @@ -246,7 +246,7 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/') expect(subject).to match('group_members/') - expect(subject).to match('subgroups/') + expect(subject).to match('labels/') end end @@ -268,7 +268,7 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/more/') expect(subject).to match('group_members/more/') - expect(subject).to match('subgroups/more/') + expect(subject).to match('labels/more/') end end end @@ -292,7 +292,7 @@ describe Gitlab::PathRegex do it 'rejects group routes' do expect(subject).not_to match('root/activity/') expect(subject).not_to match('root/group_members/') - expect(subject).not_to match('root/subgroups/') + expect(subject).not_to match('root/labels/') end end @@ -314,7 +314,7 @@ describe Gitlab::PathRegex do it 'rejects group routes' do expect(subject).not_to match('root/activity/more/') expect(subject).not_to match('root/group_members/more/') - expect(subject).not_to match('root/subgroups/more/') + expect(subject).not_to match('root/labels/more/') end end end @@ -349,7 +349,7 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('activity/') expect(subject).to match('group_members/') - expect(subject).to match('subgroups/') + expect(subject).to match('labels/') end it 'is not case sensitive' do @@ -382,7 +382,7 @@ describe Gitlab::PathRegex do it 'accepts group routes' do expect(subject).to match('root/activity/') expect(subject).to match('root/group_members/') - expect(subject).to match('root/subgroups/') + expect(subject).to match('root/labels/') end it 'is not case sensitive' do -- cgit v1.2.1 From 28c440045ed001ab6029131ab7c842b390f4e186 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 5 Sep 2017 17:27:55 +0200 Subject: Add pagination for children --- app/controllers/groups_controller.rb | 2 +- spec/controllers/groups_controller_spec.rb | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index c967488fff4..4f0d1f88e58 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -46,7 +46,6 @@ class GroupsController < Groups::ApplicationController def show @children = GroupChildrenFinder.new(current_user, parent_group: @group, params: params).execute - @children = @children.page(params[:page]) respond_to do |format| @@ -71,6 +70,7 @@ class GroupsController < Groups::ApplicationController end @children = GroupChildrenFinder.new(current_user, parent_group: parent, params: params).execute + @children = @children.page(params[:page]) respond_to do |format| format.json do diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index c96a44d6186..9b4654dc3e4 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -466,6 +466,24 @@ describe GroupsController do end 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) } + + it 'contains all subgroups' do + get :children, id: group.to_param, sort: 'id', 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', page: 2, format: :json + + expect(assigns(:children)).to contain_exactly(other_subgroup, project) + end + end end context 'for a POST request' do -- cgit v1.2.1 From 9f3995a0ca3d56fd8d219a2b01c3e6555fd91f27 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 6 Sep 2017 15:28:07 +0200 Subject: Find all children matching a query --- app/finders/group_children_finder.rb | 41 +++++++++++++++++++++++++----- spec/finders/group_children_finder_spec.rb | 14 ++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index 71bef7b0ce2..31b122e10f6 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -41,25 +41,52 @@ class GroupChildrenFinder @children ||= subgroups + projects end + def base_groups + GroupsFinder.new(current_user, + parent: parent_group, + all_available: true).execute + end + + def all_subgroups + Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).all_groups + end + + def subgroups_matching_filter + all_subgroups.search(params[:filter]) + end + def subgroups return Group.none unless Group.supports_nested_groups? return Group.none unless can?(current_user, :read_group, parent_group) - groups = GroupsFinder.new(current_user, - parent: parent_group, - all_available: true).execute - - groups = groups.search(params[:filter]) if params[:filter].present? + groups = if params[:filter] + subgroups_matching_filter + else + base_groups + end groups = groups.includes(:route).includes(:children) groups.sort(params[:sort]) end + def base_projects + GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute + end + + def projects_matching_filter + ProjectsFinder.new(current_user: current_user).execute + .search(params[:filter]) + .where(namespace: all_subgroups) + end + def projects return Project.none unless can?(current_user, :read_group, parent_group) - projects = GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute + projects = if params[:filter] + projects_matching_filter + else + base_projects + end projects = projects.includes(:route) - projects = projects.search(params[:filter]) if params[:filter].present? projects.sort(params[:sort]) end end diff --git a/spec/finders/group_children_finder_spec.rb b/spec/finders/group_children_finder_spec.rb index a2a24b2a12e..8f83f88bb97 100644 --- a/spec/finders/group_children_finder_spec.rb +++ b/spec/finders/group_children_finder_spec.rb @@ -47,6 +47,20 @@ describe GroupChildrenFinder do expect(finder.execute).to contain_exactly(matching_subgroup, matching_project) end + + context 'with matching children' do + it 'includes a group that has a subgroup matching the query' do + matching_subgroup = create(:group, name: 'testgroup', parent: subgroup) + + expect(finder.execute).to contain_exactly(matching_subgroup) + end + + it 'includes a group that has a project matching the query' do + matching_project = create(:project, namespace: subgroup, name: 'Testproject') + + expect(finder.execute).to contain_exactly(matching_project) + end + end end end -- cgit v1.2.1 From 438a0773dc850d3fa690881ea0b022bc27435e1e Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 6 Sep 2017 16:29:52 +0200 Subject: Add a concern to build hierarchies of groups --- app/models/concerns/group_hierarchy.rb | 27 ++++++++++++ app/models/group.rb | 1 + app/models/project.rb | 1 + spec/models/concerns/group_hierarchy_spec.rb | 61 ++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+) create mode 100644 app/models/concerns/group_hierarchy.rb create mode 100644 spec/models/concerns/group_hierarchy_spec.rb diff --git a/app/models/concerns/group_hierarchy.rb b/app/models/concerns/group_hierarchy.rb new file mode 100644 index 00000000000..03864023771 --- /dev/null +++ b/app/models/concerns/group_hierarchy.rb @@ -0,0 +1,27 @@ +module GroupHierarchy + def hierarchy(hierarchy_base = nil) + @hierarchy ||= tree_for_child(self, self, hierarchy_base) + end + + def parent + if self.is_a?(Project) + namespace + else + super + end + end + + def tree_for_child(child, tree, hierarchy_base) + if child.parent.nil? && hierarchy_base.present? + raise ArgumentError.new('specified base is not part of the tree') + end + + if child.parent != hierarchy_base + tree_for_child(child.parent, + { child.parent => tree }, + hierarchy_base) + else + tree + end + end +end diff --git a/app/models/group.rb b/app/models/group.rb index e746e4a12c9..848e422e067 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -6,6 +6,7 @@ class Group < Namespace include Avatarable include Referable include SelectForProjectAuthorization + include GroupHierarchy has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :group_members diff --git a/app/models/project.rb b/app/models/project.rb index 59b5a5b3cd7..c221055f8b4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -17,6 +17,7 @@ class Project < ActiveRecord::Base include ProjectFeaturesCompatibility include SelectForProjectAuthorization include Routable + include GroupHierarchy extend Gitlab::ConfigHelper extend Gitlab::CurrentSettings diff --git a/spec/models/concerns/group_hierarchy_spec.rb b/spec/models/concerns/group_hierarchy_spec.rb new file mode 100644 index 00000000000..14ac910c90d --- /dev/null +++ b/spec/models/concerns/group_hierarchy_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' + +describe GroupHierarchy, :nested_groups do + let(:parent) { create(:group) } + let(:subgroup) { create(:group, parent: parent) } + let(:subsub_group) { create(:group, parent: subgroup) } + + context 'for a group' do + describe '#hierarchy' do + it 'builds a hierarchy for a group' do + expected_hierarchy = { parent => { subgroup => subsub_group } } + + expect(subsub_group.hierarchy).to eq(expected_hierarchy) + end + + it 'builds a hierarchy upto a specified parent' do + expected_hierarchy = { subgroup => subsub_group } + + expect(subsub_group.hierarchy(parent)).to eq(expected_hierarchy) + end + + it 'raises an error if specifying a base that is not part of the tree' do + expect { subsub_group.hierarchy(double) }.to raise_error('specified base is not part of the tree') + end + end + + describe '#parent' do + it 'returns the correct parent' do + expect(subsub_group.parent).to eq(subgroup) + end + end + end + + context 'for a project' do + let(:project) { create(:project, namespace: subsub_group) } + + describe '#hierarchy' do + it 'builds a hierarchy for a group' do + expected_hierarchy = { parent => { subgroup => { subsub_group => project } } } + + expect(project.hierarchy).to eq(expected_hierarchy) + end + + it 'builds a hierarchy upto a specified parent' do + expected_hierarchy = { subsub_group => project } + + expect(project.hierarchy(subgroup)).to eq(expected_hierarchy) + end + + it 'raises an error if specifying a base that is not part of the tree' do + expect { project.hierarchy(double) }.to raise_error('specified base is not part of the tree') + end + end + + describe '#parent' do + it 'returns the correct parent' do + expect(project.parent).to eq(subsub_group) + end + end + end +end -- cgit v1.2.1 From 530cf2a2669ea1ee3c41d48a15919f875babefa4 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 7 Sep 2017 11:46:58 +0200 Subject: Don't break when building unions on empty collections --- lib/gitlab/sql/union.rb | 6 +++++- spec/lib/gitlab/sql/union_spec.rb | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/sql/union.rb b/lib/gitlab/sql/union.rb index f30c771837a..c99b262f1ca 100644 --- a/lib/gitlab/sql/union.rb +++ b/lib/gitlab/sql/union.rb @@ -26,7 +26,11 @@ module Gitlab @relations.map { |rel| rel.reorder(nil).to_sql }.reject(&:blank?) end - fragments.join("\n#{union_keyword}\n") + if fragments.any? + fragments.join("\n#{union_keyword}\n") + else + 'NULL' + end end def union_keyword diff --git a/spec/lib/gitlab/sql/union_spec.rb b/spec/lib/gitlab/sql/union_spec.rb index 8026fba9f0a..fe6422c32b6 100644 --- a/spec/lib/gitlab/sql/union_spec.rb +++ b/spec/lib/gitlab/sql/union_spec.rb @@ -29,5 +29,12 @@ describe Gitlab::SQL::Union do expect(union.to_sql).to include('UNION ALL') end + + it 'returns `NULL` if all relations are empty' do + empty_relation = User.none + union = described_class.new([empty_relation, empty_relation]) + + expect(union.to_sql).to eq('NULL') + end end end -- cgit v1.2.1 From 518216c0627cb6c4b3db62f10877b44d0e912ddb Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 7 Sep 2017 19:08:56 +0200 Subject: Merge group hierarchies when parents are shared --- app/controllers/groups_controller.rb | 1 + app/finders/group_children_finder.rb | 3 +- app/finders/group_projects_finder.rb | 1 - app/models/concerns/group_hierarchy.rb | 47 ++++++++++++++++- app/serializers/base_serializer.rb | 3 ++ app/serializers/group_child_serializer.rb | 37 ++++++++++++++ spec/models/concerns/group_hierarchy_spec.rb | 68 +++++++++++++++++++++++++ spec/serializers/group_child_serializer_spec.rb | 43 ++++++++++++++++ 8 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 spec/serializers/group_child_serializer_spec.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 4f0d1f88e58..a714f2cc7b0 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -77,6 +77,7 @@ class GroupsController < Groups::ApplicationController render json: GroupChildSerializer .new(current_user: current_user) .with_pagination(request, response) + .hierarchy_base(parent, open_hierarchy: filter[:filter].present) .represent(@children) end end diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index 31b122e10f6..eb0129947e2 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -52,7 +52,7 @@ class GroupChildrenFinder end def subgroups_matching_filter - all_subgroups.search(params[:filter]) + all_subgroups.search(params[:filter]).include(:parent) end def subgroups @@ -75,6 +75,7 @@ class GroupChildrenFinder def projects_matching_filter ProjectsFinder.new(current_user: current_user).execute .search(params[:filter]) + .include(:namespace) .where(namespace: all_subgroups) end diff --git a/app/finders/group_projects_finder.rb b/app/finders/group_projects_finder.rb index f2d3b90b8e2..6e8733bb49c 100644 --- a/app/finders/group_projects_finder.rb +++ b/app/finders/group_projects_finder.rb @@ -34,7 +34,6 @@ class GroupProjectsFinder < ProjectsFinder else collection_without_user end - union(projects) end diff --git a/app/models/concerns/group_hierarchy.rb b/app/models/concerns/group_hierarchy.rb index 03864023771..f7a62c9a607 100644 --- a/app/models/concerns/group_hierarchy.rb +++ b/app/models/concerns/group_hierarchy.rb @@ -1,6 +1,6 @@ module GroupHierarchy def hierarchy(hierarchy_base = nil) - @hierarchy ||= tree_for_child(self, self, hierarchy_base) + tree_for_child(self, self, hierarchy_base) end def parent @@ -16,7 +16,7 @@ module GroupHierarchy raise ArgumentError.new('specified base is not part of the tree') end - if child.parent != hierarchy_base + if child.parent && child.parent != hierarchy_base tree_for_child(child.parent, { child.parent => tree }, hierarchy_base) @@ -24,4 +24,47 @@ module GroupHierarchy tree end end + + def merge_hierarchy(other_element, hierarchy_base = nil) + GroupHierarchy.merge_hierarchies([self, other_element], hierarchy_base) + end + + def self.merge_hierarchies(hierarchies, hierarchy_base = nil) + hierarchies = Array.wrap(hierarchies) + return if hierarchies.empty? + + unless hierarchies.all? { |other_base| other_base.is_a?(GroupHierarchy) } + raise ArgumentError.new('element is not a hierarchy') + end + + first_hierarchy, *other_hierarchies = hierarchies + merged = first_hierarchy.hierarchy(hierarchy_base) + + other_hierarchies.each do |child| + next_hierarchy = child.hierarchy(hierarchy_base) + merged = merge_values(merged, next_hierarchy) + end + + merged + end + + def self.merge_values(first_child, second_child) + # When the first is an array, we need to go over every element to see if + # we can merge deeper. + if first_child.is_a?(Array) + first_child.map do |element| + if element.is_a?(Hash) && element.keys.any? { |k| second_child.keys.include?(k) } + element.deep_merge(second_child) { |key, first, second| merge_values(first, second) } + else + element + end + end + # If both of them are hashes, we can deep_merge with the same logic + elsif first_child.is_a?(Hash) && second_child.is_a?(Hash) + first_child.deep_merge(second_child) { |key, first, second| merge_values(first, second) } + # One of them is a root node, we just need to put them next to eachother in an array + else + Array.wrap(first_child) + Array.wrap(second_child) + end + end end diff --git a/app/serializers/base_serializer.rb b/app/serializers/base_serializer.rb index 4e6c15f673b..d78108480d3 100644 --- a/app/serializers/base_serializer.rb +++ b/app/serializers/base_serializer.rb @@ -1,5 +1,8 @@ class BaseSerializer + attr_reader :parameters + def initialize(parameters = {}) + @parameters = parameters @request = EntityRequest.new(parameters) end diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index fbf4a6783b9..ed84c3ae1d7 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -1,5 +1,42 @@ class GroupChildSerializer < BaseSerializer include WithPagination + attr_reader :hierarchy_root + entity GroupChildEntity + + def expand_hierarchy(hierarchy_root) + @hierarchy_root = hierarchy_root + self + end + + def represent(resource, opts = {}, entity_class = nil) + if hierarchy_root.present? + represent_hierarchies(resource, opts) + else + super(resource, opts) + end + end + + def represent_hierarchies(children, opts) + if children.is_a?(GroupHierarchy) + represent_hierarchy(children.hierarchy(hierarchy_root), opts) + else + children.map { |child| represent_hierarchy(child.hierarchy(hierarchy_root), opts) } + end + end + + def represent_hierarchy(hierarchy, opts) + serializer = self.class.new(parameters) + + result = if hierarchy.is_a?(Hash) + parent = hierarchy.keys.first + serializer.represent(parent, opts) + .merge(children: [serializer.represent_hierarchy(hierarchy[parent], opts)]) + else + serializer.represent(hierarchy, opts) + end + + result + end end diff --git a/spec/models/concerns/group_hierarchy_spec.rb b/spec/models/concerns/group_hierarchy_spec.rb index 14ac910c90d..1f4fab88781 100644 --- a/spec/models/concerns/group_hierarchy_spec.rb +++ b/spec/models/concerns/group_hierarchy_spec.rb @@ -29,6 +29,40 @@ describe GroupHierarchy, :nested_groups do expect(subsub_group.parent).to eq(subgroup) end end + + describe '#merge_hierarchy' do + it 'combines hierarchies' do + other_subgroup = create(:group, parent: parent) + + expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup] } + + expect(subsub_group.merge_hierarchy(other_subgroup)).to eq(expected_hierarchy) + end + end + + describe '.merge_hierarchies' do + it 'combines hierarchies until the top' do + other_subgroup = create(:group, parent: parent) + other_subsub_group = create(:group, parent: subgroup) + + groups = [other_subgroup, subsub_group, other_subsub_group] + + expected_hierarchy = { parent => [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] } + + expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) + end + + it 'combines upto a given parent' do + other_subgroup = create(:group, parent: parent) + other_subsub_group = create(:group, parent: subgroup) + + groups = [other_subgroup, subsub_group, other_subsub_group] + + expected_hierarchy = [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] + + expect(described_class.merge_hierarchies(groups, parent)).to eq(expected_hierarchy) + end + end end context 'for a project' do @@ -57,5 +91,39 @@ describe GroupHierarchy, :nested_groups do expect(project.parent).to eq(subsub_group) end end + + describe '#merge_hierarchy' do + it 'combines hierarchies' do + project = create(:project, namespace: parent) + + expected_hierarchy = { parent => [{ subgroup => subsub_group }, project] } + + expect(subsub_group.merge_hierarchy(project)).to eq(expected_hierarchy) + end + end + + describe '.merge_hierarchies' do + it 'combines hierarchies until the top' do + other_project = create(:project, namespace: parent) + other_subgroup_project = create(:project, namespace: subgroup) + + elements = [other_project, subsub_group, other_subgroup_project] + + expected_hierarchy = { parent => [other_project, { subgroup => [subsub_group, other_subgroup_project] }] } + + expect(described_class.merge_hierarchies(elements)).to eq(expected_hierarchy) + end + + it 'combines upto a given parent' do + other_project = create(:project, namespace: parent) + other_subgroup_project = create(:project, namespace: subgroup) + + elements = [other_project, subsub_group, other_subgroup_project] + + expected_hierarchy = [other_project, { subgroup => [subsub_group, other_subgroup_project] }] + + expect(described_class.merge_hierarchies(elements, parent)).to eq(expected_hierarchy) + end + end end end diff --git a/spec/serializers/group_child_serializer_spec.rb b/spec/serializers/group_child_serializer_spec.rb new file mode 100644 index 00000000000..967ed06d316 --- /dev/null +++ b/spec/serializers/group_child_serializer_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe GroupChildSerializer do + let(:request) { double('request') } + let(:user) { create(:user) } + subject(:serializer) { described_class.new(current_user: user) } + + describe '#represent' do + context 'for groups' do + it 'can render a single group' do + expect(serializer.represent(build(:group))).to be_kind_of(Hash) + end + + it 'can render a collection of groups' do + expect(serializer.represent(build_list(:group, 2))).to be_kind_of(Array) + end + end + + context 'with a hierarchy' do + let(:parent) { create(:group) } + + subject(:serializer) do + described_class.new(current_user: user).expand_hierarchy(parent) + end + + it 'expands the subgroups' do + subgroup = create(:group, parent: parent) + subsub_group = create(:group, parent: subgroup) + + json = serializer.represent(subsub_group) + subsub_group_json = json[:children].first + + expect(json[:id]).to eq(subgroup.id) + expect(subsub_group_json).not_to be_nil + expect(subsub_group_json[:id]).to eq(subsub_group.id) + end + + it 'can expand multiple trees' do + + end + end + end +end -- cgit v1.2.1 From 8f6dac4991ba7f5771a24175784f19dc1bbd4103 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 8 Sep 2017 19:22:33 +0200 Subject: Allow filtering children for a group When fetching children for a group with a filter, we will search all nested groups for results and render them in an expanded tree --- app/controllers/groups_controller.rb | 10 +++--- app/serializers/group_child_serializer.rb | 14 +++++--- spec/controllers/groups_controller_spec.rb | 30 ++++++++++++++++ spec/serializers/group_child_serializer_spec.rb | 47 +++++++++++++++++++++++-- 4 files changed, 89 insertions(+), 12 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index a714f2cc7b0..575d5476867 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -74,11 +74,11 @@ class GroupsController < Groups::ApplicationController respond_to do |format| format.json do - render json: GroupChildSerializer - .new(current_user: current_user) - .with_pagination(request, response) - .hierarchy_base(parent, open_hierarchy: filter[:filter].present) - .represent(@children) + 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 diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index ed84c3ae1d7..363f0c9b3d8 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -18,11 +18,14 @@ class GroupChildSerializer < BaseSerializer end end + protected + def represent_hierarchies(children, opts) if children.is_a?(GroupHierarchy) - represent_hierarchy(children.hierarchy(hierarchy_root), opts) + represent_hierarchy(children.hierarchy(hierarchy_root), opts).first else - children.map { |child| represent_hierarchy(child.hierarchy(hierarchy_root), opts) } + hierarchies = Array.wrap(GroupHierarchy.merge_hierarchies(children, hierarchy_root)) + hierarchies.map { |hierarchy| represent_hierarchy(hierarchy, opts) }.flatten end end @@ -30,9 +33,10 @@ class GroupChildSerializer < BaseSerializer serializer = self.class.new(parameters) result = if hierarchy.is_a?(Hash) - parent = hierarchy.keys.first - serializer.represent(parent, opts) - .merge(children: [serializer.represent_hierarchy(hierarchy[parent], opts)]) + hierarchy.map do |parent, children| + serializer.represent(parent, opts) + .merge(children: Array.wrap(serializer.represent_hierarchy(children, opts))) + end else serializer.represent(hierarchy, opts) end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 9b4654dc3e4..9cddb538a59 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -224,6 +224,36 @@ describe GroupsController do expect(assigns(:children)).to contain_exactly(public_subgroup, public_project) end end + + context 'filtering children' do + def get_filtered_list + get :children, id: group.to_param, filter: 'filter', format: :json + end + + it 'expands the tree for matching projects' do + project = create(:project, :public, namespace: public_subgroup, name: 'filterme') + + get_filtered_list + + 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_filtered_list + + 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 + end end end diff --git a/spec/serializers/group_child_serializer_spec.rb b/spec/serializers/group_child_serializer_spec.rb index 967ed06d316..7a87c1adc8f 100644 --- a/spec/serializers/group_child_serializer_spec.rb +++ b/spec/serializers/group_child_serializer_spec.rb @@ -16,7 +16,7 @@ describe GroupChildSerializer do end end - context 'with a hierarchy' do + context 'with a hierarchy', :nested_groups do let(:parent) { create(:group) } subject(:serializer) do @@ -35,8 +35,51 @@ describe GroupChildSerializer do expect(subsub_group_json[:id]).to eq(subsub_group.id) end - it 'can expand multiple trees' do + it 'can render a nested tree' do + subgroup1 = create(:group, parent: parent) + subsub_group1 = create(:group, parent: subgroup1) + subgroup2 = create(:group, parent: parent) + subsub_group2 = create(:group, parent: subgroup2) + json = serializer.represent([subsub_group1, subsub_group2]) + subgroup1_json = json.first + subsub_group1_json = subgroup1_json[:children].first + + expect(json.size).to eq(2) + expect(subgroup1_json[:id]).to eq(subgroup1.id) + expect(subsub_group1_json[:id]).to eq(subsub_group1.id) + end + end + + context 'for projects' do + it 'can render a single project' do + expect(serializer.represent(build(:project))).to be_kind_of(Hash) + end + + it 'can render a collection of projects' do + expect(serializer.represent(build_list(:project, 2))).to be_kind_of(Array) + end + + context 'with a hierarchy', :nested_groups do + let(:parent) { create(:group) } + + subject(:serializer) do + described_class.new(current_user: user).expand_hierarchy(parent) + end + + it 'can render a nested tree' do + subgroup1 = create(:group, parent: parent) + project1 = create(:project, namespace: subgroup1) + subgroup2 = create(:group, parent: parent) + project2 = create(:project, namespace: subgroup2) + + json = serializer.represent([project1, project2]) + project1_json, project2_json = json.map { |group_json| group_json[:children].first } + + expect(json.size).to eq(2) + expect(project1_json[:id]).to eq(project1.id) + expect(project2_json[:id]).to eq(project2.id) + end end end end -- cgit v1.2.1 From bb5187bb2a71f87b3ff49388f70dbcb27dfe4c6a Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Sun, 10 Sep 2017 17:20:27 +0200 Subject: Handle case where 2 matches in the same tree are found --- app/finders/group_children_finder.rb | 6 ++---- app/models/concerns/group_hierarchy.rb | 7 ++++++- spec/models/concerns/group_hierarchy_spec.rb | 6 ++++++ 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index eb0129947e2..bac3fb208d0 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -52,7 +52,7 @@ class GroupChildrenFinder end def subgroups_matching_filter - all_subgroups.search(params[:filter]).include(:parent) + all_subgroups.search(params[:filter]) end def subgroups @@ -64,7 +64,7 @@ class GroupChildrenFinder else base_groups end - groups = groups.includes(:route).includes(:children) + groups = groups groups.sort(params[:sort]) end @@ -75,7 +75,6 @@ class GroupChildrenFinder def projects_matching_filter ProjectsFinder.new(current_user: current_user).execute .search(params[:filter]) - .include(:namespace) .where(namespace: all_subgroups) end @@ -87,7 +86,6 @@ class GroupChildrenFinder else base_projects end - projects = projects.includes(:route) projects.sort(params[:sort]) end end diff --git a/app/models/concerns/group_hierarchy.rb b/app/models/concerns/group_hierarchy.rb index f7a62c9a607..9999bd87686 100644 --- a/app/models/concerns/group_hierarchy.rb +++ b/app/models/concerns/group_hierarchy.rb @@ -62,7 +62,12 @@ module GroupHierarchy # If both of them are hashes, we can deep_merge with the same logic elsif first_child.is_a?(Hash) && second_child.is_a?(Hash) first_child.deep_merge(second_child) { |key, first, second| merge_values(first, second) } - # One of them is a root node, we just need to put them next to eachother in an array + # If only one of them is a hash, we can check if the other child is already + # included, we don't need to do anything when it is. + elsif first_child.is_a?(Hash) && first_child.keys.include?(second_child) + first_child + elsif second_child.is_a?(Hash) && second_child.keys.include?(first_child) + second_child else Array.wrap(first_child) + Array.wrap(second_child) end diff --git a/spec/models/concerns/group_hierarchy_spec.rb b/spec/models/concerns/group_hierarchy_spec.rb index 1f4fab88781..bb02983c776 100644 --- a/spec/models/concerns/group_hierarchy_spec.rb +++ b/spec/models/concerns/group_hierarchy_spec.rb @@ -124,6 +124,12 @@ describe GroupHierarchy, :nested_groups do expect(described_class.merge_hierarchies(elements, parent)).to eq(expected_hierarchy) end + + it 'merges to elements in the same hierarchy' do + expected_hierarchy = { parent => subgroup } + + expect(described_class.merge_hierarchies([parent, subgroup])).to eq(expected_hierarchy) + end end end end -- cgit v1.2.1 From 79cc3c8e3e7938ea53459e34ca585ae66791199e Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Sun, 10 Sep 2017 18:43:15 +0200 Subject: Limit the amount of queries per row --- app/finders/group_children_finder.rb | 4 +- app/serializers/group_child_entity.rb | 8 ++- spec/controllers/groups_controller_spec.rb | 85 +++++++++++++++++++++++++++--- 3 files changed, 84 insertions(+), 13 deletions(-) diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index bac3fb208d0..07b97163c62 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -65,7 +65,7 @@ class GroupChildrenFinder base_groups end groups = groups - groups.sort(params[:sort]) + groups.sort(params[:sort]).includes(:route) end def base_projects @@ -86,6 +86,6 @@ class GroupChildrenFinder else base_projects end - projects.sort(params[:sort]) + projects.sort(params[:sort]).includes(:route, :namespace) end end diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index bc870541795..ab0d8a32312 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -32,11 +32,9 @@ class GroupChildEntity < Grape::Entity end def permission - if project? - object.project_members.find_by(user_id: request.current_user)&.human_access - else - object.group_members.find_by(user_id: request.current_user)&.human_access - end + return unless request&.current_user + + request.current_user.members.find_by(source: object)&.human_access end # Project only attributes diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 9cddb538a59..25778cc2b59 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -226,14 +226,10 @@ describe GroupsController do end context 'filtering children' do - def get_filtered_list - get :children, id: group.to_param, filter: 'filter', format: :json - end - it 'expands the tree for matching projects' do project = create(:project, :public, namespace: public_subgroup, name: 'filterme') - get_filtered_list + get :children, id: group.to_param, filter: 'filter', format: :json group_json = json_response.first project_json = group_json['children'].first @@ -245,7 +241,7 @@ describe GroupsController do it 'expands the tree for matching subgroups' do matched_group = create(:group, :public, parent: public_subgroup, name: 'filterme') - get_filtered_list + get :children, id: group.to_param, filter: 'filter', format: :json group_json = json_response.first matched_group_json = group_json['children'].first @@ -254,6 +250,83 @@ describe GroupsController do expect(matched_group_json['id']).to eq(matched_group.id) end end + + context 'queries per rendered element' 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 } + 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_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) + end + + 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) + end + + context 'when rendering hierarchies' do + def get_filtered_list + get :children, id: group.to_param, filter: 'filter', format: :json + end + + it 'queries the expected amount when nested rows are rendered for a group' 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) + + expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_group) + end + + it 'queries the expected amount when a new group match is added' 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) + end + + it 'queries the expected amount when nested rows are rendered for a project' do + matched_project = create(:project, :public, namespace: public_subgroup, name: 'filterme') + + control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count + + nested_group = create(:group, :public, parent: public_subgroup) + matched_project.update!(namespace: nested_group) + + expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_group) + end + + it 'queries the expected amount when a new project match is added' do + create(:project, :public, namespace: public_subgroup, name: 'filterme') + + control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count + + create(:project, :public, namespace: public_subgroup, name: 'filterme2') + + expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_project) + end + end + end end end -- cgit v1.2.1 From 6388b8feec28b0f0ca2f9e6d9c5c7b4e404fed2f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Sun, 10 Sep 2017 18:55:52 +0200 Subject: Don't include the parent in search results if it matches --- app/finders/group_children_finder.rb | 2 +- spec/finders/group_children_finder_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index 07b97163c62..9760f9ef802 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -52,7 +52,7 @@ class GroupChildrenFinder end def subgroups_matching_filter - all_subgroups.search(params[:filter]) + all_subgroups.where.not(id: parent_group).search(params[:filter]) end def subgroups diff --git a/spec/finders/group_children_finder_spec.rb b/spec/finders/group_children_finder_spec.rb index 8f83f88bb97..3df153abc6a 100644 --- a/spec/finders/group_children_finder_spec.rb +++ b/spec/finders/group_children_finder_spec.rb @@ -60,6 +60,12 @@ describe GroupChildrenFinder do expect(finder.execute).to contain_exactly(matching_project) end + + it 'does not include the parent itself' do + group.update!(name: 'test') + + expect(finder.execute).not_to include(group) + end end end end -- cgit v1.2.1 From 5998157618eafb69f6980f41639a7b485fe2467f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 12 Sep 2017 10:51:34 +0200 Subject: Include `can_leave` for a group --- app/serializers/group_child_entity.rb | 10 +++++++++- spec/serializers/group_child_entity_spec.rb | 6 ++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index ab0d8a32312..1940bc07eab 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -43,7 +43,7 @@ class GroupChildEntity < Grape::Entity # Group only attributes expose :children_count, :leave_path, :parent_id, :number_projects_with_delimiter, - :number_users_with_delimiter, :project_count, :subgroup_count, + :number_users_with_delimiter, :project_count, :subgroup_count, :can_leave, unless: lambda { |_instance, _options| project? } def children_finder @@ -66,6 +66,14 @@ class GroupChildEntity < Grape::Entity leave_group_group_members_path(object) end + def can_leave + if membership = object.members_and_requesters.find_by(user: request.current_user) + can?(request.current_user, :destroy_group_member, membership) + else + false + end + end + def number_projects_with_delimiter number_with_delimiter(project_count) end diff --git a/spec/serializers/group_child_entity_spec.rb b/spec/serializers/group_child_entity_spec.rb index 3a7d2205f53..9ddaf8f6469 100644 --- a/spec/serializers/group_child_entity_spec.rb +++ b/spec/serializers/group_child_entity_spec.rb @@ -83,6 +83,12 @@ describe GroupChildEntity do end end + it 'allows an owner to leave when there is another one' do + object.add_owner(create(:user)) + + expect(json[:can_leave]).to be_truthy + end + it_behaves_like 'group child json' end end -- cgit v1.2.1 From 960559aa6889a0b5ac1d46b84b7f7977a57d50ca Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 12 Sep 2017 12:47:47 +0200 Subject: Don't use canonical path for group children --- config/routes/group.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/config/routes/group.rb b/config/routes/group.rb index 0f0ece61a38..514f756a45f 100644 --- a/config/routes/group.rb +++ b/config/routes/group.rb @@ -41,9 +41,6 @@ scope(path: 'groups/*id', get :merge_requests, as: :merge_requests_group get :projects, as: :projects_group get :activity, as: :activity_group - scope(path: '-') do - get :children, as: :group_children - end get '/', action: :show, as: :group_canonical end @@ -56,5 +53,9 @@ constraints(GroupUrlConstrainer.new) do patch '/', action: :update put '/', action: :update delete '/', action: :destroy + + scope(path: '-') do + get :children + end end end -- cgit v1.2.1 From 3e6dd7d88daaac2dbafc234753942086e0ba0403 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 12 Sep 2017 14:58:44 +0200 Subject: Use same response-body in groups-dashboard as we do for group-home --- app/controllers/dashboard/groups_controller.rb | 9 +++---- app/serializers/group_child_serializer.rb | 5 ++-- .../dashboard/groups_controller_spec.rb | 29 ++++++++++++++++++++++ spec/serializers/group_child_serializer_spec.rb | 16 ++++++++++++ 4 files changed, 52 insertions(+), 7 deletions(-) create mode 100644 spec/controllers/dashboard/groups_controller_spec.rb diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb index 8057a0b455c..600ef890f39 100644 --- a/app/controllers/dashboard/groups_controller.rb +++ b/app/controllers/dashboard/groups_controller.rb @@ -15,7 +15,7 @@ class Dashboard::GroupsController < Dashboard::ApplicationController current_user.groups end - @groups = @groups.search(params[:filter_groups]) if params[:filter_groups].present? + @groups = @groups.search(params[:filter]) if params[:filter].present? @groups = @groups.includes(:route) @groups = @groups.sort(@sort) @groups = @groups.page(params[:page]) @@ -23,10 +23,9 @@ class Dashboard::GroupsController < Dashboard::ApplicationController respond_to do |format| format.html format.json do - render json: GroupSerializer - .new(current_user: @current_user) - .with_pagination(request, response) - .represent(@groups) + serializer = GroupChildSerializer.new(current_user: current_user) + .with_pagination(request, response) + render json: serializer.represent(@groups) end end end diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index 363f0c9b3d8..65dd1ced2a3 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -5,13 +5,14 @@ class GroupChildSerializer < BaseSerializer entity GroupChildEntity - def expand_hierarchy(hierarchy_root) + def expand_hierarchy(hierarchy_root = nil) @hierarchy_root = hierarchy_root + @expand_hierarchy = true self end def represent(resource, opts = {}, entity_class = nil) - if hierarchy_root.present? + if @expand_hierarchy represent_hierarchies(resource, opts) else super(resource, opts) diff --git a/spec/controllers/dashboard/groups_controller_spec.rb b/spec/controllers/dashboard/groups_controller_spec.rb new file mode 100644 index 00000000000..89a16c233d8 --- /dev/null +++ b/spec/controllers/dashboard/groups_controller_spec.rb @@ -0,0 +1,29 @@ +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 + + expect(json_response.first['id']).to eq(group.id) + end + + it 'filters groups' do + other_group = create(:group, name: 'filter') + other_group.add_owner(user) + + 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 + end +end diff --git a/spec/serializers/group_child_serializer_spec.rb b/spec/serializers/group_child_serializer_spec.rb index 7a87c1adc8f..e5896f54dd7 100644 --- a/spec/serializers/group_child_serializer_spec.rb +++ b/spec/serializers/group_child_serializer_spec.rb @@ -49,6 +49,22 @@ describe GroupChildSerializer do expect(subgroup1_json[:id]).to eq(subgroup1.id) expect(subsub_group1_json[:id]).to eq(subsub_group1.id) end + + context 'without a specified parent' do + subject(:serializer) do + described_class.new(current_user: user).expand_hierarchy + end + + it 'can render a tree' do + subgroup = create(:group, parent: parent) + + json = serializer.represent([subgroup]) + parent_json = json.first + + expect(parent_json[:id]).to eq(parent.id) + expect(parent_json[:children].first[:id]).to eq(subgroup.id) + end + end end context 'for projects' do -- cgit v1.2.1 From 3299a970e09ceca7ecabb3d78a5693f58ef79d79 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 12 Sep 2017 19:03:12 +0200 Subject: Handle all cases for merging a hierarchy The possible cases are: - [Array, Array] - [Array, Hash] - [Array, GroupHierarchy] - [Hash,Hash] - [Hash, GroupHierarchy] - [GroupHierarchy, GroupHierarchy] --- app/models/concerns/group_hierarchy.rb | 44 ++++++++++++++++++++-------- spec/models/concerns/group_hierarchy_spec.rb | 11 +++++++ 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/app/models/concerns/group_hierarchy.rb b/app/models/concerns/group_hierarchy.rb index 9999bd87686..9e02a17f4c3 100644 --- a/app/models/concerns/group_hierarchy.rb +++ b/app/models/concerns/group_hierarchy.rb @@ -33,7 +33,7 @@ module GroupHierarchy hierarchies = Array.wrap(hierarchies) return if hierarchies.empty? - unless hierarchies.all? { |other_base| other_base.is_a?(GroupHierarchy) } + unless hierarchies.all? { |hierarchy| hierarchy.is_a?(GroupHierarchy) } raise ArgumentError.new('element is not a hierarchy') end @@ -50,26 +50,46 @@ module GroupHierarchy def self.merge_values(first_child, second_child) # When the first is an array, we need to go over every element to see if - # we can merge deeper. - if first_child.is_a?(Array) - first_child.map do |element| - if element.is_a?(Hash) && element.keys.any? { |k| second_child.keys.include?(k) } - element.deep_merge(second_child) { |key, first, second| merge_values(first, second) } - else - element - end - end + # we can merge deeper. If no match is found, we add the element to the array + # + # Handled cases: + # [Array, Hash] + if first_child.is_a?(Array) && second_child.is_a?(Hash) + merge_hash_into_array(first_child, second_child) + elsif first_child.is_a?(Hash) && second_child.is_a?(Array) + merge_hash_into_array(second_child, first_child) # If both of them are hashes, we can deep_merge with the same logic + # + # Handled cases: + # [Hash, Hash] elsif first_child.is_a?(Hash) && second_child.is_a?(Hash) first_child.deep_merge(second_child) { |key, first, second| merge_values(first, second) } - # If only one of them is a hash, we can check if the other child is already - # included, we don't need to do anything when it is. + # If only one of them is a hash, and one of them is a GroupHierachy-object + # we can check if its already in the hash. If so, we don't need to do anything + # + # Handled cases + # [Hash, GroupHierarchy] elsif first_child.is_a?(Hash) && first_child.keys.include?(second_child) first_child elsif second_child.is_a?(Hash) && second_child.keys.include?(first_child) second_child + # If one or both elements are a GroupHierarchy, we wrap create an array + # combining them. + # + # Handled cases: + # [GroupHierarchy, Array], [GroupHierarchy, GroupHierarchy], [Array, Array] else Array.wrap(first_child) + Array.wrap(second_child) end end + + def self.merge_hash_into_array(array, new_hash) + if mergeable_index = array.index { |element| element.is_a?(Hash) && (element.keys & new_hash.keys).any? } + array[mergeable_index] = merge_values(array[mergeable_index], new_hash) + else + array << new_hash + end + + array + end end diff --git a/spec/models/concerns/group_hierarchy_spec.rb b/spec/models/concerns/group_hierarchy_spec.rb index bb02983c776..fe30895f15e 100644 --- a/spec/models/concerns/group_hierarchy_spec.rb +++ b/spec/models/concerns/group_hierarchy_spec.rb @@ -62,6 +62,17 @@ describe GroupHierarchy, :nested_groups do expect(described_class.merge_hierarchies(groups, parent)).to eq(expected_hierarchy) end + + it 'handles building a tree out of order' do + other_subgroup = create(:group, parent: parent) + other_subgroup2 = create(:group, parent: parent) + other_subsub_group = create(:group, parent: other_subgroup) + + groups = [subsub_group, other_subgroup2, other_subsub_group] + expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup2, { other_subgroup => other_subsub_group }] } + + expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) + end end end -- cgit v1.2.1 From 1fb49b8729b126360e9852b184c0ba63c330c4b5 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 13 Sep 2017 10:25:37 +0200 Subject: Only show root groups on the dashboard The children are lazy-loaded when expanding --- app/controllers/dashboard/groups_controller.rb | 16 +++------------- app/finders/group_children_finder.rb | 1 - 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb index 600ef890f39..d14427cd7a7 100644 --- a/app/controllers/dashboard/groups_controller.rb +++ b/app/controllers/dashboard/groups_controller.rb @@ -2,19 +2,9 @@ class Dashboard::GroupsController < Dashboard::ApplicationController def index @sort = params[:sort] || 'id_desc' - @groups = - if params[:parent_id] && Group.supports_nested_groups? - parent = Group.find_by(id: params[:parent_id]) - - if can?(current_user, :read_group, parent) - GroupsFinder.new(current_user, parent: parent).execute - else - Group.none - end - else - current_user.groups - end - + @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) diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index 9760f9ef802..6e2e2dfa5a1 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -64,7 +64,6 @@ class GroupChildrenFinder else base_groups end - groups = groups groups.sort(params[:sort]).includes(:route) end -- cgit v1.2.1 From 4c8942f9b8cffe6bf513c4766e8f4260c0550b61 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 13 Sep 2017 11:11:09 +0200 Subject: Replace `full_path`, `path` & `web_url` with a single `relative_path` --- app/serializers/group_child_entity.rb | 10 +++++++++- spec/serializers/group_child_entity_spec.rb | 6 ++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index 1940bc07eab..35f5a020793 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -2,7 +2,7 @@ class GroupChildEntity < Grape::Entity include ActionView::Helpers::NumberHelper include RequestAwareEntity - expose :id, :name, :path, :description, :visibility, :full_name, :full_path, :web_url, + expose :id, :name, :description, :visibility, :full_name, :relative_path, :created_at, :updated_at, :can_edit, :type, :avatar_url, :permission, :edit_path def project? @@ -31,6 +31,14 @@ class GroupChildEntity < Grape::Entity end end + def relative_path + if project? + project_path(object) + else + group_path(object) + end + end + def permission return unless request&.current_user diff --git a/spec/serializers/group_child_entity_spec.rb b/spec/serializers/group_child_entity_spec.rb index 9ddaf8f6469..54a1b1846e1 100644 --- a/spec/serializers/group_child_entity_spec.rb +++ b/spec/serializers/group_child_entity_spec.rb @@ -16,19 +16,17 @@ describe GroupChildEntity do end %w[id - path full_name - full_path avatar_url name description - web_url visibility type can_edit visibility edit_path - permission].each do |attribute| + permission + relative_path].each do |attribute| it "includes #{attribute}" do expect(json[attribute.to_sym]).to be_present end -- cgit v1.2.1 From 39df53ff0aeac24d390eea52a1df6dfe103a4c14 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 13 Sep 2017 11:29:32 +0200 Subject: Use the default sort set by the `Sortable` concern --- app/controllers/dashboard/groups_controller.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/dashboard/groups_controller.rb b/app/controllers/dashboard/groups_controller.rb index d14427cd7a7..6488e0b6c09 100644 --- a/app/controllers/dashboard/groups_controller.rb +++ b/app/controllers/dashboard/groups_controller.rb @@ -1,13 +1,11 @@ class Dashboard::GroupsController < Dashboard::ApplicationController def index - @sort = params[:sort] || 'id_desc' - @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) + @groups = @groups.sort(@sort = params[:sort]) @groups = @groups.page(params[:page]) respond_to do |format| -- cgit v1.2.1 From 3a4dc55f2924debcdbb37eb63d8ce57b1358df81 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 13 Sep 2017 17:16:30 +0200 Subject: Reuse the groups tree for explore and dashboard. --- app/controllers/concerns/group_tree.rb | 19 +++ app/controllers/dashboard/groups_controller.rb | 20 +-- app/controllers/explore/groups_controller.rb | 16 +-- spec/controllers/concerns/group_tree_spec.rb | 56 +++++++++ .../dashboard/groups_controller_spec.rb | 24 ++-- spec/controllers/explore/groups_controller_spec.rb | 24 ++++ spec/controllers/groups_controller_spec.rb | 139 ++++++++++++--------- spec/features/dashboard/groups_list_spec.rb | 4 + spec/features/explore/groups_list_spec.rb | 2 + 9 files changed, 200 insertions(+), 104 deletions(-) create mode 100644 app/controllers/concerns/group_tree.rb create mode 100644 spec/controllers/concerns/group_tree_spec.rb create mode 100644 spec/controllers/explore/groups_controller_spec.rb 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 - - expect(json_response.first['id']).to eq(group.id) - end + it 'renders group trees' do + expect(described_class).to include(GroupTree) + end - it 'filters groups' do - other_group = create(:group, name: 'filter') - other_group.add_owner(user) + 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) - get :index, filter: 'filt', format: :json - all_ids = json_response.map { |group_json| group_json['id'] } + get :index - 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) } - - it 'contains all subgroups' do - get :children, id: group.to_param, sort: 'id', 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', page: 2, format: :json - - expect(assigns(:children)).to contain_exactly(other_subgroup, project) - 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' } + 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 + 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' } + 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) + expect(response).not_to have_http_status(301) + end end - end - context 'when requesting a redirected path' do - let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + 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' } + it 'returns not found' do + post :update, id: redirect_route.path, group: { path: 'new_path' } - expect(response).to have_http_status(404) + expect(response).to have_http_status(404) + end 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 + 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 + 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 + it 'does not redirect to the correct casing' do + delete :destroy, id: group.to_param.upcase - expect(response).not_to have_http_status(301) + expect(response).not_to have_http_status(301) + end end - end - context 'when requesting a redirected path' do - let(:redirect_route) { group.redirect_routes.create(path: 'old-path') } + 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 + it 'returns not found' do + delete :destroy, id: redirect_route.path - expect(response).to have_http_status(404) + expect(response).to have_http_status(404) + end 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." + 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 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) -- cgit v1.2.1 From ea4e17e2aec525f249430b0a22dc6a8450648837 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 14 Sep 2017 09:01:29 +0200 Subject: Search subgroups on dashboard and explore views --- app/controllers/concerns/group_tree.rb | 11 +++++++--- spec/controllers/concerns/group_tree_spec.rb | 24 +++++++++++++++++++++- spec/controllers/explore/groups_controller_spec.rb | 1 - 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb index 6d5682ff769..d094216aa58 100644 --- a/app/controllers/concerns/group_tree.rb +++ b/app/controllers/concerns/group_tree.rb @@ -1,8 +1,12 @@ 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? + if params[:filter].present? + @groups = Gitlab::GroupHierarchy.new(groups).all_groups + @groups = @groups.search(params[:filter]) + else + # Only show root groups if no parent-id is given + @groups = groups.where(parent_id: params[:parent_id]) + end @groups = @groups.includes(:route) @groups = @groups.sort(@sort = params[:sort]) @groups = @groups.page(params[:page]) @@ -12,6 +16,7 @@ module GroupTree format.json do serializer = GroupChildSerializer.new(current_user: current_user) .with_pagination(request, response) + serializer.expand_hierarchy if params[:filter].present? render json: serializer.represent(@groups) end end diff --git a/spec/controllers/concerns/group_tree_spec.rb b/spec/controllers/concerns/group_tree_spec.rb index 19387f2d271..5d4fb66b492 100644 --- a/spec/controllers/concerns/group_tree_spec.rb +++ b/spec/controllers/concerns/group_tree_spec.rb @@ -5,7 +5,8 @@ describe GroupTree do let(:user) { create(:user) } controller(ApplicationController) do - include GroupTree # rubocop:disable Rspec/DescribedClass + # `described_class` is not available in this context + include GroupTree # rubocop:disable RSpec/DescribedClass def index render_group_tree Group.all @@ -43,6 +44,14 @@ describe GroupTree do expect(assigns(:groups)).to contain_exactly(subgroup) end + + it 'allows filtering for subgroups' do + subgroup = create(:group, :public, parent: group, name: 'filter') + + get :index, filter: 'filt', format: :json + + expect(assigns(:groups)).to contain_exactly(subgroup) + end end context 'json content' do @@ -51,6 +60,19 @@ describe GroupTree do expect(json_response.first['id']).to eq(group.id) end + + context 'nested groups', :nested_groups do + it 'expands the tree when filtering' do + subgroup = create(:group, :public, parent: group, name: 'filter') + + get :index, filter: 'filt', format: :json + + children_response = json_response.first['children'] + + expect(json_response.first['id']).to eq(group.id) + expect(children_response.first['id']).to eq(subgroup.id) + end + end end end end diff --git a/spec/controllers/explore/groups_controller_spec.rb b/spec/controllers/explore/groups_controller_spec.rb index 1923d054e95..9e0ad9ea86f 100644 --- a/spec/controllers/explore/groups_controller_spec.rb +++ b/spec/controllers/explore/groups_controller_spec.rb @@ -20,5 +20,4 @@ describe Explore::GroupsController do expect(assigns(:groups)).to contain_exactly(member_of_group, public_group) end - end -- cgit v1.2.1 From 20a08965bc949ea233cdde4e777698222fcabff2 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 14 Sep 2017 11:53:55 +0200 Subject: [WIP] improve number of queries when rendering a hierarchy --- app/finders/group_children_finder.rb | 6 +++--- spec/controllers/groups_controller_spec.rb | 31 ++++++++++++++++++------------ 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index 6e2e2dfa5a1..430373a92a2 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -38,7 +38,7 @@ class GroupChildrenFinder private def children - @children ||= subgroups + projects + @children ||= subgroups.with_route.includes(:route, :parent) + projects.with_route.includes(:route, :namespace) end def base_groups @@ -64,7 +64,7 @@ class GroupChildrenFinder else base_groups end - groups.sort(params[:sort]).includes(:route) + groups.sort(params[:sort]) end def base_projects @@ -85,6 +85,6 @@ class GroupChildrenFinder else base_projects end - projects.sort(params[:sort]).includes(:route, :namespace) + projects.sort(params[:sort]) end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 18f9d707e18..18791a01035 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -300,22 +300,28 @@ describe GroupsController do let(:expected_queries_per_group) { 5 } let(:expected_queries_per_project) { 0 } + before do + # Create the group before anything so it doesn't get tracked by the + # query recorder + group + end + def get_list get :children, id: group.to_param, format: :json end it 'queries the expected amount for a group row' do - control_count = ActiveRecord::QueryRecorder.new { get_list }.count + control = ActiveRecord::QueryRecorder.new { get_list } _new_group = create(:group, :public, parent: group) - expect { get_list }.not_to exceed_query_limit(control_count + expected_queries_per_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_count = ActiveRecord::QueryRecorder.new { get_list }.count + control = ActiveRecord::QueryRecorder.new { get_list } _new_project = create(:project, :public, namespace: group) - expect { get_list }.not_to exceed_query_limit(control_count + expected_queries_per_project) + expect { get_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_project) end context 'when rendering hierarchies' do @@ -326,41 +332,42 @@ describe GroupsController do it 'queries the expected amount when nested rows are rendered for a group' do matched_group = create(:group, :public, parent: public_subgroup, name: 'filterme') - control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count + control = ActiveRecord::QueryRecorder.new { get_filtered_list } nested_group = create(:group, :public, parent: public_subgroup) matched_group.update!(parent: nested_group) - expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_group) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group) end it 'queries the expected amount when a new group match is added' do create(:group, :public, parent: public_subgroup, name: 'filterme') - control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count + control = ActiveRecord::QueryRecorder.new { get_filtered_list } + create(:group, :public, parent: public_subgroup, name: 'filterme2') - expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_group) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group) end it 'queries the expected amount when nested rows are rendered for a project' do matched_project = create(:project, :public, namespace: public_subgroup, name: 'filterme') - control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count + control = ActiveRecord::QueryRecorder.new { get_filtered_list } nested_group = create(:group, :public, parent: public_subgroup) matched_project.update!(namespace: nested_group) - expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_group) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group) end it 'queries the expected amount when a new project match is added' do create(:project, :public, namespace: public_subgroup, name: 'filterme') - control_count = ActiveRecord::QueryRecorder.new { get_filtered_list }.count + control = ActiveRecord::QueryRecorder.new { get_filtered_list } create(:project, :public, namespace: public_subgroup, name: 'filterme2') - expect { get_filtered_list }.not_to exceed_query_limit(control_count + expected_queries_per_project) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_project) end end end -- cgit v1.2.1 From 9781ac552d4ae41983b2d95768e0fb06817e0ef9 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 15 Sep 2017 12:28:21 +0200 Subject: Include pagination when rendering expanded hierarchies --- app/serializers/concerns/with_pagination.rb | 6 ++++-- app/serializers/group_child_serializer.rb | 32 +++++++++++++++-------------- spec/controllers/groups_controller_spec.rb | 12 +++++++++++ 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/app/serializers/concerns/with_pagination.rb b/app/serializers/concerns/with_pagination.rb index 484c6855f7c..d29e22d6740 100644 --- a/app/serializers/concerns/with_pagination.rb +++ b/app/serializers/concerns/with_pagination.rb @@ -1,10 +1,12 @@ module WithPagination + attr_accessor :paginator + def with_pagination(request, response) - tap { @paginator = Gitlab::Serializer::Pagination.new(request, response) } + tap { self.paginator = Gitlab::Serializer::Pagination.new(request, response) } end def paginated? - @paginator.present? + paginator.present? end # super is `BaseSerializer#represent` here. diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index 65dd1ced2a3..fb47bf0b4eb 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -1,18 +1,20 @@ class GroupChildSerializer < BaseSerializer include WithPagination - attr_reader :hierarchy_root + attr_reader :hierarchy_root, :should_expand_hierarchy entity GroupChildEntity def expand_hierarchy(hierarchy_root = nil) - @hierarchy_root = hierarchy_root - @expand_hierarchy = true - self + tap do + @hierarchy_root = hierarchy_root + @should_expand_hierarchy = true + end end def represent(resource, opts = {}, entity_class = nil) - if @expand_hierarchy + if should_expand_hierarchy + paginator.paginate(resource) if paginated? represent_hierarchies(resource, opts) else super(resource, opts) @@ -33,15 +35,15 @@ class GroupChildSerializer < BaseSerializer def represent_hierarchy(hierarchy, opts) serializer = self.class.new(parameters) - result = if hierarchy.is_a?(Hash) - hierarchy.map do |parent, children| - serializer.represent(parent, opts) - .merge(children: Array.wrap(serializer.represent_hierarchy(children, opts))) - end - else - serializer.represent(hierarchy, opts) - end - - result + if hierarchy.is_a?(Hash) + hierarchy.map do |parent, children| + serializer.represent(parent, opts) + .merge(children: Array.wrap(serializer.represent_hierarchy(children, opts))) + end + elsif hierarchy.is_a?(Array) + hierarchy.map { |child| serializer.represent_hierarchy(child, opts) } + else + serializer.represent(hierarchy, opts) + end end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 18791a01035..21d5433a970 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -152,6 +152,10 @@ describe GroupsController do describe 'GET #show' do context 'pagination' do + before do + allow(Kaminari.config).to receive(:default_per_page).and_return(2) + end + 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 ) } @@ -288,6 +292,14 @@ describe GroupsController do expect(group_json['id']).to eq(public_subgroup.id) expect(matched_group_json['id']).to eq(matched_group.id) 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 -- cgit v1.2.1 From e3daa73fbfce6f178de512aeb165072f1ffde503 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 19 Sep 2017 11:05:31 +0200 Subject: Setup children in a a method and reuse for both calls --- app/controllers/groups_controller.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 575d5476867..d35364ffe1e 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -45,8 +45,7 @@ class GroupsController < Groups::ApplicationController end def show - @children = GroupChildrenFinder.new(current_user, parent_group: @group, params: params).execute - @children = @children.page(params[:page]) + setup_children(@group) respond_to do |format| format.html @@ -69,8 +68,7 @@ class GroupsController < Groups::ApplicationController render_404 end - @children = GroupChildrenFinder.new(current_user, parent_group: parent, params: params).execute - @children = @children.page(params[:page]) + setup_children(parent) respond_to do |format| format.json do @@ -119,6 +117,11 @@ class GroupsController < Groups::ApplicationController protected + def setup_children(parent) + @children = GroupChildrenFinder.new(current_user, parent_group: parent, params: params).execute + @children = @children.page(params[:page]) + end + def setup_projects set_non_archived_param params[:sort] ||= 'latest_activity_desc' -- cgit v1.2.1 From 31f775689396722e38de20157b46a75b1fe40582 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 19 Sep 2017 11:15:57 +0200 Subject: `current_user:` as a keyword argument --- app/controllers/groups_controller.rb | 4 +++- app/finders/group_children_finder.rb | 2 +- app/serializers/group_child_entity.rb | 3 ++- spec/finders/group_children_finder_spec.rb | 4 +++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index d35364ffe1e..1cec83a3bc6 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -118,7 +118,9 @@ class GroupsController < Groups::ApplicationController protected def setup_children(parent) - @children = GroupChildrenFinder.new(current_user, parent_group: parent, params: params).execute + @children = GroupChildrenFinder.new(current_user: current_user, + parent_group: parent, + params: params).execute @children = @children.page(params[:page]) end diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index 430373a92a2..93a218ee5b3 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -3,7 +3,7 @@ class GroupChildrenFinder attr_reader :current_user, :parent_group, :params - def initialize(current_user = nil, parent_group:, params: {}) + def initialize(current_user: nil, parent_group:, params: {}) @current_user = current_user @parent_group = parent_group @params = params diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index 35f5a020793..6c795da8f88 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -55,7 +55,8 @@ class GroupChildEntity < Grape::Entity unless: lambda { |_instance, _options| project? } def children_finder - @children_finder ||= GroupChildrenFinder.new(request.current_user, parent_group: object) + @children_finder ||= GroupChildrenFinder.new(current_user: request.current_user, + parent_group: object) end def children_count diff --git a/spec/finders/group_children_finder_spec.rb b/spec/finders/group_children_finder_spec.rb index 3df153abc6a..8257e158b06 100644 --- a/spec/finders/group_children_finder_spec.rb +++ b/spec/finders/group_children_finder_spec.rb @@ -4,7 +4,9 @@ describe GroupChildrenFinder do let(:user) { create(:user) } let(:group) { create(:group) } let(:params) { {} } - subject(:finder) { described_class.new(user, parent_group: group, params: params) } + subject(:finder) do + described_class.new(current_user: user, parent_group: group, params: params) + end before do group.add_owner(user) -- cgit v1.2.1 From fb7a0f8c335631e1cb8c8f91e135e49528fad70e Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 19 Sep 2017 11:46:07 +0200 Subject: More descriptive method names for projects & groups --- app/finders/group_children_finder.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb index 93a218ee5b3..7c7e7f804ec 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_children_finder.rb @@ -41,14 +41,14 @@ class GroupChildrenFinder @children ||= subgroups.with_route.includes(:route, :parent) + projects.with_route.includes(:route, :namespace) end - def base_groups + def direct_subgroups GroupsFinder.new(current_user, parent: parent_group, all_available: true).execute end def all_subgroups - Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).all_groups + Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).base_and_descendants end def subgroups_matching_filter @@ -62,12 +62,12 @@ class GroupChildrenFinder groups = if params[:filter] subgroups_matching_filter else - base_groups + direct_subgroups end groups.sort(params[:sort]) end - def base_projects + def direct_child_projects GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute end @@ -83,7 +83,7 @@ class GroupChildrenFinder projects = if params[:filter] projects_matching_filter else - base_projects + direct_child_projects end projects.sort(params[:sort]) end -- cgit v1.2.1 From 22aa034427b9392b44d9ecba0a51bb1b6c6616d7 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 19 Sep 2017 13:11:09 +0200 Subject: Rename `GroupHierarchy` to `GroupDescendant` --- app/controllers/groups_controller.rb | 6 +- app/finders/group_children_finder.rb | 90 -------------- app/finders/group_descendants_finder.rb | 92 ++++++++++++++ app/models/concerns/group_descendant.rb | 95 ++++++++++++++ app/models/concerns/group_hierarchy.rb | 95 -------------- app/models/group.rb | 2 +- app/models/project.rb | 6 +- app/serializers/group_child_entity.rb | 4 +- app/serializers/group_child_serializer.rb | 4 +- spec/finders/group_children_finder_spec.rb | 93 -------------- spec/finders/group_descendants_finder_spec.rb | 93 ++++++++++++++ spec/models/concerns/group_descendant_spec.rb | 173 ++++++++++++++++++++++++++ spec/models/concerns/group_hierarchy_spec.rb | 146 ---------------------- 13 files changed, 462 insertions(+), 437 deletions(-) delete mode 100644 app/finders/group_children_finder.rb create mode 100644 app/finders/group_descendants_finder.rb create mode 100644 app/models/concerns/group_descendant.rb delete mode 100644 app/models/concerns/group_hierarchy.rb delete mode 100644 spec/finders/group_children_finder_spec.rb create mode 100644 spec/finders/group_descendants_finder_spec.rb create mode 100644 spec/models/concerns/group_descendant_spec.rb delete mode 100644 spec/models/concerns/group_hierarchy_spec.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 1cec83a3bc6..b1deb29c61f 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -118,9 +118,9 @@ class GroupsController < Groups::ApplicationController protected def setup_children(parent) - @children = GroupChildrenFinder.new(current_user: current_user, - parent_group: parent, - params: params).execute + @children = GroupDescendantsFinder.new(current_user: current_user, + parent_group: parent, + params: params).execute @children = @children.page(params[:page]) end diff --git a/app/finders/group_children_finder.rb b/app/finders/group_children_finder.rb deleted file mode 100644 index 7c7e7f804ec..00000000000 --- a/app/finders/group_children_finder.rb +++ /dev/null @@ -1,90 +0,0 @@ -class GroupChildrenFinder - include Gitlab::Allowable - - attr_reader :current_user, :parent_group, :params - - def initialize(current_user: nil, parent_group:, params: {}) - @current_user = current_user - @parent_group = parent_group - @params = params - end - - def execute - Kaminari.paginate_array(children) - end - - # This allows us to fetch only the count without loading the objects. Unless - # the objects were already loaded. - def total_count - @total_count ||= subgroup_count + project_count - end - - def subgroup_count - @subgroup_count ||= if defined?(@children) - children.count { |child| child.is_a?(Group) } - else - subgroups.count - end - end - - def project_count - @project_count ||= if defined?(@children) - children.count { |child| child.is_a?(Project) } - else - projects.count - end - end - - private - - def children - @children ||= subgroups.with_route.includes(:route, :parent) + projects.with_route.includes(:route, :namespace) - end - - def direct_subgroups - GroupsFinder.new(current_user, - parent: parent_group, - all_available: true).execute - end - - def all_subgroups - Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).base_and_descendants - end - - def subgroups_matching_filter - all_subgroups.where.not(id: parent_group).search(params[:filter]) - end - - def subgroups - return Group.none unless Group.supports_nested_groups? - return Group.none unless can?(current_user, :read_group, parent_group) - - groups = if params[:filter] - subgroups_matching_filter - else - direct_subgroups - end - groups.sort(params[:sort]) - end - - def direct_child_projects - GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute - end - - def projects_matching_filter - ProjectsFinder.new(current_user: current_user).execute - .search(params[:filter]) - .where(namespace: all_subgroups) - end - - def projects - return Project.none unless can?(current_user, :read_group, parent_group) - - projects = if params[:filter] - projects_matching_filter - else - direct_child_projects - end - projects.sort(params[:sort]) - end -end diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb new file mode 100644 index 00000000000..f6b938fa732 --- /dev/null +++ b/app/finders/group_descendants_finder.rb @@ -0,0 +1,92 @@ +class GroupDescendantsFinder + include Gitlab::Allowable + + attr_reader :current_user, :parent_group, :params + + def initialize(current_user: nil, parent_group:, params: {}) + @current_user = current_user + @parent_group = parent_group + @params = params + end + + def execute + Kaminari.paginate_array(children) + end + + # This allows us to fetch only the count without loading the objects. Unless + # the objects were already loaded. + def total_count + @total_count ||= subgroup_count + project_count + end + + def subgroup_count + @subgroup_count ||= if defined?(@children) + children.count { |child| child.is_a?(Group) } + else + subgroups.count + end + end + + def project_count + @project_count ||= if defined?(@children) + children.count { |child| child.is_a?(Project) } + else + projects.count + end + end + + private + + def children + @children ||= subgroups.with_route.includes(:parent) + projects.with_route.includes(:namespace) + end + + def direct_child_groups + GroupsFinder.new(current_user, + parent: parent_group, + all_available: true).execute + end + + def all_descendant_groups + Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).base_and_descendants + end + + def subgroups_matching_filter + all_descendant_groups.where.not(id: parent_group).search(params[:filter]) + end + + def subgroups + return Group.none unless Group.supports_nested_groups? + return Group.none unless can?(current_user, :read_group, parent_group) + + # When filtering subgroups, we want to find all matches withing the tree of + # descendants to show to the user + groups = if params[:filter] + subgroups_matching_filter + else + direct_child_groups + end + groups.sort(params[:sort]) + end + + def direct_child_projects + GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute + end + + def projects_matching_filter + ProjectsFinder.new(current_user: current_user).execute + .search(params[:filter]) + .where(namespace: all_descendant_groups) + end + + def projects + return Project.none unless can?(current_user, :read_group, parent_group) + + projects = if params[:filter] + projects_matching_filter + else + direct_child_projects + end + projects.sort(params[:sort]) + end +end diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb new file mode 100644 index 00000000000..89a91f87a46 --- /dev/null +++ b/app/models/concerns/group_descendant.rb @@ -0,0 +1,95 @@ +module GroupDescendant + def hierarchy(hierarchy_base = nil) + expand_hierarchy_for_child(self, self, hierarchy_base) + end + + def parent + if self.is_a?(Project) + namespace + else + super + end + end + + def expand_hierarchy_for_child(child, hierarchy, hierarchy_base) + if child.parent.nil? && hierarchy_base.present? + raise ArgumentError.new('specified base is not part of the tree') + end + + if child.parent && child.parent != hierarchy_base + expand_hierarchy_for_child(child.parent, + { child.parent => hierarchy }, + hierarchy_base) + else + hierarchy + end + end + + def merge_hierarchy(other_element, hierarchy_base = nil) + GroupDescendant.merge_hierarchies([self, other_element], hierarchy_base) + end + + def self.merge_hierarchies(hierarchies, hierarchy_base = nil) + hierarchies = Array.wrap(hierarchies) + return if hierarchies.empty? + + unless hierarchies.all? { |hierarchy| hierarchy.is_a?(GroupDescendant) } + raise ArgumentError.new('element is not a hierarchy') + end + + first_hierarchy, *other_hierarchies = hierarchies + merged = first_hierarchy.hierarchy(hierarchy_base) + + other_hierarchies.each do |child| + next_hierarchy = child.hierarchy(hierarchy_base) + merged = merge_values(merged, next_hierarchy) + end + + merged + end + + def self.merge_values(first_child, second_child) + # When the first is an array, we need to go over every element to see if + # we can merge deeper. If no match is found, we add the element to the array + # + # Handled cases: + # [Array, Hash] + if first_child.is_a?(Array) && second_child.is_a?(Hash) + merge_hash_into_array(first_child, second_child) + elsif first_child.is_a?(Hash) && second_child.is_a?(Array) + merge_hash_into_array(second_child, first_child) + # If both of them are hashes, we can deep_merge with the same logic + # + # Handled cases: + # [Hash, Hash] + elsif first_child.is_a?(Hash) && second_child.is_a?(Hash) + first_child.deep_merge(second_child) { |key, first, second| merge_values(first, second) } + # If only one of them is a hash, and one of them is a GroupHierachy-object + # we can check if its already in the hash. If so, we don't need to do anything + # + # Handled cases + # [Hash, GroupDescendant] + elsif first_child.is_a?(Hash) && first_child.keys.include?(second_child) + first_child + elsif second_child.is_a?(Hash) && second_child.keys.include?(first_child) + second_child + # If one or both elements are a GroupDescendant, we wrap create an array + # combining them. + # + # Handled cases: + # [GroupDescendant, Array], [GroupDescendant, GroupDescendant], [Array, Array] + else + Array.wrap(first_child) + Array.wrap(second_child) + end + end + + def self.merge_hash_into_array(array, new_hash) + if mergeable_index = array.index { |element| element.is_a?(Hash) && (element.keys & new_hash.keys).any? } + array[mergeable_index] = merge_values(array[mergeable_index], new_hash) + else + array << new_hash + end + + array + end +end diff --git a/app/models/concerns/group_hierarchy.rb b/app/models/concerns/group_hierarchy.rb deleted file mode 100644 index 9e02a17f4c3..00000000000 --- a/app/models/concerns/group_hierarchy.rb +++ /dev/null @@ -1,95 +0,0 @@ -module GroupHierarchy - def hierarchy(hierarchy_base = nil) - tree_for_child(self, self, hierarchy_base) - end - - def parent - if self.is_a?(Project) - namespace - else - super - end - end - - def tree_for_child(child, tree, hierarchy_base) - if child.parent.nil? && hierarchy_base.present? - raise ArgumentError.new('specified base is not part of the tree') - end - - if child.parent && child.parent != hierarchy_base - tree_for_child(child.parent, - { child.parent => tree }, - hierarchy_base) - else - tree - end - end - - def merge_hierarchy(other_element, hierarchy_base = nil) - GroupHierarchy.merge_hierarchies([self, other_element], hierarchy_base) - end - - def self.merge_hierarchies(hierarchies, hierarchy_base = nil) - hierarchies = Array.wrap(hierarchies) - return if hierarchies.empty? - - unless hierarchies.all? { |hierarchy| hierarchy.is_a?(GroupHierarchy) } - raise ArgumentError.new('element is not a hierarchy') - end - - first_hierarchy, *other_hierarchies = hierarchies - merged = first_hierarchy.hierarchy(hierarchy_base) - - other_hierarchies.each do |child| - next_hierarchy = child.hierarchy(hierarchy_base) - merged = merge_values(merged, next_hierarchy) - end - - merged - end - - def self.merge_values(first_child, second_child) - # When the first is an array, we need to go over every element to see if - # we can merge deeper. If no match is found, we add the element to the array - # - # Handled cases: - # [Array, Hash] - if first_child.is_a?(Array) && second_child.is_a?(Hash) - merge_hash_into_array(first_child, second_child) - elsif first_child.is_a?(Hash) && second_child.is_a?(Array) - merge_hash_into_array(second_child, first_child) - # If both of them are hashes, we can deep_merge with the same logic - # - # Handled cases: - # [Hash, Hash] - elsif first_child.is_a?(Hash) && second_child.is_a?(Hash) - first_child.deep_merge(second_child) { |key, first, second| merge_values(first, second) } - # If only one of them is a hash, and one of them is a GroupHierachy-object - # we can check if its already in the hash. If so, we don't need to do anything - # - # Handled cases - # [Hash, GroupHierarchy] - elsif first_child.is_a?(Hash) && first_child.keys.include?(second_child) - first_child - elsif second_child.is_a?(Hash) && second_child.keys.include?(first_child) - second_child - # If one or both elements are a GroupHierarchy, we wrap create an array - # combining them. - # - # Handled cases: - # [GroupHierarchy, Array], [GroupHierarchy, GroupHierarchy], [Array, Array] - else - Array.wrap(first_child) + Array.wrap(second_child) - end - end - - def self.merge_hash_into_array(array, new_hash) - if mergeable_index = array.index { |element| element.is_a?(Hash) && (element.keys & new_hash.keys).any? } - array[mergeable_index] = merge_values(array[mergeable_index], new_hash) - else - array << new_hash - end - - array - end -end diff --git a/app/models/group.rb b/app/models/group.rb index 848e422e067..36439849086 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -6,7 +6,7 @@ class Group < Namespace include Avatarable include Referable include SelectForProjectAuthorization - include GroupHierarchy + include GroupDescendant has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :group_members diff --git a/app/models/project.rb b/app/models/project.rb index c221055f8b4..bf883de48ed 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -17,7 +17,7 @@ class Project < ActiveRecord::Base include ProjectFeaturesCompatibility include SelectForProjectAuthorization include Routable - include GroupHierarchy + include GroupDescendant extend Gitlab::ConfigHelper extend Gitlab::CurrentSettings @@ -1521,10 +1521,6 @@ class Project < ActiveRecord::Base map.public_path_for_source_path(path) end - def parent - namespace - end - def parent_changed? namespace_id_changed? end diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index 6c795da8f88..8a2624c3d32 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -55,8 +55,8 @@ class GroupChildEntity < Grape::Entity unless: lambda { |_instance, _options| project? } def children_finder - @children_finder ||= GroupChildrenFinder.new(current_user: request.current_user, - parent_group: object) + @children_finder ||= GroupDescendantsFinder.new(current_user: request.current_user, + parent_group: object) end def children_count diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index fb47bf0b4eb..1b6b2ad6e08 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -24,10 +24,10 @@ class GroupChildSerializer < BaseSerializer protected def represent_hierarchies(children, opts) - if children.is_a?(GroupHierarchy) + if children.is_a?(GroupDescendant) represent_hierarchy(children.hierarchy(hierarchy_root), opts).first else - hierarchies = Array.wrap(GroupHierarchy.merge_hierarchies(children, hierarchy_root)) + hierarchies = Array.wrap(GroupDescendant.merge_hierarchies(children, hierarchy_root)) hierarchies.map { |hierarchy| represent_hierarchy(hierarchy, opts) }.flatten end end diff --git a/spec/finders/group_children_finder_spec.rb b/spec/finders/group_children_finder_spec.rb deleted file mode 100644 index 8257e158b06..00000000000 --- a/spec/finders/group_children_finder_spec.rb +++ /dev/null @@ -1,93 +0,0 @@ -require 'spec_helper' - -describe GroupChildrenFinder do - let(:user) { create(:user) } - let(:group) { create(:group) } - let(:params) { {} } - subject(:finder) do - described_class.new(current_user: user, parent_group: group, params: params) - end - - before do - group.add_owner(user) - end - - describe '#execute' do - it 'includes projects' do - project = create(:project, namespace: group) - - expect(finder.execute).to contain_exactly(project) - end - - context 'with a filter' do - let(:params) { { filter: 'test' } } - - it 'includes only projects matching the filter' do - _other_project = create(:project, namespace: group) - matching_project = create(:project, namespace: group, name: 'testproject') - - expect(finder.execute).to contain_exactly(matching_project) - end - end - end - - context 'with nested groups', :nested_groups do - let!(:project) { create(:project, namespace: group) } - let!(:subgroup) { create(:group, parent: group) } - - describe '#execute' do - it 'contains projects and subgroups' do - expect(finder.execute).to contain_exactly(subgroup, project) - end - - context 'with a filter' do - let(:params) { { filter: 'test' } } - - it 'contains only matching projects and subgroups' do - matching_project = create(:project, namespace: group, name: 'Testproject') - matching_subgroup = create(:group, name: 'testgroup', parent: group) - - expect(finder.execute).to contain_exactly(matching_subgroup, matching_project) - end - - context 'with matching children' do - it 'includes a group that has a subgroup matching the query' do - matching_subgroup = create(:group, name: 'testgroup', parent: subgroup) - - expect(finder.execute).to contain_exactly(matching_subgroup) - end - - it 'includes a group that has a project matching the query' do - matching_project = create(:project, namespace: subgroup, name: 'Testproject') - - expect(finder.execute).to contain_exactly(matching_project) - end - - it 'does not include the parent itself' do - group.update!(name: 'test') - - expect(finder.execute).not_to include(group) - end - end - end - end - - describe '#total_count' do - it 'counts the array children were already loaded' do - finder.instance_variable_set(:@children, [build(:project)]) - - expect(finder).not_to receive(:subgroups) - expect(finder).not_to receive(:projects) - - expect(finder.total_count).to eq(1) - end - - it 'performs a count without loading children when they are not loaded yet' do - expect(finder).to receive(:subgroups).and_call_original - expect(finder).to receive(:projects).and_call_original - - expect(finder.total_count).to eq(2) - end - end - end -end diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb new file mode 100644 index 00000000000..c1268a486cf --- /dev/null +++ b/spec/finders/group_descendants_finder_spec.rb @@ -0,0 +1,93 @@ +require 'spec_helper' + +describe GroupDescendantsFinder do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:params) { {} } + subject(:finder) do + described_class.new(current_user: user, parent_group: group, params: params) + end + + before do + group.add_owner(user) + end + + describe '#execute' do + it 'includes projects' do + project = create(:project, namespace: group) + + expect(finder.execute).to contain_exactly(project) + end + + context 'with a filter' do + let(:params) { { filter: 'test' } } + + it 'includes only projects matching the filter' do + _other_project = create(:project, namespace: group) + matching_project = create(:project, namespace: group, name: 'testproject') + + expect(finder.execute).to contain_exactly(matching_project) + end + end + end + + context 'with nested groups', :nested_groups do + let!(:project) { create(:project, namespace: group) } + let!(:subgroup) { create(:group, parent: group) } + + describe '#execute' do + it 'contains projects and subgroups' do + expect(finder.execute).to contain_exactly(subgroup, project) + end + + context 'with a filter' do + let(:params) { { filter: 'test' } } + + it 'contains only matching projects and subgroups' do + matching_project = create(:project, namespace: group, name: 'Testproject') + matching_subgroup = create(:group, name: 'testgroup', parent: group) + + expect(finder.execute).to contain_exactly(matching_subgroup, matching_project) + end + + context 'with matching children' do + it 'includes a group that has a subgroup matching the query' do + matching_subgroup = create(:group, name: 'testgroup', parent: subgroup) + + expect(finder.execute).to contain_exactly(matching_subgroup) + end + + it 'includes a group that has a project matching the query' do + matching_project = create(:project, namespace: subgroup, name: 'Testproject') + + expect(finder.execute).to contain_exactly(matching_project) + end + + it 'does not include the parent itself' do + group.update!(name: 'test') + + expect(finder.execute).not_to include(group) + end + end + end + end + + describe '#total_count' do + it 'counts the array children were already loaded' do + finder.instance_variable_set(:@children, [build(:project)]) + + expect(finder).not_to receive(:subgroups) + expect(finder).not_to receive(:projects) + + expect(finder.total_count).to eq(1) + end + + it 'performs a count without loading children when they are not loaded yet' do + expect(finder).to receive(:subgroups).and_call_original + expect(finder).to receive(:projects).and_call_original + + expect(finder.total_count).to eq(2) + end + end + end +end diff --git a/spec/models/concerns/group_descendant_spec.rb b/spec/models/concerns/group_descendant_spec.rb new file mode 100644 index 00000000000..87eee515cde --- /dev/null +++ b/spec/models/concerns/group_descendant_spec.rb @@ -0,0 +1,173 @@ +require 'spec_helper' + +describe GroupDescendant, :nested_groups do + let(:parent) { create(:group) } + let(:subgroup) { create(:group, parent: parent) } + let(:subsub_group) { create(:group, parent: subgroup) } + + context 'for a group' do + describe '#hierarchy' do + it 'builds a hierarchy for a group' do + expected_hierarchy = { parent => { subgroup => subsub_group } } + + expect(subsub_group.hierarchy).to eq(expected_hierarchy) + end + + it 'builds a hierarchy upto a specified parent' do + expected_hierarchy = { subgroup => subsub_group } + + expect(subsub_group.hierarchy(parent)).to eq(expected_hierarchy) + end + + it 'raises an error if specifying a base that is not part of the tree' do + expect { subsub_group.hierarchy(double) }.to raise_error('specified base is not part of the tree') + end + end + + describe '#parent' do + it 'returns the correct parent' do + expect(subsub_group.parent).to eq(subgroup) + end + end + + describe '#merge_hierarchy' do + it 'combines hierarchies' do + other_subgroup = create(:group, parent: parent) + + expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup] } + + expect(subsub_group.merge_hierarchy(other_subgroup)).to eq(expected_hierarchy) + end + end + + describe '.merge_hierarchies' do + it 'combines hierarchies until the top' do + other_subgroup = create(:group, parent: parent) + other_subsub_group = create(:group, parent: subgroup) + + groups = [other_subgroup, subsub_group, other_subsub_group] + + expected_hierarchy = { parent => [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] } + + expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) + end + + it 'combines upto a given parent' do + other_subgroup = create(:group, parent: parent) + other_subsub_group = create(:group, parent: subgroup) + + groups = [other_subgroup, subsub_group, other_subsub_group] + + expected_hierarchy = [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] + + expect(described_class.merge_hierarchies(groups, parent)).to eq(expected_hierarchy) + end + + it 'handles building a tree out of order' do + other_subgroup = create(:group, parent: parent) + other_subgroup2 = create(:group, parent: parent) + other_subsub_group = create(:group, parent: other_subgroup) + + groups = [subsub_group, other_subgroup2, other_subsub_group] + expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup2, { other_subgroup => other_subsub_group }] } + + expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) + end + end + end + + context 'for a project' do + let(:project) { create(:project, namespace: subsub_group) } + + describe '#hierarchy' do + it 'builds a hierarchy for a group' do + expected_hierarchy = { parent => { subgroup => { subsub_group => project } } } + + expect(project.hierarchy).to eq(expected_hierarchy) + end + + it 'builds a hierarchy upto a specified parent' do + expected_hierarchy = { subsub_group => project } + + expect(project.hierarchy(subgroup)).to eq(expected_hierarchy) + end + + it 'raises an error if specifying a base that is not part of the tree' do + expect { project.hierarchy(double) }.to raise_error('specified base is not part of the tree') + end + end + + describe '#parent' do + it 'returns the correct parent' do + expect(project.parent).to eq(subsub_group) + end + end + + describe '#merge_hierarchy' do + it 'combines hierarchies' do + project = create(:project, namespace: parent) + + expected_hierarchy = { parent => [{ subgroup => subsub_group }, project] } + + expect(subsub_group.merge_hierarchy(project)).to eq(expected_hierarchy) + end + end + + describe '.merge_hierarchies' do + it 'combines hierarchies until the top' do + other_project = create(:project, namespace: parent) + other_subgroup_project = create(:project, namespace: subgroup) + + elements = [other_project, subsub_group, other_subgroup_project] + + expected_hierarchy = { parent => [other_project, { subgroup => [subsub_group, other_subgroup_project] }] } + + expect(described_class.merge_hierarchies(elements)).to eq(expected_hierarchy) + end + + it 'combines upto a given parent' do + other_project = create(:project, namespace: parent) + other_subgroup_project = create(:project, namespace: subgroup) + + elements = [other_project, subsub_group, other_subgroup_project] + + expected_hierarchy = [other_project, { subgroup => [subsub_group, other_subgroup_project] }] + + expect(described_class.merge_hierarchies(elements, parent)).to eq(expected_hierarchy) + end + + it 'merges to elements in the same hierarchy' do + expected_hierarchy = { parent => subgroup } + + expect(described_class.merge_hierarchies([parent, subgroup])).to eq(expected_hierarchy) + end + + it 'merges complex hierarchies' do + project = create(:project, namespace: parent) + sub_project = create(:project, namespace: subgroup) + subsubsub_group = create(:group, parent: subsub_group) + subsub_project = create(:project, namespace: subsub_group) + subsubsub_project = create(:project, namespace: subsubsub_group) + other_subgroup = create(:group, parent: parent) + other_subproject = create(:project, namespace: other_subgroup) + + projects = [project, subsubsub_project, sub_project, other_subproject, subsub_project] + + expected_hierarchy = [ + project, + { + subgroup => [ + { subsub_group => [{ subsubsub_group => subsubsub_project }, subsub_project] }, + sub_project + ] + }, + { other_subgroup => other_subproject } + ] + + actual_hierarchy = described_class.merge_hierarchies(projects, parent) + + expect(actual_hierarchy).to eq(expected_hierarchy) + end + end + end +end diff --git a/spec/models/concerns/group_hierarchy_spec.rb b/spec/models/concerns/group_hierarchy_spec.rb deleted file mode 100644 index fe30895f15e..00000000000 --- a/spec/models/concerns/group_hierarchy_spec.rb +++ /dev/null @@ -1,146 +0,0 @@ -require 'spec_helper' - -describe GroupHierarchy, :nested_groups do - let(:parent) { create(:group) } - let(:subgroup) { create(:group, parent: parent) } - let(:subsub_group) { create(:group, parent: subgroup) } - - context 'for a group' do - describe '#hierarchy' do - it 'builds a hierarchy for a group' do - expected_hierarchy = { parent => { subgroup => subsub_group } } - - expect(subsub_group.hierarchy).to eq(expected_hierarchy) - end - - it 'builds a hierarchy upto a specified parent' do - expected_hierarchy = { subgroup => subsub_group } - - expect(subsub_group.hierarchy(parent)).to eq(expected_hierarchy) - end - - it 'raises an error if specifying a base that is not part of the tree' do - expect { subsub_group.hierarchy(double) }.to raise_error('specified base is not part of the tree') - end - end - - describe '#parent' do - it 'returns the correct parent' do - expect(subsub_group.parent).to eq(subgroup) - end - end - - describe '#merge_hierarchy' do - it 'combines hierarchies' do - other_subgroup = create(:group, parent: parent) - - expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup] } - - expect(subsub_group.merge_hierarchy(other_subgroup)).to eq(expected_hierarchy) - end - end - - describe '.merge_hierarchies' do - it 'combines hierarchies until the top' do - other_subgroup = create(:group, parent: parent) - other_subsub_group = create(:group, parent: subgroup) - - groups = [other_subgroup, subsub_group, other_subsub_group] - - expected_hierarchy = { parent => [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] } - - expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) - end - - it 'combines upto a given parent' do - other_subgroup = create(:group, parent: parent) - other_subsub_group = create(:group, parent: subgroup) - - groups = [other_subgroup, subsub_group, other_subsub_group] - - expected_hierarchy = [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] - - expect(described_class.merge_hierarchies(groups, parent)).to eq(expected_hierarchy) - end - - it 'handles building a tree out of order' do - other_subgroup = create(:group, parent: parent) - other_subgroup2 = create(:group, parent: parent) - other_subsub_group = create(:group, parent: other_subgroup) - - groups = [subsub_group, other_subgroup2, other_subsub_group] - expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup2, { other_subgroup => other_subsub_group }] } - - expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) - end - end - end - - context 'for a project' do - let(:project) { create(:project, namespace: subsub_group) } - - describe '#hierarchy' do - it 'builds a hierarchy for a group' do - expected_hierarchy = { parent => { subgroup => { subsub_group => project } } } - - expect(project.hierarchy).to eq(expected_hierarchy) - end - - it 'builds a hierarchy upto a specified parent' do - expected_hierarchy = { subsub_group => project } - - expect(project.hierarchy(subgroup)).to eq(expected_hierarchy) - end - - it 'raises an error if specifying a base that is not part of the tree' do - expect { project.hierarchy(double) }.to raise_error('specified base is not part of the tree') - end - end - - describe '#parent' do - it 'returns the correct parent' do - expect(project.parent).to eq(subsub_group) - end - end - - describe '#merge_hierarchy' do - it 'combines hierarchies' do - project = create(:project, namespace: parent) - - expected_hierarchy = { parent => [{ subgroup => subsub_group }, project] } - - expect(subsub_group.merge_hierarchy(project)).to eq(expected_hierarchy) - end - end - - describe '.merge_hierarchies' do - it 'combines hierarchies until the top' do - other_project = create(:project, namespace: parent) - other_subgroup_project = create(:project, namespace: subgroup) - - elements = [other_project, subsub_group, other_subgroup_project] - - expected_hierarchy = { parent => [other_project, { subgroup => [subsub_group, other_subgroup_project] }] } - - expect(described_class.merge_hierarchies(elements)).to eq(expected_hierarchy) - end - - it 'combines upto a given parent' do - other_project = create(:project, namespace: parent) - other_subgroup_project = create(:project, namespace: subgroup) - - elements = [other_project, subsub_group, other_subgroup_project] - - expected_hierarchy = [other_project, { subgroup => [subsub_group, other_subgroup_project] }] - - expect(described_class.merge_hierarchies(elements, parent)).to eq(expected_hierarchy) - end - - it 'merges to elements in the same hierarchy' do - expected_hierarchy = { parent => subgroup } - - expect(described_class.merge_hierarchies([parent, subgroup])).to eq(expected_hierarchy) - end - end - end -end -- cgit v1.2.1 From 29df1ce84198801863fd1890b14099d13c6ec7fb Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 19 Sep 2017 17:24:57 +0200 Subject: Improve number of queries And document what extra queries are still being performed. --- app/finders/group_descendants_finder.rb | 2 +- spec/controllers/groups_controller_spec.rb | 50 +++++++++++++++--------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index f6b938fa732..3b891effd0c 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -38,7 +38,7 @@ class GroupDescendantsFinder private def children - @children ||= subgroups.with_route.includes(:parent) + projects.with_route.includes(:namespace) + @children ||= subgroups.with_route.includes(parent: [:route, :parent]) + projects.with_route.includes(namespace: [:route, :parent]) end def direct_child_groups diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 21d5433a970..befd346596f 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -304,26 +304,18 @@ describe GroupsController 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 - # 4. Every parent - # 5. The route for a parent - let(:expected_queries_per_group) { 5 } + # 1. Count of visible projects in the element + # 2. Count of visible subgroups in the element + let(:expected_queries_per_group) { 2 } let(:expected_queries_per_project) { 0 } - before do - # Create the group before anything so it doesn't get tracked by the - # query recorder - group - end - 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) @@ -337,18 +329,26 @@ describe GroupsController do end context 'when rendering hierarchies' do + # Extra queries per group when rendering a hierarchy: + # The route and the namespace are `included` for all matched elements + # But the parent's above those are not, so there's 2 extra queries per + # nested level: + # 1. Loading the parent that wasn't loaded yet + # 2. Loading the route for that parent. + let(:extra_queries_per_nested_level) { expected_queries_per_group + 2 } + def get_filtered_list get :children, id: group.to_param, filter: 'filter', format: :json end - it 'queries the expected amount when nested rows are rendered for a group' do - matched_group = create(:group, :public, parent: public_subgroup, name: 'filterme') + 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 } - nested_group = create(:group, :public, parent: public_subgroup) - matched_group.update!(parent: nested_group) - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group) + matched_group.update!(parent: public_subgroup) + + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) end it 'queries the expected amount when a new group match is added' do @@ -358,18 +358,17 @@ describe GroupsController do create(:group, :public, parent: public_subgroup, name: 'filterme2') - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) end - it 'queries the expected amount when nested rows are rendered for a project' do - matched_project = create(:project, :public, namespace: public_subgroup, name: 'filterme') + 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 } - nested_group = create(:group, :public, parent: public_subgroup) - matched_project.update!(namespace: nested_group) + matched_project.update!(namespace: public_subgroup) - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_group) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) end it 'queries the expected amount when a new project match is added' do @@ -377,9 +376,10 @@ describe GroupsController do control = ActiveRecord::QueryRecorder.new { get_filtered_list } - create(:project, :public, namespace: public_subgroup, name: 'filterme2') + nested_group = create(:group, :public, parent: group) + create(:project, :public, namespace: nested_group, name: 'filterme2') - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(expected_queries_per_project) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) end end end -- cgit v1.2.1 From e13753fcaa4901a840f6b33bf9e1a06185c3ba10 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 21 Sep 2017 09:20:37 +0200 Subject: Only take unarchived projects into account When finding children for a group --- app/finders/group_descendants_finder.rb | 4 ++-- spec/finders/group_descendants_finder_spec.rb | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 3b891effd0c..33fb1bf0359 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -6,7 +6,7 @@ class GroupDescendantsFinder def initialize(current_user: nil, parent_group:, params: {}) @current_user = current_user @parent_group = parent_group - @params = params + @params = params.reverse_merge(non_archived: true) end def execute @@ -74,7 +74,7 @@ class GroupDescendantsFinder end def projects_matching_filter - ProjectsFinder.new(current_user: current_user).execute + ProjectsFinder.new(current_user: current_user, params: params).execute .search(params[:filter]) .where(namespace: all_descendant_groups) end diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb index c1268a486cf..77401ba09a2 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -19,6 +19,12 @@ describe GroupDescendantsFinder do expect(finder.execute).to contain_exactly(project) end + it 'does not include archived projects' do + _archived_project = create(:project, :archived, namespace: group) + + expect(finder.execute).to be_empty + end + context 'with a filter' do let(:params) { { filter: 'test' } } -- cgit v1.2.1 From ee2744c60db2e7d8a7e35fbc62c6d236c5ba2d0a Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 22 Sep 2017 15:34:22 +0200 Subject: Don't wrap arrays twice: `children` are already wrapped in an array We do the wrapping in an array in represent_hierarchy for children. --- app/serializers/group_child_serializer.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index 1b6b2ad6e08..f2ec741d32c 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -27,8 +27,8 @@ class GroupChildSerializer < BaseSerializer if children.is_a?(GroupDescendant) represent_hierarchy(children.hierarchy(hierarchy_root), opts).first else - hierarchies = Array.wrap(GroupDescendant.merge_hierarchies(children, hierarchy_root)) - hierarchies.map { |hierarchy| represent_hierarchy(hierarchy, opts) }.flatten + hierarchies = GroupDescendant.merge_hierarchies(children, hierarchy_root) + represent_hierarchy(hierarchies, opts) end end -- cgit v1.2.1 From cd8e1b85b64ea9eb6a6a5b6de80e00ccc292c6ce Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 22 Sep 2017 16:40:55 +0200 Subject: Keep the `parent` method in `Project` --- app/models/concerns/group_descendant.rb | 8 -------- app/models/project.rb | 4 ++++ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 89a91f87a46..12c16a2cf5b 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -3,14 +3,6 @@ module GroupDescendant expand_hierarchy_for_child(self, self, hierarchy_base) end - def parent - if self.is_a?(Project) - namespace - else - super - end - end - def expand_hierarchy_for_child(child, hierarchy, hierarchy_base) if child.parent.nil? && hierarchy_base.present? raise ArgumentError.new('specified base is not part of the tree') diff --git a/app/models/project.rb b/app/models/project.rb index bf883de48ed..ad0b717308a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1521,6 +1521,10 @@ class Project < ActiveRecord::Base map.public_path_for_source_path(path) end + def parent + namespace + end + def parent_changed? namespace_id_changed? end -- cgit v1.2.1 From cd85c22faa7092edabf252fa157125ea571ed054 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 22 Sep 2017 17:36:39 +0200 Subject: Rename hierarchies to descendants where applicable --- app/models/concerns/group_descendant.rb | 40 +++++++++++++-------------- app/serializers/group_child_serializer.rb | 2 +- spec/models/concerns/group_descendant_spec.rb | 18 ++++++------ 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 12c16a2cf5b..528fcaa1917 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -1,46 +1,46 @@ module GroupDescendant - def hierarchy(hierarchy_base = nil) - expand_hierarchy_for_child(self, self, hierarchy_base) + def hierarchy(hierarchy_top = nil) + expand_hierarchy_for_child(self, self, hierarchy_top) end - def expand_hierarchy_for_child(child, hierarchy, hierarchy_base) - if child.parent.nil? && hierarchy_base.present? + def expand_hierarchy_for_child(child, hierarchy, hierarchy_top) + if child.parent.nil? && hierarchy_top.present? raise ArgumentError.new('specified base is not part of the tree') end - if child.parent && child.parent != hierarchy_base + if child.parent && child.parent != hierarchy_top expand_hierarchy_for_child(child.parent, { child.parent => hierarchy }, - hierarchy_base) + hierarchy_top) else hierarchy end end - def merge_hierarchy(other_element, hierarchy_base = nil) - GroupDescendant.merge_hierarchies([self, other_element], hierarchy_base) + def merge_hierarchy(other_element, hierarchy_top = nil) + GroupDescendant.build_hierarchy([self, other_element], hierarchy_top) end - def self.merge_hierarchies(hierarchies, hierarchy_base = nil) - hierarchies = Array.wrap(hierarchies) - return if hierarchies.empty? + def self.build_hierarchy(descendants, hierarchy_top = nil) + descendants = Array.wrap(descendants) + return if descendants.empty? - unless hierarchies.all? { |hierarchy| hierarchy.is_a?(GroupDescendant) } + unless descendants.all? { |hierarchy| hierarchy.is_a?(GroupDescendant) } raise ArgumentError.new('element is not a hierarchy') end - first_hierarchy, *other_hierarchies = hierarchies - merged = first_hierarchy.hierarchy(hierarchy_base) + first_descendant, *other_descendants = descendants + merged = first_descendant.hierarchy(hierarchy_top) - other_hierarchies.each do |child| - next_hierarchy = child.hierarchy(hierarchy_base) - merged = merge_values(merged, next_hierarchy) + other_descendants.each do |descendant| + next_descendant = descendant.hierarchy(hierarchy_top) + merged = merge_hash_tree(merged, next_descendant) end merged end - def self.merge_values(first_child, second_child) + def self.merge_hash_tree(first_child, second_child) # When the first is an array, we need to go over every element to see if # we can merge deeper. If no match is found, we add the element to the array # @@ -55,7 +55,7 @@ module GroupDescendant # Handled cases: # [Hash, Hash] elsif first_child.is_a?(Hash) && second_child.is_a?(Hash) - first_child.deep_merge(second_child) { |key, first, second| merge_values(first, second) } + first_child.deep_merge(second_child) { |key, first, second| merge_hash_tree(first, second) } # If only one of them is a hash, and one of them is a GroupHierachy-object # we can check if its already in the hash. If so, we don't need to do anything # @@ -77,7 +77,7 @@ module GroupDescendant def self.merge_hash_into_array(array, new_hash) if mergeable_index = array.index { |element| element.is_a?(Hash) && (element.keys & new_hash.keys).any? } - array[mergeable_index] = merge_values(array[mergeable_index], new_hash) + array[mergeable_index] = merge_hash_tree(array[mergeable_index], new_hash) else array << new_hash end diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index f2ec741d32c..ba81f99fff4 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -27,7 +27,7 @@ class GroupChildSerializer < BaseSerializer if children.is_a?(GroupDescendant) represent_hierarchy(children.hierarchy(hierarchy_root), opts).first else - hierarchies = GroupDescendant.merge_hierarchies(children, hierarchy_root) + hierarchies = GroupDescendant.build_hierarchy(children, hierarchy_root) represent_hierarchy(hierarchies, opts) end end diff --git a/spec/models/concerns/group_descendant_spec.rb b/spec/models/concerns/group_descendant_spec.rb index 87eee515cde..b1578fc593e 100644 --- a/spec/models/concerns/group_descendant_spec.rb +++ b/spec/models/concerns/group_descendant_spec.rb @@ -40,7 +40,7 @@ describe GroupDescendant, :nested_groups do end end - describe '.merge_hierarchies' do + describe '.build_hierarchy' do it 'combines hierarchies until the top' do other_subgroup = create(:group, parent: parent) other_subsub_group = create(:group, parent: subgroup) @@ -49,7 +49,7 @@ describe GroupDescendant, :nested_groups do expected_hierarchy = { parent => [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] } - expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) + expect(described_class.build_hierarchy(groups)).to eq(expected_hierarchy) end it 'combines upto a given parent' do @@ -60,7 +60,7 @@ describe GroupDescendant, :nested_groups do expected_hierarchy = [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] - expect(described_class.merge_hierarchies(groups, parent)).to eq(expected_hierarchy) + expect(described_class.build_hierarchy(groups, parent)).to eq(expected_hierarchy) end it 'handles building a tree out of order' do @@ -71,7 +71,7 @@ describe GroupDescendant, :nested_groups do groups = [subsub_group, other_subgroup2, other_subsub_group] expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup2, { other_subgroup => other_subsub_group }] } - expect(described_class.merge_hierarchies(groups)).to eq(expected_hierarchy) + expect(described_class.build_hierarchy(groups)).to eq(expected_hierarchy) end end end @@ -113,7 +113,7 @@ describe GroupDescendant, :nested_groups do end end - describe '.merge_hierarchies' do + describe '.build_hierarchy' do it 'combines hierarchies until the top' do other_project = create(:project, namespace: parent) other_subgroup_project = create(:project, namespace: subgroup) @@ -122,7 +122,7 @@ describe GroupDescendant, :nested_groups do expected_hierarchy = { parent => [other_project, { subgroup => [subsub_group, other_subgroup_project] }] } - expect(described_class.merge_hierarchies(elements)).to eq(expected_hierarchy) + expect(described_class.build_hierarchy(elements)).to eq(expected_hierarchy) end it 'combines upto a given parent' do @@ -133,13 +133,13 @@ describe GroupDescendant, :nested_groups do expected_hierarchy = [other_project, { subgroup => [subsub_group, other_subgroup_project] }] - expect(described_class.merge_hierarchies(elements, parent)).to eq(expected_hierarchy) + expect(described_class.build_hierarchy(elements, parent)).to eq(expected_hierarchy) end it 'merges to elements in the same hierarchy' do expected_hierarchy = { parent => subgroup } - expect(described_class.merge_hierarchies([parent, subgroup])).to eq(expected_hierarchy) + expect(described_class.build_hierarchy([parent, subgroup])).to eq(expected_hierarchy) end it 'merges complex hierarchies' do @@ -164,7 +164,7 @@ describe GroupDescendant, :nested_groups do { other_subgroup => other_subproject } ] - actual_hierarchy = described_class.merge_hierarchies(projects, parent) + actual_hierarchy = described_class.build_hierarchy(projects, parent) expect(actual_hierarchy).to eq(expected_hierarchy) end -- cgit v1.2.1 From ac0b104ae4968adaed7b94db76c0ac86badb6d6b Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 26 Sep 2017 11:22:52 +0200 Subject: Minimize the number of queries by preloading counts and ancestors By preloading the count of members, projects and subgroups of a group, we don't need to query them later. We also preload the entire hierarchy for a search result and include the counts so we don't need to query for them again --- app/finders/group_descendants_finder.rb | 63 +++++++++++++++++++++------ app/models/concerns/group_descendant.rb | 21 +++++---- app/models/project.rb | 4 ++ app/serializers/group_child_entity.rb | 21 +++++++-- spec/controllers/groups_controller_spec.rb | 34 +++++---------- spec/finders/group_descendants_finder_spec.rb | 38 +++++++--------- 6 files changed, 110 insertions(+), 71 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 33fb1bf0359..07178a026e8 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -13,12 +13,6 @@ class GroupDescendantsFinder Kaminari.paginate_array(children) end - # This allows us to fetch only the count without loading the objects. Unless - # the objects were already loaded. - def total_count - @total_count ||= subgroup_count + project_count - end - def subgroup_count @subgroup_count ||= if defined?(@children) children.count { |child| child.is_a?(Group) } @@ -38,7 +32,39 @@ class GroupDescendantsFinder private def children - @children ||= subgroups.with_route.includes(parent: [:route, :parent]) + projects.with_route.includes(namespace: [:route, :parent]) + return @children if @children + + projects_count = <<~PROJECTCOUNT + (SELECT COUNT(projects.id) AS preloaded_project_count + FROM projects WHERE projects.namespace_id = namespaces.id) + PROJECTCOUNT + subgroup_count = <<~SUBGROUPCOUNT + (SELECT COUNT(children.id) AS preloaded_subgroup_count + FROM namespaces children + WHERE children.parent_id = namespaces.id) + SUBGROUPCOUNT + member_count = <<~MEMBERCOUNT + (SELECT COUNT(members.user_id) AS preloaded_member_count + FROM members + WHERE members.source_type = 'Namespace' + AND members.source_id = namespaces.id + AND members.requested_at IS NULL) + MEMBERCOUNT + group_selects = [ + 'namespaces.*', + projects_count, + subgroup_count, + member_count + ] + + subgroups_with_counts = subgroups.with_route.select(group_selects) + + if params[:filter] + ancestors_for_project_search = ancestors_for_groups(Group.where(id: projects_matching_filter.select(:namespace_id))) + subgroups_with_counts = ancestors_for_project_search.with_route.select(group_selects) | subgroups_with_counts + end + + @children = subgroups_with_counts + projects.preload(:route) end def direct_child_groups @@ -48,11 +74,19 @@ class GroupDescendantsFinder end def all_descendant_groups - Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).base_and_descendants + Gitlab::GroupHierarchy.new(Group.where(id: parent_group)) + .base_and_descendants end def subgroups_matching_filter - all_descendant_groups.where.not(id: parent_group).search(params[:filter]) + all_descendant_groups + .where.not(id: parent_group) + .search(params[:filter]) + end + + def ancestors_for_groups(base_for_ancestors) + Gitlab::GroupHierarchy.new(base_for_ancestors) + .base_and_ancestors.where.not(id: parent_group) end def subgroups @@ -62,20 +96,23 @@ class GroupDescendantsFinder # When filtering subgroups, we want to find all matches withing the tree of # descendants to show to the user groups = if params[:filter] - subgroups_matching_filter + ancestors_for_groups(subgroups_matching_filter) else direct_child_groups end groups.sort(params[:sort]) end + def projects_for_user + Project.public_or_visible_to_user(current_user).non_archived + end + def direct_child_projects - GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute + projects_for_user.where(namespace: parent_group) end def projects_matching_filter - ProjectsFinder.new(current_user: current_user, params: params).execute - .search(params[:filter]) + projects_for_user.search(params[:filter]) .where(namespace: all_descendant_groups) end diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 528fcaa1917..f850f71d661 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -1,16 +1,19 @@ module GroupDescendant - def hierarchy(hierarchy_top = nil) - expand_hierarchy_for_child(self, self, hierarchy_top) + def hierarchy(hierarchy_top = nil, preloaded = []) + expand_hierarchy_for_child(self, self, hierarchy_top, preloaded) end - def expand_hierarchy_for_child(child, hierarchy, hierarchy_top) - if child.parent.nil? && hierarchy_top.present? + def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded = []) + parent = preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id } + parent ||= child.parent + + if parent.nil? && hierarchy_top.present? raise ArgumentError.new('specified base is not part of the tree') end - if child.parent && child.parent != hierarchy_top - expand_hierarchy_for_child(child.parent, - { child.parent => hierarchy }, + if parent && parent != hierarchy_top + expand_hierarchy_for_child(parent, + { parent => hierarchy }, hierarchy_top) else hierarchy @@ -30,10 +33,10 @@ module GroupDescendant end first_descendant, *other_descendants = descendants - merged = first_descendant.hierarchy(hierarchy_top) + merged = first_descendant.hierarchy(hierarchy_top, descendants) other_descendants.each do |descendant| - next_descendant = descendant.hierarchy(hierarchy_top) + next_descendant = descendant.hierarchy(hierarchy_top, descendants) merged = merge_hash_tree(merged, next_descendant) end diff --git a/app/models/project.rb b/app/models/project.rb index ad0b717308a..110f1ba04a4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1525,6 +1525,10 @@ class Project < ActiveRecord::Base namespace end + def parent_id + namespace_id + end + def parent_changed? namespace_id_changed? end diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index 8a2624c3d32..91e26272355 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -60,15 +60,23 @@ class GroupChildEntity < Grape::Entity end def children_count - @children_count ||= children_finder.total_count + @children_count ||= project_count + subgroup_count end def project_count - @project_count ||= children_finder.project_count + @project_count ||= if object.respond_to?(:preloaded_project_count) + object.preloaded_project_count + else + children_finder.project_count + end end def subgroup_count - @subgroup_count ||= children_finder.subgroup_count + @subgroup_count ||= if object.respond_to?(:preloaded_subgroup_count) + object.preloaded_subgroup_count + else + children_finder.subgroup_count + end end def leave_path @@ -88,6 +96,11 @@ class GroupChildEntity < Grape::Entity end def number_users_with_delimiter - number_with_delimiter(object.users.count) + member_count = if object.respond_to?(:preloaded_member_count) + object.preloaded_member_count + else + object.users.count + end + number_with_delimiter(member_count) end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index befd346596f..84207144036 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -303,10 +303,12 @@ describe GroupsController do end context 'queries per rendered element', :request_store do - # The expected extra queries for the rendered group are: + # 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 - let(:expected_queries_per_group) { 2 } + # 3. Count of members of a group + let(:expected_queries_per_group) { 0 } let(:expected_queries_per_project) { 0 } def get_list @@ -329,13 +331,9 @@ describe GroupsController do end context 'when rendering hierarchies' do - # Extra queries per group when rendering a hierarchy: - # The route and the namespace are `included` for all matched elements - # But the parent's above those are not, so there's 2 extra queries per - # nested level: - # 1. Loading the parent that wasn't loaded yet - # 2. Loading the route for that parent. - let(:extra_queries_per_nested_level) { expected_queries_per_group + 2 } + # 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 @@ -348,7 +346,7 @@ describe GroupsController do matched_group.update!(parent: public_subgroup) - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) + 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 @@ -357,8 +355,9 @@ describe GroupsController do 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_per_nested_level) + 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 @@ -368,18 +367,7 @@ describe GroupsController do matched_project.update!(namespace: public_subgroup) - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) - end - - it 'queries the expected amount when a new project match is added' do - create(:project, :public, namespace: public_subgroup, name: 'filterme') - - control = ActiveRecord::QueryRecorder.new { get_filtered_list } - - nested_group = create(:group, :public, parent: group) - create(:project, :public, namespace: nested_group, name: 'filterme2') - - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies) end end end diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb index 77401ba09a2..09a773aaf68 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -46,6 +46,18 @@ describe GroupDescendantsFinder do expect(finder.execute).to contain_exactly(subgroup, project) end + it 'includes the preloaded counts for groups' do + create(:group, parent: subgroup) + create(:project, namespace: subgroup) + subgroup.add_developer(create(:user)) + + found_group = finder.execute.detect { |child| child.is_a?(Group) } + + expect(found_group.preloaded_project_count).to eq(1) + expect(found_group.preloaded_subgroup_count).to eq(1) + expect(found_group.preloaded_member_count).to eq(1) + end + context 'with a filter' do let(:params) { { filter: 'test' } } @@ -57,16 +69,16 @@ describe GroupDescendantsFinder do end context 'with matching children' do - it 'includes a group that has a subgroup matching the query' do + it 'includes a group that has a subgroup matching the query and its parent' do matching_subgroup = create(:group, name: 'testgroup', parent: subgroup) - expect(finder.execute).to contain_exactly(matching_subgroup) + expect(finder.execute).to contain_exactly(subgroup, matching_subgroup) end - it 'includes a group that has a project matching the query' do + it 'includes the parent of a matching project' do matching_project = create(:project, namespace: subgroup, name: 'Testproject') - expect(finder.execute).to contain_exactly(matching_project) + expect(finder.execute).to contain_exactly(subgroup, matching_project) end it 'does not include the parent itself' do @@ -77,23 +89,5 @@ describe GroupDescendantsFinder do end end end - - describe '#total_count' do - it 'counts the array children were already loaded' do - finder.instance_variable_set(:@children, [build(:project)]) - - expect(finder).not_to receive(:subgroups) - expect(finder).not_to receive(:projects) - - expect(finder.total_count).to eq(1) - end - - it 'performs a count without loading children when they are not loaded yet' do - expect(finder).to receive(:subgroups).and_call_original - expect(finder).to receive(:projects).and_call_original - - expect(finder.total_count).to eq(2) - end - end end end -- cgit v1.2.1 From b92e7103fcced2d62000ed382848219016484f7b Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 26 Sep 2017 14:12:12 +0200 Subject: Fix nesting bug when rendering children of a shared subgroup --- app/finders/group_descendants_finder.rb | 2 +- app/serializers/group_child_serializer.rb | 2 +- spec/controllers/groups_controller_spec.rb | 26 ++++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 07178a026e8..4ed9c0ea39a 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -64,7 +64,7 @@ class GroupDescendantsFinder subgroups_with_counts = ancestors_for_project_search.with_route.select(group_selects) | subgroups_with_counts end - @children = subgroups_with_counts + projects.preload(:route) + @children = subgroups_with_counts + projects.with_route end def direct_child_groups diff --git a/app/serializers/group_child_serializer.rb b/app/serializers/group_child_serializer.rb index ba81f99fff4..6fa269ee5c0 100644 --- a/app/serializers/group_child_serializer.rb +++ b/app/serializers/group_child_serializer.rb @@ -41,7 +41,7 @@ class GroupChildSerializer < BaseSerializer .merge(children: Array.wrap(serializer.represent_hierarchy(children, opts))) end elsif hierarchy.is_a?(Array) - hierarchy.map { |child| serializer.represent_hierarchy(child, opts) } + hierarchy.map { |child| serializer.represent_hierarchy(child, opts) }.flatten else serializer.represent(hierarchy, opts) end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 84207144036..ff76eaee25f 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -293,6 +293,32 @@ describe GroupsController do 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 'includes pagination headers' do 2.times { |i| create(:group, :public, parent: public_subgroup, name: "filterme#{i}") } -- cgit v1.2.1 From 7a3ba8e9845b89c9f3f37d43e8edfeaa9093cfdf Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 26 Sep 2017 20:06:08 +0200 Subject: Make sure the user only sees groups he's allowed to see --- app/finders/group_descendants_finder.rb | 14 ++++++++++++-- spec/finders/group_descendants_finder_spec.rb | 28 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 4ed9c0ea39a..fca139062e5 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -73,13 +73,23 @@ class GroupDescendantsFinder all_available: true).execute end - def all_descendant_groups + def all_visible_descendant_groups + groups_table = Group.arel_table + visible_for_user = if current_user + groups_table[:id].in( + Arel::Nodes::SqlLiteral.new(GroupsFinder.new(current_user, all_available: true).execute.select(:id).to_sql) + ) + else + groups_table[:visibility_level].eq(Gitlab::VisibilityLevel::PUBLIC) + end + Gitlab::GroupHierarchy.new(Group.where(id: parent_group)) .base_and_descendants + .where(visible_for_user) end def subgroups_matching_filter - all_descendant_groups + all_visible_descendant_groups .where.not(id: parent_group) .search(params[:filter]) end diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb index 09a773aaf68..7b9dfcbfad0 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -58,6 +58,19 @@ describe GroupDescendantsFinder do expect(found_group.preloaded_member_count).to eq(1) end + it 'does not include subgroups the user does not have access to' do + subgroup.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + + public_subgroup = create(:group, :public, parent: group, path: 'public-group') + other_subgroup = create(:group, :private, parent: group, path: 'visible-private-group') + other_user = create(:user) + other_subgroup.add_developer(other_user) + + finder = described_class.new(current_user: other_user, parent_group: group) + + expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup) + end + context 'with a filter' do let(:params) { { filter: 'test' } } @@ -68,6 +81,21 @@ describe GroupDescendantsFinder do expect(finder.execute).to contain_exactly(matching_subgroup, matching_project) end + it 'does not include subgroups the user does not have access to' do + _invisible_subgroup = create(:group, :private, parent: group, name: 'test1') + other_subgroup = create(:group, :private, parent: group, name: 'test2') + public_subgroup = create(:group, :public, parent: group, name: 'test3') + other_subsubgroup = create(:group, :private, parent: other_subgroup, name: 'test4') + other_user = create(:user) + other_subgroup.add_developer(other_user) + + finder = described_class.new(current_user: other_user, + parent_group: group, + params: params) + + expect(finder.execute).to contain_exactly(other_subgroup, public_subgroup, other_subsubgroup) + end + context 'with matching children' do it 'includes a group that has a subgroup matching the query and its parent' do matching_subgroup = create(:group, name: 'testgroup', parent: subgroup) -- cgit v1.2.1 From ab5d5b7ecea5d68c00171d0750b1b2f62ffbe55d Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 26 Sep 2017 21:31:32 +0200 Subject: Make sure all queries are limited to the page size And fix some pagination bugs --- app/finders/group_descendants_finder.rb | 44 ++++++++++++++++++------------ spec/controllers/groups_controller_spec.rb | 6 ++-- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index fca139062e5..a39e6dedd1e 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -10,23 +10,20 @@ class GroupDescendantsFinder end def execute - Kaminari.paginate_array(children) + # The children array might be extended with the ancestors of projects when + # filtering. In that case, take the maximum so the aray does not get limited + # Otherwise, allow paginating through the search results + # + total_count = [children.size, subgroup_count + project_count].max + Kaminari.paginate_array(children, total_count: total_count) end def subgroup_count - @subgroup_count ||= if defined?(@children) - children.count { |child| child.is_a?(Group) } - else - subgroups.count - end + @subgroup_count ||= subgroups.count end def project_count - @project_count ||= if defined?(@children) - children.count { |child| child.is_a?(Project) } - else - projects.count - end + @project_count ||= projects.count end private @@ -57,14 +54,19 @@ class GroupDescendantsFinder member_count ] - subgroups_with_counts = subgroups.with_route.select(group_selects) + subgroups_with_counts = subgroups.with_route.page(params[:page]).per(per_page).select(group_selects) + group_page_count = subgroups_with_counts.total_pages + subgroup_page = subgroups_with_counts.current_page + + paginated_projects = projects.with_route.page(subgroup_page - group_page_count) + .per(per_page - subgroups_with_counts.size) if params[:filter] - ancestors_for_project_search = ancestors_for_groups(Group.where(id: projects_matching_filter.select(:namespace_id))) + ancestors_for_project_search = ancestors_for_groups(Group.where(id: paginated_projects.select(:namespace_id))) subgroups_with_counts = ancestors_for_project_search.with_route.select(group_selects) | subgroups_with_counts end - @children = subgroups_with_counts + projects.with_route + @children = subgroups_with_counts + paginated_projects end def direct_child_groups @@ -110,7 +112,7 @@ class GroupDescendantsFinder else direct_child_groups end - groups.sort(params[:sort]) + groups.order_by(sort) end def projects_for_user @@ -123,7 +125,7 @@ class GroupDescendantsFinder def projects_matching_filter projects_for_user.search(params[:filter]) - .where(namespace: all_descendant_groups) + .where(namespace: all_visible_descendant_groups) end def projects @@ -134,6 +136,14 @@ class GroupDescendantsFinder else direct_child_projects end - projects.sort(params[:sort]) + projects.order_by(sort) + end + + def sort + params.fetch(:sort, 'id_asc') + end + + def per_page + params.fetch(:per_page, Kaminari.config.default_per_page) end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index ff76eaee25f..575f70440e1 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -174,18 +174,18 @@ describe GroupsController do end context 'with subgroups and projects', :nested_groups do + let!(:first_page_subgroups) { create_list(:group, Kaminari.config.default_per_page, parent: group) } 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 + 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_desc', page: 2, format: :json + get :children, id: group.to_param, sort: 'id_asc', page: 2, format: :json expect(assigns(:children)).to contain_exactly(other_subgroup, project) end -- cgit v1.2.1 From af0b8e0558f529cd79a9dd061dc54ae3bfa9d1dd Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 2 Oct 2017 07:55:44 +0200 Subject: Only preload ancestors for search results in the specified parent When filtering we want all to preload all the ancestors upto the specified parent group. - root - subgroup - nested-group - project So when searching 'project', on the 'subgroup' page we want to preload 'nested-group' but not 'subgroup' or 'root' --- app/finders/group_descendants_finder.rb | 14 +++++++++++++- app/models/concerns/group_descendant.rb | 3 ++- spec/controllers/groups_controller_spec.rb | 10 ++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index a39e6dedd1e..d921e59d16f 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -96,9 +96,21 @@ class GroupDescendantsFinder .search(params[:filter]) end + # When filtering we want all to preload all the ancestors upto the specified + # parent group. + # + # - root + # - subgroup + # - nested-group + # - project + # + # So when searching 'project', on the 'subgroup' page we want to preload + # 'nested-group' but not 'subgroup' or 'root' def ancestors_for_groups(base_for_ancestors) + ancestors_for_parent = Gitlab::GroupHierarchy.new(Group.where(id: parent_group)) + .base_and_ancestors Gitlab::GroupHierarchy.new(base_for_ancestors) - .base_and_ancestors.where.not(id: parent_group) + .base_and_ancestors.where.not(id: ancestors_for_parent) end def subgroups diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index f850f71d661..7bc555e11a6 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -14,7 +14,8 @@ module GroupDescendant if parent && parent != hierarchy_top expand_hierarchy_for_child(parent, { parent => hierarchy }, - hierarchy_top) + hierarchy_top, + preloaded) else hierarchy end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 575f70440e1..d33251f2641 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -319,6 +319,16 @@ describe GroupsController do 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 'includes pagination headers' do 2.times { |i| create(:group, :public, parent: public_subgroup, name: "filterme#{i}") } -- cgit v1.2.1 From cb8e67c292260f9be3f24e81f76a5a172fbe6a2f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 2 Oct 2017 10:12:57 +0200 Subject: Improve count queries and move them to contants --- app/finders/group_descendants_finder.rb | 55 +++++++++++++++++---------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index d921e59d16f..534facd4179 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -3,6 +3,30 @@ class GroupDescendantsFinder attr_reader :current_user, :parent_group, :params + PROJECT_COUNT_SQL = <<~PROJECTCOUNT.freeze + (SELECT COUNT(*) AS preloaded_project_count + FROM projects + WHERE projects.namespace_id = namespaces.id + AND projects.archived IS NOT true) + PROJECTCOUNT + SUBGROUP_COUNT_SQL = <<~SUBGROUPCOUNT.freeze + (SELECT COUNT(*) AS preloaded_subgroup_count + FROM namespaces children + WHERE children.parent_id = namespaces.id) + SUBGROUPCOUNT + MEMBER_COUNT_SQL = <<~MEMBERCOUNT.freeze + (SELECT COUNT(*) AS preloaded_member_count + FROM members + WHERE members.source_type = 'Namespace' + AND members.source_id = namespaces.id + AND members.requested_at IS NULL) + MEMBERCOUNT + + GROUP_SELECTS = ['namespaces.*', + PROJECT_COUNT_SQL, + SUBGROUP_COUNT_SQL, + MEMBER_COUNT_SQL].freeze + def initialize(current_user: nil, parent_group:, params: {}) @current_user = current_user @parent_group = parent_group @@ -31,39 +55,16 @@ class GroupDescendantsFinder def children return @children if @children - projects_count = <<~PROJECTCOUNT - (SELECT COUNT(projects.id) AS preloaded_project_count - FROM projects WHERE projects.namespace_id = namespaces.id) - PROJECTCOUNT - subgroup_count = <<~SUBGROUPCOUNT - (SELECT COUNT(children.id) AS preloaded_subgroup_count - FROM namespaces children - WHERE children.parent_id = namespaces.id) - SUBGROUPCOUNT - member_count = <<~MEMBERCOUNT - (SELECT COUNT(members.user_id) AS preloaded_member_count - FROM members - WHERE members.source_type = 'Namespace' - AND members.source_id = namespaces.id - AND members.requested_at IS NULL) - MEMBERCOUNT - group_selects = [ - 'namespaces.*', - projects_count, - subgroup_count, - member_count - ] - - subgroups_with_counts = subgroups.with_route.page(params[:page]).per(per_page).select(group_selects) - group_page_count = subgroups_with_counts.total_pages - subgroup_page = subgroups_with_counts.current_page + subgroups_with_counts = subgroups.with_route + .page(params[:page]).per(per_page) + .select(GROUP_SELECTS) paginated_projects = projects.with_route.page(subgroup_page - group_page_count) .per(per_page - subgroups_with_counts.size) if params[:filter] ancestors_for_project_search = ancestors_for_groups(Group.where(id: paginated_projects.select(:namespace_id))) - subgroups_with_counts = ancestors_for_project_search.with_route.select(group_selects) | subgroups_with_counts + subgroups_with_counts = ancestors_for_project_search.with_route.select(GROUP_SELECTS) | subgroups_with_counts end @children = subgroups_with_counts + paginated_projects -- cgit v1.2.1 From 9870453eee8a638025bc3e12649d5918eb524518 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 2 Oct 2017 10:36:34 +0200 Subject: Stylize `GroupTree` concern --- app/controllers/concerns/group_tree.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb index d094216aa58..c5cf336463a 100644 --- a/app/controllers/concerns/group_tree.rb +++ b/app/controllers/concerns/group_tree.rb @@ -8,8 +8,8 @@ module GroupTree @groups = groups.where(parent_id: params[:parent_id]) end @groups = @groups.includes(:route) - @groups = @groups.sort(@sort = params[:sort]) - @groups = @groups.page(params[:page]) + .sort(@sort = params[:sort]) + .page(params[:page]) respond_to do |format| format.html -- cgit v1.2.1 From 8a685ca8562a288f0a14f1b5864b97e90fe8c709 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 2 Oct 2017 12:54:12 +0200 Subject: Fix bug with project pagination When projects were listed after groups, the projects that would also have been listed on the last page containing groups would be repeated. --- app/finders/group_descendants_finder.rb | 28 ++++++++++++++++++++++++++-- spec/controllers/groups_controller_spec.rb | 12 +++++++----- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 534facd4179..920dc4ab201 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -59,8 +59,7 @@ class GroupDescendantsFinder .page(params[:page]).per(per_page) .select(GROUP_SELECTS) - paginated_projects = projects.with_route.page(subgroup_page - group_page_count) - .per(per_page - subgroups_with_counts.size) + paginated_projects = paginate_projects_after_groups(subgroups_with_counts) if params[:filter] ancestors_for_project_search = ancestors_for_groups(Group.where(id: paginated_projects.select(:namespace_id))) @@ -70,6 +69,31 @@ class GroupDescendantsFinder @children = subgroups_with_counts + paginated_projects end + def paginate_projects_after_groups(loaded_subgroups) + # We adjust the pagination for projects for the combination with groups: + # - We limit the first page (page 0) where we show projects: + # Page size = 20: 17 groups, 3 projects + # - We ofset the page to start at 0 after the group pages: + # 3 pages of projects: + # - currently on page 3: Show page 0 (first page) limited to the number of + # projects that still fit the page (no offset) + # - currently on page 4: Show page 1 show all projects, offset by the number + # of projects shown on project-page 0. + group_page_count = loaded_subgroups.total_pages + subgroup_page = loaded_subgroups.current_page + group_last_page_count = subgroups.page(group_page_count).count + project_page = subgroup_page - group_page_count + offset = if project_page.zero? || group_page_count.zero? + 0 + else + per_page - group_last_page_count + end + + projects.with_route.page(project_page) + .per(per_page - loaded_subgroups.size) + .padding(offset) + end + def direct_child_groups GroupsFinder.new(current_user, parent: parent_group, diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index d33251f2641..00a6fa885bf 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -152,13 +152,15 @@ describe GroupsController do describe 'GET #show' do context 'pagination' do + let(:per_page) { 3 } + before do - allow(Kaminari.config).to receive(:default_per_page).and_return(2) + 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, Kaminari.config.default_per_page, :public, namespace: group ) } + let!(:first_page_projects) { create_list(:project, per_page, :public, namespace: group ) } it 'has projects on the first page' do get :show, id: group.to_param, sort: 'id_desc' @@ -174,9 +176,9 @@ describe GroupsController do end context 'with subgroups and projects', :nested_groups do - let!(:first_page_subgroups) { create_list(:group, Kaminari.config.default_per_page, parent: group) } + let!(:first_page_subgroups) { create_list(:group, per_page, :public, parent: group) } let!(:other_subgroup) { create(:group, :public, parent: group) } - let!(:project) { create(:project, :public, namespace: 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 @@ -187,7 +189,7 @@ describe GroupsController do 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, project) + expect(assigns(:children)).to contain_exactly(other_subgroup, *next_page_projects.take(per_page - 1)) end end end -- cgit v1.2.1 From 6c2de364dd15536d6f004e588e895ad3313533a8 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 2 Oct 2017 14:22:17 +0200 Subject: Split up adding ancestors for projects --- app/finders/group_descendants_finder.rb | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 920dc4ab201..bde93da3a3e 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -61,14 +61,19 @@ class GroupDescendantsFinder paginated_projects = paginate_projects_after_groups(subgroups_with_counts) - if params[:filter] - ancestors_for_project_search = ancestors_for_groups(Group.where(id: paginated_projects.select(:namespace_id))) - subgroups_with_counts = ancestors_for_project_search.with_route.select(GROUP_SELECTS) | subgroups_with_counts - end + subgroups_with_counts = add_project_ancestors_when_searching(subgroups_with_counts, paginated_projects) @children = subgroups_with_counts + paginated_projects end + def add_project_ancestors_when_searching(groups, projects) + return groups unless params[:filter] + + project_ancestors = ancestors_for_projects(projects) + .with_route.select(GROUP_SELECTS) + groups | project_ancestors + end + def paginate_projects_after_groups(loaded_subgroups) # We adjust the pagination for projects for the combination with groups: # - We limit the first page (page 0) where we show projects: @@ -138,6 +143,10 @@ class GroupDescendantsFinder .base_and_ancestors.where.not(id: ancestors_for_parent) end + def ancestors_for_projects(projects) + ancestors_for_groups(Group.where(id: projects.select(:namespace_id))) + end + def subgroups return Group.none unless Group.supports_nested_groups? return Group.none unless can?(current_user, :read_group, parent_group) -- cgit v1.2.1 From ef043063f9d6f9f9482707d78214709b09620a78 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 2 Oct 2017 14:23:36 +0200 Subject: Clean up public/private api of `GroupDescendant` So only methods that are used elsewhere are public. --- app/models/concerns/group_descendant.rb | 46 +++++++++++++-------------- spec/models/concerns/group_descendant_spec.rb | 32 ------------------- 2 files changed, 22 insertions(+), 56 deletions(-) diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 7bc555e11a6..7d8f051d2eb 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -3,28 +3,6 @@ module GroupDescendant expand_hierarchy_for_child(self, self, hierarchy_top, preloaded) end - def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded = []) - parent = preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id } - parent ||= child.parent - - if parent.nil? && hierarchy_top.present? - raise ArgumentError.new('specified base is not part of the tree') - end - - if parent && parent != hierarchy_top - expand_hierarchy_for_child(parent, - { parent => hierarchy }, - hierarchy_top, - preloaded) - else - hierarchy - end - end - - def merge_hierarchy(other_element, hierarchy_top = nil) - GroupDescendant.build_hierarchy([self, other_element], hierarchy_top) - end - def self.build_hierarchy(descendants, hierarchy_top = nil) descendants = Array.wrap(descendants) return if descendants.empty? @@ -44,7 +22,27 @@ module GroupDescendant merged end - def self.merge_hash_tree(first_child, second_child) + private + + def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded = []) + parent = preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id } + parent ||= child.parent + + if parent.nil? && hierarchy_top.present? + raise ArgumentError.new('specified base is not part of the tree') + end + + if parent && parent != hierarchy_top + expand_hierarchy_for_child(parent, + { parent => hierarchy }, + hierarchy_top, + preloaded) + else + hierarchy + end + end + + private_class_method def self.merge_hash_tree(first_child, second_child) # When the first is an array, we need to go over every element to see if # we can merge deeper. If no match is found, we add the element to the array # @@ -79,7 +77,7 @@ module GroupDescendant end end - def self.merge_hash_into_array(array, new_hash) + private_class_method def self.merge_hash_into_array(array, new_hash) if mergeable_index = array.index { |element| element.is_a?(Hash) && (element.keys & new_hash.keys).any? } array[mergeable_index] = merge_hash_tree(array[mergeable_index], new_hash) else diff --git a/spec/models/concerns/group_descendant_spec.rb b/spec/models/concerns/group_descendant_spec.rb index b1578fc593e..f3a0c342d35 100644 --- a/spec/models/concerns/group_descendant_spec.rb +++ b/spec/models/concerns/group_descendant_spec.rb @@ -24,22 +24,6 @@ describe GroupDescendant, :nested_groups do end end - describe '#parent' do - it 'returns the correct parent' do - expect(subsub_group.parent).to eq(subgroup) - end - end - - describe '#merge_hierarchy' do - it 'combines hierarchies' do - other_subgroup = create(:group, parent: parent) - - expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup] } - - expect(subsub_group.merge_hierarchy(other_subgroup)).to eq(expected_hierarchy) - end - end - describe '.build_hierarchy' do it 'combines hierarchies until the top' do other_subgroup = create(:group, parent: parent) @@ -97,22 +81,6 @@ describe GroupDescendant, :nested_groups do end end - describe '#parent' do - it 'returns the correct parent' do - expect(project.parent).to eq(subsub_group) - end - end - - describe '#merge_hierarchy' do - it 'combines hierarchies' do - project = create(:project, namespace: parent) - - expected_hierarchy = { parent => [{ subgroup => subsub_group }, project] } - - expect(subsub_group.merge_hierarchy(project)).to eq(expected_hierarchy) - end - end - describe '.build_hierarchy' do it 'combines hierarchies until the top' do other_project = create(:project, namespace: parent) -- cgit v1.2.1 From 167fd71348d145c6fee953004bf77ceebf6efb1e Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 2 Oct 2017 18:21:18 +0200 Subject: Always preload all elements in a hierarchy to avoid extra queries. --- app/models/concerns/group_descendant.rb | 15 ++++++++++++--- spec/models/concerns/group_descendant_spec.rb | 21 +++++++++++++++++++-- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 7d8f051d2eb..f37d23e615e 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -1,5 +1,6 @@ module GroupDescendant - def hierarchy(hierarchy_top = nil, preloaded = []) + def hierarchy(hierarchy_top = nil, preloaded = nil) + preloaded ||= ancestors_upto(hierarchy_top) expand_hierarchy_for_child(self, self, hierarchy_top, preloaded) end @@ -24,12 +25,20 @@ module GroupDescendant private - def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded = []) + def ancestors_upto(hierarchy_top = nil) + if hierarchy_top + Gitlab::GroupHierarchy.new(Group.where(id: hierarchy_top)).base_and_descendants + else + Gitlab::GroupHierarchy.new(Group.where(id: self)).all_groups + end + end + + def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded) parent = preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id } parent ||= child.parent if parent.nil? && hierarchy_top.present? - raise ArgumentError.new('specified base is not part of the tree') + raise ArgumentError.new('specified top is not part of the tree') end if parent && parent != hierarchy_top diff --git a/spec/models/concerns/group_descendant_spec.rb b/spec/models/concerns/group_descendant_spec.rb index f3a0c342d35..c17c8f2abec 100644 --- a/spec/models/concerns/group_descendant_spec.rb +++ b/spec/models/concerns/group_descendant_spec.rb @@ -7,6 +7,23 @@ describe GroupDescendant, :nested_groups do context 'for a group' do describe '#hierarchy' do + it 'only queries once for the ancestors' do + # make sure the subsub_group does not have anything cached + test_group = create(:group, parent: subsub_group).reload + + query_count = ActiveRecord::QueryRecorder.new { test_group.hierarchy }.count + + expect(query_count).to eq(1) + end + + it 'only queries once for the ancestors when a top is given' do + test_group = create(:group, parent: subsub_group).reload + + query_count = ActiveRecord::QueryRecorder.new { test_group.hierarchy(subgroup) }.count + + expect(query_count).to eq(1) + end + it 'builds a hierarchy for a group' do expected_hierarchy = { parent => { subgroup => subsub_group } } @@ -20,7 +37,7 @@ describe GroupDescendant, :nested_groups do end it 'raises an error if specifying a base that is not part of the tree' do - expect { subsub_group.hierarchy(double) }.to raise_error('specified base is not part of the tree') + expect { subsub_group.hierarchy(build_stubbed(:group)) }.to raise_error('specified top is not part of the tree') end end @@ -77,7 +94,7 @@ describe GroupDescendant, :nested_groups do end it 'raises an error if specifying a base that is not part of the tree' do - expect { project.hierarchy(double) }.to raise_error('specified base is not part of the tree') + expect { project.hierarchy(build_stubbed(:group)) }.to raise_error('specified top is not part of the tree') end end -- cgit v1.2.1 From 67815272dceb971c03bea3490ec26529b48a52b4 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 3 Oct 2017 15:32:32 +0200 Subject: Return an empty array when no matches are found --- app/models/concerns/group_descendant.rb | 2 +- spec/controllers/groups_controller_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index f37d23e615e..11f092db2ae 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -6,7 +6,7 @@ module GroupDescendant def self.build_hierarchy(descendants, hierarchy_top = nil) descendants = Array.wrap(descendants) - return if descendants.empty? + return [] if descendants.empty? unless descendants.all? { |hierarchy| hierarchy.is_a?(GroupDescendant) } raise ArgumentError.new('element is not a hierarchy') diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 00a6fa885bf..8582f31f059 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -331,6 +331,16 @@ describe GroupsController do 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}") } -- cgit v1.2.1 From de55396134e9e3de429c5c6df55ff06efb8ba329 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Wed, 4 Oct 2017 14:10:24 +0000 Subject: Groups tree enhancements for Groups Dashboard and Group Homepage --- app/assets/javascripts/filterable_list.js | 7 +- app/assets/javascripts/groups/components/app.vue | 191 +++++++++ .../javascripts/groups/components/group_folder.vue | 38 +- .../javascripts/groups/components/group_item.vue | 228 +++------- .../javascripts/groups/components/groups.vue | 23 +- .../javascripts/groups/components/item_actions.vue | 92 ++++ .../javascripts/groups/components/item_caret.vue | 25 ++ .../javascripts/groups/components/item_stats.vue | 98 +++++ .../groups/components/item_type_icon.vue | 34 ++ app/assets/javascripts/groups/constants.js | 35 ++ .../javascripts/groups/groups_filterable_list.js | 25 +- app/assets/javascripts/groups/index.js | 202 ++------- app/assets/javascripts/groups/new_group_child.js | 62 +++ .../javascripts/groups/service/groups_service.js | 38 ++ .../javascripts/groups/services/groups_service.js | 38 -- .../javascripts/groups/store/groups_store.js | 105 +++++ .../javascripts/groups/stores/groups_store.js | 167 -------- app/assets/stylesheets/framework/lists.scss | 89 +++- app/assets/stylesheets/pages/groups.scss | 115 ++++- app/helpers/sorting_helper.rb | 11 + app/views/dashboard/_groups_head.html.haml | 6 +- app/views/dashboard/groups/_empty_state.html.haml | 7 - app/views/dashboard/groups/_groups.html.haml | 9 +- app/views/dashboard/groups/index.html.haml | 4 +- app/views/explore/groups/_groups.html.haml | 6 +- app/views/explore/groups/index.html.haml | 9 +- app/views/groups/_children.html.haml | 9 +- app/views/groups/show.html.haml | 37 +- app/views/shared/groups/_dropdown.html.haml | 33 +- app/views/shared/groups/_empty_state.html.haml | 7 + app/views/shared/groups/_group.html.haml | 2 +- app/views/shared/groups/_list.html.haml | 2 +- app/views/shared/groups/_search_form.html.haml | 4 +- app/views/shared/projects/_dropdown.html.haml | 2 +- spec/features/dashboard/groups_list_spec.rb | 7 +- spec/features/explore/groups_list_spec.rb | 13 +- spec/javascripts/groups/components/app_spec.js | 440 +++++++++++++++++++ .../groups/components/group_folder_spec.js | 66 +++ .../groups/components/group_item_spec.js | 177 ++++++++ spec/javascripts/groups/components/groups_spec.js | 70 +++ .../groups/components/item_actions_spec.js | 110 +++++ .../groups/components/item_caret_spec.js | 40 ++ .../groups/components/item_stats_spec.js | 159 +++++++ .../groups/components/item_type_icon_spec.js | 54 +++ spec/javascripts/groups/group_item_spec.js | 102 ----- spec/javascripts/groups/groups_spec.js | 99 ----- spec/javascripts/groups/mock_data.js | 470 ++++++++++++++++----- .../groups/service/groups_service_spec.js | 41 ++ spec/javascripts/groups/store/groups_store_spec.js | 110 +++++ 49 files changed, 2790 insertions(+), 928 deletions(-) create mode 100644 app/assets/javascripts/groups/components/app.vue create mode 100644 app/assets/javascripts/groups/components/item_actions.vue create mode 100644 app/assets/javascripts/groups/components/item_caret.vue create mode 100644 app/assets/javascripts/groups/components/item_stats.vue create mode 100644 app/assets/javascripts/groups/components/item_type_icon.vue create mode 100644 app/assets/javascripts/groups/constants.js create mode 100644 app/assets/javascripts/groups/new_group_child.js create mode 100644 app/assets/javascripts/groups/service/groups_service.js delete mode 100644 app/assets/javascripts/groups/services/groups_service.js create mode 100644 app/assets/javascripts/groups/store/groups_store.js delete mode 100644 app/assets/javascripts/groups/stores/groups_store.js delete mode 100644 app/views/dashboard/groups/_empty_state.html.haml create mode 100644 app/views/shared/groups/_empty_state.html.haml create mode 100644 spec/javascripts/groups/components/app_spec.js create mode 100644 spec/javascripts/groups/components/group_folder_spec.js create mode 100644 spec/javascripts/groups/components/group_item_spec.js create mode 100644 spec/javascripts/groups/components/groups_spec.js create mode 100644 spec/javascripts/groups/components/item_actions_spec.js create mode 100644 spec/javascripts/groups/components/item_caret_spec.js create mode 100644 spec/javascripts/groups/components/item_stats_spec.js create mode 100644 spec/javascripts/groups/components/item_type_icon_spec.js delete mode 100644 spec/javascripts/groups/group_item_spec.js delete mode 100644 spec/javascripts/groups/groups_spec.js create mode 100644 spec/javascripts/groups/service/groups_service_spec.js create mode 100644 spec/javascripts/groups/store/groups_store_spec.js diff --git a/app/assets/javascripts/filterable_list.js b/app/assets/javascripts/filterable_list.js index 6d516a253bb..9e91f72b2ea 100644 --- a/app/assets/javascripts/filterable_list.js +++ b/app/assets/javascripts/filterable_list.js @@ -6,10 +6,11 @@ import _ from 'underscore'; */ export default class FilterableList { - constructor(form, filter, holder) { + constructor(form, filter, holder, filterInputField = 'filter_groups') { this.filterForm = form; this.listFilterElement = filter; this.listHolderElement = holder; + this.filterInputField = filterInputField; this.isBusy = false; } @@ -32,10 +33,10 @@ export default class FilterableList { onFilterInput() { const $form = $(this.filterForm); const queryData = {}; - const filterGroupsParam = $form.find('[name="filter_groups"]').val(); + const filterGroupsParam = $form.find(`[name="${this.filterInputField}"]`).val(); if (filterGroupsParam) { - queryData.filter_groups = filterGroupsParam; + queryData[this.filterInputField] = filterGroupsParam; } this.filterResults(queryData); diff --git a/app/assets/javascripts/groups/components/app.vue b/app/assets/javascripts/groups/components/app.vue new file mode 100644 index 00000000000..fdec34f5dab --- /dev/null +++ b/app/assets/javascripts/groups/components/app.vue @@ -0,0 +1,191 @@ + + + diff --git a/app/assets/javascripts/groups/components/group_folder.vue b/app/assets/javascripts/groups/components/group_folder.vue index 7cc6c4b0359..e60221fa08d 100644 --- a/app/assets/javascripts/groups/components/group_folder.vue +++ b/app/assets/javascripts/groups/components/group_folder.vue @@ -1,15 +1,27 @@ @@ -20,8 +32,20 @@ export default { v-for="(group, index) in groups" :key="index" :group="group" - :base-group="baseGroup" - :collection="groups" + :parent-group="parentGroup" /> +
  • + +