Merge pull request #30408 from openedx/jhynes/microba-1510_update-email-api

feat: MICROBA-1510; add support for updating course email content and schedules
This commit is contained in:
Justin Hynes
2022-05-19 14:10:54 -04:00
committed by GitHub
14 changed files with 681 additions and 96 deletions

View File

@@ -7,7 +7,15 @@ import logging
from django.conf import settings
from django.urls import reverse
from lms.djangoapps.bulk_email.models import CourseEmail
from common.djangoapps.course_modes.models import CourseMode
from lms.djangoapps.bulk_email.data import BulkEmailTargetChoices
from lms.djangoapps.bulk_email.models import (
CohortTarget,
CourseEmail,
CourseModeTarget,
Target
)
from lms.djangoapps.bulk_email.models_api import (
is_bulk_email_disabled_for_course,
is_bulk_email_feature_enabled,
@@ -15,6 +23,7 @@ from lms.djangoapps.bulk_email.models_api import (
)
from lms.djangoapps.discussion.notification_prefs.views import UsernameCipher
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.lib.html_to_text import html_to_text
log = logging.getLogger(__name__)
@@ -57,7 +66,7 @@ def create_course_email(course_id, sender, targets, subject, html_message, text_
Args:
course_id (CourseKey): The CourseKey of the course.
sender (String): Email author.
targets (Target): Recipient groups the message should be sent to (e.g. SEND_TO_MYSELF)
targets (List[String]): Recipient groups the message should be sent to.
subject (String)): Email subject.
html_message (String): Email body. Includes HTML markup.
text_message (String, optional): Plaintext version of email body. Defaults to None.
@@ -85,6 +94,40 @@ def create_course_email(course_id, sender, targets, subject, html_message, text_
raise ValueError from err
def update_course_email(course_id, email_id, targets, subject, html_message, plaintext_message=None):
"""
Utility function that allows a course_email instance to be updated after it has been created.
course_id (CourseKey): The CourseKey of the course.
email_id (Int): The PK `id` value of the course_email instance that is to be updated.
targets (List[String]): Recipient groups the message should be sent to.
subject (String)): Email subject.
html_message (String): Email body. Includes HTML markup.
text_message (String, optional): Plaintext version of email body. Defaults to None.
"""
log.info(f"Updating course email with id '{email_id}' in course '{course_id}'")
# generate a new stripped version of the plaintext content from the HTML markup
if plaintext_message is None:
plaintext_message = html_to_text(html_message)
# update the targets for the message
new_targets = determine_targets_for_course_email(course_id, subject, targets)
if not new_targets:
raise ValueError("Must specify at least one target (recipient group) for a course email")
# get the course email and load into memory, update the fields individually since we have to update/set a M2M
# relationship on the instance
course_email = CourseEmail.objects.get(course_id=course_id, id=email_id)
course_email.subject = subject
course_email.html_message = html_message
course_email.text_message = plaintext_message
course_email.save()
# update the targets M2M relationship
course_email.targets.clear()
course_email.targets.add(*new_targets)
course_email.save()
def get_course_email(email_id):
"""
Utility function for retrieving a CourseEmail instance from a given CourseEmail id.
@@ -101,3 +144,37 @@ def get_course_email(email_id):
log.exception(f"CourseEmail instance with id '{email_id}' could not be found")
return None
def determine_targets_for_course_email(course_id, subject, targets):
"""
Utility function to determine the targets (recipient groups) selected by an author of a course email.
Historically, this used to be a piece of logic in the CourseEmail model's `create` function but has been extracted
here so it can be used by a REST API of the `instructor_task` app.
"""
new_targets = []
for target in targets:
# split target, to handle cohort:cohort_name and track:mode_slug
target_split = target.split(':', 1)
# Ensure our desired target exists
if not BulkEmailTargetChoices.is_valid_target(target_split[0]): # pylint: disable=no-else-raise
raise ValueError(
f"Course email being sent to an unrecognized target: '{target}' for '{course_id}', subject '{subject}'"
)
elif target_split[0] == BulkEmailTargetChoices.SEND_TO_COHORT:
# target_split[1] will contain the cohort name
cohort = CohortTarget.ensure_valid_cohort(target_split[1], course_id)
new_target, _ = CohortTarget.objects.get_or_create(target_type=target_split[0], cohort=cohort)
elif target_split[0] == BulkEmailTargetChoices.SEND_TO_TRACK:
# target_split[1] contains the desired mode slug
CourseModeTarget.ensure_valid_mode(target_split[1], course_id)
# There could exist multiple CourseModes that match this query, due to differing currency types.
# The currencies do not affect user lookup though, so we can just use the first result.
mode = CourseMode.objects.filter(course_id=course_id, mode_slug=target_split[1])[0]
new_target, _ = CourseModeTarget.objects.get_or_create(target_type=target_split[0], track=mode)
else:
new_target, _ = Target.objects.get_or_create(target_type=target_split[0])
new_targets.append(new_target)
return new_targets

View File

@@ -20,3 +20,12 @@ class BulkEmailTargetChoices:
SEND_TO_LEARNERS = "learners"
SEND_TO_COHORT = "cohort"
SEND_TO_TRACK = "track"
TARGET_CHOICES = (SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS, SEND_TO_COHORT, SEND_TO_TRACK)
@classmethod
def is_valid_target(cls, target):
"""
Given the target of a message, return a boolean indicating whether the target choice is valid.
"""
return target in cls.TARGET_CHOICES

View File

@@ -270,36 +270,16 @@ class CourseEmail(Email):
"""
Create an instance of CourseEmail.
"""
# deferred import to prevent a circular import issue
from lms.djangoapps.bulk_email.api import determine_targets_for_course_email
# automatically generate the stripped version of the text from the HTML markup:
if text_message is None:
text_message = html_to_text(html_message)
new_targets = []
for target in targets:
# split target, to handle cohort:cohort_name and track:mode_slug
target_split = target.split(':', 1)
# Ensure our desired target exists
if target_split[0] not in EMAIL_TARGETS: # lint-amnesty, pylint: disable=no-else-raise
fmt = 'Course email being sent to unrecognized target: "{target}" for "{course}", subject "{subject}"'
msg = fmt.format(target=target, course=course_id, subject=subject).encode('utf8')
raise ValueError(msg)
elif target_split[0] == SEND_TO_COHORT:
# target_split[1] will contain the cohort name
cohort = CohortTarget.ensure_valid_cohort(target_split[1], course_id)
new_target, _ = CohortTarget.objects.get_or_create(target_type=target_split[0], cohort=cohort)
elif target_split[0] == SEND_TO_TRACK:
# target_split[1] contains the desired mode slug
CourseModeTarget.ensure_valid_mode(target_split[1], course_id)
new_targets = determine_targets_for_course_email(course_id, subject, targets)
# There could exist multiple CourseModes that match this query, due to differing currency types.
# The currencies do not affect user lookup though, so we can just use the first result.
mode = CourseMode.objects.filter(course_id=course_id, mode_slug=target_split[1])[0]
new_target, _ = CourseModeTarget.objects.get_or_create(target_type=target_split[0], track=mode)
else:
new_target, _ = Target.objects.get_or_create(target_type=target_split[0])
new_targets.append(new_target)
# create the task, then save it immediately:
# create the course email instance, then save it immediately:
course_email = cls(
course_id=course_id,
sender=sender,

View File

@@ -6,17 +6,25 @@ from testfixtures import LogCapture
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
from common.djangoapps.course_modes.models import CourseMode
from common.djangoapps.student.tests.factories import InstructorFactory
from lms.djangoapps.bulk_email.api import create_course_email, get_course_email
from lms.djangoapps.bulk_email.api import (
create_course_email,
determine_targets_for_course_email,
get_course_email,
update_course_email
)
from lms.djangoapps.bulk_email.data import BulkEmailTargetChoices
from lms.djangoapps.bulk_email.models import CourseEmail
from openedx.core.djangoapps.course_groups.models import CourseUserGroup
from openedx.core.lib.html_to_text import html_to_text
LOG_PATH = "lms.djangoapps.bulk_email.api"
class CreateCourseEmailTests(ModuleStoreTestCase):
class BulkEmailApiTests(ModuleStoreTestCase):
"""
Tests for the `create_course_email` function of the bulk email app's public Python API.
Tests for Python functions in the bulk_email app's public `api.py` that interact with CourseEmail instances.
"""
def setUp(self):
super().setUp()
@@ -114,6 +122,7 @@ class CreateCourseEmailTests(ModuleStoreTestCase):
email_instance = get_course_email(course_email.id)
assert email_instance.id == course_email.id
# Next, try and retrieve a CourseEmail instance that does not exist.
email_id_dne = 3463435
expected_message = (
f"CourseEmail instance with id '{email_id_dne}' could not be found"
@@ -124,3 +133,132 @@ class CreateCourseEmailTests(ModuleStoreTestCase):
log.check_present(
(LOG_PATH, "ERROR", expected_message),
)
def test_update_course_email(self):
"""
A test that verifies the ability to update a CourseEmail instance when using the public `update_course_email`
function.
"""
course_email = create_course_email(
self.course.id,
self.instructor,
self.target,
self.subject,
self.html_message,
)
updated_targets = [BulkEmailTargetChoices.SEND_TO_MYSELF, BulkEmailTargetChoices.SEND_TO_STAFF]
updated_subject = "New Email Subject"
updated_html_message = "<p>New HTML content!</p>"
expected_plaintext_message = html_to_text(updated_html_message)
update_course_email(
self.course.id,
course_email.id,
updated_targets,
updated_subject,
updated_html_message
)
updated_course_email = CourseEmail.objects.get(id=course_email.id)
assert updated_course_email.subject == updated_subject
assert updated_course_email.html_message == updated_html_message
assert updated_course_email.text_message == expected_plaintext_message
email_targets = updated_course_email.targets.values_list('target_type', flat=True)
for target in updated_targets:
assert target in email_targets
assert True
# update course email but provide the plaintext content this time
plaintext_message = "I am a plaintext message"
update_course_email(
self.course.id,
course_email.id,
updated_targets,
updated_subject,
updated_html_message,
plaintext_message=plaintext_message
)
updated_course_email = CourseEmail.objects.get(id=course_email.id)
assert updated_course_email.text_message == plaintext_message
def test_update_course_email_no_targets_expect_error(self):
"""
A test that verifies an expected error occurs when bad data is passed to the `update_course_email` function.
"""
course_email = create_course_email(
self.course.id,
self.instructor,
self.target,
self.subject,
self.html_message,
)
with self.assertRaises(ValueError):
update_course_email(
self.course.id,
course_email.id,
[],
self.subject,
self.html_message,
)
def test_determine_targets_for_course_email(self):
"""
A test to verify the basic functionality of the `determine_targets_for_course_email` function.
"""
# create cohort in our test course-run
cohort_name = "TestCohort"
cohort = CourseUserGroup.objects.create(
name=cohort_name,
course_id=self.course.id,
group_type=CourseUserGroup.COHORT
)
# create a track called 'test' in our test course-run
slug = "test-track"
CourseMode.objects.create(mode_slug=slug, course_id=self.course.id)
targets = [
BulkEmailTargetChoices.SEND_TO_MYSELF,
BulkEmailTargetChoices.SEND_TO_STAFF,
BulkEmailTargetChoices.SEND_TO_LEARNERS,
f"{BulkEmailTargetChoices.SEND_TO_COHORT}:{cohort.name}",
f"{BulkEmailTargetChoices.SEND_TO_TRACK}:{slug}"
]
returned_targets = determine_targets_for_course_email(
self.course.id,
self.subject,
targets
)
# verify all the targets we expect are there by building a list of the expected (short) display names of each
# target
expected_targets = [
BulkEmailTargetChoices.SEND_TO_MYSELF,
BulkEmailTargetChoices.SEND_TO_STAFF,
BulkEmailTargetChoices.SEND_TO_LEARNERS,
f"cohort-{cohort_name}",
f"track-{slug}"
]
for target in returned_targets:
assert target.short_display() in expected_targets
def test_determine_targets_for_course_email_invalid_target_expect_error(self):
"""
A test to verify an expected error is thrown when invalid target data is sent to the
`determine_targets_for_course` function.
"""
targets = ["OtherGroup"]
expected_error_msg = (
f"Course email being sent to an unrecognized target: '{targets[0]}' for '{self.course.id}', "
f"subject '{self.subject}'"
)
with self.assertRaises(ValueError) as value_err:
determine_targets_for_course_email(
self.course.id,
self.subject,
targets
)
assert str(value_err.exception) == expected_error_msg

View File

@@ -206,8 +206,8 @@ class TestEmailErrors(ModuleStoreTestCase):
"""
Tests exception when the to_option in the email doesn't exist
"""
with self.assertRaisesRegex(ValueError, 'Course email being sent to unrecognized target: "IDONTEXIST" *'):
email = CourseEmail.create( # pylint: disable=unused-variable
with self.assertRaisesRegex(ValueError, "Course email being sent to an unrecognized target: 'IDONTEXIST' *"):
CourseEmail.create(
self.course.id,
self.instructor,
["IDONTEXIST"],
@@ -221,7 +221,7 @@ class TestEmailErrors(ModuleStoreTestCase):
Tests exception when the cohort or course mode doesn't exist
"""
with self.assertRaisesRegex(ValueError, '.* IDONTEXIST does not exist .*'):
email = CourseEmail.create( # pylint: disable=unused-variable
CourseEmail.create(
self.course.id,
self.instructor,
[f"{target_type}:IDONTEXIST"],

View File

@@ -3500,20 +3500,13 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm
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)
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):

View File

@@ -14,7 +14,6 @@ import string
import random
import re
import dateutil
import pytz
import edx_api_doc_tools as apidocs
from django.conf import settings
@@ -90,6 +89,7 @@ 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,
@@ -2714,7 +2714,7 @@ def send_email(request, course_id):
# orphaned email object that will never be sent.
if schedule:
try:
schedule = _convert_schedule_to_utc_from_local(schedule, browser_timezone, request.user)
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}"
@@ -3602,39 +3602,6 @@ def _get_branded_email_template(course_overview):
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

View File

@@ -6,18 +6,17 @@ already been submitted, filtered either by running state or input
arguments.
"""
import datetime
import hashlib
import logging
from collections import Counter
import dateutil
import pytz
from celery.states import READY_STATES
from common.djangoapps.util import milestones_helpers
from lms.djangoapps.bulk_email.models import CourseEmail
from lms.djangoapps.bulk_email.api import get_course_email
from lms.djangoapps.certificates.models import CertificateGenerationHistory
from lms.djangoapps.instructor_task.api_helper import (
QueueConnectionError,
@@ -52,6 +51,7 @@ 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__)
@@ -315,7 +315,7 @@ def submit_bulk_course_email(request, course_key, email_id, schedule=None):
# appropriate access to the course. But make sure that the email exists.
# We also pull out the targets argument here, so that is displayed in
# the InstructorTask status.
email_obj = CourseEmail.objects.get(id=email_id)
email_obj = get_course_email(email_id)
# task_input has a limit to the size it can store, so any target_type with count > 1 is combined and counted
targets = Counter([target.target_type for target in email_obj.targets.all()])
targets = [
@@ -588,3 +588,37 @@ def process_scheduled_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

View File

@@ -0,0 +1,12 @@
"""
Exception classes for Instructor Task REST APIs.
"""
class TaskUpdateException(Exception):
"""
An exception that occurs when trying to update an instructor task instance. This covers scenarios where an updated
task schedule is not valid, or we are trying to update a task that has already been processed (is no longer in the
`SCHEDULED` state).
"""
pass # pylint: disable=unnecessary-pass

View File

@@ -6,7 +6,7 @@ from rest_framework import permissions
from common.djangoapps.student.api import is_user_staff_or_instructor_in_course
class CanViewOrDeleteScheduledBulkCourseEmails(permissions.BasePermission):
class CanViewOrModifyScheduledBulkCourseEmailTasks(permissions.BasePermission):
"""
Permission class that ensures a user is allowed to interact with the bulk course messages in a given course-run.
"""

View File

@@ -1,8 +1,10 @@
"""
Tests for the instructor_task app's REST API v1 views.
"""
import datetime
import json
from uuid import uuid4
import pytz
from celery.states import REVOKED
import ddt
@@ -17,11 +19,12 @@ from common.djangoapps.student.tests.factories import (
StaffFactory,
UserFactory,
)
from lms.djangoapps.bulk_email.api import create_course_email
from lms.djangoapps.bulk_email.api import create_course_email, get_course_email
from lms.djangoapps.bulk_email.data import BulkEmailTargetChoices
from lms.djangoapps.instructor_task.data import InstructorTaskTypes
from lms.djangoapps.instructor_task.models import InstructorTask, PROGRESS, SCHEDULED
from lms.djangoapps.instructor_task.models import InstructorTask, InstructorTaskSchedule, PROGRESS, SCHEDULED
from lms.djangoapps.instructor_task.tests.factories import InstructorTaskFactory, InstructorTaskScheduleFactory
from openedx.core.lib.html_to_text import html_to_text
User = get_user_model()
@@ -105,6 +108,13 @@ class TestScheduledBulkEmailAPIViews(APITestCase, ModuleStoreTestCase):
task_args=json.dumps(task_args),
)
def _assert_error_code_and_message(self, response, expected_error_code, expected_error_msg):
"""
Utility function to verify expected error code and messages from a response.
"""
assert response.status_code == expected_error_code
assert response.data.get("detail") == expected_error_msg
@ddt.data(
("globalstaff", 200),
("instructor_course1", 200),
@@ -151,7 +161,7 @@ class TestScheduledBulkEmailAPIViews(APITestCase, ModuleStoreTestCase):
def test_delete_schedules(self):
"""
Test case that verifies the functionality of the DeleteScheduledBulkEmailInstructorTask view.
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test verifies that, when a task schedule is cancelled, the data is in the correct state and that the task
is no longer returned with GET requests.
@@ -182,10 +192,198 @@ class TestScheduledBulkEmailAPIViews(APITestCase, ModuleStoreTestCase):
def test_delete_schedules_schedule_does_not_exist(self):
"""
Test case that verifies the functionality of the DeleteScheduledBulkEmailInstructorTask view.
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test verifies the response received when we try to delete a task schedule that does not exist.
"""
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.delete(f"{self._build_api_url(self.course1.id)}123456789")
assert response.status_code == 404
def test_delete_schedule_task_already_processed(self):
"""
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test verifies the response received when we try to delete a schedule for a task that has already been
processed by Celery.
"""
self._create_scheduled_course_emails_for_course(self.course1.id, self.instructor_course1, PROGRESS, 1)
task = InstructorTask.objects.get(course_id=self.course1.id)
schedule = InstructorTaskSchedule.objects.get(task=task)
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.delete(f"{self._build_api_url(self.course1.id)}{schedule.id}")
assert response.status_code == 400
def test_update_schedule_new_date(self):
"""
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test verifies the ability to update the date and time of a schedule.
"""
self._create_scheduled_course_emails_for_course(self.course1.id, self.instructor_course1, SCHEDULED, 1)
task = InstructorTask.objects.get(course_id=self.course1.id)
task_schedule = InstructorTaskSchedule.objects.get(task=task)
schedule_datetime = datetime.datetime(3000, 6, 1, 17, 15, 0, tzinfo=pytz.utc)
data = {
"schedule": schedule_datetime.strftime('%Y-%m-%dT%H:%M:%SZ'),
"browser_timezone": "UTC"
}
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.patch(
f"{self._build_api_url(self.course1.id)}{task_schedule.id}",
data=json.dumps(data),
content_type="application/json"
)
updated_schedule = InstructorTaskSchedule.objects.get(task=task)
assert response.status_code == 200
assert updated_schedule.task_due == schedule_datetime
def test_update_schedule_edit_email(self):
"""
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test verifies the ability to modify the contents of an email after it has been scheduled.
"""
self._create_scheduled_course_emails_for_course(self.course1.id, self.instructor_course1, SCHEDULED, 1)
task = InstructorTask.objects.get(course_id=self.course1.id)
task_schedule = InstructorTaskSchedule.objects.get(task=task)
email_id = json.loads(task.task_input).get("email_id")
data = {
"email": {
"id": email_id,
"targets": ["myself", "staff"],
"subject": "UpdatedSubject",
"message": "<p>UpdatedMessage!!!</p>"
}
}
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.patch(
f"{self._build_api_url(self.course1.id)}{task_schedule.id}",
data=json.dumps(data),
content_type="application/json"
)
course_email = get_course_email(email_id)
targets_list = course_email.targets.values_list("target_type", flat=True)
assert response.status_code == 200
assert course_email.subject == data.get("email").get("subject")
assert course_email.html_message == data.get("email").get("message")
assert course_email.text_message == html_to_text(data.get("email").get("message"))
assert len(targets_list) == 2
assert targets_list[0] == BulkEmailTargetChoices.SEND_TO_MYSELF
assert targets_list[1] == BulkEmailTargetChoices.SEND_TO_STAFF
def test_update_schedule_bad_date(self):
"""
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test ensures that we don't accept dates in the past when updating a task schedule.
"""
self._create_scheduled_course_emails_for_course(self.course1.id, self.instructor_course1, SCHEDULED, 1)
task = InstructorTask.objects.get(course_id=self.course1.id)
task_schedule = InstructorTaskSchedule.objects.get(task=task)
data = {
"schedule": "1999-05-10T17:15:00.000Z",
"browser_timezone": "UTC"
}
expected_error_msg = (
f"Cannot update instructor task schedule '{task_schedule.id}', the updated schedule occurs in the past"
)
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.patch(
f"{self._build_api_url(self.course1.id)}{task_schedule.id}",
data=json.dumps(data),
content_type="application/json"
)
self._assert_error_code_and_message(response, 400, expected_error_msg)
def test_update_schedule_bad_email_target_data(self):
"""
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test verifies error handling by simulating bad email data (bad target data).
"""
self._create_scheduled_course_emails_for_course(self.course1.id, self.instructor_course1, SCHEDULED, 1)
task = InstructorTask.objects.get(course_id=self.course1.id)
task_schedule = InstructorTaskSchedule.objects.get(task=task)
email_id = json.loads(task.task_input).get("email_id")
data = {
"email": {
"id": email_id,
"targets": ["pizzasteve"],
"subject": f"Test Subject{email_id}",
"message": "<p>Test message.</p>"
}
}
expected_error_msg = (
f"Course email being sent to an unrecognized target: 'pizzasteve' for '{self.course1.id}', subject 'Test "
f"Subject{email_id}'"
)
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.patch(
f"{self._build_api_url(self.course1.id)}{task_schedule.id}",
data=json.dumps(data),
content_type="application/json"
)
self._assert_error_code_and_message(response, 400, expected_error_msg)
def test_update_schedule_mismatched_email_id(self):
"""
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test ensures that we will not update an email if it is not associated with the correct task/schedule.
"""
self._create_scheduled_course_emails_for_course(self.course1.id, self.instructor_course1, SCHEDULED, 1)
task = InstructorTask.objects.get(course_id=self.course1.id)
task_schedule = InstructorTaskSchedule.objects.get(task=task)
data = {
"email": {
"id": 8663863102,
"targets": ["myself"],
"subject": "Test Subject!",
"message": "<p>Test message.</p>"
}
}
expected_error_msg = (
f"Cannot update instructor task '{task.id} for course '{self.course1.id}', the email id '8663863102' "
"specified in the request does not match the email id associated with the task"
)
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.patch(
f"{self._build_api_url(self.course1.id)}{task_schedule.id}",
data=json.dumps(data),
content_type="application/json"
)
self._assert_error_code_and_message(response, 400, expected_error_msg)
def test_update_schedule_dne(self):
"""
Test case that verifies the functionality of the ModifyScheduledBulkEmailInstructorTask view.
This test ensures behavior when trying to update a task schedule that does not exist.
"""
data = {
"schedule": "3000-05-10T17:15:00.000Z",
"browser_timezone": "UTC"
}
self.client.login(username=self.instructor_course1.username, password="test")
response = self.client.patch(
f"{self._build_api_url(self.course1.id)}8675309",
data=json.dumps(data),
content_type="application/json"
)
expected_error_msg = (
"Cannot update instructor task schedule '8675309', a schedule with this ID does not exist"
)
self._assert_error_code_and_message(response, 404, expected_error_msg)

View File

@@ -6,7 +6,7 @@ from django.urls import re_path
from lms.djangoapps.instructor_task.rest_api.v1.views import (
ListScheduledBulkEmailInstructorTasks,
DeleteScheduledBulkEmailInstructorTask
ModifyScheduledBulkEmailInstructorTask
)
@@ -18,7 +18,7 @@ urlpatterns = [
),
re_path(
fr"schedules/{settings.COURSE_ID_PATTERN}/bulk_email/(?P<schedule_id>[0-9]+)$",
DeleteScheduledBulkEmailInstructorTask.as_view(),
name="delete-scheduled-bulk-email-messages"
ModifyScheduledBulkEmailInstructorTask.as_view(),
name="modify-scheduled-bulk-email-messages"
)
]

View File

@@ -1,18 +1,25 @@
"""
Instructor Task Django app REST API views.
"""
import datetime
import json
import logging
import pytz
from celery.states import REVOKED
from django.db import transaction
from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication
from rest_framework.authentication import SessionAuthentication
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
from lms.djangoapps.instructor_task.rest_api.v1.serializers import ScheduledBulkEmailSerializer
from lms.djangoapps.instructor_task.rest_api.v1.permissions import CanViewOrDeleteScheduledBulkCourseEmails
from lms.djangoapps.instructor_task.rest_api.v1.permissions import CanViewOrModifyScheduledBulkCourseEmailTasks
log = logging.getLogger(__name__)
@@ -33,7 +40,7 @@ class ListScheduledBulkEmailInstructorTasks(generics.ListAPIView):
SessionAuthentication,
)
permission_classes = (
CanViewOrDeleteScheduledBulkCourseEmails,
CanViewOrModifyScheduledBulkCourseEmailTasks,
)
serializer_class = ScheduledBulkEmailSerializer
@@ -47,26 +54,34 @@ class ListScheduledBulkEmailInstructorTasks(generics.ListAPIView):
.filter(task__course_id=course_id)
.filter(task__task_state=SCHEDULED)
.filter(task__task_type=InstructorTaskTypes.BULK_COURSE_EMAIL)
.order_by('id')
)
class DeleteScheduledBulkEmailInstructorTask(generics.DestroyAPIView):
class ModifyScheduledBulkEmailInstructorTask(generics.DestroyAPIView, generics.UpdateAPIView):
"""
A view that deletes an instructor task schedule instance and revokes the associated instructor task.
A view that supports the modification of instructor task schedules. It provides the ability to:
* Delete an instructor task schedule
* Update an instructor task schedule and/or update the course email associated with the scheduled task.
Path: DELETE `api/instructor_task/v1/schedules/{course_id}/bulk_email/{task_schedule_id}`
Path: DELETE or PATCH `api/instructor_task/v1/schedules/{course_id}/bulk_email/{task_schedule_id}`
Returns:
* 200: The schedule or email content was updated successfully.
* 204: No Content - Deleting the schedule was successful.
* 404: Requested schedule object could not be found and thus could not be deleted.
* 400: Bad request - Updating the email content or schedule failed due to bad data in the request.
* 403: User does not have permission to modify the object specified.
* 404: Requested schedule object could not be found and thus could not be modified or removed.
"""
authentication_classes = (
JwtAuthentication,
SessionAuthentication,
)
permission_classes = (
CanViewOrDeleteScheduledBulkCourseEmails,
CanViewOrModifyScheduledBulkCourseEmailTasks,
)
serializer_class = ScheduledBulkEmailSerializer
def destroy(self, request, *args, **kwargs):
course_id = kwargs["course_id"]
@@ -78,11 +93,96 @@ class DeleteScheduledBulkEmailInstructorTask(generics.DestroyAPIView):
except InstructorTaskSchedule.DoesNotExist:
return Response(status=status.HTTP_404_NOT_FOUND)
# update the task's status to REVOKED and then delete the task schedule instance
task = schedule.task
# verify the task hasn't already been processed before revoking
try:
self._verify_task_state(task)
except TaskUpdateException as task_update_err:
return Response(
{"detail": str(task_update_err)},
status=status.HTTP_400_BAD_REQUEST
)
# update the task's status to REVOKED and then delete the task schedule instance
log.info(f"Revoking instructor task with id '{task.id}' for course '{task.course_id}'")
task.task_state = REVOKED
task.save()
schedule.delete()
return Response(status=status.HTTP_204_NO_CONTENT)
def patch(self, request, *args, **kwargs):
course_id = kwargs["course_id"]
schedule_id = kwargs["schedule_id"]
# extract schedule and email data from the request
schedule = request.data.get("schedule", None)
email_data = request.data.get("email", None)
try:
task_schedule = InstructorTaskSchedule.objects.get(id=schedule_id)
task = task_schedule.task
self._verify_task_state(task)
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
task_schedule.save()
if email_data:
email_id = email_data.get("id")
self._verify_task_and_email_associated(task, email_id)
targets = email_data.get("targets")
subject = email_data.get("subject")
message = email_data.get("message")
update_course_email(course_id, email_id, targets, subject, message)
except (TaskUpdateException, ValueError) as task_update_err:
return Response(
{"detail": str(task_update_err)},
status=status.HTTP_400_BAD_REQUEST
)
except InstructorTaskSchedule.DoesNotExist:
error_message = (
f"Cannot update instructor task schedule '{schedule_id}', a schedule with this ID does not exist"
)
return Response(
{"detail": error_message},
status=status.HTTP_404_NOT_FOUND
)
else:
return Response(ScheduledBulkEmailSerializer(instance=task_schedule).data)
def _verify_valid_schedule(self, schedule_id, schedule):
"""
Verifies that the updated schedule data for the task is valid. We check to make sure that the date or time
requested is not in the past.
"""
now = datetime.datetime.now(pytz.utc)
if schedule < now:
raise TaskUpdateException(
f"Cannot update instructor task schedule '{schedule_id}', the updated schedule occurs in the past"
)
def _verify_task_state(self, task):
"""
Verifies that the task is still in the `SCHEDULED` state. If we have already started (or finished) processing
the task then there is no need to update it.
"""
if task.task_state != SCHEDULED:
raise TaskUpdateException(
f"Cannot update instructor task '{task.id}' for course '{task.course_id}', this task has already been "
"processed"
)
def _verify_task_and_email_associated(self, task, email_id):
"""
Verifies that the email we are trying to update is actually associated with the task in question.
"""
email_id_from_task = json.loads(task.task_input).get("email_id", None)
if email_id_from_task != email_id:
raise TaskUpdateException(
f"Cannot update instructor task '{task.id} for course '{task.course_id}', the email id '{email_id}' "
f"specified in the request does not match the email id associated with the task"
)

View File

@@ -7,6 +7,7 @@ import json
from unittest.mock import MagicMock, Mock, patch
from uuid import uuid4
import dateutil
import pytest
import pytz
import ddt
@@ -21,6 +22,7 @@ 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,
@@ -61,6 +63,7 @@ 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'
@@ -528,3 +531,77 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa
process_scheduled_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