From d95194aa405aac3b351b4e10d0eb34baddd67313 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 3 May 2026 18:08:14 +0200 Subject: [PATCH] feat(frontend/recs): default-seed payment/term from GlobalConfig (closes #223) Three surfaces previously hardcoded `'all-upfront'` as the payment default regardless of operator configuration. All three now seed from the resolved `GlobalConfig.DefaultTerm + DefaultPayment`: 1. `loadRecommendations` adds `api.getConfig()` as a siloed third leg of the bootstrap `Promise.all` (failure is `.catch(() => null)` so a missing config endpoint cannot block rec load). Resolved values populate module-level `cachedGlobalDefaultTerm / Payment` vars. 2. `defaultBulkPurchaseState.payment` is mutated post-fetch so the bulk-toolbar first-visit fallback reflects the operator's setting. `loadBulkPurchaseState()` localStorage fallback also switched from the literal `'all-upfront'` to `cachedGlobalDefaultPayment`. 3. `resolvePerRecPaymentSeed` fallback now prefers `cachedGlobalDefaultPayment` when it is supported for the rec's (provider, service, term), falling back to `paymentOptionsFor[0]` only if the configured payment is unavailable for that combination. 4. `pickBestVariantPerCell` (select-all tiebreaker, added by #224) is replaced: it now prefers the variant matching `(cachedGlobalDefaultTerm, cachedGlobalDefaultPayment)` and falls back to highest-effective-savings only when no variant matches. Function is now exported for direct unit testing. `seedGlobalDefaults(term, payment)` exported for test injection. Four new unit tests cover the config-match tiebreaker and fallback. --- .../src/__tests__/recommendations.test.ts | 90 +++++++++++++- frontend/src/recommendations.ts | 115 ++++++++++++++---- 2 files changed, 178 insertions(+), 27 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 8d145729..041a2744 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -1,13 +1,17 @@ /** * Recommendations module tests */ -import { loadRecommendations, openPurchaseModal, getPurchaseModalRecommendations, refreshRecommendations, setupRecommendationsHandlers, clearRecommendationDetailCache } from '../recommendations'; +import { loadRecommendations, openPurchaseModal, getPurchaseModalRecommendations, refreshRecommendations, setupRecommendationsHandlers, clearRecommendationDetailCache, pickBestVariantPerCell, seedGlobalDefaults } from '../recommendations'; // Mock the api module jest.mock('../api', () => ({ getRecommendations: jest.fn(), refreshRecommendations: jest.fn(), listAccounts: jest.fn().mockResolvedValue([]), + // issue #223: getConfig is fetched on page load to resolve GlobalConfig + // defaults (DefaultTerm + DefaultPayment). Default-empty global config so + // pre-#223 tests retain their hardcoded-fallback behavior without extra setup. + getConfig: jest.fn().mockResolvedValue({ global: {} }), // Issue #111: openFanOutModal now pre-fetches per-account service // overrides to seed each bucket's Payment default. Default-empty so // pre-#111 tests retain their toolbar-seeded behavior without any @@ -2231,3 +2235,87 @@ describe('Issue #224: one-variant-per-cell radio selection', () => { expect(addCalls[0]![0]).toBe('middle'); }); }); + +// --------------------------------------------------------------------------- +// issue #223: default-seed from GlobalConfig across all 3 surfaces. +// These tests exercise the pickBestVariantPerCell config-match tiebreaker +// and the seedGlobalDefaults hook that injects resolved GlobalConfig values. +// --------------------------------------------------------------------------- + +describe('issue #223: pickBestVariantPerCell config-match tiebreaker', () => { + const rec = ( + id: string, + term: 1 | 3, + payment: string, + savings = 100, + upfront_cost = 0, + ) => ({ + id, + provider: 'aws' as const, + cloud_account_id: 'acct-1', + service: 'ec2', + resource_type: 't3.medium', + region: 'us-east-1', + engine: '', + count: 1, + term, + payment, + savings, + upfront_cost, + } as unknown as LocalRecommendation); + + afterEach(() => { + // Reset module cache to initial defaults so tests don't bleed into each other. + seedGlobalDefaults(1, 'all-upfront'); + }); + + test('prefers variant matching configured (term, payment) over highest-effective', () => { + // Two variants in one cell: 1yr/all-upfront (configured default) vs + // 3yr/no-upfront (higher effective savings). + const recs = [ + rec('want-this', 1, 'all-upfront', 300, 0), // effective = $300/mo + rec('skip-this', 3, 'no-upfront', 400, 0), // effective = $400/mo (higher) + ]; + seedGlobalDefaults(1, 'all-upfront'); + const result = pickBestVariantPerCell(recs); + expect(result).toHaveLength(1); + expect(result[0]!.id).toBe('want-this'); + }); + + test('falls back to highest-effective when no variant matches configured defaults', () => { + // Neither variant matches term=1/all-upfront; fallback picks highest effective. + const recs = [ + rec('low-effective', 3, 'all-upfront', 1200, 36000), // effective = 1200 - 1000 = $200 + rec('high-effective', 3, 'no-upfront', 400, 0), // effective = $400 + ]; + seedGlobalDefaults(1, 'all-upfront'); // neither matches + const result = pickBestVariantPerCell(recs); + expect(result).toHaveLength(1); + expect(result[0]!.id).toBe('high-effective'); + }); + + test('handles multiple independent cells and picks config-match in each', () => { + // Two distinct cells; each has a variant matching configured defaults. + const recs = [ + rec('cell-a-match', 1, 'all-upfront', 100, 0), + rec('cell-a-other', 3, 'no-upfront', 400, 0), // higher effective but different cell + { ...rec('cell-b-match', 1, 'all-upfront', 200, 0), region: 'eu-west-1', id: 'cell-b-match' }, + { ...rec('cell-b-other', 3, 'partial-upfront', 600, 0), region: 'eu-west-1', id: 'cell-b-other' }, + ]; + seedGlobalDefaults(1, 'all-upfront'); + const result = pickBestVariantPerCell(recs); + const ids = result.map((r) => r.id).sort(); + expect(ids).toEqual(['cell-a-match', 'cell-b-match']); + }); + + test('config-match with 3yr/partial-upfront as configured defaults', () => { + const recs = [ + rec('wrong-1', 1, 'all-upfront', 100, 0), + rec('want-3yr', 3, 'partial-upfront', 100, 0), + ]; + seedGlobalDefaults(3, 'partial-upfront'); + const result = pickBestVariantPerCell(recs); + expect(result).toHaveLength(1); + expect(result[0]!.id).toBe('want-3yr'); + }); +}); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 80f90da6..8a5fbd45 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -26,6 +26,21 @@ let currentPurchaseRecommendations: LocalRecommendation[] = []; // Cache of account ID → name for column display let accountNamesCache: Map = new Map(); +// issue #223: resolved GlobalConfig defaults, seeded on page load by +// loadRecommendations(). Initial values mirror the historical hardcoded +// defaults so the module is usable before the first API round-trip. +let cachedGlobalDefaultPayment: CompatPayment = 'all-upfront'; +let cachedGlobalDefaultTerm: 1 | 3 = 1; + +/** + * Inject resolved GlobalConfig defaults into the module cache. + * Exported for testing only — not part of the public API. + */ +export function seedGlobalDefaults(term: 1 | 3, payment: CompatPayment): void { + cachedGlobalDefaultTerm = term; + cachedGlobalDefaultPayment = payment; +} + // populateRecommendationsAccountFilter / populateRegionFilter / the legacy // service-filter helpers were removed in Bundle B — those DOM elements are // gone from index.html. Provider / Account values now drive an API re-fetch @@ -74,10 +89,34 @@ export async function loadRecommendations(): Promise { account_ids: accountIDs.length > 0 ? accountIDs : undefined, }; - const [data, accounts] = await Promise.all([ + const [data, accounts, cfgResponse] = await Promise.all([ api.getRecommendations(filters) as unknown as RecommendationsResponse, - api.listAccounts().catch(() => []) + api.listAccounts().catch(() => []), + // issue #223: fetch GlobalConfig so per-cell variant selection and + // bulk-toolbar defaults reflect the operator's configured preference. + // Failure is siloed — a missing/unreachable config endpoint must not + // block the recommendations load; we fall back to the module defaults. + api.getConfig().catch(() => null), ]); + // Populate the module-level GlobalConfig cache (issue #223). + if (cfgResponse?.global) { + const g = cfgResponse.global; + const t = g.default_term; + if (t === 1 || t === 3) cachedGlobalDefaultTerm = t; + const validPayments: CompatPayment[] = ['all-upfront', 'partial-upfront', 'no-upfront', 'monthly']; + if (g.default_payment && (validPayments as string[]).includes(g.default_payment)) { + cachedGlobalDefaultPayment = g.default_payment as CompatPayment; + } + // Seed the bulk-toolbar default with the resolved value so that + // loadBulkPurchaseState()'s first-visit fallback uses it. + // BulkPurchasePayment is a subset of CompatPayment ('upfront' is + // Azure-only and not a valid bulk-toolbar value); skip the seed + // if the global default is outside that subset. + const validBulkPayments: BulkPurchasePayment[] = ['all-upfront', 'partial-upfront', 'no-upfront', 'monthly']; + if ((validBulkPayments as string[]).includes(cachedGlobalDefaultPayment)) { + defaultBulkPurchaseState.payment = cachedGlobalDefaultPayment as BulkPurchasePayment; + } + } accountNamesCache = new Map(accounts.map(a => [a.id, a.name])); state.setRecommendations((data.recommendations || []) as unknown as api.Recommendation[]); state.clearSelectedRecommendations(); @@ -156,39 +195,56 @@ function cellKey(rec: LocalRecommendation): string { return `${rec.provider}|${rec.cloud_account_id ?? ''}|${rec.service}|${rec.region}|${rec.resource_type}|${rec.engine ?? ''}`; } -// pickBestVariantPerCell collapses a list of recs to one rec per cell, -// choosing the variant with the highest effective monthly savings. +// pickBestVariantPerCell collapses a list of recs to one rec per cell. // -// Effective savings amortizes the upfront cost across the term: -// effective = savings - (upfront_cost / (term * 12)) +// issue #223: tiebreaker is "matches resolved GlobalConfig.DefaultTerm + +// DefaultPayment". If no variant in a cell matches the configured defaults, +// fall back to the variant with the highest effective monthly savings +// (the original #224 behaviour). // -// Two `(savings, upfront_cost, term)` tuples that look identical on raw -// `savings` can score very differently on the amortized number — e.g. a -// 3yr all-upfront commitment with a large lump-sum upfront has a much -// lower effective monthly savings than a no-upfront variant with the -// same headline savings. Picking by amortization picks the variant -// that's actually best for the operator's wallet over the term. +// Effective savings (fallback metric) amortizes the upfront cost across +// the term: +// effective = savings - (upfront_cost / (term * 12)) // -// Used by issue #224's `select-all` handler. Sibling issue #223 will -// replace this tiebreaker with "matches resolved GlobalConfig.DefaultTerm -// + DefaultPayment" once that lands; until then, amortized savings is -// the right deterministic default. -function pickBestVariantPerCell(recs: readonly LocalRecommendation[]): LocalRecommendation[] { - const byCell = new Map(); +// Exported so unit tests can exercise it directly. +export function pickBestVariantPerCell(recs: readonly LocalRecommendation[]): LocalRecommendation[] { + // Group recs by cell key. + const groups = new Map(); + for (const r of recs) { + const k = cellKey(r); + const group = groups.get(k); + if (group) { + group.push(r); + } else { + groups.set(k, [r]); + } + } + const effective = (r: LocalRecommendation): number => { // Defensive clamp: if a rec arrives with term=0 from a malformed // fixture, treat it as 1yr so the division doesn't blow up. const monthsInTerm = Math.max(12, (r.term || 1) * 12); return r.savings - (r.upfront_cost / monthsInTerm); }; - for (const r of recs) { - const k = cellKey(r); - const existing = byCell.get(k); - if (!existing || effective(r) > effective(existing)) { - byCell.set(k, r); + + const result: LocalRecommendation[] = []; + for (const group of groups.values()) { + // Prefer the variant matching the operator's configured defaults. + const configMatch = group.find( + (r) => r.term === cachedGlobalDefaultTerm && r.payment === cachedGlobalDefaultPayment, + ); + if (configMatch) { + result.push(configMatch); + continue; + } + // No config-matching variant in this cell: fall back to highest effective savings. + let best = group[0]!; + for (let i = 1; i < group.length; i++) { + if (effective(group[i]!) > effective(best)) best = group[i]!; } + result.push(best); } - return Array.from(byCell.values()); + return result; } const SORT_HEADER_LABELS: Record = { @@ -1283,7 +1339,9 @@ function loadBulkPurchaseState(): BulkPurchaseToolbarState { // Explicit field-pick rather than spread-and-omit — avoids leaking a // legacy `term` value into the returned object even at runtime. return { - payment: parsed.payment || 'all-upfront', + // issue #223: fall back to resolved GlobalConfig.DefaultPayment rather + // than the literal 'all-upfront' when localStorage has no stored value. + payment: (parsed.payment || cachedGlobalDefaultPayment) as BulkPurchasePayment, capacity: Math.max(1, Math.min(100, Number(parsed.capacity) || 100)), }; } catch { @@ -2099,8 +2157,13 @@ function resolvePerRecPaymentSeed( // Defensive fallback: rec is missing/has-unsupported payment AND no // matching override. paymentOptionsFor always returns at least one // option for the (provider, service, term) cells the engine emits. + // issue #223: prefer GlobalConfig.DefaultPayment over the first option + // so the fallback is consistent with the operator's configured preference. const options = paymentOptionsFor(provider, rec.service, term); - return { payment: (options[0] ?? 'all-upfront') as CompatPayment, source: 'fallback' }; + const preferred = (options as string[]).includes(cachedGlobalDefaultPayment) + ? cachedGlobalDefaultPayment + : (options[0] ?? 'all-upfront') as CompatPayment; + return { payment: preferred, source: 'fallback' }; } /**