Per 9929 show password strength #494

crisnicandrei · 2024-11-27T11:45:57Z

This pull request displays the strength of a user's password when registering a new account.

Steps to test:

Go to the register page
Start inputting a password
A message will appear below with the strength of it

codecov · 2024-11-27T11:49:41Z

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 43.35%. Comparing base (241e1fb) to head (121ca22).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
.../components/password-strength/password-strength.ts	93.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   43.28%   43.35%   +0.06%     
==========================================
  Files         363      364       +1     
  Lines       11123    11153      +30     
  Branches     1818     1820       +2     
==========================================
+ Hits         4815     4835      +20     
- Misses       6148     6156       +8     
- Partials      160      162       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

meisekimiu · 2024-12-03T14:13:18Z

src/app/auth/components/signup/signup.component.html

+        waiting ||
+        signupForm.invalid ||
+        !agreedTerms ||
+        passwordStrengthMessage !== 'Strong'


I'm not sure if we want to block people from signing up just based on password strength. The intention is to show an indicator so that people can see their password strength and make their own decision on if they want to pick a stronger password.

meisekimiu · 2024-12-03T14:16:09Z

src/app/auth/components/signup/signup.component.scss

+  color: red;
+}
+
+.strength-weak {
+  color: orange;
+}
+
+.strength-medium {
+  color: blue;
+}
+
+.strength-strong {
+  color: green;


This is not an request for changes immediately but we probably want some design/accessibility review here for color choices that match Permanent's theme and also have proper contrast against the background.

meisekimiu · 2024-12-03T14:17:50Z

src/app/auth/components/signup/signup.component.ts

  }

+  ngOnInit(): void {
+    this.signupForm.controls['password'].valueChanges.subscribe((password) => {


Does this subscription get automatically cleaned up? It might, and I'd love confirmation that it does, but I feel like subscriptions have occasionally caused errors in the application once the things they refer to have been destroyed.

meisekimiu · 2024-12-03T14:19:38Z

src/app/auth/components/signup/signup.component.ts

+  passwordStrengthMessage: string = '';
+  passwordStrengthClass: string = '';


In theory we can make this typing even more specific with a type union. That way we know that Message can only ever be "Strong", "Weak", etc.

In this case it might be a bit redundant but it's nice to keep that in mind, especially when testing for string equality like we do in this component.

crisnicandrei · 2024-12-03T14:48:40Z

@meisekimiu thanks! I will do the changes! @k8lyn6 can you please suggest some color values for the messages?

k8lyn6 · 2024-12-03T14:50:39Z

@crisnicandrei we can look at it tomorrow during our accessibility review. cc @yeslikesolo

meisekimiu

We've decided this probably needs more design work before it's ready to be released. We probably don't want to erase the work already done though, so can you hide this behind a feature flag, using the FeatureFlagService to test for its presence?

crisnicandrei · 2024-12-06T13:41:10Z

@meisekimiu for sure!

crisnicandrei · 2024-12-06T15:22:26Z

@meisekimiu let me know if my use of the feature flag was correct, as i have not used it before

meisekimiu · 2024-12-09T21:17:50Z

src/app/auth/components/signup/signup.component.ts

+    this.passwordSubscription = this.signupForm.controls[
+      'password'
+    ].valueChanges.subscribe((password) => {
+      if (this.featureFlagService.isEnabled('passwordStrngth')) {


Suggested change

if (this.featureFlagService.isEnabled('passwordStrngth')) {

if (this.featureFlagService.isEnabled('password-strength')) {

There's a typo in this flag name. We might also want to use "kebab-case" for feature flags. I think it might be the most accessible to people on the experience team, though we don't have a defined standard yet so it might be fine to leave it like this.

As a small note, I like naming a component's instance of the FeatureFlagService just feature so that the code can read nicely: this.feature.isEnabled('feature-name'), but it doesn't really matter what it's named and it's easy to refactor later.

k8lyn6 · 2024-12-11T17:25:27Z

@meisekimiu can you tag me in a comment when this is ready for QA?

meisekimiu

@k8lyn6 this is ready for QA now. In theory the only step should be to verify it is controlled by the feature flag and doesn't appear by default.

k8lyn6

Sorry for the confusion with this ticket Andrei! I shouldn't have put it in the front-end-backlog without a design first. The work you did is great and as soon as Tibi has a design for us, we can pick it back up.

The feature flag seems to be working as designed. When this branch is deployed, I no longer see the password strength information.

crisnicandrei · 2025-01-08T15:33:13Z

@k8lyn6 I have pushed the changes with tibi's new design. I also have found a bug while working on this so I will create a ticket for that! Thanks!

k8lyn6 · 2025-01-08T19:55:24Z

@meisekimiu before I do a review, was this branch already merged? Should there be a separate PR for additional work? Just wanted to check before proceeding.

crisnicandrei · 2025-01-08T19:58:36Z

@k8lyn6 it was not merged

meisekimiu

I have approved this already, with the exception of the linting and prettier changes which don't really need to be checked again.

meisekimiu

I didn't realize that this incorporates new design elements and therefore needs a more thorough review. The idea behind hiding the previous changes behind a feature flag is that it allowed us to merge those changes to main and address the new design elements in their own ticket. In general it's preferable to do that so that tickets can associated with their own pull requests whenever possible, changes are visible to other developers sooner, there are less merge conflicts when integrating more frequently, and it just feels good to be able to close pull requests faster (as it stands right now, we now have to do review for a pull request opened in November, which isn't great).

It feels weird at first, but the whole point of feature flags is that they allow us to merge code for a feature that is incomplete without users seeing it. This lets us make smaller pull requests that can be integrated more frequently, since the feature doesn't have to be 100% done to warrant merging it. See https://martinfowler.com/articles/feature-toggles.html for more details. I encourage us all to take advantage of feature flags to merge things earlier and split things into multiple small tickets.

meisekimiu · 2025-01-08T20:36:05Z

src/app/auth/components/signup/signup.component.scss

+.strength-too-weak {
+  color: red;
+}
+
+.strength-weak {
+  color: orange;
+}
+
+.strength-medium {
+  color: blue;
+}
+
+.strength-strong {
+  color: green;
+}
+
+.password-strength-meter {
+  margin-top: 5px;
+  font-size: 0.9em;
+}


It seems like all of this CSS is now unused since the styles for the meter are handled by the strength-meter component, right?

meisekimiu · 2025-01-08T20:38:56Z

src/app/auth/components/signup/signup.component.ts

+      // if (this.feature.isEnabled('password-strength')) {
+      this.updatePasswordStrength(password);
+      // }


Are these supposed to be commented out? We don't want to be committing commented out code, but we did want a feature flag on this. As it stands, this enables the feature globally regardless of feature flag. (Of course, now that it's been redesigned at least a little bit we don't need the feature flag as much, but Kaitlyn says we can still use it)

meisekimiu · 2025-01-08T20:46:16Z

src/app/auth/components/signup/signup.component.ts

+  updatePasswordStrength(password: string): void {
+    const { message, class: strengthClass } =
+      this.getPasswordStrengthDetails(password);
+    this.passwordStrengthMessage = message as PasswordType;
+    this.passwordStrengthClass = strengthClass;
+  }


I know that this comes from having to bind to an input value and also deal with form values and validation, but if we want to separate the password strength meter into its own component (which we do want to do! We want to be able to reuse this functionality!), then I think all of its logic should live in the component. In other words, the SignupComponent shouldn't really know anything about password strength since that should all be happening in the password strength meter component instead.

Think about it this way: if we want to reuse this password strength meter on the "Change Password" part of the application, we'll have to duplicate all of this code into it, right?

This gets tricky, but I think with the correct combination of inputs/outputs we can properly bind the password input, its valueChanges observable, and the validity of the whole form to an external password strength meter component that can be reused elsewhere in the UI.

meisekimiu · 2025-01-08T20:48:17Z

src/app/shared/components/strength-meter/strength-meter.component.ts

+  templateUrl: './strength-meter.component.html',
+  styleUrls: ['./strength-meter.component.scss'],
+})
+export class StrengthMeterComponent implements OnChanges {


I don't know if this is actually a better name, but I think maybe naming it specifically PasswordStrengthMeterComponent (or even just PasswordStrengthComponent in case we get rid of the meter one day?) could be better. This component is specifically made for measuring password strength, and not any other form of strength.

meisekimiu · 2025-01-08T20:51:58Z

src/app/shared/components/strength-meter/strength-meter.component.ts

@@ -0,0 +1,50 @@
+import { Component, Input, OnChanges } from '@angular/core';


I'm not entirely sure where we should put this, but I want to discourage adding new things to the SharedModule. It is the easiest place to put shared components, but we want to break up that module into more specific pieces (#20) and the first step to get there is to not add any more pieces to it.

I'd say we should put this in the AuthModule as a declared, but not exported, component for the time being, since this is only used on this screen. If we add it to the "Change Password" screen later, it might make sense to just add this to our component library module.

Oh also, we should add the Prettier format pragma to this file.

meisekimiu · 2025-01-08T21:11:54Z

src/app/shared/components/strength-meter/strength-meter.component.ts

+  styleUrls: ['./strength-meter.component.scss'],
+})
+export class StrengthMeterComponent implements OnChanges {
+  progressBars: string[] = ['', '', ''];


This is not a blocker so feel free not to address it.

I think this implementation is a bit loose, while also being a bit too coupled to the output HTML I guess? If we break down this component it really has 4 possible states: null, weak, medium, and strong. But the way the state is represented internally in the component allows for at least 12* states to be represented in the progress bars alone. While something like ['weak', '', ''] is a normal state, there's nothing in the code that prevents us from having a weird nonsensical state like ['','strong','half']. It also means that in tests we have to express our assertions with these arrays, even though we're logically only testing for those 4 possible states. These tests have to know about the internal details of our component instead of testing more abstract state behavior.

_{*Technically since progressBars is only of type string[], it's possibility of states is near infinite!}

I think ideally the internal state of the application should try to be consistent with how we'd map it out logically. It helps prevent unintended errors or states from appearing and also just makes these components easier to work with. Instead of having to know what specific array values we need to set, we can just set this.strength = PasswordStrength.strong or something like that (it doesn't need to be an enum, but I used an enum since that's typically the tool used to convey a single state of many that an object can be in).

crisnicandrei · 2025-01-08T21:35:32Z

@meisekimiu great points! And thank you for the article! I will address this!

crisnicandrei · 2025-01-09T14:31:33Z

@meisekimiu pushed the changes! Thanks again for your input!

meisekimiu

Thank you for addressing the feedback. Left some additional comments. Can you rename the folder path to the Password Strength component to match its new name? And also consider moving that component into the AuthModule?

meisekimiu · 2025-01-09T16:35:50Z

src/app/auth/components/signup/signup.component.ts

 } from '@models';
 import { DeviceService } from '@shared/services/device/device.service';
 import { GoogleAnalyticsService } from '@shared/services/google-analytics/google-analytics.service';
+import { passwordStrength } from 'check-password-strength';


Suggested change

import { passwordStrength } from 'check-password-strength';

We can get rid of this import.

meisekimiu · 2025-01-09T16:36:11Z

src/app/auth/components/signup/signup.component.ts

+type PasswordType = '' | 'Too Weak' | 'Weak' | 'Medium' | 'Strong';
+


Suggested change

type PasswordType = '' | 'Too Weak' | 'Weak' | 'Medium' | 'Strong';

This can probably go too, right?

meisekimiu · 2025-01-09T16:38:54Z

src/app/shared/components/strength-meter/password-strength.ts

+  progressBars: string[] = ['', '', ''];
+
+  public strength: PasswordStrength = PasswordStrength.Null;
+  public message: string = '';
+  public passwordClass: string = '';
+  @Input() password: string = '';
+  public enabledPasswordCheckStrength: boolean;


There's a bit of style inconsistency here. Generally we like to visually separate the Input() fields from standard class properties. And some of these have access modifiers (something I generally like to see) while some don't.

Suggested change

progressBars: string[] = ['', '', ''];

public strength: PasswordStrength = PasswordStrength.Null;

public message: string = '';

public passwordClass: string = '';

@Input() password: string = '';

public enabledPasswordCheckStrength: boolean;

@Input() public password: string = '';

public progressBars: [string, string, string] = ['', '', ''];

public strength: PasswordStrength = PasswordStrength.Null;

public message: string = '';

public passwordClass: string = '';

public enabledPasswordCheckStrength: boolean;

meisekimiu · 2025-01-09T16:44:29Z

src/app/shared/components/strength-meter/password-strength.ts

+        this.strength = PasswordStrength.Weak;
+        this.message = 'weak';
+        this.passwordClass = 'too-weak';
+        this.progressBars = ['weak', '', ''];


This is just a general piece of advice. This switch statement is rather verbose, isn't it? This is at least partly due to the fact that while we're now using an enum to declare password strength, we still have to set all of those other values alongside it. We could clear up this code quite a bit if we could derive those values from our this.strength value with something like a function call in the typescript layer or a pipe in the template layer.

crisnicandrei · 2025-01-09T18:51:08Z

@meisekimiu did the changes

k8lyn6

Hey @crisnicandrei I'm not seeing any password strength indications at all when I deploy this branch. Is there something else I need to do? @meisekimiu do I need to activate the feature flag?

crisnicandrei · 2025-01-09T22:48:28Z

@k8lyn6 yes, the feature flag must be active

k8lyn6 · 2025-01-10T14:54:09Z

@crisnicandrei what is the name of the feature flag? I don't see it in the retool app (I switched the environment to dev)

crisnicandrei · 2025-01-10T15:01:34Z

@k8lyn6 it is 'password-strength', but I did not set it in retool, do I have to do that @meisekimiu ? This is the first time using the feature flag so I am unsure

k8lyn6

Oh, nice! This looks really great. I don't think we did either an accessibility check or a copy check yet, so we might need to release this under the feature flag and then adjust for accessibility (specifically, the yellow might not pass accessibility guidelines for color contrast).

For the copy, can we do the following:

Password strength: weak
Password strength: medium
Password strength: strong

crisnicandrei · 2025-01-10T21:30:59Z

@k8lyn6 what do you mean about a copy check? I have looked it up online but found nothing so I am a bit confused

k8lyn6 · 2025-01-10T21:57:39Z

My apologies for the confusion @crisnicandrei ! By copy, I mean the words and language we're using in the design (such as "Your password is strong!"). Normally, in the design process, the last thing I do with Tibi is a review of the copy so that I don't have to ask you to re-code any of the language in the design after the fact. I wasn't able to do an accessibility or copy review before the implementation, which is why we're changing up the language on the fly.

If we could swap out the text that's currently implemented with what I wrote in my review, that would be great. I think that's the only change I have for now, but Han, Natalie, and I will need to do an accessibility review of the color contrast. Because this is behind a feature flag, you should be able to merge the changes after we change the text and it won't go live on production yet. We can make additional tickets for any changes after the accessibility review.

crisnicandrei · 2025-01-13T09:47:27Z

@k8lyn6 I have updated the text

k8lyn6

Thanks for the update @crisnicandrei ! I think we're good to merge this PR, but not turn the feature on on prod yet (since it's still hidden by a feature flag). We'll do an accessibility review and add any additional tickets that are necessary afterwards.

cc @meisekimiu

This pr displays the strength of the password when the user registers a new account.

Move the logic from the signup component into the password-strength component. This way the new component will be the only one handling the password strength.

crisnicandrei requested a review from meisekimiu November 27, 2024 11:45

meisekimiu suggested changes Dec 3, 2024

View reviewed changes

k8lyn6 self-requested a review December 4, 2024 19:49

meisekimiu reviewed Dec 5, 2024

View reviewed changes

meisekimiu reviewed Dec 9, 2024

View reviewed changes

crisnicandrei requested a review from meisekimiu December 11, 2024 18:11

meisekimiu approved these changes Dec 11, 2024

View reviewed changes

k8lyn6 approved these changes Dec 11, 2024

View reviewed changes

crisnicandrei force-pushed the PER-9929-show-password-strength branch from 3d68c84 to af29c65 Compare January 8, 2025 15:39

k8lyn6 requested review from k8lyn6 and meisekimiu January 8, 2025 19:54

meisekimiu approved these changes Jan 8, 2025

View reviewed changes

meisekimiu suggested changes Jan 8, 2025

View reviewed changes

meisekimiu reviewed Jan 9, 2025

View reviewed changes

crisnicandrei requested a review from meisekimiu January 9, 2025 17:07

k8lyn6 reviewed Jan 9, 2025

View reviewed changes

k8lyn6 self-requested a review January 9, 2025 22:47

meisekimiu approved these changes Jan 10, 2025

View reviewed changes

k8lyn6 requested changes Jan 10, 2025

View reviewed changes

crisnicandrei requested a review from k8lyn6 January 13, 2025 09:47

k8lyn6 approved these changes Jan 14, 2025

View reviewed changes

crisnicandrei added 15 commits January 14, 2025 16:53

Install library

ba96793

PER-9929-display-password-strength

947bc47

This pr displays the strength of the password when the user registers a new account.

enable submit button even if password is weak

5029abf

add the feature flag service

32a7f61

added tests

69f0737

fix typo

5f42fd5

Create new stregth meter component for password strength meter

af5af91

Prettier

9f7853e

linting

629b00c

PER-9929-show-password-strength

8fa9737

Move the logic from the signup component into the password-strength component. This way the new component will be the only one handling the password strength.

Refactor the setStrength method

2d42fa3

change password strength text

5be6abe

reverse flag

9a03499

fix tests

fee6685

prettier

121ca22

crisnicandrei force-pushed the PER-9929-show-password-strength branch from 6f2e10d to 121ca22 Compare January 14, 2025 14:53

crisnicandrei merged commit aa45621 into main Jan 14, 2025
4 checks passed

crisnicandrei deleted the PER-9929-show-password-strength branch January 14, 2025 15:14

		passwordStrengthMessage: string = '';
		passwordStrengthClass: string = '';

	if (this.featureFlagService.isEnabled('passwordStrngth')) {
	if (this.featureFlagService.isEnabled('password-strength')) {

		@@ -0,0 +1,50 @@
		import { Component, Input, OnChanges } from '@angular/core';

		type PasswordType = '' \| 'Too Weak' \| 'Weak' \| 'Medium' \| 'Strong';

Per 9929 show password strength #494

Per 9929 show password strength #494

Uh oh!

Conversation

crisnicandrei commented Nov 27, 2024

Uh oh!

codecov bot commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisnicandrei commented Dec 3, 2024

Uh oh!

k8lyn6 commented Dec 3, 2024

Uh oh!

meisekimiu left a comment

Choose a reason for hiding this comment

Uh oh!

crisnicandrei commented Dec 6, 2024

Uh oh!

crisnicandrei commented Dec 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8lyn6 commented Dec 11, 2024

Uh oh!

meisekimiu left a comment

Choose a reason for hiding this comment

Uh oh!

k8lyn6 left a comment

Choose a reason for hiding this comment

Uh oh!

crisnicandrei commented Jan 8, 2025

Uh oh!

k8lyn6 commented Jan 8, 2025

Uh oh!

crisnicandrei commented Jan 8, 2025

Uh oh!

meisekimiu left a comment

Choose a reason for hiding this comment

Uh oh!

meisekimiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisnicandrei commented Jan 8, 2025

Uh oh!

crisnicandrei commented Jan 9, 2025

Uh oh!

meisekimiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisnicandrei commented Jan 9, 2025

Uh oh!

codecov bot commented Nov 27, 2024 •

edited

Loading