From faa7f544d2a14a0cbb279ea56a712a93243ef9e6 Mon Sep 17 00:00:00 2001
From: Eric Fischer <efischer@edx.org>
Date: Mon, 6 Jun 2016 16:32:59 -0400
Subject: [PATCH] Bulk Email Cohorts (#12602)

Adds cohorts as valid bulk email targets.
---
 lms/djangoapps/bulk_email/models.py           |  71 ++++--
 lms/djangoapps/bulk_email/tasks.py            |   7 +
 lms/djangoapps/bulk_email/tests/test_email.py |  63 ++++--
 .../bulk_email/tests/test_err_handling.py     |  15 +-
 .../bulk_email/tests/test_models.py           |  62 ++++--
 lms/djangoapps/instructor/tests/test_email.py |   2 +-
 lms/djangoapps/instructor/tests/utils.py      |  34 +--
 lms/djangoapps/instructor/views/api.py        |   2 +-
 .../instructor/views/instructor_dashboard.py  |   6 +
 .../views/instructor_task_helpers.py          |   4 +-
 lms/djangoapps/instructor_task/api.py         |  11 +-
 .../send_email_spec.coffee                    |  13 ++
 .../instructor_dashboard/send_email.coffee    |  38 +++-
 .../sass/course/instructor/_instructor_2.scss |  59 ++++-
 .../instructor_dashboard_2/send_email.html    | 210 ++++++++++--------
 15 files changed, 404 insertions(+), 193 deletions(-)

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 = "<html>dummy message</html>"
         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 = "<html>dummy message</html>"
+        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 = '<ul role="group" aria-label="Send to:">'
+        send_to_label = '<div class="send_to_list">Send to:</div>'
         self.assertTrue(send_to_label in response.content)
         self.assertEqual(response.status_code, 200)
 
diff --git a/lms/djangoapps/instructor/tests/utils.py b/lms/djangoapps/instructor/tests/utils.py
index 405e7e6836..f3816cbde1 100644
--- a/lms/djangoapps/instructor/tests/utils.py
+++ b/lms/djangoapps/instructor/tests/utils.py
@@ -31,7 +31,7 @@ class FakeContentTask(FakeInfo):
 
     def __init__(self, email_id, num_sent, num_failed, sent_to):
         super(FakeContentTask, self).__init__()
-        self.task_input = {'email_id': email_id, 'to_option': sent_to}
+        self.task_input = {'email_id': email_id}
         self.task_input = json.dumps(self.task_input)
         self.task_output = {'succeeded': num_sent, 'failed': num_failed}
         self.task_output = json.dumps(self.task_output)
@@ -51,20 +51,6 @@ class FakeEmail(FakeInfo):
         'created',
     ]
 
-    class FakeTarget(object):
-        """ Corresponding fake target for a fake email """
-        target_type = "expected"
-
-        def get_target_type_display(self):
-            """ Mocks out a django method """
-            return self.target_type
-
-    class FakeTargetGroup(object):
-        """ Helps to mock out a django M2M relationship """
-        def all(self):
-            """ Mocks out a django method """
-            return [FakeEmail.FakeTarget()]
-
     def __init__(self, email_id):
         super(FakeEmail, self).__init__()
         self.id = unicode(email_id)  # pylint: disable=invalid-name
@@ -75,7 +61,23 @@ class FakeEmail(FakeInfo):
         hour = random.randint(0, 23)
         minute = random.randint(0, 59)
         self.created = datetime.datetime(year, month, day, hour, minute, tzinfo=utc)
-        self.targets = FakeEmail.FakeTargetGroup()
+        self.targets = FakeTargetGroup()
+
+
+class FakeTarget(object):
+    """ Corresponding fake target for a fake email """
+    target_type = "expected"
+
+    def long_display(self):
+        """ Mocks out a class method """
+        return self.target_type
+
+
+class FakeTargetGroup(object):
+    """ Mocks out the M2M relationship between FakeEmail and FakeTarget """
+    def all(self):
+        """ Mocks out a django method """
+        return [FakeTarget()]
 
 
 class FakeEmailInfo(FakeInfo):
diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py
index ca68e24190..e8e0e7c628 100644
--- a/lms/djangoapps/instructor/views/api.py
+++ b/lms/djangoapps/instructor/views/api.py
@@ -2489,7 +2489,7 @@ def list_forum_members(request, course_id):
 @require_post_params(send_to="sending to whom", subject="subject line", message="message text")
 def send_email(request, course_id):
     """
-    Send an email to self, staff, or everyone involved in a course.
+    Send an email to self, staff, cohorts, or everyone involved in a course.
     Query Parameters:
     - 'send_to' specifies what group the email should be sent to
        Options are defined by the CourseEmail model in
diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py
index bdf3b6aec9..c54d28da6b 100644
--- a/lms/djangoapps/instructor/views/instructor_dashboard.py
+++ b/lms/djangoapps/instructor/views/instructor_dashboard.py
@@ -33,6 +33,7 @@ from courseware.access import has_access
 from courseware.courses import get_course_by_id, get_studio_url
 from django_comment_client.utils import has_forum_access
 from django_comment_common.models import FORUM_ROLE_ADMINISTRATOR
+from openedx.core.djangoapps.course_groups.cohorts import get_course_cohorts, is_course_cohorted, DEFAULT_COHORT_NAME
 from student.models import CourseEnrollment
 from shoppingcart.models import Coupon, PaidCourseRegistration, CourseRegCodeItem
 from course_modes.models import CourseMode, CourseModesArchive
@@ -609,6 +610,9 @@ def _section_send_email(course, access):
         # xblock rendering.
         request_token=uuid.uuid1().get_hex()
     )
+    cohorts = []
+    if is_course_cohorted(course_key):
+        cohorts = get_course_cohorts(course)
     email_editor = fragment.content
     section_data = {
         'section_key': 'send_email',
@@ -616,6 +620,8 @@ def _section_send_email(course, access):
         'access': access,
         'send_email': reverse('send_email', kwargs={'course_id': unicode(course_key)}),
         'editor': email_editor,
+        'cohorts': cohorts,
+        'default_cohort_name': DEFAULT_COHORT_NAME,
         'list_instructor_tasks_url': reverse(
             'list_instructor_tasks', kwargs={'course_id': unicode(course_key)}
         ),
diff --git a/lms/djangoapps/instructor/views/instructor_task_helpers.py b/lms/djangoapps/instructor/views/instructor_task_helpers.py
index 60a2d5abba..c1232e8855 100644
--- a/lms/djangoapps/instructor/views/instructor_task_helpers.py
+++ b/lms/djangoapps/instructor/views/instructor_task_helpers.py
@@ -33,7 +33,7 @@ def extract_email_features(email_task):
     From the given task, extract email content information
 
     Expects that the given task has the following attributes:
-    * task_input (dict containing email_id and to_option)
+    * task_input (dict containing email_id)
     * task_output (optional, dict containing total emails sent)
     * requester, the user who executed the task
 
@@ -57,7 +57,7 @@ def extract_email_features(email_task):
     email = CourseEmail.objects.get(id=task_input_information['email_id'])
     email_feature_dict = {
         'created': get_default_time_display(email.created),
-        'sent_to': [target.get_target_type_display() for target in email.targets.all()],
+        'sent_to': [target.long_display() for target in email.targets.all()],
         'requester': str(email_task.requester),
     }
     features = ['subject', 'html_message', 'id']
diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py
index 26352f0bf4..6301b49bf0 100644
--- a/lms/djangoapps/instructor_task/api.py
+++ b/lms/djangoapps/instructor_task/api.py
@@ -6,6 +6,7 @@ already been submitted, filtered either by running state or input
 arguments.
 
 """
+from collections import Counter
 import hashlib
 
 from celery.states import READY_STATES
@@ -279,12 +280,18 @@ def submit_bulk_course_email(request, course_key, email_id):
     # We also pull out the targets argument here, so that is displayed in
     # the InstructorTask status.
     email_obj = CourseEmail.objects.get(id=email_id)
-    targets = [target.target_type for target in email_obj.targets.all()]
+    # 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 = [
+        target if count <= 1 else
+        "{} {}".format(count, target)
+        for target, count in targets.iteritems()
+    ]
 
     task_type = 'bulk_course_email'
     task_class = send_bulk_course_email
     task_input = {'email_id': email_id, 'to_option': targets}
-    task_key_stub = "{email_id}".format(email_id=email_id)
+    task_key_stub = str(email_id)
     # create the key value by using MD5 hash:
     task_key = hashlib.md5(task_key_stub).hexdigest()
     return submit_task(request, task_type, task_class, course_key, task_input, task_key)
diff --git a/lms/static/coffee/spec/instructor_dashboard/send_email_spec.coffee b/lms/static/coffee/spec/instructor_dashboard/send_email_spec.coffee
index 38719bde50..63250d12ae 100644
--- a/lms/static/coffee/spec/instructor_dashboard/send_email_spec.coffee
+++ b/lms/static/coffee/spec/instructor_dashboard/send_email_spec.coffee
@@ -77,3 +77,16 @@ describe "Bulk Email Queueing", ->
         @send_email.$btn_send.click()
         expect($('.request-response-error').text()).toEqual('Error sending email.')
         expect(console.warn).toHaveBeenCalled()
+
+    it 'selecting all learners disables cohort selections', ->
+        @send_email.$send_to.filter("[value='learners']").click
+        @send_email.$cohort_targets.each ->
+            expect(this.disabled).toBe(true)
+        @send_email.$send_to.filter("[value='learners']").click
+        @send_email.$cohort_targets.each ->
+            expect(this.disabled).toBe(false)
+
+    it 'selected targets are listed after "send to:"', ->
+        @send_email.$send_to.click
+        $('input[name="send_to"]:checked+label').each ->
+            expect($('.send_to_list'.text())).toContain(this.innerText.replace(/\s*\n.*/g,''))
diff --git a/lms/static/coffee/src/instructor_dashboard/send_email.coffee b/lms/static/coffee/src/instructor_dashboard/send_email.coffee
index 2de77698c5..bb0f2a30fb 100644
--- a/lms/static/coffee/src/instructor_dashboard/send_email.coffee
+++ b/lms/static/coffee/src/instructor_dashboard/send_email.coffee
@@ -20,6 +20,7 @@ class @SendEmail
         # gather elements
         @$emailEditor = XBlock.initializeBlock($('.xblock-studio_view'));
         @$send_to = @$container.find("input[name='send_to']")
+        @$cohort_targets = @$send_to.filter('[value^="cohort:"]')
         @$subject = @$container.find("input[name='subject']")
         @$btn_send = @$container.find("input[name='send']")
         @$task_response = @$container.find(".request-response")
@@ -60,15 +61,19 @@ class @SendEmail
                     alert message
                     return
 
-                target_map = {
-                    "myself": gettext("Yourself"),
-                    "staff": gettext("Everyone who has staff privileges in this course"),
-                    "learners": gettext("All learners who are enrolled in this course"),
-                }
+                display_target = (value) ->
+                    if value == "myself"
+                        gettext("Yourself")
+                    else if value == "staff"
+                        gettext("Everyone who has staff privileges in this course")
+                    else if value == "learners"
+                        gettext("All learners who are enrolled in this course")
+                    else
+                        gettext("All learners in the {cohort_name} cohort").replace('{cohort_name}', value.slice(value.indexOf(':')+1))
                 success_message = gettext("Your email message was successfully queued for sending. In courses with a large number of learners, email messages to learners might take up to an hour to be sent.")
                 confirm_message = gettext("You are sending an email message with the subject {subject} to the following recipients.")
                 for target in targets
-                    confirm_message += "\n-" + target_map[target]
+                    confirm_message += "\n-" + display_target(target)
                 confirm_message += "\n\n" + gettext("Is this OK?")
                 full_confirm_message = confirm_message.replace('{subject}', subject)
 
@@ -127,6 +132,27 @@ class @SendEmail
                 error: std_ajax_err =>
                     @$content_request_response_error.text gettext("There was an error obtaining email content history for this course.")
 
+        @$send_to.change =>
+            # Ensure invalid combinations are disabled
+            if $('input#target_learners:checked').length
+                # If all is selected, cohorts can't be
+                @$cohort_targets.each ->
+                    this.checked = false
+                    this.disabled = true
+                    true
+            else
+                @$cohort_targets.each ->
+                    this.disabled = false
+                    true
+
+            # Also, keep the sent_to_list div updated
+            targets = []
+            $('input[name="send_to"]:checked+label').each ->
+                # Only use the first line, even if a subheading is present
+                targets.push(this.innerText.replace(/\s*\n.*/g,''))
+            $(".send_to_list").text(gettext("Send to:") + " " + targets.join(", "))
+
+
     fail_with_error: (msg) ->
         console.warn msg
         @$task_response.empty()
diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss
index 94610c7038..196b158bf2 100644
--- a/lms/static/sass/course/instructor/_instructor_2.scss
+++ b/lms/static/sass/course/instructor/_instructor_2.scss
@@ -406,6 +406,11 @@
 // view - bulk email
 // --------------------
 .instructor-dashboard-wrapper-2 section.idash-section#send_email {
+  h2 {
+    // override forced uppercase
+    text-transform: none;
+  }
+
   // form fields
   .list-fields {
     list-style: none;
@@ -416,14 +421,60 @@
       margin-bottom: $baseline;
       padding: 0;
 
-      .label {
-        display: block;
-      }
-
       ul {
         list-style-type: none;
       }
 
+      .send_to_list {
+        line-height: 1.3em;
+      }
+
+      input[name="send_to"] {
+        float: left;
+        margin-top: .3em;
+        margin-left: .1em;
+      }
+
+      input[name="send_to"]+label {
+        display: block;
+        padding-left: 1.3em;
+        margin-bottom: 0;
+      }
+
+      input[name="send_to"]:checked+label {
+        // "bolds" the text without causing a width recalculation
+        text-shadow: 1px 0px 0px;
+      }
+
+      input[name="send_to"]:focus+label, input[name="send_to"]:hover:not([disabled])+label {
+        background-color: #EFEFEF;
+
+        * {
+          background-color: #EFEFEF;
+        }
+      }
+
+      input[name="send_to"]:disabled+label {
+        font-weight: lighter;
+        background-color: $light-gray3
+      }
+
+      .email-targets-primary {
+        display: table-cell;
+        margin: 0;
+        width: 20%;
+      }
+
+      .email-targets-secondary {
+        display: table-cell;
+        margin: 0;
+        @include columns(2);
+
+        .subheading {
+          font-size: .9em;
+        }
+      }
+
       &:last-child {
         margin-bottom: 0;
       }
diff --git a/lms/templates/instructor/instructor_dashboard_2/send_email.html b/lms/templates/instructor/instructor_dashboard_2/send_email.html
index 60590fa28e..ea880c52ab 100644
--- a/lms/templates/instructor/instructor_dashboard_2/send_email.html
+++ b/lms/templates/instructor/instructor_dashboard_2/send_email.html
@@ -5,108 +5,122 @@ from openedx.core.djangolib.markup import HTML
 %>
 
 <div class="vert-left send-email" id="section-send-email">
-  <h2> ${_("Send Email")} </h2>
-  <div class="request-response msg msg-confirm copy" id="request-response"></div>
-  <ul class="list-fields">
-    <li class="field">
-        ${_("Send to:")}
-        <ul role="group" aria-label="${_('Send to:')}">
-            <li>
-                <label>
-                    <input type="checkbox" name="send_to" value="myself">
-                        ${_("Myself")}
-                    </input>
-                </label>
-            </li>
-            <li>
-                <label>
-                    <input type="checkbox" name="send_to" value="staff">
-                        ${_("Staff and Admin")}
-                    </input>
-                </label>
-            </li>
-            <li>
-                <label>
-                    <input type="checkbox" name="send_to" value="learners">
-                        ${_("All Learners")}
-                    </input>
-                </label>
-            </li>
-        </ul>
-    </li>
-    <li class="field">
-        <label>
-            ${_("Subject: ")}
-            <br/>
-            %if subject:
-                <input type="text" id="id_subject" name="subject" maxlength="128" size="75" value="${subject}">
-            %else:
-                <input type="text" id="id_subject" name="subject" maxlength="128" size="75">
+    <h2> ${_("Send Email")} </h2>
+    <div class="request-response msg msg-confirm copy" id="request-response"></div>
+    <ul class="list-fields">
+        <li class="field">
+            <div class="send_to_list">${_("Send to:")}</div>
+        </li>
+        <li class="field">
+        <fieldset>
+            <legend class="sr">${_("Send to:")}</legend>
+            <ul role="group" class="email-targets-primary">
+                <li>
+                    <input type="checkbox" name="send_to" value="myself" id="target_myself">
+                    <label for="target_myself">${_("Myself")}</label>
+                </li>
+                <li>
+                    <input type="checkbox" name="send_to" value="staff" id="target_staff">
+                    <label for="target_staff">${_("Staff and Administrators")}</label>
+                </li>
+                <li>
+                    <input type="checkbox" name="send_to" value="learners" id="target_learners">
+                    <label for="target_learners">${_("All Learners")}</label>
+                </li>
+            </ul>
+            %if len(section_data['cohorts']) > 0:
+            <ul role="group" class="email-targets-secondary">
+                %for cohort in section_data['cohorts']:
+                <li>
+                    <input type="checkbox" name="send_to" value="cohort:${cohort.name}" id="target_cohort_${cohort.id}">
+                    <label for="target_cohort_${cohort.id}">
+                        ${_("Cohort: ") + cohort.name}
+                        %if cohort.name == section_data['default_cohort_name']:
+                        <br/>
+                        <div class="subheading">
+                            ${_("(Students without cohort assignment)")}
+                        </div>
+                        %endif
+                    </label>
+                </li>
+                %endfor
+            </ul>
             %endif
-            <span class="tip">${_("(Maximum 128 characters)")}</span>
-        </label>
-    </li>
-    <li>
-        <label>
-            ${_("Message:")}
-            <div class="email-editor">
-                ${ HTML(section_data['editor']) }
-            </div>
-            <input type="hidden" name="message" value="">
-        </label>
-    </li>
-  </ul>
- <div class="submit-email-action">
-  <p class="copy">${_("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.")}</p>
- </div>
- <div class="submit-email-warning">
-   <p class="copy"><span style="color: red;"><b>${_("CAUTION!")}</b></span>
-     ${_("When you select Send Email, your email message is added to the queue for sending, and cannot be cancelled.")}
-   </p>
-</div>
- <br />
-  <input type="button" name="send" value="${_("Send Email")}" data-endpoint="${ section_data['send_email'] }" >
-  <div class="request-response-error"></div>
+        </fieldset>
+        </li>
+        <li class="field">
+            <label>
+                ${_("Subject: ")}
+                <br/>
+                %if subject:
+                <input type="text" id="id_subject" name="subject" maxlength="128" size="75" value="${subject}">
+                %else:
+                <input type="text" id="id_subject" name="subject" maxlength="128" size="75">
+                %endif
+                <span class="tip">${_("(Maximum 128 characters)")}</span>
+            </label>
+        </li>
+        <li>
+            <label>
+                ${_("Message:")}
+                <div class="email-editor">
+                    ${ HTML(section_data['editor']) }
+                </div>
+                <input type="hidden" name="message" value="">
+            </label>
+        </li>
+    </ul>
+    <div class="submit-email-action">
+        <p class="copy">${_("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.")}</p>
+    </div>
+    <div class="submit-email-warning">
+        <p class="copy"><span style="color: red;"><b>${_("CAUTION!")}</b></span>
+        ${_("When you select Send Email, your email message is added to the queue for sending, and cannot be cancelled.")}
+        </p>
+    </div>
+    <br />
+    <input type="button" name="send" value="${_("Send Email")}" data-endpoint="${ section_data['send_email'] }" >
+    <div class="request-response-error"></div>
+
+    %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'):
+    <div class="running-tasks-container action-type-container">
+        <hr>
+        <h2> ${_("Pending Tasks")} </h2>
+        <div class="running-tasks-section">
+            <p>${_("Email actions run in the background. The status for any active tasks - including email tasks - appears in a table below.")} </p>
+            <br />
+
+            <div class="running-tasks-table" data-endpoint="${ section_data['list_instructor_tasks_url'] }"></div>
+        </div>
+        <div class="no-pending-tasks-message"></div>
+    </div>
+
 
-%if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'):
-  <div class="running-tasks-container action-type-container">
     <hr>
-    <h2> ${_("Pending Tasks")} </h2>
-    <div class="running-tasks-section">
-      <p>${_("Email actions run in the background. The status for any active tasks - including email tasks - appears in a table below.")} </p>
-      <br />
 
-      <div class="running-tasks-table" data-endpoint="${ section_data['list_instructor_tasks_url'] }"></div>
+    <div class="vert-left email-background" id="section-task-history">
+        <h2> ${_("Email Task History")} </h2>
+        <div>
+            <p>${_("To see the content of previously sent emails, click this button:")}</p>
+            <br/>
+            <input type="button" name="task-history-email-content" value="${_("Show Sent Email History")}" data-endpoint="${ section_data['email_content_history_url'] }" >
+            <div class="content-request-response-error msg msg-warning copy"></div>
+            <p>
+            <div class="content-history-email-table">
+                <p><em>${_("To read a sent email message, click its subject.")}</em></p>
+                <br/>
+                <div class="content-history-table-inner"></div>
+            </div>
+            <div class="email-messages-wrapper"></div>
+        </div>
+        <div>
+            <p>${_("To see the status for all email tasks submitted for this course, click this button:")}</p>
+            <br/>
+            <input type="button" name="task-history-email" value="${_("Show Email Task History")}" data-endpoint="${ section_data['email_background_tasks_url'] }" >
+            <div class="history-request-response-error msg msg-warning copy"></div>
+            <div class="task-history-email-table"></div>
+        </div>
     </div>
-    <div class="no-pending-tasks-message"></div>
-  </div>
-
-
-<hr>
-
-<div class="vert-left email-background" id="section-task-history">
-  <h2> ${_("Email Task History")} </h2>
-  <div>
-    <p>${_("To see the content of previously sent emails, click this button:")}</p>
-    <br/>
-    <input type="button" name="task-history-email-content" value="${_("Sent Email History")}" data-endpoint="${ section_data['email_content_history_url'] }" >
-    <div class="content-request-response-error msg msg-warning copy"></div>
-    <p>
-    <div class="content-history-email-table">
-      <p><em>${_("To read a sent email message, click its subject.")}</em></p>
-      <br/>
-      <div class="content-history-table-inner"></div>
-    </div>
-    <div class="email-messages-wrapper"></div>
-  </div>
-  <div>
-    <p>${_("To see the status for all email tasks submitted for this course, click this button:")}</p>
-    <br/>
-    <input type="button" name="task-history-email" value="${_("Show Email Task History")}" data-endpoint="${ section_data['email_background_tasks_url'] }" >
-    <div class="history-request-response-error msg msg-warning copy"></div>
-    <div class="task-history-email-table"></div>
-  </div>
-</div>
-%endif
+    %endif
 
 </div> <!-- end section send-email -->