From f3f7c17f8b7b4245c57c6c4b3fa87888fa6f6768 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 2 Jan 2025 10:50:09 -0300 Subject: [PATCH] fix: update missing users / empty shifts check (#5322) Related to https://github.com/grafana/oncall-private/issues/2950 - Represent missing users in schedule events (so they are displayed in the web UI) - Fix schedule checks for gaps/empty shifts so they send notifications --- engine/apps/schedules/ical_utils.py | 13 +++++ .../schedules/models/custom_on_call_shift.py | 23 +++++--- engine/apps/schedules/tasks/__init__.py | 4 -- .../notify_about_empty_shifts_in_schedule.py | 19 ++----- .../tasks/notify_about_gaps_in_schedule.py | 15 +----- ...t_notify_about_empty_shifts_in_schedule.py | 46 +++++++++++++++- .../test_notify_about_gaps_in_schedule.py | 46 +++++++++++++++- .../schedules/tests/test_on_call_schedule.py | 52 +++++++++++++++++++ engine/settings/celery_task_routes.py | 4 -- 9 files changed, 175 insertions(+), 47 deletions(-) diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 8678232d..788f6865 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -54,6 +54,19 @@ logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) +class MissingUser: + """Represent a missing user in a rolling users shift.""" + + DISPLAY_NAME = "(missing)" + + def __init__(self, pk): + self.pk = pk + + @property + def username(self): + return self.DISPLAY_NAME + + EmptyShift = namedtuple( "EmptyShift", ["start", "end", "summary", "description", "attendee", "all_day", "calendar_type", "calendar_tz", "shift_pk"], diff --git a/engine/apps/schedules/models/custom_on_call_shift.py b/engine/apps/schedules/models/custom_on_call_shift.py index 812b0947..8b1dda64 100644 --- a/engine/apps/schedules/models/custom_on_call_shift.py +++ b/engine/apps/schedules/models/custom_on_call_shift.py @@ -18,6 +18,7 @@ from django.utils import timezone from django.utils.functional import cached_property from icalendar.cal import Event +from apps.schedules.ical_utils import MissingUser from apps.schedules.tasks import ( check_gaps_and_empty_shifts_in_schedule, drop_cached_ical_task, @@ -645,10 +646,6 @@ class CustomOnCallShift(models.Model): all_users_pks = set() users_queue = [] if self.rolling_users is not None: - # get all users pks from rolling_users field - for users_dict in self.rolling_users: - all_users_pks.update(users_dict.keys()) - users = User.objects.filter(pk__in=all_users_pks) # generate users_queue list with user objects if self.start_rotation_from_user_index is not None: rolling_users = ( @@ -657,10 +654,22 @@ class CustomOnCallShift(models.Model): ) else: rolling_users = self.rolling_users + + # get all users pks from rolling_users field + for users_dict in self.rolling_users: + all_users_pks.update(users_dict.keys()) + users = User.objects.filter(pk__in=all_users_pks) + users_by_id = {user.pk: user for user in users} for users_dict in rolling_users: - users_list = list(users.filter(pk__in=users_dict.keys())) - if users_list: - users_queue.append(users_list) + users_list = [] + for user_pk in users_dict.keys(): + try: + user_pk = int(user_pk) + users_list.append(users_by_id.get(user_pk, MissingUser(user_pk))) + except ValueError: + users_list.append(MissingUser(user_pk)) + users_queue.append(users_list) + return users_queue def add_rolling_users(self, rolling_users_list): diff --git a/engine/apps/schedules/tasks/__init__.py b/engine/apps/schedules/tasks/__init__.py index 5e0078dd..a5617a92 100644 --- a/engine/apps/schedules/tasks/__init__.py +++ b/engine/apps/schedules/tasks/__init__.py @@ -1,17 +1,13 @@ from .check_gaps_and_empty_shifts import check_gaps_and_empty_shifts_in_schedule # noqa: F401 from .drop_cached_ical import drop_cached_ical_for_custom_events_for_organization, drop_cached_ical_task # noqa: F401 from .notify_about_empty_shifts_in_schedule import ( # noqa: F401 - check_empty_shifts_in_schedule, notify_about_empty_shifts_in_schedule_task, schedule_notify_about_empty_shifts_in_schedule, - start_check_empty_shifts_in_schedule, start_notify_about_empty_shifts_in_schedule, ) from .notify_about_gaps_in_schedule import ( # noqa: F401 - check_gaps_in_schedule, notify_about_gaps_in_schedule_task, schedule_notify_about_gaps_in_schedule, - start_check_gaps_in_schedule, start_notify_about_gaps_in_schedule, ) from .refresh_ical_files import ( # noqa: F401 diff --git a/engine/apps/schedules/tasks/notify_about_empty_shifts_in_schedule.py b/engine/apps/schedules/tasks/notify_about_empty_shifts_in_schedule.py index afdf789c..ebe68eda 100644 --- a/engine/apps/schedules/tasks/notify_about_empty_shifts_in_schedule.py +++ b/engine/apps/schedules/tasks/notify_about_empty_shifts_in_schedule.py @@ -1,6 +1,7 @@ import pytz from celery.utils.log import get_task_logger from django.core.cache import cache +from django.db.models import Q from django.utils import timezone from apps.slack.utils import format_datetime_to_slack_with_time, post_message_to_channel @@ -10,28 +11,16 @@ from common.utils import trim_if_needed task_logger = get_task_logger(__name__) -# deprecated # todo: delete this task from here and from task routes after the next release -@shared_dedicated_queue_retry_task() -def start_check_empty_shifts_in_schedule(): - return - - -# deprecated # todo: delete this task from here and from task routes after the next release -@shared_dedicated_queue_retry_task() -def check_empty_shifts_in_schedule(schedule_pk): - return - - @shared_dedicated_queue_retry_task() def start_notify_about_empty_shifts_in_schedule(): - from apps.schedules.models import OnCallScheduleICal + from apps.schedules.models import OnCallSchedule task_logger.info("Start start_notify_about_empty_shifts_in_schedule") today = timezone.now().date() week_ago = today - timezone.timedelta(days=7) - schedules = OnCallScheduleICal.objects.filter( - empty_shifts_report_sent_at__lte=week_ago, + schedules = OnCallSchedule.objects.filter( + Q(empty_shifts_report_sent_at__lte=week_ago) | Q(empty_shifts_report_sent_at__isnull=True), slack_channel__isnull=False, organization__deleted_at__isnull=True, ) diff --git a/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py b/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py index 5047943e..7a8b2c67 100644 --- a/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py +++ b/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py @@ -1,6 +1,7 @@ import pytz from celery.utils.log import get_task_logger from django.core.cache import cache +from django.db.models import Q from django.utils import timezone from apps.slack.utils import format_datetime_to_slack_with_time, post_message_to_channel @@ -9,18 +10,6 @@ from common.custom_celery_tasks import shared_dedicated_queue_retry_task task_logger = get_task_logger(__name__) -# deprecated # todo: delete this task from here and from task routes after the next release -@shared_dedicated_queue_retry_task() -def start_check_gaps_in_schedule(): - return - - -# deprecated # todo: delete this task from here and from task routes after the next release -@shared_dedicated_queue_retry_task() -def check_gaps_in_schedule(schedule_pk): - return - - @shared_dedicated_queue_retry_task() def start_notify_about_gaps_in_schedule(): from apps.schedules.models import OnCallSchedule @@ -30,7 +19,7 @@ def start_notify_about_gaps_in_schedule(): today = timezone.now().date() week_ago = today - timezone.timedelta(days=7) schedules = OnCallSchedule.objects.filter( - gaps_report_sent_at__lte=week_ago, + Q(gaps_report_sent_at__lte=week_ago) | Q(gaps_report_sent_at__isnull=True), slack_channel__isnull=False, organization__deleted_at__isnull=True, ) diff --git a/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py b/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py index 2c6bb091..1790fd7c 100644 --- a/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py +++ b/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py @@ -5,8 +5,8 @@ import pytest from django.utils import timezone from apps.api.permissions import LegacyAccessControlRole -from apps.schedules.models import CustomOnCallShift, OnCallScheduleWeb -from apps.schedules.tasks import notify_about_empty_shifts_in_schedule_task +from apps.schedules.models import CustomOnCallShift, OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb +from apps.schedules.tasks import notify_about_empty_shifts_in_schedule_task, start_notify_about_empty_shifts_in_schedule @pytest.mark.django_db @@ -174,3 +174,45 @@ def test_empty_non_empty_shifts_trigger_notification( schedule.refresh_from_db() assert empty_shifts_report_sent_at != schedule.empty_shifts_report_sent_at assert schedule.has_empty_shifts + + +@pytest.mark.parametrize( + "schedule_class", + [OnCallScheduleWeb, OnCallScheduleICal, OnCallScheduleCalendar], +) +@pytest.mark.parametrize( + "report_sent_days_ago,expected_call", + [(8, True), (6, False), (None, True)], +) +@pytest.mark.django_db +def test_start_notify_about_empty_shifts( + make_slack_team_identity, + make_slack_channel, + make_organization, + make_schedule, + schedule_class, + report_sent_days_ago, + expected_call, +): + slack_team_identity = make_slack_team_identity() + slack_channel = make_slack_channel(slack_team_identity) + organization = make_organization(slack_team_identity=slack_team_identity) + + sent = timezone.now() - datetime.timedelta(days=report_sent_days_ago) if report_sent_days_ago else None + schedule = make_schedule( + organization, + schedule_class=schedule_class, + name="test_schedule", + slack_channel=slack_channel, + empty_shifts_report_sent_at=sent, + ) + + with patch( + "apps.schedules.tasks.notify_about_empty_shifts_in_schedule.notify_about_empty_shifts_in_schedule_task.apply_async" + ) as mock_notify: + start_notify_about_empty_shifts_in_schedule() + + if expected_call: + mock_notify.assert_called_once_with((schedule.pk,)) + else: + mock_notify.assert_not_called() diff --git a/engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py b/engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py index d775c77b..5e35cf06 100644 --- a/engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py +++ b/engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py @@ -4,8 +4,8 @@ from unittest.mock import patch import pytest from django.utils import timezone -from apps.schedules.models import CustomOnCallShift, OnCallScheduleWeb -from apps.schedules.tasks import notify_about_gaps_in_schedule_task +from apps.schedules.models import CustomOnCallShift, OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb +from apps.schedules.tasks import notify_about_gaps_in_schedule_task, start_notify_about_gaps_in_schedule @pytest.mark.django_db @@ -286,3 +286,45 @@ def test_gaps_later_than_7_days_no_triggering_notification( schedule.refresh_from_db() assert gaps_report_sent_at != schedule.gaps_report_sent_at assert schedule.has_gaps is False + + +@pytest.mark.parametrize( + "schedule_class", + [OnCallScheduleWeb, OnCallScheduleICal, OnCallScheduleCalendar], +) +@pytest.mark.parametrize( + "report_sent_days_ago,expected_call", + [(8, True), (6, False), (None, True)], +) +@pytest.mark.django_db +def test_start_notify_about_gaps( + make_slack_team_identity, + make_slack_channel, + make_organization, + make_schedule, + schedule_class, + report_sent_days_ago, + expected_call, +): + slack_team_identity = make_slack_team_identity() + slack_channel = make_slack_channel(slack_team_identity) + organization = make_organization(slack_team_identity=slack_team_identity) + + sent = timezone.now() - datetime.timedelta(days=report_sent_days_ago) if report_sent_days_ago else None + schedule = make_schedule( + organization, + schedule_class=schedule_class, + name="test_schedule", + slack_channel=slack_channel, + gaps_report_sent_at=sent, + ) + + with patch( + "apps.schedules.tasks.notify_about_gaps_in_schedule.notify_about_gaps_in_schedule_task.apply_async" + ) as mock_notify: + start_notify_about_gaps_in_schedule() + + if expected_call: + mock_notify.assert_called_once_with((schedule.pk,)) + else: + mock_notify.assert_not_called() diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index 71a029b4..d9a0b26c 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -18,6 +18,7 @@ from apps.schedules.constants import ( ICAL_STATUS_CANCELLED, ICAL_SUMMARY, ) +from apps.schedules.ical_utils import MissingUser from apps.schedules.models import ( CustomOnCallShift, OnCallSchedule, @@ -358,6 +359,57 @@ def test_filter_events_include_empty(make_organization, make_user_for_organizati assert events == expected +@pytest.mark.django_db +def test_filter_events_include_empty_if_deleted( + make_organization, make_user_for_organization, make_schedule, make_on_call_shift +): + organization = make_organization() + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_web_schedule", + ) + user = make_user_for_organization(organization) + now = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + start_date = now - timezone.timedelta(days=7) + + data = { + "start": start_date + timezone.timedelta(hours=10), + "rotation_start": start_date + timezone.timedelta(hours=10), + "duration": timezone.timedelta(hours=8), + "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]]) + + # user is deleted, shift data still exists but the shift is empty + user.delete() + + 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, + "start": on_call_shift.start, + "end": on_call_shift.start + on_call_shift.duration, + "all_day": False, + "is_override": False, + "is_empty": True, + "is_gap": False, + "priority_level": on_call_shift.priority_level, + "missing_users": [MissingUser.DISPLAY_NAME], + "users": [], + "shift": {"pk": on_call_shift.public_primary_key}, + "source": "api", + } + ] + assert events == expected + + @pytest.mark.django_db def test_filter_events_ical_all_day(make_organization, make_user_for_organization, make_schedule, get_ical): calendar = get_ical("calendar_with_all_day_event.ics") diff --git a/engine/settings/celery_task_routes.py b/engine/settings/celery_task_routes.py index 37861c43..fff08a2a 100644 --- a/engine/settings/celery_task_routes.py +++ b/engine/settings/celery_task_routes.py @@ -33,13 +33,9 @@ CELERY_TASK_ROUTES = { "apps.schedules.tasks.refresh_ical_files.refresh_ical_final_schedule": {"queue": "default"}, "apps.schedules.tasks.refresh_ical_files.start_refresh_ical_final_schedules": {"queue": "default"}, "apps.schedules.tasks.check_gaps_and_empty_shifts.check_gaps_and_empty_shifts_in_schedule": {"queue": "default"}, - "apps.schedules.tasks.notify_about_gaps_in_schedule.check_empty_shifts_in_schedule": {"queue": "default"}, "apps.schedules.tasks.notify_about_gaps_in_schedule.start_notify_about_gaps_in_schedule": {"queue": "default"}, - "apps.schedules.tasks.notify_about_gaps_in_schedule.check_gaps_in_schedule": {"queue": "default"}, "apps.schedules.tasks.notify_about_gaps_in_schedule.notify_about_gaps_in_schedule_task": {"queue": "default"}, "apps.schedules.tasks.notify_about_gaps_in_schedule.schedule_notify_about_gaps_in_schedule": {"queue": "default"}, - "apps.schedules.tasks.notify_about_gaps_in_schedule.start_check_empty_shifts_in_schedule": {"queue": "default"}, - "apps.schedules.tasks.notify_about_gaps_in_schedule.start_check_gaps_in_schedule": {"queue": "default"}, "apps.schedules.tasks.notify_about_gaps_in_schedule.start_notify_about_empty_shifts_in_schedule": { "queue": "default" },