From 59ef3e11dc2a176240935738db3fb34e430c1c30 Mon Sep 17 00:00:00 2001 From: sublime247 Date: Wed, 25 Mar 2026 15:12:43 +0100 Subject: [PATCH] refactor: eliminate duplicate user lookup patterns - Created reusable user utilities in src/common/utils/user.utils.ts - Handled exceptions like ensureUserExists seamlessly. - Extracted findUserOrThrow inside users.service.ts. - Adopted shared lookup mechanisms in auth.service.ts and payments.service.ts. - Enforced uniform exception types across user operations. Close #206 --- src/auth/auth.service.ts | 49 ++++++++---------- src/common/utils/user.utils.ts | 88 ++++++++++++++++++++++++++++++++ src/payments/payments.service.ts | 13 ++--- src/users/users.service.ts | 46 ++++++++--------- 4 files changed, 137 insertions(+), 59 deletions(-) create mode 100644 src/common/utils/user.utils.ts diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 3742079..a6bcfde 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -8,6 +8,11 @@ import { randomBytes } from 'crypto'; import { SessionService } from '../session/session.service'; import { TransactionService } from '../common/database/transaction.service'; import { UserRole } from '../users/entities/user.entity'; +import { + ensureValidCredentials, + ensureUserIsActive, + ensureValidUserToken, +} from '../common/utils/user.utils'; interface JwtTokenPayload { sub: string; @@ -102,10 +107,8 @@ export class AuthService { async login(loginDto: LoginDto): Promise { // Find user - const user = await this.usersService.findByEmail(loginDto.email); - if (!user) { - throw new UnauthorizedException('Invalid credentials'); - } + const userOrNull = await this.usersService.findByEmail(loginDto.email); + const user = ensureValidCredentials(userOrNull); // Verify password const isPasswordValid = await bcrypt.compare(loginDto.password, user.password); @@ -114,9 +117,7 @@ export class AuthService { } // Check if user is active - if (user.status !== 'active') { - throw new UnauthorizedException('Account is not active'); - } + ensureUserIsActive(user); // Update last login await this.usersService.updateLastLogin(user.id); @@ -221,16 +222,13 @@ export class AuthService { async resetPassword(resetPasswordDto: ResetPasswordDto): Promise<{ message: string }> { // Find user by reset token - const user = await this.usersService.findByPasswordResetToken(resetPasswordDto.token); - - if (!user || !user.passwordResetToken || !user.passwordResetExpires) { - throw new BadRequestException('Invalid or expired reset token'); - } - - // Check if token is expired - if (new Date() > user.passwordResetExpires) { - throw new BadRequestException('Invalid or expired reset token'); - } + const userOrNull = await this.usersService.findByPasswordResetToken(resetPasswordDto.token); + const user = ensureValidUserToken( + userOrNull, + 'passwordResetToken', + 'passwordResetExpires', + 'Invalid or expired reset token', + ); // Update password await this.usersService.update(user.id, { password: resetPasswordDto.newPassword }); @@ -261,16 +259,13 @@ export class AuthService { async verifyEmail(token: string): Promise<{ message: string }> { // Find user by verification token - const user = await this.usersService.findByEmailVerificationToken(token); - - if (!user || !user.emailVerificationToken || !user.emailVerificationExpires) { - throw new BadRequestException('Invalid or expired verification token'); - } - - // Check if token is expired - if (new Date() > user.emailVerificationExpires) { - throw new BadRequestException('Invalid or expired verification token'); - } + const userOrNull = await this.usersService.findByEmailVerificationToken(token); + const user = ensureValidUserToken( + userOrNull, + 'emailVerificationToken', + 'emailVerificationExpires', + 'Invalid or expired verification token', + ); // Update user as verified await this.usersService.update(user.id, { isEmailVerified: true }); diff --git a/src/common/utils/user.utils.ts b/src/common/utils/user.utils.ts new file mode 100644 index 0000000..e095587 --- /dev/null +++ b/src/common/utils/user.utils.ts @@ -0,0 +1,88 @@ +import { + NotFoundException, + UnauthorizedException, + BadRequestException, + ConflictException, +} from '@nestjs/common'; +import { User } from '../../users/entities/user.entity'; + +/** + * Ensures a user exists, throwing a NotFoundException otherwise. + * @param user The user object to check + * @param message Optional custom error message + * @returns The guaranteed non-null user object + */ +export function ensureUserExists(user: User | null | undefined, message = 'User not found'): User { + if (!user) { + throw new NotFoundException(message); + } + return user; +} + +/** + * Ensures a user exists for authentication purposes, throwing an UnauthorizedException otherwise. + * @param user The user object to check + * @param message Optional custom error message + * @returns The guaranteed non-null user object + */ +export function ensureValidCredentials( + user: User | null | undefined, + message = 'Invalid credentials', +): User { + if (!user) { + throw new UnauthorizedException(message); + } + return user; +} + +/** + * Ensures a user's account is active. + * @param user The user object to check + * @param message Optional custom error message + */ +export function ensureUserIsActive(user: User, message = 'Account is not active'): void { + if (user.status !== 'active') { + throw new UnauthorizedException(message); + } +} + +/** + * Ensures a user has a valid and unexpired token for a specific field. + * @param user The user object to check + * @param tokenField The property name of the token on the user object + * @param expiresField The property name of the expiration date on the user object + * @param message Optional custom error message + * @returns The guaranteed non-null user object + */ +export function ensureValidUserToken( + user: User | null | undefined, + tokenField: keyof User, + expiresField: keyof User, + message = 'Invalid or expired token', +): User { + if (!user || !user[tokenField] || !user[expiresField]) { + throw new BadRequestException(message); + } + + const expireDate = user[expiresField] as Date; + if (new Date() > expireDate) { + throw new BadRequestException(message); + } + + return user; +} + +/** + * Ensures a user does not exist, throwing a ConflictException otherwise. + * Useful for registration or email updates. + * @param user The user object to check + * @param message Optional custom error message + */ +export function ensureUserDoesNotExist( + user: User | null | undefined, + message = 'User already exists', +): void { + if (user) { + throw new ConflictException(message); + } +} diff --git a/src/payments/payments.service.ts b/src/payments/payments.service.ts index e2ed5e2..5ebd9ce 100644 --- a/src/payments/payments.service.ts +++ b/src/payments/payments.service.ts @@ -10,6 +10,7 @@ import { Invoice, InvoiceStatus } from './entities/invoice.entity'; import { RefundDto } from './dto/refund.dto'; import { CreateSubscriptionDto } from './dto/create-subscription.dto'; import { TransactionService } from '../common/database/transaction.service'; +import { ensureUserExists } from '../common/utils/user.utils'; import { PaymentProvider, PaymentMetadata, @@ -71,12 +72,10 @@ export class PaymentsService { const { courseId, amount, currency, provider, metadata } = createPaymentDto; // Verify user exists - const user = await this.userRepository.findOne({ + const userOrNull = await this.userRepository.findOne({ where: { id: userId }, }); - if (!user) { - throw new NotFoundException('User not found'); - } + const user = ensureUserExists(userOrNull); // Get payment provider const paymentProvider = this.getProvider(provider ?? 'stripe'); @@ -119,13 +118,11 @@ export class PaymentsService { const { interval } = createSubscriptionDto; // Verify user exists - const user = await this.userRepository.findOne({ + const userOrNull = await this.userRepository.findOne({ where: { id: userId }, }); - if (!user) { - throw new NotFoundException('User not found'); - } + ensureUserExists(userOrNull); // Get payment provider // const paymentProvider = this.getProvider(provider); diff --git a/src/users/users.service.ts b/src/users/users.service.ts index af0cd7b..50cb018 100644 --- a/src/users/users.service.ts +++ b/src/users/users.service.ts @@ -1,11 +1,13 @@ -import { Injectable, NotFoundException, ConflictException } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { User } from './entities/user.entity'; import { CreateUserDto } from './dto/create-user.dto'; import { UpdateUserDto } from './dto/update-user.dto'; import * as bcrypt from 'bcryptjs'; +import { ensureUserExists, ensureUserDoesNotExist } from '../common/utils/user.utils'; import { paginate, PaginatedResponse } from '../common/utils/pagination.util'; +import { PaginationQueryDto } from '../common/dto/pagination.dto'; import { GetUsersDto } from './dto/get-users.dto'; import { CachingService } from '../caching/caching.service'; import { CACHE_TTL, CACHE_PREFIXES, CACHE_EVENTS } from '../caching/caching.constants'; @@ -25,10 +27,7 @@ export class UsersService { const existingUser = await this.userRepository.findOne({ where: { email: createUserDto.email }, }); - - if (existingUser) { - throw new ConflictException('User with this email already exists'); - } + ensureUserDoesNotExist(existingUser, 'User with this email already exists'); // Hash password const hashedPassword = await bcrypt.hash(createUserDto.password, 10); @@ -65,7 +64,7 @@ export class UsersService { ); } - return await paginate(query, filter); + return await paginate(query, filter || new PaginationQueryDto()); }, CACHE_TTL.USER_PROFILE, ); @@ -76,17 +75,22 @@ export class UsersService { return await this.userRepository.findByIds(ids); } + /** + * Helper method to find a user by ID or throw NotFoundException. + * Can be used internally to eliminate duplication. + */ + async findUserOrThrow(id: string): Promise { + const user = await this.userRepository.findOne({ where: { id } }); + return ensureUserExists(user, 'User not found'); + } + async findOne(id: string): Promise { const cacheKey = `${CACHE_PREFIXES.USER_PROFILE}:${id}`; return this.cachingService.getOrSet( cacheKey, async () => { - const user = await this.userRepository.findOne({ where: { id } }); - if (!user) { - throw new NotFoundException('User not found'); - } - return user; + return await this.findUserOrThrow(id); }, CACHE_TTL.USER_PROFILE, ); @@ -109,10 +113,7 @@ export class UsersService { } async update(id: string, updateUserDto: UpdateUserDto): Promise { - const user = await this.userRepository.findOne({ where: { id } }); - if (!user) { - throw new NotFoundException('User not found'); - } + const user = await this.findUserOrThrow(id); // If updating password, hash it if (updateUserDto.password) { @@ -129,7 +130,7 @@ export class UsersService { } async updateRefreshToken(userId: string, refreshToken: string | null): Promise { - await this.userRepository.update(userId, { refreshToken }); + await this.userRepository.update(userId, { refreshToken: refreshToken as unknown as string }); // Invalidate user cache this.eventEmitter.emit(CACHE_EVENTS.USER_UPDATED, { userId }); } @@ -140,8 +141,8 @@ export class UsersService { expires: Date | null, ): Promise { await this.userRepository.update(userId, { - passwordResetToken: token, - passwordResetExpires: expires, + passwordResetToken: token as unknown as string, + passwordResetExpires: expires as unknown as Date, }); } @@ -151,8 +152,8 @@ export class UsersService { expires: Date | null, ): Promise { await this.userRepository.update(userId, { - emailVerificationToken: token, - emailVerificationExpires: expires, + emailVerificationToken: token as unknown as string, + emailVerificationExpires: expires as unknown as Date, }); } @@ -161,10 +162,7 @@ export class UsersService { } async remove(id: string): Promise { - const user = await this.userRepository.findOne({ where: { id } }); - if (!user) { - throw new NotFoundException('User not found'); - } + const user = await this.findUserOrThrow(id); await this.userRepository.remove(user); // Invalidate cache after delete