From 280adda09c891af9a9d406dda041fd986dd45ef7 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Wed, 4 Sep 2024 04:38:46 +0300 Subject: [PATCH] FEATURE: Support designating multiple groups as mods on category (#28655) Currently, categories support designating only 1 group as a moderation group on the category. This commit removes the one group limitation and makes it possible to designate multiple groups as mods on a category. Internal topic: t/124648. --- .../app/components/edit-category-settings.hbs | 9 ++-- .../app/components/edit-category-settings.js | 10 ++-- .../discourse/app/models/category.js | 2 +- .../javascripts/discourse/app/models/topic.js | 4 +- app/controllers/categories_controller.rb | 7 ++- .../reviewable_claimed_topics_controller.rb | 5 +- app/jobs/regular/notify_reviewable.rb | 40 +++++++-------- app/models/about.rb | 37 ++++++++++---- app/models/category.rb | 17 ++----- app/models/category_moderation_group.rb | 21 ++++++++ app/models/group.rb | 11 +--- app/models/reviewable.rb | 33 +++++------- app/models/reviewable_flagged_post.rb | 14 ++++-- app/models/reviewable_post.rb | 1 - app/models/reviewable_queued_post.rb | 1 - app/models/reviewable_user.rb | 1 - app/serializers/category_serializer.rb | 10 ++-- ...91047_create_category_moderation_groups.rb | 21 ++++++++ lib/guardian.rb | 22 ++++---- lib/guardian/user_guardian.rb | 9 +++- lib/post_action_creator.rb | 10 ++-- lib/topic_view.rb | 19 ++++--- .../spec/lib/chat/guardian_extensions_spec.rb | 6 +-- .../system/move_message_to_channel_spec.rb | 4 +- plugins/discourse-presence/plugin.rb | 5 +- .../spec/integration/presence_spec.rb | 2 +- .../category_moderation_group_fabricator.rb | 6 +++ spec/jobs/notify_reviewable_spec.rb | 9 ++-- .../refresh_users_reviewable_counts_spec.rb | 16 +++--- spec/lib/guardian/topic_guardian_spec.rb | 6 +-- spec/lib/guardian/user_guardian_spec.rb | 11 ++-- spec/lib/guardian_spec.rb | 25 ++++++---- spec/lib/new_post_manager_spec.rb | 9 ++-- spec/lib/post_destroyer_spec.rb | 6 ++- spec/lib/post_revisor_spec.rb | 5 +- spec/lib/topic_query_spec.rb | 3 +- spec/lib/validators/post_validator_spec.rb | 3 +- spec/models/about_spec.rb | 50 +++++++++++-------- spec/models/category_spec.rb | 44 +++------------- spec/models/post_action_spec.rb | 2 +- spec/models/reviewable_spec.rb | 20 +++++--- spec/models/user_spec.rb | 5 +- spec/requests/categories_controller_spec.rb | 49 +++++++++++++++++- spec/requests/posts_controller_spec.rb | 11 ++-- ...viewable_claimed_topics_controller_spec.rb | 8 ++- spec/requests/topics_controller_spec.rb | 32 +++++++++--- spec/serializers/category_serializer_spec.rb | 7 +-- spec/serializers/post_serializer_spec.rb | 8 +-- .../serializers/topic_view_serializer_spec.rb | 5 +- 49 files changed, 388 insertions(+), 273 deletions(-) create mode 100644 app/models/category_moderation_group.rb create mode 100644 db/migrate/20240828191047_create_category_moderation_groups.rb create mode 100644 spec/fabricators/category_moderation_group_fabricator.rb diff --git a/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs b/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs index c16182defeb..3da62905dca 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs +++ b/app/assets/javascripts/discourse/app/components/edit-category-settings.hbs @@ -120,11 +120,10 @@ {{#if this.siteSettings.enable_category_group_moderation}}
-
{{/if}} diff --git a/app/assets/javascripts/discourse/app/components/edit-category-settings.js b/app/assets/javascripts/discourse/app/components/edit-category-settings.js index 2192171cfaf..09a4333e198 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-settings.js +++ b/app/assets/javascripts/discourse/app/components/edit-category-settings.js @@ -3,7 +3,6 @@ import { and, empty } from "@ember/object/computed"; import { buildCategoryPanel } from "discourse/components/edit-category-panel"; import { setting } from "discourse/lib/computed"; import { SEARCH_PRIORITIES } from "discourse/lib/constants"; -import Group from "discourse/models/group"; import discourseComputed from "discourse-common/utils/decorators"; import I18n from "discourse-i18n"; @@ -50,10 +49,6 @@ export default class EditCategorySettings extends buildCategoryPanel( ]; } - groupFinder(term) { - return Group.findAll({ term, ignore_automatic: true }); - } - @discourseComputed availableViews() { return [ @@ -146,4 +141,9 @@ export default class EditCategorySettings extends buildCategoryPanel( let seconds = minutes ? minutes * 60 : null; this.set("category.default_slow_mode_seconds", seconds); } + + @action + onCategoryModeratingGroupsChange(groupIds) { + this.set("category.moderating_group_ids", groupIds); + } } diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index a6cf7a908d6..73014fa9759 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -761,7 +761,7 @@ export default class Category extends RestModel { "navigate_to_first_post_after_read" ), search_priority: this.search_priority, - reviewable_by_group_name: this.reviewable_by_group_name, + moderating_group_ids: this.moderating_group_ids, read_only_banner: this.read_only_banner, default_list_filter: this.default_list_filter, }), diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index 48d8462f7c9..3786a6a5b99 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -751,8 +751,8 @@ export default class Topic extends RestModel { if ( opts.force_destroy || (!deleted_by.staff && - !deleted_by.groups.some( - (group) => group.name === this.category?.reviewable_by_group_name + !deleted_by.groups.some((group) => + this.category?.moderating_group_ids?.includes(group.id) ) && !deleted_by.can_delete_all_posts_and_topics) ) { diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 590b7760ff0..2b312394c55 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -578,10 +578,9 @@ class CategoriesController < ApplicationController ] end + conditional_param_keys = [] if SiteSetting.enable_category_group_moderation? - params[:reviewable_by_group_id] = Group.where( - name: params[:reviewable_by_group_name], - ).pick(:id) if params[:reviewable_by_group_name] + conditional_param_keys << { moderating_group_ids: [] } end result = @@ -621,7 +620,7 @@ class CategoriesController < ApplicationController :allow_global_tags, :read_only_banner, :default_list_filter, - :reviewable_by_group_id, + *conditional_param_keys, category_setting_attributes: %i[ auto_bump_cooldown_days num_auto_bump_daily diff --git a/app/controllers/reviewable_claimed_topics_controller.rb b/app/controllers/reviewable_claimed_topics_controller.rb index bce77f3be65..df6f0edee88 100644 --- a/app/controllers/reviewable_claimed_topics_controller.rb +++ b/app/controllers/reviewable_claimed_topics_controller.rb @@ -36,9 +36,8 @@ class ReviewableClaimedTopicsController < ApplicationController def notify_users(topic, claimed_by) group_ids = Set.new([Group::AUTO_GROUPS[:staff]]) - if SiteSetting.enable_category_group_moderation? && - group_id = topic.category&.reviewable_by_group_id.presence - group_ids.add(group_id) + if SiteSetting.enable_category_group_moderation? && topic.category + group_ids.merge(topic.category.moderating_group_ids) end if claimed_by.present? diff --git a/app/jobs/regular/notify_reviewable.rb b/app/jobs/regular/notify_reviewable.rb index 8a5c35f641b..ef41a3b0436 100644 --- a/app/jobs/regular/notify_reviewable.rb +++ b/app/jobs/regular/notify_reviewable.rb @@ -20,23 +20,18 @@ class Jobs::NotifyReviewable < ::Jobs::Base all_updates[:admins][r.id] = payload all_updates[:moderators][r.id] = payload if r.reviewable_by_moderator? - all_updates[r.reviewable_by_group_id][r.id] = payload if r.reviewable_by_group_id + + if SiteSetting.enable_category_group_moderation? && r.category.present? + r + .category + .moderating_groups + .pluck(:id) + .each { |group_id| all_updates[group_id][r.id] = payload } + end end end DistributedMutex.synchronize("notify_reviewable_job", validity: 120) do - counts = Hash.new(0) - Reviewable - .default_visible - .pending - .group(:reviewable_by_moderator, :reviewable_by_group_id) - .pluck(:reviewable_by_moderator, :reviewable_by_group_id, "count(*)") - .each do |reviewable_by_moderator, reviewable_by_group_id, count| - counts[:admins] += count - counts[:moderators] += count if reviewable_by_moderator - counts[reviewable_by_group_id] += count if reviewable_by_group_id - end - notify_users(User.real.admins, all_updates[:admins]) if reviewable.reviewable_by_moderator? @@ -46,16 +41,21 @@ class Jobs::NotifyReviewable < ::Jobs::Base ) end - if SiteSetting.enable_category_group_moderation? && (group = reviewable.reviewable_by_group) - users = group.users.includes(:group_users).where("users.id NOT IN (?)", @contacted) + if SiteSetting.enable_category_group_moderation? && reviewable.category.present? + users = + User + .includes(:group_users) + .joins(:group_users) + .joins( + "INNER JOIN category_moderation_groups ON category_moderation_groups.group_id = group_users.group_id", + ) + .where("category_moderation_groups.category_id": reviewable.category.id) + .where("users.id NOT IN (?)", @contacted) + .distinct users.find_each do |user| - count = 0 updates = {} - user.group_users.each do |gu| - updates.merge!(all_updates[gu.group_id]) - count += counts[gu.group_id] - end + user.group_users.each { |gu| updates.merge!(all_updates[gu.group_id]) } notify_user(user, updates) end diff --git a/app/models/about.rb b/app/models/about.rb index d385e19c680..be196db913a 100644 --- a/app/models/about.rb +++ b/app/models/about.rb @@ -88,7 +88,7 @@ class About allowed_cats = Guardian.new(@user).allowed_category_ids return [] if allowed_cats.blank? - cats_with_mods = Category.where.not(reviewable_by_group_id: nil).pluck(:id) + cats_with_mods = Category.joins(:category_moderation_groups).distinct.pluck(:id) category_ids = cats_with_mods & allowed_cats return [] if category_ids.blank? @@ -96,15 +96,32 @@ class About per_cat_limit = category_mods_limit / category_ids.size per_cat_limit = 1 if per_cat_limit < 1 - results = DB.query(<<~SQL, category_ids: category_ids) - SELECT c.id category_id - , (ARRAY_AGG(u.id ORDER BY u.last_seen_at DESC))[:#{per_cat_limit}] user_ids - FROM categories c - JOIN group_users gu ON gu.group_id = c.reviewable_by_group_id - JOIN users u ON u.id = gu.user_id - WHERE c.id IN (:category_ids) - GROUP BY c.id - ORDER BY c.position + results = DB.query(<<~SQL, category_ids:) + WITH moderator_users AS ( + SELECT + cmg.category_id AS category_id, + u.id AS user_id, + u.last_seen_at, + ROW_NUMBER() OVER (PARTITION BY cmg.category_id, u.id ORDER BY u.last_seen_at DESC) as rn + FROM category_moderation_groups cmg + INNER JOIN group_users gu + ON cmg.group_id = gu.group_id + INNER JOIN users u + ON gu.user_id = u.id + WHERE cmg.category_id IN (:category_ids) + ) + SELECT id AS category_id, user_ids + FROM categories + INNER JOIN ( + SELECT + category_id, + (ARRAY_AGG(user_id ORDER BY last_seen_at DESC))[:#{per_cat_limit}] AS user_ids + FROM moderator_users + WHERE rn = 1 + GROUP BY category_id + ) X + ON X.category_id = id + ORDER BY position SQL cats = Category.where(id: results.map(&:category_id)).index_by(&:id) diff --git a/app/models/category.rb b/app/models/category.rb index 493d150e0ee..69ec2996e3c 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -7,6 +7,7 @@ class Category < ActiveRecord::Base :suppress_from_latest, # TODO: Remove when 20240212034010_drop_deprecated_columns has been promoted to pre-deploy :required_tag_group_id, # TODO: Remove when 20240212034010_drop_deprecated_columns has been promoted to pre-deploy :min_tags_from_required_group, # TODO: Remove when 20240212034010_drop_deprecated_columns has been promoted to pre-deploy + :reviewable_by_group_id, ] include Searchable @@ -37,7 +38,9 @@ class Category < ActiveRecord::Base has_many :featured_topics, through: :category_featured_topics, source: :topic has_many :category_groups, dependent: :destroy + has_many :category_moderation_groups, dependent: :destroy has_many :groups, through: :category_groups + has_many :moderating_groups, through: :category_moderation_groups, source: :group has_many :topic_timers, dependent: :destroy has_many :upload_references, as: :target, dependent: :destroy @@ -110,7 +113,6 @@ class Category < ActiveRecord::Base after_save :reset_topic_ids_cache after_save :clear_subcategory_ids after_save :clear_url_cache - after_save :update_reviewables after_save :publish_discourse_stylesheet after_save :publish_category @@ -163,8 +165,6 @@ class Category < ActiveRecord::Base has_many :category_form_templates, dependent: :destroy has_many :form_templates, through: :category_form_templates - belongs_to :reviewable_by_group, class_name: "Group" - scope :latest, -> { order("topic_count DESC") } scope :secured, @@ -1113,10 +1113,8 @@ class Category < ActiveRecord::Base ) end - def update_reviewables - if should_update_reviewables? - Reviewable.where(category_id: id).update_all(reviewable_by_group_id: reviewable_by_group_id) - end + def moderating_group_ids + category_moderation_groups.pluck(:group_id) end def self.find_by_slug_path(slug_path) @@ -1278,10 +1276,6 @@ class Category < ActiveRecord::Base self.build_category_setting if self.category_setting.blank? end - def should_update_reviewables? - SiteSetting.enable_category_group_moderation? && saved_change_to_reviewable_by_group_id? - end - def check_permissions_compatibility(parent_permissions, child_permissions) parent_groups = parent_permissions.map(&:first) @@ -1381,7 +1375,6 @@ end # navigate_to_first_post_after_read :boolean default(FALSE), not null # search_priority :integer default(0) # allow_global_tags :boolean default(FALSE), not null -# reviewable_by_group_id :integer # read_only_banner :string # default_list_filter :string(20) default("all") # allow_unlimited_owner_edits_on_first_post :boolean default(FALSE), not null diff --git a/app/models/category_moderation_group.rb b/app/models/category_moderation_group.rb new file mode 100644 index 00000000000..9cfea9d0691 --- /dev/null +++ b/app/models/category_moderation_group.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class CategoryModerationGroup < ActiveRecord::Base + belongs_to :category + belongs_to :group +end + +# == Schema Information +# +# Table name: category_moderation_groups +# +# id :bigint not null, primary key +# category_id :integer +# group_id :integer +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_category_moderation_groups_on_category_id_and_group_id (category_id,group_id) UNIQUE +# diff --git a/app/models/group.rb b/app/models/group.rb index 25a31bcc181..2b53603b4fa 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -16,6 +16,7 @@ class Group < ActiveRecord::Base self.preloaded_custom_field_names = Set.new has_many :category_groups, dependent: :destroy + has_many :category_moderation_groups, dependent: :destroy has_many :group_users, dependent: :destroy has_many :group_requests, dependent: :destroy has_many :group_mentions, dependent: :destroy @@ -24,15 +25,11 @@ class Group < ActiveRecord::Base has_many :group_archived_messages, dependent: :destroy has_many :categories, through: :category_groups + has_many :moderation_categories, through: :category_moderation_groups, source: :category has_many :users, through: :group_users has_many :human_users, -> { human_users }, through: :group_users, source: :user has_many :requesters, through: :group_requests, source: :user has_many :group_histories, dependent: :destroy - has_many :category_reviews, - class_name: "Category", - foreign_key: :reviewable_by_group_id, - dependent: :nullify - has_many :reviewables, foreign_key: :reviewable_by_group_id, dependent: :nullify has_many :group_category_notification_defaults, dependent: :destroy has_many :group_tag_notification_defaults, dependent: :destroy has_many :associated_groups, through: :group_associated_groups, dependent: :destroy @@ -81,10 +78,6 @@ class Group < ActiveRecord::Base Discourse.cache.delete("group_imap_mailboxes_#{self.id}") end - def remove_review_groups - Category.where(review_group_id: self.id).update_all(review_group_id: nil) - end - validate :name_format_validator validates :name, presence: true validate :automatic_membership_email_domains_format_validator diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index d7ec2260fca..2f3179450ef 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -7,6 +7,8 @@ class Reviewable < ActiveRecord::Base ReviewableUser: BasicReviewableUserSerializer, } + self.ignored_columns = [:reviewable_by_group_id] + class UpdateConflict < StandardError end @@ -17,13 +19,11 @@ class Reviewable < ActiveRecord::Base end end - before_save :apply_review_group attr_accessor :created_new validates_presence_of :type, :status, :created_by_id belongs_to :target, polymorphic: true belongs_to :created_by, class_name: "User" belongs_to :target_created_by, class_name: "User" - belongs_to :reviewable_by_group, class_name: "Group" # Optional, for filtering belongs_to :topic @@ -264,15 +264,6 @@ class Reviewable < ActiveRecord::Base ) end - def apply_review_group - unless SiteSetting.enable_category_group_moderation? && category.present? && - category.reviewable_by_group_id - return - end - - self.reviewable_by_group_id = category.reviewable_by_group_id - end - def actions_for(guardian, args = nil) args ||= {} @@ -408,14 +399,17 @@ class Reviewable < ActiveRecord::Base group_ids = SiteSetting.enable_category_group_moderation? ? user.group_users.pluck(:group_id) : [] - result.where( - "(reviewables.reviewable_by_moderator AND :staff) OR (reviewables.reviewable_by_group_id IN (:group_ids))", - staff: user.staff?, - group_ids: group_ids, - ).where( - "reviewables.category_id IS NULL OR reviewables.category_id IN (?)", - Guardian.new(user).allowed_category_ids, - ) + result + .left_joins(category: :category_moderation_groups) + .where( + "(reviewables.reviewable_by_moderator AND :moderator) OR (category_moderation_groups.group_id IN (:group_ids))", + moderator: user.moderator?, + group_ids: group_ids, + ) + .where( + "reviewables.category_id IS NULL OR reviewables.category_id IN (?)", + Guardian.new(user).allowed_category_ids, + ) end def self.pending_count(user) @@ -768,7 +762,6 @@ end # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null -# reviewable_by_group_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index 55ecae88441..73b380fe8e9 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -337,9 +337,16 @@ class ReviewableFlaggedPost < Reviewable user_ids = User.staff.pluck(:id) - if SiteSetting.enable_category_group_moderation? && - group_id = topic.category&.reviewable_by_group_id.presence - user_ids.concat(GroupUser.where(group_id: group_id).pluck(:user_id)) + if SiteSetting.enable_category_group_moderation? && topic.category + user_ids.concat( + GroupUser + .joins( + "INNER JOIN category_moderation_groups ON category_moderation_groups.group_id = group_users.group_id", + ) + .where("category_moderation_groups.category_id": topic.category.id) + .distinct + .pluck(:user_id), + ) user_ids.uniq! end @@ -388,7 +395,6 @@ end # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null -# reviewable_by_group_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/models/reviewable_post.rb b/app/models/reviewable_post.rb index f9fb8afb37c..4ace91c312d 100644 --- a/app/models/reviewable_post.rb +++ b/app/models/reviewable_post.rb @@ -141,7 +141,6 @@ end # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null -# reviewable_by_group_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/models/reviewable_queued_post.rb b/app/models/reviewable_queued_post.rb index 4e1cbd1507e..76ce590d165 100644 --- a/app/models/reviewable_queued_post.rb +++ b/app/models/reviewable_queued_post.rb @@ -241,7 +241,6 @@ end # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null -# reviewable_by_group_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 299fd648686..12a05027256 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -105,7 +105,6 @@ end # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null -# reviewable_by_group_id :integer # category_id :integer # topic_id :integer # score :float default(0.0), not null diff --git a/app/serializers/category_serializer.rb b/app/serializers/category_serializer.rb index 08b92ac6a5e..28f39fe0deb 100644 --- a/app/serializers/category_serializer.rb +++ b/app/serializers/category_serializer.rb @@ -26,17 +26,13 @@ class CategorySerializer < SiteCategorySerializer :custom_fields, :topic_featured_link_allowed, :search_priority, - :reviewable_by_group_name, + :moderating_group_ids, :default_slow_mode_seconds has_one :category_setting, serializer: CategorySettingSerializer, embed: :objects - def reviewable_by_group_name - object.reviewable_by_group.name - end - - def include_reviewable_by_group_name? - SiteSetting.enable_category_group_moderation? && object.reviewable_by_group_id.present? + def include_moderating_group_ids? + SiteSetting.enable_category_group_moderation? end def include_category_setting? diff --git a/db/migrate/20240828191047_create_category_moderation_groups.rb b/db/migrate/20240828191047_create_category_moderation_groups.rb new file mode 100644 index 00000000000..3028142bb2f --- /dev/null +++ b/db/migrate/20240828191047_create_category_moderation_groups.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class CreateCategoryModerationGroups < ActiveRecord::Migration[7.1] + def change + create_table :category_moderation_groups do |t| + t.integer :category_id + t.integer :group_id + + t.timestamps + end + + add_index :category_moderation_groups, %i[category_id group_id], unique: true + + execute <<~SQL + INSERT INTO category_moderation_groups (category_id, group_id, created_at, updated_at) + SELECT id, reviewable_by_group_id, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP + FROM categories + WHERE reviewable_by_group_id IS NOT NULL + SQL + end +end diff --git a/lib/guardian.rb b/lib/guardian.rb index ddcead590a9..f49f036e03c 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -136,17 +136,14 @@ class Guardian return false if !category return false if !category_group_moderation_allowed? - reviewable_by_group_id = category.reviewable_by_group_id - return false if reviewable_by_group_id.blank? + @group_moderator_categories ||= {} - @category_group_moderator_groups ||= {} - - if @category_group_moderator_groups.key?(reviewable_by_group_id) - @category_group_moderator_groups[reviewable_by_group_id] + if @group_moderator_categories.key?(category.id) + @group_moderator_categories[category.id] else - @category_group_moderator_groups[ - reviewable_by_group_id - ] = category_group_moderator_scope.exists?("categories.id": category.id) + @group_moderator_categories[category.id] = category_group_moderator_scope.exists?( + id: category.id, + ) end end @@ -692,8 +689,9 @@ class Guardian end def category_group_moderator_scope - Category.joins( - "INNER JOIN group_users ON group_users.group_id = categories.reviewable_by_group_id", - ).where("group_users.user_id = ?", user.id) + Category + .joins(:category_moderation_groups) + .joins("INNER JOIN group_users ON group_users.group_id = category_moderation_groups.group_id") + .where("group_users.user_id": user.id) end end diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb index 608ee260bf1..28da0c247b6 100644 --- a/lib/guardian/user_guardian.rb +++ b/lib/guardian/user_guardian.rb @@ -171,8 +171,13 @@ module UserGuardian ( SiteSetting.enable_category_group_moderation && Reviewable - .where(reviewable_by_group_id: @user.group_users.pluck(:group_id)) - .where("category_id IS NULL or category_id IN (?)", allowed_category_ids) + .joins( + "INNER JOIN category_moderation_groups ON category_moderation_groups.category_id = reviewables.category_id", + ) + .where( + category_id: allowed_category_ids, + "category_moderation_groups.group_id": @user.group_users.pluck(:group_id), + ) .exists? ) end diff --git a/lib/post_action_creator.rb b/lib/post_action_creator.rb index 5e5e2b86ecf..41f2432c413 100644 --- a/lib/post_action_creator.rb +++ b/lib/post_action_creator.rb @@ -364,9 +364,13 @@ class PostActionCreator create_args[:subtype] = TopicSubtype.notify_moderators create_args[:target_group_names] = [Group[:moderators].name] - if SiteSetting.enable_category_group_moderation? && - @post.topic&.category&.reviewable_by_group_id? - create_args[:target_group_names] << @post.topic.category.reviewable_by_group.name + if SiteSetting.enable_category_group_moderation? && @post.topic&.category + create_args[:target_group_names].push( + *Group + .joins(:category_moderation_groups) + .where("category_moderation_groups.category_id": @post.topic.category.id) + .pluck(:name), + ) end end diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 000464d0f62..f2df2c26b3d 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -535,16 +535,19 @@ class TopicView def category_group_moderator_user_ids @category_group_moderator_user_ids ||= begin - if SiteSetting.enable_category_group_moderation? && - @topic.category&.reviewable_by_group.present? + if SiteSetting.enable_category_group_moderation? && @topic.category.present? posts_user_ids = Set.new(@posts.map(&:user_id)) Set.new( - @topic - .category - .reviewable_by_group - .group_users - .where(user_id: posts_user_ids) - .pluck("distinct user_id"), + GroupUser + .joins( + "INNER JOIN category_moderation_groups ON category_moderation_groups.group_id = group_users.group_id", + ) + .where( + "category_moderation_groups.category_id": @topic.category.id, + user_id: posts_user_ids, + ) + .distinct + .pluck(:user_id), ) else Set.new diff --git a/plugins/chat/spec/lib/chat/guardian_extensions_spec.rb b/plugins/chat/spec/lib/chat/guardian_extensions_spec.rb index 3bd12d2ae04..3b60bc1766d 100644 --- a/plugins/chat/spec/lib/chat/guardian_extensions_spec.rb +++ b/plugins/chat/spec/lib/chat/guardian_extensions_spec.rb @@ -354,11 +354,11 @@ RSpec.describe Chat::GuardianExtensions do context "when enable_category_group_moderation is true" do before { SiteSetting.enable_category_group_moderation = true } - it "returns true if the regular user is part of the reviewable_by_group for the category" do + it "returns true if the regular user is part of the reviewable groups for the category" do moderator = Fabricate(:user) mods = Fabricate(:group) mods.add(moderator) - category.update!(reviewable_by_group: mods) + Fabricate(:category_moderation_group, category:, group: mods) expect(Guardian.new(Fabricate(:admin)).can_moderate_chat?(channel.chatable)).to eq(true) expect(Guardian.new(moderator).can_moderate_chat?(channel.chatable)).to eq(true) end @@ -443,7 +443,7 @@ RSpec.describe Chat::GuardianExtensions do moderator = Fabricate(:user) mods = Fabricate(:group) mods.add(moderator) - chatable.update!(reviewable_by_group: mods) + Fabricate(:category_moderation_group, category: chatable, group: mods) expect(Guardian.new(moderator).can_restore_chat?(message, chatable)).to eq(true) end end diff --git a/plugins/chat/spec/system/move_message_to_channel_spec.rb b/plugins/chat/spec/system/move_message_to_channel_spec.rb index 85d48fa2524..0648be9d298 100644 --- a/plugins/chat/spec/system/move_message_to_channel_spec.rb +++ b/plugins/chat/spec/system/move_message_to_channel_spec.rb @@ -28,12 +28,14 @@ RSpec.describe "Move message to channel", type: :system do fab!(:group_1) { Fabricate(:group) } fab!(:channel_1) { Fabricate(:private_category_channel, group: group_1) } fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) } + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category: channel_1.category, group: group_1) + end before do SiteSetting.enable_category_group_moderation = true group_1.add(current_user) channel_1.add(current_user) - channel_1.chatable.update!(reviewable_by_group_id: group_1.id) end it "is available" do diff --git a/plugins/discourse-presence/plugin.rb b/plugins/discourse-presence/plugin.rb index f61fc12bd76..e3c4c430c02 100644 --- a/plugins/discourse-presence/plugin.rb +++ b/plugins/discourse-presence/plugin.rb @@ -59,9 +59,8 @@ after_initialize do config.allowed_group_ids += SiteSetting.edit_all_post_groups.split("|").map(&:to_i) end - if SiteSetting.enable_category_group_moderation? && - group_id = topic.category&.reviewable_by_group_id - config.allowed_group_ids << group_id + if SiteSetting.enable_category_group_moderation? && topic.category + config.allowed_group_ids.push(*topic.category.moderating_groups.pluck(:id)) end config diff --git a/plugins/discourse-presence/spec/integration/presence_spec.rb b/plugins/discourse-presence/spec/integration/presence_spec.rb index 66b7f9776f0..6a88101dcde 100644 --- a/plugins/discourse-presence/spec/integration/presence_spec.rb +++ b/plugins/discourse-presence/spec/integration/presence_spec.rb @@ -73,7 +73,7 @@ RSpec.describe "discourse-presence" do expect(c.config.allowed_group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff]) SiteSetting.enable_category_group_moderation = true - category.update(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category:, group:) c = PresenceChannel.new("/discourse-presence/edit/#{p.id}", use_cache: false) expect(c.config.allowed_group_ids).to contain_exactly(Group::AUTO_GROUPS[:staff], group.id) diff --git a/spec/fabricators/category_moderation_group_fabricator.rb b/spec/fabricators/category_moderation_group_fabricator.rb new file mode 100644 index 00000000000..4ec6498110b --- /dev/null +++ b/spec/fabricators/category_moderation_group_fabricator.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +Fabricator(:category_moderation_group) do + category + group +end diff --git a/spec/jobs/notify_reviewable_spec.rb b/spec/jobs/notify_reviewable_spec.rb index 08a737f55dd..3b43ac103ef 100644 --- a/spec/jobs/notify_reviewable_spec.rb +++ b/spec/jobs/notify_reviewable_spec.rb @@ -54,8 +54,9 @@ RSpec.describe Jobs::NotifyReviewable do moderator.update!(last_seen_reviewable_id: moderator_reviewable.id) # Content for a group - group_reviewable = - Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) + group_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, category:) messages = MessageBus.track_publish { described_class.new.execute(reviewable_id: group_reviewable.id) } @@ -85,7 +86,9 @@ RSpec.describe Jobs::NotifyReviewable do SiteSetting.enable_category_group_moderation = false GroupUser.create!(group_id: group.id, user_id: moderator.id) - reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) + reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, category:) messages = MessageBus.track_publish("/reviewable_counts") do diff --git a/spec/jobs/refresh_users_reviewable_counts_spec.rb b/spec/jobs/refresh_users_reviewable_counts_spec.rb index 379a9b9efe6..e10d2f5db44 100644 --- a/spec/jobs/refresh_users_reviewable_counts_spec.rb +++ b/spec/jobs/refresh_users_reviewable_counts_spec.rb @@ -5,20 +5,16 @@ RSpec.describe Jobs::RefreshUsersReviewableCounts do fab!(:moderator) fab!(:user) - fab!(:group) - fab!(:topic) - fab!(:reviewable1) do - Fabricate(:reviewable, reviewable_by_group: group, reviewable_by_moderator: true, topic: topic) - end + fab!(:group) { Fabricate(:group, users: [user]) } + fab!(:category) + fab!(:topic) { Fabricate(:topic, category:) } + fab!(:category_moderation_group) { Fabricate(:category_moderation_group, category:, group:) } + fab!(:reviewable1) { Fabricate(:reviewable, reviewable_by_moderator: true, topic:, category:) } fab!(:reviewable2) { Fabricate(:reviewable, reviewable_by_moderator: false) } fab!(:reviewable3) { Fabricate(:reviewable, reviewable_by_moderator: true) } - before do - SiteSetting.enable_category_group_moderation = true - group.add(user) - topic.category.update!(reviewable_by_group: group) - end + before { SiteSetting.enable_category_group_moderation = true } describe "#execute" do it "publishes reviewable counts for the members of the specified groups" do diff --git a/spec/lib/guardian/topic_guardian_spec.rb b/spec/lib/guardian/topic_guardian_spec.rb index 7c5bd92315b..d81aa81caf3 100644 --- a/spec/lib/guardian/topic_guardian_spec.rb +++ b/spec/lib/guardian/topic_guardian_spec.rb @@ -83,7 +83,7 @@ RSpec.describe TopicGuardian do it "returns true for group moderator" do SiteSetting.enable_category_group_moderation = true expect(Guardian.new(user).can_see_deleted_topics?(topic.category)).to eq(false) - category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category:, group:) group.add(user) topic.update!(category: category) expect(Guardian.new(user).can_see_deleted_topics?(topic.category)).to eq(true) @@ -110,7 +110,7 @@ RSpec.describe TopicGuardian do it "returns true for group moderator" do SiteSetting.enable_category_group_moderation = true expect(Guardian.new(user).can_recover_topic?(Topic.with_deleted.last)).to eq(false) - category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category:, group:) group.add(user) topic.update!(category: category) expect(Guardian.new(user).can_recover_topic?(Topic.with_deleted.last)).to eq(true) @@ -255,7 +255,7 @@ RSpec.describe TopicGuardian do it "returns the topic ids for topics which are deleted but user is a category moderator of" do SiteSetting.enable_category_group_moderation = true - category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category:, group:) group.add(user) topic.update!(category: category) topic.trash!(admin) diff --git a/spec/lib/guardian/user_guardian_spec.rb b/spec/lib/guardian/user_guardian_spec.rb index fc83c031669..bb663474eb4 100644 --- a/spec/lib/guardian/user_guardian_spec.rb +++ b/spec/lib/guardian/user_guardian_spec.rb @@ -415,8 +415,10 @@ RSpec.describe UserGuardian do group.add(user) guardian = Guardian.new(user) SiteSetting.enable_category_group_moderation = true + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) - Fabricate(:reviewable_flagged_post, reviewable_by_group: group, category: nil) + Fabricate(:reviewable_flagged_post, category:) expect(guardian.can_see_review_queue?).to eq(true) end @@ -426,8 +428,10 @@ RSpec.describe UserGuardian do group.add(user) guardian = Guardian.new(user) SiteSetting.enable_category_group_moderation = false + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) - Fabricate(:reviewable_flagged_post, reviewable_by_group: group, category: nil) + Fabricate(:reviewable_flagged_post, category:) expect(guardian.can_see_review_queue?).to eq(false) end @@ -438,8 +442,9 @@ RSpec.describe UserGuardian do guardian = Guardian.new(user) SiteSetting.enable_category_group_moderation = true category = Fabricate(:category, read_restricted: true) + Fabricate(:category_moderation_group, category:, group:) - Fabricate(:reviewable_flagged_post, reviewable_by_group: group, category: category) + Fabricate(:reviewable_flagged_post, category: category) expect(guardian.can_see_review_queue?).to eq(false) end diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index deba513b000..7c81ac4f7a8 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -1072,7 +1072,8 @@ RSpec.describe Guardian do 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) + topic.category.update!(topic_id: post.topic.id) + Fabricate(:category_moderation_group, category: topic.category, group:) expect(Guardian.new(user_gm).can_see?(topic)).to be_truthy end @@ -1130,7 +1131,8 @@ RSpec.describe Guardian do 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) + post.topic.category.update!(topic_id: post.topic.id) + Fabricate(:category_moderation_group, category: post.topic.category, group:) expect(Guardian.new(user_gm).can_see?(post)).to be_truthy end @@ -1504,7 +1506,7 @@ RSpec.describe Guardian do 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) + Fabricate(:category_moderation_group, category: topic.category, group: group_user.group) expect(Guardian.new(group_user.user).can_recover_topic?(topic)).to be_truthy end @@ -1787,7 +1789,7 @@ RSpec.describe Guardian do before do SiteSetting.enable_category_group_moderation = true GroupUser.create!(group_id: group.id, user_id: cat_mod_user.id) - post.topic.category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category: post.topic.category, group:) end it "returns true as a category group moderator user" do @@ -2161,7 +2163,7 @@ RSpec.describe Guardian do it "returns true for a group member with reviewable status" do SiteSetting.enable_category_group_moderation = true GroupUser.create!(group_id: group.id, user_id: user.id) - topic.category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category: topic.category, group:) expect(Guardian.new(user).can_review_topic?(topic)).to eq(true) end end @@ -2182,7 +2184,7 @@ RSpec.describe Guardian do it "returns true for a group member with reviewable status" do SiteSetting.enable_category_group_moderation = true GroupUser.create!(group_id: group.id, user_id: user.id) - topic.category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category: topic.category, group:) expect(Guardian.new(user).can_close_topic?(topic)).to eq(true) end end @@ -2203,7 +2205,7 @@ RSpec.describe Guardian do it "returns true for a group member with reviewable status" do SiteSetting.enable_category_group_moderation = true GroupUser.create!(group_id: group.id, user_id: user.id) - topic.category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category: topic.category, group:) expect(Guardian.new(user).can_archive_topic?(topic)).to eq(true) end end @@ -2224,7 +2226,7 @@ RSpec.describe Guardian do it "returns true for a group member with reviewable status" do SiteSetting.enable_category_group_moderation = true GroupUser.create!(group_id: group.id, user_id: user.id) - topic.category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category: topic.category, group:) expect(Guardian.new(user).can_edit_staff_notes?(topic)).to eq(true) end end @@ -2348,7 +2350,7 @@ RSpec.describe Guardian do 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) + Fabricate(:category_moderation_group, category: topic.category, group: group_user.group) expect(Guardian.new(group_user.user).can_delete?(topic)).to be_truthy end @@ -2411,8 +2413,9 @@ RSpec.describe Guardian do it "returns true for category moderators" do SiteSetting.enable_category_group_moderation = true GroupUser.create(group: group, user: user) - category = Fabricate(:category, reviewable_by_group_id: group.id) + category = Fabricate(:category) post.topic.update!(category: category) + Fabricate(:category_moderation_group, category:, group:) expect(Guardian.new(user).can_delete?(post)).to eq(true) end @@ -4399,7 +4402,7 @@ RSpec.describe Guardian do it "should correctly detect category moderation" do group.add(user) - category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category:, group:) guardian = Guardian.new(user) # implementation detail, ensure memoization is good (hence testing twice) diff --git a/spec/lib/new_post_manager_spec.rb b/spec/lib/new_post_manager_spec.rb index 3695cd1594b..9c2d0d63d8d 100644 --- a/spec/lib/new_post_manager_spec.rb +++ b/spec/lib/new_post_manager_spec.rb @@ -490,9 +490,12 @@ RSpec.describe NewPostManager do end context "when posting in the category requires approval" do - let!(:user) { Fabricate(:user, refresh_auto_groups: true) } - let!(:review_group) { Fabricate(:group) } - let!(:category) { Fabricate(:category, reviewable_by_group_id: review_group.id) } + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } + fab!(:review_group) { Fabricate(:group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: review_group) + end context "when new topics require approval" do before do diff --git a/spec/lib/post_destroyer_spec.rb b/spec/lib/post_destroyer_spec.rb index dcd0e1d8eec..1d8705fc1ca 100644 --- a/spec/lib/post_destroyer_spec.rb +++ b/spec/lib/post_destroyer_spec.rb @@ -306,7 +306,8 @@ RSpec.describe PostDestroyer do before do SiteSetting.enable_category_group_moderation = true review_group = Fabricate(:group) - review_category = Fabricate(:category, reviewable_by_group_id: review_group.id) + review_category = Fabricate(:category) + Fabricate(:category_moderation_group, category: review_category, group: review_group) @reply.topic.update!(category: review_category) review_group.users << review_user end @@ -552,7 +553,8 @@ RSpec.describe PostDestroyer do before do SiteSetting.enable_category_group_moderation = true review_group = Fabricate(:group) - review_category = Fabricate(:category, reviewable_by_group_id: review_group.id) + review_category = Fabricate(:category) + Fabricate(:category_moderation_group, category: review_category, group: review_group) post.topic.update!(category: review_category) review_group.users << review_user end diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index 7c669e52a66..722e175f7ae 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -1091,8 +1091,9 @@ RSpec.describe PostRevisor do context "when logging group moderator edits" do fab!(:group_user) - fab!(:category) do - Fabricate(:category, reviewable_by_group_id: group_user.group.id, topic: topic) + fab!(:category) { Fabricate(:category, topic: topic) } + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) end before do diff --git a/spec/lib/topic_query_spec.rb b/spec/lib/topic_query_spec.rb index 5325f8ffd1f..b5e0f5ae39c 100644 --- a/spec/lib/topic_query_spec.rb +++ b/spec/lib/topic_query_spec.rb @@ -308,7 +308,8 @@ RSpec.describe TopicQuery do group_moderator = Fabricate(:user) group = Fabricate(:group) group.add(group_moderator) - category = Fabricate(:category, reviewable_by_group: group) + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) _topic = Fabricate(:topic, category: category, deleted_at: 1.year.ago) expect(TopicQuery.new(admin, status: "deleted").list_latest.topics.size).to eq(1) diff --git a/spec/lib/validators/post_validator_spec.rb b/spec/lib/validators/post_validator_spec.rb index 256d75d9c74..0ec5f947627 100644 --- a/spec/lib/validators/post_validator_spec.rb +++ b/spec/lib/validators/post_validator_spec.rb @@ -386,7 +386,8 @@ RSpec.describe PostValidator do SiteSetting.enable_category_group_moderation = true group = Fabricate(:group) GroupUser.create(group: group, user: user) - category = Fabricate(:category, reviewable_by_group_id: group.id) + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) topic.update!(category: category) Post.create!(user: other_user, topic: topic, raw: "post number 1", post_number: 1) diff --git a/spec/models/about_spec.rb b/spec/models/about_spec.rb index bf70caeef71..6a1564e0e9d 100644 --- a/spec/models/about_spec.rb +++ b/spec/models/about_spec.rb @@ -67,31 +67,28 @@ RSpec.describe About do end describe "#category_moderators" do - let(:user) { Fabricate(:user) } - let(:public_cat_moderator) { Fabricate(:user, last_seen_at: 1.month.ago) } - let(:private_cat_moderator) { Fabricate(:user, last_seen_at: 2.month.ago) } - let(:common_moderator) { Fabricate(:user, last_seen_at: 3.month.ago) } - let(:common_moderator_2) { Fabricate(:user, last_seen_at: 4.month.ago) } + fab!(:user) + fab!(:public_cat_moderator) { Fabricate(:user, last_seen_at: 1.month.ago) } + fab!(:private_cat_moderator) { Fabricate(:user, last_seen_at: 2.month.ago) } + fab!(:common_moderator) { Fabricate(:user, last_seen_at: 3.month.ago) } + fab!(:common_moderator_2) { Fabricate(:user, last_seen_at: 4.month.ago) } - let(:public_group) do - group = Fabricate(:public_group) - group.add(public_cat_moderator) - group.add(common_moderator) - group.add(common_moderator_2) - group + fab!(:public_group) do + Fabricate(:public_group, users: [public_cat_moderator, common_moderator, common_moderator_2]) end - let(:private_group) do - group = Fabricate(:group) - group.add(private_cat_moderator) - group.add(common_moderator) - group.add(common_moderator_2) - group + fab!(:private_group) do + Fabricate(:group, users: [private_cat_moderator, common_moderator, common_moderator_2]) end - let!(:public_cat) { Fabricate(:category, reviewable_by_group: public_group) } - let!(:private_cat) do - Fabricate(:private_category, group: private_group, reviewable_by_group: private_group) + fab!(:public_cat) { Fabricate(:category) } + fab!(:public_category_moderation_group) do + Fabricate(:category_moderation_group, category: public_cat, group: public_group) + end + + fab!(:private_cat) { Fabricate(:private_category, group: private_group) } + fab!(:private_category_moderation_group) do + Fabricate(:category_moderation_group, category: private_cat, group: private_group) end it "lists moderators of the category that the current user can see" do @@ -120,6 +117,19 @@ RSpec.describe About do expect(results.size).to eq(2) results.each { |res| expect(res.moderators.size).to eq(2) } end + + it "doesn't list the same user twice as a category mod if the user is member of multiple groups" do + Fabricate(:category_moderation_group, category: public_cat, group: private_group) + + results = About.new(nil).category_moderators + mods = results.find { |r| r.category.id == public_cat.id }.moderators + expect(mods.map(&:id)).to contain_exactly( + public_cat_moderator.id, + common_moderator.id, + common_moderator_2.id, + private_cat_moderator.id, + ) + end end describe "#admins" do diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 751efb4724d..e1efb376782 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -82,51 +82,19 @@ RSpec.describe Category do end end - describe "#review_group_id" do + describe "#category_moderation_groups" do fab!(:group) - fab!(:category) { Fabricate(:category_with_definition, reviewable_by_group: group) } + fab!(:category) { Fabricate(:category_with_definition) } fab!(:topic) { Fabricate(:topic, category: category) } fab!(:post) { Fabricate(:post, topic: topic) } + fab!(:category_moderation_group) { Fabricate(:category_moderation_group, category:, group:) } fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } - it "will add the group to the reviewable" do - SiteSetting.enable_category_group_moderation = true - reviewable = PostActionCreator.spam(user, post).reviewable - expect(reviewable.reviewable_by_group_id).to eq(group.id) - end - - it "will add the group to the reviewable even if created manually" do - SiteSetting.enable_category_group_moderation = true - reviewable = - ReviewableFlaggedPost.create!( - created_by: user, - payload: { - raw: "test raw", - }, - category: category, - ) - expect(reviewable.reviewable_by_group_id).to eq(group.id) - end - - it "will not add add the group to the reviewable" do - SiteSetting.enable_category_group_moderation = false - reviewable = PostActionCreator.spam(user, post).reviewable - expect(reviewable.reviewable_by_group_id).to be_nil - end - - it "will nullify the group_id if destroyed" do + it "is destroyed if the group is destroyed" do + expect(category.category_moderation_groups).to contain_exactly(category_moderation_group) reviewable = PostActionCreator.spam(user, post).reviewable group.destroy - expect(category.reload.reviewable_by_group).to be_blank - expect(reviewable.reload.reviewable_by_group_id).to be_blank - end - - it "will remove the reviewable_by_group if the category is updated" do - SiteSetting.enable_category_group_moderation = true - reviewable = PostActionCreator.spam(user, post).reviewable - category.reviewable_by_group_id = nil - category.save! - expect(reviewable.reload.reviewable_by_group_id).to be_nil + expect(category.reload.category_moderation_groups).to be_blank end end diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 02f4d8f6581..01c31448dea 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -99,7 +99,7 @@ RSpec.describe PostAction do before do SiteSetting.enable_category_group_moderation = true group.update!(messageable_level: Group::ALIAS_LEVELS[:nobody]) - post.topic.category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category: post.topic.category, group:) end it "notifies via pm" do diff --git a/spec/models/reviewable_spec.rb b/spec/models/reviewable_spec.rb index b4381d65839..de197304323 100644 --- a/spec/models/reviewable_spec.rb +++ b/spec/models/reviewable_spec.rb @@ -131,7 +131,9 @@ RSpec.describe Reviewable, type: :model do it "works with the reviewable by group" do SiteSetting.enable_category_group_moderation = true group = Fabricate(:group) - reviewable.reviewable_by_group_id = group.id + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) + reviewable.category_id = category.id reviewable.save! expect(Reviewable.list_for(user, status: :pending)).to be_empty @@ -147,7 +149,9 @@ RSpec.describe Reviewable, type: :model do it "doesn't allow review by group when disabled" do SiteSetting.enable_category_group_moderation = false group = Fabricate(:group) - reviewable.reviewable_by_group_id = group.id + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) + reviewable.category_id = category.id reviewable.save! GroupUser.create!(group_id: group.id, user_id: user.id) @@ -266,12 +270,12 @@ RSpec.describe Reviewable, type: :model do fab!(:admin) fab!(:moderator) fab!(:group) + fab!(:category) fab!(:user) { Fabricate(:user, groups: [group]) } fab!(:admin_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false) } fab!(:mod_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: true) } - fab!(:group_reviewable) do - Fabricate(:reviewable, reviewable_by_group: group, reviewable_by_moderator: false) - end + fab!(:category_moderation_group) { Fabricate(:category_moderation_group, category:, group:) } + fab!(:group_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false, category:) } context "for admins" do it "returns a list of pending reviewables that haven't been seen by the user" do @@ -638,12 +642,12 @@ RSpec.describe Reviewable, type: :model do describe ".unseen_reviewable_count" do fab!(:group) + fab!(:category) fab!(:user) fab!(:admin_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false) } fab!(:mod_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: true) } - fab!(:group_reviewable) do - Fabricate(:reviewable, reviewable_by_moderator: false, reviewable_by_group: group) - end + fab!(:category_moderation_group) { Fabricate(:category_moderation_group, category:, group:) } + fab!(:group_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false, category:) } it "doesn't include reviewables that can't be seen by the user" do SiteSetting.enable_category_group_moderation = true diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d5ce85c83d9..6e50f172259 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3407,8 +3407,9 @@ RSpec.describe User do user.update!(groups: [group]) SiteSetting.enable_category_group_moderation = true - group_reviewable = - Fabricate(:reviewable, reviewable_by_moderator: false, reviewable_by_group: group) + category = Fabricate(:category) + Fabricate(:category_moderation_group, category:, group:) + group_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false, category:) mod_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true) admin_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false) diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 5d0a23bbf8d..3c29bf0dd29 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -517,7 +517,7 @@ RSpec.describe CategoriesController do slug: "hello-cat", auto_close_hours: 72, search_priority: Searchable::PRIORITIES[:ignore], - reviewable_by_group_name: group.name, + moderating_group_ids: [group.id], permissions: { "everyone" => readonly, "staff" => create_post, @@ -527,7 +527,7 @@ RSpec.describe CategoriesController do expect(response.status).to eq(200) cat_json = response.parsed_body["category"] expect(cat_json).to be_present - expect(cat_json["reviewable_by_group_name"]).to eq(group.name) + expect(cat_json["moderating_group_ids"]).to eq([group.id]) expect(cat_json["name"]).to eq("hello") expect(cat_json["slug"]).to eq("hello-cat") expect(cat_json["color"]).to eq("ff0") @@ -634,6 +634,10 @@ RSpec.describe CategoriesController do end describe "#update" do + fab!(:mod_group_1) { Fabricate(:group) } + fab!(:mod_group_2) { Fabricate(:group) } + fab!(:mod_group_3) { Fabricate(:group) } + before { Jobs.run_immediately! } it "requires the user to be logged in" do @@ -871,6 +875,47 @@ RSpec.describe CategoriesController do expect(category.custom_fields).to eq({ "field_1" => "hi" }) expect(category.form_template_ids.count).to eq(0) end + + it "doesn't set category moderation groups if the enable_category_group_moderation setting is false" do + SiteSetting.enable_category_group_moderation = false + + put "/categories/#{category.id}.json", params: { moderating_group_ids: [mod_group_1.id] } + expect(response.status).to eq(200) + expect(category.reload.moderating_groups).to be_blank + end + + it "sets category moderation groups if the enable_category_group_moderation setting is true" do + SiteSetting.enable_category_group_moderation = true + + put "/categories/#{category.id}.json", params: { moderating_group_ids: [mod_group_1.id] } + expect(response.status).to eq(200) + expect(category.reload.moderating_groups).to contain_exactly(mod_group_1) + end + + it "removes category moderation groups and adds groups according to the moderating_group_ids param" do + SiteSetting.enable_category_group_moderation = true + + category.update!(moderating_group_ids: [mod_group_2.id]) + expect(category.reload.moderating_groups).to contain_exactly(mod_group_2) + + put "/categories/#{category.id}.json", + params: { + moderating_group_ids: [mod_group_1.id, mod_group_3.id], + } + expect(response.status).to eq(200) + expect(category.reload.moderating_groups).to contain_exactly(mod_group_1, mod_group_3) + end + + it "can remove all category moderation groups" do + SiteSetting.enable_category_group_moderation = true + + category.update!(moderating_group_ids: [mod_group_2.id, mod_group_1.id]) + expect(category.reload.moderating_groups).to contain_exactly(mod_group_2, mod_group_1) + + put "/categories/#{category.id}.json", params: { moderating_group_ids: [] } + expect(response.status).to eq(200) + expect(category.reload.moderating_groups).to be_blank + end end end end diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index c699b4cf7f9..065b9091d72 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -59,7 +59,8 @@ RSpec.shared_examples "finding and showing post" do end it "can find posts in the allowed category" do - post.topic.category.update!(reviewable_by_group_id: group.id, topic_id: topic.id) + post.topic.category.update!(topic_id: topic.id) + Fabricate(:category_moderation_group, category: post.topic.category, group:) get url expect(response.status).to eq(200) end @@ -602,7 +603,8 @@ RSpec.describe PostsController do before do SiteSetting.enable_category_group_moderation = true - post.topic.category.update!(reviewable_by_group_id: group.id, topic_id: topic.id) + Fabricate(:category_moderation_group, category: post.topic.category, group:) + post.topic.category.update!(topic_id: topic.id) sign_in(user_gm) end @@ -2880,7 +2882,7 @@ RSpec.describe PostsController do before do SiteSetting.enable_category_group_moderation = true - topic.category.update!(reviewable_by_group_id: group.id) + Fabricate(:category_moderation_group, category: topic.category, group:) sign_in(user) end @@ -2903,8 +2905,7 @@ RSpec.describe PostsController do end it "prevents a group moderator from altering notes outside of their category" do - moderatable_group = Fabricate(:group) - topic.category.update!(reviewable_by_group_id: moderatable_group.id) + topic.category.category_moderation_groups.where(group:).delete_all put "/posts/#{public_post.id}/notice.json", params: { notice: "Hello" } diff --git a/spec/requests/reviewable_claimed_topics_controller_spec.rb b/spec/requests/reviewable_claimed_topics_controller_spec.rb index d9b848deb8e..fe39f034a15 100644 --- a/spec/requests/reviewable_claimed_topics_controller_spec.rb +++ b/spec/requests/reviewable_claimed_topics_controller_spec.rb @@ -53,7 +53,7 @@ RSpec.describe ReviewableClaimedTopicsController do SiteSetting.reviewable_claiming = "optional" group = Fabricate(:group) - topic.category.update!(reviewable_by_group: group) + Fabricate(:category_moderation_group, category: topic.category, group:) messages = MessageBus.track_publish("/reviewable_claimed") do @@ -106,8 +106,7 @@ RSpec.describe ReviewableClaimedTopicsController do not_notified = Fabricate(:user) group = Fabricate(:group) - topic.category.update!(reviewable_by_group: group) - reviewable.update!(reviewable_by_group: group) + Fabricate(:category_moderation_group, category: topic.category, group:) notified = Fabricate(:user) group.add(notified) @@ -190,8 +189,7 @@ RSpec.describe ReviewableClaimedTopicsController do not_notified = Fabricate(:user) group = Fabricate(:group) - topic.category.update!(reviewable_by_group: group) - reviewable.update!(reviewable_by_group: group) + Fabricate(:category_moderation_group, category: topic.category, group:) notified = Fabricate(:user) group.add(notified) diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 94eed15ed3b..782fafb702f 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -233,7 +233,10 @@ RSpec.describe TopicsController do end describe "moving to a new topic as a group moderator" do - fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) + end fab!(:topic) { Fabricate(:topic, category: category) } fab!(:p1) { Fabricate(:post, user: group_user.user, post_number: 1, topic: topic) } fab!(:p2) { Fabricate(:post, user: group_user.user, post_number: 2, topic: topic) } @@ -388,7 +391,10 @@ RSpec.describe TopicsController do end describe "moving to an existing topic as a group moderator" do - fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) + end fab!(:topic) { Fabricate(:topic, category: category) } fab!(:p1) { Fabricate(:post, user: group_user.user, post_number: 1, topic: topic) } fab!(:p2) { Fabricate(:post, user: group_user.user, post_number: 2, topic: topic) } @@ -439,7 +445,10 @@ RSpec.describe TopicsController do end describe "moving chronologically to an existing topic as a group moderator" do - fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) + end fab!(:topic) { Fabricate(:topic, category: category) } fab!(:p1) do Fabricate( @@ -715,7 +724,10 @@ RSpec.describe TopicsController do end describe "merging into another topic as a group moderator" do - fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) + end fab!(:topic) { Fabricate(:topic, category: category) } fab!(:p1) { Fabricate(:post, user: post_author1, post_number: 1, topic: topic) } fab!(:p2) { Fabricate(:post, user: post_author2, post_number: 2, topic: topic) } @@ -752,7 +764,10 @@ RSpec.describe TopicsController do end describe "merging chronologically into another topic as a group moderator" do - fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) + end fab!(:topic) { Fabricate(:topic, category: category) } fab!(:p1) do Fabricate( @@ -1152,7 +1167,10 @@ RSpec.describe TopicsController do end describe "when logged in as a group member with reviewable status" do - fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) + end fab!(:topic) { Fabricate(:topic, category: category) } before do @@ -4970,7 +4988,7 @@ RSpec.describe TopicsController do it "allows a category moderator to create a delete timer" do user.update!(trust_level: TrustLevel[4]) Group.user_trust_level_change!(user.id, user.trust_level) - topic.category.update!(reviewable_by_group: user.groups.first) + Fabricate(:category_moderation_group, category: topic.category, group: user.groups.first) sign_in(user) diff --git a/spec/serializers/category_serializer_spec.rb b/spec/serializers/category_serializer_spec.rb index 20c03fde6e0..f545e66d2e9 100644 --- a/spec/serializers/category_serializer_spec.rb +++ b/spec/serializers/category_serializer_spec.rb @@ -4,18 +4,19 @@ RSpec.describe CategorySerializer do fab!(:user) fab!(:admin) fab!(:group) - fab!(:category) { Fabricate(:category, reviewable_by_group_id: group.id) } + fab!(:category) + fab!(:category_moderation_group) { Fabricate(:category_moderation_group, category:, group:) } it "includes the reviewable by group name if enabled" do SiteSetting.enable_category_group_moderation = true json = described_class.new(category, scope: Guardian.new, root: false).as_json - expect(json[:reviewable_by_group_name]).to eq(group.name) + expect(json[:moderating_group_ids]).to eq([group.id]) end it "doesn't include the reviewable by group name if disabled" do SiteSetting.enable_category_group_moderation = false json = described_class.new(category, scope: Guardian.new, root: false).as_json - expect(json[:reviewable_by_group_name]).to be_blank + expect(json[:moderating_group_ids]).to be_blank end it "includes custom fields" do diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 68de390caf9..73be5f6f1bc 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -284,12 +284,12 @@ RSpec.describe PostSerializer do fab!(:topic) fab!(:group_user) fab!(:post) { Fabricate(:post, topic: topic) } - - before do - SiteSetting.enable_category_group_moderation = true - topic.category.update!(reviewable_by_group_id: group_user.group.id) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category: topic.category, group: group_user.group) end + before { SiteSetting.enable_category_group_moderation = true } + it "does nothing for regular users" do expect(serialized_post_for_user(nil)[:group_moderator]).to eq(nil) end diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index 0b2bb245214..62b16e0f5a6 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -454,7 +454,10 @@ RSpec.describe TopicViewSerializer do context "with can_edit" do fab!(:group_user) - fab!(:category) { Fabricate(:category, reviewable_by_group: group_user.group) } + fab!(:category) + fab!(:category_moderation_group) do + Fabricate(:category_moderation_group, category:, group: group_user.group) + end fab!(:topic) { Fabricate(:topic, category: category) } let(:user) { group_user.user }