From e82e255531b4e17ab06a92bfb3cd2cc61074658a Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Wed, 14 Aug 2024 12:13:46 +1000 Subject: [PATCH] FIX: serialize Flags instead of PostActionType (#28362) ### Why? Before, all flags were static. Therefore, they were stored in class variables and serialized by SiteSerializer. Recently, we added an option for admins to add their own flags or disable existing flags. Therefore, the class variable had to be dropped because it was unsafe for a multisite environment. However, it started causing performance problems. ### Solution When a new Flag system is used, instead of using PostActionType, we can serialize Flags and use fragment cache for performance reasons. At the same time, we are still supporting deprecated `replace_flags` API call. When it is used, we fall back to the old solution and the admin cannot add custom flags. In a couple of months, we will be able to drop that API function and clean that code properly. However, because it may still be used, redis cache was introduced to improve performance. To test backward compatibility you can add this code to any plugin ```ruby replace_flags do |flag_settings| flag_settings.add( 4, :inappropriate, topic_type: true, notify_type: true, auto_action_type: true, ) flag_settings.add(1001, :trolling, topic_type: true, notify_type: true, auto_action_type: true) end ``` --- .../admin/config/flags_controller.rb | 4 +- app/jobs/regular/export_user_archive.rb | 18 ++- app/models/flag.rb | 13 +- app/models/post.rb | 8 +- app/models/post_action.rb | 17 ++- app/models/post_action_type.rb | 144 +++++------------- app/models/reviewable_flagged_post.rb | 16 +- app/models/translation_override.rb | 6 +- app/models/user.rb | 12 +- app/serializers/flag_serializer.rb | 42 ++++- .../post_action_type_serializer.rb | 15 +- app/serializers/post_serializer.rb | 27 +--- app/serializers/site_serializer.rb | 43 +++++- config/initializers/004-message_bus.rb | 4 - db/fixtures/003_flags.rb | 10 ++ lib/guardian/post_guardian.rb | 20 ++- lib/post_action_creator.rb | 22 +-- lib/post_action_destroyer.rb | 12 +- lib/post_action_type_view.rb | 144 ++++++++++++++++++ lib/post_destroyer.rb | 8 +- lib/topic_view.rb | 31 ++-- spec/lib/post_action_type_view_spec.rb | 77 ++++++++++ spec/models/flag_spec.rb | 26 ++++ spec/models/post_action_type_spec.rb | 16 +- spec/multisite/flags_spec.rb | 24 +++ .../api/schemas/json/site_response.json | 6 + spec/serializers/flag_serializer_spec.rb | 57 +++++++ 27 files changed, 595 insertions(+), 227 deletions(-) create mode 100644 lib/post_action_type_view.rb create mode 100644 spec/lib/post_action_type_view_spec.rb create mode 100644 spec/multisite/flags_spec.rb create mode 100644 spec/serializers/flag_serializer_spec.rb diff --git a/app/controllers/admin/config/flags_controller.rb b/app/controllers/admin/config/flags_controller.rb index a95afdf5bce..4d0915953cf 100644 --- a/app/controllers/admin/config/flags_controller.rb +++ b/app/controllers/admin/config/flags_controller.rb @@ -29,7 +29,7 @@ class Admin::Config::FlagsController < Admin::AdminController with_service(Flags::CreateFlag) do on_success do Discourse.request_refresh! - render json: result.flag, serializer: FlagSerializer + render json: result.flag, serializer: FlagSerializer, used_flag_ids: Flag.used_flag_ids end on_failure { render(json: failed_json, status: 422) } on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess } @@ -43,7 +43,7 @@ class Admin::Config::FlagsController < Admin::AdminController with_service(Flags::UpdateFlag) do on_success do Discourse.request_refresh! - render json: result.flag, serializer: FlagSerializer + render json: result.flag, serializer: FlagSerializer, used_flag_ids: Flag.used_flag_ids end on_failure { render(json: failed_json, status: 422) } on_model_not_found(:message) { raise Discourse::NotFound } diff --git a/app/jobs/regular/export_user_archive.rb b/app/jobs/regular/export_user_archive.rb index edaf02fd515..56d0315fbaf 100644 --- a/app/jobs/regular/export_user_archive.rb +++ b/app/jobs/regular/export_user_archive.rb @@ -368,20 +368,24 @@ module Jobs end end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def flags_export return enum_for(:flags_export) unless block_given? PostAction .with_deleted .where(user_id: @current_user.id) - .where(post_action_type_id: PostActionType.flag_types.values) + .where(post_action_type_id: post_action_type_view.flag_types.values) .order(:created_at) .each do |pa| yield( [ pa.id, pa.post_id, - PostActionType.flag_types[pa.post_action_type_id], + post_action_type_view.flag_types[pa.post_action_type_id], pa.created_at, pa.updated_at, pa.deleted_at, @@ -400,7 +404,7 @@ module Jobs PostAction .with_deleted .where(user_id: @current_user.id) - .where(post_action_type_id: PostActionType.types[:like]) + .where(post_action_type_id: post_action_type_view.types[:like]) .order(:created_at) .each do |pa| post = Post.with_deleted.find_by(id: pa.post_id) @@ -424,7 +428,8 @@ module Jobs PostAction .where(user_id: @current_user.id) .where.not( - post_action_type_id: PostActionType.flag_types.values + [PostActionType.types[:like]], + post_action_type_id: + post_action_type_view.flag_types.values + [post_action_type_view.types[:like]], ) .exists? end @@ -435,7 +440,8 @@ module Jobs .with_deleted .where(user_id: @current_user.id) .where.not( - post_action_type_id: PostActionType.flag_types.values + [PostActionType.types[:like]], + post_action_type_id: + post_action_type_view.flag_types.values + [post_action_type_view.types[:like]], ) .order(:created_at) .each do |pa| @@ -443,7 +449,7 @@ module Jobs [ pa.id, pa.post_id, - PostActionType.types[pa.post_action_type] || pa.post_action_type, + post_action_type_view.types[pa.post_action_type] || pa.post_action_type, pa.created_at, pa.updated_at, pa.deleted_at, diff --git a/app/models/flag.rb b/app/models/flag.rb index b662dc13e76..e10fffb4aef 100644 --- a/app/models/flag.rb +++ b/app/models/flag.rb @@ -18,7 +18,9 @@ class Flag < ActiveRecord::Base attr_accessor :skip_reset_flag_callback - default_scope { order(:position).where(score_type: false) } + default_scope do + order(:position).where(score_type: false).where.not(id: PostActionType::LIKE_POST_ACTION_ID) + end def used? PostAction.exists?(post_action_type_id: self.id) || @@ -30,9 +32,14 @@ class Flag < ActiveRecord::Base end def self.reset_flag_settings! - # Flags are memoized for better performance. After the update, we need to reload them in all processes. + # Flags are cached in Redis for better performance. After the update, + # we need to reload them in all processes. PostActionType.reload_types - MessageBus.publish("/reload_post_action_types", {}) + end + + def self.used_flag_ids + PostAction.distinct(:post_action_type_id).pluck(:post_action_type_id) | + ReviewableScore.distinct(:reviewable_score_type).pluck(:reviewable_score_type) end def system? diff --git a/app/models/post.rb b/app/models/post.rb index 7b2740e0f1b..8f9f3def332 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -555,9 +555,13 @@ class Post < ActiveRecord::Base flags.count != 0 end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def flags post_actions.where( - post_action_type_id: PostActionType.flag_types_without_additional_message.values, + post_action_type_id: post_action_type_view.flag_types_without_additional_message.values, deleted_at: nil, ) end @@ -642,7 +646,7 @@ class Post < ActiveRecord::Base edit_delay: SiteSetting.cooldown_minutes_after_hiding_posts, flag_reason: I18n.t( - "flag_reasons.#{PostActionType.types[post_action_type_id]}", + "flag_reasons.#{post_action_type_view.types[post_action_type_id]}", locale: SiteSetting.default_locale, base_path: Discourse.base_path, ), diff --git a/app/models/post_action.rb b/app/models/post_action.rb index d0bd68595bf..1e3b2dd93d9 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -144,17 +144,21 @@ class PostAction < ActiveRecord::Base save end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def is_like? - post_action_type_id == PostActionType.types[:like] + post_action_type_id == post_action_type_view.types[:like] end def is_flag? - !!PostActionType.notify_flag_types[post_action_type_id] + !!post_action_type_view.notify_flag_types[post_action_type_id] end def is_private_message? - post_action_type_id == PostActionType.types[:notify_user] || - post_action_type_id == PostActionType.types[:notify_moderators] + post_action_type_id == post_action_type_view.types[:notify_user] || + post_action_type_id == post_action_type_view.types[:notify_moderators] end # A custom rate limiter for this model @@ -182,7 +186,8 @@ class PostAction < ActiveRecord::Base end def ensure_unique_actions - post_action_type_ids = is_flag? ? PostActionType.notify_flag_types.values : post_action_type_id + post_action_type_ids = + is_flag? ? post_action_type_view.notify_flag_types.values : post_action_type_id acted = PostAction @@ -198,7 +203,7 @@ class PostAction < ActiveRecord::Base end def post_action_type_key - PostActionType.types[post_action_type_id] + post_action_type_view.types[post_action_type_id] end def update_counters diff --git a/app/models/post_action_type.rb b/app/models/post_action_type.rb index 550e6586864..b85129ce8f4 100644 --- a/app/models/post_action_type.rb +++ b/app/models/post_action_type.rb @@ -13,8 +13,18 @@ class PostActionType < ActiveRecord::Base include AnonCacheInvalidator def expire_cache - ApplicationSerializer.expire_cache_fragment!(/\Apost_action_types_/) - ApplicationSerializer.expire_cache_fragment!(/\Apost_action_flag_types_/) + Discourse.cache.redis.del( + *I18n.available_locales.map do |locale| + Discourse.cache.normalize_key("post_action_types_#{locale}") + end, + ) + Discourse.cache.redis.del( + *I18n.available_locales.map do |locale| + Discourse.cache.normalize_key("post_action_flag_types_#{locale}") + end, + ) + Discourse.cache.delete(POST_ACTION_TYPE_ALL_FLAGS_KEY) + Discourse.cache.delete(POST_ACTION_TYPE_PUBLIC_TYPE_IDS_KEY) end class << self @@ -29,117 +39,35 @@ class PostActionType < ActiveRecord::Base @flag_settings = settings || FlagSettings.new end - def types - if overridden_by_plugin_or_skipped_db? - return Enum.new(like: 2).merge!(flag_settings.flag_types) - end - Enum.new(like: 2).merge(flag_types) - end - - def expire_cache - Discourse.redis.keys("post_action_types_*").each { |key| Discourse.redis.del(key) } - Discourse.redis.keys("post_action_flag_types_*").each { |key| Discourse.redis.del(key) } - end - def reload_types @flag_settings = FlagSettings.new - ReviewableScore.reload_types PostActionType.new.expire_cache + ReviewableScore.reload_types end - def overridden_by_plugin_or_skipped_db? - flag_settings.flag_types.present? || GlobalSetting.skip_db? - end - - def all_flags - Flag.unscoped.order(:position).all - end - - def auto_action_flag_types - return flag_settings.auto_action_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.select(&:auto_action_type)) - end - - def public_types - types.except(*flag_types.keys << :notify_user) - end - - def public_type_ids - @public_type_ids ||= public_types.values - end - - def flag_types_without_additional_message - return flag_settings.without_additional_message_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.reject(&:require_message)) - end - - def flag_types - return flag_settings.flag_types if overridden_by_plugin_or_skipped_db? - - # Once replace_flag API is fully deprecated, then we can drop respond_to. It is needed right now for migration to be evaluated. - # TODO (krisk) - flag_enum(all_flags.reject { |flag| flag.respond_to?(:score_type) && flag.score_type }) - end - - def score_types - return flag_settings.flag_types if overridden_by_plugin_or_skipped_db? - - # Once replace_flag API is fully deprecated, then we can drop respond_to. It is needed right now for migration to be evaluated. - # TODO (krisk) - flag_enum(all_flags.filter { |flag| flag.respond_to?(:score_type) && flag.score_type }) - end - - # flags resulting in mod notifications - def notify_flag_type_ids - notify_flag_types.values - end - - def notify_flag_types - return flag_settings.notify_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.select(&:notify_type)) - end - - def topic_flag_types - if overridden_by_plugin_or_skipped_db? - flag_settings.topic_flag_types - else - flag_enum(all_flags.select { |flag| flag.applies_to?("Topic") }) - end - end - - def disabled_flag_types - flag_enum(all_flags.reject(&:enabled)) - end - - def enabled_flag_types - flag_enum(all_flags.filter(&:enabled)) - end - - def additional_message_types - return flag_settings.additional_message_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.select(&:require_message)) - end - - def names - all_flags.pluck(:id, :name).to_h - end - - def descriptions - all_flags.pluck(:id, :description).to_h - end - - def applies_to - all_flags.pluck(:id, :applies_to).to_h - end - - def is_flag?(sym) - flag_types.valid?(sym) - end - - private - - def flag_enum(scope) - Enum.new(scope.map { |flag| [flag.name_key.to_sym, flag.id] }.to_h) + %i[ + expire_cache + all_flags + types + overridden_by_plugin_or_skipped_db? + auto_action_flag_types + public_types + public_type_ids + flag_types_without_additional_message + flags + flag_types + score_types + notify_flag_type_ids + notify_flag_types + topic_flag_types + disabled_flag_types + additional_message_types + names + descriptions + applies_to + is_flag? + ].each do |method_name| + define_method(method_name) { |*args| PostActionTypeView.new.send(method_name, *args) } end end diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index d35736bc05a..55ecae88441 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -137,12 +137,16 @@ class ReviewableFlaggedPost < Reviewable perform_ignore_and_do_nothing(performed_by, args) end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def perform_ignore_and_do_nothing(performed_by, args) actions = PostAction .active .where(post_id: target_id) - .where(post_action_type_id: PostActionType.notify_flag_type_ids) + .where(post_action_type_id: post_action_type_view.notify_flag_type_ids) actions.each do |action| action.deferred_at = Time.zone.now @@ -199,9 +203,9 @@ class ReviewableFlaggedPost < Reviewable # -1 is the automatic system clear action_type_ids = if performed_by.id == Discourse::SYSTEM_USER_ID - PostActionType.auto_action_flag_types.values + post_action_type_view.auto_action_flag_types.values else - PostActionType.notify_flag_type_ids + post_action_type_view.notify_flag_type_ids end actions = @@ -218,7 +222,7 @@ class ReviewableFlaggedPost < Reviewable # reset all cached counters cached = {} action_type_ids.each do |atid| - column = "#{PostActionType.types[atid]}_count" + column = "#{post_action_type_view.types[atid]}_count" cached[column] = 0 if ActiveRecord::Base.connection.column_exists?(:posts, column) end @@ -274,7 +278,7 @@ class ReviewableFlaggedPost < Reviewable PostAction .active .where(post_id: target_id) - .where(post_action_type_id: PostActionType.notify_flag_types.values) + .where(post_action_type_id: post_action_type_view.notify_flag_types.values) trigger_spam = false actions.each do |action| @@ -285,7 +289,7 @@ class ReviewableFlaggedPost < Reviewable action.save DB.after_commit do action.add_moderator_post_if_needed(performed_by, :agreed, args[:post_was_deleted]) - trigger_spam = true if action.post_action_type_id == PostActionType.types[:spam] + trigger_spam = true if action.post_action_type_id == post_action_type_view.types[:spam] end end end diff --git a/app/models/translation_override.rb b/app/models/translation_override.rb index 3ded31ce20d..a9d52c28546 100644 --- a/app/models/translation_override.rb +++ b/app/models/translation_override.rb @@ -107,10 +107,8 @@ class TranslationOverride < ActiveRecord::Base end def self.expire_cache(locale, key) - if key.starts_with?("post_action_types.") - ApplicationSerializer.expire_cache_fragment!("post_action_types_#{locale}") - elsif key.starts_with?("topic_flag_types.") - ApplicationSerializer.expire_cache_fragment!("post_action_flag_types_#{locale}") + if key.starts_with?("post_action_types.") || key.starts_with?("topic_flag_types.") + PostActionType.new.expire_cache else return false end diff --git a/app/models/user.rb b/app/models/user.rb index e8ffd33b44b..cf9dc0009ae 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1229,10 +1229,14 @@ class User < ActiveRecord::Base stat.increment!(:post_edits_count) end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def flags_given_count PostAction.where( user_id: id, - post_action_type_id: PostActionType.flag_types_without_additional_message.values, + post_action_type_id: post_action_type_view.flag_types_without_additional_message.values, ).count end @@ -1245,7 +1249,7 @@ class User < ActiveRecord::Base .includes(:post_actions) .where( "post_actions.post_action_type_id" => - PostActionType.flag_types_without_additional_message.values, + post_action_type_view.flag_types_without_additional_message.values, ) .count end @@ -1459,7 +1463,7 @@ class User < ActiveRecord::Base disagreed_flag_post_ids = PostAction - .where(post_action_type_id: PostActionType.types[:spam]) + .where(post_action_type_id: post_action_type_view.types[:spam]) .where.not(disagreed_at: nil) .pluck(:post_id) @@ -1587,7 +1591,7 @@ class User < ActiveRecord::Base PostAction .where(user_id: self.id) .where(disagreed_at: nil) - .where(post_action_type_id: PostActionType.notify_flag_type_ids) + .where(post_action_type_id: post_action_type_view.notify_flag_type_ids) .count end diff --git a/app/serializers/flag_serializer.rb b/app/serializers/flag_serializer.rb index eae1b940258..8901710217e 100644 --- a/app/serializers/flag_serializer.rb +++ b/app/serializers/flag_serializer.rb @@ -1,5 +1,45 @@ # frozen_string_literal: true class FlagSerializer < ApplicationSerializer - attributes :id, :name, :name_key, :description, :applies_to, :position, :require_message, :enabled + attributes :id, + :name, + :name_key, + :description, + :short_description, + :applies_to, + :position, + :require_message, + :enabled, + :is_flag, + :applies_to, + :is_used + + def i18n_prefix + "#{@options[:target] || "post_action"}_types.#{object.name_key}" + end + + def name + # system flags are using i18n translations when custom flags are using value entered by admin + I18n.t("#{i18n_prefix}.title", default: object.name) + end + + def description + I18n.t("#{i18n_prefix}.description", default: object.description) + end + + def short_description + I18n.t("#{i18n_prefix}.short_description", base_path: Discourse.base_path, default: "") + end + + def is_flag + !object.score_type && object.id != PostActionType::LIKE_POST_ACTION_ID + end + + def is_used + @options[:used_flag_ids].include?(object.id) + end + + def applies_to + Array.wrap(object.applies_to) + end end diff --git a/app/serializers/post_action_type_serializer.rb b/app/serializers/post_action_type_serializer.rb index 2ed5bce2d79..4c3afd6f613 100644 --- a/app/serializers/post_action_type_serializer.rb +++ b/app/serializers/post_action_type_serializer.rb @@ -16,12 +16,16 @@ class PostActionTypeSerializer < ApplicationSerializer include ConfigurableUrls + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def require_message - !!PostActionType.additional_message_types[object.id] + !!post_action_type_view.additional_message_types[object.id] end def is_flag - !!PostActionType.flag_types[object.id] + !!post_action_type_view.flag_types[object.id] end def name @@ -42,15 +46,16 @@ class PostActionTypeSerializer < ApplicationSerializer end def name_key - PostActionType.types[object.id].to_s + post_action_type_view.types[object.id].to_s end def enabled - !!PostActionType.enabled_flag_types[object.id] + # flags added by API are always enabled + true end def applies_to - Array.wrap(PostActionType.applies_to[object.id]) + Flag.valid_applies_to_types end def is_used diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 0929031a920..dd444c0b704 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -290,10 +290,12 @@ class PostSerializer < BasicPostSerializer result = [] can_see_post = scope.can_see_post?(object) - public_flag_types = - (@topic_view.present? ? @topic_view.public_flag_types : PostActionType.public_types) + @post_action_type_view = + @topic_view ? @topic_view.post_action_type_view : PostActionTypeView.new - (@topic_view.present? ? @topic_view.flag_types : PostActionType.types).each do |sym, id| + public_flag_types = @post_action_type_view.public_types + + @post_action_type_view.types.each do |sym, id| count_col = "#{sym}_count".to_sym count = object.public_send(count_col) if object.respond_to?(count_col) @@ -304,22 +306,9 @@ class PostSerializer < BasicPostSerializer sym, opts: { taken_actions: actions, - notify_flag_types: - ( - if @topic_view.present? - @topic_view.notify_flag_types - else - PostActionType.notify_flag_types - end - ), - additional_message_types: - ( - if @topic_view.present? - @topic_view.additional_message_types - else - PostActionType.additional_message_types - end - ), + notify_flag_types: @post_action_type_view.notify_flag_types, + additional_message_types: @post_action_type_view.additional_message_types, + post_action_type_view: @post_action_type_view, }, can_see_post: can_see_post, ) diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index c25a51e794d..5c4515b29f9 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -111,17 +111,44 @@ class SiteSerializer < ApplicationSerializer end def post_action_types - cache_fragment("post_action_types_#{I18n.locale}") do - types = ordered_flags(PostActionType.types.values) - ActiveModel::ArraySerializer.new(types).as_json - end + Discourse + .cache + .fetch("post_action_types_#{I18n.locale}") do + if PostActionType.overridden_by_plugin_or_skipped_db? + types = ordered_flags(PostActionType.types.values) + ActiveModel::ArraySerializer.new(types).as_json + else + ActiveModel::ArraySerializer.new( + Flag.unscoped.order(:position).where(score_type: false).all, + each_serializer: FlagSerializer, + target: :post_action, + used_flag_ids: Flag.used_flag_ids, + ).as_json + end + end end def topic_flag_types - cache_fragment("post_action_flag_types_#{I18n.locale}") do - types = ordered_flags(PostActionType.topic_flag_types.values) - ActiveModel::ArraySerializer.new(types, each_serializer: TopicFlagTypeSerializer).as_json - end + Discourse + .cache + .fetch("post_action_flag_types_#{I18n.locale}") do + if PostActionType.overridden_by_plugin_or_skipped_db? + types = ordered_flags(PostActionType.topic_flag_types.values) + ActiveModel::ArraySerializer.new(types, each_serializer: TopicFlagTypeSerializer).as_json + else + ActiveModel::ArraySerializer.new( + Flag + .unscoped + .where("'Topic' = ANY(applies_to)") + .where(score_type: false) + .order(:position) + .all, + each_serializer: FlagSerializer, + target: :topic_flag, + used_flag_ids: Flag.used_flag_ids, + ).as_json + end + end end def default_archetype diff --git a/config/initializers/004-message_bus.rb b/config/initializers/004-message_bus.rb index 28ef655dfd5..d9a1959a3be 100644 --- a/config/initializers/004-message_bus.rb +++ b/config/initializers/004-message_bus.rb @@ -132,7 +132,3 @@ if Rails.env == "test" || $0 =~ /rake$/ # disable keepalive in testing MessageBus.keepalive_interval = -1 end - -if !Rails.env.test? - MessageBus.subscribe("/reload_post_action_types") { PostActionType.reload_types } -end diff --git a/db/fixtures/003_flags.rb b/db/fixtures/003_flags.rb index 5518cfcad80..b905f1fc7a1 100644 --- a/db/fixtures/003_flags.rb +++ b/db/fixtures/003_flags.rb @@ -64,3 +64,13 @@ Flag.unscoped.seed do |s| s.applies_to = %w[] s.skip_reset_flag_callback = true end +Flag.unscoped.seed do |s| + s.id = 2 + s.name = "like" + s.notify_type = false + s.auto_action_type = false + s.require_message = false + s.score_type = false + s.applies_to = %w[Post] + s.skip_reset_flag_callback = true +end diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 73365e66302..c46a7452c16 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -37,15 +37,18 @@ module PostGuardian end taken = opts[:taken_actions].try(:keys).to_a + post_action_type_view = opts[:post_action_type_view] || PostActionTypeView.new is_flag = if (opts[:notify_flag_types] && opts[:additional_message_types]) opts[:notify_flag_types][action_key] || opts[:additional_message_types][action_key] else - PostActionType.notify_flag_types[action_key] || - PostActionType.additional_message_types[action_key] + post_action_type_view.notify_flag_types[action_key] || + post_action_type_view.additional_message_types[action_key] end - already_taken_this_action = taken.any? && taken.include?(PostActionType.types[action_key]) - already_did_flagging = taken.any? && (taken & PostActionType.notify_flag_types.values).any? + already_taken_this_action = + taken.any? && taken.include?(post_action_type_view.types[action_key]) + already_did_flagging = + taken.any? && (taken & post_action_type_view.notify_flag_types.values).any? result = if authenticated? && post @@ -61,7 +64,9 @@ module PostGuardian # post made by staff, but we don't allow staff flags return false if is_flag && (!SiteSetting.allow_flagging_staff?) && post&.user&.staff? - return false if is_flag && PostActionType.disabled_flag_types.keys.include?(action_key) + if is_flag && post_action_type_view.disabled_flag_types.keys.include?(action_key) + return false + end if action_key == :notify_user && !@user.in_any_groups?(SiteSetting.personal_message_enabled_groups_map) @@ -111,12 +116,13 @@ module PostGuardian return true if is_admin? return false unless topic - type_symbol = PostActionType.types[post_action_type_id] + post_action_type_view = PostActionTypeView.new + type_symbol = post_action_type_view.types[post_action_type_id] return false if type_symbol == :bookmark return false if type_symbol == :notify_user && !is_moderator? - return can_see_flags?(topic) if PostActionType.is_flag?(type_symbol) + return can_see_flags?(topic) if post_action_type_view.is_flag?(type_symbol) true end diff --git a/lib/post_action_creator.rb b/lib/post_action_creator.rb index 23a5ee75047..5e5e2b86ecf 100644 --- a/lib/post_action_creator.rb +++ b/lib/post_action_creator.rb @@ -57,7 +57,8 @@ class PostActionCreator @post = post @post_action_type_id = post_action_type_id - @post_action_name = PostActionType.types[@post_action_type_id] + @post_action_type_view = PostActionTypeView.new + @post_action_name = @post_action_type_view.types[@post_action_type_id] @is_warning = is_warning @take_action = take_action && guardian.is_staff? @@ -96,7 +97,7 @@ class PostActionCreator if !post_can_act? || (@queue_for_review && !guardian.is_staff?) result.forbidden = true - if taken_actions&.keys&.include?(PostActionType.types[@post_action_name]) + if taken_actions&.keys&.include?(@post_action_type_view.types[@post_action_name]) result.add_error(I18n.t("action_already_performed")) else result.add_error(I18n.t("invalid_access")) @@ -115,7 +116,7 @@ class PostActionCreator # create meta topic / post if needed if @message.present? && - (PostActionType.additional_message_types.keys | %i[spam illegal]).include?( + (@post_action_type_view.additional_message_types.keys | %i[spam illegal]).include?( @post_action_name, ) creator = create_message_creator @@ -170,11 +171,11 @@ class PostActionCreator private def flagging_post? - PostActionType.notify_flag_type_ids.include?(@post_action_type_id) + @post_action_type_view.notify_flag_type_ids.include?(@post_action_type_id) end def cannot_flag_again?(reviewable) - return false if @post_action_type_id == PostActionType.types[:notify_moderators] + return false if @post_action_type_id == @post_action_type_view.types[:notify_moderators] flag_type_already_used = reviewable.reviewable_scores.any? do |rs| rs.reviewable_score_type == @post_action_type_id && !rs.pending? @@ -233,7 +234,8 @@ class PostActionCreator return if @post.hidden? return if !@created_by.staff? && @post.user&.staff? - not_auto_action_flag_type = !PostActionType.auto_action_flag_types.include?(@post_action_name) + not_auto_action_flag_type = + !@post_action_type_view.auto_action_flag_types.include?(@post_action_name) return if not_auto_action_flag_type && !@queue_for_review if @queue_for_review @@ -304,14 +306,14 @@ class PostActionCreator if post_action case @post_action_type_id - when *PostActionType.notify_flag_type_ids + when *@post_action_type_view.notify_flag_type_ids DiscourseEvent.trigger(:flag_created, post_action, self) - when PostActionType.types[:like] + when @post_action_type_view.types[:like] DiscourseEvent.trigger(:like_created, post_action, self) end end - if @post_action_type_id == PostActionType.types[:like] + if @post_action_type_id == @post_action_type_view.types[:like] GivenDailyLike.increment_for(@created_by.id) end @@ -381,7 +383,7 @@ class PostActionCreator target: @post, topic: @post.topic, reviewable_by_moderator: true, - potential_spam: @post_action_type_id == PostActionType.types[:spam], + potential_spam: @post_action_type_id == @post_action_type_view.types[:spam], payload: { targets_topic: @targets_topic, }, diff --git a/lib/post_action_destroyer.rb b/lib/post_action_destroyer.rb index 55c39469ee8..0de7906cf46 100644 --- a/lib/post_action_destroyer.rb +++ b/lib/post_action_destroyer.rb @@ -17,6 +17,10 @@ class PostActionDestroyer new(destroyed_by, post, PostActionType.types[action_key], opts).perform end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def perform result = DestroyResult.new @@ -50,14 +54,14 @@ class PostActionDestroyer post_action.remove_act!(@destroyed_by) post_action.post.unhide! if post_action.staff_took_action - if @post_action_type_id == PostActionType.types[:like] + if @post_action_type_id == post_action_type_view.types[:like] GivenDailyLike.decrement_for(@destroyed_by.id) end case @post_action_type_id - when *PostActionType.notify_flag_type_ids + when *post_action_type_view.notify_flag_type_ids DiscourseEvent.trigger(:flag_destroyed, post_action, self) - when PostActionType.types[:like] + when post_action_type_view.types[:like] DiscourseEvent.trigger(:like_destroyed, post_action, self) end @@ -78,7 +82,7 @@ class PostActionDestroyer end def notify_subscribers - name = PostActionType.types[@post_action_type_id] + name = post_action_type_view.types[@post_action_type_id] if name == :like @post.publish_change_to_clients!( :unliked, diff --git a/lib/post_action_type_view.rb b/lib/post_action_type_view.rb new file mode 100644 index 00000000000..2317b89e6b1 --- /dev/null +++ b/lib/post_action_type_view.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +class PostActionTypeView + ATTRIBUTE_NAMES = %i[ + id + name + name_key + description + notify_type + auto_action_type + require_message + applies_to + position + enabled + score_type + ] + + def all_flags + @all_flags ||= + Discourse + .cache + .fetch(PostActionType::POST_ACTION_TYPE_ALL_FLAGS_KEY) do + Flag + .unscoped + .order(:position) + .pluck(ATTRIBUTE_NAMES) + .map { |attributes| ATTRIBUTE_NAMES.zip(attributes).to_h } + end + end + + def flag_settings + @flag_settings ||= PostActionType.flag_settings + end + + def types + if overridden_by_plugin_or_skipped_db? + return Enum.new(like: PostActionType::LIKE_POST_ACTION_ID).merge!(flag_settings.flag_types) + end + Enum.new(like: PostActionType::LIKE_POST_ACTION_ID).merge(flag_types) + end + + def overridden_by_plugin_or_skipped_db? + flag_settings.flag_types.present? || GlobalSetting.skip_db? + end + + def auto_action_flag_types + return flag_settings.auto_action_types if overridden_by_plugin_or_skipped_db? + flag_enum(all_flags.select { |flag| flag[:auto_action_type] }) + end + + def public_types + types.except(*flag_types.keys << :notify_user) + end + + def public_type_ids + Discourse + .cache + .fetch(PostActionType::POST_ACTION_TYPE_PUBLIC_TYPE_IDS_KEY) { public_types.values } + end + + def flag_types_without_additional_message + return flag_settings.without_additional_message_types if overridden_by_plugin_or_skipped_db? + flag_enum(flags.reject { |flag| flag[:require_message] }) + end + + def flags + all_flags.reject do |flag| + flag[:score_type] || flag[:id] == PostActionType::LIKE_POST_ACTION_ID + end + end + + def flag_types + return flag_settings.flag_types if overridden_by_plugin_or_skipped_db? + flag_enum(flags) + end + + def score_types + return flag_settings.flag_types if overridden_by_plugin_or_skipped_db? + flag_enum(all_flags.filter { |flag| flag[:score_type] }) + end + + # flags resulting in mod notifications + def notify_flag_type_ids + notify_flag_types.values + end + + def notify_flag_types + return flag_settings.notify_types if overridden_by_plugin_or_skipped_db? + flag_enum(all_flags.select { |flag| flag[:notify_type] }) + end + + def topic_flag_types + if overridden_by_plugin_or_skipped_db? + flag_settings.topic_flag_types + else + flag_enum(all_flags.select { |flag| flag[:applies_to].include?("Topic") }) + end + end + + def disabled_flag_types + flag_enum(all_flags.reject { |flag| flag[:enabled] }) + end + + def additional_message_types + return flag_settings.additional_message_types if overridden_by_plugin_or_skipped_db? + flag_enum(all_flags.select { |flag| flag[:require_message] }) + end + + def names + all_flags.reduce({}) do |acc, f| + acc[f[:id]] = f[:name] + acc + end + end + + def descriptions + all_flags.reduce({}) do |acc, f| + acc[f[:id]] = f[:description] + acc + end + end + + def applies_to + all_flags.reduce({}) do |acc, f| + acc[f[:id]] = f[:applies_to] + acc + end + end + + def is_flag?(sym) + flag_types.valid?(sym) + end + + private + + def flag_enum(scope) + Enum.new( + scope.reduce({}) do |acc, f| + acc[f[:name_key].to_sym] = f[:id] + acc + end, + ) + end +end diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index e3f206be6e5..e302f3228d1 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -348,6 +348,10 @@ class PostDestroyer Jobs.enqueue(:feature_topic_users, topic_id: @post.topic_id) end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def trash_public_post_actions if public_post_actions = PostAction.publics.where(post_id: @post.id) public_post_actions.each { |pa| permanent? ? pa.destroy! : pa.trash!(@user) } @@ -357,7 +361,7 @@ class PostDestroyer @post.custom_fields["deleted_public_actions"] = public_post_actions.ids @post.save_custom_fields - f = PostActionType.public_types.map { |k, _| ["#{k}_count", 0] } + f = post_action_type_view.public_types.map { |k, _| ["#{k}_count", 0] } Post.with_deleted.where(id: @post.id).update_all(Hash[*f.flatten]) end end @@ -387,7 +391,7 @@ class PostDestroyer # ReviewableScore#types is a superset of PostActionType#flag_types. # If the reviewable score type is not on the latter, it means it's not a flag by a user and # must be an automated flag like `needs_approval`. There's no flag reason for these kind of types. - flag_type = PostActionType.flag_types[rs.reviewable_score_type] + flag_type = post_action_type_view.flag_types[rs.reviewable_score_type] return unless flag_type notify_responders = options[:notify_responders] diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 4123bb48dcf..9b7f0fdd53e 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -598,17 +598,28 @@ class TopicView ReviewableQueuedPost.pending.where(target_created_by: @user, topic: @topic).order(:created_at) end + def post_action_type_view + @post_action_type_view ||= PostActionTypeView.new + end + def actions_summary return @actions_summary unless @actions_summary.nil? @actions_summary = [] return @actions_summary unless post = posts&.first - PostActionType.topic_flag_types.each do |sym, id| + post_action_type_view.topic_flag_types.each do |sym, id| @actions_summary << { id: id, count: 0, hidden: false, - can_act: @guardian.post_can_act?(post, sym), + can_act: + @guardian.post_can_act?( + post, + sym, + opts: { + post_action_type_view: post_action_type_view, + }, + ), } end @@ -623,22 +634,6 @@ class TopicView @pm_params ||= TopicQuery.new(@user).get_pm_params(topic) end - def flag_types - @flag_types ||= PostActionType.types - end - - def public_flag_types - @public_flag_types ||= PostActionType.public_types - end - - def notify_flag_types - @notify_flag_types ||= PostActionType.notify_flag_types - end - - def additional_message_types - @additional_message_types ||= PostActionType.additional_message_types - end - def suggested_topics if @include_suggested @suggested_topics ||= diff --git a/spec/lib/post_action_type_view_spec.rb b/spec/lib/post_action_type_view_spec.rb new file mode 100644 index 00000000000..607839522b7 --- /dev/null +++ b/spec/lib/post_action_type_view_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +RSpec.describe PostActionTypeView do + let(:post_action_type_view) { PostActionTypeView.new } + + it "returns correct types" do + expect(post_action_type_view.flag_types).to eq( + { + illegal: 10, + inappropriate: 4, + notify_moderators: 7, + notify_user: 6, + off_topic: 3, + spam: 8, + }, + ) + expect(post_action_type_view.public_types).to eq({ like: 2 }) + + expect(post_action_type_view.notify_flag_types).to eq( + { illegal: 10, inappropriate: 4, notify_moderators: 7, off_topic: 3, spam: 8 }, + ) + + expect(post_action_type_view.topic_flag_types).to eq( + { illegal: 10, inappropriate: 4, notify_moderators: 7, spam: 8 }, + ) + + expect(post_action_type_view.additional_message_types).to eq( + { illegal: 10, notify_moderators: 7, notify_user: 6 }, + ) + expect(post_action_type_view.score_types).to eq({ needs_approval: 9 }) + + flag = Fabricate(:flag, name: "custom", enabled: false) + expect(PostActionTypeView.new.disabled_flag_types).to eq({ custom: flag.id }) + flag.destroy! + end + + it "defines names of flags" do + expect(post_action_type_view.names).to eq( + { + 6 => "notify_user", + 3 => "off_topic", + 4 => "inappropriate", + 8 => "spam", + 10 => "illegal", + 7 => "notify_moderators", + 9 => "needs_approval", + 2 => "like", + }, + ) + end + + it "defines descriptions of flags" do + flag = Fabricate(:flag, enabled: false, description: "custom flag description") + expect(post_action_type_view.descriptions[flag.id]).to eq("custom flag description") + flag.destroy! + end + + it "defines where flags can be applies to" do + expect(post_action_type_view.applies_to).to eq( + { + 6 => %w[Post Chat::Message], + 3 => %w[Post Chat::Message], + 4 => %w[Post Topic Chat::Message], + 8 => %w[Post Topic Chat::Message], + 10 => %w[Post Topic Chat::Message], + 7 => %w[Post Topic Chat::Message], + 9 => [], + 2 => ["Post"], + }, + ) + end + + it "defines is post action type is a flag" do + expect(post_action_type_view.is_flag?(:like)).to be false + expect(post_action_type_view.is_flag?(:off_topic)).to be true + end +end diff --git a/spec/models/flag_spec.rb b/spec/models/flag_spec.rb index 884a3b13688..c090b854cd0 100644 --- a/spec/models/flag_spec.rb +++ b/spec/models/flag_spec.rb @@ -70,4 +70,30 @@ RSpec.describe Flag, type: :model do %i[notify_user off_topic inappropriate spam illegal notify_moderators needs_approval], ) end + + describe ".used_flag_ids" do + fab!(:post_action) { Fabricate(:post_action, post_action_type_id: PostActionType.types[:like]) } + fab!(:post_action_2) do + Fabricate(:post_action, post_action_type_id: PostActionType.types[:like]) + end + fab!(:post_action_3) do + Fabricate(:post_action, post_action_type_id: PostActionType.types[:off_topic]) + end + fab!(:reviewable_score) do + Fabricate(:reviewable_score, reviewable_score_type: PostActionType.types[:off_topic]) + end + fab!(:reviewable_score_2) do + Fabricate(:reviewable_score, reviewable_score_type: PostActionType.types[:illegal]) + end + + it "returns an array of unique flag ids" do + expect(Flag.used_flag_ids).to eq( + [ + PostActionType.types[:like], + PostActionType.types[:off_topic], + PostActionType.types[:illegal], + ], + ) + end + end end diff --git a/spec/models/post_action_type_spec.rb b/spec/models/post_action_type_spec.rb index 77bf6d7947d..7566a1b8f26 100644 --- a/spec/models/post_action_type_spec.rb +++ b/spec/models/post_action_type_spec.rb @@ -4,17 +4,15 @@ RSpec.describe PostActionType do describe "Callbacks" do describe "#expiry_cache" do it "should clear the cache on save" do - cache = ApplicationSerializer.fragment_cache - - cache["post_action_types_#{I18n.locale}"] = "test" - cache["post_action_flag_types_#{I18n.locale}"] = "test2" + Discourse.cache.write("post_action_types_#{I18n.locale}", "test") + Discourse.cache.write("post_action_flag_types_#{I18n.locale}", "test2") PostActionType.new(name_key: "some_key").save! - expect(cache["post_action_types_#{I18n.locale}"]).to eq(nil) - expect(cache["post_action_flag_types_#{I18n.locale}"]).to eq(nil) + expect(Discourse.cache.read("post_action_types_#{I18n.locale}")).to eq(nil) + expect(Discourse.cache.read("post_action_flag_types_#{I18n.locale}")).to eq(nil) ensure - ApplicationSerializer.fragment_cache.clear + PostActionType.new.expire_cache end end end @@ -30,7 +28,9 @@ RSpec.describe PostActionType do end describe ".additional_message_types" do - before { described_class.stubs(:overridden_by_plugin_or_skipped_db?).returns(overriden) } + before do + PostActionTypeView.any_instance.stubs(:overridden_by_plugin_or_skipped_db?).returns(overriden) + end context "when overridden by plugin or skipped DB" do let(:overriden) { true } diff --git a/spec/multisite/flags_spec.rb b/spec/multisite/flags_spec.rb new file mode 100644 index 00000000000..4f800e2e385 --- /dev/null +++ b/spec/multisite/flags_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +RSpec.describe "Custom flags in multisite", type: :multisite do + describe "PostACtionType#all_flags" do + use_redis_snapshotting + + it "does not share flag definitions between sites" do + flag_1 = Flag.create!(name: "test flag 1", position: 99, applies_to: ["Post"]) + + test_multisite_connection("second") do + flag_2 = Flag.create!(name: "test flag 2", position: 99, applies_to: ["Post"]) + PostActionType.new.expire_cache + expect(PostActionType.all_flags.last).to eq( + flag_2.attributes.except("created_at", "updated_at").transform_keys(&:to_sym), + ) + end + + PostActionType.new.expire_cache + expect(PostActionType.all_flags.last).to eq( + flag_1.attributes.except("created_at", "updated_at").transform_keys(&:to_sym), + ) + end + end +end diff --git a/spec/requests/api/schemas/json/site_response.json b/spec/requests/api/schemas/json/site_response.json index a432991de95..13d7c145cd3 100644 --- a/spec/requests/api/schemas/json/site_response.json +++ b/spec/requests/api/schemas/json/site_response.json @@ -362,6 +362,9 @@ }, "is_used": { "type": "boolean" + }, + "position": { + "type": "integer" } }, "required": [ @@ -414,6 +417,9 @@ }, "is_used": { "type": "boolean" + }, + "position": { + "type": "integer" } }, "required": [ diff --git a/spec/serializers/flag_serializer_spec.rb b/spec/serializers/flag_serializer_spec.rb new file mode 100644 index 00000000000..ad949180a87 --- /dev/null +++ b/spec/serializers/flag_serializer_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +RSpec.describe FlagSerializer do + let(:flag) { Flag.find_by(name: "illegal") } + + context "when system flag" do + it "returns translated name" do + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:name]).to eq(I18n.t("post_action_types.illegal.title")) + end + + it "returns translated description" do + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:description]).to eq(I18n.t("post_action_types.illegal.description")) + end + end + + context "when custom flag" do + fab!(:flag) { Fabricate(:flag, name: "custom title", description: "custom description") } + + it "returns translated name" do + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:name]).to eq("custom title") + end + + it "returns translated description" do + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:description]).to eq("custom description") + end + end + + it "returns is_flag true for flags" do + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:is_flag]).to be true + end + + it "returns is_flag false for like" do + flag = Flag.unscoped.find_by(name: "like") + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:is_flag]).to be false + end + + it "returns is_used false when not used" do + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:is_used]).to be false + end + + it "returns is_used true when used" do + serialized = described_class.new(flag, used_flag_ids: [flag.id]).as_json + expect(serialized[:flag][:is_used]).to be true + end + + it "returns applies_to" do + serialized = described_class.new(flag, used_flag_ids: []).as_json + expect(serialized[:flag][:applies_to]).to eq(%w[Post Topic Chat::Message]) + end +end