Assist_Design/docs/development/refactoring/code-quality-improvement-plan.md

Code Quality Improvement Plan

Created: December 2025
Status: Planning
Estimated Effort: 3-4 sprints (2-week sprints)

---

Executive Summary

This document outlines a comprehensive plan to address code bloat, redundancies, and maintainability issues identified in the Customer Portal codebase. The improvements are organized into three phases based on effort and impact.

---

Phase 1: Quick Wins (Sprint 1)

Low effort, high impact changes that can be completed quickly with minimal risk.

1.1 Add getWhmcsClientIdOrThrow() to MappingsService

Problem: The same WHMCS mapping lookup pattern is duplicated in 16+ files:

const mapping = await this.mappingsService.findByUserId(userId);
if (!mapping?.whmcsClientId) {
  throw new NotFoundException("WHMCS client mapping not found");
}

Solution: Add a convenience method to MappingsService:

// File: apps/bff/src/modules/id-mappings/mappings.service.ts

/**
 * Get WHMCS client ID for a user, throwing if not found.
 * Use this when WHMCS client ID is required for an operation.
 */
async getWhmcsClientIdOrThrow(userId: string): Promise<number> {
  const mapping = await this.findByUserId(userId);
  if (!mapping?.whmcsClientId) {
    throw new NotFoundException("WHMCS client mapping not found");
  }
  return mapping.whmcsClientId;
}

/**
 * Get full mapping for a user, throwing if not found.
 * Use when you need both whmcsClientId and sfAccountId.
 */
async getMappingOrThrow(userId: string): Promise<UserIdMapping> {
  const mapping = await this.findByUserId(userId);
  if (!mapping) {
    throw new NotFoundException("User mapping not found");
  }
  return mapping;
}

Files to Update:

• apps/bff/src/modules/id-mappings/mappings.service.ts - Add methods
• apps/bff/src/modules/id-mappings/index.ts - Export if needed

Testing: Existing tests should pass; add unit tests for new methods.

---

1.2 Refactor All WHMCS Mapping Lookups

Files to Refactor (16 files):

File	Occurrences	Change
billing.controller.ts	4	Use getWhmcsClientIdOrThrow()
invoice-retrieval.service.ts	1	Use getWhmcsClientIdOrThrow(), remove private getUserMapping()
user-profile.service.ts	2	Use getWhmcsClientIdOrThrow()
subscriptions.service.ts	3	Use getWhmcsClientIdOrThrow()
esim-management.service.ts	1	Use getWhmcsClientIdOrThrow()
sim-cancellation.service.ts	2	Use getWhmcsClientIdOrThrow()
sim-topup.service.ts	1	Use getWhmcsClientIdOrThrow()
sim-order-activation.service.ts	1	Use getWhmcsClientIdOrThrow()
auth.facade.ts	2	Use getWhmcsClientIdOrThrow()

Before:

const mapping = await this.mappingsService.findByUserId(req.user.id);
if (!mapping?.whmcsClientId) {
  throw new NotFoundException("WHMCS client mapping not found");
}
return this.whmcsService.getPaymentMethods(mapping.whmcsClientId, req.user.id);

After:

const whmcsClientId = await this.mappingsService.getWhmcsClientIdOrThrow(req.user.id);
return this.whmcsService.getPaymentMethods(whmcsClientId, req.user.id);

---

1.3 Rename Conflicting getErrorMessage Functions

Problem: Two functions with the same name serve different purposes:

• @bff/core/utils/error.util.ts - Extracts message from unknown error
• @customer-portal/domain/common/errors.ts - Gets message for ErrorCode

Solution: Rename for clarity:

Location	Current Name	New Name
apps/bff/src/core/utils/error.util.ts	getErrorMessage	extractErrorMessage
packages/domain/common/errors.ts	getErrorMessage	getMessageForErrorCode

Files to Update:

• apps/bff/src/core/utils/error.util.ts - Rename function
• packages/domain/common/errors.ts - Rename function
• packages/domain/common/index.ts - Update export
• All 73 files importing from error.util.ts - Update import
• All files importing from domain errors - Update import
• apps/portal/src/lib/utils/error-handling.ts - Update usage

Migration Strategy:

1. Add new function names as aliases first
2. Update all imports
3. Remove old function names

---

1.4 Remove Unused Status Wrapper Methods

Problem: Multiple thin wrapper methods that just delegate to a generic method.

Files to Audit:

invoice-retrieval.service.ts - Check if these are called:

• getUnpaidInvoices() → calls getInvoicesByStatus(userId, "Unpaid")
• getOverdueInvoices() → calls getInvoicesByStatus(userId, "Overdue")
• getPaidInvoices() → calls getInvoicesByStatus(userId, "Paid")
• getCancelledInvoices() → calls getInvoicesByStatus(userId, "Cancelled")
• getCollectionsInvoices() → calls getInvoicesByStatus(userId, "Collections")

subscriptions.service.ts - Check if these are called:

• getSuspendedSubscriptions() → calls getSubscriptionsByStatus(userId, "Suspended")
• getCancelledSubscriptions() → calls getSubscriptionsByStatus(userId, "Cancelled")
• getPendingSubscriptions() → calls getSubscriptionsByStatus(userId, "Pending")

Action:

1. Search codebase for usages of each method
2. If unused, remove the method
3. If used in 1-2 places, consider inlining

---

Phase 2: Medium-Term Improvements (Sprint 2)

Moderate effort changes that improve code consistency and reduce duplication.

2.1 Create Centralized Error Handling Utility

Problem: Repeated try-catch patterns with logging and exception re-throwing (141 occurrences).

Solution: Create a utility function and decorator:

File: apps/bff/src/core/utils/error-handler.util.ts

import { Logger } from "nestjs-pino";
import {
  NotFoundException,
  BadRequestException,
  InternalServerErrorException,
} from "@nestjs/common";
import { extractErrorMessage } from "./error.util.js";

type RethrowableException = typeof NotFoundException | typeof BadRequestException;

interface ErrorHandlerOptions {
  context: string;
  rethrow?: RethrowableException[];
  fallbackMessage?: string;
}

/**
 * Wraps an async operation with consistent error handling.
 *
 * @example
 * return withErrorHandling(
 *   () => this.whmcsService.getInvoices(clientId, userId),
 *   this.logger,
 *   { context: `Get invoices for user ${userId}`, rethrow: [NotFoundException] }
 * );
 */
export async function withErrorHandling<T>(
  operation: () => Promise<T>,
  logger: Logger,
  options: ErrorHandlerOptions
): Promise<T> {
  const { context, rethrow = [NotFoundException, BadRequestException], fallbackMessage } = options;

  try {
    return await operation();
  } catch (error) {
    logger.error(context, { error: extractErrorMessage(error) });

    for (const ExceptionType of rethrow) {
      if (error instanceof ExceptionType) {
        throw error;
      }
    }

    throw new InternalServerErrorException(fallbackMessage ?? `${context} failed`);
  }
}

/**
 * Safe wrapper that returns null instead of throwing.
 * Useful for optional operations.
 */
export async function withErrorSuppression<T>(
  operation: () => Promise<T>,
  logger: Logger,
  context: string
): Promise<T | null> {
  try {
    return await operation();
  } catch (error) {
    logger.warn(`${context} (suppressed)`, { error: extractErrorMessage(error) });
    return null;
  }
}

Export from: apps/bff/src/core/utils/index.ts

---

2.2 Refactor Services to Use Error Handling Utility

Priority Services to Refactor:

Service	Error Blocks	Priority
invoice-retrieval.service.ts	5	High
subscriptions.service.ts	12	High
user-profile.service.ts	6	High
signup-workflow.service.ts	8	Medium
password-workflow.service.ts	4	Medium
whmcs-link-workflow.service.ts	5	Medium

Before:

async getInvoices(userId: string, options: InvoiceListQuery = {}): Promise<InvoiceList> {
  try {
    const mapping = await this.getUserMapping(userId);
    const invoiceList = await this.whmcsService.getInvoices(mapping.whmcsClientId, userId, options);
    return invoiceList;
  } catch (error) {
    this.logger.error(`Failed to get invoices for user ${userId}`, {
      error: getErrorMessage(error),
      options,
    });
    if (error instanceof NotFoundException) {
      throw error;
    }
    throw new InternalServerErrorException("Failed to retrieve invoices");
  }
}

After:

async getInvoices(userId: string, options: InvoiceListQuery = {}): Promise<InvoiceList> {
  const whmcsClientId = await this.mappingsService.getWhmcsClientIdOrThrow(userId);

  return withErrorHandling(
    () => this.whmcsService.getInvoices(whmcsClientId, userId, options),
    this.logger,
    { context: `Get invoices for user ${userId}`, fallbackMessage: "Failed to retrieve invoices" }
  );
}

---

2.3 Consolidate WHMCS Connection Methods

Problem: makeRequest and makeHighPriorityRequest have nearly identical code.

File: apps/bff/src/integrations/whmcs/connection/services/whmcs-connection-orchestrator.service.ts

Solution: Merge into single method with options:

interface WhmcsRequestOptions {
  timeout?: number;
  retryAttempts?: number;
  retryDelay?: number;
  highPriority?: boolean;  // NEW: consolidates makeHighPriorityRequest
}

async makeRequest<T>(
  action: string,
  params: Record<string, unknown> = {},
  options: WhmcsRequestOptions = {}
): Promise<T> {
  const executeRequest = async () => {
    try {
      const config = this.configService.getConfig();
      const response = await this.httpClient.makeRequest<T>(config, action, params, options);

      if (response.result === "error") {
        const errorResponse = response as WhmcsErrorResponse;
        this.errorHandler.handleApiError(errorResponse, action, params);
      }

      return response.data as T;
    } catch (error) {
      if (this.isHandledException(error)) {
        throw error;
      }
      this.errorHandler.handleRequestError(error);
    }
  };

  if (options.highPriority) {
    return this.requestQueue.executeHighPriority(executeRequest);
  }

  return this.requestQueue.execute(executeRequest, {
    priority: this.getRequestPriority(action),
    timeout: options.timeout,
    retryAttempts: options.retryAttempts,
    retryDelay: options.retryDelay,
  });
}

// DEPRECATED: Use makeRequest with { highPriority: true }
// Keep for backward compatibility, remove in next major version
async makeHighPriorityRequest<T>(
  action: string,
  params: Record<string, unknown> = {},
  options: WhmcsRequestOptions = {}
): Promise<T> {
  return this.makeRequest(action, params, { ...options, highPriority: true });
}

---

Phase 3: Long-Term Improvements (Sprints 3-4)

Higher effort changes that significantly improve architecture and maintainability.

3.1 Split SignupWorkflowService

Current State: 703 lines, multiple responsibilities

Proposed Structure:

apps/bff/src/modules/auth/infra/workflows/
├── signup/
│   ├── signup-orchestrator.service.ts    # Main workflow coordination
│   ├── signup-validation.service.ts      # Validation logic
│   ├── signup-account-resolver.service.ts # SF account lookup & caching
│   └── signup.types.ts                   # Shared types
├── password-workflow.service.ts
└── whmcs-link-workflow.service.ts

SignupOrchestratorService (orchestrator only):

@Injectable()
export class SignupOrchestratorService {
  constructor(
    private readonly validation: SignupValidationService,
    private readonly accountResolver: SignupAccountResolverService,
    private readonly usersFacade: UsersFacade,
    private readonly whmcsService: WhmcsService,
    private readonly tokenService: AuthTokenService
    // ...
  ) {}

  async signup(signupData: SignupRequest, request?: Request): Promise<AuthResultInternal> {
    // 1. Rate limit
    await this.rateLimit(request);

    // 2. Validate
    await this.validation.validateSignupData(signupData);

    // 3. Resolve account
    const account = await this.accountResolver.resolveOrCreate(signupData);

    // 4. Create WHMCS client
    const whmcsClient = await this.createWhmcsClient(signupData, account);

    // 5. Create portal user
    const user = await this.createPortalUser(signupData, account, whmcsClient);

    // 6. Generate tokens
    return this.generateAuthResult(user);
  }
}

SignupValidationService:

@Injectable()
export class SignupValidationService {
  async validateSignupData(data: SignupRequest): Promise<void> { ... }
  async validateExistingUser(email: string): Promise<void> { ... }
  async validateSfAccount(sfNumber: string): Promise<void> { ... }
}

SignupAccountResolverService:

@Injectable()
export class SignupAccountResolverService {
  private readonly cacheTtl = 30;
  private readonly cachePrefix = "auth:signup:account:";

  async resolveOrCreate(signupData: SignupRequest): Promise<SignupAccountSnapshot> { ... }
  async getAccountSnapshot(sfNumber: string): Promise<SignupAccountSnapshot | null> { ... }
  private normalizeCustomerNumber(sfNumber?: string): string | null { ... }
}

---

3.2 Split UserProfileService

Current State: 482 lines, mixes profile and dashboard concerns

Proposed Structure:

apps/bff/src/modules/users/
├── application/
│   └── users.facade.ts
├── infra/
│   ├── user-auth.repository.ts
│   ├── user-profile.service.ts     # Profile operations only
│   └── dashboard.service.ts        # NEW: Dashboard summary
└── utils/
    └── whmcs-custom-fields.util.ts # NEW: Custom field extraction

DashboardService:

@Injectable()
export class DashboardService {
  async getUserSummary(userId: string): Promise<DashboardSummary> { ... }
  private async buildStats(mapping: UserIdMapping): Promise<DashboardStats> { ... }
  private async buildNextInvoice(invoices: Invoice[]): Promise<NextInvoice | null> { ... }
  private async buildRecentActivity(...): Promise<Activity[]> { ... }
}

WhmcsCustomFieldsUtil:

export function extractCustomerNumber(customfields: unknown, fieldId: string): string | null { ... }
export function extractDateOfBirth(customfields: unknown, fieldId: string): string | null { ... }
export function extractGender(customfields: unknown, fieldId: string): string | null { ... }

---

3.3 Evaluate WhmcsService Facade

Current State: 340 lines of pure delegation methods

Options:

Option A: Remove Facade (Recommended)

• Inject specialized services directly (WhmcsInvoiceService, WhmcsSubscriptionService, etc.)
• Reduces indirection
• Clearer dependencies

Option B: Keep Facade, Add Value

• Add cross-cutting concerns (metrics, circuit breaker, retry policies)
• Make facade worth the abstraction cost

Decision Criteria:

• If no cross-cutting concerns needed → Option A
• If circuit breaker/metrics needed → Option B

Migration (Option A):

1. Create deprecation warnings on WhmcsService methods
2. Update consumers to inject specialized services
3. Remove WhmcsService after all migrations complete

---

3.4 Dead Code Audit and Removal

Audit Process:

1. Search for unused exports:

   # Find exports that are never imported
   npx ts-prune --project apps/bff/tsconfig.json
   

2. Check specific suspected methods:

   # Check if method is called anywhere
   grep -r "hasInvoices" --include="*.ts" apps/
   grep -r "getInvoiceCountByStatus" --include="*.ts" apps/
   

3. Review test coverage reports:

   • Methods with 0% coverage are likely unused

Suspected Dead Code:

File	Method	Status
invoice-retrieval.service.ts	hasInvoices()	Check usage
invoice-retrieval.service.ts	getInvoiceCountByStatus()	Check usage
subscriptions.service.ts	checkUserHasExistingSim()	Private, check if called
user-profile.service.ts	N/A	Audit after split

---

3.5 Create LoggableService Base Class (Optional)

