From 93a8e34f475e2c0b820490ed6110d21c05c834f2 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 22 Jul 2020 11:57:16 +1000 Subject: [PATCH] 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. --- lib/post_destroyer.rb | 8 ++- spec/components/post_destroyer_spec.rb | 89 ++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 3877bcccdee..ff7240bc115 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 ea5a7c90a3d..45c298c4ba6 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