Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 89 additions & 1 deletion frontend/src/__tests__/recommendations.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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');
});
});
115 changes: 89 additions & 26 deletions frontend/src/recommendations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,21 @@ let currentPurchaseRecommendations: LocalRecommendation[] = [];
// Cache of account ID → name for column display
let accountNamesCache: Map<string, string> = 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
Expand Down Expand Up @@ -74,10 +89,34 @@ export async function loadRecommendations(): Promise<void> {
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();
Expand Down Expand Up @@ -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<string, LocalRecommendation>();
// Exported so unit tests can exercise it directly.
export function pickBestVariantPerCell(recs: readonly LocalRecommendation[]): LocalRecommendation[] {
// Group recs by cell key.
const groups = new Map<string, LocalRecommendation[]>();
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<string, string> = {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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' };
}

/**
Expand Down