From 3b32d4216c430d908d57f3b905ab40f1fb50a2cb Mon Sep 17 00:00:00 2001
From: Kevin Luo <kevkluo@gmail.com>
Date: Fri, 26 Jul 2013 16:21:50 -0700
Subject: [PATCH] Add delay to course bulk email task and use SITE_NAME for
site url
Delay for possible race condition with fetching course email object.
Use settings.SITE_NAME for host name to generate email footer url.
---
lms/djangoapps/bulk_email/admin.py | 13 ++++++-
lms/djangoapps/bulk_email/models.py | 3 ++
lms/djangoapps/bulk_email/tasks.py | 37 ++++++++++++-------
lms/djangoapps/bulk_email/tests/fake_smtp.py | 18 ++++++---
.../bulk_email/tests/smtp_server_thread.py | 18 ++++++---
.../bulk_email/tests/test_course_optout.py | 9 ++++-
lms/djangoapps/bulk_email/tests/test_email.py | 13 +++++--
.../bulk_email/tests/test_err_handling.py | 13 +++++--
lms/djangoapps/instructor/views/legacy.py | 14 +++----
lms/static/sass/base/_variables.scss | 5 +++
.../courseware/instructor_dashboard.html | 6 +--
lms/templates/emails/email_footer.html | 4 +-
lms/templates/emails/email_footer.txt | 4 +-
13 files changed, 108 insertions(+), 49 deletions(-)
diff --git a/lms/djangoapps/bulk_email/admin.py b/lms/djangoapps/bulk_email/admin.py
index fe151741bc4..40da36629c8 100644
--- a/lms/djangoapps/bulk_email/admin.py
+++ b/lms/djangoapps/bulk_email/admin.py
@@ -1,11 +1,20 @@
+"""
+Django admin page for bulk email models
+"""
from django.contrib import admin
from bulk_email.models import CourseEmail, Optout
-admin.site.register(Optout)
-
class CourseEmailAdmin(admin.ModelAdmin):
+ """Admin for course email."""
readonly_fields = ('sender',)
+
+class OptoutAdmin(admin.ModelAdmin):
+ """Admin for optouts."""
+ list_display = ('email', 'course_id')
+
+
admin.site.register(CourseEmail, CourseEmailAdmin)
+admin.site.register(Optout, OptoutAdmin)
diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py
index 2804521841e..96753a44a71 100644
--- a/lms/djangoapps/bulk_email/models.py
+++ b/lms/djangoapps/bulk_email/models.py
@@ -1,3 +1,6 @@
+"""
+Models for bulk email
+"""
from django.db import models
from django.contrib.auth.models import User
diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py
index 9047f4e5695..3da03bb3dc6 100644
--- a/lms/djangoapps/bulk_email/tasks.py
+++ b/lms/djangoapps/bulk_email/tasks.py
@@ -1,3 +1,7 @@
+"""
+This module contains celery task functions for handling the sending of bulk email
+to a course.
+"""
import logging
import math
import re
@@ -20,24 +24,30 @@ from mitxmako.shortcuts import render_to_string
log = logging.getLogger(__name__)
-@task()
-def delegate_email_batches(hash_for_msg, recipient, course_id, course_url, user_id):
- '''
+@task(default_retry_delay=10, max_retries=5) # pylint: disable=E1102
+def delegate_email_batches(hash_for_msg, to_option, course_id, course_url, 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.
- Recipient is {'students', 'staff', or 'all'}
+ `to_option` is {'students', '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: " + exc.args[0])
raise Exception("get_course_by_id failed: " + exc.args[0])
- if recipient == "myself":
+ try:
+ CourseEmail.objects.get(hash=hash_for_msg)
+ except CourseEmail.DoesNotExist as exc:
+ log.warning("Failed to get CourseEmail with hash %s, retry %d", hash_for_msg, current_task.request.retries)
+ raise delegate_email_batches.retry(arg=[hash_for_msg, to_option, course_id, course_url, user_id], exc=exc)
+
+ if to_option == "myself":
recipient_qset = User.objects.filter(id=user_id).values('profile__name', 'email')
else:
staff_grpname = _course_staff_group_name(course.location)
@@ -48,9 +58,9 @@ def delegate_email_batches(hash_for_msg, recipient, course_id, course_url, user_
instructor_qset = instructor_group.user_set.values('profile__name', 'email')
recipient_qset = staff_qset | instructor_qset
- if recipient == "all":
- #Execute two queries per performance considerations for MySQL
- #https://docs.djangoproject.com/en/1.2/ref/models/querysets/#in
+ if to_option == "all":
+ # Two queries are executed per performance considerations for MySQL.
+ # See https://docs.djangoproject.com/en/1.2/ref/models/querysets/#in.
course_optouts = Optout.objects.filter(course_id=course_id).values_list('email', flat=True)
enrollment_qset = User.objects.filter(courseenrollment__course_id=course_id).exclude(email__in=list(course_optouts)).values('profile__name', 'email')
recipient_qset = recipient_qset | enrollment_qset
@@ -67,7 +77,7 @@ def delegate_email_batches(hash_for_msg, recipient, course_id, course_url, user_
return num_workers
-@task(default_retry_delay=15, max_retries=5)
+@task(default_retry_delay=15, max_retries=5) # pylint: disable=E1102
def course_email(hash_for_msg, to_list, course_title, course_url, throttle=False):
"""
Takes a subject and an html formatted email and sends it from
@@ -127,7 +137,7 @@ def course_email(hash_for_msg, to_list, course_title, course_url, throttle=False
raise exc # this will cause the outer handler to catch the exception and retry the entire task
else:
#this will fall through and not retry the message, since it will be popped
- log.warn('Email with hash ' + hash_for_msg + ' not delivered to ' + email + ' due to error: ' + exc.smtp_error)
+ log.warning('Email with hash ' + hash_for_msg + ' not delivered to ' + email + ' due to error: ' + exc.smtp_error)
num_error += 1
to_list.pop()
@@ -140,6 +150,7 @@ def course_email(hash_for_msg, to_list, course_title, course_url, throttle=False
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
+# This string format code is wrapped in this function to allow mocking for a unit test
def course_email_result(num_sent, num_error):
- return "Sent %d, Fail %d" % (num_sent, num_error)
+ """Return the formatted result of course_email sending."""
+ return "Sent {0}, Fail {1}".format(num_sent, num_error)
diff --git a/lms/djangoapps/bulk_email/tests/fake_smtp.py b/lms/djangoapps/bulk_email/tests/fake_smtp.py
index cf5872c3330..b6fb9b92a5a 100755
--- a/lms/djangoapps/bulk_email/tests/fake_smtp.py
+++ b/lms/djangoapps/bulk_email/tests/fake_smtp.py
@@ -51,11 +51,18 @@ class FakeSMTPServer(smtpd.SMTPServer):
def __init__(self, *args, **kwargs):
smtpd.SMTPServer.__init__(self, *args, **kwargs)
self.errtype = None
- self.reply = None
+ self.response = None
- def set_errtype(self, errtype, reply=''):
+ def set_errtype(self, errtype, response=''):
+ """Specify the type of error to cause smptlib to raise, with optional response string.
+
+ `errtype` -- "DATA": The server will cause smptlib to throw SMTPDataError.
+ "CONN": The server will cause smptlib to throw SMTPConnectError.
+ "DISCONN": The server will cause smptlib to throw SMTPServerDisconnected.
+
+ """
self.errtype = errtype
- self.reply = reply
+ self.response = response
def handle_accept(self):
if self.errtype == "DISCONN":
@@ -70,11 +77,12 @@ class FakeSMTPServer(smtpd.SMTPServer):
def process_message(self, *_args, **_kwargs):
if self.errtype == "DATA":
- #after failing on the first email, succeed on rest
+ # After failing on the first email, succeed on the rest.
self.errtype = None
- return self.reply
+ return self.response
else:
return None
def serve_forever(self):
+ """Start the server running until close() is called on the server."""
asyncore.loop()
diff --git a/lms/djangoapps/bulk_email/tests/smtp_server_thread.py b/lms/djangoapps/bulk_email/tests/smtp_server_thread.py
index 77b51509f13..713cd9ca642 100644
--- a/lms/djangoapps/bulk_email/tests/smtp_server_thread.py
+++ b/lms/djangoapps/bulk_email/tests/smtp_server_thread.py
@@ -1,10 +1,14 @@
+"""
+Defines a class for a thread that runs a Fake SMTP server, used for testing
+error handling from sending email.
+"""
import threading
from bulk_email.tests.fake_smtp import FakeSMTPServer
class FakeSMTPServerThread(threading.Thread):
"""
- Thread for running a fake SMTP server for testing email
+ Thread for running a fake SMTP server
"""
def __init__(self, host, port):
self.host = host
@@ -19,21 +23,25 @@ class FakeSMTPServerThread(threading.Thread):
super(FakeSMTPServerThread, self).start()
self.is_ready.wait()
if self.error:
- raise self.error
+ raise self.error # pylint: disable=E0702
def stop(self):
+ """
+ Stop the thread by closing the server instance.
+ Wait for the server thread to terminate.
+ """
if hasattr(self, 'server'):
self.server.close()
self.join()
def run(self):
"""
- Sets up the test smtp server and handle requests
+ Sets up the test smtp server and handle requests.
"""
try:
self.server = FakeSMTPServer((self.host, self.port), None)
self.is_ready.set()
self.server.serve_forever()
- except Exception, e:
- self.error = e
+ except Exception, exc: # pylint: disable=W0703
+ self.error = exc
self.is_ready.set()
diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py
index e91b11c3140..ece0e879c74 100644
--- a/lms/djangoapps/bulk_email/tests/test_course_optout.py
+++ b/lms/djangoapps/bulk_email/tests/test_course_optout.py
@@ -14,6 +14,11 @@ from student.tests.factories import UserFactory, AdminFactory, CourseEnrollmentF
@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
class TestOptoutCourseEmails(ModuleStoreTestCase):
+
+ """
+ Test that optouts are referenced in sending course email.
+ """
+
def setUp(self):
self.course = CourseFactory.create()
self.instructor = AdminFactory.create()
@@ -34,7 +39,7 @@ class TestOptoutCourseEmails(ModuleStoreTestCase):
self.client.login(username=self.instructor.username, password="test")
url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
- response = self.client.post(url, {'action': 'Send email', 'to': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
+ response = self.client.post(url, {'action': 'Send email', 'to_option': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
self.assertContains(response, "Your email was successfully queued for sending.")
#assert that self.student.email not in mail.to, outbox should be empty
@@ -52,7 +57,7 @@ class TestOptoutCourseEmails(ModuleStoreTestCase):
self.client.login(username=self.instructor.username, password="test")
url = reverse('instructor_dashboard', kwargs={'course_id': self.course.id})
- response = self.client.post(url, {'action': 'Send email', 'to': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
+ response = self.client.post(url, {'action': 'Send email', 'to_option': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
self.assertContains(response, "Your email was successfully queued for sending.")
#assert that self.student.email in mail.to
diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py
index 7aebd679f38..bc626d2d3bd 100644
--- a/lms/djangoapps/bulk_email/tests/test_email.py
+++ b/lms/djangoapps/bulk_email/tests/test_email.py
@@ -18,6 +18,11 @@ STUDENT_COUNT = 10
@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
class TestEmail(ModuleStoreTestCase):
+
+ """
+ Test that emails send correctly.
+ """
+
def setUp(self):
self.course = CourseFactory.create()
self.instructor = UserFactory.create(username="instructor", email="robot+instructor@edx.org")
@@ -29,7 +34,7 @@ class TestEmail(ModuleStoreTestCase):
self.staff = [UserFactory() for _ in xrange(STAFF_COUNT)]
staff_group = GroupFactory()
for staff in self.staff:
- staff_group.user_set.add(staff)
+ staff_group.user_set.add(staff) # pylint: disable=E1101
#create students
self.students = [UserFactory() for _ in xrange(STUDENT_COUNT)]
@@ -43,7 +48,7 @@ class TestEmail(ModuleStoreTestCase):
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': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
+ response = self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
self.assertContains(response, "Your email was successfully queued for sending.")
@@ -57,7 +62,7 @@ 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': 'staff', 'subject': 'test subject for staff', 'message': 'test message for subject'})
+ response = self.client.post(url, {'action': 'Send email', 'to_option': 'staff', 'subject': 'test subject for staff', 'message': 'test message for subject'})
self.assertContains(response, "Your email was successfully queued for sending.")
@@ -69,7 +74,7 @@ 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': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
+ response = self.client.post(url, {'action': 'Send email', 'to_option': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
self.assertContains(response, "Your email was successfully queued for sending.")
diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py
index faf6d38a87d..fd900cffc33 100644
--- a/lms/djangoapps/bulk_email/tests/test_err_handling.py
+++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py
@@ -19,6 +19,11 @@ 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)
class TestEmailErrors(ModuleStoreTestCase):
+
+ """
+ Test that errors from sending email are handled properly.
+ """
+
def setUp(self):
self.course = CourseFactory.create()
instructor = AdminFactory.create()
@@ -37,7 +42,7 @@ class TestEmailErrors(ModuleStoreTestCase):
"""
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': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
+ self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
self.assertTrue(retry.called)
(_, kwargs) = retry.call_args
exc = kwargs['exc']
@@ -55,7 +60,7 @@ class TestEmailErrors(ModuleStoreTestCase):
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': 'all', 'subject': 'test subject for all', 'message': 'test message for all'})
+ 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 that after the failed email, the rest send successfully
@@ -70,7 +75,7 @@ class TestEmailErrors(ModuleStoreTestCase):
"""
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': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
+ self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
self.assertTrue(retry.called)
(_, kwargs) = retry.call_args
exc = kwargs['exc']
@@ -84,7 +89,7 @@ class TestEmailErrors(ModuleStoreTestCase):
#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': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
+ self.client.post(url, {'action': 'Send email', 'to_option': 'myself', 'subject': 'test subject for myself', 'message': 'test message for myself'})
self.assertTrue(retry.called)
(_, kwargs) = retry.call_args
exc = kwargs['exc']
diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py
index 0aacf516a87..a6cc2cf40bc 100644
--- a/lms/djangoapps/instructor/views/legacy.py
+++ b/lms/djangoapps/instructor/views/legacy.py
@@ -83,7 +83,7 @@ def instructor_dashboard(request, course_id):
forum_admin_access = has_forum_access(request.user, course_id, FORUM_ROLE_ADMINISTRATOR)
msg = ''
- to = None
+ to_option = None
subject = None
html_message = ''
problems = []
@@ -701,13 +701,13 @@ def instructor_dashboard(request, course_id):
# email
elif action == 'Send email':
- to = request.POST.get("to")
+ to_option = request.POST.get("to_option")
subject = request.POST.get("subject")
html_message = request.POST.get("message")
email = CourseEmail(course_id=course_id,
sender=request.user,
- to=to,
+ to=to_option,
subject=subject,
html_message=html_message,
hash=md5((html_message + subject + datetime.datetime.isoformat(datetime.datetime.now())).encode('utf-8')).hexdigest())
@@ -716,7 +716,7 @@ def instructor_dashboard(request, course_id):
course_url = request.build_absolute_uri(reverse('course_root', kwargs={'course_id': course_id}))
tasks.delegate_email_batches.delay(email.hash, email.to, course_id, course_url, request.user.id)
- if to == "all":
+ 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>"
else:
@@ -810,9 +810,9 @@ s (~10k), it may take 1-2 hours to send all emails.</font>"
'course_stats': course_stats,
'msg': msg,
'modeflag': {idash_mode: 'selectedmode'},
- 'to': to, # email
- 'subject': subject, # email
- 'editor': editor, # email
+ 'to_option': to_option, # email
+ 'subject': subject, # email
+ 'editor': editor, # email
'problems': problems, # psychometrics
'plots': plots, # psychometrics
'course_errors': modulestore().get_item_errors(course.location),
diff --git a/lms/static/sass/base/_variables.scss b/lms/static/sass/base/_variables.scss
index 29fab36f1c3..ef9ac2a5b30 100644
--- a/lms/static/sass/base/_variables.scss
+++ b/lms/static/sass/base/_variables.scss
@@ -44,6 +44,11 @@ $dark-gray: #333;
// used by descriptor css
$lightGrey: #edf1f5;
$darkGrey: #8891a1;
+$blue-d1: shade($blue,20%);
+$blue-d2: shade($blue,40%);
+$blue-d4: shade($blue,80%);
+$shadow: rgba($black, 0.2);
+$shadow-l1: rgba($black, 0.1);
// edx.org marketing site variables
$m-gray: #8A8C8F;
diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html
index 35e15076fe5..e2435122292 100644
--- a/lms/templates/courseware/instructor_dashboard.html
+++ b/lms/templates/courseware/instructor_dashboard.html
@@ -445,14 +445,14 @@ function goto( mode)
%if modeflag.get('Email'):
<p>
<label for="id_to">Send to:</label>
- <select id="id_to" name="to">
+ <select id="id_to" name="to_option">
<option value="myself">Myself</option>
- %if to == "staff":
+ %if to_option == "staff":
<option value="staff" selected="selected">Staff and instructors</option>
%else:
<option value="staff">Staff and instructors</option>
%endif
- %if to == "all":
+ %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>
diff --git a/lms/templates/emails/email_footer.html b/lms/templates/emails/email_footer.html
index 577512a0153..ad9813c601f 100644
--- a/lms/templates/emails/email_footer.html
+++ b/lms/templates/emails/email_footer.html
@@ -1,6 +1,6 @@
<%! from django.core.urlresolvers import reverse %>
<br />
----<br />
-This email was automatically sent from ${settings.PLATFORM_NAME} to ${name}. <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://${site}${reverse('dashboard')}">here</a>. <br />
+To stop receiving email like this, update your course email settings <a href="https://${settings.SITE_NAME}${reverse('dashboard')}">here</a>. <br />
diff --git a/lms/templates/emails/email_footer.txt b/lms/templates/emails/email_footer.txt
index 3d530010ce0..95dffc218e9 100644
--- a/lms/templates/emails/email_footer.txt
+++ b/lms/templates/emails/email_footer.txt
@@ -1,7 +1,7 @@
<%! from django.core.urlresolvers import reverse %>
----
-This email was automatically sent from ${settings.PLATFORM_NAME} to ${name}.
+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://${site}${reverse('dashboard')}.
+To stop receiving email like this, update your account settings at https://${settings.SITE_NAME}${reverse('dashboard')}.
--
GitLab