FIX: Allow user to recover/delete post if they can review the topic (PR #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.

GitHub

Thanks a lot, Martin!

Should we also check that the post has a reviewable? Otherwise, it allows users who can review topics to delete/recover any post.

@romanrizzi good call thank you, will make the change!

    if @user.staff? || delete_removed_posts_after < 1 || (user_can_review_topic? && post_is_reviewable?)
            @reply.topic.update!(category: review_category)
            review_group.users << review_user

We only need to reload once here

Only need to reload once here

I’m not sure if we need to check a column that does not change here.

        post.topic.update!(category: review_category)
        review_group.users << review_user

I think we should avoid making this change for now since it is more of a refactor and does not affect the bug that we’re trying to solve for here.

Minor suggestion, we don’t really need two methods for our use case here. I would just fold post_is_reviewable? into user_can_review_post?

def user_can_review_post?
  Guardian.new(@user).can_review_topic?(@post.topic) && Reviewable.exists?(target: @post)
end