From 943422b4ca3b0c46aff182b2f48ebc2569def1c9 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 01:07:20 +0000 Subject: [PATCH 1/3] fix(frontend): reject fractional per-Savings-Plan coverage on save The four per-Savings-Plan coverage inputs sit outside the settings form, so native min/max validation never runs for them and the inline hint only paints a message. Save accepted any finite number, so a fractional value like 12.7 passed the backend's 0..100 range check and was stored. Apply the global coverage field's whole-number 0..100 rule to every edited SP card before any PUT is sent, and abort the save with a toast naming the offending card. Untouched and empty cards keep their existing fallback behavior. Refs #318 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/settings.test.ts | 25 ++++++++++++++++++++ frontend/src/settings.ts | 31 +++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts index 0736b4e97..46d34f3be 100644 --- a/frontend/src/__tests__/settings.test.ts +++ b/frontend/src/__tests__/settings.test.ts @@ -1020,6 +1020,31 @@ describe('Settings Module', () => { expect(isUnsavedChanges()).toBe(false); }); + test.each(['12.7', '-40', '500'])('an SP card coverage of %s aborts the save before any PUT (#318)', async (value) => { + await loadWithCoverage50(); + mockShowToast.mockClear(); + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = value; + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(api.updateServiceConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ + kind: 'error', + message: expect.stringContaining('whole number between 0 and 100 (savings-plans-compute)'), + })); + expect(isUnsavedChanges()).toBe(true); + }); + + test.each([['0', 0], ['100', 100]])('an SP card coverage of %s is within range and saved (#318)', async (value, sent) => { + await loadWithCoverage50(); + (document.getElementById('aws-savings-plans-compute-coverage') as HTMLInputElement).value = value; + + await saveGlobalSettings({ preventDefault: jest.fn() } as unknown as Event); + + expect(coverageSentFor('savings-plans-compute')).toBe(sent); + }); + test('a saved service payment edit leaves nothing unsaved (#576)', async () => { await loadWithCoverage50(); (document.getElementById('aws-ec2-payment') as HTMLSelectElement).value = 'partial-upfront'; diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index efbed45af..e0c9c6f3f 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3435,6 +3435,26 @@ 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. + */ +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; + const parsed = Number(raw); + if (Number.isInteger(parsed) && parsed >= 0 && parsed <= 100) continue; + const title = input?.closest('.service-default-card')?.querySelector('h5')?.textContent?.trim(); + bad.push(title || field.service); + } + return bad; +} + /** * Save global settings */ @@ -3519,6 +3539,17 @@ export async function saveGlobalSettings(e: Event): Promise { saveInFlight = false; return; } + // The per-Savings-Plan coverage cards sit outside the settings
, so + // native min/max validation never runs for them and the inline hint only + // paints a message. Apply the global field's whole-number 0..100 rule here, + // before any PUT, so a fractional value like 12.7 is not persisted. + const badCoverageCards = invalidEditedCardCoverages(); + if (badCoverageCards.length > 0) { + showToast({ message: `Target coverage must be a whole number between 0 and 100 (${badCoverageCards.join(', ')})`, kind: 'error' }); + if (saveBtn) saveBtn.disabled = false; + saveInFlight = false; + return; + } const rawOfferingClass = byId('setting-ec2-offering-class')?.value ?? 'convertible'; const offeringClass: 'convertible' | 'standard' = (rawOfferingClass === 'standard') ? 'standard' : 'convertible'; From f67dc4993c947a58187cbee34af33e1ea4d7d184 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 01:07:41 +0000 Subject: [PATCH 2/3] fix(frontend): show marketplace list price, fee and proceeds in cents The marketplace listing dialog rounded the list price, AWS fee and net proceeds to whole dollars independently, so the lines did not add up: a $100.50 listing showed $101 = $12 + $88. Render all three with cents and derive the fee from the rounded list and net totals, so fee + net always equals the list price shown. Refs #318 Co-Authored-By: Claude Opus 5.5 --- .../history-marketplace-sell-button.test.ts | 31 +++++++++++++++++++ frontend/src/history.ts | 14 ++++++--- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/history-marketplace-sell-button.test.ts b/frontend/src/__tests__/history-marketplace-sell-button.test.ts index 6730be160..9870779aa 100644 --- a/frontend/src/__tests__/history-marketplace-sell-button.test.ts +++ b/frontend/src/__tests__/history-marketplace-sell-button.test.ts @@ -77,6 +77,7 @@ jest.mock('../state', () => ({ import * as api from '../api'; import { getCurrentUser } from '../state'; import { confirmDialog } from '../confirmDialog'; +import { formatCurrency } from '../utils'; // Administrators group GUID -- mirrors ADMINISTRATORS_GROUP_ID in // frontend/src/permissions.ts. Without this, isAdmin() returns false and @@ -452,6 +453,36 @@ describe('Marketplace consent modal residual proration (issue #808 follow-up)', expect(text).not.toContain('$585'); }); + // Issue #318: list price, AWS fee and net proceeds were each rounded to whole + // dollars on their own, so a $142.50 listing showed $143 = $17 + $125. Run + // the real formatter and check that the three lines reconcile to the cent. + test('issue #318: list price, fee and net proceeds show cents and add up', async () => { + const realFormat = jest.requireActual('../utils').formatCurrency; + (formatCurrency as jest.Mock).mockImplementation(realFormat); + try { + const PARTWAY = new Date(Date.now() - 2.5 * 30.4375 * 24 * 60 * 60 * 1000).toISOString(); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ + purchase_id: 'ri-cents', offering_class: 'standard', term: 1, timestamp: PARTWAY, + count: 1, upfront_cost: 200, monthly_cost: 0, + })], + }); + await loadHistory(); + document.querySelector('.history-marketplace-sell-btn[data-marketplace-sell-id="ri-cents"]')!.click(); + await Promise.resolve(); + await Promise.resolve(); + + const arg = (confirmDialog as jest.Mock).mock.calls[0][0] as { body: HTMLElement }; + const values = Array.from(arg.body.querySelectorAll('dd')).map(dd => dd.textContent); + // remaining 9 of 12 months: residual 150, list 142.50, net 142.50 * 0.88 = 125.40. + expect(values).toEqual(expect.arrayContaining(['$142.50', '$17.10', '$125.40'])); + expect(values).not.toContain('$143'); + } finally { + (formatCurrency as jest.Mock).mockImplementation((val) => `$${val || 0}`); + } + }); + test('shows default price as unavailable for a no-upfront RI instead of a fabricated price', async () => { const JUST_NOW = new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(); (api.getHistory as jest.Mock).mockResolvedValue({ diff --git a/frontend/src/history.ts b/frontend/src/history.ts index b4c183d44..89593d670 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -1582,11 +1582,17 @@ function wireRowActionHandlers(container: HTMLElement): void { addRow('Resource type', purchase.resource_type || '-'); addRow('Remaining term', remainingMonths === 1 ? '1 month' : `${remainingMonths} months`); if (listPricePerUnit > 0) { + // Show cents, and derive the fee from the two rounded totals, so + // the three lines always reconcile (fee + net = list price). The + // dashboard's whole-dollar default rounded each line on its own, + // e.g. $101 = $12 + $88 for a $100.50 listing (#318). + const listCents = Math.round(listPriceTotal * 100); + const netCents = Math.round(netProceedsTotal * 100); addRow('Default list price', count > 1 - ? `${formatCurrency(listPricePerUnit)}/unit (${formatCurrency(listPriceTotal)} total for ${count} units)` - : formatCurrency(listPriceTotal)); - addRow(`AWS fee (${AWS_MARKETPLACE_FEE_PERCENT}%)`, formatCurrency(listPriceTotal * (AWS_MARKETPLACE_FEE_PERCENT / 100))); - addRow('Estimated net proceeds', formatCurrency(netProceedsTotal)); + ? `${formatCurrency(listPricePerUnit, '$', 2)}/unit (${formatCurrency(listCents / 100, '$', 2)} total for ${count} units)` + : formatCurrency(listCents / 100, '$', 2)); + addRow(`AWS fee (${AWS_MARKETPLACE_FEE_PERCENT}%)`, formatCurrency((listCents - netCents) / 100, '$', 2)); + addRow('Estimated net proceeds', formatCurrency(netCents / 100, '$', 2)); } else { // No default price can be computed (no upfront cost or unknown // term) -- listing will be rejected server-side unless a custom From ec847dfcb3be6300335590c8e61e499da5f19772 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 01:07:41 +0000 Subject: [PATCH 3/3] fix(frontend): mark the upfront as folded into monthly when amortizing With the amortize toggle on, Purchase History and the Approval Queue showed the full upfront next to a Monthly Cost that already includes it, and only the Monthly Cost header disclosed this. Upfront plus 12 times Monthly then double-counted the upfront. When the upfront is spread into the monthly figure, mute the Upfront cell, add an "(in monthly)" note and an explanatory title. Also read the amortize flag once per render instead of once per row. Refs #318 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/history.test.ts | 25 +++++++++++++++ frontend/src/history.ts | 20 ++++++++---- frontend/src/styles/components.css | 12 ++++++++ frontend/tests-e2e/amortized-upfront.spec.ts | 32 ++++++++++++++++++++ 4 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 frontend/tests-e2e/amortized-upfront.spec.ts diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index f2b8ebf97..564e4ba3e 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -33,6 +33,7 @@ jest.mock('../utils', () => ({ if (!str) return ''; return str.replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"').replace(/'/g, '''); }), + amortizedMonthly: jest.requireActual('../utils').amortizedMonthly, populateAccountFilter: jest.fn(() => Promise.resolve()) })); @@ -105,6 +106,30 @@ describe('History Module', () => { expect(queue?.querySelectorAll('td')[8]?.textContent).toBe('--'); }); + // Issue #318: with amortize on, the upfront is already in Monthly Cost, so + // the Upfront cell is muted and labeled to keep it from reading as additive. + test.each([false, true])('upfront folded into monthly with amortization %s is labeled only when on', async (amortize) => { + (getAmortizeUpfront as jest.Mock).mockReturnValue(amortize); + const purchase = { + timestamp: '2026-09-29T00:00:00Z', provider: 'aws', service: 'ec2', resource_type: 'm5.large', + region: 'us-east-1', count: 1, term: 1, upfront_cost: 1200, monthly_cost: 100, estimated_savings: 300, + }; + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ + { ...purchase, purchase_id: 'complete', status: 'completed' }, + { ...purchase, purchase_id: 'pending', status: 'pending' }, + ] }); + await loadHistory(); + const historyCells = document.querySelectorAll('#history-list tr[data-execution-id="complete"] td'); + const queueCells = document.querySelectorAll('#purchases-approval-queue tr[data-execution-id="pending"] td'); + expect(historyCells[9]?.textContent).toBe(amortize ? '$200' : '$100'); + expect(queueCells[7]?.textContent).toBe(amortize ? '$200' : '$100'); + for (const upfront of [historyCells[8], queueCells[8]]) { + expect(upfront?.classList.contains('upfront-in-monthly')).toBe(amortize); + expect(upfront?.textContent).toBe(amortize ? '$1200(in monthly)' : '$1200'); + expect(upfront?.getAttribute('title')).toBe(amortize ? 'Included in the amortized monthly cost' : null); + } + }); + describe('initHistoryDateRange', () => { test('sets default date range to 7 days', () => { initHistoryDateRange(); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 89593d670..5051f8ad3 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -562,6 +562,16 @@ function providerCell(p: HistoryPurchase): string { return `${safeProvider.toUpperCase()}`; } +// upfrontCell renders the Upfront Cost cell. With amortize on, the upfront is +// already spread into the Monthly Cost column, so the cell is muted and +// labeled to stop Upfront + 12 x Monthly being read as additive (#318). +function upfrontCell(p: HistoryPurchase, amortize: boolean): string { + const amount = formatCurrency(p.upfront_cost); + const folded = amortize && p.monthly_cost != null && p.upfront_cost != null && p.upfront_cost > 0 && p.term > 0; + if (!folded) return `${amount}`; + return `${amount}
(in monthly)
`; +} + // canCancelPendingRow returns true when the current session is permitted // to cancel the given pending/notified history row via the session-authed // Cancel button (issue #46). UX gate only — the backend @@ -1215,6 +1225,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { const colFilters = state.getPurchaseHistoryColumnFilters(); const visible = applyPurchaseHistoryColumnFilters(statusFiltered, colFilters); + const amortize = state.getAmortizeUpfront(); const tableRows = visible.map(p => { const statusCell = (() => { const badge = statusBadgeHTML(normalizeStatus(p)); @@ -1229,7 +1240,6 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { })(); const execIdAttr = p.purchase_id ? ` data-execution-id="${escapeHtmlAttr(p.purchase_id)}"` : ''; const planCellContent = renderActionCell(p); - const amortize = state.getAmortizeUpfront(); const rawMonthly = p.monthly_cost != null ? p.monthly_cost : null; const displayMonthly = (rawMonthly != null && amortize) ? (p.upfront_cost == null ? null : amortizedMonthly(rawMonthly, p.upfront_cost, p.term)) @@ -1247,7 +1257,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { ${escapeHtml(p.region)} ${p.count} ${formatTerm(p.term)} - ${formatCurrency(p.upfront_cost)} + ${upfrontCell(p, amortize)} ${monthlyCostCell} ${formatCurrency(p.estimated_savings)} ${planCellContent} @@ -1255,7 +1265,6 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { `; }).join(''); - const amortize = state.getAmortizeUpfront(); const monthlyColHeader = amortize ? 'Monthly Cost (amortized)' : 'Monthly Cost'; const fbtn = (col: PurchaseHistoryColumnId): string => renderHistoryFilterButton( col, PURCHASE_HISTORY_LABELS[col], colFilters[col] != null, @@ -1843,6 +1852,7 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { const colFilters = state.getApprovalQueueColumnFilters(); const visible = applyApprovalQueueColumnFilters(pending, colFilters); + const amortize = state.getAmortizeUpfront(); const rows = visible.map(p => { const actions = renderPendingActionButtons(p); const actionsCell = actions || '-'; @@ -1858,7 +1868,6 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { : '-'; const termCell = p.term ? escapeHtml(formatTerm(p.term)) : '-'; const paymentCell = p.payment ? escapeHtml(p.payment) : '-'; - const amortize = state.getAmortizeUpfront(); const rawMonthly = p.monthly_cost != null ? p.monthly_cost : null; const displayMonthly = (rawMonthly != null && amortize) ? (p.upfront_cost == null ? null : amortizedMonthly(rawMonthly, p.upfront_cost, p.term)) @@ -1877,7 +1886,7 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { ${termCell} ${paymentCell} ${monthlyCostCell} - ${formatCurrency(p.upfront_cost)} + ${upfrontCell(p, amortize)} ${formatCurrency(p.estimated_savings)} ${createdBy} ${actionsCell} @@ -1885,7 +1894,6 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { `; }).join(''); - const amortize = state.getAmortizeUpfront(); const monthlyColHeader = amortize ? 'Monthly Cost (amortized)' : 'Monthly Cost'; // monthlyColHeader is a hardcoded constant string (no user data), so // interpolating it directly into the template is safe. diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index 5d76ec88e..60a922e45 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -1440,6 +1440,18 @@ tr.rec-variant-row:hover td:not(.checkbox-col) { font-weight: 600; } +/* Upfront Cost cell when the amortize toggle folds the upfront into the + * Monthly Cost column (#318): muted so the two are not read as additive. + */ +.upfront-in-monthly { + color: var(--cudly-text-muted); +} + +.upfront-in-monthly-note { + font-size: 0.78rem; + line-height: 1.25; +} + /* Purchase detail status badges */ .status-badge.completed { background: var(--cudly-success-bg); diff --git a/frontend/tests-e2e/amortized-upfront.spec.ts b/frontend/tests-e2e/amortized-upfront.spec.ts new file mode 100644 index 000000000..4309e7263 --- /dev/null +++ b/frontend/tests-e2e/amortized-upfront.spec.ts @@ -0,0 +1,32 @@ +import { test, expect } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; + +// Issue #318: with amortize on, the upfront is already spread into Monthly +// Cost, so the Upfront cell is muted and labeled instead of reading as a +// separate lump sum on top of the amortized monthly figure. +test('amortize toggle labels the upfront as included in the monthly cost', async ({ page }) => { + await seedAuth(page); + await mockApi(page); + const purchase = { + purchase_id: '11111111-1111-1111-1111-111111111111', timestamp: new Date().toISOString(), + provider: 'aws', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', + count: 1, term: 1, upfront_cost: 1200, monthly_cost: 100, estimated_savings: 300, + offering_class: 'convertible', status: 'completed', account_id: 'account-1', + }; + await page.route('**/api/history**', route => route.fulfill({ json: { summary: {}, purchases: [purchase] } })); + + await page.goto('/purchases/history'); + const row = page.locator('#history-list tbody tr').first(); + const upfront = row.locator('td').nth(8); + await expect(upfront).toHaveText('$1,200'); + await expect(upfront).not.toHaveClass(/upfront-in-monthly/); + + await page.locator('#history-amortize-checkbox').check(); + await expect(row.locator('td').nth(9)).toHaveText('$200'); + await expect(upfront).toHaveClass(/upfront-in-monthly/); + await expect(upfront).toHaveAttribute('title', 'Included in the amortized monthly cost'); + await expect(upfront).toContainText('(in monthly)'); + + await page.locator('#history-amortize-checkbox').uncheck(); + await expect(upfront).toHaveText('$1,200'); +});