diff --git a/AUTHORS b/AUTHORS index 94963e4630..2f4d7efead 100644 --- a/AUTHORS +++ b/AUTHORS @@ -89,3 +89,4 @@ Akshay Jagadeesh Nick Parlante Marko Seric Felipe Montoya +Julia Hansbrough diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 23814b0454..ff74b9873b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,10 +5,14 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. - LMS: Fix issue with CourseMode expiration dates -LMS: Add PaidCourseRegistration mode, where payment is required before course registration. +LMS: Ported bulk emailing to the beta instructor dashboard. + +LMS: Add monitoring of bulk email subtasks to display progress on instructor dash. + +LMS: Add PaidCourseRegistration mode, where payment is required before course +registration. LMS: Add split testing functionality for internal use. diff --git a/common/djangoapps/terrain/course_helpers.py b/common/djangoapps/terrain/course_helpers.py index 22222d30a4..26a60ef994 100644 --- a/common/djangoapps/terrain/course_helpers.py +++ b/common/djangoapps/terrain/course_helpers.py @@ -2,7 +2,7 @@ # pylint: disable=W0621 from lettuce import world -from django.contrib.auth.models import User +from django.contrib.auth.models import User, Group from student.models import CourseEnrollment from xmodule.modulestore.django import editable_modulestore from xmodule.contentstore.django import contentstore @@ -41,15 +41,30 @@ def log_in(username='robot', password='test', email='robot@edx.org', name='Robot @world.absorb -def register_by_course_id(course_id, is_staff=False): - create_user('robot', 'password') - u = User.objects.get(username='robot') +def register_by_course_id(course_id, username='robot', password='test', is_staff=False): + create_user(username, password) + user = User.objects.get(username=username) if is_staff: u.is_staff = True u.save() CourseEnrollment.enroll(u, course_id) +@world.absorb +def add_to_course_staff(username, course_num): + """ + Add the user with `username` to the course staff group + for `course_num`. + """ + # Based on code in lms/djangoapps/courseware/access.py + group_name = "instructor_{}".format(course_num) + group, _ = Group.objects.get_or_create(name=group_name) + group.save() + + user = User.objects.get(username=username) + user.groups.add(group) + + @world.absorb def clear_courses(): # Flush and initialize the module store diff --git a/common/djangoapps/terrain/ui_helpers.py b/common/djangoapps/terrain/ui_helpers.py index e400ca9b95..52ab7462a5 100644 --- a/common/djangoapps/terrain/ui_helpers.py +++ b/common/djangoapps/terrain/ui_helpers.py @@ -45,8 +45,46 @@ def is_css_not_present(css_selector, wait_time=5): world.browser.driver.implicitly_wait(world.IMPLICIT_WAIT) @world.absorb -def css_has_text(css_selector, text, index=0): - return world.css_text(css_selector, index=index) == text +def css_has_text(css_selector, text, index=0, strip=False): + """ + Return a boolean indicating whether the element with `css_selector` + has `text`. + + If `strip` is True, strip whitespace at beginning/end of both + strings before comparing. + + If there are multiple elements matching the css selector, + use `index` to indicate which one. + """ + # If we're expecting a non-empty string, give the page + # a chance to fill in text fields. + if text: + world.wait_for(lambda _: world.css_text(css_selector, index=index)) + + actual_text = world.css_text(css_selector, index=index) + + if strip: + actual_text = actual_text.strip() + text = text.strip() + + return actual_text == text + + +@world.absorb +def css_has_value(css_selector, value, index=0): + """ + Return a boolean indicating whether the element with + `css_selector` has the specified `value`. + + If there are multiple elements matching the css selector, + use `index` to indicate which one. + """ + # If we're expecting a non-empty string, give the page + # a chance to fill in values + if value: + world.wait_for(lambda _: world.css_value(css_selector, index=index)) + + return world.css_value(css_selector, index=index) == value @world.absorb diff --git a/lms/djangoapps/bulk_email/admin.py b/lms/djangoapps/bulk_email/admin.py index 3550e1eb42..f4529a9e70 100644 --- a/lms/djangoapps/bulk_email/admin.py +++ b/lms/djangoapps/bulk_email/admin.py @@ -3,8 +3,8 @@ Django admin page for bulk email models """ from django.contrib import admin -from bulk_email.models import CourseEmail, Optout, CourseEmailTemplate -from bulk_email.forms import CourseEmailTemplateForm +from bulk_email.models import CourseEmail, Optout, CourseEmailTemplate, CourseAuthorization +from bulk_email.forms import CourseEmailTemplateForm, CourseAuthorizationAdminForm class CourseEmailAdmin(admin.ModelAdmin): @@ -57,6 +57,23 @@ unsupported tags will cause email sending to fail. return False +class CourseAuthorizationAdmin(admin.ModelAdmin): + """Admin for enabling email on a course-by-course basis.""" + form = CourseAuthorizationAdminForm + fieldsets = ( + (None, { + 'fields': ('course_id', 'email_enabled'), + 'description': ''' +Enter a course id in the following form: Org/Course/CourseRun, eg MITx/6.002x/2012_Fall +Do not enter leading or trailing slashes. There is no need to surround the course ID with quotes. +Validation will be performed on the course name, and if it is invalid, an error message will display. + +To enable email for the course, check the "Email enabled" box, then click "Save". +''' + }), + ) + admin.site.register(CourseEmail, CourseEmailAdmin) admin.site.register(Optout, OptoutAdmin) admin.site.register(CourseEmailTemplate, CourseEmailTemplateAdmin) +admin.site.register(CourseAuthorization, CourseAuthorizationAdmin) diff --git a/lms/djangoapps/bulk_email/forms.py b/lms/djangoapps/bulk_email/forms.py index fc75d4e4b6..bec05fba52 100644 --- a/lms/djangoapps/bulk_email/forms.py +++ b/lms/djangoapps/bulk_email/forms.py @@ -6,12 +6,16 @@ import logging from django import forms from django.core.exceptions import ValidationError -from bulk_email.models import CourseEmailTemplate, COURSE_EMAIL_MESSAGE_BODY_TAG +from bulk_email.models import CourseEmailTemplate, COURSE_EMAIL_MESSAGE_BODY_TAG, CourseAuthorization + +from courseware.courses import get_course_by_id +from xmodule.modulestore import MONGO_MODULESTORE_TYPE +from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) -class CourseEmailTemplateForm(forms.ModelForm): +class CourseEmailTemplateForm(forms.ModelForm): # pylint: disable=R0924 """Form providing validation of CourseEmail templates.""" class Meta: # pylint: disable=C0111 @@ -43,3 +47,32 @@ class CourseEmailTemplateForm(forms.ModelForm): template = self.cleaned_data["plain_template"] self._validate_template(template) return template + + +class CourseAuthorizationAdminForm(forms.ModelForm): # pylint: disable=R0924 + """Input form for email enabling, allowing us to verify data.""" + + class Meta: # pylint: disable=C0111 + model = CourseAuthorization + + def clean_course_id(self): + """Validate the course id""" + course_id = self.cleaned_data["course_id"] + try: + # Just try to get the course descriptor. + # If we can do that, it's a real course. + get_course_by_id(course_id, depth=1) + except Exception as exc: + msg = 'Error encountered ({0})'.format(str(exc).capitalize()) + msg += ' --- Entered course id was: "{0}". '.format(course_id) + msg += 'Please recheck that you have supplied a course id in the format: ORG/COURSE/RUN' + raise forms.ValidationError(msg) + + # Now, try and discern if it is a Studio course - HTML editor doesn't work with XML courses + is_studio_course = modulestore().get_modulestore_type(course_id) == MONGO_MODULESTORE_TYPE + if not is_studio_course: + msg = "Course Email feature is only available for courses authored in Studio. " + msg += '"{0}" appears to be an XML backed course.'.format(course_id) + raise forms.ValidationError(msg) + + return course_id diff --git a/lms/djangoapps/bulk_email/migrations/0008_add_course_authorizations.py b/lms/djangoapps/bulk_email/migrations/0008_add_course_authorizations.py new file mode 100644 index 0000000000..b200ea2bde --- /dev/null +++ b/lms/djangoapps/bulk_email/migrations/0008_add_course_authorizations.py @@ -0,0 +1,95 @@ +# -*- coding: utf-8 -*- +import datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'CourseAuthorization' + db.create_table('bulk_email_courseauthorization', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('course_id', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)), + ('email_enabled', self.gf('django.db.models.fields.BooleanField')(default=False)), + )) + db.send_create_signal('bulk_email', ['CourseAuthorization']) + + + def backwards(self, orm): + # Deleting model 'CourseAuthorization' + db.delete_table('bulk_email_courseauthorization') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'bulk_email.courseauthorization': { + 'Meta': {'object_name': 'CourseAuthorization'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'email_enabled': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'bulk_email.courseemail': { + 'Meta': {'object_name': 'CourseEmail'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'html_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'}), + 'sender': ('django.db.models.fields.related.ForeignKey', [], {'default': '1', 'to': "orm['auth.User']", 'null': 'True', 'blank': 'True'}), + 'slug': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'subject': ('django.db.models.fields.CharField', [], {'max_length': '128', 'blank': 'True'}), + 'text_message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'to_option': ('django.db.models.fields.CharField', [], {'default': "'myself'", 'max_length': '64'}) + }, + 'bulk_email.courseemailtemplate': { + 'Meta': {'object_name': 'CourseEmailTemplate'}, + 'html_template': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'plain_template': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}) + }, + 'bulk_email.optout': { + 'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'Optout'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['bulk_email'] \ No newline at end of file diff --git a/lms/djangoapps/bulk_email/models.py b/lms/djangoapps/bulk_email/models.py index 9d32dbd70c..83a691f18e 100644 --- a/lms/djangoapps/bulk_email/models.py +++ b/lms/djangoapps/bulk_email/models.py @@ -12,11 +12,21 @@ file and check it in at the same time as your model changes. To do that, """ import logging -from django.db import models +from django.db import models, transaction from django.contrib.auth.models import User +from html_to_text import html_to_text + +from django.conf import settings log = logging.getLogger(__name__) +# Bulk email to_options - the send to options that users can +# select from when they send email. +SEND_TO_MYSELF = 'myself' +SEND_TO_STAFF = 'staff' +SEND_TO_ALL = 'all' +TO_OPTIONS = [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_ALL] + class Email(models.Model): """ @@ -33,12 +43,8 @@ class Email(models.Model): class Meta: # pylint: disable=C0111 abstract = True -SEND_TO_MYSELF = 'myself' -SEND_TO_STAFF = 'staff' -SEND_TO_ALL = 'all' - -class CourseEmail(Email, models.Model): +class CourseEmail(Email): """ Stores information for an email to a course. """ @@ -51,17 +57,66 @@ class CourseEmail(Email, models.Model): # * All: This sends an email to anyone enrolled in the course, with any role # (student, staff, or instructor) # - TO_OPTIONS = ( + TO_OPTION_CHOICES = ( (SEND_TO_MYSELF, 'Myself'), (SEND_TO_STAFF, 'Staff and instructors'), (SEND_TO_ALL, 'All') ) course_id = models.CharField(max_length=255, db_index=True) - to_option = models.CharField(max_length=64, choices=TO_OPTIONS, default=SEND_TO_MYSELF) + to_option = models.CharField(max_length=64, choices=TO_OPTION_CHOICES, default=SEND_TO_MYSELF) def __unicode__(self): return self.subject + @classmethod + def create(cls, course_id, sender, to_option, subject, html_message, text_message=None): + """ + Create an instance of CourseEmail. + + The CourseEmail.save_now method makes sure the CourseEmail entry is committed. + When called from any view that is wrapped by TransactionMiddleware, + and thus in a "commit-on-success" transaction, an autocommit buried within here + will cause any pending transaction to be committed by a successful + save here. Any future database operations will take place in a + separate transaction. + """ + # automatically generate the stripped version of the text from the HTML markup: + if text_message is None: + text_message = html_to_text(html_message) + + # perform some validation here: + if to_option not in TO_OPTIONS: + fmt = 'Course email being sent to unrecognized to_option: "{to_option}" for "{course}", subject "{subject}"' + msg = fmt.format(to_option=to_option, course=course_id, subject=subject) + raise ValueError(msg) + + # create the task, then save it immediately: + course_email = cls( + course_id=course_id, + sender=sender, + to_option=to_option, + subject=subject, + html_message=html_message, + text_message=text_message, + ) + course_email.save_now() + + return course_email + + @transaction.autocommit + def save_now(self): + """ + Writes CourseEmail immediately, ensuring the transaction is committed. + + Autocommit annotation makes sure the database entry is committed. + When called from any view that is wrapped by TransactionMiddleware, + and thus in a "commit-on-success" transaction, this autocommit here + will cause any pending transaction to be committed by a successful + save here. Any future database operations will take place in a + separate transaction. + """ + self.save() + class Optout(models.Model): """ @@ -101,7 +156,11 @@ class CourseEmailTemplate(models.Model): If one isn't stored, an exception is thrown. """ - return CourseEmailTemplate.objects.get() + try: + return CourseEmailTemplate.objects.get() + except CourseEmailTemplate.DoesNotExist: + log.exception("Attempting to fetch a non-existent course email template") + raise @staticmethod def _render(format_string, message_body, context): @@ -153,3 +212,38 @@ class CourseEmailTemplate(models.Model): stored HTML template and the provided `context` dict. """ return CourseEmailTemplate._render(self.html_template, htmltext, context) + + +class CourseAuthorization(models.Model): + """ + Enable the course email feature on a course-by-course basis. + """ + # The course that these features are attached to. + course_id = models.CharField(max_length=255, db_index=True) + + # Whether or not to enable instructor email + email_enabled = models.BooleanField(default=False) + + @classmethod + def instructor_email_enabled(cls, course_id): + """ + Returns whether or not email is enabled for the given course id. + + If email has not been explicitly enabled, returns False. + """ + # If settings.MITX_FEATURES['REQUIRE_COURSE_EMAIL_AUTH'] is + # set to False, then we enable email for every course. + if not settings.MITX_FEATURES['REQUIRE_COURSE_EMAIL_AUTH']: + return True + + try: + record = cls.objects.get(course_id=course_id) + return record.email_enabled + except cls.DoesNotExist: + return False + + def __unicode__(self): + not_en = "Not " + if self.email_enabled: + not_en = "" + return u"Course '{}': Instructor Email {}Enabled".format(self.course_id, not_en) diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 1f15a8b4d7..a2f376cdcc 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -4,174 +4,500 @@ to a course. """ import math import re -import time +import random +import json +from uuid import uuid4 +from time import sleep from dogapi import dog_stats_api -from smtplib import SMTPServerDisconnected, SMTPDataError, SMTPConnectError +from smtplib import SMTPServerDisconnected, SMTPDataError, SMTPConnectError, SMTPException +from boto.ses.exceptions import ( + SESDailyQuotaExceededError, + SESMaxSendingRateExceededError, + SESAddressBlacklistedError, + SESIllegalAddressError, + SESLocalAddressCharacterError, +) +from boto.exception import AWSConnectionError + +from celery import task, current_task, group +from celery.utils.log import get_task_logger +from celery.states import SUCCESS, FAILURE, RETRY +from celery.exceptions import RetryTaskError from django.conf import settings from django.contrib.auth.models import User, Group from django.core.mail import EmailMultiAlternatives, get_connection -from django.http import Http404 -from celery import task, current_task -from celery.utils.log import get_task_logger from django.core.urlresolvers import reverse from bulk_email.models import ( CourseEmail, Optout, CourseEmailTemplate, - SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_ALL, + SEND_TO_MYSELF, SEND_TO_ALL, TO_OPTIONS, ) from courseware.access import _course_staff_group_name, _course_instructor_group_name -from courseware.courses import get_course_by_id, course_image_url +from courseware.courses import get_course, course_image_url +from instructor_task.models import InstructorTask +from instructor_task.subtasks import ( + update_subtask_status, + create_subtask_status, + increment_subtask_status, + initialize_subtask_info, + check_subtask_is_valid, +) log = get_task_logger(__name__) -@task(default_retry_delay=10, max_retries=5) # pylint: disable=E1102 -def delegate_email_batches(email_id, user_id): +# Errors that an individual email is failing to be sent, and should just +# be treated as a fail. +SINGLE_EMAIL_FAILURE_ERRORS = (SESAddressBlacklistedError, SESIllegalAddressError, SESLocalAddressCharacterError) + +# Exceptions that, if caught, should cause the task to be re-tried. +# These errors will be caught a limited number of times before the task fails. +LIMITED_RETRY_ERRORS = (SMTPConnectError, SMTPServerDisconnected, AWSConnectionError) + +# Errors that indicate that a mailing task should be retried without limit. +# An example is if email is being sent too quickly, but may succeed if sent +# more slowly. When caught by a task, it triggers an exponential backoff and retry. +# Retries happen continuously until the email is sent. +# Note that the SMTPDataErrors here are only those within the 4xx range. +# Those not in this range (i.e. in the 5xx range) are treated as hard failures +# and thus like SINGLE_EMAIL_FAILURE_ERRORS. +INFINITE_RETRY_ERRORS = (SESMaxSendingRateExceededError, SMTPDataError) + +# Errors that are known to indicate an inability to send any more emails, +# and should therefore not be retried. For example, exceeding a quota for emails. +# Also, any SMTP errors that are not explicitly enumerated above. +BULK_EMAIL_FAILURE_ERRORS = (SESDailyQuotaExceededError, SMTPException) + + +def _get_recipient_queryset(user_id, to_option, course_id, course_location): """ - 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. + Returns a query set of email recipients corresponding to the requested to_option category. - Returns the number of batches (workers) kicked off. + `to_option` is either SEND_TO_MYSELF, SEND_TO_STAFF, or SEND_TO_ALL. + + Recipients who are in more than one category (e.g. enrolled in the course and are staff or self) + will be properly deduped. """ - try: - 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. - # We might possibly want to move the blocking into the view function rather than have it in this task. - log.warning("Failed to get CourseEmail with id %s, retry %d", email_id, current_task.request.retries) - raise delegate_email_batches.retry(arg=[email_id, user_id], exc=exc) + if to_option not in TO_OPTIONS: + log.error("Unexpected bulk email TO_OPTION found: %s", to_option) + raise Exception("Unexpected bulk email TO_OPTION found: {0}".format(to_option)) - to_option = email_obj.to_option - course_id = email_obj.course_id + if to_option == SEND_TO_MYSELF: + recipient_qset = User.objects.filter(id=user_id) + else: + staff_grpname = _course_staff_group_name(course_location) + staff_group, _ = Group.objects.get_or_create(name=staff_grpname) + staff_qset = staff_group.user_set.all() + instructor_grpname = _course_instructor_group_name(course_location) + instructor_group, _ = Group.objects.get_or_create(name=instructor_grpname) + instructor_qset = instructor_group.user_set.all() + recipient_qset = staff_qset | instructor_qset + if to_option == SEND_TO_ALL: + # We also require students to have activated their accounts to + # provide verification that the provided email address is valid. + enrollment_qset = User.objects.filter( + is_active=True, + courseenrollment__course_id=course_id, + courseenrollment__is_active=True + ) + recipient_qset = recipient_qset | enrollment_qset + recipient_qset = recipient_qset.distinct() - try: - course = get_course_by_id(course_id, depth=1) - except Http404 as exc: - log.exception("get_course_by_id failed: %s", exc.args[0]) - raise Exception("get_course_by_id failed: " + exc.args[0]) + recipient_qset = recipient_qset.order_by('pk') + return recipient_qset + +def _get_course_email_context(course): + """ + Returns context arguments to apply to all emails, independent of recipient. + """ + course_id = course.id + course_title = course.display_name course_url = 'https://{}{}'.format( settings.SITE_NAME, reverse('course_root', kwargs={'course_id': course_id}) ) image_url = 'https://{}{}'.format(settings.SITE_NAME, course_image_url(course)) + email_context = { + 'course_title': course_title, + 'course_url': course_url, + 'course_image_url': image_url, + 'account_settings_url': 'https://{}{}'.format(settings.SITE_NAME, reverse('dashboard')), + 'platform_name': settings.PLATFORM_NAME, + } + return email_context - if to_option == SEND_TO_MYSELF: - recipient_qset = User.objects.filter(id=user_id) - elif to_option == SEND_TO_ALL or to_option == SEND_TO_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.all() - instructor_grpname = _course_instructor_group_name(course.location) - instructor_group, _ = Group.objects.get_or_create(name=instructor_grpname) - instructor_qset = instructor_group.user_set.all() - recipient_qset = staff_qset | instructor_qset - if to_option == SEND_TO_ALL: - enrollment_qset = User.objects.filter(courseenrollment__course_id=course_id, - courseenrollment__is_active=True) - 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)) +def _generate_subtasks(create_subtask_fcn, recipient_qset): + """ + Generates a list of subtasks to send email to a given set of recipients. - recipient_qset = recipient_qset.order_by('pk') + Arguments: + `create_subtask_fcn` : a function whose inputs are a list of recipients and a subtask_id + to assign to the new subtask. Returns the subtask that will send email to that + list of recipients. + `recipient_qset` : a query set that defines the recipients who should receive emails. + + Returns: a tuple, containing: + + * A list of subtasks that will send emails to all recipients. + * A list of subtask_ids corresponding to those subtasks. + * A count of the total number of emails being sent. + + """ total_num_emails = recipient_qset.count() - num_queries = int(math.ceil(float(total_num_emails) / float(settings.EMAILS_PER_QUERY))) + num_queries = int(math.ceil(float(total_num_emails) / float(settings.BULK_EMAIL_EMAILS_PER_QUERY))) last_pk = recipient_qset[0].pk - 1 - num_workers = 0 + num_emails_queued = 0 + task_list = [] + subtask_id_list = [] for _ in range(num_queries): - recipient_sublist = list(recipient_qset.order_by('pk').filter(pk__gt=last_pk) - .values('profile__name', 'email', 'pk')[:settings.EMAILS_PER_QUERY]) + recipient_sublist = list(recipient_qset.order_by('pk').filter(pk__gt=last_pk).values('profile__name', 'email', 'pk')[:settings.BULK_EMAIL_EMAILS_PER_QUERY]) last_pk = recipient_sublist[-1]['pk'] num_emails_this_query = len(recipient_sublist) - num_tasks_this_query = int(math.ceil(float(num_emails_this_query) / float(settings.EMAILS_PER_TASK))) + num_tasks_this_query = int(math.ceil(float(num_emails_this_query) / float(settings.BULK_EMAIL_EMAILS_PER_TASK))) chunk = int(math.ceil(float(num_emails_this_query) / float(num_tasks_this_query))) for i in range(num_tasks_this_query): to_list = recipient_sublist[i * chunk:i * chunk + chunk] - course_email.delay( - email_id, - to_list, - course.display_name, - course_url, - image_url, - False - ) - num_workers += num_tasks_this_query - return num_workers + subtask_id = str(uuid4()) + subtask_id_list.append(subtask_id) + new_subtask = create_subtask_fcn(to_list, subtask_id) + task_list.append(new_subtask) + + num_emails_queued += num_emails_this_query + + # Sanity check: we expect the chunking to be properly summing to the original count: + if num_emails_queued != total_num_emails: + error_msg = "Task {}: number of emails generated by chunking {} not equal to original total {}".format(num_emails_queued, total_num_emails) + log.error(error_msg) + raise ValueError(error_msg) + + return task_list, subtask_id_list, total_num_emails -@task(default_retry_delay=15, max_retries=5) # pylint: disable=E1102 -def course_email(email_id, to_list, course_title, course_url, image_url, throttle=False): +def perform_delegate_email_batches(entry_id, course_id, task_input, action_name): """ - Takes a primary id for a CourseEmail object and a 'to_list' of recipient objects--keys are - 'profile__name', 'email' (address), and 'pk' (in the user table). - course_title, course_url, and image_url are to memoize course properties and save lookups. + Delegates emails by querying for the list of recipients who should + get the mail, chopping up into batches of settings.BULK_EMAIL_EMAILS_PER_TASK size, + and queueing up worker jobs. - Sends to all addresses contained in to_list. Emails are sent multi-part, in both plain - text and html. + Returns the number of batches (workers) kicked off. """ - with dog_stats_api.timer('course_email.single_task.time.overall', tags=[_statsd_tag(course_title)]): - _send_course_email(email_id, to_list, course_title, course_url, image_url, throttle) + entry = InstructorTask.objects.get(pk=entry_id) + # Get inputs to use in this task from the entry. + user_id = entry.requester.id + task_id = entry.task_id -def _send_course_email(email_id, to_list, course_title, course_url, image_url, throttle): - """ - Performs the email sending task. - """ + # Perfunctory check, since expansion is made for convenience of other task + # code that doesn't need the entry_id. + if course_id != entry.course_id: + format_msg = "Course id conflict: explicit value {} does not match task value {}" + raise ValueError(format_msg.format(course_id, entry.course_id)) + + # Fetch the CourseEmail. + email_id = task_input['email_id'] try: - msg = CourseEmail.objects.get(id=email_id) + email_obj = CourseEmail.objects.get(id=email_id) except CourseEmail.DoesNotExist: - log.exception("Could not find email id:{} to send.".format(email_id)) + # The CourseEmail object should be committed in the view function before the task + # is submitted and reaches this point. + log.warning("Task %s: Failed to get CourseEmail with id %s", task_id, email_id) raise - # exclude optouts - optouts = (Optout.objects.filter(course_id=msg.course_id, - user__in=[i['pk'] for i in to_list]) - .values_list('user__email', flat=True)) + # Check to see if email batches have already been defined. This seems to + # happen sometimes when there is a loss of connection while a task is being + # queued. When this happens, the same task gets called again, and a whole + # new raft of subtasks gets queued up. We will assume that if subtasks + # have already been defined, there is no need to redefine them below. + # So we just return right away. We don't raise an exception, because we want + # the current task to be marked with whatever it had been marked with before. + if len(entry.subtasks) > 0 and len(entry.task_output) > 0: + log.warning("Task %s has already been processed for email %s! InstructorTask = %s", task_id, email_id, entry) + progress = json.loads(entry.task_output) + return progress + # Sanity check that course for email_obj matches that of the task referencing it. + if course_id != email_obj.course_id: + format_msg = "Course id conflict: explicit value {} does not match email value {}" + raise ValueError(format_msg.format(course_id, email_obj.course_id)) + + # Fetch the course object. + try: + course = get_course(course_id) + except ValueError: + log.exception("Task %s: course not found: %s", task_id, course_id) + raise + + to_option = email_obj.to_option + recipient_qset = _get_recipient_queryset(user_id, to_option, course_id, course.location) + global_email_context = _get_course_email_context(course) + + def _create_send_email_subtask(to_list, subtask_id): + """Creates a subtask to send email to a given recipient list.""" + subtask_status = create_subtask_status(subtask_id) + new_subtask = send_course_email.subtask( + ( + entry_id, + email_id, + to_list, + global_email_context, + subtask_status, + ), + task_id=subtask_id, + routing_key=settings.BULK_EMAIL_ROUTING_KEY, + ) + return new_subtask + + log.info("Task %s: Preparing to generate subtasks for course %s, email %s, to_option %s", + task_id, course_id, email_id, to_option) + task_list, subtask_id_list, total_num_emails = _generate_subtasks(_create_send_email_subtask, recipient_qset) + + # Update the InstructorTask with information about the subtasks we've defined. + log.info("Task %s: Preparing to update task for sending %d emails for course %s, email %s, to_option %s", + task_id, total_num_emails, course_id, email_id, to_option) + progress = initialize_subtask_info(entry, action_name, total_num_emails, subtask_id_list) + num_subtasks = len(subtask_id_list) + + # Now group the subtasks, and start them running. This allows all the subtasks + # in the list to be submitted at the same time. + log.info("Task %s: Preparing to queue %d email tasks (%d emails) for course %s, email %s, to %s", + task_id, num_subtasks, total_num_emails, course_id, email_id, to_option) + task_group = group(task_list) + task_group.apply_async(routing_key=settings.BULK_EMAIL_ROUTING_KEY) + + # We want to return progress here, as this is what will be stored in the + # AsyncResult for the parent task as its return value. + # The AsyncResult will then be marked as SUCCEEDED, and have this return value as its "result". + # That's okay, for the InstructorTask will have the "real" status, and monitoring code + # should be using that instead. + return progress + + +@task(default_retry_delay=settings.BULK_EMAIL_DEFAULT_RETRY_DELAY, max_retries=settings.BULK_EMAIL_MAX_RETRIES) # pylint: disable=E1102 +def send_course_email(entry_id, email_id, to_list, global_email_context, subtask_status): + """ + Sends an email to a list of recipients. + + Inputs are: + * `entry_id`: id of the InstructorTask object to which progress should be recorded. + * `email_id`: id of the CourseEmail model that is to be emailed. + * `to_list`: list of recipients. Each is represented as a dict with the following keys: + - 'profile__name': full name of User. + - 'email': email address of User. + - 'pk': primary key of User model. + * `global_email_context`: dict containing values that are unique for this email but the same + for all recipients of this email. This dict is to be used to fill in slots in email + template. It does not include 'name' and 'email', which will be provided by the to_list. + * `subtask_status` : dict containing values representing current status. Keys are: + + 'task_id' : id of subtask. This is used to pass task information across retries. + 'attempted' : number of attempts -- should equal succeeded plus failed + 'succeeded' : number that succeeded in processing + 'skipped' : number that were not processed. + 'failed' : number that failed during processing + 'retried_nomax' : number of times the subtask has been retried for conditions that + should not have a maximum count applied + 'retried_withmax' : number of times the subtask has been retried for conditions that + should have a maximum count applied + 'state' : celery state of the subtask (e.g. QUEUING, PROGRESS, RETRY, FAILURE, SUCCESS) + + Most values will be zero on initial call, but may be different when the task is + invoked as part of a retry. + + Sends to all addresses contained in to_list that are not also in the Optout table. + Emails are sent multi-part, in both plain text and html. Updates InstructorTask object + with status information (sends, failures, skips) and updates number of subtasks completed. + """ + current_task_id = subtask_status['task_id'] + num_to_send = len(to_list) + log.info("Preparing to send email %s to %d recipients as subtask %s for instructor task %d: context = %s, status=%s", + email_id, num_to_send, current_task_id, entry_id, global_email_context, subtask_status) + + # Check that the requested subtask is actually known to the current InstructorTask entry. + # If this fails, it throws an exception, which should fail this subtask immediately. + # This can happen when the parent task has been run twice, and results in duplicate + # subtasks being created for the same InstructorTask entry. We hope to catch this condition + # in perform_delegate_email_batches(), but just in case we fail to do so there, + # we check here as well. + check_subtask_is_valid(entry_id, current_task_id) + + send_exception = None + new_subtask_status = None + try: + course_title = global_email_context['course_title'] + with dog_stats_api.timer('course_email.single_task.time.overall', tags=[_statsd_tag(course_title)]): + new_subtask_status, send_exception = _send_course_email( + entry_id, + email_id, + to_list, + global_email_context, + subtask_status, + ) + except Exception: + # Unexpected exception. Try to write out the failure to the entry before failing. + log.exception("Send-email task %s: failed unexpectedly!", current_task_id) + # We got here for really unexpected reasons. Since we don't know how far + # the task got in emailing, we count all recipients as having failed. + # It at least keeps the counts consistent. + new_subtask_status = increment_subtask_status(subtask_status, failed=num_to_send, state=FAILURE) + update_subtask_status(entry_id, current_task_id, new_subtask_status) + raise + + if send_exception is None: + # Update the InstructorTask object that is storing its progress. + log.info("Send-email task %s: succeeded", current_task_id) + update_subtask_status(entry_id, current_task_id, new_subtask_status) + elif isinstance(send_exception, RetryTaskError): + # If retrying, a RetryTaskError needs to be returned to Celery. + # We assume that the the progress made before the retry condition + # was encountered has already been updated before the retry call was made, + # so we only log here. + log.warning("Send-email task %s: being retried", current_task_id) + raise send_exception # pylint: disable=E0702 + else: + log.error("Send-email task %s: failed: %s", current_task_id, send_exception) + update_subtask_status(entry_id, current_task_id, new_subtask_status) + raise send_exception # pylint: disable=E0702 + + log.info("Send-email task %s: returning status %s", current_task_id, new_subtask_status) + return new_subtask_status + + +def _filter_optouts_from_recipients(to_list, course_id): + """ + Filters a recipient list based on student opt-outs for a given course. + + Returns the filtered recipient list, as well as the number of optouts + removed from the list. + """ + optouts = Optout.objects.filter( + course_id=course_id, + user__in=[i['pk'] for i in to_list] + ).values_list('user__email', flat=True) optouts = set(optouts) + # Only count the num_optout for the first time the optouts are calculated. + # We assume that the number will not change on retries, and so we don't need + # to calculate it each time. num_optout = len(optouts) - to_list = [recipient for recipient in to_list if recipient['email'] not in optouts] + return to_list, num_optout - subject = "[" + course_title + "] " + msg.subject +def _get_source_address(course_id, course_title): + """ + Calculates an email address to be used as the 'from-address' for sent emails. + + Makes a unique from name and address for each course, e.g. + + "COURSE_TITLE" Course Staff + + """ course_title_no_quotes = re.sub(r'"', '', course_title) - from_addr = '"{0}" Course Staff <{1}>'.format(course_title_no_quotes, settings.DEFAULT_BULK_FROM_EMAIL) + + # The course_id is assumed to be in the form 'org/course_num/run', + # so pull out the course_num. Then make sure that it can be used + # in an email address, by substituting a '_' anywhere a non-(ascii, period, or dash) + # character appears. + course_num = course_id.split('/')[1] + invalid_chars = re.compile(r"[^\w.-]") + course_num = invalid_chars.sub('_', course_num) + + from_addr = '"{0}" Course Staff <{1}-{2}>'.format(course_title_no_quotes, course_num, settings.BULK_EMAIL_DEFAULT_FROM_EMAIL) + return from_addr + + +def _send_course_email(entry_id, email_id, to_list, global_email_context, subtask_status): + """ + Performs the email sending task. + + Sends an email to a list of recipients. + + Inputs are: + * `entry_id`: id of the InstructorTask object to which progress should be recorded. + * `email_id`: id of the CourseEmail model that is to be emailed. + * `to_list`: list of recipients. Each is represented as a dict with the following keys: + - 'profile__name': full name of User. + - 'email': email address of User. + - 'pk': primary key of User model. + * `global_email_context`: dict containing values that are unique for this email but the same + for all recipients of this email. This dict is to be used to fill in slots in email + template. It does not include 'name' and 'email', which will be provided by the to_list. + * `subtask_status` : dict containing values representing current status. Keys are: + + 'task_id' : id of subtask. This is used to pass task information across retries. + 'attempted' : number of attempts -- should equal succeeded plus failed + 'succeeded' : number that succeeded in processing + 'skipped' : number that were not processed. + 'failed' : number that failed during processing + 'retried_nomax' : number of times the subtask has been retried for conditions that + should not have a maximum count applied + 'retried_withmax' : number of times the subtask has been retried for conditions that + should have a maximum count applied + 'state' : celery state of the subtask (e.g. QUEUING, PROGRESS, RETRY, FAILURE, SUCCESS) + + Sends to all addresses contained in to_list that are not also in the Optout table. + Emails are sent multi-part, in both plain text and html. + + Returns a tuple of two values: + * First value is a dict which represents current progress at the end of this call. Keys are + the same as for the input subtask_status. + + * Second value is an exception returned by the innards of the method, indicating a fatal error. + In this case, the number of recipients that were not sent have already been added to the + 'failed' count above. + """ + # Get information from current task's request: + task_id = subtask_status['task_id'] + + # collect stats on progress: + num_optout = 0 + num_sent = 0 + num_error = 0 + + try: + course_email = CourseEmail.objects.get(id=email_id) + except CourseEmail.DoesNotExist as exc: + log.exception("Task %s: could not find email id:%s to send.", task_id, email_id) + raise + + # Exclude optouts (if not a retry): + # Note that we don't have to do the optout logic at all if this is a retry, + # because we have presumably already performed the optout logic on the first + # attempt. Anyone on the to_list on a retry has already passed the filter + # that existed at that time, and we don't need to keep checking for changes + # in the Optout list. + if (subtask_status['retried_nomax'] + subtask_status['retried_withmax']) == 0: + to_list, num_optout = _filter_optouts_from_recipients(to_list, course_email.course_id) + + course_title = global_email_context['course_title'] + subject = "[" + course_title + "] " + course_email.subject + from_addr = _get_source_address(course_email.course_id, course_title) course_email_template = CourseEmailTemplate.get_template() - try: connection = get_connection() connection.open() - num_sent = 0 - num_error = 0 # Define context values to use in all course emails: - email_context = { - 'name': '', - 'email': '', - 'course_title': course_title, - 'course_url': course_url, - 'course_image_url': image_url, - 'account_settings_url': 'https://{}{}'.format(settings.SITE_NAME, reverse('dashboard')), - 'platform_name': settings.PLATFORM_NAME, - } + email_context = {'name': '', 'email': ''} + email_context.update(global_email_context) while to_list: - # Update context with user-specific values: - email = to_list[-1]['email'] + # Update context with user-specific values from the user at the end of the list. + # At the end of processing this user, they will be popped off of the to_list. + # That way, the to_list will always contain the recipients remaining to be emailed. + # This is convenient for retries, which will need to send to those who haven't + # yet been emailed, but not send to those who have already been sent to. + current_recipient = to_list[-1] + email = current_recipient['email'] email_context['email'] = email - email_context['name'] = to_list[-1]['profile__name'] + email_context['name'] = current_recipient['profile__name'] # Construct message content using templates and context: - plaintext_msg = course_email_template.render_plaintext(msg.text_message, email_context) - html_msg = course_email_template.render_htmltext(msg.html_message, email_context) + plaintext_msg = course_email_template.render_plaintext(course_email.text_message, email_context) + html_msg = course_email_template.render_htmltext(course_email.html_message, email_context) # Create email: email_msg = EmailMultiAlternatives( @@ -183,66 +509,247 @@ def _send_course_email(email_id, to_list, course_title, course_url, image_url, t ) email_msg.attach_alternative(html_msg, 'text/html') - # Throttle if we tried a few times and got the rate limiter - if throttle or current_task.request.retries > 0: - time.sleep(0.2) + # Throttle if we have gotten the rate limiter. This is not very high-tech, + # but if a task has been retried for rate-limiting reasons, then we sleep + # for a period of time between all emails within this task. Choice of + # the value depends on the number of workers that might be sending email in + # parallel, and what the SES throttle rate is. + if subtask_status['retried_nomax'] > 0: + sleep(settings.BULK_EMAIL_RETRY_DELAY_BETWEEN_SENDS) try: + log.debug('Email with id %s to be sent to %s', email_id, email) + with dog_stats_api.timer('course_email.single_send.time.overall', tags=[_statsd_tag(course_title)]): connection.send_messages([email_msg]) - dog_stats_api.increment('course_email.sent', tags=[_statsd_tag(course_title)]) - - log.info('Email with id %s sent to %s', email_id, 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: - # 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 - log.warning('Email with id %s not delivered to %s due to error %s', email_id, email, exc.smtp_error) - + # This will fall through and not retry the message. + log.warning('Task %s: email with id %s not delivered to %s due to error %s', task_id, email_id, email, exc.smtp_error) dog_stats_api.increment('course_email.error', tags=[_statsd_tag(course_title)]) - num_error += 1 + except SINGLE_EMAIL_FAILURE_ERRORS as exc: + # This will fall through and not retry the message. + log.warning('Task %s: email with id %s not delivered to %s due to error %s', task_id, email_id, email, exc) + dog_stats_api.increment('course_email.error', tags=[_statsd_tag(course_title)]) + num_error += 1 + + else: + dog_stats_api.increment('course_email.sent', tags=[_statsd_tag(course_title)]) + if settings.BULK_EMAIL_LOG_SENT_EMAILS: + log.info('Email with id %s sent to %s', email_id, email) + else: + log.debug('Email with id %s sent to %s', email_id, email) + num_sent += 1 + + # Pop the user that was emailed off the end of the list only once they have + # successfully been processed. (That way, if there were a failure that + # needed to be retried, the user is still on the list.) to_list.pop() - connection.close() - return course_email_result(num_sent, num_error, num_optout) + except INFINITE_RETRY_ERRORS as exc: + dog_stats_api.increment('course_email.infinite_retry', tags=[_statsd_tag(course_title)]) + # Increment the "retried_nomax" counter, update other counters with progress to date, + # and set the state to RETRY: + subtask_progress = increment_subtask_status( + subtask_status, + succeeded=num_sent, + failed=num_error, + skipped=num_optout, + retried_nomax=1, + state=RETRY + ) + return _submit_for_retry( + entry_id, email_id, to_list, global_email_context, exc, subtask_progress, skip_retry_max=True + ) - 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=[ + except LIMITED_RETRY_ERRORS as exc: + # Errors caught here cause the email to be retried. The entire task is actually retried + # without popping the current recipient off of the existing list. + # Errors caught are those that indicate a temporary condition that might succeed on retry. + dog_stats_api.increment('course_email.limited_retry', tags=[_statsd_tag(course_title)]) + # Increment the "retried_withmax" counter, update other counters with progress to date, + # and set the state to RETRY: + subtask_progress = increment_subtask_status( + subtask_status, + succeeded=num_sent, + failed=num_error, + skipped=num_optout, + retried_withmax=1, + state=RETRY + ) + return _submit_for_retry( + entry_id, email_id, to_list, global_email_context, exc, subtask_progress, skip_retry_max=False + ) + + except BULK_EMAIL_FAILURE_ERRORS as exc: + dog_stats_api.increment('course_email.error', tags=[_statsd_tag(course_title)]) + num_pending = len(to_list) + log.exception('Task %s: email with id %d caused send_course_email task to fail with "fatal" exception. %d emails unsent.', + task_id, email_id, num_pending) + # Update counters with progress to date, counting unsent emails as failures, + # and set the state to FAILURE: + subtask_progress = increment_subtask_status( + subtask_status, + succeeded=num_sent, + failed=(num_error + num_pending), + skipped=num_optout, + state=FAILURE + ) + return subtask_progress, exc + + except Exception as exc: + # Errors caught here cause the email to be retried. The entire task is actually retried + # without popping the current recipient off of the existing list. + # These are unexpected errors. Since they might be due to a temporary condition that might + # succeed on retry, we give them a retry. + dog_stats_api.increment('course_email.limited_retry', tags=[_statsd_tag(course_title)]) + log.exception('Task %s: email with id %d caused send_course_email task to fail with unexpected exception. Generating retry.', + task_id, email_id) + # Increment the "retried_withmax" counter, update other counters with progress to date, + # and set the state to RETRY: + subtask_progress = increment_subtask_status( + subtask_status, + succeeded=num_sent, + failed=num_error, + skipped=num_optout, + retried_withmax=1, + state=RETRY + ) + return _submit_for_retry( + entry_id, email_id, to_list, global_email_context, exc, subtask_progress, skip_retry_max=False + ) + + else: + # All went well. Update counters with progress to date, + # and set the state to SUCCESS: + subtask_progress = increment_subtask_status( + subtask_status, + succeeded=num_sent, + failed=num_error, + skipped=num_optout, + state=SUCCESS + ) + # Successful completion is marked by an exception value of None. + return subtask_progress, None + finally: + # Clean up at the end. + connection.close() + + +def _get_current_task(): + """ + Stub to make it easier to test without actually running Celery. + + This is a wrapper around celery.current_task, which provides access + to the top of the stack of Celery's tasks. When running tests, however, + it doesn't seem to work to mock current_task directly, so this wrapper + is used to provide a hook to mock in tests, while providing the real + `current_task` in production. + """ + return current_task + + +def _submit_for_retry(entry_id, email_id, to_list, global_email_context, current_exception, subtask_status, skip_retry_max=False): + """ + Helper function to requeue a task for retry, using the new version of arguments provided. + + Inputs are the same as for running a task, plus two extra indicating the state at the time of retry. + These include the `current_exception` that the task encountered that is causing the retry attempt, + and the `subtask_status` that is to be returned. A third extra argument `skip_retry_max` + indicates whether the current retry should be subject to a maximum test. + + Returns a tuple of two values: + * First value is a dict which represents current progress. Keys are: + + 'task_id' : id of subtask. This is used to pass task information across retries. + 'attempted' : number of attempts -- should equal succeeded plus failed + 'succeeded' : number that succeeded in processing + 'skipped' : number that were not processed. + 'failed' : number that failed during processing + 'retried_nomax' : number of times the subtask has been retried for conditions that + should not have a maximum count applied + 'retried_withmax' : number of times the subtask has been retried for conditions that + should have a maximum count applied + 'state' : celery state of the subtask (e.g. QUEUING, PROGRESS, RETRY, FAILURE, SUCCESS) + + * Second value is an exception returned by the innards of the method. If the retry was + successfully submitted, this value will be the RetryTaskError that retry() returns. + Otherwise, it (ought to be) the current_exception passed in. + """ + task_id = subtask_status['task_id'] + log.info("Task %s: Successfully sent to %s users; failed to send to %s users (and skipped %s users)", + task_id, subtask_status['succeeded'], subtask_status['failed'], subtask_status['skipped']) + + # Calculate time until we retry this task (in seconds): + # The value for max_retries is increased by the number of times an "infinite-retry" exception + # has been retried. We want the regular retries to trigger max-retry checking, but not these + # special retries. So we count them separately. + max_retries = _get_current_task().max_retries + subtask_status['retried_nomax'] + base_delay = _get_current_task().default_retry_delay + if skip_retry_max: + # once we reach five retries, don't increase the countdown further. + retry_index = min(subtask_status['retried_nomax'], 5) + exception_type = 'sending-rate' + # if we have a cap, after all, apply it now: + if hasattr(settings, 'BULK_EMAIL_INFINITE_RETRY_CAP'): + retry_cap = settings.BULK_EMAIL_INFINITE_RETRY_CAP + subtask_status['retried_withmax'] + max_retries = min(max_retries, retry_cap) + else: + retry_index = subtask_status['retried_withmax'] + exception_type = 'transient' + + # Skew the new countdown value by a random factor, so that not all + # retries are deferred by the same amount. + countdown = ((2 ** retry_index) * base_delay) * random.uniform(.75, 1.25) + + log.warning('Task %s: email with id %d not delivered due to %s error %s, retrying send to %d recipients in %s seconds (with max_retry=%s)', + task_id, email_id, exception_type, current_exception, len(to_list), countdown, max_retries) + + # we make sure that we update the InstructorTask with the current subtask status + # *before* actually calling retry(), to be sure that there is no race + # condition between this update and the update made by the retried task. + update_subtask_status(entry_id, task_id, subtask_status) + + # Now attempt the retry. If it succeeds, it returns a RetryTaskError that + # needs to be returned back to Celery. If it fails, we return the existing + # exception. + try: + send_course_email.retry( + args=[ + entry_id, email_id, to_list, - course_title, - course_url, - image_url, - current_task.request.retries > 0 + global_email_context, + subtask_status, ], - exc=exc, - countdown=(2 ** current_task.request.retries) * 15 + exc=current_exception, + countdown=countdown, + max_retries=max_retries, + throw=True, ) - 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]) - # Close the connection before we exit - connection.close() - raise - - -# This string format code is wrapped in this function to allow mocking for a unit test -def course_email_result(num_sent, num_error, num_optout): - """Return the formatted result of course_email sending.""" - return "Sent {0}, Fail {1}, Optout {2}".format(num_sent, num_error, num_optout) + except RetryTaskError as retry_error: + # If the retry call is successful, update with the current progress: + log.exception('Task %s: email with id %d caused send_course_email task to retry.', + task_id, email_id) + return subtask_status, retry_error + except Exception as retry_exc: + # If there are no more retries, because the maximum has been reached, + # we expect the original exception to be raised. We catch it here + # (and put it in retry_exc just in case it's different, but it shouldn't be), + # and update status as if it were any other failure. That means that + # the recipients still in the to_list are counted as failures. + log.exception('Task %s: email with id %d caused send_course_email task to fail to retry. To list: %s', + task_id, email_id, [i['email'] for i in to_list]) + num_failed = len(to_list) + new_subtask_progress = increment_subtask_status(subtask_status, failed=num_failed, state=FAILURE) + return new_subtask_progress, retry_exc def _statsd_tag(course_title): diff --git a/lms/djangoapps/bulk_email/tests/test_course_optout.py b/lms/djangoapps/bulk_email/tests/test_course_optout.py index 0adf119527..378610eeb3 100644 --- a/lms/djangoapps/bulk_email/tests/test_course_optout.py +++ b/lms/djangoapps/bulk_email/tests/test_course_optout.py @@ -59,7 +59,7 @@ class TestOptoutCourseEmails(ModuleStoreTestCase): selected_email_link = 'Email' self.assertTrue(selected_email_link in response.content) - @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True}) + @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) def test_optout_course(self): """ Make sure student does not receive course email after opting out. @@ -88,7 +88,7 @@ class TestOptoutCourseEmails(ModuleStoreTestCase): # 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}) + @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) def test_optin_course(self): """ Make sure student receives course email after opting in. diff --git a/lms/djangoapps/bulk_email/tests/test_email.py b/lms/djangoapps/bulk_email/tests/test_email.py index dab7812763..9da787dbaa 100644 --- a/lms/djangoapps/bulk_email/tests/test_email.py +++ b/lms/djangoapps/bulk_email/tests/test_email.py @@ -2,6 +2,8 @@ """ Unit tests for sending course email """ +from mock import patch + from django.conf import settings from django.core import mail from django.core.urlresolvers import reverse @@ -12,11 +14,8 @@ 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 bulk_email.tasks import delegate_email_batches, course_email -from bulk_email.models import CourseEmail, Optout - -from mock import patch +from bulk_email.models import Optout +from instructor_task.subtasks import increment_subtask_status STAFF_COUNT = 3 STUDENT_COUNT = 10 @@ -30,13 +29,13 @@ class MockCourseEmailResult(object): """ emails_sent = 0 - def get_mock_course_email_result(self): + def get_mock_increment_subtask_status(self): """Wrapper for mock email function.""" - def mock_course_email_result(sent, failed, output, **kwargs): # pylint: disable=W0613 + def mock_increment_subtask_status(original_status, **kwargs): # pylint: disable=W0613 """Increments count of number of emails sent.""" - self.emails_sent += sent - return True - return mock_course_email_result + self.emails_sent += kwargs.get('succeeded', 0) + return increment_subtask_status(original_status, **kwargs) + return mock_increment_subtask_status @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) @@ -45,7 +44,7 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase): Test that emails send correctly. """ - @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True}) + @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': False}) def setUp(self): self.course = CourseFactory.create() self.instructor = UserFactory.create(username="instructor", email="robot+instructor@edx.org") @@ -244,14 +243,14 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase): [self.instructor.email] + [s.email for s in self.staff] + [s.email for s in self.students] ) - @override_settings(EMAILS_PER_TASK=3, EMAILS_PER_QUERY=7) - @patch('bulk_email.tasks.course_email_result') + @override_settings(BULK_EMAIL_EMAILS_PER_TASK=3, BULK_EMAIL_EMAILS_PER_QUERY=7) + @patch('bulk_email.tasks.increment_subtask_status') def test_chunked_queries_send_numerous_emails(self, email_mock): """ Test sending a large number of emails, to test the chunked querying """ mock_factory = MockCourseEmailResult() - email_mock.side_effect = mock_factory.get_mock_course_email_result() + email_mock.side_effect = mock_factory.get_mock_increment_subtask_status() added_users = [] for _ in xrange(LARGE_NUM_EMAILS): user = UserFactory() @@ -281,14 +280,3 @@ class TestEmailSendFromDashboard(ModuleStoreTestCase): [s.email for s in self.students] + [s.email for s in added_users if s not in optouts]) self.assertItemsEqual(outbox_contents, should_send_contents) - - -@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) -class TestEmailSendExceptions(ModuleStoreTestCase): - """ - Test that exceptions are handled correctly. - """ - def test_no_course_email_obj(self): - # Make sure course_email handles CourseEmail.DoesNotExist exception. - with self.assertRaises(CourseEmail.DoesNotExist): - course_email(101, [], "_", "_", "_", False) diff --git a/lms/djangoapps/bulk_email/tests/test_err_handling.py b/lms/djangoapps/bulk_email/tests/test_err_handling.py index abdbf4dc3b..a7ec72c56e 100644 --- a/lms/djangoapps/bulk_email/tests/test_err_handling.py +++ b/lms/djangoapps/bulk_email/tests/test_err_handling.py @@ -2,21 +2,24 @@ Unit tests for handling email sending errors """ from itertools import cycle +from mock import patch +from smtplib import SMTPDataError, SMTPServerDisconnected, SMTPConnectError + from django.test.utils import override_settings from django.conf import settings from django.core.management import call_command 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.models import CourseEmail -from bulk_email.tasks import delegate_email_batches - -from mock import patch, Mock -from smtplib import SMTPDataError, SMTPServerDisconnected, SMTPConnectError +from bulk_email.models import CourseEmail, SEND_TO_ALL +from bulk_email.tasks import perform_delegate_email_batches, send_course_email +from instructor_task.models import InstructorTask +from instructor_task.subtasks import create_subtask_status, initialize_subtask_info class EmailTestException(Exception): @@ -43,7 +46,7 @@ class TestEmailErrors(ModuleStoreTestCase): patch.stopall() @patch('bulk_email.tasks.get_connection', autospec=True) - @patch('bulk_email.tasks.course_email.retry') + @patch('bulk_email.tasks.send_course_email.retry') def test_data_err_retry(self, retry, get_conn): """ Test that celery handles transient SMTPDataErrors by retrying. @@ -64,15 +67,16 @@ class TestEmailErrors(ModuleStoreTestCase): self.assertTrue(type(exc) == SMTPDataError) @patch('bulk_email.tasks.get_connection', autospec=True) - @patch('bulk_email.tasks.course_email_result') - @patch('bulk_email.tasks.course_email.retry') + @patch('bulk_email.tasks.increment_subtask_status') + @patch('bulk_email.tasks.send_course_email.retry') def test_data_err_fail(self, retry, result, get_conn): """ Test that celery handles permanent SMTPDataErrors by failing and not retrying. """ + # have every fourth email fail due to blacklisting: get_conn.return_value.send_messages.side_effect = cycle([SMTPDataError(554, "Email address is blacklisted"), - None]) - students = [UserFactory() for _ in xrange(settings.EMAILS_PER_TASK)] + None, None, None]) + students = [UserFactory() for _ in xrange(settings.BULK_EMAIL_EMAILS_PER_TASK)] for student in students: CourseEnrollmentFactory.create(user=student, course_id=self.course.id) @@ -87,13 +91,14 @@ class TestEmailErrors(ModuleStoreTestCase): # 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, optouts), _) = result.call_args - self.assertEquals(optouts, 0) - self.assertEquals(fail, settings.EMAILS_PER_TASK / 2) - self.assertEquals(sent, settings.EMAILS_PER_TASK / 2) + ((_initial_results), kwargs) = result.call_args + self.assertEquals(kwargs['skipped'], 0) + expected_fails = int((settings.BULK_EMAIL_EMAILS_PER_TASK + 3) / 4.0) + self.assertEquals(kwargs['failed'], expected_fails) + self.assertEquals(kwargs['succeeded'], settings.BULK_EMAIL_EMAILS_PER_TASK - expected_fails) @patch('bulk_email.tasks.get_connection', autospec=True) - @patch('bulk_email.tasks.course_email.retry') + @patch('bulk_email.tasks.send_course_email.retry') def test_disconn_err_retry(self, retry, get_conn): """ Test that celery handles SMTPServerDisconnected by retrying. @@ -113,7 +118,7 @@ class TestEmailErrors(ModuleStoreTestCase): self.assertTrue(type(exc) == SMTPServerDisconnected) @patch('bulk_email.tasks.get_connection', autospec=True) - @patch('bulk_email.tasks.course_email.retry') + @patch('bulk_email.tasks.send_course_email.retry') def test_conn_err_retry(self, retry, get_conn): """ Test that celery handles SMTPConnectError by retrying. @@ -133,67 +138,107 @@ class TestEmailErrors(ModuleStoreTestCase): 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.increment_subtask_status') @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): + def test_nonexistent_email(self, mock_log, 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 + # create an InstructorTask object to pass through + course_id = self.course.id + entry = InstructorTask.create(course_id, "task_type", "task_key", "task_input", self.instructor) + task_input = {"email_id": -1} + with self.assertRaises(CourseEmail.DoesNotExist): + perform_delegate_email_batches(entry.id, course_id, task_input, "action_name") # pylint: disable=E1101 + ((log_str, _, email_id), _) = 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): + def test_nonexistent_course(self): """ Tests exception when the course in the email doesn't exist """ - email = CourseEmail(course_id="I/DONT/EXIST") + course_id = "I/DONT/EXIST" + email = CourseEmail(course_id=course_id) 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) + entry = InstructorTask.create(course_id, "task_type", "task_key", "task_input", self.instructor) + task_input = {"email_id": email.id} # pylint: disable=E1101 + with self.assertRaisesRegexp(ValueError, "Course not found"): + perform_delegate_email_batches(entry.id, course_id, task_input, "action_name") # pylint: disable=E1101 - @patch('bulk_email.tasks.log') - def test_nonexist_to_option(self, mock_log): + def test_nonexistent_to_option(self): """ 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) + entry = InstructorTask.create(self.course.id, "task_type", "task_key", "task_input", self.instructor) + task_input = {"email_id": email.id} # pylint: disable=E1101 + with self.assertRaisesRegexp(Exception, 'Unexpected bulk email TO_OPTION found: IDONTEXIST'): + perform_delegate_email_batches(entry.id, self.course.id, task_input, "action_name") # pylint: disable=E1101 + + def test_wrong_course_id_in_task(self): + """ + Tests exception when the course_id in task is not the same as one explicitly passed in. + """ + email = CourseEmail(course_id=self.course.id, to_option=SEND_TO_ALL) + email.save() + entry = InstructorTask.create("bogus_task_id", "task_type", "task_key", "task_input", self.instructor) + task_input = {"email_id": email.id} # pylint: disable=E1101 + with self.assertRaisesRegexp(ValueError, 'does not match task value'): + perform_delegate_email_batches(entry.id, self.course.id, task_input, "action_name") # pylint: disable=E1101 + + def test_wrong_course_id_in_email(self): + """ + Tests exception when the course_id in CourseEmail is not the same as one explicitly passed in. + """ + email = CourseEmail(course_id="bogus_course_id", to_option=SEND_TO_ALL) + email.save() + entry = InstructorTask.create(self.course.id, "task_type", "task_key", "task_input", self.instructor) + task_input = {"email_id": email.id} # pylint: disable=E1101 + with self.assertRaisesRegexp(ValueError, 'does not match email value'): + perform_delegate_email_batches(entry.id, self.course.id, task_input, "action_name") # pylint: disable=E1101 + + def test_send_email_undefined_subtask(self): + # test at a lower level, to ensure that the course gets checked down below too. + entry = InstructorTask.create(self.course.id, "task_type", "task_key", "task_input", self.instructor) + entry_id = entry.id # pylint: disable=E1101 + to_list = ['test@test.com'] + global_email_context = {'course_title': 'dummy course'} + subtask_id = "subtask-id-value" + subtask_status = create_subtask_status(subtask_id) + email_id = 1001 + with self.assertRaisesRegexp(ValueError, 'unable to find email subtasks of instructor task'): + send_course_email(entry_id, email_id, to_list, global_email_context, subtask_status) + + def test_send_email_missing_subtask(self): + # test at a lower level, to ensure that the course gets checked down below too. + entry = InstructorTask.create(self.course.id, "task_type", "task_key", "task_input", self.instructor) + entry_id = entry.id # pylint: disable=E1101 + to_list = ['test@test.com'] + global_email_context = {'course_title': 'dummy course'} + subtask_id = "subtask-id-value" + initialize_subtask_info(entry, "emailed", 100, [subtask_id]) + different_subtask_id = "bogus-subtask-id-value" + subtask_status = create_subtask_status(different_subtask_id) + bogus_email_id = 1001 + with self.assertRaisesRegexp(ValueError, 'unable to find status for email subtask of instructor task'): + send_course_email(entry_id, bogus_email_id, to_list, global_email_context, subtask_status) + + def dont_test_send_email_undefined_email(self): + # test at a lower level, to ensure that the course gets checked down below too. + entry = InstructorTask.create(self.course.id, "task_type", "task_key", "task_input", self.instructor) + entry_id = entry.id # pylint: disable=E1101 + to_list = ['test@test.com'] + global_email_context = {'course_title': 'dummy course'} + subtask_id = "subtask-id-value" + initialize_subtask_info(entry, "emailed", 100, [subtask_id]) + subtask_status = create_subtask_status(subtask_id) + bogus_email_id = 1001 + with self.assertRaises(CourseEmail.DoesNotExist): + # we skip the call that updates subtask status, since we've not set up the InstructorTask + # for the subtask, and it's not important to the test. + with patch('bulk_email.tasks.update_subtask_status'): + send_course_email(entry_id, bogus_email_id, to_list, global_email_context, subtask_status) diff --git a/lms/djangoapps/bulk_email/tests/test_forms.py b/lms/djangoapps/bulk_email/tests/test_forms.py new file mode 100644 index 0000000000..60a8abf95e --- /dev/null +++ b/lms/djangoapps/bulk_email/tests/test_forms.py @@ -0,0 +1,105 @@ +""" +Unit tests for bulk-email-related forms. +""" +from django.test.utils import override_settings +from django.conf import settings + +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE +from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE + +from xmodule.modulestore.django import modulestore +from xmodule.modulestore import MONGO_MODULESTORE_TYPE + +from mock import patch + +from bulk_email.models import CourseAuthorization +from bulk_email.forms import CourseAuthorizationAdminForm + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class CourseAuthorizationFormTest(ModuleStoreTestCase): + """Test the CourseAuthorizationAdminForm form for Mongo-backed courses.""" + + def setUp(self): + # Make a mongo course + self.course = CourseFactory.create() + + def tearDown(self): + """ + Undo all patches. + """ + patch.stopall() + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': True}) + def test_authorize_mongo_course(self): + # Initially course shouldn't be authorized + self.assertFalse(CourseAuthorization.instructor_email_enabled(self.course.id)) + # Test authorizing the course, which should totally work + form_data = {'course_id': self.course.id, 'email_enabled': True} + form = CourseAuthorizationAdminForm(data=form_data) + # Validation should work + self.assertTrue(form.is_valid()) + form.save() + # Check that this course is authorized + self.assertTrue(CourseAuthorization.instructor_email_enabled(self.course.id)) + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': True}) + def test_form_typo(self): + # Munge course id + bad_id = self.course.id + '_typo' + + form_data = {'course_id': bad_id, 'email_enabled': True} + form = CourseAuthorizationAdminForm(data=form_data) + # Validation shouldn't work + self.assertFalse(form.is_valid()) + + msg = u'Error encountered (Course not found.)' + msg += ' --- Entered course id was: "{0}". '.format(bad_id) + msg += 'Please recheck that you have supplied a course id in the format: ORG/COURSE/RUN' + self.assertEquals(msg, form._errors['course_id'][0]) # pylint: disable=protected-access + + with self.assertRaisesRegexp(ValueError, "The CourseAuthorization could not be created because the data didn't validate."): + form.save() + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': True}) + def test_course_name_only(self): + # Munge course id - common + bad_id = self.course.id.split('/')[-1] + + form_data = {'course_id': bad_id, 'email_enabled': True} + form = CourseAuthorizationAdminForm(data=form_data) + # Validation shouldn't work + self.assertFalse(form.is_valid()) + + msg = u'Error encountered (Need more than 1 value to unpack)' + msg += ' --- Entered course id was: "{0}". '.format(bad_id) + msg += 'Please recheck that you have supplied a course id in the format: ORG/COURSE/RUN' + self.assertEquals(msg, form._errors['course_id'][0]) # pylint: disable=protected-access + + with self.assertRaisesRegexp(ValueError, "The CourseAuthorization could not be created because the data didn't validate."): + form.save() + + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class CourseAuthorizationXMLFormTest(ModuleStoreTestCase): + """Check that XML courses cannot be authorized for email.""" + + @patch.dict(settings.MITX_FEATURES, {'ENABLE_INSTRUCTOR_EMAIL': True, 'REQUIRE_COURSE_EMAIL_AUTH': True}) + def test_xml_course_authorization(self): + course_id = 'edX/toy/2012_Fall' + # Assert this is an XML course + self.assertTrue(modulestore().get_modulestore_type(course_id) != MONGO_MODULESTORE_TYPE) + + form_data = {'course_id': course_id, 'email_enabled': True} + form = CourseAuthorizationAdminForm(data=form_data) + # Validation shouldn't work + self.assertFalse(form.is_valid()) + + msg = u"Course Email feature is only available for courses authored in Studio. " + msg += '"{0}" appears to be an XML backed course.'.format(course_id) + self.assertEquals(msg, form._errors['course_id'][0]) # pylint: disable=protected-access + + with self.assertRaisesRegexp(ValueError, "The CourseAuthorization could not be created because the data didn't validate."): + form.save() diff --git a/lms/djangoapps/bulk_email/tests/test_models.py b/lms/djangoapps/bulk_email/tests/test_models.py new file mode 100644 index 0000000000..30713e178c --- /dev/null +++ b/lms/djangoapps/bulk_email/tests/test_models.py @@ -0,0 +1,147 @@ +""" +Unit tests for bulk-email-related models. +""" +from django.test import TestCase +from django.core.management import call_command +from django.conf import settings + +from student.tests.factories import UserFactory + +from mock import patch + +from bulk_email.models import CourseEmail, SEND_TO_STAFF, CourseEmailTemplate, CourseAuthorization + + +class CourseEmailTest(TestCase): + """Test the CourseEmail model.""" + + def test_creation(self): + course_id = 'abc/123/doremi' + sender = UserFactory.create() + to_option = SEND_TO_STAFF + subject = "dummy subject" + html_message = "dummy message" + email = CourseEmail.create(course_id, sender, to_option, subject, html_message) + self.assertEquals(email.course_id, course_id) + self.assertEquals(email.to_option, SEND_TO_STAFF) + self.assertEquals(email.subject, subject) + self.assertEquals(email.html_message, html_message) + self.assertEquals(email.sender, sender) + + def test_bad_to_option(self): + course_id = 'abc/123/doremi' + sender = UserFactory.create() + to_option = "fake" + subject = "dummy subject" + html_message = "dummy message" + with self.assertRaises(ValueError): + CourseEmail.create(course_id, sender, to_option, subject, html_message) + + +class NoCourseEmailTemplateTest(TestCase): + """Test the CourseEmailTemplate model without loading the template data.""" + + def test_get_missing_template(self): + with self.assertRaises(CourseEmailTemplate.DoesNotExist): + CourseEmailTemplate.get_template() + + +class CourseEmailTemplateTest(TestCase): + """Test the CourseEmailTemplate model.""" + + def setUp(self): + # load initial content (since we don't run migrations as part of tests): + call_command("loaddata", "course_email_template.json") + + def _get_sample_plain_context(self): + """Provide sample context sufficient for rendering plaintext template""" + context = { + 'course_title': "Bogus Course Title", + 'course_url': "/location/of/course/url", + 'account_settings_url': "/location/of/account/settings/url", + 'platform_name': 'edX', + 'email': 'your-email@test.com', + } + return context + + def _get_sample_html_context(self): + """Provide sample context sufficient for rendering HTML template""" + context = self._get_sample_plain_context() + context['course_image_url'] = "/location/of/course/image/url" + return context + + def test_get_template(self): + template = CourseEmailTemplate.get_template() + self.assertIsNotNone(template.html_template) + self.assertIsNotNone(template.plain_template) + + def test_render_html_without_context(self): + template = CourseEmailTemplate.get_template() + base_context = self._get_sample_html_context() + for keyname in base_context: + context = dict(base_context) + del context[keyname] + with self.assertRaises(KeyError): + template.render_htmltext("My new html text.", context) + + def test_render_plaintext_without_context(self): + template = CourseEmailTemplate.get_template() + base_context = self._get_sample_plain_context() + for keyname in base_context: + context = dict(base_context) + del context[keyname] + with self.assertRaises(KeyError): + template.render_plaintext("My new plain text.", context) + + def test_render_html(self): + template = CourseEmailTemplate.get_template() + context = self._get_sample_html_context() + template.render_htmltext("My new html text.", context) + + def test_render_plain(self): + template = CourseEmailTemplate.get_template() + context = self._get_sample_plain_context() + template.render_plaintext("My new plain text.", context) + + +class CourseAuthorizationTest(TestCase): + """Test the CourseAuthorization model.""" + + @patch.dict(settings.MITX_FEATURES, {'REQUIRE_COURSE_EMAIL_AUTH': True}) + def test_creation_auth_on(self): + course_id = 'abc/123/doremi' + # Test that course is not authorized by default + self.assertFalse(CourseAuthorization.instructor_email_enabled(course_id)) + + # Authorize + cauth = CourseAuthorization(course_id=course_id, email_enabled=True) + cauth.save() + # Now, course should be authorized + self.assertTrue(CourseAuthorization.instructor_email_enabled(course_id)) + self.assertEquals( + cauth.__unicode__(), + "Course 'abc/123/doremi': Instructor Email Enabled" + ) + + # Unauthorize by explicitly setting email_enabled to False + cauth.email_enabled = False + cauth.save() + # Test that course is now unauthorized + self.assertFalse(CourseAuthorization.instructor_email_enabled(course_id)) + self.assertEquals( + cauth.__unicode__(), + "Course 'abc/123/doremi': Instructor Email Not Enabled" + ) + + @patch.dict(settings.MITX_FEATURES, {'REQUIRE_COURSE_EMAIL_AUTH': False}) + def test_creation_auth_off(self): + course_id = 'blahx/blah101/ehhhhhhh' + # Test that course is authorized by default, since auth is turned off + self.assertTrue(CourseAuthorization.instructor_email_enabled(course_id)) + + # Use the admin interface to unauthorize the course + cauth = CourseAuthorization(course_id=course_id, email_enabled=False) + cauth.save() + + # Now, course should STILL be authorized! + self.assertTrue(CourseAuthorization.instructor_email_enabled(course_id)) diff --git a/lms/djangoapps/bulk_email/tests/test_tasks.py b/lms/djangoapps/bulk_email/tests/test_tasks.py new file mode 100644 index 0000000000..e49a04147a --- /dev/null +++ b/lms/djangoapps/bulk_email/tests/test_tasks.py @@ -0,0 +1,398 @@ +""" +Unit tests for LMS instructor-initiated background tasks. + +Runs tasks on answers to course problems to validate that code +paths actually work. + +""" +import json +from uuid import uuid4 +from itertools import cycle, chain, repeat +from mock import patch, Mock +from smtplib import SMTPServerDisconnected, SMTPDataError, SMTPConnectError, SMTPAuthenticationError +from boto.ses.exceptions import ( + SESDailyQuotaExceededError, + SESMaxSendingRateExceededError, + SESAddressBlacklistedError, + SESIllegalAddressError, + SESLocalAddressCharacterError, +) +from boto.exception import AWSConnectionError + +from celery.states import SUCCESS, FAILURE + +from django.conf import settings +from django.core.management import call_command + +from bulk_email.models import CourseEmail, Optout, SEND_TO_ALL + +from instructor_task.tasks import send_bulk_course_email +from instructor_task.subtasks import update_subtask_status +from instructor_task.models import InstructorTask +from instructor_task.tests.test_base import InstructorTaskCourseTestCase +from instructor_task.tests.factories import InstructorTaskFactory + + +class TestTaskFailure(Exception): + """Dummy exception used for unit tests.""" + pass + + +def my_update_subtask_status(entry_id, current_task_id, new_subtask_status): + """ + Check whether a subtask has been updated before really updating. + + Check whether a subtask which has been retried + has had the retry already write its results here before the code + that was invoking the retry had a chance to update this status. + + This is the norm in "eager" mode (used by tests) where the retry is called + and run to completion before control is returned to the code that + invoked the retry. If the retries eventually end in failure (e.g. due to + a maximum number of retries being attempted), the "eager" code will return + the error for each retry as it is popped off the stack. We want to just ignore + the later updates that are called as the result of the earlier retries. + + This should not be an issue in production, where status is updated before + a task is retried, and is then updated afterwards if the retry fails. + """ + entry = InstructorTask.objects.get(pk=entry_id) + subtask_dict = json.loads(entry.subtasks) + subtask_status_info = subtask_dict['status'] + current_subtask_status = subtask_status_info[current_task_id] + + def _get_retry_count(subtask_result): + """Return the number of retries counted for the given subtask.""" + retry_count = subtask_result.get('retried_nomax', 0) + retry_count += subtask_result.get('retried_withmax', 0) + return retry_count + + current_retry_count = _get_retry_count(current_subtask_status) + new_retry_count = _get_retry_count(new_subtask_status) + if current_retry_count <= new_retry_count: + update_subtask_status(entry_id, current_task_id, new_subtask_status) + + +class TestBulkEmailInstructorTask(InstructorTaskCourseTestCase): + """Tests instructor task that send bulk email.""" + + def setUp(self): + super(TestBulkEmailInstructorTask, self).setUp() + self.initialize_course() + self.instructor = self.create_instructor('instructor') + + # load initial content (since we don't run migrations as part of tests): + call_command("loaddata", "course_email_template.json") + + def _create_input_entry(self, course_id=None): + """ + Creates a InstructorTask entry for testing. + + Overrides the base class version in that this creates CourseEmail. + """ + to_option = SEND_TO_ALL + course_id = course_id or self.course.id + course_email = CourseEmail.create(course_id, self.instructor, to_option, "Test Subject", "

This is a test message

") + task_input = {'email_id': course_email.id} # pylint: disable=E1101 + task_id = str(uuid4()) + instructor_task = InstructorTaskFactory.create( + course_id=course_id, + requester=self.instructor, + task_input=json.dumps(task_input), + task_key='dummy value', + task_id=task_id, + ) + return instructor_task + + def _run_task_with_mock_celery(self, task_class, entry_id, task_id): + """Submit a task and mock how celery provides a current_task.""" + mock_current_task = Mock() + mock_current_task.max_retries = settings.BULK_EMAIL_MAX_RETRIES + mock_current_task.default_retry_delay = settings.BULK_EMAIL_DEFAULT_RETRY_DELAY + task_args = [entry_id, {}] + + with patch('bulk_email.tasks._get_current_task') as mock_get_task: + mock_get_task.return_value = mock_current_task + return task_class.apply(task_args, task_id=task_id).get() + + def test_email_missing_current_task(self): + task_entry = self._create_input_entry() + with self.assertRaises(ValueError): + send_bulk_course_email(task_entry.id, {}) + + def test_email_undefined_course(self): + # Check that we fail when passing in a course that doesn't exist. + task_entry = self._create_input_entry(course_id="bogus/course/id") + with self.assertRaises(ValueError): + self._run_task_with_mock_celery(send_bulk_course_email, task_entry.id, task_entry.task_id) + + def test_bad_task_id_on_update(self): + task_entry = self._create_input_entry() + + def dummy_update_subtask_status(entry_id, _current_task_id, new_subtask_status): + """Passes a bad value for task_id to test update_subtask_status""" + bogus_task_id = "this-is-bogus" + update_subtask_status(entry_id, bogus_task_id, new_subtask_status) + + with self.assertRaises(ValueError): + with patch('bulk_email.tasks.update_subtask_status', dummy_update_subtask_status): + send_bulk_course_email(task_entry.id, {}) # pylint: disable=E1101 + + def _create_students(self, num_students): + """Create students for testing""" + return [self.create_student('robot%d' % i) for i in xrange(num_students)] + + def _assert_single_subtask_status(self, entry, succeeded, failed=0, skipped=0, retried_nomax=0, retried_withmax=0): + """Compare counts with 'subtasks' entry in InstructorTask table.""" + subtask_info = json.loads(entry.subtasks) + # verify subtask-level counts: + self.assertEquals(subtask_info.get('total'), 1) + self.assertEquals(subtask_info.get('succeeded'), 1 if succeeded > 0 else 0) + self.assertEquals(subtask_info.get('failed'), 0 if succeeded > 0 else 1) + # verify individual subtask status: + subtask_status_info = subtask_info.get('status') + task_id_list = subtask_status_info.keys() + self.assertEquals(len(task_id_list), 1) + task_id = task_id_list[0] + subtask_status = subtask_status_info.get(task_id) + print("Testing subtask status: {}".format(subtask_status)) + self.assertEquals(subtask_status.get('task_id'), task_id) + self.assertEquals(subtask_status.get('attempted'), succeeded + failed) + self.assertEquals(subtask_status.get('succeeded'), succeeded) + self.assertEquals(subtask_status.get('skipped'), skipped) + self.assertEquals(subtask_status.get('failed'), failed) + self.assertEquals(subtask_status.get('retried_nomax'), retried_nomax) + self.assertEquals(subtask_status.get('retried_withmax'), retried_withmax) + self.assertEquals(subtask_status.get('state'), SUCCESS if succeeded > 0 else FAILURE) + + def _test_run_with_task(self, task_class, action_name, total, succeeded, failed=0, skipped=0, retried_nomax=0, retried_withmax=0): + """Run a task and check the number of emails processed.""" + task_entry = self._create_input_entry() + parent_status = self._run_task_with_mock_celery(task_class, task_entry.id, task_entry.task_id) + + # check return value + self.assertEquals(parent_status.get('total'), total) + self.assertEquals(parent_status.get('action_name'), action_name) + + # compare with task_output entry in InstructorTask table: + entry = InstructorTask.objects.get(id=task_entry.id) + status = json.loads(entry.task_output) + self.assertEquals(status.get('attempted'), succeeded + failed) + self.assertEquals(status.get('succeeded'), succeeded) + self.assertEquals(status.get('skipped'), skipped) + self.assertEquals(status.get('failed'), failed) + self.assertEquals(status.get('total'), total) + self.assertEquals(status.get('action_name'), action_name) + self.assertGreater(status.get('duration_ms'), 0) + self.assertEquals(entry.task_state, SUCCESS) + self._assert_single_subtask_status(entry, succeeded, failed, skipped, retried_nomax, retried_withmax) + return entry + + def test_successful(self): + # Select number of emails to fit into a single subtask. + num_emails = settings.BULK_EMAIL_EMAILS_PER_TASK + # We also send email to the instructor: + self._create_students(num_emails - 1) + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + get_conn.return_value.send_messages.side_effect = cycle([None]) + self._test_run_with_task(send_bulk_course_email, 'emailed', num_emails, num_emails) + + def test_successful_twice(self): + # Select number of emails to fit into a single subtask. + num_emails = settings.BULK_EMAIL_EMAILS_PER_TASK + # We also send email to the instructor: + self._create_students(num_emails - 1) + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + get_conn.return_value.send_messages.side_effect = cycle([None]) + task_entry = self._test_run_with_task(send_bulk_course_email, 'emailed', num_emails, num_emails) + + # submit the same task a second time, and confirm that it is not run again. + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + get_conn.return_value.send_messages.side_effect = cycle([Exception("This should not happen!")]) + parent_status = self._run_task_with_mock_celery(send_bulk_course_email, task_entry.id, task_entry.task_id) + self.assertEquals(parent_status.get('total'), num_emails) + self.assertEquals(parent_status.get('succeeded'), num_emails) + self.assertEquals(parent_status.get('failed'), 0) + + def test_unactivated_user(self): + # Select number of emails to fit into a single subtask. + num_emails = settings.BULK_EMAIL_EMAILS_PER_TASK + # We also send email to the instructor: + students = self._create_students(num_emails - 1) + # mark a student as not yet having activated their email: + student = students[0] + student.is_active = False + student.save() + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + get_conn.return_value.send_messages.side_effect = cycle([None]) + self._test_run_with_task(send_bulk_course_email, 'emailed', num_emails - 1, num_emails - 1) + + def test_skipped(self): + # Select number of emails to fit into a single subtask. + num_emails = settings.BULK_EMAIL_EMAILS_PER_TASK + # We also send email to the instructor: + students = self._create_students(num_emails - 1) + # have every fourth student optout: + expected_skipped = int((num_emails + 3) / 4.0) + expected_succeeds = num_emails - expected_skipped + for index in range(0, num_emails, 4): + Optout.objects.create(user=students[index], course_id=self.course.id) + # mark some students as opting out + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + get_conn.return_value.send_messages.side_effect = cycle([None]) + self._test_run_with_task(send_bulk_course_email, 'emailed', num_emails, expected_succeeds, skipped=expected_skipped) + + def _test_email_address_failures(self, exception): + """Test that celery handles bad address errors by failing and not retrying.""" + # Select number of emails to fit into a single subtask. + num_emails = settings.BULK_EMAIL_EMAILS_PER_TASK + # We also send email to the instructor: + self._create_students(num_emails - 1) + expected_fails = int((num_emails + 3) / 4.0) + expected_succeeds = num_emails - expected_fails + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + # have every fourth email fail due to some address failure: + get_conn.return_value.send_messages.side_effect = cycle([exception, None, None, None]) + self._test_run_with_task(send_bulk_course_email, 'emailed', num_emails, expected_succeeds, failed=expected_fails) + + def test_smtp_blacklisted_user(self): + # Test that celery handles permanent SMTPDataErrors by failing and not retrying. + self._test_email_address_failures(SMTPDataError(554, "Email address is blacklisted")) + + def test_ses_blacklisted_user(self): + # Test that celery handles permanent SMTPDataErrors by failing and not retrying. + self._test_email_address_failures(SESAddressBlacklistedError(554, "Email address is blacklisted")) + + def test_ses_illegal_address(self): + # Test that celery handles permanent SMTPDataErrors by failing and not retrying. + self._test_email_address_failures(SESIllegalAddressError(554, "Email address is illegal")) + + def test_ses_local_address_character_error(self): + # Test that celery handles permanent SMTPDataErrors by failing and not retrying. + self._test_email_address_failures(SESLocalAddressCharacterError(554, "Email address contains a bad character")) + + def _test_retry_after_limited_retry_error(self, exception): + """Test that celery handles connection failures by retrying.""" + # If we want the batch to succeed, we need to send fewer emails + # than the max retries, so that the max is not triggered. + num_emails = settings.BULK_EMAIL_MAX_RETRIES + # We also send email to the instructor: + self._create_students(num_emails - 1) + expected_fails = 0 + expected_succeeds = num_emails + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + # Have every other mail attempt fail due to disconnection. + get_conn.return_value.send_messages.side_effect = cycle([exception, None]) + self._test_run_with_task( + send_bulk_course_email, + 'emailed', + num_emails, + expected_succeeds, + failed=expected_fails, + retried_withmax=num_emails + ) + + def _test_max_retry_limit_causes_failure(self, exception): + """Test that celery can hit a maximum number of retries.""" + # Doesn't really matter how many recipients, since we expect + # to fail on the first. + num_emails = 10 + # We also send email to the instructor: + self._create_students(num_emails - 1) + expected_fails = num_emails + expected_succeeds = 0 + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + # always fail to connect, triggering repeated retries until limit is hit: + get_conn.return_value.send_messages.side_effect = cycle([exception]) + with patch('bulk_email.tasks.update_subtask_status', my_update_subtask_status): + self._test_run_with_task( + send_bulk_course_email, + 'emailed', + num_emails, + expected_succeeds, + failed=expected_fails, + retried_withmax=(settings.BULK_EMAIL_MAX_RETRIES + 1) + ) + + def test_retry_after_smtp_disconnect(self): + self._test_retry_after_limited_retry_error(SMTPServerDisconnected(425, "Disconnecting")) + + def test_max_retry_after_smtp_disconnect(self): + self._test_max_retry_limit_causes_failure(SMTPServerDisconnected(425, "Disconnecting")) + + def test_retry_after_smtp_connect_error(self): + self._test_retry_after_limited_retry_error(SMTPConnectError(424, "Bad Connection")) + + def test_max_retry_after_smtp_connect_error(self): + self._test_max_retry_limit_causes_failure(SMTPConnectError(424, "Bad Connection")) + + def test_retry_after_aws_connect_error(self): + self._test_retry_after_limited_retry_error(AWSConnectionError("Unable to provide secure connection through proxy")) + + def test_max_retry_after_aws_connect_error(self): + self._test_max_retry_limit_causes_failure(AWSConnectionError("Unable to provide secure connection through proxy")) + + def test_retry_after_general_error(self): + self._test_retry_after_limited_retry_error(Exception("This is some random exception.")) + + def test_max_retry_after_general_error(self): + self._test_max_retry_limit_causes_failure(Exception("This is some random exception.")) + + def _test_retry_after_unlimited_retry_error(self, exception): + """Test that celery handles throttling failures by retrying.""" + num_emails = 8 + # We also send email to the instructor: + self._create_students(num_emails - 1) + expected_fails = 0 + expected_succeeds = num_emails + # Note that because celery in eager mode will call retries synchronously, + # each retry will increase the stack depth. It turns out that there is a + # maximum depth at which a RuntimeError is raised ("maximum recursion depth + # exceeded"). The maximum recursion depth is 90, so + # num_emails * expected_retries < 90. + expected_retries = 10 + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + # Cycle through N throttling errors followed by a success. + get_conn.return_value.send_messages.side_effect = cycle( + chain(repeat(exception, expected_retries), [None]) + ) + self._test_run_with_task( + send_bulk_course_email, + 'emailed', + num_emails, + expected_succeeds, + failed=expected_fails, + retried_nomax=(expected_retries * num_emails) + ) + + def test_retry_after_smtp_throttling_error(self): + self._test_retry_after_unlimited_retry_error(SMTPDataError(455, "Throttling: Sending rate exceeded")) + + def test_retry_after_ses_throttling_error(self): + self._test_retry_after_unlimited_retry_error(SESMaxSendingRateExceededError(455, "Throttling: Sending rate exceeded")) + + def _test_immediate_failure(self, exception): + """Test that celery can hit a maximum number of retries.""" + # Doesn't really matter how many recipients, since we expect + # to fail on the first. + num_emails = 10 + # We also send email to the instructor: + self._create_students(num_emails - 1) + expected_fails = num_emails + expected_succeeds = 0 + with patch('bulk_email.tasks.get_connection', autospec=True) as get_conn: + # always fail to connect, triggering repeated retries until limit is hit: + get_conn.return_value.send_messages.side_effect = cycle([exception]) + self._test_run_with_task( + send_bulk_course_email, + 'emailed', + num_emails, + expected_succeeds, + failed=expected_fails, + ) + + def test_failure_on_unhandled_smtp(self): + self._test_immediate_failure(SMTPAuthenticationError(403, "That password doesn't work!")) + + def test_failure_on_ses_quota_exceeded(self): + self._test_immediate_failure(SESDailyQuotaExceededError(403, "You're done for the day!")) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 78e1d51cbc..106373139c 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -36,11 +36,31 @@ def get_request_for_thread(): del frame +def get_course(course_id, depth=0): + """ + Given a course id, return the corresponding course descriptor. + + If course_id is not valid, raises a ValueError. This is appropriate + for internal use. + + depth: The number of levels of children for the modulestore to cache. + None means infinite depth. Default is to fetch no children. + """ + try: + course_loc = CourseDescriptor.id_to_location(course_id) + return modulestore().get_instance(course_id, course_loc, depth=depth) + except (KeyError, ItemNotFoundError): + raise ValueError("Course not found: {}".format(course_id)) + except InvalidLocationError: + raise ValueError("Invalid location: {}".format(course_id)) + + def get_course_by_id(course_id, depth=0): """ Given a course id, return the corresponding course descriptor. If course_id is not valid, raises a 404. + depth: The number of levels of children for the modulestore to cache. None means infinite depth """ try: @@ -51,6 +71,7 @@ def get_course_by_id(course_id, depth=0): except InvalidLocationError: raise Http404("Invalid location") + def get_course_with_access(user, course_id, action, depth=0): """ Given a course_id, look up the corresponding course descriptor, @@ -182,7 +203,6 @@ def get_course_about_section(course, section_key): raise KeyError("Invalid about key " + str(section_key)) - def get_course_info_section(request, course, section_key): """ This returns the snippet of html to be rendered on the course info page, @@ -194,8 +214,6 @@ def get_course_info_section(request, course, section_key): - updates - guest_updates """ - - loc = Location(course.location.tag, course.location.org, course.location.course, 'course_info', section_key) # Use an empty cache diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index 20cb83a411..7960515ae0 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -2,15 +2,18 @@ from django.test import TestCase from django.http import Http404 from django.test.utils import override_settings -from courseware.courses import get_course_by_id, get_cms_course_link_by_id +from courseware.courses import get_course_by_id, get_course, get_cms_course_link_by_id CMS_BASE_TEST = 'testcms' + class CoursesTest(TestCase): + """Test methods related to fetching courses.""" + def test_get_course_by_id_invalid_chars(self): """ Test that `get_course_by_id` throws a 404, rather than - an exception, when faced with unexpected characters + an exception, when faced with unexpected characters (such as unicode characters, and symbols such as = and ' ') """ with self.assertRaises(Http404): @@ -18,6 +21,17 @@ class CoursesTest(TestCase): get_course_by_id('MITx/foobar/business and management') get_course_by_id('MITx/foobar/NiñøJoséMaríáßç') + def test_get_course_invalid_chars(self): + """ + Test that `get_course` throws a ValueError, rather than + a 404, when faced with unexpected characters + (such as unicode characters, and symbols such as = and ' ') + """ + with self.assertRaises(ValueError): + get_course('MITx/foobar/statistics=introduction') + get_course('MITx/foobar/business and management') + get_course('MITx/foobar/NiñøJoséMaríáßç') + @override_settings(CMS_BASE=CMS_BASE_TEST) def test_get_cms_course_link_by_id(self): """ diff --git a/lms/djangoapps/instructor/features/bulk_email.feature b/lms/djangoapps/instructor/features/bulk_email.feature new file mode 100644 index 0000000000..8d3784c1ea --- /dev/null +++ b/lms/djangoapps/instructor/features/bulk_email.feature @@ -0,0 +1,20 @@ +@shard_2 +Feature: Bulk Email + As an instructor or course staff, + In order to communicate with students and staff + I want to send email to staff and students in a course. + + Scenario: Send bulk email + Given I am "" for a course + When I send email to "" + Then Email is sent to "" + + Examples: + | Role | Recipient | + | instructor | myself | + | instructor | course staff | + | instructor | students, staff, and instructors | + | staff | myself | + | staff | course staff | + | staff | students, staff, and instructors | + diff --git a/lms/djangoapps/instructor/features/bulk_email.py b/lms/djangoapps/instructor/features/bulk_email.py new file mode 100644 index 0000000000..220ab9ecee --- /dev/null +++ b/lms/djangoapps/instructor/features/bulk_email.py @@ -0,0 +1,162 @@ +""" +Define steps for bulk email acceptance test. +""" + +#pylint: disable=C0111 +#pylint: disable=W0621 + +from lettuce import world, step +from lettuce.django import mail +from nose.tools import assert_in, assert_true, assert_equal # pylint: disable=E0611 +from django.core.management import call_command +from django.conf import settings + + +@step(u'I am an instructor for a course') +def i_am_an_instructor(step): # pylint: disable=W0613 + + # Clear existing courses to avoid conflicts + world.clear_courses() + + # Register the instructor as staff for the course + world.register_by_course_id( + 'edx/999/Test_Course', + username='instructor', + password='password', + is_staff=True + ) + world.add_to_course_staff('instructor', '999') + + # Register another staff member + world.register_by_course_id( + 'edx/999/Test_Course', + username='staff', + password='password', + is_staff=True + ) + world.add_to_course_staff('staff', '999') + + # Register a student + world.register_by_course_id( + 'edx/999/Test_Course', + username='student', + password='password', + is_staff=False + ) + + # Log in as the instructor for the course + world.log_in( + username='instructor', + password='password', + email="instructor@edx.org", + name="Instructor" + ) + + +# Dictionary mapping a description of the email recipient +# to the corresponding