Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 58 additions & 5 deletions frontend/src/__tests__/settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -2144,7 +2145,7 @@ describe('Settings Module', () => {
});

describe('global PUT against the real index.html (issue #539)', () => {
const loadRealPage = async (): Promise<void> => {
const loadRealPage = async (providers: string[] = ['aws']): Promise<void> => {
// 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
Expand All @@ -2156,7 +2157,7 @@ describe('Settings Module', () => {
.replace(/<script[\s\S]*?<\/script>/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',
Expand Down Expand Up @@ -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';
Expand Down
6 changes: 6 additions & 0 deletions frontend/src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3654,6 +3654,12 @@ export async function saveGlobalSettings(e: Event): Promise<void> {
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
Expand Down
37 changes: 37 additions & 0 deletions frontend/tests-e2e/settings-save-changed-fields.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Loading