Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/python-app.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ jobs:

steps:
- uses: actions/checkout@v4
- name: Set up Python 3.10
- name: Set up Python 3.13
uses: actions/setup-python@v3
with:
python-version: "3.10"
python-version: "3.13"
- name: Install dependencies
run: |
python -m pip install --upgrade pip
Expand Down
13 changes: 13 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,25 +1,38 @@
annotated-doc==0.0.4
annotated-types==0.7.0
anyio==4.12.1
beautifulsoup4==4.14.3
certifi==2026.1.4
charset-normalizer==3.4.4
click==8.3.0
docraptor==3.1.0
dotenv==0.9.9
et_xmlfile==2.0.0
fastapi==0.128.5
fhir.resources==8.2.0
fhir_core==1.1.5
h11==0.16.0
httpcore==1.0.9
httpx==0.28.1
idna==3.11
Jinja2==3.1.6
numpy==2.4.2
openpyxl==3.1.5
pandas==3.0.0
pydantic==2.12.5
pydantic_core==2.41.5
python-dateutil==2.9.0.post0
python-dotenv==1.2.1
python-multipart==0.0.22
PyYAML==6.0.3
requests==2.32.5
six==1.17.0
soupsieve==2.8.3
starlette==0.52.1
stringcase==1.2.0
typing-inspection==0.4.2
typing_extensions==4.15.0
urllib3==2.6.3
usdm==0.66.0
uvicorn==0.38.0
yattag==1.16.1
70 changes: 40 additions & 30 deletions src/soa_builder/web/routers/timings.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import os
from typing import Optional

from pydantic import ValidationError

from fastapi import APIRouter, HTTPException, Request, Form
from fastapi.responses import JSONResponse, HTMLResponse, RedirectResponse
from fastapi.templating import Jinja2Templates
Expand Down Expand Up @@ -291,21 +293,25 @@ def ui_create_timing(
conn_c2.close()
except Exception:
rtf_code_uid = None
payload = TimingCreate(
name=name,
label=label,
description=description,
type=code_uid,
value=value,
value_label=value_label,
relative_to_from=rtf_code_uid,
relative_from_schedule_instance=relative_from_schedule_instance,
relative_to_schedule_instance=relative_to_schedule_instance,
window_label=window_label,
window_upper=window_upper,
window_lower=window_lower,
member_of_timeline=member_of_timeline,
)
try:
payload = TimingCreate(
name=name,
label=label,
description=description,
type=code_uid,
value=value,
value_label=value_label,
relative_to_from=rtf_code_uid,
relative_from_schedule_instance=relative_from_schedule_instance,
relative_to_schedule_instance=relative_to_schedule_instance,
window_label=window_label,
window_upper=window_upper,
window_lower=window_lower,
member_of_timeline=member_of_timeline,
)
except ValidationError as exc:
msgs = "; ".join(e["msg"] for e in exc.errors())
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI error message concatenates only msg strings, which can be ambiguous when multiple fields fail validation (it omits which field each message applies to). Consider including the Pydantic error loc (field path) alongside the message (e.g., value: ...; window_upper: ...) so users can identify what to fix.

Suggested change
msgs = "; ".join(e["msg"] for e in exc.errors())
error_messages = []
for e in exc.errors():
loc = e.get("loc") or ()
msg = e.get("msg", "")
if loc:
field_path = ".".join(str(part) for part in loc)
error_messages.append(f"{field_path}: {msg}")
else:
error_messages.append(msg)
msgs = "; ".join(error_messages)

Copilot uses AI. Check for mistakes.
raise HTTPException(400, f"Validation error: {msgs}")
create_timing(soa_id, payload)
return RedirectResponse(url=f"/ui/soa/{int(soa_id)}/timings", status_code=303)

Expand Down Expand Up @@ -623,21 +629,25 @@ def ui_update_timing(
# On any error, fall back to previous behavior (leave fields unchanged)
mapped_type = None if type_submission_value not in ("",) else ""
mapped_rtf = None if relative_to_from_submission_value not in ("",) else ""
payload = TimingUpdate(
name=name,
label=label,
description=description,
type=mapped_type,
value=value,
value_label=value_label,
relative_to_from=mapped_rtf,
relative_from_schedule_instance=relative_from_schedule_instance,
relative_to_schedule_instance=relative_to_schedule_instance,
window_label=window_label,
window_upper=window_upper,
window_lower=window_lower,
member_of_timeline=member_of_timeline,
)
try:
payload = TimingUpdate(
name=name,
label=label,
description=description,
type=mapped_type,
value=value,
value_label=value_label,
relative_to_from=mapped_rtf,
relative_from_schedule_instance=relative_from_schedule_instance,
relative_to_schedule_instance=relative_to_schedule_instance,
window_label=window_label,
window_upper=window_upper,
window_lower=window_lower,
member_of_timeline=member_of_timeline,
)
except ValidationError as exc:
msgs = "; ".join(e["msg"] for e in exc.errors())
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as UI create: the update path omits field locations from validation errors. Prefer including loc (and potentially the submitted value for duration fields) so the validation response is actionable.

Suggested change
msgs = "; ".join(e["msg"] for e in exc.errors())
# Include field locations in the validation error so clients can
# associate messages with specific inputs.
detailed_parts = []
for err in exc.errors():
loc = err.get("loc") or ()
msg = err.get("msg", "")
loc_str = ".".join(str(part) for part in loc) if loc else ""
if loc_str:
detailed_parts.append(f"{loc_str}: {msg}")
else:
detailed_parts.append(msg)
msgs = "; ".join(detailed_parts)

Copilot uses AI. Check for mistakes.
raise HTTPException(400, f"Validation error: {msgs}")
update_timing(soa_id, timing_id, payload)
return RedirectResponse(url=f"/ui/soa/{int(soa_id)}/timings", status_code=303)

Expand Down
70 changes: 69 additions & 1 deletion src/soa_builder/web/schemas.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,50 @@
import re
from typing import List, Optional

from pydantic import BaseModel
from pydantic import BaseModel, field_validator, model_validator

# ISO 8601 duration pattern supporting both standard (-P2D) and USDM (P-2D) conventions
_ISO8601_DURATION_RE = re.compile(
r"^-?P-?(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?(?:T(?=\d)(\d+H)?(\d+M)?(\d+S)?)?$"
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex as written allows a “double-negative” form like -P-2D (one - before P and another after P). The comment says you want to support either the standard -P2D or USDM P-2D conventions; accepting both signs simultaneously is likely unintended. Consider tightening the pattern (e.g., via a negative lookahead for -P- or an alternation that permits only one sign location).

Suggested change
r"^-?P-?(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?(?:T(?=\d)(\d+H)?(\d+M)?(\d+S)?)?$"
r"^(?:P|-P|P-)(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?(?:T(?=\d)(\d+H)?(\d+M)?(\d+S)?)?$"

Copilot uses AI. Check for mistakes.
)
Comment on lines +6 to +9
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex allows a double-negative form like -P-2D (leading - plus P-), which is neither standard ISO 8601 nor a clear USDM convention. Consider explicitly rejecting -P-... inputs (e.g., a simple pre-check) or refactoring the pattern into mutually exclusive alternatives so only -P... or P-... (but not both) can match.

Copilot uses AI. Check for mistakes.


def _validate_iso8601_duration(v: Optional[str]) -> Optional[str]:
if v is None:
return v
v = v.strip()
if not v:
return None
m = _ISO8601_DURATION_RE.match(v)
if not m or not any(m.group(i) is not None for i in range(1, 8)):
raise ValueError(
f"'{v}' is not a valid ISO 8601 duration (e.g. P1D, P2W, PT8H, -P2D)"
)
return v
Comment on lines +14 to +23
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning None for empty/whitespace strings collapses “explicitly cleared” and “not provided” into the same value. This is likely to break clearing existing values on update (e.g., UI text inputs typically submit "" when cleared), because downstream update logic often treats None as “leave unchanged”. Consider preserving the ability to clear fields by (a) using Pydantic’s model_fields_set to distinguish missing vs provided fields in update handling, and/or (b) normalizing empty strings consistently but still honoring explicit field presence in the update merge logic.

Suggested change
return v
v = v.strip()
if not v:
return None
m = _ISO8601_DURATION_RE.match(v)
if not m or not any(m.group(i) is not None for i in range(1, 8)):
raise ValueError(
f"'{v}' is not a valid ISO 8601 duration (e.g. P1D, P2W, PT8H, -P2D)"
)
return v
# Preserve distinction between "not provided" (None) and "explicitly cleared" (empty string)
return None
v_stripped = v.strip()
# If the value is empty or only whitespace, treat it as an explicit empty string rather than None
if not v_stripped:
return ""
m = _ISO8601_DURATION_RE.match(v_stripped)
if not m or not any(m.group(i) is not None for i in range(1, 8)):
raise ValueError(
f"'{v_stripped}' is not a valid ISO 8601 duration (e.g. P1D, P2W, PT8H, -P2D)"
)
# Return the normalized duration string (whitespace removed)
return v_stripped

Copilot uses AI. Check for mistakes.


def _validate_window_all_or_none(
window_lower: Optional[str],
window_upper: Optional[str],
window_label: Optional[str],
) -> None:
"""Enforce that window_lower, window_upper, and window_label are all provided or all absent."""

def _present(v: Optional[str]) -> bool:
return v is not None and v.strip() != ""

provided = [_present(window_lower), _present(window_upper), _present(window_label)]
if any(provided) and not all(provided):
missing = []
if not _present(window_lower):
missing.append("window_lower")
if not _present(window_upper):
missing.append("window_upper")
if not _present(window_label):
missing.append("window_label")
raise ValueError(
f"Window fields are all-or-nothing: missing {', '.join(missing)}"
)


class InstanceUpdate(BaseModel):
Expand Down Expand Up @@ -42,6 +86,18 @@ class TimingCreate(BaseModel):
window_lower: Optional[str] = None
member_of_timeline: Optional[str] = None

@field_validator("value", "window_lower", "window_upper", mode="before")
@classmethod
def check_iso8601_duration(cls, v: Optional[str]) -> Optional[str]:
return _validate_iso8601_duration(v)

@model_validator(mode="after")
def check_window_all_or_none(self) -> "TimingCreate":
_validate_window_all_or_none(
self.window_lower, self.window_upper, self.window_label
)
return self


class TimingUpdate(BaseModel):
name: str
Expand All @@ -58,6 +114,18 @@ class TimingUpdate(BaseModel):
window_lower: Optional[str] = None
member_of_timeline: Optional[str] = None

@field_validator("value", "window_lower", "window_upper", mode="before")
@classmethod
def check_iso8601_duration(cls, v: Optional[str]) -> Optional[str]:
return _validate_iso8601_duration(v)

@model_validator(mode="after")
def check_window_all_or_none(self) -> "TimingUpdate":
_validate_window_all_or_none(
self.window_lower, self.window_upper, self.window_label
)
return self


class ScheduleTimelineCreate(BaseModel):
name: str
Expand Down
4 changes: 2 additions & 2 deletions src/soa_builder/web/templates/instances.html
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ <h2>Scheduled Activity Instances for SoA {{ soa_id }}</h2>
<input name="description" placeholder="Description (Optional)" />
</div>
<div style="display: flex;flex-direction: column;gap:2px;">
<label><strong>Default Condition ID</strong></label>
<label><strong>Next Instance (default condition)</strong></label>
<select name="default_condition_uid">
<option value="">-- Select Activity Instance --</option>
{% for name, instance_uid in (instance_options or {}).items() %}
Expand Down Expand Up @@ -78,7 +78,7 @@ <h2>Scheduled Activity Instances for SoA {{ soa_id }}</h2>
<th style="text-align:left;padding:4px;">Name</th>
<th style="text-align:left;padding:4px;">Label</th>
<th style="text-align:left;padding:4px;">Description</th>
<th style="text-align:left;padding:4px;">Default Condition</th>
<th style="text-align:left;padding:4px;">Next Instance (default condition)</th>
<th style="text-align:left;padding:4px;">Epoch</th>
<th style="text-align:left;padding:4px;">Timeline ID</th>
<th style="text-align:left;padding:4px;">Timeline Exit ID</th>
Expand Down
Loading