From 5866dc6bf31be89d7bbe134006e0b64bdbc81346 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 21:19:45 +0200 Subject: [PATCH] fix(frontend/recs): effectiveSavingsPct returns null for AWS rows without on_demand_cost (closes #323) For AWS RI/SP recs, EstimatedMonthlyOnDemandCost from Cost Explorer is the canonical denominator. When it is absent the reconstruction formula (monthly_cost + savings + amortized_upfront) diverges from the true on-demand baseline, producing misleadingly high percentages. Add a provider='aws' && !hasOnDemand guard that returns null so the UI renders the em-dash sentinel instead of a silently-wrong value. Azure rows retain the reconstruction fallback since older cached rows may legitimately omit on_demand_cost while remaining valid. Update reconstruction-path tests to use provider='azure' and add three new AWS-specific cases: no on_demand_cost, on_demand_cost=0, and valid on_demand_cost computes correctly. --- .../src/__tests__/recommendations.test.ts | 48 +++++++++++++++++-- frontend/src/recommendations.ts | 7 +++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 30bf8668d..4f2534af2 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3827,9 +3827,13 @@ describe('effectiveMonthlySavings', () => { }); describe('effectiveSavingsPct', () => { + // mk() defaults to 'azure' for reconstruction-path tests. AWS rows require + // on_demand_cost to be set; without it effectiveSavingsPct returns null + // (#323). Azure reconstruction is still valid (Azure may omit on_demand_cost + // for older cached rows where the field was not yet populated). const mk = (overrides: Partial): LocalRecommendation => ({ id: 'r', - provider: 'aws', + provider: 'azure', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', @@ -3876,7 +3880,7 @@ describe('effectiveSavingsPct', () => { expect(pct!).toBeCloseTo(-17.65, 1); }); - test('undefined/null monthly_cost returns null (data not provided — cannot compute effective %)', () => { + test('undefined/null monthly_cost returns null (data not provided)', () => { // monthly_cost null/undefined means the provider API did not return a monthly // recurring breakdown. Without it we cannot reconstruct on_demand_monthly, // so effectiveSavingsPct must return null rather than collapsing the @@ -3895,6 +3899,35 @@ describe('effectiveSavingsPct', () => { expect(pct!).toBeCloseTo(100, 1); }); + // #323: AWS rows require the provider-canonical on_demand_cost; without it + // the reconstruction formula (monthly_cost + savings + amortized) diverges + // from the true CE baseline and produces misleadingly high percentages. + describe('AWS reconstruction fallback returns null (#323)', () => { + const mkAws = (overrides: Partial): LocalRecommendation => + mk({ provider: 'aws', ...overrides }); + + test('AWS row with no on_demand_cost returns null regardless of monthly_cost', () => { + // Repro: RI rec where EstimatedMonthlyOnDemandCost was absent from the + // CE response. Reconstruction gives a number but it diverges from the + // real denominator, so null is the correct sentinel. + expect(effectiveSavingsPct(mkAws({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 }))).toBeNull(); + }); + + test('AWS row with on_demand_cost=0 (treated as not-populated) returns null', () => { + // Backend nonZeroPtr converts 0 -> nil, so 0 at the frontend means the + // field was absent. Same outcome as missing. + expect(effectiveSavingsPct(mkAws({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1, on_demand_cost: 0 }))).toBeNull(); + }); + + test('AWS row with valid on_demand_cost computes normally', () => { + // When CE supplies the baseline, the formula is well-defined. + // effectiveSavings = 100 - 0 = 100; pct = 100 / 300 * 100 = 33.33% + const pct = effectiveSavingsPct(mkAws({ savings: 100, upfront_cost: 0, monthly_cost: 200, term: 1, on_demand_cost: 300 })); + expect(pct).not.toBeNull(); + expect(pct!).toBeCloseTo(33.33, 1); + }); + }); + // #274: on_demand_cost (when populated by the provider) is used directly // as the denominator instead of reconstructing it from // monthly_cost + savings + amortized. The reconstruction collapses for @@ -4136,7 +4169,8 @@ describe('Monthly Cost + Effective % column rendering', () => { }); test('no-upfront row: Monthly Cost shows rec.monthly_cost, Effective % is positive', async () => { - const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 }); + // AWS row requires on_demand_cost (#323); add it so the pct column renders. + const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1, on_demand_cost: 150 }); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [rec], @@ -4151,7 +4185,9 @@ describe('Monthly Cost + Effective % column rendering', () => { }); test('all-upfront row: Monthly Cost shows $0, Effective % accounts for amortization', async () => { - const rec = baseRec({ savings: 50, upfront_cost: 600, monthly_cost: 0, term: 1 }); + // AWS row requires on_demand_cost (#323). savings=50, upfront=600, term=1 + // => amortized=50, effectiveSavings=0. on_demand_cost=100 => pct=0.0%. + const rec = baseRec({ savings: 50, upfront_cost: 600, monthly_cost: 0, term: 1, on_demand_cost: 100 }); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [rec], @@ -4180,7 +4216,9 @@ describe('Monthly Cost + Effective % column rendering', () => { }); test('negative-effective row: Effective % cell has effective-pct-negative class', async () => { - const rec = baseRec({ savings: 10, upfront_cost: 1200, monthly_cost: 400, term: 1 }); + // AWS row requires on_demand_cost (#323). savings=10, upfront=1200, term=1 + // => amortized=100, effectiveSavings=-90. on_demand_cost=510 => pct<0. + const rec = baseRec({ savings: 10, upfront_cost: 1200, monthly_cost: 400, term: 1, on_demand_cost: 510 }); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [rec], diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index b0ab6f8ce..48ad76b59 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -1032,6 +1032,13 @@ export function effectiveSavingsPct(r: LocalRecommendation): number | null { // produces 100% / neg%). const hasOnDemand = r.on_demand_cost != null && r.on_demand_cost > 0; if (r.monthly_cost == null && !hasOnDemand) return null; + // #323: for AWS rows the on_demand_cost field is the provider-canonical + // denominator (EstimatedMonthlyOnDemandCost from Cost Explorer). When it + // is absent the reconstruction formula (monthly_cost + savings + amortized) + // diverges from the true on-demand baseline for RI/SP recs, producing + // misleadingly high percentages. Return null so the UI renders "—" rather + // than a silently-wrong value. + if (r.provider === 'aws' && !hasOnDemand) return null; const monthsInTerm = r.term * 12; const amortized = r.upfront_cost / monthsInTerm; const effectiveSavings = r.savings - amortized;