-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(frontend): Add confirm password and fix sign up problem when user does not verify. #188
Conversation
WalkthroughThis update enhances the user registration process by adding password confirmation and additional validations. In the backend, the registration logic now accepts a Changes
Sequence Diagram(s)sequenceDiagram
participant UI as SignUpModal
participant Resolver as UserResolver
participant Auth as AuthService
participant DB as Database
participant Email as EmailService
UI->>UI: Validate password strength & match locally
UI->>Resolver: Submit registration data
Resolver->>Auth: Call register() with user input
Auth->>Auth: Check if password matches confirmPassword
alt Passwords do not match
Auth-->>Resolver: Return error "Passwords do not match"
else
Auth->>DB: Check for existing user by email
alt User exists and unconfirmed
Auth->>DB: Update user with new credentials
Auth->>Email: Resend verification email
else User exists and confirmed
Auth-->>Resolver: Return error "Email already exists"
else New user
Auth->>DB: Create new user
Auth->>Email: Send verification email
end
end
Resolver-->>UI: Return registration outcome
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/src/components/sign-up-modal.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/frontend/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. backend/src/auth/auth.service.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-prettier". (The package "eslint-plugin-prettier" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "eslint-plugin-prettier" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/user/dto/register-user.input.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-prettier". (The package "eslint-plugin-prettier" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "eslint-plugin-prettier" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…y/codefox into feat-frontend-fix-signup
…y/codefox into feat-frontend-fix-signup
…y/codefox into feat-frontend-fix-signup
…y/codefox into feat-frontend-fix-signup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/src/user/user.resolver.ts (1)
66-68
: Consider using built-in NestJS exceptions instead of generic ErrorYour password validation logic is correct, but using a generic Error doesn't align with NestJS's standard error handling practices.
- if (registerUserInput.password.length < 6) { - throw new Error('Password must be at least 6 characters'); - } + if (registerUserInput.password.length < 6) { + throw new BadRequestException('Password must be at least 6 characters'); + }Don't forget to import BadRequestException from @nestjs/common at the top of the file.
backend/src/user/dto/register-user.input.ts (1)
16-19
: Consider adding a custom validator for password matchingWhile the password matching logic is currently in the service layer, you might consider adding a custom validator to ensure passwords match at the DTO level.
This would ensure validation happens before reaching your service logic:
import { InputType, Field } from '@nestjs/graphql'; import { IsEmail, IsString, MinLength, Validate } from 'class-validator'; // Example custom validator (would need to be implemented) class PasswordsMatchConstraint { validate(registerUserInput: any) { return registerUserInput.password === registerUserInput.confirmPassword; } defaultMessage() { return 'Passwords do not match'; } } @InputType() export class RegisterUserInput { // existing fields... @Validate(PasswordsMatchConstraint) confirmPassword: string; }backend/src/auth/auth.service.ts (2)
158-168
: Improved user registration flow for unconfirmed emailsThe logic for handling existing users has been improved to update the user's information if their email is not confirmed, rather than rejecting the registration outright.
Consider adding a rate limit or cooldown for unconfirmed email updates to prevent abuse. Someone could repeatedly attempt to register with another person's email, potentially changing their username/password if they eventually confirm.
// If the user exists but email is not confirmed and mail is enabled if (existingUser && !existingUser.isEmailConfirmed && this.isMailEnabled) { + // Check if a cooldown period has passed (similar to your email resend cooldown) + const cooldownPeriod = 5 * 60 * 1000; // 5 minutes in milliseconds + if ( + existingUser.lastRegistrationAttempt && + new Date().getTime() - existingUser.lastRegistrationAttempt.getTime() < cooldownPeriod + ) { + throw new ConflictException('Please wait before trying to register with this email again'); + } + // Just update the existing user and resend verification email existingUser.username = username; existingUser.password = hashedPassword; + existingUser.lastRegistrationAttempt = new Date(); await this.userRepository.save(existingUser); await this.sendVerificationEmail(existingUser); return existingUser;This would require adding a
lastRegistrationAttempt
field to the User entity.
158-168
: Consider refactoring complex conditional logicThe nested conditional logic for handling existing users can be difficult to maintain. Consider refactoring into a more structured approach.
You could extract this logic into separate, well-named methods:
private async handleExistingUser(existingUser: User, username: string, hashedPassword: string): Promise<User> { if (!existingUser.isEmailConfirmed && this.isMailEnabled) { return this.updateUnconfirmedUser(existingUser, username, hashedPassword); } throw new ConflictException('Email already exists'); } private async updateUnconfirmedUser(user: User, username: string, hashedPassword: string): Promise<User> { user.username = username; user.password = hashedPassword; await this.userRepository.save(user); await this.sendVerificationEmail(user); return user; }Then your register method would be cleaner:
async register(registerUserInput: RegisterUserInput): Promise<User> { // Validation code... const existingUser = await this.userRepository.findOne({ where: { email }, }); if (existingUser) { return this.handleExistingUser(existingUser, username, hashedPassword); } // Create new user code... }frontend/src/components/sign-up-modal.tsx (1)
52-87
: Well-implemented password validation functionThe password validation function provides comprehensive checks and appropriate feedback to users.
Consider extracting the regex patterns to constants to avoid duplication and improve maintainability. These patterns are used in both the validation function and the UI display:
// At the top of your file or in a separate utils file const PASSWORD_PATTERNS = { UPPERCASE: /[A-Z]/, LOWERCASE: /[a-z]/, NUMBERS: /\d/, SPECIAL_CHARS: /[!@#$%^&*()_+\-=[\]{};':"\\|,.<>/?]/ }; // Then in your validation function const hasUppercase = PASSWORD_PATTERNS.UPPERCASE.test(value); // etc. // And in your UI <li className={PASSWORD_PATTERNS.UPPERCASE.test(password) ? 'text-green-500' : ''}> Include at least one uppercase letter </li>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/auth/auth.service.ts
(1 hunks)backend/src/user/dto/register-user.input.ts
(1 hunks)backend/src/user/user.resolver.ts
(1 hunks)frontend/src/components/sign-up-modal.tsx
(7 hunks)
🔇 Additional comments (10)
backend/src/user/user.resolver.ts (1)
66-68
: Validation duplication with DTOThis validation duplicates the
@MinLength(6)
decorator already present in the DTO. While redundant validation doesn't hurt, it creates a maintenance burden if requirements change.Consider whether this validation could be removed since class-validator should handle it, or ensure it's consistent with the DTO validation. If you prefer belt-and-suspenders validation, you might want to extract the minimum length to a constant shared between files.
backend/src/user/dto/register-user.input.ts (1)
16-19
: LGTM! Good addition of confirmPassword fieldThe confirmPassword field has been properly set up with appropriate decorators matching the password field constraints.
backend/src/auth/auth.service.ts (2)
145-146
: LGTM! Added confirmPassword parameter extractionCorrectly extracting the new confirmPassword field from the input.
152-154
: LGTM! Password matching validationGood addition of password matching validation before continuing with registration.
frontend/src/components/sign-up-modal.tsx (6)
46-50
: LGTM! Added state variables for password confirmation and validationGood addition of state variables to manage password confirmation, error messages, and strength indicators.
111-118
: LGTM! Improved form validationGood implementation of password validation and confirmation checks before submission.
127-127
: LGTM! Added confirmPassword to form submissionCorrectly including the confirmPassword field in the GraphQL mutation.
275-310
: Verify social login functionalityThe UI for social login buttons has been added, but it's unclear if the backend integration exists to support this functionality.
Make sure the backend has corresponding OAuth integration for Google and GitHub login, or add a TODO comment if this feature is planned for future implementation.
358-426
: Great implementation of password strength meter and requirements listThe password strength visualization and detailed requirements list with real-time feedback provides an excellent user experience.
This implementation gives users clear feedback about password requirements and helps them create stronger passwords.
429-442
: LGTM! Added password confirmation fieldGood implementation of the confirmation password field matching the UX of the original password field.
Summary by CodeRabbit