check_form by robsza1122 · Pull Request #127 · mate-academy/js_check_the_form

robsza1122 · 2025-09-26T05:52:43Z

No description provided.

mateacademy-ai-mentor

Review Summary

I’m requesting changes because there are critical blockers in the current state that will prevent successful implementation and passing tests.

Critical issues to fix:
1. Argument order mismatch (src/validateRegisterForm.test.js): tests call validateRegisterForm(email, password), but parts of the description say (password, email). This will cause failing tests if followed literally. Please standardize on one order and align all materials. If you keep the tests as-is, implement validateRegisterForm(email, password) and update the description accordingly.
2. Email special characters contradiction (src/validateRegisterForm.test.js): the test expecting 'test!@mail.com' to be invalid conflicts with the description that lists ! as allowed in the local part. Decide whether ! is allowed; then either change the test expectation or update the description to remove ! from the allowed set so they match.
Minor improvements (non-blockers): consider adding a test for the “both invalid” case returning { code: 500, message: 'Password and email are invalid.' }, and optionally a positive test for Cyrillic letters in passwords if that’s a requirement.

Once these blockers are resolved, the rest of the test suite looks solid and will effectively validate implementations. Good work on the thorough coverage so far—aligning the spec with the tests will make it airtight.

✨ 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 · 2025-09-26T05:57:22Z

src/validateRegisterForm.test.js


-  // write more tests here
+  it(`password should not be shorter than 8 signs`, () => {
+    const invalidPassword = validateRegisterForm('test@mail.com', 'P@ss1');


The test calls the function as validateRegisterForm('test@mail.com', 'P@ss1') (email first, password second). The task description sometimes describes the function as taking (password, email). This mismatch will confuse implementers and cause failing tests if the implementation follows the description instead of the tests. Recommend deciding which order is intended and aligning the description and implementation with the tests. If you keep tests as-is, implement the function so the first parameter is the email and the second parameter is the password.

mateacademy-ai-mentor · 2025-09-26T05:57:22Z

src/validateRegisterForm.test.js

+  });
+
+  it(`email should not contain special characters`, () => {
+    const invalidPassword = validateRegisterForm('test!@mail.com', 'P@ssword1');


This test uses 'test!@mail.com' and expects the email to be invalid, but the task description lists ! among allowed special characters for email local part. This is a contradiction: either the description's allowed-character list should be tightened (remove !) or the test should expect that form to be valid. Please clarify which behavior you want and align the tests/description accordingly.

mateacademy-ai-mentor

Review Summary

I’m requesting changes because there are critical, blocking inconsistencies in the test suite that will cause correct implementations to fail or miss required behavior.

Critical blockers to fix:
1. Argument order mismatch (src/validateRegisterForm.test.js): tests call validateRegisterForm(email, password) while the description states (password, email). This will break otherwise-correct solutions. Choose one canonical signature and make both tests and description consistent. Suggestion: standardize on the order you prefer, then update either all test invocations or the description/examples accordingly.
2. Missing combined-invalid test (required): add a test asserting an invalid email and invalid password returns { code: 500, message: 'Password and email are invalid.' }. Without it, implementations can pass while not meeting the spec.
Important spec alignment to address:
- Allowed email characters: The test that rejects test]@mail.com is fine if ] is disallowed, but reconcile with the description’s allowed special characters (which include symbols like !). Explicitly list the allowed set and ensure tests match it. Consider adding at least one positive case for an allowed special character (e.g., user+tag@mail.com or user!@mail.com if ! is allowed).
- Password Cyrillic acceptance: Add a positive test proving Cyrillic letters are accepted when other requirements are met.
Minor cleanup (non-blocking): fix the typo in the test name to “email should have '@'”.

You’ve built a strong suite overall; aligning the signature, adding the 500-case, and clarifying allowed characters will make it fair and reliable for all implementations.

✨ 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 · 2025-09-26T07:07:57Z

src/validateRegisterForm.test.js

  });

