Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a really strong submission, and you've correctly implemented almost all of the required functionality. The authentication flow, in particular, is well-structured with a clear separation between the controller and service layers.
However, I'm requesting a few changes to address a key architectural inconsistency. The business logic for all profile management is currently implemented directly within src/controllers/profile.controller.js, while the src/services/profile.service.js file is left unused. To maintain the great separation of concerns you established elsewhere, please move this logic into the service file.
A similar, smaller issue is present in src/controllers/auth.controller.js, where the activate function's logic should also be moved to auth.service.js.
Fixing this will make your project's architecture consistent and much more maintainable. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| export async function activate(req, res, next) { | ||
| try { | ||
| const { token } = req.params; | ||
|
|
||
| const activationToken = await prisma.token.findUnique({ | ||
| where: { token }, | ||
| }); | ||
|
|
||
| if (!activationToken) { | ||
| return res.status(404).json({ message: 'Invalid activation token' }); | ||
| } | ||
|
|
||
| await prisma.user.update({ | ||
| where: { id: activationToken.userId }, | ||
| data: { isActive: true }, | ||
| }); | ||
|
|
||
| await prisma.token.delete({ | ||
| where: { id: activationToken.id }, | ||
| }); | ||
|
|
||
| res.json({ message: 'Account activated successfully' }); | ||
| } catch (error) { | ||
| next(error); | ||
| } | ||
| } |
There was a problem hiding this comment.
While this implementation works, the business logic for activating a user (finding the token, updating the user, deleting the token) is directly in the controller. For better separation of concerns and consistency with other functions in this file (like register and login), this database interaction logic should be moved into a corresponding function in auth.service.js.
| import bcrypt from 'bcrypt'; | ||
| import prisma from '../prisma/prismaClient.js'; | ||
|
|
||
| const SALT_ROUNDS = 10; | ||
|
|
||
| export async function updateName(userId, name) { | ||
| if (!name || !name.trim()) { | ||
| throw new Error('Name is required'); | ||
| } | ||
|
|
||
| const user = await prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: { name: name.trim() }, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| email: true, | ||
| isActive: true, | ||
| }, | ||
| }); | ||
|
|
||
| return user; | ||
| } | ||
|
|
||
| export async function updatePassword(userId, oldPassword, newPassword) { | ||
| if (!oldPassword || !newPassword) { | ||
| throw new Error('Passwords are required'); | ||
| } | ||
|
|
||
| const user = await prisma.user.findUnique({ | ||
| where: { id: userId }, | ||
| }); | ||
|
|
||
| const isValid = await bcrypt.compare(oldPassword, user.passwordHash); | ||
|
|
||
| if (!isValid) { | ||
| throw new Error('Old password is incorrect'); | ||
| } | ||
|
|
||
| if (newPassword.length < 8) { | ||
| throw new Error('Password must be at least 8 characters'); | ||
| } | ||
|
|
||
| const passwordHash = await bcrypt.hash(newPassword, SALT_ROUNDS); | ||
|
|
||
| await prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: { passwordHash }, | ||
| }); | ||
|
|
||
| return { message: 'Password updated successfully' }; | ||
| } | ||
|
|
||
| export async function updateEmail(userId, password, newEmail) { | ||
| if (!password || !newEmail) { | ||
| throw new Error('Password and new email are required'); | ||
| } | ||
|
|
||
| const user = await prisma.user.findUnique({ | ||
| where: { id: userId }, | ||
| }); | ||
|
|
||
| const isValid = await bcrypt.compare(password, user.passwordHash); | ||
|
|
||
| if (!isValid) { | ||
| throw new Error('Password is incorrect'); | ||
| } | ||
|
|
||
| const existing = await prisma.user.findUnique({ | ||
| where: { email: newEmail }, | ||
| }); | ||
|
|
||
| if (existing) { | ||
| throw new Error('Email already taken'); | ||
| } | ||
|
|
||
| await prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: { email: newEmail }, | ||
| }); | ||
|
|
||
| return { message: 'Email updated successfully' }; | ||
| } |
There was a problem hiding this comment.
This service file appears to be entirely unused. The business logic for profile management (updating name, password, email) is implemented directly within src/controllers/profile.controller.js. To maintain a consistent architecture with a clear separation of concerns, the logic from the controller should be moved into the functions within this service file.
src/services/profile.service.js
Outdated
| if (newPassword.length < 8) { | ||
| throw new Error('Password must be at least 8 characters'); |
There was a problem hiding this comment.
The password validation here is incomplete as it only checks the length. It should use the validatePassword utility from src/utils/passwordValidator.js to enforce all the required password complexity rules.
| await prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: { email: newEmail }, | ||
| }); |
There was a problem hiding this comment.
This function's logic does not meet the task requirements. It updates the email address directly in the database, but the requirements specify a confirmation process where a link is sent to the new email address to verify the change.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! Your solution is approved as you've successfully refactored the codebase to move all business logic into the service layer, fully addressing the previous feedback. The separation of concerns between your controllers and services is now robust and consistent across the entire application, meeting all core requirements.
As a minor point for future improvement, consider moving the password confirmation check from the passwordResetConfirm function in src/controllers/auth.controller.js into the confirmPasswordReset function in src/services/auth.service.js. This would make the placement of your validation logic fully consistent with the pattern used in the profile service.
Overall, this is a fantastic submission. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.