feat: add API Property Name Guessing Audit and Orchestrator Refactoring Plan documentation
This commit is contained in:
barsa
parent
4cb393bdb8
commit
9e442a51f8
249
docs/refactoring/api-property-guessing-audit.md
Normal file
249
docs/refactoring/api-property-guessing-audit.md
Normal file
@ -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<string, string> = {
|
||||
voiceMail: "voicemail",
|
||||
callWaiting: "callwaiting",
|
||||
worldWing: "worldwing",
|
||||
status: "state",
|
||||
simSize: "size",
|
||||
};
|
||||
|
||||
export function normalizeFreebitResponse<T>(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
|
||||
729
docs/refactoring/orchestrator-refactoring-plan.md
Normal file
729
docs/refactoring/orchestrator-refactoring-plan.md
Normal file
@ -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<string, unknown> {
|
||||
const config: Record<string, unknown> = {};
|
||||
|
||||
if (rawConfigurations && typeof rawConfigurations === "object") {
|
||||
Object.assign(config, rawConfigurations as Record<string, unknown>);
|
||||
}
|
||||
|
||||
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<string, unknown>,
|
||||
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<string, unknown>,
|
||||
idempotencyKey: string
|
||||
): Promise<OrderFulfillmentContext> {
|
||||
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<string, unknown>
|
||||
): 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<void> {
|
||||
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<void> {
|
||||
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<T>(
|
||||
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<AccountStatus> {
|
||||
// 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
|
||||
Loading…
x
Reference in New Issue
Block a user