allow '1' or true for acceptance validation. #18439
Conversation
| Topic.validates_acceptance_of(:terms_of_service) | ||
|
|
||
| assert Topic.new(terms_of_service: true).valid? | ||
| assert Topic.new(terms_of_service: "1").valid? |
There was a problem hiding this comment.
We don't need to test this here, it's already covered elsewhere.
|
This needs a changelog entry. I have no opinions on the feature itself, I'll let someone else decide whether or not to merge. |
|
Thank you @sgrif. |
| @@ -20,6 +20,11 @@ def setup!(klass) | |||
| klass.send(:attr_reader, *attr_readers) | |||
| klass.send(:attr_writer, *attr_writers) | |||
| end | |||
|
|
|||
| def acceptable_option?(value) | |||
| accept_options = [options[:accept]].flatten | |||
There was a problem hiding this comment.
How about we just do Array(options[:accept]) instead?
There was a problem hiding this comment.
Works for me. I'll update shortly. :)
On Jan 10, 2015 5:07 PM, "Sean Griffin" notifications@github.com wrote:
In activemodel/lib/active_model/validations/acceptance.rb
#18439 (diff):@@ -20,6 +20,11 @@ def setup!(klass)
klass.send(:attr_reader, *attr_readers)
klass.send(:attr_writer, *attr_writers)
end
+
def acceptable_option?(value)accept_options = [options[:accept]].flattenHow about we just do Array(options[:accept]) instead?
—
Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/18439/files#r22763094.
There was a problem hiding this comment.
All done. Should I inline the method as well?
| @@ -3,12 +3,12 @@ module ActiveModel | |||
| module Validations | |||
| class AcceptanceValidator < EachValidator # :nodoc: | |||
| def initialize(options) | |||
| super({ allow_nil: true, accept: "1" }.merge!(options)) | |||
| super({ allow_nil: true, accept: ["1", true] }.merge!(options)) | |||
There was a problem hiding this comment.
I think we should add "true" in the array. e.g. ["1", true, "true"]
There was a problem hiding this comment.
I wonder if that would lead to a possible explosion of options. i.e. "True", "TRUE"
Currently, if you need to add additional accept options you can do so.
class User < ActiveRecord::Base
validates_acceptance_of :terms_and_conditions, accept: ["True", "TRUE", "true"]
endI would like to keep this change minimal, as I believe TrueClass should be one of the default accept options.
There was a problem hiding this comment.
Somebody might using "true" in their test cases. Then what is the purpose for this patch?
There was a problem hiding this comment.
I agree with @mokhan, just true is sufficient.
There was a problem hiding this comment.
@kuldeepaggarwal - the point is now you can specify an array of acceptable values.
If people were using 'true' before their tests weren't passing, so that's a non issue. Now you will be able to accept ["1", true, "true"] as you suggest by configuring it in the class:
class User < ActiveRecord::Base
validates_acceptance_of :terms_and_conditions, accept: ["1", true, "true"]
end
You can have it accept anything in the array where before an array wasn't possible, just a single value.
allow '1' or true for acceptance validation.
While pairing on a project this morning we were confused as to why this code kept failing:
After looking through the source we found that the default value is: "1". This seems nonintuitive. To make this test pass we had to change our code to this:
This Pull Request changes the default accept from "1" to ["1", true]. Allowing you to specify an array of options. It supports single values as well for backwards compatibility.
/cc @speasley