diff --git a/app/api/routers/scholars.py b/app/api/routers/scholars.py index 7bbc642..ae3c522 100644 --- a/app/api/routers/scholars.py +++ b/app/api/routers/scholars.py @@ -261,11 +261,18 @@ async def delete_scholar( code="scholar_not_found", message="Scholar not found.", ) - await scholar_service.delete_scholar( - db_session, - profile=profile, - upload_dir=settings.scholar_image_upload_dir, - ) + try: + await scholar_service.delete_scholar( + db_session, + profile=profile, + upload_dir=settings.scholar_image_upload_dir, + ) + except scholar_service.ScholarServiceError as exc: + raise ApiException( + status_code=409, + code="scholar_delete_failed", + message=str(exc), + ) from exc structured_log( logger, "info", "api.scholars.deleted", user_id=current_user.id, scholar_profile_id=scholar_profile_id ) diff --git a/app/services/scholars/application.py b/app/services/scholars/application.py index 028e3fc..75f5e73 100644 --- a/app/services/scholars/application.py +++ b/app/services/scholars/application.py @@ -125,7 +125,11 @@ async def delete_scholar( _safe_remove_upload(upload_root, profile.profile_image_upload_path) await db_session.delete(profile) - await db_session.commit() + try: + await db_session.commit() + except IntegrityError as exc: + await db_session.rollback() + raise ScholarServiceError("Unable to delete scholar due to a database constraint.") from exc async def hydrate_profile_metadata( diff --git a/frontend/src/features/scholars/components/ScholarBatchAdd.test.ts b/frontend/src/features/scholars/components/ScholarBatchAdd.test.ts index 2d76eac..7b25632 100644 --- a/frontend/src/features/scholars/components/ScholarBatchAdd.test.ts +++ b/frontend/src/features/scholars/components/ScholarBatchAdd.test.ts @@ -2,10 +2,144 @@ import { describe, expect, it } from "vitest"; import { mount } from "@vue/test-utils"; import ScholarBatchAdd from "./ScholarBatchAdd.vue"; +import { + parseScholarIds, + parseScholarTokens, + extractScholarIdFromUrl, + validateTokenAsId, +} from "./scholar-batch-parsing"; const defaultProps = { saving: false, loading: false }; -describe("ScholarBatchAdd", () => { +describe("parseScholarIds (unit)", () => { + it("parses a single bare ID", () => { + expect(parseScholarIds("A-UbBTPM15wL")).toEqual(["A-UbBTPM15wL"]); + }); + + it("parses multiple IDs separated by commas", () => { + expect(parseScholarIds("A-UbBTPM15wL, B-UbBTPM15wL")).toEqual(["A-UbBTPM15wL", "B-UbBTPM15wL"]); + }); + + it("deduplicates IDs", () => { + expect(parseScholarIds("A-UbBTPM15wL\nA-UbBTPM15wL")).toEqual(["A-UbBTPM15wL"]); + }); + + it("extracts ID from standard Google Scholar URL", () => { + expect(parseScholarIds("https://scholar.google.com/citations?hl=en&user=A-UbBTPM15wL")).toEqual(["A-UbBTPM15wL"]); + }); + + it("extracts ID from URL with trailing slash", () => { + expect(parseScholarIds("https://scholar.google.com/citations?user=A-UbBTPM15wL/")).toEqual(["A-UbBTPM15wL"]); + }); + + it("extracts ID from URL with fragment", () => { + expect(parseScholarIds("https://scholar.google.com/citations?user=A-UbBTPM15wL#section")).toEqual(["A-UbBTPM15wL"]); + }); + + it("extracts ID from URL with extra query params", () => { + expect(parseScholarIds("https://scholar.google.com/citations?hl=en&user=A-UbBTPM15wL&view_op=list_works&sortby=pubdate")).toEqual(["A-UbBTPM15wL"]); + }); + + it("handles URL-encoded characters in non-ID parts", () => { + expect(parseScholarIds("https://scholar.google.com/citations?hl=en&user=A-UbBTPM15wL&label=%E4%B8%AD%E6%96%87")).toEqual(["A-UbBTPM15wL"]); + }); + + it("rejects malformed IDs (too short)", () => { + expect(parseScholarIds("ABC123")).toEqual([]); + }); + + it("rejects malformed IDs (too long)", () => { + expect(parseScholarIds("ABCDEF1234567")).toEqual([]); + }); + + it("rejects IDs with special characters", () => { + expect(parseScholarIds("ABCDEF12345!")).toEqual([]); + }); + + it("rejects IDs with embedded whitespace", () => { + expect(parseScholarIds("ABC DEF12345")).toEqual([]); + }); + + it("handles mixed valid and invalid tokens", () => { + expect(parseScholarIds("A-UbBTPM15wL, invalid, B-UbBTPM15wL")).toEqual(["A-UbBTPM15wL", "B-UbBTPM15wL"]); + }); + + it("returns empty array for empty string", () => { + expect(parseScholarIds("")).toEqual([]); + }); + + it("returns empty array for whitespace only", () => { + expect(parseScholarIds(" ")).toEqual([]); + }); +}); + +describe("extractScholarIdFromUrl", () => { + it("returns null for non-URL strings", () => { + expect(extractScholarIdFromUrl("not-a-url")).toBeNull(); + }); + + it("returns null for URL without user param", () => { + expect(extractScholarIdFromUrl("https://scholar.google.com/citations?hl=en")).toBeNull(); + }); + + it("extracts from URL with trailing slashes", () => { + expect(extractScholarIdFromUrl("https://scholar.google.com/citations?user=A-UbBTPM15wL///")).toBe("A-UbBTPM15wL"); + }); + + it("strips fragment before parsing", () => { + expect(extractScholarIdFromUrl("https://scholar.google.com/citations?user=A-UbBTPM15wL#foo")).toBe("A-UbBTPM15wL"); + }); + + it("returns null when user param is invalid length", () => { + expect(extractScholarIdFromUrl("https://scholar.google.com/citations?user=short")).toBeNull(); + }); +}); + +describe("validateTokenAsId", () => { + it("returns null for valid 12-char ID", () => { + expect(validateTokenAsId("ABCDEF123456")).toBeNull(); + }); + + it("detects wrong length", () => { + expect(validateTokenAsId("ABC")).toContain("12 characters"); + }); + + it("detects invalid characters", () => { + expect(validateTokenAsId("ABCDEF12345!")).toContain("invalid characters"); + }); + + it("detects whitespace", () => { + expect(validateTokenAsId("ABC DEF12345")).toContain("whitespace"); + }); + + it("detects empty input", () => { + expect(validateTokenAsId("")).toContain("empty"); + }); +}); + +describe("parseScholarTokens (per-token errors)", () => { + it("marks invalid tokens with error messages", () => { + const tokens = parseScholarTokens("A-UbBTPM15wL, short, B-UbBTPM15wL"); + expect(tokens).toHaveLength(3); + expect(tokens[0].id).toBe("A-UbBTPM15wL"); + expect(tokens[0].error).toBeNull(); + expect(tokens[1].id).toBeNull(); + expect(tokens[1].error).toContain("12 characters"); + expect(tokens[2].id).toBe("B-UbBTPM15wL"); + }); + + it("marks duplicate tokens", () => { + const tokens = parseScholarTokens("A-UbBTPM15wL, A-UbBTPM15wL"); + expect(tokens[1].error).toBe("duplicate"); + }); + + it("marks bad URL tokens", () => { + const tokens = parseScholarTokens("https://scholar.google.com/citations?hl=en"); + expect(tokens[0].error).toContain("could not extract"); + }); +}); + +describe("ScholarBatchAdd (component)", () => { it("renders the heading and input", () => { const wrapper = mount(ScholarBatchAdd, { props: defaultProps }); expect(wrapper.text()).toContain("Add Scholar Profiles"); @@ -73,4 +207,18 @@ describe("ScholarBatchAdd", () => { const wrapper = mount(ScholarBatchAdd, { props: { ...defaultProps, saving: true } }); expect(wrapper.text()).toContain("Adding..."); }); + + it("shows validation errors for invalid tokens", async () => { + const wrapper = mount(ScholarBatchAdd, { props: defaultProps }); + await wrapper.find("textarea").setValue("A-UbBTPM15wL, bad!"); + const errors = wrapper.find("[data-testid='validation-errors']"); + expect(errors.exists()).toBe(true); + }); + + it("shows skipped count alongside valid count", async () => { + const wrapper = mount(ScholarBatchAdd, { props: defaultProps }); + await wrapper.find("textarea").setValue("A-UbBTPM15wL, tooshort"); + expect(wrapper.text()).toContain("1 valid"); + expect(wrapper.text()).toContain("1 skipped"); + }); }); diff --git a/frontend/src/features/scholars/components/ScholarBatchAdd.vue b/frontend/src/features/scholars/components/ScholarBatchAdd.vue index bfafbda..60f89b9 100644 --- a/frontend/src/features/scholars/components/ScholarBatchAdd.vue +++ b/frontend/src/features/scholars/components/ScholarBatchAdd.vue @@ -3,6 +3,7 @@ import { computed, ref } from "vue"; import AppButton from "@/components/ui/AppButton.vue"; import AppCard from "@/components/ui/AppCard.vue"; import AppHelpHint from "@/components/ui/AppHelpHint.vue"; +import { parseScholarTokens } from "./scholar-batch-parsing"; defineProps<{ saving: boolean; @@ -15,38 +16,13 @@ const emit = defineEmits<{ const scholarBatchInput = ref(""); -const SCHOLAR_ID_PATTERN = /^[a-zA-Z0-9_-]{12}$/; -const URL_USER_PARAM_PATTERN = /(?:\?|&)user=([a-zA-Z0-9_-]{12})(?:&|#|$)/i; - -function parseScholarIds(raw: string): string[] { - const ordered: string[] = []; - const seen = new Set(); - const tokens = raw.split(/[\s,;]+/).map((v) => v.trim()).filter((v) => v.length > 0); - for (const token of tokens) { - let candidate: string | null = null; - if (SCHOLAR_ID_PATTERN.test(token)) candidate = token; - if (!candidate) { - const directParamMatch = token.match(URL_USER_PARAM_PATTERN); - if (directParamMatch) candidate = directParamMatch[1]; - } - if (!candidate && token.includes("scholar.google")) { - try { - const parsed = new URL(token); - const userParam = parsed.searchParams.get("user"); - if (userParam && SCHOLAR_ID_PATTERN.test(userParam)) candidate = userParam; - } catch (_error) { /* Ignore non-URL tokens. */ } - } - if (!candidate || seen.has(candidate)) continue; - seen.add(candidate); - ordered.push(candidate); - } - return ordered; -} - -const parsedBatchCount = computed(() => parseScholarIds(scholarBatchInput.value).length); +const parsedTokens = computed(() => parseScholarTokens(scholarBatchInput.value)); +const validIds = computed(() => parsedTokens.value.filter((t) => t.id !== null)); +const invalidTokens = computed(() => parsedTokens.value.filter((t) => t.id === null)); +const parsedBatchCount = computed(() => validIds.value.length); function onSubmit(): void { - const ids = parseScholarIds(scholarBatchInput.value); + const ids = validIds.value.map((t) => t.id!); if (ids.length > 0) { emit("add-scholars", ids); scholarBatchInput.value = ""; @@ -76,11 +52,33 @@ function onSubmit(): void { /> -
+

- Parsed IDs: {{ parsedBatchCount }} + {{ parsedBatchCount }} valid ID{{ parsedBatchCount === 1 ? "" : "s" }} +

- +
    +
  • + #{{ item.index }}: {{ item.error }} + {{ item.raw.length > 60 ? item.raw.slice(0, 57) + "..." : item.raw }} +
    • +
  • + +{{ invalidTokens.length - 5 }} more skipped +
    • +
+
+
+ Parsed IDs: 0 +
+ +
+ {{ saving ? "Adding..." : "Add scholars" }}
diff --git a/frontend/src/features/scholars/components/ScholarSettingsModal.vue b/frontend/src/features/scholars/components/ScholarSettingsModal.vue index 4a129f8..511bf66 100644 --- a/frontend/src/features/scholars/components/ScholarSettingsModal.vue +++ b/frontend/src/features/scholars/components/ScholarSettingsModal.vue @@ -47,7 +47,14 @@ function scholarPublicationsRoute(profile: ScholarProfile): { name: string; quer :image-url="scholar.profile_image_url" />
-

{{ scholarLabel(scholar) }}

+

+ {{ scholarLabel(scholar) }} + Pending +

ID: {{ scholar.scholar_id }}

diff --git a/frontend/src/features/scholars/components/ScholarStatusBadges.vue b/frontend/src/features/scholars/components/ScholarStatusBadges.vue new file mode 100644 index 0000000..5460658 --- /dev/null +++ b/frontend/src/features/scholars/components/ScholarStatusBadges.vue @@ -0,0 +1,29 @@ + + + diff --git a/frontend/src/features/scholars/components/scholar-batch-parsing.ts b/frontend/src/features/scholars/components/scholar-batch-parsing.ts new file mode 100644 index 0000000..45baba7 --- /dev/null +++ b/frontend/src/features/scholars/components/scholar-batch-parsing.ts @@ -0,0 +1,76 @@ +const SCHOLAR_ID_PATTERN = /^[a-zA-Z0-9_-]{12}$/; + +export interface ParsedToken { + index: number; + raw: string; + id: string | null; + error: string | null; +} + +export function extractScholarIdFromUrl(token: string): string | null { + const cleaned = token.replace(/\/+$/, "").replace(/#.*$/, ""); + try { + const parsed = new URL(cleaned); + const userParam = parsed.searchParams.get("user"); + if (!userParam) return null; + const decoded = decodeURIComponent(userParam).trim(); + if (SCHOLAR_ID_PATTERN.test(decoded)) return decoded; + return null; + } catch { + return null; + } +} + +export function validateTokenAsId(token: string): string | null { + const trimmed = token.trim(); + if (!trimmed) return "empty input"; + if (/\s/.test(trimmed)) return "contains whitespace"; + if (trimmed.length !== 12) return `must be 12 characters (got ${trimmed.length})`; + if (!/^[a-zA-Z0-9_-]+$/.test(trimmed)) { + return "contains invalid characters (only a-z, A-Z, 0-9, _ and - allowed)"; + } + return null; +} + +export function parseScholarTokens(raw: string): ParsedToken[] { + const tokens = raw.split(/[\s,;]+/).map((v) => v.trim()).filter((v) => v.length > 0); + const results: ParsedToken[] = []; + const seen = new Set(); + + for (let i = 0; i < tokens.length; i++) { + const token = tokens[i]; + + if (SCHOLAR_ID_PATTERN.test(token)) { + if (seen.has(token)) { + results.push({ index: i + 1, raw: token, id: null, error: "duplicate" }); + continue; + } + seen.add(token); + results.push({ index: i + 1, raw: token, id: token, error: null }); + continue; + } + + if (token.includes("scholar.google") || token.startsWith("http")) { + const extracted = extractScholarIdFromUrl(token); + if (extracted) { + if (seen.has(extracted)) { + results.push({ index: i + 1, raw: token, id: null, error: "duplicate" }); + continue; + } + seen.add(extracted); + results.push({ index: i + 1, raw: token, id: extracted, error: null }); + continue; + } + results.push({ index: i + 1, raw: token, id: null, error: "could not extract scholar ID from URL" }); + continue; + } + + const reason = validateTokenAsId(token); + results.push({ index: i + 1, raw: token, id: null, error: reason ?? "invalid scholar ID" }); + } + return results; +} + +export function parseScholarIds(raw: string): string[] { + return parseScholarTokens(raw).filter((t) => t.id !== null).map((t) => t.id!); +} diff --git a/frontend/src/pages/DashboardPage.vue b/frontend/src/pages/DashboardPage.vue index c7da221..3f7df35 100644 --- a/frontend/src/pages/DashboardPage.vue +++ b/frontend/src/pages/DashboardPage.vue @@ -494,6 +494,11 @@ watch(

@@ -527,6 +532,9 @@ watch(
{{ formatDate(run.start_dt) }} • {{ run.new_publication_count }} new + diff --git a/frontend/src/pages/ScholarsPage.vue b/frontend/src/pages/ScholarsPage.vue index 16b2a96..29541d9 100644 --- a/frontend/src/pages/ScholarsPage.vue +++ b/frontend/src/pages/ScholarsPage.vue @@ -31,6 +31,7 @@ import ScholarAvatar from "@/features/scholars/components/ScholarAvatar.vue"; import ScholarBatchAdd from "@/features/scholars/components/ScholarBatchAdd.vue"; import ScholarNameSearch from "@/features/scholars/components/ScholarNameSearch.vue"; import ScholarSettingsModal from "@/features/scholars/components/ScholarSettingsModal.vue"; +import ScholarStatusBadges from "@/features/scholars/components/ScholarStatusBadges.vue"; import { ApiRequestError } from "@/lib/api/errors"; import { useRunStatusStore } from "@/stores/run_status"; @@ -505,7 +506,10 @@ watch(
-

{{ scholarLabel(item) }}

+

+ {{ scholarLabel(item) }} + +

Publications Open profile @@ -529,7 +533,10 @@ watch(
- {{ scholarLabel(item) }} + + {{ scholarLabel(item) }} + +
Publications Open profile diff --git a/tests/integration/test_api_scholars.py b/tests/integration/test_api_scholars.py index 0d6f3a0..1fd8ba9 100644 --- a/tests/integration/test_api_scholars.py +++ b/tests/integration/test_api_scholars.py @@ -70,6 +70,110 @@ async def test_api_scholars_crud_flow(db_session: AsyncSession) -> None: assert delete_response.json()["data"]["message"] == "Scholar deleted." +@pytest.mark.integration +@pytest.mark.db +@pytest.mark.asyncio +async def test_api_delete_scholar_returns_404_for_nonexistent(db_session: AsyncSession) -> None: + await insert_user( + db_session, + email="api-delete-404@example.com", + password="api-password", + ) + + client = TestClient(app) + login_user(client, email="api-delete-404@example.com", password="api-password") + headers = api_csrf_headers(client) + + response = client.delete("/api/v1/scholars/999999", headers=headers) + assert response.status_code == 404 + assert response.json()["error"]["code"] == "scholar_not_found" + + +@pytest.mark.integration +@pytest.mark.db +@pytest.mark.asyncio +async def test_api_delete_scholar_cascades_linked_publications(db_session: AsyncSession) -> None: + user_id = await insert_user( + db_session, + email="api-delete-cascade@example.com", + password="api-password", + ) + scholar_result = await db_session.execute( + text( + """ + INSERT INTO scholar_profiles (user_id, scholar_id, display_name, is_enabled) + VALUES (:user_id, :scholar_id, :display_name, true) + RETURNING id + """ + ), + {"user_id": user_id, "scholar_id": "delCASCADE01", "display_name": "Cascade Test"}, + ) + scholar_profile_id = int(scholar_result.scalar_one()) + pub_result = await db_session.execute( + text( + """ + INSERT INTO publications (fingerprint_sha256, title_raw, title_normalized, citation_count) + VALUES (:fingerprint, :title_raw, :title_normalized, 0) + RETURNING id + """ + ), + { + "fingerprint": f"{(user_id + 7000):064x}", + "title_raw": "Cascade Publication", + "title_normalized": "cascadepublication", + }, + ) + publication_id = int(pub_result.scalar_one()) + await db_session.execute( + text( + """ + INSERT INTO scholar_publications (scholar_profile_id, publication_id, is_read) + VALUES (:scholar_profile_id, :publication_id, false) + """ + ), + {"scholar_profile_id": scholar_profile_id, "publication_id": publication_id}, + ) + await db_session.commit() + + client = TestClient(app) + login_user(client, email="api-delete-cascade@example.com", password="api-password") + headers = api_csrf_headers(client) + + delete_response = client.delete(f"/api/v1/scholars/{scholar_profile_id}", headers=headers) + assert delete_response.status_code == 200 + assert delete_response.json()["data"]["message"] == "Scholar deleted." + + link_result = await db_session.execute( + text("SELECT count(*) FROM scholar_publications WHERE scholar_profile_id = :id"), + {"id": scholar_profile_id}, + ) + assert int(link_result.scalar_one()) == 0 + + +@pytest.mark.integration +@pytest.mark.db +@pytest.mark.asyncio +async def test_api_create_scholar_rejects_corrupted_id(db_session: AsyncSession) -> None: + await insert_user( + db_session, + email="api-bad-id@example.com", + password="api-password", + ) + + client = TestClient(app) + login_user(client, email="api-bad-id@example.com", password="api-password") + headers = api_csrf_headers(client) + + for bad_id in ["short", "ABCDEF12345!", "", " ", "AB CD EF1234"]: + response = client.post( + "/api/v1/scholars", + json={"scholar_id": bad_id}, + headers=headers, + ) + assert response.status_code == 400, f"Expected 400 for scholar_id={bad_id!r}" + assert response.json()["error"]["code"] == "invalid_scholar" + + @pytest.mark.integration @pytest.mark.db @pytest.mark.asyncio diff --git a/tests/unit/test_scholar_validators.py b/tests/unit/test_scholar_validators.py index 460b315..185d805 100644 --- a/tests/unit/test_scholar_validators.py +++ b/tests/unit/test_scholar_validators.py @@ -42,6 +42,34 @@ class TestValidateScholarId: with pytest.raises(ScholarServiceError, match="12"): validate_scholar_id("ABCDEF12345!") + def test_rejects_empty_string(self) -> None: + with pytest.raises(ScholarServiceError, match="12"): + validate_scholar_id("") + + def test_rejects_whitespace_only(self) -> None: + with pytest.raises(ScholarServiceError, match="12"): + validate_scholar_id(" ") + + def test_rejects_embedded_spaces(self) -> None: + with pytest.raises(ScholarServiceError, match="12"): + validate_scholar_id("ABCDEF 12345") + + def test_rejects_tab_characters(self) -> None: + with pytest.raises(ScholarServiceError, match="12"): + validate_scholar_id("ABCDEF\t12345") + + def test_rejects_url_as_id(self) -> None: + with pytest.raises(ScholarServiceError, match="12"): + validate_scholar_id("https://scholar.google.com/citations?user=ABCDEF123456") + + def test_rejects_newline_in_id(self) -> None: + with pytest.raises(ScholarServiceError, match="12"): + validate_scholar_id("ABCDEF\n12345") + + def test_rejects_unicode_characters(self) -> None: + with pytest.raises(ScholarServiceError, match="12"): + validate_scholar_id("ABCDEF12345\u00e9") + class TestNormalizeDisplayName: def test_returns_stripped_name(self) -> None: