DEV: Don't allow context-free system post destruction (#32523)

There are instances of posts being deleted by system_user where the context is left blank in the staff action logs, leading to confusion about why exactly they have been deleted.

This change deprecates using the PostDestroyer as system_user without providing a context, and adds a context to all call sites currently missing it in core. Plugins to be done after this is merged.

app/controllers/topics_controller.rb

@@ -697,7 +697,7 @@ class TopicsController < ApplicationController
     PostDestroyer.new(
       current_user,
       topic.ordered_posts.with_deleted.first,
-      context: params[:context],
+      context: params[:context].presence || I18n.t("staff_action_logs.api_post_delete"),
       force_destroy: force_destroy,
     ).destroy
 

app/services/destroy_task.rb

@@ -21,7 +21,11 @@ class DestroyTask
       @io.puts "Deleting #{topic.slug}..."
       first_post = topic.ordered_posts.first
       return @io.puts "Topic.ordered_posts.first was nil" if first_post.nil?
-      @io.puts PostDestroyer.new(Discourse.system_user, first_post).destroy
+      @io.puts PostDestroyer.new(
+                 Discourse.system_user,
+                 first_post,
+                 context: I18n.t("staff_action_logs.cli_bulk_post_delete"),
+               ).destroy
     end
   end
 
@@ -37,7 +41,11 @@ class DestroyTask
     topics.find_each do |topic|
       first_post = topic.ordered_posts.first
       return @io.puts "Topic.ordered_posts.first was nil for topic: #{topic.id}" if first_post.nil?
-      PostDestroyer.new(Discourse.system_user, first_post).destroy
+      PostDestroyer.new(
+        Discourse.system_user,
+        first_post,
+        context: I18n.t("staff_action_logs.cli_bulk_post_delete"),
+      ).destroy
     end
     topics = Topic.where(category_id: c.id, pinned_at: nil)
     @io.puts "There are #{topics.count} topics that could not be deleted in #{c.slug} category"
@@ -97,7 +105,11 @@ class DestroyTask
       .find_each do |pm|
         @io.puts "Destroying #{pm.slug} pm"
         first_post = pm.ordered_posts.first
-        @io.puts PostDestroyer.new(Discourse.system_user, first_post).destroy
+        @io.puts PostDestroyer.new(
+                   Discourse.system_user,
+                   first_post,
+                   context: I18n.t("staff_action_logs.cli_bulk_post_delete"),
+                 ).destroy
       end
   end
 

app/services/group_message.rb

@@ -70,7 +70,13 @@ class GroupMessage
         )
     end
 
-    posts.find_each { |post| PostDestroyer.new(Discourse.system_user, post).destroy }
+    posts.find_each do |post|
+      PostDestroyer.new(
+        Discourse.system_user,
+        post,
+        context: I18n.t("staff_action_logs.reminder_deleted"),
+      ).destroy
+    end
   end
 
   def message_params

app/services/user_destroyer.rb

@@ -156,7 +156,11 @@ class UserDestroyer
       if post.is_first_post? && category_topic_ids.include?(post.topic_id)
         post.update!(user: Discourse.system_user)
       else
-        PostDestroyer.new(@actor.staff? ? @actor : Discourse.system_user, post).destroy
+        PostDestroyer.new(
+          @actor.staff? ? @actor : Discourse.system_user,
+          post,
+          context: I18n.t("staff_action_logs.user_associated_posts_deleted"),
+        ).destroy
       end
 
       if post.topic && post.is_first_post?

config/locales/server.en.yml

@@ -5588,6 +5588,14 @@ en:
       restored: Restored
     bulk_user_delete: "deleted in a bulk delete operation"
     cli_bulk_post_delete: "Bulk deleted from rake task"