+  it(`email should contain only english letter`, () => {
+    const invalidPassword = validateRegisterForm('tęst@mail.com', 'P@ssword1');


The call on this line uses the email as the first argument ('tęst@mail.com', 'P@ssword1'). This is an instance of the larger argument-order mismatch: tests consistently call validateRegisterForm(email, password), but the project description/examples indicate (password, email). Decide which signature is canonical and make tests and description consistent. Currently this will cause otherwise-correct implementations to fail.

mateacademy-ai-mentor · 2025-09-26T07:07:57Z

src/validateRegisterForm.test.js

+  });
+
+  it(`email should not contain special characters`, () => {
+    const invalidPassword = validateRegisterForm('test]@mail.com', 'P@ssword1');


This test checks that an email containing ] is invalid ('test]@mail.com'). That is fine if ] is intended to be disallowed, but reconcile this with the description's list of allowed special characters (which currently includes characters such as !). Explicitly confirm which special characters are allowed in the local-part and update tests/description accordingly to remove ambiguity.

mateacademy-ai-mentor · 2025-09-26T07:07:57Z

src/validateRegisterForm.test.js

+    expect(invalidPassword.message).toBe('Email is invalid.');
+  });
+
+  it(`email should should have '@'`, () => {


Typo in test name: change "email should should have '@'" to "email should have '@'" for clarity. (Not functionally blocking.)

mateacademy-ai-mentor · 2025-09-26T07:07:58Z

src/validateRegisterForm.test.js

+
+    expect(invalidPassword.code).toBe(422);
+    expect(invalidPassword.message).toBe('Password is invalid.');
+  });


Add a test here (or just after the suite) that asserts the combined-invalid case returns { code: 500, message: 'Password and email are invalid.' }. The project description requires this behavior, but I do not see a test covering it in the file. Adding it will ensure full compliance.

mateacademy-ai-mentor · 2025-09-26T07:07:58Z

src/validateRegisterForm.test.js

