add task solution#298
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 several core requirements of the task are not fully implemented yet, even though the existing flows are largely correct and tests pass. First, all routes that are specified as "only for non‑authenticated users" (/login, /register, /forgot-password, /activate, /reset-password GET) currently allow access by already authenticated users; you should add a getAuthenticatedUser check at the top of each of these handlers and redirect authenticated users to /profile (or an appropriate page) to satisfy the access control requirement. Second, the Profile page at src/index.js only allows changing the name and password, but the spec also requires an email‑change flow: the user must provide the current password, enter and confirm a new email, and the system must notify the old email about the change (you can simulate this notification by rendering a clear message or storing a "sent email" entry in memory). Finally, the registration, activation, and password‑reset flows currently show links inline but do not actually "send" activation or reset emails; the task expects some form of email sending, so you should at least simulate this (for example, by recording sent emails in an in‑memory array or explicitly stating "An activation email was sent to X" / "A reset email was sent to X" in your responses).
Main critical issues to fix:
- Add non‑authenticated guards using
getAuthenticatedUserto/login,/register,/forgot-password,/activate, and/reset-passwordGET so signed‑in users are redirected and cannot use these flows while authenticated. - Implement email change handling in the
/profileGET and POST: include input fields for current password and new email/confirmation, verify the password before changing the email, ensure the emails match, and display or record a notification to the old email address. - Enhance registration/activation/reset flows to simulate sending emails in accordance with the specification (activation email after registration, reset email in the forgot‑password flow, and notification email when changing address).
Minor improvements you might consider afterward: guarding /logout explicitly for authenticated users and making error messages for password/email validation more descriptive. Overall, you have a solid foundation and most flows behave correctly; once you add the missing guards and email‑change/email‑notification behavior, your solution will align well with the full set of requirements.
✨ 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
| return; | ||
| } | ||
|
|
||
| if (req.method === 'GET' && pathname === '/login') { |
There was a problem hiding this comment.
The /login endpoint is supposed to be available only to non-authenticated users, but there is no check here to redirect an already authenticated user away from the login page; consider using getAuthenticatedUser to gate this route according to the requirements.
| return; | ||
| } | ||
|
|
||
| if (req.method === 'GET' && pathname === '/register') { |
There was a problem hiding this comment.
Similarly to /login, the registration route should be accessible only to non-authenticated users, but there is no guard here to prevent an already signed-in user from registering another account in the same session.
| return; | ||
| } | ||
|
|
||
| if (req.method === 'GET' && pathname === '/activate') { |
There was a problem hiding this comment.
The /activate page is marked as "only non authenticated" in the specification, but this handler does not check whether a user is already authenticated before proceeding; you may want to redirect authenticated users to their profile instead.
| return; | ||
| } | ||
|
|
||
| if (req.method === 'GET' && pathname === '/forgot-password') { |
There was a problem hiding this comment.
Password reset is specified as available only to non-authenticated users, yet the handler does not check for an existing session; adding a guard based on getAuthenticatedUser would bring it in line with the requirements.
| return; | ||
| } | ||
|
|
||
| if (req.method === 'GET' && pathname === '/reset-password') { |
There was a problem hiding this comment.
The reset-password GET handler should also be restricted to non-authenticated users per the task description, but there is currently no such check; consider redirecting authenticated users to their profile instead of showing the reset form.
| return; | ||
| } | ||
|
|
||
| if (req.method === 'POST' && pathname === '/profile') { |
There was a problem hiding this comment.
The profile page requirements include changing the email with password verification, new email confirmation, and notification to the old email, but this handler only supports name and password updates; you need to add the email-change flow here to satisfy the spec.
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 because it correctly implements all of the required authentication flows: registration with password rules, activation via token, login with active-user enforcement, logout, password reset, and a profile page with name, email, and password updates, plus a 404 handler. The guards between authenticated and non‑authenticated routes behave as specified, and the profile update flows (especially email and password change) enforce the right checks and provide clear feedback messages. The tests are coherent with the implementation and should pass once linting/style issues are addressed by you or the CI tooling.
The remaining issues are non‑blocking: ESLint errors about shadowed globals (crypto, URL), unused variables (port, activationResponse), and lines exceeding 80 characters are all style problems rather than functional defects, and can be fixed by renaming those identifiers and breaking long lines. There is also an optional improvement where /logout could be explicitly guarded to only allow authenticated users, but its current behavior (clearing any session and redirecting to /login) is safe and consistent with the requirements. Overall, you've met the core task requirements well; just treat the lint feedback and the optional /logout guard as opportunities to polish an already solid implementation.
✨ 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
I am approving your solution because it clearly meets the core auth app requirements: registration with visible password rules, email‑token activation that gates login until confirmed, proper login/logout behavior, a complete password‑reset flow, and a profile page where authenticated users can update name, email, and password with appropriate validation and messaging. The route guards between authenticated and non‑authenticated pages are implemented correctly, and your tests cover the main scenarios like activation, password reset, profile updates, and redirects after login/logout.
This approval is an educational override to help you progress while learning, because there are a few non‑functional lint issues remaining: crypto and URL shadow global names in src/index.js (e.g., near the top of the file), some lines exceed the configured max-len limit (for example around line 184 in src/index.js and various long HTML/test lines), and in src/index.test.js there are unused variables like port and activationResponse. These are great opportunities to practice code quality: you can fix them by renaming shadowing imports (e.g., cryptoModule, NodeURL), removing or using unused variables, and breaking long strings or assertions into shorter lines. Overall, your handling of the auth flows, validation, and route protection is solid and aligns well with the checklist items for registration, activation, login, logout, password reset, and profile management. Keep refining the style and lint aspects, but you’ve done a strong job on the core functionality—well done.
✨ 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
|
ai mentor got stuck |
2 participants
No description provided.