From 0ab5312bf1cf068fd45db3700729e78130c505ea Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Fri, 2 Jun 2023 14:28:04 -0300 Subject: [PATCH] Use X-WR-TIMEZONE as calendar tz when available, fallback to UTC (#2091) When checking current shifts to trigger slack notifications, a stable value for timezone should be used (to avoid triggering multiple duplicated notifications; using the first `VTIMEZONE` value available may change on refresh, besides being potentially incorrect). Also, `VTIMEZONE` shouldn't be used as the calendar timezone, it just describes a [timezone definition](https://icalendar.org/iCalendar-RFC-5545/3-6-5-time-zone-component.html) which can later be [used when specifying a start/end date/datetime](https://icalendar.org/iCalendar-RFC-5545/3-2-19-time-zone-identifier.html) ("The parameter MUST be specified on properties with a DATE-TIME value if the DATE-TIME is not either a UTC or a "floating" time."). OTOH, Google uses the non-standard[ `X-WR-TIMEZONE` ](https://github.com/collective/icalendar/issues/343)field as fallback TZ for date-times without a `TZID`. Try to use this when available, otherwise fallback to `UTC`. --- CHANGELOG.md | 1 + .../tests/test_notify_ical_schedule_shift.py | 127 ++++++++++++++++++ engine/apps/schedules/ical_utils.py | 5 +- .../apps/schedules/tests/test_ical_utils.py | 55 ++++++++ 4 files changed, 184 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cc9fedb..41b18aa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fix demo alert for inbound email integration by @vadimkerr ([#2081](https://github.com/grafana/oncall/pull/2081)) +- Fix calendar TZ used when comparing current shifts triggering slack shift notifications ([#2091](https://github.com/grafana/oncall/pull/2091)) ## v1.2.35 (2023-06-01) diff --git a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py index a3009f49..a502e616 100644 --- a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py @@ -304,3 +304,130 @@ def test_current_shift_changes_trigger_notification( notify_ical_schedule_shift(schedule.oncallschedule_ptr_id) assert mock_slack_api_call.called + + +@pytest.mark.django_db +def test_vtimezone_changes_no_triggering_notification( + make_organization_and_user_with_slack_identities, + make_user, + make_schedule, +): + organization, _, _, _ = make_organization_and_user_with_slack_identities() + make_user(organization=organization, username="user1") + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + ical_before = textwrap.dedent( + """ + BEGIN:VCALENDAR + PRODID:-//Google Inc//Google Calendar 70.9054//EN + VERSION:2.0 + CALSCALE:GREGORIAN + X-WR-TIMEZONE:Europe/London + METHOD:PUBLISH + BEGIN:VTIMEZONE + TZID:Europe/Rome + BEGIN:STANDARD + TZOFFSETFROM:0200 + TZOFFSETTO:0100 + TZNAME:CET + DTSTART:19701025T030000 + END:STANDARD + END:VTIMEZONE + BEGIN:VTIMEZONE + TZID:America/Argentina/Buenos_Aires + X-LIC-LOCATION:America/Argentina/Buenos_Aires + BEGIN:STANDARD + TZOFFSETFROM:-0300 + TZOFFSETTO:-0300 + TZNAME:-03 + DTSTART:19700101T000000 + END:STANDARD + END:VTIMEZONE + BEGIN:VEVENT + DTSTART;VALUE=DATE:20230101 + DTEND;VALUE=DATE:20230102 + RRULE:FREQ=DAILY + DTSTAMP:20230101T000000 + UID:id1@google.com + CREATED:20230101T000000 + DESCRIPTION: + LAST-MODIFIED:20230101T000000 + LOCATION: + SEQUENCE:1 + STATUS:CONFIRMED + SUMMARY:user1 + TRANSP:TRANSPARENT + END:VEVENT + END:VCALENDAR""" + ) + + # same data, timezones in different order (eg. google usually randomly reorders them) + ical_after = textwrap.dedent( + """ + BEGIN:VCALENDAR + PRODID:-//Google Inc//Google Calendar 70.9054//EN + VERSION:2.0 + CALSCALE:GREGORIAN + METHOD:PUBLISH + X-WR-TIMEZONE:Europe/London + BEGIN:VTIMEZONE + TZID:America/Argentina/Buenos_Aires + X-LIC-LOCATION:America/Argentina/Buenos_Aires + BEGIN:STANDARD + TZOFFSETFROM:-0300 + TZOFFSETTO:-0300 + TZNAME:-03 + DTSTART:19700101T000000 + END:STANDARD + END:VTIMEZONE + BEGIN:VTIMEZONE + TZID:Europe/Rome + BEGIN:STANDARD + TZOFFSETFROM:0200 + TZOFFSETTO:0100 + TZNAME:CET + DTSTART:19701025T030000 + END:STANDARD + END:VTIMEZONE + BEGIN:VEVENT + DTSTART;VALUE=DATE:20230101 + DTEND;VALUE=DATE:20230102 + RRULE:FREQ=DAILY + DTSTAMP:20230101T000000 + UID:id1@google.com + CREATED:20230101T000000 + DESCRIPTION: + LAST-MODIFIED:20230101T000000 + LOCATION: + SEQUENCE:1 + STATUS:CONFIRMED + SUMMARY:user1 + TRANSP:TRANSPARENT + END:VEVENT + END:VCALENDAR""" + ) + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleICal, + name="test_ical_schedule", + channel="channel", + ical_url_primary="url", + prev_ical_file_primary=ical_before, + cached_ical_file_primary=ical_after, + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + # setup current shifts before checking/triggering for notifications + calendar = icalendar.Calendar.from_ical(ical_before) + current_shifts, _ = get_current_shifts_from_ical(calendar, schedule, 0) + schedule.current_shifts = json.dumps(current_shifts, default=str) + schedule.empty_oncall = False + schedule.save() + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_ical_schedule_shift(schedule.oncallschedule_ptr_id) + + assert not mock_slack_api_call.called diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 4c02a70e..2f794f9f 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -575,10 +575,7 @@ def calculate_shift_diff(first_shift, second_shift): def get_icalendar_tz_or_utc(icalendar): - try: - calendar_timezone = icalendar.walk("VTIMEZONE")[0]["TZID"] - except (IndexError, KeyError): - calendar_timezone = "UTC" + calendar_timezone = icalendar.get("X-WR-TIMEZONE", "UTC") if pytz_timezone := is_valid_timezone(calendar_timezone): return pytz_timezone diff --git a/engine/apps/schedules/tests/test_ical_utils.py b/engine/apps/schedules/tests/test_ical_utils.py index 7bea4457..0f395238 100644 --- a/engine/apps/schedules/tests/test_ical_utils.py +++ b/engine/apps/schedules/tests/test_ical_utils.py @@ -2,12 +2,14 @@ import datetime import textwrap from uuid import uuid4 +import icalendar import pytest import pytz from django.utils import timezone from apps.api.permissions import LegacyAccessControlRole from apps.schedules.ical_utils import ( + get_icalendar_tz_or_utc, is_icals_equal, list_of_oncall_shifts_from_ical, list_users_to_notify_from_ical, @@ -17,6 +19,59 @@ from apps.schedules.ical_utils import ( from apps.schedules.models import CustomOnCallShift, OnCallSchedule, OnCallScheduleCalendar, OnCallScheduleWeb +def test_get_icalendar_tz_or_utc(): + ical_data = textwrap.dedent( + """ + BEGIN:VCALENDAR + PRODID:-//Google Inc//Google Calendar 70.9054//EN + VERSION:2.0 + CALSCALE:GREGORIAN + METHOD:PUBLISH + X-WR-TIMEZONE:Europe/London + BEGIN:VTIMEZONE + TZID:America/Argentina/Buenos_Aires + X-LIC-LOCATION:America/Argentina/Buenos_Aires + BEGIN:STANDARD + TZOFFSETFROM:-0300 + TZOFFSETTO:-0300 + TZNAME:-03 + DTSTART:19700101T000000 + END:STANDARD + END:VTIMEZONE + END:VCALENDAR + """ + ) + ical = icalendar.Calendar.from_ical(ical_data) + tz = get_icalendar_tz_or_utc(ical) + assert tz == pytz.timezone("Europe/London") + + +def test_get_icalendar_tz_or_utc_fallback(): + ical_data = textwrap.dedent( + """ + BEGIN:VCALENDAR + PRODID:-//Google Inc//Google Calendar 70.9054//EN + VERSION:2.0 + CALSCALE:GREGORIAN + METHOD:PUBLISH + BEGIN:VTIMEZONE + TZID:America/Argentina/Buenos_Aires + X-LIC-LOCATION:America/Argentina/Buenos_Aires + BEGIN:STANDARD + TZOFFSETFROM:-0300 + TZOFFSETTO:-0300 + TZNAME:-03 + DTSTART:19700101T000000 + END:STANDARD + END:VTIMEZONE + END:VCALENDAR + """ + ) + ical = icalendar.Calendar.from_ical(ical_data) + tz = get_icalendar_tz_or_utc(ical) + assert tz == pytz.timezone("UTC") + + @pytest.mark.django_db def test_users_in_ical_email_case_insensitive(make_organization_and_user, make_user_for_organization): organization, user = make_organization_and_user()