Hiding the Password Strength progress when the field is empty #910

jloveland · 2015-09-13T20:38:35Z

Renaming strength meter to requirements meter, hiding when password field is empty, and refactoring directives to use $validators, adding tests

jloveland · 2015-09-13T20:42:36Z

@mleanos

mleanos · 2015-09-13T22:39:54Z

@jloveland This is what I had originally changed to hide the indicator. However, I'd rather be checking the passwordDetails.newPassword model. Could you explain what undesired behavior exists, if you return password from here? https://github.com/meanjs/mean/blob/master/modules/users/client/directives/password-validator.client.directive.js#L33
I ended up returning password & I couldn't see any negative side effects. I don't think for the purposes of this directive, that it should set the ngModel to be undefined. It could have a negative impact over something else relying on the model.

It was brought up to me by someone testing my app, that the wording "Password Strength" is misleading. This isn't really a standard strength indicator; it's a requirement meter. How do you feel about changing the wording? Maybe something along the lines of "Minimum Strength Requirement" ?

jloveland · 2015-09-14T02:01:25Z

@mleanos I agree. I was reviewing more docs and see that I can use $validators instead of $parse. This will leave the model in tact as the user types. I will push an update soon.

Also, do you think Password Requirements above the progress bar would be clear enough instead of Password Strength?

jloveland · 2015-09-14T02:17:47Z

depending on whether we decide to change the name from Password Strength to Password Requirements, I can change the name of variables to match the language...

mleanos · 2015-09-14T07:29:44Z

@jloveland I think Password Requirements would be much more clear.

As for the model, try merely changing the returned value from undefined to password and play around with it. I did this & couldn't see anything else that behaved differently, other than the model being intact. You might see something that I don't. The required validator that is already implemented on that field should be sufficient.

jloveland · 2015-09-15T00:24:49Z

@mleanos I made some updates, renaming the strength meter to requirements meter. I am also hiding the requirements meter by checking the required validator only.

I am convinced that moving from the $parsers pipeline to the $validators pipeline is the best choice with Angular 1.3. see this blog for reference:
http://blog.thoughtram.io/angularjs/2015/01/11/exploring-angular-1.3-validators-pipeline.html

mleanos · 2015-09-15T00:47:11Z

modules/users/client/directives/password-validator.client.directive.js

+              { color: "primary", progress: "80"},
+              { color: "success", progress: "100"}
+            ];
+            var requirementsMax = requirementsMeter.length;


where is this being used? I don't see it.

good catch! it's not used, I will remove.

mleanos · 2015-09-15T02:53:01Z

@jloveland Looks great! I'm still concerned as to why the the ng-model for the password remains undefined until the validators pass... Can you explain that a bit to me? I think I'm missing something very fundamental.

My preference would be to not affect the ng-model & leave it intact, but for these purposes it's probably not a huge deal.

lirantal · 2015-09-15T07:02:44Z

@ilanbiala I added you to review this one

ilanbiala · 2015-09-15T16:42:30Z

@lirantal are you okay with the change? If so, then @jloveland please add tests. I'd prefer from now on to prevent bugs and issues that we require tests with any PRs, because it already helps to make reviewing easier and already shows some validity of the code.

ilanbiala · 2015-09-15T16:43:56Z

modules/users/client/directives/password-verify.client.directive.js

-              if (origin !== viewValue) {
-                modelCtrl.$setValidity("passwordVerify", false);
-                return undefined;
+              if (origin !== password) {


Can we just clean this up to be a ternary op?

ilanbiala · 2015-09-15T16:44:43Z

@jloveland another thing, I remember @rhutchison doing something with ng-messages. @rhutchison was that cleaner?

lirantal · 2015-09-15T20:46:47Z

I'm ok with this.
I'll also add that if you are submitting a PR please add screenshots, which is extra important when you are doing frontend PRs. If I did that for my server-side PRs then you should have no problems doing it for frontend.

jloveland · 2015-09-16T00:32:35Z

@mleanos, ngModel = undefined is the default behavior by design for the $validators pipeline. Please review this issue: angular/angular.js#1412

jloveland · 2015-09-16T00:39:44Z

@ilanbiala and @rhutchison I added the errors returned from password-validator within the ng-messages block. for example:
https://github.com/meanjs/mean/blob/master/modules/users/client/views/authentication/signup.client.view.html#L40
do you see anything wrong with this approach?

mleanos · 2015-09-16T01:04:01Z

@jloveland Thanks for the reference. I think for the purposes of this exact implementation (used for Password validation), we can go with it.

However, if we implement $validators somewhere else in the project, we should pay attention to this behavior. I can see some cases where the ngModel value would need to be retained even when not valid.

This LGTM, and is ready to be merged from my perspective.

mleanos · 2015-09-16T01:10:36Z

modules/users/client/directives/password-verify.client.directive.js

              }
-            });
+            };
          }
        });
     }


@jloveland Slight indentation issue.

mleanos · 2015-09-16T01:18:16Z

For reference.. If the behavior of the ngModel being set to undefined is undesired, we could use the allowInvalid ng-model-options setting.

ng-model-options="{ allowInvalid: true }"

This will allow the ng-model to retain the value, while still marking the model as invalid from the $validator. Again, I don't think this option is needed for this particular case. But wanted to drop this here for anyone coming along later.

jloveland · 2015-09-21T18:15:04Z

@ilanbiala I am trying to develop karma and e2e tests for this. I am seeing this error with protractor: Error: Angular could not be found on the page http://localhost:3000/authentication/signin : retries looking for angular exceeded. Is this a bug with mean.js? or am I doing something wrong?

ilanbiala · 2015-09-21T22:11:27Z

@jloveland I think you have something set up incorrectly. In a separate commit, can you add what you have so far for tests?

ilanbiala · 2015-10-04T14:58:37Z

@jloveland is the testing issue fixed with the PR you have open right now?

jloveland · 2015-10-05T00:54:21Z

@ilanbiala e2e testing issues have been resolved.
I am planning on adding unit tests for the directives to this PR.
Should I also add e2e tests here? or would it be easier to review if I added those e2e tests to a different PR?

ilanbiala · 2015-10-05T02:38:09Z

@jloveland add all the tests to this PR.

jloveland · 2015-10-09T17:42:09Z

package.json

@@ -110,6 +110,7 @@
    "karma-ng-html2js-preprocessor": "^0.1.2",
    "karma-phantomjs-launcher": "~0.2.0",
    "load-grunt-tasks": "^3.2.0",
+    "protractor": "git://github.com/angular/protractor.git#master",


for some reason, this isn't doing the trick with node v.10 even though this PR was thought to fix the issue.
https://github.com/angular/protractor/pull/2591/files
https://travis-ci.org/angular/protractor/jobs/84351567

Anyone have thoughts on this?

@ilanbiala I am seeing that travis is failing to run using node v0.10. it looks like the error is due to node_modules/grunt-protractor-runner/node_modules/protractor/lib/configParser.js:102:29) which seems to be an embedded protractor version..so, I suppose we need to wait until grunt-protractor-runner upgrades or we can use my fork..not sure what the best option is..

ilanbiala · 2015-10-09T17:48:22Z

@jloveland what do you mean?

jloveland · 2015-10-09T18:14:57Z

@ilanbiala sorry for all the chatter here..never mind, I was looking at an old build...looks like all tests pass

ilanbiala · 2015-10-09T18:21:26Z

@jloveland just curious to see if any functionality wasn't covered in the tests, do you think you could try adding https://github.com/karma-runner/karma-coverage to Grunt and Gulp in a PR so we can verify?

jloveland · 2015-10-09T18:38:24Z

@ilanbiala I am also curious about that. I can add another PR for karma coverage.

I was having issues writing unit tests for the directives. For the password-validator and password-verify directives, I am having trouble determining how to mock the services that the directive depends on...also, it doesn't seem to me that the directives are being loaded in the tests. I added e2e tests instead of unit tests because of the challenges I am having with figuring out how to unit test the directive that is dependent on services..

ilanbiala · 2015-10-09T22:51:23Z

@jloveland let's start with a PR for karma/e2e coverage, and then we can see specifically what else needs to be tested and how to rework the tests so they are clean and intuitive.

mleanos · 2015-10-11T03:29:53Z

@jloveland Per our Gitter discussion...

I verified the directives are being loaded with the tests. However, just like you, I was experiencing the password-validator $parser wasn't executing. I had to manually execute the $parser function on the model, inside a test like so..

scope.userForm.password.$parsers[0](scope.credentials.password); - may not be the correct way to handle this, but at least it's a hint at what we need to do. Might want to put this into a function inside the test file for reuse.

I verified that scope.passwordErrors was being properly populated based on the validation.

Also, to answer your question about whether or not we should put this into a separate PR. I think so. I think we should put Directive tests into their own files, since they behave differently than the other tests. For instance, we could add a new test file to modules/users/tests/client called password-validator.client.directive.tests.js

Let me know what you think about this.

jloveland · 2015-10-14T02:03:18Z

Thanks to the huge help from @mleanos, I was able to add tests for the password-validator and password-verify directives.

@ilanbiala I have PR #985 with karma test coverage. Does anyone out there want to take on the e2e coverage PR?

…actoring directives to use $validators

mleanos · 2015-10-14T03:07:44Z

@jloveland I was happy to help out!

The tests look great. However, I did run into an issue with the E2E tests. I got the same error, as the build.
https://travis-ci.org/meanjs/mean/jobs/85249711#L715

All in all, this looks really good. +8.5 is nothing to sneeze at :) Great job!

jloveland · 2015-10-14T03:21:56Z

@mleanos thanks for reviewing. I am fixing the e2e test now..also rebasing..

mleanos · 2015-10-14T03:38:21Z

Everything is passing now! Thanks for all the hard work you put into this @jloveland Our test coverage is getting there.

Now that there's so many more E2E tests, it's entertaining to sit back & watch them run :)

LGTM

lirantal · 2015-10-15T13:39:52Z

Good job guys 👍

jloveland · 2015-10-17T00:44:39Z

I believe this is ready to merge.

codydaig · 2015-10-17T03:23:57Z

LGTM 👍

Hide the password strength progress when the field is empty

jloveland force-pushed the hide-password-validator branch from b981798 to 675e5bd Compare September 14, 2015 02:15

jloveland force-pushed the hide-password-validator branch from 675e5bd to 422cfa6 Compare September 15, 2015 00:05

mleanos reviewed Sep 15, 2015
View reviewed changes

lirantal assigned ilanbiala Sep 15, 2015

lirantal added this to the 0.4.2 milestone Sep 15, 2015

lirantal added the enhance:feature label Sep 15, 2015

ilanbiala reviewed Sep 15, 2015
View reviewed changes

mleanos reviewed Sep 16, 2015
View reviewed changes

modules/users/client/directives/password-verify.client.directive.js

}

});

};

}

});

}

Copy link

Member

mleanos Sep 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jloveland Slight indentation issue.

jloveland force-pushed the hide-password-validator branch from 422cfa6 to 94f23f2 Compare September 16, 2015 03:03

jloveland mentioned this pull request Sep 23, 2015

E2E tests not executing #929

Closed

jloveland force-pushed the hide-password-validator branch from e40241c to a96f094 Compare October 8, 2015 01:25

jloveland mentioned this pull request Oct 9, 2015

Adding protractor options to fix travis failing #968

Merged

jloveland force-pushed the hide-password-validator branch 2 times, most recently from 67261c3 to 90a74fa Compare October 9, 2015 17:29

jloveland reviewed Oct 9, 2015
View reviewed changes

jloveland force-pushed the hide-password-validator branch from 90a74fa to c3386a5 Compare October 9, 2015 18:09

renaming strength meter, hiding when password field is empty, and ref…

f733efb

…actoring directives to use $validators

adding tests for directives

6514044

jloveland force-pushed the hide-password-validator branch from a077494 to 6514044 Compare October 14, 2015 03:26

ilanbiala added a commit that referenced this pull request Oct 17, 2015

Merge pull request #910 from jloveland/hide-password-validator

cc486d5

Hide the password strength progress when the field is empty

ilanbiala merged commit cc486d5 into meanjs:master Oct 17, 2015

Hiding the Password Strength progress when the field is empty #910

Hiding the Password Strength progress when the field is empty #910

Conversation

jloveland commented Sep 13, 2015

jloveland commented Sep 13, 2015

mleanos commented Sep 13, 2015

jloveland commented Sep 14, 2015

jloveland commented Sep 14, 2015

mleanos commented Sep 14, 2015

jloveland commented Sep 15, 2015

mleanos Sep 15, 2015

Choose a reason for hiding this comment

jloveland Sep 16, 2015

Choose a reason for hiding this comment

mleanos commented Sep 15, 2015

lirantal commented Sep 15, 2015

ilanbiala commented Sep 15, 2015

ilanbiala Sep 15, 2015

Choose a reason for hiding this comment

jloveland Sep 16, 2015

Choose a reason for hiding this comment

ilanbiala commented Sep 15, 2015

lirantal commented Sep 15, 2015

jloveland commented Sep 16, 2015

jloveland commented Sep 16, 2015

mleanos commented Sep 16, 2015

mleanos Sep 16, 2015

Choose a reason for hiding this comment

mleanos commented Sep 16, 2015

jloveland commented Sep 21, 2015

ilanbiala commented Sep 21, 2015

ilanbiala commented Oct 4, 2015

jloveland commented Oct 5, 2015

ilanbiala commented Oct 5, 2015

jloveland Oct 9, 2015

Choose a reason for hiding this comment

jloveland Oct 9, 2015

Choose a reason for hiding this comment

ilanbiala commented Oct 9, 2015

jloveland commented Oct 9, 2015

ilanbiala commented Oct 9, 2015

jloveland commented Oct 9, 2015

ilanbiala commented Oct 9, 2015

mleanos commented Oct 11, 2015

jloveland commented Oct 14, 2015

mleanos commented Oct 14, 2015

jloveland commented Oct 14, 2015

mleanos commented Oct 14, 2015

lirantal commented Oct 15, 2015

jloveland commented Oct 17, 2015

codydaig commented Oct 17, 2015