From a10b82e4fece9d9a3fdfb793101a2abe22e1f4bc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 00:30:57 +0200 Subject: [PATCH 1/2] fix(approval): stop telling permanent 403/404 details failures to retry A 403/404 on the approval details fetch means the user lacks view:purchases or account access, so retrying cannot succeed. Say so for those statuses and keep the retry wording for network errors and 5xx. Refs #529. --- .../src/__tests__/purchases-deeplink.test.ts | 21 +++++++++++++++++++ frontend/src/approval-details.ts | 9 ++++++++ frontend/src/history.ts | 6 ++++-- frontend/src/purchases-deeplink.ts | 6 ++++-- 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/purchases-deeplink.test.ts b/frontend/src/__tests__/purchases-deeplink.test.ts index 477929a01..b0ecb3756 100644 --- a/frontend/src/__tests__/purchases-deeplink.test.ts +++ b/frontend/src/__tests__/purchases-deeplink.test.ts @@ -47,6 +47,27 @@ describe('handlePurchaseDeeplink approve (issue #247)', () => { expect(window.location.pathname).toBe('/purchases'); }); + it.each([ + [403, true], + [404, true], + [500, false], + [undefined, false], + ])('details fetch failing with status %s: permission wording only=%s', async (status, permanent) => { + const err = Object.assign(new Error(status ? `HTTP ${status}` : 'Failed to fetch'), { status }); + (api.getPurchaseDetails as jest.Mock).mockRejectedValue(err); + + await expect(handlePurchaseDeeplink()).resolves.toBe(true); + + const text = document.querySelector('[role="alert"]')?.textContent ?? ''; + if (permanent) { + expect(text).toContain('view:purchases'); + expect(text).not.toContain('retry'); + } else { + expect(text).toContain('Open the approval link again to retry.'); + expect(text).not.toContain('view: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, diff --git a/frontend/src/approval-details.ts b/frontend/src/approval-details.ts index 61d756e11..2e34daf60 100644 --- a/frontend/src/approval-details.ts +++ b/frontend/src/approval-details.ts @@ -351,7 +351,16 @@ export async function buildApprovalDetailsBody(executionId: string): Promise { } catch (err) { console.error('Failed to load purchase details for approval:', err); showToast({ - message: `${approvalDetailsUnavailableMessage(err)} Open the approval link again to retry.`, + message: isPermanentDetailsError(err) + ? approvalDetailsUnavailableMessage(err) + : `${approvalDetailsUnavailableMessage(err)} Open the approval link again to retry.`, kind: 'error', timeout: null, }); From 5e57929e87b497afca78184ab94e18cbbe123490 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 00:42:45 +0200 Subject: [PATCH 2/2] fix(frontend): stop advising retry for permanent approval-details errors Keep the server message for 403 and 404, say 404 may mean not found or out of account access, leave History wording as on main, and make the e2e retry case use a retryable 500. --- .../src/__tests__/purchases-deeplink.test.ts | 36 +++++++++++-------- frontend/src/approval-details.ts | 13 ++++--- frontend/src/history.ts | 6 ++-- .../history-action-confirmation.spec.ts | 17 ++++++++- 4 files changed, 49 insertions(+), 23 deletions(-) diff --git a/frontend/src/__tests__/purchases-deeplink.test.ts b/frontend/src/__tests__/purchases-deeplink.test.ts index b0ecb3756..ee04a444c 100644 --- a/frontend/src/__tests__/purchases-deeplink.test.ts +++ b/frontend/src/__tests__/purchases-deeplink.test.ts @@ -48,24 +48,32 @@ describe('handlePurchaseDeeplink approve (issue #247)', () => { }); it.each([ - [403, true], - [404, true], - [500, false], - [undefined, false], - ])('details fetch failing with status %s: permission wording only=%s', async (status, permanent) => { - const err = Object.assign(new Error(status ? `HTTP ${status}` : 'Failed to fetch'), { status }); - (api.getPurchaseDetails as jest.Mock).mockRejectedValue(err); + [403, 'forbidden', 'view:purchases'], + [404, 'execution not found', 'not found or is outside your account access'], + ])('details fetch failing with %s is permanent: no retry advice, keeps server message', async (status, message, wording) => { + (api.getPurchaseDetails as jest.Mock).mockRejectedValue(Object.assign(new Error(message), { status })); await expect(handlePurchaseDeeplink()).resolves.toBe(true); const text = document.querySelector('[role="alert"]')?.textContent ?? ''; - if (permanent) { - expect(text).toContain('view:purchases'); - expect(text).not.toContain('retry'); - } else { - expect(text).toContain('Open the approval link again to retry.'); - expect(text).not.toContain('view:purchases'); - } + expect(text).toContain(wording); + expect(text).toContain(`(${message})`); + expect(text).not.toContain('retry'); + }); + + it.each([ + [500, 'internal error'], + [401, 'unauthorized'], + [undefined, 'Failed to fetch'], + ])('details fetch failing with %s keeps the retry path', async (status, message) => { + (api.getPurchaseDetails as jest.Mock).mockRejectedValue(Object.assign(new Error(message), { status })); + + await expect(handlePurchaseDeeplink()).resolves.toBe(true); + + const text = document.querySelector('[role="alert"]')?.textContent ?? ''; + expect(text).toContain('Open the approval link again to retry.'); + expect(text).toContain(`(${message})`); + expect(text).not.toContain('view:purchases'); }); it('shows the upfront amount and approves when details load', async () => { diff --git a/frontend/src/approval-details.ts b/frontend/src/approval-details.ts index 2e34daf60..587266990 100644 --- a/frontend/src/approval-details.ts +++ b/frontend/src/approval-details.ts @@ -358,9 +358,14 @@ export function isPermanentDetailsError(err: unknown): boolean { } export function approvalDetailsUnavailableMessage(err: unknown): string { - if (isPermanentDetailsError(err)) { - return 'This purchase cannot be approved without showing the amount, and your account cannot load its details. You need the view:purchases permission and access to the purchase\'s account.'; - } 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}).`; + const base = `This purchase cannot be approved without showing the amount, and its details could not be loaded (${cause}).`; + const status = (err as { status?: number } | null)?.status; + if (status === 403) { + return `${base} Loading details requires the view:purchases permission and access to the purchase's account.`; + } + if (status === 404) { + return `${base} The purchase was not found or is outside your account access.`; + } + return base; } diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 497c6c6b9..3de24240b 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 { approvalDetailsUnavailableMessage, buildApprovalDetailsBody, isPermanentDetailsError } from './approval-details'; +import { approvalDetailsUnavailableMessage, buildApprovalDetailsBody } from './approval-details'; import { showToast } from './toast'; import { getCurrentUser } from './state'; import { canAccess } from './permissions'; @@ -1301,9 +1301,7 @@ function wireRowActionHandlers(container: HTMLElement): void { detailsBody = await buildApprovalDetailsBody(id); } catch (detailsError) { console.error('Failed to load purchase details for approval:', detailsError); - showToast({ message: isPermanentDetailsError(detailsError) - ? approvalDetailsUnavailableMessage(detailsError) - : `${approvalDetailsUnavailableMessage(detailsError)} Try again.`, kind: 'error' }); + showToast({ message: `${approvalDetailsUnavailableMessage(detailsError)} Try again.`, kind: 'error' }); return; } const ok = await confirmDialog({ diff --git a/frontend/tests-e2e/history-action-confirmation.spec.ts b/frontend/tests-e2e/history-action-confirmation.spec.ts index 857db70df..4d27b96d6 100644 --- a/frontend/tests-e2e/history-action-confirmation.spec.ts +++ b/frontend/tests-e2e/history-action-confirmation.spec.ts @@ -122,7 +122,7 @@ test('approve is not offered when the purchase details cannot be loaded (issue # await page.route(`**/api/purchases/${ID}`, route => { detailGets++; return detailGets === 1 - ? route.fulfill({ status: 403, json: { error: 'forbidden' } }) + ? route.fulfill({ status: 500, json: { error: 'internal error' } }) : route.fulfill({ json: DETAILS }); }); await page.goto('/purchases/history'); @@ -137,6 +137,21 @@ test('approve is not offered when the purchase details cannot be loaded (issue # await expect.poll(() => fixture.posts.length).toBe(1); }); +for (const [status, wording] of [[403, 'view:purchases'], [404, 'outside your account access']] as const) { + test(`approval link with details ${status} shows permanent wording without retry advice (issue #529)`, async ({ page }) => { + const fixture = await historyFixture(page, 'approve'); + await page.route(`**/api/purchases/${ID}`, route => route.fulfill({ status, json: { error: 'denied by server' } })); + await page.goto(`/purchases/approve/${ID}?token=tok`); + const alert = page.getByRole('alert'); + await expect(alert).toContainText('cannot be approved without showing the amount'); + await expect(alert).toContainText(wording); + await expect(alert).toContainText('denied by server'); + await expect(alert).not.toContainText(/try again|retry/i); + await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(0); + expect(fixture.posts).toHaveLength(0); + }); +} + test('canceling an independent row does not steal focus from another confirmation', async ({ page }) => { const fixture = await historyFixture(page, 'approve'); const details = gate();