judgegem / judge

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

Validation executing twice for 'String' types #39

Closed goldnuggets24 closed 9 years ago

goldnuggets24 commented 10 years ago

Your gem is fantastic and working well on my Rails 4 app. However, I noticed that it's printing error messages twice on String types. I have a text field where I am using this Gem for website validation for instance, and it is returning 'is invalid is invalid' on running 'Judge.validate' only one time. Is there a solution for this? Thanks, and thank you for a fantastic product!

danflynn76 commented 10 years ago

I'm having the same issue no matter what field's type is. I am also on Rails 4.

goldnuggets24 commented 10 years ago

I actually have a fix in my JS. Not sure if this will help, but it's working well for my validations. IF it works for you, awesome!

    $("form input").keyup(function(event) {
      var id, messages, queue, validations;
      $(".messages").remove();
      id = event.target.id;
      queue = new judge.ValidationQueue(document.getElementById(id));
      validations = queue.validations;
      messages = validations[0].messages;
      return $(this).after("<span class='messages'> " + messages + "</span>");
    });
    return;
joecorcoran commented 9 years ago

Was this an issue with Judge or with the event binding? Would like to know if I can close this or if I need to investigate a bug. A minimal reproduction would be great.

goldnuggets24 commented 9 years ago

Just an issue with the event binding. The gem itself is working excellent in my Rails 4 env.

On Tuesday, November 18, 2014, Joe Corcoran notifications@github.com wrote:

Was this an issue with Judge or with the event binding? Would like to know if I can close this or if I need to investigate a bug. A minimal reproduction would be great.

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

Mike

joecorcoran commented 9 years ago

Cool, will close this. :+1:

Dinuz commented 9 years ago

@joecorcoran

First of all thank you for the great gem, and the awesome work that you done until now. I really love the philosophy behind this gem, and the fact that is not tied with a strict front-end structure as other gem are.

Unfortunately I am having the same identical issue. The fix provided by @goldnuggets24 works only for the first validation rule, the other validations on the same field are simply ignored.

I am not sure if this is a bug or it's just how the judge gem is suppose to perform.

I am going to post a simple case that easily can reproduce the issue(if it is this a real issue anyway).

SIMPLE CASE

(A) Rails 4 model:

class InviteRequest < ActiveRecord::Base
  validates :email, :presence => true #{:message => 'Please enter your email address first.'}
  validates :email, :format => {:with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i} #{/\w+([-+.']\w+)*@\w+([-.]\w+)*\.\w+([-.]\w+)*/i, :message => 'This email doesn\'t look right.' }
end

(B) Jquery Plugin to wrap the judge gem (THIS IS A PIECE OF THE PLUGIN PROTOTYPE PART)

init: function() {
        _self = this;
        this._bindEvents();
},

_bindEvents: function() { 
        this.$el.on('blur', '[data-validate]', function() {
                        return _self._validateInput(this);
        });
},

_validateInput: function(element) {
        judge.validate(element, {
            valid: function(element) {
                        console.log('YEAHH');
                     },
            invalid: function(element,messages) {
                        console.log(messages);
                     } 
        });
   }

Note: the same behaviour is present just writing a pure js functionality to check the valid invalid state with judge.

(C) THE ISSUE

What seems to me an issue, and I repeat maybe it's just how the gem is suppose to work, is the fact that the validation seems to be run multiple times. If in my model I have 1 validation, judge perform one validation, if I have 2 validation, it perform the two validation two times, if 3 3 times.

As you can simply see in a dev console(in my case with 2 validation on the same field):

aa

The line 207 is exactly the line in the invalid callback where i put the console.log. As you can see it is executed two times as two are the validation present in my model, and that would not be weird if the messages were just single message (one for each validation). Instead what happen is that the messages (array containing all the validation error messages) is returned the two times.

Please let me understand if I want filter the error messages, and let my app display only the first error that is present in the array, what i need to do?

I would love for example throw out just the error of the specific validation that is failing, not all the errors.

Thank you for every explanation.

Best Dinuz

Dinuz commented 9 years ago

@joecorcoran

Another weird thing that I just saw is that judge add to my format validation also a blank message.

Just look the picture below, why there is a blank message (a presence validation) inside of a format validation?

cc

It should be just a format (Invalid message non a blank one too) in the validator.

goldnuggets24 commented 9 years ago

Just wanted to chime in again... you are absolutely right that my fix is only returning a fix that will allow you to execute the first error message in the error messages array, thus removing access to any other validations you may have placed inside your model.

This can be fixed by adding an iterator to your JS. Something like this should return all available error messages one time only (untested):

    $("form input").keyup(function(event) {
      var id, queue, validations, validationsLength;
      id = event.target.id;
      queue = new judge.ValidationQueue(document.getElementById(id));
      validations = queue.validations;
      var validationsLength = validations.length;
for (var i = 0; i < validationsLength; i++) {
    alert(validations[i].messages);
    });
    return;

I realize this doesn't solve the main issue here, but I did want to chime in if it helps you at least solve the problem enough to work with it for now.

Dinuz commented 9 years ago

@goldnuggets24 Thank you so much, I am going to try your solution to test it. Anyway I patched my code in order to work with it for now, but for what I can infer, the multiple validation is an internal judge issue. I asked directly to @joecorcoran if he could kindly give some explanation (maybe this is the way in which the gem is suppose to work, and I am just approaching the thing in a wrong way).

What really is suspicious is the fact that also the validation message in the data attribute are kind of mess up, and not in line with what I read in the Readme of the gem. Again, if this is my misunderstanding I apologize, but I would just love to understand eventually why.

Thanks Dinuz

goldnuggets24 commented 9 years ago

I totally hear you. I'm on the same page as you. I think the gem is wonderful and I may not be understanding correctly, but I would also love some more details on why were finding some of these issues.

On Monday, November 24, 2014, Massimiliano notifications@github.com wrote:

@goldnuggets24 https://github.com/goldnuggets24 Thank you so much, I am going to try your solution to test it. Anyway I patched my code in order to work with it for now, but for what I can infer, the multiple validation is an internal judge issue. I asked directly to @joecorcoran https://github.com/joecorcoran if he could kindly give some explanation (maybe this is the way in which the gem is suppose to work, and I am just approaching the thing in a wrong way).

What really is suspicious is the fact that also the validation message in the data attribute are kind of mess up, and not in line with what I read in the Readme of the gem. Again, if this is my misunderstanding I apologize, but I would just love to understand eventually why.

Thanks Dinuz

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

Mike

Dinuz commented 9 years ago

Well lets wait what joe will say:)

Sent from my iPhone

On Nov 24, 2014, at 2:56 PM, Mike Levine notifications@github.com wrote:

I totally hear you. I'm on the same page as you. I think the gem is wonderful and I may not be understanding correctly, but I would also love some more details on why were finding some of these issues.

On Monday, November 24, 2014, Massimiliano notifications@github.com wrote:

@goldnuggets24 https://github.com/goldnuggets24 Thank you so much, I am going to try your solution to test it. Anyway I patched my code in order to work with it for now, but for what I can infer, the multiple validation is an internal judge issue. I asked directly to @joecorcoran https://github.com/joecorcoran if he could kindly give some explanation (maybe this is the way in which the gem is suppose to work, and I am just approaching the thing in a wrong way).

What really is suspicious is the fact that also the validation message in the data attribute are kind of mess up, and not in line with what I read in the Readme of the gem. Again, if this is my misunderstanding I apologize, but I would just love to understand eventually why.

Thanks Dinuz

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

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

Dinuz commented 9 years ago

@goldnuggets24

I think I found where the issue is and what generates it. I believe that the callbacks valid, or invalid, are called multiple times (the judge validation is a queue system). There was no way to let the gem work as I wanted, without putting my hands in the code.

I then prefered digging around and testing different options (and actually your code lighted up my bulb:))

I used the same scheme of before except that this time I used the judge.validate with just one callback that actually doesn't do absolutely nothing except returning the result of the judge.validate, and use the result in another function checkValidateResult, in which first I map the result (.map) and after I search (.find) on the mapped object for a msg.length>0. In this way I can easily after perform an if else check, and show valid or invalid (actually here valid and invalid will change the css structure and show the error or the valid). This method allow me to show only one error msg at the time, and dynamically change it while typing or bluring or changing the input value. The method return only one msg in case of an error due to the .find method that just stop after the first result returning it. This means that If I have multiple validations on the same input, the method will just return the first failing validation message.

