From 7165bb528ba6b723fa70191813b00d72c62015ba Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Tue, 21 Nov 2023 13:35:36 +0500 Subject: [PATCH] feat: updated filter logic to send notifications (#33743) * feat: updated filter logic to send notifications --- .../notifications/base_notification.py | 16 ++--- .../core/djangoapps/notifications/filters.py | 55 +++++++++++++- .../notifications/tests/test_filters.py | 72 +++++++++++++++++-- 3 files changed, 127 insertions(+), 16 deletions(-) diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index 619dbb97a2b4..877b029ebc28 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -5,7 +5,7 @@ from .utils import find_app_in_normalized_apps, find_pref_in_normalized_prefs -FILTER_AUDIT_EXPIRED = 'filter_audit_expired' +FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE = 'filter_audit_expired_users_with_no_role' COURSE_NOTIFICATION_TYPES = { 'new_comment_on_response': { @@ -19,7 +19,7 @@ 'replier_name': 'replier name', }, 'email_template': '', - 'filters': [FILTER_AUDIT_EXPIRED] + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, 'new_comment': { 'notification_app': 'discussion', @@ -33,7 +33,7 @@ 'replier_name': 'replier name', }, 'email_template': '', - 'filters': [FILTER_AUDIT_EXPIRED] + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, 'new_response': { 'notification_app': 'discussion', @@ -46,7 +46,7 @@ 'replier_name': 'replier name', }, 'email_template': '', - 'filters': [FILTER_AUDIT_EXPIRED] + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, 'new_discussion_post': { 'notification_app': 'discussion', @@ -63,7 +63,7 @@ 'username': 'Post author name', }, 'email_template': '', - 'filters': [FILTER_AUDIT_EXPIRED] + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, 'new_question_post': { 'notification_app': 'discussion', @@ -80,7 +80,7 @@ 'username': 'Post author name', }, 'email_template': '', - 'filters': [FILTER_AUDIT_EXPIRED] + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, 'response_on_followed_post': { 'notification_app': 'discussion', @@ -95,7 +95,7 @@ 'replier_name': 'replier name', }, 'email_template': '', - 'filters': [FILTER_AUDIT_EXPIRED] + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, 'comment_on_followed_post': { 'notification_app': 'discussion', @@ -111,7 +111,7 @@ 'replier_name': 'replier name', }, 'email_template': '', - 'filters': [FILTER_AUDIT_EXPIRED] + 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, } diff --git a/openedx/core/djangoapps/notifications/filters.py b/openedx/core/djangoapps/notifications/filters.py index ea71f276d2c3..da9642811c81 100644 --- a/openedx/core/djangoapps/notifications/filters.py +++ b/openedx/core/djangoapps/notifications/filters.py @@ -2,12 +2,20 @@ Notification filters """ import logging +from typing import List from django.utils import timezone from common.djangoapps.course_modes.models import CourseMode -from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment from openedx.core.djangoapps.course_date_signals.utils import get_expected_duration +from openedx.core.djangoapps.django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_MODERATOR, + Role +) from openedx.core.djangoapps.notifications.base_notification import COURSE_NOTIFICATION_TYPES from openedx.features.course_duration_limits.models import CourseDurationLimitConfig from xmodule.modulestore.django import modulestore @@ -21,9 +29,36 @@ class NotificationFilter: """ @staticmethod - def filter_audit_expired(user_ids, course) -> list: + def get_users_with_course_role(user_ids: List[int], course_id: str) -> List[int]: """ - Check if the user has access to the course + Get users with a course role for the given course. + """ + return CourseAccessRole.objects.filter( + user_id__in=user_ids, + course_id=course_id, + ).values_list('user_id', flat=True) + + @staticmethod + def get_users_with_forum_roles(user_ids: List[int], course_id: str) -> List[int]: + """ + Get users with forum roles for the given course. + """ + return Role.objects.filter( + + course_id=course_id, + users__id__in=user_ids, + name__in=[ + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_GROUP_MODERATOR, + ] + + ).values_list('users__id', flat=True) + + def filter_audit_expired_users_with_no_role(self, user_ids, course) -> list: + """ + Check if the user has access to the course this would be true if the user has a course role or a forum role """ verified_mode = CourseMode.verified_mode_for_course(course=course, include_expired=True) access_duration = get_expected_duration(course.id) @@ -34,15 +69,29 @@ def filter_audit_expired(user_ids, course) -> list: course.id, ) return user_ids + + users_with_course_role = self.get_users_with_course_role(user_ids, course.id) + users_with_forum_roles = self.get_users_with_forum_roles(user_ids, course.id) enrollments = CourseEnrollment.objects.filter( user_id__in=user_ids, course_id=course.id, mode=CourseMode.AUDIT, + user__is_staff=False, ) + if course_time_limit.enabled_for_course(course.id): enrollments = enrollments.filter(created__gte=course_time_limit.enabled_as_of) logger.info("NotificationFilter: Number of audit enrollments for course %s: %s", course.id, enrollments.count()) + for enrollment in enrollments: + if enrollment.user_id in users_with_course_role or enrollment.user_id in users_with_forum_roles: + logger.info( + "NotificationFilter: User %s has a course or forum role for course %s, so they will not be " + "filtered out", + enrollment.user_id, + course.id, + ) + continue content_availability_date = max(enrollment.created, course.start) expiration_date = content_availability_date + access_duration logger.info("NotificationFilter: content_availability_date: %s and access_duration: %s", diff --git a/openedx/core/djangoapps/notifications/tests/test_filters.py b/openedx/core/djangoapps/notifications/tests/test_filters.py index d1aff28de702..ed2aa2f87b6b 100644 --- a/openedx/core/djangoapps/notifications/tests/test_filters.py +++ b/openedx/core/djangoapps/notifications/tests/test_filters.py @@ -9,8 +9,16 @@ from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.roles import CourseInstructorRole from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_MODERATOR, + Role +) from openedx.core.djangoapps.notifications.filters import NotificationFilter from openedx.features.course_duration_limits.models import CourseDurationLimitConfig from openedx.features.course_experience.tests.views.helpers import add_course_mode @@ -40,7 +48,7 @@ def setUp(self): expired_audit.save() @mock.patch("openedx.core.djangoapps.course_date_signals.utils.get_course_run_details") - def test_audit_expired_filter( + def test_audit_expired_filter_with_no_role( self, mock_get_course_run_details, ): @@ -49,14 +57,14 @@ def test_audit_expired_filter( """ mock_get_course_run_details.return_value = {'weeks_to_complete': 4} - result = NotificationFilter.filter_audit_expired( + result = NotificationFilter().filter_audit_expired_users_with_no_role( [self.user.id, self.user_1.id], self.course, ) self.assertEqual([self.user_1.id], result) mock_get_course_run_details.return_value = {'weeks_to_complete': 7} - result = NotificationFilter.filter_audit_expired( + result = NotificationFilter().filter_audit_expired_users_with_no_role( [self.user.id, self.user_1.id], self.course, ) @@ -69,14 +77,15 @@ def test_audit_expired_filter( ) # weeks_to_complete is set to 4 because we want to test if CourseDurationLimitConfig is working correctly. mock_get_course_run_details.return_value = {'weeks_to_complete': 4} - result = NotificationFilter.filter_audit_expired( + result = NotificationFilter().filter_audit_expired_users_with_no_role( [self.user.id, self.user_1.id], self.course, ) self.assertEqual([self.user.id, self.user_1.id], result) @mock.patch("openedx.core.djangoapps.course_date_signals.utils.get_course_run_details") - @mock.patch("openedx.core.djangoapps.notifications.filters.NotificationFilter.filter_audit_expired") + @mock.patch( + "openedx.core.djangoapps.notifications.filters.NotificationFilter.filter_audit_expired_users_with_no_role") def test_apply_filter( self, mock_filter_audit_expired, @@ -94,3 +103,56 @@ def test_apply_filter( ) self.assertEqual([self.user.id, self.user_1.id], result) mock_filter_audit_expired.assert_called_once() + + @mock.patch("openedx.core.djangoapps.course_date_signals.utils.get_course_run_details") + def test_audit_expired_for_course_staff( + self, + mock_get_course_run_details, + ): + """ + Test if filter_audit_expired function is working correctly for course staff + """ + + mock_get_course_run_details.return_value = {'weeks_to_complete': 4} + result = NotificationFilter().filter_audit_expired_users_with_no_role( + [self.user.id, self.user_1.id], + self.course, + ) + self.assertEqual([self.user_1.id], result) + CourseInstructorRole(self.course.id).add_users(self.user) + result = NotificationFilter().filter_audit_expired_users_with_no_role( + [self.user.id, self.user_1.id], + self.course, + ) + self.assertEqual([self.user.id, self.user_1.id], result) + + @mock.patch("openedx.core.djangoapps.course_date_signals.utils.get_course_run_details") + @ddt.data( + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_GROUP_MODERATOR, + ) + def test_audit_expired_for_forum_roles( + self, + role_name, + mock_get_course_run_details, + + ): + """ + Test if filter_audit_expired function is working correctly for forum roles + """ + + mock_get_course_run_details.return_value = {'weeks_to_complete': 4} + result = NotificationFilter().filter_audit_expired_users_with_no_role( + [self.user.id, self.user_1.id], + self.course, + ) + self.assertEqual([self.user_1.id], result) + role = Role.objects.get_or_create(course_id=self.course.id, name=role_name)[0] + role.users.add(self.user.id) + result = NotificationFilter().filter_audit_expired_users_with_no_role( + [self.user.id, self.user_1.id], + self.course, + ) + self.assertEqual([self.user.id, self.user_1.id], result)