feat: move new forum content creation notifications to use course wid…

…e notification event

lms/djangoapps/discussion/rest_api/discussions_notifications.py

@@ -3,12 +3,10 @@
 """
 from django.conf import settings
 from lms.djangoapps.discussion.django_comment_client.permissions import get_team
-from openedx_events.learning.data import UserNotificationData
-from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED
+from openedx_events.learning.data import UserNotificationData, CourseNotificationData
+from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED, COURSE_NOTIFICATION_REQUESTED
 
-from common.djangoapps.student.models import CourseEnrollment
-from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole
-from openedx.core.djangoapps.course_groups.models import CourseCohortsSettings, CourseUserGroup
+from openedx.core.djangoapps.course_groups.models import CourseCohortsSettings
 from openedx.core.djangoapps.discussions.utils import get_divided_discussions
 from django.utils.translation import gettext_lazy as _
 
@@ -19,7 +17,6 @@
     FORUM_ROLE_COMMUNITY_TA,
     FORUM_ROLE_MODERATOR,
     CourseDiscussionSettings,
-    Role
 )
 
 
@@ -62,6 +59,29 @@ def _send_notification(self, user_ids, notification_type, extra_context=None):
         )
         USER_NOTIFICATION_REQUESTED.send_event(notification_data=notification_data)
 
+    def _send_course_wide_notification(self, notification_type, audience_filters=None, extra_context=None):
+        """
+        Send notification to all users in the course
+        """
+        if not extra_context:
+            extra_context = {}
+
+        notification_data = CourseNotificationData(
+            course_key=self.course.id,
+            content_context={
+                "replier_name": self.creator.username,
+                "post_title": self.thread.title,
+                "course_name": self.course.display_name,
+                "sender_id": self.creator.id,
+                **extra_context,
+            },
+            notification_type=notification_type,
+            content_url=f"{settings.DISCUSSIONS_MICROFRONTEND_URL}/{str(self.course.id)}/posts/{self.thread.id}",
+            app_name="discussion",
+            audience_filters=audience_filters,
+        )
+        COURSE_NOTIFICATION_REQUESTED.send_event(course_notification_data=notification_data)
+
     def _get_parent_response(self):
         """
         Get parent response object
@@ -175,7 +195,7 @@ def send_response_on_followed_post_notification(self):
 
     def _create_cohort_course_audience(self):
         """
-        Creates audience based on user cohort and role
+        Creates audience filter based on user cohort and role
         """
         course_key_str = str(self.course.id)
         discussion_cohorted = is_discussion_cohorted(course_key_str)
@@ -193,43 +213,21 @@ def _create_cohort_course_audience(self):
             group_id = int(group_id)
 
         # Course wide topics
-        topic_id = self.thread.attributes['commentable_id']
         all_topics = divided_inline_discussions + divided_course_wide_discussions
+        topic_id = self.thread.attributes['commentable_id']
         topic_divided = topic_id in all_topics or discussion_settings.always_divide_inline_discussions
 
         # Team object from topic id
         team = get_team(topic_id)
-
-        user_ids = []
         if team:
-            user_ids = team.users.all().values_list('id', flat=True)
-        elif discussion_cohorted and topic_divided and group_id is not None:
-            users_in_cohort = CourseUserGroup.objects.filter(
-                course_id=course_key_str, id=group_id
-            ).values_list('users__id', flat=True)
-            user_ids.extend(users_in_cohort)
-
-            privileged_roles = [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA]
-            privileged_users = Role.objects.filter(
-                name__in=privileged_roles,
-                course_id=course_key_str
-            ).values_list('users__id', flat=True)
-            user_ids.extend(privileged_users)
-
-            staff_users = CourseStaffRole(self.course.id).users_with_role().values_list('id', flat=True)
-            user_ids.extend(staff_users)
-
-            admin_users = CourseInstructorRole(self.course.id).users_with_role().values_list('id', flat=True)
-            user_ids.extend(admin_users)
-        else:
-            user_ids = CourseEnrollment.objects.filter(
-                course__id=course_key_str, is_active=True
-            ).values_list('user__id', flat=True)
-
-        unique_user_ids = list(set(user_ids))
-        if self.creator.id in unique_user_ids:
-            unique_user_ids.remove(self.creator.id)
-        return unique_user_ids
+            return {
+                'teams': [team.team_id],
+            }
+        if discussion_cohorted and topic_divided and group_id is not None:
+            return {
+                'cohorts': [group_id],
+            }
+        return {}
 
     def send_new_thread_created_notification(self):
         """
@@ -244,12 +242,22 @@ def send_new_thread_created_notification(self):
         if notification_type not in ['new_discussion_post', 'new_question_post']:
             raise ValueError(f'Invalid notification type {notification_type}')
 
-        user_ids = self._create_cohort_course_audience()
+        audience_filters = self._create_cohort_course_audience()
+
+        if audience_filters:
+            # If the audience is cohorted/teamed, we add the course and forum roles to the audience.
+            # Include course staff and instructors for course wide discussion notifications.
+            audience_filters['course_roles'] = ['staff', 'instructor']
+
+            # Include privileged forum roles for course wide discussion notifications.
+            audience_filters['discussion_roles'] = [
+                FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA
+            ]
         context = {
             'username': self.creator.username,
             'post_title': self.thread.title
         }
-        self._send_notification(user_ids, notification_type, context)
+        self._send_course_wide_notification(notification_type, audience_filters, context)
 
 
 def is_discussion_cohorted(course_key_str):

lms/djangoapps/discussion/rest_api/tests/test_tasks.py

@@ -8,7 +8,7 @@
 import httpretty
 from django.conf import settings
 from edx_toggles.toggles.testutils import override_waffle_flag
-from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED
+from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED, COURSE_NOTIFICATION_REQUESTED
 
 from common.djangoapps.student.models import CourseEnrollment
 from common.djangoapps.student.tests.factories import StaffFactory, UserFactory
@@ -158,14 +158,6 @@ def _create_thread(self, thread_type="discussion", group_id=None):
         self.register_get_thread_response(thread)
         return thread
 
-    def assert_users_id_list(self, user_ids_1, user_ids_2):
-        """
-        Assert whether the user ids in two lists are same
-        """
-        assert len(user_ids_1) == len(user_ids_2)
-        for user_id in user_ids_1:
-            assert user_id in user_ids_2
-
     def test_basic(self):
         """
         Left empty intentionally. This test case is inherited from DiscussionAPIViewTestMixin
@@ -176,23 +168,14 @@ def test_not_authenticated(self):
         Left empty intentionally. This test case is inherited from DiscussionAPIViewTestMixin
         """
 
-    def test_no_notification_if_course_has_no_enrollments(self):
-        """
-        Tests no notification is send if course has no enrollments
-        """
-        handler = mock.Mock()
-        USER_NOTIFICATION_REQUESTED.connect(handler)
-        send_thread_created_notification(self.thread['id'], str(self.course.id), self.author.id)
-        self.assertEqual(handler.call_count, 0)
-
     @ddt.data(
         ('new_question_post',),
         ('new_discussion_post',),
     )
     @ddt.unpack
     def test_notification_is_send_to_all_enrollments(self, notification_type):
         """
-        Tests notification is send to all users if course is not cohorted
+        Tests notification is sent to all users if course is not cohorted
         """
         self._assign_enrollments()
         thread_type = (
@@ -202,12 +185,13 @@ def test_notification_is_send_to_all_enrollments(self, notification_type):
         )
         thread = self._create_thread(thread_type=thread_type)
         handler = mock.Mock()
-        USER_NOTIFICATION_REQUESTED.connect(handler)
+        COURSE_NOTIFICATION_REQUESTED.connect(handler)
         send_thread_created_notification(thread['id'], str(self.course.id), self.author.id)
         self.assertEqual(handler.call_count, 1)
-        assert notification_type == handler.call_args[1]['notification_data'].notification_type
-        user_ids_list = [user.id for user in self.notification_to_all_users]
-        self.assert_users_id_list(user_ids_list, handler.call_args[1]['notification_data'].user_ids)
+        course_notification_data = handler.call_args[1]['course_notification_data']
+        assert notification_type == course_notification_data.notification_type
+        notification_audience_filters = {}
+        assert notification_audience_filters == course_notification_data.audience_filters
 
     @ddt.data(
         ('cohort_1', 'new_question_post'),
@@ -218,7 +202,7 @@ def test_notification_is_send_to_all_enrollments(self, notification_type):
     @ddt.unpack
     def test_notification_is_send_to_cohort_ids(self, cohort_text, notification_type):
         """
