FIX: Destroy all posts when hard deleting topic (#17359)

Hard deleting topics that contained soft deleted posts or small actions
used to create orphan posts because only the first post was hard
deleted. This commit adds an error message if there are still posts left
in the topic that must be hard deleted first or hard deletes all small
actions too immediately (there is no other way of hard deleting a small
action because there is no wrench menu).

app/controllers/posts_controller.rb

@@ -321,14 +321,12 @@ class PostsController < ApplicationController
 
   def destroy
     post = find_post_from_params
+    force_destroy = ActiveModel::Type::Boolean.new.cast(params[:force_destroy])
 
-    force_destroy = false
-    if params[:force_destroy].present?
+    if force_destroy
       if !guardian.can_permanently_delete?(post)
         return render_json_error post.cannot_permanently_delete_reason(current_user), status: 403
       end
-
-      force_destroy = true
     else
       guardian.ensure_can_delete!(post)
     end
@@ -338,8 +336,12 @@ class PostsController < ApplicationController
       RateLimiter.new(current_user, "delete_post_per_day", SiteSetting.max_post_deletions_per_day, 1.day).performed!
     end
 
-    destroyer = PostDestroyer.new(current_user, post, context: params[:context], force_destroy: force_destroy)
-    destroyer.destroy
+    PostDestroyer.new(
+      current_user,
+      post,
+      context: params[:context],
+      force_destroy: force_destroy
+    ).destroy
 
     render body: nil
   end

app/controllers/topics_controller.rb

@@ -621,20 +621,22 @@ class TopicsController < ApplicationController
 
   def destroy
     topic = Topic.with_deleted.find_by(id: params[:id])
+    force_destroy = ActiveModel::Type::Boolean.new.cast(params[:force_destroy])
 
-    force_destroy = false
-    if params[:force_destroy].present?
+    if force_destroy
       if !guardian.can_permanently_delete?(topic)
         return render_json_error topic.cannot_permanently_delete_reason(current_user), status: 403
       end
-
-      force_destroy = true
     else
       guardian.ensure_can_delete!(topic)
     end
 
-    first_post = topic.posts.with_deleted.order(:post_number).first
-    PostDestroyer.new(current_user, first_post, context: params[:context], force_destroy: force_destroy).destroy
+    PostDestroyer.new(
+      current_user,
+      topic.ordered_posts.with_deleted.first,
+      context: params[:context],
+      force_destroy: force_destroy
+    ).destroy
 
     render body: nil
   rescue Discourse::InvalidAccess

app/models/topic.rb

@@ -1786,7 +1786,12 @@ class Topic < ActiveRecord::Base
   end
 
   def cannot_permanently_delete_reason(user)
-    if self.posts_count > 0
+    all_posts_count = Post.with_deleted
+      .where(topic_id: self.id)
+      .where(post_type: [Post.types[:regular], Post.types[:moderator_action], Post.types[:whisper]])
+      .count
+
+    if posts_count > 0 || all_posts_count > 1
       I18n.t('post.cannot_permanently_delete.many_posts')
     elsif self.deleted_by_id == user&.id && self.deleted_at >= Post::PERMANENT_DELETE_TIMER.ago
       time_left = RateLimiter.time_left(Post::PERMANENT_DELETE_TIMER.to_i - Time.zone.now.to_i + self.deleted_at.to_i)

lib/guardian/topic_guardian.rb

@@ -158,7 +158,20 @@ module TopicGuardian
   def can_permanently_delete_topic?(topic)
     return false if !SiteSetting.can_permanently_delete
     return false if !topic
+
+    # Ensure that all posts (including small actions) are at least soft
+    # deleted.
     return false if topic.posts_count > 0
+
+    # All other posts that were deleted still must be permanently deleted
+    # before the topic can be deleted with the exception of small action
+    # posts that will be deleted right before the topic is.
+    all_posts_count = Post.with_deleted
+      .where(topic_id: topic.id)
+      .where(post_type: [Post.types[:regular], Post.types[:moderator_action], Post.types[:whisper]])
+      .count
+    return false if all_posts_count > 1
+
     return false if !is_admin? || !can_see_topic?(topic)
     return false if !topic.deleted_at
     return false if topic.deleted_by_id == @user.id && topic.deleted_at >= Post::PERMANENT_DELETE_TIMER.ago

lib/post_destroyer.rb

@@ -145,6 +145,14 @@ class PostDestroyer
   # show up in the topic
   # Permanent option allows to hard delete.
   def perform_delete
+    # 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|
+        PostDestroyer.new(@user, post, @opts).destroy if post.id != @post.id
+      end
+    end
+
     Post.transaction do
       permanent? ? @post.destroy! : @post.trash!(@user)
       if @post.topic

spec/models/topic_spec.rb

@@ -3030,6 +3030,9 @@ RSpec.describe Topic do
       expect(topic.reload.cannot_permanently_delete_reason(Fabricate(:admin))).to eq(I18n.t('post.cannot_permanently_delete.many_posts'))
 
       PostDestroyer.new(admin, post_2.reload).destroy
+      expect(topic.reload.cannot_permanently_delete_reason(Fabricate(:admin))).to eq(I18n.t('post.cannot_permanently_delete.many_posts'))
+
+      PostDestroyer.new(admin, post_2.reload, force_destroy: true).destroy
       expect(topic.reload.cannot_permanently_delete_reason(Fabricate(:admin))).to eq(nil)
     end
 

spec/requests/topics_controller_spec.rb

@@ -1188,6 +1188,48 @@ RSpec.describe TopicsController do
         end
       end
     end
+
+    describe 'force destroy' do
+      fab!(:post) { Fabricate(:post, topic: topic, post_number: 1) }
+
+      before do
+        SiteSetting.can_permanently_delete = true
+
+        sign_in(admin)
+      end
+
+      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
+
+        delete "/t/#{topic.id}.json", params: { force_destroy: true }
+
+        expect(response.status).to eq(200)
+
+        expect(Topic.find_by(id: topic.id)).to eq(nil)
+        expect(Post.find_by(id: post.id)).to eq(nil)
+        expect(Post.find_by(id: small_action_post.id)).to eq(nil)
+      end
+
+      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
+
+        delete "/t/#{topic.id}.json", params: { force_destroy: true }
+
+        expect(response.status).to eq(403)
+      end
+
+      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
+
+        delete "/t/#{topic.id}.json", params: { force_destroy: true }
+
+        expect(response.status).to eq(403)
+      end
+    end
   end
 
   describe '#id_for_slug' do