From fad1fac196df5d9fc38e5f294367a42e28efcef4 Mon Sep 17 00:00:00 2001
From: Bianca Nenciu <nbianca@users.noreply.github.com>
Date: Tue, 16 Feb 2021 17:45:12 +0200
Subject: [PATCH] FIX: Update topic_count when updating visibility (#11946)
Updating a topic's visibility did not increase or decrease the
topic_count of a category, but Category.update_stats does ignore
unlisted topics which resulted in inconsistencies when deleting
such topics.
---
app/models/topic.rb | 9 +++++----
app/services/topic_status_updater.rb | 4 ++++
spec/models/topic_spec.rb | 27 +++++++++++++++++++++++++++
3 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 90e37ca8797..b4d1080eebf 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -132,7 +132,7 @@ class Topic < ActiveRecord::Base
def trash!(trashed_by = nil)
if deleted_at.nil?
- update_category_topic_count_by(-1)
+ update_category_topic_count_by(-1) if visible?
CategoryTagStat.topic_deleted(self) if self.tags.present?
DiscourseEvent.trigger(:topic_trashed, self)
end
@@ -142,7 +142,7 @@ class Topic < ActiveRecord::Base
def recover!(recovered_by = nil)
unless deleted_at.nil?
- update_category_topic_count_by(1)
+ update_category_topic_count_by(1) if visible?
CategoryTagStat.topic_recovered(self) if self.tags.present?
DiscourseEvent.trigger(:topic_recovered, self)
end
@@ -1648,8 +1648,9 @@ class Topic < ActiveRecord::Base
def update_category_topic_count_by(num)
if category_id.present?
Category
- .where(['id = ?', category_id])
- .update_all("topic_count = topic_count " + (num > 0 ? '+' : '') + "#{num}")
+ .where('id = ?', category_id)
+ .where('topic_id != ? OR topic_id IS NULL', self.id)
+ .update_all("topic_count = topic_count + #{num.to_i}")
end
end
diff --git a/app/services/topic_status_updater.rb b/app/services/topic_status_updater.rb
index 1e93cea3c76..1013c2d0dc9 100644
--- a/app/services/topic_status_updater.rb
+++ b/app/services/topic_status_updater.rb
@@ -46,6 +46,10 @@ TopicStatusUpdater = Struct.new(:topic, :user) do
UserProfile.remove_featured_topic_from_all_profiles(topic)
end
+ if status.visible?
+ topic.update_category_topic_count_by(status.enabled? ? 1 : -1)
+ end
+
if @topic_timer
if status.manually_closing_topic? || status.closing_topic?
topic.delete_topic_timer(TopicTimer.types[:close])
diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb
index bdde75489a6..822e7b8da6d 100644
--- a/spec/models/topic_spec.rb
+++ b/spec/models/topic_spec.rb
@@ -1112,6 +1112,8 @@ describe Topic do
end
context 'visibility' do
+ let(:category) { Fabricate(:category_with_definition) }
+
context 'disable' do
it 'should not be visible and have correct counts' do
topic.update_status('visible', false, @user)
@@ -1121,6 +1123,14 @@ describe Topic do
expect(topic.bumped_at).to eq_time(@original_bumped_at)
end
+ it 'decreases topic_count of topic category' do
+ topic.update!(category: category)
+ Category.update_stats
+
+ expect { topic.update_status('visible', false, @user) }
+ .to change { category.reload.topic_count }.by(-1)
+ end
+
it 'removes itself as featured topic on user profiles' do
user.user_profile.update(featured_topic_id: topic.id)
expect(user.user_profile.featured_topic).to eq(topic)
@@ -1142,6 +1152,13 @@ describe Topic do
expect(topic.moderator_posts_count).to eq(1)
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)
+
+ expect { topic.update_status('visible', true, @user) }
+ .to change { category.reload.topic_count }.by(1)
+ end
end
end
@@ -2144,6 +2161,11 @@ describe Topic do
topic = Fabricate(:topic, category: category, deleted_at: 1.day.ago)
expect { topic.trash!(moderator) }.to_not change { category.reload.topic_count }
end
+
+ it "doesn't subtract 1 if topic is unlisted" do
+ topic = Fabricate(:topic, category: category, visible: false)
+ expect { topic.trash!(moderator) }.to_not change { category.reload.topic_count }
+ end
end
it "trashes topic embed record" do
@@ -2169,6 +2191,11 @@ describe Topic do
topic = Fabricate(:topic, category: category)
expect { topic.recover! }.to_not change { category.reload.topic_count }
end
+
+ it "doesn't add 1 if topic is not visible" do
+ topic = Fabricate(:topic, category: category, visible: false)
+ expect { topic.recover! }.to_not change { category.reload.topic_count }
+ end
end
it "recovers topic embed record" do