Skip to content

Conversation

@pendingintent
Copy link
Owner

Additional unit tests for routers code
Timeline selector for displaying individual timelines in matrix
Added Biomedical Concept names to the martrix audit report

Copilot AI review requested due to automatic review settings January 20, 2026 19:40
@pendingintent pendingintent self-assigned this Jan 20, 2026
@pendingintent pendingintent added the enhancement New feature or request label Jan 20, 2026
@pendingintent pendingintent added this to the v1.2-beta milestone Jan 20, 2026
Copy link
Contributor

Copilot AI left a 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>
Copilot AI review requested due to automatic review settings January 20, 2026 19:43
Copy link
Contributor

Copilot AI left a 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.

Comment on lines +3672 to +3678
# 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)
Copy link

Copilot AI Jan 20, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +340 to +354
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);
Copy link

Copilot AI Jan 20, 2026

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.

Suggested change
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 {

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +26
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)"
)
Copy link

Copilot AI Jan 20, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +2
Method,Path,Type,Description,Response Type
GET,/,UI,Index page - lists studies & create form,HTML
Copy link

Copilot AI Jan 20, 2026

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.

Copilot uses AI. Check for mistakes.
css fixes

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings January 20, 2026 19:52
Copy link
Contributor

Copilot AI left a 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 %}>
Copy link

Copilot AI Jan 20, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +3674 to +3677
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] = []
Copy link

Copilot AI Jan 20, 2026

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.

Copilot uses AI. Check for mistakes.
> - 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.
Copy link

Copilot AI Jan 20, 2026

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.

Suggested change
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.

Copilot uses AI. Check for mistakes.
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)
Copy link

Copilot AI Jan 20, 2026

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.

Suggested change
- **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)

Copilot uses AI. Check for mistakes.
@pendingintent pendingintent merged commit b072617 into master Jan 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants