diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index 462bf73735..bf5aa34145 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -93,7 +93,10 @@ def get_override_for_ccx(ccx, block, name, default=None): block_overrides = overrides.get(non_ccx_key, {}) if name in block_overrides: - return block.fields[name].from_json(block_overrides[name]) + try: + return block.fields[name].from_json(block_overrides[name]) + except KeyError: + return block_overrides[name] else: return default @@ -114,6 +117,8 @@ def _get_overrides_for_ccx(ccx): for override in query: block_overrides = overrides.setdefault(override.location, {}) block_overrides[override.field] = json.loads(override.value) + block_overrides[override.field + "_id"] = override.id + block_overrides[override.field + "_instance"] = override overrides_cache[ccx] = overrides @@ -130,23 +135,33 @@ def override_field_for_ccx(ccx, block, name, value): field = block.fields[name] 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=serialized_value - ) - except IntegrityError: - transaction.commit() - override = CcxFieldOverride.objects.get( - ccx=ccx, - location=block.location, - field=name - ) + override_has_changes = False + + override = get_override_for_ccx(ccx, block, name + "_instance") + if override: + override_has_changes = serialized_value != override.value + + if not override: + try: + override = CcxFieldOverride.objects.create( + ccx=ccx, + location=block.location, + field=name, + value=serialized_value + ) + _get_overrides_for_ccx(ccx).setdefault(block.location, {})[name + "_id"] = override.id + except IntegrityError: + transaction.commit() + kwargs = {'ccx': ccx, 'location': block.location, 'field': name} + override = CcxFieldOverride.objects.get(**kwargs) + override_has_changes = serialized_value != override.value + + if override_has_changes: override.value = serialized_value - override.save() + override.save() + _get_overrides_for_ccx(ccx).setdefault(block.location, {})[name] = value_json + _get_overrides_for_ccx(ccx).setdefault(block.location, {})[name + "_instance"] = override def clear_override_for_ccx(ccx, block, name): @@ -162,7 +177,30 @@ def clear_override_for_ccx(ccx, block, name): location=block.location, field=name).delete() - _get_overrides_for_ccx(ccx).setdefault(block.location, {}).pop(name) + clear_ccx_field_info_from_ccx_map(ccx, block, name) except CcxFieldOverride.DoesNotExist: pass + + +def clear_ccx_field_info_from_ccx_map(ccx, block, name): # pylint: disable=invalid-name + """ + Remove field information from ccx overrides mapping dictionary + """ + try: + ccx_override_map = _get_overrides_for_ccx(ccx).setdefault(block.location, {}) + ccx_override_map.pop(name) + ccx_override_map.pop(name + "_id") + ccx_override_map.pop(name + "_instance") + except KeyError: + pass + + +def bulk_delete_ccx_override_fields(ccx, ids): + """ + Bulk delete for CcxFieldOverride model + """ + ids = filter(None, ids) + ids = list(set(ids)) + if ids: + CcxFieldOverride.objects.filter(ccx=ccx, id__in=ids).delete() diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index d473c0aafa..dafd442872 100644 --- a/lms/djangoapps/ccx/tests/test_overrides.py +++ b/lms/djangoapps/ccx/tests/test_overrides.py @@ -94,15 +94,35 @@ class TestFieldOverrides(ModuleStoreTestCase): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) self.assertEquals(chapter.start, ccx_start) - def test_override_num_queries(self): + def test_override_num_queries_new_field(self): """ - Test that overriding and accessing a field produce same number of queries. + Test that for creating new field executed only create query """ ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) chapter = self.ccx.course.get_children()[0] - with self.assertNumQueries(3): + with self.assertNumQueries(1): + override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) + + def test_override_num_queries_update_existing_field(self): + """ + Test that overriding existing field executed create, fetch and update queries. + """ + ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) + new_ccx_start = datetime.datetime(2015, 12, 25, 00, 00, tzinfo=pytz.UTC) + chapter = self.ccx.course.get_children()[0] + override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) + with self.assertNumQueries(2): + override_field_for_ccx(self.ccx, chapter, 'start', new_ccx_start) + + def test_override_num_queries_field_value_not_changed(self): + """ + Test that if value of field does not changed no query execute. + """ + ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC) + chapter = self.ccx.course.get_children()[0] + override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) + with self.assertNumQueries(0): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) - dummy = chapter.start def test_overriden_field_access_produces_no_extra_queries(self): """ @@ -110,11 +130,8 @@ 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(3): + with self.assertNumQueries(1): override_field_for_ccx(self.ccx, chapter, 'start', ccx_start) - dummy1 = chapter.start - dummy2 = chapter.start - dummy3 = chapter.start def test_override_is_inherited(self): """ diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index f34bd8acb4..859ff2ef02 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -53,6 +53,8 @@ from .overrides import ( clear_override_for_ccx, get_override_for_ccx, override_field_for_ccx, + clear_ccx_field_info_from_ccx_map, + bulk_delete_ccx_override_fields, ) @@ -196,42 +198,50 @@ def save_ccx(request, course, ccx=None): if not ccx: raise Http404 - def override_fields(parent, data, graded, earliest=None): + def override_fields(parent, data, graded, earliest=None, ccx_ids_to_delete=None): """ Recursively apply CCX schedule data to CCX by overriding the `visible_to_staff_only`, `start` and `due` fields for units in the course. """ + if ccx_ids_to_delete is None: + ccx_ids_to_delete = [] blocks = { str(child.location): child for child in parent.get_children()} + for unit in data: block = blocks[unit['location']] override_field_for_ccx( ccx, block, 'visible_to_staff_only', unit['hidden']) + start = parse_date(unit['start']) if start: if not earliest or start < earliest: earliest = start override_field_for_ccx(ccx, block, 'start', start) else: - clear_override_for_ccx(ccx, block, 'start') + ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'start_id')) + clear_ccx_field_info_from_ccx_map(ccx, block, 'start') + due = parse_date(unit['due']) if due: override_field_for_ccx(ccx, block, 'due', due) else: - clear_override_for_ccx(ccx, block, 'due') + ccx_ids_to_delete.append(get_override_for_ccx(ccx, block, 'due_id')) + clear_ccx_field_info_from_ccx_map(ccx, block, 'due') if not unit['hidden'] and block.graded: graded[block.format] = graded.get(block.format, 0) + 1 children = unit.get('children', None) if children: - override_fields(block, children, graded, earliest) - return earliest + override_fields(block, children, graded, earliest, ccx_ids_to_delete) + return earliest, ccx_ids_to_delete graded = {} - earliest = override_fields(course, json.loads(request.body), graded) + earliest, ccx_ids_to_delete = override_fields(course, json.loads(request.body), graded, []) + bulk_delete_ccx_override_fields(ccx, ccx_ids_to_delete) if earliest: override_field_for_ccx(ccx, course, 'start', earliest)