Add mobile app push notifications for shift swap requests #2717

vstpme · 2023-08-01T17:31:56Z

What this PR does

Adds mobile app push notifications for shift swap requests.

Which issue(s) this PR fixes

#2630

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
CHANGELOG.md updated (or pr:no changelog PR label added if not required)

vstpme · 2023-08-01T22:38:30Z

engine/apps/mobile_app/tasks.py

@@ -230,13 +231,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(


Reusing this method to generate subtitles both for on-call shifts and SSRs

vstpme · 2023-08-01T22:38:52Z

engine/apps/mobile_app/tasks.py

@@ -499,3 +499,183 @@ 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,))
+
+
+# TODO: break down tasks.py into multiple files


Leaving this TODO out of the scope of this PR

agreed, good idea. Created #2722 to track this.

vstpme · 2023-08-01T22:41:03Z

engine/apps/schedules/models/on_call_schedule.py

@@ -57,7 +57,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]+)")


This didn't match usernames containing dots (e.g. vadim.stepanov). Added a test for this case.

vstpme · 2023-08-01T22:41:53Z

engine/apps/schedules/models/shift_swap_request.py

@@ -60,7 +60,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)


This is required to override created_at in tests: FactoryBoy/factory_boy#102 (comment)

vstpme · 2023-08-01T22:43:40Z

engine/apps/mobile_app/tasks.py

+    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


let me know if you have any ideas on notification title

What about "Your teammate has opened a shift swap request"? wdyt?

vstpme · 2023-08-01T22:54:33Z

engine/apps/mobile_app/tasks.py

+    # TODO: check with the mobile team
+    resource_url = reverse("api-internal:shift_swap-detail", kwargs={"pk": shift_swap_request.public_primary_key})


need to check this with @grafana/grafana-oncall-mobile, see here for the actual URL format

vstpme · 2023-08-01T23:05:33Z

engine/apps/schedules/migrations/0016_alter_shiftswaprequest_created_at.py

See #2717 (comment)

joeyorlando

great work, looks awesome!

joeyorlando · 2023-08-02T07:24:18Z

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",


this also needs to be added to CELERY_TASK_ROUTES (and also in oncall-private), right?

added in 747f3d7

joeyorlando · 2023-08-02T07:24:33Z

engine/apps/user_management/models/user.py

@@ -283,6 +285,30 @@ def timezone(self) -> typing.Optional[str]:
    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


joeyorlando · 2023-08-02T07:29:51Z

engine/apps/mobile_app/tasks.py

@@ -499,3 +499,183 @@ 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,))
+
+
+# TODO: break down tasks.py into multiple files


agreed, good idea. Created #2722 to track this.

joeyorlando · 2023-08-02T07:31:04Z

engine/apps/mobile_app/tasks.py

+    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_for_user(shift_swap_request, user)


should this happen at the very end of notify_user_about_shift_swap_request? In case something inside there fails we probably don't want to mark the notification as sent. wdyt?

I didn't want to do that to make sure the mobile app doesn't spam people. I'd rather not receive any SSR notifications at all than receive more than one (potentially once every 15 minutes if something goes wrong between sending the push notification and marking the notification sent). The notify_user_about_shift_swap_request task has a retry policy, so if something goes wrong while sending the push notification (e.g. network issue), the task will probably succeed after a retry. Lmk if this makes sense

task has a retry policy

ahh yes, forgot about this. Makes sense 👍

joeyorlando · 2023-08-02T07:33:50Z

engine/apps/mobile_app/tasks.py

+    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


What about "Your teammate has opened a shift swap request"? wdyt?

joeyorlando · 2023-08-02T07:36:32Z

engine/apps/mobile_app/tasks.py

+        logger.info(f"Info notifications are not enabled for user {user_pk}")
+        return
+
+    if shift_swap_request.status != ShiftSwapRequest.Statuses.OPEN:


nit: we could create a @property on ShiftSwapRequest as such:

@property def is_open(self) -> bool: return not any([self.is_deleted, self.is_taken, self.is_past_due])

Good call, added in f747ec1

joeyorlando · 2023-08-02T07:36:53Z

engine/apps/mobile_app/tasks.py

+    return (
+        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
+    )


vstpme added 11 commits August 1, 2023 18:31

WIP

3888ff7

Merge branch 'dev' into vadimkerr/ssr-push-notifications

bdb1843

add test on usernames with dots

c9db7fe

test

2fe4978

basic tests

2dff0ca

more tests

6580a12

move is_in_working_hours to User model

339836b

tests

89cc155

rename methods

a62dbbc

increase window size to 1 week

715bead

add comments

629dc67

vstpme added the pr:no public docs Added to a PR that does not require public documentation updates label Aug 1, 2023

Merge branch 'dev' into vadimkerr/ssr-push-notifications

13038b3

vstpme changed the title ~~Shift swap request push notifications~~ Add mobile app push notifications for shift swap requests Aug 1, 2023

vstpme added 4 commits August 1, 2023 23:29

Changelog

76279a5

simplify _get_shift_swap_requests_to_notify

0a19d72

add resource_url

1f99f8c

typing

174dba2

vstpme commented Aug 1, 2023

View reviewed changes

vstpme marked this pull request as ready for review August 1, 2023 23:08

vstpme requested a review from a team August 1, 2023 23:08

joeyorlando mentioned this pull request Aug 2, 2023

Refactor engine/apps/mobile_app/tasks.py into smaller individual task files #2722

Closed

joeyorlando approved these changes Aug 2, 2023

View reviewed changes

vstpme added 6 commits August 2, 2023 09:24

add is_open property

f747ec1

fix is_in_working_hours on empty working hours

c02912b

update subtitle

221fee3

revert subtitle changes

43c0911

revert subtitle changes

b8e7b18

add tasks to CELERY_TASK_ROUTES

747f3d7

vstpme added 2 commits August 2, 2023 11:55

resource_url -> route

87819d8

fix test

dfef412

vstpme merged commit c855258 into dev Aug 2, 2023

vstpme deleted the vadimkerr/ssr-push-notifications branch August 2, 2023 11:26

vstpme mentioned this pull request Aug 11, 2023

Shift Swap Requests - when a new request is created, intelligently send out mobile app push notifications #2630

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mobile app push notifications for shift swap requests #2717

Add mobile app push notifications for shift swap requests #2717

vstpme commented Aug 1, 2023 •

edited

Loading

vstpme Aug 1, 2023

vstpme Aug 1, 2023

joeyorlando Aug 2, 2023

vstpme Aug 1, 2023 •

edited

Loading

vstpme Aug 1, 2023

vstpme Aug 1, 2023

joeyorlando Aug 2, 2023

vstpme Aug 1, 2023

vstpme Aug 1, 2023

joeyorlando left a comment

joeyorlando Aug 2, 2023

vstpme Aug 2, 2023

joeyorlando Aug 2, 2023

joeyorlando Aug 2, 2023

joeyorlando Aug 2, 2023

vstpme Aug 2, 2023

joeyorlando Aug 2, 2023

joeyorlando Aug 2, 2023

joeyorlando Aug 2, 2023

vstpme Aug 2, 2023

joeyorlando Aug 2, 2023

		# TODO: check with the mobile team
		resource_url = reverse("api-internal:shift_swap-detail", kwargs={"pk": shift_swap_request.public_primary_key})

Add mobile app push notifications for shift swap requests #2717

Add mobile app push notifications for shift swap requests #2717

Conversation

vstpme commented Aug 1, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstpme Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyorlando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstpme commented Aug 1, 2023 •

edited

Loading

vstpme Aug 1, 2023 •

edited

Loading