From dd598ecf21458a4c36dd3bb680926117d6442c97 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Mon, 24 Apr 2023 13:19:40 -0400 Subject: [PATCH 01/13] add info_notifications_enabled column to user mobile app settings --- ...usersettings_info_notifications_enabled.py | 26 +++++++++++++++++++ engine/apps/mobile_app/models.py | 4 +++ engine/apps/mobile_app/serializers.py | 1 + .../mobile_app/tests/test_user_settings.py | 2 ++ 4 files changed, 33 insertions(+) create mode 100644 engine/apps/mobile_app/migrations/0004_mobileappusersettings_info_notifications_enabled.py diff --git a/engine/apps/mobile_app/migrations/0004_mobileappusersettings_info_notifications_enabled.py b/engine/apps/mobile_app/migrations/0004_mobileappusersettings_info_notifications_enabled.py new file mode 100644 index 0000000000..77a75ef8c0 --- /dev/null +++ b/engine/apps/mobile_app/migrations/0004_mobileappusersettings_info_notifications_enabled.py @@ -0,0 +1,26 @@ +# Generated by Django 3.2.18 on 2023-04-24 16:48 + +from django.db import migrations, models +from django_add_default_value import AddDefaultValue + + +class Migration(migrations.Migration): + + dependencies = [ + ('mobile_app', '0003_mobileappusersettings'), + ] + + operations = [ + migrations.AddField( + model_name='mobileappusersettings', + name='info_notifications_enabled', + field=models.BooleanField(default=True), + ), + # migrations.AddField enforces the default value on the app level, which leads to the issues during release + # adding same default value on the database level + AddDefaultValue( + model_name='mobileappusersettings', + name='info_notifications_enabled', + value=True, + ), + ] diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index 92d0e7f83c..0ed6c64958 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -107,3 +107,7 @@ class VolumeType(models.TextChoices): # For the "Mobile push important" step it's possible to make notifications non-critical # if "override DND" setting is disabled in the app important_notification_override_dnd = models.BooleanField(default=True) + + # this is used for non escalation related push notifications such as the + # "You're going OnCall soon" push notification + info_notifications_enabled = models.BooleanField(default=True) diff --git a/engine/apps/mobile_app/serializers.py b/engine/apps/mobile_app/serializers.py index 1338ecdc97..87f396580d 100644 --- a/engine/apps/mobile_app/serializers.py +++ b/engine/apps/mobile_app/serializers.py @@ -15,4 +15,5 @@ class Meta: "important_notification_volume_type", "important_notification_volume", "important_notification_override_dnd", + "info_notifications_enabled", ) diff --git a/engine/apps/mobile_app/tests/test_user_settings.py b/engine/apps/mobile_app/tests/test_user_settings.py index de14d9dfc8..d8fde665b6 100644 --- a/engine/apps/mobile_app/tests/test_user_settings.py +++ b/engine/apps/mobile_app/tests/test_user_settings.py @@ -24,6 +24,7 @@ def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume_type": "constant", "important_notification_volume": 0.8, "important_notification_override_dnd": True, + "info_notifications_enabled": True, } @@ -42,6 +43,7 @@ def test_user_settings_put(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume_type": "intensifying", "important_notification_volume": 1, "important_notification_override_dnd": False, + "info_notifications_enabled": False, } response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=auth_token) From e9233c761e2a3a86ce14678766d3752c8d0d796c Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 25 Apr 2023 08:28:27 -0400 Subject: [PATCH 02/13] wip --- ...ions_enabled.py => 0004_auto_20230425_1033.py} | 15 ++++++++++++++- engine/apps/mobile_app/models.py | 11 +++++++++++ engine/apps/mobile_app/serializers.py | 1 + .../apps/mobile_app/tests/test_user_settings.py | 4 +++- 4 files changed, 29 insertions(+), 2 deletions(-) rename engine/apps/mobile_app/migrations/{0004_mobileappusersettings_info_notifications_enabled.py => 0004_auto_20230425_1033.py} (52%) diff --git a/engine/apps/mobile_app/migrations/0004_mobileappusersettings_info_notifications_enabled.py b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py similarity index 52% rename from engine/apps/mobile_app/migrations/0004_mobileappusersettings_info_notifications_enabled.py rename to engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py index 77a75ef8c0..ccfc2c29ef 100644 --- a/engine/apps/mobile_app/migrations/0004_mobileappusersettings_info_notifications_enabled.py +++ b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.18 on 2023-04-24 16:48 +# Generated by Django 3.2.18 on 2023-04-25 10:33 from django.db import migrations, models from django_add_default_value import AddDefaultValue @@ -23,4 +23,17 @@ class Migration(migrations.Migration): name='info_notifications_enabled', value=True, ), + + migrations.AddField( + model_name='mobileappusersettings', + name='going_oncall_notification_timing', + field=models.IntegerField(choices=[(0, 'one hour before'), (1, 'twelve hours before'), (2, 'one day before'), (3, 'one week before')], default=1), + ), + # migrations.AddField enforces the default value on the app level, which leads to the issues during release + # adding same default value on the database level + AddDefaultValue( + model_name='mobileappusersettings', + name='going_oncall_notification_timing', + value=1, + ), ] diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index 0ed6c64958..040bda2d48 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -111,3 +111,14 @@ class VolumeType(models.TextChoices): # this is used for non escalation related push notifications such as the # "You're going OnCall soon" push notification info_notifications_enabled = models.BooleanField(default=True) + + # these choices + the below column are used to calculate when to send the "You're Going OnCall soon" + # push notification + ONE_HOUR, TWELVE_HOURS, ONE_DAY, ONE_WEEK = range(4) + NOTIFICATION_TIMING_CHOICES = ( + (ONE_HOUR, "one hour before"), + (TWELVE_HOURS, "twelve hours before"), + (ONE_DAY, "one day before"), + (ONE_WEEK, "one week before"), + ) + going_oncall_notification_timing = models.IntegerField(choices=NOTIFICATION_TIMING_CHOICES, default=TWELVE_HOURS) diff --git a/engine/apps/mobile_app/serializers.py b/engine/apps/mobile_app/serializers.py index 87f396580d..02e68abfb5 100644 --- a/engine/apps/mobile_app/serializers.py +++ b/engine/apps/mobile_app/serializers.py @@ -16,4 +16,5 @@ class Meta: "important_notification_volume", "important_notification_override_dnd", "info_notifications_enabled", + "going_oncall_notification_timing", ) diff --git a/engine/apps/mobile_app/tests/test_user_settings.py b/engine/apps/mobile_app/tests/test_user_settings.py index d8fde665b6..bc9c81a665 100644 --- a/engine/apps/mobile_app/tests/test_user_settings.py +++ b/engine/apps/mobile_app/tests/test_user_settings.py @@ -6,7 +6,7 @@ @pytest.mark.django_db def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token): - organization, user, auth_token = make_organization_and_user_with_mobile_app_auth_token() + _, _, auth_token = make_organization_and_user_with_mobile_app_auth_token() client = APIClient() url = reverse("mobile_app:user_settings") @@ -25,6 +25,7 @@ def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume": 0.8, "important_notification_override_dnd": True, "info_notifications_enabled": True, + "going_oncall_notification_timing": 1, } @@ -44,6 +45,7 @@ def test_user_settings_put(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume": 1, "important_notification_override_dnd": False, "info_notifications_enabled": False, + "going_oncall_notification_timing": 3, } response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=auth_token) From f77b6099591c92b4e23818299ee1d5e898fd4192 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 25 Apr 2023 08:48:54 -0400 Subject: [PATCH 03/13] wip --- .../mobile_app/migrations/0004_auto_20230425_1033.py | 4 ++-- engine/apps/mobile_app/models.py | 10 +++------- engine/apps/mobile_app/tests/test_user_settings.py | 6 +++--- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py index ccfc2c29ef..d5a44e4e5a 100644 --- a/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py +++ b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py @@ -27,13 +27,13 @@ class Migration(migrations.Migration): migrations.AddField( model_name='mobileappusersettings', name='going_oncall_notification_timing', - field=models.IntegerField(choices=[(0, 'one hour before'), (1, 'twelve hours before'), (2, 'one day before'), (3, 'one week before')], default=1), + field=models.IntegerField(choices=[(0, 'twelve hours before')], default=0), ), # migrations.AddField enforces the default value on the app level, which leads to the issues during release # adding same default value on the database level AddDefaultValue( model_name='mobileappusersettings', name='going_oncall_notification_timing', - value=1, + value=0, ), ] diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index 040bda2d48..0851e84ff8 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -114,11 +114,7 @@ class VolumeType(models.TextChoices): # these choices + the below column are used to calculate when to send the "You're Going OnCall soon" # push notification - ONE_HOUR, TWELVE_HOURS, ONE_DAY, ONE_WEEK = range(4) - NOTIFICATION_TIMING_CHOICES = ( - (ONE_HOUR, "one hour before"), - (TWELVE_HOURS, "twelve hours before"), - (ONE_DAY, "one day before"), - (ONE_WEEK, "one week before"), - ) + # ONE_HOUR, TWELVE_HOURS, ONE_DAY, ONE_WEEK = range(4) + TWELVE_HOURS = 0 + NOTIFICATION_TIMING_CHOICES = ((TWELVE_HOURS, "twelve hours before"),) going_oncall_notification_timing = models.IntegerField(choices=NOTIFICATION_TIMING_CHOICES, default=TWELVE_HOURS) diff --git a/engine/apps/mobile_app/tests/test_user_settings.py b/engine/apps/mobile_app/tests/test_user_settings.py index bc9c81a665..fe43e460cb 100644 --- a/engine/apps/mobile_app/tests/test_user_settings.py +++ b/engine/apps/mobile_app/tests/test_user_settings.py @@ -25,13 +25,13 @@ def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume": 0.8, "important_notification_override_dnd": True, "info_notifications_enabled": True, - "going_oncall_notification_timing": 1, + "going_oncall_notification_timing": 0, } @pytest.mark.django_db def test_user_settings_put(make_organization_and_user_with_mobile_app_auth_token): - organization, user, auth_token = make_organization_and_user_with_mobile_app_auth_token() + _, _, auth_token = make_organization_and_user_with_mobile_app_auth_token() client = APIClient() url = reverse("mobile_app:user_settings") @@ -45,7 +45,7 @@ def test_user_settings_put(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume": 1, "important_notification_override_dnd": False, "info_notifications_enabled": False, - "going_oncall_notification_timing": 3, + "going_oncall_notification_timing": 0, } response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=auth_token) From 99def9085253b26b714209fa6ba532b7f6b45e6c Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 26 Apr 2023 16:06:08 -0400 Subject: [PATCH 04/13] wip --- engine/apps/mobile_app/fcm_relay.py | 10 ++-- engine/apps/mobile_app/tasks.py | 92 +++++++++++++++++++++++++++++ engine/settings/base.py | 5 ++ 3 files changed, 103 insertions(+), 4 deletions(-) diff --git a/engine/apps/mobile_app/fcm_relay.py b/engine/apps/mobile_app/fcm_relay.py index a8a4720156..ed7ef2fd05 100644 --- a/engine/apps/mobile_app/fcm_relay.py +++ b/engine/apps/mobile_app/fcm_relay.py @@ -41,7 +41,7 @@ def post(self, request): try: token = request.data["token"] data = request.data["data"] - apns = request.data["apns"] + apns = request.data.get("apns") # optional android = request.data.get("android") # optional except KeyError: return Response(status=status.HTTP_400_BAD_REQUEST) @@ -53,7 +53,7 @@ def post(self, request): @shared_dedicated_queue_retry_task( autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else 5 ) -def fcm_relay_async(token, data, apns, android=None): +def fcm_relay_async(token, data, apns=None, android=None): message = _get_message_from_request_data(token, data, apns, android) # https://firebase.google.com/docs/cloud-messaging/http-server-ref#interpret-downstream @@ -68,9 +68,11 @@ def _get_message_from_request_data(token, data, apns, android): """ Create Message object from JSON payload from OSS instance. """ - return Message( - token=token, data=data, apns=_deserialize_apns(apns), android=AndroidConfig(**android) if android else None + token=token, + data=data, + apns=_deserialize_apns(apns) if apns else None, + android=AndroidConfig(**android) if android else None, ) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 972b34a27b..00b7001aeb 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -3,7 +3,9 @@ import requests from celery.utils.log import get_task_logger +from django.apps import apps from django.conf import settings +from django.utils import timezone from fcm_django.models import FCMDevice from firebase_admin.exceptions import FirebaseError from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message @@ -22,6 +24,13 @@ logger.setLevel(logging.DEBUG) +# @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) +def send_push_notification(user, device_registration_id, message) -> None: + # TODO: refactor notify_user_async and conditionally_send_going_oncall_push_notifications_for_schedule + # to deduplicate most of their logic to this method + pass + + @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) def notify_user_async(user_pk, alert_group_pk, notification_policy_pk, critical): # avoid circular import @@ -228,3 +237,86 @@ def _get_fcm_message(alert_group, user, registration_id, critical): }, ), ) + + +@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) +def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) -> None: + logger.info(f"Start calculate_going_oncall_push_notifications_for_schedule for schedule {schedule_pk}") + + OnCallSchedule = apps.get_model("schedules", "OnCallSchedule") + + try: + schedule = OnCallSchedule.objects.get(pk=schedule_pk) + except OnCallSchedule.DoesNotExist: + logger.info(f"Tried to notify user about going on-call for non-existing schedule {schedule_pk}") + return + + schedule_events = schedule.final_events("UTC", timezone.now(), days=5) + + for schedule_event in schedule_events: + users = schedule_event["users"] + + for user in users: + user_pk = user["pk"] + + try: + user = User.objects.get(pk=user_pk) + except User.DoesNotExist: + logger.warning(f"User {user_pk} does not exist") + continue + + device_to_notify = FCMDevice.objects.filter(user=user).first() + + if not device_to_notify: + logger.info(f"User {user_pk} has no device set up") + continue + + # TODO: determine if the timing for the notification is correct + + message = Message( + token=device_to_notify.registration_id, + data={ + # from the docs.. + # A dictionary of data fields (optional). All keys and values in the dictionary must be strings + "type": "oncall.message", + "title": "You are going on call in X time for schedule foo", # TODO: + "subtitle": "foo bar baz", # TODO: + "body": "blah blah blah", # TODO: + "thread_id": f"{schedule.public_primary_key}:{user.public_primary_key}:going-oncall", + }, + ) + + if settings.IS_OPEN_SOURCE: + # FCM relay uses cloud connection to send push notifications + from apps.oss_installation.models import CloudConnector + + if not CloudConnector.objects.exists(): + logger.error(f"Error while sending a mobile push notification: not connected to cloud") + return + + try: + response = send_push_notification_to_fcm_relay(message) + logger.debug(f"FCM relay response: {response}") + except HTTPError as e: + if status.HTTP_400_BAD_REQUEST <= e.response.status_code < status.HTTP_500_INTERNAL_SERVER_ERROR: + # do not retry on HTTP client errors (4xx errors) + logger.error( + f"Error while sending a mobile push notification: HTTP client error {e.response.status_code}" + ) + return + else: + raise + else: + # https://firebase.google.com/docs/cloud-messaging/http-server-ref#interpret-downstream + response = device_to_notify.send_message(message) + logger.debug(f"FCM response: {response}") + + if isinstance(response, FirebaseError): + raise response + + +@shared_dedicated_queue_retry_task() +def conditionally_send_going_oncall_push_notifications_for_all_schedules() -> None: + OnCallSchedule = apps.get_model("schedules", "OnCallSchedule") + for schedule in OnCallSchedule.objects.all(): + conditionally_send_going_oncall_push_notifications_for_schedule.apply_async((schedule.pk,)) diff --git a/engine/settings/base.py b/engine/settings/base.py index 88a4a4de11..d6d47d648d 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -473,6 +473,11 @@ class BrokerTypes: "schedule": 60 * 10, "args": (), }, + "conditionally_send_going_oncall_push_notifications_for_all_schedules": { + "task": "apps.mobile_app.tasks.conditionally_send_going_oncall_push_notifications_for_all_schedules", + "schedule": 10 * 60, + "args": (), + }, } INTERNAL_IPS = ["127.0.0.1"] From d4b272decdadaae0b57f61e2239fba83b1b9da1c Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 27 Apr 2023 19:50:52 -0400 Subject: [PATCH 05/13] WIP --- .../migrations/0004_auto_20230425_1033.py | 2 +- engine/apps/mobile_app/models.py | 15 +- engine/apps/mobile_app/tasks.py | 396 ++++++++++-------- .../apps/mobile_app/tests/test_fcm_relay.py | 4 +- .../apps/mobile_app/tests/test_notify_user.py | 8 +- .../mobile_app/tests/test_user_settings.py | 24 +- .../apps/schedules/models/on_call_schedule.py | 48 ++- 7 files changed, 295 insertions(+), 202 deletions(-) diff --git a/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py index d5a44e4e5a..3f60bcd183 100644 --- a/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py +++ b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py @@ -27,7 +27,7 @@ class Migration(migrations.Migration): migrations.AddField( model_name='mobileappusersettings', name='going_oncall_notification_timing', - field=models.IntegerField(choices=[(0, 'twelve hours before')], default=0), + field=models.IntegerField(choices=[(43200, 'twelve hours before'), (86400, 'one day before'), (604800, 'one week before')], default=43200), ), # migrations.AddField enforces the default value on the app level, which leads to the issues during release # adding same default value on the database level diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index 0851e84ff8..c1f32fbdfc 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -115,6 +115,15 @@ class VolumeType(models.TextChoices): # these choices + the below column are used to calculate when to send the "You're Going OnCall soon" # push notification # ONE_HOUR, TWELVE_HOURS, ONE_DAY, ONE_WEEK = range(4) - TWELVE_HOURS = 0 - NOTIFICATION_TIMING_CHOICES = ((TWELVE_HOURS, "twelve hours before"),) - going_oncall_notification_timing = models.IntegerField(choices=NOTIFICATION_TIMING_CHOICES, default=TWELVE_HOURS) + TWELVE_HOURS_IN_SECONDS = 12 * 60 * 60 + ONE_DAY_IN_SECONDS = TWELVE_HOURS_IN_SECONDS * 2 + ONE_WEEK_IN_SECONDS = ONE_DAY_IN_SECONDS * 7 + + NOTIFICATION_TIMING_CHOICES = ( + (TWELVE_HOURS_IN_SECONDS, "twelve hours before"), + (ONE_DAY_IN_SECONDS, "one day before"), + (ONE_WEEK_IN_SECONDS, "one week before"), + ) + going_oncall_notification_timing = models.IntegerField( + choices=NOTIFICATION_TIMING_CHOICES, default=TWELVE_HOURS_IN_SECONDS + ) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 00b7001aeb..a9fac91999 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -1,9 +1,11 @@ import json import logging +import typing +from enum import Enum +import humanize import requests from celery.utils.log import get_task_logger -from django.apps import apps from django.conf import settings from django.utils import timezone from fcm_django.models import FCMDevice @@ -15,76 +17,60 @@ from apps.alerts.models import AlertGroup from apps.base.utils import live_settings from apps.mobile_app.alert_rendering import get_push_notification_message +from apps.schedules.models.on_call_schedule import OnCallSchedule, ScheduleEvent from apps.user_management.models import User from common.api_helpers.utils import create_engine_url from common.custom_celery_tasks import shared_dedicated_queue_retry_task +if typing.TYPE_CHECKING: + from apps.mobile_app.models import MobileAppUserSettings + + MAX_RETRIES = 1 if settings.DEBUG else 10 logger = get_task_logger(__name__) logger.setLevel(logging.DEBUG) -# @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) -def send_push_notification(user, device_registration_id, message) -> None: - # TODO: refactor notify_user_async and conditionally_send_going_oncall_push_notifications_for_schedule - # to deduplicate most of their logic to this method - pass +class MessageImportanceType(str, Enum): + NORMAL = "oncall.message" + CRITICAL = "oncall.critical_message" -@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) -def notify_user_async(user_pk, alert_group_pk, notification_policy_pk, critical): - # avoid circular import - from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord +class FCMMessageData(typing.TypedDict): + title: str + subtitle: str + body: str - try: - user = User.objects.get(pk=user_pk) - except User.DoesNotExist: - logger.warning(f"User {user_pk} does not exist") - return - try: - alert_group = AlertGroup.all_objects.get(pk=alert_group_pk) - except AlertGroup.DoesNotExist: - logger.warning(f"Alert group {alert_group_pk} does not exist") - return +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 + """ + url = create_engine_url("mobile_app/v1/fcm_relay", override_base=settings.GRAFANA_CLOUD_ONCALL_API_URL) - try: - notification_policy = UserNotificationPolicy.objects.get(pk=notification_policy_pk) - except UserNotificationPolicy.DoesNotExist: - logger.warning(f"User notification policy {notification_policy_pk} does not exist") - return + response = requests.post( + url, headers={"Authorization": live_settings.GRAFANA_CLOUD_ONCALL_TOKEN}, json=json.loads(str(message)) + ) + response.raise_for_status() - def _create_error_log_record(): - """ - Utility method to create a UserNotificationPolicyLogRecord with error - """ - UserNotificationPolicyLogRecord.objects.create( - author=user, - type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED, - notification_policy=notification_policy, - alert_group=alert_group, - reason="Mobile push notification error", - notification_step=notification_policy.step, - notification_channel=notification_policy.notify_by, - ) + return response - device_to_notify = FCMDevice.objects.filter(user=user).first() - # create an error log in case user has no devices set up - if not device_to_notify: - _create_error_log_record() - logger.error(f"Error while sending a mobile push notification: user {user_pk} has no device set up") - return +def _send_push_notification( + device_to_notify: FCMDevice, message: Message, error_cb: typing.Optional[typing.Callable[..., None]] = None +) -> None: + logger.debug(f"Sending push notification with message: {message}") - message = _get_fcm_message(alert_group, user, device_to_notify.registration_id, critical) - logger.debug(f"Sending push notification with message: {message};") + def _error_cb(): + if error_cb: + error_cb() if settings.IS_OPEN_SOURCE: # FCM relay uses cloud connection to send push notifications from apps.oss_installation.models import CloudConnector if not CloudConnector.objects.exists(): - _create_error_log_record() + _error_cb() logger.error(f"Error while sending a mobile push notification: not connected to cloud") return @@ -94,7 +80,7 @@ def _create_error_log_record(): except HTTPError as e: if status.HTTP_400_BAD_REQUEST <= e.response.status_code < status.HTTP_500_INTERNAL_SERVER_ERROR: # do not retry on HTTP client errors (4xx errors) - _create_error_log_record() + _error_cb() logger.error( f"Error while sending a mobile push notification: HTTP client error {e.response.status_code}" ) @@ -110,21 +96,56 @@ def _create_error_log_record(): raise response -def send_push_notification_to_fcm_relay(message): - """ - Send push notification to FCM relay on cloud instance: apps.mobile_app.fcm_relay.FCMRelayView - """ - url = create_engine_url("mobile_app/v1/fcm_relay", override_base=settings.GRAFANA_CLOUD_ONCALL_API_URL) +def _construct_fcm_message( + device_to_notify: FCMDevice, + thread_id: str, + data: FCMMessageData, + apns_payload: typing.Optional[APNSPayload] = None, + critical_message_type: bool = False, +) -> Message: + apns_config_kwargs = {} - response = requests.post( - url, headers={"Authorization": live_settings.GRAFANA_CLOUD_ONCALL_TOKEN}, json=json.loads(str(message)) - ) - response.raise_for_status() + if apns_payload is not None: + apns_config_kwargs["payload"] = apns_payload - return response + return Message( + token=device_to_notify.registration_id, + data={ + # from the docs.. + # A dictionary of data fields (optional). All keys and values in the dictionary must be strings + **data, + "type": MessageImportanceType.CRITICAL if critical_message_type else MessageImportanceType.NORMAL, + "thread_id": thread_id, + }, + android=AndroidConfig( + # from the docs + # https://firebase.google.com/docs/cloud-messaging/concept-options#setting-the-priority-of-a-message + # + # Normal priority. + # Normal priority messages are delivered immediately when the app is in the foreground. + # For backgrounded apps, delivery may be delayed. For less time-sensitive messages, such as notifications + # of new email, keeping your UI in sync, or syncing app data in the background, choose normal delivery + # priority. + # + # High priority. + # FCM attempts to deliver high priority messages immediately even if the device is in Doze mode. + # High priority messages are for time-sensitive, user visible content. + priority="high", + ), + apns=APNSConfig( + **apns_config_kwargs, + headers={ + # From the docs + # https://firebase.google.com/docs/cloud-messaging/concept-options#setting-the-priority-of-a-message + "apns-priority": "10", + }, + ), + ) -def _get_fcm_message(alert_group, user, registration_id, critical): +def _get_alert_group_escalation_fcm_message( + alert_group: AlertGroup, user: User, device_to_notify: FCMDevice, critical: bool +) -> Message: # avoid circular import from apps.mobile_app.models import MobileAppUserSettings @@ -162,98 +183,157 @@ def _get_fcm_message(alert_group, user, registration_id, critical): else mobile_app_user_settings.default_notification_sound_name ) + MobileAppUserSettings.IOS_SOUND_NAME_EXTENSION # iOS app expects the filename to have an extension - return Message( - token=registration_id, - data={ - # from the docs.. - # A dictionary of data fields (optional). All keys and values in the dictionary must be strings - # - # alert_group.status is an int so it must be casted... - "orgId": alert_group.channel.organization.public_primary_key, - "orgName": alert_group.channel.organization.stack_slug, - "alertGroupId": alert_group.public_primary_key, - "status": str(alert_group.status), - "type": "oncall.critical_message" if critical else "oncall.message", - "title": alert_title, - "subtitle": alert_subtitle, - "body": alert_body, - "thread_id": thread_id, - # 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_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_volume_type": mobile_app_user_settings.important_notification_volume_type, - "important_notification_volume": str(mobile_app_user_settings.important_notification_volume), - "important_notification_override_dnd": json.dumps( - mobile_app_user_settings.important_notification_override_dnd - ), - }, - android=AndroidConfig( - # from the docs - # https://firebase.google.com/docs/cloud-messaging/concept-options#setting-the-priority-of-a-message - # - # Normal priority. - # Normal priority messages are delivered immediately when the app is in the foreground. - # For backgrounded apps, delivery may be delayed. For less time-sensitive messages, such as notifications - # of new email, keeping your UI in sync, or syncing app data in the background, choose normal delivery - # priority. - # - # High priority. - # FCM attempts to deliver high priority messages immediately even if the device is in Doze mode. - # High priority messages are for time-sensitive, user visible content. - priority="high", + fcm_message_data: FCMMessageData = { + "title": alert_title, + "subtitle": alert_subtitle, + "body": alert_body, + "orgId": alert_group.channel.organization.public_primary_key, + "orgName": alert_group.channel.organization.stack_slug, + "alertGroupId": alert_group.public_primary_key, + # 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 ), - apns=APNSConfig( - payload=APNSPayload( - aps=Aps( - thread_id=thread_id, - badge=number_of_alerts, - alert=ApsAlert(title=alert_title, subtitle=alert_subtitle, body=alert_body), - sound=CriticalSound( - # The notification shouldn't be critical if the user has disabled "override DND" setting - critical=overrideDND, - name=apns_sound_name, - volume=apns_volume, - ), - custom_data={ - "interruption-level": "critical" if overrideDND else "time-sensitive", - }, - ), + "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_volume_type": mobile_app_user_settings.important_notification_volume_type, + "important_notification_volume": str(mobile_app_user_settings.important_notification_volume), + "important_notification_override_dnd": json.dumps(mobile_app_user_settings.important_notification_override_dnd), + } + + apns_payload = APNSPayload( + aps=Aps( + thread_id=thread_id, + badge=number_of_alerts, + alert=ApsAlert(title=alert_title, subtitle=alert_subtitle, body=alert_body), + sound=CriticalSound( + # The notification shouldn't be critical if the user has disabled "override DND" setting + critical=overrideDND, + name=apns_sound_name, + volume=apns_volume, ), - headers={ - # From the docs - # https://firebase.google.com/docs/cloud-messaging/concept-options#setting-the-priority-of-a-message - "apns-priority": "10", + custom_data={ + "interruption-level": "critical" if overrideDND else "time-sensitive", }, ), ) + return _construct_fcm_message(device_to_notify, thread_id, fcm_message_data, apns_payload, critical) + + +def _get_youre_going_oncall_fcm_message( + user: User, schedule: OnCallSchedule, device_to_notify: FCMDevice, seconds_until_going_oncall: int +) -> Message: + thread_id = f"{schedule.public_primary_key}:{user.public_primary_key}:going-oncall" + data: FCMMessageData = { + "title": f"You are going on call in {humanize.naturaldelta(seconds_until_going_oncall)} for schedule {schedule.name}", + "subtitle": "foo bar baz", # TODO: what should this be? + "body": "blah blah blah", # TODO: what should this be? + } + + return _construct_fcm_message(device_to_notify, thread_id, data) + + +@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) +def notify_user_async(user_pk, alert_group_pk, notification_policy_pk, critical): + # avoid circular import + from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord + + try: + user = User.objects.get(pk=user_pk) + except User.DoesNotExist: + logger.warning(f"User {user_pk} does not exist") + return + + try: + alert_group = AlertGroup.all_objects.get(pk=alert_group_pk) + except AlertGroup.DoesNotExist: + logger.warning(f"Alert group {alert_group_pk} does not exist") + return + + try: + notification_policy = UserNotificationPolicy.objects.get(pk=notification_policy_pk) + except UserNotificationPolicy.DoesNotExist: + logger.warning(f"User notification policy {notification_policy_pk} does not exist") + return + + def _create_error_log_record(): + """ + Utility method to create a UserNotificationPolicyLogRecord with error + """ + UserNotificationPolicyLogRecord.objects.create( + author=user, + type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED, + notification_policy=notification_policy, + alert_group=alert_group, + reason="Mobile push notification error", + notification_step=notification_policy.step, + notification_channel=notification_policy.notify_by, + ) + + device_to_notify = FCMDevice.objects.filter(user=user).first() + + # create an error log in case user has no devices set up + if not device_to_notify: + _create_error_log_record() + logger.error(f"Error while sending a mobile push notification: user {user_pk} has no device set up") + return + + message = _get_alert_group_escalation_fcm_message(alert_group, user, device_to_notify, critical) + _send_push_notification(device_to_notify, message, _create_error_log_record) + + +def should_we_send_going_oncall_push_notification( + now: timezone.datetime, user_settings: "MobileAppUserSettings", schedule_event: ScheduleEvent +) -> bool: + NOTIFICATION_TIMING_BUFFER = 15 * 60 # 15 minutes in seconds + + # this _should_ always be positive since final_events is returning only events in the future + seconds_until_shift_starts = schedule_event["start"] - now + + # user_wants_to_receive_info_notifications = user_settings.info_notifications_enabled + # user_notification_timing_preference = user_settings.going_oncall_notification_timing + + # notification_timing_is_right = () + + # TODO: finish figuring out this logic + # example. + # shift starts in 11h58m + # user wants to receieve notification at the 12h mark + # send if shift_start_time <= (user_preference + 10mins) or + + if seconds_until_shift_starts - NOTIFICATION_TIMING_BUFFER: + return True + return False + @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) -> None: - logger.info(f"Start calculate_going_oncall_push_notifications_for_schedule for schedule {schedule_pk}") + # avoid circular import + from apps.mobile_app.models import MobileAppUserSettings - OnCallSchedule = apps.get_model("schedules", "OnCallSchedule") + logger.info(f"Start calculate_going_oncall_push_notifications_for_schedule for schedule {schedule_pk}") try: - schedule = OnCallSchedule.objects.get(pk=schedule_pk) + schedule: OnCallSchedule = OnCallSchedule.objects.get(pk=schedule_pk) except OnCallSchedule.DoesNotExist: logger.info(f"Tried to notify user about going on-call for non-existing schedule {schedule_pk}") return - schedule_events = schedule.final_events("UTC", timezone.now(), days=5) + now = timezone.now() - for schedule_event in schedule_events: + # TODO: add better logging + for schedule_event in schedule.final_events("UTC", now, days=5): users = schedule_event["users"] for user in users: @@ -271,52 +351,16 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) logger.info(f"User {user_pk} has no device set up") continue - # TODO: determine if the timing for the notification is correct - - message = Message( - token=device_to_notify.registration_id, - data={ - # from the docs.. - # A dictionary of data fields (optional). All keys and values in the dictionary must be strings - "type": "oncall.message", - "title": "You are going on call in X time for schedule foo", # TODO: - "subtitle": "foo bar baz", # TODO: - "body": "blah blah blah", # TODO: - "thread_id": f"{schedule.public_primary_key}:{user.public_primary_key}:going-oncall", - }, - ) - - if settings.IS_OPEN_SOURCE: - # FCM relay uses cloud connection to send push notifications - from apps.oss_installation.models import CloudConnector - - if not CloudConnector.objects.exists(): - logger.error(f"Error while sending a mobile push notification: not connected to cloud") - return - - try: - response = send_push_notification_to_fcm_relay(message) - logger.debug(f"FCM relay response: {response}") - except HTTPError as e: - if status.HTTP_400_BAD_REQUEST <= e.response.status_code < status.HTTP_500_INTERNAL_SERVER_ERROR: - # do not retry on HTTP client errors (4xx errors) - logger.error( - f"Error while sending a mobile push notification: HTTP client error {e.response.status_code}" - ) - return - else: - raise - else: - # https://firebase.google.com/docs/cloud-messaging/http-server-ref#interpret-downstream - response = device_to_notify.send_message(message) - logger.debug(f"FCM response: {response}") + mobile_app_user_settings, _ = MobileAppUserSettings.objects.get_or_create(user=user) - if isinstance(response, FirebaseError): - raise response + if should_we_send_going_oncall_push_notification(now, mobile_app_user_settings, schedule_event): + message = _get_youre_going_oncall_fcm_message( + user, schedule, device_to_notify, mobile_app_user_settings.going_oncall_notification_timing + ) + _send_push_notification(device_to_notify, message) @shared_dedicated_queue_retry_task() def conditionally_send_going_oncall_push_notifications_for_all_schedules() -> None: - OnCallSchedule = apps.get_model("schedules", "OnCallSchedule") for schedule in OnCallSchedule.objects.all(): conditionally_send_going_oncall_push_notifications_for_schedule.apply_async((schedule.pk,)) diff --git a/engine/apps/mobile_app/tests/test_fcm_relay.py b/engine/apps/mobile_app/tests/test_fcm_relay.py index 476346172c..bc2b3e582d 100644 --- a/engine/apps/mobile_app/tests/test_fcm_relay.py +++ b/engine/apps/mobile_app/tests/test_fcm_relay.py @@ -9,7 +9,7 @@ from rest_framework.test import APIClient from apps.mobile_app.fcm_relay import FCMRelayThrottler, _get_message_from_request_data, fcm_relay_async -from apps.mobile_app.tasks import _get_fcm_message +from apps.mobile_app.tasks import _get_alert_group_escalation_fcm_message @pytest.mark.django_db @@ -118,7 +118,7 @@ def test_fcm_relay_serialize_deserialize( make_alert(alert_group=alert_group, raw_request_data={}) # Imitate sending a message to the FCM relay endpoint - original_message = _get_fcm_message(alert_group, user, device.registration_id, critical=False) + original_message = _get_alert_group_escalation_fcm_message(alert_group, user, device, critical=False) request_data = json.loads(str(original_message)) # Imitate receiving a message from the FCM relay endpoint diff --git a/engine/apps/mobile_app/tests/test_notify_user.py b/engine/apps/mobile_app/tests/test_notify_user.py index 9f76d2f76c..48e38a17fe 100644 --- a/engine/apps/mobile_app/tests/test_notify_user.py +++ b/engine/apps/mobile_app/tests/test_notify_user.py @@ -6,7 +6,7 @@ from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord from apps.mobile_app.models import MobileAppUserSettings -from apps.mobile_app.tasks import _get_fcm_message, notify_user_async +from apps.mobile_app.tasks import _get_alert_group_escalation_fcm_message, notify_user_async from apps.oss_installation.models import CloudConnector MOBILE_APP_BACKEND_ID = 5 @@ -223,7 +223,7 @@ def test_fcm_message_user_settings( alert_group = make_alert_group(alert_receive_channel) make_alert(alert_group=alert_group, raw_request_data={}) - message = _get_fcm_message(alert_group, user, device.registration_id, critical=False) + message = _get_alert_group_escalation_fcm_message(alert_group, user, device, critical=False) # Check user settings are passed to FCM message assert message.data["default_notification_sound_name"] == "default_sound.mp3" @@ -253,7 +253,7 @@ def test_fcm_message_user_settings_critical( alert_group = make_alert_group(alert_receive_channel) make_alert(alert_group=alert_group, raw_request_data={}) - message = _get_fcm_message(alert_group, user, device.registration_id, critical=True) + message = _get_alert_group_escalation_fcm_message(alert_group, user, device, critical=True) # Check user settings are passed to FCM message assert message.data["default_notification_sound_name"] == "default_sound.mp3" @@ -286,7 +286,7 @@ def test_fcm_message_user_settings_critical_override_dnd_disabled( # Disable important notification override DND MobileAppUserSettings.objects.create(user=user, important_notification_override_dnd=False) - message = _get_fcm_message(alert_group, user, device.registration_id, critical=True) + message = _get_alert_group_escalation_fcm_message(alert_group, user, device, critical=True) # Check user settings are passed to FCM message assert message.data["important_notification_override_dnd"] == "false" diff --git a/engine/apps/mobile_app/tests/test_user_settings.py b/engine/apps/mobile_app/tests/test_user_settings.py index fe43e460cb..b4dcc87c40 100644 --- a/engine/apps/mobile_app/tests/test_user_settings.py +++ b/engine/apps/mobile_app/tests/test_user_settings.py @@ -25,12 +25,23 @@ def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume": 0.8, "important_notification_override_dnd": True, "info_notifications_enabled": True, - "going_oncall_notification_timing": 0, + "going_oncall_notification_timing": 43200, } @pytest.mark.django_db -def test_user_settings_put(make_organization_and_user_with_mobile_app_auth_token): +@pytest.mark.parametrize( + "going_oncall_notification_timing,expected_status_code", + [ + (43200, status.HTTP_200_OK), + (86400, status.HTTP_200_OK), + (604800, status.HTTP_200_OK), + (500, status.HTTP_400_BAD_REQUEST), + ], +) +def test_user_settings_put( + make_organization_and_user_with_mobile_app_auth_token, going_oncall_notification_timing, expected_status_code +): _, _, auth_token = make_organization_and_user_with_mobile_app_auth_token() client = APIClient() @@ -45,11 +56,12 @@ def test_user_settings_put(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume": 1, "important_notification_override_dnd": False, "info_notifications_enabled": False, - "going_oncall_notification_timing": 0, + "going_oncall_notification_timing": going_oncall_notification_timing, } response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=auth_token) - assert response.status_code == status.HTTP_200_OK + assert response.status_code == expected_status_code - # Check the values are updated correctly - assert response.json() == data + if expected_status_code == status.HTTP_200_OK: + # Check the values are updated correctly + assert response.json() == data diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index ee08ff9fc9..de27062883 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -3,7 +3,7 @@ import itertools from collections import defaultdict from enum import Enum -from typing import Iterable, Optional, TypedDict +from typing import Iterable, List, Optional, Tuple, TypedDict, Union import icalendar import pytz @@ -69,6 +69,34 @@ class QualityReport(TypedDict): overloaded_users: list[QualityReportOverloadedUser] +class ScheduleEventUser(TypedDict): + display_name: str + pk: str + + +class ScheduleEventShift(TypedDict): + pk: str + + +class ScheduleEvent(TypedDict): + all_day: bool + start: datetime.datetime + end: datetime.datetime + users: List[ScheduleEventUser] + missing_users: List[str] + priority_level: Union[int, None] + source: Union[str, None] + calendar_type: Union[int, None] + is_empty: bool + is_gap: bool + is_override: bool + shift: ScheduleEventShift + + +ScheduleEvents = List[ScheduleEvent] +ScheduleEventIntervals = List[List[datetime.datetime]] + + def generate_public_primary_key_for_oncall_schedule_channel(): prefix = "S" new_public_primary_key = generate_public_primary_key(prefix) @@ -256,7 +284,7 @@ def filter_events( with_gap=False, filter_by=None, all_day_datetime=False, - ): + ) -> ScheduleEvents: """Return filtered events from schedule.""" shifts = ( list_of_oncall_shifts_from_ical( @@ -513,15 +541,15 @@ def get_balance_score_by_duration_map(dur_map: dict[str, datetime.timedelta]) -> "overloaded_users": overloaded_users, } - def _resolve_schedule(self, events): + def _resolve_schedule(self, events: ScheduleEvents) -> ScheduleEvents: """Calculate final schedule shifts considering rotations and overrides.""" if not events: return [] - def event_start_cmp_key(e): + def event_start_cmp_key(e: ScheduleEvent) -> datetime.datetime: return e["start"] - def event_cmp_key(e): + def event_cmp_key(e: ScheduleEvent) -> Tuple[int, int, datetime.datetime]: """Sorting key criteria for events.""" start = event_start_cmp_key(e) return ( @@ -530,7 +558,7 @@ def event_cmp_key(e): start, ) - def insort_event(eventlist, e): + def insort_event(eventlist: ScheduleEvents, e: ScheduleEvent) -> None: """Insert event keeping ordering criteria into already sorted event list.""" idx = 0 for i in eventlist: @@ -540,7 +568,7 @@ def insort_event(eventlist, e): break eventlist.insert(idx, e) - def _merge_intervals(evs): + def _merge_intervals(evs: ScheduleEvents) -> ScheduleEventIntervals: """Keep track of scheduled intervals.""" if not evs: return [] @@ -562,8 +590,8 @@ def _merge_intervals(evs): # split the event, or fix start/end timestamps accordingly intervals = [] - resolved = [] - pending = events + resolved: ScheduleEvents = [] + pending: ScheduleEvents = events current_interval_idx = 0 # current scheduled interval being checked current_type = OnCallSchedule.TYPE_ICAL_OVERRIDES # current calendar type current_priority = None # current priority level being resolved @@ -638,7 +666,7 @@ def _merge_intervals(evs): resolved.sort(key=lambda e: (event_start_cmp_key(e), e["shift"]["pk"] or "")) return resolved - def _merge_events(self, events): + def _merge_events(self, events: ScheduleEvents) -> ScheduleEvents: """Merge user groups same-shift events.""" if events: merged = [events[0]] From 6243c98108822e6191b31b074ddddc209c8dc765 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 27 Apr 2023 19:54:32 -0400 Subject: [PATCH 06/13] wip --- engine/apps/mobile_app/fcm_relay.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/engine/apps/mobile_app/fcm_relay.py b/engine/apps/mobile_app/fcm_relay.py index ed7ef2fd05..0ea9205352 100644 --- a/engine/apps/mobile_app/fcm_relay.py +++ b/engine/apps/mobile_app/fcm_relay.py @@ -41,7 +41,7 @@ def post(self, request): try: token = request.data["token"] data = request.data["data"] - apns = request.data.get("apns") # optional + apns = request.data["apns"] android = request.data.get("android") # optional except KeyError: return Response(status=status.HTTP_400_BAD_REQUEST) @@ -53,7 +53,7 @@ def post(self, request): @shared_dedicated_queue_retry_task( autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else 5 ) -def fcm_relay_async(token, data, apns=None, android=None): +def fcm_relay_async(token, data, apns, android=None): message = _get_message_from_request_data(token, data, apns, android) # https://firebase.google.com/docs/cloud-messaging/http-server-ref#interpret-downstream @@ -69,10 +69,7 @@ def _get_message_from_request_data(token, data, apns, android): Create Message object from JSON payload from OSS instance. """ return Message( - token=token, - data=data, - apns=_deserialize_apns(apns) if apns else None, - android=AndroidConfig(**android) if android else None, + token=token, data=data, apns=_deserialize_apns(apns), android=AndroidConfig(**android) if android else None ) From 4748f329084d268d0edbd033e569ebcd0d9caf6a Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 2 May 2023 12:25:37 -0400 Subject: [PATCH 07/13] add tests --- engine/apps/mobile_app/fcm_relay.py | 1 + engine/apps/mobile_app/tasks.py | 38 +++++---- .../test_your_going_oncall_notification.py | 80 +++++++++++++++++++ 3 files changed, 103 insertions(+), 16 deletions(-) create mode 100644 engine/apps/mobile_app/tests/test_your_going_oncall_notification.py diff --git a/engine/apps/mobile_app/fcm_relay.py b/engine/apps/mobile_app/fcm_relay.py index 0ea9205352..a8a4720156 100644 --- a/engine/apps/mobile_app/fcm_relay.py +++ b/engine/apps/mobile_app/fcm_relay.py @@ -68,6 +68,7 @@ def _get_message_from_request_data(token, data, apns, android): """ Create Message object from JSON payload from OSS instance. """ + return Message( token=token, data=data, apns=_deserialize_apns(apns), android=AndroidConfig(**android) if android else None ) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index a9fac91999..2cbfd0dfcc 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -1,5 +1,6 @@ import json import logging +import math import typing from enum import Enum @@ -38,8 +39,8 @@ class MessageImportanceType(str, Enum): class FCMMessageData(typing.TypedDict): title: str - subtitle: str - body: str + subtitle: typing.Optional[str] + body: typing.Optional[str] def send_push_notification_to_fcm_relay(message: Message) -> requests.Response: @@ -237,8 +238,6 @@ def _get_youre_going_oncall_fcm_message( thread_id = f"{schedule.public_primary_key}:{user.public_primary_key}:going-oncall" data: FCMMessageData = { "title": f"You are going on call in {humanize.naturaldelta(seconds_until_going_oncall)} for schedule {schedule.name}", - "subtitle": "foo bar baz", # TODO: what should this be? - "body": "blah blah blah", # TODO: what should this be? } return _construct_fcm_message(device_to_notify, thread_id, data) @@ -299,21 +298,28 @@ def should_we_send_going_oncall_push_notification( NOTIFICATION_TIMING_BUFFER = 15 * 60 # 15 minutes in seconds # this _should_ always be positive since final_events is returning only events in the future - seconds_until_shift_starts = schedule_event["start"] - now + seconds_until_shift_starts = math.floor((schedule_event["start"] - now).total_seconds()) - # user_wants_to_receive_info_notifications = user_settings.info_notifications_enabled - # user_notification_timing_preference = user_settings.going_oncall_notification_timing + user_wants_to_receive_info_notifications = user_settings.info_notifications_enabled + # int representing num of seconds before the shift starts that the user wants to be notified + user_notification_timing_preference = user_settings.going_oncall_notification_timing - # notification_timing_is_right = () + if not user_wants_to_receive_info_notifications: + logger.info("not sending going oncall push notification because info_notifications_enabled is false") + return False - # TODO: finish figuring out this logic - # example. - # shift starts in 11h58m - # user wants to receieve notification at the 12h mark - # send if shift_start_time <= (user_preference + 10mins) or + lower_limit = seconds_until_shift_starts - NOTIFICATION_TIMING_BUFFER - if seconds_until_shift_starts - NOTIFICATION_TIMING_BUFFER: + timing_logging_msg = ( + f"lower_limit: {lower_limit}\n" + f"user timing preference: {user_notification_timing_preference}\n" + f"seconds_until_shift_starts: {seconds_until_shift_starts}" + ) + + if lower_limit <= user_notification_timing_preference <= seconds_until_shift_starts: + logger.info(f"timing is right to send going oncall push notification\n{timing_logging_msg}") return True + logger.info(f"timing is not right to send going oncall push notification\n{timing_logging_msg}") return False @@ -332,15 +338,15 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) now = timezone.now() - # TODO: add better logging for schedule_event in schedule.final_events("UTC", now, days=5): users = schedule_event["users"] for user in users: user_pk = user["pk"] + logger.info(f"Evaluating if we should send push notification for schedule {schedule_pk} for user {user_pk}") try: - user = User.objects.get(pk=user_pk) + user = User.objects.get(public_primary_key=user_pk) except User.DoesNotExist: logger.warning(f"User {user_pk} does not exist") continue 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 new file mode 100644 index 0000000000..b15e32c1d5 --- /dev/null +++ b/engine/apps/mobile_app/tests/test_your_going_oncall_notification.py @@ -0,0 +1,80 @@ +from unittest import mock + +import pytest +from django.utils import timezone +from fcm_django.models import FCMDevice + +from apps.mobile_app.models import MobileAppUserSettings +from apps.mobile_app.tasks import ( + conditionally_send_going_oncall_push_notifications_for_all_schedules, + conditionally_send_going_oncall_push_notifications_for_schedule, + should_we_send_going_oncall_push_notification, +) +from apps.schedules.models import OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb +from apps.schedules.models.on_call_schedule import ScheduleEvent + +MOBILE_APP_BACKEND_ID = 5 +CLOUD_LICENSE_NAME = "Cloud" +OPEN_SOURCE_LICENSE_NAME = "OpenSource" + + +def _create_schedule_event() -> ScheduleEvent: + return { + # TODO: + "start": timezone.now(), + } + + +@pytest.mark.django_db +def test_should_we_send_going_oncall_push_notification(make_organization_and_user, make_schedule): + # create a user and connect a mobile device + organization, user = make_organization_and_user() + FCMDevice.objects.create(user=user, registration_id="test_device_id") + + now = timezone.now() + + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + print(schedule) + user_mobile_settings = MobileAppUserSettings.objects.create(user=user, important_notification_override_dnd=False) + + # TODO: finish test + should_we_send_going_oncall_push_notification(now, user_mobile_settings, _create_schedule_event()) + + +@pytest.mark.django_db +def test_conditionally_send_going_oncall_push_notifications_for_schedule(make_organization_and_user, make_schedule): + # create a user and connect a mobile device + organization, user = make_organization_and_user() + FCMDevice.objects.create(user=user, registration_id="test_device_id") + + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + # TODO: finish test + conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) + + +@mock.patch("apps.mobile_app.tasks.conditionally_send_going_oncall_push_notifications_for_schedule", return_value=None) +@pytest.mark.django_db +def test_conditionally_send_going_oncall_push_notifications_for_all_schedules( + mocked_conditionally_send_going_oncall_push_notifications_for_schedule, + make_organization_and_user, + make_schedule, +): + # create a user and connect a mobile device + organization, user = make_organization_and_user() + FCMDevice.objects.create(user=user, registration_id="test_device_id") + + schedule1 = make_schedule(organization, schedule_class=OnCallScheduleCalendar) + schedule2 = make_schedule(organization, schedule_class=OnCallScheduleICal) + schedule3 = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + conditionally_send_going_oncall_push_notifications_for_all_schedules() + + mocked_conditionally_send_going_oncall_push_notifications_for_schedule.apply_async.assert_has_calls( + [ + mock.call((schedule1.pk,)), + mock.call((schedule2.pk,)), + mock.call((schedule3.pk,)), + ], + any_order=True, + ) From e399a1d37823048b36fd0bf2daf5f3d109328ef6 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 2 May 2023 12:29:18 -0400 Subject: [PATCH 08/13] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 671fa857dc..e3a06075ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add filter descriptions to web ui by @iskhakov ([1845](https://github.com/grafana/oncall/pull/1845)) - Add "Notifications Receiver" RBAC role by @joeyorlando ([#1853](https://github.com/grafana/oncall/pull/1853)) +- Add a new mobile app push notification which notifies users when they are going on by @joeyorlando ([#1814](https://github.com/grafana/oncall/pull/1814)) ### Changed From 3335209caeee989c25bb7c59a6c53e1bf5ed7684 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 2 May 2023 14:05:39 -0400 Subject: [PATCH 09/13] add user/FCMdevice task cache --- engine/apps/mobile_app/tasks.py | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 2cbfd0dfcc..81aae828e2 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -328,6 +328,9 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) # avoid circular import from apps.mobile_app.models import MobileAppUserSettings + user_cache: typing.Dict[str, User] = {} + device_cache: typing.Dict[str, FCMDevice] = {} + logger.info(f"Start calculate_going_oncall_push_notifications_for_schedule for schedule {schedule_pk}") try: @@ -345,17 +348,24 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) user_pk = user["pk"] logger.info(f"Evaluating if we should send push notification for schedule {schedule_pk} for user {user_pk}") - try: - user = User.objects.get(public_primary_key=user_pk) - except User.DoesNotExist: - logger.warning(f"User {user_pk} does not exist") - continue - - device_to_notify = FCMDevice.objects.filter(user=user).first() - - if not device_to_notify: - logger.info(f"User {user_pk} has no device set up") - continue + user = user_cache.get(user_pk, None) + if user is None: + try: + user = User.objects.get(public_primary_key=user_pk) + user_cache[user_pk] = user + except User.DoesNotExist: + logger.warning(f"User {user_pk} does not exist") + continue + + device_to_notify = device_cache.get(user_pk, None) + if device_to_notify is None: + device_to_notify = FCMDevice.objects.filter(user=user).first() + + if not device_to_notify: + logger.info(f"User {user_pk} has no device set up") + continue + else: + device_cache[user_pk] = device_to_notify mobile_app_user_settings, _ = MobileAppUserSettings.objects.get_or_create(user=user) From 24191cb4ac5873e3e098d0788d6c1139580bfce2 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 2 May 2023 14:06:39 -0400 Subject: [PATCH 10/13] fetch schedule events for next 7 days --- engine/apps/mobile_app/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 81aae828e2..03ef405e40 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -341,7 +341,7 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) now = timezone.now() - for schedule_event in schedule.final_events("UTC", now, days=5): + for schedule_event in schedule.final_events("UTC", now, days=7): users = schedule_event["users"] for user in users: From c323eac5d1af995f7ac467b1d488bf869d100d06 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 4 May 2023 09:57:14 -0400 Subject: [PATCH 11/13] resend notification 15mins before also, cache notifications sent to avoid resending --- engine/apps/mobile_app/tasks.py | 82 +++++- .../test_your_going_oncall_notification.py | 269 +++++++++++++++--- 2 files changed, 305 insertions(+), 46 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 03ef405e40..3df746c291 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -8,6 +8,7 @@ import requests from celery.utils.log import get_task_logger from django.conf import settings +from django.core.cache import cache from django.utils import timezone from fcm_django.models import FCMDevice from firebase_admin.exceptions import FirebaseError @@ -292,10 +293,27 @@ def _create_error_log_record(): _send_push_notification(device_to_notify, message, _create_error_log_record) +def _shift_starts_within_range( + timing_window_lower: int, timing_window_upper: int, seconds_until_shift_starts: int +) -> bool: + return timing_window_lower <= seconds_until_shift_starts <= timing_window_upper + + def should_we_send_going_oncall_push_notification( now: timezone.datetime, user_settings: "MobileAppUserSettings", schedule_event: ScheduleEvent -) -> bool: - NOTIFICATION_TIMING_BUFFER = 15 * 60 # 15 minutes in seconds +) -> typing.Optional[int]: + """ + If the user should be set a "you're going oncall" push notification, return the number of seconds + until they will be going oncall. + + If no notification should be sent, return None. + + Currently we will send notifications for the following scenarios: + - schedule is starting in user's "configured notification timing preference" +/- a 4 minute buffer + - schedule is starting within the next fifteen minutes + """ + NOTIFICATION_TIMING_BUFFER = 4 * 60 # 4 minutes in seconds + FIFTEEN_MINUTES_IN_SECONDS = 15 * 60 # this _should_ always be positive since final_events is returning only events in the future seconds_until_shift_starts = math.floor((schedule_event["start"] - now).total_seconds()) @@ -306,21 +324,36 @@ def should_we_send_going_oncall_push_notification( if not user_wants_to_receive_info_notifications: logger.info("not sending going oncall push notification because info_notifications_enabled is false") - return False + return - lower_limit = seconds_until_shift_starts - NOTIFICATION_TIMING_BUFFER + # 8 minute window where the notification could be sent (4 mins before or 4 mins after) + timing_window_lower = user_notification_timing_preference - NOTIFICATION_TIMING_BUFFER + timing_window_upper = user_notification_timing_preference + NOTIFICATION_TIMING_BUFFER + + shift_starts_within_users_notification_timing_preference = _shift_starts_within_range( + timing_window_lower, timing_window_upper, seconds_until_shift_starts + ) + shift_starts_within_fifteen_minutes = _shift_starts_within_range( + 0, FIFTEEN_MINUTES_IN_SECONDS, seconds_until_shift_starts + ) timing_logging_msg = ( - f"lower_limit: {lower_limit}\n" - f"user timing preference: {user_notification_timing_preference}\n" - f"seconds_until_shift_starts: {seconds_until_shift_starts}" + f"seconds_until_shift_starts: {seconds_until_shift_starts}\n" + f"user_notification_timing_preference: {user_notification_timing_preference}\n" + f"timing_window_lower: {timing_window_lower}\n" + f"timing_window_upper: {timing_window_upper}\n" + f"shift_starts_within_users_notification_timing_preference: {shift_starts_within_users_notification_timing_preference}\n" + f"shift_starts_within_fifteen_minutes: {shift_starts_within_fifteen_minutes}" ) - if lower_limit <= user_notification_timing_preference <= seconds_until_shift_starts: + if shift_starts_within_users_notification_timing_preference or shift_starts_within_fifteen_minutes: logger.info(f"timing is right to send going oncall push notification\n{timing_logging_msg}") - return True + return seconds_until_shift_starts logger.info(f"timing is not right to send going oncall push notification\n{timing_logging_msg}") - return False + + +def _generate_going_oncall_push_notification_cache_key(user_pk: str, schedule_event: ScheduleEvent, time: str) -> str: + return f"going_oncall_push_notification:{user_pk}:{schedule_event['shift']['pk']}:{time}" @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) @@ -328,6 +361,7 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) # avoid circular import from apps.mobile_app.models import MobileAppUserSettings + PUSH_NOTIFICATION_TRACKING_CACHE_KEY_TTL = 80 * 60 # 80 minutes user_cache: typing.Dict[str, User] = {} device_cache: typing.Dict[str, FCMDevice] = {} @@ -340,8 +374,18 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) return now = timezone.now() + current_date_with_hour = now.replace(minute=0, second=0, microsecond=0).strftime("%Y-%m-%d-%Hh") + schedule_final_events = schedule.final_events("UTC", now, days=7) + + relevant_cache_keys = [ + _generate_going_oncall_push_notification_cache_key(user["pk"], schedule_event, current_date_with_hour) + for schedule_event in schedule_final_events + for user in schedule_event["users"] + ] - for schedule_event in schedule.final_events("UTC", now, days=7): + relevant_notifications_already_sent = cache.get_many(relevant_cache_keys) + + for schedule_event in schedule_final_events: users = schedule_event["users"] for user in users: @@ -369,11 +413,25 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) mobile_app_user_settings, _ = MobileAppUserSettings.objects.get_or_create(user=user) - if should_we_send_going_oncall_push_notification(now, mobile_app_user_settings, schedule_event): + cache_key = _generate_going_oncall_push_notification_cache_key( + user_pk, schedule_event, current_date_with_hour + ) + already_sent_this_push_notification = cache_key in relevant_notifications_already_sent + + if ( + should_we_send_going_oncall_push_notification(now, mobile_app_user_settings, schedule_event) + and not already_sent_this_push_notification + ): message = _get_youre_going_oncall_fcm_message( user, schedule, device_to_notify, mobile_app_user_settings.going_oncall_notification_timing ) _send_push_notification(device_to_notify, message) + cache.set(cache_key, True, PUSH_NOTIFICATION_TRACKING_CACHE_KEY_TTL) + else: + logger.info( + f"Skipping sending going oncall push notification for user {user_pk} and shift {schedule_event['shift']['pk']}. " + f"Already sent: {already_sent_this_push_notification}" + ) @shared_dedicated_queue_retry_task() 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 b15e32c1d5..ad4f2fe083 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 @@ -1,74 +1,275 @@ +import typing from unittest import mock import pytest +from django.core.cache import cache from django.utils import timezone from fcm_django.models import FCMDevice +from apps.mobile_app import tasks from apps.mobile_app.models import MobileAppUserSettings -from apps.mobile_app.tasks import ( - conditionally_send_going_oncall_push_notifications_for_all_schedules, - conditionally_send_going_oncall_push_notifications_for_schedule, - should_we_send_going_oncall_push_notification, -) from apps.schedules.models import OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb from apps.schedules.models.on_call_schedule import ScheduleEvent -MOBILE_APP_BACKEND_ID = 5 -CLOUD_LICENSE_NAME = "Cloud" -OPEN_SOURCE_LICENSE_NAME = "OpenSource" +ONE_HOUR_IN_SECONDS = 60 * 60 +ONCALL_TIMING_PREFERENCE = ONE_HOUR_IN_SECONDS * 12 + + +class ScheduleEventUser(typing.TypedDict): + pk: str + + +@pytest.fixture(autouse=True) +def clear_cache(): + cache.clear() -def _create_schedule_event() -> ScheduleEvent: - return { - # TODO: - "start": timezone.now(), - } +def _create_schedule_event( + start_time: timezone.datetime, shift_pk: str, users: typing.List[ScheduleEventUser] +) -> ScheduleEvent: + return {"start": start_time, "shift": {"pk": shift_pk}, "users": users} +@pytest.mark.parametrize( + "timing_window_lower,timing_window_upper,seconds_until_shift_starts,expected", + [ + (0, 15 * 60, 0, True), + (0, 15 * 60, 1, True), + (0, 15 * 60, (15 * 60) - 1, True), + (0, 15 * 60, 15 * 60, True), + ], +) @pytest.mark.django_db -def test_should_we_send_going_oncall_push_notification(make_organization_and_user, make_schedule): - # create a user and connect a mobile device - organization, user = make_organization_and_user() - FCMDevice.objects.create(user=user, registration_id="test_device_id") +def test_shift_starts_within_range(timing_window_lower, timing_window_upper, seconds_until_shift_starts, expected): + assert ( + tasks._shift_starts_within_range(timing_window_lower, timing_window_upper, seconds_until_shift_starts) + == expected + ) - now = timezone.now() - schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) - print(schedule) - user_mobile_settings = MobileAppUserSettings.objects.create(user=user, important_notification_override_dnd=False) +@pytest.mark.parametrize( + "info_notifications_enabled,now,going_oncall_notification_timing,schedule_start,expected", + [ + # shift starts in 1h5m, user timing preference is 1h - don't send + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 13, 10, 0), + None, + ), + # shift starts in 1h4m, user timing preference is 1h - send only if info_notifications_enabled is true + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 13, 9, 0), + 64 * 60, + ), + ( + False, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 13, 9, 0), + None, + ), + # shift starts in 56m, user timing preference is 1h - send only if info_notifications_enabled is true + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 13, 1, 0), + 56 * 60, + ), + ( + False, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 13, 1, 0), + None, + ), + # shift starts in 55m, user timing preference is 1h - don't send + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 13, 0, 0), + None, + ), + # shift starts in 16m, don't send + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 12, 21, 0), + None, + ), + # shift starts in 15m - send only if info_notifications_enabled is true + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 12, 20, 0), + 15 * 60, + ), + ( + False, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 12, 20, 0), + None, + ), + # shift starts in 0secs - send only if info_notifications_enabled is true + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 12, 5, 0), + 0, + ), + ( + False, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 12, 5, 0), + None, + ), + # shift started 5secs ago - don't send + ( + True, + timezone.datetime(2022, 5, 2, 12, 5, 0), + ONE_HOUR_IN_SECONDS, + timezone.datetime(2022, 5, 2, 12, 4, 55), + None, + ), + ], +) +@pytest.mark.django_db +def test_should_we_send_going_oncall_push_notification( + make_organization_and_user, + info_notifications_enabled, + now, + going_oncall_notification_timing, + schedule_start, + expected, +): + _, user = make_organization_and_user() + user_mobile_settings = MobileAppUserSettings.objects.create( + user=user, + info_notifications_enabled=info_notifications_enabled, + going_oncall_notification_timing=going_oncall_notification_timing, + ) - # TODO: finish test - should_we_send_going_oncall_push_notification(now, user_mobile_settings, _create_schedule_event()) + assert ( + tasks.should_we_send_going_oncall_push_notification( + now, user_mobile_settings, _create_schedule_event(schedule_start, "12345", []) + ) + == expected + ) + + +def test_generate_going_oncall_push_notification_cache_key() -> None: + user_pk = "adfad" + schedule_event = {"shift": {"dfdfdf"}} + time = "2023-05-04-11h" + + assert ( + tasks._generate_going_oncall_push_notification_cache_key(user_pk, schedule_event, time) + == f"going_oncall_push_notification:{user_pk}:{schedule_event['shift']['pk']}:{time}" + ) +@mock.patch("apps.mobile_app.tasks._send_push_notification") @pytest.mark.django_db -def test_conditionally_send_going_oncall_push_notifications_for_schedule(make_organization_and_user, make_schedule): - # create a user and connect a mobile device +def test_conditionally_send_going_oncall_push_notifications_for_schedule_schedule_not_found( + mocked_send_push_notification, +): + tasks.conditionally_send_going_oncall_push_notifications_for_schedule("cvnmcvnmcnvmcnvmncv") + mocked_send_push_notification.assert_not_called() + + +@mock.patch("apps.mobile_app.tasks.OnCallSchedule.final_events") +@mock.patch("apps.mobile_app.tasks._send_push_notification") +@mock.patch("apps.mobile_app.tasks.should_we_send_going_oncall_push_notification") +@mock.patch("apps.mobile_app.tasks._get_youre_going_oncall_fcm_message") +@mock.patch("apps.mobile_app.tasks.timezone.now") +@pytest.mark.django_db +def test_conditionally_send_going_oncall_push_notifications_for_schedule( + mock_timezone_now, + mock_get_youre_going_oncall_fcm_message, + mock_should_we_send_going_oncall_push_notification, + mock_send_push_notification, + mock_oncall_schedule_final_events, + make_organization_and_user, + make_schedule, +): + timezone_now = timezone.datetime(2023, 5, 4, 10) + mock_timezone_now.return_value = timezone_now + organization, user = make_organization_and_user() - FCMDevice.objects.create(user=user, registration_id="test_device_id") + + shift_pk = "mncvmnvc" + user_pk = user.public_primary_key + mock_fcm_message = {"foo": "bar"} + final_events = [ + _create_schedule_event( + timezone.now(), + shift_pk, + [ + { + "pk": user_pk, + }, + ], + ), + ] + + mock_get_youre_going_oncall_fcm_message.return_value = mock_fcm_message + mock_should_we_send_going_oncall_push_notification.return_value = True + mock_oncall_schedule_final_events.return_value = final_events schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + current_date_with_hour = timezone_now.replace(minute=0, second=0, microsecond=0).strftime("%Y-%m-%d-%Hh") + cache_key = f"going_oncall_push_notification:{user_pk}:{shift_pk}:{current_date_with_hour}" + + assert cache.get(cache_key) is None + + # no device available + tasks.conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) + mock_send_push_notification.assert_not_called() + + # device available + device = FCMDevice.objects.create(user=user, registration_id="test_device_id") + MobileAppUserSettings.objects.create(user=user, going_oncall_notification_timing=ONCALL_TIMING_PREFERENCE) + + tasks.conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) - # TODO: finish test - conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) + mock_get_youre_going_oncall_fcm_message.assert_called_once_with(user, schedule, device, ONCALL_TIMING_PREFERENCE) + mock_send_push_notification.assert_called_once_with(device, mock_fcm_message) + assert cache.get(cache_key) is True + # we shouldn't double send the same push notification for the same user/shift within the same hour + tasks.conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) + assert mock_send_push_notification.call_count == 1 -@mock.patch("apps.mobile_app.tasks.conditionally_send_going_oncall_push_notifications_for_schedule", return_value=None) + # we will resend the push notification for the same user/shift for a different hour + mock_timezone_now.return_value = timezone_now + timezone.timedelta(hours=1) + tasks.conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) + assert mock_send_push_notification.call_count == 2 + + +@mock.patch("apps.mobile_app.tasks.conditionally_send_going_oncall_push_notifications_for_schedule") @pytest.mark.django_db def test_conditionally_send_going_oncall_push_notifications_for_all_schedules( mocked_conditionally_send_going_oncall_push_notifications_for_schedule, make_organization_and_user, make_schedule, ): - # create a user and connect a mobile device - organization, user = make_organization_and_user() - FCMDevice.objects.create(user=user, registration_id="test_device_id") - + organization, _ = make_organization_and_user() schedule1 = make_schedule(organization, schedule_class=OnCallScheduleCalendar) schedule2 = make_schedule(organization, schedule_class=OnCallScheduleICal) schedule3 = make_schedule(organization, schedule_class=OnCallScheduleWeb) - conditionally_send_going_oncall_push_notifications_for_all_schedules() + tasks.conditionally_send_going_oncall_push_notifications_for_all_schedules() mocked_conditionally_send_going_oncall_push_notifications_for_schedule.apply_async.assert_has_calls( [ From e71deac58ccbb95872105656c82ecd83e4536b01 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 4 May 2023 10:55:19 -0400 Subject: [PATCH 12/13] minor changes --- CHANGELOG.md | 2 +- engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py | 2 +- .../mobile_app/tests/test_your_going_oncall_notification.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3a06075ab..a78a61626b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add filter descriptions to web ui by @iskhakov ([1845](https://github.com/grafana/oncall/pull/1845)) - Add "Notifications Receiver" RBAC role by @joeyorlando ([#1853](https://github.com/grafana/oncall/pull/1853)) -- Add a new mobile app push notification which notifies users when they are going on by @joeyorlando ([#1814](https://github.com/grafana/oncall/pull/1814)) +- Add a new mobile app push notification which notifies users when they are going on call by @joeyorlando ([#1814](https://github.com/grafana/oncall/pull/1814)) ### Changed diff --git a/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py index 3f60bcd183..4e1cf9c811 100644 --- a/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py +++ b/engine/apps/mobile_app/migrations/0004_auto_20230425_1033.py @@ -34,6 +34,6 @@ class Migration(migrations.Migration): AddDefaultValue( model_name='mobileappusersettings', name='going_oncall_notification_timing', - value=0, + value=43200, ), ] 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 ad4f2fe083..52d56dc85a 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 @@ -170,7 +170,7 @@ def test_should_we_send_going_oncall_push_notification( def test_generate_going_oncall_push_notification_cache_key() -> None: user_pk = "adfad" - schedule_event = {"shift": {"dfdfdf"}} + schedule_event = {"shift": {"pk": "dfdfdf"}} time = "2023-05-04-11h" assert ( @@ -184,7 +184,7 @@ def test_generate_going_oncall_push_notification_cache_key() -> None: def test_conditionally_send_going_oncall_push_notifications_for_schedule_schedule_not_found( mocked_send_push_notification, ): - tasks.conditionally_send_going_oncall_push_notifications_for_schedule("cvnmcvnmcnvmcnvmncv") + tasks.conditionally_send_going_oncall_push_notifications_for_schedule(12345) mocked_send_push_notification.assert_not_called() From 5c6a64d96f35690e4684993729baac1a11f7bd64 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 4 May 2023 12:52:10 -0400 Subject: [PATCH 13/13] PR comments --- CHANGELOG.md | 5 +- engine/apps/mobile_app/tasks.py | 17 +++---- .../test_your_going_oncall_notification.py | 46 +++++++++---------- 3 files changed, 32 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41787a7d80..c500a53810 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 +### Added + +- Add a new mobile app push notification which notifies users when they are going on call by @joeyorlando ([#1814](https://github.com/grafana/oncall/pull/1814)) + ### Changed - Improve ical comparison when checking for imported ical updates ([1870](https://github.com/grafana/oncall/pull/1870)) @@ -23,7 +27,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add filter descriptions to web ui by @iskhakov ([1845](https://github.com/grafana/oncall/pull/1845)) - Add "Notifications Receiver" RBAC role by @joeyorlando ([#1853](https://github.com/grafana/oncall/pull/1853)) -- Add a new mobile app push notification which notifies users when they are going on call by @joeyorlando ([#1814](https://github.com/grafana/oncall/pull/1814)) ### Changed diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 3df746c291..d0476c5849 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -312,7 +312,7 @@ def should_we_send_going_oncall_push_notification( - schedule is starting in user's "configured notification timing preference" +/- a 4 minute buffer - schedule is starting within the next fifteen minutes """ - NOTIFICATION_TIMING_BUFFER = 4 * 60 # 4 minutes in seconds + NOTIFICATION_TIMING_BUFFER = 7 * 60 # 7 minutes in seconds FIFTEEN_MINUTES_IN_SECONDS = 15 * 60 # this _should_ always be positive since final_events is returning only events in the future @@ -326,7 +326,7 @@ def should_we_send_going_oncall_push_notification( logger.info("not sending going oncall push notification because info_notifications_enabled is false") return - # 8 minute window where the notification could be sent (4 mins before or 4 mins after) + # 14 minute window where the notification could be sent (7 mins before or 7 mins after) timing_window_lower = user_notification_timing_preference - NOTIFICATION_TIMING_BUFFER timing_window_upper = user_notification_timing_preference + NOTIFICATION_TIMING_BUFFER @@ -352,8 +352,8 @@ def should_we_send_going_oncall_push_notification( logger.info(f"timing is not right to send going oncall push notification\n{timing_logging_msg}") -def _generate_going_oncall_push_notification_cache_key(user_pk: str, schedule_event: ScheduleEvent, time: str) -> str: - return f"going_oncall_push_notification:{user_pk}:{schedule_event['shift']['pk']}:{time}" +def _generate_going_oncall_push_notification_cache_key(user_pk: str, schedule_event: ScheduleEvent) -> str: + return f"going_oncall_push_notification:{user_pk}:{schedule_event['shift']['pk']}" @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) @@ -361,7 +361,7 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) # avoid circular import from apps.mobile_app.models import MobileAppUserSettings - PUSH_NOTIFICATION_TRACKING_CACHE_KEY_TTL = 80 * 60 # 80 minutes + PUSH_NOTIFICATION_TRACKING_CACHE_KEY_TTL = 60 * 60 # 60 minutes user_cache: typing.Dict[str, User] = {} device_cache: typing.Dict[str, FCMDevice] = {} @@ -374,11 +374,10 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) return now = timezone.now() - current_date_with_hour = now.replace(minute=0, second=0, microsecond=0).strftime("%Y-%m-%d-%Hh") schedule_final_events = schedule.final_events("UTC", now, days=7) relevant_cache_keys = [ - _generate_going_oncall_push_notification_cache_key(user["pk"], schedule_event, current_date_with_hour) + _generate_going_oncall_push_notification_cache_key(user["pk"], schedule_event) for schedule_event in schedule_final_events for user in schedule_event["users"] ] @@ -413,9 +412,7 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) mobile_app_user_settings, _ = MobileAppUserSettings.objects.get_or_create(user=user) - cache_key = _generate_going_oncall_push_notification_cache_key( - user_pk, schedule_event, current_date_with_hour - ) + cache_key = _generate_going_oncall_push_notification_cache_key(user_pk, schedule_event) already_sent_this_push_notification = cache_key in relevant_notifications_already_sent if ( 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 52d56dc85a..76b77b7ba1 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 @@ -50,50 +50,50 @@ def test_shift_starts_within_range(timing_window_lower, timing_window_upper, sec @pytest.mark.parametrize( "info_notifications_enabled,now,going_oncall_notification_timing,schedule_start,expected", [ - # shift starts in 1h5m, user timing preference is 1h - don't send + # shift starts in 1h8m, user timing preference is 1h - don't send ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), ONE_HOUR_IN_SECONDS, - timezone.datetime(2022, 5, 2, 13, 10, 0), + timezone.datetime(2022, 5, 2, 13, 13, 0), None, ), - # shift starts in 1h4m, user timing preference is 1h - send only if info_notifications_enabled is true + # shift starts in 1h7m, user timing preference is 1h - send only if info_notifications_enabled is true ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), ONE_HOUR_IN_SECONDS, - timezone.datetime(2022, 5, 2, 13, 9, 0), - 64 * 60, + timezone.datetime(2022, 5, 2, 13, 12, 0), + 67 * 60, ), ( False, timezone.datetime(2022, 5, 2, 12, 5, 0), ONE_HOUR_IN_SECONDS, - timezone.datetime(2022, 5, 2, 13, 9, 0), + timezone.datetime(2022, 5, 2, 13, 12, 0), None, ), - # shift starts in 56m, user timing preference is 1h - send only if info_notifications_enabled is true + # shift starts in 53m, user timing preference is 1h - send only if info_notifications_enabled is true ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), ONE_HOUR_IN_SECONDS, - timezone.datetime(2022, 5, 2, 13, 1, 0), - 56 * 60, + timezone.datetime(2022, 5, 2, 12, 58, 0), + 53 * 60, ), ( False, timezone.datetime(2022, 5, 2, 12, 5, 0), ONE_HOUR_IN_SECONDS, - timezone.datetime(2022, 5, 2, 13, 1, 0), + timezone.datetime(2022, 5, 2, 12, 58, 0), None, ), - # shift starts in 55m, user timing preference is 1h - don't send + # shift starts in 52m, user timing preference is 1h - don't send ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), ONE_HOUR_IN_SECONDS, - timezone.datetime(2022, 5, 2, 13, 0, 0), + timezone.datetime(2022, 5, 2, 12, 57, 0), None, ), # shift starts in 16m, don't send @@ -171,11 +171,10 @@ def test_should_we_send_going_oncall_push_notification( def test_generate_going_oncall_push_notification_cache_key() -> None: user_pk = "adfad" schedule_event = {"shift": {"pk": "dfdfdf"}} - time = "2023-05-04-11h" assert ( - tasks._generate_going_oncall_push_notification_cache_key(user_pk, schedule_event, time) - == f"going_oncall_push_notification:{user_pk}:{schedule_event['shift']['pk']}:{time}" + tasks._generate_going_oncall_push_notification_cache_key(user_pk, schedule_event) + == f"going_oncall_push_notification:{user_pk}:{schedule_event['shift']['pk']}" ) @@ -192,10 +191,8 @@ def test_conditionally_send_going_oncall_push_notifications_for_schedule_schedul @mock.patch("apps.mobile_app.tasks._send_push_notification") @mock.patch("apps.mobile_app.tasks.should_we_send_going_oncall_push_notification") @mock.patch("apps.mobile_app.tasks._get_youre_going_oncall_fcm_message") -@mock.patch("apps.mobile_app.tasks.timezone.now") @pytest.mark.django_db def test_conditionally_send_going_oncall_push_notifications_for_schedule( - mock_timezone_now, mock_get_youre_going_oncall_fcm_message, mock_should_we_send_going_oncall_push_notification, mock_send_push_notification, @@ -203,9 +200,6 @@ def test_conditionally_send_going_oncall_push_notifications_for_schedule( make_organization_and_user, make_schedule, ): - timezone_now = timezone.datetime(2023, 5, 4, 10) - mock_timezone_now.return_value = timezone_now - organization, user = make_organization_and_user() shift_pk = "mncvmnvc" @@ -228,8 +222,7 @@ def test_conditionally_send_going_oncall_push_notifications_for_schedule( mock_oncall_schedule_final_events.return_value = final_events schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) - current_date_with_hour = timezone_now.replace(minute=0, second=0, microsecond=0).strftime("%Y-%m-%d-%Hh") - cache_key = f"going_oncall_push_notification:{user_pk}:{shift_pk}:{current_date_with_hour}" + cache_key = f"going_oncall_push_notification:{user_pk}:{shift_pk}" assert cache.get(cache_key) is None @@ -247,14 +240,17 @@ def test_conditionally_send_going_oncall_push_notifications_for_schedule( mock_send_push_notification.assert_called_once_with(device, mock_fcm_message) assert cache.get(cache_key) is True - # we shouldn't double send the same push notification for the same user/shift within the same hour + # we shouldn't double send the same push notification for the same user/shift tasks.conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) assert mock_send_push_notification.call_count == 1 - # we will resend the push notification for the same user/shift for a different hour - mock_timezone_now.return_value = timezone_now + timezone.timedelta(hours=1) + # if the cache key expires we will resend the push notification for the same user/shift + # (in reality we're setting a timeout on the cache key, here we will just delete it to simulate this) + cache.delete(cache_key) + tasks.conditionally_send_going_oncall_push_notifications_for_schedule(schedule.pk) assert mock_send_push_notification.call_count == 2 + assert cache.get(cache_key) is True @mock.patch("apps.mobile_app.tasks.conditionally_send_going_oncall_push_notifications_for_schedule")