diff --git a/common/djangoapps/util/json_request.py b/common/djangoapps/util/json_request.py index 8cc188ec2b..593723c37d 100644 --- a/common/djangoapps/util/json_request.py +++ b/common/djangoapps/util/json_request.py @@ -66,7 +66,7 @@ class JsonResponse(HttpResponse): elif isinstance(resp_obj, QuerySet): content = serialize('json', resp_obj) else: - content = json.dumps(resp_obj, cls=encoder, indent=2, ensure_ascii=False) + content = json.dumps(resp_obj, cls=encoder, indent=2, ensure_ascii=True) kwargs.setdefault("content_type", "application/json") if status: kwargs["status"] = status diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index 8c9af2226f..e73941691c 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -127,12 +127,10 @@ class BulkEmailPage(PageObject): """ Selects the specified recipient from the selector. Assumes that recipient is not None. """ - recipient_selector_css = "select[name='send_to']" - select_option_by_text( - self.q(css=self._bounded_selector(recipient_selector_css)), recipient - ) + recipient_selector_css = "input[name='send_to'][value='{}']".format(recipient) + self.q(css=self._bounded_selector(recipient_selector_css))[0].click() - def send_message(self, recipient): + def send_message(self, recipients): """ Send a test message to the specified recipient. """ @@ -140,7 +138,8 @@ class BulkEmailPage(PageObject): test_subject = "Hello" test_body = "This is a test email" - self._select_recipient(recipient) + for recipient in recipients: + self._select_recipient(recipient) self.q(css=self._bounded_selector("input[name='subject']")).fill(test_subject) self.q(css=self._bounded_selector("iframe#mce_0_ifr"))[0].click() self.q(css=self._bounded_selector("iframe#mce_0_ifr"))[0].send_keys(test_body) @@ -156,7 +155,7 @@ class BulkEmailPage(PageObject): is covered by the bulk_email unit tests. """ confirmation_selector = self._bounded_selector(".msg-confirm") - expected_text = u"Your email was successfully queued for sending." + expected_text = u"Your email message was successfully queued for sending." EmptyPromise( lambda: expected_text in self.q(css=confirmation_selector)[0].text, "Message Queued Confirmation" diff --git a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py b/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py index a05eedca66..a83d43963b 100644 --- a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py +++ b/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py @@ -58,7 +58,7 @@ class BulkEmailTest(BaseInstructorDashboardTest): instructor_dashboard_page = self.visit_instructor_dashboard() self.send_email_page = instructor_dashboard_page.select_bulk_email() - @ddt.data("Myself", "Staff and admins", "All (students, staff, and admins)") + @ddt.data(["myself"], ["staff"], ["learners"], ["myself", "staff", "learners"]) def test_email_queued_for_sending(self, recipient): self.assertTrue(self.send_email_page.is_browser_on_page()) self.send_email_page.send_message(recipient) diff --git a/lms/djangoapps/bulk_email/migrations/0004_add_email_targets.py b/lms/djangoapps/bulk_email/migrations/0004_add_email_targets.py new file mode 100644 index 0000000000..e50d3733a1 --- /dev/null +++ b/lms/djangoapps/bulk_email/migrations/0004_add_email_targets.py @@ -0,0 +1,40 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_groups', '0001_initial'), + ('bulk_email', '0003_config_model_feature_flag'), + ] + + operations = [ + migrations.CreateModel( + name='Target', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('target_type', models.CharField(max_length=64, choices=[(b'myself', b'Myself'), (b'staff', b'Staff and instructors'), (b'learners', b'All students'), (b'cohort', b'Specific cohort')])), + ], + ), + migrations.AlterField( + model_name='courseemail', + name='to_option', + field=models.CharField(max_length=64, choices=[(b'deprecated', b'deprecated')]), + ), + migrations.CreateModel( + name='CohortTarget', + fields=[ + ('target_ptr', models.OneToOneField(parent_link=True, auto_created=True, primary_key=True, serialize=False, to='bulk_email.Target')), + ('cohort', models.ForeignKey(to='course_groups.CourseUserGroup')), + ], + bases=('bulk_email.target',), + ), + migrations.AddField( + model_name='courseemail', + name='targets', + field=models.ManyToManyField(to='bulk_email.Target'), + ), + ] diff --git a/lms/djangoapps/bulk_email/migrations/0005_move_target_data.py b/lms/djangoapps/bulk_email/migrations/0005_move_target_data.py new file mode 100644 index 0000000000..44c364fad6 --- /dev/null +++ b/lms/djangoapps/bulk_email/migrations/0005_move_target_data.py @@ -0,0 +1,51 @@ +# -*- coding: utf- +from __future__ import unicode_literals + +from django.db import migrations, models +from django.db.utils import DatabaseError +from bulk_email.models import EMAIL_TARGETS, SEND_TO_MYSELF + +def to_option_to_targets(apps, schema_editor): + CourseEmail = apps.get_model("bulk_email", "CourseEmail") + Target = apps.get_model("bulk_email", "Target") + db_alias = schema_editor.connection.alias + try: + for email in CourseEmail.objects.using(db_alias).all().iterator(): + new_target, created = Target.objects.using(db_alias).get_or_create( + target_type=email.to_option + ) + email.targets.add(new_target) + email.save() + except DatabaseError: + # Student module history table will fail this migration otherwise + pass + +def targets_to_to_option(apps, schema_editor): + CourseEmail = apps.get_model("bulk_email", "CourseEmail") + db_alias = schema_editor.connection.alias + try: + for email in CourseEmail.objects.using(db_alias).all().iterator(): + # Note this is not a perfect 1:1 backwards migration - targets can hold more information than to_option can. + # We use the first valid value from targets, or 'myself' if none can be found + email.to_option = next( + ( + t_type for t_type in ( + target.target_type for target in email.targets.all() + ) if t_type in EMAIL_TARGETS + ), + SEND_TO_MYSELF + ) + email.save() + except DatabaseError: + # Student module history table will fail this migration otherwise + pass + +class Migration(migrations.Migration): + + dependencies = [ + ('bulk_email', '0004_add_email_targets'), + ] + + operations = [ + migrations.RunPython(to_option_to_targets, targets_to_to_option), + ] diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index 5470de8b43..7f4c8a9ace 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/lms/djangoapps/bulk_email/models.py @@ -13,27 +13,25 @@ file and check it in at the same time as your model changes. To do that, """ import logging import markupsafe + from django.conf import settings from django.contrib.auth.models import User from django.db import models +from openedx.core.djangoapps.course_groups.models import CourseUserGroup from openedx.core.lib.html_to_text import html_to_text from openedx.core.lib.mail_utils import wrap_message from config_models.models import ConfigurationModel +from student.roles import CourseStaffRole, CourseInstructorRole from xmodule_django.models import CourseKeyField + from util.keyword_substitution import substitute_keywords_with_data +from util.query import use_read_replica_if_available log = logging.getLogger(__name__) -# Bulk email to_options - the send to options that users can -# select from when they send email. -SEND_TO_MYSELF = 'myself' -SEND_TO_STAFF = 'staff' -SEND_TO_ALL = 'all' -TO_OPTIONS = [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_ALL] - class Email(models.Model): """ @@ -52,6 +50,94 @@ class Email(models.Model): abstract = True +# Bulk email targets - the send to options that users can select from when they send email. +SEND_TO_MYSELF = 'myself' +SEND_TO_STAFF = 'staff' +SEND_TO_LEARNERS = 'learners' +SEND_TO_COHORT = 'cohort' +EMAIL_TARGETS = [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS, SEND_TO_COHORT] +EMAIL_TARGET_DESCRIPTIONS = ['Myself', 'Staff and instructors', 'All students', 'Specific cohort'] +EMAIL_TARGET_CHOICES = zip(EMAIL_TARGETS, EMAIL_TARGET_DESCRIPTIONS) + + +class Target(models.Model): + """ + A way to refer to a particular group (within a course) as a "Send to:" target. + """ + target_type = models.CharField(max_length=64, choices=EMAIL_TARGET_CHOICES) + + class Meta(object): + app_label = "bulk_email" + + def __unicode__(self): + return "CourseEmail Target for: {}".format(self.target_type) + + def get_users(self, course_id, user_id=None): + """ + Gets the users for a given target. + + Result is returned in the form of a queryset, and may contain duplicates. + """ + staff_qset = CourseStaffRole(course_id).users_with_role() + instructor_qset = CourseInstructorRole(course_id).users_with_role() + staff_instructor_qset = (staff_qset | instructor_qset) + enrollment_qset = User.objects.filter( + is_active=True, + courseenrollment__course_id=course_id, + courseenrollment__is_active=True + ) + if self.target_type == SEND_TO_MYSELF: + if user_id is None: + raise ValueError("Must define self user to send email to self.") + user = User.objects.filter(id=user_id) + return use_read_replica_if_available(user) + elif self.target_type == SEND_TO_STAFF: + return use_read_replica_if_available(staff_instructor_qset) + elif self.target_type == SEND_TO_LEARNERS: + return use_read_replica_if_available(enrollment_qset.exclude(id__in=staff_instructor_qset)) + elif self.target_type == SEND_TO_COHORT: + return User.objects.none() # TODO: cohorts aren't hooked up, put that logic here + else: + raise ValueError("Unrecognized target type {}".format(self.target_type)) + + +class CohortTarget(Target): + """ + Subclass of Target, specifically referring to a cohort. + """ + cohort = models.ForeignKey('course_groups.CourseUserGroup') + + class Meta: + app_label = "bulk_email" + + def __init__(self, *args, **kwargs): + kwargs['target_type'] = SEND_TO_COHORT + super(CohortTarget, self).__init__(*args, **kwargs) + + def __unicode__(self): + return "CourseEmail CohortTarget: {}".format(self.cohort) + + @classmethod + def ensure_valid_cohort(cls, cohort_name, course_id): + """ + Ensures cohort_name is a valid cohort for course_id. + + Returns the cohort if valid, raises an error otherwise. + """ + if cohort_name is None: + raise ValueError("Cannot create a CohortTarget without specifying a cohort_name.") + try: + cohort = CourseUserGroup.get(name=cohort_name, course_id=course_id) + except CourseUserGroup.DoesNotExist: + raise ValueError( + "Cohort {cohort} does not exist in course {course_id}".format( + cohort=cohort_name, + course_id=course_id + ) + ) + return cohort + + class CourseEmail(Email): """ Stores information for an email to a course. @@ -59,22 +145,10 @@ class CourseEmail(Email): class Meta(object): app_label = "bulk_email" - # Three options for sending that we provide from the instructor dashboard: - # * Myself: This sends an email to the staff member that is composing the email. - # - # * Staff and instructors: This sends an email to anyone in the staff group and - # anyone in the instructor group - # - # * All: This sends an email to anyone enrolled in the course, with any role - # (student, staff, or instructor) - # - TO_OPTION_CHOICES = ( - (SEND_TO_MYSELF, 'Myself'), - (SEND_TO_STAFF, 'Staff and instructors'), - (SEND_TO_ALL, 'All') - ) course_id = CourseKeyField(max_length=255, db_index=True) - to_option = models.CharField(max_length=64, choices=TO_OPTION_CHOICES, default=SEND_TO_MYSELF) + # to_option is deprecated and unused, but dropping db columns is hard so it's still here for legacy reasons + to_option = models.CharField(max_length=64, choices=[("deprecated", "deprecated")]) + targets = models.ManyToManyField(Target) template_name = models.CharField(null=True, max_length=255) from_addr = models.CharField(null=True, max_length=255) @@ -83,8 +157,8 @@ class CourseEmail(Email): @classmethod def create( - cls, course_id, sender, to_option, subject, html_message, - text_message=None, template_name=None, from_addr=None): + cls, course_id, sender, targets, subject, html_message, + text_message=None, template_name=None, from_addr=None, cohort_name=None): """ Create an instance of CourseEmail. """ @@ -92,23 +166,32 @@ class CourseEmail(Email): if text_message is None: text_message = html_to_text(html_message) - # perform some validation here: - if to_option not in TO_OPTIONS: - fmt = 'Course email being sent to unrecognized to_option: "{to_option}" for "{course}", subject "{subject}"' - msg = fmt.format(to_option=to_option, course=course_id, subject=subject) - raise ValueError(msg) + new_targets = [] + for target in targets: + # Ensure our desired target exists + if target not in EMAIL_TARGETS: + fmt = 'Course email being sent to unrecognized target: "{target}" for "{course}", subject "{subject}"' + msg = fmt.format(target=target, course=course_id, subject=subject) + raise ValueError(msg) + elif target == SEND_TO_COHORT: + cohort = CohortTarget.ensure_valid_cohort(cohort_name, course_id) + new_target, _ = CohortTarget.objects.get_or_create(target_type=target, cohort=cohort) + else: + new_target, _ = Target.objects.get_or_create(target_type=target) + new_targets.append(new_target) # create the task, then save it immediately: course_email = cls( course_id=course_id, sender=sender, - to_option=to_option, subject=subject, html_message=html_message, text_message=text_message, template_name=template_name, from_addr=from_addr, ) + course_email.save() # Must exist in db before setting M2M relationship values + course_email.targets.add(*new_targets) course_email.save() return course_email @@ -295,7 +378,7 @@ class BulkEmailFlag(ConfigurationModel): def __unicode__(self): current_model = BulkEmailFlag.current() - return u"".format( + return u"BulkEmailFlag: enabled {}, require_course_email_auth: {}".format( current_model.is_enabled(), current_model.require_course_email_auth ) diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index d090482455..abad08abaa 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -37,9 +37,7 @@ from django.core.mail.message import forbid_multi_line_headers from django.core.urlresolvers import reverse from bulk_email.models import ( - CourseEmail, Optout, - SEND_TO_MYSELF, SEND_TO_ALL, TO_OPTIONS, - SEND_TO_STAFF, + CourseEmail, Optout, Target ) from courseware.courses import get_course from openedx.core.lib.courses import course_image_url @@ -99,53 +97,6 @@ BULK_EMAIL_FAILURE_ERRORS = ( ) -def _get_recipient_querysets(user_id, to_option, course_id): - """ - Returns a list of query sets of email recipients corresponding to the - requested `to_option` category. - - `to_option` is either SEND_TO_MYSELF, SEND_TO_STAFF, or SEND_TO_ALL. - - Recipients who are in more than one category (e.g. enrolled in the course - and are staff or self) will be properly deduped. - """ - if to_option not in TO_OPTIONS: - log.error("Unexpected bulk email TO_OPTION found: %s", to_option) - raise Exception("Unexpected bulk email TO_OPTION found: {0}".format(to_option)) - - if to_option == SEND_TO_MYSELF: - user = User.objects.filter(id=user_id) - return [use_read_replica_if_available(user)] - else: - staff_qset = CourseStaffRole(course_id).users_with_role() - instructor_qset = CourseInstructorRole(course_id).users_with_role() - staff_instructor_qset = (staff_qset | instructor_qset).distinct() - if to_option == SEND_TO_STAFF: - return [use_read_replica_if_available(staff_instructor_qset)] - - if to_option == SEND_TO_ALL: - # We also require students to have activated their accounts to - # provide verification that the provided email address is valid. - enrollment_qset = User.objects.filter( - is_active=True, - courseenrollment__course_id=course_id, - courseenrollment__is_active=True - ) - - # to avoid duplicates, we only want to email unenrolled course staff - # members here - unenrolled_staff_qset = staff_instructor_qset.exclude( - courseenrollment__course_id=course_id, courseenrollment__is_active=True - ) - - # use read_replica if available - recipient_qsets = [ - use_read_replica_if_available(unenrolled_staff_qset), - use_read_replica_if_available(enrollment_qset), - ] - return recipient_qsets - - def _get_course_email_context(course): """ Returns context arguments to apply to all emails, independent of recipient. @@ -220,16 +171,23 @@ def perform_delegate_email_batches(entry_id, course_id, task_input, action_name) course = get_course(course_id) # Get arguments that will be passed to every subtask. - to_option = email_obj.to_option + targets = email_obj.targets.all() global_email_context = _get_course_email_context(course) - recipient_qsets = _get_recipient_querysets(user_id, to_option, course_id) + recipient_qsets = [ + target.get_users(course_id, user_id) + for target in targets + ] + combined_set = User.objects.none() + for qset in recipient_qsets: + combined_set |= qset + combined_set = combined_set.distinct() recipient_fields = ['profile__name', 'email'] - log.info(u"Task %s: Preparing to queue subtasks for sending emails for course %s, email %s, to_option %s", - task_id, course_id, email_id, to_option) + log.info(u"Task %s: Preparing to queue subtasks for sending emails for course %s, email %s", + task_id, course_id, email_id) - total_recipients = sum([recipient_queryset.count() for recipient_queryset in recipient_qsets]) + total_recipients = combined_set.count() routing_key = settings.BULK_EMAIL_ROUTING_KEY # if there are few enough emails, send them through a different queue @@ -257,7 +215,7 @@ def perform_delegate_email_batches(entry_id, course_id, task_input, action_name) entry, action_name, _create_send_email_subtask, - recipient_qsets, + [combined_set], recipient_fields, settings.BULK_EMAIL_EMAILS_PER_TASK, total_recipients, diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py index 4618fb7268..d70db462f9 100644 --- a/lms/djangoapps/bulk_email/tests/test_course_optout.py +++ b/lms/djangoapps/bulk_email/tests/test_course_optout.py @@ -75,15 +75,16 @@ class TestOptoutCourseEmails(ModuleStoreTestCase): test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for all', 'message': 'test message for all' } response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) - # Assert that self.student.email not in mail.to, outbox should be empty - self.assertEqual(len(mail.outbox), 0) + # Assert that self.student.email not in mail.to, outbox should only contain "myself" target + self.assertEqual(len(mail.outbox), 1) + self.assertEqual(mail.outbox[0].to[0], self.instructor.email) def test_optin_course(self): """ @@ -102,14 +103,15 @@ class TestOptoutCourseEmails(ModuleStoreTestCase): test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for all', 'message': 'test message for all' } response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) - # Assert that self.student.email in mail.to - self.assertEqual(len(mail.outbox), 1) - self.assertEqual(len(mail.outbox[0].to), 1) - self.assertEquals(mail.outbox[0].to[0], self.student.email) + # Assert that self.student.email in mail.to, along with "myself" target + self.assertEqual(len(mail.outbox), 2) + sent_addresses = [message.to[0] for message in mail.outbox] + self.assertIn(self.student.email, sent_addresses) + self.assertIn(self.instructor.email, sent_addresses) diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index 079c47c727..9eb140e7d1 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -140,7 +140,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) BulkEmailFlag.objects.create(enabled=True, require_course_email_auth=True) test_email = { 'action': 'Send email', - 'send_to': 'myself', + 'send_to': '["myself"]', 'subject': 'test subject for myself', 'message': 'test message for myself' } @@ -157,7 +157,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) # (in the setUp method), we can test sending an email. test_email = { 'action': 'send', - 'send_to': 'myself', + 'send_to': '["myself"]', 'subject': 'test subject for myself', 'message': 'test message for myself' } @@ -186,7 +186,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) # (in the setUp method), we can test sending an email. test_email = { 'action': 'Send email', - 'send_to': 'staff', + 'send_to': '["staff"]', 'subject': 'test subject for staff', 'message': 'test message for subject' } @@ -210,7 +210,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for all', 'message': 'test message for all' } @@ -271,7 +271,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) uni_subject = u'téśt śúbjéćt főŕ áĺĺ' test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': uni_subject, 'message': 'test message for all' } @@ -300,7 +300,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for all', 'message': 'test message for all' } @@ -324,7 +324,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) """ test_email = { 'action': 'Send email', - 'send_to': 'myself', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for self', 'message': 'test message for self' } @@ -397,7 +397,7 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for all', 'message': 'test message for all' } @@ -435,7 +435,7 @@ class TestEmailSendFromDashboard(EmailSendFromDashboardTestCase): uni_message = u'ẗëṡẗ ṁëṡṡäġë ḟöṛ äḷḷ ｲ乇丂ｲ ﾶ乇丂丂ﾑg乇 ｷo尺 ﾑﾚﾚ тэѕт мэѕѕаБэ fоѓ аll' test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for all', 'message': uni_message } diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index 189dfdf050..597c0aa839 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -14,7 +14,7 @@ from mock import patch, Mock from nose.plugins.attrib import attr from smtplib import SMTPDataError, SMTPServerDisconnected, SMTPConnectError -from bulk_email.models import CourseEmail, SEND_TO_ALL, BulkEmailFlag +from bulk_email.models import CourseEmail, SEND_TO_MYSELF, BulkEmailFlag from bulk_email.tasks import perform_delegate_email_batches, send_course_email from instructor_task.models import InstructorTask from instructor_task.subtasks import ( @@ -60,10 +60,15 @@ class TestEmailErrors(ModuleStoreTestCase): 'course_id': self.course.id.to_deprecated_string(), 'success': True, } + + @classmethod + def setUpClass(cls): + super(TestEmailErrors, cls).setUpClass() BulkEmailFlag.objects.create(enabled=True, require_course_email_auth=False) - def tearDown(self): - super(TestEmailErrors, self).tearDown() + @classmethod + def tearDownClass(cls): + super(TestEmailErrors, cls).tearDownClass() BulkEmailFlag.objects.all().delete() @patch('bulk_email.tasks.get_connection', autospec=True) @@ -75,7 +80,7 @@ class TestEmailErrors(ModuleStoreTestCase): get_conn.return_value.send_messages.side_effect = SMTPDataError(455, "Throttling: Sending rate exceeded") test_email = { 'action': 'Send email', - 'send_to': 'myself', + 'send_to': '["myself"]', 'subject': 'test subject for myself', 'message': 'test message for myself' } @@ -98,13 +103,14 @@ class TestEmailErrors(ModuleStoreTestCase): # have every fourth email fail due to blacklisting: get_conn.return_value.send_messages.side_effect = cycle([SMTPDataError(554, "Email address is blacklisted"), None, None, None]) - students = [UserFactory() for _ in xrange(settings.BULK_EMAIL_EMAILS_PER_TASK)] + # Don't forget to account for the "myself" instructor user + students = [UserFactory() for _ in xrange(settings.BULK_EMAIL_EMAILS_PER_TASK - 1)] for student in students: CourseEnrollmentFactory.create(user=student, course_id=self.course.id) test_email = { 'action': 'Send email', - 'send_to': 'all', + 'send_to': '["myself", "staff", "learners"]', 'subject': 'test subject for all', 'message': 'test message for all' } @@ -129,7 +135,7 @@ class TestEmailErrors(ModuleStoreTestCase): get_conn.return_value.open.side_effect = SMTPServerDisconnected(425, "Disconnecting") test_email = { 'action': 'Send email', - 'send_to': 'myself', + 'send_to': '["myself"]', 'subject': 'test subject for myself', 'message': 'test message for myself' } @@ -151,7 +157,7 @@ class TestEmailErrors(ModuleStoreTestCase): test_email = { 'action': 'Send email', - 'send_to': 'myself', + 'send_to': '["myself"]', 'subject': 'test subject for myself', 'message': 'test message for myself' } @@ -198,19 +204,26 @@ class TestEmailErrors(ModuleStoreTestCase): """ Tests exception when the to_option in the email doesn't exist """ - email = CourseEmail(course_id=self.course.id, to_option="IDONTEXIST") - email.save() - entry = InstructorTask.create(self.course.id, "task_type", "task_key", "task_input", self.instructor) - task_input = {"email_id": email.id} - with self.assertRaisesRegexp(Exception, 'Unexpected bulk email TO_OPTION found: IDONTEXIST'): - perform_delegate_email_batches(entry.id, self.course.id, task_input, "action_name") + with self.assertRaisesRegexp(Exception, 'Course email being sent to unrecognized target: "IDONTEXIST" *'): + email = CourseEmail.create( # pylint: disable=unused-variable + self.course.id, + self.instructor, + ["IDONTEXIST"], + "re: subject", + "dummy body goes here" + ) def test_wrong_course_id_in_task(self): """ Tests exception when the course_id in task is not the same as one explicitly passed in. """ - email = CourseEmail(course_id=self.course.id, to_option=SEND_TO_ALL) - email.save() + email = CourseEmail.create( + self.course.id, + self.instructor, + [SEND_TO_MYSELF], + "re: subject", + "dummy body goes here" + ) entry = InstructorTask.create("bogus/task/id", "task_type", "task_key", "task_input", self.instructor) task_input = {"email_id": email.id} with self.assertRaisesRegexp(ValueError, 'does not match task value'): @@ -220,8 +233,13 @@ class TestEmailErrors(ModuleStoreTestCase): """ Tests exception when the course_id in CourseEmail is not the same as one explicitly passed in. """ - email = CourseEmail(course_id=SlashSeparatedCourseKey("bogus", "course", "id"), to_option=SEND_TO_ALL) - email.save() + email = CourseEmail.create( + SlashSeparatedCourseKey("bogus", "course", "id"), + self.instructor, + [SEND_TO_MYSELF], + "re: subject", + "dummy body goes here" + ) entry = InstructorTask.create(self.course.id, "task_type", "task_key", "task_input", self.instructor) task_input = {"email_id": email.id} with self.assertRaisesRegexp(ValueError, 'does not match email value'): diff --git a/lms/djangoapps/bulk_email/tests/test_models.py b/lms/djangoapps/bulk_email/tests/test_models.py index 67a51db1b0..f0894a15bc 100644 --- a/lms/djangoapps/bulk_email/tests/test_models.py +++ b/lms/djangoapps/bulk_email/tests/test_models.py @@ -25,9 +25,9 @@ class CourseEmailTest(TestCase): to_option = SEND_TO_STAFF subject = "dummy subject" html_message = "dummy message" - email = CourseEmail.create(course_id, sender, to_option, subject, html_message) + email = CourseEmail.create(course_id, sender, [to_option], subject, html_message) self.assertEquals(email.course_id, course_id) - self.assertEquals(email.to_option, SEND_TO_STAFF) + self.assertIn(SEND_TO_STAFF, [target.target_type for target in email.targets.all()]) self.assertEquals(email.subject, subject) self.assertEquals(email.html_message, html_message) self.assertEquals(email.sender, sender) @@ -41,10 +41,10 @@ class CourseEmailTest(TestCase): template_name = "branded_template" from_addr = "branded@branding.com" email = CourseEmail.create( - course_id, sender, to_option, subject, html_message, template_name=template_name, from_addr=from_addr + course_id, sender, [to_option], subject, html_message, template_name=template_name, from_addr=from_addr ) self.assertEquals(email.course_id, course_id) - self.assertEquals(email.to_option, SEND_TO_STAFF) + self.assertEquals(email.targets.all()[0].target_type, SEND_TO_STAFF) self.assertEquals(email.subject, subject) self.assertEquals(email.html_message, html_message) self.assertEquals(email.sender, sender) diff --git a/lms/djangoapps/bulk_email/tests/test_tasks.py b/lms/djangoapps/bulk_email/tests/test_tasks.py index d0f1bc1a1b..0f457effc3 100644 --- a/lms/djangoapps/bulk_email/tests/test_tasks.py +++ b/lms/djangoapps/bulk_email/tests/test_tasks.py @@ -32,7 +32,7 @@ from django.core.management import call_command from xmodule.modulestore.tests.factories import CourseFactory -from bulk_email.models import CourseEmail, Optout, SEND_TO_ALL +from bulk_email.models import CourseEmail, Optout, SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS from instructor_task.tasks import send_bulk_course_email from instructor_task.subtasks import update_subtask_status, SubtaskStatus @@ -94,10 +94,10 @@ class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): Overrides the base class version in that this creates CourseEmail. """ - to_option = SEND_TO_ALL + targets = [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS] course_id = course_id or self.course.id course_email = CourseEmail.create( - course_id, self.instructor, to_option, "Test Subject", "

