address mobile device push notification delivery issue when user had > 1 registered device (#2421)

# What this PR does

Address issue where if the user had multiple registered devices w/ FCM,
doing django queries like `.first()` could potentially pick the wrong
device. Do this in two ways:
1. set the `DELETE_INACTIVE_DEVICES` `fcm_django` setting to `True`.
According to the
[docs](20e275618b/README.rst (L127-L130)),
this works as follows:

> devices to which notifications cannot be sent, are deleted upon
receiving error response from FCM
2. Customizing the `FCMDevice` model provided by `fcm_django`. Add a new
method, `get_active_device_for_user`, so that we can centralize the
logic for this rather than duplicating
`FCMDevice.objects.filter(user=user).first()`

## Which issue(s) this PR fixes

https://raintank-corp.slack.com/archives/C0229FD3CE9/p1688461915752119

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required) (N/A)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)

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)
 

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

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

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__)

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")

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

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

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

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
 
 

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
 

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
 

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",
 }