docs: [FC-0047] Push notifications ADR #277
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @NiedielnitsevIvan! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
NiedielnitsevIvan
changed the title
mphilbrick211
added
the
needs test run
|
@NiedielnitsevIvan I've run the tests, there are a few failures. I've also invited you to the triage team so that tests will run automatically for you in the future. |
mphilbrick211
removed
the
needs test run
| ------ | ||
|
|
||
| The goal of ACE framework is to provide extensible mechanism for extending delivery channels and policies. | ||
| However, as of May 2024, edx-ace supports various email delivery channelslike django and Sailthru as well as third party API integration for Braze, but lacks support for direct mobile push notifications. |
There was a problem hiding this comment.
| However, as of May 2024, edx-ace supports various email delivery channelslike django and Sailthru as well as third party API integration for Braze, but lacks support for direct mobile push notifications. | |
| However, as of May 2024, edx-ace supports various email delivery channels like django and Sailthru as well as third party API integration for Braze, but lacks support for direct mobile push notifications. |
|
|
||
| The goal of ACE framework is to provide extensible mechanism for extending delivery channels and policies. | ||
| However, as of May 2024, edx-ace supports various email delivery channelslike django and Sailthru as well as third party API integration for Braze, but lacks support for direct mobile push notifications. | ||
| Adding push notifications delivery channel willenable real-time communication with users through their mobile devices, enhancing userengagement and ensuring timely delivery of important information. |
There was a problem hiding this comment.
| Adding push notifications delivery channel willenable real-time communication with users through their mobile devices, enhancing userengagement and ensuring timely delivery of important information. | |
| Adding push notifications delivery channel will enable real-time communication with users through their mobile devices, enhancing user engagement and ensuring timely delivery of important information. |
| Data for push notifications must be formatted according to mobile push notification requirements. | ||
| It should be possible to add optional payloads for advanced functionality (e.g. data for notification actions). | ||
| Send formatted push notifications using the firebase_admin_ SDK. | ||
| Integration with django-push-notifications_ to simplify the process of sending notifications. |
There was a problem hiding this comment.
I assume that from the developer perspective, I interact with ACE. ACE's push notification channel uses django-push-notifications (DPN). DPN can be configured to use multiple delivery mechanisms to the phone, including firebase. Is that correct?
There was a problem hiding this comment.
In general, yes, DPN can be configured to use multiple delivery mechanisms to the phone, such as APNS, FCM(firebase), and WNS. But for now, we have chosen FCM because it can send notifications to both Android and IOS devices.
There was a problem hiding this comment.
Do you have an idea of what additional work would be needed to support the other delivery mechanisms? In the past some operators have not been willing or able to integrate with specific cloud providers, so where possible we prefer to not lock ourselves into one.
There was a problem hiding this comment.
@bmtcril Additionally, to what @NiedielnitsevIvan mentioned below (#277 (comment)), it is worth mentioning that a third-party channel for push notifications was introduced earlier by edX (see ADR). The Braze channel supports both emails and mobile push notifications, with all configurations managed through the Braze interface, requiring minimal changes on the edx-ace side. So operators would have several references on how to integrate with 3rd party providers.
Once again, all push notification services are just convenient wrappers around Firebase and Apple APNS, which are free and fully supported by Django push notifications. Therefore, in most cases, to integrate a new push notifications channel, I believe most of the utilities can be reused from the Firebase channel, and only the send method needs to be overridden.
| authenticated communication with the Firebase Cloud Messaging (FCM) service. | ||
|
|
||
| To create a new push notification, on edx-platform side the following steps are required: | ||
| - Create a new message type class that extends `BaseMessageType`, defining the |
There was a problem hiding this comment.
BaseMessageType is from where? Is the idea that we would have a new class for every push notification? For example: EnrollmentExpiringPushNotification, DeadlineApproachingPushNotificatin?
There was a problem hiding this comment.
The BaseMessageType from openedx.core.djangoapps.ace_common.message, which is currently used to create classes for emails. This will also allow you to use the existing classes EnrollEnrolled, AllowedEnroll, etc. to send push notifications by simply extending the context where necessary and creating new templates with notification body.
(I'v add this to the text)
| - Create a new message type class that extends `BaseMessageType`, defining the | ||
| message type and its associated renderer. | ||
| - Create new body.txt and subject.txt templates for the push notification content like how it is done for email. | ||
| Example path: `lms/templates/instructor/edx_ace/enrollenrolled/push/body.txt`. |
There was a problem hiding this comment.
What does the override the template story look like for users?
There was a problem hiding this comment.
I don't exactly understand what you mean.
Here, I meant that the paths of push notification templates will have the similar path as emails have now, only the channel name will change.
There was a problem hiding this comment.
What does the override the template story look like for users?
@e0d edx-ace templates are overrideable via the comprehensive theme. Similiar to how Footer, Header and base.html is overridden.
There was a problem hiding this comment.
Thanks @NiedielnitsevIvan. Few things that caught my attention:
- I have few formatting nits.
- A question regarding the specifics of how we handle the case where two types of notifications are needed.
- I'd like to learn more about what are expected Python configuration from platform operators besides getting a Firebase account.
- I'd like to learn about expected Tutor experience.
It should also be noted that django-push-notifications_ does not currently implement sending notifications to IOS devices using the FCM channel, although the FCM service itself supports it.
-
What are other iOS-specific gaps needed to be in-place in the platform. Shouldn't such support added to the
django-push-notificationspackager itself? -
Any additional i18n and translations implications? by default ace templates supports it so hopefully we'd keep it the same.
Just in case you missed it, there's an existing ACE design.rst document which is worth reviewing and updating if needed.
| Context | ||
| ------ |
There was a problem hiding this comment.
Please fix it so preview works in GitHub: https://github.com/raccoongang/edx-ace/blob/NiedielnitsevIvan/feat/Push-notifications-ADR/docs/decisions/0002-push-notifications.rst
| Context | |
| ------ | |
| Context | |
| ------- |
| Decision | ||
| ------ |
There was a problem hiding this comment.
| Decision | |
| ------ | |
| Decision | |
| -------- |
| Consequences | ||
| ------ |
There was a problem hiding this comment.
| Consequences | |
| ------ | |
| Consequences | |
| ------------ |
| - Create a new message type class that extends `BaseMessageType`, defining the | ||
| message type and its associated renderer. | ||
| - Create new body.txt and subject.txt templates for the push notification content like how it is done for email. | ||
| Example path: `lms/templates/instructor/edx_ace/enrollenrolled/push/body.txt`. |
There was a problem hiding this comment.
What does the override the template story look like for users?
@e0d edx-ace templates are overrideable via the comprehensive theme. Similiar to how Footer, Header and base.html is overridden.
| This means that support for IOS devices should be added on the edx-ace side. | ||
|
|
||
| This will involve: | ||
| - PushNotificationRenderer: Responsible for formatting and structuring the content |
| of a push notification. This includes setting the notification's title, body, | ||
| and optional data payload. The renderer will ensure that the notification content | ||
| adheres to the specifications required by the firebase_admin_ SDK. | ||
| - PushNotificationChannel: Handles sending formatted push notifications using |
| - Create a new message type class that extends existing `BaseMessageType` from | ||
| `openedx.core.djangoapps.ace_common.message`, defining the message type and its associated renderer. |
There was a problem hiding this comment.
I'm not sure if I understand this part of the proposal correctly in terms of its implication on notifications such as discussion notifications:
message_context = _build_message_context(context)
message = ResponseNotification().personalize(
Recipient(thread_author.id, thread_author.email),
_get_course_language(context['course_id']),
message_context
)
ace.send(message)Would would the code look like if we need to support both email and push notifications? Do we send two messages or the same ResponseNotification message would handle both push and email notifications?
There was a problem hiding this comment.
ace.send already supports sending via multiple channels. It turns out that in order to send both push and email notifications, you need to add a new channel push_notifications to ACE_ENABLED_CHANNELS. In this case, ace.send will send both push and email notifications, but it is worth noting that the necessary templates must also be present, otherwise we will get the TemplateDoesNotExist exception at the rendering stage.
I also suggest adding the ace.send_by_channel method to ace, which will be able to send notifications to the required channel. This will allow for more flexibility, for example, in cases with discussion notifications when we need to send a push for each new response, while an email is sent only to the first response.
In this case, the code will look like this:
message_context = _build_message_context(context, notification_type='forum_response')
message = ResponseNotification().personalize(
Recipient(thread_author.id, thread_author.email),
_get_course_language(context['course_id']),
message_context
)
log.info('Sending forum comment notification with context %s', message_context)
if _is_first_comment(context['comment_id'], context['thread_id']):
ace.send(message)
else:
ace.send_for_channel(message, 'push')
For the first response (as it is currently implemented), both notifications will be sent, and for all other replies, only push notifications will be sent.
The _build_message_context method should also be extended:
'push_notification_extra_context': {
'notification_type': 'notification_type',
'thread_id': context['thread_id'],
'comment_id': context['comment_id'],
}
The presence of push_notification_extra_context will be checked in the CoursePushNotificationOptout, and based on this, it will be determined whether the PushNotificationChannel is allowed.
There was a problem hiding this comment.
Thanks @NiedielnitsevIvan! Looks better.
I have few notes about the update. It looks good and now the proposal needs an update to remove the outdated suggestions regarding the new message type.
Could you please include your comment in the proposal document, so I can comment easier?
I'm thinking that ace.send(message, limit_to_channels=None) should be used instead of a new method. This way a message can only be sent to a channel if it's in channels_for_message and (limit_to_channels is None or chanel in limit_to_channels) to avoid developer errors.
https://github.com/openedx/edx-ace/blob/master/edx_ace/ace.py#L27-L58
I also will double check on the push_notification_extra_context dicutionary structure.
There was a problem hiding this comment.
Added this to the description, considering your suggestion.
| 6. Dependency on Firebase Cloud Messaging (FCM) service, which might introduce external service dependency risks. | ||
|
|
||
|
|
||
| It was decided not to expand the codebase unnecessarily and not to independently implement |
|
@OmarIthawi Thank you for your questions, other answers are below in the comments.
In addition to getting a Firebase account, you will need to add/update the following settings in edx-platform (most likely in edx-platform/openedx/core/djangoapps/ace_common/settings/):
And add
This works correctly with Tutor, but requires some small changes. Currently, the Tutor settings have a buggy value of
At the moment, we didn't plan to add this directly to the
That's right, there should be no consequences for i18n and translations, they will be supported and push notifications. |
Please add a section for all of the above in the proposal. ACE isn't too simple, and it's only maintained every once in a while |
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/feat/Push-notifications-ADR
branch
from
f4d01e1 to
d84e2d2
Compare
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/feat/Push-notifications-ADR
branch
from
d84e2d2 to
92940b4
Compare
NiedielnitsevIvan
force-pushed
the
NiedielnitsevIvan/feat/Push-notifications-ADR
branch
from
92940b4 to
563268d
Compare
|
Thanks @NiedielnitsevIvan, I see you've added the details. I'll review and get back to you. |
| It turns out that in order to send both push and email notifications, | ||
| you need to add a new channel `push_notifications` to the `ACE_ENABLED_CHANNELS`. | ||
| In this case, `ace.send` will send both push and email notifications, | ||
| but it is worth noting that the necessary templates must also be present, |
There was a problem hiding this comment.
I think this is probably too much technical detail for an ADR
There was a problem hiding this comment.
Okay, I just wanted to emphasize here once again the need to add new templates for correct works. But if you think it's too detailed, I'll remove it.
| django-push-notifications_ to manage user device tokens. | ||
| Provide secure and authenticated communication with Firebase Cloud Messaging (FCM). | ||
|
|
||
| It is proposed to add a new optional argument `limit_to_channels=None` to the |
There was a problem hiding this comment.
Would it make more sense to make this default to the existing channel instead of None? This would allow senders the option of opt-ing in to push notifications now instead of having them on by default for every message type, and also have the same effect if we add more channels later.
Or maybe I'm missing something and that handled at a policy level?
There was a problem hiding this comment.
User preferences for push notifications will be handled at the policy level. But here I have described the ability to send notifications via a specific channel. For example, an auto-enroll email for unregistered users, because push notifications are meaningless because BE doesn't know the device token for that user.
| Data for push notifications must be formatted according to mobile push notification requirements. | ||
| It should be possible to add optional payloads for advanced functionality (e.g. data for notification actions). | ||
| Send formatted push notifications using the firebase_admin_ SDK. | ||
| Integration with django-push-notifications_ to simplify the process of sending notifications. |
There was a problem hiding this comment.
Do you have an idea of what additional work would be needed to support the other delivery mechanisms? In the past some operators have not been willing or able to integrate with specific cloud providers, so where possible we prefer to not lock ourselves into one.
Additional channels using other services can be added in a similar way to what is proposed here. I suggest using the Firebase service because it covers both IOS and Android platforms at once, and is the standard for push notifications on Android. That's why most other services still use Firebase Cloud Messaging internally. |
Good afternoon, please let me know when you will be able to review the changes in this PR again. Thank you in advance! |
@NiedielnitsevIvan Sorry for the delays here. I needed to juggle the FC 55 closure among other projects. I will get to this tomorrow and have a review, then I'll take a couple of days off and get back to it on the 20th of June. |
|
While this is running afoul of the codecov fork rate limit issue, the coverage is rated 94% here which is fine. I don't see any problems merging it if other folks approve. |
|
@OmarIthawi @e0d any other thoughts on this ADR? Since we haven't had any input from the repo owners I will merge this Monday if no one objects. |
|
I have no objections so far a I owe another review anyway, which should happen before Monday. |
There was a problem hiding this comment.
Looks good to me. Thanks @NiedielnitsevIvan for your patience with the slow review and addressing the review notes.
Co-authored-by: Omar Al-Ithawi <[email protected]>
|
@NiedielnitsevIvan 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description:
This PR adresses to adding ADR for push notifications.
JIRA: FC-0047
Reviewers:
Merge checklist:
Post merge:
finished.