-
Notifications
You must be signed in to change notification settings - Fork 1
UI features #57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI features #57
Conversation
… as the first parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the SoA Workbench with comprehensive unit test coverage, a timeline selector UI feature, and improved biomedical concept display in audit reports.
Changes:
- Added 12 new router test files covering all API/UI endpoints (~216 tests)
- Implemented timeline selector in matrix view for switching between schedule timelines
- Enhanced Excel audit export to show biomedical concept names alongside UIDs
- Added documentation files for test organization and API endpoint catalog
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_routers_visits.py | Comprehensive tests for visits router (20 tests) |
| tests/test_routers_timings.py | Tests for timings router including ISO8601 durations (23 tests) |
| tests/test_routers_schedule_timelines.py | Tests for schedule timelines with main timeline validation (20 tests) |
| tests/test_routers_rules.py | Tests for transition rules router (21 tests) |
| tests/test_routers_rollback.py | Tests for rollback audit endpoints (14 tests) |
| tests/test_routers_instances.py | Tests for scheduled activity instances (16 tests) |
| tests/test_routers_freezes.py | Tests for freeze/snapshot operations (14 tests) |
| tests/test_routers_epochs.py | Tests for epochs router with reordering (12 tests) |
| tests/test_routers_elements.py | Tests for elements router with audit trails (13 tests) |
| tests/test_routers_audits.py | Tests for audit trail endpoints (14 tests) |
| tests/test_routers_arms.py | Tests for arms router with UID generation (14 tests) |
| tests/test_routers_activities.py | Tests for activities router with bulk operations (19 tests) |
| tests/test_epoch_reorder_audit_api.py | Enhanced database safety checks for test isolation |
| tests/TEST_FILES_REVIEW.md | Documentation of test organization and coverage analysis |
| tests/ROUTER_TESTS_README.md | Summary of router test files and validation status |
| src/soa_builder/web/templates/edit.html | Timeline selector UI with matrix grouping by timeline |
| src/soa_builder/web/routers/instances.py | Fixed template response call signature |
| src/soa_builder/web/initialize_database.py | Reorganized schema initialization with documentation |
| src/soa_builder/web/app.py | Timeline selector logic and concept names in Excel export |
| docs/api_endpoints.csv | Complete catalog of 165+ endpoints |
| README_endpoints.md | Comprehensive API documentation with curl examples |
| README.md | Updated architecture notes and documentation references |
| .github/copilot-instructions.md | Developer guide with USDM model relationships |
Comments suppressed due to low confidence (1)
src/soa_builder/web/templates/edit.html:1
- Typo in field name 'conatctModes' should be 'contactModes' (missing 't' between 'con' and 'act').
{% extends 'base.html' %}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
spelling Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Group instances by timeline | ||
| instances_by_timeline = {} | ||
| for inst in instances: | ||
| timeline_key = inst.get("member_of_timeline") or "unassigned" | ||
| if timeline_key not in instances_by_timeline: | ||
| instances_by_timeline[timeline_key] = [] | ||
| instances_by_timeline[timeline_key].append(inst) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instance grouping logic can be simplified using collections.defaultdict to reduce redundant key existence checks and improve readability. Replace the explicit dictionary initialization pattern with defaultdict.
| console.log('Switching to timeline:', timelineUid); | ||
|
|
||
| // Hide all matrix containers | ||
| document.querySelectorAll('.matrix-container').forEach(function(container) { | ||
| console.log('Found container with timeline:', container.dataset.timeline); | ||
| container.style.display = 'none'; | ||
| }); | ||
|
|
||
| // Show selected timeline's matrix | ||
| const targetContainer = document.querySelector(`.matrix-container[data-timeline="${timelineUid}"]`); | ||
| console.log('Target container:', targetContainer); | ||
| if (targetContainer) { | ||
| targetContainer.style.display = 'block'; | ||
| } else { | ||
| console.warn('No matrix container found for timeline:', timelineUid); |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switchTimeline function contains debug console.log statements that should be removed before production deployment to reduce console noise and potential performance overhead in production environments.
| console.log('Switching to timeline:', timelineUid); | |
| // Hide all matrix containers | |
| document.querySelectorAll('.matrix-container').forEach(function(container) { | |
| console.log('Found container with timeline:', container.dataset.timeline); | |
| container.style.display = 'none'; | |
| }); | |
| // Show selected timeline's matrix | |
| const targetContainer = document.querySelector(`.matrix-container[data-timeline="${timelineUid}"]`); | |
| console.log('Target container:', targetContainer); | |
| if (targetContainer) { | |
| targetContainer.style.display = 'block'; | |
| } else { | |
| console.warn('No matrix container found for timeline:', timelineUid); | |
| // Hide all matrix containers | |
| document.querySelectorAll('.matrix-container').forEach(function(container) { | |
| container.style.display = 'none'; | |
| }); | |
| // Show selected timeline's matrix | |
| const targetContainer = document.querySelector(`.matrix-container[data-timeline="${timelineUid}"]`); | |
| if (targetContainer) { | |
| targetContainer.style.display = 'block'; | |
| } else { | |
| if "soa_builder_web.db" in db_path and "test" not in db_path: | ||
| raise RuntimeError( | ||
| f"DANGER: Test trying to use production database: {db_path}. " | ||
| "Expected test database (soa_builder_web_tests.db)" | ||
| ) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The production database check uses substring matching which could produce false positives (e.g., a path like /soa_builder_web.db.backup would match). Consider using more precise path matching with Path.name or exact filename comparison for more robust database path validation.
| Method,Path,Type,Description,Response Type | ||
| GET,/,UI,Index page - lists studies & create form,HTML |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSV file lacks a header comment or version information indicating when it was generated and what version of the API it documents. Consider adding a comment row or companion metadata file to track CSV generation date and API version for maintainability.
css fixes Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {% for timeline_uid, timeline_instances in instances_by_timeline.items() %} | ||
| <div class="matrix-container" | ||
| data-timeline="{{ timeline_uid }}" | ||
| {% if timeline_uid == default_timeline %}style="display: block;"{% else %}style="display: none;"{% endif %}> |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline style attributes for show/hide logic can be difficult to maintain. Consider using CSS classes (e.g., .hidden { display: none; } and .visible { display: block; }) and toggling class names in JavaScript instead of manipulating style attributes directly.
| for inst in instances: | ||
| timeline_key = inst.get("member_of_timeline") or "unassigned" | ||
| if timeline_key not in instances_by_timeline: | ||
| instances_by_timeline[timeline_key] = [] |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This grouping logic could be simplified using collections.defaultdict(list) to eliminate the explicit key existence check and improve readability.
| > - Time values are ISO-8601 UTC. | ||
| > - All IDs are integers unless stated otherwise. | ||
| > - Errors use FastAPI default error model: `{"detail": "message"}`. | ||
| Complete documentation for all 165+ API and UI endpoints in the SoA Workbench application. |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The claim of "165+ endpoints" should be verified against the actual count in docs/api_endpoints.csv (which shows 157 rows including header). Update the count to match the actual number of documented endpoints for accuracy.
| Complete documentation for all 165+ API and UI endpoints in the SoA Workbench application. | |
| Complete documentation for all 156 API and UI endpoints in the SoA Workbench application. |
| 8. **Router imports**: Import routers at top of `app.py`, mount with `app.include_router()` | ||
|
|
||
| ## Reference Files | ||
| - **API endpoints catalog**: `docs/api_endpoints.csv` (165 endpoints: method, path, type, description, response format) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoint count (165) in the copilot instructions should match the count referenced in README_endpoints.md and the actual CSV file. Verify the accurate count and update consistently across all documentation files.
| - **API endpoints catalog**: `docs/api_endpoints.csv` (165 endpoints: method, path, type, description, response format) | |
| - **API endpoints catalog**: `docs/api_endpoints.csv` (API endpoints: method, path, type, description, response format) |
Additional unit tests for routers code
Timeline selector for displaying individual timelines in matrix
Added Biomedical Concept names to the martrix audit report