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.

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

[PM-2057] update two factor email dialog #8974

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

Conversation

vinith-kovan
Copy link
Collaborator

Type of change

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

Objective

Update Two Factor email component to use the component library.

Code changes

two-factor-email.component.html - Updated the bootstrap CSS to use the equivalent components from the Component Library.
two-factor-email.component.ts - Updated the dialog service from modal.
two-factor-setup.component.ts - Updated the email switch case to call the two-factor email dialog service.

Screenshots

t0w-factor-email.-.Made.with.Clipchamp.mp4

@vinith-kovan vinith-kovan requested a review from a team as a code owner April 29, 2024 16:42
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Apr 29, 2024
Copy link
Contributor

github-actions bot commented Apr 29, 2024

Logo
Checkmarx One – Scan Summary & Details63878db2-5d47-40ff-b6b9-0efdb0e33be0

No New Or Fixed Issues Found

</button>
<button type="button" class="btn btn-outline-secondary" data-dismiss="modal">
{{ "close" | i18n }}
<form [formGroup]="formGroup" [bitSubmit]="submit" [appApiAction]="formPromise" *ngIf="authed">
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 and formPromise from this component

Copy link

codecov bot commented May 20, 2024

Codecov Report

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

Project coverage is 28.05%. Comparing base (c0bb7b9) to head (23dbfb6).

Files Patch % Lines
...rc/app/auth/settings/two-factor-email.component.ts 10.71% 23 Missing and 2 partials ⚠️
...rc/app/auth/settings/two-factor-setup.component.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8974      +/-   ##
==========================================
- Coverage   28.06%   28.05%   -0.02%     
==========================================
  Files        2391     2391              
  Lines       70512    70531      +19     
  Branches    13205    13208       +3     
==========================================
- Hits        19789    19785       -4     
- Misses      49158    49178      +20     
- Partials     1565     1568       +3     

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

@vinith-kovan vinith-kovan requested a review from jlf0dev May 21, 2024 18:02
const authComp: DialogRef<boolean, any> = TwoFactorEmailComponent.open(this.dialogService, {
data: result,
});
authComp.componentInstance.onChangeStatus.subscribe((enabled: boolean) => {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to have a takeUntil that will clean up the subscription when the authComp.closed observable emits.

@vinith-kovan vinith-kovan requested a review from jlf0dev May 27, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Marks a PR as requiring QA approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants