Skip to content
This repository has been archived by the owner on Jul 1, 2020. It is now read-only.

CheckFormValidity and validation-error-to #16

Closed
tlastad opened this issue Mar 30, 2015 · 21 comments
Closed

CheckFormValidity and validation-error-to #16

tlastad opened this issue Mar 30, 2015 · 21 comments

Comments

@tlastad
Copy link
Contributor

tlastad commented Mar 30, 2015

I use validation with validation-directive like this

input ... validation="required" validation-error-to="#displayError">

When I try to use

new validationService().checkFormValidity($scope.form) 

I do not get any error messages.
Removing validation-to-error works. But that does not help me. :)

I figured that it is because validation-common.updateErrorMsg is not instantiated with the correct values for self (specifically self.validatorAttrs) since I define it using validation-directive, but checkFormValidity is in validation-service, of which I have just instantiated a new empty instance.

I have made a hack in checkFormValidity lines 110-113 by changing the following:

if(!!elm && elm.length > 0) {
   ctrl.$setTouched(); // make the element as it was touched for CSS
   self.commonObj.updateErrorMsg(form.$validationSummary[i].message, {valid: false, elm: elm, submitted: true});
}

to

if(!!elm && elm.length > 0) {
   ctrl.$setTouched(); // make the element as it was touched for CSS
   elm.blur()
}

This I assume is not a good idea, but it will fire the original validators that were registered on the controls...
I am at a loss how to get it to work, or if it is even supposed to work like I want it to :)

tlastad

@ghiscoding
Copy link
Owner

Ah that is nice to see there's already some persons using the code I pushed in the weekend haha, so quick :)

The code that I made was actually to add a functionality that was requested by another user, I did not even try it in my own project yet (apart from this Git sample) and might not be bulletproof, it is at the very early stage (last weekend). My next step was to actually start looking at Protractor for the tons of checks for this kind of project, it is becoming a must with the tons of functionality that this project is starting to be hehe ;)

In the mean time, I am home for lunch and I made a very quick test and your patch might at the end, be a much better solution. I made a quick change and test, I'm using only use the elm.blur() that you suggested and it actually seems like a much better experience (without the long hack of updateErrorMsg that I made), I didn't think about using that way :)

Could you try to replace line 110-113 by the following piece of code:

if(!!elm && elm.length > 0) {
  elm.blur();
}

I won't have time to try everything out, lunch time isn't enough, but I would be curious to see if it work completely with your project. Let me know please... and if it does, I would be happy to have a pull request from your side since you found it :)

@tlastad
Copy link
Contributor Author

tlastad commented Mar 30, 2015

I'm off work now, so I won't have access to the code until tomorrow.
But I tried it earlier, an it needs ctrl.$setTouched(); in order for updateErrorMsg to do it's stuff fully...
I'll test it out tomorrow and do a pull request if it works.

@ghiscoding
Copy link
Owner

Cool thanks, I'll wait for the pull request update

@ghiscoding
Copy link
Owner

After some more testing, this end up not working (at least not with jqLite), it keeps telling me that elm.blur() is not a function. As for the explanation of my code... the reason of why I have put this function checkFormValidity() in the Service is simply because I cannot call it directly from the Directive so I thought it make sense to put it in the Service. Calling this function with an empty Service is not a problem ( I do it myself on line 48 of my app.js inside the Directive controller), this function use the $validationSummary which is outside of Service object.

BUT I believe that I found your problem and actually fixed it an hour ago with another code change that I pushed to correct an issue I found (a73505b)

I am assuming that your problem is because you do not have a name attribute on your form as you mentioned in the other pull request. So the change that I made an hour ago is to accept 2 types of arguments for the checkFormValidity() function, it can now take $scope alone or the $scope.formname. When you don't have a name on the form, your only option is to use the $scope alone so your code should be:

// before, this will not work without a form name
new validationService().checkFormValidity($scope.form)

// after, this will work with or without a form name
new validationService().checkFormValidity($scope)