+    api_post_delete: "Deleted via API"
+    reminder_deleted: "Replaced by new reminder"
+    user_associated_posts_deleted: "User was destroyed"
+    imap_email_deleted: "Email has been deleted on the IMAP server"
+    imap_email_marked_as_spam: "Email has been moved to spam on the IMAP server"
+    auto_deleted_hidden_posts: "Automatically destroyed hidden posts"
+    seed_data_topic_updated: "Seed data topic updated"
+    seed_data_topic_deleted: "Seed data topic deleted"
 
   reviewables:
     already_handled: "Thanks, but we've already reviewed that post and determined it does not need to be flagged again."

lib/imap/sync.rb

@@ -225,7 +225,11 @@ module Imap
             "Deleting post ID #{incoming.post_id}, topic id #{incoming.topic_id}; email has been deleted on the IMAP server.",
             @group,
           )
-          PostDestroyer.new(Discourse.system_user, incoming.post).destroy
+          PostDestroyer.new(
+            Discourse.system_user,
+            incoming.post,
+            context: I18n.t("staff_action_logs.imap_email_deleted"),
+          ).destroy
         end
 
         # the email has moved mailboxes, we don't want to try trashing again next time
@@ -266,7 +270,11 @@ module Imap
             "Deleting post ID #{incoming.post_id}, topic id #{incoming.topic_id}; email has been moved to spam on the IMAP server.",
             @group,
           )
-          PostDestroyer.new(Discourse.system_user, incoming.post).destroy
+          PostDestroyer.new(
+            Discourse.system_user,
+            incoming.post,
+            context: I18n.t("staff_action_logs.imap_email_marked_as_spam"),
+          ).destroy
         end
 
         # the email has moved mailboxes, we don't want to try marking as spam again next time

lib/post_destroyer.rb

@@ -9,7 +9,13 @@ class PostDestroyer
     Post
       .where(deleted_at: nil, hidden: true)
       .where("hidden_at < ?", 30.days.ago)
-      .find_each { |post| PostDestroyer.new(Discourse.system_user, post).destroy }
+      .find_each do |post|
+        PostDestroyer.new(
+          Discourse.system_user,
+          post,
+          context: "Automatically destroyed hidden posts",
+        ).destroy
+      end
   end
 
   def self.destroy_stubs
@@ -57,6 +63,12 @@ class PostDestroyer
     @post = post
     @topic = post.topic || Topic.with_deleted.find_by(id: @post.topic_id)
     @opts = opts
+
+    if user == Discourse.system_user && opts[:context].blank?
+      Discourse.deprecate(<<~WARNING, drop_from: "3.6.0", output_in_test: true)
+        Using PostDestroyer as system user without providing a context will be an error in future versions.
+      WARNING
+    end
   end
 
   def destroy

lib/seed_data/topics.rb

@@ -210,7 +210,11 @@ module SeedData
 
       if !skip_changed || unchanged?(post)
         if post.trashed?
-          PostDestroyer.new(Discourse.system_user, post).recover
+          PostDestroyer.new(
+            Discourse.system_user,
+            post,
+            context: I18n.t("staff_action_logs.seed_data_topic_updated"),
+          ).recover
           post.reload
         end
 
@@ -226,7 +230,13 @@ module SeedData
       post = find_post(site_setting_name)
       return if !post
 
-      PostDestroyer.new(Discourse.system_user, post).destroy if !skip_changed || unchanged?(post)
+      if !skip_changed || unchanged?(post)
+        PostDestroyer.new(
+          Discourse.system_user,
+          post,
+          context: I18n.t("staff_action_logs.seed_data_topic_deleted"),
+        ).destroy
+      end
     end
 
     def find_post(site_setting_name, deleted: false)

spec/lib/imap/sync_spec.rb

@@ -373,7 +373,11 @@ RSpec.describe Imap::Sync do
         provider.stubs(:uids).with(to: 100).returns([100])
         provider.stubs(:uids).with(from: 101).returns([])
 
-        PostDestroyer.new(Discourse.system_user, incoming_100.post).destroy
+        PostDestroyer.new(
+          Discourse.system_user,
+          incoming_100.post,
+          context: "Automated testing",
+        ).destroy
         provider
           .stubs(:emails)
           .with([100], %w[UID FLAGS LABELS ENVELOPE], anything)

