Skip to content

Commit cfb8525

Browse files
committed
fix(api): current_savings zero for services with no active commitments (closes #1031)
summarizeRecommendationsWithCoverage incorrectly wrote CurrentSavings to the coverage-scaled potential amount (same as PotentialSavings). Because getDashboardSummary only overwrites CurrentSavings for services present in the purchase-history result, services with recommendations but no active purchases shipped a non-zero current_savings equal to potential_savings. This was wrong: the frontend renders current_savings as committed/realized savings from active purchase history, so an uncommitted service must ship 0. The partial fix in #926 (aggregateActiveCommitmentsPerService + purchase- history overwrite) was correct in concept but left the incorrect assignment in summarizeRecommendationsWithCoverage active, masking the bug for services that had no purchases. Fix: remove svc.CurrentSavings += scaled from summarizeRecommendationsWithCoverage. CurrentSavings is getDashboardSummary's responsibility (via the overwrite loop that calls aggregateActiveCommitmentsPerService). The function comment and the three affected test assertions/functions are updated accordingly. Regression test added: TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments asserts current_savings: 0 for services with recommendations but no purchases.
1 parent 27fdb06 commit cfb8525

2 files changed

Lines changed: 80 additions & 40 deletions

File tree

‎internal/api/handler_dashboard.go‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,12 @@ func (h *Handler) filterDashboardRecommendations(ctx context.Context, session *S
221221
// therefore correct: it projects "how much would I save if I only bought
222222
// RIs to cover X% of my instances" against the 100%-coverage baseline
223223
// that every provider gives us. Verified by TestSummarizeRecommendationsWithCoverage_100PctContract.
224+
//
225+
// Note: CurrentSavings is NOT set here. It represents committed/realized
226+
// savings from active purchase history, populated separately in
227+
// getDashboardSummary via aggregateActiveCommitmentsPerService. A service
228+
// with recommendations but no active purchases correctly ships
229+
// current_savings: 0. (Issue #1031)
224230
func summarizeRecommendationsWithCoverage( //nolint:gocritic // unnamedResult: return names would conflict with body locals
225231
recs []config.RecommendationRecord,
226232
coverageByKey map[string]float64,
@@ -245,17 +251,6 @@ func summarizeRecommendationsWithCoverage( //nolint:gocritic // unnamedResult: r
245251
total += scaled
246252
svc := byService[rep.rec.Service]
247253
svc.PotentialSavings += scaled
248-
// CurrentSavings is the committed/realized monthly savings for the
249-
// service: the full 100%-coverage potential (rec.Savings) projected
250-
// down to the operator-configured coverage %. scaledSavings already
251-
// computes exactly that (rec.Savings * min(coverage,100)/100), so we
252-
// reuse it rather than re-deriving the coverage lookup. When no
253-
// coverage override exists the rec falls through to full savings, so
254-
// CurrentSavings == PotentialSavings (nothing committed-away yet); a
255-
// configured coverage < 100 pulls CurrentSavings below the potential.
256-
// Issue #908: this field was previously never set, so the Home
257-
// chart's current-savings underlay always rendered as $0.
258-
svc.CurrentSavings += scaled
259254
byService[rep.rec.Service] = svc
260255
}
261256
return total, byService

‎internal/api/handler_dashboard_test.go‎

Lines changed: 74 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -265,9 +265,10 @@ func TestSummarizeRecommendationsWithCoverage(t *testing.T) {
265265
total, byService := summarizeRecommendationsWithCoverage(tc.recs, tc.coverage)
266266
assert.InDelta(t, tc.wantTotal, total, 0.0001)
267267
assert.InDelta(t, tc.wantTotal, byService["rds"].PotentialSavings, 0.0001)
268-
// Issue #908: CurrentSavings (committed/realized) is sourced from
269-
// the same coverage-scaled amount, so it tracks the scaled total.
270-
assert.InDelta(t, tc.wantTotal, byService["rds"].CurrentSavings, 0.0001)
268+
// CurrentSavings is populated by getDashboardSummary from purchase
269+
// history, not here — it must remain 0 from this function (issue #1031).
270+
assert.Equal(t, 0.0, byService["rds"].CurrentSavings,
271+
"summarize must not set CurrentSavings")
271272
})
272273
}
273274
}
@@ -322,15 +323,14 @@ func TestSummarizeRecommendationsWithCoverage_100PctContract(t *testing.T) {
322323
"nil coverage map must return un-scaled savings (issue #201 contract)")
323324
}
324325

