diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 4f76016f7..efbd28981 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -67,12 +67,13 @@ 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, accountUUIDs, accountExternalIDsByProvider) + activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider) - // 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. + // Populate CurrentSavings on each per-service bucket from actual + // purchase_history commitments. Services with recommendations but no + // active commitments correctly keep CurrentSavings=0 (absent from + // currentSavingsByService). summarizeRecommendationsWithCoverage only + // sets PotentialSavings; CurrentSavings is exclusively owned here. for svc, monthlySavings := range currentSavingsByService { entry := byService[svc] entry.CurrentSavings = monthlySavings @@ -251,6 +252,13 @@ func summarizeRecommendationsWithCoverage( //nolint:gocritic // unnamedResult: r total += scaled svc := byService[rep.rec.Service] svc.PotentialSavings += scaled + // CurrentSavings is populated by getDashboardSummary from actual + // purchase_history commitments (calculateCommitmentMetrics). It is NOT + // derived from recommendations here: doing so would fabricate + // CurrentSavings == PotentialSavings for services with no active + // commitments (i.e. recommend-but-never-purchased). Services absent + // from currentSavingsByService correctly default to 0 per the + // dashboard.ts contract. byService[rep.rec.Service] = svc } return total, byService @@ -559,16 +567,23 @@ func commitmentExpiry(p config.PurchaseHistoryRecord) time.Time { } // 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 synthesized from failed/canceled/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. +// yet expired as of `now`, its status is one of the successful terminal states +// ("" for DB-backed rows where the column is unpersisted, or "completed"), and +// it has not been revoked. Rows synthesized from failed/canceled/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 { + // Revoked commitments are never active regardless of status or term. + // GetActivePurchaseHistory already excludes them in SQL; this is a + // defense-in-depth guard for any caller that passes rows from a different + // query (e.g. GetPurchaseHistoryFiltered). + if p.RevokedAt != nil { + return false + } // Status is unpersisted (dynamodbav:"-"); DB rows always read back as "". // Synthesized rows set it to "failed", "expired", "canceled", "pending", // "notified", "approved", "running", or "paused". Only "" and "completed" @@ -598,10 +613,10 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco // fetchCommitmentPurchases loads the active purchase_history rows that // calculateCommitmentMetrics aggregates, applying the pre-resolved account -// scope. 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. +// 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. // // - non-nil but empty accountUUIDs (no external groups): explicit "scoped to // zero accounts" sentinel (a restricted session whose allowed_accounts match @@ -613,7 +628,10 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco // row cap, so older-but-still-active 1y/3y commitments cannot be silently // dropped the way a newest-first LIMIT 1000 page dropped them (issue #1140); // the result is bounded by the number of live commitments. -func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) { +// - 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) { if accountUUIDs != nil && len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 { return nil, false } @@ -623,6 +641,17 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, logging.Errorf("dashboard: failed to fetch commitment purchases; KPIs will be zeroed: %v", err) return nil, false } + + if provider != "" { + filtered := purchases[:0] + for _rvc := range purchases { + if purchases[_rvc].Provider == provider { + filtered = append(filtered, purchases[_rvc]) + } + } + purchases = filtered + } + return purchases, true } @@ -632,6 +661,11 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, // scope-to-query mapping (including the zero-account short-circuit for issue // #956) lives in fetchCommitmentPurchases. // +// provider, when non-empty, restricts the result set to purchases for that +// cloud provider so the KPIs match the provider chip selection. This mirrors +// the recommendations half which passes the same provider param to +// ListRecommendations. +// // 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 @@ -641,9 +675,9 @@ 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, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) { +func (h *Handler) calculateCommitmentMetrics(ctx context.Context, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) { currentTime := time.Now() - purchases, ok := h.fetchCommitmentPurchases(ctx, currentTime, accountUUIDs, accountExternalIDsByProvider) + purchases, ok := h.fetchCommitmentPurchases(ctx, currentTime, provider, accountUUIDs, accountExternalIDsByProvider) if !ok { return 0, 0, 0, nil } diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index b89cbc4e2..80755f61c 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -846,7 +846,7 @@ 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 := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 0, activeCommitments) assert.Equal(t, 0.0, committedMonthly) @@ -860,7 +860,7 @@ 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 := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 0, activeCommitments) assert.Equal(t, 0.0, committedMonthly) @@ -886,7 +886,7 @@ 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 := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 100.0, committedMonthly) @@ -913,7 +913,7 @@ 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 := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) // Should skip expired commitments assert.Equal(t, 0, activeCommitments) @@ -940,7 +940,7 @@ 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 := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) assert.Equal(t, 1, activeCommitments) assert.Equal(t, 50.0, committedMonthly) @@ -973,7 +973,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}}) + activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}}) // Only the status="" row counts; the failed row must be excluded. assert.Equal(t, 1, activeCommitments, @@ -1007,7 +1007,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, uuids, nil) + activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", uuids, nil) assert.Equal(t, 2, activeCommitments) assert.Equal(t, 250.0, committedMonthly, @@ -1035,7 +1035,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) { handler := &Handler{config: mockStore} activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics( - ctx, []string{"bbbbbbbb-1111-2222-3333-444444444444"}, map[string][]string{"aws": {"999988887777"}}) + ctx, "", []string{"bbbbbbbb-1111-2222-3333-444444444444"}, map[string][]string{"aws": {"999988887777"}}) assert.Equal(t, 1, activeCommitments, "external-id-only commitment must be counted") assert.Equal(t, 175.0, committedMonthly) @@ -1097,7 +1097,7 @@ func TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000(t *testing.T) handler := &Handler{config: mockStore} - activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, nil) + activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, nil) assert.Equal(t, 1, activeCommitments, "the still-active 3y commitment beyond the old 1000-row cap must be counted") @@ -1585,3 +1585,101 @@ func TestFirstServiceConfig(t *testing.T) { } }) } + +// TestIsActiveCommitment_RevokedReturnsFalse is the defect-#2 regression guard. +// A revoked commitment (RevokedAt != nil) must never be treated as active, +// regardless of whether its term window is still open. Pre-fix, isActiveCommitment +// did not check RevokedAt, so revoked rows slipped through into KPI totals. +func TestIsActiveCommitment_RevokedReturnsFalse(t *testing.T) { + now := time.Now() + revokedAt := now.AddDate(0, -1, 0) // revoked 1 month ago + p := config.PurchaseHistoryRecord{ + Timestamp: now.AddDate(0, -3, 0), // purchased 3 months ago + Term: 1, // 1-year term, still within window + RevokedAt: &revokedAt, + Status: "", + } + // Pre-fix: isActiveCommitment returned true (only checked term expiry + status). + // Post-fix: must return false because RevokedAt != nil. + assert.False(t, isActiveCommitment(p, now), + "revoked commitment must not be active even when its term has not expired") +} + +// TestHandler_calculateCommitmentMetrics_RevokedExcluded is the defect-#2 +// regression guard for the dashboard KPI path. A revoked purchase returned by +// GetActivePurchaseHistory (e.g., from a DB snapshot before the SQL fix was +// deployed) must not contribute to activeCommitments or committedMonthly. +func TestHandler_calculateCommitmentMetrics_RevokedExcluded(t *testing.T) { + ctx := context.Background() + now := time.Now() + revokedAt := now.AddDate(0, -1, 0) + purchases := []config.PurchaseHistoryRecord{ + { + // Active, non-revoked commitment. + Service: "ec2", + Timestamp: now.AddDate(0, -3, 0), + Term: 1, + EstimatedSavings: 100.0, + }, + { + // Revoked commitment: term still open but revoked_at is set. + // Pre-fix: counted as active, inflating KPIs. + Service: "ec2", + Timestamp: now.AddDate(0, -6, 0), + Term: 1, + EstimatedSavings: 999.0, + RevokedAt: &revokedAt, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), + []string(nil), map[string][]string{"": {"acct-1"}}).Return(purchases, nil) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{config: mockStore} + activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics( + ctx, "", nil, map[string][]string{"": {"acct-1"}}) + + assert.Equal(t, 1, activeCommitments, + "revoked commitment must not increment activeCommitments") + assert.InDelta(t, 100.0, committedMonthly, 0.001, + "revoked commitment savings must not appear in committedMonthly") + assert.InDelta(t, 100.0, savingsByService["ec2"], 0.001, + "revoked commitment must be excluded from per-service savings map") +} + +// TestHandler_calculateCommitmentMetrics_ProviderFilter is the defect-#4 +// regression guard. When a provider chip is active, commitment KPIs must be +// restricted to that provider only. Pre-fix, calculateCommitmentMetrics ignored +// the provider param, so aws-only KPIs mixed in azure/gcp purchases. +func TestHandler_calculateCommitmentMetrics_ProviderFilter(t *testing.T) { + ctx := context.Background() + now := time.Now() + purchases := []config.PurchaseHistoryRecord{ + {Provider: "aws", Service: "ec2", Timestamp: now.AddDate(0, -3, 0), Term: 1, EstimatedSavings: 100.0}, + {Provider: "azure", Service: "vm", Timestamp: now.AddDate(0, -3, 0), Term: 1, EstimatedSavings: 200.0}, + {Provider: "gcp", Service: "cud", Timestamp: now.AddDate(0, -3, 0), Term: 1, EstimatedSavings: 300.0}, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"), + []string(nil), map[string][]string(nil)).Return(purchases, nil) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + handler := &Handler{config: mockStore} + + // Filter to aws only. + activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics( + ctx, "aws", nil, nil) + + assert.Equal(t, 1, activeCommitments, + "provider filter must restrict activeCommitments to aws rows only") + assert.InDelta(t, 100.0, committedMonthly, 0.001, + "provider filter must restrict committedMonthly to aws rows only") + assert.InDelta(t, 100.0, savingsByService["ec2"], 0.001) + assert.NotContains(t, savingsByService, "vm", + "azure rows must not appear when filtering to aws") + assert.NotContains(t, savingsByService, "cud", + "gcp rows must not appear when filtering to aws") +} diff --git a/internal/api/handler_history.go b/internal/api/handler_history.go index 850664fcc..d6779670e 100644 --- a/internal/api/handler_history.go +++ b/internal/api/handler_history.go @@ -1018,6 +1018,17 @@ func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistoryS summary := HistorySummary{TotalPurchases: len(purchases)} for _rvc := range purchases { p := purchases[_rvc] + // Revoked commitments are excluded from dollar KPIs regardless of their + // status: the provider has cancelled the commitment, so UpfrontCost and + // EstimatedSavings no longer represent active spend or realized savings. + // They count toward TotalPurchases (for auditability) and TotalRevoked + // (so the UI can surface the count), but not TotalCompleted. + // GetActivePurchaseHistory already excludes revoked rows via SQL; this + // guard handles the GetPurchaseHistoryFiltered path used by History. + if p.RevokedAt != nil { + summary.TotalRevoked++ + continue + } // Non-completed rows count toward TotalPurchases and their specific // bucket (pending / in-progress / failed / expired / canceled) but // are excluded from the dollar totals — the money hasn't been committed diff --git a/internal/api/handler_history_test.go b/internal/api/handler_history_test.go index 701af77d1..fb3282a5c 100644 --- a/internal/api/handler_history_test.go +++ b/internal/api/handler_history_test.go @@ -2035,3 +2035,40 @@ func TestHandler_getHistory_ExpireIfStale_LambdaGuard(t *testing.T) { assert.Equal(t, "pending", historyResp.Purchases[0].Status) }) } + +// TestSummarizePurchaseHistory_RevokedExcludedFromKPIs is the defect-#2 +// regression guard. A revoked commitment (RevokedAt != nil) must count toward +// TotalPurchases and TotalRevoked, but must be excluded from TotalCompleted and +// all dollar KPIs. Pre-fix, summarizePurchaseHistory had no revoked case, so +// revoked rows fell through to the completed-dollar-total path and inflated +// TotalUpfront and TotalMonthlySavings. +func TestSummarizePurchaseHistory_RevokedExcludedFromKPIs(t *testing.T) { + now := time.Now() + revokedAt := now.AddDate(0, -1, 0) // revoked 1 month ago + + purchases := []config.PurchaseHistoryRecord{ + // Completed, non-revoked: must contribute to dollar totals. + {Status: "completed", UpfrontCost: 100.0, EstimatedSavings: 10.0}, + // Revoked: term still open but revoked_at is set. + // Pre-fix: fell through to the completed branch, inflating TotalUpfront + // by 500 and TotalMonthlySavings by 50. + {Status: "completed", UpfrontCost: 500.0, EstimatedSavings: 50.0, RevokedAt: &revokedAt}, + // Revoked with empty status (legacy DB row): same guard must apply. + {Status: "", UpfrontCost: 750.0, EstimatedSavings: 75.0, RevokedAt: &revokedAt}, + } + + summary := summarizePurchaseHistory(purchases) + + assert.Equal(t, 3, summary.TotalPurchases, "all rows count toward TotalPurchases") + assert.Equal(t, 2, summary.TotalRevoked, + "revoked rows must be counted in TotalRevoked") + assert.Equal(t, 1, summary.TotalCompleted, + "revoked rows must not inflate TotalCompleted") + + assert.InDelta(t, 100.0, summary.TotalUpfront, 0.001, + "revoked upfront cost must not appear in TotalUpfront") + assert.InDelta(t, 10.0, summary.TotalMonthlySavings, 0.001, + "revoked savings must not appear in TotalMonthlySavings") + assert.InDelta(t, 120.0, summary.TotalAnnualSavings, 0.001, + "TotalAnnualSavings must exclude revoked rows") +} diff --git a/internal/api/types.go b/internal/api/types.go index e51e2e093..48e49b027 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -850,9 +850,9 @@ type HistoryResponse struct { // HistorySummary provides aggregate statistics for purchase history. // TotalPurchases is the total count of rows (completed + all non-completed // states); the per-state counters break it down so the UI can render -// meaningful totals. Dollar totals count completed rows only: pending, -// in-progress, failed, expired, and canceled rows are all excluded because -// no money was committed for any of those states. +// meaningful totals. Dollar totals count completed, non-revoked rows only: +// pending, in-progress, failed, expired, canceled, and revoked rows are all +// excluded because no money is being actively committed for those states. type HistorySummary struct { TotalPurchases int `json:"total_purchases"` TotalCompleted int `json:"total_completed"` @@ -862,9 +862,13 @@ type HistorySummary struct { // Tracked separately from pending and excluded from the dollar totals so an // interrupted approval (issue #621) stays visible without inflating // committed spend/savings. - TotalInProgress int `json:"total_in_progress"` - TotalFailed int `json:"total_failed"` - TotalExpired int `json:"total_expired"` + TotalInProgress int `json:"total_in_progress"` + TotalFailed int `json:"total_failed"` + TotalExpired int `json:"total_expired"` + // TotalRevoked counts commitments that have been revoked/refunded via + // MarkPurchaseRevoked. They are excluded from TotalCompleted and all dollar + // totals because the provider has canceled the commitment. + TotalRevoked int `json:"total_revoked"` TotalUpfront float64 `json:"total_upfront"` TotalMonthlySavings float64 `json:"total_monthly_savings"` TotalAnnualSavings float64 `json:"total_annual_savings"` diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b53628283..b302995fc 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -1796,6 +1796,7 @@ func (s *PostgresStore) GetActivePurchaseHistory(ctx context.Context, asOf time. conds := []string{ "term > 0", "timestamp + make_interval(hours => term * 8760) >= $1", + "revoked_at IS NULL", } args := []any{asOf} conds, args = appendAccountPredicate(conds, args, accountIDs, externalIDsByProvider) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 4dc3d942e..9e8d0ab00 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1036,6 +1036,8 @@ func TestPGXMock_GetPurchaseHistoryFiltered_NoFilters(t *testing.T) { // the full 21-column SELECT including the issue-#290 revocation columns: // before this fix the query selected only 17 columns while // queryPurchaseHistory scans 21 destinations, so every call failed at Scan. +// The `revoked_at IS NULL` clause (defect #2 fix) ensures revoked commitments +// are never returned by this path. func TestPGXMock_GetActivePurchaseHistory_Unscoped(t *testing.T) { mock := newMock(t) store := storeWith(mock) @@ -1044,7 +1046,7 @@ func TestPGXMock_GetActivePurchaseHistory_Unscoped(t *testing.T) { now := time.Now().Truncate(time.Second) rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "acct-1")...) mock.ExpectQuery( - `SELECT account_id, purchase_id, .*revocation_window_closes_at, revoked_at, revoked_via, support_case_id FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 ORDER BY timestamp DESC$`, + `SELECT account_id, purchase_id, .*revocation_window_closes_at, revoked_at, revoked_via, support_case_id FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 AND revoked_at IS NULL ORDER BY timestamp DESC$`, ).WithArgs(now).WillReturnRows(rows) records, err := store.GetActivePurchaseHistory(ctx, now, nil, nil) @@ -1057,7 +1059,9 @@ func TestPGXMock_GetActivePurchaseHistory_Unscoped(t *testing.T) { // account predicate (same shape as GetPurchaseHistoryFiltered, issues // #701/#498/#866) composes with the active filter, again with no LIMIT, so the // dashboard KPI path and the inventory endpoints get the complete active set -// for the selected account scope (issue #1140). +// for the selected account scope (issue #1140). The `revoked_at IS NULL` clause +// (defect #2 fix) is also present so revoked commitments never appear even for +// account-scoped queries. func TestPGXMock_GetActivePurchaseHistory_AccountScoped(t *testing.T) { mock := newMock(t) store := storeWith(mock) @@ -1066,7 +1070,7 @@ func TestPGXMock_GetActivePurchaseHistory_AccountScoped(t *testing.T) { now := time.Now().Truncate(time.Second) rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "111122223333")...) mock.ExpectQuery( - `FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 AND \(cloud_account_id = ANY\(\$2\) OR \(provider = \$3 AND account_id = ANY\(\$4\)\)\) ORDER BY timestamp DESC$`, + `FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 AND revoked_at IS NULL AND \(cloud_account_id = ANY\(\$2\) OR \(provider = \$3 AND account_id = ANY\(\$4\)\)\) ORDER BY timestamp DESC$`, ).WithArgs(now, []string{"acct-uuid-1"}, "aws", []string{"111122223333"}).WillReturnRows(rows) records, err := store.GetActivePurchaseHistory(ctx, now,