From 40cee37bcc694cf6c3905ac2918b4bbcd45c6af3 Mon Sep 17 00:00:00 2001
From: Robin Ward <robin.ward@gmail.com>
Date: Fri, 7 Apr 2017 17:10:01 -0400
Subject: [PATCH] FIX: Don't insert topic status messages unless the status
changes
---
app/services/topic_status_updater.rb | 27 ++++++++---
spec/models/topic_spec.rb | 24 ++++++----
spec/services/topic_status_updater_spec.rb | 56 ++++++++++++++++++++++
3 files changed, 91 insertions(+), 16 deletions(-)
diff --git a/app/services/topic_status_updater.rb b/app/services/topic_status_updater.rb
index 5bd72a7b4bb..795751a0d79 100644
--- a/app/services/topic_status_updater.rb
+++ b/app/services/topic_status_updater.rb
@@ -4,23 +4,36 @@ TopicStatusUpdater = Struct.new(:topic, :user) do
@topic_status_update = topic.topic_status_update
+ updated = nil
Topic.transaction do
- change(status, opts)
- highest_post_number = topic.highest_post_number
- create_moderator_post_for(status, opts[:message])
- update_read_state_for(status, highest_post_number)
+ updated = change(status, opts)
+ if updated
+ highest_post_number = topic.highest_post_number
+ create_moderator_post_for(status, opts[:message])
+ update_read_state_for(status, highest_post_number)
+ end
end
+
+ updated
end
private
def change(status, opts={})
+ result = true
+
if status.pinned? || status.pinned_globally?
topic.update_pinned(status.enabled?, status.pinned_globally?, opts[:until])
elsif status.autoclosed?
- topic.update_column('closed', status.enabled?)
+ rc = Topic.where(id: topic.id, closed: !status.enabled?).update_all(closed: status.enabled?)
+ topic.closed = status.enabled?
+ result = false if rc == 0
else
- topic.update_column(status.name, status.enabled?)
+ rc = Topic.where(:id => topic.id, status.name => !status.enabled)
+ .update_all(status.name => status.enabled?)
+
+ topic.send("#{status.name}=", status.enabled?)
+ result = false if rc == 0
end
if @topic_status_update
@@ -38,6 +51,8 @@ TopicStatusUpdater = Struct.new(:topic, :user) do
(status.disabled? && status.visible?))
CategoryFeaturedTopic.where(topic_id: topic.id).delete_all
end
+
+ result
end
def create_moderator_post_for(status, message=nil)
diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb
index a25f4db326b..5feef5180db 100644
--- a/spec/models/topic_spec.rb
+++ b/spec/models/topic_spec.rb
@@ -689,14 +689,16 @@ describe Topic do
context 'archived' do
context 'disable' do
before do
- @topic.update_status('archived', false, @user)
- @topic.reload
+ @archived_topic = Fabricate(:topic, archived: true, bumped_at: 1.hour.ago)
+ @original_bumped_at = @archived_topic.bumped_at.to_f
+ @archived_topic.update_status('archived', false, @user)
+ @archived_topic.reload
end
it 'should archive correctly' do
- expect(@topic).not_to be_archived
- expect(@topic.bumped_at.to_f).to eq(@original_bumped_at)
- expect(@topic.moderator_posts_count).to eq(1)
+ expect(@archived_topic).not_to be_archived
+ expect(@archived_topic.bumped_at.to_f).to eq(@original_bumped_at)
+ expect(@archived_topic.moderator_posts_count).to eq(1)
end
end
@@ -719,14 +721,16 @@ describe Topic do
shared_examples_for 'a status that closes a topic' do
context 'disable' do
before do
- @topic.update_status(status, false, @user)
- @topic.reload
+ @closed_topic = Fabricate(:topic, closed: true, bumped_at: 1.hour.ago)
+ @original_bumped_at = @closed_topic.bumped_at.to_f
+ @closed_topic.update_status(status, false, @user)
+ @closed_topic.reload
end
it 'should not be pinned' do
- expect(@topic).not_to be_closed
- expect(@topic.moderator_posts_count).to eq(1)
- expect(@topic.bumped_at.to_f).not_to eq(@original_bumped_at)
+ expect(@closed_topic).not_to be_closed
+ expect(@closed_topic.moderator_posts_count).to eq(1)
+ expect(@closed_topic.bumped_at.to_f).not_to eq(@original_bumped_at)
end
end
diff --git a/spec/services/topic_status_updater_spec.rb b/spec/services/topic_status_updater_spec.rb
index 3609082ec64..17f3be231d6 100644
--- a/spec/services/topic_status_updater_spec.rb
+++ b/spec/services/topic_status_updater_spec.rb
@@ -56,4 +56,60 @@ describe TopicStatusUpdater do
expect(last_post.raw).to eq(I18n.t("topic_statuses.autoclosed_enabled_lastpost_hours", count: 10))
end
+ describe "repeat actions" do
+
+ shared_examples "an action that doesn't repeat" do
+ it "does not perform the update twice" do
+ topic = Fabricate(:topic, status_name => false)
+ updated = TopicStatusUpdater.new(topic, admin).update!(status_name, true)
+ expect(updated).to eq(true)
+ expect(topic.send("#{status_name}?")).to eq(true)
+
+ updated = TopicStatusUpdater.new(topic, admin).update!(status_name, true)
+ expect(updated).to eq(false)
+ expect(topic.posts.where(post_type: Post.types[:small_action]).count).to eq(1)
+
+ updated = TopicStatusUpdater.new(topic, admin).update!(status_name, false)
+ expect(updated).to eq(true)
+ expect(topic.send("#{status_name}?")).to eq(false)
+
+ updated = TopicStatusUpdater.new(topic, admin).update!(status_name, false)
+ expect(updated).to eq(false)
+ expect(topic.posts.where(post_type: Post.types[:small_action]).count).to eq(2)
+ end
+
+ end
+
+ it_behaves_like "an action that doesn't repeat" do
+ let(:status_name) { "closed" }
+ end
+
+ it_behaves_like "an action that doesn't repeat" do
+ let(:status_name) { "visible" }
+ end
+
+ it_behaves_like "an action that doesn't repeat" do
+ let(:status_name) { "archived" }
+ end
+
+ it "updates autoclosed" do
+ topic = Fabricate(:topic)
+ updated = TopicStatusUpdater.new(topic, admin).update!('autoclosed', true)
+ expect(updated).to eq(true)
+ expect(topic.closed?).to eq(true)
+
+ updated = TopicStatusUpdater.new(topic, admin).update!('autoclosed', true)
+ expect(updated).to eq(false)
+ expect(topic.posts.where(post_type: Post.types[:small_action]).count).to eq(1)
+
+ updated = TopicStatusUpdater.new(topic, admin).update!('autoclosed', false)
+ expect(updated).to eq(true)
+ expect(topic.closed?).to eq(false)
+
+ updated = TopicStatusUpdater.new(topic, admin).update!('autoclosed', false)
+ expect(updated).to eq(false)
+ expect(topic.posts.where(post_type: Post.types[:small_action]).count).to eq(2)
+ end
+
+ end
end