From 0494afac854b4047e927aecee6141780ff2d2d8a Mon Sep 17 00:00:00 2001 From: Yulya Artyukhina Date: Thu, 3 Aug 2023 14:38:01 +0200 Subject: [PATCH 01/17] Update schedule slack notifications (#2710) # What this PR does Update schedule slack notifications to use schedule final events instead of getting events from iCal ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- CHANGELOG.md | 1 + .../tasks/notify_ical_schedule_shift.py | 404 +++++------------- .../tests/test_notify_ical_schedule_shift.py | 369 +++++++++++++++- engine/apps/schedules/ical_utils.py | 52 +-- .../apps/schedules/models/on_call_schedule.py | 24 +- .../notify_about_empty_shifts_in_schedule.py | 2 +- .../tasks/notify_about_gaps_in_schedule.py | 18 +- ...t_notify_about_empty_shifts_in_schedule.py | 170 ++++++++ .../test_notify_about_gaps_in_schedule.py | 281 ++++++++++++ .../schedules/tests/test_on_call_schedule.py | 4 +- engine/apps/slack/scenarios/schedules.py | 54 +-- 11 files changed, 989 insertions(+), 390 deletions(-) create mode 100644 engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py create mode 100644 engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ccf1e172..14458e77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Skip past due swap requests when calculating events ([2718](https://github.com/grafana/oncall/pull/2718)) +- Update schedule slack notifications to use schedule final events by @Ferril ([#2710](https://github.com/grafana/oncall/pull/2710)) ### Fixed diff --git a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py index e5f1d136..0324783f 100644 --- a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py @@ -1,17 +1,11 @@ import datetime import json -from copy import copy +import typing +from typing import TYPE_CHECKING from django.utils import timezone -from apps.schedules.ical_events import ical_events -from apps.schedules.ical_utils import ( - calculate_shift_diff, - event_start_end_all_day_with_respect_to_type, - get_icalendar_tz_or_utc, - get_usernames_from_ical_event, - memoized_users_in_ical, -) +from apps.schedules.ical_utils import calculate_shift_diff, parse_event_uid from apps.slack.scenarios import scenario_step from apps.slack.slack_client import SlackClientWithErrorHandling from apps.slack.slack_client.exceptions import SlackAPIException, SlackAPITokenException @@ -19,159 +13,45 @@ from common.custom_celery_tasks import shared_dedicated_queue_retry_task from .task_logger import task_logger +if TYPE_CHECKING: + from apps.schedules.models import OnCallSchedule -def get_current_shifts_from_ical(calendar, schedule, min_priority=0): - calendar_tz = get_icalendar_tz_or_utc(calendar) - now = datetime.datetime.now(timezone.utc) - events_from_ical_for_three_days = ical_events.get_events_from_ical_between( - calendar, now - datetime.timedelta(days=1), now + datetime.timedelta(days=1) - ) - shifts = {} - current_users = {} - for event in events_from_ical_for_three_days: - usernames, priority = get_usernames_from_ical_event(event) - users = memoized_users_in_ical(tuple(usernames), schedule.organization) - if len(users) > 0: - event_start, event_end, all_day_event = event_start_end_all_day_with_respect_to_type(event, calendar_tz) - if event["UID"] in shifts: - existing_event = shifts[event["UID"]] - if existing_event["start"] < now < existing_event["end"]: - continue - shifts[event["UID"]] = { - "users": [u.pk for u in users], - "start": event_start, - "end": event_end, - "all_day": all_day_event, - "priority": priority + min_priority, # increase priority for overrides - "priority_increased_by": min_priority, +def convert_prev_shifts_to_new_format(prev_shifts: dict, schedule: "OnCallSchedule") -> list: + new_prev_shifts = [] + user_ids = [] + users_info: typing.Dict[int, typing.Dict[str, str]] = {} + for shift in prev_shifts.values(): + user_ids.extend(shift.get("users", [])) + prev_users = schedule.organization.users.filter(id__in=user_ids) + for user in prev_users: + users_info.setdefault( + user.id, + { + "display_name": user.username, + "email": user.email, + "pk": user.public_primary_key, + "avatar_full": user.avatar_full_url, + }, + ) + for uid, shift in prev_shifts.items(): + shift_pk, _ = parse_event_uid(uid) + new_prev_shifts.append( + { + "users": [users_info[user_pk] for user_pk in shift["users"]], + "start": shift["start"], + "end": shift["end"], + "all_day": shift["all_day"], + "priority_level": shift["priority"], + "shift": {"pk": shift_pk}, } - current_users[event["UID"]] = users - - return shifts, current_users - - -def get_next_shifts_from_ical(calendar, schedule, min_priority=0, days_to_lookup=3): - calendar_tz = get_icalendar_tz_or_utc(calendar) - now = datetime.datetime.now(timezone.utc) - next_events_from_ical = ical_events.get_events_from_ical_between( - calendar, now - datetime.timedelta(days=1), now + datetime.timedelta(days=days_to_lookup) - ) - shifts = {} - for event in next_events_from_ical: - usernames, priority = get_usernames_from_ical_event(event) - users = memoized_users_in_ical(tuple(usernames), schedule.organization) - if len(users) > 0: - event_start, event_end, all_day_event = event_start_end_all_day_with_respect_to_type(event, calendar_tz) - - # next_shifts are not stored in db so we can use User objects directly - shifts[f"{event_start.timestamp()}_{event['UID']}"] = { - "users": users, - "start": event_start, - "end": event_end, - "all_day": all_day_event, - "priority": priority + min_priority, # increase priority for overrides - "priority_increased_by": min_priority, - } - - return shifts - - -def recalculate_shifts_with_respect_to_priority(shifts, users=None): - flag = True - while flag: - splitted_shifts = {} - uids_to_pop = set() - splitted = False - flag = False - for outer_k, outer_shift in shifts.items(): - if not splitted: - for inner_k, inner_shift in shifts.items(): - if outer_k == inner_k: - continue - else: - if outer_shift.get("priority", 0) > inner_shift.get("priority", 0): - if outer_shift["start"] > inner_shift["start"] and outer_shift["end"] < inner_shift["end"]: - new_uid_r = f"{inner_k}-split-r" - new_uid_l = f"{inner_k}-split-l" - splitted_shift_left = copy(inner_shift) - splitted_shift_right = copy(inner_shift) - splitted_shift_left["end"] = outer_shift["start"] - splitted_shift_right["start"] = outer_shift["end"] - splitted_shift_left["all_day"] = False - splitted_shift_right["all_day"] = False - splitted_shifts[new_uid_l] = splitted_shift_left - splitted_shifts[new_uid_r] = splitted_shift_right - uids_to_pop.add(inner_k) - if users is not None: - users[new_uid_l] = users[inner_k] - users[new_uid_r] = users[inner_k] - - splitted = True - flag = True - break - elif outer_shift["start"] <= inner_shift["start"] < outer_shift["end"] < inner_shift["end"]: - inner_shift["start"] = outer_shift["end"] - flag = True - elif outer_shift["end"] >= inner_shift["end"] > outer_shift["start"] > inner_shift["start"]: - inner_shift["end"] = outer_shift["start"] - flag = True - elif ( - outer_shift["start"] <= inner_shift["start"] - and outer_shift["end"] >= inner_shift["end"] - ): - uids_to_pop.add(inner_k) - flag = True - else: - flag = False - elif outer_shift.get("priority", 0) < inner_shift.get("priority", 0): - if inner_shift["start"] > outer_shift["start"] and inner_shift["end"] < outer_shift["end"]: - new_uid_r = f"{outer_k}-split-r" - new_uid_l = f"{outer_k}-split-l" - splitted_shift_left = copy(outer_shift) - splitted_shift_right = copy(outer_shift) - splitted_shift_left["all_day"] = False - splitted_shift_right["all_day"] = False - splitted_shift_left["end"] = inner_shift["start"] - splitted_shift_right["start"] = inner_shift["end"] - splitted_shifts[new_uid_l] = splitted_shift_left - splitted_shifts[new_uid_r] = splitted_shift_right - uids_to_pop.add(outer_k) - - if users is not None: - users[new_uid_l] = users[outer_k] - users[new_uid_r] = users[outer_k] - - splitted = True - flag = True - break - elif inner_shift["start"] <= outer_shift["start"] < inner_shift["end"] < outer_shift["end"]: - outer_shift["start"] = inner_shift["end"] - flag = True - elif inner_shift["end"] >= outer_shift["end"] > inner_shift["start"] > outer_shift["start"]: - outer_shift["end"] = inner_shift["start"] - flag = True - elif ( - inner_shift["start"] <= outer_shift["start"] - and inner_shift["end"] >= outer_shift["end"] - ): - uids_to_pop.add(outer_k) - flag = True - else: - flag = False - else: - flag = False - else: - break - - shifts.update(splitted_shifts) - for uid in uids_to_pop: - shifts.pop(uid) + ) + return new_prev_shifts @shared_dedicated_queue_retry_task() def notify_ical_schedule_shift(schedule_pk): - task_logger.info(f"Notify ical schedule shift {schedule_pk}") + task_logger.info(f"Start notify ical schedule shift {schedule_pk}") from apps.schedules.models import OnCallSchedule try: @@ -183,160 +63,104 @@ def notify_ical_schedule_shift(schedule_pk): return if schedule.organization.slack_team_identity is None: - task_logger.info(f"Trying to notify ical schedule shift with no slack team identity {schedule_pk}") + task_logger.info( + f"Trying to notify ical schedule shift with no slack team identity {schedule_pk}, " + f"organization {schedule.organization_id}" + ) return elif schedule.organization.deleted_at: - task_logger.info(f"Trying to notify ical schedule shift from deleted organization {schedule_pk}") + task_logger.info( + f"Trying to notify ical schedule shift from deleted organization {schedule_pk}, " + f"organization {schedule.organization_id}" + ) return + task_logger.info(f"Notify ical schedule shift {schedule_pk}, organization {schedule.organization_id}") + MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT = 3 now = datetime.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 - current_calendars = schedule.get_icalendars() - current_shifts = {} - # expected current_shifts structure: - # { - # some uid: { - # "users": [users pks], - # "start": event start date, - # "end": event end date, - # "all_day": bool if event has all-day type, - # "priority": priority level, - # "priority_increased_by": min priority level of primary calendar, (for primary calendar event it is 0) - # }, - # } + current_shifts = schedule.final_events(now, now, with_empty=False, with_gap=False) - # Current_user dict exists because it's bad idea to serialize User objects. - # Instead users' pks are stored in db for calculation related to shift diff. - # When it is needed to pass shift's user (e.g. in def get_report_blocks_ical()) - # we take users from current_users{} by shift uuid and replace users' pk - current_users = {} + 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 + if prev_shifts and isinstance(prev_shifts, dict): + prev_shifts = convert_prev_shifts_to_new_format(prev_shifts, schedule) + prev_shifts_updated = True - overrides_priority = 0 - for calendar in current_calendars: - if calendar is not None: - current_shifts_result, current_users_result = get_current_shifts_from_ical( - calendar, - schedule, - overrides_priority, - ) - if overrides_priority == 0 and current_shifts_result: - overrides_priority = max([current_shifts_result[uid]["priority"] for uid in current_shifts_result]) + 1 - current_shifts.update(current_shifts_result) - current_users.update(current_users_result) - - recalculate_shifts_with_respect_to_priority(current_shifts, current_users) - - # drop events that don't intersection with current time - drop = [] - for uid, current_shift in current_shifts.items(): - if not current_shift["start"] < now < current_shift["end"]: - drop.append(uid) - for item in drop: - current_shifts.pop(item) - - # 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(): + for prev_shift in prev_shifts: 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_shifts = calculate_shift_diff(current_shifts, prev_shifts) - if shift_changed: - task_logger.info(f"shifts_changed: {diff_uids}") - # Get only new/changed shifts to send a reminder message. - new_shifts = [] - for uid in diff_uids: - # using copy to not to mutate original current_shifts dict which will be stored in db as current_shifts - new_shift = copy(current_shifts[uid]) - # replace users' pk by objects to make reminder message from new shifts - new_shift["users"] = current_users[uid] - new_shifts.append(new_shift) - new_shifts = sorted(new_shifts, key=lambda shift: shift["start"]) - - if len(new_shifts) != 0: - days_to_lookup = (new_shifts[-1]["end"].date() - now.date()).days + 1 - days_to_lookup = max([days_to_lookup, MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT]) - else: - days_to_lookup = MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT - - next_shifts = {} - next_overrides_priority = 0 - - for calendar in current_calendars: - if calendar is not None: - next_shifts_result = get_next_shifts_from_ical( - calendar, - schedule, - next_overrides_priority, - days_to_lookup=days_to_lookup, - ) - if next_overrides_priority == 0 and next_shifts_result: - next_overrides_priority = ( - max([next_shifts_result[uid]["priority"] for uid in next_shifts_result]) + 1 - ) - - next_shifts.update(next_shifts_result) - - recalculate_shifts_with_respect_to_priority(next_shifts) - - # drop events that already started - drop = [] - for uid, next_shift in next_shifts.items(): - if now > next_shift["start"]: - drop.append(uid) - for item in drop: - next_shifts.pop(item) - - next_shifts_from_ical = sorted(next_shifts.values(), key=lambda shift: shift["start"]) - - upcoming_shifts = [] - # Add the earliest next_shift - if len(next_shifts_from_ical) > 0: - earliest_shift = next_shifts_from_ical[0] - upcoming_shifts.append(earliest_shift) - # Check if there are next shifts with the same start as the earliest - for shift in next_shifts_from_ical[1:]: - if shift["start"] == earliest_shift["start"]: - upcoming_shifts.append(shift) - - empty_oncall = len(current_shifts) == 0 - if empty_oncall: - schedule.empty_oncall = True - else: - schedule.empty_oncall = False + # Do not notify if there is no difference between current and previous shifts + if not shift_changed: + task_logger.info(f"No shift diff found for schedule {schedule_pk}, organization {schedule.organization_id}") + # If prev shifts were converted to a new format, update related field in db + if prev_shifts_updated: schedule.current_shifts = json.dumps(current_shifts, default=str) + schedule.save(update_fields=["current_shifts"]) + return - schedule.save(update_fields=["current_shifts", "empty_oncall"]) + new_shifts = sorted(diff_shifts, key=lambda shift: shift["start"]) - if len(new_shifts) > 0 or empty_oncall: - task_logger.info(f"new_shifts: {new_shifts}") + # get days_to_lookup for next shifts + if len(new_shifts) != 0: + max_end_date = max([shift["end"].date() for shift in new_shifts]) + days_to_lookup = (max_end_date - now.date()).days + 1 + days_to_lookup = max([days_to_lookup, MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT]) + else: + days_to_lookup = MIN_DAYS_TO_LOOKUP_FOR_THE_END_OF_EVENT + + datetime_end = now + datetime.timedelta(days=days_to_lookup) + + next_shifts_unfiltered = schedule.final_events(now, datetime_end, with_empty=False, with_gap=False) + # drop events that already started + next_shifts = [] + for next_shift in next_shifts_unfiltered: + if now < next_shift["start"]: + next_shifts.append(next_shift) + + upcoming_shifts = [] + # Add the earliest next_shift + if len(next_shifts) > 0: + earliest_shift = next_shifts[0] + upcoming_shifts.append(earliest_shift) + # Check if there are next shifts with the same start as the earliest + for shift in next_shifts[1:]: + if shift["start"] == earliest_shift["start"]: + upcoming_shifts.append(shift) + + schedule.empty_oncall = len(current_shifts) == 0 + if not schedule.empty_oncall: + schedule.current_shifts = json.dumps(current_shifts, default=str) + + schedule.save(update_fields=["current_shifts", "empty_oncall"]) + + if len(new_shifts) > 0 or schedule.empty_oncall: + task_logger.info(f"new_shifts: {new_shifts}") + if schedule.notify_oncall_shift_freq != OnCallSchedule.NotifyOnCallShiftFreq.NEVER: slack_client = SlackClientWithErrorHandling(schedule.organization.slack_team_identity.bot_access_token) step = scenario_step.ScenarioStep.get_step("schedules", "EditScheduleShiftNotifyStep") - report_blocks = step.get_report_blocks_ical(new_shifts, upcoming_shifts, schedule, empty_oncall) + report_blocks = step.get_report_blocks_ical(new_shifts, upcoming_shifts, schedule, schedule.empty_oncall) - if schedule.notify_oncall_shift_freq != OnCallSchedule.NotifyOnCallShiftFreq.NEVER: - try: - slack_client.api_call( - "chat.postMessage", - channel=schedule.channel, - blocks=report_blocks, - text=f"On-call shift for schedule {schedule.name} has changed", - ) - except SlackAPITokenException: - pass - except SlackAPIException as e: - if e.response["error"] == "channel_not_found": - print(e) - elif e.response["error"] == "is_archived": - print(e) - elif e.response["error"] == "invalid_auth": - print(e) - else: - raise e + try: + slack_client.api_call( + "chat.postMessage", + channel=schedule.channel, + blocks=report_blocks, + text=f"On-call shift for schedule {schedule.name} has changed", + ) + except SlackAPITokenException: + pass + except SlackAPIException as e: + expected_exceptions = ["channel_not_found", "is_archived", "invalid_auth"] + if e.response["error"] in expected_exceptions: + print(e) + else: + raise e diff --git a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py index e5396eca..d5925c9d 100644 --- a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py @@ -3,14 +3,13 @@ import json import textwrap 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 get_current_shifts_from_ical, notify_ical_schedule_shift +from apps.alerts.tasks.notify_ical_schedule_shift import notify_ical_schedule_shift from apps.schedules.ical_utils import memoized_users_in_ical -from apps.schedules.models import CustomOnCallShift, OnCallScheduleCalendar, OnCallScheduleICal +from apps.schedules.models import CustomOnCallShift, OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb ICAL_DATA = """ BEGIN:VCALENDAR @@ -72,7 +71,7 @@ def test_current_overrides_ical_schedule_is_none( ) # this should not raise - notify_ical_schedule_shift(ical_schedule.oncallschedule_ptr_id) + notify_ical_schedule_shift(ical_schedule.pk) @pytest.mark.django_db @@ -102,7 +101,7 @@ def test_next_shift_notification_long_shifts( with patch("apps.alerts.tasks.notify_ical_schedule_shift.datetime", Mock(wraps=datetime)) as mock_datetime: mock_datetime.datetime.now.return_value = datetime.datetime(2021, 9, 29, 12, 0, tzinfo=pytz.UTC) with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: - notify_ical_schedule_shift(ical_schedule.oncallschedule_ptr_id) + notify_ical_schedule_shift(ical_schedule.pk) slack_blocks = mock_slack_api_call.call_args_list[0][1]["blocks"] notification = slack_blocks[0]["text"]["text"] @@ -176,12 +175,12 @@ def test_overrides_changes_no_current_no_triggering_notification( schedule_class=OnCallScheduleCalendar, name="test_schedule", channel="channel", - prev_ical_file_overrides=ical_before, - cached_ical_file_overrides=ical_after, + prev_ical_file_overrides=None, + cached_ical_file_overrides=ical_before, ) now = timezone.now().replace(microsecond=0) - start_date = now - timezone.timedelta(days=7) + start_date = now - timezone.timedelta(days=7, minutes=1) data = { "start": start_date, @@ -197,14 +196,15 @@ def test_overrides_changes_no_current_no_triggering_notification( 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) + current_shifts = schedule.final_events(now, now, False, False) schedule.current_shifts = json.dumps(current_shifts, default=str) schedule.empty_oncall = False + schedule.cached_ical_file_overrides = ical_after + schedule.prev_ical_file_overrides = ical_before schedule.save() with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: - notify_ical_schedule_shift(schedule.oncallschedule_ptr_id) + notify_ical_schedule_shift(schedule.pk) assert not mock_slack_api_call.called @@ -231,7 +231,7 @@ def test_no_changes_no_triggering_notification( ) now = timezone.now().replace(microsecond=0) - start_date = now - timezone.timedelta(days=7) + start_date = now - timezone.timedelta(days=7, minutes=1) data = { "start": start_date, "rotation_start": start_date, @@ -246,14 +246,13 @@ def test_no_changes_no_triggering_notification( 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) + current_shifts = schedule.final_events(now, now, False, False) 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) + notify_ical_schedule_shift(schedule.pk) assert not mock_slack_api_call.called @@ -301,11 +300,146 @@ def test_current_shift_changes_trigger_notification( schedule.save() with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: - notify_ical_schedule_shift(schedule.oncallschedule_ptr_id) + notify_ical_schedule_shift(schedule.pk) assert mock_slack_api_call.called +@pytest.mark.django_db +def test_next_shift_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") + user2 = make_user(organization=organization, username="user2") + # 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_1 = now - datetime.timedelta(days=7, minutes=1) + data_1 = { + "start": start_date_1, + "rotation_start": start_date_1, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + } + on_call_shift_1 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data_1 + ) + on_call_shift_1.add_rolling_users([[user1]]) + on_call_shift_1.schedules.add(schedule) + + start_date_2 = now + datetime.timedelta(minutes=10) + data_2 = { + "start": start_date_2, + "rotation_start": start_date_2, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 2, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + } + on_call_shift_2 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data_2 + ) + on_call_shift_2.add_rolling_users([[user1]]) + on_call_shift_2.schedules.add(schedule) + + schedule.refresh_ical_file() + + # setup empty current shifts before checking/triggering for notifications + current_shifts = schedule.final_events(now, now, False, False) + schedule.current_shifts = json.dumps(current_shifts, default=str) + schedule.empty_oncall = False + schedule.save() + + on_call_shift_2.add_rolling_users([[user2]]) + schedule.refresh_ical_file() + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_ical_schedule_shift(schedule.pk) + + assert not mock_slack_api_call.called + + +@pytest.mark.django_db +def test_lower_priority_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") + user2 = make_user(organization=organization, username="user2") + # 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 - datetime.timedelta(days=7, minutes=1) + data_1 = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 2, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + } + on_call_shift_1 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data_1 + ) + on_call_shift_1.add_rolling_users([[user1]]) + on_call_shift_1.schedules.add(schedule) + + data_2 = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + } + on_call_shift_2 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data_2 + ) + on_call_shift_2.add_rolling_users([[user1]]) + on_call_shift_2.schedules.add(schedule) + + schedule.refresh_ical_file() + + # setup empty current shifts before checking/triggering for notifications + current_shifts = schedule.final_events(now, now, False, False) + schedule.current_shifts = json.dumps(current_shifts, default=str) + schedule.empty_oncall = False + schedule.save() + + on_call_shift_2.add_rolling_users([[user2]]) + schedule.refresh_ical_file() + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_ical_schedule_shift(schedule.pk) + + assert not mock_slack_api_call.called + + @pytest.mark.django_db def test_vtimezone_changes_no_triggering_notification( make_organization_and_user_with_slack_identities, @@ -414,20 +548,213 @@ def test_vtimezone_changes_no_triggering_notification( name="test_ical_schedule", channel="channel", ical_url_primary="url", - prev_ical_file_primary=ical_before, - cached_ical_file_primary=ical_after, + prev_ical_file_primary=None, + cached_ical_file_primary=ical_before, prev_ical_file_overrides=None, cached_ical_file_overrides=None, ) # setup current shifts before checking/triggering for notifications - calendar = icalendar.Calendar.from_ical(ical_before) - current_shifts, _ = get_current_shifts_from_ical(calendar, schedule, 0) + now = datetime.datetime.now(timezone.utc) + current_shifts = schedule.final_events(now, now, False, False) + schedule.current_shifts = json.dumps(current_shifts, default=str) + schedule.empty_oncall = False + # update schedule cached ical to ical_after + schedule.prev_ical_file_primary = ical_before + schedule.cached_ical_file_primary = ical_after + schedule.save() + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_ical_schedule_shift(schedule.pk) + + assert not mock_slack_api_call.called + + +@pytest.mark.django_db +def test_no_changes_no_triggering_notification_from_old_to_new_task_version( + 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 with old version of shifts structure before checking/triggering for notifications + current_shifts = { + "test_shift_uid": { + "users": [user1.pk], + "start": start_date, + "end": start_date + data["duration"], + "all_day": False, + "priority": data["priority_level"], + "priority_increased_by": 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) + notify_ical_schedule_shift(schedule.pk) assert not mock_slack_api_call.called + + +@pytest.mark.django_db +def test_current_shift_changes_trigger_notification_from_old_to_new_task_version( + 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") + user2 = make_user(organization=organization, username="user2") + # 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 - datetime.timedelta(days=7) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.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 current shifts with old version of shifts structure before checking/triggering for notifications + current_shifts = { + "test_shift_uid": { + "users": [user1.pk], + "start": start_date, + "end": start_date + data["duration"], + "all_day": False, + "priority": data["priority_level"], + "priority_increased_by": 0, + } + } + schedule.current_shifts = json.dumps(current_shifts, default=str) + schedule.empty_oncall = False + schedule.save() + + on_call_shift.add_rolling_users([[user2]]) + schedule.refresh_ical_file() + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_ical_schedule_shift(schedule.pk) + + assert mock_slack_api_call.called + + +@pytest.mark.django_db +def test_next_shift_notification_long_and_short_shifts( + 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") + user2 = make_user(organization=organization, username="user2") + user3 = make_user(organization=organization, username="user3") + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date_1 = now - datetime.timedelta(days=1) + data_1 = { + "start": start_date_1, + "rotation_start": start_date_1, + "duration": datetime.timedelta(seconds=3600 * 24 * 7), # one week duration + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_WEEKLY, + "schedule": schedule, + } + on_call_shift_1 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data_1 + ) + on_call_shift_1.add_rolling_users([[user1], [user2]]) + + start_date_2 = now - datetime.timedelta(hours=1) + data_2 = { + "start": start_date_2, + "rotation_start": start_date_2, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_WEEKLY, + "schedule": schedule, + } + on_call_shift_2 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data_2 + ) + on_call_shift_2.add_rolling_users([[user3]]) + + 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.pk) + + assert mock_slack_api_call.called + notification = mock_slack_api_call.call_args[1]["blocks"][0]["text"]["text"] + new_shift_notification, next_shift_notification = notification.split("\n\n") + + assert "*New on-call shift:*\n[L1] user1" in new_shift_notification + assert "[L1] user3" in new_shift_notification + assert "*Next on-call shift:*\n[L1] user2" in notification diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 0324a462..b20cb0ea 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -504,20 +504,23 @@ def ical_date_to_datetime(date, tz, start): return date, all_day -def calculate_shift_diff(first_shift, second_shift): - fields_to_compare = ["users", "end", "start", "all_day", "priority"] +def calculate_shift_diff(shifts: list, prev_shifts: list) -> typing.Tuple[bool, list]: + """ + Get shifts diff comparing with the previous shifts + """ + fields_to_compare = ["users", "end", "start", "all_day", "priority_level", "shift"] - shift_changed = set(first_shift.keys()) != set(second_shift.keys()) - if not shift_changed: - diff = set() - for k, v in first_shift.items(): - for f in fields_to_compare: - if v.get(f) != second_shift[k].get(f): - shift_changed = True - diff.add(k) - break - else: - diff = set(first_shift.keys()) - set(second_shift.keys()) + shifts_fields = [{k: v for k, v in shift.items() if k in fields_to_compare} for shift in shifts] + prev_shifts_fields = [{k: v for k, v in shift.items() if k in fields_to_compare} for shift in prev_shifts] + + shift_changed = len(shifts) != len(prev_shifts) + + diff = [] + + for idx, shift in enumerate(shifts_fields): + if shift not in prev_shifts_fields: + shift_changed = True + diff.append(shifts[idx]) return shift_changed, diff @@ -611,29 +614,6 @@ def user_ical_export(user: "User", schedules: "OnCallScheduleQuerySet") -> bytes return ical_obj.to_ical() -def list_of_gaps_in_schedule( - schedule: "OnCallSchedule", start_date: datetime.date, end_date: datetime.date -) -> DatetimeIntervals: - calendars = schedule.get_icalendars() - intervals: DatetimeIntervals = [] - start_datetime = datetime.datetime.combine(start_date, datetime.time.min) + datetime.timedelta(milliseconds=1) - start_datetime = start_datetime.astimezone(pytz.UTC) - end_datetime = datetime.datetime.combine(end_date, datetime.time.max).astimezone(pytz.UTC) - - for calendar in calendars: - if calendar is not None: - calendar_tz = get_icalendar_tz_or_utc(calendar) - events = ical_events.get_events_from_ical_between( - calendar, - start_datetime, - end_datetime, - ) - for event in events: - start, end, _ = event_start_end_all_day_with_respect_to_type(event, calendar_tz) - intervals.append(DatetimeInterval(start, end)) - return detect_gaps(intervals, start_datetime, end_datetime) - - def detect_gaps(intervals: DatetimeIntervals, start: datetime.datetime, end: datetime.datetime) -> DatetimeIntervals: gaps: DatetimeIntervals = [] intervals = sorted(intervals, key=lambda dt: dt.start) diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index 5f5c49f7..56963e08 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -38,7 +38,6 @@ from apps.schedules.ical_utils import ( fetch_ical_file_or_get_error, get_oncall_users_for_multiple_schedules, list_of_empty_shifts_in_schedule, - list_of_gaps_in_schedule, list_of_oncall_shifts_from_ical, ) from apps.schedules.models import CustomOnCallShift @@ -279,9 +278,10 @@ class OnCallSchedule(PolymorphicModel): (self.prev_ical_file_overrides, self.cached_ical_file_overrides), ] - def check_gaps_for_next_week(self): - today = timezone.now().date() - 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)) + 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"]) @@ -367,7 +367,9 @@ class OnCallSchedule(PolymorphicModel): end = shift["end"] - datetime.timedelta(days=1) if all_day else shift["end"] if all_day and all_day_datetime: start = datetime.datetime.combine(start, datetime.datetime.min.time(), tzinfo=pytz.UTC) - end = datetime.datetime.combine(end, datetime.datetime.max.time(), tzinfo=pytz.UTC) + end = datetime.datetime.combine(end, datetime.datetime.max.time(), tzinfo=pytz.UTC).replace( + microsecond=0 + ) is_gap = shift.get("is_gap", False) shift_json: ScheduleEvent = { "all_day": all_day, @@ -403,9 +405,17 @@ class OnCallSchedule(PolymorphicModel): return events - def final_events(self, datetime_start: datetime.datetime, datetime_end: datetime.datetime) -> ScheduleEvents: + def final_events( + self, + datetime_start: datetime.datetime, + datetime_end: datetime.datetime, + with_empty: bool = True, + with_gap: bool = True, + ) -> ScheduleEvents: """Return schedule final events, after resolving shifts and overrides.""" - events = self.filter_events(datetime_start, datetime_end, with_empty=True, with_gap=True, all_day_datetime=True) + events = self.filter_events( + datetime_start, datetime_end, with_empty=with_empty, with_gap=with_gap, all_day_datetime=True + ) events = self._resolve_schedule(events, datetime_start, datetime_end) return events 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 1eed8863..a4a123e6 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 @@ -109,7 +109,7 @@ def notify_about_empty_shifts_in_schedule(schedule_pk): f'From {empty_shift.start.strftime("%b %d")} to {empty_shift.end.strftime("%b %d")}\n' ) text += all_day_text - text += f"*All-day* event in {empty_shift.calendar_tz} TZ\n" + 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" 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 40f0c1b9..5ef72f45 100644 --- a/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py +++ b/engine/apps/schedules/tasks/notify_about_gaps_in_schedule.py @@ -1,9 +1,10 @@ +import datetime + import pytz from celery.utils.log import get_task_logger from django.core.cache import cache from django.utils import timezone -from apps.schedules.ical_utils import list_of_gaps_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 @@ -79,20 +80,21 @@ def notify_about_gaps_in_schedule(schedule_pk): task_logger.info(f"Tried to notify_about_gaps_in_schedule for non-existing schedule {schedule_pk}") return - today = timezone.now().date() - 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)) + gaps = [event for event in events if event["is_gap"] and not event["is_empty"]] + schedule.gaps_report_sent_at = 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): - if gap.start: - start_verbal = format_datetime_to_slack_with_time(gap.start.astimezone(pytz.UTC).timestamp()) + if gap["start"]: + start_verbal = format_datetime_to_slack_with_time(gap["start"].astimezone(pytz.UTC).timestamp()) else: start_verbal = "..." - if gap.end: - end_verbal = format_datetime_to_slack_with_time(gap.end.astimezone(pytz.UTC).timestamp()) + if gap["end"]: + end_verbal = format_datetime_to_slack_with_time(gap["end"].astimezone(pytz.UTC).timestamp()) else: end_verbal = "..." text += f"From {start_verbal} to {end_verbal} (your TZ)\n" diff --git a/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py b/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py new file mode 100644 index 00000000..c4680715 --- /dev/null +++ b/engine/apps/schedules/tests/test_notify_about_empty_shifts_in_schedule.py @@ -0,0 +1,170 @@ +import datetime +from unittest.mock import patch + +import pytest +from django.utils import timezone + +from apps.api.permissions import LegacyAccessControlRole +from apps.schedules.ical_utils import memoized_users_in_ical +from apps.schedules.models import CustomOnCallShift, OnCallScheduleWeb +from apps.schedules.tasks import notify_about_empty_shifts_in_schedule + + +@pytest.mark.django_db +def test_no_empty_shifts_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=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - datetime.timedelta(days=7, minutes=1) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + schedule.refresh_ical_file() + + empty_shifts_report_sent_at = schedule.empty_shifts_report_sent_at + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_empty_shifts_in_schedule(schedule.pk) + + assert not mock_slack_api_call.called + + schedule.refresh_from_db() + assert empty_shifts_report_sent_at != schedule.empty_shifts_report_sent_at + + +@pytest.mark.django_db +def test_empty_shifts_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", role=LegacyAccessControlRole.VIEWER) + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - datetime.timedelta(days=7, minutes=1) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + schedule.refresh_ical_file() + + empty_shifts_report_sent_at = schedule.empty_shifts_report_sent_at + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_empty_shifts_in_schedule(schedule.pk) + + assert mock_slack_api_call.called + + schedule.refresh_from_db() + assert empty_shifts_report_sent_at != schedule.empty_shifts_report_sent_at + assert schedule.has_empty_shifts + + +@pytest.mark.django_db +def test_empty_non_empty_shifts_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") + user2 = make_user(organization=organization, username="user2", role=LegacyAccessControlRole.VIEWER) + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + # non-empty shift has higher priority + now = timezone.now().replace(microsecond=0) + start_date = now - datetime.timedelta(days=7, minutes=1) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 2, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift_1 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift_1.add_rolling_users([[user1]]) + + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift_2 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift_2.add_rolling_users([[user2]]) + schedule.refresh_ical_file() + + empty_shifts_report_sent_at = schedule.empty_shifts_report_sent_at + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_empty_shifts_in_schedule(schedule.pk) + + assert mock_slack_api_call.called + + schedule.refresh_from_db() + assert empty_shifts_report_sent_at != schedule.empty_shifts_report_sent_at + assert schedule.has_empty_shifts 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 new file mode 100644 index 00000000..7f4946c3 --- /dev/null +++ b/engine/apps/schedules/tests/test_notify_about_gaps_in_schedule.py @@ -0,0 +1,281 @@ +import datetime +from unittest.mock import patch + +import pytest +from django.utils import timezone + +from apps.schedules.ical_utils import memoized_users_in_ical +from apps.schedules.models import CustomOnCallShift, OnCallScheduleWeb +from apps.schedules.tasks import notify_about_gaps_in_schedule + + +@pytest.mark.django_db +def test_no_gaps_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=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - datetime.timedelta(days=7, minutes=1) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + schedule.refresh_ical_file() + + gaps_report_sent_at = schedule.gaps_report_sent_at + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_gaps_in_schedule(schedule.pk) + + assert not mock_slack_api_call.called + + schedule.refresh_from_db() + assert gaps_report_sent_at != schedule.gaps_report_sent_at + assert schedule.check_gaps_for_next_week() is False + + +@pytest.mark.django_db +def test_gaps_in_the_past_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=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date_1 = now - datetime.timedelta(days=1, minutes=1) + data = { + "start": start_date_1, + "rotation_start": start_date_1, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift_1 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift_1.add_rolling_users([[user1]]) + + start_date_2 = now - datetime.timedelta(days=5, minutes=1) + until_date = start_date_2 + datetime.timedelta(days=3) + data = { + "start": start_date_2, + "rotation_start": start_date_2, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + "until": until_date, + } + on_call_shift_2 = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift_2.add_rolling_users([[user1]]) + schedule.refresh_ical_file() + + gaps_report_sent_at = schedule.gaps_report_sent_at + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_gaps_in_schedule(schedule.pk) + + assert not mock_slack_api_call.called + + schedule.refresh_from_db() + assert gaps_report_sent_at != schedule.gaps_report_sent_at + assert schedule.check_gaps_for_next_week() is False + + +@pytest.mark.django_db +def test_gaps_now_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=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - datetime.timedelta(days=1, minutes=1) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + "interval": 2, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + schedule.refresh_ical_file() + + gaps_report_sent_at = schedule.gaps_report_sent_at + + assert schedule.has_gaps is False + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_gaps_in_schedule(schedule.pk) + + assert mock_slack_api_call.called + + 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 +def test_gaps_near_future_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=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + now = timezone.now().replace(microsecond=0) + start_date = now - datetime.timedelta(days=7, minutes=1) + until_date = now + datetime.timedelta(days=3) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + "until": until_date, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + schedule.refresh_ical_file() + + gaps_report_sent_at = schedule.gaps_report_sent_at + + assert schedule.has_gaps is False + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_gaps_in_schedule(schedule.pk) + + assert mock_slack_api_call.called + + 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 +def test_gaps_later_than_7_days_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() + + now = timezone.now().replace(microsecond=0) + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + 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) + data = { + "start": start_date, + "rotation_start": start_date, + "duration": datetime.timedelta(seconds=3600 * 24), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + "until": until_date, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + schedule.refresh_ical_file() + + gaps_report_sent_at = schedule.gaps_report_sent_at + + with patch("apps.slack.slack_client.SlackClientWithErrorHandling.api_call") as mock_slack_api_call: + notify_about_gaps_in_schedule(schedule.pk) + + assert not mock_slack_api_call.called + + schedule.refresh_from_db() + assert gaps_report_sent_at != schedule.gaps_report_sent_at + assert schedule.check_gaps_for_next_week() is False diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index aa478fe2..603c69ae 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -299,13 +299,13 @@ def test_filter_events_ical_all_day(make_organization, make_user_for_organizatio True, ["@Alex"], datetime.datetime(2021, 1, 27, 0, 0, tzinfo=pytz.UTC), - datetime.datetime(2021, 1, 27, 23, 59, 59, 999999, tzinfo=pytz.UTC), + datetime.datetime(2021, 1, 27, 23, 59, 59, tzinfo=pytz.UTC), ), ( True, ["@Alice"], datetime.datetime(2021, 1, 27, 0, 0, tzinfo=pytz.UTC), - datetime.datetime(2021, 1, 28, 23, 59, 59, 999999, tzinfo=pytz.UTC), + datetime.datetime(2021, 1, 28, 23, 59, 59, tzinfo=pytz.UTC), ), ( False, diff --git a/engine/apps/slack/scenarios/schedules.py b/engine/apps/slack/scenarios/schedules.py index 84306683..b470fea2 100644 --- a/engine/apps/slack/scenarios/schedules.py +++ b/engine/apps/slack/scenarios/schedules.py @@ -169,9 +169,10 @@ class EditScheduleShiftNotifyStep(scenario_step.ScenarioStep): now_text = "Inviting . No one on-call now!\n" elif schedule.notify_empty_oncall == schedule.NotifyEmptyOnCall.PREV: user_ids: typing.List[str] = [] - for item in json.loads(schedule.current_shifts).values(): - user_ids.extend(item.get("users", [])) - prev_users = organization.users.filter(id__in=user_ids) + for item in json.loads(schedule.current_shifts): + user_ids_from_shift = [u["pk"] for u in item.get("users", [])] + user_ids.extend(user_ids_from_shift) + prev_users = organization.users.filter(public_primary_key__in=user_ids) users_verbal = " ".join( [f"{user.get_username_with_slack_verbal(mention=True)}" for user in prev_users] ) @@ -183,6 +184,8 @@ class EditScheduleShiftNotifyStep(scenario_step.ScenarioStep): now_text = "" for shift in new_shifts: users = shift["users"] + user_ids_from_shift = [u["pk"] for u in users] + users = organization.users.filter(public_primary_key__in=user_ids_from_shift) now_text += cls.get_ical_shift_notification_text(shift, schedule.mention_oncall_start, users) now_text = "*New on-call shift:*\n" + now_text @@ -195,6 +198,8 @@ class EditScheduleShiftNotifyStep(scenario_step.ScenarioStep): next_text = "" for shift in next_shifts: users = shift["users"] + user_ids_from_shift = [u["pk"] for u in users] + users = organization.users.filter(public_primary_key__in=user_ids_from_shift) next_text += cls.get_ical_shift_notification_text(shift, schedule.mention_oncall_next, users) next_text = "\n*Next on-call shift:*\n" + next_text @@ -242,30 +247,29 @@ class EditScheduleShiftNotifyStep(scenario_step.ScenarioStep): @classmethod def get_ical_shift_notification_text(cls, shift, mention, users) -> str: - if shift["all_day"]: - notification = " ".join([f"{user.get_username_with_slack_verbal(mention=mention)}" for user in users]) - user_verbal = shift["users"][0].get_username_with_slack_verbal( - mention=False, - ) - if shift["start"].day == shift["end"].day: - all_day_text = shift["start"].strftime("%b %d") + notification = "" + for user in users: + if shift["all_day"]: + user_notification = user.get_username_with_slack_verbal(mention=mention) + if shift["start"].day == shift["end"].day: + all_day_text = shift["start"].strftime("%b %d") + else: + all_day_text = f'From {shift["start"].strftime("%b %d")} to {shift["end"].strftime("%b %d")}' + user_notification += f' {all_day_text} _All-day event in timezone "UTC"_\n' else: - all_day_text = f'From {shift["start"].strftime("%b %d")} to {shift["end"].strftime("%b %d")}' - notification += ( - f" {all_day_text} _All-day event in *{user_verbal}'s* timezone_ " f'- {shift["users"][0].timezone}.\n' - ) - else: - shift_start_timestamp = shift["start"].astimezone(pytz.UTC).timestamp() - shift_end_timestamp = shift["end"].astimezone(pytz.UTC).timestamp() + shift_start_timestamp = shift["start"].astimezone(pytz.UTC).timestamp() + shift_end_timestamp = shift["end"].astimezone(pytz.UTC).timestamp() - notification = ( - " ".join([f"{user.get_username_with_slack_verbal(mention=mention)}" for user in users]) - + f" from {format_datetime_to_slack_with_time(shift_start_timestamp)}" - f" to {format_datetime_to_slack_with_time(shift_end_timestamp)}\n" - ) - priority = shift.get("priority", 0) - shift.get("priority_increased_by", 0) - if priority != 0: - notification = f"[L{shift.get('priority')}] {notification}" + user_notification = ( + user.get_username_with_slack_verbal(mention=mention) + + f" from {format_datetime_to_slack_with_time(shift_start_timestamp)}" + f" to {format_datetime_to_slack_with_time(shift_end_timestamp)}\n" + ) + if not shift["is_override"]: + priority = shift.get("priority_level", 0) or 0 + if priority != 0: + user_notification = f"[L{priority}] {user_notification}" + notification += user_notification return notification From 6f3c62e05fb852f7c1a50882e1ed9d91c3f24ecc Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Thu, 3 Aug 2023 14:36:07 +0100 Subject: [PATCH 02/17] Add stack slug to organization options for direct paging (#2743) # What this PR does Adds stack slug to organization options for `/escalate` (a single Grafana org might have multiple stacks). Screenshot 2023-08-03 at 14 24 58 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- CHANGELOG.md | 4 ++++ engine/apps/slack/scenarios/paging.py | 2 +- .../slack/tests/test_scenario_steps/test_paging.py | 12 ++++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14458e77..2ba43140 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Changed + +- Add stack slug to organization options for direct paging Slash command by @vadimkerr ([#2743](https://github.com/grafana/oncall/pull/2743)) + ## v1.3.22 (2023-08-03) ### Added diff --git a/engine/apps/slack/scenarios/paging.py b/engine/apps/slack/scenarios/paging.py index 65440394..9fcff1c6 100644 --- a/engine/apps/slack/scenarios/paging.py +++ b/engine/apps/slack/scenarios/paging.py @@ -515,7 +515,7 @@ def _get_organization_select( { "text": { "type": "plain_text", - "text": f"{org.org_title}", + "text": f"{org.org_title} ({org.stack_slug})", "emoji": True, }, "value": f"{org.pk}", diff --git a/engine/apps/slack/tests/test_scenario_steps/test_paging.py b/engine/apps/slack/tests/test_scenario_steps/test_paging.py index 6e39e90b..77835980 100644 --- a/engine/apps/slack/tests/test_scenario_steps/test_paging.py +++ b/engine/apps/slack/tests/test_scenario_steps/test_paging.py @@ -24,7 +24,9 @@ from apps.slack.scenarios.paging import ( OnPagingUserChange, Policy, StartDirectPaging, + _get_organization_select, ) +from apps.user_management.models import Organization def make_slack_payload( @@ -401,3 +403,13 @@ def test_remove_schedule(make_organization_and_user_with_slack_identities, make_ metadata = json.loads(mock_slack_api_call.call_args.kwargs["view"]["private_metadata"]) assert metadata[DataKey.SCHEDULES] == {} assert metadata[DataKey.USERS] == {str(user.pk): Policy.DEFAULT} + + +@pytest.mark.django_db +def test_get_organization_select(make_organization): + organization = make_organization(org_title="Organization", stack_slug="stack_slug") + select = _get_organization_select(Organization.objects.filter(pk=organization.pk), organization, "test") + + assert len(select["element"]["options"]) == 1 + assert select["element"]["options"][0]["value"] == str(organization.pk) + assert select["element"]["options"][0]["text"]["text"] == "Organization (stack_slug)" From d5defd10d25c02629e08cbf11ec5efa28918de15 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Thu, 3 Aug 2023 15:25:37 +0100 Subject: [PATCH 03/17] Add more logging for `SlackUserGroup.update_oncall_members` (#2741) # What this PR does Adds more logging for `SlackUserGroup.update_oncall_members` so it's easier to debug. ## Which issue(s) this PR fixes Related to https://github.com/grafana/support-escalations/issues/6936 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- engine/apps/slack/models/slack_usergroup.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/engine/apps/slack/models/slack_usergroup.py b/engine/apps/slack/models/slack_usergroup.py index fcafedd9..013cfe29 100644 --- a/engine/apps/slack/models/slack_usergroup.py +++ b/engine/apps/slack/models/slack_usergroup.py @@ -80,27 +80,34 @@ class SlackUserGroup(models.Model): @property def oncall_slack_user_identities(self): users = set(user for schedule in self.oncall_schedules.get_oncall_users().values() for user in schedule) - slack_user_identities = [user.slack_user_identity for user in users if user.slack_user_identity is not None] + slack_user_identities = [] + for user in users: + if user.slack_user_identity is not None: + slack_user_identities.append(user.slack_user_identity) + else: + logger.warning(f"User {user.pk} does not have a Slack account connected") + return slack_user_identities def update_oncall_members(self): slack_ids = [slack_user_identity.slack_id for slack_user_identity in self.oncall_slack_user_identities] + logger.info(f"Updating usergroup {self.slack_id}, members {slack_ids}") # Slack doesn't allow user groups to be empty if len(slack_ids) == 0: + logger.info(f"Skipping usergroup {self.slack_id}, the list of members is empty") return # Do not send requests to Slack API in case user group is populated correctly already if self.members is not None and set(self.members) == set(slack_ids): + logger.info(f"Skipping usergroup {self.slack_id}, already populated correctly") return try: self.update_members(slack_ids) except SlackAPIException as e: if e.response["error"] == "permission_denied": - logger.warning( - "Could not update the usergroup with Slack ID: {} due to permission_denied".format(self.slack_id) - ) + logger.warning(f"Could not update usergroup {self.slack_id} due to permission_denied") def update_members(self, slack_ids): sc = SlackClientWithErrorHandling(self.slack_team_identity.bot_access_token) From f2b82f6c67456a7523950d7ca8233d788ffaebee Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 3 Aug 2023 17:50:40 +0200 Subject: [PATCH 04/17] fix make start command when using mysql/postgres as db (#2744) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # What this PR does Tested `make start` w/ both `mysql` and `postgres` in `COMPOSE_PROFILES` and things spin up properly. - Fixes some `yaml` formatting issues introduced [here](https://github.com/grafana/oncall/pull/2728/files#diff-f5d10b03472abe3719098ae8a8855468e92524ebe790c39a34d2c632f3f0486d) in #2728 which were causing the `mysql` container to fail to start up when running `make start` - Addresses #2492 by specifying `required: false` under `depends_on`, for containers in `docker-compose-developer.yml` which have container(s) which may be conditionally spun-up based on `COMPOSE_PROFILES`. Basically in v2.20.2 they introduced [this change](https://github.com/docker/compose/releases/tag/v2.20.2#:~:text=Add%20support%20of%20depends_on.required%20attribute%20by) which was a breaking change for our setup and preventing us from upgrading to >= 2.19.0. ## Which issue(s) this PR fixes Closes #2492 ‼️ with this PR you will need to make sure you are running `docker-compose` >= `v2.20.2`, otherwise 👇 ‼️ ![image](https://github.com/grafana/oncall/assets/9406895/312bdeb7-e1c5-4774-b2e9-6facddd641c1) ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- dev/README.md | 2 +- docker-compose-developer.yml | 12 +++++++++--- docker-compose-mysql-rabbitmq.yml | 7 +++---- docker-compose.yml | 3 +-- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/dev/README.md b/dev/README.md index 99edd275..6e65d2a5 100644 --- a/dev/README.md +++ b/dev/README.md @@ -35,7 +35,7 @@ environment variable. **NOTE**: the `docker-compose-developer.yml` file uses some syntax/features that are only supported by Docker Compose v2. For instructions on how to enable this (if you haven't already done so), see [here](https://www.docker.com/blog/announcing-compose-v2-general-availability/). Ensure you have Docker Compose - version 2.10 or above installed - update instructions are [here](https://docs.docker.com/compose/install/linux/). + version 2.20.2 or above installed - update instructions are [here](https://docs.docker.com/compose/install/linux/). 2. Run `make init start`. By default this will run everything in Docker, using SQLite as the database and Redis as the message broker/cache. See [`COMPOSE_PROFILES`](#compose_profiles) below for more details on how to swap out/disable which components are run in Docker. diff --git a/docker-compose-developer.yml b/docker-compose-developer.yml index ca9adae4..0fc4df5b 100644 --- a/docker-compose-developer.yml +++ b/docker-compose-developer.yml @@ -144,12 +144,16 @@ services: depends_on: postgres: condition: service_healthy + required: false mysql: condition: service_healthy + required: false rabbitmq: condition: service_healthy + required: false redis: condition: service_healthy + required: false profiles: - engine @@ -208,7 +212,7 @@ services: container_name: mysql labels: *oncall-labels image: mysql:8.0.32 - command: > + command: >- --default-authentication-plugin=mysql_native_password --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci --max_connections=1024 restart: always @@ -236,7 +240,7 @@ services: container_name: mysql_to_create_grafana_db labels: *oncall-labels image: mysql:8.0.32 - command: > + command: >- bash -c "mysql -h mysql -uroot -pempty -e 'CREATE DATABASE IF NOT EXISTS grafana CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;'" depends_on: @@ -276,7 +280,7 @@ services: container_name: postgres_to_create_grafana_db labels: *oncall-labels image: postgres:14.4 - command: > + command: >- bash -c "PGPASSWORD=empty psql -U postgres -h postgres -tc \"SELECT 1 FROM pg_database WHERE datname = 'grafana'\" | grep -q 1 || PGPASSWORD=empty psql -U postgres -h postgres -c \"CREATE DATABASE grafana\"" @@ -326,8 +330,10 @@ services: depends_on: postgres: condition: service_healthy + required: false mysql: condition: service_healthy + required: false profiles: - grafana volumes: diff --git a/docker-compose-mysql-rabbitmq.yml b/docker-compose-mysql-rabbitmq.yml index 77d972ae..5cf2b472 100644 --- a/docker-compose-mysql-rabbitmq.yml +++ b/docker-compose-mysql-rabbitmq.yml @@ -28,8 +28,7 @@ services: restart: always ports: - "8080:8080" - command: > - sh -c "uwsgi --ini uwsgi.ini" + command: sh -c "uwsgi --ini uwsgi.ini" environment: *oncall-environment depends_on: mysql: @@ -68,7 +67,7 @@ services: mysql: image: mysql:8.0.32 - command: > + command: >- --default-authentication-plugin=mysql_native_password --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci restart: always @@ -123,7 +122,7 @@ services: mysql_to_create_grafana_db: image: mysql:8.0.32 - command: > + command: >- bash -c "mysql -h ${MYSQL_HOST:-mysql} -uroot -p${MYSQL_PASSWORD:?err} -e 'CREATE DATABASE IF NOT EXISTS grafana CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;'" depends_on: diff --git a/docker-compose.yml b/docker-compose.yml index 5ceb8077..2dadfc3a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -22,8 +22,7 @@ services: restart: always ports: - "8080:8080" - command: > - sh -c "uwsgi --ini uwsgi.ini" + command: sh -c "uwsgi --ini uwsgi.ini" environment: *oncall-environment volumes: - oncall_data:/var/lib/oncall From 62f993c9c9c000e851fd20d41e16e56101bb2c39 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Fri, 4 Aug 2023 10:38:21 +0100 Subject: [PATCH 05/17] Enable push notifications for SSR beneficiary for testing purposes (#2752) --- engine/apps/schedules/models/shift_swap_request.py | 4 +++- engine/apps/schedules/tests/test_shift_swap_request.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index ebf48811..5e2de394 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -157,7 +157,9 @@ class ShiftSwapRequest(models.Model): @property def possible_benefactors(self) -> QuerySet["User"]: - return self.schedule.related_users().exclude(pk=self.beneficiary_id) + # TODO: exclude the beneficiary from this list + # Temporarily including the beneficiary in the list of possible benefactors for testing purposes + return self.schedule.related_users() @property def web_link(self) -> str: diff --git a/engine/apps/schedules/tests/test_shift_swap_request.py b/engine/apps/schedules/tests/test_shift_swap_request.py index a6735484..685959c0 100644 --- a/engine/apps/schedules/tests/test_shift_swap_request.py +++ b/engine/apps/schedules/tests/test_shift_swap_request.py @@ -166,4 +166,6 @@ def test_possible_benefactors(shift_swap_request_setup) -> None: with patch.object(ssr.schedule, "related_users") as mock_related_users: mock_related_users.return_value = User.objects.filter(pk__in=[beneficiary.pk, benefactor.pk]) - assert list(ssr.possible_benefactors) == [benefactor] + # TODO: exclude the beneficiary from this list + # Temporarily including the beneficiary in the list of possible benefactors for testing purposes + assert set(ssr.possible_benefactors) == {benefactor, beneficiary} From d0c535760073c95959acce4c8004286a015ad9ed Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Fri, 4 Aug 2023 09:09:12 -0300 Subject: [PATCH 06/17] Do not show override shortcut when web overrides are disabled (#2745) Fixes https://github.com/grafana/oncall/issues/2387 --- CHANGELOG.md | 4 ++++ .../src/containers/ScheduleSlot/ScheduleSlot.tsx | 7 +++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ba43140..66a74f1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add stack slug to organization options for direct paging Slash command by @vadimkerr ([#2743](https://github.com/grafana/oncall/pull/2743)) +### Fixed + +- Do not show override shortcut when web overrides are disabled ([#2745](https://github.com/grafana/oncall/pull/2745)) + ## v1.3.22 (2023-08-03) ### Added diff --git a/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx b/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx index 04c741a4..65661bcb 100644 --- a/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx +++ b/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx @@ -108,6 +108,7 @@ const ScheduleSlot: FC = observer((props) => { isOncall={isOncall} currentTimezone={currentTimezone} event={event} + scheduleId={scheduleId} handleAddOverride={handleAddOverride} simplified={simplified} color={color} @@ -130,13 +131,14 @@ interface ScheduleSlotDetailsProps { isOncall: boolean; currentTimezone: Timezone; event: Event; + scheduleId: Schedule['id']; handleAddOverride: (event: React.SyntheticEvent) => void; simplified?: boolean; color: string; } const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { - const { user, currentTimezone, event, handleAddOverride, simplified, color } = props; + const { user, currentTimezone, event, scheduleId, handleAddOverride, simplified, color } = props; const store = useStore(); const { scheduleStore } = store; @@ -144,6 +146,7 @@ const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { const currentMoment = useMemo(() => dayjs(), []); const shift = scheduleStore.shifts[event.shift?.pk]; + const enableWebOverrides = scheduleStore.items[scheduleId]?.enable_web_overrides; return (
@@ -199,7 +202,7 @@ const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { {dayjs(event.end).tz(currentTimezone).format('DD MMM, HH:mm')} - {!simplified && !event.is_override && ( + {!simplified && !event.is_override && enableWebOverrides && ( + ) : ( + + )} + + + + +
+ + ); +}; + +export default ShiftSwapForm; diff --git a/grafana-plugin/src/containers/RotationForm/parts/UserItem.tsx b/grafana-plugin/src/containers/RotationForm/parts/UserItem.tsx index 41054531..dc1a0601 100644 --- a/grafana-plugin/src/containers/RotationForm/parts/UserItem.tsx +++ b/grafana-plugin/src/containers/RotationForm/parts/UserItem.tsx @@ -37,7 +37,7 @@ const UserItem = ({ pk, shiftColor, shiftStart, shiftEnd }: UserItemProps) => { const duration = dayjs(shiftEnd).diff(dayjs(shiftStart), 'seconds'); return ( -
+
{duration <= WEEK_IN_SECONDS && ( { let color = undefined; @@ -37,3 +40,35 @@ export const findColor = (shiftId: Shift['id'], layers: Layer[], overrides?) => return color; }; + +export const findClosestUserEvent = (startMoment: dayjs.Dayjs, userPk: User['pk'], layers: Layer[]) => { + let minDiff; + let closestEvent; + + if (!layers) { + return undefined; + } + + for (let i = 0; i < layers.length; i++) { + for (let j = 0; j < layers[i].shifts.length; j++) { + const shift = layers[i].shifts[j]; + const events = shift.events; + for (let k = 0; k < events.length; k++) { + const event = events[k]; + const diff = dayjs(event.start).diff(startMoment, 'seconds'); + + if ( + event.users.some((user) => user.pk === userPk) && + !event.users.some((user) => user.swap_request) && + diff > 0 && + (minDiff === undefined || diff < minDiff) + ) { + minDiff = diff; + closestEvent = event; + } + } + } + } + + return closestEvent; +}; diff --git a/grafana-plugin/src/containers/Rotations/Rotations.tsx b/grafana-plugin/src/containers/Rotations/Rotations.tsx index a2262ae8..c1a3725f 100644 --- a/grafana-plugin/src/containers/Rotations/Rotations.tsx +++ b/grafana-plugin/src/containers/Rotations/Rotations.tsx @@ -14,14 +14,16 @@ import Rotation from 'containers/Rotation/Rotation'; import RotationForm from 'containers/RotationForm/RotationForm'; import { WithPermissionControlTooltip } from 'containers/WithPermissionControl/WithPermissionControlTooltip'; import { getColor, getLayersFromStore } from 'models/schedule/schedule.helpers'; -import { Layer, Schedule, ScheduleType, Shift } from 'models/schedule/schedule.types'; +import { Layer, Schedule, ScheduleType, Shift, ShiftSwap, Event } from 'models/schedule/schedule.types'; import { Timezone } from 'models/timezone/timezone.types'; +import { User } from 'models/user/user.types'; +import { getUTCString } from 'pages/schedule/Schedule.helpers'; import { WithStoreProps } from 'state/types'; import { withMobXProviderContext } from 'state/withStore'; import { UserActions } from 'utils/authorization'; import { DEFAULT_TRANSITION_TIMEOUT } from './Rotations.config'; -import { findColor } from './Rotations.helpers'; +import { findClosestUserEvent, findColor } from './Rotations.helpers'; import styles from './Rotations.module.css'; @@ -35,11 +37,14 @@ interface RotationsProps extends WithStoreProps { onShowRotationForm: (shiftId: Shift['id'] | 'new') => void; onClick: (id: Shift['id'] | 'new') => void; onShowOverrideForm: (shiftId: 'new', shiftStart: dayjs.Dayjs, shiftEnd: dayjs.Dayjs) => void; + onShowShiftSwapForm: (id: ShiftSwap['id'] | 'new', params?: Partial) => void; onCreate: () => void; onUpdate: () => void; onDelete: () => void; + onShiftSwapRequest: (beneficiary: User['pk'], swap_start: string, swap_end: string) => void; disabled: boolean; filters: ScheduleFiltersType; + onSlotClick?: (event: Event) => void; } interface RotationsState { @@ -68,6 +73,11 @@ class Rotations extends Component { shiftIdToShowRotationForm, disabled, filters, + onShowShiftSwapForm, + onSlotClick, + store: { + userStore: { currentUserPk }, + }, } = this.props; const { layerPriority, shiftStartToShowRotationForm, shiftEndToShowRotationForm } = this.state; @@ -104,35 +114,55 @@ class Rotations extends Component { Rotations
- {disabled ? ( - isTypeReadOnly ? ( - -
+ + + {disabled ? ( + isTypeReadOnly ? ( + +
+ +
+ + ) : ( + -
- + + ) + ) : options.length > 0 ? ( + ) : ( - - - - ) - ) : options.length > 0 ? ( - - ) : ( - - )} + + )} +
@@ -164,6 +194,8 @@ class Rotations extends Component { this.onRotationClick(shiftId, shiftStart, shiftEnd); }} handleAddOverride={this.handleShowOverrideForm} + handleAddShiftSwap={onShowShiftSwapForm} + onShiftSwapClick={onShowShiftSwapForm} color={getColor(layerIndex, rotationIndex)} events={events} layerIndex={layerIndex} @@ -173,6 +205,7 @@ class Rotations extends Component { transparent={isPreview} tutorialParams={isPreview && store.scheduleStore.rotationFormLiveParams} filters={filters} + onSlotClick={onSlotClick} /> ))} diff --git a/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx b/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx index 502eb572..f7b90a2c 100644 --- a/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx +++ b/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx @@ -11,7 +11,7 @@ import Text from 'components/Text/Text'; import TimelineMarks from 'components/TimelineMarks/TimelineMarks'; import Rotation from 'containers/Rotation/Rotation'; import { getLayersFromStore, getOverridesFromStore, getShiftsFromStore } from 'models/schedule/schedule.helpers'; -import { Schedule, Shift } from 'models/schedule/schedule.types'; +import { Schedule, Shift, ShiftSwap, Event } from 'models/schedule/schedule.types'; import { Timezone } from 'models/timezone/timezone.types'; import { WithStoreProps } from 'state/types'; import { withMobXProviderContext } from 'state/withStore'; @@ -30,8 +30,10 @@ interface ScheduleFinalProps extends WithStoreProps { simplified?: boolean; onClick: (shiftId: Shift['id']) => void; onShowOverrideForm: (shiftId: 'new', shiftStart: dayjs.Dayjs, shiftEnd: dayjs.Dayjs) => void; + onShowShiftSwapForm: (id: ShiftSwap['id'] | 'new', params?: Partial) => void; disabled?: boolean; filters: ScheduleFiltersType; + onSlotClick?: (event: Event) => void; } interface ScheduleOverridesState { @@ -45,7 +47,8 @@ class ScheduleFinal extends Component ); diff --git a/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx b/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx index 65661bcb..143ad043 100644 --- a/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx +++ b/grafana-plugin/src/containers/ScheduleSlot/ScheduleSlot.tsx @@ -1,4 +1,4 @@ -import React, { FC, useMemo } from 'react'; +import React, { FC, useCallback, useMemo } from 'react'; import { Button, HorizontalGroup, Icon, Tooltip, VerticalGroup } from '@grafana/ui'; import cn from 'classnames/bind'; @@ -8,8 +8,8 @@ import { observer } from 'mobx-react'; import { ScheduleFiltersType } from 'components/ScheduleFilters/ScheduleFilters.types'; import Text from 'components/Text/Text'; import WorkingHours from 'components/WorkingHours/WorkingHours'; -import { getShiftName } from 'models/schedule/schedule.helpers'; -import { Event, Schedule } from 'models/schedule/schedule.types'; +import { getShiftName, SHIFT_SWAP_COLOR } from 'models/schedule/schedule.helpers'; +import { Event, Schedule, ShiftSwap } from 'models/schedule/schedule.types'; import { getTzOffsetString } from 'models/timezone/timezone.helpers'; import { Timezone } from 'models/timezone/timezone.types'; import { User } from 'models/user/user.types'; @@ -25,17 +25,39 @@ interface ScheduleSlotProps { startMoment: dayjs.Dayjs; currentTimezone: Timezone; handleAddOverride: (event: React.MouseEvent) => void; + handleAddShiftSwap: (event: React.MouseEvent) => void; + onShiftSwapClick: (id: ShiftSwap['id']) => void; color?: string; simplified?: boolean; filters?: ScheduleFiltersType; + onClick: (event: React.MouseEvent) => void; } const cx = cn.bind(styles); const ScheduleSlot: FC = observer((props) => { - const { event, scheduleId, currentTimezone, color, handleAddOverride, simplified, filters } = props; + const { + event, + scheduleId, + currentTimezone, + color, + handleAddOverride, + handleAddShiftSwap, + onShiftSwapClick, + simplified, + filters, + onClick, + } = props; const { users } = event; + const getShiftSwapClickHandler = useCallback((swapId: ShiftSwap['id']) => { + return (event: React.MouseEvent) => { + event.stopPropagation(); + + onShiftSwapClick(swapId); + }; + }, []); + const start = dayjs(event.start); const end = dayjs(event.end); @@ -49,8 +71,10 @@ const ScheduleSlot: FC = observer((props) => { const onCallNow = store.scheduleStore.items[scheduleId]?.on_call_now; + const enableWebOverrides = store.scheduleStore.items[scheduleId]?.enable_web_overrides; + return ( -
+
{event.is_gap ? ( }>
@@ -63,9 +87,10 @@ const ScheduleSlot: FC = observer((props) => { }} /> ) : ( - users.map(({ display_name, pk: userPk }) => { + users.map(({ display_name, pk: userPk, swap_request }) => { const storeUser = store.userStore.items[userPk]; + const isCurrentUserSlot = userPk === store.userStore.currentUserPk; const inactive = filters && filters.users.length && !filters.users.includes(userPk); const title = storeUser ? getTitle(storeUser) : display_name; @@ -74,14 +99,22 @@ const ScheduleSlot: FC = observer((props) => { storeUser && onCallNow && onCallNow.some((onCallUser) => storeUser.pk === onCallUser.pk) ); + const isShiftSwap = Boolean(swap_request); + + let backgroundColor = color; + if (isShiftSwap) { + backgroundColor = SHIFT_SWAP_COLOR; + } + const scheduleSlotContent = (
- {storeUser && ( + {storeUser && (!swap_request || swap_request.user) && ( = observer((props) => { duration={duration} /> )} -
{title}
+
+ {swap_request && !swap_request.user ? : title} +
); @@ -104,14 +139,23 @@ const ScheduleSlot: FC = observer((props) => { key={userPk} content={ } > @@ -131,14 +175,27 @@ interface ScheduleSlotDetailsProps { isOncall: boolean; currentTimezone: Timezone; event: Event; - scheduleId: Schedule['id']; handleAddOverride: (event: React.SyntheticEvent) => void; + handleAddShiftSwap: (event: React.SyntheticEvent) => void; simplified?: boolean; color: string; + isShiftSwap?: boolean; + beneficiaryName?: string; + benefactorName?: string; } const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { - const { user, currentTimezone, event, scheduleId, handleAddOverride, simplified, color } = props; + const { + user, + currentTimezone, + event, + handleAddOverride, + handleAddShiftSwap, + color, + isShiftSwap, + beneficiaryName, + benefactorName, + } = props; const store = useStore(); const { scheduleStore } = store; @@ -146,7 +203,6 @@ const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { const currentMoment = useMemo(() => dayjs(), []); const shift = scheduleStore.shifts[event.shift?.pk]; - const enableWebOverrides = scheduleStore.items[scheduleId]?.enable_web_overrides; return (
@@ -156,16 +212,34 @@ const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => {
- {getShiftName(shift)} + {isShiftSwap ? 'Shift swap' : getShiftName(shift)}
- +
- - {user?.username} - + {isShiftSwap ? ( + + Swap pair + + {beneficiaryName} (creator) + + {benefactorName ? ( + + {benefactorName} (taken by) + + ) : ( + + Not taken yet + + )} + + ) : ( + + {user?.username} + + )}
@@ -202,13 +276,18 @@ const ScheduleSlotDetails = (props: ScheduleSlotDetailsProps) => { {dayjs(event.end).tz(currentTimezone).format('DD MMM, HH:mm')} - {!simplified && !event.is_override && enableWebOverrides && ( - + + {handleAddShiftSwap && ( + + )} + {handleAddOverride && ( - - )} + )} +
); diff --git a/grafana-plugin/src/containers/UsersTimezones/UsersTimezones.tsx b/grafana-plugin/src/containers/UsersTimezones/UsersTimezones.tsx index 2b79241a..53ffe433 100644 --- a/grafana-plugin/src/containers/UsersTimezones/UsersTimezones.tsx +++ b/grafana-plugin/src/containers/UsersTimezones/UsersTimezones.tsx @@ -80,8 +80,8 @@ const UsersTimezones: FC = (props) => { light startMoment={currentMoment.startOf('day')} duration={24 * 60 * 60} - timezone={userStore.currentUser.timezone} - workingHours={userStore.currentUser.working_hours} + timezone={userStore.currentUser?.timezone} + workingHours={userStore.currentUser?.working_hours} className={cx('working-hours')} /> {/*
diff --git a/grafana-plugin/src/models/schedule/schedule.helpers.ts b/grafana-plugin/src/models/schedule/schedule.helpers.ts index 91a52043..21d4e396 100644 --- a/grafana-plugin/src/models/schedule/schedule.helpers.ts +++ b/grafana-plugin/src/models/schedule/schedule.helpers.ts @@ -204,6 +204,8 @@ const L3_COLORS = ['#377277', '#638282', '#364E4E', '#423220']; const OVERRIDE_COLORS = ['#C69B06', '#C2C837']; +export const SHIFT_SWAP_COLOR = '#C69B06'; + const COLORS = [L1_COLORS, L2_COLORS, L3_COLORS]; export const getColor = (layerIndex: number, rotationIndex: number) => { diff --git a/grafana-plugin/src/models/schedule/schedule.ts b/grafana-plugin/src/models/schedule/schedule.ts index 4071088f..392e2cf3 100644 --- a/grafana-plugin/src/models/schedule/schedule.ts +++ b/grafana-plugin/src/models/schedule/schedule.ts @@ -26,6 +26,7 @@ import { ShiftEvents, RotationFormLiveParams, ScheduleScoreQualityResponse, + ShiftSwap, } from './schedule.types'; export class ScheduleStore extends BaseStore { @@ -44,6 +45,9 @@ export class ScheduleStore extends BaseStore { @observable.shallow relatedUsers: { [id: string]: { [key: string]: Event } } = {}; + @observable.shallow + shiftSwaps: { [id: string]: ShiftSwap } = {}; + @observable.shallow rotations: { [id: string]: { @@ -432,4 +436,24 @@ export class ScheduleStore extends BaseStore { method: 'GET', }); } + + async createShiftSwap(params: Partial) { + return await makeRequest(`/shift_swaps/`, { method: 'POST', data: params }).catch(this.onApiError); + } + + async deleteShiftSwap(shiftSwapId: ShiftSwap['id']) { + return await makeRequest(`/shift_swaps/${shiftSwapId}`, { method: 'DELETE' }).catch(this.onApiError); + } + + async takeShiftSwap(shiftSwapId: ShiftSwap['id']) { + return await makeRequest(`/shift_swaps/${shiftSwapId}/take`, { method: 'POST' }).catch(this.onApiError); + } + + async loadShiftSwap(id: ShiftSwap['id']) { + const result = await makeRequest(`/shift_swaps/${id}`, {}); + + this.shiftSwaps = { ...this.shiftSwaps, [id]: result }; + + return result; + } } diff --git a/grafana-plugin/src/models/schedule/schedule.types.ts b/grafana-plugin/src/models/schedule/schedule.types.ts index 80b2d417..9107e28b 100644 --- a/grafana-plugin/src/models/schedule/schedule.types.ts +++ b/grafana-plugin/src/models/schedule/schedule.types.ts @@ -81,6 +81,11 @@ export interface Rotation { export type RotationType = 'final' | 'rotation' | 'override'; +export interface SwapRequest { + pk: ShiftSwap['id']; + user: Partial & { display_name: string }; +} + export interface Event { all_day: boolean; calendar_type: ScheduleType; @@ -92,7 +97,11 @@ export interface Event { shift: { pk: Shift['id'] | null }; source: string; start: string; - users: Array<{ display_name: User['username']; pk: User['pk'] }>; + users: Array<{ + display_name: User['username']; + pk: User['pk']; + swap_request?: SwapRequest; + }>; is_override: boolean; } @@ -120,6 +129,19 @@ export interface ScheduleScoreQualityResponse { overloaded_users: Array<{ id: string; username: string; score: number }>; } +export interface ShiftSwap { + id: string; + created_at: string; + updated_at: string; + schedule: Schedule['id']; + swap_start: string; + swap_end: string; + beneficiary: User['pk']; + status: 'open' | 'taken' | 'past_due'; + benefactor: User['pk']; + description: string; +} + export enum ScheduleScoreQualityResult { Bad = 'Bad', Low = 'Low', diff --git a/grafana-plugin/src/pages/schedule/Schedule.tsx b/grafana-plugin/src/pages/schedule/Schedule.tsx index 4103a0ee..9b3e297d 100644 --- a/grafana-plugin/src/pages/schedule/Schedule.tsx +++ b/grafana-plugin/src/pages/schedule/Schedule.tsx @@ -27,6 +27,7 @@ import ScheduleQuality from 'components/ScheduleQuality/ScheduleQuality'; import Text from 'components/Text/Text'; import UserTimezoneSelect from 'components/UserTimezoneSelect/UserTimezoneSelect'; import WithConfirm from 'components/WithConfirm/WithConfirm'; +import ShiftSwapForm from 'containers/RotationForm/ShiftSwapForm'; import Rotations from 'containers/Rotations/Rotations'; import ScheduleFinal from 'containers/Rotations/ScheduleFinal'; import ScheduleOverrides from 'containers/Rotations/ScheduleOverrides'; @@ -34,7 +35,7 @@ import ScheduleForm from 'containers/ScheduleForm/ScheduleForm'; import ScheduleICalSettings from 'containers/ScheduleIcalLink/ScheduleIcalLink'; import UsersTimezones from 'containers/UsersTimezones/UsersTimezones'; import { WithPermissionControlTooltip } from 'containers/WithPermissionControl/WithPermissionControlTooltip'; -import { Schedule, ScheduleType, Shift } from 'models/schedule/schedule.types'; +import { Event, Schedule, ScheduleType, Shift, ShiftSwap } from 'models/schedule/schedule.types'; import { Timezone } from 'models/timezone/timezone.types'; import { PageProps, WithStoreProps } from 'state/types'; import { withMobXProviderContext } from 'state/withStore'; @@ -64,10 +65,14 @@ interface SchedulePageState extends PageBaseState { showScheduleICalSettings: boolean; lastUpdated: number; filters: ScheduleFiltersType; + shiftSwapIdToShowForm?: ShiftSwap['id'] | 'new'; + shiftSwapParamsToShowForm?: Partial; } @observer class SchedulePage extends React.Component { + highlightMyShiftsWasToggled = false; + constructor(props: SchedulePageProps) { super(props); @@ -132,6 +137,8 @@ class SchedulePage extends React.Component shiftStartToShowOverrideForm, shiftEndToShowOverrideForm, filters, + shiftSwapIdToShowForm, + shiftSwapParamsToShowForm, } = this.state; const { isNotFoundError } = errorData; @@ -280,6 +287,8 @@ class SchedulePage extends React.Component disabled={disabledRotationForm} onShowOverrideForm={this.handleShowOverridesForm} filters={filters} + onShowShiftSwapForm={this.handleShowShiftSwapForm} + onSlotClick={shiftSwapIdToShowForm ? this.onSlotClick : undefined} /> onShowOverrideForm={this.handleShowOverridesForm} disabled={disabledRotationForm} filters={filters} + onShowShiftSwapForm={this.handleShowShiftSwapForm} + onSlotClick={shiftSwapIdToShowForm ? this.onSlotClick : undefined} /> )} + {shiftSwapIdToShowForm && ( + + )} )} @@ -537,6 +558,52 @@ class SchedulePage extends React.Component store.scheduleStore.delete(id).then(() => history.replace(`${PLUGIN_ROOT}/schedules`)); }; + + handleShowShiftSwapForm = (id: ShiftSwap['id'], params: Partial) => { + const { filters } = this.state; + + const { + store: { userStore }, + } = this.props; + + if (!filters.users.includes(userStore.currentUserPk)) { + this.setState({ filters: { ...filters, users: [...this.state.filters.users, userStore.currentUserPk] } }); + this.highlightMyShiftsWasToggled = true; + } + + this.setState({ shiftSwapIdToShowForm: id, shiftSwapParamsToShowForm: params }); + }; + + handleHideShiftSwapForm = () => { + const { filters } = this.state; + + const { + store: { userStore }, + } = this.props; + + if (this.highlightMyShiftsWasToggled) { + this.highlightMyShiftsWasToggled = false; + const index = filters.users.indexOf(userStore.currentUserPk); + + if (index > -1) { + const newUsers = [...filters.users]; + newUsers.splice(index, 1); + + this.setState({ filters: { ...filters, users: newUsers } }); + } + } + this.setState({ shiftSwapIdToShowForm: undefined, shiftSwapParamsToShowForm: undefined }); + }; + + onSlotClick = (event: Event) => { + this.setState({ + shiftSwapParamsToShowForm: { + ...this.state.shiftSwapParamsToShowForm, + swap_start: event.start, + swap_end: event.end, + }, + }); + }; } export default withRouter(withMobXProviderContext(SchedulePage)); From ceeb3b8f5b5c65c619435d95b1425dec315891fb Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 4 Aug 2023 16:12:33 +0200 Subject: [PATCH 08/17] remove shift swap feature flag (#2755) --- engine/apps/api/urls.py | 5 +---- engine/apps/mobile_app/tasks.py | 4 ---- .../tests/test_shift_swap_request.py | 21 ++----------------- engine/settings/base.py | 1 - engine/settings/ci-test.py | 2 -- 5 files changed, 3 insertions(+), 30 deletions(-) diff --git a/engine/apps/api/urls.py b/engine/apps/api/urls.py index 164b2e12..d5f3dc43 100644 --- a/engine/apps/api/urls.py +++ b/engine/apps/api/urls.py @@ -1,4 +1,3 @@ -from django.conf import settings from django.urls import include, path, re_path from common.api_helpers.optional_slash_router import OptionalSlashRouter, optional_slash_path @@ -66,9 +65,7 @@ router.register(r"heartbeats", IntegrationHeartBeatView, basename="integration_h router.register(r"tokens", PublicApiTokenView, basename="api_token") router.register(r"live_settings", LiveSettingViewSet, basename="live_settings") router.register(r"oncall_shifts", OnCallShiftView, basename="oncall_shifts") - -if settings.FEATURE_SHIFT_SWAPS_ENABLED: - router.register(r"shift_swaps", ShiftSwapViewSet, basename="shift_swap") +router.register(r"shift_swaps", ShiftSwapViewSet, basename="shift_swap") urlpatterns = [ path("", include(router.urls)), diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index df619e2e..456cbe45 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -518,10 +518,6 @@ def notify_shift_swap_requests() -> None: """ A periodic task that notifies users about shift swap requests. """ - - if not settings.FEATURE_SHIFT_SWAPS_ENABLED: - return - for shift_swap_request in _get_shift_swap_requests_to_notify(timezone.now()): notify_shift_swap_request.delay(shift_swap_request.pk) diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 46d8b485..9d763713 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -108,9 +108,7 @@ def test_get_shift_swap_requests_to_notify_starts_not_soon( @pytest.mark.django_db -def test_notify_shift_swap_requests(make_organization, make_user, make_schedule, make_shift_swap_request, settings): - settings.FEATURE_SHIFT_SWAPS_ENABLED = True - +def test_notify_shift_swap_requests(make_organization, make_user, make_schedule, make_shift_swap_request): organization = make_organization() user = make_user(organization=organization) schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) @@ -134,17 +132,6 @@ def test_notify_shift_swap_requests(make_organization, make_user, make_schedule, mock_notify_shift_swap_request.assert_called_once_with(shift_swap_request.pk) -@pytest.mark.django_db -def test_notify_shift_swap_requests_feature_flag_disabled( - make_organization, make_user, make_schedule, make_shift_swap_request, settings -): - settings.FEATURE_SHIFT_SWAPS_ENABLED = False - with patch("apps.mobile_app.tasks._get_shift_swap_requests_to_notify") as mock_get_shift_swap_requests_to_notify: - notify_shift_swap_requests() - - mock_get_shift_swap_requests_to_notify.assert_not_called() - - @pytest.mark.django_db def test_notify_shift_swap_request(make_organization, make_user, make_schedule, make_shift_swap_request, settings): organization = make_organization() @@ -250,11 +237,7 @@ def test_notify_shift_swap_request_success( @pytest.mark.django_db -def test_notify_user_about_shift_swap_request( - make_organization, make_user, make_schedule, make_shift_swap_request, settings -): - settings.FEATURE_SHIFT_SWAPS_ENABLED = True - +def test_notify_user_about_shift_swap_request(make_organization, make_user, make_schedule, make_shift_swap_request): organization = make_organization() beneficiary = make_user(organization=organization, name="John Doe", username="john.doe") benefactor = make_user(organization=organization) diff --git a/engine/settings/base.py b/engine/settings/base.py index 29f08f38..0d2bc46a 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -64,7 +64,6 @@ FEATURE_SLACK_INTEGRATION_ENABLED = getenv_boolean("FEATURE_SLACK_INTEGRATION_EN FEATURE_MULTIREGION_ENABLED = getenv_boolean("FEATURE_MULTIREGION_ENABLED", default=False) FEATURE_INBOUND_EMAIL_ENABLED = getenv_boolean("FEATURE_INBOUND_EMAIL_ENABLED", default=False) FEATURE_PROMETHEUS_EXPORTER_ENABLED = getenv_boolean("FEATURE_PROMETHEUS_EXPORTER_ENABLED", default=False) -FEATURE_SHIFT_SWAPS_ENABLED = getenv_boolean("FEATURE_SHIFT_SWAPS_ENABLED", default=False) FEATURE_GRAFANA_ALERTING_V2_ENABLED = getenv_boolean("FEATURE_GRAFANA_ALERTING_V2_ENABLED", default=False) GRAFANA_CLOUD_ONCALL_HEARTBEAT_ENABLED = getenv_boolean("GRAFANA_CLOUD_ONCALL_HEARTBEAT_ENABLED", default=True) GRAFANA_CLOUD_NOTIFICATIONS_ENABLED = getenv_boolean("GRAFANA_CLOUD_NOTIFICATIONS_ENABLED", default=True) diff --git a/engine/settings/ci-test.py b/engine/settings/ci-test.py index 21fc9216..9751c49d 100644 --- a/engine/settings/ci-test.py +++ b/engine/settings/ci-test.py @@ -40,5 +40,3 @@ TWILIO_ACCOUNT_SID = "dummy_twilio_account_sid" TWILIO_AUTH_TOKEN = "dummy_twilio_auth_token" EXTRA_MESSAGING_BACKENDS = [("apps.base.tests.messaging_backend.TestOnlyBackend", 42)] - -FEATURE_SHIFT_SWAPS_ENABLED = True From 84122825690c0b68b08bc043def45dfe6b15667d Mon Sep 17 00:00:00 2001 From: Maxim Mordasov Date: Fri, 4 Aug 2023 17:46:54 +0300 Subject: [PATCH 09/17] merge final schedule in as less lines as possible (#2649) # What this PR does Merge final schedule in less rows ## Which issue(s) this PR fixes [Final schedule shifts should lay in one line](https://github.com/grafana/oncall/issues/1665) ## 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) --- CHANGELOG.md | 1 + .../src/containers/Rotation/Rotation.tsx | 10 +- .../containers/Rotations/ScheduleFinal.tsx | 28 ++-- .../src/models/schedule/schedule.helpers.ts | 146 ++++++++++++++++-- .../src/models/schedule/schedule.types.ts | 1 + .../src/pages/schedule/Schedule.tsx | 13 +- 6 files changed, 160 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d112992..57067135 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Shift Swap Requests Web UI ([#2593](https://github.com/grafana/oncall/issues/2593)) +- Final schedule shifts should lay in one line [1665](https://github.com/grafana/oncall/issues/1665) ### Changed diff --git a/grafana-plugin/src/containers/Rotation/Rotation.tsx b/grafana-plugin/src/containers/Rotation/Rotation.tsx index 6fa5836f..486c988f 100644 --- a/grafana-plugin/src/containers/Rotation/Rotation.tsx +++ b/grafana-plugin/src/containers/Rotation/Rotation.tsx @@ -7,7 +7,7 @@ import hash from 'object-hash'; import { ScheduleFiltersType } from 'components/ScheduleFilters/ScheduleFilters.types'; import ScheduleSlot from 'containers/ScheduleSlot/ScheduleSlot'; -import { Schedule, Event, RotationFormLiveParams, ShiftSwap } from 'models/schedule/schedule.types'; +import { Schedule, Event, RotationFormLiveParams, Shift, ShiftSwap } from 'models/schedule/schedule.types'; import { Timezone } from 'models/timezone/timezone.types'; import RotationTutorial from './RotationTutorial'; @@ -33,6 +33,7 @@ interface RotationProps { tutorialParams?: RotationFormLiveParams; simplified?: boolean; filters?: ScheduleFiltersType; + getColor?: (shiftId: Shift['id']) => string; onSlotClick?: (event: Event) => void; } @@ -42,7 +43,7 @@ const Rotation: FC = (props) => { scheduleId, startMoment, currentTimezone, - color, + color: propsColor, days = 7, transparent = false, tutorialParams, @@ -52,6 +53,7 @@ const Rotation: FC = (props) => { onShiftSwapClick, simplified, filters, + getColor, onSlotClick, } = props; @@ -113,7 +115,7 @@ const Rotation: FC = (props) => { }, [events]); return ( -
+
{tutorialParams && } {events ? ( @@ -130,7 +132,7 @@ const Rotation: FC = (props) => { event={event} startMoment={startMoment} currentTimezone={currentTimezone} - color={color} + color={propsColor || getColor(event.shift?.pk)} handleAddOverride={getAddOverrideClickHandler(event)} handleAddShiftSwap={getAddShiftSwapClickHandler(event)} onShiftSwapClick={onShiftSwapClick} diff --git a/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx b/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx index f7b90a2c..59685916 100644 --- a/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx +++ b/grafana-plugin/src/containers/Rotations/ScheduleFinal.tsx @@ -10,7 +10,12 @@ import { ScheduleFiltersType } from 'components/ScheduleFilters/ScheduleFilters. import Text from 'components/Text/Text'; import TimelineMarks from 'components/TimelineMarks/TimelineMarks'; import Rotation from 'containers/Rotation/Rotation'; -import { getLayersFromStore, getOverridesFromStore, getShiftsFromStore } from 'models/schedule/schedule.helpers'; +import { + flattenFinalShifs, + getLayersFromStore, + getOverridesFromStore, + getShiftsFromStore, +} from 'models/schedule/schedule.helpers'; import { Schedule, Shift, ShiftSwap, Event } from 'models/schedule/schedule.types'; import { Timezone } from 'models/timezone/timezone.types'; import { WithStoreProps } from 'state/types'; @@ -55,7 +60,7 @@ class ScheduleFinal extends Component 1; + const getColor = (shiftId: Shift['id']) => findColor(shiftId, layers, overrides); + return ( <>
@@ -82,7 +89,7 @@ class ScheduleFinal extends Component {shifts && shifts.length ? ( - shifts.map(({ shiftId, events }, index) => { + shifts.map(({ events }, index) => { return ( @@ -120,18 +126,6 @@ class ScheduleFinal extends Component { - const { onClick, disabled } = this.props; - - return () => { - if (disabled) { - return; - } - - onClick(shiftId); - }; - }; - onSearchTermChangeCallback = () => {}; handleShowOverrideForm = (shiftStart: dayjs.Dayjs, shiftEnd: dayjs.Dayjs) => { diff --git a/grafana-plugin/src/models/schedule/schedule.helpers.ts b/grafana-plugin/src/models/schedule/schedule.helpers.ts index 21d4e396..41125388 100644 --- a/grafana-plugin/src/models/schedule/schedule.helpers.ts +++ b/grafana-plugin/src/models/schedule/schedule.helpers.ts @@ -8,6 +8,23 @@ export const getFromString = (moment: dayjs.Dayjs) => { return moment.format('YYYY-MM-DD'); }; +const createGap = (start, end) => { + return { + start, + end, + is_gap: true, + users: [], + all_day: false, + shift: null, + missing_users: [], + is_empty: true, + calendar_type: ScheduleType.API, + priority_level: null, + source: 'web', + is_override: false, + }; +}; + export const fillGaps = (events: Event[]) => { const newEvents = []; @@ -18,19 +35,7 @@ export const fillGaps = (events: Event[]) => { if (nextEvent) { if (nextEvent.start !== event.end) { - newEvents.push({ - start: event.end, - end: nextEvent.start, - is_gap: true, - users: [], - all_day: false, - shift: null, - missing_users: [], - is_empty: true, - calendar_type: ScheduleType.API, - priority_level: null, - source: 'web', - }); + newEvents.push(createGap(event.end, nextEvent.start)); } } } @@ -69,6 +74,119 @@ export const getShiftsFromStore = ( : (store.scheduleStore.events[scheduleId]?.['final']?.[getFromString(startMoment)] as any); }; +export const flattenFinalShifs = (shifts: ShiftEvents[]) => { + if (!shifts) { + return undefined; + } + + function splitToPairs(shifts: ShiftEvents[]) { + const pairs = []; + for (let i = 0; i < shifts.length - 1; i++) { + for (let j = i + 1; j < shifts.length; j++) { + pairs.push([ + { ...shifts[i], events: [...shifts[i].events] }, + { ...shifts[j], events: [...shifts[j].events] }, + ]); + } + } + + return pairs; + } + + let pairs = splitToPairs(shifts); + + while (pairs.length > 0) { + const currentPair = pairs.shift(); + + const merged = mergePair(currentPair); + + if (merged !== currentPair) { + // means pair was fully merged + + shifts = shifts.filter((shift) => !currentPair.some((pairShift) => pairShift.shiftId === shift.shiftId)); + shifts.unshift(merged[0]); + pairs = splitToPairs(shifts); + } + } + + function mergePair(pair: ShiftEvents[]): ShiftEvents[] { + const recipient = { ...pair[0], events: [...pair[0].events] }; + const donor = pair[1]; + + const donorEvents = donor.events.filter((event) => !event.is_gap); + + for (let i = 0; i < donorEvents.length; i++) { + const donorEvent = donorEvents[i]; + + const eventStartMoment = dayjs(donorEvent.start); + const eventEndMoment = dayjs(donorEvent.end); + + const suitablerRecepientGapIndex = recipient.events.findIndex((event) => { + if (!event.is_gap) { + return false; + } + + const gap = event; + + const gapStartMoment = dayjs(gap.start); + const gapEndMoment = dayjs(gap.end); + + return gapStartMoment.isSameOrBefore(eventStartMoment) && gapEndMoment.isSameOrAfter(eventEndMoment); + }); + + if (suitablerRecepientGapIndex > -1) { + const suitablerRecepientGap = recipient.events[suitablerRecepientGapIndex]; + + const itemsToAdd = []; + const leftGap = createGap(suitablerRecepientGap.start, donorEvent.start); + if (leftGap.start !== leftGap.end) { + itemsToAdd.push(leftGap); + } + itemsToAdd.push(donorEvent); + + const rightGap = createGap(donorEvent.end, suitablerRecepientGap.end); + if (rightGap.start !== rightGap.end) { + itemsToAdd.push(rightGap); + } + + recipient.events = [ + ...recipient.events.slice(0, suitablerRecepientGapIndex), + ...itemsToAdd, + ...recipient.events.slice(suitablerRecepientGapIndex + 1), + ]; + } else { + const firstRecepientEvent = recipient.events[0]; + const firstRecepientEventStartMoment = dayjs(firstRecepientEvent.start); + + const lastRecepientEvent = recipient.events[recipient.events.length - 1]; + const lastRecepientEventEndMoment = dayjs(lastRecepientEvent.end); + + if (eventEndMoment.isSameOrBefore(firstRecepientEventStartMoment)) { + const itemsToAdd = [donorEvent]; + if (donorEvent.end !== firstRecepientEvent.start) { + itemsToAdd.push(createGap(donorEvent.end, firstRecepientEvent.start)); + } + recipient.events = [...itemsToAdd, ...recipient.events]; + } else if (eventStartMoment.isSameOrAfter(lastRecepientEventEndMoment)) { + const itemsToAdd = [donorEvent]; + if (lastRecepientEvent.end !== donorEvent.start) { + itemsToAdd.unshift(createGap(lastRecepientEvent.end, donorEvent.start)); + } + recipient.events = [...recipient.events, ...itemsToAdd]; + } else { + // the pair can't be fully merged + + return pair; + } + } + } + + return [recipient]; + } + + return shifts; +}; + export const getLayersFromStore = (store: RootStore, scheduleId: Schedule['id'], startMoment: dayjs.Dayjs): Layer[] => { return store.scheduleStore.rotationPreview ? store.scheduleStore.rotationPreview[getFromString(startMoment)] @@ -79,7 +197,7 @@ export const getOverridesFromStore = ( store: RootStore, scheduleId: Schedule['id'], startMoment: dayjs.Dayjs -): Layer[] | ShiftEvents[] => { +): ShiftEvents[] => { return store.scheduleStore.overridePreview ? store.scheduleStore.overridePreview[getFromString(startMoment)] : (store.scheduleStore.events[scheduleId]?.['override']?.[getFromString(startMoment)] as Layer[]); diff --git a/grafana-plugin/src/models/schedule/schedule.types.ts b/grafana-plugin/src/models/schedule/schedule.types.ts index 9107e28b..0e57de64 100644 --- a/grafana-plugin/src/models/schedule/schedule.types.ts +++ b/grafana-plugin/src/models/schedule/schedule.types.ts @@ -120,6 +120,7 @@ export interface Layer { export interface ShiftEvents { shiftId: string; events: Event[]; + priority: number; isPreview?: boolean; } diff --git a/grafana-plugin/src/pages/schedule/Schedule.tsx b/grafana-plugin/src/pages/schedule/Schedule.tsx index 9b3e297d..b8c56e83 100644 --- a/grafana-plugin/src/pages/schedule/Schedule.tsx +++ b/grafana-plugin/src/pages/schedule/Schedule.tsx @@ -283,12 +283,17 @@ class SchedulePage extends React.Component scheduleId={scheduleId} currentTimezone={currentTimezone} startMoment={startMoment} - onClick={this.handleShowForm} disabled={disabledRotationForm} onShowOverrideForm={this.handleShowOverridesForm} filters={filters} onShowShiftSwapForm={this.handleShowShiftSwapForm} - onSlotClick={shiftSwapIdToShowForm ? this.onSlotClick : undefined} + onSlotClick={ + shiftSwapIdToShowForm + ? this.adjustShiftSwapForm + : (event: Event) => { + this.handleShowForm(event.shift.pk); + } + } /> disabled={disabledRotationForm} filters={filters} onShowShiftSwapForm={this.handleShowShiftSwapForm} - onSlotClick={shiftSwapIdToShowForm ? this.onSlotClick : undefined} + onSlotClick={shiftSwapIdToShowForm ? this.adjustShiftSwapForm : undefined} /> this.setState({ shiftSwapIdToShowForm: undefined, shiftSwapParamsToShowForm: undefined }); }; - onSlotClick = (event: Event) => { + adjustShiftSwapForm = (event: Event) => { this.setState({ shiftSwapParamsToShowForm: { ...this.state.shiftSwapParamsToShowForm, From bc78fc29e5eeb54220f1cfe45c9a07a7954c2c21 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Fri, 4 Aug 2023 16:13:03 +0100 Subject: [PATCH 10/17] Revert "Enable push notifications for SSR beneficiary for testing purposes" (#2757) Reverts grafana/oncall#2752 --- engine/apps/schedules/models/shift_swap_request.py | 4 +--- engine/apps/schedules/tests/test_shift_swap_request.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index 5e2de394..ebf48811 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -157,9 +157,7 @@ class ShiftSwapRequest(models.Model): @property def possible_benefactors(self) -> QuerySet["User"]: - # TODO: exclude the beneficiary from this list - # Temporarily including the beneficiary in the list of possible benefactors for testing purposes - return self.schedule.related_users() + return self.schedule.related_users().exclude(pk=self.beneficiary_id) @property def web_link(self) -> str: diff --git a/engine/apps/schedules/tests/test_shift_swap_request.py b/engine/apps/schedules/tests/test_shift_swap_request.py index 685959c0..a6735484 100644 --- a/engine/apps/schedules/tests/test_shift_swap_request.py +++ b/engine/apps/schedules/tests/test_shift_swap_request.py @@ -166,6 +166,4 @@ def test_possible_benefactors(shift_swap_request_setup) -> None: with patch.object(ssr.schedule, "related_users") as mock_related_users: mock_related_users.return_value = User.objects.filter(pk__in=[beneficiary.pk, benefactor.pk]) - # TODO: exclude the beneficiary from this list - # Temporarily including the beneficiary in the list of possible benefactors for testing purposes - assert set(ssr.possible_benefactors) == {benefactor, beneficiary} + assert list(ssr.possible_benefactors) == [benefactor] From bb9f647608b2b3aad51e6f5e13e6131843277975 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Fri, 4 Aug 2023 14:43:54 -0300 Subject: [PATCH 11/17] Filter out untaken swaps from final schedule and shift notifications (#2748) Avoid creating (or notifying) about potential event splits resulting from untaken swap requests. --- CHANGELOG.md | 1 + .../tasks/notify_ical_schedule_shift.py | 6 +- .../tests/test_notify_ical_schedule_shift.py | 65 ++++++++++ .../apps/schedules/models/on_call_schedule.py | 25 +++- .../schedules/models/shift_swap_request.py | 2 +- .../schedules/tests/test_on_call_schedule.py | 122 +++++++++++++++--- .../tests/test_shift_swap_request.py | 8 ++ 7 files changed, 205 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57067135..f3a0e133 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Add stack slug to organization options for direct paging Slash command by @vadimkerr ([#2743](https://github.com/grafana/oncall/pull/2743)) +- Avoid creating (or notifying about) potential event splits resulting from untaken swap requests ([#2748](https://github.com/grafana/oncall/pull/2748)) ### Fixed diff --git a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py index 0324783f..9321717c 100644 --- a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py @@ -81,7 +81,7 @@ def notify_ical_schedule_shift(schedule_pk): now = datetime.datetime.now(timezone.utc) - current_shifts = schedule.final_events(now, now, with_empty=False, with_gap=False) + current_shifts = schedule.final_events(now, now, with_empty=False, with_gap=False, ignore_untaken_swaps=True) prev_shifts = json.loads(schedule.current_shifts) if not schedule.empty_oncall else [] prev_shifts_updated = False @@ -119,7 +119,9 @@ def notify_ical_schedule_shift(schedule_pk): datetime_end = now + datetime.timedelta(days=days_to_lookup) - next_shifts_unfiltered = schedule.final_events(now, datetime_end, with_empty=False, with_gap=False) + next_shifts_unfiltered = schedule.final_events( + now, datetime_end, with_empty=False, with_gap=False, ignore_untaken_swaps=True + ) # drop events that already started next_shifts = [] for next_shift in next_shifts_unfiltered: diff --git a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py index d5925c9d..5d931181 100644 --- a/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py +++ b/engine/apps/alerts/tests/test_notify_ical_schedule_shift.py @@ -305,6 +305,71 @@ def test_current_shift_changes_trigger_notification( assert mock_slack_api_call.called +@pytest.mark.django_db +@pytest.mark.parametrize("swap_taken", [False, True]) +def test_current_shift_changes_swap_split( + make_organization_and_user_with_slack_identities, + make_user, + make_schedule, + make_on_call_shift, + make_shift_swap_request, + swap_taken, +): + organization, _, _, _ = make_organization_and_user_with_slack_identities() + user1 = make_user(organization=organization, username="user1") + user2 = make_user(organization=organization, username="user2") + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_schedule", + channel="channel", + prev_ical_file_overrides=None, + cached_ical_file_overrides=None, + ) + + today = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + duration = timezone.timedelta(hours=23, minutes=59, seconds=59) + data = { + "start": today, + "rotation_start": today, + "duration": duration, + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user1]]) + + # setup in progress swap request + swap_request = make_shift_swap_request( + schedule, + user1, + swap_start=today, + swap_end=today + timezone.timedelta(days=2), + ) + if swap_taken: + swap_request.benefactor = user2 + swap_request.save() + + 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.pk) + + text_block = mock_slack_api_call.call_args_list[0][1]["blocks"][0]["text"]["text"] + assert "user2" in text_block if swap_taken else "user1" in text_block + + @pytest.mark.django_db def test_next_shift_changes_no_triggering_notification( make_organization_and_user_with_slack_identities, diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index 56963e08..bbb16b94 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -344,6 +344,7 @@ class OnCallSchedule(PolymorphicModel): with_gap: bool = False, filter_by: str | None = None, all_day_datetime: bool = False, + ignore_untaken_swaps: bool = False, from_cached_final: bool = False, ) -> ScheduleEvents: """Return filtered events from schedule.""" @@ -401,7 +402,9 @@ class OnCallSchedule(PolymorphicModel): events = self._merge_events(events) # annotate events with swap request details swapping users as needed - events = self._apply_swap_requests(events, datetime_start, datetime_end) + events = self._apply_swap_requests( + events, datetime_start, datetime_end, ignore_untaken_swaps=ignore_untaken_swaps + ) return events @@ -411,10 +414,16 @@ class OnCallSchedule(PolymorphicModel): datetime_end: datetime.datetime, with_empty: bool = True, with_gap: bool = True, + ignore_untaken_swaps: bool = False, ) -> ScheduleEvents: """Return schedule final events, after resolving shifts and overrides.""" events = self.filter_events( - datetime_start, datetime_end, with_empty=with_empty, with_gap=with_gap, all_day_datetime=True + datetime_start, + datetime_end, + with_empty=with_empty, + with_gap=with_gap, + all_day_datetime=True, + ignore_untaken_swaps=ignore_untaken_swaps, ) events = self._resolve_schedule(events, datetime_start, datetime_end) return events @@ -442,7 +451,7 @@ class OnCallSchedule(PolymorphicModel): # setup calendar with final schedule shift events calendar = create_base_icalendar(self.name) - events = self.final_events(datetime_start, datetime_end) + events = self.final_events(datetime_start, datetime_end, ignore_untaken_swaps=True) updated_ids = set() for e in events: for u in e["users"]: @@ -647,7 +656,11 @@ class OnCallSchedule(PolymorphicModel): } def _apply_swap_requests( - self, events: ScheduleEvents, datetime_start: datetime.datetime, datetime_end: datetime.datetime + self, + events: ScheduleEvents, + datetime_start: datetime.datetime, + datetime_end: datetime.datetime, + ignore_untaken_swaps: bool = False, ) -> ScheduleEvents: """Apply swap requests details to schedule events.""" # get swaps requests affecting this schedule / time range @@ -663,8 +676,8 @@ class OnCallSchedule(PolymorphicModel): # apply swaps sequentially for swap in swaps: - if swap.is_past_due: - # ignore untaken expired requests + if swap.is_past_due or (ignore_untaken_swaps and not swap.is_taken): + # ignore expired requests, or untaken if specified continue i = 0 while i < len(events): diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index ebf48811..11894d31 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -127,7 +127,7 @@ class ShiftSwapRequest(models.Model): @property def is_past_due(self) -> bool: - return timezone.now() > self.swap_start + return not self.is_taken and timezone.now() > self.swap_start @property def is_open(self) -> bool: diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index 603c69ae..42ba8107 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -2169,21 +2169,36 @@ def test_swap_request_split_both( with patch("apps.schedules.models.on_call_schedule.EXPORT_WINDOW_DAYS_BEFORE", 0): schedule.refresh_ical_final_schedule() assert schedule.cached_ical_final_schedule - expected_events = [ - # start, end, user - (start, start + duration, user.username), # today shift unchanged - (start + timezone.timedelta(days=1), start + timezone.timedelta(days=1, hours=1), user.username), # first split - ( - start + timezone.timedelta(days=1, hours=1), - start + timezone.timedelta(days=1, hours=2), - other_user.username if swap_taken else user.username, - ), # second split - ( - start + timezone.timedelta(days=1, hours=2), - start + timezone.timedelta(days=1, hours=3), - user.username, - ), # third split - ] + if swap_taken: + expected_events = [ + # start, end, user + (start, start + duration, user.username), # today shift unchanged + ( + start + timezone.timedelta(days=1), + start + timezone.timedelta(days=1, hours=1), + user.username, + ), # first split + ( + start + timezone.timedelta(days=1, hours=1), + start + timezone.timedelta(days=1, hours=2), + other_user.username if swap_taken else user.username, + ), # second split + ( + start + timezone.timedelta(days=1, hours=2), + start + timezone.timedelta(days=1, hours=3), + user.username, + ), # third split + ] + else: + expected_events = [ + # start, end, user + (start, start + duration, user.username), # today shift unchanged + ( + start + timezone.timedelta(days=1), + start + timezone.timedelta(days=1) + duration, + user.username, + ), # no split + ] calendar = icalendar.Calendar.from_ical(schedule.cached_ical_final_schedule) for component in calendar.walk(): if component.name == ICAL_COMPONENT_VEVENT: @@ -2195,6 +2210,83 @@ def test_swap_request_split_both( assert event in expected_events +@pytest.mark.django_db +@pytest.mark.parametrize("swap_taken", [False, True]) +def test_swap_request_ignore_untaken( + make_organization, + make_user_for_organization, + make_schedule, + make_on_call_shift, + make_shift_swap_request, + swap_taken, +): + organization = make_organization() + user = make_user_for_organization(organization) + other_user = make_user_for_organization(organization) + + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + today = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + start = today + timezone.timedelta(hours=12) + duration = timezone.timedelta(hours=3) + data = { + "start": start, + "rotation_start": start, + "duration": duration, + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user]]) + + tomorrow = today + timezone.timedelta(days=1) + # setup swap request + swap_request = make_shift_swap_request( + schedule, + user, + swap_start=tomorrow + timezone.timedelta(hours=13), + swap_end=tomorrow + timezone.timedelta(hours=14), + ) + if swap_taken: + swap_request.take(other_user) + + # set flag to ignore untaken swaps + events = schedule.filter_events(today, today + timezone.timedelta(days=2), ignore_untaken_swaps=True) + + if swap_taken: + expected = [ + # start, end, swap requested + (start, start + duration, False), # today shift unchanged + (start + timezone.timedelta(days=1), start + timezone.timedelta(days=1, hours=1), False), # first split + ( + start + timezone.timedelta(days=1, hours=1), + start + timezone.timedelta(days=1, hours=2), + True, + ), # second split + ( + start + timezone.timedelta(days=1, hours=2), + start + timezone.timedelta(days=1, hours=3), + False, + ), # third split + ] + else: + expected = [ + # start, end, swap requested + (start, start + duration, False), # today shift unchanged + (start + timezone.timedelta(days=1), start + timezone.timedelta(days=1) + duration, False), # no split + ] + + returned = [(e["start"], e["end"], bool(e["users"][0].get("swap_request", False))) for e in events] + assert returned == expected + # check swap request details + if swap_taken: + assert events[2]["users"][0]["swap_request"]["pk"] == swap_request.public_primary_key + assert events[2]["users"][0]["pk"] == other_user.public_primary_key + assert events[2]["users"][0]["swap_request"]["user"]["pk"] == user.public_primary_key + + @pytest.mark.django_db @pytest.mark.parametrize("swap_taken", [False, True]) def test_swap_request_whole_shift( diff --git a/engine/apps/schedules/tests/test_shift_swap_request.py b/engine/apps/schedules/tests/test_shift_swap_request.py index a6735484..63de2b75 100644 --- a/engine/apps/schedules/tests/test_shift_swap_request.py +++ b/engine/apps/schedules/tests/test_shift_swap_request.py @@ -46,6 +46,14 @@ def test_status_taken(shift_swap_request_setup) -> None: assert ssr.status == ShiftSwapRequest.Statuses.TAKEN assert ssr.is_taken is True + # taken in the past it's still taken + now = timezone.now() + ssr.swap_start = now - timezone.timedelta(days=2) + ssr.save() + assert ssr.status == ShiftSwapRequest.Statuses.TAKEN + assert ssr.is_taken is True + assert ssr.is_past_due is False + @pytest.mark.django_db def test_status_past_due(shift_swap_request_setup) -> None: From 424b2b80f8eaeac53bb6086e3f693c18a3c2e55b Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 7 Aug 2023 10:55:17 +0100 Subject: [PATCH 12/17] Add backend support for push notification sounds with custom extensions (#2759) # What this PR does Instead of always adding `.aiff` or `.mp3` at the end of notification sound names depending on the platform (iOS vs Android), add them only if no extension is present already. This should make it possible to use sounds with custom extensions. ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- CHANGELOG.md | 3 +- engine/apps/mobile_app/demo_push.py | 22 +++----- engine/apps/mobile_app/models.py | 20 ++++++++ engine/apps/mobile_app/tasks.py | 50 ++++++------------- .../apps/mobile_app/tests/test_demo_push.py | 2 + .../apps/mobile_app/tests/test_notify_user.py | 2 + .../tests/test_shift_swap_request.py | 3 +- .../mobile_app/tests/test_user_settings.py | 35 +++++++++++++ .../test_your_going_oncall_notification.py | 9 ++-- engine/apps/mobile_app/types.py | 19 +++++++ 10 files changed, 107 insertions(+), 58 deletions(-) create mode 100644 engine/apps/mobile_app/types.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f3a0e133..02dd2683 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Shift Swap Requests Web UI ([#2593](https://github.com/grafana/oncall/issues/2593)) -- Final schedule shifts should lay in one line [1665](https://github.com/grafana/oncall/issues/1665) +- Final schedule shifts should lay in one line ([#1665](https://github.com/grafana/oncall/issues/1665)) +- Add backend support for push notification sounds with custom extensions by @vadimkerr ([#2759](https://github.com/grafana/oncall/pull/2759)) ### Changed diff --git a/engine/apps/mobile_app/demo_push.py b/engine/apps/mobile_app/demo_push.py index 557c34e3..4ac67969 100644 --- a/engine/apps/mobile_app/demo_push.py +++ b/engine/apps/mobile_app/demo_push.py @@ -6,7 +6,8 @@ import typing from firebase_admin.messaging import APNSPayload, Aps, ApsAlert, CriticalSound, Message from apps.mobile_app.exceptions import DeviceNotSet -from apps.mobile_app.tasks import FCMMessageData, MessageType, _construct_fcm_message, _send_push_notification, logger +from apps.mobile_app.tasks import _construct_fcm_message, _send_push_notification, logger +from apps.mobile_app.types import FCMMessageData, MessageType, Platform from apps.user_management.models import User if typing.TYPE_CHECKING: @@ -38,27 +39,22 @@ def _get_test_escalation_fcm_message(user: User, device_to_notify: "FCMDevice", # APNS only allows to specify volume for critical notifications apns_volume = mobile_app_user_settings.important_notification_volume if critical else None - apns_sound_name = ( - mobile_app_user_settings.important_notification_sound_name - if critical - else mobile_app_user_settings.default_notification_sound_name - ) + MobileAppUserSettings.IOS_SOUND_NAME_EXTENSION # iOS app expects the filename to have an extension + message_type = MessageType.IMPORTANT if critical else MessageType.DEFAULT + apns_sound_name = mobile_app_user_settings.get_notification_sound_name(message_type, Platform.IOS) fcm_message_data: FCMMessageData = { "title": get_test_push_title(critical), # Pass user settings, so the Android app can use them to play the correct sound and volume - "default_notification_sound_name": ( - mobile_app_user_settings.default_notification_sound_name - + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION + "default_notification_sound_name": mobile_app_user_settings.get_notification_sound_name( + MessageType.DEFAULT, Platform.ANDROID ), "default_notification_volume_type": mobile_app_user_settings.default_notification_volume_type, "default_notification_volume": str(mobile_app_user_settings.default_notification_volume), "default_notification_volume_override": json.dumps( mobile_app_user_settings.default_notification_volume_override ), - "important_notification_sound_name": ( - mobile_app_user_settings.important_notification_sound_name - + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION + "important_notification_sound_name": mobile_app_user_settings.get_notification_sound_name( + MessageType.IMPORTANT, Platform.ANDROID ), "important_notification_volume_type": mobile_app_user_settings.important_notification_volume_type, "important_notification_volume": str(mobile_app_user_settings.important_notification_volume), @@ -84,8 +80,6 @@ def _get_test_escalation_fcm_message(user: User, device_to_notify: "FCMDevice", ), ) - message_type = MessageType.CRITICAL if critical else MessageType.NORMAL - return _construct_fcm_message(message_type, device_to_notify, thread_id, fcm_message_data, apns_payload) diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index a4a79abd..46680ae1 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -10,6 +10,7 @@ from fcm_django.models import FCMDevice as BaseFCMDevice from apps.auth_token import constants, crypto from apps.auth_token.models import BaseAuthToken +from apps.mobile_app.types import MessageType, Platform if typing.TYPE_CHECKING: from apps.user_management.models import Organization, User @@ -175,3 +176,22 @@ class MobileAppUserSettings(models.Model): locale = models.CharField(max_length=50, null=True) time_zone = models.CharField(max_length=100, default="UTC") + + def get_notification_sound_name(self, message_type: MessageType, platform: Platform) -> str: + sound_name = { + MessageType.DEFAULT: self.default_notification_sound_name, + MessageType.IMPORTANT: self.important_notification_sound_name, + MessageType.INFO: self.info_notification_sound_name, + }[message_type] + + # If sound name already contains an extension, return it as is + if "." in sound_name: + return sound_name + + # Add appropriate extension based on platform, for cases when no extension is specified in the sound name + extension = { + Platform.IOS: self.IOS_SOUND_NAME_EXTENSION, + Platform.ANDROID: self.ANDROID_SOUND_NAME_EXTENSION, + }[platform] + + return f"{sound_name}{extension}" diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 456cbe45..d4eb354e 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -3,7 +3,6 @@ import json import logging import math import typing -from enum import Enum import humanize import pytz @@ -20,6 +19,7 @@ from rest_framework import status from apps.alerts.models import AlertGroup from apps.base.utils import live_settings from apps.mobile_app.alert_rendering import get_push_notification_subtitle +from apps.mobile_app.types import FCMMessageData, MessageType, Platform from apps.schedules.models import ShiftSwapRequest from apps.schedules.models.on_call_schedule import OnCallSchedule, ScheduleEvent from apps.user_management.models import User @@ -36,18 +36,6 @@ logger = get_task_logger(__name__) logger.setLevel(logging.DEBUG) -class MessageType(str, Enum): - NORMAL = "oncall.message" - CRITICAL = "oncall.critical_message" - INFO = "oncall.info" - - -class FCMMessageData(typing.TypedDict): - title: str - subtitle: typing.Optional[str] - body: typing.Optional[str] - - def send_push_notification_to_fcm_relay(message: Message) -> requests.Response: """ Send push notification to FCM relay on cloud instance: apps.mobile_app.fcm_relay.FCMRelayView @@ -168,11 +156,8 @@ def _get_alert_group_escalation_fcm_message( # APNS only allows to specify volume for critical notifications apns_volume = mobile_app_user_settings.important_notification_volume if critical else None - apns_sound_name = ( - mobile_app_user_settings.important_notification_sound_name - if critical - else mobile_app_user_settings.default_notification_sound_name - ) + MobileAppUserSettings.IOS_SOUND_NAME_EXTENSION # iOS app expects the filename to have an extension + message_type = MessageType.IMPORTANT if critical else MessageType.DEFAULT + apns_sound_name = mobile_app_user_settings.get_notification_sound_name(message_type, Platform.IOS) fcm_message_data: FCMMessageData = { "title": alert_title, @@ -183,18 +168,16 @@ def _get_alert_group_escalation_fcm_message( # alert_group.status is an int so it must be casted... "status": str(alert_group.status), # Pass user settings, so the Android app can use them to play the correct sound and volume - "default_notification_sound_name": ( - mobile_app_user_settings.default_notification_sound_name - + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION + "default_notification_sound_name": mobile_app_user_settings.get_notification_sound_name( + MessageType.DEFAULT, Platform.ANDROID ), "default_notification_volume_type": mobile_app_user_settings.default_notification_volume_type, "default_notification_volume": str(mobile_app_user_settings.default_notification_volume), "default_notification_volume_override": json.dumps( mobile_app_user_settings.default_notification_volume_override ), - "important_notification_sound_name": ( - mobile_app_user_settings.important_notification_sound_name - + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION + "important_notification_sound_name": mobile_app_user_settings.get_notification_sound_name( + MessageType.IMPORTANT, Platform.ANDROID ), "important_notification_volume_type": mobile_app_user_settings.important_notification_volume_type, "important_notification_volume": str(mobile_app_user_settings.important_notification_volume), @@ -222,8 +205,6 @@ def _get_alert_group_escalation_fcm_message( ), ) - message_type = MessageType.CRITICAL if critical else MessageType.NORMAL - return _construct_fcm_message(message_type, device_to_notify, thread_id, fcm_message_data, apns_payload) @@ -268,8 +249,6 @@ def _get_youre_going_oncall_fcm_message( thread_id = f"{schedule.public_primary_key}:{user.public_primary_key}:going-oncall" mobile_app_user_settings, _ = MobileAppUserSettings.objects.get_or_create(user=user) - info_notification_sound_name = mobile_app_user_settings.info_notification_sound_name - notification_title = _get_youre_going_oncall_notification_title(seconds_until_going_oncall) notification_subtitle = _get_youre_going_oncall_notification_subtitle( schedule, schedule_event, mobile_app_user_settings @@ -278,7 +257,9 @@ def _get_youre_going_oncall_fcm_message( data: FCMMessageData = { "title": notification_title, "subtitle": notification_subtitle, - "info_notification_sound_name": f"{info_notification_sound_name}{MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION}", + "info_notification_sound_name": mobile_app_user_settings.get_notification_sound_name( + MessageType.INFO, Platform.ANDROID + ), "info_notification_volume_type": mobile_app_user_settings.info_notification_volume_type, "info_notification_volume": str(mobile_app_user_settings.info_notification_volume), "info_notification_volume_override": json.dumps(mobile_app_user_settings.info_notification_volume_override), @@ -290,7 +271,7 @@ def _get_youre_going_oncall_fcm_message( alert=ApsAlert(title=notification_title, subtitle=notification_subtitle), sound=CriticalSound( critical=False, - name=f"{info_notification_sound_name}{MobileAppUserSettings.IOS_SOUND_NAME_EXTENSION}", + name=mobile_app_user_settings.get_notification_sound_name(MessageType.INFO, Platform.IOS), ), custom_data={ "interruption-level": "time-sensitive", @@ -641,8 +622,6 @@ def _shift_swap_request_fcm_message( device_to_notify: "FCMDevice", mobile_app_user_settings: "MobileAppUserSettings", ) -> Message: - from apps.mobile_app.models import MobileAppUserSettings - thread_id = f"{shift_swap_request.public_primary_key}:{user.public_primary_key}:ssr" notification_title = "New shift swap request" beneficiary_name = shift_swap_request.beneficiary.name or shift_swap_request.beneficiary.username @@ -655,8 +634,8 @@ def _shift_swap_request_fcm_message( "title": notification_title, "subtitle": notification_subtitle, "route": route, - "info_notification_sound_name": ( - mobile_app_user_settings.info_notification_sound_name + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION + "info_notification_sound_name": mobile_app_user_settings.get_notification_sound_name( + MessageType.INFO, Platform.ANDROID ), "info_notification_volume_type": mobile_app_user_settings.info_notification_volume_type, "info_notification_volume": str(mobile_app_user_settings.info_notification_volume), @@ -669,8 +648,7 @@ def _shift_swap_request_fcm_message( alert=ApsAlert(title=notification_title, subtitle=notification_subtitle), sound=CriticalSound( critical=False, - name=mobile_app_user_settings.info_notification_sound_name - + MobileAppUserSettings.IOS_SOUND_NAME_EXTENSION, + name=mobile_app_user_settings.get_notification_sound_name(MessageType.INFO, Platform.IOS), ), custom_data={ "interruption-level": "time-sensitive", diff --git a/engine/apps/mobile_app/tests/test_demo_push.py b/engine/apps/mobile_app/tests/test_demo_push.py index 73041ac3..abf5f6eb 100644 --- a/engine/apps/mobile_app/tests/test_demo_push.py +++ b/engine/apps/mobile_app/tests/test_demo_push.py @@ -34,6 +34,7 @@ def test_test_escalation_fcm_message_user_settings( assert message.apns.payload.aps.badge is None assert message.apns.payload.aps.alert.title == get_test_push_title(critical=False) assert message.data["title"] == get_test_push_title(critical=False) + assert message.data["type"] == "oncall.message" @pytest.mark.django_db @@ -67,6 +68,7 @@ def test_escalation_fcm_message_user_settings_critical( assert message.apns.payload.aps.badge is None assert message.apns.payload.aps.alert.title == get_test_push_title(critical=True) assert message.data["title"] == get_test_push_title(critical=True) + assert message.data["type"] == "oncall.critical_message" @pytest.mark.django_db diff --git a/engine/apps/mobile_app/tests/test_notify_user.py b/engine/apps/mobile_app/tests/test_notify_user.py index 17e28d12..b7ba5046 100644 --- a/engine/apps/mobile_app/tests/test_notify_user.py +++ b/engine/apps/mobile_app/tests/test_notify_user.py @@ -234,6 +234,7 @@ def test_fcm_message_user_settings( assert message.data["important_notification_volume"] == "0.8" assert message.data["important_notification_volume_override"] == "true" assert message.data["important_notification_override_dnd"] == "true" + assert message.data["type"] == "oncall.message" # Check APNS notification sound is set correctly apns_sound = message.apns.payload.aps.sound @@ -265,6 +266,7 @@ def test_fcm_message_user_settings_critical( assert message.data["important_notification_volume"] == "0.8" assert message.data["important_notification_volume_override"] == "true" assert message.data["important_notification_override_dnd"] == "true" + assert message.data["type"] == "oncall.critical_message" # Check APNS notification sound is set correctly apns_sound = message.apns.payload.aps.sound diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 9d763713..afd216a7 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -9,7 +9,6 @@ from apps.mobile_app.models import FCMDevice, MobileAppUserSettings from apps.mobile_app.tasks import ( SSR_EARLIEST_NOTIFICATION_OFFSET, SSR_NOTIFICATION_WINDOW, - MessageType, _get_shift_swap_requests_to_notify, _has_user_been_notified_for_shift_swap_request, _mark_shift_swap_request_notified_for_user, @@ -261,7 +260,7 @@ def test_notify_user_about_shift_swap_request(make_organization, make_user, make assert mock_send_push_notification.call_args.args[0] == device_to_notify message: Message = mock_send_push_notification.call_args.args[1] - assert message.data["type"] == MessageType.INFO + assert message.data["type"] == "oncall.info" assert message.data["title"] == "New shift swap request" assert message.data["subtitle"] == "John Doe, Test Schedule" assert ( diff --git a/engine/apps/mobile_app/tests/test_user_settings.py b/engine/apps/mobile_app/tests/test_user_settings.py index 063a142c..43ceede9 100644 --- a/engine/apps/mobile_app/tests/test_user_settings.py +++ b/engine/apps/mobile_app/tests/test_user_settings.py @@ -3,6 +3,9 @@ from django.urls import reverse from rest_framework import status from rest_framework.test import APIClient +from apps.mobile_app.models import MobileAppUserSettings +from apps.mobile_app.types import MessageType, Platform + @pytest.mark.django_db def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token): @@ -140,3 +143,35 @@ def test_user_settings_time_zone_must_be_valid(make_organization_and_user_with_m response = client.put(url, data=null_timezone, format="json", HTTP_AUTHORIZATION=auth_token) assert response.status_code == status.HTTP_400_BAD_REQUEST + + +@pytest.mark.parametrize( + "message_type,platform,sound_names,expected_sound_name", + [ + (MessageType.DEFAULT, Platform.ANDROID, ["default", "empty", "empty"], "default.mp3"), + (MessageType.DEFAULT, Platform.ANDROID, ["default.extension", "empty", "empty"], "default.extension"), + (MessageType.DEFAULT, Platform.IOS, ["default", "empty", "empty"], "default.aiff"), + (MessageType.DEFAULT, Platform.IOS, ["default.extension", "empty", "empty"], "default.extension"), + (MessageType.IMPORTANT, Platform.ANDROID, ["empty", "important", "empty"], "important.mp3"), + (MessageType.IMPORTANT, Platform.ANDROID, ["empty", "important.extension", "empty"], "important.extension"), + (MessageType.IMPORTANT, Platform.IOS, ["empty", "important", "empty"], "important.aiff"), + (MessageType.IMPORTANT, Platform.IOS, ["empty", "important.extension", "empty"], "important.extension"), + (MessageType.INFO, Platform.ANDROID, ["empty", "empty", "info"], "info.mp3"), + (MessageType.INFO, Platform.ANDROID, ["empty", "empty", "info.extension"], "info.extension"), + (MessageType.INFO, Platform.IOS, ["empty", "empty", "info"], "info.aiff"), + (MessageType.INFO, Platform.IOS, ["empty", "empty", "info.extension"], "info.extension"), + ], +) +@pytest.mark.django_db +def test_get_notification_sound_name( + make_organization_and_user, message_type, platform, sound_names, expected_sound_name +): + organization, user = make_organization_and_user() + mobile_app_user_settings = MobileAppUserSettings.objects.create( + user=user, + default_notification_sound_name=sound_names[0], + important_notification_sound_name=sound_names[1], + info_notification_sound_name=sound_names[2], + ) + + assert mobile_app_user_settings.get_notification_sound_name(message_type, platform) == expected_sound_name diff --git a/engine/apps/mobile_app/tests/test_your_going_oncall_notification.py b/engine/apps/mobile_app/tests/test_your_going_oncall_notification.py index a218fd6f..3cb90061 100644 --- a/engine/apps/mobile_app/tests/test_your_going_oncall_notification.py +++ b/engine/apps/mobile_app/tests/test_your_going_oncall_notification.py @@ -8,6 +8,7 @@ from django.utils import timezone from apps.mobile_app import tasks from apps.mobile_app.models import FCMDevice, MobileAppUserSettings +from apps.mobile_app.types import MessageType, Platform from apps.schedules.models import OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb from apps.schedules.models.on_call_schedule import ScheduleEvent @@ -217,9 +218,7 @@ def test_get_youre_going_oncall_fcm_message( data = { "title": mock_notification_title, "subtitle": mock_notification_subtitle, - "info_notification_sound_name": ( - maus.info_notification_sound_name + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION - ), + "info_notification_sound_name": maus.get_notification_sound_name(MessageType.INFO, Platform.ANDROID), "info_notification_volume_type": maus.info_notification_volume_type, "info_notification_volume": str(maus.info_notification_volume), "info_notification_volume_override": json.dumps(maus.info_notification_volume_override), @@ -233,7 +232,7 @@ def test_get_youre_going_oncall_fcm_message( mock_aps_alert.assert_called_once_with(title=mock_notification_title, subtitle=mock_notification_subtitle) mock_critical_sound.assert_called_once_with( - critical=False, name=maus.info_notification_sound_name + MobileAppUserSettings.IOS_SOUND_NAME_EXTENSION + critical=False, name=maus.get_notification_sound_name(MessageType.INFO, Platform.IOS) ) mock_aps.assert_called_once_with( thread_id=notification_thread_id, @@ -249,7 +248,7 @@ def test_get_youre_going_oncall_fcm_message( mock_get_youre_going_oncall_notification_title.assert_called_once_with(seconds_until_going_oncall) mock_construct_fcm_message.assert_called_once_with( - tasks.MessageType.INFO, device, notification_thread_id, data, mock_apns_payload.return_value + MessageType.INFO, device, notification_thread_id, data, mock_apns_payload.return_value ) diff --git a/engine/apps/mobile_app/types.py b/engine/apps/mobile_app/types.py new file mode 100644 index 00000000..210a4696 --- /dev/null +++ b/engine/apps/mobile_app/types.py @@ -0,0 +1,19 @@ +import typing +from enum import StrEnum + + +class MessageType(StrEnum): + DEFAULT = "oncall.message" + IMPORTANT = "oncall.critical_message" + INFO = "oncall.info" + + +class Platform(StrEnum): + ANDROID = "android" + IOS = "ios" + + +class FCMMessageData(typing.TypedDict): + title: str + subtitle: typing.Optional[str] + body: typing.Optional[str] From a1a9e0c33c0c2fe5a78e68af3409dcc3ce9d5f1c Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Mon, 7 Aug 2023 10:11:46 -0300 Subject: [PATCH 13/17] Handle ical schedule import with duplicated event UIDs (#2760) --- CHANGELOG.md | 1 + engine/apps/schedules/constants.py | 2 ++ engine/apps/schedules/ical_utils.py | 17 ++++++++-- .../calendars/modified_recurring_event.ics | 34 +++++++++++++++++++ .../apps/schedules/tests/test_ical_utils.py | 14 ++++++++ .../schedules/tests/test_on_call_schedule.py | 19 +++++++++++ 6 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 engine/apps/schedules/tests/calendars/modified_recurring_event.ics diff --git a/CHANGELOG.md b/CHANGELOG.md index 02dd2683..b3800828 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Do not show override shortcut when web overrides are disabled ([#2745](https://github.com/grafana/oncall/pull/2745)) +- Handle ical schedule import with duplicated event UIDs ([#2760](https://github.com/grafana/oncall/pull/2760)) ## v1.3.22 (2023-08-03) diff --git a/engine/apps/schedules/constants.py b/engine/apps/schedules/constants.py index fd5a52a0..492aa6bb 100644 --- a/engine/apps/schedules/constants.py +++ b/engine/apps/schedules/constants.py @@ -7,6 +7,8 @@ ICAL_SUMMARY = "SUMMARY" ICAL_DESCRIPTION = "DESCRIPTION" ICAL_ATTENDEE = "ATTENDEE" ICAL_UID = "UID" +ICAL_SEQUENCE = "SEQUENCE" +ICAL_RECURRENCE_ID = "RECURRENCE-ID" ICAL_RRULE = "RRULE" ICAL_UNTIL = "UNTIL" ICAL_LAST_MODIFIED = "LAST-MODIFIED" diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index b20cb0ea..8862aa5b 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -22,6 +22,8 @@ from apps.schedules.constants import ( ICAL_DATETIME_START, ICAL_DESCRIPTION, ICAL_LOCATION, + ICAL_RECURRENCE_ID, + ICAL_SEQUENCE, ICAL_SUMMARY, ICAL_UID, RE_EVENT_UID_V1, @@ -198,8 +200,12 @@ def get_shifts_dict( result_datetime = [] result_date = [] for event in events: + sequence = event.get(ICAL_SEQUENCE) + recurrence_id = event.get(ICAL_RECURRENCE_ID) + if recurrence_id: + recurrence_id = recurrence_id.dt.isoformat() priority = parse_priority_from_string(event.get(ICAL_SUMMARY, "[L0]")) - pk, source = parse_event_uid(event.get(ICAL_UID)) + pk, source = parse_event_uid(event.get(ICAL_UID), sequence=sequence, recurrence_id=recurrence_id) users = get_users_from_ical_event(event, schedule.organization) missing_users = get_missing_users_from_ical_event(event, schedule.organization) event_calendar_type = calendar_type @@ -394,7 +400,7 @@ def parse_priority_from_string(string: str) -> int: return priority -def parse_event_uid(string: str): +def parse_event_uid(string: str, sequence: str = None, recurrence_id: str = None): pk = None source = None source_verbal = None @@ -411,6 +417,13 @@ def parse_event_uid(string: str): else: # fallback to use the UID string as the rotation ID pk = string + # in ical imported calendars, sequence and/or recurrence_id + # distinguish main recurring event vs instance modification + # (see https://icalendar.org/iCalendar-RFC-5545/3-8-4-4-recurrence-id.html) + if sequence: + pk = f"{pk}_{sequence}" + if recurrence_id: + pk = f"{pk}_{recurrence_id}" if source is not None: source = int(source) diff --git a/engine/apps/schedules/tests/calendars/modified_recurring_event.ics b/engine/apps/schedules/tests/calendars/modified_recurring_event.ics new file mode 100644 index 00000000..bf46bcc1 --- /dev/null +++ b/engine/apps/schedules/tests/calendars/modified_recurring_event.ics @@ -0,0 +1,34 @@ +BEGIN:VCALENDAR +PRODID:-//Google Inc//Google Calendar 70.9054//EN +VERSION:2.0 +CALSCALE:GREGORIAN +METHOD:PUBLISH +X-WR-CALNAME:SRE_Magnet +X-WR-TIMEZONE:Europe/Berlin +BEGIN:VEVENT +DTSTART;VALUE=DATE:20230717 +DTEND;VALUE=DATE:20230725 +RRULE:FREQ=WEEKLY;WKST=MO;UNTIL=20230806;INTERVAL=3;BYDAY=MO +DTSTAMP:20230714T035605Z +UID:eventuid@google.com +CREATED:20230626T123728Z +LAST-MODIFIED:20230707T095912Z +SEQUENCE:1 +STATUS:CONFIRMED +SUMMARY:user +TRANSP:TRANSPARENT +END:VEVENT +BEGIN:VEVENT +DTSTART;TZID=Europe/Warsaw:20230717T100000 +DTEND;TZID=Europe/Warsaw:20230724T100000 +DTSTAMP:20230714T035605Z +UID:eventuid@google.com +RECURRENCE-ID;TZID=Europe/Warsaw:19700101T010000 +CREATED:20230626T123728Z +LAST-MODIFIED:20230707T095912Z +SEQUENCE:2 +STATUS:CONFIRMED +SUMMARY:user +TRANSP:TRANSPARENT +END:VEVENT +END:VCALENDAR \ No newline at end of file diff --git a/engine/apps/schedules/tests/test_ical_utils.py b/engine/apps/schedules/tests/test_ical_utils.py index fdd4b365..39951fef 100644 --- a/engine/apps/schedules/tests/test_ical_utils.py +++ b/engine/apps/schedules/tests/test_ical_utils.py @@ -304,6 +304,20 @@ def test_parse_event_uid_fallback(): assert source is None +def test_parse_recurrent_event_uid_fallback_modified(): + # use ical existing UID for imported events + event_uid = "someid@google.com" + pk, source = parse_event_uid(event_uid, sequence="2") + assert pk == f"{event_uid}_2" + assert source is None + pk, source = parse_event_uid(event_uid, recurrence_id="other-id") + assert pk == f"{event_uid}_other-id" + assert source is None + pk, source = parse_event_uid(event_uid, sequence="3", recurrence_id="other-id") + assert pk == f"{event_uid}_3_other-id" + assert source is None + + def test_is_icals_equal_compare_events(): with_vtimezone = textwrap.dedent( """ diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index 42ba8107..8c09cfc7 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -2477,3 +2477,22 @@ def test_swap_request_no_changes( events_after = schedule.filter_events(today, today + timezone.timedelta(days=2)) assert events_before == events_after + + +@pytest.mark.django_db +def test_filter_events_ical_duplicated_uid(make_organization, make_user_for_organization, make_schedule, get_ical): + calendar = get_ical("modified_recurring_event.ics") + organization = make_organization() + schedule = make_schedule(organization, schedule_class=OnCallScheduleCalendar) + schedule.cached_ical_file_primary = calendar.to_ical() + make_user_for_organization(organization, username="user") + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + datetime_start = datetime.datetime(2023, 7, 17, 0, 0, tzinfo=pytz.UTC) + datetime_end = datetime_start + datetime.timedelta(days=7) + events = schedule.final_events(datetime_start, datetime_end) + + assert len(events) == 2 + assert events[0]["shift"]["pk"] == "eventuid@google.com_1" + assert events[1]["shift"]["pk"] == "eventuid@google.com_2_1970-01-01T01:00:00+01:00" From 638c9a31427bc265c790ac3675b680c61c7dd765 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 8 Aug 2023 13:46:18 +0100 Subject: [PATCH 14/17] Add instruction on removing nullable fields from Django models (#2659) Adds an instruction on removing nullable fields without downtime. --- dev/README.md | 29 +++++++ engine/common/migrations/__init__.py | 0 engine/common/migrations/remove_field.py | 79 +++++++++++++++++++ .../management/commands/remove_field.py | 64 +++++++++++++++ 4 files changed, 172 insertions(+) create mode 100644 engine/common/migrations/__init__.py create mode 100644 engine/common/migrations/remove_field.py create mode 100644 engine/engine/management/commands/remove_field.py diff --git a/dev/README.md b/dev/README.md index 6e65d2a5..a800b8c3 100644 --- a/dev/README.md +++ b/dev/README.md @@ -427,3 +427,32 @@ backwards compatible See [django-migration-linter checklist](https://github.com/3YOURMIND/django-migration-linter/blob/main/docs/incompatibilities.md) for the common mistakes and best practices + +### Removing a nullable field from a model + +> This only works for nullable fields (fields with `null=True` in the field definition). +> +> DO NOT USE THIS APPROACH FOR NON-NULLABLE FIELDS, IT CAN BREAK THINGS! + +1. Remove all usages of the field you want to remove. Make sure the field is not used anywhere, including filtering, +querying, or explicit field referencing from views, models, forms, serializers, etc. +2. Remove the field from the model definition. +3. Generate migrations using the following management command: + + ```python + python manage.py remove_field + ``` + + Example: `python manage.py remove_field alerts AlertReceiveChannel restricted_at` + + This command will generate two migrations that **MUST BE DEPLOYED IN TWO SEPARATE RELEASES**: + - Migration #1 will remove the field from Django's state, but not from the database. Release #1 must include + migration #1, and must not include migration #2. + - Migration #2 will remove the field from the database. Stash this migration for use in a future release. + +4. Make release #1 (removal of the field + migration #1). Once released and deployed, Django will not be +aware of this field anymore, but the field will be still present in the database. This allows for a gradual migration, +where the field is no longer used in new code, but still exists in the database for backward compatibility with old code. +5. In any subsequent release, include migration #2 (the one that removes the field from the database). +6. After releasing and deploying migration #2, the field will be removed both from the database and Django state, +without backward compatibility issues or downtime 🎉 diff --git a/engine/common/migrations/__init__.py b/engine/common/migrations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/engine/common/migrations/remove_field.py b/engine/common/migrations/remove_field.py new file mode 100644 index 00000000..a079bb54 --- /dev/null +++ b/engine/common/migrations/remove_field.py @@ -0,0 +1,79 @@ +from django.db import connection +from django.db.migrations import RemoveField +from django.db.migrations.loader import MigrationLoader + + +class RemoveFieldState(RemoveField): + """ + Remove field from Django's migration state, but not from the database. + This is essentially the same as RemoveField, but database_forwards and database_backwards methods are modified + to do nothing. + """ + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + pass + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + pass + + def describe(self): + return f"{super().describe()} (state)" + + @property + def migration_name_fragment(self): + return f"{super().migration_name_fragment}_state" + + +class RemoveFieldDB(RemoveField): + """ + Remove field from the database, but not from Django's migration state. + This is implemented as a custom operation, because Django's RemoveField operation does not support + removing fields from the database after it has been removed from the state. The workaround is to use the state + that was in effect before the field was removed from the state (i.e. just before the RemoveFieldState migration). + """ + + def __init__(self, model_name, name, remove_state_migration): + """ + Specifying "remove_state_migration" allows database operations to run against a particular historical state. + Example: remove_state_migration = ("alerts", "0014_alertreceivechannel_restricted_at") will "trick" Django + into thinking that the last applied migration in the "alerts" app is 0013. + """ + super().__init__(model_name, name) + self.remove_state_migration = remove_state_migration + + def deconstruct(self): + """Update serialized representation of the operation.""" + deconstructed = super().deconstruct() + return ( + deconstructed[0], + deconstructed[1], + deconstructed[2] | {"remove_state_migration": self.remove_state_migration} + ) + + def state_forwards(self, app_label, state): + """Skip any state changes.""" + pass + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + # use historical state instead of what Django provides + from_state = self.state_before_remove_state_migration + + super().database_forwards(app_label, schema_editor, from_state, to_state) + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + # use historical state instead of what Django provides + to_state = self.state_before_remove_state_migration + + super().database_backwards(app_label, schema_editor, from_state, to_state) + + def describe(self): + return f"{super().describe()} (db)" + + @property + def migration_name_fragment(self): + return f"{super().migration_name_fragment}_db" + + @property + def state_before_remove_state_migration(self): + """Get project state just before migration "remove_state_migration" was applied.""" + return MigrationLoader(connection).project_state(self.remove_state_migration, at_end=False) diff --git a/engine/engine/management/commands/remove_field.py b/engine/engine/management/commands/remove_field.py new file mode 100644 index 00000000..fd7e2ef6 --- /dev/null +++ b/engine/engine/management/commands/remove_field.py @@ -0,0 +1,64 @@ +from django.core.management import BaseCommand +from django.db import connection +from django.db.migrations import Migration +from django.db.migrations.autodetector import MigrationAutodetector +from django.db.migrations.loader import MigrationLoader +from django.db.migrations.writer import MigrationWriter + +from common.migrations.remove_field import RemoveFieldDB, RemoveFieldState + + +class Command(BaseCommand): + """ + Generate two migrations that remove a field from the state and the database separately. + This allows removing a field in 2 separate releases and avoid downtime. + """ + + def add_arguments(self, parser): + parser.add_argument( + "args", nargs=3, help="app_label model_name field_name, example: alerts AlertReceiveChannel restricted_at" + ) + + def handle(self, *args, **options): + app_label, model_name, field_name = args + + # Check that the app, the model, and the field to be removed exist + project_state = MigrationLoader(connection).project_state() + model_state = project_state.apps.get_model(app_label, model_name) + model_state._meta.get_field(field_name) + + # Write migration that removes the field from the state + remove_state_migration = self.write_operation( + app_label, RemoveFieldState(model_name=model_name, name=field_name), project_state + ) + + # Write migration that removes the field from the database + self.write_operation( + app_label, + RemoveFieldDB( + model_name=model_name, name=field_name, remove_state_migration=(app_label, remove_state_migration.name) + ), + project_state, + ) + + @staticmethod + def write_operation(app_label, operation, project_state): + """ + Some Django magic to write a single-operation migration to a file, so it's similar to what Django would generate + when running the "makemigrations" command. + """ + + migration_class = type("Migration", (Migration,), {"operations": [operation]}) + + changes = MigrationAutodetector(project_state, project_state).arrange_for_graph( + changes={app_label: [migration_class(None, app_label)]}, + graph=MigrationLoader(connection).graph, + migration_name=operation.migration_name_fragment, + ) + + migration = changes[app_label][0] + writer = MigrationWriter(migration) + with open(writer.path, "w", encoding="utf-8") as file: + file.write(writer.as_string()) + + return migration From fd19dd422a5d1a6a4924cb2a17ddd19cbaa09b91 Mon Sep 17 00:00:00 2001 From: Ildar Iskhakov Date: Thu, 10 Aug 2023 10:25:00 +0800 Subject: [PATCH 15/17] Use periodic task for heartbeats (#2723) # What this PR does ## Which issue(s) this PR fixes ## 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) --------- Co-authored-by: Joey Orlando Co-authored-by: Michael Derynck --- CHANGELOG.md | 1 + engine/apps/heartbeat/models.py | 86 +++--------- engine/apps/heartbeat/tasks.py | 130 ++++++++++++------ .../tests/test_integration_heartbeat.py | 130 +++++++++--------- .../metadata/heartbeat/elastalert.py | 16 +-- .../metadata/heartbeat/formatted_webhook.py | 12 +- .../metadata/heartbeat/grafana.py | 12 +- .../integrations/metadata/heartbeat/prtg.py | 12 +- .../metadata/heartbeat/webhook.py | 14 +- .../integrations/metadata/heartbeat/zabbix.py | 12 +- .../apps/integrations/tests/test_ratelimit.py | 2 - engine/apps/integrations/views.py | 11 +- engine/config_integrations/elastalert.py | 9 +- engine/config_integrations/webhook.py | 17 ++- engine/settings/base.py | 5 + 15 files changed, 239 insertions(+), 230 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3800828..a5fde585 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add stack slug to organization options for direct paging Slash command by @vadimkerr ([#2743](https://github.com/grafana/oncall/pull/2743)) - Avoid creating (or notifying about) potential event splits resulting from untaken swap requests ([#2748](https://github.com/grafana/oncall/pull/2748)) +- Refactor heartbeats into a periodic task ([2723](https://github.com/grafana/oncall/pull/2723)) ### Fixed diff --git a/engine/apps/heartbeat/models.py b/engine/apps/heartbeat/models.py index eb9b9cd6..9d8018df 100644 --- a/engine/apps/heartbeat/models.py +++ b/engine/apps/heartbeat/models.py @@ -4,10 +4,9 @@ from urllib.parse import urljoin from django.conf import settings from django.core.validators import MinLengthValidator -from django.db import models, transaction +from django.db import models from django.utils import timezone -from apps.integrations.tasks import create_alert from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length logger = logging.getLogger(__name__) @@ -43,10 +42,26 @@ class IntegrationHeartBeat(models.Model): created_at = models.DateTimeField(auto_now_add=True) timeout_seconds = models.IntegerField(default=0) + last_heartbeat_time = models.DateTimeField(default=None, null=True) + """ + Stores the latest received heartbeat signal time + """ + last_checkup_task_time = models.DateTimeField(default=None, null=True) + """ + Deprecated. This field is not used. TODO: remove it + """ + actual_check_up_task_id = models.CharField(max_length=100) + """ + Deprecated. Stored the latest scheduled `integration_heartbeat_checkup` task id. TODO: remove it + """ + previous_alerted_state_was_life = models.BooleanField(default=True) + """ + Last status of the heartbeat. Determines if integration was alive on latest checkup + """ public_primary_key = models.CharField( max_length=20, @@ -83,73 +98,6 @@ class IntegrationHeartBeat(models.Model): def link(self) -> str: return urljoin(self.alert_receive_channel.integration_url, "heartbeat/") - @classmethod - def perform_heartbeat_check(cls, heartbeat_id: int, task_request_id: str) -> None: - with transaction.atomic(): - heartbeats = cls.objects.filter(pk=heartbeat_id).select_for_update() - if len(heartbeats) == 0: - logger.info(f"Heartbeat {heartbeat_id} not found {task_request_id}") - return - heartbeat = heartbeats[0] - if task_request_id == heartbeat.actual_check_up_task_id: - heartbeat.check_heartbeat_state_and_save() - else: - logger.info(f"Heartbeat {heartbeat_id} is not actual {task_request_id}") - - def check_heartbeat_state_and_save(self) -> bool: - """ - Use this method if you want just check heartbeat status. - """ - state_changed = self.check_heartbeat_state() - if state_changed: - self.save(update_fields=["previous_alerted_state_was_life"]) - return state_changed - - def check_heartbeat_state(self) -> bool: - """ - Actually checking heartbeat. - Use this method if you want to do changes of heartbeat instance while checking its status. - ( See IntegrationHeartBeatAPIView.post() for example ) - """ - state_changed = False - if self.is_expired: - if self.previous_alerted_state_was_life: - self.on_heartbeat_expired() - self.previous_alerted_state_was_life = False - state_changed = True - else: - if not self.previous_alerted_state_was_life: - self.on_heartbeat_restored() - self.previous_alerted_state_was_life = True - state_changed = True - return state_changed - - def on_heartbeat_restored(self) -> None: - create_alert.apply_async( - kwargs={ - "title": self.alert_receive_channel.heartbeat_restored_title, - "message": self.alert_receive_channel.heartbeat_restored_message, - "image_url": None, - "link_to_upstream_details": None, - "alert_receive_channel_pk": self.alert_receive_channel.pk, - "integration_unique_data": {}, - "raw_request_data": self.alert_receive_channel.heartbeat_restored_payload, - }, - ) - - def on_heartbeat_expired(self) -> None: - create_alert.apply_async( - kwargs={ - "title": self.alert_receive_channel.heartbeat_expired_title, - "message": self.alert_receive_channel.heartbeat_expired_message, - "image_url": None, - "link_to_upstream_details": None, - "alert_receive_channel_pk": self.alert_receive_channel.pk, - "integration_unique_data": {}, - "raw_request_data": self.alert_receive_channel.heartbeat_expired_payload, - }, - ) - # Insight logs @property def insight_logs_type_verbal(self) -> str: diff --git a/engine/apps/heartbeat/tasks.py b/engine/apps/heartbeat/tasks.py index 071af2fb..d02fac9b 100644 --- a/engine/apps/heartbeat/tasks.py +++ b/engine/apps/heartbeat/tasks.py @@ -1,57 +1,105 @@ -from time import perf_counter +import datetime from celery.utils.log import get_task_logger +from django.conf import settings from django.db import transaction +from django.db.models import DateTimeField, DurationField, ExpressionWrapper, F +from django.db.models.functions import Cast from django.utils import timezone +from apps.heartbeat.models import IntegrationHeartBeat +from apps.integrations.tasks import create_alert from common.custom_celery_tasks import shared_dedicated_queue_retry_task +from settings.base import DatabaseTypes logger = get_task_logger(__name__) @shared_dedicated_queue_retry_task() -def integration_heartbeat_checkup(heartbeat_id: int) -> None: - from apps.heartbeat.models import IntegrationHeartBeat +def check_heartbeats() -> str: + """ + Periodic task to check heartbeats status change and create alerts (or auto-resolve alerts) if needed + """ + # Heartbeat is considered enabled if it + # * has timeout_seconds set to non-zero (non-default) value, + # * received at least one checkup (last_heartbeat_time set to non-null value)\ - IntegrationHeartBeat.perform_heartbeat_check(heartbeat_id, integration_heartbeat_checkup.request.id) + def _get_timeout_expression() -> ExpressionWrapper: + if settings.DATABASES["default"]["ENGINE"] == f"django.db.backends.{DatabaseTypes.POSTGRESQL}": + # DurationField: When used on PostgreSQL, the data type used is an interval + # https://docs.djangoproject.com/en/3.2/ref/models/fields/#durationfield + return ExpressionWrapper(datetime.timedelta(seconds=1) * F("timeout_seconds"), output_field=DurationField()) + else: + # DurationField: ...Otherwise a bigint of microseconds is used... + # microseconds = seconds * 10**6 + # https://docs.djangoproject.com/en/3.2/ref/models/fields/#durationfield + return ExpressionWrapper(F("timeout_seconds") * 10**6, output_field=DurationField()) + + enabled_heartbeats = ( + IntegrationHeartBeat.objects.filter(last_heartbeat_time__isnull=False) + .exclude(timeout_seconds=0) + .annotate(period_start=(Cast(timezone.now() - _get_timeout_expression(), DateTimeField()))) + ) + with transaction.atomic(): + # Heartbeat is considered expired if it + # * is enabled, + # * is not already expired, + # * last check in was before the timeout period start + expired_heartbeats = enabled_heartbeats.select_for_update().filter( + last_heartbeat_time__lte=F("period_start"), previous_alerted_state_was_life=True + ) + # Schedule alert creation for each expired heartbeat after transaction commit + for heartbeat in expired_heartbeats: + transaction.on_commit( + lambda: create_alert.apply_async( + kwargs={ + "title": heartbeat.alert_receive_channel.heartbeat_expired_title, + "message": heartbeat.alert_receive_channel.heartbeat_expired_message, + "image_url": None, + "link_to_upstream_details": None, + "alert_receive_channel_pk": heartbeat.alert_receive_channel.pk, + "integration_unique_data": {}, + "raw_request_data": heartbeat.alert_receive_channel.heartbeat_expired_payload, + }, + ) + ) + # Update previous_alerted_state_was_life to False + expired_count = expired_heartbeats.update(previous_alerted_state_was_life=False) + with transaction.atomic(): + # Heartbeat is considered restored if it + # * is enabled, + # * last check in was after the timeout period start, + # * was is alerted state (previous_alerted_state_was_life is False), i.e. was expired + restored_heartbeats = enabled_heartbeats.select_for_update().filter( + last_heartbeat_time__gte=F("period_start"), previous_alerted_state_was_life=False + ) + # Schedule auto-resolve alert creation for each expired heartbeat after transaction commit + for heartbeat in restored_heartbeats: + transaction.on_commit( + lambda: create_alert.apply_async( + kwargs={ + "title": heartbeat.alert_receive_channel.heartbeat_restored_title, + "message": heartbeat.alert_receive_channel.heartbeat_restored_message, + "image_url": None, + "link_to_upstream_details": None, + "alert_receive_channel_pk": heartbeat.alert_receive_channel.pk, + "integration_unique_data": {}, + "raw_request_data": heartbeat.alert_receive_channel.heartbeat_restored_payload, + }, + ) + ) + restored_count = restored_heartbeats.update(previous_alerted_state_was_life=True) + return f"Found {expired_count} expired and {restored_count} restored heartbeats" + + +@shared_dedicated_queue_retry_task() +def integration_heartbeat_checkup(heartbeat_id: int) -> None: + """Deprecated. TODO: Remove this task after this task cleared from queue""" + pass @shared_dedicated_queue_retry_task() def process_heartbeat_task(alert_receive_channel_pk): - start = perf_counter() - from apps.heartbeat.models import IntegrationHeartBeat - - with transaction.atomic(): - heartbeats = IntegrationHeartBeat.objects.filter( - alert_receive_channel__pk=alert_receive_channel_pk, - ).select_for_update() - if len(heartbeats) == 0: - logger.info(f"Integration Heartbeat for alert_receive_channel {alert_receive_channel_pk} was not found.") - return - else: - heartbeat = heartbeats[0] - heartbeat_selected = perf_counter() - logger.info( - f"IntegrationHeartBeat selected for alert_receive_channel {alert_receive_channel_pk} in {heartbeat_selected - start}" - ) - task = integration_heartbeat_checkup.apply_async( - (heartbeat.pk,), - countdown=heartbeat.timeout_seconds + 1, - ) - is_touched = heartbeat.last_heartbeat_time is not None - heartbeat.actual_check_up_task_id = task.id - heartbeat.last_heartbeat_time = timezone.now() - update_fields = ["actual_check_up_task_id", "last_heartbeat_time"] - task_started = perf_counter() - logger.info( - f"heartbeat_checkup task started for alert_receive_channel {alert_receive_channel_pk} in {task_started - start}" - ) - if is_touched: - state_changed = heartbeat.check_heartbeat_state() - state_checked = perf_counter() - logger.info( - f"state checked for alert_receive_channel {alert_receive_channel_pk} in {state_checked - start}" - ) - if state_changed: - update_fields.append("previous_alerted_state_was_life") - heartbeat.save(update_fields=update_fields) + IntegrationHeartBeat.objects.filter( + alert_receive_channel__pk=alert_receive_channel_pk, + ).update(last_heartbeat_time=timezone.now()) diff --git a/engine/apps/heartbeat/tests/test_integration_heartbeat.py b/engine/apps/heartbeat/tests/test_integration_heartbeat.py index c3797dcf..e2cdb8c3 100644 --- a/engine/apps/heartbeat/tests/test_integration_heartbeat.py +++ b/engine/apps/heartbeat/tests/test_integration_heartbeat.py @@ -4,83 +4,77 @@ import pytest from django.utils import timezone from apps.alerts.models import AlertReceiveChannel +from apps.heartbeat.tasks import check_heartbeats +from apps.integrations.tasks import create_alert @pytest.mark.django_db -@patch("apps.heartbeat.models.IntegrationHeartBeat.on_heartbeat_expired", return_value=None) @pytest.mark.parametrize("integration", [AlertReceiveChannel.INTEGRATION_FORMATTED_WEBHOOK]) -def test_integration_heartbeat_expired( - mocked_handler, make_organization_and_user, make_alert_receive_channel, make_integration_heartbeat, integration +def test_check_heartbeats( + make_organization_and_user, + make_alert_receive_channel, + make_integration_heartbeat, + integration, + django_capture_on_commit_callbacks, ): + # No heartbeats, nothing happens + with patch.object(create_alert, "apply_async") as mock_create_alert_apply_async: + with django_capture_on_commit_callbacks(execute=True): + result = check_heartbeats() + assert result == "Found 0 expired and 0 restored heartbeats" + assert mock_create_alert_apply_async.call_count == 0 + + # Prepare heartbeat team, _ = make_organization_and_user() - # Some short timeout and last_heartbeat_time to make sure that heartbeat is expired - timeout = 1 - last_heartbeat_time = timezone.now() - timezone.timedelta(seconds=timeout * 10) - alert_receive_channel = make_alert_receive_channel(team, integration=integration) - integration_heartbeat = make_integration_heartbeat( - alert_receive_channel, timeout, last_heartbeat_time=last_heartbeat_time - ) - integration_heartbeat.check_heartbeat_state_and_save() - assert mocked_handler.called - - -@pytest.mark.django_db -@patch("apps.heartbeat.models.IntegrationHeartBeat.on_heartbeat_expired", return_value=None) -@pytest.mark.parametrize("integration", [AlertReceiveChannel.INTEGRATION_FORMATTED_WEBHOOK]) -def test_integration_heartbeat_already_expired( - mocked_handler, make_organization_and_user, make_alert_receive_channel, make_integration_heartbeat, integration -): - team, _ = make_organization_and_user() - # Some short timeout and last_heartbeat_time to make sure that heartbeat is expired - timeout = 1 - last_heartbeat_time = timezone.now() - timezone.timedelta(seconds=timeout * 10) - alert_receive_channel = make_alert_receive_channel(team, integration=integration) - integration_heartbeat = make_integration_heartbeat( - alert_receive_channel, - timeout, - last_heartbeat_time=last_heartbeat_time, - previous_alerted_state_was_life=False, - ) - integration_heartbeat.check_heartbeat_state_and_save() - assert mocked_handler.called is False - - -@pytest.mark.django_db -@patch("apps.heartbeat.models.IntegrationHeartBeat.on_heartbeat_restored", return_value=None) -@pytest.mark.parametrize("integration", [AlertReceiveChannel.INTEGRATION_FORMATTED_WEBHOOK]) -def test_integration_heartbeat_restored( - mocked_handler, make_organization_and_user, make_alert_receive_channel, make_integration_heartbeat, integration -): - team, _ = make_organization_and_user() - # Some long timeout and last_heartbeat_time to make sure that heartbeat is not expired - timeout = 1000 + timeout = 60 last_heartbeat_time = timezone.now() alert_receive_channel = make_alert_receive_channel(team, integration=integration) integration_heartbeat = make_integration_heartbeat( - alert_receive_channel, - timeout, - last_heartbeat_time=last_heartbeat_time, - previous_alerted_state_was_life=False, + alert_receive_channel, timeout, last_heartbeat_time=last_heartbeat_time, previous_alerted_state_was_life=True ) - integration_heartbeat.check_heartbeat_state_and_save() - assert mocked_handler.called + # Heartbeat is alive, nothing happens + with patch.object(create_alert, "apply_async") as mock_create_alert_apply_async: + with django_capture_on_commit_callbacks(execute=True): + result = check_heartbeats() + assert result == "Found 0 expired and 0 restored heartbeats" + assert mock_create_alert_apply_async.call_count == 0 -@pytest.mark.django_db -@patch("apps.heartbeat.models.IntegrationHeartBeat.on_heartbeat_restored", return_value=None) -@pytest.mark.parametrize("integration", [AlertReceiveChannel.INTEGRATION_FORMATTED_WEBHOOK]) -def test_integration_heartbeat_restored_and_alert_was_not_sent( - mocked_handler, make_organization_and_user, make_alert_receive_channel, make_integration_heartbeat, integration -): - team, _ = make_organization_and_user() - # Some long timeout and last_heartbeat_time to make sure that heartbeat is not expired - timeout = 1000 - last_heartbeat_time = timezone.now() - alert_receive_channel = make_alert_receive_channel(team, integration=integration) - integration_heartbeat = make_integration_heartbeat( - alert_receive_channel, - timeout, - last_heartbeat_time=last_heartbeat_time, - ) - integration_heartbeat.check_heartbeat_state_and_save() - assert mocked_handler.called is False + # Hearbeat expires, send an alert + integration_heartbeat.refresh_from_db() + integration_heartbeat.last_heartbeat_time = timezone.now() - timezone.timedelta(seconds=timeout * 10) + integration_heartbeat.save() + with patch.object(create_alert, "apply_async") as mock_create_alert_apply_async: + with django_capture_on_commit_callbacks(execute=True): + result = check_heartbeats() + assert result == "Found 1 expired and 0 restored heartbeats" + assert mock_create_alert_apply_async.call_count == 1 + + # Heartbeat is still expired, nothing happens + integration_heartbeat.refresh_from_db() + with patch.object(create_alert, "apply_async") as mock_create_alert_apply_async: + with django_capture_on_commit_callbacks(execute=True): + result = check_heartbeats() + assert result == "Found 0 expired and 0 restored heartbeats" + assert mock_create_alert_apply_async.call_count == 0 + + # Hearbeat restored, send an auto-resolve alert + integration_heartbeat.refresh_from_db() + integration_heartbeat.last_heartbeat_time = timezone.now() + integration_heartbeat.save() + with patch.object(create_alert, "apply_async") as mock_create_alert_apply_async: + with django_capture_on_commit_callbacks(execute=True): + result = check_heartbeats() + assert result == "Found 0 expired and 1 restored heartbeats" + assert mock_create_alert_apply_async.call_count == 1 + + # Heartbeat is alive, nothing happens + integration_heartbeat.refresh_from_db() + integration_heartbeat.last_heartbeat_time = timezone.now() + integration_heartbeat.save() + integration_heartbeat.refresh_from_db() + with patch.object(create_alert, "apply_async") as mock_create_alert_apply_async: + with django_capture_on_commit_callbacks(execute=True): + result = check_heartbeats() + assert result == "Found 0 expired and 0 restored heartbeats" + assert mock_create_alert_apply_async.call_count == 0 diff --git a/engine/apps/integrations/metadata/heartbeat/elastalert.py b/engine/apps/integrations/metadata/heartbeat/elastalert.py index 04a05d67..8394577e 100644 --- a/engine/apps/integrations/metadata/heartbeat/elastalert.py +++ b/engine/apps/integrations/metadata/heartbeat/elastalert.py @@ -12,12 +12,12 @@ heartbeat_expired_message = heartbeat_text.heartbeat_expired_message heartbeat_expired_payload = { "alert_uid": "0eaf37c8-e1eb-4714-b79e-7c648b6a96fa", "title": heartbeat_expired_title, - "image_url": None, "state": "alerting", - "link_to_upstream_details": None, "message": heartbeat_expired_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": False, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": False, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": False, # Keep for backwards compatibility } heartbeat_restored_title = heartbeat_text.heartbeat_restored_title @@ -26,10 +26,10 @@ heartbeat_restored_message = heartbeat_text.heartbeat_restored_message heartbeat_restored_payload = { "alert_uid": "0eaf37c8-e1eb-4714-b79e-7c648b6a96fa", "title": heartbeat_restored_title, - "image_url": None, "state": "ok", - "link_to_upstream_details": None, "message": heartbeat_restored_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": True, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": True, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": True, # Keep for backwards compatibility } diff --git a/engine/apps/integrations/metadata/heartbeat/formatted_webhook.py b/engine/apps/integrations/metadata/heartbeat/formatted_webhook.py index 3e44b57e..72278b15 100644 --- a/engine/apps/integrations/metadata/heartbeat/formatted_webhook.py +++ b/engine/apps/integrations/metadata/heartbeat/formatted_webhook.py @@ -17,8 +17,10 @@ heartbeat_expired_payload = { "state": "alerting", "link_to_upstream_details": None, "message": heartbeat_expired_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": False, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": False, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": False, # Keep for backwards compatibility } heartbeat_restored_title = heartbeat_text.heartbeat_restored_title @@ -31,6 +33,8 @@ heartbeat_restored_payload = { "state": "ok", "link_to_upstream_details": None, "message": heartbeat_restored_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": True, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": True, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": True, # Keep for backwards compatibility } diff --git a/engine/apps/integrations/metadata/heartbeat/grafana.py b/engine/apps/integrations/metadata/heartbeat/grafana.py index a71011ed..67954ab9 100644 --- a/engine/apps/integrations/metadata/heartbeat/grafana.py +++ b/engine/apps/integrations/metadata/heartbeat/grafana.py @@ -14,8 +14,10 @@ heartbeat_expired_payload = { "state": "alerting", "title": heartbeat_expired_title, "message": heartbeat_expired_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": False, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": False, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": False, # Keep for backwards compatibility } heartbeat_restored_title = f"[OK] {heartbeat_text.heartbeat_restored_title}" @@ -25,6 +27,8 @@ heartbeat_restored_payload = { "state": "ok", "title": heartbeat_restored_title, "message": heartbeat_restored_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": True, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": True, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": True, # Keep for backwards compatibility } diff --git a/engine/apps/integrations/metadata/heartbeat/prtg.py b/engine/apps/integrations/metadata/heartbeat/prtg.py index ddf16333..42c6965c 100644 --- a/engine/apps/integrations/metadata/heartbeat/prtg.py +++ b/engine/apps/integrations/metadata/heartbeat/prtg.py @@ -17,8 +17,10 @@ heartbeat_expired_payload = { "state": "alerting", "link_to_upstream_details": None, "message": heartbeat_expired_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": False, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": False, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": False, # Keep for backwards compatibility } heartbeat_restored_title = heartbeat_text.heartbeat_restored_title @@ -31,6 +33,8 @@ heartbeat_restored_payload = { "state": "ok", "link_to_upstream_details": None, "message": heartbeat_restored_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": True, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": True, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": True, # Keep for backwards compatibility } diff --git a/engine/apps/integrations/metadata/heartbeat/webhook.py b/engine/apps/integrations/metadata/heartbeat/webhook.py index e6283e36..b3bf3504 100644 --- a/engine/apps/integrations/metadata/heartbeat/webhook.py +++ b/engine/apps/integrations/metadata/heartbeat/webhook.py @@ -13,12 +13,12 @@ heartbeat_expired_message = heartbeat_text.heartbeat_expired_message heartbeat_expired_payload = { "alert_uid": "7973c835-ff3f-46e4-9444-06df127b6f8e", "title": heartbeat_expired_title, - "image_url": None, "state": "alerting", - "link_to_upstream_details": None, "message": heartbeat_expired_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": False, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": False, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": False, # Keep for backwards compatibility } heartbeat_restored_title = heartbeat_text.heartbeat_restored_title @@ -31,6 +31,8 @@ heartbeat_restored_payload = { "state": "ok", "link_to_upstream_details": None, "message": heartbeat_restored_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": True, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": True, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": True, # Keep for backwards compatibility } diff --git a/engine/apps/integrations/metadata/heartbeat/zabbix.py b/engine/apps/integrations/metadata/heartbeat/zabbix.py index b336b75b..d961bd83 100644 --- a/engine/apps/integrations/metadata/heartbeat/zabbix.py +++ b/engine/apps/integrations/metadata/heartbeat/zabbix.py @@ -16,8 +16,10 @@ heartbeat_expired_payload = { "state": "alerting", "link_to_upstream_details": None, "message": heartbeat_expired_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": False, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": False, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": False, # Keep for backwards compatibility } heartbeat_restored_title = heartbeat_text.heartbeat_restored_title @@ -30,6 +32,8 @@ heartbeat_restored_payload = { "state": "ok", "link_to_upstream_details": None, "message": heartbeat_restored_message, - "is_amixr_heartbeat": True, - "is_amixr_heartbeat_restored": True, + "is_oncall_heartbeat": True, + "is_oncall_heartbeat_restored": True, + "is_amixr_heartbeat": True, # Keep for backwards compatibility + "is_amixr_heartbeat_restored": True, # Keep for backwards compatibility } diff --git a/engine/apps/integrations/tests/test_ratelimit.py b/engine/apps/integrations/tests/test_ratelimit.py index 97b56937..5598c4f7 100644 --- a/engine/apps/integrations/tests/test_ratelimit.py +++ b/engine/apps/integrations/tests/test_ratelimit.py @@ -96,5 +96,3 @@ def test_ratelimit_integration_heartbeats( response = c.get(url) assert response.status_code == 429 - - assert mocked_task.call_count == 1 diff --git a/engine/apps/integrations/views.py b/engine/apps/integrations/views.py index 638df507..c694d87e 100644 --- a/engine/apps/integrations/views.py +++ b/engine/apps/integrations/views.py @@ -3,6 +3,7 @@ import logging from django.conf import settings from django.core.exceptions import PermissionDenied +from django.db import OperationalError from django.http import HttpResponseBadRequest, JsonResponse from django.utils.decorators import method_decorator from django.views.decorators.csrf import csrf_exempt @@ -324,6 +325,10 @@ class IntegrationHeartBeatAPIView(AlertChannelDefiningMixin, IntegrationHeartBea return Response(status=200) def _process_heartbeat_signal(self, request, alert_receive_channel): - process_heartbeat_task.apply_async( - (alert_receive_channel.pk,), - ) + try: + process_heartbeat_task(alert_receive_channel.pk) + # If database is not ready, fallback to celery task + except OperationalError: + process_heartbeat_task.apply_async( + (alert_receive_channel.pk,), + ) diff --git a/engine/config_integrations/elastalert.py b/engine/config_integrations/elastalert.py index 2cd00b1a..a2e13b30 100644 --- a/engine/config_integrations/elastalert.py +++ b/engine/config_integrations/elastalert.py @@ -46,14 +46,7 @@ source_link = None grouping_id = '{{ payload.get("alert_uid", "")}}' -resolve_condition = """\ -{%- if "is_amixr_heartbeat_restored" in payload -%} -{# We don't know the payload format from your integration. #} -{# The heartbeat alerts will go here so we check for our own key #} -{{ payload["is_amixr_heartbeat_restored"] }} -{%- else -%} -{{ payload.get("state", "").upper() == "OK" }} -{%- endif %}""" +resolve_condition = """{{ payload.get("state", "").upper() == "OK" }}""" acknowledge_condition = None diff --git a/engine/config_integrations/webhook.py b/engine/config_integrations/webhook.py index 945cbb5f..a492ef72 100644 --- a/engine/config_integrations/webhook.py +++ b/engine/config_integrations/webhook.py @@ -45,16 +45,15 @@ telegram_image_url = slack_image_url source_link = "{{ payload.url }}" -grouping_id = "{{ payload }}" +grouping_id = """\ +{% if "is_oncall_heartbeat" in payload %} +{# Case for heartbeat alerts generated by Grafana OnCall #} +{{- payload.alert_uid }} +{% else %} +{{- payload }} +{% endif %}""" -resolve_condition = """\ -{%- if "is_amixr_heartbeat_restored" in payload -%} -{# We don't know the payload format from your integration. #} -{# The heartbeat alerts will go here so we check for our own key #} -{{ payload["is_amixr_heartbeat_restored"] }} -{%- else -%} -{{ payload.get("state", "").upper() == "OK" }} -{%- endif %}""" +resolve_condition = """{{ payload.get("state", "").upper() == "OK" }}""" acknowledge_condition = None example_payload = {"message": "This alert was sent by user for demonstration purposes"} diff --git a/engine/settings/base.py b/engine/settings/base.py index 0d2bc46a..e598bd17 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -488,6 +488,11 @@ CELERY_BEAT_SCHEDULE = { "schedule": 60 * 30, "args": (), }, + "check_heartbeats": { + "task": "apps.heartbeat.tasks.check_heartbeats", + "schedule": crontab(minute="*/2"), # every 2 minutes + "args": (), + }, } if ESCALATION_AUDITOR_ENABLED: From e87bb912ae843af479791fb389b6e3033182437a Mon Sep 17 00:00:00 2001 From: Rares Mardare Date: Thu, 10 Aug 2023 09:35:32 +0300 Subject: [PATCH 16/17] Use UserSettingsWrite instead of OtherSettingsWrite for phone tab (#2772) # What this PR does Sets the correct permission when accessing Phone Verification screen (Cloud/OSS) ## Which issue(s) this PR fixes Fix for https://github.com/grafana/oncall/issues/2761 ## 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) --- CHANGELOG.md | 1 + .../parts/tabs/CloudPhoneSettings/CloudPhoneSettings.tsx | 2 +- .../parts/tabs/PhoneVerification/PhoneVerification.tsx | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5fde585..8b896403 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Do not show override shortcut when web overrides are disabled ([#2745](https://github.com/grafana/oncall/pull/2745)) - Handle ical schedule import with duplicated event UIDs ([#2760](https://github.com/grafana/oncall/pull/2760)) +- Allow Editor to access Phone Verification ([#2772](https://github.com/grafana/oncall/pull/2772)) ## v1.3.22 (2023-08-03) diff --git a/grafana-plugin/src/containers/UserSettings/parts/tabs/CloudPhoneSettings/CloudPhoneSettings.tsx b/grafana-plugin/src/containers/UserSettings/parts/tabs/CloudPhoneSettings/CloudPhoneSettings.tsx index 0b7614d1..29809d02 100644 --- a/grafana-plugin/src/containers/UserSettings/parts/tabs/CloudPhoneSettings/CloudPhoneSettings.tsx +++ b/grafana-plugin/src/containers/UserSettings/parts/tabs/CloudPhoneSettings/CloudPhoneSettings.tsx @@ -112,7 +112,7 @@ const CloudPhoneSettings = observer((props: CloudPhoneSettingsProps) => { return ( diff --git a/grafana-plugin/src/containers/UserSettings/parts/tabs/PhoneVerification/PhoneVerification.tsx b/grafana-plugin/src/containers/UserSettings/parts/tabs/PhoneVerification/PhoneVerification.tsx index f7ffe8e7..a354c0f0 100644 --- a/grafana-plugin/src/containers/UserSettings/parts/tabs/PhoneVerification/PhoneVerification.tsx +++ b/grafana-plugin/src/containers/UserSettings/parts/tabs/PhoneVerification/PhoneVerification.tsx @@ -188,7 +188,7 @@ const PhoneVerification = observer((props: PhoneVerificationProps) => { } return ( - + {isPhoneValid && !user.verified_phone_number && ( From 5fcff31e4ab24228e655e40074ea425b44dbf2cb Mon Sep 17 00:00:00 2001 From: Yulya Artyukhina Date: Thu, 10 Aug 2023 10:13:23 +0200 Subject: [PATCH 17/17] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b896403..0042dd69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## Unreleased +## v1.3.23 (2023-08-10) ### Added