From 8fac3bc060ca7c79dce33117903b715841d4385f Mon Sep 17 00:00:00 2001 From: Hassan Raza Date: Fri, 27 Jun 2025 15:25:22 +0500 Subject: [PATCH] feat: Add notify all learners option for discussion post (#36922) * feat: Add notify all learners option for discussion post * fix: Remove waffle flag from default notification dict --- lms/djangoapps/discussion/rest_api/api.py | 16 ++++-- .../rest_api/discussions_notifications.py | 7 +-- lms/djangoapps/discussion/rest_api/tasks.py | 15 +++++- .../discussion/rest_api/tests/test_api.py | 13 +++-- .../discussion/rest_api/tests/test_tasks.py | 49 +++++++++++++------ .../discussion/rest_api/tests/test_views.py | 1 + lms/djangoapps/discussion/rest_api/utils.py | 23 +++++++++ lms/djangoapps/discussion/signals/handlers.py | 5 +- .../notifications/base_notification.py | 18 +++++++ .../djangoapps/notifications/config/waffle.py | 10 ++++ .../core/djangoapps/notifications/models.py | 2 +- .../notifications/tests/test_utils.py | 5 +- .../core/djangoapps/notifications/utils.py | 15 +++++- .../core/djangoapps/notifications/views.py | 14 +++++- 14 files changed, 158 insertions(+), 35 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 100482ea56..e1f83cd7e6 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -127,7 +127,8 @@ from .utils import ( get_usernames_for_course, get_usernames_from_search_string, set_attribute, - is_posting_allowed + is_posting_allowed, + can_user_notify_all_learners ) User = get_user_model() @@ -333,6 +334,8 @@ def get_course(request, course_key, check_tab=True): course.get_discussion_blackout_datetimes() ) discussion_tab = CourseTabList.get_tab_by_type(course.tabs, 'discussion') + is_course_staff = CourseStaffRole(course_key).has_user(request.user) + is_course_admin = CourseInstructorRole(course_key).has_user(request.user) return { "id": str(course_key), "is_posting_enabled": is_posting_enabled, @@ -358,8 +361,8 @@ def get_course(request, course_key, check_tab=True): }), "is_group_ta": bool(user_roles & {FORUM_ROLE_GROUP_MODERATOR}), "is_user_admin": request.user.is_staff, - "is_course_staff": CourseStaffRole(course_key).has_user(request.user), - "is_course_admin": CourseInstructorRole(course_key).has_user(request.user), + "is_course_staff": is_course_staff, + "is_course_admin": is_course_admin, "provider": course_config.provider_type, "enable_in_context": course_config.enable_in_context, "group_at_subsection": course_config.plugin_configuration.get("group_at_subsection", False), @@ -372,6 +375,9 @@ def get_course(request, course_key, check_tab=True): for (reason_code, label) in CLOSE_REASON_CODES.items() ], 'show_discussions': bool(discussion_tab and discussion_tab.is_enabled(course, request.user)), + 'is_notify_all_learners_enabled': can_user_notify_all_learners( + course_key, user_roles, is_course_staff, is_course_admin + ), } @@ -1469,6 +1475,8 @@ def create_thread(request, thread_data): if not discussion_open_for_user(course, user): raise DiscussionBlackOutException + notify_all_learners = thread_data.pop("notify_all_learners", False) + context = get_context(course, request) _check_initializable_thread_fields(thread_data, context) discussion_settings = CourseDiscussionSettings.get(course_key) @@ -1484,7 +1492,7 @@ def create_thread(request, thread_data): raise ValidationError(dict(list(serializer.errors.items()) + list(actions_form.errors.items()))) serializer.save() cc_thread = serializer.instance - thread_created.send(sender=None, user=user, post=cc_thread) + thread_created.send(sender=None, user=user, post=cc_thread, notify_all_learners=notify_all_learners) api_thread = serializer.data _do_extra_actions(api_thread, cc_thread, list(thread_data.keys()), actions_form, context, request) diff --git a/lms/djangoapps/discussion/rest_api/discussions_notifications.py b/lms/djangoapps/discussion/rest_api/discussions_notifications.py index 57e231b966..648259249d 100644 --- a/lms/djangoapps/discussion/rest_api/discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/discussions_notifications.py @@ -318,17 +318,18 @@ class DiscussionNotificationSender: self._populate_context_with_ids_for_mobile(context, notification_type) self._send_notification([self.creator.id], notification_type, extra_context=context) - def send_new_thread_created_notification(self): + def send_new_thread_created_notification(self, notify_all_learners=False): """ Send notification based on notification_type """ thread_type = self.thread.attributes['thread_type'] - notification_type = ( + + notification_type = "new_instructor_all_learners_post" if notify_all_learners else ( "new_question_post" if thread_type == "question" else ("new_discussion_post" if thread_type == "discussion" else "") ) - if notification_type not in ['new_discussion_post', 'new_question_post']: + if notification_type not in ['new_discussion_post', 'new_question_post', 'new_instructor_all_learners_post']: raise ValueError(f'Invalid notification type {notification_type}') audience_filters = self._create_cohort_course_audience() diff --git a/lms/djangoapps/discussion/rest_api/tasks.py b/lms/djangoapps/discussion/rest_api/tasks.py index cbf4389889..e87804b1ca 100644 --- a/lms/djangoapps/discussion/rest_api/tasks.py +++ b/lms/djangoapps/discussion/rest_api/tasks.py @@ -6,8 +6,11 @@ from django.contrib.auth import get_user_model from edx_django_utils.monitoring import set_code_owner_attribute from opaque_keys.edx.locator import CourseKey +from common.djangoapps.student.roles import CourseStaffRole, CourseInstructorRole from lms.djangoapps.courseware.courses import get_course_with_access +from lms.djangoapps.discussion.django_comment_client.utils import get_user_role_names from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender +from lms.djangoapps.discussion.rest_api.utils import can_user_notify_all_learners from openedx.core.djangoapps.django_comment_common.comment_client import Comment from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS @@ -17,7 +20,7 @@ User = get_user_model() @shared_task @set_code_owner_attribute -def send_thread_created_notification(thread_id, course_key_str, user_id): +def send_thread_created_notification(thread_id, course_key_str, user_id, notify_all_learners=False): """ Send notification when a new thread is created """ @@ -26,9 +29,17 @@ def send_thread_created_notification(thread_id, course_key_str, user_id): return thread = Thread(id=thread_id).retrieve() user = User.objects.get(id=user_id) + + if notify_all_learners: + is_course_staff = CourseStaffRole(course_key).has_user(user) + is_course_admin = CourseInstructorRole(course_key).has_user(user) + user_roles = get_user_role_names(user, course_key) + if not can_user_notify_all_learners(course_key, user_roles, is_course_staff, is_course_admin): + return + course = get_course_with_access(user, 'load', course_key, check_if_enrolled=True) notification_sender = DiscussionNotificationSender(thread, course, user) - notification_sender.send_new_thread_created_notification() + notification_sender.send_new_thread_created_notification(notify_all_learners) @shared_task diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index 48b943543c..d0eae92316 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -214,6 +214,7 @@ class GetCourseTest(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase) 'edit_reasons': [{'code': 'test-edit-reason', 'label': 'Test Edit Reason'}], 'post_close_reasons': [{'code': 'test-close-reason', 'label': 'Test Close Reason'}], 'show_discussions': True, + 'is_notify_all_learners_enabled': False } @ddt.data( @@ -1379,7 +1380,9 @@ class CreateThreadTest( "read": True, }) self.register_post_thread_response(cs_thread) - with self.assert_signal_sent(api, 'thread_created', sender=None, user=self.user, exclude_args=('post',)): + with self.assert_signal_sent( + api, 'thread_created', sender=None, user=self.user, exclude_args=('post', 'notify_all_learners') + ): actual = create_thread(self.request, self.minimal_data) expected = self.expected_thread_data({ "id": "test_id", @@ -1445,7 +1448,9 @@ class CreateThreadTest( _assign_role_to_user(user=self.user, course_id=self.course.id, role=FORUM_ROLE_MODERATOR) - with self.assert_signal_sent(api, 'thread_created', sender=None, user=self.user, exclude_args=('post',)): + with self.assert_signal_sent( + api, 'thread_created', sender=None, user=self.user, exclude_args=('post', 'notify_all_learners') + ): actual = create_thread(self.request, self.minimal_data) expected = self.expected_thread_data({ "author_label": "Moderator", @@ -1517,7 +1522,9 @@ class CreateThreadTest( "read": True, }) self.register_post_thread_response(cs_thread) - with self.assert_signal_sent(api, 'thread_created', sender=None, user=self.user, exclude_args=('post',)): + with self.assert_signal_sent( + api, 'thread_created', sender=None, user=self.user, exclude_args=('post', 'notify_all_learners') + ): create_thread(self.request, data) event_name, event_data = mock_emit.call_args[0] assert event_name == 'edx.forum.thread.created' diff --git a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py index a6eb4948ce..70aeb8dd4a 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py @@ -29,7 +29,7 @@ from openedx.core.djangoapps.django_comment_common.models import ( FORUM_ROLE_STUDENT, CourseDiscussionSettings ) -from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS, ENABLE_NOTIFY_ALL_LEARNERS from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -190,29 +190,46 @@ class TestNewThreadCreatedNotification(DiscussionAPIViewTestMixin, ModuleStoreTe """ @ddt.data( - ('new_question_post',), - ('new_discussion_post',), + ('new_question_post', False, False), + ('new_discussion_post', False, False), + ('new_discussion_post', True, True), + ('new_discussion_post', True, False), ) @ddt.unpack - def test_notification_is_send_to_all_enrollments(self, notification_type): + def test_notification_is_send_to_all_enrollments( + self, notification_type, notify_all_learners, waffle_flag_enabled + ): """ Tests notification is sent to all users if course is not cohorted """ self._assign_enrollments() thread_type = ( - "discussion" - if notification_type == "new_discussion_post" - else ("question" if notification_type == "new_question_post" else "") + "discussion" if notification_type == "new_discussion_post" else "question" ) - thread = self._create_thread(thread_type=thread_type) - handler = mock.Mock() - COURSE_NOTIFICATION_REQUESTED.connect(handler) - send_thread_created_notification(thread['id'], str(self.course.id), self.author.id) - self.assertEqual(handler.call_count, 1) - 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 + + with override_waffle_flag(ENABLE_NOTIFY_ALL_LEARNERS, active=waffle_flag_enabled): + thread = self._create_thread(thread_type=thread_type) + handler = mock.Mock() + COURSE_NOTIFICATION_REQUESTED.connect(handler) + + send_thread_created_notification( + thread['id'], + str(self.course.id), + self.author.id, + notify_all_learners + ) + expected_handler_calls = 0 if notify_all_learners and not waffle_flag_enabled else 1 + self.assertEqual(handler.call_count, expected_handler_calls) + + if handler.call_count: + course_notification_data = handler.call_args[1]['course_notification_data'] + expected_type = ( + 'new_instructor_all_learners_post' + if notify_all_learners and waffle_flag_enabled + else notification_type + ) + self.assertEqual(course_notification_data.notification_type, expected_type) + self.assertEqual(course_notification_data.audience_filters, {}) @ddt.data( ('cohort_1', 'new_question_post'), diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 1df43ee4f9..3e11b1ba5f 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -557,6 +557,7 @@ class CourseViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "edit_reasons": [{"code": "test-edit-reason", "label": "Test Edit Reason"}], "post_close_reasons": [{"code": "test-close-reason", "label": "Test Close Reason"}], 'show_discussions': True, + 'is_notify_all_learners_enabled': False } ) diff --git a/lms/djangoapps/discussion/rest_api/utils.py b/lms/djangoapps/discussion/rest_api/utils.py index e7dca49910..bddd81c3f0 100644 --- a/lms/djangoapps/discussion/rest_api/utils.py +++ b/lms/djangoapps/discussion/rest_api/utils.py @@ -11,6 +11,7 @@ from pytz import UTC from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole from lms.djangoapps.discussion.django_comment_client.utils import has_discussion_privileges +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFY_ALL_LEARNERS from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, PostingRestriction from openedx.core.djangoapps.django_comment_common.models import ( FORUM_ROLE_ADMINISTRATOR, @@ -379,3 +380,25 @@ def is_posting_allowed(posting_restrictions: str, blackout_schedules: List): return not any(schedule["start"] <= now <= schedule["end"] for schedule in blackout_schedules) else: return False + + +def can_user_notify_all_learners(course_key, user_roles, is_course_staff, is_course_admin): + """ + Check if user posting is allowed to notify all learners based on the given restrictions + + Args: + course_key (CourseKey): CourseKey for which user creating any discussion post. + user_roles (Dict): Roles of the posting user + is_course_staff (Boolean): Whether the user has a course staff access. + is_course_admin (Boolean): Whether the user has a course admin access. + + Returns: + bool: True if posting for all learner is allowed to this user, False otherwise. + """ + is_staff_or_instructor = any([ + user_roles.intersection({FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR}), + is_course_staff, + is_course_admin, + ]) + + return is_staff_or_instructor and ENABLE_NOTIFY_ALL_LEARNERS.is_enabled(course_key) diff --git a/lms/djangoapps/discussion/signals/handlers.py b/lms/djangoapps/discussion/signals/handlers.py index 73c19d2785..ec14cd8281 100644 --- a/lms/djangoapps/discussion/signals/handlers.py +++ b/lms/djangoapps/discussion/signals/handlers.py @@ -166,7 +166,10 @@ def create_thread_created_notification(*args, **kwargs): """ user = kwargs['user'] post = kwargs['post'] - send_thread_created_notification.apply_async(args=[post.id, post.attributes['course_id'], user.id]) + notify_all_learners = kwargs.get('notify_all_learners', False) + send_thread_created_notification.apply_async( + args=[post.id, post.attributes['course_id'], user.id, notify_all_learners] + ) @receiver(signals.comment_created) diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index e47486df32..d345eb1263 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -230,6 +230,24 @@ COURSE_NOTIFICATION_TYPES = { 'email_template': '', 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE], }, + 'new_instructor_all_learners_post': { + 'notification_app': 'discussion', + 'name': 'new_instructor_all_learners_post', + 'is_core': False, + 'info': '', + 'web': True, + 'email': False, + 'email_cadence': EmailCadence.DAILY, + 'push': False, + 'non_editable': [], + 'content_template': _('<{p}>Your instructor posted <{strong}>{post_title}'), + 'grouped_content_template': '', + 'content_context': { + 'post_title': 'Post title', + }, + 'email_template': '', + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] + }, } COURSE_NOTIFICATION_APPS = { diff --git a/openedx/core/djangoapps/notifications/config/waffle.py b/openedx/core/djangoapps/notifications/config/waffle.py index 4a1481f6f5..cfc7e1039f 100644 --- a/openedx/core/djangoapps/notifications/config/waffle.py +++ b/openedx/core/djangoapps/notifications/config/waffle.py @@ -49,3 +49,13 @@ ENABLE_ORA_GRADE_NOTIFICATION = CourseWaffleFlag(f"{WAFFLE_NAMESPACE}.enable_ora # .. toggle_warning: When the flag is ON, Notifications Grouping feature is enabled. # .. toggle_tickets: INF-1472 ENABLE_NOTIFICATION_GROUPING = CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.enable_notification_grouping', __name__) + +# .. toggle_name: notifications.post_enable_notify_all_learners +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Waffle flag to enable the notify all learners on discussion post +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2025-06-11 +# .. toggle_warning: When the flag is ON, notification to all learners feature is enabled on discussion post. +# .. toggle_tickets: INF-1917 +ENABLE_NOTIFY_ALL_LEARNERS = CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.enable_post_notify_all_learners', __name__) diff --git a/openedx/core/djangoapps/notifications/models.py b/openedx/core/djangoapps/notifications/models.py index 2864fe2408..fc994b46ea 100644 --- a/openedx/core/djangoapps/notifications/models.py +++ b/openedx/core/djangoapps/notifications/models.py @@ -26,7 +26,7 @@ NOTIFICATION_CHANNELS = ['web', 'push', 'email'] ADDITIONAL_NOTIFICATION_CHANNEL_SETTINGS = ['email_cadence'] # Update this version when there is a change to any course specific notification type or app. -COURSE_NOTIFICATION_CONFIG_VERSION = 13 +COURSE_NOTIFICATION_CONFIG_VERSION = 14 def get_course_notification_preference_config(): diff --git a/openedx/core/djangoapps/notifications/tests/test_utils.py b/openedx/core/djangoapps/notifications/tests/test_utils.py index 066c7b7f59..c00f2ba237 100644 --- a/openedx/core/djangoapps/notifications/tests/test_utils.py +++ b/openedx/core/djangoapps/notifications/tests/test_utils.py @@ -311,7 +311,10 @@ class TestVisibilityFilter(unittest.TestCase): 'core': {'web': True, 'push': True, 'email': True, 'email_cadence': 'Daily'}, 'content_reported': {'web': True, 'push': True, 'email': True, 'email_cadence': 'Daily'}, 'new_question_post': {'web': False, 'push': False, 'email': False, 'email_cadence': 'Daily'}, - 'new_discussion_post': {'web': False, 'push': False, 'email': False, 'email_cadence': 'Daily'} + 'new_discussion_post': {'web': False, 'push': False, 'email': False, 'email_cadence': 'Daily'}, + 'new_instructor_all_learners_post': { + 'web': True, 'push': False, 'email': False, 'email_cadence': 'Daily' + } }, 'core_notification_types': [ 'new_response', 'comment_on_followed_post', diff --git a/openedx/core/djangoapps/notifications/utils.py b/openedx/core/djangoapps/notifications/utils.py index 62e51b7ab9..762777e35b 100644 --- a/openedx/core/djangoapps/notifications/utils.py +++ b/openedx/core/djangoapps/notifications/utils.py @@ -4,9 +4,11 @@ Utils function for notifications app import copy from typing import Dict, List, Set +from opaque_keys.edx.keys import CourseKey + from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment from openedx.core.djangoapps.django_comment_common.models import Role -from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS, ENABLE_NOTIFY_ALL_LEARNERS from openedx.core.lib.cache_utils import request_cached @@ -132,12 +134,21 @@ def remove_preferences_with_no_access(preferences: dict, user) -> dict: user=user, course_id=preferences['course_id'] ).values_list('role', flat=True) - preferences['notification_preference_config'] = filter_out_visible_notifications( + + user_preferences = filter_out_visible_notifications( user_preferences, notifications_with_visibility_settings, user_forum_roles, user_course_roles ) + + course_key = CourseKey.from_string(preferences['course_id']) + discussion_config = user_preferences.get('discussion', {}) + notification_types = discussion_config.get('notification_types', {}) + + if notification_types and not ENABLE_NOTIFY_ALL_LEARNERS.is_enabled(course_key): + notification_types.pop('new_instructor_all_learners_post', None) + return preferences diff --git a/openedx/core/djangoapps/notifications/views.py b/openedx/core/djangoapps/notifications/views.py index 49e368ed1a..bece28da1f 100644 --- a/openedx/core/djangoapps/notifications/views.py +++ b/openedx/core/djangoapps/notifications/views.py @@ -26,7 +26,7 @@ from openedx.core.djangoapps.notifications.serializers import add_info_to_notifi from openedx.core.djangoapps.user_api.models import UserPreference from .base_notification import COURSE_NOTIFICATION_APPS -from .config.waffle import ENABLE_NOTIFICATIONS +from .config.waffle import ENABLE_NOTIFICATIONS, ENABLE_NOTIFY_ALL_LEARNERS from .events import ( notification_preference_update_event, notification_preferences_viewed_event, @@ -603,13 +603,23 @@ class AggregatedNotificationPreferences(APIView): notification_configs = aggregate_notification_configs( notification_configs ) + course_ids = notification_preferences.values_list('course_id', flat=True) + filter_out_visible_preferences_by_course_ids( request.user, notification_configs, - notification_preferences.values_list('course_id', flat=True), + course_ids, ) + notification_preferences_viewed_event(request) notification_configs = add_info_to_notification_config(notification_configs) + + discussion_config = notification_configs.get('discussion', {}) + notification_types = discussion_config.get('notification_types', {}) + + if not any(ENABLE_NOTIFY_ALL_LEARNERS.is_enabled(course_key) for course_key in course_ids): + notification_types.pop('new_instructor_all_learners_post', None) + return Response({ 'status': 'success', 'message': 'Notification preferences retrieved',