Refactoring schedule final events (#2651)

Update `list_users_to_notify_from_ical` to use schedule final events

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Update the direct paging feature to page for acknowledged & silenced alert groups,
   and show a warning for resolved alert groups by @vadimkerr ([#2639](https://github.com/grafana/oncall/pull/2639))
+- Update checking on-call users to use schedule final events ([#2651](https://github.com/grafana/oncall/pull/2651))
 
 ### Fixed
 

engine/apps/alerts/escalation_snapshot/snapshot_classes/escalation_policy_snapshot.py

@@ -275,7 +275,7 @@ class EscalationPolicySnapshot:
                 escalation_policy_step=self.step,
             )
         else:
-            notify_to_users_list = list_users_to_notify_from_ical(on_call_schedule, include_viewers=True)
+            notify_to_users_list = list_users_to_notify_from_ical(on_call_schedule)
             if notify_to_users_list is None:
                 log_record = AlertGroupLogRecord(
                     type=AlertGroupLogRecord.TYPE_ESCALATION_FAILED,

engine/apps/alerts/tests/test_escalation_policy_snapshot.py

@@ -246,11 +246,8 @@ def test_escalation_step_notify_on_call_schedule_viewer_user(
     )
     assert expected_eta + timezone.timedelta(seconds=15) > result.eta > expected_eta - timezone.timedelta(seconds=15)
     assert result == expected_result
-    assert notify_schedule_step.log_records.filter(type=AlertGroupLogRecord.TYPE_ESCALATION_TRIGGERED).exists()
-    assert list(escalation_policy_snapshot.notify_to_users_queue) == list(
-        list_users_to_notify_from_ical(schedule, include_viewers=True)
-    )
-    assert list(escalation_policy_snapshot.notify_to_users_queue) == [viewer]
+    assert notify_schedule_step.log_records.filter(type=AlertGroupLogRecord.TYPE_ESCALATION_FAILED).exists()
+    assert list(escalation_policy_snapshot.notify_to_users_queue) == []
     assert mocked_execute_tasks.called
 
 

engine/apps/api/tests/test_schedules.py

@@ -1227,7 +1227,7 @@ def test_filter_events_final_schedule(
             "is_gap": is_gap,
             "is_override": is_override,
             "priority_level": priority,
-            "start": start_date + timezone.timedelta(hours=start, milliseconds=1 if start == 0 else 0),
+            "start": start_date + timezone.timedelta(hours=start),
             "user": user,
         }
         for start, duration, user, priority, is_gap, is_override in expected

engine/apps/api/views/on_call_shifts.py

@@ -1,3 +1,6 @@
+import datetime
+
+import pytz
 from django.db.models import Q
 from django_filters.rest_framework import DjangoFilterBackend
 from rest_framework import status
@@ -106,8 +109,13 @@ class OnCallShiftView(TeamFilteringMixin, PublicPrimaryKeyMixin, UpdateSerialize
         updated_shift_pk = self.request.data.get("shift_pk")
         shift = CustomOnCallShift(**validated_data)
         schedule = shift.schedule
+
+        pytz_tz = pytz.timezone(user_tz)
+        datetime_start = datetime.datetime.combine(starting_date, datetime.time.min, tzinfo=pytz_tz)
+        datetime_end = datetime_start + datetime.timedelta(days=days)
+
         shift_events, final_events = schedule.preview_shift(
-            shift, user_tz, starting_date, days, updated_shift_pk=updated_shift_pk
+            shift, datetime_start, datetime_end, updated_shift_pk=updated_shift_pk
         )
         data = {
             "rotation": shift_events,

engine/apps/api/views/schedule.py

@@ -1,6 +1,8 @@
+import datetime
 import functools
 import operator
 
+import pytz
 from django.core.exceptions import ObjectDoesNotExist
 from django.db.models import Count, OuterRef, Subquery
 from django.db.utils import IntegrityError
@@ -274,12 +276,16 @@ class ScheduleView(
 
     @action(detail=True, methods=["get"])
     def events(self, request, pk):
-        user_tz, date = self.get_request_timezone()
+        user_tz, starting_date = self.get_request_timezone()
         with_empty = self.request.query_params.get("with_empty", False) == "true"
         with_gap = self.request.query_params.get("with_gap", False) == "true"
 
         schedule = self.get_object()
-        events = schedule.filter_events(user_tz, date, days=1, with_empty=with_empty, with_gap=with_gap)
+
+        pytz_tz = pytz.timezone(user_tz)
+        datetime_start = datetime.datetime.combine(starting_date, datetime.time.min, tzinfo=pytz_tz)
+        datetime_end = datetime_start + datetime.timedelta(days=1)
+        events = schedule.filter_events(datetime_start, datetime_end, with_empty=with_empty, with_gap=with_gap)
 
         slack_channel = (
             {
@@ -312,19 +318,22 @@ class ScheduleView(
 
         schedule = self.get_object()
 
+        pytz_tz = pytz.timezone(user_tz)
+        datetime_start = datetime.datetime.combine(starting_date, datetime.time.min, tzinfo=pytz_tz)
+        datetime_end = datetime_start + datetime.timedelta(days=days)
+
         if filter_by is not None and filter_by != EVENTS_FILTER_BY_FINAL:
             filter_by = OnCallSchedule.PRIMARY if filter_by == EVENTS_FILTER_BY_ROTATION else OnCallSchedule.OVERRIDES
             events = schedule.filter_events(
-                user_tz,
-                starting_date,
-                days=days,
+                datetime_start,
+                datetime_end,
                 with_empty=True,
                 with_gap=resolve_schedule,
                 filter_by=filter_by,
                 all_day_datetime=True,
             )
         else:  # return final schedule
-            events = schedule.final_events(user_tz, starting_date, days)
+            events = schedule.final_events(datetime_start, datetime_end)
 
         result = {
             "id": schedule.public_primary_key,
@@ -337,11 +346,11 @@ class ScheduleView(
     @action(detail=True, methods=["get"])
     def next_shifts_per_user(self, request, pk):
         """Return next shift for users in schedule."""
-        user_tz, _ = self.get_request_timezone()
         now = timezone.now()
-        starting_date = now.date()
+        datetime_end = now + datetime.timedelta(days=30)
         schedule = self.get_object()
-        events = schedule.final_events(user_tz, starting_date, days=30)
+
+        events = schedule.final_events(now, datetime_end)
 
         users = {u.public_primary_key: None for u in schedule.related_users()}
         for e in events:
@@ -373,10 +382,11 @@ class ScheduleView(
         schedule = self.get_object()
 
         _, date = self.get_request_timezone()
+        datetime_start = datetime.datetime.combine(date, datetime.time.min, tzinfo=pytz.UTC)
         days = self.request.query_params.get("days")
         days = int(days) if days else None
 
-        return Response(schedule.quality_report(date, days))
+        return Response(schedule.quality_report(datetime_start, days))
 
     @action(detail=False, methods=["get"])
     def type_options(self, request):

engine/apps/mobile_app/tasks.py

@@ -439,7 +439,8 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk)
         return
 
     now = timezone.now()
-    schedule_final_events = schedule.final_events("UTC", now, days=7)
+    datetime_end = now + datetime.timedelta(days=7)
+    schedule_final_events = schedule.final_events(now, datetime_end)
 
     relevant_cache_keys = [
         _generate_going_oncall_push_notification_cache_key(user["pk"], schedule_event)

engine/apps/public_api/views/schedules.py

@@ -1,5 +1,7 @@
+import datetime
 import logging
 
+import pytz
 from django_filters import rest_framework as filters
 from rest_framework import status
 from rest_framework.decorators import action
@@ -147,8 +149,12 @@ class OnCallScheduleChannelView(RateLimitHeadersMixin, UpdateSerializerMixin, Mo
         end_date = serializer.validated_data["end_date"]
         days_between_start_and_end = (end_date - start_date).days
 
-        final_schedule_events: ScheduleEvents = schedule.final_events("UTC", start_date, days_between_start_and_end)
+        datetime_start = datetime.datetime.combine(start_date, datetime.time.min, tzinfo=pytz.UTC)
+        datetime_end = datetime_start + datetime.timedelta(
+            days=days_between_start_and_end - 1, hours=23, minutes=59, seconds=59
+        )
 
+        final_schedule_events: ScheduleEvents = schedule.final_events(datetime_start, datetime_end)
         logger.info(
             f"Exporting oncall shifts for schedule {pk} between dates {start_date} and {end_date}. {len(final_schedule_events)} shift events were found."
         )

engine/apps/schedules/ical_utils.py

@@ -61,7 +61,6 @@ IcalEvents = typing.List[IcalEvent]
 def users_in_ical(
     usernames_from_ical: typing.List[str],
     organization: "Organization",
-    include_viewers=False,
     users_to_filter: typing.Optional["UserQuerySet"] = None,
 ) -> typing.Sequence["User"]:
     """
@@ -94,11 +93,10 @@ def users_in_ical(
             }
         )
 
-    users_found_in_ical = organization.users
-    if not include_viewers:
-        users_found_in_ical = users_found_in_ical.filter(
-            **User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization)
-        )
+    # users_found_in_ical = organization.users
+    users_found_in_ical = organization.users.filter(
+        **User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization)
+    )
 
     users_found_in_ical = users_found_in_ical.filter(
         (Q(username__in=usernames_from_ical) | Q(email__lower__in=emails_from_ical))
@@ -118,11 +116,10 @@ def memoized_users_in_ical(
 # used for display schedule events on web
 def list_of_oncall_shifts_from_ical(
     schedule: "OnCallSchedule",
-    date: datetime.date,
-    user_timezone: str = "UTC",
+    datetime_start: datetime.datetime,
+    datetime_end: datetime.datetime,
     with_empty_shifts: bool = False,
     with_gaps: bool = False,
-    days: int = 1,
     filter_by: str | None = None,
     from_cached_final: bool = False,
 ):
@@ -152,16 +149,6 @@ def list_of_oncall_shifts_from_ical(
     else:
         calendars = schedule.get_icalendars()
 
-    # TODO: Review offset usage
-    pytz_tz = pytz.timezone(user_timezone)
-
-    # utcoffset can technically return None, but we're confident it is a timedelta here
-    user_timezone_offset: datetime.timedelta = datetime.datetime.now().astimezone(pytz_tz).utcoffset()  # type: ignore[assignment]
-
-    datetime_min = datetime.datetime.combine(date, datetime.time.min) + datetime.timedelta(milliseconds=1)
-    datetime_start = (datetime_min - user_timezone_offset).astimezone(pytz.UTC)
-    datetime_end = datetime_start + datetime.timedelta(days=days - 1, hours=23, minutes=59, seconds=59)
-
     result_datetime = []
     result_date = []
 
@@ -204,6 +191,7 @@ def list_of_oncall_shifts_from_ical(
             )
 
     def event_start_cmp_key(e):
+        pytz_tz = pytz.timezone("UTC")
         return (
             datetime.datetime.combine(e["start"], datetime.datetime.min.time(), tzinfo=pytz_tz)
             if type(e["start"]) == datetime.date
@@ -348,7 +336,6 @@ def list_of_empty_shifts_in_schedule(
 def list_users_to_notify_from_ical(
     schedule: "OnCallSchedule",
     events_datetime: typing.Optional[datetime.datetime] = None,
-    include_viewers: bool = False,
     users_to_filter: typing.Optional["UserQuerySet"] = None,
 ) -> typing.Sequence["User"]:
     """
@@ -359,7 +346,6 @@ def list_users_to_notify_from_ical(
         schedule,
         events_datetime,
         events_datetime,
-        include_viewers=include_viewers,
         users_to_filter=users_to_filter,
     )
 
@@ -368,35 +354,15 @@ def list_users_to_notify_from_ical_for_period(
     schedule: "OnCallSchedule",
     start_datetime: datetime.datetime,
     end_datetime: datetime.datetime,
-    include_viewers=False,
     users_to_filter=None,
 ) -> typing.Sequence["User"]:
-    # get list of iCalendars from current iCal files. If there is more than one calendar, primary calendar will always
-    # be the first
-    calendars = schedule.get_icalendars()
-    # reverse calendars to make overrides calendar the first, if schedule is iCal
-    calendars = calendars[::-1]
     users_found_in_ical: typing.Sequence["User"] = []
-    # at first check overrides calendar and return users from it if it exists and on-call users are found
-    for calendar in calendars:
-        if calendar is None:
-            continue
-        events = ical_events.get_events_from_ical_between(calendar, start_datetime, end_datetime)
+    events = schedule.final_events(start_datetime, end_datetime)
+    usernames = []
+    for event in events:
+        usernames += [u["email"] for u in event.get("users", [])]
 
-        parsed_ical_events: typing.Dict[int, typing.List[str]] = {}
-        for event in events:
-            current_usernames, current_priority = get_usernames_from_ical_event(event)
-            parsed_ical_events.setdefault(current_priority, []).extend(current_usernames)
-        # find users by usernames. if users are not found for shift, get users from lower priority
-        for _, usernames in sorted(parsed_ical_events.items(), reverse=True):
-            users_found_in_ical = users_in_ical(
-                usernames, schedule.organization, include_viewers=include_viewers, users_to_filter=users_to_filter
-            )
-            if users_found_in_ical:
-                break
-        if users_found_in_ical:
-            # if users are found in the overrides calendar, there is no need to check primary calendar
-            break
+    users_found_in_ical = users_in_ical(usernames, schedule.organization, users_to_filter=users_to_filter)
     return users_found_in_ical
 
 

engine/apps/schedules/models/on_call_schedule.py

@@ -307,9 +307,8 @@ class OnCallSchedule(PolymorphicModel):
 
     def filter_events(
         self,
-        user_timezone,
-        starting_date,
-        days,
+        datetime_start,
+        datetime_end,
         with_empty=False,
         with_gap=False,
         filter_by=None,
@@ -320,11 +319,10 @@ class OnCallSchedule(PolymorphicModel):
         shifts = (
             list_of_oncall_shifts_from_ical(
                 self,
-                starting_date,
-                user_timezone,
+                datetime_start,
+                datetime_end,
                 with_empty,
                 with_gap,
-                days=days,
                 filter_by=filter_by,
                 from_cached_final=from_cached_final,
             )
@@ -369,26 +367,23 @@ class OnCallSchedule(PolymorphicModel):
         # combine multiple-users same-shift events into one
         return self._merge_events(events)
 
-    def final_events(self, user_tz, starting_date, days):
+    def final_events(self, datetime_start, datetime_end):
         """Return schedule final events, after resolving shifts and overrides."""
-        events = self.filter_events(
-            user_tz, starting_date, days=days, with_empty=True, with_gap=True, all_day_datetime=True
-        )
-        events = self._resolve_schedule(events)
+        events = self.filter_events(datetime_start, datetime_end, with_empty=True, with_gap=True, all_day_datetime=True)
+        events = self._resolve_schedule(events, datetime_start, datetime_end)
         return events
 
     def refresh_ical_final_schedule(self):
-        tz = "UTC"
         now = timezone.now()
         # window to consider: from now, -15 days + 6 months
         delta = EXPORT_WINDOW_DAYS_BEFORE
-        starting_datetime = now - datetime.timedelta(days=delta)
-        starting_date = starting_datetime.date()
         days = EXPORT_WINDOW_DAYS_AFTER + delta
+        datetime_start = now.replace(hour=0, minute=0, second=0, microsecond=0) - datetime.timedelta(days=delta)
+        datetime_end = datetime_start + datetime.timedelta(days=days - 1, hours=23, minutes=59, seconds=59)
 
         # setup calendar with final schedule shift events
         calendar = create_base_icalendar(self.name)
-        events = self.final_events(tz, starting_date, days)
+        events = self.final_events(datetime_start, datetime_end)
         updated_ids = set()
         for e in events:
             for u in e["users"]:
@@ -417,12 +412,12 @@ class OnCallSchedule(PolymorphicModel):
                         dtend_datetime = datetime.datetime.combine(
                             dtend.dt, datetime.datetime.min.time(), tzinfo=pytz.UTC
                         )
-                    if dtend_datetime and dtend_datetime < starting_datetime:
+                    if dtend_datetime and dtend_datetime < datetime_start:
                         # event ended before window start
                         continue
                     is_cancelled = component.get(ICAL_STATUS)
                     last_modified = component.get(ICAL_LAST_MODIFIED)
-                    if is_cancelled and last_modified and last_modified.dt < starting_datetime:
+                    if is_cancelled and last_modified and last_modified.dt < datetime_start:
                         # drop already ended events older than the window we consider
                         continue
                     elif is_cancelled and not last_modified:
@@ -441,17 +436,18 @@ class OnCallSchedule(PolymorphicModel):
         self.save(update_fields=["cached_ical_final_schedule"])
 
     def upcoming_shift_for_user(self, user, days=7):
-        user_tz = user.timezone or "UTC"
         now = timezone.now()
         # consider an extra day before to include events from UTC yesterday
-        starting_date = now.date() - datetime.timedelta(days=1)
+        datetime_start = now - datetime.timedelta(days=1)
+        datetime_end = datetime_start + datetime.timedelta(days=days)
+
         current_shift = upcoming_shift = None
 
         if self.cached_ical_final_schedule is None:
             # no final schedule info available
             return None, None
 
-        events = self.filter_events(user_tz, starting_date, days=days, all_day_datetime=True, from_cached_final=True)
+        events = self.filter_events(datetime_start, datetime_end, all_day_datetime=True, from_cached_final=True)
         for e in events:
             if e["end"] < now:
                 # shift is finished, ignore
@@ -475,13 +471,13 @@ class OnCallSchedule(PolymorphicModel):
         """
         # get events to consider for calculation
         if date is None:
-            today = datetime.datetime.now(tz=timezone.utc)
+            today = timezone.now()
             date = today - datetime.timedelta(days=7 - today.weekday())  # start of next week in UTC
         if days is None:
             days = 52 * 7  # consider next 52 weeks (~1 year)
+        datetime_end = date + datetime.timedelta(days=days - 1, hours=23, minutes=59, seconds=59)
 
-        events = self.final_events(user_tz="UTC", starting_date=date, days=days)
-
+        events = self.final_events(date, datetime_end)
         # an event is “good” if it's not a gap and not empty
         good_events: ScheduleEvents = [event for event in events if not event["is_gap"] and not event["is_empty"]]
         if not good_events:
@@ -591,8 +587,13 @@ class OnCallSchedule(PolymorphicModel):
             "overloaded_users": overloaded_users,
         }
 
-    def _resolve_schedule(self, events: ScheduleEvents) -> ScheduleEvents:
-        """Calculate final schedule shifts considering rotations and overrides."""
+    def _resolve_schedule(
+        self, events: ScheduleEvents, datetime_start: datetime.datetime, datetime_end: datetime.datetime
+    ) -> ScheduleEvents:
+        """Calculate final schedule shifts considering rotations and overrides.
+
+        Exclude events that after split/update are out of the requested (datetime_start, datetime_end) range.
+        """
         if not events:
             return []
 
@@ -678,16 +679,20 @@ class OnCallSchedule(PolymorphicModel):
                 # 1. add a split event copy to schedule the time before the already scheduled interval
                 to_add = ev.copy()
                 to_add["end"] = intervals[current_interval_idx][0]
-                resolved.append(to_add)
+                if to_add["end"] >= datetime_start:
+                    # only include if updated event ends inside the requested time range
+                    resolved.append(to_add)
                 # 2. check if there is still time to be scheduled after the current scheduled interval ends
                 if ev["end"] > intervals[current_interval_idx][1]:
                     # event ends after current interval, update event start timestamp to match the interval end
                     # and process the updated event as any other event
                     ev["start"] = intervals[current_interval_idx][1]
-                    # reorder pending events after updating current event start date
-                    # (ie. insert the event where it should be to keep the order criteria)
-                    # TODO: switch to bisect insert on python 3.10 (or consider heapq)
-                    insort_event(pending, ev)
+                    if ev["start"] < datetime_end:
+                        # only include event if it is still inside the requested time range
+                        # reorder pending events after updating current event start date
+                        # (ie. insert the event where it should be to keep the order criteria)
+                        # TODO: switch to bisect insert on python 3.10 (or consider heapq)
+                        insort_event(pending, ev)
                 # done, go to next event
 
             elif ev["start"] >= intervals[current_interval_idx][0] and ev["end"] <= intervals[current_interval_idx][1]:
@@ -757,7 +762,7 @@ class OnCallSchedule(PolymorphicModel):
             ical += f"{end_line}\r\n"
         return ical
 
-    def preview_shift(self, custom_shift, user_tz, starting_date, days, updated_shift_pk=None):
+    def preview_shift(self, custom_shift, datetime_start, datetime_end, updated_shift_pk=None):
         """Return unsaved rotation and final schedule preview events."""
         if custom_shift.type == CustomOnCallShift.TYPE_OVERRIDE:
             qs = self.custom_shifts.filter(type=CustomOnCallShift.TYPE_OVERRIDE)
@@ -790,11 +795,12 @@ class OnCallSchedule(PolymorphicModel):
         setattr(self, ical_attr, ical_file)
 
         # filter events using a temporal overriden calendar including the not-yet-saved shift
-        events = self.filter_events(user_tz, starting_date, days=days, with_empty=True, with_gap=True)
+        events = self.filter_events(datetime_start, datetime_end, with_empty=True, with_gap=True)
+
         # return preview events for affected shifts
         updated_shift_pks = {s.public_primary_key for s in extra_shifts}
         shift_events = [e.copy() for e in events if e["shift"]["pk"] in updated_shift_pks]
-        final_events = self._resolve_schedule(events)
+        final_events = self._resolve_schedule(events, datetime_start, datetime_end)
 
         _invalidate_cache(self, ical_property)
         setattr(self, ical_attr, original_value)
@@ -993,11 +999,11 @@ class OnCallScheduleCalendar(OnCallSchedule):
             ical += f"{end_line}\r\n"
         return ical
 
-    def preview_shift(self, custom_shift, user_tz, starting_date, days, updated_shift_pk=None):
+    def preview_shift(self, custom_shift, datetime_start, datetime_end, updated_shift_pk=None):
         """Return unsaved rotation and final schedule preview events."""
         if custom_shift.type != CustomOnCallShift.TYPE_OVERRIDE:
             raise ValueError("Invalid shift type")
-        return super().preview_shift(custom_shift, user_tz, starting_date, days, updated_shift_pk=updated_shift_pk)
+        return super().preview_shift(custom_shift, datetime_start, datetime_end, updated_shift_pk=updated_shift_pk)
 
     @property
     def insight_logs_type_verbal(self):

engine/apps/schedules/tests/test_ical_utils.py

@@ -83,23 +83,18 @@ def test_users_in_ical_email_case_insensitive(make_organization_and_user, make_u
 
 
 @pytest.mark.django_db
-@pytest.mark.parametrize("include_viewers", [True, False])
-def test_users_in_ical_viewers_inclusion(make_organization_and_user, make_user_for_organization, include_viewers):
+def test_users_in_ical_viewers_inclusion(make_organization_and_user, make_user_for_organization):
     organization, user = make_organization_and_user()
     viewer = make_user_for_organization(organization, role=LegacyAccessControlRole.VIEWER)
 
     usernames = [user.username, viewer.username]
-    result = users_in_ical(usernames, organization, include_viewers=include_viewers)
-    if include_viewers:
-        assert set(result) == {user, viewer}
-    else:
-        assert set(result) == {user}
+    result = users_in_ical(usernames, organization)
+    assert set(result) == {user}
 
 
 @pytest.mark.django_db
-@pytest.mark.parametrize("include_viewers", [True, False])
 def test_list_users_to_notify_from_ical_viewers_inclusion(
-    make_organization_and_user, make_user_for_organization, make_schedule, make_on_call_shift, include_viewers
+    make_organization_and_user, make_user_for_organization, make_schedule, make_on_call_shift
 ):
     organization, user = make_organization_and_user()
     viewer = make_user_for_organization(organization, role=LegacyAccessControlRole.VIEWER)
@@ -121,14 +116,10 @@ def test_list_users_to_notify_from_ical_viewers_inclusion(
 
     # get users on-call
     date = date + timezone.timedelta(minutes=5)
-    users_on_call = list_users_to_notify_from_ical(schedule, date, include_viewers=include_viewers)
+    users_on_call = list_users_to_notify_from_ical(schedule, date)
 
-    if include_viewers:
-        assert len(users_on_call) == 2
-        assert set(users_on_call) == {user, viewer}
-    else:
-        assert len(users_on_call) == 1
-        assert set(users_on_call) == {user}
+    assert len(users_on_call) == 1
+    assert set(users_on_call) == {user}
 
 
 @pytest.mark.django_db
@@ -161,7 +152,49 @@ def test_list_users_to_notify_from_ical_until_terminated_event(
     date = date + timezone.timedelta(minutes=5)
     # this should not raise despite the shift configuration (until < rotation start)
     users_on_call = list_users_to_notify_from_ical(schedule, date)
-    assert users_on_call == []
+    assert list(users_on_call) == []
+
+
+@pytest.mark.django_db
+def test_list_users_to_notify_from_ical_overlapping_events(
+    make_organization_and_user, make_user_for_organization, make_schedule, make_on_call_shift
+):
+    organization, user = make_organization_and_user()
+    another_user = make_user_for_organization(organization)
+
+    schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb)
+    start = timezone.now() - timezone.timedelta(hours=1)
+    data = {
+        "start": start,
+        "rotation_start": start,
+        "duration": timezone.timedelta(hours=3),
+        "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]])
+
+    data = {
+        "start": start + timezone.timedelta(minutes=30),
+        "rotation_start": start + timezone.timedelta(minutes=30),
+        "duration": timezone.timedelta(hours=2),
+        "priority_level": 2,
+        "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([[another_user]])
+
+    # get users on-call now
+    users_on_call = list_users_to_notify_from_ical(schedule)
+
+    assert len(users_on_call) == 1
+    assert set(users_on_call) == {another_user}
 
 
 @pytest.mark.django_db
@@ -173,17 +206,26 @@ def test_shifts_dict_all_day_middle_event(make_organization, make_schedule, get_
 
     day_to_check_iso = "2021-01-27T15:27:14.448059+00:00"
     parsed_iso_day_to_check = datetime.datetime.fromisoformat(day_to_check_iso).replace(tzinfo=pytz.UTC)
-    requested_date = (parsed_iso_day_to_check - timezone.timedelta(days=1)).date()
-    shifts = list_of_oncall_shifts_from_ical(schedule, requested_date, days=3, with_empty_shifts=True)
+    requested_datetime = parsed_iso_day_to_check - timezone.timedelta(days=1)
+    datetime_end = requested_datetime + timezone.timedelta(days=2)
+    shifts = list_of_oncall_shifts_from_ical(schedule, requested_datetime, datetime_end, with_empty_shifts=True)
     assert len(shifts) == 5
     for s in shifts:
-        start = s["start"].date() if isinstance(s["start"], datetime.datetime) else s["start"]
-        end = s["end"].date() if isinstance(s["end"], datetime.datetime) else s["end"]
+        start = (
+            s["start"]
+            if isinstance(s["start"], datetime.datetime)
+            else datetime.datetime.combine(s["start"], datetime.time.min, tzinfo=pytz.UTC)
+        )
+        end = (
+            s["end"]
+            if isinstance(s["end"], datetime.datetime)
+            else datetime.datetime.combine(s["start"], datetime.time.max, tzinfo=pytz.UTC)
+        )
         # event started in the given period, or ended in that period, or is happening during the period
         assert (
-            requested_date <= start <= requested_date + timezone.timedelta(days=3)
-            or requested_date <= end <= requested_date + timezone.timedelta(days=3)
-            or start <= requested_date <= end
+            requested_datetime <= start <= requested_datetime + timezone.timedelta(days=2)
+            or requested_datetime <= end <= requested_datetime + timezone.timedelta(days=2)
+            or start <= requested_datetime <= end
         )
 
 
@@ -197,7 +239,8 @@ def test_shifts_dict_from_cached_final(
     organization = make_organization()
     u1 = make_user_for_organization(organization)
 
-    yesterday = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) - timezone.timedelta(days=1)
+    today = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0)
+    yesterday = today - timezone.timedelta(days=1)
     schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb)
     data = {
         "start": yesterday + timezone.timedelta(hours=10),
@@ -227,7 +270,7 @@ def test_shifts_dict_from_cached_final(
 
     shifts = [
         (s["calendar_type"], s["start"], list(s["users"]))
-        for s in list_of_oncall_shifts_from_ical(schedule, yesterday, days=1, from_cached_final=True)
+        for s in list_of_oncall_shifts_from_ical(schedule, yesterday, today, from_cached_final=True)
     ]
     expected_events = [
         (OnCallSchedule.PRIMARY, on_call_shift.start, [u1]),

engine/apps/schedules/tests/test_on_call_schedule.py

@@ -81,7 +81,8 @@ def test_filter_events(make_organization, make_user_for_organization, make_sched
     override.add_rolling_users([[user]])
 
     # filter primary non-empty shifts only
-    events = schedule.filter_events("UTC", start_date, days=3, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY)
+    end_date = start_date + timezone.timedelta(days=3)
+    events = schedule.filter_events(start_date, end_date, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY)
     expected = [
         {
             "calendar_type": OnCallSchedule.TYPE_ICAL_PRIMARY,
@@ -109,7 +110,8 @@ def test_filter_events(make_organization, make_user_for_organization, make_sched
     assert events == expected
 
     # filter overrides only
-    events = schedule.filter_events("UTC", start_date, days=3, filter_by=OnCallSchedule.TYPE_ICAL_OVERRIDES)
+    end_date = start_date + timezone.timedelta(days=3)
+    events = schedule.filter_events(start_date, end_date, filter_by=OnCallSchedule.TYPE_ICAL_OVERRIDES)
     expected_override = [
         {
             "calendar_type": OnCallSchedule.TYPE_ICAL_OVERRIDES,
@@ -136,7 +138,8 @@ def test_filter_events(make_organization, make_user_for_organization, make_sched
     assert events == expected_override
 
     # no type filter
-    events = schedule.filter_events("UTC", start_date, days=3)
+    end_date = start_date + timezone.timedelta(days=3)
+    events = schedule.filter_events(start_date, end_date)
     assert events == expected_override + expected
 
 
@@ -165,13 +168,12 @@ def test_filter_events_include_gaps(make_organization, make_user_for_organizatio
     )
     on_call_shift.add_rolling_users([[user]])
 
-    events = schedule.filter_events(
-        "UTC", start_date, days=1, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY, with_gap=True
-    )
+    end_date = start_date + timezone.timedelta(days=1)
+    events = schedule.filter_events(start_date, end_date, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY, with_gap=True)
     expected = [
         {
             "calendar_type": None,
-            "start": start_date + timezone.timedelta(milliseconds=1),
+            "start": start_date,
             "end": on_call_shift.start,
             "all_day": False,
             "is_override": False,
@@ -207,7 +209,7 @@ def test_filter_events_include_gaps(make_organization, make_user_for_organizatio
         {
             "calendar_type": None,
             "start": on_call_shift.start + on_call_shift.duration,
-            "end": on_call_shift.start + timezone.timedelta(hours=13, minutes=59, seconds=59, milliseconds=1),
+            "end": on_call_shift.start + timezone.timedelta(hours=14),
             "all_day": False,
             "is_override": False,
             "is_empty": False,
@@ -247,9 +249,8 @@ def test_filter_events_include_empty(make_organization, make_user_for_organizati
     )
     on_call_shift.add_rolling_users([[user]])
 
-    events = schedule.filter_events(
-        "UTC", start_date, days=1, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY, with_empty=True
-    )
+    end_date = start_date + timezone.timedelta(days=1)
+    events = schedule.filter_events(start_date, end_date, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY, with_empty=True)
     expected = [
         {
             "calendar_type": OnCallSchedule.TYPE_ICAL_PRIMARY,
@@ -282,9 +283,10 @@ def test_filter_events_ical_all_day(make_organization, make_user_for_organizatio
 
     day_to_check_iso = "2021-01-27T15:27:14.448059+00:00"
     parsed_iso_day_to_check = datetime.datetime.fromisoformat(day_to_check_iso).replace(tzinfo=pytz.UTC)
-    start_date = (parsed_iso_day_to_check - timezone.timedelta(days=1)).date()
+    datetime_start = parsed_iso_day_to_check - timezone.timedelta(days=1)
+    datetime_end = datetime_start + datetime.timedelta(days=1, hours=23, minutes=59, seconds=59)
 
-    events = schedule.final_events("UTC", start_date, days=2)
+    events = schedule.final_events(datetime_start, datetime_end)
     expected_events = [
         # all_day, users, start, end
         (
@@ -311,6 +313,12 @@ def test_filter_events_ical_all_day(make_organization, make_user_for_organizatio
             datetime.datetime(2021, 1, 27, 8, 0, tzinfo=pytz.UTC),
             datetime.datetime(2021, 1, 27, 17, 0, tzinfo=pytz.UTC),
         ),
+        (
+            False,
+            ["@Bernard Desruisseaux"],
+            datetime.datetime(2021, 1, 28, 8, 0, tzinfo=pytz.UTC),
+            datetime.datetime(2021, 1, 28, 17, 0, tzinfo=pytz.UTC),
+        ),
     ]
     expected = [
         {"all_day": all_day, "users": users, "start": start, "end": end}
@@ -388,7 +396,8 @@ def test_final_schedule_events(make_organization, make_user_for_organization, ma
         )
         on_call_shift.add_rolling_users([[user]])
 
-    returned_events = schedule.final_events("UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    returned_events = schedule.final_events(start_date, datetime_end)
 
     expected = (
         # start (h), duration (H), user, priority, is_gap, is_override
@@ -414,7 +423,7 @@ def test_final_schedule_events(make_organization, make_user_for_organization, ma
             "is_gap": is_gap,
             "is_override": is_override,
             "priority_level": priority,
-            "start": start_date + timezone.timedelta(hours=start, milliseconds=1 if start == 0 else 0),
+            "start": start_date + timezone.timedelta(hours=start),
             "user": user,
         }
         for start, duration, user, priority, is_gap, is_override in expected
@@ -482,7 +491,8 @@ def test_final_schedule_override_no_priority_shift(
     )
     override.add_rolling_users([[user_b]])
 
-    returned_events = schedule.final_events("UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    returned_events = schedule.final_events(start_date, datetime_end)
 
     expected = (
         # start (h), duration (H), user, priority, is_override
@@ -552,7 +562,8 @@ def test_final_schedule_splitting_events(
         )
         on_call_shift.add_rolling_users([[user]])
 
-    returned_events = schedule.final_events("UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    returned_events = schedule.final_events(start_date, datetime_end)
 
     expected = (
         # start (h), duration (H), user, priority
@@ -621,7 +632,8 @@ def test_final_schedule_splitting_same_time_events(
         )
         on_call_shift.add_rolling_users([[user]])
 
-    returned_events = schedule.final_events("UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    returned_events = schedule.final_events(start_date, datetime_end)
 
     expected = (
         # start (h), duration (H), user, priority
@@ -695,7 +707,8 @@ def test_preview_shift(make_organization, make_user_for_organization, make_sched
         rolling_users=[{other_user.pk: other_user.public_primary_key}],
     )
 
-    rotation_events, final_events = schedule.preview_shift(new_shift, "UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    rotation_events, final_events = schedule.preview_shift(new_shift, start_date, datetime_end)
 
     # check rotation events
     expected_rotation_events = [
@@ -796,7 +809,8 @@ def test_preview_shift_do_not_change_rotation_events(
     )
     other_shift.add_rolling_users([[other_user]])
 
-    rotation_events, final_events = schedule.preview_shift(on_call_shift, "UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    rotation_events, final_events = schedule.preview_shift(on_call_shift, start_date, datetime_end)
 
     # check rotation events
     expected_rotation_events = [
@@ -852,7 +866,8 @@ def test_preview_shift_no_user(make_organization, make_user_for_organization, ma
         rolling_users=[],
     )
 
-    rotation_events, final_events = schedule.preview_shift(new_shift, "UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    rotation_events, final_events = schedule.preview_shift(new_shift, start_date, datetime_end)
 
     # check rotation events
     expected_rotation_events = [
@@ -930,7 +945,8 @@ def test_preview_override_shift(make_organization, make_user_for_organization, m
         rolling_users=[{other_user.pk: other_user.public_primary_key}],
     )
 
-    rotation_events, final_events = schedule.preview_shift(new_shift, "UTC", start_date, days=1)
+    datetime_end = start_date + timezone.timedelta(days=1)
+    rotation_events, final_events = schedule.preview_shift(new_shift, start_date, datetime_end)
 
     # check rotation events
     expected_rotation_events = [
@@ -1089,7 +1105,8 @@ def test_filter_events_none_cache_unchanged(
     # schedule is removed from db
     schedule.delete()
 
-    events = schedule.filter_events("UTC", start_date, days=5, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY)
+    end_date = start_date + timezone.timedelta(days=5)
+    events = schedule.filter_events(start_date, end_date, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY)
     expected = []
     assert events == expected
 
@@ -1272,7 +1289,8 @@ def test_api_schedule_preview_requires_override(make_organization, make_schedule
     )
 
     with pytest.raises(ValueError):
-        schedule.preview_shift(non_override_shift, "UTC", now, 1)
+        datetime_end = now + timezone.timedelta(days=1)
+        schedule.preview_shift(non_override_shift, now, datetime_end)
 
 
 @pytest.mark.django_db
@@ -1817,4 +1835,5 @@ def test_event_until_non_utc(make_organization, make_schedule):
     now = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0)
 
     # check this works without raising exception
-    schedule.final_events("UTC", now, days=7)
+    datetime_end = now + timezone.timedelta(days=7)
+    schedule.final_events(now, datetime_end)

engine/apps/schedules/tests/test_quality_score.py

@@ -190,7 +190,7 @@ def test_get_schedule_score_weekdays(
     assert response.json() == {
         "total_score": 86,
         "comments": [
-            {"type": "warning", "text": "Schedule has gaps (29% not covered)"},
+            {"type": "warning", "text": "Schedule has gaps (28% not covered)"},
             {"type": "info", "text": "Schedule is perfectly balanced"},
         ],
         "overloaded_users": [],
@@ -351,7 +351,7 @@ def test_get_schedule_score_all_week_imbalanced_weekends(
             {
                 "id": user.public_primary_key,
                 "username": user.username,
-                "score": 29,
+                "score": 28,
             }
             for user in users[:4]
         ],