From 9e442a51f83142be4483652afea6e517d3ab24c3 Mon Sep 17 00:00:00 2001 From: barsa Date: Tue, 3 Feb 2026 17:37:42 +0900 Subject: [PATCH] feat: add API Property Name Guessing Audit and Orchestrator Refactoring Plan documentation --- .../api-property-guessing-audit.md | 249 ++++++ .../orchestrator-refactoring-plan.md | 729 ++++++++++++++++++ 2 files changed, 978 insertions(+) create mode 100644 docs/refactoring/api-property-guessing-audit.md create mode 100644 docs/refactoring/orchestrator-refactoring-plan.md diff --git a/docs/refactoring/api-property-guessing-audit.md b/docs/refactoring/api-property-guessing-audit.md new file mode 100644 index 00000000..dac25b59 --- /dev/null +++ b/docs/refactoring/api-property-guessing-audit.md @@ -0,0 +1,249 @@ +# API Property Name Guessing Audit + +> **Created:** 2026-02-03 +> **Status:** Needs Resolution +> **Priority:** Medium-High + +## Overview + +This document catalogs all instances where the codebase "guesses" which property name an external API will return. These patterns indicate **missing type safety** at system boundaries and were introduced because the exact API response structure was unknown during implementation. + +--- + +## Problem Statement + +The codebase contains 30+ instances of fallback property access patterns like: + +```typescript +// We don't know if the API returns "voicemail" or "voiceMail" +const value = account.voicemail ?? account.voiceMail; +``` + +This creates: + +- **Runtime uncertainty** - code works but we don't know which branch executes +- **Dead code potential** - one fallback may never be used +- **Maintenance burden** - developers must remember to check both variants +- **Type system weakness** - TypeScript can't help us catch mismatches + +--- + +## Complete Inventory + +### 1. Freebit Integration (15 instances) + +The Freebit API returns properties with inconsistent casing. Both variants are defined in types because we observed both in production. + +#### Voice Options (camelCase vs lowercase) + +| Location | Pattern | Fields | +| ------------------------------------------- | -------------------------------------------- | --------------------- | +| `freebit-mapper.service.ts:134` | `account.voicemail ?? account.voiceMail` | Voice mail setting | +| `freebit-mapper.service.ts:136` | `account.callwaiting ?? account.callWaiting` | Call waiting setting | +| `freebit-mapper.service.ts:140` | `account.worldwing ?? account.worldWing` | International roaming | +| `domain/sim/providers/freebit/mapper.ts:89` | `account.voicemail ?? account.voiceMail` | Duplicate | +| `domain/sim/providers/freebit/mapper.ts:90` | `account.callwaiting ?? account.callWaiting` | Duplicate | +| `domain/sim/providers/freebit/mapper.ts:91` | `account.worldwing ?? account.worldWing` | Duplicate | + +#### Account Status/Identity + +| Location | Pattern | Purpose | +| ------------------------------- | ----------------------------------------------------- | ------------------ | +| `freebit-mapper.service.ts:103` | `account.networkType ?? account.contractLine ?? "4G"` | Network generation | +| `freebit-mapper.service.ts:189` | `account.state ?? account.status ?? "pending"` | Account status | +| `freebit-mapper.service.ts:195` | `account.msisdn ?? account.account ?? ""` | Phone number | + +#### Type Definitions Allow Both (Root Cause) + +**File:** `apps/bff/src/integrations/freebit/interfaces/freebit.types.ts` + +```typescript +// Lines 31-32: Status field ambiguity +state: "active" | "suspended" | "temporary" | "waiting" | "obsolete"; +status?: "active" | "suspended" | "temporary" | "waiting" | "obsolete"; + +// Lines 42-43: SIM size field ambiguity +size?: "standard" | "nano" | "micro" | "esim"; +simSize?: "standard" | "nano" | "micro" | "esim"; + +// Lines 52-57: Voice options casing ambiguity +voicemail?: "10" | "20" | number | null; +voiceMail?: "10" | "20" | number | null; +callwaiting?: "10" | "20" | number | null; +callWaiting?: "10" | "20" | number | null; +worldwing?: "10" | "20" | number | null; +worldWing?: "10" | "20" | number | null; +``` + +--- + +### 2. MNP Data Mapping (10 instances) + +Salesforce stores MNP (Mobile Number Portability) data with different field names than Freebit expects. + +**File:** `apps/bff/src/modules/orders/services/sim-fulfillment.service.ts` + +| Line | Salesforce Name | Freebit Name | Purpose | +| ---- | -------------------------- | ------------------- | ---------------------- | +| 449 | `mnpNumber` | `reserveNumber` | MNP reservation number | +| 450 | `mnpExpiry` | `reserveExpireDate` | Reservation expiry | +| 451 | `mvnoAccountNumber` | `account` | MVNO account ID | +| 452 | `portingFirstName` | `firstnameKanji` | First name (kanji) | +| 453 | `portingLastName` | `lastnameKanji` | Last name (kanji) | +| 455 | `portingFirstNameKatakana` | `firstnameZenKana` | First name (katakana) | +| 458 | `portingLastNameKatakana` | `lastnameZenKana` | Last name (katakana) | +| 460 | `portingGender` | `gender` | Gender | +| 461 | `portingDateOfBirth` | `birthday` | Date of birth | +| 444 | `source["isMnp"]` | `config["isMnp"]` | MNP flag location | + +--- + +### 3. WHMCS Integration (4 instances) + +Different WHMCS API endpoints return slightly different field names. + +#### Billing Mapper + +**File:** `packages/domain/billing/providers/whmcs/mapper.ts` + +| Line | Pattern | Cause | +| ---- | ----------------------------------------------- | ----------------------- | +| 80 | `invoicePayload.invoiceid ?? invoicePayload.id` | List vs detail endpoint | +| 120 | `listItem.invoiceid ?? listItem.id` | Same issue | + +#### Customer Mapper + +**File:** `packages/domain/customer/providers/whmcs/mapper.ts` + +| Line | Pattern | Cause | +| ---- | --------------------------------------------------- | ------------------------- | +| 54 | `client.fullstate ?? client.state` | Full name vs abbreviation | +| 58 | `client.phonenumberformatted ?? client.phonenumber` | Formatted vs raw | + +--- + +### 4. Order/Product Data (1 instance) + +**File:** `packages/domain/orders/helpers.ts` + +| Line | Pattern | Cause | +| ---- | ---------------------------------------- | ---------------------- | +| 362 | `item.totalPrice ?? item.unitPrice ?? 0` | Different price fields | + +--- + +## Summary Statistics + +| Source System | Guessing Patterns | Severity | +| -------------------- | ----------------- | -------- | +| Freebit | 15 | High | +| Salesforce ↔ Freebit | 10 | Medium | +| WHMCS | 4 | Low | +| Internal | 1 | Low | +| **Total** | **30** | | + +--- + +## Recommended Actions + +### Phase 1: Normalization Layer + +Create a preprocessing step that normalizes field names at the API boundary: + +```typescript +// New file: freebit-response-normalizer.ts +const FIELD_ALIASES: Record = { + voiceMail: "voicemail", + callWaiting: "callwaiting", + worldWing: "worldwing", + status: "state", + simSize: "size", +}; + +export function normalizeFreebitResponse(raw: T): T { + // Recursively rename aliased fields to canonical names + return transformKeys(raw, FIELD_ALIASES); +} +``` + +Apply in `freebit-client.service.ts` immediately after receiving API response. + +### Phase 2: Type Strictness + +Update raw types to allow only ONE canonical name: + +```typescript +// Before (allows both) +export interface FreebitAccountDetail { + voicemail?: "10" | "20" | number | null; + voiceMail?: "10" | "20" | number | null; // DELETE THIS +} + +// After (single source of truth) +export interface FreebitAccountDetail { + voicemail?: "10" | "20" | number | null; // Canonical name only +} +``` + +### Phase 3: Cleanup + +Remove all fallback patterns from mappers: + +```typescript +// Before +const voiceMailEnabled = parseFlag(account.voicemail ?? account.voiceMail); + +// After (normalizer already handled this) +const voiceMailEnabled = parseFlag(account.voicemail); +``` + +### Phase 4: MNP Field Standardization + +For MNP data, choose ONE canonical naming convention and update Salesforce field mappings: + +| Canonical Name | Use This Everywhere | +| ------------------- | ------------------------------ | +| `reserveNumber` | Not `mnpNumber` | +| `reserveExpireDate` | Not `mnpExpiry` | +| `firstnameKanji` | Not `portingFirstName` | +| `lastnameKanji` | Not `portingLastName` | +| `firstnameZenKana` | Not `portingFirstNameKatakana` | +| `lastnameZenKana` | Not `portingLastNameKatakana` | +| `gender` | Not `portingGender` | +| `birthday` | Not `portingDateOfBirth` | + +Map explicitly in the Salesforce mapper, not with fallbacks in sim-fulfillment. + +--- + +## File Checklist + +Files requiring updates after resolution: + +### Freebit + +- [ ] `apps/bff/src/integrations/freebit/interfaces/freebit.types.ts` +- [ ] `apps/bff/src/integrations/freebit/services/freebit-mapper.service.ts` +- [ ] `packages/domain/sim/providers/freebit/raw.types.ts` +- [ ] `packages/domain/sim/providers/freebit/mapper.ts` + +### WHMCS + +- [ ] `packages/domain/billing/providers/whmcs/mapper.ts` +- [ ] `packages/domain/billing/providers/whmcs/raw.types.ts` +- [ ] `packages/domain/customer/providers/whmcs/mapper.ts` +- [ ] `packages/domain/customer/providers/whmcs/raw.types.ts` + +### Orders + +- [ ] `apps/bff/src/modules/orders/services/sim-fulfillment.service.ts` +- [ ] `packages/domain/orders/helpers.ts` + +--- + +## Success Criteria + +1. **Zero fallback patterns** - No `a ?? b` for same conceptual field +2. **Strict raw types** - Each field has exactly one name +3. **Normalization at edge** - Field name mapping happens once, at API boundary +4. **Type safety** - TypeScript catches field name mismatches at compile time diff --git a/docs/refactoring/orchestrator-refactoring-plan.md b/docs/refactoring/orchestrator-refactoring-plan.md new file mode 100644 index 00000000..a828cce6 --- /dev/null +++ b/docs/refactoring/orchestrator-refactoring-plan.md @@ -0,0 +1,729 @@ +# Orchestrator Refactoring Plan + +## Overview + +This document outlines the refactoring plan for BFF orchestrators to align with enterprise-grade code structure patterns used by companies like Google, Amazon, and Microsoft. + +**Date**: 2026-02-03 +**Branch**: alt-design (post-merge from main) + +--- + +## Orchestrator Assessment Summary + +| Orchestrator | Lines | Status | Priority | +| ------------------------------------------- | ----- | -------------------- | -------- | +| `order-fulfillment-orchestrator.service.ts` | ~990 | ❌ Needs Refactoring | **HIGH** | +| `subscriptions-orchestrator.service.ts` | ~475 | ⚠️ Minor Issues | LOW | +| `auth-orchestrator.service.ts` | ~324 | ✅ Mostly Good | LOW | +| `order-orchestrator.service.ts` | ~270 | ✅ Good | NONE | +| `sim-orchestrator.service.ts` | ~200 | ✅ Excellent | NONE | +| `billing-orchestrator.service.ts` | ~47 | ✅ Perfect | NONE | + +--- + +## HIGH Priority: Order Fulfillment Orchestrator + +### Current Problems + +1. **Inline Anonymous Functions in Distributed Transaction** (lines 172-557) + - 10 transaction steps defined as inline arrow functions + - Each step contains 20-80 lines of business logic + - Violates Single Responsibility Principle + - Difficult to unit test individual steps + +2. **File Size**: ~990 lines (should be < 300 for an orchestrator) + +3. **Mixed Concerns**: Helper methods embedded in orchestrator + - `extractConfigurations()` (60+ lines) - data extraction + - `extractContactIdentity()` (50+ lines) - data extraction + - `formatBirthdayToYYYYMMDD()` (30 lines) - date formatting + +4. **Duplicate Logic**: Step tracking pattern repeated in each step + +### Target Architecture + +``` +apps/bff/src/modules/orders/ +├── services/ +│ ├── order-fulfillment-orchestrator.service.ts (~200 lines) +│ ├── order-fulfillment-steps/ +│ │ ├── index.ts +│ │ ├── validation.step.ts +│ │ ├── sf-status-update.step.ts +│ │ ├── sim-fulfillment.step.ts +│ │ ├── sf-activated-update.step.ts +│ │ ├── whmcs-mapping.step.ts +│ │ ├── whmcs-create.step.ts +│ │ ├── whmcs-accept.step.ts +│ │ ├── sf-registration-complete.step.ts +│ │ └── opportunity-update.step.ts +│ └── mappers/ +│ └── order-configuration.mapper.ts +``` + +### Refactoring Steps + +#### Step 1: Create Step Interface and Base Class + +```typescript +// apps/bff/src/modules/orders/services/order-fulfillment-steps/fulfillment-step.interface.ts + +import type { OrderFulfillmentContext } from "../order-fulfillment-orchestrator.service.js"; +import type { TransactionStep } from "@bff/infra/database/services/distributed-transaction.service.js"; + +export interface FulfillmentStepConfig { + context: OrderFulfillmentContext; + logger: Logger; +} + +export interface FulfillmentStep { + readonly id: string; + readonly description: string; + readonly critical: boolean; + + /** + * Build the transaction step for the distributed transaction + */ + build(config: FulfillmentStepConfig): TransactionStep; + + /** + * Check if this step should be included based on context + */ + shouldInclude(context: OrderFulfillmentContext): boolean; +} +``` + +#### Step 2: Extract Each Step to Its Own Class + +**Example: SIM Fulfillment Step** + +```typescript +// apps/bff/src/modules/orders/services/order-fulfillment-steps/sim-fulfillment.step.ts + +import { Injectable, Inject } from "@nestjs/common"; +import { Logger } from "nestjs-pino"; +import type { FulfillmentStep, FulfillmentStepConfig } from "./fulfillment-step.interface.js"; +import type { OrderFulfillmentContext } from "../order-fulfillment-orchestrator.service.js"; +import { SimFulfillmentService } from "../sim-fulfillment.service.js"; +import { OrderConfigurationMapper } from "../mappers/order-configuration.mapper.js"; +import { extractErrorMessage } from "@bff/core/utils/error.util.js"; + +@Injectable() +export class SimFulfillmentStep implements FulfillmentStep { + readonly id = "sim_fulfillment"; + readonly description = "SIM activation via Freebit (PA05-18 + PA02-01 + PA05-05)"; + readonly critical = true; // Set dynamically based on SIM type + + constructor( + private readonly simFulfillmentService: SimFulfillmentService, + private readonly configMapper: OrderConfigurationMapper, + @Inject(Logger) private readonly logger: Logger + ) {} + + shouldInclude(context: OrderFulfillmentContext): boolean { + return context.orderDetails?.orderType === "SIM"; + } + + build(config: FulfillmentStepConfig) { + const { context } = config; + + return { + id: this.id, + description: this.description, + execute: async () => this.execute(context), + rollback: async () => this.rollback(context), + critical: context.validation?.sfOrder?.SIM_Type__c === "Physical SIM", + }; + } + + private async execute(context: OrderFulfillmentContext) { + if (context.orderDetails?.orderType !== "SIM") { + return { activated: false, simType: "eSIM" as const }; + } + + const sfOrder = context.validation?.sfOrder; + const configurations = this.configMapper.extractConfigurations( + context.payload?.configurations, + sfOrder + ); + + const assignedPhysicalSimId = this.extractAssignedSimId(sfOrder); + const voiceMailEnabled = sfOrder?.SIM_Voice_Mail__c === true; + const callWaitingEnabled = sfOrder?.SIM_Call_Waiting__c === true; + const contactIdentity = this.configMapper.extractContactIdentity(sfOrder); + + this.logger.log("Starting SIM fulfillment (before WHMCS)", { + orderId: context.sfOrderId, + simType: sfOrder?.SIM_Type__c, + assignedPhysicalSimId, + voiceMailEnabled, + callWaitingEnabled, + hasContactIdentity: !!contactIdentity, + }); + + const result = await this.simFulfillmentService.fulfillSimOrder({ + orderDetails: context.orderDetails, + configurations, + assignedPhysicalSimId, + voiceMailEnabled, + callWaitingEnabled, + contactIdentity, + }); + + context.simFulfillmentResult = result; + return result; + } + + private async rollback(context: OrderFulfillmentContext) { + this.logger.warn("SIM fulfillment step needs rollback - manual intervention may be required", { + sfOrderId: context.sfOrderId, + simFulfillmentResult: context.simFulfillmentResult, + }); + } + + private extractAssignedSimId(sfOrder?: SalesforceOrderRecord | null): string | undefined { + return typeof sfOrder?.Assign_Physical_SIM__c === "string" + ? sfOrder.Assign_Physical_SIM__c + : undefined; + } +} +``` + +#### Step 3: Extract Configuration Mapper + +```typescript +// apps/bff/src/modules/orders/services/mappers/order-configuration.mapper.ts + +import { Injectable } from "@nestjs/common"; +import type { SalesforceOrderRecord } from "@customer-portal/domain/orders/providers"; +import type { ContactIdentityData } from "../sim-fulfillment.service.js"; + +@Injectable() +export class OrderConfigurationMapper { + /** + * Extract and normalize configurations from payload and Salesforce order + */ + extractConfigurations( + rawConfigurations: unknown, + sfOrder?: SalesforceOrderRecord | null + ): Record { + const config: Record = {}; + + if (rawConfigurations && typeof rawConfigurations === "object") { + Object.assign(config, rawConfigurations as Record); + } + + if (sfOrder) { + this.fillFromSalesforceOrder(config, sfOrder); + } + + return config; + } + + /** + * Extract contact identity data from Salesforce order porting fields + */ + extractContactIdentity(sfOrder?: SalesforceOrderRecord | null): ContactIdentityData | undefined { + if (!sfOrder) return undefined; + + const firstnameKanji = sfOrder.Porting_FirstName__c; + const lastnameKanji = sfOrder.Porting_LastName__c; + const firstnameKana = sfOrder.Porting_FirstName_Katakana__c; + const lastnameKana = sfOrder.Porting_LastName_Katakana__c; + const genderRaw = sfOrder.Porting_Gender__c; + const birthdayRaw = sfOrder.Porting_DateOfBirth__c; + + if (!this.validateNameFields(firstnameKanji, lastnameKanji, firstnameKana, lastnameKana)) { + return undefined; + } + + const gender = this.validateGender(genderRaw); + if (!gender) return undefined; + + const birthday = this.formatBirthdayToYYYYMMDD(birthdayRaw); + if (!birthday) return undefined; + + return { + firstnameKanji: firstnameKanji!, + lastnameKanji: lastnameKanji!, + firstnameKana: firstnameKana!, + lastnameKana: lastnameKana!, + gender, + birthday, + }; + } + + /** + * Format birthday from various formats to YYYYMMDD + */ + formatBirthdayToYYYYMMDD(dateStr?: string | null): string | undefined { + if (!dateStr) return undefined; + + if (/^\d{8}$/.test(dateStr)) { + return dateStr; + } + + const isoMatch = dateStr.match(/^(\d{4})-(\d{2})-(\d{2})/); + if (isoMatch) { + return `${isoMatch[1]}${isoMatch[2]}${isoMatch[3]}`; + } + + try { + const date = new Date(dateStr); + if (!isNaN(date.getTime())) { + const year = date.getFullYear(); + const month = String(date.getMonth() + 1).padStart(2, "0"); + const day = String(date.getDate()).padStart(2, "0"); + return `${year}${month}${day}`; + } + } catch { + // Parsing failed + } + + return undefined; + } + + private fillFromSalesforceOrder( + config: Record, + sfOrder: SalesforceOrderRecord + ): void { + const fieldMappings: Array<[string, keyof SalesforceOrderRecord]> = [ + ["simType", "SIM_Type__c"], + ["eid", "EID__c"], + ["activationType", "Activation_Type__c"], + ["scheduledAt", "Activation_Scheduled_At__c"], + ["mnpPhone", "MNP_Phone_Number__c"], + ["mnpNumber", "MNP_Reservation_Number__c"], + ["mnpExpiry", "MNP_Expiry_Date__c"], + ["mvnoAccountNumber", "MVNO_Account_Number__c"], + ["portingFirstName", "Porting_FirstName__c"], + ["portingLastName", "Porting_LastName__c"], + ["portingFirstNameKatakana", "Porting_FirstName_Katakana__c"], + ["portingLastNameKatakana", "Porting_LastName_Katakana__c"], + ["portingGender", "Porting_Gender__c"], + ["portingDateOfBirth", "Porting_DateOfBirth__c"], + ]; + + for (const [configKey, sfField] of fieldMappings) { + if (!config[configKey] && sfOrder[sfField]) { + config[configKey] = sfOrder[sfField]; + } + } + + // Handle MNP flag specially + if (!config["isMnp"] && sfOrder.MNP_Application__c) { + config["isMnp"] = "true"; + } + } + + private validateNameFields( + firstnameKanji?: string | null, + lastnameKanji?: string | null, + firstnameKana?: string | null, + lastnameKana?: string | null + ): boolean { + return !!(firstnameKanji && lastnameKanji && firstnameKana && lastnameKana); + } + + private validateGender(genderRaw?: string | null): "M" | "F" | undefined { + return genderRaw === "M" || genderRaw === "F" ? genderRaw : undefined; + } +} +``` + +#### Step 4: Create Step Builder Service + +```typescript +// apps/bff/src/modules/orders/services/order-fulfillment-steps/fulfillment-step-builder.service.ts + +import { Injectable, Inject } from "@nestjs/common"; +import { Logger } from "nestjs-pino"; +import type { TransactionStep } from "@bff/infra/database/services/distributed-transaction.service.js"; +import type { OrderFulfillmentContext } from "../order-fulfillment-orchestrator.service.js"; + +// Import all steps +import { ValidationStep } from "./validation.step.js"; +import { SfStatusUpdateStep } from "./sf-status-update.step.js"; +import { SimFulfillmentStep } from "./sim-fulfillment.step.js"; +import { SfActivatedUpdateStep } from "./sf-activated-update.step.js"; +import { WhmcsMappingStep } from "./whmcs-mapping.step.js"; +import { WhmcsCreateStep } from "./whmcs-create.step.js"; +import { WhmcsAcceptStep } from "./whmcs-accept.step.js"; +import { SfRegistrationCompleteStep } from "./sf-registration-complete.step.js"; +import { OpportunityUpdateStep } from "./opportunity-update.step.js"; + +@Injectable() +export class FulfillmentStepBuilder { + private readonly allSteps: FulfillmentStep[]; + + constructor( + validationStep: ValidationStep, + sfStatusUpdateStep: SfStatusUpdateStep, + simFulfillmentStep: SimFulfillmentStep, + sfActivatedUpdateStep: SfActivatedUpdateStep, + whmcsMappingStep: WhmcsMappingStep, + whmcsCreateStep: WhmcsCreateStep, + whmcsAcceptStep: WhmcsAcceptStep, + sfRegistrationCompleteStep: SfRegistrationCompleteStep, + opportunityUpdateStep: OpportunityUpdateStep, + @Inject(Logger) private readonly logger: Logger + ) { + // Define step execution order + this.allSteps = [ + sfStatusUpdateStep, + simFulfillmentStep, + sfActivatedUpdateStep, + whmcsMappingStep, + whmcsCreateStep, + whmcsAcceptStep, + sfRegistrationCompleteStep, + opportunityUpdateStep, + ]; + } + + /** + * Build transaction steps based on context + */ + buildTransactionSteps(context: OrderFulfillmentContext): TransactionStep[] { + const config = { context, logger: this.logger }; + + return this.allSteps + .filter(step => step.shouldInclude(context)) + .map(step => step.build(config)); + } +} +``` + +#### Step 5: Refactor Orchestrator to Use Step Builder + +```typescript +// apps/bff/src/modules/orders/services/order-fulfillment-orchestrator.service.ts (REFACTORED) + +import { Injectable, Inject } from "@nestjs/common"; +import { Logger } from "nestjs-pino"; +import { DistributedTransactionService } from "@bff/infra/database/services/distributed-transaction.service.js"; +import { FulfillmentStepBuilder } from "./order-fulfillment-steps/fulfillment-step-builder.service.js"; +import { OrderFulfillmentValidator } from "./order-fulfillment-validator.service.js"; +import { OrderFulfillmentErrorService } from "./order-fulfillment-error.service.js"; +import { OrderEventsService } from "./order-events.service.js"; +import { OrdersCacheService } from "./orders-cache.service.js"; +import { extractErrorMessage } from "@bff/core/utils/error.util.js"; +import { FulfillmentException } from "@bff/core/exceptions/domain-exceptions.js"; + +// ... types remain the same ... + +@Injectable() +export class OrderFulfillmentOrchestrator { + constructor( + @Inject(Logger) private readonly logger: Logger, + private readonly stepBuilder: FulfillmentStepBuilder, + private readonly validator: OrderFulfillmentValidator, + private readonly errorService: OrderFulfillmentErrorService, + private readonly transactionService: DistributedTransactionService, + private readonly orderEvents: OrderEventsService, + private readonly ordersCache: OrdersCacheService + ) {} + + async executeFulfillment( + sfOrderId: string, + payload: Record, + idempotencyKey: string + ): Promise { + const context = this.initializeContext(sfOrderId, idempotencyKey, payload); + + this.logger.log("Starting transactional fulfillment orchestration", { + sfOrderId, + idempotencyKey, + }); + + try { + // Step 1: Validate + const validation = await this.validator.validateFulfillmentRequest(sfOrderId, idempotencyKey); + context.validation = validation; + + if (validation.isAlreadyProvisioned) { + return this.handleAlreadyProvisioned(context); + } + + // Step 2: Build and execute transaction steps + const steps = this.stepBuilder.buildTransactionSteps(context); + const result = await this.transactionService.executeDistributedTransaction(steps, { + description: `Order fulfillment for ${sfOrderId}`, + timeout: 300000, + continueOnNonCriticalFailure: true, + }); + + if (!result.success) { + throw new FulfillmentException(result.error || "Fulfillment transaction failed", { + sfOrderId, + idempotencyKey, + stepsExecuted: result.stepsExecuted, + stepsRolledBack: result.stepsRolledBack, + }); + } + + this.logger.log("Transactional fulfillment completed successfully", { + sfOrderId, + stepsExecuted: result.stepsExecuted, + duration: result.duration, + }); + + await this.invalidateCaches(context); + return context; + } catch (error) { + await this.handleFulfillmentError(context, error as Error); + throw error; + } + } + + private initializeContext( + sfOrderId: string, + idempotencyKey: string, + payload: Record + ): OrderFulfillmentContext { + return { + sfOrderId, + idempotencyKey, + validation: null, + payload, + steps: [], + }; + } + + private handleAlreadyProvisioned(context: OrderFulfillmentContext): OrderFulfillmentContext { + this.logger.log("Order already provisioned, skipping fulfillment", { + sfOrderId: context.sfOrderId, + }); + this.orderEvents.publish(context.sfOrderId, { + orderId: context.sfOrderId, + status: "Completed", + activationStatus: "Activated", + stage: "completed", + source: "fulfillment", + message: "Order already provisioned", + timestamp: new Date().toISOString(), + payload: { whmcsOrderId: context.validation?.whmcsOrderId }, + }); + return context; + } + + private async invalidateCaches(context: OrderFulfillmentContext): Promise { + const accountId = context.validation?.sfOrder?.AccountId; + await Promise.all([ + this.ordersCache.invalidateOrder(context.sfOrderId), + accountId ? this.ordersCache.invalidateAccountOrders(accountId) : Promise.resolve(), + ]).catch(error => { + this.logger.warn("Failed to invalidate caches", { error: extractErrorMessage(error) }); + }); + } + + private async handleFulfillmentError( + context: OrderFulfillmentContext, + error: Error + ): Promise { + await this.invalidateCaches(context); + await this.errorService.handleAndReport(context, error); + this.orderEvents.publishFailure(context.sfOrderId, error.message); + } + + getFulfillmentSummary(context: OrderFulfillmentContext) { + // ... same as before, no changes needed ... + } +} +``` + +#### Step 6: Update Module Registration + +```typescript +// apps/bff/src/modules/orders/orders.module.ts + +// Add new providers +import { FulfillmentStepBuilder } from "./services/order-fulfillment-steps/fulfillment-step-builder.service.js"; +import { OrderConfigurationMapper } from "./services/mappers/order-configuration.mapper.js"; +import { + SfStatusUpdateStep, + SimFulfillmentStep, + SfActivatedUpdateStep, + WhmcsMappingStep, + WhmcsCreateStep, + WhmcsAcceptStep, + SfRegistrationCompleteStep, + OpportunityUpdateStep, +} from "./services/order-fulfillment-steps/index.js"; + +@Module({ + providers: [ + // Existing services... + + // New step classes + FulfillmentStepBuilder, + OrderConfigurationMapper, + SfStatusUpdateStep, + SimFulfillmentStep, + SfActivatedUpdateStep, + WhmcsMappingStep, + WhmcsCreateStep, + WhmcsAcceptStep, + SfRegistrationCompleteStep, + OpportunityUpdateStep, + ], +}) +export class OrdersModule {} +``` + +--- + +## LOW Priority: Subscriptions Orchestrator + +### Current Issues + +1. Business logic in filter methods (lines 193-250): + - `getExpiringSoon()` - date filtering logic + - `getRecentActivity()` - date filtering logic + - `searchSubscriptions()` - search logic + +2. Cache helper methods could be in a separate service + +### Recommended Changes + +1. Extract date filtering to a utility: + + ```typescript + // @bff/core/utils/date-filter.util.ts + export function filterByDateRange( + items: T[], + dateExtractor: (item: T) => Date, + range: { start?: Date; end?: Date } + ): T[] { ... } + ``` + +2. Consider extracting `SubscriptionFilterService` if filtering becomes more complex + +**Status**: Not urgent - current implementation is acceptable + +--- + +## LOW Priority: Auth Orchestrator + +### Current Issues + +1. `getAccountStatus()` method (lines 241-296) contains complex conditional logic + - Could be extracted to `AccountStatusResolver` service + +### Recommended Changes + +```typescript +// apps/bff/src/modules/auth/application/account-status-resolver.service.ts + +@Injectable() +export class AccountStatusResolver { + async resolve(email: string): Promise { + // Move logic from getAccountStatus here + } +} +``` + +**Status**: Not urgent - method is well-contained and testable as-is + +--- + +## No Changes Needed + +### Order Orchestrator ✅ + +- Clean thin delegation pattern +- Appropriate size (~270 lines) +- Single private method is justified + +### SIM Orchestrator ✅ + +- Excellent thin delegation +- All methods delegate to specialized services +- `getSimInfo` has reasonable composition logic + +### Billing Orchestrator ✅ + +- Perfect thin facade pattern +- Pure delegation only +- Model example for others + +--- + +## Implementation Checklist + +### Phase 1: High Priority (Order Fulfillment Orchestrator) + +- [ ] Create `order-fulfillment-steps/` directory +- [ ] Create `fulfillment-step.interface.ts` +- [ ] Create `OrderConfigurationMapper` service +- [ ] Extract `SfStatusUpdateStep` +- [ ] Extract `SimFulfillmentStep` +- [ ] Extract `SfActivatedUpdateStep` +- [ ] Extract `WhmcsMappingStep` +- [ ] Extract `WhmcsCreateStep` +- [ ] Extract `WhmcsAcceptStep` +- [ ] Extract `SfRegistrationCompleteStep` +- [ ] Extract `OpportunityUpdateStep` +- [ ] Create `FulfillmentStepBuilder` service +- [ ] Refactor `OrderFulfillmentOrchestrator` to use step builder +- [ ] Update `OrdersModule` with new providers +- [ ] Add unit tests for each step class +- [ ] Add integration tests for step builder + +### Phase 2: Low Priority + +- [ ] Extract `SubscriptionFilterService` (if needed) +- [ ] Extract `AccountStatusResolver` (if needed) + +--- + +## Testing Strategy + +### Step Classes + +Each step class should have unit tests that verify: + +1. `shouldInclude()` returns correct boolean based on context +2. `execute()` performs expected operations +3. `rollback()` handles cleanup appropriately +4. Error cases are handled correctly + +### Step Builder + +Integration tests should verify: + +1. Correct steps are included for SIM orders +2. Correct steps are included for non-SIM orders +3. Step order is maintained +4. Context is properly passed between steps + +### Orchestrator + +E2E tests should verify: + +1. Complete fulfillment flow works +2. Partial failures trigger rollbacks +3. Already provisioned orders are handled +4. Error reporting works correctly + +--- + +## Benefits of This Refactoring + +1. **Testability**: Each step can be unit tested in isolation +2. **Maintainability**: Changes to one step don't affect others +3. **Readability**: Orchestrator becomes a thin coordinator +4. **Extensibility**: New steps can be added without modifying orchestrator +5. **Reusability**: Steps can potentially be reused in other workflows +6. **Debugging**: Easier to identify which step failed +7. **Code Review**: Smaller, focused PRs for each step + +--- + +## References + +- Google Engineering Practices: https://google.github.io/eng-practices/ +- Microsoft .NET Application Architecture: https://docs.microsoft.com/en-us/dotnet/architecture/ +- Clean Architecture by Robert C. Martin +- Domain-Driven Design by Eric Evans