diff --git a/frontend/apps/web/src/pages/ListPage.tsx b/frontend/apps/web/src/pages/ListPage.tsx index f242108..9850192 100644 --- a/frontend/apps/web/src/pages/ListPage.tsx +++ b/frontend/apps/web/src/pages/ListPage.tsx @@ -41,6 +41,7 @@ import { FilterBar } from '@dar/search'; import { useToast } from '../toast'; import { CHANGELIST_FILTERS_PARAM, withPreservedFilters } from '../changelistFilters'; +import { handleActionResult } from './action-result'; // Lazy-loaded so the @dnd-kit suite (the heaviest dep in this modal) // only lands in the bundle of users who open the Customize modal @@ -132,6 +133,14 @@ export function ListPage() { lockedColsKey(appLabel, modelName), ); const [colsOpen, setColsOpen] = useState(false); + // Popup-blocked fallback (#644). When an action returns a redirect + // and `window.open` is suppressed by the browser's popup blocker + // (the call isn't a direct user gesture by then — we're inside an + // async handler after the POST resolves), surface a banner with a + // clickable link so the redirect isn't silently swallowed. + const [pendingRedirect, setPendingRedirect] = useState<{ url: string; label: string } | null>( + null, + ); // Drag-to-resize column widths, persisted per app/model (a UI // preference, like hidden columns) via @dar/customization. The Table @@ -334,31 +343,18 @@ export function ListPage() { const result = await client.runAction(appLabel, modelName, action.name, pks); setSelected(new Set()); setSelectAcross(false); - // Intermediate / form-returning action (#250). When the admin action - // returns an HttpResponse (e.g. Django's delete-selected, or a - // custom action that needs a confirmation/parameter page), the - // backend forwards its Location as `redirect`. Open it in a new tab - // so the operator can complete the flow there — the SPA stays - // mounted, no silent no-op. (Parameterised in-SPA forms are a - // follow-up; this closes the "silent no-op" minimum.) - if (result.redirect) { - window.open(result.redirect, '_blank', 'noopener,noreferrer'); - toast.info(`${action.label} opened in a new tab.`); - return; - } - await refresh(); - // Prefer the action's own message_user output (#442); fall back to a - // generic confirmation when the action queued nothing. - const msgs = result.messages ?? []; - if (msgs.length > 0) { - for (const m of msgs) { - if (m.level === 'error' || m.level === 'warning') toast.error(m.message); - else if (m.level === 'info' || m.level === 'debug') toast.info(m.message); - else toast.success(m.message); - } - } else { - toast.success(`${action.label} — ${count} item${count === 1 ? '' : 's'}.`); - } + // Per-result dispatch lives in `handleActionResult` so the + // popup-blocked + refresh + level-toast flow is unit-testable + // without rendering the whole page (#250, #442, #632, #644). + await handleActionResult({ + result, + action, + count, + toast, + refresh, + openLink: (url) => window.open(url, '_blank', 'noopener,noreferrer'), + onPopupBlocked: (info) => setPendingRedirect(info), + }); } catch (e) { toast.error(e instanceof Error ? e.message : 'Action failed.'); } finally { @@ -661,6 +657,34 @@ export function ListPage() { )} + {/* Popup-blocked fallback for an action redirect (#644). If + window.open returns null (the browser suppressed the + new-tab because we're past the direct-user-gesture moment), + render a clickable link banner so the redirect URL isn't + silently swallowed. The user opens it manually with a real + click; dismiss closes the banner. Amber = "this needs your + attention but it isn't an error." */} + {pendingRedirect && ( +
+ + {pendingRedirect.label} wants to open a page —{' '} + setPendingRedirect(null)} + > + click to open in a new tab + + . + + +
+ )} + {/* Select-all-across-pages (#386). Once the whole page is ticked and more rows match, offer to extend the selection to the entire filtered set (Django changelist parity); while active, say so and diff --git a/frontend/apps/web/src/pages/action-result.test.ts b/frontend/apps/web/src/pages/action-result.test.ts new file mode 100644 index 0000000..e9683f6 --- /dev/null +++ b/frontend/apps/web/src/pages/action-result.test.ts @@ -0,0 +1,130 @@ +// Lock the bulk-action result handling (#250 / #442 / #632 / #644): +// +// 1. redirect → `openLink` opens new tab → info toast +// 2. redirect → `openLink` returns null (popup blocked) → +// pendingRedirect banner, NOT silently swallowed +// 3. refresh always fires (BOTH redirect and no-redirect paths) +// 4. messages[] toasts by level (error/warning → red, +// info/debug → blue, success → green) +// 5. No messages + no redirect → generic " — N item(s)" toast +// 6. No messages + WITH redirect → no generic toast (the redirect's +// destination surfaces the next-step UX) + +import { describe, expect, it, vi } from 'vitest'; + +import type { ActionDescriptor, ActionRunResponse } from '@dar/data'; + +import { handleActionResult } from './action-result'; + +const action: ActionDescriptor = { + name: 'export', + label: 'Export selected', + description: '', + requires_confirmation: false, +}; + +function makeArgs( + result: ActionRunResponse, + overrides: { openReturns?: Window | null } = {}, +) { + const toast = { + success: vi.fn<(m: string) => void>(), + error: vi.fn<(m: string) => void>(), + info: vi.fn<(m: string) => void>(), + }; + const refresh = vi.fn<() => Promise>(async () => {}); + // `openReturns` may be `null` (popup blocked); `??` would + // coalesce a real null away, so check for `undefined` (omitted). + const openLink = vi.fn<(url: string) => Window | null>( + () => ('openReturns' in overrides ? overrides.openReturns! : ({} as Window)), + ); + const onPopupBlocked = vi.fn<(info: { url: string; label: string }) => void>(); + return { + args: { result, action, count: 3, toast, refresh, openLink, onPopupBlocked }, + toast, + refresh, + openLink, + onPopupBlocked, + }; +} + +describe('handleActionResult', () => { + it('opens redirect in a new tab and shows an info toast', async () => { + const { args, toast, openLink, onPopupBlocked } = makeArgs({ + executed: true, + action: 'export', + redirect: '/exports/build/?ids=1,2,3', + }); + await handleActionResult(args); + expect(openLink).toHaveBeenCalledWith('/exports/build/?ids=1,2,3'); + expect(toast.info).toHaveBeenCalledWith('Export selected opened in a new tab.'); + expect(onPopupBlocked).not.toHaveBeenCalled(); + }); + + it('surfaces a pendingRedirect when window.open is suppressed (popup blocker)', async () => { + const { args, toast, openLink, onPopupBlocked } = makeArgs( + { executed: true, action: 'export', redirect: 'https://exports.example/123' }, + { openReturns: null }, + ); + await handleActionResult(args); + expect(openLink).toHaveBeenCalledOnce(); + expect(onPopupBlocked).toHaveBeenCalledWith({ + url: 'https://exports.example/123', + label: 'Export selected', + }); + // Crucially: no info toast in this path — the URL would be lost + // without the banner the caller renders from the pendingRedirect. + expect(toast.info).not.toHaveBeenCalled(); + }); + + it('refreshes the changelist on BOTH the redirect and no-redirect paths', async () => { + const withRedirect = makeArgs({ executed: true, action: 'export', redirect: '/x/' }); + await handleActionResult(withRedirect.args); + expect(withRedirect.refresh).toHaveBeenCalledOnce(); + + const noRedirect = makeArgs({ executed: true, action: 'export' }); + await handleActionResult(noRedirect.args); + expect(noRedirect.refresh).toHaveBeenCalledOnce(); + }); + + it('toasts each messages[] entry by level (error → red, info → blue, success → green)', async () => { + const { args, toast } = makeArgs({ + executed: true, + action: 'export', + messages: [ + { level: 'error', message: 'one failed' }, + { level: 'info', message: 'two queued' }, + { level: 'success', message: 'three done' }, + ], + }); + await handleActionResult(args); + expect(toast.error).toHaveBeenCalledWith('one failed'); + expect(toast.info).toHaveBeenCalledWith('two queued'); + expect(toast.success).toHaveBeenCalledWith('three done'); + }); + + it('falls back to a generic success toast when no messages and no redirect', async () => { + const { args, toast } = makeArgs({ executed: true, action: 'export' }); + await handleActionResult(args); + expect(toast.success).toHaveBeenCalledWith('Export selected — 3 items.'); + }); + + it('uses singular "item" when count is 1', async () => { + const { args, toast } = makeArgs({ executed: true, action: 'export' }); + args.count = 1; + await handleActionResult(args); + expect(toast.success).toHaveBeenCalledWith('Export selected — 1 item.'); + }); + + it('does NOT emit the generic toast when the action redirected (the destination shows the next-step UX)', async () => { + const { args, toast } = makeArgs({ + executed: true, + action: 'export', + redirect: '/exports/build/', + }); + await handleActionResult(args); + // info toast for the new-tab open is fine; the generic + // "— N item(s)" success toast must NOT fire. + expect(toast.success).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/apps/web/src/pages/action-result.ts b/frontend/apps/web/src/pages/action-result.ts new file mode 100644 index 0000000..13724dc --- /dev/null +++ b/frontend/apps/web/src/pages/action-result.ts @@ -0,0 +1,61 @@ +// handleActionResult — what to do with the JSON envelope a +// changelist bulk-action returns (#250, #442, #632, #644). Pure +// function so the redirect / popup-blocked / messages flow is +// unit-testable without rendering the whole ListPage. +// +// Mirrors Django admin's `ModelAdmin.response_action` contract: +// +// - An action that returns `HttpResponse` (typically a +// `HttpResponseRedirect` to an intermediate / confirmation +// page, an export wizard, a hijack flow) → the API surfaces +// the `Location` as `result.redirect`. Open it in a NEW TAB +// (`noopener,noreferrer`) so the changelist stays in view. +// - Popup blocker eats `window.open` → return a +// `{url, label}` pendingRedirect for the caller to render as a +// clickable banner; the redirect must never be silently lost. +// - Refresh the changelist on BOTH branches so any side-effects +// the action had on the rows are visible. +// - Messages with a `level` tag drive the toast colour (#632). +// - When there are no messages AND no redirect, fall back to a +// generic " — N item(s)." success toast — a redirect's +// own destination usually surfaces next-step UX, no extra +// "Done" toast needed for that path. + +import type { ActionDescriptor, ActionRunResponse } from '@dar/data'; + +import type { ToastApi } from '../toast'; +import { toastMessages } from '../toast'; + +export interface HandleActionResultArgs { + result: ActionRunResponse; + action: ActionDescriptor; + count: number; + toast: ToastApi; + refresh: () => Promise; + /** Injected for testability. Production: `(url) => + * window.open(url, '_blank', 'noopener,noreferrer')`. Returns + * `null` when the popup blocker suppresses the call. */ + openLink: (url: string) => Window | null; + /** Called when `openLink` returns `null`. Caller renders a + * clickable fallback banner so the redirect URL isn't lost. */ + onPopupBlocked: (info: { url: string; label: string }) => void; +} + +export async function handleActionResult(args: HandleActionResultArgs): Promise { + const { result, action, count, toast, refresh, openLink, onPopupBlocked } = args; + if (result.redirect) { + const opened = openLink(result.redirect); + if (opened) { + toast.info(`${action.label} opened in a new tab.`); + } else { + onPopupBlocked({ url: result.redirect, label: action.label }); + } + } + await refresh(); + const msgs = result.messages ?? []; + if (msgs.length > 0) { + toastMessages(toast, msgs); + } else if (!result.redirect) { + toast.success(`${action.label} — ${count} item${count === 1 ? '' : 's'}.`); + } +} diff --git a/frontend/apps/web/src/toast.tsx b/frontend/apps/web/src/toast.tsx index 3a5994b..30ca4ee 100644 --- a/frontend/apps/web/src/toast.tsx +++ b/frontend/apps/web/src/toast.tsx @@ -18,7 +18,7 @@ interface Toast { message: string; } -interface ToastApi { +export interface ToastApi { success: (message: string) => void; error: (message: string) => void; info: (message: string) => void; diff --git a/poetry.lock b/poetry.lock index d7c0192..72244c3 100644 --- a/poetry.lock +++ b/poetry.lock @@ -556,14 +556,14 @@ jsonschema = ">=4.0,<5.0" [[package]] name = "django-admin-rest-api" -version = "1.1.0" +version = "1.1.1" description = "A JSON REST API for the Django admin — same permissions, same ModelAdmin, no new features. Powers django-admin-react and django-admin-mcp." optional = false python-versions = "<4.0,>=3.10" groups = ["main"] files = [ - {file = "django_admin_rest_api-1.1.0-py3-none-any.whl", hash = "sha256:310479ae8ef2619c45b24e4a02b7b51111afae01dd519cd936de9ff5e13e01f0"}, - {file = "django_admin_rest_api-1.1.0.tar.gz", hash = "sha256:b9e9eff5e7ad5181ba3622f95e5f9b2320e01db833b0390de283f8ce1481ad9b"}, + {file = "django_admin_rest_api-1.1.1-py3-none-any.whl", hash = "sha256:dacc9103d6a0a6c40cfa5e9e3f687681f3ce8bfe019050833eb848a4da2385d5"}, + {file = "django_admin_rest_api-1.1.1.tar.gz", hash = "sha256:1780f59ed648262cf67aab98b7bb8a3d7551472c03e89a319a220ed6c089bfd7"}, ] [package.dependencies] diff --git a/pyproject.toml b/pyproject.toml index c67fec0..2e0a78e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "django-admin-react" -version = "1.5.0" +version = "1.5.1" description = "A drop-in React single-page admin for Django, driven entirely by ModelAdmin." authors = ["django-admin-react contributors"] license = "MIT"