Add a way to validate min and max value of an integer site setting

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}"

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
+    type: integer
-  topics_per_period_in_top_page: 50
+    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

lib/site_setting_extension.rb

@@ -82,8 +82,8 @@ module SiteSettingExtension
       if opts[:refresh]
         refresh_settings << name
       end
-      if v = opts[:validator]
+      if validator_type = opts[:type]
-        validators[name] = v.is_a?(String) ? v.constantize : v
+        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)
+    if v = validators[name]
-      raise Discourse::InvalidParameters.new(v.error_message(val))
+      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)
 

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

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

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

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"

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
+      validator.valid_value?('').should == true
-      described_class.valid_value?(nil).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

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

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
+      validator.valid_value?('').should == true
-      described_class.valid_value?(nil).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