diff --git a/.github/instructions/services.instructions.md b/.github/instructions/services.instructions.md index fb555fa..acd85c5 100644 --- a/.github/instructions/services.instructions.md +++ b/.github/instructions/services.instructions.md @@ -136,7 +136,7 @@ Atomicity rules: ## Schema Drift and Legacy Compatibility Policy -- Prefer schema migration or startup reconciliation over runtime compatibility paths in service writes. +- Prefer schema migration over startup reconciliation or runtime compatibility paths in service writes. - Do not add legacy read/write compatibility code in service workflows by default. - If drift is discovered and a migration decision is ambiguous (for example, one-way destructive DDL, uncertain data retention impact, or unknown deployment sequence), pause and ask the user to choose migration vs compatibility before coding. - If a temporary compatibility path is explicitly approved, document an expiration/removal plan in the same change. diff --git a/.gitignore b/.gitignore index e1b3d32..c9dc1f0 100644 --- a/.gitignore +++ b/.gitignore @@ -23,8 +23,7 @@ data/* .test-backups/ # Temporary migration files -.migration-bundle/* -.migration-bundle-v5test/* -.migration-bundle-v5test2/* -data.old/* +.migration-bundle-v51 +data.pre-v50-20260823/* +data.pre-v51-20260823-120434/* diff --git a/docs/data_migration.md b/docs/data_migration.md index 794c8dc..3cdcb4d 100644 --- a/docs/data_migration.md +++ b/docs/data_migration.md @@ -33,7 +33,7 @@ uv run python tools/export_import_migration.py migrate --bundle-dir .migration-b ## What gets migrated -- Tables (in dependency order): `document_type`, `person_role`, `tag`, `document`, `person`, `photo`, `document_person`, `document_tag`, `job`, `source`, `job_source`, `execution_attempt`. +- Tables (in dependency order): `document_type`, `person_role`, `tag`, `document`, `person`, `photo`, `document_person`, `document_tag`, `person_tag`, `job`, `source`, `job_source`, `execution_attempt`. - Media tree under `UPLOAD_DIR`. The bundle contains: @@ -48,6 +48,7 @@ Legacy V4.x portrait/homepage backfill in the export step: - If the source DB has no `photo` table, the exporter synthesizes `photo` rows from legacy `person.portrait_path` values and from legacy homepage image files under `UPLOAD_DIR/homepage`. - Legacy portrait and homepage image files are copied into the unified `UPLOAD_DIR/photos/{photo_id}{suffix}` layout in the migration bundle. - Legacy homepage markdown is relocated from `UPLOAD_DIR/homepage/homepage.md` to `UPLOAD_DIR/homepage.md`. +- Legacy `person.full_name` values are split into `given_names` + `last_name` for V5.1 schema compatibility. ## Cutover diff --git a/docs/roadmap_plan.md b/docs/roadmap_plan.md index 8671b85..fc8aa6f 100644 --- a/docs/roadmap_plan.md +++ b/docs/roadmap_plan.md @@ -137,6 +137,25 @@ Migration policy for legacy installs: --- +## V5.1 — Person table structural redesign + +- Replace `person.full_name` with split required fields: + - `last_name` + - `given_names` (first + middle) +- Remove `display_name` and `maiden_name` from active schema/UI. +- Keep `family_search_id` optional and unique (not required in this version). +- Add `person_tag` many-to-many links so People use the same Tag registry as Documents. +- Update Archival Entities: People list columns to: + - Last Name + - First & Middle + - FamilySearch ID + - Birth Date + - Death Date + - # Documents +- Migration/export-import behavior backfills split names from legacy `full_name` values. + +--- + ## V6.0 — Server Hosting Migration Your stated approach (Postgres in Docker, app in Docker, Cloudflare Tunnel) is @@ -210,8 +229,11 @@ forecloses them later: --- -## Open items for you -- V5.0 unified `photos` table: needs the follow-up design discussion you - flagged (exact schema, how photos link to homepage vs. person context) - before implementation. -- Confirm this version numbering/grouping matches your intent before work starts. +## Current status +- V5.0 unified photos has been implemented with: + - shared `photo` table (`person_id` nullable for homepage ownership), + - flat media storage under `UPLOAD_DIR/photos/{photo_id}{suffix}`, + - migration backfill from legacy Person portraits and homepage images, + - homepage markdown relocated to `UPLOAD_DIR/homepage.md`. +- Version numbering/grouping is now established by implementation and can + proceed to V5.1 planning/execution. diff --git a/docs/schema.md b/docs/schema.md index 963b0aa..955d6de 100644 --- a/docs/schema.md +++ b/docs/schema.md @@ -22,9 +22,11 @@ erDiagram Document ||--o{ DocumentPerson : links Document ||--o{ DocumentTag : tagged Person ||--o{ DocumentPerson : links + Person ||--o{ PersonTag : tagged Person ||--o{ Photo : owns PersonRole ||--o{ DocumentPerson : labels Tag ||--o{ DocumentTag : labels + Tag ||--o{ PersonTag : labels Job ||--o{ JobSource : includes Source ||--o{ JobSource : participates JobSource ||--o{ ExecutionAttempt : attempts @@ -110,9 +112,8 @@ erDiagram | Field | Type | Notes | | :--- | :--- | :--- | | `id` | `UUID` | PK | -| `full_name` | `str` | required | -| `display_name` | `str \| None` | optional | -| `maiden_name` | `str \| None` | optional | +| `last_name` | `str` | required | +| `given_names` | `str` | required | | `birth_date` | `date \| None` | optional | | `birth_date_raw` | `str \| None` | optional | | `birth_place` | `str \| None` | optional | @@ -164,6 +165,19 @@ Constraint: Constraint: - `UniqueConstraint(document_id, tag_id)` named `uq_document_tag` +### `PersonTag` + +| Field | Type | Notes | +| :--- | :--- | :--- | +| `id` | `UUID` | PK | +| `person_id` | `UUID` | FK -> `person.id`, indexed | +| `tag_id` | `UUID` | FK -> `tag.id`, indexed | +| `created_at` | `datetime` | default now | +| `updated_at` | `datetime` | default now, onupdate | + +Constraint: +- `UniqueConstraint(person_id, tag_id)` named `uq_person_tag` + ### `Job` | Field | Type | Notes | diff --git a/docs/ui/pages/people.md b/docs/ui/pages/people.md index c62094e..4d5ad6d 100644 --- a/docs/ui/pages/people.md +++ b/docs/ui/pages/people.md @@ -19,9 +19,9 @@ People manages reusable historical-person records. A Person may appear in many D - The title is **Archival Entities: People**. - **Create new person** opens the create route. -- The table defaults to Full Name order and supports search and column sorting. -- Columns are Full Name, FamilySearch ID, Birth Date, Death Date, and # Documents. -- Full Name is left-aligned; FamilySearch ID, date columns, and # Documents are centered. +- The table defaults to Last Name order and supports search and column sorting. +- Columns are Last Name, First & Middle, FamilySearch ID, Birth Date, Death Date, and # Documents. +- Last Name and First & Middle are left-aligned; FamilySearch ID, date columns, and # Documents are centered. - # Documents reflects how many linked Documents each Person is connected to. - Birth and death values independently prefer exact date, then approximate date, then `Unknown`. - Selecting a row opens Person Detail. @@ -31,35 +31,36 @@ People manages reusable historical-person records. A Person may appear in many D Required: -- Full name. +- Last name. +- First & middle names. Optional: -- Display name and maiden name. - Exact and approximate birth/death dates. - Birth/death places. - Biography. - FamilySearch ID. +- Tags. Rules: -- Missing Full name blocks save with a warning. +- Missing last name or first/middle names blocks save with a warning. - Exact date inputs are native browser date inputs. - FamilySearch IDs are normalized and validated by `PeopleService`. -- Photos are managed on Person Detail (not in create/edit form fields). +- Tags use the shared Tag registry and support inline add/select behavior. +- Photos are managed from Person Detail via `/people/{person_id}/photos` (not in create/edit form fields). - Metadata JSON remains hidden. - Save success returns to Person Detail. ## Detail Behavior -- The header provides **New Document**, **Edit Person**, and **Delete**. +- The header provides **New Document**, **Edit Person**, **Edit Photo(s)**, and **Delete**. - **New Document** opens Document creation with this Person requested for author preselection. - Person Detail shows a single-photo viewer with **Previous/Next** navigation; the page-level **Edit Photo(s)** header action opens photo management. - Photo management (upload, description edit, set-primary, delete) is intentionally moved to `/people/{person_id}/photos`. -- Biographical Record shows names, compact birth/death dates, and places. +- Biographical Record shows split names, computed full name, tags, compact birth/death dates, and places. - Birth and death place values are clickable links to Google Maps when present. - FamilySearch ID is shown as a metadata value and is clickable to the FamilySearch person details route when present. -- Maiden Name is only shown in Biographical Record when a value exists. - Biography has an explicit empty value. - Linked Documents render as a table with **Document Name**, **Role**, and **Number of Pages**; selecting a row opens Document Detail. - No links shows both an empty state and guidance to link from a Document workflow. @@ -83,7 +84,7 @@ Rules: ## Acceptance Checklist - List fields, alignment, date fallback, search, sorting, and navigation match this contract. -- Full name is enforced on create and edit. +- Last name and first/middle names are enforced on create and edit. - FamilySearch ID validation and link generation use the fixed supported identifier format. - Photo upload and rendering remain constrained to supported media paths. - New Document carries the Person context. diff --git a/src/transcription/db/__init__.py b/src/transcription/db/__init__.py index 05eb3f9..3ed6f25 100644 --- a/src/transcription/db/__init__.py +++ b/src/transcription/db/__init__.py @@ -1,6 +1,7 @@ from .operations import create_all from .operations import reconcile_canonical_media_paths from .operations import reconcile_legacy_job_source_columns +from .operations import reconcile_person_name_columns from .runtime import dispose_database_runtime from .runtime import initialize_database_runtime from .session import session_scope @@ -12,6 +13,7 @@ __all__ = [ "initialize_database_runtime", "reconcile_canonical_media_paths", "reconcile_legacy_job_source_columns", + "reconcile_person_name_columns", "session_scope", "transaction_scope", ] diff --git a/src/transcription/db/migration.py b/src/transcription/db/migration.py index d78a01c..32a3d98 100644 --- a/src/transcription/db/migration.py +++ b/src/transcription/db/migration.py @@ -37,6 +37,7 @@ EXPORT_TABLE_ORDER = ( "photo", "document_person", "document_tag", + "person_tag", "job", "source", "job_source", @@ -86,9 +87,11 @@ def export_bundle(*, source_db_url: str, source_upload_dir: Path, bundle_dir: Pa source_table = metadata.tables[table_name] target_table = current_metadata.tables[table_name] - export_columns = [ - column.name for column in target_table.columns if column.name in source_table.columns - ] + export_columns = [column.name for column in target_table.columns if column.name in source_table.columns] + if table_name == "person" and "full_name" in source_table.columns: + for legacy_column in ("full_name",): + if legacy_column not in export_columns: + export_columns.append(legacy_column) if table_name == "person" and "portrait_path" in source_table.columns: legacy_portrait_rows = connection.execute( select(source_table.c["id"], source_table.c["portrait_path"]).where( @@ -213,10 +216,27 @@ def _serialize_row(row: dict[str, Any], *, table_name: str, source_upload_dir: P preferred_prefix="photos/", ) continue + if table_name == "person" and key == "full_name" and isinstance(serialized_value, str): + given_names, last_name = _split_legacy_full_name(serialized_value) + serialized["given_names"] = given_names + serialized["last_name"] = last_name + continue serialized[key] = serialized_value + if table_name == "person": + serialized["given_names"] = str(serialized.get("given_names") or "").strip() + serialized["last_name"] = str(serialized.get("last_name") or "").strip() return serialized +def _split_legacy_full_name(full_name: str) -> tuple[str, str]: + tokens = [token for token in full_name.strip().split() if token] + if len(tokens) >= 2: + return (" ".join(tokens[:-1]), tokens[-1]) + if len(tokens) == 1: + return (tokens[0], tokens[0]) + return ("Unknown", "Unknown") + + def _serialize_value(key: str, value: Any) -> Any: if isinstance(value, UUID): return str(value) diff --git a/src/transcription/db/models.py b/src/transcription/db/models.py index 7a88acb..c48d7f0 100644 --- a/src/transcription/db/models.py +++ b/src/transcription/db/models.py @@ -139,6 +139,10 @@ class Tag(SQLModel, table=True): back_populates="tag_ref", sa_relationship_kwargs={"lazy": "raise"}, ) + person_tags: list["PersonTag"] = Relationship( + back_populates="tag_ref", + sa_relationship_kwargs={"lazy": "raise"}, + ) class Document(SQLModel, table=True): @@ -176,9 +180,8 @@ class Person(SQLModel, table=True): """A historical person linked to one or more documents.""" id: UUID = Field(default_factory=uuid4, primary_key=True) - full_name: str - display_name: str | None = None - maiden_name: str | None = None + last_name: str + given_names: str birth_date: date | None = None birth_date_raw: str | None = None birth_place: str | None = None @@ -200,11 +203,20 @@ class Person(SQLModel, table=True): document_people: list["DocumentPerson"] = Relationship( back_populates="person", sa_relationship_kwargs={"lazy": "raise"} ) + person_tags: list["PersonTag"] = Relationship( + back_populates="person", + sa_relationship_kwargs={"lazy": "raise"}, + ) photos: list["Photo"] = Relationship( back_populates="person", sa_relationship_kwargs={"lazy": "raise"}, ) + @property + def full_name(self) -> str: + """Presentation-friendly combined name.""" + return f"{self.given_names} {self.last_name}".strip() + class Photo(SQLModel, table=True): """A reusable image record for Person and homepage galleries.""" @@ -282,6 +294,32 @@ class DocumentTag(SQLModel, table=True): ) +class PersonTag(SQLModel, table=True): + """Associates People with Tags.""" + + __tablename__ = "person_tag" + + id: UUID = Field(default_factory=uuid4, primary_key=True) + person_id: UUID = Field(foreign_key="person.id", index=True) + tag_id: UUID = Field(foreign_key="tag.id", index=True) + created_at: datetime = Field(default_factory=lambda: datetime.now(UTC)) + updated_at: datetime = Field( + default_factory=lambda: datetime.now(UTC), + sa_column_kwargs={"onupdate": lambda: datetime.now(UTC)}, + ) + + __table_args__ = (UniqueConstraint("person_id", "tag_id", name="uq_person_tag"),) + + person: Optional["Person"] = Relationship( + back_populates="person_tags", + sa_relationship_kwargs={"lazy": "raise"}, + ) + tag_ref: Optional["Tag"] = Relationship( + back_populates="person_tags", + sa_relationship_kwargs={"lazy": "raise"}, + ) + + class Job(SQLModel, table=True): """A transcription job tied to a single document.""" diff --git a/src/transcription/db/operations.py b/src/transcription/db/operations.py index c6c27c4..9ba3b9d 100644 --- a/src/transcription/db/operations.py +++ b/src/transcription/db/operations.py @@ -107,6 +107,7 @@ async def reconcile_canonical_media_paths(*, engine: AsyncEngine | None = None) rows_changed += 1 return rows_changed + async with active_engine.begin() as connection: rows_changed = await connection.run_sync(_reconcile) if rows_changed: @@ -114,6 +115,62 @@ async def reconcile_canonical_media_paths(*, engine: AsyncEngine | None = None) return rows_changed +async def reconcile_person_name_columns(*, engine: AsyncEngine | None = None) -> int: + """Backfill V5.1 Person name columns on existing databases.""" + active_engine = engine or resolve_engine() + if not hasattr(active_engine, "begin"): + return 0 + + def _reconcile(sync_connection) -> int: + rows_changed = 0 + inspector = sqlalchemy_inspect(sync_connection) + table_names = set(inspector.get_table_names()) + if "person" not in table_names: + return 0 + present_columns = {column["name"] for column in inspector.get_columns("person")} + if "last_name" not in present_columns: + sync_connection.execute(text('alter table "person" add column "last_name" varchar')) + if "given_names" not in present_columns: + sync_connection.execute(text('alter table "person" add column "given_names" varchar')) + + query = ( + text('select id, full_name, given_names, last_name from "person"') + if "full_name" in present_columns + else text('select id, null as full_name, given_names, last_name from "person"') + ) + rows = sync_connection.execute(query).mappings().all() + for row in rows: + given_names = (str(row.get("given_names") or "")).strip() + last_name = (str(row.get("last_name") or "")).strip() + if given_names and last_name: + continue + tokens = [token for token in str(row.get("full_name") or "").split() if token] + if len(tokens) >= 2: + given_names, last_name = (" ".join(tokens[:-1]), tokens[-1]) + elif len(tokens) == 1: + given_names = tokens[0] + last_name = tokens[0] + else: + given_names = "Unknown" + last_name = "Unknown" + sync_connection.execute( + text('update "person" set given_names = :given_names, last_name = :last_name where id = :id'), + { + "id": row["id"], + "given_names": given_names, + "last_name": last_name, + }, + ) + rows_changed += 1 + return rows_changed + + async with active_engine.begin() as connection: + rows_changed = await connection.run_sync(_reconcile) + if rows_changed: + logger.warning("Backfilled V5.1 name columns for %s person row(s)", rows_changed) + return rows_changed + + def _canonical_relative_path(value: str, *, preferred_prefix: str) -> str | None: normalized = value.strip().replace("\\", "/") if not normalized: diff --git a/src/transcription/services/people.py b/src/transcription/services/people.py index 1f11421..0069808 100644 --- a/src/transcription/services/people.py +++ b/src/transcription/services/people.py @@ -23,7 +23,9 @@ from ..db.models import Document from ..db.models import DocumentPerson from ..db.models import Photo from ..db.models import Person +from ..db.models import PersonTag from ..db.models import PersonRole +from ..db.models import Tag from ..errors import AppError from ..errors import ErrorCategory from .base import ServiceBase @@ -43,6 +45,10 @@ class PersonRoleError(PeopleError): """Raised when Person Role maintenance fails.""" +class PersonTagError(PeopleError): + """Raised when Person tag maintenance fails.""" + + class PersonRoleRegistry(RegistryService[PersonRole]): """Person Role registry maintenance.""" @@ -79,6 +85,25 @@ def normalize_family_search_id(value: str | None) -> str | None: type PersonRoleSummary = RegistrySummary +class PersonTagRegistry(RegistryService[Tag]): + """Tag registry maintenance for Person tag assignment.""" + + model = Tag + error = PersonTagError + noun = "Tag" + short_noun = "tag" + referenced_retainer = "historical People" + + def reference_model(self) -> type[SQLModel]: + return PersonTag + + def reference_id_column(self) -> Any: + return col(PersonTag.id) + + def reference_key_column(self) -> Any: + return col(PersonTag.tag_id) + + @dataclass(frozen=True, slots=True) class DocumentPersonInput: """Complete desired relationship for one Person on a Document.""" @@ -97,6 +122,7 @@ class PeopleService(ServiceBase): ) -> None: super().__init__(session_factory, settings) self._person_roles = PersonRoleRegistry(self.session_factory, self.settings) + self._person_tags = PersonTagRegistry(self.session_factory, self.settings) async def create_person(self, person: Person, *, session: AsyncSession | None = None) -> Person: async with self._session_scope(session) as _session: @@ -130,7 +156,7 @@ class PeopleService(ServiceBase): existing = await _session.get( Person, person.id, - options=(selectinload(Person.document_people), selectinload(Person.photos)), + options=(selectinload(Person.document_people), selectinload(Person.person_tags), selectinload(Person.photos)), ) if existing is None: raise self._not_found(f"Person with id {person.id} not found") @@ -142,6 +168,8 @@ class PeopleService(ServiceBase): ) for link in list(existing.document_people): await _session.delete(link) + for link in list(existing.person_tags): + await _session.delete(link) await _session.delete(existing) await self._finalize(session=_session, caller_session=session) @@ -205,6 +233,7 @@ class PeopleService(ServiceBase): .selectinload(orm_attribute(DocumentPerson.document)) .selectinload(orm_attribute(Document.sources)), selectinload(Person.document_people).selectinload(orm_attribute(DocumentPerson.role_ref)), + selectinload(Person.person_tags).selectinload(orm_attribute(PersonTag.tag_ref)), selectinload(Person.photos), ) .where(Person.id == person_id) @@ -217,9 +246,62 @@ class PeopleService(ServiceBase): async def list_people(self, *, session: AsyncSession | None = None) -> Sequence[Person]: async with self._session_scope(session) as _session: - query = select(Person).options(selectinload(Person.document_people)) + query = select(Person).options( + selectinload(Person.document_people), + selectinload(Person.person_tags).selectinload(orm_attribute(PersonTag.tag_ref)), + ) return (await _session.exec(query)).all() + async def sync_person_tags_by_labels( + self, + *, + person_id: UUID, + labels: Sequence[str], + session: AsyncSession | None = None, + ) -> None: + """Replace a Person's tag set using label-based assignment.""" + normalized_labels = [self._person_tags.normalize_label(label) for label in labels] + deduplicated_labels = list(dict.fromkeys(normalized_labels)) + label_keys = [self._person_tags.label_key(label) for label in deduplicated_labels] + + async with self._session_scope(session) as _session: + existing_person = await _session.get(Person, person_id) + if existing_person is None: + raise PeopleError( + f"Person with id {person_id} not found", + category=ErrorCategory.NOT_FOUND, + suggestion="Refresh and select an existing person.", + ) + + existing_tags = ( + (await _session.exec(select(Tag).where(col(Tag.normalized_label).in_(label_keys)))) + .all() + if label_keys + else [] + ) + tags_by_key = {tag.normalized_label: tag for tag in existing_tags} + selected_tag_ids: set[UUID] = set() + + for label in deduplicated_labels: + key = self._person_tags.label_key(label) + tag = tags_by_key.get(key) + if tag is None: + tag = await self._person_tags.create_entry(label=label, is_active=True, session=_session) + tags_by_key[key] = tag + selected_tag_ids.add(tag.id) + + links = (await _session.exec(select(PersonTag).where(PersonTag.person_id == person_id))).all() + existing_ids = {link.tag_id for link in links} + + for link in links: + if link.tag_id not in selected_tag_ids: + await _session.delete(link) + + for tag_id in selected_tag_ids - existing_ids: + _session.add(PersonTag(person_id=person_id, tag_id=tag_id)) + + await self._finalize(session=_session, caller_session=session) + async def list_person_roles( self, *, diff --git a/src/transcription/services/workflows.py b/src/transcription/services/workflows.py index bc2ace1..67fde77 100644 --- a/src/transcription/services/workflows.py +++ b/src/transcription/services/workflows.py @@ -14,6 +14,7 @@ from ..db.models import JobPurpose from ..db.models import JobSource from ..db.models import JobSourceStatus from ..db.models import JobStatus +from ..db.models import Person from ..db.models import Source from ..db.session import transaction_scope from ..errors import AppError @@ -72,6 +73,32 @@ async def update_document_with_people( return updated +async def create_person_with_tags( + *, + person: Person, + tag_labels: list[str], + people: PeopleService, +) -> Person: + """Create a Person and its complete tag set atomically.""" + async with transaction_scope(session_factory=people.session_factory) as session: + created = await people.create_person(person, session=session) + await people.sync_person_tags_by_labels(person_id=created.id, labels=tag_labels, session=session) + return created + + +async def update_person_with_tags( + *, + person: Person, + tag_labels: list[str], + people: PeopleService, +) -> Person: + """Update a Person and its complete tag set atomically.""" + async with transaction_scope(session_factory=people.session_factory) as session: + updated = await people.update_person(person, session=session) + await people.sync_person_tags_by_labels(person_id=updated.id, labels=tag_labels, session=session) + return updated + + async def create_source_retranscription_job( *, source_id, diff --git a/src/transcription/ui/components/formatters.py b/src/transcription/ui/components/formatters.py index da0a940..8b544bf 100644 --- a/src/transcription/ui/components/formatters.py +++ b/src/transcription/ui/components/formatters.py @@ -45,11 +45,7 @@ def compact_date(exact: date | None, approximate: str | None) -> str: def person_selector_label(person: Person) -> str: """Build a readable selector label without treating names as identity.""" - preferred = (person.display_name or "").strip() - full_name = person.full_name.strip() - label = preferred if not preferred or preferred == full_name else f"{preferred} - {full_name}" - if not label: - label = full_name + label = person.full_name.strip() if person.birth_date is not None: return f"{label} ({person.birth_date.year})" approximate_year = YEAR_PATTERN.search(person.birth_date_raw or "") diff --git a/src/transcription/ui/components/table/people.py b/src/transcription/ui/components/table/people.py index 9f8dfdb..f2fc34d 100644 --- a/src/transcription/ui/components/table/people.py +++ b/src/transcription/ui/components/table/people.py @@ -19,7 +19,8 @@ class PersonTableRow: """Read model consumed by the people table component.""" id: UUID - full_name: str + last_name: str + given_names: str family_search_id: str birth_date: str death_date: str @@ -30,7 +31,8 @@ def _serialize_rows(rows: Sequence[PersonTableRow]) -> list[dict[str, Any]]: return [ { "id": str(row.id), - "full_name": row.full_name, + "last_name": row.last_name, + "given_names": row.given_names, "family_search_id": row.family_search_id or "Not set", "birth_date": row.birth_date or "Unknown", "death_date": row.death_date or "Unknown", @@ -51,9 +53,16 @@ def render_people_table(rows: Sequence[PersonTableRow]) -> None: rows=_serialize_rows(rows), columns=[ { - "name": "full_name", - "label": "Full Name", - "field": "full_name", + "name": "last_name", + "label": "Last Name", + "field": "last_name", + "sortable": True, + "classes": "font-serif font-semibold text-left ui-table-cell-wrap", + }, + { + "name": "given_names", + "label": "First & Middle", + "field": "given_names", "sortable": True, "classes": "font-serif font-semibold text-left ui-table-cell-wrap", }, @@ -83,14 +92,14 @@ def render_people_table(rows: Sequence[PersonTableRow]) -> None: "classes": "font-mono", }, ], - default_sort_by="full_name", - search_placeholder="Search people by name, FamilySearch ID, or dates...", + default_sort_by="last_name", + search_placeholder="Search people by last name, given names, FamilySearch ID, or dates...", on_row_click_id=lambda person_id: ui.navigate.to(f"/people/{person_id}"), ) # Custom column template adding an archival entity icon next to person's name table.add_slot( - "body-cell-full_name", + "body-cell-last_name", r"""
diff --git a/src/transcription/ui/pages/people_page.py b/src/transcription/ui/pages/people_page.py index f5215cf..1099d69 100644 --- a/src/transcription/ui/pages/people_page.py +++ b/src/transcription/ui/pages/people_page.py @@ -12,11 +12,14 @@ from nicegui import ui from transcription.config import Settings from transcription.db.models import Person +from transcription.services.documents import DocumentService from transcription.errors import ErrorCategory from transcription.services.people import PeopleError from transcription.services.people import PeopleService from transcription.services.photos import PhotoError from transcription.services.photos import PhotosService +from transcription.services.workflows import create_person_with_tags +from transcription.services.workflows import update_person_with_tags from transcription.ui.components.app_shell import render_navigation_header from transcription.ui.components.cards import archival_card from transcription.ui.components.confirm_delete import render_delete_actions @@ -49,9 +52,8 @@ from ...db.session import SessionFactoryDep class PersonFormFields: """Bound input widgets for the Person create and edit forms.""" - full_name: ui.input - display_name: ui.input - maiden_name: ui.input + last_name: ui.input + given_names: ui.input birth_date: ui.input birth_date_raw: ui.input birth_place: ui.input @@ -60,6 +62,7 @@ class PersonFormFields: death_place: ui.input biography: ui.textarea family_search_id: ui.input + tags: ui.select def register_page() -> None: # noqa: PLR0915 @@ -95,7 +98,8 @@ def register_page() -> None: # noqa: PLR0915 rows = [ PersonTableRow( id=person.id, - full_name=person.full_name, + last_name=person.last_name, + given_names=person.given_names, family_search_id=person.family_search_id or "", birth_date=compact_date(person.birth_date, person.birth_date_raw), death_date=compact_date(person.death_date, person.death_date_raw), @@ -108,19 +112,23 @@ def register_page() -> None: # noqa: PLR0915 @ui.page("/people/new") async def person_create_page(request: Request, session_factory: SessionFactoryDep) -> None: people_service = PeopleService(session_factory=session_factory) + document_service = DocumentService(session_factory=session_factory) render_navigation_header(current_path="/people") draft_person_id = uuid4() with ui.column().classes("w-full max-w-4xl mx-auto p-4 gap-4"): - page_header("Create Person Record", subtitle="Full name is required.") + page_header("Create Person Record", subtitle="Last name and first/middle names are required.") + tag_catalog = await document_service.list_tags(active_only=True) form = _render_person_form_fields( + tag_options=[tag.label for tag in tag_catalog], ) async def submit_create() -> None: - full_name = (form.full_name.value or "").strip() - if not full_name: - ui.notify("Full name is required.", type="warning") + last_name = (form.last_name.value or "").strip() + given_names = (form.given_names.value or "").strip() + if not last_name or not given_names: + ui.notify("Last name and first/middle names are required.", type="warning") return birth_date = parse_iso_date(form.birth_date.value) @@ -128,9 +136,8 @@ def register_page() -> None: # noqa: PLR0915 candidate = Person( id=draft_person_id, - full_name=full_name, - display_name=(form.display_name.value or "").strip() or None, - maiden_name=(form.maiden_name.value or "").strip() or None, + last_name=last_name, + given_names=given_names, birth_date=birth_date, birth_date_raw=(form.birth_date_raw.value or "").strip() or None, birth_place=(form.birth_place.value or "").strip() or None, @@ -144,7 +151,11 @@ def register_page() -> None: # noqa: PLR0915 create_outcome = await run_ui_action( operation="people.create", title="Create failed", - action=lambda: people_service.create_person(candidate), + action=lambda: create_person_with_tags( + person=candidate, + tag_labels=_resolve_selected_tag_labels(form.tags.value), + people=people_service, + ), ) if not create_outcome.ok or create_outcome.value is None: return @@ -350,6 +361,7 @@ def register_page() -> None: # noqa: PLR0915 @ui.page("/people/{person_id}/edit") async def person_edit_page(person_id: str, request: Request, session_factory: SessionFactoryDep) -> None: people_service = PeopleService(session_factory=session_factory) + document_service = DocumentService(session_factory=session_factory) render_navigation_header(current_path="/people") parsed_person_id = parsed_record_id(person_id, noun="Person") @@ -366,16 +378,19 @@ def register_page() -> None: # noqa: PLR0915 return with ui.column().classes("w-full max-w-4xl mx-auto p-4 gap-4"): - page_header("Edit Person Record", subtitle="Full name is required.") + page_header("Edit Person Record", subtitle="Last name and first/middle names are required.") + tag_catalog = await document_service.list_tags(active_only=False) form = _render_person_form_fields( person=person, + tag_options=[tag.label for tag in tag_catalog], ) async def submit_edit() -> None: - full_name = (form.full_name.value or "").strip() - if not full_name: - ui.notify("Full name is required.", type="warning") + last_name = (form.last_name.value or "").strip() + given_names = (form.given_names.value or "").strip() + if not last_name or not given_names: + ui.notify("Last name and first/middle names are required.", type="warning") return birth_date = parse_iso_date(form.birth_date.value) @@ -383,9 +398,8 @@ def register_page() -> None: # noqa: PLR0915 candidate = Person( id=person.id, - full_name=full_name, - display_name=(form.display_name.value or "").strip() or None, - maiden_name=(form.maiden_name.value or "").strip() or None, + last_name=last_name, + given_names=given_names, birth_date=birth_date, birth_date_raw=(form.birth_date_raw.value or "").strip() or None, birth_place=(form.birth_place.value or "").strip() or None, @@ -402,7 +416,11 @@ def register_page() -> None: # noqa: PLR0915 save_outcome = await run_ui_action( operation="people.edit.save", title="Save failed", - action=lambda: people_service.update_person(candidate), + action=lambda: update_person_with_tags( + person=candidate, + tag_labels=_resolve_selected_tag_labels(form.tags.value), + people=people_service, + ), ) if not save_outcome.ok: return @@ -479,21 +497,17 @@ def register_page() -> None: # noqa: PLR0915 def _render_person_form_fields( *, person: Person | None = None, + tag_options: list[str], ) -> PersonFormFields: with archival_card(extra_classes="gap-3"): - with ui.row().classes("w-full gap-3 grid grid-cols-1 md:grid-cols-3"): - full_name_input = ( - ui.input(label="Full name", value=person.full_name if person else "") + with ui.row().classes("w-full gap-3 grid grid-cols-1 md:grid-cols-2"): + last_name_input = ( + ui.input(label="Last name", value=person.last_name if person else "") .props("outlined") .classes("ui-form-surface") ) - display_name_input = ( - ui.input(label="Display name", value=person.display_name if person and person.display_name else "") - .props("outlined") - .classes("ui-form-surface") - ) - maiden_name_input = ( - ui.input(label="Maiden name", value=person.maiden_name if person and person.maiden_name else "") + given_names_input = ( + ui.input(label="First & middle", value=person.given_names if person else "") .props("outlined") .classes("ui-form-surface") ) @@ -558,11 +572,30 @@ def _render_person_form_fields( .props("outlined") .classes("w-full ui-form-surface") ) + selected_tags = ( + sorted( + [ + link.tag_ref.label + for link in (person.person_tags if person is not None else []) + if link.tag_ref is not None + ], + key=str.casefold, + ) + if person is not None + else [] + ) + tags_input = ui.select( + sorted(tag_options, key=str.casefold), + label="Tags", + value=selected_tags, + multiple=True, + with_input=True, + new_value_mode="add-unique", + ).props("outlined use-chips").classes("w-full ui-form-surface") return PersonFormFields( - full_name=full_name_input, - display_name=display_name_input, - maiden_name=maiden_name_input, + last_name=last_name_input, + given_names=given_names_input, birth_date=birth_date_input, birth_date_raw=birth_date_raw_input, birth_place=birth_place_input, @@ -571,6 +604,7 @@ def _render_person_form_fields( death_place=death_place_input, biography=biography_input, family_search_id=family_search_id_input, + tags=tags_input, ) @@ -653,10 +687,9 @@ def _render_photo_viewer_with_navigation( def _render_person_biographical_zone(person: Person) -> None: with ui.column().classes("col-span-12 lg:col-span-4 gap-4"): with archival_card(title="Biographical Record"): + metadata_row("Last Name:", person.last_name) + metadata_row("First & Middle:", person.given_names) metadata_row("Full Name:", person.full_name) - metadata_row("Display Name:", person.display_name or "Not set") - if person.maiden_name: - metadata_row("Maiden Name:", person.maiden_name) metadata_row("Birth Date:", compact_date(person.birth_date, person.birth_date_raw)) if person.birth_place: metadata_link_row( @@ -681,6 +714,11 @@ def _render_person_biographical_zone(person: Person) -> None: person.family_search_id, family_search_url(person.family_search_id), ) + tags = sorted( + [link.tag_ref.label for link in person.person_tags if link.tag_ref is not None], + key=str.casefold, + ) + metadata_row("Tags:", ", ".join(tags) if tags else "Not set") with archival_card(title="System Logistics"): ui.label(f"Created: {person.created_at.isoformat()}").classes("text-[11px] ui-text-muted") @@ -763,3 +801,26 @@ def _render_linked_documents(person: Person) -> None: # --- Utilities --- + + +def _resolve_selected_tag_labels(value: object) -> list[str]: + def flatten(item: object) -> list[str]: + if item is None: + return [] + if isinstance(item, str): + return [item] + if isinstance(item, dict): + if "value" in item: + return flatten(item.get("value")) + if "label" in item: + return flatten(item.get("label")) + return [] + if isinstance(item, (list, tuple, set)): + values: list[str] = [] + for child in item: + values.extend(flatten(child)) + return values + return [str(item)] + + labels = [candidate.strip() for candidate in flatten(value) if candidate and candidate.strip()] + return list(dict.fromkeys(labels)) diff --git a/tests/api/test_v4_documents.py b/tests/api/test_v4_documents.py index 18155b7..5834bfd 100644 --- a/tests/api/test_v4_documents.py +++ b/tests/api/test_v4_documents.py @@ -37,7 +37,10 @@ def _seed_document_and_person( async def _seed() -> tuple[UUID, UUID]: async with session_scope(database_url=db_url) as session: document = Document(name=document_name) - person = Person(full_name=person_name) + tokens = [token for token in person_name.split() if token] + given_names = " ".join(tokens[:-1]) if len(tokens) >= 2 else person_name + last_name = tokens[-1] if len(tokens) >= 2 else person_name + person = Person(given_names=given_names, last_name=last_name) session.add(document) session.add(person) await session.commit() diff --git a/tests/services/test_document_service.py b/tests/services/test_document_service.py index dda6ac2..52d3886 100644 --- a/tests/services/test_document_service.py +++ b/tests/services/test_document_service.py @@ -13,6 +13,7 @@ from transcription.db.models import DocumentPerson from transcription.db.models import DocumentTag from transcription.db.models import Job from transcription.db.models import Person +from transcription.db.models import PersonTag from transcription.db.models import Source from transcription.db.models import Tag from transcription.db.models import Photo @@ -132,7 +133,7 @@ async def test_delete_document_removes_person_links(default_session_factory, tmp name="person-linked-delete", ) ) - person = await people_service.create_person(Person(full_name="Linked Person")) + person = await people_service.create_person(Person(given_names="Linked", last_name="Person")) author_role = await people_service.create_person_role(label="Author") await people_service.create_document_person( DocumentPerson( @@ -197,7 +198,7 @@ async def test_read_person_detail_loads_document_links(default_session_factory): name="linked-doc", ) ) - person = await people_service.create_person(Person(full_name="Linked Person")) + person = await people_service.create_person(Person(given_names="Linked", last_name="Person")) author_role = await people_service.create_person_role(label="Author") await people_service.create_document_person( DocumentPerson( @@ -221,16 +222,17 @@ async def test_update_person_refreshes_updated_timestamp(default_session_factory created = await service.create_person( Person( - full_name="timestamp-person", + given_names="timestamp", + last_name="person", updated_at=datetime(2000, 1, 1, tzinfo=UTC), ) ) original_updated_at = created.updated_at - created.display_name = "updated" + created.given_names = "updated" updated = await service.update_person(created) - assert updated.display_name == "updated" + assert updated.given_names == "updated" assert updated.updated_at >= original_updated_at @@ -245,7 +247,7 @@ async def test_delete_person_removes_links_when_linked_documents_exist(default_s name="block-person-delete-doc", ) ) - person = await service.create_person(Person(full_name="Blocked Person")) + person = await service.create_person(Person(given_names="Blocked", last_name="Person")) author_role = await service.create_person_role(label="Author") await service.create_document_person( DocumentPerson( @@ -268,7 +270,7 @@ async def test_delete_person_removes_links_when_linked_documents_exist(default_s async def test_delete_person_succeeds_when_unlinked(default_session_factory): service = PeopleService(session_factory=default_session_factory) - person = await service.create_person(Person(full_name="Free Person")) + person = await service.create_person(Person(given_names="Free", last_name="Person")) await service.delete_person(person) @@ -280,7 +282,7 @@ async def test_delete_person_succeeds_when_unlinked(default_session_factory): async def test_delete_person_blocks_when_photos_exist(default_session_factory): service = PeopleService(session_factory=default_session_factory) - person = await service.create_person(Person(full_name="Photo Protected Person")) + person = await service.create_person(Person(given_names="Photo Protected", last_name="Person")) async with service._session_scope() as session: session.add( Photo( @@ -313,7 +315,7 @@ async def test_update_document_person_changes_role_id(default_session_factory): service = PeopleService(session_factory=default_session_factory) document = await documents_service.create_document(Document(id=uuid4(), name="role-sync-doc")) - person = await service.create_person(Person(full_name="Role Sync Person")) + person = await service.create_person(Person(given_names="Role Sync", last_name="Person")) author_role = await service.create_person_role(label="Author") recipient_role = await service.create_person_role(label="Recipient") link = await service.create_document_person( @@ -357,3 +359,25 @@ async def test_sync_document_tags_by_labels_creates_and_replaces_tags(default_se assert len(listed) == 1 listed_labels = {link.tag_ref.label for link in listed[0].document_tags if link.tag_ref is not None} assert listed_labels == {"Census", "Research"} + + +@pytest.mark.asyncio +async def test_sync_person_tags_by_labels_creates_and_replaces_tags(default_session_factory): + people = PeopleService(session_factory=default_session_factory) + person = await people.create_person(Person(given_names="Tagged", last_name="Person")) + + await people.sync_person_tags_by_labels(person_id=person.id, labels=["Family", "Census"]) + await people.sync_person_tags_by_labels(person_id=person.id, labels=["Census", "Research"]) + + async with people._session_scope() as session: + links = (await session.exec(select(PersonTag).where(PersonTag.person_id == person.id))).all() + tags = (await session.exec(select(Tag))).all() + + assert len(links) == 2 + linked_ids = {link.tag_id for link in links} + linked_labels = {tag.label for tag in tags if tag.id in linked_ids} + assert linked_labels == {"Census", "Research"} + + detail = await people.read_person_detail(person.id) + listed_labels = {link.tag_ref.label for link in detail.person_tags if link.tag_ref is not None} + assert listed_labels == {"Census", "Research"} diff --git a/tests/services/test_photo_service.py b/tests/services/test_photo_service.py index 534ca07..7ad5e33 100644 --- a/tests/services/test_photo_service.py +++ b/tests/services/test_photo_service.py @@ -18,7 +18,7 @@ async def test_create_photo_persists_media_and_primary_state(default_session_fac settings = Settings(openrouter_api_key="test-key", upload_dir=tmp_path / "uploads") people = PeopleService(session_factory=default_session_factory) photos = PhotosService(session_factory=default_session_factory, settings=settings) - person = await people.create_person(Person(full_name="Photo Person")) + person = await people.create_person(Person(given_names="Photo", last_name="Person")) first = await photos.create_photo(person_id=person.id, filename="one.png", file_bytes=PNG_BYTES) second = await photos.create_photo(person_id=person.id, filename="two.png", file_bytes=PNG_BYTES) @@ -36,7 +36,7 @@ async def test_set_primary_and_delete_promotes_next_photo(default_session_factor settings = Settings(openrouter_api_key="test-key", upload_dir=tmp_path / "uploads") people = PeopleService(session_factory=default_session_factory) photos = PhotosService(session_factory=default_session_factory, settings=settings) - person = await people.create_person(Person(full_name="Primary Person")) + person = await people.create_person(Person(given_names="Primary", last_name="Person")) first = await photos.create_photo(person_id=person.id, filename="one.png", file_bytes=PNG_BYTES) second = await photos.create_photo(person_id=person.id, filename="two.png", file_bytes=PNG_BYTES) diff --git a/tests/services/test_settings_services.py b/tests/services/test_settings_services.py index 3ceff41..0554e92 100644 --- a/tests/services/test_settings_services.py +++ b/tests/services/test_settings_services.py @@ -95,7 +95,7 @@ async def test_person_role_delete_allows_unreferenced_and_blocks_referenced(defa unused = await people.create_person_role(label="Witness") referenced = await people.create_person_role(label="Creator") document = await documents.create_document(Document(name="Role document")) - person = await people.create_person(Person(full_name="Role Person")) + person = await people.create_person(Person(given_names="Role", last_name="Person")) await people.create_document_person( DocumentPerson( document_id=document.id, @@ -131,7 +131,7 @@ async def test_custom_person_role_can_be_used_for_document_link(default_session_ people = PeopleService(session_factory=default_session_factory) role = await people.create_person_role(label="Witness") document = await documents.create_document(Document(name="Witnessed document")) - person = await people.create_person(Person(full_name="Archive Witness")) + person = await people.create_person(Person(given_names="Archive", last_name="Witness")) link = await people.add_document_person_link( document_id=document.id, diff --git a/tests/services/test_timestamps.py b/tests/services/test_timestamps.py index fc5d6fc..3844044 100644 --- a/tests/services/test_timestamps.py +++ b/tests/services/test_timestamps.py @@ -34,10 +34,11 @@ async def test_document_update_advances_updated_at(default_session_factory): @pytest.mark.asyncio async def test_person_update_advances_updated_at(default_session_factory): people = PeopleService(session_factory=default_session_factory) - person = await people.create_person(Person(full_name="Grace Hopper")) + person = await people.create_person(Person(given_names="Grace", last_name="Hopper")) original = person.updated_at - person.full_name = "Rear Adm. Grace Hopper" + person.given_names = "Rear Adm. Grace" + person.last_name = "Hopper" updated = await people.update_person(person) assert updated.updated_at > original diff --git a/tests/services/test_v2_crud.py b/tests/services/test_v2_crud.py index 63fdd56..e878814 100644 --- a/tests/services/test_v2_crud.py +++ b/tests/services/test_v2_crud.py @@ -28,7 +28,7 @@ async def test_people_service_handles_person_and_document_person_crud(default_se people_service = PeopleService(session_factory=default_session_factory) document = await documents.create_document(Document(id=uuid4(), name="person-doc")) - person = await people_service.create_person(Person(full_name="Ada Lovelace")) + person = await people_service.create_person(Person(given_names="Ada", last_name="Lovelace")) author_role = await people_service.create_person_role(label="Author") recipient_role = await people_service.create_person_role(label="Recipient") @@ -61,15 +61,15 @@ async def test_people_service_handles_person_and_document_person_crud(default_se async def test_people_service_normalizes_and_rejects_duplicate_family_search_ids(default_session_factory): people_service = PeopleService(session_factory=default_session_factory) - created = await people_service.create_person(Person(full_name="Hig Higgins", family_search_id=" g8t4-mdq ")) + created = await people_service.create_person(Person(given_names="Hig", last_name="Higgins", family_search_id=" g8t4-mdq ")) assert created.family_search_id == "G8T4-MDQ" with pytest.raises(PeopleError) as duplicate: - await people_service.create_person(Person(full_name="Duplicate Hig", family_search_id="G8T4-MDQ")) + await people_service.create_person(Person(given_names="Duplicate", last_name="Hig", family_search_id="G8T4-MDQ")) assert duplicate.value.category == ErrorCategory.CONFLICT with pytest.raises(PeopleError) as malformed: - await people_service.create_person(Person(full_name="Malformed", family_search_id="not-an-id")) + await people_service.create_person(Person(given_names="Malformed", last_name="Person", family_search_id="not-an-id")) assert malformed.value.category == ErrorCategory.VALIDATION @@ -228,7 +228,7 @@ async def test_document_detail_loads_linked_person_relationship(default_session_ people_service = PeopleService(session_factory=default_session_factory) document = await documents.create_document(Document(id=uuid4(), name="detail-person-doc")) - person = await people_service.create_person(Person(full_name="Grace Hopper")) + person = await people_service.create_person(Person(given_names="Grace", last_name="Hopper")) author_role = await people_service.create_person_role(label="Author") await people_service.create_document_person( DocumentPerson( diff --git a/tests/services/test_v44_workflows.py b/tests/services/test_v44_workflows.py index 9e2c705..5cefa7c 100644 --- a/tests/services/test_v44_workflows.py +++ b/tests/services/test_v44_workflows.py @@ -45,7 +45,7 @@ async def test_update_document_with_people_rolls_back_document_and_links(default people = PeopleService(session_factory=default_session_factory) role = await people.create_person_role(label="Witness") inactive = await people.create_person_role(label="Former Witness", is_active=False) - person = await people.create_person(Person(full_name="Archive Witness")) + person = await people.create_person(Person(given_names="Archive", last_name="Witness")) document = await create_document_with_people( document=Document(name="Original name"), links=[DocumentPersonInput(person_id=person.id, role_id=role.id)], @@ -83,7 +83,7 @@ async def test_direct_link_writes_reject_new_inactive_role_assignments(default_s people = PeopleService(session_factory=default_session_factory) active = await people.create_person_role(label="Witness") inactive = await people.create_person_role(label="Former Witness", is_active=False) - person = await people.create_person(Person(full_name="Archive Witness")) + person = await people.create_person(Person(given_names="Archive", last_name="Witness")) document = await documents.create_document(Document(name="Role rules")) link = await people.add_document_person_link( document_id=document.id, @@ -114,7 +114,7 @@ async def test_document_print_projection_uses_semantic_author_and_current_text(d document = await documents.create_document( Document(name="Print Me", notes="Archive note", document_type_id=document_type.id) ) - person = await people.create_person(Person(full_name="Historic Author")) + person = await people.create_person(Person(given_names="Historic", last_name="Author")) async with people._session_scope() as session: author = PersonRole( diff --git a/tests/test_db.py b/tests/test_db.py index 8072ff8..8dd3474 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -20,6 +20,7 @@ from transcription.db import dispose_database_runtime from transcription.db import initialize_database_runtime from transcription.db import reconcile_canonical_media_paths from transcription.db import reconcile_legacy_job_source_columns +from transcription.db import reconcile_person_name_columns from transcription.db import session_scope from transcription.db.models import DocumentType from transcription.db.models import PersonRole @@ -55,6 +56,7 @@ async def test_create_all_creates_expected_tables(tmp_path): assert "person_role" in table_names assert "document_person" in table_names assert "document_tag" in table_names + assert "person_tag" in table_names assert "job" in table_names assert "source" in table_names assert "job_source" in table_names @@ -141,7 +143,7 @@ async def test_create_all_declares_hot_path_indexes(tmp_path): database = inspect(sync_connection) indexes = { table: [index["column_names"] for index in database.get_indexes(table)] - for table in ("job", "source", "job_source", "document", "document_person", "document_tag") + for table in ("job", "source", "job_source", "document", "document_person", "document_tag", "person_tag") } job_source_unique = [ constraint["column_names"] @@ -151,9 +153,13 @@ async def test_create_all_declares_hot_path_indexes(tmp_path): constraint["column_names"] for constraint in database.get_unique_constraints("document_tag") ] - return indexes, job_source_unique, document_tag_unique + person_tag_unique = [ + constraint["column_names"] + for constraint in database.get_unique_constraints("person_tag") + ] + return indexes, job_source_unique, document_tag_unique, person_tag_unique - indexes, job_source_unique, document_tag_unique = await connection.run_sync(collect) + indexes, job_source_unique, document_tag_unique, person_tag_unique = await connection.run_sync(collect) assert ["status", "date_created"] in indexes["job"] assert ["document_id"] in indexes["job"] @@ -168,6 +174,46 @@ async def test_create_all_declares_hot_path_indexes(tmp_path): assert ["document_id"] in indexes["document_tag"] assert ["tag_id"] in indexes["document_tag"] assert ["document_id", "tag_id"] in document_tag_unique + assert ["person_id"] in indexes["person_tag"] + assert ["tag_id"] in indexes["person_tag"] + assert ["person_id", "tag_id"] in person_tag_unique + finally: + await dispose_database_runtime() + + +@pytest.mark.asyncio +async def test_reconcile_person_name_columns_backfills_split_names(tmp_path): + settings = Settings( + openrouter_api_key="test-key", + database=SqliteSettings(path=str(tmp_path / "legacy-person-name.db")), + environment="test", + ) + runtime = initialize_database_runtime(settings=settings) + + try: + await create_all(engine=runtime.engine) + async with runtime.engine.begin() as connection: + await connection.execute(text('alter table "person" add column "full_name" varchar')) + await connection.execute( + text( + 'insert into "person" (id, full_name, given_names, last_name, created_at, updated_at) ' + "values (:id, :full_name, '', '', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" + ), + {"id": "55" * 16, "full_name": "Ada Lovelace"}, + ) + + changed = await reconcile_person_name_columns(engine=runtime.engine) + assert changed >= 1 + + async with runtime.engine.connect() as connection: + row = ( + await connection.execute( + text('select given_names, last_name from "person" where id = :id'), + {"id": "55" * 16}, + ) + ).one() + assert row[0] == "Ada" + assert row[1] == "Lovelace" finally: await dispose_database_runtime() @@ -227,10 +273,10 @@ async def test_reconcile_canonical_media_paths_normalizes_source_and_photo_paths async with runtime.engine.begin() as connection: await connection.execute( text( - 'insert into "person" (id, full_name, created_at, updated_at) ' - 'values (:id, :full_name, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)' + 'insert into "person" (id, given_names, last_name, created_at, updated_at) ' + 'values (:id, :given_names, :last_name, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)' ), - {"id": "11" * 16, "full_name": "Portrait"}, + {"id": "11" * 16, "given_names": "Portrait", "last_name": "Person"}, ) await connection.execute( text( diff --git a/tests/test_models.py b/tests/test_models.py index ffe3fd6..f8a4297 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -53,7 +53,7 @@ def _persist_document(session) -> Document: def _persist_person(session, **overrides: Any) -> Person: - defaults: dict[str, Any] = {"full_name": "Ada Lovelace"} + defaults: dict[str, Any] = {"given_names": "Ada", "last_name": "Lovelace"} defaults.update(overrides) person = Person(**defaults) session.add(person) @@ -195,10 +195,10 @@ class TestSourceModel: class TestPersonAndDocumentPersonModel: def test_family_search_id_is_unique_when_present(self, session): - session.add(Person(full_name="First Person", family_search_id="G8T4-MDQ")) + session.add(Person(given_names="First", last_name="Person", family_search_id="G8T4-MDQ")) session.commit() - session.add(Person(full_name="Second Person", family_search_id="G8T4-MDQ")) + session.add(Person(given_names="Second", last_name="Person", family_search_id="G8T4-MDQ")) with pytest.raises(IntegrityError): session.commit() diff --git a/tests/tools/test_export_import_migration.py b/tests/tools/test_export_import_migration.py index 4989f02..a51a57e 100644 --- a/tests/tools/test_export_import_migration.py +++ b/tests/tools/test_export_import_migration.py @@ -168,9 +168,15 @@ def test_export_import_migration_backfills_legacy_portraits_and_homepage_images( target_engine = create_engine(target_db_url) try: with target_engine.connect() as connection: + person_name = connection.execute( + text('select given_names, last_name from "person" where id = :id'), + {"id": person_id}, + ).one() photos = connection.execute( text('select person_id, path, is_primary from "photo" order by person_id is not null desc, created_at asc') ).all() + assert person_name[0] == "Legacy" + assert person_name[1] == "Portrait" assert len(photos) == 2 person_photo = next(row for row in photos if row[0] is not None) homepage_photo = next(row for row in photos if row[0] is None) diff --git a/tests/ui/test_documents_page.py b/tests/ui/test_documents_page.py index 9f04d11..bea00ce 100644 --- a/tests/ui/test_documents_page.py +++ b/tests/ui/test_documents_page.py @@ -28,7 +28,7 @@ async def seed_person_and_document(): async with session_scope() as session: letter_type = (await session.exec(select(DocumentType).where(DocumentType.label == "Letter"))).one() author_role = (await session.exec(select(PersonRole).where(PersonRole.semantic_key == "author"))).one() - person = Person(full_name="Zenna Cochran") + person = Person(given_names="Zenna", last_name="Cochran") session.add(person) await session.flush() @@ -122,8 +122,8 @@ class TestDocumentsPageRendering: _, client = app_client async with session_scope() as session: person = Person( - full_name="Albert Edward Higgins", - display_name="Hig", + given_names="Albert Edward", + last_name="Higgins", birth_date=date(1885, 1, 2), ) session.add(person) @@ -133,7 +133,7 @@ class TestDocumentsPageRendering: response = client.get(f"/ui/documents/new?person_id={person_id}") assert response.status_code == 200 - assert "Hig - Albert Edward Higgins (1885)" in response.text + assert "Albert Edward Higgins (1885)" in response.text @pytest.mark.asyncio async def test_document_detail_page_renders_bento_grid_and_metadata(self, app_client, seed_person_and_document): diff --git a/tests/ui/test_formatters.py b/tests/ui/test_formatters.py index f0658f4..b553bb5 100644 --- a/tests/ui/test_formatters.py +++ b/tests/ui/test_formatters.py @@ -15,18 +15,18 @@ def test_compact_date_prefers_exact_then_approximate_then_unknown(): def test_person_selector_label_disambiguates_without_changing_identity(): person = Person( - full_name="Albert Edward Higgins", - display_name="Hig", + given_names="Albert Edward", + last_name="Higgins", birth_date=date(1885, 1, 2), ) - assert person_selector_label(person) == "Hig - Albert Edward Higgins (1885)" + assert person_selector_label(person) == "Albert Edward Higgins (1885)" approximate = Person( - full_name="Albert Edward Higgins", - display_name="Hig", + given_names="Albert Edward", + last_name="Higgins", birth_date_raw="about 1912", ) - assert person_selector_label(approximate) == "Hig - Albert Edward Higgins (1912)" + assert person_selector_label(approximate) == "Albert Edward Higgins (1912)" def test_family_search_url_uses_fixed_person_details_route(): diff --git a/tests/ui/test_people_page.py b/tests/ui/test_people_page.py index 516ebb0..97e6e8f 100644 --- a/tests/ui/test_people_page.py +++ b/tests/ui/test_people_page.py @@ -35,17 +35,18 @@ class TestPeoplePageRendering: _, client = app_client async with session_scope() as session: - session.add(Person(full_name="Ada Lovelace", display_name="Ada")) + session.add(Person(given_names="Ada", last_name="Lovelace")) await session.commit() response = client.get("/ui/people") assert response.status_code == 200 - assert "Ada Lovelace" in response.text + assert "Lovelace" in response.text + assert "Ada" in response.text + assert "Last Name" in response.text + assert "First & Middle" in response.text or "First & Middle" in response.text assert "FamilySearch ID" in response.text assert "# Documents" in response.text - assert "Display Name" not in response.text - assert "Maiden Name" not in response.text @pytest.mark.asyncio async def test_people_page_shows_document_counts(self, app_client): @@ -53,7 +54,7 @@ class TestPeoplePageRendering: async with session_scope() as session: role = (await session.exec(select(PersonRole).where(PersonRole.semantic_key == "author"))).one() - person = Person(full_name="Counted Person") + person = Person(given_names="Counted", last_name="Person") document = Document(name="Linked For Count") session.add_all([person, document]) await session.flush() @@ -72,7 +73,7 @@ class TestPeoplePageRendering: assert response.status_code == 200 assert "Create Person Record" in response.text - assert "Full name is required." in response.text + assert "Last name and first/middle names are required." in response.text assert "Birth date" in response.text assert "Death date" in response.text assert "Birth date (YYYY-MM-DD)" not in response.text @@ -90,9 +91,8 @@ class TestPeoplePageRendering: async with session_scope() as session: person = Person( - full_name="Grace Hopper", - display_name="Grace", - maiden_name="Murray", + given_names="Grace", + last_name="Hopper", birth_date=date(1906, 12, 9), birth_date_raw="1906", birth_place="New York", @@ -111,8 +111,8 @@ class TestPeoplePageRendering: assert response.status_code == 200 assert "Grace Hopper" in response.text assert "Full Name:" in response.text - assert "Display Name:" in response.text - assert "Maiden Name:" in response.text + assert "Last Name:" in response.text + assert "First & Middle:" in response.text or "First & Middle:" in response.text assert "Birth Date:" in response.text assert "1906-12-09" in response.text assert "Death Date:" in response.text @@ -141,7 +141,7 @@ class TestPeoplePageRendering: photo_file.write_bytes(b"portrait") async with session_scope() as session: - person = Person(full_name="Portrait Person") + person = Person(given_names="Portrait", last_name="Person") session.add(person) await session.flush() session.add( @@ -172,7 +172,7 @@ class TestPeoplePageRendering: photo_file.write_bytes(b"portrait") async with session_scope() as session: - person = Person(full_name="Gallery Person") + person = Person(given_names="Gallery", last_name="Person") session.add(person) await session.flush() session.add( @@ -202,7 +202,7 @@ class TestPeoplePageRendering: async with session_scope() as session: author_role = (await session.exec(select(PersonRole).where(PersonRole.semantic_key == "author"))).one() - person = Person(full_name="Linked Person") + person = Person(given_names="Linked", last_name="Person") document = Document(name="Linked Document") session.add_all([person, document]) await session.flush() @@ -250,11 +250,11 @@ class TestPeoplePageRendering: assert "Open" not in response.text @pytest.mark.asyncio - async def test_person_detail_page_hides_empty_maiden_name(self, app_client): + async def test_person_detail_page_hides_removed_maiden_name_field(self, app_client): _, client = app_client async with session_scope() as session: - person = Person(full_name="No Maiden Name") + person = Person(given_names="No Maiden", last_name="Name") session.add(person) await session.commit() person_id = str(person.id) @@ -285,7 +285,7 @@ class TestPeoplePageRendering: _, client = app_client async with session_scope() as session: - person = Person(full_name="Editable Person", display_name="EP") + person = Person(given_names="Editable", last_name="Person") session.add(person) await session.commit() person_id = str(person.id) @@ -294,8 +294,9 @@ class TestPeoplePageRendering: assert response.status_code == 200 assert "Edit Person Record" in response.text - assert "Full name is required." in response.text - assert "Editable Person" in response.text + assert "Last name and first/middle names are required." in response.text + assert "Editable" in response.text + assert "Person" in response.text assert "Save changes" in response.text @pytest.mark.asyncio @@ -303,7 +304,7 @@ class TestPeoplePageRendering: _, client = app_client async with session_scope() as session: - person = Person(full_name="Safe Delete") + person = Person(given_names="Safe", last_name="Delete") session.add(person) await session.commit() person_id = str(person.id)