Trigger slack shift notifications on current shift change

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)
 

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,

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