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.
This commit is contained in:
Martin Brennan 2020-07-22 11:57:16 +10:00 committed by GitHub
parent 16961dee76
commit 93a8e34f47
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 95 additions and 2 deletions

View File

@ -65,7 +65,7 @@ class PostDestroyer
delete_removed_posts_after = @opts[:delete_removed_posts_after] || SiteSetting.delete_removed_posts_after 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 perform_delete
elsif @user.id == @post.user_id elsif @user.id == @post.user_id
mark_for_deletion(delete_removed_posts_after) mark_for_deletion(delete_removed_posts_after)
@ -85,7 +85,7 @@ class PostDestroyer
end end
def recover def recover
if @user.staff? && @post.deleted_at if (@user.staff? || post_is_reviewable?) && @post.deleted_at
staff_recovered staff_recovered
elsif @user.staff? || @user.id == @post.user_id elsif @user.staff? || @user.id == @post.user_id
user_recovered user_recovered
@ -213,6 +213,10 @@ class PostDestroyer
private 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 # 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 # this ensures users relying on this information can keep unread tracking
# working as desired # working as desired

View File

@ -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::NEW_TOPIC).count).to eq(1)
expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::REPLY).count).to eq(1) expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::REPLY).count).to eq(1)
end 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 end
end end
@ -423,6 +465,53 @@ describe PostDestroyer do
end end
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 context "as an admin" do
it "deletes the post" do it "deletes the post" do
PostDestroyer.new(admin, post).destroy PostDestroyer.new(admin, post).destroy