feat: normalize datetimes with deployment time zones #1075
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdd a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🪛 Ruff (0.14.8)ami/main/migrations/0079_deployment_time_zone.py7-9: Mutable class attributes should be annotated with (RUF012) 11-21: Mutable class attributes should be annotated with (RUF012) ami/main/models.py66-66: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (4)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds deployment-specific time zone support to normalize datetime fields when saving Event, SourceImage, Detection, and Classification records. The implementation adds a time_zone field to deployments with IANA validation and updates datetime fields to be stored in UTC while respecting each deployment's configured time zone for interpretation of naive datetimes.
Key Changes:
- Added
time_zonefield to Deployment model with IANA validation - Implemented pre-save signal handlers to normalize datetime fields to UTC based on deployment time zones
- Exposed
time_zonefield through deployment serializers for API access
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ami/main/migrations/0079_deployment_time_zone.py | Adds the time_zone field to Deployment model with default from settings |
| ami/main/models.py | Adds time_zone field with validation logic to ensure valid IANA time zone identifiers |
| ami/main/signals.py | Implements pre-save signal handler to normalize datetime fields across Event, SourceImage, Detection, and Classification models |
| ami/main/api/serializers.py | Exposes time_zone field in deployment serializers for API clients |
| ami/main/tests.py | Adds basic unit tests for time zone validation, serializer exposure, and UTC normalization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ami/main/models.py (1)
609-613: Tighten time_zone validation exception handlingThe
time_zonefield andclean()logic look functionally correct, but the validation currently catches a broadExceptionand loses the original traceback.Consider narrowing and chaining the exception instead:
-from django.core.exceptions import ValidationError +from django.core.exceptions import ValidationError +from zoneinfo import ZoneInfo, ZoneInfoNotFoundError @@ def clean(self): super().clean() if self.time_zone: try: - from zoneinfo import ZoneInfo # Local import to avoid module load cost when unused - ZoneInfo(self.time_zone) - except Exception as exc: - raise ValidationError({"time_zone": f"Invalid IANA time zone '{self.time_zone}': {exc}"}) + except ZoneInfoNotFoundError as exc: + raise ValidationError( + {"time_zone": f"Invalid IANA time zone '{self.time_zone}': {exc}"} + ) from excThis keeps behavior the same while avoiding a blind catch and preserves the root cause for debugging.
Also applies to: 670-678
ami/main/tests.py (2)
52-82: Time zone normalization tests are well‑targeted; consider a couple of robustness tweaksThe three tests nicely cover:
- Validation of invalid
Deployment.time_zone.- Exposure of
time_zonevia a minimalDeploymentSerializer.- Pre‑save normalization of
Eventdatetimes to UTC using the deployment’s IANA zone whenUSE_TZ=True.Two optional improvements you might consider:
- In
test_event_presave_normalizes_with_deployment_tz, assert the full normalizeddatetime(including date and minute/second) rather than onlyhour, to lock in exact behavior.- Add a complementary test for
USE_TZ=Falseto ensure signals leave datetimes unchanged in that configuration.These are non‑blocking; current tests already exercise the main behavior.
62-65: Use an immutable tuple forMeta.fieldsto satisfy RUF012 and avoid mutable class attributesRuff’s RUF012 warning about mutable class attributes likely targets
fields = ["id", "time_zone"]in the innerMetaclass. In DRF serializers, it's common (and sufficient) to makefieldsa tuple so it’s immutable rather than annotating withClassVar.You can change this to:
- class Meta(DeploymentSerializer.Meta): - fields = ["id", "time_zone"] + class Meta(DeploymentSerializer.Meta): + fields = ("id", "time_zone")Functionality remains identical, but the linter warning should go away and it makes the intent of
fieldsbeing constant clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ami/main/api/serializers.py(3 hunks)ami/main/migrations/0079_deployment_time_zone.py(1 hunks)ami/main/models.py(2 hunks)ami/main/signals.py(2 hunks)ami/main/tests.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
ami/main/tests.py (2)
ami/main/api/serializers.py (2)
DeploymentSerializer(442-520)Meta(40-47)ami/main/models.py (19)
Project(228-468)name(1021-1022)Deployment(599-931)Meta(416-468)Meta(485-486)Meta(522-523)Meta(667-668)Meta(1008-1016)Meta(1915-1928)Meta(2162-2165)Meta(2317-2318)Meta(2524-2528)Meta(2890-2907)Meta(3485-3499)Meta(3524-3526)Meta(3538-3539)Meta(3556-3558)Meta(3578-3579)Event(977-1156)
ami/main/signals.py (1)
ami/main/models.py (4)
Classification(2266-2410)Detection(2414-2591)Event(977-1156)SourceImage(1680-1928)
ami/main/models.py (1)
ami/base/serializers.py (2)
clean(121-146)clean(179-182)
🪛 Ruff (0.14.7)
ami/main/tests.py
64-64: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
ami/main/signals.py
134-134: Unused function argument: kwargs
(ARG001)
ami/main/models.py
677-677: Do not catch blind exception: Exception
(BLE001)
678-678: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
ami/main/migrations/0079_deployment_time_zone.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (3)
ami/main/migrations/0079_deployment_time_zone.py (1)
1-21: Migration matches model change and looks correctThe migration cleanly adds
Deployment.time_zonewith the same default, max_length, and help_text as in the model, and the dependency chain is consistent. No issues from a schema or rollout perspective.ami/main/api/serializers.py (1)
170-195: Consistent exposure of deployment time_zone in serializersIncluding
"time_zone"inDeploymentListSerializer,DeploymentNestedSerializer, andDeploymentNestedSerializerWithLocationAndCountskeeps the API surface aligned with the new model field and makes the per‑deployment TZ visible wherever deployments are embedded. This looks consistent and non-breaking.Also applies to: 231-239, 243-258
ami/main/tests.py (1)
7-20: New imports are appropriate and correctly scoped
ValidationError,timezone, andDeploymentSerializerare all used in the new tests and imported from the right modules; no issues here.
wietzesuijker
force-pushed
the
feat/timezone-normalization
branch
2 times, most recently
from
3450aa4 to
2bf941f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ami/main/signals.py (1)
143-161: Datetime normalization logic is correct and robust; consider tiny defensive tweakThe signal handler:
- Guards on
settings.USE_TZ, avoiding surprises when time zones are disabled.- Uses the deployment’s
time_zonewhen available, or falls back totimezone.get_default_timezone_name(), then totimezone.get_default_timezone()onZoneInfoNotFoundError.- Interprets naive datetimes as local to the resolved tz via
timezone.make_aware(value, tz).- Converts both naive+aware values to UTC via
value.astimezone(timezone.utc)before saving.This matches the PR intent (store UTC, interpret naive values in deployment tz) and should behave correctly even when deployment is
Noneor has an empty/invalidtime_zone. Thegetattr(..., None) or timezone.get_default_timezone_name()idiom avoids the earlierZoneInfo(None)issue.If you want an extra layer of safety against unexpected
TypeErrorcoming fromZoneInfo(...)(e.g., in case of a misconfigured default timezone), you could broaden the exception slightly:- try: - tz = ZoneInfo(getattr(deployment, "time_zone", None) or timezone.get_default_timezone_name()) - except ZoneInfoNotFoundError: - tz = timezone.get_default_timezone() + try: + tz = ZoneInfo(getattr(deployment, "time_zone", None) or timezone.get_default_timezone_name()) + except (ZoneInfoNotFoundError, TypeError): + tz = timezone.get_default_timezone()Not required, but a low-cost defensive enhancement.
ami/main/migrations/0079_deployment_time_zone.py (1)
5-21: Deployment.time_zone migration matches model intent; linter warning is non-blockingThe migration cleanly adds
deployment.time_zoneas aCharField(max_length=64, default=settings.TIME_ZONE, help_text=...), which aligns with the Deployment model and the PR goal of using a deployment-specific IANA timezone with a settings-based default. The dependency on0078_classification_applied_tois consistent with the model history.Ruff’s RUF012 warning about annotating
dependencies/operationsasClassVaris purely stylistic here; migrations are normally left as-is. You can either ignore it or quiet it in your lint config if it’s noisy.ami/main/tests.py (1)
52-86: Timezone normalization tests align with intended behavior; consider future extensionsThe three new tests do what this PR needs:
test_deployment_invalid_time_zone_raisescorrectly expectsDeployment.full_clean()to raiseValidationErrorfor an obviously invalid IANA name ("Mars/Phobos"), exercising the model’s ZoneInfo-based validation.test_deployment_serializer_exposes_time_zoneverifies thattime_zoneis present and correctly serialized via a minimal wrapper aroundDeploymentSerializer, using a realistic request context.test_event_presave_normalizes_with_deployment_tzchecks that naive Eventstart/endvalues for anAmerica/New_Yorkdeployment are stored as UTC-aware datetimes with the expected shifted values (00:00/01:00 EST → 05:00/06:00 UTC) whenUSE_TZ=True, including explicittzinfo == timezone.utcassertions.This provides solid regression coverage for the new normalization behavior. If you want to expand coverage in a follow-up, natural candidates would be:
- Similar normalization tests for
SourceImage.timestamp/last_modified,Detection.timestamp/detection_time, andClassification.timestamp.- Behavior when deployment is
Noneordeployment.time_zoneis unset, and when an already-aware datetime is provided.- An explicit
USE_TZ=Falsetest that confirms normalization is skipped.None of that is blocking for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ami/main/api/serializers.py(3 hunks)ami/main/migrations/0079_deployment_time_zone.py(1 hunks)ami/main/models.py(3 hunks)ami/main/signals.py(2 hunks)ami/main/tests.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ami/main/models.py
- ami/main/api/serializers.py
🧰 Additional context used
🧬 Code graph analysis (1)
ami/main/signals.py (1)
ami/main/models.py (5)
Classification(2265-2409)Detection(2413-2590)Event(976-1155)Project(229-469)SourceImage(1679-1927)
🪛 Ruff (0.14.7)
ami/main/migrations/0079_deployment_time_zone.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-21: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (5)
ami/main/signals.py (4)
2-14: Imports and model coverage for TZ normalization look consistentThe added imports (ZoneInfo, ZoneInfoNotFoundError, settings, timezone, and the Event/SourceImage/Detection/Classification models) are appropriate for the new TZ normalization logic, and they align with the models/fields referenced later in the file. No issues here.
118-123: _TZ_FIELDS mapping matches model fields and intended behaviorThe
_TZ_FIELDSmapping cleanly centralizes which datetime fields are normalized per model, and all listed field names exist on the corresponding models (Event.start/end/calculated_fields_updated_at, SourceImage.timestamp/last_modified, Detection.timestamp/detection_time, Classification.timestamp). This keeps the signal handler generic and easy to extend later.
126-140: Deployment resolution logic now correctly handles Classification via Detection chain
_resolve_deployment()now walks:
instance.deployment(Event, SourceImage, maybe others)instance.source_image.deployment(Detection)instance.detection.source_image.deployment(Classification)and returns
Noneotherwise. This fixes the earlier gap for Classification while safely handling missing relations viagetattr. The fallback toNoneis sensible, allowing_normalize_datetimesto fall back to the default timezone.
163-164: Signal registration is appropriate and idempotentLooping over
_TZ_FIELDSto connect_normalize_datetimeswithdispatch_uid=f"tznorm-{model.__name__}"andweak=Falseis a solid pattern: it ensures one receiver per model even if the module is imported multiple times, and avoids receiver GC issues.ami/main/tests.py (1)
7-21: New imports correctly support TZ testsThe added imports (
ValidationError,timezone, andDeploymentSerializer) are all used in the new TestTimeZoneNormalization tests and are appropriate for the new assertions and serializer behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wietzesuijker
force-pushed
the
feat/timezone-normalization
branch
3 times, most recently
from
b3d494c to
098e92f
Compare
|
Excellent! Thank you for your contribution @wietzesuijker! I stored the images in local times for this project since we generally want to compare moth behavior based on local time no matter the location or the time of year, e.g. what activity is there at 1AM in Oregon vs. 1AM in Germany? (solar time would be even better!). Perhaps we can store both local and UTC time in the DB? Normalizing to UTC is the standard, but I found it adds more chance for error when you are working with biological observations and I haven't found the utility. Did you have a particular motivation for normalizing to UTC? We likely need to track the timezone with the timestamp on each SourceImage record to account for daylight savings time (which offset is active when the image was taken). One safe initial feature could be to add the timezone field to the deployment, just to track that metadata with the location. But not modify the timestamps yet. I am very happy that you were able to navigate the code base and make this change! |
wietzesuijker
force-pushed
the
feat/timezone-normalization
branch
from
098e92f to
48185a7
Compare
|
Thanks @mihow!! I followed up on your feedback and force-pushed from 098e92f to 0a364bb: (diff) What changed vs the prior PR state (098e92f)
In scope for this PR
Out of scope / follow-ups
|
wietzesuijker
force-pushed
the
feat/timezone-normalization
branch
from
48185a7 to
0a364bb
Compare
2 participants
Summary
Normalize datetime saves using deployment-specific time zones and surface
time_zoneon deployments.List of Changes
time_zone(IANA) with validation and serializer exposure.time_zone(fallback: settings TZ; no-op whenUSE_TZ=False).0079_deployment_time_zoneadds the field.Related Issues
N/A
Note that I'm learning about Antenna and haven't worked on Django projects in a while. Please do dismiss if invalid / not relevant.
Detailed Description
The change enforces UTC storage while respecting each deployment's configured IANA
time_zone. On pre-save, date/time fields for Events, SourceImages, Detections, and Classifications are made aware in the deployment's zone and converted to UTC; whenUSE_TZ=False, values are left untouched. Deployments now carry atime_zonefield (defaulting to settings) validated against IANA entries and exposed via serializers to enable API clients/admins to set it. No historical backfill is performed.How to Test the Changes
Automated:
docker compose run --rm django python manage.py test ami.main.tests.TestTimeZoneNormalizationScreenshots
N/A
Deployment Notes
0079_deployment_time_zone(adds field, no data backfill).time_zoneon existing deployments via admin/API to improve future ingests.Checklist
Summary by CodeRabbit
New Features
Validation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.