From 4cfab2db8befde8f121555c445b010eabd7a640d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=86=D0=B2=D0=B0=D0=BD=20=D0=9D=D1=94=D0=B4=D1=94=D0=BB?= =?UTF-8?q?=D1=8C=D0=BD=D1=96=D1=86=D0=B5=D0=B2?= Date: Wed, 29 May 2024 15:21:18 +0300 Subject: [PATCH 1/3] feat: [AXM-556] refactor discussion push notifications sending --- lms/djangoapps/discussion/tasks.py | 64 ++++++++++++++++--- .../edx_ace/commentnotification/push/body.txt | 3 + .../commentnotification/push/subject.txt | 3 + .../djangoapps/ace_common/settings/common.py | 2 +- .../ace_common/settings/production.py | 2 +- 5 files changed, 62 insertions(+), 12 deletions(-) create mode 100644 lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/body.txt create mode 100644 lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/subject.txt diff --git a/lms/djangoapps/discussion/tasks.py b/lms/djangoapps/discussion/tasks.py index 7d19cbb9cd63..ce6baaa5873d 100644 --- a/lms/djangoapps/discussion/tasks.py +++ b/lms/djangoapps/discussion/tasks.py @@ -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 @@ -74,6 +75,12 @@ def __init__(self, *args, **kwargs): self.options['transactional'] = True +class CommentNotification(BaseMessageType): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.options['transactional'] = True + + @shared_task(base=LoggedTask) @set_code_owner_attribute def send_ace_message(context): # lint-amnesty, pylint: disable=missing-function-docstring @@ -82,17 +89,40 @@ 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 context['comment_author_id'] != context['thread_author_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) @set_code_owner_attribute @@ -153,8 +183,17 @@ def _should_send_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_not_subcomment(context['comment_id']) and - _is_first_comment(context['comment_id'], context['thread_id']) + _is_not_subcomment(context['comment_id']) + ) + + +def _should_send_subcomment_message(context): + cc_thread_author = cc.User(id=context['thread_author_id'], course_id=context['course_id']) + comment_author_is_thread_author = context['comment_author_id'] == context['thread_author_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 ) @@ -164,9 +203,13 @@ def _is_content_still_reported(context): 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 @@ -204,7 +247,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']) @@ -219,7 +262,8 @@ def _build_message_context(context): # lint-amnesty, pylint: disable=missing-fu 'comment_username': comment_author.username, 'post_link': post_link, 'push_notification_extra_context': { - 'notification_type': 'forum_comment', + 'notification_type': notification_type, + 'topic_id': context['thread_commentable_id'], 'thread_id': context['thread_id'], 'comment_id': context['comment_id'], }, diff --git a/lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/body.txt b/lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/body.txt new file mode 100644 index 000000000000..391e3d8ef4d7 --- /dev/null +++ b/lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/body.txt @@ -0,0 +1,3 @@ +{% load i18n %} +{% blocktrans trimmed %}{{ comment_username }} commented to {{ thread_title }}:{% endblocktrans %} +{{ comment_body_text }} diff --git a/lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/subject.txt b/lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/subject.txt new file mode 100644 index 000000000000..d2298a812990 --- /dev/null +++ b/lms/djangoapps/discussion/templates/discussion/edx_ace/commentnotification/push/subject.txt @@ -0,0 +1,3 @@ +{% load i18n %} + +{% blocktrans %}Comment to {{ thread_title }}{% endblocktrans %} diff --git a/openedx/core/djangoapps/ace_common/settings/common.py b/openedx/core/djangoapps/ace_common/settings/common.py index dd1d5b680763..9dd0962b2d3a 100644 --- a/openedx/core/djangoapps/ace_common/settings/common.py +++ b/openedx/core/djangoapps/ace_common/settings/common.py @@ -36,7 +36,7 @@ def plugin_settings(settings): # lint-amnesty, pylint: disable=missing-function if getattr(settings, 'FIREBASE_APP', None): settings.ACE_ENABLED_CHANNELS.append(settings.ACE_CHANNEL_DEFAULT_PUSH) - settings.ACE_ENABLED_POLICIES.append(settings.ACE_CHANNEL_DEFAULT_PUSH) + settings.ACE_ENABLED_POLICIES.append('bulk_push_notification_optout') settings.PUSH_NOTIFICATIONS_SETTINGS = { 'CONFIG': 'push_notifications.conf.AppConfig', diff --git a/openedx/core/djangoapps/ace_common/settings/production.py b/openedx/core/djangoapps/ace_common/settings/production.py index d3409e13f306..0d128850f536 100644 --- a/openedx/core/djangoapps/ace_common/settings/production.py +++ b/openedx/core/djangoapps/ace_common/settings/production.py @@ -33,7 +33,7 @@ def plugin_settings(settings): settings.FIREBASE_APP = setup_firebase_app(settings.FIREBASE_CREDENTIALS, settings.FCM_APP_NAME) if settings.FIREBASE_APP: settings.ACE_ENABLED_CHANNELS.append(settings.ACE_CHANNEL_DEFAULT_PUSH) - settings.ACE_ENABLED_POLICIES.append(settings.ACE_CHANNEL_DEFAULT_PUSH) + settings.ACE_ENABLED_POLICIES.append('bulk_push_notification_optout') settings.PUSH_NOTIFICATIONS_SETTINGS = { 'CONFIG': 'push_notifications.conf.AppConfig', From 036f9898a275f73d33731ddfcf13429ae48e65c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=86=D0=B2=D0=B0=D0=BD=20=D0=9D=D1=94=D0=B4=D1=94=D0=BB?= =?UTF-8?q?=D1=8C=D0=BD=D1=96=D1=86=D0=B5=D0=B2?= Date: Wed, 29 May 2024 15:27:46 +0300 Subject: [PATCH 2/3] fix: fix typo --- lms/templates/instructor/edx_ace/addbetatester/push/subject.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/instructor/edx_ace/addbetatester/push/subject.txt b/lms/templates/instructor/edx_ace/addbetatester/push/subject.txt index ca1838195f55..973411afd35f 100644 --- a/lms/templates/instructor/edx_ace/addbetatester/push/subject.txt +++ b/lms/templates/instructor/edx_ace/addbetatester/push/subject.txt @@ -1,4 +1,4 @@ {% load i18n %} {% autoescape off %} -{% blocktrans %}You have been invited to a betca test for {{ course_name }}{% endblocktrans %} +{% blocktrans %}You have been invited to a beta test for {{ course_name }}{% endblocktrans %} {% endautoescape %} From b8639e377a4a7753bb3f67b68ee6515a48c40c4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=86=D0=B2=D0=B0=D0=BD=20=D0=9D=D1=94=D0=B4=D1=94=D0=BB?= =?UTF-8?q?=D1=8C=D0=BD=D1=96=D1=86=D0=B5=D0=B2?= Date: Wed, 29 May 2024 16:06:54 +0300 Subject: [PATCH 3/3] test: [AXM-556] add topic_id to tests --- lms/djangoapps/discussion/tasks.py | 4 ++-- lms/djangoapps/discussion/tests/test_tasks.py | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/discussion/tasks.py b/lms/djangoapps/discussion/tasks.py index ce6baaa5873d..23af89888134 100644 --- a/lms/djangoapps/discussion/tasks.py +++ b/lms/djangoapps/discussion/tasks.py @@ -98,7 +98,7 @@ def send_ace_message(context): # lint-amnesty, pylint: disable=missing-function message_context ) log.info('Sending forum comment notification with context %s', message_context) - if context['comment_author_id'] != context['thread_author_id']: + if _is_first_comment(context['comment_id'], context['thread_id']): limit_to_channels = None else: limit_to_channels = [ChannelType.PUSH] @@ -263,7 +263,7 @@ def _build_message_context(context, notification_type='forum_comment'): # lint- 'post_link': post_link, 'push_notification_extra_context': { 'notification_type': notification_type, - 'topic_id': context['thread_commentable_id'], + 'topic_id': context.get('thread_commentable_id', ''), 'thread_id': context['thread_id'], 'comment_id': context['comment_id'], }, diff --git a/lms/djangoapps/discussion/tests/test_tasks.py b/lms/djangoapps/discussion/tests/test_tasks.py index d3d3edb1f9e2..be9d2e994b70 100644 --- a/lms/djangoapps/discussion/tests/test_tasks.py +++ b/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 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 @@ -222,6 +222,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) @@ -231,10 +233,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 @@ -286,7 +290,8 @@ def test_send_discussion_email_notification(self, user_subscribed): 'site': site, 'site_id': site.id, 'push_notification_extra_context': { - 'notification_type': 'forum_comment', + 'notification_type': 'forum_response', + 'topic_id': thread['commentable_id'], 'thread_id': thread['id'], 'comment_id': comment['id'], }, @@ -332,7 +337,12 @@ 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 + # from edx_ace.channel import ChannelType + # import pdb; pdb.set_trace() + should_email_send = _is_first_comment(comment_dict['id'], thread['id']) + assert should_email_send is False + + # self.mock_ace_send.assert_called_once_with(self.mock_message, [ChannelType.PUSH]) assert not self.mock_ace_send.called def test_subcomment_should_not_send_email(self):