diff --git a/backend/app/services/relationship_service.py b/backend/app/services/relationship_service.py index 0f3df4c..59229ff 100644 --- a/backend/app/services/relationship_service.py +++ b/backend/app/services/relationship_service.py @@ -4,7 +4,7 @@ Writes require editor rights; reads go through the privacy engine.""" import uuid from datetime import UTC, datetime -from sqlalchemy import or_, select +from sqlalchemy import and_, or_, select from sqlalchemy.ext.asyncio import AsyncSession from app.models.enums import ParentChildQualifier, RelationshipType @@ -49,6 +49,38 @@ async def create_relationship( if not await _person_in_tree(session, pid, tree.id): raise NotFound("person not found in this tree") + # Reject an equivalent existing edge so the same two people can't be linked + # the same way twice. parent_child is directional (parent -> child); + # partnership/sibling are symmetric, so match the pair in either order. + if type is RelationshipType.parent_child: + pair = and_( + Relationship.person_from_id == person_from_id, + Relationship.person_to_id == person_to_id, + ) + else: + pair = or_( + and_( + Relationship.person_from_id == person_from_id, + Relationship.person_to_id == person_to_id, + ), + and_( + Relationship.person_from_id == person_to_id, + Relationship.person_to_id == person_from_id, + ), + ) + existing = ( + await session.execute( + select(Relationship.id).where( + Relationship.tree_id == tree.id, + Relationship.type == type, + Relationship.deleted_at.is_(None), + pair, + ) + ) + ).scalar_one_or_none() + if existing is not None: + raise Conflict("these two people are already linked that way") + relationship = Relationship( tree_id=tree.id, type=type, diff --git a/backend/tests/test_relationship_dedup.py b/backend/tests/test_relationship_dedup.py new file mode 100644 index 0000000..89101bc --- /dev/null +++ b/backend/tests/test_relationship_dedup.py @@ -0,0 +1,65 @@ +"""Duplicate relationships are rejected (no double-linking).""" + +from tests.conftest import auth, register + + +async def _setup(client, email): + h = auth(await register(client, email)) + tid = (await client.post("/api/v1/trees", json={"name": "T"}, headers=h)).json()["id"] + + async def person(given): + return ( + await client.post(f"/api/v1/trees/{tid}/persons", json={"given": given}, headers=h) + ).json()["id"] + + return h, tid, person + + +async def test_duplicate_parent_child_rejected(client): + h, tid, person = await _setup(client, "dup-pc@example.com") + karl = await person("Karl") + kid = await person("Kid") + body = {"type": "parent_child", "person_from_id": karl, "person_to_id": kid} + + first = await client.post(f"/api/v1/trees/{tid}/relationships", json=body, headers=h) + assert first.status_code == 201 + dup = await client.post(f"/api/v1/trees/{tid}/relationships", json=body, headers=h) + assert dup.status_code == 409 + + +async def test_duplicate_partnership_either_direction_rejected(client): + h, tid, person = await _setup(client, "dup-sp@example.com") + a = await person("A") + b = await person("B") + + first = await client.post( + f"/api/v1/trees/{tid}/relationships", + json={"type": "partnership", "person_from_id": a, "person_to_id": b}, + headers=h, + ) + assert first.status_code == 201 + # Same couple, reversed order — still a duplicate. + dup = await client.post( + f"/api/v1/trees/{tid}/relationships", + json={"type": "partnership", "person_from_id": b, "person_to_id": a}, + headers=h, + ) + assert dup.status_code == 409 + + +async def test_reverse_parent_child_is_allowed(client): + """A->B as parent_child shouldn't block B->A (different meaning).""" + h, tid, person = await _setup(client, "dup-rev@example.com") + a = await person("A") + b = await person("B") + r1 = await client.post( + f"/api/v1/trees/{tid}/relationships", + json={"type": "parent_child", "person_from_id": a, "person_to_id": b}, + headers=h, + ) + r2 = await client.post( + f"/api/v1/trees/{tid}/relationships", + json={"type": "parent_child", "person_from_id": b, "person_to_id": a}, + headers=h, + ) + assert r1.status_code == 201 and r2.status_code == 201 diff --git a/frontend/app/trees/[id]/persons/[personId]/page.tsx b/frontend/app/trees/[id]/persons/[personId]/page.tsx index 506c697..5e298c3 100644 --- a/frontend/app/trees/[id]/persons/[personId]/page.tsx +++ b/frontend/app/trees/[id]/persons/[personId]/page.tsx @@ -150,6 +150,7 @@ export default function PersonDetailPage() { const [relKind, setRelKind] = useState<"parent" | "child" | "partner" | "sibling">("parent"); const [relOther, setRelOther] = useState(""); const [relQual, setRelQual] = useState("biological"); + const [relErr, setRelErr] = useState(null); // Add-name form + inline edit. const [nameType, setNameType] = useState("married"); @@ -363,9 +364,12 @@ export default function PersonDetailPage() { async function addRel(e: React.FormEvent) { e.preventDefault(); if (!relOther) return; + setRelErr(null); if (await linkRelative(relOther)) { setRelOther(""); load(); + } else { + setRelErr("They're already linked that way."); } } @@ -1142,6 +1146,7 @@ export default function PersonDetailPage() { )} + {relErr &&

{relErr}

} diff --git a/frontend/app/trees/[id]/tree/page.tsx b/frontend/app/trees/[id]/tree/page.tsx index 3cdb672..73dbbc1 100644 --- a/frontend/app/trees/[id]/tree/page.tsx +++ b/frontend/app/trees/[id]/tree/page.tsx @@ -39,6 +39,7 @@ export default function TreePage() { const [status, setStatus] = useState<"loading" | "empty" | "ready" | "error">("loading"); const [focusId, setFocusId] = useState(null); const [mode, setMode] = useState("landscape"); + const [renderNote, setRenderNote] = useState(null); useEffect(() => { let cancelled = false; @@ -108,11 +109,46 @@ export default function TreePage() { if (status !== "ready" || mode === "fan" || !containerRef.current) return; let cancelled = false; (async () => { - // Only link to people that still exist — a soft-deleted person leaves - // dangling relationship rows, and family-chart breaks on an id with no - // matching datum. Filter them out so a deletion never blanks the tree. + // Sanitize the graph before handing it to family-chart, which recurses + // through parents and will blow the stack (blank tree) on a cycle — e.g. a + // person edited into being their own ancestor. const alive = new Set(people.map((pp) => pp.id)); - const keep = (ids: string[]) => ids.filter((id) => alive.has(id)); + const ok = (ids: string[], self: string) => + [...new Set(ids)].filter((id) => alive.has(id) && id !== self); + + // Build an acyclic set of parent edges: skip any edge that would make a + // person their own ancestor. Children are derived from the kept edges so + // parent/child stays consistent. + const parentsMap = new Map(); + const childrenMap = new Map(); + let dropped = 0; + const isAncestorOf = (ancestor: string, of: string): boolean => { + const stack = [...(parentsMap.get(of) ?? [])]; + const seen = new Set(); + while (stack.length) { + const n = stack.pop()!; + if (n === ancestor) return true; + if (seen.has(n)) continue; + seen.add(n); + for (const p of parentsMap.get(n) ?? []) stack.push(p); + } + return false; + }; + for (const pp of people) { + const accepted: string[] = []; + for (const par of ok(parentsOf(pp.id), pp.id)) { + // Edge "pp has parent par" loops if pp is already an ancestor of par. + if (isAncestorOf(pp.id, par)) { + dropped++; + continue; + } + accepted.push(par); + parentsMap.set(pp.id, accepted); + childrenMap.set(par, [...(childrenMap.get(par) ?? []), pp.id]); + } + parentsMap.set(pp.id, accepted); + } + const data = people.map((pp) => { const [fn, ln] = splitName(pp.primary_name); return { @@ -124,32 +160,47 @@ export default function TreePage() { gender: pp.gender === "female" ? "F" : "M", }, rels: { - spouses: keep(partnersOf(pp.id)), - parents: keep(parentsOf(pp.id)), - children: keep(childrenOf(pp.id)), + spouses: ok(partnersOf(pp.id), pp.id), + parents: parentsMap.get(pp.id) ?? [], + children: childrenMap.get(pp.id) ?? [], }, }; }); const f3 = await import("family-chart"); if (cancelled || !containerRef.current) return; - containerRef.current.innerHTML = ""; - const chart = f3.createChart(containerRef.current, data); - chart.setCardHtml().setCardDisplay([["first name", "last name"], ["birthday"]]); - if (mode === "portrait") chart.setOrientationVertical(); - else chart.setOrientationHorizontal(); - // Show enough generations that a recenter reveals grandparents + children. - chart.setAncestryDepth?.(3); - chart.setProgenyDepth?.(2); - // Default card click recenters the whole hourglass; sync focus for the - // "Open profile" link after every (re)build. - chart.setAfterUpdate?.(() => { - const md = chart.getMainDatum?.(); - const id = md?.id ?? md?.data?.id; - if (id) setFocusId(id); - }); - chartRef.current = chart; - if (focusId) chart.updateMainId(focusId); - chart.updateTree({ initial: true }); + try { + containerRef.current.innerHTML = ""; + const chart = f3.createChart(containerRef.current, data); + chart.setCardHtml().setCardDisplay([["first name", "last name"], ["birthday"]]); + if (mode === "portrait") chart.setOrientationVertical(); + else chart.setOrientationHorizontal(); + // Show enough generations that a recenter reveals grandparents + children. + chart.setAncestryDepth?.(3); + chart.setProgenyDepth?.(2); + // Default card click recenters the whole hourglass; sync focus for the + // "Open profile" link after every (re)build. + chart.setAfterUpdate?.(() => { + const md = chart.getMainDatum?.(); + const id = md?.id ?? md?.data?.id; + if (id) setFocusId(id); + }); + chartRef.current = chart; + if (focusId) chart.updateMainId(focusId); + chart.updateTree({ initial: true }); + setRenderNote( + dropped > 0 + ? `Skipped ${dropped} conflicting parent link${dropped === 1 ? "" : "s"} (a person can't be their own ancestor). Open the people involved to fix the relationship.` + : null, + ); + } catch (err) { + // Never leave a blank canvas — show a message and let them fix via the + // Family view / person pages. + console.error("tree render failed", err); + if (containerRef.current) containerRef.current.innerHTML = ""; + setRenderNote( + "The tree couldn't be drawn — a relationship may be conflicting. Use the Family view to open the affected people and check their parents/children.", + ); + } })(); return () => { cancelled = true; @@ -242,6 +293,12 @@ export default function TreePage() { )} {status === "error" &&

Could not render the tree.

} + {renderNote && mode !== "fan" && ( +

+ {renderNote} +

+ )} + {status === "ready" && mode === "fan" && focusId ? (