diff --git a/frontend/src/__tests__/settings-service-ids.test.ts b/frontend/src/__tests__/settings-service-ids.test.ts index 317492498..42a8dd36e 100644 --- a/frontend/src/__tests__/settings-service-ids.test.ts +++ b/frontend/src/__tests__/settings-service-ids.test.ts @@ -113,6 +113,9 @@ describe('Settings Azure/GCP service ids (issue #613)', () => { it('every Azure/GCP id the cards save and the override list offers is a backend service slug', async () => { await loadGlobalSettings(); + // Only edited rows are saved (#539): edit each Azure/GCP term. + document.querySelectorAll('select[id^="azure-"][id$="-term"], select[id^="gcp-"][id$="-term"]') + .forEach(sel => { sel.value = sel.value === '1' ? '3' : '1'; }); await saveGlobalSettings(new Event('submit')); const slugs = backendServiceSlugs(); const offered: string[] = []; @@ -148,6 +151,27 @@ describe('Settings Azure/GCP service ids (issue #613)', () => { expect(puts.find(c => c.provider === 'gcp' && c.service === 'cache')!.cfg['term']).toBe(1); }); + it('writes a re-keyed row in full once, then only its changed fields', async () => { + mockConfig([ + { provider: 'azure', service: 'vm', term: 1, payment: 'monthly', enabled: true, coverage: 80, ramp_schedule: 'r1' }, + ]); + await loadGlobalSettings(); + await saveGlobalSettings(new Event('submit')); + const first = servicePuts().filter(c => c.provider === 'azure' && c.service === 'compute'); + expect(first).toHaveLength(1); + expect(first[0]!.cfg).toEqual(expect.objectContaining({ ramp_schedule: 'r1', coverage: 80 })); + (api.updateServiceConfig as jest.Mock).mockClear(); + + await saveGlobalSettings(new Event('submit')); + expect(servicePuts()).toEqual([]); + + (document.getElementById('azure-vm-term') as HTMLSelectElement).value = '3'; + await saveGlobalSettings(new Event('submit')); + const second = servicePuts(); + expect(second).toHaveLength(1); + expect(Object.keys(second[0]!.cfg).sort()).toEqual(['provider', 'service', 'term']); + }); + it('prefers a row already stored under the current id over its legacy twin', async () => { mockConfig([ { provider: 'azure', service: 'vm', term: 1, payment: 'monthly', enabled: true, coverage: 80 }, diff --git a/frontend/src/__tests__/settings-service-payment.test.ts b/frontend/src/__tests__/settings-service-payment.test.ts index 607203ab9..ca8361e45 100644 --- a/frontend/src/__tests__/settings-service-payment.test.ts +++ b/frontend/src/__tests__/settings-service-payment.test.ts @@ -48,6 +48,12 @@ function mountRealIndexHtml(): void { document.body.innerHTML = new DOMParser().parseFromString(html, 'text/html').body.innerHTML; } +// Flip every per-service term card so each row counts as edited and is PUT. +function editEveryServiceTerm(): void { + document.querySelectorAll('select[id^="aws-"][id$="-term"], select[id^="azure-"][id$="-term"], select[id^="gcp-"][id$="-term"]') + .forEach(sel => { sel.value = sel.value === '1' ? '3' : '1'; }); +} + type ServiceCall = { provider: string; service: string; payment: string }; function serviceCalls(): ServiceCall[] { @@ -76,6 +82,7 @@ describe('Settings Save payment tokens (issue #545)', () => { async function save(): Promise { await loadGlobalSettings(); + editEveryServiceTerm(); await saveGlobalSettings(new Event('submit')); } @@ -93,6 +100,7 @@ describe('Settings Save payment tokens (issue #545)', () => { it('sends the Azure option the user picked, and GCP monthly whatever the global default is', async () => { await loadGlobalSettings(); + editEveryServiceTerm(); (document.getElementById('azure-vm-payment') as HTMLSelectElement).value = 'monthly'; (document.getElementById('setting-default-payment') as HTMLSelectElement).value = 'partial-upfront'; await saveGlobalSettings(new Event('submit')); @@ -121,8 +129,15 @@ describe('Settings Save payment tokens (issue #545)', () => { await expect(loadGlobalSettings()).resolves.not.toThrow(); expect(document.getElementById('gcp-storage-term')).toBeNull(); expect((document.getElementById('gcp-compute-term') as HTMLSelectElement).value).toBe('1'); + editEveryServiceTerm(); await saveGlobalSettings(new Event('submit')); - expect(serviceCalls().filter(c => c.provider === 'gcp').every(c => c.payment === 'monthly')).toBe(true); + // gcp/compute has a stored row, so only its term goes out; the new rows carry monthly. + const gcp = serviceCalls().filter(c => c.provider === 'gcp'); + expect(gcp.map(c => [c.service, c.payment])).toEqual([ + ['compute', undefined], + ['relational-db', 'monthly'], + ['cache', 'monthly'], + ]); expect(serviceCalls().some(c => c.service === 'storage')).toBe(false); }); @@ -160,7 +175,8 @@ describe('Settings Save payment tokens (issue #545)', () => { await saveGlobalSettings(new Event('submit')); - expect(api.updateServiceConfig).toHaveBeenCalledTimes(17); + // Only the two edited rows are PUT (#539). + expect(api.updateServiceConfig).toHaveBeenCalledTimes(2); expect(mockShowToast).toHaveBeenCalledTimes(1); expect(mockShowToast).toHaveBeenCalledWith({ message: 'Settings saved, but 1 service failed: azure/compute: invalid payment option', @@ -203,6 +219,7 @@ describe('Settings Save payment tokens (issue #545)', () => { throw provider === 'aws' && service === 'ec2' ? new Error('boom') : denied(); }); await loadGlobalSettings(); + editEveryServiceTerm(); await saveGlobalSettings(new Event('submit')); @@ -218,6 +235,7 @@ describe('Settings Save payment tokens (issue #545)', () => { await loadGlobalSettings(); const notify = document.getElementById('setting-notification-days') as HTMLInputElement; notify.value = '9'; + editEveryServiceTerm(); await saveGlobalSettings(new Event('submit')); @@ -244,6 +262,20 @@ describe('Settings Save payment tokens (issue #545)', () => { expect(serviceCalls().find(c => c.provider === 'azure' && c.service === 'compute')!.payment).toBe(expected); }); + it('a stored legacy payment the card normalised on load goes out with a partial PUT of its row', async () => { + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { enabled_providers: ['azure'], default_term: 3, default_payment: 'all-upfront', default_coverage: 80 }, + services: [{ provider: 'azure', service: 'compute', term: 3, payment: 'no-upfront', enabled: true, coverage: 80 }], + }); + await loadGlobalSettings(); + (document.getElementById('azure-vm-term') as HTMLSelectElement).value = '1'; + await saveGlobalSettings(new Event('submit')); + + expect((api.updateServiceConfig as jest.Mock).mock.calls).toEqual([ + ['azure', 'compute', { provider: 'azure', service: 'compute', term: 1, payment: 'monthly' }], + ]); + }); + it('never falls back to upfront when a payment select has no matching option at Save', async () => { await loadGlobalSettings(); (document.getElementById('azure-vm-payment') as HTMLSelectElement).value = 'junk'; diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 99f238f32..f4d060700 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -538,6 +538,23 @@ describe('Settings Module', () => { beforeEach(fillForm); + // Load a pristine config so the saved snapshot matches the form, as in the + // real page; an edit made afterwards is then the only dirty field. + const loadPristine = async (): Promise => { + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { + enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', + default_coverage: 80, notification_days_before: 3, + }, + services: [ + { provider: 'aws', service: 'savings-plans-compute', enabled: true, term: 3, payment: 'all-upfront', coverage: 80 }, + { provider: 'aws', service: 'savings-plans-sagemaker', enabled: true, term: 3, payment: 'all-upfront', coverage: 80 }, + { provider: 'aws', service: 'savings-plans-database', enabled: true, term: 3, payment: 'all-upfront', coverage: 80 }, + ], + }); + await loadGlobalSettings(); + }; + test('prevents default form submission', async () => { (api.updateConfig as jest.Mock).mockResolvedValue({}); @@ -700,6 +717,7 @@ describe('Settings Module', () => { }); test('saveGlobalSettings sends per-card coverage from SP coverage input (issue #136)', async () => { + await loadPristine(); (api.updateConfig as jest.Mock).mockResolvedValue({}); (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); window.alert = jest.fn(); @@ -716,12 +734,11 @@ describe('Settings Module', () => { expect(computeCall).toBeDefined(); expect(computeCall![2].coverage).toBe(60); - // Other SP cards keep their default value. + // Other SP cards were not edited, so they are not sent (#539). const ec2Call = (api.updateServiceConfig as jest.Mock).mock.calls.find( ([provider, service]) => provider === 'aws' && service === 'savings-plans-ec2instance', ); - expect(ec2Call).toBeDefined(); - expect(ec2Call![2].coverage).toBe(80); + expect(ec2Call).toBeUndefined(); }); describe('global PUT carries only changed fields (issue #539)', () => { @@ -737,6 +754,7 @@ describe('Settings Module', () => { }, services: [ { provider: 'aws', service: 'ec2', enabled: true, term: 1, payment: 'no-upfront', coverage: 50 }, + { provider: 'aws', service: 'savings-plans-compute', enabled: true, term: 1, payment: 'no-upfront', coverage: 50 }, ], }); (api.updateConfig as jest.Mock).mockClear().mockResolvedValue({}); @@ -764,15 +782,14 @@ describe('Settings Module', () => { expect(api.updateConfig).toHaveBeenCalledWith({ default_term: 3 }); }); - test('changing the coverage sends only the coverage and still propagates to the services', async () => { + test('changing the coverage sends only the coverage and no service PUTs', async () => { await loadStaleTab(); (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '70'; await save(); expect(api.updateConfig).toHaveBeenCalledWith({ default_coverage: 70 }); - const ec2 = (api.updateServiceConfig as jest.Mock).mock.calls.find(([p, svc]) => p === 'aws' && svc === 'ec2'); - expect(ec2![2].coverage).toBe(70); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); }); test('a second save does not resend a field the first save persisted', async () => { @@ -841,13 +858,88 @@ describe('Settings Module', () => { expect(api.updateConfig).toHaveBeenCalledWith({ notification_email: 'second@test.com' }); }); - test('an unchanged form sends no global PUT but still saves the services', async () => { + test('an unchanged form sends no global PUT and no service PUTs', async () => { + await loadStaleTab(); + + await save(); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + }); + + test('editing one service card PUTs only that row and only the edited field', async () => { + await loadStaleTab(); + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '65'; + + await save(); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(api.updateServiceConfig).toHaveBeenCalledTimes(1); + expect(api.updateServiceConfig).toHaveBeenCalledWith('aws', 'savings-plans-compute', expect.objectContaining({ coverage: 65 })); + const body = (api.updateServiceConfig as jest.Mock).mock.calls[0][2]; + expect(Object.keys(body).sort()).toEqual(['coverage', 'provider', 'service']); + }); + + test('editing only a recommendation filter sends only that filter', async () => { + await loadStaleTab(); + const regions = document.createElement('input'); + regions.id = 'aws-ec2-include-regions'; + regions.value = 'us-east-1, eu-west-1'; + document.getElementById('global-settings-form')!.appendChild(regions); + + await save(); + + expect(api.updateServiceConfig).toHaveBeenCalledTimes(1); + expect(api.updateServiceConfig).toHaveBeenCalledWith('aws', 'ec2', { + provider: 'aws', service: 'ec2', include_regions: ['us-east-1', 'eu-west-1'], + }); + }); + + test('a filter saved by one Save is not resent by the next', async () => { + await loadStaleTab(); + const regions = document.createElement('input'); + regions.id = 'aws-ec2-include-regions'; + regions.value = 'us-east-1'; + document.getElementById('global-settings-form')!.appendChild(regions); + await save(); + expect(api.updateServiceConfig).toHaveBeenCalledTimes(1); + (api.updateServiceConfig as jest.Mock).mockClear(); + + await save(); + + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + }); + + test('an edited SP coverage cleared to empty blocks the save and stays dirty', async () => { await loadStaleTab(); + const card = document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement; + card.value = ''; + mockShowToast.mockClear(); await save(); expect(api.updateConfig).not.toHaveBeenCalled(); - expect(api.updateServiceConfig).toHaveBeenCalled(); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining('coverage must be a whole number between 0 and 100 (savings-plans-compute)'), + kind: 'error', + })); + expect(isUnsavedChanges()).toBe(true); + + card.value = '65'; + await save(); + expect(api.updateServiceConfig).toHaveBeenCalledWith('aws', 'savings-plans-compute', expect.objectContaining({ coverage: 65 })); + }); + + test('a second save after a service PUT succeeded resends nothing', async () => { + await loadStaleTab(); + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '65'; + await save(); + (api.updateServiceConfig as jest.Mock).mockClear(); + + await save(); + + expect(api.updateServiceConfig).not.toHaveBeenCalled(); }); }); @@ -871,51 +963,57 @@ describe('Settings Module', () => { return call?.[2].coverage; }; - test('a changed global coverage is not reverted by the per-service PUTs', async () => { + test('a changed global coverage sends no service PUTs and shows the propagated value on the SP cards', 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); + expect(api.updateConfig).toHaveBeenCalledWith({ default_coverage: 0 }); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + expect((document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value).toBe('0'); + expect(isUnsavedChanges()).toBe(false); }); - test('an SP card coverage edited in the same save wins over the new default', async () => { + test('an SP card coverage edited in the same save wins over the new default and is the only service PUT', 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('ec2')).toBeUndefined(); expect(coverageSentFor('savings-plans-compute')).toBe(60); + expect(api.updateServiceConfig).toHaveBeenCalledTimes(1); + expect((document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement).value).toBe('70'); }); - test('a second save without reload does not resend the pre-save coverage', async () => { + test('a second save without reload resends nothing', async () => { await loadWithCoverage50(); (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '70'; await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + (api.updateConfig as jest.Mock).mockClear(); (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); + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); }); - test('an SP card edit saved after a global change survives a further save', async () => { + test('an SP card edit saved after a global change is not resent by 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); + expect(coverageSentFor('savings-plans-compute')).toBe(70); (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); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + expect((document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value).toBe('70'); }); test('a saved SP card edit does not shield the card from a later global change', async () => { @@ -927,16 +1025,17 @@ describe('Settings Module', () => { await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); - expect(coverageSentFor('ec2')).toBe(20); - expect(coverageSentFor('savings-plans-compute')).toBe(20); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + expect((document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value).toBe('20'); }); - test('after a partial failure the retry keeps the saved SP card coverage', async () => { + test('after a partial failure the retry resends only the failed 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'; + (document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement).value = '30'; (api.updateServiceConfig as jest.Mock).mockImplementation(async (provider: string, svc: string) => { - if (provider === 'aws' && svc === 'ec2') throw new Error('ec2 failed'); + if (provider === 'aws' && svc === 'savings-plans-ec2instance') throw new Error('sp failed'); }); await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); @@ -945,15 +1044,15 @@ describe('Settings Module', () => { (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); + expect(coverageSentFor('savings-plans-compute')).toBeUndefined(); + expect(coverageSentFor('savings-plans-ec2instance')).toBe(30); }); - test('after a partial failure a further global change still reaches the SP card', async () => { + test('after a failed service PUT a further global change still reaches an unedited SP card', async () => { await loadWithCoverage50(); - (document.getElementById('setting-default-coverage') as HTMLInputElement).value = '0'; + (document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement).value = '30'; (api.updateServiceConfig as jest.Mock).mockImplementation(async (provider: string, svc: string) => { - if (provider === 'aws' && svc === 'ec2') throw new Error('ec2 failed'); + if (provider === 'aws' && svc === 'savings-plans-ec2instance') throw new Error('sp failed'); }); await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); @@ -961,8 +1060,8 @@ describe('Settings Module', () => { (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); + expect(coverageSentFor('savings-plans-ec2instance')).toBe(30); + expect((document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value).toBe('20'); }); test('a rejected global PUT leaves the coverage dirty and sends no service PUTs', async () => { @@ -1070,6 +1169,7 @@ describe('Settings Module', () => { }); test('saveGlobalSettings sends per-card enabled=false when SP enabled checkbox unchecked (issue #136)', async () => { + await loadPristine(); (api.updateConfig as jest.Mock).mockResolvedValue({}); (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); window.alert = jest.fn(); @@ -1086,12 +1186,11 @@ describe('Settings Module', () => { expect(dbCall).toBeDefined(); expect(dbCall![2].enabled).toBe(false); - // Other SP cards remain enabled. + // Untouched SP cards are not sent at all (#539). const computeCall = (api.updateServiceConfig as jest.Mock).mock.calls.find( ([provider, service]) => provider === 'aws' && service === 'savings-plans-compute', ); - expect(computeCall).toBeDefined(); - expect(computeCall![2].enabled).toBe(true); + expect(computeCall).toBeUndefined(); }); test('loadGlobalSettings populates SP coverage and enabled from service config (issue #136)', async () => { @@ -1124,21 +1223,19 @@ describe('Settings Module', () => { expect(ec2Enabled.checked).toBe(true); }); - test('calls updateServiceConfig once per service field (17 calls)', async () => { + test('an unedited form PUTs no service rows, an edit PUTs only its row', async () => { + await loadPristine(); (api.updateConfig as jest.Mock).mockResolvedValue({}); - (api.updateServiceConfig as jest.Mock).mockResolvedValue(undefined); + (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); window.alert = jest.fn(); const event = { preventDefault: jest.fn() } as unknown as Event; await saveGlobalSettings(event); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); - // 5 AWS RIs (ec2, rds, elasticache, opensearch, redshift) + 4 AWS - // SP (compute, ec2instance, sagemaker, database) + 5 Azure (vm, - // sql, cosmosdb, redis, search) + 3 GCP (compute, sql, memorystore). - // Pre-rev-2 was 16 (1 umbrella SP + 1 sagemaker, plus 5 - // RIs); the SP split replaced those two with four per-plan-type - // entries, net +2. - expect(api.updateServiceConfig).toHaveBeenCalledTimes(17); + (document.getElementById('aws-savings-plans-sagemaker-term') as HTMLSelectElement).value = '1'; + await saveGlobalSettings(event); + expect(api.updateServiceConfig).toHaveBeenCalledTimes(1); }); // Issue #301: configurable recommendations cache-staleness threshold + lookback @@ -1950,9 +2047,6 @@ describe('Settings Module', () => { credentials: {}, }); - const serviceCall = (service: string) => - (api.updateServiceConfig as jest.Mock).mock.calls.find(([p, s]) => p === 'aws' && s === service); - const saveUnchanged = async () => { (api.updateConfig as jest.Mock).mockResolvedValue({}); (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); @@ -1969,8 +2063,7 @@ describe('Settings Module', () => { await saveUnchanged(); expect(api.updateConfig).not.toHaveBeenCalled(); - expect(serviceCall('savings-plans-compute')![2].coverage).toBe(0); - expect(serviceCall('ec2')![2].coverage).toBe(0); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); }); test('an absent default_coverage renders the 80 default and an unedited Save sends no global PUT', async () => { @@ -1996,8 +2089,7 @@ describe('Settings Module', () => { await saveUnchanged(); - expect(serviceCall('savings-plans-compute')![2].coverage).toBe(0); - expect(serviceCall('savings-plans-ec2instance')![2].coverage).toBe(50); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); }); test('the override modal inherit label shows the stored 0%', async () => { diff --git a/frontend/src/api/settings.ts b/frontend/src/api/settings.ts index 2eebdfd7f..50fa4dc28 100644 --- a/frontend/src/api/settings.ts +++ b/frontend/src/api/settings.ts @@ -16,7 +16,7 @@ export async function getConfig(): Promise { /** * Update per-service configuration */ -export async function updateServiceConfig(provider: string, service: string, cfg: ServiceConfig): Promise { +export async function updateServiceConfig(provider: string, service: string, cfg: Partial): Promise { return apiRequest(`/config/service/${provider}/${service}`, { method: 'PUT', body: JSON.stringify(cfg) diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 5ced7a3ac..e9625d25a 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -198,6 +198,10 @@ const GLOBAL_CONFIG_FIELDS: ReadonlyArray(); + // saveInFlight guards saveGlobalSettings against concurrent invocations // (rapid Save clicks, Enter-in-form, etc.). Toggled in a try/finally and // mirrored on the Save button's disabled attribute so the UI reflects the @@ -3175,8 +3179,13 @@ export async function loadGlobalSettings(): Promise { // is the only one the scheduler and purchase flow match. The card DOM ids // still use the old names, so look them up through SERVICE_FIELDS rather // than deriving them from the row's service id. - const services = canonicalizeServiceConfigs(data.services ?? []); + const stored = data.services ?? []; + const services = canonicalizeServiceConfigs(stored); loadedServiceConfigs = services; + // Re-keyed rows have no backend row under the current id yet, so the next + // Save writes them in full instead of as a partial change (#539). + const storedKeys = new Set(stored.map(r => `${r.provider}/${r.service}`)); + legacyRekeyedServices = new Set(services.filter(r => !storedKeys.has(`${r.provider}/${r.service}`)).map(r => `${r.provider}/${r.service}`)); for (const svc of services) { const field = SERVICE_FIELDS.find(f => f.provider === svc.provider && f.service === svc.service); if (!field) continue; @@ -3350,24 +3359,111 @@ async function renderAccountsOverview(): Promise { /** * Names of the per-Savings-Plan coverage cards whose edited value is not a - * whole number between 0 and 100. Untouched cards and empty inputs are not - * checked: the save keeps their persisted value, as it always has. + * whole number between 0 and 100. Untouched cards are not checked; an edited + * card cleared to empty is invalid, not silently dropped. */ function invalidEditedCardCoverages(): string[] { const bad: string[] = []; for (const field of SERVICE_FIELDS) { if (!('coverageId' in field) || !field.coverageId) continue; const input = byId(field.coverageId); - const raw = input?.value ?? ''; - if (raw.trim() === '' || getFieldValue(field.coverageId) === savedSnapshot[field.coverageId]) continue; + if (!input) continue; + const raw = input.value; + if (getFieldValue(field.coverageId) === savedSnapshot[field.coverageId]) continue; + // Number('') is 0, so an edited-to-empty input must be rejected explicitly. const parsed = Number(raw); - if (Number.isInteger(parsed) && parsed >= 0 && parsed <= 100) continue; + if (raw.trim() !== '' && Number.isInteger(parsed) && parsed >= 0 && parsed <= 100) continue; const title = input?.closest('.service-default-card')?.querySelector('h5')?.textContent?.trim(); bad.push(title || field.service); } return bad; } +const SERVICE_FILTER_KEYS = [ + 'include_engines', 'exclude_engines', 'include_types', 'exclude_types', + 'include_regions', 'exclude_regions', +] as const; + +function isDirtyField(id: string): boolean { + return id in savedSnapshot && getFieldValue(id) !== savedSnapshot[id]; +} + +/** + * The fields of one service row the user changed since load/save, or null when + * nothing changed (no PUT). A card's term, payment, coverage and enabled + * controls count as changed when they differ from the saved snapshot; the + * recommendation filters are compared with the loaded row. + */ +function changedServiceFields( + field: (typeof SERVICE_FIELDS)[number], + base: api.ServiceConfig | undefined, + filters: Exclude, { message: string }> | undefined, +): Partial | null { + const changes: Partial = {}; + if (isDirtyField(field.termId)) { + changes.term = parseInt(byId(field.termId)?.value || '3', 10); + } + if (field.paymentId && isDirtyField(field.paymentId)) { + changes.payment = byId(field.paymentId)?.value || normalizePaymentValue('no-upfront', field.provider); + } + if ('coverageId' in field && field.coverageId && isDirtyField(field.coverageId)) { + const rawCov = byId(field.coverageId)?.value ?? ''; + if (rawCov !== '' && Number.isFinite(Number(rawCov))) changes.coverage = Number(rawCov); + } + if ('enabledId' in field && field.enabledId && isDirtyField(field.enabledId)) { + changes.enabled = byId(field.enabledId)?.checked ?? true; + } + if (filters) { + for (const key of SERVICE_FILTER_KEYS) { + if (filters[key].join(',') !== (base?.[key] ?? []).join(',')) changes[key] = filters[key]; + } + if (filters.min_count !== (base?.min_count ?? 0)) changes.min_count = filters.min_count; + } + if (Object.keys(changes).length === 0) return null; + if (!changes.payment && field.paymentId && base) { + // A stored payment the card normalises on load (#545) goes out with any + // PUT of the row, or the backend validates the legacy token and rejects it. + const shown = byId(field.paymentId)?.value; + if (shown && shown !== base.payment) changes.payment = shown; + } + return changes; +} + +/** + * Required fields for a service row the backend has no record of yet, where a + * partial body would fail validation. Mirrors what a first Save always sent. + */ +function newServiceConfigDefaults( + field: (typeof SERVICE_FIELDS)[number], + settings: api.Config, +): api.ServiceConfig { + const paymentEl = field.paymentId ? byId(field.paymentId) : null; + return { + provider: field.provider, + service: field.service, + enabled: true, + term: parseInt(byId(field.termId)?.value || '3', 10), + payment: paymentEl?.value || normalizePaymentValue(field.paymentId ? 'no-upfront' : settings.default_payment, field.provider), + coverage: settings.default_coverage, + }; +} + +/** + * The backend copied a changed default coverage onto every service row, so show + * it on the Savings Plan cards the user did not edit in this Save (an edited + * card keeps its value and is sent as its own change). + */ +function showPropagatedCoverage(coverage: number, sentFields: Record): void { + for (const field of SERVICE_FIELDS) { + if (!('coverageId' in field) || !field.coverageId) continue; + const el = byId(field.coverageId); + if (!el || getFieldValue(field.coverageId) !== savedSnapshot[field.coverageId]) continue; + el.value = String(coverage); + savedSnapshot[field.coverageId] = String(coverage); + sentFields[field.coverageId] = String(coverage); + } +} + /** * Save global settings */ @@ -3516,6 +3612,7 @@ export async function saveGlobalSettings(e: Event): Promise { // after a partial failure does not send the old coverage back. savedSnapshot['setting-default-coverage'] = sentFields['setting-default-coverage']!; loadedServiceConfigs = loadedServiceConfigs.map(s => ({ ...s, coverage: settings.default_coverage })); + showPropagatedCoverage(settings.default_coverage, sentFields); } // Read + validate the per-service recommendation filters up front so a bad @@ -3535,72 +3632,31 @@ export async function saveGlobalSettings(e: Event): Promise { } const serviceSaves = SERVICE_FIELDS.map((field) => { - const { provider, service, termId, paymentId } = field; - const term = parseInt(byId(termId)?.value || '3', 10); - const payment = paymentId - ? (byId(paymentId)?.value || normalizePaymentValue('no-upfront', provider)) - : normalizePaymentValue(settings.default_payment, provider); + const { provider, service } = field; const base = loadedServiceConfigs.find(s => s.provider === provider && s.service === service); - // Carry forward every field the UI doesn't own (e.g. ramp_schedule). - // The recommendation-filter fields are now UI-owned and overwritten from - // the card's controls below. - - // Issue #136: per-product SP cards expose their own coverage and enabled - // 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 enabled = base?.enabled ?? true; - if ('coverageId' in field && field.coverageId) { - const rawCov = byId(field.coverageId)?.value ?? ''; - const parsed = Number(rawCov); - // 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); - if (el) enabled = el.checked; - } - const filters = filterValuesByService.get(`${provider}-${service}`); - const cfg: api.ServiceConfig = { - ...(base ?? {}), - provider, - service, - enabled, - term, - payment, - coverage, - // Filters: send the card's current values. isServiceFilterError was - // already ruled out above, so `filters` here is always the value form. - ...(filters && !isServiceFilterError(filters) ? { - include_engines: filters.include_engines, - exclude_engines: filters.exclude_engines, - include_types: filters.include_types, - exclude_types: filters.exclude_types, - include_regions: filters.include_regions, - exclude_regions: filters.exclude_regions, - min_count: filters.min_count, - } : {}), - }; + // Only the rows (and the fields of a row) the user changed go out, so a + // stale tab cannot revert a service another admin edited (#539). + const changes = changedServiceFields(field, base, filters && !isServiceFilterError(filters) ? filters : undefined); + const rekeyed = base !== undefined && legacyRekeyedServices.has(`${provider}/${service}`); + if (!changes && !rekeyed) return null; + const fresh = newServiceConfigDefaults(field, settings); + let cfg: Partial; + if (!base) cfg = { ...fresh, ...changes }; + else if (rekeyed) cfg = { ...base, term: fresh.term, payment: fresh.payment, ...changes }; + else cfg = { provider, service, ...changes }; return api.updateServiceConfig(provider, service, cfg).then(() => { + legacyRekeyedServices.delete(`${provider}/${service}`); loadedServiceConfigs = [ ...loadedServiceConfigs.filter(s => !(s.provider === provider && s.service === service)), - cfg, + { ...(base ?? {}), ...cfg } as api.ServiceConfig, ]; - if ('coverageId' in field && field.coverageId) { - const el = byId(field.coverageId); - // Show the persisted (possibly propagated) value only when the card - // still holds what this Save read; an edit typed during the PUT stays - // dirty against the persisted value and goes out with the next Save. - if (el && getFieldValue(field.coverageId) === sentFields[field.coverageId]) el.value = String(coverage); - savedSnapshot[field.coverageId] = String(coverage); - } }); }); - const results = await Promise.allSettled(serviceSaves); + // Only rows with a PUT take part in the outcome; an unchanged row is neither + // a success nor a failure. + const savedFields = SERVICE_FIELDS.filter((_, i) => serviceSaves[i] !== null); + const results = await Promise.allSettled(serviceSaves.filter((p): p is Promise => p !== null)); // The global PUT and every fulfilled service PUT are persisted, so the // snapshot moves forward for them. A rejected service keeps its previous @@ -3612,7 +3668,7 @@ export async function saveGlobalSettings(e: Event): Promise { const failures: string[] = []; results.forEach((result, i) => { if (result.status === 'fulfilled') return; - const field = SERVICE_FIELDS[i]!; + const field = savedFields[i]!; failures.push(`${field.provider}/${field.service}: ${(result.reason as Error).message}`); const ids = [field.termId, field.paymentId, 'coverageId' in field ? field.coverageId : null, 'enabledId' in field ? field.enabledId : null]; for (const id of ids) { diff --git a/frontend/tests-e2e/settings-save-changed-fields.spec.ts b/frontend/tests-e2e/settings-save-changed-fields.spec.ts new file mode 100644 index 000000000..b2d1c990d --- /dev/null +++ b/frontend/tests-e2e/settings-save-changed-fields.spec.ts @@ -0,0 +1,43 @@ +import { test, expect } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; + +// Issue #539: Save carries only what the user changed, so a tab opened before +// another admin's save cannot write its stale defaults or service rows back. +test('Settings Save sends only the edited field and no service rows', 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: Array<{ url: string; body: unknown }> = []; + await page.route('**/api/config', route => { + if (route.request().method() === 'PUT') { + puts.push({ url: route.request().url(), body: route.request().postDataJSON() }); + return route.fulfill({ json: { status: 'updated' } }); + } + return route.fulfill({ json: { global: { + enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 70, notification_days_before: 3, + }, services: [ + { provider: 'aws', service: 'ec2', enabled: true, term: 3, payment: 'all-upfront', coverage: 70 }, + ] } }); + }); + await page.route('**/api/config/service/**', route => { + puts.push({ url: route.request().url(), body: 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/purchasing'); + + await expect(page.locator('#setting-default-coverage')).toHaveValue('70'); + await page.locator('#setting-default-coverage').fill('60'); + await page.locator('#save-purchasing-btn').click(); + + await expect(page.locator('.toast, [role="status"]').filter({ hasText: 'Settings saved' }).first()).toBeVisible(); + expect(puts).toHaveLength(1); + expect(new URL(puts[0]!.url).pathname).toBe('/api/config'); + expect(puts[0]!.body).toEqual({ default_coverage: 60 }); +}); diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 18a57957a..182e2425a 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -188,13 +188,15 @@ func (h *Handler) checkCommitmentOptionCombo(ctx context.Context, cfg config.Ser return nil } -// serviceConfigFilterKeys are the recommendation-filter JSON fields the -// Settings UI now edits. They are overlaid onto the existing row only when the +// serviceConfigOverlayKeys are the JSON fields the Settings UI edits: the four +// scalars (enabled, term, payment, coverage) and the recommendation filters. +// They are overlaid onto the existing row only when the // request body actually contains the key, so a partial PUT from a non-UI // client (or an older UI build) never zeroes a filter the operator set // elsewhere. RampSchedule is intentionally absent: it is still set out-of-band // and the UI does not own it. -var serviceConfigFilterKeys = []string{ +var serviceConfigOverlayKeys = []string{ + "enabled", "term", "payment", "coverage", "include_engines", "exclude_engines", "include_regions", "exclude_regions", "include_types", "exclude_types", @@ -202,13 +204,12 @@ var serviceConfigFilterKeys = []string{ } // mergeServiceConfig loads any existing service config and overlays the -// UI-editable fields from cfg onto it. The four always-present scalar fields -// (Enabled, Term, Payment, Coverage) are overlaid unconditionally; the -// recommendation-filter fields (include/exclude engines/regions/types, -// min_count) are overlaid only when the request body actually carried the -// corresponding key (per presentKeys), so a partial PUT cannot silently wipe a -// filter set out-of-band. body is the raw request JSON used to detect which -// filter keys are present. +// UI-editable fields from cfg onto it. Every field (Enabled, Term, Payment, +// Coverage and the include/exclude engines/regions/types and min_count +// filters) is overlaid only when the request body actually carried the +// corresponding key (per presentKeys), so a partial PUT cannot silently +// revert or wipe a field it did not mean to touch. body is the raw request +// JSON used to detect which keys are present. // // A "not found" error means no existing record — cfg is returned unchanged. // Any other DB error is returned to prevent a partial write from clobbering @@ -225,15 +226,11 @@ func mergeServiceConfig(ctx context.Context, store config.StoreInterface, cfg co return cfg, nil } - existing.Enabled = cfg.Enabled - existing.Term = cfg.Term - existing.Payment = cfg.Payment - existing.Coverage = cfg.Coverage - - present, perr := presentKeys(body, serviceConfigFilterKeys) + present, perr := presentKeys(body, serviceConfigOverlayKeys) if perr != nil { return cfg, perr } + overlayPresentScalarFields(existing, &cfg, present) overlayPresentFilterFields(existing, &cfg, present) return *existing, nil } @@ -255,6 +252,25 @@ func presentKeys(body string, keys []string) (map[string]bool, error) { return present, nil } +// overlayPresentScalarFields copies enabled, term, payment and coverage from src +// onto dst only when the body carried the key, so a client that sends just the +// field the user edited (the Settings page, #539) cannot revert the others. A +// present zero value (enabled:false, coverage:0) still applies. +func overlayPresentScalarFields(dst, src *config.ServiceConfig, present map[string]bool) { + if present["enabled"] { + dst.Enabled = src.Enabled + } + if present["term"] { + dst.Term = src.Term + } + if present["payment"] { + dst.Payment = src.Payment + } + if present["coverage"] { + dst.Coverage = src.Coverage + } +} + // overlayPresentFilterFields copies each filter field from src onto dst only // when its JSON key was present in the request body. Split out of // mergeServiceConfig to keep that function under the cyclomatic-complexity @@ -344,10 +360,10 @@ func (h *Handler) updateServiceConfig(ctx context.Context, req *events.LambdaFun cfg.Service = parts[1] } - // Merge: overlay the scalar UI fields unconditionally and the - // recommendation-filter fields only when the body carried them, so a - // partial PUT never zeroes a filter (ramp_schedule, or a filter set - // out-of-band) the request didn't mean to touch. + // Merge: overlay each field (the scalars and the recommendation filters) + // only when the body carried its key, so a partial PUT never reverts or + // zeroes a field (ramp_schedule, a filter set out-of-band) it didn't mean + // to touch. Validation below runs on the merged row. merged, mergeErr := mergeServiceConfig(ctx, h.config, cfg, req.Body) if mergeErr != nil { return nil, mergeErr diff --git a/internal/api/handler_config_propagation_integration_test.go b/internal/api/handler_config_propagation_integration_test.go index d5c034fec..ba9bfaf04 100644 --- a/internal/api/handler_config_propagation_integration_test.go +++ b/internal/api/handler_config_propagation_integration_test.go @@ -74,6 +74,19 @@ func TestUpdateConfigPropagatesOnlyChangedDefaults(t *testing.T) { require.Equal(t, ec2, service("ec2")) require.Equal(t, rds, service("rds")) + // A partial PUT of an unrelated key (#539) leaves the stored defaults and + // every service row untouched. + defaultsBefore, err := store.GetGlobalConfig(ctx) + require.NoError(t, err) + require.NoError(t, put(`{"notification_email": "ops@example.com"}`)) + defaultsAfter, err := store.GetGlobalConfig(ctx) + require.NoError(t, err) + require.Equal(t, []any{defaultsBefore.DefaultTerm, defaultsBefore.DefaultPayment, defaultsBefore.DefaultCoverage}, + []any{defaultsAfter.DefaultTerm, defaultsAfter.DefaultPayment, defaultsAfter.DefaultCoverage}) + require.Equal(t, "ops@example.com", *defaultsAfter.NotificationEmail) + require.Equal(t, ec2, service("ec2")) + require.Equal(t, rds, service("rds")) + _, err = pg.DB.Pool().Exec(ctx, ` CREATE FUNCTION reject_service_config_update() RETURNS trigger AS $$ BEGIN RAISE EXCEPTION 'service_configs update rejected'; END $$ LANGUAGE plpgsql; @@ -86,3 +99,52 @@ func TestUpdateConfigPropagatesOnlyChangedDefaults(t *testing.T) { require.Equal(t, 1, after.DefaultTerm, "global change must roll back with the failed propagation") require.Equal(t, ec2, service("ec2")) } + +// TestUpdateConfigPaymentDefaultSkipsAzureAndGCP: the global default payment is +// an AWS token, so changing it must not rewrite Azure or GCP rows (an Azure +// monthly row would turn upfront; a GCP row would fail validation forever). +func TestUpdateConfigPaymentDefaultSkipsAzureAndGCP(t *testing.T) { + ctx := context.Background() + pg := testhelpers.RequireMigratedDatabase(ctx, t, "../database/postgres/migrations") + store := config.NewPostgresStore(pg.DB) + + global, err := store.GetGlobalConfig(ctx) + require.NoError(t, err) + global.EnabledProviders = []string{"aws", "azure", "gcp"} + global.DefaultTerm, global.DefaultPayment, global.DefaultCoverage = 1, "no-upfront", 80 + global.DefaultRampSchedule = config.RampImmediate + require.NoError(t, store.SaveGlobalConfig(ctx, global)) + for _, svc := range []config.ServiceConfig{ + {Provider: "aws", Service: "ec2", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80, RampSchedule: config.RampImmediate}, + {Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "monthly", Coverage: 80, RampSchedule: config.RampImmediate}, + {Provider: "gcp", Service: "compute", Enabled: true, Term: 1, Payment: "monthly", Coverage: 80, RampSchedule: config.RampImmediate}, + } { + svc := svc + require.NoError(t, store.SaveServiceConfig(ctx, &svc)) + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + mockAuth.grantAdmin() + handler := &Handler{config: store, auth: mockAuth} + headers := map[string]string{"Authorization": "Bearer admin-token"} + + _, err = handler.updateConfig(ctx, &events.LambdaFunctionURLRequest{Headers: headers, Body: `{"default_payment": "all-upfront"}`}) + require.NoError(t, err) + + payment := func(provider string) string { + name := "compute" + if provider == "aws" { + name = "ec2" + } + svc, getErr := store.GetServiceConfig(ctx, provider, name) + require.NoError(t, getErr) + return svc.Payment + } + require.Equal(t, "all-upfront", payment("aws")) + require.Equal(t, "monthly", payment("azure")) + require.Equal(t, "monthly", payment("gcp")) + + _, err = handler.updateServiceConfig(ctx, &events.LambdaFunctionURLRequest{Headers: headers, Body: `{"term": 3}`}, "gcp/compute") + require.NoError(t, err) +} diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index cee0157ca..c89d073de 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -721,6 +721,113 @@ func TestMergeServiceConfig_PresenceAwareFilterOverlay(t *testing.T) { }) } +// TestMergeServiceConfig_PresenceAwareScalarOverlay verifies that the four +// scalar fields (enabled, term, payment, coverage) are overlaid only when the +// body carries the key, so a stale tab that PUTs just the field the user +// edited cannot revert the others (#539). A present zero value still applies. +func TestMergeServiceConfig_PresenceAwareScalarOverlay(t *testing.T) { + ctx := context.Background() + newExisting := func() *config.ServiceConfig { + return &config.ServiceConfig{ + Provider: "aws", Service: "rds", Enabled: true, Term: 3, + Payment: "all-upfront", Coverage: 50, + } + } + run := func(t *testing.T, req config.ServiceConfig, body string) config.ServiceConfig { + t.Helper() + store := new(MockConfigStore) + store.On("GetServiceConfig", ctx, "aws", "rds").Return(newExisting(), nil) + t.Cleanup(func() { store.AssertExpectations(t) }) + req.Provider, req.Service = "aws", "rds" + merged, err := mergeServiceConfig(ctx, store, req, body) + require.NoError(t, err) + return merged + } + + t.Run("only coverage sent -> other scalars preserved", func(t *testing.T) { + merged := run(t, config.ServiceConfig{Coverage: 90}, `{"coverage":90}`) + assert.Equal(t, 90.0, merged.Coverage) + assert.True(t, merged.Enabled) + assert.Equal(t, 3, merged.Term) + assert.Equal(t, "all-upfront", merged.Payment) + }) + + t.Run("only term sent -> other scalars preserved", func(t *testing.T) { + merged := run(t, config.ServiceConfig{Term: 1}, `{"term":1}`) + assert.Equal(t, 1, merged.Term) + assert.Equal(t, 50.0, merged.Coverage) + assert.Equal(t, "all-upfront", merged.Payment) + assert.True(t, merged.Enabled) + }) + + t.Run("present zero values still apply", func(t *testing.T) { + merged := run(t, config.ServiceConfig{}, `{"enabled":false,"coverage":0}`) + assert.False(t, merged.Enabled) + assert.Equal(t, 0.0, merged.Coverage) + assert.Equal(t, 3, merged.Term) + }) + + t.Run("only payment sent -> other scalars preserved", func(t *testing.T) { + merged := run(t, config.ServiceConfig{Payment: "no-upfront"}, `{"payment":"no-upfront"}`) + assert.Equal(t, "no-upfront", merged.Payment) + assert.Equal(t, 3, merged.Term) + assert.Equal(t, 50.0, merged.Coverage) + }) +} + +// TestHandler_updateServiceConfig_PartialPut drives the endpoint with partial +// bodies (#539): absent keys keep the stored row, and validation runs on the +// merged row, not on the partial body. +func TestHandler_updateServiceConfig_PartialPut(t *testing.T) { + ctx := context.Background() + newHandler := func(existing *config.ServiceConfig) (*Handler, *MockConfigStore) { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com", + }, nil) + mockAuth.grantAdmin() + store := new(MockConfigStore) + store.On("GetServiceConfig", ctx, existing.Provider, existing.Service).Return(existing, nil) + return &Handler{config: store, auth: mockAuth}, store + } + put := func(h *Handler, body, path string) (*StatusResponse, error) { + return h.updateServiceConfig(ctx, &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: body, + }, path) + } + + t.Run("absent keys keep the stored row", func(t *testing.T) { + h, store := newHandler(&config.ServiceConfig{ + Provider: "aws", Service: "rds", Enabled: true, Term: 3, + Payment: "all-upfront", Coverage: 55, MinCount: 4, + }) + var saved *config.ServiceConfig + store.On("SaveServiceConfig", ctx, mock.AnythingOfType("*config.ServiceConfig")). + Run(func(args mock.Arguments) { saved = args.Get(1).(*config.ServiceConfig) }).Return(nil) + + _, err := put(h, `{"term":1}`, "aws/rds") + require.NoError(t, err) + require.NotNil(t, saved) + assert.Equal(t, 1, saved.Term) + assert.True(t, saved.Enabled) + assert.Equal(t, "all-upfront", saved.Payment) + assert.Equal(t, 55.0, saved.Coverage) + assert.Equal(t, 4, saved.MinCount) + }) + + t.Run("validation covers the merged row", func(t *testing.T) { + h, store := newHandler(&config.ServiceConfig{ + Provider: "gcp", Service: "compute", Enabled: true, Term: 1, + Payment: "all-upfront", Coverage: 80, + }) + _, err := put(h, `{"term":3}`, "gcp/compute") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid payment option") + store.AssertNotCalled(t, "SaveServiceConfig", mock.Anything, mock.Anything) + }) +} + func TestHandler_updateServiceConfig_InvalidBody(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 51a497744..8277b659a 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -259,8 +259,11 @@ paths: tags: [Configuration] summary: Update per-service configuration description: > - Provider and service are taken from the path. The response does not - echo the configuration; read it back with GET. + Provider and service are taken from the path. This is a partial + update: any enabled, term, payment, coverage or filter key absent from + the body keeps its stored value, and the merged row is validated. A + key that is present, even with a zero value, replaces the stored one. + The response does not echo the configuration; read it back with GET. parameters: - $ref: '#/components/parameters/CSRFToken' requestBody: diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index e31f7c9c8..879dc305c 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -224,11 +224,16 @@ func (s *PostgresStore) UpdateGlobalConfigAtomic(ctx context.Context, apply func } // propagateChangedDefaults overwrites a service-config column only when its -// global default changed, so a round-tripped or partial PUT keeps per-service +// global default changed (the payment default only on AWS rows), so a round-tripped or partial PUT keeps per-service // customisations. func propagateChangedDefaults(ctx context.Context, tx pgx.Tx, before, after *GlobalConfig) error { termChanged := after.DefaultTerm != before.DefaultTerm - paymentChanged := after.DefaultPayment != before.DefaultPayment + // The global default payment is an AWS token. Azure and GCP rows keep their + // own tokens (monthly/upfront): copying "all-upfront" onto them is an + // upfront charge nobody chose on Azure and fails GCP validation on every + // later save. Write it only to AWS rows, and only when AWS accepts it. + paymentChanged := after.DefaultPayment != before.DefaultPayment && + slices.Contains(ValidPaymentOptionsByProvider["aws"], after.DefaultPayment) coverageChanged := after.DefaultCoverage != before.DefaultCoverage rampChanged := after.DefaultRampSchedule != before.DefaultRampSchedule if !termChanged && !paymentChanged && !coverageChanged && !rampChanged { @@ -237,7 +242,7 @@ func propagateChangedDefaults(ctx context.Context, tx pgx.Tx, before, after *Glo const query = ` UPDATE service_configs SET term = CASE WHEN $1 THEN $2 ELSE term END, - payment = CASE WHEN $3 THEN $4 ELSE payment END, + payment = CASE WHEN $3 AND provider = 'aws' THEN $4 ELSE payment END, coverage = CASE WHEN $5 THEN $6 ELSE coverage END, ramp_schedule = CASE WHEN $7 THEN $8 ELSE ramp_schedule END `