spec/lib/post_destroyer_spec.rb

@@ -318,7 +318,7 @@ RSpec.describe PostDestroyer do
             end
 
             def changes_deleted_at_to_nil
-              PostDestroyer.new(Discourse.system_user, @reply).destroy
+              PostDestroyer.new(Discourse.system_user, @reply, context: "Automated testing").destroy
               @reply.reload
               expect(@reply.user_deleted).to eq(false)
               expect(@reply.deleted_at).not_to eq(nil)
@@ -925,7 +925,7 @@ RSpec.describe PostDestroyer do
 
     it "should not send the flags_agreed_and_post_deleted message if it was deleted by system" do
       expect(ReviewableFlaggedPost.pending.count).to eq(1)
-      PostDestroyer.new(Discourse.system_user, second_post).destroy
+      PostDestroyer.new(Discourse.system_user, second_post, context: "Automated testing").destroy
       expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
       expect(ReviewableFlaggedPost.pending.count).to eq(0)
     end
@@ -1065,7 +1065,7 @@ RSpec.describe PostDestroyer do
     end
 
     fab!(:post)
-    let(:reporter) { Discourse.system_user }
+    let(:reporter) { Fabricate(:moderator) }
     let(:reply) { Fabricate(:post, topic: post.topic) }
     let(:reviewable_reply) { PostActionCreator.off_topic(reporter, reply).reviewable }
 

spec/models/post_spec.rb

@@ -1933,9 +1933,13 @@ RSpec.describe Post do
           create_post(topic_id: topic.id, post_type: Post.types[:whisper])
         end
 
-      updates_topic_updated_at { PostDestroyer.new(Discourse.system_user, post).destroy }
+      updates_topic_updated_at do
+        PostDestroyer.new(Discourse.system_user, post, context: "Automated testing").destroy
+      end
 
-      updates_topic_updated_at { PostDestroyer.new(Discourse.system_user, post).recover }
+      updates_topic_updated_at do
+        PostDestroyer.new(Discourse.system_user, post, context: "Automated testing").recover
+      end
     end
   end
 

spec/requests/reviewable_claimed_topics_controller_spec.rb

@@ -74,7 +74,7 @@ RSpec.describe ReviewableClaimedTopicsController do
 
       it "works with deleted topics" do
         first_post = topic.first_post || Fabricate(:post, topic: topic)
-        PostDestroyer.new(Discourse.system_user, first_post).destroy
+        PostDestroyer.new(Discourse.system_user, first_post, context: "Automated testing").destroy
 
         post "/reviewable_claimed_topics.json", params: params
 
@@ -177,7 +177,7 @@ RSpec.describe ReviewableClaimedTopicsController do
     it "works with deleted topics" do
       SiteSetting.reviewable_claiming = "optional"
       first_post = topic.first_post || Fabricate(:post, topic: topic)
-      PostDestroyer.new(Discourse.system_user, first_post).destroy
+      PostDestroyer.new(Discourse.system_user, first_post, context: "Automated testing").destroy
 
       delete "/reviewable_claimed_topics/#{claimed.topic_id}.json"
 

spec/requests/topics_controller_spec.rb

@@ -182,7 +182,7 @@ RSpec.describe TopicsController do
 
         describe "when topic has been deleted" do
           it "should still be able to move posts" do
-            PostDestroyer.new(admin, topic.first_post).destroy
+            PostDestroyer.new(admin, topic.first_post, context: "Automated testing").destroy
 
             expect(topic.reload.deleted_at).to_not be_nil
 
@@ -1562,8 +1562,12 @@ RSpec.describe TopicsController do
 
       it "force destroys all deleted small actions in topic too" do
         small_action_post = Fabricate(:small_action, topic: topic)
