From 851bd5cba60f4a61f7799b154c6186b5c035de16 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 18:45:00 +0200 Subject: [PATCH 1/2] feat(settings): per-product Coverage and Enabled controls on SP cards (closes #136) Add Coverage % and Enabled toggle to each of the four Savings Plans cards (Compute, EC2 Instance, SageMaker, Database), matching the per-product settings already available on RI cards. - SERVICE_FIELDS SP entries extended with coverageId/enabledId - TRACKED_FIELDS includes the new IDs for dirty-state detection - loadGlobalSettings populates per-card coverage/enabled from service config, falling back to global default_coverage when absent - saveGlobalSettings reads per-card coverage/enabled from DOM when the card exposes the controls; non-SP cards continue to inherit global settings - index.html: Coverage % number input and Enabled checkbox added to each SP card in the settings form - Tests: DOM fixture updated; 3 new tests cover save-coverage, save-enabled, and load-from-service-config paths; HTML structural assertions extended --- frontend/src/__tests__/settings.test.ts | 84 +++++++++++++++++++++++++ frontend/src/index.html | 8 +++ frontend/src/settings.ts | 43 ++++++++++--- 3 files changed, 128 insertions(+), 7 deletions(-) diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index c4848ce62..450b7b6df 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -95,6 +95,15 @@ describe('Settings Module', () => { + + + + + + + + + @@ -590,6 +599,9 @@ describe('Settings Module', () => { for (const planType of ['compute', 'ec2instance', 'sagemaker', 'database']) { expect(html).toMatch(new RegExp(`id="aws-savings-plans-${planType}-term"`)); expect(html).toMatch(new RegExp(`id="aws-savings-plans-${planType}-payment"`)); + // Issue #136: coverage and enabled controls must be present on each card. + expect(html).toMatch(new RegExp(`id="aws-savings-plans-${planType}-coverage"`)); + expect(html).toMatch(new RegExp(`id="aws-savings-plans-${planType}-enabled"`)); } }); @@ -626,6 +638,78 @@ describe('Settings Module', () => { expect(cfg.payment).toBe('no-upfront'); }); + test('saveGlobalSettings sends per-card coverage from SP coverage input (issue #136)', async () => { + (api.updateConfig as jest.Mock).mockResolvedValue({}); + (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); + window.alert = jest.fn(); + + // Pin compute SP to 60% coverage, leave others at default 80%. + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = '60'; + + const event = { preventDefault: jest.fn() } as unknown as Event; + await saveGlobalSettings(event); + + const computeCall = (api.updateServiceConfig as jest.Mock).mock.calls.find( + ([provider, service]) => provider === 'aws' && service === 'savings-plans-compute', + ); + expect(computeCall).toBeDefined(); + expect(computeCall![2].coverage).toBe(60); + + // Other SP cards keep their default value. + 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); + }); + + test('saveGlobalSettings sends per-card enabled=false when SP enabled checkbox unchecked (issue #136)', async () => { + (api.updateConfig as jest.Mock).mockResolvedValue({}); + (api.updateServiceConfig as jest.Mock).mockClear().mockResolvedValue(undefined); + window.alert = jest.fn(); + + // Disable the database SP card. + (document.getElementById('aws-savings-plans-database-enabled') as HTMLInputElement).checked = false; + + const event = { preventDefault: jest.fn() } as unknown as Event; + await saveGlobalSettings(event); + + const dbCall = (api.updateServiceConfig as jest.Mock).mock.calls.find( + ([provider, service]) => provider === 'aws' && service === 'savings-plans-database', + ); + expect(dbCall).toBeDefined(); + expect(dbCall![2].enabled).toBe(false); + + // Other SP cards remain enabled. + 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); + }); + + test('loadGlobalSettings populates SP coverage and enabled from service config (issue #136)', async () => { + (api.getConfig as jest.Mock).mockResolvedValue({ + global: { enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 80 }, + services: [ + { provider: 'aws', service: 'savings-plans-compute', term: 1, payment: 'no-upfront', coverage: 65, enabled: false }, + ], + }); + setupSettingsHandlers(); + await loadGlobalSettings(); + + const coverageEl = document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement; + const enabledEl = document.getElementById('aws-savings-plans-compute-enabled') as HTMLInputElement; + expect(coverageEl.value).toBe('65'); + expect(enabledEl.checked).toBe(false); + + // Cards without an explicit service row fall back to global defaults. + const ec2Coverage = document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement; + const ec2Enabled = document.getElementById('aws-savings-plans-ec2instance-enabled') as HTMLInputElement; + expect(ec2Coverage.value).toBe('80'); + expect(ec2Enabled.checked).toBe(true); + }); + test('calls updateServiceConfig once per service field (18 calls)', async () => { (api.updateConfig as jest.Mock).mockResolvedValue({}); (api.updateServiceConfig as jest.Mock).mockResolvedValue(undefined); diff --git a/frontend/src/index.html b/frontend/src/index.html index 6ce57e316..ade5fa6fb 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -506,24 +506,32 @@
Compute Savings Plans

EC2, Fargate, Lambda — most flexible

+ +
EC2 Instance Savings Plans

EC2 only, region-locked — deepest discount

+ +
SageMaker Savings Plans

SageMaker training/inference

+ +
Database Savings Plans

Reserved for future AWS Database Savings Plans (currently not GA; defaults stored for forward-compatibility)

+ +
diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index e9ebc1838..a37c1c128 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -91,10 +91,12 @@ const SERVICE_FIELDS = [ // (when present) overrides the SageMaker slot from PR #71's sagemaker // row. Lambda is intentionally NOT a separate card — Lambda has no // standalone SP product; its commitments roll up into Compute SP. - { provider: 'aws', service: 'savings-plans-compute', termId: 'aws-savings-plans-compute-term', paymentId: 'aws-savings-plans-compute-payment' }, - { provider: 'aws', service: 'savings-plans-ec2instance', termId: 'aws-savings-plans-ec2instance-term', paymentId: 'aws-savings-plans-ec2instance-payment' }, - { provider: 'aws', service: 'savings-plans-sagemaker', termId: 'aws-savings-plans-sagemaker-term', paymentId: 'aws-savings-plans-sagemaker-payment' }, - { provider: 'aws', service: 'savings-plans-database', termId: 'aws-savings-plans-database-term', paymentId: 'aws-savings-plans-database-payment' }, + // Issue #136: SP cards carry per-card coverageId and enabledId so users + // can set divergent coverage and toggle each plan type independently. + { provider: 'aws', service: 'savings-plans-compute', termId: 'aws-savings-plans-compute-term', paymentId: 'aws-savings-plans-compute-payment', coverageId: 'aws-savings-plans-compute-coverage', enabledId: 'aws-savings-plans-compute-enabled' }, + { provider: 'aws', service: 'savings-plans-ec2instance', termId: 'aws-savings-plans-ec2instance-term', paymentId: 'aws-savings-plans-ec2instance-payment', coverageId: 'aws-savings-plans-ec2instance-coverage', enabledId: 'aws-savings-plans-ec2instance-enabled' }, + { provider: 'aws', service: 'savings-plans-sagemaker', termId: 'aws-savings-plans-sagemaker-term', paymentId: 'aws-savings-plans-sagemaker-payment', coverageId: 'aws-savings-plans-sagemaker-coverage', enabledId: 'aws-savings-plans-sagemaker-enabled' }, + { provider: 'aws', service: 'savings-plans-database', termId: 'aws-savings-plans-database-term', paymentId: 'aws-savings-plans-database-payment', coverageId: 'aws-savings-plans-database-coverage', enabledId: 'aws-savings-plans-database-enabled' }, { provider: 'azure', service: 'vm', termId: 'azure-vm-term', paymentId: 'azure-vm-payment' }, { provider: 'azure', service: 'sql', termId: 'azure-sql-term', paymentId: 'azure-sql-payment' }, { provider: 'azure', service: 'cosmosdb', termId: 'azure-cosmosdb-term', paymentId: 'azure-cosmosdb-payment' }, @@ -120,6 +122,9 @@ const TRACKED_FIELDS = [ // Per-service fields ...SERVICE_FIELDS.map(f => f.termId), ...SERVICE_FIELDS.filter(f => f.paymentId !== null).map(f => f.paymentId as string), + // Per-product SP fields (issue #136) + ...SERVICE_FIELDS.filter(f => 'coverageId' in f).map(f => (f as { coverageId: string }).coverageId), + ...SERVICE_FIELDS.filter(f => 'enabledId' in f).map(f => (f as { enabledId: string }).enabledId), ]; // Snapshot of field values at last save (or initial load). @@ -2674,6 +2679,12 @@ export async function loadGlobalSettings(): Promise { if (termEl) termEl.value = String(svc.term); const paymentEl = document.getElementById(`${key}-payment`) as HTMLSelectElement | null; if (paymentEl) paymentEl.value = svc.payment; + // Issue #136: populate per-product SP coverage and enabled fields when + // the card exposes them. Other service cards fall through (IDs absent). + const coverageEl = document.getElementById(`${key}-coverage`) as HTMLInputElement | null; + if (coverageEl) coverageEl.value = String(svc.coverage ?? data.global?.default_coverage ?? 80); + const enabledEl = document.getElementById(`${key}-enabled`) as HTMLInputElement | null; + if (enabledEl) enabledEl.checked = svc.enabled !== false; } } @@ -2903,7 +2914,8 @@ export async function saveGlobalSettings(e: Event): Promise { try { await api.updateConfig(settings); - const serviceSaves = SERVICE_FIELDS.map(({ provider, service, termId, paymentId }) => { + 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 || 'all-upfront') @@ -2913,14 +2925,31 @@ export async function saveGlobalSettings(e: Event): Promise { // include_engines, etc. can be set out-of-band (API, future UI, migration) // and a full UPSERT that only honoured the four term/payment/enabled/coverage // fields would silently wipe them every time the user clicked Save. + + // 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); + if (rawCov !== '' && Number.isFinite(parsed)) coverage = parsed; + } + if ('enabledId' in field && field.enabledId) { + const el = byId(field.enabledId); + if (el) enabled = el.checked; + } + const cfg: api.ServiceConfig = { ...(base ?? {}), provider, service, - enabled: base?.enabled ?? true, + enabled, term, payment, - coverage: base?.coverage ?? settings.default_coverage, + coverage, }; return api.updateServiceConfig(provider, service, cfg); }); From 822a4aa35ca0b18600e6b089587a69f8f2b787fc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 12:17:52 +0200 Subject: [PATCH 2/2] fix(settings): apply SP coverage/enabled fallback when service row absent loadGlobalSettings only populated per-product Savings Plans coverage and enabled controls for services present in the config response. A SP card whose service row was missing kept the HTML default (80) instead of falling back to global.default_coverage, which then persisted an incorrect value on the next save. Iterate SERVICE_FIELDS (the source of truth for SP card IDs) and overlay any matching service-config values, so every SP card gets the global default when its row is absent. Harden the regression test with a non-default seeded DOM state and a non-80 global default so the fallback assignment is actually exercised. Addresses CodeRabbit review on PR #580. --- frontend/src/__tests__/settings.test.ts | 14 +++++++-- frontend/src/settings.ts | 41 ++++++++++++++++--------- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 450b7b6df..2a5dade9e 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -689,8 +689,15 @@ describe('Settings Module', () => { }); test('loadGlobalSettings populates SP coverage and enabled from service config (issue #136)', async () => { + // Seed a non-default initial DOM state on the fallback card so the test + // fails if the fallback assignment is skipped (it would otherwise pass on + // the HTML defaults, which happen to equal the asserted values). + (document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement).value = '12'; + (document.getElementById('aws-savings-plans-ec2instance-enabled') as HTMLInputElement).checked = false; + (api.getConfig as jest.Mock).mockResolvedValue({ - global: { enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 80 }, + // Non-80 global default so the fallback writes an observably different value. + global: { enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 67 }, services: [ { provider: 'aws', service: 'savings-plans-compute', term: 1, payment: 'no-upfront', coverage: 65, enabled: false }, ], @@ -703,10 +710,11 @@ describe('Settings Module', () => { expect(coverageEl.value).toBe('65'); expect(enabledEl.checked).toBe(false); - // Cards without an explicit service row fall back to global defaults. + // Cards without an explicit service row fall back to the global default + // (67, not the HTML default 80) and re-enable from the seeded false state. const ec2Coverage = document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement; const ec2Enabled = document.getElementById('aws-savings-plans-ec2instance-enabled') as HTMLInputElement; - expect(ec2Coverage.value).toBe('80'); + expect(ec2Coverage.value).toBe('67'); expect(ec2Enabled.checked).toBe(true); }); diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index a37c1c128..1e49948a1 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -2671,20 +2671,33 @@ export async function loadGlobalSettings(): Promise { updateCollectionScheduleVisibility(); } - if (data.services) { - loadedServiceConfigs = data.services; - for (const svc of data.services) { - const key = `${svc.provider}-${svc.service}`; - const termEl = document.getElementById(`${key}-term`) as HTMLSelectElement | null; - if (termEl) termEl.value = String(svc.term); - const paymentEl = document.getElementById(`${key}-payment`) as HTMLSelectElement | null; - if (paymentEl) paymentEl.value = svc.payment; - // Issue #136: populate per-product SP coverage and enabled fields when - // the card exposes them. Other service cards fall through (IDs absent). - const coverageEl = document.getElementById(`${key}-coverage`) as HTMLInputElement | null; - if (coverageEl) coverageEl.value = String(svc.coverage ?? data.global?.default_coverage ?? 80); - const enabledEl = document.getElementById(`${key}-enabled`) as HTMLInputElement | null; - if (enabledEl) enabledEl.checked = svc.enabled !== false; + const services = data.services ?? []; + loadedServiceConfigs = services; + for (const svc of services) { + const key = `${svc.provider}-${svc.service}`; + const termEl = document.getElementById(`${key}-term`) as HTMLSelectElement | null; + if (termEl) termEl.value = String(svc.term); + const paymentEl = document.getElementById(`${key}-payment`) as HTMLSelectElement | null; + if (paymentEl) paymentEl.value = svc.payment; + } + + // Issue #136: populate per-product SP coverage and enabled fields for every + // SP card the DOM exposes, not just those with a service row. A card whose + // service row is absent from the response must still fall back to + // global.default_coverage (and enabled=true); leaving it at the HTML default + // (80) would persist an incorrect value on the user's next save. Iterate + // SERVICE_FIELDS (the source of truth for card IDs) and overlay any matching + // service-config values keyed by `${provider}-${service}`. + const svcByKey = new Map(services.map(s => [`${s.provider}-${s.service}`, s] as const)); + for (const field of SERVICE_FIELDS) { + const svc = svcByKey.get(`${field.provider}-${field.service}`); + if ('coverageId' in field && field.coverageId) { + const el = byId(field.coverageId); + if (el) el.value = String(svc?.coverage ?? data.global?.default_coverage ?? 80); + } + if ('enabledId' in field && field.enabledId) { + const el = byId(field.enabledId); + if (el) el.checked = svc?.enabled !== false; } }