Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the teacher password reset flow, mirroring the existing student password reset functionality. The implementation adds JWT-based password reset tokens with SHA-256 hashing, updates the database schema with password reset fields and indexes, and adds corresponding API endpoints and UI components. However, the PR also includes several unrelated changes to forms, configuration files, and a removal of logout functionality that should be addressed.
Changes:
- Added teacher password reset flow with JWT token generation, email sending, and token verification
- Updated client to redirect users to their respective login pages (student/teacher) after successful password reset
- Fixed API bug where teacher password reset was calling the student endpoint
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/types/auth/auth.types.ts | Added PasswordResetTokenPayload type for JWT token structure |
| server/src/services/jwt/jwt.service.ts | Added methods to create and verify password reset JWT tokens |
| server/src/services/auth/auth.service.ts | Extended password reset logic to support both student and teacher roles |
| server/src/services/teacher/teacher.service.ts | Added passwordReset field initialization for new teachers |
| server/src/repositories/queryRepositories/teacher.query.ts | Added method to find teachers by reset token hash |
| server/src/repositories/commandRepositories/teacher.command.ts | Added methods to update password reset token and reset password |
| server/src/db/schemes/types/teacher.types.ts | Added passwordReset field to teacher type |
| server/src/db/schemes/teacherSchema.ts | Added passwordReset field and partial index for token lookups |
| server/src/routes/authRoute.ts | Added route for teacher password reset requests |
| server/src/controllers/auth.controller.ts | Added controller for teacher password reset requests |
| client/src/pages/resetPasswordPage/resetPasswordPage.tsx | Added role detection and redirect to respective login pages after reset |
| client/src/api/auth/auth.api.ts | Fixed teacher password reset endpoint (was calling student endpoint) and removed logoutApi |
| client/src/components/headerPrivate/TopBar.tsx | Removed logout button and modal (unrelated to password reset) |
| client/src/components/auth/signUpForm/SignUpForm.tsx | Changed form reset behavior and removed disabled state from submit button |
| client/src/components/auth/loginForm/LoginForm.tsx | Changed form reset behavior and removed disabled state from submit button |
| server/package-lock.json | Reordered dependencies (automatic npm formatting) |
| client/jest.config.js | Formatting changes (unrelated) |
| client/.babelrc | Formatting changes (unrelated) |
| client/setupTests.ts | Quote style change (unrelated) |
Files not reviewed (1)
- server/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const onSubmitForm = async (data: RegisterFormTypes) => { | ||
| try { | ||
| await onSubmit({ ...data, role }); | ||
| reset(); | ||
| } catch { | ||
| reset({ | ||
| firstName: data.firstName, | ||
| lastName: data.lastName, | ||
| email: data.email, | ||
| password: "", | ||
| }); | ||
| } catch { | ||
| // error is handled by react-query onError (toast), keep form values | ||
| } |
There was a problem hiding this comment.
The order of reset() and the catch block has been swapped, but the logic remains the same - reset is now called on success before the catch. However, the reset() inside the try block (line 42-47) will clear the password field on success, while the catch block comment says "keep form values" but does nothing. This appears to be a refactor unrelated to teacher password reset. Consider whether this behavioral change (resetting form values on success) is intentional and desired.
| type="submit" | ||
| disabled={loading} | ||
| > | ||
| <Button variant="secondary" size="auth" type="submit"> |
There was a problem hiding this comment.
The disabled prop has been removed from the submit button. This means the button will be clickable even while a submission is in progress, potentially allowing duplicate submissions. This change appears unrelated to the teacher password reset feature.
| <Button variant="secondary" size="auth" type="submit"> | |
| <Button variant="secondary" size="auth" type="submit" disabled={loading}> |
| type="submit" | ||
| disabled={loading} | ||
| > | ||
| <Button variant="secondary" size="auth" type="submit"> |
There was a problem hiding this comment.
The disabled prop has been removed from the submit button, similar to the SignUpForm. This could allow duplicate login attempts while a request is in progress. This change is unrelated to the teacher password reset feature.
| <Button variant="secondary" size="auth" type="submit"> | |
| <Button variant="secondary" size="auth" type="submit" disabled={loading}> |
| @@ -1,4 +1,4 @@ | |||
| { | |||
| "presets": ["@babel/preset-env", "@babel/preset-react"], | |||
| "presets": ["@babel/preset-env" , "@babel/preset-react"], | |||
There was a problem hiding this comment.
Formatting change to .babelrc (adding a space before the comma) is unrelated to the teacher password reset feature and should be in a separate PR focused on code formatting.
| // Polyfill for TextEncoder - Otherwise it creates an error 'TextEncoder' is not defined inside | ||
| // the react-router-dom library. | ||
| import { TextEncoder, TextDecoder } from "util"; | ||
| import { TextEncoder, TextDecoder } from 'util'; |
There was a problem hiding this comment.
Quote style change from double quotes to single quotes is unrelated to the teacher password reset feature and should be in a separate PR focused on code formatting or linting rules.
| <button className="text-[#474747] hover:text-[#8A8A8A] transition-colors"> | ||
| <HelpIcon className="w-[19.5px] h-[19.5px]" /> | ||
| </button> |
There was a problem hiding this comment.
The logout functionality has been removed from the TopBar component. This change appears unrelated to the teacher password reset feature described in the PR. If this removal is intentional, it should be documented in the PR description or moved to a separate PR. If not intentional, the logout button and modal should be restored.
| export async function requestPasswordResetStudentApi(data: { email: string }) { | ||
| await apiPublic.post("/api/auth/request-password-reset-student", data); | ||
| } | ||
|
|
||
| export async function requestPasswordResetTeacherApi(data: { email: string }) { | ||
| await apiPublic.post("/api/auth/request-password-reset-student", data); | ||
| await apiPublic.post("/api/auth/request-password-reset-teacher", data); |
There was a problem hiding this comment.
The logoutApi function has been removed from the exports. This change appears unrelated to the teacher password reset feature and is not mentioned in the PR description. The function is still used in useLogoutMutation.ts. If logout functionality is being deprecated, this should be a separate PR with proper cleanup of all dependent code.
| const onSubmitForm = async (data: LoginFormTypes) => { | ||
| try { | ||
| await onSubmit({ ...data, role }); | ||
| reset(); | ||
| } catch { | ||
| reset({ email: data.email, password: "" }); | ||
| } catch { | ||
| // error is handled by react-query onError (toast), keep form values | ||
| } |
There was a problem hiding this comment.
Similar to the SignUpForm, the order of reset() and catch block logic has been changed. The reset() now only keeps the email and clears the password on success. The catch block does nothing despite the comment saying "keep form values". This appears to be an unrelated refactor.
| export default { | ||
| testEnvironment: "jsdom", | ||
| setupFilesAfterEnv: ["./setupTests.ts"], | ||
| transform: { | ||
| "^.+\\.(t|j)sx?$": "babel-jest", | ||
| }, | ||
| moduleFileExtensions: ["ts", "tsx", "js", "jsx", "json"], | ||
| moduleNameMapper: { | ||
| "\\.(css|less)$": "<rootDir>/__mocks__/styleMock.js", | ||
| "\\.(jpg|jpeg|png|svg)$": "<rootDir>/__mocks__/fileMock.js", | ||
| }, | ||
| testEnvironment: "jsdom", | ||
| setupFilesAfterEnv: ["./setupTests.ts"], | ||
| transform: { | ||
| "^.+\\.(t|j)sx?$": "babel-jest" | ||
| }, | ||
| moduleFileExtensions: ["ts", "tsx", "js", "jsx", "json"], | ||
| moduleNameMapper: { | ||
| "\\.(css|less)$": "<rootDir>/__mocks__/styleMock.js", | ||
| "\\.(jpg|jpeg|png|svg)$": "<rootDir>/__mocks__/fileMock.js" | ||
| } | ||
| }; |
There was a problem hiding this comment.
Formatting changes to Jest configuration appear unrelated to the teacher password reset feature. These should ideally be in a separate PR focused on code formatting or linting setup.
| "uuid": "^13.0.0", | ||
| "nodemailer": "^8.0.1" |
There was a problem hiding this comment.
Reordering of dependencies in package-lock.json is unrelated to the teacher password reset feature. This appears to be an automatic reformatting by npm and could create merge conflicts unnecessarily.
I added the teacher password reset logic. It follows the same flow as the student forgot password process. Additionally, once the password is successfully reset for either a student or a teacher, the user is redirected to their respective login page.