diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index 7f4c8a9ace..6d45fed467 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/lms/djangoapps/bulk_email/models.py @@ -1,15 +1,5 @@ """ Models for bulk email - -WE'RE USING MIGRATIONS! - -If you make changes to this model, be sure to create an appropriate migration -file and check it in at the same time as your model changes. To do that, - -1. Go to the edx-platform dir -2. ./manage.py lms schemamigration bulk_email --auto description_of_your_change -3. Add the migration file created in edx-platform/lms/djangoapps/bulk_email/migrations/ - """ import logging import markupsafe @@ -19,6 +9,7 @@ from django.contrib.auth.models import User from django.db import models from openedx.core.djangoapps.course_groups.models import CourseUserGroup +from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_name from openedx.core.lib.html_to_text import html_to_text from openedx.core.lib.mail_utils import wrap_message @@ -55,14 +46,24 @@ 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) +EMAIL_TARGET_CHOICES = zip( + [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_LEARNERS, SEND_TO_COHORT], + ['Myself', 'Staff and instructors', 'All students', 'Specific cohort'] +) +EMAIL_TARGETS = {target[0] for target in EMAIL_TARGET_CHOICES} class Target(models.Model): """ A way to refer to a particular group (within a course) as a "Send to:" target. + + Django hackery in this class - polymorphism does not work well in django, for reasons relating to how + each class is represented by its own database table. Due to this, we can't just override + methods of Target in CohortTarget and get the child method, as one would expect. The + workaround is to check to see that a given target is a CohortTarget (self.target_type == + SEND_TO_COHORT), then explicitly call the method on self.cohorttarget, which is created + by django as part of this inheritance setup. These calls require pylint disable no-member in + several locations in this class. """ target_type = models.CharField(max_length=64, choices=EMAIL_TARGET_CHOICES) @@ -70,7 +71,25 @@ class Target(models.Model): app_label = "bulk_email" def __unicode__(self): - return "CourseEmail Target for: {}".format(self.target_type) + return "CourseEmail Target: {}".format(self.short_display()) + + def short_display(self): + """ + Returns a short display name + """ + if self.target_type == SEND_TO_COHORT: + return self.cohorttarget.short_display() # pylint: disable=no-member + else: + return self.target_type + + def long_display(self): + """ + Returns a long display name + """ + if self.target_type == SEND_TO_COHORT: + return self.cohorttarget.long_display() # pylint: disable=no-member + else: + return self.get_target_type_display() def get_users(self, course_id, user_id=None): """ @@ -96,7 +115,7 @@ class Target(models.Model): 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 + return self.cohorttarget.cohort.users.filter(id__in=enrollment_qset) # pylint: disable=no-member else: raise ValueError("Unrecognized target type {}".format(self.target_type)) @@ -114,8 +133,11 @@ class CohortTarget(Target): kwargs['target_type'] = SEND_TO_COHORT super(CohortTarget, self).__init__(*args, **kwargs) - def __unicode__(self): - return "CourseEmail CohortTarget: {}".format(self.cohort) + def short_display(self): + return "{}-{}".format(self.target_type, self.cohort.name) + + def long_display(self): + return "Cohort: {}".format(self.cohort.name) @classmethod def ensure_valid_cohort(cls, cohort_name, course_id): @@ -127,7 +149,7 @@ class CohortTarget(Target): 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) + cohort = get_cohort_by_name(name=cohort_name, course_key=course_id) except CourseUserGroup.DoesNotExist: raise ValueError( "Cohort {cohort} does not exist in course {course_id}".format( @@ -168,16 +190,19 @@ class CourseEmail(Email): new_targets = [] for target in targets: + # split target, to handle cohort:cohort_name + target_split = target.split(':', 1) # Ensure our desired target exists - if target not in EMAIL_TARGETS: + if target_split[0] 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) + 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) else: - new_target, _ = Target.objects.get_or_create(target_type=target) + new_target, _ = Target.objects.get_or_create(target_type=target_split[0]) new_targets.append(new_target) # create the task, then save it immediately: diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index abad08abaa..3fd044fd94 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -195,6 +195,13 @@ def perform_delegate_email_batches(entry_id, course_id, task_input, action_name) if total_recipients <= settings.BULK_EMAIL_JOB_SIZE_THRESHOLD: routing_key = settings.BULK_EMAIL_ROUTING_KEY_SMALL_JOBS + # Weird things happen if we allow empty querysets as input to emailing subtasks + # The task appears to hang at "0 out of 0 completed" and never finishes. + if total_recipients == 0: + msg = u"Bulk Email Task: Empty recipient set" + log.warning(msg) + raise ValueError(msg) + def _create_send_email_subtask(to_list, initial_subtask_status): """Creates a subtask to send email to a given recipient list.""" subtask_id = initial_subtask_status.task_id diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index 9eb140e7d1..062da01ad9 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -18,6 +18,8 @@ from django.test.utils import override_settings from bulk_email.models import Optout, BulkEmailFlag from bulk_email.tasks import _get_source_address +from openedx.core.djangoapps.course_groups.models import CourseCohort +from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort from courseware.tests.factories import StaffFactory, InstructorFactory from instructor_task.subtasks import update_subtask_status from student.roles import CourseStaffRole @@ -113,6 +115,7 @@ class EmailSendFromDashboardTestCase(SharedModuleStoreTestCase): self.login_as_user(self.instructor) + # Pulling up the instructor dash email view here allows us to test sending emails in tests self.goto_instructor_dash_email_view() self.send_mail_url = reverse( 'send_email', kwargs={'course_id': unicode(self.course.id)} @@ -153,15 +156,12 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) """ Make sure email send to myself goes to myself. """ - # Now we know we have pulled up the instructor dash's email view - # (in the setUp method), we can test sending an email. test_email = { 'action': 'send', 'send_to': '["myself"]', 'subject': 'test subject for myself', 'message': 'test message for myself' } - # Post the email to the instructor dashboard API response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) @@ -182,15 +182,12 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) """ Make sure email send to staff and instructors goes there. """ - # Now we know we have pulled up the instructor dash's email view - # (in the setUp method), we can test sending an email. test_email = { 'action': 'Send email', 'send_to': '["staff"]', 'subject': 'test subject for staff', 'message': 'test message for subject' } - # Post the email to the instructor dashboard API response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) @@ -201,12 +198,51 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) [self.instructor.email] + [s.email for s in self.staff] ) + def test_send_to_cohort(self): + """ + Make sure email sent to a cohort goes there. + """ + cohort = CourseCohort.create(cohort_name='test cohort', course_id=self.course.id) + for student in self.students: + add_user_to_cohort(cohort.course_user_group, student.username) + test_email = { + 'action': 'Send email', + 'send_to': '["cohort:{}"]'.format(cohort.course_user_group.name), + 'subject': 'test subject for cohort', + 'message': 'test message for cohort' + } + response = self.client.post(self.send_mail_url, test_email) + self.assertEquals(json.loads(response.content), self.success_content) + + self.assertItemsEqual( + [e.to[0] for e in mail.outbox], + [s.email for s in self.students] + ) + + def test_send_to_cohort_unenrolled(self): + """ + Make sure email sent to a cohort does not go to unenrolled members of the cohort. + """ + self.students.append(UserFactory()) # user will be added to cohort, but not enrolled in course + cohort = CourseCohort.create(cohort_name='test cohort', course_id=self.course.id) + for student in self.students: + add_user_to_cohort(cohort.course_user_group, student.username) + test_email = { + 'action': 'Send email', + 'send_to': '["cohort:{}"]'.format(cohort.course_user_group.name), + 'subject': 'test subject for cohort', + 'message': 'test message for cohort' + } + response = self.client.post(self.send_mail_url, test_email) + self.assertEquals(json.loads(response.content), self.success_content) + + self.assertEquals(len(mail.outbox), len(self.students) - 1) + self.assertNotIn(self.students[-1].email, [e.to[0] for e in mail.outbox]) + def test_send_to_all(self): """ Make sure email send to all goes there. """ - # Now we know we have pulled up the instructor dash's email view - # (in the setUp method), we can test sending an email. test_email = { 'action': 'Send email', @@ -214,7 +250,6 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) 'subject': 'test subject for all', 'message': 'test message for all' } - # Post the email to the instructor dashboard API response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) @@ -265,8 +300,6 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) """ Make sure email (with Unicode characters) send to all goes there. """ - # Now we know we have pulled up the instructor dash's email view - # (in the setUp method), we can test sending an email. uni_subject = u'téśt śúbjéćt főŕ áĺĺ' test_email = { @@ -275,7 +308,6 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) 'subject': uni_subject, 'message': 'test message for all' } - # Post the email to the instructor dashboard API response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) @@ -290,8 +322,6 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) """ Make sure email (with Unicode characters) send to all goes there. """ - # Now we know we have pulled up the instructor dash's email view - # (in the setUp method), we can test sending an email. # Create a student with Unicode in their first & last names unicode_user = UserFactory(first_name=u'Ⓡⓞⓑⓞⓣ', last_name=u'ՇﻉรՇ') @@ -304,7 +334,6 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) 'subject': 'test subject for all', 'message': 'test message for all' } - # Post the email to the instructor dashboard API response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) @@ -401,7 +430,6 @@ class TestEmailSendFromDashboardMockedHtmlToText(EmailSendFromDashboardTestCase) 'subject': 'test subject for all', 'message': 'test message for all' } - # Post the email to the instructor dashboard API response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) @@ -429,8 +457,6 @@ class TestEmailSendFromDashboard(EmailSendFromDashboardTestCase): """ Make sure email (with Unicode characters) send to all goes there. """ - # Now we know we have pulled up the instructor dash's email view - # (in the setUp method), we can test sending an email. uni_message = u'ẗëṡẗ ṁëṡṡäġë ḟöṛ äḷḷ イ乇丂イ ᄊ乇丂丂ムg乇 キo尺 ムレレ тэѕт мэѕѕаБэ fоѓ аll' test_email = { @@ -439,7 +465,6 @@ class TestEmailSendFromDashboard(EmailSendFromDashboardTestCase): 'subject': 'test subject for all', 'message': uni_message } - # Post the email to the instructor dashboard API response = self.client.post(self.send_mail_url, test_email) self.assertEquals(json.loads(response.content), self.success_content) diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index 597c0aa839..9c4363b4df 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -204,7 +204,7 @@ class TestEmailErrors(ModuleStoreTestCase): """ Tests exception when the to_option in the email doesn't exist """ - with self.assertRaisesRegexp(Exception, 'Course email being sent to unrecognized target: "IDONTEXIST" *'): + with self.assertRaisesRegexp(ValueError, 'Course email being sent to unrecognized target: "IDONTEXIST" *'): email = CourseEmail.create( # pylint: disable=unused-variable self.course.id, self.instructor, @@ -213,6 +213,19 @@ class TestEmailErrors(ModuleStoreTestCase): "dummy body goes here" ) + def test_nonexistent_cohort(self): + """ + Tests exception when the cohort doesn't exist + """ + with self.assertRaisesRegexp(ValueError, 'Cohort IDONTEXIST does not exist *'): + email = CourseEmail.create( # pylint: disable=unused-variable + self.course.id, + self.instructor, + ["cohort: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. diff --git a/lms/djangoapps/bulk_email/tests/test_models.py b/lms/djangoapps/bulk_email/tests/test_models.py index f0894a15bc..fd37b9a59c 100644 --- a/lms/djangoapps/bulk_email/tests/test_models.py +++ b/lms/djangoapps/bulk_email/tests/test_models.py @@ -10,8 +10,16 @@ from student.tests.factories import UserFactory from mock import patch, Mock from nose.plugins.attrib import attr -from bulk_email.models import CourseEmail, SEND_TO_STAFF, CourseEmailTemplate, CourseAuthorization, BulkEmailFlag -from opaque_keys.edx.locations import SlashSeparatedCourseKey +from bulk_email.models import ( + CourseEmail, + SEND_TO_COHORT, + SEND_TO_STAFF, + CourseEmailTemplate, + CourseAuthorization, + BulkEmailFlag +) +from openedx.core.djangoapps.course_groups.models import CourseCohort +from opaque_keys.edx.keys import CourseKey @attr('shard_1') @@ -20,20 +28,20 @@ class CourseEmailTest(TestCase): """Test the CourseEmail model.""" def test_creation(self): - course_id = SlashSeparatedCourseKey('abc', '123', 'doremi') + course_id = CourseKey.from_string('abc/123/doremi') sender = UserFactory.create() to_option = SEND_TO_STAFF subject = "dummy subject" html_message = "dummy message" email = CourseEmail.create(course_id, sender, [to_option], subject, html_message) - self.assertEquals(email.course_id, course_id) + self.assertEqual(email.course_id, course_id) 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) + self.assertEqual(email.subject, subject) + self.assertEqual(email.html_message, html_message) + self.assertEqual(email.sender, sender) def test_creation_with_optional_attributes(self): - course_id = SlashSeparatedCourseKey('abc', '123', 'doremi') + course_id = CourseKey.from_string('abc/123/doremi') sender = UserFactory.create() to_option = SEND_TO_STAFF subject = "dummy subject" @@ -43,16 +51,16 @@ class CourseEmailTest(TestCase): email = CourseEmail.create( 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.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) - self.assertEquals(email.template_name, template_name) - self.assertEquals(email.from_addr, from_addr) + self.assertEqual(email.course_id, course_id) + self.assertEqual(email.targets.all()[0].target_type, SEND_TO_STAFF) + self.assertEqual(email.subject, subject) + self.assertEqual(email.html_message, html_message) + self.assertEqual(email.sender, sender) + self.assertEqual(email.template_name, template_name) + self.assertEqual(email.from_addr, from_addr) def test_bad_to_option(self): - course_id = SlashSeparatedCourseKey('abc', '123', 'doremi') + course_id = CourseKey.from_string('abc/123/doremi') sender = UserFactory.create() to_option = "fake" subject = "dummy subject" @@ -60,6 +68,20 @@ class CourseEmailTest(TestCase): with self.assertRaises(ValueError): CourseEmail.create(course_id, sender, to_option, subject, html_message) + def test_cohort_target(self): + course_id = CourseKey.from_string('abc/123/doremi') + sender = UserFactory.create() + to_option = 'cohort:test cohort' + subject = "dummy subject" + html_message = "dummy message" + CourseCohort.create(cohort_name='test cohort', course_id=course_id) + email = CourseEmail.create(course_id, sender, [to_option], subject, html_message) + self.assertEqual(len(email.targets.all()), 1) + target = email.targets.all()[0] + self.assertEqual(target.target_type, SEND_TO_COHORT) + self.assertEqual(target.short_display(), 'cohort-test cohort') + self.assertEqual(target.long_display(), 'Cohort: test cohort') + @attr('shard_1') class NoCourseEmailTemplateTest(TestCase): @@ -179,7 +201,7 @@ class CourseAuthorizationTest(TestCase): def test_creation_auth_on(self): BulkEmailFlag.objects.create(enabled=True, require_course_email_auth=True) - course_id = SlashSeparatedCourseKey('abc', '123', 'doremi') + course_id = CourseKey.from_string('abc/123/doremi') # Test that course is not authorized by default self.assertFalse(BulkEmailFlag.feature_enabled(course_id)) @@ -188,7 +210,7 @@ class CourseAuthorizationTest(TestCase): cauth.save() # Now, course should be authorized self.assertTrue(BulkEmailFlag.feature_enabled(course_id)) - self.assertEquals( + self.assertEqual( cauth.__unicode__(), "Course 'abc/123/doremi': Instructor Email Enabled" ) @@ -198,14 +220,14 @@ class CourseAuthorizationTest(TestCase): cauth.save() # Test that course is now unauthorized self.assertFalse(BulkEmailFlag.feature_enabled(course_id)) - self.assertEquals( + self.assertEqual( cauth.__unicode__(), "Course 'abc/123/doremi': Instructor Email Not Enabled" ) def test_creation_auth_off(self): BulkEmailFlag.objects.create(enabled=True, require_course_email_auth=False) - course_id = SlashSeparatedCourseKey('blahx', 'blah101', 'ehhhhhhh') + course_id = CourseKey.from_string('blahx/blah101/ehhhhhhh') # Test that course is authorized by default, since auth is turned off self.assertTrue(BulkEmailFlag.feature_enabled(course_id)) diff --git a/lms/djangoapps/instructor/tests/test_email.py b/lms/djangoapps/instructor/tests/test_email.py index 68fa981056..e48121c943 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 = '
${_("We recommend sending learners no more than one email message per week. Before you send your email, review the text carefully and send it to yourself first, so that you can preview the formatting and make sure embedded images and links work correctly.")}
-${_("CAUTION!")} - ${_("When you select Send Email, your email message is added to the queue for sending, and cannot be cancelled.")} -
-${_("We recommend sending learners no more than one email message per week. Before you send your email, review the text carefully and send it to yourself first, so that you can preview the formatting and make sure embedded images and links work correctly.")}
+${_("CAUTION!")} + ${_("When you select Send Email, your email message is added to the queue for sending, and cannot be cancelled.")} +
+${_("Email actions run in the background. The status for any active tasks - including email tasks - appears in a table below.")}
+${_("Email actions run in the background. The status for any active tasks - including email tasks - appears in a table below.")}
-${_("To see the content of previously sent emails, click this button:")}
++
${_("To read a sent email message, click its subject.")}
+${_("To see the status for all email tasks submitted for this course, click this button:")}
+${_("To see the content of previously sent emails, click this button:")}
--
${_("To read a sent email message, click its subject.")}
-${_("To see the status for all email tasks submitted for this course, click this button:")}
-