diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 23e1b179..8d145729 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -38,6 +38,7 @@ jest.mock('../state', () => ({ getCurrentAccountIDs: jest.fn().mockReturnValue([]), setCurrentAccountIDs: jest.fn(), getRecommendations: jest.fn().mockReturnValue([]), + getRecommendationByID: jest.fn().mockReturnValue(undefined), setRecommendations: jest.fn(), getSelectedRecommendationIDs: jest.fn().mockReturnValue(new Set()), clearSelectedRecommendations: jest.fn(), @@ -371,6 +372,44 @@ describe('Recommendations Module', () => { expect(calledIds).not.toContain('rec-azure-sub2'); }); + // Issue #224: when a sibling variant is selected but currently hidden by a + // column filter (e.g. user filtered to 3yr, but 1yr sibling is still + // selected in state), checking a 3yr sibling must evict the hidden 1yr + // selection. The fix iterates state.getRecommendations() (full loaded set) + // rather than the filtered `recommendations` array rendered in the DOM. + test('checking a visible variant evicts a same-cell sibling hidden by filter', async () => { + const allRecs = [ + { id: 'rec-1yr', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 100, upfront_cost: 0 }, + { id: 'rec-3yr', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 3, savings: 300, upfront_cost: 0 }, + ]; + // Simulate: API returns all recs, but only the 3yr one is visible (1yr is + // hidden by filter). state.getRecommendations() returns the full set. + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: {}, + recommendations: [allRecs[1]], // only 3yr visible in rendered list + regions: [] + }); + (state.getRecommendations as jest.Mock).mockReturnValue(allRecs); // full set in state + (state as unknown as { getRecommendationByID: jest.Mock }).getRecommendationByID.mockImplementation((id: string) => allRecs.find((r) => r.id === id)); + // Simulate: 1yr is already selected in state. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['rec-1yr'])); + + await loadRecommendations(); + + // Tick the 3yr checkbox. + const cb = document.querySelector('input[data-rec-id="rec-3yr"]'); + expect(cb).not.toBeNull(); + cb!.checked = true; + cb!.dispatchEvent(new Event('change')); + + // The hidden 1yr sibling must be evicted. + const removeCalls = (state.removeSelectedRecommendation as jest.Mock).mock.calls.map(c => c[0]); + expect(removeCalls).toContain('rec-1yr'); + // And the 3yr rec is added. + const addCalls = (state.addSelectedRecommendation as jest.Mock).mock.calls.map(c => c[0]); + expect(addCalls).toContain('rec-3yr'); + }); + test('purchase button opens modal for that recommendation', async () => { const mockRecs = [ { id: 'rec-11', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 100, upfront_cost: 500 } @@ -380,6 +419,7 @@ describe('Recommendations Module', () => { recommendations: mockRecs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(mockRecs); await loadRecommendations(); @@ -408,6 +448,7 @@ describe('Recommendations Module', () => { recommendations: mockRecs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(mockRecs); await loadRecommendations(); @@ -2023,3 +2064,170 @@ describe('Issue #132: bulk-buy collapses SP plan types into one bucket', () => { expect(sectionTitles.some((t) => t.includes('AWS / ec2'))).toBe(true); }); }); + +// Issue #224: at most one (term, payment) variant per physical-resource +// cell can be selected at any time. After PR #195's per-cell fan-out (2 +// terms × 3 payments per cell), naive selection produces wrong purchase +// intent — manual checking lets the user accumulate sibling commitments, +// and `select-all` over-commits 6×. Cell = `(provider, account, service, +// region, resource_type, engine)`. The fix lives in two places: the +// per-row checkbox change handler (deselect any in-cell sibling on check) +// and the select-all handler (group by cell, pick highest-effective per). +describe('Issue #224: one-variant-per-cell radio selection', () => { + beforeEach(() => { + document.body.replaceChildren(); + const recsTab = document.createElement('div'); + recsTab.id = 'recommendations-tab'; + recsTab.className = 'tab-content active'; + const summary = document.createElement('div'); + summary.id = 'recommendations-summary'; + const list = document.createElement('div'); + list.id = 'recommendations-list'; + recsTab.appendChild(summary); + recsTab.appendChild(list); + document.body.appendChild(recsTab); + const purchaseModal = document.createElement('div'); + purchaseModal.id = 'purchase-modal'; + purchaseModal.className = 'hidden'; + document.body.appendChild(purchaseModal); + jest.clearAllMocks(); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); + }); + + // (a) Manual toggle: two variants of the same cell. Checking variant B + // when variant A is already selected must remove A first, leaving only B. + test('(a) checking variant B in same cell deselects sibling variant A', async () => { + const recs = [ + { id: 'cellA-1y-allup', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 1, savings: 100, upfront_cost: 500 }, + { id: 'cellA-3y-noup', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 200, upfront_cost: 0 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + // state.getRecommendations() is the full loaded set (used by the sibling-eviction loop). + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + // Pretend variant A is already selected (the "user previously checked it" state). + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cellA-1y-allup'])); + await loadRecommendations(); + + // Tick variant B in the same cell. + const cbs = Array.from(document.querySelectorAll('tbody input[data-rec-id]')); + const variantB = cbs.find((cb) => cb.dataset['recId'] === 'cellA-3y-noup'); + expect(variantB).toBeDefined(); + variantB!.checked = true; + variantB!.dispatchEvent(new Event('change')); + + // The handler must have removed sibling A AND added B. + const removed = (state.removeSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + const added = (state.addSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + expect(removed).toContain('cellA-1y-allup'); + expect(added).toContain('cellA-3y-noup'); + // Sanity: B was not also removed, A was not also added. + expect(removed).not.toContain('cellA-3y-noup'); + expect(added).not.toContain('cellA-1y-allup'); + }); + + // (b) Cross-cell independence: selecting a rec in cell X must not + // affect cell Y's selection state. The radio enforcement is per-cell, + // not global. + test('(b) selecting in cell X does not affect cell Y selections', async () => { + const recs = [ + { id: 'cellX-1y', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 1, savings: 100, upfront_cost: 500 }, + { id: 'cellY-1y', provider: 'aws', cloud_account_id: 'acct-1', service: 'rds', resource_type: 'db.r5.large', region: 'us-east-1', engine: 'mysql', count: 1, term: 1, savings: 200, upfront_cost: 1000 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + // Pretend cellY is already selected. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cellY-1y'])); + await loadRecommendations(); + + // Tick cellX. + const cbs = Array.from(document.querySelectorAll('tbody input[data-rec-id]')); + const cellX = cbs.find((cb) => cb.dataset['recId'] === 'cellX-1y'); + expect(cellX).toBeDefined(); + cellX!.checked = true; + cellX!.dispatchEvent(new Event('change')); + + // cellY must NOT have been removed — cells are independent. + const removed = (state.removeSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + expect(removed).not.toContain('cellY-1y'); + const added = (state.addSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + expect(added).toContain('cellX-1y'); + }); + + // (c) Select-all collapses to one-per-cell. Three distinct cells × six + // variants each = 18 recs. After select-all, exactly 3 should be added, + // not 18 (one per cell). This is the headline money-impact fix. + test('(c) select-all picks exactly one variant per cell (3 cells × 6 variants → 3 selected)', async () => { + const cells = [ + { account: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '' }, + { account: 'acct-1', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', engine: '' }, + { account: 'acct-1', service: 'rds', resource_type: 'db.r5.large', region: 'eu-west-1', engine: 'mysql' }, + ]; + const variants = [ + { term: 1, payment: 'all-upfront' }, + { term: 1, payment: 'partial-upfront' }, + { term: 1, payment: 'no-upfront' }, + { term: 3, payment: 'all-upfront' }, + { term: 3, payment: 'partial-upfront' }, + { term: 3, payment: 'no-upfront' }, + ]; + const recs: Array> = []; + let i = 0; + for (const c of cells) { + for (const v of variants) { + recs.push({ + id: `c${i++}`, + provider: 'aws', cloud_account_id: c.account, service: c.service, + resource_type: c.resource_type, region: c.region, engine: c.engine, + count: 1, term: v.term, savings: 100 + i, upfront_cost: 500, + }); + } + } + expect(recs).toHaveLength(18); + + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + await loadRecommendations(); + + const selectAll = document.getElementById('select-all-recs') as HTMLInputElement; + expect(selectAll).not.toBeNull(); + selectAll.checked = true; + selectAll.dispatchEvent(new Event('change')); + + // Exactly 3 add calls — one per cell. + expect((state.addSelectedRecommendation as jest.Mock).mock.calls).toHaveLength(3); + // And clearSelectedRecommendations was called first to drop any stale state. + expect(state.clearSelectedRecommendations).toHaveBeenCalled(); + }); + + // (d) Tiebreaker: when multiple variants share a cell, select-all picks + // the variant with the highest EFFECTIVE monthly savings (amortizing + // upfront over term * 12 months) — NOT the highest raw `savings`. + // Concrete example: a 3yr/all-upfront with $36000 upfront + $1200/mo + // headline savings has effective = 1200 - 36000/36 = $200. A 1yr/no-upfront + // with $0 upfront + $300/mo headline savings has effective = $300. The + // 1yr/no-upfront wins despite the 3yr's higher raw `savings`. + test('(d) select-all picks highest-effective-savings (amortized) per cell', async () => { + const recs = [ + // 3yr/all-upfront — high raw savings ($1200/mo) but huge upfront drags effective to $200/mo. + { id: 'big-upfront', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 1200, upfront_cost: 36000 }, + // 1yr/no-upfront — lower raw ($300/mo) but $0 upfront → effective stays at $300/mo. + { id: 'no-upfront', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 1, savings: 300, upfront_cost: 0 }, + // 3yr/partial-upfront — middle of the road ($600/mo savings, $7200 upfront → effective = 600 - 7200/36 = $400). + { id: 'middle', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 600, upfront_cost: 7200 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + await loadRecommendations(); + + const selectAll = document.getElementById('select-all-recs') as HTMLInputElement; + selectAll.checked = true; + selectAll.dispatchEvent(new Event('change')); + + // Exactly one add call (single cell, one variant picked). + const addCalls = (state.addSelectedRecommendation as jest.Mock).mock.calls; + expect(addCalls).toHaveLength(1); + // The "middle" variant has the highest effective ($400/mo > $300 > $200) — picked. + expect(addCalls[0]![0]).toBe('middle'); + }); +}); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index abf17259..80f90da6 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -142,6 +142,55 @@ const SORTABLE_STRING_COLUMNS: Record string region: (r) => r.region ?? '', }; +// cellKey identifies the physical-resource cell a rec belongs to. +// After PR #195's per-(term, payment) fan-out, a single physical resource +// produces up to 6 alternative rec rows (2 terms × 3 payments). The +// `(provider, cloud_account_id, service, region, resource_type, engine)` +// prefix is the cell — recs sharing this prefix are alternatives, not +// additions. Same prefix the scheduler ID encoding uses +// (scheduler.go:856-858, PR #189) but without the (term, payment) suffix. +// +// Used by issue #224's radio enforcement: at most one variant per cell +// can be selected at any time. +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. +// +// Effective savings amortizes the upfront cost across the term: +// effective = savings - (upfront_cost / (term * 12)) +// +// 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. +// +// 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(); + 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); + } + } + return Array.from(byCell.values()); +} + const SORT_HEADER_LABELS: Record = { provider: 'Provider', account: 'Account', @@ -1930,7 +1979,16 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { if (selectAllCheckbox) { selectAllCheckbox.addEventListener('change', () => { if (selectAllCheckbox.checked) { - recommendations.forEach((r) => state.addSelectedRecommendation(r.id)); + // Issue #224: select-all picks ONE variant per cell (highest-effective- + // savings) rather than every visible row. After PR #195's per-(term, + // payment) fan-out, naive "select every row" produces 6× the intended + // commitments per resource — wrong purchase intent. Clear current + // selection first so a stale choice from a different filter context + // doesn't bleed through. + state.clearSelectedRecommendations(); + for (const r of pickBestVariantPerCell(recommendations)) { + state.addSelectedRecommendation(r.id); + } } else { state.clearSelectedRecommendations(); } @@ -1942,11 +2000,30 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { // changes so a stale selection from a previous filter is a no-op // once the user narrows, rather than pointing at whichever rec // happens to occupy the old index position. + // + // Issue #224: enforce one-variant-per-cell radio behaviour on check. + // When the user checks a variant, deselect any other variant of the + // same cell that's already selected — a single physical resource + // can only carry one (term, payment) commitment at a time. container.querySelectorAll('input[data-rec-id]').forEach(cb => { cb.addEventListener('change', () => { const id = cb.dataset['recId'] || ''; if (!id) return; if (cb.checked) { + const newRec = recommendations.find((r) => r.id === id); + if (newRec) { + const newCell = cellKey(newRec); + const selected = state.getSelectedRecommendationIDs(); + // Scan the full loaded set (not just the filtered view) so that + // hidden siblings (e.g. filtered-out term variants) are also + // deselected, preserving the one-variant-per-cell contract. + const allLoaded = state.getRecommendations() as unknown as LocalRecommendation[]; + for (const r of allLoaded) { + if (r.id !== id && selected.has(r.id) && cellKey(r) === newCell) { + state.removeSelectedRecommendation(r.id); + } + } + } state.addSelectedRecommendation(id); } else { state.removeSelectedRecommendation(id);