DEV: Refactor Theme#settings to return a hash instead of array (#25516)

Why this change?

Returning an array makes it hard to immediately retrieve a setting by
name and makes the retrieval an O(N) operation. By returning an array,
we make it easier for us to lookup a setting by name and retrieval is
O(1) as well.

app/models/theme.rb

@@ -647,14 +647,15 @@ class Theme < ActiveRecord::Base
 
   def settings
     field = settings_field
-    return [] unless field && field.error.nil?
-    settings = []
+    settings = {}
 
-    ThemeSettingsParser
-      .new(field)
-      .load do |name, default, type, opts|
-        settings << ThemeSettingsManager.create(name, default, type, self, opts)
-      end
+    if field && field.error.nil?
+      ThemeSettingsParser
+        .new(field)
+        .load do |name, default, type, opts|
+          settings[name] = ThemeSettingsManager.create(name, default, type, self, opts)
+        end
+    end
 
     settings
   end
@@ -668,7 +669,7 @@ class Theme < ActiveRecord::Base
   def cached_default_settings
     Theme.get_set_cache "default_settings_for_theme_#{self.id}" do
       settings_hash = {}
-      self.settings.each { |setting| settings_hash[setting.name] = setting.default }
+      self.settings.each { |name, setting| settings_hash[name] = setting.default }
 
       theme_uploads = build_theme_uploads_hash
       settings_hash["theme_uploads"] = theme_uploads if theme_uploads.present?
@@ -682,7 +683,7 @@ class Theme < ActiveRecord::Base
 
   def build_settings_hash
     hash = {}
-    self.settings.each { |setting| hash[setting.name] = setting.value }
+    self.settings.each { |name, setting| hash[name] = setting.value }
 
     theme_uploads = build_theme_uploads_hash
     hash["theme_uploads"] = theme_uploads if theme_uploads.present?
@@ -724,13 +725,13 @@ class Theme < ActiveRecord::Base
   #   theme.get_setting(:some_string) => "hello"
   #
   def get_setting(setting_name)
-    target_setting = settings.find { |setting| setting.name == setting_name.to_sym }
+    target_setting = settings[setting_name.to_sym]
     raise Discourse::NotFound unless target_setting
     target_setting.value
   end
 
   def update_setting(setting_name, new_value)
-    target_setting = settings.find { |setting| setting.name == setting_name }
+    target_setting = settings[setting_name.to_sym]
     raise Discourse::NotFound unless target_setting
 
     target_setting.value = new_value

app/serializers/theme_serializer.rb

@@ -56,7 +56,7 @@ class ThemeSerializer < BasicThemeSerializer
   end
 
   def settings
-    object.settings.map { |setting| ThemeSettingsSerializer.new(setting, root: false) }
+    object.settings.map { |_name, setting| ThemeSettingsSerializer.new(setting, root: false) }
   rescue ThemeSettingsParser::InvalidYaml => e
     @errors << e.message
     nil

spec/lib/theme_settings_manager_spec.rb

@@ -4,6 +4,7 @@ require "theme_settings_manager"
 
 RSpec.describe ThemeSettingsManager do
   let!(:theme) { Fabricate(:theme) }
+
   let(:theme_settings) do
     yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/valid_settings.yaml")
     theme.set_field(target: :settings, name: "yaml", value: yaml)
@@ -11,20 +12,16 @@ RSpec.describe ThemeSettingsManager do
     theme.settings
   end
 
-  def find_by_name(name)
-    theme_settings.find { |setting| setting.name == name }
-  end
-
   describe "Enum" do
     it "only accepts values from its choices" do
-      enum_setting = find_by_name(:enum_setting)
+      enum_setting = theme_settings[:enum_setting]
       expect { enum_setting.value = "trust level 2" }.to raise_error(Discourse::InvalidParameters)
       expect { enum_setting.value = "trust level 0" }.not_to raise_error
 
-      enum_setting = find_by_name(:enum_setting_02)
+      enum_setting = theme_settings[:enum_setting_02]
       expect { enum_setting.value = "10" }.not_to raise_error
 
-      enum_setting = find_by_name(:enum_setting_03)
+      enum_setting = theme_settings[:enum_setting_03]
       expect { enum_setting.value = "10" }.not_to raise_error
       expect { enum_setting.value = 1 }.not_to raise_error
       expect { enum_setting.value = 15 }.to raise_error(Discourse::InvalidParameters)
@@ -33,7 +30,7 @@ RSpec.describe ThemeSettingsManager do
 
   describe "Bool" do
     it "is either true or false" do
-      bool_setting = find_by_name(:boolean_setting)
+      bool_setting = theme_settings[:boolean_setting]
       expect(bool_setting.value).to eq(true) # default
 
       bool_setting.value = "true"
@@ -52,7 +49,7 @@ RSpec.describe ThemeSettingsManager do
 
   describe "Integer" do
     it "is always an integer" do
-      int_setting = find_by_name(:integer_setting)
+      int_setting = theme_settings[:integer_setting]
       int_setting.value = 1.6
       theme.reload
       expect(int_setting.value).to eq(1)
@@ -71,7 +68,7 @@ RSpec.describe ThemeSettingsManager do
     end
 
     it "can have min or max value" do
-      int_setting = find_by_name(:integer_setting_02)
+      int_setting = theme_settings[:integer_setting_02]
       expect { int_setting.value = 0 }.to raise_error(Discourse::InvalidParameters)
       expect { int_setting.value = 61 }.to raise_error(Discourse::InvalidParameters)
 
@@ -87,7 +84,7 @@ RSpec.describe ThemeSettingsManager do
 
   describe "Float" do
     it "is always a float" do
-      float_setting = find_by_name(:float_setting)
+      float_setting = theme_settings[:float_setting]
       float_setting.value = 1.615
       theme.reload
       expect(float_setting.value).to eq(1.615)
@@ -102,7 +99,7 @@ RSpec.describe ThemeSettingsManager do
     end
 
     it "can have min or max value" do
-      float_setting = find_by_name(:float_setting)
+      float_setting = theme_settings[:float_setting]
       expect { float_setting.value = 1.4 }.to raise_error(Discourse::InvalidParameters)
       expect { float_setting.value = 10.01 }.to raise_error(Discourse::InvalidParameters)
       expect { float_setting.value = "text" }.to raise_error(Discourse::InvalidParameters)
@@ -115,7 +112,7 @@ RSpec.describe ThemeSettingsManager do
 
   describe "String" do
     it "can have min or max length" do
-      string_setting = find_by_name(:string_setting_02)
+      string_setting = theme_settings[:string_setting_02]
       expect { string_setting.value = "a" }.to raise_error(Discourse::InvalidParameters)
 
       string_setting.value = "ab"
@@ -130,20 +127,20 @@ RSpec.describe ThemeSettingsManager do
     end
 
     it "can be a textarea" do
-      expect(find_by_name(:string_setting_02).textarea).to eq(false)
-      expect(find_by_name(:string_setting_03).textarea).to eq(true)
+      expect(theme_settings[:string_setting_02].textarea).to eq(false)
+      expect(theme_settings[:string_setting_03].textarea).to eq(true)
     end
 
     it "supports json schema" do
-      expect(find_by_name(:string_setting_03).json_schema).to eq(false)
-      expect(find_by_name(:invalid_json_schema_setting).json_schema).to eq(false)
-      expect(find_by_name(:valid_json_schema_setting).json_schema).to be_truthy
+      expect(theme_settings[:string_setting_03].json_schema).to eq(false)
+      expect(theme_settings[:invalid_json_schema_setting].json_schema).to eq(false)
+      expect(theme_settings[:valid_json_schema_setting].json_schema).to be_truthy
     end
   end
 
   describe "List" do
     it "can have a list type" do
-      list_setting = find_by_name(:compact_list_setting)
+      list_setting = theme_settings[:compact_list_setting]
       expect(list_setting.list_type).to eq("compact")
     end
   end
@@ -152,7 +149,7 @@ RSpec.describe ThemeSettingsManager do
     let!(:upload) { Fabricate(:upload) }
 
     it "saves the upload id" do
-      upload_setting = find_by_name(:upload_setting)
+      upload_setting = theme_settings[:upload_setting]
       upload_setting.value = upload.url
       theme.reload
 
@@ -164,7 +161,7 @@ RSpec.describe ThemeSettingsManager do
     describe "#value" do
       context "when it's changed to a custom upload" do
         it "returns CDN URL" do
-          upload_setting = find_by_name(:upload_setting)
+          upload_setting = theme_settings[:upload_setting]
           upload_setting.value = upload.url
           theme.reload
 
@@ -181,7 +178,7 @@ RSpec.describe ThemeSettingsManager do
             upload_id: upload.id,
           )
           theme.save!
-          upload_setting = find_by_name(:upload_setting)
+          upload_setting = theme_settings[:upload_setting]
           expect(upload_setting.value).to eq(Discourse.store.cdn_url(upload.url))
         end
       end

spec/models/remote_theme_spec.rb

@@ -198,7 +198,7 @@ RSpec.describe RemoteTheme do
       expect(mapped.length).to eq(12)
 
       expect(theme.settings.length).to eq(1)
-      expect(theme.settings.first.value).to eq(true)
+      expect(theme.settings[:boolean_setting].value).to eq(true)
 
       expect(remote.remote_updated_at).to eq_time(time)
 
@@ -247,7 +247,7 @@ RSpec.describe RemoteTheme do
       expect(mapped["1-scss"]).to eq(scss_data)
 
       expect(theme.settings.length).to eq(1)
-      expect(theme.settings.first.value).to eq(32)
+      expect(theme.settings[:integer_setting].value).to eq(32)
 
       expect(remote.remote_updated_at).to eq_time(time)
       expect(remote.about_url).to eq("https://newsite.com/about")

spec/models/theme_spec.rb

@@ -328,7 +328,7 @@ HTML
       expect(scss).to include("background-color:red")
       expect(scss).to include("font-size:25px")
 
-      setting = theme.settings.find { |s| s.name == :font_size }
+      setting = theme.settings[:font_size]
       setting.value = "30px"
       theme.save!
 
@@ -418,7 +418,7 @@ HTML
       expect(theme_field.javascript_cache.content).to include("alert(settings.name)")
       expect(theme_field.javascript_cache.content).to include("let a = () => {}")
 
-      setting = theme.settings.find { |s| s.name == :name }
+      setting = theme.settings[:name]
       setting.value = "bill"
       theme.save!