FIX: first fixes after code review

app/models/post_flag.rb → app/models/flag.rb

@@ -1,35 +1,32 @@
 # frozen_string_literal: true
 
-class PostFlag < ActiveRecord::Base
+class Flag < ActiveRecord::Base
   scope :enabled, -> { where(enabled: true) }
 
-  before_create :set_custom_id, unless: :system?
   before_save :set_position
-  after_save :reset_post_action_types!
-  after_destroy :reset_post_action_types!
+  after_save :reset_flag_settings!
+  after_destroy :reset_flag_settings!
 
   def used?
     PostAction.exists?(post_action_type_id: self.id)
   end
 
-  def self.reset_post_action_types!
+  def self.reset_flag_settings!
     PostActionType.initialize_flag_settings
   end
 
-  private
-
-  def reset_post_action_types!
-    self.class.reset_post_action_types!
+  def system?
+    self.id < 1000
   end
 
-  def set_custom_id
-    return if self.id.to_i > 1000
-    self.id = PostFlag.maximum(:id).next
-    self.id += 1000
+  private
+
+  def reset_flag_settings!
+    self.class.reset_flag_settings!
   end
 
   def set_position
-    self.position = PostFlag.maximum(:position).to_i + 1 if !self.position
+    self.position = Flag.maximum(:position).to_i + 1 if !self.position
   end
 end
 

app/models/post_action_type.rb

@@ -82,21 +82,19 @@ def is_flag?(sym)
     def initialize_flag_settings
       @types = nil
       @flag_settings = FlagSettings.new
-      if ActiveRecord::Base.connection.table_exists?(:post_flags)
-        PostFlag
-          .enabled
-          .order(:position)
-          .find_each do |post_flag|
-            @flag_settings.add(
-              post_flag.id,
-              post_flag.name.to_sym,
-              topic_type: post_flag.topic_type,
-              notify_type: post_flag.notify_type,
-              auto_action_type: post_flag.auto_action_type,
-              custom_type: post_flag.custom_type,
-            )
-          end
-      end
+      Flag
+        .enabled
+        .order(:position)
+        .each do |post_flag|
+          @flag_settings.add(
+            post_flag.id,
+            post_flag.name.to_sym,
+            topic_type: post_flag.topic_type,
+            notify_type: post_flag.notify_type,
+            auto_action_type: post_flag.auto_action_type,
+            custom_type: post_flag.custom_type,
+          )
+        end
     end
   end
 

db/fixtures/003_post_flags.rb → db/fixtures/003_flags.rb

@@ -1,60 +1,54 @@
 # frozen_string_literal: true
 
-PostFlag.seed do |s|
+Flag.seed do |s|
   s.id = 3
   s.name = "off_topic"
   s.position = 2
-  s.system = true
   s.topic_type = false
   s.notify_type = true
   s.auto_action_type = true
   s.custom_type = false
 end
-PostFlag.seed do |s|
+Flag.seed do |s|
   s.id = 4
   s.name = "inappropriate"
   s.position = 3
-  s.system = true
   s.topic_type = true
   s.notify_type = true
   s.auto_action_type = true
   s.custom_type = false
 end
-PostFlag.seed do |s|
+Flag.seed do |s|
   s.id = 8
   s.name = "spam"
   s.position = 4
-  s.system = true
   s.topic_type = true
   s.notify_type = true
   s.auto_action_type = true
   s.custom_type = false
 end
-PostFlag.seed do |s|
+Flag.seed do |s|
   s.id = 6
   s.name = "notify_user"
   s.position = 0
-  s.system = true
   s.topic_type = false
   s.notify_type = false
   s.auto_action_type = false
   s.custom_type = true
 end
-PostFlag.seed do |s|
+Flag.seed do |s|
   s.id = 7
   s.name = "notify_moderators"
   s.position = 1
-  s.system = true
   s.topic_type = true
   s.notify_type = true
   s.auto_action_type = true
   s.custom_type = true
 end
