diff --git a/COMPREHENSIVE_CODE_REVIEW_2025.md b/COMPREHENSIVE_CODE_REVIEW_2025.md new file mode 100644 index 00000000..68c4b294 --- /dev/null +++ b/COMPREHENSIVE_CODE_REVIEW_2025.md @@ -0,0 +1,855 @@ +# Comprehensive Code Review Report +**Customer Portal - Architecture & Code Quality Analysis** + +**Date**: October 28, 2025 +**Reviewer**: AI Code Reviewer +**Scope**: Full codebase review for separation of concerns, domain design, and code quality + +--- + +## Executive Summary + +The customer portal codebase demonstrates **good overall architecture** with a clear BFF (Backend for Frontend) pattern and a dedicated domain package. However, there are **several critical architectural violations** and **technical debt** that need to be addressed to achieve true separation of concerns and maintainable code. + +### Key Metrics +- **Architecture Score**: 7/10 (Good foundation, needs refinement) +- **Separation of Concerns**: 6/10 (Domain exists but inconsistently used) +- **Code Quality**: 7.5/10 (Generally clean with some issues) +- **File Structure**: 8/10 (Well-organized, some redundancy) + +--- + +## ✅ Strengths (What's Done Well) + +### 1. **Domain Package Architecture** +- **Well-structured domain package** at `packages/domain/` with clear separation by business domain (catalog, orders, sim, billing, etc.) +- **Schema-first approach** using Zod for runtime validation with TypeScript type inference +- **Provider adapters** properly isolated (Salesforce, WHMCS, Freebit) with clean mapping logic +- **Centralized validation** schemas in domain package + +### 2. **BFF Layer Implementation** +- **Clean controller layer** with proper use of NestJS decorators and guards +- **Service-oriented architecture** with clear separation (orchestrators, validators, builders) +- **Proper dependency injection** patterns throughout +- **Structured logging** using nestjs-pino +- **Rate limiting** on sensitive endpoints (orders, catalog) + +### 3. **Frontend Architecture** +- **Feature-based organization** in `apps/portal/src/features/` +- **Component hierarchy** (atoms, molecules, organisms) following atomic design +- **Zustand state management** for complex state (auth, catalog configuration) +- **React Query integration** for data fetching +- **Proper hooks abstraction** for business logic encapsulation + +### 4. **Type Safety** +- **Strong TypeScript usage** throughout the codebase +- **Type inference from Zod schemas** (schema-first approach) +- **Minimal use of `any` types** + +--- + +## ❌ Critical Issues (Must Fix) + +### 1. **Business Logic in Frontend** ⚠️ CRITICAL + +**Location**: `apps/portal/src/features/catalog/utils/catalog.utils.ts` + +**Issue**: Frontend contains business logic for filtering, sorting, and calculating product values. + +```typescript +// ❌ BAD: Frontend has business logic +export function filterProducts( + products: CatalogProduct[], + filters: { category?: string; priceMin?: number; priceMax?: number; search?: string; } +): CatalogProduct[] { + return products.filter(product => { + if (typeof filters.priceMin === "number") { + const price = product.monthlyPrice ?? product.oneTimePrice ?? 0; + if (price < filters.priceMin) return false; + } + // ... more filtering logic + }); +} + +export function sortProducts(products: CatalogProduct[], sortBy: "name" | "price" = "name"): CatalogProduct[] { + // ... sorting logic +} +``` + +**Why This Is Wrong**: +- **Separation of concerns violated**: Frontend should only display data, not transform it +- **Inconsistent business rules**: Client-side logic can diverge from server-side +- **Security risk**: Business rules can be bypassed by modifying frontend code +- **Performance**: Heavy operations on client-side + +**Solution**: +```typescript +// ✅ GOOD: Move to BFF +// apps/bff/src/modules/catalog/services/catalog.service.ts +async getFilteredProducts( + filters: CatalogFilter +): Promise { + // Apply filtering server-side + // Return pre-filtered, pre-calculated results +} +``` + +**Impact**: HIGH - Violates fundamental architectural principles + +--- + +### 2. **Business Validation Logic Split Between Layers** ⚠️ HIGH + +**Location**: +- `apps/portal/src/features/checkout/hooks/useCheckout.ts` (lines 51-59, 211-213) +- `apps/bff/src/modules/orders/services/order-validator.service.ts` + +**Issue**: Business validation duplicated in frontend and backend. + +```typescript +// ❌ BAD: Business validation in frontend hook +const hasActiveInternetSubscription = useMemo(() => { + if (!Array.isArray(activeSubs)) return false; + return activeSubs.some( + subscription => + String(subscription.groupName || subscription.productName || "") + .toLowerCase() + .includes("internet") && + String(subscription.status || "").toLowerCase() === "active" + ); +}, [activeSubs]); + +// Client-side guard: prevent Internet orders if an Internet subscription already exists +if (orderType === ORDER_TYPE.INTERNET && hasActiveInternetSubscription && !isDevEnvironment) { + throw new Error(ACTIVE_INTERNET_WARNING_MESSAGE); +} +``` + +**Why This Is Wrong**: +- **Duplicate validation**: Same rule exists in BFF (`validateInternetDuplication`) +- **Inconsistency risk**: Frontend and backend rules can diverge +- **Security**: Client-side checks can be bypassed +- **Development mode override**: Business rules shouldn't be environment-dependent + +**Solution**: +```typescript +// ✅ GOOD: Single source of truth in BFF +// Frontend only displays warnings/hints +// BFF enforces business rules absolutely +// Remove all business rule enforcement from frontend +``` + +**Impact**: HIGH - Security and consistency risk + +--- + +### 3. **Frontend Stores Contain Business Logic** ⚠️ MEDIUM-HIGH + +**Location**: `apps/portal/src/features/catalog/services/catalog.store.ts` + +**Issue**: Store contains complex business logic for building checkout parameters, validating forms, and transforming data. + +```typescript +// ❌ BAD: Complex transformation logic in frontend store (lines 196-304) +buildSimCheckoutParams: () => { + const { sim } = get(); + if (!sim.planSku) return null; + + const rawSelections: Record = { + plan: sim.planSku, + simType: sim.simType, + activationType: sim.activationType, + }; + + // 50+ lines of complex transformation logic + // Building MNP data, normalizing selections, etc. +} +``` + +**Why This Is Wrong**: +- **Business logic in presentation layer**: Stores should only manage state, not transform it +- **Complex transformations**: Should be in domain package +- **Tight coupling**: Frontend tightly coupled to domain implementation details + +**Solution**: +```typescript +// ✅ GOOD: Move to domain package +// packages/domain/orders/helpers.ts +export function buildSimCheckoutSelections(config: SimConfigState): OrderSelections { + // Transformation logic here +} + +// Frontend store only calls helper +buildSimCheckoutParams: () => { + const { sim } = get(); + return buildSimCheckoutSelections(sim); +} +``` + +**Impact**: MEDIUM-HIGH - Maintainability and testability issues + +--- + +### 4. **Validation Schema Duplication** ⚠️ MEDIUM + +**Location**: Multiple files + +**Issue**: Validation patterns duplicated across layers instead of using domain schemas. + +```typescript +// ❌ BAD: Local validation schema in BFF +// apps/bff/src/integrations/salesforce/utils/soql.util.ts +const nonEmptyStringSchema = z.string().min(1, "Value cannot be empty").trim(); +const soqlFieldNameSchema = z.string().trim().regex(/^[A-Za-z0-9_.]+$/, "Invalid SOQL field name"); + +// ❌ BAD: Controller-specific response schema +// apps/bff/src/modules/orders/orders.controller.ts +private readonly createOrderResponseSchema = apiSuccessResponseSchema( + z.object({ + sfOrderId: z.string(), + status: z.string(), + message: z.string(), + }) +); +``` + +**Why This Is Wrong**: +- **Duplication**: Same validation logic defined in multiple places +- **Inconsistency**: Validation rules can diverge +- **Not DRY**: Violates Don't Repeat Yourself principle + +**Solution**: +```typescript +// ✅ GOOD: Define once in domain +// packages/domain/common/validation.ts +export const nonEmptyStringSchema = z.string().min(1).trim(); +export const salesforceIdSchema = z.string().length(18); + +// packages/domain/orders/schema.ts +export const orderCreateResponseSchema = z.object({ + sfOrderId: z.string(), + status: z.string(), + message: z.string(), +}); +``` + +**Impact**: MEDIUM - Code duplication and maintenance burden + +--- + +### 5. **Infrastructure Types in Business Layer** ⚠️ MEDIUM + +**Location**: Various service files + +**Issue**: Infrastructure-specific types defined in business services instead of proper boundaries. + +```typescript +// ❌ BAD: Infrastructure type in service +// apps/bff/src/modules/invoices/services/invoice-retrieval.service.ts +interface UserMappingInfo { + userId: string; + whmcsClientId: number; +} + +// ❌ BAD: BFF-specific interface that could be domain type +// apps/bff/src/modules/orders/services/order-fulfillment-validator.service.ts +export interface OrderFulfillmentValidationResult { + sfOrder: SalesforceOrderRecord; + clientId: number; + isAlreadyProvisioned: boolean; + whmcsOrderId?: string; +} +``` + +**Why This Is Wrong**: +- **Blurred boundaries**: Infrastructure concerns mixed with business logic +- **Tight coupling**: Services coupled to specific infrastructure details +- **Hard to test**: Infrastructure types make unit testing harder + +**Solution**: +```typescript +// ✅ GOOD: Define in appropriate domain or create proper abstraction +// packages/domain/orders/types.ts +export interface OrderFulfillmentValidation { + orderId: string; + customerAccountId: string; + isProvisioned: boolean; + externalOrderId?: string; +} +``` + +**Impact**: MEDIUM - Architectural clarity and testability + +--- + +### 6. **Complex Business Logic in Frontend Hooks** ⚠️ MEDIUM + +**Location**: `apps/portal/src/features/checkout/hooks/useCheckout.ts` (lines 90-123) + +**Issue**: Complex parsing and configuration building in frontend hooks. + +```typescript +// ❌ BAD: Complex business logic in hook (lines 90-123) +const { selections, configurations } = useMemo(() => { + const rawSelections: Record = {}; + params.forEach((value, key) => { + if (key !== "type") { + rawSelections[key] = value; + } + }); + + try { + const normalizedSelections = normalizeOrderSelections(rawSelections); + let configuration: OrderConfigurations | undefined; + + try { + configuration = buildOrderConfigurations(normalizedSelections); + } catch (error) { + logger.warn("Failed to derive order configurations from selections", { + error: error instanceof Error ? error.message : String(error), + }); + } + + return { + selections: normalizedSelections, + configurations: configuration, + }; + } catch (error) { + // ... error handling + } +}, [params]); +``` + +**Why This Is Wrong**: +- **Complex logic in UI layer**: Hooks should be simple state managers +- **Silent error swallowing**: Try-catch with fallbacks hide issues +- **Hard to test**: Logic tied to React hooks ecosystem + +**Solution**: +```typescript +// ✅ GOOD: Move to service layer +// apps/portal/src/features/checkout/services/checkout-params.service.ts +export class CheckoutParamsService { + static parseSelections(params: URLSearchParams): OrderSelections { + // Parsing logic + } + + static buildConfigurations(selections: OrderSelections): OrderConfigurations { + // Building logic + } +} + +// Hook becomes simple +const selections = CheckoutParamsService.parseSelections(params); +const configurations = CheckoutParamsService.buildConfigurations(selections); +``` + +**Impact**: MEDIUM - Maintainability and testability + +--- + +## 🔴 Architectural Issues + +### 1. **Inconsistent Domain Package Usage** + +**Issue**: Not all business types are in the domain package. Some are scattered across BFF and Portal. + +**Examples**: +- `CheckoutItem` used in Portal but not defined in domain +- `PricingTier` defined in UI components +- `CatalogFilter` has TODO comment indicating missing domain type + +**Solution**: +- Move ALL business types to domain package +- Frontend and BFF should ONLY import from domain +- Domain package should be the single source of truth + +--- + +### 2. **Lack of Domain Services** + +**Issue**: Business logic spread across BFF services without clear domain service layer. + +**Observation**: +``` +packages/domain/ + catalog/ + schema.ts ✅ + contract.ts ✅ + providers/ ✅ + index.ts ✅ + services/ ❌ MISSING + validation.ts ❌ MISSING +``` + +**Solution**: +```typescript +// packages/domain/orders/services/order-validation.service.ts +export class OrderValidationService { + static validateOrderFulfillment(order: Order): ValidationResult { + // Pure business logic here + } +} +``` + +**Impact**: Architectural clarity and testability + +--- + +### 3. **Mixed Responsibility in Controllers** + +**Issue**: Controllers doing more than HTTP request/response handling. + +**Example**: +```typescript +// ❌ BAD: Controller handles parsing logic +@Get("internet/plans") +async getInternetPlans(@Request() req: RequestWithUser) { + const userId = req.user?.id; + if (!userId) { + const catalog = await this.internetCatalog.getCatalogData(); + return parseInternetCatalog(catalog); // Parsing in controller + } + // ... +} +``` + +**Solution**: +```typescript +// ✅ GOOD: Service handles all logic +@Get("internet/plans") +async getInternetPlans(@Request() req: RequestWithUser) { + return this.catalogService.getInternetPlansForUser(req.user?.id); +} +``` + +--- + +### 4. **Tight Coupling to External Services** + +**Issue**: Business logic services directly coupled to integration services (WHMCS, Salesforce). + +**Example**: +```typescript +// apps/bff/src/modules/orders/services/order-validator.service.ts +constructor( + private readonly whmcs: WhmcsConnectionOrchestratorService, + private readonly mappings: MappingsService, + // ... directly depends on infrastructure +) +``` + +**Solution**: Introduce repository pattern or ports/adapters to decouple business logic from infrastructure. + +--- + +## 🟡 Code Quality Issues + +### 1. **Inconsistent Error Handling** + +**Issue**: Mix of generic `throw new Error()` and typed exceptions. + +**Found**: +- Some services use typed exceptions (`SimActivationException`, `OrderValidationException`) +- Many still use generic `throw new Error()` +- Inconsistent error message formatting + +**Solution**: Standardize on typed exception hierarchy across all services. + +--- + +### 2. **Missing Input Validation in Some Services** + +**Issue**: Some service methods don't validate inputs before processing. + +**Example**: +```typescript +// apps/bff/src/modules/orders/services/sim-fulfillment.service.ts (lines 28-34) +const simType = this.readEnum(configurations.simType, ["eSIM", "Physical SIM"]) ?? "eSIM"; +const eid = this.readString(configurations.eid); +// ... no validation that configurations is not null/undefined +``` + +**Solution**: Add input validation at service boundaries. + +--- + +### 3. **Complex Service Methods** + +**Issue**: Some service methods are very long (100+ lines) with complex logic. + +**Examples**: +- `OrderFulfillmentOrchestrator.fulfillOrder()` (likely 200+ lines) +- `SimFulfillmentService.fulfillSimOrder()` (100 lines with nested conditionals) + +**Solution**: Break down into smaller, focused methods with single responsibility. + +--- + +### 4. **Inadequate Logging Context** + +**Issue**: Some log statements lack sufficient context for debugging. + +**Example**: +```typescript +this.logger.log("SIM fulfillment completed successfully", { + orderId: orderDetails.id, + account: phoneNumber, + planSku, +}); +// Missing: simType, activationType, whether MNP was used, etc. +``` + +**Solution**: Add comprehensive context to all log statements. + +--- + +### 5. **Magic Strings and Numbers** + +**Issue**: Various magic strings and numbers scattered throughout code. + +**Examples**: +```typescript +// apps/portal/src/features/checkout/hooks/useCheckout.ts +const ACTIVE_INTERNET_WARNING_MESSAGE = "You already have an active..."; // Should be in constants +const isDevEnvironment = process.env.NODE_ENV === "development"; // Direct env check + +// apps/bff/src/modules/orders/controllers/checkout.service.ts (line 55) +if (simType === "eSIM" && (!eid || eid.length < 15)) { // Magic number 15 +``` + +**Solution**: Extract to constants or configuration. + +--- + +## 📁 File Structure Issues + +### 1. **Inconsistent Module Organization** + +**Issue**: Some modules have deep nesting, others are flat. + +**Example**: +``` +modules/auth/ + application/ + infra/ + workflows/ + workflows/ ← Redundant nesting + presentation/ + services/ + +vs. + +modules/catalog/ + services/ + catalog.controller.ts + catalog.module.ts +``` + +**Solution**: Standardize on consistent depth and organization. + +--- + +### 2. **Mixed Concerns in Directories** + +**Issue**: Some directories contain both business logic and infrastructure code. + +**Example**: +``` +modules/orders/ + controllers/ ← HTTP layer + services/ ← Mix of business logic and orchestration + queue/ ← Infrastructure + types/ ← Shared types +``` + +**Solution**: Separate by architectural layer (domain, application, infrastructure, presentation). + +--- + +### 3. **Redundant Type Directories** + +**Issue**: Types scattered across multiple locations. + +**Found**: +- `apps/bff/src/types/` +- `apps/bff/src/modules/*/types/` +- `packages/domain/*/` + +**Solution**: All business types in domain, infrastructure types in their respective modules. + +--- + +## 🔧 Technical Debt + +### 1. **Build Artifacts in Source** ⚠️ RESOLVED + +**Status**: Fixed (per CODEBASE_ANALYSIS.md) +- Previously: 224 generated files in source tree +- Now: Build outputs isolated to `dist/` + +--- + +### 2. **Console.log Statements** + +**Issue**: 3 console.log instances found in portal (should use logger) + +**Found**: `apps/portal/src/lib/logger.ts:2` + +**Solution**: Replace with proper logging infrastructure. + +--- + +### 3. **Incomplete Type Safety** + +**Issue**: Some areas still have loose typing. + +**Examples**: +- `unknown` and `any` usage in error handling +- Generic `Record` for complex objects +- Missing discriminated unions for variant types + +--- + +### 4. **Missing Tests** + +**Observation**: No test files found in search (likely in separate test directories). + +**Recommendation**: Ensure comprehensive test coverage, especially for: +- Domain validation logic +- Business rules +- Order fulfillment workflows +- Integration adapters + +--- + +## 📊 Detailed Findings by Domain + +### Authentication & Authorization + +**Strengths**: +- ✅ Clean store implementation with Zustand +- ✅ Proper JWT handling with httpOnly cookies +- ✅ Session management with refresh tokens + +**Issues**: +- ⚠️ Complex workflow logic in frontend store (auth.store.ts) +- ⚠️ Nested directory structure (`infra/workflows/workflows/`) + +--- + +### Catalog & Product Management + +**Strengths**: +- ✅ Domain types well-defined (CatalogProduct, InternetPlan, SimProduct, etc.) +- ✅ Provider adapters cleanly separated +- ✅ Caching implemented (CatalogCacheService) + +**Issues**: +- ❌ Filtering and sorting logic in frontend utils +- ❌ Business logic in catalog store +- ⚠️ Parsing logic in controllers instead of services + +--- + +### Orders & Checkout + +**Strengths**: +- ✅ Clear orchestration pattern (OrderOrchestrator) +- ✅ Separate validation service (OrderValidator) +- ✅ Domain schemas for order types +- ✅ Business validation in domain package + +**Issues**: +- ❌ Frontend validation duplicating backend validation +- ❌ Complex checkout hook with business logic +- ⚠️ Long service methods that could be broken down + +--- + +### Subscriptions & Billing + +**Strengths**: +- ✅ Clean data fetching with React Query +- ✅ Proper type definitions + +**Issues**: +- ⚠️ Minimal business logic encapsulation +- ⚠️ Direct WHMCS coupling in some areas + +--- + +## 🎯 Recommendations + +### Immediate Actions (High Priority) + +#### 1. **Remove Business Logic from Frontend** ⚠️ CRITICAL +- **Timeline**: 1-2 weeks +- **Impact**: HIGH +- **Action**: + - Move `filterProducts`, `sortProducts` from `catalog.utils.ts` to BFF + - Remove client-side business validation from `useCheckout` hook + - Simplify frontend stores to only manage UI state + +#### 2. **Consolidate Validation Schemas** ⚠️ HIGH +- **Timeline**: 1 week +- **Impact**: MEDIUM +- **Action**: + - Move all validation schemas to domain package + - Remove duplicate schemas from BFF and Portal + - Update imports across codebase + +#### 3. **Extract Business Types to Domain** ⚠️ HIGH +- **Timeline**: 1 week +- **Impact**: MEDIUM +- **Action**: + - Move `OrderFulfillmentValidationResult`, `PricingTier`, `CheckoutItem` to domain + - Define missing types (`CatalogFilter`, etc.) + - Update all imports + +--- + +### Medium Priority Actions + +#### 4. **Introduce Domain Services Layer** +- **Timeline**: 2-3 weeks +- **Impact**: MEDIUM +- **Action**: + - Create `packages/domain/*/services/` directories + - Move pure business logic from BFF services to domain services + - BFF services become thin orchestration layer + +#### 5. **Standardize Error Handling** +- **Timeline**: 1-2 weeks +- **Impact**: MEDIUM +- **Action**: + - Complete typed exception hierarchy + - Replace all generic `throw new Error()` with typed exceptions + - Add proper error context + +#### 6. **Refactor Long Service Methods** +- **Timeline**: 2 weeks +- **Impact**: LOW-MEDIUM +- **Action**: + - Break down methods >50 lines into smaller functions + - Apply Single Responsibility Principle + - Improve testability + +--- + +### Long-term Improvements + +#### 7. **Implement Repository Pattern** +- **Timeline**: 3-4 weeks +- **Impact**: MEDIUM +- **Action**: + - Introduce repositories to decouple from Salesforce/WHMCS + - Define ports/adapters architecture + - Improve testability with mocking + +#### 8. **Comprehensive Testing Strategy** +- **Timeline**: Ongoing +- **Impact**: HIGH +- **Action**: + - Unit tests for all domain services + - Integration tests for BFF services + - E2E tests for critical user flows + +#### 9. **Documentation Improvements** +- **Timeline**: 1-2 weeks +- **Impact**: LOW +- **Action**: + - Architecture Decision Records (ADRs) + - API documentation + - Code comments for complex business logic + +--- + +## 📋 Implementation Plan + +### Phase 1: Critical Fixes (Weeks 1-3) +1. **Week 1**: Remove business logic from frontend utilities +2. **Week 2**: Consolidate validation schemas in domain +3. **Week 3**: Extract business types to domain package + +### Phase 2: Architectural Improvements (Weeks 4-6) +4. **Week 4-5**: Introduce domain services layer +5. **Week 6**: Standardize error handling across codebase + +### Phase 3: Quality Improvements (Weeks 7-9) +6. **Week 7**: Refactor long service methods +7. **Week 8-9**: Implement repository pattern + +### Phase 4: Long-term Excellence (Weeks 10+) +8. **Week 10+**: Comprehensive testing strategy +9. **Ongoing**: Documentation improvements + +--- + +## 📈 Success Metrics + +### Code Quality Metrics +- **Type Safety**: 95%+ of code strongly typed +- **Business Logic in Domain**: 90%+ of business logic in domain package +- **Validation Consistency**: Single validation schema per business rule +- **Test Coverage**: 80%+ for domain and business logic + +### Architectural Metrics +- **Separation of Concerns**: Clear boundaries between layers +- **Coupling**: Low coupling between modules +- **Cohesion**: High cohesion within modules +- **Import Clarity**: All business types imported from domain + +--- + +## 🎓 Learning & Best Practices + +### What to Continue +1. **Schema-first approach**: Continue using Zod schemas as source of truth +2. **Provider adapters**: Continue clean separation of external service integration +3. **Feature-based organization**: Frontend feature structure is good +4. **Dependency injection**: Continue using DI in BFF + +### What to Stop +1. **Business logic in frontend**: Stop putting business rules in UI components/hooks +2. **Duplicate validation**: Stop defining validation schemas in multiple places +3. **Direct external service coupling**: Stop directly coupling business logic to infrastructure +4. **Magic values**: Stop using magic strings/numbers without constants + +### What to Start +1. **Domain services**: Start creating pure business logic services in domain +2. **Repository pattern**: Start abstracting data access +3. **Typed exceptions**: Start using typed exceptions everywhere +4. **Comprehensive logging**: Start adding rich context to all logs + +--- + +## 🔍 Conclusion + +The customer portal has a **solid architectural foundation** with good separation between BFF and Portal, and a well-structured domain package. However, there are **critical violations of separation of concerns**, particularly business logic leaking into the frontend. + +### Summary Assessment +- **Architecture**: 7/10 - Good foundation but needs refinement +- **Domain Design**: 6/10 - Well-structured but underutilized +- **Code Quality**: 7.5/10 - Generally clean with some technical debt +- **Maintainability**: 6.5/10 - Will improve significantly after addressing issues + +### Priority Focus Areas +1. **Remove all business logic from frontend** (Critical) +2. **Consolidate validation in domain** (High) +3. **Complete domain type migration** (High) +4. **Introduce domain services layer** (Medium) + +By addressing these issues systematically, the codebase will achieve: +- ✅ True separation of concerns +- ✅ Single source of truth for business logic +- ✅ Improved testability +- ✅ Better maintainability +- ✅ Cleaner architecture + +--- + +**Next Steps**: Prioritize Phase 1 actions and create detailed task tickets for implementation. + +**Review Schedule**: Re-assess after Phase 1 completion (3 weeks) to measure progress. + diff --git a/apps/portal/src/config/environment.ts b/apps/portal/src/config/environment.ts new file mode 100644 index 00000000..76888c93 --- /dev/null +++ b/apps/portal/src/config/environment.ts @@ -0,0 +1 @@ +export const IS_DEVELOPMENT = process.env.NODE_ENV === "development"; diff --git a/apps/portal/src/features/catalog/components/base/CardBadge.tsx b/apps/portal/src/features/catalog/components/base/CardBadge.tsx index 377107b0..76b8cf99 100644 --- a/apps/portal/src/features/catalog/components/base/CardBadge.tsx +++ b/apps/portal/src/features/catalog/components/base/CardBadge.tsx @@ -1,18 +1,18 @@ "use client"; -export type BadgeVariant = - | "gold" - | "platinum" - | "silver" - | "recommended" - | "family" - | "new" +export type BadgeVariant = + | "gold" + | "platinum" + | "silver" + | "recommended" + | "family" + | "new" | "default"; interface CardBadgeProps { text: string; variant?: BadgeVariant; - size?: "sm" | "md"; + size?: "xs" | "sm" | "md"; } export function CardBadge({ text, variant = "default", size = "md" }: CardBadgeProps) { @@ -35,12 +35,22 @@ export function CardBadge({ text, variant = "default", size = "md" }: CardBadgeP } }; - const sizeClasses = size === "sm" ? "text-xs px-2 py-0.5" : "text-xs px-2.5 py-1"; + const sizeClasses = (() => { + switch (size) { + case "xs": + return "text-[11px] px-1.5 py-[2px]"; + case "sm": + return "text-xs px-2 py-0.5"; + default: + return "text-xs px-2.5 py-1"; + } + })(); return ( - + {text} ); } - diff --git a/apps/portal/src/features/catalog/components/internet/InternetPlanCard.tsx b/apps/portal/src/features/catalog/components/internet/InternetPlanCard.tsx index c3491791..0135919d 100644 --- a/apps/portal/src/features/catalog/components/internet/InternetPlanCard.tsx +++ b/apps/portal/src/features/catalog/components/internet/InternetPlanCard.tsx @@ -11,6 +11,8 @@ import { useRouter } from "next/navigation"; import { CardPricing } from "@/features/catalog/components/base/CardPricing"; import { CardBadge } from "@/features/catalog/components/base/CardBadge"; import type { BadgeVariant } from "@/features/catalog/components/base/CardBadge"; +import { useCatalogStore } from "@/features/catalog/services/catalog.store"; +import { IS_DEVELOPMENT } from "@/config/environment"; interface InternetPlanCardProps { plan: InternetPlanCatalogItem; @@ -30,8 +32,7 @@ export function InternetPlanCard({ const isGold = tier === "Gold"; const isPlatinum = tier === "Platinum"; const isSilver = tier === "Silver"; - const isDevEnvironment = process.env.NODE_ENV === "development"; - const isDisabled = disabled && !isDevEnvironment; + const isDisabled = disabled && !IS_DEVELOPMENT; const installationPrices = installations .map(installation => { @@ -116,7 +117,7 @@ export function InternetPlanCard({ installations.length > 0 && minInstallationPrice > 0 ? `Installation from ¥${minInstallationPrice.toLocaleString()}` : null, - ].filter((Boolean)) as string[]; + ].filter(Boolean) as string[]; return fallbackFeatures.map(renderFeature); }; @@ -130,9 +131,13 @@ export function InternetPlanCard({ {/* Header with badges and pricing */}
-
- - {isGold && } +
+ + {isGold && }
@@ -160,44 +165,7 @@ export function InternetPlanCard({ {/* Features */}

Your Plan Includes:

-
    - {plan.catalogMetadata?.features && plan.catalogMetadata.features.length > 0 ? ( - plan.catalogMetadata.features.map((feature, index) => ( -
  • - ✓ - {feature} -
    • - )) - ) : ( - <> -
  • - ✓ - NTT Optical Fiber (Flet's Hikari Next) -
    • -
  • - ✓ - - {plan.internetOfferingType?.includes("Apartment") ? "Mansion" : "Home"}{" "} - {plan.internetOfferingType?.includes("10G") - ? "10Gbps" - : plan.internetOfferingType?.includes("100M") - ? "100Mbps" - : "1Gbps"} connection - -
    • -
  • - ✓ - ISP connection protocols: IPoE and PPPoE -
    • - {installations.length > 0 && minInstallationPrice > 0 && ( -
  • - ✓ - Installation from ¥{minInstallationPrice.toLocaleString()} -
    • - )} - - )} -
+
    {renderPlanFeatures()}
{/* Action Button */} @@ -207,6 +175,9 @@ export function InternetPlanCard({ rightIcon={!isDisabled ? : undefined} onClick={() => { if (isDisabled) return; + const { resetInternetConfig, setInternetConfig } = useCatalogStore.getState(); + resetInternetConfig(); + setInternetConfig({ planSku: plan.sku, currentStep: 1 }); router.push(`/catalog/internet/configure?plan=${plan.sku}`); }} > diff --git a/apps/portal/src/features/catalog/components/internet/configure/InternetConfigureContainer.tsx b/apps/portal/src/features/catalog/components/internet/configure/InternetConfigureContainer.tsx index f28648de..3dbb797a 100644 --- a/apps/portal/src/features/catalog/components/internet/configure/InternetConfigureContainer.tsx +++ b/apps/portal/src/features/catalog/components/internet/configure/InternetConfigureContainer.tsx @@ -1,5 +1,6 @@ "use client"; +import { useEffect, useState, type ReactElement } from "react"; import { PageLayout } from "@/components/templates/PageLayout"; import { ProgressSteps } from "@/components/molecules"; import { Button } from "@/components/atoms/button"; @@ -58,10 +59,18 @@ export function InternetConfigureContainer({ currentStep, setCurrentStep, }: Props) { + const [renderedStep, setRenderedStep] = useState(currentStep); + const [transitionPhase, setTransitionPhase] = useState<"idle" | "enter" | "exit">("idle"); // Use local state ONLY for step validation, step management now in Zustand - const { - canProceedFromStep, - } = useConfigureState(plan, installations, addons, mode, selectedInstallation, currentStep, setCurrentStep); + const { canProceedFromStep } = useConfigureState( + plan, + installations, + addons, + mode, + selectedInstallation, + currentStep, + setCurrentStep + ); const handleAddonSelection = (newSelectedSkus: string[]) => setSelectedAddonSkus(newSelectedSkus); @@ -70,6 +79,30 @@ export function InternetConfigureContainer({ completed: currentStep > step.number, })); + useEffect(() => { + if (currentStep === renderedStep) return; + setTransitionPhase("exit"); + const exitTimer = window.setTimeout(() => { + setRenderedStep(currentStep); + setTransitionPhase("enter"); + }, 160); + + return () => { + window.clearTimeout(exitTimer); + }; + }, [currentStep, renderedStep]); + + useEffect(() => { + if (transitionPhase !== "enter") return; + const enterTimer = window.setTimeout(() => { + setTransitionPhase("idle"); + }, 240); + + return () => { + window.clearTimeout(enterTimer); + }; + }, [transitionPhase]); + if (loading) { return ; } @@ -88,6 +121,88 @@ export function InternetConfigureContainer({ ); } + const isTransitioning = transitionPhase !== "idle"; + + let stepContent: ReactElement | null = null; + switch (renderedStep) { + case 1: + stepContent = ( + canProceedFromStep(1) && setCurrentStep(2)} + /> + ); + break; + case 2: + stepContent = ( + setCurrentStep(1)} + onNext={() => canProceedFromStep(2) && setCurrentStep(3)} + /> + ); + break; + case 3: + stepContent = selectedInstallation ? ( + setCurrentStep(2)} + onNext={() => setCurrentStep(4)} + /> + ) : ( + setCurrentStep(1)} + onNext={() => canProceedFromStep(2) && setCurrentStep(3)} + /> + ); + break; + case 4: + stepContent = selectedInstallation ? ( + setCurrentStep(3)} + onConfirm={onConfirm} + /> + ) : ( + canProceedFromStep(1) && setCurrentStep(2)} + /> + ); + break; + default: + stepContent = ( + canProceedFromStep(1) && setCurrentStep(2)} + /> + ); + } + return ( } @@ -104,62 +219,15 @@ export function InternetConfigureContainer({
{/* Step Content */} -
- {currentStep === 1 && ( - canProceedFromStep(1) && setCurrentStep(2)} - /> - )} - - {currentStep === 2 && ( - setCurrentStep(1)} - onNext={() => canProceedFromStep(2) && setCurrentStep(3)} - /> - )} - - {currentStep === 3 && selectedInstallation && ( - setCurrentStep(2)} - onNext={() => setCurrentStep(4)} - /> - )} - - {currentStep === 4 && selectedInstallation && ( - setCurrentStep(3)} - onConfirm={onConfirm} - /> - )} +
+ {stepContent}
); } -function PlanHeader({ - plan, -}: { - plan: InternetPlanCatalogItem; -}) { +function PlanHeader({ plan }: { plan: InternetPlanCatalogItem }) { return (
- {isFamilyPlan && ( - - )} + {isFamilyPlan && }
{/* Pricing */}
- + {isFamilyPlan && (
Discounted pricing applied
)} @@ -48,10 +45,14 @@ export function SimPlanCard({ plan, isFamily }: { plan: SimCatalogProduct; isFam
{/* Action Button */} -
- ) : paymentMethods && paymentMethods.paymentMethods.length > 0 ? ( -

- Payment will be processed using your card on file after approval. -

+ ) : paymentMethodList.length > 0 ? ( +
+ {paymentMethodDisplay ? ( +
+
+
+

+ Default payment method +

+

+ {paymentMethodDisplay.title} +

+ {paymentMethodDisplay.subtitle ? ( +

+ {paymentMethodDisplay.subtitle} +

+ ) : null} +
+ +
+
+ ) : null} +

+ We securely charge your saved payment method after the order is approved. Need + to make changes? Visit Billing & Payments. +

+
) : (
@@ -254,4 +288,81 @@ export function CheckoutContainer() { ); } +function buildPaymentMethodDisplay(method: PaymentMethod): { title: string; subtitle?: string } { + const descriptor = + method.cardType?.trim() || + method.bankName?.trim() || + method.description?.trim() || + method.gatewayName?.trim() || + "Saved payment method"; + + const trimmedLastFour = + typeof method.cardLastFour === "string" && method.cardLastFour.trim().length > 0 + ? method.cardLastFour.trim().slice(-4) + : null; + + const headline = + trimmedLastFour && method.type?.toLowerCase().includes("card") + ? `${descriptor} · •••• ${trimmedLastFour}` + : descriptor; + + const details = new Set(); + + if (method.bankName && !headline.toLowerCase().includes(method.bankName.trim().toLowerCase())) { + details.add(method.bankName.trim()); + } + + const expiry = normalizeExpiryLabel(method.expiryDate); + if (expiry) { + details.add(`Exp ${expiry}`); + } + + if (!trimmedLastFour && method.cardLastFour && method.cardLastFour.trim().length > 0) { + details.add(`Ends ${method.cardLastFour.trim().slice(-4)}`); + } + + if (method.type?.toLowerCase().includes("bank") && method.description?.trim()) { + details.add(method.description.trim()); + } + + const subtitle = details.size > 0 ? Array.from(details).join(" · ") : undefined; + return { title: headline, subtitle }; +} + +function normalizeExpiryLabel(expiry?: string | null): string | null { + if (!expiry) return null; + const value = expiry.trim(); + if (!value) return null; + + if (/^\d{4}-\d{2}$/.test(value)) { + const [year, month] = value.split("-"); + return `${month}/${year.slice(-2)}`; + } + + if (/^\d{2}\/\d{4}$/.test(value)) { + const [month, year] = value.split("/"); + return `${month}/${year.slice(-2)}`; + } + + if (/^\d{2}\/\d{2}$/.test(value)) { + return value; + } + + const digits = value.replace(/\D/g, ""); + + if (digits.length === 6) { + const year = digits.slice(2, 4); + const month = digits.slice(4, 6); + return `${month}/${year}`; + } + + if (digits.length === 4) { + const month = digits.slice(0, 2); + const year = digits.slice(2, 4); + return `${month}/${year}`; + } + + return value; +} + export default CheckoutContainer; diff --git a/packages/domain/orders/checkout.ts b/packages/domain/orders/checkout.ts new file mode 100644 index 00000000..d4b642d3 --- /dev/null +++ b/packages/domain/orders/checkout.ts @@ -0,0 +1,284 @@ +import type { AccessModeValue } from "./contract"; +import type { OrderSelections } from "./schema"; +import { normalizeOrderSelections } from "./helpers"; +import type { ActivationType, MnpData, SimCardType } from "../sim"; + +/** + * Draft representation of Internet checkout configuration. + * Only includes business-relevant fields that should be transformed into selections. + */ +export interface InternetCheckoutDraft { + planSku?: string | null | undefined; + accessMode?: AccessModeValue | null | undefined; + installationSku?: string | null | undefined; + addonSkus?: readonly string[] | null | undefined; +} + +/** + * Patch representation of Internet checkout state derived from persisted selections. + * Consumers can merge this object into their local UI state. + */ +export interface InternetCheckoutStatePatch { + planSku?: string | null; + accessMode?: AccessModeValue | null; + installationSku?: string | null; + addonSkus?: string[]; +} + +/** + * Draft representation of SIM checkout configuration. + */ +export interface SimCheckoutDraft { + planSku?: string | null | undefined; + simType?: SimCardType | null | undefined; + activationType?: ActivationType | null | undefined; + eid?: string | null | undefined; + scheduledActivationDate?: string | null | undefined; + wantsMnp?: boolean | null | undefined; + mnpData?: Partial | null | undefined; + addonSkus?: readonly string[] | null | undefined; +} + +/** + * Patch representation of SIM checkout state derived from persisted selections. + */ +export interface SimCheckoutStatePatch { + planSku?: string | null; + simType?: SimCardType | null; + activationType?: ActivationType | null; + eid?: string; + scheduledActivationDate?: string; + wantsMnp?: boolean; + selectedAddons?: string[]; + mnpData?: Partial; +} + +const normalizeString = (value: unknown): string | undefined => { + if (typeof value !== "string") return undefined; + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : undefined; +}; + +const normalizeSkuList = (values: readonly string[] | null | undefined): string | undefined => { + if (!Array.isArray(values)) return undefined; + const sanitized = values + .map(normalizeString) + .filter((entry): entry is string => Boolean(entry)); + if (sanitized.length === 0) { + return undefined; + } + return sanitized.join(","); +}; + +const parseAddonList = (value: string | undefined): string[] => { + if (!value) return []; + return value + .split(",") + .map(entry => entry.trim()) + .filter(Boolean); +}; + +const coalescePlanSku = (selections: OrderSelections): string | null => { + const planCandidates = [ + selections.planSku, + selections.planIdSku, + selections.plan, + selections.planId, + ]; + + for (const candidate of planCandidates) { + const normalized = normalizeString(candidate); + if (normalized) { + return normalized; + } + } + + return null; +}; + +/** + * Build normalized order selections for Internet checkout from a UI draft. + * Ensures only business-relevant data is emitted. + */ +export function buildInternetCheckoutSelections( + draft: InternetCheckoutDraft +): OrderSelections { + const raw: Record = {}; + + const planSku = normalizeString(draft.planSku); + if (planSku) { + raw.plan = planSku; + raw.planSku = planSku; + } + + const accessMode = draft.accessMode ?? null; + if (accessMode) { + raw.accessMode = accessMode; + } + + const installationSku = normalizeString(draft.installationSku); + if (installationSku) { + raw.installationSku = installationSku; + } + + const addons = normalizeSkuList(draft.addonSkus); + if (addons) { + raw.addons = addons; + } + + return normalizeOrderSelections(raw); +} + +/** + * Derive Internet checkout UI state from normalized selections. + */ +export function deriveInternetCheckoutState( + selections: OrderSelections +): InternetCheckoutStatePatch { + const patch: InternetCheckoutStatePatch = { + addonSkus: parseAddonList(selections.addons), + }; + + const planSku = coalescePlanSku(selections); + if (planSku) { + patch.planSku = planSku; + } + + if (selections.accessMode) { + patch.accessMode = selections.accessMode; + } + + const installationSku = normalizeString(selections.installationSku); + if (installationSku) { + patch.installationSku = installationSku; + } + + return patch; +} + +/** + * Build normalized order selections for SIM checkout from a UI draft. + */ +export function buildSimCheckoutSelections(draft: SimCheckoutDraft): OrderSelections { + const raw: Record = {}; + + const planSku = normalizeString(draft.planSku); + if (planSku) { + raw.plan = planSku; + raw.planSku = planSku; + } + + if (draft.simType) { + raw.simType = draft.simType; + } + + if (draft.activationType) { + raw.activationType = draft.activationType; + } + + const eid = normalizeString(draft.eid); + if (draft.simType === "eSIM" && eid) { + raw.eid = eid; + } + + const scheduledAt = normalizeString(draft.scheduledActivationDate); + if (draft.activationType === "Scheduled" && scheduledAt) { + raw.scheduledAt = scheduledAt; + } + + const addons = normalizeSkuList(draft.addonSkus); + if (addons) { + raw.addons = addons; + } + + const wantsMnp = Boolean(draft.wantsMnp); + if (wantsMnp) { + raw.isMnp = "true"; + const mnpData = draft.mnpData ?? {}; + const assignIfPresent = (key: keyof MnpData, targetKey: keyof typeof raw) => { + const normalized = normalizeString(mnpData[key]); + if (normalized) { + raw[targetKey] = normalized; + } + }; + + assignIfPresent("reservationNumber", "mnpNumber"); + assignIfPresent("expiryDate", "mnpExpiry"); + assignIfPresent("phoneNumber", "mnpPhone"); + assignIfPresent("mvnoAccountNumber", "mvnoAccountNumber"); + assignIfPresent("portingLastName", "portingLastName"); + assignIfPresent("portingFirstName", "portingFirstName"); + assignIfPresent("portingLastNameKatakana", "portingLastNameKatakana"); + assignIfPresent("portingFirstNameKatakana", "portingFirstNameKatakana"); + assignIfPresent("portingGender", "portingGender"); + assignIfPresent("portingDateOfBirth", "portingDateOfBirth"); + } + + return normalizeOrderSelections(raw); +} + +/** + * Derive SIM checkout UI state from normalized selections. + */ +export function deriveSimCheckoutState(selections: OrderSelections): SimCheckoutStatePatch { + const planSku = coalescePlanSku(selections); + const simType = selections.simType ?? null; + const activationType = selections.activationType ?? null; + const eid = normalizeString(selections.eid); + const scheduledActivationDate = normalizeString(selections.scheduledAt); + const addonSkus = parseAddonList(selections.addons); + + const wantsMnp = Boolean( + selections.isMnp && + typeof selections.isMnp === "string" && + selections.isMnp.toLowerCase() === "true" + ); + + const mnpFields: Partial = {}; + + const assignField = (source: keyof OrderSelections, target: keyof MnpData) => { + const normalized = normalizeString(selections[source]); + if (normalized) { + mnpFields[target] = normalized; + } + }; + + if (wantsMnp) { + assignField("mnpNumber", "reservationNumber"); + assignField("mnpExpiry", "expiryDate"); + assignField("mnpPhone", "phoneNumber"); + assignField("mvnoAccountNumber", "mvnoAccountNumber"); + assignField("portingLastName", "portingLastName"); + assignField("portingFirstName", "portingFirstName"); + assignField("portingLastNameKatakana", "portingLastNameKatakana"); + assignField("portingFirstNameKatakana", "portingFirstNameKatakana"); + assignField("portingGender", "portingGender"); + assignField("portingDateOfBirth", "portingDateOfBirth"); + } + + const patch: SimCheckoutStatePatch = { + selectedAddons: addonSkus, + wantsMnp, + }; + + if (planSku) { + patch.planSku = planSku; + } + + if (simType) { + patch.simType = simType; + } + + if (activationType) { + patch.activationType = activationType; + } + + patch.eid = eid ?? ""; + patch.scheduledActivationDate = scheduledActivationDate ?? ""; + + if (wantsMnp && Object.keys(mnpFields).length > 0) { + patch.mnpData = mnpFields; + } + + return patch; +} diff --git a/packages/domain/orders/index.ts b/packages/domain/orders/index.ts index a1a7e2df..9ec6c8b5 100644 --- a/packages/domain/orders/index.ts +++ b/packages/domain/orders/index.ts @@ -45,6 +45,16 @@ export { normalizeOrderSelections, type BuildSimOrderConfigurationsOptions, } from "./helpers"; +export { + buildInternetCheckoutSelections, + deriveInternetCheckoutState, + buildSimCheckoutSelections, + deriveSimCheckoutState, + type InternetCheckoutDraft, + type InternetCheckoutStatePatch, + type SimCheckoutDraft, + type SimCheckoutStatePatch, +} from "./checkout"; // Re-export types for convenience export type {