Skip to content

Add FE for password-reset verification#2795

Merged
gbrodman merged 2 commits into
google:masterfrom
gbrodman:resetVerify
Aug 19, 2025
Merged

Add FE for password-reset verification#2795
gbrodman merged 2 commits into
google:masterfrom
gbrodman:resetVerify

Conversation

@gbrodman

@gbrodman gbrodman commented Aug 7, 2025

Copy link
Copy Markdown
Collaborator

Tested locally and on alpha with dummy values (and throwing an exception).

I was able to reuse a bit of code from the EPP password reset, but not all of it.


This change is Reviewable

@gbrodman
gbrodman requested a review from ptkach August 8, 2025 17:07

@ptkach ptkach left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 2 of 9 files at r1, all commit messages.
Reviewable status: 2 of 9 files reviewed, 1 unresolved discussion


console-webapp/src/app/settings/security/eppPasswordEdit.component.ts line 61 at r1 (raw file):

  standalone: false,
})
export default class EppPasswordEditComponent extends PasswordUpdateFormComponent {

This is a little unconventional, typically you'd extract shared functionality into a new component like say PasswordResetForm with it's own html and css and ts and then just insert it into both epp and password-reset-verify as a tag like < password-reset-form />. The reason is it avoids duplication of html and css, which in your case seems to be duplicated (html) . I basically did similar with UsersListComponent so you can check it out for example.

Code quote:

 extends PasswordUpdateFormComponent

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reviewable status: 2 of 9 files reviewed, 1 unresolved discussion (waiting on @ptkach)


console-webapp/src/app/settings/security/eppPasswordEdit.component.ts line 61 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

This is a little unconventional, typically you'd extract shared functionality into a new component like say PasswordResetForm with it's own html and css and ts and then just insert it into both epp and password-reset-verify as a tag like < password-reset-form />. The reason is it avoids duplication of html and css, which in your case seems to be duplicated (html) . I basically did similar with UsersListComponent so you can check it out for example.

I was considering that / considering more shared code but I didn't see a way of doing that given that the restrictions on the forms are not the same. For instance, EPP passwords must be 6-16 characters long whereas lock passwords can be any length.

(plus things like language, error codes, etc may be different)

Also, this (obviously) doesn't require putting in the old password whereas the other one does. Thoughts?

@ptkach ptkach left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewable status: 2 of 9 files reviewed, 1 unresolved discussion (waiting on @gbrodman)


console-webapp/src/app/settings/security/eppPasswordEdit.component.ts line 61 at r1 (raw file):

Previously, gbrodman wrote…

I was considering that / considering more shared code but I didn't see a way of doing that given that the restrictions on the forms are not the same. For instance, EPP passwords must be 6-16 characters long whereas lock passwords can be any length.

(plus things like language, error codes, etc may be different)

Also, this (obviously) doesn't require putting in the old password whereas the other one does. Thoughts?

Yeah I realize that - that's usually done in one of 2 ways:

  1. you keep business logic of the component and pass things like formGroup: FormGroup and showOldPasswordField:boolean as a parameters, which then affect how component renders html and builds formFields. Then out to EppPassword and PasswordResetVerify it passes events with password values
  2. you keep it aware of business logic and just pass down "flavor" as a param and allow it render html and provide formGroup accordingly.

Either works and leads to a minimum duplication

Tested locally and on alpha with dummy values (and throwing an
exception).

I was able to reuse a bit of code from the EPP password reset, but not
all of it.

@gbrodman gbrodman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reviewable status: 1 of 14 files reviewed, 1 unresolved discussion (waiting on @ptkach)


console-webapp/src/app/settings/security/eppPasswordEdit.component.ts line 61 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

Yeah I realize that - that's usually done in one of 2 ways:

  1. you keep business logic of the component and pass things like formGroup: FormGroup and showOldPasswordField:boolean as a parameters, which then affect how component renders html and builds formFields. Then out to EppPassword and PasswordResetVerify it passes events with password values
  2. you keep it aware of business logic and just pass down "flavor" as a param and allow it render html and provide formGroup accordingly.

Either works and leads to a minimum duplication

I ended up doing what I believe you meant by option number 1. Thoughts on this?

I do think this is probably better.

@ptkach ptkach left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 2 of 9 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman)


console-webapp/src/app/settings/security/eppPasswordEdit.component.ts line 61 at r1 (raw file):

Previously, gbrodman wrote…

I ended up doing what I believe you meant by option number 1. Thoughts on this?

I do think this is probably better.

That's exactly it. Looks better to me.

@gbrodman
gbrodman enabled auto-merge August 19, 2025 01:59
@gbrodman
gbrodman added this pull request to the merge queue Aug 19, 2025
Merged via the queue into google:master with commit 4738b97 Aug 19, 2025
8 of 9 checks passed
@gbrodman
gbrodman deleted the resetVerify branch August 19, 2025 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants