feat: implement notification module

[prudentbird]

Description

This PR introduces a complete notification system focused on user signup approval workflow and enhances test coverage for the newly added components.

Notification Module Implementation

• Added NotificationModule to manage notifications for user signup approval.
• Implemented NotificationService and NotificationProcessor to handle queued notification jobs.
• Added support for both individual and bulk email notifications targeting unverified users.
• Updated AdminController to enable bulk user approval actions.
• Introduced email templates for both bulk and individual approval notification events.
• Improves admin workflow efficiency through automation.

Unit Tests

• Added unit tests for NotificationProcessor, validating processing logic, verification handling, and admin notification flow.
• Added unit tests for NotificationService, confirming queue job creation and error handling.
• Implemented dependency mocks for isolated and reliable testing.

Related Issue

Fixes #66

Type of Change

• feat: New feature
• test: Test additions/updates

How Has This Been Tested?

• Unit tests

Test Evidence

Checklist

• My code follows the project's coding style
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Additional Notes

This enhancement strengthens the signup workflow by automating notifications, improving admin visibility, and ensuring reliable functionality through robust test coverage.

• Update FRONTEND_URL env
• Add NOTIFICATION_INDIVIDUAL_THRESHOLD env
• Add NOTIFICATION_BULK_THRESHOLD env

[claude]

Claude finished @prudentbird's task —— View job

---

📋 Comprehensive Code Review

I've completed a thorough review of the notification module implementation. Here's my detailed analysis:

✅ Positive Aspects

Architecture & Design:

• Well-structured module with clear separation of concerns
• Proper dependency injection and NestJS patterns
• Queue-based processing with BullMQ for reliability
• Comprehensive test coverage with proper mocking

Code Quality:

• Consistent error handling with trySafe pattern
• Good logging throughout the application
• Input validation for UUIDs and required parameters
• Clean TypeScript interfaces and DTOs

⚠️ Security Concerns

1. URL Construction Vulnerability (notification.processor.ts:90, notification.processor.ts:118)

   const approvalUrl = `${frontendUrl}/admin/approve?userId=${user.id}`;
   const approvalUrl = `${frontendUrl}/admin/approve?userIds=${userIds}`;

   • Direct string interpolation could lead to XSS if frontendUrl contains malicious content
   • Recommendation: Use URL constructor and validate/sanitize the frontend URL

2. Email Extraction Logic (notification.processor.ts:91, notification.processor.ts:121)

   const userName = user.email.split('@')[0];

   • No validation that email contains '@' symbol
   • Recommendation: Add email format validation before splitting

🐛 Potential Bugs

1. Race Condition in Bulk Notification Logic (notification.processor.ts:62-72)

   if (count <= 3) {
     await this.sendIndividualNotification(user, admins, frontendUrl);
   } else {
     const countAfterInitialThree = count - 3;
     if (countAfterInitialThree % 5 === 0) {
       // Send bulk notification
     }
   }

   • The logic assumes the "last 5 users" are always the intended recipients
   • Multiple concurrent signups could cause notifications to be sent for wrong users
   • Recommendation: Pass specific user list instead of recalculating

2. Missing Error Handling (notification.processor.ts:106, notification.processor.ts:137)

   await Promise.all(emailPromises);

   • If one email fails, others might succeed but error state is unclear
   • Recommendation: Use Promise.allSettled() and handle partial failures

🚀 Performance Considerations

1. Database Query Inefficiency (user.model-action.ts:17-25)

   return this.repository.find({
     where: {
       status: UserStatus.UNVERIFIED,
       createdAt: MoreThanOrEqual(twentyFourHoursAgo),
     },
     order: { createdAt: 'ASC' },
   });

   • Should add database index on (status, createdAt) for optimal performance
   • Consider limiting result set size to prevent memory issues

2. Admin Query Optimization (user.model-action.ts:28-34)

   • Consider caching admin list since it changes infrequently
   • Add select clause to only fetch required fields (email)

🧪 Test Coverage Assessment

Strengths:

• Good coverage of core functionality
• Proper mocking of dependencies
• Edge case testing (missing user, queue failures)

Gaps:

• Missing integration tests for the complete workflow
• No tests for bulk notification thresholds and edge cases
• Email template rendering not tested

📝 Code Style & Best Practices

1. Magic Numbers (notification.processor.ts:60, notification.processor.ts:64)

   if (count <= 3) {
   if (countAfterInitialThree % 5 === 0) {

   • Recommendation: Extract to configuration constants

2. Type Safety (notification.processor.ts:85-89)

   private async sendIndividualNotification(
     user: { id: string; email: string },
     admins: { email: string }[],

   • Recommendation: Create proper interfaces instead of inline types

🔧 Specific Recommendations

1. Add Configuration Support:

   // Add to config
   NOTIFICATION_INDIVIDUAL_THRESHOLD: 3,
   NOTIFICATION_BULK_THRESHOLD: 5,

2. Enhance Error Handling:

   const results = await Promise.allSettled(emailPromises);
   const failed = results.filter(r => r.status === 'rejected');
   if (failed.length > 0) {
     this.logger.warn(`${failed.length} emails failed to send`);
   }

3. Add URL Validation:

   private validateAndBuildUrl(baseUrl: string, path: string, params: Record<string, string>): string {
     // Proper URL construction with validation
   }

🎯 Overall Assessment

This is a well-implemented feature with good architecture and testing. The main concerns are around security (URL construction), potential race conditions in the bulk notification logic, and some performance optimizations needed for the database queries.

Priority fixes:

1. 🔴 Fix URL construction security issue
2. 🟡 Address race condition in bulk notification logic
3. 🟡 Add database index for performance
4. 🟢 Extract magic numbers to configuration

The feature successfully addresses the requirement for automated user signup notifications with a scalable queue-based approach.

[claude]

Claude finished @prudentbird's task —— View job

---

📋 PR Review: Notification Module Implementation

I've completed a comprehensive review of this notification system implementation. Here's my detailed analysis:

Review Tasks:

• Analyze core notification module structure and architecture
• Review NotificationService and NotificationProcessor implementations
• Examine security considerations and potential vulnerabilities
• Assess performance implications and optimizations
• Evaluate test coverage and quality
• Review email templates and admin controller changes
• Provide final recommendations and feedback

---

✅ Strengths of the Implementation

Architecture & Design

• Clean separation of concerns: Well-structured with distinct service and processor layers
• Queue-based processing: Proper use of BullMQ for reliable background job processing
• NestJS best practices: Correct dependency injection, module structure, and decorators
• Smart notification logic: Intelligent escalation from individual to bulk notifications

Code Quality

• Consistent error handling: Good use of trySafe helper throughout
• Comprehensive logging: Proper logging at all critical points for debugging
• TypeScript interfaces: Well-defined types for job data and service contracts
• Input validation: UUID validation and required parameter checks

Test Coverage

• Excellent coverage: Both service and processor have comprehensive unit tests
• Edge case handling: Tests for missing users, verified users, no admins, etc.
• Proper mocking: Clean isolation of dependencies for reliable testing
• Bulk notification logic: Thorough testing of the 3+5 notification threshold

---

⚠️ Critical Issues & Recommendations

1. Security Vulnerabilities

URL Construction Risk (notification.processor.ts:93, notification.processor.ts:121)

const approvalUrl = `${frontendUrl}/admin/approve?userId=${user.id}`;

• Issue: Direct string interpolation without validation
• Risk: XSS if frontendUrl contains malicious content
• Fix: Use URL constructor and validate frontend URL

Email Parsing Without Validation (notification.processor.ts:94, notification.processor.ts:124)

const userName = user.email.split('@')[0];

• Issue: No validation that email contains '@' symbol
• Risk: Runtime error if email format is invalid
• Fix: Add email format validation before parsing

2. Logic & Reliability Issues

Race Condition in Bulk Logic (notification.processor.ts:64-66)

if (countAfterInitialThree % 5 === 0) {
  const lastFiveUsers = unverifiedUsers.slice(-5);
  await this.sendBulkNotification(lastFiveUsers, admins, frontendUrl);
}

• Issue: Concurrent signups could cause wrong users to be included in bulk notifications
• Fix: Pass specific user list instead of recalculating based on "last 5"

Incomplete Error Handling (notification.processor.ts:109, notification.processor.ts:140)

await Promise.all(emailPromises);

• Issue: If one email fails, unclear which succeeded vs failed
• Fix: Use Promise.allSettled() for partial failure handling

3. Performance Concerns

Database Query Optimization (user.model-action.ts:17-25)

return this.repository.find({
  where: {
    status: UserStatus.UNVERIFIED,
    createdAt: MoreThanOrEqual(twentyFourHoursAgo),
  },
  order: { createdAt: 'ASC' },
});

• Issue: Missing database index on (status, createdAt)
• Impact: Slow queries with large user tables
• Fix: Add compound index and consider result limit

Admin Query Inefficiency (user.model-action.ts:28-34)

• Issue: Fetches full admin records when only emails needed
• Fix: Add .select(['email']) to query only required fields

4. Code Maintainability

Magic Numbers (notification.processor.ts:60, notification.processor.ts:64)

if (count <= 3) {
if (countAfterInitialThree % 5 === 0) {

• Fix: Extract to configuration constants:

NOTIFICATION_INDIVIDUAL_THRESHOLD: 3,
NOTIFICATION_BULK_THRESHOLD: 5,

---

🔧 Specific Fix Recommendations

1. Secure URL Construction

private buildApprovalUrl(frontendUrl: string, params: Record<string, string>): string {
  try {
    const url = new URL('/admin/approve', frontendUrl);
    Object.entries(params).forEach(([key, value]) => {
      url.searchParams.set(key, value);
    });
    return url.toString();
  } catch (error) {
    throw new Error('Invalid frontend URL configuration');
  }
}

2. Enhanced Error Handling

const results = await Promise.allSettled(emailPromises);
const failures = results.filter(r => r.status === 'rejected');
if (failures.length > 0) {
  this.logger.warn(`${failures.length}/${emailPromises.length} emails failed`);
}

3. Email Validation

private extractUserName(email: string): string {
  if (!email.includes('@')) {
    throw new Error('Invalid email format');
  }
  return email.split('@')[0];
}

---

🎯 Priority Ratings

1. 🔴 Critical: Fix URL construction security issue
2. 🟡 High: Address race condition in bulk notification logic
3. 🟡 High: Add database performance optimizations
4. 🟢 Medium: Extract configuration constants
5. 🟢 Low: Enhance error handling granularity

---

💡 Additional Suggestions

1. Configuration Management: Move thresholds to environment variables
2. Monitoring: Add metrics for notification success/failure rates
3. Retry Logic: Consider implementing retry mechanisms for failed emails
4. Template Validation: Validate email templates at startup
5. Admin Caching: Cache admin list since it changes infrequently

---

📊 Overall Assessment

This is a well-architected notification system that successfully implements the user signup approval workflow. The code demonstrates good practices with NestJS, proper testing, and clean separation of concerns. The main concerns are around security (URL construction) and potential race conditions in the bulk notification logic.

Recommendation: ✅ Approve with requested changes - The core functionality is solid, but the security issues should be addressed before merge.

---

[claude]

Claude finished @prudentbird's task —— View job

---

📋 PR Review: Notification Module Implementation

I've completed a comprehensive review of this notification system implementation. Here's my detailed analysis:

Review Tasks:

• Analyze core notification module structure and architecture
• Review NotificationService and NotificationProcessor implementations
• Examine security considerations and potential vulnerabilities
• Assess performance implications and optimizations
• Evaluate test coverage and quality
• Review email templates and admin controller changes
• Provide final recommendations and feedback

---

✅ Strengths of the Implementation

Architecture & Design

• Clean separation of concerns: Well-structured with distinct service and processor layers
• Queue-based processing: Proper use of BullMQ for reliable background job processing
• NestJS best practices: Correct dependency injection, module structure, and decorators
• Smart notification logic: Intelligent escalation from individual to bulk notifications
• Configuration-driven: Thresholds properly moved to environment variables

Code Quality

• Consistent error handling: Excellent use of trySafe helper throughout
• Comprehensive logging: Proper logging at all critical points for debugging
• TypeScript interfaces: Well-defined types for job data and service contracts
• Input validation: UUID validation and required parameter checks
• Enhanced error handling: Uses Promise.allSettled() for partial email failure handling

Test Coverage

• Excellent coverage: Both service and processor have comprehensive unit tests
• Edge case handling: Tests for missing users, verified users, no admins, etc.
• Proper mocking: Clean isolation of dependencies for reliable testing
• Bulk notification logic: Thorough testing of the 3+5 notification threshold logic

---

⚠️ Critical Issues & Recommendations

1. Security Vulnerabilities

URL Construction Risk (notification.processor.ts:122, notification.processor.ts:158)

const approvalUrl = `${this.configService.get<string>('FRONTEND_URL')}/admin/approve?userId=${user.id}`;

• Issue: Direct string interpolation without validation
• Risk: XSS if FRONTEND_URL contains malicious content
• Priority: 🔴 Critical
• Fix: Use URL constructor and validate frontend URL

Email Parsing Without Validation (notification.processor.ts:123, notification.processor.ts:161)

const userName = user.email.split('@')[0];

• Issue: No validation that email contains '@' symbol
• Risk: Runtime error if email format is invalid
• Priority: 🟡 High
• Fix: Add email format validation before parsing

2. Logic & Reliability Issues

Race Condition in Bulk Logic (notification.processor.ts:82-90)

const lastBulkUsers = unverifiedUsers.slice(-this.configService.get<number>('NOTIFICATION_BULK_THRESHOLD')!);
const isCurrentUserInBulk = lastBulkUsers.some((u) => u.id === userId);

• Issue: The processor correctly checks if current user is in the "last N users" group, which is good
• However: Concurrent signups could still cause notifications to include wrong users due to timing
• Priority: 🟡 High
• Fix: Consider passing specific user list in job data instead of recalculating

3. Performance Concerns

Database Query Optimization (user.model-action.ts:17-25)

return this.repository.find({
  where: {
    status: UserStatus.UNVERIFIED,
    createdAt: MoreThanOrEqual(twentyFourHoursAgo),
  },
  order: { createdAt: 'ASC' },
});

• Issue: Missing database index on (status, createdAt)
• Impact: Slow queries with large user tables
• Priority: 🟡 High
• Fix: Add compound index and consider result limit

Admin Query Efficiency (user.model-action.ts:28-34)

return this.repository.find({
  where: {
    role: In([UserRole.ADMIN, UserRole.SUPER_ADMIN]),
  },
});

• Issue: Fetches full admin records when only emails needed
• Priority: 🟢 Medium
• Fix: Add .select(['email']) to query only required fields

4. Email Template Security

Template XSS Prevention: The email templates look well-structured but should validate all template variables:

• {{userName}}, {{userEmail}} should be HTML-escaped
• {{approvalUrl}} should be validated as safe URL

---

🔧 Specific Fix Recommendations

1. Secure URL Construction

private buildApprovalUrl(frontendUrl: string, params: Record<string, string>): string {
  try {
    const url = new URL('/admin/approve', frontendUrl);
    Object.entries(params).forEach(([key, value]) => {
      url.searchParams.set(key, value);
    });
    return url.toString();
  } catch (error) {
    throw new Error('Invalid frontend URL configuration');
  }
}

2. Email Validation

private extractUserName(email: string): string {
  if (!email.includes('@')) {
    throw new Error('Invalid email format');
  }
  return email.split('@')[0];
}

3. Database Index

CREATE INDEX idx_user_status_createdat ON users(status, created_at);

---

💡 Additional Suggestions

1. Admin Caching: Cache admin list since it changes infrequently
2. Monitoring: Add metrics for notification success/failure rates
3. Retry Logic: Consider implementing retry mechanisms for failed emails
4. Template Validation: Validate email templates at startup
5. Rate Limiting: Consider rate limiting notification processing to prevent email spam

---

📊 Overall Assessment

This is a well-architected notification system that successfully implements the user signup approval workflow. The code demonstrates:

✅ Good practices with NestJS patterns
✅ Proper testing and error handling
✅ Clean separation of concerns
✅ Smart escalation logic from individual to bulk notifications
✅ Configuration-driven thresholds

Primary concerns: Security (URL construction) and potential race conditions need addressing.

🎯 Priority Ratings

1. 🔴 Critical: Fix URL construction security issue
2. 🟡 High: Address email validation and potential race conditions
3. 🟡 High: Add database performance optimizations
4. 🟢 Medium: Enhance admin query efficiency
5. 🟢 Low: Add caching and monitoring enhancements

---

Recommendation: ✅ Approve with requested changes - The core functionality is solid and well-tested, but the security issues should be addressed before merge.

---