Merge pull request #4746 from Stanford-Online/njdupoux/instructor-email-history-fixes

Email content history fix and additions

lms/djangoapps/instructor/tests/test_api.py

@@ -1980,22 +1980,27 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas
         """
         patch.stopall()
 
-    def setup_fake_email_info(self, num_emails):
+    def setup_fake_email_info(self, num_emails, with_failures=False):
         """ Initialize the specified number of fake emails """
         for email_id in range(num_emails):
             num_sent = random.randint(1, 15401)
-            self.tasks[email_id] = FakeContentTask(email_id, num_sent, 'expected')
+            if with_failures:
+                failed = random.randint(1, 15401)
+            else:
+                failed = 0
+
+            self.tasks[email_id] = FakeContentTask(email_id, num_sent, failed, 'expected')
             self.emails[email_id] = FakeEmail(email_id)
-            self.emails_info[email_id] = FakeEmailInfo(self.emails[email_id], num_sent)
+            self.emails_info[email_id] = FakeEmailInfo(self.emails[email_id], num_sent, failed)
 
     def get_matching_mock_email(self, **kwargs):
         """ Returns the matching mock emails for the given id """
         email_id = kwargs.get('id', 0)
         return self.emails[email_id]
 
-    def get_email_content_response(self, num_emails, task_history_request):
+    def get_email_content_response(self, num_emails, task_history_request, with_failures=False):
         """ Calls the list_email_content endpoint and returns the repsonse """
-        self.setup_fake_email_info(num_emails)
+        self.setup_fake_email_info(num_emails, with_failures)
         task_history_request.return_value = self.tasks.values()
         url = reverse('list_email_content', kwargs={'course_id': self.course.id.to_deprecated_string()})
         with patch('instructor.views.api.CourseEmail.objects.get') as mock_email_info:
@@ -2004,6 +2009,19 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas
         self.assertEqual(response.status_code, 200)
         return response
 
+    def check_emails_sent(self, num_emails, task_history_request, with_failures=False):
+        """ Tests sending emails with or without failures """
+        response = self.get_email_content_response(num_emails, task_history_request, with_failures)
+        self.assertTrue(task_history_request.called)
+        expected_email_info = [email_info.to_dict() for email_info in self.emails_info.values()]
+        actual_email_info = json.loads(response.content)['emails']
+
+        self.assertEqual(len(actual_email_info), num_emails)
+        for exp_email, act_email in zip(expected_email_info, actual_email_info):
+            self.assertDictEqual(exp_email, act_email)
+
+        self.assertEqual(expected_email_info, actual_email_info)
+
     def test_content_list_one_email(self, task_history_request):
         """ Test listing of bulk emails when email list has one email """
         response = self.get_email_content_response(1, task_history_request)
@@ -2030,20 +2048,11 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas
 
     def test_content_list_email_content_many(self, task_history_request):
         """ Test listing of bulk emails sent large amount of emails """
-        response = self.get_email_content_response(50, task_history_request)
-        self.assertTrue(task_history_request.called)
-        expected_email_info = [email_info.to_dict() for email_info in self.emails_info.values()]
-        actual_email_info = json.loads(response.content)['emails']
-
-        self.assertEqual(len(actual_email_info), 50)
-        for exp_email, act_email in zip(expected_email_info, actual_email_info):
-            self.assertDictEqual(exp_email, act_email)
-
-        self.assertEqual(actual_email_info, expected_email_info)
+        self.check_emails_sent(50, task_history_request)
 
     def test_list_email_content_error(self, task_history_request):
         """ Test handling of error retrieving email """
-        invalid_task = FakeContentTask(0, 0, 'test')
+        invalid_task = FakeContentTask(0, 0, 0, 'test')
         invalid_task.make_invalid_input()
         task_history_request.return_value = [invalid_task]
         url = reverse('list_email_content', kwargs={'course_id': self.course.id.to_deprecated_string()})
@@ -2054,9 +2063,36 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas
         returned_email_info = json.loads(response.content)['emails']
         self.assertEqual(len(returned_email_info), 1)
         returned_info = returned_email_info[0]
-        for info in ['created', 'sent_to', 'email', 'number_sent']:
+        for info in ['created', 'sent_to', 'email', 'number_sent', 'requester']:
             self.assertEqual(returned_info[info], None)
 
+    def test_list_email_with_failure(self, task_history_request):
+        """ Test the handling of email task that had failures """
+        self.check_emails_sent(1, task_history_request, True)
+
+    def test_list_many_emails_with_failures(self, task_history_request):
+        """ Test the handling of many emails with failures """
+        self.check_emails_sent(50, task_history_request, True)
+
+    def test_list_email_with_no_successes(self, task_history_request):
+        task_info = FakeContentTask(0, 0, 10, 'expected')
+        email = FakeEmail(0)
+        email_info = FakeEmailInfo(email, 0, 10)
+        task_history_request.return_value = [task_info]
+        url = reverse('list_email_content', kwargs={'course_id': self.course.id.to_deprecated_string()})
+        with patch('instructor.views.api.CourseEmail.objects.get') as mock_email_info:
+            mock_email_info.return_value = email
+            response = self.client.get(url, {})
+        self.assertEqual(response.status_code, 200)
+
+        self.assertTrue(task_history_request.called)
+        returned_info_list = json.loads(response.content)['emails']
+
+        self.assertEqual(len(returned_info_list), 1)
+        returned_info = returned_info_list[0]
+        expected_info = email_info.to_dict()
+        self.assertDictEqual(expected_info, returned_info)
+
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 @override_settings(ANALYTICS_SERVER_URL="http://robotanalyticsserver.netbot:900/")

lms/djangoapps/instructor/tests/utils.py

@@ -26,14 +26,16 @@ class FakeContentTask(FakeInfo):
     FEATURES = [
         'task_input',
         'task_output',
+        'requester',
     ]
 
-    def __init__(self, email_id, num_sent, sent_to):
+    def __init__(self, email_id, num_sent, num_failed, sent_to):
         super(FakeContentTask, self).__init__()
         self.task_input = {'email_id': email_id, 'to_option': sent_to}
         self.task_input = json.dumps(self.task_input)
-        self.task_output = {'total': num_sent}
+        self.task_output = {'succeeded': num_sent, 'failed': num_failed}
         self.task_output = json.dumps(self.task_output)
+        self.requester = 'expected'
 
     def make_invalid_input(self):
         """Corrupt the task input field to test errors"""
@@ -67,7 +69,8 @@ class FakeEmailInfo(FakeInfo):
         u'created',
         u'sent_to',
         u'email',
-        u'number_sent'
+        u'number_sent',
+        u'requester',
     ]
 
     EMAIL_FEATURES = [
@@ -76,9 +79,15 @@ class FakeEmailInfo(FakeInfo):
         u'id'
     ]
 
-    def __init__(self, fake_email, num_sent):
+    def __init__(self, fake_email, num_sent, num_failed):
         super(FakeEmailInfo, self).__init__()
         self.created = get_default_time_display(fake_email.created)
-        self.number_sent = num_sent
+
+        number_sent = str(num_sent) + ' sent'
+        if num_failed > 0:
+            number_sent += ', ' + str(num_failed) + " failed"
+
+        self.number_sent = number_sent
         fake_email_dict = fake_email.to_dict()
         self.email = {feature: fake_email_dict[feature] for feature in self.EMAIL_FEATURES}
+        self.requester = u'expected'

lms/djangoapps/instructor/views/instructor_task_helpers.py

@@ -7,6 +7,7 @@ import logging
 from util.date_utils import get_default_time_display
 from bulk_email.models import CourseEmail
 from django.utils.translation import ugettext as _
+from django.utils.translation import ungettext
 from instructor_task.views import get_task_completion_info
 
 log = logging.getLogger(__name__)
@@ -21,7 +22,8 @@ def email_error_information():
         'created',
         'sent_to',
         'email',
-        'number_sent'
+        'number_sent',
+        'requester',
     ]
     return {info: None for info in expected_info}
 
@@ -33,6 +35,7 @@ def extract_email_features(email_task):
     Expects that the given task has the following attributes:
     * task_input (dict containing email_id and to_option)
     * task_output (optional, dict containing total emails sent)
+    * requester, the user who executed the task
 
     With this information, gets the corresponding email object from the
     bulk emails table, and loads up a dict containing the following:
@@ -40,6 +43,7 @@ def extract_email_features(email_task):
     * sent_to, the group the email was delivered to
     * email, dict containing the subject, id, and html_message of an email
     * number_sent, int number of emails sent
+    * requester, the user who sent the emails
     If task_input cannot be loaded, then the email cannot be loaded
     and None is returned for these fields.
     """
