From 00393859a49082641e481676ddc1a9a78aee34d9 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 22:10:59 +0000 Subject: [PATCH] fix(frontend): name instance types and the price change in RI exchange approve and quote The Execute confirm already names instance types and the hourly price change (#781), but two money surfaces still showed the opaque view: the quote summary listed raw source and target hourly prices with no delta, and the Approve confirm in exchange history led with the target offering UUID as one run-on sentence. The quote summary now adds an Hourly Price Change row, and the Approve confirm uses the same labeled lines as Execute (Exchange: 2 x m5.large (ri ids), For: 1 x m5.xlarge (offering id)), so the user can see what an irreversible exchange swaps before committing. The confirm body builders move to riexchange-confirm.ts so the Execute and Approve dialogs share one formatter and riexchange.ts shrinks. The two openExchangeModal describes now re-seed listTargetOfferings, since a resetAllMocks elsewhere in the file broke them under --randomize. Refs #709 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/riexchange-confirm.test.ts | 81 +++++++++++++++++ frontend/src/__tests__/riexchange.test.ts | 19 ++++ frontend/src/riexchange-confirm.ts | 90 +++++++++++++++++++ frontend/src/riexchange.ts | 48 +--------- .../ri-exchange-confirmation.spec.ts | 4 +- 5 files changed, 197 insertions(+), 45 deletions(-) create mode 100644 frontend/src/__tests__/riexchange-confirm.test.ts create mode 100644 frontend/src/riexchange-confirm.ts diff --git a/frontend/src/__tests__/riexchange-confirm.test.ts b/frontend/src/__tests__/riexchange-confirm.test.ts new file mode 100644 index 000000000..8426c272f --- /dev/null +++ b/frontend/src/__tests__/riexchange-confirm.test.ts @@ -0,0 +1,81 @@ +/** + * RI exchange confirm bodies (issue #709): instance types lead, the exact + * offering id stays visible, and the quoted hourly price change is shown. + */ +import { buildApproveConfirmBody, buildExchangeConfirmBody, hourlyPriceDelta } from '../riexchange-confirm'; +import type { ExchangeQuoteSummary, RIExchangeHistoryRecord } from '../api'; + +const offering = '4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91'; + +function quote(over: Partial = {}): ExchangeQuoteSummary { + return { + IsValidExchange: true, + ValidationFailureReason: '', + CurrencyCode: 'USD', + PaymentDueRaw: '12.50', + SourceHourlyPriceRaw: '0.1000', + SourceRemainingUpfrontRaw: '', + SourceRemainingTotalRaw: '', + TargetHourlyPriceRaw: '0.1250', + TargetRemainingUpfrontRaw: '', + TargetRemainingTotalRaw: '', + Region: 'us-west-2', + ...over, + }; +} + +function record(over: Partial = {}): RIExchangeHistoryRecord { + return { + id: 'h1', account_id: 'a1', exchange_id: '', region: 'us-east-1', + source_ri_ids: ['ri-1', 'ri-2'], source_instance_type: 'm5.large', source_count: 2, + target_offering_id: offering, target_instance_type: 'm5.xlarge', target_count: 1, + payment_due: '7.25', status: 'pending', mode: 'manual', + created_at: '2026-10-01T00:00:00Z', updated_at: '2026-10-01T00:00:00Z', + ...over, + }; +} + +describe('hourlyPriceDelta', () => { + it('describes an increase with the monthly estimate', () => { + expect(hourlyPriceDelta(quote())).toBe('+0.0250 USD/hr, about +18.25 USD/month'); + }); + + it('describes a decrease with a minus sign', () => { + expect(hourlyPriceDelta(quote({ SourceHourlyPriceRaw: '0.10', TargetHourlyPriceRaw: '0.08' }))) + .toBe('-0.0200 USD/hr, about -14.60 USD/month'); + }); + + it('returns null when either price is missing or not a number', () => { + expect(hourlyPriceDelta(quote({ SourceHourlyPriceRaw: '' }))).toBeNull(); + expect(hourlyPriceDelta(quote({ TargetHourlyPriceRaw: 'n/a' }))).toBeNull(); + }); +}); + +describe('buildExchangeConfirmBody', () => { + it('omits the price change line when the quote has no hourly prices', () => { + const text = buildExchangeConfirmBody({ + quote: quote({ SourceHourlyPriceRaw: '', TargetHourlyPriceRaw: '' }), + sources: ['1 × m5.large (ri-1)'], + targets: ['1 × m5.xlarge'], + }).textContent ?? ''; + expect(text).toContain('Pay now: USD 12.50'); + expect(text).not.toContain('Hourly price change'); + }); +}); + +describe('buildApproveConfirmBody', () => { + it('leads with instance types and keeps the offering id next to the target', () => { + const text = buildApproveConfirmBody(record()).textContent ?? ''; + expect(text).toContain('Previously quoted payment: USD 7.25'); + expect(text).toContain('Exchange: 2 × m5.large (ri-1, ri-2)'); + expect(text).toContain(`For: 1 × m5.xlarge (offering ${offering})`); + expect(text).toContain('Region: us-east-1'); + expect(text).toContain('cannot be reversed'); + }); + + it('falls back to ids when the record carries no instance types', () => { + const text = buildApproveConfirmBody(record({ source_instance_type: '', target_instance_type: '' })).textContent ?? ''; + expect(text).toContain('Exchange: ri-1, ri-2'); + expect(text).toContain(`For: 1 × offering ${offering}`); + }); +}); diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index e9f42d41c..2741adfce 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -87,6 +87,9 @@ describe('openExchangeModal', () => { beforeEach(() => { modal = createModal(); + // Other describes call jest.resetAllMocks(), which wipes the default + // listTargetOfferings resolution; restore it so test order does not matter. + (api.listTargetOfferings as jest.Mock).mockResolvedValue([]); }); afterEach(() => { @@ -258,6 +261,21 @@ describe('openExchangeModal', () => { document.querySelector('.modal-confirm-close')?.click(); }); + it('shows the hourly price change in the quote summary (issue #709)', async () => { + (api.getExchangeQuote as jest.Mock).mockResolvedValueOnce({ + IsValidExchange: true, ValidationFailureReason: '', CurrencyCode: 'USD', PaymentDueRaw: '12.50', + SourceHourlyPriceRaw: '0.1000', SourceRemainingUpfrontRaw: '', SourceRemainingTotalRaw: '', + TargetHourlyPriceRaw: '0.1250', TargetRemainingUpfrontRaw: '', TargetRemainingTotalRaw: '', Region: 'us-west-2', + }); + openExchangeModal('ri-abc', 3, 'm5.large', [ + { instance_type: 'm5.large', offering_id: '4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91', effective_monthly_cost: 42.5 }, + ]); + Array.from(modal.querySelectorAll('button')).find((b) => b.textContent === 'Get Quote')?.click(); + await new Promise((resolve) => setTimeout(resolve, 0)); + const rows = Array.from(modal.querySelectorAll('.quote-row')).map((r) => r.textContent); + expect(rows).toContain('Hourly Price Change:+0.0250 USD/hr, about +18.25 USD/month'); + }); + it('invalidates a fetched quote when the count changes, hiding Execute (issue #244)', async () => { const mockGetQuote = api.getExchangeQuote as jest.Mock; mockGetQuote.mockResolvedValueOnce({ @@ -634,6 +652,7 @@ describe('fillQuoteFromRI', () => { beforeEach(() => { modal = createModal(); + (api.listTargetOfferings as jest.Mock).mockResolvedValue([]); }); afterEach(() => { diff --git a/frontend/src/riexchange-confirm.ts b/frontend/src/riexchange-confirm.ts new file mode 100644 index 000000000..67b9391d6 --- /dev/null +++ b/frontend/src/riexchange-confirm.ts @@ -0,0 +1,90 @@ +/** + * Readable bodies for the RI exchange money confirms and quote summary. + * + * An exchange is irreversible, so every place that asks the user to commit + * to one (Execute after a quote, Approve from history) names what is given + * up and what is received by instance type first, keeps the exact offering + * id next to it for verification, and shows the quoted price change. + */ + +import type { ExchangeQuoteSummary, RIExchangeHistoryRecord } from './api'; + +// Hours in an average month; only used for the approximate monthly delta. +const HOURS_PER_MONTH = 730; + +export interface ExchangeConfirmParts { + quote: ExchangeQuoteSummary; + sources: string[]; + targets: string[]; +} + +// hourlyPriceDelta describes the quoted hourly price change as +// "+0.0250 USD/hr, about +18.25 USD/month", or returns null when the quote +// lacks either hourly price, so callers never show a made-up delta. +export function hourlyPriceDelta(quote: ExchangeQuoteSummary): string | null { + const src = Number(quote.SourceHourlyPriceRaw); + const tgt = Number(quote.TargetHourlyPriceRaw); + if (!quote.SourceHourlyPriceRaw || !quote.TargetHourlyPriceRaw || !Number.isFinite(src) || !Number.isFinite(tgt)) { + return null; + } + const delta = tgt - src; + const sign = delta >= 0 ? '+' : '-'; + const abs = Math.abs(delta); + const cur = quote.CurrencyCode; + return `${sign}${abs.toFixed(4)} ${cur}/hr, about ${sign}${(abs * HOURS_PER_MONTH).toFixed(2)} ${cur}/month`; +} + +function confirmBody(lines: Array<[string, string]>, warning: string): HTMLElement { + const root = document.createElement('div'); + for (const [label, value] of lines) { + const p = document.createElement('p'); + const strong = document.createElement('strong'); + strong.textContent = value; + p.append(`${label}: `, strong); + root.appendChild(p); + } + const warn = document.createElement('p'); + warn.textContent = warning; + root.appendChild(warn); + return root; +} + +export function buildExchangeConfirmBody({ quote, sources, targets }: ExchangeConfirmParts): HTMLElement { + const lines: Array<[string, string]> = [ + ['Pay now', `${quote.CurrencyCode} ${quote.PaymentDueRaw}`], + ['Exchange', sources.join(', ')], + ['For', targets.join(', ')], + ['Region', quote.Region ?? ''], + ]; + const delta = hourlyPriceDelta(quote); + if (delta) { + const src = Number(quote.SourceHourlyPriceRaw).toFixed(4); + const tgt = Number(quote.TargetHourlyPriceRaw).toFixed(4); + lines.push(['Hourly price change', `${quote.CurrencyCode} ${src} → ${tgt} (${delta})`]); + } + return confirmBody(lines, 'This exchange executes immediately and cannot be reversed.'); +} + +// buildApproveConfirmBody describes a pending history record. The history +// API stores payment_due in USD (the execute cap is max_payment_due_usd), +// and carries the instance types, so the confirm leads with those rather +// than the target offering UUID. +export function buildApproveConfirmBody(record: RIExchangeHistoryRecord): HTMLElement { + const ids = record.source_ri_ids.join(', '); + const source = record.source_instance_type + ? `${record.source_count} × ${record.source_instance_type} (${ids})` + : ids; + const target = record.target_instance_type + ? `${record.target_count} × ${record.target_instance_type} (offering ${record.target_offering_id})` + : `${record.target_count} × offering ${record.target_offering_id}`; + return confirmBody( + [ + ['Previously quoted payment', `USD ${record.payment_due}`], + ['Exchange', source], + ['For', target], + ['Region', record.region], + ], + 'Approving executes immediately using a fresh quote, within the configured per-exchange ' + + 'and remaining daily spending limits. This exchange cannot be reversed.', + ); +} diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 4b0855f7b..5090670c4 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -8,6 +8,7 @@ import * as state from './state'; import { formatDate, formatDateTime, escapeHtml, formatCurrency } from './utils'; import { switchTab, switchSettingsSubTab } from './navigation'; import { confirmDialog } from './confirmDialog'; +import { buildApproveConfirmBody, buildExchangeConfirmBody, hourlyPriceDelta } from './riexchange-confirm'; import { parseNumericFilter, applyColumnFilters as applyColumnFiltersLib, @@ -1966,45 +1967,6 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy } } -interface ExchangeConfirmParts { - quote: ExchangeQuoteSummary; - sources: string[]; - targets: string[]; -} - -// Hours in an average month; only used for the approximate monthly delta. -const HOURS_PER_MONTH = 730; - -function buildExchangeConfirmBody({ quote, sources, targets }: ExchangeConfirmParts): HTMLElement { - const root = document.createElement('div'); - const line = (label: string, value: string): void => { - const p = document.createElement('p'); - const strong = document.createElement('strong'); - strong.textContent = value; - p.append(`${label}: `, strong); - root.appendChild(p); - }; - line('Pay now', `${quote.CurrencyCode} ${quote.PaymentDueRaw}`); - line('Exchange', sources.join(', ')); - line('For', targets.join(', ')); - line('Region', quote.Region ?? ''); - const src = Number(quote.SourceHourlyPriceRaw); - const tgt = Number(quote.TargetHourlyPriceRaw); - if (quote.SourceHourlyPriceRaw && quote.TargetHourlyPriceRaw && Number.isFinite(src) && Number.isFinite(tgt)) { - const delta = tgt - src; - const sign = delta >= 0 ? '+' : '-'; - const abs = Math.abs(delta); - line( - 'Hourly price change', - `${quote.CurrencyCode} ${src.toFixed(4)} \u2192 ${tgt.toFixed(4)} (${sign}${abs.toFixed(4)} ${quote.CurrencyCode}/hr, about ${sign}${(abs * HOURS_PER_MONTH).toFixed(2)} ${quote.CurrencyCode}/month)`, - ); - } - const warn = document.createElement('p'); - warn.textContent = 'This exchange executes immediately and cannot be reversed.'; - root.appendChild(warn); - return root; -} - function setResultText(container: HTMLElement, message: string, cls: string): void { container.textContent = ''; const p = document.createElement('p'); @@ -2036,6 +1998,8 @@ function renderModalQuoteResult(container: HTMLElement, quote: ExchangeQuoteSumm ['Source Hourly Price', quote.SourceHourlyPriceRaw], ['Target Hourly Price', quote.TargetHourlyPriceRaw], ]; + const delta = hourlyPriceDelta(quote); + if (delta) rows.push(['Hourly Price Change', delta]); if (quote.OutputReservedInstancesExp) { rows.push(['New RI Expiry', quote.OutputReservedInstancesExp]); } @@ -2425,11 +2389,7 @@ async function handleRIExchangeApproveClick(btn: HTMLButtonElement, record: RIEx btn.disabled = true; const confirmed = await confirmDialog({ title: 'Approve RI Exchange', - body: `Previously quoted payment: USD ${record.payment_due}. ` - + `Source RIs: ${record.source_ri_ids.join(', ')}. Region: ${record.region}. ` - + `Target: ${record.target_count} × ${record.target_offering_id} (${record.target_instance_type}). ` - + 'Approving executes immediately using a fresh quote, within the configured per-exchange ' - + 'and remaining daily spending limits. This exchange cannot be reversed.', + body: buildApproveConfirmBody(record), confirmLabel: 'Approve', destructive: true, }); diff --git a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts index e61ebe377..7eac2abf6 100644 --- a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts +++ b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts @@ -85,6 +85,7 @@ async function openQuote(page: Page) { await page.locator('.modal-exchange-target-select').selectOption(offering); await page.getByRole('button', { name: 'Get Quote', exact: true }).click(); await expect(page.locator('#modal-exchange-result')).toContainText('Valid Exchange'); + await expect(page.locator('#modal-exchange-result')).toContainText('Hourly Price Change:-0.0200 USD/hr, about -14.60 USD/month'); } const execute = (page: Page) => page.locator('#ri-exchange-modal').getByRole('button', { name: 'Execute Exchange', exact: true }); @@ -134,7 +135,8 @@ for (const action of ['Execute', 'Approve']) { if (action === 'Approve') { await expect(dialog(page)).toContainText('Previously quoted payment: USD 123.456789'); await expect(dialog(page)).toContainText('fresh quote'); - await expect(dialog(page)).toContainText(`1 × ${offering}`); + await expect(dialog(page)).toContainText(`2 × m5.large (${ri})`); + await expect(dialog(page)).toContainText(`1 × m5.xlarge (offering ${offering})`); } if (dismissal === 'Escape') await page.keyboard.press('Escape'); else if (dismissal === 'backdrop') await dialog(page).click({ position: { x: 2, y: 2 } });