From 4db63f1559e152e80510869fb3ec9c631ca52ea6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 27 Oct 2026 17:28:54 +0100 Subject: [PATCH] fix(settings): refuse a save that would leave no provider enabled An empty enabled_providers is contradictory in the backend: the recommendation filters read it as "all" while the scheduler collects nothing. After rebasing the tab's provider changes onto the server's current list, Save now aborts with an error toast before any config or service PUT when the merged list is empty. A save that does not touch the provider checkboxes is unaffected, so a stored empty list can still be left alone while editing other settings. Closes #1531 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/settings.test.ts | 63 +++++++++++++++++-- frontend/src/settings.ts | 6 ++ .../settings-save-changed-fields.spec.ts | 37 +++++++++++ 3 files changed, 101 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 36710802b..1b33a4636 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -655,10 +655,11 @@ describe('Settings Module', () => { window.alert = jest.fn(); const event = { preventDefault: jest.fn() } as unknown as Event; - await saveGlobalSettings(event); + await expect(saveGlobalSettings(event)).resolves.toBeUndefined(); - // Should still call updateConfig with default values - expect(api.updateConfig).toHaveBeenCalled(); + // No provider checkbox exists, so the empty list is refused (#1531) + // instead of being written. + expect(api.updateConfig).not.toHaveBeenCalled(); }); // Issue #22 follow-up: AWS Savings Plans split into four per-plan-type @@ -2144,7 +2145,7 @@ describe('Settings Module', () => { }); describe('global PUT against the real index.html (issue #539)', () => { - const loadRealPage = async (): Promise => { + const loadRealPage = async (providers: string[] = ['aws']): Promise => { // eslint-disable-next-line @typescript-eslint/no-require-imports const fs = require('fs') as typeof import('fs'); // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -2156,7 +2157,7 @@ describe('Settings Module', () => { .replace(//gi, ''); (api.getConfig as jest.Mock).mockResolvedValue({ global: { - enabled_providers: ['aws'], + enabled_providers: providers, notification_email: 'old@test.com', auto_collect: true, collection_schedule: 'daily', @@ -2217,6 +2218,58 @@ describe('Settings Module', () => { expect(globalBody()).toEqual({ enabled_providers: ['aws', 'gcp'] }); }); + describe('refuses a save that leaves no provider enabled (issue #1531)', () => { + const serverProviders = (providers: string[]): void => { + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { enabled_providers: providers, default_term: 3, default_payment: 'all-upfront', default_coverage: 80 }, + services: [], + }); + }; + const uncheckAws = (): void => { + (document.getElementById('provider-aws') as HTMLInputElement).checked = false; + }; + + test('unchecking the only enabled provider is refused with a toast and nothing sent', async () => { + await loadRealPage(['aws']); + mockShowToast.mockClear(); + uncheckAws(); + + await save(); + + expect(mockShowToast).toHaveBeenCalledWith({ message: 'Enable at least one provider before saving.', kind: 'error' }); + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + }); + + test('unchecking one of two providers saves the remaining one', async () => { + await loadRealPage(['aws', 'gcp']); + uncheckAws(); + + await save(); + + expect(globalBody()).toEqual({ enabled_providers: ['gcp'] }); + }); + + test('a stored empty list does not block editing an unrelated field', async () => { + await loadRealPage([]); + (document.getElementById('setting-notification-email') as HTMLInputElement).value = 'new@test.com'; + + await save(); + + expect(globalBody()).toEqual({ notification_email: 'new@test.com' }); + }); + + test('another admin enabling gcp makes the merged list non-empty, so the save is allowed', async () => { + await loadRealPage(['aws']); + serverProviders(['gcp']); + uncheckAws(); + + await save(); + + expect(globalBody()).toEqual({ enabled_providers: ['gcp'] }); + }); + }); + test('editing only the collection schedule saves it', async () => { await loadRealPage(); (document.getElementById('setting-collection-schedule') as HTMLSelectElement).value = 'weekly'; diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 85b1d0e3a..fc7c5d7e5 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3654,6 +3654,12 @@ export async function saveGlobalSettings(e: Event): Promise { if (globalSent) { if ('grace_period_days' in changedGlobal || 'enabled_providers' in changedGlobal) { await rebaseMultiFieldKeys(changedGlobal, isFieldChanged); + // The backend disagrees with itself on an empty list (filters read it as + // "all", the scheduler collects nothing), so never write one (#1531). + if (changedGlobal.enabled_providers?.length === 0) { + showToast({ message: 'Enable at least one provider before saving.', kind: 'error' }); + return; + } } await api.updateConfig(changedGlobal); // Persisted: advance the snapshot now, so a later validation failure or diff --git a/frontend/tests-e2e/settings-save-changed-fields.spec.ts b/frontend/tests-e2e/settings-save-changed-fields.spec.ts index 79c65ff2f..94b08d709 100644 --- a/frontend/tests-e2e/settings-save-changed-fields.spec.ts +++ b/frontend/tests-e2e/settings-save-changed-fields.spec.ts @@ -84,3 +84,40 @@ test('Settings Save keeps another admin\'s grace value and refuses an emptied gr expect(puts).toEqual([{ grace_period_days: { aws: 3, azure: 5, gcp: 7 } }]); await expect(page.locator('#setting-grace-aws')).toHaveValue('3'); }); + +// Issue #1531: an empty enabled_providers list is contradictory in the backend +// (filters read it as "all", the scheduler collects nothing), so Save refuses it. +test('Settings Save refuses to leave no provider enabled', async ({ page }) => { + await page.route('**/*', route => new URL(route.request().url()).hostname === '127.0.0.1' + ? route.fallback() : route.abort()); + await seedAuth(page); + await mockApi(page); + const puts: unknown[] = []; + await page.route('**/api/config', route => { + if (route.request().method() === 'PUT') { + puts.push(route.request().postDataJSON()); + return route.fulfill({ json: { status: 'updated' } }); + } + return route.fulfill({ json: { global: { + enabled_providers: ['aws'], notification_email: 'ops@example.com', default_term: 3, default_payment: 'all-upfront', default_coverage: 70, notification_days_before: 3, + }, services: [] } }); + }); + await page.route('**/api/config/service/**', route => { + puts.push(route.request().postDataJSON()); + return route.fulfill({ json: { status: 'updated' } }); + }); + await page.route('**/api/commitment-options', route => route.fulfill({ json: { status: 'ok', aws: {} } })); + await page.route('**/api/ri-exchange/config', route => route.fulfill({ json: { + mode: 'manual', auto_exchange_enabled: false, utilization_threshold: 80, + max_payment_per_exchange_usd: 0, max_payment_daily_usd: 0, lookback_days: 30, + } })); + await page.route('**/api/ladder/**', route => route.fulfill({ json: { configs: [], runs: [], events: [], total_count: 0, total_usd_hr: '0.000000' } })); + await page.goto('/admin/general'); + + await expect(page.locator('#provider-aws')).toBeChecked(); + await page.locator('#provider-aws').uncheck(); + await page.locator('#save-settings-btn').click(); + + await expect(page.locator('.toast, [role="status"]').filter({ hasText: 'Enable at least one provider' }).first()).toBeVisible(); + expect(puts).toHaveLength(0); +});