FIX: defer flags (only) when handling a flag and deleting replies (#6702)

FIX: defer flags (only) when handling a flag and deleting replies (#6702)

From 40f10855c621a2b059c5d91191a8b4a3b29adbf2 Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@techapj.com>
Date: Thu, 29 Nov 2018 22:44:18 +0530
Subject: [PATCH] FIX: defer flags (only) when handling a flag and deleting
 replies (#6702)


diff --git a/app/assets/javascripts/admin/models/flagged-post.js.es6 b/app/assets/javascripts/admin/models/flagged-post.js.es6
index d2a9c45..fde2cdf 100644
--- a/app/assets/javascripts/admin/models/flagged-post.js.es6
+++ b/app/assets/javascripts/admin/models/flagged-post.js.es6
@@ -136,7 +136,7 @@ export default Post.extend({
             label: I18n.t("yes_value"),
             class: "btn-danger",
             callback() {
-              Post.deleteMany(replies.map(r => r.id))
+              Post.deleteMany(replies.map(r => r.id), { deferFlags: true })
                 .then(action)
                 .then(resolve)
                 .catch(error => {
diff --git a/app/assets/javascripts/discourse/models/post.js.es6 b/app/assets/javascripts/discourse/models/post.js.es6
index 3ea49fd..4de7c3b 100644
--- a/app/assets/javascripts/discourse/models/post.js.es6
+++ b/app/assets/javascripts/discourse/models/post.js.es6
@@ -369,10 +369,10 @@ Post.reopenClass({
     });
   },
 
-  deleteMany(post_ids) {
+  deleteMany(post_ids, { deferFlags = false } = {}) {
     return ajax("/posts/destroy_many", {
       type: "DELETE",
-      data: { post_ids }
+      data: { post_ids, defer_flags: deferFlags }
     });
   },
 
diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
index f35ae00..e4e0ccf 100644
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@@ -336,6 +336,7 @@ class PostsController < ApplicationController
 
   def destroy_many
     params.require(:post_ids)
+    defer_flags = params[:defer_flags] || false
 
     posts = Post.where(id: post_ids_including_replies)
     raise Discourse::InvalidParameters.new(:post_ids) if posts.blank?
@@ -344,7 +345,7 @@ class PostsController < ApplicationController
     posts.each { |p| guardian.ensure_can_delete!(p) }
 
     Post.transaction do
-      posts.each { |p| PostDestroyer.new(current_user, p).destroy }
+      posts.each { |p| PostDestroyer.new(current_user, p, defer_flags: defer_flags).destroy }
     end
 
     render body: nil
diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb
index 7336e98..efa292e 100644
--- a/lib/post_destroyer.rb
+++ b/lib/post_destroyer.rb
@@ -147,7 +147,11 @@ class PostDestroyer
       update_user_counts
       TopicUser.update_post_action_cache(post_id: @post.id)
       DB.after_commit do
-        agree_with_flags
+        if @opts[:defer_flags].to_s == "true"
+          defer_flags
+        else
+          agree_with_flags
+        end
       end
     end
 
@@ -225,7 +229,7 @@ class PostDestroyer
     if @post.has_active_flag? && @user.id > 0 && @user.staff?
       Jobs.enqueue(
         :send_system_message,
-        user_id: @post.user.id,
+        user_id: @post.user_id,
         message_type: :flags_agreed_and_post_deleted,
         message_options: {
           url: @post.url,
@@ -241,6 +245,10 @@ class PostDestroyer
     PostAction.agree_flags!(@post, @user, delete_post: true)
   end
 
+  def defer_flags
+    PostAction.defer_flags!(@post, @user, delete_post: true)
+  end
+
   def trash_user_actions
     UserAction.where(target_post_id: @post.id).each do |ua|
       row = {
diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb
index 7a91c3b..e29543b 100644
--- a/spec/components/post_destroyer_spec.rb
+++ b/spec/components/post_destroyer_spec.rb
@@ -632,7 +632,7 @@ describe PostDestroyer do
     let!(:flag) { PostAction.act(moderator, second_post, PostActionType.types[:off_topic]) }
 
     before do
-      SiteSetting.queue_jobs = false
+      Jobs::SendSystemMessage.clear
     end
 
     it "should delete public post actions and agree with flags" do
@@ -650,36 +650,39 @@ describe PostDestroyer do
       expect(second_post.bookmark_count).to eq(0)
       expect(second_post.off_topic_count).to eq(1)
 
-      notification = second_post.user.notifications.where(notification_type: Notification.types[:private_message]).last
-      expect(notification).to be_present
-      expect(notification.topic.title).to eq(I18n.t('system_messages.flags_agreed_and_post_deleted.subject_template'))
+      expect(Jobs::SendSystemMessage.jobs.size).to eq(1)
     end
 
     it "should not send the flags_agreed_and_post_deleted message if it was deleted by system" do
-      second_post.expects(:update_flagged_posts_count)
+      expect(PostAction.flagged_posts_count).to eq(1)
       PostDestroyer.new(Discourse.system_user, second_post).destroy
-      expect(
-        Topic.where(title: I18n.t('system_messages.flags_agreed_and_post_deleted.subject_template')).exists?
-      ).to eq(false)
+      expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
+      expect(PostAction.flagged_posts_count).to eq(0)
     end
 
     it "should not send the flags_agreed_and_post_deleted message if it was deleted by author" do
       SiteSetting.delete_removed_posts_after = 0
-      second_post.expects(:update_flagged_posts_count)
+      expect(PostAction.flagged_posts_count).to eq(1)
       PostDestroyer.new(second_post.user, second_post).destroy
-      expect(
-        Topic.where(title: I18n.t('system_messages.flags_agreed_and_post_deleted.subject_template')).exists?
-      ).to eq(false)
+      expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
+      expect(PostAction.flagged_posts_count).to eq(0)
     end
 
     it "should not send the flags_agreed_and_post_deleted message if flags were deferred" do
-      second_post.expects(:update_flagged_posts_count)
+      expect(PostAction.flagged_posts_count).to eq(1)
       PostAction.defer_flags!(second_post, moderator)
       second_post.reload
+      expect(PostAction.flagged_posts_count).to eq(0)
+
       PostDestroyer.new(moderator, second_post).destroy
-      expect(
-        Topic.where(title: I18n.t('system_messages.flags_agreed_and_post_deleted.subject_template')).exists?
-      ).to eq(false)
+      expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
+    end
+
+    it "should not send the flags_agreed_and_post_deleted message if defer_flags is true" do
+      expect(PostAction.flagged_posts_count).to eq(1)
+      PostDestroyer.new(moderator, second_post, defer_flags: true).destroy
+      expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
+      expect(PostAction.flagged_posts_count).to eq(0)
     end
 
     it "should set the deleted_public_actions custom field" do
diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb
index dac7ad7..e3e6490 100644
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@@ -243,6 +243,24 @@ describe PostsController do
           delete "/posts/destroy_many.json", params: { post_ids: [post1.id], reply_post_ids: [post1.id] }
         end
       end
+
+      context "deleting flagged posts" do
+        let(:moderator) { Fabricate(:moderator) }
+
+        before do
+          PostAction.act(moderator, post1, PostActionType.types[:off_topic])
+          PostAction.act(moderator, post2, PostActionType.types[:off_topic])
+          Jobs::SendSystemMessage.clear
+        end
+
+        it "defers the posts" do
+          sign_in(moderator)
+          expect(PostAction.flagged_posts_count).to eq(2)
+          delete "/posts/destroy_many.json", params: { post_ids: [post1.id, post2.id], defer_flags: true }
+          expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
+          expect(PostAction.flagged_posts_count).to eq(0)
+        end
+      end
     end
   end

GitHub

1 Like

in this spot this feels odd to me, in fact we should probably clean up all of @opts handling so the first time we get @opts passed in we handle parsing and extracting stuff into something tidy and setting local attributes. Then this can simply be if @defer_flags and further up it can be StaffActionLogger.new(@user).log_post_deletion(@post, @context)

1 Like

defer_flags = params[:defer_flags].to_s == "true" if we want to allow both string and bool here. The earlier you can get clean params the betterer

1 Like

Overall this looks great to me, but I think it is worth cleaning up internals a bit here for future travelers.

1 Like

Did some string/boolean cleanup in FEATURE: defer flags when deleting child replies (#7111)

1 Like