The reason of why I accept both arguments is because I found that using $scope.formname.$validationSummary does not always work (not without a form name) so I fill the summary in 2 locations ($scope.$validationSummary and if form as a name I would then also fill $scope.formname.$validationSummary. This was exactly the problem you encountered in the other pull request #15 you made. I updated also the README, you can see that at link: https://github.com/ghiscoding/angular-validation#submit

I tried myself by removing the form name, then using the validation-error-to and use only the $scope as argument, it now works correctly for me. Let me know if that fixes your problem, please note that you will need to get my new code for that to work.

@tlastad
Copy link
Contributor Author

tlastad commented Mar 31, 2015

Thanks.
I tried it out and it works the same for me.

if(!!elm && elm.length > 0) {
  ctrl.$setTouched(); // make the element as it was touched for CSS
  elm.blur();         
}

works fine, but I'm running on jquery 1.11, not jqLite.

if(!!elm && elm.length > 0) {
  ctrl.$setTouched(); // make the element as it was touched for CSS
  self.commonObj.updateErrorMsg(obj.$validationSummary[i].message, {valid: false, elm: elm, submitted: true});  
}

does not work...

A small misunderstanding. My form has a name, but there are other forms on the page before my form.

document.querySelectorAll('form');
 --> 
[
 <form action="http://...." method="GET">, 
 <form action="http://...." method="GET" style="display: inline">, 
 <form name="formName" novalidate method="POST" class="ng-pristine ng-invalid ng-invalid-validation">
]

But I do think I might have some sort of scope problem (that's what I get for taking over someone elses project :) ).
$scope.$validationSummary does not exist, but $scope.formName. and $scope.formName.$validationSummary exists. So as long as I continue to send in new validationService().checkFormValidity($scope.formName) it works, but without validation-error-to="#error"
This might have something to do with the fact that I'm working on a pretty complex wizard.
That means one controller to work the step by step navigation, one controller for each view with the app and some services to bind them all together.
That means that the validation-directive is fired on each view, which mostly has it's own controller, but the service is fired from the wizard controller. It also doesn't help that we're all newbies in angular. So $scope is an elusive thing... :)

But I was thinking...
There are two main problems here:
One is that updateErrorMsg does not have all the data it needs to do it's job, and the second is that "communication" between the directive and the service is difficult.
We do have the $validationSummary that is outside of both, and accessible by both...

The reason that the elm.blur(); idea works is that each entire validator object is attached to it. How about making $validationSummary contain a list of all the validator objects with messages and status, and attach the blur event to indexes in $validationSummary?
That way all the data is available all the time. It shouldn't use much (any) more memory as the elm.blur(); event will just point to $validationSummary instead of keeping each event with all its data global...

@tlastad
Copy link
Contributor Author

tlastad commented Mar 31, 2015

And an added bonus:
With the wizard, new validators are added for each step. but if I step back, I cannot move forward again as I now have invalid fields in a coming step...
Example:
wizard has two steps, each with validators.
step 1, validator a and b -> validate them and move to step 2.
step 2, validator c is added -> go back to step 1.
step 1, is valid but move to step 2 is a no go as validator c that I added in step 2 previously is invalid :)

But with all the validators available in $validationSummary, I can remove validator c on "back button"...

This might be well out of scope for you, but it's an idea :)

@tlastad
Copy link
Contributor Author

tlastad commented Mar 31, 2015

I did a test and it seems to work for me.
This is just a basic test, and probably needs a lot of refinement :)

What I've basically done is add commonObj to $validationSummary, and then use that when I can. Since I've not touched the basic functionality of $validationSummary, it only contains invalid objects. That means I can wipe it when going backwards in the wizard.

