clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-7330] Create SetPasswordComponent (email verification)

Open rr-bw opened this issue 1 year ago • 2 comments

🎟️ Tracking

  • PM-7330 - Create new SetPasswordComponent
  • PM-5133 - [Web] JIT-provisioned SSO user should use new SetPasswordComponent

📔 Objective

Creates a base SetPasswordV2Component class file in Libs, and a Web SetPasswordV2Component, which is routed to when the email verification feature flag is on.

Much of the logic for the V2 component is copied from our existing SetPasswordComponent and then modified to work with our new InputPasswordComponent.

📸 Screenshots

https://github.com/bitwarden/clients/assets/102181210/1bc091af-16a8-4484-8a39-4a17f11143b3

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

rr-bw avatar Jun 24 '24 19:06 rr-bw

Codecov Report

Attention: Patch coverage is 42.07317% with 95 lines in your changes missing coverage. Please review.

Project coverage is 31.96%. Comparing base (06b370e) to head (a88278e).

Files Patch % Lines
...lar/set-password-jit/set-password-jit.component.ts 0.00% 53 Missing :warning:
apps/desktop/src/app/services/services.module.ts 0.00% 8 Missing :warning:
...t-password-jit/default-set-password-jit.service.ts 86.66% 8 Missing :warning:
...c/app/services/desktop-set-password-jit.service.ts 0.00% 7 Missing :warning:
...angular/input-password/input-password.component.ts 0.00% 6 Missing :warning:
...s/set-password-jit/web-set-password-jit.service.ts 50.00% 5 Missing :warning:
libs/auth/src/angular/index.ts 0.00% 5 Missing :warning:
libs/angular/src/auth/components/sso.component.ts 50.00% 2 Missing :warning:
...ssword-jit/set-password-jit.service.abstraction.ts 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9810      +/-   ##
==========================================
+ Coverage   31.89%   31.96%   +0.06%     
==========================================
  Files        2661     2667       +6     
  Lines       79086    79239     +153     
  Branches    14787    14806      +19     
==========================================
+ Hits        25228    25325      +97     
- Misses      51949    52005      +56     
  Partials     1909     1909              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 24 '24 19:06 codecov[bot]

Logo Checkmarx One – Scan Summary & Details0acba5a0-a423-474e-b576-83884432a4b4

No New Or Fixed Issues Found

github-actions[bot] avatar Jun 24 '24 19:06 github-actions[bot]

I also just noticed that the success toasts weren't showing in the videos, are those from the previous iteration of this PR?

Also going to tag @danielleflinn to see if she'd like to do a design review on this one, the MP policy requirements on the browser look a little...tight.

jlf0dev avatar Jul 07 '24 22:07 jlf0dev

This looks good! I do agree that both the browser and desktop MP policy callouts look a bit off though. Is there a way we can fix the spacing between the leftmost edge of the bullets and the text?

danielleflinn avatar Jul 08 '24 14:07 danielleflinn

I also just noticed that the success toasts weren't showing in the videos, are those from the previous iteration of this PR?

@jlf0dev Yes, those were previous videos. Here's an updated one with toasts.

https://github.com/bitwarden/clients/assets/102181210/f91d4187-924b-48e5-8a60-925418d7b843

rr-bw avatar Jul 10 '24 00:07 rr-bw

I do agree that both the browser and desktop MP policy callouts look a bit off though. Is there a way we can fix the spacing between the leftmost edge of the bullets and the text?

@danielleflinn Here are updated screenshots for Desktop and Browser:

desktop-list

browser-list

rr-bw avatar Jul 10 '24 00:07 rr-bw

@rr-bw looks great! Thank you

danielleflinn avatar Jul 10 '24 00:07 danielleflinn

@jlf0dev I addressed the recent feedback and also updated the PR description with a new screencast for each client. I am having one issue with Browser. As you can see in the video, setting the password works fine, but I cannot get the account to sync with the org and get access to the org. Here is the error I get:

Screenshot 2024-07-03 at 11 38 35 AM

Am I potentially doing something wrong with service injection? I also tried adding the following to main.background.ts, but that did not help.

Screenshot 2024-07-03 at 12 33 56 PM

rr-bw avatar Jul 11 '24 19:07 rr-bw

@rr-bw I just noticed a few inconsistencies with Figma. Are we able to make the title of this page "Join organization" (once we can support dynamic text in the Anon layout title we can update this to "Join [Org name]" to match Figma), and can we change the CTA to "Create account"?

Also, I know in Figma we have the "Finishing joining org..." text in the content area; but what do you think about moving this to the Anon layout subtitle?

If we are using this UI for another flow other than the Join organization we can probably keep the text the same, but if it is just for the Join Org flow, lets customize it a bit for the user.

danielleflinn avatar Jul 11 '24 22:07 danielleflinn

I am having one issue with Browser. As you can see in the video, setting the password works fine, but I cannot get the account to sync with the org and get access to the org.

Issue fixed in a conversation with @jlf0dev

rr-bw avatar Jul 11 '24 22:07 rr-bw

Are we able to make the title of this page "Join organization" (once we can support dynamic text in the Anon layout title we can update this to "Join [Org name]" to match Figma), and can we change the CTA to "Create account"?

Also, I know in Figma we have the "Finishing joining org..." text in the content area; but what do you think about moving this to the Anon layout subtitle?

@danielleflinn For sure, here's an updated screenshot:

Screenshot 2024-07-12 at 11 59 51 AM

rr-bw avatar Jul 12 '24 19:07 rr-bw

@rr-bw Thank you this looks great! I'll make sure things are updated in Figma accordingly for QA

danielleflinn avatar Jul 12 '24 19:07 danielleflinn