Merge pull request #1209 from edx/brian/email-grouping

Task progress notification on instructor dash

CHANGELOG.rst

@@ -9,6 +9,8 @@ LMS: Disable data download buttons on the instructor dashboard for large courses
 
 LMS: Ported bulk emailing to the beta instructor dashboard.
 
+LMS: Add monitoring of bulk email subtasks to display progress on instructor dash.
+
 LMS: Refactor and clean student dashboard templates.
 
 LMS: Fix issue with CourseMode expiration dates

lms/djangoapps/bulk_email/models.py

@@ -12,8 +12,9 @@ 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
 
 log = logging.getLogger(__name__)
 
@@ -33,12 +34,14 @@ class Email(models.Model):
     class Meta:  # pylint: disable=C0111
         abstract = True
 
+
 SEND_TO_MYSELF = 'myself'
 SEND_TO_STAFF = 'staff'
 SEND_TO_ALL = 'all'
+TO_OPTIONS = [SEND_TO_MYSELF, SEND_TO_STAFF, SEND_TO_ALL]
 
 
-class CourseEmail(Email, models.Model):
+class CourseEmail(Email):
     """
     Stores information for an email to a course.
     """
@@ -51,17 +54,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 +153,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):

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)
@@ -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)

lms/djangoapps/bulk_email/tests/test_models.py

+"""
+Unit tests for bulk-email-related models.
+"""
+from django.test import TestCase
+from django.core.management import call_command
+
+from student.tests.factories import UserFactory
+
+from bulk_email.models import CourseEmail, SEND_TO_STAFF, CourseEmailTemplate
+
+
+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 = "<html>dummy message</html>"
+        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 = "<html>dummy message</html>"
+        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)

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

lms/djangoapps/courseware/tests/test_courses.py

@@ -4,16 +4,19 @@ import mock
 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
 from xmodule.modulestore.django import get_default_store_name_for_current_request
 
 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):
@@ -21,6 +24,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):
         """

lms/djangoapps/instructor/views/api.py

@@ -40,8 +40,6 @@ import analytics.csvs
 import csv
 
 from bulk_email.models import CourseEmail
-from html_to_text import html_to_text
-from bulk_email import tasks
 
 log = logging.getLogger(__name__)
 
@@ -755,7 +753,7 @@ def send_email(request, course_id):
     Send an email to self, staff, or everyone involved in a course.
     Query Parameters:
     - 'send_to' specifies what group the email should be sent to
-       Options are defined by the Email model in
+       Options are defined by the CourseEmail model in
        lms/djangoapps/bulk_email/models.py
     - 'subject' specifies email's subject
     - 'message' specifies email's content
@@ -763,17 +761,15 @@ def send_email(request, course_id):
     send_to = request.POST.get("send_to")
     subject = request.POST.get("subject")
     message = request.POST.get("message")
-    text_message = html_to_text(message)
-    email = CourseEmail(
-        course_id=course_id,
-        sender=request.user,
-        to_option=send_to,
-        subject=subject,
-        html_message=message,
-        text_message=text_message,
-    )
-    email.save()
-    tasks.delegate_email_batches.delay(email.id, request.user.id)  # pylint: disable=E1101
+
+    # Create the CourseEmail object.  This is saved immediately, so that
+    # any transaction that has been pending up to this point will also be
+    # committed.
+    email = CourseEmail.create(course_id, request.user, send_to, subject, message)
+
+    # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes)
+    instructor_task.api.submit_bulk_course_email(request, course_id, email.id)  # pylint: disable=E1101
+
     response_payload = {'course_id': course_id}
     return JsonResponse(response_payload)
 

lms/djangoapps/instructor/views/legacy.py

@@ -30,6 +30,7 @@ from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.html_module import HtmlDescriptor
 
+from bulk_email.models import CourseEmail
 from courseware import grades
 from courseware.access import (has_access, get_access_group_name,
                                course_beta_test_group_name)
@@ -46,7 +47,8 @@ from instructor_task.api import (get_running_instructor_tasks,
                                  get_instructor_task_history,
                                  submit_rescore_problem_for_all_students,
                                  submit_rescore_problem_for_student,
-                                 submit_reset_problem_attempts_for_all_students)
+                                 submit_reset_problem_attempts_for_all_students,
+                                 submit_bulk_course_email)
 from instructor_task.views import get_task_completion_info
 from mitxmako.shortcuts import render_to_response
 from psychometrics import psychoanalyze
@@ -58,10 +60,6 @@ from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
 from django.utils.translation import ugettext as _u
 
-from bulk_email.models import CourseEmail
-from html_to_text import html_to_text
-from bulk_email import tasks
-
 log = logging.getLogger(__name__)
 
 # internal commands for managing forum roles:
@@ -720,29 +718,24 @@ def instructor_dashboard(request, course_id):
         email_to_option = request.POST.get("to_option")
         email_subject = request.POST.get("subject")
         html_message = request.POST.get("message")
-        text_message = html_to_text(html_message)
-
-        email = CourseEmail(
-            course_id=course_id,
-            sender=request.user,
-            to_option=email_to_option,
-            subject=email_subject,
-            html_message=html_message,
-            text_message=text_message
-        )
 
-        email.save()
+        # Create the CourseEmail object.  This is saved immediately, so that
+        # any transaction that has been pending up to this point will also be
+        # committed.
+        email = CourseEmail.create(course_id, request.user, email_to_option, email_subject, html_message)
 
-        tasks.delegate_email_batches.delay(
-            email.id,
-            request.user.id
-        )
+        # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes)
+        submit_bulk_course_email(request, course_id, email.id)  # pylint: disable=E1101
 
         if email_to_option == "all":
             email_msg = '<div class="msg msg-confirm"><p class="copy">Your email was successfully queued for sending. Please note that for large public classes (~10k), it may take 1-2 hours to send all emails.</p></div>'
         else:
             email_msg = '<div class="msg msg-confirm"><p class="copy">Your email was successfully queued for sending.</p></div>'
 
+    elif "Show Background Email Task History" in action:
+        message, datatable = get_background_task_table(course_id, task_type='bulk_course_email')
+        msg += message
+
     #----------------------------------------
     # psychometrics
 
@@ -877,6 +870,7 @@ def instructor_dashboard(request, course_id):
 
     return render_to_response('courseware/instructor_dashboard.html', context)
 
+
 def _do_remote_gradebook(user, course, action, args=None, files=None):
     '''
     Perform remote gradebook action.  Returns msg, datatable.
@@ -1527,7 +1521,7 @@ def dump_grading_context(course):
     return msg
 
 
-def get_background_task_table(course_id, problem_url, student=None):
+def get_background_task_table(course_id, problem_url=None, student=None, task_type=None):
     """
     Construct the "datatable" structure to represent background task history.
 
@@ -1538,14 +1532,16 @@ def get_background_task_table(course_id, problem_url, student=None):
     Returns a tuple of (msg, datatable), where the msg is a possible error message,
     and the datatable is the datatable to be used for display.
     """
-    history_entries = get_instructor_task_history(course_id, problem_url, student)
+    history_entries = get_instructor_task_history(course_id, problem_url, student, task_type)
     datatable = {}
     msg = ""
     # first check to see if there is any history at all
     # (note that we don't have to check that the arguments are valid; it
     # just won't find any entries.)
     if (history_entries.count()) == 0:
-        if student is not None:
+        if problem_url is None:
+            msg += '<font color="red">Failed to find any background tasks for course "{course}".</font>'.format(course=course_id)
+        elif student is not None:
             template = '<font color="red">Failed to find any background tasks for course "{course}", module "{problem}" and student "{student}".</font>'
             msg += template.format(course=course_id, problem=problem_url, student=student.username)
         else:
@@ -1582,7 +1578,9 @@ def get_background_task_table(course_id, problem_url, student=None):
                    task_message]
             datatable['data'].append(row)
 
-        if student is not None:
+        if problem_url is None:
+            datatable['title'] = "{course_id}".format(course_id=course_id)
+        elif student is not None:
             datatable['title'] = "{course_id} > {location} > {student}".format(course_id=course_id,
                                                                                location=problem_url,
                                                                                student=student.username)

lms/djangoapps/instructor_task/api.py

@@ -6,6 +6,7 @@ already been submitted, filtered either by running state or input
 arguments.
 
 """
+import hashlib
 
 from celery.states import READY_STATES
 
@@ -14,11 +15,13 @@ from xmodule.modulestore.django import modulestore
 from instructor_task.models import InstructorTask
 from instructor_task.tasks import (rescore_problem,
                                    reset_problem_attempts,
-                                   delete_problem_state)
+                                   delete_problem_state,
+                                   send_bulk_course_email)
 
 from instructor_task.api_helper import (check_arguments_for_rescoring,
                                         encode_problem_and_student_input,
                                         submit_task)
+from bulk_email.models import CourseEmail
 
 
 def get_running_instructor_tasks(course_id):
@@ -34,14 +37,18 @@ def get_running_instructor_tasks(course_id):
     return instructor_tasks.order_by('-id')
 
 
-def get_instructor_task_history(course_id, problem_url, student=None):
+def get_instructor_task_history(course_id, problem_url=None, student=None, task_type=None):
     """
     Returns a query of InstructorTask objects of historical tasks for a given course,
-    that match a particular problem and optionally a student.
+    that optionally match a particular problem, a student, and/or a task type.
     """
-    _, task_key = encode_problem_and_student_input(problem_url, student)
+    instructor_tasks = InstructorTask.objects.filter(course_id=course_id)
+    if problem_url is not None or student is not None:
+        _, task_key = encode_problem_and_student_input(problem_url, student)
+        instructor_tasks = instructor_tasks.filter(task_key=task_key)
+    if task_type is not None:
+        instructor_tasks = instructor_tasks.filter(task_type=task_type)
 
-    instructor_tasks = InstructorTask.objects.filter(course_id=course_id, task_key=task_key)
     return instructor_tasks.order_by('-id')
 
 
@@ -162,3 +169,40 @@ def submit_delete_problem_state_for_all_students(request, course_id, problem_url
     task_class = delete_problem_state
     task_input, task_key = encode_problem_and_student_input(problem_url)
     return submit_task(request, task_type, task_class, course_id, task_input, task_key)
+
+
+def submit_bulk_course_email(request, course_id, email_id):
+    """
+    Request to have bulk email sent as a background task.
+
+    The specified CourseEmail object will be sent be updated for all students who have enrolled
+    in a course.  Parameters are the `course_id` and the `email_id`, the id of the CourseEmail object.
+
+    AlreadyRunningError is raised if the same recipients are already being emailed with the same
+    CourseEmail object.
+
+    This method makes sure the InstructorTask 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.
+    """
+    # Assume that the course is defined, and that the user has already been verified to have
+    # appropriate access to the course. But make sure that the email exists.
+    # We also pull out the To argument here, so that is displayed in
+    # the InstructorTask status.
+    email_obj = CourseEmail.objects.get(id=email_id)
+    to_option = email_obj.to_option
+
+    task_type = 'bulk_course_email'
+    task_class = send_bulk_course_email
+    # Pass in the to_option as a separate argument, even though it's (currently)
+    # in the CourseEmail.  That way it's visible in the progress status.
+    # (At some point in the future, we might take the recipient out of the CourseEmail,
+    # so that the same saved email can be sent to different recipients, as it is tested.)
+    task_input = {'email_id': email_id, 'to_option': to_option}
+    task_key_stub = "{email_id}_{to_option}".format(email_id=email_id, to_option=to_option)
+    # create the key value by using MD5 hash:
+    task_key = hashlib.md5(task_key_stub).hexdigest()
+    return submit_task(request, task_type, task_class, course_id, task_input, task_key)

lms/djangoapps/instructor_task/api_helper.py

@@ -58,13 +58,14 @@ def _reserve_task(course_id, task_type, task_key, task_input, requester):
     return InstructorTask.create(course_id, task_type, task_key, task_input, requester)
 
 
-def _get_xmodule_instance_args(request):
+def _get_xmodule_instance_args(request, task_id):
     """
     Calculate parameters needed for instantiating xmodule instances.
 
     The `request_info` will be passed to a tracking log function, to provide information
     about the source of the task request.   The `xqueue_callback_url_prefix` is used to
     permit old-style xqueue callbacks directly to the appropriate module in the LMS.
+    The `task_id` is also passed to the tracking log function.
     """
     request_info = {'username': request.user.username,
                     'ip': request.META['REMOTE_ADDR'],
@@ -74,6 +75,7 @@ def _get_xmodule_instance_args(request):
 
     xmodule_instance_args = {'xqueue_callback_url_prefix': get_xqueue_callback_url_prefix(request),
                              'request_info': request_info,
+                             'task_id': task_id,
                              }
     return xmodule_instance_args
 
@@ -88,10 +90,16 @@ def _update_instructor_task(instructor_task, task_result):
     is usually not saved.  In general, tasks that have finished (either with
     success or failure) should have their entries updated by the task itself,
     so are not updated here.  Tasks that are still running are not updated
-    while they run.  So the one exception to the no-save rule are tasks that
+    and saved while they run.  The one exception to the no-save rule are tasks that
     are in a "revoked" state.  This may mean that the task never had the
     opportunity to update the InstructorTask entry.
 
+    Tasks that are in progress and have subtasks doing the processing do not look
+    to the task's AsyncResult object.  When subtasks are running, the
+    InstructorTask object itself is updated with the subtasks' progress,
+    not any AsyncResult object.  In this case, the InstructorTask is
+    not updated at all.
+
     Calculates json to store in "task_output" field of the `instructor_task`,
     as well as updating the task_state.
 
@@ -108,11 +116,21 @@ def _update_instructor_task(instructor_task, task_result):
     returned_result = task_result.result
     result_traceback = task_result.traceback
 
-    # Assume we don't always update the InstructorTask entry if we don't have to:
+    # Assume we don't always save the InstructorTask entry if we don't have to,
+    # but that in most cases we will update the InstructorTask in-place with its
+    # current progress.
+    entry_needs_updating = True
     entry_needs_saving = False
     task_output = None
 
-    if result_state in [PROGRESS, SUCCESS]:
+    if instructor_task.task_state == PROGRESS and len(instructor_task.subtasks) > 0:
+        # This happens when running subtasks:  the result object is marked with SUCCESS,
+        # meaning that the subtasks have successfully been defined.  However, the InstructorTask
+        # will be marked as in PROGRESS, until the last subtask completes and marks it as SUCCESS.
+        # We want to ignore the parent SUCCESS if subtasks are still running, and just trust the
+        # contents of the InstructorTask.
+        entry_needs_updating = False
+    elif result_state in [PROGRESS, SUCCESS]:
         # construct a status message directly from the task result's result:
         # it needs to go back with the entry passed in.
         log.info("background task (%s), state %s:  result: %s", task_id, result_state, returned_result)
@@ -134,12 +152,13 @@ def _update_instructor_task(instructor_task, task_result):
     # save progress and state into the entry, even if it's not being saved:
     # when celery is run in "ALWAYS_EAGER" mode, progress needs to go back
     # with the entry passed in.
-    instructor_task.task_state = result_state
-    if task_output is not None:
-        instructor_task.task_output = task_output
+    if entry_needs_updating:
+        instructor_task.task_state = result_state
+        if task_output is not None:
+            instructor_task.task_output = task_output
 
-    if entry_needs_saving:
-        instructor_task.save()
+        if entry_needs_saving:
+            instructor_task.save()
 
 
 def get_updated_instructor_task(task_id):
@@ -175,7 +194,7 @@ def get_status_from_instructor_task(instructor_task):
       'in_progress': boolean indicating if task is still running.
       'task_progress': dict containing progress information.  This includes:
           'attempted': number of attempts made
-          'updated': number of attempts that "succeeded"
+          'succeeded': number of attempts that "succeeded"
           'total': number of possible subtasks to attempt
           'action_name': user-visible verb to use in status messages.  Should be past-tense.
           'duration_ms': how long the task has (or had) been running.
@@ -214,7 +233,7 @@ def check_arguments_for_rescoring(course_id, problem_url):
 
 def encode_problem_and_student_input(problem_url, student=None):
     """
-    Encode problem_url and optional student into task_key and task_input values.
+    Encode optional problem_url and optional student into task_key and task_input values.
 
     `problem_url` is full URL of the problem.
     `student` is the user object of the student
@@ -257,7 +276,7 @@ def submit_task(request, task_type, task_class, course_id, task_input, task_key)
 
     # submit task:
     task_id = instructor_task.task_id
-    task_args = [instructor_task.id, _get_xmodule_instance_args(request)]
+    task_args = [instructor_task.id, _get_xmodule_instance_args(request, task_id)]  # pylint: disable=E1101
     task_class.apply_async(task_args, task_id=task_id)
 
     return instructor_task

lms/djangoapps/instructor_task/migrations/0002_add_subtask_field.py

+# -*- 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 field 'InstructorTask.subtasks'
+        db.add_column('instructor_task_instructortask', 'subtasks',
+                      self.gf('django.db.models.fields.TextField')(default='', blank=True),
+                      keep_default=False)
+
+
+    def backwards(self, orm):
+        # Deleting field 'InstructorTask.subtasks'
+        db.delete_column('instructor_task_instructortask', 'subtasks')
+
+
+    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'})
+        },
+        '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'})
+        },
+        'instructor_task.instructortask': {
+            'Meta': {'object_name': 'InstructorTask'},
+            'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'null': 'True', 'blank': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'requester': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}),
+            'subtasks': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
+            'task_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'task_input': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
+            'task_key': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'task_output': ('django.db.models.fields.CharField', [], {'max_length': '1024', 'null': 'True'}),
+            'task_state': ('django.db.models.fields.CharField', [], {'max_length': '50', 'null': 'True', 'db_index': 'True'}),
+            'task_type': ('django.db.models.fields.CharField', [], {'max_length': '50', 'db_index': 'True'}),
+            'updated': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'blank': 'True'})
+        }
+    }
+
+    complete_apps = ['instructor_task']
\ No newline at end of file

lms/djangoapps/instructor_task/models.py

@@ -56,6 +56,7 @@ class InstructorTask(models.Model):
     requester = models.ForeignKey(User, db_index=True)
     created = models.DateTimeField(auto_now_add=True, null=True)
     updated = models.DateTimeField(auto_now=True)
+    subtasks = models.TextField(blank=True)  # JSON dictionary
 
     def __repr__(self):
         return 'InstructorTask<%r>' % ({

lms/djangoapps/instructor_task/tasks.py

@@ -19,14 +19,21 @@ a problem URL and optionally a student.  These are used to set up the initial va
 of the query for traversing StudentModule objects.
 
 """
+from django.utils.translation import ugettext_noop
 from celery import task
-from instructor_task.tasks_helper import (update_problem_module_state,
-                                          rescore_problem_module_state,
-                                          reset_attempts_module_state,
-                                          delete_problem_module_state)
-
-
-@task
+from functools import partial
+from instructor_task.tasks_helper import (
+    run_main_task,
+    BaseInstructorTask,
+    perform_module_state_update,
+    rescore_problem_module_state,
+    reset_attempts_module_state,
+    delete_problem_module_state,
+)
+from bulk_email.tasks import perform_delegate_email_batches
+
+
+@task(base=BaseInstructorTask)  # pylint: disable=E1102
 def rescore_problem(entry_id, xmodule_instance_args):
     """Rescores a problem in a course, for all students or one specific student.
 
@@ -45,15 +52,19 @@ def rescore_problem(entry_id, xmodule_instance_args):
     `xmodule_instance_args` provides information needed by _get_module_instance_for_task()
     to instantiate an xmodule instance.
     """
-    action_name = 'rescored'
-    update_fcn = rescore_problem_module_state
-    filter_fcn = lambda(modules_to_update): modules_to_update.filter(state__contains='"done": true')
-    return update_problem_module_state(entry_id,
-                                       update_fcn, action_name, filter_fcn=filter_fcn,
-                                       xmodule_instance_args=xmodule_instance_args)
+    # Translators: This is a past-tense verb that is inserted into task progress messages as {action}.
+    action_name = ugettext_noop('rescored')
+    update_fcn = partial(rescore_problem_module_state, xmodule_instance_args)
+
+    def filter_fcn(modules_to_update):
+        """Filter that matches problems which are marked as being done"""
+        return modules_to_update.filter(state__contains='"done": true')
+
+    visit_fcn = partial(perform_module_state_update, update_fcn, filter_fcn)
+    return run_main_task(entry_id, visit_fcn, action_name)
 
 
-@task
+@task(base=BaseInstructorTask)  # pylint: disable=E1102
 def reset_problem_attempts(entry_id, xmodule_instance_args):
     """Resets problem attempts to zero for a particular problem for all students in a course.
 
@@ -68,14 +79,14 @@ def reset_problem_attempts(entry_id, xmodule_instance_args):
     `xmodule_instance_args` provides information needed by _get_module_instance_for_task()
     to instantiate an xmodule instance.
     """
-    action_name = 'reset'
-    update_fcn = reset_attempts_module_state
-    return update_problem_module_state(entry_id,
-                                       update_fcn, action_name, filter_fcn=None,
-                                       xmodule_instance_args=xmodule_instance_args)
+    # Translators: This is a past-tense verb that is inserted into task progress messages as {action}.
+    action_name = ugettext_noop('reset')
+    update_fcn = partial(reset_attempts_module_state, xmodule_instance_args)
+    visit_fcn = partial(perform_module_state_update, update_fcn, None)
+    return run_main_task(entry_id, visit_fcn, action_name)
 
 
-@task
+@task(base=BaseInstructorTask)  # pylint: disable=E1102
 def delete_problem_state(entry_id, xmodule_instance_args):
     """Deletes problem state entirely for all students on a particular problem in a course.
 
@@ -90,8 +101,29 @@ def delete_problem_state(entry_id, xmodule_instance_args):
     `xmodule_instance_args` provides information needed by _get_module_instance_for_task()
     to instantiate an xmodule instance.
     """
-    action_name = 'deleted'
-    update_fcn = delete_problem_module_state
-    return update_problem_module_state(entry_id,
-                                       update_fcn, action_name, filter_fcn=None,
-                                       xmodule_instance_args=xmodule_instance_args)
+    # Translators: This is a past-tense verb that is inserted into task progress messages as {action}.
+    action_name = ugettext_noop('deleted')
+    update_fcn = partial(delete_problem_module_state, xmodule_instance_args)
+    visit_fcn = partial(perform_module_state_update, update_fcn, None)
+    return run_main_task(entry_id, visit_fcn, action_name)
+
+
+@task(base=BaseInstructorTask)  # pylint: disable=E1102
+def send_bulk_course_email(entry_id, _xmodule_instance_args):
+    """Sends emails to recipients enrolled in a course.
+
+    `entry_id` is the id value of the InstructorTask entry that corresponds to this task.
+    The entry contains the `course_id` that identifies the course, as well as the
+    `task_input`, which contains task-specific input.
+
+    The task_input should be a dict with the following entries:
+
+      'email_id': the full URL to the problem to be rescored.  (required)
+
+    `_xmodule_instance_args` provides information needed by _get_module_instance_for_task()
+    to instantiate an xmodule instance.  This is unused here.
+    """
+    # Translators: This is a past-tense verb that is inserted into task progress messages as {action}.
+    action_name = ugettext_noop('emailed')
+    visit_fcn = perform_delegate_email_batches
+    return run_main_task(entry_id, visit_fcn, action_name)

lms/djangoapps/instructor_task/tests/test_api.py

@@ -6,16 +6,21 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 
 from courseware.tests.factories import UserFactory
 
-from instructor_task.api import (get_running_instructor_tasks,
-                                 get_instructor_task_history,
-                                 submit_rescore_problem_for_all_students,
-                                 submit_rescore_problem_for_student,
-                                 submit_reset_problem_attempts_for_all_students,
-                                 submit_delete_problem_state_for_all_students)
+from bulk_email.models import CourseEmail, SEND_TO_ALL
+from instructor_task.api import (
+    get_running_instructor_tasks,
+    get_instructor_task_history,
+    submit_rescore_problem_for_all_students,
+    submit_rescore_problem_for_student,
+    submit_reset_problem_attempts_for_all_students,
+    submit_delete_problem_state_for_all_students,
+    submit_bulk_course_email,
+)
 
 from instructor_task.api_helper import AlreadyRunningError
 from instructor_task.models import InstructorTask, PROGRESS
 from instructor_task.tests.test_base import (InstructorTaskTestCase,
+                                             InstructorTaskCourseTestCase,
                                              InstructorTaskModuleTestCase,
                                              TEST_COURSE_ID)
 
@@ -42,12 +47,28 @@ class InstructorTaskReportTest(InstructorTaskTestCase):
             expected_ids.append(self._create_success_entry().task_id)
             expected_ids.append(self._create_progress_entry().task_id)
         task_ids = [instructor_task.task_id for instructor_task
-                    in get_instructor_task_history(TEST_COURSE_ID, self.problem_url)]
+                    in get_instructor_task_history(TEST_COURSE_ID, problem_url=self.problem_url)]
         self.assertEquals(set(task_ids), set(expected_ids))
+        # make the same call using explicit task_type:
+        task_ids = [instructor_task.task_id for instructor_task
+                    in get_instructor_task_history(
+                        TEST_COURSE_ID,
+                        problem_url=self.problem_url,
+                        task_type='rescore_problem'
+                    )]
+        self.assertEquals(set(task_ids), set(expected_ids))
+        # make the same call using a non-existent task_type:
+        task_ids = [instructor_task.task_id for instructor_task
+                    in get_instructor_task_history(
+                        TEST_COURSE_ID,
+                        problem_url=self.problem_url,
+                        task_type='dummy_type'
+                    )]
+        self.assertEquals(set(task_ids), set())
 
 
-class InstructorTaskSubmitTest(InstructorTaskModuleTestCase):
-    """Tests API methods that involve the submission of background tasks."""
+class InstructorTaskModuleSubmitTest(InstructorTaskModuleTestCase):
+    """Tests API methods that involve the submission of module-based background tasks."""
 
     def setUp(self):
         self.initialize_course()
@@ -136,3 +157,29 @@ class InstructorTaskSubmitTest(InstructorTaskModuleTestCase):
 
     def test_submit_delete_all(self):
         self._test_submit_task(submit_delete_problem_state_for_all_students)
+
+
+class InstructorTaskCourseSubmitTest(InstructorTaskCourseTestCase):
+    """Tests API methods that involve the submission of course-based background tasks."""
+
+    def setUp(self):
+        self.initialize_course()
+        self.student = UserFactory.create(username="student", email="student@edx.org")
+        self.instructor = UserFactory.create(username="instructor", email="instructor@edx.org")
+
+    def _define_course_email(self):
+        """Create CourseEmail object for testing."""
+        course_email = CourseEmail.create(self.course.id, self.instructor, SEND_TO_ALL, "Test Subject", "<p>This is a test message</p>")
+        return course_email.id  # pylint: disable=E1101
+
+    def test_submit_bulk_email_all(self):
+        email_id = self._define_course_email()
+        instructor_task = submit_bulk_course_email(self.create_task_request(self.instructor), self.course.id, email_id)
+
+        # test resubmitting, by updating the existing record:
+        instructor_task = InstructorTask.objects.get(id=instructor_task.id)  # pylint: disable=E1101
+        instructor_task.task_state = PROGRESS
+        instructor_task.save()
+
+        with self.assertRaises(AlreadyRunningError):
+            instructor_task = submit_bulk_course_email(self.create_task_request(self.instructor), self.course.id, email_id)

lms/djangoapps/instructor_task/tests/test_base.py

@@ -88,7 +88,7 @@ class InstructorTaskTestCase(TestCase):
     def _create_progress_entry(self, student=None, task_state=PROGRESS):
         """Creates a InstructorTask entry representing a task in progress."""
         progress = {'attempted': 3,
-                    'updated': 2,
+                    'succeeded': 2,
                     'total': 5,
                     'action_name': 'rescored',
                     }
@@ -96,10 +96,10 @@ class InstructorTaskTestCase(TestCase):
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
-class InstructorTaskModuleTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase):
+class InstructorTaskCourseTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase):
     """
     Base test class for InstructorTask-related tests that require
-    the setup of a course and problem in order to access StudentModule state.
+    the setup of a course.
     """
     course = None
     current_user = None
@@ -120,6 +120,7 @@ class InstructorTaskModuleTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase)
         # add a sequence to the course to which the problems can be added
         self.problem_section = ItemFactory.create(parent_location=chapter.location,
                                                   category='sequential',
+                                                  metadata={'graded': True, 'format': 'Homework'},
                                                   display_name=TEST_SECTION_NAME)
 
     @staticmethod
@@ -130,12 +131,12 @@ class InstructorTaskModuleTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase)
     def login_username(self, username):
         """Login the user, given the `username`."""
         if self.current_user != username:
-            self.login(InstructorTaskModuleTestCase.get_user_email(username), "test")
+            self.login(InstructorTaskCourseTestCase.get_user_email(username), "test")
             self.current_user = username
 
     def _create_user(self, username, is_staff=False):
         """Creates a user and enrolls them in the test course."""
-        email = InstructorTaskModuleTestCase.get_user_email(username)
+        email = InstructorTaskCourseTestCase.get_user_email(username)
         thisuser = UserFactory.create(username=username, email=email, is_staff=is_staff)
         CourseEnrollmentFactory.create(user=thisuser, course_id=self.course.id)
         return thisuser
@@ -149,6 +150,31 @@ class InstructorTaskModuleTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase)
         return self._create_user(username, is_staff=False)
 
     @staticmethod
+    def get_task_status(task_id):
+        """Use api method to fetch task status, using mock request."""
+        mock_request = Mock()
+        mock_request.REQUEST = {'task_id': task_id}
+        response = instructor_task_status(mock_request)
+        status = json.loads(response.content)
+        return status
+
+    def create_task_request(self, requester_username):
+        """Generate request that can be used for submitting tasks"""
+        request = Mock()
+        request.user = User.objects.get(username=requester_username)
+        request.get_host = Mock(return_value="testhost")
+        request.META = {'REMOTE_ADDR': '0:0:0:0', 'SERVER_NAME': 'testhost'}
+        request.is_secure = Mock(return_value=False)
+        return request
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class InstructorTaskModuleTestCase(InstructorTaskCourseTestCase):
+    """
+    Base test class for InstructorTask-related tests that require
+    the setup of a course and problem in order to access StudentModule state.
+    """
+    @staticmethod
     def problem_location(problem_url_name):
         """
         Create an internal location for a test problem.
@@ -191,21 +217,3 @@ class InstructorTaskModuleTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase)
                                          module_type=descriptor.location.category,
                                          module_state_key=descriptor.location.url(),
                                          )
-
-    @staticmethod
-    def get_task_status(task_id):
-        """Use api method to fetch task status, using mock request."""
-        mock_request = Mock()
-        mock_request.REQUEST = {'task_id': task_id}
-        response = instructor_task_status(mock_request)
-        status = json.loads(response.content)
-        return status
-
-    def create_task_request(self, requester_username):
-        """Generate request that can be used for submitting tasks"""
-        request = Mock()
-        request.user = User.objects.get(username=requester_username)
-        request.get_host = Mock(return_value="testhost")
-        request.META = {'REMOTE_ADDR': '0:0:0:0', 'SERVER_NAME': 'testhost'}
-        request.is_secure = Mock(return_value=False)
-        return request

lms/djangoapps/instructor_task/tests/test_integration.py

@@ -227,7 +227,7 @@ class TestRescoringTask(TestIntegrationTask):
         self.assertEqual(task_input['problem_url'], InstructorTaskModuleTestCase.problem_location(problem_url_name))
         status = json.loads(instructor_task.task_output)
         self.assertEqual(status['attempted'], 1)
-        self.assertEqual(status['updated'], 0)
+        self.assertEqual(status['succeeded'], 0)
         self.assertEqual(status['total'], 1)
 
     def define_code_response_problem(self, problem_url_name):

lms/djangoapps/instructor_task/views.py

@@ -3,6 +3,7 @@ import json
 import logging
 
 from django.http import HttpResponse
+from django.utils.translation import ugettext as _
 
 from celery.states import FAILURE, REVOKED, READY_STATES
 
@@ -40,7 +41,7 @@ def instructor_task_status(request):
 
     Status is returned as a JSON-serialized dict, wrapped as the content of a HTTPResponse.
 
-    The task_id can be specified to this view in one of three ways:
+    The task_id can be specified to this view in one of two ways:
 
     * by making a request containing 'task_id' as a parameter with a single value
       Returns a dict containing status information for the specified task_id
@@ -65,7 +66,7 @@ def instructor_task_status(request):
       'in_progress': boolean indicating if task is still running.
       'task_progress': dict containing progress information.  This includes:
           'attempted': number of attempts made
-          'updated': number of attempts that "succeeded"
+          'succeeded': number of attempts that "succeeded"
           'total': number of possible subtasks to attempt
           'action_name': user-visible verb to use in status messages.  Should be past-tense.
           'duration_ms': how long the task has (or had) been running.
@@ -105,68 +106,118 @@ def get_task_completion_info(instructor_task):
     succeeded = False
 
     if instructor_task.task_state not in STATES_WITH_STATUS:
-        return (succeeded, "No status information available")
+        return (succeeded, _("No status information available"))
 
     # we're more surprised if there is no output for a completed task, but just warn:
     if instructor_task.task_output is None:
-        log.warning("No task_output information found for instructor_task {0}".format(instructor_task.task_id))
-        return (succeeded, "No status information available")
+        log.warning(_("No task_output information found for instructor_task {0}").format(instructor_task.task_id))
+        return (succeeded, _("No status information available"))
 
     try:
         task_output = json.loads(instructor_task.task_output)
     except ValueError:
-        fmt = "No parsable task_output information found for instructor_task {0}: {1}"
+        fmt = _("No parsable task_output information found for instructor_task {0}: {1}")
         log.warning(fmt.format(instructor_task.task_id, instructor_task.task_output))
-        return (succeeded, "No parsable status information available")
+        return (succeeded, _("No parsable status information available"))
 
     if instructor_task.task_state in [FAILURE, REVOKED]:
-        return (succeeded, task_output.get('message', 'No message provided'))
+        return (succeeded, task_output.get('message', _('No message provided')))
 
-    if any([key not in task_output for key in ['action_name', 'attempted', 'updated', 'total']]):
-        fmt = "Invalid task_output information found for instructor_task {0}: {1}"
+    if any([key not in task_output for key in ['action_name', 'attempted', 'total']]):
+        fmt = _("Invalid task_output information found for instructor_task {0}: {1}")
         log.warning(fmt.format(instructor_task.task_id, instructor_task.task_output))
-        return (succeeded, "No progress status information available")
+        return (succeeded, _("No progress status information available"))
 
-    action_name = task_output['action_name']
+    action_name = _(task_output['action_name'])
     num_attempted = task_output['attempted']
-    num_updated = task_output['updated']
     num_total = task_output['total']
 
+    # In earlier versions of this code, the key 'updated' was used instead of
+    # (the more general) 'succeeded'.  In order to support history that may contain
+    # output with the old key, we check for values with both the old and the current
+    # key, and simply sum them.
+    num_succeeded = task_output.get('updated', 0) + task_output.get('succeeded', 0)
+    num_skipped = task_output.get('skipped', 0)
+
     student = None
+    problem_url = None
+    email_id = None
     try:
         task_input = json.loads(instructor_task.task_input)
     except ValueError:
-        fmt = "No parsable task_input information found for instructor_task {0}: {1}"
+        fmt = _("No parsable task_input information found for instructor_task {0}: {1}")
         log.warning(fmt.format(instructor_task.task_id, instructor_task.task_input))
     else:
         student = task_input.get('student')
+        problem_url = task_input.get('problem_url')
+        email_id = task_input.get('email_id')
 
     if instructor_task.task_state == PROGRESS:
         # special message for providing progress updates:
-        msg_format = "Progress: {action} {updated} of {attempted} so far"
-    elif student is not None:
+        # Translators: {action} is a past-tense verb that is localized separately. {attempted} and {succeeded} are counts.
+        msg_format = _("Progress: {action} {succeeded} of {attempted} so far")
+    elif student is not None and problem_url is not None:
+        # this reports on actions on problems for a particular student:
         if num_attempted == 0:
-            msg_format = "Unable to find submission to be {action} for student '{student}'"
-        elif num_updated == 0:
-            msg_format = "Problem failed to be {action} for student '{student}'"
+            # Translators: {action} is a past-tense verb that is localized separately. {student} is a student identifier.
+            msg_format = _("Unable to find submission to be {action} for student '{student}'")
+        elif num_succeeded == 0:
+            # Translators: {action} is a past-tense verb that is localized separately. {student} is a student identifier.
+            msg_format = _("Problem failed to be {action} for student '{student}'")
         else:
             succeeded = True
-            msg_format = "Problem successfully {action} for student '{student}'"
-    elif num_attempted == 0:
-        msg_format = "Unable to find any students with submissions to be {action}"
-    elif num_updated == 0:
-        msg_format = "Problem failed to be {action} for any of {attempted} students"
-    elif num_updated == num_attempted:
-        succeeded = True
-        msg_format = "Problem successfully {action} for {attempted} students"
-    else:  # num_updated < num_attempted
-        msg_format = "Problem {action} for {updated} of {attempted} students"
+            # Translators: {action} is a past-tense verb that is localized separately. {student} is a student identifier.
+            msg_format = _("Problem successfully {action} for student '{student}'")
+    elif student is None and problem_url is not None:
+        # this reports on actions on problems for all students:
+        if num_attempted == 0:
+            # Translators: {action} is a past-tense verb that is localized separately.
+            msg_format = _("Unable to find any students with submissions to be {action}")
+        elif num_succeeded == 0:
+            # Translators: {action} is a past-tense verb that is localized separately. {attempted} is a count.
+            msg_format = _("Problem failed to be {action} for any of {attempted} students")
+        elif num_succeeded == num_attempted:
+            succeeded = True
+            # Translators: {action} is a past-tense verb that is localized separately. {attempted} is a count.
+            msg_format = _("Problem successfully {action} for {attempted} students")
+        else:  # num_succeeded < num_attempted
+            # Translators: {action} is a past-tense verb that is localized separately. {succeeded} and {attempted} are counts.
+            msg_format = _("Problem {action} for {succeeded} of {attempted} students")
+    elif email_id is not None:
+        # this reports on actions on bulk emails
+        if num_attempted == 0:
+            # Translators: {action} is a past-tense verb that is localized separately.
+            msg_format = _("Unable to find any recipients to be {action}")
+        elif num_succeeded == 0:
+            # Translators: {action} is a past-tense verb that is localized separately. {attempted} is a count.
+            msg_format = _("Message failed to be {action} for any of {attempted} recipients ")
+        elif num_succeeded == num_attempted:
+            succeeded = True
+            # Translators: {action} is a past-tense verb that is localized separately. {attempted} is a count.
+            msg_format = _("Message successfully {action} for {attempted} recipients")
+        else:  # num_succeeded < num_attempted
+            # Translators: {action} is a past-tense verb that is localized separately. {succeeded} and {attempted} are counts.
+            msg_format = _("Message {action} for {succeeded} of {attempted} recipients")
+    else:
+        # provide a default:
+        # Translators: {action} is a past-tense verb that is localized separately. {succeeded} and {attempted} are counts.
+        msg_format = _("Status: {action} {succeeded} of {attempted}")
+
+    if num_skipped > 0:
+        # Translators: {skipped} is a count.  This message is appended to task progress status messages.
+        msg_format += _(" (skipping {skipped})")
 
     if student is None and num_attempted != num_total:
-        msg_format += " (out of {total})"
+        # Translators: {total} is a count.  This message is appended to task progress status messages.
+        msg_format += _(" (out of {total})")
 
     # Update status in task result object itself:
-    message = msg_format.format(action=action_name, updated=num_updated,
-                                attempted=num_attempted, total=num_total,
-                                student=student)
+    message = msg_format.format(
+        action=action_name,
+        succeeded=num_succeeded,
+        attempted=num_attempted,
+        total=num_total,
+        skipped=num_skipped,
+        student=student
+    )
     return (succeeded, message)

lms/envs/aws.py

@@ -109,8 +109,6 @@ EMAIL_FILE_PATH = ENV_TOKENS.get('EMAIL_FILE_PATH', None)
 EMAIL_HOST = ENV_TOKENS.get('EMAIL_HOST', 'localhost')  # django default is localhost
 EMAIL_PORT = ENV_TOKENS.get('EMAIL_PORT', 25)  # django default is 25
 EMAIL_USE_TLS = ENV_TOKENS.get('EMAIL_USE_TLS', False)  # django default is False
-EMAILS_PER_TASK = ENV_TOKENS.get('EMAILS_PER_TASK', 100)
-EMAILS_PER_QUERY = ENV_TOKENS.get('EMAILS_PER_QUERY', 1000)
 SITE_NAME = ENV_TOKENS['SITE_NAME']
 SESSION_ENGINE = ENV_TOKENS.get('SESSION_ENGINE', SESSION_ENGINE)
 SESSION_COOKIE_DOMAIN = ENV_TOKENS.get('SESSION_COOKIE_DOMAIN')
@@ -130,10 +128,9 @@ LOG_DIR = ENV_TOKENS['LOG_DIR']
 
 CACHES = ENV_TOKENS['CACHES']
 
-#Email overrides
+# Email overrides
 DEFAULT_FROM_EMAIL = ENV_TOKENS.get('DEFAULT_FROM_EMAIL', DEFAULT_FROM_EMAIL)
 DEFAULT_FEEDBACK_EMAIL = ENV_TOKENS.get('DEFAULT_FEEDBACK_EMAIL', DEFAULT_FEEDBACK_EMAIL)
-DEFAULT_BULK_FROM_EMAIL = ENV_TOKENS.get('DEFAULT_BULK_FROM_EMAIL', DEFAULT_BULK_FROM_EMAIL)
 ADMINS = ENV_TOKENS.get('ADMINS', ADMINS)
 SERVER_EMAIL = ENV_TOKENS.get('SERVER_EMAIL', SERVER_EMAIL)
 TECH_SUPPORT_EMAIL = ENV_TOKENS.get('TECH_SUPPORT_EMAIL', TECH_SUPPORT_EMAIL)
@@ -143,7 +140,21 @@ PAYMENT_SUPPORT_EMAIL = ENV_TOKENS.get('PAYMENT_SUPPORT_EMAIL', PAYMENT_SUPPORT_
 PAID_COURSE_REGISTRATION_CURRENCY = ENV_TOKENS.get('PAID_COURSE_REGISTRATION_CURRENCY',
                                                    PAID_COURSE_REGISTRATION_CURRENCY)
 
-#Theme overrides
+# Bulk Email overrides
+BULK_EMAIL_DEFAULT_FROM_EMAIL = ENV_TOKENS.get('BULK_EMAIL_DEFAULT_FROM_EMAIL', BULK_EMAIL_DEFAULT_FROM_EMAIL)
+BULK_EMAIL_EMAILS_PER_TASK = ENV_TOKENS.get('BULK_EMAIL_EMAILS_PER_TASK', BULK_EMAIL_EMAILS_PER_TASK)
+BULK_EMAIL_EMAILS_PER_QUERY = ENV_TOKENS.get('BULK_EMAIL_EMAILS_PER_QUERY', BULK_EMAIL_EMAILS_PER_QUERY)
+BULK_EMAIL_DEFAULT_RETRY_DELAY = ENV_TOKENS.get('BULK_EMAIL_DEFAULT_RETRY_DELAY', BULK_EMAIL_DEFAULT_RETRY_DELAY)
+BULK_EMAIL_MAX_RETRIES = ENV_TOKENS.get('BULK_EMAIL_MAX_RETRIES', BULK_EMAIL_MAX_RETRIES)
+BULK_EMAIL_INFINITE_RETRY_CAP = ENV_TOKENS.get('BULK_EMAIL_INFINITE_RETRY_CAP', BULK_EMAIL_INFINITE_RETRY_CAP)
+BULK_EMAIL_LOG_SENT_EMAILS = ENV_TOKENS.get('BULK_EMAIL_LOG_SENT_EMAILS', BULK_EMAIL_LOG_SENT_EMAILS)
+BULK_EMAIL_RETRY_DELAY_BETWEEN_SENDS = ENV_TOKENS.get('BULK_EMAIL_RETRY_DELAY_BETWEEN_SENDS', BULK_EMAIL_RETRY_DELAY_BETWEEN_SENDS)
+# We want Bulk Email running on the high-priority queue, so we define the
+# routing key that points to it.  At the moment, the name is the same.
+# We have to reset the value here, since we have changed the value of the queue name.
+BULK_EMAIL_ROUTING_KEY = HIGH_PRIORITY_QUEUE
+
+# Theme overrides
 THEME_NAME = ENV_TOKENS.get('THEME_NAME', None)
 if not THEME_NAME is None:
     enable_theme(THEME_NAME)
@@ -152,10 +163,10 @@ if not THEME_NAME is None:
 # Marketing link overrides
 MKTG_URL_LINK_MAP.update(ENV_TOKENS.get('MKTG_URL_LINK_MAP', {}))
 
-#Timezone overrides
+# Timezone overrides
 TIME_ZONE = ENV_TOKENS.get('TIME_ZONE', TIME_ZONE)
 
-#Additional installed apps
+# Additional installed apps
 for app in ENV_TOKENS.get('ADDL_INSTALLED_APPS', []):
     INSTALLED_APPS += (app,)
 

lms/envs/common.py

@@ -114,6 +114,7 @@ MITX_FEATURES = {
     # analytics experiments
     'ENABLE_INSTRUCTOR_ANALYTICS': False,
 
+    # bulk email available to instructors:
     'ENABLE_INSTRUCTOR_EMAIL': False,
 
     # enable analytics server.
@@ -340,7 +341,7 @@ TRACKING_BACKENDS = {
     }
 }
 
-# Backawrds compatibility with ENABLE_SQL_TRACKING_LOGS feature flag.
+# Backwards compatibility with ENABLE_SQL_TRACKING_LOGS feature flag.
 # In the future, adding the backend to TRACKING_BACKENDS enough.
 if MITX_FEATURES.get('ENABLE_SQL_TRACKING_LOGS'):
     TRACKING_BACKENDS.update({
@@ -425,12 +426,9 @@ HTTPS = 'on'
 ROOT_URLCONF = 'lms.urls'
 IGNORABLE_404_ENDS = ('favicon.ico')
 
-# Email
+# Platform Email
 EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'
 DEFAULT_FROM_EMAIL = 'registration@edx.org'
-DEFAULT_BULK_FROM_EMAIL = 'no-reply@courseupdates.edx.org'
-EMAILS_PER_TASK = 100
-EMAILS_PER_QUERY = 1000
 DEFAULT_FEEDBACK_EMAIL = 'feedback@edx.org'
 SERVER_EMAIL = 'devops@edx.org'
 TECH_SUPPORT_EMAIL = 'technical@edx.org'
@@ -812,6 +810,45 @@ CELERY_QUEUES = {
     DEFAULT_PRIORITY_QUEUE: {}
 }
 
+# let logging work as configured:
+CELERYD_HIJACK_ROOT_LOGGER = False
+
+################################ Bulk Email ###################################
+
+# Suffix used to construct 'from' email address for bulk emails.
+# A course-specific identifier is prepended.
+BULK_EMAIL_DEFAULT_FROM_EMAIL = 'no-reply@courseupdates.edx.org'
+
+# Parameters for breaking down course enrollment into subtasks.
+BULK_EMAIL_EMAILS_PER_TASK = 100
+BULK_EMAIL_EMAILS_PER_QUERY = 1000
+
+# Initial delay used for retrying tasks.  Additional retries use
+# longer delays.  Value is in seconds.
+BULK_EMAIL_DEFAULT_RETRY_DELAY = 30
+
+# Maximum number of retries per task for errors that are not related
+# to throttling.
+BULK_EMAIL_MAX_RETRIES = 5
+
+# Maximum number of retries per task for errors that are related to
+# throttling.  If this is not set, then there is no cap on such retries.
+BULK_EMAIL_INFINITE_RETRY_CAP = 1000
+
+# We want Bulk Email running on the high-priority queue, so we define the
+# routing key that points to it.  At the moment, the name is the same.
+BULK_EMAIL_ROUTING_KEY = HIGH_PRIORITY_QUEUE
+
+# Flag to indicate if individual email addresses should be logged as they are sent
+# a bulk email message.
+BULK_EMAIL_LOG_SENT_EMAILS = False
+
+# Delay in seconds to sleep between individual mail messages being sent,
+# when a bulk email task is retried for rate-related reasons.  Choose this
+# value depending on the number of workers that might be sending email in
+# parallel, and what the SES rate is.
+BULK_EMAIL_RETRY_DELAY_BETWEEN_SENDS = 0.02
+
 ################################### APPS ######################################
 INSTALLED_APPS = (
     # Standard ones that are always installed...

lms/templates/courseware/instructor_dashboard.html

@@ -550,6 +550,13 @@ function goto( mode)
         return true;
       }
     </script>
+
+    <p>These email actions run in the background, and status for active email tasks will appear in a table below.
+       To see status for all bulk email tasks submitted for this course, click on this button:
+    </p>
+    <p>
+      <input type="submit" name="action" value="Show Background Email Task History">
+    </p>
 %endif
 
     </form>

requirements/edx/github.txt

@@ -17,6 +17,6 @@
 # Our libraries:
 -e git+https://github.com/edx/XBlock.git@cee38a15f#egg=XBlock
 -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail
--e git+https://github.com/edx/diff-cover.git@v0.2.5#egg=diff_cover
+-e git+https://github.com/edx/diff-cover.git@v0.2.6#egg=diff_cover
 -e git+https://github.com/edx/js-test-tool.git@v0.1.1#egg=js_test_tool
 -e git+https://github.com/edx/django-waffle.git@823a102e48#egg=django-waffle