Bulk email - final tweaks and cleanup

lms/djangoapps/bulk_email/tasks.py

@@ -27,24 +27,16 @@ log = get_task_logger(__name__)
 
 
 @task(default_retry_delay=10, max_retries=5)  # pylint: disable=E1102
-def delegate_email_batches(email_id, to_option, course_id, course_url, user_id):
+def delegate_email_batches(email_id, user_id):
     """
     Delegates emails by querying for the list of recipients who should
     get the mail, chopping up into batches of settings.EMAILS_PER_TASK size,
     and queueing up worker jobs.
 
-    `to_option` is {'myself', 'staff', or 'all'}
-
     Returns the number of batches (workers) kicked off.
     """
     try:
-        course = get_course_by_id(course_id)
-    except Http404 as exc:
-        log.error("get_course_by_id failed: %s", exc.args[0])
-        raise Exception("get_course_by_id failed: " + exc.args[0])
-
-    try:
-        CourseEmail.objects.get(id=email_id)
+        email_obj = CourseEmail.objects.get(id=email_id)
     except CourseEmail.DoesNotExist as exc:
         # The retry behavior here is necessary because of a race condition between the commit of the transaction
         # that creates this CourseEmail row and the celery pipeline that starts this task.
@@ -87,7 +79,6 @@ def delegate_email_batches(email_id, to_option, course_id, course_url, user_id):
         log.error("Unexpected bulk email TO_OPTION found: %s", to_option)
         raise Exception("Unexpected bulk email TO_OPTION found: {0}".format(to_option))
 
-    image_url = course_image_url(course)
     recipient_qset = recipient_qset.order_by('pk')
     total_num_emails = recipient_qset.count()
     num_queries = int(math.ceil(float(total_num_emails) / float(settings.EMAILS_PER_QUERY)))
@@ -135,9 +126,10 @@ def course_email(email_id, to_list, course_title, course_url, image_url, throttl
                                      user__in=[i['pk'] for i in to_list])
                              .values_list('user__email', flat=True))
 
+    optouts = set(optouts)
     num_optout = len(optouts)
 
-    to_list = filter(lambda x: x['email'] not in set(optouts), to_list)
+    to_list = filter(lambda x: x['email'] not in optouts, to_list)
 
     subject = "[" + course_title + "] " + msg.subject
 
@@ -208,6 +200,9 @@ def course_email(email_id, to_list, course_title, course_url, image_url, throttl
 
     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
+        # Reasoning is that all of these errors may be temporary condition.
+        log.warning('Email with id %d not delivered due to temporary error %s, retrying send to %d recipients',
+                    email_id, exc, len(to_list))
         raise course_email.retry(
             arg=[
                 email_id,
@@ -220,6 +215,11 @@ def course_email(email_id, to_list, course_title, course_url, image_url, throttl
             exc=exc,
             countdown=(2 ** current_task.request.retries) * 15
         )
+    except:
+        log.exception('Email with id %d caused course_email task to fail with uncaught exception. To list: %s',
+                      email_id,
+                      [i['email'] for i in to_list])
+        raise
 
 
 # This string format code is wrapped in this function to allow mocking for a unit test

lms/djangoapps/bulk_email/tests/test_err_handling.py

@@ -12,14 +12,20 @@ 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.models import CourseEmail
+from bulk_email.tasks import delegate_email_batches
 from bulk_email.tests.smtp_server_thread import FakeSMTPServerThread
 
-from mock import patch
+from mock import patch, Mock
 from smtplib import SMTPDataError, SMTPServerDisconnected, SMTPConnectError
 
 TEST_SMTP_PORT = 1025
 
 
+class EmailTestException(Exception):
+    pass
+
+
 @override_settings(
     MODULESTORE=TEST_DATA_MONGO_MODULESTORE,
     EMAIL_BACKEND='django.core.mail.backends.smtp.EmailBackend',
@@ -33,8 +39,8 @@ class TestEmailErrors(ModuleStoreTestCase):
 
     def setUp(self):
         self.course = CourseFactory.create()
-        instructor = AdminFactory.create()
-        self.client.login(username=instructor.username, password="test")
+        self.instructor = AdminFactory.create()
+        self.client.login(username=self.instructor.username, password="test")
 
         # load initial content (since we don't run migrations as part of tests):
         call_command("loaddata", "course_email_template.json")
@@ -145,3 +151,68 @@ class TestEmailErrors(ModuleStoreTestCase):
         (_, kwargs) = retry.call_args
         exc = kwargs['exc']
         self.assertTrue(type(exc) == SMTPConnectError)
+
+    @patch('bulk_email.tasks.course_email_result')
+    @patch('bulk_email.tasks.course_email.retry')
+    @patch('bulk_email.tasks.log')
+    @patch('bulk_email.tasks.get_connection', Mock(return_value=EmailTestException))
+    def test_general_exception(self, mock_log, retry, result):
+        """
+        Tests the if the error is not SMTP-related, we log and reraise
+        """
+        test_email = {
+            'action': 'Send email',
+            'to_option': 'myself',
+            'subject': 'test subject for myself',
+            'message': 'test message for myself'
+        }
+        # For some reason (probably the weirdness of testing with celery tasks) assertRaises doesn't work here
+        # so we assert on the arguments of log.exception
+        self.client.post(self.url, test_email)
+        ((log_str, email_id, to_list), _) = mock_log.exception.call_args
+        self.assertTrue(mock_log.exception.called)
+        self.assertIn('caused course_email task to fail with uncaught exception.', log_str)
+        self.assertEqual(email_id, 1)
+        self.assertEqual(to_list, [self.instructor.email])
+        self.assertFalse(retry.called)
+        self.assertFalse(result.called)
+
+    @patch('bulk_email.tasks.course_email_result')
+    @patch('bulk_email.tasks.delegate_email_batches.retry')
+    @patch('bulk_email.tasks.log')
+    def test_nonexist_email(self, mock_log, retry, result):
+        """
+        Tests retries when the email doesn't exist
+        """
+        delegate_email_batches.delay(-1, self.instructor.id)
+        ((log_str, email_id, num_retries), _) = mock_log.warning.call_args
+        self.assertTrue(mock_log.warning.called)
+        self.assertIn('Failed to get CourseEmail with id', log_str)
+        self.assertEqual(email_id, -1)
+        self.assertTrue(retry.called)
+        self.assertFalse(result.called)
+
+    @patch('bulk_email.tasks.log')
+    def test_nonexist_course(self, mock_log):
+        """
+        Tests exception when the course in the email doesn't exist
+        """
+        email = CourseEmail(course_id="I/DONT/EXIST")
+        email.save()
+        delegate_email_batches.delay(email.id, self.instructor.id)
+        ((log_str, _), _) = mock_log.exception.call_args
+        self.assertTrue(mock_log.exception.called)
+        self.assertIn('get_course_by_id failed:', log_str)
+
+    @patch('bulk_email.tasks.log')
+    def test_nonexist_to_option(self, mock_log):
+        """
+        Tests exception when the to_option in the email doesn't exist
+        """
+        email = CourseEmail(course_id=self.course.id, to_option="IDONTEXIST")
+        email.save()
+        delegate_email_batches.delay(email.id, self.instructor.id)
+        ((log_str, opt_str), _) = mock_log.error.call_args
+        self.assertTrue(mock_log.error.called)
+        self.assertIn('Unexpected bulk email TO_OPTION found', log_str)
+        self.assertEqual("IDONTEXIST", opt_str)

lms/djangoapps/bulk_email/tests/tests.py → lms/djangoapps/instructor/tests/test_legacy_email.py

@@ -99,12 +99,12 @@ class TestStudentDashboardEmailView(ModuleStoreTestCase):
         # URL for dashboard
         self.url = reverse('dashboard')
         # URL for email settings modal
-        self.email_modal_link = '<a href="#email-settings-modal" class="email-settings" rel="leanModal" data-course-id="{0}/{1}/{2}" data-course-number="{1}" data-optout="False">Email Settings</a>'.format(
-            self.course.org,
-            self.course.number,
-            self.course.display_name.replace(' ', '_')
-            )
-
+        self.email_modal_link = (('<a href="#email-settings-modal" class="email-settings" rel="leanModal" '
+                                 'data-course-id="{0}/{1}/{2}" data-course-number="{1}" '
+                                 'data-optout="False">Email Settings</a>')
+                                 .format(self.course.org,
+                                         self.course.number,
+                                         self.course.display_name.replace(' ', '_')))
 
     def tearDown(self):
         """
@@ -118,7 +118,6 @@ class TestStudentDashboardEmailView(ModuleStoreTestCase):
         response = self.client.get(self.url)
         self.assertTrue(self.email_modal_link in response.content)
 
-
     @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': False})
     def test_email_flag_false(self):
         # Assert that the URL for the email view is not in the response

lms/djangoapps/instructor/views/legacy.py

@@ -84,8 +84,8 @@ def instructor_dashboard(request, course_id):
 
     msg = ''
     email_msg = ''
-    to_option = None
-    subject = None
+    email_to_option = None
+    email_subject = None
     html_message = ''
     show_email_tab = False
     problems = []
@@ -703,30 +703,26 @@ def instructor_dashboard(request, course_id):
     # email
 
     elif action == 'Send email':
-        to_option = request.POST.get("to_option")
-        subject = request.POST.get("subject")
+        email_to_option = request.POST.get("to_option")
+        email_subject = request.POST.get("subject")
         html_message = request.POST.get("message")
         text_message = html_to_text(html_message)
 
         email = CourseEmail(course_id=course_id,
                             sender=request.user,
-                            to_option=to_option,
-                            subject=subject,
+                            to_option=email_to_option,
+                            subject=email_subject,
                             html_message=html_message,
                             text_message=text_message)
 
         email.save()
 
-        course_url = request.build_absolute_uri(reverse('course_root', kwargs={'course_id': course_id}))
         tasks.delegate_email_batches.delay(
             email.id,
-            email.to_option,
-            course_id,
-            course_url,
             request.user.id
         )
 
-        if to_option == "all":
+        if email_to_option == "all":
             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:
             email_msg = '<div class="msg msg-confirm"><p class="copy">Your email was successfully queued for sending.</p></div>'
@@ -799,9 +795,9 @@ def instructor_dashboard(request, course_id):
     # HTML editor for email
     if idash_mode == 'Email':
         html_module = HtmlDescriptor(course.system, {'data': html_message})
-        editor = wrap_xmodule(html_module.get_html, html_module, 'xmodule_edit.html')()
+        email_editor = wrap_xmodule(html_module.get_html, html_module, 'xmodule_edit.html')()
     else:
-        editor = None
+        email_editor = None
 
     # Flag for whether or not we display the email tab (depending upon
     # what backing store this course using (Mongo vs. XML))
@@ -825,11 +821,13 @@ def instructor_dashboard(request, course_id):
                'course_stats': course_stats,
                'msg': msg,
                'modeflag': {idash_mode: 'selectedmode'},
-               'to_option': to_option,            # email
-               'subject': subject,                # email
-               'editor': editor,                  # email
+
+               'to_option': email_to_option,      # email
+               'subject': email_subject,          # email
+               'editor': email_editor,            # email
                'email_msg': email_msg,            # email
                'show_email_tab': show_email_tab,  # email
+
                'problems': problems,		# psychometrics
                'plots': plots,			# psychometrics
                'course_errors': modulestore().get_item_errors(course.location),

lms/templates/emails/email_footer.html

-<%! from django.core.urlresolvers import reverse %>
-<br />
-----<br />
-This email was automatically sent from ${settings.PLATFORM_NAME}. <br />
-You are receiving this email at address ${ email } because you are enrolled in <a href="${course_url}">${ course_title }</a>.<br />
-To stop receiving email like this, update your course email settings <a href="https://${settings.SITE_NAME}${reverse('dashboard')}">here</a>. <br />

lms/templates/emails/email_footer.txt

-<%! from django.core.urlresolvers import reverse %>
-
-----
-This email was automatically sent from ${settings.PLATFORM_NAME}.
-You are receiving this email at address ${ email } because you are enrolled in ${ course_title }
-(URL: ${course_url} ).
-To stop receiving email like this, update your account settings at https://${settings.SITE_NAME}${reverse('dashboard')}.