Load counts everywhere we render a group tree

7 files changed, 99 insertions, 71 deletions

diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb
index 9104d4a32cf..e969087f0b5 100644
--- a/app/controllers/concerns/group_tree.rb
+++ b/app/controllers/concerns/group_tree.rb
@@ -7,7 +7,7 @@ module GroupTree
       # 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.with_selects_for_list
                 .sort(@sort = params[:sort])
                 .page(params[:page])
 
diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb
index 4e7ae6f499b..1e6523b70a2 100644
--- a/app/finders/group_descendants_finder.rb
+++ b/app/finders/group_descendants_finder.rb
@@ -3,30 +3,6 @@ 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
@@ -60,7 +36,7 @@ class GroupDescendantsFinder
   end
 
   def collections
-    [subgroups.with_route.select(GROUP_SELECTS), projects]
+    [subgroups.with_selects_for_list, projects]
   end
 
   def paginator
@@ -113,7 +89,7 @@ class GroupDescendantsFinder
     projects_to_load_ancestors_of = projects.where.not(namespace: parent_group)
     groups_to_load_ancestors_of = Group.where(id: projects_to_load_ancestors_of.select(:namespace_id))
     ancestors_for_groups(groups_to_load_ancestors_of)
-      .with_route.select(GROUP_SELECTS)
+      .with_selects_for_list
   end
 
   def subgroups
diff --git a/app/models/concerns/loaded_in_group_list.rb b/app/models/concerns/loaded_in_group_list.rb
new file mode 100644
index 00000000000..e73ddcfb567
--- /dev/null
+++ b/app/models/concerns/loaded_in_group_list.rb
@@ -0,0 +1,65 @@
+module LoadedInGroupList
+  extend ActiveSupport::Concern
+
+  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
+
+  COUNT_SELECTS = ['namespaces.*',
+                   PROJECT_COUNT_SQL,
+                   SUBGROUP_COUNT_SQL,
+                   MEMBER_COUNT_SQL].freeze
+
+  module ClassMethods
+    def with_counts
+      select(COUNT_SELECTS)
+    end
+
+    def with_selects_for_list
+      with_route.with_counts
+    end
+  end
+
+  def children_count
+    @children_count ||= project_count + subgroup_count
+  end
+
+  def project_count
+    @project_count ||= if respond_to?(:preloaded_project_count)
+                         preloaded_project_count
+                       else
+                         projects.non_archived.count
+                       end
+  end
+
+  def subgroup_count
+    @subgroup_count ||= if respond_to?(:preloaded_subgroup_count)
+                          preloaded_subgroup_count
+                        else
+                          children.count
+                        end
+  end
+
+  def member_count
+    @member_count ||= if respond_to?(:preloaded_member_count)
+                        preloaded_member_count
+                      else
+                        users.count
+                      end
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index 36439849086..07fb62bb249 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 LoadedInGroupList
   include GroupDescendant
 
   has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb
index 91e26272355..6cfdd93e9bb 100644
--- a/app/serializers/group_child_entity.rb
+++ b/app/serializers/group_child_entity.rb
@@ -54,31 +54,6 @@ class GroupChildEntity < Grape::Entity
          :number_users_with_delimiter, :project_count, :subgroup_count, :can_leave,
          unless: lambda { |_instance, _options| project? }
 
-  def children_finder
-    @children_finder ||= GroupDescendantsFinder.new(current_user: request.current_user,
-                                                    parent_group: object)
-  end
-
-  def children_count
-    @children_count ||= project_count + subgroup_count
-  end
-
-  def 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 ||= if object.respond_to?(:preloaded_subgroup_count)
-                          object.preloaded_subgroup_count
-                        else
-                          children_finder.subgroup_count
-                        end
-  end
-
   def leave_path
     leave_group_group_members_path(object)
   end
@@ -92,15 +67,10 @@ class GroupChildEntity < Grape::Entity
   end
 
   def number_projects_with_delimiter
-    number_with_delimiter(project_count)
+    number_with_delimiter(object.project_count)
   end
 
   def number_users_with_delimiter
-    member_count = if object.respond_to?(:preloaded_member_count)
-                     object.preloaded_member_count
-                   else
-                     object.users.count
-                   end
-    number_with_delimiter(member_count)
+    number_with_delimiter(object.member_count)
   end
 end
diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb
index 7b9dfcbfad0..86a7a793457 100644
--- a/spec/finders/group_descendants_finder_spec.rb
+++ b/spec/finders/group_descendants_finder_spec.rb
@@ -46,18 +46,6 @@ 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
-
       it 'does not include subgroups the user does not have access to' do
         subgroup.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
 
diff --git a/spec/models/concerns/loaded_in_group_list_spec.rb b/spec/models/concerns/loaded_in_group_list_spec.rb
new file mode 100644
index 00000000000..d64b288aa0c
--- /dev/null
+++ b/spec/models/concerns/loaded_in_group_list_spec.rb
@@ -0,0 +1,28 @@
+require 'spec_helper'
+
+describe LoadedInGroupList do
+  let(:parent) { create(:group) }
+  subject(:found_group) { Group.with_selects_for_list.find_by(id: parent.id) }
+
+  before do
+    create(:group, parent: parent)
+    create(:project, namespace: parent)
+    parent.add_developer(create(:user))
+  end
+
+  describe '.with_selects_for_list' do
+    it 'includes the preloaded counts for groups' do
+      found_group = Group.with_selects_for_list.find_by(id: parent.id)
+
+      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
+  end
+
+  describe '#children_count' do
+    it 'counts groups and projects' do
+      expect(found_group.children_count).to eq(2)
+    end
+  end
+end