diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index b174c3ae..c2399940 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -56,7 +56,14 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey) targetCoverage := h.resolveTargetCoverage(ctx) - activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider) + activeCommitments, committedMonthly, ytdSavings, currentSavingsByService, err := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider) + if err != nil { + // Issue #72: purchase history is a hard dependency of the commitment + // KPIs, same as recommendations are of the savings KPIs above. Fail + // the request rather than ship $0 committed / 0 commitments, which + // would be indistinguishable from a real empty portfolio. + return nil, err + } // Populate CurrentSavings on each per-service bucket from actual // purchase_history commitments. Services with recommendations but no @@ -617,12 +624,13 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco // calculateCommitmentMetrics aggregates, applying the pre-resolved account // scope and an optional provider filter. Extracted to keep the parent under the // cyclomatic limit (mirrors the appendAccountPredicate / accountMatchesFilters -// pattern). Returns ok=false on a store error or an explicit zero-account scope -// so the caller emits zeroed KPIs without querying. +// pattern). Returns (nil, nil) for an explicit zero-account scope, so the +// caller emits genuinely-zero KPIs without querying, and a wrapped error on a +// store failure (issue #72: a failed read must never be reported as $0). // // - non-nil but empty accountUUIDs (no external groups): explicit "scoped to // zero accounts" sentinel (a restricted session whose allowed_accounts match -// nothing). Returns (nil, false) WITHOUT querying so a scoped user never sees +// nothing). Returns (nil, nil) WITHOUT querying so a scoped user never sees // all-accounts data (issue #956). A nil/empty filter sent to the store would // match every row (no WHERE clause), so this must short-circuit. // - otherwise: GetActivePurchaseHistory with the dual-column account predicate @@ -633,15 +641,14 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco // - provider: when non-empty, only purchases for that provider are returned, // mirroring fetchCommitmentRecords' in-memory provider filter so KPIs match // the provider chip selection. -func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) { +func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, error) { if accountUUIDs != nil && len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 { - return nil, false + return nil, nil } purchases, err := h.config.GetActivePurchaseHistory(ctx, asOf, accountUUIDs, accountExternalIDsByProvider) if err != nil { - logging.Errorf("dashboard: failed to fetch commitment purchases; KPIs will be zeroed: %v", err) - return nil, false + return nil, fmt.Errorf("failed to get active purchase history: %w", err) } if provider != "" { @@ -654,7 +661,7 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, purchases = filtered } - return purchases, true + return purchases, nil } // calculateCommitmentMetrics aggregates active-commitment counts and monthly @@ -677,11 +684,13 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, // 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, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) { +// +// A store error is returned, never reported as zeroed metrics (issue #72). +func (h *Handler) calculateCommitmentMetrics(ctx context.Context, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64, err error) { currentTime := time.Now() - purchases, ok := h.fetchCommitmentPurchases(ctx, currentTime, provider, accountUUIDs, accountExternalIDsByProvider) - if !ok { - return 0, 0, 0, nil + purchases, err := h.fetchCommitmentPurchases(ctx, currentTime, provider, accountUUIDs, accountExternalIDsByProvider) + if err != nil { + return 0, 0, 0, nil, err } yearStart := time.Date(currentTime.Year(), 1, 1, 0, 0, 0, 0, time.UTC) @@ -717,7 +726,7 @@ func (h *Handler) calculateCommitmentMetrics(ctx context.Context, provider strin ytdSavings += p.EstimatedSavings * float64(elapsedWholeMonths(countFrom, currentTime)) } - return activeCommitments, committedMonthly, ytdSavings, savingsByService + return activeCommitments, committedMonthly, ytdSavings, savingsByService, nil } // elapsedWholeMonths returns the number of complete calendar months between @@ -736,16 +745,20 @@ func elapsedWholeMonths(from, to time.Time) int { return months } -// calculateCurrentCoverage calculates the current coverage percentage. -func (h *Handler) calculateCurrentCoverage(potentialSavings, committedMonthly float64) float64 { - if potentialSavings == 0 { - return 100.0 // No recommendations means 100% coverage - } - +// calculateCurrentCoverage returns committed / (committed + potential) as a +// percentage, or nil when both are zero and there is nothing to measure. +// +// It used to return a hard-coded 100% whenever potentialSavings was 0, even +// with nothing committed, so an empty or zeroed commitment half rendered as a +// reassuring "100% coverage, $0 committed" (issue #72). Committed savings with +// no remaining recommendations still compute to 100% through the ratio; only +// the 0/0 case is now reported as unknown (JSON null) rather than invented. +func (h *Handler) calculateCurrentCoverage(potentialSavings, committedMonthly float64) *float64 { totalPossible := potentialSavings + committedMonthly if totalPossible == 0 { - return 0 + return nil } - return (committedMonthly / totalPossible) * 100 + coverage := (committedMonthly / totalPossible) * 100 + return &coverage } diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 58934c38..867697bc 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -852,7 +852,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + activeCommitments, committedMonthly, ytdSavings, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + require.NoError(t, err) assert.Equal(t, 0, activeCommitments) assert.Equal(t, 0.0, committedMonthly) @@ -860,18 +861,17 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { assert.Empty(t, savingsByService) }) - t.Run("purchase history error returns zeros", func(t *testing.T) { + // Issue #72: a store error must be returned, not reported as zeroed KPIs. + t.Run("purchase history error is returned, not zeroed", func(t *testing.T) { mockStore := new(MockConfigStore) - mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(nil, errors.New("db error")) + storeErr := errors.New("db error") + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), []string(nil), map[string][]string{"": {"account-123"}}).Return(nil, storeErr) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) - - assert.Equal(t, 0, activeCommitments) - assert.Equal(t, 0.0, committedMonthly) - assert.Equal(t, 0.0, ytdSavings) - assert.Nil(t, savingsByService) + _, _, _, _, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + require.Error(t, err) + assert.ErrorIs(t, err, storeErr) }) t.Run("with active commitments", func(t *testing.T) { @@ -892,7 +892,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + activeCommitments, committedMonthly, ytdSavings, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + require.NoError(t, err) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 100.0, committedMonthly) @@ -919,7 +920,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + activeCommitments, committedMonthly, ytdSavings, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + require.NoError(t, err) // Should skip expired commitments assert.Equal(t, 0, activeCommitments) @@ -946,7 +948,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + require.NoError(t, err) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 50.0, committedMonthly) @@ -979,7 +982,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + activeCommitments, committedMonthly, _, _, err := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) + require.NoError(t, err) // Only the status="" row counts; the failed row must be excluded. assert.Equal(t, 1, activeCommitments, @@ -1013,7 +1017,8 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", uuids, nil) + activeCommitments, committedMonthly, _, _, err := handler.calculateCommitmentMetrics(ctx, "", uuids, nil) + require.NoError(t, err) assert.Equal(t, 2, activeCommitments) assert.Equal(t, 250.0, committedMonthly, @@ -1040,8 +1045,9 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics( + activeCommitments, committedMonthly, _, _, err := handler.calculateCommitmentMetrics( ctx, "", []string{"bbbbbbbb-1111-2222-3333-444444444444"}, map[string][]string{"aws": {"999988887777"}}) + require.NoError(t, err) assert.Equal(t, 1, activeCommitments, "external-id-only commitment must be counted") assert.Equal(t, 175.0, committedMonthly) @@ -1103,7 +1109,8 @@ func TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000(t *testing.T) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, nil) + activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics(ctx, "", nil, nil) + require.NoError(t, err) assert.Equal(t, 1, activeCommitments, "the still-active 3y commitment beyond the old 1000-row cap must be counted") @@ -1327,27 +1334,94 @@ func TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments(t *test func TestHandler_calculateCurrentCoverage(t *testing.T) { handler := &Handler{} - t.Run("no potential savings returns 100%", func(t *testing.T) { + t.Run("no potential savings with commitments returns 100%", func(t *testing.T) { coverage := handler.calculateCurrentCoverage(0.0, 100.0) - assert.Equal(t, 100.0, coverage) + require.NotNil(t, coverage) + assert.Equal(t, 100.0, *coverage) }) t.Run("no committed monthly", func(t *testing.T) { coverage := handler.calculateCurrentCoverage(100.0, 0.0) - assert.Equal(t, 0.0, coverage) + require.NotNil(t, coverage) + assert.Equal(t, 0.0, *coverage) }) t.Run("50% coverage", func(t *testing.T) { coverage := handler.calculateCurrentCoverage(100.0, 100.0) - assert.Equal(t, 50.0, coverage) + require.NotNil(t, coverage) + assert.Equal(t, 50.0, *coverage) }) - t.Run("both zero returns 0%", func(t *testing.T) { - coverage := handler.calculateCurrentCoverage(0.0, 0.0) - assert.Equal(t, 100.0, coverage) + // Issue #72: with nothing committed and nothing left to buy there is no + // ratio to report. The old code returned a fabricated 100%. + t.Run("both zero returns nil, not 100%", func(t *testing.T) { + assert.Nil(t, handler.calculateCurrentCoverage(0.0, 0.0)) }) } +// TestHandleRequest_DashboardSummary_PurchaseHistoryErrorIs500 is the issue #72 +// regression test, through the router so the status code and body are pinned. +// It uses the issue's compounding scenario: no recommendations (potential +// savings 0) plus a purchase-history store error. Before the fix this returned +// 200 with active_commitments 0, committed_monthly 0, ytd_savings 0 and +// current_coverage 100: a fabricated "fully covered" dashboard. +func TestHandleRequest_DashboardSummary_PurchaseHistoryErrorIs500(t *testing.T) { + ctx := context.Background() + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + + mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything).Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) + mockStore.On("GetActivePurchaseHistory", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil, errors.New("pq: connection refused to db-host-internal")) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{ + scheduler: mockScheduler, + config: mockStore, + corsAllowedOrigin: "*", + apiKey: "test-key", + } + + resp, err := handler.HandleRequest(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": "test-key"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "GET", + Path: "/api/dashboard/summary", + }, + }, + }) + require.NoError(t, err) + assert.Equal(t, 500, resp.StatusCode, "a failed purchase-history read must not ship zeroed KPIs as 200: %s", resp.Body) + assert.NotContains(t, resp.Body, "active_commitments", "no KPI payload on failure") + assert.NotContains(t, resp.Body, "db-host-internal", "store error detail must not leak to the client") +} + +// TestGetDashboardSummary_NothingToMeasure_CoverageIsNull pins the JSON shape +// of the 0/0 coverage case (issue #72): current_coverage is an explicit null, +// which the dashboard renders as "--", never 100. +func TestGetDashboardSummary_NothingToMeasure_CoverageIsNull(t *testing.T) { + ctx := context.Background() + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + + mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything).Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return([]config.PurchaseHistoryRecord{}, 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) + assert.Nil(t, result.CurrentCoverage) + + body, err := json.Marshal(result) + require.NoError(t, err) + assert.Contains(t, string(body), `"current_coverage":null`) +} + func TestHandler_getDashboardSummary_Errors(t *testing.T) { ctx := context.Background() @@ -1363,6 +1437,27 @@ func TestHandler_getDashboardSummary_Errors(t *testing.T) { assert.Contains(t, err.Error(), "failed to get recommendations") }) + // Issue #72: purchase history is as much a hard dependency as + // recommendations; a store error must fail the request, not zero the KPIs. + t.Run("purchase history error", func(t *testing.T) { + mockScheduler := new(MockScheduler) + mockStore := new(MockConfigStore) + storeErr := errors.New("db error") + + mockScheduler.On("ListRecommendations", mock.Anything, mock.Anything).Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{DefaultCoverage: 80.0}, nil) + mockStore.On("GetActivePurchaseHistory", ctx, mock.Anything, mock.Anything, mock.Anything).Return(nil, storeErr) + + mockAuth, req := adminDashboardReq(ctx) + handler := &Handler{auth: mockAuth, scheduler: mockScheduler, config: mockStore} + + result, err := handler.getDashboardSummary(ctx, req, map[string]string{}) + require.Error(t, err) + assert.ErrorIs(t, err, storeErr) + assert.Contains(t, err.Error(), "failed to get active purchase history") + assert.Nil(t, result) + }) + t.Run("nil global config uses default coverage", func(t *testing.T) { mockScheduler := new(MockScheduler) mockStore := new(MockConfigStore) @@ -1857,8 +1952,9 @@ func TestHandler_calculateCommitmentMetrics_RevokedExcluded(t *testing.T) { t.Cleanup(func() { mockStore.AssertExpectations(t) }) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics( + activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics( ctx, "", nil, map[string][]string{"": {"acct-1"}}) + require.NoError(t, err) assert.Equal(t, 1, activeCommitments, "revoked commitment must not increment activeCommitments") @@ -1889,8 +1985,9 @@ func TestHandler_calculateCommitmentMetrics_ProviderFilter(t *testing.T) { handler := &Handler{config: mockStore} // Filter to aws only. - activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics( + activeCommitments, committedMonthly, _, savingsByService, err := handler.calculateCommitmentMetrics( ctx, "aws", nil, nil) + require.NoError(t, err) assert.Equal(t, 1, activeCommitments, "provider filter must restrict activeCommitments to aws rows only") diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 4523ab22..0e14092a 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3471,6 +3471,11 @@ components: current_coverage: type: number format: double + nullable: true + description: >- + Committed / (committed + potential) monthly savings, as a + percentage. Null when both are zero, so there is nothing to + measure. target_coverage: type: number format: double diff --git a/internal/api/types.go b/internal/api/types.go index 5cdd6dfa..4378b885 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -645,9 +645,11 @@ type DashboardSummaryResponse struct { TotalRecommendations int `json:"total_recommendations"` ActiveCommitments int `json:"active_commitments"` CommittedMonthly float64 `json:"committed_monthly"` - CurrentCoverage float64 `json:"current_coverage"` - TargetCoverage float64 `json:"target_coverage"` - YTDSavings float64 `json:"ytd_savings"` + // CurrentCoverage is null when there is neither committed nor potential + // savings to measure (0/0), instead of a fabricated 100% (issue #72). + CurrentCoverage *float64 `json:"current_coverage"` + TargetCoverage float64 `json:"target_coverage"` + YTDSavings float64 `json:"ytd_savings"` } // ServiceSavings holds savings data for a service.