diff --git a/CHANGELOG.md b/CHANGELOG.md index 277717d0..687c0a07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Simplify Direct Paging workflow. Now when using Direct Paging you either simply specify a team, or one or more users to page by @joeyorlando ([#3128](https://github.com/grafana/oncall/pull/3128)) +- Enable timing options for mobile push notifications, allow multi-select by @Ferril ([#3187](https://github.com/grafana/oncall/pull/3187)) ### Fixed diff --git a/engine/apps/mobile_app/migrations/0011_alter_mobileappusersettings_going_oncall_notification_timing.py b/engine/apps/mobile_app/migrations/0011_alter_mobileappusersettings_going_oncall_notification_timing.py new file mode 100644 index 00000000..c758fa8c --- /dev/null +++ b/engine/apps/mobile_app/migrations/0011_alter_mobileappusersettings_going_oncall_notification_timing.py @@ -0,0 +1,30 @@ +# Generated by Django 3.2.20 on 2023-10-30 09:25 + +import apps.mobile_app.models +import django_migration_linter as linter + +from django.db import migrations, models +from apps.mobile_app.models import default_notification_timing_options + + +def set_going_oncall_notification_timing_to_default(apps, schema_editor): + MobileAppUserSettings = apps.get_model("mobile_app", "MobileAppUserSettings") + default = default_notification_timing_options() + MobileAppUserSettings.objects.all().update(going_oncall_notification_timing=default) + + +class Migration(migrations.Migration): + + dependencies = [ + ('mobile_app', '0010_mobileappusersettings_time_zone'), + ] + + operations = [ + linter.IgnoreMigration(), + migrations.AlterField( + model_name='mobileappusersettings', + name='going_oncall_notification_timing', + field=models.JSONField(default=apps.mobile_app.models.default_notification_timing_options), + ), + migrations.RunPython(set_going_oncall_notification_timing_to_default, migrations.RunPython.noop), + ] diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index d526f160..cb24a396 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -4,6 +4,7 @@ import typing from django.core import validators from django.db import models +from django.db.models import JSONField from django.utils import timezone from fcm_django.models import FCMDevice as BaseFCMDevice @@ -21,6 +22,10 @@ def get_expire_date(): return timezone.now() + timezone.timedelta(seconds=MOBILE_APP_AUTH_VERIFICATION_TOKEN_TIMEOUT_SECONDS) +def default_notification_timing_options(): + return [MobileAppUserSettings.FIFTEEN_MINUTES_IN_SECONDS] + + class ActiveFCMDeviceQuerySet(models.QuerySet): def filter(self, *args, **kwargs): return super().filter(*args, **kwargs, active=True) @@ -159,19 +164,20 @@ class MobileAppUserSettings(models.Model): # 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) + FIFTEEN_MINUTES_IN_SECONDS = 15 * 60 + ONE_HOUR_IN_SECONDS = 60 * 60 + SIX_HOURS_IN_SECONDS = 6 * 60 * 60 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 = ( + (FIFTEEN_MINUTES_IN_SECONDS, "fifteen minutes before"), + (ONE_HOUR_IN_SECONDS, "one hour before"), + (SIX_HOURS_IN_SECONDS, "six hours before"), (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 ) + going_oncall_notification_timing = JSONField(default=default_notification_timing_options) locale = models.CharField(max_length=50, null=True) time_zone = models.CharField(max_length=100, default="UTC") diff --git a/engine/apps/mobile_app/serializers.py b/engine/apps/mobile_app/serializers.py index 4551deec..8a321aff 100644 --- a/engine/apps/mobile_app/serializers.py +++ b/engine/apps/mobile_app/serializers.py @@ -1,3 +1,5 @@ +import typing + from rest_framework import serializers from apps.mobile_app.models import MobileAppUserSettings @@ -6,6 +8,7 @@ from common.api_helpers.custom_fields import TimeZoneField class MobileAppUserSettingsSerializer(serializers.ModelSerializer): time_zone = TimeZoneField(required=False, allow_null=False) + going_oncall_notification_timing = serializers.ListField(required=False, allow_null=False) class Meta: model = MobileAppUserSettings @@ -28,3 +31,15 @@ class MobileAppUserSettingsSerializer(serializers.ModelSerializer): "locale", "time_zone", ) + + def validate_going_oncall_notification_timing( + self, going_oncall_notification_timing: typing.Optional[typing.List[int]] + ) -> typing.Optional[typing.List[int]]: + if going_oncall_notification_timing is not None: + if len(going_oncall_notification_timing) == 0: + raise serializers.ValidationError(detail="invalid timing options") + notification_timing_options = [opt[0] for opt in MobileAppUserSettings.NOTIFICATION_TIMING_CHOICES] + for option in going_oncall_notification_timing: + if option not in notification_timing_options: + raise serializers.ValidationError(detail="invalid timing options") + return going_oncall_notification_timing diff --git a/engine/apps/mobile_app/tasks/going_oncall_notification.py b/engine/apps/mobile_app/tasks/going_oncall_notification.py index 248778a6..05406e10 100644 --- a/engine/apps/mobile_app/tasks/going_oncall_notification.py +++ b/engine/apps/mobile_app/tasks/going_oncall_notification.py @@ -121,7 +121,6 @@ def _should_we_send_push_notification( an `int` which represents the # of seconds until the oncall shift starts. """ 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 seconds_until_shift_starts = math.floor((schedule_event["start"] - now).total_seconds()) @@ -134,32 +133,33 @@ def _should_we_send_push_notification( logger.info("not sending going oncall push notification because info_notifications_enabled is false") return None - # 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 + for timing_preference in user_notification_timing_preference: + # 14 minute window where the notification could be sent (7 mins before or 7 mins after) + timing_window_lower = timing_preference - NOTIFICATION_TIMING_BUFFER + timing_window_upper = 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 - ) + shift_starts_within_users_notification_timing_preference = _shift_starts_within_range( + timing_window_lower, timing_window_upper, seconds_until_shift_starts + ) - timing_logging_msg = ( + if shift_starts_within_users_notification_timing_preference: + logger.info( + f"timing is right to send going oncall push notification\n" + f"seconds_until_shift_starts: {seconds_until_shift_starts}\n" + f"user_notification_timing_preference: {user_notification_timing_preference}\n" + f"current timing_preference: {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" + ) + return seconds_until_shift_starts + + logger.info( + f"timing is not right to send going oncall push notification\n" 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}" + f"shift_starts_within_users_notification_timing_preference: False\n" ) - - # Temporary remove `shift_starts_within_users_notification_timing_preference` from condition to send notification only 15 minutes before the shift starts - # TODO: Return it once mobile app ready and default value is changed (https://github.com/grafana/oncall/issues/1999) - if shift_starts_within_fifteen_minutes: - logger.info(f"timing is right to send going oncall push notification\n{timing_logging_msg}") - return seconds_until_shift_starts - logger.info(f"timing is not right to send going oncall push notification\n{timing_logging_msg}") return None diff --git a/engine/apps/mobile_app/tests/tasks/test_going_oncall_notification.py b/engine/apps/mobile_app/tests/tasks/test_going_oncall_notification.py index a15067ba..c4f6d799 100644 --- a/engine/apps/mobile_app/tests/tasks/test_going_oncall_notification.py +++ b/engine/apps/mobile_app/tests/tasks/test_going_oncall_notification.py @@ -21,6 +21,7 @@ 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 +FIFTEEN_MINUTES_IN_SECONDS = 15 * 60 ONE_HOUR_IN_SECONDS = 60 * 60 ONCALL_TIMING_PREFERENCE = ONE_HOUR_IN_SECONDS * 12 @@ -254,7 +255,7 @@ def test_get_fcm_message( ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 13, 13, 0), None, ), @@ -262,14 +263,14 @@ def test_get_fcm_message( ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 13, 12, 0), - None, + 67 * 60, ), ( False, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 13, 12, 0), None, ), @@ -277,14 +278,14 @@ def test_get_fcm_message( ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 12, 58, 0), - None, + 53 * 60, ), ( False, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 12, 58, 0), None, ), @@ -292,7 +293,7 @@ def test_get_fcm_message( ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 12, 57, 0), None, ), @@ -300,37 +301,30 @@ def test_get_fcm_message( ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [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 + # shift starts in 15m, user timing preference is 1h and 15m - send only if info_notifications_enabled is true ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS, FIFTEEN_MINUTES_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, + [ONE_HOUR_IN_SECONDS, FIFTEEN_MINUTES_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, - ), + # shift starts in 0secs - don't send ( False, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 12, 5, 0), None, ), @@ -338,7 +332,7 @@ def test_get_fcm_message( ( True, timezone.datetime(2022, 5, 2, 12, 5, 0), - ONE_HOUR_IN_SECONDS, + [ONE_HOUR_IN_SECONDS], timezone.datetime(2022, 5, 2, 12, 4, 55), None, ), diff --git a/engine/apps/mobile_app/tests/test_user_settings.py b/engine/apps/mobile_app/tests/test_user_settings.py index 43ceede9..9a7238c9 100644 --- a/engine/apps/mobile_app/tests/test_user_settings.py +++ b/engine/apps/mobile_app/tests/test_user_settings.py @@ -33,20 +33,42 @@ def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token "important_notification_volume_override": True, "important_notification_override_dnd": True, "info_notifications_enabled": False, - "going_oncall_notification_timing": 43200, + "going_oncall_notification_timing": [900], "locale": None, "time_zone": "UTC", } +@pytest.mark.django_db +def test_user_settings_get_notification_timing_options(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:notification_timing_options") + + choices = [ + {"value": item[0], "display_name": item[1]} for item in MobileAppUserSettings.NOTIFICATION_TIMING_CHOICES + ] + + response = client.get(url, HTTP_AUTHORIZATION=auth_token) + assert response.status_code == status.HTTP_200_OK + + # Check the default values are correct + assert response.json() == choices + + @pytest.mark.django_db @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), + ([MobileAppUserSettings.FIFTEEN_MINUTES_IN_SECONDS], status.HTTP_200_OK), + ([MobileAppUserSettings.ONE_HOUR_IN_SECONDS], status.HTTP_200_OK), + ([MobileAppUserSettings.SIX_HOURS_IN_SECONDS], status.HTTP_200_OK), + ([MobileAppUserSettings.TWELVE_HOURS_IN_SECONDS], status.HTTP_200_OK), + ([MobileAppUserSettings.ONE_DAY_IN_SECONDS], status.HTTP_200_OK), + ([MobileAppUserSettings.ONE_DAY_IN_SECONDS, MobileAppUserSettings.ONE_HOUR_IN_SECONDS], status.HTTP_200_OK), + ([123], status.HTTP_400_BAD_REQUEST), + ([], status.HTTP_400_BAD_REQUEST), ], ) def test_user_settings_put( diff --git a/engine/apps/mobile_app/urls.py b/engine/apps/mobile_app/urls.py index 5d4898e9..ed36874c 100644 --- a/engine/apps/mobile_app/urls.py +++ b/engine/apps/mobile_app/urls.py @@ -1,5 +1,5 @@ from apps.mobile_app.fcm_relay import FCMRelayView -from apps.mobile_app.views import FCMDeviceAuthorizedViewSet, MobileAppAuthTokenAPIView, MobileAppUserSettingsAPIView +from apps.mobile_app.views import FCMDeviceAuthorizedViewSet, MobileAppAuthTokenAPIView, MobileAppUserSettingsViewSet from common.api_helpers.optional_slash_router import OptionalSlashRouter, optional_slash_path app_name = "mobile_app" @@ -10,7 +10,16 @@ router.register("fcm", FCMDeviceAuthorizedViewSet, basename="fcm") urlpatterns = [ *router.urls, optional_slash_path("auth_token", MobileAppAuthTokenAPIView.as_view(), name="auth_token"), - optional_slash_path("user_settings", MobileAppUserSettingsAPIView.as_view(), name="user_settings"), + optional_slash_path( + "user_settings/notification_timing_options", + MobileAppUserSettingsViewSet.as_view({"get": "notification_timing_options"}), + name="notification_timing_options", + ), + optional_slash_path( + "user_settings", + MobileAppUserSettingsViewSet.as_view({"get": "retrieve", "put": "update", "patch": "partial_update"}), + name="user_settings", + ), ] urlpatterns += [ diff --git a/engine/apps/mobile_app/views.py b/engine/apps/mobile_app/views.py index 035b68fa..8a4fe558 100644 --- a/engine/apps/mobile_app/views.py +++ b/engine/apps/mobile_app/views.py @@ -1,5 +1,5 @@ from fcm_django.api.rest_framework import FCMDeviceAuthorizedViewSet as BaseFCMDeviceAuthorizedViewSet -from rest_framework import generics, status +from rest_framework import mixins, status, viewsets from rest_framework.exceptions import NotFound from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response @@ -54,7 +54,11 @@ class MobileAppAuthTokenAPIView(APIView): return Response(status=status.HTTP_204_NO_CONTENT) -class MobileAppUserSettingsAPIView(generics.RetrieveUpdateAPIView): +class MobileAppUserSettingsViewSet( + mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, + viewsets.GenericViewSet, +): authentication_classes = (MobileAppAuthTokenAuthentication,) permission_classes = (IsAuthenticated,) serializer_class = MobileAppUserSettingsSerializer @@ -62,3 +66,9 @@ class MobileAppUserSettingsAPIView(generics.RetrieveUpdateAPIView): def get_object(self): mobile_app_settings, _ = MobileAppUserSettings.objects.get_or_create(user=self.request.user) return mobile_app_settings + + def notification_timing_options(self, request): + choices = [ + {"value": item[0], "display_name": item[1]} for item in MobileAppUserSettings.NOTIFICATION_TIMING_CHOICES + ] + return Response(choices)