DEV: Centralise logic for validating a theme setting value (#25764)

Why this change?

The logic for validating a theme setting's value and default value was
not consistent as each part of the code would implement its own logic.
This is not ideal as the default value may be validated differently than
when we are setting a new value. Therefore, this commit seeks to
refactor all the validation logic for a theme setting's value into a
single service class.

What does this change do?

Introduce the `ThemeSettingsValidator` service class which holds all the
necessary helper methods required to validate a theme setting's value

app/models/theme_field.rb

@@ -310,6 +310,7 @@ class ThemeField < ActiveRecord::Base
     return unless self.name == "yaml"
 
     errors = []
+
     begin
       ThemeSettingsParser
         .new(self)
@@ -325,17 +326,22 @@ class ThemeField < ActiveRecord::Base
             end
           end
 
-          errors << I18n.t("#{translation_key}.default_value_missing", name: name) if default.nil?
+          unless ThemeSettingsValidator.is_value_present?(default)
-
+            errors << I18n.t("#{translation_key}.default_value_missing", name: name)
-          if (min = opts[:min]) && (max = opts[:max])
+            next
-            unless ThemeSetting.value_in_range?(default, (min..max), type)
-              errors << I18n.t("#{translation_key}.default_out_range", name: name)
-            end
           end
 
-          unless ThemeSetting.acceptable_value_for_type?(default, type)
+          unless ThemeSettingsValidator.is_valid_value_type?(default, type)
             errors << I18n.t("#{translation_key}.default_not_match_type", name: name)
           end
+
+          if (setting_errors = ThemeSettingsValidator.validate_value(default, type, opts)).present?
+            errors << I18n.t(
+              "#{translation_key}.default_value_not_valid",
+              name: name,
+              error_messages: setting_errors.join(" "),
+            )
+          end
         end
     rescue ThemeSettingsParser::InvalidYaml => e
       errors << e.message

app/models/theme_setting.rb

@@ -31,32 +31,6 @@ class ThemeSetting < ActiveRecord::Base
     TYPES_ENUM
   end
 
-  def self.acceptable_value_for_type?(value, type)
-    case type
-    when self.types[:integer]
-      value.is_a?(Integer)
-    when self.types[:float]
-      value.is_a?(Integer) || value.is_a?(Float)
-    when self.types[:bool]
-      value.is_a?(TrueClass) || value.is_a?(FalseClass)
-    when self.types[:list]
-      value.is_a?(String)
-    when self.types[:objects]
-      # TODO: This is a simple check now but we want to validate the default objects agianst the schema as well.
-      value.is_a?(Array)
-    else
-      true
-    end
-  end
-
-  def self.value_in_range?(value, range, type)
-    if type == self.types[:integer] || type == self.types[:float]
-      range.include? value
-    elsif type == self.types[:string]
-      range.include? value.to_s.length
-    end
-  end
-
   def self.guess_type(value)
     case value
     when Integer

config/locales/server.en.yml

@@ -122,18 +122,16 @@ en:
       invalid_yaml: "Provided YAML is invalid."
       data_type_inclusion: "Setting `%{name}` type is unsupported. Supported types are `integer`, `bool`, `list`, `enum` and `upload`"
       name_too_long: "There is a setting with a too long name. Maximum length is 255"
-      default_value_missing: "Setting `%{name}` has no default value"
+      default_value_missing: "Setting `%{name}` has no default value."
       default_not_match_type: "Setting `%{name}` default value's type doesn't match with the setting type."
-      default_out_range: "Setting `%{name}` default value isn't in the specified range."
+      default_value_not_valid: "Setting `%{name}` default value isn't valid. %{error_messages}"
       enum_value_not_valid: "Selected value isn't one of the enum choices."
-      number_value_not_valid: "New value isn't within the allowed range."
+      number_value_not_valid_min_max: "Value must be between %{min} and %{max}."
-      number_value_not_valid_min_max: "It must be between %{min} and %{max}."
+      number_value_not_valid_min: "Value must be larger than or equal to %{min}."
-      number_value_not_valid_min: "It must be larger than or equal to %{min}."
+      number_value_not_valid_max: "Value must be smaller than or equal to %{max}."
-      number_value_not_valid_max: "It must be smaller than or equal to %{max}."
+      string_value_not_valid_min_max: "Value must be between %{min} and %{max} characters long."
-      string_value_not_valid: "New value length isn't within the allowed range."
+      string_value_not_valid_min: "Value must be at least %{min} characters long."
-      string_value_not_valid_min_max: "It must be between %{min} and %{max} characters long."
+      string_value_not_valid_max: "Value must be at most %{max} characters long."
-      string_value_not_valid_min: "It must be at least %{min} characters long."
-      string_value_not_valid_max: "It must be at most %{max} characters long."
       objects:
         required: "must be present"
         invalid_type: "%{type} is not a valid type"

lib/theme_settings_manager.rb

@@ -78,29 +78,11 @@ class ThemeSettingsManager
     record
   end
 
-  def is_valid_value?(new_value)
-    true
-  end
-
-  def invalid_value_error_message
-    name = type == @types[:integer] || type == @types[:float] ? "number" : type_name
-    primary_key = "themes.settings_errors.#{name}_value_not_valid"
-
-    secondary_key = primary_key
-    secondary_key += "_min" if has_min?
-    secondary_key += "_max" if has_max?
-
-    translation = I18n.t(primary_key)
-    return translation if secondary_key == primary_key
-
-    translation += " #{I18n.t(secondary_key, min: @opts[:min], max: @opts[:max])}"
-    translation
-  end
-
   def ensure_is_valid_value!(new_value)
-    unless is_valid_value?(new_value)
+    return if new_value.nil?
-      raise Discourse::InvalidParameters.new invalid_value_error_message
+
-    end
+    error_messages = ThemeSettingsValidator.validate_value(new_value, type, @opts)
+    raise Discourse::InvalidParameters.new error_messages.join(" ") if error_messages.present?
   end
 
   def has_min?

lib/theme_settings_manager/enum.rb

@@ -7,10 +7,6 @@ class ThemeSettingsManager::Enum < ThemeSettingsManager
     match || val
   end
 
-  def is_valid_value?(new_value)
-    choices.include?(new_value) || choices.map(&:to_s).include?(new_value)
-  end
-
   def choices
     @opts[:choices]
   end

lib/theme_settings_manager/float.rb

@@ -12,8 +12,4 @@ class ThemeSettingsManager::Float < ThemeSettingsManager
   def value=(new_value)
     super(self.class.cast(new_value))
   end
-
-  def is_valid_value?(new_value)
-    (@opts[:min]..@opts[:max]).include? new_value.to_f
-  end
 end

lib/theme_settings_manager/integer.rb

@@ -12,8 +12,4 @@ class ThemeSettingsManager::Integer < ThemeSettingsManager
   def value=(new_value)
     super(self.class.cast(new_value))
   end
-
-  def is_valid_value?(new_value)
-    (@opts[:min]..@opts[:max]).include? new_value.to_i
-  end
 end

lib/theme_settings_manager/string.rb

@@ -1,10 +1,6 @@
 # frozen_string_literal: true
 
 class ThemeSettingsManager::String < ThemeSettingsManager
-  def is_valid_value?(new_value)
-    (@opts[:min]..@opts[:max]).include? new_value.to_s.length
-  end
-
   def textarea
     @opts[:textarea]
   end

lib/theme_settings_validator.rb

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+# Service class that holds helper methods that can be used to validate theme settings.
+class ThemeSettingsValidator
+  class << self
+    def is_value_present?(value)
+      !value.nil?
+    end
+
+    def is_valid_value_type?(value, type)
+      case type
+      when self.types[:integer]
+        value.is_a?(Integer)
+      when self.types[:float]
+        value.is_a?(Integer) || value.is_a?(Float)
+      when self.types[:bool]
+        value.is_a?(TrueClass) || value.is_a?(FalseClass)
+      when self.types[:list]
+        value.is_a?(String)
+      when self.types[:objects]
+        value.is_a?(Array) && value.all? { |v| v.is_a?(Hash) }
+      else
+        true
+      end
+    end
+
+    def validate_value(value, type, opts)
+      errors = []
+
+      case type
+      when types[:enum]
+        unless opts[:choices].include?(value) || opts[:choices].map(&:to_s).include?(value)
+          errors << I18n.t(
+            "themes.settings_errors.enum_value_not_valid",
+            choices: opts[:choices].join(", "),
+          )
+        end
+      when types[:integer], types[:float]
+        validate_value_in_range!(
+          value,
+          min: opts[:min],
+          max: opts[:max],
+          errors:,
+          translation_prefix: "number",
+        )
+      when types[:string]
+        validate_value_in_range!(
+          value.length,
+          min: opts[:min],
+          max: opts[:max],
+          errors:,
+          translation_prefix: "string",
+        )
+      end
+
+      errors
+    end
+
+    private
+
+    def types
+      ThemeSetting.types
+    end
+
+    def validate_value_in_range!(value, min:, max:, errors:, translation_prefix:)
+      if min && max && max != Float::INFINITY && !(min..max).include?(value)
+        errors << I18n.t(
+          "themes.settings_errors.#{translation_prefix}_value_not_valid_min_max",
+          min: min,
+          max: max,
+        )
+      elsif min && value < min
+        errors << I18n.t(
+          "themes.settings_errors.#{translation_prefix}_value_not_valid_min",
+          min: min,
+        )
+      elsif max && value > max
+        errors << I18n.t(
+          "themes.settings_errors.#{translation_prefix}_value_not_valid_max",
+          max: max,
+        )
+      end
+    end
+  end
+end

spec/models/theme_field_spec.rb

@@ -392,9 +392,23 @@ HTML
 
   it "generates errors when default value is not within allowed range" do
     field = create_yaml_field(get_fixture("invalid"))
-    expect(field.error).to include(I18n.t("#{key}.default_out_range", name: "default_out_of_range"))
+
     expect(field.error).to include(
-      I18n.t("#{key}.default_out_range", name: "string_default_out_of_range"),
+      I18n.t(
+        "#{key}.default_value_not_valid",
+        name: "default_out_of_range",
+        error_messages: [I18n.t("#{key}.number_value_not_valid_min_max", min: 1, max: 20)].join(
+          " ",
+        ),
+      ),
+    )
+
+    expect(field.error).to include(
+      I18n.t(
+        "#{key}.default_value_not_valid",
+        name: "string_default_out_of_range",
+        error_messages: [I18n.t("#{key}.string_value_not_valid_min", min: 20)].join(" "),
+      ),
     )
   end