Merge remote-tracking branch 'origin/master' into jill/collection-delete

README.rst

@@ -124,18 +124,47 @@ sites)::
   ./manage.py lms collectstatic
   ./manage.py cms collectstatic
 
+Set up CMS SSO (for Development)::
+
+  ./manage.py lms manage_user studio_worker [email protected] --unusable-password
+  # DO NOT DO THIS IN PRODUCTION. It will make your auth insecure.
+  ./manage.py lms create_dot_application studio-sso-id studio_worker \
+      --grant-type authorization-code \
+      --skip-authorization \
+      --redirect-uris 'http://localhost:18010/complete/edx-oauth2/' \
+      --scopes user_id  \
+      --client-id 'studio-sso-id' \
+      --client-secret 'studio-sso-secret'
+
+Set up CMS SSO (for Production):
+
+* Create the CMS user and the OAuth application::
+
+    ./manage.py lms manage_user studio_worker <[email protected]> --unusable-password
+    ./manage.py lms create_dot_application studio-sso-id studio_worker \
+        --grant-type authorization-code \
+        --skip-authorization \
+        --redirect-uris 'http://localhost:18010/complete/edx-oauth2/' \
+        --scopes user_id
+
+* Log into Django admin (eg. http://localhost:18000/admin/oauth2_provider/application/),
+  click into the application you created above (``studio-sso-id``), and copy its "Client secret".
+* In your private LMS_CFG yaml file or your private Django settings module:
+
+ * Set ``SOCIAL_AUTH_EDX_OAUTH2_KEY`` to the client ID (``studio-sso-id``).
+ * Set ``SOCIAL_AUTH_EDX_OAUTH2_SECRET`` to the client secret (which you copied).
 Run the Platform
 ----------------
 
 First, ensure MySQL, Mongo, and Memcached are running.
 
 Start the LMS::
 
-  ./manage.py lms runserver
+  ./manage.py lms runserver 18000
 
 Start the CMS::
 
-  ./manage.py cms runserver
+  ./manage.py cms runserver 18010
 
 This will give you a mostly-headless Open edX platform. Most frontends have
 been migrated to "Micro-Frontends (MFEs)" which need to be installed and run

cms/envs/devstack.py

@@ -267,7 +267,8 @@ def should_show_debug_toolbar(request):  # lint-amnesty, pylint: disable=missing
 ################ Using LMS SSO for login to Studio ################
 SOCIAL_AUTH_EDX_OAUTH2_KEY = 'studio-sso-key'
 SOCIAL_AUTH_EDX_OAUTH2_SECRET = 'studio-sso-secret'  # in stage, prod would be high-entropy secret
-SOCIAL_AUTH_EDX_OAUTH2_URL_ROOT = 'http://edx.devstack.lms:18000'  # routed internally server-to-server
+# routed internally server-to-server
+SOCIAL_AUTH_EDX_OAUTH2_URL_ROOT = ENV_TOKENS.get('SOCIAL_AUTH_EDX_OAUTH2_URL_ROOT', 'http://edx.devstack.lms:18000')
 SOCIAL_AUTH_EDX_OAUTH2_PUBLIC_URL_ROOT = 'http://localhost:18000'  # used in browser redirect
 
 # Don't form the return redirect URL with HTTPS on devstack

lms/djangoapps/ccx/api/v0/tests/test_views.py

@@ -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
 

lms/djangoapps/ccx/api/v0/views.py

@@ -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)
@@ -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)

lms/djangoapps/ccx/utils.py

@@ -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:
@@ -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
 
 
@@ -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
@@ -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
@@ -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:
@@ -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,
                 )

lms/djangoapps/ccx/views.py

@@ -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)

lms/djangoapps/discussion/signals/handlers.py

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

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):
+    """
+    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
@@ -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)
@@ -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
@@ -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'])
@@ -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'])
     })

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 }}

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

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

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 }}: {{ comment_body|truncatechars:200 }}{% endblocktrans %}

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

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

lms/djangoapps/discussion/tests/test_tasks.py

@@ -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
@@ -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)
@@ -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
@@ -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,
@@ -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]
@@ -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):