From 48510629879d80e3e66db62fe511312be7e14430 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 3 Feb 2015 11:50:41 -0500 Subject: [PATCH 1/2] Don't instantiate bogus modulestores just to get template rendering in old ORA --- .../combined_open_ended_modulev1.py | 4 +-- .../combined_open_ended_rubric.py | 8 +++--- .../controller_query_service.py | 6 ++--- .../grading_service_module.py | 4 +-- .../open_ended_module.py | 2 +- .../openendedchild.py | 4 +-- .../peer_grading_service.py | 5 ++-- .../self_assessment_module.py | 2 +- .../xmodule/xmodule/peer_grading_module.py | 4 +-- .../open_ended_notifications.py | 27 ++----------------- .../staff_grading_service.py | 13 +-------- lms/djangoapps/open_ended_grading/tests.py | 2 +- lms/djangoapps/open_ended_grading/utils.py | 15 +---------- 13 files changed, 24 insertions(+), 72 deletions(-) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py index e2cebeabc6..2f3d2f7159 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py @@ -138,7 +138,7 @@ class CombinedOpenEndedV1Module(): self.skip_basic_checks = instance_state.get('skip_spelling_checks', SKIP_BASIC_CHECKS) in TRUE_DICT if system.open_ended_grading_interface: - self.peer_gs = PeerGradingService(system.open_ended_grading_interface, system) + self.peer_gs = PeerGradingService(system.open_ended_grading_interface, system.render_template) else: self.peer_gs = MockPeerGradingService() @@ -159,7 +159,7 @@ class CombinedOpenEndedV1Module(): raise self.display_due_date = self.timeinfo.display_due_date - self.rubric_renderer = CombinedOpenEndedRubric(system, True) + self.rubric_renderer = CombinedOpenEndedRubric(system.render_template, True) rubric_string = stringify_children(definition['rubric']) self._max_score = self.rubric_renderer.check_if_rubric_is_parseable(rubric_string, location, MAX_SCORE_ALLOWED) diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_rubric.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_rubric.py index de90378f6c..16ea4acba8 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_rubric.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_rubric.py @@ -40,10 +40,10 @@ class RubricParsingError(Exception): class CombinedOpenEndedRubric(object): TEMPLATE_DIR = "combinedopenended/openended" - def __init__(self, system, view_only=False): + def __init__(self, render_template, view_only=False): self.has_score = False self.view_only = view_only - self.system = system + self.render_template = render_template def render_rubric(self, rubric_xml, score_list=None): ''' @@ -70,7 +70,7 @@ class CombinedOpenEndedRubric(object): rubric_template = '{0}/open_ended_rubric.html'.format(self.TEMPLATE_DIR) if self.view_only: rubric_template = '{0}/open_ended_view_only_rubric.html'.format(self.TEMPLATE_DIR) - html = self.system.render_template( + html = self.render_template( rubric_template, { 'categories': rubric_categories, @@ -254,7 +254,7 @@ class CombinedOpenEndedRubric(object): else: correct.append(.5) - html = self.system.render_template( + html = self.render_template( '{0}/open_ended_combined_rubric.html'.format(self.TEMPLATE_DIR), { 'categories': rubric_categories, diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py index b20c0279bc..9f84b8cbe9 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/controller_query_service.py @@ -13,8 +13,8 @@ class ControllerQueryService(GradingService): METRIC_NAME = 'edxapp.open_ended_grading.controller_query_service' - def __init__(self, config, system): - config['system'] = system + def __init__(self, config, render_template): + config['render_template'] = render_template super(ControllerQueryService, self).__init__(config) self.url = config['url'] + config['grading_controller'] self.login_url = self.url + '/login/' @@ -105,7 +105,7 @@ class MockControllerQueryService(object): Mock controller query service for testing """ - def __init__(self, config, system): + def __init__(self, config, render_template): pass def check_for_eta(self, *args, **kwargs): diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/grading_service_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/grading_service_module.py index 5be3728ae0..e1e7d52145 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/grading_service_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/grading_service_module.py @@ -27,7 +27,7 @@ class GradingService(object): self.username = config['username'] self.password = config['password'] self.session = requests.Session() - self.system = config['system'] + self.render_template = config['render_template'] def _login(self): """ @@ -142,7 +142,7 @@ class GradingService(object): try: if 'rubric' in response: rubric = response['rubric'] - rubric_renderer = CombinedOpenEndedRubric(self.system, view_only) + rubric_renderer = CombinedOpenEndedRubric(self.render_template, view_only) rubric_dict = rubric_renderer.render_rubric(rubric) success = rubric_dict['success'] rubric_html = rubric_dict['html'] diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py index 78645816aa..b4801ad16b 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py @@ -484,7 +484,7 @@ class OpenEndedModule(openendedchild.OpenEndedChild): feedback = self._convert_longform_feedback_to_html(response_items) rubric_scores = [] if response_items['rubric_scores_complete'] is True: - rubric_renderer = CombinedOpenEndedRubric(system, True) + rubric_renderer = CombinedOpenEndedRubric(system.render_template, True) rubric_dict = rubric_renderer.render_rubric(response_items['rubric_xml']) success = rubric_dict['success'] rubric_feedback = rubric_dict['html'] diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py index a461eff98d..f0ac9052b5 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py @@ -145,9 +145,9 @@ class OpenEndedChild(object): # Used for progress / grading. Currently get credit just for # completion (doesn't matter if you self-assessed correct/incorrect). if system.open_ended_grading_interface: - self.peer_gs = PeerGradingService(system.open_ended_grading_interface, system) + self.peer_gs = PeerGradingService(system.open_ended_grading_interface, system.render_template) self.controller_qs = controller_query_service.ControllerQueryService( - system.open_ended_grading_interface, system + system.open_ended_grading_interface, system.render_template ) else: self.peer_gs = MockPeerGradingService() diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py index 036ed74b22..da76e1867d 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/peer_grading_service.py @@ -14,8 +14,8 @@ class PeerGradingService(GradingService): METRIC_NAME = 'edxapp.open_ended_grading.peer_grading_service' - def __init__(self, config, system): - config['system'] = system + def __init__(self, config, render_template): + config['render_template'] = render_template super(PeerGradingService, self).__init__(config) self.url = config['url'] + config['peer_grading'] self.login_url = self.url + '/login/' @@ -27,7 +27,6 @@ class PeerGradingService(GradingService): self.get_problem_list_url = self.url + '/get_problem_list/' self.get_notifications_url = self.url + '/get_notifications/' self.get_data_for_location_url = self.url + '/get_data_for_location/' - self.system = system def get_data_for_location(self, problem_location, student_id): if isinstance(problem_location, UsageKey): diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py index efc4f38d3b..e9881db344 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/self_assessment_module.py @@ -119,7 +119,7 @@ class SelfAssessmentModule(openendedchild.OpenEndedChild): if self.child_state == self.INITIAL: return '' - rubric_renderer = CombinedOpenEndedRubric(system, False) + rubric_renderer = CombinedOpenEndedRubric(system.render_template, False) rubric_dict = rubric_renderer.render_rubric(self.child_rubric) success = rubric_dict['success'] rubric_html = rubric_dict['html'] diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index e28561a6e1..f275977a7b 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -125,7 +125,7 @@ class PeerGradingModule(PeerGradingFields, XModule): # We need to set the location here so the child modules can use it. self.runtime.set('location', self.location) if (self.runtime.open_ended_grading_interface): - self.peer_gs = PeerGradingService(self.system.open_ended_grading_interface, self.system) + self.peer_gs = PeerGradingService(self.system.open_ended_grading_interface, self.system.render_template) else: self.peer_gs = MockPeerGradingService() @@ -497,7 +497,7 @@ class PeerGradingModule(PeerGradingFields, XModule): try: response = self.peer_gs.save_calibration_essay(**data_dict) if 'actual_rubric' in response: - rubric_renderer = combined_open_ended_rubric.CombinedOpenEndedRubric(self.system, True) + rubric_renderer = combined_open_ended_rubric.CombinedOpenEndedRubric(self.system.render_template, True) response['actual_rubric'] = rubric_renderer.render_rubric(response['actual_rubric'])['html'] return response except GradingServiceError: diff --git a/lms/djangoapps/open_ended_grading/open_ended_notifications.py b/lms/djangoapps/open_ended_grading/open_ended_notifications.py index cf1185e1c7..98773bfedb 100644 --- a/lms/djangoapps/open_ended_grading/open_ended_notifications.py +++ b/lms/djangoapps/open_ended_grading/open_ended_notifications.py @@ -10,7 +10,6 @@ from xmodule.open_ended_grading_classes.controller_query_service import Controll from xmodule.modulestore.django import ModuleI18nService from courseware.access import has_access -from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem from edxmako.shortcuts import render_to_string from student.models import unique_id_for_user from util.cache import cache @@ -66,17 +65,7 @@ def staff_grading_notifications(course, user): def peer_grading_notifications(course, user): - system = LmsModuleSystem( - track_function=None, - get_module=None, - render_template=render_to_string, - replace_urls=None, - descriptor_runtime=None, - services={ - 'i18n': ModuleI18nService(), - }, - ) - peer_gs = peer_grading_service.PeerGradingService(settings.OPEN_ENDED_GRADING_INTERFACE, system) + peer_gs = peer_grading_service.PeerGradingService(settings.OPEN_ENDED_GRADING_INTERFACE, render_to_string) pending_grading = False img_path = "" course_id = course.id @@ -128,20 +117,8 @@ def combined_notifications(course, user): if not user.is_authenticated(): return notification_dict - #Define a mock modulesystem - system = LmsModuleSystem( - static_url="/static", - track_function=None, - get_module=None, - render_template=render_to_string, - replace_urls=None, - descriptor_runtime=None, - services={ - 'i18n': ModuleI18nService(), - }, - ) #Initialize controller query service using our mock system - controller_qs = ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, system) + controller_qs = ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, render_to_string) student_id = unique_id_for_user(user) user_is_staff = has_access(user, 'staff', course) course_id = course.id diff --git a/lms/djangoapps/open_ended_grading/staff_grading_service.py b/lms/djangoapps/open_ended_grading/staff_grading_service.py index 7d7200fba8..4830aa5b20 100644 --- a/lms/djangoapps/open_ended_grading/staff_grading_service.py +++ b/lms/djangoapps/open_ended_grading/staff_grading_service.py @@ -14,7 +14,6 @@ from xmodule.open_ended_grading_classes.grading_service_module import GradingSer from xmodule.modulestore.django import ModuleI18nService from courseware.access import has_access -from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem from edxmako.shortcuts import render_to_string from student.models import unique_id_for_user @@ -90,17 +89,7 @@ class StaffGradingService(GradingService): METRIC_NAME = 'edxapp.open_ended_grading.staff_grading_service' def __init__(self, config): - config['system'] = LmsModuleSystem( - static_url='/static', - track_function=None, - get_module=None, - render_template=render_to_string, - replace_urls=None, - descriptor_runtime=None, - services={ - 'i18n': ModuleI18nService(), - }, - ) + config['render_template'] = render_to_string super(StaffGradingService, self).__init__(config) self.url = config['url'] + config['staff_grading'] self.login_url = self.url + '/login/' diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 5f97ec2afa..26115b7a12 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -469,7 +469,7 @@ class TestPanel(ModuleStoreTestCase): Mock( return_value=controller_query_service.MockControllerQueryService( settings.OPEN_ENDED_GRADING_INTERFACE, - utils.SYSTEM + utils.render_to_string ) ) ) diff --git a/lms/djangoapps/open_ended_grading/utils.py b/lms/djangoapps/open_ended_grading/utils.py index 4ab0c4d7ac..273178e9f0 100644 --- a/lms/djangoapps/open_ended_grading/utils.py +++ b/lms/djangoapps/open_ended_grading/utils.py @@ -9,7 +9,6 @@ from xmodule.open_ended_grading_classes.grading_service_module import GradingSer from django.utils.translation import ugettext as _ from django.conf import settings -from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem from edxmako.shortcuts import render_to_string @@ -26,18 +25,6 @@ GRADER_DISPLAY_NAMES = { STUDENT_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify course staff.") STAFF_ERROR_MESSAGE = _("Error occurred while contacting the grading service. Please notify your edX point of contact.") -SYSTEM = LmsModuleSystem( - static_url='/static', - track_function=None, - get_module=None, - render_template=render_to_string, - replace_urls=None, - descriptor_runtime=None, - services={ - 'i18n': ModuleI18nService(), - }, -) - def generate_problem_url(problem_url_parts, base_course_url): """ @@ -85,7 +72,7 @@ def create_controller_query_service(): """ Return an instance of a service that can query edX ORA. """ - return ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, SYSTEM) + return ControllerQueryService(settings.OPEN_ENDED_GRADING_INTERFACE, render_to_string) class StudentProblemList(object): From 095b96b041b3e273322d500a878b2e24dbfd0351 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Fri, 6 Feb 2015 13:09:56 -0500 Subject: [PATCH 2/2] Fix unicode error when sending confirmation email in shopping cart Wrap send email operation in try/except to avoid rolling back the transaction once a user has paid. --- lms/djangoapps/shoppingcart/models.py | 20 +++++++++---- .../shoppingcart/tests/test_models.py | 28 +++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 32562455d1..819a8327b7 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -481,10 +481,18 @@ class Order(models.Model): log.exception('Exception at creating pdf file.') pdf_file = None - self.send_confirmation_emails( - orderitems, self.order_type == OrderTypes.BUSINESS, - csv_file, pdf_file, site_name, courses_info - ) + try: + self.send_confirmation_emails( + orderitems, self.order_type == OrderTypes.BUSINESS, + csv_file, pdf_file, site_name, courses_info + ) + except Exception: # pylint: disable=broad-except + # Catch all exceptions here, since the Django view implicitly + # wraps this in a transaction. If the order completes successfully, + # we don't want to roll back just because we couldn't send + # the confirmation email. + log.exception('Error occurred while sending payment confirmation email') + self._emit_order_event('Completed Order', orderitems) def refund(self): @@ -1460,7 +1468,9 @@ class CertificateItem(OrderItem): "If you haven't verified your identity yet, please start the verification process ({verification_url})." ).format(verification_url=verification_url) - return "{verification_reminder} {refund_reminder}".format( + # Need this to be unicode in case the reminder strings + # have been translated and contain non-ASCII unicode + return u"{verification_reminder} {refund_reminder}".format( verification_reminder=verification_reminder, refund_reminder=refund_reminder ) diff --git a/lms/djangoapps/shoppingcart/tests/test_models.py b/lms/djangoapps/shoppingcart/tests/test_models.py index 63d0703b76..c8c4e9e0cd 100644 --- a/lms/djangoapps/shoppingcart/tests/test_models.py +++ b/lms/djangoapps/shoppingcart/tests/test_models.py @@ -375,6 +375,34 @@ class OrderTest(ModuleStoreTestCase): cart.generate_receipt_instructions() mock_gen_inst.assert_called_with() + def test_confirmation_email_error(self): + CourseMode.objects.create( + course_id=self.course_key, + mode_slug="verified", + mode_display_name="Verified", + min_price=self.cost + ) + + cart = Order.get_cart_for_user(self.user) + CertificateItem.add_to_order(cart, self.course_key, self.cost, 'verified') + + # Simulate an error when sending the confirmation + # email. This should NOT raise an exception. + # If it does, then the implicit view-level + # transaction could cause a roll-back, effectively + # reversing order fulfillment. + with patch.object(mail.message.EmailMessage, 'send') as mock_send: + mock_send.side_effect = Exception("Kaboom!") + cart.purchase() + + # Verify that the purchase completed successfully + self.assertEqual(cart.status, 'purchased') + + # Verify that the user is enrolled as "verified" + mode, is_active = CourseEnrollment.enrollment_mode_for_user(self.user, self.course_key) + self.assertTrue(is_active) + self.assertEqual(mode, 'verified') + class OrderItemTest(TestCase): def setUp(self):