diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index d1839a0673..46d7c24d2d 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/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') diff --git a/cms/envs/dev.py b/cms/envs/dev.py index ca48481184..465d542c5b 100644 --- a/cms/envs/dev.py +++ b/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', } } } diff --git a/cms/urls.py b/cms/urls.py index 4695904b13..eb925a7069 100644 --- a/cms/urls.py +++ b/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[^/]+)/(?P[^/]+)/course/(?P[^/]+)$', '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'), ) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index f3f0187f1f..46f6c7ad7c 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/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 ''' diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 505537446f..b495cd0aee 100644 --- a/common/lib/xmodule/setup.py +++ b/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", ] } diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index c3e32732f3..8d6604b06b 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/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: - - - - - """ 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: + + + + + + """ experiment = xml_object.get('experiment') if experiment is None: diff --git a/common/lib/xmodule/xmodule/translation_module.py b/common/lib/xmodule/xmodule/backcompat_module.py similarity index 100% rename from common/lib/xmodule/xmodule/translation_module.py rename to common/lib/xmodule/xmodule/backcompat_module.py diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 1e1e27a65e..67e3aac93d 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/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(), } diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 4cc894f8e6..d7bcf8d94e 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -19,9 +19,13 @@ URL_RE = re.compile(""" (/(?P[^/]+))? """, 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. diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 17866be4af..c70defaf64 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index d0decd1ecf..ed678fdd0e 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 140dd8d162..9be9b0f900 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/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) diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index f0bd8dc17e..afb7f1c494 100644 --- a/lms/djangoapps/courseware/models.py +++ b/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()] diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 99c2a586c4..7e1f28c180 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/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 ''' diff --git a/lms/templates/problem.html b/lms/templates/problem.html index d051740710..a78a1042a6 100644 --- a/lms/templates/problem.html +++ b/lms/templates/problem.html @@ -30,9 +30,6 @@ % if answer_available: % endif - % if explain : - Explanation - % endif % if attempts_allowed :
You have used ${ attempts_used } of ${ attempts_allowed } submissions diff --git a/rakefile b/rakefile index 4e560978ba..073bb421b9 100644 --- a/rakefile +++ b/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].each do |system| - task_name = "test_#{system}" - report_dir = File.join(REPORT_DIR, task_name) +[: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| + 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"