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

Ability to specify multiple contexts when defining a validation. #13754

Merged
merged 1 commit into from Jan 27, 2014

Conversation

@vpuzzella
Copy link
Contributor

@vpuzzella vpuzzella commented Jan 18, 2014

Example:

validates_presence_of :name, on: [:update, :custom_validation_context]

@vpuzzella
Copy link
Contributor Author

@vpuzzella vpuzzella commented Jan 18, 2014

This was requested in #9659, but I think it was closed by mistake.

@robin850
robin850 reviewed Jan 18, 2014
View changes
activemodel/lib/active_model/validations.rb Outdated
@@ -67,7 +67,7 @@ module ClassMethods
#
# Options:
# * <tt>:on</tt> - Specifies the context where this validation is active

This comment has been minimized.

@robin850

robin850 Jan 18, 2014
Member

I think that context could be plural here now. Also, what do you think about :

Specifies the contexts where this validation is active. You can either pass a symbol or a collection of actions.

?

This comment has been minimized.

@robin850

robin850 Jan 18, 2014
Member

Also, I think that the documentation of the #validate method should be updated as well.

This comment has been minimized.

@vpuzzella

vpuzzella Jan 18, 2014
Author Contributor

Done.

@robin850
robin850 reviewed Jan 18, 2014
View changes
activemodel/lib/active_model/validations.rb Outdated
@@ -67,7 +67,7 @@ module ClassMethods
#
# Options:
# * <tt>:on</tt> - Specifies the context where this validation is active
# (e.g. <tt>on: :create</tt> or <tt>on: :custom_validation_context</tt>)
# (e.g. <tt>on: :create</tt> or <tt>on: :custom_validation_context or <tt>on: [:create, :custom_validation_context]</tt>)

This comment has been minimized.

@robin850

robin850 Jan 18, 2014
Member

Could you wrap this around 80 chars please ?

This comment has been minimized.

@vpuzzella

vpuzzella Jan 18, 2014
Author Contributor

Done.

@robin850
robin850 reviewed Jan 18, 2014
View changes
activemodel/test/cases/validations/validations_context_test.rb Outdated
end

This comment has been minimized.

@robin850

robin850 Jan 18, 2014
Member

✂️

This comment has been minimized.

@vpuzzella

vpuzzella Jan 18, 2014
Author Contributor

Done. Thanks for the feedback!

@rbarazi
Copy link

@rbarazi rbarazi commented Jan 19, 2014

👍

@senny
senny reviewed Jan 20, 2014
View changes
activemodel/test/cases/validations/validations_context_test.rb Outdated

topic = Topic.new
assert topic.invalid?(:context2), "Validation does run on context 2"
assert topic.errors[:base].include?(ERROR_MESSAGE)

This comment has been minimized.

@senny

senny Jan 20, 2014
Member

let's also add the case where the validation does not run.

This comment has been minimized.

@vpuzzella

vpuzzella Jan 20, 2014
Author Contributor

Added another test case. Thanks for the feedback!

@@ -1,3 +1,11 @@
* Ability to specify multiple contexts when defining a validation.

This comment has been minimized.

@senny

senny Jan 20, 2014
Member

Can you add Example: before the example code? See other entries for reference.

This comment has been minimized.

@vpuzzella

vpuzzella Jan 20, 2014
Author Contributor

Example added to changelog.

@robin850
Copy link
Member

@robin850 robin850 commented Jan 25, 2014

@vpuzzella : For future contributions, could you post a comment once your patch is updated please ? :-) GitHub does not create notifications when you push the commit. Thank you!

This looks good to me so far!

@vpuzzella
Copy link
Contributor Author

@vpuzzella vpuzzella commented Jan 25, 2014

@robin850, will do. At this point I feel the patch is complete. Is there anything else required in order to have the PR accepted?

@robin850
Copy link
Member

@robin850 robin850 commented Jan 25, 2014

@vpuzzella : Thanks! Everything seems fine to me but I'm not a merger, this decision is up to @senny.

@senny
Copy link
Member

@senny senny commented Jan 27, 2014

@vpuzzella the PR does no longer apply and needs a rebase.

Example:

validates_presence_of :name, on: [:update, :custom_validation_context]
@vpuzzella
Copy link
Contributor Author

@vpuzzella vpuzzella commented Jan 27, 2014

@senny, rebased and CI build passing.

carlosantoniodasilva added a commit that referenced this pull request Jan 27, 2014
Ability to specify multiple contexts when defining a validation.
@carlosantoniodasilva carlosantoniodasilva merged commit 50d9717 into rails:master Jan 27, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Jan 27, 2014

Thanks.

@pierre-pretorius
Copy link

@pierre-pretorius pierre-pretorius commented Jun 6, 2014

This feature is very useful thank you. I have the following suggestion: when someone specifies a validation with context :save, it should be called for :create and :update.

For example, if you have two steps, the first step someone needs to enter his name, the second step his lastname, but it should only be able to save if name, surname and token is populated, you currently have to do it like this:

validates :name, presence: true, on: [:create, :update, :step1]
validates :lastname, presence: true, on: [:create, :update, :step2]
validates :token, presence: true, on: [:create, :update]

It would be much simpler if you could write it as:

validates :name, presence: true, on: [:save, :step1]
validates :lastname, presence: true, on: [:save, :step2]
validates :token, presence: true, on: :save

In fact, consider the how callbacks are specified (before_save, before_create, before_update) this actually seems like a bug.

@pierre-pretorius
Copy link

@pierre-pretorius pierre-pretorius commented Jun 24, 2014

@vpuzzella Any comments on my suggestion above?

@robin850
Copy link
Member

@robin850 robin850 commented Jun 25, 2014

@pierre-pretorius : Please post feature requests to the rails-core mailing list or send a patch (unless I'm wrong, this seems straightforward here) ; thank you! :-)

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.