Fixes #21170 - move password strength meter logic to react #4882

amirfefer · 2017-10-02T17:54:25Z

password_strength.js uses jquery_pwstrength_bootstrap wrapper
I've added react-password-strength - react component, which renders password input with integrated password strength meter by using zxcvbn to calculate a password strength score.
In addition, I moved verify password logic to wepback (it was in password_strength file as well)
Tests are missing for now.

after update:
Try online with Stroybook

before update:

theforeman-bot · 2017-10-02T17:54:25Z

There were the following issues with the commit message:

bb7d2ce must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

houndci-bot · 2017-10-02T17:54:37Z

webpack/stories/index.js

@@ -14,6 +14,7 @@ import PowerStatusInner from
 import Store from '../assets/javascripts/react_app/redux';
 import Toast from '../assets/javascripts/react_app/components/toastNotifications/toastListitem';
 import StorageContainer from '../assets/javascripts/react_app/components/hosts/storage/vmware';
+import PasswordStrength from '../assets/javascripts/react_app/components/passwordStrength'


Missing semicolon semi

houndci-bot · 2017-10-02T17:54:37Z

webpack/assets/javascripts/foreman_users.js

+    else {
+      form.removeClass('has-success has-error');
+    }
+  }


Closing curly brace does not appear on the same line as the subsequent block brace-style

houndci-bot · 2017-10-02T17:54:38Z

webpack/assets/javascripts/foreman_users.js

+    if (password !== '') {
+      html = __('Password match');
+      form.removeClass('has-error').addClass('has-success');
+    }


Closing curly brace does not appear on the same line as the subsequent block brace-style

houndci-bot · 2017-10-02T17:54:38Z

webpack/assets/javascripts/foreman_users.js

@@ -47,3 +47,25 @@ export function testMail(item, url, param = {}) {
    }
  });
 }
