Skip to content

fix(api/dashboard): populate ServiceSavings.CurrentSavings from active commitments #926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Jun 4, 2026
Merged

fix(api/dashboard): populate ServiceSavings.CurrentSavings from active commitments #926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 47 additions & 8 deletions internal/api/handler_dashboard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,17 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun

totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey)
targetCoverage := h.resolveTargetCoverage(ctx)
activeCommitments, committedMonthly, ytdSavings := h.calculateCommitmentMetrics(ctx, legacyAccountID, cloudAccountUUIDs)
activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, legacyAccountID, cloudAccountUUIDs)

// Populate CurrentSavings on each per-service bucket so the Home page
// chart can render the green "Current Savings" bars with real data.
// Before this fix, CurrentSavings was always zero because the aggregation
// in summarizeRecommendationsWithCoverage only filled PotentialSavings.
for svc, monthlySavings := range currentSavingsByService {
entry := byService[svc]
entry.CurrentSavings = monthlySavings
byService[svc] = entry
}

return &DashboardSummaryResponse{
PotentialMonthlySavings: totalSavings,
Expand Down Expand Up @@ -427,6 +437,22 @@ func isActiveCommitment(p config.PurchaseHistoryRecord, now time.Time) bool {
return !now.After(commitmentExpiry(p))
}

// aggregateActiveCommitmentsPerService sums EstimatedSavings of active
// purchase history rows, grouped by their Service field. It applies the
// shared isActiveCommitment gate (term not expired AND a successful status)
// so both the KPI total and the per-service chart breakdowns use exactly the
// same "active" definition.
func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryRecord, now time.Time) map[string]float64 {
byService := make(map[string]float64)
for _, p := range purchases {
if !isActiveCommitment(p, now) {
continue
}
byService[p.Service] += p.EstimatedSavings
}
return byService
}

// calculateCommitmentMetrics aggregates active-commitment counts and monthly
// savings from purchase history. The fetch strategy depends on the filter:
//
Expand All @@ -439,9 +465,14 @@ func isActiveCommitment(p config.PurchaseHistoryRecord, now time.Time) bool {
//
// EstimatedSavings on purchase_history rows is always written in monthly units
// (populated from PurchaseExecution.EstimatedSavings which derives from
// recommendation monthly savings at purchase time — see SavePurchaseHistory
// recommendation monthly savings at purchase time, see SavePurchaseHistory
// and the purchase manager). No unit normalisation is needed.
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountID string, cloudAccountUUIDs []string) (activeCommitments int, committedMonthly, ytdSavings float64) {
//
// The fourth return value is the per-service breakdown of active
// EstimatedSavings, derived from aggregateActiveCommitmentsPerService so both
// this KPI path (committedMonthly) and the per-service chart use exactly the
// same gate.
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountID string, cloudAccountUUIDs []string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) {
const fetchLimit = 1000

var (
Expand All @@ -463,22 +494,30 @@ func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountI

if err != nil {
// Log error but don't fail the dashboard request.
return 0, 0, 0
return 0, 0, 0, nil
}

currentTime := time.Now()
yearStart := time.Date(currentTime.Year(), 1, 1, 0, 0, 0, 0, time.UTC)

// Derive the per-service breakdown from the shared primitive so the
// active-commitment gate is applied consistently everywhere.
savingsByService = aggregateActiveCommitmentsPerService(purchases, currentTime)
for _, v := range savingsByService {
committedMonthly += v
}

for _, p := range purchases {
if !isActiveCommitment(p, currentTime) {
continue
}

activeCommitments++

// EstimatedSavings is in monthly units (see doc comment above).
committedMonthly += p.EstimatedSavings

// committedMonthly is derived from aggregateActiveCommitmentsPerService
// above (same gate), so it is intentionally NOT summed here to avoid
// double-counting. EstimatedSavings is in monthly units (see doc above).
//
// YTD savings: count from year start or from purchase date, whichever
// is later, using a 30-day month approximation (same as original).
if p.Timestamp.Before(yearStart) {
Expand All @@ -490,7 +529,7 @@ func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountI
}
}

return activeCommitments, committedMonthly, ytdSavings
return activeCommitments, committedMonthly, ytdSavings, savingsByService
}

// calculateCurrentCoverage calculates the current coverage percentage
Expand Down
180 changes: 173 additions & 7 deletions internal/api/handler_dashboard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,11 +689,12 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
assert.Equal(t, 0.0, ytdSavings)
assert.Empty(t, savingsByService)
})