+    expect(invalidPassword.message).toBe('Password is invalid.');
+  });
+
+  it(`password should contain at least


The test description around the password checks references the requirement for at least 1 digit, 1 special char, and 1 uppercase letter. Additionally, the spec requires that Cyrillic letters are accepted in passwords. Consider adding a positive test that uses Cyrillic letters in the password to ensure implementations do not reject them unintentionally.

mateacademy-ai-mentor

Review Summary

Educational approval: You’ve built a solid Jest suite that covers all four required branches, includes precise code/message assertions, and nicely validates password rules (great job keeping the Cyrillic acceptance test like P@ssword1Д — that aligns with the spec). However, there are two blocking inconsistencies to address so correct implementations won’t fail the tests.

Critical issues to fix

Argument order mismatch (multiple lines in src/validateRegisterForm.test.js): The spec states validateRegisterForm(password, email), but many tests call it as (email, password). For example, the test calling validateRegisterForm('tęst@mail.com', 'P@ssword1') should be validateRegisterForm('P@ssword1', 'tęst@mail.com'). Why: otherwise implementations that follow the spec will fail. How to fix: swap arguments in every invocation so the first param is password and the second is email.
Allowed email special character contradiction: A test rejects 'test!@mail.com', but the description allows ! in the local part. Why: this directly contradicts the rules and produces false negatives. How to fix: either make it a positive test (expect valid for an allowed char like ! or +) or replace with a truly invalid email like test]@mail.com and keep it negative. Also update the combined-invalid test to continue exercising the 500 path with both inputs invalid.

Minor improvements

Add/keep at least one positive test explicitly accepting an allowed special character in the local part (e.g., user+tag@mail.com) with a valid password, to enforce the intended rule.

You’re very close — with the argument order standardized and the special-character tests aligned to the spec, your suite will be reliable and instructive. 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 · 2025-09-26T07:30:03Z

src/validateRegisterForm.test.js

+  it(`email should contain only english letter`, () => {
+    const invalidPassword = validateRegisterForm('tęst@mail.com', 'P@ssword1');
+
+    expect(invalidPassword.code).toBe(422);
+    expect(invalidPassword.message).toBe('Email is invalid.');
+  });


Critical: This test calls validateRegisterForm('tęst@mail.com', 'P@ssword1') with the email as the first argument. The task description requires the signature (password, email) (password first). Update this call to pass (password, email) so the tests match the spec. More generally, ensure every test in this file uses the canonical argument order from the description, otherwise correct implementations will fail the tests. See the description for the required signature and rules.

mateacademy-ai-mentor · 2025-09-26T07:30:03Z

src/validateRegisterForm.test.js

+  it(`email should not contain special characters`, () => {
+    const invalidPassword = validateRegisterForm('test!@mail.com', 'P@ssword1');
+
+    expect(invalidPassword.code).toBe(422);
+    expect(invalidPassword.message).toBe('Email is invalid.');
+  });


Important: This test uses 'test!@mail.com' and expects the email to be invalid. The description explicitly lists ! among allowed special characters in the email local part, so this input should be accepted by the email rules. Either change this test to a positive acceptance test (expect success for an allowed special character) or replace the input with an email containing a truly disallowed character (for example 'test]@mail.com') and expect invalid. Update any other tests that reuse 'test!@mail.com' accordingly.

mateacademy-ai-mentor · 2025-09-26T07:30:03Z

src/validateRegisterForm.test.js

+  it(`email should not have dots on the beginning`, () => {
+    const invalidPassword = validateRegisterForm('.test@mail.com', 'P@ssword1');
+
+    expect(invalidPassword.code).toBe(422);
+    expect(invalidPassword.message).toBe('Email is invalid.');
+  });
+
+  it(`email should have '@'`, () => {
+    const invalidPassword = validateRegisterForm('testmail.com', 'P@ssword1');
+
+    expect(invalidPassword.code).toBe(422);
+    expect(invalidPassword.message).toBe('Email is invalid.');
+  });
+
+  it(`email should not start from dot in domain part`, () => {
+    const invalidPassword = validateRegisterForm('test@.mail.com', 'P@ssword1');
+
+    expect(invalidPassword.code).toBe(422);
+    expect(invalidPassword.message).toBe('Email is invalid.');
+  });
+
+  it(`email should not contain double dots`, () => {
+    const invalidPassword
+     = validateRegisterForm('test@..mail.com', 'P@ssword1');
+
+    expect(invalidPassword.code).toBe(422);
+    expect(invalidPassword.message).toBe('Email is invalid.');
+  });


The file contains many more calls to validateRegisterForm that likewise pass (email, password). After you fix the examples above, swap arguments in all other test invocations so they use (password, email) consistently. This change touches the majority of validateRegisterForm calls in the file — please update them all to match the spec.

mateacademy-ai-mentor · 2025-09-26T07:30:03Z

src/validateRegisterForm.test.js

+  it(`password should accept cyryllic letters`, () => {
+    const invalidPassword = validateRegisterForm('test@mail.com', 'P@ssword1Д');
+
+    expect(invalidPassword.code).toBe(200);
+    expect(invalidPassword.message).toBe('Email and password are valid.');
+  });


Good: This test verifies acceptance of Cyrillic letters in the password by passing 'P@ssword1Д' and expecting a success response. Keep this — it validates the requirement that passwords accept Cyrillic characters when other constraints are met.

mateacademy-ai-mentor · 2025-09-26T07:30:03Z

src/validateRegisterForm.test.js

+  it(`should return message when password and email are invalid`, () => {
+    const invalidPassword = validateRegisterForm('test!@mail.com', 'pass');
+
+    expect(invalidPassword.code).toBe(500);
+    expect(invalidPassword.message).toBe('Password and email are invalid.');
+  });


Combined-invalid test: currently uses 'test!@mail.com' with 'pass' and expects the combined 500 error. If you change how 'test!@mail.com' is treated (per the comment above), update this test input so it still exercises the combined-invalid branch (both email and password invalid). Ensure the test continues to expect { code: 500, message: 'Password and email are invalid.' }.

check_form

e8514c5

mateacademy-ai-mentor suggested changes Sep 26, 2025

View reviewed changes

changing order of tests and fix test SPECIAL CHARACTERS

0b8cff7

robsza1122 requested a review from mateacademy-ai-mentor September 26, 2025 07:02

mateacademy-ai-mentor suggested changes Sep 26, 2025

View reviewed changes

fix mistakes

5bcb3df

robsza1122 requested a review from mateacademy-ai-mentor September 26, 2025 07:25

mateacademy-ai-mentor approved these changes Sep 26, 2025

View reviewed changes

Conversation

robsza1122 commented Sep 26, 2025

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants