From be80ef3a7f9bd909cb42e2058cb1e8fb7bdef0f6 Mon Sep 17 00:00:00 2001 From: Qubad786 Date: Thu, 25 Aug 2016 16:55:40 +0500 Subject: [PATCH] prioritize Library's defaults over inherited ones. --- .../xmodule/modulestore/inheritance.py | 19 +++ .../xmodule/modulestore/split_mongo/split.py | 4 +- .../split_mongo/split_mongo_kvs.py | 8 +- .../xmodule/xmodule/tests/test_xml_module.py | 130 +++++++++++++++--- 4 files changed, 139 insertions(+), 22 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index bd2089578f..21fda25534 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -283,6 +283,16 @@ class InheritingFieldData(KvsFieldData): super(InheritingFieldData, self).__init__(**kwargs) self.inheritable_names = set(inheritable_names) + def has_default_value(self, name): + """ + Return whether or not the field `name` has a default value + """ + has_default_value = getattr(self._kvs, 'has_default_value', False) + if callable(has_default_value): + return has_default_value(name) + + return has_default_value + def default(self, block, name): """ The default for an inheritable name is found on a parent. @@ -294,6 +304,15 @@ class InheritingFieldData(KvsFieldData): # node of the tree, the block's default will be used. field = block.fields[name] ancestor = block.get_parent() + # In case, if block's parent is of type 'library_content', + # bypass inheritance and use kvs' default instead of reusing + # from parent as '_copy_from_templates' puts fields into + # defaults. + if ancestor and \ + ancestor.location.category == 'library_content' and \ + self.has_default_value(name): + return super(InheritingFieldData, self).default(block, name) + while ancestor is not None: if field.is_set_on(ancestor): return field.read_json(ancestor) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index b186ca82ec..8d4549a6d1 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -2517,7 +2517,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): del new_block_info.defaults['markdown'] # - new_block_info.fields = existing_block_info.fields # Preserve any existing overrides + # Preserve any existing overrides + new_block_info.fields = existing_block_info.fields + if 'children' in new_block_info.defaults: del new_block_info.defaults['children'] # Will be set later diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py index c40f44f78f..417f383d24 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py @@ -136,7 +136,7 @@ class SplitMongoKVS(InheritanceKeyValueStore): def has(self, key): """ - Is the given field explicitly set in this kvs (not inherited nor default) + Is the given field explicitly set in this kvs (neither inherited nor default) """ # handle any special cases if key.scope not in self.VALID_SCOPES: @@ -158,6 +158,12 @@ class SplitMongoKVS(InheritanceKeyValueStore): # if someone changes it so that they do, then change any tests of field.name in xx._field_data return key.field_name in self._fields + def has_default_value(self, field_name): + """ + Is the given field has default value in this kvs + """ + return field_name in self._defaults + def default(self, key): """ Check to see if the default should be from the template's defaults (if any) diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index 0335031d87..dcc0f2310d 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -5,6 +5,7 @@ import unittest from mock import Mock from nose.tools import assert_equals, assert_not_equals, assert_true, assert_false, assert_in, assert_not_in # pylint: disable=no-name-in-module +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from xblock.field_data import DictFieldData from xblock.fields import Scope, String, Dict, Boolean, Integer, Float, Any, List @@ -12,6 +13,7 @@ from xblock.runtime import KvsFieldData, DictKeyValueStore from xmodule.fields import Date, Timedelta, RelativeTime from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin, InheritingFieldData +from xmodule.modulestore.split_mongo.split_mongo_kvs import SplitMongoKVS from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field from xmodule.course_module import CourseDescriptor from xmodule.seq_module import SequenceDescriptor @@ -56,15 +58,20 @@ class TestFields(object): class InheritingFieldDataTest(unittest.TestCase): - """Tests of InheritingFieldData.""" + """ + Tests of InheritingFieldData. + """ class TestableInheritingXBlock(XmlDescriptor): - """An XBlock we can use in these tests.""" + """ + An XBlock we can use in these tests. + """ inherited = String(scope=Scope.settings, default="the default") not_inherited = String(scope=Scope.settings, default="nothing") def setUp(self): super(InheritingFieldDataTest, self).setUp() + self.dummy_course_key = CourseLocator('test_org', 'test_123', 'test_run') self.system = get_test_descriptor_system() self.all_blocks = {} self.system.get_block = self.all_blocks.get @@ -73,11 +80,34 @@ class InheritingFieldDataTest(unittest.TestCase): kvs=DictKeyValueStore({}), ) + def get_block_using_split_kvs(self, block_type, block_id, fields, defaults): + """ + Construct an Xblock with split mongo kvs. + """ + kvs = SplitMongoKVS( + definition=Mock(), + initial_values=fields, + default_values=defaults, + parent=None + ) + self.field_data = InheritingFieldData( + inheritable_names=['inherited'], + kvs=kvs, + ) + block = self.get_a_block( + usage_id=self.get_usage_id(block_type, block_id) + ) + + return block + def get_a_block(self, usage_id=None): - """Construct an XBlock for testing with.""" + """ + Construct an XBlock for testing with. + """ scope_ids = Mock() if usage_id is None: - usage_id = "_auto%d" % len(self.all_blocks) + block_id = "_auto{id}".format(id=len(self.all_blocks)) + usage_id = self.get_usage_id("course", block_id) scope_ids.usage_id = usage_id block = self.system.construct_xblock_from_class( self.TestableInheritingXBlock, @@ -87,14 +117,24 @@ class InheritingFieldDataTest(unittest.TestCase): self.all_blocks[usage_id] = block return block + def get_usage_id(self, block_type, block_id): + """ + Constructs usage id using 'block_type' and 'block_id' + """ + return BlockUsageLocator(self.dummy_course_key, block_type=block_type, block_id=block_id) + def test_default_value(self): - # Blocks with nothing set with return the fields' defaults. + """ + Test that the Blocks with nothing set with return the fields' defaults. + """ block = self.get_a_block() self.assertEqual(block.inherited, "the default") self.assertEqual(block.not_inherited, "nothing") def test_set_value(self): - # If you set a value, that's what you get back. + """ + Test that If you set a value, that's what you get back. + """ block = self.get_a_block() block.inherited = "Changed!" block.not_inherited = "New Value!" @@ -102,36 +142,86 @@ class InheritingFieldDataTest(unittest.TestCase): self.assertEqual(block.not_inherited, "New Value!") def test_inherited(self): - # A child with get a value inherited from the parent. - parent = self.get_a_block(usage_id="parent") - parent.inherited = "Changed!" - self.assertEqual(parent.inherited, "Changed!") + """ + Test that a child with get a value inherited from the parent. + """ + parent_block = self.get_a_block(usage_id=self.get_usage_id("course", "parent")) + parent_block.inherited = "Changed!" + self.assertEqual(parent_block.inherited, "Changed!") - child = self.get_a_block(usage_id="child") - child.parent = "parent" + child = self.get_a_block(usage_id=self.get_usage_id("vertical", "child")) + child.parent = parent_block.location self.assertEqual(child.inherited, "Changed!") def test_inherited_across_generations(self): - # A child with get a value inherited from a great-grandparent. - parent = self.get_a_block(usage_id="parent") + """ + Test that a child with get a value inherited from a great-grandparent. + """ + parent = self.get_a_block(usage_id=self.get_usage_id("course", "parent")) parent.inherited = "Changed!" self.assertEqual(parent.inherited, "Changed!") for child_num in range(10): - usage_id = "child_{}".format(child_num) + usage_id = self.get_usage_id("vertical", "child_{}".format(child_num)) child = self.get_a_block(usage_id=usage_id) - child.parent = "parent" + child.parent = parent.location self.assertEqual(child.inherited, "Changed!") def test_not_inherited(self): - # Fields not in the inherited_names list won't be inherited. - parent = self.get_a_block(usage_id="parent") + """ + Test that the fields not in the inherited_names list won't be inherited. + """ + parent = self.get_a_block(usage_id=self.get_usage_id("course", "parent")) parent.not_inherited = "Changed!" self.assertEqual(parent.not_inherited, "Changed!") - child = self.get_a_block(usage_id="child") - child.parent = "parent" + child = self.get_a_block(usage_id=self.get_usage_id("vertical", "child")) + child.parent = parent.location self.assertEqual(child.not_inherited, "nothing") + def test_non_defaults_inherited_across_lib(self): + """ + Test that a child inheriting from library_content block, inherits fields + from parent if these fields are not in its defaults. + """ + parent_block = self.get_block_using_split_kvs( + block_type="library_content", + block_id="parent", + fields=dict(inherited="changed!"), + defaults=dict(inherited="parent's default"), + ) + self.assertEqual(parent_block.inherited, "changed!") + + child = self.get_block_using_split_kvs( + block_type="problem", + block_id="child", + fields={}, + defaults={}, + ) + child.parent = parent_block.location + self.assertEqual(child.inherited, "changed!") + + def test_defaults_not_inherited_across_lib(self): + """ + Test that a child inheriting from library_content block, does not inherit + fields from parent if these fields are in its defaults already. + """ + parent_block = self.get_block_using_split_kvs( + block_type="library_content", + block_id="parent", + fields=dict(inherited="changed!"), + defaults=dict(inherited="parent's default"), + ) + self.assertEqual(parent_block.inherited, "changed!") + + child = self.get_block_using_split_kvs( + block_type="library_content", + block_id="parent", + fields={}, + defaults=dict(inherited="child's default"), + ) + child.parent = parent_block.location + self.assertEqual(child.inherited, "child's default") + class EditableMetadataFieldsTest(unittest.TestCase): def test_display_name_field(self):