DEV: Add auto map from TL -> group site settings in DeprecatedSettings (#24959)

When setting an old TL based site setting in the console e.g.:

SiteSetting.min_trust_level_to_allow_ignore = TrustLevel[3]

We will silently convert this to the corresponding Group::AUTO_GROUP. And vice-versa, when we read the value on the old setting, we will automatically get the lowest trust level corresponding to the lowest auto group for the new setting in the database.

app/models/group.rb

@@ -145,6 +145,15 @@ class Group < ActiveRecord::Base
     @visibility_levels = Enum.new(public: 0, logged_on_users: 1, members: 2, staff: 3, owners: 4)
   end
 
+  def self.auto_groups_between(lower, upper)
+    lower_group = Group::AUTO_GROUPS[lower.to_sym]
+    upper_group = Group::AUTO_GROUPS[upper.to_sym]
+
+    return [] if lower_group.blank? || upper_group.blank?
+
+    (lower_group..upper_group).to_a & AUTO_GROUPS.values
+  end
+
   validates :mentionable_level, inclusion: { in: ALIAS_LEVELS.values }
   validates :messageable_level, inclusion: { in: ALIAS_LEVELS.values }
 

lib/site_settings/deprecated_settings.rb

@@ -35,9 +35,80 @@ module SiteSettings::DeprecatedSettings
     ["min_trust_level_to_allow_ignore", "ignore_allowed_groups", false, "3.3"],
   ]
 
+  OVERRIDE_TL_GROUP_SETTINGS = %w[
+    anonymous_posting_min_trust_level
+    shared_drafts_min_trust_level
+    min_trust_level_for_here_mention
+    approve_unless_trust_level
+    approve_new_topics_unless_trust_level
+    email_in_min_trust
+    min_trust_to_edit_wiki_post
+    allow_uploaded_avatars
+    min_trust_to_create_topic
+    min_trust_to_edit_post
+    min_trust_to_flag_posts
+    tl4_delete_posts_and_topics
+    min_trust_level_to_allow_user_card_background
+    min_trust_level_to_allow_invite
+    min_trust_level_to_allow_ignore
+  ]
+
+  def group_to_tl(old_setting, new_setting)
+    tl_and_staff = is_tl_and_staff_setting?(old_setting)
+
+    valid_auto_groups =
+      self.public_send("#{new_setting}_map") &
+        # We only want auto groups, no actual groups because they cannot be
+        # mapped to TLs.
+        Group.auto_groups_between(tl_and_staff ? :admins : :trust_level_0, :trust_level_4)
+
+    # We don't want to return nil because this could lead to permission holes;
+    # so we return the max available permission in this case.
+    return tl_and_staff ? "admin" : TrustLevel[4] if valid_auto_groups.empty?
+
+    if tl_and_staff
+      valid_auto_groups_excluding_staff_and_admins =
+        valid_auto_groups - [Group::AUTO_GROUPS[:staff], Group::AUTO_GROUPS[:admins]]
+
+      if valid_auto_groups_excluding_staff_and_admins.any?
+        return valid_auto_groups_excluding_staff_and_admins.min - Group::AUTO_GROUPS[:trust_level_0]
+      end
+
+      if valid_auto_groups.include?(Group::AUTO_GROUPS[:staff])
+        "staff"
+      elsif valid_auto_groups.include?(Group::AUTO_GROUPS[:admins])
+        "admin"
+      end
+    else
+      valid_auto_groups.min - Group::AUTO_GROUPS[:trust_level_0]
+    end
+  end
+
+  def tl_to_group(old_setting, val)
+    tl_and_staff = is_tl_and_staff_setting?(old_setting)
+
+    if val == "admin"
+      Group::AUTO_GROUPS[:admins]
+    elsif val == "staff"
+      Group::AUTO_GROUPS[:staff]
+    else
+      if tl_and_staff
+        "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}|#{val.to_i + Group::AUTO_GROUPS[:trust_level_0]}"
+      else
+        "#{val.to_i + Group::AUTO_GROUPS[:trust_level_0]}"
+      end
+    end
+  end
+
+  def is_tl_and_staff_setting?(old_setting)
+    SiteSetting.type_supervisor.get_type(old_setting.to_sym) == :enum &&
+      SiteSetting.type_supervisor.get_enum_class(old_setting.to_sym).name ==
+        TrustLevelAndStaffSetting.name
+  end
+
   def setup_deprecated_methods
     SETTINGS.each do |old_setting, new_setting, override, version|
-      unless override
+      if !override
         SiteSetting.singleton_class.public_send(
           :alias_method,
           :"_#{old_setting}",
@@ -45,6 +116,12 @@ module SiteSettings::DeprecatedSettings
         )
       end
 
+      if OVERRIDE_TL_GROUP_SETTINGS.include?(old_setting)
+        define_singleton_method "_group_to_tl_#{old_setting}" do |warn: true|
+          group_to_tl(old_setting, new_setting)
+        end
+      end
+
       define_singleton_method old_setting do |warn: true|
         if warn
           Discourse.deprecate(
@@ -53,10 +130,14 @@ module SiteSettings::DeprecatedSettings
           )
         end
 
-        self.public_send(override ? new_setting : "_#{old_setting}")
+        if OVERRIDE_TL_GROUP_SETTINGS.include?(old_setting)
+          self.public_send("_group_to_tl_#{old_setting}")
+        else
+          self.public_send(override ? new_setting : "_#{old_setting}")
+        end
       end
 
-      unless override
+      if !override
         SiteSetting.singleton_class.public_send(
           :alias_method,
           :"_#{old_setting}?",
@@ -75,7 +156,7 @@ module SiteSettings::DeprecatedSettings
         self.public_send("#{override ? new_setting : "_" + old_setting}?")
       end
 
-      unless override
+      if !override
         SiteSetting.singleton_class.public_send(
           :alias_method,
           :"_#{old_setting}=",
@@ -91,7 +172,14 @@ module SiteSettings::DeprecatedSettings
           )
         end
 
-        self.public_send("#{override ? new_setting : "_" + old_setting}=", val)
+        if OVERRIDE_TL_GROUP_SETTINGS.include?(old_setting)
+          # We want to set both the new group setting here to the equivalent of the
+          # TL, as well as setting the TL value in the DB so they remain in sync.
+          self.public_send("_#{old_setting}=", val)
+          self.public_send("#{new_setting}=", tl_to_group(old_setting, val).to_s)
+        else
+          self.public_send("#{override ? new_setting : "_" + old_setting}=", val)
+        end
       end
     end
   end

lib/site_settings/type_supervisor.rb

@@ -173,7 +173,7 @@ class SiteSettings::TypeSupervisor
     result = { type: type.to_s }
 
     if type == :enum
-      if (klass = enum_class(name))
+      if (klass = get_enum_class(name))
         result.merge!(valid_values: klass.values, translate_names: klass.translate_names?)
       else
         result.merge!(
@@ -206,6 +206,10 @@ class SiteSettings::TypeSupervisor
     result
   end
 
+  def get_enum_class(name)
+    @enums[name]
+  end
+
   def get_type(name)
     self.class.types[@types[name.to_sym]]
   end
@@ -239,8 +243,8 @@ class SiteSettings::TypeSupervisor
 
   def validate_value(name, type, val)
     if type == self.class.types[:enum]
-      if enum_class(name)
-        unless enum_class(name).valid_value?(val)
+      if get_enum_class(name)
+        unless get_enum_class(name).valid_value?(val)
           raise Discourse::InvalidParameters.new("Invalid value `#{val}` for `#{name}`")
         end
       else
@@ -289,10 +293,6 @@ class SiteSettings::TypeSupervisor
     self.class.parse_value_type(val)
   end
 
-  def enum_class(name)
-    @enums[name]
-  end
-
   def json_schema_class(name)
     @json_schemas[name]
   end

spec/fixtures/site_settings/deprecated_test.yml

@@ -0,0 +1,10 @@
+category1:
+  use_https: true
+  min_trust_level_to_allow_invite:
+    default: 2
+    enum: "TrustLevelSetting"
+    hidden: true
+  min_trust_level_to_allow_invite_tl_and_staff:
+    default: 2
+    enum: "TrustLevelAndStaffSetting"
+    hidden: true

spec/lib/site_settings/deprecated_settings_spec.rb

@@ -0,0 +1,233 @@
+# frozen_string_literal: true
+
+RSpec.describe SiteSettings::DeprecatedSettings do
+  def deprecate_override!(settings, tl_group_overrides = [])
+    @original_settings = SiteSettings::DeprecatedSettings::SETTINGS.dup
+    SiteSettings::DeprecatedSettings::SETTINGS.clear
+    SiteSettings::DeprecatedSettings::SETTINGS.push(settings)
+
+    if tl_group_overrides.any?
+      @original_override_tl_group = SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.dup
+      SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.clear
+      SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.push(*tl_group_overrides)
+    end
+
+    SiteSetting.setup_deprecated_methods
+  end
+
+  after do
+    if defined?(@original_settings)
+      SiteSettings::DeprecatedSettings::SETTINGS.clear
+      SiteSettings::DeprecatedSettings::SETTINGS.concat(@original_settings)
+    end
+
+    if defined?(@original_override_tl_group)
+      SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.clear
+      SiteSettings::DeprecatedSettings::OVERRIDE_TL_GROUP_SETTINGS.concat(
+        @original_override_tl_group,
+      )
+    end
+  end
+
+  describe "when not overriding deprecated settings" do
+    let(:override) { false }
+
+    # Necessary because Discourse.deprecate uses redis to see if the warning
+    # was already logged.
+    use_redis_snapshotting
+
+    # NOTE: This fixture has some completely made up settings (e.g. min_trust_level_to_allow_invite_tl_and_staff)
+    let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" }
+
+    before do
+      SiteSetting.force_https = true
+      SiteSetting.load_settings(deprecated_test)
+    end
+
+    it "should not act as a proxy to the new methods" do
+      deprecate_override!(["use_https", "force_https", override, "0.0.1"])
+
+      SiteSetting.use_https = false
+
+      expect(SiteSetting.force_https).to eq(true)
+      expect(SiteSetting.force_https?).to eq(true)
+    end
+
+    it "should log warnings when deprecated settings are called" do
+      deprecate_override!(["use_https", "force_https", override, "0.0.1"])
+
+      logger =
+        track_log_messages do
+          expect(SiteSetting.use_https).to eq(true)
+          expect(SiteSetting.use_https?).to eq(true)
+        end
+      expect(logger.warnings.count).to eq(3)
+
+      logger = track_log_messages { SiteSetting.use_https(warn: false) }
+      expect(logger.warnings.count).to eq(0)
+    end
+  end
+
+  describe "when overriding deprecated settings" do
+    let(:override) { true }
+    let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" }
+
+    before do
+      SiteSetting.force_https = true
+      SiteSetting.load_settings(deprecated_test)
+    end
+
+    it "should act as a proxy to the new methods" do
+      deprecate_override!(["use_https", "force_https", override, "0.0.1"])
+
+      SiteSetting.use_https = false
+
+      expect(SiteSetting.force_https).to eq(false)
+      expect(SiteSetting.force_https?).to eq(false)
+    end
+
+    it "should log warnings when deprecated settings are called" do
+      deprecate_override!(["use_https", "force_https", override, "0.0.1"])
+
+      logger =
+        track_log_messages do
+          expect(SiteSetting.use_https).to eq(true)
+          expect(SiteSetting.use_https?).to eq(true)
+        end
+      expect(logger.warnings.count).to eq(2)
+
+      logger = track_log_messages { SiteSetting.use_https(warn: false) }
+      expect(logger.warnings.count).to eq(0)
+    end
+  end
+
+  describe "when overriding a trust level setting with a group setting" do
+    let(:override) { false }
+    let(:deprecated_test) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_test.yml" }
+
+    before { SiteSetting.load_settings(deprecated_test) }
+
+    context "when getting an old TrustLevelSetting" do
+      before do
+        deprecate_override!(
+          ["min_trust_level_to_allow_invite", "invite_allowed_groups", override, "0.0.1"],
+        )
+      end
+
+      it "uses the minimum trust level from the trust level auto groups in the new group setting" do
+        SiteSetting.invite_allowed_groups =
+          "#{Group::AUTO_GROUPS[:trust_level_3]}|#{Group::AUTO_GROUPS[:trust_level_4]}"
+        expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[3])
+      end
+
+      it "returns TL4 if there are no trust level auto groups in the new group setting" do
+        SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s
+        expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4])
+      end
+
+      it "returns TL4 if there are only staff and admin auto groups in the new group setting" do
+        SiteSetting.invite_allowed_groups =
+          "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}"
+        expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4])
+      end
+
+      it "returns TL4 if there are no automated invite_allowed_groups" do
+        SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s
+        expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4])
+      end
+    end
+
+    context "when getting an old TrustLevelAndStaffSetting" do
+      before do
+        deprecate_override!(
+          [
+            "min_trust_level_to_allow_invite_tl_and_staff",
+            "invite_allowed_groups",
+            override,
+            "0.0.1",
+          ],
+          ["min_trust_level_to_allow_invite_tl_and_staff"],
+        )
+      end
+
+      it "returns staff if there are staff and admin auto groups in the new group setting" do
+        SiteSetting.invite_allowed_groups =
+          "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}"
+        expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("staff")
+      end
+
+      it "returns admin if there is only the admin auto group in the new group setting" do
+        SiteSetting.invite_allowed_groups = "#{Group::AUTO_GROUPS[:admins]}"
+        expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin")
+      end
+
+      it "returns the min trust level if the admin auto group as well as lower TL auto groups in the new group setting" do
+        SiteSetting.invite_allowed_groups =
+          "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:trust_level_3]}"
+        expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq(TrustLevel[3])
+      end
+
+      it "returns admin if there are no automated invite_allowed_groups" do
+        SiteSetting.invite_allowed_groups = Fabricate(:group).id.to_s
+        expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin")
+      end
+    end
+
+    context "when setting an old TrustLevelSetting" do
+      before do
+        deprecate_override!(
+          ["min_trust_level_to_allow_invite", "invite_allowed_groups", override, "0.0.1"],
+          ["min_trust_level_to_allow_invite"],
+        )
+      end
+
+      it "converts the provided trust level to the appropriate auto group" do
+        SiteSetting.min_trust_level_to_allow_invite = TrustLevel[4]
+        expect(SiteSetting.min_trust_level_to_allow_invite).to eq(TrustLevel[4])
+        expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:trust_level_4].to_s)
+      end
+
+      it "raises error with an invalid trust level" do
+        expect { SiteSetting.min_trust_level_to_allow_invite = 66 }.to raise_error(
+          Discourse::InvalidParameters,
+        )
+      end
+    end
+
+    context "when setting an old TrustLevelAndStaffSetting" do
+      before do
+        deprecate_override!(
+          [
+            "min_trust_level_to_allow_invite_tl_and_staff",
+            "invite_allowed_groups",
+            override,
+            "0.0.1",
+          ],
+          ["min_trust_level_to_allow_invite_tl_and_staff"],
+        )
+      end
+
+      it "converts the provided trust level to the appropriate auto group" do
+        SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = "admin"
+        expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("admin")
+        expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:admins].to_s)
+
+        SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = "staff"
+        expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq("staff")
+        expect(SiteSetting.invite_allowed_groups).to eq(Group::AUTO_GROUPS[:staff].to_s)
+
+        SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = TrustLevel[3]
+        expect(SiteSetting.min_trust_level_to_allow_invite_tl_and_staff).to eq(TrustLevel[3])
+        expect(SiteSetting.invite_allowed_groups).to eq(
+          "#{Group::AUTO_GROUPS[:admins]}|#{Group::AUTO_GROUPS[:staff]}|#{Group::AUTO_GROUPS[:trust_level_3]}",
+        )
+      end
+
+      it "raises error with an invalid trust level" do
+        expect { SiteSetting.min_trust_level_to_allow_invite_tl_and_staff = 66 }.to raise_error(
+          Discourse::InvalidParameters,
+        )
+      end
+    end
+  end
+end

spec/models/group_spec.rb

@@ -242,6 +242,31 @@ RSpec.describe Group do
     end
   end
 
+  describe ".auto_groups_between" do
+    it "returns the auto groups between lower and upper bounds" do
+      expect(
+        described_class.auto_groups_between(:trust_level_0, :trust_level_3),
+      ).to contain_exactly(10, 11, 12, 13)
+    end
+
+    it "excludes the undefined groups between staff and TL0" do
+      expect(described_class.auto_groups_between(:admins, :trust_level_0)).to contain_exactly(
+        1,
+        2,
+        3,
+        10,
+      )
+    end
+
+    it "returns an empty array when lower group is higher than upper group" do
+      expect(described_class.auto_groups_between(:trust_level_1, :trust_level_0)).to be_empty
+    end
+
+    it "returns an empty array when passing an unknown group" do
+      expect(described_class.auto_groups_between(:trust_level_0, :trust_level_1337)).to be_empty
+    end
+  end
+
   describe ".refresh_automatic_group!" do
     it "does not include staged users in any automatic groups" do
       staged = Fabricate(:staged, trust_level: 1)

spec/models/site_setting_spec.rb

@@ -144,43 +144,6 @@ RSpec.describe SiteSetting do
     end
   end
 
-  describe "deprecated site settings" do
-    before do
-      SiteSetting.force_https = true
-      @orig_logger = Rails.logger
-      Rails.logger = @fake_logger = FakeLogger.new
-    end
-
-    after { Rails.logger = @orig_logger }
-
-    it "should act as a proxy to the new methods" do
-      begin
-        original_settings = SiteSettings::DeprecatedSettings::SETTINGS.dup
-        SiteSettings::DeprecatedSettings::SETTINGS.clear
-
-        SiteSettings::DeprecatedSettings::SETTINGS.push(["use_https", "force_https", true, "0.0.1"])
-
-        SiteSetting.setup_deprecated_methods
-
-        expect do
-          expect(SiteSetting.use_https).to eq(true)
-          expect(SiteSetting.use_https?).to eq(true)
-        end.to change { @fake_logger.warnings.count }.by(2)
-
-        expect do SiteSetting.use_https(warn: false) end.to_not change { @fake_logger.warnings }
-
-        SiteSetting.use_https = false
-
-        expect(SiteSetting.force_https).to eq(false)
-        expect(SiteSetting.force_https?).to eq(false)
-      ensure
-        SiteSettings::DeprecatedSettings::SETTINGS.clear
-
-        SiteSettings::DeprecatedSettings::SETTINGS.concat(original_settings)
-      end
-    end
-  end
-
   describe "cached settings" do
     it "should recalculate cached setting when dependent settings are changed" do
       SiteSetting.blocked_attachment_filenames = "foo"