[fix] Graceful fallback to default timezone for invalid inputs#766
[fix] Graceful fallback to default timezone for invalid inputs#766UltraBot05 wants to merge 7 commits intoopenwisp:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes implement timezone normalization to handle deprecated timezone names (e.g., Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI Job FailuresHello @UltraBot05, There are multiple Failures & Remediation
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/monitoring/tests/test_api.py`:
- Around line 699-705: The test currently only asserts the response is not 500
for an invalid timezone; update the subtest (in monitoring/tests/test_api.py) to
assert the expected successful fallback behavior from normalize_timezone: call
the endpoint with timezone "Antarctica/Banana" and assert response.status_code
== 200 (and optionally assert the returned payload contains the expected
charts/data keys), referencing normalize_timezone and settings.TIME_ZONE to
ensure we expect a 200 fallback to the configured timezone rather than allowing
a 400/500.
In `@openwisp_monitoring/utils.py`:
- Around line 48-55: The current legacy_map in openwisp_monitoring/utils.py only
covers five deprecated timezones; expand legacy_map to include additional common
IANA aliases (e.g., add mappings like "Europe/Kiev"->"Europe/Kyiv",
"Asia/Rangoon"->"Asia/Yangon", and other well-known legacy names) and/or
implement fallback normalization using pytz/zoneinfo where available: update the
code paths that reference legacy_map (the legacy_map dict and any callers that
resolve timezones) to first attempt direct lookup in legacy_map, then try
pytz.timezone normalization or zoneinfo lookup to map aliases to canonical
names, and finally fall back to raising a clear error if unresolved. Ensure you
modify the legacy_map symbol and the timezone resolution logic so all lookups
use the expanded map + normalization fallback.
- Around line 59-67: The hardcoded filesystem check using tz_path and
os.path.exists should be replaced with the cross-platform zoneinfo API: attempt
to construct zoneinfo.ZoneInfo(normalized_tz) and catch
zoneinfo.ZoneInfoNotFoundError to determine non-existence, log/return
accordingly; update the timezone verification block that references
normalized_tz and tz_name to use this approach and preserve the normalized-tz
logging when different, and add the tzdata package to project dependencies so
ZoneInfo works on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f33c90b4-2c69-4220-ad21-2d25aafe7794
📒 Files selected for processing (4)
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/utils.pyopenwisp_monitoring/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/views.pyopenwisp_monitoring/utils.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.py
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.py
🔇 Additional comments (5)
openwisp_monitoring/utils.py (1)
40-46: LGTM: Cache decorator and empty input handling.The
@lru_cache(maxsize=128)is appropriate for timezone normalization since results are deterministic. The early return for emptytz_namecorrectly defaults to the project's TIME_ZONE setting.openwisp_monitoring/views.py (2)
58-63: LGTM: Clean integration of timezone normalization.The flow correctly captures the raw timezone value, normalizes it via
normalize_timezone(), then validates with pytz. This ensures the fallback timezone is also validated before use.
17-17: Import placement is appropriate.The import from
.utilsfollows the existing pattern of local imports in the file.openwisp_monitoring/device/tests/test_api.py (1)
643-656: LGTM: Test correctly verifies fallback behavior.The renamed test and updated assertions properly validate that:
- Invalid timezone inputs return 200 (not 400/500)
- Response contains charts data, confirming the query executed with fallback timezone
- SQL injection attempts in timezone are handled safely
openwisp_monitoring/monitoring/tests/test_api.py (1)
682-697: LGTM: Legacy timezone test correctly validates normalization.The test for
Asia/Calcuttaproperly verifies that the deprecated timezone is normalized and the API returns a 200 response with valid chart data. The docstring clearly references the linked issue#728.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_monitoring/views.py (1)
9-10: 🧹 Nitpick | 🔵 TrivialMinor: Redundant timezone imports.
Both
timezoneandtimezone as tzare imported from pytz. Consider using only one alias consistently.♻️ Suggested cleanup
-from pytz import timezone -from pytz import timezone as tz +from pytz import timezone as tzThen update line 45 to use
tz(tmz)instead oftimezone(tmz)for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/views.py` around lines 9 - 10, Remove the duplicate pytz import and standardize on a single alias: keep "from pytz import timezone as tz" and delete "from pytz import timezone"; then update the usage site that currently calls timezone(tmz) (referencing the variable tmz and its call) to use tz(tmz) so the import and usage are consistent (adjust any other timezone(...) calls to tz(...) if present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/utils.py`:
- Around line 40-46: The lru_cache on normalize_timezone causes a falsy tz_name
(None/empty) to cache settings.TIME_ZONE, breaking tests that override
TIME_ZONE; fix by ensuring falsy inputs are returned without using the cache —
move the "if not tz_name: return settings.TIME_ZONE" check outside the cached
path (for example, keep a thin non-cached wrapper that handles falsy tz_name and
delegates to a cached inner function for real tz names) so normalize_timezone
still benefits from lru_cache for real tz strings but never caches the
settings-dependent fallback; reference normalize_timezone and its lru_cache
usage when applying the change.
- Around line 59-68: The code uses ZoneInfo directly and assumes system tzdata;
update imports to fall back to backports.zoneinfo on older Pythons (wrap the
import in a try/except and import ZoneInfo and ZoneInfoNotFoundError from
backports.zoneinfo if zoneinfo is unavailable) and ensure tzdata and
backports.zoneinfo are added to project dependencies (requirements.txt /
setup.py extras), and add a python_requires constraint in setup.py (e.g., set an
appropriate minimum like ">=3.8" or the project's supported minimum) so installs
on Python <3.9 get the backport and tzdata available; check usages in
openwisp_monitoring/utils.py where ZoneInfo/ZoneInfoNotFoundError are referenced
to confirm the new import is used.
---
Outside diff comments:
In `@openwisp_monitoring/views.py`:
- Around line 9-10: Remove the duplicate pytz import and standardize on a single
alias: keep "from pytz import timezone as tz" and delete "from pytz import
timezone"; then update the usage site that currently calls timezone(tmz)
(referencing the variable tmz and its call) to use tz(tmz) so the import and
usage are consistent (adjust any other timezone(...) calls to tz(...) if
present).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 050ee1cf-ce8d-49a0-abf8-e7a619cb14ef
📒 Files selected for processing (4)
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/utils.pyopenwisp_monitoring/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/views.pyopenwisp_monitoring/utils.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/device/tests/test_api.py
🔇 Additional comments (4)
openwisp_monitoring/utils.py (1)
48-55: Legacy timezone map noted as incomplete in prior review.The mapping covers only 5 common deprecated timezones. This was already flagged in a previous review with suggestions to expand the map or use pytz normalization.
openwisp_monitoring/views.py (1)
58-63: LGTM! Timezone normalization integration is correct.The two-step process (normalize then validate) ensures graceful fallback while maintaining backward compatibility with the existing pytz validation. If
normalize_timezonefalls back tosettings.TIME_ZONE, pytz validation should pass for any properly configured Django project.openwisp_monitoring/device/tests/test_api.py (1)
643-656: LGTM! Test correctly validates fallback behavior.The renamed test and updated assertions properly verify that:
- Invalid timezones no longer cause 400/500 errors
- The API gracefully falls back and returns 200 with chart data
- SQL injection attempts in timezone parameters are safely handled
This aligns well with the
normalize_timezoneimplementation that falls back tosettings.TIME_ZONEfor unrecognized inputs.openwisp_monitoring/monitoring/tests/test_api.py (1)
682-706: Test improvements look good; minor note on assertion fragility.The test correctly validates both legacy timezone normalization and invalid timezone fallback, with strengthened assertions per previous review feedback.
One potential fragility:
assertGreater(len(response.data["charts"]), 0)on line 706 assumes metrics/charts exist. If the test environment has no chart data, this could fail for reasons unrelated to timezone handling. Consider whether assertingchartsis a list (like line 697) is sufficient for the invalid timezone subtest, since the primary goal is verifying no 500 error.
Test Failures in OpenWISP Monitoring CIHello @UltraBot05, Test Failure:
|
Modified MonitoringApiViewMixin to handle UnknownTimeZoneError by falling back to the system default timezone. Added strict regression tests in device and monitoring modules to verify chart data availability on fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openwisp_monitoring/monitoring/tests/test_api.py (1)
699-706: 🧹 Nitpick | 🔵 TrivialMake invalid-timezone assertion as strict as the legacy-timezone assertion.
At Line 705 you only assert key presence; asserting list type here too keeps both subtests consistent and guards payload shape regressions.
✅ Tightened assertion
with self.subTest("Test invalid timezone fallback (Antarctica/Banana)"): response = self.client.get( path, {"timezone": "Antarctica/Banana", "time": "7d"} ) self.assertEqual(response.status_code, 200) self.assertIn("charts", response.data) + self.assertIsInstance(response.data["charts"], list)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/monitoring/tests/test_api.py` around lines 699 - 706, The invalid-timezone subtest currently only asserts that the "charts" key exists; make it as strict as the legacy-timezone subtest by also asserting that response.data["charts"] is a list. Update the assertion in the subtest that uses response = self.client.get(..., {"timezone": "Antarctica/Banana", "time": "7d"}) to check both status_code == 200 and that response.data["charts"] is a list (use the same type-check approach used in the legacy-timezone assertion so the two subtests remain consistent).openwisp_monitoring/utils.py (1)
40-76:⚠️ Potential issue | 🟠 MajorAvoid caching settings-dependent fallback values in
normalize_timezone.At Line 45-Line 46 and Line 70-Line 76, the result depends on
settings.TIME_ZONE, but the full function is cached bytz_name. This can leak stale values acrossoverride_settings(TIME_ZONE=...)test contexts.💡 Proposed fix
-@lru_cache(maxsize=128) def normalize_timezone(tz_name): """ Normalizes legacy timezones and verifies availability via zoneinfo. """ - if not tz_name: - return getattr(settings, "TIME_ZONE", "UTC") + fallback = getattr(settings, "TIME_ZONE", "UTC") + if not tz_name: + return fallback + return _normalize_timezone_cached(tz_name, fallback) + + +@lru_cache(maxsize=128) +def _normalize_timezone_cached(tz_name, fallback): + """ + Cached normalization for explicit timezone strings. + `fallback` is part of the cache key to avoid stale settings-dependent values. + """ # Mapping of legacy timezones legacy_map = { "Asia/Calcutta": "Asia/Kolkata", "Asia/Saigon": "Asia/Ho_Chi_Minh", @@ - # Fallback if above is false - fallback = getattr(settings, "TIME_ZONE", "UTC") + # Fallback if above is false logger.warning( f"Timezone '{normalized_tz}' (original: '{tz_name}') not found in OS zoneinfo. " f"Falling back to system TIME_ZONE '{fallback}'." ) return fallback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/utils.py` around lines 40 - 76, The normalize_timezone function is currently decorated with `@lru_cache`(maxsize=128) but its return can depend on settings.TIME_ZONE (when tz_name is falsy or when fallback is used), which leaks stale values across test contexts; remove the `@lru_cache` decorator from normalize_timezone so results are computed per-call, or if you must cache, change the cache key to include the current settings.TIME_ZONE (e.g. implement a custom cache that keys on (tz_name, settings.TIME_ZONE)) and ensure ZoneInfo/ZoneInfoNotFoundError logic and the legacy_map lookup remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/device/tests/test_api.py`:
- Around line 651-656: Wrap each iteration over wrong_timezone_values in a
unittest subTest to make failures explicit: inside the loop around the call to
self._url(d.pk, d.key) and the subsequent self.client.get(url) use
self.subTest(timezone=tz_value) so assertions on response.status_code and
response.data (including response.data["charts"]) are reported per timezone
value.
---
Duplicate comments:
In `@openwisp_monitoring/monitoring/tests/test_api.py`:
- Around line 699-706: The invalid-timezone subtest currently only asserts that
the "charts" key exists; make it as strict as the legacy-timezone subtest by
also asserting that response.data["charts"] is a list. Update the assertion in
the subtest that uses response = self.client.get(..., {"timezone":
"Antarctica/Banana", "time": "7d"}) to check both status_code == 200 and that
response.data["charts"] is a list (use the same type-check approach used in the
legacy-timezone assertion so the two subtests remain consistent).
In `@openwisp_monitoring/utils.py`:
- Around line 40-76: The normalize_timezone function is currently decorated with
`@lru_cache`(maxsize=128) but its return can depend on settings.TIME_ZONE (when
tz_name is falsy or when fallback is used), which leaks stale values across test
contexts; remove the `@lru_cache` decorator from normalize_timezone so results are
computed per-call, or if you must cache, change the cache key to include the
current settings.TIME_ZONE (e.g. implement a custom cache that keys on (tz_name,
settings.TIME_ZONE)) and ensure ZoneInfo/ZoneInfoNotFoundError logic and the
legacy_map lookup remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96c34339-4dba-48bb-9785-b77a84bc9495
📒 Files selected for processing (4)
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/utils.pyopenwisp_monitoring/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/utils.pyopenwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/views.py
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/monitoring/tests/test_api.py
🔇 Additional comments (1)
openwisp_monitoring/views.py (1)
58-59: Good normalization entry point before chart query flow.Normalizing
raw_timezoneat Line 59 before the rest of request processing is the right integration point and aligns with the fallback behavior expected by the regression tests.
CI Failure AnalysisHello @UltraBot05, Test FailuresFailure ExplanationThe CI pipeline failed due to a "double free or corruption (fasttop)" error, which is a critical memory corruption issue. This type of error typically indicates a bug in how memory is being managed, likely within the C extensions or underlying libraries that Python is using. The traceback shows the error occurring during the closing of database connections in Django, specifically involving SQLite. The presence of RemediationTo address this, the following steps are recommended:
|
Test Failures DetectedHello @UltraBot05, There are test failures in the CI log. Failure 1: Test Failure - "double free or corruption (fasttop)" and "Fatal Python error: Aborted"Explanation: Remediation:
Failure 2: Test Failure - Multiple Slow TestsExplanation: Remediation:
The |
|
I made this PR to fix the I initially used All CI checks are now passing. I've addressed the CodeRabbit feedback by implementing |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I am fine with this approach, I left some comments below.
…ed validation behavior
Test Failure in openwisp_monitoring.monitoring.tests.test_apiHello @UltraBot05, Test Failure: ImportError in
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/legacy_tz_utils.py`:
- Around line 33-35: The fallback resolution can be None when settings.TIME_ZONE
is explicitly set to None, causing normalize_timezone() to return None; update
the fallback logic (the variable named fallback in this module) to coalesce a
None value to "UTC" (e.g., use a fallback = getattr(settings, "TIME_ZONE",
"UTC") and then if fallback is None: fallback = "UTC", or use an or-style
coalesce) so normalize_timezone() always receives a valid timezone string when
tz_name is falsy.
In `@openwisp_monitoring/monitoring/tests/test_api.py`:
- Line 10: The test imports normalize_timezone from openwisp_monitoring.utils
but production code uses openwisp_monitoring.legacy_tz_utils.normalize_timezone;
update the import in the test (test_api.py) to import normalize_timezone from
openwisp_monitoring.legacy_tz_utils so the test exercises the same
implementation used by openwisp_monitoring.views and remains direct and
accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5716c005-a054-42f7-a657-916c42ff180a
📒 Files selected for processing (4)
openwisp_monitoring/device/tests/test_api.pyopenwisp_monitoring/legacy_tz_utils.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/views.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/legacy_tz_utils.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/views.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/legacy_tz_utils.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/views.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/legacy_tz_utils.pyopenwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/views.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/device/tests/test_api.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/monitoring/tests/test_api.pyopenwisp_monitoring/device/tests/test_api.py
🔇 Additional comments (3)
openwisp_monitoring/views.py (1)
58-59: Timezone normalization is correctly wired into request handling.This keeps deprecated aliases from reaching backend query generation while preserving existing validation for truly invalid values.
openwisp_monitoring/device/tests/test_api.py (1)
657-665: Good focused regression test for legacy timezone fallback.The assertions validate successful execution of the fallback path without coupling to dataset-specific chart values.
openwisp_monitoring/monitoring/tests/test_api.py (1)
683-715: The new timezone regression tests are solid and focused.They cover both deprecated alias handling and fallback behavior across settings overrides without over-constraining payload content.
|
Hey @nemesifier. I've Successfully implemented your changes, and the following bot requests too. now the CIs are passing. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai I have addressed your concerns, please review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainThanks for the update, 🏁 Script executed: Length of output: 15895
1. 🐛 Test assertion checks the wrong thing (monitoring/tests/test_api.py)In self.assertIn("Unkown Time Zone", response.data)
# Option A – exact match on the detail field:
self.assertEqual(response.data["detail"], "Unkown Time Zone")
# Option B – substring check inside the detail value:
self.assertIn("Unkown Time Zone", str(response.data["detail"]))2.
|
2 participants
Modified
MonitoringApiViewMixinto handleUnknownTimeZoneErrorby falling back to the system default timezone. Added strict regression tests in device and monitoring modules to verify chart data availability on fallback.Checklist
Reference to Existing Issue
Closes #728.
Description of Changes
This PR fixes an issue where the monitoring API would fail with a
400 Bad Requestwhen provided an invalidtimezonestring.MonitoringApiViewMixinto catchUnknownTimeZoneErrorand default to the system'sTIME_ZONEinstead of breaking the request.test_get_device_metrics_bad_timezone_fallback(in both device and monitoring tests). The tests now explicitly verify thatresponse.data["charts"]is populated with data, proving the fallback query executes successfully.Screenshot
earlier vs after