From c5970de764f96ec509d0cbfb7852168da1df9731 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 20:59:13 +0200 Subject: [PATCH 1/4] ux(frontend): group SP plan types in bulk-buy fan-out modal (closes #249) When a mixed-SP bucket (2+ plan types) lands in the fan-out modal, append a collapsible
section listing per-plan-type subtotals. Collapsed by default to keep the modal compact; the native chevron lets operators inspect the Compute/SageMaker/EC2 Instance/Database split before submitting. Non-SP and single-plan-type SP buckets are unaffected. Adds one DOM integration test asserting the "+2 plan types" summary and per-plan-type rows render for a mixed-SP + EC2 fan-out scenario. --- .../src/__tests__/recommendations.test.ts | 47 +++++++++++++++++++ frontend/src/recommendations.ts | 44 +++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 30bf8668d..7244d6640 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3447,6 +3447,53 @@ describe('Issue #132: bulk-buy collapses SP plan types into one bucket', () => { // Non-SP bucket title still uses the raw service slug. expect(sectionTitles.some((t) => t.includes('AWS / ec2'))).toBe(true); }); + + // Issue #249: mixed-SP bucket renders collapsible per-plan-type sub-rows + // inside the fan-out section so the operator can see how the bulk-buy is + // split across plan types before submitting. + test('mixed-SP fan-out section renders per-plan-type breakdown (closes #249)', async () => { + const recs = [ + { id: 's1', provider: 'aws', cloud_account_id: 'a1', service: 'savings-plans-compute', resource_type: 'sp', region: 'us-east-1', count: 2, term: 1, savings: 100, upfront_cost: 500 }, + { id: 's2', provider: 'aws', cloud_account_id: 'a1', service: 'savings-plans-sagemaker', resource_type: 'sp', region: 'us-east-1', count: 1, term: 1, savings: 200, upfront_cost: 800 }, + { id: 'e1', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 50, upfront_cost: 300 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(recs.map((r) => r.id as string))); + + await loadRecommendations(); + (document.getElementById('bulk-purchase-btn') as HTMLButtonElement).click(); + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + + // The SP bucket section must contain a
element with the + // plan-type breakdown (issue #249). + const spSection = Array.from(document.querySelectorAll('.fanout-bucket')).find( + (s) => s.querySelector('h4')?.textContent?.includes('Savings Plans'), + ); + expect(spSection).toBeDefined(); + + // The collapsible group is present with "+2 plan types" summary. + const details = spSection!.querySelector('details.fanout-sp-plan-types'); + expect(details).not.toBeNull(); + const summaryText = details!.querySelector('summary')?.textContent ?? ''; + expect(summaryText).toBe('+2 plan types'); + + // Each plan type gets its own row labelled with the short plan-type name. + const planRows = Array.from(details!.querySelectorAll('p.fanout-sp-plan-type-row')) + .map((el) => el.textContent ?? ''); + expect(planRows).toHaveLength(2); + expect(planRows.some((t) => t.startsWith('Savings Plans (Compute)'))).toBe(true); + expect(planRows.some((t) => t.startsWith('Savings Plans (SageMaker)'))).toBe(true); + + // EC2 (non-SP) bucket must NOT render the plan-type breakdown. + const ec2Section = Array.from(document.querySelectorAll('.fanout-bucket')).find( + (s) => s.querySelector('h4')?.textContent?.includes('ec2'), + ); + expect(ec2Section).toBeDefined(); + expect(ec2Section!.querySelector('details.fanout-sp-plan-types')).toBeNull(); + }); }); // Issue #658: Azure SP rows (service="savingsplans") must collapse into diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index b0ab6f8ce..d93468b83 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -3549,6 +3549,50 @@ function renderFanOutBucketSection(b: FanOutBucket): HTMLElement { totals.textContent = `${bucketTotal.count} commitments · ${formatCurrency(bucketTotal.upfront)} upfront · ${formatCostForPeriod(bucketTotal.savings, bPeriod)} savings ${periodSuffix(bPeriod)}`; section.appendChild(totals); + // Issue #249: for mixed-SP buckets with 2+ distinct plan types, render + // a collapsible per-plan-type breakdown so operators can see how their + // bulk-buy is split across Compute / SageMaker / EC2 Instance / Database + // plan types before submitting. Collapsed by default to keep the modal + // compact; the chevron in the affords expand. + if (isSPBucket) { + // Group recs by their per-rec service slug. + const byPlanType = new Map(); + for (const r of b.recs) { + const existing = byPlanType.get(r.service); + if (existing) { + existing.push(r); + } else { + byPlanType.set(r.service, [r]); + } + } + if (byPlanType.size >= 2) { + const details = document.createElement('details'); + details.className = 'fanout-sp-plan-types'; + const summaryEl = document.createElement('summary'); + summaryEl.className = 'fanout-sp-plan-types-summary'; + summaryEl.textContent = `+${byPlanType.size} plan types`; + details.appendChild(summaryEl); + + for (const [slug, planRecs] of byPlanType) { + const planLabel = savingsPlansBucketLabel([slug]); + const planTotal = planRecs.reduce( + (acc, r) => ({ + count: acc.count + r.count, + upfront: acc.upfront + r.upfront_cost, + savings: acc.savings + r.savings, + }), + { count: 0, upfront: 0, savings: 0 }, + ); + const row = document.createElement('p'); + row.className = 'fanout-sp-plan-type-row'; + row.textContent = `${planLabel}: ${planTotal.count} commitment${planTotal.count === 1 ? '' : 's'} · ${formatCurrency(planTotal.upfront)} upfront · ${formatCostForPeriod(planTotal.savings, bPeriod)} savings ${periodSuffix(bPeriod)}`; + details.appendChild(row); + } + + section.appendChild(details); + } + } + return section; } From 024272b0e7766aa70ed3269bf1edebf3b1441f0d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 19:02:58 +0200 Subject: [PATCH 2/4] fix(opportunities): exclude umbrella SP slugs from plan-type fan-out count UMBRELLA_SLUGS ("savings-plans", "savingsplans") represent the SP family as a whole, not a concrete plan type. Including them in byPlanType inflated the "+N plan types" count and caused a spurious non-concrete row to render in the collapsible breakdown. Filter them out before populating the map so the size check and rendered rows only reflect real plan types. Exports UMBRELLA_SLUGS from purchase-compatibility for reuse, adds a regression test covering the mixed umbrella + concrete slug case. --- .../src/__tests__/recommendations.test.ts | 30 +++++++++++++++++++ frontend/src/lib/purchase-compatibility.ts | 2 +- frontend/src/recommendations.ts | 8 ++++- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 7244d6640..c38512d3f 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3494,6 +3494,36 @@ describe('Issue #132: bulk-buy collapses SP plan types into one bucket', () => { expect(ec2Section).toBeDefined(); expect(ec2Section!.querySelector('details.fanout-sp-plan-types')).toBeNull(); }); + + // Issue #249 regression: umbrella slugs ("savings-plans", "savingsplans") + // must be excluded from byPlanType so they do not inflate the "+N plan + // types" count or render a spurious non-concrete plan-type row. + test('umbrella SP slugs excluded from plan-type fan-out count (issue #249)', async () => { + const recs = [ + { id: 's1', provider: 'aws', cloud_account_id: 'a1', service: 'savings-plans-compute', resource_type: 'sp', region: 'us-east-1', count: 2, term: 1, savings: 100, upfront_cost: 500 }, + { id: 's2', provider: 'aws', cloud_account_id: 'a1', service: 'savings-plans', resource_type: 'sp', region: 'us-east-1', count: 1, term: 1, savings: 200, upfront_cost: 800 }, + { id: 's3', provider: 'azure', cloud_account_id: 'a1', service: 'savingsplans', resource_type: 'sp', region: 'us-east-1', count: 1, term: 1, savings: 50, upfront_cost: 300 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(recs.map((r) => r.id as string))); + + await loadRecommendations(); + (document.getElementById('bulk-purchase-btn') as HTMLButtonElement).click(); + await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); + + const spSection = Array.from(document.querySelectorAll('.fanout-bucket')).find( + (s) => s.querySelector('h4')?.textContent?.includes('Savings Plans'), + ); + expect(spSection).toBeDefined(); + + // Only 1 concrete plan type (Compute) after filtering umbrella slugs; + // the collapsible block must NOT be rendered (size < 2). + const details = spSection!.querySelector('details.fanout-sp-plan-types'); + expect(details).toBeNull(); + }); }); // Issue #658: Azure SP rows (service="savingsplans") must collapse into diff --git a/frontend/src/lib/purchase-compatibility.ts b/frontend/src/lib/purchase-compatibility.ts index 005ea1b7b..40f161ca1 100644 --- a/frontend/src/lib/purchase-compatibility.ts +++ b/frontend/src/lib/purchase-compatibility.ts @@ -109,7 +109,7 @@ const SP_SHORT_LABEL: Record = { // slug and the legacy AWS SP umbrella (common.ServiceSavingsPlans). It is // a family marker, not a plan-type label, so it is treated the same way // as SAVINGS_PLANS_BUCKET_KEY. -const UMBRELLA_SLUGS = new Set([SAVINGS_PLANS_BUCKET_KEY, 'savingsplans']); +export const UMBRELLA_SLUGS = new Set([SAVINGS_PLANS_BUCKET_KEY, 'savingsplans']); // savingsPlansBucketLabel formats the bulk-buy bucket title for one // or more SP plan types. Returns: diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index d93468b83..18bafdea7 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -14,6 +14,7 @@ import { paymentOptionsFor, SAVINGS_PLANS_BUCKET_KEY, savingsPlansBucketLabel, + UMBRELLA_SLUGS, type Payment as CompatPayment, type Provider as CompatProvider, } from './lib/purchase-compatibility'; @@ -3555,9 +3556,14 @@ function renderFanOutBucketSection(b: FanOutBucket): HTMLElement { // plan types before submitting. Collapsed by default to keep the modal // compact; the chevron in the affords expand. if (isSPBucket) { - // Group recs by their per-rec service slug. + // Group recs by their per-rec service slug, excluding umbrella slugs + // (e.g. "savings-plans", "savingsplans") that represent the SP family + // as a whole rather than a specific plan type. Including them would + // inflate the "+N plan types" count and render a spurious non-concrete + // plan-type row. Issue #249. const byPlanType = new Map(); for (const r of b.recs) { + if (UMBRELLA_SLUGS.has(r.service)) continue; const existing = byPlanType.get(r.service); if (existing) { existing.push(r); From 1bd1c5cc5bef3069b637692ad02da7cd31e61ee3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 23:40:23 +0200 Subject: [PATCH 3/4] fix(recommendations/sp-grouping): harden SP bucket lookup in test (CR #826) Switch find+find from .find() to .filter()+loop so the umbrella-slug exclusion regression is asserted against every SP fanout-bucket, not just the first match, preventing false-positive passes with multi-provider SP fixtures. --- frontend/src/__tests__/recommendations.test.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index c38512d3f..f922c22e5 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3514,15 +3514,16 @@ describe('Issue #132: bulk-buy collapses SP plan types into one bucket', () => { (document.getElementById('bulk-purchase-btn') as HTMLButtonElement).click(); await Promise.resolve(); await Promise.resolve(); await Promise.resolve(); - const spSection = Array.from(document.querySelectorAll('.fanout-bucket')).find( + const spSections = Array.from(document.querySelectorAll('.fanout-bucket')).filter( (s) => s.querySelector('h4')?.textContent?.includes('Savings Plans'), ); - expect(spSection).toBeDefined(); + expect(spSections.length).toBeGreaterThan(0); // Only 1 concrete plan type (Compute) after filtering umbrella slugs; - // the collapsible block must NOT be rendered (size < 2). - const details = spSection!.querySelector('details.fanout-sp-plan-types'); - expect(details).toBeNull(); + // the collapsible block must NOT be rendered (size < 2) for any SP bucket. + for (const section of spSections) { + expect(section.querySelector('details.fanout-sp-plan-types')).toBeNull(); + } }); }); From 9eef9d2683bedcfe15b4ab422c8c7c73ec69677c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 23:31:40 +0200 Subject: [PATCH 4/4] fix(purchase-compatibility): type UMBRELLA_SLUGS as ReadonlySet (CR #826) Prevents accidental mutation via .add()/.delete() on the exported Set. All existing consumers use only .has(), so the narrowed type is safe. --- frontend/src/lib/purchase-compatibility.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/lib/purchase-compatibility.ts b/frontend/src/lib/purchase-compatibility.ts index 40f161ca1..f6ac892a0 100644 --- a/frontend/src/lib/purchase-compatibility.ts +++ b/frontend/src/lib/purchase-compatibility.ts @@ -109,7 +109,7 @@ const SP_SHORT_LABEL: Record = { // slug and the legacy AWS SP umbrella (common.ServiceSavingsPlans). It is // a family marker, not a plan-type label, so it is treated the same way // as SAVINGS_PLANS_BUCKET_KEY. -export const UMBRELLA_SLUGS = new Set([SAVINGS_PLANS_BUCKET_KEY, 'savingsplans']); +export const UMBRELLA_SLUGS: ReadonlySet = new Set([SAVINGS_PLANS_BUCKET_KEY, 'savingsplans']); // savingsPlansBucketLabel formats the bulk-buy bucket title for one // or more SP plan types. Returns: