From c160a189ad65493bbd3c0b34ec61bc4b2ed9e62e Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Tue, 27 Aug 2013 10:22:25 -0700 Subject: [PATCH] Bulk email - final tweaks and cleanup --- .../fixtures/course_email_template.json | 2 +- lms/djangoapps/bulk_email/tasks.py | 24 +++--- .../bulk_email/tests/test_err_handling.py | 77 ++++++++++++++++++- .../tests/test_legacy_email.py} | 13 ++-- lms/djangoapps/instructor/views/legacy.py | 30 ++++---- lms/templates/emails/email_footer.html | 6 -- lms/templates/emails/email_footer.txt | 7 -- 7 files changed, 107 insertions(+), 52 deletions(-) rename lms/djangoapps/{bulk_email/tests/tests.py => instructor/tests/test_legacy_email.py} (92%) delete mode 100644 lms/templates/emails/email_footer.html delete mode 100644 lms/templates/emails/email_footer.txt diff --git a/lms/djangoapps/bulk_email/fixtures/course_email_template.json b/lms/djangoapps/bulk_email/fixtures/course_email_template.json index 076dedbd14..bea136df38 100644 --- a/lms/djangoapps/bulk_email/fixtures/course_email_template.json +++ b/lms/djangoapps/bulk_email/fixtures/course_email_template.json @@ -3,7 +3,7 @@ "pk": 1, "model": "bulk_email.courseemailtemplate", "fields": { - "plain_template": "{course_title}\n\n{{message_body}}\r\n----\r\nCopyright 2013 edX, All rights reserved.\r\n----\r\nConnect with edX: Facebook (http://facebook.com/edxonline)\nTwitter (http://twitter.com/edxonline)\nGoogle+ (https://plus.google.com/108235383044095082735)\nMeetup (http://www.meetup.com/edX-Communities/)\r\n----\r\n This email was automatically sent from {platform_name}.\r\nYou are receiving this email at address {email} because you are enrolled in {course_title}\r\n(URL: {course_url} ).\r\nTo stop receiving email like this, update your account settings at {account_settings_url}.\r\n", + "plain_template": "{course_title}\n\n{{message_body}}\r\n----\r\nCopyright 2013 edX, All rights reserved.\r\n----\r\nConnect with edX:\r\nFacebook (http://facebook.com/edxonline)\r\nTwitter (http://twitter.com/edxonline)\r\nGoogle+ (https://plus.google.com/108235383044095082735)\r\nMeetup (http://www.meetup.com/edX-Communities/)\r\n----\r\nThis email was automatically sent from {platform_name}.\r\nYou are receiving this email at address {email} because you are enrolled in {course_title}\r\n(URL: {course_url} ).\r\nTo stop receiving email like this, update your account settings at {account_settings_url}.\r\n", "html_template": " Update from {course_title}

edX
Connect with edX:        

{course_title}
{{message_body}}
       
Copyright © 2013 edX, All rights reserved.


Our mailing address is:
edX
11 Cambridge Center, Suite 101
Cambridge, MA, USA 02142


This email was automatically sent from {platform_name}.
You are receiving this email at address {email} because you are enrolled in {course_title}.
To stop receiving email like this, update your course email settings here.
" } } diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 2153090844..c9be4f5347 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/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 diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index e8874ea18e..61bdd315e9 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/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) diff --git a/lms/djangoapps/bulk_email/tests/tests.py b/lms/djangoapps/instructor/tests/test_legacy_email.py similarity index 92% rename from lms/djangoapps/bulk_email/tests/tests.py rename to lms/djangoapps/instructor/tests/test_legacy_email.py index 210ae63df0..d8761466b0 100644 --- a/lms/djangoapps/bulk_email/tests/tests.py +++ b/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 = 'Email Settings'.format( - self.course.org, - self.course.number, - self.course.display_name.replace(' ', '_') - ) - + self.email_modal_link = (('Email Settings') + .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 diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index 7326fe2e98..b9295557d7 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/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 = '

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.

' else: email_msg = '

Your email was successfully queued for sending.

' @@ -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), diff --git a/lms/templates/emails/email_footer.html b/lms/templates/emails/email_footer.html deleted file mode 100644 index ad9813c601..0000000000 --- a/lms/templates/emails/email_footer.html +++ /dev/null @@ -1,6 +0,0 @@ -<%! 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 }.
-To stop receiving email like this, update your course email settings here.
diff --git a/lms/templates/emails/email_footer.txt b/lms/templates/emails/email_footer.txt deleted file mode 100644 index 95dffc218e..0000000000 --- a/lms/templates/emails/email_footer.txt +++ /dev/null @@ -1,7 +0,0 @@ -<%! 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')}.