FEATURE: Deleting a user with their posts also deletes chat messages. (#19194)

This commit introduce a new API for registering callbacks, which we'll execute when a user gets destroyed, and the `delete_posts` opt is true. The chat plugin registers one callback and queues a job to destroy every message from that user in batches.

app/models/reviewable_flagged_post.rb

@@ -72,7 +72,7 @@ class ReviewableFlaggedPost < Reviewable
 
     build_action(actions, :ignore, icon: 'external-link-alt')
 
-    if potential_spam? && guardian.can_delete_all_posts?(target_created_by)
+    if potential_spam? && guardian.can_delete_user?(target_created_by)
       delete_user_actions(actions)
     end
 

app/services/user_destroyer.rb

@@ -35,6 +35,10 @@ class UserDestroyer
       category_topic_ids = Category.where("topic_id IS NOT NULL").pluck(:topic_id)
 
       if opts[:delete_posts]
+        DiscoursePluginRegistry.user_destroyer_on_content_deletion_callbacks.each do |cb|
+          cb.call(user, @guardian, opts)
+        end
+
         agree_with_flags(user) if opts[:delete_as_spammer]
         block_external_urls(user) if opts[:block_urls]
         delete_posts(user, category_topic_ids, opts)

lib/discourse_plugin_registry.rb

@@ -97,6 +97,8 @@ class DiscoursePluginRegistry
 
   define_filtered_register :email_unsubscribers
 
+  define_filtered_register :user_destroyer_on_content_deletion_callbacks
+
   def self.register_auth_provider(auth_provider)
     self.auth_providers << auth_provider
   end

lib/plugin/instance.rb

@@ -1138,6 +1138,17 @@ class Plugin::Instance
     HashtagAutocompleteService.register_type_in_context(type, context, priority)
   end
 
+  ##
+  # Register a block that will be called when the UserDestroyer runs
+  # with the :delete_posts opt set to true. It's important to note that the block will
+  # execute before any other :delete_posts actions, it allows us to manipulate flags
+  # before agreeing with them. For example, discourse-akismet makes use of this
+  #
+  # @param {Block} callback to be called with the user, guardian, and the destroyer opts as arguments
+  def register_user_destroyer_on_content_deletion_callback(callback)
+    DiscoursePluginRegistry.register_user_destroyer_on_content_deletion_callback(callback, self)
+  end
+
   protected
 
   def self.js_path

plugins/chat/app/jobs/regular/delete_user_messages.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+module Jobs
+  class DeleteUserMessages < ::Jobs::Base
+    def execute(args)
+      return if args[:user_id].nil?
+
+      ChatMessageDestroyer.new
+        .destroy_in_batches(ChatMessage.with_deleted.where(user_id: args[:user_id]))
+    end
+  end
+end

plugins/chat/app/jobs/scheduled/delete_old_chat_messages.rb

@@ -9,47 +9,32 @@ module Jobs
       delete_dm_channel_messages
     end
 
+    private
+
     def delete_public_channel_messages
       return unless valid_day_value?(:chat_channel_retention_days)
 
-      ChatMessage
-        .in_public_channel
-        .with_deleted
-        .created_before(SiteSetting.chat_channel_retention_days.days.ago)
-        .in_batches(of: 200)
-        .each do |relation|
-          destroyed_ids = relation.destroy_all.pluck(:id)
-          reset_last_read_message_id(destroyed_ids)
-          delete_flags(destroyed_ids)
-        end
+      ChatMessageDestroyer.new.destroy_in_batches(
+        ChatMessage
+          .in_public_channel
+          .with_deleted
+          .created_before(SiteSetting.chat_channel_retention_days.days.ago)
+      )
     end
 
     def delete_dm_channel_messages
       return unless valid_day_value?(:chat_dm_retention_days)
 
-      ChatMessage
-        .in_dm_channel
-        .with_deleted
-        .created_before(SiteSetting.chat_dm_retention_days.days.ago)
-        .in_batches(of: 200)
-        .each do |relation|
-          destroyed_ids = relation.destroy_all.pluck(:id)
-          reset_last_read_message_id(destroyed_ids)
-        end
+      ChatMessageDestroyer.new.destroy_in_batches(
+        ChatMessage
+          .in_dm_channel
+          .with_deleted
+          .created_before(SiteSetting.chat_dm_retention_days.days.ago)
+      )
     end
 
     def valid_day_value?(setting_name)
       (SiteSetting.public_send(setting_name) || 0).positive?
     end
-
-    def reset_last_read_message_id(ids)
-      UserChatChannelMembership.where(last_read_message_id: ids).update_all(
-        last_read_message_id: nil,
-      )
-    end
-
-    def delete_flags(message_ids)
-      ReviewableChatMessage.where(target_id: message_ids).destroy_all
-    end
   end
 end

plugins/chat/app/services/chat_message_destroyer.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+class ChatMessageDestroyer
+  def destroy_in_batches(chat_messages_query, batch_size: 200)
+    chat_messages_query.in_batches(of: batch_size).each do |relation|
+      destroyed_ids = relation.destroy_all.pluck(:id)
+      reset_last_read(destroyed_ids)
+      delete_flags(destroyed_ids)
+    end
+  end
+
+  private
+
+  def reset_last_read(message_ids)
+    UserChatChannelMembership.where(last_read_message_id: message_ids).update_all(
+      last_read_message_id: nil,
+    )
+  end
+
+  def delete_flags(message_ids)
+    ReviewableChatMessage.where(target_id: message_ids).destroy_all
+  end
+end

plugins/chat/plugin.rb

@@ -192,11 +192,13 @@ after_initialize do
   load File.expand_path("../app/jobs/regular/chat_notify_mentioned.rb", __FILE__)
   load File.expand_path("../app/jobs/regular/chat_notify_watching.rb", __FILE__)
   load File.expand_path("../app/jobs/regular/update_channel_user_count.rb", __FILE__)
+  load File.expand_path("../app/jobs/regular/delete_user_messages.rb", __FILE__)
   load File.expand_path("../app/jobs/scheduled/delete_old_chat_messages.rb", __FILE__)
   load File.expand_path("../app/jobs/scheduled/update_user_counts_for_chat_channels.rb", __FILE__)
   load File.expand_path("../app/jobs/scheduled/email_chat_notifications.rb", __FILE__)
   load File.expand_path("../app/jobs/scheduled/auto_join_users.rb", __FILE__)
   load File.expand_path("../app/services/chat_publisher.rb", __FILE__)
+  load File.expand_path("../app/services/chat_message_destroyer.rb", __FILE__)
   load File.expand_path("../app/controllers/api_controller.rb", __FILE__)
   load File.expand_path("../app/controllers/api/chat_channels_controller.rb", __FILE__)
   load File.expand_path("../app/controllers/api/chat_channel_memberships_controller.rb", __FILE__)
@@ -729,6 +731,10 @@ after_initialize do
     limited_pretty_text_markdown_rules: ChatMessage::MARKDOWN_IT_RULES,
     hashtag_configurations: HashtagAutocompleteService.contexts_with_ordered_types,
   }
