Merge pull request #1965 from edx/feature/christina/fields

Pass through the value (either default or inherited) that a metadata field WILL have if it is not explicitly set.

cms/templates/widgets/metadata-edit.html

@@ -8,7 +8,7 @@
     % for field_name, field_value in editable_metadata_fields.items():
     <li>
       % if field_name == 'source_code':
-        % if field_value['is_default'] is False:
+        % if field_value['explicitly_set'] is True:
             <a href="#hls-modal-${hlskey}" style="color:yellow;" id="hls-trig-${hlskey}" >Edit High Level Source</a>
         % endif
       % else:
@@ -26,8 +26,10 @@
         % if False:
             <label>Help: ${field_value['field'].help}</label>
             <label>Type: ${type(field_value['field']).__name__}</label>
-            <label>Inherited: ${field_value['is_inherited']}</label>
-            <label>Default: ${field_value['is_default']}</label>
+            <label>Inheritable: ${field_value['inheritable']}</label>
+            <label>Showing inherited value: ${field_value['inheritable'] and not field_value['explicitly_set']}</label>
+            <label>Explicitly set: ${field_value['explicitly_set']}</label>
+            <label>Default value: ${field_value['default_value']}</label>
             % if field_value['field'].values:
               <label>Possible values:</label>
               % for value in field_value['field'].values:
@@ -40,7 +42,7 @@
     % endfor
   </ul>
 
-  % if 'source_code' in editable_metadata_fields and not editable_metadata_fields['source_code']['is_default']:
+  % if 'source_code' in editable_metadata_fields and editable_metadata_fields['source_code']['explicitly_set']:
     <%include file="source-edit.html" />
   % endif
 

common/lib/xmodule/xmodule/modulestore/inheritance.py

@@ -31,15 +31,22 @@ def inherit_metadata(descriptor, model_data):
     Only metadata specified in self.inheritable_metadata will
     be inherited
     """
+    # The inherited values that are actually being used.
     if not hasattr(descriptor, '_inherited_metadata'):
         setattr(descriptor, '_inherited_metadata', {})
 
+    # All inheritable metadata values (for which a value exists in model_data).
+    if not hasattr(descriptor, '_inheritable_metadata'):
+        setattr(descriptor, '_inheritable_metadata', {})
+
     # Set all inheritable metadata from kwargs that are
     # in self.inheritable_metadata and aren't already set in metadata
     for attr in INHERITABLE_METADATA:
-        if attr not in descriptor._model_data and attr in model_data:
-            descriptor._inherited_metadata[attr] = model_data[attr]
-            descriptor._model_data[attr] = model_data[attr]
+        if attr in model_data:
+            descriptor._inheritable_metadata[attr] = model_data[attr]
+            if attr not in descriptor._model_data:
+                descriptor._inherited_metadata[attr] = model_data[attr]
+                descriptor._model_data[attr] = model_data[attr]
 
 
 def own_metadata(module):

common/lib/xmodule/xmodule/tests/test_import.py

@@ -151,6 +151,10 @@ class ImportTestCase(BaseCourseTestCase):
         # Check that the child inherits due correctly
         child = descriptor.get_children()[0]
         self.assertEqual(child.lms.due, Date().from_json(v))
+        self.assertEqual(child._inheritable_metadata, child._inherited_metadata)
+        self.assertEqual(2, len(child._inherited_metadata))
+        self.assertEqual('1970-01-01T00:00:00Z', child._inherited_metadata['start'])
+        self.assertEqual(v, child._inherited_metadata['due'])
 
         # Now export and check things
         resource_fs = MemoryFS()
@@ -184,6 +188,60 @@ class ImportTestCase(BaseCourseTestCase):
         self.assertEqual(chapter_xml.tag, 'chapter')
         self.assertFalse('due' in chapter_xml.attrib)
 
+    def test_metadata_no_inheritance(self):
+        """
+        Checks that default value of None (for due) does not get marked as inherited.
+        """
+        system = self.get_system()
+        url_name = 'test1'
+        start_xml = '''
+        <course org="{org}" course="{course}"
+                url_name="{url_name}" unicorn="purple">
+            <chapter url="hi" url_name="ch" display_name="CH">
+                <html url_name="h" display_name="H">Two houses, ...</html>
+            </chapter>
+        </course>'''.format(org=ORG, course=COURSE, url_name=url_name)
+        descriptor = system.process_xml(start_xml)
+        compute_inherited_metadata(descriptor)
+
+        self.assertEqual(descriptor.lms.due, None)
+
+        # Check that the child does not inherit a value for due
+        child = descriptor.get_children()[0]
+        self.assertEqual(child.lms.due, None)
+        self.assertEqual(child._inheritable_metadata, child._inherited_metadata)
+        self.assertEqual(1, len(child._inherited_metadata))
+        self.assertEqual('1970-01-01T00:00:00Z', child._inherited_metadata['start'])
+
+    def test_metadata_override_default(self):
+        """
+        Checks that due date can be overriden at child level.
+        """
+        system = self.get_system()
+        course_due = 'March 20 17:00'
+        child_due = 'April 10 00:00'
+        url_name = 'test1'
+        start_xml = '''
+        <course org="{org}" course="{course}"
+                due="{due}" url_name="{url_name}" unicorn="purple">
+            <chapter url="hi" url_name="ch" display_name="CH">
+                <html url_name="h" display_name="H">Two houses, ...</html>
+            </chapter>
+        </course>'''.format(due=course_due, org=ORG, course=COURSE, url_name=url_name)
+        descriptor = system.process_xml(start_xml)
+        child = descriptor.get_children()[0]
+        child._model_data['due'] = child_due
+        compute_inherited_metadata(descriptor)
+
+        self.assertEqual(descriptor.lms.due, Date().from_json(course_due))
+        self.assertEqual(child.lms.due, Date().from_json(child_due))
+        # Test inherited metadata. Due does not appear here (because explicitly set on child).
+        self.assertEqual(1, len(child._inherited_metadata))
+        self.assertEqual('1970-01-01T00:00:00Z', child._inherited_metadata['start'])
+        # Test inheritable metadata. This has the course inheritable value for due.
+        self.assertEqual(2, len(child._inheritable_metadata))
+        self.assertEqual(course_due, child._inheritable_metadata['due'])
+
     def test_is_pointer_tag(self):
         """
         Check that is_pointer_tag works properly.

