Merge pull request #30192 from openedx/jhynes/microba-1507_scheduled-tasks

feat: update existing functions to support scheduled email tasks

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

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")

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)
 
 

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()