diff --git a/CHANGELOG.md b/CHANGELOG.md index 259981f8..706e9f15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +## v1.3.6 (2023-07-05) + +### Fixed + +- Address issue where having multiple registered mobile apps for a user could lead to issues in delivering push + notifications by @joeyorlando ([#2421](https://github.com/grafana/oncall/pull/2421)) + ## v1.3.5 (2023-07-05) ### Fixed @@ -23,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - UI drawer updates for webhooks2 ([#2419](https://github.com/grafana/oncall/pull/2419)) -- Removed url from sms notification, changed format ([2317](https://github.com/grafana/oncall/pull/2317)) +- Removed url from sms notification, changed format ([2317](https://github.com/grafana/oncall/pull/2317)) ## v1.3.3 (2023-06-29) diff --git a/engine/apps/mobile_app/backend.py b/engine/apps/mobile_app/backend.py index bec8c180..83f46b47 100644 --- a/engine/apps/mobile_app/backend.py +++ b/engine/apps/mobile_app/backend.py @@ -1,7 +1,6 @@ import json from django.conf import settings -from fcm_django.models import FCMDevice from apps.base.messaging import BaseMessagingBackend from apps.mobile_app.tasks import notify_user_async @@ -29,13 +28,15 @@ class MobileAppBackend(BaseMessagingBackend): ) def unlink_user(self, user): - from apps.mobile_app.models import MobileAppAuthToken + from apps.mobile_app.models import FCMDevice, MobileAppAuthToken token = MobileAppAuthToken.objects.get(user=user) token.delete() # delete push notification related info for user - FCMDevice.objects.filter(user=user).delete() + user_active_device = FCMDevice.get_active_device_for_user(user) + if user_active_device is not None: + user_active_device.delete() def serialize_user(self, user): from apps.mobile_app.models import MobileAppAuthToken diff --git a/engine/apps/mobile_app/demo_push.py b/engine/apps/mobile_app/demo_push.py index 1b6bcce2..557c34e3 100644 --- a/engine/apps/mobile_app/demo_push.py +++ b/engine/apps/mobile_app/demo_push.py @@ -1,17 +1,22 @@ import json import random import string +import typing -from fcm_django.models import FCMDevice 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.user_management.models import User +if typing.TYPE_CHECKING: + from apps.mobile_app.models import FCMDevice + def send_test_push(user, critical=False): - device_to_notify = FCMDevice.objects.filter(user=user).first() + from apps.mobile_app.models import FCMDevice + + device_to_notify = FCMDevice.get_active_device_for_user(user) if device_to_notify is None: logger.info(f"send_test_push: fcm_device not found user_id={user.id}") raise DeviceNotSet @@ -19,7 +24,7 @@ def send_test_push(user, critical=False): _send_push_notification(device_to_notify, message) -def _get_test_escalation_fcm_message(user: User, device_to_notify: FCMDevice, critical: bool) -> Message: +def _get_test_escalation_fcm_message(user: User, device_to_notify: "FCMDevice", critical: bool) -> Message: # TODO: this method is copied from _get_alert_group_escalation_fcm_message # to have same notification/sound/overrideDND logic. Ideally this logic should be abstracted, not repeated. from apps.mobile_app.models import MobileAppUserSettings diff --git a/engine/apps/mobile_app/fcm_relay.py b/engine/apps/mobile_app/fcm_relay.py index a8a47201..060d39f6 100644 --- a/engine/apps/mobile_app/fcm_relay.py +++ b/engine/apps/mobile_app/fcm_relay.py @@ -2,7 +2,6 @@ import logging from celery.utils.log import get_task_logger from django.conf import settings -from fcm_django.models import FCMDevice from firebase_admin.exceptions import FirebaseError from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message from rest_framework import status @@ -12,6 +11,7 @@ from rest_framework.throttling import UserRateThrottle from rest_framework.views import APIView from apps.auth_token.auth import ApiTokenAuthentication +from apps.mobile_app.models import FCMDevice from common.custom_celery_tasks import shared_dedicated_queue_retry_task task_logger = get_task_logger(__name__) diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index aa94b435..afd4ddab 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -1,13 +1,18 @@ -from typing import Tuple +from __future__ import annotations # https://stackoverflow.com/a/33533514 + +import typing from django.conf import settings from django.core import validators from django.db import models from django.utils import timezone +from fcm_django.models import FCMDevice as BaseFCMDevice from apps.auth_token import constants, crypto from apps.auth_token.models import BaseAuthToken -from apps.user_management.models import Organization, User + +if typing.TYPE_CHECKING: + from apps.user_management.models import Organization, User MOBILE_APP_AUTH_VERIFICATION_TOKEN_TIMEOUT_SECONDS = 60 * (5 if settings.DEBUG else 1) @@ -16,6 +21,19 @@ def get_expire_date(): return timezone.now() + timezone.timedelta(seconds=MOBILE_APP_AUTH_VERIFICATION_TOKEN_TIMEOUT_SECONDS) +class ActiveFCMDeviceQuerySet(models.QuerySet): + def filter(self, *args, **kwargs): + return super().filter(*args, **kwargs, active=True) + + +class FCMDevice(BaseFCMDevice): + active_objects = ActiveFCMDeviceQuerySet.as_manager() + + @classmethod + def get_active_device_for_user(cls, user: "User") -> FCMDevice | None: + return cls.active_objects.filter(user=user).first() + + class MobileAppVerificationTokenQueryset(models.QuerySet): def filter(self, *args, **kwargs): now = timezone.now() @@ -38,7 +56,9 @@ class MobileAppVerificationToken(BaseAuthToken): expire_date = models.DateTimeField(default=get_expire_date) @classmethod - def create_auth_token(cls, user: User, organization: Organization) -> Tuple["MobileAppVerificationToken", str]: + def create_auth_token( + cls, user: "User", organization: "Organization" + ) -> typing.Tuple["MobileAppVerificationToken", str]: token_string = crypto.generate_short_token_string() digest = crypto.hash_token_string(token_string) @@ -54,13 +74,17 @@ class MobileAppVerificationToken(BaseAuthToken): class MobileAppAuthToken(BaseAuthToken): objects: models.Manager["MobileAppAuthToken"] - user = models.OneToOneField(to=User, null=False, blank=False, on_delete=models.CASCADE) + user = models.OneToOneField(to="user_management.User", null=False, blank=False, on_delete=models.CASCADE) organization = models.ForeignKey( - to=Organization, null=False, blank=False, related_name="mobile_app_auth_tokens", on_delete=models.CASCADE + to="user_management.Organization", + null=False, + blank=False, + related_name="mobile_app_auth_tokens", + on_delete=models.CASCADE, ) @classmethod - def create_auth_token(cls, user: User, organization: Organization) -> Tuple["MobileAppAuthToken", str]: + def create_auth_token(cls, user: "User", organization: "Organization") -> typing.Tuple["MobileAppAuthToken", str]: token_string = crypto.generate_token_string() digest = crypto.hash_token_string(token_string) @@ -82,7 +106,7 @@ class MobileAppUserSettings(models.Model): CONSTANT = "constant" INTENSIFYING = "intensifying" - user = models.OneToOneField(to=User, null=False, on_delete=models.CASCADE) + user = models.OneToOneField(to="user_management.User", null=False, on_delete=models.CASCADE) # Push notification settings for default notifications default_notification_sound_name = models.CharField(max_length=100, default="default_sound") diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 9134640b..31b6d289 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -12,7 +12,6 @@ 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 from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message from requests import HTTPError @@ -29,7 +28,7 @@ from common.l10n import format_localized_datetime, format_localized_time from common.timezones import is_valid_timezone if typing.TYPE_CHECKING: - from apps.mobile_app.models import MobileAppUserSettings + from apps.mobile_app.models import FCMDevice, MobileAppUserSettings MAX_RETRIES = 1 if settings.DEBUG else 10 @@ -64,7 +63,7 @@ def send_push_notification_to_fcm_relay(message: Message) -> requests.Response: def _send_push_notification( - device_to_notify: FCMDevice, message: Message, error_cb: typing.Optional[typing.Callable[..., None]] = None + device_to_notify: "FCMDevice", message: Message, error_cb: typing.Optional[typing.Callable[..., None]] = None ) -> None: logger.debug(f"Sending push notification to device type {device_to_notify.type} with message: {message}") @@ -105,7 +104,7 @@ def _send_push_notification( def _construct_fcm_message( message_type: MessageType, - device_to_notify: FCMDevice, + device_to_notify: "FCMDevice", thread_id: str, data: FCMMessageData, apns_payload: typing.Optional[APNSPayload] = None, @@ -151,7 +150,7 @@ def _construct_fcm_message( def _get_alert_group_escalation_fcm_message( - alert_group: AlertGroup, user: User, device_to_notify: FCMDevice, critical: bool + alert_group: AlertGroup, user: User, device_to_notify: "FCMDevice", critical: bool ) -> Message: # avoid circular import from apps.mobile_app.models import MobileAppUserSettings @@ -265,7 +264,7 @@ def _get_youre_going_oncall_notification_subtitle( def _get_youre_going_oncall_fcm_message( user: User, schedule: OnCallSchedule, - device_to_notify: FCMDevice, + device_to_notify: "FCMDevice", seconds_until_going_oncall: int, schedule_event: ScheduleEvent, ) -> Message: @@ -314,6 +313,7 @@ def _get_youre_going_oncall_fcm_message( def notify_user_async(user_pk, alert_group_pk, notification_policy_pk, critical): # avoid circular import from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord + from apps.mobile_app.models import FCMDevice try: user = User.objects.get(pk=user_pk) @@ -347,7 +347,7 @@ def notify_user_async(user_pk, alert_group_pk, notification_policy_pk, critical) notification_channel=notification_policy.notify_by, ) - device_to_notify = FCMDevice.objects.filter(user=user).first() + device_to_notify = FCMDevice.get_active_device_for_user(user) # create an error log in case user has no devices set up if not device_to_notify: @@ -431,11 +431,11 @@ def _generate_going_oncall_push_notification_cache_key(user_pk: str, schedule_ev @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: # avoid circular import - from apps.mobile_app.models import MobileAppUserSettings + from apps.mobile_app.models import FCMDevice, MobileAppUserSettings PUSH_NOTIFICATION_TRACKING_CACHE_KEY_TTL = 60 * 60 # 60 minutes user_cache: typing.Dict[str, User] = {} - device_cache: typing.Dict[str, FCMDevice] = {} + device_cache: typing.Dict[str, "FCMDevice"] = {} logger.info(f"Start calculate_going_oncall_push_notifications_for_schedule for schedule {schedule_pk}") @@ -473,7 +473,7 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) device_to_notify = device_cache.get(user_pk, None) if device_to_notify is None: - device_to_notify = FCMDevice.objects.filter(user=user).first() + device_to_notify = FCMDevice.get_active_device_for_user(user) if not device_to_notify: continue diff --git a/engine/apps/mobile_app/tests/test_demo_push.py b/engine/apps/mobile_app/tests/test_demo_push.py index cb6ae51a..73041ac3 100644 --- a/engine/apps/mobile_app/tests/test_demo_push.py +++ b/engine/apps/mobile_app/tests/test_demo_push.py @@ -1,8 +1,7 @@ import pytest -from fcm_django.models import FCMDevice from apps.mobile_app.demo_push import _get_test_escalation_fcm_message, get_test_push_title -from apps.mobile_app.models import MobileAppUserSettings +from apps.mobile_app.models import FCMDevice, MobileAppUserSettings @pytest.mark.django_db diff --git a/engine/apps/mobile_app/tests/test_fcm_device_model.py b/engine/apps/mobile_app/tests/test_fcm_device_model.py new file mode 100644 index 00000000..2561a3fa --- /dev/null +++ b/engine/apps/mobile_app/tests/test_fcm_device_model.py @@ -0,0 +1,13 @@ +import pytest + +from apps.mobile_app.models import FCMDevice + + +@pytest.mark.django_db +def test_get_active_device_for_user_works(make_organization_and_user): + _, user = make_organization_and_user() + FCMDevice.objects.create(user=user, registration_id="inactive_device_id", active=False) + active_device = FCMDevice.objects.create(user=user, registration_id="active_device_id", active=True) + + assert FCMDevice.objects.filter(user=user).count() == 2 + assert FCMDevice.get_active_device_for_user(user) == active_device diff --git a/engine/apps/mobile_app/tests/test_fcm_relay.py b/engine/apps/mobile_app/tests/test_fcm_relay.py index bc2b3e58..83e665fd 100644 --- a/engine/apps/mobile_app/tests/test_fcm_relay.py +++ b/engine/apps/mobile_app/tests/test_fcm_relay.py @@ -3,12 +3,12 @@ from unittest.mock import patch import pytest from django.urls import reverse -from fcm_django.models import FCMDevice from firebase_admin.exceptions import FirebaseError from rest_framework import status 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.models import FCMDevice from apps.mobile_app.tasks import _get_alert_group_escalation_fcm_message diff --git a/engine/apps/mobile_app/tests/test_notify_user.py b/engine/apps/mobile_app/tests/test_notify_user.py index b3b64eef..17e28d12 100644 --- a/engine/apps/mobile_app/tests/test_notify_user.py +++ b/engine/apps/mobile_app/tests/test_notify_user.py @@ -1,11 +1,10 @@ from unittest.mock import patch import pytest -from fcm_django.models import FCMDevice from firebase_admin.exceptions import FirebaseError from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord -from apps.mobile_app.models import MobileAppUserSettings +from apps.mobile_app.models import FCMDevice, MobileAppUserSettings from apps.mobile_app.tasks import _get_alert_group_escalation_fcm_message, notify_user_async from apps.oss_installation.models import CloudConnector 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 ebf438c0..3d02384a 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 @@ -5,10 +5,9 @@ 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.models import FCMDevice, MobileAppUserSettings from apps.schedules.models import OnCallScheduleCalendar, OnCallScheduleICal, OnCallScheduleWeb from apps.schedules.models.on_call_schedule import ScheduleEvent diff --git a/engine/settings/base.py b/engine/settings/base.py index aaf1c256..a7326a6b 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -619,7 +619,7 @@ FCM_DJANGO_SETTINGS = { "DEFAULT_FIREBASE_APP": initialize_app(credential=credential, options={"projectId": FCM_PROJECT_ID}), "APP_VERBOSE_NAME": "OnCall", "ONE_DEVICE_PER_USER": True, - "DELETE_INACTIVE_DEVICES": False, + "DELETE_INACTIVE_DEVICES": True, "UPDATE_ON_DUPLICATE_REG_ID": True, "USER_MODEL": "user_management.User", }