Develop#292
Conversation
dotenv.config() belongs in src/index.js before any other require, not inside the db utility module where it acts as a hidden side effect.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has several critical gaps that block task completion. First, registration is missing the 'name' field - the task explicitly requires collecting "name, email and password", but the database schema, registration controller, user creation flow, and normalize function all lack name support. Second, the password reset flow is completely missing - no endpoints exist for POST /reset-password-request, GET /reset-password/:token, or POST /reset-password, and the mailer lacks sendPasswordResetLink. Third, Profile page backend doesn't exist - there are no endpoints for authenticated users to change their name, change password (with old password verification), or change email (with password confirmation and notification to old email). Fourth, no 404 handler exists for undefined routes as required. These are not minor issues - they are explicit requirements from the task that must be implemented for basic functionality.
✨ 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
| // This file was generated by Prisma, and assumes you have installed the following: | ||
| // npm install --save-dev prisma dotenv | ||
| import "dotenv/config"; |
There was a problem hiding this comment.
This file uses TypeScript syntax (import, export default) while the rest of the project appears to be JavaScript-based (e.g., src/utils/db.js uses CommonJS require). This TypeScript config may not be executable with standard Node.js without additional setup.
| -- CreateTable | ||
| CREATE TABLE "users" ( | ||
| "id" TEXT NOT NULL DEFAULT gen_random_uuid(), |
There was a problem hiding this comment.
The users table schema only has id, email, password, created_at, activation_token - but registration requires collecting 'name' which doesn't exist in the schema or user creation flow.
| -- CreateTable | ||
| CREATE TABLE "users" ( | ||
| "id" TEXT NOT NULL DEFAULT gen_random_uuid(), | ||
| "email" TEXT NOT NULL, | ||
| "password" TEXT NOT NULL, |
There was a problem hiding this comment.
Missing Profile page backend - the task requires endpoints to change name, change password (with old password verification), and change email (with password confirmation and notification to old email). No profile controller or routes exist.
|
|
||
| CONSTRAINT "tokens_pkey" PRIMARY KEY ("id") | ||
| ); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "tokens_user_id_key" ON "tokens"("user_id"); | ||
|
|
There was a problem hiding this comment.
The task requires password reset routes: POST /reset-password-request, GET /reset-password/:token, POST /reset-password. These routes are missing from the auth router.
| -- CreateTable | ||
| CREATE TABLE "tokens" ( | ||
| "id" SERIAL NOT NULL, | ||
| "user_id" TEXT NOT NULL, | ||
| "token" TEXT NOT NULL, | ||
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
|
|
||
| CONSTRAINT "tokens_pkey" PRIMARY KEY ("id") | ||
| ); | ||
|
|
||
| -- CreateIndex | ||
| CREATE UNIQUE INDEX "tokens_user_id_key" ON "tokens"("user_id"); | ||
|
|
||
| -- AddForeignKey |
There was a problem hiding this comment.
The task requires Profile page endpoints to: (1) change name, (2) change password with old password verification, (3) change email with password confirmation and notification to old email. The users controller only has getAll - no profile management functionality exists.
| const users = await usersRepository.getAllActive(); | ||
|
|
||
| res.json(users.map(userService.normalize)); | ||
| }; | ||
|
|
||
| const usersController = { getAll }; | ||
|
|
||
| module.exports = { usersController }; |
There was a problem hiding this comment.
The task requires password reset endpoints that don't exist in this router. Need routes for: resetting password with token, sending reset email, etc.
| res.json({ accessToken, user: userData }); | ||
| } | ||
|
|
||
| const register = async (req, res) => { |
There was a problem hiding this comment.
The registration function only extracts 'email' and 'password' from req.body, but the task requires collecting 'name, email and password'. The 'name' field is missing from extraction, validation, and database creation.
| } | ||
|
|
||
| const activationToken = randomBytes(32).toString('hex'); | ||
|
|
There was a problem hiding this comment.
The user is created without a 'name' field, but the task requires registration to collect name. Either the database schema needs a name column, or this needs to be added to the create call.
|
|
||
| const authController = { | ||
| register, | ||
| activate, | ||
| login, | ||
| refresh, | ||
| logout, |
There was a problem hiding this comment.
Password reset flow is completely missing from the controller. The task requires: (1) Ask for email endpoint, (2) Show email sent page, (3) Reset password confirmation with password/confirmation fields that must be equal, (4) Success page with link to login. None of these endpoints or controller methods exist.
| 'use strict'; | ||
|
|
||
| const { randomBytes } = require('crypto'); | ||
| const bcrypt = require('bcrypt'); | ||
| const { usersRepository } = require('../entity/users.repository.js'); | ||
| const { mailer } = require('../utils/mailer.js'); | ||
| const { userService } = require('../services/user.service.js'); | ||
| const { jwt } = require('../utils/jwt.js'); | ||
| const { tokensRepository } = require('../entity/tokens.repository.js'); | ||
|
|
||
| const SALT_ROUNDS = 10; | ||
|
|
||
| async function sendAuthentication(res, user) { | ||
| const userData = userService.normalize(user); | ||
| const accessToken = jwt.generateAccessToken(userData); |
There was a problem hiding this comment.
No profile controller or endpoints exist for authenticated users to change their name, password (with old password verification), or email (with password confirmation and notification to old email) as required by the task.
|
|
||
| const usersRouter = Router(); | ||
|
|
||
| usersRouter.get('/', authMiddleware, usersController.getAll); | ||
|
|
There was a problem hiding this comment.
The task requires Profile page functionality to change name, password, and email - but the users router only has a GET / endpoint for listing users. No profile management routes exist (e.g., PUT /profile for updating user details).
| function create(userId, token) { | ||
| return db.token.create({ | ||
| data: { userId, token }, | ||
| }); |
There was a problem hiding this comment.
The task requires Profile page endpoints to change name, password (with old password), and email (with password confirmation and old email notification) for authenticated users. These routes are not implemented - only GET / is present.
|
|
||
| app.use( | ||
| cors({ | ||
| origin: process.env.CLIENT_URL, | ||
| credentials: true, |
There was a problem hiding this comment.
The usersRepository.create function signature is create(email, password, activationToken) - but the task requires collecting 'name' during registration, so this needs to accept a name parameter.
|
|
||
| app.use('/auth', authRouter); | ||
| app.use('/users', usersRouter); | ||
|
|
||
| module.exports = { app }; |
There was a problem hiding this comment.
The app.js has no catch-all route handler for undefined routes. The task requires returning 404 for all pages not defined above.
| 'use strict'; | ||
|
|
||
| const { db } = require('../utils/db.js'); | ||
|
|
||
| function getByEmail(email) { | ||
| return db.user.findUnique({ | ||
| where: { email }, | ||
| }); | ||
| } | ||
|
|
||
| function create(email, password, activationToken) { |
There was a problem hiding this comment.
The users router only has a GET / route for listing users. The task requires Profile page endpoints to change name, change password (with old password verification), and change email (with password confirmation and notification to old email). No profile update routes exist.
| 'use strict'; | ||
|
|
||
| const { db } = require('../utils/db.js'); | ||
|
|
||
| function getByEmail(email) { | ||
| return db.user.findUnique({ | ||
| where: { email }, | ||
| }); | ||
| } | ||
|
|
||
| function create(email, password, activationToken) { | ||
| return db.user.create({ | ||
| data: { email, password, activationToken }, | ||
| }); | ||
| } | ||
|
|
||
| function activate(email) { | ||
| return db.user.update({ | ||
| where: { email }, | ||
| data: { activationToken: null }, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
The app has no catch-all 404 handler. The task requires returning 404 for all pages not defined above (registration, activation, login, logout, password reset, profile).
|
|
||
| function create(email, password, activationToken) { | ||
| return db.user.create({ | ||
| data: { email, password, activationToken }, |
There was a problem hiding this comment.
The create function only accepts email, password, and activationToken parameters. The task requires registration to collect 'name, email and password', so the create function needs to accept and store a name field.
|
|
||
| const usersRepository = { | ||
| getByEmail, | ||
| create, | ||
| activate, | ||
| getAllActive, | ||
| }; | ||
|
|
||
| module.exports = { usersRepository }; |
There was a problem hiding this comment.
The usersRepository is missing update methods needed for the Profile page: updateName (for changing name), updatePassword (for changing password with old password verification), and updateEmail (for changing email with password confirmation).
|
|
||
| function normalize({ id, email }) { | ||
| return { id, email }; |
There was a problem hiding this comment.
The normalize function only returns { id, email }, but the task requires collecting 'name' during registration, so the normalized user object should include name.
|
|
||
| const userService = { | ||
| normalize, | ||
| validateEmail, | ||
| validatePassword, |
There was a problem hiding this comment.
The task requires password reset confirmation and profile password change to validate that 'password' and 'confirmation' fields are equal. A validatePasswordConfirmation(password, confirmation) function is missing.
|
|
||
| const userService = { | ||
| normalize, | ||
| validateEmail, | ||
| validatePassword, |
There was a problem hiding this comment.
The task requires collecting 'name' during registration and allowing name changes on Profile page, but validateName(name) function is missing from the service.
| function authMiddleware(req, res, next) { | ||
| const authHeader = req.headers['authorization']; | ||
| const [, accessToken] = (authHeader || '').split(' '); |
There was a problem hiding this comment.
The user.service.js normalize function only returns {id, email}, but since the task requires collecting 'name' during registration, normalize should also include the name field.
| const db = new PrismaClient(); | ||
|
|
||
| module.exports = { db }; |
There was a problem hiding this comment.
The normalize function only returns { id, email } but should include 'name' since the task requires collecting name during registration and the user object should be fully representable.
| const { app } = require('./app.js'); | ||
|
|
There was a problem hiding this comment.
The normalize function only returns { id, email } but should also return 'name' since the task requires collecting name during registration and the Profile page should display/manage it.
|
|
||
| const jwt = { | ||
| generateAccessToken, | ||
| validateAccessToken, |
There was a problem hiding this comment.
The task requires a password reset flow that includes 'Show email sent page' - meaning the system must send a password reset email. A sendPasswordResetLink(email, resetToken) function is missing from the mailer module.
|
|
||
| return send(email, 'Account activation', html); | ||
| } | ||
|
|
There was a problem hiding this comment.
The mailer.js has sendActivationLink but is missing sendPasswordResetLink function required for the password reset flow. Need a function to send password reset emails with a reset token link.
|
|
||
| return send(email, 'Account activation', html); | ||
| } | ||
|
|
There was a problem hiding this comment.
The task requires Profile page email change to 'notify the old email about the change'. Need a sendEmailChangeNotification function that sends notification to the old email address when the user changes their email.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes to address several critical requirements that are not implemented as specified in the task description.
Critical Issues to Fix:
-
Login inactive account feedback (auth.controller.js line ~96): When
user.activationToken !== null, the code returns generic "Invalid credentials" instead of a specific message prompting the user to activate their email. The task explicitly states: "If user is not active ask them to activate their email". Return something like "Please activate your email first" for inactive accounts. -
Password reset missing confirmation field (auth.controller.js line ~160): The
resetPasswordendpoint only destructurespasswordfrom the request body, but the task explicitly requires: "Reset Password confirmation page (withpasswordandconfirmationfields that must be equal)". Extractconfirmationfrom the body and validate thatpassword === confirmationbefore updating. -
Profile password change missing confirmation (profile.controller.js
updatePassword): The task states it requires "new password and confirmation", but onlyoldPasswordandnewPasswordare extracted. AddnewPasswordConfirmationand validate equality before hashing. -
Profile email change missing confirmation (profile.controller.js
updateEmail): The task states: "To change an email you should type the password, confirm the new email", but onlypasswordandnewEmailare extracted. AddnewEmailConfirmationand validate thatnewEmail === newEmailConfirmationbefore proceeding.
What was done well: The core authentication flow (registration, activation, login, logout), JWT middleware, database schema with all required fields, and email sending functions are all correctly implemented.
Please update these four functions to include the missing confirmation field validations as specified in the requirements.
✨ 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
|
|
||
| if (!user || !isPasswordValid || user.activationToken !== null) { |
There was a problem hiding this comment.
The task requirement states: 'If user is not active ask them to activate their email'. Currently, when user.activationToken !== null, line 96 returns generic 'Invalid credentials'. Consider returning a specific message like 'Please activate your email first' when detecting an inactive account.
| }; | ||
|
|
||
| const resetPassword = async (req, res) => { | ||
| const { token } = req.params; |
There was a problem hiding this comment.
The task requirement explicitly states: 'add Reset Password confirmation page (with password and confirmation fields that must be equal)'. Currently, line 160 only destructures password from the body. You need to also extract confirmation, validate that password === confirmation, and return an error if they don't match.
|
|
||
| const updatePassword = async (req, res) => { |
There was a problem hiding this comment.
The task requirement states: 'It allows to change a password (require an old one, new password and confirmation)'. Currently only oldPassword and newPassword are required. Add a confirmation field extraction and validate that newPassword === confirmation before proceeding.
|
|
||
| const updateEmail = async (req, res) => { |
There was a problem hiding this comment.
The task requirement states: 'To change an email you should type the password, confirm the new email'. Currently only password and newEmail are required. Add newEmailConfirmation field extraction and validate that newEmail === newEmailConfirmation before proceeding.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution — your backend now cleanly implements registration with validation and activation emails, login/logout with JWT, password reset with confirmation, and profile updates (name, password, email) including all the extra confirmation and notification logic [CHECKLIST ITEMS #1–3, #8, #13, #15, #18–20, #26–32]. The structure with controllers, repositories, services, middleware, and utilities is clear and consistent, and your Prisma migrations, JWT handling, and mailer integration all line up correctly with how the routes behave.
The only notable architectural nuance is that “only non‑authenticated” constraints for registration, login, and password reset are enforced on the frontend rather than with backend middleware, and logout is not protected by authMiddleware; this is acceptable for this educational task, but worth reflecting on if you later want stricter server‑side access control [related to CHECKLIST ITEMS #4, #6, #9–12]. As an educational note to help you progress, you could experiment with optional middleware that blocks these routes when a valid access token is present, just to deepen your understanding of access control patterns. Overall, this is a solid, coherent implementation that satisfies the required flows, and you’ve handled the tricky parts (token lifecycles, confirmations, and status messages) very well — keep building on this foundation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.