+
+  register_user_destroyer_on_content_deletion_callback(
+    Proc.new { |user| Jobs.enqueue(:delete_user_messages, user_id: user.id) }
+  )
 end
 
 if Rails.env == "test"

plugins/chat/spec/jobs/delete_old_chat_messages_spec.rb

@@ -109,40 +109,6 @@ describe Jobs::DeleteOldChatMessages do
       SiteSetting.chat_channel_retention_days = 800
       expect { described_class.new.execute }.not_to change { ChatMessage.in_public_channel.count }
     end
-
-    it "resets last_read_message_id from memberships" do
-      SiteSetting.chat_channel_retention_days = 20
-      membership =
-        UserChatChannelMembership.create!(
-          user: Fabricate(:user),
-          chat_channel: public_channel,
-          last_read_message_id: public_days_old_30.id,
-          following: true,
-          desktop_notification_level: 2,
-          mobile_notification_level: 2,
-        )
-      described_class.new.execute
-
-      expect(membership.reload.last_read_message_id).to be_nil
-    end
-
-    it "deletes flags associated to deleted chat messages" do
-      SiteSetting.chat_channel_retention_days = 10
-      guardian = Guardian.new(Discourse.system_user)
-      Chat::ChatReviewQueue.new.flag_message(
-        public_days_old_20,
-        guardian,
-        ReviewableScore.types[:off_topic],
-      )
-
-      reviewable = ReviewableChatMessage.last
-      expect(reviewable).to be_present
-
-      described_class.new.execute
-
-      expect { public_days_old_20.reload }.to raise_exception(ActiveRecord::RecordNotFound)
-      expect { reviewable.reload }.to raise_exception(ActiveRecord::RecordNotFound)
-    end
   end
 
   describe "dm channels" do
