Clarify the time zone in the chart (fixes DEV-133)#550
Clarify the time zone in the chart (fixes DEV-133)#550aptiko merged 1 commit intoopenmeteo:masterfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 980a09c5b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except iso8601.ParseError: | ||
| return None | ||
| def _parse_date(self, adate: str): | ||
| return dt.datetime.fromisoformat(adate) |
There was a problem hiding this comment.
Z-suffixed timestamps in _parse_date
_parse_date now uses datetime.fromisoformat, but this parser rejects ISO strings ending in Z on Python 3.9/3.10 (which are still documented as supported in doc/general/install.rst). In this same commit, the chart frontend switched to toISOString(), which always sends ...Z, so zoom/pan refetches (and any API client following the updated docs) will get a 400 on those supported runtimes instead of returning data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR removes the iso8601 dependency and clarifies that chart timestamps are displayed in UTC timezone. The changes include:
Changes:
- Removed the
iso8601library dependency and replaced all usages with Python's built-indt.datetime.fromisoformat() - Updated API to require timezone in date parameters, returning 400 error if timezone is missing
- Added UTC timezone labels to chart axes and tooltips in the UI
- Added unit of measurement symbol to the y-axis of charts
- Changed JavaScript to send full ISO 8601 strings (including timezone) instead of truncated strings
- Updated documentation to clarify timezone requirements
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| requirements.txt | Removed iso8601 library dependency |
| enhydris/api/views.py | Replaced iso8601 with fromisoformat(), enforced timezone requirement, normalized dates to UTC, added error handling for invalid dates |
| enhydris/api/tests/test_views/test_timeseries.py | Updated tests to include timezone in date strings, added test for missing timezone, updated mock expectations |
| enhydris/telemetry/models.py | Replaced iso8601.parse_date with dt.datetime.fromisoformat |
| enhydris/synoptic/models.py | Replaced iso8601.parse_date with dt.datetime.fromisoformat |
| enhydris/static/js/enhydris.js | Send full ISO strings with timezone, added "t (UTC)" label to x-axis, added unit label to y-axis, updated tooltip format to show UTC |
| enhydris/templates/enhydris/timeseries_group_detail/main-default.html | Added enhydris.unit variable for displaying unit of measurement symbol |
| doc/dev/webservice-api.rst | Updated documentation to clarify timezone requirement and Unix timestamp format |
| .isort.cfg | Removed iso8601 from known_third_party list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _parse_date(self, adate: str): | ||
| return dt.datetime.fromisoformat(adate) |
There was a problem hiding this comment.
The behavior for invalid dates has changed but the old tests in TsdataInvalidStartOrEndDateTestCase were not updated. The tests test_invalid_start_date and test_invalid_end_date (lines 454-462 in the test file) expect invalid dates like "hello" to be silently ignored and None passed to get_data. With the new implementation, _parse_date will raise ValueError when dt.datetime.fromisoformat() fails, which will be caught by the try-except blocks in _get_data and return a 400 error. These old tests need to be updated to expect 400 status codes instead of expecting get_data to be called with None values.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #550 +/- ##
==========================================
- Coverage 97.47% 97.42% -0.06%
==========================================
Files 55 55
Lines 3528 3529 +1
==========================================
- Hits 3439 3438 -1
- Misses 89 91 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
It might have been better to render the chart in the timeseries' timezone, but this was a little bit hard and could be premature. So at least I've made it clear that the time zone is UTC.
aptiko
force-pushed
the
dev-133--chart-time-zone
branch
from
980a09c to
cf47856
Compare
It might have been better to render the chart in the timeseries' timezone, but this was a little bit hard and could be premature. So at least I've made it clear that the time zone is UTC.
Checklist: