From cc2126749add64ef35883ab0d6899e675fafe5b4 Mon Sep 17 00:00:00 2001 From: Hassan Raza Date: Mon, 21 Jul 2025 15:07:00 +0500 Subject: [PATCH] chore: Exclude course staff preferences from general user notifications (#37040) --- .../notifications/tests/test_views.py | 86 +++++++++++++------ .../core/djangoapps/notifications/utils.py | 34 ++++++++ .../core/djangoapps/notifications/views.py | 5 +- 3 files changed, 99 insertions(+), 26 deletions(-) diff --git a/openedx/core/djangoapps/notifications/tests/test_views.py b/openedx/core/djangoapps/notifications/tests/test_views.py index ff962db448..30837c9b86 100644 --- a/openedx/core/djangoapps/notifications/tests/test_views.py +++ b/openedx/core/djangoapps/notifications/tests/test_views.py @@ -21,7 +21,7 @@ from rest_framework import status from rest_framework.test import APIClient, APITestCase from common.djangoapps.student.models import CourseEnrollment -from common.djangoapps.student.roles import CourseStaffRole +from common.djangoapps.student.roles import CourseStaffRole, CourseInstructorRole from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory @@ -38,7 +38,8 @@ from openedx.core.djangoapps.notifications.models import ( Notification, get_course_notification_preference_config_version, NotificationPreference ) -from openedx.core.djangoapps.notifications.serializers import NotificationCourseEnrollmentSerializer +from openedx.core.djangoapps.notifications.serializers import NotificationCourseEnrollmentSerializer, \ + add_non_editable_in_preference from openedx.core.djangoapps.user_api.models import UserPreference from openedx.core.djangoapps.notifications.email.utils import update_user_preferences_from_patch from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -46,7 +47,7 @@ from xmodule.modulestore.tests.factories import CourseFactory from ..base_notification import COURSE_NOTIFICATION_APPS, COURSE_NOTIFICATION_TYPES, NotificationAppManager, \ NotificationTypeManager -from ..utils import get_notification_types_with_visibility_settings +from ..utils import get_notification_types_with_visibility_settings, exclude_inaccessible_preferences User = get_user_model() @@ -1453,13 +1454,15 @@ class GetAggregateNotificationPreferencesTest(APITestCase): }) -class TestNotificationPreferencesView(APITestCase): +@ddt.ddt +class TestNotificationPreferencesView(ModuleStoreTestCase): """ Tests for the NotificationPreferencesView API view. """ def setUp(self): # Set up a user and API client + super().setUp() self.default_data = { "status": "success", "message": "Notification preferences retrieved successfully.", @@ -1584,20 +1587,67 @@ class TestNotificationPreferencesView(APITestCase): } } } - self.user = User.objects.create_user(username='testuser', password='testpass') + self.TEST_PASSWORD = 'testpass' + self.user = UserFactory(password=self.TEST_PASSWORD) self.client = APIClient() self.client.force_authenticate(user=self.user) self.url = reverse('notification-preferences-aggregated-v2') # Adjust with the actual name + self.course = CourseFactory.create(display_name='test course 1', run="Testing_course_1") - def test_get_notification_preferences(self): + @ddt.data( + ("forum", FORUM_ROLE_ADMINISTRATOR, ['content_reported'], ['ora_staff_notifications']), + ("forum", FORUM_ROLE_MODERATOR, ['content_reported'], ['ora_staff_notifications']), + ("forum", FORUM_ROLE_COMMUNITY_TA, ['content_reported'], ['ora_staff_notifications']), + ("course", CourseStaffRole.ROLE, ['ora_staff_notifications'], ['content_reported']), + ("course", CourseInstructorRole.ROLE, ['ora_staff_notifications'], ['content_reported']), + (None, None, [], ['ora_staff_notifications', 'content_reported']), + ) + @ddt.unpack + def test_get_notification_preferences(self, role_type, role, visible_apps, hidden_apps): """ - Test case: Get notification preferences for the authenticated user + Test: Notification preferences visibility for users with forum, course, or no role. """ + role_instance = None + + if role_type == "course": + if role == CourseInstructorRole.ROLE: + CourseStaffRole(self.course.id).add_users(self.user) + else: + CourseInstructorRole(self.course.id).add_users(self.user) + self.client.login(username=self.user.username, password='testpass') + + elif role_type == "forum": + role_instance = RoleFactory(name=role, course_id=self.course.id) + role_instance.users.add(self.user) + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.data['status'], 'success') self.assertIn('data', response.data) - self.assertEqual(response.data['data'], self.default_data['data']) + + expected_data = exclude_inaccessible_preferences(self.default_data['data'], self.user) + expected_data = add_non_editable_in_preference(expected_data) + + self.assertEqual(response.data['data'], expected_data) + + notification_apps = {} + for app in ['discussion', 'grading']: + notification_apps.update(response.data['data'][app]['notification_types']) + + for app in visible_apps: + self.assertIn(app, notification_apps, msg=f"{app} should be visible for role: {role_type}") + + for app in hidden_apps: + self.assertNotIn(app, notification_apps, msg=f"{app} should NOT be visible for role: {role_type}") + + if role_type == "forum": + role_instance.users.clear() + elif role_type == "course": + if role == CourseInstructorRole.ROLE: + CourseStaffRole(self.course.id).remove_users(self.user) + else: + CourseInstructorRole(self.course.id).remove_users(self.user) def test_if_data_is_correctly_aggregated(self): """ @@ -1642,12 +1692,6 @@ class TestNotificationPreferencesView(APITestCase): "push": False, "email_cadence": "Daily" }, - "content_reported": { - "web": False, - "email": False, - "push": False, - "email_cadence": "Daily" - }, "new_instructor_all_learners_post": { "web": False, "email": False, @@ -1664,7 +1708,6 @@ class TestNotificationPreferencesView(APITestCase): "non_editable": { "new_discussion_post": ["push"], "new_question_post": ["push"], - "content_reported": ["push"], "new_instructor_all_learners_post": ["push"] } }, @@ -1693,12 +1736,6 @@ class TestNotificationPreferencesView(APITestCase): "enabled": True, "core_notification_types": [], "notification_types": { - "ora_staff_notifications": { - "web": False, - "email": False, - "push": False, - "email_cadence": "Daily" - }, "ora_grade_assigned": { "web": False, "email": False, @@ -1713,8 +1750,7 @@ class TestNotificationPreferencesView(APITestCase): } }, "non_editable": { - "ora_grade_assigned": ["push"], - "ora_staff_notifications": ["push"] + "ora_grade_assigned": ["push"] } }, "enrollments": { @@ -1769,7 +1805,9 @@ class TestNotificationPreferencesView(APITestCase): response = self.client.put(self.url, update_data, format='json') self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.data['status'], 'success') - cadence_set = NotificationPreference.objects.filter(type__in=core_types).values_list('email_cadence', flat=True) + cadence_set = NotificationPreference.objects.filter(user=self.user, type__in=core_types).values_list( + 'email_cadence', flat=True + ) self.assertEqual(len(set(cadence_set)), 1) self.assertIn('Weekly', set(cadence_set)) diff --git a/openedx/core/djangoapps/notifications/utils.py b/openedx/core/djangoapps/notifications/utils.py index 86f643f1fb..fac192113e 100644 --- a/openedx/core/djangoapps/notifications/utils.py +++ b/openedx/core/djangoapps/notifications/utils.py @@ -295,3 +295,37 @@ def filter_out_visible_preferences_by_course_ids(user, preferences: Dict, course forum_roles, course_roles ) + + +def get_user_forum_access_roles(user_id: int) -> List[str]: + """ + Get forum roles for the given user in all course. + + :param user_id: User ID + :return: List of forum roles + """ + return list(Role.objects.filter(users__id=user_id).values_list('name', flat=True)) + + +def exclude_inaccessible_preferences(user_preferences: dict, user): + """ + Exclude notifications from user preferences that the user has no access to, + based on forum and course roles. + + :param user_preferences: Dictionary of user notification preferences + :param user: Django User object + :return: Updated user_preferences dictionary (modified in-place) + """ + forum_roles = get_user_forum_access_roles(user.id) + visible_notifications = get_notification_types_with_visibility_settings() + course_roles = CourseAccessRole.objects.filter( + user=user + ).values_list('role', flat=True) + + filter_out_visible_notifications( + user_preferences, + visible_notifications, + forum_roles, + course_roles + ) + return user_preferences diff --git a/openedx/core/djangoapps/notifications/views.py b/openedx/core/djangoapps/notifications/views.py index 773ca4270b..58fd13b882 100644 --- a/openedx/core/djangoapps/notifications/views.py +++ b/openedx/core/djangoapps/notifications/views.py @@ -49,7 +49,8 @@ from .tasks import create_notification_preference from .utils import ( aggregate_notification_configs, filter_out_visible_preferences_by_course_ids, - get_show_notifications_tray + get_show_notifications_tray, + exclude_inaccessible_preferences ) @@ -702,7 +703,7 @@ class NotificationPreferencesView(APIView): type_details['email'] = user_pref.email type_details['push'] = user_pref.push type_details['email_cadence'] = user_pref.email_cadence - + exclude_inaccessible_preferences(structured_preferences, request.user) return Response({ 'status': 'success', 'message': 'Notification preferences retrieved successfully.',