feat: add per-deployment timezone metadata#1177
feat: add per-deployment timezone metadata#1177wietzesuijker wants to merge 1 commit intoRolnickLab:mainfrom
Conversation
👷 Deploy request for antenna-ssec pending review.Visit the deploys page to approve it
|
✅ Deploy Preview for antenna-preview canceled.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a new Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
ami/main/admin.py (1)
131-147:⚠️ Potential issue | 🟡 MinorAdd
time_zonetolist_filtertoo.This updates the admin list and search surfaces, but the PR objective says timezone should also be filterable.
list_filterat Line 186 is still only("project",), so admins still can't filter deployments by timezone from the sidebar.Suggested change
- list_filter = ("project",) + list_filter = ("project", "time_zone")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/admin.py` around lines 131 - 147, The admin configuration exposes time_zone in list_display and search_fields but not in list_filter, so add "time_zone" to the list_filter tuple (currently defined as list_filter = ("project",)) so admins can filter by timezone; update the list_filter declaration (in the same admin class where list_display/list_search are declared) to include "time_zone" (ensuring the value remains a proper tuple with commas as needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/main/models.py`:
- Around line 710-715: The time_zone field currently uses a dynamic default
(settings.TIME_ZONE) which can diverge from the migration backfill; change the
default on the time_zone CharField in ami/main/models.py (the time_zone field
definition) to the stable string used in the migration backfill (e.g.
"America/New_York") instead of settings.TIME_ZONE, preserving max_length,
validators (validate_time_zone) and help_text so runtime defaults match migrated
rows.
In `@ami/main/tests.py`:
- Around line 3822-3828: The test method test_api_list_includes_time_zone is
using the wrong query parameter; change the request URL to use the deployments
filter key project_id instead of project so the view's project-specific listing
path is exercised (update the url variable in test_api_list_includes_time_zone
to f"/api/v2/deployments/?project_id={self.project.pk}"). Ensure no other
assertions need adjustment and keep using self.client.get, response.status_code
check, and the time_zone assertion as-is.
---
Outside diff comments:
In `@ami/main/admin.py`:
- Around line 131-147: The admin configuration exposes time_zone in list_display
and search_fields but not in list_filter, so add "time_zone" to the list_filter
tuple (currently defined as list_filter = ("project",)) so admins can filter by
timezone; update the list_filter declaration (in the same admin class where
list_display/list_search are declared) to include "time_zone" (ensuring the
value remains a proper tuple with commas as needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a0d7a8d-b9bf-4274-aff0-ad45be98d543
📒 Files selected for processing (5)
ami/main/admin.pyami/main/api/serializers.pyami/main/migrations/0083_deployment_time_zone.pyami/main/models.pyami/main/tests.py
wietzesuijker
force-pushed
the
feat/deployment-timezone
branch
from
44d706d to
57f64b1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ami/main/tests.py (1)
3822-3828:⚠️ Potential issue | 🟡 MinorPin the list assertion to
self.deployment.Line 3828 only checks the first row, so this can still pass as long as any deployment is returned for the project. Assert against the row for
self.deployment.pkand verify its value.Suggested fix
def test_api_list_includes_time_zone(self): url = f"/api/v2/deployments/?project_id={self.project.pk}" response = self.client.get(url) self.assertEqual(response.status_code, status.HTTP_200_OK) results = response.data["results"] - self.assertTrue(len(results) > 0) - self.assertIn("time_zone", results[0]) + row = next((r for r in results if r["id"] == self.deployment.pk), None) + self.assertIsNotNone(row) + self.assertEqual(row["time_zone"], "America/New_York")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/tests.py` around lines 3822 - 3828, The test test_api_list_includes_time_zone currently asserts time_zone exists only on results[0], which can be some other deployment; instead locate the deployment row matching self.deployment.pk in the response.results and assert that that row contains the "time_zone" key and its expected value. Update the test to iterate/search results for an item where item["pk"] == self.deployment.pk (or item["id"] as used by the API) and then assert "time_zone" in that item and compare it to the expected value from self.deployment.time_zone.
🧹 Nitpick comments (2)
ami/main/migrations/0083_deployment_time_zone.py (1)
3-20: Freeze the validator under a stable module path before shipping this migration.This migration now depends on
ami.main.models.validate_time_zone, so a futuremodels.pysplit/rename will break fresh installs at migration0083. Since this repo is already moving model-related code intoami.main.models_future.*, I’d move the validator to a small stable module and point both the model field and the migration there now. Mirror the same import change inami/main/models.py.♻️ Suggested change
- import ami.main.models + from ami.main.validators import validate_time_zone from django.db import migrations, models ... - validators=[ami.main.models.validate_time_zone], + validators=[validate_time_zone],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/migrations/0083_deployment_time_zone.py` around lines 3 - 20, The migration currently references ami.main.models.validate_time_zone which will break if models.py is moved; extract the validate_time_zone function into a small stable module (e.g., ami.main.validators) and update both the model field definition that uses validate_time_zone and the migration that adds the time_zone field to import the validator from that stable module so the migration freezes a stable import path for validate_time_zone.ami/main/tests.py (1)
3792-3799: Assert serialized output, notMeta.fields.These tests currently check an implementation detail, so they can miss regressions in actual serializer behavior. Instantiate each serializer with
self.deploymentand assert the serialized payload contains the expectedtime_zone.Proposed refactor
def test_list_serializer_includes_time_zone(self): - self.assertIn("time_zone", DeploymentListSerializer.Meta.fields) + data = DeploymentListSerializer(instance=self.deployment).data + self.assertEqual(data["time_zone"], self.deployment.time_zone) def test_nested_serializer_includes_time_zone(self): - self.assertIn("time_zone", DeploymentNestedSerializer.Meta.fields) + data = DeploymentNestedSerializer(instance=self.deployment).data + self.assertEqual(data["time_zone"], self.deployment.time_zone) def test_nested_with_location_serializer_includes_time_zone(self): - self.assertIn("time_zone", DeploymentNestedSerializerWithLocationAndCounts.Meta.fields) + data = DeploymentNestedSerializerWithLocationAndCounts(instance=self.deployment).data + self.assertEqual(data["time_zone"], self.deployment.time_zone)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/tests.py` around lines 3792 - 3799, The tests are asserting Meta.fields rather than actual serialized output; update each test (test_list_serializer_includes_time_zone, test_nested_serializer_includes_time_zone, test_nested_with_location_serializer_includes_time_zone) to instantiate the appropriate serializer (DeploymentListSerializer, DeploymentNestedSerializer, DeploymentNestedSerializerWithLocationAndCounts) with self.deployment (and any required context), call serializer.data, and assert that "time_zone" is present in the resulting dict and equals the expected value (e.g., self.deployment.time_zone).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ami/main/tests.py`:
- Around line 3822-3828: The test test_api_list_includes_time_zone currently
asserts time_zone exists only on results[0], which can be some other deployment;
instead locate the deployment row matching self.deployment.pk in the
response.results and assert that that row contains the "time_zone" key and its
expected value. Update the test to iterate/search results for an item where
item["pk"] == self.deployment.pk (or item["id"] as used by the API) and then
assert "time_zone" in that item and compare it to the expected value from
self.deployment.time_zone.
---
Nitpick comments:
In `@ami/main/migrations/0083_deployment_time_zone.py`:
- Around line 3-20: The migration currently references
ami.main.models.validate_time_zone which will break if models.py is moved;
extract the validate_time_zone function into a small stable module (e.g.,
ami.main.validators) and update both the model field definition that uses
validate_time_zone and the migration that adds the time_zone field to import the
validator from that stable module so the migration freezes a stable import path
for validate_time_zone.
In `@ami/main/tests.py`:
- Around line 3792-3799: The tests are asserting Meta.fields rather than actual
serialized output; update each test (test_list_serializer_includes_time_zone,
test_nested_serializer_includes_time_zone,
test_nested_with_location_serializer_includes_time_zone) to instantiate the
appropriate serializer (DeploymentListSerializer, DeploymentNestedSerializer,
DeploymentNestedSerializerWithLocationAndCounts) with self.deployment (and any
required context), call serializer.data, and assert that "time_zone" is present
in the resulting dict and equals the expected value (e.g.,
self.deployment.time_zone).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 388e9656-4cbb-4d44-9254-fb83fa91b67b
📒 Files selected for processing (5)
ami/main/admin.pyami/main/api/serializers.pyami/main/migrations/0083_deployment_time_zone.pyami/main/models.pyami/main/tests.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ami/main/api/serializers.py
- ami/main/admin.py
wietzesuijker
force-pushed
the
feat/deployment-timezone
branch
from
57f64b1 to
b2c7991
Compare
1 participant
Summary
Add a
time_zonefield to Deployment so each station can record its IANA timezone. Metadata only — no timestamps are modified. Following up on #1075 and @mihow's suggestion to track timezone with the location first.Deployment.time_zoneCharField, validated againstzoneinfo.ZoneInfo, defaults to"America/New_York"Related Issues
Supersedes #1075 (closed).
How to Test the Changes
Deployment Notes
Migration
0083_deployment_time_zoneadds a column with a default — no downtime, no backfill.Checklist
Summary by CodeRabbit