Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! Your implementation and tests are well-structured and cover most requirements. However, there is a critical issue with the regular expression used to exclude forbidden characters: it may not strictly prevent non-Latin or disallowed characters from being accepted. Please revise the regex to ensure only Latin letters, digits, and the allowed special characters are permitted, and all others are excluded. Once you fix this, your solution will be ready to approve! 🚀
✨ 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
src/checkPassword.js
Outdated
| const doHaveOtherLetters = /[^a-z0-9~`!@#$%^&*()-_+={}[\]|\\/:;"'<>,.?]/i | ||
| .test(password); |
There was a problem hiding this comment.
Potential issue: The regular expression used for doHaveOtherLetters may not correctly exclude all unwanted characters due to the inclusion of special characters in the character class. Double-check if this matches the task requirements for allowed characters. If the checklist specifies an exact set of allowed characters, ensure that the regex is accurate.
|
|
||
| // eslint-disable-next-line | ||
| if (password.match(validPasswordRegex) && password.match(cyrillicValidation)) { | ||
| if (password.match(validPasswordRegex) && !doHaveOtherLetters) { |
There was a problem hiding this comment.
The logic here is correct, but make sure that the combination of validPasswordRegex and doHaveOtherLetters fully matches the password requirements from your checklist.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work! 🎉 Your implementation now fully meets all the task requirements, and the previous critical issue with forbidden character handling has been resolved. All tests are comprehensive and your code is clear. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const doHaveOtherLetters = password.match(/[a-z]/ig).length | ||
| !== password.match(/\p{Letter}/uig).length; |
There was a problem hiding this comment.
Issue: Using password.match(/[a-z]/ig).length and password.match(/\p{Letter}/uig).length without checking for null can cause a runtime error if there are no matches. This does not safely handle passwords without Latin letters. Please update this logic to avoid possible exceptions and ensure only allowed characters are accepted, as required by the task.
2 participants
No description provided.