DEV: introduce new API to look up dynamic site setting

This removes all uses of both `send` and `public_send` from consumers of
SiteSetting and instead introduces a `get` helper for dynamic lookup

This leads to much cleaner and safer code long term as we are always explicit
to test that a site setting is really there before sending an arbitrary
string to the class

It also removes a couple of risky stubs from the auth provider test

app/controllers/static_controller.rb

@@ -27,7 +27,7 @@ class StaticController < ApplicationController
 
     if map.has_key?(@page)
       site_setting_key = map[@page][:redirect]
-      url = SiteSetting.send(site_setting_key)
+      url = SiteSetting.get(site_setting_key)
       return redirect_to(url) unless url.blank?
     end
 

app/jobs/onceoff/migrate_url_site_settings.rb

@@ -73,7 +73,7 @@ module Jobs
           for_site_setting: true
         ).create_for(Discourse.system_user.id)
 
-        SiteSetting.public_send("#{new_setting}=", upload)
+        SiteSetting.set(new_setting, upload)
       end
     end
 

app/jobs/regular/enable_bootstrap_mode.rb

@@ -10,8 +10,8 @@ module Jobs
       return unless user.is_singular_admin?
 
       # let's enable bootstrap mode settings
-      SiteSetting.set_and_log('default_trust_level', TrustLevel[1]) if SiteSetting.send('default_trust_level') == TrustLevel[0]
+      SiteSetting.set_and_log('default_trust_level', TrustLevel[1]) if SiteSetting.get('default_trust_level') == TrustLevel[0]
-      SiteSetting.set_and_log('default_email_digest_frequency', 1440) if SiteSetting.send('default_email_digest_frequency') == 10080
+      SiteSetting.set_and_log('default_email_digest_frequency', 1440) if SiteSetting.get('default_email_digest_frequency') == 10080
       SiteSetting.set_and_log('bootstrap_mode_enabled', true)
     end
   end

app/jobs/scheduled/clean_up_deprecated_url_site_settings.rb

@@ -5,7 +5,7 @@ module Jobs
     def execute(args)
       Jobs::MigrateUrlSiteSettings::SETTINGS.each do |old_setting, new_setting|
         if SiteSetting.where("name = ? AND value IS NOT NULL", new_setting).exists?
-          SiteSetting.public_send("#{old_setting}=", nil, warn: false)
+          SiteSetting.set(old_setting, nil, warn: false)
           SiteSetting.find_by(name: old_setting).destroy!
         end
       end

app/jobs/scheduled/disable_bootstrap_mode.rb

@@ -7,8 +7,8 @@ module Jobs
       total_users = User.human_users.count
 
       if SiteSetting.bootstrap_mode_min_users == 0 || total_users > SiteSetting.bootstrap_mode_min_users
-        SiteSetting.set_and_log('default_trust_level', TrustLevel[0]) if SiteSetting.send('default_trust_level') == TrustLevel[1]
+        SiteSetting.set_and_log('default_trust_level', TrustLevel[0]) if SiteSetting.get('default_trust_level') == TrustLevel[1]
-        SiteSetting.set_and_log('default_email_digest_frequency', 10080) if SiteSetting.send('default_email_digest_frequency') == 1440
+        SiteSetting.set_and_log('default_email_digest_frequency', 10080) if SiteSetting.get('default_email_digest_frequency') == 1440
         SiteSetting.set_and_log('bootstrap_mode_enabled', false)
       end
     end

app/models/category.rb

@@ -285,7 +285,7 @@ class Category < ActiveRecord::Base
   def remove_site_settings
     SiteSetting.all_settings.each do |s|
       if s[:type] == 'category' && s[:value].to_i == self.id
-        SiteSetting.send("#{s[:setting]}=", '')
+        SiteSetting.set(s[:setting], '')
       end
     end
 

app/models/embedding.rb

@@ -23,7 +23,7 @@ class Embedding < OpenStruct
 
   def save
     Embedding.settings.each do |s|
-      SiteSetting.send("#{s}=", send(s))
+      SiteSetting.set(s, public_send(s))
     end
     true
   rescue Discourse::InvalidParameters => p

app/models/post_action.rb