325-
// TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings is the
326-
// issue #908 regression: by_service[svc].current_savings must be populated
327-
// (and keyed/scaled identically to potential) so the Home chart's
328-
// current-savings underlay renders instead of being a flat $0 series.
329-
//
330-
// Before the fix, summarizeRecommendationsWithCoverage set only
331-
// PotentialSavings, leaving CurrentSavings at its float64 zero value for
332-
// every service regardless of configured coverage.
333-
func TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings(t *testing.T) {
326+
// TestSummarizeRecommendationsWithCoverage_DoesNotSetCurrentSavings asserts
327+
// that summarizeRecommendationsWithCoverage only populates PotentialSavings.
328+
// CurrentSavings represents committed/realized savings from active purchase
329+
// history and is populated separately in getDashboardSummary via
330+
// aggregateActiveCommitmentsPerService. Mixing the two here would cause a
331+
// service with recommendations but no purchases to falsely report non-zero
332+
// CurrentSavings (issue #1031).
333+
func TestSummarizeRecommendationsWithCoverage_DoesNotSetCurrentSavings(t *testing.T) {
334334
acctA := "acct-A"
335335
keyEC2 := config.AccountConfigKey(acctA, "aws", "ec2")
336336
keyRDS := config.AccountConfigKey(acctA, "aws", "rds")
@@ -345,21 +345,18 @@ func TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings(t *testing
345345

346346
_, byService := summarizeRecommendationsWithCoverage(recs, coverage)
347347

348-
// current_savings is non-zero where coverage exists and is scaled the
349-
// same way potential is (rec.Savings * coverage/100), keyed per service.
350-
assert.InDelta(t, 600.0, byService["ec2"].CurrentSavings, 0.001,
351-
"ec2 current_savings = 1000 * 60/100")
352-
assert.InDelta(t, 100.0, byService["rds"].CurrentSavings, 0.001,
353-
"rds current_savings = 400 * 25/100")
354-
355-
// And it matches PotentialSavings (both flow from the same scaled amount).
356-
assert.InDelta(t, byService["ec2"].PotentialSavings, byService["ec2"].CurrentSavings, 0.001)
357-
assert.InDelta(t, byService["rds"].PotentialSavings, byService["rds"].CurrentSavings, 0.001)
358-
359-
// Sanity: every populated service has a strictly positive current_savings
360-
// when coverage is configured (the bug produced 0 here).
361-
require.Positive(t, byService["ec2"].CurrentSavings)
362-
require.Positive(t, byService["rds"].CurrentSavings)
348+
// PotentialSavings is scaled correctly by coverage.
349+
assert.InDelta(t, 600.0, byService["ec2"].PotentialSavings, 0.001,
350+
"ec2 potential_savings = 1000 * 60/100")
351+
assert.InDelta(t, 100.0, byService["rds"].PotentialSavings, 0.001,
352+
"rds potential_savings = 400 * 25/100")
353+
354+
// CurrentSavings is NOT set here — it remains the float64 zero value.
355+
// getDashboardSummary populates it from purchase history (issue #1031).
356+
assert.Equal(t, 0.0, byService["ec2"].CurrentSavings,
357+
"summarize must not set CurrentSavings; that is getDashboardSummary's responsibility")
358+
assert.Equal(t, 0.0, byService["rds"].CurrentSavings,
359+
"summarize must not set CurrentSavings; that is getDashboardSummary's responsibility")
363360
}
364361

365362
// TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell is the
@@ -408,8 +405,8 @@ func TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell(t *testing.
408405
// sum of all variants (100+200+300+50 = 650).
409406
assert.InDelta(t, 350.0, byService["ec2"].PotentialSavings, 0.0001,
410407
"by_service potential must sum per-cell MAX (300+50), not all variants")
411-
assert.InDelta(t, 350.0, byService["ec2"].CurrentSavings, 0.0001,
412-
"by_service current must dedupe identically to potential")
408+
assert.Equal(t, 0.0, byService["ec2"].CurrentSavings,
409+
"summarize must not set CurrentSavings; getDashboardSummary populates it from purchase history")
413410
// The headline total dedupes the same way.
414411
assert.InDelta(t, 350.0, total, 0.0001,
415412
"total must sum per-cell MAX (300+50), not all variants")
@@ -1269,6 +1266,54 @@ func TestHandler_getDashboardSummary_CurrentSavingsJSON(t *testing.T) {
12691266
"current_savings field must carry the active purchase's EstimatedSavings")
12701267
}
12711268

1269+
// TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments is the
1270+
// issue #1031 regression: a service with recommendations but no active purchase
1271+
// history must ship current_savings: 0, not current_savings: potential_savings.
1272+
//
1273+
// The bug was that summarizeRecommendationsWithCoverage set CurrentSavings to
1274+
// the same scaled value as PotentialSavings. getDashboardSummary only overwrites
1275+
// CurrentSavings for services that appear in the purchase-history result, so
1276+
// services with no purchases retained the wrong non-zero value.
1277+
func TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments(t *testing.T) {
1278+
ctx := context.Background()
1279+
1280+
// No purchases at all — represents a new account that has recommendations
1281+
// but has not yet acted on them.
1282+
mockScheduler := new(MockScheduler)
1283+
mockStore := new(MockConfigStore)
1284+
1285+
mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(
1286+
[]config.RecommendationRecord{
1287+
{Service: "EC2", Savings: 500.0},
1288+
{Service: "RDS", Savings: 300.0},
1289+
}, nil)
1290+
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
1291+
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return(
1292+
[]config.PurchaseHistoryRecord{}, nil)
1293+
1294+
mockAuth, req := adminDashboardReq(ctx)
1295+
handler := &Handler{
1296+
auth: mockAuth,
1297+
scheduler: mockScheduler,
1298+
config: mockStore,
1299+
}
1300+
1301+
result, err := handler.getDashboardSummary(ctx, req, map[string]string{})
1302+
require.NoError(t, err)
1303+
1304+
// PotentialSavings must be populated from recommendations.
1305+
assert.InDelta(t, 500.0, result.ByService["EC2"].PotentialSavings, 0.001)
1306+
assert.InDelta(t, 300.0, result.ByService["RDS"].PotentialSavings, 0.001)
1307+
1308+
// CurrentSavings must be 0 — no active commitments exist yet.
1309+
// Before the fix, this incorrectly equalled PotentialSavings because
1310+
// summarizeRecommendationsWithCoverage also wrote CurrentSavings (issue #1031).
1311+
assert.Equal(t, 0.0, result.ByService["EC2"].CurrentSavings,
1312+
"no active commitments: current_savings must be 0, not equal to potential_savings")
1313+
assert.Equal(t, 0.0, result.ByService["RDS"].CurrentSavings,
1314+
"no active commitments: current_savings must be 0, not equal to potential_savings")
1315+
}
1316+
12721317
func TestHandler_calculateCurrentCoverage(t *testing.T) {
12731318
handler := &Handler{}
12741319

0 commit comments

Comments
 (0)