_validateInput: function(element) {
  var _result = judge.validate(element, function() {
    return;
  });

  _that._checkValidateResult(element,_result);
}

I wrote a plugin to use in my whole app, and it allow to apply css for valid or invalid, and moreover show a message in case of error or an hint when focusing on the specific input.

I remain very open to discuss any other issue with you, and more than happy to talk and chat with @joecorcoran about all this. The gem works very well, but if this is the way in which is suppose to work, the example in the readme section is very confusing, and maybe we should add another example.

Anyway Happy Thanksgiving Guys

joecorcoran commented 9 years ago

Okay, reopening this as it looks like a real issue. I'm looking into it.

joecorcoran commented 9 years ago

Take a look at the pull request above – does it fix this issue?

Dinuz commented 9 years ago

@joecorcoran That was exactly what I thought to do if you were not answering:) I didn't want touch your code without hearing what was your opinion.

Yes the pull request that you placed above fix the issue (at least for the simple validations that I am running now).

For what concern the regex validation, your code present an incongruence with rails, the way in which the regex are defined in js and in rails are a bit different.

Someone else already proposed this pull request, but I didn't see implemented yet in your code, and I added to my judge (otherwise there is no way of running a regex format validation):

var convertRegExp = function(string) {
    var parts  = string.slice(1, -1).split(':'),
    flags  = parts.shift().replace('?', ''),
    //source = parts.join(':').replace(/\\\\/g, '\\');  // THIS LINE IS REPLACED WITH THE FOLLOW:
    source = parts.join(':').replace(/\\\\/g, '\\').replace('\\A', '^').replace('\\z', '$');
    return new RegExp(source, convertFlags(flags));
 };

Thanks for the patch in the code:) Hope anyway that I was useful in the bug identification:)

Cheers Dinuz

goldnuggets24 commented 9 years ago

This pull request is fantastic. Seriously thank you so much for taking care of this, and for taking care of it so quickly! I'll take a closer look at it later this weekend but it's working great in my codebase.

On Saturday, November 29, 2014, Massimiliano notifications@github.com wrote:

@joecorcoran https://github.com/joecorcoran That was exactly what I thought to do if you were not answering:) I didn't want touch your code without hearing what was your opinion.

Yes the pull request that you placed above fix the issue (at least for the simple validations that I am running now).

For what concern the regex validation, your code present an incongruence with rails, the way in which the regex are defined in js and in rails are a bit different.

Someone else already proposed this pull request, but I didn't see implemented yet in your code, and I added to my judge (otherwise there is no way of running a regex format validation):

var convertRegExp = function(string) { var parts = string.slice(1, -1).split(':'), flags = parts.shift().replace('?', ''), //source = parts.join(':').replace(/\/g, '\'); // THIS LINE IS REPLACED WITH THE FOLLOW: source = parts.join(':').replace(/\/g, '\').replace('\A', '^').replace('\z', '$'); return new RegExp(source, convertFlags(flags)); };

Thanks for the patch in the code:) Hope anyway that I was useful in the bug identification:)

Cheers Dinuz

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

Mike

joecorcoran commented 9 years ago

Alright great, I'm gonna close this issue and release a new patch version. Let's keep the regexp issue in one place and not here.

Dinuz commented 9 years ago

That sounds great!

I agree on leaving the reflex patch in another place, I wanted just remind you to include it in the new release!

Also check please what the other guy was saying about the singleton...look like that is the reason if I am not wrong issue #24 https://github.com/joecorcoran/judge/issues/24#issuecomment-64962861

This just in order to release a single patch update:)

Great job btw Joe

Sent from my iPhone

On Nov 30, 2014, at 2:03 PM, Joe Corcoran notifications@github.com wrote:

Alright great, I'm gonna close this issue and release a new patch version. Let's keep the regexp issue in one place and not here.

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