From 1f1709d249cf282baf532b7dd6c599d0e9c88b6a Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Wed, 11 Sep 2024 15:30:20 +1000 Subject: [PATCH] FIX: use a custom prefix for custom flags (#28839) Currently, when the custom flag has the same name as the system flag (which is disabled) then it is not displayed. To fix the problem, `custom_` prefix as `name_key` is used to distinguish between the system and the custom flag. I considered writing a migration to fix existing custom flags name key. However, at the end of migration I would need to run rails code to reset cache `Flag.reset_flag_settings!`. I decided to skip that step as it is a very edge case. If someone has the same flag name as the system flag, then all they have to do is edit the flag and click save. In addition, I made 2 small fixes: - edit flag title was missing translation; - flag form UI was not showing that description is the required field. --- .../addon/components/admin-flags-form.gjs | 2 +- app/models/flag.rb | 5 ++- config/locales/client.en.yml | 1 + spec/lib/post_action_type_view_spec.rb | 4 +- spec/models/flag_spec.rb | 37 ++++++++++++------- spec/models/post_action_spec.rb | 4 +- spec/system/admin_flags_spec.rb | 26 ++++++++++++- 7 files changed, 56 insertions(+), 23 deletions(-) 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 bf0ffab74cd..2d09b602f6d 100644 --- a/app/assets/javascripts/admin/addon/components/admin-flags-form.gjs +++ b/app/assets/javascripts/admin/addon/components/admin-flags-form.gjs @@ -135,7 +135,7 @@ export default class AdminFlagsForm extends Component { diff --git a/app/models/flag.rb b/app/models/flag.rb index e10fffb4aef..619e43bdd00 100644 --- a/app/models/flag.rb +++ b/app/models/flag.rb @@ -43,7 +43,7 @@ class Flag < ActiveRecord::Base end def system? - self.id < MAX_SYSTEM_FLAG_ID + self.id.present? && self.id < MAX_SYSTEM_FLAG_ID end def applies_to?(type) @@ -61,7 +61,8 @@ class Flag < ActiveRecord::Base end def set_name_key - self.name_key = self.name.squeeze(" ").gsub(" ", "_").gsub(/[^\w]/, "").downcase + prefix = self.system? ? "" : "custom_" + self.name_key = "#{prefix}#{self.name.squeeze(" ").gsub(" ", "_").gsub(/[^\w]/, "").downcase}" end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 2db37838c13..7568f3f125e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -5617,6 +5617,7 @@ en: saved: "saved!" flags: header: "Moderation Flags" + edit_header: "Edit Flag" subheader: "The flagging system in Discourse helps you and your moderator team manage content and user behavior, keeping your community respectful and healthy. The defaults are suitable for most communities and you don’t have to change them. However, if your site has particular requirements you can disable flags you don’t need and add your own custom flags." description: "Description" enabled: "Enabled?" diff --git a/spec/lib/post_action_type_view_spec.rb b/spec/lib/post_action_type_view_spec.rb index 607839522b7..5d5a85fc04c 100644 --- a/spec/lib/post_action_type_view_spec.rb +++ b/spec/lib/post_action_type_view_spec.rb @@ -29,8 +29,8 @@ RSpec.describe PostActionTypeView do ) 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 = Fabricate(:flag, name: "flag", enabled: false) + expect(PostActionTypeView.new.disabled_flag_types).to eq({ custom_flag: flag.id }) flag.destroy! end diff --git a/spec/models/flag_spec.rb b/spec/models/flag_spec.rb index c090b854cd0..9c2597e815e 100644 --- a/spec/models/flag_spec.rb +++ b/spec/models/flag_spec.rb @@ -17,14 +17,14 @@ RSpec.describe Flag, type: :model do end it "has correct name key" do - flag = Fabricate(:flag, name: "CuStOm Flag!!!") + flag = Fabricate(:flag, name: "FlAg!!!") expect(flag.name_key).to eq("custom_flag") flag.update!(name: "It's Illegal") - expect(flag.name_key).to eq("its_illegal") + expect(flag.name_key).to eq("custom_its_illegal") flag.update!(name: "THIS IS SPaM!+)(*&^%$#@@@!)") - expect(flag.name_key).to eq("this_is_spam") + expect(flag.name_key).to eq("custom_this_is_spam") flag.destroy! end @@ -37,17 +37,9 @@ RSpec.describe Flag, type: :model do %i[notify_user off_topic inappropriate spam illegal notify_moderators needs_approval], ) - flag = Fabricate(:flag, name: "custom") + flag = Fabricate(:flag, name: "flag") expect(PostActionType.flag_types.keys).to eq( - %i[notify_user off_topic inappropriate spam illegal notify_moderators custom], - ) - expect(ReviewableScore.types.keys).to eq( - %i[notify_user off_topic inappropriate spam illegal notify_moderators custom needs_approval], - ) - - flag.update!(name: "edited_custom") - expect(PostActionType.flag_types.keys).to eq( - %i[notify_user off_topic inappropriate spam illegal notify_moderators edited_custom], + %i[notify_user off_topic inappropriate spam illegal notify_moderators custom_flag], ) expect(ReviewableScore.types.keys).to eq( %i[ @@ -57,7 +49,24 @@ RSpec.describe Flag, type: :model do spam illegal notify_moderators - edited_custom + custom_flag + needs_approval + ], + ) + + flag.update!(name: "edited_flag") + expect(PostActionType.flag_types.keys).to eq( + %i[notify_user off_topic inappropriate spam illegal notify_moderators custom_edited_flag], + ) + expect(ReviewableScore.types.keys).to eq( + %i[ + notify_user + off_topic + inappropriate + spam + illegal + notify_moderators + custom_edited_flag needs_approval ], ) diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 01c31448dea..708ab07bf6c 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -791,7 +791,7 @@ RSpec.describe PostAction do PostActionCreator.new( Discourse.system_user, post, - PostActionType.types[:flag_without_message], + PostActionType.types[:custom_flag_without_message], message: "WAT", ).perform @@ -824,7 +824,7 @@ RSpec.describe PostAction do PostActionCreator.new( Discourse.system_user, post, - PostActionType.types[:flag_with_message], + PostActionType.types[:custom_flag_with_message], message: "WAT", ).perform diff --git a/spec/system/admin_flags_spec.rb b/spec/system/admin_flags_spec.rb index d7aec68d877..f826946a30b 100644 --- a/spec/system/admin_flags_spec.rb +++ b/spec/system/admin_flags_spec.rb @@ -108,7 +108,7 @@ describe "Admin Flags Page", type: :system do ) # update - admin_flags_page.visit.click_edit_flag("vulgar") + admin_flags_page.visit.click_edit_flag("custom_vulgar") admin_flag_form_page.fill_in_name("Tasteless").click_save expect(admin_flags_page).to have_flags( @@ -132,7 +132,7 @@ describe "Admin Flags Page", type: :system do ) # delete - admin_flags_page.visit.click_delete_flag("tasteless").confirm_delete + admin_flags_page.visit.click_delete_flag("custom_tasteless").confirm_delete expect(admin_flags_page).to have_no_flag("tasteless") @@ -146,6 +146,28 @@ describe "Admin Flags Page", type: :system do "It's Illegal", "Something Else", ) + + # custom flag with same name as system flag + admin_flags_page.visit.toggle("inappropriate") + admin_flags_page.click_add_flag + admin_flag_form_page + .fill_in_name("Inappropriate") + .fill_in_description("New flag description") + .select_applies_to("Topic") + .select_applies_to("Post") + .click_save + + topic_page.visit_topic(post.topic).open_flag_topic_modal + + expect(flag_modal).to have_choices( + "It's Spam", + "It's Illegal", + "Something Else", + "Inappropriate", + ) + + Flag.system.where(name: "illegal").update!(enabled: true) + admin_flags_page.visit.click_delete_flag("custom_inappropriate").confirm_delete end it "has settings tab" do