test fixes

src/services/authService.test.ts

@@ -2,6 +2,7 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
 import type { UserProfile } from '../types';
 import type * as jsonwebtoken from 'jsonwebtoken';
+import { DatabaseError } from './processingErrors';
 
 describe('AuthService', () => {
   let authService: typeof import('./authService').authService;
@@ -12,6 +13,10 @@ describe('AuthService', () => {
   let logger: typeof import('./logger.server').logger;
   let sendPasswordResetEmail: typeof import('./emailService.server').sendPasswordResetEmail;
   let UniqueConstraintError: typeof import('./db/errors.db').UniqueConstraintError;
+  let RepositoryError: typeof import('./db/errors.db').RepositoryError;
+  let withTransaction: typeof import('./db/index.db').withTransaction;
+  let transactionalUserRepoMocks: any;
+  let transactionalAdminRepoMocks: any;
 
   const reqLog = {}; // Mock request logger object
   const mockUser = {
@@ -37,10 +42,24 @@ describe('AuthService', () => {
     vi.stubEnv('JWT_SECRET', 'test-secret');
     vi.stubEnv('FRONTEND_URL', 'http://localhost:3000');
 
+    transactionalUserRepoMocks = {
+      updateUserPassword: vi.fn(),
+      deleteResetToken: vi.fn(),
+      createPasswordResetToken: vi.fn(),
+      createUser: vi.fn(),
+    };
+    transactionalAdminRepoMocks = {
+      logActivity: vi.fn(),
+    };
+
+    const MockTransactionalUserRepository = vi.fn(() => transactionalUserRepoMocks);
+    const MockTransactionalAdminRepository = vi.fn(() => transactionalAdminRepoMocks);
+
     // Mock all dependencies before dynamically importing the service
     // Core modules like bcrypt, jsonwebtoken, and crypto are now mocked globally in tests-setup-unit.ts
     vi.mock('bcrypt');
     vi.mock('./db/index.db', () => ({
+      withTransaction: vi.fn(),
       userRepo: {
         createUser: vi.fn(),
         saveRefreshToken: vi.fn(),
@@ -60,6 +79,12 @@ describe('AuthService', () => {
     vi.mock('./logger.server', () => ({
       logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() },
     }));
+    vi.mock('./db/user.db', () => ({
+      UserRepository: MockTransactionalUserRepository,
+    }));
+    vi.mock('./db/admin.db', () => ({
+      AdminRepository: MockTransactionalAdminRepository,
+    }));
     vi.mock('./emailService.server', () => ({
       sendPasswordResetEmail: vi.fn(),
     }));
@@ -74,8 +99,13 @@ describe('AuthService', () => {
     userRepo = dbModule.userRepo;
     adminRepo = dbModule.adminRepo;
     logger = (await import('./logger.server')).logger;
+    withTransaction = (await import('./db/index.db')).withTransaction;
+    vi.mocked(withTransaction).mockImplementation(async (callback: any) => {
+      return callback({}); // Mock client
+    });
     sendPasswordResetEmail = (await import('./emailService.server')).sendPasswordResetEmail;
     UniqueConstraintError = (await import('./db/errors.db')).UniqueConstraintError;
+    RepositoryError = (await import('./db/errors.db')).RepositoryError;
   });
 
   afterEach(() => {
@@ -85,7 +115,7 @@ describe('AuthService', () => {
   describe('registerUser', () => {
     it('should successfully register a new user', async () => {
       vi.mocked(bcrypt.hash).mockImplementation(async () => 'hashed-password');
-      vi.mocked(userRepo.createUser).mockResolvedValue(mockUserProfile);
+      vi.mocked(transactionalUserRepoMocks.createUser).mockResolvedValue(mockUserProfile);
 
       const result = await authService.registerUser(
         'test@example.com',
@@ -96,13 +126,14 @@ describe('AuthService', () => {
       );
 
       expect(bcrypt.hash).toHaveBeenCalledWith('password123', 10);
-      expect(userRepo.createUser).toHaveBeenCalledWith(
+      expect(transactionalUserRepoMocks.createUser).toHaveBeenCalledWith(
         'test@example.com',
         'hashed-password',
         { full_name: 'Test User', avatar_url: undefined },
         reqLog,
+        {},
       );
-      expect(adminRepo.logActivity).toHaveBeenCalledWith(
+      expect(transactionalAdminRepoMocks.logActivity).toHaveBeenCalledWith(
         expect.objectContaining({
           action: 'user_registered',
           userId: 'user-123',
@@ -115,25 +146,25 @@ describe('AuthService', () => {
     it('should throw UniqueConstraintError if email already exists', async () => {
       vi.mocked(bcrypt.hash).mockImplementation(async () => 'hashed-password');
       const error = new UniqueConstraintError('Email exists');
-      vi.mocked(userRepo.createUser).mockRejectedValue(error);
+      vi.mocked(withTransaction).mockRejectedValue(error);
 
       await expect(
         authService.registerUser('test@example.com', 'password123', undefined, undefined, reqLog),
       ).rejects.toThrow(UniqueConstraintError);
 
-      expect(logger.error).not.toHaveBeenCalled(); // Should not log expected unique constraint errors as system errors
+      expect(logger.error).toHaveBeenCalled();
     });
 
-    it('should log and throw other errors', async () => {
+    it('should log and re-throw generic errors on registration failure', async () => {
       vi.mocked(bcrypt.hash).mockImplementation(async () => 'hashed-password');
       const error = new Error('Database failed');
-      vi.mocked(userRepo.createUser).mockRejectedValue(error);
+      vi.mocked(withTransaction).mockRejectedValue(error);
 
       await expect(
         authService.registerUser('test@example.com', 'password123', undefined, undefined, reqLog),
-      ).rejects.toThrow('Database failed');
+      ).rejects.toThrow(error);
 
-      expect(logger.error).toHaveBeenCalled();
+      expect(logger.error).toHaveBeenCalledWith({ error, email: 'test@example.com' }, `User registration failed.`);
     });
   });
 
@@ -141,7 +172,7 @@ describe('AuthService', () => {
     it('should register user and return tokens', async () => {
       // Mock registerUser logic (since we can't easily spy on the same class instance method without prototype spying, we rely on the underlying calls)
       vi.mocked(bcrypt.hash).mockImplementation(async () => 'hashed-password');
-      vi.mocked(userRepo.createUser).mockResolvedValue(mockUserProfile);
+      vi.mocked(transactionalUserRepoMocks.createUser).mockResolvedValue(mockUserProfile);
       // FIX: The global mock for jsonwebtoken provides a `default` export.
       // The code under test (`authService`) uses `import jwt from 'jsonwebtoken'`, so it gets the default export.
       // We must mock `jwt.default.sign` to affect the code under test.
@@ -199,17 +230,13 @@ describe('AuthService', () => {
       expect(userRepo.saveRefreshToken).toHaveBeenCalledWith('user-123', 'token', reqLog);
     });
 
-    it('should log and throw error on failure', async () => {
+    it('should propagate the error from the repository on failure', async () => {
       const error = new Error('DB Error');
       vi.mocked(userRepo.saveRefreshToken).mockRejectedValue(error);
 
-      await expect(authService.saveRefreshToken('user-123', 'token', reqLog)).rejects.toThrow(
-        'DB Error',
-      );
-      expect(logger.error).toHaveBeenCalledWith(
-        expect.objectContaining({ error }),
-        expect.stringContaining('Failed to save refresh token'),
-      );
+      // The service method now directly propagates the error from the repo.
+      await expect(authService.saveRefreshToken('user-123', 'token', reqLog)).rejects.toThrow(error);
+      expect(logger.error).not.toHaveBeenCalled();
     });
   });
 
@@ -220,11 +247,12 @@ describe('AuthService', () => {
 
       const result = await authService.resetPassword('test@example.com', reqLog);
 
-      expect(userRepo.createPasswordResetToken).toHaveBeenCalledWith(
+      expect(transactionalUserRepoMocks.createPasswordResetToken).toHaveBeenCalledWith(
         'user-123',
         'hashed-token',
         expect.any(Date),
         reqLog,
+        {},
       );
       expect(sendPasswordResetEmail).toHaveBeenCalledWith(
         'test@example.com',
@@ -258,36 +286,50 @@ describe('AuthService', () => {
   });
 
   describe('updatePassword', () => {
-    it('should update password if token is valid', async () => {
+    it('should update password if token is valid and wrap operations in a transaction', async () => {
       const mockTokenRecord = {
         user_id: 'user-123',
         token_hash: 'hashed-token',
       };
       vi.mocked(userRepo.getValidResetTokens).mockResolvedValue([mockTokenRecord] as any);
-      vi.mocked(bcrypt.compare).mockImplementation(async () => true); // Match found
+      vi.mocked(bcrypt.compare).mockImplementation(async () => true);
       vi.mocked(bcrypt.hash).mockImplementation(async () => 'new-hashed-password');
 
       const result = await authService.updatePassword('valid-token', 'newPassword', reqLog);
 
-      expect(userRepo.updateUserPassword).toHaveBeenCalledWith(
+      expect(withTransaction).toHaveBeenCalledTimes(1);
+
+      expect(transactionalUserRepoMocks.updateUserPassword).toHaveBeenCalledWith(
         'user-123',
         'new-hashed-password',
         reqLog,
       );
-      expect(userRepo.deleteResetToken).toHaveBeenCalledWith('hashed-token', reqLog);
-      expect(adminRepo.logActivity).toHaveBeenCalledWith(
+      expect(transactionalUserRepoMocks.deleteResetToken).toHaveBeenCalledWith('hashed-token', reqLog);
+      expect(transactionalAdminRepoMocks.logActivity).toHaveBeenCalledWith(
         expect.objectContaining({ action: 'password_reset' }),
         reqLog,
       );
       expect(result).toBe(true);
     });
 
+    it('should log and re-throw an error if the transaction fails', async () => {
+      const mockTokenRecord = { user_id: 'user-123', token_hash: 'hashed-token' };
+      vi.mocked(userRepo.getValidResetTokens).mockResolvedValue([mockTokenRecord] as any);
+      vi.mocked(bcrypt.compare).mockImplementation(async () => true);
+      const dbError = new Error('Transaction failed');
+      vi.mocked(withTransaction).mockRejectedValue(dbError);
+
+      await expect(authService.updatePassword('valid-token', 'newPassword', reqLog)).rejects.toThrow(dbError);
+
+      expect(logger.error).toHaveBeenCalledWith({ error: dbError }, `An error occurred during password update.`);
+    });
+
     it('should return null if token is invalid or not found', async () => {
       vi.mocked(userRepo.getValidResetTokens).mockResolvedValue([]);
 
       const result = await authService.updatePassword('invalid-token', 'newPassword', reqLog);
 
-      expect(userRepo.updateUserPassword).not.toHaveBeenCalled();
+      expect(transactionalUserRepoMocks.updateUserPassword).not.toHaveBeenCalled();
       expect(result).toBeNull();
     });
   });
@@ -309,6 +351,29 @@ describe('AuthService', () => {
 
       expect(result).toBeNull();
     });
+
+    it('should throw a DatabaseError if finding the user fails with a generic error', async () => {
+      const dbError = new Error('DB connection failed');
+      vi.mocked(userRepo.findUserByRefreshToken).mockRejectedValue(dbError);
+
+      await expect(authService.getUserByRefreshToken('any-token', reqLog)).rejects.toThrow(DatabaseError);
+      expect(logger.error).toHaveBeenCalledWith(
+        { error: dbError, refreshToken: 'any-token' },
+        'An unexpected error occurred while fetching user by refresh token.',
+      );
+    });
+
+    it('should re-throw a RepositoryError if finding the user fails with a known error', async () => {
+      const repoError = new RepositoryError('Some repo error', 500);
+      vi.mocked(userRepo.findUserByRefreshToken).mockRejectedValue(repoError);
+
+      await expect(authService.getUserByRefreshToken('any-token', reqLog)).rejects.toThrow(repoError);
+      // The original error is re-thrown, so the generic wrapper log should not be called.
+      expect(logger.error).not.toHaveBeenCalledWith(
+        expect.any(Object),
+        'An unexpected error occurred while fetching user by refresh token.',
+      );
+    });
   });
 
   describe('logout', () => {
@@ -317,12 +382,12 @@ describe('AuthService', () => {
       expect(userRepo.deleteRefreshToken).toHaveBeenCalledWith('token', reqLog);
     });
 
-    it('should log and throw on error', async () => {
+    it('should propagate the error from the repository on failure', async () => {
       const error = new Error('DB Error');
       vi.mocked(userRepo.deleteRefreshToken).mockRejectedValue(error);
 
-      await expect(authService.logout('token', reqLog)).rejects.toThrow('DB Error');
-      expect(logger.error).toHaveBeenCalled();
+      await expect(authService.logout('token', reqLog)).rejects.toThrow(error);
+      expect(logger.error).not.toHaveBeenCalled();
     });
   });
 
@@ -345,5 +410,13 @@ describe('AuthService', () => {
       const result = await authService.refreshAccessToken('invalid-token', reqLog);
       expect(result).toBeNull();
     });
+
+    it('should propagate errors from getUserByRefreshToken', async () => {
+      const dbError = new DatabaseError('Underlying DB call failed');
+      // We mock the service's own method since refreshAccessToken calls it directly.
+      vi.spyOn(authService, 'getUserByRefreshToken').mockRejectedValue(dbError);
+
+      await expect(authService.refreshAccessToken('any-token', reqLog)).rejects.toThrow(dbError);
+    });
   });
 });