add solution#284
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has a critical bug in src/services/token.service.js (lines 5-11) that will crash the application on first-time login. When Token.findOne returns null, the Token.create call creates a new token but doesn't assign it to the token variable. This causes line 9 (token.refreshToken = newToken) to throw a TypeError: 'Cannot set properties of null (reading 'refreshToken')'. To fix this, assign the result of Token.create() to the token variable: token = await Token.create(...).
Additionally, there's a minor import inconsistency in user.service.js:2 where api.error is imported without the .js extension while other files include it—this should be standardized for consistency.
✨ 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 (!token) { | ||
| await Token.create({ userId, refreshToken: newToken }); | ||
| } | ||
|
|
||
| token.refreshToken = newToken; | ||
|
|
There was a problem hiding this comment.
Critical bug: When token is null (new user without existing token), the code enters the if block but never assigns the created token to token. Line 9 will crash with 'Cannot set properties of null' when trying to save a new refresh token for a user who logs in for the first time.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation successfully addresses all task requirements for the authentication system. Both critical issues from the previous review have been fixed: the token.service.js now properly assigns Token.create() to the token variable, and the import statement in user.service.js includes the .js extension for consistency.
The code implements all required functionality correctly: registration with password validation and activation email, login with inactive account handling, logout, password reset flow, and profile management for name/password/email changes. The middleware files, models, and routes all implement their responsibilities properly.
A minor suggestion for improvement: in errorMiddleware.js, consider adding return after handling an ApiError to prevent unnecessary execution, though this doesn't affect functionality since the response is already sent. Overall, your solution meets the requirements and is ready for use.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (error instanceof ApiError) { | ||
| response.status(error.status).send({ | ||
| message: error.message, | ||
| errors: error.errors, | ||
| }); |
There was a problem hiding this comment.
After handling an ApiError and sending a response, the middleware continues executing and sends a second response (lines 10-14) with a 500 status. This causes a runtime error: 'Cannot set headers after they are sent to the client'. Add return; after line 8 to prevent this.
| @@ -0,0 +1,182 @@ | |||
| import { ApiError } from '../exceptions/api.error.js'; | |||
There was a problem hiding this comment.
The import statement is missing the .js extension while other files in this project consistently include it. For consistency with files like user.service.js:2, consider adding the extension: import { Token } from '../models/token.js';
No description provided.