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:

```typescript
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`:

```typescript
// 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:**

```typescript
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:**

```typescript
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`

```typescript
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:**

```typescript
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:**

```typescript
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:

```typescript
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):

```typescript
@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:**

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

**SignupAccountResolverService:**

```typescript
@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:**

```typescript
@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:**

```typescript
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:**

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

2. **Check specific suspected methods:**

   ```bash
   # 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:**

```typescript
// 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:**

```typescript
@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 |