@@ -166,21 +132,5 @@ describe Jobs::DeleteOldChatMessages do
       SiteSetting.chat_dm_retention_days = 800
       expect { described_class.new.execute }.not_to change { ChatMessage.in_dm_channel.count }
     end
-
-    it "resets last_read_message_id from memberships" do
-      SiteSetting.chat_dm_retention_days = 20
-      membership =
-        UserChatChannelMembership.create!(
-          user: Fabricate(:user),
-          chat_channel: dm_channel,
-          last_read_message_id: dm_days_old_30.id,
-          following: true,
-          desktop_notification_level: 2,
-          mobile_notification_level: 2,
-        )
-      described_class.new.execute
-
-      expect(membership.reload.last_read_message_id).to be_nil
-    end
   end
 end

plugins/chat/spec/jobs/regular/delete_user_messages_spec.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::DeleteUserMessages do
+  describe "#execute" do
+    fab!(:user_1) { Fabricate(:user) }
+    fab!(:channel) { Fabricate(:chat_channel) }
+    fab!(:chat_message) { Fabricate(:chat_message, chat_channel: channel, user: user_1) }
+
+    it "deletes messages from the user" do
+      subject.execute(user_id: user_1)
+
+      expect { chat_message.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it "doesn't delete messages from other users" do
+      user_2 = Fabricate(:user)
+      user_2_message = Fabricate(:chat_message, chat_channel: channel, user: user_2)
+
+      subject.execute(user_id: user_1)
+
+      expect(user_2_message.reload).to be_present
+    end
+
+    it "deletes trashed messages" do
+      chat_message.trash!
+
+      subject.execute(user_id: user_1)
+
+      expect(ChatMessage.with_deleted.where(id: chat_message.id)).to be_empty
+    end
+  end
+end

plugins/chat/spec/plugin_spec.rb

@@ -421,4 +421,17 @@ describe Chat do
       end
     end
   end
+
+  describe "Deleting posts while deleting a user" do
+    fab!(:user) { Fabricate(:user) }
+
+    it "queues a job to also delete chat messages" do
+      deletion_opts = { delete_posts: true }
+
+      expect { UserDestroyer.new(Discourse.system_user).destroy(user, deletion_opts) }.to change(
+        Jobs::DeleteUserMessages.jobs,
+        :size,
+      ).by(1)
+    end
+  end
 end

plugins/chat/spec/services/chat_message_destroyer_spec.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+RSpec.describe ChatMessageDestroyer do
+  describe "#destroy_in_batches" do
+    fab!(:message_1) { Fabricate(:chat_message) }
+    fab!(:user_1) { Fabricate(:user) }
+
+    it "resets last_read_message_id from memberships" do
+      membership =
+        UserChatChannelMembership.create!(
+          user: user_1,
+          chat_channel: message_1.chat_channel,
+          last_read_message: message_1,
+          following: true,
+          desktop_notification_level: 2,
+          mobile_notification_level: 2,
+        )
+
+      described_class.new.destroy_in_batches(ChatMessage.where(id: message_1.id))
+
+      expect(membership.reload.last_read_message_id).to be_nil
+    end
+
+    it "deletes flags associated to deleted chat messages" do
+      guardian = Guardian.new(Discourse.system_user)
+      Chat::ChatReviewQueue.new.flag_message(
+        message_1,
+        guardian,
+        ReviewableScore.types[:off_topic],
+      )
+
+      reviewable = ReviewableChatMessage.last
+      expect(reviewable).to be_present
+
+      described_class.new.destroy_in_batches(ChatMessage.where(id: message_1.id))
+
+      expect { message_1.reload }.to raise_exception(ActiveRecord::RecordNotFound)
+      expect { reviewable.reload }.to raise_exception(ActiveRecord::RecordNotFound)
+    end
+
+    it "doesn't delete other messages" do
+      message_2 = Fabricate(:chat_message, chat_channel: message_1.chat_channel)
+
+      described_class.new.destroy_in_batches(ChatMessage.where(id: message_1.id))
+
+      expect { message_1.reload }.to raise_exception(ActiveRecord::RecordNotFound)
+      expect(message_2.reload).to be_present
+    end
+  end
+end

spec/lib/plugin/instance_spec.rb

@@ -770,4 +770,36 @@ RSpec.describe Plugin::Instance do
       expect(About.displayed_plugin_stat_groups).to eq([])
     end
   end
+
+  describe "#register_user_destroyer_on_content_deletion_callback" do
+    let(:plugin) { Plugin::Instance.new }
+
+    after do
+      DiscoursePluginRegistry.reset_register!(:user_destroyer_on_content_deletion_callbacks)
+    end
+
+    fab!(:user) { Fabricate(:user) }
+
+    it "calls the callback when the UserDestroyer runs with the delete_posts opt set to true" do
+      callback_called = false
+
+      cb = Proc.new { callback_called = true }
+      plugin.register_user_destroyer_on_content_deletion_callback(cb)
+
+      UserDestroyer.new(Discourse.system_user).destroy(user, { delete_posts: true })
+
+      expect(callback_called).to eq(true)
+    end
+
+    it "doesn't run the callback when delete_posts opt is not true" do
+      callback_called = false
+
+      cb = Proc.new { callback_called = true }
+      plugin.register_user_destroyer_on_content_deletion_callback(cb)
+
+      UserDestroyer.new(Discourse.system_user).destroy(user, {})
+
+      expect(callback_called).to eq(false)
+    end
+  end
 end

spec/models/reviewable_flagged_post_spec.rb

@@ -83,6 +83,25 @@ RSpec.describe ReviewableFlaggedPost, type: :model do
         expect(reviewable.actions_for(guardian).has?(:agree_and_silence)).to eq(false)
         expect(reviewable.actions_for(guardian).has?(:agree_and_suspend)).to eq(false)
       end
+
+      context "when flagged as potential_spam" do
+        before { reviewable.update!(potential_spam: true) }
+
+        it "excludes delete action if the reviewer cannot delete the user" do
+          post.user.user_stat.update!(
+            first_post_created_at: 1.year.ago,
+            post_count: User::MAX_STAFF_DELETE_POST_COUNT + 1
+          )
+
+          expect(reviewable.actions_for(guardian).has?(:delete_user)).to be false
+          expect(reviewable.actions_for(guardian).has?(:delete_user_block)).to be false
+        end
+
+        it "includes delete actions if the reviewer can delete the user" do
+          expect(reviewable.actions_for(guardian).has?(:delete_user)).to be true
+          expect(reviewable.actions_for(guardian).has?(:delete_user_block)).to be true
+        end
+      end
     end
 
     it "agree_and_keep agrees with the flags and keeps the post" do