Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: fix issue with incorrect bulk email schedules

Browse Source 
[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.
This commit is contained in:
Justin Hynes
2022-06-01 08:40:36 -04:00
parent 05f54e8074
commit d7ae3181b6
5 changed files with 37 additions and 228 deletions
Download Patch File Download Diff File Expand all files Collapse all files

100
lms/djangoapps/instructor/tests/test_api.py
View File

@@ -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:

43
lms/djangoapps/instructor/views/api.py
View File

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

36
lms/djangoapps/instructor_task/api.py
View File

@@ -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

9
lms/djangoapps/instructor_task/rest_api/v1/views.py
View File

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

77
lms/djangoapps/instructor_task/tests/test_api.py
View File

@@ -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