FEATURE: Allow category group moderators to delete topics (#11069)

* FEATURE - allow category group moderators to delete topics

* Allow individual posts to be deleted

* DEV - refactor for new `can_moderate_topic?` method

app/assets/javascripts/discourse/app/widgets/topic-admin-menu.js

@@ -153,7 +153,10 @@ export default createWidget("topic-admin-menu", {
       });
     }
 
-    if (this.get("currentUser.canManageTopic")) {
+    if (
+      this.get("currentUser.canManageTopic") ||
+      details.get("can_moderate_category")
+    ) {
       if (details.get("can_delete")) {
         this.addActionButton({
           className: "topic-admin-delete",

app/assets/javascripts/discourse/tests/acceptance/topic-admin-menu-test.js

@@ -28,6 +28,9 @@ acceptance("Topic - Admin Menu", function (needs) {
       exists(".toggle-admin-menu"),
       "The admin menu button was rendered"
     );
+
+    await click(".toggle-admin-menu");
+    assert.ok(exists(".topic-admin-delete"), "The delete item was rendered");
   });
 
   test("Enter as a user with moderator and admin permissions", async function (assert) {

app/assets/javascripts/discourse/tests/fixtures/topic.js

@@ -5583,6 +5583,7 @@ export default {
     details: {
       notification_level: 3,
       notifications_reason_id: 1,
+      can_delete: true,
       can_edit: true,
       can_create_post: true,
       can_move_posts: true,
@@ -5593,6 +5594,7 @@ export default {
       can_archive_topic: true,
       can_split_merge_topic: true,
       can_edit_staff_notes: true,
+      can_moderate_category: true,
       participants: [
         {
           id: 3,

app/controllers/post_action_users_controller.rb

@@ -9,10 +9,8 @@ class PostActionUsersController < ApplicationController
     page = params[:page].to_i
     page_size = (params[:limit] || 200).to_i
 
-    finder = Post.where(id: params[:id].to_i)
+    # Find the post, and then determine if they can see the post (if deleted)
-    finder = finder.with_deleted if guardian.is_staff?
+    post = Post.with_deleted.where(id: params[:id].to_i).first
-
-    post = finder.first
     guardian.ensure_can_see!(post)
 
     unknown_user_ids = Set.new

app/controllers/posts_controller.rb

@@ -299,7 +299,7 @@ class PostsController < ApplicationController
 
   def destroy
     post = find_post_from_params
-    unless current_user.staff?
+    unless guardian.can_moderate_topic?(post.topic)
       RateLimiter.new(current_user, "delete_post_per_min", SiteSetting.max_post_deletions_per_minute, 1.minute).performed!
       RateLimiter.new(current_user, "delete_post_per_day", SiteSetting.max_post_deletions_per_day, 1.day).performed!
     end
@@ -320,7 +320,7 @@ class PostsController < ApplicationController
 
   def recover
     post = find_post_from_params
-    unless current_user.staff?
+    unless guardian.can_moderate_topic?(post.topic)
       RateLimiter.new(current_user, "delete_post_per_min", SiteSetting.max_post_deletions_per_minute, 1.minute).performed!
       RateLimiter.new(current_user, "delete_post_per_day", SiteSetting.max_post_deletions_per_day, 1.day).performed!
     end
@@ -802,14 +802,21 @@ class PostsController < ApplicationController
   end
 
   def find_post_using(finder)
-    # Include deleted posts if the user is staff
+    # A deleted post can be seen by staff or a category group moderator for the topic.
-    finder = finder.with_deleted if current_user.try(:staff?)
+    # But we must find the deleted post to determine which category it belongs to, so
-    post = finder.first
+    # we must find.with_deleted
+    post = finder.with_deleted.first
     raise Discourse::NotFound unless post
 
-    # load deleted topic
+    post.topic = Topic.with_deleted.find(post.topic_id)
-    post.topic = Topic.with_deleted.find(post.topic_id) if current_user.try(:staff?)
+
-    raise Discourse::NotFound unless post.topic
+    if !post.topic ||
+       (
+        (post.deleted_at.present? || post.topic.deleted_at.present?) &&
+        !guardian.can_moderate_topic?(post.topic)
+       )
+      raise Discourse::NotFound
+    end
 
     guardian.ensure_can_see!(post)
     post

app/serializers/topic_view_details_serializer.rb

@@ -20,7 +20,8 @@ class TopicViewDetailsSerializer < ApplicationSerializer
      :can_close_topic,
      :can_archive_topic,
      :can_split_merge_topic,
-     :can_edit_staff_notes]
+     :can_edit_staff_notes,
+     :can_moderate_category]
   end
 
   attributes(
@@ -145,6 +146,7 @@ class TopicViewDetailsSerializer < ApplicationSerializer
   alias :include_can_archive_topic? :can_perform_action_available_to_group_moderators?
   alias :include_can_split_merge_topic? :can_perform_action_available_to_group_moderators?
   alias :include_can_edit_staff_notes? :can_perform_action_available_to_group_moderators?
+  alias :include_can_moderate_category? :can_perform_action_available_to_group_moderators?
 
   def include_can_publish_page?
     scope.can_publish_page?(object.topic)

lib/guardian.rb

@@ -94,6 +94,9 @@ class Guardian
   end
 
   def is_category_group_moderator?(category)
+    return false unless category
+    return false unless authenticated?
+
     @is_category_group_moderator ||= begin
       SiteSetting.enable_category_group_moderation? &&
         category.present? &&

lib/guardian/post_guardian.rb

@@ -181,17 +181,22 @@ module PostGuardian
     return false if post.is_first_post?
 
     # Can't delete posts in archived topics unless you are staff
-    return false if !is_staff? && post.topic.archived?
+    can_moderate = can_moderate_topic?(post.topic)
+    return false if !can_moderate && post.topic&.archived?
 
     # You can delete your own posts
     return !post.user_deleted? if is_my_own?(post)
 
-    is_staff?
+    can_moderate
   end
 
   # Recovery Method
   def can_recover_post?(post)
-    if is_staff?
+    return false unless post
+
+    topic = Topic.with_deleted.find(post.topic_id) if post.topic_id
+
+    if can_moderate_topic?(topic)
       !!post.deleted_at
     else
       is_my_own?(post) && post.user_deleted && !post.deleted_at
@@ -212,7 +217,7 @@ module PostGuardian
     return true if is_admin?
     return false unless can_see_topic?(post.topic)
     return false unless post.user == @user || Topic.visible_post_types(@user).include?(post.post_type)
-    return false if !is_moderator? && post.deleted_at.present?
+    return false if !(is_moderator? || is_category_group_moderator?(post.topic.category)) && post.deleted_at.present?
 
     true
   end
@@ -261,8 +266,8 @@ module PostGuardian
     is_staff?
   end
 
-  def can_see_deleted_posts?
+  def can_see_deleted_posts?(category = nil)
-    is_staff?
+    is_staff? || is_category_group_moderator?(category)
   end
 
   def can_view_raw_email?(post)

lib/guardian/topic_guardian.rb

@@ -19,6 +19,7 @@ module TopicGuardian
 
     is_category_group_moderator?(topic.category)
   end
+  alias :can_moderate_topic? :can_review_topic?
 
   def can_create_shared_draft?
     is_staff? && SiteSetting.shared_drafts_enabled?
@@ -115,7 +116,7 @@ module TopicGuardian
 
   # Recovery Method
   def can_recover_topic?(topic)
-    if is_staff?
+    if is_staff? || (topic&.category && is_category_group_moderator?(topic.category))
       !!(topic && topic.deleted_at)
     else
       topic && can_recover_post?(topic.ordered_posts.first)
@@ -124,7 +125,7 @@ module TopicGuardian
 
   def can_delete_topic?(topic)
     !topic.trashed? &&
-    (is_staff? || (is_my_own?(topic) && topic.posts_count <= 1 && topic.created_at && topic.created_at > 24.hours.ago)) &&
+    (is_staff? || (is_my_own?(topic) && topic.posts_count <= 1 && topic.created_at && topic.created_at > 24.hours.ago) || is_category_group_moderator?(topic.category)) &&
     !topic.is_category_topic? &&
     !Discourse.static_doc_topic_ids.include?(topic.id)
   end
@@ -142,14 +143,14 @@ module TopicGuardian
     authenticated? && topic && @user.has_trust_level?(TrustLevel[1])
   end
 
-  def can_see_deleted_topics?
+  def can_see_deleted_topics?(category)
-    is_staff?
+    is_staff? || is_category_group_moderator?(category)
   end
 
   def can_see_topic?(topic, hide_deleted = true)
     return false unless topic
     return true if is_admin?
-    return false if hide_deleted && topic.deleted_at && !can_see_deleted_topics?
+    return false if hide_deleted && topic.deleted_at && !can_see_deleted_topics?(topic.category)
 
     if topic.private_message?
       return authenticated? && topic.all_allowed_users.where(id: @user.id).exists?

lib/post_destroyer.rb

@@ -66,7 +66,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 || post_is_reviewable?
+    if delete_removed_posts_after < 1 || post_is_reviewable? || Guardian.new(@user).can_moderate_topic?(topic)
       perform_delete
     elsif @user.id == @post.user_id
       mark_for_deletion(delete_removed_posts_after)
@@ -86,7 +86,7 @@ class PostDestroyer
   end
 
   def recover
-    if (@user.staff? || post_is_reviewable?) && @post.deleted_at
+    if (post_is_reviewable? || Guardian.new(@user).can_moderate_topic?(@post.topic)) && @post.deleted_at
       staff_recovered
     elsif @user.staff? || @user.id == @post.user_id
       user_recovered
@@ -221,6 +221,8 @@ class PostDestroyer
   private
 
   def post_is_reviewable?
+    return true if @user.staff?
+
     topic = @post.topic || Topic.with_deleted.find(@post.topic_id)
     Guardian.new(@user).can_review_topic?(topic) && Reviewable.exists?(target: @post)
   end

lib/topic_view.rb

@@ -704,7 +704,7 @@ class TopicView
       .includes({ user: :primary_group }, :reply_to_user, :deleted_by, :incoming_email, :topic)
       .order('sort_order')
     @posts = filter_post_types(@posts)
-    @posts = @posts.with_deleted if @guardian.can_see_deleted_posts?
+    @posts = @posts.with_deleted if @guardian.can_see_deleted_posts?(@topic.category)
     @posts
   end
 
@@ -720,7 +720,7 @@ class TopicView
 
   def unfiltered_posts
     result = filter_post_types(@topic.posts)
-    result = result.with_deleted if @guardian.can_see_deleted_posts?
+    result = result.with_deleted if @guardian.can_see_deleted_posts?(@topic.category)
     result = result.where("user_id IS NOT NULL") if @exclude_deleted_users
     result = result.where(hidden: false) if @exclude_hidden
     result
@@ -776,7 +776,7 @@ class TopicView
     # copy the filter for has_deleted? method
     @predelete_filtered_posts = @filtered_posts.spawn
 
-    if @guardian.can_see_deleted_posts? && !@show_deleted && has_deleted?
+    if @guardian.can_see_deleted_posts?(@topic.category) && !@show_deleted && has_deleted?
       @filtered_posts = @filtered_posts.where(
         "posts.deleted_at IS NULL OR posts.post_number = 1"
       )

spec/components/guardian_spec.rb

@@ -847,12 +847,32 @@ describe Guardian do
         expect(Guardian.new(admin).can_banner_topic?(nil)).to be_falsey
         expect(Guardian.new(admin).can_banner_topic?(topic)).to be_truthy
       end
+
+      it 'respects category group moderator settings' do
+        group_user = Fabricate(:group_user)
+        user_gm = group_user.user
+        group = group_user.group
+        SiteSetting.enable_category_group_moderation = true
+
+        topic = Fabricate(:topic)
+
+        expect(Guardian.new(user_gm).can_see?(topic)).to be_truthy
+
+        topic.trash!(admin)
+        topic.reload
+
+        expect(Guardian.new(user_gm).can_see?(topic)).to be_falsey
+
+        topic.category.update!(reviewable_by_group_id: group.id, topic_id: post.topic.id)
+        expect(Guardian.new(user_gm).can_see?(topic)).to be_truthy
+      end
     end
 
     describe 'a Post' do
+      fab!(:post) { Fabricate(:post) }
       fab!(:another_admin) { Fabricate(:admin) }
+
       it 'correctly handles post visibility' do
-        post = Fabricate(:post)
         topic = post.topic
 
         expect(Guardian.new(user).can_see?(post)).to be_truthy
@@ -870,8 +890,25 @@ describe Guardian do
         expect(Guardian.new(admin).can_see?(post)).to be_truthy
       end
 
+      it 'respects category group moderator settings' do
+        group_user = Fabricate(:group_user)
+        user_gm = group_user.user
+        group = group_user.group
+        SiteSetting.enable_category_group_moderation = true
+
+        expect(Guardian.new(user_gm).can_see?(post)).to be_truthy
+
+        post.trash!(another_admin)
+        post.reload
+
+        expect(Guardian.new(user_gm).can_see?(post)).to be_falsey
+
+        post.topic.category.update!(reviewable_by_group_id: group.id, topic_id: post.topic.id)
+        expect(Guardian.new(user_gm).can_see?(post)).to be_truthy
+      end
+
       it 'respects whispers' do
-        regular_post = Fabricate.build(:post)
+        regular_post = post
         whisper_post = Fabricate.build(:post, post_type: Post.types[:whisper])
 
         anon_guardian = Guardian.new
@@ -1194,6 +1231,29 @@ describe Guardian do
         end
       end
     end
+
+    context 'category group moderation is enabled' do
+      fab!(:group_user) { Fabricate(:group_user) }
+
+      before do
+        topic.save!
+        post.save!
+
+        SiteSetting.enable_category_group_moderation = true
+        PostDestroyer.new(moderator, topic.first_post).destroy
+        topic.reload
+      end
+
+      it "returns false if user is not a member of the appropriate group" do
+        expect(Guardian.new(group_user.user).can_recover_topic?(topic)).to be_falsey
+      end
+
+      it "returns true if user is a member of the appropriate group" do
+        topic.category.update!(reviewable_by_group_id: group_user.group.id)
+
+        expect(Guardian.new(group_user.user).can_recover_topic?(topic)).to be_truthy
+      end
+    end
   end
 
   describe "can_recover_post?" do
@@ -1979,6 +2039,25 @@ describe Guardian do
         topic.update!(posts_count: 1, created_at: 48.hours.ago)
         expect(Guardian.new(topic.user).can_delete?(topic)).to be_falsey
       end
+
+      context 'category group moderation is enabled' do
+        fab!(:group_user) { Fabricate(:group_user) }
+
+        before do
+          SiteSetting.enable_category_group_moderation = true
+        end
+
+        it "returns false if user is not a member of the appropriate group" do
+          expect(Guardian.new(group_user.user).can_delete?(topic)).to be_falsey
+        end
+
+        it "returns true if user is a member of the appropriate group" do
+          topic.category.update!(reviewable_by_group_id: group_user.group.id)
+
+          expect(Guardian.new(group_user.user).can_delete?(topic)).to be_truthy
+        end
+      end
+
     end
 
     context 'a Post' do

spec/components/post_destroyer_spec.rb

@@ -313,19 +313,6 @@ describe PostDestroyer do
               end
             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
@@ -516,21 +503,6 @@ describe PostDestroyer do
           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

spec/requests/posts_controller_spec.rb

@@ -45,6 +45,28 @@ shared_examples 'finding and showing post' do
       get url
       expect(response.status).to eq(200)
     end
+
+    context "category group moderator" do
+      fab!(:group_user) { Fabricate(:group_user) }
+      let(:user_gm) { group_user.user }
+      let(:group) { group_user.group }
+
+      before do
+        SiteSetting.enable_category_group_moderation = true
+        sign_in(user_gm)
+      end
+
+      it "can find posts in the allowed category" do
+        post.topic.category.update!(reviewable_by_group_id: group.id, topic_id: topic.id)
+        get url
+        expect(response.status).to eq(200)
+      end
+
+      it "can't find posts outside of the allowed category" do
+        get url
+        expect(response.status).to eq(404)
+      end
+    end
   end
 end