feat(dev-portal): add reset password #10659
Conversation
naouf4l-grav
force-pushed
the
APIM-4338-add-reset-password
branch
from
a72c72b to
5069da0
Compare
naouf4l-grav
force-pushed
the
APIM-4338-add-reset-password
branch
from
5069da0 to
2d251dd
Compare
naouf4l-grav
changed the title
| @@ -50,6 +50,9 @@ | |||
| Log in | |||
| </button> | |||
| </mat-card-actions> | |||
| <mat-card-content> | |||
| <a class="log-in__form__reset" routerLink="/reset-password">Forget password?</a> | |||
There was a problem hiding this comment.
| <a class="log-in__form__reset" routerLink="/reset-password">Forget password?</a> | |
| <a class="log-in__form__reset" routerLink="/reset-password">Forgot password?</a> |
Also, since you have hard-coded text, you can add i18n="@@someUniqueIdHere" to this tag 👍
| &__reset { | ||
| display: flex; | ||
| justify-content: center; | ||
| color: #0f0e11; |
There was a problem hiding this comment.
Is it possible to reference a variable in the variables.scss rather than hard-coding the color here?
There was a problem hiding this comment.
Same thing here: is there a variable that you can reference for the colors in variables.scss ?
Also, having a fixed-width in pixels might make things difficult for adapting to screens. Could you specify the width in a percentage?
| { | ||
| path: 'reset-password/confirm/:token', | ||
| component: ResetPasswordConfirmationComponent, | ||
| canActivate: [redirectGuard, anonymousGuard], | ||
| }, |
There was a problem hiding this comment.
Non blocking comment: This works, but just a heads up that since this shares the same root with the ResetPasswordComponent, you can nest it as a child of ResetPasswordComponent.
| <mat-form-field class="reset-password__form__input"> | ||
| <mat-label i18n="@@resetPasswordConfirmationConfirmedPassword">Confirmed Password</mat-label> | ||
| <input matInput formControlName="confirmedPassword" type="password" appearance="outlined" (input)="update1()" /> | ||
| @if (resetPasswordConfirmationForm.controls['confirmedPassword'].hasError('passwordMismatch')) { |
There was a problem hiding this comment.
Since you are using a typed form, you can use some shortcuts 😉
| @if (resetPasswordConfirmationForm.controls['confirmedPassword'].hasError('passwordMismatch')) { | |
| @if (resetPasswordConfirmationForm.controls.confirmedPassword.hasError('passwordMismatch')) { |
|
|
||
| this.resetPasswordService | ||
| .resetPassword(this.resetPasswordForm.value.username, currentUrl) | ||
| .pipe(take(1)) |
There was a problem hiding this comment.
Heads up: best practice is to always include a takeUntilDestroyed(this.destroyRef) as the last step in a pipe. This guarantees that when the component is being destroyed, the subscription will be closed.
This can be done for the Observables across the other components you have added in this PR.
| console.log(password.value); | ||
| console.log(confirmedPassword.value); |
| const password = control.parent?.get('password'); | ||
| const confirmedPassword = control.parent?.get('confirmedPassword'); |
There was a problem hiding this comment.
This seems fairly specific to how the ResetPasswordComponent works (aka. it needs a parent, and the parent control needs to have a password and confirmedPassword controls). Is there a way to make this more generic or if not, then to have it better placed in order to be scoped to the ResetPasswordComponent?
| httpClientMock = { | ||
| post: jest.fn().mockReturnValue(of({})), | ||
| }; |
There was a problem hiding this comment.
Using mocks is fine, but we typically use the HttpTestingController to assess service calls. Specifically to test that:
- the correct URL has been called
- if it's a
POST, the correct request body has been sent - verify that we do not call the same endpoint multiple times
- verify that there are no requests outstanding
Could you please make sure to cover these points when using mock?
| ); | ||
| return JSON.parse(jsonPayload); | ||
| } catch (error) { | ||
| console.error('Failed to parse token:', error); |
Issue
https://gravitee.atlassian.net/browse/APIM-4338
Description
Add reset password feature.
Additional context
📚 View the storybook of this branch here