New HTML content!
" + 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 diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index 3727472b33..3f3ffe9b69 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -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"], diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index f12cfdc429..08a27a0619 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -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): diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index a2fde5d20f..6ad7f2057c 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -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 diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 03ac9d0338..dbf87ea030 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -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 diff --git a/lms/djangoapps/instructor_task/rest_api/v1/exceptions.py b/lms/djangoapps/instructor_task/rest_api/v1/exceptions.py new file mode 100644 index 0000000000..5121206122 --- /dev/null +++ b/lms/djangoapps/instructor_task/rest_api/v1/exceptions.py @@ -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 diff --git a/lms/djangoapps/instructor_task/rest_api/v1/permissions.py b/lms/djangoapps/instructor_task/rest_api/v1/permissions.py index 7685e5624c..704fb9a2f1 100644 --- a/lms/djangoapps/instructor_task/rest_api/v1/permissions.py +++ b/lms/djangoapps/instructor_task/rest_api/v1/permissions.py @@ -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. """ diff --git a/lms/djangoapps/instructor_task/rest_api/v1/tests/test_views.py b/lms/djangoapps/instructor_task/rest_api/v1/tests/test_views.py index b2103b7733..398c7ad34b 100644 --- a/lms/djangoapps/instructor_task/rest_api/v1/tests/test_views.py +++ b/lms/djangoapps/instructor_task/rest_api/v1/tests/test_views.py @@ -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": "UpdatedMessage!!!
" + } + } + + 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": "Test message.
" + } + } + 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": "Test message.
" + } + } + 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) diff --git a/lms/djangoapps/instructor_task/rest_api/v1/urls.py b/lms/djangoapps/instructor_task/rest_api/v1/urls.py index a25915e19a..8b4064204c 100644 --- a/lms/djangoapps/instructor_task/rest_api/v1/urls.py +++ b/lms/djangoapps/instructor_task/rest_api/v1/urls.py @@ -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