This is a test message

" + course_id, self.instructor, targets, "Test Subject", "

This is a test message

" ) task_input = {'email_id': course_email.id} task_id = str(uuid4()) @@ -427,8 +427,9 @@ class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): course_image = u'在淡水測試.jpg' self.course = CourseFactory.create(course_image=course_image) - num_emails = 1 - self._create_students(num_emails) + num_emails = 2 + # We also send email to the instructor: + self._create_students(num_emails - 1) with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: get_conn.return_value.send_messages.side_effect = cycle([None]) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 7a3391b008..2e7d864e2b 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -238,7 +238,7 @@ class TestInstructorAPIDenyLevels(SharedModuleStoreTestCase, LoginEnrollmentTest ('update_forum_role_membership', {'unique_student_identifier': self.user.email, 'rolename': 'Moderator', 'action': 'allow'}), ('list_forum_members', {'rolename': FORUM_ROLE_COMMUNITY_TA}), - ('send_email', {'send_to': 'staff', 'subject': 'test', 'message': 'asdf'}), + ('send_email', {'send_to': '["staff"]', 'subject': 'test', 'message': 'asdf'}), ('list_instructor_tasks', {}), ('list_background_email_tasks', {}), ('list_report_downloads', {}), @@ -3415,7 +3415,7 @@ class TestInstructorSendEmail(SharedModuleStoreTestCase, LoginEnrollmentTestCase test_subject = u'\u1234 test subject' test_message = u'\u6824 test message' cls.full_test_message = { - 'send_to': 'staff', + 'send_to': '["myself", "staff"]', 'subject': test_subject, 'message': test_message, } @@ -3464,10 +3464,19 @@ class TestInstructorSendEmail(SharedModuleStoreTestCase, LoginEnrollmentTestCase }) self.assertEqual(response.status_code, 400) + def test_send_email_invalid_sendto(self): + url = reverse('send_email', kwargs={'course_id': self.course.id.to_deprecated_string()}) + response = self.client.post(url, { + 'send_to': '["invalid_target", "staff"]', + 'subject': 'test subject', + 'message': 'test message', + }) + self.assertEqual(response.status_code, 400) + def test_send_email_no_subject(self): url = reverse('send_email', kwargs={'course_id': self.course.id.to_deprecated_string()}) response = self.client.post(url, { - 'send_to': 'staff', + 'send_to': '["staff"]', 'message': 'test message', }) self.assertEqual(response.status_code, 400) @@ -3475,7 +3484,7 @@ class TestInstructorSendEmail(SharedModuleStoreTestCase, LoginEnrollmentTestCase def test_send_email_no_message(self): url = reverse('send_email', kwargs={'course_id': self.course.id.to_deprecated_string()}) response = self.client.post(url, { - 'send_to': 'staff', + 'send_to': '["staff"]', 'subject': 'test subject', }) self.assertEqual(response.status_code, 400) diff --git a/lms/djangoapps/instructor/tests/test_email.py b/lms/djangoapps/instructor/tests/test_email.py index 1197a1c0a7..68fa981056 100644 --- a/lms/djangoapps/instructor/tests/test_email.py +++ b/lms/djangoapps/instructor/tests/test_email.py @@ -56,7 +56,7 @@ class TestNewInstructorDashboardEmailViewMongoBacked(SharedModuleStoreTestCase): response = self.client.get(self.url) self.assertIn(self.email_link, response.content) - send_to_label = '' + send_to_label = '