@@ -164,10 +164,10 @@ class PostAction < ActiveRecord::Base
 
     %w(like flag bookmark).each do |type|
       if send("is_#{type}?")
-        limit = SiteSetting.send("max_#{type}s_per_day")
+        limit = SiteSetting.get("max_#{type}s_per_day")
 
         if is_like? && user && user.trust_level >= 2
-          multiplier = SiteSetting.send("tl#{user.trust_level}_additional_likes_per_day_multiplier").to_f
+          multiplier = SiteSetting.get("tl#{user.trust_level}_additional_likes_per_day_multiplier").to_f
           multiplier = 1.0 if multiplier < 1.0
 
           limit = (limit * multiplier).to_i

app/models/topic.rb

@@ -1433,7 +1433,7 @@ class Topic < ActiveRecord::Base
   end
 
   def apply_per_day_rate_limit_for(key, method_name)
-    RateLimiter.new(user, "#{key}-per-day", SiteSetting.send(method_name), 1.day.to_i)
+    RateLimiter.new(user, "#{key}-per-day", SiteSetting.get(method_name), 1.day.to_i)
   end
 
   def create_invite_notification!(target_user, notification_type, username)

app/models/user.rb

@@ -1351,7 +1351,7 @@ class User < ActiveRecord::Base
     values = []
 
     %w{watching watching_first_post tracking muted}.each do |s|
-      category_ids = SiteSetting.send("default_categories_#{s}").split("|")
+      category_ids = SiteSetting.get("default_categories_#{s}").split("|")
       category_ids.each do |category_id|
         values << "(#{self.id}, #{category_id}, #{CategoryUser.notification_levels[s.to_sym]})"
       end

app/serializers/auth_provider_serializer.rb

@@ -5,17 +5,17 @@ class AuthProviderSerializer < ApplicationSerializer
              :icon
 
   def title_override
-    return SiteSetting.send(object.title_setting) if object.title_setting
+    return SiteSetting.get(object.title_setting) if object.title_setting
     object.title
   end
 
   def pretty_name_override
-    return SiteSetting.send(object.pretty_name_setting) if object.pretty_name_setting
+    return SiteSetting.get(object.pretty_name_setting) if object.pretty_name_setting
     object.pretty_name
   end
 
   def full_screen_login
-    return SiteSetting.send(object.full_screen_login_setting) if object.full_screen_login_setting
+    return SiteSetting.get(object.full_screen_login_setting) if object.full_screen_login_setting
     return object.full_screen_login if object.full_screen_login
     false
   end

lib/auth/open_id_authenticator.rb

@@ -10,7 +10,7 @@ class Auth::OpenIdAuthenticator < Auth::Authenticator
   end
 
   def enabled?
-    SiteSetting.send(@enabled_site_setting)
+    SiteSetting.get(@enabled_site_setting)
   end
 
   def description_for_user(user)

lib/plugin/instance.rb

@@ -81,7 +81,7 @@ class Plugin::Instance
   end
 
   def enabled?
-    @enabled_site_setting ? SiteSetting.send(@enabled_site_setting) : true
+    @enabled_site_setting ? SiteSetting.get(@enabled_site_setting) : true
   end
 
   delegate :name, to: :metadata
@@ -532,7 +532,7 @@ class Plugin::Instance
       rescue NotImplementedError
         provider.authenticator.define_singleton_method(:enabled?) do
           Discourse.deprecate("#{provider.authenticator.class.name} should define an `enabled?` function. Patching for now.")
-          return SiteSetting.send(provider.enabled_setting) if provider.enabled_setting
+          return SiteSetting.get(provider.enabled_setting) if provider.enabled_setting
           Discourse.deprecate("#{provider.authenticator.class.name} has not defined an enabled_setting. Defaulting to true.")
           true
         end

lib/rate_limiter/on_create_record.rb

@@ -14,9 +14,9 @@ class RateLimiter
 
       limit_key = "create_#{self.class.name.underscore}"
       max_setting = if user && user.new_user? && SiteSetting.has_setting?("rate_limit_new_user_#{limit_key}")
-        SiteSetting.send("rate_limit_new_user_#{limit_key}")
+        SiteSetting.get("rate_limit_new_user_#{limit_key}")
       else
-        SiteSetting.send("rate_limit_#{limit_key}")
+        SiteSetting.get("rate_limit_#{limit_key}")
       end
       @rate_limiter = RateLimiter.new(user, limit_key, 1, max_setting)
     end

lib/seed_data/categories.rb

@@ -95,7 +95,7 @@ module SeedData
 
     def create_category(site_setting_name:, name:, description:, position:, color:, text_color:,
                         permissions:, force_permissions:, force_existence: false)
-      category_id = SiteSetting.send(site_setting_name)
+      category_id = SiteSetting.get(site_setting_name)
 
       if should_create_category?(category_id, force_existence)
         category = Category.new(
@@ -111,7 +111,7 @@ module SeedData
         category.set_permissions(permissions)
         category.save!
 
-        SiteSetting.send("#{site_setting_name}=", category.id)
+        SiteSetting.set(site_setting_name, category.id)
       elsif category = Category.find_by(id: category_id)
         if description.present? && (category.topic_id.blank? || !Topic.exists?(category.topic_id))
           category.description = description
@@ -159,7 +159,7 @@ module SeedData
     end
 
     def find_category(site_setting_name)
-      category_id = SiteSetting.send(site_setting_name)
+      category_id = SiteSetting.get(site_setting_name)
       Category.find_by(id: category_id) if category_id > 0
     end
 

lib/seed_data/topics.rb

@@ -122,7 +122,7 @@ module SeedData
     end
 
     def create_topic(site_setting_name:, title:, raw:, category: nil, static_first_reply: false, after_create: nil)
-      topic_id = SiteSetting.send(site_setting_name)
+      topic_id = SiteSetting.get(site_setting_name)
       return if topic_id > 0 || Topic.find_by(id: topic_id)
 
       post = PostCreator.create!(
@@ -144,7 +144,7 @@ module SeedData
 
       after_create&.call(post)
 
-      SiteSetting.send("#{site_setting_name}=", post.topic_id)
+      SiteSetting.set(site_setting_name, post.topic_id)
     end
 
     def update_topic(site_setting_name:, title:, raw:, static_first_reply: false, skip_changed:)
@@ -163,7 +163,7 @@ module SeedData
     end
 
     def find_post(site_setting_name)
-      topic_id = SiteSetting.send(site_setting_name)
+      topic_id = SiteSetting.get(site_setting_name)
       Post.find_by(topic_id: topic_id, post_number: 1) if topic_id > 0
     end
 
@@ -172,7 +172,7 @@ module SeedData
     end
 
     def setting_value(site_setting_key)
-      SiteSetting.send(site_setting_key).presence || "<ins>#{site_setting_key}</ins>"
+      SiteSetting.get(site_setting_key).presence || "<ins>#{site_setting_key}</ins>"
     end
 
     def first_reply(post)

lib/site_icon_manager.rb

@@ -65,7 +65,7 @@ module SiteIconManager
 
   def self.resolve_original(info)
     info[:settings].each do |setting_name|
-      value = SiteSetting.send(setting_name)
+      value = SiteSetting.get(setting_name)
       return value if value
     end
     return Upload.find(SKETCH_LOGO_ID) if info[:fallback_to_sketch]

lib/site_setting_extension.rb

@@ -390,10 +390,14 @@ module SiteSettingExtension
     end
   end
 
-  def set(name, value)
+  def set(name, value, options = nil)
     if has_setting?(name)
       value = filter_value(name, value)
-      self.public_send("#{name}=", value)
+      if options
+        self.public_send("#{name}=", value, options)
+      else
+        self.public_send("#{name}=", value)
+      end
       Discourse.request_refresh! if requires_refresh?(name)
     else
       raise Discourse::InvalidParameters.new("Either no setting named '#{name}' exists or value provided is invalid")
@@ -407,7 +411,15 @@ module SiteSettingExtension
       value = prev_value = "[FILTERED]" if secret_settings.include?(name.to_sym)
       StaffActionLogger.new(user).log_site_setting_change(name, prev_value, value)
     else
-      raise Discourse::InvalidParameters
+      raise Discourse::InvalidParameters.new("No setting named '#{name}' exists")
+    end
+  end
+
+  def get(name)
+    if has_setting?(name)
+      self.public_send(name)
+    else
+      raise Discourse::InvalidParameters.new("No setting named '#{name}' exists")
     end
   end
 

lib/tasks/typepad.thor

@@ -12,7 +12,7 @@ class Typepad < Thor
 
     backup_settings = {}
     %w(email_domains_blacklist).each do |s|
-      backup_settings[s] = SiteSetting.send(s)
+      backup_settings[s] = SiteSetting.get(s)
     end
 
     user = User.where(username_lower: options[:post_as].downcase).first
@@ -26,7 +26,6 @@ class Typepad < Thor
       exit 1
     end
 
-    inside_block = true
     input = ""
 
     entries = []
@@ -103,7 +102,7 @@ class Typepad < Thor
   ensure
     RateLimiter.enable
     backup_settings.each do |s, v|
-      SiteSetting.send("#{s}=", v)
+      SiteSetting.set(s, v)
     end
   end
 

lib/validators/upload_validator.rb

@@ -123,7 +123,7 @@ class Validators::UploadValidator < ActiveModel::Validator
     max_size_kb = if upload.for_export
       SiteSetting.max_export_file_size_kb
     else
-      SiteSetting.send("max_#{type}_size_kb")
+      SiteSetting.get("max_#{type}_size_kb")
     end
     max_size_bytes = max_size_kb.kilobytes
 

lib/wizard/builder.rb

@@ -278,7 +278,7 @@ class Wizard
     protected
 
     def replace_setting_value(updater, raw, field_name)
-      old_value = SiteSetting.send(field_name)
+      old_value = SiteSetting.get(field_name)
       old_value = field_name if old_value.blank?
 
       new_value = updater.fields[field_name.to_sym]

lib/wizard/step_updater.rb

@@ -37,7 +37,7 @@ class Wizard
         value = Upload.get_from_url(value) || ''
       end
 
-      SiteSetting.set_and_log(id, value, @current_user) if SiteSetting.send(id) != value
+      SiteSetting.set_and_log(id, value, @current_user) if SiteSetting.get(id) != value
     end
 
     def apply_setting(id)

script/import_scripts/base.rb

@@ -95,7 +95,7 @@ class ImportScripts::Base
     @site_settings_during_import = get_site_settings_for_import
 
     @site_settings_during_import.each do |key, value|
-      @old_site_settings[key] = SiteSetting.send(key)
+      @old_site_settings[key] = SiteSetting.get(key)
       SiteSetting.set(key, value)
     end
 
@@ -108,7 +108,7 @@ class ImportScripts::Base
 
   def reset_site_settings
     @old_site_settings.each do |key, value|
-      current_value = SiteSetting.send(key)
+      current_value = SiteSetting.get(key)
       SiteSetting.set(key, value) unless current_value != @site_settings_during_import[key]
     end
 

spec/components/plugin/instance_spec.rb

@@ -130,10 +130,11 @@ describe Plugin::Instance do
   end
 
   it 'patches the enabled? function for auth_providers if not defined' do
-    SiteSetting.stubs(:ubuntu_login_enabled).returns(false)
-
     plugin = Plugin::Instance.new
 
+    # lets piggy back on another boolean setting, so we don't dirty our SiteSetting object
+    SiteSetting.enable_badges = false
+
     # No enabled_site_setting
     authenticator = Auth::Authenticator.new
     plugin.auth_provider(authenticator: authenticator)
@@ -143,26 +144,28 @@ describe Plugin::Instance do
     # With enabled site setting
     plugin = Plugin::Instance.new
     authenticator = Auth::Authenticator.new
-    plugin.auth_provider(enabled_setting: 'ubuntu_login_enabled', authenticator: authenticator)
+    plugin.auth_provider(enabled_setting: 'enable_badges', authenticator: authenticator)
     plugin.notify_before_auth
     expect(authenticator.enabled?).to eq(false)
 
     # Defines own method
     plugin = Plugin::Instance.new
-    SiteSetting.stubs(:ubuntu_login_enabled).returns(true)
+
+    SiteSetting.enable_badges = true
     authenticator = Class.new(Auth::Authenticator) do
       def enabled?
         false
       end
     end.new
-    plugin.auth_provider(enabled_setting: 'ubuntu_login_enabled', authenticator: authenticator)
+    plugin.auth_provider(enabled_setting: 'enable_badges', authenticator: authenticator)
     plugin.notify_before_auth
     expect(authenticator.enabled?).to eq(false)
   end
 
   context "activate!" do
     before do
-      SiteSetting.stubs(:ubuntu_login_enabled).returns(false)
+      # lets piggy back on another boolean setting, so we don't dirty our SiteSetting object
+      SiteSetting.enable_badges = false
     end
 
     it "can activate plugins correctly" do

spec/components/site_setting_extension_spec.rb

@@ -532,6 +532,23 @@ describe SiteSettingExtension do
     end
   end
 
+  describe ".get" do
+    before do
+      settings.setting(:title, "Discourse v1")
+      settings.refresh!
+    end
+
+    it "works correctly" do
+      expect {
+        settings.get("frogs_in_africa")
+      }.to raise_error(Discourse::InvalidParameters)
+
+      expect(settings.get(:title)).to eq("Discourse v1")
+      expect(settings.get("title")).to eq("Discourse v1")
+    end
+
+  end
+
   describe ".set_and_log" do
     before do
       settings.setting(:s3_secret_access_key, "old_secret_key", secret: true)

spec/fixtures/plugins/my_plugin/plugin.rb

@@ -6,7 +6,7 @@
 # authors: Frank Zappa
 
 auth_provider title: 'with Ubuntu',
-              authenticator: Auth::OpenIdAuthenticator.new('ubuntu', 'https://login.ubuntu.com', 'ubuntu_login_enabled', trusted: true),
+              authenticator: Auth::OpenIdAuthenticator.new('ubuntu', 'https://login.ubuntu.com', 'enable_badges', trusted: true),
               message: 'Authenticating with Ubuntu (make sure pop up blockers are not enbaled)',
               frame_width: 1000,   # the frame size used for the pop up window, overrides default
               frame_height: 800

spec/jobs/clean_up_deprecated_url_site_settings_spec.rb

@@ -16,8 +16,8 @@ RSpec.describe Jobs::CleanUpDeprecatedUrlSiteSettings do
   it 'should clean up the old deprecated site settings correctly' do
     logo_upload = Fabricate(:upload)
     SiteSetting.logo = logo_upload
-    SiteSetting.public_send("logo_url=", '/test/some/url', warn: false)
+    SiteSetting.set("logo_url", '/test/some/url', warn: false)
-    SiteSetting.public_send("logo_small_url=", '/test/another/url', warn: false)
+    SiteSetting.set("logo_small_url", '/test/another/url', warn: false)
 
     expect do
       described_class.new.execute({})

spec/jobs/migrate_url_site_settings_spec.rb

@@ -67,8 +67,8 @@ RSpec.describe Jobs::MigrateUrlSiteSettings do
       twitter_summary_large_image
       push_notifications_icon
     }.each do |setting|
-      expect(SiteSetting.public_send(setting)).to eq(upload)
+      expect(SiteSetting.get(setting)).to eq(upload)
-      expect(SiteSetting.public_send(setting).is_a?(Upload)).to eq(true)
+      expect(SiteSetting.get(setting).is_a?(Upload)).to eq(true)
     end
   end
 end

spec/lib/site_settings/validations_spec.rb

@@ -11,7 +11,7 @@ describe SiteSettings::Validations do
 
     shared_examples "s3 bucket validation" do
       def change_bucket_value(value)
-        SiteSetting.public_send("#{other_setting_name}=", value)
+        SiteSetting.set(other_setting_name, value)
       end
 
       it "shouldn't raise an error when both buckets are blank" do

spec/models/admin_dashboard_problem_spec.rb

@@ -133,37 +133,37 @@ describe AdminDashboardData do
     shared_examples 'problem detection for login providers' do
       context 'when disabled' do
         it 'returns nil' do
-          SiteSetting.public_send("#{enable_setting}=", false)
+          SiteSetting.set(enable_setting, false)
           expect(subject).to be_nil
         end
       end
 
       context 'when enabled' do
         before do
-          SiteSetting.public_send("#{enable_setting}=", true)
+          SiteSetting.set(enable_setting, true)
         end
 
         it 'returns nil when key and secret are set' do
-          SiteSetting.public_send("#{key}=", '12313213')
+          SiteSetting.set(key, '12313213')
-          SiteSetting.public_send("#{secret}=", '12312313123')
+          SiteSetting.set(secret, '12312313123')
           expect(subject).to be_nil
         end
 
         it 'returns a string when key is not set' do
-          SiteSetting.public_send("#{key}=", '')
+          SiteSetting.set(key, '')
-          SiteSetting.public_send("#{secret}=", '12312313123')
+          SiteSetting.set(secret, '12312313123')
           expect(subject).to_not be_nil
         end
 
         it 'returns a string when secret is not set' do
-          SiteSetting.public_send("#{key}=", '123123')
+          SiteSetting.set(key, '123123')
-          SiteSetting.public_send("#{secret}=", '')
+          SiteSetting.set(secret, '')
           expect(subject).to_not be_nil
         end
 
         it 'returns a string when key and secret are not set' do
-          SiteSetting.public_send("#{key}=", '')
+          SiteSetting.set(key, '')
-          SiteSetting.public_send("#{secret}=", '')
+          SiteSetting.set(secret, '')
           expect(subject).to_not be_nil
         end
       end
@@ -205,7 +205,7 @@ describe AdminDashboardData do
         ['a', ''].repeated_permutation(keys.size) do |*values|
           hash = Hash[keys.zip(values)]
           hash.each do |key, value|
-            SiteSetting.public_send("#{key}=", value)
+            SiteSetting.set(key, value)
           end
           yield hash
         end
@@ -213,9 +213,9 @@ describe AdminDashboardData do
 
       context 'when setting is enabled' do
         before do
-          all_setting_keys.each { |key| SiteSetting.public_send("#{key}=", 'foo') }
+          all_setting_keys.each { |key| SiteSetting.set(key, 'foo') }
-          SiteSetting.public_send("#{setting[:key]}=", setting[:enabled_value])
+          SiteSetting.set(setting[:key], setting[:enabled_value])
-          SiteSetting.public_send("#{bucket_key}=", bucket_value)
+          SiteSetting.set(bucket_key, bucket_value)
         end
 
         context 'when bucket is blank' do
@@ -262,7 +262,7 @@ describe AdminDashboardData do
 
       context 'when setting is not enabled' do
         before do
-          SiteSetting.public_send("#{setting[:key]}=", setting[:disabled_value])
+          SiteSetting.set(setting[:key], setting[:disabled_value])
         end
 
         it "always returns nil" do

spec/models/user_spec.rb

@@ -1222,7 +1222,6 @@ describe User do
     end
 
     it "returns custom color if restrict_letter_avatar_colors site setting is set" do
-      colors = SiteSetting.restrict_letter_avatar_colors.split("|")
       expect(User.letter_avatar_color("username_one")).to eq("2F70AC")
       expect(User.letter_avatar_color("username_two")).to eq("ED207B")
       expect(User.letter_avatar_color("username_three")).to eq("AAAAAA")

spec/requests/session_controller_spec.rb

@@ -267,11 +267,7 @@ RSpec.describe SessionController do
       SiteSetting.enable_sso = true
       SiteSetting.sso_secret = @sso_secret
 
-      # We have 2 options, either fabricate an admin or don't
-      # send welcome messages
       Fabricate(:admin)
-      # skip for now
-      # SiteSetting.send_welcome_message = false
     end
 
     let(:headers) { { host: Discourse.current_hostname } }

spec/requests/static_controller_spec.rb

@@ -151,7 +151,7 @@ describe StaticController do
 
         context "when #{setting_name} site setting is set" do
           before do
-            SiteSetting.public_send("#{setting_name}=", 'http://example.com/page')
+            SiteSetting.set(setting_name, 'http://example.com/page')
           end
 
           it "redirects to the #{setting_name}" do