diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 597d74ce6f..380388b545 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -90,10 +90,12 @@ def add_histogram(get_html, module): # TODO (ichuang): Remove after fall 2012 LMS migration done if settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'): - [filepath, filename] = module.definition.get('filename','') + [filepath, filename] = module.definition.get('filename', ['', None]) osfs = module.system.filestore if filename is not None and osfs.exists(filename): - filepath = filename # if original, unmangled filename exists then use it (github doesn't like symlinks) + # if original, unmangled filename exists then use it (github + # doesn't like symlinks) + filepath = filename data_dir = osfs.root_path.rsplit('/')[-1] edit_link = "https://github.com/MITx/%s/tree/master/%s" % (data_dir,filepath) else: diff --git a/common/lib/capa/capa/inputtypes.py b/common/lib/capa/capa/inputtypes.py index 583d29b82e..5092e5c378 100644 --- a/common/lib/capa/capa/inputtypes.py +++ b/common/lib/capa/capa/inputtypes.py @@ -204,7 +204,7 @@ def extract_choices(element): raise Exception("[courseware.capa.inputtypes.extract_choices] \ Expected a tag; got %s instead" % choice.tag) - choice_text = ''.join([etree.tostring(x) for x in choice]) + choice_text = ''.join([x.text for x in choice]) choices.append((choice.get("name"), choice_text)) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 9420560c02..4a4e827752 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -800,6 +800,12 @@ class CodeResponse(LoncapaResponse): ''' Grade student code using an external queueing server, called 'xqueue' + Expects 'xqueue' dict in ModuleSystem with the following keys: + system.xqueue = { 'interface': XqueueInterface object, + 'callback_url': Per-StudentModule callback URL where results are posted (string), + 'default_queuename': Default queuename to submit request (string) + } + External requests are only submitted for student submission grading (i.e. and not for getting reference answers) ''' @@ -873,15 +879,16 @@ class CodeResponse(LoncapaResponse): 'edX_cmd': 'get_score', 'edX_tests': self.tests, 'processor': self.code, - 'edX_student_response': unicode(submission), # unicode on File object returns its filename } # Submit request - if hasattr(submission, 'read'): # Test for whether submission is a file + if is_file(submission): + contents.update({'edX_student_response': submission.name}) (error, msg) = qinterface.send_to_queue(header=xheader, body=json.dumps(contents), file_to_upload=submission) else: + contents.update({'edX_student_response': submission}) (error, msg) = qinterface.send_to_queue(header=xheader, body=json.dumps(contents)) diff --git a/common/lib/capa/capa/util.py b/common/lib/capa/capa/util.py index 1dc113cd20..005494e8c0 100644 --- a/common/lib/capa/capa/util.py +++ b/common/lib/capa/capa/util.py @@ -39,5 +39,18 @@ def convert_files_to_filenames(answers): ''' new_answers = dict() for answer_id in answers.keys(): - new_answers[answer_id] = unicode(answers[answer_id]) + if is_file(answers[answer_id]): + new_answers[answer_id] = answers[answer_id].name + else: + new_answers[answer_id] = answers[answer_id] return new_answers + +def is_file(file_to_test): + ''' + Duck typing to check if 'file_to_test' is a File object + ''' + is_file = True + for method in ['read', 'name']: + if not hasattr(file_to_test, method): + is_file = False + return is_file diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 8d6604b06b..9d4744e2c9 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -103,7 +103,9 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): experiment = xml_object.get('experiment') if experiment is None: - raise InvalidDefinitionError("ABTests must specify an experiment. Not found in:\n{xml}".format(xml=etree.tostring(xml_object, pretty_print=True))) + raise InvalidDefinitionError( + "ABTests must specify an experiment. Not found in:\n{xml}" + .format(xml=etree.tostring(xml_object, pretty_print=True))) definition = { 'data': { @@ -127,7 +129,9 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): definition['data']['group_content'][name] = child_content_urls definition['children'].extend(child_content_urls) - default_portion = 1 - sum(portion for (name, portion) in definition['data']['group_portions'].items()) + default_portion = 1 - sum( + portion for (name, portion) in definition['data']['group_portions'].items()) + if default_portion < 0: raise InvalidDefinitionError("ABTest portions must add up to less than or equal to 1") diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 7ce76def32..ba49f98257 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -119,9 +119,9 @@ class CapaModule(XModule): if self.show_answer == "": self.show_answer = "closed" - if instance_state != None: + if instance_state is not None: instance_state = json.loads(instance_state) - if instance_state != None and 'attempts' in instance_state: + if instance_state is not None and 'attempts' in instance_state: self.attempts = instance_state['attempts'] self.name = only_one(dom2.xpath('/problem/@name')) @@ -130,7 +130,7 @@ class CapaModule(XModule): if weight_string: self.weight = float(weight_string) else: - self.weight = 1 + self.weight = None if self.rerandomize == 'never': seed = 1 @@ -238,7 +238,7 @@ class CapaModule(XModule): content = {'name': self.metadata['display_name'], 'html': html, 'weight': self.weight, - } + } # We using strings as truthy values, because the terminology of the # check button is context-specific. @@ -563,6 +563,11 @@ class CapaDescriptor(RawDescriptor): module_class = CapaModule + # Capa modules have some additional metadata: + # TODO (vshnayder): do problems have any other metadata? Do they + # actually use type and points? + metadata_attributes = RawDescriptor.metadata_attributes + ('type', 'points') + # VS[compat] # TODO (cpennington): Delete this method once all fall 2012 course are being # edited in the cms @@ -572,8 +577,3 @@ class CapaDescriptor(RawDescriptor): 'problems/' + path[8:], path[8:], ] - - @classmethod - def split_to_file(cls, xml_object): - '''Problems always written in their own files''' - return True diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index acdc574220..ce1b93aa44 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -3,6 +3,7 @@ import time import dateutil.parser import logging +from xmodule.util.decorators import lazyproperty from xmodule.graders import load_grading_policy from xmodule.modulestore import Location from xmodule.seq_module import SequenceDescriptor, SequenceModule @@ -12,13 +13,9 @@ log = logging.getLogger(__name__) class CourseDescriptor(SequenceDescriptor): module_class = SequenceModule - metadata_attributes = SequenceDescriptor.metadata_attributes + ('org', 'course') def __init__(self, system, definition=None, **kwargs): super(CourseDescriptor, self).__init__(system, definition, **kwargs) - - self._grader = None - self._grade_cutoffs = None msg = None try: @@ -39,34 +36,84 @@ class CourseDescriptor(SequenceDescriptor): def has_started(self): return time.gmtime() > self.start - + @property def grader(self): - self.__load_grading_policy() - return self._grader - + return self.__grading_policy['GRADER'] + @property def grade_cutoffs(self): - self.__load_grading_policy() - return self._grade_cutoffs - - - def __load_grading_policy(self): - if not self._grader or not self._grade_cutoffs: - policy_string = "" - - try: - with self.system.resources_fs.open("grading_policy.json") as grading_policy_file: - policy_string = grading_policy_file.read() - except (IOError, ResourceNotFoundError): - log.warning("Unable to load course settings file from grading_policy.json in course " + self.id) - - grading_policy = load_grading_policy(policy_string) - - self._grader = grading_policy['GRADER'] - self._grade_cutoffs = grading_policy['GRADE_CUTOFFS'] - - + return self.__grading_policy['GRADE_CUTOFFS'] + + @lazyproperty + def __grading_policy(self): + policy_string = "" + + try: + with self.system.resources_fs.open("grading_policy.json") as grading_policy_file: + policy_string = grading_policy_file.read() + except (IOError, ResourceNotFoundError): + log.warning("Unable to load course settings file from grading_policy.json in course " + self.id) + + grading_policy = load_grading_policy(policy_string) + + return grading_policy + + + @lazyproperty + def grading_context(self): + """ + This returns a dictionary with keys necessary for quickly grading + a student. They are used by grades.grade() + + The grading context has two keys: + graded_sections - This contains the sections that are graded, as + well as all possible children modules that can affect the + grading. This allows some sections to be skipped if the student + hasn't seen any part of it. + + The format is a dictionary keyed by section-type. The values are + arrays of dictionaries containing + "section_descriptor" : The section descriptor + "xmoduledescriptors" : An array of xmoduledescriptors that + could possibly be in the section, for any student + + all_descriptors - This contains a list of all xmodules that can + effect grading a student. This is used to efficiently fetch + all the xmodule state for a StudentModuleCache without walking + the descriptor tree again. + + + """ + + all_descriptors = [] + graded_sections = {} + + def yield_descriptor_descendents(module_descriptor): + for child in module_descriptor.get_children(): + yield child + for module_descriptor in yield_descriptor_descendents(child): + yield module_descriptor + + for c in self.get_children(): + sections = [] + for s in c.get_children(): + if s.metadata.get('graded', False): + # TODO: Only include modules that have a score here + xmoduledescriptors = [child for child in yield_descriptor_descendents(s)] + + section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : xmoduledescriptors} + + section_format = s.metadata.get('format', "") + graded_sections[ section_format ] = graded_sections.get( section_format, [] ) + [section_description] + + all_descriptors.extend(xmoduledescriptors) + all_descriptors.append(s) + + return { 'graded_sections' : graded_sections, + 'all_descriptors' : all_descriptors,} + + @staticmethod def id_to_location(course_id): '''Convert the given course_id (org/course/name) to a location object. diff --git a/common/lib/xmodule/xmodule/css/video/display.scss b/common/lib/xmodule/xmodule/css/video/display.scss index 789a267755..bb69c30abb 100644 --- a/common/lib/xmodule/xmodule/css/video/display.scss +++ b/common/lib/xmodule/xmodule/css/video/display.scss @@ -207,7 +207,7 @@ div.video { h3 { color: #999; float: left; - font-size: 12px; + font-size: em(14); font-weight: normal; letter-spacing: 1px; padding: 0 lh(.25) 0 lh(.5); @@ -221,6 +221,7 @@ div.video { margin-bottom: 0; padding: 0 lh(.5) 0 0; line-height: 46px; + color: #fff; } &:hover, &:active, &:focus { diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index ecc90873b9..882d181b8b 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/common/lib/xmodule/xmodule/error_module.py @@ -1,5 +1,8 @@ -import sys +import hashlib import logging +import random +import string +import sys from pkg_resources import resource_string from lxml import etree @@ -35,7 +38,8 @@ class ErrorDescriptor(EditingDescriptor): error_msg='Error not available'): '''Create an instance of this descriptor from the supplied data. - Does not try to parse the data--just stores it. + Does not require that xml_data be parseable--just stores it and exports + as-is if not. Takes an extra, optional, parameter--the error that caused an issue. (should be a string, or convert usefully into one). @@ -45,6 +49,13 @@ class ErrorDescriptor(EditingDescriptor): definition = {'data': inner} inner['error_msg'] = str(error_msg) + # Pick a unique url_name -- the sha1 hash of the xml_data. + # NOTE: We could try to pull out the url_name of the errored descriptor, + # but url_names aren't guaranteed to be unique between descriptor types, + # and ErrorDescriptor can wrap any type. When the wrapped module is fixed, + # it will be written out with the original url_name. + url_name = hashlib.sha1(xml_data).hexdigest() + try: # If this is already an error tag, don't want to re-wrap it. xml_obj = etree.fromstring(xml_data) @@ -63,7 +74,7 @@ class ErrorDescriptor(EditingDescriptor): inner['contents'] = xml_data # TODO (vshnayder): Do we need a unique slug here? Just pick a random # 64-bit num? - location = ['i4x', org, course, 'error', 'slug'] + location = ['i4x', org, course, 'error', url_name] metadata = {} # stays in the xml_data return cls(system, definition, location=location, metadata=metadata) diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 260b84278b..d8837d876f 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -13,6 +13,7 @@ from .html_checker import check_html log = logging.getLogger("mitx.courseware") + class HtmlModule(XModule): def get_html(self): return self.html @@ -36,18 +37,19 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # are being edited in the cms @classmethod def backcompat_paths(cls, path): - origpath = path if path.endswith('.html.xml'): - path = path[:-9] + '.html' #backcompat--look for html instead of xml + path = path[:-9] + '.html' # backcompat--look for html instead of xml candidates = [] while os.sep in path: candidates.append(path) _, _, path = path.partition(os.sep) # also look for .html versions instead of .xml - if origpath.endswith('.xml'): - candidates.append(origpath[:-4] + '.html') - return candidates + nc = [] + for candidate in candidates: + if candidate.endswith('.xml'): + nc.append(candidate[:-4] + '.html') + return candidates + nc # NOTE: html descriptors are special. We do not want to parse and # export them ourselves, because that can break things (e.g. lxml @@ -69,7 +71,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): if filename is None: definition_xml = copy.deepcopy(xml_object) cls.clean_metadata_from_xml(definition_xml) - return {'data' : stringify_children(definition_xml)} + return {'data': stringify_children(definition_xml)} else: filepath = cls._format_filepath(xml_object.tag, filename) @@ -80,7 +82,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # online and has imported all current (fall 2012) courses from xml if not system.resources_fs.exists(filepath): candidates = cls.backcompat_paths(filepath) - #log.debug("candidates = {0}".format(candidates)) + log.debug("candidates = {0}".format(candidates)) for candidate in candidates: if system.resources_fs.exists(candidate): filepath = candidate @@ -95,7 +97,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): log.warning(msg) system.error_tracker("Warning: " + msg) - definition = {'data' : html} + definition = {'data': html} # TODO (ichuang): remove this after migration # for Fall 2012 LMS migration: keep filename (and unmangled filename) @@ -109,17 +111,11 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # add more info and re-raise raise Exception(msg), None, sys.exc_info()[2] - @classmethod - def split_to_file(cls, xml_object): - '''Never include inline html''' - return True - - # TODO (vshnayder): make export put things in the right places. def definition_to_xml(self, resource_fs): '''If the contents are valid xml, write them to filename.xml. Otherwise, - write just the tag to filename.xml, and the html + write just to filename.xml, and the html string to filename.html. ''' try: @@ -138,4 +134,3 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): elt = etree.Element('html') elt.set("filename", self.url_name) return elt - diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 46fcf19469..f9093f355a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -188,21 +188,26 @@ class XMLModuleStore(ModuleStoreBase): course_file = StringIO(clean_out_mako_templating(course_file.read())) course_data = etree.parse(course_file).getroot() + org = course_data.get('org') if org is None: - log.error("No 'org' attribute set for course in {dir}. " + msg = ("No 'org' attribute set for course in {dir}. " "Using default 'edx'".format(dir=course_dir)) + log.error(msg) + tracker(msg) org = 'edx' course = course_data.get('course') if course is None: - log.error("No 'course' attribute set for course in {dir}." + msg = ("No 'course' attribute set for course in {dir}." " Using default '{default}'".format( dir=course_dir, default=course_dir )) + log.error(msg) + tracker(msg) course = course_dir system = ImportSystem(self, org, course, course_dir, tracker) diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 5f7f41bf8d..e742a58471 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -122,16 +122,3 @@ class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor): etree.fromstring(child.export_to_xml(resource_fs))) return xml_object - @classmethod - def split_to_file(cls, xml_object): - # Note: if we end up needing subclasses, can port this logic there. - yes = ('chapter',) - no = ('course',) - - if xml_object.tag in yes: - return True - elif xml_object.tag in no: - return False - - # otherwise maybe--delegate to superclass. - return XmlDescriptor.split_to_file(xml_object) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 7f6fcfe00c..1115d85c7f 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -15,6 +15,7 @@ import xmodule import capa.calc as calc import capa.capa_problem as lcp from capa.correctmap import CorrectMap +from capa.util import convert_files_to_filenames from xmodule import graders, x_module from xmodule.x_module import ModuleSystem from xmodule.graders import Score, aggregate_scores @@ -31,7 +32,7 @@ i4xs = ModuleSystem( user=Mock(), filestore=fs.osfs.OSFS(os.path.dirname(os.path.realpath(__file__))), debug=True, - xqueue_callback_url='/', + xqueue={'interface':None, 'callback_url':'/', 'default_queuename': 'testqueue'}, is_staff=False ) @@ -278,7 +279,6 @@ class StringResponseWithHintTest(unittest.TestCase): class CodeResponseTest(unittest.TestCase): ''' Test CodeResponse - ''' def test_update_score(self): problem_file = os.path.dirname(__file__) + "/test_files/coderesponse.xml" @@ -327,7 +327,18 @@ class CodeResponseTest(unittest.TestCase): self.assertFalse(test_lcp.correct_map.is_queued(answer_ids[j])) # Should be dequeued, message delivered else: self.assertTrue(test_lcp.correct_map.is_queued(answer_ids[j])) # Should be queued, message undelivered - + + def test_convert_files_to_filenames(self): + problem_file = os.path.dirname(__file__) + "/test_files/coderesponse.xml" + fp = open(problem_file) + answers_with_file = {'1_2_1': 'String-based answer', + '1_3_1': ['answer1', 'answer2', 'answer3'], + '1_4_1': fp} + answers_converted = convert_files_to_filenames(answers_with_file) + self.assertEquals(answers_converted['1_2_1'], 'String-based answer') + self.assertEquals(answers_converted['1_3_1'], ['answer1', 'answer2', 'answer3']) + self.assertEquals(answers_converted['1_4_1'], fp.name) + class ChoiceResponseTest(unittest.TestCase): diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index eacf8352be..bcbf81861c 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -1,36 +1,107 @@ -from xmodule.modulestore.xml import XMLModuleStore -from nose.tools import assert_equals -from nose import SkipTest -from tempfile import mkdtemp +import unittest + from fs.osfs import OSFS +from nose.tools import assert_equals, assert_true +from path import path +from tempfile import mkdtemp + +from xmodule.modulestore.xml import XMLModuleStore + +# from ~/mitx_all/mitx/common/lib/xmodule/xmodule/tests/ +# to ~/mitx_all/mitx/common/test +TEST_DIR = path(__file__).abspath().dirname() +for i in range(4): + TEST_DIR = TEST_DIR.dirname() +TEST_DIR = TEST_DIR / 'test' + +DATA_DIR = TEST_DIR / 'data' -def check_export_roundtrip(data_dir): - print "Starting import" - initial_import = XMLModuleStore('org', 'course', data_dir, eager=True) - initial_course = initial_import.course +def strip_metadata(descriptor, key): + """ + Recursively strips tag from all children. + """ + print "strip {key} from {desc}".format(key=key, desc=descriptor.location.url()) + descriptor.metadata.pop(key, None) + for d in descriptor.get_children(): + strip_metadata(d, key) - print "Starting export" - export_dir = mkdtemp() - fs = OSFS(export_dir) - xml = initial_course.export_to_xml(fs) - with fs.open('course.xml', 'w') as course_xml: - course_xml.write(xml) - - print "Starting second import" - second_import = XMLModuleStore('org', 'course', export_dir, eager=True) - - print "Checking key equality" - assert_equals(initial_import.modules.keys(), second_import.modules.keys()) - - print "Checking module equality" - for location in initial_import.modules.keys(): - print "Checking", location - assert_equals(initial_import.modules[location], second_import.modules[location]) +def strip_filenames(descriptor): + """ + Recursively strips 'filename' from all children's definitions. + """ + print "strip filename from {desc}".format(desc=descriptor.location.url()) + descriptor.definition.pop('filename', None) + for d in descriptor.get_children(): + strip_filenames(d) -def test_toy_roundtrip(): - dir = "" - # TODO: add paths and make this run. - raise SkipTest() - check_export_roundtrip(dir) + +class RoundTripTestCase(unittest.TestCase): + '''Check that our test courses roundtrip properly''' + def check_export_roundtrip(self, data_dir, course_dir): + print "Starting import" + initial_import = XMLModuleStore(data_dir, eager=True, course_dirs=[course_dir]) + + courses = initial_import.get_courses() + self.assertEquals(len(courses), 1) + initial_course = courses[0] + + print "Starting export" + export_dir = mkdtemp() + print "export_dir: {0}".format(export_dir) + fs = OSFS(export_dir) + export_course_dir = 'export' + export_fs = fs.makeopendir(export_course_dir) + + xml = initial_course.export_to_xml(export_fs) + with export_fs.open('course.xml', 'w') as course_xml: + course_xml.write(xml) + + print "Starting second import" + second_import = XMLModuleStore(export_dir, eager=True, + course_dirs=[export_course_dir]) + + courses2 = second_import.get_courses() + self.assertEquals(len(courses2), 1) + exported_course = courses2[0] + + print "Checking course equality" + # HACK: data_dir metadata tags break equality because they + # aren't real metadata, and depend on paths. Remove them. + strip_metadata(initial_course, 'data_dir') + strip_metadata(exported_course, 'data_dir') + + # HACK: filenames change when changing file formats + # during imports from old-style courses. Ignore them. + strip_filenames(initial_course) + strip_filenames(exported_course) + + self.assertEquals(initial_course, exported_course) + + print "Checking key equality" + self.assertEquals(sorted(initial_import.modules.keys()), + sorted(second_import.modules.keys())) + + print "Checking module equality" + for location in initial_import.modules.keys(): + print "Checking", location + if location.category == 'html': + print ("Skipping html modules--they can't import in" + " final form without writing files...") + continue + self.assertEquals(initial_import.modules[location], + second_import.modules[location]) + + + def setUp(self): + self.maxDiff = None + + def test_toy_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "toy") + + def test_simple_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "simple") + + def test_full_roundtrip(self): + self.check_export_roundtrip(DATA_DIR, "full") diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 0d3e2260fb..407057a4ab 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -5,6 +5,7 @@ from fs.memoryfs import MemoryFS from lxml import etree from xmodule.x_module import XMLParsingSystem, XModuleDescriptor +from xmodule.xml_module import is_pointer_tag from xmodule.errortracker import make_error_tracker from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError @@ -46,22 +47,17 @@ class DummySystem(XMLParsingSystem): raise Exception("Shouldn't be called") - - class ImportTestCase(unittest.TestCase): '''Make sure module imports work properly, including for malformed inputs''' - - @staticmethod def get_system(): '''Get a dummy system''' return DummySystem() def test_fallback(self): - '''Make sure that malformed xml loads as an ErrorDescriptor.''' + '''Check that malformed xml loads as an ErrorDescriptor.''' bad_xml = '''''' - system = self.get_system() descriptor = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', 'course', @@ -70,6 +66,22 @@ class ImportTestCase(unittest.TestCase): self.assertEqual(descriptor.__class__.__name__, 'ErrorDescriptor') + + def test_unique_url_names(self): + '''Check that each error gets its very own url_name''' + bad_xml = '''''' + bad_xml2 = '''''' + system = self.get_system() + + descriptor1 = XModuleDescriptor.load_from_xml(bad_xml, system, 'org', + 'course', None) + + descriptor2 = XModuleDescriptor.load_from_xml(bad_xml2, system, 'org', + 'course', None) + + self.assertNotEqual(descriptor1.location, descriptor2.location) + + def test_reimport(self): '''Make sure an already-exported error xml tag loads properly''' @@ -111,30 +123,65 @@ class ImportTestCase(unittest.TestCase): xml_out = etree.fromstring(xml_str_out) self.assertEqual(xml_out.tag, 'sequential') - def test_metadata_inherit(self): - """Make sure metadata inherits properly""" + def test_metadata_import_export(self): + """Two checks: + - unknown metadata is preserved across import-export + - inherited metadata doesn't leak to children. + """ system = self.get_system() - v = "1 hour" - start_xml = ''' - - Two houses, ... - '''.format(grace=v) + v = '1 hour' + org = 'foo' + course = 'bbhh' + url_name = 'test1' + start_xml = ''' + + + Two houses, ... + + '''.format(grace=v, org=org, course=course, url_name=url_name) descriptor = XModuleDescriptor.load_from_xml(start_xml, system, - 'org', 'course') + org, course) - print "Errors: {0}".format(system.errorlog.errors) print descriptor, descriptor.metadata self.assertEqual(descriptor.metadata['graceperiod'], v) + self.assertEqual(descriptor.metadata['unicorn'], 'purple') - # Check that the child inherits correctly + # Check that the child inherits graceperiod correctly child = descriptor.get_children()[0] self.assertEqual(child.metadata['graceperiod'], v) - # Now export and see if the chapter tag has a graceperiod attribute + # check that the child does _not_ inherit any unicorns + self.assertTrue('unicorn' not in child.metadata) + + # Now export and check things resource_fs = MemoryFS() exported_xml = descriptor.export_to_xml(resource_fs) + + # Check that the exported xml is just a pointer print "Exported xml:", exported_xml - root = etree.fromstring(exported_xml) - chapter_tag = root[0] - self.assertEqual(chapter_tag.tag, 'chapter') - self.assertFalse('graceperiod' in chapter_tag.attrib) + pointer = etree.fromstring(exported_xml) + self.assertTrue(is_pointer_tag(pointer)) + # but it's a special case course pointer + self.assertEqual(pointer.attrib['course'], course) + self.assertEqual(pointer.attrib['org'], org) + + # Does the course still have unicorns? + with resource_fs.open('course/{url_name}.xml'.format(url_name=url_name)) as f: + course_xml = etree.fromstring(f.read()) + + self.assertEqual(course_xml.attrib['unicorn'], 'purple') + + # the course and org tags should be _only_ in the pointer + self.assertTrue('course' not in course_xml.attrib) + self.assertTrue('org' not in course_xml.attrib) + + # did we successfully strip the url_name from the definition contents? + self.assertTrue('url_name' not in course_xml.attrib) + + # Does the chapter tag now have a graceperiod attribute? + # hardcoded path to child + with resource_fs.open('chapter/ch.xml') as f: + chapter_xml = etree.fromstring(f.read()) + self.assertEqual(chapter_xml.tag, 'chapter') + self.assertFalse('graceperiod' in chapter_xml.attrib) diff --git a/common/lib/xmodule/xmodule/util/__init__.py b/common/lib/xmodule/xmodule/util/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/lib/xmodule/xmodule/util/decorators.py b/common/lib/xmodule/xmodule/util/decorators.py new file mode 100644 index 0000000000..81ab747a3e --- /dev/null +++ b/common/lib/xmodule/xmodule/util/decorators.py @@ -0,0 +1,36 @@ + + +def lazyproperty(fn): + """ + Use this decorator for lazy generation of properties that + are expensive to compute. From http://stackoverflow.com/a/3013910/86828 + + + Example: + class Test(object): + + @lazyproperty + def a(self): + print 'generating "a"' + return range(5) + + Interactive Session: + >>> t = Test() + >>> t.__dict__ + {} + >>> t.a + generating "a" + [0, 1, 2, 3, 4] + >>> t.__dict__ + {'_lazy_a': [0, 1, 2, 3, 4]} + >>> t.a + [0, 1, 2, 3, 4] + """ + + attr_name = '_lazy_' + fn.__name__ + @property + def _lazyprop(self): + if not hasattr(self, attr_name): + setattr(self, attr_name, fn(self)) + return getattr(self, attr_name) + return _lazyprop \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index f013aac785..f8f7499911 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -6,6 +6,7 @@ from fs.errors import ResourceNotFoundError from functools import partial from lxml import etree from lxml.etree import XMLSyntaxError +from pprint import pprint from xmodule.modulestore import Location from xmodule.errortracker import exc_info_to_str @@ -550,9 +551,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet): if not eq: for attr in self.equality_attributes: - print(getattr(self, attr, None), - getattr(other, attr, None), - getattr(self, attr, None) == getattr(other, attr, None)) + pprint((getattr(self, attr, None), + getattr(other, attr, None), + getattr(self, attr, None) == getattr(other, attr, None))) return eq @@ -643,7 +644,7 @@ class ModuleSystem(object): user=None, filestore=None, debug=False, - xqueue = None, + xqueue=None, is_staff=False): ''' Create a closure around the system environment. diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 7a12ed869d..c9fee6c9a5 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -11,22 +11,44 @@ import sys log = logging.getLogger(__name__) -_AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata') + +def is_pointer_tag(xml_obj): + """ + Check if xml_obj is a pointer tag: . + No children, one attribute named url_name. + + Special case for course roots: the pointer is + + + xml_obj: an etree Element + + Returns a bool. + """ + if xml_obj.tag != "course": + expected_attr = set(['url_name']) + else: + expected_attr = set(['url_name', 'course', 'org']) + + actual_attr = set(xml_obj.attrib.keys()) + return len(xml_obj) == 0 and actual_attr == expected_attr + + + +_AttrMapBase = namedtuple('_AttrMap', 'from_xml to_xml') class AttrMap(_AttrMapBase): """ - A class that specifies a metadata_key, and two functions: + A class that specifies two functions: - to_metadata: convert value from the xml representation into + from_xml: convert value from the xml representation into an internal python representation - from_metadata: convert the internal python representation into + to_xml: convert the internal python representation into the value to store in the xml. """ - 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) + def __new__(_cls, from_xml=lambda x: x, + to_xml=lambda x: x): + return _AttrMapBase.__new__(_cls, from_xml, to_xml) class XmlDescriptor(XModuleDescriptor): @@ -39,19 +61,28 @@ class XmlDescriptor(XModuleDescriptor): # The attributes will be removed from the definition xml passed # to definition_from_xml, and from the xml returned by definition_to_xml + + # Note -- url_name isn't in this list because it's handled specially on + # import and export. + + # TODO (vshnayder): Do we need a list of metadata we actually + # understand? And if we do, is this the place? + # Related: What's the right behavior for clean_metadata? metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc', - 'ispublic', # if True, then course is listed for all users; see + 'ispublic', # if True, then course is listed for all users; see # VS[compat] Remove once unused. 'name', 'slug') + metadata_to_strip = ('data_dir', + # VS[compat] -- remove the below attrs once everything is in the CMS + 'course', 'org', 'url_name', 'filename') # A dictionary mapping xml attribute names AttrMaps that describe how # to import and export them xml_attribute_map = { # type conversion: want True/False in python, "true"/"false" in xml - 'graded': AttrMap('graded', - lambda val: val == 'true', + 'graded': AttrMap(lambda val: val == 'true', lambda val: str(val).lower()), } @@ -101,12 +132,32 @@ class XmlDescriptor(XModuleDescriptor): """ return etree.parse(file_object).getroot() + @classmethod + def load_file(cls, filepath, fs, location): + ''' + Open the specified file in fs, and call cls.file_to_xml on it, + returning the lxml object. + + Add details and reraise on error. + ''' + try: + with fs.open(filepath) as file: + return cls.file_to_xml(file) + except Exception as err: + # Add info about where we are, but keep the traceback + msg = 'Unable to load file contents at path %s for item %s: %s ' % ( + filepath, location.url(), str(err)) + raise Exception, msg, sys.exc_info()[2] + + @classmethod def load_definition(cls, xml_object, system, location): '''Load a descriptor definition from the specified xml_object. Subclasses should not need to override this except in special cases (e.g. html module)''' + # VS[compat] -- the filename tag should go away once everything is + # converted. (note: make sure html files still work once this goes away) filename = xml_object.get('filename') if filename is None: definition_xml = copy.deepcopy(xml_object) @@ -120,22 +171,14 @@ class XmlDescriptor(XModuleDescriptor): # again in the correct format. This should go away once the CMS is # online and has imported all current (fall 2012) courses from xml if not system.resources_fs.exists(filepath) and hasattr( - cls, - 'backcompat_paths'): + cls, 'backcompat_paths'): candidates = cls.backcompat_paths(filepath) for candidate in candidates: if system.resources_fs.exists(candidate): filepath = candidate break - try: - with system.resources_fs.open(filepath) as file: - definition_xml = cls.file_to_xml(file) - except Exception: - msg = 'Unable to load file contents at path %s for item %s' % ( - filepath, location.url()) - # Add info about where we are, but keep the traceback - raise Exception, msg, sys.exc_info()[2] + definition_xml = cls.load_file(filepath, system.resources_fs, location) cls.clean_metadata_from_xml(definition_xml) definition = cls.definition_from_xml(definition_xml, system) @@ -146,6 +189,28 @@ class XmlDescriptor(XModuleDescriptor): return definition + @classmethod + def load_metadata(cls, xml_object): + """ + Read the metadata attributes from this xml_object. + + Returns a dictionary {key: value}. + """ + metadata = {} + for attr in xml_object.attrib: + val = xml_object.get(attr) + if val is not None: + # VS[compat]. Remove after all key translations done + attr = cls._translate(attr) + + if attr in cls.metadata_to_strip: + # don't load these + continue + + attr_map = cls.xml_attribute_map.get(attr, AttrMap()) + metadata[attr] = attr_map.from_xml(val) + return metadata + @classmethod def from_xml(cls, xml_data, system, org=None, course=None): @@ -160,26 +225,27 @@ class XmlDescriptor(XModuleDescriptor): url identifiers """ xml_object = etree.fromstring(xml_data) - # VS[compat] -- just have the url_name lookup once translation is done - slug = xml_object.get('url_name', xml_object.get('slug')) - location = Location('i4x', org, course, xml_object.tag, slug) + # VS[compat] -- just have the url_name lookup, once translation is done + url_name = xml_object.get('url_name', xml_object.get('slug')) + location = Location('i4x', org, course, xml_object.tag, url_name) - def load_metadata(): - metadata = {} - for attr in cls.metadata_attributes: - val = xml_object.get(attr) - if val is not None: - # VS[compat]. Remove after all key translations done - attr = cls._translate(attr) + # VS[compat] -- detect new-style each-in-a-file mode + if is_pointer_tag(xml_object): + # new style: + # read the actual definition file--named using url_name + filepath = cls._format_filepath(xml_object.tag, url_name) + definition_xml = cls.load_file(filepath, system.resources_fs, location) + else: + definition_xml = xml_object - attr_map = cls.xml_attribute_map.get(attr, AttrMap(attr)) - metadata[attr_map.metadata_key] = attr_map.to_metadata(val) - return metadata + definition = cls.load_definition(definition_xml, system, location) + # VS[compat] -- make Ike's github preview links work in both old and + # new file layouts + if is_pointer_tag(xml_object): + # new style -- contents actually at filepath + definition['filename'] = [filepath, filepath] - definition = cls.load_definition(xml_object, system, location) - metadata = load_metadata() - # VS[compat] -- just have the url_name lookup once translation is done - slug = xml_object.get('url_name', xml_object.get('slug')) + metadata = cls.load_metadata(definition_xml) return cls( system, definition, @@ -193,20 +259,6 @@ class XmlDescriptor(XModuleDescriptor): name=name, ext=cls.filename_extension) - @classmethod - def split_to_file(cls, xml_object): - ''' - Decide whether to write this object to a separate file or not. - - xml_object: an xml definition of an instance of cls. - - This default implementation will split if this has more than 7 - descendant tags. - - Can be overridden by subclasses. - ''' - return len(list(xml_object.iter())) > 7 - def export_to_xml(self, resource_fs): """ Returns an xml string representing this module, and all modules @@ -227,42 +279,39 @@ class XmlDescriptor(XModuleDescriptor): xml_object = self.definition_to_xml(resource_fs) self.__class__.clean_metadata_from_xml(xml_object) - # Set the tag first, so it's right if writing to a file + # Set the tag so we get the file path right xml_object.tag = self.category - # Write it to a file if necessary - if self.split_to_file(xml_object): - # Put this object in its own file - filepath = self.__class__._format_filepath(self.category, self.url_name) - resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) - with resource_fs.open(filepath, 'w') as file: - file.write(etree.tostring(xml_object, pretty_print=True)) - # ...and remove all of its children here - for child in xml_object: - xml_object.remove(child) - # also need to remove the text of this object. - xml_object.text = '' - # and the tail for good measure... - xml_object.tail = '' + def val_for_xml(attr): + """Get the value for this attribute that we want to store. + (Possible format conversion through an AttrMap). + """ + attr_map = self.xml_attribute_map.get(attr, AttrMap()) + return attr_map.to_xml(self.own_metadata[attr]) + # Add the non-inherited metadata + for attr in sorted(self.own_metadata): + # don't want e.g. data_dir + if attr not in self.metadata_to_strip: + xml_object.set(attr, val_for_xml(attr)) - xml_object.set('filename', self.url_name) + # Write the definition to a file + filepath = self.__class__._format_filepath(self.category, self.url_name) + resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True) + with resource_fs.open(filepath, 'w') as file: + file.write(etree.tostring(xml_object, pretty_print=True)) - # Add the metadata - xml_object.set('url_name', self.url_name) - for attr in self.metadata_attributes: - attr_map = self.xml_attribute_map.get(attr, AttrMap(attr)) - metadata_key = attr_map.metadata_key + # And return just a pointer with the category and filename. + record_object = etree.Element(self.category) + record_object.set('url_name', self.url_name) - if (metadata_key not in self.metadata or - metadata_key in self._inherited_metadata): - continue + # Special case for course pointers: + if self.category == 'course': + # add org and course attributes on the pointer tag + record_object.set('org', self.location.org) + record_object.set('course', self.location.course) - val = attr_map.from_metadata(self.metadata[metadata_key]) - xml_object.set(attr, val) - - # Now we just have to make it beautiful - return etree.tostring(xml_object, pretty_print=True) + return etree.tostring(record_object, pretty_print=True) def definition_to_xml(self, resource_fs): """ diff --git a/common/test/data/simple/course.xml b/common/test/data/simple/course.xml index b92158cdb7..86dc8df45c 100644 --- a/common/test/data/simple/course.xml +++ b/common/test/data/simple/course.xml @@ -2,7 +2,7 @@