From 17b39eda490059eea4f589a96624239306b3d392 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 12:42:04 +0200 Subject: [PATCH 1/5] fix(frontend): money confirms state what is bought and how much Execute now, Run now and the RI exchange execute confirm now show a shared stat band (charged today, monthly, total over term) plus the commitment lines with account and region, focus Cancel first, and Run now ignores a second click while its dialog or request is open. The RI exchange confirm names source and target instance types instead of offering UUIDs and shows the hourly price change from the quote. Refs #707, #709, #701, #711 Co-Authored-By: Claude Sonnet 5.5 --- .../src/__tests__/commitmentSummary.test.ts | 62 +++++++++ frontend/src/__tests__/plans.test.ts | 28 +++++ .../__tests__/purchase-modal-submit.test.ts | 9 +- frontend/src/__tests__/riexchange.test.ts | 32 +++++ frontend/src/app.ts | 21 +++- frontend/src/commitmentSummary.ts | 119 ++++++++++++++++++ frontend/src/plans.ts | 59 +++++++-- frontend/src/riexchange.ts | 59 ++++++++- frontend/src/styles/components.css | 32 +++++ 9 files changed, 398 insertions(+), 23 deletions(-) create mode 100644 frontend/src/__tests__/commitmentSummary.test.ts create mode 100644 frontend/src/commitmentSummary.ts diff --git a/frontend/src/__tests__/commitmentSummary.test.ts b/frontend/src/__tests__/commitmentSummary.test.ts new file mode 100644 index 000000000..ee2f1c023 --- /dev/null +++ b/frontend/src/__tests__/commitmentSummary.test.ts @@ -0,0 +1,62 @@ +import { buildCommitmentSummary, commitmentTotals, paymentLabel, withIntro, type CommitmentLine } from '../commitmentSummary'; + +const line = (over: Partial = {}): CommitmentLine => ({ + service: 'ec2', + resourceType: 'm5.large', + count: 2, + term: 3, + payment: 'no-upfront', + upfront: 0, + monthly: 100, + ...over, +}); + +describe('commitmentTotals', () => { + it('sums upfront, monthly and the term total (3 years = 36 months)', () => { + const t = commitmentTotals([line({ upfront: 500, monthly: 100 }), line({ upfront: 0, monthly: 50, term: 1 })]); + expect(t.upfront).toBe(500); + expect(t.monthly).toBe(150); + expect(t.termTotal).toBe(500 + 100 * 36 + 50 * 12); + }); + + it('is null for monthly and term total when any line has no monthly cost', () => { + const t = commitmentTotals([line(), line({ monthly: null, upfront: 10 })]); + expect(t.upfront).toBe(10); + expect(t.monthly).toBeNull(); + expect(t.termTotal).toBeNull(); + }); +}); + +describe('paymentLabel', () => { + it.each([ + ['all-upfront', 'All upfront'], + ['partial_upfront', 'Partial upfront'], + ['no-upfront', 'No upfront'], + ['monthly', 'monthly'], + ])('%s -> %s', (raw, label) => expect(paymentLabel(raw)).toBe(label)); +}); + +describe('buildCommitmentSummary', () => { + it('shows charged today, monthly and total over term', () => { + const el = buildCommitmentSummary([line({ upfront: 1200, monthly: 10, term: 1, account: 'prod' })]); + const text = el.textContent ?? ''; + expect(text).toContain('Charged today$1,200.00'); + expect(text).toContain('Monthly$10.00'); + expect(text).toContain('Total over term$1,320.00'); + expect(text).toContain('2 × ec2 m5.large · 1 Year, No upfront · prod'); + }); + + it('says so when the monthly cost is not reported, and truncates long lists', () => { + const el = buildCommitmentSummary(Array.from({ length: 7 }, () => line({ monthly: null }))); + expect(el.textContent).toContain('MonthlyNot reported'); + expect(el.textContent).toContain('Total over termNot available'); + expect(el.querySelectorAll('li')).toHaveLength(6); + expect(el.textContent).toContain('and 2 more'); + }); + + it('withIntro puts the sentence before the summary', () => { + const body = withIntro('Intro.', buildCommitmentSummary([line()])); + expect(body.firstElementChild?.textContent).toBe('Intro.'); + expect(body.querySelector('.commitment-summary')).not.toBeNull(); + }); +}); diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index 08e547e49..560df22fc 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -957,6 +957,34 @@ describe('Plans Module', () => { expect(api.runPlannedPurchase).not.toHaveBeenCalled(); }); + test('run confirm names the step and shows the amount charged today', async () => { + mockConfirmDialog.mockClear(); + mockConfirmDialog.mockResolvedValue(false); + + (document.querySelector('[data-action="run"]') as HTMLButtonElement).click(); + await new Promise(resolve => setTimeout(resolve, 50)); + + const opts = mockConfirmDialog.mock.calls[0]![0] as { title: string; body: HTMLElement }; + expect(opts.title).toBe('Run step 1/4 of "Test Plan" now?'); + expect(opts.body.textContent).toMatch(/Charged today\$1,?000/); + expect(opts.body.textContent).toContain('5 \u00d7 ec2 t3.medium'); + }); + + test('a second click while the run confirm is open does not stack a second dialog', async () => { + mockConfirmDialog.mockClear(); + let resolveDialog: (v: boolean) => void = () => undefined; + mockConfirmDialog.mockReturnValue(new Promise(r => { resolveDialog = r; })); + + const runBtn = document.querySelector('[data-action="run"]') as HTMLButtonElement; + runBtn.click(); + runBtn.click(); + await new Promise(resolve => setTimeout(resolve, 20)); + expect(mockConfirmDialog).toHaveBeenCalledTimes(1); + + resolveDialog(false); + await new Promise(resolve => setTimeout(resolve, 20)); + }); + test('pause action pauses purchase', async () => { (api.pausePlannedPurchase as jest.Mock).mockResolvedValue({}); (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); diff --git a/frontend/src/__tests__/purchase-modal-submit.test.ts b/frontend/src/__tests__/purchase-modal-submit.test.ts index d10cf9f09..efa5810b4 100644 --- a/frontend/src/__tests__/purchase-modal-submit.test.ts +++ b/frontend/src/__tests__/purchase-modal-submit.test.ts @@ -308,9 +308,12 @@ describe('Issue #251: Execute Now makes no cancellation promise', () => { (document.getElementById('execute-purchase-btn') as HTMLButtonElement).click(); await flush(); expect(confirmDialog).toHaveBeenCalledTimes(1); - const opts = (confirmDialog as jest.Mock).mock.calls[0]![0] as { title: string; body: string }; - expect(opts.body).toMatch(/charge the full upfront amount immediately/); - expect(`${opts.title} ${opts.body}`).not.toMatch(/AWS|cancell|24 hours/i); + const opts = (confirmDialog as jest.Mock).mock.calls[0]![0] as { title: string; body: HTMLElement }; + const bodyText = opts.body.textContent ?? ''; + expect(bodyText).toMatch(/charges the amount shown under "Charged today" immediately/); + expect(bodyText).toContain('Charged today'); + expect(bodyText).toContain(formatCurrency(rec.upfront_cost, '$', 2)); + expect(`${opts.title} ${bodyText}`).not.toMatch(/AWS|cancell|24 hours/i); }); }); diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 83b17691b..798ccdd0b 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -223,6 +223,38 @@ describe('openExchangeModal', () => { expect(execReq.region).toBe('us-west-2'); }); + it('names instance types and the hourly price change in the execute confirm', async () => { + const mockGetQuote = api.getExchangeQuote as jest.Mock; + mockGetQuote.mockResolvedValueOnce({ + IsValidExchange: true, + ValidationFailureReason: '', + CurrencyCode: 'USD', + PaymentDueRaw: '12.50', + SourceHourlyPriceRaw: '0.1000', + SourceRemainingUpfrontRaw: '', + SourceRemainingTotalRaw: '', + TargetHourlyPriceRaw: '0.1250', + TargetRemainingUpfrontRaw: '', + TargetRemainingTotalRaw: '', + Region: 'us-west-2', + }); + const offeringUUID = '4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91'; + openExchangeModal('ri-abc', 3, 'm5.large', [ + { instance_type: 'm5.large', offering_id: offeringUUID, effective_monthly_cost: 42.5 }, + ]); + Array.from(modal.querySelectorAll('button')).find((b) => b.textContent === 'Get Quote')?.click(); + await new Promise((resolve) => setTimeout(resolve, 0)); + Array.from(modal.querySelectorAll('button')).find((b) => b.textContent === 'Execute Exchange')?.click(); + + const text = document.querySelector('.modal-confirm-body')?.textContent ?? ''; + expect(text).toContain('Pay now: USD 12.50'); + expect(text).toContain('3 \u00d7 m5.large'); + expect(text).not.toContain(offeringUUID); + expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250/hr, about +18.25/month)'); + expect(document.activeElement?.textContent).toBe('Cancel'); + document.querySelector('.modal-confirm-close')?.click(); + }); + it('invalidates a fetched quote when the count changes, hiding Execute (issue #244)', async () => { const mockGetQuote = api.getExchangeQuote as jest.Mock; mockGetQuote.mockResolvedValueOnce({ diff --git a/frontend/src/app.ts b/frontend/src/app.ts index b187ae818..a4b9cee92 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -6,7 +6,7 @@ import * as api from './api'; import * as state from './state'; import { showLoginModal, showAdminSetupModal, showResetPasswordModal, updateUserUI } from './auth'; import { loadDashboard, setupDashboardHandlers } from './dashboard'; -import { setupRecommendationsHandlers, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, getFanOutBuckets, clearFanOutBuckets, getExecuteMode, clearExecuteMode, fanOutBucketAccountLabel, type FanOutBucket } from './recommendations'; +import { setupRecommendationsHandlers, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, getFanOutBuckets, clearFanOutBuckets, getExecuteMode, clearExecuteMode, fanOutBucketAccountLabel, getAccountName, type FanOutBucket } from './recommendations'; import { switchTab, applyTabFromPath, initRouter, switchSettingsSubTab, canonicalTabPath } from './navigation'; import { savePlan, setupPlanHandlers, closePlanModal, openNewPlanModal, closePurchaseModal } from './plans'; import { saveGlobalSettings, setupSettingsHandlers, resetSettings } from './settings'; @@ -18,6 +18,7 @@ import { setupRIExchangeHandlers, saveAutomationSettings } from './riexchange'; import { showToast } from './toast'; import { formatPaymentAdjustmentNotice } from './commitmentOptions'; import { confirmDialog } from './confirmDialog'; +import { buildCommitmentSummary, withIntro } from './commitmentSummary'; import { handlePurchaseDeeplink } from './purchases-deeplink'; import { handleArcheraDeeplink, openArcheraOfferModal } from './archera'; import { closeModal } from './modal'; @@ -398,16 +399,30 @@ export async function handleExecutePurchase(): Promise { // - Approval path: low-friction, non-destructive. // - Direct-execute path: red destructive dialog with cost callout and // immediate-charge callout; no cancellation promise (issue #251). + const summary = buildCommitmentSummary( + recsWithPayment.map((r) => ({ + service: r.service, + provider: r.provider, + region: r.region, + resourceType: r.resource_type, + count: r.count, + term: r.term, + payment: r.payment, + upfront: r.upfront_cost, + monthly: r.monthly_cost ?? null, + ...(r.cloud_account_id ? { account: getAccountName(r.cloud_account_id) } : {}), + })), + ); const ok = isDirect ? await confirmDialog({ title: `Execute ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} now?`, - body: 'This will charge the full upfront amount immediately. This bypasses the approval step.', + body: withIntro('This charges the amount shown under "Charged today" immediately and bypasses the approval step. The monthly amount, if any, is billed for the whole term.', summary), confirmLabel: 'Execute Purchase Now', destructive: true, }) : await confirmDialog({ title: `Send ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} for approval?`, - body: 'This will email an approval request to the configured approver. Cloud commitments are charged only after the approver clicks the link in that email.', + body: withIntro('This emails an approval request to the configured approver. Nothing is charged until the approver clicks the link in that email.', summary), confirmLabel: 'Send for approval', destructive: false, }); diff --git a/frontend/src/commitmentSummary.ts b/frontend/src/commitmentSummary.ts new file mode 100644 index 000000000..f3c1e5503 --- /dev/null +++ b/frontend/src/commitmentSummary.ts @@ -0,0 +1,119 @@ +/** + * Shared "what exactly will be bought" block for money-moving confirm + * dialogs (Execute now, Run now, RI exchange). Pure DOM builder plus the + * totals helper, so every confirm states the same three numbers: what is + * charged today, what recurs monthly, and the total over the term. + */ + +import { formatCurrency, formatTerm } from './utils'; + +export interface CommitmentLine { + service: string; + provider?: string; + region?: string; + resourceType: string; + count: number; + /** Term in years. */ + term: number; + payment: string; + upfront: number; + /** null when the provider did not report a recurring breakdown. */ + monthly: number | null; + /** Resolved display name of the account, when known. */ + account?: string; +} + +export interface CommitmentTotals { + upfront: number; + /** null when any line has no reported monthly cost. */ + monthly: number | null; + /** upfront + monthly x 12 x term; null when monthly is null. */ + termTotal: number | null; +} + +const MAX_LISTED_LINES = 5; + +export function commitmentTotals(lines: readonly CommitmentLine[]): CommitmentTotals { + let upfront = 0; + let monthly: number | null = 0; + let termTotal: number | null = 0; + for (const l of lines) { + upfront += l.upfront; + if (l.monthly === null || monthly === null || termTotal === null) { + monthly = null; + termTotal = null; + continue; + } + monthly += l.monthly; + termTotal += l.upfront + l.monthly * 12 * l.term; + } + return { upfront, monthly, termTotal }; +} + +export function paymentLabel(payment: string): string { + switch (payment.replace(/_/g, '-')) { + case 'all-upfront': return 'All upfront'; + case 'partial-upfront': return 'Partial upfront'; + case 'no-upfront': return 'No upfront'; + default: return payment; + } +} + +function stat(label: string, value: string): HTMLElement { + const el = document.createElement('div'); + el.className = 'commitment-summary-stat'; + const l = document.createElement('span'); + l.className = 'commitment-summary-label'; + l.textContent = label; + const v = document.createElement('strong'); + v.className = 'commitment-summary-value'; + v.textContent = value; + el.append(l, v); + return el; +} + +function describe(l: CommitmentLine): string { + const where = [l.account, l.region].filter(Boolean).join(', '); + const what = `${l.count} × ${l.service} ${l.resourceType}`; + const terms = `${formatTerm(l.term)}, ${paymentLabel(l.payment)}`; + return [what, terms, where].filter(Boolean).join(' · '); +} + +export function buildCommitmentSummary(lines: readonly CommitmentLine[]): HTMLElement { + const totals = commitmentTotals(lines); + const root = document.createElement('div'); + root.className = 'commitment-summary'; + + const band = document.createElement('div'); + band.className = 'commitment-summary-band'; + band.append( + stat('Charged today', formatCurrency(totals.upfront, '$', 2)), + stat('Monthly', totals.monthly === null ? 'Not reported' : formatCurrency(totals.monthly, '$', 2)), + stat('Total over term', totals.termTotal === null ? 'Not available' : formatCurrency(totals.termTotal, '$', 2)), + ); + root.appendChild(band); + + const list = document.createElement('ul'); + list.className = 'commitment-summary-lines'; + for (const l of lines.slice(0, MAX_LISTED_LINES)) { + const li = document.createElement('li'); + li.textContent = describe(l); + list.appendChild(li); + } + if (lines.length > MAX_LISTED_LINES) { + const li = document.createElement('li'); + li.textContent = `and ${lines.length - MAX_LISTED_LINES} more`; + list.appendChild(li); + } + root.appendChild(list); + return root; +} + +/** Intro sentence followed by the summary block, as one dialog body. */ +export function withIntro(intro: string, summary: HTMLElement): HTMLElement { + const root = document.createElement('div'); + const p = document.createElement('p'); + p.textContent = intro; + root.append(p, summary); + return root; +} diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 049533ea4..9b9f5f3e2 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -4,6 +4,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 { showToast } from './toast'; import { confirmDialog } from './confirmDialog'; @@ -775,26 +776,58 @@ function getPlannedPurchaseStatusClass(status: string): string { } } +// Purchase ids whose "Run now" confirm or request is open, so a second click +// on the row button cannot stack a second dialog or send a second request. +const runNowInFlight = new Set(); + +function runNowBody(target: PlannedPurchase | undefined): string | HTMLElement { + if (!target) return 'This immediately executes the purchase.'; + if (target.has_recommendations === false) { + return 'This step has no recommendations attached, so running it will not buy anything.'; + } + const summary = buildCommitmentSummary([{ + service: target.service, + provider: target.provider, + region: target.region, + resourceType: target.resource_type, + count: target.count, + term: target.term, + payment: target.payment, + upfront: target.upfront_cost, + // Planned purchases carry no recurring-cost breakdown. + monthly: null, + }]); + return withIntro(`This immediately buys the commitment below for plan "${target.plan_name}", scheduled for ${formatDate(target.scheduled_date)}. It cannot be undone.`, summary); +} + +async function runPlannedPurchaseNow(purchaseId: string): Promise { + if (runNowInFlight.has(purchaseId)) return; + runNowInFlight.add(purchaseId); + try { + const target = lastLoadedPurchases.find(p => p.id === purchaseId); + const ok = await confirmDialog({ + title: target ? `Run step ${target.step_number}/${target.total_steps} of "${target.plan_name}" now?` : 'Run purchase now?', + body: runNowBody(target), + confirmLabel: 'Run now', + destructive: true, + }); + if (!ok) return; + await api.runPlannedPurchase(purchaseId); + showToast({ message: 'Purchase executed successfully', kind: 'success', timeout: 5_000 }); + } finally { + runNowInFlight.delete(purchaseId); + } +} + /** * Handle planned purchase action */ async function handlePlannedPurchaseAction(action: string, purchaseId: string, planId = ''): Promise { try { switch (action) { - case 'run': { - // Use styled async dialog (11-L2) instead of blocking browser confirm(). - const runOk = await confirmDialog({ - title: 'Run purchase now?', - body: 'This will immediately execute the purchase.', - confirmLabel: 'Run now', - destructive: true, - }); - if (runOk) { - await api.runPlannedPurchase(purchaseId); - showToast({ message: 'Purchase executed successfully', kind: 'success', timeout: 5_000 }); - } + case 'run': + await runPlannedPurchaseNow(purchaseId); break; - } case 'pause': // Pause is reversible and scoped to a single execution: the plan stays // enabled (unlike Disable plan) and the row stays listed with a Paused diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 12674fd93..35dfba0f9 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -1847,12 +1847,24 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy try { confirmed = await confirmDialog({ title: 'Execute RI Exchange', - body: `Quoted payment: ${quote.CurrencyCode} ${quote.PaymentDueRaw}. ` - + `Source RIs: ${request.ri_ids.join(', ')}. Region: ${quote.Region}. ` - + `Targets: ${targets.map(target => `${target.count} × ${target.offering_id}`).join(', ')}. ` - + 'This exchange executes immediately and cannot be reversed.', + body: buildExchangeConfirmBody({ + quote, + sources: request.ri_ids.map(id => { + const ri = currentRIs.find(r => r.reserved_instance_id === id); + return ri ? `${ri.instance_count} \u00d7 ${ri.instance_type} (${id})` : id; + }), + targets: targets.map(target => { + const o = awsOfferings.find(x => x.offering_id === target.offering_id); + if (o) return `${target.count} \u00d7 ${o.instance_type} (${o.offering_type})`; + const alt = alternativeTargets?.find(x => x.offering_id === target.offering_id); + return alt + ? `${target.count} \u00d7 ${alt.instance_type}` + : `${target.count} \u00d7 offering ${target.offering_id}`; + }), + }), confirmLabel: 'Execute Exchange', destructive: true, + initialFocus: 'cancel', }); if (!confirmed) return; setResultText(resultContainer, 'Executing exchange...', 'loading'); @@ -1884,6 +1896,45 @@ 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)}/hr, about ${sign}${(abs * HOURS_PER_MONTH).toFixed(2)}/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'); diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index d9f1fe6f0..5af4fd1e9 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -1696,3 +1696,35 @@ tr.rec-variant-row:hover td:not(.checkbox-col) { text-align: right; font-variant-numeric: tabular-nums; } + +/* Money confirm dialogs: shared stat band (commitmentSummary.ts). */ +.commitment-summary-band { + display: grid; + grid-template-columns: repeat(auto-fit, minmax(120px, 1fr)); + gap: 0.5rem; + margin: 0.75rem 0; +} + +.commitment-summary-stat { + display: flex; + flex-direction: column; + padding: 0.5rem 0.75rem; + background: var(--cudly-surface-muted); + border-radius: 6px; +} + +.commitment-summary-label { + font-size: 0.8rem; + color: var(--cudly-text-muted); +} + +.commitment-summary-value { + font-size: 1.15rem; + font-variant-numeric: tabular-nums; +} + +.commitment-summary-lines { + margin: 0.5rem 0 0; + padding-left: 1.1rem; + font-size: 0.9rem; +} From 919881dc6a46deb869ec504de778c091457ad952 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:01:34 +0200 Subject: [PATCH 2/5] fix(frontend): keep the exact offering id in the RI exchange confirm The confirm shows the instance type first and the offering id next to it, so the exact target stays verifiable. Refs #709 Co-Authored-By: Claude Sonnet 5.5 --- frontend/src/__tests__/riexchange.test.ts | 3 ++- frontend/src/riexchange.ts | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 798ccdd0b..ad2f11cb7 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -249,7 +249,8 @@ describe('openExchangeModal', () => { const text = document.querySelector('.modal-confirm-body')?.textContent ?? ''; expect(text).toContain('Pay now: USD 12.50'); expect(text).toContain('3 \u00d7 m5.large'); - expect(text).not.toContain(offeringUUID); + // The exact offering id stays visible next to the instance type. + expect(text).toContain(`3 \u00d7 m5.large (offering ${offeringUUID})`); expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250/hr, about +18.25/month)'); expect(document.activeElement?.textContent).toBe('Cancel'); document.querySelector('.modal-confirm-close')?.click(); diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 35dfba0f9..c057be884 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -1855,10 +1855,10 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy }), targets: targets.map(target => { const o = awsOfferings.find(x => x.offering_id === target.offering_id); - if (o) return `${target.count} \u00d7 ${o.instance_type} (${o.offering_type})`; + if (o) return `${target.count} \u00d7 ${o.instance_type} (${o.offering_type}, offering ${target.offering_id})`; const alt = alternativeTargets?.find(x => x.offering_id === target.offering_id); return alt - ? `${target.count} \u00d7 ${alt.instance_type}` + ? `${target.count} \u00d7 ${alt.instance_type} (offering ${target.offering_id})` : `${target.count} \u00d7 offering ${target.offering_id}`; }), }), From e783e1cffbc04eff295518e11457f837a4b7b627 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:06:44 +0200 Subject: [PATCH 3/5] fix(frontend): label approval-path amounts and flag unreported monthly fees The approval confirm shows "Upfront on approval" instead of "Charged today", since nothing is charged until the approver acts. A line that is not all upfront with no monthly figure says monthly fees apply. The exchange price change names its currency. Refs #707, #709 Co-Authored-By: Claude Sonnet 5.5 --- .../src/__tests__/commitmentSummary.test.ts | 17 +++++++++++++++++ .../__tests__/purchase-modal-submit.test.ts | 14 ++++++++++++++ frontend/src/__tests__/riexchange.test.ts | 2 +- frontend/src/app.ts | 1 + frontend/src/commitmentSummary.ts | 18 +++++++++++++++--- frontend/src/riexchange.ts | 2 +- 6 files changed, 49 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/commitmentSummary.test.ts b/frontend/src/__tests__/commitmentSummary.test.ts index ee2f1c023..c08d55875 100644 --- a/frontend/src/__tests__/commitmentSummary.test.ts +++ b/frontend/src/__tests__/commitmentSummary.test.ts @@ -27,6 +27,23 @@ describe('commitmentTotals', () => { }); }); +describe('summary options and hints', () => { + it('relabels the upfront stat for the approval path', () => { + const text = buildCommitmentSummary([line({ upfront: 8400 })], { upfrontLabel: 'Upfront on approval' }).textContent; + expect(text).toContain('Upfront on approval$8,400.00'); + expect(text).not.toContain('Charged today'); + }); + + it('hints that monthly fees apply for non all-upfront lines with no monthly figure', () => { + expect(buildCommitmentSummary([line({ monthly: null, payment: 'partial-upfront' })]).textContent) + .toContain('MonthlyNot reported (monthly fees apply)'); + expect(buildCommitmentSummary([line({ monthly: null, payment: 'all-upfront' })]).textContent) + .toContain('MonthlyNot reported'); + expect(buildCommitmentSummary([line({ monthly: null, payment: 'all-upfront' })]).textContent) + .not.toContain('fees apply'); + }); +}); + describe('paymentLabel', () => { it.each([ ['all-upfront', 'All upfront'], diff --git a/frontend/src/__tests__/purchase-modal-submit.test.ts b/frontend/src/__tests__/purchase-modal-submit.test.ts index efa5810b4..21b50e881 100644 --- a/frontend/src/__tests__/purchase-modal-submit.test.ts +++ b/frontend/src/__tests__/purchase-modal-submit.test.ts @@ -299,6 +299,20 @@ describe('Issue #251: Execute Now makes no cancellation promise', () => { expect(api.executePurchase).not.toHaveBeenCalled(); }); + test('approval confirm labels the amount as upfront on approval, not charged today', async () => { + const rec: LocalRecommendation = { ...buildRows()[0]!, provider: 'aws', service: 'ec2', term: 1 }; + (state.getRecommendations as jest.Mock).mockReturnValue([rec]); + (confirmDialog as jest.Mock).mockClear(); + await openPurchaseModal([rec]); + (document.getElementById('execute-purchase-btn') as HTMLButtonElement).click(); + await flush(); + const opts = (confirmDialog as jest.Mock).mock.calls[0]![0] as { body: HTMLElement }; + const text = opts.body.textContent ?? ''; + expect(text).toContain('Nothing is charged until the approver'); + expect(text).toContain(`Upfront on approval${formatCurrency(rec.upfront_cost, '$', 2)}`); + expect(text).not.toContain('Charged today'); + }); + test('direct-execute confirm dialog makes no cancellation promise', async () => { const rec: LocalRecommendation = { ...buildRows()[0]!, provider: 'aws', service: 'ec2', term: 1 }; (state.getRecommendations as jest.Mock).mockReturnValue([rec]); diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index ad2f11cb7..8ec4b2ef3 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -251,7 +251,7 @@ describe('openExchangeModal', () => { expect(text).toContain('3 \u00d7 m5.large'); // The exact offering id stays visible next to the instance type. expect(text).toContain(`3 \u00d7 m5.large (offering ${offeringUUID})`); - expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250/hr, about +18.25/month)'); + expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250 USD/hr, about +18.25 USD/month)'); expect(document.activeElement?.textContent).toBe('Cancel'); document.querySelector('.modal-confirm-close')?.click(); }); diff --git a/frontend/src/app.ts b/frontend/src/app.ts index a4b9cee92..b69b82051 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -412,6 +412,7 @@ export async function handleExecutePurchase(): Promise { monthly: r.monthly_cost ?? null, ...(r.cloud_account_id ? { account: getAccountName(r.cloud_account_id) } : {}), })), + isDirect ? {} : { upfrontLabel: 'Upfront on approval' }, ); const ok = isDirect ? await confirmDialog({ diff --git a/frontend/src/commitmentSummary.ts b/frontend/src/commitmentSummary.ts index f3c1e5503..ffbc4b3ed 100644 --- a/frontend/src/commitmentSummary.ts +++ b/frontend/src/commitmentSummary.ts @@ -72,6 +72,13 @@ function stat(label: string, value: string): HTMLElement { return el; } +// A partial or no-upfront commitment bills monthly; saying only "not +// reported" would read as "no monthly fee". +function monthlyUnknownText(lines: readonly CommitmentLine[]): string { + const bills = lines.some(l => paymentLabel(l.payment) !== 'All upfront'); + return bills ? 'Not reported (monthly fees apply)' : 'Not reported'; +} + function describe(l: CommitmentLine): string { const where = [l.account, l.region].filter(Boolean).join(', '); const what = `${l.count} × ${l.service} ${l.resourceType}`; @@ -79,7 +86,12 @@ function describe(l: CommitmentLine): string { return [what, terms, where].filter(Boolean).join(' · '); } -export function buildCommitmentSummary(lines: readonly CommitmentLine[]): HTMLElement { +export interface SummaryOptions { + /** Label for the upfront stat; 'Upfront on approval' when nothing is charged yet. */ + upfrontLabel?: string; +} + +export function buildCommitmentSummary(lines: readonly CommitmentLine[], opts: SummaryOptions = {}): HTMLElement { const totals = commitmentTotals(lines); const root = document.createElement('div'); root.className = 'commitment-summary'; @@ -87,8 +99,8 @@ export function buildCommitmentSummary(lines: readonly CommitmentLine[]): HTMLEl const band = document.createElement('div'); band.className = 'commitment-summary-band'; band.append( - stat('Charged today', formatCurrency(totals.upfront, '$', 2)), - stat('Monthly', totals.monthly === null ? 'Not reported' : formatCurrency(totals.monthly, '$', 2)), + stat(opts.upfrontLabel ?? 'Charged today', formatCurrency(totals.upfront, '$', 2)), + stat('Monthly', totals.monthly === null ? monthlyUnknownText(lines) : formatCurrency(totals.monthly, '$', 2)), stat('Total over term', totals.termTotal === null ? 'Not available' : formatCurrency(totals.termTotal, '$', 2)), ); root.appendChild(band); diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index c057be884..109e48478 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -1926,7 +1926,7 @@ function buildExchangeConfirmBody({ quote, sources, targets }: ExchangeConfirmPa const abs = Math.abs(delta); line( 'Hourly price change', - `${quote.CurrencyCode} ${src.toFixed(4)} \u2192 ${tgt.toFixed(4)} (${sign}${abs.toFixed(4)}/hr, about ${sign}${(abs * HOURS_PER_MONTH).toFixed(2)}/month)`, + `${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'); From d015b9327964e1a669a88239b7f397a7b0014df7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:13:08 +0200 Subject: [PATCH 4/5] test(frontend): e2e asserts instance type and offering id in the exchange confirm Refs #709 Co-Authored-By: Claude Sonnet 5.5 --- frontend/tests-e2e/ri-exchange-confirmation.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts index 060679ba9..e61ebe377 100644 --- a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts +++ b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts @@ -104,10 +104,10 @@ for (const multiple of [false, true]) { } await execute(page).click(); await expect(dialog(page)).toContainText(`USD ${quotedPayment}`); - await expect(dialog(page)).toContainText(`2 × ${offering}`); + await expect(dialog(page)).toContainText(`2 × m5.xlarge (No Upfront, offering ${offering})`); await expect(dialog(page)).toContainText(ri); await expect(dialog(page)).toContainText('us-east-1'); - if (multiple) await expect(dialog(page)).toContainText(`3 × ${secondOffering}`); + if (multiple) await expect(dialog(page)).toContainText(`3 × m5.xlarge (No Upfront, offering ${secondOffering})`); expect(state.requests).toHaveLength(0); // Destructive dialogs open on the safe action, so a reflex Enter cancels. await expect(dialog(page).getByRole('button', { name: 'Cancel', exact: true })).toBeFocused(); From 27d5801273d220e0def606141c8f4c82dbc213a8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:16:13 +0200 Subject: [PATCH 5/5] fix(frontend): recognise the Azure upfront and monthly payment spellings Refs #707 Co-Authored-By: Claude Sonnet 5.5 --- frontend/src/__tests__/commitmentSummary.test.ts | 11 ++++++++++- frontend/src/commitmentSummary.ts | 2 ++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/commitmentSummary.test.ts b/frontend/src/__tests__/commitmentSummary.test.ts index c08d55875..03aa4ded6 100644 --- a/frontend/src/__tests__/commitmentSummary.test.ts +++ b/frontend/src/__tests__/commitmentSummary.test.ts @@ -34,6 +34,13 @@ describe('summary options and hints', () => { expect(text).not.toContain('Charged today'); }); + it('treats the Azure "upfront" spelling as all upfront (no monthly-fee hint)', () => { + const text = buildCommitmentSummary([line({ monthly: null, payment: 'upfront' })]).textContent; + expect(text).toContain('MonthlyNot reported'); + expect(text).not.toContain('fees apply'); + expect(text).toContain('All upfront'); + }); + it('hints that monthly fees apply for non all-upfront lines with no monthly figure', () => { expect(buildCommitmentSummary([line({ monthly: null, payment: 'partial-upfront' })]).textContent) .toContain('MonthlyNot reported (monthly fees apply)'); @@ -49,7 +56,9 @@ describe('paymentLabel', () => { ['all-upfront', 'All upfront'], ['partial_upfront', 'Partial upfront'], ['no-upfront', 'No upfront'], - ['monthly', 'monthly'], + ['upfront', 'All upfront'], + ['monthly', 'Monthly'], + ['custom', 'custom'], ])('%s -> %s', (raw, label) => expect(paymentLabel(raw)).toBe(label)); }); diff --git a/frontend/src/commitmentSummary.ts b/frontend/src/commitmentSummary.ts index ffbc4b3ed..84b0762d6 100644 --- a/frontend/src/commitmentSummary.ts +++ b/frontend/src/commitmentSummary.ts @@ -52,9 +52,11 @@ export function commitmentTotals(lines: readonly CommitmentLine[]): CommitmentTo export function paymentLabel(payment: string): string { switch (payment.replace(/_/g, '-')) { + case 'upfront': // Azure spelling of all-upfront case 'all-upfront': return 'All upfront'; case 'partial-upfront': return 'Partial upfront'; case 'no-upfront': return 'No upfront'; + case 'monthly': return 'Monthly'; default: return payment; } }