From 4c92826c2625e24e3e367ca0af39f249ed1298ee Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 16 Jan 2025 09:19:16 -0300 Subject: [PATCH] chore: update schedule checks notification period and improve wording (#5412) Related to https://github.com/grafana/oncall-private/issues/2994 - Extend gaps/empty shift checks to consider 30 days (customizable via param, eventually make it customizable per schedule?); ie. every week (per beat schedule), check the schedule next 30 days - Trigger checks via async task on schedule API updates (instead of a sync call) - Update notifications wording / link to schedule --- engine/apps/api/serializers/schedule_base.py | 13 +++++++--- .../apps/api/serializers/schedule_calendar.py | 8 ++++-- engine/apps/api/serializers/schedule_ical.py | 3 ++- engine/apps/api/serializers/schedule_web.py | 8 ++++-- engine/apps/api/views/schedule.py | 2 +- engine/apps/schedules/constants.py | 1 + .../apps/schedules/models/on_call_schedule.py | 13 +++++----- .../tasks/check_gaps_and_empty_shifts.py | 2 +- .../notify_about_empty_shifts_in_schedule.py | 9 +++---- .../tasks/notify_about_gaps_in_schedule.py | 9 +++---- .../tests/test_check_gaps_and_empty_shifts.py | 25 ++++++++++--------- .../test_notify_about_gaps_in_schedule.py | 7 +++--- 12 files changed, 58 insertions(+), 42 deletions(-) diff --git a/engine/apps/api/serializers/schedule_base.py b/engine/apps/api/serializers/schedule_base.py index 242d6b1a..441180d8 100644 --- a/engine/apps/api/serializers/schedule_base.py +++ b/engine/apps/api/serializers/schedule_base.py @@ -2,8 +2,13 @@ from rest_framework import serializers from apps.api.serializers.slack_channel import SlackChannelSerializer from apps.api.serializers.user_group import UserGroupSerializer +from apps.schedules.constants import SCHEDULE_CHECK_NEXT_DAYS from apps.schedules.models import OnCallSchedule -from apps.schedules.tasks import schedule_notify_about_empty_shifts_in_schedule, schedule_notify_about_gaps_in_schedule +from apps.schedules.tasks import ( + check_gaps_and_empty_shifts_in_schedule, + schedule_notify_about_empty_shifts_in_schedule, + schedule_notify_about_gaps_in_schedule, +) from common.api_helpers.custom_fields import TeamPrimaryKeyRelatedField from common.api_helpers.mixins import EagerLoadingMixin from common.api_helpers.utils import CurrentOrganizationDefault @@ -44,8 +49,8 @@ class ScheduleBaseSerializer(EagerLoadingMixin, serializers.ModelSerializer): "Cannot update the user group, make sure to grant user group modification rights to " "non-admin users in Slack workspace settings" ) - SCHEDULE_HAS_GAPS_WARNING = "Schedule has unassigned time periods during next 7 days" - SCHEDULE_HAS_EMPTY_SHIFTS_WARNING = "Schedule has empty shifts during next 7 days" + SCHEDULE_HAS_GAPS_WARNING = f"Schedule has unassigned time periods during next {SCHEDULE_CHECK_NEXT_DAYS} days" + SCHEDULE_HAS_EMPTY_SHIFTS_WARNING = f"Schedule has empty shifts during next {SCHEDULE_CHECK_NEXT_DAYS} days" def get_warnings(self, obj): can_update_user_groups = self.context.get("can_update_user_groups", False) @@ -81,7 +86,7 @@ class ScheduleBaseSerializer(EagerLoadingMixin, serializers.ModelSerializer): def create(self, validated_data): created_schedule = super().create(validated_data) - created_schedule.check_gaps_and_empty_shifts_for_next_week() + check_gaps_and_empty_shifts_in_schedule.apply_async((created_schedule.pk,)) schedule_notify_about_empty_shifts_in_schedule.apply_async((created_schedule.pk,)) schedule_notify_about_gaps_in_schedule.apply_async((created_schedule.pk,)) return created_schedule diff --git a/engine/apps/api/serializers/schedule_calendar.py b/engine/apps/api/serializers/schedule_calendar.py index d6c35ea4..69f962c1 100644 --- a/engine/apps/api/serializers/schedule_calendar.py +++ b/engine/apps/api/serializers/schedule_calendar.py @@ -2,7 +2,11 @@ from rest_framework import serializers from apps.api.serializers.schedule_base import ScheduleBaseSerializer from apps.schedules.models import OnCallScheduleCalendar -from apps.schedules.tasks import schedule_notify_about_empty_shifts_in_schedule, schedule_notify_about_gaps_in_schedule +from apps.schedules.tasks import ( + check_gaps_and_empty_shifts_in_schedule, + schedule_notify_about_empty_shifts_in_schedule, + schedule_notify_about_gaps_in_schedule, +) from apps.slack.models import SlackChannel, SlackUserGroup from common.api_helpers.custom_fields import OrganizationFilteredPrimaryKeyRelatedField, TimeZoneField from common.api_helpers.utils import validate_ical_url @@ -58,7 +62,7 @@ class ScheduleCalendarCreateSerializer(ScheduleCalendarSerializer): or old_enable_web_overrides != updated_enable_web_overrides ): updated_schedule.drop_cached_ical() - updated_schedule.check_gaps_and_empty_shifts_for_next_week() + check_gaps_and_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_gaps_in_schedule.apply_async((instance.pk,)) return updated_schedule diff --git a/engine/apps/api/serializers/schedule_ical.py b/engine/apps/api/serializers/schedule_ical.py index 4fb1b9f2..21315ece 100644 --- a/engine/apps/api/serializers/schedule_ical.py +++ b/engine/apps/api/serializers/schedule_ical.py @@ -1,6 +1,7 @@ from apps.api.serializers.schedule_base import ScheduleBaseSerializer from apps.schedules.models import OnCallScheduleICal from apps.schedules.tasks import ( + check_gaps_and_empty_shifts_in_schedule, refresh_ical_final_schedule, schedule_notify_about_empty_shifts_in_schedule, schedule_notify_about_gaps_in_schedule, @@ -87,7 +88,7 @@ class ScheduleICalUpdateSerializer(ScheduleICalCreateSerializer): if old_ical_url_primary != updated_ical_url_primary or old_ical_url_overrides != updated_ical_url_overrides: updated_schedule.drop_cached_ical() - updated_schedule.check_gaps_and_empty_shifts_for_next_week() + check_gaps_and_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_gaps_in_schedule.apply_async((instance.pk,)) # for iCal-based schedules we need to refresh final schedule information diff --git a/engine/apps/api/serializers/schedule_web.py b/engine/apps/api/serializers/schedule_web.py index 3a503041..e8aea128 100644 --- a/engine/apps/api/serializers/schedule_web.py +++ b/engine/apps/api/serializers/schedule_web.py @@ -1,6 +1,10 @@ from apps.api.serializers.schedule_base import ScheduleBaseSerializer from apps.schedules.models import OnCallScheduleWeb -from apps.schedules.tasks import schedule_notify_about_empty_shifts_in_schedule, schedule_notify_about_gaps_in_schedule +from apps.schedules.tasks import ( + check_gaps_and_empty_shifts_in_schedule, + schedule_notify_about_empty_shifts_in_schedule, + schedule_notify_about_gaps_in_schedule, +) from apps.slack.models import SlackChannel, SlackUserGroup from common.api_helpers.custom_fields import OrganizationFilteredPrimaryKeyRelatedField, TimeZoneField @@ -41,7 +45,7 @@ class ScheduleWebCreateSerializer(ScheduleWebSerializer): updated_time_zone = updated_schedule.time_zone if old_time_zone != updated_time_zone: updated_schedule.drop_cached_ical() - updated_schedule.check_gaps_and_empty_shifts_for_next_week() + check_gaps_and_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_empty_shifts_in_schedule.apply_async((instance.pk,)) schedule_notify_about_gaps_in_schedule.apply_async((instance.pk,)) return updated_schedule diff --git a/engine/apps/api/views/schedule.py b/engine/apps/api/views/schedule.py index e30aa8cb..2d816211 100644 --- a/engine/apps/api/views/schedule.py +++ b/engine/apps/api/views/schedule.py @@ -481,7 +481,7 @@ class ScheduleView( def reload_ical(self, request, pk): schedule = self.get_object(annotate=False) schedule.drop_cached_ical() - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() if schedule.user_group is not None: update_slack_user_group_for_schedules.apply_async((schedule.user_group.pk,)) diff --git a/engine/apps/schedules/constants.py b/engine/apps/schedules/constants.py index 3f8f556e..26192c58 100644 --- a/engine/apps/schedules/constants.py +++ b/engine/apps/schedules/constants.py @@ -29,5 +29,6 @@ EXPORT_WINDOW_DAYS_BEFORE = 15 SCHEDULE_ONCALL_CACHE_KEY_PREFIX = "schedule_oncall_users_" SCHEDULE_ONCALL_CACHE_TTL = 15 * 60 # 15 minutes in seconds +SCHEDULE_CHECK_NEXT_DAYS = 30 PREFETCHED_SHIFT_SWAPS = "prefetched_shift_swaps" diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index e57cf4bc..691d172b 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -33,6 +33,7 @@ from apps.schedules.constants import ( ICAL_SUMMARY, ICAL_UID, PREFETCHED_SHIFT_SWAPS, + SCHEDULE_CHECK_NEXT_DAYS, ) from apps.schedules.ical_utils import ( EmptyShifts, @@ -293,9 +294,9 @@ class OnCallSchedule(PolymorphicModel): (self.prev_ical_file_overrides, self.cached_ical_file_overrides), ] - def check_gaps_and_empty_shifts_for_next_week(self) -> None: + def check_gaps_and_empty_shifts_for_next_days(self, days=SCHEDULE_CHECK_NEXT_DAYS) -> None: datetime_start = timezone.now() - datetime_end = datetime_start + datetime.timedelta(days=7) + datetime_end = datetime_start + datetime.timedelta(days=days) # get empty shifts from all events and gaps from final events events = self.filter_events( @@ -313,14 +314,14 @@ class OnCallSchedule(PolymorphicModel): self.has_empty_shifts = has_empty_shifts self.save(update_fields=["has_gaps", "has_empty_shifts"]) - def get_gaps_for_next_week(self) -> ScheduleEvents: + def get_gaps_for_next_days(self, days=SCHEDULE_CHECK_NEXT_DAYS) -> ScheduleEvents: today = timezone.now() - events = self.final_events(today, today + datetime.timedelta(days=7)) + events = self.final_events(today, today + datetime.timedelta(days=days)) return [event for event in events if event["is_gap"]] - def get_empty_shifts_for_next_week(self) -> EmptyShifts: + def get_empty_shifts_for_next_days(self, days=SCHEDULE_CHECK_NEXT_DAYS) -> EmptyShifts: today = timezone.now().date() - return list_of_empty_shifts_in_schedule(self, today, today + datetime.timedelta(days=7)) + return list_of_empty_shifts_in_schedule(self, today, today + datetime.timedelta(days=days)) def drop_cached_ical(self): self._drop_primary_ical_file() diff --git a/engine/apps/schedules/tasks/check_gaps_and_empty_shifts.py b/engine/apps/schedules/tasks/check_gaps_and_empty_shifts.py index 0a05471a..47042029 100644 --- a/engine/apps/schedules/tasks/check_gaps_and_empty_shifts.py +++ b/engine/apps/schedules/tasks/check_gaps_and_empty_shifts.py @@ -19,5 +19,5 @@ def check_gaps_and_empty_shifts_in_schedule(schedule_pk): task_logger.info(f"Tried to check_gaps_and_empty_shifts_in_schedule for non-existing schedule {schedule_pk}") return - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() task_logger.info(f"Finish check_gaps_and_empty_shifts_in_schedule {schedule_pk}") 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 ebe68eda..aba334b4 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 @@ -48,15 +48,15 @@ def notify_about_empty_shifts_in_schedule_task(schedule_pk): task_logger.info(f"Tried to notify_about_empty_shifts_in_schedule_task for non-existing schedule {schedule_pk}") return - empty_shifts = schedule.get_empty_shifts_for_next_week() + empty_shifts = schedule.get_empty_shifts_for_next_days() schedule.empty_shifts_report_sent_at = timezone.now().date() if len(empty_shifts) != 0: schedule.has_empty_shifts = True text = ( - f'Tried to parse schedule *"{schedule.name}"* and found events without associated users.\n' - f"To ensure you don't miss any notifications, use a Grafana username as the event name in the calendar. " - f"The user should have Editor or Admin access.\n\n" + f"Reviewing *{schedule.slack_url}* on-call schedule found events without valid associated users.\n" + f"To ensure you don't miss any notifications, make sure user exists (or you provided a valid Grafana username). " + f"The user should have the right permissions, or be an Editor or Admin.\n\n" ) for idx, empty_shift in enumerate(empty_shifts): start_timestamp = empty_shift.start.astimezone(pytz.UTC).timestamp() @@ -80,7 +80,6 @@ def notify_about_empty_shifts_in_schedule_task(schedule_pk): text += '*All-day* event in "UTC" TZ\n' else: text += f"From {format_datetime_to_slack_with_time(start_timestamp)} to {format_datetime_to_slack_with_time(end_timestamp)} (your TZ)\n" - text += f"_From {OnCallSchedule.CALENDAR_TYPE_VERBAL[empty_shift.calendar_type]} calendar_\n" if idx != len(empty_shifts) - 1: text += "\n\n" post_message_to_channel(schedule.organization, schedule.slack_channel_slack_id, text) 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 7a8b2c67..8bc00135 100644 --- a/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py +++ b/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py @@ -48,13 +48,13 @@ def notify_about_gaps_in_schedule_task(schedule_pk): task_logger.info(f"Tried to notify_about_gaps_in_schedule_task for non-existing schedule {schedule_pk}") return - gaps = schedule.get_gaps_for_next_week() + gaps = schedule.get_gaps_for_next_days() schedule.gaps_report_sent_at = timezone.now().date() if len(gaps) != 0: schedule.has_gaps = True - text = f"There are time periods that are unassigned in *{schedule.name}* on-call schedule.\n" - for idx, gap in enumerate(gaps): + text = f"There are time periods that are unassigned in *{schedule.slack_url}* on-call schedule.\n" + for gap in gaps: if gap["start"]: start_verbal = format_datetime_to_slack_with_time(gap["start"].astimezone(pytz.UTC).timestamp()) else: @@ -64,8 +64,7 @@ def notify_about_gaps_in_schedule_task(schedule_pk): else: end_verbal = "..." text += f"From {start_verbal} to {end_verbal} (your TZ)\n" - if idx != len(gaps) - 1: - text += "\n\n" + text += "\n\n" post_message_to_channel(schedule.organization, schedule.slack_channel_slack_id, text) else: schedule.has_gaps = False diff --git a/engine/apps/schedules/tests/test_check_gaps_and_empty_shifts.py b/engine/apps/schedules/tests/test_check_gaps_and_empty_shifts.py index 63ca428a..b5461771 100644 --- a/engine/apps/schedules/tests/test_check_gaps_and_empty_shifts.py +++ b/engine/apps/schedules/tests/test_check_gaps_and_empty_shifts.py @@ -4,6 +4,7 @@ import pytest from django.utils import timezone from apps.api.permissions import LegacyAccessControlRole +from apps.schedules.constants import SCHEDULE_CHECK_NEXT_DAYS from apps.schedules.models import CustomOnCallShift, OnCallScheduleWeb @@ -34,7 +35,7 @@ def test_no_empty_shifts_no_gaps( ) on_call_shift.add_rolling_users([[user1]]) schedule.refresh_ical_file() - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() schedule.refresh_from_db() assert schedule.has_gaps is False @@ -73,7 +74,7 @@ def test_no_empty_shifts_but_gaps_now( assert schedule.has_gaps is False assert schedule.has_empty_shifts is False - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() schedule.refresh_from_db() assert schedule.has_gaps is True @@ -111,7 +112,7 @@ def test_empty_shifts_no_gaps( assert schedule.has_gaps is False assert schedule.has_empty_shifts is False - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() schedule.refresh_from_db() assert schedule.has_gaps is False @@ -150,7 +151,7 @@ def test_empty_shifts_and_gaps( assert schedule.has_gaps is False assert schedule.has_empty_shifts is False - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() schedule.refresh_from_db() assert schedule.has_gaps is True @@ -206,7 +207,7 @@ def test_empty_shifts_and_gaps_in_the_past( assert schedule.has_gaps is False assert schedule.has_empty_shifts is False - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() schedule.refresh_from_db() assert schedule.has_gaps is False @@ -225,9 +226,9 @@ def test_empty_shifts_and_gaps_in_the_future( user2 = make_user(organization=organization, username="user2", role=LegacyAccessControlRole.ADMIN) schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb, name="test_schedule") - # empty shift with gaps starts in 7 days 1 min + # empty shift with gaps starts in SCHEDULE_CHECK_NEXT_DAYS days 1 min now = timezone.now().replace(microsecond=0) - start_date = now + datetime.timedelta(days=7, minutes=1) + start_date = now + datetime.timedelta(days=SCHEDULE_CHECK_NEXT_DAYS, minutes=1) data = { "start": start_date, "rotation_start": start_date, @@ -241,9 +242,9 @@ def test_empty_shifts_and_gaps_in_the_future( organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data ) on_call_shift.add_rolling_users([[user1]]) - # normal shift ends in 7 days 1 min - start_date2 = now - datetime.timedelta(days=7, minutes=1) - until = now + datetime.timedelta(days=7, minutes=1) + # normal shift ends in SCHEDULE_CHECK_NEXT_DAYS days 1 min + start_date2 = now - datetime.timedelta(days=SCHEDULE_CHECK_NEXT_DAYS, minutes=1) + until = now + datetime.timedelta(days=SCHEDULE_CHECK_NEXT_DAYS, minutes=1) data2 = { "start": start_date2, "rotation_start": start_date2, @@ -262,8 +263,8 @@ def test_empty_shifts_and_gaps_in_the_future( assert schedule.has_gaps is False assert schedule.has_empty_shifts is False - schedule.check_gaps_and_empty_shifts_for_next_week() + schedule.check_gaps_and_empty_shifts_for_next_days() schedule.refresh_from_db() - # no gaps and empty shifts in the next 7 days + # no gaps and empty shifts in the next SCHEDULE_CHECK_NEXT_DAYS days assert schedule.has_gaps is False assert schedule.has_empty_shifts is False 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 5e35cf06..6f2f827d 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,6 +4,7 @@ from unittest.mock import patch import pytest from django.utils import timezone +from apps.schedules.constants import SCHEDULE_CHECK_NEXT_DAYS 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 @@ -236,7 +237,7 @@ def test_gaps_near_future_trigger_notification( @pytest.mark.django_db -def test_gaps_later_than_7_days_no_triggering_notification( +def test_gaps_later_than_days_no_triggering_notification( make_slack_team_identity, make_slack_channel, make_organization, @@ -259,8 +260,8 @@ def test_gaps_later_than_7_days_no_triggering_notification( prev_ical_file_overrides=None, cached_ical_file_overrides=None, ) - start_date = now - datetime.timedelta(days=7, minutes=1) - until_date = now + datetime.timedelta(days=8) + start_date = now - datetime.timedelta(days=SCHEDULE_CHECK_NEXT_DAYS, minutes=1) + until_date = now + datetime.timedelta(days=SCHEDULE_CHECK_NEXT_DAYS + 1) data = { "start": start_date, "rotation_start": start_date,