From 55521590d1caf2e5d7c409139ed1edf1227bb04e Mon Sep 17 00:00:00 2001 From: njdup Date: Thu, 7 Aug 2014 17:03:08 +0000 Subject: [PATCH] Email content history fix and additions This addresses a bug in the email content history table where "Unknown" was displayed in the number of emails sent column if any sort of failure occurred during email sending. This behavior has been editted so now the number of emails that failed to send is displayed, along with the number of emails that were successfully sent. If the email task is still pending, "0 sent" is displayed. As a small addition, the table now also includes the authors of previously sent emails, and the modal window for an email also displays its author. --- lms/djangoapps/instructor/tests/test_api.py | 70 ++++++++++++++----- lms/djangoapps/instructor/tests/utils.py | 19 +++-- .../views/instructor_task_helpers.py | 37 +++++++--- .../src/instructor_dashboard/util.coffee | 20 ++++-- 4 files changed, 112 insertions(+), 34 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index bacaeb9e4a..59227e9825 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/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/") diff --git a/lms/djangoapps/instructor/tests/utils.py b/lms/djangoapps/instructor/tests/utils.py index 884fb95ba1..b2444fe1b1 100644 --- a/lms/djangoapps/instructor/tests/utils.py +++ b/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' diff --git a/lms/djangoapps/instructor/views/instructor_task_helpers.py b/lms/djangoapps/instructor/views/instructor_task_helpers.py index 4c89e6addd..11d0e5e399 100644 --- a/lms/djangoapps/instructor/views/instructor_task_helpers.py +++ b/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 diff --git a/lms/static/coffee/src/instructor_dashboard/util.coffee b/lms/static/coffee/src/instructor_dashboard/util.coffee index 1010474651..a9ace5f643 100644 --- a/lms/static/coffee/src/instructor_dashboard/util.coffee +++ b/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 = $('').text(value['subject']).html() return '

' + subject_text + '

' +# Formats the author field for the email content history table +sent_by_formatter = (row, cell, value, columnDef, dataContext) -> + if value is null then return "

" + gettext("Unknown") + "

" else return '

' + value + '

' + # Formats the created field for the email content history table created_formatter = (row, cell, value, columnDef, dataContext) -> - if !value then return "

" + gettext("Unknown") + "

" else return '

' + value + '

' + if value is null then return "

" + gettext("Unknown") + "

" else return '

' + value + '

' # Formats the number sent field for the email content history table number_sent_formatter = (row, cell, value, columndDef, dataContext) -> - if !value then return "

" + gettext("Unknown") + "

" else return '

' + value + '

' + if value is null then return "

" + gettext("Unknown") + "

" else return '

' + value + '

' # Creates a table to display the content of bulk course emails # sent in the past @@ -153,6 +157,14 @@ create_email_content_table = ($table_emails, $table_emails_inner, email_data) -> minWidth: 80 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' @@ -203,7 +215,7 @@ create_email_message_views = ($messages_wrapper, emails) -> # HTML escape the subject line subject_text = $('').text(email_info.email['subject']).html() $email_header.append $('

', class: "message-bold").html('' + gettext('Subject:') + ' ' + subject_text) - + $email_header.append $('

', class: "message-bold").html('' + gettext('Sent By:') + ' ' + email_info.requester) $email_header.append $('

', class: "message-bold").html('' + gettext('Time Sent:') + ' ' + email_info.created) $email_header.append $('

', class: "message-bold").html('' + gettext('Sent To:') + ' ' + email_info.sent_to) $email_wrapper.append $email_header