- Notifications
You must be signed in to change notification settings - Fork1.1k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
BrunoQuaresma commentedOct 16, 2024
MrPeacockNLB commentedOct 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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.', |
bcpeinhardtOct 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.| exportconstrequestOneTimePassword=()=>{ | ||
| 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.
johnstcn left a comment
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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 inmain.
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.
johnstcn commentedOct 17, 2024
@MrPeacockNLB All good advice! There's more context on the design of the feature in#14232 if you're interested in looking futher. |
mtojek left a comment
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 👍
aaa1223 intomainUh oh!
There was an error while loading.Please reload this page.
Demo:
Screen.Recording.2024-10-16.at.14.00.35.mp4