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

feat: [FC-0047] add functionality to send push notifications #282

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion edx_ace/ace.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,18 @@
)
ace.send(msg)
"""
import logging

from django.template import TemplateDoesNotExist

from edx_ace import delivery, policy, presentation
from edx_ace.channel import get_channel_for_message
from edx_ace.errors import ChannelError, UnsupportedChannelError

log = logging.getLogger(__name__)


def send(msg):
def send(msg, limit_to_channels=None):
"""
Send a message to a recipient.

Expand All @@ -37,19 +43,32 @@ def send(msg):

Args:
msg (Message): The message to send.
limit_to_channels (list of ChannelType, optional): If provided, only send the message over the specified
channels. If not provided, the message will be sent over all channels that the policies allow.
"""
msg.report_basics()

channels_for_message = policy.channels_for(msg)

for channel_type in channels_for_message:
if limit_to_channels and channel_type not in limit_to_channels:
log.info('Skipping channel %s', channel_type)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should lower the log in this case? I'm imagining this will spam logs due to the sheer amount of notifications being sent.

Suggested change
log.info('Skipping channel %s', channel_type)
log.debug('Skipping channel %s', channel_type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.


try:
channel = get_channel_for_message(channel_type, msg)
except UnsupportedChannelError:
continue

try:
rendered_message = presentation.render(channel, msg)
except TemplateDoesNotExist as error:
msg.report(
'template_error',
'Unable to send message because template not found\n' + str(error)
)
continue

try:
delivery.deliver(channel, rendered_message, msg)
except ChannelError as error:
msg.report(
Expand Down
37 changes: 20 additions & 17 deletions edx_ace/channel/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class ChannelMap:
"""
A class that represents a channel map, usually as described in Django settings and `setup.py` files.
"""

def __init__(self, channels_list):
"""
Initialize a ChannelMap.
Expand Down Expand Up @@ -170,27 +171,29 @@ def get_channel_for_message(channel_type, message):
Channel: The selected channel object.
"""
channels_map = channels()
channel_names = []

if channel_type == ChannelType.EMAIL:
if message.options.get('transactional'):
channel_names = [settings.ACE_CHANNEL_TRANSACTIONAL_EMAIL, settings.ACE_CHANNEL_DEFAULT_EMAIL]
else:
channel_names = [settings.ACE_CHANNEL_DEFAULT_EMAIL]

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

@NiedielnitsevIvan NiedielnitsevIvan Jul 23, 2024

Choose a reason for hiding this comment

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

Removed.

try:
possible_channels = [
channels_map.get_channel_by_name(channel_type, channel_name)
for channel_name in channel_names
]
except KeyError:
return channels_map.get_default_channel(channel_type)

# First see if any channel specifically demands to deliver this message
for channel in possible_channels:
if channel.overrides_delivery_for_message(message):
return channel

# Else the normal path: use the preferred channel for this message type
return possible_channels[0]

return channels_map.get_default_channel(channel_type)
if channel_type == ChannelType.PUSH:
Copy link
Member

Choose a reason for hiding this comment

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

Costmetic changes:

Suggested change
if channel_type == ChannelType.PUSH:
elif channel_type == ChannelType.PUSH:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

channel_names = [settings.ACE_CHANNEL_DEFAULT_PUSH]

try:
possible_channels = [
channels_map.get_channel_by_name(channel_type, channel_name)
for channel_name in channel_names
]
except KeyError:
return channels_map.get_default_channel(channel_type)

# First see if any channel specifically demands to deliver this message
for channel in possible_channels:
if channel.overrides_delivery_for_message(message):
return channel

# Else the normal path: use the preferred channel for this message type
return possible_channels[0] if possible_channels else channels_map.get_default_channel(channel_type)
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure if this is valid anymore to return a single channel for a message.

Would you mind elaborating on why this change was made?

Can we send the same message to both Email and Push?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the get_channel_for_message function, which returns only one channel for the received type and message, both in the old implementation and in the new one.

I reworked this to avoid duplicating code when adding a new channel.

This way, messages can be sent via both Email and Push channels at once.

Copy link
Member

Choose a reason for hiding this comment

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

@NiedielnitsevIvan I'm having a hard time understanding this function and its role after this change. I suppose we may need to have a get_channel[s]_for_message function instead.

I understand that it returns a single channel, but I don't think that makes sense anymore given that we send multi-channel messages.

Copy link
Member

@OmarIthawi OmarIthawi Jul 24, 2024

Choose a reason for hiding this comment

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

Minor style changes to improve readability.

Suggested change
return possible_channels[0] if possible_channels else channels_map.get_default_channel(channel_type)
if possible_channels:
return possible_channels[0]
else:
return channels_map.get_default_channel(channel_type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review, I have corrected the last comment, so the PR is ready to be merged and released for further use in this PR.

110 changes: 110 additions & 0 deletions edx_ace/channel/push_notification.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
"""
Channel for sending push notifications.
"""
import logging
import re

from firebase_admin.messaging import APNSConfig, APNSPayload, Aps, ApsAlert
from push_notifications.gcm import dict_to_fcm_message, send_message
from push_notifications.models import GCMDevice

from django.conf import settings

from edx_ace.channel import Channel, ChannelType
from edx_ace.errors import FatalChannelDeliveryError
from edx_ace.message import Message
from edx_ace.renderers import RenderedPushNotification

LOG = logging.getLogger(__name__)
APNS_DEFAULT_PRIORITY = '5'
APNS_DEFAULT_PUSH_TYPE = 'alert'


class PushNotificationChannel(Channel):
"""
A channel for sending push notifications.
"""

channel_type = ChannelType.PUSH

@classmethod
def enabled(cls):
"""
Returns true if the push notification settings are configured.
"""
return bool(getattr(settings, 'PUSH_NOTIFICATIONS_SETTINGS', None))

def deliver(self, message: Message, rendered_message: RenderedPushNotification) -> None:
"""
Transmit a rendered message to a recipient.

