From eea7b4660333e3d25d4854a876b7fc15eb189edb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 12:17:15 +0100 Subject: [PATCH 1/6] fix(api): merge service config PUT fields only when present mergeServiceConfig overlaid enabled, term, payment and coverage from every PUT body, so a client sending just the field the user edited reset the other three to zero values. A stale Settings tab sending a full row could likewise revert a row another admin had changed. Overlay the four scalars only when the body carries the key, like the recommendation filters already are; a present zero (enabled:false, coverage:0) still applies. Global PUT /api/config already merges by presence and propagates only changed defaults; a Postgres test now pins that a partial PUT of an unrelated key leaves defaults and service rows untouched. Part of #539 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_config.go | 48 +++++++++++------ ...ler_config_propagation_integration_test.go | 13 +++++ internal/api/handler_config_test.go | 54 +++++++++++++++++++ 3 files changed, 99 insertions(+), 16 deletions(-) diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 18a57957a..4317219d0 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 diff --git a/internal/api/handler_config_propagation_integration_test.go b/internal/api/handler_config_propagation_integration_test.go index d5c034fec..15267c4eb 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; diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index cee0157ca..8e0f209bb 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -721,6 +721,60 @@ 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) + }) +} + func TestHandler_updateServiceConfig_InvalidBody(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) From dd88959f0be5dc4b7947c6baaf51c80f65d4abf5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 12:17:39 +0100 Subject: [PATCH 2/6] fix(settings): PUT only the service rows and fields the user changed Save PUT every service row (17) with the term, payment, coverage and enabled values read from the form, so a tab opened before another admin's change wrote the stale values back over that admin's service config. Diff each row against the saved snapshot and the loaded row, skip rows with no change, and send just the changed keys now that the backend overlays them by presence. A changed default coverage is no longer pushed onto the rows by the client (the backend already propagates it in the global PUT); the unedited Savings Plan cards are refreshed to the new value instead. Rows loaded from a legacy Azure/GCP id are still written in full once so the #613 re-key keeps working, and a stored payment the card normalised on load goes out with any PUT of its row (#545). Tests: unedited form sends no service PUTs, one edit sends one row with only the edited key, a second save resends nothing, a Playwright check of the Purchasing Save. Existing tests that relied on every row being PUT now edit the rows they assert on. Part of #539. Grace-period and enabled_providers are still sent whole. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/settings-service-ids.test.ts | 3 + .../settings-service-payment.test.ts | 17 +- frontend/src/__tests__/settings.test.ts | 154 +++++++++++----- frontend/src/api/settings.ts | 2 +- frontend/src/settings.ts | 174 ++++++++++++------ .../settings-save-changed-fields.spec.ts | 43 +++++ 6 files changed, 281 insertions(+), 112 deletions(-) create mode 100644 frontend/tests-e2e/settings-save-changed-fields.spec.ts diff --git a/frontend/src/__tests__/settings-service-ids.test.ts b/frontend/src/__tests__/settings-service-ids.test.ts index 317492498..b50679694 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[] = []; diff --git a/frontend/src/__tests__/settings-service-payment.test.ts b/frontend/src/__tests__/settings-service-payment.test.ts index 607203ab9..c05d56a9f 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,10 @@ 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. + expect(serviceCalls().filter(c => c.provider === 'gcp').every(c => c.payment === undefined || c.payment === 'monthly')).toBe(true); expect(serviceCalls().some(c => c.service === 'storage')).toBe(false); }); @@ -160,7 +170,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 +214,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 +230,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')); diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 99f238f32..58d316ce7 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,52 @@ 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).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 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 +927,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 +989,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 +1008,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 +1024,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 +1133,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 +1150,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 +1187,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 +2011,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 +2027,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 +2053,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..4765d14db 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; @@ -3368,6 +3377,91 @@ function invalidEditedCardCoverages(): string[] { 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 +3610,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 +3630,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 +3666,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 }); +}); From f1f1bc6fccd5560fe16ef51942aa33cb75beb62c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 12:36:46 +0100 Subject: [PATCH 3/6] fix(config): propagate the default payment only to providers that accept it propagateChangedDefaults wrote the global default payment onto every service_configs row. The default is an AWS token, so an Azure monthly row became all-upfront (an upfront charge nobody chose) and a GCP row became all-upfront, which GCP validation rejects on every later save. The client used to mask this by re-PUTting every row; it no longer does. Write the payment default only to AWS rows, and only when AWS accepts the value. Term and coverage are valid for every provider and keep propagating to all rows. Also document that PUT /api/config/service is a partial update, fix the stale handler comment, and add handler tests that a partial body keeps the stored row and that validation runs on the merged row. Part of #539 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_config.go | 8 +-- ...ler_config_propagation_integration_test.go | 49 +++++++++++++++++ internal/api/handler_config_test.go | 53 +++++++++++++++++++ internal/api/openapi.yaml | 7 ++- internal/config/store_postgres.go | 11 ++-- 5 files changed, 119 insertions(+), 9 deletions(-) diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index 4317219d0..182e2425a 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -360,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 15267c4eb..ba9bfaf04 100644 --- a/internal/api/handler_config_propagation_integration_test.go +++ b/internal/api/handler_config_propagation_integration_test.go @@ -99,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 8e0f209bb..c89d073de 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -775,6 +775,59 @@ func TestMergeServiceConfig_PresenceAwareScalarOverlay(t *testing.T) { }) } +// 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 ` From e968bc16cadcbb0134fcf646016391f9d9d18810 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 12:36:57 +0100 Subject: [PATCH 4/6] test(settings): pin the service PUT diff against surviving mutations Restore the exact GCP payment assertion in the stored-row test and add tests for the three behaviours a mutation could drop: a normalised stored payment rides along with a partial PUT of its row (#545), the loaded row advances after a successful PUT so a saved filter is not resent, and a legacy re-keyed row is written in full only once. Part of #539 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/settings-service-ids.test.ts | 21 +++++++++++++++++++ .../settings-service-payment.test.ts | 21 ++++++++++++++++++- frontend/src/__tests__/settings.test.ts | 15 +++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/settings-service-ids.test.ts b/frontend/src/__tests__/settings-service-ids.test.ts index b50679694..42a8dd36e 100644 --- a/frontend/src/__tests__/settings-service-ids.test.ts +++ b/frontend/src/__tests__/settings-service-ids.test.ts @@ -151,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 c05d56a9f..ca8361e45 100644 --- a/frontend/src/__tests__/settings-service-payment.test.ts +++ b/frontend/src/__tests__/settings-service-payment.test.ts @@ -132,7 +132,12 @@ describe('Settings Save payment tokens (issue #545)', () => { editEveryServiceTerm(); await saveGlobalSettings(new Event('submit')); // gcp/compute has a stored row, so only its term goes out; the new rows carry monthly. - expect(serviceCalls().filter(c => c.provider === 'gcp').every(c => c.payment === undefined || c.payment === 'monthly')).toBe(true); + 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); }); @@ -257,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 58d316ce7..ddf5a6d22 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -895,6 +895,21 @@ describe('Settings Module', () => { }); }); + 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('a second save after a service PUT succeeded resends nothing', async () => { await loadStaleTab(); (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '65'; From 3e11b0007fec08216a8d11e48f329464f95c179c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 14:36:40 +0100 Subject: [PATCH 5/6] fix(settings): refuse an empty edited coverage instead of dropping it invalidEditedCardCoverages skipped an empty edited Savings Plan coverage input, so Save omitted the coverage, the server kept the old value, the snapshot recorded the blank as saved and "Settings saved successfully" showed while the input stayed empty. Treat an edited card cleared to empty as invalid, with the same toast as an out-of-range value, so the save is blocked and the field stays dirty. Unedited cards and absent inputs are unaffected. Part of #539 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/settings.test.ts | 21 +++++++++++++++++++++ frontend/src/settings.ts | 12 +++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index ddf5a6d22..c47c9b9ab 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -910,6 +910,27 @@ describe('Settings Module', () => { 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).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining('Target coverage must be a whole number between 0 and 100'), + 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'; diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 4765d14db..e9625d25a 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3359,18 +3359,20 @@ 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); } From 2529b281e4493b340846b865cf609b077c8c8bae Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 14:49:45 +0100 Subject: [PATCH 6/6] fix(settings): match the renamed coverage toast in the empty-coverage test origin/main renamed the settings coverage validation toast from "Target coverage" to "Dashboard coverage" (#1463). The empty edited coverage test asserted the old wording, so it passed on the branch alone and failed on the merge with main. Assert the stable tail of the message and the card name instead. Part of #539 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/settings.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index c47c9b9ab..f4d060700 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -921,7 +921,7 @@ describe('Settings Module', () => { expect(api.updateConfig).not.toHaveBeenCalled(); expect(api.updateServiceConfig).not.toHaveBeenCalled(); expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ - message: expect.stringContaining('Target coverage must be a whole number between 0 and 100'), + message: expect.stringContaining('coverage must be a whole number between 0 and 100 (savings-plans-compute)'), kind: 'error', })); expect(isUnsavedChanges()).toBe(true);