Conversation
…ng test for window values
…rkbench into ui-features Update
There was a problem hiding this comment.
Pull request overview
Updates timing validation and UI hints to reduce ambiguity around duration/window fields, aligning tests with ISO 8601-like durations (including USDM’s P-2D style).
Changes:
- Enforced ISO 8601 duration validation for
value,window_lower, andwindow_upperin Pydantic schemas. - Enforced an all-or-nothing rule for
window_lower/window_upper/window_labeland added extensive router test coverage. - Updated UI templates to guide users toward ISO 8601 duration inputs and clarified labels.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_timings.py | Updates expectations to ISO 8601 duration strings for timing values/windows. |
| tests/test_routers_timings.py | Adds validation test suite for duration formats and window all-or-none behavior. |
| src/soa_builder/web/templates/timings.html | Adds input patterns/tooltips and rearranges fields; adds client-side “window group” enforcement. |
| src/soa_builder/web/templates/instances.html | Clarifies “default condition” label copy in the instances UI. |
| src/soa_builder/web/schemas.py | Adds duration regex validation + window all-or-none model validation. |
| src/soa_builder/web/routers/timings.py | Converts Pydantic validation errors into HTTP 400 for UI endpoints. |
| .github/workflows/python-app.yml | Bumps CI Python version. |
| 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 |
There was a problem hiding this comment.
Returning None for blank strings breaks PATCH semantics in update_timing: when a client sends an empty string to clear value/window_lower/window_upper, the validator converts it to None, and the update logic interprets None as 'leave unchanged'. This makes these fields effectively non-clearable via UI/API. Consider preserving empty-string intent (e.g., return \"\" for whitespace-only inputs, or otherwise distinguish 'missing' vs 'provided but blank' for TimingUpdate).
| member_of_timeline=member_of_timeline, | ||
| ) | ||
| except ValidationError as exc: | ||
| msgs = "; ".join(e["msg"] for e in exc.errors()) |
There was a problem hiding this comment.
The UI error message drops field context (loc), so users won’t know which input is invalid when multiple validations fail. Consider including loc alongside msg (e.g., window_upper: <msg>) when building msgs from exc.errors().
| msgs = "; ".join(e["msg"] for e in exc.errors()) | |
| error_messages = [] | |
| for e in exc.errors(): | |
| loc = e.get("loc") | |
| if loc: | |
| loc_str = ".".join(str(part) for part in loc) | |
| error_messages.append(f"{loc_str}: {e.get('msg', '')}") | |
| else: | |
| error_messages.append(e.get("msg", "Unknown validation error")) | |
| msgs = "; ".join(error_messages) |
| <div style="display:flex;flex-direction:column;gap:2px;"> | ||
| <label><strong>Value</strong></label> | ||
| <input name="value" placeholder="Value (optional)" /> | ||
| <input name="value" placeholder="e.g. P7D (optional)" pattern="-?P-?(\d+Y)?(\d+M)?(\d+W)?(\d+D)?(T(\d+H)?(\d+M)?(\d+S)?)?" title="ISO 8601 duration, e.g. P1D, P2W, PT8H, -P2D" /> |
There was a problem hiding this comment.
The HTML pattern is more permissive than the server validator (e.g., it appears to accept bare P and PT because all components are optional), which will allow users to submit values that only fail after server-side validation. Consider tightening the pattern to align with the backend regex/logic (at minimum, require at least one numeric component and require a digit after T if T is present).
| {% extends 'base.html' %} | ||
| {% block content %} | ||
| <h2>Timings for SoA {{ soa_id }}</h2> | ||
| <h2 title="Timing definitions should be read as 'the <Timing.relativeFromScheduledInstance> node is <Timing.value> <Timing.type of before or after> the <Timing.relativeToScheduledInstance> node.'">Timings for SoA {{ soa_id }}</h2> |
There was a problem hiding this comment.
Relying on a long title tooltip for core explanatory content is not very accessible (many screen readers don’t announce it consistently, and it isn’t keyboard discoverable). Consider rendering this help text visibly (or via an explicit help element with aria-describedby) rather than only in title.
| <h2 title="Timing definitions should be read as 'the <Timing.relativeFromScheduledInstance> node is <Timing.value> <Timing.type of before or after> the <Timing.relativeToScheduledInstance> node.'">Timings for SoA {{ soa_id }}</h2> | |
| <h2 aria-describedby="timings-help">Timings for SoA {{ soa_id }}</h2> | |
| <p id="timings-help" style="font-size:0.9em;color:#555;max-width:60rem;"> | |
| Timing definitions should be read as “the <Timing.relativeFromScheduledInstance> node is <Timing.value> <Timing.type of before or after> the <Timing.relativeToScheduledInstance> node.” | |
| </p> |
No description provided.