save_cxx slow transaction

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()

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):
         """

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)