diff --git a/frontend/src/__tests__/approval-details.test.ts b/frontend/src/__tests__/approval-details.test.ts index ca4d19e8f..67e1f10cb 100644 --- a/frontend/src/__tests__/approval-details.test.ts +++ b/frontend/src/__tests__/approval-details.test.ts @@ -240,14 +240,6 @@ describe('renderApprovalDetailsBody', () => { expect(stats[5]?.querySelector('.approval-details-stat-value')?.textContent).toBe('0'); }); - it('falls back to a legacy text sentence when recommendations are empty', () => { - const details = makeDetails([]); - const body = renderApprovalDetailsBody(details, new Map()); - expect(body.querySelector('.approval-details-fallback')).not.toBeNull(); - expect(body.querySelector('.approval-details-table')).toBeNull(); - expect(body.textContent).toContain('Cloud commitments will be charged'); - }); - it('escapes user-controlled string values in the table', () => { const rec = makeRec({ resource_type: '' }); const body = renderApprovalDetailsBody(makeDetails([rec]), new Map()); diff --git a/frontend/src/__tests__/history-approval-queue.test.ts b/frontend/src/__tests__/history-approval-queue.test.ts index 5e787f7ff..c2e6da911 100644 --- a/frontend/src/__tests__/history-approval-queue.test.ts +++ b/frontend/src/__tests__/history-approval-queue.test.ts @@ -27,6 +27,13 @@ jest.mock('../api', () => ({ getConfig: jest.fn().mockResolvedValue({ global: {} }), approvePurchase: jest.fn(), cancelPurchase: jest.fn(), + getPurchaseDetails: jest.fn().mockResolvedValue({ + total_upfront_cost: 100, + estimated_savings: 10, + recommendations: [{ provider: 'aws', service: 'ec2', region: 'us-east-1', count: 1, term: 1, upfront_cost: 100, savings: 10 }], + }), + listAccounts: jest.fn().mockResolvedValue([]), + getDeploymentInfo: jest.fn().mockResolvedValue({}), })); jest.mock('../navigation', () => ({ diff --git a/frontend/src/__tests__/history-approve-button.test.ts b/frontend/src/__tests__/history-approve-button.test.ts index 295b4d536..f8fd3c892 100644 --- a/frontend/src/__tests__/history-approve-button.test.ts +++ b/frontend/src/__tests__/history-approve-button.test.ts @@ -25,7 +25,7 @@ jest.mock('../api', () => ({ getConfig: jest.fn().mockResolvedValue({ global: {} }), approvePurchase: jest.fn(), cancelPurchase: jest.fn(), - getPurchaseDetails: jest.fn().mockResolvedValue({ recommendations: [] }), + getPurchaseDetails: jest.fn(), listAccounts: jest.fn().mockResolvedValue([]), getDeploymentInfo: jest.fn().mockResolvedValue({}), })); @@ -112,6 +112,11 @@ const REG_USER = { ], }; const OTHER_UUID = 'other-uuid'; +const DETAILS = { + total_upfront_cost: 100, + estimated_savings: 10, + recommendations: [{ provider: 'aws', service: 'ec2', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', upfront_cost: 100, savings: 10 }], +}; function deferred(): { promise: Promise; resolve(value: T): void; reject(error: Error): void } { let resolve!: (value: T) => void; @@ -129,14 +134,14 @@ describe('History action ownership and refresh recovery (#249)', () => { jest.clearAllMocks(); (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); (api.getHistory as jest.Mock).mockReset().mockResolvedValue(data); - (api.getPurchaseDetails as jest.Mock).mockReset().mockResolvedValue({ recommendations: [] }); + (api.getPurchaseDetails as jest.Mock).mockReset().mockResolvedValue(DETAILS); (api.approvePurchase as jest.Mock).mockReset().mockResolvedValue(undefined); (confirmDialog as jest.Mock).mockReset().mockResolvedValue(true); (showToast as jest.Mock).mockReset(); }); test('details fetch owns both actions and same-ID projections while other IDs remain independent', async () => { - const details = deferred<{ recommendations: never[] }>(); + const details = deferred(); (api.getPurchaseDetails as jest.Mock).mockReturnValue(details.promise); (api.getHistory as jest.Mock).mockResolvedValue({ ...data, purchases: [...data.purchases, makeRow({ purchase_id: 'other' })] }); (confirmDialog as jest.Mock).mockResolvedValue(false); @@ -148,7 +153,7 @@ describe('History action ownership and refresh recovery (#249)', () => { document.querySelector('#purchases-approval-queue [data-approve-id="guarded"]')!.click(); document.querySelector('#history-list [data-approve-id="other"]')!.click(); const disabled = btn.disabled; - details.resolve({ recommendations: [] }); + details.resolve(DETAILS); await settleActions(); expect(disabled).toBe(true); expect(api.getPurchaseDetails).toHaveBeenCalledTimes(2); @@ -158,6 +163,25 @@ describe('History action ownership and refresh recovery (#249)', () => { expect(btn.disabled).toBe(false); }); + test.each([ + ['the details fetch fails', () => (api.getPurchaseDetails as jest.Mock).mockRejectedValue(new Error('403 Forbidden'))], + ['the details carry no recommendations', () => (api.getPurchaseDetails as jest.Mock).mockResolvedValue({ ...DETAILS, recommendations: [] })], + ])('Approve is not offered when %s (issue #247)', async (_label, arrange) => { + arrange(); + console.error = jest.fn(); + await loadHistory(); + const btn = document.querySelector('#history-list .history-approve-btn')!; + btn.click(); + await settleActions(); + expect(confirmDialog).not.toHaveBeenCalled(); + expect(api.approvePurchase).not.toHaveBeenCalled(); + expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ + kind: 'error', + message: expect.stringContaining('cannot be approved without showing the amount'), + })); + expect(btn.disabled).toBe(false); + }); + test.each(['cancel', 'reject', 'throw', 'api-error'] as const)('%s releases ownership for a later explicit attempt', async outcome => { await loadHistory(); const btn = document.querySelector('#history-list .history-approve-btn')!; @@ -304,6 +328,7 @@ describe('History inline Approve button (issue #286)', () => { beforeEach(() => { setupDOM(); jest.clearAllMocks(); + (api.getPurchaseDetails as jest.Mock).mockResolvedValue(DETAILS); }); test('admin sees Approve on every pending row, regardless of creator', async () => { diff --git a/frontend/src/__tests__/purchases-deeplink.test.ts b/frontend/src/__tests__/purchases-deeplink.test.ts index 4c86c8609..477929a01 100644 --- a/frontend/src/__tests__/purchases-deeplink.test.ts +++ b/frontend/src/__tests__/purchases-deeplink.test.ts @@ -1,13 +1,70 @@ /** - * Deep-link parser tests. The handler itself is side-effectful (toast, - * confirmDialog, fetch, location.replaceState) and exercised end-to-end - * in manual smoke — here we pin the pure parser that decides whether a - * given URL *is* a deep-link, which is the piece most likely to - * regress when someone adds a new SPA route that overlaps with the - * `/purchases/` prefix. + * Deep-link parser and approve-handler tests. The parser decides whether a + * given URL *is* a deep-link; the handler tests drive the real toast and + * confirm dialog with only the network layer mocked. */ -import { parsePurchaseDeeplink } from '../purchases-deeplink'; +jest.mock('../api', () => ({ + getPurchaseDetails: jest.fn(), + listAccounts: jest.fn().mockResolvedValue([]), + getDeploymentInfo: jest.fn().mockResolvedValue({}), +})); +jest.mock('../api/client', () => ({ apiRequest: jest.fn() })); + +import * as api from '../api'; +import { apiRequest } from '../api/client'; +import { handlePurchaseDeeplink, parsePurchaseDeeplink } from '../purchases-deeplink'; + +const EXEC_ID = '11111111-1111-1111-1111-111111111111'; + +describe('handlePurchaseDeeplink approve (issue #247)', () => { + beforeEach(() => { + document.body.innerHTML = ''; + jest.clearAllMocks(); + (apiRequest as jest.Mock).mockResolvedValue({ status: 'approved' }); + window.history.replaceState({}, '', `/purchases/approve/${EXEC_ID}?token=tok-xyz`); + jest.spyOn(console, 'error').mockImplementation(() => undefined); + }); + + afterEach(() => jest.restoreAllMocks()); + + it.each([ + ['the details fetch fails', () => (api.getPurchaseDetails as jest.Mock).mockRejectedValue(new Error('403 Forbidden'))], + ['the details carry no recommendations', () => (api.getPurchaseDetails as jest.Mock).mockResolvedValue({ + execution_id: EXEC_ID, status: 'pending', total_upfront_cost: 0, estimated_savings: 0, recommendations: [], + })], + ])('does not offer Approve when %s', async (_label, arrange) => { + arrange(); + + await expect(handlePurchaseDeeplink()).resolves.toBe(true); + + expect(document.querySelector('.modal-confirm-backdrop')).toBeNull(); + const buttons = Array.from(document.querySelectorAll('button')).map(b => b.textContent); + expect(buttons).not.toContain('Approve purchase'); + const alert = document.querySelector('[role="alert"]'); + expect(alert?.textContent).toContain('cannot be approved without showing the amount'); + expect(apiRequest).not.toHaveBeenCalled(); + expect(window.location.pathname).toBe('/purchases'); + }); + + it('shows the upfront amount and approves when details load', async () => { + (api.getPurchaseDetails as jest.Mock).mockResolvedValue({ + execution_id: EXEC_ID, status: 'pending', total_upfront_cost: 1200, estimated_savings: 10, + recommendations: [{ id: 'r1', provider: 'aws', service: 'ec2', region: 'us-east-1', resource_type: 'm5.large', + count: 1, term: 1, payment: 'all-upfront', upfront_cost: 1200, monthly_cost: 0, savings: 10, selected: true, purchased: false }], + }); + + const done = handlePurchaseDeeplink(); + await new Promise(resolve => setTimeout(resolve, 0)); + expect(document.querySelector('.approval-details-header')?.textContent).toContain('$1,200'); + const approve = Array.from(document.querySelectorAll('.modal-confirm-actions button')) + .find(b => b.textContent === 'Approve purchase'); + approve!.click(); + await done; + + expect(apiRequest).toHaveBeenCalledWith(`/purchases/approve/${EXEC_ID}`, expect.objectContaining({ method: 'POST' })); + }); +}); describe('parsePurchaseDeeplink', () => { it('parses an approve deep-link with a token', () => { diff --git a/frontend/src/approval-details.ts b/frontend/src/approval-details.ts index b64010e53..61d756e11 100644 --- a/frontend/src/approval-details.ts +++ b/frontend/src/approval-details.ts @@ -18,12 +18,9 @@ * The pure builder `renderApprovalDetailsBody(details, accountsById)` * is split out from the network-fetching wrapper * `buildApprovalDetailsBody(executionId)` so unit tests can exercise - * the rendering without mocking fetch. Both code paths share the same - * fallback shape: when the GET fails (404, 403, network error) we - * return a plain-text element preserving the legacy behaviour so the - * dialog still opens and the Approve button still works — just without - * the details. Showing a half-built modal would be worse UX than - * gracefully falling back. + * the rendering without mocking fetch. When the details cannot be loaded + * or carry no recommendations the wrapper rejects, so callers never offer + * Approve without the amount (issue #247). */ import * as api from './api'; @@ -44,8 +41,7 @@ export type AccountsById = Map; /** * Build an HTMLElement to pass as `body` to confirmDialog when the * user is about to approve a purchase. Renders the rich details from - * `details.recommendations`, falling back to a plain text element - * (with the legacy approval sentence) when no recs are present. + * `details.recommendations`. * * Exported separately from `buildApprovalDetailsBody` so tests can * render the body deterministically — no fetch mocking required. @@ -55,16 +51,6 @@ export function renderApprovalDetailsBody(details: PurchaseDetails, accountsById root.className = 'approval-details'; const recs = details.recommendations ?? []; - if (recs.length === 0) { - // Direct-execute paths (capacity_percent flow) can sometimes - // race the JSONB write — surface the legacy sentence so the user - // can still confirm rather than blocking the click on a missing - // payload. Shared helper keeps the fallback text identical to - // the network-failure branch. - root.appendChild(buildApprovalDetailsFallback()); - return root; - } - root.appendChild(renderApprovalDetailsHeader(details, recs, hostAWSAccountID)); root.appendChild(renderApprovalDetailsTable(recs, accountsById, hostAWSAccountID)); return root; @@ -332,50 +318,40 @@ export function computeEffectiveSavingsPct(rec: Recommendation): number | null { /** * Network-fetching wrapper. Fetches `/api/purchases/{id}` + the * accounts list in parallel, builds the rich body, and returns it. - * Any fetch failure falls back to a plain-text element carrying the - * legacy approval sentence so confirmDialog still works. + * Rejects when the purchase details fail to load or carry no + * recommendations; callers must then refuse to offer Approve. */ export async function buildApprovalDetailsBody(executionId: string): Promise { - try { - // Fetch purchase details, account list, and public info in parallel. - // listAccounts and getPublicInfo are caught inline so the modal still - // renders the full details when either endpoint is unreachable; the - // per-rec table degrades gracefully. console.warn keeps failures - // traceable rather than silently dropping them. - const [details, accounts, deploymentInfo] = await Promise.all([ - api.getPurchaseDetails(executionId), - api.listAccounts().catch((err) => { - console.warn('Failed to load accounts for approval modal — falling back to UUID-prefixed labels:', err); - return [] as CloudAccount[]; - }), - // getDeploymentInfo requires an authenticated session (AuthUser). The - // approval modal is only reachable post-login, so this is safe. On - // failure (e.g. token expiry) the orphan label degrades to "Account deleted" - // which is the safe default (#633). - api.getDeploymentInfo().catch((err) => { - console.warn('Failed to load deployment info for approval modal — orphan label will show "Account deleted":', err); - return undefined; - }), - ]); - const accountsById = new Map(); - for (const acct of accounts) accountsById.set(acct.id, acct); - const hostAWSAccountID = deploymentInfo?.deployment_aws_account_id; - return renderApprovalDetailsBody(details, accountsById, hostAWSAccountID); - } catch (err) { - console.error('Failed to load purchase details for approval modal:', err); - return buildApprovalDetailsFallback(); + // Fetch purchase details, account list, and public info in parallel. + // listAccounts and getPublicInfo are caught inline so the modal still + // renders the full details when either endpoint is unreachable; the + // per-rec table degrades gracefully. console.warn keeps failures + // traceable rather than silently dropping them. + const [details, accounts, deploymentInfo] = await Promise.all([ + api.getPurchaseDetails(executionId), + api.listAccounts().catch((err) => { + console.warn('Failed to load accounts for approval modal — falling back to UUID-prefixed labels:', err); + return [] as CloudAccount[]; + }), + // getDeploymentInfo requires an authenticated session (AuthUser). The + // approval modal is only reachable post-login, so this is safe. On + // failure (e.g. token expiry) the orphan label degrades to "Account deleted" + // which is the safe default (#633). + api.getDeploymentInfo().catch((err) => { + console.warn('Failed to load deployment info for approval modal — orphan label will show "Account deleted":', err); + return undefined; + }), + ]); + if (!details.recommendations?.length) { + throw new Error('the purchase details contain no recommendations'); } + const accountsById = new Map(); + for (const acct of accounts) accountsById.set(acct.id, acct); + const hostAWSAccountID = deploymentInfo?.deployment_aws_account_id; + return renderApprovalDetailsBody(details, accountsById, hostAWSAccountID); } -/** - * Build the legacy approval sentence as a standalone fallback - * element. Extracted so the two failure paths (empty recommendations - * + fetch failure) render identical text and class hooks; previously - * the literal string lived in two places and could drift. - */ -function buildApprovalDetailsFallback(): HTMLElement { - const fallback = document.createElement('div'); - fallback.className = 'approval-details-fallback'; - fallback.textContent = 'This authorises the purchase to execute. Cloud commitments will be charged once the executor picks up the approved row.'; - return fallback; +export function approvalDetailsUnavailableMessage(err: unknown): string { + const cause = err instanceof Error ? err.message : String(err); + return `This purchase cannot be approved without showing the amount, and its details could not be loaded (${cause}).`; } diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 42ac0ef5a..b42ee6acb 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -9,7 +9,7 @@ import type { HistoryResponse, HistorySummary, HistoryPurchase } from './types'; import type { RevokeQuote } from './api'; import { switchTab } from './navigation'; import { confirmDialog } from './confirmDialog'; -import { buildApprovalDetailsBody } from './approval-details'; +import { approvalDetailsUnavailableMessage, buildApprovalDetailsBody } from './approval-details'; import { showToast } from './toast'; import { getCurrentUser } from './state'; import { canAccess } from './permissions'; @@ -1295,9 +1295,15 @@ function wireRowActionHandlers(container: HTMLElement): void { // Issue #374: show the per-rec details (service / engine / // resource / region / count / term + payment / costs) in the // modal so the user has informed consent before authorising a - // financial commitment. buildApprovalDetailsBody falls back to - // the legacy text sentence if the GET fails. - const detailsBody = await buildApprovalDetailsBody(id); + // financial commitment. Without them, Approve is not offered (#247). + let detailsBody: HTMLElement; + try { + detailsBody = await buildApprovalDetailsBody(id); + } catch (detailsError) { + console.error('Failed to load purchase details for approval:', detailsError); + showToast({ message: `${approvalDetailsUnavailableMessage(detailsError)} Try again.`, kind: 'error' }); + return; + } const ok = await confirmDialog({ title: 'Approve this pending purchase?', body: detailsBody, diff --git a/frontend/src/purchases-deeplink.ts b/frontend/src/purchases-deeplink.ts index f194d8a50..d0cf920d6 100644 --- a/frontend/src/purchases-deeplink.ts +++ b/frontend/src/purchases-deeplink.ts @@ -21,7 +21,7 @@ import { apiRequest } from './api/client'; import { showToast } from './toast'; import { confirmDialog } from './confirmDialog'; -import { buildApprovalDetailsBody } from './approval-details'; +import { approvalDetailsUnavailableMessage, buildApprovalDetailsBody } from './approval-details'; type DeeplinkAction = 'approve' | 'cancel'; @@ -83,7 +83,18 @@ export async function handlePurchaseDeeplink(): Promise { // approval is non-destructive to AWS. let body: string | HTMLElement; if (dl.action === 'approve') { - body = await buildApprovalDetailsBody(dl.id); + try { + body = await buildApprovalDetailsBody(dl.id); + } catch (err) { + console.error('Failed to load purchase details for approval:', err); + showToast({ + message: `${approvalDetailsUnavailableMessage(err)} Open the approval link again to retry.`, + kind: 'error', + timeout: null, + }); + window.history.replaceState({}, '', '/purchases'); + return true; + } } else { body = `You're about to ${gerund} purchase execution ${dl.id}. This action will be recorded against your logged-in account.`; } diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index 67ebba245..d9f1fe6f0 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -1048,13 +1048,6 @@ tr.rec-variant-row:hover td:not(.checkbox-col) { gap: 1rem; } -.approval-details-fallback { - margin: 0; - color: #444; - line-height: 1.5; - font-size: 0.95rem; -} - .approval-details-header { display: grid; grid-template-columns: repeat(auto-fit, minmax(120px, 1fr)); diff --git a/frontend/tests-e2e/history-action-confirmation.spec.ts b/frontend/tests-e2e/history-action-confirmation.spec.ts index e5e212a42..857db70df 100644 --- a/frontend/tests-e2e/history-action-confirmation.spec.ts +++ b/frontend/tests-e2e/history-action-confirmation.spec.ts @@ -5,6 +5,12 @@ const ID = '11111111-1111-1111-1111-111111111111'; const actions = ['approve', 'cancel', 'retry', 'revoke', 'marketplace-sell', 'marketplace-cancel'] as const; type Action = typeof actions[number]; +const DETAILS = { + execution_id: ID, status: 'pending', total_upfront_cost: 1200, estimated_savings: 10, + recommendations: [{ id: 'r1', provider: 'aws', service: 'ec2', region: 'us-east-1', resource_type: 'm5.large', + count: 1, term: 1, payment: 'all-upfront', upfront_cost: 1200, monthly_cost: 0, savings: 10 }], +}; + function gate(): { promise: Promise; release(): void } { let release!: () => void; const promise = new Promise(resolve => { release = resolve; }); @@ -40,7 +46,7 @@ async function historyFixture(page: Page, action: Action) { } else { await route.fulfill({ json: route.request().url().includes('/calculate') ? { refund_amount: 100, refund_currency: 'USD' } - : { execution_id: ID, status: 'pending', recommendations: [] } }); + : DETAILS }); } }); return { row, posts, failNextMutation: () => { failNext = true; } }; @@ -95,7 +101,7 @@ test('actual double-click and second projection cannot duplicate pending approva await page.route(`**/api/purchases/${ID}`, async route => { detailGets++; await details.promise; - await route.fulfill({ json: { execution_id: ID, status: 'pending', recommendations: [] } }); + await route.fulfill({ json: DETAILS }); }); await page.goto('/purchases/history'); const btn = page.locator('#history-list .history-approve-btn'); @@ -110,13 +116,34 @@ test('actual double-click and second projection cannot duplicate pending approva await expect.poll(() => fixture.posts.length).toBe(1); }); +test('approve is not offered when the purchase details cannot be loaded (issue #247)', async ({ page }) => { + const fixture = await historyFixture(page, 'approve'); + let detailGets = 0; + await page.route(`**/api/purchases/${ID}`, route => { + detailGets++; + return detailGets === 1 + ? route.fulfill({ status: 403, json: { error: 'forbidden' } }) + : route.fulfill({ json: DETAILS }); + }); + await page.goto('/purchases/history'); + const btn = page.locator('#history-list .history-approve-btn'); + await btn.click(); + await expect(page.getByRole('alert')).toContainText('cannot be approved without showing the amount'); + await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(0); + await expect(btn).toBeEnabled(); + await btn.click(); + await expect(page.locator('.modal-confirm-body')).toContainText('$1,200'); + await page.keyboard.press('Enter'); + await expect.poll(() => fixture.posts.length).toBe(1); +}); + test('canceling an independent row does not steal focus from another confirmation', async ({ page }) => { const fixture = await historyFixture(page, 'approve'); const details = gate(); await page.route('**/api/history**', route => route.fulfill({ json: { summary: {}, purchases: [fixture.row, { ...fixture.row, purchase_id: 'other-execution' }] } })); await page.route('**/api/purchases/*', async route => { await details.promise; - await route.fulfill({ json: { status: 'pending', recommendations: [] } }); + await route.fulfill({ json: DETAILS }); }); await page.goto('/purchases/history'); const buttons = page.locator('#history-list .history-approve-btn');