Merge branch 'tc-fix-group-finder-subgrouping' into 'master'

app/finders/groups_finder.rb

@@ -5,8 +5,10 @@ class GroupsFinder < UnionFinder
   end
 
   def execute
-    groups = find_union(all_groups, Group).with_route.order_id_desc
-    by_parent(groups)
+    items = all_groups.map do |item|
+      by_parent(item)
+    end
+    find_union(items, Group).with_route.order_id_desc
   end
 
   private
@@ -16,12 +18,22 @@ class GroupsFinder < UnionFinder
   def all_groups
     groups = []
 
-    groups << current_user.authorized_groups if current_user
+    if current_user
+      groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups
+    end
     groups << Group.unscoped.public_to_user(current_user)
 
     groups
   end
 
+  def groups_for_ancestors
+    current_user.authorized_groups
+  end
+
+  def groups_for_descendants
+    current_user.groups
+  end
+
   def by_parent(groups)
     return groups unless params[:parent]
 

lib/gitlab/group_hierarchy.rb

@@ -3,33 +3,38 @@ module Gitlab
   #
   # This class uses recursive CTEs and as a result will only work on PostgreSQL.
   class GroupHierarchy
-    attr_reader :base, :model
-
-    # base - An instance of ActiveRecord::Relation for which to get parent or
-    #        child groups.
-    def initialize(base)
-      @base = base
-      @model = base.model
+    attr_reader :ancestors_base, :descendants_base, :model
+
+    # ancestors_base - An instance of ActiveRecord::Relation for which to
+    #                  get parent groups.
+    # descendants_base - An instance of ActiveRecord::Relation for which to
+    #                    get child groups. If omitted, ancestors_base is used.
+    def initialize(ancestors_base, descendants_base = ancestors_base)
+      raise ArgumentError.new("Model of ancestors_base does not match model of descendants_base") if ancestors_base.model != descendants_base.model
+
+      @ancestors_base = ancestors_base
+      @descendants_base = descendants_base
+      @model = ancestors_base.model
     end
 
-    # Returns a relation that includes the base set of groups and all their
-    # ancestors (recursively).
+    # Returns a relation that includes the ancestors_base set of groups
+    # and all their ancestors (recursively).
     def base_and_ancestors
-      return model.none unless Group.supports_nested_groups?
+      return ancestors_base unless Group.supports_nested_groups?
 
       base_and_ancestors_cte.apply_to(model.all)
     end
 
-    # Returns a relation that includes the base set of groups and all their
-    # descendants (recursively).
+    # Returns a relation that includes the descendants_base set of groups
+    # and all their descendants (recursively).
     def base_and_descendants
-      return model.none unless Group.supports_nested_groups?
+      return descendants_base unless Group.supports_nested_groups?
 
       base_and_descendants_cte.apply_to(model.all)
     end
 
-    # Returns a relation that includes the base groups, their ancestors, and the
-    # descendants of the base groups.
+    # Returns a relation that includes the base groups, their ancestors,
+    # and the descendants of the base groups.
     #
     # The resulting query will roughly look like the following:
     #
@@ -48,8 +53,10 @@ module Gitlab
     #
     # Using this approach allows us to further add criteria to the relation with
     # Rails thinking it's selecting data the usual way.
+    #
+    # If nested groups are not supported, ancestors_base is returned.
     def all_groups
-      return base unless Group.supports_nested_groups?
+      return ancestors_base unless Group.supports_nested_groups?
 
       ancestors = base_and_ancestors_cte
       descendants = base_and_descendants_cte
@@ -72,7 +79,7 @@ module Gitlab
     def base_and_ancestors_cte
       cte = SQL::RecursiveCTE.new(:base_and_ancestors)
 
-      cte << base.except(:order)
+      cte << ancestors_base.except(:order)
 
       # Recursively get all the ancestors of the base set.
       cte << model.
@@ -86,7 +93,7 @@ module Gitlab
     def base_and_descendants_cte
       cte = SQL::RecursiveCTE.new(:base_and_descendants)
 
-      cte << base.except(:order)
+      cte << descendants_base.except(:order)
 
       # Recursively get all the descendants of the base set.
       cte << model.

spec/controllers/groups_controller_spec.rb

@@ -2,7 +2,7 @@ require 'rails_helper'
 
 describe GroupsController do
   let(:user) { create(:user) }
-  let(:group) { create(:group) }
+  let(:group) { create(:group, :public) }
   let(:project) { create(:empty_project, namespace: group) }
   let!(:group_member) { create(:group_member, group: group, user: user) }
 
@@ -35,14 +35,15 @@ describe GroupsController do
         sign_in(user)
       end
 
-      it 'shows the public subgroups' do
+      it 'shows all subgroups' do
         get :subgroups, id: group.to_param
 
