From 6ad34a0152934b05a6ffe7cfec951c033dc58e69 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Thu, 25 Jan 2024 08:13:58 +0800 Subject: [PATCH] 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. --- .../refresh_users_reviewable_counts.rb | 7 ++-- app/models/group.rb | 42 ++++++++++--------- spec/models/group_spec.rb | 27 ++++++------ 3 files changed, 39 insertions(+), 37 deletions(-) diff --git a/app/jobs/regular/refresh_users_reviewable_counts.rb b/app/jobs/regular/refresh_users_reviewable_counts.rb index 10fa49467ff..27e743d14d7 100644 --- a/app/jobs/regular/refresh_users_reviewable_counts.rb +++ b/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( - &:publish_reviewable_counts - ) + User + .human_users + .where(id: GroupUser.where(group_id: group_ids).distinct.select(:user_id)) + .each(&:publish_reviewable_counts) end end diff --git a/app/models/group.rb b/app/models/group.rb index 0357e98695c..26dd49a28eb 100644 --- a/app/models/group.rb +++ b/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 = "" - - if only_group_ids.present? - where_sql = "WHERE group_id IN (#{only_group_ids.map(&:to_i).join(",")})" - end + where_sql = + if only_group_ids.present? + "WHERE group_id IN (#{only_group_ids.map(&:to_i).join(",")}) AND user_id > 0" + else + "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 - FROM X - WHERE id = X.group_id - AND user_count <> X.users + SET user_count = tally.users + FROM tally + WHERE id = tally.group_id + 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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 13cebcdb879..06d63d096a6 100644 --- a/spec/models/group_spec.rb +++ b/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(groups.count).to eq Group::AUTO_GROUPS.count + expect(Group.count).to eq Group::AUTO_GROUPS.count - g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:admins] } - expect(g.users.count).to eq g.user_count - expect(g.users.pluck(:id)).to contain_exactly(admin.id) + g = Group[:admins] + expect(g.human_users.count).to eq(g.user_count) + expect(g.human_users).to contain_exactly(admin) - g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:staff] } - expect(g.users.count).to eq g.user_count - expect(g.users.pluck(:id)).to contain_exactly(admin.id) + g = Group[:admins] + expect(g.human_users.count).to eq(g.user_count) + expect(g.human_users).to contain_exactly(admin) - g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:trust_level_1] } - expect(g.users.count).to eq g.user_count - expect(g.users.pluck(:id)).to contain_exactly(admin.id, user.id) + g = Group[:trust_level_1] + expect(g.human_users.count).to eq(g.user_count) + expect(g.human_users).to contain_exactly(admin, user) - g = groups.find { |grp| grp.id == Group::AUTO_GROUPS[:trust_level_2] } - expect(g.users.count).to eq g.user_count - expect(g.users.pluck(:id)).to contain_exactly(user.id) + g = Group[:trust_level_2] + expect(g.human_users.count).to eq(g.user_count) + expect(g.human_users).to contain_exactly(user) end it "can set members via usernames helper" do