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

Validation should allow empty field value if use allow_nil: true #737

Closed
soulfly opened this issue Mar 30, 2018 · 5 comments
Closed

Validation should allow empty field value if use allow_nil: true #737

soulfly opened this issue Mar 30, 2018 · 5 comments
Assignees
Milestone

Comments

@soulfly
Copy link

soulfly commented Mar 30, 2018

I use the following validation in my Rails model:

  validates :website, length: {maximum: 200},
                                 format: {with: URI::regexp},
                              allow_nil: true

Unfortunately it does not allow me to save empty field value, I'm getting the following error when submit a form:
image

Looks like it comes from here
https://github.com/DavyJonesLocker/client_side_validations/blob/master/coffeescript/rails.validations.coffee#L287

From my understanding we may need to add return if options.allow_nil == true as well there

And also replace the line
hash[:allow_blank] = true if options[:allow_blank]
with
hash[:allow_blank] = true if options[:allow_nil] || options[:allow_blank]

here https://github.com/DavyJonesLocker/client_side_validations/blob/master/lib/client_side_validations/active_model/length.rb#L30

So you already have it for 'numeric' validation here https://github.com/DavyJonesLocker/client_side_validations/blob/master/lib/client_side_validations/active_model/numericality.rb#L44
so I propose to do the same for 'length' validation

Do you have any issues with the suggestion?

I use Rails 4.2 and 4.2.12 version of this gem but it looks like the issue is persist on master as well

@soulfly
Copy link
Author

soulfly commented Apr 3, 2018

@tagliala do you have any feedback here? I'm also happy to prepare a PR on my side, just let me know is it worth trying.

@tagliala
Copy link
Contributor

tagliala commented Apr 6, 2018

@soulfly thanks, will do that by myself on the master branch. I will also check for other "allow_blank"

btw I will not fix the old 4.2 branch

@tagliala tagliala self-assigned this Apr 6, 2018
@tagliala tagliala added this to the 11.1.2 milestone Apr 6, 2018
@soulfly
Copy link
Author

soulfly commented Apr 6, 2018

Great thanks!

btw I will not fix the old 4.2 branch

that's fine, I will port your changes by myself to some custom branch

@tagliala
Copy link
Contributor

tagliala commented Apr 7, 2018

@soulfly could you please give a try to the master branch?

@soulfly
Copy link
Author

soulfly commented Apr 9, 2018

Thanks @tagliala ! I will give it a try

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

No branches or pull requests

2 participants