-
Notifications
You must be signed in to change notification settings - Fork 926
feat(site): add forgot password link #15108
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
Conversation
@bpmct hint: always implement such a feature with care. Please consider to double check such feature on both sides frontend and backend. E.g.:
Just my 2 cents... |
UPDATE notification_templates | ||
SET | ||
title_template = E'Reset your password for Coder', | ||
body_template = E'Hi {{.UserName}},\n\nA request to reset the password for your Coder account has been made.\n\nIf you did not request to reset your password, you can ignore this message.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you did not request to reset your password, you can ignore this message.
Did this language come from a developer or product?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! I just refactored the body template using the previous verbiage, but let's check with @stirby.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify.
Hi {{.UserName}},\n\nUse the link below to reset your password.\n\nIf you did not make this request, you can ignore this message.
export const requestOneTimePassword = () => { | ||
return { | ||
mutationFn: (req: RequestOneTimePasscodeRequest) => | ||
API.requestOneTimePassword(req), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could we pass the function directly as below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, when we do that, we are not able to spy on the method. I'm not sure why, but we have encountered this issue in jest as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weeeeird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a couple of small issues with the migration to update the template text. Other than that, I don't see any issues once those are addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chore: you can also delete this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chore: you can delete this file. We should probably gitignore this path in future.
coderd/database/migrations/000264_update_forgot_password_notification.down.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chore (blocking): Please increment the migration number to one higher than what is currently in main
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use ./coderd/database/migrations/fix_migration_numbers.sh main
to do this automatically, btw.
@MrPeacockNLB All good advice! There's more context on the design of the feature in #14232 if you're interested in looking futher. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it 👍
Demo:
Screen.Recording.2024-10-16.at.14.00.35.mp4