Merge pull request #1463 from bobaivigitalpoint-ui/bobaivigitalpoint-ui/userapi-1347-1350

test(users): UsersService unit tests and controller integration tests; structured logging and error handling

Author: nanaf6203-bit

docs/api/users-error-handling.md

@@ -0,0 +1,3 @@
+# Users API error handling
+
+`GET /users/:id` returned `200` with an empty body when the id was unknown or the account was deactivated, so clients could not tell "no such user" from success. It now responds `404 User not found`, matching the other lookups (`me/profile`, statistics). Authorization is unchanged: the route is still admin-only, so the response does not widen what a caller can learn.

docs/api/users-structured-logging.md

@@ -0,0 +1,17 @@
+# Users API structured logging
+
+`UsersService` now uses the project's `AppLogger`. Events are logged as a short constant message plus a metadata object with a stable `event` name, so the JSON logs produced in production can be filtered and alerted on:
+
+| `event` | Level | Fields |
+| --- | --- | --- |
+| `users.created` | log | `userId`, `emailHash` |
+| `users.blocked` | warn | `userId` |
+| `users.unblocked` | log | `userId` |
+| `users.deleted` | warn | `userId` |
+| `users.deactivated` | log | `userId`, `emailHash`, `scheduledDeletionAt` |
+| `users.reactivation_token_generated` | log | `userId`, `emailHash` |
+| `users.reactivated` | log | `userId`, `emailHash` |
+| `users.deactivated_purged` | log | `deletedCount` |
+| `users.export_file_deleted` | log | `file` |
+
+Emails appear only as a short hash. The reactivation-token log line previously contained the raw email address; that is fixed. `AppLogger` also redacts fields such as `password`, `token` and `secret` before writing.

docs/api/users-tests.md

@@ -0,0 +1,6 @@
+# Users API tests
+
+- **Unit** - `src/users/users.service.spec.ts` covers `UsersService` with a mocked Prisma client: account creation (hashed password, secrets stripped, history/preferences seeded, referral-code collision retry, referrer linking), active-only lookups, block/unblock, verify/unverify, removal, referral stats and login history.
+- **Integration** - `test/e2e/users-controller.integration.spec.ts` boots `UsersController` over HTTP with the same `ValidationPipe` settings as `main.ts`, mocked services and role-aware guards: authentication and admin-role enforcement, `GET /users/:id` including the 404 case, body validation for create and update (invalid values, unknown fields), search query coercion and rejection, and the self-service `me/*` routes.
+
+Run with `npx jest src/users/users.service.spec.ts test/e2e/users-controller.integration.spec.ts`.

src/users/users.controller.ts

