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

Refactor gaps and empty shift checks #3785

Merged
merged 6 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
3 changes: 1 addition & 2 deletions engine/apps/api/serializers/schedule_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,8 @@ def validate(self, attrs):

def create(self, validated_data):
created_schedule = super().create(validated_data)
created_schedule.check_empty_shifts_for_next_week()
created_schedule.check_gaps_and_empty_shifts_for_next_week()
schedule_notify_about_empty_shifts_in_schedule.apply_async((created_schedule.pk,))
created_schedule.check_gaps_for_next_week()
schedule_notify_about_gaps_in_schedule.apply_async((created_schedule.pk,))
return created_schedule

Expand Down
3 changes: 1 addition & 2 deletions engine/apps/api/serializers/schedule_calendar.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@ def update(self, instance, validated_data):
or old_enable_web_overrides != updated_enable_web_overrides
):
updated_schedule.drop_cached_ical()
updated_schedule.check_empty_shifts_for_next_week()
updated_schedule.check_gaps_for_next_week()
updated_schedule.check_gaps_and_empty_shifts_for_next_week()
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: 1 addition & 2 deletions engine/apps/api/serializers/schedule_ical.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,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_empty_shifts_for_next_week()
updated_schedule.check_gaps_for_next_week()
updated_schedule.check_gaps_and_empty_shifts_for_next_week()
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
3 changes: 1 addition & 2 deletions engine/apps/api/serializers/schedule_web.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,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_empty_shifts_for_next_week()
updated_schedule.check_gaps_for_next_week()
updated_schedule.check_gaps_and_empty_shifts_for_next_week()
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: 1 addition & 2 deletions engine/apps/api/views/schedule.py
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,7 @@ def type_options(self, request):
def reload_ical(self, request, pk):
schedule = self.get_object(annotate=False)
schedule.drop_cached_ical()
schedule.check_empty_shifts_for_next_week()
schedule.check_gaps_for_next_week()
schedule.check_gaps_and_empty_shifts_for_next_week()

if schedule.user_group is not None:
update_slack_user_group_for_schedules.apply_async((schedule.user_group.pk,))
Expand Down
2 changes: 2 additions & 0 deletions engine/apps/schedules/models/custom_on_call_shift.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from icalendar.cal import Event

from apps.schedules.tasks import (
check_gaps_and_empty_shifts_in_schedule,
drop_cached_ical_task,
refresh_ical_final_schedule,
schedule_notify_about_empty_shifts_in_schedule,
Expand Down Expand Up @@ -692,6 +693,7 @@ def refresh_schedule(self):
schedule = self.schedule.get_real_instance()
schedule.refresh_ical_file()
refresh_ical_final_schedule.apply_async((schedule.pk,))
check_gaps_and_empty_shifts_in_schedule.apply_async((schedule.pk,))

def start_drop_ical_and_check_schedule_tasks(self, schedule):
drop_cached_ical_task.apply_async((schedule.pk,))
Expand Down
36 changes: 24 additions & 12 deletions engine/apps/schedules/models/on_call_schedule.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
ICAL_UID,
)
from apps.schedules.ical_utils import (
EmptyShifts,
create_base_icalendar,
fetch_ical_file_or_get_error,
get_oncall_users_for_multiple_schedules,
Expand Down Expand Up @@ -278,22 +279,33 @@ def get_prev_and_current_ical_files(self):
(self.prev_ical_file_overrides, self.cached_ical_file_overrides),
]

def check_gaps_for_next_week(self) -> bool:
def check_gaps_and_empty_shifts_for_next_week(self) -> None:
datetime_start = timezone.now()
datetime_end = datetime_start + datetime.timedelta(days=7)
events = self.filter_events(
datetime_start,
datetime_end,
with_empty=True,
with_gap=True,
all_day_datetime=True,
)
final_events = self._resolve_schedule(events, datetime_start, datetime_end)
# get empty shifts from all events and gaps from final events
has_empty_shifts = len([event for event in events if event["is_empty"]]) != 0
has_gaps = len([final_event for final_event in final_events if final_event["is_gap"]]) != 0
if has_gaps != self.has_gaps or has_empty_shifts != self.has_empty_shifts:
self.has_gaps = has_gaps
self.has_empty_shifts = has_empty_shifts
self.save(update_fields=["has_gaps", "has_empty_shifts"])

def get_gaps_for_next_week(self) -> ScheduleEvents:
today = timezone.now()
events = self.final_events(today, today + datetime.timedelta(days=7))
gaps = [event for event in events if event["is_gap"] and not event["is_empty"]]
has_gaps = len(gaps) != 0
self.has_gaps = has_gaps
self.save(update_fields=["has_gaps"])
return has_gaps
return [event for event in events if event["is_gap"]]

def check_empty_shifts_for_next_week(self):
def get_empty_shifts_for_next_week(self) -> EmptyShifts:
today = timezone.now().date()
empty_shifts = list_of_empty_shifts_in_schedule(self, today, today + datetime.timedelta(days=7))
has_empty_shifts = len(empty_shifts) != 0
self.has_empty_shifts = has_empty_shifts
self.save(update_fields=["has_empty_shifts"])
return has_empty_shifts
return list_of_empty_shifts_in_schedule(self, today, today + datetime.timedelta(days=7))

def drop_cached_ical(self):
self._drop_primary_ical_file()
Expand Down
1 change: 1 addition & 0 deletions engine/apps/schedules/tasks/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
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,
Expand Down
23 changes: 23 additions & 0 deletions engine/apps/schedules/tasks/check_gaps_and_empty_shifts.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
from celery.utils.log import get_task_logger

from common.custom_celery_tasks import shared_dedicated_queue_retry_task

task_logger = get_task_logger(__name__)


@shared_dedicated_queue_retry_task()
def check_gaps_and_empty_shifts_in_schedule(schedule_pk):
from apps.schedules.models import OnCallSchedule

task_logger.info(f"Start check_gaps_and_empty_shifts_in_schedule {schedule_pk}")

try:
schedule = OnCallSchedule.objects.get(
pk=schedule_pk,
)
except OnCallSchedule.DoesNotExist:
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()
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 @@ -3,44 +3,23 @@
from django.core.cache import cache
from django.utils import timezone

from apps.schedules.ical_utils import list_of_empty_shifts_in_schedule
from apps.slack.utils import format_datetime_to_slack_with_time, post_message_to_channel
from common.custom_celery_tasks import shared_dedicated_queue_retry_task
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():
from apps.schedules.models import OnCallSchedule

task_logger.info("Start start_notify_about_empty_shifts_in_schedule")

schedules = OnCallSchedule.objects.all()

for schedule in schedules:
check_empty_shifts_in_schedule.apply_async((schedule.pk,))

task_logger.info("Finish start_notify_about_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):
from apps.schedules.models import OnCallSchedule

task_logger.info(f"Start check_empty_shifts_in_schedule {schedule_pk}")

try:
schedule = OnCallSchedule.objects.get(
pk=schedule_pk,
)
except OnCallSchedule.DoesNotExist:
task_logger.info(f"Tried to check_empty_shifts_in_schedule for non-existing schedule {schedule_pk}")
return

schedule.check_empty_shifts_for_next_week()
task_logger.info(f"Finish check_empty_shifts_in_schedule {schedule_pk}")
return


@shared_dedicated_queue_retry_task()
Expand All @@ -54,6 +33,7 @@ def start_notify_about_empty_shifts_in_schedule():
schedules = OnCallScheduleICal.objects.filter(
empty_shifts_report_sent_at__lte=week_ago,
channel__isnull=False,
organization__deleted_at__isnull=True,
)

for schedule in schedules:
Expand All @@ -79,9 +59,8 @@ 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

today = timezone.now().date()
empty_shifts = list_of_empty_shifts_in_schedule(schedule, today, today + timezone.timedelta(days=7))
schedule.empty_shifts_report_sent_at = today
empty_shifts = schedule.get_empty_shifts_for_next_week()
schedule.empty_shifts_report_sent_at = timezone.now().date()

if len(empty_shifts) != 0:
schedule.has_empty_shifts = True
Expand Down
37 changes: 7 additions & 30 deletions engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import datetime

import pytz
from celery.utils.log import get_task_logger
from django.core.cache import cache
Expand All @@ -11,36 +9,16 @@
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():
from apps.schedules.models import OnCallSchedule

task_logger.info("Start start_check_gaps_in_schedule")

schedules = OnCallSchedule.objects.all()

for schedule in schedules:
check_gaps_in_schedule.apply_async((schedule.pk,))

task_logger.info("Finish 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):
from apps.schedules.models import OnCallSchedule

task_logger.info(f"Start check_gaps_in_schedule {schedule_pk}")

try:
schedule = OnCallSchedule.objects.get(
pk=schedule_pk,
)
except OnCallSchedule.DoesNotExist:
task_logger.info(f"Tried to check_gaps_in_schedule for non-existing schedule {schedule_pk}")
return

schedule.check_gaps_for_next_week()
task_logger.info(f"Finish check_gaps_in_schedule {schedule_pk}")
return


@shared_dedicated_queue_retry_task()
Expand All @@ -54,6 +32,7 @@ def start_notify_about_gaps_in_schedule():
schedules = OnCallSchedule.objects.filter(
gaps_report_sent_at__lte=week_ago,
channel__isnull=False,
organization__deleted_at__isnull=True,
)

for schedule in schedules:
Expand All @@ -80,10 +59,8 @@ 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

now = timezone.now()
events = schedule.final_events(now, now + datetime.timedelta(days=7))
gaps = [event for event in events if event["is_gap"] and not event["is_empty"]]
schedule.gaps_report_sent_at = now.date()
gaps = schedule.get_gaps_for_next_week()
schedule.gaps_report_sent_at = timezone.now().date()

if len(gaps) != 0:
schedule.has_gaps = True
Expand Down
8 changes: 7 additions & 1 deletion engine/apps/schedules/tasks/refresh_ical_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@

from apps.alerts.tasks import notify_ical_schedule_shift # type: ignore[no-redef]
from apps.schedules.ical_utils import is_icals_equal, update_cached_oncall_users_for_schedule
from apps.schedules.tasks import notify_about_empty_shifts_in_schedule_task, notify_about_gaps_in_schedule_task
from apps.schedules.tasks import (
check_gaps_and_empty_shifts_in_schedule,
notify_about_empty_shifts_in_schedule_task,
notify_about_gaps_in_schedule_task,
)
from apps.slack.tasks import start_update_slack_user_group_for_schedules
from common.custom_celery_tasks import shared_dedicated_queue_retry_task

Expand Down Expand Up @@ -84,6 +88,8 @@ def refresh_ical_file(schedule_pk):
# update cached schedule on-call users
update_cached_oncall_users_for_schedule(schedule)

check_gaps_and_empty_shifts_in_schedule.apply_async((schedule_pk,))

if run_task:
notify_about_empty_shifts_in_schedule_task.apply_async((schedule_pk,))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should queue these 2 tasks after we are sure the gaps/empty shifts were updated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tasks are not about updating these fields, they notify users in Slack about gaps and empty shifts if ical was changed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. but these tasks, when run, will re-check for gaps and empty shifts (and update the fields too, it seems), so in some cases we will be doing the work twice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... 🤔 but I don't see how to avoid this without more detailed refactoring, since these notfify tasks use different types of events with different fields: ScheduleEvent and EmptyShifts. For regular update I use ScheduleEvent for both checks and parse calendars only once because we don't need an additional information about empty events there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense. I would at least leave some comment/TODO/issue that we would like to refactor this at some point.

notify_about_gaps_in_schedule_task.apply_async((schedule_pk,))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def test_no_empty_shifts_no_triggering_notification(

schedule.refresh_from_db()
assert empty_shifts_report_sent_at != schedule.empty_shifts_report_sent_at
assert schedule.has_empty_shifts is False


@pytest.mark.django_db
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def test_no_gaps_no_triggering_notification(

schedule.refresh_from_db()
assert gaps_report_sent_at != schedule.gaps_report_sent_at
assert schedule.check_gaps_for_next_week() is False
assert schedule.has_gaps is False


@pytest.mark.django_db
Expand Down Expand Up @@ -115,7 +115,7 @@ def test_gaps_in_the_past_no_triggering_notification(

schedule.refresh_from_db()
assert gaps_report_sent_at != schedule.gaps_report_sent_at
assert schedule.check_gaps_for_next_week() is False
assert schedule.has_gaps is False


@pytest.mark.django_db
Expand Down Expand Up @@ -166,7 +166,6 @@ def test_gaps_now_trigger_notification(
schedule.refresh_from_db()
assert gaps_report_sent_at != schedule.gaps_report_sent_at
assert schedule.has_gaps is True
assert schedule.check_gaps_for_next_week() is True


@pytest.mark.django_db
Expand Down Expand Up @@ -218,7 +217,6 @@ def test_gaps_near_future_trigger_notification(
schedule.refresh_from_db()
assert gaps_report_sent_at != schedule.gaps_report_sent_at
assert schedule.has_gaps is True
assert schedule.check_gaps_for_next_week() is True


@pytest.mark.django_db
Expand Down Expand Up @@ -267,4 +265,4 @@ 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.check_gaps_for_next_week() is False
assert schedule.has_gaps is False
10 changes: 0 additions & 10 deletions engine/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -506,21 +506,11 @@ class BrokerTypes:
"schedule": crontab(minute=1, hour=12, day_of_week="monday"),
"args": (),
},
"start_check_gaps_in_schedule": {
"task": "apps.schedules.tasks.notify_about_gaps_in_schedule.start_check_gaps_in_schedule",
"schedule": crontab(minute=0, hour=0),
"args": (),
},
"start_notify_about_empty_shifts_in_schedule": {
"task": "apps.schedules.tasks.notify_about_empty_shifts_in_schedule.start_notify_about_empty_shifts_in_schedule",
"schedule": crontab(minute=0, hour=12, day_of_week="monday"),
"args": (),
},
"start_check_empty_shifts_in_schedule": {
"task": "apps.schedules.tasks.notify_about_empty_shifts_in_schedule.start_check_empty_shifts_in_schedule",
"schedule": crontab(minute=0, hour=0),
"args": (),
},
"populate_slack_usergroups": {
"task": "apps.slack.tasks.populate_slack_usergroups",
"schedule": crontab(minute=0, hour=9, day_of_week="monday,wednesday,friday"),
Expand Down
1 change: 1 addition & 0 deletions engine/settings/celery_task_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"apps.schedules.tasks.refresh_ical_files.start_refresh_ical_files": {"queue": "default"},
"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"},
Expand Down
Loading