Bulk Email: Add design styling

Switch to using decorators; refactor and cleanup tests.

common/djangoapps/student/views.py

@@ -27,6 +27,7 @@ from django_future.csrf import ensure_csrf_cookie
 from django.utils.http import cookie_date
 from django.utils.http import base36_to_int
 from django.utils.translation import ugettext as _
+from django.views.decorators.http import require_POST
 
 from ratelimitbackend.exceptions import RateLimitException
 
@@ -68,8 +69,7 @@ Article = namedtuple('Article', 'title url author image deck publication publish
 
 
 def csrf_token(context):
-    ''' A csrf token that can be included in a form.
-    '''
+    """A csrf token that can be included in a form."""
     csrf_token = context.get('csrf_token', '')
     if csrf_token == 'NOTPROVIDED':
         return ''
@@ -82,12 +82,12 @@ def csrf_token(context):
 # This means that it should always return the same thing for anon
 # users. (in particular, no switching based on query params allowed)
 def index(request, extra_context={}, user=None):
-    '''
+    """
     Render the edX main page.
 
     extra_context is used to allow immediate display of certain modal windows, eg signup,
     as used by external_auth.
-    '''
+    """
 
     # The course selection work is done in courseware.courses.
     domain = settings.MITX_FEATURES.get('FORCE_UNIVERSITY_DOMAIN')  # normally False
@@ -411,7 +411,7 @@ def accounts_login(request, error=""):
 # Need different levels of logging
 @ensure_csrf_cookie
 def login_user(request, error=""):
-    ''' AJAX request to log in the user. '''
+    """AJAX request to log in the user."""
     if 'email' not in request.POST or 'password' not in request.POST:
         return HttpResponse(json.dumps({'success': False,
                                         'value': _('There was an error receiving your login information. Please email us.')}))  # TODO: User error message
@@ -494,11 +494,11 @@ def login_user(request, error=""):
 
 @ensure_csrf_cookie
 def logout_user(request):
-    '''
+    """
     HTTP request to log out the user. Redirects to marketing page.
     Deletes both the CSRF and sessionid cookies so the marketing
     site can determine the logged in state of the user
-    '''
+    """
     # We do not log here, because we have a handler registered
     # to perform logging on successful logouts.
     logout(request)
@@ -512,8 +512,7 @@ def logout_user(request):
 @login_required
 @ensure_csrf_cookie
 def change_setting(request):
-    ''' JSON call to change a profile setting: Right now, location
-    '''
+    """JSON call to change a profile setting: Right now, location"""
     # TODO (vshnayder): location is no longer used
     up = UserProfile.objects.get(user=request.user)  # request.user.profile_cache
     if 'location' in request.POST:
@@ -581,10 +580,10 @@ def _do_create_account(post_vars):
 
 @ensure_csrf_cookie
 def create_account(request, post_override=None):
-    '''
+    """
     JSON call to create new edX account.
     Used by form in signup_modal.html, which is included into navigation.html
-    '''
+    """
     js = {'success': False}
 
     post_vars = post_override if post_override else request.POST
@@ -818,10 +817,10 @@ def begin_exam_registration(request, course_id):
 
 @ensure_csrf_cookie
 def create_exam_registration(request, post_override=None):
-    '''
+    """
     JSON call to create a test center exam registration.
     Called by form in test_center_register.html
-    '''
+    """
     post_vars = post_override if post_override else request.POST
 
     # first determine if we need to create a new TestCenterUser, or if we are making any update
@@ -974,8 +973,7 @@ def auto_auth(request):
 
 @ensure_csrf_cookie
 def activate_account(request, key):
-    ''' When link in activation e-mail is clicked
-    '''
+    """When link in activation e-mail is clicked"""
     r = Registration.objects.filter(activation_key=key)
     if len(r) == 1:
         user_logged_in = request.user.is_authenticated()
@@ -1010,7 +1008,7 @@ def activate_account(request, key):
 
 @ensure_csrf_cookie
 def password_reset(request):
-    ''' Attempts to send a password reset e-mail. '''
+    """ Attempts to send a password reset e-mail. """
     if request.method != "POST":
         raise Http404
 
@@ -1032,9 +1030,9 @@ def password_reset_confirm_wrapper(
     uidb36=None,
     token=None,
 ):
-    ''' A wrapper around django.contrib.auth.views.password_reset_confirm.
+    """ A wrapper around django.contrib.auth.views.password_reset_confirm.
         Needed because we want to set the user as active at this step.
-    '''
+    """
     # cribbed from django.contrib.auth.views.password_reset_confirm
     try:
         uid_int = base36_to_int(uidb36)
@@ -1076,8 +1074,8 @@ def reactivation_email_for_user(user):
 
 @ensure_csrf_cookie
 def change_email_request(request):
-    ''' AJAX call from the profile page. User wants a new e-mail.
-    '''
+    """ AJAX call from the profile page. User wants a new e-mail.
+    """
     ## Make sure it checks for existing e-mail conflicts
     if not request.user.is_authenticated:
         raise Http404
@@ -1132,9 +1130,9 @@ def change_email_request(request):
 @ensure_csrf_cookie
 @transaction.commit_manually
 def confirm_email_change(request, key):
-    ''' User requested a new e-mail. This is called when the activation
+    """ User requested a new e-mail. This is called when the activation
     link is clicked. We confirm with the old e-mail, and update
-    '''
+    """
     try:
         try:
             pec = PendingEmailChange.objects.get(activation_key=key)
@@ -1191,7 +1189,7 @@ def confirm_email_change(request, key):
 
 @ensure_csrf_cookie
 def change_name_request(request):
-    ''' Log a request for a new name. '''
+    """ Log a request for a new name. """
     if not request.user.is_authenticated:
         raise Http404
 
@@ -1215,7 +1213,7 @@ def change_name_request(request):
 
 @ensure_csrf_cookie
 def pending_name_changes(request):
-    ''' Web page which allows staff to approve or reject name changes. '''
+    """ Web page which allows staff to approve or reject name changes. """
     if not request.user.is_staff:
         raise Http404
 
@@ -1231,7 +1229,7 @@ def pending_name_changes(request):
 
 @ensure_csrf_cookie
 def reject_name_change(request):
-    ''' JSON: Name change process. Course staff clicks 'reject' on a given name change '''
+    """ JSON: Name change process. Course staff clicks 'reject' on a given name change """
     if not request.user.is_staff:
         raise Http404
 
@@ -1269,32 +1267,31 @@ def accept_name_change_by_id(id):
 
 @ensure_csrf_cookie
 def accept_name_change(request):
-    ''' JSON: Name change process. Course staff clicks 'accept' on a given name change
+    """ JSON: Name change process. Course staff clicks 'accept' on a given name change
 
     We used this during the prototype but now we simply record name changes instead
     of manually approving them. Still keeping this around in case we want to go
     back to this approval method.
-    '''
+    """
     if not request.user.is_staff:
         raise Http404
 
     return accept_name_change_by_id(int(request.POST['id']))
 
 
+@require_POST
+@login_required
 @ensure_csrf_cookie
 def change_email_settings(request):
     """Modify logged-in user's setting for receiving emails from a course."""
-    if request.method != "POST":
-        return HttpResponseNotAllowed(["POST"])
-
     user = request.user
-    if not user.is_authenticated():
-        return HttpResponseForbidden()
 
     course_id = request.POST.get("course_id")
     receive_emails = request.POST.get("receive_emails")
     if receive_emails:
-        Optout.objects.filter(email=user.email, course_id=course_id).delete()
+        optout_object = Optout.objects.filter(email=user.email, course_id=course_id)
+        if optout_object:
+            optout_object.delete()
         log.info(u"User {0} ({1}) opted to receive emails from course {2}".format(user.username, user.email, course_id))
         track.views.server_track(request, "change-email-settings", {"receive_emails": "yes", "course": course_id}, page='dashboard')
     else:

lms/djangoapps/bulk_email/migrations/0001_initial.py

@@ -33,7 +33,6 @@ class Migration(SchemaMigration):
         # Adding unique constraint on 'Optout', fields ['email', 'course_id']
         db.create_unique('bulk_email_optout', ['email', 'course_id'])
 
-
     def backwards(self, orm):
         # Removing unique constraint on 'Optout', fields ['email', 'course_id']
         db.delete_unique('bulk_email_optout', ['email', 'course_id'])
@@ -44,7 +43,6 @@ class Migration(SchemaMigration):
         # Deleting model 'Optout'
         db.delete_table('bulk_email_optout')
 
-
     models = {
         'auth.group': {
             'Meta': {'object_name': 'Group'},
@@ -102,4 +100,4 @@ class Migration(SchemaMigration):
         }
     }
 
-    complete_apps = ['bulk_email']
+    complete_apps = ['bulk_email']

lms/djangoapps/bulk_email/models.py

@@ -10,6 +10,7 @@ class Email(models.Model):
     Abstract base class for common information for an email.
     """
     sender = models.ForeignKey(User, default=1, blank=True, null=True)
+    # The unique hash for this email. Used to quickly look up an email (see `tasks.py`)
     hash = models.CharField(max_length=128, db_index=True)
     subject = models.CharField(max_length=128, blank=True)
     html_message = models.TextField(null=True, blank=True)
@@ -24,10 +25,20 @@ class CourseEmail(Email, models.Model):
     """
     Stores information for an email to a course.
     """
-    TO_OPTIONS = (('myself', 'Myself'),
-                  ('staff', 'Staff and instructors'),
-                  ('all', 'All')
-                  )
+    # 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_OPTIONS = (
+        ('myself', 'Myself'),
+        ('staff', 'Staff and instructors'),
+        ('all', 'All')
+    )
     course_id = models.CharField(max_length=255, db_index=True)
     to = models.CharField(max_length=64, choices=TO_OPTIONS, default='myself')
 

lms/djangoapps/bulk_email/tasks.py

@@ -31,7 +31,7 @@ def delegate_email_batches(hash_for_msg, to_option, course_id, course_url, user_
     get the mail, chopping up into batches of settings.EMAILS_PER_TASK size,
     and queueing up worker jobs.
 
-    `to_option` is {'students', 'staff', or 'all'}
+    `to_option` is {'myself', 'staff', or 'all'}
 
     Returns the number of batches (workers) kicked off.
     """
@@ -49,7 +49,8 @@ def delegate_email_batches(hash_for_msg, to_option, course_id, course_url, user_
 
     if to_option == "myself":
         recipient_qset = User.objects.filter(id=user_id).values('profile__name', 'email')
-    else:
+
+    elif to_option == "all" or to_option == "staff":
         staff_grpname = _course_staff_group_name(course.location)
         staff_group, _ = Group.objects.get_or_create(name=staff_grpname)
         staff_qset = staff_group.user_set.values('profile__name', 'email')
@@ -66,8 +67,12 @@ def delegate_email_batches(hash_for_msg, to_option, course_id, course_url, user_
             recipient_qset = recipient_qset | enrollment_qset
         recipient_qset = recipient_qset.distinct()
 
+    else:
+        log.error("Unexpected bulk email TO_OPTION found: %s", to_option)
+        raise Exception("Unexpected bulk email TO_OPTION found: {0}".format(to_option))
+
     recipient_list = list(recipient_qset)
-    total_num_emails = recipient_qset.count()
+    total_num_emails = len(recipient_list)
     num_workers = int(math.ceil(float(total_num_emails) / float(settings.EMAILS_PER_TASK)))
     chunk = int(math.ceil(float(total_num_emails) / float(num_workers)))
 
@@ -97,7 +102,7 @@ def course_email(hash_for_msg, to_list, course_title, course_url, throttle=False
     (plaintext, err_from_stderr) = process.communicate(input=msg.html_message.encode('utf-8'))  # use lynx to get plaintext
 
     course_title_no_quotes = re.sub(r'"', '', course_title)
-    from_addr = '"%s" Course Staff <%s>' % (course_title_no_quotes, settings.DEFAULT_BULK_FROM_EMAIL)
+    from_addr = '"{0}" Course Staff <{1}>'.format(course_title_no_quotes, settings.DEFAULT_BULK_FROM_EMAIL)
 
     if err_from_stderr:
         log.info(err_from_stderr)
@@ -108,20 +113,33 @@ def course_email(hash_for_msg, to_list, course_title, course_url, throttle=False
         num_sent = 0
         num_error = 0
 
+        email_context = {
+            'name': '',
+            'email': '',
+            'course_title': course_title,
+            'course_url': course_url
+        }
         while to_list:
             (name, email) = to_list[-1].values()
-            html_footer = render_to_string('emails/email_footer.html',
-                                           {'name': name,
-                                            'email': email,
-                                            'course_title': course_title,
-                                            'course_url': course_url})
-            plain_footer = render_to_string('emails/email_footer.txt',
-                                            {'name': name,
-                                             'email': email,
-                                             'course_title': course_title,
-                                             'course_url': course_url})
+            email_context['name'] = name
+            email_context['email'] = email
 
-            email_msg = EmailMultiAlternatives(subject, plaintext + plain_footer.encode('utf-8'), from_addr, [email], connection=connection)
+            html_footer = render_to_string(
+                'emails/email_footer.html',
+                email_context
+            )
+            plain_footer = render_to_string(
+                'emails/email_footer.txt',
+                email_context
+            )
+
+            email_msg = EmailMultiAlternatives(
+                subject,
+                plaintext + plain_footer.encode('utf-8'),
+                from_addr,
+                [email],
+                connection=connection
+            )
             email_msg.attach_alternative(msg.html_message + html_footer.encode('utf-8'), 'text/html')
 
             if throttle or current_task.request.retries > 0:  # throttle if we tried a few times and got the rate limiter
@@ -132,11 +150,12 @@ def course_email(hash_for_msg, to_list, course_title, course_url, throttle=False
                 log.info('Email with hash ' + hash_for_msg + ' sent to ' + email)
                 num_sent += 1
             except SMTPDataError as exc:
-                #According to SMTP spec, we'll retry error codes in the 4xx range.  5xx range indicates hard failure
+                # According to SMTP spec, we'll retry error codes in the 4xx range.  5xx range indicates hard failure
                 if exc.smtp_code >= 400 and exc.smtp_code < 500:
-                    raise exc  # this will cause the outer handler to catch the exception and retry the entire task
+                    # This will cause the outer handler to catch the exception and retry the entire task
+                    raise exc
                 else:
-                    #this will fall through and not retry the message, since it will be popped
+                    # This will fall through and not retry the message, since it will be popped
                     log.warning('Email with hash ' + hash_for_msg + ' not delivered to ' + email + ' due to error: ' + exc.smtp_error)
                     num_error += 1
 
@@ -146,8 +165,18 @@ def course_email(hash_for_msg, to_list, course_title, course_url, throttle=False
         return course_email_result(num_sent, num_error)
 
     except (SMTPDataError, SMTPConnectError, SMTPServerDisconnected) as exc:
-        #error caught here cause the email to be retried.  The entire task is actually retried without popping the list
-        raise course_email.retry(arg=[hash_for_msg, to_list, course_title, course_url, current_task.request.retries > 0], exc=exc, countdown=(2 ** current_task.request.retries) * 15)
+        # Error caught here cause the email to be retried.  The entire task is actually retried without popping the list
+        raise course_email.retry(
+            arg=[
+                hash_for_msg,
+                to_list,
+                course_title,
+                course_url,
+                current_task.request.retries > 0
+             ],
+            exc=exc,
+            countdown=(2 ** current_task.request.retries) * 15
+        )
 
 
 # This string format code is wrapped in this function to allow mocking for a unit test

lms/djangoapps/bulk_email/tests/test_course_optout.py

@@ -4,12 +4,16 @@ Unit tests for student optouts from course email
 import json
 
 from django.core import mail
-from django.test.utils import override_settings
 from django.core.urlresolvers import reverse
+from django.conf import settings
+from django.test.utils import override_settings
+
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
+from student.tests.factories import UserFactory, AdminFactory, CourseEnrollmentFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
-from student.tests.factories import UserFactory, AdminFactory, CourseEnrollmentFactory
+
+from mock import patch
 
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
@@ -27,6 +31,24 @@ class TestOptoutCourseEmails(ModuleStoreTestCase):
 
         self.client.login(username=self.student.username, password="test")
 
+    def navigate_to_email_view(self):
+        """Navigate to the instructor dash's email view"""
+        # Pull up email view on instructor dashboard
+        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
+        response = self.client.get(url)
+        email_link = '<a href="#" onclick="goto(\'Email\')" class="None">Email</a>'
+        # If this fails, it is likely because ENABLE_INSTRUCTOR_EMAIL is set to False
+        self.assertTrue(email_link in response.content)
+
+        # Select the Email view of the instructor dash
+        session = self.client.session
+        session['idash_mode'] = 'Email'
+        session.save()
+        response = self.client.get(url)
+        selected_email_link = '<a href="#" onclick="goto(\'Email\')" class="selectedmode">Email</a>'
+        self.assertTrue(selected_email_link in response.content)
+
+    @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True})
     def test_optout_course(self):
         """
         Make sure student does not receive course email after opting out.
@@ -36,15 +58,24 @@ class TestOptoutCourseEmails(ModuleStoreTestCase):
         self.assertEquals(json.loads(response.content), {'success': True})
 
         self.client.logout()
+
         self.client.login(username=self.instructor.username, password="test")
+        self.navigate_to_email_view()
 
         url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        response = self.client.post(url, {'action': 'Send email', 'to_option': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
+        test_email = {
+            'action': 'Send email',
+            'to_option': 'all',
+            'subject': 'test subject for all',
+            'message': 'test message for all'
+        }
+        response = self.client.post(url, test_email)
         self.assertContains(response, "Your email was successfully queued for sending.")
 
-        #assert that self.student.email not in mail.to, outbox should be empty
+        # Assert that self.student.email not in mail.to, outbox should be empty
         self.assertEqual(len(mail.outbox), 0)
 
+    @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True})
     def test_optin_course(self):
         """
         Make sure student receives course email after opting in.
@@ -54,13 +85,22 @@ class TestOptoutCourseEmails(ModuleStoreTestCase):
         self.assertEquals(json.loads(response.content), {'success': True})
 
         self.client.logout()
+
         self.client.login(username=self.instructor.username, password="test")
+        self.navigate_to_email_view()
 
         url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        response = self.client.post(url, {'action': 'Send email', 'to_option': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
+        test_email = {
+            'action': 'Send email',
+            'to_option': 'all',
+            'subject': 'test subject for all',
+            'message': 'test message for all'
+        }
+        response = self.client.post(url, test_email)
+
         self.assertContains(response, "Your email was successfully queued for sending.")
 
-        #assert that self.student.email in mail.to
+        # 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)

lms/djangoapps/bulk_email/tests/test_email.py

@@ -3,52 +3,79 @@ Unit tests for sending course email
 """
 
 from django.test.utils import override_settings
+from django.conf import settings
+from django.core import mail
 from django.core.urlresolvers import reverse
+
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
+from student.tests.factories import UserFactory, GroupFactory, CourseEnrollmentFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
-from student.tests.factories import UserFactory, GroupFactory, CourseEnrollmentFactory
-from django.core import mail
+
 from bulk_email.tasks import delegate_email_batches, course_email
 from bulk_email.models import CourseEmail
 
+from mock import patch
+
 STAFF_COUNT = 3
 STUDENT_COUNT = 10
 
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
-class TestEmail(ModuleStoreTestCase):
-
+class TestEmailSendFromDashboard(ModuleStoreTestCase):
     """
     Test that emails send correctly.
     """
 
+    @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True})
     def setUp(self):
         self.course = CourseFactory.create()
         self.instructor = UserFactory.create(username="instructor", email="robot+instructor@edx.org")
-        #Create instructor group for course
+        # Create instructor group for course
         instructor_group = GroupFactory.create(name="instructor_MITx/999/Robot_Super_Course")
         instructor_group.user_set.add(self.instructor)
 
-        #create staff
+        # Create staff
         self.staff = [UserFactory() for _ in xrange(STAFF_COUNT)]
         staff_group = GroupFactory()
         for staff in self.staff:
             staff_group.user_set.add(staff)  # pylint: disable=E1101
 
-        #create students
+        # Create students
         self.students = [UserFactory() for _ in xrange(STUDENT_COUNT)]
         for student in self.students:
             CourseEnrollmentFactory.create(user=student, course_id=self.course.id)
 
         self.client.login(username=self.instructor.username, password="test")
 
+        # Pull up email view on instructor dashboard
+        self.url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
+        response = self.client.get(self.url)
+        email_link = '<a href="#" onclick="goto(\'Email\')" class="None">Email</a>'
+        # If this fails, it is likely because ENABLE_INSTRUCTOR_EMAIL is set to False
+        self.assertTrue(email_link in response.content)
+
+        # Select the Email view of the instructor dash
+        session = self.client.session
+        session['idash_mode'] = 'Email'
+        session.save()
+        response = self.client.get(self.url)
+        selected_email_link = '<a href="#" onclick="goto(\'Email\')" class="selectedmode">Email</a>'
+        self.assertTrue(selected_email_link in response.content)
+
     def test_send_to_self(self):
         """
         Make sure email send to myself goes to myself.
         """
-        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        response = self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for 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 email',
+            'to_option': 'myself',
+            'subject': 'test subject for myself',
+            'message': 'test message for myself'
+        }
+        response = self.client.post(self.url, test_email)
 
         self.assertContains(response, "Your email was successfully queued for sending.")
 
@@ -61,8 +88,15 @@ class TestEmail(ModuleStoreTestCase):
         """
         Make sure email send to staff and instructors goes there.
         """
-        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        response = self.client.post(url, {'action': 'Send email', 'to_option': 'staff', 'subject': 'test subject for staff', 'message': 'test message for subject'})
+        # 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',
+            'to_option': 'staff',
+            'subject': 'test subject for staff',
+            'message': 'test message for subject'
+        }
+        response = self.client.post(self.url, test_email)
 
         self.assertContains(response, "Your email was successfully queued for sending.")
 
@@ -73,24 +107,35 @@ class TestEmail(ModuleStoreTestCase):
         """
         Make sure email send to all goes there.
         """
-        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        response = self.client.post(url, {'action': 'Send email', 'to_option': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
+        # 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',
+            'to_option': 'all',
+            'subject': 'test subject for all',
+            'message': 'test message for all'
+        }
+        response = self.client.post(self.url, test_email)
 
         self.assertContains(response, "Your email was successfully queued for sending.")
 
         self.assertEquals(len(mail.outbox), 1 + len(self.staff) + len(self.students))
         self.assertItemsEqual([e.to[0] for e in mail.outbox], [self.instructor.email] + [s.email for s in self.staff] + [s.email for s in self.students])
 
+
+@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
+class TestEmailSendExceptions(ModuleStoreTestCase):
+    """
+    Test that exceptions are handled correctly.
+    """
+
     def test_get_course_exc(self):
-        """
-        Make sure delegate_email_batches handles Http404 exception from get_course_by_id.
-        """
+        # Make sure delegate_email_batches handles Http404 exception from get_course_by_id.
         with self.assertRaises(Exception):
             delegate_email_batches("_", "_", "blah/blah/blah", "_", "_")
 
     def test_no_course_email_obj(self):
-        """
-        Make sure course_email handles CourseEmail.DoesNotExist exception.
-        """
+        # Make sure course_email handles CourseEmail.DoesNotExist exception.
         with self.assertRaises(CourseEmail.DoesNotExist):
             course_email("dummy hash", [], "_", "_", False)

lms/djangoapps/bulk_email/tests/test_err_handling.py

@@ -5,10 +5,12 @@ Unit tests for handling email sending errors
 from django.test.utils import override_settings
 from django.conf import settings
 from django.core.urlresolvers import reverse
+
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from student.tests.factories import UserFactory, AdminFactory, CourseEnrollmentFactory
+
 from bulk_email.tests.smtp_server_thread import FakeSMTPServerThread
 
 from mock import patch
@@ -17,9 +19,13 @@ from smtplib import SMTPDataError, SMTPServerDisconnected, SMTPConnectError
 TEST_SMTP_PORT = 1025
 
 
-@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE, EMAIL_BACKEND='django.core.mail.backends.smtp.EmailBackend', EMAIL_HOST='localhost', EMAIL_PORT=TEST_SMTP_PORT)
+@override_settings(
+    MODULESTORE=TEST_DATA_MONGO_MODULESTORE,
+    EMAIL_BACKEND='django.core.mail.backends.smtp.EmailBackend',
+    EMAIL_HOST='localhost',
+    EMAIL_PORT=TEST_SMTP_PORT
+)
 class TestEmailErrors(ModuleStoreTestCase):
-
     """
     Test that errors from sending email are handled properly.
     """
@@ -32,6 +38,8 @@ class TestEmailErrors(ModuleStoreTestCase):
         self.smtp_server_thread = FakeSMTPServerThread('localhost', TEST_SMTP_PORT)
         self.smtp_server_thread.start()
 
+        self.url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
+
     def tearDown(self):
         self.smtp_server_thread.stop()
 
@@ -40,9 +48,20 @@ class TestEmailErrors(ModuleStoreTestCase):
         """
         Test that celery handles transient SMTPDataErrors by retrying.
         """
-        self.smtp_server_thread.server.set_errtype("DATA", "454 Throttling failure: Daily message quota exceeded.")
-        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
+        self.smtp_server_thread.server.set_errtype(
+            "DATA",
+            "454 Throttling failure: Daily message quota exceeded."
+        )
+
+        test_email = {
+            'action': 'Send email',
+            'to_option': 'myself',
+            'subject': 'test subject for myself',
+            'message': 'test message for myself'
+        }
+        self.client.post(self.url, test_email)
+
+        # Test that we retry upon hitting a 4xx error
         self.assertTrue(retry.called)
         (_, kwargs) = retry.call_args
         exc = kwargs['exc']
@@ -54,16 +73,26 @@ class TestEmailErrors(ModuleStoreTestCase):
         """
         Test that celery handles permanent SMTPDataErrors by failing and not retrying.
         """
-        self.smtp_server_thread.server.set_errtype("DATA", "554 Message rejected: Email address is not verified.")
+        self.smtp_server_thread.server.set_errtype(
+            "DATA",
+            "554 Message rejected: Email address is not verified."
+        )
+
         students = [UserFactory() for _ in xrange(settings.EMAILS_PER_TASK)]
         for student in students:
             CourseEnrollmentFactory.create(user=student, course_id=self.course.id)
 
-        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        self.client.post(url, {'action': 'Send email', 'to_option': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
-        self.assertFalse(retry.called)
+        test_email = {
+            'action': 'Send email',
+            'to_option': 'all',
+            'subject': 'test subject for all',
+            'message': 'test message for all'
+        }
+        self.client.post(self.url, test_email)
 
-        #test that after the failed email, the rest send successfully
+        # We shouldn't retry when hitting a 5xx error
+        self.assertFalse(retry.called)
+        # Test that after the rejected email, the rest still successfully send
         ((sent, fail), _) = result.call_args
         self.assertEquals(fail, 1)
         self.assertEquals(sent, settings.EMAILS_PER_TASK - 1)
@@ -73,9 +102,18 @@ class TestEmailErrors(ModuleStoreTestCase):
         """
         Test that celery handles SMTPServerDisconnected by retrying.
         """
-        self.smtp_server_thread.server.set_errtype("DISCONN", "Server disconnected, please try again later.")
-        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
+        self.smtp_server_thread.server.set_errtype(
+            "DISCONN",
+            "Server disconnected, please try again later."
+        )
+        test_email = {
+            'action': 'Send email',
+            'to_option': 'myself',
+            'subject': 'test subject for myself',
+            'message': 'test message for myself'
+        }
+        self.client.post(self.url, test_email)
+
         self.assertTrue(retry.called)
         (_, kwargs) = retry.call_args
         exc = kwargs['exc']
@@ -86,10 +124,17 @@ class TestEmailErrors(ModuleStoreTestCase):
         """
         Test that celery handles SMTPConnectError by retrying.
         """
-        #SMTP reply is already specified in fake SMTP Channel created
+        # SMTP reply is already specified in fake SMTP Channel created
         self.smtp_server_thread.server.set_errtype("CONN")
-        url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
-        self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
+
+        test_email = {
+            'action': 'Send email',
+            'to_option': 'myself',
+            'subject': 'test subject for myself',
+            'message': 'test message for myself'
+        }
+        self.client.post(self.url, test_email)
+
         self.assertTrue(retry.called)
         (_, kwargs) = retry.call_args
         exc = kwargs['exc']

lms/djangoapps/bulk_email/tests/tests.py

@@ -26,18 +26,23 @@ class TestInstructorDashboardEmailView(ModuleStoreTestCase):
         instructor = AdminFactory.create()
         self.client.login(username=instructor.username, password="test")
 
+        # URL for instructor dash
+        self.url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
+        # URL for email view
+        self.email_link = '<a href="#" onclick="goto(\'Email\')" class="None">Email</a>'
+
     @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True})
     def test_email_flag_true(self):
-        response = self.client.get(reverse('instructor_dashboard',
-                                   kwargs={'course_id': self.course.id}))
-        email_link = '<a href="#" onclick="goto(\'Email\')" class="None">Email</a>'
-        self.assertTrue(email_link in response.content)
+        response = self.client.get(self.url)
+        self.assertTrue(self.email_link in response.content)
 
+        # Select the Email view of the instructor dash
         session = self.client.session
         session['idash_mode'] = 'Email'
         session.save()
-        response = self.client.get(reverse('instructor_dashboard',
-                                   kwargs={'course_id': self.course.id}))
+        response = self.client.get(self.url)
+
+        # Ensure we've selected the view properly and that the send_to field is present.
         selected_email_link = '<a href="#" onclick="goto(\'Email\')" class="selectedmode">Email</a>'
         self.assertTrue(selected_email_link in response.content)
         send_to_label = '<label for="id_to">Send to:</label>'
@@ -45,7 +50,5 @@ class TestInstructorDashboardEmailView(ModuleStoreTestCase):
 
     @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': False})
     def test_email_flag_false(self):
-        response = self.client.get(reverse('instructor_dashboard',
-                                   kwargs={'course_id': self.course.id}))
-        email_link = '<a href="#" onclick="goto(\'Email\')" class="None">Email</a>'
-        self.assertFalse(email_link in response.content)
+        response = self.client.get(self.url)
+        self.assertFalse(self.email_link in response.content)

lms/djangoapps/instructor/views/legacy.py

@@ -83,6 +83,7 @@ def instructor_dashboard(request, course_id):
     forum_admin_access = has_forum_access(request.user, course_id, FORUM_ROLE_ADMINISTRATOR)
 
     msg = ''
+    email_msg = ''
     to_option = None
     subject = None
     html_message = ''
@@ -717,10 +718,9 @@ def instructor_dashboard(request, course_id):
         tasks.delegate_email_batches.delay(email.hash, email.to, course_id, course_url, request.user.id)
 
         if to_option == "all":
-            msg = "<font color='green'>Your email was successfully queued for sending. Please note that for large public classe\
-s (~10k), it may take 1-2 hours to send all emails.</font>"
+            email_msg = '<div class="msg msg-confirm"><p class="copy">Your email was successfully queued for sending. Please note that for large public classes (~10k), it may take 1-2 hours to send all emails.</p></div>'
         else:
-            msg = "<font color='green'>Your email was successfully queued for sending.</font>"
+            email_msg = '<div class="msg msg-confirm"><p class="copy">Your email was successfully queued for sending.</p></div>'
 
     #----------------------------------------
     # psychometrics
@@ -809,6 +809,7 @@ s (~10k), it may take 1-2 hours to send all emails.</font>"
                'datatable': datatable,
                'course_stats': course_stats,
                'msg': msg,
+               'email_msg': email_msg,
                'modeflag': {idash_mode: 'selectedmode'},
                'to_option': to_option,  # email
                'subject': subject,      # email

lms/static/sass/course/instructor/_instructor.scss

@@ -17,5 +17,56 @@
       @extend .top-header;
     }
 	}
+
+  // form fields
+  .list-fields {
+    list-style: none;
+    margin: 0;
+    padding: 0;
+
+    .field {
+      margin-bottom: 20px;
+      padding: 0;
+
+      &:last-child {
+        margin-bottom: 0;
+      }
+    }
+  }
+
+  // system feedback - messages
+  .msg {
+    border-radius: 1px;
+    padding: 10px 15px;
+    margin-bottom: 20px;
+
+    .copy {
+      font-weight: 600;
+    }
+  }
+
+  .msg-confirm {
+    border-top: 2px solid green;
+    background: tint(green,90%);
+
+    .copy {
+      color: green;
+    }
+  }
+
+  .list-advice {
+    list-style: none;
+    padding: 0;
+    margin: 20px 0;
+
+    .item {
+      font-weight: 600;
+      margin-bottom: 10px;
+
+      &:last-child {
+        margin-bottom: 0;
+      }
+    }
+  }
 }
 

lms/templates/courseware/instructor_dashboard.html

@@ -443,38 +443,51 @@ function goto( mode)
 ##-----------------------------------------------------------------------------
 
 %if modeflag.get('Email'):
-    <p>
-    <label for="id_to">Send to:</label>
-    <select id="id_to" name="to_option">
-      <option value="myself">Myself</option>
-      %if to_option == "staff":
-        <option value="staff" selected="selected">Staff and instructors</option>
-      %else:
-	<option value="staff">Staff and instructors</option>
-      %endif
-      %if to_option == "all":
-        <option value="all" selected="selected">All (students, staff and instructors)</option>
-      %else:
-	<option value="all">All (students, staff and instructors)</option>
-      %endif
-    </select>
-    <label for="id_subject">Subject: </label>
-    %if subject:
-      <input type="text" id="id_subject" name="subject" maxlength="100" size="75" value="${subject}">
-    %else:
-      <input type="text" id="id_subject" name="subject" maxlength="100" size="75">
+    %if email_msg:
+        <p></p><p>${email_msg}</p>
     %endif
-    <label>Message:</label>
-    <div class="email-editor">
-      ${editor}
-    </div>
-    <input type="hidden" name="message" value="">
-    </p>
+
+    <ul class="list-fields">
+      <li class="field">
+        <label for="id_to">${_("Send to:")}</label>
+        <select id="id_to" name="to_option">
+          <option value="myself">${_("Myself")}</option>
+          %if to_option == "staff":
+          <option value="staff" selected="selected">${_("Staff and instructors")}</option>
+          %else:
+          <option value="staff">${_("Staff and instructors")}</option>
+              %endif
+              %if to_option == "all":
+                <option value="all" selected="selected">${_("All (students, staff and instructors)")}</option>
+              %else:
+          <option value="all">${_("All (students, staff and instructors)")}</option>
+          %endif
+        </select>
+      </li>
+
+      <li class="field">
+        <label for="id_subject">${_("Subject: ")}</label>
+        %if subject:
+          <input type="text" id="id_subject" name="subject" maxlength="100" size="75" value="${subject}">
+        %else:
+          <input type="text" id="id_subject" name="subject" maxlength="100" size="75">
+        %endif
+      </li>
+
+      <li class="field">
+        <label>Message:</label>
+        <div class="email-editor">
+          ${editor}
+        </div>
+        <input type="hidden" name="message" value="">
+      </li>
+    </ul>
+
     <div class="submit-email-action">
-      Please try not to email students more than once a day. Important things to consider before sending:
-      <ul>
-        <li>Have you read over the email to make sure it says everything you want to say?</li>
-        <li>Have you sent the email to yourself first to make sure you're happy with how it's displayed?</li>
+      ${_("Please try not to email students more than once a day. Important things to consider before sending:")}
+      <ul class="list-advice">
+        <li class="item">${_("Have you read over the email to make sure it says everything you want to say?")}</li>
+        <li class="item">${_("Have you sent the email to yourself first to make sure you're happy with how it's displayed?")}</li>
       </ul>
       <input type="submit" name="action" value="Send email">
     </div>
@@ -520,7 +533,7 @@ function goto( mode)
 
   %if analytics_results.get("StudentsDropoffPerDay"):
     <p>
-      ${_("Student activity day by day")} 
+      ${_("Student activity day by day")}
       (${analytics_results["StudentsDropoffPerDay"]['time']})
     </p>
     <div>

lms/templates/dashboard.html

@@ -306,7 +306,7 @@
               % endif
             % endif
             <a href="#unenroll-modal" class="unenroll" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}">${_('Unregister')}</a>
-            <a href="#email-settings-modal" class="email-settings" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}" data-optout="${course.id in course_optouts}">Email Settings</a>
+            <a href="#email-settings-modal" class="email-settings" rel="leanModal" data-course-id="${course.id}" data-course-number="${course.number}" data-optout="${course.id in course_optouts}">${_('Email Settings')}</a>
           </section>
         </article>
 
@@ -343,13 +343,13 @@
 <section id="email-settings-modal" class="modal">
   <div class="inner-wrapper">
     <header>
-      <h2>Email Settings for <span id="email_settings_course_number"></span></h2>
+      <h2>${_('Email Settings for {course_number}').format(course_number='<span id="email_settings_course_number"></span>')}</h2>
       <hr/>
     </header>
 
     <form id="email_settings_form" method="post">
       <input name="course_id" id="email_settings_course_id" type="hidden" />
-      <label>Receive course emails <input type="checkbox" id="receive_emails" name="receive_emails" /></label>
+      <label>${_("Receive course emails")} <input type="checkbox" id="receive_emails" name="receive_emails" /></label>
       <div class="submit">
         <input type="submit" id="submit" value="Save Settings" />
       </div>