From 021ed3bca671ae665979afec0168ba30b21121a8 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 29 Nov 2012 13:24:07 -0500 Subject: [PATCH 1/4] fix crash with working with custom tags with XML filestore --- common/lib/xmodule/xmodule/modulestore/xml.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index abd9c7d0eb..e3ad1fb0dd 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -53,6 +53,8 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): self.unnamed = defaultdict(int) # category -> num of new url_names for that category self.used_names = defaultdict(set) # category -> set of used url_names self.org, self.course, self.url_name = course_id.split('/') + # cdodge: adding the course_id as passed in for later reference rather than having to recomine the org/course/url_name + self.course_id = course_id self.load_error_modules = load_error_modules def process_xml(xml): From 050cb89cf77f9ba2f95682c3c5679594494a4100 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 29 Nov 2012 13:24:41 -0500 Subject: [PATCH 2/4] fix crash with working with custom tags with XML filestore --- common/lib/xmodule/xmodule/template_module.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/template_module.py b/common/lib/xmodule/xmodule/template_module.py index cca2cb0ca8..07ef2c6511 100644 --- a/common/lib/xmodule/xmodule/template_module.py +++ b/common/lib/xmodule/xmodule/template_module.py @@ -3,7 +3,7 @@ from xmodule.raw_module import RawDescriptor from lxml import etree from mako.template import Template from xmodule.modulestore.django import modulestore - +import logging class CustomTagModule(XModule): """ @@ -61,7 +61,7 @@ class CustomTagDescriptor(RawDescriptor): # cdodge: look up the template as a module template_loc = self.location._replace(category='custom_tag_template', name=template_name) - template_module = modulestore().get_item(template_loc) + template_module = modulestore().get_instance(system.course_id, template_loc) template_module_data = template_module.definition['data'] template = Template(template_module_data) return template.render(**params) From 46870f43914d85621f8f180f2cbbac54bfb7aa2f Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 29 Nov 2012 15:09:28 -0500 Subject: [PATCH 3/4] be sure to always pass in a course namespace - which can be just the location of the module itself since it has the same org/course pair - when rewriting links. Also, allow for an option parameter in get_module() to disable the wrapping of the module's HTML with the xmodule_display.html. This is needed for ancillary course content such as 'about', 'course info', which are now stored as xmodules, but should not be wrapped in xmodule_display.html as it breaks some styling --- common/lib/xmodule/xmodule/capa_module.py | 4 ++-- lms/djangoapps/courseware/courses.py | 8 +++----- lms/djangoapps/courseware/module_render.py | 13 +++++++++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index ea17c23bb4..525cf0ba87 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -347,7 +347,7 @@ class CapaModule(XModule): id=self.location.html_id(), ajax_url=self.system.ajax_url) + html + "" # now do the substitutions which are filesystem based, e.g. '/static/' prefixes - return self.system.replace_urls(html, self.metadata['data_dir']) + return self.system.replace_urls(html, self.metadata['data_dir'], self.location) def handle_ajax(self, dispatch, get): ''' @@ -451,7 +451,7 @@ class CapaModule(XModule): new_answers = dict() for answer_id in answers: try: - new_answer = {answer_id: self.system.replace_urls(answers[answer_id], self.metadata['data_dir'])} + new_answer = {answer_id: self.system.replace_urls(answers[answer_id], self.metadata['data_dir'], self.location)} except TypeError: log.debug('Unable to perform URL substitution on answers[%s]: %s' % (answer_id, answers[answer_id])) new_answer = {answer_id: answers[answer_id]} diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 6300b2c491..f3e435e4dc 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -148,7 +148,7 @@ def get_course_about_section(course, section_key): request = get_request_for_thread() loc = course.location._replace(category='about', name=section_key) - course_module = get_module(request.user, request, loc, None, course.id, not_found_ok = True) + course_module = get_module(request.user, request, loc, None, course.id, not_found_ok = True, wrap_xmodule_display = False) html = '' @@ -186,8 +186,7 @@ def get_course_info_section(request, cache, course, section_key): loc = Location(course.location.tag, course.location.org, course.location.course, 'course_info', section_key) - course_module = get_module(request.user, request, loc, cache, course.id) - + course_module = get_module(request.user, request, loc, cache, course.id, wrap_xmodule_display = False) html = '' if course_module is not None: @@ -196,7 +195,6 @@ def get_course_info_section(request, cache, course, section_key): return html - # TODO: Fix this such that these are pulled in as extra course-specific tabs. # arjun will address this by the end of October if no one does so prior to # then. @@ -222,7 +220,7 @@ def get_course_syllabus_section(course, section_key): filepath = find_file(fs, dirs, section_key + ".html") with fs.open(filepath) as htmlFile: return replace_urls(htmlFile.read().decode('utf-8'), - course.metadata['data_dir']) + course.metadata['data_dir'], course.location) except ResourceNotFoundError: log.exception("Missing syllabus section {key} in course {url}".format( key=section_key, url=course.location.url())) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 3880170169..9343301fb7 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -115,7 +115,7 @@ def toc_for_course(user, request, course, active_chapter, active_section): return chapters -def get_module(user, request, location, student_module_cache, course_id, position=None, not_found_ok = False): +def get_module(user, request, location, student_module_cache, course_id, position=None, not_found_ok = False, wrap_xmodule_display = True): """ Get an instance of the xmodule class identified by location, setting the state based on an existing StudentModule, or creating one if none @@ -136,7 +136,7 @@ def get_module(user, request, location, student_module_cache, course_id, positio if possible. If not possible, return None. """ try: - return _get_module(user, request, location, student_module_cache, course_id, position) + return _get_module(user, request, location, student_module_cache, course_id, position, wrap_xmodule_display) except ItemNotFoundError: if not not_found_ok: log.exception("Error in get_module") @@ -146,7 +146,7 @@ def get_module(user, request, location, student_module_cache, course_id, positio log.exception("Error in get_module") return None -def _get_module(user, request, location, student_module_cache, course_id, position=None): +def _get_module(user, request, location, student_module_cache, course_id, position=None, wrap_xmodule_display = True): """ Actually implement get_module. See docstring there for details. """ @@ -261,8 +261,13 @@ def _get_module(user, request, location, student_module_cache, course_id, positi # Make an error module return err_descriptor.xmodule_constructor(system)(None, None) + _get_html = module.get_html + + if wrap_xmodule_display == True: + _get_html = wrap_xmodule(module.get_html, module, 'xmodule_display.html') + module.get_html = replace_static_urls( - wrap_xmodule(module.get_html, module, 'xmodule_display.html'), + _get_html, module.metadata['data_dir'] if 'data_dir' in module.metadata else '', course_namespace = module.location._replace(category=None, name=None)) From 2aceb2e44a5e6aa487d1b246909ebbb567bdd659 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 29 Nov 2012 15:56:03 -0500 Subject: [PATCH 4/4] make sure when defining the course namespace in the replace url methods that we explicitly state the parameter name. There are some optional parameters and if we don't define the parameter name, it'll end put being interpreted as a different parameter --- common/djangoapps/static_replace.py | 1 + common/lib/xmodule/xmodule/capa_module.py | 4 ++-- lms/djangoapps/courseware/courses.py | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/static_replace.py b/common/djangoapps/static_replace.py index ee63e3cf93..e75362d784 100644 --- a/common/djangoapps/static_replace.py +++ b/common/djangoapps/static_replace.py @@ -60,6 +60,7 @@ def replace(static_url, prefix=None, course_namespace=None): def replace_urls(text, staticfiles_prefix=None, replace_prefix='/static/', course_namespace=None): + def replace_url(static_url): return replace(static_url, staticfiles_prefix, course_namespace = course_namespace) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 525cf0ba87..9922b1b8a0 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -347,7 +347,7 @@ class CapaModule(XModule): id=self.location.html_id(), ajax_url=self.system.ajax_url) + html + "" # now do the substitutions which are filesystem based, e.g. '/static/' prefixes - return self.system.replace_urls(html, self.metadata['data_dir'], self.location) + return self.system.replace_urls(html, self.metadata['data_dir'], course_namespace=self.location) def handle_ajax(self, dispatch, get): ''' @@ -451,7 +451,7 @@ class CapaModule(XModule): new_answers = dict() for answer_id in answers: try: - new_answer = {answer_id: self.system.replace_urls(answers[answer_id], self.metadata['data_dir'], self.location)} + new_answer = {answer_id: self.system.replace_urls(answers[answer_id], self.metadata['data_dir'], course_namespace=self.location)} except TypeError: log.debug('Unable to perform URL substitution on answers[%s]: %s' % (answer_id, answers[answer_id])) new_answer = {answer_id: answers[answer_id]} diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index f3e435e4dc..6de309076d 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -220,7 +220,7 @@ def get_course_syllabus_section(course, section_key): filepath = find_file(fs, dirs, section_key + ".html") with fs.open(filepath) as htmlFile: return replace_urls(htmlFile.read().decode('utf-8'), - course.metadata['data_dir'], course.location) + course.metadata['data_dir'], course_namespace=course.location) except ResourceNotFoundError: log.exception("Missing syllabus section {key} in course {url}".format( key=section_key, url=course.location.url()))