-        PostDestroyer.new(Discourse.system_user, post).destroy
-        PostDestroyer.new(Discourse.system_user, small_action_post).destroy
+        PostDestroyer.new(Discourse.system_user, post, context: "Automated testing").destroy
+        PostDestroyer.new(
+          Discourse.system_user,
+          small_action_post,
+          context: "Automated testing",
+        ).destroy
 
         delete "/t/#{topic.id}.json", params: { force_destroy: true }
 
@@ -1576,8 +1580,12 @@ RSpec.describe TopicsController do
 
       it "creates a log and clean up previously recorded sensitive information" do
         small_action_post = Fabricate(:small_action, topic: topic)
-        PostDestroyer.new(Discourse.system_user, post).destroy
-        PostDestroyer.new(Discourse.system_user, small_action_post).destroy
+        PostDestroyer.new(Discourse.system_user, post, context: "Automated testing").destroy
+        PostDestroyer.new(
+          Discourse.system_user,
+          small_action_post,
+          context: "Automated testing",
+        ).destroy
 
         delete "/t/#{topic.id}.json", params: { force_destroy: true }
 
@@ -1595,7 +1603,7 @@ RSpec.describe TopicsController do
 
       it "does not allow to destroy topic if not all posts were force destroyed" do
         _other_post = Fabricate(:post, topic: topic, post_number: 2)
-        PostDestroyer.new(Discourse.system_user, post).destroy
+        PostDestroyer.new(Discourse.system_user, post, context: "Automated testing").destroy
 
         delete "/t/#{topic.id}.json", params: { force_destroy: true }
 
@@ -1604,7 +1612,11 @@ RSpec.describe TopicsController do
 
       it "does not allow to destroy topic if not all small action posts were deleted" do
         small_action_post = Fabricate(:small_action, topic: topic)
-        PostDestroyer.new(Discourse.system_user, small_action_post).destroy
+        PostDestroyer.new(
+          Discourse.system_user,
+          small_action_post,
+          context: "Automated testing",
+        ).destroy
 
         delete "/t/#{topic.id}.json", params: { force_destroy: true }
 

spec/serializers/topic_view_details_serializer_spec.rb

@@ -30,7 +30,9 @@ RSpec.describe TopicViewDetailsSerializer do
 
   describe "#can_permanently_delete" do
     let(:post) do
-      Fabricate(:post).tap { |post| PostDestroyer.new(Discourse.system_user, post).destroy }
+      Fabricate(:post).tap do |post|
+        PostDestroyer.new(Discourse.system_user, post, context: "Automated testing").destroy
+      end
     end
 
     before { SiteSetting.can_permanently_delete = true }

spec/serializers/web_hook_post_serializer_spec.rb

@@ -65,7 +65,7 @@ RSpec.describe WebHookPostSerializer do
 
   it "should only include deleted topic title for staffs" do
     topic = post.topic
-    PostDestroyer.new(Discourse.system_user, post).destroy
+    PostDestroyer.new(Discourse.system_user, post, context: "Automated testing").destroy
     post.reload
 
     [nil, post.user, Fabricate(:user)].each do |user|

spec/services/username_changer_spec.rb

@@ -561,7 +561,7 @@ RSpec.describe UsernameChanger do
           it "replaces the username in quote tags when the post is deleted" do
             post =
               create_post_and_change_username(raw: raw) do |p|
-                PostDestroyer.new(Discourse.system_user, p).destroy
+                PostDestroyer.new(Discourse.system_user, p, context: "Automated testing").destroy
               end
 
             expect(post.raw).to eq(expected_raw)

spec/system/topic_page_spec.rb

@@ -57,8 +57,8 @@ describe "Topic page", type: :system do
       post3 = Fabricate(:post, topic: topic, cooked: "post3")
       post4 = Fabricate(:post, topic: topic, cooked: "post4")
 
-      PostDestroyer.new(Discourse.system_user, post2).destroy
-      PostDestroyer.new(Discourse.system_user, post3).destroy
+      PostDestroyer.new(Discourse.system_user, post2, context: "Automated testing").destroy
+      PostDestroyer.new(Discourse.system_user, post3, context: "Automated testing").destroy
 
       sign_in admin
     end