diff --git a/src/gamification/points/points.service.spec.ts b/src/gamification/points/points.service.spec.ts index 66fbdf10..c62e261a 100644 --- a/src/gamification/points/points.service.spec.ts +++ b/src/gamification/points/points.service.spec.ts @@ -4,34 +4,60 @@ import { EventEmitter2 } from '@nestjs/event-emitter'; import { PointsService } from './points.service'; import { UserProgress } from '../entities/user-progress.entity'; import { PointTransaction } from '../entities/point-transaction.entity'; -import { TiersService } from '../tiers/tiers.service'; +import { TiersService, TIER_THRESHOLDS } from '../tiers/tiers.service'; import { Tier } from '../enums/tier.enum'; import { PointActivityType } from '../enums/point-activity.enum'; +import { GAMIFICATION_EVENTS } from '../events/gamification.events'; + +// --------------------------------------------------------------------------- +// Factory helpers +// --------------------------------------------------------------------------- const mockRepo = () => ({ findOne: jest.fn(), find: jest.fn(), - create: jest.fn((v) => v), - save: jest.fn((v) => Promise.resolve(v)), + // create() merges the provided value so callers get back the same shape + create: jest.fn((v) => ({ ...v })), + // save() resolves with exactly the object passed in so the persisted state + // is what the service assigned BEFORE calling save (Issue #1000). + save: jest.fn((v) => Promise.resolve({ ...v })), }); +// Real getTierForPoints logic, based on TIER_THRESHOLDS, so tier-boundary +// tests exercise the actual computation path. +const realGetTierForPoints = (totalPoints: number): Tier => { + const order: Tier[] = [Tier.BRONZE, Tier.SILVER, Tier.GOLD, Tier.PLATINUM, Tier.DIAMOND]; + let tier = Tier.BRONZE; + for (const t of order) { + if (totalPoints >= TIER_THRESHOLDS[t]) tier = t; + } + return tier; +}; + const mockTiersService = () => ({ - getTierForPoints: jest.fn().mockReturnValue(Tier.BRONZE), + getTierForPoints: jest.fn().mockImplementation(realGetTierForPoints), }); +// --------------------------------------------------------------------------- +// Shared setup +// --------------------------------------------------------------------------- + describe('PointsService', () => { let service: PointsService; let progressRepo: ReturnType; let txRepo: ReturnType; let tiersService: ReturnType; + let emitter: { emit: jest.Mock }; beforeEach(async () => { + emitter = { emit: jest.fn() }; + const module: TestingModule = await Test.createTestingModule({ providers: [ PointsService, { provide: getRepositoryToken(UserProgress), useFactory: mockRepo }, { provide: getRepositoryToken(PointTransaction), useFactory: mockRepo }, - { provide: EventEmitter2, useValue: { emit: jest.fn() } }, + { provide: EventEmitter2, useValue: emitter }, { provide: TiersService, useFactory: mockTiersService }, ], }).compile(); @@ -42,6 +68,10 @@ describe('PointsService', () => { tiersService = module.get(TiersService); }); + // ------------------------------------------------------------------------- + // addPoints — basic behaviour + // ------------------------------------------------------------------------- + describe('addPoints', () => { it('creates a transaction and updates progress', async () => { progressRepo.findOne.mockResolvedValue(null); @@ -75,7 +105,7 @@ describe('PointsService', () => { expect(progress.level).toBe(2); // floor(1100/1000)+1 = 2 }); - it('detects tier promotion', async () => { + it('detects tier promotion when tier changes', async () => { const existing: Partial = { totalPoints: 900, xp: 900, @@ -83,12 +113,12 @@ describe('PointsService', () => { tier: Tier.BRONZE, }; progressRepo.findOne.mockResolvedValue(existing); - tiersService.getTierForPoints.mockReturnValue(Tier.SILVER); + // 900 + 200 = 1100 → SILVER threshold is 1000 const { tierPromoted } = await service.addPoints('user-1', 200, 'TEST'); expect(tierPromoted).toBe(true); }); - it('does not flag promotion when tier unchanged', async () => { + it('does not flag promotion when tier is unchanged', async () => { const existing: Partial = { totalPoints: 100, xp: 100, @@ -96,12 +126,136 @@ describe('PointsService', () => { tier: Tier.BRONZE, }; progressRepo.findOne.mockResolvedValue(existing); - tiersService.getTierForPoints.mockReturnValue(Tier.BRONZE); + // 100 + 50 = 150 — still BRONZE const { tierPromoted } = await service.addPoints('user-1', 50, 'TEST'); expect(tierPromoted).toBe(false); }); + + // ------------------------------------------------------------------------- + // Issue #1000 — tier must be persisted in the same save call + // ------------------------------------------------------------------------- + + it('persists the new tier inside the save call (Issue #1000)', async () => { + const existing: Partial = { + totalPoints: 900, + xp: 900, + level: 1, + tier: Tier.BRONZE, + }; + progressRepo.findOne.mockResolvedValue(existing); + + // 900 + 200 = 1100 → SILVER + await service.addPoints('user-1', 200, 'TEST'); + + // The argument that was passed to save() must already carry the new tier, + // not the old one — no second save should be needed. + const savedArg: Partial = progressRepo.save.mock.calls[0][0]; + expect(savedArg.tier).toBe(Tier.SILVER); + }); + + it('returned progress.tier matches what was persisted (Issue #1000)', async () => { + progressRepo.findOne.mockResolvedValue(null); + + // Starting from 0, award enough to reach SILVER (1000 pts) + const { progress } = await service.addPoints('user-1', 1000, 'TEST'); + + expect(progress.tier).toBe(Tier.SILVER); + }); + + it('emits POINTS_AWARDED only after save resolves (Issue #1000)', async () => { + progressRepo.findOne.mockResolvedValue(null); + + // Make save() resolve asynchronously but controllably + let resolveSave!: (v: any) => void; + progressRepo.save.mockReturnValueOnce( + new Promise((r) => { + resolveSave = r; + }), + ); + + const addPromise = service.addPoints('user-1', 100, 'TEST'); + + // Event must NOT fire before save resolves + expect(emitter.emit).not.toHaveBeenCalled(); + + resolveSave({ totalPoints: 100, xp: 100, level: 1, tier: Tier.BRONZE }); + await addPromise; + + expect(emitter.emit).toHaveBeenCalledWith( + GAMIFICATION_EVENTS.POINTS_AWARDED, + expect.any(Object), + ); + }); + + it('does not emit if save rejects (Issue #1000)', async () => { + progressRepo.findOne.mockResolvedValue(null); + progressRepo.save.mockRejectedValueOnce(new Error('DB write failed')); + + await expect(service.addPoints('user-1', 100, 'TEST')).rejects.toThrow('DB write failed'); + + expect(emitter.emit).not.toHaveBeenCalled(); + }); + }); + + // ------------------------------------------------------------------------- + // Issue #1000 — tier boundary crossing test: promotion fires exactly once + // ------------------------------------------------------------------------- + + describe('tier boundary crossing across two awards', () => { + it('promoton event fires once and persisted tier is correct after crossing SILVER boundary', async () => { + // First award: stays in BRONZE (900 pts) + progressRepo.findOne.mockResolvedValueOnce(null); + const first = await service.addPoints('user-1', 900, 'TEST'); + + expect(first.tierPromoted).toBe(false); + expect(first.progress.tier).toBe(Tier.BRONZE); + + // Simulate what the DB would return on the next read — BRONZE with 900pts + const afterFirstSave = { ...progressRepo.save.mock.results[0].value }; + + // Second award: crosses into SILVER (900 + 200 = 1100) + progressRepo.findOne.mockResolvedValueOnce(afterFirstSave); + const second = await service.addPoints('user-1', 200, 'TEST'); + + expect(second.tierPromoted).toBe(true); + expect(second.progress.tier).toBe(Tier.SILVER); + + // The saved argument on the second save must carry SILVER + const secondSaveArg: Partial = + progressRepo.save.mock.calls[progressRepo.save.mock.calls.length - 1][0]; + expect(secondSaveArg.tier).toBe(Tier.SILVER); + + // Total POINTS_AWARDED emissions across both calls + const promotionEmits = emitter.emit.mock.calls.filter( + ([event]) => event === GAMIFICATION_EVENTS.POINTS_AWARDED, + ); + expect(promotionEmits).toHaveLength(2); // one per award, regardless of tier change + + // tierPromoted was false on first and true on second — exactly one boundary crossing + expect(first.tierPromoted).toBe(false); + expect(second.tierPromoted).toBe(true); + }); + + it('does not fire a second promotion when the same boundary is re-approached', async () => { + // User already at SILVER (1000 pts), award more without crossing GOLD (5000 pts) + const existing: Partial = { + totalPoints: 1000, + xp: 1000, + level: 2, + tier: Tier.SILVER, + }; + progressRepo.findOne.mockResolvedValue(existing); + + const { tierPromoted } = await service.addPoints('user-1', 100, 'TEST'); + + expect(tierPromoted).toBe(false); + }); }); + // ------------------------------------------------------------------------- + // awardActivity + // ------------------------------------------------------------------------- + describe('awardActivity', () => { it('uses POINT_RULES for known activity types', async () => { progressRepo.findOne.mockResolvedValue(null); @@ -110,6 +264,10 @@ describe('PointsService', () => { }); }); + // ------------------------------------------------------------------------- + // getUserProgress + // ------------------------------------------------------------------------- + describe('getUserProgress', () => { it('returns null when no progress exists', async () => { progressRepo.findOne.mockResolvedValue(null); diff --git a/src/gamification/points/points.service.ts b/src/gamification/points/points.service.ts index c2db0f09..77f630fc 100644 --- a/src/gamification/points/points.service.ts +++ b/src/gamification/points/points.service.ts @@ -8,6 +8,7 @@ import { User } from '../../users/entities/user.entity'; import { GAMIFICATION_EVENTS, PointsAwardedEvent } from '../events/gamification.events'; import { TiersService } from '../tiers/tiers.service'; import { PointActivityType, POINT_RULES } from '../enums/point-activity.enum'; +import { Tier } from '../enums/tier.enum'; // /** Points per XP level (level = floor(xp / XP_PER_LEVEL) + 1) */ // const XP_PER_LEVEL = 1000; @@ -38,6 +39,14 @@ export class PointsService { /** * Award an arbitrary number of points for a custom activity string. * Returns the updated progress and whether a tier promotion occurred. + * + * Issue #1000 — tier computation and persistence ordering fix: + * 1. `previousTier` is captured from the loaded entity before any mutation. + * 2. `newTier` is derived from the projected `totalPoints` BEFORE save, + * then assigned to `progress.tier` so a single `save()` call persists + * both the new point total and the correct tier atomically. + * 3. The POINTS_AWARDED event is emitted only after `save()` resolves, so + * a rolled-back or failed write never publishes a phantom promotion. */ async addPoints( userId: string, @@ -63,19 +72,27 @@ export class PointsService { }); } + // Capture the tier currently on the record before any mutation so we can + // detect a real boundary crossing after the save commits. + const previousTier = progress.tier ?? Tier.BRONZE; + progress.totalPoints += points; progress.xp += points; + progress.level = Math.floor(progress.xp / 1000) + 1; - const newLevel = Math.floor(progress.xp / 1000) + 1; - progress.level = newLevel; + // Derive the new tier from the projected total and assign it BEFORE save + // so the single repository call persists both points and tier together. + const newTier = this.tiersService.getTierForPoints(progress.totalPoints); + progress.tier = newTier; - const previousTier = progress.tier; const saved = await this.userProgressRepository.save(progress); - const newTier = this.tiersService.getTierForPoints(saved.totalPoints); - saved.tier = newTier; + + // tierPromoted is true only when the boundary is actually crossed and the + // value is now durable in the database. const tierPromoted = newTier !== previousTier; - // Emit event so BadgesService can react + // Emit only after the DB write succeeds so a save failure does not + // publish a promotion that never actually committed. this.eventEmitter.emit( GAMIFICATION_EVENTS.POINTS_AWARDED, new PointsAwardedEvent(userId, saved.totalPoints, saved.level), diff --git a/src/notifications/dlq/dlq.service.ts b/src/notifications/dlq/dlq.service.ts new file mode 100644 index 00000000..9c202ac0 --- /dev/null +++ b/src/notifications/dlq/dlq.service.ts @@ -0,0 +1,159 @@ +import { Injectable, Logger, NotFoundException, Optional } from '@nestjs/common'; +import { InjectRepository } from '@nestjs/typeorm'; +import { Repository } from 'typeorm'; +import { Counter, Registry } from 'prom-client'; +import { Notification, NotificationStatus } from '../entities/notification.entity'; +import { NotificationsQueueService } from '../notifications.queue'; +import { CustomMetricsService } from '../../monitoring/custom-metrics.service'; + +/** Prometheus metric name — matches the spec name `notification_dlq_total`. */ +export const NOTIFICATION_DLQ_METRIC = 'notification_dlq_total'; + +/** + * Alert threshold: fire an alert once the DLQ depth (cumulative entries) + * exceeds this value. Tune per deployment via the threshold on CustomMetricsService. + */ +const DLQ_ALERT_THRESHOLD = 5; + +/** Maximum delivery attempts before a notification is considered permanently failed. */ +export const MAX_DELIVERY_ATTEMPTS = 5; + +export interface DlqItem { + id: string; + userId: string; + type: string; + title: string; + failureReason: string | null; + deliveryAttempts: number; + createdAt: Date; + updatedAt: Date; + metadata: Record | null; +} + +/** + * DlqService + * + * Issue #830 — dead-letter queue handling for permanently failing notification + * jobs. The DLQ is modelled as a query over the existing `notifications` table + * for rows in `FAILED` status. No extra DB table is required. + * + * Responsibilities: + * - List all DLQ entries with full error context. + * - Reprocess individual entries by resetting their status to PENDING and + * re-publishing via NotificationsQueueService. + * - Increment `notification_dlq_total` (Prometheus Counter) each time a + * notification is permanently dead-lettered. + * - Expose a Gauge-threshold alert via CustomMetricsService so a Prometheus + * alert fires when DLQ depth exceeds the configured threshold. + */ +@Injectable() +export class DlqService { + private readonly logger = new Logger(DlqService.name); + + /** prom-client Counter registered on the shared registry. */ + private readonly dlqCounter: Counter; + + constructor( + @InjectRepository(Notification) + private readonly notificationRepository: Repository, + private readonly queueService: NotificationsQueueService, + @Optional() private readonly customMetrics?: CustomMetricsService, + @Optional() private readonly promRegistry?: Registry, + ) { + // Register the Prometheus counter on the provided registry (or default). + this.dlqCounter = new Counter({ + name: NOTIFICATION_DLQ_METRIC, + help: 'Total number of notification jobs permanently moved to the dead-letter queue', + labelNames: ['type'], + registers: this.promRegistry ? [this.promRegistry] : [], + }); + + // Register the alert definition in CustomMetricsService so an alert fires + // when cumulative DLQ entries breach the threshold (Issue #830). + this.customMetrics?.define({ + name: NOTIFICATION_DLQ_METRIC, + description: 'Notification jobs that exhausted all retries and landed in the DLQ', + type: 'counter', + alertThreshold: DLQ_ALERT_THRESHOLD, + }); + } + + /** + * List all permanently-failed notifications (the DLQ). + * Returns full error context so an admin can triage the issue. + */ + async listDlq(limit = 50, offset = 0): Promise<{ items: DlqItem[]; total: number }> { + const [notifications, total] = await this.notificationRepository.findAndCount({ + where: { status: NotificationStatus.FAILED }, + order: { updatedAt: 'DESC' }, + take: limit, + skip: offset, + }); + + const items: DlqItem[] = notifications.map((n) => ({ + id: n.id, + userId: n.userId, + type: n.type, + title: n.title, + failureReason: n.failureReason, + deliveryAttempts: n.deliveryAttempts, + createdAt: n.createdAt, + updatedAt: n.updatedAt, + metadata: n.metadata, + })); + + return { items, total }; + } + + /** + * Reprocess a single DLQ entry. + * Resets the notification to PENDING and re-publishes it via the queue. + * Returns the updated notification. + */ + async retryJob(notificationId: string): Promise { + const notification = await this.notificationRepository.findOne({ + where: { id: notificationId, status: NotificationStatus.FAILED }, + }); + + if (!notification) { + throw new NotFoundException( + `DLQ item ${notificationId} not found or is not in FAILED status`, + ); + } + + // Reset for reprocessing — clear the failure state and decrement attempts + // so the exponential backoff restarts from a clean state. + notification.status = NotificationStatus.PENDING; + notification.deliveryAttempts = 0; + notification.failureReason = null as any; + const reset = await this.notificationRepository.save(notification); + + this.logger.log(`DLQ item ${notificationId} reset to PENDING for reprocessing`); + + // Re-publish to the queue. Errors here propagate to the caller as 5xx. + await this.queueService.publishToTopic(reset); + + this.logger.log(`DLQ item ${notificationId} re-queued successfully`); + return reset; + } + + /** + * Record that a notification has been permanently dead-lettered. + * Called by NotificationsQueueService after max retries are exhausted. + * + * Increments the Prometheus counter and the CustomMetrics gauge so alerting + * fires when the DLQ depth breaches the threshold (Issue #830). + */ + recordDlqEntry(notification: Notification): void { + // prom-client counter — scraped by Prometheus at /metrics. + this.dlqCounter.inc({ type: notification.type }); + + // CustomMetricsService increment — triggers AlertingService threshold check. + this.customMetrics?.increment(NOTIFICATION_DLQ_METRIC, 1, { type: notification.type }); + + this.logger.warn( + `Notification ${notification.id} (type=${notification.type}) permanently dead-lettered ` + + `after ${notification.deliveryAttempts} attempts`, + ); + } +} diff --git a/src/payments/payments.module.ts b/src/payments/payments.module.ts index 838e5b94..72732735 100644 --- a/src/payments/payments.module.ts +++ b/src/payments/payments.module.ts @@ -12,6 +12,9 @@ import { PricingService } from './services/pricing.service'; import { PricingController } from './controllers/pricing.controller'; import { PaymentReconciliationJob } from './reconciliation/reconciliation.service'; import { PaymentReconciliationController } from './reconciliation/reconciliation.controller'; +import { SubscriptionsService } from './subscriptions/subscriptions.service'; +import { SubscriptionsController } from './subscriptions/subscriptions.controller'; +import { PaymentProviderService } from './providers/payment-provider.service'; /** * PaymentsModule @@ -24,6 +27,10 @@ import { PaymentReconciliationController } from './reconciliation/reconciliation * * Issue #856 — imports AuditLogModule so PaymentReconciliationJob can log * PAYMENT_RECONCILIATION_MISMATCH audit events. + * + * Issue #1007 — registers SubscriptionsService, SubscriptionsController, and + * PaymentProviderService so that prorated upgrade charges and downgrade credits + * are wired into the DI container. */ @Module({ imports: [ @@ -33,8 +40,8 @@ import { PaymentReconciliationController } from './reconciliation/reconciliation IdempotencyModule, HttpModule, ], - providers: [PricingService, PaymentReconciliationJob], - controllers: [PricingController, PaymentReconciliationController], - exports: [PricingService, CurrencyModule, IdempotencyModule, PaymentReconciliationJob], + providers: [PricingService, PaymentReconciliationJob, SubscriptionsService, PaymentProviderService], + controllers: [PricingController, PaymentReconciliationController, SubscriptionsController], + exports: [PricingService, CurrencyModule, IdempotencyModule, PaymentReconciliationJob, SubscriptionsService, PaymentProviderService], }) export class PaymentsModule {} diff --git a/src/payments/providers/payment-provider.service.ts b/src/payments/providers/payment-provider.service.ts new file mode 100644 index 00000000..ec21c9dc --- /dev/null +++ b/src/payments/providers/payment-provider.service.ts @@ -0,0 +1,72 @@ +import { Injectable, Logger } from '@nestjs/common'; + +export interface ChargeResult { + chargeId: string; + status: 'succeeded' | 'failed'; + amount: number; + currency: string; +} + +export interface CreditResult { + creditId: string; + status: 'applied' | 'scheduled'; + amount: number; + currency: string; +} + +/** + * PaymentProviderService + * + * Thin abstraction over the payment provider (Stripe / PayPal / etc.). + * The concrete implementation should call the real provider SDK; this + * default implementation throws so that tests must supply a mock and + * production deployments must configure a real provider. + * + * Issue #1007 — extracted so SubscriptionsService can inject it and + * tests can mock it without touching the real provider. + */ +@Injectable() +export class PaymentProviderService { + private readonly logger = new Logger(PaymentProviderService.name); + + /** + * Charge the customer's default payment method for `amount` (in the + * subscription's currency). Returns the provider's charge/payment-intent + * ID on success, or throws on failure. + */ + async chargeCustomer( + userId: string, + amount: number, + currency: string, + metadata: Record = {}, + ): Promise { + // TODO: replace with real Stripe call: + // const intent = await stripe.paymentIntents.create({ amount: Math.round(amount * 100), currency, ... }); + this.logger.warn( + `chargeCustomer called for user ${userId} amount=${amount} — no real provider configured`, + ); + throw new Error( + 'No payment provider configured. Inject a concrete PaymentProviderService implementation.', + ); + } + + /** + * Issue a credit (balance adjustment or refund) to the customer. + * Returns the provider credit/balance-transaction ID. + */ + async issueCredit( + userId: string, + amount: number, + currency: string, + metadata: Record = {}, + ): Promise { + // TODO: replace with real Stripe call: + // const credit = await stripe.customers.createBalanceTransaction(customerId, { amount: -Math.round(amount * 100), currency }); + this.logger.warn( + `issueCredit called for user ${userId} amount=${amount} — no real provider configured`, + ); + throw new Error( + 'No payment provider configured. Inject a concrete PaymentProviderService implementation.', + ); + } +} diff --git a/src/payments/subscriptions/subscriptions.service.spec.ts b/src/payments/subscriptions/subscriptions.service.spec.ts new file mode 100644 index 00000000..11f5e6ea --- /dev/null +++ b/src/payments/subscriptions/subscriptions.service.spec.ts @@ -0,0 +1,308 @@ +import { BadRequestException, NotFoundException, PaymentRequiredException } from '@nestjs/common'; +import { SubscriptionsService } from './subscriptions.service'; +import { + Subscription, + SubscriptionStatus, + SubscriptionInterval, +} from '../entities/subscription.entity'; +import { PaymentProviderService } from '../providers/payment-provider.service'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +const PERIOD_END = new Date(Date.now() + 15 * 24 * 60 * 60 * 1000); // 15 days from now + +function makeSubscription(overrides: Partial = {}): Subscription { + return { + id: 'sub-1', + userId: 'user-1', + status: SubscriptionStatus.ACTIVE, + interval: SubscriptionInterval.MONTHLY, + amount: 9.99, // plan-basic monthly + currency: 'USD', + currentPeriodStart: new Date(Date.now() - 15 * 24 * 60 * 60 * 1000), + currentPeriodEnd: PERIOD_END, + cancelAtPeriodEnd: false, + cancelledAt: null as any, + trialStart: null as any, + trialEnd: null as any, + properties: {}, + providerSubscriptionId: 'prov-sub-1', + version: 1, + user: {} as any, + createdAt: new Date(), + updatedAt: new Date(), + deletedAt: undefined, + ...overrides, + } as Subscription; +} + +function makeRepo(subscription: Subscription) { + return { + findOne: jest.fn().mockResolvedValue(subscription), + save: jest.fn().mockImplementation(async (s: Subscription) => ({ ...s })), + }; +} + +function makeEventEmitter() { + return { emit: jest.fn() }; +} + +function makeProvider() { + return { + chargeCustomer: jest.fn(), + issueCredit: jest.fn(), + } as unknown as jest.Mocked; +} + +function buildService( + repo: ReturnType, + eventEmitter: ReturnType, + provider: jest.Mocked, +): SubscriptionsService { + return new SubscriptionsService(repo as any, eventEmitter as any, provider); +} + +// --------------------------------------------------------------------------- +// upgradeSubscription +// --------------------------------------------------------------------------- + +describe('SubscriptionsService.upgradeSubscription', () => { + it('charges the prorated difference and saves the new plan on success', async () => { + const subscription = makeSubscription(); + const repo = makeRepo(subscription); + const emitter = makeEventEmitter(); + const provider = makeProvider(); + + provider.chargeCustomer.mockResolvedValue({ + chargeId: 'ch_test_123', + status: 'succeeded', + amount: expect.any(Number), + currency: 'USD', + }); + + const service = buildService(repo, emitter, provider); + + const result = await service.upgradeSubscription('sub-1', { + planId: 'plan-pro', // $19.99 > $9.99 — valid upgrade + }); + + // Charge must happen + expect(provider.chargeCustomer).toHaveBeenCalledTimes(1); + const [userId, amount, currency, meta] = provider.chargeCustomer.mock.calls[0]; + expect(userId).toBe('user-1'); + expect(amount).toBeGreaterThan(0); + expect(currency).toBe('USD'); + expect(meta).toMatchObject({ subscriptionId: 'sub-1', type: 'subscription_upgrade_proration' }); + + // Plan change persisted + expect(repo.save).toHaveBeenCalledTimes(1); + const saved: Subscription = repo.save.mock.calls[0][0]; + expect(saved.amount).toBe(19.99); + expect(saved.properties?.upgradeChargeId).toBe('ch_test_123'); + expect(saved.properties?.proratedAmount).toBeGreaterThan(0); + + // Event emitted with chargeId + expect(emitter.emit).toHaveBeenCalledWith( + 'subscription.upgraded', + expect.objectContaining({ chargeId: 'ch_test_123', planId: 'plan-pro' }), + ); + + // Returned subscription reflects new amount + expect(result.amount).toBe(19.99); + }); + + it('leaves the subscription on the original plan when the charge fails', async () => { + const subscription = makeSubscription(); + const repo = makeRepo(subscription); + const emitter = makeEventEmitter(); + const provider = makeProvider(); + + provider.chargeCustomer.mockRejectedValue(new Error('Card declined')); + + const service = buildService(repo, emitter, provider); + + await expect( + service.upgradeSubscription('sub-1', { planId: 'plan-pro' }), + ).rejects.toBeInstanceOf(PaymentRequiredException); + + // Subscription must NOT be saved after a failed charge + expect(repo.save).not.toHaveBeenCalled(); + + // No event should be emitted for a failed upgrade + expect(emitter.emit).not.toHaveBeenCalledWith('subscription.upgraded', expect.anything()); + }); + + it('throws BadRequestException if new plan is not more expensive', async () => { + const subscription = makeSubscription({ amount: 19.99 }); // already pro + const repo = makeRepo(subscription); + const provider = makeProvider(); + + const service = buildService(repo, makeEventEmitter(), provider); + + await expect( + service.upgradeSubscription('sub-1', { planId: 'plan-basic' }), + ).rejects.toBeInstanceOf(BadRequestException); + + expect(provider.chargeCustomer).not.toHaveBeenCalled(); + expect(repo.save).not.toHaveBeenCalled(); + }); + + it('throws NotFoundException when subscription does not exist', async () => { + const repo = { findOne: jest.fn().mockResolvedValue(null), save: jest.fn() }; + const service = buildService(repo as any, makeEventEmitter(), makeProvider()); + + await expect( + service.upgradeSubscription('sub-missing', { planId: 'plan-pro' }), + ).rejects.toBeInstanceOf(NotFoundException); + }); +}); + +// --------------------------------------------------------------------------- +// downgradeSubscription — immediate credit +// --------------------------------------------------------------------------- + +describe('SubscriptionsService.downgradeSubscription (credit)', () => { + it('issues a prorated credit and saves the lower plan immediately', async () => { + const subscription = makeSubscription({ amount: 19.99 }); // plan-pro + const repo = makeRepo(subscription); + const emitter = makeEventEmitter(); + const provider = makeProvider(); + + provider.issueCredit.mockResolvedValue({ + creditId: 'cre_test_456', + status: 'applied', + amount: expect.any(Number), + currency: 'USD', + }); + + const service = buildService(repo, emitter, provider); + + const result = await service.downgradeSubscription('sub-1', { + planId: 'plan-basic', // $9.99 < $19.99 — valid downgrade + prorationType: 'credit', + }); + + // Credit must be issued + expect(provider.issueCredit).toHaveBeenCalledTimes(1); + const [userId, amount, currency, meta] = provider.issueCredit.mock.calls[0]; + expect(userId).toBe('user-1'); + expect(amount).toBeGreaterThan(0); + expect(currency).toBe('USD'); + expect(meta).toMatchObject({ + subscriptionId: 'sub-1', + type: 'subscription_downgrade_proration', + }); + + // Plan change persisted + expect(repo.save).toHaveBeenCalledTimes(1); + const saved: Subscription = repo.save.mock.calls[0][0]; + expect(saved.amount).toBe(9.99); + expect(saved.properties?.downgradeCreditId).toBe('cre_test_456'); + expect(saved.properties?.proratedCredit).toBeGreaterThan(0); + + // Event emitted with creditId + expect(emitter.emit).toHaveBeenCalledWith( + 'subscription.downgraded', + expect.objectContaining({ + creditId: 'cre_test_456', + deferred: false, + prorationType: 'credit', + }), + ); + + expect(result.amount).toBe(9.99); + }); + + it('leaves the subscription unchanged when credit issuance fails', async () => { + const subscription = makeSubscription({ amount: 19.99 }); + const repo = makeRepo(subscription); + const emitter = makeEventEmitter(); + const provider = makeProvider(); + + provider.issueCredit.mockRejectedValue(new Error('Provider unavailable')); + + const service = buildService(repo, emitter, provider); + + await expect( + service.downgradeSubscription('sub-1', { planId: 'plan-basic', prorationType: 'credit' }), + ).rejects.toBeInstanceOf(BadRequestException); + + // Subscription must NOT be saved after a failed credit + expect(repo.save).not.toHaveBeenCalled(); + expect(emitter.emit).not.toHaveBeenCalledWith('subscription.downgraded', expect.anything()); + }); +}); + +// --------------------------------------------------------------------------- +// downgradeSubscription — deferred (prorationType: 'none') +// --------------------------------------------------------------------------- + +describe('SubscriptionsService.downgradeSubscription (deferred, prorationType=none)', () => { + it('records a pendingDowngrade and defers the plan change without issuing a credit', async () => { + const subscription = makeSubscription({ amount: 19.99 }); + const repo = makeRepo(subscription); + const emitter = makeEventEmitter(); + const provider = makeProvider(); + + const service = buildService(repo, emitter, provider); + + const result = await service.downgradeSubscription('sub-1', { + planId: 'plan-basic', + prorationType: 'none', + }); + + // No provider call should occur for deferred downgrades + expect(provider.issueCredit).not.toHaveBeenCalled(); + expect(provider.chargeCustomer).not.toHaveBeenCalled(); + + // Subscription saved with pending downgrade metadata + expect(repo.save).toHaveBeenCalledTimes(1); + const saved: Subscription = repo.save.mock.calls[0][0]; + expect(saved.amount).toBe(19.99); // amount unchanged until period end + expect(saved.properties?.pendingDowngrade).toMatchObject({ + planId: 'plan-basic', + amount: 9.99, + }); + expect(saved.properties?.pendingDowngrade?.effectiveAt).toEqual(PERIOD_END); + expect(saved.properties?.prorationType).toBe('none'); + + // Event indicates deferred change + expect(emitter.emit).toHaveBeenCalledWith( + 'subscription.downgraded', + expect.objectContaining({ + deferred: true, + prorationType: 'none', + effectiveAt: PERIOD_END, + }), + ); + + // Plan amount is still the original — not yet changed + expect(result.amount).toBe(19.99); + }); + + it('defaults prorationType to "credit" when not supplied', async () => { + const subscription = makeSubscription({ amount: 19.99 }); + const repo = makeRepo(subscription); + const emitter = makeEventEmitter(); + const provider = makeProvider(); + + provider.issueCredit.mockResolvedValue({ + creditId: 'cre_default', + status: 'applied', + amount: 5, + currency: 'USD', + }); + + const service = buildService(repo, emitter, provider); + + // prorationType not supplied → should default to 'credit' + await service.downgradeSubscription('sub-1', { planId: 'plan-basic' }); + + expect(provider.issueCredit).toHaveBeenCalledTimes(1); + expect(repo.save).toHaveBeenCalledTimes(1); + const saved: Subscription = repo.save.mock.calls[0][0]; + expect(saved.amount).toBe(9.99); + }); +}); diff --git a/src/payments/subscriptions/subscriptions.service.ts b/src/payments/subscriptions/subscriptions.service.ts index a49a803a..57fb7bd4 100644 --- a/src/payments/subscriptions/subscriptions.service.ts +++ b/src/payments/subscriptions/subscriptions.service.ts @@ -1,4 +1,10 @@ -import { Injectable, Logger, BadRequestException, NotFoundException } from '@nestjs/common'; +import { + Injectable, + Logger, + BadRequestException, + NotFoundException, + PaymentRequiredException, +} from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { @@ -13,9 +19,18 @@ import { UpgradeSubscriptionDto, DowngradeSubscriptionDto, } from './dto/subscription-action.dto'; +import { PaymentProviderService } from '../providers/payment-provider.service'; /** - * Handles subscription lifecycle management including pause, resume, upgrade, downgrade + * Handles subscription lifecycle management including pause, resume, upgrade, downgrade. + * + * Issue #1007 — upgradeSubscription and downgradeSubscription now: + * 1. Compute the prorated amount/credit BEFORE mutating the subscription. + * 2. Attempt the charge or credit via PaymentProviderService. + * 3. Only persist the plan change after the payment succeeds. + * 4. Leave the subscription on its original plan if the charge fails. + * 5. Record the provider chargeId / creditId on the subscription for reconciliation. + * 6. Honour prorationType='none' by deferring the plan change to currentPeriodEnd. */ @Injectable() export class SubscriptionsService { @@ -25,6 +40,7 @@ export class SubscriptionsService { @InjectRepository(Subscription) private subscriptionRepository: Repository, private eventEmitter: EventEmitter2, + private paymentProviderService: PaymentProviderService, ) {} /** @@ -68,7 +84,6 @@ export class SubscriptionsService { ); } - // Update subscription with pause metadata without canceling the subscription. subscription.properties = { ...subscription.properties, pausedAt: new Date(), @@ -79,7 +94,6 @@ export class SubscriptionsService { const updated = await this.subscriptionRepository.save(subscription); - // Emit event for downstream processing (notify user, analytics, etc.) this.eventEmitter.emit('subscription.paused', { subscriptionId: updated.id, userId: updated.userId, @@ -105,7 +119,6 @@ export class SubscriptionsService { throw new BadRequestException('Subscription is not paused'); } - // Update subscription to active subscription.status = SubscriptionStatus.ACTIVE; subscription.cancelAtPeriodEnd = false; subscription.properties = { @@ -117,7 +130,6 @@ export class SubscriptionsService { const updated = await this.subscriptionRepository.save(subscription); - // Emit event for downstream processing this.eventEmitter.emit('subscription.resumed', { subscriptionId: updated.id, userId: updated.userId, @@ -130,7 +142,14 @@ export class SubscriptionsService { } /** - * Upgrade subscription to a different plan + * Upgrade subscription to a higher-priced plan. + * + * Order of operations (Issue #1007): + * 1. Validate state. + * 2. Compute proratedAmount (net charge = new prorated charge − old prorated credit). + * 3. Charge the customer via the payment provider. + * 4. Only if the charge succeeds: update the plan and persist. + * 5. On charge failure: leave subscription unchanged and rethrow. */ async upgradeSubscription( subscriptionId: string, @@ -153,14 +172,41 @@ export class SubscriptionsService { ); } - // Calculate prorated amount + // Compute proration — do this before any mutation so a failed charge + // leaves the subscription record untouched. const daysRemaining = this.calculateDaysRemaining(subscription.currentPeriodEnd); const totalDaysInPeriod = this.calculateDaysInPeriod(subscription.interval); const proratedCredit = (oldAmount * daysRemaining) / totalDaysInPeriod; const proratedCharge = (newAmount * daysRemaining) / totalDaysInPeriod; - const proratedAmount = proratedCharge - proratedCredit; + const proratedAmount = Number((proratedCharge - proratedCredit).toFixed(2)); + + // Attempt the charge BEFORE mutating the subscription (Issue #1007). + let chargeId: string; + try { + const chargeResult = await this.paymentProviderService.chargeCustomer( + subscription.userId, + proratedAmount, + subscription.currency, + { + subscriptionId, + oldPlanAmount: oldAmount, + newPlanId: dto.planId, + type: 'subscription_upgrade_proration', + }, + ); + chargeId = chargeResult.chargeId; + } catch (err) { + // Charge failed — subscription is NOT mutated. Propagate so the + // controller returns 402 / 400 and the DB record stays on the old plan. + this.logger.warn( + `Prorated upgrade charge failed for subscription ${subscriptionId}: ${(err as Error).message}`, + ); + throw new PaymentRequiredException( + `Prorated charge of ${proratedAmount} ${subscription.currency} failed: ${(err as Error).message}`, + ); + } - // Update subscription + // Payment confirmed — now update the subscription. subscription.amount = newAmount; subscription.interval = dto.billingCycle ? (dto.billingCycle as SubscriptionInterval) @@ -172,29 +218,39 @@ export class SubscriptionsService { proratedAmount, proratedCredit, proratedCharge, + // Record the provider charge ID for reconciliation (Issue #1007). + upgradeChargeId: chargeId, }; const updated = await this.subscriptionRepository.save(subscription); - // Emit event for payment processing this.eventEmitter.emit('subscription.upgraded', { subscriptionId: updated.id, userId: updated.userId, oldAmount, newAmount, proratedAmount, + chargeId, planId: dto.planId, }); this.logger.log( - `Subscription ${subscriptionId} upgraded from $${oldAmount} to $${newAmount} (prorated: $${proratedAmount})`, + `Subscription ${subscriptionId} upgraded from $${oldAmount} to $${newAmount} ` + + `(prorated charge: $${proratedAmount}, chargeId: ${chargeId})`, ); return updated; } /** - * Downgrade subscription to a different plan + * Downgrade subscription to a lower-priced plan. + * + * prorationType controls behaviour (Issue #1007): + * - 'credit' (default) — issue a prorated credit immediately, then apply the lower plan now. + * - 'none' — defer the plan change to currentPeriodEnd (no credit issued now). + * + * In both cases the subscription record is only mutated after the provider + * call returns successfully. */ async downgradeSubscription( subscriptionId: string, @@ -217,15 +273,81 @@ export class SubscriptionsService { ); } - // Calculate prorated credit based on prorationType + const prorationType = dto.prorationType ?? 'credit'; + + // 'none' — defer the plan change to the end of the current period. + // No charge or credit is issued now; the actual plan switch will be + // handled when the subscription renews. + if (prorationType === 'none') { + subscription.cancelAtPeriodEnd = false; + subscription.properties = { + ...subscription.properties, + pendingDowngrade: { + planId: dto.planId, + amount: newAmount, + billingCycle: dto.billingCycle, + scheduledAt: new Date(), + effectiveAt: subscription.currentPeriodEnd, + }, + downgradedFrom: { planId: subscription.properties?.planId, amount: oldAmount }, + prorationType, + }; + + const updated = await this.subscriptionRepository.save(subscription); + + this.eventEmitter.emit('subscription.downgraded', { + subscriptionId: updated.id, + userId: updated.userId, + oldAmount, + newAmount, + prorationType, + deferred: true, + effectiveAt: subscription.currentPeriodEnd, + planId: dto.planId, + }); + + this.logger.log( + `Subscription ${subscriptionId} downgrade deferred to ${subscription.currentPeriodEnd.toISOString()} ` + + `(new plan: ${dto.planId}, prorationType: none)`, + ); + + return updated; + } + + // 'credit' (or any other value) — issue the prorated credit immediately + // and apply the lower plan now. const daysRemaining = this.calculateDaysRemaining(subscription.currentPeriodEnd); const totalDaysInPeriod = this.calculateDaysInPeriod(subscription.interval); - const proratedCharge = (newAmount * daysRemaining) / totalDaysInPeriod; const oldProratedCharge = (oldAmount * daysRemaining) / totalDaysInPeriod; - const proratedCredit = oldProratedCharge - proratedCharge; - const prorationType = dto.prorationType || 'credit'; + const newProratedCharge = (newAmount * daysRemaining) / totalDaysInPeriod; + const proratedCredit = Number((oldProratedCharge - newProratedCharge).toFixed(2)); + + // Issue the credit BEFORE mutating the subscription (Issue #1007). + let creditId: string; + try { + const creditResult = await this.paymentProviderService.issueCredit( + subscription.userId, + proratedCredit, + subscription.currency, + { + subscriptionId, + oldPlanAmount: oldAmount, + newPlanId: dto.planId, + type: 'subscription_downgrade_proration', + }, + ); + creditId = creditResult.creditId; + } catch (err) { + // Credit issuance failed — subscription is NOT mutated. + this.logger.warn( + `Prorated credit issuance failed for subscription ${subscriptionId}: ${(err as Error).message}`, + ); + throw new BadRequestException( + `Failed to issue prorated credit of ${proratedCredit} ${subscription.currency}: ${(err as Error).message}`, + ); + } - // Update subscription + // Credit confirmed — now apply the lower plan. subscription.amount = newAmount; subscription.interval = dto.billingCycle ? (dto.billingCycle as SubscriptionInterval) @@ -236,24 +358,27 @@ export class SubscriptionsService { downgradedAt: new Date(), prorationType, proratedCredit, - proratedCharge, + // Record the provider credit ID for reconciliation (Issue #1007). + downgradeCreditId: creditId, }; const updated = await this.subscriptionRepository.save(subscription); - // Emit event for payment/credit processing this.eventEmitter.emit('subscription.downgraded', { subscriptionId: updated.id, userId: updated.userId, oldAmount, newAmount, proratedCredit, + creditId, prorationType, + deferred: false, planId: dto.planId, }); this.logger.log( - `Subscription ${subscriptionId} downgraded from $${oldAmount} to $${newAmount} (credit: $${proratedCredit})`, + `Subscription ${subscriptionId} downgraded from $${oldAmount} to $${newAmount} ` + + `(prorated credit: $${proratedCredit}, creditId: ${creditId})`, ); return updated; @@ -308,7 +433,6 @@ export class SubscriptionsService { `Attempting renewal for subscription ${subscriptionId} (attempt ${attempt}/${maxRetries})`, ); - // Emit event for payment processor to handle this.eventEmitter.emit('subscription.renewal_attempt', { subscriptionId, userId: subscription.userId, @@ -345,7 +469,6 @@ export class SubscriptionsService { ); if (attempt === maxRetries) { - // Mark as past due after all retries exhausted subscription.status = SubscriptionStatus.PAST_DUE; subscription.properties = { ...subscription.properties, @@ -365,7 +488,6 @@ export class SubscriptionsService { return false; } - // Exponential backoff before next attempt const backoffMs = Math.pow(2, attempt - 1) * 1000; await new Promise((resolve) => setTimeout(resolve, backoffMs)); } @@ -387,9 +509,12 @@ export class SubscriptionsService { }, delayMs); } + // --------------------------------------------------------------------------- // Helper methods + // --------------------------------------------------------------------------- + private async getNewPlanAmount(planId: string, billingCycle?: string): Promise { - // For now, use an in-app plan price map; replace with a plan service or database lookup when available. + // Static plan price map — replace with a PlanService / DB lookup when available. const planPrices: Record = { 'plan-basic': 9.99, 'plan-pro': 19.99, @@ -439,7 +564,6 @@ export class SubscriptionsService { * Legacy placeholder - for backward compatibility */ async processSubscription(): Promise { - // Logic to process subscription payments return { success: true }; } } diff --git a/src/search/search.service.spec.ts b/src/search/search.service.spec.ts index 61cd7f59..2c6dbe15 100644 --- a/src/search/search.service.spec.ts +++ b/src/search/search.service.spec.ts @@ -1,3 +1,4 @@ +import { BadRequestException } from '@nestjs/common'; import { SearchService } from './search.service'; import { Repository } from 'typeorm'; import { ElasticsearchService } from '@nestjs/elasticsearch'; @@ -219,3 +220,466 @@ describe('SearchService (Issue #814 full-text search)', () => { }); }); }); + +// --------------------------------------------------------------------------- +// Issue #999 — getAvailableFilters derives facets from real data +// --------------------------------------------------------------------------- + +describe('SearchService.getAvailableFilters (Issue #999)', () => { + let service: SearchService; + let courseRepository: jest.Mocked>; + + /** Build a fluent query-builder mock whose getRawMany returns `rows`. */ + function makeAggQb(rows: any[]) { + return { + select: jest.fn().mockReturnThis(), + addSelect: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + groupBy: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + limit: jest.fn().mockReturnThis(), + getRawMany: jest.fn().mockResolvedValue(rows), + }; + } + + beforeEach(() => { + courseRepository = { createQueryBuilder: jest.fn() } as any; + service = new SearchService( + courseRepository as any, + { search: jest.fn() } as any, + undefined, + undefined as any, + ); + }); + + it('returns facet values and counts derived from published courses', async () => { + const aggQb = makeAggQb([ + { category: 'programming', count: '12' }, + { category: 'design', count: '4' }, + ]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + const result = await service.getAvailableFilters(); + + // Category facets reflect DB data + expect(result.categories).toEqual([ + { value: 'programming', count: 12 }, + { value: 'design', count: 4 }, + ]); + + // levels and languages are empty arrays until those columns are added + expect(result.levels).toEqual([]); + expect(result.languages).toEqual([]); + }); + + it('filters only published courses in the aggregation query', async () => { + const aggQb = makeAggQb([]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + await service.getAvailableFilters(); + + expect(aggQb.where).toHaveBeenCalledWith('course.status = :status', { + status: 'published', + }); + }); + + it('omits null and empty-string category values', async () => { + const aggQb = makeAggQb([]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + await service.getAvailableFilters(); + + expect(aggQb.andWhere).toHaveBeenCalledWith('course.category IS NOT NULL'); + expect(aggQb.andWhere).toHaveBeenCalledWith("course.category <> ''"); + }); + + it('returns cached result on repeated calls within the TTL window', async () => { + const cacheStore = new Map(); + const cacheManager = { + get: jest.fn((key: string) => Promise.resolve(cacheStore.get(key))), + set: jest.fn((key: string, val: any) => { + cacheStore.set(key, val); + return Promise.resolve(); + }), + del: jest.fn((key: string) => { + cacheStore.delete(key); + return Promise.resolve(); + }), + } as any; + + service = new SearchService( + courseRepository as any, + { search: jest.fn() } as any, + undefined, + cacheManager, + ); + + const aggQb = makeAggQb([{ category: 'programming', count: '5' }]); + courseRepository.createQueryBuilder.mockReturnValue(aggQb as any); + + // First call: cache miss — hits DB + await service.getAvailableFilters(); + expect(courseRepository.createQueryBuilder).toHaveBeenCalledTimes(1); + + // Second call: cache hit — does NOT hit DB again + await service.getAvailableFilters(); + expect(courseRepository.createQueryBuilder).toHaveBeenCalledTimes(1); + }); + + it('invalidates the facets cache when a course is created or updated', async () => { + const cacheStore = new Map(); + const cacheManager = { + get: jest.fn((key: string) => Promise.resolve(cacheStore.get(key))), + set: jest.fn((key: string, val: any) => { + cacheStore.set(key, val); + return Promise.resolve(); + }), + del: jest.fn((key: string) => { + cacheStore.delete(key); + return Promise.resolve(); + }), + } as any; + + service = new SearchService( + courseRepository as any, + { search: jest.fn() } as any, + undefined, + cacheManager, + ); + + // Warm the cache + const aggQb = makeAggQb([{ category: 'programming', count: '5' }]); + courseRepository.createQueryBuilder.mockReturnValue(aggQb as any); + await service.getAvailableFilters(); + expect(cacheManager.set).toHaveBeenCalledTimes(1); + + // Simulate a course creation event + await (service as any).onCourseChanged({ id: 'course-new' }); + + expect(cacheManager.del).toHaveBeenCalledWith(expect.stringContaining('facets')); + + // Next request re-runs the aggregation + await service.getAvailableFilters(); + expect(courseRepository.createQueryBuilder).toHaveBeenCalledTimes(2); + }); + + it('returns empty arrays and logs error when aggregation fails', async () => { + courseRepository.createQueryBuilder.mockImplementationOnce(() => { + throw new Error('DB connection lost'); + }); + + const result = await service.getAvailableFilters(); + + expect(result).toEqual({ categories: [], levels: [], languages: [] }); + }); +}); + +// --------------------------------------------------------------------------- +// Issue #999 — validateFilters rejects unknown category values +// --------------------------------------------------------------------------- + +describe('SearchService.validateFilters (Issue #999)', () => { + let service: SearchService; + let courseRepository: jest.Mocked>; + + function makeAggQb(rows: any[]) { + return { + select: jest.fn().mockReturnThis(), + addSelect: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + groupBy: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + limit: jest.fn().mockReturnThis(), + getRawMany: jest.fn().mockResolvedValue(rows), + }; + } + + beforeEach(() => { + courseRepository = { createQueryBuilder: jest.fn() } as any; + service = new SearchService( + courseRepository as any, + { search: jest.fn() } as any, + undefined, + undefined as any, + ); + }); + + it('passes when the category exists in the live facet set', async () => { + const aggQb = makeAggQb([{ category: 'programming', count: '5' }]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + await expect( + service.validateFilters({ category: 'programming' }), + ).resolves.toBeUndefined(); + }); + + it('passes when an array of categories are all valid', async () => { + const aggQb = makeAggQb([ + { category: 'programming', count: '5' }, + { category: 'design', count: '3' }, + ]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + await expect( + service.validateFilters({ category: ['programming', 'design'] }), + ).resolves.toBeUndefined(); + }); + + it('throws BadRequestException for an unknown category string', async () => { + const aggQb = makeAggQb([{ category: 'programming', count: '5' }]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + await expect( + service.validateFilters({ category: 'web-development' }), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('throws BadRequestException listing all unknown values in the message', async () => { + const aggQb = makeAggQb([{ category: 'programming', count: '5' }]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + const err = await service + .validateFilters({ category: ['unknown-a', 'unknown-b'] }) + .catch((e) => e); + + expect(err).toBeInstanceOf(BadRequestException); + expect((err as BadRequestException).message).toContain('unknown-a'); + expect((err as BadRequestException).message).toContain('unknown-b'); + }); + + it('passes when no category filter is supplied', async () => { + await expect(service.validateFilters({})).resolves.toBeUndefined(); + // No DB query should be run + expect(courseRepository.createQueryBuilder).not.toHaveBeenCalled(); + }); + + it('search() returns 400 when a filter with an unknown category is passed', async () => { + // Facet agg returns only 'programming' + const aggQb = makeAggQb([{ category: 'programming', count: '5' }]); + courseRepository.createQueryBuilder.mockReturnValueOnce(aggQb as any); + + await expect( + service.search('', { category: 'nonexistent' }), + ).rejects.toBeInstanceOf(BadRequestException); + }); +}); + +/** + * Issue #814 — verifies the SearchService constructs the new `to_tsquery` + * parameter correctly, falls back gracefully when Elasticsearch is unavailable, + * and sanitizes tsquery metacharacters in autocomplete. + */ +describe('SearchService (Issue #814 full-text search)', () => { + let service: SearchService; + let courseRepository: jest.Mocked>; + let elasticsearch: { search: jest.Mock }; + let isolationService: { getTenantId: jest.Mock }; + + beforeEach(() => { + courseRepository = { + createQueryBuilder: jest.fn(), + } as any; + + elasticsearch = { search: jest.fn() }; + isolationService = { getTenantId: jest.fn().mockReturnValue(null) }; + + service = new SearchService( + courseRepository as any, + elasticsearch as any, + isolationService as any, + undefined as any, + ); + }); + + function makeQb(result: { rows: any[]; total: number }) { + const qb: any = { + addSelect: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + orWhere: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + addOrderBy: jest.fn().mockReturnThis(), + skip: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + getManyAndCount: jest.fn().mockResolvedValue([result.rows, result.total]), + }; + return qb; + } + + it('uses tsvector ranking when a query is supplied and DB is the source', async () => { + // Simulate ES unavailable so the DB path is taken. + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [{ id: 'c1', title: 'Hooks' }], total: 1 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search('react hooks'); + + // The call to qb.where(...) is a Brackets instance wrapping a nested + // callback. Verify it's a single argument and that the inner callback + // drove both the tsquery match and the ILIKE fallback. + expect(qb.where).toHaveBeenCalledTimes(1); + const bracketsArg = qb.where.mock.calls[0][0]; + expect(typeof bracketsArg).toBe('object'); + expect(typeof bracketsArg.whereFactory).toBe('function'); + + // Simulate the bracket factory invocation to inspect what it produces. + const innerQb = { + where: jest.fn().mockReturnThis(), + orWhere: jest.fn().mockReturnThis(), + }; + bracketsArg.whereFactory(innerQb); + expect(innerQb.where).toHaveBeenCalledWith( + expect.stringContaining('search_vector @@ plainto_tsquery'), + { query: 'react hooks' }, + ); + expect(innerQb.orWhere).toHaveBeenCalledWith(expect.stringContaining('title ILIKE'), { + fallback: expect.stringContaining('react'), + }); + + // When relevance ordering is in play, qb.orderBy is called with 'rank'. + expect(qb.orderBy).toHaveBeenCalledWith('rank', 'DESC'); + }); + + it('falls back to createdAt ordering when no query is supplied', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.search(''); + + expect(qb.where).not.toHaveBeenCalled(); + expect(qb.orderBy).toHaveBeenCalledWith('course.createdAt', 'DESC'); + }); + + it('autocomplete uses simple tsquery with prefix operator', async () => { + const qb: any = { + where: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + select: jest.fn().mockReturnThis(), + getMany: jest.fn().mockResolvedValue([{ id: 'c1', title: 'React' }]), + }; + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.getAutoComplete('re'); + + expect(qb.where).toHaveBeenCalledWith( + expect.stringContaining('to_tsquery'), + expect.objectContaining({ tsq: 're:*' }), + ); + }); + + it('autocomplete sanitizes tsquery metacharacters', async () => { + const qb: any = { + where: jest.fn().mockReturnThis(), + orderBy: jest.fn().mockReturnThis(), + take: jest.fn().mockReturnThis(), + select: jest.fn().mockReturnThis(), + getMany: jest.fn().mockResolvedValue([]), + }; + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + await service.getAutoComplete('a&b|c'); + + expect(qb.where).toHaveBeenCalledWith( + expect.stringContaining('to_tsquery'), + expect.objectContaining({ tsq: 'a b c:*' }), + ); + }); + + it('uses Elasticsearch fast-path when present and successful', async () => { + elasticsearch.search.mockResolvedValueOnce({ + hits: { total: { value: 1 }, hits: [{ _source: { id: 'c1', title: 'X' } }] }, + } as any); + + const result = await service.search('react'); + + expect(result.source).toBe('elasticsearch'); + expect(result.total).toBe(1); + expect(elasticsearch.search).toHaveBeenCalled(); + expect(courseRepository.createQueryBuilder).not.toHaveBeenCalled(); + }); + + it('falls back to DB when Elasticsearch throws', async () => { + elasticsearch.search.mockRejectedValueOnce(new Error('ES down')); + const qb = makeQb({ rows: [], total: 0 }); + courseRepository.createQueryBuilder.mockReturnValueOnce(qb); + + const result = await service.search('react'); + + expect(result.source).toBeUndefined(); + expect(result.query).toBe('react'); + }); + + describe('Issue #889 — Tenant isolation enforcement on Elasticsearch queries', () => { + it('tenant_a_search_excludes_tenant_b_content', async () => { + isolationService.getTenantId.mockReturnValue('tenant-a'); + + const mockEsDocs = [ + { _source: { id: 'c1', title: 'Course A1', tenantId: 'tenant-a' } }, + { _source: { id: 'c2', title: 'Course A2', tenantId: 'tenant-a' } }, + ]; + + elasticsearch.search.mockImplementationOnce(async (params: any) => { + const filterClause = params.query?.bool?.filter; + expect(filterClause).toEqual(expect.arrayContaining([{ term: { tenantId: 'tenant-a' } }])); + + return { + hits: { + total: { value: mockEsDocs.length }, + hits: mockEsDocs, + }, + }; + }); + + const result = await service.search('Course'); + + expect(result.source).toBe('elasticsearch'); + expect(result.results.length).toBe(2); + const hasTenantBContent = result.results.some((doc: any) => doc.tenantId === 'tenant-b'); + expect(hasTenantBContent).toBe(false); + }); + + it('tenant_filter_applied_with_empty_query', async () => { + isolationService.getTenantId.mockReturnValue('tenant-a'); + + elasticsearch.search.mockResolvedValueOnce({ + hits: { total: { value: 0 }, hits: [] }, + }); + + await service.search(''); + + expect(elasticsearch.search).toHaveBeenCalledWith( + expect.objectContaining({ + query: expect.objectContaining({ + bool: expect.objectContaining({ + filter: expect.arrayContaining([{ term: { tenantId: 'tenant-a' } }]), + }), + }), + }), + ); + }); + + it('tenant_filter_applied_across_all_query_methods', async () => { + isolationService.getTenantId.mockReturnValue('tenant-a'); + + elasticsearch.search.mockResolvedValueOnce({ + hits: { total: { value: 0 }, hits: [] }, + }); + + await (service as any).tryElasticsearch('test', undefined, 1, 20); + + expect(elasticsearch.search).toHaveBeenCalledWith( + expect.objectContaining({ + query: expect.objectContaining({ + bool: expect.objectContaining({ + filter: expect.arrayContaining([service.buildTenantFilter('tenant-a')]), + }), + }), + }), + ); + }); + }); +}); diff --git a/src/search/search.service.ts b/src/search/search.service.ts index f2fdd794..1c8f0f83 100644 --- a/src/search/search.service.ts +++ b/src/search/search.service.ts @@ -1,12 +1,15 @@ -import { Injectable, Logger, Inject, Optional } from '@nestjs/common'; +import { Injectable, Logger, Inject, Optional, BadRequestException } from '@nestjs/common'; import { CACHE_MANAGER } from '@nestjs/cache-manager'; import { ElasticsearchService as NestElasticsearchService } from '@nestjs/elasticsearch'; import type { Cache } from 'cache-manager'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository, Brackets } from 'typeorm'; -import { Course } from '../courses/entities/course.entity'; +import { Course, CourseStatus } from '../courses/entities/course.entity'; import { LRUCache } from 'lru-cache'; import { IsolationService } from '../tenancy/isolation/isolation.service'; +import { OnEvent } from '@nestjs/event-emitter'; +import { CACHE_EVENTS, CACHE_TTL, CACHE_PREFIXES } from '../caching/caching.constants'; +import { SEARCH_CONSTANTS } from './search.constants'; export interface SearchFilters { category?: string | string[]; @@ -28,12 +31,26 @@ export interface SearchFilters { }; } +export interface FacetValue { + value: string; + count: number; +} + +export interface AvailableFilters { + categories: FacetValue[]; + levels: FacetValue[]; + languages: FacetValue[]; +} + interface AutocompleteResult { title: string; type: 'course' | 'category' | 'trending'; metadata?: Record; } +/** Cache key for the derived facet aggregation. */ +const FACETS_CACHE_KEY = `${CACHE_PREFIXES.SEARCH}:facets`; + /** * Issue #814 — full-text search backed by a `tsvector` generated column with a * GIN index (see migration `1783000000003-add-course-full-text-search.ts`). @@ -43,6 +60,12 @@ interface AutocompleteResult { * lowercases, applies the stemming configuration, ANDs all terms). * `to_tsvector` is already cached in `course.search_vector`, so this is just * an index scan + ranking — no seq scan, even at 100k+ rows. + * + * Issue #999 — getAvailableFilters() now runs real GROUP BY aggregations over + * published courses rather than returning a hardcoded literal. Results are + * cached (CACHE_TTL.SEARCH_RESULTS) and invalidated on COURSE_CREATED / + * COURSE_UPDATED / COURSE_DELETED events. Incoming SearchFilters.category + * values are validated against the live facet set. */ @Injectable() export class SearchService { @@ -74,6 +97,12 @@ export class SearchService { limit: number = 20, ): Promise { const safeQuery = query?.trim() ?? ''; + + // Issue #999 — validate incoming filter values against the live facet set. + if (filters) { + await this.validateFilters(filters); + } + const cacheKey = `search:${safeQuery}:${JSON.stringify(filters)}:${sort}:${page}`; if (this.cacheManager) { @@ -182,12 +211,109 @@ export class SearchService { } } - async getAvailableFilters(): Promise { - return { - categories: ['programming', 'web-development', 'data-science', 'design', 'business'], - levels: ['beginner', 'intermediate', 'advanced'], - languages: ['en', 'es', 'fr', 'de', 'zh'], - }; + /** + * Issue #999 — derives filter facets from real published courses via GROUP BY + * aggregations. Results are cached under FACETS_CACHE_KEY with a short TTL + * (CACHE_TTL.SEARCH_RESULTS = 2 min) and invalidated whenever a course is + * created, updated, or deleted. + * + * Shape: `{ categories: FacetValue[], levels: FacetValue[], languages: FacetValue[] }` + * where each FacetValue is `{ value: string; count: number }`. + * Zero-count values are omitted — every returned facet will yield at least + * one result when applied as a filter. + */ + async getAvailableFilters(): Promise { + // Return cached result when available to avoid re-running the aggregation + // on every facet request within the TTL window (Issue #999). + if (this.cacheManager) { + const cached = await this.cacheManager.get(FACETS_CACHE_KEY); + if (cached) return cached; + } + + const facets = await this.aggregateFacets(); + + if (this.cacheManager) { + await this.cacheManager.set(FACETS_CACHE_KEY, facets, CACHE_TTL.SEARCH_RESULTS); + } + + return facets; + } + + /** + * Runs the GROUP BY aggregations against the `course` table and assembles + * the AvailableFilters response. Only published courses contribute. + */ + private async aggregateFacets(): Promise { + try { + // Category facet — `category` is a real nullable column on `course`. + const categoryRows: Array<{ category: string; count: string }> = await this.courseRepository + .createQueryBuilder('course') + .select('course.category', 'category') + .addSelect('COUNT(*)', 'count') + .where('course.status = :status', { status: CourseStatus.PUBLISHED }) + .andWhere('course.category IS NOT NULL') + .andWhere("course.category <> ''") + .groupBy('course.category') + .orderBy('count', 'DESC') + .limit(SEARCH_CONSTANTS.AGG_CATEGORIES_SIZE) + .getRawMany(); + + const categories: FacetValue[] = categoryRows.map((row) => ({ + value: row.category, + count: parseInt(row.count, 10), + })); + + // `level` and `language` are not yet columns on the Course entity. + // Return empty arrays as placeholders so the API contract is stable; + // these will be populated once the columns are added. + const levels: FacetValue[] = []; + const languages: FacetValue[] = []; + + return { categories, levels, languages }; + } catch (err) { + this.logger.error(`Facet aggregation failed: ${(err as Error).message}`); + return { categories: [], levels: [], languages: [] }; + } + } + + /** + * Validates incoming SearchFilters against the live facet values. + * Throws BadRequestException (HTTP 400) for any unknown category value. + * + * Issue #999 — prevents hardcoded facet lists from drifting out of sync + * with accepted filter values. + */ + async validateFilters(filters: SearchFilters): Promise { + if (!filters.category) return; + + const requestedCategories = Array.isArray(filters.category) + ? filters.category + : [filters.category]; + + const facets = await this.getAvailableFilters(); + const validCategories = new Set(facets.categories.map((f) => f.value)); + + const unknown = requestedCategories.filter((c) => !validCategories.has(c)); + if (unknown.length > 0) { + throw new BadRequestException( + `Unknown filter value(s) for 'category': ${unknown.join(', ')}. ` + + `Valid values are: ${[...validCategories].join(', ') || '(none — no published courses yet)'}`, + ); + } + } + + /** + * Issue #999 — invalidate the facets cache whenever a course is created, + * updated, or deleted so the next request re-runs the aggregation. + */ + @OnEvent(CACHE_EVENTS.COURSE_CREATED) + @OnEvent(CACHE_EVENTS.COURSE_UPDATED) + @OnEvent(CACHE_EVENTS.COURSE_DELETED) + async onCourseChanged(_payload: { id: string }): Promise { + if (this.cacheManager) { + await this.cacheManager.del(FACETS_CACHE_KEY); + this.logger.debug('Facets cache invalidated after course change'); + } } buildTenantFilter(tenantId: string): { term: { tenantId: string } } {