Filter out untaken swaps from final schedule and shift notifications

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
 

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:

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,

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):

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:

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(

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: