DEV: Exclude system users when calculating group user count (#25400)

We want to exclude the system user from group user counts, since intuitively admins wouldn't include them.

Originally this was accomplished by booting said system user from the groups, but this is causing problems, because the system user needs TL group membership to perform certain tasks.

After this PR, system user is still in the TL groups, but excluded when refreshing the user count.

app/jobs/regular/refresh_users_reviewable_counts.rb

@@ -4,8 +4,9 @@ class Jobs::RefreshUsersReviewableCounts < ::Jobs::Base
   def execute(args)
     group_ids = args[:group_ids]
     return if group_ids.blank?
-    User.where(id: GroupUser.where(group_id: group_ids).distinct.pluck(:user_id)).each(
+    User
-      &:publish_reviewable_counts
+      .human_users
-    )
+      .where(id: GroupUser.where(group_id: group_ids).distinct.select(:user_id))
+      .each(&:publish_reviewable_counts)
   end
 end

app/models/group.rb

@@ -554,13 +554,13 @@ class Group < ActiveRecord::Base
     remove_subquery =
       case name
       when :admins
-        "SELECT id FROM users WHERE id <= 0 OR NOT admin OR staged"
+        "SELECT id FROM users WHERE NOT admin OR staged"
       when :moderators
-        "SELECT id FROM users WHERE id <= 0 OR NOT moderator OR staged"
+        "SELECT id FROM users WHERE NOT moderator OR staged"
       when :staff
-        "SELECT id FROM users WHERE id <= 0 OR (NOT admin AND NOT moderator) OR staged"
+        "SELECT id FROM users WHERE (NOT admin AND NOT moderator) OR staged"
       when :trust_level_0, :trust_level_1, :trust_level_2, :trust_level_3, :trust_level_4
-        "SELECT id FROM users WHERE id <= 0 OR trust_level < #{id - 10} OR staged"
+        "SELECT id FROM users WHERE trust_level < #{id - 10} OR staged"
       end
 
     removed_user_ids = DB.query_single <<-SQL
@@ -584,15 +584,15 @@ class Group < ActiveRecord::Base
     insert_subquery =
       case name
       when :admins
-        "SELECT id FROM users WHERE id > 0 AND admin AND NOT staged"
+        "SELECT id FROM users WHERE admin AND NOT staged"
       when :moderators
-        "SELECT id FROM users WHERE id > 0 AND moderator AND NOT staged"
+        "SELECT id FROM users WHERE moderator AND NOT staged"
       when :staff
-        "SELECT id FROM users WHERE id > 0 AND (moderator OR admin) AND NOT staged"
+        "SELECT id FROM users WHERE (moderator OR admin) AND NOT staged"
       when :trust_level_1, :trust_level_2, :trust_level_3, :trust_level_4
-        "SELECT id FROM users WHERE id > 0 AND trust_level >= #{id - 10} AND NOT staged"
+        "SELECT id FROM users WHERE trust_level >= #{id - 10} AND NOT staged"
       when :trust_level_0
-        "SELECT id FROM users WHERE id > 0 AND NOT staged"
+        "SELECT id FROM users WHERE NOT staged"
       end
 
     added_user_ids = DB.query_single <<-SQL
@@ -636,14 +636,15 @@ class Group < ActiveRecord::Base
   end
 
   def self.reset_groups_user_count!(only_group_ids: [])
-    where_sql = ""
+    where_sql =
-
+      if only_group_ids.present?
-    if only_group_ids.present?
+        "WHERE group_id IN (#{only_group_ids.map(&:to_i).join(",")}) AND user_id > 0"
-      where_sql = "WHERE group_id IN (#{only_group_ids.map(&:to_i).join(",")})"
+      else
-    end
+        "WHERE user_id > 0"
+      end
 
     DB.exec <<-SQL
-      WITH X AS (
+      WITH tally AS (
         SELECT
           group_id,
           COUNT(user_id) users
@@ -652,10 +653,10 @@ class Group < ActiveRecord::Base
         GROUP BY group_id
       )
       UPDATE groups
-         SET user_count = X.users
+         SET user_count = tally.users
-        FROM X
+        FROM tally
-       WHERE id = X.group_id
+       WHERE id = tally.group_id
-         AND user_count <> X.users
+         AND user_count <> tally.users
     SQL
   end
   private_class_method :reset_groups_user_count!
@@ -940,7 +941,8 @@ class Group < ActiveRecord::Base
       SET user_count =
         (SELECT COUNT(gu.user_id)
          FROM group_users gu
-         WHERE gu.group_id = g.id)
+         WHERE gu.group_id = g.id
+         AND gu.user_id > 0)
       WHERE g.id = #{self.id};
     SQL
   end

spec/models/group_spec.rb

@@ -491,24 +491,23 @@ RSpec.describe Group do
     Group.delete_all
     Group.refresh_automatic_groups!
 
-    groups = Group.includes(:users).to_a
+    expect(Group.count).to eq Group::AUTO_GROUPS.count
-    expect(groups.count).to eq Group::AUTO_GROUPS.count
 
-    g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:admins] }
+    g = Group[:admins]
-    expect(g.users.count).to eq g.user_count
+    expect(g.human_users.count).to eq(g.user_count)
-    expect(g.users.pluck(:id)).to contain_exactly(admin.id)
+    expect(g.human_users).to contain_exactly(admin)
 
-    g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:staff] }
+    g = Group[:admins]
-    expect(g.users.count).to eq g.user_count
+    expect(g.human_users.count).to eq(g.user_count)
-    expect(g.users.pluck(:id)).to contain_exactly(admin.id)
+    expect(g.human_users).to contain_exactly(admin)
 
-    g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:trust_level_1] }
+    g = Group[:trust_level_1]
-    expect(g.users.count).to eq g.user_count
+    expect(g.human_users.count).to eq(g.user_count)
-    expect(g.users.pluck(:id)).to contain_exactly(admin.id, user.id)
+    expect(g.human_users).to contain_exactly(admin, user)
 
-    g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:trust_level_2] }
+    g = Group[:trust_level_2]
-    expect(g.users.count).to eq g.user_count
+    expect(g.human_users.count).to eq(g.user_count)
-    expect(g.users.pluck(:id)).to contain_exactly(user.id)
+    expect(g.human_users).to contain_exactly(user)
   end
 
   it "can set members via usernames helper" do