From 2d085c426d608ded9981bea5e11a78dae1873448 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 24 Jul 2013 14:07:39 -0400 Subject: [PATCH 1/2] Add xlint check for any policy fields whose default changed. Warn user that the course behavior may change. --- .../xmodule/modulestore/xml_importer.py | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 71c6983644..8df768f442 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -138,7 +138,7 @@ def import_module_from_xml(modulestore, static_content_store, course_data_path, # For example, what I'm seeing is -> # Note the dropped element closing tag. This causes the LMS to fail when rendering modules - that's # no good, so we have to do this kludge - if isinstance(module.data, str) or isinstance(module.data, unicode): # some module 'data' fields are non strings which blows up the link traversal code + if isinstance(module.data, str) or isinstance(module.data, unicode): # some module 'data' fields are non strings which blows up the link traversal code lxml_rewrite_links(module.data, lambda link: verify_content_links(module, course_data_path, static_content_store, link, remap_dict)) for key in remap_dict.keys(): @@ -315,7 +315,7 @@ def import_module(module, store, course_data_path, static_content_store, allow_n # For example, what I'm seeing is -> # Note the dropped element closing tag. This causes the LMS to fail when rendering modules - that's # no good, so we have to do this kludge - if isinstance(module_data, str) or isinstance(module_data, unicode): # some module 'data' fields are non strings which blows up the link traversal code + if isinstance(module_data, str) or isinstance(module_data, unicode): # some module 'data' fields are non strings which blows up the link traversal code lxml_rewrite_links(module_data, lambda link: verify_content_links(module, course_data_path, static_content_store, link, remap_dict)) for key in remap_dict.keys(): @@ -523,6 +523,26 @@ def validate_data_source_paths(data_dir, course_dir): return err_cnt, warn_cnt +def validate_course_policy(module_store, course_id): + """ + Validate that the course explicitly sets values for any fields whose defaults may have changed between + the export and the import. + + Does not add to error count as these are just warnings. + """ + # is there a reliable way to get the module location just given the course_id? + warn_cnt = 0 + for module in module_store.modules[course_id].itervalues(): + if module.location.category == 'course': + if not 'rerandomize' in module._model_data: + warn_cnt += 1 + print 'WARN: course policy does not specify value for "rerandomize" whose default is now "never". The behavior of your course may change.' + if not 'showanswer' in module._model_data: + warn_cnt += 1 + print 'WARN: course policy does not specify value for "showanswer" whose default is now "finished". The behavior of your course may change.' + return warn_cnt + + def perform_xlint(data_dir, course_dirs, default_class='xmodule.raw_module.RawDescriptor', load_error_modules=True): @@ -568,6 +588,8 @@ def perform_xlint(data_dir, course_dirs, err_cnt += validate_category_hierarchy(module_store, course_id, "chapter", "sequential") # constrain that sequentials only have 'verticals' err_cnt += validate_category_hierarchy(module_store, course_id, "sequential", "vertical") + # validate the course policy overrides any defaults which have changed over time + warn_cnt += validate_course_policy(module_store, course_id) # don't allow metadata on verticals, since we can't edit them in studio err_cnt += validate_no_non_editable_metadata(module_store, course_id, "vertical") # don't allow metadata on chapters, since we can't edit them in studio From 400ff494cba050f5e61dedb3b3fc0f85d59e05e0 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 24 Jul 2013 14:08:34 -0400 Subject: [PATCH 2/2] Remove presumption of user's intent for imported xml attrs Moved to xlint rather than setting to old default. --- common/lib/xmodule/xmodule/capa_module.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index e8f8baf45f..6d11a02611 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -1155,20 +1155,6 @@ class CapaDescriptor(CapaFields, RawDescriptor): path[8:], ] - @classmethod - def from_xml(cls, xml_data, system, org=None, course=None): - """ - Augment regular translation w/ setting the pre-Studio defaults. - """ - problem = super(CapaDescriptor, cls).from_xml(xml_data, system, org, course) - course_policy = system.policy.setdefault('course/{}'.format(system.url_name), {}) - # pylint: disable=W0212 - if 'showanswer' not in problem._model_data and 'showanswer' not in course_policy: - problem.showanswer = "closed" - if 'rerandomize' not in problem._model_data and 'rerandomize' not in course_policy: - problem.rerandomize = "always" - return problem - @property def non_editable_metadata_fields(self): non_editable_fields = super(CapaDescriptor, self).non_editable_metadata_fields