FIX: Don't insert topic status messages unless the status changes

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)

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

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