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)
2023-07-20 20:12:54 +02:00 · 2023-07-20 20:12:54 +02:00 · 67393dd2ba
commit 67393dd2ba
parent ed6389ca8f
7 changed files with 74 additions and 24 deletions
--- 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))
--- 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',
+        ),
+    ]
--- a/engine/apps/mobile_app/models.py
+++ b/engine/apps/mobile_app/models.py
@ -174,3 +174,4 @@ class MobileAppUserSettings(models.Model):
    )

    locale = models.CharField(max_length=50, null=True)
+    time_zone = models.CharField(max_length=100, default="UTC")
--- 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 MobileAppUserSettingsSerializer(serializers.ModelSerializer):
            "info_notifications_enabled",
            "going_oncall_notification_timing",
            "locale",
+            "time_zone",
        )
--- a/engine/apps/mobile_app/tasks.py
+++ b/engine/apps/mobile_app/tasks.py
@ -25,7 +25,6 @@ from apps.user_management.models import User
 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 = {
--- 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
--- 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(