Conversation
…t names when labels not set
There was a problem hiding this comment.
Pull request overview
This PR enhances the UI with collapsible biomedical concepts, improved matrix headers, timeline-based XLSX export, and scheduled activity instance reordering capabilities.
Changes:
- Added collapsible lists for biomedical concepts with count badges and toggle functionality
- Enhanced matrix headers to display epochs (with merged cells), encounters, instances, study days, timing, and visit windows
- Implemented separate XLSX worksheets for each timeline with complete header metadata
- Added drag-to-reorder functionality for scheduled activity instances with persistent order saving
- Updated database schema to include
hrefcolumn inactivity_concepttable andorder_indexin instances - Standardized header labels across UI templates (changed "UID" to "id")
- Added "Return to Edit Page" navigation links across entity management pages
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/soa_builder/web/templates/timings.html | Added return link; changed header from "UID" to "id" |
| src/soa_builder/web/templates/schedule_timelines.html | Added return link; changed header from "UID" to "id" |
| src/soa_builder/web/templates/rules.html | Added return link; changed header from "UID" to "id" |
| src/soa_builder/web/templates/instances.html | Added return link, order column, reorder controls with Save Order button, and client-side JS for drag functionality |
| src/soa_builder/web/templates/epochs.html | Moved Save Order button into table header; changed "UID" to "id" |
| src/soa_builder/web/templates/encounters.html | Added return link; changed header from "UID" to "id" |
| src/soa_builder/web/templates/elements.html | Added return link; changed header from "UID" to "id" |
| src/soa_builder/web/templates/edit_backup.html | Removed entire backup template file (642 lines deleted) |
| src/soa_builder/web/templates/edit.html | Enhanced matrix with epoch row merging, added instance/timing rows, improved header display logic |
| src/soa_builder/web/templates/concepts_cell.html | Implemented collapsible concepts with toggle, count pill, and unique cell IDs per timeline context |
| src/soa_builder/web/templates/arms.html | Added return link; changed header from "UID" to "id" |
| src/soa_builder/web/routers/instances.py | Added reorder API endpoint with audit logging |
| src/soa_builder/web/migrate_database.py | Added migration for href column in activity_concept table |
| src/soa_builder/web/initialize_database.py | Updated schema to include href column in activity_concept table definition |
| src/soa_builder/web/app.py | Added timeline-based XLSX export with header rows; refactored instance queries to use LEFT JOINs; created helper functions for enriched data and header generation |
| <button style="background:#c62828;color:#fff;border:none;padding:2px 6px;border-radius:3px;cursor:pointer;"> | ||
| Delete | ||
| </button> |
There was a problem hiding this comment.
The button text "Delete" is split across multiple lines unnecessarily. This should be on a single line for consistency with the rest of the codebase and readability. Recommend placing the text immediately after the opening tag.
| <button style="background:#c62828;color:#fff;border:none;padding:2px 6px;border-radius:3px;cursor:pointer;"> | |
| Delete | |
| </button> | |
| <button style="background:#c62828;color:#fff;border:none;padding:2px 6px;border-radius:3px;cursor:pointer;">Delete</button> |
| <tr class="matrix-epoch-row"> | ||
| <th></th> | ||
| <th style="text-align:right;font-size:0.50em;">Epoch:-></th> | ||
| {% set prev_epoch = namespace(value=None, count=0) %} |
There was a problem hiding this comment.
The variable name prev_epoch is misleading because it stores the current epoch being accumulated, not the previous one. Rename to current_epoch_group or accumulated_epoch to better reflect its purpose.
| LEFT JOIN epoch e ON e.epoch_uid = i.epoch_uid AND e.soa_id = i.soa_id | ||
| LEFT JOIN timing tm ON tm.id = v.scheduledAtId AND tm.soa_id = v.soa_id | ||
| WHERE i.soa_id=? | ||
| ORDER BY COALESCE(i.member_of_timeline, 'zzz'), LENGTH(i.instance_uid), i.instance_uid |
There was a problem hiding this comment.
The magic string 'zzz' is used as a sentinel value to push unassigned timelines to the end of the sort. This is fragile if timeline UIDs ever start with characters lexicographically after 'z'. Use a more robust approach like a CASE expression or separate the unassigned instances explicitly.
| ORDER BY COALESCE(i.member_of_timeline, 'zzz'), LENGTH(i.instance_uid), i.instance_uid | |
| ORDER BY | |
| CASE WHEN i.member_of_timeline IS NULL THEN 1 ELSE 0 END, | |
| i.member_of_timeline, | |
| LENGTH(i.instance_uid), | |
| i.instance_uid |
| def _add_header_rows_to_worksheet(worksheet, enriched_instances): | ||
| """Add header rows to a worksheet with instance metadata.""" | ||
| # Insert 6 rows at the top for header rows | ||
| worksheet.insert_rows(1, 6) |
There was a problem hiding this comment.
The magic number 6 represents the number of header rows (Epoch, Encounter, Instance, Study Day, Timing, Visit Window) but is not documented or named. Define a constant like HEADER_ROW_COUNT = 6 to make the code self-documenting and easier to maintain if header rows change.
| def _add_header_rows_to_worksheet(worksheet, enriched_instances): | |
| """Add header rows to a worksheet with instance metadata.""" | |
| # Insert 6 rows at the top for header rows | |
| worksheet.insert_rows(1, 6) | |
| HEADER_ROW_COUNT = 6 | |
| def _add_header_rows_to_worksheet(worksheet, enriched_instances): | |
| """Add header rows to a worksheet with instance metadata.""" | |
| # Insert 6 rows at the top for header rows | |
| worksheet.insert_rows(1, HEADER_ROW_COUNT) |
| df_tl["Concept UIDs"] = concept_titles_strings | ||
|
|
||
| # Sanitize sheet name (max 31 chars, no special chars) | ||
| sheet_name = f"SoA - {timeline_name}"[:31] |
There was a problem hiding this comment.
The magic number 31 represents Excel's maximum sheet name length but is not documented. Define a constant like EXCEL_MAX_SHEET_NAME_LENGTH = 31 to make this constraint explicit and easier to maintain.
| LEFT JOIN epoch e ON e.epoch_uid = i.epoch_uid AND e.soa_id = i.soa_id | ||
| LEFT JOIN timing tm ON tm.id = v.scheduledAtId AND tm.soa_id = v.soa_id | ||
| WHERE i.soa_id=? | ||
| ORDER BY COALESCE(i.member_of_timeline, 'zzz'), LENGTH(i.instance_uid), i.instance_uid |
There was a problem hiding this comment.
Duplicate of the same magic string pattern found in _fetch_enriched_instances (line 1982). Both queries use 'zzz' as a sentinel, which should be addressed consistently across both functions.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| conn = _connect() | ||
| cur = conn.cursor() | ||
| cur.execute( | ||
| "SELECT id,name FROM instances WHERE soa_id=? ORDER BY order_index", (soa_id,) |
There was a problem hiding this comment.
The query selects rows ordered by order_index, but the order_index column is not included in the migration scripts shown in this PR. While line 102 in instances.html displays i.order_index, there's no corresponding migration adding this column to the instances table (only member_of_timeline and href migrations are present). This will cause a SQL error if the column doesn't exist.
| def _migrate_activity_concept_add_href(): | ||
| """Add href column to store value for API from which the codeSystem and codeSystemVersion USDM properties can be derived""" |
There was a problem hiding this comment.
Corrected capitalization in 'Biomdical' to 'Biomedical' in the PR description.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| {# Partial for concepts cell in matrix (add/remove only; concepts immutable) #} | ||
| <td class="concepts-cell" id="concepts-cell-{{ activity_id }}" style="vertical-align:top;"> | ||
| {% set concept_count = selected_list | length %} | ||
| {% set cell_id_suffix = timeline_context ~ '-' ~ activity_id if timeline_context is defined else 'soa-' ~ soa_id ~ '-act-' ~ activity_id %} |
There was a problem hiding this comment.
Corrected spelling of 'Concpets' to 'Concepts' in the PR description.
| @router.post("/soa/{soa_id}/instances/reorder", response_class=JSONResponse) | ||
| def reorder_instances_api( | ||
| soa_id: int, | ||
| order: List[int] = Body(..., embed=True), # JSON body: {"order":[...]} |
There was a problem hiding this comment.
The comment describes the expected JSON structure but uses 'embed=True' which wraps the parameter in a JSON object. Consider clarifying that the request body should be {\"order\": [1, 2, 3]} rather than just [1, 2, 3] to match the actual parameter configuration.
| order: List[int] = Body(..., embed=True), # JSON body: {"order":[...]} | |
| order: List[int] = Body(..., embed=True), # JSON body: {"order": [1, 2, 3]} |
| def _add_header_rows_to_worksheet(worksheet, enriched_instances): | ||
| """Add header rows to a worksheet with instance metadata.""" | ||
| # Insert 6 rows at the top for header rows | ||
| worksheet.insert_rows(1, 6) |
There was a problem hiding this comment.
The comment says '6 rows' but the header construction shows 6 distinct semantic rows (Epoch, Encounter, Instance, Study Day, Timing, Visit Window). Consider adding a comment explaining what each of these 6 rows represents for future maintainability.
| } | ||
| } | ||
|
|
||
| fetch('/soa/' + soaId + '/instances/reorder', { |
There was a problem hiding this comment.
The fetch endpoint uses /soa/ prefix while the router defines the endpoint at /soa/{soa_id}/instances/reorder. This URL construction is correct but consider using template literals for better readability: fetch(`/soa/${soaId}/instances/reorder`, {
| df_tl["Concept UIDs"] = concept_titles_strings | ||
|
|
||
| # Sanitize sheet name (max 31 chars, no special chars) | ||
| sheet_name = f"SoA - {timeline_name}"[:31] |
There was a problem hiding this comment.
The sheet name is truncated to 31 characters (Excel's limit) after adding the 'SoA - ' prefix. If timeline_name is long, this could result in multiple sheets with identical truncated names causing overwrites. Consider truncating timeline_name first to ensure uniqueness: sheet_name = f\"SoA - {timeline_name[:24]}\"
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| col_idx = 3 | ||
| epoch_groups = [] # Track (value, start_col, end_col) for merging | ||
| prev_epoch = None | ||
| start_col = 3 |
There was a problem hiding this comment.
The variable start_col is initialized to 3 twice (lines 2018 and 2021), which is redundant. The initialization on line 2021 should be removed since col_idx is already set to 3 on line 2018.
| start_col = 3 |
| </tr> | ||
| {% else %} | ||
| <tr><td colspan="12" style="padding:6px;color:#777;">No instances yet.</td></tr> | ||
| <tr><td colspan="13" style="padding:6px;color:#777;">No instances yet.</td></tr> |
There was a problem hiding this comment.
The hardcoded colspan value of 13 creates a maintenance burden. If columns are added or removed from the table, this value must be manually updated. Consider calculating the colspan dynamically or using a variable.
| sheet_name = f"SoA - {timeline_name}"[:31] | ||
| sheet_name = ( | ||
| sheet_name.replace("/", "-") | ||
| .replace("\\", "-") | ||
| .replace("*", "-") | ||
| .replace("?", "-") | ||
| .replace(":", "-") | ||
| .replace("[", "-") | ||
| .replace("]", "-") | ||
| ) |
There was a problem hiding this comment.
Sanitizing the sheet name happens in two separate steps: truncation first, then character replacement. If the original timeline_name is exactly 31 characters and contains invalid characters that need replacement, the final sheet_name could still be 31 characters but with different content than intended. Consider sanitizing before truncation to ensure consistent behavior.
Update of format on UI pages
Collapsible lists of Biomdical Concpets associated with activities
XLSX SoA export now includes all headers as shown in UI matrix
Separate matrix for each timeline
Reorder scheduled activity instances
New database columns in migration scripts