From 8693d288c86d4813d731ec258cbbbb7d6cd7864c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 18 Dec 2012 09:32:36 -0500 Subject: [PATCH] Fix errors from running unit tests (some tests still fail) --- common/lib/xmodule/xmodule/abtest_module.py | 23 +++++--- common/lib/xmodule/xmodule/course_module.py | 4 +- common/lib/xmodule/xmodule/html_module.py | 4 +- common/lib/xmodule/xmodule/raw_module.py | 2 +- .../xmodule/xmodule/self_assessment_module.py | 2 +- common/lib/xmodule/xmodule/seq_module.py | 2 +- common/lib/xmodule/xmodule/x_module.py | 54 ++++++++++++++++--- common/lib/xmodule/xmodule/xml_module.py | 9 ++-- lms/djangoapps/courseware/access.py | 4 +- lms/djangoapps/courseware/grades.py | 2 + lms/djangoapps/courseware/module_render.py | 2 + 11 files changed, 78 insertions(+), 30 deletions(-) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 4ee74eb29c..9c639e8f30 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -79,7 +79,6 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): experiment = String(help="Experiment that this A/B test belongs to", scope=Scope.content) group_portions = Object(help="What proportions of students should go in each group", default={}) - group_assignments = Object(help="What group this user belongs to", scope=Scope(student=True, module=ModuleScope.TYPE), default={}) group_content = Object(help="What content to display to each group", scope=Scope.content, default={DEFAULT: []}) @classmethod @@ -99,12 +98,16 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): "ABTests must specify an experiment. Not found in:\n{xml}" .format(xml=etree.tostring(xml_object, pretty_print=True))) + group_portions = {} + group_content = {} + children = [] + for group in xml_object: if group.tag == 'default': name = DEFAULT else: name = group.get('name') - self.group_portions[name] = float(group.get('portion', 0)) + group_portions[name] = float(group.get('portion', 0)) child_content_urls = [] for child in group: @@ -114,19 +117,23 @@ class ABTestDescriptor(RawDescriptor, XmlDescriptor): log.exception("Unable to load child when parsing ABTest. Continuing...") continue - self.group_content[name] = child_content_urls - self.children.extend(child_content_urls) + group_content[name] = child_content_urls + children.extend(child_content_urls) default_portion = 1 - sum( - portion for (name, portion) in definition['data']['group_portions'].items()) + portion for (name, portion) in group_portions.items() + ) if default_portion < 0: raise InvalidDefinitionError("ABTest portions must add up to less than or equal to 1") - self.group_portions[DEFAULT] = default_portion - self.children.sort() + group_portions[DEFAULT] = default_portion + children.sort() - return definition + return { + 'group_portions': group_portions, + 'group_content': group_content, + }, children def definition_to_xml(self, resource_fs): xml_object = etree.Element('abtest') diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 596344d934..510857303a 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -270,12 +270,12 @@ class CourseDescriptor(SequenceDescriptor): wiki_slug = wiki_tag.attrib.get("slug", default=None) xml_object.remove(wiki_tag) - definition = super(CourseDescriptor, cls).definition_from_xml(xml_object, system) + definition, children = super(CourseDescriptor, cls).definition_from_xml(xml_object, system) definition.setdefault('data', {})['textbooks'] = textbooks definition['data']['wiki_slug'] = wiki_slug - return definition + return definition, children def has_ended(self): """ diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 6d86fb90a8..562eeeb361 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -87,7 +87,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: # html is special. cls.filename_extension is 'xml', but # if 'filename' is in the definition, that means to load @@ -131,7 +131,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # for Fall 2012 LMS migration: keep filename (and unmangled filename) definition['filename'] = [ filepath, filename ] - return definition + return definition, [] except (ResourceNotFoundError) as err: msg = 'Unable to load file contents at path {0}: {1} '.format( diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index 5ff16098ac..bdbc049712 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -13,7 +13,7 @@ class RawDescriptor(XmlDescriptor, XMLEditingDescriptor): """ @classmethod def definition_from_xml(cls, xml_object, system): - return {'data': etree.tostring(xml_object, pretty_print=True)} + return {'data': etree.tostring(xml_object, pretty_print=True)}, [] def definition_to_xml(self, resource_fs): try: diff --git a/common/lib/xmodule/xmodule/self_assessment_module.py b/common/lib/xmodule/xmodule/self_assessment_module.py index 2f4674cf7c..ca6eae9913 100644 --- a/common/lib/xmodule/xmodule/self_assessment_module.py +++ b/common/lib/xmodule/xmodule/self_assessment_module.py @@ -428,7 +428,7 @@ class SelfAssessmentDescriptor(XmlDescriptor, EditingDescriptor): 'prompt': parse('prompt'), 'submitmessage': parse('submitmessage'), 'hintprompt': parse('hintprompt'), - } + }, [] def definition_to_xml(self, resource_fs): diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 6e80d1cf61..a136473653 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -136,7 +136,7 @@ class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor): if system.error_tracker is not None: system.error_tracker("ERROR: " + str(e)) continue - return {'children': children} + return {}, children def definition_to_xml(self, resource_fs): xml_object = etree.Element('sequential') diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index a38b8e5549..6c499aa611 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -165,11 +165,8 @@ class XModule(HTMLSnippet): ''' Return module instances for all the children of this module. ''' - if not self.has_children: - return [] - if self._loaded_children is None: - children = [self.system.get_module(loc) for loc in self.children] + children = [self.system.get_module(loc) for loc in self.get_children_locations()] # get_module returns None if the current user doesn't have access # to the location. self._loaded_children = [c for c in children if c is not None] @@ -179,6 +176,24 @@ class XModule(HTMLSnippet): def __unicode__(self): return ''.format(self.id) + def get_children_locations(self): + ''' + Returns the locations of each of child modules. + + Overriding this changes the behavior of get_children and + anything that uses get_children, such as get_display_items. + + This method will not instantiate the modules of the children + unless absolutely necessary, so it is cheaper to call than get_children + + These children will be the same children returned by the + descriptor unless descriptor.has_dynamic_children() is true. + ''' + if not self.has_children: + return [] + + return self.children + def get_display_items(self): ''' Returns a list of descendent module instances that will display @@ -437,7 +452,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): on the contents of json_data. json_data must contain a 'location' element, and must be suitable to be - passed into the subclasses `from_json` method. + passed into the subclasses `from_json` method as model_data """ class_ = XModuleDescriptor.load_class( json_data['location']['category'], @@ -451,12 +466,35 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates): Creates an instance of this descriptor from the supplied json_data. This may be overridden by subclasses - json_data: A json object specifying the definition and any optional - keyword arguments for the XModuleDescriptor + json_data: A json object with the keys 'definition' and 'metadata', + definition: A json object with the keys 'data' and 'children' + data: A json value + children: A list of edX Location urls + metadata: A json object with any keys + + This json_data is transformed to model_data using the following rules: + 1) The model data contains all of the fields from metadata + 2) The model data contains the 'children' array + 3) If 'definition.data' is a json object, model data contains all of its fields + Otherwise, it contains the single field 'data' + 4) Any value later in this list overrides a value earlier in this list system: A DescriptorSystem for interacting with external resources """ - return cls(system=system, location=json_data['location'], model_data=json_data) + model_data = {} + model_data.update(json_data.get('metadata', {})) + + definition = json_data.get('definition', {}) + if 'children' in definition: + model_data['children'] = definition['children'] + + if 'data' in definition: + if isinstance(definition['data'], dict): + model_data.update(definition['data']) + else: + model_data['data'] = definition['data'] + + return cls(system=system, location=json_data['location'], model_data=model_data) # ================================= XML PARSING ============================ @staticmethod diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 50c4f7aa6d..754d9b523e 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -220,7 +220,7 @@ class XmlDescriptor(XModuleDescriptor): definition_metadata = get_metadata_from_xml(definition_xml) cls.clean_metadata_from_xml(definition_xml) - definition = cls.definition_from_xml(definition_xml, system) + definition, children = cls.definition_from_xml(definition_xml, system) if definition_metadata: definition['definition_metadata'] = definition_metadata @@ -228,7 +228,7 @@ class XmlDescriptor(XModuleDescriptor): # for Fall 2012 LMS migration: keep filename (and unmangled filename) definition['filename'] = [ filepath, filename ] - return definition + return definition, children @classmethod def load_metadata(cls, xml_object): @@ -289,7 +289,7 @@ class XmlDescriptor(XModuleDescriptor): else: definition_xml = xml_object # this is just a pointer, not the real definition content - definition = cls.load_definition(definition_xml, system, location) # note this removes metadata + definition, children = cls.load_definition(definition_xml, system, location) # note this removes metadata # VS[compat] -- make Ike's github preview links work in both old and # new file layouts if is_pointer_tag(xml_object): @@ -311,13 +311,12 @@ class XmlDescriptor(XModuleDescriptor): # Set/override any metadata specified by policy k = policy_key(location) if k in system.policy: - if k == 'video/labintro': print k, metadata, system.policy[k] cls.apply_policy(metadata, system.policy[k]) model_data = {} model_data.update(metadata) model_data.update(definition) - if k == 'video/labintro': print model_data + model_data['children'] = children return cls( system, diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index c6342e9d13..4318bf81bf 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -226,9 +226,9 @@ def _has_access_descriptor(user, descriptor, action, course_context=None): return True # Check start date - if descriptor.start is not None: + if descriptor.lms.start is not None: now = time.gmtime() - if now > descriptor.start: + if now > descriptor.lms.start: # after start date, everyone can see it debug("Allow: now > start date") return True diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index b81147f905..1d894d3707 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -36,6 +36,8 @@ def yield_dynamic_descriptor_descendents(descriptor, module_creator): def get_dynamic_descriptor_children(descriptor): if descriptor.has_dynamic_children(): module = module_creator(descriptor) + if module is None: + print "FOO", descriptor child_locations = module.get_children_locations() return [descriptor.system.load_item(child_location) for child_location in child_locations ] else: diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 0aac05568c..3fd1abec55 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -313,9 +313,11 @@ def _get_module(user, request, location, student_module_cache, course_id, positi import_system = descriptor.system if has_access(user, location, 'staff', course_id): err_descriptor = ErrorDescriptor.from_xml(str(descriptor), import_system, + org=descriptor.location.org, course=descriptor.location.course, error_msg=exc_info_to_str(sys.exc_info())) else: err_descriptor = NonStaffErrorDescriptor.from_xml(str(descriptor), import_system, + org=descriptor.location.org, course=descriptor.location.course, error_msg=exc_info_to_str(sys.exc_info())) # Make an error module