Fix tests except for conditional module and open ended grading

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -6,6 +6,7 @@ from django.conf import settings
 from django.core.urlresolvers import reverse
 from path import path
 from tempdir import mkdtemp_clean
+from datetime import timedelta
 import json
 from fs.osfs import OSFS
 import copy
@@ -27,6 +28,7 @@ from xmodule.contentstore.django import contentstore
 from xmodule.templates import update_templates
 from xmodule.modulestore.xml_exporter import export_to_xml
 from xmodule.modulestore.xml_importer import import_from_xml
+from xmodule.modulestore.inheritance import own_metadata
 from xmodule.templates import update_templates
 
 from xmodule.capa_module import CapaDescriptor
@@ -218,7 +220,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         # compare what's on disk compared to what we have in our course
         with fs.open('grading_policy.json','r') as grading_policy:
             on_disk = loads(grading_policy.read())
-            self.assertEqual(on_disk, course.definition['data']['grading_policy'])
+            self.assertEqual(on_disk, course.grading_policy)
 
         #check for policy.json
         self.assertTrue(fs.exists('policy.json'))
@@ -227,7 +229,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         with fs.open('policy.json','r') as course_policy:
             on_disk = loads(course_policy.read())
             self.assertIn('course/6.002_Spring_2012', on_disk)
-            self.assertEqual(on_disk['course/6.002_Spring_2012'], course.metadata)
+            self.assertEqual(on_disk['course/6.002_Spring_2012'], own_metadata(course))
 
         # remove old course
         delete_course(ms, cs, location)
@@ -444,8 +446,7 @@ class ContentStoreTest(ModuleStoreTestCase):
 
         # let's assert on the metadata_inheritance on an existing vertical
         for vertical in verticals:
-            self.assertIn('xqa_key', vertical.metadata)
-            self.assertEqual(course.metadata['xqa_key'], vertical.metadata['xqa_key'])
+            self.assertEqual(course.lms.xqa_key, vertical.lms.xqa_key)
 
         self.assertGreater(len(verticals), 0)
 
@@ -455,31 +456,28 @@ class ContentStoreTest(ModuleStoreTestCase):
         # crate a new module and add it as a child to a vertical
         ms.clone_item(source_template_location, new_component_location)
         parent = verticals[0]
-        ms.update_children(parent.location, parent.definition.get('children', []) + [new_component_location.url()])
+        ms.update_children(parent.location, parent.children + [new_component_location.url()])
 
         # flush the cache
         ms.get_cached_metadata_inheritance_tree(new_component_location, -1)
         new_module = ms.get_item(new_component_location)
 
         # check for grace period definition which should be defined at the course level
-        self.assertIn('graceperiod', new_module.metadata)
+        self.assertEqual(parent.lms.graceperiod, new_module.lms.graceperiod)
 
-        self.assertEqual(parent.metadata['graceperiod'], new_module.metadata['graceperiod'])
-
-        self.assertEqual(course.metadata['xqa_key'], new_module.metadata['xqa_key'])
+        self.assertEqual(course.lms.xqa_key, new_module.lms.xqa_key)
 
         #
         # now let's define an override at the leaf node level
         #
-        new_module.metadata['graceperiod'] = '1 day'
-        ms.update_metadata(new_module.location, new_module.metadata)
+        new_module.lms.graceperiod = timedelta(1)
+        ms.update_metadata(new_module.location, own_metadata(new_module))
 
         # flush the cache and refetch
         ms.get_cached_metadata_inheritance_tree(new_component_location, -1)
         new_module = ms.get_item(new_component_location)
 
