Merge pull request #301 from edx/adam/add_force_save_fields_2

Adam/add force save fields 2

AUTHORS

@@ -34,3 +34,4 @@ Eugeny Kolpakov <eugeny.kolpakov@gmail.com>
 David Bodor <david.gabor.bodor@gmail.com>
 Dhruv Baldawa <dhruvbaldawa@gmail.com>
 Matjaz Gregoric <mtyaka@gmail.com>
+Adam Palay <adam@edx.org>

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 or value is EXPLICITLY_SET:
+        if self not in xblock._dirty_fields:
             xblock._dirty_fields[self] = copy.deepcopy(value)
 
     def _is_dirty(self, xblock):
@@ -498,11 +498,22 @@ def __set__(self, xblock, value):
         Setting a value does not update the underlying data store; the
         new value is kept in the cache and the xblock is marked as
         dirty until `save` is explicitly called.
+
+        Note, if there's already a cached value and it's equal to the value
+        we're trying to cache, we won't do anything.
         """
         value = self._check_or_enforce_type(value)
-        # Mark the field as dirty and update the cache:
-        self._mark_dirty(xblock, EXPLICITLY_SET)
-        self._set_cached_value(xblock, value)
+        cached_value = self._get_cached_value(xblock)
+        try:
+            value_has_changed = cached_value != value
+        except Exception:  # pylint: disable=broad-except
+            # if we can't compare the values for whatever reason
+            # (i.e. timezone aware and unaware datetimes), just reset the value.
+            value_has_changed = True
+        if value_has_changed:
+            # Mark the field as dirty and update the cache
+            self._mark_dirty(xblock, EXPLICITLY_SET)
+            self._set_cached_value(xblock, value)
 
     def __delete__(self, xblock):
         """

xblock/mixins.py

@@ -7,6 +7,7 @@
 import inspect
 import logging
 from lxml import etree
+import copy
 
 try:
     import simplesjson as json  # pylint: disable=F0401
@@ -244,40 +245,61 @@ def save(self):
         if not self._dirty_fields:
             # nop if _dirty_fields attribute is empty
             return
+
+        fields_to_save = self._get_fields_to_save()
+        if fields_to_save:
+            self.force_save_fields(fields_to_save)
+
+    def force_save_fields(self, field_names):
+        """
+        Save all fields that are specified in `field_names`, even
+        if they are not dirty.
+        """
+        fields = [self.fields[field_name] for field_name in field_names]
+        fields_to_save_json = {}
+        for field in fields:
+            fields_to_save_json[field.name] = field.to_json(self._field_data_cache[field.name])
+
         try:
-            fields_to_save = self._get_fields_to_save()
             # Throws KeyValueMultiSaveError if things go wrong
-            self._field_data.set_many(self, fields_to_save)
-
+            self._field_data.set_many(self, fields_to_save_json)
         except KeyValueMultiSaveError as save_error:
-            saved_fields = [field for field in self._dirty_fields if field.name in save_error.saved_field_names]
+            saved_fields = [field for field in fields if field.name in save_error.saved_field_names]
             for field in saved_fields:
                 # should only find one corresponding field
-                del self._dirty_fields[field]
-            raise XBlockSaveError(saved_fields, self._dirty_fields.keys())
+                fields.remove(field)
+                # if the field was dirty, delete from dirty fields
+                self._reset_dirty_field(field)
+            raise XBlockSaveError(saved_fields, fields)
 
         # Remove all dirty fields, since the save was successful
-        self._clear_dirty_fields()
+        for field in fields:
+            self._reset_dirty_field(field)
 
     def _get_fields_to_save(self):
         """
-        Create dictionary mapping between dirty fields and data cache values.
-        A `field` is an instance of `Field`.
+        Get an xblock's dirty fields.
         """
-        fields_to_save = {}
-        for field in self._dirty_fields.keys():
-            # If the field value isn't the same as the baseline we recorded
-            # when it was read, then save it
-            if field._is_dirty(self):  # pylint: disable=protected-access
-                fields_to_save[field.name] = field.to_json(self._field_data_cache[field.name])
-        return fields_to_save
+        # If the field value isn't the same as the baseline we recorded
+        # when it was read, then save it
+        # pylint: disable=protected-access
+        return [field.name for field in self._dirty_fields if field._is_dirty(self)]
 
     def _clear_dirty_fields(self):
         """
         Remove all dirty fields from an XBlock.
         """
         self._dirty_fields.clear()
 
+    def _reset_dirty_field(self, field):
+        """
+        Resets dirty field value with the value from the field data cache.
+        """
+        if field in self._dirty_fields:
+            self._dirty_fields[field] = copy.deepcopy(
+                self._field_data_cache[field.name]
+            )
+
     def __repr__(self):
         # `ScopedStorageMixin` obtains the `fields` attribute from the `ModelMetaclass`.
         # Since this is not understood by static analysis, silence this error.

xblock/test/test_core.py

@@ -640,7 +640,8 @@ class MutableTester(XBlock):
 
     # Now test after having explicitly set the field.
     mutable_test.save()
-    assert_equals(len(mutable_test._dirty_fields), 0)
+    # _dirty_fields shouldn't be cleared here
+    assert_equals(len(mutable_test._dirty_fields), 1)
     _test_get = mutable_test.list_field
     assert_equals(len(mutable_test._dirty_fields), 1)
 

xblock/test/test_fields.py

@@ -27,7 +27,7 @@
 )
 
 from xblock.test.tools import (
-    assert_equals, assert_not_equals, assert_in, assert_not_in, assert_false, assert_true, TestRuntime
+    assert_equals, assert_not_equals, assert_in, assert_not_in, assert_false, TestRuntime
 )
 from xblock.fields import scope_key, ScopeIds
 
@@ -514,6 +514,23 @@ def test_values_dict():
     assert_equals({"min": 1, "max": 100}, test_field.values)
 
 
+def test_set_incomparable_fields():
+    # if we can't compare a field's value to the value it's going to be reset to
+    # (i.e. timezone aware and unaware datetimes), just reset the value.
+
+    class FieldTester(XBlock):
+        """Test block for this test."""
+        incomparable = Field(scope=Scope.settings)
+
+    not_timezone_aware = dt.datetime(2015, 1, 1)
+    timezone_aware = dt.datetime(2015, 1, 1, tzinfo=pytz.UTC)
+    runtime = TestRuntime(services={'field-data': DictFieldData({})})
+    field_tester = FieldTester(runtime, scope_ids=Mock(spec=ScopeIds))
+    field_tester.incomparable = not_timezone_aware
+    field_tester.incomparable = timezone_aware
+    assert_equals(field_tester.incomparable, timezone_aware)
+
+
 def test_twofaced_field_access():
     # Check that a field with different to_json and from_json representations
     # persists and saves correctly.
@@ -548,8 +565,8 @@ class FieldTester(XBlock):
 
 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
+    Check that setting field to the same value marks mutable fields as dirty.
+    However, since the value hasn't changed, these fields won't be saved.
     """
     class FieldTester(XBlock):
         """Test block for set - get test."""
@@ -570,13 +587,13 @@ class FieldTester(XBlock):
     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_not_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))
+    assert_false(field_tester.fields['non_mutable'].is_set_on(field_tester))
+    assert_false(field_tester.fields['list_field'].is_set_on(field_tester))
+    assert_false(field_tester.fields['dict_field'].is_set_on(field_tester))
 
 
 class SentinelTest(unittest.TestCase):

xblock/test/test_fields_api.py

@@ -163,12 +163,26 @@ 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):
+    def test_set_after_get_always_force_saves(self):
         with patch.object(self.field_data, 'set_many') as patched_set_many:
             self.set(self.get())
+
+            self.block.force_save_fields(['field'])
+
+            patched_set_many.assert_called_with(
+                self.block, {'field': self.get()}
+            )
+
+    def test_set_after_get_doesnt_save(self):
+        with patch.object(self.field_data, 'set_many') as patched_set_many:
+
+            self.set(self.get())
             self.block.save()
+            assert_false(patched_set_many.called)
 
-            patched_set_many.assert_called_with(self.block, {'field': self.get()})
+            self.set(self.new_value)
+            self.block.save()
+            assert_true(patched_set_many.called)
 
 
 class MutationProperties(object):
@@ -206,6 +220,27 @@ def test_mutation_without_save_makes_non_default(self):
         self.mutate(self.get())
         assert_false(self.is_default())
 
+    def test_mutate_pointer_after_save(self):
+        pointer = self.get()
+        self.mutate(pointer)
+        self.block.save()
+        assert_equals(pointer, self.field_data.get(self.block, 'field'))
+
+        # now check what happens when we mutate a field
+        # that we haven't retrieved through __get__
+        # (which would have marked it as dirty)
+        self.mutate(pointer)
+        self.block.save()
+        assert_equals(pointer, self.field_data.get(self.block, 'field'))
+
+    def test_set_save_mutate_save(self):
+        pointer = self.new_value
+        self.set(pointer)
+        self.block.save()
+        self.mutate(pointer)
+        self.block.save()
+        assert_equals(pointer, self.field_data.get(self.block, 'field'))
+
 
 class InitialValueProperties(object):
     """