Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a few critical gaps that prevent the project from fully meeting the task requirements and could break key auth flows at runtime. The main blockers are that several required endpoints are missing or not wired correctly in src/routes/auth.route.js (in particular, routes for account activation, logout, and token refresh that should be guarded with guestMiddleware/authMiddleware), and the email-change flow does not require confirmation of the new email even though the spec explicitly says the new email must be confirmed before saving. Additionally, your reset-password and change-password handlers expect a field named confirmPassword, while the spec and tests expect confirmation, which will cause the backend to reject valid client requests that follow the documented contract. Another critical issue is that controllers use tokenService (for saving/getting/removing refresh tokens), but src/services/token.service.js does not appear in the provided code; if this file is missing or incomplete, refresh and logout flows will either throw runtime errors or fail silently.
To move this to completion, please (1) add and correctly protect the missing /activate/:token, /logout, and /refresh routes in src/routes/auth.route.js, (2) update your password reset/change logic to accept the confirmation field name as required, (3) require and validate a confirmation of the new email in the updateEmail controller before updating the user, and (4) ensure a working token.service.js implementation exists and is imported correctly. As smaller but worthwhile improvements, avoid using client.sync({ force: true }) without await in setup.js/src/index.js (and remove force: true unless you truly intend to drop all tables), and fix user-facing typos in src/exeptions/api.error.js such as Aunthorized user and Not found page to make responses clearer. Overall, you’ve implemented most of the core logic (registration, activation handling, JWT issuance, password reset, and profile updates) very well; once you address these specific blockers the solution will be in good shape.
✨ 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
| constructor({ message, status, errors = {} }) { | ||
| super(message); | ||
|
|
||
| this.status = status; |
There was a problem hiding this comment.
Calling client.sync without awaiting the returned promise may let the script exit before the sync completes. Also force: true is destructive (drops all tables). Consider wrapping this in an async function and await client.sync(...), and remove force: true unless you intentionally want to drop existing data.
|
|
||
| static unauthorized(errors) { | ||
| return new ApiError({ | ||
| message: 'Aunthorized user', |
There was a problem hiding this comment.
Typo in the unauthorized error message: Aunthorized user. Consider fixing the spelling to Unauthorized user (or Unauthorized) to avoid confusion.
|
|
||
| static notFound(errors) { | ||
| return new ApiError({ | ||
| message: 'Not found page', |
There was a problem hiding this comment.
Message for not found errors is Not found page. Consider making this message clearer (for example Not found or Resource not found) so API responses are consistent and easy to understand.
| import { User } from './src/models/user.js'; | ||
| import { client } from './src/utils/db.js'; | ||
|
|
||
| client.sync({ force: true }); |
There was a problem hiding this comment.
In setup.js you call client.sync({ force: true }) but do not await it or handle errors. client.sync returns a Promise — wrap this call in an async function and await it or handle the returned promise to ensure DB sync completes and to catch errors.
| import express from 'express'; | ||
| import cookieParser from 'cookie-parser'; | ||
| import { authRouter } from './routes/auth.route.js'; | ||
| import { errorMiddleweare } from './middlewares/errorMiddleware.js'; |
There was a problem hiding this comment.
Calling client.sync({ force: true }) will drop all data (destructive). Consider making this explicit (or behind a flag), and await the call inside an async function so you handle the promise and any errors (e.g. await client.sync({ force: true })).
|
|
||
| app.use('/auth', authRouter); | ||
|
|
||
| app.use((req, res) => { |
There was a problem hiding this comment.
Typo in error message: message: 'Aunthorized user' — consider correcting to Unauthorized user for clarity and consistency.
| import { client } from '../utils/db.js'; | ||
|
|
||
| export const User = client.define('user', { | ||
| name: { |
There was a problem hiding this comment.
setup.js calls client.sync({ force: true }) without awaiting the returned Promise and uses force: true which drops all tables (destructive). Wrap this in an async function and await client.sync(...), and avoid force: true unless you intentionally want to wipe the database.
|
|
||
| export const User = client.define('user', { | ||
| name: { | ||
| type: DataTypes.STRING, |
There was a problem hiding this comment.
tokenService is imported and used in this controller (save/getByToken/remove). I don't see src/services/token.service.js provided — ensure that file exists and implements the expected functions; otherwise refresh/ logout token flows will fail at runtime.
| password: { | ||
| type: DataTypes.STRING, | ||
| allowNull: false, | ||
| }, |
There was a problem hiding this comment.
Typo in ApiError message: message: 'Aunthorized user'. Consider correcting to Unauthorized user (or Unauthorized) for a clear user-facing message.
| }, | ||
|
|
||
| isActivated: { | ||
| type: DataTypes.BOOLEAN, |
There was a problem hiding this comment.
The not-found ApiError message is Not found page. Consider a clearer phrasing such as Not found or Resource not found for consistency and clarity in API responses.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a few critical mismatches with the required API contract that will break the tests and client behavior. In src/controllers/auth.controller.js, the resetPassword handler currently reads and validates req.body.confirmPassword and returns validation errors under the confirmPassword key, but the spec/tests require the field to be named confirmation everywhere (both in the request body and in any error payloads) [CHECKLIST ITEM #24]. Similarly, in the change-password/update-password handler you correctly accept a confirmation field, but one of the error responses still returns an error object under confirmPassword instead of confirmation, which will cause assertions that check field names to fail [CHECKLIST ITEM #25]. In src/routes/auth.route.js, the activation flow is not fully wired: the controller expects req.params.activationToken, so the route must be defined as something like router.get('/activate/:activationToken', ...), and currently /activate, /logout, and /refresh are either missing or not correctly connected/protected, so core flows (activation, logout, token refresh) will not function as required [CHECKLIST ITEM #29]. The forgot-password and reset-password routes are also duplicated, with one set missing guestMiddleware, meaning authenticated users can hit flows that should be guest-only; you should remove the unprotected duplicates and keep only the versions wrapped in guestMiddleware [CHECKLIST ITEM #11]. Once you standardize the confirmation field usage, fix the error payload keys, and wire/protect the missing auth routes as described, your implementation will align much better with the task requirements and should pass the tests. Overall your auth logic, validation, token handling, and email flows are solid, so these are focused, fixable issues rather than a redesign—adjusting them will complete a strong solution.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved and the core authentication flows (registration, activation, login, logout, refresh, password reset, and profile updates) are implemented correctly and consistently with the requirements, including the use of the confirmation field in both reset-password and change-password flows [CHECKLIST ITEM #24, #25]. You did particularly well wiring the routes and middleware so that guest-only routes (register, login, activate, forgot/reset password) are protected with guestMiddleware, while profile and logout are correctly guarded with authMiddleware, and your use of structured ApiError responses makes error handling clear and predictable. Token handling is solid: access and refresh JWTs use separate keys and expirations, refresh tokens are persisted in the database, and the /refresh and /logout endpoints behave as expected for session management [CHECKLIST ITEM #29].
Because there are a few minor improvement opportunities, consider this an educational approval to help you keep progressing while you polish details: for example, your auth/guest middlewares currently only look at the Authorization header and could optionally be extended later to treat a valid refresh-token cookie as an authenticated session, and some small naming inconsistencies (like the error middleware export typo) may slightly hurt readability. It could also be helpful to centralize token and reset-link expirations into configuration constants so they’re easier to adjust and reason about, and to document clearly that the client should handle redirects after activation and logout since the server returns tokens or status codes rather than performing redirects itself. Overall, your implementation demonstrates a strong grasp of the auth flows and test expectations, and these remaining points are refinements rather than blockers—nice work getting this to a solid, working state.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.