FIX: Allow user to recover/delete post if they can review the topic (#10273)

FIX: Allow user to recover/delete post if they can review the topic (#10273)

To reproduce the initial issue here:

  1. A user makes a post, which discourse-akismet marks as spam (I cheated and called DiscourseAkismet::PostsBouncer.new.send(:mark_as_spam, post) for this)
  2. The post lands in the review queue
  3. The category the topic is in has a reviewable_by_group_id
  4. A user in that group goes and looks at the Review queue, decides the post is not spam, and clicks Not Spam
  5. Weird stuff happens because the PostDestroyer#recover method didn’t handle this (the user who clicked Not Spam was not the owner of the post and was not a staff member, so the post didn’t get un-destroyed and post counts didn’t get updated)

Now users who belong to a group who can review a category now have the ability to recover/delete posts fully.

diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb
index 3877bcc..ff7240b 100644
--- a/lib/post_destroyer.rb
+++ b/lib/post_destroyer.rb
@@ -65,7 +65,7 @@ class PostDestroyer
 
     delete_removed_posts_after = @opts[:delete_removed_posts_after] || SiteSetting.delete_removed_posts_after
 
-    if @user.staff? || delete_removed_posts_after < 1
+    if @user.staff? || delete_removed_posts_after < 1 || post_is_reviewable?
       perform_delete
     elsif @user.id == @post.user_id
       mark_for_deletion(delete_removed_posts_after)
@@ -85,7 +85,7 @@ class PostDestroyer
   end
 
   def recover
-    if @user.staff? && @post.deleted_at
+    if (@user.staff? || post_is_reviewable?) && @post.deleted_at
       staff_recovered
     elsif @user.staff? || @user.id == @post.user_id
       user_recovered
@@ -213,6 +213,10 @@ class PostDestroyer
 
   private
 
+  def post_is_reviewable?
+    Guardian.new(@user).can_review_topic?(@post.topic) && Reviewable.exists?(target: @post)
+  end
+
   # we need topics to change if ever a post in them is deleted or created
   # this ensures users relying on this information can keep unread tracking
   # working as desired
diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb
index ea5a7c9..45c298c 100644
--- a/spec/components/post_destroyer_spec.rb
+++ b/spec/components/post_destroyer_spec.rb
@@ -272,6 +272,48 @@ describe PostDestroyer do
           expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::NEW_TOPIC).count).to eq(1)
           expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::REPLY).count).to eq(1)
         end
+
+        context "recovered by user with access to moderate topic category" do
+          fab!(:review_user) { Fabricate(:user) }
+
+          before do
+            SiteSetting.enable_category_group_moderation = true
+            review_group = Fabricate(:group)
+            review_category = Fabricate(:category, reviewable_by_group_id: review_group.id)
+            @reply.topic.update!(category: review_category)
+            review_group.users << review_user
+          end
+
+          context "when the post has a Reviewable record" do
+            before do
+              ReviewableFlaggedPost.needs_review!(target: @reply, created_by: Fabricate(:user))
+            end
+
+            it "changes deleted_at to nil" do
+              PostDestroyer.new(Discourse.system_user, @reply).destroy
+              @reply.reload
+              expect(@reply.user_deleted).to eq(false)
+              expect(@reply.deleted_at).not_to eq(nil)
+
+              PostDestroyer.new(review_user, @reply).recover
+              @reply.reload
+              expect(@reply.deleted_at).to eq(nil)
+            end
+          end
+
+          context "when the post does not have a Reviewable record" do
+            it "does not recover the post" do
+              PostDestroyer.new(Discourse.system_user, @reply).destroy
+              @reply.reload
+              expect(@reply.user_deleted).to eq(false)
+              expect(@reply.deleted_at).not_to eq(nil)
+
+              PostDestroyer.new(review_user, @reply).recover
+              @reply.reload
+              expect(@reply.deleted_at).not_to eq(nil)
+            end
+          end
+        end
       end
     end
   end
@@ -423,6 +465,53 @@ describe PostDestroyer do
       end
     end
 
+    context "deleted by user with access to moderate topic category" do
+      fab!(:review_user) { Fabricate(:user) }
+
+      before do
+        SiteSetting.enable_category_group_moderation = true
+        review_group = Fabricate(:group)
+        review_category = Fabricate(:category, reviewable_by_group_id: review_group.id)
+        post.topic.update!(category: review_category)
+        review_group.users << review_user
+      end
+
+      context "when the post has a reviewable" do
+        it "deletes the post" do
+          author = post.user
+          reply = create_post(topic_id: post.topic_id, user: author)
+          ReviewableFlaggedPost.needs_review!(target: reply, created_by: Fabricate(:user))
+
+          post_count = author.post_count
+          history_count = UserHistory.count
+
+          PostDestroyer.new(review_user, reply).destroy
+
+          expect(reply.deleted_at).to be_present
+          expect(reply.deleted_by).to eq(review_user)
+
+          author.reload
+          expect(author.post_count).to eq(post_count - 1)
+          expect(UserHistory.count).to eq(history_count + 1)
+        end
+      end
+
+      context "when the post does not have a reviewable" do
+        it "does not delete the post" do
+          author = post.user
+          reply = create_post(topic_id: post.topic_id, user: author)
+
+          post_count = author.post_count
+          history_count = UserHistory.count
+
+          PostDestroyer.new(review_user, reply).destroy
+
+          expect(reply.deleted_at).not_to be_present
+          expect(reply.deleted_by).to eq(nil)
+        end
+      end
+    end
+
     context "as an admin" do
       it "deletes the post" do
         PostDestroyer.new(admin, post).destroy

GitHub sha: 93a8e34f

1 Like

This commit appears in #10273 which was merged by martin.