Merge branch 'master' of github.com:MITx/mitx into fix/cdodge/serializing-ints-as-empty-strings

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -5,7 +5,7 @@ from django.test.utils import override_settings
 from django.conf import settings
 from django.core.urlresolvers import reverse
 from path import path
-from tempfile import mkdtemp
+from tempdir import mkdtemp_clean
 import json
 from fs.osfs import OSFS
 import copy
@@ -194,7 +194,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         import_from_xml(ms, 'common/test/data/', ['full'])
         location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012')
 
-        root_dir = path(mkdtemp())
+        root_dir = path(mkdtemp_clean())
 
         print 'Exporting to tempdir = {0}'.format(root_dir)
 
@@ -264,6 +264,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.assertContains(resp, '/c4x/edX/full/asset/handouts_schematic_tutorial.pdf')
 
 
+
 class ContentStoreTest(ModuleStoreTestCase):
     """
     Tests for the CMS ContentStore application.
@@ -433,6 +434,52 @@ class ContentStoreTest(ModuleStoreTestCase):
         # make sure we found the item (e.g. it didn't error while loading)
         self.assertFalse(asserted)   
 
+    def test_metadata_inheritance(self):
+        import_from_xml(modulestore(), 'common/test/data/', ['full'])
+
+        ms = modulestore('direct')
+        course = ms.get_item(Location(['i4x', 'edX', 'full', 'course', '6.002_Spring_2012', None]))
+
+        verticals = ms.get_items(['i4x', 'edX', 'full', 'vertical', None, None])
+
+        # 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.assertGreater(len(verticals), 0)
+
+        new_component_location = Location('i4x', 'edX', 'full', 'html', 'new_component')
+        source_template_location = Location('i4x', 'edx', 'templates', 'html', 'Empty')
+        
+        # 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()])
+
+        # 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(course.metadata['graceperiod'], new_module.metadata['graceperiod'])
+
+        #
+        # 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)
+
+        # 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'])
+
+
 class TemplateTestCase(ModuleStoreTestCase):
 
     def test_template_cleanup(self):        

cms/djangoapps/contentstore/tests/tests.py

@@ -4,7 +4,6 @@ from django.test.client import Client
 from django.conf import settings
 from django.core.urlresolvers import reverse
 from path import path
-from tempfile import mkdtemp
 import json
 from fs.osfs import OSFS
 import copy

cms/envs/common.py

@@ -20,7 +20,6 @@ Longer TODO:
 """
 
 import sys
-import tempfile
 import os.path
 import os
 import lms.envs.common
@@ -59,7 +58,8 @@ sys.path.append(COMMON_ROOT / 'lib')
 
 ############################# WEB CONFIGURATION #############################
 # This is where we stick our compiled template files.
-MAKO_MODULE_DIR = tempfile.mkdtemp('mako')
+from tempdir import mkdtemp_clean
+MAKO_MODULE_DIR = mkdtemp_clean('mako')
 MAKO_TEMPLATES = {}
 MAKO_TEMPLATES['main'] = [
     PROJECT_ROOT / 'templates',

common/djangoapps/mitxmako/makoloader.py

@@ -9,6 +9,7 @@ from django.template.loaders.app_directories import Loader as AppDirectoriesLoad
 from mitxmako.template import Template
 import mitxmako.middleware
 
+import tempdir
 
 log = logging.getLogger(__name__)
 
@@ -30,7 +31,7 @@ class MakoLoader(object):
 
         if module_directory is None:
             log.warning("For more caching of mako templates, set the MAKO_MODULE_DIR in settings!")
-            module_directory = tempfile.mkdtemp()
+            module_directory = tempdir.mkdtemp_clean()
 
         self.module_directory = module_directory
 

common/djangoapps/mitxmako/middleware.py

@@ -13,7 +13,7 @@
 #   limitations under the License.
 
 from mako.lookup import TemplateLookup
-import tempfile
+import tempdir
 from django.template import RequestContext
 from django.conf import settings
 
@@ -29,7 +29,7 @@ class MakoMiddleware(object):
         module_directory = getattr(settings, 'MAKO_MODULE_DIR', None)
 
         if module_directory is None:
-            module_directory = tempfile.mkdtemp()
+            module_directory = tempdir.mkdtemp_clean()
 
         for location in template_locations:
             lookup[location] = TemplateLookup(directories=template_locations[location],

common/djangoapps/student/management/commands/tests/test_pearson.py

@@ -7,6 +7,7 @@ import logging
 import os
 from tempfile import mkdtemp
 import cStringIO
+import shutil
 import sys
 
 from django.test import TestCase
@@ -143,23 +144,18 @@ class PearsonTestCase(TestCase):
     '''
     Base class for tests running Pearson-related commands
     '''
-    import_dir = mkdtemp(prefix="import")
-    export_dir = mkdtemp(prefix="export")
 
     def assertErrorContains(self, error_message, expected):
         self.assertTrue(error_message.find(expected) >= 0, 'error message "{}" did not contain "{}"'.format(error_message, expected))
 
-    def tearDown(self):
-        def delete_temp_dir(dirname):
-            if os.path.exists(dirname):
-                for filename in os.listdir(dirname):
-                    os.remove(os.path.join(dirname, filename))
-                os.rmdir(dirname)
-
-        # clean up after any test data was dumped to temp directory
-        delete_temp_dir(self.import_dir)
-        delete_temp_dir(self.export_dir)
+    def setUp(self):
+        self.import_dir = mkdtemp(prefix="import")
+        self.addCleanup(shutil.rmtree, self.import_dir)
+        self.export_dir = mkdtemp(prefix="export")
+        self.addCleanup(shutil.rmtree, self.export_dir)
 
+    def tearDown(self):
+        pass
         # and clean up the database:
 #        TestCenterUser.objects.all().delete()
 #        TestCenterRegistration.objects.all().delete()

common/lib/tempdir.py

+"""Make temporary directories nicely."""
+
+import atexit
+import os.path
+import shutil
+import tempfile
+
+def mkdtemp_clean(suffix="", prefix="tmp", dir=None):
+    """Just like mkdtemp, but the directory will be deleted when the process ends."""
+    the_dir = tempfile.mkdtemp(suffix=suffix, prefix=prefix, dir=dir)
+    atexit.register(cleanup_tempdir, the_dir)
+    return the_dir
+
+def cleanup_tempdir(the_dir):
+    """Called on process exit to remove a temp directory."""
+    if os.path.exists(the_dir):
+        shutil.rmtree(the_dir)

common/lib/xmodule/xmodule/mako_module.py

@@ -44,5 +44,6 @@ class MakoModuleDescriptor(XModuleDescriptor):
     # cdodge: encapsulate a means to expose "editable" metadata fields (i.e. not internal system metadata)
     @property
     def editable_metadata_fields(self):
-        subset = [name for name in self.metadata.keys() if name not in self.system_metadata_fields]
+        subset = [name for name in self.metadata.keys() if name not in self.system_metadata_fields and 
+            name not in self._inherited_metadata]
         return subset

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

 import pymongo
 import sys
 import logging
+import copy
 
 from bson.son import SON
 from fs.osfs import OSFS
 from itertools import repeat
 from path import path
+from datetime import datetime, timedelta
 
 from importlib import import_module
 from xmodule.errortracker import null_error_tracker, exc_info_to_str
@@ -27,9 +29,11 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     """
     A system that has a cache of module json that it will use to load modules
     from, with a backup of calling to the underlying modulestore for more data
+    TODO (cdodge) when the 'split module store' work has been completed we can remove all
+    references to metadata_inheritance_tree
     """
     def __init__(self, modulestore, module_data, default_class, resources_fs,
-                 error_tracker, render_template):
+                 error_tracker, render_template, metadata_inheritance_tree = None):
         """
         modulestore: the module store that can be used to retrieve additional modules
 
@@ -54,6 +58,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         # cdodge: other Systems have a course_id attribute defined. To keep things consistent, let's
         # define an attribute here as well, even though it's None
         self.course_id = None
+        self.metadata_inheritance_tree = metadata_inheritance_tree
 
     def load_item(self, location):
         location = Location(location)
@@ -61,11 +66,13 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         if json_data is None:
             return self.modulestore.get_item(location)
         else:
-            # TODO (vshnayder): metadata inheritance is somewhat broken because mongo, doesn't
-            # always load an entire course.  We're punting on this until after launch, and then
-            # will build a proper course policy framework.
+            # load the module and apply the inherited metadata
             try:
-                return XModuleDescriptor.load_from_json(json_data, self, self.default_class)
+                module = XModuleDescriptor.load_from_json(json_data, self, self.default_class)
+                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)
+                return module
             except:
                 return ErrorDescriptor.from_json(
                     json_data,
@@ -142,6 +149,82 @@ class MongoModuleStore(ModuleStoreBase):
         self.fs_root = path(fs_root)
         self.error_tracker = error_tracker
         self.render_template = render_template
+        self.metadata_inheritance_cache = {}
+
+    def get_metadata_inheritance_tree(self, location):
+        '''
+        TODO (cdodge) This method can be deleted when the 'split module store' work has been completed
+        '''
+        
+        # get all collections in the course, this query should not return any leaf nodes
+        query = { '_id.org' : location.org,
+                '_id.course' : location.course,
+                '_id.revision' : None,
+                'definition.children':{'$ne': []}
+                }
+        # we just want the Location, children, and metadata
+        record_filter = {'_id':1,'definition.children':1,'metadata':1}
+
+        # call out to the DB
+        resultset = self.collection.find(query, record_filter)
+
+        results_by_url = {}
+        root = None
+
+        # now go through the results and order them by the location url
+        for result in resultset:
+            location = Location(result['_id'])
+            results_by_url[location.url()] = result
+            if location.category == 'course':
+                root = location.url()
+
+        # now traverse the tree and compute down the inherited metadata
+        metadata_to_inherit = {}
+        def _compute_inherited_metadata(url):
+            my_metadata = results_by_url[url]['metadata']
+            for key in my_metadata.keys():
+                if key not in XModuleDescriptor.inheritable_metadata:
+                    del my_metadata[key]
+            results_by_url[url]['metadata'] = my_metadata
+
+            # go through all the children and recurse, but only if we have
+            # in the result set. Remember results will not contain leaf nodes
+            for child in results_by_url[url].get('definition',{}).get('children',[]):
+                if child in results_by_url:
+                    new_child_metadata = copy.deepcopy(my_metadata)
+                    new_child_metadata.update(results_by_url[child]['metadata'])
+                    results_by_url[child]['metadata'] = new_child_metadata
+                    metadata_to_inherit[child] = new_child_metadata
+                    _compute_inherited_metadata(child)
+                else:
+                    # this is likely a leaf node, so let's record what metadata we need to inherit
+                    metadata_to_inherit[child] = my_metadata
+        
+        if root is not None:
+            _compute_inherited_metadata(root)
+
+        cache = {'parent_metadata': metadata_to_inherit, 
+            'timestamp' : datetime.now()}
+
+        return cache
+
+    def get_cached_metadata_inheritance_tree(self, location, max_age_allowed):
+        '''
+        TODO (cdodge) This method can be deleted when the 'split module store' work has been completed
+        '''
+        cache_name = '{0}/{1}'.format(location.org, location.course)
+        cache = self.metadata_inheritance_cache.get(cache_name,{'parent_metadata': {}, 
+            'timestamp': datetime.now() - timedelta(hours=1)})
+        age = (datetime.now() - cache['timestamp'])
+
+        if age.seconds >= max_age_allowed:
+            logging.debug('loading entire inheritance tree for {0}'.format(cache_name))
+            cache = self.get_metadata_inheritance_tree(location)
+            self.metadata_inheritance_cache[cache_name] = cache
+
+        return cache
+
+
 
     def _clean_item_data(self, item):
         """
@@ -196,6 +279,8 @@ class MongoModuleStore(ModuleStoreBase):
 
         resource_fs = OSFS(root)
 
+        # TODO (cdodge): When the 'split module store' work has been completed, we should remove
+        # the 'metadata_inheritance_tree' parameter
         system = CachingDescriptorSystem(
             self,
             data_cache,
@@ -203,6 +288,7 @@ class MongoModuleStore(ModuleStoreBase):
             resource_fs,
             self.error_tracker,
             self.render_template,
+            metadata_inheritance_tree = self.get_cached_metadata_inheritance_tree(Location(item['location']), 60)
         )
         return system.load_item(item['location'])
 
@@ -261,11 +347,11 @@ class MongoModuleStore(ModuleStoreBase):
             descendents of the queried modules for more efficient results later
             in the request. The depth is counted in the number of
             calls to get_children() to cache. None indicates to cache all descendents.
-
         """
         location = Location.ensure_fully_specified(location)
         item = self._find_one(location)
-        return self._load_items([item], depth)[0]
+        module = self._load_items([item], depth)[0]
+        return module
 
     def get_instance(self, course_id, location, depth=0):
         """
@@ -285,7 +371,8 @@ class MongoModuleStore(ModuleStoreBase):
             sort=[('revision', pymongo.ASCENDING)],
         )
 
-        return self._load_items(list(items), depth)
+        modules = self._load_items(list(items), depth)
+        return modules
 
     def clone_item(self, source, location):
         """
@@ -313,7 +400,7 @@ class MongoModuleStore(ModuleStoreBase):
             raise DuplicateItemError(location)
 
 
-    def get_course_for_item(self, location):
+    def get_course_for_item(self, location, depth=0):
         '''
         VS[compat]
         cdodge: for a given Xmodule, return the course that it belongs to
@@ -327,7 +414,7 @@ class MongoModuleStore(ModuleStoreBase):
         # know the 'name' parameter in this context, so we have
         # to assume there's only one item in this query even though we are not specifying a name
         course_search_location = ['i4x', location.org, location.course, 'course', None]
-        courses = self.get_items(course_search_location)
+        courses = self.get_items(course_search_location, depth=depth)
 
         # make sure we found exactly one match on this above course search
         found_cnt = len(courses)

common/lib/xmodule/xmodule/open_ended_grading_classes/open_ended_module.py

@@ -108,7 +108,7 @@ class OpenEndedModule(openendedchild.OpenEndedChild):
         self.answer = find_with_default(oeparam, 'answer_display', 'No answer given.')
 
         parsed_grader_payload.update({
-            'location': system.location.url(),
+            'location': self.location_string,
             'course_id': system.course_id,
             'prompt': prompt_string,
             'rubric': rubric_string,
@@ -138,7 +138,7 @@ class OpenEndedModule(openendedchild.OpenEndedChild):
         """
 
         event_info = dict()
-        event_info['problem_id'] = system.location.url()
+        event_info['problem_id'] = self.location_string
         event_info['student_id'] = system.anonymous_student_id
         event_info['survey_responses'] = get
 

common/lib/xmodule/xmodule/open_ended_grading_classes/openendedchild.py

@@ -108,6 +108,12 @@ class OpenEndedChild(object):
         self.peer_gs = PeerGradingService(system.open_ended_grading_interface, system)
 
         self.system = system
+        self.location_string = location
+        try:
+            self.location_string = self.location_string.url()
+        except:
+            pass
+
         self.setup_response(system, location, definition, descriptor)
 
     def setup_response(self, system, location, definition, descriptor):
@@ -418,7 +424,8 @@ class OpenEndedChild(object):
         return success, string
 
     def check_if_student_can_submit(self):
-        location = self.system.location.url()
+        location = self.location_string
+
         student_id = self.system.anonymous_student_id
         success = False
         allowed_to_submit = True

common/lib/xmodule/xmodule/tests/test_export.py

@@ -4,7 +4,7 @@ from fs.osfs import OSFS
 from nose.tools import assert_equals, assert_true
 from path import path
 from tempfile import mkdtemp
-from shutil import copytree
+import shutil
 
 from xmodule.modulestore.xml import XMLModuleStore
 
@@ -46,11 +46,11 @@ class RoundTripTestCase(unittest.TestCase):
         Thus we make sure that export and import work properly.
     '''
     def check_export_roundtrip(self, data_dir, course_dir):
-        root_dir = path(mkdtemp())
+        root_dir = path(self.temp_dir)
         print "Copying test course to temp dir {0}".format(root_dir)
 
         data_dir = path(data_dir)
-        copytree(data_dir / course_dir, root_dir / course_dir)
+        shutil.copytree(data_dir / course_dir, root_dir / course_dir)
 
         print "Starting import"
         initial_import = XMLModuleStore(root_dir, course_dirs=[course_dir])
@@ -108,6 +108,8 @@ class RoundTripTestCase(unittest.TestCase):
 
     def setUp(self):
         self.maxDiff = None
+        self.temp_dir = mkdtemp()
+        self.addCleanup(shutil.rmtree, self.temp_dir)
 
     def test_toy_roundtrip(self):
         self.check_export_roundtrip(DATA_DIR, "toy")

common/lib/xmodule/xmodule/tests/test_self_assessment.py

@@ -67,10 +67,18 @@ class SelfAssessmentTest(unittest.TestCase):
         def get_fake_item(name):
             return responses[name]
 
+        def get_data_for_location(self,location,student):
+            return {
+            'count_graded' : 0,
+            'count_required' : 0,
+            'student_sub_count': 0,
+            }
+
         mock_query_dict = MagicMock()
         mock_query_dict.__getitem__.side_effect = get_fake_item
         mock_query_dict.getlist = get_fake_item
 
+        self.module.peer_gs.get_data_for_location = get_data_for_location
 
         self.assertEqual(self.module.get_score()['score'], 0)
 

common/lib/xmodule/xmodule/x_module.py

@@ -411,7 +411,6 @@ class ResourceTemplates(object):
 
         return templates
 
-
 class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates):
     """
     An XModuleDescriptor is a specification for an element of a course. This
@@ -585,11 +584,11 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates):
     def inherit_metadata(self, metadata):
         """
         Updates this module with metadata inherited from a containing module.
-        Only metadata specified in self.inheritable_metadata will
+        Only metadata specified in inheritable_metadata will
         be inherited
         """
         # Set all inheritable metadata from kwargs that are
-        # in self.inheritable_metadata and aren't already set in metadata
+        # in inheritable_metadata and aren't already set in metadata
         for attr in self.inheritable_metadata:
             if attr not in self.metadata and attr in metadata:
                 self._inherited_metadata.add(attr)

lms/envs/common.py

@@ -20,7 +20,6 @@ Longer TODO:
 """
 import sys
 import os
-import tempfile
 from xmodule.static_content import write_module_styles, write_module_js
 
 from path import path
@@ -133,7 +132,8 @@ OPENID_PROVIDER_TRUSTED_ROOTS = ['cs50.net', '*.cs50.net']
 ################################## MITXWEB #####################################
 # This is where we stick our compiled template files. Most of the app uses Mako
 # templates
-MAKO_MODULE_DIR = tempfile.mkdtemp('mako')
+from tempdir import mkdtemp_clean
+MAKO_MODULE_DIR = mkdtemp_clean('mako')
 MAKO_TEMPLATES = {}
 MAKO_TEMPLATES['main'] = [PROJECT_ROOT / 'templates',
                           COMMON_ROOT / 'templates',