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 mobile push notifications functionality #34971

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions lms/djangoapps/ccx/api/v0/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -730,8 +730,8 @@ def make_ccx(self, max_students_allowed=200):
course_id=ccx_course_key,
student_email=self.coach.email,
auto_enroll=True,
email_students=False,
email_params=email_params,
message_students=False,
message_params=email_params,
)
return ccx

Expand Down
8 changes: 4 additions & 4 deletions lms/djangoapps/ccx/api/v0/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -505,8 +505,8 @@ def post(self, request):
course_id=ccx_course_key,
student_email=coach.email,
auto_enroll=True,
email_students=True,
email_params=email_params,
message_students=True,
message_params=email_params,
)
# assign staff role for the coach to the newly created ccx
assign_staff_role_to_ccx(ccx_course_key, coach, master_course_object.id)
Expand Down Expand Up @@ -768,8 +768,8 @@ def patch(self, request, ccx_course_id=None):
course_id=ccx_course_key,
student_email=coach.email,
auto_enroll=True,
email_students=True,
email_params=email_params,
message_students=True,
message_params=email_params,
)
# make the new coach staff on the CCX
assign_staff_role_to_ccx(ccx_course_key, coach, master_course_object.id)
Expand Down
26 changes: 16 additions & 10 deletions lms/djangoapps/ccx/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,13 @@ def ccx_students_enrolling_center(action, identifiers, email_students, course_ke
log.info("%s", error)
errors.append(error)
break
enroll_email(course_key, email, auto_enroll=True, email_students=email_students, email_params=email_params)
enroll_email(
course_key,
email,
auto_enroll=True,
message_students=email_students,
message_params=email_params
)
elif action == 'Unenroll' or action == 'revoke': # lint-amnesty, pylint: disable=consider-using-in
for identifier in identifiers:
try:
Expand All @@ -278,7 +284,7 @@ def ccx_students_enrolling_center(action, identifiers, email_students, course_ke
log.info("%s", exp)
errors.append(f"{exp}")
continue
unenroll_email(course_key, email, email_students=email_students, email_params=email_params)
unenroll_email(course_key, email, message_students=email_students, message_params=email_params)
return errors


Expand Down Expand Up @@ -348,8 +354,8 @@ def add_master_course_staff_to_ccx(master_course, ccx_key, display_name, send_em
course_id=ccx_key,
student_email=staff.email,
auto_enroll=True,
email_students=send_email,
email_params=email_params,
message_students=send_email,
message_params=email_params,
)

# allow 'staff' access on ccx to staff of master course
Expand All @@ -373,8 +379,8 @@ def add_master_course_staff_to_ccx(master_course, ccx_key, display_name, send_em
course_id=ccx_key,
student_email=instructor.email,
auto_enroll=True,
email_students=send_email,
email_params=email_params,
message_students=send_email,
message_params=email_params,
)

# allow 'instructor' access on ccx to instructor of master course
Expand Down Expand Up @@ -417,8 +423,8 @@ def remove_master_course_staff_from_ccx(master_course, ccx_key, display_name, se
unenroll_email(
course_id=ccx_key,
student_email=staff.email,
email_students=send_email,
email_params=email_params,
message_students=send_email,
message_params=email_params,
)

for instructor in list_instructor:
Expand All @@ -430,6 +436,6 @@ def remove_master_course_staff_from_ccx(master_course, ccx_key, display_name, se
unenroll_email(
course_id=ccx_key,
student_email=instructor.email,
email_students=send_email,
email_params=email_params,
message_students=send_email,
message_params=email_params,
)
4 changes: 2 additions & 2 deletions lms/djangoapps/ccx/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,8 @@ def create_ccx(request, course, ccx=None):
course_id=ccx_id,
student_email=request.user.email,
auto_enroll=True,
email_students=True,
email_params=email_params,
message_students=True,
message_params=email_params,
)

assign_staff_role_to_ccx(ccx_id, request.user, course.id)
Expand Down
2 changes: 2 additions & 0 deletions lms/djangoapps/discussion/signals/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,10 @@ def create_message_context(comment, site):
'course_id': str(thread.course_id),
'comment_id': comment.id,
'comment_body': comment.body,
'comment_body_text': comment.body_text,
'comment_author_id': comment.user_id,
'comment_created_at': comment.created_at, # comment_client models dates are already serialized
'comment_parent_id': comment.parent_id,
'thread_id': thread.id,
'thread_title': thread.title,
'thread_author_id': thread.user_id,
Expand Down
71 changes: 63 additions & 8 deletions lms/djangoapps/discussion/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user
from django.contrib.sites.models import Site
from edx_ace import ace
from edx_ace.channel import ChannelType
from edx_ace.recipient import Recipient
from edx_ace.utils import date
from edx_django_utils.monitoring import set_code_owner_attribute
Expand Down Expand Up @@ -74,6 +75,12 @@ def __init__(self, *args, **kwargs):
self.options['transactional'] = True


class CommentNotification(BaseMessageType):
"""
Notify discussion participants of new comments.
"""


@shared_task(base=LoggedTask)
@set_code_owner_attribute
def send_ace_message(context): # lint-amnesty, pylint: disable=missing-function-docstring
Expand All @@ -82,16 +89,39 @@ def send_ace_message(context): # lint-amnesty, pylint: disable=missing-function
if _should_send_message(context):
context['site'] = Site.objects.get(id=context['site_id'])
thread_author = User.objects.get(id=context['thread_author_id'])
with emulate_http_request(site=context['site'], user=thread_author):
message_context = _build_message_context(context)
comment_author = User.objects.get(id=context['comment_author_id'])
with emulate_http_request(site=context['site'], user=comment_author):
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 email notification with context %s', message_context)
ace.send(message)
log.info('Sending forum comment notification with context %s', message_context)
if _is_first_comment(context['comment_id'], context['thread_id']):
limit_to_channels = None
else:
limit_to_channels = [ChannelType.PUSH]
ace.send(message, limit_to_channels=limit_to_channels)
_track_notification_sent(message, context)

elif _should_send_subcomment_message(context):
context['site'] = Site.objects.get(id=context['site_id'])
comment_author = User.objects.get(id=context['comment_author_id'])
thread_author = User.objects.get(id=context['thread_author_id'])

with emulate_http_request(site=context['site'], user=comment_author):
message_context = _build_message_context(context)
message = CommentNotification().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)
ace.send(message, limit_to_channels=[ChannelType.PUSH])
_track_notification_sent(message, context)
else:
return


@shared_task(base=LoggedTask)
Expand Down Expand Up @@ -154,19 +184,36 @@ def _should_send_message(context):
return (
_is_user_subscribed_to_thread(cc_thread_author, context['thread_id']) and
_is_not_subcomment(context['comment_id']) and
_is_first_comment(context['comment_id'], context['thread_id'])
not _comment_author_is_thread_author(context)
)


def _should_send_subcomment_message(context):
cc_thread_author = cc.User(id=context['thread_author_id'], course_id=context['course_id'])
return (
_is_user_subscribed_to_thread(cc_thread_author, context['thread_id']) and
_is_subcomment(context['comment_id']) and
not _comment_author_is_thread_author(context)
)


def _comment_author_is_thread_author(context):
return context.get('comment_author_id', '') == context['thread_author_id']


def _is_content_still_reported(context):
if context.get('comment_id') is not None:
return len(cc.Comment.find(context['comment_id']).abuse_flaggers) > 0
return len(cc.Thread.find(context['thread_id']).abuse_flaggers) > 0


def _is_not_subcomment(comment_id):
def _is_subcomment(comment_id):
comment = cc.Comment.find(id=comment_id).retrieve()
return not getattr(comment, 'parent_id', None)
return getattr(comment, 'parent_id', None)


def _is_not_subcomment(comment_id):
return not _is_subcomment(comment_id)


def _is_first_comment(comment_id, thread_id): # lint-amnesty, pylint: disable=missing-function-docstring
Expand Down Expand Up @@ -204,7 +251,7 @@ def _get_course_language(course_id):
return language


def _build_message_context(context): # lint-amnesty, pylint: disable=missing-function-docstring
def _build_message_context(context, notification_type='forum_comment'): # lint-amnesty, pylint: disable=missing-function-docstring
message_context = get_base_template_context(context['site'])
message_context.update(context)
thread_author = User.objects.get(id=context['thread_author_id'])
Expand All @@ -218,6 +265,14 @@ def _build_message_context(context): # lint-amnesty, pylint: disable=missing-fu
'thread_username': thread_author.username,
'comment_username': comment_author.username,
'post_link': post_link,
'push_notification_extra_context': {
'course_id': str(context['course_id']),
'parent_id': str(context['comment_parent_id']),
'notification_type': notification_type,
'topic_id': str(context['thread_commentable_id']),
'thread_id': context['thread_id'],
'comment_id': context['comment_id'],
},
'comment_created_at': date.deserialize(context['comment_created_at']),
'thread_created_at': date.deserialize(context['thread_created_at'])
})
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{% load i18n %}
{% blocktrans trimmed %}{{ comment_username }} commented to {{ thread_title }}:{% endblocktrans %}
{{ comment_body_text }}
Comment on lines +1 to +3
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be converted to a space.

Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{% load i18n %}
{% blocktrans %}Comment to {{ thread_title }}{% endblocktrans %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{% load i18n %}
{% blocktrans trimmed %}{{ comment_username }} replied to {{ thread_title }}: {{ comment_body|truncatechars:200 }}{% endblocktrans %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{% load i18n %}
{% blocktrans %}Response to {{ thread_title }}{% endblocktrans %}
26 changes: 23 additions & 3 deletions lms/djangoapps/discussion/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
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,
)
from openedx.core.djangoapps.ace_common.template_context import get_base_template_context
from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
from openedx.core.djangoapps.django_comment_common.models import ForumsConfig
Expand Down Expand Up @@ -222,6 +226,8 @@ def setUp(self):

self.ace_send_patcher = mock.patch('edx_ace.ace.send')
self.mock_ace_send = self.ace_send_patcher.start()
self.mock_message_patcher = mock.patch('lms.djangoapps.discussion.tasks.ResponseNotification')
self.mock_message = self.mock_message_patcher.start()

thread_permalink = '/courses/discussion/dummy_discussion_id'
self.permalink_patcher = mock.patch('lms.djangoapps.discussion.tasks.permalink', return_value=thread_permalink)
Expand All @@ -231,10 +237,12 @@ def tearDown(self):
super().tearDown()
self.request_patcher.stop()
self.ace_send_patcher.stop()
self.mock_message_patcher.stop()
self.permalink_patcher.stop()

@ddt.data(True, False)
def test_send_discussion_email_notification(self, user_subscribed):
self.mock_message_patcher.stop()
if user_subscribed:
non_matching_id = 'not-a-match'
# with per_page left with a default value of 1, this ensures
Expand Down Expand Up @@ -271,8 +279,10 @@ def test_send_discussion_email_notification(self, user_subscribed):
expected_message_context.update({
'comment_author_id': self.comment_author.id,
'comment_body': comment['body'],
'comment_body_text': comment.body_text,
'comment_created_at': ONE_HOUR_AGO,
'comment_id': comment['id'],
'comment_parent_id': comment['parent_id'],
'comment_username': self.comment_author.username,
'course_id': self.course.id,
'thread_author_id': self.thread_author.id,
Expand All @@ -283,7 +293,15 @@ def test_send_discussion_email_notification(self, user_subscribed):
'thread_commentable_id': thread['commentable_id'],
'post_link': f'https://{site.domain}{self.mock_permalink.return_value}',
'site': site,
'site_id': site.id
'site_id': site.id,
'push_notification_extra_context': {
'notification_type': 'forum_response',
'topic_id': thread['commentable_id'],
'course_id': comment['course_id'],
'parent_id': str(comment['parent_id']),
'thread_id': thread['id'],
'comment_id': comment['id'],
},
})
expected_recipient = Recipient(self.thread_author.id, self.thread_author.email)
actual_message = self.mock_ace_send.call_args_list[0][0][0]
Expand Down Expand Up @@ -326,7 +344,9 @@ def run_should_not_send_email_test(self, thread, comment_dict):
'comment_id': comment_dict['id'],
'thread_id': thread['id'],
})
assert actual_result is False

should_email_send = _is_first_comment(comment_dict['id'], thread['id'])
assert not should_email_send
assert not self.mock_ace_send.called

def test_subcomment_should_not_send_email(self):
Expand Down
4 changes: 2 additions & 2 deletions lms/djangoapps/instructor/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ def _change_access(course, user, level, action, send_email=True):
course_id=course.id,
student_email=user.email,
auto_enroll=True,
email_students=send_email,
email_params=email_params,
message_students=send_email,
message_params=email_params,
)
role.add_users(user)
elif action == 'revoke':
Expand Down
Loading
Loading