diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py
index e6d328740e2868e8f5b07c1ae734070a9b89e18e..ebd067942ef1da196d9c1e9ddf34c3b5375d48c8 100644
--- a/common/djangoapps/student/views.py
+++ b/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:
diff --git a/lms/djangoapps/bulk_email/migrations/0001_initial.py b/lms/djangoapps/bulk_email/migrations/0001_initial.py
index 02de3b909ccb5b34e169dbcc856b960c5e524d62..99c99d4efce4295fb97a158412e730e7b5fa85c8 100644
--- a/lms/djangoapps/bulk_email/migrations/0001_initial.py
+++ b/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']
\ No newline at end of file
+ complete_apps = ['bulk_email']
diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py
index 96753a44a71cc2071a8f6db7170b5acfa9c2f831..1eed87791ee270751b72e9ee515d3494dd55275a 100644
--- a/lms/djangoapps/bulk_email/models.py
+++ b/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')
diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py
index 3da03bb3dc62471dde43c4c4557e0c40c710283e..4facbc8655db96c53af1c1a143dd2f23bb429c8e 100644
--- a/lms/djangoapps/bulk_email/tasks.py
+++ b/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_msg = EmailMultiAlternatives(subject, plaintext + plain_footer.encode('utf-8'), from_addr, [email], connection=connection)
+ email_context['name'] = name
+ email_context['email'] = email
+
+ 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
diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py
index ece0e879c744b57014cac6ff19ba4ae72dd498b2..297e5806f7c4b6e8387fe519c34e307d2eb74a87 100644
--- a/lms/djangoapps/bulk_email/tests/test_course_optout.py
+++ b/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)
diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py
index bc626d2d3bda8e904d41ff1241695fbdbec2635f..57918936cfcb5887356c0c459b9f9e4ef00151cc 100644
--- a/lms/djangoapps/bulk_email/tests/test_email.py
+++ b/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)
diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py
index fd900cffc333fc869ccaa17819079b666f5276fc..9a5555b749d5150619d133c8d3228e06594520d9 100644
--- a/lms/djangoapps/bulk_email/tests/test_err_handling.py
+++ b/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']
diff --git a/lms/djangoapps/bulk_email/tests/tests.py b/lms/djangoapps/bulk_email/tests/tests.py
index 71404a1c35049d8a05f11007d92a75a7c18d58aa..1d1d7bec19bc21d661c51255f9a3a1db08167134 100644
--- a/lms/djangoapps/bulk_email/tests/tests.py
+++ b/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)
diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py
index a6cc2cf40bcaf660a51c04f5577b16ec5cbf682f..5f3e9a6e01fac7b4b535a014ae0fa91e6e22580e 100644
--- a/lms/djangoapps/instructor/views/legacy.py
+++ b/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
diff --git a/lms/static/sass/course/instructor/_instructor.scss b/lms/static/sass/course/instructor/_instructor.scss
index f5f75e2be9fbe3c2dfcb02ba51585733080132ad..eee4afffc6a7714cedfe1996743d6a7312d16ac7 100644
--- a/lms/static/sass/course/instructor/_instructor.scss
+++ b/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;
+ }
+ }
+ }
}
diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html
index e24351222924b3d76519b9ae78582e1b6eee4675..ec5abe69063889d22a15504115c56cbd585212bb 100644
--- a/lms/templates/courseware/instructor_dashboard.html
+++ b/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>
diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html
index 3d0d5e52ee7b61502080d60720a9664e31292cf2..96c1741e69f15249bbb295d81ad31cc34a8e1af1 100644
--- a/lms/templates/dashboard.html
+++ b/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>