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
This commit is contained in:
Justin Visser 2026-03-07 18:00:59 +01:00
parent f8e3098fc3
commit f501ea4f94
8 changed files with 181 additions and 33 deletions

View file

@ -53,12 +53,12 @@ def _parse_ids_param(ids: str | None) -> list[int] | None:
return None return None
try: try:
return [int(p) for p in parts] return [int(p) for p in parts]
except ValueError: except ValueError as exc:
raise ApiException( raise ApiException(
status_code=400, status_code=400,
code="invalid_ids_param", code="invalid_ids_param",
message="The 'ids' parameter must be a comma-separated list of integers.", message="The 'ids' parameter must be a comma-separated list of integers.",
) ) from exc
@router.get( @router.get(

View file

@ -0,0 +1,28 @@
<script setup lang="ts">
import AppButton from "@/components/ui/AppButton.vue";
import AppModal from "@/components/ui/AppModal.vue";
withDefaults(
defineProps<{
open: boolean;
title: string;
message: string;
confirmLabel?: string;
cancelLabel?: string;
variant?: "danger" | "default";
}>(),
{ confirmLabel: "Confirm", cancelLabel: "Cancel", variant: "default" },
);
const emit = defineEmits<{ confirm: []; cancel: [] }>();
</script>
<template>
<AppModal :open="open" :title="title" @close="emit('cancel')">
<p class="mb-6 text-sm text-secondary">{{ message }}</p>
<div class="flex justify-end gap-2">
<AppButton variant="secondary" @click="emit('cancel')">{{ cancelLabel }}</AppButton>
<AppButton :variant="variant === 'danger' ? 'danger' : 'primary'" @click="emit('confirm')">{{ confirmLabel }}</AppButton>
</div>
</AppModal>
</template>

View file

@ -109,4 +109,40 @@ describe("useScholarBulkActions", () => {
await bulk.onApplyBulkAction(); await bulk.onApplyBulkAction();
expect(bulk.selectedIds.value.size).toBe(0); 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);
});
}); });

View file

