Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow '1' or true for acceptance validation. #18439

Merged
merged 1 commit into from Jan 12, 2015

Conversation

Projects
None yet
5 participants
@mokhan
Copy link
Contributor

commented Jan 10, 2015

While pairing on a project this morning we were confused as to why this code kept failing:

# user.rb
class User < ActiveRecord::Base
  attr_accessor :terms_and_conditions
  validates_acceptance_of :terms_and_conditions
end
describe User do
  it "is valid" do
    user = User.new(terms_and_conditions: true)
    expect(user).to be_valid
  end
end

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:

# user.rb
class User
  attr_accessor :terms_and_conditions
  validates_acceptance_of :terms_and_conditions, accept: true
end

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

@sgrif

View changes

activemodel/test/cases/validations/acceptance_validation_test.rb Outdated
Topic.validates_acceptance_of(:terms_of_service)

assert Topic.new(terms_of_service: true).valid?
assert Topic.new(terms_of_service: "1").valid?

This comment has been minimized.

Copy link
@sgrif

sgrif Jan 10, 2015

Contributor

We don't need to test this here, it's already covered elsewhere.

@sgrif

View changes

activemodel/lib/active_model/validations/acceptance.rb Outdated

def valid_option?(value)
accept_options = [options[:accept]].flatten
accept_options.include?(value)

This comment has been minimized.

Copy link
@sgrif

sgrif Jan 10, 2015

Contributor

Why not just options[:accept].include?(value)?

This comment has been minimized.

Copy link
@mokhan

mokhan Jan 10, 2015

Author Contributor

To preserve backward compatibility for anyone who currently uses "accept: 'I agree.'"

  1 || ^[[H^[[JRun options: --seed 14171                                                                                                                               
  2 ||.
  3 || # Running:
  4 ||.
  5 || ...F..
  6 ||.
  7 || Finished in 0.040334s, 148.7579 runs/s, 322.3087 assertions/s.
  8 ||.
  9 ||   1) Failure:
 10 || AcceptanceValidationTest#test_terms_of_service_agreement_with_accept_value [test/cases/validations/acceptance_validation_test.rb:47]:
 11 || Failed assertion, no message given.
 12 ||.
 13 || 6 runs, 13 assertions, 1 failures, 0 errors, 0 skips
["I agree."].flatten # ["I agree."]
[[1, "true"]].flatten # [1, "true"]

If it's overkill please let me know, I'll gladly update.

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2015

This needs a changelog entry. I have no opinions on the feature itself, I'll let someone else decide whether or not to merge.

@mokhan mokhan force-pushed the mokhan:validates-acceptance-of-array branch 2 times, most recently Jan 10, 2015

@mokhan

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2015

Thank you @sgrif.

@mokhan mokhan force-pushed the mokhan:validates-acceptance-of-array branch Jan 10, 2015

@sgrif

View changes

activemodel/lib/active_model/validations/acceptance.rb Outdated
@@ -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

This comment has been minimized.

Copy link
@sgrif

sgrif Jan 11, 2015

Contributor

How about we just do Array(options[:accept]) instead?

This comment has been minimized.

Copy link
@mokhan

mokhan Jan 11, 2015

Author Contributor

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]].flatten
    

How 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.

This comment has been minimized.

Copy link
@mokhan

mokhan Jan 11, 2015

Author Contributor

All done. Should I inline the method as well?

This comment has been minimized.

Copy link
@sgrif

sgrif Jan 11, 2015

Contributor

Up to you.

@mokhan mokhan force-pushed the mokhan:validates-acceptance-of-array branch 2 times, most recently Jan 11, 2015

The default for validates_acceptance_of is now "1" and true.
In the past, only "1" was the default and you were required to add
accept: true.

This comment has been minimized.

Copy link
@prathamesh-sonpatki

prathamesh-sonpatki Jan 11, 2015

Member

@mokhan Please add CHANGELOG at the top of the file. New entries get at the top.

This comment has been minimized.

Copy link
@mokhan

mokhan Jan 11, 2015

Author Contributor

Done. Thanks @prathamesh-sonpatki

@mokhan mokhan force-pushed the mokhan:validates-acceptance-of-array branch to 140557e Jan 11, 2015

@@ -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))

This comment has been minimized.

Copy link
@kuldeepaggarwal

kuldeepaggarwal Jan 12, 2015

Contributor

I think we should add "true" in the array. e.g. ["1", true, "true"]

This comment has been minimized.

Copy link
@mokhan

mokhan Jan 12, 2015

Author Contributor

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"]
end

I would like to keep this change minimal, as I believe TrueClass should be one of the default accept options.

This comment has been minimized.

Copy link
@kuldeepaggarwal

kuldeepaggarwal Jan 12, 2015

Contributor

Somebody might using "true" in their test cases. Then what is the purpose for this patch?

This comment has been minimized.

Copy link
@sgrif

sgrif Jan 12, 2015

Contributor

I agree with @mokhan, just true is sufficient.

This comment has been minimized.

Copy link
@pboling

pboling Jan 23, 2015

Contributor

@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.

sgrif added a commit that referenced this pull request Jan 12, 2015

Merge pull request #18439 from mokhan/validates-acceptance-of-array
allow '1' or true for acceptance validation.

@sgrif sgrif merged commit 72570ea into rails:master Jan 12, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@rails rails locked and limited conversation to collaborators Jan 23, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.