Skip to content

Commit fdcede3

Browse files
cristimcristim
authored
chore(ui/history): improve Retry toast wording (#708)
* chore(ui/history): improve Retry toast wording Closes #707 * 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). * 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.
1 parent 6f6b68b commit fdcede3

2 files changed

Lines changed: 128 additions & 10 deletions

File tree

‎frontend/src/__tests__/history-retry-button.test.ts‎

Lines changed: 110 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,12 @@
1616
* 6. retry_execution_id set → no Retry button (act on the descendant).
1717
* 7. lineage links: "↻ Retried as #abc" for predecessors, "↻ Retry #n" for retries.
1818
* 8. Click + decline confirmDialog → no API call.
19-
* 9. Click + accept → retryPurchase + reload + toast.
19+
* 9. Click + accept → retryPurchase + reload + success toast (email_sent true).
2020
* 10. retryPurchase rejects → toast.error + button re-enabled.
2121
* 11. Over-threshold click sends force=true.
22+
* 12. email_sent absent but status=pending → success toast.
23+
* 13. email_sent false → warning toast (approval email failed).
24+
* 14. email_sent false + status pending → warning toast (explicit failure overrides status).
2225
*/
2326

2427
import { loadHistory } from '../history';
@@ -214,7 +217,13 @@ describe('History inline Retry button (issue #47)', () => {
214217
test('threshold-reached row renders override button + sends force=true on accept', async () => {
215218
(getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER);
216219
(confirmDialog as jest.Mock).mockResolvedValue(true);
217-
(api.retryPurchase as jest.Mock).mockResolvedValue({ execution_id: 'new', original_execution: 'r-1' });
220+
(api.retryPurchase as jest.Mock).mockResolvedValue({
221+
execution_id: 'new',
222+
original_execution: 'r-1',
223+
status: 'pending',
224+
retry_attempt_n: 6,
225+
email_sent: true,
226+
});
218227
(api.getHistory as jest.Mock).mockResolvedValue({
219228
summary: {},
220229
purchases: [
@@ -286,10 +295,16 @@ describe('History inline Retry button (issue #47)', () => {
286295
expect(api.retryPurchase).not.toHaveBeenCalled();
287296
});
288297

289-
test('accepted confirm posts retry + reloads + toasts', async () => {
298+
test('accepted confirm posts retry + reloads + success toast when email_sent is true', async () => {
290299
(getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER);
291300
(confirmDialog as jest.Mock).mockResolvedValue(true);
292-
(api.retryPurchase as jest.Mock).mockResolvedValue({ execution_id: 'new', original_execution: 'r-1' });
301+
(api.retryPurchase as jest.Mock).mockResolvedValue({
302+
execution_id: 'new',
303+
original_execution: 'r-1',
304+
status: 'pending',
305+
retry_attempt_n: 1,
306+
email_sent: true,
307+
});
293308
(api.getHistory as jest.Mock).mockResolvedValue({
294309
summary: {},
295310
purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })],
@@ -303,7 +318,97 @@ describe('History inline Retry button (issue #47)', () => {
303318

304319
expect(api.retryPurchase).toHaveBeenCalledWith('r-1', undefined);
305320
expect(api.getHistory).toHaveBeenCalledTimes(2);
306-
expect(showToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'success' }));
321+
expect(showToast).toHaveBeenCalledWith(
322+
expect.objectContaining({ kind: 'success', message: 'Purchase request sent for approval' }),
323+
);
324+
});
325+
326+
test('retry success toast uses approval wording when status is pending but email_sent absent', async () => {
327+
(getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER);
328+
(confirmDialog as jest.Mock).mockResolvedValue(true);
329+
// Backend may omit email_sent; status==='pending' is sufficient.
330+
(api.retryPurchase as jest.Mock).mockResolvedValue({
331+
execution_id: 'new',
332+
original_execution: 'r-1',
333+
status: 'pending',
334+
retry_attempt_n: 1,
335+
});
336+
(api.getHistory as jest.Mock).mockResolvedValue({
337+
summary: {},
338+
purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })],
339+
});
340+
341+
await loadHistory();
342+
const btn = document.querySelector<HTMLButtonElement>('.history-retry-btn');
343+
btn?.click();
344+
await new Promise((r) => setTimeout(r, 10));
345+
346+
expect(showToast).toHaveBeenCalledWith(
347+
expect.objectContaining({ kind: 'success', message: 'Purchase request sent for approval' }),
348+
);
349+
});
350+
351+
test('retry shows warning toast when email_sent is false', async () => {
352+
(getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER);
353+
(confirmDialog as jest.Mock).mockResolvedValue(true);
354+
(api.retryPurchase as jest.Mock).mockResolvedValue({
355+
execution_id: 'new',
356+
original_execution: 'r-1',
357+
status: 'failed',
358+
retry_attempt_n: 1,
359+
email_sent: false,
360+
});
361+
(api.getHistory as jest.Mock).mockResolvedValue({
362+
summary: {},
363+
purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })],
364+
});
365+
console.error = jest.fn();
366+
367+
await loadHistory();
368+
const btn = document.querySelector<HTMLButtonElement>('.history-retry-btn');
369+
btn?.click();
370+
await new Promise((r) => setTimeout(r, 10));
371+
372+
expect(showToast).toHaveBeenCalledWith(
373+
expect.objectContaining({
374+
kind: 'warning',
375+
message: expect.stringContaining('approval email failed'),
376+
}),
377+
);
378+
});
379+
380+
test('email_sent false overrides pending status - shows warning not success toast', async () => {
381+
// This is the case CR round 2 identified: email_sent===false must win over
382+
// status==='pending'. The prior logic short-circuited on the status check
383+
// and could produce a false success toast on conflicting payloads.
384+
(getCurrentUser as jest.Mock).mockReturnValue(ADMIN_USER);
385+
(confirmDialog as jest.Mock).mockResolvedValue(true);
386+
(api.retryPurchase as jest.Mock).mockResolvedValue({
387+
execution_id: 'new',
388+
original_execution: 'r-1',
389+
status: 'pending',
390+
retry_attempt_n: 1,
391+
email_sent: false,
392+
});
393+
(api.getHistory as jest.Mock).mockResolvedValue({
394+
summary: {},
395+
purchases: [makeRow({ purchase_id: 'r-1', created_by_user_id: ADMIN_USER.id })],
396+
});
397+
398+
await loadHistory();
399+
const btn = document.querySelector<HTMLButtonElement>('.history-retry-btn');
400+
btn?.click();
401+
await new Promise((r) => setTimeout(r, 10));
402+
403+
expect(showToast).toHaveBeenCalledWith(
404+
expect.objectContaining({
405+
kind: 'warning',
406+
message: expect.stringContaining('approval email failed'),
407+
}),
408+
);
409+
expect(showToast).not.toHaveBeenCalledWith(
410+
expect.objectContaining({ kind: 'success' }),
411+
);
307412
});
308413

309414
test('retry API failure surfaces toast and re-enables the button', async () => {

‎frontend/src/history.ts‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -786,8 +786,9 @@ function wireRowActionHandlers(container: HTMLElement): void {
786786
});
787787
if (!ok) return;
788788
btn.disabled = true;
789+
let retryResult: Awaited<ReturnType<typeof api.retryPurchase>>;
789790
try {
790-
await api.retryPurchase(id, overThreshold ? { force: true } : undefined);
791+
retryResult = await api.retryPurchase(id, overThreshold ? { force: true } : undefined);
791792
} catch (retryError) {
792793
console.error('Failed to retry purchase:', retryError);
793794
// Surface structured retry hints from the backend (issue #47):
@@ -809,10 +810,22 @@ function wireRowActionHandlers(container: HTMLElement): void {
809810
btn.disabled = false;
810811
return;
811812
}
812-
// Retry POST succeeded — surface success regardless of whether
813-
// the refresh works. The reload error path mirrors the cancel
814-
// flow above.
815-
showToast({ message: 'Retry execution created', kind: 'success', timeout: 5_000 });
813+
// Gate the toast on the approval email outcome reported by the backend.
814+
// email_sent===false is an explicit failure signal that overrides any
815+
// status-based inference; show a warning even when status==='pending'.
816+
// email_sent===true or a pending/notified status (with email_sent absent)
817+
// means the approval request is in the queue.
818+
const emailExplicitlyFailed = retryResult.email_sent === false;
819+
const emailOk = !emailExplicitlyFailed && (
820+
retryResult.email_sent === true
821+
|| retryResult.status === 'pending'
822+
|| retryResult.status === 'notified'
823+
);
824+
if (emailOk) {
825+
showToast({ message: 'Purchase request sent for approval', kind: 'success', timeout: 5_000 });
826+
} else {
827+
showToast({ message: 'Retry created but approval email failed - check your notification settings', kind: 'warning', timeout: 8_000 });
828+
}
816829
try {
817830
await loadHistory();
818831
} catch (reloadError) {

0 commit comments

Comments
 (0)