Merge branch 'dz-nested-groups-access-improvements' into 'master'

Nested groups feature improvemetns

See merge request !8448

app/helpers/groups_helper.rb

@@ -14,7 +14,7 @@ module GroupsHelper
   def group_title(group, name = nil, url = nil)
     full_title = ''
 
-    group.parents.each do |parent|
+    group.ancestors.each do |parent|
       full_title += link_to(simple_sanitize(parent.name), group_path(parent))
       full_title += ' / '.html_safe
     end

app/models/concerns/routable.rb

@@ -60,6 +60,21 @@ module Routable
         joins(:route).where(wheres.join(' OR '))
       end
     end
+
+    # Builds a relation to find multiple objects that are nested under user membership
+    #
+    # Usage:
+    #
+    #     Klass.member_descendants(1)
+    #
+    # Returns an ActiveRecord::Relation.
+    def member_descendants(user_id)
+      joins(:route).
+        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%')
+               INNER JOIN members ON members.source_id = r2.source_id
+               AND members.source_type = r2.source_type").
+        where('members.user_id = ?', user_id)
+    end
   end
 
   private

app/models/group.rb

@@ -201,7 +201,7 @@ class Group < Namespace
   end
 
   def members_with_parents
-    GroupMember.where(requested_at: nil, source_id: parents.map(&:id).push(id))
+    GroupMember.where(requested_at: nil, source_id: ancestors.map(&:id).push(id))
   end
 
   def users_with_parents

app/models/namespace.rb

@@ -185,8 +185,26 @@ class Namespace < ActiveRecord::Base
       end
   end
 
-  def parents
-    @parents ||= parent ? parent.parents + [parent] : []
+  # Scopes the model on ancestors of the record
+  def ancestors
+    if parent_id
+      path = route.path
+      paths = []
+
+      until path.blank?
+        path = path.rpartition('/').first
+        paths << path
+      end
+
+      self.class.joins(:route).where('routes.path IN (?)', paths).reorder('routes.path ASC')
+    else
+      self.class.none
+    end
+  end
+
+  # Scopes the model on direct and indirect children of the record
+  def descendants
+    self.class.joins(:route).where('routes.path LIKE ?', "#{route.path}/%").reorder('routes.path ASC')
   end
 
   private

app/models/route.rb

@@ -8,15 +8,16 @@ class Route < ActiveRecord::Base
     presence: true,
     uniqueness: { case_sensitive: false }
 
-  after_update :rename_children, if: :path_changed?
+  after_update :rename_descendants, if: :path_changed?
 
-  def rename_children
+  def rename_descendants
     # We update each row separately because MySQL does not have regexp_replace.
     # rubocop:disable Rails/FindEach
     Route.where('path LIKE ?', "#{path_was}/%").each do |route|
       # Note that update column skips validation and callbacks.
-      # We need this to avoid recursive call of rename_children method
+      # We need this to avoid recursive call of rename_descendants method
       route.update_column(:path, route.path.sub(path_was, path))
     end
+    # rubocop:enable Rails/FindEach
   end
 end

app/models/user.rb

@@ -439,6 +439,15 @@ class User < ActiveRecord::Base
     Group.where("namespaces.id IN (#{union.to_sql})")
   end
 
+  def nested_groups
+    Group.member_descendants(id)
+  end
+
+  def nested_projects
+    Project.joins(:namespace).where('namespaces.parent_id IS NOT NULL').
+      member_descendants(id)
+  end
+
   def refresh_authorized_projects
     Users::RefreshAuthorizedProjectsService.new(self).execute
   end

app/services/users/refresh_authorized_projects_service.rb

@@ -118,7 +118,8 @@ module Users
         user.personal_projects.select("#{user.id} AS user_id, projects.id AS project_id, #{Gitlab::Access::MASTER} AS access_level"),
         user.groups_projects.select_for_project_authorization,
         user.projects.select_for_project_authorization,
-        user.groups.joins(:shared_projects).select_for_project_authorization
+        user.groups.joins(:shared_projects).select_for_project_authorization,
+        user.nested_projects.select_for_project_authorization
       ]
 
       Gitlab::SQL::Union.new(relations)

spec/models/concerns/routable_spec.rb

@@ -68,4 +68,14 @@ describe Group, 'Routable' do
       end
     end
   end
