From 9730a994cdd1f3aac64573f21246db4eeb7aac35 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 18:46:01 -0700 Subject: [PATCH] fix(dashboard): apply provider filter to savings-history chart (refs #498) The Savings History / savings-over-time chart and YTD KPI sparkline are backed by GET /history/analytics. The handler read and applied the account filter but never read params["provider"], and QueryHistory had no provider predicate, so the series showed identical all-provider data regardless of the selected AWS/Azure/GCP chip (QA row 2.3). The Home dashboard trend chart also deliberately dropped the provider param when calling the endpoint. Thread the provider through end to end, mirroring the working /history and trends (QueryByService) paths: - handler getHistoryAnalytics: read provider, normalise the "all" sentinel to "" (no filter) and validateProvider at the boundary, then pass it to QueryHistory. - PostgresAnalyticsClient.QueryHistory + AnalyticsClientInterface: add a provider param and an optional parameter-bound `AND provider = $N` WHERE fragment ("" = all providers), positioned after the dual-column account binds. - dashboard.ts loadSavingsTrendChart: forward the provider chip (and name it in the empty-state via buildFilterDesc, now that the query is actually provider-scoped). modules/savings-history.ts already forwarded provider; correct its stale "no-op until #502" comment. Regression tests fail pre-fix and pass post-fix: - TestQueryHistory_ProviderFilter / _ProviderFilterWithAccount assert the provider bind and its position. - TestHandler_getHistoryAnalytics_ProviderFilter / _ProviderAllIsNoFilter / _InvalidProvider cover threading, the "all" sentinel, and boundary rejection. - dashboard.test.ts asserts the provider chip is forwarded to the API and omitted when no provider is selected. Also reset the topbar-filter mocks in the savings-trend describe block so tests stay isolated now that the chart reads the provider chip. --- frontend/src/__tests__/dashboard.test.ts | 47 +++++++++++-- frontend/src/dashboard.ts | 34 +++++---- frontend/src/modules/savings-history.ts | 4 +- internal/api/analytics_postgres.go | 20 ++++-- internal/api/analytics_postgres_test.go | 69 +++++++++++++++++-- internal/api/handler_analytics.go | 14 +++- internal/api/handler_analytics_test.go | 67 +++++++++++++++--- internal/api/handler_coverage_test.go | 2 +- .../api/handler_per_account_perms_test.go | 4 +- internal/api/types.go | 2 +- 10 files changed, 212 insertions(+), 51 deletions(-) diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index 43623f2da..8896d78e6 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -840,6 +840,13 @@ describe('Dashboard Module', () => { b30.textContent = '30d'; document.body.replaceChildren(canvas, empty, b90, b30); (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + // Reset the topbar filter mocks to the documented default (no filter) + // so each test starts isolated. clearAllMocks() resets call history but + // not mockReturnValue implementations, so a provider/account value set by + // an earlier test would otherwise leak in (now that the chart reads the + // provider chip, issue #498). + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); }); test('uses daily interval for the default 90-day range', async () => { @@ -1135,10 +1142,10 @@ describe('Dashboard Module', () => { expect(empty?.textContent).toContain('No purchase history yet'); }); - test('empty-state does NOT mention provider even when a provider filter is active (#764)', async () => { - // The analytics endpoint ignores the provider param until #764 lands. - // Showing "No savings history for aws." would imply the query was - // scoped to that provider, which is false. Generic copy is used instead. + test('empty-state names the provider when a provider filter is active (issue #498, QA 2.3)', async () => { + // The analytics endpoint now applies the provider param as a WHERE + // filter, so the query IS scoped to the selected provider — naming it + // in the empty-state is accurate. Mirrors the account-filter empty-state. (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); @@ -1147,9 +1154,35 @@ describe('Dashboard Module', () => { const empty = document.getElementById('savings-trend-empty'); expect(empty?.classList.contains('hidden')).toBe(false); - // Provider name must not appear in the message. - expect(empty?.textContent).not.toContain('aws'); - expect(empty?.textContent).toContain('No purchase history yet'); + // Provider name (uppercased by buildFilterDesc) appears in the message. + expect(empty?.textContent).toContain('AWS'); + expect(empty?.textContent).toContain('No savings history for'); + }); + + test('loadSavingsTrendChart forwards the provider chip to the analytics API (issue #498, QA 2.3)', async () => { + // Regression: previously the chart dropped the provider param, so the + // savings-history series showed the same all-provider data regardless of + // the selected provider. The fix forwards provider to scope the query. + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsTrendChart(); + + expect(api.getSavingsAnalytics).toHaveBeenCalledWith( + expect.objectContaining({ provider: 'aws' }) + ); + }); + + test('loadSavingsTrendChart omits provider when no provider filter is active', async () => { + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsTrendChart(); + + const call = (api.getSavingsAnalytics as jest.Mock).mock.calls[0]?.[0]; + expect(call).not.toHaveProperty('provider'); }); }); diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 5d21187e0..351b0a899 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -1033,13 +1033,15 @@ export async function loadSavingsTrendChart(): Promise { const interval: 'hourly' | 'daily' | 'weekly' = intervalDays <= 7 ? 'hourly' : intervalDays <= 90 ? 'daily' : 'weekly'; try { - // Always forward account_ids to the chart so its data scope matches the - // KPI tiles above it. The backend /history/analytics handler accepts a - // single `account_id`; api.getSavingsAnalytics also sets the singular - // param for single-account requests (see api/history.ts). - // provider is not forwarded: the analytics backend does not yet support - // provider-scoped queries (handler_analytics.go has no provider param). + // Always forward account_ids AND provider to the chart so its data scope + // matches the KPI tiles above it. The backend /history/analytics handler + // accepts a single `account_id`; api.getSavingsAnalytics also sets the + // singular param for single-account requests (see api/history.ts). The + // backend now applies the provider chip as a provider WHERE filter on the + // savings-history query, so forward it to scope the chart (issue #498, + // QA 2.3). const accountIDs = state.getCurrentAccountIDs(); + const provider = state.getCurrentProvider(); const data = await api.getSavingsAnalytics({ // For 'all': send the epoch sentinel so the backend returns unbounded // history. Omitting start would cause parseDateRange to default to @@ -1047,6 +1049,7 @@ export async function loadSavingsTrendChart(): Promise { start: isAllRange ? epochStart : new Date(windowStartMs).toISOString(), end: now.toISOString(), interval, + ...(provider ? { provider } : {}), ...(accountIDs.length > 0 ? { account_ids: accountIDs } : {}), }); @@ -1087,18 +1090,13 @@ export async function loadSavingsTrendChart(): Promise { if (points.length === 0) { canvas.classList.add('hidden'); if (empty) { - if (accountIDs.length > 0) { - // Account IDs are forwarded to the backend (see call above), so - // mentioning them in the empty-state is accurate. - empty.textContent = `No savings history for ${accountIDs.join(', ')}.`; - } else { - // Provider is intentionally NOT mentioned here: the analytics - // endpoint does not accept a provider param yet (tracked in #764), - // so the query always returns all-provider data regardless of the - // topbar provider filter. Claiming provider scope would be - // misleading — drop it until #764 lands. - empty.textContent = 'No purchase history yet.'; - } + // Both provider and account are forwarded to the backend (see call + // above), so naming the active filter in the empty-state is accurate + // (issue #498). buildFilterDesc returns '' when no filter is active. + const filterDesc = buildFilterDesc(provider, accountIDs); + empty.textContent = filterDesc + ? `No savings history for ${filterDesc}.` + : 'No purchase history yet.'; empty.classList.remove('hidden'); } if (savingsTrendChart) { savingsTrendChart.destroy(); savingsTrendChart = null; } diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 7d4b61031..4b97ebae0 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -88,8 +88,8 @@ export async function loadSavingsHistory(): Promise { // is single-select, and the backend's /history/analytics takes a single // account_id (see handler_analytics.go), so we forward the only selected // ID, mirroring dashboard.ts loadSavingsTrendChart. The provider chip is - // forwarded too; the backend honours it once #502 lands (until then it is - // a harmless no-op param and account_ids does the filtering). + // forwarded too and the backend now applies it as a provider WHERE filter + // on the savings-history query (issue #498, QA 2.3). const currentProvider = state.getCurrentProvider(); const currentAccountIDs = state.getCurrentAccountIDs(); diff --git a/internal/api/analytics_postgres.go b/internal/api/analytics_postgres.go index 0c9bb57f0..a66619cf7 100644 --- a/internal/api/analytics_postgres.go +++ b/internal/api/analytics_postgres.go @@ -143,11 +143,15 @@ func sortedProviderKeys(m map[string][]string) []string { // QueryHistory aggregates purchase_history rows bucketed by interval. // Empty accountUUIDs AND accountExternalIDsByProvider means "all accounts // accessible to the caller"; scoping is enforced upstream in the handler. -// Returns data points in ascending order and a summary covering the full window. +// A non-empty provider ("aws"/"azure"/"gcp") restricts to rows of that +// provider; "" means all providers (the global-filter "all" sentinel is +// normalised to "" in the handler). Returns data points in ascending order +// and a summary covering the full window. func (c *PostgresAnalyticsClient) QueryHistory( ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, + provider string, start, end time.Time, interval string, ) ([]HistoryDataPoint, *HistorySummary, error) { @@ -159,11 +163,17 @@ func (c *PostgresAnalyticsClient) QueryHistory( // Single query grouped by (bucket, service, provider) so we can assemble // both the top-line bucket totals and the by_service / by_provider // breakdowns without a second trip to the DB. The account predicate is the - // shared dual-column clause (see accountFilterClause). + // shared dual-column clause (see accountFilterClause); the optional provider + // predicate mirrors QueryByService (parameter-bound, "" = no filter). // // #nosec G201 — `unit` is allowlisted by intervalToTruncUnit above and the - // account clause is parameter-bound (no user input interpolated). + // account / provider clauses are parameter-bound (no user input interpolated). accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{start, end}) + providerClause := "" + if provider != "" { + args = append(args, provider) + providerClause = fmt.Sprintf(" AND provider = $%d", len(args)) + } query := fmt.Sprintf(` SELECT date_trunc('%s', timestamp) AS bucket, service, @@ -174,10 +184,10 @@ func (c *PostgresAnalyticsClient) QueryHistory( FROM purchase_history WHERE timestamp >= $1 AND timestamp <= $2 - AND %s + AND %s%s GROUP BY bucket, service, provider ORDER BY bucket ASC - `, unit, accountClause) + `, unit, accountClause, providerClause) rows, err := c.db.Query(ctx, query, args...) if err != nil { diff --git a/internal/api/analytics_postgres_test.go b/internal/api/analytics_postgres_test.go index 6b4b98cd6..1baf5aef6 100644 --- a/internal/api/analytics_postgres_test.go +++ b/internal/api/analytics_postgres_test.go @@ -72,7 +72,7 @@ func TestQueryHistory_Success(t *testing.T) { start := time.Date(2026, 4, 20, 0, 0, 0, 0, time.UTC) end := time.Date(2026, 4, 24, 0, 0, 0, 0, time.UTC) - points, summary, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"acct-1"}}, start, end, "daily") + points, summary, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"acct-1"}}, "", start, end, "daily") require.NoError(t, err) require.Len(t, points, 2) @@ -103,7 +103,7 @@ func TestQueryHistory_Success(t *testing.T) { func TestQueryHistory_BadInterval(t *testing.T) { client, _ := newMockAnalyticsClient(t) - _, _, err := client.QueryHistory(context.Background(), nil, nil, time.Now(), time.Now(), "yearly") + _, _, err := client.QueryHistory(context.Background(), nil, nil, "", time.Now(), time.Now(), "yearly") assert.Error(t, err) } @@ -113,7 +113,7 @@ func TestQueryHistory_QueryError(t *testing.T) { mock.ExpectQuery(`SELECT date_trunc`). WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()). WillReturnError(errors.New("db down")) - _, _, err := client.QueryHistory(context.Background(), nil, nil, time.Now(), time.Now(), "daily") + _, _, err := client.QueryHistory(context.Background(), nil, nil, "", time.Now(), time.Now(), "daily") assert.Error(t, err) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -191,7 +191,7 @@ func TestQueryHistory_DualColumnFilter(t *testing.T) { start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) - points, summary, err := client.QueryHistory(ctx, []string{uuid}, map[string][]string{"aws": {external}}, start, end, "daily") + points, summary, err := client.QueryHistory(ctx, []string{uuid}, map[string][]string{"aws": {external}}, "", start, end, "daily") require.NoError(t, err) require.Len(t, points, 1) assert.InDelta(t, 50.0, points[0].TotalSavings, 1e-9) @@ -244,7 +244,64 @@ func TestQueryHistory_CrossProviderScoped(t *testing.T) { start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) - _, _, err := client.QueryHistory(ctx, nil, map[string][]string{"aws": {"123"}, "azure": {"123"}}, start, end, "daily") + _, _, err := client.QueryHistory(ctx, nil, map[string][]string{"aws": {"123"}, "azure": {"123"}}, "", start, end, "daily") + require.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestQueryHistory_ProviderFilter is the regression for the Savings History +// chart provider-filter bug (issue #498, QA 2.3): selecting a provider in the +// global filter must scope the savings-over-time series to that provider. The +// handler forwards the chip value, so QueryHistory must append +// `AND provider = $N` bound to the provider so Azure/GCP rows are excluded. +// Pre-fix QueryHistory ignored the provider entirely and this test fails +// because no provider bind is emitted. +func TestQueryHistory_ProviderFilter(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + + bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + // Only the aws row comes back from the DB; the provider predicate is what + // keeps azure/gcp rows out, so the mock returns just the aws bucket. + rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}). + AddRow(bucket, "ec2", "aws", 10.0, 5.0, 1) + + // No account filter (all-accessible), so the account clause degrades to TRUE + // and start/end are $1/$2; the provider bind is therefore $3. + mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND TRUE AND provider = \$3`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "aws"). + WillReturnRows(rows) + + start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) + end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) + points, _, err := client.QueryHistory(ctx, nil, nil, "aws", start, end, "daily") + require.NoError(t, err) + require.Len(t, points, 1) + assert.InDelta(t, 10.0, points[0].TotalSavings, 1e-9) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestQueryHistory_ProviderFilterWithAccount verifies the provider bind is +// positioned after the dual-column account binds (provider becomes $6 when a +// UUID + provider-grouped external id precede it), so account + provider +// filtering compose correctly (issue #498). +func TestQueryHistory_ProviderFilterWithAccount(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + uuid := "aabbccdd-1234-5678-abcd-aabbccddee00" + external := "123456789012" + + bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}). + AddRow(bucket, "ec2", "aws", 50.0, 20.0, 1) + + mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(cloud_account_id = ANY\(\$3\) OR \(provider = \$4 AND account_id = ANY\(\$5\)\)\) AND provider = \$6`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{uuid}, "aws", []string{external}, "aws"). + WillReturnRows(rows) + + start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) + end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) + _, _, err := client.QueryHistory(ctx, []string{uuid}, map[string][]string{"aws": {external}}, "aws", start, end, "daily") require.NoError(t, err) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -267,7 +324,7 @@ func TestQueryHistory_ExternalIDFilter(t *testing.T) { start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) - _, _, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"123456789012"}}, start, end, "daily") + _, _, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"123456789012"}}, "", start, end, "daily") require.NoError(t, err) assert.NoError(t, mock.ExpectationsWereMet()) } diff --git a/internal/api/handler_analytics.go b/internal/api/handler_analytics.go index 48d26d480..514397de9 100644 --- a/internal/api/handler_analytics.go +++ b/internal/api/handler_analytics.go @@ -134,6 +134,18 @@ func (h *Handler) getHistoryAnalytics(ctx context.Context, req *events.LambdaFun interval = "hourly" } + // Normalise + validate the global-filter provider chip, mirroring + // parseHistoryFilters: "all" (and "") mean no provider filter, anything + // else must be a known provider. Without this the chart ignored the + // provider chip entirely (issue #498, QA 2.3). + provider := params["provider"] + if provider == "all" { + provider = "" // explicit "no filter" sentinel + } + if err := validateProvider(provider); err != nil { + return nil, err + } + // For scoped users we require account_id and validate it's in their scope. // We don't (yet) support analytics across a subset — the underlying // aggregate takes a single account_id. An unrestricted/admin session @@ -154,7 +166,7 @@ func (h *Handler) getHistoryAnalytics(ctx context.Context, req *events.LambdaFun accountUUIDs, accountExternalIDsByProvider := h.resolveSingleAccountFilterIDs(ctx, accountID) // Aggregate history from the analytics client (Postgres-backed). - dataPoints, summary, err := h.analyticsClient.QueryHistory(ctx, accountUUIDs, accountExternalIDsByProvider, start, end, interval) + dataPoints, summary, err := h.analyticsClient.QueryHistory(ctx, accountUUIDs, accountExternalIDsByProvider, provider, start, end, interval) if err != nil { return nil, fmt.Errorf("failed to query analytics: %w", err) } diff --git a/internal/api/handler_analytics_test.go b/internal/api/handler_analytics_test.go index 40e957002..9622b55dc 100644 --- a/internal/api/handler_analytics_test.go +++ b/internal/api/handler_analytics_test.go @@ -19,8 +19,8 @@ type MockAnalyticsClient struct { mock.Mock } -func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) { - args := m.Called(ctx, accountUUIDs, accountExternalIDsByProvider, start, end, interval) +func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) { + args := m.Called(ctx, accountUUIDs, accountExternalIDsByProvider, provider, start, end, interval) if args.Get(0) == nil { return nil, nil, args.Error(2) } @@ -49,7 +49,7 @@ func TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount(t *testing.T) { accountExternal := "999988887777" mockClient := new(MockAnalyticsClient) - mockClient.On("QueryHistory", ctx, []string{accountUUID}, map[string][]string{"aws": {accountExternal}}, mock.Anything, mock.Anything, "hourly"). + mockClient.On("QueryHistory", ctx, []string{accountUUID}, map[string][]string{"aws": {accountExternal}}, "", mock.Anything, mock.Anything, "hourly"). Return([]HistoryDataPoint{{TotalSavings: 50.0, PurchaseCount: 1}}, &HistorySummary{TotalPurchases: 1}, nil) mockStore := new(MockConfigStore) @@ -63,7 +63,7 @@ func TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount(t *testing.T) { result, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"account_id": accountUUID}) require.NoError(t, err) require.NotNil(t, result) - mockClient.AssertCalled(t, "QueryHistory", ctx, []string{accountUUID}, map[string][]string{"aws": {accountExternal}}, mock.Anything, mock.Anything, "hourly") + mockClient.AssertCalled(t, "QueryHistory", ctx, []string{accountUUID}, map[string][]string{"aws": {accountExternal}}, "", mock.Anything, mock.Anything, "hourly") } // MockAnalyticsCollector is a mock implementation of AnalyticsCollectorInterface @@ -104,7 +104,7 @@ func TestHandler_getHistoryAnalytics_Success(t *testing.T) { // "account-123" is not a known UUID, so resolveSingleAccountFilterIDs // treats it as an external account number: QueryHistory is called with no // UUIDs and account-123 as the external id. - mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string{"": {"account-123"}}, mock.Anything, mock.Anything, "hourly").Return(dataPoints, summary, nil) + mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string{"": {"account-123"}}, "", mock.Anything, mock.Anything, "hourly").Return(dataPoints, summary, nil) mockAuth, req := adminAnalyticsReq(ctx) handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} @@ -140,7 +140,7 @@ func TestHandler_getHistoryAnalytics_DefaultInterval(t *testing.T) { dataPoints := []HistoryDataPoint{} // Empty account_id: resolver returns no UUIDs and no external ids. - mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, "hourly").Return(dataPoints, (*HistorySummary)(nil), nil) + mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string(nil), "", mock.Anything, mock.Anything, "hourly").Return(dataPoints, (*HistorySummary)(nil), nil) mockAuth, req := adminAnalyticsReq(ctx) handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} @@ -150,7 +150,58 @@ func TestHandler_getHistoryAnalytics_DefaultInterval(t *testing.T) { _, err := handler.getHistoryAnalytics(ctx, req, params) require.NoError(t, err) - mockClient.AssertCalled(t, "QueryHistory", ctx, []string(nil), map[string][]string(nil), mock.Anything, mock.Anything, "hourly") + mockClient.AssertCalled(t, "QueryHistory", ctx, []string(nil), map[string][]string(nil), "", mock.Anything, mock.Anything, "hourly") +} + +// TestHandler_getHistoryAnalytics_ProviderFilter is the handler-level +// regression for the Savings History provider-filter bug (issue #498, QA 2.3): +// the provider chip value must reach QueryHistory so the chart is scoped to the +// selected provider. Pre-fix the handler never read params["provider"] and +// always passed "" — this asserts the exact provider arg. +func TestHandler_getHistoryAnalytics_ProviderFilter(t *testing.T) { + ctx := context.Background() + mockClient := new(MockAnalyticsClient) + mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string(nil), "aws", mock.Anything, mock.Anything, "hourly"). + Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil) + + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} + + _, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"provider": "aws"}) + require.NoError(t, err) + mockClient.AssertCalled(t, "QueryHistory", ctx, []string(nil), map[string][]string(nil), "aws", mock.Anything, mock.Anything, "hourly") +} + +// TestHandler_getHistoryAnalytics_ProviderAllIsNoFilter verifies the "all" +// sentinel is normalised to "" (no provider filter), matching parseHistoryFilters +// and the other charts (issue #498). +func TestHandler_getHistoryAnalytics_ProviderAllIsNoFilter(t *testing.T) { + ctx := context.Background() + mockClient := new(MockAnalyticsClient) + mockClient.On("QueryHistory", ctx, []string(nil), map[string][]string(nil), "", mock.Anything, mock.Anything, "hourly"). + Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil) + + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} + + _, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"provider": "all"}) + require.NoError(t, err) + mockClient.AssertCalled(t, "QueryHistory", ctx, []string(nil), map[string][]string(nil), "", mock.Anything, mock.Anything, "hourly") +} + +// TestHandler_getHistoryAnalytics_InvalidProvider rejects an unknown provider at +// the boundary with a 400 rather than silently ignoring it (issue #498). +func TestHandler_getHistoryAnalytics_InvalidProvider(t *testing.T) { + ctx := context.Background() + mockClient := new(MockAnalyticsClient) + + mockAuth, req := adminAnalyticsReq(ctx) + handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} + + _, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"provider": "oracle"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid provider") + mockClient.AssertNotCalled(t, "QueryHistory") } func TestHandler_getHistoryAnalytics_InvalidDateRange(t *testing.T) { @@ -173,7 +224,7 @@ func TestHandler_getHistoryAnalytics_QueryError(t *testing.T) { ctx := context.Background() mockClient := new(MockAnalyticsClient) - mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, errors.New("query failed")) + mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, errors.New("query failed")) mockAuth, req := adminAnalyticsReq(ctx) handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)} diff --git a/internal/api/handler_coverage_test.go b/internal/api/handler_coverage_test.go index f444cebbc..1750614de 100644 --- a/internal/api/handler_coverage_test.go +++ b/internal/api/handler_coverage_test.go @@ -599,7 +599,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { t.Run("getHistoryAnalyticsHandler", func(t *testing.T) { mockAuth, req := adminAnalyticsReq(ctx) mockClient := new(MockAnalyticsClient) - mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil) + mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil) h := &Handler{auth: mockAuth, analyticsClient: mockClient} router := NewRouter(h) diff --git a/internal/api/handler_per_account_perms_test.go b/internal/api/handler_per_account_perms_test.go index 36021848b..04b84d799 100644 --- a/internal/api/handler_per_account_perms_test.go +++ b/internal/api/handler_per_account_perms_test.go @@ -372,7 +372,7 @@ func TestPerAccountPerms_HistoryAnalytics_AllowedAccountSucceeds(t *testing.T) { // permsAccA is a known account UUID (no external id in the fixture), so it // is matched on cloud_account_id only — the uuid set carries it, externals // is nil. - mockClient.On("QueryHistory", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything). + mockClient.On("QueryHistory", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return([]HistoryDataPoint{}, &HistorySummary{}, nil) mockStore := new(MockConfigStore) @@ -393,7 +393,7 @@ func TestPerAccountPerms_HistoryAnalytics_AllowedAccountSucceeds(t *testing.T) { require.NoError(t, err, "scoped user must be able to query analytics for account-A") require.NotNil(t, result) // Confirm the analytics backend was reached — not short-circuited. - mockClient.AssertCalled(t, "QueryHistory", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything) + mockClient.AssertCalled(t, "QueryHistory", ctx, []string{permsAccA}, map[string][]string(nil), mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // ─── 5. GET /history/breakdown ─────────────────────────────────────────────── diff --git a/internal/api/types.go b/internal/api/types.go index 755ed17e2..03a338304 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -91,7 +91,7 @@ type CommitmentOptsInterface interface { // the external ids stay grouped by provider so a reused external number across // providers cannot leak the wrong rows (issue #701/#498/#866). type AnalyticsClientInterface interface { - QueryHistory(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) + QueryHistory(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, provider string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) QueryBreakdown(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, start, end time.Time, dimension string) (map[string]BreakdownValue, error) }