From 64b67e0a4fcf8ca9d4badb72f833594cb4985300 Mon Sep 17 00:00:00 2001
From: Osama Sayegh <asooomaasoooma90@gmail.com>
Date: Thu, 12 Sep 2024 22:28:49 +0300
Subject: [PATCH] FIX: Exclude inactive and silenced users from /about page
 stats (#28877)

The user directory (`/u`) excludes inactive and silenced users from the list, so for the sake parity, it makes sense to also exclude those users from the /about page stats.

Internal topic: t/70928.
---
 lib/statistics.rb           |  12 ++--
 spec/lib/statistics_spec.rb | 108 ++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 4 deletions(-)

diff --git a/lib/statistics.rb b/lib/statistics.rb
index 1beda7cd6b7..3caeabb9776 100644
--- a/lib/statistics.rb
+++ b/lib/statistics.rb
@@ -70,11 +70,15 @@ class Statistics
   end
 
   def self.users
+    query = User.real.not_silenced.activated
+
+    query = query.where(approved: true) if SiteSetting.must_approve_users
+
     {
-      last_day: User.real.where("created_at > ?", 1.day.ago).count,
-      "7_days": User.real.where("created_at > ?", 7.days.ago).count,
-      "30_days": User.real.where("created_at > ?", 30.days.ago).count,
-      count: User.real.count,
+      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,
     }
   end
 
diff --git a/spec/lib/statistics_spec.rb b/spec/lib/statistics_spec.rb
index 02c568c8d1c..25d35e7a847 100644
--- a/spec/lib/statistics_spec.rb
+++ b/spec/lib/statistics_spec.rb
@@ -53,6 +53,114 @@ RSpec.describe Statistics do
   fab!(:users) { Fabricate.times(5, :user) }
   let(:date) { DateTime.parse("2024-03-01 13:00") }
 
+  describe ".users" do
+    before { User.real.destroy_all }
+
+    it "doesn't count silenced or inactive users" do
+      res = described_class.users
+      expect(res[:last_day]).to eq(0)
+      expect(res[:"7_days"]).to eq(0)
+      expect(res[:"30_days"]).to eq(0)
+      expect(res[:count]).to eq(0)
+
+      user = Fabricate(:user, active: true)
+      user2 = Fabricate(:user, active: true)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(2)
+      expect(res[:"7_days"]).to eq(2)
+      expect(res[:"30_days"]).to eq(2)
+      expect(res[:count]).to eq(2)
+
+      user.update!(active: false)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(1)
+      expect(res[:"7_days"]).to eq(1)
+      expect(res[:"30_days"]).to eq(1)
+      expect(res[:count]).to eq(1)
+
+      user2.update!(silenced_till: 1.month.from_now)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(0)
+      expect(res[:"7_days"]).to eq(0)
+      expect(res[:"30_days"]).to eq(0)
+      expect(res[:count]).to eq(0)
+    end
+
+    it "doesn't include unapproved users if must_approve_users setting is true" do
+      SiteSetting.must_approve_users = false
+
+      user = Fabricate(:user, active: true, approved: false)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(1)
+      expect(res[:"7_days"]).to eq(1)
+      expect(res[:"30_days"]).to eq(1)
+      expect(res[:count]).to eq(1)
+
+      SiteSetting.must_approve_users = true
+      # changing the site setting approves all existing users
+      # flip this one back to unapproved
+      user.reload.update!(approved: false)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(0)
+      expect(res[:"7_days"]).to eq(0)
+      expect(res[:"30_days"]).to eq(0)
+      expect(res[:count]).to eq(0)
+
+      user.update!(approved: true)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(1)
+      expect(res[:"7_days"]).to eq(1)
+      expect(res[:"30_days"]).to eq(1)
+      expect(res[:count]).to eq(1)
+    end
+
+    it "counts users in the time windows they were created in" do
+      res = described_class.users
+      expect(res[:last_day]).to eq(0)
+      expect(res[:"7_days"]).to eq(0)
+      expect(res[:"30_days"]).to eq(0)
+      expect(res[:count]).to eq(0)
+
+      Fabricate(:user, active: true, created_at: 31.days.ago)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(0)
+      expect(res[:"7_days"]).to eq(0)
+      expect(res[:"30_days"]).to eq(0)
+      expect(res[:count]).to eq(1)
+
+      Fabricate(:user, active: true, created_at: 28.days.ago)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(0)
+      expect(res[:"7_days"]).to eq(0)
+      expect(res[:"30_days"]).to eq(1)
+      expect(res[:count]).to eq(2)
+
+      Fabricate(:user, active: true, created_at: 6.days.ago)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(0)
+      expect(res[:"7_days"]).to eq(1)
+      expect(res[:"30_days"]).to eq(2)
+      expect(res[:count]).to eq(3)
+
+      Fabricate(:user, active: true, created_at: 6.hours.ago)
+
+      res = described_class.users
+      expect(res[:last_day]).to eq(1)
+      expect(res[:"7_days"]).to eq(2)
+      expect(res[:"30_days"]).to eq(3)
+      expect(res[:count]).to eq(4)
+    end
+  end
+
   describe ".participating_users" do
     it "returns no participating users by default" do
       pu = described_class.participating_users