From c65a3c9ceaabb557bb296feecd9bb23737ef9649 Mon Sep 17 00:00:00 2001 From: Michael Derynck Date: Thu, 3 Oct 2024 14:04:55 -0600 Subject: [PATCH] Fix for user appearing in shift rotation when they should not be (#5064) # What this PR does Fix calculation for interval when building ical events for schedules with end dates and masked days. Add a test which makes it easier to test different cases that can occur. ## Which issue(s) this PR closes Related to https://github.com/grafana/support-escalations/issues/12388 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes. --------- Co-authored-by: Matias Bordese --- .../schedules/models/custom_on_call_shift.py | 51 +- .../tests/custom_shift_test_cases.py | 474 ++++++++++++++++++ .../tests/test_custom_on_call_shift.py | 74 +++ 3 files changed, 581 insertions(+), 18 deletions(-) create mode 100644 engine/apps/schedules/tests/custom_shift_test_cases.py diff --git a/engine/apps/schedules/models/custom_on_call_shift.py b/engine/apps/schedules/models/custom_on_call_shift.py index 8fcdb38a..812b0947 100644 --- a/engine/apps/schedules/models/custom_on_call_shift.py +++ b/engine/apps/schedules/models/custom_on_call_shift.py @@ -323,38 +323,53 @@ class CustomOnCallShift(models.Model): # we may need to iterate several times over users until we get a seen combination # use the group index as reference since user groups could repeat in the queue cycle_user_groups = itertools.cycle(range(len(users_queue))) - orig_start = last_start = start + previous_day = None all_rotations_checked = False # we need to go through each individual day day_by_day_rrule = copy.deepcopy(self.event_ical_rules) day_by_day_rrule["interval"] = 1 for user_group_id in cycle_user_groups: for i in range(self.interval): - if not start: # means that rotation ends before next event starts - all_rotations_checked = True - break - last_start = start - day = CustomOnCallShift.ICAL_WEEKDAY_MAP[start.weekday()] - # double-check day is valid (when until is set, we may get unexpected days) - if day in self.by_day: + if not start: + # means that rotation ended before next event starts + # keep iterating to track missing combinations and get the right week_interval + if previous_day is None: + day = self.by_day[0] + else: + previous_index = self.by_day.index(previous_day) + day = self.by_day[(previous_index + 1) % len(self.by_day)] + if (user_group_id, day, i) in combinations: all_rotations_checked = True break - - starting_dates.append(start) combinations.append((user_group_id, day, i)) - # get next event date following the original rule - event_ical = self.generate_ical(start, 1, None, 1, time_zone, custom_rrule=day_by_day_rrule) - start = self.get_rotation_date(event_ical, get_next_date=True, interval=1) + previous_day = day + else: + day = CustomOnCallShift.ICAL_WEEKDAY_MAP[start.weekday()] + # double-check day is valid (when until is set, we may get unexpected days) + if day in self.by_day: + if (user_group_id, day, i) in combinations: + all_rotations_checked = True + break + + starting_dates.append(start) + combinations.append((user_group_id, day, i)) + previous_day = day + # get next event date following the original rule + event_ical = self.generate_ical(start, 1, None, 1, time_zone, custom_rrule=day_by_day_rrule) + start = self.get_rotation_date(event_ical, get_next_date=True, interval=1) + if all_rotations_checked: break - week_interval = 1 - if orig_start and last_start: - # number of weeks used to cover all combinations - week_interval = ((last_start - orig_start).days // 7) or 1 + # interval is given by the number of weeks required to cover all combinations + week_interval = (len(combinations) // len(self.by_day)) or 1 + counter = 1 - for (user_group_id, day, _), start in zip(combinations, starting_dates): + for (user_group_id, day, _), start in itertools.zip_longest(combinations, starting_dates, fillvalue=None): + if not start: + # means that rotation ended before next event starts, no more events to generate + break users = users_queue[user_group_id] for user_counter, user in enumerate(users, start=1): # setup weekly events, for each user group/day combinations, diff --git a/engine/apps/schedules/tests/custom_shift_test_cases.py b/engine/apps/schedules/tests/custom_shift_test_cases.py new file mode 100644 index 00000000..14f5a70e --- /dev/null +++ b/engine/apps/schedules/tests/custom_shift_test_cases.py @@ -0,0 +1,474 @@ +from typing import List, Tuple + +from apps.schedules.models import CustomOnCallShift + +""" +Simplified shift definition to test that shifts contain the correct users. +This test is only checking the start date of the shift is landing on the correct day. +If you need a test for precise hours that should be a different test so that these cases +can stay easy to define. + +Define custom shift test case parameters: +users_per_group - Recurrence groups, generates users for each group based on number, Users are named A-Z +shift_start - Date to start shift +day_mask - Which days are included, None to include all +total_days - Length of shift (end = start + total_days) +frequency - Recurrence frequency +interval - Recurrence interval +expected_result - Dict where each day displays the users that should be scheduled +""" +CUSTOM_SHIFT_TEST_CASES: List[Tuple] = [ + ( + # 0 - Original Test Case + [1, 1, 1], + "2024-08-01", + ["FR"], + 21, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-02": "A", + "2024-08-09": "B", + "2024-08-16": "C", + }, + ), + ( + # 1 - Weekdays Start Monday Daily + [1, 1, 1, 1, 1], + "2024-08-05", + ["MO", "TU", "WE", "TH", "FR"], + 14, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-05": "A", + "2024-08-06": "B", + "2024-08-07": "C", + "2024-08-08": "D", + "2024-08-09": "E", + "2024-08-12": "A", + "2024-08-13": "B", + "2024-08-14": "C", + "2024-08-15": "D", + "2024-08-16": "E", + }, + ), + ( + # 2 - Weekdays Start Monday Daily Interval 2 + [1, 1, 1, 1], + "2024-08-05", + ["MO", "TU", "WE", "TH", "FR"], + 14, + CustomOnCallShift.FREQUENCY_DAILY, + 2, + { + "2024-08-05": "A", + "2024-08-06": "A", + "2024-08-07": "B", + "2024-08-08": "B", + "2024-08-09": "C", + "2024-08-12": "C", + "2024-08-13": "D", + "2024-08-14": "D", + "2024-08-15": "A", + "2024-08-16": "A", + }, + ), + ( + # 3 - Weekdays Start Monday Weekly + [1, 1, 1, 1, 1], + "2024-08-05", + ["MO", "TU", "WE", "TH", "FR"], + 14, + CustomOnCallShift.FREQUENCY_WEEKLY, + 1, + { + "2024-08-05": "A", + "2024-08-06": "A", + "2024-08-07": "A", + "2024-08-08": "A", + "2024-08-09": "A", + "2024-08-12": "B", + "2024-08-13": "B", + "2024-08-14": "B", + "2024-08-15": "B", + "2024-08-16": "B", + }, + ), + ( + # 4 - Weekdays Start Monday Monthly + [1, 1, 1, 1, 1], + "2024-08-26", + ["MO", "TU", "WE", "TH", "FR"], + 14, + CustomOnCallShift.FREQUENCY_MONTHLY, + 1, + { + "2024-08-26": "A", + "2024-08-27": "A", + "2024-08-28": "A", + "2024-08-29": "A", + "2024-08-30": "A", + "2024-09-02": "B", + "2024-09-03": "B", + "2024-09-04": "B", + "2024-09-05": "B", + "2024-09-06": "B", + }, + ), + ( + # 5 - Weekdays Start Thursday Daily + [1, 1, 1, 1, 1], + "2024-08-08", + ["MO", "TU", "WE", "TH", "FR"], + 14, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-08": "A", + "2024-08-09": "B", + "2024-08-12": "C", + "2024-08-13": "D", + "2024-08-14": "E", + "2024-08-15": "A", + "2024-08-16": "B", + "2024-08-19": "C", + "2024-08-20": "D", + "2024-08-21": "E", + }, + ), + ( + # 6 - Weekdays Start Thursday Weekly + [1, 1, 1, 1, 1], + "2024-08-08", + ["MO", "TU", "WE", "TH", "FR"], + 14, + CustomOnCallShift.FREQUENCY_WEEKLY, + 1, + { + "2024-08-08": "A", + "2024-08-09": "A", + "2024-08-12": "B", + "2024-08-13": "B", + "2024-08-14": "B", + "2024-08-15": "B", + "2024-08-16": "B", + "2024-08-19": "C", + "2024-08-20": "C", + "2024-08-21": "C", + }, + ), + ( + # 7 - Weekdays Start Thursday Monthly + [1, 1, 1, 1, 1], + "2024-08-29", + ["MO", "TU", "WE", "TH", "FR"], + 14, + CustomOnCallShift.FREQUENCY_MONTHLY, + 1, + { + "2024-08-29": "A", + "2024-08-30": "A", + "2024-09-02": "B", + "2024-09-03": "B", + "2024-09-04": "B", + "2024-09-05": "B", + "2024-09-06": "B", + "2024-09-09": "B", + "2024-09-10": "B", + "2024-09-11": "B", + }, + ), + ( + # 8 - All Days uneven groups + [2, 1], + "2024-08-14", + None, + 9, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-14": "AB", + "2024-08-15": "C", + "2024-08-16": "AB", + "2024-08-17": "C", + "2024-08-18": "AB", + "2024-08-19": "C", + "2024-08-20": "AB", + "2024-08-21": "C", + "2024-08-22": "AB", + }, + ), + ( + # 9 - Weekends Start Saturday + [1, 1, 1], + "2024-08-03", + ["SA", "SU"], + 15, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-03": "A", + "2024-08-04": "B", + "2024-08-10": "C", + "2024-08-11": "A", + "2024-08-17": "B", + }, + ), + ( + # 10 - Weekends Start Saturday Users > Shifts + [1, 1, 1, 1, 1], + "2024-08-03", + ["SA", "SU"], + 14, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-03": "A", + "2024-08-04": "B", + "2024-08-10": "C", + "2024-08-11": "D", + }, + ), + ( + # 11 - Weekends Start Saturday Users > Shifts + [1, 1, 1, 1, 1], + "2024-08-03", + ["SA", "SU"], + 14, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-03": "A", + "2024-08-04": "B", + "2024-08-10": "C", + "2024-08-11": "D", + }, + ), + ( + # 12 - Weekends Start Thursday + [1], + "2024-08-01", + ["SA", "SU"], + 5, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-03": "A", + "2024-08-04": "A", + }, + ), + ( + # 13 - Weekends Start Thursday + [1, 1, 1], + "2024-08-01", + ["SA", "SU"], + 17, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-03": "A", + "2024-08-04": "B", + "2024-08-10": "C", + "2024-08-11": "A", + "2024-08-17": "B", + }, + ), + ( + # 14 - Weekends Start Thursday Users > Shifts + [1, 1, 1, 1, 1, 1], + "2024-08-01", + ["SA", "SU"], + 17, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-08-03": "A", + "2024-08-04": "B", + "2024-08-10": "C", + "2024-08-11": "D", + "2024-08-17": "E", + }, + ), + ( + # 15 - Weekends Start Thursday Interval 2 + [1, 1, 1], + "2024-08-01", + ["SA", "SU"], + 17, + CustomOnCallShift.FREQUENCY_DAILY, + 2, + { + "2024-08-03": "A", + "2024-08-04": "A", + "2024-08-10": "B", + "2024-08-11": "B", + "2024-08-17": "C", + }, + ), + ( + # 16 - Weekends Start Saturday Weekly + [1, 1, 1], + "2024-08-01", + ["SA", "SU"], + 18, + CustomOnCallShift.FREQUENCY_WEEKLY, + 1, + { + "2024-08-03": "A", + "2024-08-04": "A", + "2024-08-10": "B", + "2024-08-11": "B", + "2024-08-17": "C", + "2024-08-18": "C", + }, + ), + ( + # 17 - Weekends Start Saturday Weekly Interval 2 + [1, 1, 1], + "2024-08-01", + ["SA", "SU"], + 18, + CustomOnCallShift.FREQUENCY_WEEKLY, + 2, + { + "2024-08-03": "A", + "2024-08-04": "A", + "2024-08-17": "B", + "2024-08-18": "B", + }, + ), + ( + # 18 - Weekends Start Thursday Monthly + [1, 1, 1], + "2024-08-29", + ["SA", "SU"], + 18, + CustomOnCallShift.FREQUENCY_MONTHLY, + 1, + { + "2024-08-31": "A", + "2024-09-01": "B", + "2024-09-07": "B", + "2024-09-08": "B", + "2024-09-14": "B", + "2024-09-15": "B", + }, + ), + ( + # 19 - No days (==All) + [1, 1, 1, 1, 1, 1, 1, 1, 1, 1], + "2024-09-30", + [], + 18, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-09-30": "A", + "2024-10-01": "B", + "2024-10-02": "C", + "2024-10-03": "D", + "2024-10-04": "E", + "2024-10-05": "F", + "2024-10-06": "G", + "2024-10-07": "H", + "2024-10-08": "I", + "2024-10-09": "J", + "2024-10-10": "A", + "2024-10-11": "B", + "2024-10-12": "C", + "2024-10-13": "D", + "2024-10-14": "E", + "2024-10-15": "F", + "2024-10-16": "G", + "2024-10-17": "H", + }, + ), + ( + # 20 - All days + [1, 1, 1, 1, 1, 1, 1, 1, 1, 1], + "2024-09-30", + ["MO", "TU", "WE", "TH", "FR", "SA", "SU"], + 18, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + { + "2024-09-30": "A", + "2024-10-01": "B", + "2024-10-02": "C", + "2024-10-03": "D", + "2024-10-04": "E", + "2024-10-05": "F", + "2024-10-06": "G", + "2024-10-07": "H", + "2024-10-08": "I", + "2024-10-09": "J", + "2024-10-10": "A", + "2024-10-11": "B", + "2024-10-12": "C", + "2024-10-13": "D", + "2024-10-14": "E", + "2024-10-15": "F", + "2024-10-16": "G", + "2024-10-17": "H", + }, + ), + ( + # 21 - No days (==All) 7 Interval + # Note: This test verifies expected behavior not the correct result, + # it *should* be equivalent to #22, but it is not using the daily_by_day logic + [1, 1, 1, 1, 1, 1, 1, 1, 1, 1], + "2024-09-30", + [], + 18, + CustomOnCallShift.FREQUENCY_DAILY, + 7, + { + "2024-09-30": "A", + "2024-10-07": "B", + "2024-10-14": "C", + "2024-10-15": "D", + "2024-10-16": "E", + "2024-10-17": "F", + }, + ), + ( + # 22 - All days 7 Interval + [1, 1, 1, 1, 1, 1, 1, 1, 1, 1], + "2024-09-30", + ["MO", "TU", "WE", "TH", "FR", "SA", "SU"], + 18, + CustomOnCallShift.FREQUENCY_DAILY, + 7, + { + "2024-09-30": "A", + "2024-10-01": "A", + "2024-10-02": "A", + "2024-10-03": "A", + "2024-10-04": "A", + "2024-10-05": "A", + "2024-10-06": "A", + "2024-10-07": "B", + "2024-10-08": "B", + "2024-10-09": "B", + "2024-10-10": "B", + "2024-10-11": "B", + "2024-10-12": "B", + "2024-10-13": "B", + "2024-10-14": "C", + "2024-10-15": "C", + "2024-10-16": "C", + "2024-10-17": "C", + }, + ), + ( + # 23 - Single User 1 day + [1], + "2024-08-01", + ["MO"], + 14, + CustomOnCallShift.FREQUENCY_DAILY, + 1, + {"2024-08-05": "A", "2024-08-12": "A"}, + ), +] diff --git a/engine/apps/schedules/tests/test_custom_on_call_shift.py b/engine/apps/schedules/tests/test_custom_on_call_shift.py index 6537b053..27129846 100644 --- a/engine/apps/schedules/tests/test_custom_on_call_shift.py +++ b/engine/apps/schedules/tests/test_custom_on_call_shift.py @@ -1,12 +1,17 @@ import datetime from calendar import monthrange +from collections import defaultdict from unittest.mock import patch +from zoneinfo import ZoneInfo +import icalendar import pytest from django.utils import timezone +from recurring_ical_events import UnfoldableCalendar from apps.schedules.ical_utils import list_users_to_notify_from_ical from apps.schedules.models import CustomOnCallShift, OnCallSchedule, OnCallScheduleCalendar, OnCallScheduleWeb +from apps.schedules.tests.custom_shift_test_cases import CUSTOM_SHIFT_TEST_CASES @pytest.mark.django_db @@ -1826,3 +1831,72 @@ def test_refresh_schedule(make_organization_and_user, make_schedule, make_on_cal assert mock_refresh_final.apply_async.called assert schedule.cached_ical_file_primary is not None assert schedule.cached_ical_file_overrides is not None + + +@pytest.mark.parametrize( + "users_per_group, shift_start, day_mask, total_days, frequency, interval, expected_result", CUSTOM_SHIFT_TEST_CASES +) +@pytest.mark.django_db +def test_ical_shift_generation( + make_organization, + make_user_for_organization, + make_schedule, + make_on_call_shift, + users_per_group, + shift_start, + day_mask, + total_days, + frequency, + interval, + expected_result, +): + organization = make_organization() + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_web_schedule", + ) + total_users = sum(users_per_group) + users = [make_user_for_organization(organization, username=chr(i + 64)) for i in range(1, total_users + 1)] + + start = datetime.datetime.strptime(shift_start, "%Y-%m-%d").replace(tzinfo=ZoneInfo("UTC")) + data = { + "start": start, + "rotation_start": start, + "until": start + timezone.timedelta(days=total_days), + "duration": timezone.timedelta(hours=12), + "frequency": frequency, + "by_day": day_mask, + "schedule": schedule, + "interval": interval, + "priority_level": 1, + "week_start": CustomOnCallShift.MONDAY, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + rolling_users = [] + start_user = 0 + for count in users_per_group: + end = start_user + count + rolling_users.append(users[start_user:end]) + start_user = end + on_call_shift.add_rolling_users(rolling_users) + + query_start = start + query_end = data["until"] + + calendar = icalendar.Calendar.from_ical(schedule._ical_file_primary) + events = UnfoldableCalendar(calendar).between(query_start, query_end) + + day_events = defaultdict(str) + for event in events: + event_start = event["DTSTART"].dt + event_summary = event["SUMMARY"].strip()[-1] + event_date = event_start.date().strftime("%Y-%m-%d") + day_events[event_date] += event_summary + + for k, v in day_events.items(): + day_events[k] = "".join(sorted(v)) + + assert day_events == expected_result