diff --git a/CHANGELOG.md b/CHANGELOG.md index 57067135c3..f3a0e13323 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Add stack slug to organization options for direct paging Slash command by @vadimkerr ([#2743](https://github.com/grafana/oncall/pull/2743)) +- Avoid creating (or notifying about) potential event splits resulting from untaken swap requests ([#2748](https://github.com/grafana/oncall/pull/2748)) ### Fixed diff --git a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py index 0324783fa1..9321717c5c 100644 --- a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py @@ -81,7 +81,7 @@ def notify_ical_schedule_shift(schedule_pk): now = datetime.datetime.now(timezone.utc) - current_shifts = schedule.final_events(now, now, with_empty=False, with_gap=False) + 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 @@ -119,7 +119,9 @@ def notify_ical_schedule_shift(schedule_pk): datetime_end = now + datetime.timedelta(days=days_to_lookup) - next_shifts_unfiltered = schedule.final_events(now, datetime_end, with_empty=False, with_gap=False) + 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: 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 d5925c9dcc..5d931181eb 100644 --- a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py @@ -305,6 +305,71 @@ def test_current_shift_changes_trigger_notification( assert mock_slack_api_call.called +@pytest.mark.django_db +@pytest.mark.parametrize("swap_taken", [False, True]) +def test_current_shift_changes_swap_split( + make_organization_and_user_with_slack_identities, + make_user, + make_schedule, + make_on_call_shift, + make_shift_swap_request, + swap_taken, +): + 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(hours=23, minutes=59, seconds=59) + data = { + "start": today, + "rotation_start": today, + "duration": duration, + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "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_request = make_shift_swap_request( + schedule, + user1, + swap_start=today, + swap_end=today + timezone.timedelta(days=2), + ) + if swap_taken: + swap_request.benefactor = user2 + swap_request.save() + + 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.pk) + + text_block = mock_slack_api_call.call_args_list[0][1]["blocks"][0]["text"]["text"] + assert "user2" in text_block if swap_taken else "user1" in text_block + + @pytest.mark.django_db def test_next_shift_changes_no_triggering_notification( make_organization_and_user_with_slack_identities, diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index 56963e0826..bbb16b9496 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -344,6 +344,7 @@ def filter_events( with_gap: bool = False, filter_by: str | None = None, all_day_datetime: bool = False, + ignore_untaken_swaps: bool = False, from_cached_final: bool = False, ) -> ScheduleEvents: """Return filtered events from schedule.""" @@ -401,7 +402,9 @@ def filter_events( events = self._merge_events(events) # annotate events with swap request details swapping users as needed - events = self._apply_swap_requests(events, datetime_start, datetime_end) + events = self._apply_swap_requests( + events, datetime_start, datetime_end, ignore_untaken_swaps=ignore_untaken_swaps + ) return events @@ -411,10 +414,16 @@ def final_events( datetime_end: datetime.datetime, with_empty: bool = True, with_gap: bool = True, + ignore_untaken_swaps: bool = False, ) -> ScheduleEvents: """Return schedule final events, after resolving shifts and overrides.""" events = self.filter_events( - datetime_start, datetime_end, with_empty=with_empty, with_gap=with_gap, all_day_datetime=True + datetime_start, + datetime_end, + with_empty=with_empty, + with_gap=with_gap, + all_day_datetime=True, + ignore_untaken_swaps=ignore_untaken_swaps, ) events = self._resolve_schedule(events, datetime_start, datetime_end) return events @@ -442,7 +451,7 @@ def refresh_ical_final_schedule(self): # setup calendar with final schedule shift events calendar = create_base_icalendar(self.name) - events = self.final_events(datetime_start, datetime_end) + events = self.final_events(datetime_start, datetime_end, ignore_untaken_swaps=True) updated_ids = set() for e in events: for u in e["users"]: @@ -647,7 +656,11 @@ def get_balance_score_by_duration_map(dur_map: DurationMap) -> float: } def _apply_swap_requests( - self, events: ScheduleEvents, datetime_start: datetime.datetime, datetime_end: datetime.datetime + self, + events: ScheduleEvents, + datetime_start: datetime.datetime, + datetime_end: datetime.datetime, + ignore_untaken_swaps: bool = False, ) -> ScheduleEvents: """Apply swap requests details to schedule events.""" # get swaps requests affecting this schedule / time range @@ -663,8 +676,8 @@ def _insert_event(index: int, event: ScheduleEvent) -> int: # apply swaps sequentially for swap in swaps: - if swap.is_past_due: - # ignore untaken expired requests + if swap.is_past_due or (ignore_untaken_swaps and not swap.is_taken): + # ignore expired requests, or untaken if specified continue i = 0 while i < len(events): diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index ebf4881174..11894d312e 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -127,7 +127,7 @@ def is_taken(self) -> bool: @property def is_past_due(self) -> bool: - return timezone.now() > self.swap_start + return not self.is_taken and timezone.now() > self.swap_start @property def is_open(self) -> bool: diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index 603c69aeda..42ba8107f9 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -2169,21 +2169,36 @@ def test_swap_request_split_both( with patch("apps.schedules.models.on_call_schedule.EXPORT_WINDOW_DAYS_BEFORE", 0): schedule.refresh_ical_final_schedule() assert schedule.cached_ical_final_schedule - expected_events = [ - # start, end, user - (start, start + duration, user.username), # today shift unchanged - (start + timezone.timedelta(days=1), start + timezone.timedelta(days=1, hours=1), user.username), # first split - ( - start + timezone.timedelta(days=1, hours=1), - start + timezone.timedelta(days=1, hours=2), - other_user.username if swap_taken else user.username, - ), # second split - ( - start + timezone.timedelta(days=1, hours=2), - start + timezone.timedelta(days=1, hours=3), - user.username, - ), # third split - ] + if swap_taken: + expected_events = [ + # start, end, user + (start, start + duration, user.username), # today shift unchanged + ( + start + timezone.timedelta(days=1), + start + timezone.timedelta(days=1, hours=1), + user.username, + ), # first split + ( + start + timezone.timedelta(days=1, hours=1), + start + timezone.timedelta(days=1, hours=2), + other_user.username if swap_taken else user.username, + ), # second split + ( + start + timezone.timedelta(days=1, hours=2), + start + timezone.timedelta(days=1, hours=3), + user.username, + ), # third split + ] + else: + expected_events = [ + # start, end, user + (start, start + duration, user.username), # today shift unchanged + ( + start + timezone.timedelta(days=1), + start + timezone.timedelta(days=1) + duration, + user.username, + ), # no split + ] calendar = icalendar.Calendar.from_ical(schedule.cached_ical_final_schedule) for component in calendar.walk(): if component.name == ICAL_COMPONENT_VEVENT: @@ -2195,6 +2210,83 @@ def test_swap_request_split_both( assert event in expected_events +@pytest.mark.django_db +@pytest.mark.parametrize("swap_taken", [False, True]) +def test_swap_request_ignore_untaken( + make_organization, + make_user_for_organization, + make_schedule, + make_on_call_shift, + make_shift_swap_request, + swap_taken, +): + organization = make_organization() + user = make_user_for_organization(organization) + other_user = make_user_for_organization(organization) + + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + today = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + start = today + timezone.timedelta(hours=12) + duration = timezone.timedelta(hours=3) + data = { + "start": start, + "rotation_start": start, + "duration": duration, + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "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([[user]]) + + tomorrow = today + timezone.timedelta(days=1) + # setup swap request + swap_request = make_shift_swap_request( + schedule, + user, + swap_start=tomorrow + timezone.timedelta(hours=13), + swap_end=tomorrow + timezone.timedelta(hours=14), + ) + if swap_taken: + swap_request.take(other_user) + + # set flag to ignore untaken swaps + events = schedule.filter_events(today, today + timezone.timedelta(days=2), ignore_untaken_swaps=True) + + if swap_taken: + expected = [ + # start, end, swap requested + (start, start + duration, False), # today shift unchanged + (start + timezone.timedelta(days=1), start + timezone.timedelta(days=1, hours=1), False), # first split + ( + start + timezone.timedelta(days=1, hours=1), + start + timezone.timedelta(days=1, hours=2), + True, + ), # second split + ( + start + timezone.timedelta(days=1, hours=2), + start + timezone.timedelta(days=1, hours=3), + False, + ), # third split + ] + else: + expected = [ + # start, end, swap requested + (start, start + duration, False), # today shift unchanged + (start + timezone.timedelta(days=1), start + timezone.timedelta(days=1) + duration, False), # no split + ] + + returned = [(e["start"], e["end"], bool(e["users"][0].get("swap_request", False))) for e in events] + assert returned == expected + # check swap request details + if swap_taken: + assert events[2]["users"][0]["swap_request"]["pk"] == swap_request.public_primary_key + assert events[2]["users"][0]["pk"] == other_user.public_primary_key + assert events[2]["users"][0]["swap_request"]["user"]["pk"] == user.public_primary_key + + @pytest.mark.django_db @pytest.mark.parametrize("swap_taken", [False, True]) def test_swap_request_whole_shift( diff --git a/engine/apps/schedules/tests/test_shift_swap_request.py b/engine/apps/schedules/tests/test_shift_swap_request.py index a6735484db..63de2b7539 100644 --- a/engine/apps/schedules/tests/test_shift_swap_request.py +++ b/engine/apps/schedules/tests/test_shift_swap_request.py @@ -46,6 +46,14 @@ def test_status_taken(shift_swap_request_setup) -> None: assert ssr.status == ShiftSwapRequest.Statuses.TAKEN assert ssr.is_taken is True + # taken in the past it's still taken + now = timezone.now() + ssr.swap_start = now - timezone.timedelta(days=2) + ssr.save() + assert ssr.status == ShiftSwapRequest.Statuses.TAKEN + assert ssr.is_taken is True + assert ssr.is_past_due is False + @pytest.mark.django_db def test_status_past_due(shift_swap_request_setup) -> None: