DEV: move post flags into database #26951

lis2 · 2024-05-09T04:15:45Z

This is preparation for a feature that will allow admins to define their custom flags. Current behaviour should stay untouched.

A basic guardian was also defined. In addition, default translations are handled. Old flags will use translations, but at this point, admin defined flags will not be translated.

martin-brennan

Just a few initial comments, will leave it there for now though.

app/models/post_action_type.rb

app/models/post_flag.rb

config/locales/client.en.yml

This is preparation for a feature that will allow admins to define their custom flags. Current behaviour should stay untouched.

lis2 · 2024-05-14T03:15:00Z

db/migrate/20240423054323_create_flags.rb

+      t.boolean :topic_type, default: false, null: false
+      t.boolean :notify_type, default: false, null: false
+      t.boolean :auto_action_type, default: false, null: false
+      t.boolean :custom_type, default: false, null: false


@martin-brennan what do you think about adding chat_type? Even if right now, all custom flags will be working with chat as well, later we will be easily able to modify UI to be more granular. It might be premature, we can always add a column later.

I would like to have something reasonably safe deployed and then make constant improvements.

I think we need to rethink these type columns based on the discussions in /t/127494/29 internally, where we have landed on adding an applies_to column (or something similar), which would indicate an array of polymorphic types that the flag applies to (e.g. Post, Topic, Chat::Message).

Plugins would need to be able to register their own type so it can be chosen in the UI and stored in this column, and so we can tell via DiscoursePluginRegistry whether to render flags of that type based on whether the plugin is enabled.

The other column names here (notify_type, auto_action_type, custom_type) are all a bit confusing too, because they are just boolean columns indicating different configurations for the flag. I can see this has been partially informed by FlagSettings, but I think they could be a bit more generic. Something like:

notify_type - will_notify

auto_action_type - performs_auto_action

custom_type - ???

What I really struggle with is what all of these actually mean practically. Do you have a definition or idea of what each of them do, and if so can you give me a quick summary? Especially the notify_type is quite confusing; if we wanted to expand the flag to notify certain groups per /t/127978 how would we do this? Why are notify_user and notify_moderators individual flag names rather than different configurations of the same flag? We need more clarity on this before proceeding.

And what makes a flag be custom_type true? It seems very arbitrary right now.

This can mostly be disregarded now, see latest comment. We still do need to think about how notify/auto action stuff will work down the road though, which may require migrations.

martin-brennan

Okay there is a lot here, but we are building the foundation :) Sorry if some of this is a bit unclear, it's been a long day, please let me know if you need clarification on anything. The main thing I am concerned about is the various different "things" each flag can be used against (post, topic, chat message) and how the existing legacy post action type and reviewable score records tie into all of this.

app/models/flag.rb

martin-brennan · 2024-05-14T06:07:37Z

app/models/post_action_type.rb

+      Flag
+        .enabled
+        .order(:position)
+        .each do |post_flag|


Still called post_flag here, it should be just flag.

Also once we have applies_to (or similar) column we would only want to get the applies_to: ["Post"] flags here.

martin-brennan · 2024-05-14T06:10:20Z

db/migrate/20240423054323_create_flags.rb

+      t.boolean :topic_type, default: false, null: false
+      t.boolean :notify_type, default: false, null: false
+      t.boolean :auto_action_type, default: false, null: false
+      t.boolean :custom_type, default: false, null: false


I think we need to rethink these type columns based on the discussions in /t/127494/29 internally, where we have landed on adding an applies_to column (or something similar), which would indicate an array of polymorphic types that the flag applies to (e.g. Post, Topic, Chat::Message).

Plugins would need to be able to register their own type so it can be chosen in the UI and stored in this column, and so we can tell via DiscoursePluginRegistry whether to render flags of that type based on whether the plugin is enabled.

The other column names here (notify_type, auto_action_type, custom_type) are all a bit confusing too, because they are just boolean columns indicating different configurations for the flag. I can see this has been partially informed by FlagSettings, but I think they could be a bit more generic. Something like:

notify_type - will_notify

auto_action_type - performs_auto_action

custom_type - ???

What I really struggle with is what all of these actually mean practically. Do you have a definition or idea of what each of them do, and if so can you give me a quick summary? Especially the notify_type is quite confusing; if we wanted to expand the flag to notify certain groups per /t/127978 how would we do this? Why are notify_user and notify_moderators individual flag names rather than different configurations of the same flag? We need more clarity on this before proceeding.

martin-brennan · 2024-05-14T06:12:10Z

app/models/flag.rb

+# frozen_string_literal: true
+
+class Flag < ActiveRecord::Base
+  scope :enabled, -> { where(enabled: true) }


Might also want a scope to only return the system flags.

martin-brennan · 2024-05-14T06:12:23Z

spec/lib/guardian/flag_guardian_spec.rb

+    end
+
+    it "returns false when flag is system" do
+      flag.update!(id: 5)


Rather than updating this one, we should query the system flag, since it will exist in the database from the seed. E.g. Flag.where(name: "off_topic"), or if you had a scope you could do Flag.system.first

martin-brennan · 2024-05-14T07:36:34Z

db/migrate/20240423054323_create_flags.rb

+class CreateFlags < ActiveRecord::Migration[7.0]
+  def change
+    create_table :flags do |t|
+      t.string :name, unique: true


I feel like the name here should be the name_key in the database, and we should have a separate name column which would be the real, human-readable name for the custom flag. We would calculate the name_key when the flag is created and ensure it does not overlap with any of the system flags (which would have an empty name column since we have translations for them).

We also need a description column here for the custom flags.

martin-brennan · 2024-05-14T07:37:05Z

db/migrate/20240423054323_create_flags.rb

+      t.boolean :topic_type, default: false, null: false
+      t.boolean :notify_type, default: false, null: false
+      t.boolean :auto_action_type, default: false, null: false
+      t.boolean :custom_type, default: false, null: false


And what makes a flag be custom_type true? It seems very arbitrary right now.

martin-brennan · 2024-05-14T07:41:31Z

app/models/flag.rb

+  end
+
+  def self.reset_flag_settings!
+    PostActionType.reload_types


I think it would be worth doing a writeup about how the PostActionType and ReviewableScore flag types differ. All of this intertwining with PostActionType makes me nervous, but maybe we can't do much about it for historical reasons. I just want us to be super clear about how the flagging system fits together especially when it comes to custom flags defined by admins.

martin-brennan · 2024-05-14T07:41:53Z

db/fixtures/003_flags.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+Flag.seed do |s|


Just double checking, these do get created on an existing site right?

martin-brennan · 2024-05-15T01:14:28Z

OK re-evaluating based on your internal comments and guidance on the _type columns Kris, I think the main change we need to make here is to:

Add an applies_to column which would be either an array type or just a simple string type with a separator like Post|Topic|Chat::Message
Remove the topic_type since it is covered by the above
Do the name/name_key changes suggested, I think that will help us split the translation out and it will be good to cook the name key underscore replacement stuff on create
Add a description column

Then also just the few other suggestions here that are more minor then it can be good to go.

lis2 · 2024-05-16T00:44:49Z

@martin-brennan thank you for the review. Everything should be fixed 🤞

martin-brennan

I think this is a good foundation Kris, thanks for taking the time to address review comments :)

martin-brennan · 2024-05-20T00:51:43Z

app/models/flag.rb

+  end
+
+  def set_name_key
+    self.name_key = self.name.gsub(" ", "_").gsub(/[^\w]/, "").downcase


Might be worth adding a couple of unit tests for the model with variations on the name to make sure that this name_key method works OK (e.g. with special characters, things like It's Illegal)

github-actions bot added the i18n PRs which update English locale files or i18n related code label May 9, 2024

martin-brennan reviewed May 10, 2024

View reviewed changes

DEV: move post flags into database

cfdd870

This is preparation for a feature that will allow admins to define their custom flags. Current behaviour should stay untouched.

lis2 force-pushed the custom-flags branch 2 times, most recently from d4f1a1f to 286d8ad Compare May 13, 2024 04:30

FIX: first fixes after code review

b0161a0

lis2 force-pushed the custom-flags branch from 286d8ad to b0161a0 Compare May 13, 2024 05:39

github-actions bot added the chat PRs which include a change to Chat plugin label May 13, 2024

lis2 added 2 commits May 14, 2024 12:51

FIX: changes for chat

74a0709

FIX: remove duplicated translations

dd1ef0a

lis2 marked this pull request as ready for review May 14, 2024 03:07

lis2 requested a review from martin-brennan May 14, 2024 03:07

lis2 commented May 14, 2024

View reviewed changes

martin-brennan reviewed May 14, 2024

View reviewed changes

lis2 added 4 commits May 15, 2024 16:02

FIX: more improvements

c50a2f2

FIX: chat

cba62c4

FIX: system flag

486230c

FIX: annotations

f1dbeae

martin-brennan approved these changes May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEV: move post flags into database #26951

DEV: move post flags into database #26951

lis2 commented May 9, 2024

martin-brennan left a comment

lis2 May 14, 2024 •

edited

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 15, 2024

martin-brennan left a comment

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan May 14, 2024

martin-brennan commented May 15, 2024

lis2 commented May 16, 2024

martin-brennan left a comment

martin-brennan May 20, 2024

		@@ -0,0 +1,56 @@
		# frozen_string_literal: true

		Flag.seed do \|s\|

DEV: move post flags into database #26951

Are you sure you want to change the base?

DEV: move post flags into database #26951

Conversation

lis2 commented May 9, 2024

martin-brennan left a comment

Choose a reason for hiding this comment

lis2 May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-brennan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-brennan commented May 15, 2024

lis2 commented May 16, 2024

martin-brennan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lis2 May 14, 2024 •

edited