validation-common.js:
From:

 function updateErrorMsg(message, attrs) {
      var self = this;
...
...
function addToValidationSummary(self, message) {
...
      // if message is empty, remove it from the validation summary
      if(index >= 0 && message === '') {
        validationSummary.splice(index, 1);
      }else if(message !== '') {
        var errorObj = { field: elmName, message: message};
...
...

To:

  function updateErrorMsg(message, attrs) {
      // attrs.obj - if set should be a commonObj, and can be self
      var self  = (!!attrs && attrs.obj) ? attrs.obj : this;
...
...
function addToValidationSummary(self, message) {
...
      // if message is empty, remove it from the validation summary
      if(index >= 0 && message === '') {
        validationSummary.splice(index, 1);
      }else if(message !== '') {
        var errorObj = { field: elmName, message: message, obj: self};     // Add self to summary
...
...

validation-service.js:
From:

 function checkFormValidity(obj) {
     ....
      // loop through $validationSummary and display errors when found on each field
      for(var i = 0, ln = obj.$validationSummary.length; i < ln; i++) {
        isValid = false;
        elmName = obj.$validationSummary[i].field;
        elm = angular.element(document.querySelector('[name="'+elmName+'"]:not([disabled]):not([ng-disabled]'));
        ctrl = angular.element(elm).controller('ngModel');

        if(!!elm && elm.length > 0) {
            ctrl.$setTouched(); // make the element as it was touched for CSS
            self.commonObj.updateErrorMsg(obj.$validationSummary[i].message, {valid: false, elm: elm, submitted: true});
        }
      }
      return isValid;
    }

To:

 function checkFormValidity(obj) {
   ...
      // loop through $validationSummary and display errors when found on each field
      for(var i = 0, ln = obj.$validationSummary.length; i < ln; i++) {
        isValid = false;
        // I've got it all now, so use it 
        elm = obj.$validationSummary[i].obj.elm;
        ctrl = obj.$validationSummary[i].obj.ctrl;

        if(!!elm && elm.length > 0) {
          ctrl.$setTouched(); // make the element as it was touched for CSS
          // send events commonObj to updateError
          self.commonObj.updateErrorMsg(obj.$validationSummary[i].message, { valid: false, obj: obj.$validationSummary[i].obj, submitted: true });
        }
      }
      return isValid;
    }

And last, my own navigationcontroller:

...
self.NextStep = function () {
  if (new ValidationService().checkFormValidity($scope.formName)) {
    service.nextStep();
  }
};
self.PreviousStep = function () {
  $scope.formName.$validationSummary = [];
  service.previousStep();
};
...

@ghiscoding
Copy link
Owner

Wow you've done a lot, and it looks promising. Let me start by explaining why I chose the Service and why I use the $validationSummary inside the checkFormValidity(), simply because in the Directive I have no other ways to trigger some changes to each inputs, but then I thought, well I have all the errors I want in the validation summary so I could use that to display my errors (in theory that would work in both Directive & Service). That was a brief introduction of why I chose to do it that way, you probably figured it out already but it's worth to know.

Now for the elm.blur() that probably works using jQuery but doesn't seem to work with jqLite, I would prefer to have all my code working with jqLite (even though in most of my project I use jQuery), the code is longer with jqLite but that way I don't need other external dependencies. On the other hand you could still use the blur without breaking my code, something like this:

if( typeof elm.blur() === "function() ) {
  elm.blur();
else {
  // regular code
}

Going further in your code suggestion and I believe that you are on the right path with the code change, I like since it doesn't break current code (adding a new piece to the summary object is perfect idea) and would work with jqLite too. I'm about done with my lunch break so I won't be able to test your code before the evening... as I said I like your code, but I just need to test it out with just the Service controller and make sure that it works in both cases. If you have the time to test the Service controller I made too, then I would be happy to have a pull request with the change.

As for your own navigationController (wizard), I would also be happy to have it as a sample, if you have time you can create the wizard and store it inside the /more-examples/ folder, there is already a dynamic example in there.

Thanks.. I will surely look at all your code tonight.

@ghiscoding
Copy link
Owner

By the way, in my design I decided to keep $validationSummary in 2 locations, the $scope.$validationSummary and the $scope.formname.$validationSummary, I strongly prefer the 2nd option with the formname but since some might not always a name on their form (might a dynamic form), then I chose to add it in the $scope as well, more options to everyone...

In your case, you said that you do have a name on it, but the summary does not appear in your $scope, probably because your $scope is new or re-initialize when you change controller? Just a thought...

I saw in your code the property submitted: true is that use in the updateErrorMsg or anywhere else? Just curious of what you do with it since I don't see any call to it...

@tlastad
Copy link
Contributor Author

tlastad commented Mar 31, 2015

Lets see: There's a lot of information in this thread now :)

  • I understand the reasoning for $validationSummary inside the checkFormValidity() and it works perfectly.
  • I can understand your wish to keep support of jqLite, and I've dropped the elm.blur(); method.
    It works better when I push commonObj to $validationSummary anyway.
  • The wizard... I'm working on an inherited first draft version of the wizard now, and we've already made a new version of it in another of our projects. I'll try to get approval to add a version to your samples when it's more complete. But it might take a while.
  • The reason I have problems with the $scope.$validationSummary variable is due to the way the wizard is built. $scope.$validationSummary is available in each view controller's scope, but not in the "wizard" controller scope. $scope.formName.$validationSummary however is visible throughout. So then I use that :)
pseudocode
<div ng-app="app" ng-controller="wizardController">
    <form name="formName" novalidate>
        <div ng-views>
           // several views with own controllers
           // the validation-directive is also invoked here, on elements in each view
           // the wizardController/wizardService decides which view to show and when to show it
        </div>
    </form>
</div>
  • submitted: true I think is when checkFormValidity calls self.commonObj.updateErrorMsg. And that I think is your code :)
  • I have pushed all my changes to tlastad/angular-validation/ so you can see them for yourself It's this commit. I don't really intend to keep them as is, but it's easter holiday here now and that means work is closed until tuesday 7th.
    Please disregard the added requiredselection validator and isValidationRequired function. That's just me with my head stuck a bit to deep in the sand...:)

This is your code. I'm just making some suggestions. If they work, and you want to include them, great! If not, such is life :)

@tlastad
Copy link
Contributor Author

tlastad commented Mar 31, 2015

Btw. is there a way to show a custom errormessage?
Say I have a couple of radiobuttons, and user has to select one.
It works if I set a name and make each required, But then I only get the message "Field is required".
I would like to get a different message.. i.e. "Please make a choice".
Would it maybe work to add a new property "validation-message='(key or string)'"
Hmm...

@ghiscoding
Copy link
Owner

Already Easter holiday, lucky you, I only have Friday off and it's just Tuesday here :P

Thanks for all the info, I will definitely take a look at your code tonight and tomorrow, and from there we will see what we can tweak. I already like what you have suggested so there is big potential to get something working :)

For the custom error message, I was actually thinking about creating another function in the Service that is completely independent from everything (the actual updateErrorMsg relies on the fact that you have a global, elm, scope, etc... which isn't always present). I am thinking about adding a new function in the Service that can be called independently, something like displayErrorMessage(obj) then you could easily call it like displayErrorMessage({ elmName: "input1", text: "your text"}) and since it's an object, I can grow it in the future with extra functionality, the only problem I can quickly see with this is that it might create multiple error messages after the input, unless of course we pass it an ID or a class name... or I could also create another Validator that could look like this required:t where (t) would be your text, look at the match:n,t Validator, it does exactly that.

@tlastad
Copy link
Contributor Author

tlastad commented Mar 31, 2015

Sounds good :)

Thinking about the possible displayErrorMessage(obj)... would that be "accessible" from validation-directive?
Again, I'm thinking from my perspecive.
Considering we actually have validation in several scopes/controllers, the easiest way for us to add validation seems to use the directive approach. That way the validators will be created as required when we load a new view, and destroyed when we change to another view. At the same time checkFormValidity() will prevent us from leaving an invalid view.

Look at it like this:
view1.html:
<input type="text" name="txt1" validation="required">
view1Controller.js:
do stuff for view1 "only"
view2.html:
<input type="text" name="txt2" validation="required">
view2Controller.js:
do stuff for view2 "only"

index.html

<div ng-app="app" ng-controller="wizardController as wiz">
    <form name="formName" novalidate>
        <div ng-views>
           // several views with own controllers
           // the validation-directive is also invoked here, on elements in each view
           // the wizardController/wizardService decides which view to show and when to show it
        </div>
        <button name="next" ng-click="wiz.nextStep">
        <button name="prev" ng-click="wiz.previousStep">
    </form>
</div>

wizardController.js

self.nextStep = function () {
  if (new ValidationService().checkFormValidity($scope.formName)) {
    service.nextStep();
  }
};
self.previousStep = function () {
  $scope.formName.$validationSummary = [];
  service.previousStep();
};

As you can see, all our developers need to think about when developing is to determine which field needs validation, and assign it where it's needed. WizardController will be part of our local framework, and will only need to be referenced in. No custom code for each wizard.
(I don't know how well I'm explaining this... :) )

Edit: I guess what I'm really thinking is that it would be 'messy' if we have to call displayErrorMessage(obj) from the viewControllers as they right now don't have anything to do with validation. Only the wizard and the views thamselves care about it. And only the wizard knows when we're changing view...

required:t will mean that required:t and match:n,t can have custom text, but no other validator.
I was picturing it more like

<input type="text" name="txt1" validation="required" >  //<-- default message
<input type="text" name="txt1" validation="required" validation-error-to="#displayError" > //<-- default message
<input type="text" name="txt1" validation="required" validation-message="Please do stuff">
<input type="text" name="txt1" validation="required" validation-message="Please do stuff" validation-error-to="#displayError">

I don't know how much more work it will be to do that, and I assume it will mean that the service will have to be extended as well...
I also don't know how that will fit into your ideas of how this should work.

Oh well... It's an idea :)

It's late here, and I'm off to bed. Have a nice evening :)

@ghiscoding
Copy link
Owner

I added it as you said, it's 3 lines of code and I pushed it online already (without a Revision tag), you can see it here 17f7216 (line 261-264 of common.js)
I was not hard to put in but it has some disadvantage (which might be irrelevant in your case if you use only 1 validator). The disadvantage is that the validation-message is global to the input, so if you have 3 validations on the same line well it would not know which validator is associated to your alternate... but in your case if you use only 1 validator (required) then it's working fine. For example

<!-- If any of the 3 validators fail, it will always display "Please do stuff" in all cases -->
<!-- so you will not be able to know if your text is too short or too long, etc... -->
<input type="text" name="txt1" validation="min_len:15|max_len:255|required" validation-message="Please do stuff">

While the alternate text would have the advantage of being associated to that particular validator. There is no limitation in how many validators you put with alternate text, it just looks funny on your definition of it though because it becomes very long... For example:

<input type="text" name="txt1" validation="min_len:15|max_len:255|required:Please do Stuff|match:input2:Your password does not match">

As you can see, it's more flexible this way, it's just longer text within the validation attribute. I did not implement it yet, it's much more code implication so maybe over next weekend. If I ever do that one, I would rather do this feature for all validators, not just on the required, but I might have to use another way of passing the text. Maybe something like this validation="between_len:5,255,alt=Between Text|required:alt=Your other text" with passing alt= to the validator. Each validator are totally independent and if we can pass additional properties to it, it's just an extra property to the same validator but I would probably have to recode my validation-rules and validation-common for that implementation (if I do it).

Happy Easter

@tlastad
Copy link
Contributor Author

tlastad commented Apr 1, 2015

Yeah.. I did not think that through, did I :)
I didn't think about multiple validators on same control...
validation="between_len:5,255,alt=Between Text|required:alt=Your other text"might be the best option all things considered. It allows for alternate text, but makes it clear that it's an option.
The fact that it might make it look funny for a long definition does not really matter. There's a lot of funny markup out there. :) Also, In most cases I think maybe one or two validators might have alt text, while the rest uses standard text.
Also, I guess in most cases it would maybe look like this?
validation="between_len:5,255|required:alt={{altReq | translate}}|match:input2:{{altMatch | translate}}"

@ghiscoding
Copy link
Owner

Nice suggestion validation="between_len:5,255|required:alt={{altReq | translate}}|match:input2:{{altMatch | translate}}" but I just wondered if this will work or fail. If the translate happens before validation is interpreted then yes it will work

but if the translate happens after my interpretation then it would fail, the reason is that I already use the pipe | for splitting the validators, so in this case it would try to use required:alt={{altReq as a validator, then translate}} as another validator... so it's a matter of timing, I will look into having the alt= first and we'll see what happen after.

As for your other code, I tried it in the Service and it doesn't work properly but it's related to the way I coded the commonObj and the Service object. I get issues that adding the object inside the $validationSummary is always taking the last element object of the form (for example, let say you have input1 to input15, copying the elm and ctrl objects into each validationSummary end up being input15 references for all of them, while on the Directive it works correctly). I know the reason and I think that I have another option, I have to change a bunch of code for that but it should work. I will implement a new variable in the common for holding all the form elements, not just the validation summary (like this var formElements = [{ field: "input1", valid: false, elm: elm, ctrl: ctrl}] this part is already coded and working) then after that you could simply loop through them for the checkFormValidity and that would work better compare to just the $validationSummary. I will work on that Friday since I will be off work :)

Hopefully will learn Protractor or Jasmine sometime soon, since there is so much to test under this project that is growing by the weeks..haha

Talk later, thanks for the feedback and enjoy your Easter :)

