Merge pull request #295 from edx/eugeny/import-export-fix

EXPLICITLY_SET always overrides _dirty_fields content

xblock/fields.py

@@ -405,7 +405,7 @@ def _mark_dirty(self, xblock, value):
 
         # Deep copy the value being marked as dirty, so that there
         # is a baseline to check against when saving later
-        if self not in xblock._dirty_fields:
+        if self not in xblock._dirty_fields or value is EXPLICITLY_SET:
             xblock._dirty_fields[self] = copy.deepcopy(value)
 
     def _is_dirty(self, xblock):

xblock/test/test_fields.py

@@ -26,7 +26,9 @@
     UNIQUE_ID
 )
 
-from xblock.test.tools import assert_equals, assert_not_equals, assert_not_in, TestRuntime
+from xblock.test.tools import (
+    assert_equals, assert_not_equals, assert_in, assert_not_in, assert_false, assert_true, TestRuntime
+)
 from xblock.fields import scope_key, ScopeIds
 
 
@@ -544,6 +546,39 @@ class FieldTester(XBlock):
     assert_not_in('how_many', field_tester._get_fields_to_save())   # pylint: disable=W0212
 
 
+def test_setting_the_same_value_marks_field_as_dirty():
+    """
+    Check that setting field to the same value does not mark mutable fields as dirty.
+    This might be an unexpected behavior though
+    """
+    class FieldTester(XBlock):
+        """Test block for set - get test."""
+        non_mutable = String(scope=Scope.settings)
+        list_field = List(scope=Scope.settings)
+        dict_field = Dict(scope=Scope.settings)
+
+    runtime = TestRuntime(services={'field-data': DictFieldData({})})
+    field_tester = FieldTester(runtime, scope_ids=Mock(spec=ScopeIds))
+
+    # precondition checks
+    assert_equals(len(field_tester._dirty_fields), 0)
+    assert_false(field_tester.fields['list_field'].is_set_on(field_tester))
+    assert_false(field_tester.fields['dict_field'].is_set_on(field_tester))
+    assert_false(field_tester.fields['non_mutable'].is_set_on(field_tester))
+
+    field_tester.non_mutable = field_tester.non_mutable
+    field_tester.list_field = field_tester.list_field
+    field_tester.dict_field = field_tester.dict_field
+
+    assert_in(field_tester.fields['non_mutable'], field_tester._dirty_fields)
+    assert_in(field_tester.fields['list_field'], field_tester._dirty_fields)
+    assert_in(field_tester.fields['dict_field'], field_tester._dirty_fields)
+
+    assert_true(field_tester.fields['non_mutable'].is_set_on(field_tester))
+    assert_true(field_tester.fields['list_field'].is_set_on(field_tester))
+    assert_true(field_tester.fields['dict_field'].is_set_on(field_tester))
+
+
 class SentinelTest(unittest.TestCase):
     """
     Tests of :ref:`xblock.fields.Sentinel`.

xblock/test/test_fields_api.py

@@ -25,7 +25,7 @@
 """
 
 import copy
-from mock import Mock
+from mock import Mock, patch
 
 from xblock.core import XBlock
 from xblock.fields import Integer, List, String, ScopeIds, UNIQUE_ID
@@ -163,6 +163,13 @@ def test_delete_with_save_writes(self):
         assert_false(self.field_data.has(self.block, 'field'))
         assert_true(self.is_default())
 
+    def test_set_after_get_always_saves(self):
+        with patch.object(self.field_data, 'set_many') as patched_set_many:
+            self.set(self.get())
+            self.block.save()
+
+            patched_set_many.assert_called_with(self.block, {'field': self.get()})
+
 
 class MutationProperties(object):
     """