From 684a41a5ac4abeeead3ab9571d591c127e766f8a Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 4 Jul 2023 12:03:48 +0200 Subject: [PATCH 1/3] set DELETE_INACTIVE_DEVICES to true --- CHANGELOG.md | 5 +++ engine/apps/mobile_app/backend.py | 7 ++-- engine/apps/mobile_app/demo_push.py | 11 ++++-- engine/apps/mobile_app/fcm_relay.py | 2 +- engine/apps/mobile_app/models.py | 38 +++++++++++++++---- engine/apps/mobile_app/tasks.py | 20 +++++----- .../apps/mobile_app/tests/test_demo_push.py | 3 +- .../mobile_app/tests/test_fcm_device_model.py | 23 +++++++++++ .../apps/mobile_app/tests/test_fcm_relay.py | 2 +- .../apps/mobile_app/tests/test_notify_user.py | 3 +- .../test_your_going_oncall_notification.py | 3 +- engine/settings/base.py | 2 +- 12 files changed, 87 insertions(+), 32 deletions(-) create mode 100644 engine/apps/mobile_app/tests/test_fcm_device_model.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 259981f836..87f7b6a2f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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)) +### 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.3 (2023-06-29) ### Added diff --git a/engine/apps/mobile_app/backend.py b/engine/apps/mobile_app/backend.py index bec8c180ed..83f46b47e2 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 @@ def generate_user_verification_code(self, user): ) 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 1b6bcce275..557c34e370 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 a8a4720156..060d39f638 100644 --- a/engine/apps/mobile_app/fcm_relay.py +++ b/engine/apps/mobile_app/fcm_relay.py @@ -2,7 +2,6 @@ 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.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 aa94b43532..83e148d6f3 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.get(user=user) + + 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 @@ def create_auth_token(cls, user: User, organization: Organization) -> Tuple["Mob 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 VolumeType(models.TextChoices): 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 9134640b1c..31b6d2891c 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -12,7 +12,6 @@ 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.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 _error_cb(): 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 _format_datetime(dt: datetime.datetime) -> str: 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 _create_error_log_record(): 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 cb6ae51a79..73041ac376 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 0000000000..dc57f60846 --- /dev/null +++ b/engine/apps/mobile_app/tests/test_fcm_device_model.py @@ -0,0 +1,23 @@ +import pytest +from django.core.exceptions import MultipleObjectsReturned + +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 + + # if the user has two active devices, django will complain. however, we "should" never get to this + # case because we have the ONE_DEVICE_PER_USER FCM django setting set to True + _, user = make_organization_and_user() + FCMDevice.objects.create(user=user, registration_id="active_device_1") + FCMDevice.objects.create(user=user, registration_id="active_device_2") + + with pytest.raises(MultipleObjectsReturned): + FCMDevice.get_active_device_for_user(user) diff --git a/engine/apps/mobile_app/tests/test_fcm_relay.py b/engine/apps/mobile_app/tests/test_fcm_relay.py index bc2b3e582d..83e665fd99 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 @@ 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 b3b64eefe5..17e28d12c5 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 ebf438c0a8..3d02384afd 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 @@ 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 aaf1c2563b..a7326a6be1 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -619,7 +619,7 @@ class BrokerTypes: "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", } From d7a6f453c7ad96c72b71609794645e42416ebbc4 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 5 Jul 2023 16:58:47 +0200 Subject: [PATCH 2/3] update changelog --- CHANGELOG.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87f7b6a2f7..706e9f1575 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,12 +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)) - -### 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)) +- Removed url from sms notification, changed format ([2317](https://github.com/grafana/oncall/pull/2317)) ## v1.3.3 (2023-06-29) From 7014b24b2788a5a233ddac825c1dfa395cf5b5e3 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 5 Jul 2023 17:09:30 +0200 Subject: [PATCH 3/3] update tests --- engine/apps/mobile_app/models.py | 2 +- engine/apps/mobile_app/tests/test_fcm_device_model.py | 10 ---------- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index 83e148d6f3..afd4ddab93 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -31,7 +31,7 @@ class FCMDevice(BaseFCMDevice): @classmethod def get_active_device_for_user(cls, user: "User") -> FCMDevice | None: - return cls.active_objects.get(user=user) + return cls.active_objects.filter(user=user).first() class MobileAppVerificationTokenQueryset(models.QuerySet): diff --git a/engine/apps/mobile_app/tests/test_fcm_device_model.py b/engine/apps/mobile_app/tests/test_fcm_device_model.py index dc57f60846..2561a3fa5f 100644 --- a/engine/apps/mobile_app/tests/test_fcm_device_model.py +++ b/engine/apps/mobile_app/tests/test_fcm_device_model.py @@ -1,5 +1,4 @@ import pytest -from django.core.exceptions import MultipleObjectsReturned from apps.mobile_app.models import FCMDevice @@ -12,12 +11,3 @@ def test_get_active_device_for_user_works(make_organization_and_user): assert FCMDevice.objects.filter(user=user).count() == 2 assert FCMDevice.get_active_device_for_user(user) == active_device - - # if the user has two active devices, django will complain. however, we "should" never get to this - # case because we have the ONE_DEVICE_PER_USER FCM django setting set to True - _, user = make_organization_and_user() - FCMDevice.objects.create(user=user, registration_id="active_device_1") - FCMDevice.objects.create(user=user, registration_id="active_device_2") - - with pytest.raises(MultipleObjectsReturned): - FCMDevice.get_active_device_for_user(user)