From 951d7181540de6a88c7e0eb6edcd7e1e37ff6caf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 12:51:25 +0100 Subject: [PATCH 1/2] fix(settings): repair GCP rows holding an AWS payment and pin the AWS-only propagation Before the AWS-only payment propagation, a changed global default could copy an AWS token onto GCP rows, which GCP validation then rejects on every later save. GCP cards have no payment control, so treat a GCP row whose stored payment is not monthly like the normalised-payment case (#545): the next PUT of that row carries payment monthly. Also add an integration case for the "AWS accepts it" clause of propagateChangedDefaults: a default payment of monthly leaves AWS rows unchanged. Part of #539 Co-Authored-By: Claude Opus 5.5 --- .../settings-service-payment.test.ts | 15 ++++++++ frontend/src/settings.ts | 6 ++-- ...es_payment_propagation_integration_test.go | 36 +++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 internal/config/store_postgres_payment_propagation_integration_test.go diff --git a/frontend/src/__tests__/settings-service-payment.test.ts b/frontend/src/__tests__/settings-service-payment.test.ts index ca8361e45..6a92429ec 100644 --- a/frontend/src/__tests__/settings-service-payment.test.ts +++ b/frontend/src/__tests__/settings-service-payment.test.ts @@ -276,6 +276,21 @@ describe('Settings Save payment tokens (issue #545)', () => { ]); }); + it.each([ + ['all-upfront', { provider: 'gcp', service: 'compute', term: 1, payment: 'monthly' }], + ['monthly', { provider: 'gcp', service: 'compute', term: 1 }], + ])('a GCP row holding payment %p: a term edit PUTs %j', async (stored, body) => { + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { enabled_providers: ['gcp'], default_term: 3, default_payment: 'all-upfront', default_coverage: 80 }, + services: [{ provider: 'gcp', service: 'compute', term: 3, payment: stored, enabled: true, coverage: 80 }], + }); + await loadGlobalSettings(); + (document.getElementById('gcp-compute-term') as HTMLSelectElement).value = '1'; + await saveGlobalSettings(new Event('submit')); + + expect((api.updateServiceConfig as jest.Mock).mock.calls).toEqual([['gcp', 'compute', body]]); + }); + 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/settings.ts b/frontend/src/settings.ts index 68f928607..b26e4d0a9 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3420,10 +3420,12 @@ function changedServiceFields( 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) { + if (!changes.payment && 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; + // GCP cards have no payment control and GCP only accepts monthly, so a row + // holding another token (an AWS default copied onto it) is repaired too. + const shown = field.paymentId ? byId(field.paymentId)?.value : field.provider === 'gcp' ? 'monthly' : undefined; if (shown && shown !== base.payment) changes.payment = shown; } return changes; diff --git a/internal/config/store_postgres_payment_propagation_integration_test.go b/internal/config/store_postgres_payment_propagation_integration_test.go new file mode 100644 index 000000000..9689c87e3 --- /dev/null +++ b/internal/config/store_postgres_payment_propagation_integration_test.go @@ -0,0 +1,36 @@ +//go:build integration +// +build integration + +package config + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" +) + +// A default payment valid globally but not for AWS ("monthly") must not be +// copied onto AWS rows (#539). +func TestIntegration_PropagateDefaultPayment_SkipsValueAWSRejects(t *testing.T) { + ctx := context.Background() + store := setupRampStepStore(ctx, t) + + global, err := store.GetGlobalConfig(ctx) + require.NoError(t, err) + global.DefaultPayment = "no-upfront" + require.NoError(t, store.SaveGlobalConfig(ctx, global)) + require.NoError(t, store.SaveServiceConfig(ctx, &ServiceConfig{ + Provider: "aws", Service: "ec2", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80, RampSchedule: RampImmediate, + })) + + _, err = store.UpdateGlobalConfigAtomic(ctx, func(c *GlobalConfig) error { + c.DefaultPayment = "monthly" + return nil + }) + require.NoError(t, err) + + svc, err := store.GetServiceConfig(ctx, "aws", "ec2") + require.NoError(t, err) + require.Equal(t, "no-upfront", svc.Payment) +} From f424efecb00b6c9557b5fc15362cd41e0ffe9434 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 26 Oct 2026 15:49:25 +0100 Subject: [PATCH 2/2] fix(settings): show an inline coverage error and refuse an empty cache window An emptied Savings Plan coverage card was only caught by the Save toast. Mark the card validators required so the inline error shows too. An emptied cache-stale-hours input read as Number('') = 0 and saved as "disable auto-refresh". 0 is a legal value (the backend accepts 0..8760), so refuse only the empty string with the existing validation error. Also pin that an unedited coverage card is never flagged by the Save-time check, whatever its stored value. Part of #539 Co-Authored-By: Claude Opus 5.5 --- .../settings-service-payment.test.ts | 10 ++++++ frontend/src/__tests__/settings.test.ts | 35 +++++++++++++++++++ frontend/src/settings.ts | 11 +++--- 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/settings-service-payment.test.ts b/frontend/src/__tests__/settings-service-payment.test.ts index 6a92429ec..55d6afba6 100644 --- a/frontend/src/__tests__/settings-service-payment.test.ts +++ b/frontend/src/__tests__/settings-service-payment.test.ts @@ -291,6 +291,16 @@ describe('Settings Save payment tokens (issue #545)', () => { expect((api.updateServiceConfig as jest.Mock).mock.calls).toEqual([['gcp', 'compute', body]]); }); + it('an emptied Savings Plan coverage card shows an inline error', async () => { + await loadGlobalSettings(); + setupSettingsHandlers(); + const card = document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement; + card.value = ''; + card.dispatchEvent(new Event('blur')); + + expect(card.getAttribute('aria-invalid')).toBe('true'); + }); + 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 b44cac070..36710802b 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -931,6 +931,41 @@ describe('Settings Module', () => { expect(api.updateServiceConfig).toHaveBeenCalledWith('aws', 'savings-plans-compute', expect.objectContaining({ coverage: 65 })); }); + test('an unedited SP card is never flagged, whatever its stored value', async () => { + await loadStaleTab(); + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { enabled_providers: ['aws'], default_term: 1, default_payment: 'no-upfront', default_coverage: 50, notification_days_before: 3 }, + services: [{ provider: 'aws', service: 'savings-plans-compute', enabled: true, term: 1, payment: 'no-upfront', coverage: 150 }], + }); + await loadGlobalSettings(); + (document.getElementById('setting-notification-email') as HTMLInputElement).value = 'new@test.com'; + mockShowToast.mockClear(); + + await save(); + + expect(api.updateConfig).toHaveBeenCalledWith({ notification_email: 'new@test.com' }); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'success' })); + }); + + test('an emptied cache stale threshold is refused, an explicit 0 is saved', async () => { + await loadStaleTab(); + const hours = document.getElementById('setting-recs-stale-hours') as HTMLInputElement; + hours.value = ''; + mockShowToast.mockClear(); + + await save(); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + message: expect.stringContaining('Cache stale threshold must be a whole number'), + kind: 'error', + })); + + hours.value = '0'; + await save(); + expect(api.updateConfig).toHaveBeenCalledWith({ recommendations_cache_stale_hours: 0 }); + }); + 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 b26e4d0a9..0796fdb30 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -2621,7 +2621,7 @@ export function setupSettingsHandlers(signal?: AbortSignal): void { // save-time-only toast contract) — so the inline-error UX is uniform // across Settings. wireInlineRangeValidation('setting-notification-days', { signal, requireInteger: true }); - wireInlineRangeValidation('setting-recs-stale-hours', { signal, requireInteger: true }); + wireInlineRangeValidation('setting-recs-stale-hours', { signal, requireInteger: true, required: true }); wireInlineRangeValidation('setting-default-coverage', { signal, requireInteger: true, required: true }); wireInlineRangeValidation('setting-grace-aws', { signal, requireInteger: true }); wireInlineRangeValidation('setting-grace-azure', { signal, requireInteger: true }); @@ -2634,7 +2634,7 @@ export function setupSettingsHandlers(signal?: AbortSignal): void { .forEach(f => wireInlineRangeValidation( (f as { coverageId: string }).coverageId, - { signal, requireInteger: true }, + { signal, requireInteger: true, required: true }, ) ); @@ -3543,8 +3543,11 @@ export async function saveGlobalSettings(e: Event): Promise { saveInFlight = false; return; } - const rawStaleHours = Number(byId('setting-recs-stale-hours')?.value ?? '24'); - if (!Number.isFinite(rawStaleHours) || !Number.isInteger(rawStaleHours) || rawStaleHours < 0 || rawStaleHours > 8760) { + const staleHoursText = byId('setting-recs-stale-hours')?.value ?? '24'; + const rawStaleHours = Number(staleHoursText); + // 0 is a legal value (disables auto-refresh) but Number('') is also 0, so an + // emptied field must be rejected explicitly rather than saved as "disable". + if (staleHoursText.trim() === '' || !Number.isFinite(rawStaleHours) || !Number.isInteger(rawStaleHours) || rawStaleHours < 0 || rawStaleHours > 8760) { showToast({ message: 'Cache stale threshold must be a whole number between 0 and 8760 hours (0 = disable)', kind: 'error' }); if (saveBtn) saveBtn.disabled = false; saveInFlight = false;