-
Notifications
You must be signed in to change notification settings - Fork 19
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
Forget password restructured #140
Conversation
…th restructured files - frontend
…th restructured files - backend
…nd use a Response Type for cleaner error handling as per James
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.
2 small things but otherwise ready to go 🚀
server/src/users/users.controller.ts
Outdated
@@ -17,6 +29,16 @@ export class UsersController { | |||
return user; | |||
} | |||
|
|||
@Post('reset_password') | |||
async resetPassword(@Request() req, @Response({ passthrough: true }) response: ResponseT) { |
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.
oh one more thing here, return typing
i think should be ): Promise<void> {
since we're not returning anything
nbd if we don't have it, just a nice to have
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.
great. done.
value={formData.email} | ||
placeholder="[email protected]" | ||
onChange={handleChange} | ||
error={error?.type === 'email' ? error.message : null} |
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.
right now your error state is always null, because you don't have a setError
ever being called (which would come from the useState
line)
i say just hardcode pass in null
for now, remove that error state from here, and write up an issue to move the error state management to live inside the EmailInput, not ideal to have to rewrite that everywhere
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.
makes sense. Thanks. Changed.
…lin ForgetPassword.tsx set error as null in EmailInput to ease changes when error handeling is set inside EmailInput entirely
Made these two fixes. I'll archive the old PR and branch once we/I merge this into main. |
Dev Summary
This replaces PR 138 - forget password
Because all the client files had moved it felt easier (?) to just make a new branch and bring my changes in since it hit far fewer files.
I addressed issues that were on PR 138 (except where noted - see below)
This is the first part fo the Password Reset Flow issue (I didn't link it because this PR shouldn't close that issue as it will only be half completed)
client - front end files
src/features/users/Auth/ForgetPassword.tsx - created file, based on Login.tsx - prompts for email, once it gets an email should display a snackbar saying message is sent and remove field to enter password. This response should not let the user know this the email was found in the database. it then routes the form data (email) to api/users/reset_password on backend
src/routes.tsx - modified Routes to go to ForgetPassword.tsx when user clicks Forgot Password link from login.tsx page
src/features/action/assests/SimpleSnackbar.tsx - modified this as it had a hardcoded message. Now it takes in a message to be displayed (but defaults to "You Claimed This!" so as not to effect the other page that uses this function
Additionally now I have changed:
src/features/Main.tsx - to handle the more dynamic route set up we are using for ForgetPassword.
src/features/users/Auth/PasswordInput.tsx - changed the Forget Password link to use the dynamic ability to grab it from routes.tsx
server - backend files
src/users/users.controller.ts - importer Request and created a route for Post('reset_password'). It takes the email and attempts to find the users using already created function from user.serve findByEmail.
If it finds the user. Right now it doesn't do anything (though I tested that it knows if it finds the user via console.log's - but I commented those out.
PREVIOUS PR CHANGES
use kabab case for routes - this didn't need to be fixed as they got fixed in ynda's changes and I matched that.
README.md mistake - cleaned up since I rebranched and didn't keep my mistake change to this file
correct props typing in SimpleSnackbar - fixed
change the handling of my resetPassword method in the UserController to use a cleaner Response method in error handling and slot the eventually email sending in the try piece of code. - fixed and removed all old comments too.
clear commented out imports and code in ForgotPassword.tsx (leftover from what I copied from Login.tsx) - fixed
clear the email field (form data) after form is submitted - fixed
Modify button to change it's text while loading (instead of a seperate piece of text below the button) - fixed
Don't resolve error handling of email form - not fixed. The PasswordInput.tsx file is looking for that to sent, and so I left it until such time as I (or someone) can fix it in other places that the component is called.
NEXT STEPS
Need to send an email to the user if the user is found. This email will need to have a token so it can send that along to the change password page (not created yet).
The frontend change password page needs to be created as well as backend route to do that process.
Some of my research shows that I think we might need to create a table to hold these tokens temporarily. So when they are created they are stored and can be compared when the password update request comes in. These tokens should be removed after a time as well as removed if the password is updated.
James seems to think this is not needed and he's most likely right! But I link to one example (though it's node.js) that does create a table with the temporary toknes below in resources.
Test Plan
On the front end you can click Forgot Password (from the login screen) and be taken to a new page to enter your email. Regardless of what email you entered you should get the same result.
(backend should only find the user if you enter an email that exists in your database - but right now that does not change functionality)
Resources
This is for node.js but I think a similar methodology will be needed for nest.js
https://blog.logrocket.com/implementing-a-secure-password-reset-in-node-js/