-        self.assertIn('graceperiod', new_module.metadata)
-        self.assertEqual('1 day', new_module.metadata['graceperiod'])
+        self.assertEqual(timedelta(1), new_module.lms.graceperiod)
 
 
 class TemplateTestCase(ModuleStoreTestCase):

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -287,31 +287,31 @@ class CourseMetadataEditingTest(CourseTestCase):
 
     def test_update_from_json(self):
         test_model = CourseMetadata.update_from_json(self.course_location,
-            { "a" : 1,
-              "b_a_c_h" : { "c" : "test" },
-              "test_text" : "a text string"})
+            { "advertised_start" : "start A",
+              "testcenter_info" : { "c" : "test" },
+              "days_early_for_beta" : 2})
         self.update_check(test_model)
         # try fresh fetch to ensure persistence
         test_model = CourseMetadata.fetch(self.course_location)
         self.update_check(test_model)
         # now change some of the existing metadata
         test_model = CourseMetadata.update_from_json(self.course_location,
-            { "a" : 2,
+            { "advertised_start" : "start B",
               "display_name" : "jolly roger"})
         self.assertIn('display_name', test_model, 'Missing editable metadata field')
         self.assertEqual(test_model['display_name'], 'jolly roger', "not expected value")
-        self.assertIn('a', test_model, 'Missing revised a metadata field')
-        self.assertEqual(test_model['a'], 2, "a not expected value")
+        self.assertIn('advertised_start', test_model, 'Missing revised advertised_start metadata field')
+        self.assertEqual(test_model['advertised_start'], 'start B', "advertised_start not expected value")
 
     def update_check(self, test_model):
         self.assertIn('display_name', test_model, 'Missing editable metadata field')
         self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value")
-        self.assertIn('a', test_model, 'Missing new a metadata field')
-        self.assertEqual(test_model['a'], 1, "a not expected value")
-        self.assertIn('b_a_c_h', test_model, 'Missing b_a_c_h metadata field')
-        self.assertDictEqual(test_model['b_a_c_h'], { "c" : "test" }, "b_a_c_h not expected value")
-        self.assertIn('test_text', test_model, 'Missing test_text metadata field')
-        self.assertEqual(test_model['test_text'], "a text string", "test_text not expected value")
+        self.assertIn('advertised_start', test_model, 'Missing new advertised_start metadata field')
+        self.assertEqual(test_model['advertised_start'], 'start A', "advertised_start not expected value")
+        self.assertIn('testcenter_info', test_model, 'Missing testcenter_info metadata field')
+        self.assertDictEqual(test_model['testcenter_info'], { "c" : "test" }, "testcenter_info not expected value")
+        self.assertIn('days_early_for_beta', test_model, 'Missing days_early_for_beta metadata field')
+        self.assertEqual(test_model['days_early_for_beta'], 2, "days_early_for_beta not expected value")
 
 
     def test_delete_key(self):
@@ -322,5 +322,5 @@ class CourseMetadataEditingTest(CourseTestCase):
         self.assertEqual(test_model['display_name'], 'Testing', "not expected value")
         self.assertIn('rerandomize', test_model, 'Missing rerandomize metadata field')
         # check for deletion effectiveness
-        self.assertNotIn('showanswer', test_model, 'showanswer field still in')
-        self.assertNotIn('xqa_key', test_model, 'xqa_key field still in')
+        self.assertEqual('closed', test_model['showanswer'], 'showanswer field still in')
+        self.assertEqual(None, test_model['xqa_key'], 'xqa_key field still in')

cms/djangoapps/contentstore/views.py

@@ -464,6 +464,9 @@ class SessionKeyValueStore(KeyValueStore):
         except (KeyError, InvalidScopeError):
             del self._session[tuple(key)]
 
+    def has(self, key):
+        return key in self._model_data or key in self._session
+
 
 def preview_module_system(request, preview_id, descriptor):
     """
@@ -662,7 +665,7 @@ def save_item(request):
 
         # commit to datastore
         # TODO (cpennington): This really shouldn't have to do this much reaching in to get the metadata
-        store.update_metadata(item_location, existing_item._model_data._kvs._metadata)
+        store.update_metadata(item_location, own_metadat(existing_item))
 
     return HttpResponse()
 

cms/djangoapps/models/settings/course_metadata.py

 from xmodule.modulestore import Location
 from contentstore.utils import get_modulestore
 from xmodule.x_module import XModuleDescriptor
+from xmodule.modulestore.inheritance import own_metadata
 
 
 class CourseMetadata(object):
@@ -10,7 +11,7 @@ class CourseMetadata(object):
     '''
     # __new_advanced_key__ is used by client not server; so, could argue against it being here
     FILTERED_LIST = XModuleDescriptor.system_metadata_fields + ['start', 'end', 'enrollment_start', 'enrollment_end', 'tabs', 'graceperiod', '__new_advanced_key__']
-    
+
     @classmethod
     def fetch(cls, course_location):
         """
@@ -18,53 +19,60 @@ class CourseMetadata(object):
         """
         if not isinstance(course_location, Location):
             course_location = Location(course_location)
-            
+
         course = {}
-        
+
         descriptor = get_modulestore(course_location).get_item(course_location)
-        
-        for k, v in descriptor.metadata.iteritems():
-            if k not in cls.FILTERED_LIST:
-                course[k] = v
-                
+
+        for field in descriptor.fields + descriptor.lms.fields:
+            if field.name not in cls.FILTERED_LIST:
+                course[field.name] = field.read_from(descriptor)
+
         return course
-        
+
     @classmethod
     def update_from_json(cls, course_location, jsondict):
         """
         Decode the json into CourseMetadata and save any changed attrs to the db.
-        
+
         Ensures none of the fields are in the blacklist.
         """
         descriptor = get_modulestore(course_location).get_item(course_location)
-        
+
         dirty = False
 
         for k, v in jsondict.iteritems():
             # should it be an error if one of the filtered list items is in the payload?
-            if k not in cls.FILTERED_LIST and (k not in descriptor.metadata or descriptor.metadata[k] != v):
+            if k in cls.FILTERED_LIST:
+                continue
+
+            if hasattr(descriptor, k) and getattr(descriptor, k) != v:
+                dirty = True
+                setattr(descriptor, k, v)
+            elif hasattr(descriptor.lms, k) and getattr(descriptor.lms, k) != k:
                 dirty = True
-                descriptor.metadata[k] = v
-            
+                setattr(descriptor.lms, k, v)
+
         if dirty:
-            get_modulestore(course_location).update_metadata(course_location, descriptor.metadata)
-            
+            get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor))
+
         # Could just generate and return a course obj w/o doing any db reads, but I put the reads in as a means to confirm
         # it persisted correctly
         return cls.fetch(course_location)
-    
+
     @classmethod
     def delete_key(cls, course_location, payload):
         '''
         Remove the given metadata key(s) from the course. payload can be a single key or [key..]
         '''
         descriptor = get_modulestore(course_location).get_item(course_location)
-        
+
         for key in payload['deleteKeys']:
-            if key in descriptor.metadata:
-                del descriptor.metadata[key]
-    
-        get_modulestore(course_location).update_metadata(course_location, descriptor.metadata)
-        
+            if hasattr(descriptor, key):
+                delattr(descriptor, key)
+            elif hasattr(descriptor.lms, key):
+                delattr(descriptor.lms, key)
+
+        get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor))
+
         return cls.fetch(course_location)
-        
\ No newline at end of file

common/lib/xmodule/xmodule/course_module.py

@@ -50,7 +50,7 @@ class StringOrDate(Date):
 
         try:
             return time.strftime(self.time_format, value)
-        except ValueError:
+        except (ValueError, TypeError):
             return value
 
 
@@ -449,8 +449,10 @@ class CourseDescriptor(SequenceDescriptor):
         specified. Returns specified list even if is_cohorted and/or auto_cohort are
         false.
         """
-        return self.metadata.get("cohort_config", {}).get(
-            "auto_cohort_groups", [])
+        if self.cohort_config is None:
+            return []
+        else:
+            return self.cohort_config.get("auto_cohort_groups", [])
 
 
     @property

common/lib/xmodule/xmodule/mako_module.py

@@ -45,9 +45,9 @@ class MakoModuleDescriptor(XModuleDescriptor):
     @property
     def editable_metadata_fields(self):
         fields = {}
-        for field, value in own_metadata(self):
-            if field.name in self.system_metadata_fields:
+        for field, value in own_metadata(self).items():
+            if field in self.system_metadata_fields:
                 continue
 
-            fields[field.name] = value
+            fields[field] = value
         return fields

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

@@ -5,9 +5,6 @@ INHERITABLE_METADATA = (
     'graded', 'start', 'due', 'graceperiod', 'showanswer', 'rerandomize',
     # TODO (ichuang): used for Fall 2012 xqa server access
     'xqa_key',
-    # TODO: This is used by the XMLModuleStore to provide for locations for
-    # static files, and will need to be removed when that code is removed
-    'data_dir'
     # How many days early to show a course element to beta testers (float)
     # intended to be set per-course, but can be overridden in for specific
     # elements.  Can be a float.
@@ -33,13 +30,13 @@ def inherit_metadata(descriptor, model_data):
     be inherited
     """
     if not hasattr(descriptor, '_inherited_metadata'):
-        setattr(descriptor, '_inherited_metadata', set())
+        setattr(descriptor, '_inherited_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.add(attr)
+            descriptor._inherited_metadata[attr] = model_data[attr]
             descriptor._model_data[attr] = model_data[attr]
 
 
@@ -52,15 +49,19 @@ def own_metadata(module):
     metadata = {}
     for field in module.fields + module.lms.fields:
         # Only save metadata that wasn't inherited
-        if (field.scope == Scope.settings and
-            field.name not in inherited_metadata and
-            field.name in module._model_data):
+        if field.scope != Scope.settings:
+            continue
 
-            try:
-                metadata[field.name] = field.read_from(module)
-            except KeyError:
-                # Ignore any missing keys in _model_data
-                pass
+        if field.name in inherited_metadata and module._model_data[field.name] == inherited_metadata[field.name]:
+            continue
 
+        if field.name not in module._model_data:
+            continue
+
+        try:
+            metadata[field.name] = module._model_data[field.name]
+        except KeyError:
+            # Ignore any missing keys in _model_data
+            pass
 
     return metadata

common/lib/xmodule/xmodule/modulestore/mongo.py

@@ -22,7 +22,7 @@ from . import ModuleStoreBase, Location
 from .draft import DraftModuleStore
 from .exceptions import (ItemNotFoundError,
                          DuplicateItemError)
-from .inheritance import own_metadata, INHERITABLE_METADATA
+from .inheritance import own_metadata, INHERITABLE_METADATA, inherit_metadata
 
 
 log = logging.getLogger(__name__)
@@ -84,6 +84,18 @@ class MongoKeyValueStore(KeyValueStore):
         else:
             raise InvalidScopeError(key.scope)
 
+    def has(self, key):
+        if key.scope in (Scope.children, Scope.parent):
+            return True
+        elif key.scope == Scope.settings:
+            return key.field_name in self._metadata
+        elif key.scope == Scope.content:
+            if key.field_name == 'data' and not isinstance(self._data, dict):
+                return True
+            else:
+                return key.field_name in self._data
+        else:
+            raise InvalidScopeError(key.scope)
 
 MongoUsage = namedtuple('MongoUsage', 'id, def_id')
 
@@ -146,7 +158,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 module = class_(self, location, model_data)
                 if self.metadata_inheritance_tree is not None:
                     metadata_to_inherit = self.metadata_inheritance_tree.get('parent_metadata', {}).get(location.url(), {})
-                    module.inherit_metadata(metadata_to_inherit)
+                    inherit_metadata(module, metadata_to_inherit)
                 return module
             except:
                 log.debug("Failed to load descriptor", exc_info=True)

common/lib/xmodule/xmodule/modulestore/xml_exporter.py

 import logging
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.inheritance import own_metadata
 from fs.osfs import OSFS
 from json import dumps
 
@@ -31,14 +32,12 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d
     # export the grading policy
     policies_dir = export_fs.makeopendir('policies')
     course_run_policy_dir = policies_dir.makeopendir(course.location.name)
-    if 'grading_policy' in course.definition['data']:
-        with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy:
-            grading_policy.write(dumps(course.grading_policy))
+    with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy:
+        grading_policy.write(dumps(course.grading_policy))
 
     # export all of the course metadata in policy.json
     with course_run_policy_dir.open('policy.json', 'w') as course_policy:
-        policy = {}
-        policy = {'course/' + course.location.name: course.metadata}
+        policy = {'course/' + course.location.name: own_metadata(course)}
         course_policy.write(dumps(policy))
 
 

common/lib/xmodule/xmodule/tests/test_capa_module.py

@@ -98,6 +98,8 @@ class CapaFactory(object):
         if correct:
             # TODO: probably better to actually set the internal state properly, but...
             module.get_score = lambda: {'score': 1, 'total': 1}
+        else:
+            module.get_score = lambda: {'score': 0, 'total': 1}
 
         return module
 

lms/djangoapps/courseware/model_data.py

@@ -336,6 +336,25 @@ class LmsKeyValueStore(KeyValueStore):
         else:
             field_object.delete()
 
+    def has(self, key):
+        if key.field_name in self._descriptor_model_data:
+            return key.field_name in self._descriptor_model_data
+
+        if key.scope == Scope.parent:
+            return True
+
+        if key.scope not in self._allowed_scopes:
+            raise InvalidScopeError(key.scope)
+
+        field_object = self._model_data_cache.find(key)
+        if field_object is None:
+            return False
+
+        if key.scope == Scope.student_state:
+            return key.field_name in json.loads(field_object.state)
+        else:
+            return True
+
 
 LmsUsage = namedtuple('LmsUsage', 'id, def_id')
 

local-requirements.txt

@@ -6,4 +6,4 @@
 # XBlock:
 # Might change frequently, so put it in local-requirements.txt,
 # but conceptually is an external package, so it is in a separate repo.
--e git+ssh://git@github.com/MITx/xmodule-debugger@8f82a3b7fc#egg=XBlock
+-e git+ssh://git@github.com/MITx/xmodule-debugger@e3c4bc#egg=XBlock