From 3888ff73944d040f5d37f67847937b12a47d689c Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 18:31:11 +0100 Subject: [PATCH 01/22] WIP --- engine/apps/mobile_app/tasks.py | 180 +++++++++++++++++- .../test_your_going_oncall_notification.py | 68 ++----- .../apps/schedules/models/on_call_schedule.py | 2 +- engine/settings/base.py | 4 + 4 files changed, 196 insertions(+), 58 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 41b36a3108..595e0bbba8 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -11,6 +11,7 @@ from celery.utils.log import get_task_logger from django.conf import settings from django.core.cache import cache +from django.db.models import DateTimeField, ExpressionWrapper, F, Max from django.utils import timezone from firebase_admin.exceptions import FirebaseError from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message @@ -20,6 +21,7 @@ from apps.alerts.models import AlertGroup from apps.base.utils import live_settings from apps.mobile_app.alert_rendering import get_push_notification_subtitle +from apps.schedules.models import ShiftSwapRequest from apps.schedules.models.on_call_schedule import OnCallSchedule, ScheduleEvent from apps.user_management.models import User from common.api_helpers.utils import create_engine_url @@ -230,13 +232,12 @@ def _get_youre_going_oncall_notification_title(seconds_until_going_oncall: int) return f"Your on-call shift starts in {humanize.naturaldelta(seconds_until_going_oncall)}" -def _get_youre_going_oncall_notification_subtitle( +def _get_shift_subtitle( schedule: OnCallSchedule, - schedule_event: ScheduleEvent, + shift_start: datetime.datetime, + shift_end: datetime.datetime, mobile_app_user_settings: "MobileAppUserSettings", ) -> str: - shift_start = schedule_event["start"] - shift_end = schedule_event["end"] shift_starts_and_ends_on_same_day = shift_start.date() == shift_end.date() dt_formatter_func = format_localized_time if shift_starts_and_ends_on_same_day else format_localized_datetime @@ -269,8 +270,8 @@ def _get_youre_going_oncall_fcm_message( mobile_app_user_settings, _ = MobileAppUserSettings.objects.get_or_create(user=user) 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 + notification_subtitle = _get_shift_subtitle( + schedule, schedule_event["start"], schedule_event["end"], mobile_app_user_settings ) data: FCMMessageData = { @@ -499,3 +500,170 @@ def conditionally_send_going_oncall_push_notifications_for_schedule(schedule_pk) def conditionally_send_going_oncall_push_notifications_for_all_schedules() -> None: for schedule in OnCallSchedule.objects.all(): conditionally_send_going_oncall_push_notifications_for_schedule.apply_async((schedule.pk,)) + + +EARLIEST_NOTIFICATION_OFFSET = datetime.timedelta(weeks=4) +WINDOW = datetime.timedelta(days=1) + + +@shared_dedicated_queue_retry_task() +def notify_shift_swap_requests() -> None: + if not settings.FEATURE_SHIFT_SWAPS_ENABLED: + return + + for shift_swap_request in _get_shift_swap_requests_to_notify(timezone.now()): + notify_shift_swap_request.delay(shift_swap_request.pk) + + +def _get_shift_swap_requests_to_notify(now: datetime.datetime): + # This is the same as notification_window_start = max(created_at, swap_start - EARLIEST_NOTIFICATION_OFFSET) + notification_window_start = Max( + F("created_at"), ExpressionWrapper(F("swap_start") - EARLIEST_NOTIFICATION_OFFSET, output_field=DateTimeField()) + ) + + # This is the same as notification_window_end = swap_start + WINDOW + notification_window_end = ExpressionWrapper(F("notification_window_start") + WINDOW, output_field=DateTimeField()) + + # For every shift swap request, we assign a window of time in which we can notify users about it. + # Here we select all the shift swap requests for which now is within its notification window. + return ShiftSwapRequest.objects.annotate( + notification_window_start=notification_window_start, + notification_window_end=notification_window_end, + ).filter( + swap_start__lt=now, + benefactor__isnull=True, + notification_window_start__lte=now, + notification_window_end__gte=now, + ) + + +@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) +def notify_shift_swap_request(shift_swap_request_pk: int) -> None: + try: + shift_swap_request = ShiftSwapRequest.objects.get(pk=shift_swap_request_pk) + except ShiftSwapRequest.DoesNotExist: + logger.info(f"ShiftSwapRequest {shift_swap_request_pk} does not exist") + return + + now = timezone.now() + users_to_notify = shift_swap_request.schedule.related_users().exclude(pk=shift_swap_request.beneficiary_id) + for user in users_to_notify: + if _should_notify_user(shift_swap_request, user, now): + notify_user_about_shift_swap_request.delay(shift_swap_request.pk, user.pk) + _mark_notified(shift_swap_request, user) + + +@shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) +def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: int) -> None: + # avoid circular import + from apps.mobile_app.models import FCMDevice, MobileAppUserSettings + + try: + shift_swap_request = ShiftSwapRequest.objects.get(pk=shift_swap_request_pk) + except ShiftSwapRequest.DoesNotExist: + logger.info(f"ShiftSwapRequest {shift_swap_request_pk} does not exist") + return + + try: + user = User.objects.get(pk=user_pk) + except User.DoesNotExist: + logger.info(f"User {user_pk} does not exist") + return + + device_to_notify = FCMDevice.get_active_device_for_user(user) + if not device_to_notify: + logger.info(f"FCMDevice does not exist for user {user_pk}") + return + + try: + mobile_app_user_settings = MobileAppUserSettings.objects.get(user=user) + except MobileAppUserSettings.DoesNotExist: + logger.info(f"MobileAppUserSettings does not exist for user {user_pk}") + return + + message = _get_shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings) + _send_push_notification(device_to_notify, message) + + +def _should_notify_user(shift_swap_request: ShiftSwapRequest, user: User, now: datetime.datetime) -> bool: + return _is_in_working_hours(user, now) and not _already_notified(shift_swap_request, user) + + +def _is_in_working_hours(user: User, now: datetime.datetime) -> bool: + # avoid circular import + from apps.mobile_app.models import MobileAppUserSettings + + today = now.date() + day_name = today.strftime("%A").lower() + + # TODO: refactor working hours in User model + day_start_time_str = user.working_hours[day_name][0]["start"] + day_start_time = datetime.time.fromisoformat(day_start_time_str) + + day_end_time_str = user.working_hours[day_name][0]["end"] + day_end_time = datetime.time.fromisoformat(day_end_time_str) + + try: + user_settings = MobileAppUserSettings.objects.get(user=user) + except MobileAppUserSettings.DoesNotExist: + return False + + day_start = datetime.datetime.combine(today, day_start_time, tzinfo=pytz.timezone(user_settings.time_zone)) + day_end = datetime.datetime.combine(today, day_end_time, tzinfo=pytz.timezone(user_settings.time_zone)) + + return day_start <= now <= day_end + + +def _mark_notified(shift_swap_request: ShiftSwapRequest, user: User) -> None: + key = _cache_key(shift_swap_request, user) + cache.set(key, True, timeout=WINDOW.total_seconds()) + + +def _already_notified(shift_swap_request: ShiftSwapRequest, user: User) -> bool: + key = _cache_key(shift_swap_request, user) + return cache.get(key) is True + + +def _cache_key(shift_swap_request: ShiftSwapRequest, user: User) -> str: + return f"ssr_push:{shift_swap_request.pk}:{user.pk}" + + +def _get_shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings): + from apps.mobile_app.models import MobileAppUserSettings + + thread_id = f"{shift_swap_request.public_primary_key}:{user.public_primary_key}:ssr" + notification_title = "You have a new shift swap request" + notification_subtitle = _get_shift_subtitle( + shift_swap_request.schedule, + shift_swap_request.swap_start, + shift_swap_request.swap_end, + mobile_app_user_settings, + ) + + data: FCMMessageData = { + "title": notification_title, + "subtitle": notification_subtitle, + "info_notification_sound_name": ( + mobile_app_user_settings.info_notification_sound_name + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION + ), + "info_notification_volume_type": mobile_app_user_settings.info_notification_volume_type, + "info_notification_volume": str(mobile_app_user_settings.info_notification_volume), + "info_notification_volume_override": json.dumps(mobile_app_user_settings.info_notification_volume_override), + } + + apns_payload = APNSPayload( + aps=Aps( + thread_id=thread_id, + alert=ApsAlert(title=notification_title, subtitle=notification_subtitle), + sound=CriticalSound( + critical=False, + name=mobile_app_user_settings.info_notification_sound_name + + MobileAppUserSettings.IOS_SOUND_NAME_EXTENSION, + ), + custom_data={ + "interruption-level": "time-sensitive", + }, + ), + ) + + return _construct_fcm_message(MessageType.INFO, device_to_notify, thread_id, data, apns_payload) 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 a218fd6ffa..52d45c0d7b 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 @@ -54,8 +54,6 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m organization, user = make_organization_and_user() user2 = make_user(organization=organization) schedule = make_schedule(organization, name=schedule_name, schedule_class=OnCallScheduleWeb) - shift_pk = "mncvmnvc" - user_pk = user.public_primary_key user_locale = "fr_CA" seconds_until_going_oncall = 600 humanized_time_until_going_oncall = "10 minutes" @@ -66,28 +64,6 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m multiple_day_shift_start = timezone.datetime(2023, 7, 8, 9, 0, 0) multiple_day_shift_end = timezone.datetime(2023, 7, 12, 17, 0, 0) - same_day_shift = _create_schedule_event( - same_day_shift_start, - same_day_shift_end, - shift_pk, - [ - { - "pk": user_pk, - }, - ], - ) - - multiple_day_shift = _create_schedule_event( - multiple_day_shift_start, - multiple_day_shift_end, - shift_pk, - [ - { - "pk": user_pk, - }, - ], - ) - maus = MobileAppUserSettings.objects.create(user=user, locale=user_locale) maus_no_locale = MobileAppUserSettings.objects.create(user=user2) @@ -95,9 +71,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) - same_day_shift_no_locale_subtitle = tasks._get_youre_going_oncall_notification_subtitle( - schedule, same_day_shift, maus_no_locale + same_day_shift_subtitle = tasks._get_shift_subtitle(schedule, same_day_shift_start, same_day_shift_end, maus) + same_day_shift_no_locale_subtitle = tasks._get_shift_subtitle( + schedule, same_day_shift_start, same_day_shift_end, maus_no_locale ) assert same_day_shift_title == f"Your on-call shift starts in {humanized_time_until_going_oncall}" @@ -108,11 +84,11 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m # multiple day shift ################## 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 + multiple_day_shift_subtitle = tasks._get_shift_subtitle( + schedule, multiple_day_shift_start, multiple_day_shift_end, maus ) - multiple_day_shift_no_locale_subtitle = tasks._get_youre_going_oncall_notification_subtitle( - schedule, multiple_day_shift, maus_no_locale + multiple_day_shift_no_locale_subtitle = tasks._get_shift_subtitle( + schedule, multiple_day_shift_start, multiple_day_shift_end, maus_no_locale ) assert multiple_day_shift_title == f"Your on-call shift starts in {humanized_time_until_going_oncall}" @@ -131,14 +107,13 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m ], ) @pytest.mark.django_db -def test_get_youre_going_oncall_notification_subtitle( +def test_get_shift_subtitle( make_organization, make_user_for_organization, make_schedule, user_timezone, expected_shift_times ): schedule_name = "asdfasdfasdfasdf" organization = make_organization() user = make_user_for_organization(organization) - user_pk = user.public_primary_key maus = MobileAppUserSettings.objects.create(user=user, time_zone=user_timezone) schedule = make_schedule(organization, name=schedule_name, schedule_class=OnCallScheduleWeb) @@ -146,24 +121,13 @@ def test_get_youre_going_oncall_notification_subtitle( shift_start = timezone.datetime(2023, 7, 8, 9, 0, 0) shift_end = timezone.datetime(2023, 7, 8, 17, 0, 0) - shift = _create_schedule_event( - shift_start, - shift_end, - "asdfasdfasdf", - [ - { - "pk": user_pk, - }, - ], - ) - assert ( - tasks._get_youre_going_oncall_notification_subtitle(schedule, shift, maus) + tasks._get_shift_subtitle(schedule, shift_start, shift_end, maus) == f"{expected_shift_times}\nSchedule {schedule_name}" ) -@mock.patch("apps.mobile_app.tasks._get_youre_going_oncall_notification_subtitle") +@mock.patch("apps.mobile_app.tasks._get_shift_subtitle") @mock.patch("apps.mobile_app.tasks._get_youre_going_oncall_notification_title") @mock.patch("apps.mobile_app.tasks._construct_fcm_message") @mock.patch("apps.mobile_app.tasks.APNSPayload") @@ -178,7 +142,7 @@ def test_get_youre_going_oncall_fcm_message( mock_apns_payload, mock_construct_fcm_message, mock_get_youre_going_oncall_notification_title, - mock_get_youre_going_oncall_notification_subtitle, + mock_get_shift_subtitle, make_organization, make_user_for_organization, make_schedule, @@ -191,7 +155,7 @@ def test_get_youre_going_oncall_fcm_message( mock_construct_fcm_message.return_value = mock_fcm_message mock_get_youre_going_oncall_notification_title.return_value = mock_notification_title - mock_get_youre_going_oncall_notification_subtitle.return_value = mock_notification_subtitle + mock_get_shift_subtitle.return_value = mock_notification_subtitle organization = make_organization() user_tz = "Europe/Amsterdam" @@ -200,9 +164,11 @@ def test_get_youre_going_oncall_fcm_message( schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) notification_thread_id = f"{schedule.public_primary_key}:{user_pk}:going-oncall" + shift_start = shift_end = timezone.now() + schedule_event = _create_schedule_event( - timezone.now(), - timezone.now(), + shift_start, + shift_end, shift_pk, [ { @@ -245,7 +211,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) + mock_get_shift_subtitle.assert_called_once_with(schedule, shift_start, shift_end, maus) mock_get_youre_going_oncall_notification_title.assert_called_once_with(seconds_until_going_oncall) mock_construct_fcm_message.assert_called_once_with( diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index 7ee1085513..a240ab691f 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -56,7 +56,7 @@ RE_ICAL_SEARCH_USERNAME = r"SUMMARY:(\[L[0-9]+\] )?{}" -RE_ICAL_FETCH_USERNAME = re.compile(r"SUMMARY:(?:\[L[0-9]+\] )?(\w+)") +RE_ICAL_FETCH_USERNAME = re.compile(r"SUMMARY:(?:\[L[0-9]+\] )?([^\s]+)") # Utility classes for schedule quality report diff --git a/engine/settings/base.py b/engine/settings/base.py index 89f61d437e..8b881d2c25 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -480,6 +480,10 @@ class BrokerTypes: "task": "apps.mobile_app.tasks.conditionally_send_going_oncall_push_notifications_for_all_schedules", "schedule": 10 * 60, }, + "notify_shift_swap_requests": { + "task": "apps.mobile_app.tasks.notify_shift_swap_requests", + "schedule": 10 * 60, + }, "save_organizations_ids_in_cache": { "task": "apps.metrics_exporter.tasks.save_organizations_ids_in_cache", "schedule": 60 * 30, From c9db7fece0d47bf2f7f9e107c70f544c95de5ce2 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 18:42:48 +0100 Subject: [PATCH 02/22] add test on usernames with dots --- .../schedules/tests/test_on_call_schedule.py | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index beb707d8ff..68589ace37 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -1158,6 +1158,49 @@ def test_schedule_related_users(make_organization, make_user_for_organization, m assert set(users) == set([user_a, user_d, user_e]) +@pytest.mark.django_db +def test_schedule_related_users_usernames( + make_organization, make_user_for_organization, make_on_call_shift, make_schedule +): + """ + Check different usernames, including those with special characters and uppercase letters + """ + organization = make_organization() + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_web_schedule", + ) + + now = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + start_date = now - timezone.timedelta(days=7) + + # Check different usernames, including those with special characters and uppercase letters + usernames = ["test", "test.test", "test.test@test.test", "TEST.TEST@TEST.TEST"] + users = [make_user_for_organization(organization, username=u) for u in usernames] + # clear users pks <-> organization cache (persisting between tests) + memoized_users_in_ical.cache_clear() + + for user in users: + data = { + "start": start_date, + "rotation_start": start_date, + "duration": timezone.timedelta(hours=1), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user]]) + + schedule.refresh_ical_file() + schedule.refresh_from_db() + + assert set(schedule.related_users()) == set(users) + + @pytest.mark.django_db(transaction=True) def test_filter_events_none_cache_unchanged( make_organization, make_user_for_organization, make_schedule, make_on_call_shift From 2fe49783e971f6de3d3824a7d5edd060699559dc Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 18:43:01 +0100 Subject: [PATCH 03/22] test --- .../apps/mobile_app/tests/test_your_going_oncall_notification.py | 1 - 1 file changed, 1 deletion(-) 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 52d45c0d7b..75cfda7c0b 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 @@ -165,7 +165,6 @@ def test_get_youre_going_oncall_fcm_message( notification_thread_id = f"{schedule.public_primary_key}:{user_pk}:going-oncall" shift_start = shift_end = timezone.now() - schedule_event = _create_schedule_event( shift_start, shift_end, From 2dff0cad0cd0a4dca860fd42b5aa5bf71bbe859c Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 19:49:03 +0100 Subject: [PATCH 04/22] basic tests --- engine/apps/mobile_app/models.py | 2 +- engine/apps/mobile_app/tasks.py | 18 +++-- .../tests/test_shift_swap_request.py | 77 +++++++++++++++++++ .../0016_alter_shiftswaprequest_created_at.py | 19 +++++ .../schedules/models/shift_swap_request.py | 6 +- .../tests/test_shift_swap_request.py | 10 +++ 6 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 engine/apps/mobile_app/tests/test_shift_swap_request.py create mode 100644 engine/apps/schedules/migrations/0016_alter_shiftswaprequest_created_at.py diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index 89c6703e07..a4a79abd23 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -142,7 +142,7 @@ class VolumeType(models.TextChoices): # Push notification settings for info notifications # this is used for non escalation related push notifications such as the - # "You're going OnCall soon" push notification + # "You're going OnCall soon" and "You have a new shift swap request" push notifications info_notifications_enabled = models.BooleanField(default=False) info_notification_sound_name = models.CharField(max_length=100, default="default_sound", null=True) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 595e0bbba8..54dbcdfaeb 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -11,7 +11,7 @@ from celery.utils.log import get_task_logger from django.conf import settings from django.core.cache import cache -from django.db.models import DateTimeField, ExpressionWrapper, F, Max +from django.db.models import DateTimeField, ExpressionWrapper, F, Max, QuerySet from django.utils import timezone from firebase_admin.exceptions import FirebaseError from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message @@ -515,13 +515,13 @@ def notify_shift_swap_requests() -> None: notify_shift_swap_request.delay(shift_swap_request.pk) -def _get_shift_swap_requests_to_notify(now: datetime.datetime): +def _get_shift_swap_requests_to_notify(now: datetime.datetime) -> QuerySet[ShiftSwapRequest]: # This is the same as notification_window_start = max(created_at, swap_start - EARLIEST_NOTIFICATION_OFFSET) notification_window_start = Max( F("created_at"), ExpressionWrapper(F("swap_start") - EARLIEST_NOTIFICATION_OFFSET, output_field=DateTimeField()) ) - # This is the same as notification_window_end = swap_start + WINDOW + # This is the same as notification_window_end = notification_window_start + WINDOW notification_window_end = ExpressionWrapper(F("notification_window_start") + WINDOW, output_field=DateTimeField()) # For every shift swap request, we assign a window of time in which we can notify users about it. @@ -530,8 +530,8 @@ def _get_shift_swap_requests_to_notify(now: datetime.datetime): notification_window_start=notification_window_start, notification_window_end=notification_window_end, ).filter( - swap_start__lt=now, benefactor__isnull=True, + swap_start__gt=now, notification_window_start__lte=now, notification_window_end__gte=now, ) @@ -546,8 +546,7 @@ def notify_shift_swap_request(shift_swap_request_pk: int) -> None: return now = timezone.now() - users_to_notify = shift_swap_request.schedule.related_users().exclude(pk=shift_swap_request.beneficiary_id) - for user in users_to_notify: + for user in shift_swap_request.possible_benefactors: if _should_notify_user(shift_swap_request, user, now): notify_user_about_shift_swap_request.delay(shift_swap_request.pk, user.pk) _mark_notified(shift_swap_request, user) @@ -586,7 +585,12 @@ def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: in def _should_notify_user(shift_swap_request: ShiftSwapRequest, user: User, now: datetime.datetime) -> bool: - return _is_in_working_hours(user, now) and not _already_notified(shift_swap_request, user) + mobile_app_user_settings = MobileAppUserSettings.objects.get(user=user) + return ( + mobile_app_user_settings.info_notifications_enabled + and _is_in_working_hours(user, now) + and not _already_notified(shift_swap_request, user) + ) def _is_in_working_hours(user: User, now: datetime.datetime) -> bool: diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py new file mode 100644 index 0000000000..31048d7ea7 --- /dev/null +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -0,0 +1,77 @@ +import pytest +from django.utils import timezone + +from apps.mobile_app.tasks import EARLIEST_NOTIFICATION_OFFSET, WINDOW, _get_shift_swap_requests_to_notify +from apps.schedules.models import OnCallScheduleWeb + +MICROSECOND = timezone.timedelta(microseconds=1) + + +@pytest.mark.django_db +def test_get_shift_swap_requests_to_notify_starts_soon( + make_organization, make_user, make_schedule, make_shift_swap_request +): + organization = make_organization() + user = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=7) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + assert list(_get_shift_swap_requests_to_notify(now - MICROSECOND)) == [] + assert list(_get_shift_swap_requests_to_notify(now)) == [shift_swap_request] + assert list(_get_shift_swap_requests_to_notify(now + WINDOW)) == [shift_swap_request] + assert list(_get_shift_swap_requests_to_notify(now + WINDOW + MICROSECOND)) == [] + + +@pytest.mark.django_db +def test_get_shift_swap_requests_to_notify_starts_very_soon( + make_organization, make_user, make_schedule, make_shift_swap_request +): + organization = make_organization() + user = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(minutes=1) + swap_end = swap_start + timezone.timedelta(minutes=10) + + shift_swap_request = make_shift_swap_request( + schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + assert list(_get_shift_swap_requests_to_notify(now - MICROSECOND)) == [] + assert list(_get_shift_swap_requests_to_notify(now)) == [shift_swap_request] + assert list(_get_shift_swap_requests_to_notify(now + timezone.timedelta(minutes=1))) == [] + + +@pytest.mark.django_db +def test_get_shift_swap_requests_to_notify_starts_not_soon( + make_organization, make_user, make_schedule, make_shift_swap_request +): + organization = make_organization() + user = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + assert list(_get_shift_swap_requests_to_notify(now)) == [] + assert list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET - MICROSECOND)) == [] + assert list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET)) == [shift_swap_request] + assert list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET + WINDOW)) == [ + shift_swap_request + ] + assert ( + list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET + WINDOW + MICROSECOND)) == [] + ) diff --git a/engine/apps/schedules/migrations/0016_alter_shiftswaprequest_created_at.py b/engine/apps/schedules/migrations/0016_alter_shiftswaprequest_created_at.py new file mode 100644 index 0000000000..e8565d866b --- /dev/null +++ b/engine/apps/schedules/migrations/0016_alter_shiftswaprequest_created_at.py @@ -0,0 +1,19 @@ +# Generated by Django 3.2.20 on 2023-08-01 18:16 + +from django.db import migrations, models +import django.utils.timezone + + +class Migration(migrations.Migration): + + dependencies = [ + ('schedules', '0015_shiftswaprequest_slack_message'), + ] + + operations = [ + migrations.AlterField( + model_name='shiftswaprequest', + name='created_at', + field=models.DateTimeField(default=django.utils.timezone.now), + ), + ] diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index a9cc899bbe..c7afa6f0c9 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -59,7 +59,7 @@ class ShiftSwapRequest(models.Model): default=generate_public_primary_key_for_shift_swap_request, ) - created_at = models.DateTimeField(auto_now_add=True) + created_at = models.DateTimeField(default=timezone.now) updated_at = models.DateTimeField(auto_now=True) deleted_at = models.DateTimeField(null=True) @@ -149,6 +149,10 @@ def slack_channel_id(self) -> str | None: def organization(self) -> "Organization": return self.schedule.organization + @property + def possible_benefactors(self): + return self.schedule.related_users().exclude(pk=self.beneficiary_id) + @property def web_link(self) -> str: # TODO: finish this once we know the proper URL we'll need diff --git a/engine/apps/schedules/tests/test_shift_swap_request.py b/engine/apps/schedules/tests/test_shift_swap_request.py index 17d5122527..ba8fc95778 100644 --- a/engine/apps/schedules/tests/test_shift_swap_request.py +++ b/engine/apps/schedules/tests/test_shift_swap_request.py @@ -6,6 +6,7 @@ from apps.schedules import exceptions from apps.schedules.models import CustomOnCallShift, ShiftSwapRequest +from apps.user_management.models import User @pytest.mark.django_db @@ -152,3 +153,12 @@ def test_related_shifts(shift_swap_request_setup, make_on_call_shift) -> None: ] returned_events = [(e["start"], e["end"], e["users"][0]["pk"], e["users"][0]["swap_request"]["pk"]) for e in events] assert returned_events == expected + + +@pytest.mark.django_db +def test_possible_benefactors(shift_swap_request_setup) -> None: + ssr, beneficiary, benefactor = shift_swap_request_setup() + + with patch.object(ssr.schedule, "related_users") as mock_related_users: + mock_related_users.return_value = User.objects.filter(pk__in=[beneficiary.pk, benefactor.pk]) + assert list(ssr.possible_benefactors) == [benefactor] From 6580a124d9133a8a9d630ad60d17937a6607d6af Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 21:00:23 +0100 Subject: [PATCH 05/22] more tests --- engine/apps/mobile_app/tasks.py | 12 +- .../tests/test_shift_swap_request.py | 211 +++++++++++++++++- 2 files changed, 220 insertions(+), 3 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 54dbcdfaeb..140408b454 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -580,12 +580,22 @@ def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: in logger.info(f"MobileAppUserSettings does not exist for user {user_pk}") return + if not mobile_app_user_settings.info_notifications_enabled: + logger.info(f"Info notifications are not enabled for user {user_pk}") + return + message = _get_shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings) _send_push_notification(device_to_notify, message) def _should_notify_user(shift_swap_request: ShiftSwapRequest, user: User, now: datetime.datetime) -> bool: - mobile_app_user_settings = MobileAppUserSettings.objects.get(user=user) + from apps.mobile_app.models import MobileAppUserSettings + + try: + mobile_app_user_settings = MobileAppUserSettings.objects.get(user=user) + except MobileAppUserSettings.DoesNotExist: + return False + return ( mobile_app_user_settings.info_notifications_enabled and _is_in_working_hours(user, now) diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 31048d7ea7..1e01171e8e 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -1,12 +1,33 @@ +from unittest.mock import PropertyMock, patch + import pytest +from django.core.cache import cache from django.utils import timezone +from firebase_admin.messaging import Message -from apps.mobile_app.tasks import EARLIEST_NOTIFICATION_OFFSET, WINDOW, _get_shift_swap_requests_to_notify -from apps.schedules.models import OnCallScheduleWeb +from apps.mobile_app.models import FCMDevice, MobileAppUserSettings +from apps.mobile_app.tasks import ( + EARLIEST_NOTIFICATION_OFFSET, + WINDOW, + MessageType, + _already_notified, + _get_shift_swap_requests_to_notify, + _mark_notified, + _should_notify_user, + notify_shift_swap_request, + notify_shift_swap_requests, + notify_user_about_shift_swap_request, +) +from apps.schedules.models import OnCallScheduleWeb, ShiftSwapRequest +from apps.user_management.models import User MICROSECOND = timezone.timedelta(microseconds=1) +def test_window_more_than_24_hours(): + assert WINDOW >= timezone.timedelta(hours=24) + + @pytest.mark.django_db def test_get_shift_swap_requests_to_notify_starts_soon( make_organization, make_user, make_schedule, make_shift_swap_request @@ -75,3 +96,189 @@ def test_get_shift_swap_requests_to_notify_starts_not_soon( assert ( list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET + WINDOW + MICROSECOND)) == [] ) + + +@pytest.mark.django_db +def test_notify_shift_swap_requests(make_organization, make_user, make_schedule, make_shift_swap_request, settings): + settings.FEATURE_SHIFT_SWAPS_ENABLED = True + + organization = make_organization() + user = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + with patch.object(notify_shift_swap_request, "delay") as mock_notify_shift_swap_request: + with patch( + "apps.mobile_app.tasks._get_shift_swap_requests_to_notify", + return_value=ShiftSwapRequest.objects.filter(pk=shift_swap_request.pk), + ) as mock_get_shift_swap_requests_to_notify: + notify_shift_swap_requests() + + mock_get_shift_swap_requests_to_notify.assert_called_once() + mock_notify_shift_swap_request.assert_called_once_with(shift_swap_request.pk) + + +@pytest.mark.django_db +def test_notify_shift_swap_requests_feature_flag_disabled( + make_organization, make_user, make_schedule, make_shift_swap_request, settings +): + settings.FEATURE_SHIFT_SWAPS_ENABLED = False + with patch("apps.mobile_app.tasks._get_shift_swap_requests_to_notify") as mock_get_shift_swap_requests_to_notify: + notify_shift_swap_requests() + + mock_get_shift_swap_requests_to_notify.assert_not_called() + + +@pytest.mark.django_db +def test_notify_shift_swap_request(make_organization, make_user, make_schedule, make_shift_swap_request, settings): + organization = make_organization() + user = make_user(organization=organization) + other_user = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + with patch.object(notify_user_about_shift_swap_request, "delay") as mock_notify_user_about_shift_swap_request: + with patch("apps.mobile_app.tasks._should_notify_user", return_value=True): + with patch.object( + ShiftSwapRequest, + "possible_benefactors", + new_callable=PropertyMock(return_value=User.objects.filter(pk=other_user.pk)), + ): + notify_shift_swap_request(shift_swap_request.pk) + + mock_notify_user_about_shift_swap_request.assert_called_once_with(shift_swap_request.pk, other_user.pk) + + +@pytest.mark.django_db +def test_notify_shift_swap_request_should_not_notify_user( + make_organization, make_user, make_schedule, make_shift_swap_request, settings +): + organization = make_organization() + user = make_user(organization=organization) + other_user = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + with patch.object(notify_user_about_shift_swap_request, "delay") as mock_notify_user_about_shift_swap_request: + with patch("apps.mobile_app.tasks._should_notify_user", return_value=False): + with patch.object( + ShiftSwapRequest, + "possible_benefactors", + new_callable=PropertyMock(return_value=User.objects.filter(pk=other_user.pk)), + ): + notify_shift_swap_request(shift_swap_request.pk) + + mock_notify_user_about_shift_swap_request.assert_not_called() + + +@pytest.mark.django_db +def test_notify_user_about_shift_swap_request( + make_organization, make_user, make_schedule, make_shift_swap_request, settings +): + organization = make_organization() + beneficiary = make_user(organization=organization) + benefactor = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb, name="Test") + + device_to_notify = FCMDevice.objects.create(user=benefactor, registration_id="test_device_id") + MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) + + now = timezone.datetime(2023, 8, 1, 19, 38, tzinfo=timezone.utc) + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + with patch("apps.mobile_app.tasks._send_push_notification") as mock_send_push_notification: + notify_user_about_shift_swap_request(shift_swap_request.pk, benefactor.pk) + + mock_send_push_notification.assert_called_once() + assert mock_send_push_notification.call_args.args[0] == device_to_notify + + message: Message = mock_send_push_notification.call_args.args[1] + assert message.data["type"] == MessageType.INFO + assert message.data["title"] == "You have a new shift swap request" + assert message.data["subtitle"] == "11/9/23, 7:38\u202fPM - 11/10/23, 7:38\u202fPM\nSchedule Test" + assert message.apns.payload.aps.sound.critical is False + + +@pytest.mark.django_db +def test_should_notify_user(make_organization, make_user, make_schedule, make_shift_swap_request): + organization = make_organization() + beneficiary = make_user(organization=organization) + benefactor = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + assert not MobileAppUserSettings.objects.exists() + assert _should_notify_user(shift_swap_request, benefactor, now) is False + + MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) + assert _should_notify_user(shift_swap_request, benefactor, now) is False + + with patch("apps.mobile_app.tasks._is_in_working_hours", return_value=True): + with patch("apps.mobile_app.tasks._already_notified", return_value=True): + assert _should_notify_user(shift_swap_request, benefactor, now) is False + + with patch("apps.mobile_app.tasks._is_in_working_hours", return_value=False): + with patch("apps.mobile_app.tasks._already_notified", return_value=False): + assert _should_notify_user(shift_swap_request, benefactor, now) is False + + with patch("apps.mobile_app.tasks._is_in_working_hours", return_value=True): + with patch("apps.mobile_app.tasks._already_notified", return_value=False): + assert _should_notify_user(shift_swap_request, benefactor, now) is True + + +@pytest.mark.django_db +def test_mark_notified(make_organization, make_user, make_schedule, make_shift_swap_request): + organization = make_organization() + beneficiary = make_user(organization=organization) + benefactor = make_user(organization=organization) + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + now = timezone.now() + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + cache.clear() + assert _already_notified(shift_swap_request, benefactor) is False + _mark_notified(shift_swap_request, benefactor) + assert _already_notified(shift_swap_request, benefactor) is True + + with patch.object(cache, "set") as mock_cache_set: + _mark_notified(shift_swap_request, benefactor) + assert mock_cache_set.call_args.kwargs["timeout"] == WINDOW.total_seconds() From 339836b1bcc70a4997abdacd4f74a7219e956ff8 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 21:09:10 +0100 Subject: [PATCH 06/22] move is_in_working_hours to User model --- engine/apps/mobile_app/tasks.py | 27 +------------------ .../tests/test_shift_swap_request.py | 6 ++--- engine/apps/user_management/models/user.py | 20 ++++++++++++++ 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 140408b454..670e917eac 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -598,36 +598,11 @@ def _should_notify_user(shift_swap_request: ShiftSwapRequest, user: User, now: d return ( mobile_app_user_settings.info_notifications_enabled - and _is_in_working_hours(user, now) + and user.is_in_working_hours(now, mobile_app_user_settings.time_zone) and not _already_notified(shift_swap_request, user) ) -def _is_in_working_hours(user: User, now: datetime.datetime) -> bool: - # avoid circular import - from apps.mobile_app.models import MobileAppUserSettings - - today = now.date() - day_name = today.strftime("%A").lower() - - # TODO: refactor working hours in User model - day_start_time_str = user.working_hours[day_name][0]["start"] - day_start_time = datetime.time.fromisoformat(day_start_time_str) - - day_end_time_str = user.working_hours[day_name][0]["end"] - day_end_time = datetime.time.fromisoformat(day_end_time_str) - - try: - user_settings = MobileAppUserSettings.objects.get(user=user) - except MobileAppUserSettings.DoesNotExist: - return False - - day_start = datetime.datetime.combine(today, day_start_time, tzinfo=pytz.timezone(user_settings.time_zone)) - day_end = datetime.datetime.combine(today, day_end_time, tzinfo=pytz.timezone(user_settings.time_zone)) - - return day_start <= now <= day_end - - def _mark_notified(shift_swap_request: ShiftSwapRequest, user: User) -> None: key = _cache_key(shift_swap_request, user) cache.set(key, True, timeout=WINDOW.total_seconds()) diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 1e01171e8e..40f44a5abd 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -246,15 +246,15 @@ def test_should_notify_user(make_organization, make_user, make_schedule, make_sh MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) assert _should_notify_user(shift_swap_request, benefactor, now) is False - with patch("apps.mobile_app.tasks._is_in_working_hours", return_value=True): + with patch.object(benefactor, "is_in_working_hours", return_value=True): with patch("apps.mobile_app.tasks._already_notified", return_value=True): assert _should_notify_user(shift_swap_request, benefactor, now) is False - with patch("apps.mobile_app.tasks._is_in_working_hours", return_value=False): + with patch.object(benefactor, "is_in_working_hours", return_value=False): with patch("apps.mobile_app.tasks._already_notified", return_value=False): assert _should_notify_user(shift_swap_request, benefactor, now) is False - with patch("apps.mobile_app.tasks._is_in_working_hours", return_value=True): + with patch.object(benefactor, "is_in_working_hours", return_value=True): with patch("apps.mobile_app.tasks._already_notified", return_value=False): assert _should_notify_user(shift_swap_request, benefactor, now) is True diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index e6579a9f11..6eaba3c9e1 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -1,8 +1,10 @@ +import datetime import json import logging import typing from urllib.parse import urljoin +import pytz from django.conf import settings from django.core.validators import MinLengthValidator from django.db import models @@ -283,6 +285,24 @@ def timezone(self) -> typing.Optional[str]: def timezone(self, value): self._timezone = value + def is_in_working_hours(self, dt: datetime.datetime, timezone: typing.Optional[str] = None) -> bool: + if not timezone: + timezone = self.timezone + + today = dt.date() + day_name = today.strftime("%A").lower() + + day_start_time_str = self.working_hours[day_name][0]["start"] + day_start_time = datetime.time.fromisoformat(day_start_time_str) + + day_end_time_str = self.working_hours[day_name][0]["end"] + day_end_time = datetime.time.fromisoformat(day_end_time_str) + + day_start = datetime.datetime.combine(today, day_start_time, tzinfo=pytz.timezone(timezone)) + day_end = datetime.datetime.combine(today, day_end_time, tzinfo=pytz.timezone(timezone)) + + return day_start <= dt <= day_end + def short(self): return { "username": self.username, From 89cc155ca870ca804b8980784d52fbf24201bb47 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 22:13:28 +0100 Subject: [PATCH 07/22] tests --- engine/apps/user_management/models/user.py | 20 +++++--- .../apps/user_management/tests/test_user.py | 49 +++++++++++++++++++ 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index 6eaba3c9e1..853f884fa5 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -285,12 +285,14 @@ def timezone(self) -> typing.Optional[str]: def timezone(self, value): self._timezone = value - def is_in_working_hours(self, dt: datetime.datetime, timezone: typing.Optional[str] = None) -> bool: - if not timezone: - timezone = self.timezone + def is_in_working_hours(self, dt: datetime.datetime, tz: typing.Optional[str] = None) -> bool: + assert dt.tzinfo == pytz.utc # only pass in UTC - today = dt.date() - day_name = today.strftime("%A").lower() + if not tz: + tz = self.timezone + + dt = dt.astimezone(pytz.timezone(tz)) + day_name = dt.date().strftime("%A").lower() day_start_time_str = self.working_hours[day_name][0]["start"] day_start_time = datetime.time.fromisoformat(day_start_time_str) @@ -298,8 +300,12 @@ def is_in_working_hours(self, dt: datetime.datetime, timezone: typing.Optional[s day_end_time_str = self.working_hours[day_name][0]["end"] day_end_time = datetime.time.fromisoformat(day_end_time_str) - day_start = datetime.datetime.combine(today, day_start_time, tzinfo=pytz.timezone(timezone)) - day_end = datetime.datetime.combine(today, day_end_time, tzinfo=pytz.timezone(timezone)) + day_start = dt.replace( + hour=day_start_time.hour, minute=day_start_time.minute, second=day_start_time.second, microsecond=0 + ) + day_end = dt.replace( + hour=day_end_time.hour, minute=day_end_time.minute, second=day_end_time.second, microsecond=0 + ) return day_start <= dt <= day_end diff --git a/engine/apps/user_management/tests/test_user.py b/engine/apps/user_management/tests/test_user.py index 6928d48979..4664376a69 100644 --- a/engine/apps/user_management/tests/test_user.py +++ b/engine/apps/user_management/tests/test_user.py @@ -1,4 +1,5 @@ import pytest +from django.utils import timezone from apps.api.permissions import LegacyAccessControlRole from apps.user_management.models import User @@ -28,3 +29,51 @@ def test_lower_email_filter(make_organization, make_user_for_organization): assert User.objects.get(email__lower="testinguser@test.com") == user assert User.objects.filter(email__lower__in=["testinguser@test.com"]).get() == user + + +@pytest.mark.django_db +def test_is_in_working_hours(make_organization, make_user_for_organization): + organization = make_organization() + user = make_user_for_organization(organization, _timezone="Europe/London") + + _7_59_utc = timezone.datetime(2023, 8, 1, 7, 59, 59, tzinfo=timezone.utc) + _8_utc = timezone.datetime(2023, 8, 1, 8, 0, 0, tzinfo=timezone.utc) + _17_utc = timezone.datetime(2023, 8, 1, 16, 0, 0, tzinfo=timezone.utc) + _17_01_utc = timezone.datetime(2023, 8, 1, 16, 0, 1, tzinfo=timezone.utc) + + assert user.is_in_working_hours(_7_59_utc) is False + assert user.is_in_working_hours(_8_utc) is True + assert user.is_in_working_hours(_17_utc) is True + assert user.is_in_working_hours(_17_01_utc) is False + + +@pytest.mark.django_db +def test_is_in_working_hours_next_day(make_organization, make_user_for_organization): + organization = make_organization() + user = make_user_for_organization( + organization, + working_hours={ + "tuesday": [{"start": "17:00:00", "end": "18:00:00"}], + "wednesday": [{"start": "01:00:00", "end": "02:00:00"}], + }, + ) + + _8_59_utc = timezone.datetime(2023, 8, 1, 8, 59, 59, tzinfo=timezone.utc) # 4:59pm on Tuesday in Singapore + _9_utc = timezone.datetime(2023, 8, 1, 9, 0, 0, tzinfo=timezone.utc) # 5pm on Tuesday in Singapore + _10_utc = timezone.datetime(2023, 8, 1, 10, 0, 0, tzinfo=timezone.utc) # 6pm on Tuesday in Singapore + _10_01_utc = timezone.datetime(2023, 8, 1, 10, 0, 1, tzinfo=timezone.utc) # 6:01pm on Tuesday in Singapore + + _16_59_utc = timezone.datetime(2023, 8, 1, 16, 59, 0, tzinfo=timezone.utc) # 00:59am on Wednesday in Singapore + _17_utc = timezone.datetime(2023, 8, 1, 17, 0, 0, tzinfo=timezone.utc) # 1am on Wednesday in Singapore + _18_utc = timezone.datetime(2023, 8, 1, 18, 0, 0, tzinfo=timezone.utc) # 2am on Wednesday in Singapore + _18_01_utc = timezone.datetime(2023, 8, 1, 18, 0, 1, tzinfo=timezone.utc) # 2:01am on Wednesday in Singapore + + tz = "Asia/Singapore" + assert user.is_in_working_hours(_8_59_utc, tz=tz) is False + assert user.is_in_working_hours(_9_utc, tz=tz) is True + assert user.is_in_working_hours(_10_utc, tz=tz) is True + assert user.is_in_working_hours(_10_01_utc, tz=tz) is False + assert user.is_in_working_hours(_16_59_utc, tz=tz) is False + assert user.is_in_working_hours(_17_utc, tz=tz) is True + assert user.is_in_working_hours(_18_utc, tz=tz) is True + assert user.is_in_working_hours(_18_01_utc, tz=tz) is False From a62dbbc3cd97879bb5c18a470a69fc8d8abcfb00 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 22:40:16 +0100 Subject: [PATCH 08/22] rename methods --- engine/apps/mobile_app/tasks.py | 41 ++++--- .../tests/test_shift_swap_request.py | 112 +++++++++++++----- 2 files changed, 107 insertions(+), 46 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 670e917eac..5a381ec634 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -502,8 +502,8 @@ def conditionally_send_going_oncall_push_notifications_for_all_schedules() -> No conditionally_send_going_oncall_push_notifications_for_schedule.apply_async((schedule.pk,)) -EARLIEST_NOTIFICATION_OFFSET = datetime.timedelta(weeks=4) -WINDOW = datetime.timedelta(days=1) +SSR_EARLIEST_NOTIFICATION_OFFSET = datetime.timedelta(weeks=4) +SSR_NOTIFICATION_WINDOW = datetime.timedelta(days=1) @shared_dedicated_queue_retry_task() @@ -516,13 +516,16 @@ def notify_shift_swap_requests() -> None: def _get_shift_swap_requests_to_notify(now: datetime.datetime) -> QuerySet[ShiftSwapRequest]: - # This is the same as notification_window_start = max(created_at, swap_start - EARLIEST_NOTIFICATION_OFFSET) + # This is the same as notification_window_start = max(created_at, swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET) notification_window_start = Max( - F("created_at"), ExpressionWrapper(F("swap_start") - EARLIEST_NOTIFICATION_OFFSET, output_field=DateTimeField()) + F("created_at"), + ExpressionWrapper(F("swap_start") - SSR_EARLIEST_NOTIFICATION_OFFSET, output_field=DateTimeField()), ) - # This is the same as notification_window_end = notification_window_start + WINDOW - notification_window_end = ExpressionWrapper(F("notification_window_start") + WINDOW, output_field=DateTimeField()) + # This is the same as notification_window_end = notification_window_start + SSR_NOTIFICATION_WINDOW + notification_window_end = ExpressionWrapper( + F("notification_window_start") + SSR_NOTIFICATION_WINDOW, output_field=DateTimeField() + ) # For every shift swap request, we assign a window of time in which we can notify users about it. # Here we select all the shift swap requests for which now is within its notification window. @@ -547,9 +550,9 @@ def notify_shift_swap_request(shift_swap_request_pk: int) -> None: now = timezone.now() for user in shift_swap_request.possible_benefactors: - if _should_notify_user(shift_swap_request, user, now): + if _should_notify_user_about_shift_swap_request(shift_swap_request, user, now): notify_user_about_shift_swap_request.delay(shift_swap_request.pk, user.pk) - _mark_notified(shift_swap_request, user) + _mark_shift_swap_request_notified(shift_swap_request, user) @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) @@ -584,11 +587,13 @@ def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: in logger.info(f"Info notifications are not enabled for user {user_pk}") return - message = _get_shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings) + message = _shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings) _send_push_notification(device_to_notify, message) -def _should_notify_user(shift_swap_request: ShiftSwapRequest, user: User, now: datetime.datetime) -> bool: +def _should_notify_user_about_shift_swap_request( + shift_swap_request: ShiftSwapRequest, user: User, now: datetime.datetime +) -> bool: from apps.mobile_app.models import MobileAppUserSettings try: @@ -599,25 +604,25 @@ def _should_notify_user(shift_swap_request: ShiftSwapRequest, user: User, now: d return ( mobile_app_user_settings.info_notifications_enabled and user.is_in_working_hours(now, mobile_app_user_settings.time_zone) - and not _already_notified(shift_swap_request, user) + and not _is_shift_swap_request_already_notified(shift_swap_request, user) ) -def _mark_notified(shift_swap_request: ShiftSwapRequest, user: User) -> None: - key = _cache_key(shift_swap_request, user) - cache.set(key, True, timeout=WINDOW.total_seconds()) +def _mark_shift_swap_request_notified(shift_swap_request: ShiftSwapRequest, user: User) -> None: + key = _shift_swap_request_cache_key(shift_swap_request, user) + cache.set(key, True, timeout=SSR_NOTIFICATION_WINDOW.total_seconds()) -def _already_notified(shift_swap_request: ShiftSwapRequest, user: User) -> bool: - key = _cache_key(shift_swap_request, user) +def _is_shift_swap_request_already_notified(shift_swap_request: ShiftSwapRequest, user: User) -> bool: + key = _shift_swap_request_cache_key(shift_swap_request, user) return cache.get(key) is True -def _cache_key(shift_swap_request: ShiftSwapRequest, user: User) -> str: +def _shift_swap_request_cache_key(shift_swap_request: ShiftSwapRequest, user: User) -> str: return f"ssr_push:{shift_swap_request.pk}:{user.pk}" -def _get_shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings): +def _shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings): from apps.mobile_app.models import MobileAppUserSettings thread_id = f"{shift_swap_request.public_primary_key}:{user.public_primary_key}:ssr" diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 40f44a5abd..9f750a3644 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -7,25 +7,26 @@ from apps.mobile_app.models import FCMDevice, MobileAppUserSettings from apps.mobile_app.tasks import ( - EARLIEST_NOTIFICATION_OFFSET, - WINDOW, + SSR_EARLIEST_NOTIFICATION_OFFSET, + SSR_NOTIFICATION_WINDOW, MessageType, - _already_notified, _get_shift_swap_requests_to_notify, - _mark_notified, - _should_notify_user, + _is_shift_swap_request_already_notified, + _mark_shift_swap_request_notified, + _should_notify_user_about_shift_swap_request, notify_shift_swap_request, notify_shift_swap_requests, notify_user_about_shift_swap_request, ) -from apps.schedules.models import OnCallScheduleWeb, ShiftSwapRequest +from apps.schedules.models import CustomOnCallShift, OnCallScheduleWeb, ShiftSwapRequest from apps.user_management.models import User +from apps.user_management.models.user import default_working_hours MICROSECOND = timezone.timedelta(microseconds=1) def test_window_more_than_24_hours(): - assert WINDOW >= timezone.timedelta(hours=24) + assert SSR_NOTIFICATION_WINDOW >= timezone.timedelta(hours=24) @pytest.mark.django_db @@ -46,8 +47,8 @@ def test_get_shift_swap_requests_to_notify_starts_soon( assert list(_get_shift_swap_requests_to_notify(now - MICROSECOND)) == [] assert list(_get_shift_swap_requests_to_notify(now)) == [shift_swap_request] - assert list(_get_shift_swap_requests_to_notify(now + WINDOW)) == [shift_swap_request] - assert list(_get_shift_swap_requests_to_notify(now + WINDOW + MICROSECOND)) == [] + assert list(_get_shift_swap_requests_to_notify(now + SSR_NOTIFICATION_WINDOW)) == [shift_swap_request] + assert list(_get_shift_swap_requests_to_notify(now + SSR_NOTIFICATION_WINDOW + MICROSECOND)) == [] @pytest.mark.django_db @@ -88,13 +89,20 @@ def test_get_shift_swap_requests_to_notify_starts_not_soon( ) assert list(_get_shift_swap_requests_to_notify(now)) == [] - assert list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET - MICROSECOND)) == [] - assert list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET)) == [shift_swap_request] - assert list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET + WINDOW)) == [ + assert list(_get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET - MICROSECOND)) == [] + assert list(_get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET)) == [ shift_swap_request ] + assert list( + _get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET + SSR_NOTIFICATION_WINDOW) + ) == [shift_swap_request] assert ( - list(_get_shift_swap_requests_to_notify(swap_start - EARLIEST_NOTIFICATION_OFFSET + WINDOW + MICROSECOND)) == [] + list( + _get_shift_swap_requests_to_notify( + swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET + SSR_NOTIFICATION_WINDOW + MICROSECOND + ) + ) + == [] ) @@ -152,7 +160,7 @@ def test_notify_shift_swap_request(make_organization, make_user, make_schedule, ) with patch.object(notify_user_about_shift_swap_request, "delay") as mock_notify_user_about_shift_swap_request: - with patch("apps.mobile_app.tasks._should_notify_user", return_value=True): + with patch("apps.mobile_app.tasks._should_notify_user_about_shift_swap_request", return_value=True): with patch.object( ShiftSwapRequest, "possible_benefactors", @@ -181,7 +189,7 @@ def test_notify_shift_swap_request_should_not_notify_user( ) with patch.object(notify_user_about_shift_swap_request, "delay") as mock_notify_user_about_shift_swap_request: - with patch("apps.mobile_app.tasks._should_notify_user", return_value=False): + with patch("apps.mobile_app.tasks._should_notify_user_about_shift_swap_request", return_value=False): with patch.object( ShiftSwapRequest, "possible_benefactors", @@ -192,6 +200,54 @@ def test_notify_shift_swap_request_should_not_notify_user( mock_notify_user_about_shift_swap_request.assert_not_called() +@pytest.mark.django_db +def test_notify_shift_swap_request_success( + make_organization, make_user, make_schedule, make_on_call_shift, make_shift_swap_request, settings +): + organization = make_organization() + beneficiary = make_user(organization=organization) + + # Set up the benefactor + benefactor = make_user( + organization=organization, + working_hours={day: [{"start": "00:00:00", "end": "23:59:59"}] for day in default_working_hours().keys()}, + ) + MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) + cache.clear() + + # Create schedule with the beneficiary and the benefactor in it + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + now = timezone.now() + for user in [benefactor, beneficiary]: + data = { + "start": now - timezone.timedelta(days=1), + "rotation_start": now - timezone.timedelta(days=1), + "duration": timezone.timedelta(hours=1), + "priority_level": 1, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user]]) + + schedule.refresh_ical_file() + schedule.refresh_from_db() + + swap_start = now + timezone.timedelta(days=100) + swap_end = swap_start + timezone.timedelta(days=1) + + shift_swap_request = make_shift_swap_request( + schedule, beneficiary, swap_start=swap_start, swap_end=swap_end, created_at=now + ) + + with patch.object(notify_user_about_shift_swap_request, "delay") as mock_notify_user_about_shift_swap_request: + notify_shift_swap_request(shift_swap_request.pk) + + mock_notify_user_about_shift_swap_request.assert_called_once_with(shift_swap_request.pk, benefactor.pk) + + @pytest.mark.django_db def test_notify_user_about_shift_swap_request( make_organization, make_user, make_schedule, make_shift_swap_request, settings @@ -241,22 +297,22 @@ def test_should_notify_user(make_organization, make_user, make_schedule, make_sh ) assert not MobileAppUserSettings.objects.exists() - assert _should_notify_user(shift_swap_request, benefactor, now) is False + assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) - assert _should_notify_user(shift_swap_request, benefactor, now) is False + assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False with patch.object(benefactor, "is_in_working_hours", return_value=True): - with patch("apps.mobile_app.tasks._already_notified", return_value=True): - assert _should_notify_user(shift_swap_request, benefactor, now) is False + with patch("apps.mobile_app.tasks._is_shift_swap_request_already_notified", return_value=True): + assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False with patch.object(benefactor, "is_in_working_hours", return_value=False): - with patch("apps.mobile_app.tasks._already_notified", return_value=False): - assert _should_notify_user(shift_swap_request, benefactor, now) is False + with patch("apps.mobile_app.tasks._is_shift_swap_request_already_notified", return_value=False): + assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False with patch.object(benefactor, "is_in_working_hours", return_value=True): - with patch("apps.mobile_app.tasks._already_notified", return_value=False): - assert _should_notify_user(shift_swap_request, benefactor, now) is True + with patch("apps.mobile_app.tasks._is_shift_swap_request_already_notified", return_value=False): + assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is True @pytest.mark.django_db @@ -275,10 +331,10 @@ def test_mark_notified(make_organization, make_user, make_schedule, make_shift_s ) cache.clear() - assert _already_notified(shift_swap_request, benefactor) is False - _mark_notified(shift_swap_request, benefactor) - assert _already_notified(shift_swap_request, benefactor) is True + assert _is_shift_swap_request_already_notified(shift_swap_request, benefactor) is False + _mark_shift_swap_request_notified(shift_swap_request, benefactor) + assert _is_shift_swap_request_already_notified(shift_swap_request, benefactor) is True with patch.object(cache, "set") as mock_cache_set: - _mark_notified(shift_swap_request, benefactor) - assert mock_cache_set.call_args.kwargs["timeout"] == WINDOW.total_seconds() + _mark_shift_swap_request_notified(shift_swap_request, benefactor) + assert mock_cache_set.call_args.kwargs["timeout"] == SSR_NOTIFICATION_WINDOW.total_seconds() From 715beada6bdac72697154c2b1e9e9bb435c95484 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 22:52:55 +0100 Subject: [PATCH 09/22] increase window size to 1 week --- engine/apps/mobile_app/tasks.py | 6 +++++- engine/apps/mobile_app/tests/test_shift_swap_request.py | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 5a381ec634..7f9cf55dd6 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -503,7 +503,7 @@ def conditionally_send_going_oncall_push_notifications_for_all_schedules() -> No SSR_EARLIEST_NOTIFICATION_OFFSET = datetime.timedelta(weeks=4) -SSR_NOTIFICATION_WINDOW = datetime.timedelta(days=1) +SSR_NOTIFICATION_WINDOW = datetime.timedelta(weeks=1) @shared_dedicated_queue_retry_task() @@ -587,6 +587,10 @@ def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: in logger.info(f"Info notifications are not enabled for user {user_pk}") return + if shift_swap_request.status != ShiftSwapRequest.Statuses.OPEN: + logger.info(f"Shift swap request {shift_swap_request_pk} is not open anymore") + return + message = _shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings) _send_push_notification(device_to_notify, message) diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 9f750a3644..592db82759 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -26,7 +26,12 @@ def test_window_more_than_24_hours(): - assert SSR_NOTIFICATION_WINDOW >= timezone.timedelta(hours=24) + """ + SSR_NOTIFICATION_WINDOW must be more than one week, otherwise it's not possible to guarantee that the + notification will be sent according to users' working hours. For example, if user only works on Fridays 10am-2pm, + and a shift swap request is created on Friday 3pm, we must wait for a whole week to send the notification. + """ + assert SSR_NOTIFICATION_WINDOW >= timezone.timedelta(weeks=1) @pytest.mark.django_db @@ -38,7 +43,7 @@ def test_get_shift_swap_requests_to_notify_starts_soon( schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) now = timezone.now() - swap_start = now + timezone.timedelta(days=7) + swap_start = now + timezone.timedelta(days=10) swap_end = swap_start + timezone.timedelta(days=1) shift_swap_request = make_shift_swap_request( From 629dc67b36cc79c312b7f5d3ee1ef93b7372bec0 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 23:26:54 +0100 Subject: [PATCH 10/22] add comments --- engine/apps/mobile_app/tasks.py | 45 ++++++++++++++----- .../tests/test_shift_swap_request.py | 18 ++++---- 2 files changed, 44 insertions(+), 19 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 7f9cf55dd6..ad59dfc789 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -502,12 +502,25 @@ def conditionally_send_going_oncall_push_notifications_for_all_schedules() -> No conditionally_send_going_oncall_push_notifications_for_schedule.apply_async((schedule.pk,)) +# TODO: break down tasks.py into multiple files + +# Don't send notifications for shift swap requests that start more than 4 weeks in the future SSR_EARLIEST_NOTIFICATION_OFFSET = datetime.timedelta(weeks=4) + +# Once it's time to send out notifications, send them over the course of a week. +# This is because users can be in multiple timezones / have different working hours configured, +# so we can't just send all notifications at once, but need to wait for the users to be in their working hours. +# Once a notification is sent to a user, they won't be notified again for the same shift swap request for a week. +# After a week, the shift swap request won't be in the notification window anymore (see _get_shift_swap_requests_to_notify). SSR_NOTIFICATION_WINDOW = datetime.timedelta(weeks=1) @shared_dedicated_queue_retry_task() def notify_shift_swap_requests() -> None: + """ + A periodic task that notifies users about shift swap requests. + """ + if not settings.FEATURE_SHIFT_SWAPS_ENABLED: return @@ -516,6 +529,12 @@ def notify_shift_swap_requests() -> None: def _get_shift_swap_requests_to_notify(now: datetime.datetime) -> QuerySet[ShiftSwapRequest]: + """ + Returns shifts swap requests that are open and are in the notification window. + This method can return the same shift swap request multiple times while it's in the notification window, + but users are only notified once per shift swap request (see _mark_shift_swap_request_notified_for_user). + """ + # This is the same as notification_window_start = max(created_at, swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET) notification_window_start = Max( F("created_at"), @@ -527,8 +546,7 @@ def _get_shift_swap_requests_to_notify(now: datetime.datetime) -> QuerySet[Shift F("notification_window_start") + SSR_NOTIFICATION_WINDOW, output_field=DateTimeField() ) - # For every shift swap request, we assign a window of time in which we can notify users about it. - # Here we select all the shift swap requests for which now is within its notification window. + # Return shift swap requests that are not started yet, have no benefactor, and are in the notification window. return ShiftSwapRequest.objects.annotate( notification_window_start=notification_window_start, notification_window_end=notification_window_end, @@ -542,6 +560,9 @@ def _get_shift_swap_requests_to_notify(now: datetime.datetime) -> QuerySet[Shift @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) def notify_shift_swap_request(shift_swap_request_pk: int) -> None: + """ + Notify relevant users for an individual shift swap request. + """ try: shift_swap_request = ShiftSwapRequest.objects.get(pk=shift_swap_request_pk) except ShiftSwapRequest.DoesNotExist: @@ -552,11 +573,14 @@ def notify_shift_swap_request(shift_swap_request_pk: int) -> None: for user in shift_swap_request.possible_benefactors: if _should_notify_user_about_shift_swap_request(shift_swap_request, user, now): notify_user_about_shift_swap_request.delay(shift_swap_request.pk, user.pk) - _mark_shift_swap_request_notified(shift_swap_request, user) + _mark_shift_swap_request_notified_for_user(shift_swap_request, user) @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: int) -> None: + """ + Send a push notification about a shift swap request to an individual user. + """ # avoid circular import from apps.mobile_app.models import FCMDevice, MobileAppUserSettings @@ -598,26 +622,27 @@ def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: in def _should_notify_user_about_shift_swap_request( shift_swap_request: ShiftSwapRequest, user: User, now: datetime.datetime ) -> bool: + # avoid circular import from apps.mobile_app.models import MobileAppUserSettings try: mobile_app_user_settings = MobileAppUserSettings.objects.get(user=user) except MobileAppUserSettings.DoesNotExist: - return False + return False # don't notify if the app is not configured return ( - mobile_app_user_settings.info_notifications_enabled - and user.is_in_working_hours(now, mobile_app_user_settings.time_zone) - and not _is_shift_swap_request_already_notified(shift_swap_request, user) + mobile_app_user_settings.info_notifications_enabled # info notifications must be enabled + and user.is_in_working_hours(now, mobile_app_user_settings.time_zone) # user must be in working hours + and not _has_user_been_notified_for_shift_swap_request(shift_swap_request, user) # don't notify twice ) -def _mark_shift_swap_request_notified(shift_swap_request: ShiftSwapRequest, user: User) -> None: +def _mark_shift_swap_request_notified_for_user(shift_swap_request: ShiftSwapRequest, user: User) -> None: key = _shift_swap_request_cache_key(shift_swap_request, user) cache.set(key, True, timeout=SSR_NOTIFICATION_WINDOW.total_seconds()) -def _is_shift_swap_request_already_notified(shift_swap_request: ShiftSwapRequest, user: User) -> bool: +def _has_user_been_notified_for_shift_swap_request(shift_swap_request: ShiftSwapRequest, user: User) -> bool: key = _shift_swap_request_cache_key(shift_swap_request, user) return cache.get(key) is True @@ -630,7 +655,7 @@ def _shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, from apps.mobile_app.models import MobileAppUserSettings thread_id = f"{shift_swap_request.public_primary_key}:{user.public_primary_key}:ssr" - notification_title = "You have a new shift swap request" + notification_title = "You have a new shift swap request" # TODO: decide on the exact wording notification_subtitle = _get_shift_subtitle( shift_swap_request.schedule, shift_swap_request.swap_start, diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 592db82759..b79869eaf4 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -11,8 +11,8 @@ SSR_NOTIFICATION_WINDOW, MessageType, _get_shift_swap_requests_to_notify, - _is_shift_swap_request_already_notified, - _mark_shift_swap_request_notified, + _has_user_been_notified_for_shift_swap_request, + _mark_shift_swap_request_notified_for_user, _should_notify_user_about_shift_swap_request, notify_shift_swap_request, notify_shift_swap_requests, @@ -308,15 +308,15 @@ def test_should_notify_user(make_organization, make_user, make_schedule, make_sh assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False with patch.object(benefactor, "is_in_working_hours", return_value=True): - with patch("apps.mobile_app.tasks._is_shift_swap_request_already_notified", return_value=True): + with patch("apps.mobile_app.tasks._has_user_been_notified_for_shift_swap_request", return_value=True): assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False with patch.object(benefactor, "is_in_working_hours", return_value=False): - with patch("apps.mobile_app.tasks._is_shift_swap_request_already_notified", return_value=False): + with patch("apps.mobile_app.tasks._has_user_been_notified_for_shift_swap_request", return_value=False): assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False with patch.object(benefactor, "is_in_working_hours", return_value=True): - with patch("apps.mobile_app.tasks._is_shift_swap_request_already_notified", return_value=False): + with patch("apps.mobile_app.tasks._has_user_been_notified_for_shift_swap_request", return_value=False): assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is True @@ -336,10 +336,10 @@ def test_mark_notified(make_organization, make_user, make_schedule, make_shift_s ) cache.clear() - assert _is_shift_swap_request_already_notified(shift_swap_request, benefactor) is False - _mark_shift_swap_request_notified(shift_swap_request, benefactor) - assert _is_shift_swap_request_already_notified(shift_swap_request, benefactor) is True + assert _has_user_been_notified_for_shift_swap_request(shift_swap_request, benefactor) is False + _mark_shift_swap_request_notified_for_user(shift_swap_request, benefactor) + assert _has_user_been_notified_for_shift_swap_request(shift_swap_request, benefactor) is True with patch.object(cache, "set") as mock_cache_set: - _mark_shift_swap_request_notified(shift_swap_request, benefactor) + _mark_shift_swap_request_notified_for_user(shift_swap_request, benefactor) assert mock_cache_set.call_args.kwargs["timeout"] == SSR_NOTIFICATION_WINDOW.total_seconds() From 76279a566a8b20d087dd1a72eaf8482243cdbb40 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 23:29:54 +0100 Subject: [PATCH 11/22] Changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59d6c8e008..82ae90cade 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Added + +- Add mobile app push notifications for shift swap requests by @vadimkerr ([#2717](https://github.com/grafana/oncall/pull/2717)) + ### Changed - Skip past due swap requests when calculating events ([2718](https://github.com/grafana/oncall/pull/2718)) From 0a19d72f8afd4021dc1d032361f466c1eeb00b10 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 23:37:09 +0100 Subject: [PATCH 12/22] simplify _get_shift_swap_requests_to_notify --- engine/apps/mobile_app/tasks.py | 31 ++++++------------ .../tests/test_shift_swap_request.py | 32 ++++++++----------- 2 files changed, 24 insertions(+), 39 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index ad59dfc789..b9d145c32e 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -11,7 +11,6 @@ from celery.utils.log import get_task_logger from django.conf import settings from django.core.cache import cache -from django.db.models import DateTimeField, ExpressionWrapper, F, Max, QuerySet from django.utils import timezone from firebase_admin.exceptions import FirebaseError from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message @@ -528,34 +527,24 @@ def notify_shift_swap_requests() -> None: notify_shift_swap_request.delay(shift_swap_request.pk) -def _get_shift_swap_requests_to_notify(now: datetime.datetime) -> QuerySet[ShiftSwapRequest]: +def _get_shift_swap_requests_to_notify(now: datetime.datetime) -> list[ShiftSwapRequest]: """ Returns shifts swap requests that are open and are in the notification window. This method can return the same shift swap request multiple times while it's in the notification window, but users are only notified once per shift swap request (see _mark_shift_swap_request_notified_for_user). """ - # This is the same as notification_window_start = max(created_at, swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET) - notification_window_start = Max( - F("created_at"), - ExpressionWrapper(F("swap_start") - SSR_EARLIEST_NOTIFICATION_OFFSET, output_field=DateTimeField()), - ) + shift_swap_requests_in_notification_window = [] + for shift_swap_request in ShiftSwapRequest.objects.filter(benefactor__isnull=True, swap_start__gt=now): + notification_window_start = max( + shift_swap_request.created_at, shift_swap_request.swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET + ) + notification_window_end = min(notification_window_start + SSR_NOTIFICATION_WINDOW, shift_swap_request.swap_end) - # This is the same as notification_window_end = notification_window_start + SSR_NOTIFICATION_WINDOW - notification_window_end = ExpressionWrapper( - F("notification_window_start") + SSR_NOTIFICATION_WINDOW, output_field=DateTimeField() - ) + if notification_window_start <= now <= notification_window_end: + shift_swap_requests_in_notification_window.append(shift_swap_request) - # Return shift swap requests that are not started yet, have no benefactor, and are in the notification window. - return ShiftSwapRequest.objects.annotate( - notification_window_start=notification_window_start, - notification_window_end=notification_window_end, - ).filter( - benefactor__isnull=True, - swap_start__gt=now, - notification_window_start__lte=now, - notification_window_end__gte=now, - ) + return shift_swap_requests_in_notification_window @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=MAX_RETRIES) diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index b79869eaf4..6ab784ab23 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -50,10 +50,10 @@ def test_get_shift_swap_requests_to_notify_starts_soon( schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now ) - assert list(_get_shift_swap_requests_to_notify(now - MICROSECOND)) == [] - assert list(_get_shift_swap_requests_to_notify(now)) == [shift_swap_request] - assert list(_get_shift_swap_requests_to_notify(now + SSR_NOTIFICATION_WINDOW)) == [shift_swap_request] - assert list(_get_shift_swap_requests_to_notify(now + SSR_NOTIFICATION_WINDOW + MICROSECOND)) == [] + assert _get_shift_swap_requests_to_notify(now - MICROSECOND) == [] + assert _get_shift_swap_requests_to_notify(now) == [shift_swap_request] + assert _get_shift_swap_requests_to_notify(now + SSR_NOTIFICATION_WINDOW) == [shift_swap_request] + assert _get_shift_swap_requests_to_notify(now + SSR_NOTIFICATION_WINDOW + MICROSECOND) == [] @pytest.mark.django_db @@ -72,9 +72,9 @@ def test_get_shift_swap_requests_to_notify_starts_very_soon( schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now ) - assert list(_get_shift_swap_requests_to_notify(now - MICROSECOND)) == [] - assert list(_get_shift_swap_requests_to_notify(now)) == [shift_swap_request] - assert list(_get_shift_swap_requests_to_notify(now + timezone.timedelta(minutes=1))) == [] + assert _get_shift_swap_requests_to_notify(now - MICROSECOND) == [] + assert _get_shift_swap_requests_to_notify(now) == [shift_swap_request] + assert _get_shift_swap_requests_to_notify(now + timezone.timedelta(minutes=1)) == [] @pytest.mark.django_db @@ -93,19 +93,15 @@ def test_get_shift_swap_requests_to_notify_starts_not_soon( schedule, user, swap_start=swap_start, swap_end=swap_end, created_at=now ) - assert list(_get_shift_swap_requests_to_notify(now)) == [] - assert list(_get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET - MICROSECOND)) == [] - assert list(_get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET)) == [ - shift_swap_request - ] - assert list( - _get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET + SSR_NOTIFICATION_WINDOW) + assert _get_shift_swap_requests_to_notify(now) == [] + assert _get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET - MICROSECOND) == [] + assert _get_shift_swap_requests_to_notify(swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET) == [shift_swap_request] + assert _get_shift_swap_requests_to_notify( + swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET + SSR_NOTIFICATION_WINDOW ) == [shift_swap_request] assert ( - list( - _get_shift_swap_requests_to_notify( - swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET + SSR_NOTIFICATION_WINDOW + MICROSECOND - ) + _get_shift_swap_requests_to_notify( + swap_start - SSR_EARLIEST_NOTIFICATION_OFFSET + SSR_NOTIFICATION_WINDOW + MICROSECOND ) == [] ) From 1f99f8c04cf5c332a0d11059dd4f05f14039a56d Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Tue, 1 Aug 2023 23:52:45 +0100 Subject: [PATCH 13/22] add resource_url --- engine/apps/mobile_app/tasks.py | 5 +++++ engine/apps/mobile_app/tests/test_shift_swap_request.py | 3 +++ 2 files changed, 8 insertions(+) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index b9d145c32e..1b18b35ecb 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -11,6 +11,7 @@ from celery.utils.log import get_task_logger from django.conf import settings from django.core.cache import cache +from django.urls import reverse from django.utils import timezone from firebase_admin.exceptions import FirebaseError from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message @@ -652,9 +653,13 @@ def _shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings, ) + # TODO: check with the mobile team + resource_url = reverse("api-internal:shift_swap-detail", kwargs={"pk": shift_swap_request.public_primary_key}) + data: FCMMessageData = { "title": notification_title, "subtitle": notification_subtitle, + "resource_url": resource_url, "info_notification_sound_name": ( mobile_app_user_settings.info_notification_sound_name + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION ), diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 6ab784ab23..1ef801aea5 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -253,6 +253,8 @@ def test_notify_shift_swap_request_success( def test_notify_user_about_shift_swap_request( make_organization, make_user, make_schedule, make_shift_swap_request, settings ): + settings.FEATURE_SHIFT_SWAPS_ENABLED = True + organization = make_organization() beneficiary = make_user(organization=organization) benefactor = make_user(organization=organization) @@ -279,6 +281,7 @@ def test_notify_user_about_shift_swap_request( assert message.data["type"] == MessageType.INFO assert message.data["title"] == "You have a new shift swap request" assert message.data["subtitle"] == "11/9/23, 7:38\u202fPM - 11/10/23, 7:38\u202fPM\nSchedule Test" + assert message.data["resource_url"] == f"/api/internal/v1/shift_swaps/{shift_swap_request.public_primary_key}" assert message.apns.payload.aps.sound.critical is False From 174dba2ac677536e6050797da68dd7218e900442 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 00:01:41 +0100 Subject: [PATCH 14/22] typing --- engine/apps/mobile_app/tasks.py | 7 ++++++- engine/apps/schedules/models/shift_swap_request.py | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 1b18b35ecb..47c3ce74a6 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -641,7 +641,12 @@ def _shift_swap_request_cache_key(shift_swap_request: ShiftSwapRequest, user: Us return f"ssr_push:{shift_swap_request.pk}:{user.pk}" -def _shift_swap_request_fcm_message(shift_swap_request, user, device_to_notify, mobile_app_user_settings): +def _shift_swap_request_fcm_message( + shift_swap_request: ShiftSwapRequest, + user: User, + device_to_notify: "FCMDevice", + mobile_app_user_settings: "MobileAppUserSettings", +) -> Message: from apps.mobile_app.models import MobileAppUserSettings thread_id = f"{shift_swap_request.public_primary_key}:{user.public_primary_key}:ssr" diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index ff7b918fd8..95c952f8dc 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -4,6 +4,7 @@ from django.conf import settings from django.core.validators import MinLengthValidator from django.db import models +from django.db.models import QuerySet from django.utils import timezone from apps.schedules import exceptions @@ -151,7 +152,7 @@ def organization(self) -> "Organization": return self.schedule.organization @property - def possible_benefactors(self): + def possible_benefactors(self) -> QuerySet["User"]: return self.schedule.related_users().exclude(pk=self.beneficiary_id) @property From f747ec19c850a963e8db1223f5e72499c0d88d2a Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 09:24:55 +0100 Subject: [PATCH 15/22] add is_open property --- engine/apps/mobile_app/tasks.py | 2 +- engine/apps/schedules/models/shift_swap_request.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 47c3ce74a6..7fd9fe1d63 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -601,7 +601,7 @@ def notify_user_about_shift_swap_request(shift_swap_request_pk: int, user_pk: in logger.info(f"Info notifications are not enabled for user {user_pk}") return - if shift_swap_request.status != ShiftSwapRequest.Statuses.OPEN: + if not shift_swap_request.is_open: logger.info(f"Shift swap request {shift_swap_request_pk} is not open anymore") return diff --git a/engine/apps/schedules/models/shift_swap_request.py b/engine/apps/schedules/models/shift_swap_request.py index 95c952f8dc..ebf4881174 100644 --- a/engine/apps/schedules/models/shift_swap_request.py +++ b/engine/apps/schedules/models/shift_swap_request.py @@ -129,6 +129,10 @@ def is_taken(self) -> bool: def is_past_due(self) -> bool: return timezone.now() > self.swap_start + @property + def is_open(self) -> bool: + return not any((self.is_deleted, self.is_taken, self.is_past_due)) + @property def status(self) -> str: if self.is_deleted: From c02912b2a6024b0573e0a2653582c371e76e323c Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 09:41:19 +0100 Subject: [PATCH 16/22] fix is_in_working_hours on empty working hours --- engine/apps/user_management/models/user.py | 14 +++++++++++++- engine/apps/user_management/tests/test_user.py | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index 853f884fa5..f92571e8f4 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -286,20 +286,32 @@ def timezone(self, value): self._timezone = value def is_in_working_hours(self, dt: datetime.datetime, tz: typing.Optional[str] = None) -> bool: - assert dt.tzinfo == pytz.utc # only pass in UTC + assert dt.tzinfo == pytz.utc, "dt must be in UTC" + # Default to user's timezone if not tz: tz = self.timezone + # If user has no timezone set, any time is considered non-working hours + if not tz: + return False + + # Convert to user's timezone and get day name (e.g. monday) dt = dt.astimezone(pytz.timezone(tz)) day_name = dt.date().strftime("%A").lower() + # If no working hours for the day, return False + if day_name not in self.working_hours or not self.working_hours[day_name]: + return False + + # Extract start and end time for the day from working hours day_start_time_str = self.working_hours[day_name][0]["start"] day_start_time = datetime.time.fromisoformat(day_start_time_str) day_end_time_str = self.working_hours[day_name][0]["end"] day_end_time = datetime.time.fromisoformat(day_end_time_str) + # Calculate day start and end datetime day_start = dt.replace( hour=day_start_time.hour, minute=day_start_time.minute, second=day_start_time.second, microsecond=0 ) diff --git a/engine/apps/user_management/tests/test_user.py b/engine/apps/user_management/tests/test_user.py index 4664376a69..5fd8baffe8 100644 --- a/engine/apps/user_management/tests/test_user.py +++ b/engine/apps/user_management/tests/test_user.py @@ -77,3 +77,20 @@ def test_is_in_working_hours_next_day(make_organization, make_user_for_organizat assert user.is_in_working_hours(_17_utc, tz=tz) is True assert user.is_in_working_hours(_18_utc, tz=tz) is True assert user.is_in_working_hours(_18_01_utc, tz=tz) is False + + +@pytest.mark.django_db +def test_is_in_working_hours_no_timezone(make_organization, make_user_for_organization): + organization = make_organization() + user = make_user_for_organization(organization, _timezone=None) + + assert user.is_in_working_hours(timezone.now()) is False + + +@pytest.mark.django_db +def test_is_in_working_hours_weekend(make_organization, make_user_for_organization): + organization = make_organization() + user = make_user_for_organization(organization, working_hours={"saturday": []}, _timezone=None) + + on_saturday = timezone.datetime(2023, 8, 5, 12, 0, 0, tzinfo=timezone.utc) + assert user.is_in_working_hours(on_saturday, "UTC") is False From 221fee34373be5b1db2975a7be99758eef007367 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 11:24:11 +0100 Subject: [PATCH 17/22] update subtitle --- engine/apps/mobile_app/tasks.py | 10 +++------- .../apps/mobile_app/tests/test_shift_swap_request.py | 8 ++++---- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 7fd9fe1d63..437c4147c7 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -650,13 +650,9 @@ def _shift_swap_request_fcm_message( from apps.mobile_app.models import MobileAppUserSettings thread_id = f"{shift_swap_request.public_primary_key}:{user.public_primary_key}:ssr" - notification_title = "You have a new shift swap request" # TODO: decide on the exact wording - notification_subtitle = _get_shift_subtitle( - shift_swap_request.schedule, - shift_swap_request.swap_start, - shift_swap_request.swap_end, - mobile_app_user_settings, - ) + notification_title = "New shift swap request" + beneficiary_name = shift_swap_request.beneficiary.name or shift_swap_request.beneficiary.username + notification_subtitle = f"{beneficiary_name}, {shift_swap_request.schedule.name}" # TODO: check with the mobile team resource_url = reverse("api-internal:shift_swap-detail", kwargs={"pk": shift_swap_request.public_primary_key}) diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 1ef801aea5..ea5710d4b7 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -256,9 +256,9 @@ def test_notify_user_about_shift_swap_request( settings.FEATURE_SHIFT_SWAPS_ENABLED = True organization = make_organization() - beneficiary = make_user(organization=organization) + beneficiary = make_user(organization=organization, name="John Doe", username="john.doe") benefactor = make_user(organization=organization) - schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb, name="Test") + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb, name="Test Schedule") device_to_notify = FCMDevice.objects.create(user=benefactor, registration_id="test_device_id") MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) @@ -279,8 +279,8 @@ def test_notify_user_about_shift_swap_request( message: Message = mock_send_push_notification.call_args.args[1] assert message.data["type"] == MessageType.INFO - assert message.data["title"] == "You have a new shift swap request" - assert message.data["subtitle"] == "11/9/23, 7:38\u202fPM - 11/10/23, 7:38\u202fPM\nSchedule Test" + assert message.data["title"] == "New shift swap request" + assert message.data["subtitle"] == "John Doe, Test Schedule" assert message.data["resource_url"] == f"/api/internal/v1/shift_swaps/{shift_swap_request.public_primary_key}" assert message.apns.payload.aps.sound.critical is False From 43c0911b97f7ab2e5acaab130a528c1610b9053b Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 11:28:56 +0100 Subject: [PATCH 18/22] revert subtitle changes --- engine/apps/mobile_app/tasks.py | 12 ++-- .../test_your_going_oncall_notification.py | 67 ++++++++++++++----- 2 files changed, 58 insertions(+), 21 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index 437c4147c7..c6f3eae23d 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -232,12 +232,14 @@ def _get_youre_going_oncall_notification_title(seconds_until_going_oncall: int) return f"Your on-call shift starts in {humanize.naturaldelta(seconds_until_going_oncall)}" -def _get_shift_subtitle( +def _get_youre_going_oncall_notification_subtitle( schedule: OnCallSchedule, - shift_start: datetime.datetime, - shift_end: datetime.datetime, + schedule_event: ScheduleEvent, mobile_app_user_settings: "MobileAppUserSettings", ) -> str: + shift_start = schedule_event["start"] + shift_end = schedule_event["end"] + shift_starts_and_ends_on_same_day = shift_start.date() == shift_end.date() dt_formatter_func = format_localized_time if shift_starts_and_ends_on_same_day else format_localized_datetime @@ -270,8 +272,8 @@ def _get_youre_going_oncall_fcm_message( mobile_app_user_settings, _ = MobileAppUserSettings.objects.get_or_create(user=user) notification_title = _get_youre_going_oncall_notification_title(seconds_until_going_oncall) - notification_subtitle = _get_shift_subtitle( - schedule, schedule_event["start"], schedule_event["end"], mobile_app_user_settings + notification_subtitle = _get_youre_going_oncall_notification_subtitle( + schedule, schedule_event, mobile_app_user_settings ) data: FCMMessageData = { 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 75cfda7c0b..a218fd6ffa 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 @@ -54,6 +54,8 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m organization, user = make_organization_and_user() user2 = make_user(organization=organization) schedule = make_schedule(organization, name=schedule_name, schedule_class=OnCallScheduleWeb) + shift_pk = "mncvmnvc" + user_pk = user.public_primary_key user_locale = "fr_CA" seconds_until_going_oncall = 600 humanized_time_until_going_oncall = "10 minutes" @@ -64,6 +66,28 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m multiple_day_shift_start = timezone.datetime(2023, 7, 8, 9, 0, 0) multiple_day_shift_end = timezone.datetime(2023, 7, 12, 17, 0, 0) + same_day_shift = _create_schedule_event( + same_day_shift_start, + same_day_shift_end, + shift_pk, + [ + { + "pk": user_pk, + }, + ], + ) + + multiple_day_shift = _create_schedule_event( + multiple_day_shift_start, + multiple_day_shift_end, + shift_pk, + [ + { + "pk": user_pk, + }, + ], + ) + maus = MobileAppUserSettings.objects.create(user=user, locale=user_locale) maus_no_locale = MobileAppUserSettings.objects.create(user=user2) @@ -71,9 +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_shift_subtitle(schedule, same_day_shift_start, same_day_shift_end, maus) - same_day_shift_no_locale_subtitle = tasks._get_shift_subtitle( - schedule, same_day_shift_start, same_day_shift_end, maus_no_locale + 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 ) assert same_day_shift_title == f"Your on-call shift starts in {humanized_time_until_going_oncall}" @@ -84,11 +108,11 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m # multiple day shift ################## multiple_day_shift_title = tasks._get_youre_going_oncall_notification_title(seconds_until_going_oncall) - multiple_day_shift_subtitle = tasks._get_shift_subtitle( - schedule, multiple_day_shift_start, multiple_day_shift_end, maus + multiple_day_shift_subtitle = tasks._get_youre_going_oncall_notification_subtitle( + schedule, multiple_day_shift, maus ) - multiple_day_shift_no_locale_subtitle = tasks._get_shift_subtitle( - schedule, multiple_day_shift_start, multiple_day_shift_end, maus_no_locale + multiple_day_shift_no_locale_subtitle = tasks._get_youre_going_oncall_notification_subtitle( + schedule, multiple_day_shift, maus_no_locale ) assert multiple_day_shift_title == f"Your on-call shift starts in {humanized_time_until_going_oncall}" @@ -107,13 +131,14 @@ def test_get_youre_going_oncall_notification_title(make_organization_and_user, m ], ) @pytest.mark.django_db -def test_get_shift_subtitle( +def test_get_youre_going_oncall_notification_subtitle( make_organization, make_user_for_organization, make_schedule, user_timezone, expected_shift_times ): schedule_name = "asdfasdfasdfasdf" organization = make_organization() user = make_user_for_organization(organization) + user_pk = user.public_primary_key maus = MobileAppUserSettings.objects.create(user=user, time_zone=user_timezone) schedule = make_schedule(organization, name=schedule_name, schedule_class=OnCallScheduleWeb) @@ -121,13 +146,24 @@ def test_get_shift_subtitle( shift_start = timezone.datetime(2023, 7, 8, 9, 0, 0) shift_end = timezone.datetime(2023, 7, 8, 17, 0, 0) + shift = _create_schedule_event( + shift_start, + shift_end, + "asdfasdfasdf", + [ + { + "pk": user_pk, + }, + ], + ) + assert ( - tasks._get_shift_subtitle(schedule, shift_start, shift_end, maus) + tasks._get_youre_going_oncall_notification_subtitle(schedule, shift, maus) == f"{expected_shift_times}\nSchedule {schedule_name}" ) -@mock.patch("apps.mobile_app.tasks._get_shift_subtitle") +@mock.patch("apps.mobile_app.tasks._get_youre_going_oncall_notification_subtitle") @mock.patch("apps.mobile_app.tasks._get_youre_going_oncall_notification_title") @mock.patch("apps.mobile_app.tasks._construct_fcm_message") @mock.patch("apps.mobile_app.tasks.APNSPayload") @@ -142,7 +178,7 @@ def test_get_youre_going_oncall_fcm_message( mock_apns_payload, mock_construct_fcm_message, mock_get_youre_going_oncall_notification_title, - mock_get_shift_subtitle, + mock_get_youre_going_oncall_notification_subtitle, make_organization, make_user_for_organization, make_schedule, @@ -155,7 +191,7 @@ def test_get_youre_going_oncall_fcm_message( mock_construct_fcm_message.return_value = mock_fcm_message mock_get_youre_going_oncall_notification_title.return_value = mock_notification_title - mock_get_shift_subtitle.return_value = mock_notification_subtitle + mock_get_youre_going_oncall_notification_subtitle.return_value = mock_notification_subtitle organization = make_organization() user_tz = "Europe/Amsterdam" @@ -164,10 +200,9 @@ def test_get_youre_going_oncall_fcm_message( schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) notification_thread_id = f"{schedule.public_primary_key}:{user_pk}:going-oncall" - shift_start = shift_end = timezone.now() schedule_event = _create_schedule_event( - shift_start, - shift_end, + timezone.now(), + timezone.now(), shift_pk, [ { @@ -210,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_shift_subtitle.assert_called_once_with(schedule, shift_start, shift_end, maus) + 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( From b8e7b188c7e2ff8e2c36e77d13dd9e16b1161828 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 11:30:01 +0100 Subject: [PATCH 19/22] revert subtitle changes --- engine/apps/mobile_app/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index c6f3eae23d..fed6304d6b 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -239,7 +239,6 @@ def _get_youre_going_oncall_notification_subtitle( ) -> str: shift_start = schedule_event["start"] shift_end = schedule_event["end"] - shift_starts_and_ends_on_same_day = shift_start.date() == shift_end.date() dt_formatter_func = format_localized_time if shift_starts_and_ends_on_same_day else format_localized_datetime From 747f3d7a83464440930ffd5850aba97ebc105134 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 11:34:38 +0100 Subject: [PATCH 20/22] add tasks to CELERY_TASK_ROUTES --- engine/settings/prod_without_db.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/settings/prod_without_db.py b/engine/settings/prod_without_db.py index 64c5896f73..f8d4342656 100644 --- a/engine/settings/prod_without_db.py +++ b/engine/settings/prod_without_db.py @@ -56,6 +56,9 @@ def on_uwsgi_worker_exit(): "apps.metrics_exporter.tasks.start_calculate_and_cache_metrics": {"queue": "default"}, "apps.metrics_exporter.tasks.start_recalculation_for_new_metric": {"queue": "default"}, "apps.metrics_exporter.tasks.save_organizations_ids_in_cache": {"queue": "default"}, + "apps.mobile_app.tasks.notify_shift_swap_requests": {"queue": "default"}, + "apps.mobile_app.tasks.notify_shift_swap_request": {"queue": "default"}, + "apps.mobile_app.tasks.notify_user_about_shift_swap_request": {"queue": "default"}, "apps.schedules.tasks.refresh_ical_files.refresh_ical_file": {"queue": "default"}, "apps.schedules.tasks.refresh_ical_files.start_refresh_ical_files": {"queue": "default"}, "apps.schedules.tasks.notify_about_gaps_in_schedule.check_empty_shifts_in_schedule": {"queue": "default"}, From 87819d8f24a91b59f534b6ef8778e8f5106da980 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 11:55:09 +0100 Subject: [PATCH 21/22] resource_url -> route --- engine/apps/mobile_app/tasks.py | 7 +++---- engine/apps/mobile_app/tests/test_shift_swap_request.py | 5 ++++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/engine/apps/mobile_app/tasks.py b/engine/apps/mobile_app/tasks.py index fed6304d6b..a4cd988961 100644 --- a/engine/apps/mobile_app/tasks.py +++ b/engine/apps/mobile_app/tasks.py @@ -11,7 +11,6 @@ from celery.utils.log import get_task_logger from django.conf import settings from django.core.cache import cache -from django.urls import reverse from django.utils import timezone from firebase_admin.exceptions import FirebaseError from firebase_admin.messaging import AndroidConfig, APNSConfig, APNSPayload, Aps, ApsAlert, CriticalSound, Message @@ -655,13 +654,13 @@ def _shift_swap_request_fcm_message( beneficiary_name = shift_swap_request.beneficiary.name or shift_swap_request.beneficiary.username notification_subtitle = f"{beneficiary_name}, {shift_swap_request.schedule.name}" - # TODO: check with the mobile team - resource_url = reverse("api-internal:shift_swap-detail", kwargs={"pk": shift_swap_request.public_primary_key}) + # The mobile app will use this route to open the shift swap request + route = f"/schedules/{shift_swap_request.schedule.public_primary_key}/ssrs/{shift_swap_request.public_primary_key}" data: FCMMessageData = { "title": notification_title, "subtitle": notification_subtitle, - "resource_url": resource_url, + "route": route, "info_notification_sound_name": ( mobile_app_user_settings.info_notification_sound_name + MobileAppUserSettings.ANDROID_SOUND_NAME_EXTENSION ), diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index ea5710d4b7..79acef782e 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -281,7 +281,10 @@ def test_notify_user_about_shift_swap_request( assert message.data["type"] == MessageType.INFO assert message.data["title"] == "New shift swap request" assert message.data["subtitle"] == "John Doe, Test Schedule" - assert message.data["resource_url"] == f"/api/internal/v1/shift_swaps/{shift_swap_request.public_primary_key}" + assert ( + message.data["route"] + == f"/schedules/{schedule.public_primary_key}/ssrs/{shift_swap_request.public_primary_key}" + ) assert message.apns.payload.aps.sound.critical is False From dfef4128e1201d0c83d05c9e2b8bd7ea32b0421f Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Wed, 2 Aug 2023 12:07:38 +0100 Subject: [PATCH 22/22] fix test --- engine/apps/mobile_app/tests/test_shift_swap_request.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/engine/apps/mobile_app/tests/test_shift_swap_request.py b/engine/apps/mobile_app/tests/test_shift_swap_request.py index 79acef782e..46d8b485a2 100644 --- a/engine/apps/mobile_app/tests/test_shift_swap_request.py +++ b/engine/apps/mobile_app/tests/test_shift_swap_request.py @@ -306,9 +306,12 @@ def test_should_notify_user(make_organization, make_user, make_schedule, make_sh assert not MobileAppUserSettings.objects.exists() assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False - MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=True) + mobile_app_settings = MobileAppUserSettings.objects.create(user=benefactor, info_notifications_enabled=False) assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False + mobile_app_settings.info_notifications_enabled = True + mobile_app_settings.save(update_fields=["info_notifications_enabled"]) + with patch.object(benefactor, "is_in_working_hours", return_value=True): with patch("apps.mobile_app.tasks._has_user_been_notified_for_shift_swap_request", return_value=True): assert _should_notify_user_about_shift_swap_request(shift_swap_request, benefactor, now) is False