Merge pull request #173 from MITx/cpennington/pull-155-cleanup

This cleans up remaining issues w/ https://github.com/MITx/mitx/pull/155

cms/djangoapps/contentstore/views.py

@@ -59,16 +59,3 @@ def save_item(request):
         export_to_github(course, "CMS Edit")
 
     return HttpResponse(json.dumps({}))
-
-
-def temp_force_export(request):
-    org = 'mit.edu'
-    course = '6002xs12'
-    name = '6.002_Spring_2012'
-    course = modulestore().get_item(['i4x', org, course, 'course', name])
-    fs = OSFS('../data-export-test')
-    xml = course.export_to_xml(fs)
-    with fs.open('course.xml', 'w') as course_xml:
-        course_xml.write(xml)
-
-    return HttpResponse('Done')

cms/envs/dev.py

@@ -16,8 +16,8 @@ MODULESTORE = {
         'OPTIONS': {
             'default_class': 'xmodule.raw_module.RawDescriptor',
             'host': 'localhost',
-            'db': 'mongo_base',
-            'collection': 'key_store',
+            'db': 'xmodule',
+            'collection': 'modulestore',
         }
     }
 }

cms/urls.py

@@ -10,7 +10,6 @@ urlpatterns = ('',
     url(r'^edit_item$', 'contentstore.views.edit_item', name='edit_item'),
     url(r'^save_item$', 'contentstore.views.save_item', name='save_item'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)$', 'contentstore.views.course_index', name='course_index'),
-    url(r'^temp_force_export$', 'contentstore.views.temp_force_export'),
     url(r'^github_service_hook$', 'github_sync.views.github_post_receive'),
 )
 

common/lib/capa/capa/capa_problem.py

@@ -74,11 +74,11 @@ class LoncapaProblem(object):
 
         Arguments:
 
-         - problem_text : xml defining the problem
-         - id           : string used as the identifier for this problem; often a filename (no spaces)
-         - state        : student state (represented as a dict)
-         - seed         : random number generator seed (int)
-         - system       : I4xSystem instance which provides OS, rendering, and user context 
+         - problem_text (string): xml defining the problem
+         - id           (string): identifier for this problem; often a filename (no spaces)
+         - state        (dict): student state
+         - seed         (int): random number generator seed (int)
+         - system       (I4xSystme): I4xSystem instance which provides OS, rendering, and user context 
 
         '''
 
@@ -126,7 +126,7 @@ class LoncapaProblem(object):
         self.done = False
 
     def __unicode__(self):
-        return u"LoncapaProblem ({0})".format(self.problem_text)
+        return u"LoncapaProblem ({0})".format(self.problem_id)
 
     def get_state(self):
         ''' Stored per-user session data neeeded to:
@@ -265,7 +265,7 @@ class LoncapaProblem(object):
                 parent = inc.getparent()			# insert  new XML into tree in place of inlcude
                 parent.insert(parent.index(inc),incxml)
                 parent.remove(inc)
-                log.debug('Included %s into %s' % (file, self.id))
+                log.debug('Included %s into %s' % (file, self.problem_id))
 
     def _extract_context(self, tree, seed=struct.unpack('i', os.urandom(4))[0]):  # private
         '''

common/lib/xmodule/setup.py

@@ -18,21 +18,21 @@ setup(
     entry_points={
         'xmodule.v1': [
             "abtest = xmodule.abtest_module:ABTestDescriptor",
-            "book = xmodule.translation_module:TranslateCustomTagDescriptor",
+            "book = xmodule.backcompat_module:TranslateCustomTagDescriptor",
             "chapter = xmodule.seq_module:SequenceDescriptor",
             "course = xmodule.course_module:CourseDescriptor",
             "customtag = xmodule.template_module:CustomTagDescriptor",
-            "discuss = xmodule.translation_module:TranslateCustomTagDescriptor",
+            "discuss = xmodule.backcompat_module:TranslateCustomTagDescriptor",
             "html = xmodule.html_module:HtmlDescriptor",
-            "image = xmodule.translation_module:TranslateCustomTagDescriptor",
+            "image = xmodule.backcompat_module:TranslateCustomTagDescriptor",
             "problem = xmodule.capa_module:CapaDescriptor",
             "problemset = xmodule.vertical_module:VerticalDescriptor",
-            "section = xmodule.translation_module:SemanticSectionDescriptor",
+            "section = xmodule.backcompat_module:SemanticSectionDescriptor",
             "sequential = xmodule.seq_module:SequenceDescriptor",
-            "slides = xmodule.translation_module:TranslateCustomTagDescriptor",
+            "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor",
             "vertical = xmodule.vertical_module:VerticalDescriptor",
             "video = xmodule.video_module:VideoDescriptor",
-            "videodev = xmodule.translation_module:TranslateCustomTagDescriptor",
+            "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor",
             "videosequence = xmodule.seq_module:SequenceDescriptor",
         ]
     }

common/lib/xmodule/xmodule/abtest_module.py

@@ -29,13 +29,6 @@ def group_from_value(groups, v):
 class ABTestModule(XModule):
     """
     Implements an A/B test with an aribtrary number of competing groups
-
-    Format:
-    <abtest>
-        <group name="a" portion=".1"><contenta/></group>
-        <group name="b" portion=".2"><contentb/></group>
-        <default><contentdefault/></default>
-    </abtest>
     """
 
     def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs):
@@ -60,15 +53,53 @@ class ABTestModule(XModule):
                 in self.definition['data']['group_content'][self.group]]
 
 
+# TODO (cpennington): Use Groups should be a first class object, rather than being
+# managed by ABTests
 class ABTestDescriptor(RawDescriptor, XmlDescriptor):
     module_class = ABTestModule
 
     def __init__(self, system, definition=None, **kwargs):
+        """
+        definition is a dictionary with the following layout:
+            {'data': {
+                'experiment': 'the name of the experiment',
+                'group_portions': {
+                    'group_a': 0.1,
+                    'group_b': 0.2
+                },
+                'group_contents': {
+                    'group_a': [
+                        'url://for/content/module/1',
+                        'url://for/content/module/2',
+                    ],
+                    'group_b': [
+                        'url://for/content/module/3',
+                    ],
+                    DEFAULT: [
+                        'url://for/default/content/1'
+                    ]
+                }
+            },
+            'children': [
+                'url://for/content/module/1',
+                'url://for/content/module/2',
+                'url://for/content/module/3',
+                'url://for/default/content/1',
+            ]}
+        """
         kwargs['shared_state_key'] = definition['data']['experiment']
         RawDescriptor.__init__(self, system, definition, **kwargs)
 
     @classmethod
     def definition_from_xml(cls, xml_object, system):
+        """
+        XML Format:
+        <abtest experiment="experiment_name">
+            <group name="a" portion=".1"><contenta/></group>
+            <group name="b" portion=".2"><contentb/></group>
+            <default><contentdefault/></default>
+        </abtest>
+        """
         experiment = xml_object.get('experiment')
 
         if experiment is None:

common/lib/xmodule/xmodule/capa_module.py

@@ -65,10 +65,8 @@ class ComplexEncoder(json.JSONEncoder):
 
 
 class CapaModule(XModule):
-    ''' Interface between capa_problem and x_module. Originally a hack
-    meant to be refactored out, but it seems to be serving a useful
-    prupose now. We can e.g .destroy and create the capa_problem on a
-    reset.
+    '''
+    An XModule implementing LonCapa format problems, implemented by way of capa.capa_problem.LoncapaProblem
     '''
     icon_class = 'problem'
 
@@ -80,11 +78,6 @@ class CapaModule(XModule):
 
         dom2 = etree.fromstring(definition['data'])
 
-        self.explanation = "problems/" + only_one(dom2.xpath('/problem/@explain'),
-                                                  default="closed")
-        # TODO: Should be converted to: self.explanation=only_one(dom2.xpath('/problem/@explain'), default="closed")
-        self.explain_available = only_one(dom2.xpath('/problem/@explain_available'))
-
         display_due_date_string = self.metadata.get('due', None)
         if display_due_date_string is not None:
             self.display_due_date = dateutil.parser.parse(display_due_date_string)
@@ -246,16 +239,6 @@ class CapaModule(XModule):
         if self.max_attempts is None and self.rerandomize != "always":
             save_button = False
 
-        # Check if explanation is available, and if so, give a link
-        explain = ""
-        if self.lcp.done and self.explain_available == 'attempted':
-            explain = self.explanation
-        if self.closed() and self.explain_available == 'closed':
-            explain = self.explanation
-
-        if len(explain) == 0:
-            explain = False
-
         context = {'problem': content,
                    'id': self.id,
                    'check_button': check_button,
@@ -265,7 +248,6 @@ class CapaModule(XModule):
                    'ajax_url': self.system.ajax_url,
                    'attempts_used': self.attempts,
                    'attempts_allowed': self.max_attempts,
-                   'explain': explain,
                    'progress': self.get_progress(),
                    }
 

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -19,9 +19,13 @@ URL_RE = re.compile("""
     (/(?P<revision>[^/]+))?
     """, re.VERBOSE)
 
+# TODO (cpennington): We should decide whether we want to expand the
+# list of valid characters in a location
 INVALID_CHARS = re.compile(r"[^\w.-]")
 
 _LocationBase = namedtuple('LocationBase', 'tag org course category name revision')
+
+
 class Location(_LocationBase):
     '''
     Encodes a location.

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -82,9 +82,9 @@ class XMLModuleStore(ModuleStore):
                 course = course_dir
 
             class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
-                def __init__(self, modulestore):
+                def __init__(self, xmlstore):
                     """
-                    modulestore: the XMLModuleStore to store the loaded modules in
+                    xmlstore: the XMLModuleStore to store the loaded modules in
                     """
                     self.unnamed_modules = 0
                     self.used_slugs = set()
@@ -110,18 +110,18 @@ class XMLModuleStore(ModuleStore):
                             # log.debug('-> slug=%s' % slug)
                             xml_data.set('slug', slug)
 
-                        module = XModuleDescriptor.load_from_xml(etree.tostring(xml_data), self, org, course, modulestore.default_class)
+                        module = XModuleDescriptor.load_from_xml(etree.tostring(xml_data), self, org, course, xmlstore.default_class)
                         log.debug('==> importing module location %s' % repr(module.location))
-                        modulestore.modules[module.location] = module
+                        xmlstore.modules[module.location] = module
 
-                        if modulestore.eager:
+                        if xmlstore.eager:
                             module.get_children()
                         return module
 
                     system_kwargs = dict(
                         render_template=lambda: '',
-                        load_item=modulestore.get_item,
-                        resources_fs=OSFS(modulestore.data_dir / course_dir),
+                        load_item=xmlstore.get_item,
+                        resources_fs=OSFS(xmlstore.data_dir / course_dir),
                         process_xml=process_xml
                     )
                     MakoDescriptorSystem.__init__(self, **system_kwargs)

common/lib/xmodule/xmodule/x_module.py

@@ -96,7 +96,9 @@ class XModule(object):
         kwargs: Optional arguments. Subclasses should always accept kwargs and pass them
             to the parent class constructor.
             Current known uses of kwargs:
-                metadata: A dictionary containing data that specifies information that is particular
+                metadata: SCAFFOLDING - This dictionary will be split into several different types of metadata
+                    in the future (course policy, modification history, etc).
+                    A dictionary containing data that specifies information that is particular
                     to a problem in the context of a course
         '''
         self.system = system

common/lib/xmodule/xmodule/xml_module.py

@@ -3,9 +3,14 @@ from xmodule.x_module import XModuleDescriptor
 from lxml import etree
 import copy
 import logging
+from collections import namedtuple
 
 log = logging.getLogger(__name__)
 
+
+# TODO (cpennington): This was implemented in an attempt to improve performance,
+# but the actual improvement wasn't measured (and it was implemented late at night).
+# We should check if it hurts, and whether there's a better way of doing lazy loading
 class LazyLoadingDict(MutableMapping):
     """
     A dictionary object that lazily loads it's contents from a provided
@@ -58,6 +63,18 @@ class LazyLoadingDict(MutableMapping):
         self._loaded = True
 
 
+_AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata')
+
+
+class AttrMap(_AttrMapBase):
+    """
+    A class that specifies a metadata_key, a function to transform an xml attribute to be placed in that key,
+    and to transform that key value
+    """
+    def __new__(_cls, metadata_key, to_metadata=lambda x: x, from_metadata=lambda x: x):
+        return _AttrMapBase.__new__(_cls, metadata_key, to_metadata, from_metadata)
+
+
 class XmlDescriptor(XModuleDescriptor):
     """
     Mixin class for standardized parsing of from xml
@@ -71,6 +88,13 @@ class XmlDescriptor(XModuleDescriptor):
     metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize',
         'due', 'graded', 'name', 'slug')
 
+    # A dictionary mapping xml attribute names to functions of the value
+    # that return the metadata key and value
+    xml_attribute_map = {
+        'graded': AttrMap('graded', lambda val: val == 'true', lambda val: str(val).lower()),
+        'name': AttrMap('display_name'),
+    }
+
     @classmethod
     def definition_from_xml(cls, xml_object, system):
         """
@@ -116,16 +140,11 @@ class XmlDescriptor(XModuleDescriptor):
 
         def metadata_loader():
             metadata = {}
-            for attr in ('format', 'graceperiod', 'showanswer', 'rerandomize', 'due'):
-                from_xml = xml_object.get(attr)
-                if from_xml is not None:
-                    metadata[attr] = from_xml
-
-            if xml_object.get('graded') is not None:
-                metadata['graded'] = xml_object.get('graded') == 'true'
-
-            if xml_object.get('name') is not None:
-                metadata['display_name'] = xml_object.get('name')
+            for attr in cls.metadata_attributes:
+                val = xml_object.get(attr)
+                if val is not None:
+                    attr_map = cls.xml_attribute_map.get(attr, AttrMap(attr))
+                    metadata[attr_map.metadata_key] = attr_map.to_metadata(val)
 
             return metadata
 
@@ -171,6 +190,8 @@ class XmlDescriptor(XModuleDescriptor):
 
         The returned XML should be able to be parsed back into an identical XModuleDescriptor
         using the from_xml method with the same system, org, and course
+
+        resource_fs is a pyfilesystem office (from the fs package)
         """
         xml_object = self.definition_to_xml(resource_fs)
         self.__class__.clean_metadata_from_xml(xml_object)
@@ -191,15 +212,15 @@ class XmlDescriptor(XModuleDescriptor):
         xml_object.set('slug', self.name)
         xml_object.tag = self.category
 
-        for attr in ('format', 'graceperiod', 'showanswer', 'rerandomize', 'due'):
-            if attr in self.metadata and attr not in self._inherited_metadata:
-                xml_object.set(attr, self.metadata[attr])
+        for attr in self.metadata_attributes:
+            attr_map = self.xml_attribute_map.get(attr, AttrMap(attr))
+            metadata_key = attr_map.metadata_key
 
-        if 'graded' in self.metadata and 'graded' not in self._inherited_metadata:
-            xml_object.set('graded', str(self.metadata['graded']).lower())
+            if metadata_key not in self.metadata or metadata_key in self._inherited_metadata:
+                continue
 
-        if 'display_name' in self.metadata:
-            xml_object.set('name', self.metadata['display_name'])
+            val = attr_map.from_metadata(self.metadata[metadata_key])
+            xml_object.set(attr, val)
 
         return etree.tostring(xml_object, pretty_print=True)
 

lms/djangoapps/courseware/models.py

@@ -72,6 +72,10 @@ class StudentModuleCache(object):
         '''
         Find any StudentModule objects that are needed by any child modules of the
         supplied descriptor. Avoids making multiple queries to the database
+
+        descriptor: An XModuleDescriptor
+        depth is the number of levels of descendent modules to load StudentModules for, in addition to
+            the supplied descriptor. If depth is None, load all descendent StudentModules
         '''
         if user.is_authenticated():
             module_ids = self._get_module_state_keys(descriptor, depth)
@@ -92,7 +96,11 @@ class StudentModuleCache(object):
     def _get_module_state_keys(self, descriptor, depth):
         '''
         Get a list of the state_keys needed for StudentModules
-        required for this chunk of module xml
+        required for this module descriptor
+
+        descriptor: An XModuleDescriptor
+        depth is the number of levels of descendent modules to load StudentModules for, in addition to
+            the supplied descriptor. If depth is None, load all descendent StudentModules
         '''
         keys = [descriptor.location.url()]
 

lms/djangoapps/courseware/module_render.py

@@ -41,7 +41,7 @@ class I4xSystem(object):
                           module instance object.
         render_template - a function that takes (template_file, context), and returns
                           rendered html.
-        user - The user to base the seed off of for this request
+        user - The user to base the random number generator seed off of for this request
         filestore - A filestore ojbect.  Defaults to an instance of OSFS based at
                     settings.DATA_DIR.
         replace_urls - TEMPORARY - A function like static_replace.replace_urls
@@ -176,20 +176,20 @@ def get_section(course_module, chapter, section):
 
 
 def get_module(user, request, location, student_module_cache, position=None):
-    ''' Get an instance of the xmodule class corresponding to module_xml,
+    ''' Get an instance of the xmodule class identified by location,
     setting the state based on an existing StudentModule, or creating one if none
     exists.
 
     Arguments:
       - user                  : current django User
       - request               : current django HTTPrequest
-      - module_xml            : lxml etree of xml subtree for the requested module
+      - location              : A Location-like object identifying the module to load
       - student_module_cache  : a StudentModuleCache
       - position              : extra information from URL for user-specified
                                 position within module
 
     Returns:
-      - a tuple (xmodule instance, instance_module, shared_module, module type).
+      - a tuple (xmodule instance, instance_module, shared_module, module category).
         instance_module is a StudentModule specific to this module for this student
         shared_module is a StudentModule specific to all modules with the same 'shared_state_key' attribute, or None if the module doesn't elect to share state
     '''

lms/templates/problem.html

@@ -30,9 +30,6 @@
     % if answer_available:
     <input class="show" type="button" value="Show Answer">
     % endif
-    % if explain :
-    <a href="/courseware/6.002_Spring_2012/${ explain }" class="new-page">Explanation</a>
-    % endif
     % if attempts_allowed :
     <section class="submission_feedback">
     You have used ${ attempts_used } of ${ attempts_allowed } submissions

rakefile

@@ -42,21 +42,6 @@ end
 task :default => [:test, :pep8, :pylint]
 
 directory REPORT_DIR
-directory LMS_REPORT_DIR
-
-desc "Run pep8 on all libraries"
-task :pep8 => REPORT_DIR do
-    sh("pep8 --ignore=E501 lms/djangoapps common/lib/* | tee #{REPORT_DIR}/pep8.report")
-end
-
-desc "Run pylint on all libraries"
-task :pylint => REPORT_DIR do
-    Dir["lms/djangoapps/*", "common/lib/*"].each do |app|
-        ENV['PYTHONPATH'] = File.dirname(app)
-        app = File.basename(app)
-        sh("pylint --rcfile=.pylintrc -f parseable #{app} | tee #{REPORT_DIR}/#{app}.pylint.report")
-    end
-end
 
 default_options = {
     :lms => '8000',
@@ -68,18 +53,39 @@ task :predjango do
     sh('pip install -e common/lib/xmodule')
 end
 
+[:lms, :cms, :common].each do |system|
+    report_dir = File.join(REPORT_DIR, system.to_s)
+    directory report_dir
+
+    desc "Run pep8 on all #{system} code"
+    task "pep8_#{system}" => report_dir do
+        sh("pep8 --ignore=E501 #{system}/djangoapps #{system}/lib | tee #{report_dir}/pep8.report")
+    end
+    task :pep8 => "pep8_#{system}"
+
+    desc "Run pylint on all #{system} code"
+    task "pylint_#{system}" => report_dir do
+        Dir["#{system}/djangoapps/*", "#{system}/lib/*"].each do |app|
+            ENV['PYTHONPATH'] = File.dirname(app)
+            app = File.basename(app)
+            sh("pylint --rcfile=.pylintrc -f parseable #{app} | tee #{report_dir}/#{app}.pylint.report")
+        end
+    end
+    task :pylint => "pylint_#{system}"
+end
+
 [:lms, :cms].each do |system|
-    task_name = "test_#{system}"
-    report_dir = File.join(REPORT_DIR, task_name)
+    report_dir = File.join(REPORT_DIR, system.to_s)
     directory report_dir
 
+    # Per System tasks
     desc "Run all django tests on our djangoapps for the #{system}"
-    task task_name => [report_dir, :predjango] do
+    task "test_#{system}" => [report_dir, :predjango] do
         ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml")
         ENV['NOSE_COVER_HTML_DIR'] = File.join(report_dir, "cover")
         sh(django_admin(system, :test, 'test', *Dir["#{system}/djangoapps/*"].each))
     end
-    task :test => task_name
+    task :test => "test_#{system}"
 
     desc <<-desc
         Start the #{system} locally with the specified environment (defaults to dev).
@@ -89,7 +95,8 @@ end
         args.with_defaults(:env => 'dev', :options => default_options[system])
         sh(django_admin(system, args.env, 'runserver', args.options))
     end
-    
+
+    # Per environment tasks
     Dir["#{system}/envs/*.py"].each do |env_file|
         env = File.basename(env_file).gsub(/\.py/, '')
         desc "Attempt to import the settings file #{system}.envs.#{env} and report any errors"