DEV: Correct order of triggering topic trashed and recovered event (#19313)

Previously we would trigger the event before the `Topic#deleted_at`
column has been updated making it hard for plugins to correctly work
with the model when its new state has not been persisted in the
database.

app/models/topic.rb

@@ -131,25 +131,35 @@ class Topic < ActiveRecord::Base
   end
 
   def trash!(trashed_by = nil)
+    trigger_event = false
+
     if deleted_at.nil?
       update_category_topic_count_by(-1) if visible?
       CategoryTagStat.topic_deleted(self) if self.tags.present?
-      DiscourseEvent.trigger(:topic_trashed, self)
+      trigger_event = true
     end
+
     super(trashed_by)
+
+    DiscourseEvent.trigger(:topic_trashed, self) if trigger_event
+
     self.topic_embed.trash! if has_topic_embed?
   end
 
   def recover!(recovered_by = nil)
+    trigger_event = false
+
     unless deleted_at.nil?
       update_category_topic_count_by(1) if visible?
       CategoryTagStat.topic_recovered(self) if self.tags.present?
-      DiscourseEvent.trigger(:topic_recovered, self)
+      trigger_event = true
     end
 
     # Note parens are required because superclass doesn't take `recovered_by`
     super()
 
+    DiscourseEvent.trigger(:topic_recovered, self) if trigger_event
+
     unless (topic_embed = TopicEmbed.with_deleted.find_by_topic_id(id)).nil?
       topic_embed.recover!
     end

spec/models/topic_spec.rb

@@ -2392,6 +2392,8 @@ RSpec.describe Topic do
   end
 
   describe 'trash!' do
+    fab!(:topic) { Fabricate(:topic) }
+
     context "with category's topic count" do
       fab!(:category) { Fabricate(:category_with_definition) }
 
@@ -2412,16 +2414,35 @@ RSpec.describe Topic do
     end
 
     it "trashes topic embed record" do
-      topic = Fabricate(:topic)
       post = Fabricate(:post, topic: topic, post_number: 1)
       topic_embed = TopicEmbed.create!(topic_id: topic.id, embed_url: "https://blog.codinghorror.com/password-rules-are-bullshit", post_id: post.id)
       topic.trash!
       topic_embed.reload
       expect(topic_embed.deleted_at).not_to eq(nil)
     end
+
+    it 'triggers the topic trashed event' do
+      events = DiscourseEvent.track_events(:topic_trashed) do
+        topic.trash!
+      end
+
+      expect(events.size).to eq(1)
+    end
+
+    it 'does not trigger the topic trashed event when topic is already trashed' do
+      topic.trash!
+
+      events = DiscourseEvent.track_events(:topic_trashed) do
+        topic.trash!
+      end
+
+      expect(events.size).to eq(0)
+    end
   end
 
   describe 'recover!' do
+    fab!(:topic) { Fabricate(:topic) }
+
     context "with category's topic count" do
       fab!(:category) { Fabricate(:category_with_definition) }
 
@@ -2449,6 +2470,27 @@ RSpec.describe Topic do
       topic_embed.reload
       expect(topic_embed.deleted_at).to be_nil
     end
+
+    it 'triggers the topic recovered event' do
+      topic.trash!
+
+      events = DiscourseEvent.track_events(:topic_recovered) do
+        topic.recover!
+      end
+
+      expect(events.size).to eq(1)
+    end
+
+    it 'does not trigger the topic recovered event when topic is already recovered' do
+      topic.trash!
+      topic.recover!
+
+      events = DiscourseEvent.track_events(:topic_recovered) do
+        topic.recover!
+      end
+
+      expect(events.size).to eq(0)
+    end
   end
 
   describe "new user limits" do