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

Add support for validation contexts #246

Closed
wants to merge 1 commit into from

Conversation

sj26
Copy link
Contributor

@sj26 sj26 commented Mar 4, 2013

There's no easy way to test validation contexts with the current shoulda matchers. The context must be provided in the call to the instance's valid? method. This patch adds a simple way to optionally pass a context.

Example:

class User
  include ActiveModel::Model
  validates_presence_of :email, on: :emailable
end

describe User do
  it { should_not validate_presence_of(:email) }
  it { should validate_presence_of(:email).on(:emailable) }
end

Previously this would have to involve stubbing the implementation detail validation_context, simply setting it is not enough as it is overridden by every run of valid?:

describe User do
  it { should_not validate_presence_of(:email) }
  context do
    before { subject.stub(validation_context: :emailable) }
    it { should validate_presence_of(:email) }
  end
end

This can also be used for the idiomatic activerecord contexts:

class User < ActiveRecord::Base
  validates_presence_of :password, on: :create
end

describe User do
  it { should validate_presence_of(:password).on(:create) }
  it { should_not validate_presence_of(:password).on(:update) }
end


context "without the validation context" do
it "allows a bad value" do
model.should allow_value("xyz").for(:attr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using your let above and model.should here (and lines 43, 47), take advantage of the validating_format method that's already available (from spec/shoulda/matchers/active_model/allow_value_matcher_spec.rb). It'll look something like this:

 validating_format(:with => /abc/, :on => :customisable).should allow_value("xyz").for(:attr)

@mxie
Copy link
Contributor

mxie commented Mar 25, 2013

I like this idea, and it looks like this would fulfill #131 as well, so that's awesome! However, please rebase and check out my comment on the spec.

@mxie
Copy link
Contributor

mxie commented Mar 29, 2013

Also, I just noticed that your allow_value_matcher_spec.rb file is in a different directory structure than it's supposed to be in...definitely rebase and resolve that please. Thanks!

@sj26
Copy link
Contributor Author

sj26 commented Apr 4, 2013

Sorry for the delays, but I've rebased and fixed up those specs—master has moved fast! Should be good to go now.

@camelmasa
Copy link

👍

@drapergeek
Copy link
Contributor

Thanks @sj26! Merged.

@anujbiyani
Copy link

Any reason why this functionality wasn't added to validate_numericality_of?

@mcmire
Copy link
Collaborator

mcmire commented Sep 11, 2013

I believe this should work with all matchers, is this not working with validate_numericality_of?

@anujbiyani
Copy link

Failures:

  1) Upload 
     Failure/Error: it { should validate_numericality_of(:picture_count).on(:create).is_greater_than(0) }
     NoMethodError:
       undefined method `on' for #<Shoulda::Matchers::ActiveModel::ValidateNumericalityOfMatcher:0x007fa324d82c40>
     # ./spec/models/upload_spec.rb:11:in `block (2 levels) in <top (required)>'

This is on version 2.3.0.

I'll try and open a PR (or at least a separate issue).

@mcmire
Copy link
Collaborator

mcmire commented Sep 11, 2013

Okay, thanks.

@sj26
Copy link
Contributor Author

sj26 commented Sep 12, 2013

@anujbiyani did #313 fix your issue? There was an oversight which means some matchers weren't accepting #on.

@sj26
Copy link
Contributor Author

sj26 commented Sep 12, 2013

Ah, no, I see— ValidateNumericalityOfMatcher doesn't inherit from ValidationMatcher since 6ba8c9f. Have you submitted a pull yet, or shall I?

(I don't see anything in your activity so I'll have a stab.)

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

Successfully merging this pull request may close these issues.

6 participants