@@ -51,26 +55,43 @@ def extract_email_features(email_task):
         return email_error_information()
 
     email = CourseEmail.objects.get(id=task_input_information['email_id'])
-
-    creation_time = get_default_time_display(email.created)
-    email_feature_dict = {'created': creation_time, 'sent_to': task_input_information['to_option']}
+    email_feature_dict = {
+        'created': get_default_time_display(email.created),
+        'sent_to': task_input_information['to_option'],
+        'requester': str(getattr(email_task, 'requester')),
+    }
     features = ['subject', 'html_message', 'id']
     email_info = {feature: unicode(getattr(email, feature)) for feature in features}
 
     # Pass along email as an object with the information we desire
     email_feature_dict['email'] = email_info
 
-    number_sent = None
+    # Translators: number sent refers to the number of emails sent
+    number_sent = _('0 sent')
     if hasattr(email_task, 'task_output') and email_task.task_output is not None:
         try:
             task_output = json.loads(email_task.task_output)
         except ValueError:
             log.error("Could not parse task output as valid json; task output: %s", email_task.task_output)
         else:
-            if 'total' in task_output:
-                number_sent = int(task_output['total'])
-    email_feature_dict['number_sent'] = number_sent
+            if 'succeeded' in task_output and task_output['succeeded'] > 0:
+                num_emails = task_output['succeeded']
+                number_sent = ungettext(
+                    "{num_emails} sent",
+                    "{num_emails} sent",
+                    num_emails
+                ).format(num_emails=num_emails)
+
+            if 'failed' in task_output and task_output['failed'] > 0:
+                num_emails = task_output['failed']
+                number_sent += ", "
+                number_sent += ungettext(
+                    "{num_emails} failed",
+                    "{num_emails} failed",
+                    num_emails
+                ).format(num_emails=num_emails)
 
+    email_feature_dict['number_sent'] = number_sent
     return email_feature_dict
 
 

lms/static/coffee/src/instructor_dashboard/util.coffee

@@ -121,17 +121,21 @@ create_task_list_table = ($table_tasks, tasks_data) ->
 
 # Formats the subject field for email content history table
 subject_formatter = (row, cell, value, columnDef, dataContext) ->
-  if !value then return gettext("An error occurred retrieving your email. Please try again later, and contact technical support if the problem persists.")
+  if value is null then return gettext("An error occurred retrieving your email. Please try again later, and contact technical support if the problem persists.")
   subject_text = $('<span>').text(value['subject']).html()
   return '<p><a href="#email_message_' + value['id']+ '" id="email_message_' + value['id'] + '_trig">' + subject_text + '</a></p>'
 
+# Formats the author field for the email content history table
+sent_by_formatter = (row, cell, value, columnDef, dataContext) ->
+  if value is null then return "<p>" + gettext("Unknown") + "</p>" else return '<p>' + value + '</p>'
+
 # Formats the created field for the email content history table
 created_formatter = (row, cell, value, columnDef, dataContext) ->
-  if !value then return "<p>" + gettext("Unknown") + "</p>" else return '<p>' + value + '</p>'
+  if value is null then return "<p>" + gettext("Unknown") + "</p>" else return '<p>' + value + '</p>'
 
 # Formats the number sent field for the email content history table
 number_sent_formatter = (row, cell, value, columndDef, dataContext) ->
-  if !value then return "<p>" + gettext("Unknown") + "</p>" else return '<p>' + value + '</p>'
+  if value is null then return "<p>" + gettext("Unknown") + "</p>" else return '<p>' + value + '</p>'
 
 # Creates a table to display the content of bulk course emails
 # sent in the past
@@ -154,6 +158,14 @@ create_email_content_table = ($table_emails, $table_emails_inner, email_data) ->
       cssClass: "email-content-cell"
       formatter: subject_formatter
     ,
+      id: 'requester'
+      field: 'requester'
+      name: gettext('Sent By')
+      minWidth: 80
+      maxWidth: 100
+      cssClass: "email-content-cell"
+      formatter: sent_by_formatter
+    ,
       id: 'created'
       field: 'created'
       name: gettext('Time Sent')
@@ -203,7 +215,7 @@ create_email_message_views = ($messages_wrapper, emails) ->
     # HTML escape the subject line
     subject_text = $('<span>').text(email_info.email['subject']).html()
     $email_header.append $('<h2>', class: "message-bold").html('<em>' + gettext('Subject:') + '</em> ' + subject_text)
-
+    $email_header.append $('<h2>', class: "message-bold").html('<em>' + gettext('Sent By:') + '</em> ' + email_info.requester)
     $email_header.append $('<h2>', class: "message-bold").html('<em>' + gettext('Time Sent:') + '</em> ' + email_info.created)
     $email_header.append $('<h2>', class: "message-bold").html('<em>' + gettext('Sent To:') + '</em> ' + email_info.sent_to)
     $email_wrapper.append $email_header