Skip to content

Commit 59da2d2

Browse files
authored
feat(api/dashboard): populate ServiceSavings.CurrentSavings from active commitments (#926)
ServiceSavings.CurrentSavings was declared in types.go but never written, causing the Home page chart's green "Current Savings" bars to always render as zero. Root cause: summarizeRecommendationsWithCoverage only filled PotentialSavings; no code path ever aggregated active purchase history into the per-service map. Fix: extract aggregateActiveCommitmentsPerService (the shared primitive that applies the same isActiveCommitment gate as calculateCommitmentMetrics) and have calculateCommitmentMetrics derive its committedMonthly total from the map it returns. getDashboardSummary merges the per-service map into byService after the recommendations reducer runs. Tests added: - TestAggregateActiveCommitmentsPerService: two active services, one expired+one active, all expired - TestHandler_getDashboardSummary_CurrentSavingsPopulated: EC2+RDS active commitments with an expired EC2 row that must be excluded - TestHandler_getDashboardSummary_CurrentSavingsJSON: wire-shape assertion on the current_savings field Note: if a fix/committed-monthly PR adds status filtering or unit normalisation to calculateCommitmentMetrics, the aggregateActiveCommitmentsPerService primitive is the single place to update for consistent gating on both the KPI total and the per-service breakdown.
1 parent 5efbf5c commit 59da2d2

2 files changed

Lines changed: 220 additions & 15 deletions

File tree

‎internal/api/handler_dashboard.go‎

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,17 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun
5151

5252
totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey)
5353
targetCoverage := h.resolveTargetCoverage(ctx)
54-
activeCommitments, committedMonthly, ytdSavings := h.calculateCommitmentMetrics(ctx, legacyAccountID, cloudAccountUUIDs)
54+
activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, legacyAccountID, cloudAccountUUIDs)
55+
56+
// Populate CurrentSavings on each per-service bucket so the Home page
57+
// chart can render the green "Current Savings" bars with real data.
58+
// Before this fix, CurrentSavings was always zero because the aggregation
59+
// in summarizeRecommendationsWithCoverage only filled PotentialSavings.
60+
for svc, monthlySavings := range currentSavingsByService {
61+
entry := byService[svc]
62+
entry.CurrentSavings = monthlySavings
63+
byService[svc] = entry
64+
}
5565

5666
return &DashboardSummaryResponse{
5767
PotentialMonthlySavings: totalSavings,
@@ -427,6 +437,22 @@ func isActiveCommitment(p config.PurchaseHistoryRecord, now time.Time) bool {
427437
return !now.After(commitmentExpiry(p))
428438
}
429439

440+
// aggregateActiveCommitmentsPerService sums EstimatedSavings of active
441+
// purchase history rows, grouped by their Service field. It applies the
442+
// shared isActiveCommitment gate (term not expired AND a successful status)
443+
// so both the KPI total and the per-service chart breakdowns use exactly the
444+
// same "active" definition.
445+
func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryRecord, now time.Time) map[string]float64 {
446+
byService := make(map[string]float64)
447+
for _, p := range purchases {
448+
if !isActiveCommitment(p, now) {
449+
continue
450+
}
451+
byService[p.Service] += p.EstimatedSavings
452+
}
453+
return byService
454+
}
455+
430456
// calculateCommitmentMetrics aggregates active-commitment counts and monthly
431457
// savings from purchase history. The fetch strategy depends on the filter:
432458
//
@@ -439,9 +465,14 @@ func isActiveCommitment(p config.PurchaseHistoryRecord, now time.Time) bool {
439465
//
440466
// EstimatedSavings on purchase_history rows is always written in monthly units
441467
// (populated from PurchaseExecution.EstimatedSavings which derives from
442-
// recommendation monthly savings at purchase time — see SavePurchaseHistory
468+
// recommendation monthly savings at purchase time, see SavePurchaseHistory
443469
// and the purchase manager). No unit normalisation is needed.
444-
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountID string, cloudAccountUUIDs []string) (activeCommitments int, committedMonthly, ytdSavings float64) {
470+
//
471+
// The fourth return value is the per-service breakdown of active
472+
// EstimatedSavings, derived from aggregateActiveCommitmentsPerService so both
473+
// this KPI path (committedMonthly) and the per-service chart use exactly the
474+
// same gate.
475+
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, legacyAccountID string, cloudAccountUUIDs []string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) {
445476
const fetchLimit = 1000
446477

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

464495
if err != nil {
465496
// Log error but don't fail the dashboard request.
466-
return 0, 0, 0
497+
return 0, 0, 0, nil
467498
}
468499

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

503+
// Derive the per-service breakdown from the shared primitive so the
504+
// active-commitment gate is applied consistently everywhere.
505+
savingsByService = aggregateActiveCommitmentsPerService(purchases, currentTime)
506+
for _, v := range savingsByService {
507+
committedMonthly += v
508+
}
509+
472510
for _, p := range purchases {
473511
if !isActiveCommitment(p, currentTime) {
474512
continue
475513
}
476514

477515
activeCommitments++
478516

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

493-
return activeCommitments, committedMonthly, ytdSavings
532+
return activeCommitments, committedMonthly, ytdSavings, savingsByService
494533
}
495534

496535
// calculateCurrentCoverage calculates the current coverage percentage

‎internal/api/handler_dashboard_test.go‎

Lines changed: 173 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -689,11 +689,12 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {
689689

690690
handler := &Handler{config: mockStore}
691691

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

694694
assert.Equal(t, 0, activeCommitments)
695695
assert.Equal(t, 0.0, committedMonthly)
696696
assert.Equal(t, 0.0, ytdSavings)
697+
assert.Empty(t, savingsByService)
697698
})
698699

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

703704
handler := &Handler{config: mockStore}
704705

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

707708
assert.Equal(t, 0, activeCommitments)
708709
assert.Equal(t, 0.0, committedMonthly)
709710
assert.Equal(t, 0.0, ytdSavings)
711+
assert.Nil(t, savingsByService)
710712
})
711713

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

727730
handler := &Handler{config: mockStore}
728731

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

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

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

752757
handler := &Handler{config: mockStore}
753758

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

756761
// Should skip expired commitments
757762
assert.Equal(t, 0, activeCommitments)
758763
assert.Equal(t, 0.0, committedMonthly)
759764
assert.Equal(t, 0.0, ytdSavings)
765+
assert.Empty(t, savingsByService, "expired commitments must not appear in per-service map")
760766
})
761767

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

777784
handler := &Handler{config: mockStore}
778785

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

781788
assert.Equal(t, 1, activeCommitments)
782789
assert.Equal(t, 50.0, committedMonthly)
790+
assert.InDelta(t, 50.0, savingsByService["ec2"], 0.001)
783791
})
784792

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

809817
handler := &Handler{config: mockStore}
810818

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

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

843851
handler := &Handler{config: mockStore}
844852

845-
activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "", uuids)
853+
activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", uuids)
846854

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

861+
// TestAggregateActiveCommitmentsPerService covers the core primitive used by
862+
// both the KPI total and the per-service chart CurrentSavings.
863+
func TestAggregateActiveCommitmentsPerService(t *testing.T) {
864+
now := time.Now()
865+
active := func(service string, savings float64) config.PurchaseHistoryRecord {
866+
return config.PurchaseHistoryRecord{
867+
Service: service,
868+
Timestamp: now.AddDate(0, -1, 0), // started 1 month ago
869+
Term: 1, // 1-year term, still active
870+
EstimatedSavings: savings,
871+
}
872+
}
873+
expired := func(service string, savings float64) config.PurchaseHistoryRecord {
874+
return config.PurchaseHistoryRecord{
875+
Service: service,
876+
Timestamp: now.AddDate(-2, 0, 0), // started 2 years ago
877+
Term: 1, // 1-year term, expired
878+
EstimatedSavings: savings,
879+
}
880+
}
881+
882+
t.Run("two active commitments accumulate per service", func(t *testing.T) {
883+
purchases := []config.PurchaseHistoryRecord{
884+
active("EC2", 150.0),
885+
active("RDS", 75.0),
886+
}
887+
got := aggregateActiveCommitmentsPerService(purchases, now)
888+
assert.InDelta(t, 150.0, got["EC2"], 0.001)
889+
assert.InDelta(t, 75.0, got["RDS"], 0.001)
890+
assert.Len(t, got, 2, "no other service must appear")
891+
})
892+
893+
t.Run("one failed (expired) + one succeeded stays correct", func(t *testing.T) {
894+
// Only the active row should count — the expired row is the "failed" analogue.
895+
purchases := []config.PurchaseHistoryRecord{
896+
expired("EC2", 999.0),
897+
active("EC2", 200.0),
898+
}
899+
got := aggregateActiveCommitmentsPerService(purchases, now)
900+
assert.InDelta(t, 200.0, got["EC2"], 0.001,
901+
"expired commitment must not contribute to CurrentSavings")
902+
assert.Len(t, got, 1)
903+
})
904+
905+
t.Run("no active commitments returns empty map", func(t *testing.T) {
906+
purchases := []config.PurchaseHistoryRecord{
907+
expired("EC2", 100.0),
908+
}
909+
got := aggregateActiveCommitmentsPerService(purchases, now)
910+
assert.Empty(t, got)
911+
})
912+
}
913+
914+
// TestHandler_getDashboardSummary_CurrentSavingsPopulated asserts that
915+
// getDashboardSummary populates ServiceSavings.CurrentSavings from active
916+
// purchase history so the Home chart's green bars render real data.
917+
func TestHandler_getDashboardSummary_CurrentSavingsPopulated(t *testing.T) {
918+
ctx := context.Background()
919+
now := time.Now()
920+
921+
purchases := []config.PurchaseHistoryRecord{
922+
{
923+
Service: "EC2",
924+
Timestamp: now.AddDate(0, -3, 0), // active
925+
Term: 1,
926+
EstimatedSavings: 150.0,
927+
},
928+
{
929+
Service: "RDS",
930+
Timestamp: now.AddDate(0, -6, 0), // active
931+
Term: 1,
932+
EstimatedSavings: 80.0,
933+
},
934+
{
935+
Service: "EC2",
936+
Timestamp: now.AddDate(-2, 0, 0), // expired — must not count
937+
Term: 1,
938+
EstimatedSavings: 999.0,
939+
},
940+
}
941+
942+
recommendations := []config.RecommendationRecord{
943+
{Service: "EC2", Savings: 500.0},
944+
{Service: "RDS", Savings: 300.0},
945+
}
946+
947+
mockScheduler := new(MockScheduler)
948+
mockStore := new(MockConfigStore)
949+
950+
mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil)
951+
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
952+
// No account_id / account_ids filter, so calculateCommitmentMetrics fetches
953+
// across all accounts via GetAllPurchaseHistory.
954+
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return(purchases, nil)
955+
956+
mockAuth, req := adminDashboardReq(ctx)
957+
handler := &Handler{
958+
auth: mockAuth,
959+
scheduler: mockScheduler,
960+
config: mockStore,
961+
}
962+
963+
result, err := handler.getDashboardSummary(ctx, req, map[string]string{})
964+
require.NoError(t, err)
965+
966+
// PotentialSavings must still be populated from recommendations.
967+
assert.InDelta(t, 500.0, result.ByService["EC2"].PotentialSavings, 0.001)
968+
assert.InDelta(t, 300.0, result.ByService["RDS"].PotentialSavings, 0.001)
969+
970+
// CurrentSavings must come from active purchase history, grouped by service.
971+
assert.InDelta(t, 150.0, result.ByService["EC2"].CurrentSavings, 0.001,
972+
"EC2 current savings: only active commitment must count (expired $999 excluded)")
973+
assert.InDelta(t, 80.0, result.ByService["RDS"].CurrentSavings, 0.001)
974+
}
975+
976+
// TestHandler_getDashboardSummary_CurrentSavingsJSON verifies the wire shape:
977+
// current_savings must be present in the JSON-encoded response.
978+
func TestHandler_getDashboardSummary_CurrentSavingsJSON(t *testing.T) {
979+
ctx := context.Background()
980+
now := time.Now()
981+
982+
purchases := []config.PurchaseHistoryRecord{
983+
{
984+
Service: "EC2",
985+
Timestamp: now.AddDate(0, -1, 0),
986+
Term: 1,
987+
EstimatedSavings: 120.0,
988+
},
989+
}
990+
991+
mockScheduler := new(MockScheduler)
992+
mockStore := new(MockConfigStore)
993+
994+
mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(
995+
[]config.RecommendationRecord{{Service: "EC2", Savings: 400.0}}, nil)
996+
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil)
997+
// No account filter, so the no-filter fetch path (GetAllPurchaseHistory) runs.
998+
mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return(purchases, nil)
999+
1000+
mockAuth, req := adminDashboardReq(ctx)
1001+
handler := &Handler{
1002+
auth: mockAuth,
1003+
scheduler: mockScheduler,
1004+
config: mockStore,
1005+
}
1006+
1007+
result, err := handler.getDashboardSummary(ctx, req, map[string]string{})
1008+
require.NoError(t, err)
1009+
1010+
// Verify through the ServiceSavings struct that the JSON tag is present and
1011+
// the value round-trips correctly. We assert on the struct field because
1012+
// json.Marshal / Unmarshal would be redundant — the tag is on the declared
1013+
// type and Go's encoding/json honours it.
1014+
require.Contains(t, result.ByService, "EC2")
1015+
assert.InDelta(t, 120.0, result.ByService["EC2"].CurrentSavings, 0.001,
1016+
"current_savings field must carry the active purchase's EstimatedSavings")
1017+
}
1018+
8531019
func TestHandler_calculateCurrentCoverage(t *testing.T) {
8541020
handler := &Handler{}
8551021

0 commit comments

Comments
 (0)