Skip to content
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

#187354251 Update / changing user password #26

Closed
wants to merge 3 commits into from

Conversation

P-Rwirangira
Copy link
Collaborator

@P-Rwirangira P-Rwirangira commented Apr 24, 2024

Purpose

The purpose of the following PR is to help the user to change his/her password but not when forgotten the first password

Changes Made

-Added a controller in userController.ts for updating/changing user password
-Added API endpoint for updating/changing password in userRouter.js
-Added tests

Testing Instructions

Check on localhost:{PORT}/users/update-password (On Postman use POST method)

Related Issues

No issues at the moment

Checklist

Please review the following checklist and make sure all tasks are complete before submitting:

  • Code follows the project's coding standards
  • Changes are covered by tests
  • Documentation is updated (if applicable)
  • All tests pass

@P-Rwirangira P-Rwirangira added bug Something isn't working ready and removed bug Something isn't working labels Apr 24, 2024
@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch from 21dc179 to 7ba2e95 Compare April 25, 2024 20:29
@@ -33,5 +34,38 @@ const createUser = async (req: Request, res: Response): Promise<void> => {
res.status(500).send('Internal Server Error');
}
};
const updatePassword = async (req: Request, res: Response): Promise<void> => {
try {
const { oldPassword, newPassword, email } = req.body;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot request an email from a user who is already signed in. Instead, explore methods for changing or updating the password using various resources. Retrieve the email from the token or from the request object appended by an authentication middleware.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to extracting id from the token

Comment on lines 41 to 64
const user = await User.findOne({
where: {
email: email,
},
});

if (!user) {
res.status(400).send('User not found');
return;
}
const match = await bcrypt.compare(oldPassword, user.password);
if (!match) {
res.status(400).send('The old password is incorrect!');
}
const hashedNewPassword = await bcrypt.hash(newPassword, saltRound);
await User.update(
{ password: hashedNewPassword },
{
where: {
email: email,
},
}
);
res.status(200).json({ message: 'Successfully updated user password!' });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done, but please ensure you include ok: true or ok: false in the response. For example

 res.status(400).json({
    ok: false,
    message: 'User not found'
 );

Please avoid using send() and use json() instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 66 to 67
logger.error('Error updating user:', error);
res.status(500).send('Internal Server Error');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the reusable function sendInternalErrorResponse from src/validations. Refer to how other controller functions have utilized it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to using the reusable fucntions

Comment on lines 57 to 83
api/users/update-password:
put:
summary: Update user password
consumes:
- application/json
produces:
- application/json
parameters:
- in: body
name: passwordUpdate
description: Password update data
required: true
schema:
type: object
properties:
oldPassword:
type: string
newPassword:
type: string
email:
type: string
responses:
'200':
description: Successfully updated user password
schema:
type: object
properties:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done in the auth.yml

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 2 to 30
import { Router } from 'express';
import { createUser } from '../controllers/userController';
import { createUser, updatePassword } from '../controllers/userController';

const router = Router();

router.post('/', createUser);

router.put('/update-password', updatePassword);
export default router;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be in th authRoute.ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch 3 times, most recently from 05cc1a2 to feb4cbc Compare April 26, 2024 10:50
Copy link
Collaborator

@hozayves hozayves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes Patrick

Great work on the PR! Just a couple of suggestions:

  1. It might be beneficial to move the updatePassword controller into the user controller file for better code organization.
  2. Consider capturing the email from the logged-in user using the auth middleware from @JeanIrad instead of from the request body. This can enhance security and align with best practices for handling user data.

@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch 2 times, most recently from b9024c9 to 1d7a5d4 Compare April 26, 2024 13:42
@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch 2 times, most recently from bf572db to e95789e Compare April 28, 2024 11:28
@hozayves hozayves self-requested a review April 30, 2024 11:19
@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch 4 times, most recently from 86204b3 to 9f670eb Compare April 30, 2024 16:27
@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch from 9f670eb to 39f3855 Compare April 30, 2024 16:30
@P-Rwirangira P-Rwirangira force-pushed the 187354251-ft-update-password branch from 39f3855 to 13e35e1 Compare April 30, 2024 17:12
@P-Rwirangira P-Rwirangira deleted the 187354251-ft-update-password branch April 30, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants