FIX: participating users statistics... (#29293)

* FIX: participating users statistics...

... was (mis-)counting

- bots
- anonymous users
- suspended users

There's now a "valid_users" function that holds the AR query for valid users and which is used in all "users", "active_users", and "participating_users" queries.

Internal ref - t/138435

app/models/user.rb

@@ -303,6 +303,7 @@ class User < ActiveRecord::Base
   scope :not_suspended, -> { where("suspended_till IS NULL OR suspended_till <= ?", Time.zone.now) }
   scope :activated, -> { where(active: true) }
   scope :not_staged, -> { where(staged: false) }
+  scope :approved, -> { where(approved: true) }
 
   scope :filter_by_username,
         ->(filter) do

lib/statistics.rb

@@ -33,21 +33,20 @@ class Statistics
 
   def self.active_users
     {
-      last_day: User.where("last_seen_at > ?", 1.day.ago).count,
-      "7_days": User.where("last_seen_at > ?", 7.days.ago).count,
-      "30_days": User.where("last_seen_at > ?", 30.days.ago).count,
+      last_day: valid_users.where("last_seen_at > ?", 1.day.ago).count,
+      "7_days": valid_users.where("last_seen_at > ?", 7.days.ago).count,
+      "30_days": valid_users.where("last_seen_at > ?", 30.days.ago).count,
     }
   end
 
   def self.likes
+    likes = UserAction.where(action_type: UserAction::LIKE)
+
     {
-      last_day:
-        UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 1.day.ago).count,
-      "7_days":
-        UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 7.days.ago).count,
-      "30_days":
-        UserAction.where(action_type: UserAction::LIKE).where("created_at > ?", 30.days.ago).count,
-      count: UserAction.where(action_type: UserAction::LIKE).count,
+      last_day: likes.where("created_at > ?", 1.day.ago).count,
+      "7_days": likes.where("created_at > ?", 7.days.ago).count,
+      "30_days": likes.where("created_at > ?", 30.days.ago).count,
+      count: likes.count,
     }
   end
 
@@ -61,24 +60,22 @@ class Statistics
   end
 
   def self.topics
+    topics = Topic.listable_topics
+
     {
-      last_day: Topic.listable_topics.where("created_at > ?", 1.day.ago).count,
-      "7_days": Topic.listable_topics.where("created_at > ?", 7.days.ago).count,
-      "30_days": Topic.listable_topics.where("created_at > ?", 30.days.ago).count,
-      count: Topic.listable_topics.count,
+      last_day: topics.where("created_at > ?", 1.day.ago).count,
+      "7_days": topics.where("created_at > ?", 7.days.ago).count,
+      "30_days": topics.where("created_at > ?", 30.days.ago).count,
+      count: topics.count,
     }
   end
 
   def self.users
-    query = User.real.not_silenced.activated
-
-    query = query.where(approved: true) if SiteSetting.must_approve_users
-
     {
-      last_day: query.where("created_at > ?", 1.day.ago).count,
-      "7_days": query.where("created_at > ?", 7.days.ago).count,
-      "30_days": query.where("created_at > ?", 30.days.ago).count,
-      count: query.count,
+      last_day: valid_users.where("created_at > ?", 1.day.ago).count,
+      "7_days": valid_users.where("created_at > ?", 7.days.ago).count,
+      "30_days": valid_users.where("created_at > ?", 30.days.ago).count,
+      count: valid_users.count,
     }
   end
 
@@ -144,20 +141,31 @@ class Statistics
 
   private
 
+  def self.valid_users
+    users = User.real.activated.not_suspended.not_silenced
+    users = users.approved if SiteSetting.must_approve_users
+    users
+  end
+
   def self.participating_users_count(date)
     subqueries = [
       "SELECT DISTINCT user_id FROM user_actions WHERE created_at > :date AND action_type IN (:action_types)",
     ]
 
     if ActiveRecord::Base.connection.data_source_exists?("chat_messages")
-      subqueries << "SELECT DISTINCT user_id FROM chat_messages WHERE created_at > :date"
+      subqueries << "SELECT DISTINCT user_id FROM chat_messages WHERE created_at > :date AND deleted_at IS NULL"
     end
 
     if ActiveRecord::Base.connection.data_source_exists?("chat_message_reactions")
       subqueries << "SELECT DISTINCT user_id FROM chat_message_reactions WHERE created_at > :date"
     end
 
-    sql = "SELECT COUNT(user_id) FROM (#{subqueries.join(" UNION ")}) u"
+    sql = <<~SQL
+      WITH valid_users AS (#{valid_users.select(:id).to_sql})
+      SELECT COUNT(DISTINCT user_id) 
+      FROM (#{subqueries.join(" UNION ")}) participating_users
+      JOIN valid_users ON valid_users.id = participating_users.user_id
+    SQL
 
     DB.query_single(sql, date: date, action_types: UserAction::USER_ACTED_TYPES).first
   end

spec/lib/statistics_spec.rb

@@ -56,7 +56,7 @@ RSpec.describe Statistics do
   describe ".users" do
     before { User.real.destroy_all }
 
-    it "doesn't count silenced or inactive users" do
+    it "doesn't count inactive, silenced, or suspended users" do
       res = described_class.users
       expect(res[:last_day]).to eq(0)
       expect(res[:"7_days"]).to eq(0)
@@ -65,6 +65,15 @@ RSpec.describe Statistics do
 
       user = Fabricate(:user, active: true)
       user2 = Fabricate(:user, active: true)
+      user3 = Fabricate(:user, active: true)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(3)
+      expect(res[:"7_days"]).to eq(3)
+      expect(res[:"30_days"]).to eq(3)
+      expect(res[:count]).to eq(3)
+
+      user.update!(active: false)
 
       res = described_class.users
       expect(res[:last_day]).to eq(2)
@@ -72,7 +81,7 @@ RSpec.describe Statistics do
       expect(res[:"30_days"]).to eq(2)
       expect(res[:count]).to eq(2)
 
-      user.update!(active: false)
+      user2.update!(silenced_till: 1.month.from_now)
 
       res = described_class.users
       expect(res[:last_day]).to eq(1)
@@ -80,7 +89,7 @@ RSpec.describe Statistics do
       expect(res[:"30_days"]).to eq(1)
       expect(res[:count]).to eq(1)
 
-      user2.update!(silenced_till: 1.month.from_now)
+      user3.update!(suspended_till: 1.month.from_now)
 
       res = described_class.users
       expect(res[:last_day]).to eq(0)
@@ -188,6 +197,11 @@ RSpec.describe Statistics do
       Fabricate(:user_action, action_type: UserAction::NEW_PRIVATE_MESSAGE)
       expect(described_class.participating_users[:last_day]).to eq(1)
     end
+
+    it "doesn't count bots" do
+      Fabricate(:user_action, action_type: UserAction::LIKE, user: Discourse.system_user)
+      expect(described_class.participating_users[:last_day]).to eq(0)
+    end
   end
 
   describe ".visitors" do