judgegem / judge

Client-side form validation for Rails
MIT License
256 stars 41 forks source link

Super BUG - The validate load in data all the validation existing for a specific field #45

Closed Dinuz closed 8 years ago

Dinuz commented 9 years ago

@joecorcoran

I am attaching an image because is more easy to look than write:

judge4

This is a sign_in form (field request are email and password), as you can see in the image the gem load in data-validable all another bunch of validation that are not present in my form. The most evident is the confirmation password, I have that validation in my user model, but i have for a sign_up form, in another page. Why the validator load also this? actually it loads all the possible validation present in the model for a specific field.

How this is possible?

I really don't understand it. Any thoughts, words or ideas would be more than appreciated.

Best Dinuz

Dinuz commented 9 years ago

Actually this I a very bad behavior: the gem throws a js error because on the signin form, when it goes to validate the password field, doesn't find obviously a password confirmation field...the js error is related with the confirmation validator!

This Could be in someway related to the rails version I am using? The 4.2. Rc3

If so probably the gem should be updated on the server side code.

I am waiting for your feedback @joecorcoran

joecorcoran commented 9 years ago

Don't validate the password on your sign in page then...? Judge doesn't do any validation by default, you must be both using Judge::FormBuilder and also calling some JS to run the validations on that page. You can validate inputs one by one, if you want to validate some fields but not the password.

Dinuz commented 9 years ago

Dear Joe, I don't understand. Please correct me if I am wrong. Supposedly I should use validate on the input field(from a rails point of view), to tell to judge which field I want to validate. Then judge automatically would put that specific validations(the validations active for the field), in a data-validate tag, and that is used by judge(js in this case), to run the client side validations for that field. In other words, if for example I have a form with an email, and in my model I have presence and format validations, adding the validate true to the field, should bring judge to create a data-validate with just presence and format. Instead what happens is that judge create the data-validate with all the validations existing for a field email, also if these validations are in different models. Looks like judge doesn't distinguish between email of model A and email of model B, but instead put all the email validations(a+b).

Obviously judge doesn't perform any validations, if after adding the data-validate I don't call some js to run them.

The problem that I see, is not related with the js call, but instead I think, it is related with the way in which the data-validate is built!

If you look into the image that I previously attached, you see in the data field multiple repetition of the same validation, and that should be wrong!

What do you think?

Sent from my iPhone

On Jan 1, 2015, at 11:42 AM, Joe Corcoran notifications@github.com wrote:

Don't validate the password on your sign in page then...? Judge doesn't do any validation by default, you must be both using Judge::FormBuilder and also calling some JS to run the validations on that page. You can validate inputs one by one, if you want to validate some fields but not the password.

— Reply to this email directly or view it on GitHub.

joecorcoran commented 9 years ago

What does your User model look like? I see three validators in the password field data attribute: presence, length and confirmation.

Dinuz commented 9 years ago

Exactly Joe...you see 3 validators, but actually they should be just 2! This is a sign in form, and there is not a password confirmation field. The password confirmation field exist on the signup form where you choose a password and confirm it. The user model obviously is the same(if can help refer to a devise user model), but the validators the you want to run are different on the sign in and sign up form.

What I see it's happening(and maybe this is related to the way in which judge build up the data-validator tag), if we refer to the password attribute, is that judge is not able to distinguish between the presence or absence of a specific field, and instead it loads any validators that is set on the attribute.

I mean, if my form doesn't have a password confirmation field, i don't want see a confirmation validator in the data-validator attribute.

Moreover the thing get super frustrating when a same attribute is used on multiple models. If we refer for example to the email attribute, in my case it is part of the user model, and is also part of a beta invite model. Here if I put a validator for the email in the beta invite model, and put other completely different validators on the email in the user model, when I use judge on the form(eg the sign in form for the user again), judge create a data-validator attribute for the email, that include also the validator of the email in the beta invite model. This in my opinion is really wrong! I mean doesn't make any sense at all. In a user model form, I shouldn't have any validators of the beta invite model, also if the email attribute is present in the two models. They are two different models, and they have two different purpose and logic. Judge clearly is not able to recognize this difference, and treat the email attribute as one(mixing together validators of different models) Please let me understand what really is not so clear here?

joecorcoran commented 9 years ago

Okay, the sign in form: I can sort of see why it might be annoying that things break without the presence of the confirmation input. At the same time – why validate a password field on a sign in form? The user is either going to enter their own password correctly or not, I don't see the value in giving them hints about length and presence. On a sign up form, sure, but when logging in? What's the point? Maybe a way of specifying which validators you care about would be useful in the general case. Something like

// pseudo-code
judge.validate(element, ['presence', 'length'], {
  valid: function() {},
  invalid: function() {}
});

might be good, but aside from that, I'm struggling to see how Judge is supposed to know the specifics of your UX requirements.

The multiple models thing sounds a bit worrying. I suspect it's something specific to your app, but I'd love to see a minimal reproduction of the problem in the form of a new Rails app, so we can talk about it more.

Dinuz commented 9 years ago

I agree with you! The sign in form was just an example, and probably not the best one. But have the ability to specify which validators you care about I believe is a real great thing.

The multiple models thing instead is kind of scary, and as I said before tomorrow I will build up a base rails app, just for testing purposes.

In the meantime I can assure you is not related with my specific app(the way in which it is build is really basic, there are not crazy things around...it's just a devise model and another basic beta model). But let's see how it behaves with a real basic app(I will just bootstrap it and auto generate with rails, without any front end stuff, except the js call to judge).

Note(I mentioned it on the other post about the new judge release), this mixing models stuff become more nasty if I use a custom validator that use a call to the judge controller(I mean if the validator is an ajax one).

Joe don't get me wrong, I really want that judge works in the right way, simply because I think is the best implementation of a client side validator, and it is very light and compact. If we can tweak a bit judge to perform its task in a better way I will be very happy, and your gem definitely will be the best gem for this type of thing.

Sent from my iPhone

On Jan 1, 2015, at 5:59 PM, Joe Corcoran notifications@github.com wrote:

Okay, the sign in form: I can sort of see why it might be annoying that things break without the presence of the confirmation input. At the same time – why validate a password field on a sign in form? The user is either going to enter their own password correctly or not, I don't see the value in giving them hints about length and presence. On a sign up form, sure, but when logging in? What's the point? Maybe a way of specifying which validators you care about would be useful in the general case. Something like

judge.validate(element, ['presence', 'length'], { valid: function() {}, invalid: function() {} }); might be good, but aside from that, I'm struggling to see how Judge is supposed to know the specifics of your UX requirements.

The multiple models thing sounds a bit worrying. I suspect it's something specific to your app, but I'd love to see a minimal reproduction of the problem in the form of a new Rails app, so we can talk about it more.

— Reply to this email directly or view it on GitHub.

Dinuz commented 9 years ago

Probably the less intrusive way to specify which validators someone cares about, should be done on the rails side(instead of just validate, we could add the functionality there - eg skip something, I believe the old client-side-validations gem has something similar). This will be more robust than doing the same thing in js(I mean I prefer having my data-validate attribute be changed on server side), for the simple reason that this will give me control directly on the form(while I put down the html of the form), and will allow me to use a generic call in js for every form.

Sent from my iPhone

On Jan 1, 2015, at 5:59 PM, Joe Corcoran notifications@github.com wrote:

Okay, the sign in form: I can sort of see why it might be annoying that things break without the presence of the confirmation input. At the same time – why validate a password field on a sign in form? The user is either going to enter their own password correctly or not, I don't see the value in giving them hints about length and presence. On a sign up form, sure, but when logging in? What's the point? Maybe a way of specifying which validators you care about would be useful in the general case. Something like

judge.validate(element, ['presence', 'length'], { valid: function() {}, invalid: function() {} }); might be good, but aside from that, I'm struggling to see how Judge is supposed to know the specifics of your UX requirements.

The multiple models thing sounds a bit worrying. I suspect it's something specific to your app, but I'd love to see a minimal reproduction of the problem in the form of a new Rails app, so we can talk about it more.

— Reply to this email directly or view it on GitHub.

Dinuz commented 9 years ago

Ok @joecorcoran I set up the basic app, and I put it in https://github.com/Dinuz/JudgeTest (it's a public repository that you can access without problems).

All the issue that I talked about are replicated in the repository.

The basic app has a devise user model (the root page is the signin), and has another model called invite request together with its own controller (the only actions are new and create).

In the lib folder there are two files (one is a jquery plugin to run judge on form, and the other is just an extension for judge - a custom validator ajax style).

Looking forward to hearing from you....

I also saw you request for help (maintainers), and I will be happy to help out, but I should probably understand better the logic of your code (some sort of explanation would be great).

Best Dinuz

Dinuz commented 9 years ago

@joecorcoran Did you see the repository?

joecorcoran commented 9 years ago

Saw it, thanks! Haven't had much time this weekend though, will take a proper look as soon as I can.

On 3 January 2015 at 16:33, Massimiliano notifications@github.com wrote:

@joecorcoran https://github.com/joecorcoran Did you see the repository?

— Reply to this email directly or view it on GitHub https://github.com/joecorcoran/judge/issues/45#issuecomment-68598410.

Dinuz commented 9 years ago

Joe, I just want to point out(and maybe this is not the case), but rails with the version 4 changed completely how json is handled. Could be this an issue?

jamesmk commented 9 years ago

@Dinuz I reported the confirmation issue as well and recently submitted a PR (https://github.com/joecorcoran/judge/pull/55) to move confirmation validation to the confirmation field. This should resolve one of your issues here.

I'm having a harder time understanding your multiple models issue. I looked at your repo and noticed validations for :email on both models are very similar. What specific part of data-validate on the sign in form do you feel is getting polluted by InviteRequest?

thanks

joecorcoran commented 8 years ago

Closing this – it's been open for a long time with no activity.