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
6 changes: 6 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ et_xmlfile==2.0.0
fhir.resources==8.2.0
fhir_core==1.1.5
idna==3.11
iniconfig==2.3.0
Jinja2==3.1.6
MarkupSafe==3.0.3
mccabe==0.7.0
mypy_extensions==1.1.0
nodeenv==1.9.1
numpy==2.4.2
openpyxl==3.1.5
pandas==3.0.0
Expand Down
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 6, 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 e["msg"] and drops which field failed (loc). If multiple fields fail validation, users won’t know which input to fix. Consider including the field path (e.g., derived from e["loc"]) in the message, or returning structured error details that the template can render next to the offending field(s).

Suggested change
msgs = "; ".join(e["msg"] for e in exc.errors())
# Include field path (loc) in error messages so users know which input failed
error_messages = []
for e in exc.errors():
loc = e.get("loc")
if loc:
# Pydantic typically includes "body" as the first element; skip it for UI friendliness
if isinstance(loc, (list, tuple)):
loc_parts = [str(part) for part in loc if part != "body"]
field_path = ".".join(loc_parts) if loc_parts else ""
else:
field_path = str(loc)
if field_path:
error_messages.append(f"{field_path}: {e.get('msg')}")
else:
error_messages.append(e.get("msg"))
else:
error_messages.append(e.get("msg"))
msgs = "; ".join(m for m in error_messages if m)

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())
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+)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.

This regex will accept strings like P1DT (a trailing T with no time components) because the time components after T are all optional. If you intend strict ISO-8601 duration validation, require at least one time component when T is present (e.g., ensure T is followed by \d+H or \d+M or \d+S). If you don’t intend strict ISO, consider updating the comment/error text to reflect the accepted superset.

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

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

The pattern ^-?P-? allows a double-negative prefix like -P-2D, which is likely unintended (it’s neither standard ISO -P2D nor the USDM-style P-2D). Consider changing the regex to allow exactly one sign position (either before P or immediately after P), but not both.

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


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)
Comment on lines +89 to +92
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

The PR description mentions “all-or-nothing validation of Timing window values”, but backend validation currently only checks ISO formatting per-field. As a result, API callers can submit any subset of window_lower/window_upper/window_label and it will pass schema validation (the UI JS can be bypassed). Add a backend validator (e.g., a model-level validator) to enforce that either all three window fields are provided (non-empty after stripping) or none are provided, for both TimingCreate and TimingUpdate.

Copilot uses AI. Check for mistakes.

@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