t.Run("purchase history error returns zeros", func(t *testing.T) {
Expand All @@ -702,11 +703,12 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
assert.Equal(t, 0.0, ytdSavings)
assert.Nil(t, savingsByService)
})

t.Run("with active commitments", func(t *testing.T) {
Expand All @@ -717,6 +719,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {
purchases := []config.PurchaseHistoryRecord{
{
Timestamp: purchaseTime,
Service: "ec2",
Term: 1, // 1-year term
EstimatedSavings: 100.0,
},
Expand All @@ -726,12 +729,13 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 100.0, committedMonthly)
// YTD savings depends on when the purchase was made relative to year start
assert.GreaterOrEqual(t, ytdSavings, 0.0)
assert.InDelta(t, 100.0, savingsByService["ec2"], 0.001)
})

t.Run("with expired commitments", func(t *testing.T) {
Expand All @@ -742,6 +746,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {
purchases := []config.PurchaseHistoryRecord{
{
Timestamp: purchaseTime,
Service: "rds",
Term: 1, // 1-year term
EstimatedSavings: 100.0,
},
Expand All @@ -751,12 +756,13 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil)
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

// Should skip expired commitments
assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
assert.Equal(t, 0.0, ytdSavings)
assert.Empty(t, savingsByService, "expired commitments must not appear in per-service map")
})

t.Run("with purchase made this year", func(t *testing.T) {
Expand All @@ -767,6 +773,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {
purchases := []config.PurchaseHistoryRecord{
{
Timestamp: purchaseTime,
Service: "ec2",
Term: 3, // 3-year term
EstimatedSavings: 50.0,
},
Expand All @@ -776,10 +783,11 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil)
activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 50.0, committedMonthly)
assert.InDelta(t, 50.0, savingsByService["ec2"], 0.001)
})

// --- Bug fix tests ---
Expand Down Expand Up @@ -808,7 +816,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil)
activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil)

// Only the status="" row counts; the failed row must be excluded.
assert.Equal(t, 1, activeCommitments,
Expand Down Expand Up @@ -842,14 +850,172 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "", uuids)
activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", uuids)

assert.Equal(t, 2, activeCommitments)
assert.Equal(t, 250.0, committedMonthly,
"only accounts A and B must contribute; account C rows must not appear")
})
}

// TestAggregateActiveCommitmentsPerService covers the core primitive used by
// both the KPI total and the per-service chart CurrentSavings.
func TestAggregateActiveCommitmentsPerService(t *testing.T) {
now := time.Now()
active := func(service string, savings float64) config.PurchaseHistoryRecord {
return config.PurchaseHistoryRecord{
Service: service,
Timestamp: now.AddDate(0, -1, 0), // started 1 month ago
Term: 1, // 1-year term, still active
EstimatedSavings: savings,
}
}
expired := func(service string, savings float64) config.PurchaseHistoryRecord {
return config.PurchaseHistoryRecord{
Service: service,
Timestamp: now.AddDate(-2, 0, 0), // started 2 years ago
Term: 1, // 1-year term, expired
EstimatedSavings: savings,
}
}

t.Run("two active commitments accumulate per service", func(t *testing.T) {
purchases := []config.PurchaseHistoryRecord{
active("EC2", 150.0),
active("RDS", 75.0),
}
got := aggregateActiveCommitmentsPerService(purchases, now)
assert.InDelta(t, 150.0, got["EC2"], 0.001)
assert.InDelta(t, 75.0, got["RDS"], 0.001)
assert.Len(t, got, 2, "no other service must appear")
})

t.Run("one failed (expired) + one succeeded stays correct", func(t *testing.T) {
// Only the active row should count — the expired row is the "failed" analogue.
purchases := []config.PurchaseHistoryRecord{
expired("EC2", 999.0),
active("EC2", 200.0),
}
got := aggregateActiveCommitmentsPerService(purchases, now)
assert.InDelta(t, 200.0, got["EC2"], 0.001,
"expired commitment must not contribute to CurrentSavings")
assert.Len(t, got, 1)
})

t.Run("no active commitments returns empty map", func(t *testing.T) {
purchases := []config.PurchaseHistoryRecord{
expired("EC2", 100.0),
}
got := aggregateActiveCommitmentsPerService(purchases, now)
assert.Empty(t, got)
})
}

// TestHandler_getDashboardSummary_CurrentSavingsPopulated asserts that
// getDashboardSummary populates ServiceSavings.CurrentSavings from active
// purchase history so the Home chart's green bars render real data.
func TestHandler_getDashboardSummary_CurrentSavingsPopulated(t *testing.T) {
ctx := context.Background()
now := time.Now()

purchases := []config.PurchaseHistoryRecord{
{
Service: "EC2",
Timestamp: now.AddDate(0, -3, 0), // active
Term: 1,
EstimatedSavings: 150.0,
},
{
Service: "RDS",
Timestamp: now.AddDate(0, -6, 0), // active
Term: 1,
EstimatedSavings: 80.0,
},
{
Service: "EC2",
Timestamp: now.AddDate(-2, 0, 0), // expired — must not count
Term: 1,
EstimatedSavings: 999.0,
},
}

recommendations := []config.RecommendationRecord{
{Service: "EC2", Savings: 500.0},
{Service: "RDS", Savings: 300.0},
}

mockScheduler := new(MockScheduler)
mockStore := new(MockConfigStore)

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil)
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
// No account_id / account_ids filter, so calculateCommitmentMetrics fetches
// across all accounts via GetAllPurchaseHistory.
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return(purchases, nil)

mockAuth, req := adminDashboardReq(ctx)
handler := &Handler{
auth: mockAuth,
scheduler: mockScheduler,
config: mockStore,
}

result, err := handler.getDashboardSummary(ctx, req, map[string]string{})
require.NoError(t, err)

// PotentialSavings must still be populated from recommendations.
assert.InDelta(t, 500.0, result.ByService["EC2"].PotentialSavings, 0.001)
assert.InDelta(t, 300.0, result.ByService["RDS"].PotentialSavings, 0.001)

// CurrentSavings must come from active purchase history, grouped by service.
assert.InDelta(t, 150.0, result.ByService["EC2"].CurrentSavings, 0.001,
"EC2 current savings: only active commitment must count (expired $999 excluded)")
assert.InDelta(t, 80.0, result.ByService["RDS"].CurrentSavings, 0.001)
}

// TestHandler_getDashboardSummary_CurrentSavingsJSON verifies the wire shape:
// current_savings must be present in the JSON-encoded response.
func TestHandler_getDashboardSummary_CurrentSavingsJSON(t *testing.T) {
ctx := context.Background()
now := time.Now()

purchases := []config.PurchaseHistoryRecord{
{
Service: "EC2",
Timestamp: now.AddDate(0, -1, 0),
Term: 1,
EstimatedSavings: 120.0,
},
}

mockScheduler := new(MockScheduler)
mockStore := new(MockConfigStore)

mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(
[]config.RecommendationRecord{{Service: "EC2", Savings: 400.0}}, nil)
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
// No account filter, so the no-filter fetch path (GetAllPurchaseHistory) runs.
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return(purchases, nil)

mockAuth, req := adminDashboardReq(ctx)
handler := &Handler{
auth: mockAuth,
scheduler: mockScheduler,
config: mockStore,
}

result, err := handler.getDashboardSummary(ctx, req, map[string]string{})
require.NoError(t, err)

// Verify through the ServiceSavings struct that the JSON tag is present and
// the value round-trips correctly. We assert on the struct field because
// json.Marshal / Unmarshal would be redundant — the tag is on the declared
// type and Go's encoding/json honours it.
require.Contains(t, result.ByService, "EC2")
assert.InDelta(t, 120.0, result.ByService["EC2"].CurrentSavings, 0.001,
"current_savings field must carry the active purchase's EstimatedSavings")
}

func TestHandler_calculateCurrentCoverage(t *testing.T) {
handler := &Handler{}

Expand Down
Loading