diff --git a/frontend/src/__tests__/api-inventory.test.ts b/frontend/src/__tests__/api-inventory.test.ts index af6672b0..8de419c2 100644 --- a/frontend/src/__tests__/api-inventory.test.ts +++ b/frontend/src/__tests__/api-inventory.test.ts @@ -1,9 +1,10 @@ /** - * Tests for the inventory API module (issue #340 deferred sub-task). + * Tests for the inventory API module (issue #340 deferred sub-task, #866). * * Verifies the wire format / envelope handling for - * GET /api/inventory/commitments. The backend handler logic is covered - * in handler_inventory_test.go; here we lock down the client adapter. + * GET /api/inventory/commitments and GET /api/inventory/coverage, + * including the provider + account_id query params added by issue #866. + * Backend handler logic is covered in handler_inventory_test.go. */ import { apiRequest } from '../api/client'; @@ -18,7 +19,7 @@ describe('listActiveCommitments', () => { (apiRequest as jest.Mock).mockReset(); }); - test('calls /inventory/commitments without query string by default', async () => { + test('calls /inventory/commitments without query string when no filter', async () => { (apiRequest as jest.Mock).mockResolvedValue({ commitments: [] }); const result = await listActiveCommitments(); @@ -30,11 +31,29 @@ describe('listActiveCommitments', () => { test('appends URL-encoded account_id when scoped to one account', async () => { (apiRequest as jest.Mock).mockResolvedValue({ commitments: [] }); - await listActiveCommitments('acc/with special'); + await listActiveCommitments({ accountID: 'acc/with special' }); expect(apiRequest).toHaveBeenCalledWith('/inventory/commitments?account_id=acc%2Fwith%20special'); }); + test('appends provider query param when provider filter is set', async () => { + (apiRequest as jest.Mock).mockResolvedValue({ commitments: [] }); + + await listActiveCommitments({ provider: 'aws' }); + + expect(apiRequest).toHaveBeenCalledWith('/inventory/commitments?provider=aws'); + }); + + test('appends both account_id and provider when both are set', async () => { + (apiRequest as jest.Mock).mockResolvedValue({ commitments: [] }); + + await listActiveCommitments({ accountID: 'acc-1', provider: 'azure' }); + + const url = (apiRequest as jest.Mock).mock.calls[0][0] as string; + expect(url).toContain('account_id=acc-1'); + expect(url).toContain('provider=azure'); + }); + test('returns the commitments array unwrapped from the envelope', async () => { const commitments = [ { @@ -74,7 +93,7 @@ describe('getCoverageBreakdown', () => { (apiRequest as jest.Mock).mockReset(); }); - test('calls /inventory/coverage', async () => { + test('calls /inventory/coverage without query string when no filter', async () => { (apiRequest as jest.Mock).mockResolvedValue({ providers: [] }); await getCoverageBreakdown(); @@ -82,6 +101,32 @@ describe('getCoverageBreakdown', () => { expect(apiRequest).toHaveBeenCalledWith('/inventory/coverage'); }); + test('appends provider query param when provider filter is set', async () => { + (apiRequest as jest.Mock).mockResolvedValue({ providers: [] }); + + await getCoverageBreakdown({ provider: 'gcp' }); + + expect(apiRequest).toHaveBeenCalledWith('/inventory/coverage?provider=gcp'); + }); + + test('appends account_id query param when accountID filter is set', async () => { + (apiRequest as jest.Mock).mockResolvedValue({ providers: [] }); + + await getCoverageBreakdown({ accountID: 'acc-42' }); + + expect(apiRequest).toHaveBeenCalledWith('/inventory/coverage?account_id=acc-42'); + }); + + test('appends both params when both are set', async () => { + (apiRequest as jest.Mock).mockResolvedValue({ providers: [] }); + + await getCoverageBreakdown({ accountID: 'acc-1', provider: 'aws' }); + + const url = (apiRequest as jest.Mock).mock.calls[0][0] as string; + expect(url).toContain('account_id=acc-1'); + expect(url).toContain('provider=aws'); + }); + test('returns the full response envelope including providers array', async () => { const payload = { providers: [ diff --git a/frontend/src/__tests__/inventory.test.ts b/frontend/src/__tests__/inventory.test.ts index 052e7a02..04e88bc1 100644 --- a/frontend/src/__tests__/inventory.test.ts +++ b/frontend/src/__tests__/inventory.test.ts @@ -1,8 +1,10 @@ /** - * Inventory & Coverage section tests (issue #340 T4, #754). + * Inventory & Coverage section tests (issue #340 T4, #754, #866). * * Verifies the sub-tab switching machinery for the umbrella section AND - * the per-commitment / coverage fetch+render flows. + * the per-commitment / coverage fetch+render flows, AND the chip- + * subscription pattern (issue #866: Main Header chips must propagate to + * both sub-tabs). */ // loadRIExchange is a side-effect import from a module that touches the @@ -20,9 +22,22 @@ jest.mock('../api', () => ({ getCoverageBreakdown: jest.fn(), })); +// Mock state so chip subscription tests can control provider/account +// values and capture the registered callbacks. subscribeProvider and +// subscribeAccount must return a function (the unsubscribe handle) since +// inventory.ts calls the return value to tear down old subscriptions on +// repeated loadInventory() calls (feedback_event_listener_dedup pattern). +jest.mock('../state', () => ({ + subscribeProvider: jest.fn(() => jest.fn()), + subscribeAccount: jest.fn(() => jest.fn()), + getCurrentProvider: jest.fn(() => ''), + getCurrentAccountIDs: jest.fn(() => []), +})); + import { loadInventory, switchInventorySubSection, loadActiveCommitments, loadCoverageBreakdown } from '../inventory'; import { loadRIExchange } from '../riexchange'; import * as api from '../api'; +import * as state from '../state'; import type { ProviderCoverageSection } from '../api'; function buildInventoryDOM(): void { @@ -126,6 +141,13 @@ describe('Inventory & Coverage sub-section switching', () => { // getCoverageBreakdown is invoked when switching to the coverage sub-tab. (api.getCoverageBreakdown as jest.Mock).mockReset(); (api.getCoverageBreakdown as jest.Mock).mockResolvedValue({ providers: [] }); + // Reset state mocks to defaults. subscribeProvider/Account must keep + // returning an unsubscribe function so inventory.ts can call it during + // teardown without throwing. + (state.subscribeProvider as jest.Mock).mockReset().mockReturnValue(jest.fn()); + (state.subscribeAccount as jest.Mock).mockReset().mockReturnValue(jest.fn()); + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); }); afterEach(() => { @@ -193,6 +215,10 @@ describe('loadActiveCommitments — fetch + render flow', () => { beforeEach(() => { buildInventoryDOM(); (api.listActiveCommitments as jest.Mock).mockReset(); + (state.subscribeProvider as jest.Mock).mockReturnValue(jest.fn()); + (state.subscribeAccount as jest.Mock).mockReturnValue(jest.fn()); + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); }); afterEach(() => { @@ -295,6 +321,10 @@ describe('loadCoverageBreakdown — fetch + render flow', () => { beforeEach(() => { buildInventoryDOM(); (api.getCoverageBreakdown as jest.Mock).mockReset(); + (state.subscribeProvider as jest.Mock).mockReturnValue(jest.fn()); + (state.subscribeAccount as jest.Mock).mockReturnValue(jest.fn()); + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); }); afterEach(() => { @@ -413,3 +443,196 @@ describe('loadCoverageBreakdown — fetch + render flow', () => { expect(barTh!.getAttribute('aria-label')).toBe('Coverage bar'); }); }); + +// ────────────────────────────────────────────── +// Chip subscription wiring (issue #866) +// +// Main Header global chips (Provider + Account) must propagate to the +// Active Commitments and Coverage sub-tabs. Mirrors the savings-history +// subscriber tests (PR #741) and dashboard tests (PR #747). +// ────────────────────────────────────────────── + +describe('chip subscriptions (issue #866)', () => { + /** + * Add an inventory-tab div with the .active class so isInventoryTabActive() + * returns true. The DOM is built fresh in buildInventoryDOM but doesn't + * receive the .active class — add it here for the guard to pass. + */ + function activateInventoryTab(): void { + const tab = document.getElementById('inventory-tab'); + if (tab) tab.classList.add('active'); + } + + beforeEach(() => { + buildInventoryDOM(); + (api.listActiveCommitments as jest.Mock).mockResolvedValue([]); + (api.getCoverageBreakdown as jest.Mock).mockResolvedValue({ providers: [] }); + // Each test's loadInventory() call will call subscribeProvider/Account; + // return a fresh jest.fn() as the unsubscribe handle each time. + (state.subscribeProvider as jest.Mock).mockReset().mockReturnValue(jest.fn()); + (state.subscribeAccount as jest.Mock).mockReset().mockReturnValue(jest.fn()); + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + }); + + afterEach(() => { + clearDOM(); + }); + + test('loadInventory registers callbacks with state.subscribeProvider and state.subscribeAccount', () => { + loadInventory(); + + expect(state.subscribeProvider).toHaveBeenCalledTimes(1); + expect(state.subscribeAccount).toHaveBeenCalledTimes(1); + expect(typeof (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0]).toBe('function'); + expect(typeof (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0]).toBe('function'); + }); + + test('account chip change re-fetches Active Commitments when inventory tab is active', async () => { + activateInventoryTab(); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-X']); + + // Start on active-commitments sub-tab. Force the sub-section in case + // a prior test left module-scoped currentSubSection at 'coverage'. + loadInventory(); + switchInventorySubSection('active-commitments'); + + // Clear the initial load call, then simulate chip change. + (api.listActiveCommitments as jest.Mock).mockClear(); + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + accountCb(); + // queueMicrotask drains within a setTimeout(0). + await new Promise((r) => setTimeout(r, 0)); + + expect(api.listActiveCommitments).toHaveBeenCalledTimes(1); + expect(api.listActiveCommitments).toHaveBeenCalledWith( + expect.objectContaining({ accountID: 'acct-X' }) + ); + }); + + test('provider chip change re-fetches Active Commitments when inventory tab is active', async () => { + activateInventoryTab(); + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + + loadInventory(); + switchInventorySubSection('active-commitments'); + + (api.listActiveCommitments as jest.Mock).mockClear(); + const providerCb = (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + providerCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.listActiveCommitments).toHaveBeenCalledTimes(1); + expect(api.listActiveCommitments).toHaveBeenCalledWith( + expect.objectContaining({ provider: 'aws' }) + ); + }); + + test('provider chip change re-fetches Coverage when coverage sub-tab is active', async () => { + activateInventoryTab(); + (state.getCurrentProvider as jest.Mock).mockReturnValue('azure'); + + loadInventory(); + // Switch to the coverage sub-tab. + switchInventorySubSection('coverage'); + (api.getCoverageBreakdown as jest.Mock).mockClear(); + + const providerCb = (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + providerCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getCoverageBreakdown).toHaveBeenCalledTimes(1); + expect(api.getCoverageBreakdown).toHaveBeenCalledWith( + expect.objectContaining({ provider: 'azure' }) + ); + }); + + test('account chip change re-fetches Coverage when coverage sub-tab is active', async () => { + activateInventoryTab(); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-Y']); + + loadInventory(); + switchInventorySubSection('coverage'); + (api.getCoverageBreakdown as jest.Mock).mockClear(); + + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getCoverageBreakdown).toHaveBeenCalledTimes(1); + expect(api.getCoverageBreakdown).toHaveBeenCalledWith( + expect.objectContaining({ accountID: 'acct-Y' }) + ); + }); + + test('does NOT re-fetch when inventory tab is inactive (active-tab guard)', async () => { + // First load with tab active to fix module-scoped currentSubSection + // to 'active-commitments'. Then deactivate the tab and verify the + // chip callbacks early-return. + activateInventoryTab(); + loadInventory(); + switchInventorySubSection('active-commitments'); + const tab = document.getElementById('inventory-tab'); + if (tab) tab.classList.remove('active'); + + (api.listActiveCommitments as jest.Mock).mockClear(); + (api.getCoverageBreakdown as jest.Mock).mockClear(); + const providerCb = (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + providerCb(); + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.listActiveCommitments).not.toHaveBeenCalled(); + expect(api.getCoverageBreakdown).not.toHaveBeenCalled(); + }); + + test('coalesces back-to-back provider+account fires into one fetch', async () => { + activateInventoryTab(); + + loadInventory(); + switchInventorySubSection('active-commitments'); + (api.listActiveCommitments as jest.Mock).mockClear(); + + const providerCb = (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + + // Simulate topbar: clear accounts then set provider synchronously. + accountCb(); + providerCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.listActiveCommitments).toHaveBeenCalledTimes(1); + }); + + test('filter-aware empty state: shows provider name when provider chip is set', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('gcp'); + + await loadActiveCommitments(); + + const list = document.getElementById('active-commitments-list')!; + const empty = list.querySelector('.empty'); + expect(empty).not.toBeNull(); + expect(empty!.textContent).toContain('"gcp"'); + }); + + test('filter-aware empty state: shows account ID when account chip is set', async () => { + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-42']); + + await loadActiveCommitments(); + + const list = document.getElementById('active-commitments-list')!; + const empty = list.querySelector('.empty'); + expect(empty).not.toBeNull(); + expect(empty!.textContent).toContain('acct-42'); + }); + + test('generic empty state: shown when no chip filters are active', async () => { + await loadActiveCommitments(); + + const list = document.getElementById('active-commitments-list')!; + const empty = list.querySelector('.empty'); + expect(empty).not.toBeNull(); + expect(empty!.textContent).toMatch(/no active commitments found across your registered accounts/i); + }); +}); diff --git a/frontend/src/api/inventory.ts b/frontend/src/api/inventory.ts index 288f9cf8..32ce5c19 100644 --- a/frontend/src/api/inventory.ts +++ b/frontend/src/api/inventory.ts @@ -10,14 +10,23 @@ import { apiRequest } from './client'; import type { CoverageBreakdownResponse, InventoryCommitment } from './types'; +/** Shared filter params for the two Inventory endpoints. */ +export interface InventoryFilter { + provider?: string; + accountID?: string; +} + /** * List active (non-expired) commitments across the user's accessible - * accounts. Pass `accountID` to scope the read to a single account; omit - * for the full cross-account list. The backend always filters expired - * rows out and sorts by soonest-expiring first. + * accounts. Both `provider` and `accountID` are forwarded as query params + * so the global topbar chips propagate into the result set (issue #866). + * The backend filters expired rows and sorts by soonest-expiring first. */ -export async function listActiveCommitments(accountID?: string): Promise { - const qs = accountID ? `?account_id=${encodeURIComponent(accountID)}` : ''; +export async function listActiveCommitments(filter: InventoryFilter = {}): Promise { + const parts: string[] = []; + if (filter.accountID) parts.push(`account_id=${encodeURIComponent(filter.accountID)}`); + if (filter.provider) parts.push(`provider=${encodeURIComponent(filter.provider)}`); + const qs = parts.length > 0 ? `?${parts.join('&')}` : ''; const resp = await apiRequest<{ commitments: InventoryCommitment[] }>(`/inventory/commitments${qs}`); return resp.commitments ?? []; } @@ -26,7 +35,13 @@ export async function listActiveCommitments(accountID?: string): Promise { - return apiRequest('/inventory/coverage'); +export async function getCoverageBreakdown(filter: InventoryFilter = {}): Promise { + const parts: string[] = []; + if (filter.accountID) parts.push(`account_id=${encodeURIComponent(filter.accountID)}`); + if (filter.provider) parts.push(`provider=${encodeURIComponent(filter.provider)}`); + const qs = parts.length > 0 ? `?${parts.join('&')}` : ''; + return apiRequest(`/inventory/coverage${qs}`); } diff --git a/frontend/src/inventory.ts b/frontend/src/inventory.ts index 8ca889a8..84f277f7 100644 --- a/frontend/src/inventory.ts +++ b/frontend/src/inventory.ts @@ -15,6 +15,7 @@ import type { ProviderCoverageSection, CoverageServiceRow } from './api'; import { loadRIExchange } from './riexchange'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { formatCurrency, formatDate } from './utils'; +import * as state from './state'; type InventorySubSection = 'active-commitments' | 'coverage' | 'ri-exchange'; @@ -76,6 +77,9 @@ const ACTIVE_COMMITMENTS_COLS = 10; * children with a shimmer skeleton on entry, then either the rendered * table or an empty-state / error paragraph on completion. Idempotent — * safe to call on every sub-tab switch and on every refresh click. + * + * Reads the current provider/account chips from state so that changing a + * chip while on this sub-tab re-fetches with the new scope (issue #866). */ export async function loadActiveCommitments(): Promise { const container = document.getElementById(ACTIVE_COMMITMENTS_LIST_ID); @@ -83,14 +87,19 @@ export async function loadActiveCommitments(): Promise { wireRefreshButton(); + const provider = state.getCurrentProvider(); + const accountIDs = state.getCurrentAccountIDs(); + // account chip is single-select; forward only when exactly one is active. + const accountID = accountIDs.length === 1 ? accountIDs[0] : undefined; + // 5 rows × 10 cols matches the rendered table shape (see // renderActiveCommitmentsTable). The renderer wipes the container's // children for a clean handoff from the skeleton. showSkeletonRows(container, 5, ACTIVE_COMMITMENTS_COLS); try { - const commitments = await api.listActiveCommitments(); - renderActiveCommitmentsTable(container, commitments); + const commitments = await api.listActiveCommitments({ provider: provider || undefined, accountID }); + renderActiveCommitmentsTable(container, commitments, provider, accountID); } catch (error) { teardownSkeleton(container); const err = error as Error; @@ -133,18 +142,45 @@ function renderEmptyParagraph(container: HTMLElement, message: string): void { container.appendChild(p); } +/** + * Build a context-aware empty-state message for the active-commitments + * table. When chip filters are active the message names the scope so + * the user knows the result is filtered rather than globally empty. + */ +function buildActiveCommitmentsEmptyMessage(provider?: string, accountID?: string): string { + if (provider && accountID) { + return `No active commitments for provider "${provider}" and account ${accountID}.`; + } + if (provider) { + return `No active commitments for provider "${provider}".`; + } + if (accountID) { + return `No active commitments for account ${accountID}.`; + } + return 'No active commitments found across your registered accounts.'; +} + /** * Render the per-commitment table into `container`. Empty list yields * an inline `.empty` paragraph instead of an empty table so the user * gets a real message ("no active commitments"), not a blank header. * + * When a chip filter is active the empty-state message names the scope + * so the user understands the result is filtered, not globally empty. + * * All text uses textContent / DOM construction — no innerHTML — to * keep the section safe by default against any unescaped backend * field (issue #340 XSS posture). */ -function renderActiveCommitmentsTable(container: HTMLElement, commitments: api.InventoryCommitment[]): void { +function renderActiveCommitmentsTable( + container: HTMLElement, + commitments: api.InventoryCommitment[], + provider?: string, + accountID?: string, +): void { if (!commitments || commitments.length === 0) { - renderEmptyParagraph(container, 'No active commitments found across your registered accounts.'); + const msg = buildActiveCommitmentsEmptyMessage(provider, accountID); + renderEmptyParagraph(container, msg); return; } @@ -228,6 +264,9 @@ const PROVIDER_DISPLAY_NAMES: Record = { * Fetch and render per-provider coverage breakdowns into #coverage-providers. * Shows a skeleton on entry, then either the rendered sections or an error. * Idempotent — safe to call on every sub-tab switch and on every refresh click. + * + * Reads the current provider/account chips from state so that changing a + * chip while on this sub-tab re-fetches with the new scope (issue #866). */ export async function loadCoverageBreakdown(): Promise { const container = document.getElementById(COVERAGE_CONTAINER_ID); @@ -235,11 +274,15 @@ export async function loadCoverageBreakdown(): Promise { wireCoverageRefreshButton(); + const provider = state.getCurrentProvider(); + const accountIDs = state.getCurrentAccountIDs(); + const accountID = accountIDs.length === 1 ? accountIDs[0] : undefined; + // One skeleton row per known provider while loading. showSkeletonRows(container, 3, 1); try { - const data = await api.getCoverageBreakdown(); + const data = await api.getCoverageBreakdown({ provider: provider || undefined, accountID }); renderCoverageBreakdown(container, data.providers); } catch (error) { teardownSkeleton(container); @@ -380,6 +423,65 @@ function wireSubNavListeners(): void { listenersWired = true; } +/** + * True when the Inventory & Coverage tab is the currently-visible top-level + * tab. The chip-subscription reload skips the fetch when this returns false + * so we don't burn an API call (or trigger a skeleton flash) for a section + * the user isn't looking at — switchTab('inventory') runs loadInventory() + * on next entry anyway, which re-fetches with the current chip state. + */ +function isInventoryTabActive(): boolean { + return document.getElementById('inventory-tab')?.classList.contains('active') === true; +} + +// Unsubscribe handles for the chip subscriptions. Re-assigned each time +// loadInventory() wires them so repeated tab-switches don't stack duplicate +// listeners — the old pair is torn down before a new pair is registered. +let unsubscribeProvider: (() => void) | null = null; +let unsubscribeAccount: (() => void) | null = null; + +/** + * Wire provider + account chip subscriptions (issue #866). + * + * Mirrors the pattern from PR #741 (Purchases) and PR #747 (Home): + * - Active-tab guard: only fire when the Inventory tab is active. + * - queueMicrotask coalescing: topbar-filters.ts fires BOTH the + * account-clear AND the provider-set subscribers synchronously on a + * single chip change. Without coalescing the two back-to-back fires + * would kick off two fetches; with it they collapse into one. + * - Re-check the active-tab guard inside the microtask: a tab switch + * between the chip change and the microtask flush cancels the + * now-unneeded fetch. + * + * Called from loadInventory() on every Inventory tab-switch. Tears down + * the previous subscription pair first so repeated switches don't stack + * duplicate listeners. + */ +function wireChipSubscriptions(): void { + // Tear down any existing subscriptions to avoid stacking on repeated + // tab-switches. + if (unsubscribeProvider) { unsubscribeProvider(); unsubscribeProvider = null; } + if (unsubscribeAccount) { unsubscribeAccount(); unsubscribeAccount = null; } + + let reloadQueued = false; + const scheduleReload = (): void => { + if (!isInventoryTabActive() || reloadQueued) return; + reloadQueued = true; + queueMicrotask(() => { + reloadQueued = false; + if (!isInventoryTabActive()) return; + if (currentSubSection === 'active-commitments') { + void loadActiveCommitments(); + } else if (currentSubSection === 'coverage') { + void loadCoverageBreakdown(); + } + }); + }; + + unsubscribeProvider = state.subscribeProvider(scheduleReload); + unsubscribeAccount = state.subscribeAccount(scheduleReload); +} + /** * Initialize the Inventory & Coverage section. Called by navigation.ts' * switchTab when 'inventory' is selected. Defaults to active-commitments @@ -387,5 +489,6 @@ function wireSubNavListeners(): void { */ export function loadInventory(): void { wireSubNavListeners(); + wireChipSubscriptions(); switchInventorySubSection(currentSubSection ?? DEFAULT_SUB_SECTION); } diff --git a/internal/api/handler_inventory.go b/internal/api/handler_inventory.go index 6323ffda..3fdc4ed5 100644 --- a/internal/api/handler_inventory.go +++ b/internal/api/handler_inventory.go @@ -70,16 +70,42 @@ func (h *Handler) listActiveCommitments(ctx context.Context, req *events.LambdaF } // fetchCommitmentRecords reads purchase history from the store, honouring -// an optional `account_id` query param the same way fetchPurchaseHistory -// does for /api/history. Limit defaults to MaxListLimit — commitments -// are a strict subset of purchase history (we drop expired rows before -// returning) so a high cap is appropriate; an over-truncation here -// would silently hide rows the user is entitled to see. +// optional `account_id` and `provider` query params the same way +// fetchPurchaseHistory does for /api/history. Limit defaults to +// MaxListLimit — commitments are a strict subset of purchase history (we +// drop expired rows before returning) so a high cap is appropriate; an +// over-truncation here would silently hide rows the user is entitled to. +// +// `provider` filtering is applied in-memory after the store read so that +// the existing single-account and all-accounts store paths remain +// unchanged; the record set is small enough that a post-read filter has +// negligible cost. func (h *Handler) fetchCommitmentRecords(ctx context.Context, params map[string]string) ([]config.PurchaseHistoryRecord, error) { + var rows []config.PurchaseHistoryRecord + var err error if accountID := params["account_id"]; accountID != "" { - return h.config.GetPurchaseHistory(ctx, accountID, config.MaxListLimit) + rows, err = h.config.GetPurchaseHistory(ctx, accountID, config.MaxListLimit) + } else { + rows, err = h.config.GetAllPurchaseHistory(ctx, config.MaxListLimit) + } + if err != nil { + return nil, err } - return h.config.GetAllPurchaseHistory(ctx, config.MaxListLimit) + + // Apply provider filter in-memory. An absent or empty param means + // "all providers". Case-sensitive match — providers are always + // lowercase in the store (aws, azure, gcp). + if provider := params["provider"]; provider != "" { + filtered := rows[:0] + for _, r := range rows { + if r.Provider == provider { + filtered = append(filtered, r) + } + } + rows = filtered + } + + return rows, nil } // buildInventoryCommitment maps a PurchaseHistoryRecord to the @@ -151,7 +177,11 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu // --- on-demand gap: recommendations ------------------------------------- // Recommendations represent uncommitted demand that could be purchased. // Their Savings field is the monthly on-demand cost of the uncovered gap. - recs, err := h.scheduler.ListRecommendations(ctx, config.RecommendationFilter{}) + // Scope recs to the account chip the same way fetchCommitmentRecords scopes + // commitments above — otherwise the covered side honours the chip but the + // on-demand side bleeds in other accounts' gaps, producing misleading + // per-service coverage (issue #866 follow-up: CR pass on PR #881). + recs, err := h.scheduler.ListRecommendations(ctx, buildCoverageRecFilter(params)) if err != nil { // Non-fatal: recommendations are best-effort for coverage display. // An empty rec list is treated as "no uncovered gap" — coverage @@ -166,14 +196,43 @@ func (h *Handler) getCoverageBreakdown(ctx context.Context, req *events.LambdaFu return nil, err } } - onDemandByKey := make(map[string]float64) - for _, rec := range recs { - onDemandByKey[rec.Provider+":"+rec.Service] += rec.Savings - } + onDemandByKey := aggregateOnDemandByKey(recs, params["provider"]) return buildCoverageBreakdown(coveredByKey, onDemandByKey), nil } +// buildCoverageRecFilter translates the query-string chip params into a +// RecommendationFilter for ListRecommendations. Currently scopes by +// account_id only — provider filtering is applied in aggregateOnDemandByKey +// because the response envelope always includes the full known-providers +// list (a per-provider filter at fetch time would still need an in-memory +// pass to enumerate the other providers as "no usage detected"). +// +// Extracted from getCoverageBreakdown to keep that function under the +// gocyclo budget after PR #881's extraction. +func buildCoverageRecFilter(params map[string]string) config.RecommendationFilter { + filter := config.RecommendationFilter{} + if accountID := params["account_id"]; accountID != "" { + filter.AccountIDs = []string{accountID} + } + return filter +} + +// aggregateOnDemandByKey builds the "provider:service" → monthly-savings map +// from a recommendation slice, applying an optional provider filter. +// Pulled out of getCoverageBreakdown to keep that function under the +// cyclomatic limit. +func aggregateOnDemandByKey(recs []config.RecommendationRecord, providerFilter string) map[string]float64 { + out := make(map[string]float64) + for _, rec := range recs { + if providerFilter != "" && rec.Provider != providerFilter { + continue + } + out[rec.Provider+":"+rec.Service] += rec.Savings + } + return out +} + // buildCoverageBreakdown constructs the CoverageBreakdownResponse from // the two pre-aggregated maps. Extracted so it can be unit-tested without // requiring a full Handler. diff --git a/internal/api/handler_inventory_test.go b/internal/api/handler_inventory_test.go index 76eeb5c1..047dafec 100644 --- a/internal/api/handler_inventory_test.go +++ b/internal/api/handler_inventory_test.go @@ -165,6 +165,54 @@ func TestHandler_listActiveCommitments_AccountFilter(t *testing.T) { mockStore.AssertNotCalled(t, "GetAllPurchaseHistory") } +// TestHandler_listActiveCommitments_ProviderFilter verifies the provider +// query param filters results in-memory, returning only commitments whose +// Provider field matches the requested value (issue #866). +func TestHandler_listActiveCommitments_ProviderFilter(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + now := time.Now() + purchases := []config.PurchaseHistoryRecord{ + { + AccountID: "acc-1", + PurchaseID: "p-aws", + Provider: "aws", + Service: "ec2", + Timestamp: now.AddDate(0, -3, 0), + Term: 1, + Count: 1, + MonthlyCost: 80.0, + }, + { + AccountID: "acc-1", + PurchaseID: "p-azure", + Provider: "azure", + Service: "compute", + Timestamp: now.AddDate(0, -3, 0), + Term: 1, + Count: 2, + MonthlyCost: 120.0, + }, + } + + mockStore.On("GetAllPurchaseHistory", ctx, config.MaxListLimit).Return(purchases, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{{ID: "acc-1", Name: "Account One"}}, nil + } + + mockAuth, req := adminInventoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore} + + result, err := handler.listActiveCommitments(ctx, req, map[string]string{"provider": "aws"}) + require.NoError(t, err) + + resp := result.(InventoryCommitmentsResponse) + require.Len(t, resp.Commitments, 1, "only the aws commitment should pass the provider filter") + assert.Equal(t, "aws", resp.Commitments[0].Provider) + assert.Equal(t, "p-aws", splitPurchaseID(resp.Commitments[0].ID)) +} + // TestHandler_listActiveCommitments_SortedByExpiry verifies soonest-expiring // is first — the dashboard framing is "what do I need to renew next?", so // surfacing the imminent end_date on top keeps the UI's order intuitive @@ -407,3 +455,121 @@ func TestHandler_getCoverageBreakdown_Integration(t *testing.T) { // coverage = 150 / (150+350) * 100 = 30% assert.InDelta(t, 30.0, *aws.OverallCoveragePct, 0.001) } + +// TestHandler_getCoverageBreakdown_ProviderAndAccountChip locks down the CR +// follow-up to PR #881: when the Main Header chips select BOTH a provider and +// an account, getCoverageBreakdown must scope the on-demand (recommendations) +// side AND the covered (commitments) side to the same account. Before the +// fix the on-demand leg called ListRecommendations with a zero filter, so +// the covered side respected the account chip while the on-demand side +// bled in other accounts' gaps — producing misleading per-service coverage. +// +// The test seeds commitments + recs spanning two accounts and two providers, +// then asserts: +// - GetPurchaseHistory is called with the selected account (single-account +// read path), GetAllPurchaseHistory is NOT called. +// - ListRecommendations is called with RecommendationFilter.AccountIDs set +// to the selected account — the assertion the regression hinges on. +// - The aws provider section reflects only the acc-1+aws rows on both +// sides (covered=200, on-demand=300 → 40% coverage). +func TestHandler_getCoverageBreakdown_ProviderAndAccountChip(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockScheduler := new(MockScheduler) + t.Cleanup(func() { + mockStore.AssertExpectations(t) + mockScheduler.AssertExpectations(t) + }) + + now := time.Now() + // Only the acc-1 + aws commitment should reach the aws section. + // GetPurchaseHistory(acc-1) is the single-account read path; the + // account_id chip routes through it, so the store only returns acc-1 + // rows here. The provider chip is applied in-memory by + // fetchCommitmentRecords to drop the azure row. + acc1Purchases := []config.PurchaseHistoryRecord{ + { + AccountID: "acc-1", + PurchaseID: "p-aws-acc1", + Provider: "aws", + Service: "ec2", + Timestamp: now.AddDate(0, -3, 0), + Term: 1, + MonthlyCost: 200.0, + }, + { + AccountID: "acc-1", + PurchaseID: "p-azure-acc1", + Provider: "azure", + Service: "compute", + Timestamp: now.AddDate(0, -3, 0), + Term: 1, + MonthlyCost: 999.0, // must be dropped by the provider=aws chip + }, + } + + // The mock scheduler simulates a real store: it only returns recs that + // match the filter. The assertion that AccountIDs is plumbed lives on + // the mock's expected-argument match — if the handler regresses to + // passing RecommendationFilter{} the mock won't match and the test + // will fail with an unexpected-call message naming the actual call. + acc1Recs := []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Savings: 300.0, CloudAccountID: strPtr("acc-1")}, + // An azure rec for the same account exists in the real store but + // would be filtered out by aggregateOnDemandByKey's provider chip. + {Provider: "azure", Service: "compute", Savings: 999.0, CloudAccountID: strPtr("acc-1")}, + } + + mockStore.On("GetPurchaseHistory", ctx, "acc-1", config.MaxListLimit).Return(acc1Purchases, nil) + mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) { + return []config.CloudAccount{ + {ID: "acc-1", Name: "Account One"}, + {ID: "acc-2", Name: "Account Two"}, + }, nil + } + // The crux of the F1 fix: the on-demand fetch MUST scope by account_id. + mockScheduler.On( + "ListRecommendations", + ctx, + config.RecommendationFilter{AccountIDs: []string{"acc-1"}}, + ).Return(acc1Recs, nil) + + mockAuth, req := adminInventoryReq(ctx) + handler := &Handler{auth: mockAuth, config: mockStore, scheduler: mockScheduler} + + result, err := handler.getCoverageBreakdown(ctx, req, map[string]string{ + "provider": "aws", + "account_id": "acc-1", + }) + require.NoError(t, err) + + resp, ok := result.(CoverageBreakdownResponse) + require.True(t, ok) + require.Len(t, resp.Providers, 3, "envelope always carries 3 known providers") + + // Locate the aws section by provider (knownProviders ordering is stable + // but we look it up by name so a future reorder doesn't masquerade as a + // real regression). + var aws *ProviderCoverageSection + for i := range resp.Providers { + if resp.Providers[i].Provider == "aws" { + aws = &resp.Providers[i] + break + } + } + require.NotNil(t, aws) + require.NotNil(t, aws.Services, "aws section must have services from the acc-1 rows") + require.Len(t, aws.Services, 1, "only ec2 contributes after provider=aws chip") + + ec2 := aws.Services[0] + assert.Equal(t, "ec2", ec2.Service) + assert.Equal(t, 200.0, ec2.CoveredMonthly, "covered comes from acc-1 purchase only") + assert.Equal(t, 300.0, ec2.OnDemandMonthly, "on-demand comes from acc-1 rec only") + require.NotNil(t, ec2.CoveragePct) + // 200 / (200+300) * 100 = 40 + assert.InDelta(t, 40.0, *ec2.CoveragePct, 0.001) + + // GetAllPurchaseHistory must NOT be called when account_id is set — + // belt-and-braces with the per-account read path on the covered side. + mockStore.AssertNotCalled(t, "GetAllPurchaseHistory") +}