Args:
message: The message to transmit.
rendered_message: The rendered content of the message that has been personalized
for this particular recipient.
"""
device_tokens = self.get_user_device_tokens(message.recipient.lms_user_id)
if not device_tokens:
LOG.info(
'Recipient with ID %s has no push token. Skipping push notification.',
message.recipient.lms_user_id
)
return

for token in device_tokens:
self.send_message(message, token, rendered_message)

def send_message(self, message: Message, token: str, rendered_message: RenderedPushNotification) -> None:
"""
Send a push notification to a device by token.
"""
notification_data = {
'title': self.compress_spaces(rendered_message.title),
'body': self.compress_spaces(rendered_message.body),
'notification_key': token,
**message.context.get('push_notification_extra_context', {}),
}
message = dict_to_fcm_message(notification_data)
# Note: By default dict_to_fcm_message does not support APNS configuration,
# only Android configuration, so we need to collect and set it manually.
apns_config = self.collect_apns_config(notification_data)
message.apns = apns_config
try:
send_message(token, message, settings.FCM_APP_NAME)
except Exception as e:
LOG.exception('Failed to send push notification to %s', token)
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

raise FatalChannelDeliveryError(f'Failed to send push notification to {token}') from e

@staticmethod
def collect_apns_config(notification_data: dict) -> APNSConfig:
"""
Collect APNS configuration with payload for the push notification.

This APNSConfig must be set to notifications for Firebase to send push notifications to iOS devices.
Notification has default priority and visibility settings, described in Apple's documentation.
(https://developer.apple.com/documentation/usernotifications/sending-notification-requests-to-apns)
"""
apns_alert = ApsAlert(title=notification_data['title'], body=notification_data['body'])
aps = Aps(alert=apns_alert, sound='default')
return APNSConfig(
headers={'apns-priority': APNS_DEFAULT_PRIORITY, 'apns-push-type': APNS_DEFAULT_PUSH_TYPE},
payload=APNSPayload(aps)
)

@staticmethod
def get_user_device_tokens(user_id: int) -> list:
"""
Get the device tokens for a user.
"""
return list(GCMDevice.objects.filter(
user_id=user_id,
cloud_message_type='FCM',
active=True,
).values_list('registration_id', flat=True))

@staticmethod
def compress_spaces(html_str: str) -> str:
"""
Compress spaces and remove newlines to make it easier to author templates.
"""
return re.sub('\\s+', ' ', html_str, re.UNICODE).strip()
1 change: 1 addition & 0 deletions edx_ace/presentation.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

RENDERERS = {
ChannelType.EMAIL: renderers.EmailRenderer(),
ChannelType.PUSH: renderers.PushNotificationRenderer(),
}


Expand Down
1 change: 1 addition & 0 deletions edx_ace/push_notifications/views/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from push_notifications.api.rest_framework import GCMDeviceViewSet
18 changes: 18 additions & 0 deletions edx_ace/renderers.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,21 @@ class EmailRenderer(AbstractRenderer):
A renderer for :attr:`.ChannelType.EMAIL` channels.
"""
rendered_message_cls = RenderedEmail


@attr.s
class RenderedPushNotification:
"""
Encapsulates all values needed to send a :class:`.Message`
over an :attr:`.ChannelType.PUSH`.
"""

title = attr.ib()
body = attr.ib()


class PushNotificationRenderer(AbstractRenderer):
"""
A renderer for :attr:`.ChannelType.PUSH` channels.
"""
rendered_message_cls = RenderedPushNotification
1 change: 1 addition & 0 deletions edx_ace/tests/channel/test_channel_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def setUp(self):
},
ACE_CHANNEL_DEFAULT_EMAIL='braze_email',
ACE_CHANNEL_TRANSACTIONAL_EMAIL='file_email',
ACE_CHANNEL_DEFAULT_PUSH='push_notification',
)
def test_get_channel_for_message(self):
channel_map = ChannelMap([
Expand Down
Loading
Loading