diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 519642150..1fe372662 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -713,6 +713,134 @@ describe('Settings Module', () => { expect(ec2Call![2].coverage).toBe(80); }); + describe('global coverage propagation (issue #522)', () => { + const loadWithCoverage50 = async (): Promise => { + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 50 }, + services: [ + { provider: 'aws', service: 'ec2', enabled: true, term: 3, payment: 'all-upfront', coverage: 50 }, + { provider: 'aws', service: 'savings-plans-compute', enabled: true, term: 3, payment: 'all-upfront', coverage: 50 }, + ], + }); + (api.updateConfig as jest.Mock).mockResolvedValue({}); + (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); + await loadGlobalSettings(); + }; + const coverageSentFor = (service: string): number | undefined => { + const call = (api.updateServiceConfig as jest.Mock).mock.calls.find( + ([provider, svc]) => provider === 'aws' && svc === service, + ); + return call?.[2].coverage; + }; + + test('a changed global coverage is not reverted by the per-service PUTs', async () => { + await loadWithCoverage50(); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '0'; + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('ec2')).toBe(0); + expect(coverageSentFor('savings-plans-compute')).toBe(0); + }); + + test('an SP card coverage edited in the same save wins over the new default', async () => { + await loadWithCoverage50(); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '70'; + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '60'; + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('ec2')).toBe(70); + expect(coverageSentFor('savings-plans-compute')).toBe(60); + }); + + test('a second save without reload does not resend the pre-save coverage', async () => { + await loadWithCoverage50(); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '70'; + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + (api.updateServiceConfig as jest.Mock).mockClear(); + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('ec2')).toBe(70); + expect(coverageSentFor('savings-plans-compute')).toBe(70); + }); + + test('an SP card edit saved after a global change survives a further save', async () => { + await loadWithCoverage50(); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '0'; + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '70'; + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + (api.updateServiceConfig as jest.Mock).mockClear(); + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('ec2')).toBe(0); + expect(coverageSentFor('savings-plans-compute')).toBe(70); + }); + + test('a saved SP card edit does not shield the card from a later global change', async () => { + await loadWithCoverage50(); + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '70'; + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '20'; + (api.updateServiceConfig as jest.Mock).mockClear(); + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('ec2')).toBe(20); + expect(coverageSentFor('savings-plans-compute')).toBe(20); + }); + + test('after a partial failure the retry keeps the saved SP card coverage', async () => { + await loadWithCoverage50(); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '0'; + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '70'; + (api.updateServiceConfig as jest.Mock).mockImplementation(async (provider: string, svc: string) => { + if (provider === 'aws' && svc === 'ec2') throw new Error('ec2 failed'); + }); + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + expect(coverageSentFor('savings-plans-compute')).toBe(70); + + (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('ec2')).toBe(0); + expect(coverageSentFor('savings-plans-compute')).toBe(70); + }); + + test('after a partial failure a further global change still reaches the SP card', async () => { + await loadWithCoverage50(); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '0'; + (api.updateServiceConfig as jest.Mock).mockImplementation(async (provider: string, svc: string) => { + if (provider === 'aws' && svc === 'ec2') throw new Error('ec2 failed'); + }); + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '20'; + (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('ec2')).toBe(20); + expect(coverageSentFor('savings-plans-compute')).toBe(20); + }); + + test('an empty global coverage field is rejected instead of saving 0', async () => { + await loadWithCoverage50(); + (document.getElementById('setting-default-coverage') as HTMLInputElement).value = ''; + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + message: 'Target coverage must be a whole number between 0 and 100', + kind: 'error', + })); + }); + }); + test('saveGlobalSettings sends per-card enabled=false when SP enabled checkbox unchecked (issue #136)', async () => { (api.updateConfig as jest.Mock).mockResolvedValue({}); (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 6c48b5e5f..0552e8f7b 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3479,8 +3479,10 @@ export async function saveGlobalSettings(e: Event): Promise { saveInFlight = false; return; } - const rawCoverage = Number(byId('setting-default-coverage')?.value ?? '80'); - if (!Number.isFinite(rawCoverage) || !Number.isInteger(rawCoverage) || rawCoverage < 0 || rawCoverage > 100) { + const coverageText = byId('setting-default-coverage')?.value ?? '80'; + const rawCoverage = Number(coverageText); + // Number('') is 0, so an empty field must be rejected explicitly. + if (coverageText.trim() === '' || !Number.isFinite(rawCoverage) || !Number.isInteger(rawCoverage) || rawCoverage < 0 || rawCoverage > 100) { showToast({ message: 'Target coverage must be a whole number between 0 and 100', kind: 'error' }); if (saveBtn) saveBtn.disabled = false; saveInFlight = false; @@ -3524,7 +3526,18 @@ export async function saveGlobalSettings(e: Event): Promise { } try { + // The backend copies a changed default onto every service row, so a changed + // default must win over the stale per-service coverage (#522). Decide it from + // the dirty snapshot before the save refreshes it. + const defaultCoverageChanged = 'setting-default-coverage' in savedSnapshot + && getFieldValue('setting-default-coverage') !== savedSnapshot['setting-default-coverage']; await api.updateConfig(settings); + if (defaultCoverageChanged) { + // The default is persisted and propagated; mirror that locally so a retry + // after a partial failure does not send the old coverage back. + savedSnapshot['setting-default-coverage'] = getFieldValue('setting-default-coverage'); + loadedServiceConfigs = loadedServiceConfigs.map(s => ({ ...s, coverage: settings.default_coverage })); + } // Read + validate the per-service recommendation filters up front so a bad // min-count aborts the whole save with a targeted toast (rather than firing @@ -3557,12 +3570,17 @@ export async function saveGlobalSettings(e: Event): Promise { // controls. Read from the DOM when the card has the controls; fall back // to the base row value (or the global default) otherwise so RI and // Azure/GCP cards continue to inherit the global settings. - let coverage = base?.coverage ?? settings.default_coverage; + let coverage = defaultCoverageChanged + ? settings.default_coverage + : (base?.coverage ?? settings.default_coverage); let enabled = base?.enabled ?? true; if ('coverageId' in field && field.coverageId) { const rawCov = byId(field.coverageId)?.value ?? ''; const parsed = Number(rawCov); - if (rawCov !== '' && Number.isFinite(parsed)) coverage = parsed; + // Only a value the user edited on the card overrides the propagated + // default; an untouched card still shows the pre-save coverage. + const cardEdited = getFieldValue(field.coverageId) !== savedSnapshot[field.coverageId]; + if (cardEdited && rawCov !== '' && Number.isFinite(parsed)) coverage = parsed; } if ('enabledId' in field && field.enabledId) { const el = byId(field.enabledId); @@ -3590,7 +3608,17 @@ export async function saveGlobalSettings(e: Event): Promise { min_count: filters.min_count, } : {}), }; - return api.updateServiceConfig(provider, service, cfg); + return api.updateServiceConfig(provider, service, cfg).then(() => { + loadedServiceConfigs = [ + ...loadedServiceConfigs.filter(s => !(s.provider === provider && s.service === service)), + cfg, + ]; + if ('coverageId' in field && field.coverageId) { + const el = byId(field.coverageId); + if (el) el.value = String(coverage); + savedSnapshot[field.coverageId] = getFieldValue(field.coverageId); + } + }); }); await Promise.all(serviceSaves);