From d7ae3181b6355b700c1ba1c0983eb6a597badd0c Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Wed, 1 Jun 2022 08:40:36 -0400 Subject: [PATCH] fix: fix issue with incorrect bulk email schedules [MICROBA-1835] * The DateTime string received from the Comms MFE was already in UTC so there is no need to convert the schedule to UTC on the backend. --- lms/djangoapps/instructor/tests/test_api.py | 100 +++--------------- lms/djangoapps/instructor/views/api.py | 43 +++----- lms/djangoapps/instructor_task/api.py | 36 ------- .../instructor_task/rest_api/v1/views.py | 9 +- .../instructor_task/tests/test_api.py | 77 -------------- 5 files changed, 37 insertions(+), 228 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 08a27a0619..92d9625f1c 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -95,7 +95,7 @@ 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.djangoapps.user_api.preferences.api import 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 @@ -3399,11 +3399,6 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm 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) @@ -3491,15 +3486,13 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm 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): + def test_send_email_with_schedule(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" + schedule = "2099-05-02T14:00:00.000Z" self.full_test_message['schedule'] = schedule - self.full_test_message['browser_timezone'] = timezone - expected_schedule = self._get_expected_schedule(schedule, timezone) + expected_schedule = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) url = reverse('send_email', kwargs={'course_id': str(self.course.id)}) response = self.client.post(url, self.full_test_message) @@ -3509,95 +3502,36 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm assert arg_schedule == expected_schedule @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): + def test_send_email_with_malformed_schedule_expect_error(self, mock_task_api): + schedule = "Blub Glub" 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", - ] + expected_message = ( + f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: 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]), - ) + log.check_present((LOG_PATH, "ERROR", expected_message),) + mock_task_api.assert_not_called() 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" - ] + expected_message = ( + f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: the " + "requested 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]), - ) + log.check_present((LOG_PATH, "ERROR", expected_message),) class MockCompletionInfo: diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 6ad7f2057c..8388fffb4f 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -14,6 +14,7 @@ import string import random import re +import dateutil import pytz import edx_api_doc_tools as apidocs from django.conf import settings @@ -89,7 +90,6 @@ from lms.djangoapps.discussion.django_comment_client.utils import ( ) from lms.djangoapps.instructor import enrollment from lms.djangoapps.instructor.access import ROLES, allow_access, list_with_level, revoke_access, update_forum_role -from lms.djangoapps.instructor_task.api import convert_schedule_to_utc_from_local from lms.djangoapps.instructor.constants import INVOICE_KEY from lms.djangoapps.instructor.enrollment import ( enroll_email, @@ -2698,7 +2698,7 @@ def send_email(request, 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) + log.warning(f"Email is not enabled for course {course_id}") return HttpResponseForbidden("Email is not enabled for this course.") targets = json.loads(request.POST.get("send_to")) @@ -2706,20 +2706,22 @@ def send_email(request, course_id): 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", "") - # 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. + schedule_dt = None 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)) + # convert the schedule from a string to a datetime, then check if its a valid future date and time, dateutil + # will throw a ValueError if the schedule is no good. + schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) + if schedule_dt < datetime.datetime.now(pytz.utc): + raise ValueError("the requested schedule is in the past") + except ValueError as value_error: + error_message = ( + f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: " + f"{value_error}" + ) + log.error(error_message) + return HttpResponseBadRequest(error_message) # 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. @@ -2742,7 +2744,7 @@ def send_email(request, course_id): 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, schedule) + task_api.submit_bulk_course_email(request, course_id, email.id, schedule_dt) response_payload = { 'course_id': str(course_id), @@ -3600,16 +3602,3 @@ def _get_branded_email_template(course_overview): template_name = template_name.get(course_overview.display_org_with_default) return template_name - - -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 03c4d29b0a..bbf5bf7b28 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -11,7 +11,6 @@ import hashlib import logging from collections import Counter -import dateutil import pytz from celery.states import READY_STATES @@ -51,7 +50,6 @@ from lms.djangoapps.instructor_task.tasks import ( send_bulk_course_email, generate_anonymous_ids_for_course ) -from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order log = logging.getLogger(__name__) @@ -589,37 +587,3 @@ def process_scheduled_instructor_tasks(): submit_scheduled_task(schedule) except QueueConnectionError as exc: log.error(f"Error processing scheduled task with task id '{schedule.task.id}': {exc}") - - -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 ISO8601 - string. - 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 diff --git a/lms/djangoapps/instructor_task/rest_api/v1/views.py b/lms/djangoapps/instructor_task/rest_api/v1/views.py index 5b1664dcd1..10ebfb21b3 100644 --- a/lms/djangoapps/instructor_task/rest_api/v1/views.py +++ b/lms/djangoapps/instructor_task/rest_api/v1/views.py @@ -6,6 +6,7 @@ import json import logging import pytz +import dateutil from celery.states import REVOKED from django.db import transaction from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication @@ -14,7 +15,6 @@ from rest_framework.response import Response from rest_framework import generics, status from lms.djangoapps.bulk_email.api import update_course_email -from lms.djangoapps.instructor_task.api import convert_schedule_to_utc_from_local from lms.djangoapps.instructor_task.data import InstructorTaskTypes from lms.djangoapps.instructor_task.models import InstructorTaskSchedule, SCHEDULED from lms.djangoapps.instructor_task.rest_api.v1.exceptions import TaskUpdateException @@ -126,10 +126,9 @@ class ModifyScheduledBulkEmailInstructorTask(generics.DestroyAPIView, generics.U with transaction.atomic(): if schedule: - browser_timezone = request.data.get("browser_timezone", None) - schedule_utc = convert_schedule_to_utc_from_local(schedule, browser_timezone, request.user) - self._verify_valid_schedule(schedule_id, schedule_utc) - task_schedule.task_due = schedule_utc + schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) + self._verify_valid_schedule(schedule_id, schedule_dt) + task_schedule.task_due = schedule_dt task_schedule.save() if email_data: email_id = email_data.get("id") diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 79d2184582..4b84fbff7f 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -7,7 +7,6 @@ import json from unittest.mock import MagicMock, Mock, patch from uuid import uuid4 -import dateutil import pytest import pytz import ddt @@ -22,7 +21,6 @@ from lms.djangoapps.bulk_email.data import BulkEmailTargetChoices from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.certificates.models import CertificateGenerationHistory from lms.djangoapps.instructor_task.api import ( - convert_schedule_to_utc_from_local, SpecificStudentIdMissingError, generate_anonymous_ids, generate_certificates_for_students, @@ -63,7 +61,6 @@ from lms.djangoapps.instructor_task.tests.test_base import ( InstructorTaskTestCase, TestReportMixin ) -from openedx.core.djangoapps.user_api.preferences.api import set_user_preference, delete_user_preference LOG_PATH = 'lms.djangoapps.instructor_task.api' @@ -531,77 +528,3 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa process_scheduled_instructor_tasks() log.check_present((LOG_PATH, "ERROR", expected_messages[0]),) - - -@patch('lms.djangoapps.bulk_email.models.html_to_text', Mock(return_value='Mocking CourseEmail.text_message', autospec=True)) # lint-amnesty, pylint: disable=line-too-long -class ScheduledInstructorTaskTests(TestReportMixin, InstructorTaskCourseTestCase): - """ - Tests API methods that support scheduled instructor tasks - """ - def setUp(self): - super().setUp() - self.instructor = UserFactory.create(username="instructor", email="instructor@edx.org") - - def tearDown(self): - super().tearDown() - delete_user_preference(self.instructor, 'time_zone', 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_convert_schedule_to_utc_from_local_no_user_preference(self): - """ - A test that verifies the behavior of the `convert_schedule_to_utc_from_local` function. Verifies that we use - the browser provided timezone data if there is no preferred timezone set by the user in their account settings. - """ - schedule = "2099-05-02T14:00:00.000Z" - browser_timezone = "America/New_York" - expected_schedule = self._get_expected_schedule(schedule, browser_timezone) - - schedule_utc = convert_schedule_to_utc_from_local(schedule, browser_timezone, self.instructor) - - assert schedule_utc == expected_schedule - - def test_convert_schedule_to_utc_from_local_with_preferred_timezone(self): - """ - A test that verifies the behavior of the `convert_schedule_to_utc_from_local` function. Verifies that we use the - preferred timezone if a user has set one in their account settings. - """ - schedule = "2099-05-02T14:00:00.000Z" - preferred_timezone = "America/Anchorage" - set_user_preference(self.instructor, 'time_zone', preferred_timezone) - expected_schedule = self._get_expected_schedule(schedule, preferred_timezone) - - schedule_utc = convert_schedule_to_utc_from_local(schedule, "", self.instructor) - - assert schedule_utc == expected_schedule - - def test_convert_schedule_to_utc_from_local_with_preferred_and_browser_timezone(self): - """ - A test that verifies the behavior of the `convert_schedule_to_utc_from_local` function. Verifies that we use - the preferred timezone over the browser timezone when provided both values. - """ - schedule = "2099-05-02T14:00:00.000Z" - browser_timezone = "America/New_York" - preferred_timezone = "America/Anchorage" - set_user_preference(self.instructor, 'time_zone', preferred_timezone) - expected_schedule = self._get_expected_schedule(schedule, preferred_timezone) - - schedule_utc = convert_schedule_to_utc_from_local(schedule, browser_timezone, self.instructor) - - assert schedule_utc == expected_schedule - - def test_convert_schedule_to_utc_from_local_with_invalid_timezone(self): - """ - A test that verifies the behavior of the `convert_schedule_to_utc_from_local` function. Verifies an error - condition if the application cannot determine a timezone to use for conversion. - """ - schedule = "2099-05-02T14:00:00.000Z" - expected_error_message = "Unable to determine the time zone to use to convert the schedule to UTC" - - with self.assertRaises(ValueError) as value_err: - convert_schedule_to_utc_from_local(schedule, "Flim/Flam", self.instructor) - - assert str(value_err.exception) == expected_error_message