Update schedule slack notifications #2710

Ferril · 2023-08-01T09:39:58Z

What this PR does

Update schedule slack notifications to use schedule final events instead of getting events from iCal

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
CHANGELOG.md updated (or pr:no changelog PR label added if not required)

…otifications

matiasb

LGTM

A few comments added.
Great you added more tests covering different notification trigger (or not) scenarios 👍
And really nice to see the notify task simplification!

matiasb · 2023-08-01T17:15:33Z

engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py

-    gaps = list_of_gaps_in_schedule(schedule, today, today + timezone.timedelta(days=7))
-    schedule.gaps_report_sent_at = today
+    now = timezone.now()
+    events = schedule.final_events(now, now + datetime.timedelta(days=7), with_empty=False)


Do we need to set with_empty=False here? final_events will still return gaps if there are any, won't it? (and empty shifts should be detected by the respective task, right?)

Also, could we use the check_gaps_for_next_week method from schedule in this case? (I see it may require some refactoring to reuse it here, maybe for a next PR)

You are right, we can filter it in the task. It makes sense since we need to filer the output anyway.

I think, if we change using of check_gaps_for_next_week we should also change using check_empty_shifts_for_next_week since they have pretty similar logic. I would propose to do it in separate PR

matiasb · 2023-08-01T17:38:05Z

engine/apps/schedules/models/on_call_schedule.py

-        gaps = list_of_gaps_in_schedule(self, today, today + datetime.timedelta(days=7))
+    def check_gaps_for_next_week(self) -> bool:
+        today = timezone.now()
+        events = self.final_events(today, today + datetime.timedelta(days=7), False, True)


Similar as noted below, wondering if if/why we need to pass with_empty=False here.

Also, I think it would be clearer setting these flags as kw args make it easier to understand what we are filtering at a quick glance.

matiasb · 2023-08-01T17:41:14Z

engine/apps/schedules/models/on_call_schedule.py

@@ -396,9 +398,11 @@ def filter_events(

        return events

-    def final_events(self, datetime_start, datetime_end):
+    def final_events(self, datetime_start, datetime_end, with_empty=True, with_gap=True):


How sure are that we need to enable these flags here? I would really like (to believe?) that when checking final events, we are interested in the final schedule events only, without needing to tune the options.

It's mostly needed for slack notifications about new/next shifts. We need only shifts without gaps and empties there. We already have logic to filter such kind of events in filter_events and it makes notify_ical_schedule_shift cleaner.

matiasb · 2023-08-01T17:56:48Z

engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py

+    on_call_shift.add_rolling_users([[user1]])
+    schedule_no_gaps.refresh_ical_file()
+
+    assert schedule_no_gaps.check_gaps_for_next_week() is False


Maybe these check_gaps_for_next_week checks could be added to the previous test cases? (and then we may not need all the variations below which check almost the same scenarios that above?)

makes sense 👍

matiasb · 2023-08-01T18:00:28Z

engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py

+
+
+@pytest.mark.django_db
+def test_empty_non_empty_shifts_trigger_notification(


I guess this is the original behavior, but wondering if this should be a warning in the schedule UI but not triggering a notification (since there is still an active user for the shift). In any case, something to discuss later.

Agree, we should discuss it later. I think, the main point is make customer know that something is wrong with one of their shifts despite the presence of an active user.

matiasb · 2023-08-01T18:08:20Z

engine/apps/alerts/tasks/notify_ical_schedule_shift.py

+
+    prev_shifts = json.loads(schedule.current_shifts) if not schedule.empty_oncall else []
+    prev_shifts_updated = False
+    # convert prev_shifts to new events format for compatibility with the previous version of this task


It will be nice to leave a task for later to drop this conversion. Optional, if you think it makes sense, I would separate this "translation" in a helper function in this module (so it is simpler to follow the task logic ignoring this compatibility thing)

matiasb · 2023-08-01T18:15:21Z

engine/apps/alerts/tasks/notify_ical_schedule_shift.py

+
+    datetime_end = now + datetime.timedelta(days=days_to_lookup)
+
+    next_shifts_unfiltered = schedule.final_events(now, datetime_end, False, False)


Not now, but maybe we could consider having only one call to final_events?

It would be great, but in this case we need to decide how to calculate days_to_lookup

matiasb · 2023-08-01T18:18:44Z

engine/apps/alerts/tasks/notify_ical_schedule_shift.py

+        if now < next_shift["start"]:
+            next_shifts.append(next_shift)
+
+    next_shifts = sorted(next_shifts, key=lambda shift: shift["start"])


final_events should already return events sorted by shift["start"]

matiasb · 2023-08-01T18:25:24Z

engine/apps/alerts/tasks/notify_ical_schedule_shift.py

+        schedule.empty_oncall = True
+    else:
+        schedule.empty_oncall = False
+        schedule.current_shifts = json.dumps(current_shifts, default=str)


nit, I think this could be changed to something like

schedule.empty_oncall = len(current_shifts) == 0 if not schedule.empty_oncall: schedule.current_shifts = json.dumps(current_shifts, default=str)

?

…e tests

# Conflicts: # CHANGELOG.md # engine/apps/schedules/models/on_call_schedule.py # engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py # engine/apps/slack/scenarios/schedules.py

Ferril added 2 commits August 1, 2023 11:34

Use list of final events instead of parsing ical for schedule slack n…

e098553

…otifications

Update tests

2a27486

Ferril added the pr:no public docs Added to a PR that does not require public documentation updates label Aug 1, 2023

Ferril requested a review from a team August 1, 2023 09:39

Update CHANGELOG.md

d6294f6

matiasb approved these changes Aug 1, 2023

View reviewed changes

Ferril added 4 commits August 2, 2023 15:08

Fix schedule shift slack notification task

0cad345

Add test

0d74e14

Correct filtering schedule events for slack notification tasks, updat…

b70cb29

…e tests

Merge branch 'dev' into update-schedule-slack-notifications

810b4d2

# Conflicts: # CHANGELOG.md # engine/apps/schedules/models/on_call_schedule.py # engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py # engine/apps/slack/scenarios/schedules.py

matiasb approved these changes Aug 2, 2023

View reviewed changes

Ferril added this pull request to the merge queue Aug 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 3, 2023

Ferril added this pull request to the merge queue Aug 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 3, 2023

Ferril and others added 2 commits August 3, 2023 09:34

Merge branch 'dev' into update-schedule-slack-notifications

80594e9

Merge branch 'dev' into update-schedule-slack-notifications

1926f60

Ferril added this pull request to the merge queue Aug 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 3, 2023

mypy fix

7e34ca0

Ferril added this pull request to the merge queue Aug 3, 2023

Merged via the queue into dev with commit 0494afa Aug 3, 2023

Ferril deleted the update-schedule-slack-notifications branch August 3, 2023 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update schedule slack notifications #2710

Update schedule slack notifications #2710

Ferril commented Aug 1, 2023 •

edited

Loading

matiasb left a comment

matiasb Aug 1, 2023

Ferril Aug 2, 2023

matiasb Aug 1, 2023

matiasb Aug 1, 2023

Ferril Aug 2, 2023 •

edited

Loading

matiasb Aug 1, 2023

Ferril Aug 2, 2023

matiasb Aug 1, 2023

Ferril Aug 2, 2023

matiasb Aug 1, 2023

matiasb Aug 1, 2023

Ferril Aug 2, 2023

matiasb Aug 1, 2023

matiasb Aug 1, 2023



		@pytest.mark.django_db
		def test_empty_non_empty_shifts_trigger_notification(


		datetime_end = now + datetime.timedelta(days=days_to_lookup)

		next_shifts_unfiltered = schedule.final_events(now, datetime_end, False, False)

Update schedule slack notifications #2710

Update schedule slack notifications #2710

Conversation

Ferril commented Aug 1, 2023 • edited Loading

What this PR does

Checklist

matiasb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ferril Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ferril commented Aug 1, 2023 •

edited

Loading

Ferril Aug 2, 2023 •

edited

Loading