diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index e4955524d6..e79397c262 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -10,8 +10,10 @@ import shutil import tempfile from unittest.mock import Mock, NonCallableMock, patch +import dateutil import ddt import pytest +import pytz from boto.exception import BotoServerError from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user @@ -27,6 +29,7 @@ from edx_toggles.toggles.testutils import override_waffle_flag from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import UsageKey from pytz import UTC +from testfixtures import LogCapture from xmodule.fields import Date from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import ( @@ -91,12 +94,14 @@ from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_COMM from openedx.core.djangoapps.django_comment_common.utils import seed_permissions_roles from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin +from openedx.core.djangoapps.user_api.preferences.api import set_user_preference, delete_user_preference from openedx.core.lib.teams_config import TeamsConfig from openedx.core.lib.xblock_utils import grade_histogram from openedx.features.course_experience import RELATIVE_DATES_FLAG from .test_tools import msk_from_problem_urlname +LOG_PATH = "lms.djangoapps.instructor.views.api" DATE_FIELD = Date() EXPECTED_CSV_HEADER = ( '"code","redeem_code_url","course_id","company_name","created_by","redeemed_by","invoice_id","purchaser",' @@ -3369,13 +3374,6 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm def setUpClass(cls): super().setUpClass() cls.course = CourseFactory.create() - test_subject = '\u1234 test subject' - test_message = '\u6824 test message' - cls.full_test_message = { - 'send_to': '["myself", "staff"]', - 'subject': test_subject, - 'message': test_message, - } BulkEmailFlag.objects.create(enabled=True, require_course_email_auth=False) @classmethod @@ -3385,10 +3383,26 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm def setUp(self): super().setUp() + test_subject = '\u1234 test subject' + test_message = '\u6824 test message' + self.full_test_message = { + 'send_to': '["myself", "staff"]', + 'subject': test_subject, + 'message': test_message, + } self.instructor = InstructorFactory(course_key=self.course.id) self.client.login(username=self.instructor.username, password='test') + def tearDown(self): + super().tearDown() + delete_user_preference(self.instructor, 'time_zone', username=self.instructor.username) + + def _get_expected_schedule(self, schedule, timezone): + local_tz = dateutil.tz.gettz(timezone) + local_dt = dateutil.parser.parse(schedule).replace(tzinfo=local_tz) + return local_dt.astimezone(pytz.utc) + def test_send_email_as_logged_in_instructor(self): url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) response = self.client.post(url, self.full_test_message) @@ -3475,6 +3489,122 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm html_message=self.full_test_message['message'], template_name=org_template, from_addr=org_email).count() + @patch("lms.djangoapps.instructor.views.api.task_api.submit_bulk_course_email") + def test_send_email_with_schedule_and_timezone(self, mock_task_api): + """ + Test for the new scheduling logic added to the `send_email` function. + """ + schedule = "2030-05-02T14:00:00.000Z" + timezone = "America/New_York" + self.full_test_message['schedule'] = schedule + self.full_test_message['browser_timezone'] = timezone + expected_schedule = self._get_expected_schedule(schedule, timezone) + expected_messages = [ + f"Converting requested schedule from local time '{schedule}' with the timezone '{timezone}' to UTC", + ] + + url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) + with LogCapture() as log: + response = self.client.post(url, self.full_test_message) + + assert response.status_code == 200 + _, _, _, arg_schedule = mock_task_api.call_args.args + assert arg_schedule == expected_schedule + log.check_present( + (LOG_PATH, "INFO", expected_messages[0]), + ) + + @patch("lms.djangoapps.instructor.views.api.task_api.submit_bulk_course_email") + def test_send_email_with_schedule_and_no_browser_timezone(self, mock_task_api): + """ + Test that verifies we will retrieve and use the preferred time zone if possible. + """ + schedule = "2030-05-02T14:00:00.000Z" + timezone = "America/New_York" + self.full_test_message['schedule'] = schedule + self.full_test_message['browser_timezone'] = "" + expected_schedule = self._get_expected_schedule(schedule, timezone) + set_user_preference(self.instructor, 'time_zone', timezone) + + url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) + response = self.client.post(url, self.full_test_message) + + assert response.status_code == 200 + _, _, _, arg_schedule = mock_task_api.call_args.args + assert arg_schedule == expected_schedule + + @patch("lms.djangoapps.instructor.views.api.task_api.submit_bulk_course_email") + def test_send_email_with_schedule_and_preferred_timezone(self, mock_task_api): + """ + Test that verifies we will use the preferred timezone over the browser timezone if possible. + """ + schedule = "2030-05-02T14:00:00.000Z" + preferred_timezone = "America/Anchorage" + self.full_test_message['schedule'] = schedule + self.full_test_message['browser_timezone'] = "America/New_York" + expected_schedule = self._get_expected_schedule(schedule, preferred_timezone) + set_user_preference(self.instructor, 'time_zone', preferred_timezone) + + url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) + response = self.client.post(url, self.full_test_message) + + assert response.status_code == 200 + _, _, _, arg_schedule = mock_task_api.call_args.args + assert arg_schedule == expected_schedule + + def test_send_email_with_malformed_schedule_expect_error(self): + self.full_test_message['schedule'] = "Blub Glub" + self.full_test_message['browser_timezone'] = "America/New_York" + expected_messages = [ + "Error occurred while attempting to create a scheduled bulk email task: unknown string format", + ] + + url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) + with LogCapture() as log: + response = self.client.post(url, self.full_test_message) + + assert response.status_code == 400 + log.check_present( + (LOG_PATH, "ERROR", expected_messages[0]), + ) + + def test_send_email_with_malformed_timezone_expect_error(self): + self.full_test_message['schedule'] = "2030-05-02T14:00:00.000Z" + self.full_test_message['browser_timezone'] = "Flim/Flam" + expected_messages = [ + "Error occurred while attempting to create a scheduled bulk email task: Unable to determine the time zone " + "to use to convert the schedule to UTC", + ] + + url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) + with LogCapture() as log: + response = self.client.post(url, self.full_test_message) + + assert response.status_code == 400 + log.check_present( + (LOG_PATH, "ERROR", expected_messages[0]), + ) + + def test_send_email_with_lapsed_date_expect_error(self): + schedule = "2020-01-01T00:00:00.000Z" + timezone = "America/New_York" + self.full_test_message['schedule'] = schedule + self.full_test_message['browser_timezone'] = timezone + expected_schedule = self._get_expected_schedule(schedule, timezone) + expected_messages = [ + "Error occurred while attempting to create a scheduled bulk email task: The requested schedule " + f"'{expected_schedule}' is in the past" + ] + + url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) + with LogCapture() as log: + response = self.client.post(url, self.full_test_message) + + assert response.status_code == 400 + log.check_present( + (LOG_PATH, "ERROR", expected_messages[0]), + ) + class MockCompletionInfo: """Mock for get_task_completion_info""" @@ -3717,7 +3847,9 @@ class TestInstructorEmailContentList(SharedModuleStoreTestCase, LoginEnrollmentT self.setup_fake_email_info(num_emails, with_failures) task_history_request.return_value = list(self.tasks.values()) url = reverse('list_email_content', kwargs={'course_id': str(self.course.id)}) - with patch('lms.djangoapps.instructor.views.api.CourseEmail.objects.get') as mock_email_info: + with patch( + 'lms.djangoapps.instructor.views.instructor_task_helpers.CourseEmail.objects.get' + ) as mock_email_info: mock_email_info.side_effect = self.get_matching_mock_email response = self.client.post(url, {}) assert response.status_code == 200 @@ -3794,7 +3926,9 @@ class TestInstructorEmailContentList(SharedModuleStoreTestCase, LoginEnrollmentT email_info = FakeEmailInfo(email, 0, 10) task_history_request.return_value = [task_info] url = reverse('list_email_content', kwargs={'course_id': str(self.course.id)}) - with patch('lms.djangoapps.instructor.views.api.CourseEmail.objects.get') as mock_email_info: + with patch( + 'lms.djangoapps.instructor.views.instructor_task_helpers.CourseEmail.objects.get' + ) as mock_email_info: mock_email_info.return_value = email response = self.client.post(url, {}) assert response.status_code == 200 diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index efa6deee2b..9651c0b397 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -7,12 +7,15 @@ Many of these GETs may become PUTs in the future. """ import csv +import datetime import json import logging import string import random import re +import dateutil +import pytz import edx_api_doc_tools as apidocs from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user @@ -71,8 +74,7 @@ from common.djangoapps.util.file import ( ) from common.djangoapps.util.json_request import JsonResponse, JsonResponseBadRequest from common.djangoapps.util.views import require_global_staff -from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled -from lms.djangoapps.bulk_email.models import CourseEmail +from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled, create_course_email from lms.djangoapps.certificates import api as certs_api from lms.djangoapps.certificates.models import ( CertificateStatuses @@ -2692,6 +2694,7 @@ def send_email(request, course_id): - 'message' specifies email's content """ course_id = CourseKey.from_string(course_id) + course_overview = CourseOverview.get_from_id(course_id) if not is_bulk_email_feature_enabled(course_id): log.warning('Email is not enabled for course %s', course_id) @@ -2700,51 +2703,45 @@ def send_email(request, course_id): targets = json.loads(request.POST.get("send_to")) subject = request.POST.get("subject") message = request.POST.get("message") + # optional, this is a date and time in the form of an ISO8601 string + schedule = request.POST.get("schedule", "") + # optional, this is the timezone captured from the author's browser when requesting a scheduled email + browser_timezone = request.POST.get("browser_timezone", "") - # allow two branding points to come from Site Configuration: which CourseEmailTemplate should be used - # and what the 'from' field in the email should be - # - # If these are None (there is no site configuration enabled for the current site) than - # the system will use normal system defaults - course_overview = CourseOverview.get_from_id(course_id) - from_addr = configuration_helpers.get_value('course_email_from_addr') - if isinstance(from_addr, dict): - # If course_email_from_addr is a dict, we are customizing - # the email template for each organization that has courses - # on the site. The dict maps from addresses by org allowing - # us to find the correct from address to use here. - from_addr = from_addr.get(course_overview.display_org_with_default) + # If this is a scheduled bulk email request then we try to convert the requested schedule date to UTC. We do this + # before we attempt to create the email object instance in case there is an issue as we don't want to have an + # orphaned email object that will never be sent. + if schedule: + try: + schedule = _convert_schedule_to_utc_from_local(schedule, browser_timezone, request.user) + _determine_valid_schedule(schedule) + except ValueError as error: + error_message = f"Error occurred while attempting to create a scheduled bulk email task: {error}" + log.error(f"{error_message}") + return HttpResponseBadRequest(repr(error_message)) - template_name = configuration_helpers.get_value('course_email_template_name') - if isinstance(template_name, dict): - # If course_email_template_name is a dict, we are customizing - # the email template for each organization that has courses - # on the site. The dict maps template names by org allowing - # us to find the correct template to use here. - template_name = template_name.get(course_overview.display_org_with_default) + # Retrieve the customized email "from address" and email template from site configuration for the course/partner. If + # there is no site configuration enabled for the current site then we use system defaults for both. + from_addr = _get_branded_email_from_address(course_overview) + template_name = _get_branded_email_template(course_overview) - # Create the CourseEmail object. This is saved immediately, so that - # any transaction that has been pending up to this point will also be - # committed. - # TODO: convert to use bulk_email app's `create_course_email` API function and remove direct import and use of - # bulk_email model + # Create the CourseEmail object. This is saved immediately so that any transaction that has been pending up to this + # point will also be committed. try: - email = CourseEmail.create( + email = create_course_email( course_id, request.user, targets, subject, message, template_name=template_name, - from_addr=from_addr + from_addr=from_addr, ) except ValueError as err: - log.exception('Cannot create course email for course %s requested by user %s for targets %s', - course_id, request.user, targets) return HttpResponseBadRequest(repr(err)) # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) - task_api.submit_bulk_course_email(request, course_id, email.id) + task_api.submit_bulk_course_email(request, course_id, email.id, schedule) response_payload = { 'course_id': str(course_id), @@ -3560,3 +3557,91 @@ def _get_certificate_for_user(course_key, student): ) return certificate + + +def _get_branded_email_from_address(course_overview): + """ + Checks and retrieves a customized "from address", if one exists for the course/org. This is the email address that + learners will see the message coming from. + + Args: + course_overview (CourseOverview): The course overview instance for the course-run. + + Returns: + String: The customized "from address" to be used in messages sent by the bulk course email tool for this + course or org. + """ + from_addr = configuration_helpers.get_value('course_email_from_addr') + if isinstance(from_addr, dict): + # If course_email_from_addr is a dict, we are customizing the email template for each organization that has + # courses on the site. The dict maps from addresses by org allowing us to find the correct from address to use + # here. + from_addr = from_addr.get(course_overview.display_org_with_default) + + return from_addr + + +def _get_branded_email_template(course_overview): + """ + Checks and retrieves the custom email template, if one exists for the course/org, to style messages sent by the bulk + course email tool. + + Args: + course_overview (CourseOverview): The course overview instance for the course-run. + + Returns: + String: The name of the custom email template to use for this course or org. + """ + template_name = configuration_helpers.get_value('course_email_template_name') + if isinstance(template_name, dict): + # If course_email_template_name is a dict, we are customizing the email template for each organization that has + # courses on the site. The dict maps template names by org allowing us to find the correct template to use here. + template_name = template_name.get(course_overview.display_org_with_default) + + return template_name + + +def _convert_schedule_to_utc_from_local(schedule, browser_timezone, user): + """ + Utility function to help convert the schedule of an instructor task from the requesters local time and timezone + (taken from the request) to a UTC datetime. + + Args: + schedule (String): The desired time to execute a scheduled task, in local time, in the form of an ISOString. + timezone (String): The time zone, as captured by the user's web browser, in the form of a string. + user (User): The user requesting the action, captured from the originating web request. Used to lookup the + the time zone preference as set in the user's account settings. + + Returns: + DateTime: A datetime instance describing when to execute this schedule task converted to the UTC timezone. + """ + # look up the requesting user's timezone from their account settings + preferred_timezone = get_user_preference(user, 'time_zone', username=user.username) + # use the user's preferred timezone (if available), otherwise use the browser timezone. + timezone = preferred_timezone if preferred_timezone else browser_timezone + + # convert the schedule to UTC + log.info(f"Converting requested schedule from local time '{schedule}' with the timezone '{timezone}' to UTC") + + local_tz = dateutil.tz.gettz(timezone) + if local_tz is None: + raise ValueError( + "Unable to determine the time zone to use to convert the schedule to UTC" + ) + local_dt = dateutil.parser.parse(schedule).replace(tzinfo=local_tz) + schedule_utc = local_dt.astimezone(pytz.utc) + + return schedule_utc + + +def _determine_valid_schedule(schedule): + """ + Utility function that determines if the requested schedule is in the future. Raises ValueError if the schedule time + has already lapsed. + + Args: + schedule (DateTime): UTC DateTime representing the desired date and time to process a scheduled instructor task. + """ + now = datetime.datetime.now(pytz.utc) + if schedule < now: + raise ValueError(f"The requested schedule '{schedule}' is in the past") diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index c43ca04931..5ad5d4196e 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -22,6 +22,7 @@ from lms.djangoapps.instructor_task.api_helper import ( check_entrance_exam_problems_for_rescoring, encode_entrance_exam_and_student_input, encode_problem_and_student_input, + schedule_task, submit_task ) from lms.djangoapps.instructor_task.models import InstructorTask @@ -292,7 +293,7 @@ def submit_delete_entrance_exam_state_for_student(request, usage_key, student): return submit_task(request, task_type, task_class, usage_key.course_key, task_input, task_key) -def submit_bulk_course_email(request, course_key, email_id): +def submit_bulk_course_email(request, course_key, email_id, schedule=None): """ Request to have bulk email sent as a background task. @@ -321,6 +322,10 @@ def submit_bulk_course_email(request, course_key, email_id): task_key_stub = str(email_id) # create the key value by using MD5 hash: task_key = hashlib.md5(task_key_stub.encode('utf-8')).hexdigest() + + if schedule: + return schedule_task(request, task_type, course_key, task_input, task_key, schedule) + return submit_task(request, task_type, task_class, course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index fd0c842944..7df5a357bb 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -2,9 +2,11 @@ Test for LMS instructor background task queue management """ +import datetime from unittest.mock import MagicMock, Mock, patch import pytest +import pytz import ddt from celery.states import FAILURE @@ -389,3 +391,17 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa {}, '' ) + + @patch("lms.djangoapps.instructor_task.api.schedule_task") + @patch("lms.djangoapps.instructor_task.api.submit_task") + def test_submit_bulk_course_email_with_schedule(self, mock_submit_task, mock_schedule_task): + email_id = self._define_course_email() + schedule = datetime.datetime(2030, 8, 15, 8, 15, 12, 0, pytz.utc) + submit_bulk_course_email( + self.create_task_request(self.instructor), + self.course.id, + email_id, + schedule + ) + mock_schedule_task.assert_called_once() + mock_submit_task.assert_not_called()