finished #223

Kanezoor · 2026-01-08T16:39:47Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great job on the overall structure of the application! The code is well-organized, and you've correctly implemented the middleware and database setup. However, I'm requesting a few changes because there are some critical bugs that prevent key features from working correctly.

Here are the main issues to address:

Broken Login and Password Reset: In src/controllers/authController.js, several calls to res.render() are using URL paths like '/login' instead of view names like 'login'. This will cause the server to crash on a failed login attempt.
Missing await in Password Reset: The password reset page won't work for valid users because the models.User.findOne call in getResetPasswordPage is missing an await. This causes the token validation to fail.
Missing Middleware: The activation route in src/routes/authRoutes.js is missing the requireGuest middleware, which is a key requirement to ensure only non-authenticated users can access it.
Typos: There are a couple of typos causing errors: updadeName in authRoutes.js should be updateName, and user.mail in the updateEmail function should be user.email.

Please fix these issues to get the application fully functional. You're very close!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-01-08T16:42:01Z

src/controllers/authController.js

+  const { token } = req.params;
+
+  try {
+    const user = models.User.findOne({ where: { resetToken: token } });


models.User.findOne is an asynchronous operation. You need to use await here to get the user object. Without await, the user variable will be a promise, and the if (!user) check on the next line will not work as intended, allowing users with invalid tokens to see the password reset page.

mateacademy-ai-mentor · 2026-01-08T16:42:01Z

src/controllers/authController.js

+  }
+};
+
+const updadeName = async (req, res) => {


There's a typo in the function name. It should be updateName.

mateacademy-ai-mentor · 2026-01-08T16:42:01Z

src/controllers/authController.js

+  }
+};
+
+const updateEmail = async (req, res) => {


The requirements state that to change an email, the user should "confirm the new email". This implementation is missing the logic for confirming the new email address, for example, by comparing newEmail with a newEmailConfirmation field from the request body.

mateacademy-ai-mentor · 2026-01-08T16:42:02Z

src/controllers/authController.js

+      });
+    }
+
+    const oldEmail = user.mail;


The User model defines the email field as email, not mail. user.mail will be undefined. You should use user.email to get the old email address for the notification.

mateacademy-ai-mentor · 2026-01-08T16:42:02Z

src/controllers/authController.js

+  const { email, password } = req.body;
+
+  if (!email || !password) {
+    return res.render('/login', { error: 'Please provide email and password' });


The res.render() function expects a view name (like 'login'), not a URL path ('/login'). This will cause an error when trying to render the view.

mateacademy-ai-mentor · 2026-01-08T16:42:02Z

src/controllers/authController.js

+  try {
+    const user = await models.User.findOne({ where: { email } });
+
+    if (user) {


If a user with the provided email is not found, no response is sent from this function. This will cause the browser request to hang and eventually time out. For security reasons and a better user experience, you should always render the 'resetEmailSent' page, regardless of whether the user was found.

mateacademy-ai-mentor · 2026-01-08T16:42:02Z

src/db.js

+  dialect: 'postgres',
+  host: process.env.POSTGRES_HOST || 'localhost',
+  port: process.env.POSTGRES_PORT || 5432,
+  username: process.env.POSTRGRES_USER || 'postgres',


It seems there's a typo in this environment variable name. Did you mean POSTGRES_USER?

mateacademy-ai-mentor · 2026-01-08T16:42:02Z

src/routes/authRoutes.js

+router.get('/register', requireGuest, authController.getRegisterPage);
+router.post('/register', requireGuest, authController.register);
+
+router.get('/activate/:token', authController.activateAcount);


The task description specifies that the activation page should be for "only non authenticated" users. To enforce this, you should add the requireGuest middleware to this route, just like you have for the register and login routes.

mateacademy-ai-mentor · 2026-01-08T16:42:02Z

src/routes/authRoutes.js

+  if (!req.session.userId) {
+    return res.redirect('/register');
+  }


The requireAuth middleware on line 13 already ensures that req.session.userId exists, redirecting to /login if it doesn't. This if block is therefore redundant because its condition will never be met. You can remove this check.

mateacademy-ai-mentor · 2026-01-08T16:42:02Z

src/routes/authRoutes.js

+  authController.resetPassword,
+);
+
+router.post('/profile/name', requireAuth, authController.updadeName);


This route is using a controller function named updadeName, which appears to have a typo. For better code clarity and maintainability, it should probably be updateName. Ensure the function name is corrected in both the controller and here.

mateacademy-ai-mentor

Review Summary

Excellent progress on this submission! You've successfully fixed most of the critical bugs from the previous review. However, I am requesting changes because a few critical issues remain that break core functionality.

Here are the blockers that need to be addressed:

Name update is broken: In src/routes/authRoutes.js, the controller function is misspelled as updadeName instead of updateName, which causes a runtime error.
Password reset hangs: The requestPasswordReset function in src/controllers/authController.js does not send a response if the email is not found, causing the client's request to time out. You should always send a response to avoid this.
Database connection will fail: In src/db.js, the environment variable for the database user has a typo (POSTRGRES_USER). It should be POSTGRES_USER.
Email change is incomplete: The updateEmail function is missing the required logic to confirm the new email address, which is part of the task requirements.

Please address these points to ensure all features are working as expected. You're very close to completing this!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-01-08T17:15:48Z

src/controllers/authController.js

+
+const updatePassword = async (req, res) => {
+  const { oldPassword, newPassword, confirmation } = req.body;
+  const userId = await req.session.userId;


The await keyword is not necessary here because req.session.userId is a value, not a Promise. While this doesn't break the functionality, it's good practice to only use await with promises to avoid confusion.

mateacademy-ai-mentor · 2026-01-08T17:15:48Z

src/controllers/authController.js

+};
+
+const updateEmail = async (req, res) => {
+  const { newEmail, password } = req.body;


The task description requires the user to "confirm the new email" when updating their email address. This typically means the form should have a second field for confirmation, and this function should check that the two new email fields match. That validation seems to be missing.

mateacademy-ai-mentor · 2026-01-08T17:15:48Z

src/controllers/authController.js

+    if (user) {
+      const resetToken = uuidv4();
+
+      user.resetToken = resetToken;
+      await user.save();
+
+      // eslint-disable-next-line no-console
+      console.log(`SENDING RESET EMAIL TO: ${email}`);
+
+      // eslint-disable-next-line no-console
+      console.log(
+        `RESET LINK: http://localhost:3000/reset-password/${resetToken}`,
+      );
+
+      res.render('resetEmailSent');
+    }


If a user with the provided email is not found, the if (user) condition is false, and the function ends without sending a response. This will cause the client's request to hang and eventually time out. It's also a security best practice to always send the same response (e.g., render the 'resetEmailSent' page) whether the email exists or not, to prevent attackers from figuring out which emails are registered in your system.

mateacademy-ai-mentor · 2026-01-08T17:15:48Z

src/db.js

+  dialect: 'postgres',
+  host: process.env.POSTGRES_HOST || 'localhost',
+  port: process.env.POSTGRES_PORT || 5432,
+  username: process.env.POSTRGRES_USER || 'postgres',


There appears to be a typo in this environment variable name. It should likely be POSTGRES_USER to be consistent with the other POSTGRES_* variables.

mateacademy-ai-mentor · 2026-01-08T17:15:48Z

src/routes/authRoutes.js

+  if (!req.session.userId) {
+    return res.redirect('/register');
+  }


This check for req.session.userId is redundant. The requireAuth middleware on the previous line already performs this exact check and will redirect the user to /login if they are not authenticated. You can safely remove this if block.

mateacademy-ai-mentor · 2026-01-08T17:15:48Z

src/routes/authRoutes.js

+  authController.resetPassword,
+);
+
+router.post('/profile/name', requireAuth, authController.updadeName);


This appears to be a typo that was also mentioned in the previous review. The function name in authController.js is updateName, but here it is spelled updadeName. This will cause a TypeError when a user tries to update their name.

mateacademy-ai-mentor

Review Summary

Excellent work on this task! I am approving your solution as an educational override so you can move forward, as you've successfully fixed all the critical issues and met all the project requirements. The application is now fully functional, which is a fantastic achievement.

I have one small suggestion for improvement in src/routes/authRoutes.js. The if block checking for req.session.userId is redundant because the requireAuth middleware you've applied to the route already handles this. Removing it will make your code a bit cleaner.

Overall, this is a very strong submission. Keep up the great work!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-01-08T17:41:49Z

src/routes/authRoutes.js

+  if (!req.session.userId) {
+    return res.redirect('/register');
+  }


This check for req.session.userId is redundant. The requireAuth middleware you've applied to this route on line 13 already performs this exact check and redirects to '/login' if the user is not authenticated. You can safely remove this if block.

finished

8feb437

mateacademy-ai-mentor suggested changes Jan 8, 2026

View reviewed changes

addressed couple of issues from code review

7bd2cd6

Kanezoor requested a review from mateacademy-ai-mentor January 8, 2026 17:13

mateacademy-ai-mentor suggested changes Jan 8, 2026

View reviewed changes

fixed couple of issues

7bebdf9

Kanezoor requested a review from mateacademy-ai-mentor January 8, 2026 17:40

mateacademy-ai-mentor approved these changes Jan 8, 2026

View reviewed changes

finished #223

Are you sure you want to change the base?

finished #223

Uh oh!

Conversation

Kanezoor commented Jan 8, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants