huacnlee / rails-settings-cached

Global settings for your Rails application.
Other
1.06k stars 202 forks source link

Add validates options to special the Rails Validation for fields #201

Closed huacnlee closed 3 years ago

huacnlee commented 3 years ago

Close #153

Add validates options to special the Rails Validation for fields.

class Setting < RailsSettings::Base
  # cache_prefix { "v1" }
  field :app_name, default: "Rails Settings", validates: { presence: true, length: { in: 2..20 } }
  field :default_locale, default: "zh-CN", validates: { presence: true, inclusion: { in: %w[zh-CN en jp], message: "is not included in [zh-CN, en, jp]" } }
end

Now validate will work on record save.

setting = Setting.find_or_initialize_by(var: :app_name)
setting.value = ""
setting.valid?
# => false
setting.errors.full_messages
# => ["App name can't be blank", "App name too short (minimum is 2 characters)"]

setting = Setting.find_or_initialize_by(var: :default_locale)
setting.value = "zh-TW"
setting.save
# => false
setting.errors.full_messages
# => ["Default locale is not included in [zh-CN, en, jp]"]
setting.value = "en"
setting.valid?
# => true
Ex-Ark commented 3 years ago

Thanks for the feature, I'm glad to see this implemented in the gem directly 👍

I have a strange behaviour with this, tell me what you think please

The validations works when using setting.value = "something" and then setting.valid? but not when using Setting.some_setting = "something".

The readme says the gem basic usage is like this :

irb > Setting.user_limits
20
irb > Setting.user_limits = "30"
irb > Setting.user_limits
30
irb > Setting.user_limits = 45
irb > Setting.user_limits
45

but if we want validation are we forced to use Setting.find_by ... and valid? ? This feels misleading and prone to errors i.m.o

Here is a proving example (I cloned the gem locally and edited the minitest cases) :

# rails-settings-cached/test/models/setting.rb

class Setting < RailsSettings::Base
  cache_prefix { "v1" }
  field :api_quota, type: :integer, default: 1000, validates: { numericality: {only_integer: true, greater_than_or_equal_to: 1} }
end

# rails-settings-cached/test/validation_test.rb

require "test_helper"

class ValidationTest < ActiveSupport::TestCase
  test "validation" do
        # below tests work
    setting = Setting.find_or_initialize_by(var: "api_quota")
    assert_equal false, setting.valid? # OK
    assert_errors_on setting, :api_quota, ["Api quota is not a number"] # OK
    setting.value = -1000
    assert_equal false, setting.valid? # OK
    assert_errors_on setting, :api_quota, ["Api quota must be greater than or equal to 1"] # OK

    Setting.api_quota = 1000
    assert_equal 1000, Setting.api_quota # OK
    Setting.api_quota = -1000
        # below test doesn't work !
    assert_not_equal -1000, Setting.api_quota # KO
  end
end
bundle exec rake test
Failure:
Expected -1000 to not be equal to -1000

So by doing Setting.api_quota = -1000 it is bypassing validations

Is it intended behaviour ? If so can you add some lines in the documentation about that ?

On another note, I observed than doing Setting.api_quota = "helloworld" sets the integer value to 0, should the expected behaviour be to raise an error : invalid type instead of overriding the setting with invalid value ?

huacnlee commented 3 years ago

@Ex-Ark

In fact, I ignored verification of the direct assignment to the Setting key Setting.foo = "bar", because I haven't thought about how to do the API design here to be reasonable.

So the currently added validates options just adds a way to do validate.

Maybe I will enable validation on direct assignment in next version.

Here is an example for do validation by save or valid? method:

https://github.com/ruby-china/homeland/blob/master/app/controllers/admin/site_configs_controller.rb#L19

Ex-Ark commented 3 years ago

Thanks for clarifying as well as the example 👍

Looking forward to future versions