@ghiscoding
Copy link
Owner

I think that after a lot of code change and fixes, I made everything working and more. I took your concept of using the object within the checkFormValidity() but I saved the object at the early stage instead of validationSummary, that seems to work exactly like it should have been at the beginning. Try it out with your wizard and let me know if it works or not.. When I did the commit it closed this thread but I can re-open if need be. If you get other problems, you could also post a new thread since this one is quite long now.

Some new functions have been added that might interest you in the commonService, there is 3 new ones: getFormElements(), getFormElementByName(), removeFromValidationSummary() and all of them are used directly in the validationService. The FormElement is something new that I added to keep the object (as your concept idea), it holds the { elm, ctrl, attrs, isValid and message } of each element.

I also added the alternate text, the translate also work with it, you can check the README in the "available validators" section I am explaining how to use it, the html template also has 1 alternate text in both the Directive and Service, you can check each of them, it's on the <select>. I am quite happy with the functionality. The Plunker was updated too.

Hope that everything works for you... You could also favorite my project in Github ;)

See ya

@tlastad
Copy link
Contributor Author

tlastad commented Apr 4, 2015

Wow, you've been busy :)
I look forward to checking it out on Tuesday.
Now enjoy the rest of your easter holiday :)

@ghiscoding ghiscoding reopened this May 6, 2015
ghiscoding added a commit that referenced this issue May 6, 2015
- Added option to display only last error message instead of all
messages at once
- Added new template to display ngRepeat examples, also help
troubleshooting the follwoing bug.
- Fixed a bug (probably linked to issue #16) where changing route on
View/Controller would make the ValidationSummary fail when coming back
to original View/Controller, this bug was associated to the fact that
the ValidationSummary kept growing from Controller to Controller, now
this ValidationSummary is wipe out as soon as we detect a route change.
@ghiscoding
Copy link
Owner

Hello again, been a long time... I think that I finally found and fixed the original bug you had, I just happen to find it out the hard way. I found out that by changing to another View/Controller the validations would work on both but then coming back to the original View/Controller would make my ValidationSummary misbehaving, after troubleshooting for some hours, I found out the cause being that I never cleared up these 2 important array variables (formElements & validationSummary) and so they kept growing and growing and holding values of previous controllers. As soon as I found out, I put in place a reset a of these 2 variables whenever a route change occurs and that fixed my problem.

This is the code that I added in the validation-common.js:

// watch on route change, then reset some global variables, so that we don't cary over other controller/view validations
$rootScope.$on("$routeChangeStart", function (event, next, current) {
    if (!bypassRootScopeReset) {
        formElements = [];        // array containing all form elements, valid or invalid
        validationSummary = [];   // array containing the list of invalid fields inside a validationSummary
    }
});

In case the last piece of code is giving you problems, I also added a function to let you bypass this new reset that is now default, you can call the bypass through the service object

new validationService().setBypassRootScopeReset(true);

And finally, I believe that the function clearInvalidValidatorsInSummary() you created in the validation-service.js is no longer needed (but no worries I will leave it there). I strongly suggest you to try the new version 1.3.23 of my code that I pushed and remove your call to your clearInvalidValidatorsInSummary().

I have troubleshoot my problem with the new template I added to show ngRepeat, trying the first time worked but then going to another template and coming back to this ngRepeat would fail on any subsequent try. So I really believe that my new version of 1.3.23 is fixing your original problem and I referenced it in the commit :)

I have reopened this issue and would be really happy to get some feedback. If you have no time to test it out, no worries I will close the issue in a week or so. Thanks again!

@tlastad
Copy link
Contributor Author

tlastad commented May 6, 2015

Hi.

Good job.
I've removed my usage of clearInvalidValidatorsInSummary, and it works just fine.
So I guess it's safe to remove clearInvalidValidatorsInSummary.:)

@ghiscoding
Copy link
Owner

Awesome, thanks for the feedback, I will leave the function there, it's just couple lines of code.
Thanks for the quick response :)

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

No branches or pull requests

2 participants