diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index 283417252d..62791721d8 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/lms/djangoapps/bulk_email/models.py @@ -4,11 +4,14 @@ Models for bulk email import logging +from datetime import datetime +from dateutil.relativedelta import relativedelta import markupsafe from config_models.models import ConfigurationModel -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth import get_user_model from django.db import models +from django.conf import settings from opaque_keys.edx.django.models import CourseKeyField @@ -23,6 +26,7 @@ from openedx.core.djangoapps.enrollments.errors import CourseModeNotFoundError from openedx.core.lib.html_to_text import html_to_text from openedx.core.lib.mail_utils import wrap_message +User = get_user_model() log = logging.getLogger(__name__) @@ -117,6 +121,13 @@ class Target(models.Model): courseenrollment__is_active=True ) enrollment_qset = User.objects.filter(enrollment_query) + + # filter out learners from the message who are no longer active in the course-run based on last login + last_login_eligibility_period = settings.BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD + if last_login_eligibility_period and isinstance(last_login_eligibility_period, int): + cutoff = datetime.now() - relativedelta(months=last_login_eligibility_period) + enrollment_qset = enrollment_qset.exclude(last_login__lte=cutoff) + if self.target_type == SEND_TO_MYSELF: if user_id is None: raise ValueError("Must define self user to send email to self.") diff --git a/lms/djangoapps/bulk_email/tests/factories.py b/lms/djangoapps/bulk_email/tests/factories.py new file mode 100644 index 0000000000..6f945b39f5 --- /dev/null +++ b/lms/djangoapps/bulk_email/tests/factories.py @@ -0,0 +1,14 @@ +""" +Provides factories for BulkEmail models. +""" + +from factory.django import DjangoModelFactory + +from lms.djangoapps.bulk_email.models import SEND_TO_LEARNERS, Target + + +class TargetFactory(DjangoModelFactory): + class Meta: + model = Target + + target_type = SEND_TO_LEARNERS diff --git a/lms/djangoapps/bulk_email/tests/test_models.py b/lms/djangoapps/bulk_email/tests/test_models.py index 4940b5e472..5699f00253 100644 --- a/lms/djangoapps/bulk_email/tests/test_models.py +++ b/lms/djangoapps/bulk_email/tests/test_models.py @@ -2,19 +2,20 @@ Unit tests for bulk-email-related models. """ - import datetime +from dateutil.relativedelta import relativedelta from unittest.mock import Mock, patch import pytest import ddt from django.core.management import call_command from django.test import TestCase +from django.test.utils import override_settings from opaque_keys.edx.keys import CourseKey from pytz import UTC from common.djangoapps.course_modes.models import CourseMode -from common.djangoapps.student.tests.factories import UserFactory +from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled from lms.djangoapps.bulk_email.models import ( SEND_TO_COHORT, @@ -26,6 +27,7 @@ from lms.djangoapps.bulk_email.models import ( CourseEmailTemplate, Optout ) +from lms.djangoapps.bulk_email.tests.factories import TargetFactory from openedx.core.djangoapps.course_groups.models import CourseCohort from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -310,3 +312,59 @@ class CourseAuthorizationTest(TestCase): # Now, course should STILL be authorized! assert is_bulk_email_feature_enabled(course_id) + + +class TargetFilterTest(ModuleStoreTestCase): + """ + Tests for the optional filtering of recipients from the results of the `get_users` method of the Target model. + """ + + def setUp(self): + super().setUp() + self.user1 = UserFactory(last_login=datetime.datetime.now()) + self.user2 = UserFactory(last_login=datetime.datetime.now() - relativedelta(months=2)) + self.user3 = UserFactory() + self.course = CourseFactory() + CourseEnrollmentFactory( + is_active=True, + mode='verified', + course_id=self.course.id, + user=self.user1 + ) + CourseEnrollmentFactory( + is_active=True, + mode='audit', + course_id=self.course.id, + user=self.user2 + ) + CourseEnrollmentFactory( + is_active=False, + mode='verified', + course_id=self.course.id, + user=self.user3 + ) + self.target = TargetFactory() + + @override_settings(BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD=None) + def test_target_no_last_login_eligibility(self): + """ + Verifies the default behavior stays the same if the `BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD` is not + set. + """ + result = self.target.get_users(self.course.id) + + assert result.count() == 2 + assert result.filter(id=self.user1.id).exists() + assert result.filter(id=self.user2.id).exists() + + @override_settings(BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD=1) + def test_target_last_login_eligibility_set(self): + """ + Verifies that users with a `login_date` beyond the treshold set according to the + `BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD` are excluded from the final results of the queryset returned + callers of the `get_users` method. + """ + result = self.target.get_users(self.course.id) + + assert result.count() == 1 + assert result.filter(id=self.user1.id).exists() diff --git a/lms/djangoapps/bulk_email/tests/test_tasks.py b/lms/djangoapps/bulk_email/tests/test_tasks.py index 3a719c0993..412dfedf4d 100644 --- a/lms/djangoapps/bulk_email/tests/test_tasks.py +++ b/lms/djangoapps/bulk_email/tests/test_tasks.py @@ -7,6 +7,8 @@ paths actually work. """ +from datetime import datetime +from dateutil.relativedelta import relativedelta import json from itertools import chain, cycle, repeat from smtplib import SMTPAuthenticationError, SMTPConnectError, SMTPDataError, SMTPServerDisconnected @@ -28,6 +30,7 @@ from boto.ses.exceptions import ( from celery.states import FAILURE, SUCCESS from django.conf import settings from django.core.management import call_command +from django.test.utils import override_settings from opaque_keys.edx.locator import CourseLocator from lms.djangoapps.bulk_email.tasks import _get_course_email_context @@ -472,3 +475,20 @@ class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): assert 'account_settings_url' in result assert 'email_settings_url' in result assert 'platform_name' in result + + @override_settings(BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD=1) + def test_ineligible_recipients_filtered_by_last_login(self): + """ + Test that verifies active and enrolled students with last_login dates beyond the set threshold are not sent bulk + course emails. + """ + # create students and enrollments for test, then update the last_login dates to fit the scenario under test + students = self._create_students(2) + students[0].last_login = datetime.now() + students[1].last_login = datetime.now() - relativedelta(months=2) + + with patch('lms.djangoapps.bulk_email.tasks.get_connection', autospec=True) as get_conn: + get_conn.return_value.send_messages.side_effect = cycle([None]) + # we should expect only one email to be sent as the other learner is not eligible to receive the message + # based on their last_login date + self._test_run_with_task(send_bulk_course_email, 'emailed', 1, 1) diff --git a/lms/envs/common.py b/lms/envs/common.py index c27cb1b6dc..9e6438f8bc 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -4841,3 +4841,9 @@ TEAMS_HELP_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-runn TEXTBOOKS_HELP_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/course_assets/textbooks.html" WIKI_HELP_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/course_assets/course_wiki.html" CUSTOM_PAGES_HELP_URL = "https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/course_assets/pages.html#adding-custom-pages" + +################# Bulk Course Email Settings ################# +# If set, recipients of bulk course email messages will be filtered based on the last_login date of their User account. +# The expected value is an Integer representing the cutoff point (in months) for inclusion to the message. Example: +# a value of `3` would include learners who have logged in within the past 3 months. +BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD = None diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index 54918e0f3d..a51c909db3 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -401,11 +401,6 @@ if FEATURES.get('ENABLE_ENTERPRISE_INTEGRATION'): DCS_SESSION_COOKIE_SAMESITE = 'Lax' DCS_SESSION_COOKIE_SAMESITE_FORCE_ALL = True -##################################################################### -# See if the developer has any local overrides. -if os.path.isfile(join(dirname(abspath(__file__)), 'private.py')): - from .private import * # pylint: disable=import-error,wildcard-import - ########################## THEMING ####################### # If you want to enable theming in devstack, uncomment this section and add any relevant # theme directories to COMPREHENSIVE_THEME_DIRS @@ -439,3 +434,9 @@ PROCTORING_USER_OBFUSCATION_KEY = '85920908f28904ed733fe576320db18cabd7b6cd' #################### Webpack Configuration Settings ############################## WEBPACK_LOADER['DEFAULT']['TIMEOUT'] = 5 + +################# New settings must go ABOVE this line ################# +######################################################################## +# See if the developer has any local overrides. +if os.path.isfile(join(dirname(abspath(__file__)), 'private.py')): + from .private import * # pylint: disable=import-error,wildcard-import