Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've built a solid structure for the validation function and tests, which is a great start. However, this requires changes because the core validation logic doesn't fully meet the task requirements.
Specifically, the regular expressions in src/validateRegisterForm.js need to be updated:
- Password Validation: The current regex does not accept Cyrillic letters (
Aa-Яя) as required. - Email Validation: The regex incorrectly rejects several required special characters (e.g.,
!,#,+,_).
After updating the validation logic, please also adjust the tests in src/validateRegisterForm.test.js to verify that these valid inputs are now accepted. You're very close to finishing this!
✨ 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
| }); | ||
|
|
||
| it(`should return error for valid email and password without number`, () => { | ||
| it(`should return success message for the invalid input`, () => { |
There was a problem hiding this comment.
The test description should return success message for the invalid input is a bit confusing. It would be clearer if it described the expected outcome, for example, should return a failure message when both email and password are invalid.
| expect(invalidPassword.message).toBe('Password is invalid.'); | ||
| }); | ||
|
|
||
| it(`should fail for password with invalid letters`, () => { |
There was a problem hiding this comment.
This test correctly checks for an invalid character in a password. However, the requirements also state that passwords must accept Cyrillic letters (Aa-Яя). The test suite is missing a positive test case to verify that a valid password containing these letters will pass validation.
|
|
||
| it(`should fail for email with invalid characters`, () => { | ||
| const invalidPassword | ||
| = validateRegisterForm('test$@mail.com', 'P@ssword1!'); |
There was a problem hiding this comment.
According to the task description, the $ character is a valid character for an email address. This test incorrectly expects validation to fail for an email containing it. A separate test should be added to confirm that emails with valid special characters (e.g., !, #, $) are accepted.
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); | ||
|
|
||
| it(`should fail if top-level domain starts with a dot`, () => { |
There was a problem hiding this comment.
This test has the same description as the one starting on line 120. Additionally, the case being tested (test@mail..com) involves consecutive dots, which is already covered by the test on line 104. This makes the test redundant.
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); | ||
|
|
||
| it(`should fail for email with double dots `, () => { |
There was a problem hiding this comment.
The description for this test, should fail for email with double dots, is misleading. The test actually checks for an invalid character (a colon) in the email, not double dots. The description should be updated to accurately reflect what is being tested.
| @@ -12,7 +12,7 @@ function validateRegisterForm(email, password) { | |||
| const validPassword = /^(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[^a-zA-Z0-9])(?!.*\s).{8,16}$/; | |||
There was a problem hiding this comment.
The current regular expression for password validation does not meet the requirement to accept Cyrillic letters (Aa-Яя). It only checks for Latin letters (a-z, A-Z), so a password containing Cyrillic characters would incorrectly be marked as invalid.
|
|
||
| // eslint-disable-next-line max-len | ||
| const validEmailMask = new RegExp(/^([\w-]+(?:\.[\w-]+)*)@((?:[\w-]+\.)*\w[\w-]{0,66})\./i); | ||
| const validEmailMask = /^[A-Za-z0-9]+(\.[A-Za-z0-9]+)*@[A-Za-z0-9-]+(\.[A-Za-z0-9-]+)*\.[A-Za-z]{2,}$/; |
There was a problem hiding this comment.
This regular expression for email validation is too restrictive. The requirements state that emails can contain the special characters ! # $ % & ' * + - / = ? ^ _ { | } ~, but your current regex does not permit them in the local part of the email address (before the @`).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job updating the email validation logic—it now correctly handles all the required special characters.
However, this submission requires changes because the password validation still doesn't meet a key requirement. The regular expression in src/validateRegisterForm.js needs to be updated to accept Cyrillic letters (Аа-Яя). Currently, it will incorrectly fail for a valid password like 'Пароль!1'.
To help you verify the fix, please add a test case in src/validateRegisterForm.test.js for a valid password containing Cyrillic characters. Additionally, a couple of your test descriptions are misleading and should be updated for clarity as noted in the comments.
You're very close to completing this!
✨ 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
| }); | ||
|
|
||
| it(`should return error for valid email and password without number`, () => { | ||
| it(`should return success message for the invalid input`, () => { |
There was a problem hiding this comment.
This test description is confusing. Since the test checks a failure case (both inputs invalid), a title like should return an error when both email and password are invalid would be more accurate.
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); | ||
|
|
||
| it(`should fail for email with invalid characters`, () => { |
There was a problem hiding this comment.
This test description is misleading as it contradicts the test's expectation. The email 'test$@mail.com' is valid, and the test correctly expects a 200 success code. Please update the description to something like should pass for an email with valid special characters.
| @@ -12,7 +12,7 @@ function validateRegisterForm(email, password) { | |||
| const validPassword = /^(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[^a-zA-Z0-9])(?!.*\s).{8,16}$/; | |||
There was a problem hiding this comment.
This regular expression still doesn't meet the requirement to accept Cyrillic letters (Аа-Яя). According to the task, the password should be valid with both Latin and Cyrillic characters.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you keep moving forward.
Excellent work updating the password validation logic to support Cyrillic characters—that was the main goal and you've implemented it perfectly!
Before you move on, please take a moment to address the feedback in your test file, src/validateRegisterForm.test.js. It's crucial to add a new test case that verifies a valid password with Cyrillic characters (e.g., 'Пароль!1') passes validation. Also, several test descriptions are misleading and don't match the actual test case, such as the one testing for a colon (:) but described as checking for 'double dots'. Correcting these will make your test suite much stronger and clearer.
Great progress on this task, keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| }); | ||
|
|
||
| it(`should return error for valid email and password without number`, () => { | ||
| it(`should return success message for the invalid input`, () => { |
There was a problem hiding this comment.
This test description is confusing. It says "success message for the invalid input," which is a contradiction. A clearer description would be should return an error message for invalid email and password.
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); | ||
|
|
||
| it(`should fail for email with invalid characters`, () => { |
There was a problem hiding this comment.
This test's description states it should fail, but the test correctly passes with a status code of 200. The $ character is a valid special character for emails according to the requirements. Please update the description to reflect that this is a success case, for example: should succeed for email with a valid special character.
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); | ||
|
|
||
| it(`should fail if top-level domain starts with a dot`, () => { |
There was a problem hiding this comment.
This description is a duplicate and doesn't accurately describe the test case. The input 'test@mail..com' is being tested for consecutive dots, not a top-level domain starting with a dot. Please update the description to be more specific to the input.
| expect(invalidPassword.message).toBe('Email is invalid.'); | ||
| }); | ||
|
|
||
| it(`should fail for email with double dots `, () => { |
There was a problem hiding this comment.
The description here mentions 'double dots', but the test input 'test:autor@mail.com' actually contains an invalid colon character (:). Please correct the description to match what is being tested.
No description provided.