Conversation
…ng test for window values
…rkbench into ui-features Update
There was a problem hiding this comment.
Pull request overview
This PR introduces ISO 8601 duration validation for timing fields (value/window bounds), enforces an “all-or-nothing” rule for window fields, updates the UI to guide correct inputs, expands test coverage, and bumps CI to Python 3.13.
Changes:
- Add Pydantic validation for ISO 8601 duration strings and enforce window field grouping rules.
- Update timings UI to include ISO 8601 input patterns and client-side window-group validation.
- Expand router tests for ISO 8601 validation and window all-or-none behavior; update CI Python version.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_timings.py | Updates assertions to expect ISO 8601 durations for value/window fields. |
| tests/test_routers_timings.py | Adds extensive API/UI tests for duration validation and window all-or-none rules. |
| src/soa_builder/web/templates/timings.html | Updates form/table layout, adds HTML patterns + JS validation for window fields. |
| src/soa_builder/web/templates/instances.html | Renames “Default Condition” label/column to clarify meaning in UI. |
| src/soa_builder/web/schemas.py | Adds ISO 8601 duration validation + window group model validation via Pydantic. |
| src/soa_builder/web/routers/timings.py | Adds UI-layer handling of Pydantic ValidationError to return HTTP 400. |
| .github/workflows/python-app.yml | Changes CI to run on Python 3.13. |
| # 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)?)?$" | ||
| ) |
There was a problem hiding this comment.
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.
| <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 looser than the backend validator: it will accept invalid values like bare P and PT (since all components after P/T are optional). This creates a UI/backend mismatch where the browser allows input that the API rejects. Align the pattern with the backend rules by (1) requiring at least one duration component, and (2) requiring at least one time component when T is present (also consider anchoring with ^...$ for clarity). Apply the same tightened pattern to window_lower/window_upper inputs too.
| <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" /> | |
| <input name="value" placeholder="e.g. P7D (optional)" pattern="^-?P(?=(?:[^T]*\d+[YMWD])|(?:.*T(?:\d+H|\d+M|\d+S)))(?:\d+Y)?(?:\d+M)?(?:\d+W)?(?:\d+D)?(?:T(?:(?:\d+H(?:\d+M)?(?:\d+S)?)|(?:\d+M(?:\d+S)?)|(?:\d+S)))?$" title="ISO 8601 duration, e.g. P1D, P2W, PT8H, -P2D" /> |
| </tr> | ||
| {% else %} | ||
| <tr><td colspan="12" style="padding:6px;color:#777;">No timings yet.</td></tr> | ||
| <tr><td colspan="16" style="padding:6px;color:#777;">No timings yet.</td></tr> |
There was a problem hiding this comment.
The empty-state row uses colspan=\"16\", but the header row appears to define 15 columns (Name, Label, Description, Rel From Instance, Value, Value Label, Type, Rel To Instance, Window Lower, Window Upper, Window Label, Member of Timeline, Rel To/From, Save, Delete). This mismatch can break table layout; update colspan to match the actual column count.
| 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 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.
| 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) |
| - uses: actions/checkout@v4 | ||
| - name: Set up Python 3.10 | ||
| - name: Set up Python 3.13 | ||
| uses: actions/setup-python@v3 |
There was a problem hiding this comment.
CI is now targeting Python 3.13, but the workflow still uses actions/setup-python@v3, which is quite old. Update to a newer major (commonly @v5) to ensure best support for recent Python versions, security updates, and improved caching behavior.
| uses: actions/setup-python@v3 | |
| uses: actions/setup-python@v5 |
| 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 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.
| 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 |
|
|
||
| # 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)?)?$" |
There was a problem hiding this comment.
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).
| 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)?)?$" |
| </tr> | ||
| {% else %} | ||
| <tr><td colspan="12" style="padding:6px;color:#777;">No timings yet.</td></tr> | ||
| <tr><td colspan="16" style="padding:6px;color:#777;">No timings yet.</td></tr> |
There was a problem hiding this comment.
The table currently has 15 columns (based on the <th> headers), but the “No timings yet.” row uses colspan="16", which will render misaligned in many browsers. Update the colspan to match the actual number of visible columns.
| def test_window_accepts_valid_iso8601_durations(): | ||
| """Test that various valid ISO 8601 durations are accepted.""" | ||
| r = client.post("/soa", json={"name": "Window Valid ISO"}) | ||
| soa_id = r.json()["id"] | ||
|
|
||
| valid_durations = [ | ||
| ("P1D", "P1D"), | ||
| ("P2W", "P2W"), | ||
| ("PT8H", "PT8H"), | ||
| ("-P2D", "-P2D"), | ||
| ("P-2D", "P-2D"), | ||
| ("P1Y2M3D", "P1Y2M3D"), | ||
| ("PT1H30M", "PT1H30M"), | ||
| ] | ||
| for i, (lower, upper) in enumerate(valid_durations): | ||
| timing_data = { | ||
| "name": f"Valid Duration {i}", | ||
| "window_lower": lower, | ||
| "window_upper": upper, | ||
| "window_label": f"Window {i}", | ||
| } | ||
| resp = client.post(f"/soa/{soa_id}/timings", json=timing_data) | ||
| assert resp.status_code == 201, f"Failed for duration: {lower}/{upper}" | ||
| data = resp.json() | ||
| assert data["window_lower"] == lower | ||
| assert data["window_upper"] == upper |
There was a problem hiding this comment.
There’s a lot of repetition in the new validation tests (create SOA → POST/PATCH timing → assert status). Consider converting these into parametrized pytest tests (and/or using a shared fixture for soa_id) to reduce duplication and make it easier to extend the accepted/rejected duration matrices.
| 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.
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.
| 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) |
Manually creating