From cd686c2089312676b1c75ed5d4042b6cedd152aa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 20:30:01 +0200 Subject: [PATCH 1/2] feat(ui/opportunities): expose AWS recommendation lookback selector (closes #909) Surface the recommendation lookback window (7/30/60 days) directly in the Opportunities view, mirroring the Admin > Settings control. Both read and write the same global recommendations_lookback_days, so changing one is reflected by the other on next load. - Add a #recommendations-toolbar selector rendered by recommendations.ts, prefilled from /api/config (default 7) and gated by the update:config permission: non-admin sessions see a disabled control with a tooltip explaining why, never an editable control whose only outcome is a 403. - Tooltip wording verified against the collectors: only the AWS collector (scheduler.fetchAndConvert -> RecommendationParams.LookbackPeriod) consumes the field; Azure/GCP GetAllRecommendations take no lookback parameter. So the scope is "Applies to AWS recommendations only (Cost Explorer lookback window); Azure and GCP are unaffected." - On change: strict-integer + enum validation, persist the FULL cached config with only the lookback overridden (the backend PUT preserves only the two cycle-params when omitted, so a partial body would wipe siblings), then trigger a recommendations re-collect and reload the list. The selector is disabled while in flight; a persist failure reverts the value and toasts. - Extract the shared refresh+toast+reload block into startRecommendationsRefresh so the stale-on-open path and the lookback-change path dedup against one in-flight collect. Tests: new recommendations-lookback.test.ts (prefill, admin/non-admin gating, persist+refresh+reload, error revert). Full recommendations|settings suite green. --- .../recommendations-lookback.test.ts | 211 +++++++++++++++++ frontend/src/index.html | 6 + frontend/src/recommendations.ts | 213 +++++++++++++++++- frontend/src/styles/components.css | 19 ++ 4 files changed, 447 insertions(+), 2 deletions(-) create mode 100644 frontend/src/__tests__/recommendations-lookback.test.ts diff --git a/frontend/src/__tests__/recommendations-lookback.test.ts b/frontend/src/__tests__/recommendations-lookback.test.ts new file mode 100644 index 000000000..429f8fe21 --- /dev/null +++ b/frontend/src/__tests__/recommendations-lookback.test.ts @@ -0,0 +1,211 @@ +/** + * Issue #909: Opportunities lookback selector tests. + * + * Verifies: + * - the 7/30/60-day selector prefills from /api/config + * - admin sees an editable control with the AWS-scope tooltip + * - non-admin (user / readonly) sees the control disabled with the + * no-permission tooltip (backend stays authoritative) + * - changing the value persists the full config (override only the + * lookback, not wiping other fields), triggers a recommendations + * refresh, and reloads the list + * - a persist failure toasts the error and reverts the selector + * - a tampered/out-of-range value is rejected client-side + */ +import { + loadRecommendations, + resetCachedGlobalConfig, + resetAutoRefreshInFlight, +} from '../recommendations'; +import * as api from '../api'; +import { refreshRecommendations as refreshRecsAPI } from '../api/recommendations'; +import { showToast } from '../toast'; + +jest.mock('../api', () => ({ + getRecommendations: jest.fn().mockResolvedValue({ summary: {}, recommendations: [], regions: [] }), + refreshRecommendations: jest.fn(), + getConfig: jest.fn(), + updateConfig: jest.fn().mockResolvedValue({}), + listAccounts: jest.fn().mockResolvedValue([]), + listAccountServiceOverrides: jest.fn().mockResolvedValue([]), +})); + +jest.mock('../api/recommendations', () => ({ + getRecommendationsFreshness: jest.fn().mockResolvedValue({ + // Fresh (within the 24h budget) so the auto-refresh-on-open path does + // not fire and interfere with the explicit change-driven refresh. + last_collected_at: new Date().toISOString(), + last_collection_error: null, + }), + refreshRecommendations: jest.fn().mockResolvedValue({}), +})); + +jest.mock('../state', () => ({ + getCurrentProvider: jest.fn().mockReturnValue('all'), + setCurrentProvider: jest.fn(), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + setCurrentAccountIDs: jest.fn(), + getRecommendations: jest.fn().mockReturnValue([]), + getRecommendationByID: jest.fn().mockReturnValue(undefined), + setRecommendations: jest.fn(), + getSelectedRecommendationIDs: jest.fn().mockReturnValue(new Set()), + clearSelectedRecommendations: jest.fn(), + addSelectedRecommendation: jest.fn(), + removeSelectedRecommendation: jest.fn(), + getRecommendationsSort: jest.fn().mockReturnValue({ column: 'savings', direction: 'desc' }), + setRecommendationsSort: jest.fn(), + getRecommendationsColumnFilters: jest.fn().mockReturnValue({}), + setRecommendationsColumnFilter: jest.fn(), + clearAllRecommendationsColumnFilters: jest.fn(), + getVisibleRecommendations: jest.fn().mockReturnValue([]), + setVisibleRecommendations: jest.fn(), + getCostPeriod: jest.fn().mockReturnValue('monthly'), + setCostPeriod: jest.fn(), + getHiddenColumns: jest.fn().mockReturnValue(new Set()), + setHiddenColumns: jest.fn(), + getCurrentUser: jest.fn(), +})); + +jest.mock('../toast', () => ({ + showToast: jest.fn().mockReturnValue({ dismiss: jest.fn() }), +})); + +import * as state from '../state'; + +const mockUser = (role: string | null) => { + (state.getCurrentUser as jest.Mock).mockReturnValue( + role === null ? null : { id: 'u', email: 'u@example.com', role }, + ); +}; + +const setupDom = () => { + const recsTab = document.createElement('div'); + recsTab.id = 'opportunities-tab'; + recsTab.className = 'tab-content active'; + const summary = document.createElement('div'); + summary.id = 'recommendations-summary'; + const toolbar = document.createElement('div'); + toolbar.id = 'recommendations-toolbar'; + toolbar.className = 'recommendations-toolbar'; + const list = document.createElement('div'); + list.id = 'recommendations-list'; + recsTab.append(summary, toolbar, list); + document.body.replaceChildren(recsTab); +}; + +const getSelect = () => document.getElementById('recs-lookback-days') as HTMLSelectElement | null; + +// A realistic full GlobalConfig so we can assert the change handler does +// not wipe sibling fields when it round-trips the config on save. +const baseConfig = { + enabled_providers: ['aws', 'azure', 'gcp'], + default_term: 3, + default_payment: 'all-upfront', + default_coverage: 80, + auto_collect: true, + collection_schedule: 'daily', + notification_days_before: 7, + recommendations_cache_stale_hours: 24, + recommendations_lookback_days: 30, +}; + +beforeEach(() => { + jest.clearAllMocks(); + resetCachedGlobalConfig(); + resetAutoRefreshInFlight(); + setupDom(); + (api.getConfig as jest.Mock).mockResolvedValue({ global: baseConfig }); +}); + +describe('Opportunities lookback selector (issue #909)', () => { + test('prefills the selector from the current config value', async () => { + mockUser('admin'); + await loadRecommendations(); + const select = getSelect(); + expect(select).not.toBeNull(); + expect(select!.value).toBe('30'); + // All three accepted enum options present. + expect([...select!.options].map(o => o.value)).toEqual(['7', '30', '60']); + }); + + test('defaults to 7 when the config omits the lookback field', async () => { + mockUser('admin'); + (api.getConfig as jest.Mock).mockResolvedValue({ global: { enabled_providers: ['aws'] } }); + await loadRecommendations(); + expect(getSelect()!.value).toBe('7'); + }); + + test('admin sees an enabled control with the AWS-scope tooltip', async () => { + mockUser('admin'); + await loadRecommendations(); + const select = getSelect()!; + expect(select.disabled).toBe(false); + const tip = document.querySelector('#recommendations-toolbar .tooltip-text')!; + expect(tip.textContent).toContain('AWS recommendations only'); + expect(tip.textContent).toContain('Azure and GCP are unaffected'); + }); + + test.each(['user', 'readonly', null])('non-admin (%s) sees a disabled control with a permission tooltip', async (role) => { + mockUser(role); + await loadRecommendations(); + const select = getSelect()!; + expect(select.disabled).toBe(true); + const tip = document.querySelector('#recommendations-toolbar .tooltip-text')!; + expect(tip.textContent).toContain('admin'); + }); + + test('changing the value persists the full config (override only lookback) and refreshes', async () => { + mockUser('admin'); + await loadRecommendations(); + const select = getSelect()!; + + select.value = '60'; + select.dispatchEvent(new Event('change')); + // Let the async change handler settle (persist + refresh + reload). + await new Promise(r => setTimeout(r, 0)); + await new Promise(r => setTimeout(r, 0)); + + expect(api.updateConfig).toHaveBeenCalledTimes(1); + const sent = (api.updateConfig as jest.Mock).mock.calls[0][0]; + expect(sent.recommendations_lookback_days).toBe(60); + // Sibling fields preserved -- a partial PUT would have wiped these. + expect(sent.enabled_providers).toEqual(['aws', 'azure', 'gcp']); + expect(sent.default_term).toBe(3); + expect(sent.collection_schedule).toBe('daily'); + + // Re-collect triggered for the new window. + expect(refreshRecsAPI).toHaveBeenCalledTimes(1); + }); + + test('does nothing when the value is unchanged', async () => { + mockUser('admin'); + await loadRecommendations(); + const select = getSelect()!; + select.value = '30'; // same as baseConfig + select.dispatchEvent(new Event('change')); + await new Promise(r => setTimeout(r, 0)); + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(refreshRecsAPI).not.toHaveBeenCalled(); + }); + + test('persist failure toasts an error and reverts the selector', async () => { + mockUser('admin'); + (api.updateConfig as jest.Mock).mockRejectedValueOnce(new Error('boom')); + await loadRecommendations(); + const select = getSelect()!; + + select.value = '60'; + select.dispatchEvent(new Event('change')); + await new Promise(r => setTimeout(r, 0)); + await new Promise(r => setTimeout(r, 0)); + + expect(refreshRecsAPI).not.toHaveBeenCalled(); + // Reverted to the last-known value, re-enabled. + expect(select.value).toBe('30'); + expect(select.disabled).toBe(false); + const errToast = (showToast as jest.Mock).mock.calls.find( + (c) => c[0].kind === 'error' && /lookback/i.test(c[0].message), + ); + expect(errToast).toBeTruthy(); + }); +}); diff --git a/frontend/src/index.html b/frontend/src/index.html index 3e4f178a5..8fd5160a2 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -111,6 +111,12 @@

Upcoming Scheduled Purchases

+ +