feat: [FC-0047] add mobile push notifications functionality #34971

NiedielnitsevIvan · 2024-06-11T15:03:38Z

Description

This PR addressed to adding the ability to send push notifications to the OeX mobile app on IOS and Android, using edx-ace functionality and Firebase Cloud Messaging as a cloud provider.

Some push notifications are also added here directly, namely:

push notifications for user enroll;
push notifications for user unenroll;
push notifications for add course beta testers;
push notifications for remove course beta testers;
push notification event to discussions.

Useful information to include:

Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
"Developer", and "Operator".
Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
changes.

Supporting information

FC-0047
TBD

Testing instructions

Install edx-ace version 1.9.0.
Add FIREBASE_CREDENTIALS or FIREBASE_CREDENTIALS_PATH to the platform settings.
Add a new push_notification channel to ACE_ENABLED_CHANEL.
Log in to OeX.
Install the OeX mobile application on your device.
Add your device token to FCM Devices with a link to your user from the admin panel.
Enroll/unenroll the user(from step 7) in the “Instructor” tab to trigger the push notification event.
Make sure you receive push notifications on your device.

Deadline

"None"

Other information

Edx-ace push notifications ARD - docs: [FC-0047] Push notifications ADR edx-ace#277
This PR depends on Edx-ace PR which adds the necessary functionality.

openedx-webhooks · 2024-06-11T15:03:43Z

Thanks for the pull request, @NiedielnitsevIvan!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

NiedielnitsevIvan · 2024-06-13T15:08:11Z

To fix pipelines, it is necessary to merge this PR in edx-ace and release a new version so that this PR can rely on it.

OmarIthawi

Thanks for your contirbution.

I've added a review a long while ago, but for some reasons it's not showing up.

Please let me know whant do you think about the suggested change.

OmarIthawi · 2024-06-23T16:06:15Z

lms/djangoapps/discussion/tasks.py

+class CommentNotification(BaseMessageType):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.options['transactional'] = True


Transactioanl should be used for important emails like password reset and others. I think in this case, CommentNotification isn't transactional

Suggested change

class CommentNotification(BaseMessageType):

def __init__(self, *args, **kwargs):

super().__init__(*args, **kwargs)

self.options['transactional'] = True

class CommentNotification(BaseMessageType):

pass

Fixed, thanks.

OmarIthawi · 2024-06-23T16:09:03Z

lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/body.txt

+{% load i18n %}
+{% blocktrans trimmed %}{{ comment_username }} commented to {{ thread_title }}:{% endblocktrans %}
+{{ comment_body_text }}


Would the message reach the device with a new line before {{ comment_body_text }} or would it be converted to a space.

Will be converted to a space.

OmarIthawi · 2024-06-23T16:11:24Z

lms/djangoapps/discussion/templates/discussion/edx_ace/responsenotification/push/subject.txt

@@ -0,0 +1,2 @@
+{% load i18n %}


Please rename to title.txt

OmarIthawi · 2024-06-23T16:12:11Z

lms/djangoapps/discussion/tests/test_tasks.py

@@ -19,7 +19,7 @@
 import openedx.core.djangoapps.django_comment_common.comment_client as cc
 from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory
 from lms.djangoapps.discussion.signals.handlers import ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY
-from lms.djangoapps.discussion.tasks import _should_send_message, _track_notification_sent
+from lms.djangoapps.discussion.tasks import _is_first_comment, _should_send_message, _track_notification_sent


consider breaking into new lines

Suggested change

from lms.djangoapps.discussion.tasks import _is_first_comment, _should_send_message, _track_notification_sent

from lms.djangoapps.discussion.tasks import (

_is_first_comment,

_should_send_message,

_track_notification_sent,

)

OmarIthawi · 2024-06-23T16:13:19Z

lms/djangoapps/discussion/tests/test_tasks.py

-        assert actual_result is False
+
+        should_email_send = _is_first_comment(comment_dict['id'], thread['id'])
+        assert should_email_send is False


Suggested change

assert should_email_send is False

assert not should_email_send

OmarIthawi · 2024-06-23T16:14:34Z

lms/djangoapps/instructor/enrollment.py

+    if email_params:
+        email_params.update({


Consider using message_params instead to be less confusing.

OmarIthawi · 2024-06-23T16:16:41Z

lms/djangoapps/mobile_api/notifications/views.py

+from rest_framework import status
+from rest_framework.response import Response
+
+from edx_ace.push_notifications.views import GCMDeviceViewSet as GCMDeviceViewSetBase


Please provide a link to those classes, I couldn't find those in openedx/edx-ace#282

This class is imported from django-push-notifications into edx-ace so that there is no direct dependency, and in case of need, it can be changed in edx-ace without changing the implementation in the platform.

https://github.com/openedx/edx-ace/blob/master/edx_ace/push_notifications/views/__init__.py

OmarIthawi · 2024-06-23T16:16:57Z

lms/djangoapps/mobile_api/notifications/views.py

+
+
+@mobile_view()
+class GCMDeviceViewSet(GCMDeviceViewSetBase):


What's the security controls for this view?

Default for mobile_view decorator:

authentication_classes = ( JwtAuthentication, BearerAuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser ) permission_classes = (IsAuthenticated,)

OmarIthawi · 2024-08-11T18:54:42Z

@NiedielnitsevIvan @GlugovGrGlib I see you're still actively pushing changes to this branch. Thank you for your efforts!

Is there anything I can help with to push this work forward? I know I've delayed other pull requests so I want to double check that I'm not blocking this one.

GlugovGrGlib · 2024-08-11T21:25:19Z

Hello @OmarIthawi with this latest commits we were able to address your comments from previous CR run and make all CI checks to pass. This PR is ready for the yet another review, hopefully final CR pass from your side.

OmarIthawi

@NiedielnitsevIvan you've done a really good job by refactoring the email --> message.

Thank you so much. This is very helpful. The pull request looks good and frankly ready to be merged.

I've added many small notes please check them.

Push notificaiton messages review

I recommend merging this pull request just to avoid any more delays and conflicts.

However, with all due respect, I think there's a little effort went into reviewing the UX of the notification messages. It's alright we're all engineers and aren't masters in business communications.

Therefore I recommend after merging this pull request, having someone with a User Experience skills and/or product management to review the push notification messages to ensure it's up to the current standards and provides an adequate experience to the user on their phone (cc: @e0d).

OmarIthawi · 2024-08-29T23:08:24Z

lms/djangoapps/discussion/tasks.py

+class CommentNotification(BaseMessageType):
+    pass


Suggested change

class CommentNotification(BaseMessageType):

pass

class CommentNotification(BaseMessageType):

"""

Notify discussion participants of new comments.

"""

OmarIthawi · 2024-08-29T23:11:56Z

lms/djangoapps/discussion/templates/discussion/edx_ace/responsenotification/push/body.txt

@@ -0,0 +1,2 @@
+{% load i18n %}
+{% blocktrans trimmed %}{{ comment_username }} replied to {{ thread_title }}{% endblocktrans %}


So, we're not including any excerpt? Depending on how much mobile notifications limit we have in mobile phones.

I'd recommed to add the comment's first 200 characters or so:

Suggested change

{% blocktrans trimmed %}{{ comment_username }} replied to {{ thread_title }}{% endblocktrans %}

{% blocktrans trimmed %}{{ comment_username }} replied to {{ thread_title }}: {{ comment_body_excerpt }}{% endblocktrans %}

comment_body_excerpt would be either the whole comment or part of it ending with ....

In general, this was not in the text of the push notifications when they were approved, but if you insist, I added it.

OmarIthawi · 2024-08-29T23:13:45Z

lms/djangoapps/mobile_api/notifications/urls.py

+from .views import GCMDeviceViewSet
+
+
+CREATE_GCM_DEVICE = GCMDeviceViewSet.as_view({'post': 'create'})


Suggested change

CREATE_GCM_DEVICE = GCMDeviceViewSet.as_view({'post': 'create'})

create_gcm_device_post_view = GCMDeviceViewSet.as_view({'post': 'create'})

OmarIthawi · 2024-08-29T23:13:47Z

lms/djangoapps/mobile_api/notifications/urls.py

+CREATE_GCM_DEVICE = GCMDeviceViewSet.as_view({'post': 'create'})
+
+
+urlpatterns = [
+    path('create-token/', CREATE_GCM_DEVICE, name='gcmdevice-list'),


Suggested change

CREATE_GCM_DEVICE = GCMDeviceViewSet.as_view({'post': 'create'})

urlpatterns = [

path('create-token/', CREATE_GCM_DEVICE, name='gcmdevice-list'),

CREATE_GCM_DEVICE = GCMDeviceViewSet.as_view({'post': 'create'})

urlpatterns = [

path('create-token/', create_gcm_device_post_view, name='gcmdevice-list'),

OmarIthawi · 2024-08-29T23:17:52Z

lms/templates/instructor/edx_ace/allowedunenroll/push/body.txt

@@ -0,0 +1,5 @@
+{% load i18n %}
+{% autoescape off %}
+{% blocktrans %}Dear Student,{% endblocktrans %}


Formality isn't ideal in mobile in my opinion.

Suggested change

{% blocktrans %}Dear Student,{% endblocktrans %}

OmarIthawi · 2024-08-29T23:18:09Z

lms/templates/instructor/edx_ace/allowedenroll/push/body.txt

@@ -0,0 +1,5 @@
+{% load i18n %}
+{% autoescape off %}
+{% blocktrans %}Dear student,{% endblocktrans %}


Suggested change

{% blocktrans %}Dear student,{% endblocktrans %}

OmarIthawi · 2024-08-29T23:21:41Z

openedx/core/djangoapps/ace_common/utils.py

+    try:
+        import firebase_admin  # pylint: disable=import-outside-toplevel
+    except ImportError:
+        log.error('Could not import firebase_admin package.')
+        return
+


I think this is a core import that should be kept is a real import since the package is installed always in the requirements.

Suggested change

try:

import firebase_admin # pylint: disable=import-outside-toplevel

except ImportError:

log.error('Could not import firebase_admin package.')

return

import firebase_admin # pylint: disable=import-outside-toplevel

feat: [FC-0047] Add push notifications for user enroll feat: [FC-0047] Add push notifications for user unenroll feat: [FC-0047] Add push notifications for add course beta testers feat: [FC-0047] Add push notifications for remove course beta testers feat: [FC-0047] Add push notification event to discussions

OmarIthawi · 2024-09-07T15:01:38Z

@NiedielnitsevIvan thank you so much for going above and beyond. Please let me know when the tests are fixed so we can go ahead and merge it.

…t-push-notifications-chanel

…cations-chanel' of github.com:raccoongang/edx-platform into NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel

NiedielnitsevIvan · 2024-09-11T13:01:06Z

@NiedielnitsevIvan thank you so much for going above and beyond. Please let me know when the tests are fixed so we can go ahead and merge it.

@OmarIthawi Checks are fixed.

…t-push-notifications-chanel

OmarIthawi

Thanks a lot @NiedielnitsevIvan! Looks good to me since last time, now the tests are fixed I give it ✅

GlugovGrGlib · 2024-09-13T12:52:58Z

@bmtcril as this PR is finally approved, could you please check and merge this PR?

bmtcril · 2024-09-13T14:59:27Z

Thanks @GlugovGrGlib @NiedielnitsevIvan , I'm doing a last pass run through now. This seems to me to be a medium risk PR to land, so what I'd like to do is:

Notify 2U in the usual cc-edx-platform channel that this is coming and give them time to review / ask questions
Try to schedule time when one of you can be available to do the merge in case of any issues / reverts
We often hold off from doing these merges on a Friday, is there I time during 2U business hours on Monday I can suggest merging?

Thanks, this is looking great!

bmtcril

Tested LMS functionality locally, though I don't have the ability to actually trigger push notifications at this point I was able to confirm that things worked both with the new settings set and without them.

bmtcril · 2024-09-16T15:36:33Z

Merging on this is delayed while 2U works out issues on their side. Better to let the release queue clear than potentially get caught up in rollbacks and reverts while things get stable there. Discussion is happening in #cc-edx-platform if you'd like to follow along, but it seems likely it will take a day or two before we can merge. Sorry for the delay.

edx-pipeline-bot · 2024-09-20T15:55:44Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-09-20T16:17:09Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-09-20T16:17:09Z

2U Release Notice: This PR has been deployed to the edX production environment.

NiedielnitsevIvan requested a review from a team as a code owner June 11, 2024 15:03

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 11, 2024

NiedielnitsevIvan mentioned this pull request Jun 11, 2024

feat: [FC-0047] add functionality to send push notifications openedx/edx-ace#282

Merged

13 tasks

NiedielnitsevIvan changed the title ~~feat: [FC-0047] add settings for edx-ace push notifications~~ feat: [FC-0047] add mobile push notifications functionality Jun 11, 2024

NiedielnitsevIvan force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch 3 times, most recently from e9e1876 to b325343 Compare June 12, 2024 17:24

GlugovGrGlib requested review from OmarIthawi and bmtcril June 13, 2024 10:27

NiedielnitsevIvan marked this pull request as draft June 13, 2024 15:05

NiedielnitsevIvan force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from eef6da0 to b8c6acb Compare July 3, 2024 07:15

GlugovGrGlib mentioned this pull request Jul 8, 2024

docs: [AXM-425] add docs for configure mobile push notifications raccoongang/edx-platform#2589

Closed

NiedielnitsevIvan force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from a437349 to 2718724 Compare July 30, 2024 17:23

NiedielnitsevIvan marked this pull request as ready for review July 30, 2024 17:23

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Jul 30, 2024

NiedielnitsevIvan force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch 2 times, most recently from be2bc82 to 0d08485 Compare August 5, 2024 08:31

GlugovGrGlib force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from 0d08485 to 46082e8 Compare August 5, 2024 17:47

OmarIthawi reviewed Aug 5, 2024

View reviewed changes

NiedielnitsevIvan force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from 46082e8 to cdbdd8c Compare August 9, 2024 09:50

NiedielnitsevIvan requested a review from OmarIthawi August 9, 2024 09:52

GlugovGrGlib force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from 61c2aca to d88d880 Compare August 10, 2024 13:22

GlugovGrGlib force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from a1dcec7 to 480ccca Compare August 16, 2024 12:58

OmarIthawi approved these changes Aug 29, 2024

View reviewed changes

NiedielnitsevIvan and others added 2 commits September 5, 2024 15:29

refactor: [FC-0047] rename subject files to title

ce21022

NiedielnitsevIvan and others added 7 commits September 5, 2024 15:30

refactor: [FC-0047] fix review issues

9d0c456

refactor: change name of the policy to course push optout

69078f3

build: remove diff for requirements

c7235e3

fix: rename email to message params

8a70544

fix: fix linter warning

c70a6e7

fix: remove docs from app level

c733dc8

style: [FC-0047] fix code style issues

13e3024

NiedielnitsevIvan force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from 480ccca to a6c1ebf Compare September 6, 2024 14:41

NiedielnitsevIvan requested a review from OmarIthawi September 6, 2024 14:43

chore: [FC-0047] change push notifications texts

eef99a6

NiedielnitsevIvan force-pushed the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch from a6c1ebf to eef99a6 Compare September 6, 2024 14:45

NiedielnitsevIvan and others added 3 commits September 11, 2024 15:26

Merge branch 'master' into NiedielnitsevIvan/FC-0047/feature/implemen…

6a2fe8c

…t-push-notifications-chanel

style: [FC-0047] remove unnecessary pass

069a52a

Merge branch 'NiedielnitsevIvan/FC-0047/feature/implement-push-notifi…

c193d94

…cations-chanel' of github.com:raccoongang/edx-platform into NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel

Merge branch 'master' into NiedielnitsevIvan/FC-0047/feature/implemen…

5fe7a2e

…t-push-notifications-chanel

OmarIthawi approved these changes Sep 13, 2024

View reviewed changes

GlugovGrGlib assigned GlugovGrGlib and bmtcril and unassigned GlugovGrGlib Sep 13, 2024

bmtcril approved these changes Sep 13, 2024

View reviewed changes

bmtcril merged commit 471bdd2 into openedx:master Sep 20, 2024
49 checks passed

GlugovGrGlib deleted the NiedielnitsevIvan/FC-0047/feature/implement-push-notifications-chanel branch October 1, 2024 17:30

	assert should_email_send is False
	assert not should_email_send

		@@ -0,0 +1,2 @@
		{% load i18n %}
		{% blocktrans trimmed %}{{ comment_username }} replied to {{ thread_title }}{% endblocktrans %}

		from .views import GCMDeviceViewSet


		CREATE_GCM_DEVICE = GCMDeviceViewSet.as_view({'post': 'create'})

	CREATE_GCM_DEVICE = GCMDeviceViewSet.as_view({'post': 'create'})
	create_gcm_device_post_view = GCMDeviceViewSet.as_view({'post': 'create'})

feat: [FC-0047] add mobile push notifications functionality #34971

feat: [FC-0047] add mobile push notifications functionality #34971

Conversation

NiedielnitsevIvan commented Jun 11, 2024

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Jun 11, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

NiedielnitsevIvan commented Jun 13, 2024

OmarIthawi left a comment • 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

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

OmarIthawi commented Aug 11, 2024

GlugovGrGlib commented Aug 11, 2024 • edited Loading

OmarIthawi left a comment • edited Loading

Choose a reason for hiding this comment

Push notificaiton messages review

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi commented Sep 7, 2024

NiedielnitsevIvan commented Sep 11, 2024

OmarIthawi left a comment

Choose a reason for hiding this comment

GlugovGrGlib commented Sep 13, 2024

bmtcril commented Sep 13, 2024

bmtcril left a comment

Choose a reason for hiding this comment

bmtcril commented Sep 16, 2024

edx-pipeline-bot commented Sep 20, 2024

edx-pipeline-bot commented Sep 20, 2024

edx-pipeline-bot commented Sep 20, 2024

openedx-webhooks commented Jun 11, 2024 •

edited

Loading

OmarIthawi left a comment •

edited

Loading

GlugovGrGlib commented Aug 11, 2024 •

edited

Loading

OmarIthawi left a comment •

edited

Loading