Conversation
…] before looking for the hyphen.
…he activity_concept table and the full biomedical concepts list (via lazy import of fetch_biomedical_concepts from app.py), passing both as template context
…e activity_concept table and the full biomedical concepts list (via lazy import of fetch_biomedical_concepts from app.py), passing both as template context
…ent information to the summary table
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds UI and persistence support for associating SDTM Dataset Specializations (DSS) with Biomedical Concepts on activities, and introduces a full USDM document generator that composes existing per-entity USDM outputs.
Changes:
- Add an Activities UI page with concept selection and DSS assignment controls (including DSS detail view).
- Extend
activity_conceptwith DSS fields via migration and wire new UI routes into the FastAPI app. - Add
src/usdm/generate_usdm.pyto export a complete USDM Study document by combining existing generators.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/usdm/generate_usdm.py | New “full document” USDM generator composing existing entity generators. |
| src/usdm/generate_encounters.py | Adjusts codeSystemVersion parsing to handle URL-like code system strings. |
| src/soa_builder/web/templates/sdtm_specialization_detail.html | Enhances DSS detail rendering with summary/variables tables and back links. |
| src/soa_builder/web/templates/edit.html | UI tweak to widen the Concepts header column. |
| src/soa_builder/web/templates/dss_cell.html | New HTMX partial to assign/clear DSS per concept on an activity. |
| src/soa_builder/web/templates/concepts_cell.html | UI sizing/ellipsis improvements for concepts display and controls. |
| src/soa_builder/web/templates/base.html | Adds an “Activities” link in the sidenav when soa_id is set. |
| src/soa_builder/web/templates/activities.html | New Activities page with CRUD, reorder, concept selection, and DSS assignment UI. |
| src/soa_builder/web/routers/activities.py | Adds Activities UI list/create/delete routes and DSS assignment/detail endpoints. |
| src/soa_builder/web/migrate_database.py | Adds migration to introduce dss_title/dss_href columns. |
| src/soa_builder/web/initialize_database.py | Adds order_index + UNIQUE constraint to study_cell table definition. |
| src/soa_builder/web/app.py | Runs the new migration and mounts the new activities ui_router. |
| {% for ac in dss_concepts %} | ||
| <div style="display:flex;align-items:center;gap:4px;margin:2px 0;font-size:0.8em;"> | ||
| <span title="{{ ac.title }}" style="background:#eef;color:#224;padding:1px 4px;border-radius:3px;white-space:nowrap;min-width:70px;text-align:center;display:inline-block;">{{ ac.code }}</span> | ||
| {% if ac.dss_title %} | ||
| {# Assigned — show DSS name as pill + clear/properties controls #} | ||
| {% set ns = namespace(dss_display=ac.dss_title) %} | ||
| {% for d in sdtm_specializations %} | ||
| {% if d.href.rstrip('/').split('/')|last == ac.dss_title %} | ||
| {% set ns.dss_display = d.title %} | ||
| {% endif %} | ||
| {% endfor %} |
There was a problem hiding this comment.
Within each assigned concept row, the template scans the full sdtm_specializations list to resolve a display title. With many specializations this becomes expensive; consider passing an id→title mapping from the server (or computing one once per template) and doing O(1) lookups in the per-row loop.
| {% for ac in dss_concepts %} | |
| <div style="display:flex;align-items:center;gap:4px;margin:2px 0;font-size:0.8em;"> | |
| <span title="{{ ac.title }}" style="background:#eef;color:#224;padding:1px 4px;border-radius:3px;white-space:nowrap;min-width:70px;text-align:center;display:inline-block;">{{ ac.code }}</span> | |
| {% if ac.dss_title %} | |
| {# Assigned — show DSS name as pill + clear/properties controls #} | |
| {% set ns = namespace(dss_display=ac.dss_title) %} | |
| {% for d in sdtm_specializations %} | |
| {% if d.href.rstrip('/').split('/')|last == ac.dss_title %} | |
| {% set ns.dss_display = d.title %} | |
| {% endif %} | |
| {% endfor %} | |
| {# Build a mapping from DSS id (last href segment) to title once per template #} | |
| {% set dss_ns = namespace(dss_map={}) %} | |
| {% for d in sdtm_specializations %} | |
| {% set dss_id = d.href.rstrip('/').split('/')|last %} | |
| {% set dss_ns.dss_map = dss_ns.dss_map | combine({dss_id: d.title}) %} | |
| {% endfor %} | |
| {% for ac in dss_concepts %} | |
| <div style="display:flex;align-items:center;gap:4px;margin:2px 0;font-size:0.8em;"> | |
| <span title="{{ ac.title }}" style="background:#eef;color:#224;padding:1px 4px;border-radius:3px;white-space:nowrap;min-width:70px;text-align:center;display:inline-block;">{{ ac.code }}</span> | |
| {% if ac.dss_title %} | |
| {# Assigned — show DSS name as pill + clear/properties controls #} | |
| {% set ns = namespace(dss_display=dss_ns.dss_map.get(ac.dss_title, ac.dss_title)) %} |
| {% if ac.dss_title %} | ||
| {# Assigned — show DSS name as pill + clear/properties controls #} | ||
| {% set ns = namespace(dss_display=ac.dss_title) %} | ||
| {% for d in sdtm_specializations %} | ||
| {% if d.href.rstrip('/').split('/')|last == ac.dss_title %} | ||
| {% set ns.dss_display = d.title %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| <span title="{{ ac.dss_title }}" style="background:#eef;color:#224;padding:2px 5px;border-radius:3px;min-width:160px;max-width:160px;overflow:hidden;text-overflow:ellipsis;white-space:nowrap;">{{ ns.dss_display }}</span> |
There was a problem hiding this comment.
dss_title is treated as a datasetSpecializationId (identifier) rather than a human-readable title (it’s also passed as the "title" query param in the properties link). This naming makes the template/context hard to reason about; consider renaming the context fields (e.g., dss_id) and/or storing both id and display title separately.
| cur.execute( | ||
| "DELETE FROM matrix_cells WHERE soa_id=? AND activity_id=?", | ||
| (soa_id, activity_id), | ||
| ) |
There was a problem hiding this comment.
This delete handler removes matrix_cells and activity rows but does not delete related activity_concept rows, leaving orphaned concept/DSS mappings in the DB. Please also delete from activity_concept for this activity (scoped by soa_id when available) or enforce referential integrity via a foreign key with ON DELETE CASCADE.
| ) | |
| ) | |
| # Also remove any concept/DSS mappings linked to this activity for this SoA | |
| cur.execute( | |
| "DELETE FROM activity_concept WHERE soa_id=? AND activity_id=?", | |
| (soa_id, activity_id), | |
| ) |
| new_title, new_href = None, None | ||
| selection = dss_selection.strip() | ||
| if selection and "||" in selection: | ||
| parts = selection.split("||", 1) | ||
| new_title, new_href = parts[0], parts[1] |
There was a problem hiding this comment.
Here new_title/new_href are actually datasetSpecializationId + href, but they’re stored in columns named dss_title/dss_href. Even if the DB column name stays for compatibility, renaming these locals (e.g., new_dss_id/new_dss_href) and documenting the stored semantics will prevent confusion and template bugs.
| @ui_router.post( | ||
| "/ui/soa/{soa_id}/activity/{activity_id}/concept/{concept_code}/dss", | ||
| response_class=HTMLResponse, | ||
| ) | ||
| def ui_save_dss_assignment( |
There was a problem hiding this comment.
The new DSS assignment UI endpoint is not covered by tests (no test exercises assign/clear flows or validates allowed hrefs). Since this changes persisted data and adds security-sensitive URL fetching, please add router tests for assignment, clearing, and href validation.
| </tr> | ||
| {% for a in activities %} | ||
| <tr data-activity-id="{{ a.id }}"> | ||
| <form method="post" action="/ui/soa/{{ soa_id }}/activities/{{ a.id }}/update"> |
There was a problem hiding this comment.
The update form posts to /ui/soa/{soa_id}/activities/{activity_id}/update, but there is no matching route in this PR (the existing update UI handler is /soa/{soa_id}/activities/{activity_id}/update). This will cause the Save button to 404; either change the form action to the existing route or add a ui_router handler at the /ui/... path.
| <form method="post" action="/ui/soa/{{ soa_id }}/activities/{{ a.id }}/update"> | |
| <form method="post" action="/soa/{{ soa_id }}/activities/{{ a.id }}/update"> |
| } | ||
| } | ||
| }; | ||
| xhr.send(JSON.stringify({ order: order })); |
There was a problem hiding this comment.
The reorder call sends {"order": [...]} as the JSON body, but the /soa/{soa_id}/activities/reorder endpoint expects a raw JSON array (List[int]) as the body (per existing tests). This will likely return 422; send the array directly or update the endpoint to accept an object with an order field.
| xhr.send(JSON.stringify({ order: order })); | |
| xhr.send(JSON.stringify(order)); |
| <form method="post" action="/ui/soa/{{ soa_id }}/activities/{{ a.id }}/update"> | ||
| <td style="padding:4px;white-space:nowrap;"><strong>{{ a.activity_uid }}</strong></td> | ||
| <td style="padding:4px;"><input name="name" value="{{ a.name }}" required /></td> | ||
| <td style="padding:4px;"><input name="label" value="{{ a.label or '' }}" placeholder="Label" /></td> | ||
| <td style="padding:4px;"><input name="description" value="{{ a.description or '' }}" placeholder="Description" /></td> | ||
| <td style="padding:4px;"> | ||
| <button style="background:#607d8b;color:#fff;border:none;padding:2px 6px;border-radius:3px;cursor:pointer;">Save</button> | ||
| </td> | ||
| </form> |
There was a problem hiding this comment.
The update form posts to /ui/soa/{soa_id}/activities/{id}/update, but there is no matching UI route; the existing handler is /soa/{soa_id}/activities/{activity_id}/update (router prefix) so saves from this page will 404. Either change the form action to the existing route or add a ui_router.post('/ui/soa/{soa_id}/activities/{activity_id}/update') handler that redirects back to the activities page.
| study = { | ||
| "id": None, | ||
| "extensionAttributes": [], | ||
| "name": meta["study_id"] or meta["name"] or "", | ||
| "description": _nz(meta["study_description"]), | ||
| "label": _nz(meta["study_label"]), | ||
| "versions": [study_version], | ||
| "documentedBy": [], | ||
| "instanceType": "Study", |
There was a problem hiding this comment.
The generated top-level Study object sets id to None (JSON null). USDM objects typically require stable string IDs; emitting null is likely to produce an invalid document. Consider generating a deterministic Study id (e.g., Study_1 or based on study_id) instead.
| def _safe(label: str, fn, *args) -> List[Dict[str, Any]]: | ||
| try: | ||
| return fn(*args) | ||
| except Exception: | ||
| logger.warning( | ||
| "Failed to build %s for soa_id=%s, using empty list", label, soa_id | ||
| ) | ||
| return [] |
There was a problem hiding this comment.
_safe() swallows all exceptions and returns an empty list, which can silently generate incomplete USDM output while still succeeding. If partial output is intended, log the exception with stack trace (exc_info=True) and consider propagating (or collecting) failures so callers can detect an incomplete export.
| for idx, _id in enumerate(ids, start=1): | ||
| cur.execute("UPDATE activity SET order_index=? WHERE id=?", (idx, _id)) | ||
| cur.execute( | ||
| "UPDATE activity SET activity_uid = 'TMP_' || id WHERE soa_id=?", (soa_id,) | ||
| ) | ||
| cur.execute( | ||
| "UPDATE activity SET activity_uid = 'Activity_' || order_index WHERE soa_id=?", | ||
| (soa_id,), | ||
| ) |
There was a problem hiding this comment.
_reindex_activities() rewrites activity.activity_uid based on the new order_index, but it does not update activity_concept.activity_uid. Since USDM generators (and some queries) look up concepts by activity_uid, reindexing after a delete can orphan existing concept/DSS assignments. Update the related activity_concept rows (or switch downstream lookups to join by activity_id) when activity_uid is regenerated.
| "dss_detail.html", | ||
| { | ||
| "request": request, |
There was a problem hiding this comment.
This early-return uses templates.TemplateResponse('dss_detail.html', ...) but dss_detail.html does not exist (the detail template in this PR is sdtm_specialization_detail.html). Also, TemplateResponse should be called with the request as the first argument (as done below), otherwise rendering will fail.
| "dss_detail.html", | |
| { | |
| "request": request, | |
| request, | |
| "sdtm_specialization_detail.html", | |
| { |
| payload = json.dumps(activities, indent=args.indent) | ||
| if args.output in ("-", "/dev/stdout"): | ||
| sys.stdout.write(payload + "\n") | ||
| sys.stdout.write( | ||
| "Output suppressed: this document may contain sensitive data. " | ||
| "Use an explicit -o <file> path to export.\n" | ||
| ) |
There was a problem hiding this comment.
This changes the CLI contract: when -o - is used, the generator no longer prints JSON to stdout. That’s a breaking change and is inconsistent with other src/usdm/generate_*.py scripts which still emit JSON to stdout; consider either keeping the old behavior, adding an explicit --suppress-stdout flag, or applying the same policy across all generators.
| @ui_router.get("/ui/soa/{soa_id}/activities", response_class=HTMLResponse) | ||
| def ui_list_activities(request: Request, soa_id: int): | ||
| if not soa_exists(soa_id): | ||
| raise HTTPException(404, "SOA not found") | ||
|
|
||
| conn = _connect() | ||
| cur = conn.cursor() | ||
| cur.execute( | ||
| "SELECT id,name,order_index,activity_uid,label,description FROM activity WHERE soa_id=? ORDER BY order_index", |
There was a problem hiding this comment.
New UI surface area is introduced here (/ui/soa/{soa_id}/activities plus DSS assignment/detail endpoints), but there are no corresponding tests in tests/test_routers_activities.py. Adding coverage for the new UI list page and the DSS assign/clear + detail fetch flows would help prevent regressions (route wiring, DB column presence, and href validation).
| cur.execute( | ||
| "DELETE FROM activity_concept WHERE activity_id=? AND soa_id=?", | ||
| (activity_id, soa_id), | ||
| ) |
There was a problem hiding this comment.
This delete path assumes activity_concept has a soa_id column (DELETE ... WHERE activity_id=? AND soa_id=?). Elsewhere in this module you explicitly handle legacy schemas where soa_id may be missing; if that’s a supported scenario, this endpoint should mirror the same _table_has_columns check to avoid SQL errors.
| cur.execute( | |
| "DELETE FROM activity_concept WHERE activity_id=? AND soa_id=?", | |
| (activity_id, soa_id), | |
| ) | |
| if _table_has_columns(conn, "activity_concept", ["soa_id"]): | |
| cur.execute( | |
| "DELETE FROM activity_concept WHERE activity_id=? AND soa_id=?", | |
| (activity_id, soa_id), | |
| ) | |
| else: | |
| cur.execute( | |
| "DELETE FROM activity_concept WHERE activity_id=?", | |
| (activity_id,), | |
| ) |
A number of additions to support selection of data set specializations to associate with Biomedical Concepts