From 89d47490575fecab12452f3502dacbadbeae04e4 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 9 Feb 2015 14:07:57 -0500 Subject: [PATCH 1/3] get the raw DescriptorSystem, not the CombinedSystem when instantiating xmodules (TNL-1226) get the raw DescriptorSystem, not the CombinedSystem when instantiating xmodules (TNL-1226) quality fixes remove trailing comma --- cms/djangoapps/contentstore/views/preview.py | 3 ++- common/lib/xmodule/xmodule/tests/__init__.py | 4 ++-- .../xmodule/xmodule/tests/test_library_content.py | 4 ++-- .../xmodule/tests/test_split_test_module.py | 2 +- common/lib/xmodule/xmodule/tests/test_vertical.py | 2 +- common/lib/xmodule/xmodule/x_module.py | 6 ++++++ lms/djangoapps/courseware/module_render.py | 2 +- .../courseware/tests/test_module_render.py | 15 +++++++++------ 8 files changed, 24 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 56b2fe2258..66e4d9de18 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -185,7 +185,8 @@ def _preview_module_system(request, descriptor, field_data): wrappers=wrappers, error_descriptor_class=ErrorDescriptor, get_user_role=lambda: get_user_role(request.user, course_id), - descriptor_runtime=descriptor.runtime, + # Get the raw DescriptorSystem, not the CombinedSystem + descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access services={ "i18n": ModuleI18nService(), "field-data": field_data, diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 6b6b6ddbd4..53c0dc15b9 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -94,7 +94,7 @@ def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')): """ user = Mock(name='get_test_system.user', is_staff=False) - descriptor_system = get_test_descriptor_system(), + descriptor_system = get_test_descriptor_system() def get_module(descriptor): """Mocks module_system get_module function""" @@ -107,7 +107,7 @@ def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')): # Descriptors can all share a single DescriptorSystem. # So, bind to the same one as the current descriptor. - module_system.descriptor_runtime = descriptor.runtime._descriptor_system + module_system.descriptor_runtime = descriptor._runtime # pylint: disable=protected-access descriptor.bind_for_student(module_system, descriptor._field_data) diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index 77835c04b1..f8e80513f1 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -54,14 +54,14 @@ class LibraryContentTest(MixedSplitTestCase): Bind a module (part of self.course) so we can access student-specific data. """ module_system = get_test_system(course_id=self.course.location.course_key) - module_system.descriptor_runtime = module.runtime + module_system.descriptor_runtime = module.runtime._descriptor_system # pylint: disable=protected-access module_system._services['library_tools'] = self.tools # pylint: disable=protected-access def get_module(descriptor): """Mocks module_system get_module function""" sub_module_system = get_test_system(course_id=self.course.location.course_key) sub_module_system.get_module = get_module - sub_module_system.descriptor_runtime = descriptor.runtime + sub_module_system.descriptor_runtime = descriptor._runtime # pylint: disable=protected-access descriptor.bind_for_student(sub_module_system, descriptor._field_data) # pylint: disable=protected-access return descriptor diff --git a/common/lib/xmodule/xmodule/tests/test_split_test_module.py b/common/lib/xmodule/xmodule/tests/test_split_test_module.py index bd16cdbecc..97f71114b0 100644 --- a/common/lib/xmodule/xmodule/tests/test_split_test_module.py +++ b/common/lib/xmodule/xmodule/tests/test_split_test_module.py @@ -78,7 +78,7 @@ class SplitTestModuleTest(XModuleXmlImportTest, PartitionTestCase): self.course_sequence = self.course.get_children()[0] self.module_system = get_test_system() - self.module_system.descriptor_runtime = self.course.runtime._descriptor_system # pylint: disable=protected-access + self.module_system.descriptor_runtime = self.course._runtime # pylint: disable=protected-access self.course.runtime.export_fs = MemoryFS() self.partitions_service = StaticPartitionService( diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index e515ed2c03..9da65d75d0 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -27,7 +27,7 @@ class BaseVerticalModuleTest(XModuleXmlImportTest): course_seq = self.course.get_children()[0] self.module_system = get_test_system() - self.module_system.descriptor_runtime = self.course.runtime._descriptor_system # pylint: disable=protected-access + self.module_system.descriptor_runtime = self.course._runtime # pylint: disable=protected-access self.course.runtime.export_fs = MemoryFS() self.vertical = course_seq.get_children()[0] diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 892e0a99ae..468cd81533 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -3,6 +3,7 @@ import os import sys import yaml +from contracts import contract, new_contract from functools import partial from lxml import etree from collections import namedtuple @@ -1307,6 +1308,9 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # p pass +new_contract('DescriptorSystem', DescriptorSystem) + + class XMLParsingSystem(DescriptorSystem): def __init__(self, process_xml, **kwargs): """ @@ -1427,6 +1431,8 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # pylin Note that these functions can be closures over e.g. a django request and user, or other environment-specific info. """ + + @contract(descriptor_runtime='DescriptorSystem') def __init__( self, static_url, track_function, get_module, render_template, replace_urls, descriptor_runtime, user=None, filestore=None, diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 93cb59a49f..6fe7f2b276 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -642,7 +642,7 @@ def get_module_system_for_user(user, field_data_cache, 'user': DjangoXBlockUserService(user, user_is_staff=user_is_staff), }, get_user_role=lambda: get_user_role(user, course_id), - descriptor_runtime=descriptor.runtime, + descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access rebind_noauth_module_to_user=rebind_noauth_module_to_user, user_location=user_location, request_token=request_token, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index b4156ccc35..95c9e08e94 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -41,7 +41,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory, check_mongo_calls -from xmodule.x_module import XModuleDescriptor, XModule, STUDENT_VIEW +from xmodule.x_module import XModuleDescriptor, XModule, STUDENT_VIEW, CombinedSystem TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT @@ -903,13 +903,16 @@ class TestAnonymousStudentId(ModuleStoreTestCase, LoginEnrollmentTestCase): _field_data=Mock(spec=FieldData), location=location, static_asset_path=None, - runtime=Mock( + _runtime=Mock( spec=Runtime, resources_fs=None, - mixologist=Mock(_mixins=()) + mixologist=Mock(_mixins=(), name='mixologist'), + name='runtime', ), scope_ids=Mock(spec=ScopeIds), + name='descriptor' ) + descriptor.runtime = CombinedSystem(descriptor._runtime, None) # pylint: disable=protected-access # Use the xblock_class's bind_for_student method descriptor.bind_for_student = partial(xblock_class.bind_for_student, descriptor) @@ -919,10 +922,10 @@ class TestAnonymousStudentId(ModuleStoreTestCase, LoginEnrollmentTestCase): return render.get_module_for_descriptor_internal( user=self.user, descriptor=descriptor, - field_data_cache=Mock(spec=FieldDataCache), + field_data_cache=Mock(spec=FieldDataCache, name='field_data_cache'), course_id=course_id, - track_function=Mock(), # Track Function - xqueue_callback_url_prefix=Mock(), # XQueue Callback Url Prefix + track_function=Mock(name='track_function'), # Track Function + xqueue_callback_url_prefix=Mock(name='xqueue_callback_url_prefix'), # XQueue Callback Url Prefix request_token='request_token', ).xmodule_runtime.anonymous_student_id From 12f89b3c2d7a5212eb63f36fadf1b8fd3aaeb933 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 9 Feb 2015 12:13:35 -0500 Subject: [PATCH 2/3] Make registration endpoint CSRF exempt --- common/djangoapps/student/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 774cf23168..9480cabb1b 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -1370,7 +1370,7 @@ def _do_create_account(post_vars, extended_profile=None): return (user, profile, registration) -@ensure_csrf_cookie +@csrf_exempt def create_account(request, post_override=None): # pylint: disable-msg=too-many-statements """ JSON call to create new edX account. From 2f7bee4254534a19f886df4168ad48584519da28 Mon Sep 17 00:00:00 2001 From: Syed Hassan Raza Date: Tue, 10 Feb 2015 17:04:35 +0500 Subject: [PATCH 3/3] add logging for send_email_task --- lms/djangoapps/bulk_email/tasks.py | 107 +++++++++++++++++++++++++++-- 1 file changed, 102 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/bulk_email/tasks.py b/lms/djangoapps/bulk_email/tasks.py index 22101b46c5..4cb5384137 100644 --- a/lms/djangoapps/bulk_email/tasks.py +++ b/lms/djangoapps/bulk_email/tasks.py @@ -6,6 +6,7 @@ import re import random import json from time import sleep +from collections import Counter import dogstats_wrapper as dog_stats_api from smtplib import SMTPServerDisconnected, SMTPDataError, SMTPConnectError, SMTPException @@ -418,12 +419,31 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas 'failed' count above. """ # Get information from current task's request: + parent_task_id = InstructorTask.objects.get(pk=entry_id).task_id task_id = subtask_status.task_id + total_recipients = len(to_list) + recipient_num = 0 + total_recipients_successful = 0 + total_recipients_failed = 0 + recipients_info = Counter() + + log.info( + "BulkEmail ==> Task: %s, SubTask: %s, EmailId: %s, TotalRecipients: %s", + parent_task_id, + task_id, + email_id, + total_recipients + ) try: course_email = CourseEmail.objects.get(id=email_id) except CourseEmail.DoesNotExist as exc: - log.exception("Task %s: could not find email id:%s to send.", task_id, email_id) + log.exception( + "BulkEmail ==> Task: %s, SubTask: %s, EmailId: %s, Could not find email to send.", + parent_task_id, + task_id, + email_id + ) raise # Exclude optouts (if not a retry): @@ -459,6 +479,7 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas # That way, the to_list will always contain the recipients remaining to be emailed. # This is convenient for retries, which will need to send to those who haven't # yet been emailed, but not send to those who have already been sent to. + recipient_num += 1 current_recipient = to_list[-1] email = current_recipient['email'] email_context['email'] = email @@ -489,29 +510,81 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas sleep(settings.BULK_EMAIL_RETRY_DELAY_BETWEEN_SENDS) try: - log.debug('Email with id %s to be sent to %s', email_id, email) - + log.info( + "BulkEmail ==> Task: %s, SubTask: %s, EmailId: %s, Recipient num: %s/%s, \ + Recipient name: %s, Email address: %s", + parent_task_id, + task_id, + email_id, + recipient_num, + total_recipients, + current_recipient['profile__name'], + email + ) with dog_stats_api.timer('course_email.single_send.time.overall', tags=[_statsd_tag(course_title)]): connection.send_messages([email_msg]) except SMTPDataError as exc: # According to SMTP spec, we'll retry error codes in the 4xx range. 5xx range indicates hard failure. + total_recipients_failed += 1 + log.error( + "BulkEmail ==> Status: Failed(SMTPDataError), Task: %s, SubTask: %s, EmailId: %s, \ + Recipient num: %s/%s, Email address: %s", + parent_task_id, + task_id, + email_id, + recipient_num, + total_recipients, + email + ) if exc.smtp_code >= 400 and exc.smtp_code < 500: # This will cause the outer handler to catch the exception and retry the entire task. raise exc else: # This will fall through and not retry the message. - log.warning('Task %s: email with id %s not delivered to %s due to error %s', task_id, email_id, email, exc.smtp_error) + log.warning( + 'BulkEmail ==> Task: %s, SubTask: %s, EmailId: %s, Recipient num: %s/%s, \ + Email not delivered to %s due to error %s', + parent_task_id, + task_id, + email_id, + recipient_num, + total_recipients, + email, + exc.smtp_error + ) dog_stats_api.increment('course_email.error', tags=[_statsd_tag(course_title)]) subtask_status.increment(failed=1) except SINGLE_EMAIL_FAILURE_ERRORS as exc: # This will fall through and not retry the message. - log.warning('Task %s: email with id %s not delivered to %s due to error %s', task_id, email_id, email, exc) + total_recipients_failed += 1 + log.error( + "BulkEmail ==> Status: Failed(SINGLE_EMAIL_FAILURE_ERRORS), Task: %s, SubTask: %s, \ + EmailId: %s, Recipient num: %s/%s, Email address: %s, Exception: %s", + parent_task_id, + task_id, + email_id, + recipient_num, + total_recipients, + email, + exc + ) dog_stats_api.increment('course_email.error', tags=[_statsd_tag(course_title)]) subtask_status.increment(failed=1) else: + total_recipients_successful += 1 + log.info( + "BulkEmail ==> Status: Success, Task: %s, SubTask: %s, EmailId: %s, \ + Recipient num: %s/%s, Email address: %s,", + parent_task_id, + task_id, + email_id, + recipient_num, + total_recipients, + email + ) dog_stats_api.increment('course_email.sent', tags=[_statsd_tag(course_title)]) if settings.BULK_EMAIL_LOG_SENT_EMAILS: log.info('Email with id %s sent to %s', email_id, email) @@ -522,8 +595,32 @@ def _send_course_email(entry_id, email_id, to_list, global_email_context, subtas # Pop the user that was emailed off the end of the list only once they have # successfully been processed. (That way, if there were a failure that # needed to be retried, the user is still on the list.) + recipients_info[email] += 1 to_list.pop() + log.info( + "BulkEmail ==> Task: %s, SubTask: %s, EmailId: %s, Total Successful Recipients: %s/%s, \ + Failed Recipients: %s/%s", + parent_task_id, + task_id, + email_id, + total_recipients_successful, + total_recipients, + total_recipients_failed, + total_recipients + ) + duplicate_recipients = ["{0} ({1})".format(email, repetition) + for email, repetition in recipients_info.most_common() if repetition > 1] + if duplicate_recipients: + log.info( + "BulkEmail ==> Task: %s, SubTask: %s, EmailId: %s, Total Duplicate Recipients [%s]: [%s]", + parent_task_id, + task_id, + email_id, + len(duplicate_recipients), + ', '.join(duplicate_recipients) + ) + except INFINITE_RETRY_ERRORS as exc: dog_stats_api.increment('course_email.infinite_retry', tags=[_statsd_tag(course_title)]) # Increment the "retried_nomax" counter, update other counters with progress to date,