generated from john/python-template
V4.6 Phase 5 follow-up: consolidate remaining hand-rolled tables
Completes the table consolidation deferred within Phase 5. Section 4 of the review lists table construction as a duplication pattern; three call sites had been left bypassing the canonical builder. - table/common.py: build_table gains a row_key parameter so callers with a non-"id" primary key can use it. - linked_people.py: replaces its hand-rolled ui.table with build_table (row_key="person_id", selection="multiple", rows_per_page=0, no search). - print_preview_page.py: _render_metadata_table and _render_job_table now share a local _render_print_table helper. Print tables deliberately do not use build_table - they must never paginate or render a search box, and they carry print-only styling. The helper docstring records that rationale. - tests/test_ui_boundaries.py: new AST guard asserting ui.table() is called from exactly two modules - components/table/common.py and pages/print_preview_page.py. Also closes the intermittent tests/ui/test_jobs_page.py failure observed once after Phase 5 as environmental. Unreproduced across ~54 sequential full-suite runs (including a 25-run soak) and 5 concurrent-process runs. No code change. Verification: ruff check src tests clean; 275 passed, 4 skipped. Co-authored-by: Copilot App <[email protected]>
This commit is contained in:
@@ -15,6 +15,7 @@ from transcription.services.people import DocumentPersonInput
|
|||||||
from transcription.ui.components.formatters import parse_uuid
|
from transcription.ui.components.formatters import parse_uuid
|
||||||
from transcription.ui.components.formatters import person_selector_label
|
from transcription.ui.components.formatters import person_selector_label
|
||||||
from transcription.ui.components.primitives import destructive_button
|
from transcription.ui.components.primitives import destructive_button
|
||||||
|
from transcription.ui.components.table.common import build_table
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True, slots=True)
|
@dataclass(frozen=True, slots=True)
|
||||||
@@ -64,16 +65,18 @@ class LinkedPeopleEditor:
|
|||||||
}
|
}
|
||||||
for link in sorted(self.links, key=lambda item: self._person_label(item.person_id).casefold())
|
for link in sorted(self.links, key=lambda item: self._person_label(item.person_id).casefold())
|
||||||
]
|
]
|
||||||
self.table = ui.table(
|
self.table = build_table(
|
||||||
columns=[
|
rows,
|
||||||
|
[
|
||||||
{"name": "person", "label": "Person", "field": "person", "align": "left", "sortable": True},
|
{"name": "person", "label": "Person", "field": "person", "align": "left", "sortable": True},
|
||||||
{"name": "role", "label": "Role", "field": "role", "align": "left", "sortable": True},
|
{"name": "role", "label": "Role", "field": "role", "align": "left", "sortable": True},
|
||||||
],
|
],
|
||||||
rows=rows,
|
default_sort_by="person",
|
||||||
row_key="person_id",
|
show_search=False,
|
||||||
selection="multiple",
|
selection="multiple",
|
||||||
pagination={"rowsPerPage": 0, "sortBy": "person"},
|
rows_per_page=0,
|
||||||
).classes("w-full ui-table")
|
row_key="person_id",
|
||||||
|
)
|
||||||
|
|
||||||
with ui.row().classes("w-full items-center gap-2"):
|
with ui.row().classes("w-full items-center gap-2"):
|
||||||
ui.button("Add", icon="add", on_click=self._begin_add).classes("ui-btn-primary")
|
ui.button("Add", icon="add", on_click=self._begin_add).classes("ui-btn-primary")
|
||||||
|
|||||||
@@ -50,6 +50,7 @@ def build_table(
|
|||||||
on_row_click_id: Callable[[str], None] | None = None,
|
on_row_click_id: Callable[[str], None] | None = None,
|
||||||
selection: str | None = None,
|
selection: str | None = None,
|
||||||
rows_per_page: int = 25,
|
rows_per_page: int = 25,
|
||||||
|
row_key: str = "id",
|
||||||
) -> Any:
|
) -> Any:
|
||||||
"""Build a styled Quasar table widget with optional client-side filtering and row-click handlers."""
|
"""Build a styled Quasar table widget with optional client-side filtering and row-click handlers."""
|
||||||
pagination: dict[str, Any] = {"rowsPerPage": rows_per_page}
|
pagination: dict[str, Any] = {"rowsPerPage": rows_per_page}
|
||||||
@@ -70,7 +71,7 @@ def build_table(
|
|||||||
table_kwargs: dict[str, Any] = {
|
table_kwargs: dict[str, Any] = {
|
||||||
"rows": rows,
|
"rows": rows,
|
||||||
"columns": columns,
|
"columns": columns,
|
||||||
"row_key": "id",
|
"row_key": row_key,
|
||||||
"pagination": pagination,
|
"pagination": pagination,
|
||||||
}
|
}
|
||||||
if selection is not None:
|
if selection is not None:
|
||||||
|
|||||||
@@ -3,6 +3,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import re
|
import re
|
||||||
|
from typing import Any
|
||||||
from uuid import UUID
|
from uuid import UUID
|
||||||
|
|
||||||
from nicegui import ui
|
from nicegui import ui
|
||||||
@@ -111,6 +112,30 @@ def _render_facsimile_source(*, document_id: UUID, source: DocumentPrintSource,
|
|||||||
ui.label(text).classes("print-transcription print-preserve-lines")
|
ui.label(text).classes("print-transcription print-preserve-lines")
|
||||||
|
|
||||||
|
|
||||||
|
def _render_print_table(
|
||||||
|
*,
|
||||||
|
columns: list[dict[str, Any]],
|
||||||
|
rows: list[dict[str, Any]],
|
||||||
|
extra_classes: str,
|
||||||
|
hide_header: bool,
|
||||||
|
) -> None:
|
||||||
|
"""Render one unpaginated print-layout table.
|
||||||
|
|
||||||
|
Print tables deliberately bypass `build_table`: they carry print-only styling,
|
||||||
|
must never paginate or expose a search box, and are rendered for a static
|
||||||
|
document rather than an interactive page.
|
||||||
|
"""
|
||||||
|
props = ["flat", "hide-bottom"]
|
||||||
|
if hide_header:
|
||||||
|
props.insert(1, "hide-header")
|
||||||
|
ui.table(
|
||||||
|
columns=columns,
|
||||||
|
rows=rows,
|
||||||
|
row_key="field",
|
||||||
|
pagination={"rowsPerPage": 0},
|
||||||
|
).props(" ".join(props)).classes(f"print-data-table {extra_classes}")
|
||||||
|
|
||||||
|
|
||||||
def _render_metadata_table(projection: DocumentPrintProjection) -> None:
|
def _render_metadata_table(projection: DocumentPrintProjection) -> None:
|
||||||
rows = [
|
rows = [
|
||||||
{"field": "Author", "value": ", ".join(projection.authors) or "Not set"},
|
{"field": "Author", "value": ", ".join(projection.authors) or "Not set"},
|
||||||
@@ -122,19 +147,19 @@ def _render_metadata_table(projection: DocumentPrintProjection) -> None:
|
|||||||
{"field": "Location Created", "value": projection.location_created or "Not set"},
|
{"field": "Location Created", "value": projection.location_created or "Not set"},
|
||||||
{"field": "Archival Identifier", "value": projection.archive_identifier or "Not set"},
|
{"field": "Archival Identifier", "value": projection.archive_identifier or "Not set"},
|
||||||
]
|
]
|
||||||
ui.table(
|
_render_print_table(
|
||||||
columns=[
|
columns=[
|
||||||
{"name": "field", "label": "", "field": "field", "align": "left"},
|
{"name": "field", "label": "", "field": "field", "align": "left"},
|
||||||
{"name": "value", "label": "", "field": "value", "align": "left"},
|
{"name": "value", "label": "", "field": "value", "align": "left"},
|
||||||
],
|
],
|
||||||
rows=rows,
|
rows=rows,
|
||||||
row_key="field",
|
extra_classes="print-metadata-table",
|
||||||
pagination={"rowsPerPage": 0},
|
hide_header=True,
|
||||||
).props("flat hide-header hide-bottom").classes("print-data-table print-metadata-table")
|
)
|
||||||
|
|
||||||
|
|
||||||
def _render_job_table(jobs: tuple[DocumentPrintJob, ...]) -> None:
|
def _render_job_table(jobs: tuple[DocumentPrintJob, ...]) -> None:
|
||||||
columns = [{"name": "field", "label": "", "field": "field", "align": "left"}]
|
columns: list[dict[str, Any]] = [{"name": "field", "label": "", "field": "field", "align": "left"}]
|
||||||
for index in range(1, len(jobs) + 1):
|
for index in range(1, len(jobs) + 1):
|
||||||
columns.append({"name": f"job_{index}", "label": f"Job {index}", "field": f"job_{index}", "align": "left"})
|
columns.append({"name": f"job_{index}", "label": f"Job {index}", "field": f"job_{index}", "align": "left"})
|
||||||
fields = (
|
fields = (
|
||||||
@@ -153,12 +178,12 @@ def _render_job_table(jobs: tuple[DocumentPrintJob, ...]) -> None:
|
|||||||
}
|
}
|
||||||
for field, value in fields
|
for field, value in fields
|
||||||
]
|
]
|
||||||
ui.table(
|
_render_print_table(
|
||||||
columns=columns,
|
columns=columns,
|
||||||
rows=rows,
|
rows=rows,
|
||||||
row_key="field",
|
extra_classes="print-job-table",
|
||||||
pagination={"rowsPerPage": 0},
|
hide_header=False,
|
||||||
).props("flat hide-bottom").classes("print-data-table print-job-table")
|
)
|
||||||
|
|
||||||
|
|
||||||
def reflow_transcription(text: str) -> list[str]:
|
def reflow_transcription(text: str) -> list[str]:
|
||||||
|
|||||||
@@ -83,3 +83,33 @@ def test_no_component_resolves_request_or_application_state():
|
|||||||
if found:
|
if found:
|
||||||
violations[str(path.relative_to(COMPONENTS_DIR))] = found
|
violations[str(path.relative_to(COMPONENTS_DIR))] = found
|
||||||
assert violations == {}
|
assert violations == {}
|
||||||
|
|
||||||
|
|
||||||
|
# `build_table` owns the interactive table styling; `print_preview_page` owns the
|
||||||
|
# print-only table, which must never paginate or expose a search box.
|
||||||
|
TABLE_OWNERS = frozenset({"components/table/common.py", "pages/print_preview_page.py"})
|
||||||
|
|
||||||
|
|
||||||
|
def _calls_ui_table(tree: ast.Module) -> bool:
|
||||||
|
for node in ast.walk(tree):
|
||||||
|
if not isinstance(node, ast.Call):
|
||||||
|
continue
|
||||||
|
func = node.func
|
||||||
|
if (
|
||||||
|
isinstance(func, ast.Attribute)
|
||||||
|
and func.attr == "table"
|
||||||
|
and isinstance(func.value, ast.Name)
|
||||||
|
and func.value.id == "ui"
|
||||||
|
):
|
||||||
|
return True
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
def test_only_the_designated_owners_construct_a_raw_table():
|
||||||
|
"""Review section 4: table styling lives in one place, not in every page."""
|
||||||
|
offenders = sorted(
|
||||||
|
str(path.relative_to(UI_DIR)).replace("\\", "/")
|
||||||
|
for path in UI_DIR.rglob("*.py")
|
||||||
|
if _calls_ui_table(ast.parse(path.read_text(encoding="utf-8")))
|
||||||
|
)
|
||||||
|
assert set(offenders) == TABLE_OWNERS
|
||||||
|
|||||||
Reference in New Issue
Block a user