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_descendants_finder.rb similarity index 82% rename from app/finders/group_children_finder.rb rename to app/finders/group_descendants_finder.rb index 7c7e7f804ec..f6b938fa732 100644 --- a/app/finders/group_children_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -1,4 +1,4 @@ -class GroupChildrenFinder +class GroupDescendantsFinder include Gitlab::Allowable attr_reader :current_user, :parent_group, :params @@ -38,31 +38,33 @@ class GroupChildrenFinder private def children - @children ||= subgroups.with_route.includes(:route, :parent) + projects.with_route.includes(:route, :namespace) + @children ||= subgroups.with_route.includes(:parent) + projects.with_route.includes(:namespace) end - def direct_subgroups + def direct_child_groups GroupsFinder.new(current_user, parent: parent_group, all_available: true).execute end - def all_subgroups + def all_descendant_groups 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]) + 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_subgroups + direct_child_groups end groups.sort(params[:sort]) end @@ -74,7 +76,7 @@ class GroupChildrenFinder def projects_matching_filter ProjectsFinder.new(current_user: current_user).execute .search(params[:filter]) - .where(namespace: all_subgroups) + .where(namespace: all_descendant_groups) end def projects diff --git a/app/models/concerns/group_hierarchy.rb b/app/models/concerns/group_descendant.rb similarity index 78% rename from app/models/concerns/group_hierarchy.rb rename to app/models/concerns/group_descendant.rb index 9e02a17f4c3..89a91f87a46 100644 --- a/app/models/concerns/group_hierarchy.rb +++ b/app/models/concerns/group_descendant.rb @@ -1,6 +1,6 @@ -module GroupHierarchy +module GroupDescendant def hierarchy(hierarchy_base = nil) - tree_for_child(self, self, hierarchy_base) + expand_hierarchy_for_child(self, self, hierarchy_base) end def parent @@ -11,29 +11,29 @@ module GroupHierarchy end end - def tree_for_child(child, tree, hierarchy_base) + 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 - tree_for_child(child.parent, - { child.parent => tree }, - hierarchy_base) + expand_hierarchy_for_child(child.parent, + { child.parent => hierarchy }, + hierarchy_base) else - tree + hierarchy end end def merge_hierarchy(other_element, hierarchy_base = nil) - GroupHierarchy.merge_hierarchies([self, other_element], hierarchy_base) + 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?(GroupHierarchy) } + unless hierarchies.all? { |hierarchy| hierarchy.is_a?(GroupDescendant) } raise ArgumentError.new('element is not a hierarchy') end @@ -68,16 +68,16 @@ module GroupHierarchy # we can check if its already in the hash. If so, we don't need to do anything # # Handled cases - # [Hash, GroupHierarchy] + # [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 GroupHierarchy, we wrap create an array + # If one or both elements are a GroupDescendant, we wrap create an array # combining them. # # Handled cases: - # [GroupHierarchy, Array], [GroupHierarchy, GroupHierarchy], [Array, Array] + # [GroupDescendant, Array], [GroupDescendant, GroupDescendant], [Array, Array] else Array.wrap(first_child) + Array.wrap(second_child) 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_descendants_finder_spec.rb similarity index 98% rename from spec/finders/group_children_finder_spec.rb rename to spec/finders/group_descendants_finder_spec.rb index 8257e158b06..c1268a486cf 100644 --- a/spec/finders/group_children_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe GroupChildrenFinder do +describe GroupDescendantsFinder do let(:user) { create(:user) } let(:group) { create(:group) } let(:params) { {} } diff --git a/spec/models/concerns/group_hierarchy_spec.rb b/spec/models/concerns/group_descendant_spec.rb similarity index 82% rename from spec/models/concerns/group_hierarchy_spec.rb rename to spec/models/concerns/group_descendant_spec.rb index fe30895f15e..87eee515cde 100644 --- a/spec/models/concerns/group_hierarchy_spec.rb +++ b/spec/models/concerns/group_descendant_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe GroupHierarchy, :nested_groups do +describe GroupDescendant, :nested_groups do let(:parent) { create(:group) } let(:subgroup) { create(:group, parent: parent) } let(:subsub_group) { create(:group, parent: subgroup) } @@ -141,6 +141,33 @@ describe GroupHierarchy, :nested_groups do 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