From 0e8b303d2ea0702d32978263953813f2c30a93fb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 10 Jul 2015 13:58:34 -0400 Subject: [PATCH] Cache CcxFieldOverrides on a per-request basis --- lms/djangoapps/ccx/overrides.py | 68 ++++++++++--------- .../tests/test_field_override_performance.py | 6 +- lms/djangoapps/ccx/tests/test_overrides.py | 4 +- lms/djangoapps/ccx/tests/test_views.py | 29 +++----- 4 files changed, 50 insertions(+), 57 deletions(-) diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index a2098da6a0..462bf73735 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -84,35 +84,40 @@ def get_override_for_ccx(ccx, block, name, default=None): specify the block and the name of the field. If the field is not overridden for the given ccx, returns `default`. """ - if not hasattr(block, '_ccx_overrides'): - block._ccx_overrides = {} # pylint: disable=protected-access - overrides = block._ccx_overrides.get(ccx.id) # pylint: disable=protected-access - if overrides is None: - overrides = _get_overrides_for_ccx(ccx, block) - block._ccx_overrides[ccx.id] = overrides # pylint: disable=protected-access - return overrides.get(name, default) + overrides = _get_overrides_for_ccx(ccx) + + if isinstance(block.location, CCXBlockUsageLocator): + non_ccx_key = block.location.to_block_locator() + else: + non_ccx_key = block.location + + block_overrides = overrides.get(non_ccx_key, {}) + if name in block_overrides: + return block.fields[name].from_json(block_overrides[name]) + else: + return default -def _get_overrides_for_ccx(ccx, block): +def _get_overrides_for_ccx(ccx): """ Returns a dictionary mapping field name to overriden value for any overrides set on this block for this CCX. """ - overrides = {} - # block as passed in may have a location specific to a CCX, we must strip - # that for this query - location = block.location - if isinstance(block.location, CCXBlockUsageLocator): - location = block.location.to_block_locator() - query = CcxFieldOverride.objects.filter( - ccx=ccx, - location=location - ) - for override in query: - field = block.fields[override.field] - value = field.from_json(json.loads(override.value)) - overrides[override.field] = value - return overrides + overrides_cache = request_cache.get_cache('ccx-overrides') + + if ccx not in overrides_cache: + overrides = {} + query = CcxFieldOverride.objects.filter( + ccx=ccx, + ) + + for override in query: + block_overrides = overrides.setdefault(override.location, {}) + block_overrides[override.field] = json.loads(override.value) + + overrides_cache[ccx] = overrides + + return overrides_cache[ccx] @transaction.commit_on_success @@ -123,23 +128,25 @@ def override_field_for_ccx(ccx, block, name, value): value to set for the given field. """ field = block.fields[name] - value = json.dumps(field.to_json(value)) + value_json = field.to_json(value) + serialized_value = json.dumps(value_json) try: override = CcxFieldOverride.objects.create( ccx=ccx, location=block.location, field=name, - value=value) + value=serialized_value + ) except IntegrityError: transaction.commit() override = CcxFieldOverride.objects.get( ccx=ccx, location=block.location, - field=name) - override.value = value + field=name + ) + override.value = serialized_value override.save() - if hasattr(block, '_ccx_overrides'): - del block._ccx_overrides[ccx.id] # pylint: disable=protected-access + _get_overrides_for_ccx(ccx).setdefault(block.location, {})[name] = value_json def clear_override_for_ccx(ccx, block, name): @@ -155,8 +162,7 @@ def clear_override_for_ccx(ccx, block, name): location=block.location, field=name).delete() - if hasattr(block, '_ccx_overrides'): - del block._ccx_overrides[ccx.id] # pylint: disable=protected-access + _get_overrides_for_ccx(ccx).setdefault(block.location, {}).pop(name) except CcxFieldOverride.DoesNotExist: pass diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index b9977b8657..77247650fb 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -250,9 +250,9 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): ('ccx', 1, True, False): (23, 4, 9), ('ccx', 2, True, False): (68, 19, 54), ('ccx', 3, True, False): (263, 84, 215), - ('ccx', 1, True, True): (33, 4, 13), - ('ccx', 2, True, True): (68, 19, 84), - ('ccx', 3, True, True): (263, 84, 335), + ('ccx', 1, True, True): (25, 4, 13), + ('ccx', 2, True, True): (70, 19, 84), + ('ccx', 3, True, True): (265, 84, 335), ('no_overrides', 1, False, False): (23, 4, 9), ('no_overrides', 2, False, False): (68, 19, 54), ('no_overrides', 3, False, False): (263, 84, 215), diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index 01ff0db111..d473c0aafa 100644 --- a/lms/djangoapps/ccx/tests/test_overrides.py +++ b/lms/djangoapps/ccx/tests/test_overrides.py @@ -100,7 +100,7 @@ class TestFieldOverrides(ModuleStoreTestCase): """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) chapter = self.ccx.course.get_children()[0] - with self.assertNumQueries(4): + with self.assertNumQueries(3): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) dummy = chapter.start @@ -110,7 +110,7 @@ class TestFieldOverrides(ModuleStoreTestCase): """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) chapter = self.ccx.course.get_children()[0] - with self.assertNumQueries(4): + with self.assertNumQueries(3): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) dummy1 = chapter.start dummy2 = chapter.start diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 78300ab5de..d9a8182017 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -21,6 +21,7 @@ from django.test.utils import override_settings from django.test import RequestFactory from edxmako.shortcuts import render_to_response # pylint: disable=import-error from student.models import CourseEnrollment +from request_cache.middleware import RequestCache from student.roles import CourseCcxCoachRole # pylint: disable=import-error from student.tests.factories import ( # pylint: disable=import-error AdminFactory, @@ -503,26 +504,6 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): role.add_users(coach) ccx = CcxFactory(course_id=course.id, coach=self.coach) - # Apparently the test harness doesn't use LmsFieldStorage, and I'm not - # sure if there's a way to poke the test harness to do so. So, we'll - # just inject the override field storage in this brute force manner. - OverrideFieldData.provider_classes = None - # pylint: disable=protected-access - for block in iter_blocks(course): - block._field_data = OverrideFieldData.wrap(coach, course, block._field_data) - new_cache = {'tabs': [], 'discussion_topics': []} - if 'grading_policy' in block._field_data_cache: - new_cache['grading_policy'] = block._field_data_cache['grading_policy'] - block._field_data_cache = new_cache - - def cleanup_provider_classes(): - """ - After everything is done, clean up by un-doing the change to the - OverrideFieldData object that is done during the wrap method. - """ - OverrideFieldData.provider_classes = None - self.addCleanup(cleanup_provider_classes) - # override course grading policy and make last section invisible to students override_field_for_ccx(ccx, course, 'grading_policy', { 'GRADER': [ @@ -561,9 +542,13 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): self.client.login(username=coach.username, password="test") + self.addCleanup(RequestCache.clear_request_cache) + @patch('ccx.views.render_to_response', intercept_renderer) def test_gradebook(self): self.course.enable_ccx = True + RequestCache.clear_request_cache() + url = reverse( 'ccx_gradebook', kwargs={'course_id': self.ccx_key} @@ -580,6 +565,8 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): def test_grades_csv(self): self.course.enable_ccx = True + RequestCache.clear_request_cache() + url = reverse( 'ccx_grades_csv', kwargs={'course_id': self.ccx_key} @@ -591,11 +578,11 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): response.content.strip().split('\n') ) data = dict(zip(headers, row)) + self.assertTrue('HW 04' not in data) self.assertEqual(data['HW 01'], '0.75') self.assertEqual(data['HW 02'], '0.5') self.assertEqual(data['HW 03'], '0.25') self.assertEqual(data['HW Avg'], '0.5') - self.assertTrue('HW 04' not in data) @patch('courseware.views.render_to_response', intercept_renderer) def test_student_progress(self):