-
Notifications
You must be signed in to change notification settings - Fork 46
feat(events): store short timezone label on Buzz Event #277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d858059
1318210
3a6130f
e22bb8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import frappe | ||
| from frappe.utils.data import get_datetime | ||
|
|
||
| from buzz.utils import get_time_zone_label | ||
|
|
||
|
|
||
| def execute(): | ||
| events = frappe.get_all( | ||
| "Buzz Event", | ||
| filters={ | ||
| "time_zone": ("is", "set"), | ||
| "start_date": ("is", "set"), | ||
| "start_time": ("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) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,12 @@ | ||
| import functools | ||
| import re | ||
| from collections.abc import Callable | ||
| from datetime import datetime | ||
| from zoneinfo import ZoneInfo, ZoneInfoNotFoundError | ||
|
|
||
| import frappe | ||
| from frappe.custom.doctype.custom_field.custom_field import create_custom_fields | ||
| from frappe.utils import now_datetime | ||
|
|
||
|
|
||
| def is_app_installed(app_name: str) -> bool: | ||
|
|
@@ -189,3 +193,84 @@ def generate_ics_file(event_doc, attendee_email: str): | |
|
|
||
| # nosemgrep: frappe-semgrep-rules.rules.security.frappe-ssti | ||
| return frappe.render_template("templates/ics/ics.jinja2", context, is_path=True) | ||
|
|
||
|
|
||
| # Curated abbreviations for zones where tzdata only provides a numeric offset | ||
| # (tzdata dropped invented abbreviations in 2017). Zones with real tzdata | ||
| # abbreviations (IST, EST, CET, ...) never reach this map. | ||
| # ponytail: DST-observing zones here (e.g. Chile) are pinned to their standard | ||
| # form; extend get_time_zone_label with per-date variants if that ever matters. | ||
| TIMEZONE_ABBREVIATIONS = { | ||
| "America/Araguaina": "BRT", | ||
| "America/Argentina/Buenos_Aires": "ART", | ||
| "America/Bogota": "COT", | ||
| "America/Caracas": "VET", | ||
| "America/Godthab": "WGT", | ||
| "America/Nuuk": "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/Ho_Chi_Minh": "ICT", | ||
| "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", | ||
|
Comment on lines
+208
to
+239
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
|
|
||
| def get_time_zone_label(time_zone: str | None, reference_datetime: datetime | None = None) -> str: | ||
| """Short display label for an IANA time zone, e.g. "IST", "GST", "GMT+5:45". | ||
|
|
||
| Resolution order: tzdata abbreviation for the reference date (DST-aware), | ||
| then the curated map, then a formatted GMT offset. | ||
| """ | ||
| if not time_zone: | ||
| return "" | ||
|
|
||
| try: | ||
| zone = ZoneInfo(time_zone) | ||
| except (ZoneInfoNotFoundError, ValueError): | ||
| return "" | ||
|
|
||
| reference = reference_datetime or now_datetime() | ||
| if reference.tzinfo: | ||
| moment = reference.astimezone(zone) | ||
| else: | ||
| moment = reference.replace(tzinfo=zone) | ||
|
|
||
| abbreviation = moment.tzname() | ||
| if re.fullmatch(r"[A-Z]{2,5}", abbreviation): | ||
| return abbreviation | ||
|
|
||
| if time_zone in TIMEZONE_ABBREVIATIONS: | ||
| return TIMEZONE_ABBREVIATIONS[time_zone] | ||
|
|
||
| total_minutes = int(moment.utcoffset().total_seconds()) // 60 | ||
| sign = "+" if total_minutes >= 0 else "-" | ||
| hours, minutes = divmod(abs(total_minutes), 60) | ||
| label = f"GMT{sign}{hours}" | ||
| if minutes: | ||
| label += f":{minutes:02d}" | ||
| return label | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start_date/start_timeThe filter only requires
time_zoneto be set, but notstart_dateorstart_time. If any event in the database hastime_zonepopulated but those fields null,get_datetime("None None")is called — which either returnsNone(causingget_time_zone_labelto fall back tonow_datetime()and potentially stamp the wrong DST abbreviation) or raises, aborting the migration mid-run and leaving the backfill incomplete.set_time_zone_labelinbuzz_event.pyexplicitly guards against this same combination withif 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).