From 98964d7c3ebdb929bd17de949f1adb730d52eda4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 10:49:18 +0200 Subject: [PATCH] fix(frontend): never offer Approve without the purchase amount buildApprovalDetailsBody caught every getPurchaseDetails failure, and the empty-recommendations case, and returned a one-sentence body with no upfront cost or savings. Both callers (the email deep-link and the History and approval-queue Approve buttons) then opened the confirm dialog with a live "Approve purchase" button, so a 403, 404 or network error asked the user to commit money without showing how much. The builder now rejects in those cases. The deep-link shows a sticky error toast and leaves the page without opening the dialog; the History button shows an error toast and re-enables the row so the user can retry. The backend approves the stored execution without an amount check, so the modal is the only place the amount is shown before the commitment. Closes #247 --- .../src/__tests__/approval-details.test.ts | 8 -- .../__tests__/history-approval-queue.test.ts | 7 ++ .../__tests__/history-approve-button.test.ts | 33 ++++++- .../src/__tests__/purchases-deeplink.test.ts | 71 ++++++++++++-- frontend/src/approval-details.ts | 94 +++++++------------ frontend/src/history.ts | 14 ++- frontend/src/purchases-deeplink.ts | 15 ++- frontend/src/styles/components.css | 7 -- .../history-action-confirmation.spec.ts | 33 ++++++- 9 files changed, 188 insertions(+), 94 deletions(-) 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');