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

Clear up confusion w/ presence around belongs_to associations. (#1214) breaks existing behaviors. #1218

Closed
kamillle opened this issue Jun 11, 2019 · 7 comments · Fixed by #1231

Comments

@kamillle
Copy link

kamillle commented Jun 11, 2019

v4.1.0 includes a change Clear up confusion w/ presence around belongs_to associations. (#1214 ) . This breaks existing behaviors.

My application code is below.

class HogeForm
  include ActiveModel::Model
  include ActiveModel::Attributes

  attribute :user
  attribute :note, :string

  validates :user, presence: true
  validates :note, presence: true, if: %i[user]

  def save
    return false if invalid?

    # do something
  end
end

And test code is below.

RSpec.describe HogeForm do
  context 'Validations' do
    it { is_expected.to validate_presence_of(:user) }
    ...
    ...
    ...
  end
end

When execute it { is_expected.to validate_presence_of(:user) }, then user attribute is inserted nil until v4.0.1, but in v.4.1.0 user attribute is inserted "" (empty string) and validates :note, presence: true, if: %i[user] is run unintentional.

Why did you determine to change the behavior? and why not restore the behavior?

@mcmire
Copy link
Collaborator

mcmire commented Jun 11, 2019

The presence validation calls .present? on your attribute to determine whether it's "present". Presence is defined as either an empty string or nil: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/object/blank.rb#L6. Therefore, to truly test the presence validation, you need to see what happens if your attribute is an empty string. The matcher really should have been doing that all along, so that's why I added it.

That said, I can change it so that it only acts on an ActiveRecord model so that your tests pass again.

@mcmire
Copy link
Collaborator

mcmire commented Jun 13, 2019

@kamillle Looking back at this, I'm having trouble writing a failing test for this based on your example. Are your tests breaking currently, and if so, which ones?

@kamillle
Copy link
Author

kamillle commented Jun 14, 2019

@mcmire oops...
Sorry for providing wrong code.
Correct code is below, plz check it.

### application code ###
class HogeForm
  include ActiveModel::Model
  include ActiveModel::Attributes

  attribute :user

  validates :user, presence: true
  validate :validate_something, if: %i[user]

  def save
    return false if invalid?

    # do something
  end

  private.
  def validate_something
    return if user.email

    errors.add(:base, 'Error!!!')
  end
end

### test code ###
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe HogeForm, type: :model do
  context 'Validations' do
    it { is_expected.to validate_presence_of(:user) }
  end
end

it { is_expected.to validate_presence_of(:user) } was passed until v4.0.1, but the test cat not be passed in v4.1.0.

  $ bin/rspec spec/forms/hoge_form_spec.rb
Running via Spring preloader in process 34469

Randomized with seed 26709
F

Failures:

  1) HogeForm Validations should validate that :user cannot be empty/falsy
     Failure/Error: return if user.email

     NoMethodError:
       undefined method `email' for "":String
     # ./app/forms/hoge_form.rb:19:in `validate_something'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:426:in `block in make_lambda'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:179:in `block (2 levels) in halting_and_conditional'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:606:in `block (2 levels) in default_terminator'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:605:in `catch'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:605:in `block in default_terminator'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:180:in `block in halting_and_conditional'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:513:in `block in invoke_before'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:513:in `each'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:513:in `invoke_before'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:131:in `run_callbacks'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/callbacks.rb:816:in `_run_validate_callbacks'
     # ./.bundle/ruby/2.5.0/gems/activemodel-5.2.2.1/lib/active_model/validations.rb:409:in `run_validations!'
     # ./.bundle/ruby/2.5.0/gems/activemodel-5.2.2.1/lib/active_model/validations.rb:339:in `valid?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validator.rb:96:in `perform_validation'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validator.rb:89:in `validation_result'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validator.rb:85:in `validation_error_messages'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validator.rb:64:in `messages'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validator.rb:25:in `has_messages?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validator.rb:55:in `messages_match?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validator.rb:21:in `call'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:38:in `matches?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `each'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `detect'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/allow_value_matcher/attribute_setters_and_validators.rb:24:in `first_passing'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:534:in `public_send'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:534:in `run'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/allow_value_matcher.rb:401:in `does_not_match?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/disallow_value_matcher.rb:32:in `matches?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validation_matcher.rb:162:in `run_allow_or_disallow_matcher'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validation_matcher.rb:98:in `disallows_value_of'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:233:in `disallows_original_or_typecast_value?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:156:in `block in matches?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:155:in `each'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:155:in `all?'
     # ./.bundle/ruby/2.5.0/gems/shoulda-matchers-4.1.0/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb:155:in `matches?'
     # ./spec/forms/hoge_form_spec.rb:9:in `block (3 levels) in <top (required)>'
     # ./.bundle/ruby/2.5.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:54:in `load'
     # ./.bundle/ruby/2.5.0/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:54:in `load'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/dependencies.rb:285:in `block in load'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/dependencies.rb:257:in `load_dependency'
     # ./.bundle/ruby/2.5.0/gems/activesupport-5.2.2.1/lib/active_support/dependencies.rb:285:in `load'
     # ./.bundle/ruby/2.5.0/gems/spring-commands-rspec-1.0.4/lib/spring/commands/rspec.rb:18:in `call'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/command_wrapper.rb:38:in `call'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/application.rb:201:in `block in serve'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/application.rb:171:in `fork'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/application.rb:171:in `serve'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/application.rb:141:in `block in run'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/application.rb:135:in `loop'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/application.rb:135:in `run'
     # ./.bundle/ruby/2.5.0/gems/spring-2.0.2/lib/spring/application/boot.rb:19:in `<top (required)>'
     # -e:1:in `<main>'

@mcmire
Copy link
Collaborator

mcmire commented Jul 7, 2019

@kamillle Hey, I just pushed up a branch, fix-presence-validation-for-activemodel, that should fix this issue. Can you temporarily switch to it in your Gemfile and see if makes your tests pass?

@kamillle
Copy link
Author

kamillle commented Jul 7, 2019

@mcmire Sure. I'll check that!! Thank you!😍

@kamillle
Copy link
Author

kamillle commented Jul 8, 2019

@mcmire The fix is completely working in my case!!! Appriciate your working.😆🙌🙌

@mcmire
Copy link
Collaborator

mcmire commented Jul 16, 2019

This fix is live in v4.1.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants