Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PM-4956] two factor component migration #9204

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[PM-4956] two factor component migration #9204

wants to merge 6 commits into from
+160 −169

Conversation

vinith-kovan
Copy link
Collaborator

@vinith-kovan vinith-kovan commented May 16, 2024 •
edited

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [x] Other

Objective

Migrate the Two Factor Component to use the component library.

Code changes

  • two-factor.component.html: Added form group and bit form fields
  • two-factor.component.ts: Added new form group, converted submit to arrow function
  • oss-routing.module.ts: Added Routing for Anon layout wrapper component.

Screenshots

two-factor.-.Made.with.Clipchamp.1.mp4
Untitled.video.-.Made.with.Clipchamp.2.mp4

@vinith-kovan vinith-kovan requested a review from a team as a code owner May 16, 2024 04:52
@vinith-kovan vinith-kovan requested a review from jlf0dev May 16, 2024 04:52
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label May 16, 2024
@codecov Codecov
Copy link

codecov bot commented May 16, 2024 •
edited

Codecov Report

Attention: Patch coverage is 16.66667% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 28.04%. Comparing base (81c1456) to head (ab04871).

Files Patch % Lines
apps/web/src/app/auth/two-factor.component.ts 18.18% 8 Missing and 1 partial ⚠️
apps/web/src/app/oss-routing.module.ts 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9204      +/-   ##
==========================================
- Coverage   28.05%   28.04%   -0.01%     
==========================================
  Files        2391     2391              
  Lines       70521    70532      +11     
  Branches    13207    13208       +1     
==========================================
+ Hits        19782    19783       +1     
- Misses      49174    49183       +9     
- Partials     1565     1566       +1

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 16, 2024 •
edited

Logo
Checkmarx One – Scan Summary & Details – 23aa25ab-c58b-457f-8218-9338652e5adf

No New Or Fixed Issues Found

jlf0dev
jlf0dev requested changes May 17, 2024
View reviewed changes
Copy link
Member

@jlf0dev jlf0dev left a comment •
edited

Choose a reason for hiding this comment

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

Since this is a very complex part of our code, please add some videos of successfully completing different 2fa methods and switching between them.

Tagging @bitwarden/dept-design for review as well.

apps/web/src/app/auth/two-factor.component.html Outdated
@@ -1,173 +1,136 @@
<form
#form
(ngSubmit)="submit()"
[bitSubmit]="submitForm"
[appApiAction]="formPromise"
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the appApiAction since the bitSubmit handles the loading state.

apps/web/src/app/auth/two-factor.component.html Outdated
href="#"
appStopClick
(click)="sendEmail(true)"
[appApiAction]="emailPromise"
Copy link
Member

Choose a reason for hiding this comment

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

This should use the bitAction directive instead of the appApiAction

@danielleflinn danielleflinn self-requested a review May 17, 2024 19:40
@vinith-kovan vinith-kovan requested a review from jlf0dev May 21, 2024 11:45
@jlf0dev
Copy link
Member

jlf0dev commented May 22, 2024

Since this is a very complex part of our code, please add some videos of successfully completing different 2fa methods and switching between them.

Hi @vinith-kovan, any chance we can get a video showing off some of the other functionality? If you're not sure how to do that please let me know. Thanks!

@vinith-kovan
Copy link
Collaborator Author

@jlf0dev I have added a video of two-factor authenticator, please take a look.

@danielleflinn
Copy link
Member

I'm not seeing the page title that we discussed in slack in the screen recording. Please add "Verify your Identity" to the 2FA page.

@vinith-kovan
Copy link
Collaborator Author

Hi @danielleflinn "Verify your Identity" has been added.

image

jlf0dev
jlf0dev reviewed May 29, 2024
View reviewed changes
apps/web/src/app/auth/two-factor.component.html
</a>
</div>
<div class="text-center">
<a biLink href="#" appStopClick (click)="anotherMethod()">{{
Copy link
Member

Choose a reason for hiding this comment

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

Is bilink here supposed to be bitlink?

apps/web/src/app/auth/two-factor.component.html
Comment on lines +77 to +82
<i
class="bwi bwi-spinner tw-text-muted bwi-spin pull-right"
title="{{ 'loading' | i18n }}"
*ngIf="selectedProviderType === providerType.WebAuthn"
aria-hidden="true"
></i>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this spinner, I'm not sure how much value its adding here

jlf0dev
jlf0dev reviewed May 29, 2024
View reviewed changes
apps/web/src/app/auth/two-factor.component.ts
Comment on lines +42 to +45
formGroup = this.formBuilder.group({
token: ["", [Validators.required]],
remember: [false],
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting some strange behavior with the default updateOn behavior, as you can see in this video.

Screen.Recording.2024-05-29.at.11.51.37.AM.mov

Can you please change this to update on submit as so:

  formGroup = this.formBuilder.group({
    token: [
      "",
      {
        validators: [Validators.required],
        updateOn: "submit",
      },
    ],
    remember: [false],
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlf0dev jlf0dev jlf0dev requested changes

@danielleflinn danielleflinn danielleflinn approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
needs-qa Marks a PR as requiring QA approval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vinith-kovan @jlf0dev @danielleflinn