From 0e05d092caa4b6c5bae11eedcecc69a877d6397c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:12:29 -0400 Subject: [PATCH 01/22] Remove temporary export url in favor of export round trip test --- cms/djangoapps/contentstore/views.py | 13 ------------- cms/urls.py | 1 - 2 files changed, 14 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 185ea3868d..fd1b1bb782 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/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'), ) From 78f2a1ebe7d1fef0d1245a75f59c85474eb06bb2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:14:40 -0400 Subject: [PATCH 02/22] Add comments to LoncapaProblem constructor with types --- common/lib/capa/capa/capa_problem.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index f3f0187f1f..d624d1e423 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 ''' From 5463f2f348c53e372eda5fabc04ebf5ca8a0271f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:15:28 -0400 Subject: [PATCH 03/22] Log the correct id attribute in capa_problem --- common/lib/capa/capa/capa_problem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index d624d1e423..20aabbdf31 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -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 ''' From aeae2e02e243e9977eff036c7c4abf89cd416f43 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:19:47 -0400 Subject: [PATCH 04/22] Add docstrings to abtest with xml and json formats --- common/lib/xmodule/xmodule/abtest_module.py | 43 +++++++++++++++++---- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index c3e32732f3..979d04405d 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): @@ -64,11 +57,47 @@ 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: From c3964c54470dccb48e478b51eb251fd12bbdc374 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:21:09 -0400 Subject: [PATCH 05/22] Clarify CapaModule docstring --- common/lib/xmodule/xmodule/capa_module.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 1e1e27a65e..7744b3029f 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' From 9fdd5b11344929700791d097cf2b146a9cc26648 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:23:19 -0400 Subject: [PATCH 06/22] Don't dump the entire LoncapaProblem text in the unicode string --- common/lib/capa/capa/capa_problem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 20aabbdf31..46f6c7ad7c 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -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: From 1d90b686230be221d2335b3657d3b1b02c4a97b0 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:25:13 -0400 Subject: [PATCH 07/22] Renaming the mongo database and collection used for the xmodule module store --- cms/envs/dev.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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', } } } From e7d44a260624643bd365405baafd949efde9074a Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:28:38 -0400 Subject: [PATCH 08/22] Remove explain attribute in favor of solution stanza as exemplified in edx4edx --- common/lib/xmodule/xmodule/capa_module.py | 16 ---------------- lms/templates/problem.html | 3 --- 2 files changed, 19 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 7744b3029f..67e3aac93d 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -78,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) @@ -244,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, @@ -263,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/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 From 3cef71431e6471858f3aae57d1769e94bfc2879d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:46:41 -0400 Subject: [PATCH 09/22] Make mapping from xml attributes to metadata values less manual --- common/lib/xmodule/xmodule/xml_module.py | 26 +++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 140dd8d162..9d14236e7e 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -71,6 +71,13 @@ class XmlDescriptor(XModuleDescriptor): metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'due', 'graded', 'name', 'slug') + # A dictionary mapping xml attribute names to function of the value + # that return the metadata key and value + xml_attribute_map = { + 'graded': lambda val: ('graded', val == 'true'), + 'name': lambda val: ('display_name', val), + } + @classmethod def definition_from_xml(cls, xml_object, system): """ @@ -116,16 +123,15 @@ 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: + map_fn = cls.xml_attribute_map.get(attr) + if map_fn is None: + metadata[attr] = val + else: + key, val = map_fn(val) + metadata[key] = val return metadata From 77fea77726106d40b0ba551ddec9cca1da879388 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 2 Jul 2012 20:52:18 -0400 Subject: [PATCH 10/22] Define type of resource_fs in export_to_xml docstring --- common/lib/xmodule/xmodule/xml_module.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 9d14236e7e..4a744693a5 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -177,6 +177,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) From c7df948030948d0a38d7abf0cb61829970728d3b Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 3 Jul 2012 08:37:32 -0400 Subject: [PATCH 11/22] Run pep8 and pylint per environment --- rakefile | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/rakefile b/rakefile index 3d965e183b..82ac3f0490 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" From 780eb646fc5ddd285b5c9df072e150d6fc3ef8f4 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 09:53:03 -0400 Subject: [PATCH 12/22] Add note that metadata dictionary is scaffolding --- common/lib/xmodule/xmodule/x_module.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 From 8119e6b939dbe53ee77bc9c4b3c5c77ba396903b Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 10:21:39 -0400 Subject: [PATCH 13/22] Adding comment about future of user groups --- common/lib/xmodule/xmodule/abtest_module.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 979d04405d..8d6604b06b 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -53,6 +53,8 @@ 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 From 8e4c7c878ebc4ac4c46a0baede2c2c07f1a6b483 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 10:23:06 -0400 Subject: [PATCH 14/22] Add comment about location invalid chars --- common/lib/xmodule/xmodule/modulestore/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 1e8ee838d7..3930ee9ff3 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -19,6 +19,8 @@ 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') From cdd55373c69797a8bb114246f239ee44f1ed4f84 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 13:17:50 -0400 Subject: [PATCH 15/22] Make it clear that the module for backwards compatability is just that --- common/lib/xmodule/setup.py | 12 ++++++------ .../{translation_module.py => backcompat_module.py} | 0 2 files changed, 6 insertions(+), 6 deletions(-) rename common/lib/xmodule/xmodule/{translation_module.py => backcompat_module.py} (100%) 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/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 From a4d2245b44738977444d9ec5b2d3e65b55ac2994 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 13:20:34 -0400 Subject: [PATCH 16/22] Add comment about LazyLoadingDict --- common/lib/xmodule/xmodule/xml_module.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 4a744693a5..7673706b18 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -6,6 +6,10 @@ import logging 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 From f0717570c0960daafb6f7c29f2667093a9d6456c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 17:45:34 -0400 Subject: [PATCH 17/22] Clean up how mapping from metadata to xml attributes is done --- .../xmodule/xmodule/modulestore/__init__.py | 2 + common/lib/xmodule/xmodule/xml_module.py | 41 +++++++++++-------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 3930ee9ff3..e8b5e3ff66 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -24,6 +24,8 @@ URL_RE = re.compile(""" 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/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 7673706b18..9be9b0f900 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -3,6 +3,7 @@ from xmodule.x_module import XModuleDescriptor from lxml import etree import copy import logging +from collections import namedtuple log = logging.getLogger(__name__) @@ -62,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 @@ -75,11 +88,11 @@ class XmlDescriptor(XModuleDescriptor): metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'due', 'graded', 'name', 'slug') - # A dictionary mapping xml attribute names to function of the value + # A dictionary mapping xml attribute names to functions of the value # that return the metadata key and value xml_attribute_map = { - 'graded': lambda val: ('graded', val == 'true'), - 'name': lambda val: ('display_name', val), + 'graded': AttrMap('graded', lambda val: val == 'true', lambda val: str(val).lower()), + 'name': AttrMap('display_name'), } @classmethod @@ -130,12 +143,8 @@ class XmlDescriptor(XModuleDescriptor): for attr in cls.metadata_attributes: val = xml_object.get(attr) if val is not None: - map_fn = cls.xml_attribute_map.get(attr) - if map_fn is None: - metadata[attr] = val - else: - key, val = map_fn(val) - metadata[key] = val + attr_map = cls.xml_attribute_map.get(attr, AttrMap(attr)) + metadata[attr_map.metadata_key] = attr_map.to_metadata(val) return metadata @@ -203,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) From 1263d82d06abc5d19ae89795e20dff818bc7d3b0 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 17:54:17 -0400 Subject: [PATCH 18/22] Add comments on StudentModuleCache arguments --- lms/djangoapps/courseware/models.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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()] From 920c3972923ac039a5748a5f6276bc32c4d46a47 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 17:54:56 -0400 Subject: [PATCH 19/22] Be more specific about what the random number generator seed is --- lms/djangoapps/courseware/module_render.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 99c2a586c4..65f7f44a72 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 From e39e454ff202651b1b15161159a0c5aace12cb2d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 17:55:58 -0400 Subject: [PATCH 20/22] Correct comments for get_module --- lms/djangoapps/courseware/module_render.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 65f7f44a72..5dad342612 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -176,14 +176,14 @@ 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 From dfad0decc39bf4607d568ee2b01430b4ec57d23d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 17:59:49 -0400 Subject: [PATCH 21/22] Disambiguating name in XMLModuleStore definition --- common/lib/xmodule/xmodule/modulestore/xml.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 7474666e21..364850082d 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) From c468d47aa26e08609050b878b9cc2528280da914 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 11 Jul 2012 18:00:04 -0400 Subject: [PATCH 22/22] Minor change in comment about module type/category --- lms/djangoapps/courseware/module_render.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 5dad342612..7e1f28c180 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -189,7 +189,7 @@ def get_module(user, request, location, student_module_cache, position=None): 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 '''