diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index 817870dd4..6c0bd9bc5 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -1,7 +1,7 @@ /** * History module tests */ -import { initHistoryDateRange, viewPlanHistory, loadHistory } from '../history'; +import { initHistoryDateRange, viewPlanHistory, loadHistory, setupHistoryHandlers } from '../history'; // Mock the dependent modules jest.mock('../api', () => ({ @@ -316,4 +316,51 @@ describe('History Module', () => { }); }); }); + + // Issue #701: setupHistoryHandlers must subscribe to the global topbar + // provider/account filter chips so the Purchase History table and + // Approval Queue reload when a chip changes. PR #716 fixed the backend + // filter params but the frontend subscription was never registered in + // app.ts -- adding this suite guards against a regression. + describe('setupHistoryHandlers (issue #701)', () => { + beforeEach(() => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [] }); + }); + + test('registers a callback with state.subscribeProvider', () => { + setupHistoryHandlers(); + expect((require('../state').subscribeProvider as jest.Mock)).toHaveBeenCalledTimes(1); + }); + + test('registers a callback with state.subscribeAccount', () => { + setupHistoryHandlers(); + expect((require('../state').subscribeAccount as jest.Mock)).toHaveBeenCalledTimes(1); + }); + + test('provider change triggers loadHistory', async () => { + setupHistoryHandlers(); + const stateModule = require('../state'); + const providerCb = (stateModule.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + expect(typeof providerCb).toBe('function'); + + (api.getHistory as jest.Mock).mockClear(); + providerCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getHistory).toHaveBeenCalledTimes(1); + }); + + test('account change triggers loadHistory', async () => { + setupHistoryHandlers(); + const stateModule = require('../state'); + const accountCb = (stateModule.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + expect(typeof accountCb).toBe('function'); + + (api.getHistory as jest.Mock).mockClear(); + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getHistory).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/frontend/src/__tests__/index.test.ts b/frontend/src/__tests__/index.test.ts index 8f9baebfc..015724152 100644 --- a/frontend/src/__tests__/index.test.ts +++ b/frontend/src/__tests__/index.test.ts @@ -19,7 +19,8 @@ jest.mock('../recommendations', () => ({ })); jest.mock('../history', () => ({ - loadHistory: jest.fn() + loadHistory: jest.fn(), + setupHistoryHandlers: jest.fn(), })); jest.mock('../settings', () => ({ diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index 6c2317f7e..b4edce4df 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -77,6 +77,7 @@ jest.mock('../apikeys', () => ({ jest.mock('../history', () => ({ loadHistory: jest.fn(), + setupHistoryHandlers: jest.fn(), })); jest.mock('../modules/savings-history', () => ({ diff --git a/frontend/src/__tests__/savings-history.test.ts b/frontend/src/__tests__/savings-history.test.ts index 0939323d9..e952f0353 100644 --- a/frontend/src/__tests__/savings-history.test.ts +++ b/frontend/src/__tests__/savings-history.test.ts @@ -55,7 +55,10 @@ describe('Savings History Module', () => {
- +
$0 $0/hr @@ -975,4 +978,164 @@ describe('Savings History Module', () => { expect(mockChartInstance.destroy).toHaveBeenCalled(); }); }); + + // Issue #701: when the chart returns no data because a filter is active, the + // empty-state message must distinguish "nothing in this scope" from "no data + // at all". Without a filter the original message applies; with a filter the + // message should name the active filter so the user understands why the chart + // is blank. + describe('empty-state copy with active filter (issue #701)', () => { + test('shows filter name in empty-state heading when provider chip is set', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('AWS'); + expect(heading?.textContent).not.toContain('available yet'); + }); + + test('shows help text asking to broaden filter when a filter is active', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('gcp'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const help = emptyEl?.querySelector('p.help-text'); + expect(help?.textContent).toMatch(/broaden/i); + }); + + test('shows original message when no filter is active', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('available yet'); + }); + + test('shows filter name in empty-state heading when account chip is set', async () => { + const testUUID = 'aabbccdd-1234-5678-abcd-aabbccddee00'; + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([testUUID]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain(testUUID); + }); + + test('shows provider and account in heading when both chips are set', async () => { + const testUUID = 'aabbccdd-1234-5678-abcd-aabbccddee00'; + (state.getCurrentProvider as jest.Mock).mockReturnValue('azure'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([testUUID]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('AZURE'); + expect(heading?.textContent).toContain(testUUID); + }); + + test('shows distinct error copy on API failure (not the filter-aware empty state)', async () => { + // CR feedback on PR #741: a fetch failure used to fall through to the + // "no data for the selected filter" empty state, which masked real + // outages (network/5xx/auth). The error path must now render its own + // copy so the user can tell "filter matched nothing" from "the fetch + // never completed". + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockRejectedValue(new Error('network failure')); + console.error = jest.fn(); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + const help = emptyEl?.querySelector('p.help-text'); + expect(heading?.textContent).toBe('Failed to load savings history.'); + expect(help?.textContent).toContain('network failure'); + // Error copy is NOT the empty-state copy — it does not name the filter. + expect(heading?.textContent).not.toContain('AWS'); + expect(heading?.textContent).not.toContain('available yet'); + }); + + test('error state hides chart container and stats', async () => { + (getSavingsAnalytics as jest.Mock).mockRejectedValue(new Error('boom')); + console.error = jest.fn(); + + await loadSavingsHistory(); + + const chartContainer = document.getElementById('savings-history-chart')?.parentElement; + const statsEl = document.getElementById('savings-stats'); + expect(chartContainer?.classList.contains('hidden')).toBe(true); + expect(statsEl?.classList.contains('hidden')).toBe(true); + }); + + test('error state handles non-Error rejection with "Unknown error"', async () => { + // Generic reject value (e.g. a thrown string from a non-Error catch path) + // must still surface a helpful message rather than `[object Object]`. + (getSavingsAnalytics as jest.Mock).mockRejectedValue('something broke'); + console.error = jest.fn(); + + await loadSavingsHistory(); + + const help = document.getElementById('savings-history-empty')?.querySelector('p.help-text'); + expect(help?.textContent).toContain('Unknown error'); + }); + }); + + // CR feedback on PR #741 F2: a provider chip set to the "all" sentinel + // is the user explicitly choosing unfiltered, so buildFilterDesc must not + // include "ALL" in the empty-state copy. + describe('buildFilterDesc — "all" sentinel handling (PR #741 F2)', () => { + test('treats provider === "all" as unfiltered (no provider clause in heading)', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('all'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const heading = document.getElementById('savings-history-empty')?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('available yet'); + expect(heading?.textContent).not.toMatch(/all/i); + }); + + test('treats provider === "ALL" (case-insensitive) as unfiltered', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('ALL'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const heading = document.getElementById('savings-history-empty')?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('available yet'); + expect(heading?.textContent).not.toContain('ALL'); + }); + + test('still shows account clause when provider === "all" but account chip is set', async () => { + const testUUID = 'aabbccdd-1234-5678-abcd-aabbccddee00'; + (state.getCurrentProvider as jest.Mock).mockReturnValue('all'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([testUUID]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const heading = document.getElementById('savings-history-empty')?.querySelector('p:first-child'); + expect(heading?.textContent).toContain(testUUID); + expect(heading?.textContent).not.toMatch(/all,/i); + }); + }); }); diff --git a/frontend/src/__tests__/sidebar-anchors.test.ts b/frontend/src/__tests__/sidebar-anchors.test.ts index f5abd9e07..506b7c3e9 100644 --- a/frontend/src/__tests__/sidebar-anchors.test.ts +++ b/frontend/src/__tests__/sidebar-anchors.test.ts @@ -49,7 +49,7 @@ jest.mock('../settings', () => ({ })); jest.mock('../users', () => ({ setupUserHandlers: jest.fn() })); jest.mock('../apikeys', () => ({ initApiKeys: jest.fn() })); -jest.mock('../history', () => ({ loadHistory: jest.fn() })); +jest.mock('../history', () => ({ loadHistory: jest.fn(), setupHistoryHandlers: jest.fn() })); jest.mock('../modules/savings-history', () => ({ initSavingsHistory: jest.fn() })); jest.mock('../riexchange', () => ({ setupRIExchangeHandlers: jest.fn(), diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 242d190d5..124b7e0dc 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -12,7 +12,7 @@ import { savePlan, setupPlanHandlers, closePlanModal, openNewPlanModal, closePur import { saveGlobalSettings, setupSettingsHandlers, resetSettings } from './settings'; import { setupUserHandlers } from './users'; import { initApiKeys } from './apikeys'; -import { loadHistory } from './history'; +import { loadHistory, setupHistoryHandlers } from './history'; import { initSavingsHistory } from './modules/savings-history'; import { setupRIExchangeHandlers, saveAutomationSettings } from './riexchange'; import { showToast } from './toast'; @@ -167,6 +167,12 @@ export function setupEventListeners(): void { // Setup savings history charts initSavingsHistory(); + // Wire provider/account topbar filter chips to Purchase History + + // Approval Queue (issue #701). initSavingsHistory above subscribes + // the chart; setupHistoryHandlers subscribes the two list consumers + // so all three reload together when a chip changes. + setupHistoryHandlers(); + // Setup feedback link setupFeedbackLink(); diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 7b3926916..928e4e651 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -35,6 +35,11 @@ export async function loadSavingsHistory(): Promise { const currentProvider = state.getCurrentProvider(); const currentAccountIDs = state.getCurrentAccountIDs(); + // Build a human-readable description of the active filter so the + // empty-state message distinguishes "no data yet" from "nothing in + // the selected scope". Used when the API returns 0 data points. + const filterDesc = buildFilterDesc(currentProvider, currentAccountIDs); + try { const data = await getSavingsAnalytics({ start: start.toISOString(), @@ -45,7 +50,7 @@ export async function loadSavingsHistory(): Promise { }); if (!data.data_points || data.data_points.length === 0) { - showEmptyState(chartContainer, emptyEl, statsEl); + showEmptyState(chartContainer, emptyEl, statsEl, filterDesc); return; } @@ -57,21 +62,48 @@ export async function loadSavingsHistory(): Promise { renderSavingsStats(data); renderSavingsChart(data.data_points, interval); } catch (error) { - console.error('Failed to load savings history:', error instanceof Error ? error.message : 'Unknown error'); - showEmptyState(chartContainer, emptyEl, statsEl); + const msg = error instanceof Error ? error.message : 'Unknown error'; + console.error('Failed to load savings history:', msg); + showErrorState(chartContainer, emptyEl, statsEl, msg); } } /** - * Show empty state when no data is available + * Show empty state when no data is available. + * + * filterDesc is a short human-readable description of the active + * topbar filter (e.g. "AWS" or "account abc-123"). When non-empty + * the empty-state copy says "no data for the selected filter" so + * the user understands the chart is hidden because of scoping, not + * because no purchases exist at all (issue #701). */ function showEmptyState( chartContainer: HTMLElement | null | undefined, emptyEl: HTMLElement | null, - statsEl: HTMLElement | null + statsEl: HTMLElement | null, + filterDesc: string = '', ): void { if (chartContainer) chartContainer.classList.add('hidden'); - if (emptyEl) emptyEl.classList.remove('hidden'); + if (emptyEl) { + emptyEl.classList.remove('hidden'); + // Update the copy inside the empty-state element so the message + // reflects whether a filter is active. The element is always + // present in the DOM (hidden by CSS class, not removed), so we + // can safely write innerHTML here — this is the only place that + // mutates it and the content is built from trusted constants. + const heading = emptyEl.querySelector('p:first-child'); + const help = emptyEl.querySelector('p.help-text'); + if (heading) { + heading.textContent = filterDesc + ? `No savings data for the selected filter (${filterDesc}).` + : 'No savings history data available yet.'; + } + if (help) { + help.textContent = filterDesc + ? 'Try broadening the filter or selecting a different period.' + : 'Data will be collected hourly once you have active purchases.'; + } + } if (statsEl) statsEl.classList.add('hidden'); // Clear any existing chart @@ -81,6 +113,45 @@ function showEmptyState( } } +/** + * Show an explicit error state when the API call fails. Reuses the empty-state + * DOM element but sets copy that makes clear this is a fetch failure, not + * simply a lack of data (issue #701 CR finding). + */ +function showErrorState( + chartContainer: HTMLElement | null | undefined, + emptyEl: HTMLElement | null, + statsEl: HTMLElement | null, + message: string, +): void { + if (chartContainer) chartContainer.classList.add('hidden'); + if (statsEl) statsEl.classList.add('hidden'); + if (emptyEl) { + emptyEl.classList.remove('hidden'); + const heading = emptyEl.querySelector('p:first-child'); + const help = emptyEl.querySelector('p.help-text'); + if (heading) heading.textContent = 'Failed to load savings history.'; + if (help) help.textContent = `Please retry. (${message})`; + } + if (savingsChart) { + savingsChart.destroy(); + savingsChart = null; + } +} + +/** + * Build a short human-readable description of the active topbar filter + * for use in the Savings History empty-state message. Returns '' when + * no filter is active (no chip selected) so callers can distinguish + * "unfiltered empty" from "filtered empty". + */ +function buildFilterDesc(provider: string, accountIDs: readonly string[]): string { + const parts: string[] = []; + if (provider && provider.toLowerCase() !== 'all') parts.push(provider.toUpperCase()); + if (accountIDs.length > 0) parts.push(accountIDs[0] ?? ''); + return parts.join(', '); +} + /** * Get start/end dates and interval based on period selection */ diff --git a/internal/api/analytics_postgres.go b/internal/api/analytics_postgres.go index 747d7e757..1deed6401 100644 --- a/internal/api/analytics_postgres.go +++ b/internal/api/analytics_postgres.go @@ -93,6 +93,13 @@ func (c *PostgresAnalyticsClient) QueryHistory( // both the top-line bucket totals and the by_service / by_provider // breakdowns without a second trip to the DB. // + // account_id is the legacy VARCHAR(20) external-account identifier + // (e.g. a 12-digit AWS account number) written by older code paths. + // cloud_account_id is the UUID FK to cloud_accounts added in migration + // 000011 and written by all current purchase flows. The topbar Account + // chip sends the cloud_accounts.id UUID, so we match BOTH columns so + // that rows written by either path are included (issue #701). + // // #nosec G201 — `unit` is allowlisted by intervalToTruncUnit above. query := fmt.Sprintf(` SELECT date_trunc('%s', timestamp) AS bucket, @@ -104,7 +111,7 @@ func (c *PostgresAnalyticsClient) QueryHistory( FROM purchase_history WHERE timestamp >= $1 AND timestamp <= $2 - AND ($3 = '' OR account_id = $3) + AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3) GROUP BY bucket, service, provider ORDER BY bucket ASC `, unit) @@ -186,6 +193,9 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( return nil, err } + // account_id / cloud_account_id dual-column match: see QueryHistory + // comment for rationale (issue #701). + // // #nosec G201 — `column` is allowlisted by dimensionToColumn above. query := fmt.Sprintf(` SELECT %s AS bucket, @@ -195,7 +205,7 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( FROM purchase_history WHERE timestamp >= $1 AND timestamp <= $2 - AND ($3 = '' OR account_id = $3) + AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3) GROUP BY bucket ORDER BY savings DESC `, column) diff --git a/internal/api/analytics_postgres_test.go b/internal/api/analytics_postgres_test.go index df4789aab..e05bef11e 100644 --- a/internal/api/analytics_postgres_test.go +++ b/internal/api/analytics_postgres_test.go @@ -164,3 +164,56 @@ func TestQueryBreakdown_ZeroTotalYieldsZeroPct(t *testing.T) { assert.Equal(t, 0.0, out["ec2"].Percentage, "percentage must be 0 when total savings is 0") assert.NoError(t, mock.ExpectationsWereMet()) } + +// TestQueryHistory_CloudAccountIDFilter verifies that QueryHistory accepts a +// cloud_accounts UUID as the accountID parameter (issue #701). The WHERE +// clause matches on BOTH account_id (legacy VARCHAR(20) external ID) and +// cloud_account_id::text (UUID FK), so rows written by either code path are +// included. pgxmock validates the SQL shape and arg binding. +func TestQueryHistory_CloudAccountIDFilter(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + uuid := "aabbccdd-1234-5678-abcd-aabbccddee00" + + 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 \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). + 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, summary, err := client.QueryHistory(ctx, uuid, start, end, "daily") + require.NoError(t, err) + require.Len(t, points, 1) + assert.InDelta(t, 50.0, points[0].TotalSavings, 1e-9) + require.NotNil(t, summary) + assert.Equal(t, 1, summary.TotalPurchases) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestQueryBreakdown_CloudAccountIDFilter verifies that QueryBreakdown accepts +// a UUID as the accountID (issue #701). Mirrors TestQueryHistory_CloudAccountIDFilter. +func TestQueryBreakdown_CloudAccountIDFilter(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + uuid := "aabbccdd-1234-5678-abcd-aabbccddee00" + + rows := mock.NewRows([]string{"bucket", "savings", "upfront", "purchases"}). + AddRow("rds", 200.0, 80.0, 3) + + mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). + WillReturnRows(rows) + + out, err := client.QueryBreakdown(ctx, uuid, + time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC), + time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC), + "service") + require.NoError(t, err) + require.Len(t, out, 1) + assert.InDelta(t, 200.0, out["rds"].TotalSavings, 1e-9) + assert.NoError(t, mock.ExpectationsWereMet()) +}