Skip to content

feat: proposal approach for auth project#266

Open
ht1204 wants to merge 2 commits intomate-academy:masterfrom
ht1204:develop
Open

feat: proposal approach for auth project#266
ht1204 wants to merge 2 commits intomate-academy:masterfrom
ht1204:develop

Conversation

@ht1204
Copy link
Copy Markdown

@ht1204 ht1204 commented Apr 11, 2026

Description

  • Proposal approach for auth project

Copy link
Copy Markdown

@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 requesting changes because one core requirement from the task is not yet met in your activation flow. The specification says that after successful activation the user must be redirected to the Profile page, but in src/routes/auth.js your activation handler currently sets the JWT cookie and then renders an activation success view (see the final res.render(...) in that route) instead of doing res.redirect('/profile'), so users never land on the Profile page as required. Please update that handler so that, after a successful activation and cookie set, it responds with a redirect to /profile rather than rendering the activation page.

In addition, your exported validatePassword helper (around lines 20–45 where it uses password.length) assumes password is always defined; if it were ever called with undefined, it would throw a runtime error, so consider adding a small defensive check like if (!password) return ['Password is required']; at the top. Functionally, the rest of your auth, profile, and password-reset flows look solid and align well with the requirements (guards, JWT handling, and route behaviors are correctly implemented). Once you fix the activation redirect and harden validatePassword, your solution will match the task specification much more closely.


✨ 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

Comment thread src/routes/auth.js Outdated
Comment on lines +203 to +206
res.render('auth/activate', {
title: 'Account Activated',
success: true,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After successful activation you set a JWT cookie and then render the activation view. The task requires that the user is redirected to the Profile page after activation. Replace the final render with a redirect to /profile (after setting the cookie) so activation redirects to the profile as specified.

Comment thread src/public/css/style.css
Comment on lines +185 to +206

.profile-info {
background: #f8f9fa;
border-radius: 4px;
padding: 15px;
margin-bottom: 20px;
}

.profile-info p {
margin-bottom: 5px;
}

.profile-actions {
margin-top: 20px;
padding-top: 20px;
border-top: 1px solid #eee;
}

.tabs {
display: flex;
gap: 0;
margin-bottom: 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The checklist requires that after activation the application redirects to the Profile page. Here you set the JWT cookie but render an activation page (lines 203–206) instead of redirecting. Consider calling res.redirect('/profile') after setting the cookie (or redirecting the user) so they land on the Profile page as required.

Comment thread src/public/css/style.css
Comment on lines +20 to +45
.navbar {
background-color: #2c3e50;
padding: 15px 0;
margin-bottom: 30px;
}

.navbar .container {
display: flex;
justify-content: space-between;
align-items: center;
}

.logo {
color: #fff;
text-decoration: none;
font-size: 1.5em;
font-weight: bold;
}

.nav-links {
display: flex;
align-items: center;
gap: 15px;
}

.nav-link {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

validatePassword (lines 20–45) uses password.length without checking if password is defined. Callers currently guard before calling, but since this function is exported and reused, consider adding an early check (e.g., if (!password) return ['Password is required'];) to avoid a runtime error when called with undefined.

Copy link
Copy Markdown

@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

Your solution is approved because it implements all required authentication, activation, password reset, profile, and 404 flows and behaves correctly according to the task requirements. Registration, login, activation, logout, and protected profile routes are wired with the correct guest/authenticate middleware, JWT cookies are issued and validated properly, and users are redirected to /profile or /auth/login as expected. The password reset flow correctly handles requesting a reset, confirming a new password with confirmation, and showing a success page, and the profile routes allow changing name, password, and email with appropriate validations. There are no critical blockers or failing behaviors that would prevent task completion. For future improvement, consider surfacing email-sending failures to logs or the UI more clearly rather than only logging a generic message, and double-check that your reset-success page includes an obvious link back to the login page. You might also streamline validation so that a missing-password error is reported once rather than potentially from both the route and validatePassword, but this is purely cosmetic—overall, this is solid work.


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

Footnotes

  1. Rate AI review example

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