Consolidated study timing pages into one to support easier workflow#116
Merged
pendingintent merged 8 commits intomasterfrom Feb 23, 2026
Merged
Consolidated study timing pages into one to support easier workflow#116pendingintent merged 8 commits intomasterfrom
pendingintent merged 8 commits intomasterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates the previously separate “Schedule Timelines”, “Scheduled Activity Instances”, and “Timings” UI pages into a single “Study Timing” page, and embeds Scheduled Activity Instance CRUD into the main SoA edit workflow. Router redirects were updated to return users to the originating UI page after form actions.
Changes:
- Added a new consolidated
/ui/soa/{soa_id}/study_timingpage with collapsible sections for timelines, instances, and timings. - Refactored the three standalone timing-related pages to reuse new
_..._section.htmlpartial templates. - Updated UI create/update/delete redirects for timelines/instances/timings to return to the referring UI page when safe.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/soa_builder/web/templates/timings.html | Replaced full page with _timings_section.html include. |
| src/soa_builder/web/templates/study_timing.html | New consolidated UI page with collapsible sections + localStorage persistence. |
| src/soa_builder/web/templates/schedule_timelines.html | Replaced full page with _schedule_timelines_section.html include. |
| src/soa_builder/web/templates/instances.html | Replaced full page with _instances_section.html include. |
| src/soa_builder/web/templates/edit.html | Embedded instances CRUD section into the edit page and persisted its collapse state. |
| src/soa_builder/web/templates/base.html | Updated nav to point to the consolidated Study Timing page only. |
| src/soa_builder/web/templates/_timings_section.html | New timings partial extracted from prior timings page. |
| src/soa_builder/web/templates/_schedule_timelines_section.html | New schedule timelines partial extracted from prior schedule timelines page. |
| src/soa_builder/web/templates/_instances_section.html | New instances partial extracted from prior instances page. |
| src/soa_builder/web/routers/timings.py | Added same-origin referer-based redirect helper for UI actions. |
| src/soa_builder/web/routers/schedule_timelines.py | Added consolidated study_timing UI route + referer-based redirects for UI actions. |
| src/soa_builder/web/routers/instances.py | Added same-origin referer-based redirect helper for UI actions. |
| src/soa_builder/web/app.py | Added data needed to render the embedded instances CRUD section in edit page context. |
Comments suppressed due to low confidence (1)
src/soa_builder/web/templates/_instances_section.html:103
- HTML validity: placing a
<form>directly inside a<tr>is invalid markup (forms should wrap the table or be inside a<td>). Some browsers/assistive tech handle this inconsistently. Restructure the row so each form is contained within table cells (or wrap the entire row/table in a form) to avoid unexpected submission/layout issues.
<tr data-instance-id="{{ i.id }}">
<form method="post" action="/ui/soa/{{ soa_id }}/instances/{{ i.id }}/update">
<td style="padding:4px;white-space:nowrap;"><strong>{{ i.instance_uid }}</strong></td>
<td style="padding:4px;white-space:nowrap;"><strong>{{ i.order_index }}</strong></td>
<td style="padding:4px;"><input name="name" value="{{ i.name or '' }}" placeholder="Name" /></td>
<td style="padding:4px;"><input name="label" value="{{ i.label or '' }}" placeholder="Label" /></td>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…/soa-workbench into consolidate-timing
…ehavior and that all three sections render
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Comment on lines
+152
to
+164
|
|
||
| # Schedule timelines data | ||
| schedule_timelines = list_schedule_timelines(soa_id) | ||
| instance_options = get_scheduled_activity_instance(soa_id) | ||
|
|
||
| # Instances data | ||
| instances = list_instances(soa_id) | ||
| encounter_options = get_encounter_id(soa_id) | ||
| epoch_options = get_epoch_uid(soa_id) | ||
| schedule_timelines_options = get_schedule_timeline(soa_id) | ||
|
|
||
| # Timings data (with code_uid -> submission_value decoding) | ||
| timings = list_timings(soa_id) |
There was a problem hiding this comment.
This module calls list_instances() / list_timings() imported from other router modules to build the UI page. Those are route handlers (decorated endpoints), so reusing them as internal data access couples this page to HTTP-layer code and makes refactors risky. Prefer extracting the underlying DB/query logic into a shared function (or utils layer) and have both the API and UI routes call that.
Comment on lines
+62
to
+70
| def redirect_url_from_referer(request: Request, fallback: str) -> str: | ||
| """Return the Referer URL if it's a same-origin /ui/ path, else fallback.""" | ||
| referer = request.headers.get("referer", "") | ||
| if referer: | ||
| parsed = urlparse(referer) | ||
| base = urlparse(str(request.base_url)) | ||
| if parsed.netloc == base.netloc and parsed.path.startswith("/ui/"): | ||
| return urlunparse(("", "", parsed.path, "", parsed.query, parsed.fragment)) | ||
| return fallback |
There was a problem hiding this comment.
The new redirect_url_from_referer utility function lacks test coverage. Consider adding tests to verify:
- Redirects to referer when it's a same-origin /ui/ path
- Falls back to provided URL when referer is missing
- Falls back when referer is different origin
- Falls back when referer doesn't start with /ui/
- Preserves query parameters and fragments from referer
This would help prevent regressions and document the expected behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Study timing UI pages consolidated to support easier creation/management.
Added Scheduled Activity Instance create/update/delete to the edit.html to allow users to create Activities and Scheduled Activity Instances on the edit page to create a simple SOA matrix immediatel without the need to navigatet to other UI pages.