From 8366f398d34348673ef38d2905f22df300cde518 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Sun, 23 Aug 2026 18:19:19 +0200 Subject: [PATCH 01/10] ui/graphs: stop the KPIs counting a day the chart does not show Context: - getAssetKPIs advanced its end date by a day to "make the end date inclusive", but both callers already hand it the chart's exclusive end, and the endpoint ends its own window before `end` as well (_get_sensor_stats filters event_start < end). - Every KPI therefore covered one day more than the chart beside it. On a seeded asset with one unit of energy per day, a three-day selection reported a total of four. - Advancing the date also mutated it in place, and that Date object is the one held by storeEndDate and previousResult.end, so the chart's own end date moved a day forward on every selection. Not advancing it at all fixes both. Change: - Pass the chart's end date through unchanged. - Assert the endpoint's window semantics, which the page depends on. Signed-off-by: F.N. Claessen --- documentation/changelog.rst | 2 + .../api/v3_0/tests/test_assets_api.py | 72 +++++++++++++++++++ .../ui/templates/includes/graphs.html | 3 +- 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 21168003cc..038a7b93f6 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -17,6 +17,8 @@ Infrastructure / Support Bugfixes ----------- +* KPIs on the asset page counted one day more than the selected time range [see `PR #XXXX `_] + v1.0.0 | August 14, 2026 ============================ diff --git a/flexmeasures/api/v3_0/tests/test_assets_api.py b/flexmeasures/api/v3_0/tests/test_assets_api.py index 5eb5ff8364..edf6e121fb 100644 --- a/flexmeasures/api/v3_0/tests/test_assets_api.py +++ b/flexmeasures/api/v3_0/tests/test_assets_api.py @@ -1782,3 +1782,75 @@ def test_get_asset_chart_session_vars_with_canonical_params( with client.session_transaction() as sess: assert sess.get("event_starts_after") == "2025-05-01T00:00:00+02:00" assert sess.get("event_ends_before") == "2025-05-02T00:00:00+02:00" + + +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +def test_kpi_window_end_is_exclusive( + db, client, setup_api_test_data, setup_sources, requesting_user +): + """The KPI window ends before `end`, as the chart's own window does. + + The asset page derives the KPI window from the chart's, + so the two have to agree on whether the end is part of the window. + """ + from datetime import datetime, timedelta + + from pytz import utc + + from flexmeasures.data.models.generic_assets import GenericAsset, GenericAssetType + from flexmeasures.data.models.time_series import Sensor, TimedBelief + + window_start = datetime(2022, 1, 1, tzinfo=utc) + asset_type = ( + db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() + ) + if asset_type is None: + asset_type = GenericAssetType(name="battery") + db.session.add(asset_type) + db.session.flush() + asset = GenericAsset( + name="kpi window asset", + generic_asset_type=asset_type, + account_id=requesting_user.account_id, + ) + db.session.add(asset) + db.session.flush() + sensor = Sensor( + name="kpi window sensor", + generic_asset=asset, + event_resolution=timedelta(days=1), + unit="MWh", + ) + db.session.add(sensor) + db.session.flush() + source = list(setup_sources.values())[0] + db.session.bulk_insert_mappings( + TimedBelief, + [ + dict( + event_start=window_start + timedelta(days=day), + belief_horizon=timedelta(0), + event_value=1.0, + sensor_id=sensor.id, + source_id=source.id, + cumulative_probability=0.5, + ) + for day in range(5) + ], + ) + asset.sensors_to_show_as_kpis = [ + {"title": "Total", "sensor": sensor.id, "function": "sum"} + ] + db.session.flush() + + response = client.get( + url_for("AssetAPI:get_kpis", id=asset.id), + query_string={ + "start": window_start.isoformat(), + "end": (window_start + timedelta(days=3)).isoformat(), + }, + ) + assert response.status_code == 200 + assert ( + response.json["data"][0]["downsample_value"] == 3.0 + ), "a three-day window must total three daily values, not four" diff --git a/flexmeasures/ui/templates/includes/graphs.html b/flexmeasures/ui/templates/includes/graphs.html index da24384baa..d4a34c9c7b 100644 --- a/flexmeasures/ui/templates/includes/graphs.html +++ b/flexmeasures/ui/templates/includes/graphs.html @@ -1031,7 +1031,8 @@ {% if active_subpage == "asset_graph" and has_kpis %} function getAssetKPIs(startDate, endDate) { const start = encodeURIComponent(toIsoStringWithOffset(startDate)); - endDate.setDate(endDate.getDate() + 1); // make the end date inclusive + // The endpoint ends its window before `end`, exactly as the chart does, + // so the chart's own end date is passed through unchanged. const end = encodeURIComponent(toIsoStringWithOffset(endDate)); const kpiCards = document.querySelector('#kpi-cards'); return fetch('/api/v3_0/assets/{{ asset.id }}/kpis?start=' + start + '&end=' + end, { From e07e5ad8ca1f442834eacbb785efea210104d10b Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Sun, 23 Aug 2026 18:21:12 +0200 Subject: [PATCH 02/10] docs: fill in the PR number in the changelog entry Context: - The entry was added before the PR existed, with an XXXX placeholder. Change: - Point it at PR #2434. Signed-off-by: F.N. Claessen --- documentation/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 038a7b93f6..9678079d0a 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -17,7 +17,7 @@ Infrastructure / Support Bugfixes ----------- -* KPIs on the asset page counted one day more than the selected time range [see `PR #XXXX `_] +* KPIs on the asset page counted one day more than the selected time range [see `PR #2434 `_] v1.0.0 | August 14, 2026 From 4a242f557026878c07dfa9e914482103e23414c5 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Sun, 23 Aug 2026 18:52:37 +0200 Subject: [PATCH 03/10] ui/graphs: record why the KPI end date is not advanced Context: - The advance was not arbitrary. Until PR #1909 this function read picker.getEndDate(), which is the last day selected, so advancing it by a day correctly produced an exclusive end. - PR #1909 (v0.30.3, January 2026) gave the function start and end parameters and had the callers pass the chart's already-exclusive end, but kept the advance, which from then on added a day too many. Change: - Note that history where the advance used to be, so it does not get put back. Signed-off-by: F.N. Claessen --- flexmeasures/ui/templates/includes/graphs.html | 2 ++ 1 file changed, 2 insertions(+) diff --git a/flexmeasures/ui/templates/includes/graphs.html b/flexmeasures/ui/templates/includes/graphs.html index d4a34c9c7b..ee64ccb730 100644 --- a/flexmeasures/ui/templates/includes/graphs.html +++ b/flexmeasures/ui/templates/includes/graphs.html @@ -1033,6 +1033,8 @@ const start = encodeURIComponent(toIsoStringWithOffset(startDate)); // The endpoint ends its window before `end`, exactly as the chart does, // so the chart's own end date is passed through unchanged. + // Advancing it by a day here was right while this function read picker.getEndDate(), which is the last day selected; + // PR #1909 changed the callers to pass the chart's exclusive end, at which point the advance became one day too many. const end = encodeURIComponent(toIsoStringWithOffset(endDate)); const kpiCards = document.querySelector('#kpi-cards'); return fetch('/api/v3_0/assets/{{ asset.id }}/kpis?start=' + start + '&end=' + end, { From a48553b40aabcf025a3e6973cd354a956de91723 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Sun, 23 Aug 2026 20:00:12 +0200 Subject: [PATCH 04/10] ui/static: send the KPI window at the instant it means Context: - toIsoStringWithOffset appended the local UTC offset to date.toISOString(), which is UTC, without moving the clock time, so the string named an instant wrong by exactly that offset. - getAssetKPIs is its only caller, so this shifted the very window this PR is about. West of UTC the shift lands on a different day for sensors of daily resolution, so KPIs could report a different day than the chart. Change: - Write the local clock time, then append the offset. - The same fix is in PR #2435, where the JavaScript tests that found it live. Both branches carry identical text, so they merge either way. The changelog entry for it stays in #2435. Signed-off-by: F.N. Claessen --- flexmeasures/ui/static/js/daterange-utils.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/flexmeasures/ui/static/js/daterange-utils.js b/flexmeasures/ui/static/js/daterange-utils.js index 45adccfa65..fd6befb040 100644 --- a/flexmeasures/ui/static/js/daterange-utils.js +++ b/flexmeasures/ui/static/js/daterange-utils.js @@ -195,12 +195,15 @@ export function toIsoStringWithOffset(date) { const offsetHours = Math.floor(Math.abs(offset) / 60); const offsetMinutes = Math.abs(offset) % 60; - const isoString = date.toISOString(); - - const formattedIsoString = isoString.replace('Z', - `${(offset <= 0 ? '+' : '-')}${String(offsetHours).padStart(2, '0')}:${String(offsetMinutes).padStart(2, '0')}`); - - return formattedIsoString; + // Write the local clock time, not the UTC one. + // Appending a local offset to date.toISOString(), which is UTC, would name a different instant, + // one that is wrong by exactly the offset. + const pad = (value, width = 2) => String(value).padStart(width, '0'); + const localIsoString = `${date.getFullYear()}-${pad(date.getMonth() + 1)}-${pad(date.getDate())}` + + `T${pad(date.getHours())}:${pad(date.getMinutes())}:${pad(date.getSeconds())}.${pad(date.getMilliseconds(), 3)}`; + + return localIsoString + + `${(offset <= 0 ? '+' : '-')}${pad(offsetHours)}:${pad(offsetMinutes)}`; } /** From 8725185e3304d1d4d95bc345c99668de874d2bc4 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Sun, 23 Aug 2026 23:38:23 +0200 Subject: [PATCH 05/10] ui/graphs: correct the comments about the KPI window Context: - Adversarial review of this branch found three comments left saying things the code no longer does, or never quite did. - Two still gave "getAssetKPIs bumps storeEndDate by +1 day" as the reason fastChartWindow is snapshotted as epoch numbers. That reason is gone, and a maintainer trusting it could drop the snapshot or restore the advance. - One claimed the endpoint ends its window "exactly as the chart does". The predicates differ: the chart keeps events whose end falls on the window's end, the endpoint keeps events whose start falls before it. They agree only for the daily, midnight-aligned sensors the endpoint documents. Change: - Give the snapshot its real reason, and state where the two predicates part company. - Say in the JSDoc of toIsoStringWithOffset that the components written are the local clock ones, which is the whole point of the fix. Signed-off-by: F.N. Claessen --- flexmeasures/ui/static/js/daterange-utils.js | 7 +++++-- flexmeasures/ui/templates/includes/graphs.html | 11 ++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/flexmeasures/ui/static/js/daterange-utils.js b/flexmeasures/ui/static/js/daterange-utils.js index fd6befb040..84616101e3 100644 --- a/flexmeasures/ui/static/js/daterange-utils.js +++ b/flexmeasures/ui/static/js/daterange-utils.js @@ -185,9 +185,12 @@ export function computeSimulationRanges(startDate, endDate, minRes = "hour") { } /** - * Takes a Date object and returns an ISO string with the timezone offset appended + * Takes a Date object and returns an ISO string with the timezone offset appended. + * + * The date and time written are the local clock ones, so the string names the same instant as the Date. + * * @param {Date} date - The date to format - * @returns {string} An ISO string with the timezone offset appended, e.g. "2022-08-23T15:04:05.000+02:00" + * @returns {string} The local clock time with its offset, e.g. "2022-08-23T15:04:05.000+02:00" for 15:04 local in +02:00 */ export function toIsoStringWithOffset(date) { const offset = date.getTimezoneOffset(); diff --git a/flexmeasures/ui/templates/includes/graphs.html b/flexmeasures/ui/templates/includes/graphs.html index ee64ccb730..5a75b51baa 100644 --- a/flexmeasures/ui/templates/includes/graphs.html +++ b/flexmeasures/ui/templates/includes/graphs.html @@ -67,9 +67,9 @@ // rebuild the groupSpec client-side (with y-axis titles) after an in-browser // sensors_to_show save without a page reload. let sensorTypeById = new Map(); - // The current query window as plain epoch-ms {start, end}, captured before any - // later mutation of the shared Date objects (getAssetKPIs bumps storeEndDate by - // +1 day). Used as the fast chart's fixed x-axis domain (matches Vega-Lite). + // The current query window as plain epoch-ms {start, end}, + // captured so that later changes to the shared Date objects cannot move it. + // Used as the fast chart's fixed x-axis domain (matches Vega-Lite). let fastChartWindow = null; if ('{{ active_page }}' == 'assets') { @@ -666,7 +666,7 @@ storeStartDate = startDate; storeEndDate = endDate; - // Capture the window as primitives now, before getAssetKPIs mutates endDate (+1 day). + // Capture the window as primitives, fixing the fast chart's x-axis to the selection made here. fastChartWindow = { start: startDate.getTime(), end: endDate.getTime() }; var queryStartDate = (startDate != null) ? (startDate.toISOString()) : (null); var queryEndDate = (endDate != null) ? (endDate.toISOString()) : (null); @@ -1031,8 +1031,9 @@ {% if active_subpage == "asset_graph" and has_kpis %} function getAssetKPIs(startDate, endDate) { const start = encodeURIComponent(toIsoStringWithOffset(startDate)); - // The endpoint ends its window before `end`, exactly as the chart does, + // The endpoint ends its window before `end`, as the chart does for the daily, midnight-aligned sensors it documents this for, // so the chart's own end date is passed through unchanged. + // The two predicates are not identical: the chart keeps events whose end falls on the window's end, the endpoint keeps events whose start falls before it. // Advancing it by a day here was right while this function read picker.getEndDate(), which is the last day selected; // PR #1909 changed the callers to pass the chart's exclusive end, at which point the advance became one day too many. const end = encodeURIComponent(toIsoStringWithOffset(endDate)); From 85116ecb6bb90d4576e1fd3e814798c3781f0e40 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Sun, 23 Aug 2026 23:38:24 +0200 Subject: [PATCH 06/10] tests: cover the fix itself, not only the contract beneath it Context: - Adversarial review: the test added here exercised the KPI endpoint, which this branch does not change. It passed on main, and would pass with both fixes reverted, so it could never catch a regression of either. The repo asks that a new test be shown to fail before it is called done. - The endpoint test was also blind to a shifted window: every belief was worth 1.0, so it only counted days rather than identifying them. Change: - Assert on the rendered page that getAssetKPIs passes its window on untouched. Reintroducing the advance turns it red, which is how the regression PR #1909 caused would have been caught. - Give each day a distinct value, so a window of the right length but the wrong days no longer passes, and add a case pinning that the endpoint reads the offset it is given rather than the clock time alone. - Assert the KPI list length before indexing it, compare with approx, and build the fixtures each test needs rather than borrowing assets that earlier tests in the module delete. Signed-off-by: F.N. Claessen --- .../api/v3_0/tests/test_assets_api.py | 99 ++++++++++++++----- flexmeasures/ui/tests/test_asset_crud.py | 74 ++++++++++++++ 2 files changed, 151 insertions(+), 22 deletions(-) diff --git a/flexmeasures/api/v3_0/tests/test_assets_api.py b/flexmeasures/api/v3_0/tests/test_assets_api.py index edf6e121fb..01a13b8d27 100644 --- a/flexmeasures/api/v3_0/tests/test_assets_api.py +++ b/flexmeasures/api/v3_0/tests/test_assets_api.py @@ -1784,21 +1784,18 @@ def test_get_asset_chart_session_vars_with_canonical_params( assert sess.get("event_ends_before") == "2025-05-02T00:00:00+02:00" -@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) -def test_kpi_window_end_is_exclusive( - db, client, setup_api_test_data, setup_sources, requesting_user -): - """The KPI window ends before `end`, as the chart's own window does. +def _asset_with_daily_kpi(db, requesting_user, setup_sources, days: int): + """Build an asset whose KPI sums a distinct value per day. - The asset page derives the KPI window from the chart's, - so the two have to agree on whether the end is part of the window. + The values differ per day, so that a window covering the wrong days totals differently, + rather than merely covering the same number of days. """ - from datetime import datetime, timedelta + from datetime import datetime from pytz import utc - from flexmeasures.data.models.generic_assets import GenericAsset, GenericAssetType - from flexmeasures.data.models.time_series import Sensor, TimedBelief + from flexmeasures.data.models.generic_assets import GenericAssetType + from flexmeasures.data.models.time_series import TimedBelief window_start = datetime(2022, 1, 1, tzinfo=utc) asset_type = ( @@ -1809,14 +1806,14 @@ def test_kpi_window_end_is_exclusive( db.session.add(asset_type) db.session.flush() asset = GenericAsset( - name="kpi window asset", + name=f"kpi window asset ({days} days)", generic_asset_type=asset_type, account_id=requesting_user.account_id, ) db.session.add(asset) db.session.flush() sensor = Sensor( - name="kpi window sensor", + name=f"kpi window sensor ({days} days)", generic_asset=asset, event_resolution=timedelta(days=1), unit="MWh", @@ -1830,27 +1827,85 @@ def test_kpi_window_end_is_exclusive( dict( event_start=window_start + timedelta(days=day), belief_horizon=timedelta(0), - event_value=1.0, + event_value=float(day + 1), sensor_id=sensor.id, source_id=source.id, cumulative_probability=0.5, ) - for day in range(5) + for day in range(days) ], ) asset.sensors_to_show_as_kpis = [ {"title": "Total", "sensor": sensor.id, "function": "sum"} ] db.session.flush() + return asset, window_start + +def _kpi_total(client, asset, start, end): + """Ask the KPI endpoint for one window and return its single value.""" response = client.get( url_for("AssetAPI:get_kpis", id=asset.id), - query_string={ - "start": window_start.isoformat(), - "end": (window_start + timedelta(days=3)).isoformat(), - }, + query_string={"start": start, "end": end}, ) - assert response.status_code == 200 - assert ( - response.json["data"][0]["downsample_value"] == 3.0 - ), "a three-day window must total three daily values, not four" + assert response.status_code == 200, response.json + kpis = response.json["data"] + assert len(kpis) == 1, f"expected exactly one KPI, got {kpis}" + return kpis[0]["downsample_value"] + + +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +def test_kpi_window_end_is_exclusive( + db, client, setup_api_test_data, setup_sources, requesting_user +): + """The KPI window ends before `end`, as the chart's own window does. + + The asset page derives the KPI window from the chart's, + so the two have to agree on whether the end is part of the window. + """ + asset, window_start = _asset_with_daily_kpi( + db, requesting_user, setup_sources, days=5 + ) + + # Days one to five carry the values 1 to 5, so the first three total six. + # Treating the end as inclusive would add the fourth day and total ten. + total = _kpi_total( + client, + asset, + window_start.isoformat(), + (window_start + timedelta(days=3)).isoformat(), + ) + assert total == pytest.approx( + 6.0 + ), "the window must end before its end date, not on it" + + +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +def test_kpi_window_honours_the_offset_it_is_given( + db, client, setup_api_test_data, setup_sources, requesting_user +): + """A window written with a UTC offset names the instants that offset implies. + + The asset page sends its window as local clock times carrying an offset, + so the endpoint has to read the offset rather than the clock time alone. + """ + asset, _ = _asset_with_daily_kpi(db, requesting_user, setup_sources, days=6) + + # Midnight UTC on the first, written as one o'clock in +01:00. + # These are the first three days, worth 1 + 2 + 3. + total = _kpi_total( + client, asset, "2022-01-01T01:00:00+01:00", "2022-01-04T01:00:00+01:00" + ) + assert total == pytest.approx( + 6.0 + ), "the offset must be read, not just the clock time" + + # The same clock times read as UTC name instants an hour later, + # which drops the first day and picks up the fourth, worth 2 + 3 + 4. + shifted = _kpi_total( + client, asset, "2022-01-01T01:00:00+00:00", "2022-01-04T01:00:00+00:00" + ) + assert shifted == pytest.approx( + 9.0 + ), "an hour later is a different three days, so the two windows must not agree" + assert total != shifted, "the assertion above only means something if these differ" diff --git a/flexmeasures/ui/tests/test_asset_crud.py b/flexmeasures/ui/tests/test_asset_crud.py index dea1dd79d0..4eb2c59963 100644 --- a/flexmeasures/ui/tests/test_asset_crud.py +++ b/flexmeasures/ui/tests/test_asset_crud.py @@ -641,3 +641,77 @@ def test_group_field_hints_on_properties_page( assert lone_page.status_code == 200 assert b"Consider setting" not in lone_page.data assert b"Child assets can" not in lone_page.data + + +def _rendered_function(page: str, name: str) -> str: + """Return the body of a JavaScript function as the page actually emits it. + + Jinja conditionals decide what reaches the browser, + so the rendered page is what has to be checked, not the template. + """ + start = page.index(f"function {name}(") + depth, opened = 0, False + for index in range(start, len(page)): + if page[index] == "{": + depth, opened = depth + 1, True + elif page[index] == "}": + depth -= 1 + if opened and depth == 0: + return page[start : index + 1] + raise AssertionError(f"the body of {name} is not closed in the rendered page") + + +def test_asset_kpis_are_asked_for_the_window_the_chart_shows( + db, client, setup_ui_test_data, as_admin +): + """getAssetKPIs must pass its window on untouched. + + Advancing the end date here was correct until PR #1909 gave this function its arguments, + and re-advancing it would make every KPI cover a day the chart does not show. + """ + # Built here rather than taken from a shared fixture, + # since tests earlier in this module delete those assets. + from flexmeasures.data.models.generic_assets import GenericAssetType + + admin = find_user_by_email("flexmeasures-admin@seita.nl") + asset_type = ( + db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() + ) + if asset_type is None: + asset_type = GenericAssetType(name="battery") + db.session.add(asset_type) + db.session.flush() + asset = GenericAsset( + name="kpi window rendering asset", + generic_asset_type=asset_type, + account_id=admin.account_id, + ) + db.session.add(asset) + db.session.flush() + sensor = Sensor( + name="kpi window rendering sensor", + generic_asset=asset, + event_resolution=timedelta(days=1), + unit="MWh", + ) + db.session.add(sensor) + db.session.flush() + asset.sensors_to_show_as_kpis = [ + {"title": "Total", "sensor": sensor.id, "function": "sum"} + ] + db.session.flush() + + response = client.get( + url_for("AssetCrudUI:graphs", id=asset.id), follow_redirects=True + ) + assert response.status_code == 200 + page = response.get_data(as_text=True) + body = _rendered_function(page, "getAssetKPIs") + + assert "setDate" not in body, ( + "getAssetKPIs must not move the window it is given; " + f"its callers already pass the chart's exclusive end. Rendered body:\n{body}" + ) + assert ( + body.count("toIsoStringWithOffset") == 2 + ), "both ends of the KPI window should be formatted the same way" From c48ef0d654fc2bed32f3d56eacc2759615dc416c Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Tue, 1 Sep 2026 12:56:14 +0200 Subject: [PATCH 07/10] api/v3_0: total the beliefs the chart draws in a KPI Context: - @nhoening found that multiple selected days gave the wrong total: with January 15 worth 122 from one source and January 16 worth 100 from an uploaded point, the two days together reported 100 rather than 222, and every range including the 16th reported 100. - The cause is that the KPI read get_sensor_stats, which groups belief rows by data source, and then took whichever source the database returned first. Reproduced exactly, and on this machine the arbitrary winner was the other one, which is itself the point. - The same reading has a second consequence: those stats aggregate every belief row, with no belief-time filter, so a revised value was added to the value it revised. One day believed first as 50 and later as 122 reported 172, where the chart shows 122. Change: - Read the beliefs the chart draws, one value per event, and reduce those. A KPI is read beside the chart, so it should total what the chart totals. - get_downsample_function_and_value now takes those values rather than per-source statistics. Its only caller is this endpoint; the sensor stats endpoint is untouched, since a per-source breakdown is the point there. Signed-off-by: F.N. Claessen --- documentation/changelog.rst | 1 + flexmeasures/api/v3_0/assets.py | 12 +++++++++-- flexmeasures/data/utils.py | 37 +++++++++++++++++++-------------- 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index e868a9066b..c9beb8283f 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -24,6 +24,7 @@ Bugfixes * The time range sent when loading an asset's KPIs was off by the viewer's UTC offset, so KPIs could cover the wrong days [see `PR #2435 `_] * KPIs on the asset page counted one day more than the selected time range [see `PR #2434 `_] +* KPIs on the asset page now total exactly what the chart beside them draws: a sensor reported by several sources counted only one of them, and a revised value was counted on top of the value it revised [see `PR #2434 `_] diff --git a/flexmeasures/api/v3_0/assets.py b/flexmeasures/api/v3_0/assets.py index 6c420350f4..570e9db29e 100644 --- a/flexmeasures/api/v3_0/assets.py +++ b/flexmeasures/api/v3_0/assets.py @@ -1887,10 +1887,18 @@ def get_kpis(self, id: int, asset: GenericAsset, start, end): kpis = [] for kpi in asset_kpis: sensor = Sensor.query.get(kpi["sensor"]) - sensor_stats = get_sensor_stats(sensor, start, end, sort_keys=False) + # The beliefs the chart draws: one value per event, the most recent one. + # Aggregating belief rows instead would count a revision on top of what it revised, + # and would count each source separately when several report the same sensor. + beliefs = sensor.search_beliefs( + event_starts_after=start, + event_ends_before=end, + most_recent_beliefs_only=True, + ) + values = beliefs["event_value"].dropna() downsample_function, downsample_value = get_downsample_function_and_value( - kpi, sensor, sensor_stats + kpi, sensor, values ) kpi_dict = { "title": kpi["title"], diff --git a/flexmeasures/data/utils.py b/flexmeasures/data/utils.py index 840415e62e..b684c5d560 100644 --- a/flexmeasures/data/utils.py +++ b/flexmeasures/data/utils.py @@ -273,26 +273,31 @@ def save_to_db( return status -def get_downsample_function_and_value( - kpi: dict, sensor: Sensor, sensor_stats: dict -) -> tuple: +def get_downsample_function_and_value(kpi: dict, sensor: Sensor, values) -> tuple: + """Reduce a sensor's values over a window to the single number a KPI shows. + + :param kpi: The `sensors_to_show_as_kpis` entry, which may name a function. + :param sensor: The sensor the KPI describes, whose unit decides the default function. + :param values: One value per event, as the chart draws them, rather than one row per belief. + :returns: The function used, and the value it produced. + """ downsample_function = kpi.get("function", None) if downsample_function is None: if sensor.unit == "%": downsample_function = "mean" else: downsample_function = "sum" - try: - if downsample_function == "mean": - downsample_value = dict(next(iter(sensor_stats.values())))["Mean value"] - elif downsample_function == "max": - downsample_value = dict(next(iter(sensor_stats.values())))["Max value"] - elif downsample_function == "min": - downsample_value = dict(next(iter(sensor_stats.values())))["Min value"] - else: - downsample_value = dict(next(iter(sensor_stats.values())))[ - "Sum over values" - ] - except StopIteration: - downsample_value = 0 + + # An empty window has nothing to reduce, and sum() over none of it is not a KPI of zero cost. + if len(values) == 0: + return downsample_function, 0 + + if downsample_function == "mean": + downsample_value = values.mean() + elif downsample_function == "max": + downsample_value = values.max() + elif downsample_function == "min": + downsample_value = values.min() + else: + downsample_value = values.sum() return downsample_function, downsample_value From 03b8145e807bc02ed8f3be1e7f467a49c1bcede1 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Tue, 1 Sep 2026 12:56:15 +0200 Subject: [PATCH 08/10] tests: assert what the KPI reports, not how the page is written Context: - Review by @nhoening: the rendered-page assertions test artefacts rather than outcomes, the imports belong at the top of the module, and the helpers should carry type hints. Change: - Drop the rendered-page test. What it guarded, that getAssetKPIs does not move its window, is now covered by asserting the reported total. - Add a test that a KPI totals exactly what the chart draws, over a sensor with two sources and a revised belief. It fails on the previous behaviour, reporting 100 or 172 where the chart shows 229. - Hoist the imports, type the helpers, and correct the expectation of the offset test: now that the KPI reads the chart's beliefs, the two select the same events, so a window overlapping a fourth day counts it. Signed-off-by: F.N. Claessen --- .../api/v3_0/tests/test_assets_api.py | 121 ++++++++++++++++-- .../ui/templates/includes/graphs.html | 6 +- flexmeasures/ui/tests/test_asset_crud.py | 74 ----------- 3 files changed, 109 insertions(+), 92 deletions(-) diff --git a/flexmeasures/api/v3_0/tests/test_assets_api.py b/flexmeasures/api/v3_0/tests/test_assets_api.py index 01a13b8d27..f8c462951d 100644 --- a/flexmeasures/api/v3_0/tests/test_assets_api.py +++ b/flexmeasures/api/v3_0/tests/test_assets_api.py @@ -1,11 +1,15 @@ import json -from datetime import timedelta +from datetime import datetime, timedelta from flask import url_for import pytest from sqlalchemy import select, func +from pytz import utc + from flexmeasures.data.models.audit_log import AssetAuditLog +from flexmeasures.data.models.generic_assets import GenericAssetType +from flexmeasures.data.models.time_series import TimedBelief from flexmeasures.data.models.generic_assets import GenericAsset from flexmeasures.data.models.time_series import Sensor from flexmeasures.data.services.users import find_user_by_email @@ -1784,19 +1788,14 @@ def test_get_asset_chart_session_vars_with_canonical_params( assert sess.get("event_ends_before") == "2025-05-02T00:00:00+02:00" -def _asset_with_daily_kpi(db, requesting_user, setup_sources, days: int): +def _asset_with_daily_kpi( + db, requesting_user, setup_sources, days: int +) -> tuple[GenericAsset, datetime]: """Build an asset whose KPI sums a distinct value per day. The values differ per day, so that a window covering the wrong days totals differently, rather than merely covering the same number of days. """ - from datetime import datetime - - from pytz import utc - - from flexmeasures.data.models.generic_assets import GenericAssetType - from flexmeasures.data.models.time_series import TimedBelief - window_start = datetime(2022, 1, 1, tzinfo=utc) asset_type = ( db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() @@ -1842,7 +1841,7 @@ def _asset_with_daily_kpi(db, requesting_user, setup_sources, days: int): return asset, window_start -def _kpi_total(client, asset, start, end): +def _kpi_total(client, asset: GenericAsset, start: str, end: str) -> float: """Ask the KPI endpoint for one window and return its single value.""" response = client.get( url_for("AssetAPI:get_kpis", id=asset.id), @@ -1900,12 +1899,106 @@ def test_kpi_window_honours_the_offset_it_is_given( 6.0 ), "the offset must be read, not just the clock time" - # The same clock times read as UTC name instants an hour later, - # which drops the first day and picks up the fourth, worth 2 + 3 + 4. + # The same clock times read as UTC name instants an hour later. + # The window then overlaps a fourth day, and the KPI covers what the chart draws, + # which is every day the window touches: 1 + 2 + 3 + 4. shifted = _kpi_total( client, asset, "2022-01-01T01:00:00+00:00", "2022-01-04T01:00:00+00:00" ) assert shifted == pytest.approx( - 9.0 - ), "an hour later is a different three days, so the two windows must not agree" + 10.0 + ), "an hour later is a different set of days, so the two windows must not agree" assert total != shifted, "the assertion above only means something if these differ" + + +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +def test_kpi_reports_what_the_chart_draws( + db, client, setup_api_test_data, setup_sources, requesting_user +): + """A KPI is read beside the chart, so it must describe the same beliefs. + + Two ways that used to diverge: several sources reporting one sensor were counted separately, + and a revised belief was counted on top of the belief it revised. + """ + asset_type = ( + db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() + ) + asset = GenericAsset( + name="kpi agrees with chart", + generic_asset_type=asset_type, + account_id=requesting_user.account_id, + ) + db.session.add(asset) + db.session.flush() + sensor = Sensor( + name="kpi agrees with chart sensor", + generic_asset=asset, + event_resolution=timedelta(days=1), + unit="EUR", + ) + db.session.add(sensor) + db.session.flush() + + sources = list(setup_sources.values()) + scheduled, uploaded = sources[0], sources[-1] + assert scheduled.id != uploaded.id, "this test needs two distinct sources" + + window_start = datetime(2030, 1, 15, tzinfo=utc) + db.session.bulk_insert_mappings( + TimedBelief, + [ + # One day per source, as when a single point is uploaded beside computed data. + dict( + event_start=window_start, + belief_horizon=timedelta(0), + event_value=122.0, + sensor_id=sensor.id, + source_id=scheduled.id, + cumulative_probability=0.5, + ), + dict( + event_start=window_start + timedelta(days=1), + belief_horizon=timedelta(0), + event_value=100.0, + sensor_id=sensor.id, + source_id=uploaded.id, + cumulative_probability=0.5, + ), + # A third day believed twice, the later belief revising the earlier one. + dict( + event_start=window_start + timedelta(days=2), + belief_horizon=timedelta(days=2), + event_value=50.0, + sensor_id=sensor.id, + source_id=scheduled.id, + cumulative_probability=0.5, + ), + dict( + event_start=window_start + timedelta(days=2), + belief_horizon=timedelta(days=1), + event_value=7.0, + sensor_id=sensor.id, + source_id=scheduled.id, + cumulative_probability=0.5, + ), + ], + ) + asset.sensors_to_show_as_kpis = [ + {"title": "Daily costs", "sensor": sensor.id, "function": "sum"} + ] + db.session.flush() + + window_end = window_start + timedelta(days=3) + total = _kpi_total(client, asset, window_start.isoformat(), window_end.isoformat()) + + drawn = sensor.search_beliefs( + event_starts_after=window_start, + event_ends_before=window_end, + most_recent_beliefs_only=True, + ) + assert total == pytest.approx( + float(drawn["event_value"].sum()) + ), "the KPI must total exactly the values the chart draws" + assert total == pytest.approx( + 229.0 + ), "122 from one source, 100 from another, and 7 revising 50, is 229" diff --git a/flexmeasures/ui/templates/includes/graphs.html b/flexmeasures/ui/templates/includes/graphs.html index 73a052ec17..b4cd89b481 100644 --- a/flexmeasures/ui/templates/includes/graphs.html +++ b/flexmeasures/ui/templates/includes/graphs.html @@ -1033,10 +1033,8 @@ {% if active_subpage == "asset_graph" and has_kpis %} function getAssetKPIs(startDate, endDate) { const start = encodeURIComponent(toIsoStringWithOffset(startDate)); - // The endpoint ends its window before `end`, as the chart does for the daily, midnight-aligned sensors it documents this for, - // so the chart's own end date is passed through unchanged. - // The two predicates are not identical: the chart keeps events whose end falls on the window's end, the endpoint keeps events whose start falls before it. - // Advancing it by a day here was right while this function read picker.getEndDate(), which is the last day selected; + // The endpoint selects the same events as the chart, so the chart's own window is passed through unchanged. + // Advancing the end by a day here was right while this function read picker.getEndDate(), which is the last day selected; // PR #1909 changed the callers to pass the chart's exclusive end, at which point the advance became one day too many. const end = encodeURIComponent(toIsoStringWithOffset(endDate)); const kpiCards = document.querySelector('#kpi-cards'); diff --git a/flexmeasures/ui/tests/test_asset_crud.py b/flexmeasures/ui/tests/test_asset_crud.py index 4eb2c59963..dea1dd79d0 100644 --- a/flexmeasures/ui/tests/test_asset_crud.py +++ b/flexmeasures/ui/tests/test_asset_crud.py @@ -641,77 +641,3 @@ def test_group_field_hints_on_properties_page( assert lone_page.status_code == 200 assert b"Consider setting" not in lone_page.data assert b"Child assets can" not in lone_page.data - - -def _rendered_function(page: str, name: str) -> str: - """Return the body of a JavaScript function as the page actually emits it. - - Jinja conditionals decide what reaches the browser, - so the rendered page is what has to be checked, not the template. - """ - start = page.index(f"function {name}(") - depth, opened = 0, False - for index in range(start, len(page)): - if page[index] == "{": - depth, opened = depth + 1, True - elif page[index] == "}": - depth -= 1 - if opened and depth == 0: - return page[start : index + 1] - raise AssertionError(f"the body of {name} is not closed in the rendered page") - - -def test_asset_kpis_are_asked_for_the_window_the_chart_shows( - db, client, setup_ui_test_data, as_admin -): - """getAssetKPIs must pass its window on untouched. - - Advancing the end date here was correct until PR #1909 gave this function its arguments, - and re-advancing it would make every KPI cover a day the chart does not show. - """ - # Built here rather than taken from a shared fixture, - # since tests earlier in this module delete those assets. - from flexmeasures.data.models.generic_assets import GenericAssetType - - admin = find_user_by_email("flexmeasures-admin@seita.nl") - asset_type = ( - db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() - ) - if asset_type is None: - asset_type = GenericAssetType(name="battery") - db.session.add(asset_type) - db.session.flush() - asset = GenericAsset( - name="kpi window rendering asset", - generic_asset_type=asset_type, - account_id=admin.account_id, - ) - db.session.add(asset) - db.session.flush() - sensor = Sensor( - name="kpi window rendering sensor", - generic_asset=asset, - event_resolution=timedelta(days=1), - unit="MWh", - ) - db.session.add(sensor) - db.session.flush() - asset.sensors_to_show_as_kpis = [ - {"title": "Total", "sensor": sensor.id, "function": "sum"} - ] - db.session.flush() - - response = client.get( - url_for("AssetCrudUI:graphs", id=asset.id), follow_redirects=True - ) - assert response.status_code == 200 - page = response.get_data(as_text=True) - body = _rendered_function(page, "getAssetKPIs") - - assert "setDate" not in body, ( - "getAssetKPIs must not move the window it is given; " - f"its callers already pass the chart's exclusive end. Rendered body:\n{body}" - ) - assert ( - body.count("toIsoStringWithOffset") == 2 - ), "both ends of the KPI window should be formatted the same way" From 3878c0a0fb835c9680f27356c8dd6b136102a179 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Tue, 1 Sep 2026 13:05:27 +0200 Subject: [PATCH 09/10] api/v3_0: drop the import the KPI change left behind Context: - CI's flake8 caught F401: get_sensor_stats was still imported in assets.py after get_kpis stopped calling it. I had been skipping flake8 locally, which is exactly why this reached CI. Change: - Remove the import. The sensor stats endpoint still uses the function; the KPI endpoint no longer does. Signed-off-by: F.N. Claessen --- flexmeasures/api/v3_0/assets.py | 1 - 1 file changed, 1 deletion(-) diff --git a/flexmeasures/api/v3_0/assets.py b/flexmeasures/api/v3_0/assets.py index 570e9db29e..3070448f10 100644 --- a/flexmeasures/api/v3_0/assets.py +++ b/flexmeasures/api/v3_0/assets.py @@ -26,7 +26,6 @@ ) from flexmeasures.data.services.sensors import ( build_asset_jobs_data, - get_sensor_stats, ) from flexmeasures.api.common.schemas.scheduling import ( flex_context_schema_openAPI, From 6ddda9f1844c0ffa3c0eba47324190748eb3c47d Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Tue, 1 Sep 2026 18:14:12 +0200 Subject: [PATCH 10/10] api/v3_0: count each event under the day it starts in Context: - @nhoening found that after the previous commit every one-day selection also counted its neighbour: with data on the 15th and 16th, the 15th reported 222 and the 14th reported 122. - Reading the chart's beliefs brought the chart's selection rule with it, which takes events *overlapping* the window. His sensor's daily events sit on the UTC grid while he reads them from +01:00, so every local day runs 23:00 to 23:00 UTC and straddles two events, and each event fell into two adjacent days. - Reproduced only once the events were placed on the UTC grid; with events at local midnight the totals were right, which is what made the first attempt look correct. Change: - Keep reading the beliefs the chart draws, but count each event under the window it starts in. Summing what the chart draws would count a straddling event under both neighbours. - This is a deliberate difference from the chart: the chart draws such an event in both days, the KPI counts it once. Signed-off-by: F.N. Claessen --- documentation/changelog.rst | 2 +- flexmeasures/api/v3_0/assets.py | 5 ++ .../api/v3_0/tests/test_assets_api.py | 80 +++++++++++++++++-- 3 files changed, 80 insertions(+), 7 deletions(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 7d9a29c5b7..1ba1119c5e 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -23,7 +23,7 @@ Bugfixes ----------- * KPIs on the asset page counted one day more than the selected time range [see `PR #2434 `_] -* KPIs on the asset page now total exactly what the chart beside them draws: a sensor reported by several sources counted only one of them, and a revised value was counted on top of the value it revised [see `PR #2434 `_] +* KPIs on the asset page now total the values the chart beside them draws, counting each event under the day it starts in: a sensor reported by several sources counted only one of them, and a revised value was counted on top of the value it revised [see `PR #2434 `_] * The time range sent when loading an asset's KPIs was off by the viewer's UTC offset, so KPIs could cover the wrong days [see `PR #2435 `_] diff --git a/flexmeasures/api/v3_0/assets.py b/flexmeasures/api/v3_0/assets.py index 3070448f10..09defbaa4d 100644 --- a/flexmeasures/api/v3_0/assets.py +++ b/flexmeasures/api/v3_0/assets.py @@ -1894,6 +1894,11 @@ def get_kpis(self, id: int, asset: GenericAsset, start, end): event_ends_before=end, most_recent_beliefs_only=True, ) + # Count each event once, under the window it starts in. + # The search also returns events that merely overlap the window, which the chart draws, + # but a total that included them would count one event under two adjacent selections. + event_starts = beliefs.index.get_level_values("event_start") + beliefs = beliefs[(event_starts >= start) & (event_starts < end)] values = beliefs["event_value"].dropna() downsample_function, downsample_value = get_downsample_function_and_value( diff --git a/flexmeasures/api/v3_0/tests/test_assets_api.py b/flexmeasures/api/v3_0/tests/test_assets_api.py index f8c462951d..4bd78a2cc3 100644 --- a/flexmeasures/api/v3_0/tests/test_assets_api.py +++ b/flexmeasures/api/v3_0/tests/test_assets_api.py @@ -1899,14 +1899,13 @@ def test_kpi_window_honours_the_offset_it_is_given( 6.0 ), "the offset must be read, not just the clock time" - # The same clock times read as UTC name instants an hour later. - # The window then overlaps a fourth day, and the KPI covers what the chart draws, - # which is every day the window touches: 1 + 2 + 3 + 4. + # The same clock times read as UTC name instants an hour later, + # which drops the first day and picks up the fourth: 2 + 3 + 4. shifted = _kpi_total( client, asset, "2022-01-01T01:00:00+00:00", "2022-01-04T01:00:00+00:00" ) assert shifted == pytest.approx( - 10.0 + 9.0 ), "an hour later is a different set of days, so the two windows must not agree" assert total != shifted, "the assertion above only means something if these differ" @@ -1996,9 +1995,78 @@ def test_kpi_reports_what_the_chart_draws( event_ends_before=window_end, most_recent_beliefs_only=True, ) + starts = drawn.index.get_level_values("event_start") + within = drawn[(starts >= window_start) & (starts < window_end)] assert total == pytest.approx( - float(drawn["event_value"].sum()) - ), "the KPI must total exactly the values the chart draws" + float(within["event_value"].sum()) + ), "the KPI must total the values the chart draws, for the events this window owns" assert total == pytest.approx( 229.0 ), "122 from one source, 100 from another, and 7 revising 50, is 229" + + +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +def test_kpi_counts_each_event_under_one_day_only( + db, client, setup_api_test_data, setup_sources, requesting_user +): + """A day's KPI covers the events that day owns, not those merely overlapping it. + + A daily sensor on the UTC grid, read from a timezone an hour ahead, has every event + straddling the boundary between two local days. + Counting an event under both would total it twice across neighbouring selections. + """ + asset_type = ( + db.session.query(GenericAssetType).filter_by(name="battery").one_or_none() + ) + asset = GenericAsset( + name="kpi owns its events", + generic_asset_type=asset_type, + account_id=requesting_user.account_id, + ) + db.session.add(asset) + db.session.flush() + sensor = Sensor( + name="kpi owns its events sensor", + generic_asset=asset, + event_resolution=timedelta(days=1), + unit="EUR", + timezone="UTC", + ) + db.session.add(sensor) + db.session.flush() + source = list(setup_sources.values())[0] + + first = datetime(2030, 1, 15, tzinfo=utc) + db.session.bulk_insert_mappings( + TimedBelief, + [ + dict( + event_start=first + timedelta(days=offset), + belief_horizon=timedelta(0), + event_value=value, + sensor_id=sensor.id, + source_id=source.id, + cumulative_probability=0.5, + ) + for offset, value in ((0, 122.0), (1, 100.0)) + ], + ) + asset.sensors_to_show_as_kpis = [ + {"title": "Daily costs", "sensor": sensor.id, "function": "sum"} + ] + db.session.flush() + + # Local days in +01:00, so each runs from 23:00 UTC to 23:00 UTC and straddles both events. + def local_day(day: int) -> str: + return f"2030-01-{day:02d}T00:00:00+01:00" + + totals = { + day: _kpi_total(client, asset, local_day(day), local_day(day + 1)) + for day in (15, 16, 17) + } + assert totals[15] == pytest.approx(122.0), "the 15th owns only its own event" + assert totals[16] == pytest.approx(100.0), "the 16th must not also count the 15th" + assert totals[17] == pytest.approx(0.0), "the 17th owns nothing" + assert sum(totals.values()) == pytest.approx( + 222.0 + ), "each event counts once across neighbouring days, not twice"