From 5d383c7d1d8b350fb7f71fa28844a7309e5c0974 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 1 Jun 2023 13:27:14 -0300 Subject: [PATCH] Trigger slack shift notifications on current shift change (#2080) Before this change, a diff ical check (which happens with frequency with imported ical), particularly with overrides in an API/terraform schedule would trigger unexpected slack notifications because the prev vs current ical comparison will flag a diff, but when comparing current and previous shifts, `current_shifts` will have the shift in progress while the `prev_shifts` calculated from the overrides-only diff will most of the time be empty (unless you set/change an override at current time). Simplified the checks to always compare previous current shifts (ie. the ones in the schedule from the DB) vs the recalculated ones using the (refreshed) ical data from the schedule. --- CHANGELOG.md | 1 + .../tasks/notify_ical_schedule_shift.py | 70 +----- .../tests/test_notify_ical_schedule_shift.py | 203 +++++++++++++++++- 3 files changed, 210 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e74b72c0..8d095db4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix a bug with permissions for telegram user settings by @alexintech ([#2075](https://github.com/grafana/oncall/pull/2075)) - Fix orphaned messages in Slack by @vadimkerr ([#2023](https://github.com/grafana/oncall/pull/2023)) +- Fix duplicated slack shift-changed notifications ([#2080](https://github.com/grafana/oncall/pull/2080)) ## v1.2.34 (2023-05-31) diff --git a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py index 93e11209..dac6838a 100644 --- a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py @@ -2,7 +2,6 @@ import datetime import json from copy import copy -import icalendar from django.apps import apps from django.utils import timezone @@ -12,7 +11,6 @@ from apps.schedules.ical_utils import ( event_start_end_all_day_with_respect_to_type, get_icalendar_tz_or_utc, get_usernames_from_ical_event, - is_icals_equal, memoized_users_in_ical, ) from apps.slack.scenarios import scenario_step @@ -191,8 +189,6 @@ def notify_ical_schedule_shift(schedule_pk): MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT = 3 - ical_changed = False - now = timezone.datetime.now(timezone.utc) # get list of iCalendars from current iCal files. If there is more than one calendar, primary calendar will always # be the first @@ -240,60 +236,15 @@ def notify_ical_schedule_shift(schedule_pk): for item in drop: current_shifts.pop(item) - is_prev_ical_diff = False - prev_overrides_priority = 0 - prev_shifts = {} - prev_users = {} + # compare events from prev and current shifts + prev_shifts = json.loads(schedule.current_shifts) if not schedule.empty_oncall else {} + # convert datetimes which was dumped to str back to datetime to calculate shift diff correct + str_format = "%Y-%m-%d %X%z" + for prev_shift in prev_shifts.values(): + prev_shift["start"] = datetime.datetime.strptime(prev_shift["start"], str_format) + prev_shift["end"] = datetime.datetime.strptime(prev_shift["end"], str_format) - # Get list of tuples with prev and current ical file for each calendar. If there is more than one calendar, primary - # calendar will be the first. - # example result for ical calendar: - # [(prev_ical_file_primary, current_ical_file_primary), (prev_ical_file_overrides, current_ical_file_overrides)] - # example result for calendar with custom events: - # [(prev_ical_file, current_ical_file)] - prev_and_current_ical_files = schedule.get_prev_and_current_ical_files() - - for prev_ical_file, current_ical_file in prev_and_current_ical_files: - if prev_ical_file and (not current_ical_file or not is_icals_equal(current_ical_file, prev_ical_file)): - task_logger.info(f"ical files are different") - # If icals are not equal then compare current_events from them - is_prev_ical_diff = True - prev_calendar = icalendar.Calendar.from_ical(prev_ical_file) - - prev_shifts_result, prev_users_result = get_current_shifts_from_ical( - prev_calendar, - schedule, - prev_overrides_priority, - ) - if prev_overrides_priority == 0 and prev_shifts_result: - prev_overrides_priority = max([prev_shifts_result[uid]["priority"] for uid in prev_shifts_result]) + 1 - - prev_shifts.update(prev_shifts_result) - prev_users.update(prev_users_result) - - recalculate_shifts_with_respect_to_priority(prev_shifts, prev_users) - - if is_prev_ical_diff: - # drop events that don't intersection with current time - drop = [] - for uid, prev_shift in prev_shifts.items(): - if not prev_shift["start"] < now < prev_shift["end"]: - drop.append(uid) - for item in drop: - prev_shifts.pop(item) - - shift_changed, diff_uids = calculate_shift_diff(current_shifts, prev_shifts) - - else: - # Else comparing events from prev and current shifts - prev_shifts = json.loads(schedule.current_shifts) if not schedule.empty_oncall else {} - # convert datetimes which was dumped to str back to datetime to calculate shift diff correct - str_format = "%Y-%m-%d %X%z" - for prev_shift in prev_shifts.values(): - prev_shift["start"] = datetime.datetime.strptime(prev_shift["start"], str_format) - prev_shift["end"] = datetime.datetime.strptime(prev_shift["end"], str_format) - - shift_changed, diff_uids = calculate_shift_diff(current_shifts, prev_shifts) + shift_changed, diff_uids = calculate_shift_diff(current_shifts, prev_shifts) if shift_changed: task_logger.info(f"shifts_changed: {diff_uids}") @@ -370,11 +321,6 @@ def notify_ical_schedule_shift(schedule_pk): if schedule.notify_oncall_shift_freq != OnCallSchedule.NotifyOnCallShiftFreq.NEVER: try: - if ical_changed: - slack_client.api_call( - "chat.postMessage", channel=schedule.channel, text=f"Schedule {schedule.name} was changed" - ) - slack_client.api_call( "chat.postMessage", channel=schedule.channel, 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 a2d8fa9b..a3009f49 100644 --- a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py @@ -1,13 +1,16 @@ +import json +import textwrap from datetime import datetime from unittest.mock import Mock, patch +import icalendar import pytest import pytz from django.utils import timezone -from apps.alerts.tasks.notify_ical_schedule_shift import notify_ical_schedule_shift +from apps.alerts.tasks.notify_ical_schedule_shift import get_current_shifts_from_ical, notify_ical_schedule_shift from apps.schedules.ical_utils import memoized_users_in_ical -from apps.schedules.models import OnCallScheduleICal +from apps.schedules.models import CustomOnCallShift, OnCallScheduleCalendar, OnCallScheduleICal ICAL_DATA = """ BEGIN:VCALENDAR @@ -105,3 +108,199 @@ def test_next_shift_notification_long_shifts( notification = slack_blocks[0]["text"]["text"] assert "*New on-call shift:*\nuser2" in notification assert "*Next on-call shift:*\nuser1" in notification + + +@pytest.mark.django_db +def test_overrides_changes_no_current_no_triggering_notification( + make_organization_and_user_with_slack_identities, + make_user, + make_schedule, + make_on_call_shift, +): + organization, _, _, _ = make_organization_and_user_with_slack_identities() + user1 = 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 + METHOD:PUBLISH + BEGIN:VEVENT + DTSTART:20230101T020000 + DTEND:20230101T170000 + 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""" + ) + + # event outside current time is changed + ical_after = textwrap.dedent( + """ + BEGIN:VCALENDAR + PRODID:-//Google Inc//Google Calendar 70.9054//EN + VERSION:2.0 + CALSCALE:GREGORIAN + METHOD:PUBLISH + BEGIN:VEVENT + DTSTART:20230101T020000 + DTEND:20230101T210000 + DTSTAMP:20230101T000000 + UID:id1@google.com + CREATED:20230101T000000 + DESCRIPTION: + LAST-MODIFIED:20230101T000000 + LOCATION: + SEQUENCE:2 + STATUS:CONFIRMED + SUMMARY:user1 + TRANSP:TRANSPARENT + END:VEVENT + END:VCALENDAR""" + ) + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleCalendar, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=ical_before, + cached_ical_file_overrides=ical_after, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - timezone.timedelta(days=7) + + data = { + "start": start_date, + "rotation_start": start_date, + "duration": timezone.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + on_call_shift.schedules.add(schedule) + + # setup current shifts before checking/triggering for notifications + calendar = icalendar.Calendar.from_ical(schedule._ical_file_primary) + 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 + + +@pytest.mark.django_db +def test_no_changes_no_triggering_notification( + make_organization_and_user_with_slack_identities, + make_user, + make_schedule, + make_on_call_shift, +): + organization, _, _, _ = make_organization_and_user_with_slack_identities() + user1 = make_user(organization=organization, username="user1") + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleCalendar, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - timezone.timedelta(days=7) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": timezone.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + on_call_shift.schedules.add(schedule) + + # setup current shifts before checking/triggering for notifications + calendar = icalendar.Calendar.from_ical(schedule._ical_file_primary) + 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 + + +@pytest.mark.django_db +def test_current_shift_changes_trigger_notification( + make_organization_and_user_with_slack_identities, + make_user, + make_schedule, + make_on_call_shift, +): + organization, _, _, _ = make_organization_and_user_with_slack_identities() + user1 = make_user(organization=organization, username="user1") + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleCalendar, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - timezone.timedelta(days=7) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": timezone.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + on_call_shift.schedules.add(schedule) + schedule.refresh_ical_file() + + # setup empty current shifts before checking/triggering for notifications + schedule.current_shifts = json.dumps({}, 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 mock_slack_api_call.called