FIX: do not agree flags by default when deleting posts

FIX: do not agree flags by default when deleting posts

From cb6fc8057b7f66e3f0b19b0ec62f23a6823e1d2f Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@techapj.com>
Date: Mon, 26 Nov 2018 21:28:37 +0530
Subject: [PATCH] FIX: do not agree flags by default when deleting posts


diff --git a/app/controllers/admin/flags_controller.rb b/app/controllers/admin/flags_controller.rb
index a94eae4..5d1be83 100644
--- a/app/controllers/admin/flags_controller.rb
+++ b/app/controllers/admin/flags_controller.rb
@@ -86,8 +86,7 @@ class Admin::FlagsController < Admin::AdminController
     restore_post = params[:action_on_post] == "restore"
 
     if delete_post
-      # PostDestroy calls PostAction.agree_flags!
-      destroy_post(post)
+      destroy_post(post, agree_flags: true)
     elsif restore_post
       PostAction.agree_flags!(post, current_user, delete_post)
       PostDestroyer.new(current_user, post).recover
@@ -138,12 +137,12 @@ class Admin::FlagsController < Admin::AdminController
 
   private
 
-  def destroy_post(post)
+  def destroy_post(post, agree_flags: false)
     if post.is_first_post?
       topic = Topic.find_by(id: post.topic_id)
       guardian.ensure_can_delete!(topic) if topic.present?
     end
 
-    PostDestroyer.new(current_user, post).destroy
+    PostDestroyer.new(current_user, post, agree_flags: agree_flags).destroy
   end
 end
diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb
index 7336e98..4398d06 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[:agree_flags]
+          agree_with_flags
+        else
+          defer_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,
@@ -237,10 +241,13 @@ class PostDestroyer
         }
       )
     end
-
     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..83fbc3b 100644
--- a/spec/components/post_destroyer_spec.rb
+++ b/spec/components/post_destroyer_spec.rb
@@ -632,13 +632,13 @@ 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
       second_post.expects(:update_flagged_posts_count)
 
-      PostDestroyer.new(moderator, second_post).destroy
+      PostDestroyer.new(moderator, second_post, agree_flags: true).destroy
 
       expect(PostAction.find_by(id: bookmark.id)).to eq(nil)
 
@@ -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)
-      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(PostAction.flagged_posts_count).to eq(1)
+      PostDestroyer.new(Discourse.system_user, second_post, agree_flags: true).destroy
+      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)
-      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(PostAction.flagged_posts_count).to eq(1)
+      PostDestroyer.new(second_post.user, second_post, agree_flags: true).destroy
+      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
-      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(PostAction.flagged_posts_count).to eq(0)
+
+      PostDestroyer.new(moderator, second_post, agree_flags: true).destroy
+      expect(Jobs::SendSystemMessage.jobs.size).to eq(0)
+    end
+
+    it "should not send the flags_agreed_and_post_deleted message if agree_flags is false" do
+      expect(PostAction.flagged_posts_count).to eq(1)
+      PostDestroyer.new(moderator, second_post, agree_flags: false).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

GitHub

What is the context of this change?

When agreeing with a flag if staff opts to delete all the child replies then all the child posts flags (if they have any) gets agreed by default. This commit defaults posts deletions to defer any outstanding flag instead of agreeing by default.

1 Like

Does this mean we amended a default here, previously if you delete a post explicitly we agree with flags automatically. Does this not mean we change behavior here?

1 Like

Yes, we amended a default here. Prior to this change when deleting single/multiple posts we used to agree with flags by default, now we defer the flag unless staff explicitly agrees with flag. Is this not desirable? I thought the end goal here was to mark the flag as agreed only when staff does it knowingly. Or should we agree with flag (automatically) only when deleting a single post and not in the case of multiple posts (bulk deletion)?

Prior to this change when deleting single/multiple posts we used to agree with flags by default, now we defer the flag unless staff explicitly agrees with flag

To be clear this is how it is supposed to work

  • old BAD behavior: delete flagged post and all replies, all flags on all posts agreed with

  • new GOOD behavior: delete flagged post and all replies, flags on parent post agreed with, flags on replies are ignored

The only behavior change is in the very specific area of “delete flagged post and all replies”. no other behavior should have changed. all other flag behaviors should remain as-is.

2 Likes

I reverted this commit for now to discuss default behavior of flag handling when bulk deleting posts.

1 Like