-        Tests if notification is send only to privileged users and cohort members if the
+        Tests if notification is sent only to privileged users and cohort members if the
         course is cohorted
         """
         self._assign_enrollments()
@@ -238,12 +222,17 @@ def test_notification_is_send_to_cohort_ids(self, cohort_text, notification_type
         cohort_id = cohort.id
         thread = self._create_thread(group_id=cohort_id, thread_type=thread_type)
         handler = mock.Mock()
-        USER_NOTIFICATION_REQUESTED.connect(handler)
+        COURSE_NOTIFICATION_REQUESTED.connect(handler)
         send_thread_created_notification(thread['id'], str(self.course.id), self.author.id)
-        assert notification_type == handler.call_args[1]['notification_data'].notification_type
+        course_notification_data = handler.call_args[1]['course_notification_data']
+        assert notification_type == course_notification_data.notification_type
+        notification_audience_filters = {
+            'cohorts': [cohort_id],
+            'course_roles': ['staff', 'instructor'],
+            'discussion_roles': ['Administrator', 'Moderator', 'Community TA'],
+        }
+        assert notification_audience_filters == handler.call_args[1]['course_notification_data'].audience_filters
         self.assertEqual(handler.call_count, 1)
-        user_ids_list = [user.id for user in audience]
-        self.assert_users_id_list(user_ids_list, handler.call_args[1]['notification_data'].user_ids)
 
 
 @ddt.ddt

openedx/core/djangoapps/notifications/audience_filters.py

@@ -4,9 +4,14 @@
 
 from abc import abstractmethod
 
+from opaque_keys.edx.keys import CourseKey
+
 from common.djangoapps.course_modes.models import CourseMode
 from common.djangoapps.student.models import CourseEnrollment
+from common.djangoapps.student.roles import CourseStaffRole, CourseInstructorRole
 from lms.djangoapps.discussion.django_comment_client.utils import get_users_with_roles
+from lms.djangoapps.teams.models import CourseTeam
+from openedx.core.djangoapps.course_groups.models import CourseUserGroup
 from openedx.core.djangoapps.django_comment_common.models import (
     FORUM_ROLE_ADMINISTRATOR,
     FORUM_ROLE_MODERATOR,
@@ -33,11 +38,10 @@ def filter(self, values):
         pass
 
 
-class RoleAudienceFilter(NotificationAudienceFilterBase):
+class ForumRoleAudienceFilter(NotificationAudienceFilterBase):
     """
     Filter class for roles
     """
-    # TODO: Add course roles to this. We currently support only forum roles
     allowed_filters = [
         FORUM_ROLE_ADMINISTRATOR,
         FORUM_ROLE_MODERATOR,
@@ -55,6 +59,36 @@ def filter(self, roles):
         return [user.id for user in get_users_with_roles(roles, self.course_key)]
 
 
+class CourseRoleAudienceFilter(NotificationAudienceFilterBase):
+    """
+    Filter class for course roles
+    """
+    allowed_filters = ['staff', 'instructor']
+
+    def filter(self, course_roles):
+        """
+        Filter users based on their course roles
+        """
+        if not self.is_valid_filter(course_roles):
+            raise ValueError(f'Invalid roles {course_roles} passed to CourseRoleAudienceFilter')
+
+        user_ids = []
+
+        course_key = self.course_key
+        if not isinstance(course_key, CourseKey):
+            course_key = CourseKey.from_string(course_key)
+
+        if 'staff' in course_roles:
+            staff_users = CourseStaffRole(course_key).users_with_role().values_list('id', flat=True)
+            user_ids.extend(staff_users)
+
+        if 'instructor' in course_roles:
+            instructor_users = CourseInstructorRole(course_key).users_with_role().values_list('id', flat=True)
+            user_ids.extend(instructor_users)
+
+        return user_ids
+
+
 class EnrollmentAudienceFilter(NotificationAudienceFilterBase):
     """
     Filter class for enrollment modes
@@ -71,3 +105,39 @@ def filter(self, enrollment_modes):
             course_id=self.course_key,
             mode__in=enrollment_modes,
         ).values_list('user_id', flat=True)
+
+
+class TeamAudienceFilter(NotificationAudienceFilterBase):
+    """
+    Filter class for team roles
+    """
+
+    def filter(self, team_ids):
+        """
+        Filter users based on team id
+        """
+        teams = CourseTeam.objects.filter(team_id__in=team_ids, course_id=self.course_key)
+
+        if not teams:   # invalid team ids passed
+            raise ValueError(f'Invalid Team ids {team_ids} passed to TeamAudienceFilter for course {self.course_key}')
+
+        user_ids = []
+        for team in teams:
+            user_ids.extend(team.users.all().values_list('id', flat=True))
+
+        return user_ids
+
+
+class CohortAudienceFilter(NotificationAudienceFilterBase):
+    """
+    Filter class for cohort roles
+    """
+
+    def filter(self, group_ids):
+        """
+        Filter users based on their cohort ids
+        """
+        users_in_cohort = CourseUserGroup.objects.filter(
+            course_id=self.course_key, id__in=group_ids
+        ).values_list('users__id', flat=True)
+        return users_in_cohort