@@ -179,8 +179,11 @@ export class UsersController {
   @ApiResponse({ status: 403, description: 'Admin role required' })
   @ApiResponse({ status: 404, description: 'User not found' })
   @Get(':id')
-  findOne(@Param('id') id: string) {
-    return this.usersService.findOne(id);
+  async findOne(@Param('id') id: string) {
+    const user = await this.usersService.findOne(id);
+    // Previously an unknown or deactivated id produced 200 with an empty body.
+    if (!user) throw new NotFoundException('User not found');
+    return user;
   }
 
   @UseGuards(JwtAuthGuard, RolesGuard)

src/users/users.service.spec.ts

@@ -0,0 +1,217 @@
+import { PrismaService } from '../database/prisma.service';
+import { SessionsService } from '../sessions/sessions.service';
+import { AppLogger } from '../common/logger';
+import { UsersService } from './users.service';
+
+jest.mock('../auth/security.utils', () => ({
+  ...jest.requireActual('../auth/security.utils'),
+  // bcrypt at 12 rounds is slow and irrelevant to what is being tested.
+  hashPassword: jest.fn(async (password: string) => `hashed:${password}`),
+}));
+
+type Mock = jest.Mock;
+
+function makePrisma() {
+  return {
+    user: {
+      findUnique: jest.fn(),
+      findFirst: jest.fn(),
+      findMany: jest.fn(),
+      create: jest.fn(),
+      update: jest.fn(),
+      delete: jest.fn(),
+      count: jest.fn(),
+    },
+    loginHistory: { findMany: jest.fn() },
+  } as unknown as { user: Record<string, Mock>; loginHistory: Record<string, Mock> };
+}
+
+describe('UsersService', () => {
+  let prisma: ReturnType<typeof makePrisma>;
+  let service: UsersService;
+
+  beforeEach(() => {
+    jest.spyOn(AppLogger.prototype, 'log').mockImplementation(() => undefined);
+    jest.spyOn(AppLogger.prototype, 'warn').mockImplementation(() => undefined);
+    prisma = makePrisma();
+    service = new UsersService(
+      prisma as unknown as PrismaService,
+      { revokeAllSessions: jest.fn() } as unknown as SessionsService,
+    );
+  });
+
+  afterEach(() => jest.restoreAllMocks());
+
+  describe('create', () => {
+    const dto = {
+      email: 'new@example.com',
+      password: 'Str0ng!Pass',
+      firstName: 'Ada',
+      lastName: 'Lovelace',
+    };
+
+    beforeEach(() => {
+      prisma.user.findUnique.mockResolvedValue(null); // referral code is free, no referrer
+      prisma.user.create.mockImplementation(async ({ data }: { data: Record<string, unknown> }) => ({
+        id: 'u1',
+        ...data,
+        twoFactorSecret: 'secret',
+      }));
+    });
+
+    it('stores a hash, never the plain password, and strips secrets from the result', async () => {
+      const result = await service.create(dto);
+
+      const data = prisma.user.create.mock.calls[0][0].data;
+      expect(data.password).toBe('hashed:Str0ng!Pass');
+      expect(result).not.toHaveProperty('password');
+      expect(result).not.toHaveProperty('twoFactorSecret');
+      expect(result).toMatchObject({ id: 'u1', email: 'new@example.com' });
+    });
+
+    it('seeds password history and default notification preferences', async () => {
+      await service.create(dto);
+
+      const data = prisma.user.create.mock.calls[0][0].data;
+      expect(data.passwordHistory).toEqual({ create: { passwordHash: 'hashed:Str0ng!Pass' } });
+      expect(data.preferences.create).toMatchObject({
+        emailNotifications: true,
+        inAppNotifications: true,
+        smsNotifications: false,
+      });
+    });
+
+    it('generates a 6-character upper-case referral code', async () => {
+      await service.create(dto);
+      expect(prisma.user.create.mock.calls[0][0].data.referralCode).toMatch(/^[0-9A-Z]{1,6}$/);
+    });
+
+    it('retries until the generated referral code is unused', async () => {
+      prisma.user.findUnique
+        .mockResolvedValueOnce({ id: 'taken' }) // first code collides
+        .mockResolvedValueOnce(null); // second is free
+
+      await service.create(dto);
+
+      expect(prisma.user.findUnique).toHaveBeenCalledTimes(2);
+    });
+
+    it('links the new user to the referrer when a valid referral code is given', async () => {
+      prisma.user.findUnique
+        .mockResolvedValueOnce(null) // generated code is free
+        .mockResolvedValueOnce({ id: 'referrer-1' }); // supplied code resolves
+
+      await service.create({ ...dto, referralCode: 'FRIEND' });
+
+      expect(prisma.user.findUnique).toHaveBeenLastCalledWith({ where: { referralCode: 'FRIEND' } });
+      expect(prisma.user.create.mock.calls[0][0].data.referredById).toBe('referrer-1');
+    });
+
+    it('ignores an unknown referral code', async () => {
+      await service.create({ ...dto, referralCode: 'NOPE' });
+      expect(prisma.user.create.mock.calls[0][0].data.referredById).toBeNull();
+    });
+  });
+
+  describe('lookups', () => {
+    it('findOne only returns active users and selects public fields', async () => {
+      prisma.user.findFirst.mockResolvedValue({ id: 'u1' });
+
+      await expect(service.findOne('u1')).resolves.toEqual({ id: 'u1' });
+
+      const args = prisma.user.findFirst.mock.calls[0][0];
+      expect(args.where).toEqual({ id: 'u1', isDeactivated: false });
+      expect(args.select).not.toHaveProperty('password');
+      expect(args.select).toMatchObject({ email: true, role: true });
+    });
+
+    it('findOne returns null for an unknown or deactivated user', async () => {
+      prisma.user.findFirst.mockResolvedValue(null);
+      await expect(service.findOne('missing')).resolves.toBeNull();
+    });
+
+    it('findByEmail looks up by exact email', async () => {
+      prisma.user.findUnique.mockResolvedValue({ id: 'u1' });
+      await service.findByEmail('a@example.com');
+      expect(prisma.user.findUnique).toHaveBeenCalledWith({ where: { email: 'a@example.com' } });
+    });
+  });
+
+  describe('admin state changes', () => {
+    beforeEach(() => {
+      prisma.user.update.mockImplementation(async ({ data }: { data: Record<string, unknown> }) => data);
+    });
+
+    it('block / unblock toggle isBlocked', async () => {
+      await service.block('u1');
+      await service.unblock('u1');
+
+      expect(prisma.user.update.mock.calls[0][0]).toMatchObject({
+        where: { id: 'u1' },
+        data: { isBlocked: true },
+      });
+      expect(prisma.user.update.mock.calls[1][0]).toMatchObject({ data: { isBlocked: false } });
+    });
+
+    it('verify / unverify toggle isVerified and select only id and isVerified', async () => {
+      await service.verify('u1');
+      await service.unverify('u1');
+
+      expect(prisma.user.update.mock.calls[0][0]).toMatchObject({
+        data: { isVerified: true },
+        select: { id: true, isVerified: true },
+      });
+      expect(prisma.user.update.mock.calls[1][0]).toMatchObject({ data: { isVerified: false } });
+    });
+
+    it('remove deletes the user', async () => {
+      prisma.user.delete.mockResolvedValue({ id: 'u1' });
+      await expect(service.remove('u1')).resolves.toEqual({ id: 'u1' });
+      expect(prisma.user.delete).toHaveBeenCalledWith({ where: { id: 'u1' } });
+    });
+  });
+
+  describe('referrals', () => {
+    it('getReferralStats combines the referral code with the referral count', async () => {
+      prisma.user.count.mockResolvedValue(3);
+      prisma.user.findUnique.mockResolvedValue({ referralCode: 'ABC123' });
+
+      await expect(service.getReferralStats('u1')).resolves.toEqual({
+        referralCode: 'ABC123',
+        referralCount: 3,
+      });
+      expect(prisma.user.count).toHaveBeenCalledWith({ where: { referredById: 'u1' } });
+    });
+
+    it('getReferralStats tolerates a missing user', async () => {
+      prisma.user.count.mockResolvedValue(0);
+      prisma.user.findUnique.mockResolvedValue(null);
+      await expect(service.getReferralStats('ghost')).resolves.toEqual({
+        referralCode: undefined,
+        referralCount: 0,
+      });
+    });
+
+    it('getMyReferrals selects only non-sensitive fields', async () => {
+      prisma.user.findMany.mockResolvedValue([]);
+      await service.getMyReferrals('u1');
+
+      const args = prisma.user.findMany.mock.calls[0][0];
+      expect(args.where).toEqual({ referredById: 'u1' });
+      expect(Object.keys(args.select).sort()).toEqual(['createdAt', 'email', 'firstName', 'id', 'lastName']);
+    });
+  });
+
+  describe('getLoginHistory', () => {
+    it('returns the caller\'s logins newest first without extra fields', async () => {
+      prisma.loginHistory.findMany.mockResolvedValue([{ timestamp: new Date(), ipAddress: '1.1.1.1' }]);
+
+      await service.getLoginHistory('u1');
+
+      const args = prisma.loginHistory.findMany.mock.calls[0][0];
+      expect(args.where).toEqual({ userId: 'u1' });
+      expect(args.orderBy).toEqual({ timestamp: 'desc' });
+      expect(args.select).toEqual({ timestamp: true, ipAddress: true, userAgent: true });
+    });
+  });
+});

src/users/users.service.ts

@@ -1,6 +1,5 @@
 import {
   Injectable,
-  Logger,
   OnModuleInit,
   Optional,
   NotFoundException,
@@ -29,7 +28,7 @@ import { userRegistrationsTotal } from '../metrics/metrics.controller';
 
 @Injectable()
 export class UsersService implements OnModuleInit {
-  private readonly logger = new Logger(UsersService.name);
+  private readonly logger = new AppLogger(UsersService.name);
 
   constructor(
     private readonly prisma: PrismaService,
@@ -207,7 +206,7 @@ export class UsersService implements OnModuleInit {
 
       if (now - stats.mtimeMs > expirationTime) {
         fs.unlinkSync(filepath);
-        this.logger.log(`Deleted expired export file: ${file}`);
+        this.logger.log('Deleted expired export file', { event: 'users.export_file_deleted', file });
       }
     });
   }
@@ -307,6 +306,7 @@ export class UsersService implements OnModuleInit {
     });
 
     userRegistrationsTotal.inc({ method: 'email' });
+    this.logger.log('User created', { event: 'users.created', userId: user.id, emailHash: redactEmail(user.email) });
 
     return sanitizeUser(user);
   }
@@ -433,7 +433,7 @@ export class UsersService implements OnModuleInit {
   }
 
   async block(id: string) {
-    return this.prisma.user.update({
+    const blocked = await this.prisma.user.update({
       where: { id },
       data: { isBlocked: true },
       select: {
@@ -442,10 +442,12 @@ export class UsersService implements OnModuleInit {
         isBlocked: true,
       },
     });
+    this.logger.warn('User blocked', { event: 'users.blocked', userId: id });
+    return blocked;
   }
 
   async unblock(id: string) {
-    return this.prisma.user.update({
+    const unblocked = await this.prisma.user.update({
       where: { id },
       data: { isBlocked: false },
       select: {
@@ -454,12 +456,16 @@ export class UsersService implements OnModuleInit {
         isBlocked: true,
       },
     });
+    this.logger.log('User unblocked', { event: 'users.unblocked', userId: id });
+    return unblocked;
   }
 
   async remove(id: string) {
-    return this.prisma.user.delete({
+    const removed = await this.prisma.user.delete({
       where: { id },
     });
+    this.logger.warn('User deleted', { event: 'users.deleted', userId: id });
+    return removed;
   }
 
   async exportPersonalData(id: string) {
@@ -562,11 +568,12 @@ export class UsersService implements OnModuleInit {
       },
     });
 
-    this.logger.log(
-      `User ${userId} (${redactEmail(user.email)}) deactivated. Scheduled deletion: ${
-        scheduledDeletionAt ? scheduledDeletionAt.toISOString() : 'None'
-      }`,
-    );
+    this.logger.log('User deactivated', {
+      event: 'users.deactivated',
+      userId,
+      emailHash: redactEmail(user.email),
+      scheduledDeletionAt: scheduledDeletionAt ? scheduledDeletionAt.toISOString() : null,
+    });
 
     await this.sessionsService.revokeAllSessions(userId);
 
@@ -597,7 +604,11 @@ export class UsersService implements OnModuleInit {
       },
     });
 
-    this.logger.log(`Reactivation token generated for user ${userId} (${user.email})`);
+    this.logger.log('Reactivation token generated', {
+      event: 'users.reactivation_token_generated',
+      userId,
+      emailHash: redactEmail(user.email),
+    });
 
     return {
       message: 'Reactivation token generated. Use the token to reactivate your account.',
@@ -677,7 +688,11 @@ export class UsersService implements OnModuleInit {
       },
     });
 
-    this.logger.log(`User ${userId} (${redactEmail(user.email)}) reactivated`);
+    this.logger.log('User reactivated', {
+      event: 'users.reactivated',
+      userId,
+      emailHash: redactEmail(user.email),
+    });
 
     return updatedUser;
   }
@@ -833,7 +848,10 @@ export class UsersService implements OnModuleInit {
       });
     });
 
-    this.logger.log(`Deleted ${result.count} deactivated users`);
+    this.logger.log('Deleted deactivated users', {
+      event: 'users.deactivated_purged',
+      deletedCount: result.count,
+    });
 
     return { deletedCount: result.count };
   }