From f89ebeaa29a09e5c8d777f178c3fa9d449367175 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 19:44:23 +0200 Subject: [PATCH 1/3] chore(ui/history): improve Retry toast wording Closes #707 --- frontend/src/history.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 9287bde6b..d8e156886 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -812,7 +812,7 @@ function wireRowActionHandlers(container: HTMLElement): void { // Retry POST succeeded — surface success regardless of whether // the refresh works. The reload error path mirrors the cancel // flow above. - showToast({ message: 'Retry execution created', kind: 'success', timeout: 5_000 }); + showToast({ message: 'Purchase request sent for approval', kind: 'success', timeout: 5_000 }); try { await loadHistory(); } catch (reloadError) { From 8c63d50d3873ae8faf8ede568a8ab88f57c058d9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 22:23:38 +0200 Subject: [PATCH 2/3] fix(ui/history): gate retry success toast on email_sent / status response The "Purchase request sent for approval" toast was shown unconditionally after a successful retry POST. The backend's RetryPurchaseResult already carries email_sent (bool) and status (string); when the approval email fails, the execution is created with status "failed" and email_sent false, but the toast previously still claimed approval was requested. Gate the toast: show success when email_sent===true or status==='pending'/'notified'; show a warning toast when email_sent===false or status indicates failure. Adds three new test cases covering the email-sent, status-only, and email-failed branches. Resolves CR finding on PR #708 (discussion_r3299766964). --- .../__tests__/history-retry-button.test.ts | 80 +++++++++++++++++-- frontend/src/history.ts | 19 +++-- 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index 52a11691e..66eece6fb 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -16,9 +16,11 @@ * 6. retry_execution_id set → no Retry button (act on the descendant). * 7. lineage links: "↻ Retried as #abc" for predecessors, "↻ Retry #n" for retries. * 8. Click + decline confirmDialog → no API call. - * 9. Click + accept → retryPurchase + reload + toast. + * 9. Click + accept → retryPurchase + reload + success toast (email_sent true). * 10. retryPurchase rejects → toast.error + button re-enabled. * 11. Over-threshold click sends force=true. + * 12. email_sent absent but status=pending → success toast. + * 13. email_sent false → warning toast (approval email failed). */ import { loadHistory } from '../history'; @@ -214,7 +216,13 @@ describe('History inline Retry button (issue #47)', () => { test('threshold-reached row renders override button + sends force=true on accept', async () => { (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); (confirmDialog as jest.Mock).mockResolvedValue(true); - (api.retryPurchase as jest.Mock).mockResolvedValue({ execution_id: 'new', original_execution: 'r-1' }); + (api.retryPurchase as jest.Mock).mockResolvedValue({ + execution_id: 'new', + original_execution: 'r-1', + status: 'pending', + retry_attempt_n: 6, + email_sent: true, + }); (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ @@ -286,10 +294,16 @@ describe('History inline Retry button (issue #47)', () => { expect(api.retryPurchase).not.toHaveBeenCalled(); }); - test('accepted confirm posts retry + reloads + toasts', async () => { + test('accepted confirm posts retry + reloads + success toast when email_sent is true', async () => { (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); (confirmDialog as jest.Mock).mockResolvedValue(true); - (api.retryPurchase as jest.Mock).mockResolvedValue({ execution_id: 'new', original_execution: 'r-1' }); + (api.retryPurchase as jest.Mock).mockResolvedValue({ + execution_id: 'new', + original_execution: 'r-1', + status: 'pending', + retry_attempt_n: 1, + email_sent: true, + }); (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], @@ -303,7 +317,63 @@ describe('History inline Retry button (issue #47)', () => { expect(api.retryPurchase).toHaveBeenCalledWith('r-1', undefined); expect(api.getHistory).toHaveBeenCalledTimes(2); - expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'success' })); + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ kind: 'success', message: 'Purchase request sent for approval' }), + ); + }); + + test('retry success toast uses approval wording when status is pending but email_sent absent', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + // Backend may omit email_sent; status==='pending' is sufficient. + (api.retryPurchase as jest.Mock).mockResolvedValue({ + execution_id: 'new', + original_execution: 'r-1', + status: 'pending', + retry_attempt_n: 1, + }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], + }); + + await loadHistory(); + const btn = document.querySelector('.history-retry-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ kind: 'success', message: 'Purchase request sent for approval' }), + ); + }); + + test('retry shows warning toast when email_sent is false', async () => { + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + (api.retryPurchase as jest.Mock).mockResolvedValue({ + execution_id: 'new', + original_execution: 'r-1', + status: 'failed', + retry_attempt_n: 1, + email_sent: false, + }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], + }); + console.error = jest.fn(); + + await loadHistory(); + const btn = document.querySelector('.history-retry-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ + kind: 'warning', + message: expect.stringContaining('approval email failed'), + }), + ); }); test('retry API failure surfaces toast and re-enables the button', async () => { diff --git a/frontend/src/history.ts b/frontend/src/history.ts index d8e156886..cfa3e2f67 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -786,8 +786,9 @@ function wireRowActionHandlers(container: HTMLElement): void { }); if (!ok) return; btn.disabled = true; + let retryResult: Awaited>; try { - await api.retryPurchase(id, overThreshold ? { force: true } : undefined); + retryResult = await api.retryPurchase(id, overThreshold ? { force: true } : undefined); } catch (retryError) { console.error('Failed to retry purchase:', retryError); // Surface structured retry hints from the backend (issue #47): @@ -809,10 +810,18 @@ function wireRowActionHandlers(container: HTMLElement): void { btn.disabled = false; return; } - // Retry POST succeeded — surface success regardless of whether - // the refresh works. The reload error path mirrors the cancel - // flow above. - showToast({ message: 'Purchase request sent for approval', kind: 'success', timeout: 5_000 }); + // Gate the toast on the approval email outcome reported by the backend. + // email_sent===true (or status==='pending'/'notified') means the approval + // request is in the queue; false means the execution was created but the + // email delivery failed, so show a partial-failure warning instead. + const emailOk = retryResult.email_sent === true + || retryResult.status === 'pending' + || retryResult.status === 'notified'; + if (emailOk) { + showToast({ message: 'Purchase request sent for approval', kind: 'success', timeout: 5_000 }); + } else { + showToast({ message: 'Retry created but approval email failed - check your notification settings', kind: 'warning', timeout: 8_000 }); + } try { await loadHistory(); } catch (reloadError) { From 25ab8a1f45f9058e4c3334662939f0ddf7f84fa9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 23:29:37 +0200 Subject: [PATCH 3/3] fix(ui/history): explicit email_sent=false wins over status fallback in retry toast gating Guard the status-based emailOk fallback with !emailExplicitlyFailed so that a backend payload carrying email_sent===false always shows the warning toast, even when status is 'pending' or 'notified'. Add test case #14 covering the conflicting-payload scenario CR round 2 identified. --- .../__tests__/history-retry-button.test.ts | 35 +++++++++++++++++++ frontend/src/history.ts | 14 +++++--- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/history-retry-button.test.ts b/frontend/src/__tests__/history-retry-button.test.ts index 66eece6fb..1a9d361c6 100644 --- a/frontend/src/__tests__/history-retry-button.test.ts +++ b/frontend/src/__tests__/history-retry-button.test.ts @@ -21,6 +21,7 @@ * 11. Over-threshold click sends force=true. * 12. email_sent absent but status=pending → success toast. * 13. email_sent false → warning toast (approval email failed). + * 14. email_sent false + status pending → warning toast (explicit failure overrides status). */ import { loadHistory } from '../history'; @@ -376,6 +377,40 @@ describe('History inline Retry button (issue #47)', () => { ); }); + test('email_sent false overrides pending status - shows warning not success toast', async () => { + // This is the case CR round 2 identified: email_sent===false must win over + // status==='pending'. The prior logic short-circuited on the status check + // and could produce a false success toast on conflicting payloads. + (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); + (confirmDialog as jest.Mock).mockResolvedValue(true); + (api.retryPurchase as jest.Mock).mockResolvedValue({ + execution_id: 'new', + original_execution: 'r-1', + status: 'pending', + retry_attempt_n: 1, + email_sent: false, + }); + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: {}, + purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })], + }); + + await loadHistory(); + const btn = document.querySelector('.history-retry-btn'); + btn?.click(); + await new Promise((r) => setTimeout(r, 10)); + + expect(showToast).toHaveBeenCalledWith( + expect.objectContaining({ + kind: 'warning', + message: expect.stringContaining('approval email failed'), + }), + ); + expect(showToast).not.toHaveBeenCalledWith( + expect.objectContaining({ kind: 'success' }), + ); + }); + test('retry API failure surfaces toast and re-enables the button', async () => { (getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER); (confirmDialog as jest.Mock).mockResolvedValue(true); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index cfa3e2f67..f1cf7e506 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -811,12 +811,16 @@ function wireRowActionHandlers(container: HTMLElement): void { return; } // Gate the toast on the approval email outcome reported by the backend. - // email_sent===true (or status==='pending'/'notified') means the approval - // request is in the queue; false means the execution was created but the - // email delivery failed, so show a partial-failure warning instead. - const emailOk = retryResult.email_sent === true + // email_sent===false is an explicit failure signal that overrides any + // status-based inference; show a warning even when status==='pending'. + // email_sent===true or a pending/notified status (with email_sent absent) + // means the approval request is in the queue. + const emailExplicitlyFailed = retryResult.email_sent === false; + const emailOk = !emailExplicitlyFailed && ( + retryResult.email_sent === true || retryResult.status === 'pending' - || retryResult.status === 'notified'; + || retryResult.status === 'notified' + ); if (emailOk) { showToast({ message: 'Purchase request sent for approval', kind: 'success', timeout: 5_000 }); } else {