From 16e1e1816e0ec3351f72e4ce2e384c483c383218 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 20:54:39 +0200 Subject: [PATCH] fix(api/dashboard): tighten committed_monthly aggregation (status + unit + account scope) Three bugs inflated the committed_monthly KPI on the Home page: 1. Status filter: isActiveCommitment now rejects rows whose Status field is non-empty and not "completed". DB-backed purchase_history rows always read back with Status="" (the column is not persisted); only synthesised rows from failed/cancelled/expired executions carry a non-empty status. Those were being counted as active commitments. 2. Unit verification: documented that EstimatedSavings is always written in monthly units (traces from PurchaseExecution.EstimatedSavings which derives from monthly recommendation savings). No runtime normalisation needed; comment added to prevent future ambiguity. 3. Multi-account scope: resolveDashboardAccountID was passing empty string to GetPurchaseHistory when account_ids had multiple entries, yielding WHERE account_id='' which returns no rows. Replaced with resolveDashboardAccountScope that routes to GetPurchaseHistoryFiltered (cloud_account_id = ANY($1)) for UUID-based filters, GetPurchaseHistory for the legacy account_id param, and GetAllPurchaseHistory when no filter is set. Also fixes the single-UUID case which was passing a UUID to the account_id (VARCHAR) column. New tests: status=failed row excluded; multi-account UUID filter routes to GetPurchaseHistoryFiltered and excludes out-of-scope accounts; no-filter path uses GetAllPurchaseHistory. --- internal/api/handler_dashboard.go | 120 +++++++++++++----- internal/api/handler_dashboard_test.go | 88 +++++++++++-- .../api/handler_per_account_perms_test.go | 4 +- internal/api/handler_test.go | 3 +- 4 files changed, 167 insertions(+), 48 deletions(-) diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 763a8d969..3533e992f 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -22,7 +22,7 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun return nil, err } - effectiveAccountID, err := resolveDashboardAccountID(params) + legacyAccountID, cloudAccountUUIDs, err := resolveDashboardAccountScope(params) if err != nil { return nil, err } @@ -51,7 +51,7 @@ 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, effectiveAccountID) + activeCommitments, committedMonthly, ytdSavings := h.calculateCommitmentMetrics(ctx, legacyAccountID, cloudAccountUUIDs) return &DashboardSummaryResponse{ PotentialMonthlySavings: totalSavings, @@ -65,26 +65,36 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun }, nil } -// resolveDashboardAccountID chooses the single account_id passed into the -// (legacy, single-account) commitment metrics calculation: -// - one ID in `account_ids` → use it -// - multiple IDs → empty (no filter; logged as observability) -// - none → fall back to the legacy singular `account_id` param -func resolveDashboardAccountID(params map[string]string) (string, error) { - accountIDs, err := parseAccountIDs(params["account_ids"]) - if err != nil { - return "", NewClientError(400, err.Error()) +// resolveDashboardAccountScope parses the account filter params and returns +// the two disjoint representations used by commitment-metrics fetching: +// +// - legacyAccountID: the singular `account_id` param (cloud-provider account +// number, e.g. "123456789012"), passed as-is to GetPurchaseHistory when +// no UUID-based filter is present. Empty string means "no legacy filter". +// +// - cloudAccountUUIDs: UUIDs from the `account_ids` param, matched against +// purchase_history.cloud_account_id via GetPurchaseHistoryFiltered. Non-nil +// (even if empty) when `account_ids` was present and valid. +// +// Exactly one of legacyAccountID or cloudAccountUUIDs is meaningful per call: +// when account_ids is supplied, legacyAccountID is set to "" so the caller +// uses the UUID path; when only account_id is supplied, cloudAccountUUIDs is +// nil so the caller uses the legacy path; when neither is supplied, both are +// zero-valued and the caller fetches all history. +func resolveDashboardAccountScope(params map[string]string) (legacyAccountID string, cloudAccountUUIDs []string, err error) { + uuids, parseErr := parseAccountIDs(params["account_ids"]) + if parseErr != nil { + return "", nil, NewClientError(400, parseErr.Error()) } - switch len(accountIDs) { - case 1: - return accountIDs[0], nil - case 0: - return params["account_id"], nil - default: - logging.Infof("dashboard: multi-account filter requested (%d accounts); returning unfiltered metrics until per-account breakdown is implemented", len(accountIDs)) - return "", nil + if len(uuids) > 0 { + // UUID-based multi-account filter — takes precedence over legacy param. + return "", uuids, nil } + + // No UUID filter: fall back to the legacy singular cloud-provider account + // number. Empty string means "fetch all accounts" (no filter). + return params["account_id"], nil, nil } // filterDashboardRecommendations applies the session's allowed_accounts filter @@ -396,45 +406,85 @@ func commitmentExpiry(p config.PurchaseHistoryRecord) time.Time { return p.Timestamp.Add(termDuration) } -// isActiveCommitment reports whether the purchase's term has not yet -// expired as of `now`. The boundary is strict (After): a commitment is -// active right up to the instant its term ends. Same predicate shared -// by the dashboard aggregate and the per-commitment inventory endpoint. +// isActiveCommitment reports whether the purchase is active: its term has not +// yet expired as of `now` AND its status is one of the successful terminal +// states ("" for DB-backed rows where the column is unpersisted, or +// "completed"). Rows synthesised from failed/cancelled/expired executions +// carry a non-empty status other than "completed" and are excluded so they +// do not inflate the committed_monthly KPI. The boundary is strict (After): +// a commitment is active right up to the instant its term ends. +// +// Same predicate shared by the dashboard aggregate and the per-commitment +// inventory endpoint. Status values: see PurchaseHistoryRecord.Status doc. func isActiveCommitment(p config.PurchaseHistoryRecord, now time.Time) bool { + // Status is unpersisted (dynamodbav:"-"); DB rows always read back as "". + // Synthesised rows set it to "failed", "expired", "cancelled", "pending", + // "notified", "approved", "running", or "paused". Only "" and "completed" + // represent a commitment that is actually live on the provider. + if p.Status != "" && p.Status != "completed" { + return false + } return !now.After(commitmentExpiry(p)) } -// calculateCommitmentMetrics calculates active commitments and savings from purchase history -func (h *Handler) calculateCommitmentMetrics(ctx context.Context, accountID string) (activeCommitments int, committedMonthly, ytdSavings float64) { - // Get purchase history (last 1000 purchases should be sufficient) - purchases, err := h.config.GetPurchaseHistory(ctx, accountID, 1000) +// calculateCommitmentMetrics aggregates active-commitment counts and monthly +// savings from purchase history. The fetch strategy depends on the filter: +// +// - cloudAccountUUIDs non-empty: GetPurchaseHistoryFiltered scoped to those +// cloud_account_id UUIDs (multi-account UUID filter from `account_ids`). +// - legacyAccountID non-empty: GetPurchaseHistory filtered by account_id +// (cloud-provider account number, e.g. "123456789012"; legacy single-account +// filter from the `account_id` param). +// - both empty: GetAllPurchaseHistory (no account filter). +// +// 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 +// 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) { + const fetchLimit = 1000 + + var ( + purchases []config.PurchaseHistoryRecord + err error + ) + + switch { + case len(cloudAccountUUIDs) > 0: + // Multi-account UUID filter: match purchase_history.cloud_account_id. + purchases, err = h.config.GetPurchaseHistoryFiltered(ctx, "", cloudAccountUUIDs, nil, nil, fetchLimit) + case legacyAccountID != "": + // Legacy single-account filter: match purchase_history.account_id. + purchases, err = h.config.GetPurchaseHistory(ctx, legacyAccountID, fetchLimit) + default: + // No account filter: fetch across all accounts. + purchases, err = h.config.GetAllPurchaseHistory(ctx, fetchLimit) + } + if err != nil { - // Log error but don't fail the request + // Log error but don't fail the dashboard request. return 0, 0, 0 } - // Get current time from context or use now currentTime := time.Now() yearStart := time.Date(currentTime.Year(), 1, 1, 0, 0, 0, 0, time.UTC) for _, p := range purchases { if !isActiveCommitment(p, currentTime) { - continue // Skip expired commitments + continue } - // Count active commitments activeCommitments++ - // Add to committed monthly (EstimatedSavings is typically monthly) + // EstimatedSavings is in monthly units (see doc comment above). committedMonthly += p.EstimatedSavings - // Calculate YTD savings (savings accumulated since year start) + // 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) { - // Purchase made before this year, count full year so far monthsSinceYearStart := int(currentTime.Sub(yearStart).Hours() / (24 * 30)) ytdSavings += p.EstimatedSavings * float64(monthsSinceYearStart) } else { - // Purchase made this year, count from purchase date monthsSincePurchase := int(currentTime.Sub(p.Timestamp).Hours() / (24 * 30)) ytdSavings += p.EstimatedSavings * float64(monthsSincePurchase) } diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 38a869cdc..15e017388 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -56,7 +56,8 @@ func TestHandler_getDashboardSummary(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) - mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + // No account_id / account_ids filter → calculateCommitmentMetrics uses GetAllPurchaseHistory. + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ @@ -117,7 +118,7 @@ func TestHandler_getDashboardSummary_PerAccountCoverageScalesSavings(t *testing. mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) - mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(&config.ServiceConfig{ Provider: "aws", Service: "rds", Enabled: true, Coverage: 100, }, nil) @@ -160,7 +161,7 @@ func TestHandler_getDashboardSummary_ZeroCoverageInServiceConfigFallsThroughToFu mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) - mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) // Global ServiceConfig has Coverage=0 (zero-value — operator never set it). mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return(&config.ServiceConfig{ Provider: "aws", Service: "rds", Enabled: true, Coverage: 0, @@ -688,7 +689,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123") + activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil) assert.Equal(t, 0, activeCommitments) assert.Equal(t, 0.0, committedMonthly) @@ -701,7 +702,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123") + activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil) assert.Equal(t, 0, activeCommitments) assert.Equal(t, 0.0, committedMonthly) @@ -725,7 +726,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123") + activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 100.0, committedMonthly) @@ -750,7 +751,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123") + activeCommitments, committedMonthly, ytdSavings := handler.calculateCommitmentMetrics(ctx, "account-123", nil) // Should skip expired commitments assert.Equal(t, 0, activeCommitments) @@ -775,11 +776,78 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123") + activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 50.0, committedMonthly) }) + + // --- Bug fix tests --- + + // Status filter: a row with Status="failed" must NOT be counted as an active + // commitment. The invariant: isActiveCommitment rejects any non-empty status + // that isn't "completed". + t.Run("failed status row is excluded from committedMonthly", func(t *testing.T) { + mockStore := new(MockConfigStore) + purchaseTime := time.Now().AddDate(0, -3, 0) + purchases := []config.PurchaseHistoryRecord{ + { + Timestamp: purchaseTime, + Term: 1, + EstimatedSavings: 200.0, + Status: "failed", + }, + { + Timestamp: purchaseTime, + Term: 1, + EstimatedSavings: 50.0, + // Status "" = completed DB row — must be counted + }, + } + mockStore.On("GetPurchaseHistory", ctx, "account-123", 1000).Return(purchases, nil) + + handler := &Handler{config: mockStore} + + activeCommitments, committedMonthly, _ := handler.calculateCommitmentMetrics(ctx, "account-123", nil) + + // Only the status="" row counts; the failed row must be excluded. + assert.Equal(t, 1, activeCommitments, + "failed commitment must not increment activeCommitments") + assert.Equal(t, 50.0, committedMonthly, + "failed commitment's savings must not appear in committedMonthly") + }) + + // Multi-account scope: a UUID-filtered request must use GetPurchaseHistoryFiltered + // scoped to the supplied cloud_account_id UUIDs. Rows from account C must NOT + // appear when the filter contains only A and B. + t.Run("multi-account UUID filter routes to GetPurchaseHistoryFiltered", func(t *testing.T) { + mockStore := new(MockConfigStore) + purchaseTime := time.Now().AddDate(0, -2, 0) + + accountAID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + accountBID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + + // Only rows for A and B are returned by the scoped query — the store + // enforces the filter; the handler must NOT call GetPurchaseHistory. + purchasesAB := []config.PurchaseHistoryRecord{ + {CloudAccountID: &accountAID, Timestamp: purchaseTime, Term: 1, EstimatedSavings: 100.0}, + {CloudAccountID: &accountBID, Timestamp: purchaseTime, Term: 1, EstimatedSavings: 150.0}, + } + uuids := []string{accountAID, accountBID} + mockStore.On("GetPurchaseHistoryFiltered", ctx, "", uuids, (*time.Time)(nil), (*time.Time)(nil), 1000). + Return(purchasesAB, nil) + // GetPurchaseHistory must not be called — no On registration so it + // would panic if accidentally invoked. + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{config: mockStore} + + 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") + }) } func TestHandler_calculateCurrentCoverage(t *testing.T) { @@ -827,7 +895,7 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) mockStore.On("GetGlobalConfig", ctx).Return(nil, nil) - mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ @@ -851,7 +919,7 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return([]config.RecommendationRecord{}, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) - mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockAuth, req := adminDashboardReq(ctx) handler := &Handler{ diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 56a600438..5c328c9c4 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -490,8 +490,8 @@ func TestPerAccountPerms_DashboardSummary_AggregatesAllowedSubsetOnly(t *testing // Guard against future code paths that resolve service configs before filtering: // stub rds so an unexpected GetServiceConfig call doesn't panic the test. mockStore.On("GetServiceConfig", ctx, "aws", "rds").Return((*config.ServiceConfig)(nil), nil) - // calculateCommitmentMetrics calls GetPurchaseHistory for YTD/committed totals. - mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + // calculateCommitmentMetrics calls GetAllPurchaseHistory when no account filter is set. + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { return permsAccountList(), nil } diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index ffdf6fdd0..81f63d127 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -918,7 +918,8 @@ func TestHandler_HandleRequest_GetDashboardSummary(t *testing.T) { mockScheduler.On("ListRecommendations", ctx, mock.Anything).Return(recommendations, nil) mockStore.On("GetGlobalConfig", ctx).Return(globalCfg, nil) - mockStore.On("GetPurchaseHistory", ctx, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) + // No account_id / account_ids filter → calculateCommitmentMetrics uses GetAllPurchaseHistory. + mockStore.On("GetAllPurchaseHistory", ctx, mock.Anything).Return([]config.PurchaseHistoryRecord{}, nil) handler := &Handler{ scheduler: mockScheduler,