diff --git a/frontend/src/__tests__/settings-purchasing-grace.test.ts b/frontend/src/__tests__/settings-purchasing-grace.test.ts index 67b470241..9035e56dd 100644 --- a/frontend/src/__tests__/settings-purchasing-grace.test.ts +++ b/frontend/src/__tests__/settings-purchasing-grace.test.ts @@ -229,16 +229,19 @@ describe('Settings → Purchasing grace-period inputs', () => { expect(toastArg.message).not.toMatch(/AZURE/); }); - it('empty input defaults to 7 in the save payload', async () => { - (document.getElementById('setting-grace-aws') as HTMLInputElement).value = ''; + it('an emptied grace input refuses the save instead of saving 7', async () => { + const aws = document.getElementById('setting-grace-aws') as HTMLInputElement; + aws.value = ''; + aws.dispatchEvent(new Event('input')); (document.getElementById('setting-grace-azure') as HTMLInputElement).value = '0'; - (document.getElementById('setting-grace-gcp') as HTMLInputElement).value = '5'; await saveGlobalSettings(new Event("submit")); - expect(api.updateConfig).toHaveBeenCalledTimes(1); - const payload = (api.updateConfig as jest.Mock).mock.calls[0]![0]; - expect(payload.grace_period_days).toEqual({ aws: 7, azure: 0, gcp: 5 }); + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + kind: 'error', + message: expect.stringContaining('(AWS)'), + })); }); it('resetSettings restores every grace input to 7', async () => { @@ -313,7 +316,7 @@ describe('Settings → Purchasing grace inputs: inline range validation', () => expect(errorEl('setting-grace-azure')!.textContent).toBe('Must be a whole number between 0 and 30'); }); - it('clearing the field removes aria-invalid and hides the inline error', () => { + it('clearing the field shows the inline error (a grace period is required)', () => { const input = document.getElementById('setting-grace-aws') as HTMLInputElement; input.value = '99'; fire(input, 'input'); @@ -321,8 +324,8 @@ describe('Settings → Purchasing grace inputs: inline range validation', () => input.value = ''; fire(input, 'input'); - expect(input.getAttribute('aria-invalid')).toBeNull(); - expect(errorEl('setting-grace-aws')!.classList.contains('hidden')).toBe(true); + expect(input.getAttribute('aria-invalid')).toBe('true'); + expect(errorEl('setting-grace-aws')!.classList.contains('hidden')).toBe(false); }); it('typing a valid in-range value removes aria-invalid', () => { @@ -353,3 +356,175 @@ describe('Settings → Purchasing grace inputs: inline range validation', () => expect(toastArg.kind).toBe('error'); }); }); + +/** + * #539: grace_period_days and enabled_providers are replaced whole by the + * backend, so Save re-reads the server's value and overlays only the fields + * this tab changed. + */ +describe('Settings → stale-tab Save rebases multi-field keys onto the server (#539)', () => { + const baseGlobal = { + default_term: 3, default_payment: 'all-upfront', default_coverage: 80, notification_days_before: 3, + }; + const input = (id: string): HTMLInputElement => document.getElementById(id) as HTMLInputElement; + const sent = (): Record => (api.updateConfig as jest.Mock).mock.calls[0]![0]; + const submit = (): Promise => saveGlobalSettings(new Event('submit')); + + // Loads the tab's (soon stale) view, then makes the server return `server`. + const loadStaleTab = async (loaded: Record, server: Record): Promise => { + (api.getConfig as jest.Mock).mockResolvedValueOnce({ global: { ...baseGlobal, ...loaded }, services: [] }); + await loadGlobalSettings(); + (api.getConfig as jest.Mock).mockClear().mockResolvedValue({ global: { ...baseGlobal, ...server }, services: [] }); + }; + + beforeEach(() => { + jest.clearAllMocks(); + seedDOM(); + (api.updateConfig as jest.Mock).mockResolvedValue(undefined); + }); + + it('keeps another admin\'s grace value for a provider this tab did not change, and shows it', async () => { + await loadStaleTab( + { enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, + { enabled_providers: ['aws'], grace_period_days: { aws: 3, azure: 7, gcp: 7 } }, + ); + input('setting-grace-azure').value = '9'; + + await submit(); + + expect(sent()).toEqual({ grace_period_days: { aws: 3, azure: 9, gcp: 7 } }); + expect(input('setting-grace-aws').value).toBe('3'); + (api.updateConfig as jest.Mock).mockClear(); + await submit(); + expect(api.updateConfig).not.toHaveBeenCalled(); + }); + + it('keeps a typed-in grace edit made while the PUT is in flight', async () => { + await loadStaleTab( + { enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, + { enabled_providers: ['aws'], grace_period_days: { aws: 3, azure: 7, gcp: 7 } }, + ); + input('setting-grace-azure').value = '9'; + let resolvePut!: () => void; + (api.updateConfig as jest.Mock).mockImplementationOnce(() => new Promise(r => { resolvePut = r; })); + + const saving = submit(); + await new Promise(r => setTimeout(r, 0)); + input('setting-grace-aws').value = '12'; + resolvePut(); + await saving; + + expect(input('setting-grace-aws').value).toBe('12'); + }); + + it('keeps providers another admin added when this tab unchecks one', async () => { + await loadStaleTab( + { enabled_providers: ['aws', 'azure'] }, + { enabled_providers: ['aws', 'azure', 'gcp'] }, + ); + input('provider-azure').checked = false; + + await submit(); + + expect(sent()).toEqual({ enabled_providers: ['aws', 'gcp'] }); + }); + + it('does not duplicate a provider the server already enabled', async () => { + await loadStaleTab( + { enabled_providers: ['aws'] }, + { enabled_providers: ['aws', 'gcp'] }, + ); + input('provider-gcp').checked = true; + + await submit(); + + expect(sent()).toEqual({ enabled_providers: ['aws', 'gcp'] }); + }); + + it.each([ + ['rejects', () => (api.getConfig as jest.Mock).mockRejectedValue(new Error('boom'))], + ['returns no global', () => (api.getConfig as jest.Mock).mockResolvedValue({ services: [] })], + ])('aborts the whole save, sending nothing, when the server read %s', async (_name, breakRead) => { + await loadStaleTab({ enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, {}); + breakRead(); + input('setting-grace-azure').value = '9'; + + await submit(); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' })); + (api.updateConfig as jest.Mock).mockClear(); + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { ...baseGlobal, enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, services: [], + }); + await submit(); + expect(sent()).toEqual({ grace_period_days: { aws: 7, azure: 9, gcp: 7 } }); + }); + + it('does not read the server or send grace when no grace input changed', async () => { + await loadStaleTab({ enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, {}); + input('setting-notification-email').value = 'new@test.com'; + + await submit(); + + expect(api.getConfig).not.toHaveBeenCalled(); + expect(sent()).toEqual({ notification_email: 'new@test.com' }); + }); + + // Holds the server read open so the form can change between click and PUT. + const holdServerRead = (server: Record): (() => void) => { + let release!: () => void; + (api.getConfig as jest.Mock).mockReset().mockImplementation(() => new Promise(r => { + release = () => r({ global: { ...baseGlobal, ...server }, services: [] }); + })); + return () => release(); + }; + + it('judges grace changes by the values captured at Save, not the form after the read', async () => { + await loadStaleTab({ enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, {}); + input('setting-grace-azure').value = '9'; + const release = holdServerRead({ enabled_providers: ['aws'], grace_period_days: { aws: 3, azure: 7, gcp: 7 } }); + + const saving = submit(); + input('setting-grace-aws').value = '12'; + release(); + await saving; + + expect(sent()).toEqual({ grace_period_days: { aws: 3, azure: 9, gcp: 7 } }); + }); + + it('judges provider changes by the values captured at Save, not the form after the read', async () => { + await loadStaleTab({ enabled_providers: ['aws', 'azure'] }, {}); + input('provider-azure').checked = false; + const release = holdServerRead({ enabled_providers: ['aws', 'azure', 'gcp'] }); + + const saving = submit(); + input('provider-gcp').checked = true; + release(); + await saving; + + expect(sent()).toEqual({ enabled_providers: ['aws', 'gcp'] }); + }); + + it('removes every copy of an unchecked provider the server lists twice', async () => { + await loadStaleTab({ enabled_providers: ['aws', 'azure'] }, { enabled_providers: ['aws', 'aws', 'azure'] }); + input('provider-aws').checked = false; + + await submit(); + + expect(sent()).toEqual({ enabled_providers: ['azure'] }); + }); + + it('omits a provider the server has no grace value for when it is unchanged', async () => { + await loadStaleTab( + { enabled_providers: ['aws'], grace_period_days: { aws: 7, azure: 7, gcp: 7 } }, + { enabled_providers: ['aws'], grace_period_days: { azure: 7, gcp: 7 } }, + ); + input('setting-grace-gcp').value = '4'; + + await submit(); + + expect(sent()).toEqual({ grace_period_days: { azure: 7, gcp: 4 } }); + }); +}); diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 9be676def..85b1d0e3a 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -222,8 +222,8 @@ let saveInFlight = false; * byId('foo')?.value ?? ''; // read site */ // Grace-period input: 0 is a valid user choice (disables the feature -// for that provider), so we use empty-string to signal "fall back to -// default (7)". An explicit 0 must round-trip as 0, not get swallowed. +// for that provider), so an explicit 0 must round-trip as 0, not get +// swallowed. A missing server value renders as the default (7). function populateGraceInput(id: string, value: number | undefined): void { const el = document.getElementById(id) as HTMLInputElement | null; if (!el) return; @@ -231,8 +231,8 @@ function populateGraceInput(id: string, value: number | undefined): void { } // readGraceInput parses the grace-period input, returning the numeric -// value or an error message (out of range, not a number). Empty input -// defaults to 7. +// value or an error message (empty, out of range, not a number). Empty is +// refused, not defaulted: a cleared field most likely means 0. // // The error wording mirrors what wireInlineRangeValidation renders // under the field on input/blur, so the inline indicator and the @@ -243,9 +243,8 @@ function readGraceInput(id: string): { value: number; err?: undefined } | { valu const el = document.getElementById(id) as HTMLInputElement | null; if (!el) return { value: 7 }; const raw = el.value.trim(); - if (raw === '') return { value: 7 }; const n = Number(raw); - if (!Number.isFinite(n) || !Number.isInteger(n) || n < 0 || n > 30) { + if (raw === '' || !Number.isFinite(n) || !Number.isInteger(n) || n < 0 || n > 30) { return { value: 0, err: 'Must be a whole number between 0 and 30' }; } return { value: n }; @@ -2623,9 +2622,9 @@ export function setupSettingsHandlers(signal?: AbortSignal): void { wireInlineRangeValidation('setting-notification-days', { signal, requireInteger: true }); wireInlineRangeValidation('setting-recs-stale-hours', { signal, requireInteger: true, required: true }); wireInlineRangeValidation('setting-default-coverage', { signal, requireInteger: true, required: true }); - wireInlineRangeValidation('setting-grace-aws', { signal, requireInteger: true }); - wireInlineRangeValidation('setting-grace-azure', { signal, requireInteger: true }); - wireInlineRangeValidation('setting-grace-gcp', { signal, requireInteger: true }); + wireInlineRangeValidation('setting-grace-aws', { signal, requireInteger: true, required: true }); + wireInlineRangeValidation('setting-grace-azure', { signal, requireInteger: true, required: true }); + wireInlineRangeValidation('setting-grace-gcp', { signal, requireInteger: true, required: true }); // Per-SP-plan coverage percentage inputs — must carry the same inline // validation as the global default-coverage field so users see range errors // on the service cards without waiting for Save. Issue #1411 row 7.13. @@ -3467,6 +3466,52 @@ function showPropagatedCoverage(coverage: number, sentFields: Record, + isFieldChanged: (id: string) => boolean, +): Promise { + const fresh = await api.getConfig(); + if (!fresh?.global) throw new Error('could not load the current settings to merge into'); + if ('grace_period_days' in changedGlobal) { + const merged: Record = { ...(fresh.global.grace_period_days ?? {}) }; + for (const [provider, id] of GRACE_INPUTS) { + if (isFieldChanged(id)) merged[provider] = changedGlobal.grace_period_days![provider]!; + } + changedGlobal.grace_period_days = merged; + } + if ('enabled_providers' in changedGlobal) { + let merged: api.Provider[] = [...((fresh.global.enabled_providers ?? []) as api.Provider[])]; + for (const provider of ['aws', 'azure', 'gcp'] as const) { + if (!isFieldChanged(`provider-${provider}`)) continue; + const at = merged.indexOf(provider); + const checked = changedGlobal.enabled_providers!.includes(provider); + if (checked && at < 0) merged.push(provider); + if (!checked) merged = merged.filter(p => p !== provider); + } + changedGlobal.enabled_providers = merged; + } +} + +/** Show the persisted grace map, keeping any input the user edited while the save ran. */ +function showSavedGrace(saved: Record, sentFields: Record): void { + for (const [provider, id] of GRACE_INPUTS) { + if (getFieldValue(id) !== sentFields[id]) continue; + populateGraceInput(id, saved[provider]); + savedSnapshot[id] = sentFields[id] = getFieldValue(id); + } + revalidateInlineRanges(); +} + /** * Save global settings */ @@ -3504,7 +3549,7 @@ export async function saveGlobalSettings(e: Event): Promise { // reasons. const gracePeriodDays: Record = {}; const graceErrors: string[] = []; - for (const [provider, id] of [['aws', 'setting-grace-aws'], ['azure', 'setting-grace-azure'], ['gcp', 'setting-grace-gcp']] as const) { + for (const [provider, id] of GRACE_INPUTS) { const v = readGraceInput(id); if (v.err) { graceErrors.push(provider.toUpperCase()); @@ -3599,19 +3644,24 @@ export async function saveGlobalSettings(e: Event): Promise { // Save button are unreachable after a failed first load, so the snapshot is // always populated here. laddering_enabled is not part of Save: its toggle // saves itself (ladder.ts). Multi-field keys (grace map, enabled_providers) - // are sent whole, because the backend replaces them as a unit. + // are replaced whole by the backend, so rebaseMultiFieldKeys merges them first. const changedGlobal: Partial = { ...settings }; + const isFieldChanged = (id: string): boolean => sentFields[id] !== savedSnapshot[id]; for (const [key, ids] of GLOBAL_CONFIG_FIELDS) { - if (ids.every(id => getFieldValue(id) === savedSnapshot[id])) delete changedGlobal[key]; + if (!ids.some(isFieldChanged)) delete changedGlobal[key]; } const globalSent = Object.keys(changedGlobal).length > 0; if (globalSent) { + if ('grace_period_days' in changedGlobal || 'enabled_providers' in changedGlobal) { + await rebaseMultiFieldKeys(changedGlobal, isFieldChanged); + } await api.updateConfig(changedGlobal); // Persisted: advance the snapshot now, so a later validation failure or // service-save error does not leave these fields looking unsaved. for (const [key, ids] of GLOBAL_CONFIG_FIELDS) { if (key in changedGlobal) ids.forEach(id => { savedSnapshot[id] = sentFields[id]!; }); } + if (changedGlobal.grace_period_days) showSavedGrace(changedGlobal.grace_period_days, sentFields); } if (defaultCoverageChanged) { // The default is persisted and propagated; mirror that locally so a retry diff --git a/frontend/tests-e2e/settings-save-changed-fields.spec.ts b/frontend/tests-e2e/settings-save-changed-fields.spec.ts index b2d1c990d..79c65ff2f 100644 --- a/frontend/tests-e2e/settings-save-changed-fields.spec.ts +++ b/frontend/tests-e2e/settings-save-changed-fields.spec.ts @@ -41,3 +41,46 @@ test('Settings Save sends only the edited field and no service rows', async ({ p expect(new URL(puts[0]!.url).pathname).toBe('/api/config'); expect(puts[0]!.body).toEqual({ default_coverage: 60 }); }); + +// Issue #539: the grace map is replaced whole by the backend, so Save re-reads +// it and overlays only this tab's edit instead of writing a stale map back. +test('Settings Save keeps another admin\'s grace value and refuses an emptied grace field', 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[] = []; + let gets = 0; + await page.route('**/api/config', route => { + if (route.request().method() === 'PUT') { + puts.push(route.request().postDataJSON()); + return route.fulfill({ json: { status: 'updated' } }); + } + gets++; + // The first read is the page load; later reads see another admin's AWS change. + const aws = gets === 1 ? 7 : 3; + return route.fulfill({ json: { global: { + enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 70, + notification_days_before: 3, grace_period_days: { aws, azure: 7, gcp: 7 }, + }, services: [] } }); + }); + 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/purchasing'); + + await expect(page.locator('#setting-grace-aws')).toHaveValue('7'); + await page.locator('#setting-grace-azure').fill(''); + await page.locator('#save-purchasing-btn').click(); + await expect(page.locator('.toast, [role="status"]').filter({ hasText: 'Grace period must be' }).first()).toBeVisible(); + expect(puts).toHaveLength(0); + + await page.locator('#setting-grace-azure').fill('5'); + await page.locator('#save-purchasing-btn').click(); + await expect(page.locator('.toast, [role="status"]').filter({ hasText: 'Settings saved' }).first()).toBeVisible(); + expect(puts).toEqual([{ grace_period_days: { aws: 3, azure: 5, gcp: 7 } }]); + await expect(page.locator('#setting-grace-aws')).toHaveValue('3'); +});