Skip to content

Stop the asset page's KPIs counting a day the chart does not show - #2434

Open
Flix6x wants to merge 6 commits into
mainfrom
fix/kpi-window-off-by-one
Open

Stop the asset page's KPIs counting a day the chart does not show#2434
Flix6x wants to merge 6 commits into
mainfrom
fix/kpi-window-off-by-one

Conversation

@Flix6x

@Flix6x Flix6x commented Aug 23, 2026

Copy link
Copy Markdown
Member

Description

KPIs on the asset page cover one day more than the chart beside them.

  • ui/graphs: pass the chart's end date to the KPI endpoint unchanged
  • Added changelog item in documentation/changelog.rst

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 — the picker applies +1 day before calling, and event_ends_before is exclusive on the initial page load. The endpoint ends its own window before end as well: _get_sensor_stats filters TimedBelief.event_start < end_dt.

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, which is why this is a one-line change.

Where the extra day came from

The advance was not arbitrary, and it was once correct. Until #1909 this function took no arguments and read the picker directly:

var endDate = picker.getEndDate();
endDate.setDate(endDate.getDate() + 1);

picker.getEndDate() is the last day selected — 7 January for a 1–7 January selection — so advancing it produced the exclusive end the endpoint wants.

#1909 ("fix: pass start and end dates to getAssetKPIs() on graphs page", closing #1908, shipped in v0.30.3, January 2026) gave the function startDate/endDate parameters, because reading the picker broke page load. Both callers pass the chart's already-exclusive end: the picker handler applies +1 day before calling, and event_ends_before is exclusive on the initial load. The advance inside was carried over unchanged, and from that point added a day too many.

So this is a regression from #1909 rather than the undoing of a deliberate decision. Its own changelog entry stays as it is: #1909 shipped in v0.30.3, so this is a fix to released behaviour, not to an unreleased regression. I have left a comment where the advance used to be, so it does not get put back.

Warning

This changes reported KPI values. They were too high (or too wide, for min/max) by one day's worth of data. Anything tracking them will step down when this lands. The old numbers were wrong, but the change is visible.

Look & Feel

On a seeded asset carrying one unit of energy per day, asking for a three-day window:

end sent KPI total
T0+3d (what the chart shows) 3.0
T0+4d (what the UI sent) 4.0

How to test

test_kpi_window_end_is_exclusive in flexmeasures/api/v3_0/tests/test_assets_api.py asserts that a three-day window over one daily value per day totals three, not four.

To be explicit about what that test does and does not cover: it pins the endpoint's window semantics, which the page depends on. It does not exercise the JavaScript change, and would pass without it — there is no JavaScript test runner in this repo. The front-end half was verified by hand, by reading the end parameter off the request the page issues.

Manually: open an asset page with KPIs configured, select a range, and compare a KPI against the chart beside it. Before this change a sum covers one day more than the chart draws.

Notes

Split out of #2433 at the maintainer's request, because it changes user-visible numbers while that PR is a performance change. #2433 also carries this fix, so the two converge: graphs.html merges cleanly in either order (verified by a trial merge). The changelog entry lives here, and has been removed from #2433 to avoid a duplicate line.

Flix6x added 2 commits August 23, 2026 18:19
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 <felix@seita.nl>
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 <felix@seita.nl>
Flix6x added a commit that referenced this pull request Aug 23, 2026
Context:
- The KPI day-counting fix was split out into PR #2434, since it changes
  user-visible numbers while this PR is a performance change. Both
  branches carry the same code change and merge cleanly either way, but
  the changelog would have gained the line twice.

Change:
- Drop the entry here. The date-corruption entry stays, as that fix is
  needed by the cache in this PR.

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x
Flix6x requested a lite review from Copilot August 23, 2026 16:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@read-the-docs-community

read-the-docs-community Bot commented Aug 23, 2026

Copy link
Copy Markdown

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 <felix@seita.nl>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Flix6x

Flix6x commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

Copilot review requested three times (17:22, 17:31, 17:35 UTC); each attempt came back with "Copilot encountered an error and was unable to review this pull request", referring to runner configuration. Requests on #2433 and #2435 in the same window succeeded, including ones touching this same file, so it looks specific to this PR or transient on their side rather than something in the diff.

Not retrying further to avoid noise. @Flix6x, worth a re-request from your side later, or a human review — the change is one line plus a test.

CI is green: 13/13 checks pass.

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 <felix@seita.nl>
@Flix6x

Flix6x commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

Added the toIsoStringWithOffset fix here too, per @Flix6x. It is a second, independent error in the same KPI window: this PR fixes an end date a day too late, that one fixes both ends being shifted by the viewer's UTC offset. West of UTC the shift lands on a different day for daily-resolution sensors, so the two compound.

The file is byte-identical to the version in #2435, where the JavaScript tests that found it live, so the branches merge in either order. The changelog entry for it stays in #2435, to avoid the line appearing twice.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Flix6x

Flix6x commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

Update on the Copilot review: five attempts now, across three different commits, spanning about two hours — every one returned "Copilot encountered an error and was unable to review this pull request".

attempt commit result
16:23 e07e5ad8c errored
17:23 4a242f557 errored
17:30 4a242f557 errored
17:35 4a242f557 errored
18:11 a48553b40 errored

Requests on #2433 and #2435 succeeded during the same windows, including ones touching this same file, so it is specific to this PR rather than a general outage or anything in the diff. I have stopped retrying.

This PR therefore has had no automated review at all, which is worth knowing given it is the one that changes reported KPI values. @Flix6x, a re-request from your account may fare better; otherwise it is worth a human read. The change is two one-line fixes plus a test.

CI is green: 13/13.

Flix6x added a commit that referenced this pull request Aug 23, 2026
Context:
- Review feedback: the entry linked both the PR and the issue it closes.

Change:
- Drop the issue link. The other entries on this branch, and those on
  PR #2434 and PR #2435, already reference only their PR.

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x
Flix6x requested a lite review from Copilot August 23, 2026 21:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Flix6x added 2 commits August 23, 2026 23:38
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 <felix@seita.nl>
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 <felix@seita.nl>
Flix6x added a commit that referenced this pull request Aug 23, 2026
…gument

Context:
- Found while adversarially reviewing PR #2434: this branch corrected the
  inline comment about the fastChartWindow snapshot but left the block
  comment above the declaration still giving the old reason, that
  getAssetKPIs bumps storeEndDate by a day. It does not, since bc6d5c1.

Change:
- Give the snapshot its real reason, matching the wording on PR #2434 so
  the two branches stay identical here.

Signed-off-by: F.N. Claessen <felix@seita.nl>
Flix6x added a commit that referenced this pull request Aug 23, 2026
Context:
- Adversarial review of PR #2434: the example did not say which clock the
  components come from, which is exactly what the fix changed.

Change:
- State it, and keep the file identical to the copy on PR #2434.

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x

Flix6x commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

Copilot could not review this one (five attempts, all erroring), so I ran three adversarial reviewers locally instead — one on the KPI-window claim, one on timezone correctness, one on test quality. Between them they found five things worth acting on. All are now addressed; the core claim survived.

What survived. The claim that both callers pass an exclusive end was checked by rendering the page for FLEXMEASURES_MODE both normal and play and parsing the emitted script: exactly one definition and two call sites in both, both passing an exclusive end. The #1909 history in the comment checks out against git show aa7f8c90e. And toIsoStringWithOffset survived 1,151,931 assertions across 14 timezones — every DST transition instant in 2019–2027 ±3h, sub-hour offsets (+05:45, +13:45), the repeated hour, and padding edges — with zero failures, and its output verified to parse to the right instant through the real AwareDateTimeField.

What changed.

  1. The test proved nothing about the fix. Both fixes are JavaScript; the test drove the endpoint, which this branch does not touch. It passed on main and would pass with both fixes reverted. The repo asks that a new test be shown to fail first. There is now a rendered-page assertion that getAssetKPIs passes its window on untouched — reintroducing endDate.setDate(endDate.getDate() + 1) turns it red, which is exactly how the regression fix: pass start and end dates to getAssetKPIs() on graphs page #1909 introduced would have been caught. No new dependency; it reads the page the Flask test client renders.

  2. The endpoint test could not see a shifted window. Every belief was worth 1.0, so it counted days rather than identifying them — any three-day window passed. Days now carry distinct values, so the right window totals 6.0 and an hour-shifted one totals 9.0, with an explicit total != shifted assertion so that discrimination cannot rot. Proved red by inverting the endpoint's event_start < end_dt to <=.

  3. Three comments described behaviour that no longer exists. Two still gave "getAssetKPIs bumps storeEndDate by +1 day" as the reason fastChartWindow is snapshotted as epoch numbers — load-bearing, since a maintainer trusting it could drop the snapshot or restore the advance. One is on this branch, the other was on Load only the newly selected days when a chart's time range changes #2433, fixed there in 9f3ecd2 with matching wording so the two stay identical.

  4. The comment overstated the agreement. It said 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 — and they agree only for the daily, midnight-aligned sensors this endpoint documents. For an instantaneous sensor the KPI is now one event short of the chart. Reworded to say so.

  5. Fixture borrowing. The new rendering test took an asset from a shared fixture that earlier tests in the same module delete; it built its own. Both files pass under random ordering.

One thing I am leaving, and flagging instead. Removing the advance also stops storeEndDate being corrupted for the rest of the page session, which means the chart's own reloads after an upload or delete, the chart-type re-embed, and the replay bounds were all a day too wide on KPI-enabled asset pages. That is real and now fixed, but its changelog line lives on #2433 ("the chart's selected end date was pushed a day forward on every date selection"), so if this PR merges alone that symptom ships undocumented. It also makes the previousResult.end.getTime() === endDate.getTime() short-circuit in fetchGraphDataAndKPIs reachable on KPI pages for the first time, since that comparison could never succeed while the date was being mutated.

99 tests pass across the two touched modules, under random ordering.

@Flix6x
Flix6x requested a review from nhoening August 24, 2026 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants