From 424b2b80f8eaeac53bb6086e3f693c18a3c2e55b Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 7 Aug 2023 10:55:17 +0100 Subject: [PATCH] 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]