allowing storing/updating time_zone in mobile app user settings table (…

…#2601) # What this PR does closes #2450 ## 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)
grafana · Jul 20, 2023 · 67393dd · 67393dd
1 parent ed6389c
commit 67393dd
Show file tree

Hide file tree

Showing 7 changed files with 74 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Added
+
+- Allow persisting mobile app's timezone, to allow for more accurate datetime related notifications by @joeyorlando
+  ([#2601](https://github.com/grafana/oncall/pull/2601))
+
 ### Changed
 
 - Update direct paging docs by @vadimkerr ([#2600](https://github.com/grafana/oncall/pull/2600))

diff --git a/engine/apps/mobile_app/migrations/0010_mobileappusersettings_time_zone.py b/engine/apps/mobile_app/migrations/0010_mobileappusersettings_time_zone.py
@@ -0,0 +1,26 @@
+# Generated by Django 3.2.20 on 2023-07-20 14:53
+
+from django.db import migrations, models
+from django_add_default_value import AddDefaultValue
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('mobile_app', '0009_fcmdevice'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='mobileappusersettings',
+            name='time_zone',
+            field=models.CharField(default='UTC', max_length=100),
+        ),
+        # migrations.AddField enforces the default value on the app level, which leads to the issues during release
+        # adding same default value on the database level
+        AddDefaultValue(
+            model_name='mobileappusersettings',
+            name='time_zone',
+            value='UTC',
+        ),
+    ]
diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py
@@ -174,3 +174,4 @@ class VolumeType(models.TextChoices):
     )
 
     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
@@ -1,9 +1,12 @@
 from rest_framework import serializers
 
 from apps.mobile_app.models import MobileAppUserSettings
+from common.timezones import TimeZoneField
 
 
 class MobileAppUserSettingsSerializer(serializers.ModelSerializer):
+    time_zone = TimeZoneField(required=False, allow_null=False)
+
     class Meta:
         model = MobileAppUserSettings
         fields = (
@@ -23,4 +26,5 @@ class Meta:
             "info_notifications_enabled",
             "going_oncall_notification_timing",
             "locale",
+            "time_zone",
         )
diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py
@@ -25,7 +25,6 @@
 from common.api_helpers.utils import create_engine_url
 from common.custom_celery_tasks import shared_dedicated_queue_retry_task
 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 FCMDevice, MobileAppUserSettings
@@ -235,7 +234,6 @@ def _get_youre_going_oncall_notification_subtitle(
     schedule: OnCallSchedule,
     schedule_event: ScheduleEvent,
     mobile_app_user_settings: "MobileAppUserSettings",
-    user_timezone: typing.Optional[str],
 ) -> str:
     shift_start = schedule_event["start"]
     shift_end = schedule_event["end"]
@@ -244,16 +242,11 @@ def _get_youre_going_oncall_notification_subtitle(
 
     def _format_datetime(dt: datetime.datetime) -> str:
         """
-        1. Convert the shift datetime to the user's configured timezone, if set, otherwise fallback to UTC
+        1. Convert the shift datetime to the user's mobile device's timezone
         2. Display the timezone aware datetime as a formatted string that is based on the user's configured mobile
         app locale, otherwise fallback to "en"
         """
-        if user_timezone is None or not is_valid_timezone(user_timezone):
-            user_tz = "UTC"
-        else:
-            user_tz = user_timezone
-
-        localized_dt = dt.astimezone(pytz.timezone(user_tz))
+        localized_dt = dt.astimezone(pytz.timezone(mobile_app_user_settings.time_zone))
         return dt_formatter_func(localized_dt, mobile_app_user_settings.locale)
 
     formatted_shift = f"{_format_datetime(shift_start)} - {_format_datetime(shift_end)}"
@@ -277,7 +270,7 @@ def _get_youre_going_oncall_fcm_message(
 
     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, user.timezone
+        schedule, schedule_event, mobile_app_user_settings
     )
 
     data: FCMMessageData = {

diff --git a/engine/apps/mobile_app/tests/test_user_settings.py b/engine/apps/mobile_app/tests/test_user_settings.py
@@ -32,6 +32,7 @@ def test_user_settings_get(make_organization_and_user_with_mobile_app_auth_token
         "info_notifications_enabled": False,
         "going_oncall_notification_timing": 43200,
         "locale": None,
+        "time_zone": "UTC",
     }
 
 
@@ -69,6 +70,7 @@ def test_user_settings_put(
         "info_notifications_enabled": True,
         "going_oncall_notification_timing": going_oncall_notification_timing,
         "locale": "ca_FR",
+        "time_zone": "Europe/Paris",
     }
 
     response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=auth_token)
@@ -103,6 +105,7 @@ def test_user_settings_patch(make_organization_and_user_with_mobile_app_auth_tok
         "important_notification_volume_override": False,
         "important_notification_override_dnd": False,
         "info_notifications_enabled": True,
+        "time_zone": "Europe/Luxembourg",
     }
 
     response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=auth_token)
@@ -116,3 +119,24 @@ def test_user_settings_patch(make_organization_and_user_with_mobile_app_auth_tok
     assert response.status_code == status.HTTP_200_OK
     # all original settings should stay the same, only data set in PATCH call should get updated
     assert response.json() == {**original_settings, **patch_data}
+
+
+@pytest.mark.django_db
+def test_user_settings_time_zone_must_be_valid(make_organization_and_user_with_mobile_app_auth_token):
+    _, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
+
+    valid_timezone = {"time_zone": "Europe/Luxembourg"}
+    invalid_timezone = {"time_zone": "asdflkjasdlkj"}
+    null_timezone = {"time_zone": None}
+
+    client = APIClient()
+    url = reverse("mobile_app:user_settings")
+
+    response = client.put(url, data=valid_timezone, format="json", HTTP_AUTHORIZATION=auth_token)
+    assert response.status_code == status.HTTP_200_OK
+
+    response = client.put(url, data=invalid_timezone, format="json", HTTP_AUTHORIZATION=auth_token)
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+
+    response = client.put(url, data=null_timezone, format="json", HTTP_AUTHORIZATION=auth_token)
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
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
@@ -95,11 +95,9 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m
     # same day shift
     ##################
     same_day_shift_title = tasks._get_youre_going_oncall_notification_title(seconds_until_going_oncall)
-    same_day_shift_subtitle = tasks._get_youre_going_oncall_notification_subtitle(
-        schedule, same_day_shift, maus, user.timezone
-    )
+    same_day_shift_subtitle = tasks._get_youre_going_oncall_notification_subtitle(schedule, same_day_shift, maus)
     same_day_shift_no_locale_subtitle = tasks._get_youre_going_oncall_notification_subtitle(
-        schedule, same_day_shift, maus_no_locale, user2.timezone
+        schedule, same_day_shift, maus_no_locale
     )
 
     assert same_day_shift_title == f"Your on-call shift starts in {humanized_time_until_going_oncall}"
@@ -111,10 +109,10 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m
     ##################
     multiple_day_shift_title = tasks._get_youre_going_oncall_notification_title(seconds_until_going_oncall)
     multiple_day_shift_subtitle = tasks._get_youre_going_oncall_notification_subtitle(
-        schedule, multiple_day_shift, maus, user.timezone
+        schedule, multiple_day_shift, maus
     )
     multiple_day_shift_no_locale_subtitle = tasks._get_youre_going_oncall_notification_subtitle(
-        schedule, multiple_day_shift, maus_no_locale, user2.timezone
+        schedule, multiple_day_shift, maus_no_locale
     )
 
     assert multiple_day_shift_title == f"Your on-call shift starts in {humanized_time_until_going_oncall}"
@@ -128,9 +126,8 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m
 @pytest.mark.parametrize(
     "user_timezone,expected_shift_times",
     [
-        (None, "9:00 AM - 5:00 PM"),
+        ("UTC", "9:00 AM - 5:00 PM"),
         ("Europe/Amsterdam", "11:00 AM - 7:00 PM"),
-        ("asdfasdfasdf", "9:00 AM - 5:00 PM"),
     ],
 )
 @pytest.mark.django_db
@@ -140,9 +137,9 @@ def test_get_youre_going_oncall_notification_subtitle(
     schedule_name = "asdfasdfasdfasdf"
 
     organization = make_organization()
-    user = make_user_for_organization(organization, _timezone=user_timezone)
+    user = make_user_for_organization(organization)
     user_pk = user.public_primary_key
-    maus = MobileAppUserSettings.objects.create(user=user)
+    maus = MobileAppUserSettings.objects.create(user=user, time_zone=user_timezone)
 
     schedule = make_schedule(organization, name=schedule_name, schedule_class=OnCallScheduleWeb)
 
@@ -161,7 +158,7 @@ def test_get_youre_going_oncall_notification_subtitle(
     )
 
     assert (
-        tasks._get_youre_going_oncall_notification_subtitle(schedule, shift, maus, user.timezone)
+        tasks._get_youre_going_oncall_notification_subtitle(schedule, shift, maus)
         == f"{expected_shift_times}\nSchedule {schedule_name}"
     )
 
@@ -198,7 +195,7 @@ def test_get_youre_going_oncall_fcm_message(
 
     organization = make_organization()
     user_tz = "Europe/Amsterdam"
-    user = make_user_for_organization(organization, _timezone=user_tz)
+    user = make_user_for_organization(organization)
     user_pk = user.public_primary_key
     schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb)
     notification_thread_id = f"{schedule.public_primary_key}:{user_pk}:going-oncall"
@@ -215,7 +212,7 @@ def test_get_youre_going_oncall_fcm_message(
     )
 
     device = FCMDevice.objects.create(user=user)
-    maus = MobileAppUserSettings.objects.create(user=user)
+    maus = MobileAppUserSettings.objects.create(user=user, time_zone=user_tz)
 
     data = {
         "title": mock_notification_title,
@@ -248,7 +245,7 @@ def test_get_youre_going_oncall_fcm_message(
     )
     mock_apns_payload.assert_called_once_with(aps=mock_aps.return_value)
 
-    mock_get_youre_going_oncall_notification_subtitle.assert_called_once_with(schedule, schedule_event, maus, user_tz)
+    mock_get_youre_going_oncall_notification_subtitle.assert_called_once_with(schedule, schedule_event, maus)
     mock_get_youre_going_oncall_notification_title.assert_called_once_with(seconds_until_going_oncall)
 
     mock_construct_fcm_message.assert_called_once_with(
Original file line number	Diff line number	Diff line change
Expand Up		@@ -174,3 +174,4 @@ class VolumeType(models.TextChoices):
		)

		locale = models.CharField(max_length=50, null=True)
		time_zone = models.CharField(max_length=100, default="UTC")