feature/#38 Added reset password endpoint #39
+67
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
Adds a new change-password endpoint that validates the old password, hashes the new one, and updates the user record.
- Introduces
validationChangePasswordto enforce required fields and verify the current password. - Exposes the validator in the validations index and integrates it into the REST API.
- Extends the API model with
ChangePasswordParamsand adds a new internal error code.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pods/user/validations/password-validation.ts | New validationChangePassword function with field checks and hash verification. |
| src/pods/user/validations/index.ts | Exports the new validationChangePassword validator. |
| src/pods/user/user.api-model.ts | Defines ChangePasswordParams interface. |
| src/common/custom-error/custom-error.model.ts | Adds InvalidPassword to CustomInternalCodes. |
| src/pods/user/user.rest-api.ts | Adds /change-password route, applies validation, hashing, and status mapping. |
Comments suppressed due to low confidence (2)
src/pods/user/user.api-model.ts:33
- [nitpick] Using Spanish identifiers with accents (e.g.,
contraseñaActual) may cause encoding or consistency issues; consider using English names likecurrentPassword.
contraseñaActual: string;
src/pods/user/user.rest-api.ts:120
- The new
/change-passwordendpoint and its validation logic lack accompanying tests; consider adding unit and integration tests to cover the success and error flows.
.post('/change-password', async (req, res, next) => {
Comment on lines
+10
to
+23
| return { succeded: false, error: { error: CustomInternalCodes.FieldNotInformed } }; | ||
| } | ||
|
|
||
| const user = await userRepository.getUser(passwordData.id); | ||
| if (!user) { | ||
| return { succeded: false, error: { error: CustomInternalCodes.UserNotFound } }; | ||
| } | ||
|
|
||
| const isValidPassword = await verifyHash(passwordData.contraseñaActual, user.contraseña); | ||
| if (!isValidPassword) { | ||
| return { succeded: false, error: { error: CustomInternalCodes.InvalidPassword } }; | ||
| } | ||
|
|
||
| return { succeded: true }; |
There was a problem hiding this comment.
The property succeded appears misspelled; it should likely be succeeded to match common spelling and avoid mismatches with the ValidationInfo interface.
Suggested change
| return { succeded: false, error: { error: CustomInternalCodes.FieldNotInformed } }; | |
| } | |
| const user = await userRepository.getUser(passwordData.id); | |
| if (!user) { | |
| return { succeded: false, error: { error: CustomInternalCodes.UserNotFound } }; | |
| } | |
| const isValidPassword = await verifyHash(passwordData.contraseñaActual, user.contraseña); | |
| if (!isValidPassword) { | |
| return { succeded: false, error: { error: CustomInternalCodes.InvalidPassword } }; | |
| } | |
| return { succeded: true }; | |
| return { succeeded: false, error: { error: CustomInternalCodes.FieldNotInformed } }; | |
| } | |
| const user = await userRepository.getUser(passwordData.id); | |
| if (!user) { | |
| return { succeeded: false, error: { error: CustomInternalCodes.UserNotFound } }; | |
| } | |
| const isValidPassword = await verifyHash(passwordData.contraseñaActual, user.contraseña); | |
| if (!isValidPassword) { | |
| return { succeeded: false, error: { error: CustomInternalCodes.InvalidPassword } }; | |
| } | |
| return { succeeded: true }; |
Copilot uses AI. Check for mistakes.
Positive FeedbackNegative Feedback
|
|
||
| if (validationResult.succeded) { | ||
| const hashedPassword = await hash(passwordData.nuevaContraseña); | ||
| const user = await userRepository.getUser(passwordData.id); |
There was a problem hiding this comment.
[nitpick] This call fetches the user again after validation already retrieved it; you could return the user from the validation step to reduce an extra database query.
Copilot uses AI. Check for mistakes.
Positive FeedbackNegative Feedback
| res.sendStatus(204); | ||
| } else { | ||
| const statusCode = (() => { | ||
| switch (validationResult.error?.error) { |
There was a problem hiding this comment.
The FieldNotInformed case currently falls through to the default status 400; mapping it explicitly to 422 (like UserNotFound) may better communicate input validation errors.
Copilot uses AI. Check for mistakes.
Positive FeedbackNegative Feedback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closed #38