From ba65aa3f6c1dcd94375fe9b22acccc51b74607d9 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Thu, 12 Jun 2014 18:03:03 -0400 Subject: [PATCH] Add a way to validate min and max value of an integer site setting --- config/locales/server.en.yml | 4 + config/site_settings.yml | 47 ++++++++-- lib/site_setting_extension.rb | 20 ++++- lib/validators/email_setting_validator.rb | 8 +- lib/validators/integer_setting_validator.rb | 24 +++++ lib/validators/username_setting_validator.rb | 8 +- .../components/site_setting_extension_spec.rb | 6 +- .../email_setting_validator_spec.rb | 10 ++- .../integer_setting_validator_spec.rb | 89 +++++++++++++++++++ .../username_setting_validator_spec.rb | 10 ++- 10 files changed, 198 insertions(+), 28 deletions(-) create mode 100644 lib/validators/integer_setting_validator.rb create mode 100644 spec/components/validators/integer_setting_validator_spec.rb diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 4bc81ea55..e7681e974 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -916,6 +916,10 @@ en: errors: invalid_email: "Invalid email address." invalid_username: "There's no user with that username." + invalid_integer_min_max: "Value must be between %{min} and %{max}." + invalid_integer_min: "Value must be %{min} or greater." + invalid_integer_max: "Value cannot be higher than %{max}." + invalid_integer: "Value must be an integer." notification_types: mentioned: "%{display_username} mentioned you in %{link}" diff --git a/config/site_settings.yml b/config/site_settings.yml index c0bd8fa39..1f99b3786 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1,3 +1,8 @@ +# Possible types: +# +# email - Must be a valid email address +# username - Must match the username of an existing user +# integer - Must be an integer. Can also provide max and min options to limit the range of values. required: title: client: true @@ -5,13 +10,13 @@ required: site_description: '' contact_email: default: '' - validator: 'EmailSettingValidator' + type: email notification_email: default: 'info@discourse.org' - validator: 'EmailSettingValidator' + type: email site_contact_username: default: '' - validator: 'UsernameSettingValidator' + type: username logo_url: client: true default: '/images/d-logo-sketch.png' @@ -40,6 +45,8 @@ basic: suggested_topics: client: true default: 5 + type: integer + min: 0 limit_suggested_to_category: client: false default: false @@ -118,18 +125,33 @@ basic: relative_date_duration: client: true default: 30 - topics_per_period_in_top_summary: 20 - topics_per_period_in_top_page: 50 + type: integer + min: 0 + topics_per_period_in_top_summary: + default: 20 + type: integer + min: 1 + topics_per_period_in_top_page: + default: 50 + type: integer + min: 1 category_featured_topics: client: true default: 3 + type: integer + min: 1 fixed_category_positions: client: true default: false - topics_per_page: 30 + topics_per_page: + default: 30 + type: integer + min: 10 posts_per_page: client: true default: 20 + type: integer + min: 10 enable_badges: client: true default: false @@ -150,12 +172,19 @@ users: min_username_length: client: true default: 3 + type: integer + min: 1 max_username_length: client: true default: 20 + type: integer + min: 8 + max: 60 min_password_length: client: true default: 8 + type: integer + min: 1 block_common_passwords: true # The default value of enable_google_logins changed from true to false. @@ -320,14 +349,14 @@ email: pop3s_polling_port: 995 pop3s_polling_username: default: '' - validator: 'UsernameSettingValidator' + type: username pop3s_polling_password: '' email_in: default: false client: true email_in_address: default: '' - validator: 'EmailSettingValidator' + type: email email_in_min_trust: default: 3 enum: 'MinTrustToCreateTopicSetting' @@ -490,7 +519,7 @@ embedding: feed_polling_url: '' embed_by_username: default: '' - validator: 'UsernameSettingValidator' + type: username embed_category: '' embed_post_limit: 100 embed_truncate: false diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index fd619bd0a..205d4e130 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -82,8 +82,8 @@ module SiteSettingExtension if opts[:refresh] refresh_settings << name end - if v = opts[:validator] - validators[name] = v.is_a?(String) ? v.constantize : v + if validator_type = opts[:type] + validators[name] = {class: validator_for(validator_type), opts: opts} end current[name] = current_value @@ -239,8 +239,11 @@ module SiteSettingExtension raise Discourse::InvalidParameters.new(:value) unless enum_class(name).valid_value?(val) end - if v = validators[name] and !v.valid_value?(val) - raise Discourse::InvalidParameters.new(v.error_message(val)) + if v = validators[name] + validator = v[:class].new(v[:opts]) + unless validator.valid_value?(val) + raise Discourse::InvalidParameters.new(validator.error_message(val)) + end end provider.save(name, val, type) @@ -322,6 +325,15 @@ module SiteSettingExtension end end + def validator_for(type_name) + @validator_mapping ||= { + 'email' => EmailSettingValidator, + 'username' => UsernameSettingValidator, + 'integer' => IntegerSettingValidator + } + @validator_mapping[type_name] + end + def setup_methods(name, current_value) diff --git a/lib/validators/email_setting_validator.rb b/lib/validators/email_setting_validator.rb index f499fb225..d35f7279f 100644 --- a/lib/validators/email_setting_validator.rb +++ b/lib/validators/email_setting_validator.rb @@ -1,9 +1,13 @@ class EmailSettingValidator - def self.valid_value?(val) + def initialize(opts={}) + @opts = opts + end + + def valid_value?(val) !val.present? || !!(EmailValidator.email_regex =~ val) end - def self.error_message(val) + def error_message(val) I18n.t('site_settings.errors.invalid_email') end end diff --git a/lib/validators/integer_setting_validator.rb b/lib/validators/integer_setting_validator.rb new file mode 100644 index 000000000..f3259246c --- /dev/null +++ b/lib/validators/integer_setting_validator.rb @@ -0,0 +1,24 @@ +class IntegerSettingValidator + def initialize(opts={}) + @opts = opts + end + + def valid_value?(val) + return false if val.to_i.to_s != val.to_s + return false if @opts[:min] and @opts[:min].to_i > val.to_i + return false if @opts[:max] and @opts[:max].to_i < val.to_i + true + end + + def error_message(val) + if @opts[:min] && @opts[:max] + I18n.t('site_settings.errors.invalid_integer_min_max', {min: @opts[:min], max: @opts[:max]}) + elsif @opts[:min] + I18n.t('site_settings.errors.invalid_integer_min', {min: @opts[:min]}) + elsif @opts[:max] + I18n.t('site_settings.errors.invalid_integer_max', {max: @opts[:max]}) + else + I18n.t('site_settings.errors.invalid_integer') + end + end +end diff --git a/lib/validators/username_setting_validator.rb b/lib/validators/username_setting_validator.rb index ce05cae5c..67509ebbb 100644 --- a/lib/validators/username_setting_validator.rb +++ b/lib/validators/username_setting_validator.rb @@ -1,9 +1,13 @@ class UsernameSettingValidator - def self.valid_value?(val) + def initialize(opts={}) + @opts = opts + end + + def valid_value?(val) !val.present? || User.where(username: val).exists? end - def self.error_message(val) + def error_message(val) I18n.t('site_settings.errors.invalid_username') end end diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb index 2a4517030..44b99e948 100644 --- a/spec/components/site_setting_extension_spec.rb +++ b/spec/components/site_setting_extension_spec.rb @@ -284,7 +284,7 @@ describe SiteSettingExtension do describe "setting with a validator" do before do - settings.setting(:validated_setting, "info@example.com", {validator: 'EmailSettingValidator'}) + settings.setting(:validated_setting, "info@example.com", {type: 'email'}) settings.refresh! end @@ -293,14 +293,14 @@ describe SiteSettingExtension do end it "stores valid values" do - EmailSettingValidator.expects(:valid_value?).returns(true) + EmailSettingValidator.any_instance.expects(:valid_value?).returns(true) settings.validated_setting = 'success@example.com' settings.validated_setting.should == 'success@example.com' end it "rejects invalid values" do expect { - EmailSettingValidator.expects(:valid_value?).returns(false) + EmailSettingValidator.any_instance.expects(:valid_value?).returns(false) settings.validated_setting = 'nope' }.to raise_error(Discourse::InvalidParameters) settings.validated_setting.should == "info@example.com" diff --git a/spec/components/validators/email_setting_validator_spec.rb b/spec/components/validators/email_setting_validator_spec.rb index c7ba6e5dc..8c5f800b9 100644 --- a/spec/components/validators/email_setting_validator_spec.rb +++ b/spec/components/validators/email_setting_validator_spec.rb @@ -2,17 +2,19 @@ require 'spec_helper' describe EmailSettingValidator do describe '#valid_value?' do + subject(:validator) { described_class.new } + it "returns true for blank values" do - described_class.valid_value?('').should == true - described_class.valid_value?(nil).should == true + validator.valid_value?('').should == true + validator.valid_value?(nil).should == true end it "returns true if value is a valid email address" do - described_class.valid_value?('vader@example.com').should == true + validator.valid_value?('vader@example.com').should == true end it "returns false if value is not a valid email address" do - described_class.valid_value?('my house').should == false + validator.valid_value?('my house').should == false end end end diff --git a/spec/components/validators/integer_setting_validator_spec.rb b/spec/components/validators/integer_setting_validator_spec.rb new file mode 100644 index 000000000..7b0eca1e3 --- /dev/null +++ b/spec/components/validators/integer_setting_validator_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +describe IntegerSettingValidator do + describe '#valid_value?' do + + shared_examples "for all IntegerSettingValidator opts" do + it "returns false for blank values" do + validator.valid_value?('').should == false + validator.valid_value?(nil).should == false + end + + it "returns false if value is not a valid integer" do + validator.valid_value?('two').should == false + end + end + + context "without min and max" do + subject(:validator) { described_class.new } + + include_examples "for all IntegerSettingValidator opts" + + it "returns true if value is a valid integer" do + validator.valid_value?(1).should == true + validator.valid_value?(-1).should == true + validator.valid_value?('1').should == true + validator.valid_value?('-1').should == true + end + end + + context "with min" do + subject(:validator) { described_class.new(min: 2) } + + include_examples "for all IntegerSettingValidator opts" + + it "returns true if value is equal to min" do + validator.valid_value?(2).should == true + validator.valid_value?('2').should == true + end + + it "returns true if value is greater than min" do + validator.valid_value?(3).should == true + validator.valid_value?('3').should == true + end + + it "returns false if value is less than min" do + validator.valid_value?(1).should == false + validator.valid_value?('1').should == false + end + end + + context "with max" do + subject(:validator) { described_class.new(max: 3) } + + include_examples "for all IntegerSettingValidator opts" + + it "returns true if value is equal to max" do + validator.valid_value?(3).should == true + validator.valid_value?('3').should == true + end + + it "returns true if value is less than max" do + validator.valid_value?(2).should == true + validator.valid_value?('2').should == true + end + + it "returns false if value is greater than min" do + validator.valid_value?(4).should == false + validator.valid_value?('4').should == false + end + end + + context "with min and max" do + subject(:validator) { described_class.new(min: -1, max: 3) } + + include_examples "for all IntegerSettingValidator opts" + + it "returns true if value is in range" do + validator.valid_value?(-1).should == true + validator.valid_value?(0).should == true + validator.valid_value?(3).should == true + end + + it "returns false if value is out of range" do + validator.valid_value?(4).should == false + validator.valid_value?(-2).should == false + end + end + end +end diff --git a/spec/components/validators/username_setting_validator_spec.rb b/spec/components/validators/username_setting_validator_spec.rb index 52da3c9a5..6bffe2ec5 100644 --- a/spec/components/validators/username_setting_validator_spec.rb +++ b/spec/components/validators/username_setting_validator_spec.rb @@ -2,18 +2,20 @@ require 'spec_helper' describe UsernameSettingValidator do describe '#valid_value?' do + subject(:validator) { described_class.new } + it "returns true for blank values" do - described_class.valid_value?('').should == true - described_class.valid_value?(nil).should == true + validator.valid_value?('').should == true + validator.valid_value?(nil).should == true end it "returns true if value matches an existing user's username" do Fabricate(:user, username: 'vader') - described_class.valid_value?('vader').should == true + validator.valid_value?('vader').should == true end it "returns false if value does not match a user's username" do - described_class.valid_value?('no way').should == false + validator.valid_value?('no way').should == false end end end