diff --git a/CHANGELOG.md b/CHANGELOG.md index e74b72c012..8d095db408 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 93e1120917..dac6838a1f 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 json from copy import copy -import icalendar from django.apps import apps from django.utils import timezone @@ -12,7 +11,6 @@ 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 = {} - - # 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) + # 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) - 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 a2d8fa9be2..a3009f49e5 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