Trigger slack shift notifications on current shift change (#2080)
Before this change, a diff ical check (which happens with frequency with imported ical), particularly with overrides in an API/terraform schedule would trigger unexpected slack notifications because the prev vs current ical comparison will flag a diff, but when comparing current and previous shifts, `current_shifts` will have the shift in progress while the `prev_shifts` calculated from the overrides-only diff will most of the time be empty (unless you set/change an override at current time). Simplified the checks to always compare previous current shifts (ie. the ones in the schedule from the DB) vs the recalculated ones using the (refreshed) ical data from the schedule.
This commit is contained in:
Matias Bordese
GitHub
parent
44b105343a
commit
5d383c7d1d
3 changed files with 210 additions and 64 deletions
|
|
@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
|
||||
- Fix a bug with permissions for telegram user settings by @alexintech ([#2075](https://github.com/grafana/oncall/pull/2075))
|
||||
- Fix orphaned messages in Slack by @vadimkerr ([#2023](https://github.com/grafana/oncall/pull/2023))
|
||||
- Fix duplicated slack shift-changed notifications ([#2080](https://github.com/grafana/oncall/pull/2080))
|
||||
|
||||
## v1.2.34 (2023-05-31)
|
||||
|
||||
|
|
|
|||
|
|
@ -2,7 +2,6 @@ import datetime
|
|||
import json
|
||||
from copy import copy
|
||||
|
||||
import icalendar
|
||||
from django.apps import apps
|
||||
from django.utils import timezone
|
||||
|
||||
|
|
@ -12,7 +11,6 @@ from apps.schedules.ical_utils import (
|
|||
event_start_end_all_day_with_respect_to_type,
|
||||
get_icalendar_tz_or_utc,
|
||||
get_usernames_from_ical_event,
|
||||
is_icals_equal,
|
||||
memoized_users_in_ical,
|
||||
)
|
||||
from apps.slack.scenarios import scenario_step
|
||||
|
|
@ -191,8 +189,6 @@ def notify_ical_schedule_shift(schedule_pk):
|
|||
|
||||
MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT = 3
|
||||
|
||||
ical_changed = False
|
||||
|
||||
now = timezone.datetime.now(timezone.utc)
|
||||
# get list of iCalendars from current iCal files. If there is more than one calendar, primary calendar will always
|
||||
# be the first
|
||||
|
|
@ -240,60 +236,15 @@ def notify_ical_schedule_shift(schedule_pk):
|
|||
for item in drop:
|
||||
current_shifts.pop(item)
|
||||
|
||||
is_prev_ical_diff = False
|
||||
prev_overrides_priority = 0
|
||||
prev_shifts = {}
|
||||
prev_users = {}
|
||||
# compare events from prev and current shifts
|
||||
prev_shifts = json.loads(schedule.current_shifts) if not schedule.empty_oncall else {}
|
||||
# convert datetimes which was dumped to str back to datetime to calculate shift diff correct
|
||||
str_format = "%Y-%m-%d %X%z"
|
||||
for prev_shift in prev_shifts.values():
|
||||
prev_shift["start"] = datetime.datetime.strptime(prev_shift["start"], str_format)
|
||||
prev_shift["end"] = datetime.datetime.strptime(prev_shift["end"], str_format)
|
||||
|
||||
# Get list of tuples with prev and current ical file for each calendar. If there is more than one calendar, primary
|
||||
# calendar will be the first.
|
||||
# example result for ical calendar:
|
||||
# [(prev_ical_file_primary, current_ical_file_primary), (prev_ical_file_overrides, current_ical_file_overrides)]
|
||||
# example result for calendar with custom events:
|
||||
# [(prev_ical_file, current_ical_file)]
|
||||
prev_and_current_ical_files = schedule.get_prev_and_current_ical_files()
|
||||
|
||||
for prev_ical_file, current_ical_file in prev_and_current_ical_files:
|
||||
if prev_ical_file and (not current_ical_file or not is_icals_equal(current_ical_file, prev_ical_file)):
|
||||
task_logger.info(f"ical files are different")
|
||||
# If icals are not equal then compare current_events from them
|
||||
is_prev_ical_diff = True
|
||||
prev_calendar = icalendar.Calendar.from_ical(prev_ical_file)
|
||||
|
||||
prev_shifts_result, prev_users_result = get_current_shifts_from_ical(
|
||||
prev_calendar,
|
||||
schedule,
|
||||
prev_overrides_priority,
|
||||
)
|
||||
if prev_overrides_priority == 0 and prev_shifts_result:
|
||||
prev_overrides_priority = max([prev_shifts_result[uid]["priority"] for uid in prev_shifts_result]) + 1
|
||||
|
||||
prev_shifts.update(prev_shifts_result)
|
||||
prev_users.update(prev_users_result)
|
||||
|
||||
recalculate_shifts_with_respect_to_priority(prev_shifts, prev_users)
|
||||
|
||||
if is_prev_ical_diff:
|
||||
# drop events that don't intersection with current time
|
||||
drop = []
|
||||
for uid, prev_shift in prev_shifts.items():
|
||||
if not prev_shift["start"] < now < prev_shift["end"]:
|
||||
drop.append(uid)
|
||||
for item in drop:
|
||||
prev_shifts.pop(item)
|
||||
|
||||
shift_changed, diff_uids = calculate_shift_diff(current_shifts, prev_shifts)
|
||||
|
||||
else:
|
||||
# Else comparing events from prev and current shifts
|
||||
prev_shifts = json.loads(schedule.current_shifts) if not schedule.empty_oncall else {}
|
||||
# convert datetimes which was dumped to str back to datetime to calculate shift diff correct
|
||||
str_format = "%Y-%m-%d %X%z"
|
||||
for prev_shift in prev_shifts.values():
|
||||
prev_shift["start"] = datetime.datetime.strptime(prev_shift["start"], str_format)
|
||||
prev_shift["end"] = datetime.datetime.strptime(prev_shift["end"], str_format)
|
||||
|
||||
shift_changed, diff_uids = calculate_shift_diff(current_shifts, prev_shifts)
|
||||
shift_changed, diff_uids = calculate_shift_diff(current_shifts, prev_shifts)
|
||||
|
||||
if shift_changed:
|
||||
task_logger.info(f"shifts_changed: {diff_uids}")
|
||||
|
|
@ -370,11 +321,6 @@ def notify_ical_schedule_shift(schedule_pk):
|
|||
|
||||
if schedule.notify_oncall_shift_freq != OnCallSchedule.NotifyOnCallShiftFreq.NEVER:
|
||||
try:
|
||||
if ical_changed:
|
||||
slack_client.api_call(
|
||||
"chat.postMessage", channel=schedule.channel, text=f"Schedule {schedule.name} was changed"
|
||||
)
|
||||
|
||||
slack_client.api_call(
|
||||
"chat.postMessage",
|
||||
channel=schedule.channel,
|
||||
|
|
|
|||
|
|
@ -1,13 +1,16 @@
|
|||
import json
|
||||
import textwrap
|
||||
from datetime import datetime
|
||||
from unittest.mock import Mock, patch
|
||||
|
||||
import icalendar
|
||||
import pytest
|
||||
import pytz
|
||||
from django.utils import timezone
|
||||
|
||||
from apps.alerts.tasks.notify_ical_schedule_shift import notify_ical_schedule_shift
|
||||
from apps.alerts.tasks.notify_ical_schedule_shift import get_current_shifts_from_ical, notify_ical_schedule_shift
|
||||
from apps.schedules.ical_utils import memoized_users_in_ical
|
||||
from apps.schedules.models import OnCallScheduleICal
|
||||
from apps.schedules.models import CustomOnCallShift, OnCallScheduleCalendar, OnCallScheduleICal
|
||||
|
||||
ICAL_DATA = """
|
||||
BEGIN:VCALENDAR
|
||||
|
|
@ -105,3 +108,199 @@ def test_next_shift_notification_long_shifts(
|
|||
notification = slack_blocks[0]["text"]["text"]
|
||||
assert "*New on-call shift:*\nuser2" in notification
|
||||
assert "*Next on-call shift:*\nuser1" in notification
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_overrides_changes_no_current_no_triggering_notification(
|
||||
make_organization_and_user_with_slack_identities,
|
||||
make_user,
|
||||
make_schedule,
|
||||
make_on_call_shift,
|
||||
):
|
||||
organization, _, _, _ = make_organization_and_user_with_slack_identities()
|
||||
user1 = make_user(organization=organization, username="user1")
|
||||
# clear users pks <-> organization cache (persisting between tests)
|
||||
memoized_users_in_ical.cache_clear()
|
||||
|
||||
ical_before = textwrap.dedent(
|
||||
"""
|
||||
BEGIN:VCALENDAR
|
||||
PRODID:-//Google Inc//Google Calendar 70.9054//EN
|
||||
VERSION:2.0
|
||||
CALSCALE:GREGORIAN
|
||||
METHOD:PUBLISH
|
||||
BEGIN:VEVENT
|
||||
DTSTART:20230101T020000
|
||||
DTEND:20230101T170000
|
||||
DTSTAMP:20230101T000000
|
||||
UID:id1@google.com
|
||||
CREATED:20230101T000000
|
||||
DESCRIPTION:
|
||||
LAST-MODIFIED:20230101T000000
|
||||
LOCATION:
|
||||
SEQUENCE:1
|
||||
STATUS:CONFIRMED
|
||||
SUMMARY:user1
|
||||
TRANSP:TRANSPARENT
|
||||
END:VEVENT
|
||||
END:VCALENDAR"""
|
||||
)
|
||||
|
||||
# event outside current time is changed
|
||||
ical_after = textwrap.dedent(
|
||||
"""
|
||||
BEGIN:VCALENDAR
|
||||
PRODID:-//Google Inc//Google Calendar 70.9054//EN
|
||||
VERSION:2.0
|
||||
CALSCALE:GREGORIAN
|
||||
METHOD:PUBLISH
|
||||
BEGIN:VEVENT
|
||||
DTSTART:20230101T020000
|
||||
DTEND:20230101T210000
|
||||
DTSTAMP:20230101T000000
|
||||
UID:id1@google.com
|
||||
CREATED:20230101T000000
|
||||
DESCRIPTION:
|
||||
LAST-MODIFIED:20230101T000000
|
||||
LOCATION:
|
||||
SEQUENCE:2
|
||||
STATUS:CONFIRMED
|
||||
SUMMARY:user1
|
||||
TRANSP:TRANSPARENT
|
||||
END:VEVENT
|
||||
END:VCALENDAR"""
|
||||
)
|
||||
|
||||
schedule = make_schedule(
|
||||
organization,
|
||||
schedule_class=OnCallScheduleCalendar,
|
||||
name="test_schedule",
|
||||
channel="channel",
|
||||
prev_ical_file_overrides=ical_before,
|
||||
cached_ical_file_overrides=ical_after,
|
||||
)
|
||||
|
||||
now = timezone.now().replace(microsecond=0)
|
||||
start_date = now - timezone.timedelta(days=7)
|
||||
|
||||
data = {
|
||||
"start": start_date,
|
||||
"rotation_start": start_date,
|
||||
"duration": timezone.timedelta(seconds=3600 * 24),
|
||||
"priority_level": 1,
|
||||
"frequency": CustomOnCallShift.FREQUENCY_DAILY,
|
||||
}
|
||||
on_call_shift = make_on_call_shift(
|
||||
organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data
|
||||
)
|
||||
on_call_shift.add_rolling_users([[user1]])
|
||||
on_call_shift.schedules.add(schedule)
|
||||
|
||||
# setup current shifts before checking/triggering for notifications
|
||||
calendar = icalendar.Calendar.from_ical(schedule._ical_file_primary)
|
||||
current_shifts, _ = get_current_shifts_from_ical(calendar, schedule, 0)
|
||||
schedule.current_shifts = json.dumps(current_shifts, default=str)
|
||||
schedule.empty_oncall = False
|
||||
schedule.save()
|
||||
|
||||
with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call:
|
||||
notify_ical_schedule_shift(schedule.oncallschedule_ptr_id)
|
||||
|
||||
assert not mock_slack_api_call.called
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_no_changes_no_triggering_notification(
|
||||
make_organization_and_user_with_slack_identities,
|
||||
make_user,
|
||||
make_schedule,
|
||||
make_on_call_shift,
|
||||
):
|
||||
organization, _, _, _ = make_organization_and_user_with_slack_identities()
|
||||
user1 = make_user(organization=organization, username="user1")
|
||||
# clear users pks <-> organization cache (persisting between tests)
|
||||
memoized_users_in_ical.cache_clear()
|
||||
|
||||
schedule = make_schedule(
|
||||
organization,
|
||||
schedule_class=OnCallScheduleCalendar,
|
||||
name="test_schedule",
|
||||
channel="channel",
|
||||
prev_ical_file_overrides=None,
|
||||
cached_ical_file_overrides=None,
|
||||
)
|
||||
|
||||
now = timezone.now().replace(microsecond=0)
|
||||
start_date = now - timezone.timedelta(days=7)
|
||||
data = {
|
||||
"start": start_date,
|
||||
"rotation_start": start_date,
|
||||
"duration": timezone.timedelta(seconds=3600 * 24),
|
||||
"priority_level": 1,
|
||||
"frequency": CustomOnCallShift.FREQUENCY_DAILY,
|
||||
}
|
||||
on_call_shift = make_on_call_shift(
|
||||
organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data
|
||||
)
|
||||
on_call_shift.add_rolling_users([[user1]])
|
||||
on_call_shift.schedules.add(schedule)
|
||||
|
||||
# setup current shifts before checking/triggering for notifications
|
||||
calendar = icalendar.Calendar.from_ical(schedule._ical_file_primary)
|
||||
current_shifts, _ = get_current_shifts_from_ical(calendar, schedule, 0)
|
||||
schedule.current_shifts = json.dumps(current_shifts, default=str)
|
||||
schedule.empty_oncall = False
|
||||
schedule.save()
|
||||
|
||||
with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call:
|
||||
notify_ical_schedule_shift(schedule.oncallschedule_ptr_id)
|
||||
|
||||
assert not mock_slack_api_call.called
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_current_shift_changes_trigger_notification(
|
||||
make_organization_and_user_with_slack_identities,
|
||||
make_user,
|
||||
make_schedule,
|
||||
make_on_call_shift,
|
||||
):
|
||||
organization, _, _, _ = make_organization_and_user_with_slack_identities()
|
||||
user1 = make_user(organization=organization, username="user1")
|
||||
# clear users pks <-> organization cache (persisting between tests)
|
||||
memoized_users_in_ical.cache_clear()
|
||||
|
||||
schedule = make_schedule(
|
||||
organization,
|
||||
schedule_class=OnCallScheduleCalendar,
|
||||
name="test_schedule",
|
||||
channel="channel",
|
||||
prev_ical_file_overrides=None,
|
||||
cached_ical_file_overrides=None,
|
||||
)
|
||||
|
||||
now = timezone.now().replace(microsecond=0)
|
||||
start_date = now - timezone.timedelta(days=7)
|
||||
data = {
|
||||
"start": start_date,
|
||||
"rotation_start": start_date,
|
||||
"duration": timezone.timedelta(seconds=3600 * 24),
|
||||
"priority_level": 1,
|
||||
"frequency": CustomOnCallShift.FREQUENCY_DAILY,
|
||||
}
|
||||
on_call_shift = make_on_call_shift(
|
||||
organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data
|
||||
)
|
||||
on_call_shift.add_rolling_users([[user1]])
|
||||
on_call_shift.schedules.add(schedule)
|
||||
schedule.refresh_ical_file()
|
||||
|
||||
# setup empty current shifts before checking/triggering for notifications
|
||||
schedule.current_shifts = json.dumps({}, default=str)
|
||||
schedule.empty_oncall = False
|
||||
schedule.save()
|
||||
|
||||
with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call:
|
||||
notify_ical_schedule_shift(schedule.oncallschedule_ptr_id)
|
||||
|
||||
assert mock_slack_api_call.called
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue