feat(events): store short timezone label on Buzz Event#277
Conversation
Buzz Event time_zone holds IANA names (Asia/Dubai) which read poorly on public pages, so the dashboard hid them. Saving an event now derives a short display label (GST, IST, EDT, or GMT+x fallback) into a new read-only time_zone_label field, computed at the event start datetime so DST zones get the abbreviation in effect then. Resolution order: tzdata abbreviation when it ships a real one, curated map for zones tzdata reduced to numeric offsets in 2017, formatted GMT offset as safety net. Dashboard event header shows the label again; guest booking and custom form APIs expose it; a patch backfills existing events. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Greptile SummaryThis PR stores a short timezone display label (e.g. "GST", "IST", "GMT+5:45") on Buzz Event to replace the raw IANA name that was previously hidden on public pages. The implementation uses a three-tier resolution: tzdata abbreviation (DST-aware via reference date), a 34-entry curated map for zones tzdata reduced to numeric offsets, and a formatted GMT offset fallback.
Confidence Score: 4/5Safe to merge after addressing the patch null-guard; the doctype logic and APIs are solid and well-tested. The core label-computation logic and doctype integration are well-designed and guarded — buzz/patches/set_time_zone_label_for_existing_events.py needs the same null-guard present in buzz_event.py; buzz/utils.py TIMEZONE_ABBREVIATIONS should add current canonical aliases for deprecated zone names.
|
| Filename | Overview |
|---|---|
| buzz/utils.py | Adds get_time_zone_label with three-tier resolution (tzdata → curated map → GMT offset); curated map contains deprecated IANA names without modern aliases, and replace(tzinfo=…) can mishandle aware datetimes |
| buzz/patches/set_time_zone_label_for_existing_events.py | Backfill patch for existing events; missing null-guard for start_date/start_time unlike the corresponding set_time_zone_label method, which could abort the migration or stamp wrong DST labels |
| buzz/events/doctype/buzz_event/buzz_event.py | Adds set_time_zone_label called from validate; correctly guards against missing dates and computes label at event-start time for DST accuracy |
| buzz/events/doctype/buzz_event/test_buzz_event.py | 9 new tests covering tzdata direct, DST variants, curated map, GMT fallback, invalid inputs, and controller persistence; good coverage of the main code paths |
| buzz/api/init.py | Adds time_zone_label to the guest booking data response; straightforward field addition |
| buzz/api/forms.py | Adds time_zone_label to the custom form data response; straightforward field addition |
| buzz/events/doctype/buzz_event/buzz_event.json | Adds time_zone_label as a read-only Data field in the doctype schema, positioned after time_zone |
| dashboard/src/components/EventDetailsHeader.vue | Switches the header display from the raw IANA time_zone string to the new time_zone_label short form; correct conditional guard updated accordingly |
| dashboard/src/types/Events/BuzzEvent.ts | Adds time_zone_label?: string to the TypeScript interface; straightforward type addition |
| buzz/patches.txt | Registers the new backfill patch; also fixes the missing trailing newline from the previous entry |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
buzz/patches/set_time_zone_label_for_existing_events.py:7-16
**Patch missing null-guard for `start_date`/`start_time`**
The filter only requires `time_zone` to be set, but not `start_date` or `start_time`. If any event in the database has `time_zone` populated but those fields null, `get_datetime("None None")` is called — which either returns `None` (causing `get_time_zone_label` to fall back to `now_datetime()` and potentially stamp the wrong DST abbreviation) or raises, aborting the migration mid-run and leaving the backfill incomplete. `set_time_zone_label` in `buzz_event.py` explicitly guards against this same combination with `if not (self.time_zone and self.start_date and self.start_time)` — the patch should apply the same defence, either by adding both fields to the filter or by skipping events where they are absent inside the loop.
### Issue 2 of 3
buzz/utils.py:207-236
**Curated map uses deprecated IANA names without their current aliases**
`America/Godthab` was renamed to `America/Nuuk` in tzdata 2020b, and `Asia/Saigon` was superseded by `Asia/Ho_Chi_Minh`. Both old names still work as backward-compatibility aliases in `ZoneInfo`, so events that stored the old name resolve correctly — but if a user selects the current IANA name (e.g. `America/Nuuk` from a modern timezone picker), the curated-map lookup misses and falls through to the GMT offset format (`GMT-3` instead of `WGT`). Adding the current canonical names as parallel entries (similar to how `Asia/Dacca` and `Asia/Dhaka` both appear for BST) would keep both spellings covered.
### Issue 3 of 3
buzz/utils.py:254
**`replace(tzinfo=…)` incorrectly handles an already-aware `reference_datetime`**
If a caller passes an aware datetime (one that already has `tzinfo` set, e.g. a UTC-aware value from some frappe utilities), `.replace(tzinfo=zone)` discards the existing offset without converting — the wall-clock time is silently reinterpreted as being in the event's timezone. For DST zones this can flip the chosen abbreviation (e.g. UTC 02:00 reinterpreted as local 02:00 in America/New_York might land on the wrong side of the DST boundary). The public signature accepts any `datetime`, so a comment or an assertion that `reference_datetime` must be naïve, or a `.replace(tzinfo=None)` strip before attaching the zone, would prevent a subtle misuse.
Reviews (1): Last reviewed commit: "feat(events): store short timezone label..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def execute(): | ||
| events = frappe.get_all( | ||
| "Buzz Event", | ||
| filters={"time_zone": ("is", "set")}, | ||
| fields=["name", "time_zone", "start_date", "start_time"], | ||
| ) | ||
| for event in events: | ||
| event_start = get_datetime(f"{event.start_date} {event.start_time}") | ||
| label = get_time_zone_label(event.time_zone, event_start) | ||
| frappe.db.set_value("Buzz Event", event.name, "time_zone_label", label, update_modified=False) |
There was a problem hiding this comment.
start_date/start_time
The filter only requires time_zone to be set, but not start_date or start_time. If any event in the database has time_zone populated but those fields null, get_datetime("None None") is called — which either returns None (causing get_time_zone_label to fall back to now_datetime() and potentially stamp the wrong DST abbreviation) or raises, aborting the migration mid-run and leaving the backfill incomplete. set_time_zone_label in buzz_event.py explicitly guards against this same combination with if not (self.time_zone and self.start_date and self.start_time) — the patch should apply the same defence, either by adding both fields to the filter or by skipping events where they are absent inside the loop.
Prompt To Fix With AI
This is a comment left during a code review.
Path: buzz/patches/set_time_zone_label_for_existing_events.py
Line: 7-16
Comment:
**Patch missing null-guard for `start_date`/`start_time`**
The filter only requires `time_zone` to be set, but not `start_date` or `start_time`. If any event in the database has `time_zone` populated but those fields null, `get_datetime("None None")` is called — which either returns `None` (causing `get_time_zone_label` to fall back to `now_datetime()` and potentially stamp the wrong DST abbreviation) or raises, aborting the migration mid-run and leaving the backfill incomplete. `set_time_zone_label` in `buzz_event.py` explicitly guards against this same combination with `if not (self.time_zone and self.start_date and self.start_time)` — the patch should apply the same defence, either by adding both fields to the filter or by skipping events where they are absent inside the loop.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed in 1318210 — patch filters now require start_date and start_time set, matching the controller guard. Regression test added (legacy row with null start_time no longer aborts the run).
| "America/Godthab": "WGT", | ||
| "America/Lima": "PET", | ||
| "America/Montevideo": "UYT", | ||
| "America/Santiago": "CLT", | ||
| "America/Sao_Paulo": "BRT", | ||
| "Asia/Aden": "AST", | ||
| "Asia/Almaty": "ALMT", | ||
| "Asia/Baghdad": "AST", | ||
| "Asia/Bahrain": "AST", | ||
| "Asia/Baku": "AZT", | ||
| "Asia/Bangkok": "ICT", | ||
| "Asia/Dacca": "BST", | ||
| "Asia/Dhaka": "BST", | ||
| "Asia/Dubai": "GST", | ||
| "Asia/Irkutsk": "IRKT", | ||
| "Asia/Kabul": "AFT", | ||
| "Asia/Kathmandu": "NPT", | ||
| "Asia/Krasnoyarsk": "KRAT", | ||
| "Asia/Kuwait": "AST", | ||
| "Asia/Muscat": "GST", | ||
| "Asia/Novosibirsk": "NOVT", | ||
| "Asia/Qatar": "AST", | ||
| "Asia/Riyadh": "AST", | ||
| "Asia/Saigon": "ICT", | ||
| "Asia/Tashkent": "UZT", | ||
| "Asia/Tehran": "IRST", | ||
| "Asia/Yekaterinburg": "YEKT", | ||
| "Atlantic/Azores": "AZOT", | ||
| "Atlantic/Cape_Verde": "CVT", | ||
| "Europe/Istanbul": "TRT", |
There was a problem hiding this comment.
America/Godthab was renamed to America/Nuuk in tzdata 2020b, and Asia/Saigon was superseded by Asia/Ho_Chi_Minh. Both old names still work as backward-compatibility aliases in ZoneInfo, so events that stored the old name resolve correctly — but if a user selects the current IANA name (e.g. America/Nuuk from a modern timezone picker), the curated-map lookup misses and falls through to the GMT offset format (GMT-3 instead of WGT). Adding the current canonical names as parallel entries (similar to how Asia/Dacca and Asia/Dhaka both appear for BST) would keep both spellings covered.
Prompt To Fix With AI
This is a comment left during a code review.
Path: buzz/utils.py
Line: 207-236
Comment:
**Curated map uses deprecated IANA names without their current aliases**
`America/Godthab` was renamed to `America/Nuuk` in tzdata 2020b, and `Asia/Saigon` was superseded by `Asia/Ho_Chi_Minh`. Both old names still work as backward-compatibility aliases in `ZoneInfo`, so events that stored the old name resolve correctly — but if a user selects the current IANA name (e.g. `America/Nuuk` from a modern timezone picker), the curated-map lookup misses and falls through to the GMT offset format (`GMT-3` instead of `WGT`). Adding the current canonical names as parallel entries (similar to how `Asia/Dacca` and `Asia/Dhaka` both appear for BST) would keep both spellings covered.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Added America/Nuuk and Asia/Ho_Chi_Minh entries in 1318210. Note the field's options come from the Zoom-supported list in buzz_event.js (legacy names only), so the new spellings aren't reachable from the picker today — entries added for robustness, tested.
| except (ZoneInfoNotFoundError, ValueError): | ||
| return "" | ||
|
|
||
| moment = (reference_datetime or now_datetime()).replace(tzinfo=zone) |
There was a problem hiding this comment.
replace(tzinfo=…) incorrectly handles an already-aware reference_datetime
If a caller passes an aware datetime (one that already has tzinfo set, e.g. a UTC-aware value from some frappe utilities), .replace(tzinfo=zone) discards the existing offset without converting — the wall-clock time is silently reinterpreted as being in the event's timezone. For DST zones this can flip the chosen abbreviation (e.g. UTC 02:00 reinterpreted as local 02:00 in America/New_York might land on the wrong side of the DST boundary). The public signature accepts any datetime, so a comment or an assertion that reference_datetime must be naïve, or a .replace(tzinfo=None) strip before attaching the zone, would prevent a subtle misuse.
Prompt To Fix With AI
This is a comment left during a code review.
Path: buzz/utils.py
Line: 254
Comment:
**`replace(tzinfo=…)` incorrectly handles an already-aware `reference_datetime`**
If a caller passes an aware datetime (one that already has `tzinfo` set, e.g. a UTC-aware value from some frappe utilities), `.replace(tzinfo=zone)` discards the existing offset without converting — the wall-clock time is silently reinterpreted as being in the event's timezone. For DST zones this can flip the chosen abbreviation (e.g. UTC 02:00 reinterpreted as local 02:00 in America/New_York might land on the wrong side of the DST boundary). The public signature accepts any `datetime`, so a comment or an assertion that `reference_datetime` must be naïve, or a `.replace(tzinfo=None)` strip before attaching the zone, would prevent a subtle misuse.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 1318210 — aware inputs now go through astimezone() (correct conversion) instead of replace(); naive inputs unchanged. Test covers the DST-boundary case (05:30 UTC on 2026-11-01 → EDT).
Backfill patch now skips rows missing start_date/start_time instead of aborting on unparseable dates. get_time_zone_label converts aware reference datetimes into the target zone rather than reinterpreting the wall clock. Curated map covers current IANA names for renamed zones (America/Nuuk, Asia/Ho_Chi_Minh). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
harshtandiya has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Successfully created backport PR for |
1 participant
Why
time_zonestores IANA names ("Asia/Dubai") which read poorly next to event times, so the dashboard hid the timezone entirely. Public pages need a short label like "GST", "IST", or "GMT+4".What
buzz/utils.py—get_time_zone_label(time_zone, reference_datetime):TIMEZONE_ABBREVIATIONSmap (34 entries) for zones tzdata reduced to numeric offsets in 2017 (GST, AST, ICT, NPT…)time_zone_labelData field, set invalidateat the event start datetime; guarded against missing dates (validate runs before the mandatory check)(GST)style label againdb_set, no hooks)Tests
9 new tests (TDD): tzdata direct, DST variant by reference date, curated map, GMT fallback incl. half-hour and negative offsets, empty/invalid zones, and controller persistence on insert/update/clear/DST. Full app suite green (220 tests).
🤖 Generated with Claude Code