From 6ca2396b120a049afcc20b88e59d4232c7a10191 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Wed, 21 Feb 2024 08:08:26 +0800 Subject: [PATCH] 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 | 20 ++++--- app/models/theme_setting.rb | 26 -------- config/locales/server.en.yml | 18 +++--- lib/theme_settings_manager.rb | 26 ++------ lib/theme_settings_manager/enum.rb | 4 -- lib/theme_settings_manager/float.rb | 4 -- lib/theme_settings_manager/integer.rb | 4 -- lib/theme_settings_manager/string.rb | 4 -- lib/theme_settings_validator.rb | 85 +++++++++++++++++++++++++++ spec/models/theme_field_spec.rb | 18 +++++- 10 files changed, 126 insertions(+), 83 deletions(-) create mode 100644 lib/theme_settings_validator.rb diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 61b164659c1..38dc09d28f6 100644 --- a/app/models/theme_field.rb +++ b/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? - - if (min = opts[:min]) && (max = opts[:max]) - unless ThemeSetting.value_in_range?(default, (min..max), type) - errors << I18n.t("#{translation_key}.default_out_range", name: name) - end + unless ThemeSettingsValidator.is_value_present?(default) + errors << I18n.t("#{translation_key}.default_value_missing", name: name) + next 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 diff --git a/app/models/theme_setting.rb b/app/models/theme_setting.rb index 52588ee7e62..d6698746711 100644 --- a/app/models/theme_setting.rb +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 54216cc196d..0eb40fc5f3c 100644 --- a/config/locales/server.en.yml +++ b/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: "It must be between %{min} and %{max}." - number_value_not_valid_min: "It must be larger than or equal to %{min}." - number_value_not_valid_max: "It must be smaller than or equal to %{max}." - string_value_not_valid: "New value length isn't within the allowed range." - string_value_not_valid_min_max: "It must be between %{min} and %{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." + number_value_not_valid_min_max: "Value must be between %{min} and %{max}." + number_value_not_valid_min: "Value must be larger than or equal to %{min}." + number_value_not_valid_max: "Value 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_min: "Value must be at least %{min} characters long." + string_value_not_valid_max: "Value must be at most %{max} characters long." objects: required: "must be present" invalid_type: "%{type} is not a valid type" diff --git a/lib/theme_settings_manager.rb b/lib/theme_settings_manager.rb index 749e6ebdef6..3ee7761d03e 100644 --- a/lib/theme_settings_manager.rb +++ b/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) - raise Discourse::InvalidParameters.new invalid_value_error_message - end + return if new_value.nil? + + error_messages = ThemeSettingsValidator.validate_value(new_value, type, @opts) + raise Discourse::InvalidParameters.new error_messages.join(" ") if error_messages.present? end def has_min? diff --git a/lib/theme_settings_manager/enum.rb b/lib/theme_settings_manager/enum.rb index 4dffe3d3705..d48ffc7c20c 100644 --- a/lib/theme_settings_manager/enum.rb +++ b/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 diff --git a/lib/theme_settings_manager/float.rb b/lib/theme_settings_manager/float.rb index d33bc71de2e..0fa230b6c01 100644 --- a/lib/theme_settings_manager/float.rb +++ b/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 diff --git a/lib/theme_settings_manager/integer.rb b/lib/theme_settings_manager/integer.rb index 615b93f63ae..fafa62d3063 100644 --- a/lib/theme_settings_manager/integer.rb +++ b/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 diff --git a/lib/theme_settings_manager/string.rb b/lib/theme_settings_manager/string.rb index 14928c83d0d..57650c8b283 100644 --- a/lib/theme_settings_manager/string.rb +++ b/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 diff --git a/lib/theme_settings_validator.rb b/lib/theme_settings_validator.rb new file mode 100644 index 00000000000..19ad9d7f758 --- /dev/null +++ b/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 diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 3c153aa5ba2..e9ec45c544d 100644 --- a/spec/models/theme_field_spec.rb +++ b/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