-        expect(assigns(:nested_groups)).to contain_exactly(public_subgroup)
+        expect(assigns(:nested_groups)).to contain_exactly(public_subgroup, private_subgroup)
       end
 
-      context 'being member' do
+      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)
 
           get :subgroups, id: group.to_param

spec/finders/groups_finder_spec.rb

@@ -38,28 +38,79 @@ describe GroupsFinder do
       end
     end
 
-    context 'subgroups' do
+    context 'subgroups', :nested_groups do
       let!(:parent_group) { create(:group, :public) }
       let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
       let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) }
       let!(:private_subgroup) { create(:group, :private, parent: parent_group) }
 
       context 'without a user' do
-        it 'only returns public subgroups' do
-          expect(described_class.new(nil, parent: parent_group).execute).to contain_exactly(public_subgroup)
+        it 'only returns parent and public subgroups' do
+          expect(described_class.new(nil).execute).to contain_exactly(parent_group, public_subgroup)
         end
       end
 
       context 'with a user' do
-        it 'returns public and internal subgroups' do
-          expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup)
+        subject { described_class.new(user).execute }
+
+        it 'returns parent, public, and internal subgroups' do
+          is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup)
         end
 
         context 'being member' do
-          it 'returns public subgroups, internal subgroups, and private subgroups user is member of' do
+          it 'returns parent, public subgroups, internal subgroups, and private subgroups user is member of' do
             private_subgroup.add_guest(user)
 
-            expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup, private_subgroup)
+            is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup, private_subgroup)
+          end
+        end
+
+        context 'parent group private' do
+          before do
+            parent_group.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
+          end
+
+          context 'being member of parent group' do
+            it 'returns all subgroups' do
+              parent_group.add_guest(user)
+
+              is_expected.to contain_exactly(parent_group, public_subgroup, internal_subgroup, private_subgroup)
+            end
+          end
+
+          context 'authorized to private project' do
+            context 'project one level deep' do
+              let!(:subproject) { create(:empty_project, :private, namespace: private_subgroup) }
+              before do
+                subproject.add_guest(user)
+              end
+
+              it 'includes the subgroup of the project' do
+                is_expected.to include(private_subgroup)
+              end
+
+              it 'does not include private subgroups deeper down' do
+                subsubgroup = create(:group, :private, parent: private_subgroup)
+
+                is_expected.not_to include(subsubgroup)
+              end
+            end
+
+            context 'project two levels deep' do
+              let!(:private_subsubgroup) { create(:group, :private, parent: private_subgroup) }
+              let!(:subsubproject) { create(:empty_project, :private, namespace: private_subsubgroup) }
+              before do
+                subsubproject.add_guest(user)
+              end
+
+              it 'returns all the ancestor groups' do
+                is_expected.to include(private_subsubgroup, private_subgroup, parent_group)
+              end
+
+              it 'returns the groups for a given parent' do
+                expect(described_class.new(user, parent: parent_group).execute).to include(private_subgroup)
+              end
+            end
           end
         end
       end

spec/lib/gitlab/group_hierarchy_spec.rb

@@ -17,6 +17,12 @@ describe Gitlab::GroupHierarchy, :postgresql do
     it 'includes all of the ancestors' do
       expect(relation).to include(parent, child1)
     end
+
+    it 'uses ancestors_base #initialize argument' do
+      relation = described_class.new(Group.where(id: child2.id), Group.none).base_and_ancestors
+
+      expect(relation).to include(parent, child1, child2)
+    end
   end
 
   describe '#base_and_descendants' do
@@ -31,6 +37,12 @@ describe Gitlab::GroupHierarchy, :postgresql do
     it 'includes all the descendants' do
       expect(relation).to include(child1, child2)
     end
+
+    it 'uses descendants_base #initialize argument' do
+      relation = described_class.new(Group.none, Group.where(id: parent.id)).base_and_descendants
+
+      expect(relation).to include(parent, child1, child2)
+    end
   end
 
   describe '#all_groups' do
@@ -49,5 +61,17 @@ describe Gitlab::GroupHierarchy, :postgresql do
     it 'includes the descendants' do
       expect(relation).to include(child2)
     end
+
+    it 'uses ancestors_base #initialize argument for ancestors' do
+      relation = described_class.new(Group.where(id: child1.id), Group.where(id: Group.maximum(:id).succ)).all_groups
+
+      expect(relation).to include(parent)
+    end
+
+    it 'uses descendants_base #initialize argument for descendants' do
+      relation = described_class.new(Group.where(id: Group.maximum(:id).succ), Group.where(id: child1.id)).all_groups
+
+      expect(relation).to include(child2)
+    end
   end
 end