Repository navigation
Cover much more of the UI's JavaScript with tests - #2574
Merged
Merged
Conversation
Test the chart data cache and fetch, the replay, date range, data and chart data helpers, the page-free helpers in ui-utils.js, and the classic scripts flexmeasures.js and map-init.js. Move the asset tree's layout out of _macros.html into asset-tree.js, and the flex-context editor's logic out of assets/asset_context.html into flex-context-utils.js, so that they can be tested too. Both templates are touched by no open PR. Fix what the new tests found: - embed the asset tree, the sensors list and the flex-context on the context page with tojson, so a quote in a value no longer stops the flex-context editor from loading, and names are shown as text - import getSensor on the context page, so new commitment prices follow a price sensor's currency - highlight the current asset, and recognise the "Add asset" node, by id rather than by name - keep the plus sign of a UTC offset in encodeUrlQuery - translate Python representations properly in processResourceRawJSON, and accept objects Closes #2452 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <felix@seita.nl>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <felix@seita.nl>
Documentation build overview
4 files changed± changelog.html± genindex.html± _autosummary/flexmeasures.data.models.time_series.html± api/v3_0.html |
…page Embed data in scripts with tojson rather than as Python or JSON strings marked safe: the available units, the asset's flex-model on its properties page, and the chart options, whose download file name carries the asset's name. Escape names, units, graph titles and flex-model values wherever they are built into markup, with a shared escapeHtml in ui-utils.js, and drop Sensor._ui_unit, which handed the unit to templates as markup. Correct the UI guidelines, which recommended both patterns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <felix@seita.nl>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are cohesive (test expansion + targeted refactors), include tests for the new modules and bugfixes, and the template safety adjustments are consistently applied in the touched areas.
Review effort: Lite
Findings: None
What changed in this PR
This PR substantially expands test coverage for the UI’s JavaScript (pytest-driven, headless-browser execution) and refactors previously inline template logic into importable JS modules so it can be unit-tested. It also includes several UI robustness/security fixes around embedding server data into scripts and rendering user-supplied strings as markup.
Changes:
- Add extensive JS test suite coverage for UI helpers, chart data logic, replay/date-range utilities, and classic scripts (
flexmeasures.js,map-init.js). - Refactor template-embedded JS into new modules (
asset-tree.js,flex-context-utils.js) and update templates to import/consume these modules. - Harden UI rendering and script embedding by using
tojson/HTML-safe JSON dumps and escaping user-controlled strings before inserting intoinnerHTML.
| File | Description |
|---|---|
| flexmeasures/ui/views/assets/views.py | Stop passing pre-serialized JSON/markup for flex-model and sensor unit; pass raw data for safer tojson usage in templates |
| flexmeasures/ui/utils/view_utils.py | Use HTML-safe JSON dumping for chart_options embedded into scripts (prevents script-breakout via asset names) |
| flexmeasures/ui/static/js/ui-utils.js | Add escapeHtml, unitHtml, robust pythonReprToJSON, and safer processResourceRawJSON; escape user-supplied values rendered via innerHTML |
| flexmeasures/ui/static/js/flex-context-utils.js | New testable module containing flex-context editor decision logic (field kinds, cleaning, currency inference, commitment parsing) |
| flexmeasures/ui/static/js/asset-tree.js | New testable module containing asset tree hierarchy + layout option construction for ECharts |
| flexmeasures/ui/static/js/daterange-utils.js | Fix encodeUrlQuery to preserve literal + (UTC offsets) when using URLSearchParams |
| flexmeasures/ui/templates/_macros.html | Move asset tree layout logic out of template into asset-tree.js; switch to tojson; highlight by asset id |
| flexmeasures/ui/templates/assets/asset_context.html | Embed sensors/flex-context safely with tojson; render sensor names/values via textContent; import flex-context utilities |
| flexmeasures/ui/templates/assets/asset_properties.html | Replace unsafe JSON parsing pattern with tojson; pass flex-model object; escape unit option HTML |
| flexmeasures/ui/templates/assets/asset_graph.html | Escape user-supplied names/labels; use unitHtml; avoid inserting raw values into markup |
| flexmeasures/ui/templates/includes/graphs.html | Escape KPI titles/values/units used in HTML templates |
| flexmeasures/ui/templates/sensors/index.html | Remove `_ui_unit |
| flexmeasures/data/models/time_series.py | Remove Sensor._ui_unit (UI now handles dimensionless rendering safely) |
| flexmeasures/ui/tests/js/* | Add many new JS test modules covering the refactored/new JS code paths |
| flexmeasures/ui/tests/js/README.md | Document how classic scripts are tested and when logic must be moved into modules |
| documentation/changelog.rst | Add changelog items for expanded JS testing and multiple UI bug/security fixes |
| .github/instructions/ui-terminology.instructions.md | Update UI/template safety guidance to prefer tojson and escaping/textContent patterns |
| .github/agents/ui-specialist.md | Align UI specialist checklist with the updated template safety guidance |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Flix6x
commented
Sep 21, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: F.N. Claessen <felix@seita.nl>
Contributor
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
BelhsanHmida
added a commit
that referenced
this pull request
Sep 22, 2026
This branch added a module-local escapeHtml for the copy toast, and #2574 added an exported one to the same module. The merge kept both, so ui-utils.js declared the name twice and stopped parsing, taking every JavaScript test that imports it down with it: 17 failures, red on all three Python versions. This drops the copy added here and leaves main's exported one in its place, which escapes quotes as well and is the stricter of the two. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Flix6x
added a commit
that referenced
this pull request
Sep 25, 2026
* data/services: copy an asset subtree's automations, left inactive Context: - Issue #2528: copying an asset kept its sensor structure but dropped the recurring forecasts, schedules and reports that make it operational. Change: - Added `copy_automations`, which copies every automation on a copied asset onto the corresponding new asset, keeping its name, type, cron expression and timezone, but starting it inactive and with a fresh cursor, so it inherits neither the original's run history nor its queued jobs. - References to sensors, assets, organisations and data sources are remapped by walking the marshmallow schema that describes the data generator's configuration and parameters, so the same code serves automation types and plugin generators beyond forecasts. - A reference outside the copied subtree is kept only where the destination organisation may read it; otherwise the automation is skipped, as it is when its data generator is unavailable or the remapped configuration no longer validates. - Each automation is copied inside its own savepoint, so a skipped one leaves behind neither a partial automation nor a stray generator data source, and the asset copy itself goes ahead regardless. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * api/v3_0: copy an asset's automations and report the ones left out Context: - Issue #2528: the copy endpoint duplicated assets and sensors only, so a copied site arrived without the automations that made it run. Change: - `copy_asset` now tracks the new asset IDs alongside the new sensor IDs, hands both to `copy_automations`, and returns an `AssetCopy` naming the copied asset and the automations that were skipped. - `POST /assets/<id>/copy` reports those under a new `skipped_automations` field, each with its id, name, asset and reason, and says so in the message; the audit log record on the copy names them too. - Adapted the existing copy_asset call sites in the asset API tests to the new return value. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * ui: say which automations an asset copy left out Context: - Issue #2528: a copy that silently drops an automation is only noticed when a forecast never arrives. Change: - Added `reportSkippedAutomations`, which turns the copy response's `skipped_automations` into a toast naming each automation and its reason. - Both copy flows (the buttons on the asset page and the "Copy existing" tab) call it, and wait longer before following the copy when there is something to read. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * tests: cover copying an asset's automations Context: - Issue #2528 asks for coverage of direct and descendant automations, internal and external sensor references, generator isolation, the inactive state and cross-organisation copying. Change: - Added an asset-copy test module covering all of those, plus an automation whose data generator this instance does not know, and the API response that reports what was skipped. - Added JavaScript tests for the toast that reports skipped automations. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs: note that copying an asset brings its automations along Context: - Issue #2528 changes what a user gets when they copy an asset. Change: - The asset views page now says that the automations come along switched off, and that an unsafe one is left out and reported. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs/api: log the copy endpoint's automation behaviour Context: - Issue #2528 changes what POST /assets/<id>/copy does and what it returns. Change: - Added an API change log entry (v3.0-35) describing the copied automations and the new skipped_automations response field. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * changelog: copying an asset copies its automations Context: - Issue #2528, PR #2531. Change: - Added a New features entry for the automations that now come along with an asset copy, switched off, and for the skipped ones being reported. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * ui: escape the automation names reported after an asset copy Context: - Review of PR #2531: showToast assigns its message to innerHTML, and an automation name is whatever a user typed, so a name carrying markup ran in the browser of whoever copied the asset. Change: - Escape the name and reason taken from the copy response before building the toast message. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * tests: cover escaping of reported automation names Context: - Review of PR #2531 found markup in an automation name reaching innerHTML. Change: - Added a JavaScript test feeding an img/onerror payload through reportSkippedAutomations. Without the escaping it fails by actually firing the alert in the headless browser. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs: break docstrings after punctuation, and finish a sentence Context: - Review of PR #2531. Change: - Rewrapped three docstring lines that broke mid-phrase, against the repo's line-break-after-punctuation rule. - Completed 'that the receiving organisation cannot' to 'cannot read' in the asset views documentation. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/services: keep recognising forecast automations across the type rename Context: - Review of PR #2531 pointed at the automation type name. On main the type is 'forecasts', but PR #2294 (CRUD for automations) renames it to 'forecasting', after the queue and job names, with a migration that rewrites existing rows. - Keying the output-scope check on 'forecasts' alone is correct today and becomes silently dead the moment #2294 lands: copied forecast automations would stop being checked, with nothing failing to say so. Change: - Branch on FORECAST_AUTOMATION_TYPES, holding both spellings, with a note to drop the old one once this sits on top of #2294. - Added a test asserting at least one of the spellings is still a supported type, so a further rename fails loudly instead of disabling the check. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * Check a copied automation's output scope with the renamed check #2297 renamed validate_forecast_output_scope to validate_automation_output_scope, which also takes the automation type, so after merging main this module failed to import, without any conflict to show for it. It now uses the renamed check, and applies it to report automations too, whose output sensors have to sit in their asset's subtree as a forecast's do. With #2294 on main, "forecasts" is no longer a type name, so the constant that listed both spellings, as its comment said it would, lists the output-scoped types instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Name the copy endpoint's skipped automations in kebab-case New wire field names are kebab-case since #2547, and skipped_automations has not been released yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * ui: drop the duplicate escapeHtml the merge with main left behind This branch added a module-local escapeHtml for the copy toast, and #2574 added an exported one to the same module. The merge kept both, so ui-utils.js declared the name twice and stopped parsing, taking every JavaScript test that imports it down with it: 17 failures, red on all three Python versions. This drops the copy added here and leaves main's exported one in its place, which escapes quotes as well and is the stricter of the two. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/services: skip an automation with malformed stored references, rather than fail the copy Review of PR #2531 pointed out that the remapper read every reference with a bare int(value), and assumed that list and nested fields really held a list or an object. Stored configuration and parameters are JSON that a schema wrote but that nothing re-checks on the way out, so a malformed one raised TypeError or ValidationError. Neither is AutomationNotCopyable, so it escaped the per-automation savepoint and failed the entire asset copy, which is the opposite of the failure policy this work is built around. Both paths were reachable: parameters reach the remapper unvalidated, and a stored config that no longer validates raises out of the data generator before the remapper is reached at all. Every reference now goes through _reference_id, and container shape is checked with _require_stored_type, both raising AutomationNotCopyable naming the offending value. Setting up the data generator now catches ValidationError as well as the NotImplementedError it already caught. A malformed automation is reported and skipped, and the rest of the copy goes through. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * tests: cover malformed stored references in a copied automation Two tests for the guards added alongside them: one where the parameters hold a list where a sensor ID belongs, and one where the stored generator configuration no longer validates. Each asserts that the automation is skipped with a reason while the rest of the copy goes through. Both fail by aborting the whole copy when the guards are reverted. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * List the copied automations with the rest of the automations work Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Copy the automations that can be copied, and skip the rest for a reason of their own A forecast automation timed the way automations are timed, with offsets, was skipped: its stored parameters were checked against the schema its data generator uses at run time, which never sees those offsets. A report automation was skipped for the mirror image of that, as a reporter requires the window each run resolves. Both are now checked without what a run resolves, with a stand-in window where the schema requires one, so both copy. A schedule automation is skipped on purpose, and says so: its parameters are a trigger message whose flex config holds sensor references in fields no schema walks, so a copy would keep computing with the original's sensors. It used to reach the scheduler's constructor and raise a TypeError, which failed the whole asset copy rather than skipping one automation. Anything else a stored data generator does on the way in is skipped the same way now, rather than taken out on the copy. A copy that lands in another organisation records under a data source of that organisation, where it recorded under one of the organisation it came from, which no source filter of its own would find. A reference kept as it is has to be readable by the user making the copy, not only by their organisation: a consultancy reads its client's data through users holding the consultant role. Each skipped automation gets an audit record of its own, as one record is truncated to 500 characters and the reasons are the point of recording them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Keep the toast up as long as the page stays, and the traceback of a copy that failed The toast reporting the skipped automations lasted ten seconds while the page moved on after eight, so the reasons were cut short; both now read the same constant. Re-raising a copy's failure kept its traceback, and a skipped automation is logged with one, now that anything a stored data generator does is skipped rather than raised. Also fixes a typo and a comment that wrapped mid-phrase. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Break the copied automations' comments and docstrings after punctuation Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> --------- Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> Signed-off-by: Mohamed Belhsan Hmida <149331360+BelhsanHmida@users.noreply.github.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
#2435 made the UI's JavaScript testable. This puts that to use: the JavaScript tests go from 32 to 102, and two pieces of logic move out of templates into modules so they can be tested too.
ui/tests/js: cover the chart data cache and fetch, the replay, date range, data and chart data helpers, the page-free helpers inui-utils.js, and the classic scriptsflexmeasures.jsandmap-init.jsui/static: move the asset tree's layout from_macros.htmlintoasset-tree.js, and the flex-context editor's logic fromassets/asset_context.htmlintoflex-context-utils.js, and cover bothui: fix the bugs the new tests found (below)ui: embed data in scripts withtojson, and escape user-supplied text built into markup, on every asset and sensor pagedocumentation/changelog.rstCloses #2452
What is now covered
chart-data-cache.jsmissingRanges,effectiveResolutionMs,onSameResamplingGrid,clipToWindow,dedupeRecords, andcreateChartDataCacheagainst a stand-infetch(stepping, narrowing, widening, adjacent windows, expiry, off-grid windows)chart-data-fetch.jsreplay-utils.jsdaterange-utils.jsthisMonth,lastNMonths,getOffsetBetweenTimezonesForDate,computeSimulationRanges(incl. across DST),encodeUrlQuerydata-utils.jsconvertToCSV(timestamps, horizons, nested objects)chart-data-utils.jscheckDSTTransitions,checkSourceMasking,checkStrictYAxisRangesui-utils.jsmoveArrayItem,flattenErrorPayload,extractApiErrorMessage,createReactiveState,convertHtmlToElement,processResourceRawJSON, the cachedgetAsset/getSensor/getAccount,pollJobStatusflexmeasures.jsnumberWithCommas, currency symbols,getTimeAgo,getHumanFriendlyDeltaOrTimeStr(past moments),humanizeIsoDuration,unpackData,getLatestBeliefName,sourceIdFromKey, thetimezoneFormatVega formattermap-init.jscomputeCenteredTreeLayoutasset-tree.js(new)flex-context-utils.js(new)flexmeasures.jsandmap-init.jsare classic scripts, so their tests put stand-ins for jQuery, Vega or Leaflet onwindowand load them with a<script>element. The README says how.Refactoring kept away from open PRs
The two templates refactored here are touched by no open PR. The inline code moved nearly verbatim; the templates keep the DOM and ECharts wiring, and import the rest.
Everything else only gains tests, apart from two contained fixes in
daterange-utils.jsandui-utils.js, whose hunks are far from those of #2531 and #2554. The new test file forui-utils.jsis calledtest_ui_utils_helpers.py, because #2531 adds atest_ui_utils.py. There are no tests of howgetHumanFriendlyDeltaOrTimeStrphrases future moments, because #2554 rephrases them.Bugs found
The flex-context editor did not load when a flex-context value held a double quote, such as a commitment named
Big "flex" deal. The template embedded the flex-context as its Python representation inside a JavaScript string literal, which the quote ended early. It now embeds it withtojson. The tree and the sensors list on the same page are now embedded withtojsontoo, instead of as Python representations, and the sensors modal shows sensor names as text.New commitment prices always defaulted to EUR when the flex-context priced through sensors.
activeCurrency()looked up the price sensor's unit withgetSensor, which the page never imported. TheReferenceErrorwas caught by thetryaround the lookup, so the page fell back to EUR without a word. Checked in a browser against the rendered page: with aUSD/MWhprice sensor,mainoffers0 EUR/MWh, and this branch0 USD/MWh.The asset tree highlighted assets by name. A sub-asset named like the asset whose page it is was highlighted too. Asset names are only unique among siblings, so that can happen. An asset named "Add asset" was also drawn as the green button. Both are now decided by id.
encodeUrlQueryturned the+of a UTC offset into a space, contrary to its docstring:URLSearchParamsreads a literal+as an encoded space. Nothing visible broke, becauseAwareDateTimeFieldturns spaces back into plus signs on the server, but the function now does what it says.processResourceRawJSONmangled values. It turned Python's representation of a dict into JSON with a blanket replace.'Nonetheless'became'nulltheless', and an apostrophe in a value madeJSON.parsethrow. It now translates the representation properly, and also accepts an already-parsed object, whichasset_context.htmlnow passes.Text typed by users was embedded or rendered as markup elsewhere too. The same fixes now apply on the other asset and sensor pages.
available_unitsand the properties page's flex-model are embedded withtojson, where they were a Python or JSON string markedsafeinside a string literal.chart_options, whose download file name carries the asset's name, is now dumped HTML-safe. Names, units, graph titles and flex-model values built into markup go through a newescapeHtmlinui-utils.js, inrenderSensorandrenderSensorSearchResults, in the sensor cards and plot headers ofasset_graph.html, and in the KPI cards ofgraphs.html. The sensor page renders the "dimensionless" hint in the template, soSensor._ui_unitis gone. It was private and handed the unit to templates as markup.These are one-line changes in templates that open PRs also touch (#2490, #2267, #2570), so they may need a trivial rebase there.
The UI guidelines in
.github/recommended both patterns,sensor._ui_unit | safeand parsingavailable_units | safeafter swapping quotes. They now ask fortojsonin scripts, andtextContentorescapeHtmlin JavaScript.I checked each page in headless Chrome, rendered by the app with names, a unit, a graph title and flex-context and flex-model values seeded with quotes and markup. On
main, those values stop the scripts on the context and graphs pages, and are rendered as markup on all four pages. On this branch all four pages show them as typed, and none of them logs a new console error.Potential follow-ups
Inline JavaScript left in templates that open PRs touch, and so not moved here:
includes/graphs.htmlassets/asset_graph.htmlassets/asset_properties.htmlassets/asset_new.htmlassets/asset_automations.htmlsensors/index.htmlsensors/status.htmlaccounts/account.htmlusers/user.htmlThe untouched templates with inline JavaScript left (
dashboard.html,users/users.html,includes/toasts.htmland a few smaller ones) are mostly DataTables and DOM wiring, with little logic to test.How to test
102 tests. They skip without selenium or Chrome, as before.
To see the page-level fixes, open the context page of an asset whose flex-context has a commitment with a
"in its name. Onmainthe editor stays empty.🤖 Generated with Claude Code