Problem: 107 files have identical logger injection boilerplate.

Solution:

// File: apps/bff/src/core/base/loggable.service.ts
import { Inject, Injectable } from "@nestjs/common";
import { Logger } from "nestjs-pino";

@Injectable()
export abstract class LoggableService {
  @Inject(Logger) protected readonly logger: Logger;
}

Usage:

@Injectable()
export class InvoiceRetrievalService extends LoggableService {
  constructor(
    private readonly whmcsService: WhmcsService,
    private readonly mappingsService: MappingsService
  ) {
    super();
  }
  // this.logger is available
}

Trade-offs:

• Pro: Reduces boilerplate
• Con: Adds inheritance, which can complicate testing
• Con: NestJS DI works best with explicit constructor injection

Recommendation: Low priority. Only implement if team agrees inheritance is acceptable.

---

Implementation Schedule

Sprint 1 (Week 1-2): Quick Wins

Day	Task	Assignee
1-2	1.1 Add mapping helper methods	-
2-4	1.2 Refactor all mapping lookups (16 files)	-
4-5	1.3 Rename getErrorMessage functions	-
5	1.4 Audit and remove unused wrappers	-
6-7	Testing and PR review	-

Sprint 2 (Week 3-4): Error Handling

Day	Task	Assignee
1-2	2.1 Create error handling utility	-
2-5	2.2 Refactor high-priority services	-
5-6	2.3 Consolidate WHMCS connection methods	-
7	Testing and PR review	-

Sprint 3 (Week 5-6): Service Splits

Day	Task	Assignee
1-4	3.1 Split SignupWorkflowService	-
4-7	3.2 Split UserProfileService	-

Sprint 4 (Week 7-8): Cleanup

Day	Task	Assignee
1-3	3.3 WhmcsService facade decision	-
3-5	3.4 Dead code audit	-
5-7	3.5 Optional: LoggableService	-
7	Final testing and documentation	-

---

Risk Mitigation

Testing Strategy

1. Before any refactoring:

   • Ensure existing tests pass
   • Add tests for any untested code being modified

2. For each change:

   • Run unit tests
   • Run integration tests
   • Manual smoke testing of affected flows

3. Key flows to test:

   • User signup
   • User login
   • Invoice retrieval
   • Subscription management
   • Dashboard loading

Rollback Plan

1. All changes should be in feature branches
2. Use feature flags for major architectural changes
3. Deploy to staging environment first
4. Monitor error rates after production deployment

---

Success Metrics

Metric	Before	Target
Duplicate mapping lookup occurrences	16+	0
Lines in SignupWorkflowService	703	<200
Lines in UserProfileService	482	<200
Duplicate error handling blocks	141	<30
Dead code methods	~10	0

---

Appendix: File Change Summary

Files to Create

Path	Description
apps/bff/src/core/utils/error-handler.util.ts	Centralized error handling
apps/bff/src/modules/auth/infra/workflows/signup/signup-orchestrator.service.ts	Signup orchestration
apps/bff/src/modules/auth/infra/workflows/signup/signup-validation.service.ts	Signup validation
apps/bff/src/modules/auth/infra/workflows/signup/signup-account-resolver.service.ts	Account resolution
apps/bff/src/modules/users/infra/dashboard.service.ts	Dashboard summary
apps/bff/src/modules/users/utils/whmcs-custom-fields.util.ts	Custom field extraction

Files to Modify

Path	Changes
apps/bff/src/modules/id-mappings/mappings.service.ts	Add helper methods
apps/bff/src/core/utils/error.util.ts	Rename function
packages/domain/common/errors.ts	Rename function
apps/bff/src/integrations/whmcs/connection/services/whmcs-connection-orchestrator.service.ts	Consolidate methods
16+ files with mapping lookups	Use new helper
73+ files importing error.util	Update import

Files to Delete (after migration)

Path	Reason
Unused wrapper methods	Dead code
apps/bff/src/modules/auth/infra/workflows/signup-workflow.service.ts	Replaced by split services