-PostFlag.seed do |s|
+Flag.seed do |s|
   s.id = 10
   s.name = "illegal"
   s.position = 5
-  s.system = true
   s.topic_type = true
   s.notify_type = true
   s.auto_action_type = true

db/migrate/20240423054323_create_post_flags.rb → db/migrate/20240423054323_create_flags.rb

@@ -1,17 +1,17 @@
 # frozen_string_literal: true
 
-class CreatePostFlags < ActiveRecord::Migration[7.0]
+class CreateFlags < ActiveRecord::Migration[7.0]
   def change
-    create_table :post_flags do |t|
+    create_table :flags do |t|
       t.string :name, unique: true
       t.integer :position, null: false
-      t.boolean :system, null: false
       t.boolean :enabled, default: true, null: false
       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
       t.timestamps
     end
+    DB.exec("SELECT setval('flags_id_seq', 1001, FALSE);")
   end
 end

lib/guardian.rb

@@ -4,7 +4,7 @@
 require "guardian/category_guardian"
 require "guardian/ensure_magic"
 require "guardian/group_guardian"
-require "guardian/post_flag_guardian"
+require "guardian/flag_guardian"
 require "guardian/post_guardian"
 require "guardian/post_revision_guardian"
 require "guardian/sidebar_guardian"
@@ -17,9 +17,9 @@ class Guardian
   include BookmarkGuardian
   include CategoryGuardian
   include EnsureMagic
+  include FlagGuardian
   include GroupGuardian
   include PostGuardian
-  include PostFlagGuardian
   include PostRevisionGuardian
   include SidebarGuardian
   include TagGuardian

lib/guardian/post_flag_guardian.rb → lib/guardian/flag_guardian.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-module PostFlagGuardian
+module FlagGuardian
   def can_edit_post_flag?(post_flag)
     @user.admin? && !post_flag.system? && !post_flag.used?
   end

plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb

@@ -1166,7 +1166,7 @@
 
       describe "when post contain the right answer" do
         let(:post) { Fabricate(:post, user: discobot_user, topic: topic) }
-        let(:flag) { Fabricate(:flag, post: post, user: user) }
+        let(:flag) { Fabricate(:flag_post_action, post: post, user: user) }
 
         before do
           narrative.set_data(user, state: :tutorial_flag, topic_id: topic.id)

spec/fabricators/flag_fabricator.rb

@@ -1,7 +1,3 @@
 # frozen_string_literal: true
 
-Fabricator(:flag, from: :post_action) do
-  user
-  post
-  post_action_type_id PostActionType.types[:spam]
-end
+Fabricator(:flag) { name "offtopic" }

spec/fabricators/flag_post_action_fabricator.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+Fabricator(:flag_post_action, from: :post_action) do
+  user
+  post
+  post_action_type_id PostActionType.types[:spam]
+end

spec/lib/composer_messages_finder_spec.rb

@@ -360,7 +360,7 @@
     end
 
     it "shows a message when the replier has already flagged the post" do
