Merge pull request #220 from sublime247/refactor/eliminate-duplicate-user-code

refactor: eliminate duplicate user lookup patterns

Author: RUKAYAT-CODER

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<LoginResponse> {
     // 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 });

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);
+  }
+}

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);

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<User> {
+    const user = await this.userRepository.findOne({ where: { id } });
+    return ensureUserExists(user, 'User not found');
+  }
+
   async findOne(id: string): Promise<User> {
     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<User> {
-    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<void> {
-    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<void> {
     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<void> {
     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<void> {
-    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