DEV: Destroy posts rake task enhancements (#31739)

This improves the new rake task to bulk delete posts based on feedback:

- Honour the `can_permanently_delete` site setting.
- Fix a warning about a scope order no being applied due to batching.
- Fix an issue where double delete would result in duplicate user
histories.

app/services/destroy_task.rb

@@ -49,9 +49,19 @@ class DestroyTask
   end
 
   def destroy_posts(post_ids, require_confirmation: true)
+    if !SiteSetting.can_permanently_delete
+      @io.puts "The can_permanently_delete site setting needs to be enabled to destroy posts."
+      return
+    end
+
     posts = Post.with_deleted.where(id: post_ids)
 
-    @io.puts "There are #{posts.count} posts to delete"
+    @io.puts(<<~NOTICE)
+      There are #{posts.count} posts to delete.
+
+      Note: If a post is the first post in a topic, all posts
+            in that topic will be deleted.
+    NOTICE
 
     if posts.count < post_ids.size
       @io.puts "Couldn't find the following posts:"
@@ -63,14 +73,21 @@ class DestroyTask
       exit 1 if confirm_destroy.downcase != "y"
     end
 
-    posts.find_each do |post|
-      @io.puts "Destroying post #{post.id}"
-      @io.puts PostDestroyer.new(
-                 Discourse.system_user,
-                 post,
-                 context: I18n.t("staff_action_logs.cli_bulk_post_delete"),
-                 force_destroy: true,
-               ).destroy
+    # Do first posts before the rest. The PostDestroyer will destroy
+    # all posts following a first post automatically, and we want to
+    # avoid duplicate audit trail entries by trying to delete them
+    # again. Note: Rails batching will order by pkey (post ID in this
+    # case) internally, and can not accept additional ordering options.
+    [posts.where(post_number: 1), posts].each do |scope|
+      scope.find_each(order: :desc) do |post|
+        @io.puts "Destroying post #{post.id}"
+        @io.puts PostDestroyer.new(
+                   Discourse.system_user,
+                   post,
+                   context: I18n.t("staff_action_logs.cli_bulk_post_delete"),
+                   force_destroy: true,
+                 ).destroy
+      end
     end
   end
 

lib/post_destroyer.rb

@@ -166,7 +166,7 @@ class PostDestroyer
     # All posts in the topic must be force deleted if the first is force
     # deleted (except @post which is destroyed by current instance).
     if @topic && @post.is_first_post? && permanent?
-      @topic.ordered_posts.with_deleted.reverse_order.find_each do |post|
+      @topic.posts.with_deleted.find_each do |post|
         PostDestroyer.new(@user, post, @opts).destroy if post.id != @post.id
       end
     end

spec/services/destroy_task_spec.rb

@@ -76,18 +76,36 @@ RSpec.describe DestroyTask do
     let!(:p1) { Fabricate(:post, topic: t1) }
     let!(:p2) { Fabricate(:post, topic: t1) }
     let!(:p3) { Fabricate(:post, topic: t2) }
+    let!(:p4) { Fabricate(:post, topic: t2) }
 
     before { p2.trash! }
 
-    it "destroys posts listed and creates staff action logs" do
-      expect { task.destroy_posts([p2.id, p3.id], require_confirmation: false) }.to change {
-        Post.with_deleted.count
-      }.by(-2).and change { UserHistory.pluck(:action) }.from([]).to(
-              [
-                UserHistory.actions[:delete_post_permanently],
-                UserHistory.actions[:delete_topic_permanently],
-              ],
-            )
+    context "when permanent deletion is enabled" do
+      it "destroys posts listed and creates staff action logs" do
+        SiteSetting.can_permanently_delete = true
+
+        expect {
+          task.destroy_posts([p2.id, p3.id, p4.id], require_confirmation: false)
+        }.to change { Post.with_deleted.count }.by(-3).and change {
+                UserHistory.pluck(:post_id, :topic_id, :action)
+              }.from([]).to(
+                contain_exactly(
+                  [p2.id, nil, UserHistory.actions[:delete_post_permanently]],
+                  [nil, t2.id, UserHistory.actions[:delete_topic_permanently]],
+                  [p4.id, nil, UserHistory.actions[:delete_post_permanently]],
+                ),
+              )
+      end
+    end
+
+    context "when permanent deletion is disabled in site settings" do
+      it "does not do anything" do
+        SiteSetting.can_permanently_delete = false
+
+        expect { task.destroy_posts([p2.id, p3.id], require_confirmation: false) }.not_to change {
+          Post.with_deleted.count
+        }
+      end
     end
   end