common/lib/xmodule/xmodule/tests/test_xml_module.py

@@ -9,13 +9,13 @@ from mock import Mock
 
 class TestFields(object):
     # Will be returned by editable_metadata_fields.
-    max_attempts = StringyInteger(scope=Scope.settings)
+    max_attempts = StringyInteger(scope=Scope.settings, default=1000)
     # Will not be returned by editable_metadata_fields because filtered out by non_editable_metadata_fields.
     due = Date(scope=Scope.settings)
     # Will not be returned by editable_metadata_fields because is not Scope.settings.
     student_answers = Object(scope=Scope.user_state)
     # Will be returned, and can override the inherited value from XModule.
-    display_name = String(scope=Scope.settings)
+    display_name = String(scope=Scope.settings, default='local default')
 
 
 class EditableMetadataFieldsTest(unittest.TestCase):
@@ -25,27 +25,45 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         # Tests that the xblock fields (currently tags and name) get filtered out.
         # Also tests that xml_attributes is filtered out of XmlDescriptor.
         self.assertEqual(1, len(editable_fields), "Expected only 1 editable field for xml descriptor.")
-        self.assert_display_name_default(editable_fields)
+        self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name,
+            explicitly_set=False, inheritable=False, value=None, default_value=None)
 
     def test_override_default(self):
-        # Tests that is_default is correct when a value overrides the default.
+        # Tests that explicitly_set is correct when a value overrides the default (not inheritable).
         editable_fields = self.get_xml_editable_fields({'display_name': 'foo'})
-        display_name = editable_fields['display_name']
-        self.assertFalse(display_name['is_default'])
-        self.assertEqual('foo', display_name['value'])
+        self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name,
+            explicitly_set=True, inheritable=False, value='foo', default_value=None)
 
     def test_additional_field(self):
-        editable_fields = self.get_module_editable_fields({'max_attempts' : '7'})
+        descriptor = self.get_descriptor({'max_attempts' : '7'})
+        editable_fields = descriptor.editable_metadata_fields
         self.assertEqual(2, len(editable_fields))
-        self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts, False, False, 7)
-        self.assert_display_name_default(editable_fields)
+        self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts,
+            explicitly_set=True, inheritable=False, value=7, default_value=1000)
+        self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name,
+            explicitly_set=False, inheritable=False, value='local default', default_value='local default')
 
-        editable_fields = self.get_module_editable_fields({})
-        self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts, True, False, None)
+        editable_fields = self.get_descriptor({}).editable_metadata_fields
+        self.assert_field_values(editable_fields, 'max_attempts', TestFields.max_attempts,
+            explicitly_set=False, inheritable=False, value=1000, default_value=1000)
 
     def test_inherited_field(self):
-        editable_fields = self.get_module_editable_fields({'display_name' : 'inherited'})
-        self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, False, True, 'inherited')
+        model_val = {'display_name' : 'inherited'}
+        descriptor = self.get_descriptor(model_val)
+        # Mimic an inherited value for display_name (inherited and inheritable are the same in this case).
+        descriptor._inherited_metadata = model_val
+        descriptor._inheritable_metadata = model_val
+        editable_fields = descriptor.editable_metadata_fields
+        self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name,
+            explicitly_set=False, inheritable=True, value='inherited', default_value='inherited')
+
+        descriptor = self.get_descriptor({'display_name' : 'explicit'})
+        # Mimic the case where display_name WOULD have been inherited, except we explicitly set it.
+        descriptor._inheritable_metadata = {'display_name' : 'inheritable value'}
+        descriptor._inherited_metadata = {}
+        editable_fields = descriptor.editable_metadata_fields
+        self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name,
+            explicitly_set=True, inheritable=True, value='explicit', default_value='inheritable value')
 
     # Start of helper methods
     def get_xml_editable_fields(self, model_data):
@@ -53,7 +71,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
         system.render_template = Mock(return_value="<div>Test Template HTML</div>")
         return XmlDescriptor(system=system, location=None, model_data=model_data).editable_metadata_fields
 
-    def get_module_editable_fields(self, model_data):
+    def get_descriptor(self, model_data):
         class TestModuleDescriptor(TestFields, XmlDescriptor):
 
             @property
@@ -64,16 +82,12 @@ class EditableMetadataFieldsTest(unittest.TestCase):
 
         system = test_system()
         system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-        descriptor = TestModuleDescriptor(system=system, location=None, model_data=model_data)
-        descriptor._inherited_metadata = {'display_name' : 'inherited'}
-        return descriptor.editable_metadata_fields
-
-    def assert_display_name_default(self, editable_fields):
-        self.assert_field_values(editable_fields, 'display_name', XModuleFields.display_name, True, False, None)
+        return TestModuleDescriptor(system=system, location=None, model_data=model_data)
 
-    def assert_field_values(self, editable_fields, name, field, is_default, is_inherited, value):
+    def assert_field_values(self, editable_fields, name, field, explicitly_set, inheritable, value, default_value):
         test_field = editable_fields[name]
         self.assertEqual(field, test_field['field'])
-        self.assertEqual(is_default, test_field['is_default'])
-        self.assertEqual(is_inherited, test_field['is_inherited'])
+        self.assertEqual(explicitly_set, test_field['explicitly_set'])
+        self.assertEqual(inheritable, test_field['inheritable'])
         self.assertEqual(value, test_field['value'])
+        self.assertEqual(default_value, test_field['default_value'])

common/lib/xmodule/xmodule/x_module.py

@@ -624,27 +624,28 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock):
         Can be limited by extending `non_editable_metadata_fields`.
         """
         inherited_metadata = getattr(self, '_inherited_metadata', {})
+        inheritable_metadata = getattr(self, '_inheritable_metadata', {})
         metadata = {}
         for field in self.fields:
 
             if field.scope != Scope.settings or field in self.non_editable_metadata_fields:
                 continue
 
-            inherited = False
-            default = False
+            inheritable = False
             value = getattr(self, field.name)
-            if field.name in self._model_data:
-                default = False
+            default_value = field.default
+            explicitly_set = field.name in self._model_data
+            if field.name in inheritable_metadata:
+                inheritable = True
+                default_value = field.from_json(inheritable_metadata.get(field.name))
                 if field.name in inherited_metadata:
-                    if self._model_data.get(field.name) == inherited_metadata.get(field.name):
-                        inherited = True
-            else:
-                default = True
+                    explicitly_set = False
 
             metadata[field.name] = {'field': field,
                                     'value': value,
-                                    'is_inherited': inherited,
-                                    'is_default': default}
+                                    'default_value': default_value,
+                                    'inheritable': inheritable,
+                                    'explicitly_set': explicitly_set }
 
         return metadata
 

github-requirements.txt

@@ -8,4 +8,4 @@
 -e git://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk
 
 # Our libraries:
--e git+https://github.com/edx/XBlock.git@5ce6f70a#egg=XBlock
+-e git+https://github.com/edx/XBlock.git@483e0cb1#egg=XBlock