-      Fabricate(:flag, post: self_flagged_post, user: user)
+      Fabricate(:flag_post_action, post: self_flagged_post, user: user)
       finder =
         ComposerMessagesFinder.new(
           user,
@@ -372,13 +372,13 @@
     end
 
     it "shows a message when replying to flagged topic (first post)" do
-      Fabricate(:flag, post: original_post, user: user)
+      Fabricate(:flag_post_action, post: original_post, user: user)
       finder = ComposerMessagesFinder.new(user, composer_action: "reply", topic_id: topic.id)
       expect(finder.check_dont_feed_the_trolls).to be_present
     end
 
     it "does not show a message when not enough others have flagged the post" do
-      Fabricate(:flag, post: under_flagged_post, user: other_user)
+      Fabricate(:flag_post_action, post: under_flagged_post, user: other_user)
       finder =
         ComposerMessagesFinder.new(
           user,
@@ -392,7 +392,12 @@
     it "does not show a message when the flag has already been resolved" do
       SiteSetting.dont_feed_the_trolls_threshold = 1
 
-      Fabricate(:flag, post: resolved_flag_post, user: other_user, disagreed_at: 1.hour.ago)
+      Fabricate(
+        :flag_post_action,
+        post: resolved_flag_post,
+        user: other_user,
+        disagreed_at: 1.hour.ago,
+      )
       finder =
         ComposerMessagesFinder.new(
           user,
@@ -404,8 +409,8 @@
     end
 
     it "shows a message when enough others have already flagged the post" do
-      Fabricate(:flag, post: over_flagged_post, user: other_user)
-      Fabricate(:flag, post: over_flagged_post, user: third_user)
+      Fabricate(:flag_post_action, post: over_flagged_post, user: other_user)
+      Fabricate(:flag_post_action, post: over_flagged_post, user: third_user)
       finder =
         ComposerMessagesFinder.new(
           user,

spec/lib/guardian/post_flag_guardian_spec.rb → spec/lib/guardian/flag_guardian_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe PostFlagGuardian do
+RSpec.describe FlagGuardian do
   fab!(:user)
   fab!(:admin)
   fab!(:post_flag)

spec/lib/post_revisor_spec.rb

@@ -484,7 +484,7 @@
 
         post = Fabricate(:post, raw: "hello world")
 
-        Fabricate(:flag, post: post, user: user)
+        Fabricate(:flag_post_action, post: post, user: user)
 
         revisor = PostRevisor.new(post)
         revisor.revise!(

spec/models/flag_spec.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+RSpec.describe Flag, type: :model do
+  it "has id lower than 1000 for system flags" do
+    flag = Fabricate(:flag, id: 1)
+    expect(flag.system?).to be true
+  end
+
+  it "has id greater than 1000 for non-system flags" do
+    flag = Fabricate(:flag)
+    expect(flag.system?).to be false
+    expect(flag.id).to be > 1000
+  end
+
+  before { Flag.reset_flag_settings! }
+
+  it "updates post action types when created, modified or destroyed" do
+    expect(PostActionType.flag_types.keys).to eq(
+      %i[notify_user notify_moderators off_topic inappropriate spam illegal],
+    )
+
+    flag = Fabricate(:flag, name: "custom")
+    expect(PostActionType.flag_types.keys).to eq(
+      %i[notify_user notify_moderators off_topic inappropriate spam illegal custom],
+    )
+
+    flag.update!(name: "edited_custom")
+    expect(PostActionType.flag_types.keys).to eq(
+      %i[notify_user notify_moderators off_topic inappropriate spam illegal edited_custom],
+    )
+
+    flag.destroy!
+    expect(PostActionType.flag_types.keys).to eq(
+      %i[notify_user notify_moderators off_topic inappropriate spam illegal],
+    )
+  end
+end

spec/services/auto_silence_spec.rb

@@ -72,7 +72,11 @@
     end
 
     it "returns 0 when there is one flag that has a reason other than spam" do
-      Fabricate(:flag, post: post, post_action_type_id: PostActionType.types[:off_topic])
+      Fabricate(
+        :flag_post_action,
+        post: post,
+        post_action_type_id: PostActionType.types[:off_topic],
+      )
       expect(count).to eq(0)
     end
 

spec/system/composer/dont_feed_the_trolls_popup_spec.rb

@@ -6,7 +6,7 @@
   fab!(:topic) { Fabricate(:topic, user: user) }
   fab!(:post) { Fabricate(:post, user: user, topic: topic) }
   fab!(:reply) { Fabricate(:post, user: troll, topic: topic) }
-  fab!(:flag) { Fabricate(:flag, post: reply, user: user) }
+  fab!(:flag) { Fabricate(:flag_post_action, post: reply, user: user) }
   let(:topic_page) { PageObjects::Pages::Topic.new }
 
   before { sign_in user }