Skip to content

first try#238

Open
Eksonurit wants to merge 3 commits intomate-academy:masterfrom
Eksonurit:develop
Open

first try#238
Eksonurit wants to merge 3 commits intomate-academy:masterfrom
Eksonurit:develop

Conversation

@Eksonurit
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

This is a great start with a well-structured application. However, several core requirements are missing or broken, so I am requesting changes before this can be approved.

Here are the main issues to address:

  1. Missing Profile Management: The entire feature set for the profile page is missing. You need to implement the routes, controller logic, and services for authenticated users to change their name, password, and email as required by the task.
  2. Broken Account Activation: The activation flow is not working because the code incorrectly queries for a token field on the User model, but the model defines it as activationToken. This bug affects src/services/user.service.js and src/controllers/auth.controller.js and must be fixed for activation and login to work correctly.
  3. Incomplete Registration & User Model: The User model in src/models/user.js is missing the required name field. This needs to be added to the model and integrated into the registration logic in auth.controller.js and user.service.js.
  4. Missing 404 Handler: The server needs a catch-all middleware in src/index.js to handle requests to undefined routes and respond with a 404 status.

Please focus on implementing these missing features and fixing the activation bug. You're on the right track, and I look forward to your next submission!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

};

const registration = async (req, res) => {
const { email, password } = req.body;

Choose a reason for hiding this comment

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

The task requires users to register with a name, email, and password. This implementation is missing the name field from the request body and the subsequent user creation logic.

return;
}

user.token = null;

Choose a reason for hiding this comment

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

The property on the User model is activationToken, not token. To nullify the token after activation, this should be user.activationToken = null;.

return res.sendStatus(401);
}

if (user.token) {

Choose a reason for hiding this comment

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

The check for an unactivated user is incorrect. The property on the User model is activationToken. This condition should be if (user.activationToken) to correctly check if the user needs to activate their email.

};

const resetPassword = async (req, res) => {
const { newPassword, resetToken } = req.body;

Choose a reason for hiding this comment

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

According to the requirements, the password reset process must include a password and a confirmation field, and you must verify they are equal. This implementation is missing the confirmation field and the associated validation.

allowedHeaders: ['Content-Type', 'Authorization'],
}),
);
app.use(authRouter);

Choose a reason for hiding this comment

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

The task requires a 404 response for any routes that are not defined. You should add a middleware function after your router to catch all unhandled requests and respond with a 404 status.

Comment on lines +4 to +21
export const User = client.define('users', {
email: {
type: DataTypes.STRING,
allowNull: false,
unique: true,
},
password: {
type: DataTypes.STRING,
allowNull: false,
},
activationToken: {
type: DataTypes.STRING,
},
resetToken: {
type: DataTypes.STRING,
allowNull: true,
},
});

Choose a reason for hiding this comment

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

The task requirements specify that a user should be able to register with a name, email, and password, and later change their name on the profile page. This model is missing the name field required to store this information.

<a href="${href}">CLICK</a>
`;

return send({ email, subject: 'Activate', html });

Choose a reason for hiding this comment

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

The subject for the password reset email is 'Activate', which could be confusing for the user. It should be more descriptive, such as 'Reset Password'.

authRouter.post('/logout', authController.logout);
authRouter.post('/refresh', authController.refreshenToken);
authRouter.post('/forgot-password', authController.forgotPassword);
authRouter.post('/reset-password', authController.resetPassword);

Choose a reason for hiding this comment

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

The router is missing endpoints for the profile page functionality. According to the task description, authenticated users should be able to change their name, password, and email. You need to add routes to handle these actions.

}
}

async function addUser(email, password, activationToken = false) {

Choose a reason for hiding this comment

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

The task requires users to register with a name, email, and password. This function should also accept and save the user's name when a new user is created.

async function getUserByToken(token) {
const user = await User.findOne({
where: {
token,

Choose a reason for hiding this comment

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

The field in the User model is named activationToken, not token. This query should be where: { activationToken: token } to correctly find the user for account activation.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

You've made some great progress since the last review, especially with fixing the account activation flow! However, I'm requesting changes because several core features are incomplete or have critical bugs that prevent the application from functioning as required.

Here are the main issues to address:

  1. Registration is broken: The required name field is missing from the registration logic in src/controllers/auth.controller.js and src/services/user.service.js, which prevents new users from being created.
  2. Profile management is incomplete and insecure: The routes for updating the user's name and email are missing from src/routes/auth.route.js. Additionally, all profile-related routes must be protected by your authMiddleware to prevent unauthorized access.
  3. Password reset flow is incorrect: The forgot-password endpoint in auth.controller.js should only require an email, not a password. The reset-password endpoint is also missing the required confirmation field and the logic to validate it against the new password.
  4. Middleware will crash: There is a typo in src/middlewares/authMiddleware.js. The method res.sentStatus does not exist; it should be res.sendStatus.

Please focus on these points to get the core functionality working correctly. You're getting close!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

};

const registration = async (req, res) => {
const { email, password } = req.body;

Choose a reason for hiding this comment

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

The registration process is still missing the name field, which is required by the task. You'll need to get it from req.body and pass it to the userService to create the new user.

};

const forgotPassword = async (req, res) => {
const { email, password } = req.body;

Choose a reason for hiding this comment

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

The forgot-password flow shouldn't ask for a password, as the user has forgotten it. According to the requirements, this endpoint should only ask for the user's email address to initiate the reset process.

Comment on lines +175 to +177
const isPasswordCorrect = await bcrypt.compare(password, user.password);

if (!user || !isPasswordCorrect) {

Choose a reason for hiding this comment

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

Based on the logic on the previous line, this check is also incorrect for a forgot-password flow. The user should be found only by email, and then a reset token should be generated and sent.

};

const resetPassword = async (req, res) => {
const { newPassword, resetToken } = req.body;

Choose a reason for hiding this comment

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

The requirements state that the password reset confirmation page needs password and confirmation fields that must be equal. You're missing the confirmation field from the request body and the logic to validate that it matches newPassword.

const { newName } = req.body;
const { refreshToken } = req.cookies;

const token = await tokenService.getUserByToken(refreshToken);

Choose a reason for hiding this comment

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

The tokenService does not have a method named getUserByToken. Did you mean to use findByToken?

const userData = jwtService.verifyAccessToken(token);

if (!userData) {
return res.sentStatus(401);

Choose a reason for hiding this comment

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

This is the same typo as on the line above. The method should be sendStatus.

authRouter.post('/forgot-password', authController.forgotPassword);
authRouter.post('/reset-password', authController.resetPassword);

authRouter.patch('/profile/password', authController.updatePassword);

Choose a reason for hiding this comment

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

This route, and any other routes related to profile management, must be protected to ensure they are only accessible by authenticated users. You've created an authMiddleware but it needs to be applied to these routes. Additionally, you are missing the routes for updating the user's name and email, which are required features for the profile page.

<a href="${href}">CLICK</a>
`;

return send({ email, subject: 'Activate', html });

Choose a reason for hiding this comment

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

The subject for the password reset email is 'Activate', which could be confusing for the user. It should be updated to something more appropriate, like 'Password Reset'.

Comment on lines +13 to +17
async function addUser(email, password, activationToken = false) {
const newUser = await User.create({
email,
password,
activationToken,

Choose a reason for hiding this comment

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

The addUser function is missing the name parameter, which is a required field for registration according to the task description. Since the User model has name set to allowNull: false, this function will cause an error when creating a new user. You need to add name to the function signature and include it in the User.create call.

Comment on lines +39 to +43
const normalizedUser = ({ id, email }) => {
return {
id,
email,
};

Choose a reason for hiding this comment

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

The normalized user object is missing the name property. It's good practice to include all non-sensitive user data that might be needed on the client-side, such as the user's name for display purposes.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am approving your solution as an educational override to help you progress, but it's important to address the remaining critical issues. You've done excellent work on the registration and password reset flows, and your service layer is solid!

However, the biggest problem is that the logout and profile routes in src/routes/auth.route.js are not protected by your authMiddleware, which is a major security risk. Also, a typo (sentStatus instead of sendStatus) in src/middlewares/authMiddleware.js will cause the server to crash.

Finally, please review the profile update functions in src/controllers/auth.controller.js. They are missing key logic like saving changes to the database, validating the password confirmation, and sending responses, which causes requests to time out. Keep up the great effort!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

const [, token] = authorization.split(' ');

if (!authorization || !token) {
return res.sentStatus(401);

Choose a reason for hiding this comment

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

This method name is incorrect and will cause a server crash. The correct method is sendStatus.

const { newName } = req.body;
const { refreshToken } = req.cookies;

const token = await tokenService.getUserByToken(refreshToken);

Choose a reason for hiding this comment

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

This line will cause a crash because the function tokenService.getUserByToken does not exist in token.service.js. You might have intended to use findByToken.

Also, a better practice for authenticated routes is to use middleware to verify the user and attach their data to the req object, rather than manually handling tokens in each controller.

Comment on lines +251 to +260
const isEmailBooked = userService.getUserByEmail(newEmail);

if (isEmailBooked) {
return res.status(409).send('this email is already used');
}

const activationToken = uuidv4();

user.email = newEmail;
user.activationToken = activationToken;

Choose a reason for hiding this comment

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

This function has a few issues that prevent it from working correctly:

  1. On line 251, userService.getUserByEmail is an async function, so you need to await its result. Otherwise, isEmailBooked will always be a truthy value (a pending promise).
  2. The task requires notifying the user's old email address about the change, but that logic is missing.
  3. After updating the user object on lines 259-260, the changes are not saved to the database. You're missing an await user.save().
  4. The function doesn't send a response back to the client, so the request will hang and eventually time out.

};

const updatePassword = async (req, res) => {
const { oldPassword, newPassword } = req.body;

Choose a reason for hiding this comment

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

According to the requirements, changing the password on the profile page requires an oldPassword, a newPassword, and a confirmation for the new password. This implementation is missing the confirmation field and the logic to validate that it matches the newPassword.

authRouter.post('/registration', authController.registration);
authRouter.get('/activate/:token', authController.activate);
authRouter.post('/login', authController.login);
authRouter.post('/logout', authController.logout);

Choose a reason for hiding this comment

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

The task requires the logout feature to be accessible only by authenticated users. This route needs to be protected by the authMiddleware.

Comment on lines +13 to +15
authRouter.patch('/profile/password', authController.updatePassword);
authRouter.patch('/profile/email', authController.emailUpdate);
authRouter.patch('/profile/name', authController.updateName);

Choose a reason for hiding this comment

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

All profile-related routes must be protected to ensure only authenticated users can modify their data. Please apply the authMiddleware to this block of routes (/profile/password, /profile/email, and /profile/name).

<a href="${href}">CLICK</a>
`;

return send({ email, subject: 'Activate', html });

Choose a reason for hiding this comment

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

The subject for the password reset email is hardcoded as 'Activate'. This is likely a copy-paste error and could be confusing for the user. It should be something more descriptive, like 'Reset your password'.

Comment on lines +41 to +45
export const emailService = {
sendActivationEmail,
send,
sendResetPassword,
};

Choose a reason for hiding this comment

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

The requirements state that when a user changes their email, the old email address must be notified. This service is missing a function to handle that notification. You should add a new function here, for example, sendEmailChangeNotification, that can be called from your controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants