Skip to content

Walidacja formularza rejestracji: testy email i hasła#130

Open
JoAnna578 wants to merge 9 commits intomate-academy:mainfrom
JoAnna578:develop
Open

Walidacja formularza rejestracji: testy email i hasła#130
JoAnna578 wants to merge 9 commits intomate-academy:mainfrom
JoAnna578:develop

Conversation

@JoAnna578
Copy link

Kilka słów kluczowych: Walidacja,Formularz rejestracji,Email,Hasło,Testy

Kilka słów kluczowych: Walidacja,Formularz rejestracji,Email,Hasło,Testy
Copy link

@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 all test calls use the wrong parameter order for validateRegisterForm, which is a critical blocker. The specification requires validateRegisterForm(password, email), but between lines 11–74 the tests call it as (email, password), causing email strings to be validated as passwords and vice versa. This will produce misleading failures even if the implementation is correct.

What to fix (blocking):

  • Update every invocation between lines 11–74 so the first argument is the password and the second is the email, e.g., change validateRegisterForm('test@mail.com', 'P@ssword1!') to validateRegisterForm('P@ssword1!', 'test@mail.com').

Strengths: the tests assert the exact status codes and messages and cover many invalid cases. Optional improvements (non-blocking): consider adding tests for missing uppercase, missing special character, password length boundaries (exactly 8 and 16), trailing dot in email, and a positive case with allowed special characters in the local part. Once the argument order is corrected, the suite should properly validate the implementation. Keep up the solid structure and clear expectations!


✨ 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 on lines +11 to +74
const result = validateRegisterForm('test@mail.com', 'P@ssword1!');
expect(typeof result).toBe('object');
});

it(`should return success message for the valid input`, () => {
const isValid = validateRegisterForm('test@mail.com', 'P@ssword1!');
it(`should return 200 for valid email and password`, () => {
const valid = validateRegisterForm('test@mail.com', 'P@ssword1!');
expect(valid.code).toBe(200);
expect(valid.message).toBe('Email and password are valid.');
});

it(`should return 422 for valid email and password missing digit`, () => {
const invalidPass = validateRegisterForm('test@mail.com', 'P@ssword');
expect(invalidPass.code).toBe(422);
expect(invalidPass.message).toBe('Password is invalid.');
});

expect(isValid.code).toBe(200);
expect(isValid.message).toBe('Email and password are valid.');
it(`should return 422 for invalid email and valid password`, () => {
const invalidEmail = validateRegisterForm('test@com', 'P@ssword1!');
expect(invalidEmail.code).toBe(422);
expect(invalidEmail.message).toBe('Email is invalid.');
});

it(`should return error for valid email and password without number`, () => {
const invalidPassword = validateRegisterForm('test@mail.com', 'P@ssword');
it(`should return 500 for invalid email and password`, () => {
const bothInvalid = validateRegisterForm('test@com', 'ssword1');
expect(bothInvalid.code).toBe(500);
expect(bothInvalid.message)
.toBe('Password and email are invalid.');
});

expect(invalidPassword.code).toBe(422);
expect(invalidPassword.message).toBe('Password is invalid.');
it(`should return 422 if password too short`, () => {
const tooShort = validateRegisterForm('valid@mail.com', 'Aa1!');
expect(tooShort.code).toBe(422);
expect(tooShort.message).toBe('Password is invalid.');
});

// write more tests here
it(`should return 422 if password too long`, () => {
const tooLong = validateRegisterForm(
'valid@mail.com', 'A1!aaaaaaaaaaaaaaa'
);
expect(tooLong.code).toBe(422);
expect(tooLong.message).toBe('Password is invalid.');
});

it(`should return 422 if email missing '@' symbol`, () => {
const noAt = validateRegisterForm('invalidmail.com', 'P@ssword1!');
expect(noAt.code).toBe(422);
expect(noAt.message).toBe('Email is invalid.');
});

it(`should return 422 if email starts with dot`, () => {
const startDot = validateRegisterForm('.test@mail.com', 'P@ssword1!');
expect(startDot.code).toBe(422);
expect(startDot.message).toBe('Email is invalid.');
});

it(`should return 422 if email has double dots`, () => {
const doubleDot = validateRegisterForm('test..mail@mail.com', 'P@ssword1!');
expect(doubleDot.code).toBe(422);
expect(doubleDot.message).toBe('Email is invalid.');
});

it(`should return 422 if top-level domain starts with dot`, () => {
const tldDot = validateRegisterForm('test@mail..com', 'P@ssword1!');
expect(tldDot.code).toBe(422);

Choose a reason for hiding this comment

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

Issue: All test invocations pass arguments in (email, password) order, but the specification requires the signature validateRegisterForm(password, email). Passing arguments reversed will feed the email into the password checks and vice versa, causing incorrect test behavior.

Why it matters: The test harness must call the function using the correct parameter order described in the task so it validates the implementation properly.

What to change: Update each call so the first argument is the password string and the second is the email string. Example change:

// current
validateRegisterForm('test@mail.com', 'P@ssword1!')
// should be
validateRegisterForm('P@ssword1!', 'test@mail.com')

Locations to update: every validateRegisterForm(...) call between lines 11 and 74 (all test invocations).

Copy link

@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’m requesting changes because there are critical blockers that violate the spec and will break functionality.

  • Parameter order mismatch: the function in src/validateRegisterForm.js is declared as validateRegisterForm(email, password) (see top of file) but the required signature is validateRegisterForm(password, email). This causes all validations to run on the wrong values. Fix by changing the function signature and internal usage to (password, email) and updating all references accordingly.
  • Tests call order: every test in src/validateRegisterForm.test.js passes (email, password) (e.g., around the first set of calls). Update all invocations to validateRegisterForm(password, email) so they match the required signature.
  • Password regex: the lowercase (?=.*[a-z]) and uppercase (?=.*[A-Z]) checks only cover Latin letters and the special-character check (?=.*[^a-zA-Z0-9]) will classify Cyrillic as special. The requirement is to allow English and Cyrillic letters and detect uppercase across both. Update the letter checks to include -based Cyrillic ranges (e.g., placeholders such as to ) or use Unicode property escapes with the u flag, and adjust the special-character check to exclude both Latin and Cyrillic letters and digits.
  • Email local-part: the regex only allows \w, hyphen, and dot groups, but the spec permits these additional characters: ! # $ % & ' * + - / = ? ^ _
    { | } ~`. Expand the local-part character class to include them while preserving the dot rules (no leading/trailing dot, no consecutive dots). Keep your existing checks for leading dot, starting dot in domain, and double dots.

Minor improvement: consider restricting domain labels to [A-Za-z0-9-] (underscore is not standard in domain names). Once these are addressed, your result-mapping logic should work correctly. Good job on the branching and extra dot validations; with the fixes above, you’ll meet the 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +25 to +26
const isEmailValid = validEmailMask.test(email);
const isPasswordValid = validPassword.test(password);

Choose a reason for hiding this comment

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

The code currently runs the email and password tests by using the parameter names email and password here. The task requires the function signature to be validateRegisterForm(password, email) (password first, email second). Because the implementation expects the reversed order, callers/tests that follow the spec will pass arguments in the opposite order and validation will be performed on the wrong values. Please update the function signature and parameter usage to match the required order, or (preferably) change the function declaration to accept (password, email) so it matches the specification exactly.

const validPassword = /^(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[^a-zA-Z0-9])(?!.*\s).{8,16}$/;
// Password regex: krótkie zmienne
const n = '(?=.*\\d)';
const l = '(?=.*[a-z])';

Choose a reason for hiding this comment

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

The lowercase check (?=.*[a-z]) and uppercase check (?=.*[A-Z]) only match Latin letters. The task requires that allowed letters include both English and Cyrillic (А–Я, а–я). Update these parts to include the Cyrillic ranges (for example a-zA-Z\u0400-\u04FF or use Unicode property escapes) so uppercase detection and lowercase detection work for Cyrillic characters as well.

const n = '(?=.*\\d)';
const l = '(?=.*[a-z])';
const u = '(?=.*[A-Z])';
const s = '(?=.*[^a-zA-Z0-9])';

Choose a reason for hiding this comment

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

The special-character assertion (?=.*[^a-zA-Z0-9]) treats any character outside the ASCII letters/digits range as a special character. That will incorrectly classify Cyrillic letters as "special characters". After you add Cyrillic ranges to the letter classes, update this special-character check so it excludes Cyrillic letters (e.g. use a negative class that excludes the combined letter ranges and digits). Consider using Unicode property escapes (e.g. \p{L}) with the u flag to make this clearer and correct.


// eslint-disable-next-line max-len
const validEmailMask = new RegExp(/^([\w-]+(?:\.[\w-]+)*)@((?:[\w-]+\.)*\w[\w-]{0,66})\./i);
const validPassword = new RegExp('^' + n + l + u + s + ns + len + '$');

Choose a reason for hiding this comment

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

validPassword is built from multiple parts and uses .{8,16} for length and (?!.*\s) to forbid spaces — those parts are fine for enforcing the 8–16 length and no-space requirement. However to fully meet the spec you must ensure the letter checks include Cyrillic (see earlier comments) and that the special-character check doesn't capture Cyrillic as special. Also consider using the u flag and explicit Unicode ranges to make the regex robust.

Comment on lines +20 to +23
const loc = '([\\w-]+(?:\\.[\\w-]+)*)';
const dom = '([\\w-]+(?:\\.[\\w-]+)+)';
const at = '@';
const validEmailMask = new RegExp('^' + loc + at + dom + '$', 'i');

Choose a reason for hiding this comment

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

The local-part (loc) and domain (dom) regex parts only allow \w/-/dot groups. That excludes the list of allowed special characters specified in the task (for example: ! # $ % & ' * + - / = ? ^ _ { | } ~). The local-part must accept those characters in addition to letters and digits (while still enforcing the dot rules). Please expand the local-part regex to include the allowed characters and ensure the domain rules still satisfy the TLD constraints. Also note \w` includes underscore — which is allowed — but you still need to explicitly permit the full allowed set from the spec.

Comment on lines +35 to +37
const localInvalid = local.startsWith('.');
const domainInvalid = domain.startsWith('.');
const dotsInvalid = email.includes('..');

Choose a reason for hiding this comment

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

The additional checks for leading dots and double dots (local.startsWith('.'), domain.startsWith('.'), email.includes('..')) are a useful supplement to the regex and correctly address several dot-related rules. Good to keep these, but do not rely on them instead of updating the main regex: both should work together so the allowed-special-chars requirement and dot rules are all satisfied.

Comment on lines +45 to +49
if (!isEmailValid || !emailExtraValid) {
if (isPasswordValid) {
return { code: 422, message: 'Email is invalid.' };
} else {
return { code: 500, message: 'Password and email are invalid.' };

Choose a reason for hiding this comment

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

Validation flow (the result-code branching) correctly distinguishes the four required outcomes: both valid -> 200, email invalid & password valid -> 422 (Email), password invalid & email valid -> 422 (Password), both invalid -> 500. After fixing the parameter order and the regexes above this branching logic will produce the expected outputs and messages. No change required here besides ensuring the earlier issues are fixed so the flags evaluated here are accurate.

Comment on lines +11 to +76
const result = validateRegisterForm('test@mail.com', 'P@ssword1!');
expect(typeof result).toBe('object');
});

it(`should return success message for the valid input`, () => {
const isValid = validateRegisterForm('test@mail.com', 'P@ssword1!');
it(`should return 200 for valid email and password`, () => {
const valid = validateRegisterForm('test@mail.com', 'P@ssword1!');
expect(valid.code).toBe(200);
expect(valid.message).toBe('Email and password are valid.');
});

it(`should return 422 for valid email and password missing digit`, () => {
const invalidPass = validateRegisterForm('test@mail.com', 'P@ssword');
expect(invalidPass.code).toBe(422);
expect(invalidPass.message).toBe('Password is invalid.');
});

expect(isValid.code).toBe(200);
expect(isValid.message).toBe('Email and password are valid.');
it(`should return 422 for invalid email and valid password`, () => {
const invalidEmail = validateRegisterForm('test@com', 'P@ssword1!');
expect(invalidEmail.code).toBe(422);
expect(invalidEmail.message).toBe('Email is invalid.');
});

it(`should return error for valid email and password without number`, () => {
const invalidPassword = validateRegisterForm('test@mail.com', 'P@ssword');
it(`should return 500 for invalid email and password`, () => {
const bothInvalid = validateRegisterForm('test@com', 'ssword1');
expect(bothInvalid.code).toBe(500);
expect(bothInvalid.message)
.toBe('Password and email are invalid.');
});

expect(invalidPassword.code).toBe(422);
expect(invalidPassword.message).toBe('Password is invalid.');
it(`should return 422 if password too short`, () => {
const tooShort = validateRegisterForm('valid@mail.com', 'Aa1!');
expect(tooShort.code).toBe(422);
expect(tooShort.message).toBe('Password is invalid.');
});

// write more tests here
it(`should return 422 if password too long`, () => {
const tooLong = validateRegisterForm(
'valid@mail.com', 'A1!aaaaaaaaaaaaaaa'
);
expect(tooLong.code).toBe(422);
expect(tooLong.message).toBe('Password is invalid.');
});

it(`should return 422 if email missing '@' symbol`, () => {
const noAt = validateRegisterForm('invalidmail.com', 'P@ssword1!');
expect(noAt.code).toBe(422);
expect(noAt.message).toBe('Email is invalid.');
});

it(`should return 422 if email starts with dot`, () => {
const startDot = validateRegisterForm('.test@mail.com', 'P@ssword1!');
expect(startDot.code).toBe(422);
expect(startDot.message).toBe('Email is invalid.');
});

it(`should return 422 if email has double dots`, () => {
const doubleDot = validateRegisterForm('test..mail@mail.com', 'P@ssword1!');
expect(doubleDot.code).toBe(422);
expect(doubleDot.message).toBe('Email is invalid.');
});

it(`should return 422 if top-level domain starts with dot`, () => {
const tldDot = validateRegisterForm('test@mail..com', 'P@ssword1!');
expect(tldDot.code).toBe(422);
expect(tldDot.message).toBe('Email is invalid.');
});

Choose a reason for hiding this comment

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

All test invocations in this block pass arguments in the wrong order. The required function signature is validateRegisterForm(password, email) per the task spec, but the tests call validateRegisterForm(email, password). Update every call in this range so the first argument is the password string and the second is the email string. Example:

  • Wrong: validateRegisterForm('test@mail.com', 'P@ssword1!')
  • Correct: validateRegisterForm('P@ssword1!', 'test@mail.com')

Fix the calls used for: returning an object, 200 valid case, invalid password, invalid email, both invalid, password too short, password too long, missing '@', starting dot, double dots, and TLD dot checks. After changing the tests, ensure the implementation signature matches the required (password, email) order (the implementation file currently defines function validateRegisterForm(email, password) and needs to be aligned) — see the task description and implementation for context.

Copy link

@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

Here is my approval to help you keep progressing while learning. Overall, your function and tests meet the core requirements: correct signature (password, email), proper result mapping, solid email validation (including allowed specials, dot rules, and extra dot checks), and comprehensive Jest tests. The one critical fix needed is in src/validateRegisterForm.js lines 11–13: the uppercase lookahead (?=.*[A-Z\u0400-\u04FF]) can be satisfied by lowercase Cyrillic letters because the range \u0400-\u04FF contains both cases, so it doesn’t guarantee an uppercase letter as required. To address this, use Unicode-aware properties with the u flag, e.g., (?=.*\p{Lu}) for uppercase and (?=.*[^\p{L}\d]) for special characters; alternatively, restrict the uppercase class to explicit Cyrillic uppercase ranges only. As a minor improvement, consider adding a few tests that include Cyrillic-only passwords to confirm uppercase detection works correctly after the change. Nice work on aligning the return codes/messages and covering realistic cases—keep it up!


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

Footnotes

  1. Rate AI review example

Comment on lines +11 to +13
const l = '(?=.*[a-zA-Z\u0400-\u04FF])';
const u = '(?=.*[A-Z\u0400-\u04FF])';
const s = '(?=.*[^\\da-zA-Z\u0400-\u04FF])';

Choose a reason for hiding this comment

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

High severity — uppercase detection is incorrect.

You build the password lookaheads on lines 11–13: l = '(?=.*[a-zA-Z\u0400-\u04FF])', u = '(?=.*[A-Z\u0400-\u04FF])', and s = '(?=.*[^\\da-zA-Z\u0400-\u04FF])'. The Cyrillic block \u0400-\u04FF contains both uppercase and lowercase Cyrillic letters. That means the (?=.*[A-Z\u0400-\u04FF]) lookahead intended to detect an uppercase letter can accidentally match lowercase Cyrillic characters and thus incorrectly satisfy the uppercase requirement.

Recommendation: use Unicode-aware checks so uppercase is properly detected. For example, with Unicode property escapes and keeping the u flag you can require an uppercase letter with (?=.*\p{Lu}) and require a special character with something like (?=.*[^\p{L}\d]). Alternatively, explicitly use uppercase Cyrillic ranges in the uppercase lookahead rather than including the entire Cyrillic block in both letter and uppercase classes. Keep the u flag on the RegExp when you use \p{…}. This change will ensure the password truly contains at least one uppercase letter for both Latin and Cyrillic alphabets.

(Refer to lines: 11–13.)

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