AR validations should be checked better for figuring out required fields #610

Merged
merged 4 commits into from Jul 4, 2011

Conversation

Projects
None yet
2 participants
Contributor

julian7 commented Jun 27, 2011

Current validation checks some aspects of ActiveRecord validations, but it does it too well, marking optional fields required (and, in theory, the other way around, making :presence => { :allow_blank => true } possible).

My patch has a bit stricter required checks for presence, inclusion, and length AR validators:

  • presence has no options (eg. it should not allow :allow_blank => true)
  • inclusion accepts :allow_blank
  • length accepts :allow_blank, but it does accept :minimum and :within too. When any of the latter two starts at zero, the field should not be required.

Specs attached.

julian7 added some commits Jun 27, 2011

@julian7 julian7 Added failing test for validates_length_of :maximum-only setting
Signed-off-by: Balazs Nagy <julsevern@gmail.com>
d1c3365
@julian7 julian7 html5 validators should reflect AR validations more strictly
* validates_presence_of has no options
* validates_inclusion_of can be optional only if :allow_blank => true
* validates_length_of can be optional if either :allow_blank => true,
  :minimum is > 0, or :within's least value is > 0

Signed-off-by: Balazs Nagy <julsevern@gmail.com>
be7285f
@julian7 julian7 Better specs for converting AR validators to required parameters
Signed-off-by: Balazs Nagy <julsevern@gmail.com>
7b22ff0
@julian7 julian7 input_helper_spec refactor, typo fix
Signed-off-by: Balazs Nagy <julsevern@gmail.com>
bbc4052
Owner

justinfrench commented Jun 28, 2011

Awesome work! Which open issues (if any) does this resolve? #609 seems likely based on your comments.

Contributor

julian7 commented Jun 28, 2011

I think multiple issues can be involved. I'd ask case requestors to re-test their problem on the patched version, or at least they should write a failing test.

@justinfrench justinfrench added a commit that referenced this pull request Jul 4, 2011

@justinfrench justinfrench Merge pull request #610 from julian7/validates_presence
AR validations should be checked better for figuring out required fields
88fb05f

@justinfrench justinfrench merged commit 88fb05f into justinfrench:master Jul 4, 2011

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