EDUCATOR-1571 | Send discussion notification on first response; add a...

EDUCATOR-1571 | Send discussion notification on first response; add a CourseWaffleFlag for discussion notifications.

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():
     """

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
 
-def send_message(comment):
+    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, 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])

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',

lms/djangoapps/discussion/tests/test_signals.py

 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=sender, user=user, post=post)
+            signals.comment_created.send(sender=self.sender, user=self.user, post=self.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=self.sender, user=self.user, post=self.post)
+
+            self.assertFalse(mock_send_message.called)
 
-            signals.comment_created.send(sender=sender, user=user, post=post)
+    @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)

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
 
+NOW = datetime.utcnow()
+ONE_HOUR_AGO = NOW - timedelta(hours=1)
+TWO_HOURS_AGO = NOW - timedelta(hours=2)
 
-@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)
 
-
-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([])
-
-            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):
+        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 = []
+
+        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)