@ -6,7 +6,6 @@ import {
exportScholarData, exportScholarData,
type ScholarProfile, type ScholarProfile,
} from "@/features/scholars"; } from "@/features/scholars";
import { ApiRequestError } from "@/lib/api/errors";
export type ScholarBulkAction = export type ScholarBulkAction =
| "delete_selected" | "delete_selected"
@ -21,6 +20,14 @@ export interface ScholarBulkActionOption {
label: string; label: string;
} }
export interface ConfirmState {
open: boolean;
title: string;
message: string;
variant: "danger" | "default";
onConfirm: () => void;
}
export interface BulkActionCallbacks { export interface BulkActionCallbacks {
clearMessages: () => void; clearMessages: () => void;
assignError: (error: unknown, fallback: string) => void; assignError: (error: unknown, fallback: string) => void;
@ -35,6 +42,7 @@ export function useScholarBulkActions(
const selectedIds = ref<Set<number>>(new Set()); const selectedIds = ref<Set<number>>(new Set());
const bulkAction = ref<ScholarBulkAction>("select_all"); const bulkAction = ref<ScholarBulkAction>("select_all");
const bulkBusy = ref(false); const bulkBusy = ref(false);
const confirmState = ref<ConfirmState>({ open: false, title: "", message: "", variant: "default", onConfirm: () => {} });
const selectedCount = computed(() => selectedIds.value.size); const selectedCount = computed(() => selectedIds.value.size);
const hasSelection = computed(() => selectedCount.value > 0); const hasSelection = computed(() => selectedCount.value > 0);
@ -128,21 +136,36 @@ export function useScholarBulkActions(
if (bulkAction.value === "export_selected") { await onBulkExport(); return; } if (bulkAction.value === "export_selected") { await onBulkExport(); return; }
} }
async function onBulkDelete(): Promise<void> { 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]; const ids = [...selectedIds.value];
if (!window.confirm(`Delete ${ids.length} scholar(s)? This removes all linked publications and queue data.`)) return; requestConfirm(
bulkBusy.value = true; `Delete ${ids.length} scholar(s)?`,
callbacks.clearMessages(); "This removes all linked publications and queue data. This action cannot be undone.",
try { "danger",
const result = await bulkDeleteScholars(ids); async () => {
callbacks.setSuccess(`${result.deleted_count} scholar(s) deleted.`); dismissConfirm();
selectedIds.value = new Set(); bulkBusy.value = true;
await callbacks.reloadScholars(); callbacks.clearMessages();
} catch (error) { try {
callbacks.assignError(error, "Unable to bulk delete scholars."); const result = await bulkDeleteScholars(ids);
} finally { callbacks.setSuccess(`${result.deleted_count} scholar(s) deleted.`);
bulkBusy.value = false; 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<void> { async function onBulkToggle(isEnabled: boolean): Promise<void> {
@ -188,6 +211,9 @@ export function useScholarBulkActions(
bulkActionOptions, bulkActionOptions,
bulkApplyLabel, bulkApplyLabel,
bulkApplyDisabled, bulkApplyDisabled,
confirmState,
dismissConfirm,
requestConfirm,
onToggleAll, onToggleAll,
onToggleRow, onToggleRow,
onApplyBulkAction, onApplyBulkAction,

View file

@ -25,7 +25,10 @@ const integrityRef = ref<InstanceType<typeof AdminIntegritySection> | null>(null
const repairsRef = ref<InstanceType<typeof AdminRepairsSection> | null>(null); const repairsRef = ref<InstanceType<typeof AdminRepairsSection> | null>(null);
const pdfQueueRef = ref<InstanceType<typeof AdminPdfQueueSection> | null>(null); const pdfQueueRef = ref<InstanceType<typeof AdminPdfQueueSection> | null>(null);
let loadGeneration = 0;
async function loadSection(): Promise<void> { async function loadSection(): Promise<void> {
const gen = ++loadGeneration;
clearAlerts(); clearAlerts();
try { try {
if (props.section === SECTION_USERS && usersRef.value) { if (props.section === SECTION_USERS && usersRef.value) {
@ -34,11 +37,13 @@ async function loadSection(): Promise<void> {
await integrityRef.value.load(); await integrityRef.value.load();
} else if (props.section === SECTION_REPAIRS) { } else if (props.section === SECTION_REPAIRS) {
await usersRef.value?.load(); await usersRef.value?.load();
if (gen !== loadGeneration) return;
await repairsRef.value?.load(); await repairsRef.value?.load();
} else if (props.section === SECTION_PDF && pdfQueueRef.value) { } else if (props.section === SECTION_PDF && pdfQueueRef.value) {
await pdfQueueRef.value.load(); await pdfQueueRef.value.load();
} }
} catch (error) { } catch (error) {
if (gen !== loadGeneration) return;
assignError(error, "Unable to load admin data."); assignError(error, "Unable to load admin data.");
} }
} }

View file

@ -6,6 +6,7 @@ import AsyncStateGate from "@/components/patterns/AsyncStateGate.vue";
import RequestStateAlerts from "@/components/patterns/RequestStateAlerts.vue"; import RequestStateAlerts from "@/components/patterns/RequestStateAlerts.vue";
import AppButton from "@/components/ui/AppButton.vue"; import AppButton from "@/components/ui/AppButton.vue";
import AppCard from "@/components/ui/AppCard.vue"; import AppCard from "@/components/ui/AppCard.vue";
import AppConfirmModal from "@/components/ui/AppConfirmModal.vue";
import AppEmptyState from "@/components/ui/AppEmptyState.vue"; import AppEmptyState from "@/components/ui/AppEmptyState.vue";
import AppHelpHint from "@/components/ui/AppHelpHint.vue"; import AppHelpHint from "@/components/ui/AppHelpHint.vue";
import AppInput from "@/components/ui/AppInput.vue"; import AppInput from "@/components/ui/AppInput.vue";
@ -272,23 +273,30 @@ async function onToggleScholar(): Promise<void> {
} }
} }
async function onDeleteScholar(): Promise<void> { function onDeleteScholar(): void {
const profile = activeScholarSettings.value; const profile = activeScholarSettings.value;
if (!profile) return; if (!profile) return;
const label = scholarLabel(profile); const label = scholarLabel(profile);
if (!window.confirm(`Delete scholar ${label}? This removes all linked publications and queue data.`)) return; bulk.requestConfirm(
activeScholarId.value = profile.id; `Delete ${label}?`,
clearMessages(); "This removes all linked publications and queue data. This action cannot be undone.",
try { "danger",
await deleteScholar(profile.id); async () => {
successMessage.value = `${label} deleted.`; bulk.dismissConfirm();
activeScholarSettingsId.value = null; activeScholarId.value = profile.id;
await loadScholars(); clearMessages();
} catch (error) { try {
assignError(error, "Unable to delete scholar."); await deleteScholar(profile.id);
} finally { successMessage.value = `${label} deleted.`;
activeScholarId.value = null; activeScholarSettingsId.value = null;
} await loadScholars();
} catch (error) {
assignError(error, "Unable to delete scholar.");
} finally {
activeScholarId.value = null;
}
},
);
} }
async function onSaveImageUrl(): Promise<void> { async function onSaveImageUrl(): Promise<void> {
@ -597,6 +605,16 @@ watch(
@toggle="onToggleScholar" @toggle="onToggleScholar"
@delete="onDeleteScholar" @delete="onDeleteScholar"
/> />
<AppConfirmModal
:open="bulk.confirmState.value.open"
:title="bulk.confirmState.value.title"
:message="bulk.confirmState.value.message"
:variant="bulk.confirmState.value.variant"
confirm-label="Delete"
@confirm="bulk.confirmState.value.onConfirm()"
@cancel="bulk.dismissConfirm()"
/>
</AppPage> </AppPage>
</template> </template>

View file

@ -168,6 +168,11 @@ function parseBoundedInteger(value: string, label: string, minimum: number): num
return parsed; 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 { function parseHoursToMinutes(value: string, minMinutes: number): number {
const hours = Number(value); const hours = Number(value);
if (!Number.isFinite(hours) || hours <= 0) { if (!Number.isFinite(hours) || hours <= 0) {
@ -175,7 +180,7 @@ function parseHoursToMinutes(value: string, minMinutes: number): number {
} }
const minutes = Math.round(hours * 60); const minutes = Math.round(hours * 60);
if (minutes < minMinutes) { 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; return minutes;
} }
@ -372,7 +377,7 @@ onMounted(async () => {
<AppHelpHint text="Minimum is controlled by server policy." /> <AppHelpHint text="Minimum is controlled by server policy." />
</span> </span>
<AppInput v-model="runIntervalHours" inputmode="decimal" /> <AppInput v-model="runIntervalHours" inputmode="decimal" />
<span class="text-xs text-secondary">Minimum: {{ minCheckIntervalMinutes / 60 }} hours</span> <span class="text-xs text-secondary">Minimum: {{ formatHours(minCheckIntervalMinutes) }} hours</span>
</label> </label>
<label class="grid gap-2 text-sm font-medium text-ink-secondary"> <label class="grid gap-2 text-sm font-medium text-ink-secondary">

View file

@ -138,3 +138,33 @@ async def test_export_with_ids_filter(db_session: AsyncSession) -> None:
resp_all = client.get("/api/v1/scholars/export") resp_all = client.get("/api/v1/scholars/export")
assert resp_all.status_code == 200 assert resp_all.status_code == 200
assert len(resp_all.json()["data"]["scholars"]) == 2 assert len(resp_all.json()["data"]["scholars"]) == 2
@pytest.mark.integration
@pytest.mark.db
@pytest.mark.asyncio
async def test_bulk_delete_rejects_without_csrf(db_session: AsyncSession) -> None:
await insert_user(db_session, email="csrf-bulk-del@example.com", password="pw123456")
client = TestClient(app)
login_user(client, email="csrf-bulk-del@example.com", password="pw123456")
response = client.post(
"/api/v1/scholars/bulk-delete",
json={"scholar_profile_ids": [1]},
)
assert response.status_code == 403
assert response.json()["error"]["code"] == "csrf_invalid"
@pytest.mark.integration
@pytest.mark.db
@pytest.mark.asyncio
async def test_bulk_toggle_rejects_without_csrf(db_session: AsyncSession) -> None:
await insert_user(db_session, email="csrf-bulk-tog@example.com", password="pw123456")
client = TestClient(app)
login_user(client, email="csrf-bulk-tog@example.com", password="pw123456")
response = client.post(
"/api/v1/scholars/bulk-toggle",
json={"scholar_profile_ids": [1], "is_enabled": False},
)
assert response.status_code == 403
assert response.json()["error"]["code"] == "csrf_invalid"