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 with conditions #128

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aviscasillas
Copy link

I just want to set starting point with conditional validations.
I think it'd be cool to provide some feature to execute validations just in some defined contexts.
In this PR I'm adding the :if option as a very silly way to provide this.

The `:if` options allows to define conditions for validations to be executed
just in concrete contexts.
@sxross
Copy link
Owner

sxross commented Nov 21, 2014

This is really great. Also parallels the way Rails can handle validations. Here are a couple of requests:

  • It would be good to make procs work :if -> (row) { row.password.match(/[0-9]/) != nil }
  • It would be good to have a contextual example of when this kind of validation is useful

By the way, this is failing in Travis. Sometimes Travis needs a refresh to pass.

Thanks again!

@aviscasillas
Copy link
Author

Oops! sorry for the failing tests, they go green locally 😓.

And agree with you! would be better to get it working with procs too, so I've written just the first step. I thought on something like this:

validate :some_column, presence: true, if: -> { some_accessor == my_desired_value }

But I'm not sure about how could I access the model attributes implicitly from inside the block. Do you know what I mean? Do you know how can I do it?

My use case by the moments works well with what is done in this PR:
I have a User model and I want to set different validations for creation process and for update process.
You can see something about what I'm saying in the following example:

# app/models/user.rb

class User
  include MotionModel::Model
  include MotionModel::ArrayModelAdapter
  include MotionModel::Validatable

  columns :external_id, :email, :password, :password_confirmation

  validate :email, presence: true, email: true
  validate :password, presence: true, if: :new_user?
  validate :password_confirmation, confirmation: true, if: :new_user?

  def new_user?
    external_id.nil?
  end
end
# app/screens/signup.rb

# (...)
u = User.new(email: '[email protected]', password: 'secret', password_confirmation: 'secret')
if u.save
  # do successful stuff
else
  # do failure stuff
end
# (...)

@sxross
Copy link
Owner

sxross commented Nov 21, 2014

It looks like you are doing a send for the :if unless I've misunderstood your code. This is air-code below, but see if this handles the two cases.

if validation[:if]
  case validation[:if]
    when Symbol then return send(validation[:if])
    when Proc then return call(validation[:if])
    else
      raise ArgumentError.new(':if requires a Symbol denoting a method or a Proc/lambda')
    end
  end
end

Note that the validator is an instance method of the model, so you would be able to write code like this:

validate :password, if: -> { password.length > 7 && password.match(/[0-9]/ && new_user? }

# or

validate :password, if: :strong_password?

# ...

def strong_password?
  # some code to really figure out password strength
end

What do you think? Would something like this work?

@aviscasillas
Copy link
Author

You are right, it should work. Let me check it out and I'll come back with some changes 😉
Just one comment on your example. I think that maybe the strong_password? method wouldn't be a condition, it would be a validation, and you could set this validation under a condition with something like:

validate :password, strong_password: true, if: :have_to_validate_password?

Right?

Thx!

@sxross
Copy link
Owner

sxross commented Nov 21, 2014

I just used strong_password as an example. You might want to validate that an address has a numeric zip code if it is in the US. So that might be like this:

validate :zip, if: -> {
  case country
    when 'US' return zip.match(/[^0-9]{5}(-[0-9]{4})?/
    else return !zip.empty?
  end
}

Something like that. It's just one other way to write validating code -- moving the method up to the validate macro and not writing a separate method unless there is more than one field that requires the same validation.

@aviscasillas
Copy link
Author

Yes, totally agree.

@aviscasillas
Copy link
Author

Sorry @sxross, I was thinking on it and I'm not "totally agree" with you.

  • My point: Use :if to define conditions on when the validation must be executed (rails way).
  • Your point: Use :if to define a custom validation (not the rails way).

And I think that two scopes for the same option is not correct.
I think your point could be good in a separate feature. Maybe using another option like :with:

validate :password, with: -> {  password.length > 7 && password.match(/[0-9]/ && new_user? }

What do you think?

@aviscasillas aviscasillas force-pushed the validate-with-conditions branch from d5cedb3 to 5f9d47b Compare November 22, 2014 13:33
@aviscasillas
Copy link
Author

@sxross Travis is failing again. I don't understand why, all is working fine in my machine.
Could you refresh Travis to see if the problem persists?

Thx!

@sxross
Copy link
Owner

sxross commented Nov 22, 2014

I agree. That makes perfect sense.

On Nov 22, 2014, at 3:53 AM, Albert Viscasillas Campanyà [email protected] wrote:

Sorry @sxross https://github.com/sxross, I was thinking on it and I'm not "totally agree" with you.

My point: Use :if to define conditions on when the validation must be executed (rails way).
Your point: Use :if to define a custom validation (not the rails way).
And I think that two scopes for the same option is not correct.
I think your point could be good in a separate feature. Maybe using another option like :with:

validate :password, with: -> { password.length > 7 && password.match(/[0-9]/ && new_user? }
What do you think?


Reply to this email directly or view it on GitHub #128 (comment).

@sxross
Copy link
Owner

sxross commented Nov 22, 2014

Please try removing rake from the Gemfile. See if that helps.

@aviscasillas aviscasillas force-pushed the validate-with-conditions branch 13 times, most recently from 5879b0e to aa45ed7 Compare November 22, 2014 18:56
@aviscasillas aviscasillas force-pushed the validate-with-conditions branch from aa45ed7 to 20b82cb Compare November 22, 2014 18:59
@aviscasillas
Copy link
Author

@sxross I've made it working doing some changes in Travis configuration and gem dependencies.
Can you review the last commit to check that's all right?

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

Successfully merging this pull request may close these issues.

2 participants