+
+  describe '.member_descendants' do
+    let!(:user) { create(:user) }
+    let!(:nested_group) { create(:group, parent: group) }
+
+    before { group.add_owner(user) }
+    subject { described_class.member_descendants(user.id) }
+
+    it { is_expected.to eq([nested_group]) }
+  end
 end

spec/models/namespace_spec.rb

@@ -5,6 +5,8 @@ describe Namespace, models: true do
 
   it { is_expected.to have_many :projects }
   it { is_expected.to have_many :project_statistics }
+  it { is_expected.to belong_to :parent }
+  it { is_expected.to have_many :children }
 
   it { is_expected.to validate_presence_of(:name) }
   it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) }
@@ -189,17 +191,31 @@ describe Namespace, models: true do
     it { expect(nested_group.full_name).to eq("#{group.name} / #{nested_group.name}") }
   end
 
-  describe '#parents' do
+  describe '#ancestors' do
     let(:group) { create(:group) }
     let(:nested_group) { create(:group, parent: group) }
     let(:deep_nested_group) { create(:group, parent: nested_group) }
     let(:very_deep_nested_group) { create(:group, parent: deep_nested_group) }
 
-    it 'returns the correct parents' do
-      expect(very_deep_nested_group.parents).to eq([group, nested_group, deep_nested_group])
-      expect(deep_nested_group.parents).to eq([group, nested_group])
-      expect(nested_group.parents).to eq([group])
-      expect(group.parents).to eq([])
+    it 'returns the correct ancestors' do
+      expect(very_deep_nested_group.ancestors).to eq([group, nested_group, deep_nested_group])
+      expect(deep_nested_group.ancestors).to eq([group, nested_group])
+      expect(nested_group.ancestors).to eq([group])
+      expect(group.ancestors).to eq([])
+    end
+  end
+
+  describe '#descendants' do
+    let!(:group) { create(:group) }
+    let!(:nested_group) { create(:group, parent: group) }
+    let!(:deep_nested_group) { create(:group, parent: nested_group) }
+    let!(:very_deep_nested_group) { create(:group, parent: deep_nested_group) }
+
+    it 'returns the correct descendants' do
+      expect(very_deep_nested_group.descendants.to_a).to eq([])
+      expect(deep_nested_group.descendants.to_a).to eq([very_deep_nested_group])
+      expect(nested_group.descendants.to_a).to eq([deep_nested_group, very_deep_nested_group])
+      expect(group.descendants.to_a).to eq([nested_group, deep_nested_group, very_deep_nested_group])
     end
   end
 end

spec/models/route_spec.rb

@@ -14,7 +14,7 @@ describe Route, models: true do
     it { is_expected.to validate_uniqueness_of(:path) }
   end
 
-  describe '#rename_children' do
+  describe '#rename_descendants' do
     let!(:nested_group) { create(:group, path: "test", parent: group) }
     let!(:deep_nested_group) { create(:group, path: "foo", parent: nested_group) }
     let!(:similar_group) { create(:group, path: 'gitlab-org') }

spec/models/user_spec.rb

@@ -1363,6 +1363,39 @@ describe User, models: true do
     end
   end
 
+  describe '#nested_groups' do
+    let!(:user) { create(:user) }
+    let!(:group) { create(:group) }
+    let!(:nested_group) { create(:group, parent: group) }
+
+    before do
+      group.add_owner(user)
+
+      # Add more data to ensure method does not include wrong groups
+      create(:group).add_owner(create(:user))
+    end
+
+    it { expect(user.nested_groups).to eq([nested_group]) }
+  end
+
+  describe '#nested_projects' do
+    let!(:user) { create(:user) }
+    let!(:group) { create(:group) }
+    let!(:nested_group) { create(:group, parent: group) }
+    let!(:project) { create(:project, namespace: group) }
+    let!(:nested_project) { create(:project, namespace: nested_group) }
+
+    before do
+      group.add_owner(user)
+
+      # Add more data to ensure method does not include wrong projects
+      other_project = create(:project, namespace: create(:group, :nested))
+      other_project.add_developer(create(:user))
+    end
+
+    it { expect(user.nested_projects).to eq([nested_project]) }
+  end
+
   describe '#refresh_authorized_projects', redis: true do
     let(:project1) { create(:empty_project) }
     let(:project2) { create(:empty_project) }