diff --git a/frontend/src/__tests__/allowed-accounts.test.ts b/frontend/src/__tests__/allowed-accounts.test.ts index f8b34ec8..1b82e8ee 100644 --- a/frontend/src/__tests__/allowed-accounts.test.ts +++ b/frontend/src/__tests__/allowed-accounts.test.ts @@ -52,6 +52,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val: number) => `$${val || 0}`), formatDate: jest.fn((val: string) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years: number) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/approval-queue-column-filters.test.ts b/frontend/src/__tests__/approval-queue-column-filters.test.ts index cf314054..4f08eb44 100644 --- a/frontend/src/__tests__/approval-queue-column-filters.test.ts +++ b/frontend/src/__tests__/approval-queue-column-filters.test.ts @@ -24,6 +24,7 @@ import type { ApprovalQueueColumnFilters } from '../state'; jest.mock('../api', () => ({})); jest.mock('../navigation', () => ({ switchTab: jest.fn() })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((v) => `$${v ?? 0}`), formatDate: jest.fn((v) => v), formatTerm: jest.fn((y) => `${y} Year${y === 1 ? '' : 's'}`), diff --git a/frontend/src/__tests__/dashboard-ownership-950.test.ts b/frontend/src/__tests__/dashboard-ownership-950.test.ts index d4fc01ef..9bf65fdb 100644 --- a/frontend/src/__tests__/dashboard-ownership-950.test.ts +++ b/frontend/src/__tests__/dashboard-ownership-950.test.ts @@ -67,6 +67,7 @@ jest.mock('../state', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), getDateParts: jest.fn(() => ({ day: 15, month: 'Jan' })), escapeHtml: jest.fn((str) => str || ''), diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index a08658d1..1bbd4117 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -103,9 +103,10 @@ jest.mock('../state', () => ({ // Mock utils jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => { const n = typeof val === 'number' ? val : Number(val); - if (val == null || !Number.isFinite(n)) return '--'; + if (val == null || !Number.isFinite(n)) return '—'; return `$${n}`; }), getDateParts: jest.fn(() => ({ day: 15, month: 'Jan' })), @@ -869,11 +870,11 @@ describe('Dashboard Module', () => { expect(summary?.innerHTML).toContain('Active Commitments'); expect(summary?.innerHTML).toContain('Current Coverage'); expect(summary?.innerHTML).toContain('YTD Savings'); - // Savings card must fall back to '--' (not throw, go blank, or fabricate $0). + // Savings card must fall back to '—' (not throw, go blank, or fabricate $0). const savingsCard = summary?.querySelector('.card'); expect(savingsCard?.textContent).toContain('Potential Monthly Savings'); - // When recs fail cellCount=0, the sentinel '--' is shown instead of fabricated $0. - expect(savingsCard?.innerHTML).toContain('--'); + // When recs fail cellCount=0, the sentinel '—' is shown instead of fabricated $0. + expect(savingsCard?.innerHTML).toContain('—'); expect(savingsCard?.innerHTML).not.toContain('$0'); }); @@ -934,10 +935,10 @@ describe('Dashboard Module', () => { expect(summary?.innerHTML).toContain('Current Coverage'); expect(summary?.innerHTML).toContain('YTD Savings'); - // Savings card shows '--' when recs are absent (no fabricated $0). + // Savings card shows '—' when recs are absent (no fabricated $0). const savingsCard = summary?.querySelector('.card'); expect(savingsCard?.textContent).toContain('Potential Monthly Savings'); - expect(savingsCard?.innerHTML).toContain('--'); + expect(savingsCard?.innerHTML).toContain('—'); expect(savingsCard?.innerHTML).not.toContain('$0'); }); @@ -963,9 +964,9 @@ describe('Dashboard Module', () => { const summary = document.getElementById('summary'); expect(summary?.innerHTML).toContain('Active Commitments'); - // Savings card shows '--' (no fabricated $0 when recs are absent). + // Savings card shows '—' (no fabricated $0 when recs are absent). const savingsCard = summary?.querySelector('.card'); - expect(savingsCard?.innerHTML).toContain('--'); + expect(savingsCard?.innerHTML).toContain('—'); expect(savingsCard?.innerHTML).not.toContain('$0'); }); @@ -2146,12 +2147,12 @@ describe('Dashboard Module', () => { }); }); - // H-3 regression: absent API coverage/count fields must render '--', not '$0'/'80%'/'0'. + // H-3 regression: absent API coverage/count fields must render '—', not '$0'/'80%'/'0'. // Pre-fix: `data.current_coverage || 0` fabricated '0%'; `data.target_coverage || 80` // fabricated '80%'; `data.active_commitments || 0` fabricated '0'; the savings catch // block fell back to formatCurrency(0). These tests must FAIL on pre-fix code. - describe('H-3: absent dashboard KPI fields render -- sentinel, not fabricated values', () => { - test('absent target_coverage shows -- not hardcoded 80%', async () => { + describe('H-3: absent dashboard KPI fields render the em-dash sentinel, not fabricated values', () => { + test('absent target_coverage shows an em dash, not hardcoded 80%', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ // target_coverage deliberately absent total_recommendations: 1, @@ -2169,11 +2170,11 @@ describe('Dashboard Module', () => { // Must not show the hardcoded 80% fallback expect(summary?.textContent).not.toContain('Target: 80.0%'); expect(summary?.textContent).not.toContain('Target: 80%'); - // Must show the '--' sentinel for absent target - expect(summary?.textContent).toContain('Target: --'); + // Must show the '—' sentinel for absent target + expect(summary?.textContent).toContain('Target: —'); }); - test('absent current_coverage shows -- not fabricated 0%', async () => { + test('absent current_coverage shows an em dash, not fabricated 0%', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ // current_coverage deliberately absent total_recommendations: 2, @@ -2195,11 +2196,11 @@ describe('Dashboard Module', () => { ); expect(coverageTile).toBeDefined(); const valueEl = coverageTile?.querySelector('.kpi-tile-value'); - // Must show '--' (not a fabricated '0.0%') when current_coverage is absent - expect(valueEl?.textContent).toBe('--'); + // Must show the em dash (not a fabricated '0.0%') when current_coverage is absent + expect(valueEl?.textContent).toBe('—'); }); - test('absent active_commitments shows -- not fabricated 0', async () => { + test('absent active_commitments shows an em dash, not fabricated 0', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ total_recommendations: 1, // active_commitments deliberately absent @@ -2220,12 +2221,12 @@ describe('Dashboard Module', () => { t.textContent?.includes('Active Commitments') ); expect(commitmentsTile).toBeDefined(); - // Must not show fabricated '0'; must show '--' + // Must not show fabricated '0'; must show '—' const valueEl = commitmentsTile?.querySelector('.kpi-tile-value'); - expect(valueEl?.textContent).toBe('--'); + expect(valueEl?.textContent).toBe('—'); }); - test('savings range catch block shows -- not $0 when computation throws', async () => { + test('savings range catch block shows an em dash, not $0 when computation throws', async () => { // Force the groupRecsByCell mock to throw so the catch block fires. mockGroupRecsByCell.mockImplementationOnce(() => { throw new Error('compute error'); }); (api.getDashboardSummary as jest.Mock).mockResolvedValue({ @@ -2248,8 +2249,10 @@ describe('Dashboard Module', () => { ); expect(savingsTile).toBeDefined(); const valueEl = savingsTile?.querySelector('.kpi-tile-value'); - // Must show '--' (not '$0') when the savings computation fails - expect(valueEl?.textContent).toBe('--'); + // Must show the em dash (not '$0') when the savings computation fails, + // and not in savings green: it is not a saving (issue #739). + expect(valueEl?.textContent).toBe('—'); + expect(valueEl?.classList.contains('savings')).toBe(false); }); }); }); diff --git a/frontend/src/__tests__/four-eyes-approval.test.ts b/frontend/src/__tests__/four-eyes-approval.test.ts index 86b62cb2..7b322ea3 100644 --- a/frontend/src/__tests__/four-eyes-approval.test.ts +++ b/frontend/src/__tests__/four-eyes-approval.test.ts @@ -22,6 +22,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-approval-queue.test.ts b/frontend/src/__tests__/history-approval-queue.test.ts index eb05f708..8168084a 100644 --- a/frontend/src/__tests__/history-approval-queue.test.ts +++ b/frontend/src/__tests__/history-approval-queue.test.ts @@ -41,6 +41,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-approve-button.test.ts b/frontend/src/__tests__/history-approve-button.test.ts index 437bb00e..8448ea52 100644 --- a/frontend/src/__tests__/history-approve-button.test.ts +++ b/frontend/src/__tests__/history-approve-button.test.ts @@ -35,6 +35,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-cancel-button.test.ts b/frontend/src/__tests__/history-cancel-button.test.ts index 7933e959..42df7156 100644 --- a/frontend/src/__tests__/history-cancel-button.test.ts +++ b/frontend/src/__tests__/history-cancel-button.test.ts @@ -29,6 +29,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-cancel-permissions.test.ts b/frontend/src/__tests__/history-cancel-permissions.test.ts index 24e4e45f..9a100a49 100644 --- a/frontend/src/__tests__/history-cancel-permissions.test.ts +++ b/frontend/src/__tests__/history-cancel-permissions.test.ts @@ -31,6 +31,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-column-filters.test.ts b/frontend/src/__tests__/history-column-filters.test.ts index 98cf584c..d9102d05 100644 --- a/frontend/src/__tests__/history-column-filters.test.ts +++ b/frontend/src/__tests__/history-column-filters.test.ts @@ -28,6 +28,7 @@ import type { PurchaseHistoryColumnFilters } from '../state'; jest.mock('../api', () => ({})); jest.mock('../navigation', () => ({ switchTab: jest.fn() })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((v) => `$${v ?? 0}`), formatDate: jest.fn((v) => v), formatTerm: jest.fn((y) => `${y} Year${y === 1 ? '' : 's'}`), diff --git a/frontend/src/__tests__/history-marketplace-sell-button.test.ts b/frontend/src/__tests__/history-marketplace-sell-button.test.ts index 9870779a..500418a1 100644 --- a/frontend/src/__tests__/history-marketplace-sell-button.test.ts +++ b/frontend/src/__tests__/history-marketplace-sell-button.test.ts @@ -38,6 +38,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index 7ed3495d..b50b702a 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -38,6 +38,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-revoke-button.test.ts b/frontend/src/__tests__/history-revoke-button.test.ts index 2e8bb2a2..35800dbd 100644 --- a/frontend/src/__tests__/history-revoke-button.test.ts +++ b/frontend/src/__tests__/history-revoke-button.test.ts @@ -36,6 +36,7 @@ jest.mock('../navigation', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history-truncation-notice.test.ts b/frontend/src/__tests__/history-truncation-notice.test.ts index c892d75a..1687ea47 100644 --- a/frontend/src/__tests__/history-truncation-notice.test.ts +++ b/frontend/src/__tests__/history-truncation-notice.test.ts @@ -13,6 +13,7 @@ jest.mock('../api', () => ({ })); jest.mock('../navigation', () => ({ switchTab: jest.fn() })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatDate: jest.fn((val) => (val ? new Date(val).toLocaleDateString() : '')), formatTerm: jest.fn((years) => (years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`)), diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index 96e03b55..4a428d8f 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -21,8 +21,9 @@ jest.mock('../permissions', () => ({ })); jest.mock('../utils', () => ({ - // Mirrors the real formatCurrency behaviour: null/undefined/NaN -> '--', numbers -> '$' - formatCurrency: jest.fn((val) => (val === null || val === undefined || isNaN(val)) ? '--' : `$${val}`), + NO_VALUE: '\u2014', + // Mirrors the real formatCurrency behaviour: null/undefined/NaN -> '—', numbers -> '$' + formatCurrency: jest.fn((val) => (val === null || val === undefined || isNaN(val)) ? '—' : `$${val}`), formatDate: jest.fn((val) => val ? new Date(val).toLocaleDateString() : ''), formatTerm: jest.fn((years) => years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`), escapeHtml: jest.fn((str) => str || ''), @@ -100,10 +101,15 @@ describe('History Module', () => { await loadHistory(); const history = document.querySelector('#history-list tr[data-execution-id="complete"]'); const queue = document.querySelector('#purchases-approval-queue tr[data-execution-id="pending"]'); - expect(history?.querySelectorAll('td')[8]?.textContent).toBe('--'); - expect(history?.querySelectorAll('td')[9]?.textContent).toBe(amortize ? '-' : '$50'); - expect(queue?.querySelectorAll('td')[7]?.textContent).toBe(amortize ? '-' : '$50'); - expect(queue?.querySelectorAll('td')[8]?.textContent).toBe('--'); + expect(history?.querySelectorAll('td')[8]?.textContent).toBe('—'); + expect(history?.querySelectorAll('td')[9]?.textContent).toBe(amortize ? '—' : '$50'); + expect(queue?.querySelectorAll('td')[7]?.textContent).toBe(amortize ? '—' : '$50'); + expect(queue?.querySelectorAll('td')[8]?.textContent).toBe('—'); + // Count and money columns right-align (issue #739). + const numCols = (row: Element | null): number[] => Array.from(row?.querySelectorAll('td') ?? []) + .flatMap((td, i) => (td.classList.contains('num') ? [i] : [])); + expect(numCols(history)).toEqual([6, 8, 9, 10]); + expect(numCols(queue)).toEqual([4, 7, 8, 9]); }); // Issue #318: with amortize on, the upfront is already in Monthly Cost, so @@ -674,12 +680,12 @@ describe('History Module', () => { }); }); - // H-2 regression: absent API summary must render '--' on KPI cards, not '$0'. + // H-2 regression: absent API summary must render '—' on KPI cards, not '$0'. // Pre-fix: `data.summary || {}` passed an empty HistorySummary to // renderHistorySummary, and `?? 0` coercions made money fields render as // '$0' (or the mock's '$0'). These tests must FAIL on the pre-fix code. - describe('H-2: absent summary renders -- sentinel on KPI cards', () => { - test('renderHistorySummary: absent summary (undefined) shows -- on all money cards (loadHistory)', async () => { + describe('H-2: absent summary renders the em-dash sentinel on KPI cards', () => { + test('renderHistorySummary: absent summary (undefined) shows an em dash on all money cards (loadHistory)', async () => { (api.getHistory as jest.Mock).mockResolvedValue({ // summary field deliberately absent purchases: [] @@ -692,14 +698,14 @@ describe('History Module', () => { expect(summary?.innerHTML).toContain('Monthly Savings'); expect(summary?.innerHTML).toContain('Annual Savings'); // Must show the absent sentinel, not a fabricated '$0' - expect(summary?.innerHTML).toContain('--'); - // The mock formatCurrency returns '$0' for 0 and '--' for null; + expect(summary?.innerHTML).toContain('—'); + // The mock formatCurrency returns '$0' for 0 and an em dash for null; // verify no '$0' slips through for the money cards. expect(summary?.textContent).not.toMatch(/Total Upfront Spent.*\$0/s); expect(summary?.textContent).not.toMatch(/Monthly Savings.*\$0/s); }); - test('renderHistorySummary: absent total_upfront renders -- not $0 (viewPlanHistory)', async () => { + test('renderHistorySummary: absent total_upfront renders an em dash, not $0 (viewPlanHistory)', async () => { (api.getHistory as jest.Mock).mockResolvedValue({ summary: { total_purchases: 3, @@ -713,16 +719,16 @@ describe('History Module', () => { await viewPlanHistory('plan-abc'); const summary = document.getElementById('history-summary'); - // total_purchases is present; money fields that are absent must be '--' + // total_purchases is present; money fields that are absent must be an em dash expect(summary?.textContent).toContain('3'); // formatCurrency must be called with null for the absent total_upfront - // field and must produce '--', not '$0'. This assertion fails if the + // field and must produce an em dash, not '$0'. This assertion fails if the // production code regresses to passing 0 or a fabricated value. const { formatCurrency } = jest.requireMock('../utils') as { formatCurrency: jest.Mock }; expect(formatCurrency).toHaveBeenCalledWith(null); - // The rendered card must show '--', not a dollar amount. + // The rendered card must show an em dash, not a dollar amount. expect(summary?.innerHTML).toContain('Total Upfront Spent'); - expect(summary?.innerHTML).toContain('

--

'); + expect(summary?.innerHTML).toContain('

—

'); expect(summary?.innerHTML).not.toContain('

$0

'); }); @@ -771,14 +777,16 @@ describe('History Module', () => { await loadHistory(); const summary = document.getElementById('history-summary'); - // All four cards must still render (just with '--' values). + // All four cards must still render (just with em-dash values). expect(summary?.innerHTML).toContain('Total Purchases'); expect(summary?.innerHTML).toContain('Total Upfront Spent'); expect(summary?.innerHTML).toContain('Monthly Savings'); expect(summary?.innerHTML).toContain('Annual Savings'); - // The unknown-state banner always shows '--' for every value. - const allDashes = (summary?.innerHTML.match(/--/g) || []).length; + // The unknown-state banner shows the em dash for every value, and an + // absent figure is not styled as a saving (issue #739). + const allDashes = (summary?.innerHTML.match(/—/g) || []).length; expect(allDashes).toBeGreaterThanOrEqual(4); + expect(summary?.querySelector('.value.savings')).toBeNull(); }); }); }); diff --git a/frontend/src/__tests__/money-format.test.ts b/frontend/src/__tests__/money-format.test.ts new file mode 100644 index 00000000..fc281aa4 --- /dev/null +++ b/frontend/src/__tests__/money-format.test.ts @@ -0,0 +1,23 @@ +/** + * Issue #739: money goes through one formatter. The period-scaled cost + * cells used to print `$${x.toFixed(n)}`, so a yearly figure read + * "$123456" with no separators, and absent values used three different + * placeholders. + */ +import { formatCostForPeriod } from '../recommendations'; +import { formatCurrency, NO_VALUE } from '../utils'; + +describe('money formatting (issue #739)', () => { + it('uses one em-dash placeholder for absent values', () => { + expect(NO_VALUE).toBe('—'); + expect(formatCurrency(null)).toBe(NO_VALUE); + expect(formatCostForPeriod(null, 'yearly')).toBe(NO_VALUE); + }); + + it('groups thousands in every cost period', () => { + expect(formatCostForPeriod(10288, 'yearly')).toBe('$123,456'); + expect(formatCostForPeriod(100000, 'daily')).toBe('$3,333.33'); + expect(formatCostForPeriod(2000000, 'hourly')).toBe('$2,777.7778'); + expect(formatCostForPeriod(1234.4, 'monthly')).toBe('$1,234'); + }); +}); diff --git a/frontend/src/__tests__/plans-range-validation.test.ts b/frontend/src/__tests__/plans-range-validation.test.ts index 350d85fa..049a064e 100644 --- a/frontend/src/__tests__/plans-range-validation.test.ts +++ b/frontend/src/__tests__/plans-range-validation.test.ts @@ -72,6 +72,7 @@ jest.mock('../confirmDialog', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatDate: jest.fn((v) => v ?? ''), formatTerm: jest.fn((y) => `${y}yr`), formatRampSchedule: jest.fn((v) => v ?? ''), diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index eaf60718..b09adeed 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -102,6 +102,7 @@ jest.mock('../confirmDialog', () => ({ // Mock utils jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatDate: jest.fn((val) => val ? new Date(val).toLocaleDateString() : ''), formatTerm: jest.fn((years) => years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`), formatRampSchedule: jest.fn((val) => val || 'Unknown'), @@ -251,8 +252,8 @@ describe('Plans Module', () => { test.each([ [{ current_step: 2, total_steps: 4 }, '2/4 steps'], - [{ current_step: 0, total_steps: 0 }, '--'], - [{ current_step: 0 }, '--'], + [{ current_step: 0, total_steps: 0 }, '\u2014'], + [{ current_step: 0 }, '\u2014'], ])('ramp progress %j renders %s instead of a fabricated 0/1 (#136)', async (steps, expected) => { (api.getPlans as jest.Mock).mockResolvedValue({ plans: [{ @@ -3102,7 +3103,7 @@ describe('Plans Module', () => { ramp_schedule: { type: 'immediate', percent_per_step: 100, step_interval_days: 0, current_step: 0, total_steps: 1 }, }; - test('absent provider in services renders -- not aws in plan card', async () => { + test('absent provider in services renders an em dash, not aws in plan card', async () => { const mockBadge = (jest.requireMock('../utils') as { providerBadgeHtml: jest.Mock }).providerBadgeHtml; mockBadge.mockClear(); @@ -3119,7 +3120,7 @@ describe('Plans Module', () => { expect(mockBadge).toHaveBeenCalledWith(null); }); - test('absent term in services renders -- not 3yr in plan card', async () => { + test('absent term in services renders an em dash, not 3yr in plan card', async () => { (api.getPlans as jest.Mock).mockResolvedValue({ plans: [{ ...planBase, services: { ec2: { provider: 'aws', service: 'ec2', coverage: 80 } } }] }); @@ -3128,12 +3129,12 @@ describe('Plans Module', () => { await loadPlans(); const list = document.getElementById('plans-list'); - // Pre-fix rendered '3 Years' for a missing term; post-fix renders '--' + // Pre-fix rendered '3 Years' for a missing term; post-fix renders '—' expect(list?.textContent).not.toContain('3 Years'); - expect(list?.textContent).toContain('--'); + expect(list?.textContent).toContain('—'); }); - test('absent coverage in services renders -- not 80% in plan card', async () => { + test('absent coverage in services renders an em dash, not 80% in plan card', async () => { (api.getPlans as jest.Mock).mockResolvedValue({ plans: [{ ...planBase, services: { ec2: { provider: 'aws', service: 'ec2', term: 1 } } }] }); @@ -3142,12 +3143,12 @@ describe('Plans Module', () => { await loadPlans(); const list = document.getElementById('plans-list'); - // Pre-fix rendered '80%' for a missing coverage; post-fix renders '--' + // Pre-fix rendered '80%' for a missing coverage; post-fix renders '—' expect(list?.textContent).not.toContain('80%'); - expect(list?.textContent).toContain('--'); + expect(list?.textContent).toContain('—'); }); - test('empty services object renders all -- fields in plan card', async () => { + test('empty services object renders all em-dash fields in plan card', async () => { (api.getPlans as jest.Mock).mockResolvedValue({ plans: [{ ...planBase, services: {} }] }); @@ -3157,7 +3158,7 @@ describe('Plans Module', () => { const list = document.getElementById('plans-list'); // Pre-fix: all-zero fallback { provider:'aws', term:3, coverage:80 } - // Post-fix: all '--' for missing data + // Post-fix: all '—' for missing data expect(list?.textContent).not.toContain('80%'); expect(list?.textContent).not.toContain('3 Years'); }); diff --git a/frontend/src/__tests__/purchases-placeholders-181.test.ts b/frontend/src/__tests__/purchases-placeholders-181.test.ts index bd8718f9..f7964f7a 100644 --- a/frontend/src/__tests__/purchases-placeholders-181.test.ts +++ b/frontend/src/__tests__/purchases-placeholders-181.test.ts @@ -146,7 +146,7 @@ describe('Purchases tables never render impossible values (#181)', () => { await loadHistory(); const row = cells('#history-list', 'exec-empty'); // Status, Date, Provider, Service, Type, Region, Count, Term, Upfront, Monthly, Savings, Plan - expect(row.slice(2, 11)).toEqual(['-', '-', 'No recommendations attached', '-', '-', '-', '-', '-', '-']); + expect(row.slice(2, 11)).toEqual(['—', '—', 'No recommendations attached', '—', '—', '—', '—', '—', '—']); const text = row.join(' '); expect(text).not.toMatch(/0 Years|0 commitment\(s\)|Multiple|\$0/); }); @@ -155,14 +155,14 @@ describe('Purchases tables never render impossible values (#181)', () => { await loadHistory(); const row = cells('#purchases-approval-queue', 'exec-empty'); // Date, Account, Provider, Service, Count, Term, Payment, Monthly, Upfront, Savings, Created by, Actions - expect(row.slice(2, 10)).toEqual(['-', 'No recommendations attached', '-', '-', '-', '-', '-', '-']); + expect(row.slice(2, 10)).toEqual(['—', 'No recommendations attached', '—', '—', '—', '—', '—', '—']); }); test('a real mixed-term basket keeps its values but shows a dash, not "0 Years", for the term', async () => { await loadHistory(); const row = cells('#history-list', 'exec-mixed'); expect(row[6]).toBe('2'); - expect(row[7]).toBe('-'); + expect(row[7]).toBe('—'); expect(row[8]).toBe('$1,200'); expect(row.join(' ')).not.toContain('0 Years'); }); diff --git a/frontend/src/__tests__/recommendations-enabled-providers.test.ts b/frontend/src/__tests__/recommendations-enabled-providers.test.ts index cd1680f8..a79a427f 100644 --- a/frontend/src/__tests__/recommendations-enabled-providers.test.ts +++ b/frontend/src/__tests__/recommendations-enabled-providers.test.ts @@ -71,6 +71,7 @@ jest.mock('../state', () => ({ })); jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', formatCurrency: jest.fn((val) => `$${val || 0}`), formatTerm: jest.fn((years) => years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`), escapeHtml: jest.fn((str) => str || ''), diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 534e7899..88cbe12f 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -99,7 +99,12 @@ jest.mock('../state', () => ({ // Mock utils jest.mock('../utils', () => ({ - formatCurrency: jest.fn((val) => `$${val || 0}`), + // Explicit digits (the non-monthly cost periods) render fixed decimals; + // null/undefined render the shared NO_VALUE placeholder like the real one. + formatCurrency: jest.fn((val, _currency = '$', digits?: number) => ( + val === null || val === undefined ? '\u2014' : digits ? `$${Number(val).toFixed(digits)}` : `$${val || 0}` + )), + NO_VALUE: '\u2014', formatTerm: jest.fn((years) => years == null ? '' : `${years} Year${years === 1 ? '' : 's'}`), escapeHtml: jest.fn((str) => str || ''), populateAccountFilter: jest.fn(() => Promise.resolve()), diff --git a/frontend/src/__tests__/trends.test.ts b/frontend/src/__tests__/trends.test.ts index 74fd4eb1..b3aaba1d 100644 --- a/frontend/src/__tests__/trends.test.ts +++ b/frontend/src/__tests__/trends.test.ts @@ -153,7 +153,7 @@ describe('loadTrends', () => { expect(monthly).toHaveLength(6); expect(monthly[5]).toEqual([monthLabel(latest), '$400', '80.0%']); expect(monthly[4]).toEqual([monthLabel(previous), '$400', '70.0%']); - expect(monthly[0]).toEqual([monthLabel(keys[0]!), '--', 'N/A']); + expect(monthly[0]).toEqual([monthLabel(keys[0]!), '\u2014', 'N/A']); expect(cells('trends-provider-table')).toEqual([ ['AWS', 'ec2', '$350', '75.0%'], @@ -185,7 +185,7 @@ describe('loadTrends', () => { expect((api.getAnalyticsTrends as jest.Mock).mock.calls[0][0].provider).toBe('gcp'); expect(cells('trends-provider-table')).toEqual([['GCP', 'compute', '$100', 'N/A']]); expect(cells('trends-monthly-table')[4]).toEqual([monthLabel(previous), '$100', 'N/A']); - expect(cells('trends-monthly-table')[5]).toEqual([monthLabel(latest), '--', 'N/A']); + expect(cells('trends-monthly-table')[5]).toEqual([monthLabel(latest), '\u2014', 'N/A']); }); test('period select drives the window length', async () => { diff --git a/frontend/src/__tests__/utils.test.ts b/frontend/src/__tests__/utils.test.ts index 58b4759c..572235f8 100644 --- a/frontend/src/__tests__/utils.test.ts +++ b/frontend/src/__tests__/utils.test.ts @@ -36,14 +36,14 @@ describe('formatCurrency', () => { }); test('handles null and undefined with distinct absent marker (11-N2)', () => { - // Absent/non-finite values now render as '--' to distinguish missing data + // Absent/non-finite values now render as '—' to distinguish missing data // from a real $0 balance (finding 11-N2, feedback_nullable_not_zero). - expect(formatCurrency(null as unknown as number)).toBe('--'); - expect(formatCurrency(undefined as unknown as number)).toBe('--'); + expect(formatCurrency(null as unknown as number)).toBe('—'); + expect(formatCurrency(undefined as unknown as number)).toBe('—'); }); test('handles NaN with distinct absent marker (11-N2)', () => { - expect(formatCurrency(NaN)).toBe('--'); + expect(formatCurrency(NaN)).toBe('—'); }); test('still renders real zero as $0', () => { @@ -641,20 +641,20 @@ describe('jsonClone (explicit JSON-serialisation variant)', () => { // Regression tests for finding 11-N2: formatCurrency distinguishes absent from $0. // --------------------------------------------------------------------------- describe('formatCurrency (11-N2: absent vs real zero)', () => { - test('null returns -- not $0', () => { - expect(formatCurrency(null as unknown as number)).toBe('--'); + test('null returns an em dash, not $0', () => { + expect(formatCurrency(null as unknown as number)).toBe('—'); }); - test('undefined returns -- not $0', () => { - expect(formatCurrency(undefined as unknown as number)).toBe('--'); + test('undefined returns an em dash, not $0', () => { + expect(formatCurrency(undefined as unknown as number)).toBe('—'); }); - test('NaN returns -- not $0', () => { - expect(formatCurrency(NaN)).toBe('--'); + test('NaN returns an em dash, not $0', () => { + expect(formatCurrency(NaN)).toBe('—'); }); - test('Infinity returns -- not $Infinity', () => { - expect(formatCurrency(Infinity)).toBe('--'); + test('Infinity returns an em dash, not $Infinity', () => { + expect(formatCurrency(Infinity)).toBe('—'); }); test('real zero still renders as $0', () => { diff --git a/frontend/src/approval-details.ts b/frontend/src/approval-details.ts index 1f6e97bb..93a7d992 100644 --- a/frontend/src/approval-details.ts +++ b/frontend/src/approval-details.ts @@ -92,8 +92,8 @@ function renderApprovalDetailsHeader(details: PurchaseDetails, recs: Recommendat } const totalAnnual = totalMonthly * 12; - // M-3: show '--' for absent total_upfront_cost rather than $0, which would - // falsely imply no upfront charge applies. formatCurrency handles null -> '--'. + // M-3: show NO_VALUE for absent total_upfront_cost rather than $0, which would + // falsely imply no upfront charge applies. formatCurrency handles null -> NO_VALUE. const upfront = details.total_upfront_cost ?? null; // estimated_savings on the response is monthly; aggregating the // per-rec savings field above is identical math but lets the diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index f9a97db5..e9d9eca6 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -5,7 +5,7 @@ import { Chart, registerables } from 'chart.js'; import * as api from './api'; import * as state from './state'; -import { formatCurrency, getDateParts, providerBadgeClass } from './utils'; +import { formatCurrency, getDateParts, NO_VALUE, providerBadgeClass } from './utils'; import type { DashboardSummary, UpcomingPurchase, ServiceSavings, LocalRecommendation } from './types'; import type { SavingsDataPoint } from './api'; import { showToast } from './toast'; @@ -228,22 +228,22 @@ function renderDashboardSummary(data: DashboardSummary, recs: readonly LocalReco // min/max savings (best and worst variant per physical resource cell), // avoiding the ~6x inflation of summing every variant of every cell that // the flat summary.potential_monthly_savings carries. - // Falls back to '--' when recs is empty or fetch failed; never fabricates $0 + // Falls back to NO_VALUE (an em dash) when recs is empty or fetch failed; never fabricates $0 // for an absent/failed computation (finding H-3: no silent $0 fallback). // Soft-fail (#304): wrap in try/catch so an unexpected non-iterable shape // that slips past the Array.isArray guard in loadDashboard (e.g. if // groupRecsByCell is called from a path that bypasses the guard) cannot - // blank the entire dashboard -- the savings card shows '--' instead. + // blank the entire dashboard -- the savings card shows NO_VALUE instead. let savingsDisplay: string; try { const groups = groupRecsByCell(recs); const range = pageLevelRange(groups); savingsDisplay = range.cellCount > 0 ? formatSavingsRange(range.savingsMin, range.savingsMax) - : '--'; + : NO_VALUE; } catch (recErr) { console.warn('Dashboard: failed to compute savings range from recommendations:', recErr); - savingsDisplay = '--'; + savingsDisplay = NO_VALUE; } // When no recommendations and no commitments exist, "100% coverage" is @@ -251,7 +251,7 @@ function renderDashboardSummary(data: DashboardSummary, recs: readonly LocalReco const nothingTracked = !data.total_recommendations && !data.active_commitments; // #978: round to 1 decimal place to match sibling coverage formatting (inventory.ts) // and prevent overflow at narrower screen widths. - // H-3: use null-safe checks so absent coverage/target are shown as '--'/'--' + // H-3: use null-safe checks so absent coverage/target are shown as NO_VALUE // rather than fabricating '0%'/'80%'. The hardcoded 80 default is removed // because target_coverage is a user-configured value from the API -- the // frontend must not substitute a business default when the field is absent. @@ -259,10 +259,10 @@ function renderDashboardSummary(data: DashboardSummary, recs: readonly LocalReco const targetCoverage = data.target_coverage ?? null; const coverageValue = nothingTracked ? '—' - : currentCoverage !== null ? `${currentCoverage.toFixed(1)}%` : '--'; + : currentCoverage !== null ? `${currentCoverage.toFixed(1)}%` : NO_VALUE; const coverageDetail = nothingTracked ? 'No services tracked' - : targetCoverage !== null ? `Target: ${targetCoverage.toFixed(1)}%` : 'Target: --'; + : targetCoverage !== null ? `Target: ${targetCoverage.toFixed(1)}%` : `Target: ${NO_VALUE}`; // Render KPI tiles via DOM construction (textContent / appendChild) // rather than an innerHTML template literal, per the issue #340 plan's @@ -277,8 +277,8 @@ function renderDashboardSummary(data: DashboardSummary, recs: readonly LocalReco detail: string; }> = [ { kpi: 'savings', title: 'Potential Monthly Savings', value: savingsDisplay, valueSavings: true, - detail: `${data.total_recommendations ?? '--'} recommendations` }, - { kpi: 'commitments', title: 'Active Commitments', value: data.active_commitments != null ? String(data.active_commitments) : '--', + detail: `${data.total_recommendations ?? NO_VALUE} recommendations` }, + { kpi: 'commitments', title: 'Active Commitments', value: data.active_commitments != null ? String(data.active_commitments) : NO_VALUE, detail: `${formatCurrency(data.committed_monthly)}/mo committed` }, { kpi: 'coverage', title: 'Current Coverage', value: coverageValue, detail: coverageDetail }, { kpi: 'ytd', title: 'YTD Savings', value: formatCurrency(data.ytd_savings), valueSavings: true, @@ -292,7 +292,9 @@ function renderDashboardSummary(data: DashboardSummary, recs: readonly LocalReco h3.textContent = t.title; const valueP = document.createElement('p'); valueP.classList.add('value', 'kpi-tile-value'); - if (t.valueSavings) valueP.classList.add('savings'); + // An absent figure is not a saving: keep the placeholder neutral rather + // than green (issue #739). + if (t.valueSavings && t.value !== NO_VALUE) valueP.classList.add('savings'); valueP.textContent = t.value; const detailP = document.createElement('p'); detailP.classList.add('detail', 'kpi-tile-detail'); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index b270ff02..0739cb5a 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -4,7 +4,7 @@ import * as api from './api'; import * as state from './state'; -import { formatCurrency, formatDate, formatTerm, escapeHtml, escapeHtmlAttr, amortizedMonthly } from './utils'; +import { formatCurrency, formatDate, formatTerm, escapeHtml, escapeHtmlAttr, amortizedMonthly, NO_VALUE } from './utils'; import type { HistoryResponse, HistorySummary, HistoryPurchase } from './types'; import type { RevokeQuote } from './api'; import { switchTab } from './navigation'; @@ -511,19 +511,19 @@ function renderHistorySummary(summary: HistorySummary | null): void { container.innerHTML = `

Total Purchases

-

--

+

${NO_VALUE}

Total Upfront Spent

-

--

+

${NO_VALUE}

Monthly Savings

-

--

+

${NO_VALUE}

Annual Savings

-

--

+

${NO_VALUE}

`; return; @@ -533,7 +533,7 @@ function renderHistorySummary(summary: HistorySummary | null): void { // summary renders sensibly against an older API deploy that hasn't shipped // the new counters yet. const total = summary.total_purchases ?? null; - const totalDisplay = total !== null ? String(total) : '--'; + const totalDisplay = total !== null ? String(total) : NO_VALUE; const completed = summary.total_completed ?? total; const pending = summary.total_pending ?? 0; const detail = (total !== null && pending > 0) @@ -649,7 +649,7 @@ function buildStatusChipRowHTML(purchases: HistoryPurchase[], active: StatusFilt `; } -const MUTED_DASH = '-'; +const MUTED_DASH = `${NO_VALUE}`; const NO_RECS_HTML = `${NO_RECOMMENDATIONS_LABEL}`; // #181: a row with no commitments has placeholder count/term/costs (0, "0 @@ -673,8 +673,8 @@ function providerCell(p: HistoryPurchase): string { 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)
`; + if (!folded) return `${amount}`; + return `${amount}
(in monthly)
`; } // canCancelPendingRow returns true when the current session is permitted @@ -1248,7 +1248,7 @@ function purchaseHistoryDisplayLabel( if (column === 'term') { // Match the cell: a 0 term renders as a dash, never "0 Years" (#181). const n = Number(value); - return isRealTerm(n) ? formatTerm(n) : Number.isFinite(n) ? '-' : value; + return isRealTerm(n) ? formatTerm(n) : Number.isFinite(n) ? NO_VALUE : value; } return value; } @@ -1385,7 +1385,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { : rawMonthly; const monthlyCostCell = displayMonthly != null ? formatCurrency(displayMonthly) - : '-'; + : `${NO_VALUE}`; const empty = hasNoCommitments(p); const dashOr = (html: string): string => (empty ? MUTED_DASH : html); return ` @@ -1396,11 +1396,11 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { ${dashOr(escapeHtml(p.service))} ${empty ? NO_RECS_HTML : escapeHtml(p.resource_type)} ${dashOr(escapeHtml(p.region))} - ${dashOr(escapeHtml(String(p.count ?? '')))} + ${dashOr(escapeHtml(String(p.count ?? '')))} ${termCell(p)} - ${empty ? `${MUTED_DASH}` : upfrontCell(p, amortize)} - ${dashOr(monthlyCostCell)} - ${dashOr(formatCurrency(p.estimated_savings))} + ${empty ? `${MUTED_DASH}` : upfrontCell(p, amortize)} + ${dashOr(monthlyCostCell)} + ${dashOr(formatCurrency(p.estimated_savings))} ${planCellContent} `; @@ -1421,11 +1421,11 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { Service${fbtn('service')} Type${fbtn('resource_type')} Region${fbtn('region')} - Count${fbtn('count')} + Count${fbtn('count')} Term${fbtn('term')} - Upfront Cost${fbtn('upfront_cost')} - ${escapeHtml(monthlyColHeader)} - Monthly Savings${fbtn('savings')} + Upfront Cost${fbtn('upfront_cost')} + ${escapeHtml(monthlyColHeader)} + Monthly Savings${fbtn('savings')} Plan @@ -1938,7 +1938,7 @@ function approvalQueueDisplayLabel( if (column === 'term') { // Match the cell: a 0 term renders as a dash, never "0 Years" (#181). const n = Number(value); - return isRealTerm(n) ? formatTerm(n) : Number.isFinite(n) ? '-' : value; + return isRealTerm(n) ? formatTerm(n) : Number.isFinite(n) ? NO_VALUE : value; } if (column === 'account') { // Account cells render via getAccountName() — mirror the same display. @@ -2002,27 +2002,27 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { const amortize = state.getAmortizeUpfront(); const rows = visible.map(p => { const actions = renderPendingActionButtons(p); - const actionsCell = actions || '-'; + const actionsCell = actions || `${NO_VALUE}`; // Show email when resolved; fall back to UUID so the cancel-own gate still // has something human-readable to show. Fall back to "-" for scheduler rows. const createdBy = p.created_by_user_email ? escapeHtml(p.created_by_user_email) : p.created_by_user_id ? escapeHtml(p.created_by_user_id) - : '-'; + : `${NO_VALUE}`; const accountCell = p.account_id ? escapeHtml(getAccountName(p.account_id)) - : '-'; + : `${NO_VALUE}`; const empty = hasNoCommitments(p); const dashOr = (html: string): string => (empty ? MUTED_DASH : html); - const paymentCell = p.payment ? escapeHtml(p.payment) : '-'; + const paymentCell = p.payment ? escapeHtml(p.payment) : `${NO_VALUE}`; 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)) : rawMonthly; const monthlyCostCell = displayMonthly != null ? formatCurrency(displayMonthly) - : '-'; + : `${NO_VALUE}`; const execIdAttr = p.purchase_id ? ` data-execution-id="${escapeHtmlAttr(p.purchase_id)}"` : ''; return ` @@ -2030,12 +2030,12 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { ${accountCell} ${providerCell(p)} ${empty ? NO_RECS_HTML : escapeHtml(p.service)} - ${dashOr(escapeHtml(String(p.count ?? '')))} + ${dashOr(escapeHtml(String(p.count ?? '')))} ${termCell(p)} ${dashOr(paymentCell)} - ${dashOr(monthlyCostCell)} - ${empty ? `${MUTED_DASH}` : upfrontCell(p, amortize)} - ${dashOr(formatCurrency(p.estimated_savings))} + ${dashOr(monthlyCostCell)} + ${empty ? `${MUTED_DASH}` : upfrontCell(p, amortize)} + ${dashOr(formatCurrency(p.estimated_savings))} ${createdBy} ${actionsCell} @@ -2056,12 +2056,12 @@ export function renderApprovalQueue(purchases: HistoryPurchase[]): void { Account${fbtn('account')} Provider${fbtn('provider')} Service${fbtn('service')} - Count${fbtn('count')} + Count${fbtn('count')} Term${fbtn('term')} Payment${fbtn('payment')} - ${monthlyColHeader}${fbtn('monthly_cost')} - Upfront Cost${fbtn('upfront_cost')} - Monthly Savings${fbtn('savings')} + ${monthlyColHeader}${fbtn('monthly_cost')} + Upfront Cost${fbtn('upfront_cost')} + Monthly Savings${fbtn('savings')} Created by${fbtn('created_by')} Actions diff --git a/frontend/src/index.html b/frontend/src/index.html index 031c63df..c430e85e 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -215,16 +215,16 @@

Savings History

Period Savings

-

$0.00

+

—

shown in monthly equivalents

Avg Monthly Savings

-

$0.00/mo

+

—

Peak Savings

-

$0.00/mo

+

—

diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 532e2aa9..85ee670d 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -5,7 +5,7 @@ import * as api from './api'; import * as state from './state'; import { buildCommitmentSummary, withIntro } from './commitmentSummary'; -import { formatDate, formatTerm, getStatusBadge, escapeHtml, escapeHtmlAttr, formatCurrency, CURRENCY_DEFAULT_DIGITS, providerBadgeHtml } from './utils'; +import { formatDate, formatTerm, getStatusBadge, escapeHtml, escapeHtmlAttr, formatCurrency, CURRENCY_DEFAULT_DIGITS, NO_VALUE, providerBadgeHtml } from './utils'; import { showToast } from './toast'; import { confirmDialog } from './confirmDialog'; import type { PlansResponse, LocalPlan, SavePlanData } from './types'; @@ -740,7 +740,7 @@ function extractPlanInfo(plan: BackendPlan): { provider: string | null; service: .map(s => planServiceLabel(s.service || '—')) .join(', '); // H-4: never fabricate provider/term/coverage when absent. - // Return null so callers can surface '--'/'Unknown' in the display + // Return null so callers can surface NO_VALUE/'Unknown' in the display // layer rather than silently committing to aws/3yr/80% defaults. return { provider: firstService.provider || null, @@ -799,10 +799,10 @@ async function loadPlanAccountNames(planId: string, cardEl: Element): Promise
Term - ${info.term !== null ? formatTerm(info.term) : '--'} + ${info.term !== null ? formatTerm(info.term) : NO_VALUE}
Coverage - ${info.coverage !== null ? `${info.coverage}%` : '--'} + ${info.coverage !== null ? `${info.coverage}%` : NO_VALUE}
Ramp Schedule diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 0269f84d..ec4a7508 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -5,7 +5,7 @@ import * as api from './api'; import * as state from './state'; import type { CostPeriod } from './state'; -import { formatCurrency, formatTerm, escapeHtml, CURRENCY_DEFAULT_DIGITS } from './utils'; +import { formatCurrency, formatTerm, escapeHtml, CURRENCY_DEFAULT_DIGITS, NO_VALUE } from './utils'; import { getRecommendationsFreshness, refreshRecommendations as refreshRecommendationsAPI } from './api/recommendations'; import type { RecommendationsFreshness } from './api/recommendations'; import { showToast } from './toast'; @@ -880,7 +880,7 @@ function renderRecommendationsSummary( const isSelectionView = selectedVisible.length > 0 && plr.cellCount > 0; // issue #319: scale savings by the active cost period. - // M-1: use ?? null so absent savings propagate to '--' via formatCostForPeriod + // M-1: use ?? null so absent savings propagate to NO_VALUE via formatCostForPeriod // rather than silently collapsing to $0 (scaleCost returns null only when input // is null/undefined; a genuine $0 saving returns 0, not null). const period = state.getCostPeriod(); @@ -892,16 +892,16 @@ function renderRecommendationsSummary( const savingsText = hasRecs && plr.savingsMax > 0 && scaledSavingsMin !== null && scaledSavingsMax !== null ? formatScaledRange(scaledSavingsMin, scaledSavingsMax, period) : hasRecs && (plr.savingsMin === null || plr.savingsMax === null) - ? '--' + ? NO_VALUE : formatCostForPeriod(0, period); const upfrontText = hasRecs && plr.upfrontMax > 0 ? formatSavingsRange(plr.upfrontMin, plr.upfrontMax) : formatCurrency(0); - // L-2: use '--' for absent/inapplicable payback rather than '0 months' + // L-2: use NO_VALUE for absent/inapplicable payback rather than '0 months' // which falsely implies instantaneous payback. const paybackText = hasRecs && plr.paybackMonthsMax > 0 ? formatPaybackRange(plr.paybackMonthsMin, plr.paybackMonthsMax) - : '—'; + : NO_VALUE; // issue #748: count VARIANTS (= target.length), not cells (= plr.cellCount). // "Showing X of X" in the filter-status bar also counts variants, so both // numbers now agree on the same dataset. @@ -1601,7 +1601,7 @@ export function effectiveSavingsPct(r: LocalRecommendation): number | null { // denominator (EstimatedMonthlyOnDemandCost from Cost Explorer). When it // is absent the reconstruction formula (monthly_cost + savings + amortized) // diverges from the true on-demand baseline for RI/SP recs, producing - // misleadingly high percentages. Return null so the UI renders "--" rather + // misleadingly high percentages. Return null so the UI renders NO_VALUE rather // than a silently-wrong value. if (r.provider === 'aws' && !hasOnDemand) return null; const monthsInTerm = r.term * 12; @@ -1713,6 +1713,14 @@ const PERIOD_DECIMALS: Record = { yearly: 0, }; +// The non-monthly periods use explicit per-period precision (PERIOD_DECIMALS); +// monthly keeps formatCurrency's default so it matches the rest of the +// dashboard. Both go through formatCurrency so a yearly figure prints +// "$123,456", not "$123456" (issue #739). +function formatScaled(scaled: number, period: CostPeriod): string { + return period === 'monthly' ? formatCurrency(scaled) : formatCurrency(scaled, '$', PERIOD_DECIMALS[period]); +} + /** * Format a monthly cost value scaled to the requested display period. * Returns "—" for null/undefined input. @@ -1721,15 +1729,8 @@ const PERIOD_DECIMALS: Record = { */ export function formatCostForPeriod(monthly: number | null | undefined, period: CostPeriod): string { const scaled = scaleCost(monthly, period); - if (scaled === null) return '—'; - // For the non-monthly periods we want explicit per-period decimal - // precision (PERIOD_DECIMALS), so we side-step formatCurrency (which - // uses CURRENCY_DEFAULT_DIGITS for any caller that doesn't override). - // Monthly keeps the existing formatCurrency behaviour for backward - // compatibility (and to stay in lock-step with the rest of the - // dashboard's monthly $ formatting). - if (period === 'monthly') return formatCurrency(scaled); - return `$${scaled.toFixed(PERIOD_DECIMALS[period])}`; + if (scaled === null) return NO_VALUE; + return formatScaled(scaled, period); } /** Human-readable suffix label for a period (used in column headers). */ @@ -1748,8 +1749,8 @@ export function periodSuffix(period: CostPeriod): string { * Collapses "$X – $X" to "$X". */ function formatScaledRange(min: number, max: number, period: CostPeriod): string { - const lo = period === 'monthly' ? formatCurrency(min) : `$${min.toFixed(PERIOD_DECIMALS[period])}`; - const hi = period === 'monthly' ? formatCurrency(max) : `$${max.toFixed(PERIOD_DECIMALS[period])}`; + const lo = formatScaled(min, period); + const hi = formatScaled(max, period); return lo === hi ? lo : `${lo} – ${hi}`; } @@ -3169,24 +3170,24 @@ function renderColumnCell(key: state.RecommendationsColumnId, rec: LocalRecommen case 'region': return `${escapeHtml(rec.region)}`; case 'count': - return `${rec.count}`; + return `${rec.count}`; case 'term': return `${formatTerm(rec.term)}`; case 'payment': return `${escapeHtml(formatPayment(rec.payment))}`; case 'savings': // issue #319: savings display value scales with the active cost period. - return `${formatCostForPeriod(rec.savings, ctx.period)}`; + return `${formatCostForPeriod(rec.savings, ctx.period)}`; case 'upfront_cost': // upfront_cost is one-time, not recurring \u2014 period-invariant. - return `${formatCurrency(rec.upfront_cost)}`; + return `${formatCurrency(rec.upfront_cost)}`; case 'monthly_cost': // issue #319: monthly_cost display scales with period; null still renders as em-dash. - return `${formatCostForPeriod(rec.monthly_cost, ctx.period)}`; + return `${formatCostForPeriod(rec.monthly_cost, ctx.period)}`; case 'on_demand_monthly': // issue #319: on-demand baseline scales with period to stay consistent // with the savings + monthly_cost columns. - return `${formatCostForPeriod(onDemandMonthly(rec), ctx.period)}`; + return `${formatCostForPeriod(onDemandMonthly(rec), ctx.period)}`; case 'effective_savings_pct': return `${ctx.pctText}`; case 'usage_history': @@ -3220,7 +3221,7 @@ function buildVariantRowMarkup( const badge = renderSuppressionBadge(rec); const recId = escapeHtml(rec.id); const pct = displaySavingsPct(rec); - const pctClass = pct !== null && pct < 0 ? ' class="effective-pct-negative"' : ''; + const pctClass = pct !== null && pct < 0 ? ' class="num effective-pct-negative"' : ' class="num"'; const pctText = pct === null ? '\u2014' : pct.toFixed(1) + '%'; const nestedClass = isNested ? ' rec-variant-row' : ''; const cellCtx = { accountName, badge, pct, pctClass, pctText, period }; @@ -3281,10 +3282,12 @@ function buildListMarkup( // and holds two sibling buttons, sort and filter. A focusable // role=button wrapping the filter button made Enter on the filter // bubble into the sort handler, and nested the two controls. + // Money and count columns right-align so digits line up (issue #739). + const numClass = col.kind === 'numeric' ? ' num' : ''; const ariaSort = sort.column === col.key ? ` aria-sort="${sort.direction === 'asc' ? 'ascending' : 'descending'}"` : ''; - return `${filterBtn(col.key)}`; + return `${filterBtn(col.key)}`; }; // issues #225 + #226: group by cell, sort groups, then render. diff --git a/frontend/src/styles/tables.css b/frontend/src/styles/tables.css index 4b4823cd..f33d00a5 100644 --- a/frontend/src/styles/tables.css +++ b/frontend/src/styles/tables.css @@ -33,6 +33,14 @@ th { letter-spacing: 0.04em; } +/* Money and count columns: right-aligned with tabular digits so amounts line + up for comparison (issue #739). */ +th.num, +td.num { + text-align: right; + font-variant-numeric: tabular-nums; +} + tr:hover { background: #f1f3f4; } diff --git a/frontend/src/trends.ts b/frontend/src/trends.ts index 34bce675..da8f8803 100644 --- a/frontend/src/trends.ts +++ b/frontend/src/trends.ts @@ -17,7 +17,7 @@ import * as api from './api'; import type { AnalyticsTrendsResponse, TrendsProviderRow, TrendsServiceRow } from './api'; import type { ApiError } from './api/types'; import * as state from './state'; -import { formatCurrency } from './utils'; +import { formatCurrency, NO_VALUE } from './utils'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { DEFAULT_TRENDS_PERIOD_MONTHS, @@ -219,9 +219,9 @@ function buildStats(series: TrendsMonthPoint[]): HTMLElement { const avg = meanOrNull(withData.map((p) => p.savings)); const latestCoverage = [...series].reverse().find((p) => p.coverage !== null) ?? null; grid.append( - statCard('Latest savings', latest ? `${formatCurrency(latest.savings)}/mo` : '--', + statCard('Latest savings', latest ? `${formatCurrency(latest.savings)}/mo` : NO_VALUE, latest ? `run-rate in ${latest.label}` : 'no snapshots in this period'), - statCard('Average savings', avg !== null ? `${formatCurrency(avg)}/mo` : '--', + statCard('Average savings', avg !== null ? `${formatCurrency(avg)}/mo` : NO_VALUE, `over ${withData.length} of ${series.length} months with data`), statCard('Coverage', formatCoverage(latestCoverage?.coverage), latestCoverage ? `in ${latestCoverage.label}` : 'not collected for this period'), diff --git a/frontend/src/utils.ts b/frontend/src/utils.ts index ab2b3eea..e1f62f5f 100644 --- a/frontend/src/utils.ts +++ b/frontend/src/utils.ts @@ -17,6 +17,14 @@ export function reloadPage(): void { */ export const CURRENCY_DEFAULT_DIGITS = 0; +/** + * The one placeholder for an absent value (issue #739). Money, counts, + * percentages and terms that the API did not report all render this, so a + * missing figure never reads as a real $0 and never shows as "--" on one + * tile and "—" on the next. + */ +export const NO_VALUE = '\u2014'; + /** * Format a number as currency. * @@ -46,7 +54,7 @@ export function formatCurrency( // Represent absent or non-finite values distinctly from a real $0 so // callers can see when data is missing vs actually zero (finding 11-N2). if (value === null || value === undefined || !Number.isFinite(value)) { - return '--'; + return NO_VALUE; } return `${currency}${value.toLocaleString('en-US', { minimumFractionDigits: digits, diff --git a/frontend/tests-e2e/money-columns.spec.ts b/frontend/tests-e2e/money-columns.spec.ts new file mode 100644 index 00000000..f6e4ede1 --- /dev/null +++ b/frontend/tests-e2e/money-columns.spec.ts @@ -0,0 +1,30 @@ +import { test, expect } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; + +// Issue #739: money and count columns right-align with tabular digits, and +// a yearly figure groups its thousands instead of printing "$123456". +test('Opportunities money columns are right-aligned and yearly amounts keep separators', async ({ page }) => { + await mockApi(page); + await seedAuth(page); + const big = { + id: 'big', provider: 'aws', cloud_account_id: 'acct-001', service: 'ec2', resource_type: 'm5.24xlarge', + region: 'us-east-1', count: 12, term: 3, payment: 'all-upfront', savings: 10288, upfront_cost: 250000, + monthly_cost: 0, on_demand_monthly: 18000, effective_savings_pct: 57, + }; + await page.route(/\/api\/recommendations(?:\?.*)?$/, route => route.fulfill({ json: { + recommendations: [big], summary: { total_recommendations: 1, total_upfront_cost: 250000, potential_monthly_savings: 10288, avg_payback_months: 24 }, regions: ['us-east-1'], + } })); + await page.goto('/opportunities'); + const row = page.locator('tr.recommendation-row[data-rec-id="big"]'); + await expect(row).toBeVisible(); + + for (const cell of [row.locator('td.savings'), row.locator('td.num').first()]) { + await expect(cell).toHaveCSS('text-align', 'right'); + await expect(cell).toHaveCSS('font-variant-numeric', 'tabular-nums'); + } + await expect(page.locator('th.sortable[data-sort="upfront_cost"]')).toHaveCSS('text-align', 'right'); + await expect(page.locator('th.sortable[data-sort="region"]')).toHaveCSS('text-align', 'left'); + + await page.locator('#cost-period-select').selectOption('yearly'); + await expect(page.locator('tr.recommendation-row[data-rec-id="big"] td.savings')).toHaveText('$123,456'); +}); diff --git a/frontend/tests-e2e/nullable-purchase-cost.spec.ts b/frontend/tests-e2e/nullable-purchase-cost.spec.ts index 50a4814f..33d7885d 100644 --- a/frontend/tests-e2e/nullable-purchase-cost.spec.ts +++ b/frontend/tests-e2e/nullable-purchase-cost.spec.ts @@ -20,10 +20,10 @@ test('unknown upfront remains unknown through history and inventory amortization await page.goto('/purchases/history'); const history = page.locator('#history-list tbody tr').first(); - await expect(history.locator('td').nth(8)).toHaveText('--'); + await expect(history.locator('td').nth(8)).toHaveText('\u2014'); await expect(history.locator('td').nth(9)).toHaveText('$50'); await page.locator('#history-amortize-checkbox').check(); - await expect(history.locator('td').nth(9)).toHaveText('-'); + await expect(history.locator('td').nth(9)).toHaveText('\u2014'); await page.locator('#history-amortize-checkbox').uncheck(); await expect(history.locator('td').nth(9)).toHaveText('$50'); diff --git a/frontend/tests-e2e/trends.spec.ts b/frontend/tests-e2e/trends.spec.ts index 4c411730..12f19635 100644 --- a/frontend/tests-e2e/trends.spec.ts +++ b/frontend/tests-e2e/trends.spec.ts @@ -22,7 +22,7 @@ test('Trends sub-tab renders the series, breakdowns and a laid-out chart', async const monthly = page.locator('#trends-monthly-table tbody tr'); await expect(monthly).toHaveCount(6); await expect(monthly.nth(5).locator('td').nth(1)).toHaveText('$2,040'); - await expect(monthly.nth(3).locator('td').nth(1)).toHaveText('--'); + await expect(monthly.nth(3).locator('td').nth(1)).toHaveText('\u2014'); await expect(page.locator('#trends-provider-table tbody tr')).toHaveCount(3); await expect(page.locator('#trends-service-table tbody tr')).toHaveCount(4);