Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mobile app settings backend #1571

Merged
merged 14 commits into from
Mar 22, 2023
Merged

Mobile app settings backend #1571

merged 14 commits into from
Mar 22, 2023

Conversation

vstpme
Copy link
Member

@vstpme vstpme commented Mar 17, 2023

What this PR does

Adds mobile app settings support to OnCall backend.

  • Adds a new Django model MobileAppUserSettings to store push notification settings
  • Adds a new endpoint /mobile_app/v1/user_settings to fetch/update settings from the mobile app

Some additional info on implementation: at first I wanted to extend the messaging backend system to allow storing / retrieving per-user data and implement mobile app settings based on those changes. After some thought I decided not to extend the messaging backend system and have this as functionality specific to the mobile_app Django app. Currently the messaging backend system is used by the backend and plugin UI, but mobile app settings are specific only to the mobile app and not configurable in the plugin UI.

tldr: wanted to extend messaging backend system, but decided not to do that

Usage

Get settings via API

GET /mobile_app/v1/user_settings
Example response:

{
  "default_notification_sound_name": "default_sound",  # sound name without file extension
  "default_notification_volume_type": "constant",
  "default_notification_volume": 0.8,
  "default_notification_volume_override": false,
  "important_notification_sound_name": "default_sound_important",  # sound name without file extension
  "important_notification_volume_type": "constant",
  "important_notification_volume": 0.8,
  "important_notification_override_dnd": true
}

Update settings via API

PUT /mobile_app/v1/user_settings - see example response above for payload shape.

Note that sound names must be passed without file extension. When sending push notifications, the backend will add .mp3 to sound names and pass it to push notification data for Android. For iOS, sound names will be suffixed with .aiff to be used by APNS.

Get settings from notification data for Android

All the settings from example response will be available in push notification data (along with orgId, alertGroupId, title, etc.).
Fields default_notification_volume, default_notification_volume_override and important_notification_volume , important_notification_override_dnd will be converted to strings due to FCM limitations.
Fields default_notification_sound_name and important_notification_sound_name will be suffixed with .mp3 in push notification data.

iOS limitations

While Android push notifications are handled purely on the mobile app side, iOS notifications are sent via APNS which imposes some limitations.

  • Notification volume cannot be overridden for non-critical notifications (so fields default_notification_volume_override and default_notification_volume will be disregarded for iOS notifications)
  • It's not possible to control volume type (i.e. "constant" vs "intensifying") via APNS. A possible workaround is to have different sound files for "constant" and "intensifying" and pass that as default_notification_sound_name / important_notification_sound_name.

Which issue(s) this PR fixes

Related to https://github.com/grafana/oncall-private/issues/1602

Checklist

  • Tests updated
  • No changelog updates since the changes are not user-facing

@vstpme vstpme added pr:no changelog pr:no public docs Added to a PR that does not require public documentation updates labels Mar 20, 2023
@vstpme vstpme marked this pull request as ready for review March 20, 2023 14:59
@vstpme vstpme requested review from a team March 20, 2023 14:59
@vstpme vstpme changed the title WIP: Mobile app settings backend Mobile app settings backend Mar 20, 2023
@vstpme vstpme requested review from imtoori and Dieterbe March 20, 2023 14:59
Copy link
Contributor

@matiasb matiasb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one minor comment.

engine/apps/mobile_app/tasks.py Outdated Show resolved Hide resolved
Copy link
Contributor

@joeyorlando joeyorlando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! I'm sure people will be pleased once this is released 😛

engine/apps/mobile_app/views.py Show resolved Hide resolved
engine/apps/mobile_app/tasks.py Outdated Show resolved Hide resolved
engine/apps/mobile_app/tasks.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you requested a review, i can say that it looks fine in principle, but obviously i didn' treview the python code.

@vstpme vstpme enabled auto-merge March 22, 2023 14:30
@vstpme vstpme added this pull request to the merge queue Mar 22, 2023
@vstpme vstpme merged commit ed5b5e1 into dev Mar 22, 2023
@vstpme vstpme deleted the vadimkerr/mobile-app-settings-backend branch March 22, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants