From f501ea4f94e0613852f46d59084fc11a80dafed8 Mon Sep 17 00:00:00 2001 From: Justin Visser Date: Sat, 7 Mar 2026 18:00:59 +0100 Subject: [PATCH] fix(frontend): confirm modal, settings display, race guard, and test coverage - S1: Replace window.confirm() with AppConfirmModal for delete actions - S4: Add formatHours helper to avoid ugly decimals in settings hints - W5: Add generation counter to SettingsAdminPanel to prevent stale async results when section changes rapidly - S7: Remove unused ApiRequestError import from useScholarBulkActions - S2: Add error-path tests for bulk delete/toggle network failures - S3: Add CSRF boundary tests for bulk-delete and bulk-toggle endpoints --- app/api/routers/scholars.py | 4 +- .../src/components/ui/AppConfirmModal.vue | 28 ++++++++++ .../composables/useScholarBulkActions.test.ts | 36 ++++++++++++ .../composables/useScholarBulkActions.ts | 56 ++++++++++++++----- .../features/settings/SettingsAdminPanel.vue | 5 ++ frontend/src/pages/ScholarsPage.vue | 46 ++++++++++----- frontend/src/pages/SettingsPage.vue | 9 ++- tests/integration/test_api_scholars_bulk.py | 30 ++++++++++ 8 files changed, 181 insertions(+), 33 deletions(-) create mode 100644 frontend/src/components/ui/AppConfirmModal.vue diff --git a/app/api/routers/scholars.py b/app/api/routers/scholars.py index 2fe44d6..2a0c276 100644 --- a/app/api/routers/scholars.py +++ b/app/api/routers/scholars.py @@ -53,12 +53,12 @@ def _parse_ids_param(ids: str | None) -> list[int] | None: return None try: return [int(p) for p in parts] - except ValueError: + except ValueError as exc: raise ApiException( status_code=400, code="invalid_ids_param", message="The 'ids' parameter must be a comma-separated list of integers.", - ) + ) from exc @router.get( diff --git a/frontend/src/components/ui/AppConfirmModal.vue b/frontend/src/components/ui/AppConfirmModal.vue new file mode 100644 index 0000000..044ca59 --- /dev/null +++ b/frontend/src/components/ui/AppConfirmModal.vue @@ -0,0 +1,28 @@ + + + diff --git a/frontend/src/features/scholars/composables/useScholarBulkActions.test.ts b/frontend/src/features/scholars/composables/useScholarBulkActions.test.ts index 868aad6..5a8dff9 100644 --- a/frontend/src/features/scholars/composables/useScholarBulkActions.test.ts +++ b/frontend/src/features/scholars/composables/useScholarBulkActions.test.ts @@ -109,4 +109,40 @@ describe("useScholarBulkActions", () => { await bulk.onApplyBulkAction(); expect(bulk.selectedIds.value.size).toBe(0); }); + + it("bulk delete calls assignError on network failure", async () => { + const { bulkDeleteScholars } = await import("@/features/scholars"); + vi.mocked(bulkDeleteScholars).mockRejectedValueOnce(new Error("Network error")); + + const { bulk, callbacks } = setup([makeProfile(1, "Alice")]); + bulk.onToggleRow(1, { target: { checked: true } } as unknown as Event); + bulk.bulkAction.value = "delete_selected" as ScholarBulkAction; + await bulk.onApplyBulkAction(); + + // Trigger the confirm callback + expect(bulk.confirmState.value.open).toBe(true); + await bulk.confirmState.value.onConfirm(); + + expect(callbacks.assignError).toHaveBeenCalledWith( + expect.any(Error), + "Unable to bulk delete scholars.", + ); + expect(bulk.bulkBusy.value).toBe(false); + }); + + it("bulk toggle calls assignError on network failure", async () => { + const { bulkToggleScholars } = await import("@/features/scholars"); + vi.mocked(bulkToggleScholars).mockRejectedValueOnce(new Error("Network error")); + + const { bulk, callbacks } = setup([makeProfile(1, "Alice")]); + bulk.onToggleRow(1, { target: { checked: true } } as unknown as Event); + bulk.bulkAction.value = "enable_selected" as ScholarBulkAction; + await bulk.onApplyBulkAction(); + + expect(callbacks.assignError).toHaveBeenCalledWith( + expect.any(Error), + "Unable to bulk enable scholars.", + ); + expect(bulk.bulkBusy.value).toBe(false); + }); }); diff --git a/frontend/src/features/scholars/composables/useScholarBulkActions.ts b/frontend/src/features/scholars/composables/useScholarBulkActions.ts index f4910a7..51cace7 100644 --- a/frontend/src/features/scholars/composables/useScholarBulkActions.ts +++ b/frontend/src/features/scholars/composables/useScholarBulkActions.ts @@ -6,7 +6,6 @@ import { exportScholarData, type ScholarProfile, } from "@/features/scholars"; -import { ApiRequestError } from "@/lib/api/errors"; export type ScholarBulkAction = | "delete_selected" @@ -21,6 +20,14 @@ export interface ScholarBulkActionOption { label: string; } +export interface ConfirmState { + open: boolean; + title: string; + message: string; + variant: "danger" | "default"; + onConfirm: () => void; +} + export interface BulkActionCallbacks { clearMessages: () => void; assignError: (error: unknown, fallback: string) => void; @@ -35,6 +42,7 @@ export function useScholarBulkActions( const selectedIds = ref>(new Set()); const bulkAction = ref("select_all"); const bulkBusy = ref(false); + const confirmState = ref({ open: false, title: "", message: "", variant: "default", onConfirm: () => {} }); const selectedCount = computed(() => selectedIds.value.size); const hasSelection = computed(() => selectedCount.value > 0); @@ -128,21 +136,36 @@ export function useScholarBulkActions( if (bulkAction.value === "export_selected") { await onBulkExport(); return; } } - async function onBulkDelete(): Promise { + function dismissConfirm(): void { + confirmState.value = { ...confirmState.value, open: false }; + } + + function requestConfirm(title: string, message: string, variant: "danger" | "default", onConfirm: () => void): void { + confirmState.value = { open: true, title, message, variant, onConfirm }; + } + + function onBulkDelete(): void { const ids = [...selectedIds.value]; - if (!window.confirm(`Delete ${ids.length} scholar(s)? This removes all linked publications and queue data.`)) return; - bulkBusy.value = true; - callbacks.clearMessages(); - try { - const result = await bulkDeleteScholars(ids); - callbacks.setSuccess(`${result.deleted_count} scholar(s) deleted.`); - selectedIds.value = new Set(); - await callbacks.reloadScholars(); - } catch (error) { - callbacks.assignError(error, "Unable to bulk delete scholars."); - } finally { - bulkBusy.value = false; - } + requestConfirm( + `Delete ${ids.length} scholar(s)?`, + "This removes all linked publications and queue data. This action cannot be undone.", + "danger", + async () => { + dismissConfirm(); + bulkBusy.value = true; + callbacks.clearMessages(); + try { + const result = await bulkDeleteScholars(ids); + callbacks.setSuccess(`${result.deleted_count} scholar(s) deleted.`); + selectedIds.value = new Set(); + await callbacks.reloadScholars(); + } catch (error) { + callbacks.assignError(error, "Unable to bulk delete scholars."); + } finally { + bulkBusy.value = false; + } + }, + ); } async function onBulkToggle(isEnabled: boolean): Promise { @@ -188,6 +211,9 @@ export function useScholarBulkActions( bulkActionOptions, bulkApplyLabel, bulkApplyDisabled, + confirmState, + dismissConfirm, + requestConfirm, onToggleAll, onToggleRow, onApplyBulkAction, diff --git a/frontend/src/features/settings/SettingsAdminPanel.vue b/frontend/src/features/settings/SettingsAdminPanel.vue index c281de1..d7ee685 100644 --- a/frontend/src/features/settings/SettingsAdminPanel.vue +++ b/frontend/src/features/settings/SettingsAdminPanel.vue @@ -25,7 +25,10 @@ const integrityRef = ref | null>(null const repairsRef = ref | null>(null); const pdfQueueRef = ref | null>(null); +let loadGeneration = 0; + async function loadSection(): Promise { + const gen = ++loadGeneration; clearAlerts(); try { if (props.section === SECTION_USERS && usersRef.value) { @@ -34,11 +37,13 @@ async function loadSection(): Promise { await integrityRef.value.load(); } else if (props.section === SECTION_REPAIRS) { await usersRef.value?.load(); + if (gen !== loadGeneration) return; await repairsRef.value?.load(); } else if (props.section === SECTION_PDF && pdfQueueRef.value) { await pdfQueueRef.value.load(); } } catch (error) { + if (gen !== loadGeneration) return; assignError(error, "Unable to load admin data."); } } diff --git a/frontend/src/pages/ScholarsPage.vue b/frontend/src/pages/ScholarsPage.vue index c962242..3899888 100644 --- a/frontend/src/pages/ScholarsPage.vue +++ b/frontend/src/pages/ScholarsPage.vue @@ -6,6 +6,7 @@ import AsyncStateGate from "@/components/patterns/AsyncStateGate.vue"; import RequestStateAlerts from "@/components/patterns/RequestStateAlerts.vue"; import AppButton from "@/components/ui/AppButton.vue"; import AppCard from "@/components/ui/AppCard.vue"; +import AppConfirmModal from "@/components/ui/AppConfirmModal.vue"; import AppEmptyState from "@/components/ui/AppEmptyState.vue"; import AppHelpHint from "@/components/ui/AppHelpHint.vue"; import AppInput from "@/components/ui/AppInput.vue"; @@ -272,23 +273,30 @@ async function onToggleScholar(): Promise { } } -async function onDeleteScholar(): Promise { +function onDeleteScholar(): void { const profile = activeScholarSettings.value; if (!profile) return; const label = scholarLabel(profile); - if (!window.confirm(`Delete scholar ${label}? This removes all linked publications and queue data.`)) return; - activeScholarId.value = profile.id; - clearMessages(); - try { - await deleteScholar(profile.id); - successMessage.value = `${label} deleted.`; - activeScholarSettingsId.value = null; - await loadScholars(); - } catch (error) { - assignError(error, "Unable to delete scholar."); - } finally { - activeScholarId.value = null; - } + bulk.requestConfirm( + `Delete ${label}?`, + "This removes all linked publications and queue data. This action cannot be undone.", + "danger", + async () => { + bulk.dismissConfirm(); + activeScholarId.value = profile.id; + clearMessages(); + try { + await deleteScholar(profile.id); + successMessage.value = `${label} deleted.`; + activeScholarSettingsId.value = null; + await loadScholars(); + } catch (error) { + assignError(error, "Unable to delete scholar."); + } finally { + activeScholarId.value = null; + } + }, + ); } async function onSaveImageUrl(): Promise { @@ -597,6 +605,16 @@ watch( @toggle="onToggleScholar" @delete="onDeleteScholar" /> + + diff --git a/frontend/src/pages/SettingsPage.vue b/frontend/src/pages/SettingsPage.vue index e1c0e2b..3fb50fc 100644 --- a/frontend/src/pages/SettingsPage.vue +++ b/frontend/src/pages/SettingsPage.vue @@ -168,6 +168,11 @@ function parseBoundedInteger(value: string, label: string, minimum: number): num return parsed; } +function formatHours(minutes: number): string { + const h = minutes / 60; + return Number.isInteger(h) ? String(h) : h.toFixed(2).replace(/\.?0+$/, ""); +} + function parseHoursToMinutes(value: string, minMinutes: number): number { const hours = Number(value); if (!Number.isFinite(hours) || hours <= 0) { @@ -175,7 +180,7 @@ function parseHoursToMinutes(value: string, minMinutes: number): number { } const minutes = Math.round(hours * 60); if (minutes < minMinutes) { - throw new Error(`Check interval must be at least ${minMinutes / 60} hours.`); + throw new Error(`Check interval must be at least ${formatHours(minMinutes)} hours.`); } return minutes; } @@ -372,7 +377,7 @@ onMounted(async () => { - Minimum: {{ minCheckIntervalMinutes / 60 }} hours + Minimum: {{ formatHours(minCheckIntervalMinutes) }} hours