From 06862c2cebfd4a1938a3e1740576f19343170791 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Fri, 29 Sep 2023 15:49:36 -0300 Subject: [PATCH 1/3] Fix slack notification for shift end affected by taken swap --- .../tasks/notify_ical_schedule_shift.py | 55 +++++++------ .../tests/test_notify_ical_schedule_shift.py | 81 ++++++++++++++++++- 2 files changed, 108 insertions(+), 28 deletions(-) diff --git a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py index d06e78784e..4314124973 100644 --- a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py @@ -22,6 +22,9 @@ from apps.schedules.models import OnCallSchedule +MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT = 3 + + def convert_prev_shifts_to_new_format(prev_shifts: dict, schedule: "OnCallSchedule") -> list: new_prev_shifts = [] user_ids = [] @@ -82,12 +85,6 @@ def notify_ical_schedule_shift(schedule_pk): task_logger.info(f"Notify ical schedule shift {schedule_pk}, organization {schedule.organization_id}") - MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT = 3 - - now = datetime.datetime.now(timezone.utc) - - current_shifts = schedule.final_events(now, now, with_empty=False, with_gap=False, ignore_untaken_swaps=True) - prev_shifts = json.loads(schedule.current_shifts) if not schedule.empty_oncall else [] prev_shifts_updated = False # convert prev_shifts to new events format for compatibility with the previous version of this task @@ -101,6 +98,33 @@ def notify_ical_schedule_shift(schedule_pk): prev_shift["start"] = datetime.datetime.strptime(prev_shift["start"], str_format) prev_shift["end"] = datetime.datetime.strptime(prev_shift["end"], str_format) + # get shifts in progress now + now = datetime.datetime.now(timezone.utc) + current_shifts = schedule.final_events(now, now, with_empty=False, with_gap=False, ignore_untaken_swaps=True) + + # get days_to_lookup for next shifts (which may affect current shifts) + if len(current_shifts) != 0: + max_end_date = max([shift["end"].date() for shift in current_shifts]) + days_to_lookup = (max_end_date - now.date()).days + 1 + days_to_lookup = max([days_to_lookup, MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT]) + else: + days_to_lookup = MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT + + # get updated current and upcoming shifts + datetime_end = now + datetime.timedelta(days=days_to_lookup) + next_shifts_unfiltered = schedule.final_events( + now, datetime_end, with_empty=False, with_gap=False, ignore_untaken_swaps=True + ) + + # split current and next shifts + current_shifts = [] + next_shifts = [] + for shift in next_shifts_unfiltered: + if now < shift["start"]: + next_shifts.append(shift) + else: + current_shifts.append(shift) + shift_changed, diff_shifts = calculate_shift_diff(current_shifts, prev_shifts) # Do not notify if there is no difference between current and previous shifts @@ -114,25 +138,6 @@ def notify_ical_schedule_shift(schedule_pk): new_shifts = sorted(diff_shifts, key=lambda shift: shift["start"]) - # get days_to_lookup for next shifts - if len(new_shifts) != 0: - max_end_date = max([shift["end"].date() for shift in new_shifts]) - days_to_lookup = (max_end_date - now.date()).days + 1 - days_to_lookup = max([days_to_lookup, MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT]) - else: - days_to_lookup = MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT - - datetime_end = now + datetime.timedelta(days=days_to_lookup) - - next_shifts_unfiltered = schedule.final_events( - now, datetime_end, with_empty=False, with_gap=False, ignore_untaken_swaps=True - ) - # drop events that already started - next_shifts = [] - for next_shift in next_shifts_unfiltered: - if now < next_shift["start"]: - next_shifts.append(next_shift) - upcoming_shifts = [] # Add the earliest next_shift if len(next_shifts) > 0: 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 e309111bea..31f3f7313d 100644 --- a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py @@ -7,7 +7,10 @@ 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 ( + MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT, + notify_ical_schedule_shift, +) from apps.schedules.ical_utils import memoized_users_in_ical from apps.schedules.models import CustomOnCallShift, OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb @@ -371,6 +374,75 @@ def test_current_shift_changes_swap_split( assert "user2" in text_block if swap_taken else "user1" in text_block +@pytest.mark.django_db +def test_current_shift_changes_end_affected_by_swap( + make_organization_and_user_with_slack_identities, + make_user, + make_schedule, + make_on_call_shift, + make_shift_swap_request, +): + organization, _, _, _ = make_organization_and_user_with_slack_identities() + user1 = make_user(organization=organization, username="user1") + user2 = make_user(organization=organization, username="user2") + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + today = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + duration = timezone.timedelta(days=3) + data = { + "start": today, + "rotation_start": today, + "duration": duration, + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_WEEKLY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + + # setup in progress swap request + swap_start = today + timezone.timedelta(days=1) + swap_end = today + timezone.timedelta(days=2) + make_shift_swap_request( + schedule, + user1, + swap_start=swap_start, + swap_end=swap_end, + benefactor=user2, + ) + + 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.client.SlackClient.chat_postMessage") as mock_slack_api_call: + notify_ical_schedule_shift(schedule.pk) + + current_text_block = mock_slack_api_call.call_args_list[0][1]["blocks"][1]["text"]["text"] + assert "user1" in current_text_block + assert today.strftime("%Y-%m-%d") in current_text_block + assert swap_start.strftime("%Y-%m-%d") in current_text_block + next_text_block = mock_slack_api_call.call_args_list[0][1]["blocks"][2]["text"]["text"] + assert "user2" in next_text_block + assert swap_start.strftime("%Y-%m-%d") in next_text_block + assert swap_end.strftime("%Y-%m-%d") in next_text_block + + @pytest.mark.django_db def test_next_shift_changes_no_triggering_notification( make_organization_and_user_with_slack_identities, @@ -424,8 +496,11 @@ def test_next_shift_changes_no_triggering_notification( schedule.refresh_ical_file() - # setup empty current shifts before checking/triggering for notifications - current_shifts = schedule.final_events(now, now, False, False) + # setup current shifts before checking/triggering for notifications + next_shifts = schedule.final_events( + now, now + datetime.timedelta(days=MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT), False, False + ) + current_shifts = [e for e in next_shifts if now > e["start"]] schedule.current_shifts = json.dumps(current_shifts, default=str) schedule.empty_oncall = False schedule.save() From 92afc17fa31fdc4bb181d3eeae2adf78fb9307f6 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Fri, 29 Sep 2023 16:05:48 -0300 Subject: [PATCH 2/3] Update changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8362c0028f..c9c0308b32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Fixed + +- Fix slack notification for a shift which end is affected by a taken swap ([#3092](https://github.com/grafana/oncall/pull/3092)) + ## v1.3.40 (2023-08-28) ### Added From 12f5a7387e945dd9524701f10f89b11dc48cb666 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Mon, 2 Oct 2023 09:38:02 -0300 Subject: [PATCH 3/3] Refactoring parse event UID helper --- engine/apps/schedules/ical_utils.py | 41 ++++++++++++++++------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 285ad176ef..6e04dc999f 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -419,29 +419,32 @@ def parse_event_uid(string: str, sequence: str = None, recurrence_id: str = None source = None source_verbal = None - match = RE_EVENT_UID_V2.match(string) - if match: - _, pk, _, _, source = match.groups() + match = None + if string.startswith("oncall"): + match = RE_EVENT_UID_V2.match(string) + if match: + _, pk, _, _, source = match.groups() + elif string.startswith("amixr"): + # eventually this path would be automatically deprecated + # once all ical representations are refreshed + match = RE_EVENT_UID_V1.match(string) + if match: + _, _, _, source = match.groups() else: match = RE_EVENT_UID_EXPORT.match(string) if match: pk, _, _ = match.groups() - else: - # eventually this path would be automatically deprecated - # once all ical representations are refreshed - match = RE_EVENT_UID_V1.match(string) - if match: - _, _, _, source = match.groups() - else: - # fallback to use the UID string as the rotation ID - pk = string - # in ical imported calendars, sequence and/or recurrence_id - # distinguish main recurring event vs instance modification - # (see https://icalendar.org/iCalendar-RFC-5545/3-8-4-4-recurrence-id.html) - if sequence: - pk = f"{pk}_{sequence}" - if recurrence_id: - pk = f"{pk}_{recurrence_id}" + + if not match: + # fallback to use the UID string as the rotation ID + pk = string + # in ical imported calendars, sequence and/or recurrence_id + # distinguish main recurring event vs instance modification + # (see https://icalendar.org/iCalendar-RFC-5545/3-8-4-4-recurrence-id.html) + if sequence: + pk = f"{pk}_{sequence}" + if recurrence_id: + pk = f"{pk}_{recurrence_id}" if source is not None: source = int(source)