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

Validate confirmation of with integers #1012

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -94,28 +94,48 @@ def simple_description

def matches?(subject)
super(subject)

disallows_different_value &&
allows_same_value &&
allows_missing_confirmation
type = find_type(subject)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If find_type no longer takes a subject, then you don't really need to assign a variable to the method -- you can use it as though it were a variable, and that means you don't need to pass it to every place that needs it. What do you think about renaming find_type to simply attribute_type?

disallows_different_value(type) &&
allows_same_value(type) &&
allows_missing_confirmation(type)
end

private

def disallows_different_value
disallows_value_of('different value') do |matcher|
qualify_matcher(matcher, 'some value')
def find_type(subject)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things here:

  • subject is already assigned to @subject in ValidationMatcher#matches? and defined as an attr_reader there, so there's no need to pass it to every method that needs it, you can just refer to subject anywhere in this file.
  • Also, the way you've written this, you are rescuing every exception that occurs when calling subject.class.columns_hash['attribute_to_confirm'].type. It's unclear just glancing at the code why you're doing that. Is it because columns_hash["attribute_to_confirm"] could return nil? If so, then check for that explicitly.
  • Where is "attribute_to_confirm" coming from? If it's from the test, then that's probably not correct -- the attribute name could be anything here. attribute is the attribute under test, so you probably want to use that here.

subject.class.columns_hash['attribute_to_confirm'].type rescue :string
end

def different_value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about defining these as constants rather than methods? Also, what do you think about using the plural (DIFFERENT_VALUES) rather than the singular?

{ integer: 3, string: 'different value' }
end

def some_value
{ integer: 2, string: 'some value' }
end

def same_value
{ integer: 2, string: 'same value' }
end

def any_value
{ integer: 2, string: 'any value' }
end

def disallows_different_value(type)
disallows_value_of(different_value[type]) do |matcher|
qualify_matcher(matcher, some_value[type])
end
end

def allows_same_value
allows_value_of('same value') do |matcher|
qualify_matcher(matcher, 'same value')
def allows_same_value(type)
allows_value_of(same_value[type]) do |matcher|
qualify_matcher(matcher, same_value[type])
end
end

def allows_missing_confirmation
allows_value_of('any value') do |matcher|
def allows_missing_confirmation(type)
allows_value_of(any_value[type]) do |matcher|
qualify_matcher(matcher, nil)
end
end
Expand Down
3 changes: 2 additions & 1 deletion spec/support/unit/record_validating_confirmation_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ def attribute_that_receives_error
private

def create_model
define_model(model_name, attribute_to_confirm => :string) do |model|
type = options[:type] || :string
define_model(model_name, attribute_to_confirm => type) do |model|
model.validates_confirmation_of(attribute_to_confirm, options)
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,36 @@
def validation_matcher_scenario_args
super.deep_merge(matcher_name: :validate_confirmation_of)
end

context 'when attribute to confirm is type integer' do
context 'when the model has a confirmation validation' do
it 'passes' do
builder = builder_for_record_validating_confirmation(type: :integer)
expect(builder.record).
to validate_confirmation_of(builder.attribute_to_confirm)
end
end

context 'when the model has all attributes, but does not have the confirmation'do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's tweak this wording a bit. What do you think about just using:

  • "when the model has a confirmation validation on the given attribute"
  • "when the model does not have a confirmation validation on the given attribute"

it 'fails with an appropriate error message' do
model = define_model(:example, attribute_to_confirm: :integer) do
attr_accessor :attribute_to_confirm_confirmation
end

assertion = lambda do
expect(model.new).to validate_confirmation_of(:attribute_to_confirm)
end

message = <<-MESSAGE
Example did not properly validate that
:attribute_to_confirm_confirmation matches :attribute_to_confirm.
After setting :attribute_to_confirm_confirmation to ‹2›, then setting
:attribute_to_confirm to ‹3›, the matcher expected the Example to be
invalid, but it was valid instead.
MESSAGE

expect(&assertion).to fail_with_message(message)
end
end
end
end