From f25478b3d45bdc03f8d19e92644131d52caab624 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 29 Jun 2012 11:56:38 -0400 Subject: [PATCH] Consolidate access to metadata, and allow some of it to be inherited between modules --- common/lib/xmodule/capa_module.py | 14 ++--- common/lib/xmodule/seq_module.py | 4 +- common/lib/xmodule/x_module.py | 67 +++++++++++++++------- common/lib/xmodule/xml_module.py | 16 +++++- lms/djangoapps/courseware/grades.py | 18 +++--- lms/djangoapps/courseware/module_render.py | 20 +++---- lms/templates/profile.html | 2 +- lms/templates/staff_problem_info.html | 3 +- 8 files changed, 89 insertions(+), 55 deletions(-) diff --git a/common/lib/xmodule/capa_module.py b/common/lib/xmodule/capa_module.py index b6bfc91e80..9e82fbe8d4 100644 --- a/common/lib/xmodule/capa_module.py +++ b/common/lib/xmodule/capa_module.py @@ -125,7 +125,7 @@ class CapaModule(XModule): # User submitted a problem, and hasn't reset. We don't want # more submissions. - if self.lcp.done and self.rerandomize == "always": + if self.lcp.done and self.metadata['rerandomize'] == "always": check_button = False save_button = False @@ -184,15 +184,15 @@ class CapaModule(XModule): # 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 = only_one(dom2.xpath('/problem/@due')) - if len(display_due_date_string) > 0: + 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) #log.debug("Parsed " + display_due_date_string + " to " + str(self.display_due_date)) else: self.display_due_date = None - grace_period_string = only_one(dom2.xpath('/problem/@graceperiod')) - if len(grace_period_string) > 0 and self.display_due_date: + grace_period_string = self.metadata.get('graceperiod', None) + if grace_period_string is not None and self.display_due_date: self.grace_period = parse_timedelta(grace_period_string) self.close_date = self.display_due_date + self.grace_period #log.debug("Then parsed " + grace_period_string + " to closing date" + str(self.close_date)) @@ -206,12 +206,12 @@ class CapaModule(XModule): else: self.max_attempts = None - self.show_answer = only_one(dom2.xpath('/problem/@showanswer')) + self.show_answer = self.metadata.get('showanwser', 'closed') if self.show_answer == "": self.show_answer = "closed" - self.rerandomize = only_one(dom2.xpath('/problem/@rerandomize')) + self.rerandomize = self.metadata.get('rerandomize', 'always') if self.rerandomize == "" or self.rerandomize == "always" or self.rerandomize == "true": self.rerandomize = "always" elif self.rerandomize == "false" or self.rerandomize == "per_student": diff --git a/common/lib/xmodule/seq_module.py b/common/lib/xmodule/seq_module.py index 7ef497b837..ffcaed2599 100644 --- a/common/lib/xmodule/seq_module.py +++ b/common/lib/xmodule/seq_module.py @@ -52,9 +52,9 @@ class SequenceModule(XModule): contents.append({ 'content': child.get_html(), 'title': "\n".join( - grand_child.display_name.strip() + grand_child.metadata['display_name'].strip() for grand_child in child.get_children() - if grand_child.display_name is not None + if 'metadata' in grand_child.metadata ), 'progress_status': Progress.to_js_status_str(progress), 'progress_detail': Progress.to_js_detail_str(progress), diff --git a/common/lib/xmodule/x_module.py b/common/lib/xmodule/x_module.py index d8559c9bb7..c027d1d774 100644 --- a/common/lib/xmodule/x_module.py +++ b/common/lib/xmodule/x_module.py @@ -82,11 +82,9 @@ class XModule(object): self.shared_state = shared_state self.id = self.location.url() self.name = self.location.name - self.display_name = kwargs.get('display_name', '') self.type = self.location.category + self.metadata = kwargs.get('metadata', {}) self._loaded_children = None - self.graded = kwargs.get('graded', False) - self.format = kwargs.get('format') def get_name(self): name = self.__xmltree.get('name') @@ -188,6 +186,9 @@ class XModuleDescriptor(Plugin): js = {} js_module = None + # A list of metadata that this module can inherit from its parent module + inheritable_metadata = ('graded', 'due', 'graceperiod', 'showanswer', 'rerandomize') + @staticmethod def load_from_json(json_data, system, default_class=None): """ @@ -215,7 +216,11 @@ class XModuleDescriptor(Plugin): return cls(system=system, **json_data) @staticmethod - def load_from_xml(xml_data, system, org=None, course=None, default_class=None): + def load_from_xml(xml_data, + system, + org=None, + course=None, + default_class=None): """ This method instantiates the correct subclass of XModuleDescriptor based on the contents of xml_data. @@ -282,32 +287,48 @@ class XModuleDescriptor(Plugin): Current arguments passed in kwargs: location: A keystore.Location object indicating the name and ownership of this problem - goals: A list of strings of learning goals associated with this module - display_name: The name to use for displaying this module to the user - format: The format of this module ('Homework', 'Lab', etc) - graded (bool): Whether this module is should be graded or not + shared_state_key: The key to use for sharing StudentModules with other + modules of this type + metadata: A dictionary containing the following optional keys: + goals: A list of strings of learning goals associated with this module + display_name: The name to use for displaying this module to the user + format: The format of this module ('Homework', 'Lab', etc) + graded (bool): Whether this module is should be graded or not + due (string): The due date for this module + graceperiod (string): The amount of grace period to allow when enforcing the due date + showanswer (string): When to show answers for this module + rerandomize (string): When to generate a newly randomized instance of the module data """ self.system = system self.definition = definition if definition is not None else {} self.name = Location(kwargs.get('location')).name self.type = Location(kwargs.get('location')).category self.url = Location(kwargs.get('location')).url() - self.display_name = kwargs.get('display_name') - self.format = kwargs.get('format') - self.graded = kwargs.get('graded', False) + self.metadata = kwargs.get('metadata', {}) self.shared_state_key = kwargs.get('shared_state_key') - # For now, we represent goals as a list of strings, but this - # is one of the things that we are going to be iterating on heavily - # to find the best teaching method - self.goals = kwargs.get('goals', []) - self._child_instances = None + def inherit_metadata(self, metadata): + """ + Updates this module with metadata inherited from a containing module. + Only metadata specified in self.inheritable_metadata will + be inherited + """ + # Set all inheritable metadata from kwargs that are + # in self.inheritable_metadata and aren't already set in metadata + for attr in self.inheritable_metadata: + if attr not in self.metadata and attr in metadata: + self.metadata[attr] = metadata[attr] + def get_children(self): """Returns a list of XModuleDescriptor instances for the children of this module""" if self._child_instances is None: - self._child_instances = [self.system.load_item(child) for child in self.definition.get('children', [])] + self._child_instances = [] + for child_loc in self.definition.get('children', []): + child = self.system.load_item(child_loc) + child.inherit_metadata(self.metadata) + self._child_instances.append(child) return self._child_instances @@ -322,10 +343,14 @@ class XModuleDescriptor(Plugin): Returns a constructor for an XModule. This constructor takes two arguments: instance_state and shared_state, and returns a fully nstantiated XModule """ - return partial(self.module_class, system, self.url, self.definition, - display_name=self.display_name, - format=self.format, - graded=self.graded) + return partial( + self.module_class, + system, + self.url, + self.definition, + metadata=self.metadata + ) + class DescriptorSystem(object): def __init__(self, load_item, resources_fs): diff --git a/common/lib/xmodule/xml_module.py b/common/lib/xmodule/xml_module.py index d62957c3d3..b167e52e88 100644 --- a/common/lib/xmodule/xml_module.py +++ b/common/lib/xmodule/xml_module.py @@ -29,6 +29,18 @@ class XmlDescriptor(XModuleDescriptor): """ xml_object = etree.fromstring(xml_data) + 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') + return cls( system, cls.definition_from_xml(xml_object, system), @@ -37,7 +49,5 @@ class XmlDescriptor(XModuleDescriptor): course, xml_object.tag, xml_object.get('slug')], - display_name=xml_object.get('name'), - format=xml_object.get('format'), - graded=xml_object.get('graded') == 'true', + metadata=metadata, ) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index b5fcae86e5..deab9d47d4 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -87,7 +87,7 @@ def grade_sheet(student, course, student_module_cache): for module in yield_descendents(child): yield module - graded = getattr(s, 'graded', False) + graded = s.metadata.get('graded', False) scores = [] for module in yield_descendents(s): (correct, total) = get_score(student, module, student_module_cache) @@ -105,29 +105,27 @@ def grade_sheet(student, course, student_module_cache): #We simply cannot grade a problem that is 12/0, because we might need it as a percentage graded = False - scores.append(Score(correct, total, graded, module.display_name)) + scores.append(Score(correct, total, graded, module.metadata.get('display_name'))) - section_total, graded_total = graders.aggregate_scores(scores, s.display_name) + section_total, graded_total = graders.aggregate_scores(scores, s.metadata.get('display_name')) #Add the graded total to totaled_scores - format = getattr(s, 'format', "") - subtitle = getattr(s, 'subtitle', format) + format = s.metadata.get('format', "") if format and graded_total.possible > 0: format_scores = totaled_scores.get(format, []) format_scores.append(graded_total) totaled_scores[format] = format_scores sections.append({ - 'section': s.display_name, + 'section': s.metadata.get('display_name'), 'scores': scores, 'section_total': section_total, 'format': format, - 'subtitle': subtitle, - 'due': getattr(s, "due", ""), + 'due': s.metadata.get("due", ""), 'graded': graded, }) - chapters.append({'course': course.display_name, - 'chapter': c.display_name, + chapters.append({'course': course.metadata.get('display_name'), + 'chapter': c.metadata.get('display_name'), 'sections': sections}) grader = course_settings.GRADER diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index c2d20f1b67..ee2ccee018 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -137,17 +137,17 @@ def toc_for_course(user, request, course_location, active_chapter, active_sectio sections = list() for section in chapter.get_display_items(): - active = (chapter.display_name == active_chapter and - section.display_name == active_section) + active = (chapter.metadata.get('display_name') == active_chapter and + section.metadata.get('display_name') == active_section) - sections.append({'name': section.display_name, - 'format': getattr(section, 'format', ''), - 'due': getattr(section, 'due', ''), + sections.append({'name': section.metadata.get('display_name'), + 'format': section.metadata.get('format', ''), + 'due': section.metadata.get('due', ''), 'active': active}) - chapters.append({'name': chapter.display_name, + chapters.append({'name': chapter.metadata.get('display_name'), 'sections': sections, - 'active': chapter.display_name == active_chapter}) + 'active': chapter.metadata.get('display_name') == active_chapter}) return chapters @@ -171,7 +171,7 @@ def get_section(course, chapter, section): chapter_module = None for _chapter in course_module.get_children(): - if _chapter.display_name == chapter: + if _chapter.metadata.get('display_name') == chapter: chapter_module = _chapter break @@ -180,7 +180,7 @@ def get_section(course, chapter, section): section_module = None for _section in chapter_module.get_children(): - if _section.display_name == section: + if _section.metadata.get('display_name') == section: section_module = _section break @@ -271,9 +271,9 @@ def add_histogram(module): def get_html(): module_id = module.id histogram = grade_histogram(module_id) - print histogram render_histogram = len(histogram) > 0 staff_context = {'definition': json.dumps(module.definition, indent=4), + 'metadata': json.dumps(module.metadata, indent=4), 'element_id': module.location.html_id(), 'histogram': json.dumps(histogram), 'render_histogram': render_histogram, diff --git a/lms/templates/profile.html b/lms/templates/profile.html index e732616d5a..1ba0940eff 100644 --- a/lms/templates/profile.html +++ b/lms/templates/profile.html @@ -156,7 +156,7 @@ $(function() {

${ section['section'] } ${"({0:.3n}/{1:.3n}) {2}".format( float(earned), float(total), percentageString )}

- ${section['subtitle']} + ${section['format']} %if 'due' in section and section['due']!="": due ${section['due']} %endif diff --git a/lms/templates/staff_problem_info.html b/lms/templates/staff_problem_info.html index b5e07f8af4..2cc9de9df7 100644 --- a/lms/templates/staff_problem_info.html +++ b/lms/templates/staff_problem_info.html @@ -1,6 +1,7 @@ ${module_content}
-${definition | h} +definition = ${definition | h} +metadata = ${metadata | h}
%if render_histogram: