diff --git a/lms/djangoapps/discussion/config/waffle.py b/lms/djangoapps/discussion/config/waffle.py index f864425431..2f542d1389 100644 --- a/lms/djangoapps/discussion/config/waffle.py +++ b/lms/djangoapps/discussion/config/waffle.py @@ -2,7 +2,7 @@ This module contains various configuration settings via waffle switches for the Discussions app. """ -from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace +from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag, WaffleFlagNamespace, WaffleSwitchNamespace # Namespace WAFFLE_NAMESPACE = u'discussions' @@ -10,6 +10,12 @@ WAFFLE_NAMESPACE = u'discussions' # Switches FORUM_RESPONSE_NOTIFICATIONS = u'forum_response_notifications' +SEND_NOTIFICATIONS_FOR_COURSE = CourseWaffleFlag( + waffle_namespace=WaffleFlagNamespace(name=WAFFLE_NAMESPACE), + flag_name=u'send_notifications_for_course', + flag_undefined_default=False +) + def waffle(): """ diff --git a/lms/djangoapps/discussion/signals/handlers.py b/lms/djangoapps/discussion/signals/handlers.py index ffa68a401a..99b6b70a78 100644 --- a/lms/djangoapps/discussion/signals/handlers.py +++ b/lms/djangoapps/discussion/signals/handlers.py @@ -3,12 +3,13 @@ Signal handlers related to discussions. """ import logging -from django.contrib.sites.models import Site from django.dispatch import receiver +from opaque_keys.edx.keys import CourseKey from django_comment_common import signals -from lms.djangoapps.discussion.config.waffle import waffle, FORUM_RESPONSE_NOTIFICATIONS +from lms.djangoapps.discussion.config.waffle import waffle, FORUM_RESPONSE_NOTIFICATIONS, SEND_NOTIFICATIONS_FOR_COURSE from lms.djangoapps.discussion import tasks +from openedx.core.djangoapps.theming.helpers import get_current_site log = logging.getLogger(__name__) @@ -16,11 +17,23 @@ log = logging.getLogger(__name__) @receiver(signals.comment_created) def send_discussion_email_notification(sender, user, post, **kwargs): - if waffle().is_enabled(FORUM_RESPONSE_NOTIFICATIONS): - send_message(post) + if not waffle().is_enabled(FORUM_RESPONSE_NOTIFICATIONS): + log.debug('Discussion: Response notifications waffle switch not enabled') + return + + if not SEND_NOTIFICATIONS_FOR_COURSE.is_enabled(CourseKey.from_string(post.thread.course_id)): + log.debug('Discussion: Response notifications not enabled for this course') + return + + current_site = get_current_site() + if current_site is None: + log.debug('Discussion: No current site, not sending notification') + return + + send_message(post, current_site) -def send_message(comment): +def send_message(comment, site): thread = comment.thread context = { 'course_id': unicode(thread.course_id), @@ -28,13 +41,13 @@ def send_message(comment): 'comment_body': comment.body, 'comment_author_id': comment.user_id, 'comment_username': comment.username, - 'comment_created_at': comment.created_at, - 'site_id': Site.objects.get_current().id, + 'comment_created_at': comment.created_at, # comment_client models dates are already serialized 'thread_id': thread.id, 'thread_title': thread.title, 'thread_username': thread.username, 'thread_author_id': thread.user_id, - 'thread_created_at': thread.created_at, + 'thread_created_at': thread.created_at, # comment_client models dates are already serialized 'thread_commentable_id': thread.commentable_id, + 'site_id': site.id } tasks.send_ace_message.apply_async(args=[context]) diff --git a/lms/djangoapps/discussion/tasks.py b/lms/djangoapps/discussion/tasks.py index 43852d3a86..13c61d569e 100644 --- a/lms/djangoapps/discussion/tasks.py +++ b/lms/djangoapps/discussion/tasks.py @@ -13,13 +13,13 @@ from django.contrib.sites.models import Site from celery_utils.logged_task import LoggedTask from edx_ace import ace +from edx_ace.utils import date from edx_ace.message import MessageType from edx_ace.recipient import Recipient from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.site_configuration.helpers import get_value from lms.djangoapps.django_comment_client.utils import permalink import lms.lib.comment_client as cc -from lms.lib.comment_client.utils import merge_dict from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.schedules.template_context import get_base_template_context @@ -56,7 +56,22 @@ def send_ace_message(context): 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']) + 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']) + ) + + +def _is_not_subcomment(comment_id): + comment = cc.Comment.find(id=comment_id).retrieve() + return not getattr(comment, 'parent_id', None) + + +def _is_first_comment(comment_id, thread_id): + thread = cc.Thread.find(id=thread_id).retrieve(with_responses=True) + first_comment = thread.children[0] + return first_comment.get('id') == comment_id def _is_user_subscribed_to_thread(cc_user, thread_id): @@ -78,13 +93,19 @@ def _get_course_language(course_id): def _build_message_context(context): - message_context = get_base_template_context(context['site']) - message_context.update(context) + message_context = get_base_template_context(Site.objects.get(id=context['site_id'])) + message_context.update(_deserialize_context_dates(context)) message_context['post_link'] = _get_thread_url(context) message_context['ga_tracking_pixel_url'] = _generate_ga_pixel_url(context) return message_context +def _deserialize_context_dates(context): + context['comment_created_at'] = date.deserialize(context['comment_created_at']) + context['thread_created_at'] = date.deserialize(context['thread_created_at']) + return context + + def _get_thread_url(context): thread_content = { 'type': 'thread', diff --git a/lms/djangoapps/discussion/tests/test_signals.py b/lms/djangoapps/discussion/tests/test_signals.py index ed549feacb..7b94ae31f4 100644 --- a/lms/djangoapps/discussion/tests/test_signals.py +++ b/lms/djangoapps/discussion/tests/test_signals.py @@ -1,29 +1,49 @@ from django.test import TestCase import mock +from opaque_keys.edx.keys import CourseKey + from django_comment_common import signals -from lms.djangoapps.discussion.config.waffle import waffle, FORUM_RESPONSE_NOTIFICATIONS +from lms.djangoapps.discussion.config.waffle import waffle, FORUM_RESPONSE_NOTIFICATIONS, SEND_NOTIFICATIONS_FOR_COURSE +from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag class SendMessageHandlerTestCase(TestCase): + def setUp(self): + self.sender = mock.Mock() + self.user = mock.Mock() + self.post = mock.Mock() + self.post.thread.course_id = 'course-v1:edX+DemoX+Demo_Course' + + @mock.patch('lms.djangoapps.discussion.signals.handlers.get_current_site') @mock.patch('lms.djangoapps.discussion.signals.handlers.send_message') - def test_comment_created_signal_sends_message(self, mock_send_message): + @override_waffle_flag(SEND_NOTIFICATIONS_FOR_COURSE, True) + def test_comment_created_signal_sends_message(self, mock_send_message, mock_get_current_site): with waffle().override(FORUM_RESPONSE_NOTIFICATIONS): - sender = mock.Mock() - user = mock.Mock() - post = mock.Mock() + signals.comment_created.send(sender=self.sender, user=self.user, post=self.post) - signals.comment_created.send(sender=sender, user=user, post=post) - - mock_send_message.assert_called_once_with(post) + mock_send_message.assert_called_once_with(self.post, mock_get_current_site.return_value) @mock.patch('lms.djangoapps.discussion.signals.handlers.send_message') + @override_waffle_flag(SEND_NOTIFICATIONS_FOR_COURSE, True) def test_comment_created_signal_message_not_sent_without_waffle_switch(self, mock_send_message): with waffle().override(FORUM_RESPONSE_NOTIFICATIONS, active=False): - sender = mock.Mock() - user = mock.Mock() - post = mock.Mock() - - signals.comment_created.send(sender=sender, user=user, post=post) + signals.comment_created.send(sender=self.sender, user=self.user, post=self.post) + + self.assertFalse(mock_send_message.called) + + @mock.patch('lms.djangoapps.discussion.signals.handlers.send_message') + def test_comment_created_signal_message_not_sent_without_course_waffle_flag(self, mock_send_message): + with waffle().override(FORUM_RESPONSE_NOTIFICATIONS, active=True): + signals.comment_created.send(sender=self.sender, user=self.user, post=self.post) + + self.assertFalse(mock_send_message.called) + + @mock.patch('lms.djangoapps.discussion.signals.handlers.get_current_site', return_value=None) + @mock.patch('lms.djangoapps.discussion.signals.handlers.send_message') + @override_waffle_flag(SEND_NOTIFICATIONS_FOR_COURSE, True) + def test_comment_created_signal_message_not_sent_without_site(self, mock_send_message, mock_get_current_site): + with waffle().override(FORUM_RESPONSE_NOTIFICATIONS, active=True): + signals.comment_created.send(sender=self.sender, user=self.user, post=self.post) self.assertFalse(mock_send_message.called) diff --git a/lms/djangoapps/discussion/tests/test_tasks.py b/lms/djangoapps/discussion/tests/test_tasks.py index a28eb30821..e5ca35cf41 100644 --- a/lms/djangoapps/discussion/tests/test_tasks.py +++ b/lms/djangoapps/discussion/tests/test_tasks.py @@ -15,142 +15,226 @@ import mock from django_comment_common.models import ForumsConfig from django_comment_common.signals import comment_created from edx_ace.recipient import Recipient -from lms.djangoapps.discussion.config.waffle import waffle, FORUM_RESPONSE_NOTIFICATIONS -from lms.djangoapps.discussion.tasks import _generate_ga_pixel_url +from edx_ace.utils import date +from lms.djangoapps.discussion.config.waffle import waffle, FORUM_RESPONSE_NOTIFICATIONS, SEND_NOTIFICATIONS_FOR_COURSE +from lms.djangoapps.discussion.tasks import _should_send_message, _generate_ga_pixel_url +import lms.lib.comment_client as cc from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.schedules.template_context import get_base_template_context +from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase - -@contextmanager -def mock_the_things(): - thread_permalink = '/courses/discussion/dummy_discussion_id' - with mock.patch('requests.request') as mock_request, mock.patch('edx_ace.ace.send') as mock_ace_send: - with mock.patch('lms.djangoapps.discussion.tasks.permalink', return_value=thread_permalink) as mock_permalink: - with mock.patch('lms.djangoapps.discussion.tasks.cc.Thread'): - yield (mock_request, mock_ace_send, mock_permalink) +NOW = datetime.utcnow() +ONE_HOUR_AGO = NOW - timedelta(hours=1) +TWO_HOURS_AGO = NOW - timedelta(hours=2) -def make_mock_responder(thread_ids, per_page=1): - collection = [ - {'id': thread_id} for thread_id in thread_ids - ] - - def mock_response(*args, **kwargs): +def make_mock_responder(subscribed_thread_ids=None, thread_data=None, comment_data=None, per_page=1): + def mock_subscribed_threads(method, url, **kwargs): + subscribed_thread_collection = [ + {'id': thread_id} for thread_id in subscribed_thread_ids + ] page = kwargs.get('params', {}).get('page', 1) start_index = per_page * (page - 1) end_index = per_page * page data = { - 'collection': collection[start_index: end_index], + 'collection': subscribed_thread_collection[start_index: end_index], 'page': page, - 'num_pages': int(math.ceil(len(collection) / float(per_page))), - 'thread_count': len(collection) + 'num_pages': int(math.ceil(len(subscribed_thread_collection) / float(per_page))), + 'thread_count': len(subscribed_thread_collection) } return mock.Mock(status_code=200, text=json.dumps(data), json=mock.Mock(return_value=data)) - return mock_response + + def mock_comment_find(method, url, **kwargs): + return mock.Mock(status_code=200, text=json.dumps(comment_data), json=mock.Mock(return_value=comment_data)) + + def mock_thread_find(method, url, **kwargs): + return mock.Mock(status_code=200, text=json.dumps(thread_data), json=mock.Mock(return_value=thread_data)) + + def mock_request(method, url, **kwargs): + if '/subscribed_threads' in url: + return mock_subscribed_threads(method, url, **kwargs) + if '/comments' in url: + return mock_comment_find(method, url, **kwargs) + if '/threads' in url: + return mock_thread_find(method, url, **kwargs) + + return mock_request @ddt.ddt class TaskTestCase(ModuleStoreTestCase): - + @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) - def setUp(self): - super(TaskTestCase, self).setUp() - - self.discussion_id = 'dummy_discussion_id' - self.course = CourseOverviewFactory.create(language='fr') + def setUpClass(cls): + super(TaskTestCase, cls).setUpClass() + cls.discussion_id = 'dummy_discussion_id' + cls.course = CourseOverviewFactory.create(language='fr') # Patch the comment client user save method so it does not try # to create a new cc user when creating a django user with mock.patch('student.models.cc.User.save'): - - self.thread_author = UserFactory( + cls.thread_author = UserFactory( username='thread_author', password='password', email='email' ) - self.comment_author = UserFactory( + cls.comment_author = UserFactory( username='comment_author', password='password', email='email' ) CourseEnrollmentFactory( - user=self.thread_author, - course_id=self.course.id + user=cls.thread_author, + course_id=cls.course.id ) CourseEnrollmentFactory( - user=self.comment_author, - course_id=self.course.id + user=cls.comment_author, + course_id=cls.course.id ) config = ForumsConfig.current() config.enabled = True config.save() + cls.create_thread_and_comments() + + @classmethod + def create_thread_and_comments(cls): + cls.thread = { + 'id': cls.discussion_id, + 'course_id': unicode(cls.course.id), + 'created_at': date.serialize(TWO_HOURS_AGO), + 'title': 'thread-title', + 'user_id': cls.thread_author.id, + 'username': cls.thread_author.username, + 'commentable_id': 'thread-commentable-id', + } + cls.comment = { + 'id': 'comment', + 'body': 'comment-body', + 'created_at': date.serialize(ONE_HOUR_AGO), + 'thread_id': cls.thread['id'], + 'parent_id': None, + 'user_id': cls.comment_author.id, + 'username': cls.comment_author.username, + } + cls.comment2 = { + 'id': 'comment2', + 'body': 'comment2-body', + 'created_at': date.serialize(NOW), + 'thread_id': cls.thread['id'], + 'parent_id': None, + 'user_id': cls.comment_author.id, + 'username': cls.comment_author.username + } + cls.subcomment = { + 'id': 'subcomment', + 'body': 'subcomment-body', + 'created_at': date.serialize(NOW), + 'thread_id': cls.thread['id'], + 'parent_id': cls.comment['id'], + 'user_id': cls.comment_author.id, + 'username': cls.comment_author.username, + } + cls.thread['children'] = [cls.comment, cls.comment2] + cls.comment['child_count'] = 1, + + def setUp(self): + super(TaskTestCase, self).setUp() + self.request_patcher = mock.patch('requests.request') + self.mock_request = self.request_patcher.start() + + self.ace_send_patcher = mock.patch('edx_ace.ace.send') + self.mock_ace_send = self.ace_send_patcher.start() + + thread_permalink = '/courses/discussion/dummy_discussion_id' + self.permalink_patcher = mock.patch('lms.djangoapps.discussion.tasks.permalink', return_value=thread_permalink) + self.mock_permalink = self.permalink_patcher.start() + + def tearDown(self): + super(TaskTestCase, self).tearDown() + self.request_patcher.stop() + self.ace_send_patcher.stop() + self.permalink_patcher.stop() + @ddt.data(True, False) + @override_waffle_flag(SEND_NOTIFICATIONS_FOR_COURSE, True) def test_send_discussion_email_notification(self, user_subscribed): - with mock_the_things() as mocked_items: - mock_request, mock_ace_send, mock_permalink = mocked_items - if user_subscribed: - non_matching_id = 'not-a-match' - # with per_page left with a default value of 1, this ensures - # that we test a multiple page result when calling - # comment_client.User.subscribed_threads() - mock_request.side_effect = make_mock_responder([non_matching_id, self.discussion_id]) - else: - mock_request.side_effect = make_mock_responder([]) + if user_subscribed: + non_matching_id = 'not-a-match' + # with per_page left with a default value of 1, this ensures + # that we test a multiple page result when calling + # comment_client.User.subscribed_threads() + subscribed_thread_ids = [non_matching_id, self.discussion_id] + else: + subscribed_thread_ids = [] - now = datetime.utcnow() - one_hour_ago = now - timedelta(hours=1) - thread = mock.Mock( - id=self.discussion_id, - course_id=self.course.id, - created_at=one_hour_ago, - title='thread-title', - user_id=self.thread_author.id, - username=self.thread_author.username, - commentable_id='thread-commentable-id' - ) - comment = mock.Mock( - id='comment-id', - body='comment-body', - created_at=now, - thread=thread, - user_id=self.comment_author.id, - username=self.comment_author.username - ) - user = mock.Mock() - - with waffle().override(FORUM_RESPONSE_NOTIFICATIONS): + self.mock_request.side_effect = make_mock_responder( + subscribed_thread_ids=subscribed_thread_ids, + comment_data=self.comment, + thread_data=self.thread, + ) + user = mock.Mock() + comment = cc.Comment.find(id=self.comment['id']).retrieve() + site = Site.objects.get_current() + with waffle().override(FORUM_RESPONSE_NOTIFICATIONS): + with mock.patch('lms.djangoapps.discussion.signals.handlers.get_current_site', return_value=site): comment_created.send(sender=None, user=user, post=comment) - if user_subscribed: - expected_message_context = get_base_template_context(Site.objects.get_current()) - expected_message_context.update({ - 'comment_author_id': self.comment_author.id, - 'comment_body': 'comment-body', - 'comment_created_at': now, - 'comment_id': 'comment-id', - 'comment_username': self.comment_author.username, - 'course_id': self.course.id, - 'thread_author_id': self.thread_author.id, - 'thread_created_at': one_hour_ago, - 'thread_id': self.discussion_id, - 'thread_title': 'thread-title', - 'thread_username': self.thread_author.username, - 'thread_commentable_id': 'thread-commentable-id', - 'post_link': urljoin(Site.objects.get_current().domain, mock_permalink.return_value), - 'site': Site.objects.get_current(), - 'site_id': Site.objects.get_current().id, - }) - ga_tracking_pixel_url = _generate_ga_pixel_url(expected_message_context) - expected_message_context.update({'ga_tracking_pixel_url': ga_tracking_pixel_url}) - expected_recipient = Recipient(self.thread_author.username, self.thread_author.email) - actual_message = mock_ace_send.call_args_list[0][0][0] - self.assertEqual(expected_message_context, actual_message.context) - self.assertEqual(expected_recipient, actual_message.recipient) - self.assertEqual(self.course.language, actual_message.language) - else: - self.assertFalse(mock_ace_send.called) + if user_subscribed: + expected_message_context = get_base_template_context(site) + expected_message_context.update({ + 'comment_author_id': self.comment_author.id, + 'comment_body': self.comment['body'], + 'comment_created_at': ONE_HOUR_AGO, + 'comment_id': self.comment['id'], + 'comment_username': self.comment_author.username, + 'course_id': self.course.id, + 'thread_author_id': self.thread_author.id, + 'thread_created_at': TWO_HOURS_AGO, + 'thread_id': self.discussion_id, + 'thread_title': 'thread-title', + 'thread_username': self.thread_author.username, + 'thread_commentable_id': self.thread['commentable_id'], + 'post_link': urljoin(site.domain, self.mock_permalink.return_value), + 'site': site, + 'site_id': site.id + }) + expected_message_context['ga_tracking_pixel_url'] = _generate_ga_pixel_url(expected_message_context) + expected_recipient = Recipient(self.thread_author.username, self.thread_author.email) + actual_message = self.mock_ace_send.call_args_list[0][0][0] + self.assertEqual(expected_message_context, actual_message.context) + self.assertEqual(expected_recipient, actual_message.recipient) + self.assertEqual(self.course.language, actual_message.language) + else: + self.assertFalse(self.mock_ace_send.called) + + @override_waffle_flag(SEND_NOTIFICATIONS_FOR_COURSE, True) + def run_should_not_send_email_test(self, comment_dict): + self.mock_request.side_effect = make_mock_responder( + subscribed_thread_ids=[self.discussion_id], + comment_data=comment_dict, + thread_data=self.thread, + ) + user = mock.Mock() + comment = cc.Comment.find(id=comment_dict['id']).retrieve() + with waffle().override(FORUM_RESPONSE_NOTIFICATIONS): + comment_created.send(sender=None, user=user, post=comment) + + actual_result = _should_send_message({ + 'thread_author_id': self.thread_author.id, + 'course_id': self.course.id, + 'comment_id': comment_dict['id'], + 'thread_id': self.thread['id'], + }) + self.assertEqual(actual_result, False) + self.assertFalse(self.mock_ace_send.called) + + def test_subcomment_should_not_send_email(self): + self.run_should_not_send_email_test(self.subcomment) + + def test_second_comment_should_not_send_email(self): + self.run_should_not_send_email_test(self.comment2)