+
+export function verifyPassword() {


Function 'verifyPassword' has too many statements (12). Maximum allowed is 10 max-statements

amirfefer · 2017-10-03T14:13:45Z

@Rohoover What do you say about the color scheme?

Rohoover · 2017-10-03T14:57:46Z

I think it jumps from orange to green. Maybe a yellow in between for

Weak - Red
Normal - Orange
Medium -Yellow
Strong -Green
Very Strong - Dark Green

waldenraines · 2017-10-03T15:10:34Z

app/views/users/_form.html.erb

@@ -41,8 +40,13 @@
        <% if User.current == @user  %>
          <%= password_f f, :current_password, :label => _('Current password'), :placeholder => '' %>
        <% end %>
-        <%= password_f f, :password, :label => _('Password'), :error => @user.errors[:password_hash] %>
-        <%= password_f f, :password_confirmation, :label => _('Verify') %>
+        <div class="clearfix" id="password_react"></div>


Is the .clearfix needed here?

waldenraines · 2017-10-03T15:12:57Z

webpack/assets/javascripts/react_app/components/passwordStrength/index.js

+
+    return (
+      <div className={`form-group ${error.length > 0 ? 'has-error' : ''}`}>
+        <label className="col-md-2 control-label"> { __('Password') } </label>


This label should include a for attribute to tie it to the input below for accessibility.

I suggest to use the CommonForm component instead of redefining it here (https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/components/common/forms/CommonForm.js)

waldenraines · 2017-10-03T15:17:34Z

webpack/assets/javascripts/react_app/components/passwordStrength/password_strength.scss

@@ -0,0 +1,10 @@
+.ReactPasswordStrength-input {
+  width: 100% !important;


Can we style this without the !importants? Maybe add additional classes or elements to the selector if you need more specificity.

ohadlevy · 2017-10-03T18:35:23Z

webpack/assets/javascripts/react_app/components/passwordStrength/index.js

+        </div>
+        { error.length > 0 &&
+        <span className="help-block help-inline">
+          <span className="error-message"> {error[0]} </span>


could there be more than one error? should we only display the first one?

houndci-bot · 2017-10-18T16:12:47Z

webpack/assets/javascripts/react_app/components/common/forms/CommonForm.js

@@ -25,4 +25,4 @@ const CommonForm = ({
  );
 };

-export default CommonForm;
+export default CommonForm;


Newline required at end of file but not found eol-last

houndci-bot · 2017-10-18T16:12:47Z

webpack/assets/javascripts/react_app/components/common/forms/CommonForm.js

@@ -25,4 +25,4 @@ const CommonForm = ({
  );
 };

-export default CommonForm;
+export default CommonForm;


Newline required at end of file but not found eol-last

amirfefer · 2017-10-18T16:28:40Z

@waldenraines I've mentioned your comments.
@ohadlevy, I've switched verify password validation logic from jquery to react

I've changed color pattern to @Rohoover suggestion

waldenraines

Looks good to me!

amirfefer · 2017-10-19T20:34:27Z

@Rohoover
thoughts about color scheme , text placement ?

ares · 2017-10-20T08:42:36Z

Copying out notes from UX demo with @Rohoover about this:

Password authorization - Weak/Normal/Strong visualization would not pass 528 compliance for those visually impaired. The colors associated with each strength level seems mismatched. For example: Normal is red. A red status as well as form field highlight indicates an error. Weak is grey, and also possibly not high enough contrast for the visually impaired. The solution may be to simplify the password strength to text only, “Weak Password, Strong Password”. Also - greater thought or discussion could be had around “Normal” or “Strong”. If a “Normal” indicator is shown, does this actually change user behavior in the same way “Weak” does? Perhaps only “weak” is needed.

While I think most of it is addressed and it's a great improvement already, I think there are some other things that could be considered while it's being touched.

If messages include icons, it would solve the coloring issue.
fonts seems to be different for pass match and pass strength (italic, different size)
"too short" should be perhaps "Too short" to be consistent
does medium password still get accepted?

Also when this gets in, please update/close http://projects.theforeman.org/issues/20740 which tracked this originally :-)

sharvit

Good to me! 👍

ohadlevy · 2017-12-25T08:52:11Z

@amirfefer please rebase.

ohadlevy

Initial review looks good - I've added a few comments inline.

Please also adds action/reducers tests.

Thanks!

ohadlevy · 2017-12-25T08:59:43Z

webpack/assets/javascripts/react_app/components/user/passwordStrength/passwordStrength.test.js

+  passwordNotMatched,
+} from './passwordStrength.fixtures';
+
+configure({ adapter: new Adapter() });


these setup are no longer need (see webpack/test_setup.js)

ohadlevy · 2017-12-25T08:59:54Z

webpack/assets/javascripts/react_app/components/user/passwordStrength/passwordStrength.test.js

+configure({ adapter: new Adapter() });
+
+jest.unmock('./');
+global.__ = jest.fn();


not required anymore

ohadlevy · 2017-12-25T09:00:20Z

webpack/assets/javascripts/react_app/components/user/passwordStrength/passwordStrength.test.js

+function setup(verify, error, state) {
+  const store = mockStore(state);
+
+  return mount(<PasswordStrength


would shallow work here too?

shallow doesn't render inner components therefore the resulted snapshot lacks in inner content

ohadlevy · 2017-12-25T09:04:00Z

webpack/assets/javascripts/react_app/components/user/passwordStrength/index.js

+            inputProps={{
+              name,
+              id,
+              className: `${className}`,


do you really need the string interrelation here? I think the following would just work:

inputProps={{ name, id, className }}

ohadlevy · 2017-12-25T09:04:53Z

webpack/assets/javascripts/react_app/components/user/passwordStrength/index.js

+          <CommonForm
+            label={__('Verify')}
+            touched={true}
+            error={matchMessage ? verify.error : __('Password do not matched')}


The original string was passwords do not match

amirfefer · 2017-12-26T16:08:27Z

@ohadlevy thanks, I've fixed and rebased. would you mind take another look?

mmoll · 2017-12-26T18:09:23Z

@amirfefer Is the test failure in the foreman test related to these changes?

ares · 2018-01-03T07:16:03Z

ping @amirfefer

amirfefer · 2018-01-16T16:58:36Z

I've fixed the users integration test,
Apparently the react component did not render at all via capybara.

mmoll · 2018-01-16T18:18:36Z

@amirfefer please rebase

I guess the travis test "failures" also need fixing/updating...

amirfefer · 2018-01-17T08:55:15Z

Rebased.
plus I've updated jest snapshot.

ohadlevy · 2018-01-25T10:58:56Z

@amirfefer anything left here?

ohadlevy · 2018-01-25T10:59:52Z

webpack/assets/javascripts/react_app/common/colors.scss

@@ -1 +1,7 @@
 $pf-black-200: #ededed;
+$pf-red-200: #a30000;


can we use this from the patternfly package? @sharvit ?

When my css pr will merge you would be able to import variables:
https://github.com/theforeman/foreman/pull/5212/files#diff-f430da9fad5e785cb1f815fd719fdc5aR1

ohadlevy · 2018-01-25T12:56:30Z

@amirfefer can you please also include

diff --git a/config/webpack.vendor.js b/config/webpack.vendor.js
index aed3a4bde..e6e2f023a 100644
--- a/config/webpack.vendor.js
+++ b/config/webpack.vendor.js
@@ -9,6 +9,7 @@ module.exports = [
   'react-ellipsis-with-tooltip',
   'react-numeric-input',
   'react-onclickoutside',
+  'react-password-strength',
   'patternfly-react',
   'react-redux',
   'redux',

amirfefer · 2018-01-25T13:10:38Z

@ohadlevy - done :)

amirfefer · 2018-01-28T09:33:59Z

[test foreman]

sharvit

LGTM!

amirfefer · 2018-01-28T12:50:28Z

@ohadlevy tests are green :)

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Oct 2, 2017

houndci-bot reviewed Oct 2, 2017

View reviewed changes

amirfefer changed the title ~~Fixes 21170 - move password_strength.js logic to react~~ Fixes 21170 - move password strength meter logic to react Oct 2, 2017

amirfefer force-pushed the 21170 branch from bb7d2ce to ca61065 Compare October 2, 2017 18:25

theforeman-bot added Needs testing and removed Waiting on contributor labels Oct 2, 2017

amirfefer changed the title ~~Fixes 21170 - move password strength meter logic to react~~ Fixes #21170 - move password strength meter logic to react Oct 2, 2017

amirfefer force-pushed the 21170 branch from ca61065 to 4034a9a Compare October 2, 2017 18:33

waldenraines suggested changes Oct 3, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Oct 3, 2017

ohadlevy reviewed Oct 3, 2017

View reviewed changes

amirfefer force-pushed the 21170 branch from 4034a9a to 3e33550 Compare October 18, 2017 15:13

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 18, 2017

amirfefer force-pushed the 21170 branch from 3e33550 to 0703a8e Compare October 18, 2017 16:12

houndci-bot reviewed Oct 18, 2017

View reviewed changes

amirfefer force-pushed the 21170 branch from 0703a8e to b396277 Compare October 18, 2017 16:39

waldenraines approved these changes Oct 18, 2017

View reviewed changes

amirfefer force-pushed the 21170 branch from b396277 to 88fa932 Compare October 20, 2017 11:42

sharvit approved these changes Dec 19, 2017

View reviewed changes

ohadlevy suggested changes Dec 25, 2017

View reviewed changes

amirfefer force-pushed the 21170 branch from 84747dc to e5ad452 Compare December 26, 2017 16:04

ohadlevy added Waiting on contributor and removed Needs re-review Needs testing labels Jan 2, 2018

amirfefer force-pushed the 21170 branch from e5ad452 to 8f7d639 Compare January 16, 2018 16:54

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 16, 2018

amirfefer force-pushed the 21170 branch from 8f7d639 to bcdf96c Compare January 17, 2018 08:54

ohadlevy reviewed Jan 25, 2018

View reviewed changes

Fixes #21170 - move password_strength.js to wepback

070d417

amirfefer force-pushed the 21170 branch from bcdf96c to 070d417 Compare January 25, 2018 13:09

ohadlevy approved these changes Jan 25, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Jan 25, 2018

sharvit approved these changes Jan 28, 2018

View reviewed changes

ohadlevy merged commit 6850ad9 into theforeman:develop Jan 28, 2018

		@@ -0,0 +1,10 @@
		.ReactPasswordStrength-input {
		width: 100% !important;

Fixes #21170 - move password strength meter logic to react #4882

Fixes #21170 - move password strength meter logic to react #4882

Conversation

amirfefer commented Oct 2, 2017 • edited Loading

theforeman-bot commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Oct 3, 2017

Rohoover commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Oct 18, 2017 • edited Loading

waldenraines left a comment

Choose a reason for hiding this comment

amirfefer commented Oct 19, 2017

ares commented Oct 20, 2017

sharvit left a comment

Choose a reason for hiding this comment

ohadlevy commented Dec 25, 2017

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Dec 26, 2017

mmoll commented Dec 26, 2017

ares commented Jan 3, 2018

amirfefer commented Jan 16, 2018

mmoll commented Jan 16, 2018

amirfefer commented Jan 17, 2018 • edited Loading

ohadlevy commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Jan 25, 2018

amirfefer commented Jan 25, 2018

amirfefer commented Jan 28, 2018

sharvit left a comment

Choose a reason for hiding this comment

amirfefer commented Jan 28, 2018

amirfefer commented Oct 2, 2017 •

edited

Loading

amirfefer commented Oct 18, 2017 •

edited

Loading

amirfefer commented Jan 17, 2018 •

edited

Loading