Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update schedule checks notification period and improve wording #5412

Merged
merged 2 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions engine/apps/api/serializers/schedule_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@

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
Expand Down Expand Up @@ -44,8 +49,8 @@ class Meta:
"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)
Expand Down Expand Up @@ -81,7 +86,7 @@ def validate(self, attrs):

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
Expand Down
8 changes: 6 additions & 2 deletions engine/apps/api/serializers/schedule_calendar.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

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
Expand Down Expand Up @@ -58,7 +62,7 @@ def update(self, instance, validated_data):
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
3 changes: 2 additions & 1 deletion engine/apps/api/serializers/schedule_ical.py
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -87,7 +88,7 @@ def update(self, instance, validated_data):

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
Expand Down
8 changes: 6 additions & 2 deletions engine/apps/api/serializers/schedule_web.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -41,7 +45,7 @@ def update(self, instance, validated_data):
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
2 changes: 1 addition & 1 deletion engine/apps/api/views/schedule.py
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ def type_options(self, request):
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,))
Expand Down
1 change: 1 addition & 0 deletions engine/apps/schedules/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,6 @@

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"
13 changes: 7 additions & 6 deletions engine/apps/schedules/models/on_call_schedule.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
ICAL_SUMMARY,
ICAL_UID,
PREFETCHED_SHIFT_SWAPS,
SCHEDULE_CHECK_NEXT_DAYS,
)
from apps.schedules.ical_utils import (
EmptyShifts,
Expand Down Expand Up @@ -293,9 +294,9 @@ def get_prev_and_current_ical_files(self):
(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(
Expand All @@ -313,14 +314,14 @@ def check_gaps_and_empty_shifts_for_next_week(self) -> None:
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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down
25 changes: 13 additions & 12 deletions engine/apps/schedules/tests/test_check_gaps_and_empty_shifts.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
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


Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
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

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Loading