From b876ff6281c247ca0c2a070d69e9af523ba432bc Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Fri, 11 Feb 2022 09:00:58 +0800
Subject: [PATCH] FIX: Update user stat counts when post/topic visibility
 changes. (#15883)

Breakdown of fixes in this commit:

* `UserStat#topic_count` was not updated when visibility of
the topic changed.

* `UserStat#post_count` was not updated when post was hidden or
unhidden.

* `TopicConverter` was only incrementing or decrementing the counts by 1
even if a user has multiple posts in the topic.

* The commit turns off the verbose logging by default as it is just
noise to normal users who are not debugging this problem.
---
 app/models/post.rb                            | 25 ++++++---
 app/models/topic_converter.rb                 | 47 +++++++++++++---
 app/services/topic_status_updater.rb          |  3 +-
 app/services/user_stat_count_updater.rb       | 19 ++++---
 config/site_settings.yml                      |  3 +
 lib/post_creator.rb                           |  4 +-
 spec/components/post_creator_spec.rb          |  2 +
 spec/fabricators/topic_fabricator.rb          |  7 ---
 spec/jobs/publish_topic_to_category_spec.rb   |  2 +-
 spec/models/post_spec.rb                      | 55 ++++++++++++++++---
 spec/models/topic_converter_spec.rb           | 54 +++++++++++++++---
 spec/models/topic_spec.rb                     | 37 ++++++++++---
 spec/requests/posts_controller_spec.rb        |  2 +-
 spec/requests/topics_controller_spec.rb       |  2 +-
 spec/services/user_stat_count_updater_spec.rb |  3 +
 15 files changed, 204 insertions(+), 61 deletions(-)

diff --git a/app/models/post.rb b/app/models/post.rb
index 50ecee6986b..2acb46814af 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -546,12 +546,17 @@ class Post < ActiveRecord::Base
     self.hidden_at = Time.zone.now
     self.hidden_reason_id = reason
     self.skip_unique_check = true
-    save!
 
-    Topic.where(
-      "id = :topic_id AND NOT EXISTS(SELECT 1 FROM POSTS WHERE topic_id = :topic_id AND NOT hidden)",
-      topic_id: topic_id
-    ).update_all(visible: false)
+    Post.transaction do
+      save!
+
+      Topic.where(
+        "id = :topic_id AND NOT EXISTS(SELECT 1 FROM POSTS WHERE topic_id = :topic_id AND NOT hidden)",
+        topic_id: topic_id
+      ).update_all(visible: false)
+
+      UserStatCountUpdater.decrement!(self)
+    end
 
     # inform user
     if user.present?
@@ -581,9 +586,13 @@ class Post < ActiveRecord::Base
   end
 
   def unhide!
-    self.update(hidden: false)
-    self.topic.update(visible: true) if is_first_post?
-    save(validate: false)
+    Post.transaction do
+      self.update!(hidden: false)
+      self.topic.update(visible: true) if is_first_post?
+      UserStatCountUpdater.increment!(self)
+      save(validate: false)
+    end
+
     publish_change_to_clients!(:acted)
   end
 
diff --git a/app/models/topic_converter.rb b/app/models/topic_converter.rb
index 352258eaece..e9988ab89dc 100644
--- a/app/models/topic_converter.rb
+++ b/app/models/topic_converter.rb
@@ -40,7 +40,7 @@ class TopicConverter
 
   def convert_to_private_message
     Topic.transaction do
-      @topic.update_category_topic_count_by(-1)
+      @topic.update_category_topic_count_by(-1) if @topic.visible
 
       PostRevisor.new(@topic.first_post, @topic).revise!(
         @user,
@@ -66,18 +66,47 @@ class TopicConverter
     @posters ||= @topic.posts.where("post_number > 1").distinct.pluck(:user_id)
   end
 
+  def increment_users_post_count
+    update_users_post_count(:increment)
+  end
+
+  def decrement_users_post_count
+    update_users_post_count(:decrement)
+  end
+
+  def update_users_post_count(action)
+    operation = action == :increment ? "+" : "-"
+
+    # NOTE that DirectoryItem.refresh will overwrite this by counting UserAction records.
+    #
+    # Changes user_stats (post_count) by the number of posts in the topic.
+    # First post, hidden posts and non-regular posts are ignored.
+    DB.exec(<<~SQL)
+    UPDATE user_stats
+    SET post_count = post_count #{operation} X.count
+    FROM (
+      SELECT
+        us.user_id,
+        COUNT(*) AS count
+      FROM user_stats us
+      INNER JOIN posts ON posts.topic_id = #{@topic.id.to_i} AND posts.user_id = us.user_id
+      WHERE posts.post_number > 1
+      AND NOT posts.hidden
+      AND posts.post_type = #{Post.types[:regular].to_i}
+      GROUP BY us.user_id
+    ) X
+    WHERE X.user_id = user_stats.user_id
+    SQL
+  end
+
   def update_user_stats
-    # update posts count. NOTE that DirectoryItem.refresh will overwrite this by counting UserAction records.
-    # update topics count
-    UserStat.where(user_id: posters).update_all('post_count = post_count + 1')
-    UserStat.where(user_id: @topic.user_id).update_all('topic_count = topic_count + 1')
+    increment_users_post_count
+    UserStatCountUpdater.increment!(@topic.first_post)
   end
 
   def add_allowed_users
-    # update posts count. NOTE that DirectoryItem.refresh will overwrite this by counting UserAction records.
-    # update topics count
-    UserStat.where(user_id: posters).update_all('post_count = post_count - 1')
-    UserStat.where(user_id: @topic.user_id).update_all('topic_count = topic_count - 1')
+    decrement_users_post_count
+    UserStatCountUpdater.decrement!(@topic.first_post)
 
     existing_allowed_users = @topic.topic_allowed_users.pluck(:user_id)
     users_to_allow = posters << @user.id
diff --git a/app/services/topic_status_updater.rb b/app/services/topic_status_updater.rb
index fe0f912ec0d..d5e1d95d446 100644
--- a/app/services/topic_status_updater.rb
+++ b/app/services/topic_status_updater.rb
@@ -46,8 +46,9 @@ TopicStatusUpdater = Struct.new(:topic, :user) do
       UserProfile.remove_featured_topic_from_all_profiles(topic)
     end
 
-    if status.visible?
+    if status.visible? && result
       topic.update_category_topic_count_by(status.enabled? ? 1 : -1)
+      UserStatCountUpdater.public_send(status.enabled? ? :increment! : :decrement!, topic.first_post)
     end
 
     if @topic_timer
diff --git a/app/services/user_stat_count_updater.rb b/app/services/user_stat_count_updater.rb
index 90866295d7b..caa1b07080a 100644
--- a/app/services/user_stat_count_updater.rb
+++ b/app/services/user_stat_count_updater.rb
@@ -13,9 +13,11 @@ class UserStatCountUpdater
     private
 
     def update!(post, user_stat: nil, action: :increment!)
-      return if !post.topic
-      return if post.topic.private_message?
-      stat = user_stat || post.user.user_stat
+      return if !post&.topic
+      return if action == :increment! && post.topic.private_message?
+      stat = user_stat || post.user&.user_stat
+
+      return if stat.blank?
 
       column =
         if post.is_first_post?
@@ -26,11 +28,14 @@ class UserStatCountUpdater
 
       return if column.blank?
 
+      # There are lingering bugs in the code base that does not properly increase the count when the status of the post
+      # changes. Since we have Job::DirectoryRefreshOlder which runs daily to reconcile the count, there is no need
+      # to trigger an error.
       if action == :decrement! && stat.public_send(column) < 1
-        # There are still spots in the code base which results in the counter cache going out of sync. However,
-        # we have a job that runs on a daily basis which will correct the count. Therefore, we always check that we
-        # wouldn't end up with a negative count first before inserting.
-        Rails.logger.warn("Attempted to insert negative count into UserStat##{column}\n#{caller.join('\n')}")
+        if SiteSetting.verbose_user_stat_count_logging
+          Rails.logger.warn("Attempted to insert negative count into UserStat##{column}} for post with id '#{post.id}'")
+        end
+
         return
       end
 
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 9ca6bde4e7f..62aa97487b1 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -2588,3 +2588,6 @@ dashboard:
       - flags
       - user_to_user_private_messages_with_replies
       - signups
+  verbose_user_stat_count_logging:
+    hidden: true
+    default: false
diff --git a/lib/post_creator.rb b/lib/post_creator.rb
index 2e56397c993..39271eae907 100644
--- a/lib/post_creator.rb
+++ b/lib/post_creator.rb
@@ -603,7 +603,9 @@ class PostCreator
       @user.user_stat.update!(first_post_created_at: @post.created_at)
     end
 
-    UserStatCountUpdater.increment!(@post)
+    if !@post.hidden || @post.topic.visible
+      UserStatCountUpdater.increment!(@post)
+    end
 
     if !@topic.private_message? && @post.post_type != Post.types[:whisper]
       @user.update(last_posted_at: @post.created_at)
diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb
index 465d83300f2..2c58aba6377 100644
--- a/spec/components/post_creator_spec.rb
+++ b/spec/components/post_creator_spec.rb
@@ -50,6 +50,8 @@ describe PostCreator do
       expect(post.hidden_at).to be_present
       expect(post.hidden_reason_id).to eq(hri)
       expect(post.topic.visible).to eq(false)
+      expect(post.user.topic_count).to eq(0)
+      expect(post.user.post_count).to eq(0)
     end
 
     it "ensures the user can create the topic" do
diff --git a/spec/fabricators/topic_fabricator.rb b/spec/fabricators/topic_fabricator.rb
index 2b7c47605fe..b65cd48d7fa 100644
--- a/spec/fabricators/topic_fabricator.rb
+++ b/spec/fabricators/topic_fabricator.rb
@@ -6,13 +6,6 @@ Fabricator(:topic) do
   category_id do |attrs|
     attrs[:category] ? attrs[:category].id : SiteSetting.uncategorized_category_id
   end
-
-  # Fabrication bypasses PostCreator, for performance reasons, where the counts are updated so we have to handle this manually here.
-  after_save do |topic, _transients|
-    if !topic.private_message?
-      topic.user.user_stat.increment!(:topic_count)
-    end
-  end
 end
 
 Fabricator(:deleted_topic, from: :topic) do
diff --git a/spec/jobs/publish_topic_to_category_spec.rb b/spec/jobs/publish_topic_to_category_spec.rb
index 7d895de0cac..27c21e8a5b4 100644
--- a/spec/jobs/publish_topic_to_category_spec.rb
+++ b/spec/jobs/publish_topic_to_category_spec.rb
@@ -17,7 +17,7 @@ RSpec.describe Jobs::PublishTopicToCategory do
       created_at: 5.minutes.ago
     )
 
-    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic, user: topic.user)
 
     topic
   end
diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb
index 84475ff147e..8e62d926744 100644
--- a/spec/models/post_spec.rb
+++ b/spec/models/post_spec.rb
@@ -1302,22 +1302,44 @@ describe Post do
     end
   end
 
-  describe ".hide!" do
+  describe "#hide!" do
+    fab!(:post) { Fabricate(:post) }
+
     after do
       Discourse.redis.flushdb
     end
 
-    it "should ignore the duplicate check" do
-      p1 = Fabricate(:post)
-      p2 = Fabricate(:post, user: p1.user)
+    it "should ignore the unique post validator when hiding a post with similar content as a recent post" do
+      post_2 = Fabricate(:post, user: post.user)
       SiteSetting.unique_posts_mins = 10
-      p1.store_unique_post_key
-      p2.reload.hide!(PostActionType.types[:off_topic])
-      expect(p2).to be_hidden
+      post.store_unique_post_key
+
+      expect(post_2.valid?).to eq(false)
+      expect(post_2.errors.full_messages.to_s).to include(I18n.t(:just_posted_that))
+
+      post_2.hide!(PostActionType.types[:off_topic])
+
+      expect(post_2.reload.hidden).to eq(true)
+    end
+
+    it 'should decrease user_stat topic_count for first post' do
+      expect do
+        post.hide!(PostActionType.types[:off_topic])
+      end.to change { post.user.user_stat.reload.topic_count }.from(1).to(0)
+    end
+
+    it 'should decrease user_stat post_count' do
+      post_2 = Fabricate(:post, topic: post.topic, user: post.user)
+
+      expect do
+        post_2.hide!(PostActionType.types[:off_topic])
+      end.to change { post_2.user.user_stat.reload.post_count }.from(1).to(0)
     end
   end
 
-  describe ".unhide!" do
+  describe "#unhide!" do
+    fab!(:post) { Fabricate(:post) }
+
     before { SiteSetting.unique_posts_mins = 5 }
 
     it "will unhide the first post & make the topic visible" do
@@ -1339,6 +1361,23 @@ describe Post do
       expect(post.hidden).to eq(false)
       expect(hidden_topic.visible).to eq(true)
     end
+
+    it 'should increase user_stat topic_count for first post' do
+      post.hide!(PostActionType.types[:off_topic])
+
+      expect do
+        post.unhide!
+      end.to change { post.user.user_stat.reload.topic_count }.from(0).to(1)
+    end
+
+    it 'should decrease user_stat post_count' do
+      post_2 = Fabricate(:post, topic: post.topic, user: post.user)
+      post_2.hide!(PostActionType.types[:off_topic])
+
+      expect do
+        post_2.unhide!
+      end.to change { post_2.user.user_stat.reload.post_count }.from(0).to(1)
+    end
   end
 
   it "will unhide the post but will keep the topic invisible/unlisted" do
diff --git a/spec/models/topic_converter_spec.rb b/spec/models/topic_converter_spec.rb
index ed6d050cbde..c4ea2eb2f47 100644
--- a/spec/models/topic_converter_spec.rb
+++ b/spec/models/topic_converter_spec.rb
@@ -21,10 +21,25 @@ describe TopicConverter do
         SiteSetting.allow_uncategorized_topics = true
         topic = nil
 
+        _pm_post_2 = Fabricate(:post, topic: private_message, user: author)
+        _pm_post_3 = Fabricate(:post, topic: private_message, user: author)
+
+        other_pm = Fabricate(:private_message_post).topic
+        other_pm_post = Fabricate(:private_message_post, topic: other_pm)
+        other_pm_post_2 = Fabricate(:private_message_post, topic: other_pm, user: other_pm_post.user)
+
         expect do
           topic = TopicConverter.new(first_post.topic, admin).convert_to_public_topic
           topic.reload
         end.to change { uncategorized_category.reload.topic_count }.by(1)
+          .and change { author.reload.topic_count }.from(0).to(1)
+          .and change { author.reload.post_count }.from(0).to(2)
+
+        # Ensure query does not affect users from other topics or posts as DB query to update count is quite complex.
+        expect(other_pm.user.topic_count).to eq(0)
+        expect(other_pm.user.post_count).to eq(0)
+        expect(other_pm_post.user.topic_count).to eq(0)
+        expect(other_pm_post.user.post_count).to eq(0)
 
         expect(topic).to be_valid
         expect(topic.archetype).to eq("regular")
@@ -110,7 +125,7 @@ describe TopicConverter do
     fab!(:author) { Fabricate(:user) }
     fab!(:category) { Fabricate(:category) }
     fab!(:topic) { Fabricate(:topic, user: author, category_id: category.id) }
-    fab!(:post) { Fabricate(:post, topic: topic) }
+    fab!(:post) { Fabricate(:post, topic: topic, user: topic.user) }
 
     context 'success' do
       it "converts regular topic to private message" do
@@ -121,16 +136,39 @@ describe TopicConverter do
         expect(category.reload.topic_count).to eq(0)
       end
 
-      it "updates user stats" do
-        Fabricate(:post, topic: topic, user: author)
+      it "converts unlisted topic to private message" do
+        topic.update_status('visible', false, admin)
+        private_message = topic.convert_to_private_message(post.user)
+
+        expect(private_message).to be_valid
+        expect(topic.archetype).to eq("private_message")
+        expect(topic.category_id).to eq(nil)
+        expect(topic.user.post_count).to eq(0)
+        expect(topic.user.topic_count).to eq(0)
+        expect(category.reload.topic_count).to eq(0)
+      end
+
+      it "updates user stats when converting topic to private message" do
+        _post_2 = Fabricate(:post, topic: topic, user: author)
+        _post_3 = Fabricate(:post, topic: topic, user: author)
+
+        other_topic = Fabricate(:post).topic
+        other_post = Fabricate(:post, topic: other_topic)
+
         topic_user = TopicUser.create!(user_id: author.id, topic_id: topic.id, posted: true)
-        author.user_stat.topic_count = 1
-        author.user_stat.save
-        expect(topic.user.user_stat.topic_count).to eq(1)
-        topic.convert_to_private_message(admin)
+
+        expect do
+          topic.convert_to_private_message(admin)
+        end.to change { author.reload.post_count }.from(2).to(0)
+          .and change { author.reload.topic_count }.from(1).to(0)
+
+        # Ensure query does not affect users from other topics or posts as DB query to update count is quite complex.
+        expect(other_topic.user.post_count).to eq(0)
+        expect(other_topic.user.topic_count).to eq(1)
+        expect(other_post.user.post_count).to eq(1)
+        expect(other_post.user.topic_count).to eq(0)
 
         expect(topic.reload.topic_allowed_users.where(user_id: author.id).count).to eq(1)
-        expect(topic.reload.user.user_stat.topic_count).to eq(0)
         expect(topic_user.reload.notification_level).to eq(TopicUser.notification_levels[:watching])
       end
 
diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb
index cd0f8e5a6c3..e4c5df6eeed 100644
--- a/spec/models/topic_spec.rb
+++ b/spec/models/topic_spec.rb
@@ -1219,7 +1219,11 @@ describe Topic do
   end
 
   context 'update_status' do
-    fab!(:topic) { Fabricate(:topic, bumped_at: 1.hour.ago) }
+    fab!(:post) do
+      Fabricate(:post).tap { |p| p.topic.update!(bumped_at: 1.hour.ago) }
+    end
+
+    fab!(:topic) { post.topic }
 
     before do
       @original_bumped_at = topic.bumped_at
@@ -1243,8 +1247,15 @@ describe Topic do
           topic.update!(category: category)
           Category.update_stats
 
-          expect { topic.update_status('visible', false, @user) }
-            .to change { category.reload.topic_count }.by(-1)
+          expect do
+            2.times { topic.update_status('visible', false, @user) }
+          end.to change { category.reload.topic_count }.by(-1)
+        end
+
+        it 'decreases topic_count of user stat' do
+          expect do
+            2.times { topic.update_status('visible', false, @user) }
+          end.to change { post.user.user_stat.reload.topic_count }.from(1).to(0)
         end
 
         it 'removes itself as featured topic on user profiles' do
@@ -1258,22 +1269,30 @@ describe Topic do
 
       context 'enable' do
         before do
-          topic.update_attribute :visible, false
-          topic.update_status('visible', true, @user)
+          topic.update_status('visible', false, @user)
           topic.reload
         end
 
         it 'should be visible with correct counts' do
+          topic.update_status('visible', true, @user)
+
           expect(topic).to be_visible
-          expect(topic.moderator_posts_count).to eq(1)
+          expect(topic.moderator_posts_count).to eq(2)
           expect(topic.bumped_at).to eq_time(@original_bumped_at)
         end
 
         it 'increases topic_count of topic category' do
-          topic.update!(category: category, visible: false)
+          topic.update!(category: category)
 
-          expect { topic.update_status('visible', true, @user) }
-            .to change { category.reload.topic_count }.by(1)
+          expect do
+            2.times { topic.update_status('visible', true, @user) }
+          end.to change { category.reload.topic_count }.by(1)
+        end
+
+        it 'increases topic_count of user stat' do
+          expect do
+            2.times { topic.update_status('visible', true, @user) }
+          end.to change { post.user.user_stat.reload.topic_count }.from(0).to(1)
         end
       end
     end
diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb
index 1686be0af15..ea82588a501 100644
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@@ -948,7 +948,7 @@ describe PostsController do
         end
 
         it "doesn't enqueue posts when user first creates a topic" do
-          Fabricate(:topic, user: user)
+          topic = Fabricate(:post, user: user).topic
 
           Draft.set(user, "should_clear", 0, "{'a' : 'b'}")
 
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index 03c1526e273..4d7396fd305 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -3563,7 +3563,7 @@ RSpec.describe TopicsController do
 
     describe 'converting public topic to private message' do
       fab!(:topic) { Fabricate(:topic, user: user) }
-      fab!(:post) { Fabricate(:post, user: post_author1, topic: topic) }
+      fab!(:post) { Fabricate(:post, user: user, topic: topic) }
 
       it "raises an error when the user doesn't have permission to convert topic" do
         sign_in(user)
diff --git a/spec/services/user_stat_count_updater_spec.rb b/spec/services/user_stat_count_updater_spec.rb
index 2e60a451103..35fbbf05ff1 100644
--- a/spec/services/user_stat_count_updater_spec.rb
+++ b/spec/services/user_stat_count_updater_spec.rb
@@ -11,6 +11,7 @@ describe UserStatCountUpdater do
   before do
     @orig_logger = Rails.logger
     Rails.logger = @fake_logger = FakeLogger.new
+    SiteSetting.verbose_user_stat_count_logging = true
   end
 
   after do
@@ -21,9 +22,11 @@ describe UserStatCountUpdater do
     UserStatCountUpdater.decrement!(post, user_stat: user_stat)
 
     expect(@fake_logger.warnings.last).to match("topic_count")
+    expect(@fake_logger.warnings.last).to match(post.id.to_s)
 
     UserStatCountUpdater.decrement!(post_2, user_stat: user_stat)
 
     expect(@fake_logger.warnings.last).to match("post_count")
+    expect(@fake_logger.warnings.last).to match(post_2.id.to_s)
   end
 end