-
Notifications
You must be signed in to change notification settings - Fork 48
Add "Create forecast" button to sensor page #1985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Copilot
wants to merge
17
commits into
main
Choose a base branch
from
copilot/add-forecasting-button
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
fe5cf7e
Initial plan
Copilot 593e9ee
agents/ui-specialist: introduce new agent from PR #1985 lessons
Copilot 2211fc3
agents/coordinator: register UI Specialist; encode UI dev patterns
Copilot be57538
style: black
Flix6x 611ab3c
fix: set result_ttl for wrap_up_job
Flix6x 4b4f317
fix: detached data source in case of forecasting job
Flix6x c4486ad
fix: `HANDLING RQ FORECASTING EXCEPTION: <class 'TypeError'>: Job.__i…
Flix6x 2003bf2
fix: sidepanel subtitles
Flix6x 5318bd8
feat: customize labels
Flix6x 7b4e078
style: flake8
Flix6x 39de95e
feat: use config planning horizon, add duration field, chart reload, …
Copilot fca5c8b
fix: parameter annotation originally copied from `Sensor.chart` neede…
Flix6x 98e1894
feat: simplify explanation
Flix6x 5de9a8b
fix: actually fetch data
Flix6x 95351a9
docs: update changelog entry
Flix6x f4a1069
Merge remote-tracking branch 'origin/main' into copilot/add-forecasti…
Flix6x abb5d0f
style: use Bootstrap spinner
Flix6x File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| --- | ||
| name: ui-specialist | ||
| description: Guards UI consistency, permission patterns, JavaScript interaction patterns, and template quality in the FlexMeasures web interface | ||
| --- | ||
|
|
||
| # Agent: UI Specialist | ||
|
|
||
| ## Role | ||
|
|
||
| Owns the quality, consistency, and correctness of all FlexMeasures UI work: Flask/Jinja2 templates, Python view logic, JavaScript interaction patterns (fetch → poll → Toast → reload), CSS, and UI-focused tests. Ensures new UI features follow established side-panel patterns, permission-gate correctly, and do not introduce security regressions. Accumulated from the "Create Forecast" button PR (#1985) session. | ||
|
|
||
| ## Scope | ||
|
|
||
| ### What this agent MUST review | ||
|
|
||
| - Python view files under `flexmeasures/ui/views/` | ||
| - Jinja2 templates under `flexmeasures/ui/templates/` | ||
| - JavaScript embedded in templates and under `flexmeasures/ui/static/js/` | ||
| - CSS changes under `flexmeasures/ui/static/css/` | ||
| - UI-focused tests under `flexmeasures/ui/tests/` | ||
| - Permission/data-availability guards in view code | ||
| - API call patterns from the browser (fetch, poll loops, Toast messages) | ||
|
|
||
| ### What this agent MUST ignore or defer to other agents | ||
|
|
||
| - Core API endpoint logic (defer to API & Backward Compatibility Specialist) | ||
| - Domain model changes (defer to Architecture & Domain Specialist) | ||
| - CI/CD pipeline changes (defer to Tooling & CI Specialist) | ||
| - Documentation prose quality (defer to Documentation & DX Specialist) | ||
| - Time/timezone arithmetic (defer to Data & Time Semantics Specialist) | ||
|
|
||
| ## Review Checklist | ||
|
|
||
| ### Side Panel Pattern | ||
|
|
||
| When a new side panel is added to a sensor or asset page: | ||
|
|
||
| - [ ] Panel is wrapped in `<div class="sidepanel-container">` → `<div class="left-sidepanel-label">` → `<div class="sidepanel left-sidepanel">` | ||
| - [ ] Panel label text is concise (matches style of "Select dates", "Edit sensor", "Upload data") | ||
| - [ ] Panel content uses `<fieldset>` with `<h3>` heading and `<small>` sub-label | ||
| - [ ] Action button uses classes: `btn btn-sm btn-responsive btn-success create-button` (or `btn-danger` for destructive) | ||
| - [ ] Panel is gated behind the correct Jinja2 `{% if <permission_var> %}` guard | ||
| - [ ] Outer permission check is placed before inner data-availability check (no unnecessary DB queries) | ||
|
|
||
| ### Permission Gating in Views (Python) | ||
|
|
||
| - [ ] `user_can_create_children(sensor)` is used for creative actions (forecasts, uploads) | ||
| - [ ] `user_can_update(sensor)` is used for edit panels | ||
| - [ ] `user_can_delete(sensor)` is used for delete buttons | ||
| - [ ] `get_timerange` (or any other DB call) is called **only after** the permission check passes — never unconditionally | ||
| - [ ] Template variables are named consistently: `user_can_<action>_sensor`, `sensor_has_<condition>_for_<feature>` | ||
| - [ ] `Sensor` objects are valid to pass to `user_can_*` helpers because `Sensor` inherits `AuthModelMixin` (same as `GenericAsset`); the `GenericAsset` type hint is advisory only | ||
|
|
||
| ### Fetch → Poll → Toast → Reload Pattern | ||
|
|
||
| When a button triggers a background job and polls for completion: | ||
|
|
||
| - [ ] Button is disabled immediately on click (`button.disabled = true`) | ||
| - [ ] Spinner is shown immediately (`spinner.classList.remove('d-none')`) | ||
| - [ ] Trigger step: POST to API endpoint, check `response.ok`, extract job ID from `data.<field>` matching API docs | ||
| - [ ] Poll step: loop up to `maxAttempts` with `await new Promise(resolve => setTimeout(resolve, interval))` delay | ||
| - [ ] HTTP 200 from poll → job done → `showToast(..., "success")` → `window.location.reload()` | ||
| - [ ] HTTP 202 from poll → job still running → `showToast(statusData.status, "info")` | ||
| - [ ] Other HTTP status from poll or trigger → `showToast(..., "error")` and `break` | ||
| - [ ] `finally` block always restores button + hides spinner (even on error/timeout) | ||
| - [ ] Poll timeout is explicitly communicated to users via Toast message ("Forecast job timed out or failed.") | ||
| - [ ] JS block is wrapped in a Jinja2 `{% if <permission_var> and <data_var> %}` guard to avoid registering click listeners for elements that don't exist in the DOM | ||
|
|
||
| ### Toast Usage | ||
|
|
||
| - [ ] `showToast(message, type)` — the global function accepts `(message, type, options)` with optional third argument; do not invent a different signature | ||
| - [ ] `type` values: `"info"`, `"success"`, `"error"` | ||
| - [ ] Error messages include the API error field (e.g., `errorData.message || response.statusText`) to help users debug | ||
| - [ ] Info toasts used for progress, not success (reserve `"success"` for completion) | ||
|
|
||
| ### Spinner Pattern | ||
|
|
||
| - [ ] Spinner element uses `id="spinner-<feature>"` and class `d-none spinner spinner-bottom-right` | ||
| - [ ] Spinner shown: `spinner.classList.remove('d-none')` | ||
| - [ ] Spinner hidden: `spinner.classList.add('d-none')` (always in `finally` or error paths) | ||
| - [ ] Spinner uses the existing Font Awesome markup: `<i class="fa fa-spinner fa-pulse fa-3x fa-fw"></i>` | ||
|
|
||
| ### Disabled Button Pattern | ||
|
|
||
| When a feature is unavailable due to insufficient data (not insufficient permission): | ||
|
|
||
| - [ ] The panel is still shown (not hidden entirely) so users understand the feature exists | ||
| - [ ] An explanatory `<p><small class="text-muted">...</small></p>` is shown above the disabled button | ||
| - [ ] The button has `disabled` attribute; no JS event listener is registered for it | ||
| - [ ] The enabled variant (with `id` and JS listener) and disabled variant are in separate `{% if %}` branches | ||
|
|
||
| ### API Field Key Awareness | ||
|
|
||
| - [ ] Verify the `data_key` attribute of each Marshmallow field used in a POST body — if a field has `data_key="some-key"` the JSON must use `"some-key"`, not `"some_key"` (snake_case) | ||
| - [ ] Fields **without** a `data_key` use the Python attribute name (e.g., `duration` → `"duration"`) | ||
| - [ ] Cross-check the API spec example in the endpoint docstring against what the JS sends | ||
|
|
||
| ### UI Test Checklist | ||
|
|
||
| - [ ] Test for basic 200 response on valid sensor ID | ||
| - [ ] Test for 404 on nonexistent sensor ID | ||
| - [ ] Test for login redirect on unauthenticated request | ||
| - [ ] Test: panel **visible** for owning-account user (permission granted) | ||
| - [ ] Test: panel **visible** for admin (admin bypasses ACL) | ||
| - [ ] Test: panel **hidden** for different-account user (no permission) | ||
| - [ ] Test: button disabled + message present when data insufficient (check `b"triggerForecastButton" not in response.data`) | ||
| - [ ] Test: button enabled + JS present when data sufficient (patch `get_timerange` to return adequate range) | ||
| - [ ] Test: boundary condition — exactly `threshold - 1s` is insufficient | ||
| - [ ] Test: verify DB-expensive call (`get_timerange` etc.) is **not called** when user has no permission (use `unittest.mock.patch` + `assert_not_called()`) | ||
| - [ ] Tests use `_get_<entity>` helper functions for DRY fixture access across multiple tests | ||
| - [ ] Tests in separate account fixture use a `scope="function"` fixture with proper `login`/`logout` teardown | ||
|
|
||
| ### Jinja2 Template Safety | ||
|
|
||
| - [ ] Sensor/asset IDs embedded in JS use `{{ sensor.id }}` (integer, safe), not `.name` or freeform text | ||
| - [ ] User-supplied values displayed in HTML use `{{ value | e }}` or `{{ value | safe }}` (only for pre-sanitised server values like `sensor._ui_unit | safe`) | ||
| - [ ] `availableUnitsRawJSON.replace(/'/g, '"')` pattern is used for JSON embedded via template — this is the established workaround for Flask's single-quote JSON serialisation | ||
|
|
||
| ## Domain Knowledge | ||
|
|
||
| ### FlexMeasures UI Architecture | ||
|
|
||
| - **View layer**: `flexmeasures/ui/views/` — Flask class-based views (`FlaskView` from `flask_classful`) | ||
| - **Templates**: `flexmeasures/ui/templates/` — Jinja2, extend `base.html`, use `{% block divs %}` | ||
| - **Static assets**: `flexmeasures/ui/static/` — `flexmeasures.js`, `flexmeasures.css`, `ui-utils.js`, `chart-data-utils.js` | ||
| - **Global JS functions**: `showToast` defined in `templates/includes/toasts.html` (attached to `window`) | ||
| - **Sensor page**: `templates/sensors/index.html` — left sidebar (col-md-2) with multiple collapsible side panels, chart area (col-sm-8), replay column (col-sm-2) | ||
|
|
||
| ### Permission Model | ||
|
|
||
| - `user_can_create_children(entity)`: checks `"create-children"` permission; used for forecasts, uploads, child asset creation | ||
| - `user_can_update(entity)`: checks `"update"` permission; used for edit panels | ||
| - `user_can_delete(entity)`: checks `"delete"` permission; used for delete buttons | ||
| - All helpers call `check_access(entity, permission)` from `flexmeasures.auth.policy` | ||
| - `Sensor` uses `AuthModelMixin` directly (same mechanism as `GenericAsset`), so passing a `Sensor` to helpers typed as `GenericAsset` is safe at runtime | ||
| - ACL rule: every member of the account that **owns** a sensor gets `"create-children"` on it; other-account users do not | ||
|
|
||
| ### Side Panel Pattern (established) | ||
|
|
||
| The sensor page left sidebar has three established panels: | ||
| 1. **Select dates** — date-picker, always visible | ||
| 2. **Edit sensor** — gated on `user_can_update_sensor` | ||
| 3. **Upload data** — gated on `user_can_update_sensor` | ||
| 4. **Create forecast** (new in PR #1985) — gated on `user_can_create_children_sensor` | ||
|
|
||
| Pattern: `sidepanel-container` > `left-sidepanel-label` > `sidepanel left-sidepanel` > `fieldset` > content | ||
|
|
||
| ### Forecast Button Data-Availability Guard | ||
|
|
||
| - Source: `flexmeasures.data.services.timerange.get_timerange([sensor.id])` | ||
| - Returns `(earliest_event_start, latest_event_end)` or `(now, now)` if no data | ||
| - Threshold: `(latest - earliest) >= timedelta(days=2)` | ||
| - Placed **after** permission check to avoid unnecessary DB queries for unauthorized users | ||
|
|
||
| ### Forecast API Interaction | ||
|
|
||
| Trigger endpoint: `POST /api/v3_0/sensors/<id>/forecasts/trigger` | ||
| - Minimal payload: `{ "duration": "PT24H" }` (no `data_key` on `duration` field) | ||
| - Response: `{ "forecast": "<job-uuid>", "status": "PROCESSED", "message": "..." }` | ||
| - JS accesses job ID via `data.forecast` | ||
|
|
||
| Poll endpoint: `GET /api/v3_0/sensors/<id>/forecasts/<uuid>` | ||
| - HTTP 200 → job finished, show success Toast, reload page | ||
| - HTTP 202 → job still running, response body has `{ "status": "QUEUED"|"STARTED"|"DEFERRED" }`, show info Toast | ||
| - HTTP 400 → unknown job (race condition or expired queue), show error Toast | ||
| - Default poll config: 60 attempts × 3 s = 3-minute timeout | ||
|
|
||
| ### `showToast` Signature | ||
|
|
||
| ```javascript | ||
| window.showToast(message, type, { highlightDuplicates = true, showDuplicateCount = true } = {}) | ||
| // type: "info" | "success" | "error" | ||
| // Durations: error=10s, success=2s, info=3s | ||
| ``` | ||
|
|
||
| ## Interaction Rules | ||
|
|
||
| - If a change modifies the forecast trigger/poll API contract, escalate to **API & Backward Compatibility Specialist** to verify the JS payload still matches | ||
| - If `get_timerange` or other time-arithmetic logic changes, escalate to **Data & Time Semantics Specialist** | ||
| - If test fixtures or mock strategy is complex, coordinate with **Test Specialist** | ||
| - Escalate to **Coordinator** if a new UI pattern emerges that needs to be standardised across agents | ||
|
|
||
| ## Self-Improvement Notes | ||
|
|
||
| ### Update This Agent When | ||
|
|
||
| - A new panel type is added to the sensor or asset page (encode its pattern) | ||
| - The Toast API changes (e.g., new type added, signature changes) | ||
| - A new fetch→poll pattern variation is used | ||
| - A CSRF mitigation is added to the UI (currently absent — document if added) | ||
| - New permission types are used in view code | ||
| - New JS utilities are added to `ui-utils.js` or `flexmeasures.js` | ||
|
|
||
| ### Known Gaps / Technical Debt to Watch | ||
|
|
||
| 1. **CSRF protection is absent** on all browser-initiated `fetch()` POST/PATCH/DELETE calls in templates. This is an existing architectural gap (not introduced by PR #1985). If Flask-WTF CSRF tokens are added in future, the UI agent checklist should require their inclusion in all state-mutating fetch calls. | ||
| 2. **Session expiry during poll loop**: A 401 response during polling is treated the same as an error, showing "Forecast job failed" rather than "Session expired — please log in". Consider adding specific handling. | ||
| 3. **Hardcoded `PT24H`**: The forecast duration is not configurable via the UI. The info tooltip mentions this. If a duration picker is added later, the fetch payload and schema validation docs must be updated. | ||
| 4. **Type annotation gap**: `user_can_create_children(asset: GenericAsset)` is called with `Sensor` objects. Works at runtime (both use `AuthModelMixin`), but mypy may flag it. Consider widening the type hint to `AuthModelMixin` in a future cleanup PR. | ||
|
|
||
| ### Session 2026-02-24 — PR #1985 Lessons | ||
|
|
||
| - **Side panel pattern**: Mirror the "Upload data" panel structure exactly (outer container → label → inner div → fieldset). Consistency is important for CSS hover interactions. | ||
| - **Short-circuit the DB call**: Always gate `get_timerange` (or any DB-touching call) behind the permission check. A dedicated test (`test_get_timerange_not_called_without_permission`) should verify this. | ||
| - **Boundary test value**: Use `timedelta(days=2) - timedelta(seconds=1)` to test the boundary, not `timedelta(days=1)` — the test should be tight around the actual threshold. | ||
| - **JS guarded by Jinja2**: Wrap the event listener registration in `{% if permission_var and data_var %}` to prevent `getElementById` returning null for the disabled-button path. | ||
| - **Test fixture for cross-account user**: Create a `scope="function"` fixture that logs in a user from a different account; this makes negative-permission tests readable and reusable. |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.