From c975c7fe1bc23383cdd9ae1bb90438ee7350a731 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 18 Jul 2024 10:10:22 +1000 Subject: [PATCH] FEATURE: custom flag can require additional message (#27908) Allow admin to create custom flag which requires an additional message. I decided to rename the old `custom_flag` into `require_message` as it is more descriptive. --- .../addon/components/admin-flags-form.gjs | 50 ++++++++++++++++--- .../app/components/flag-action-type.js | 2 +- .../discourse/app/components/modal/flag.js | 8 +-- .../discourse/app/models/post-action-type.js | 3 +- .../discourse/tests/fixtures/site-fixtures.js | 22 ++++---- .../discourse/tests/helpers/site.js | 16 +++--- .../stylesheets/common/admin/flags.scss | 9 ++++ .../concerns/reports/moderators_activity.rb | 2 +- app/models/flag.rb | 5 +- app/models/post.rb | 2 +- app/models/post_action_type.rb | 12 ++--- app/models/user.rb | 7 ++- app/serializers/flag_serializer.rb | 2 +- .../post_action_type_serializer.rb | 6 +-- app/services/flags/create_flag.rb | 5 +- app/services/flags/update_flag.rb | 12 ++++- config/locales/client.en.yml | 2 + db/fixtures/003_flags.rb | 14 +++--- ...te_flags_custom_type_to_require_message.rb | 19 +++++++ ...40714231516_drop_custom_type_from_flags.rb | 13 +++++ lib/badge_queries.rb | 2 +- lib/flag_settings.rb | 16 +++--- lib/guardian/post_guardian.rb | 3 +- lib/post_action_creator.rb | 38 +++++++------- lib/post_revisor.rb | 2 +- spec/lib/flag_settings_spec.rb | 10 ++-- spec/models/post_action_spec.rb | 40 ++++++++++++++- .../api/schemas/json/site_response.json | 8 +-- spec/services/flags/create_flag_spec.rb | 5 +- spec/services/flags/update_flag_spec.rb | 5 +- 30 files changed, 240 insertions(+), 100 deletions(-) create mode 100644 db/migrate/20240714231226_duplicate_flags_custom_type_to_require_message.rb create mode 100644 db/post_migrate/20240714231516_drop_custom_type_from_flags.rb diff --git a/app/assets/javascripts/admin/addon/components/admin-flags-form.gjs b/app/assets/javascripts/admin/addon/components/admin-flags-form.gjs index 07ffb00a611..ec15722b6bc 100644 --- a/app/assets/javascripts/admin/addon/components/admin-flags-form.gjs +++ b/app/assets/javascripts/admin/addon/components/admin-flags-form.gjs @@ -1,14 +1,15 @@ import Component from "@glimmer/component"; import { tracked } from "@glimmer/tracking"; -import { Input } from "@ember/component"; -import { hash } from "@ember/helper"; +import { fn, hash } from "@ember/helper"; import { TextArea } from "@ember/legacy-built-in-components"; +import { on } from "@ember/modifier"; import { action } from "@ember/object"; import { LinkTo } from "@ember/routing"; import { service } from "@ember/service"; import { isEmpty } from "@ember/utils"; import { not } from "truth-helpers"; import DButton from "discourse/components/d-button"; +import withEventValue from "discourse/helpers/with-event-value"; import { ajax } from "discourse/lib/ajax"; import { popupAjaxError } from "discourse/lib/ajax-error"; import dIcon from "discourse-common/helpers/d-icon"; @@ -23,6 +24,7 @@ export default class AdminFlagsForm extends Component { @service site; @tracked enabled = true; + @tracked requireMessage = false; @tracked name; @tracked description; @tracked appliesTo; @@ -33,6 +35,7 @@ export default class AdminFlagsForm extends Component { this.name = this.args.flag.name; this.description = this.args.flag.description; this.appliesTo = this.args.flag.applies_to; + this.requireMessage = this.args.flag.require_message; this.enabled = this.args.flag.enabled; } } @@ -73,6 +76,16 @@ export default class AdminFlagsForm extends Component { this.isUpdate ? this.update() : this.create(); } + @action + onToggleRequireMessage(e) { + this.requireMessage = e.target.checked; + } + + @action + onToggleEnabled(e) { + this.enabled = e.target.checked; + } + @bind create() { return ajax(`/admin/config/flags`, { @@ -98,6 +111,7 @@ export default class AdminFlagsForm extends Component { this.args.flag.name = response.flag.name; this.args.flag.description = response.flag.description; this.args.flag.applies_to = response.flag.applies_to; + this.args.flag.require_message = response.flag.require_message; this.args.flag.enabled = response.flag.enabled; this.router.transitionTo("adminConfig.flags"); }) @@ -112,6 +126,7 @@ export default class AdminFlagsForm extends Component { name: this.name, description: this.description, applies_to: this.appliesTo, + require_message: this.requireMessage, enabled: this.enabled, }; } @@ -132,12 +147,13 @@ export default class AdminFlagsForm extends Component { - @@ -164,9 +180,31 @@ export default class AdminFlagsForm extends Component { /> +
+ +
+
diff --git a/app/assets/javascripts/discourse/app/components/flag-action-type.js b/app/assets/javascripts/discourse/app/components/flag-action-type.js index 963742bf18e..ebcd2627dfe 100644 --- a/app/assets/javascripts/discourse/app/components/flag-action-type.js +++ b/app/assets/javascripts/discourse/app/components/flag-action-type.js @@ -35,7 +35,7 @@ export default Component.extend({ return flag === selectedFlag; }, - showMessageInput: and("flag.is_custom_flag", "selected"), + showMessageInput: and("flag.require_message", "selected"), showConfirmation: and("flag.isIllegal", "selected"), showDescription: not("showMessageInput"), isNotifyUser: equal("flag.name_key", "notify_user"), diff --git a/app/assets/javascripts/discourse/app/components/modal/flag.js b/app/assets/javascripts/discourse/app/components/modal/flag.js index 95b26ec3535..7e4a93d1187 100644 --- a/app/assets/javascripts/discourse/app/components/modal/flag.js +++ b/app/assets/javascripts/discourse/app/components/modal/flag.js @@ -73,7 +73,7 @@ export default class Flag extends Component { } get submitLabel() { - if (this.selected?.is_custom_flag) { + if (this.selected?.require_message) { return this.args.model.flagTarget.customSubmitLabel(); } @@ -97,7 +97,7 @@ export default class Flag extends Component { return false; } - if (!this.selected.is_custom_flag) { + if (!this.selected.require_message) { return true; } @@ -119,7 +119,7 @@ export default class Flag extends Component { get canTakeAction() { return ( !this.args.model.flagTarget.targetsTopic() && - !this.selected?.is_custom_flag && + !this.selected?.require_message && this.currentUser.staff ); } @@ -184,7 +184,7 @@ export default class Flag extends Component { @action createFlag(opts = {}) { - if (this.selected.is_custom_flag) { + if (this.selected.require_message) { opts.message = this.message; } this.args.model.flagTarget.create(this, opts); diff --git a/app/assets/javascripts/discourse/app/models/post-action-type.js b/app/assets/javascripts/discourse/app/models/post-action-type.js index 6864d891a08..4ce015d1696 100644 --- a/app/assets/javascripts/discourse/app/models/post-action-type.js +++ b/app/assets/javascripts/discourse/app/models/post-action-type.js @@ -1,9 +1,8 @@ -import { equal, not } from "@ember/object/computed"; +import { equal } from "@ember/object/computed"; import RestModel from "discourse/models/rest"; export const MAX_MESSAGE_LENGTH = 500; export default class PostActionType extends RestModel { - @not("is_custom_flag") notCustomFlag; @equal("name_key", "illegal") isIllegal; } diff --git a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js index b61ed7b6c22..d392b7fa166 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js @@ -568,7 +568,7 @@ export default { is_flag: false, icon: null, id: 1, - is_custom_flag: false, + require_message: false, }, { name_key: "like", @@ -578,7 +578,7 @@ export default { is_flag: false, icon: "heart", id: 2, - is_custom_flag: false, + require_message: false, }, { name_key: "off_topic", @@ -589,7 +589,7 @@ export default { is_flag: true, icon: null, id: 3, - is_custom_flag: false, + require_message: false, enabled: true, applies_to: ["Post", "Chat::Message"] }, @@ -603,7 +603,7 @@ export default { is_flag: true, icon: null, id: 4, - is_custom_flag: false, + require_message: false, enabled: true, applies_to: ["Post", "Topic", "Chat::Message"] }, @@ -615,7 +615,7 @@ export default { is_flag: false, icon: null, id: 5, - is_custom_flag: false, + require_message: false, enabled: true }, { @@ -627,7 +627,7 @@ export default { is_flag: true, icon: null, id: 8, - is_custom_flag: false, + require_message: false, enabled: true, applies_to: ["Post", "Topic", "Chat::Message"] }, @@ -641,7 +641,7 @@ export default { is_flag: true, icon: null, id: 6, - is_custom_flag: true, + require_message: true, enabled: true, applies_to: ["Post", "Topic", "Chat::Message"] }, @@ -654,7 +654,7 @@ export default { is_flag: true, icon: null, id: 7, - is_custom_flag: true, + require_message: true, enabled: true, applies_to: ["Post", "Topic", "Chat::Message"] }, @@ -668,7 +668,7 @@ export default { is_flag: true, icon: null, id: 4, - is_custom_flag: false, + require_message: false, enabled: true, applies_to: ["Post", "Topic", "Chat::Message"] }, @@ -680,7 +680,7 @@ export default { is_flag: true, icon: null, id: 8, - is_custom_flag: false, + require_message: false, enabled: true, applies_to: ["Post", "Topic", "Chat::Message"] }, @@ -692,7 +692,7 @@ export default { is_flag: true, icon: null, id: 7, - is_custom_flag: true, + require_message: true, enabled: true, applies_to: ["Post", "Topic", "Chat::Message"] }, diff --git a/app/assets/javascripts/discourse/tests/helpers/site.js b/app/assets/javascripts/discourse/tests/helpers/site.js index 1b537518c21..4a1e42b4ede 100644 --- a/app/assets/javascripts/discourse/tests/helpers/site.js +++ b/app/assets/javascripts/discourse/tests/helpers/site.js @@ -278,7 +278,7 @@ PreloadStore.store("site", { is_flag: false, icon: null, id: 1, - is_custom_flag: false, + require_message: false, }, { name_key: "like", @@ -288,7 +288,7 @@ PreloadStore.store("site", { is_flag: false, icon: "heart", id: 2, - is_custom_flag: false, + require_message: false, }, { name_key: "off_topic", @@ -299,7 +299,7 @@ PreloadStore.store("site", { is_flag: true, icon: null, id: 3, - is_custom_flag: false, + require_message: false, }, { name_key: "inappropriate", @@ -310,7 +310,7 @@ PreloadStore.store("site", { is_flag: true, icon: null, id: 4, - is_custom_flag: false, + require_message: false, }, { name_key: "vote", @@ -320,7 +320,7 @@ PreloadStore.store("site", { is_flag: false, icon: null, id: 5, - is_custom_flag: false, + require_message: false, }, { name_key: "spam", @@ -331,7 +331,7 @@ PreloadStore.store("site", { is_flag: true, icon: null, id: 8, - is_custom_flag: false, + require_message: false, }, { name_key: "notify_user", @@ -342,7 +342,7 @@ PreloadStore.store("site", { is_flag: true, icon: null, id: 6, - is_custom_flag: true, + require_message: true, }, { name_key: "notify_moderators", @@ -353,7 +353,7 @@ PreloadStore.store("site", { is_flag: true, icon: null, id: 7, - is_custom_flag: true, + require_message: true, }, ], archetypes: [{ id: "regular", name: "Regular Topic", options: [] }], diff --git a/app/assets/stylesheets/common/admin/flags.scss b/app/assets/stylesheets/common/admin/flags.scss index 9a35c11e5bc..5d159992368 100644 --- a/app/assets/stylesheets/common/admin/flags.scss +++ b/app/assets/stylesheets/common/admin/flags.scss @@ -56,6 +56,15 @@ &__applies-to.select-kit.multi-select { width: 60%; } + &__require-message-description { + clear: both; + flex-basis: 100%; + font-size: var(--font-down-2); + margin-top: 0.25em; + } + label { + flex-wrap: wrap; + } } .admin-flags__header { diff --git a/app/models/concerns/reports/moderators_activity.rb b/app/models/concerns/reports/moderators_activity.rb index 12383a58e52..6b733e93661 100644 --- a/app/models/concerns/reports/moderators_activity.rb +++ b/app/models/concerns/reports/moderators_activity.rb @@ -76,7 +76,7 @@ module Reports::ModeratorsActivity disagreed_by_id, deferred_by_id FROM post_actions - WHERE post_action_type_id IN (#{PostActionType.flag_types_without_custom.values.join(",")}) + WHERE post_action_type_id IN (#{PostActionType.flag_types_without_additional_message.values.join(",")}) AND created_at >= '#{report.start_date}' AND created_at <= '#{report.end_date}' ), diff --git a/app/models/flag.rb b/app/models/flag.rb index afecc8cf9ba..91538ea79cf 100644 --- a/app/models/flag.rb +++ b/app/models/flag.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class Flag < ActiveRecord::Base + # TODO(2025-01-15): krisk remove + self.ignored_columns = ["custom_type"] + DEFAULT_VALID_APPLIES_TO = %w[Post Topic] MAX_SYSTEM_FLAG_ID = 1000 MAX_NAME_LENGTH = 200 @@ -62,7 +65,7 @@ end # description :text # notify_type :boolean default(FALSE), not null # auto_action_type :boolean default(FALSE), not null -# custom_type :boolean default(FALSE), not null +# require_message :boolean default(FALSE), not null # applies_to :string not null, is an Array # position :integer not null # enabled :boolean default(TRUE), not null diff --git a/app/models/post.rb b/app/models/post.rb index e8ab24491b9..cbf00888fac 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -556,7 +556,7 @@ class Post < ActiveRecord::Base def flags post_actions.where( - post_action_type_id: PostActionType.flag_types_without_custom.values, + post_action_type_id: PostActionType.flag_types_without_additional_message.values, deleted_at: nil, ) end diff --git a/app/models/post_action_type.rb b/app/models/post_action_type.rb index b99450f2362..6eeeea19861 100644 --- a/app/models/post_action_type.rb +++ b/app/models/post_action_type.rb @@ -59,9 +59,9 @@ class PostActionType < ActiveRecord::Base @public_type_ids ||= public_types.values end - def flag_types_without_custom - return flag_settings.without_custom_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.reject(&:custom_type)) + 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 @@ -106,9 +106,9 @@ class PostActionType < ActiveRecord::Base flag_enum(all_flags.filter(&:enabled)) end - def custom_types - return flag_settings.custom_types if overridden_by_plugin_or_skipped_db? - flag_enum(all_flags.select(&:custom_type)) + def additional_message_types + return flag_settings.with_additional_message if overridden_by_plugin_or_skipped_db? + flag_enum(all_flags.select(&:require_message)) end def names diff --git a/app/models/user.rb b/app/models/user.rb index f38bafa1873..cd38dde9993 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1225,7 +1225,7 @@ class User < ActiveRecord::Base def flags_given_count PostAction.where( user_id: id, - post_action_type_id: PostActionType.flag_types_without_custom.values, + post_action_type_id: PostActionType.flag_types_without_additional_message.values, ).count end @@ -1236,7 +1236,10 @@ class User < ActiveRecord::Base def flags_received_count posts .includes(:post_actions) - .where("post_actions.post_action_type_id" => PostActionType.flag_types_without_custom.values) + .where( + "post_actions.post_action_type_id" => + PostActionType.flag_types_without_additional_message.values, + ) .count end diff --git a/app/serializers/flag_serializer.rb b/app/serializers/flag_serializer.rb index f5b04dd9e3e..eae1b940258 100644 --- a/app/serializers/flag_serializer.rb +++ b/app/serializers/flag_serializer.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class FlagSerializer < ApplicationSerializer - attributes :id, :name, :name_key, :description, :applies_to, :position, :enabled + attributes :id, :name, :name_key, :description, :applies_to, :position, :require_message, :enabled end diff --git a/app/serializers/post_action_type_serializer.rb b/app/serializers/post_action_type_serializer.rb index 8fffc4956ec..99fd16fe5aa 100644 --- a/app/serializers/post_action_type_serializer.rb +++ b/app/serializers/post_action_type_serializer.rb @@ -8,7 +8,7 @@ class PostActionTypeSerializer < ApplicationSerializer :description, :short_description, :is_flag, - :is_custom_flag, + :require_message, :enabled, :applies_to, :is_used, @@ -16,8 +16,8 @@ class PostActionTypeSerializer < ApplicationSerializer include ConfigurableUrls - def is_custom_flag - !!PostActionType.custom_types[object.id] + def require_message + !!PostActionType.additional_message_types[object.id] end def is_flag diff --git a/app/services/flags/create_flag.rb b/app/services/flags/create_flag.rb index 5df8f742e69..0db5616b8c4 100644 --- a/app/services/flags/create_flag.rb +++ b/app/services/flags/create_flag.rb @@ -15,6 +15,7 @@ class Flags::CreateFlag class Contract attribute :name, :string attribute :description, :string + attribute :require_message, :boolean attribute :enabled, :boolean attribute :applies_to validates :name, presence: true @@ -26,11 +27,12 @@ class Flags::CreateFlag private - def instantiate_flag(name:, description:, applies_to:, enabled:) + def instantiate_flag(name:, description:, applies_to:, require_message:, enabled:) Flag.new( name: name, description: description, applies_to: applies_to, + require_message: require_message, enabled: enabled, notify_type: true, ) @@ -51,6 +53,7 @@ class Flags::CreateFlag name: flag.name, description: flag.description, applies_to: flag.applies_to, + require_message: flag.require_message, enabled: flag.enabled, }, ) diff --git a/app/services/flags/update_flag.rb b/app/services/flags/update_flag.rb index 43b0c2f33c1..3ccf6f0a7d0 100644 --- a/app/services/flags/update_flag.rb +++ b/app/services/flags/update_flag.rb @@ -17,6 +17,7 @@ class Flags::UpdateFlag class Contract attribute :name, :string attribute :description, :string + attribute :require_message, :boolean attribute :enabled, :boolean attribute :applies_to validates :name, presence: true @@ -44,8 +45,14 @@ class Flags::UpdateFlag guardian.can_edit_flag?(flag) end - def update(flag:, name:, description:, applies_to:, enabled:) - flag.update!(name: name, description: description, applies_to: applies_to, enabled: enabled) + def update(flag:, name:, description:, applies_to:, require_message:, enabled:) + flag.update!( + name: name, + description: description, + applies_to: applies_to, + require_message: require_message, + enabled: enabled, + ) end def log(guardian:, flag:) @@ -55,6 +62,7 @@ class Flags::UpdateFlag name: flag.name, description: flag.description, applies_to: flag.applies_to, + require_message: flag.require_message, enabled: flag.enabled, }, ) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 1a30b5e896f..d1d4fe84480 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -5531,6 +5531,8 @@ en: non_editable: "You cannot edit this flag because it is a system flag or has already been used in the review system, however you can still disable it." delete_flag: "Delete flag" non_deletable: "You cannot delete this flag because it is a system flag or has already been used in the review system, however you can still disable it." + require_message: "Prompt users to provide additional reasons" + require_message_description: "When this flag is selected, a text field will be displayed for the user to provide additional detail about why they are flagging the content" more_options: title: "More options" move_up: "Move up" diff --git a/db/fixtures/003_flags.rb b/db/fixtures/003_flags.rb index b6c0d9670e6..f5a6bc781f0 100644 --- a/db/fixtures/003_flags.rb +++ b/db/fixtures/003_flags.rb @@ -5,7 +5,7 @@ Flag.seed do |s| s.name = "notify_user" s.notify_type = false s.auto_action_type = false - s.custom_type = true + s.require_message = true s.applies_to = %w[Post Chat::Message] end Flag.seed do |s| @@ -13,7 +13,7 @@ Flag.seed do |s| s.name = "off_topic" s.notify_type = true s.auto_action_type = true - s.custom_type = false + s.require_message = false s.applies_to = %w[Post Chat::Message] end Flag.seed do |s| @@ -21,7 +21,7 @@ Flag.seed do |s| s.name = "inappropriate" s.notify_type = true s.auto_action_type = true - s.custom_type = false + s.require_message = false s.applies_to = %w[Post Topic Chat::Message] end Flag.seed do |s| @@ -29,7 +29,7 @@ Flag.seed do |s| s.name = "spam" s.notify_type = true s.auto_action_type = true - s.custom_type = false + s.require_message = false s.applies_to = %w[Post Topic Chat::Message] end Flag.seed do |s| @@ -37,7 +37,7 @@ Flag.seed do |s| s.name = "illegal" s.notify_type = true s.auto_action_type = false - s.custom_type = true + s.require_message = true s.applies_to = %w[Post Topic Chat::Message] end Flag.seed do |s| @@ -45,7 +45,7 @@ Flag.seed do |s| s.name = "notify_moderators" s.notify_type = true s.auto_action_type = false - s.custom_type = true + s.require_message = true s.applies_to = %w[Post Topic Chat::Message] end Flag.unscoped.seed do |s| @@ -53,7 +53,7 @@ Flag.unscoped.seed do |s| s.name = "needs_approval" s.notify_type = false s.auto_action_type = false - s.custom_type = false + s.require_message = false s.score_type = true s.applies_to = %w[] end diff --git a/db/migrate/20240714231226_duplicate_flags_custom_type_to_require_message.rb b/db/migrate/20240714231226_duplicate_flags_custom_type_to_require_message.rb new file mode 100644 index 00000000000..85e259153b7 --- /dev/null +++ b/db/migrate/20240714231226_duplicate_flags_custom_type_to_require_message.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class DuplicateFlagsCustomTypeToRequireMessage < ActiveRecord::Migration[7.0] + def up + add_column :flags, :require_message, :boolean, default: false, null: false + + Migration::ColumnDropper.mark_readonly("flags", "custom_type") + + DB.exec <<~SQL + UPDATE flags + SET require_message = custom_type + SQL + end + + def down + Migration::ColumnDropper.drop_readonly("flags", "custom_type") + remove_column :flags, :require_message + end +end diff --git a/db/post_migrate/20240714231516_drop_custom_type_from_flags.rb b/db/post_migrate/20240714231516_drop_custom_type_from_flags.rb new file mode 100644 index 00000000000..c2075159f10 --- /dev/null +++ b/db/post_migrate/20240714231516_drop_custom_type_from_flags.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class DropCustomTypeFromFlags < ActiveRecord::Migration[7.0] + DROPPED_COLUMNS ||= { flags: %i[custom_type] } + + def up + DROPPED_COLUMNS.each { |table, columns| Migration::ColumnDropper.execute_drop(table, columns) } + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/badge_queries.rb b/lib/badge_queries.rb index 1be3b1a8085..6423747a9d0 100644 --- a/lib/badge_queries.rb +++ b/lib/badge_queries.rb @@ -73,7 +73,7 @@ module BadgeQueries SELECT pa.user_id, min(pa.id) id FROM post_actions pa JOIN badge_posts p on p.id = pa.post_id - WHERE post_action_type_id IN (#{PostActionType.flag_types_without_custom.values.join(",")}) AND + WHERE post_action_type_id IN (#{PostActionType.flag_types_without_additional_message.values.join(",")}) AND (:backfill OR pa.post_id IN (:post_ids) ) GROUP BY pa.user_id ) x diff --git a/lib/flag_settings.rb b/lib/flag_settings.rb index 7dbce78331f..631259fdfa8 100644 --- a/lib/flag_settings.rb +++ b/lib/flag_settings.rb @@ -2,11 +2,11 @@ class FlagSettings attr_reader( - :without_custom_types, + :without_additional_message_types, :notify_types, :topic_flag_types, :auto_action_types, - :custom_types, + :additional_message_types, :names, ) @@ -15,8 +15,8 @@ class FlagSettings @topic_flag_types = Enum.new @notify_types = Enum.new @auto_action_types = Enum.new - @custom_types = Enum.new - @without_custom_types = Enum.new + @additional_message_types = Enum.new + @without_additional_message_types = Enum.new @names = Enum.new end @@ -26,7 +26,7 @@ class FlagSettings topic_type: nil, notify_type: nil, auto_action_type: nil, - custom_type: nil, + require_message: nil, name: nil ) @all_flag_types[name_key] = id @@ -35,10 +35,10 @@ class FlagSettings @auto_action_types[name_key] = id if !!auto_action_type @names[id] = name if name - if !!custom_type - @custom_types[name_key] = id + if !!require_message + @additional_message_types[name_key] = id else - @without_custom_types[name_key] = id + @without_additional_message_types[name_key] = id end end diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 30eea751784..eec02252639 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -38,7 +38,8 @@ module PostGuardian taken = opts[:taken_actions].try(:keys).to_a is_flag = - PostActionType.notify_flag_types[action_key] || PostActionType.custom_types[action_key] + PostActionType.notify_flag_types[action_key] || + PostActionType.additional_message_types[action_key] already_taken_this_action = taken.any? && taken.include?(PostActionType.types[action_key]) already_did_flagging = taken.any? && (taken & PostActionType.notify_flag_types.values).any? diff --git a/lib/post_action_creator.rb b/lib/post_action_creator.rb index 1c905e2285c..23a5ee75047 100644 --- a/lib/post_action_creator.rb +++ b/lib/post_action_creator.rb @@ -115,7 +115,9 @@ class PostActionCreator # create meta topic / post if needed if @message.present? && - %i[notify_moderators notify_user spam illegal].include?(@post_action_name) + (PostActionType.additional_message_types.keys | %i[spam illegal]).include?( + @post_action_name, + ) creator = create_message_creator # We need to check if the creator exists because it's possible `create_message_creator` returns nil # in the event that a `post_action_notify_user_handler` evaluated to false, haulting the post creation. @@ -325,6 +327,7 @@ class PostActionCreator "post_action_types.#{@post_action_name}.email_title", title: @post.topic.title, locale: SiteSetting.default_locale, + default: I18n.t("post_action_types.illegal.email_title"), ) body = @@ -333,6 +336,7 @@ class PostActionCreator message: @message, link: "#{Discourse.base_url}#{@post.url}", locale: SiteSetting.default_locale, + default: I18n.t("post_action_types.illegal.email_body"), ) create_args = { @@ -342,7 +346,19 @@ class PostActionCreator raw: body, } - if %i[notify_moderators spam illegal].include?(@post_action_name) + if @post_action_name == :notify_user + create_args[:subtype] = TopicSubtype.notify_user + + create_args[:target_usernames] = @post.user.username + + # Evaluate DiscoursePluginRegistry.post_action_notify_user_handlers. + # If any return false, return early from this method + handler_values = + DiscoursePluginRegistry.post_action_notify_user_handlers.map do |handler| + handler.call(@created_by, @post, @message) + end + return if handler_values.any? { |value| value == false } + else create_args[:subtype] = TopicSubtype.notify_moderators create_args[:target_group_names] = [Group[:moderators].name] @@ -350,24 +366,6 @@ class PostActionCreator @post.topic&.category&.reviewable_by_group_id? create_args[:target_group_names] << @post.topic.category.reviewable_by_group.name end - else - create_args[:subtype] = TopicSubtype.notify_user - - if @post_action_name == :notify_user - create_args[:target_usernames] = @post.user.username - - # Evaluate DiscoursePluginRegistry.post_action_notify_user_handlers. - # If any return false, return early from this method - handler_values = - DiscoursePluginRegistry.post_action_notify_user_handlers.map do |handler| - handler.call(@created_by, @post, @message) - end - return if handler_values.any? { |value| value == false } - elsif @post_action_name != :notify_moderators - # this is a hack to allow a PM with no recipients, we should think through - # a cleaner technique, a PM with myself is valid for flagging - "x" - end end PostCreator.new(@created_by, create_args) diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index ad52a24f7cd..fc18c6e4ed6 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -542,7 +542,7 @@ class PostRevisor flaggers = [] @post .post_actions - .where(post_action_type_id: PostActionType.flag_types_without_custom.values) + .where(post_action_type_id: PostActionType.flag_types_without_additional_message.values) .each do |action| flaggers << action.user if action.user action.remove_act!(Discourse.system_user) diff --git a/spec/lib/flag_settings_spec.rb b/spec/lib/flag_settings_spec.rb index 2d34e194d65..deba079daed 100644 --- a/spec/lib/flag_settings_spec.rb +++ b/spec/lib/flag_settings_spec.rb @@ -21,7 +21,7 @@ RSpec.describe FlagSettings do settings.add(4, :inappropriate, topic_type: true) expect(settings.flag_types).to include(:inappropriate) expect(settings.topic_flag_types).to include(:inappropriate) - expect(settings.without_custom_types).to include(:inappropriate) + expect(settings.without_additional_message_types).to include(:inappropriate) end it "will add a notify type" do @@ -36,11 +36,11 @@ RSpec.describe FlagSettings do expect(settings.auto_action_types).to include(:notify_moderators) end - it "will add a custom type" do - settings.add(7, :notify_user, custom_type: true) + it "will add a type which requires message" do + settings.add(7, :notify_user, require_message: true) expect(settings.flag_types).to include(:notify_user) - expect(settings.custom_types).to include(:notify_user) - expect(settings.without_custom_types).to be_empty + expect(settings.additional_message_types).to include(:notify_user) + expect(settings.without_additional_message_types).to be_empty end end end diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index b1b7bafcd74..02f4d8f6581 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -767,7 +767,7 @@ RSpec.describe PostAction do # flags are already being tested all_types_except_flags = - PostActionType.types.except(*PostActionType.flag_types_without_custom.keys) + PostActionType.types.except(*PostActionType.flag_types_without_additional_message.keys) all_types_except_flags.values.each do |action| it "prevents user to act twice at the same time" do expect(PostActionCreator.new(eviltrout, post, action).perform).to be_success @@ -783,6 +783,25 @@ RSpec.describe PostAction do expect(result.reviewable_score.meta_topic_id).to be_nil end + it "does not create a message for custom flag when message is not required" do + flag_without_message = + Fabricate(:flag, name: "flag without message", notify_type: true, require_message: false) + + result = + PostActionCreator.new( + Discourse.system_user, + post, + PostActionType.types[:flag_without_message], + message: "WAT", + ).perform + + expect(result).to be_success + expect(result.post_action.related_post_id).to be_nil + expect(result.reviewable_score.meta_topic_id).to be_nil + + flag_without_message.destroy! + end + %i[notify_moderators notify_user spam].each do |post_action_type| it "creates a message for #{post_action_type}" do result = @@ -797,6 +816,25 @@ RSpec.describe PostAction do end end + it "creates a message for custom flags when message is required" do + flag_with_message = + Fabricate(:flag, name: "flag with message", notify_type: true, require_message: true) + + result = + PostActionCreator.new( + Discourse.system_user, + post, + PostActionType.types[:flag_with_message], + message: "WAT", + ).perform + + expect(result).to be_success + expect(result.post_action.related_post_id).to be_present + expect(result.reviewable_score.meta_topic_id).to be_present + + flag_with_message.destroy! + end + it "should raise the right errors when it fails to create a post" do user = Fabricate(:user) UserSilencer.new(user, Discourse.system_user).silence diff --git a/spec/requests/api/schemas/json/site_response.json b/spec/requests/api/schemas/json/site_response.json index 4a9392ed3ba..a432991de95 100644 --- a/spec/requests/api/schemas/json/site_response.json +++ b/spec/requests/api/schemas/json/site_response.json @@ -351,7 +351,7 @@ "is_flag": { "type": "boolean" }, - "is_custom_flag": { + "require_message": { "type": "boolean" }, "enabled": { @@ -371,7 +371,7 @@ "description", "short_description", "is_flag", - "is_custom_flag", + "require_message", "enabled", "applies_to", "is_used" @@ -403,7 +403,7 @@ "is_flag": { "type": "boolean" }, - "is_custom_flag": { + "require_message": { "type": "boolean" }, "enabled": { @@ -423,7 +423,7 @@ "description", "short_description", "is_flag", - "is_custom_flag", + "require_message", "enabled", "applies_to", "is_used" diff --git a/spec/services/flags/create_flag_spec.rb b/spec/services/flags/create_flag_spec.rb index 126f4ce3a93..0a99def3f8a 100644 --- a/spec/services/flags/create_flag_spec.rb +++ b/spec/services/flags/create_flag_spec.rb @@ -7,6 +7,7 @@ RSpec.describe(Flags::CreateFlag) do name: name, description: description, applies_to: applies_to, + require_message: require_message, enabled: enabled, ) end @@ -15,6 +16,7 @@ RSpec.describe(Flags::CreateFlag) do let(:description) { "custom flag description" } let(:applies_to) { ["Topic"] } let(:enabled) { true } + let(:require_message) { true } context "when user is not allowed to perform the action" do fab!(:current_user) { Fabricate(:user) } @@ -72,6 +74,7 @@ RSpec.describe(Flags::CreateFlag) do expect(flag.name).to eq("custom flag name") expect(flag.description).to eq("custom flag description") expect(flag.applies_to).to eq(["Topic"]) + expect(flag.require_message).to be true expect(flag.enabled).to be true end @@ -80,7 +83,7 @@ RSpec.describe(Flags::CreateFlag) do expect(UserHistory.last).to have_attributes( custom_type: "create_flag", details: - "name: custom flag name\ndescription: custom flag description\napplies_to: [\"Topic\"]\nenabled: true", + "name: custom flag name\ndescription: custom flag description\napplies_to: [\"Topic\"]\nrequire_message: true\nenabled: true", ) end end diff --git a/spec/services/flags/update_flag_spec.rb b/spec/services/flags/update_flag_spec.rb index 620a6bbcca9..8292c05ae45 100644 --- a/spec/services/flags/update_flag_spec.rb +++ b/spec/services/flags/update_flag_spec.rb @@ -10,6 +10,7 @@ RSpec.describe(Flags::UpdateFlag) do name: name, description: description, applies_to: applies_to, + require_message: require_message, enabled: enabled, ) end @@ -19,6 +20,7 @@ RSpec.describe(Flags::UpdateFlag) do let(:name) { "edited custom flag name" } let(:description) { "edited custom flag description" } let(:applies_to) { ["Topic"] } + let(:require_message) { true } let(:enabled) { false } context "when user is not allowed to perform the action" do @@ -74,6 +76,7 @@ RSpec.describe(Flags::UpdateFlag) do expect(flag.reload.name).to eq("edited custom flag name") expect(flag.description).to eq("edited custom flag description") expect(flag.applies_to).to eq(["Topic"]) + expect(flag.require_message).to be true expect(flag.enabled).to be false end @@ -82,7 +85,7 @@ RSpec.describe(Flags::UpdateFlag) do expect(UserHistory.last).to have_attributes( custom_type: "update_flag", details: - "name: edited custom flag name\ndescription: edited custom flag description\napplies_to: [\"Topic\"]\nenabled: false", + "name: edited custom flag name\ndescription: edited custom flag description\napplies_to: [\"Topic\"]\nrequire_message: true\nenabled: false", ) end end