From 3dffcaca1cb620ab5863ff1af7f736d78f3e7cbc Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 08:42:08 -0400 Subject: [PATCH 01/15] Fixed capitalization in gradebook css. --- lms/templates/gradebook.html | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lms/templates/gradebook.html b/lms/templates/gradebook.html index e5721969e9..bdc1516339 100644 --- a/lms/templates/gradebook.html +++ b/lms/templates/gradebook.html @@ -11,11 +11,11 @@ <%static:css group='course'/> @@ -54,7 +54,7 @@ data_class = "grade_" + letter_grade %> - ${ "{0:.0%}".format( percentage ) } + ${ "{0:.0%}".format( percentage ) } %for student in students: From f872e41d1d8e091a162df8f050c91a62b2f7fd26 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 13:43:30 -0400 Subject: [PATCH 02/15] Added descriptor to xmodule init. Now xmodules have pointer to their descriptor. --- common/lib/xmodule/xmodule/abtest_module.py | 4 ++-- common/lib/xmodule/xmodule/capa_module.py | 4 ++-- common/lib/xmodule/xmodule/html_module.py | 4 ++-- common/lib/xmodule/xmodule/seq_module.py | 4 ++-- common/lib/xmodule/xmodule/template_module.py | 4 ++-- common/lib/xmodule/xmodule/vertical_module.py | 4 ++-- common/lib/xmodule/xmodule/video_module.py | 4 ++-- common/lib/xmodule/xmodule/x_module.py | 4 +++- 8 files changed, 17 insertions(+), 15 deletions(-) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index 8d6604b06b..9f64473fac 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -31,8 +31,8 @@ class ABTestModule(XModule): Implements an A/B test with an aribtrary number of competing groups """ - def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs) + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): + XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) if shared_state is None: diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index a384edf234..02717884bb 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -80,9 +80,9 @@ class CapaModule(XModule): js_module_name = "Problem" css = {'scss': [resource_string(__name__, 'css/capa/display.scss')]} - def __init__(self, system, location, definition, instance_state=None, + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, instance_state, + XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) self.attempts = 0 diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 260b84278b..a892184f87 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -17,9 +17,9 @@ class HtmlModule(XModule): def get_html(self): return self.html - def __init__(self, system, location, definition, + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, + XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) self.html = self.definition['data'] diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 5f7f41bf8d..5bef8596c2 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -25,9 +25,9 @@ class SequenceModule(XModule): css = {'scss': [resource_string(__name__, 'css/sequence/display.scss')]} js_module_name = "Sequence" - def __init__(self, system, location, definition, instance_state=None, + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, + XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) self.position = 1 diff --git a/common/lib/xmodule/xmodule/template_module.py b/common/lib/xmodule/xmodule/template_module.py index 260ad009f9..6ce81aa03c 100644 --- a/common/lib/xmodule/xmodule/template_module.py +++ b/common/lib/xmodule/xmodule/template_module.py @@ -26,9 +26,9 @@ class CustomTagModule(XModule): More information given in the text """ - def __init__(self, system, location, definition, + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, + XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) xmltree = etree.fromstring(self.definition['data']) diff --git a/common/lib/xmodule/xmodule/vertical_module.py b/common/lib/xmodule/xmodule/vertical_module.py index 884fd0a056..52e5825e43 100644 --- a/common/lib/xmodule/xmodule/vertical_module.py +++ b/common/lib/xmodule/xmodule/vertical_module.py @@ -10,8 +10,8 @@ class_priority = ['video', 'problem'] class VerticalModule(XModule): ''' Layout module for laying out submodules vertically.''' - def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs) + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): + XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) self.contents = None def get_html(self): diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index fb68ba982b..ec3a99584d 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -23,9 +23,9 @@ class VideoModule(XModule): css = {'scss': [resource_string(__name__, 'css/video/display.scss')]} js_module_name = "Video" - def __init__(self, system, location, definition, + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): - XModule.__init__(self, system, location, definition, + XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) xmltree = etree.fromstring(self.definition['data']) self.youtube = xmltree.get('youtube') diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index f013aac785..deac250e9d 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -142,7 +142,7 @@ class XModule(HTMLSnippet): # in the module icon_class = 'other' - def __init__(self, system, location, definition, + def __init__(self, system, location, definition, descriptor, instance_state=None, shared_state=None, **kwargs): ''' Construct a new xmodule @@ -188,6 +188,7 @@ class XModule(HTMLSnippet): self.system = system self.location = Location(location) self.definition = definition + self.descriptor = descriptor self.instance_state = instance_state self.shared_state = shared_state self.id = self.location.url() @@ -426,6 +427,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): system, self.location, self.definition, + self, metadata=self.metadata ) From 638a5059da994905294e478ca36aadced52d4923 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 14:34:05 -0400 Subject: [PATCH 03/15] Added stores_state attribute for xmoduledescriptors as a way to declare if the module stores state in the database. --- common/lib/xmodule/xmodule/capa_module.py | 2 ++ common/lib/xmodule/xmodule/seq_module.py | 2 ++ common/lib/xmodule/xmodule/video_module.py | 2 ++ common/lib/xmodule/xmodule/x_module.py | 4 ++++ lms/djangoapps/courseware/grades.py | 4 ++++ lms/djangoapps/courseware/models.py | 11 ++++++----- lms/djangoapps/courseware/module_render.py | 18 +++++++++++------- 7 files changed, 31 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 02717884bb..2af40fbbed 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -562,6 +562,8 @@ class CapaDescriptor(RawDescriptor): """ module_class = CapaModule + + stores_state = True # VS[compat] # TODO (cpennington): Delete this method once all fall 2012 course are being diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 5bef8596c2..2e5b799d3b 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -107,6 +107,8 @@ class SequenceModule(XModule): class SequenceDescriptor(MakoModuleDescriptor, XmlDescriptor): mako_template = 'widgets/sequence-edit.html' module_class = SequenceModule + + stores_state = True # For remembering where in the sequence the student is @classmethod def definition_from_xml(cls, xml_object, system): diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index ec3a99584d..7d11dea283 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -80,3 +80,5 @@ class VideoModule(XModule): class VideoDescriptor(RawDescriptor): module_class = VideoModule + + stores_state = True diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index deac250e9d..cfb1a664b0 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -303,6 +303,10 @@ class XModuleDescriptor(Plugin, HTMLSnippet): """ entry_point = "xmodule.v1" module_class = XModule + + # Attributes for inpsection of the descriptor + stores_state = False # Indicates wether the xmodule state should be + # stored in a database (independent of shared state) # A list of metadata that this module can inherit from its parent module inheritable_metadata = ( diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 925b8429c6..926040ab6f 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -183,6 +183,10 @@ def get_score(user, problem, student_module_cache): problem: an XModule cache: A StudentModuleCache """ + if not problem.descriptor.stores_state: + # These are not problems, and do not store state + return (None, None) + correct = 0.0 # If the ID is not in the cache, add the item diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index 631a750643..aa9f946733 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -69,10 +69,10 @@ class StudentModuleCache(object): """ def __init__(self, user, descriptors): ''' - Find any StudentModule objects that are needed by any child modules of the - supplied descriptor, or caches only the StudentModule objects specifically - for every descriptor in descriptors. Avoids making multiple queries to the - database. + Find any StudentModule objects that are needed by any descriptor + in descriptors. Avoids making multiple queries to the database. + Note: Only modules that have store_state = True or have shared + state will have a StudentModule. Arguments user: The user for which to fetch maching StudentModules @@ -134,7 +134,8 @@ class StudentModuleCache(object): ''' keys = [] for descriptor in descriptors: - keys.append(descriptor.location.url()) + if descriptor.stores_state: + keys.append(descriptor.location.url()) shared_state_key = getattr(descriptor, 'shared_state_key', None) if shared_state_key is not None: diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 5e74e6f42e..d60192fa31 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -127,19 +127,18 @@ def get_module(user, request, location, student_module_cache, position=None): descriptor = modulestore().get_item(location) #TODO Only check the cache if this module can possibly have state + instance_module = None + shared_module = None if user.is_authenticated(): - instance_module = student_module_cache.lookup(descriptor.category, - descriptor.location.url()) + if descriptor.stores_state: + instance_module = student_module_cache.lookup(descriptor.category, + descriptor.location.url()) shared_state_key = getattr(descriptor, 'shared_state_key', None) if shared_state_key is not None: shared_module = student_module_cache.lookup(descriptor.category, shared_state_key) - else: - shared_module = None - else: - instance_module = None - shared_module = None + instance_state = instance_module.state if instance_module is not None else None @@ -206,6 +205,11 @@ def get_instance_module(user, module, student_module_cache): or None if this is an anonymous user """ if user.is_authenticated(): + if not module.descriptor.stores_state: + log.exception("Attempted to get the instance_module for a module " + + str(module.id) + " which does not store state.") + return None + instance_module = student_module_cache.lookup(module.category, module.location.url()) From 98d4d44e4534584225398e89567f416c9fcf8659 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 14:49:00 -0400 Subject: [PATCH 04/15] Fixed test from previous commit. --- common/lib/xmodule/xmodule/tests/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 1115d85c7f..db454aa483 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -708,6 +708,6 @@ class ModuleProgressTest(unittest.TestCase): ''' def test_xmodule_default(self): '''Make sure default get_progress exists, returns None''' - xm = x_module.XModule(i4xs, 'a://b/c/d/e', {}) + xm = x_module.XModule(i4xs, 'a://b/c/d/e', None, {}) p = xm.get_progress() self.assertEqual(p, None) From a9d67c3e8d4276ae59cc00a79dd9ecb6baff74c8 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 15:20:47 -0400 Subject: [PATCH 05/15] Added has_score attribute to xmodule, for problem-like xmodules. --- common/lib/xmodule/xmodule/capa_module.py | 1 + common/lib/xmodule/xmodule/course_module.py | 8 ++++---- common/lib/xmodule/xmodule/x_module.py | 3 +++ lms/djangoapps/courseware/grades.py | 4 ++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 7ca33841dc..5501d6301c 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -564,6 +564,7 @@ class CapaDescriptor(RawDescriptor): module_class = CapaModule stores_state = True + has_score = True # Capa modules have some additional metadata: # TODO (vshnayder): do problems have any other metadata? Do they diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index ce1b93aa44..7ec9f54cd2 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -99,10 +99,10 @@ class CourseDescriptor(SequenceDescriptor): 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} + xmoduledescriptors = list(yield_descriptor_descendents(s)) + + # The xmoduledescriptors included here are only the ones that have scores. + section_description = { 'section_descriptor' : s, 'xmoduledescriptors' : filter(lambda child: child.has_score, xmoduledescriptors) } section_format = s.metadata.get('format', "") graded_sections[ section_format ] = graded_sections.get( section_format, [] ) + [section_description] diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 9ed1854c5b..7bc8a72731 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -308,6 +308,9 @@ class XModuleDescriptor(Plugin, HTMLSnippet): # Attributes for inpsection of the descriptor stores_state = False # Indicates wether the xmodule state should be # stored in a database (independent of shared state) + has_score = False # This indicates whether the xmodule is a problem-type. + # It should respond to max_score() and grade(). It can be graded or ungraded + # (like a practice problem). # A list of metadata that this module can inherit from its parent module inheritable_metadata = ( diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 926040ab6f..737e4245e0 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -183,8 +183,8 @@ def get_score(user, problem, student_module_cache): problem: an XModule cache: A StudentModuleCache """ - if not problem.descriptor.stores_state: - # These are not problems, and do not store state + if not problem.descriptor.stores_state or not problem.descriptor.has_score: + # These are not problems, and do not have a score return (None, None) correct = 0.0 From 1f34cf33fa2007641f5f6ceeae0dc98fd1d70fd5 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 16:01:42 -0400 Subject: [PATCH 06/15] Speed improvements by making the mako rendering in CustomTagModule lazy. --- common/lib/xmodule/xmodule/template_module.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/template_module.py b/common/lib/xmodule/xmodule/template_module.py index 6ce81aa03c..668dd1fb63 100644 --- a/common/lib/xmodule/xmodule/template_module.py +++ b/common/lib/xmodule/xmodule/template_module.py @@ -2,6 +2,7 @@ from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from lxml import etree from mako.template import Template +from util.decorators import lazyproperty class CustomTagModule(XModule): @@ -33,21 +34,26 @@ class CustomTagModule(XModule): xmltree = etree.fromstring(self.definition['data']) if 'impl' in xmltree.attrib: - template_name = xmltree.attrib['impl'] + self._template_name = xmltree.attrib['impl'] else: # VS[compat] backwards compatibility with old nested customtag structure child_impl = xmltree.find('impl') if child_impl is not None: - template_name = child_impl.text + self._template_name = child_impl.text else: # TODO (vshnayder): better exception type raise Exception("Could not find impl attribute in customtag {0}" .format(location)) + + self._params = dict(xmltree.items()) - params = dict(xmltree.items()) + + @lazyproperty + def html(self): with self.system.filestore.open( - 'custom_tags/{name}'.format(name=template_name)) as template: - self.html = Template(template.read()).render(**params) + 'custom_tags/{name}'.format(name=self._template_name)) as template: + return Template(template.read()).render(**self._params) + def get_html(self): return self.html From ec04b0cb139814df6febc8abc2fd09996596e646 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 16:16:31 -0400 Subject: [PATCH 07/15] Changed yield_module_descendents to be stack-based, so it plays nicer with the profiler. --- lms/djangoapps/courseware/grades.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 737e4245e0..3be3f954f9 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -12,10 +12,12 @@ from models import StudentModule log = logging.getLogger("mitx.courseware") def yield_module_descendents(module): - for child in module.get_display_items(): - yield child - for module in yield_module_descendents(child): - yield module + stack = module.get_display_items() + + while len(stack) > 0: + next_module = stack.pop() + stack.extend( next_module.get_display_items() ) + yield next_module def grade(student, request, course, student_module_cache=None): """ From 25662db914cf9e8b5169dc82f9f29afd4e64ce18 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 18:32:08 -0400 Subject: [PATCH 08/15] Lazily loading capa problems (for faster grading). --- common/lib/xmodule/xmodule/capa_module.py | 26 +++++++++++++++-------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 5501d6301c..f6b902dd73 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -11,13 +11,14 @@ from datetime import timedelta from lxml import etree from pkg_resources import resource_string -from xmodule.x_module import XModule -from xmodule.raw_module import RawDescriptor -from xmodule.exceptions import NotFoundError -from progress import Progress from capa.capa_problem import LoncapaProblem from capa.responsetypes import StudentInputError from capa.util import convert_files_to_filenames +from progress import Progress +from util.decorators import lazyproperty +from xmodule.x_module import XModule +from xmodule.raw_module import RawDescriptor +from xmodule.exceptions import NotFoundError log = logging.getLogger("mitx.courseware") @@ -118,9 +119,11 @@ class CapaModule(XModule): if self.show_answer == "": self.show_answer = "closed" - + if instance_state is not None: instance_state = json.loads(instance_state) + self._instance_state = instance_state # We will need this later to lazily load lcp + if instance_state is not None and 'attempts' in instance_state: self.attempts = instance_state['attempts'] @@ -132,16 +135,20 @@ class CapaModule(XModule): else: self.weight = None + + + @lazyproperty + def lcp(self): if self.rerandomize == 'never': seed = 1 elif self.rerandomize == "per_student" and hasattr(system, 'id'): seed = system.id else: seed = None - + try: - self.lcp = LoncapaProblem(self.definition['data'], self.location.html_id(), - instance_state, seed=seed, system=self.system) + return LoncapaProblem(self.definition['data'], self.location.html_id(), + self._instance_state, seed=seed, system=self.system) except Exception as err: msg = 'cannot create LoncapaProblem {loc}: {err}'.format( loc=self.location.url(), err=err) @@ -158,12 +165,13 @@ class CapaModule(XModule): problem_text = ('' 'Problem %s has an error:%s' % (self.location.url(), msg)) - self.lcp = LoncapaProblem( + return LoncapaProblem( problem_text, self.location.html_id(), instance_state, seed=seed, system=self.system) else: # add extra info and raise raise Exception(msg), None, sys.exc_info()[2] + @property def rerandomize(self): From eb58f7c37c7c095ee7579539f3f6297893e0ec7f Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 18:32:53 -0400 Subject: [PATCH 09/15] Changed to more specific import of xmodule.util.decorators. --- common/lib/xmodule/xmodule/capa_module.py | 2 +- common/lib/xmodule/xmodule/template_module.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index f6b902dd73..3fedc46715 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -15,7 +15,7 @@ from capa.capa_problem import LoncapaProblem from capa.responsetypes import StudentInputError from capa.util import convert_files_to_filenames from progress import Progress -from util.decorators import lazyproperty +from xmodule.util.decorators import lazyproperty from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from xmodule.exceptions import NotFoundError diff --git a/common/lib/xmodule/xmodule/template_module.py b/common/lib/xmodule/xmodule/template_module.py index 668dd1fb63..075ebba4ca 100644 --- a/common/lib/xmodule/xmodule/template_module.py +++ b/common/lib/xmodule/xmodule/template_module.py @@ -2,7 +2,7 @@ from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from lxml import etree from mako.template import Template -from util.decorators import lazyproperty +from xmodule.util.decorators import lazyproperty class CustomTagModule(XModule): From 718499d7933ed2684491c95e8a5d2dff203fdd22 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 18:37:27 -0400 Subject: [PATCH 10/15] Fix test failure from recent commit. --- common/lib/xmodule/xmodule/capa_module.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 3fedc46715..3563e23611 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -141,7 +141,7 @@ class CapaModule(XModule): def lcp(self): if self.rerandomize == 'never': seed = 1 - elif self.rerandomize == "per_student" and hasattr(system, 'id'): + elif self.rerandomize == "per_student" and hasattr(self.system, 'id'): seed = system.id else: seed = None @@ -167,7 +167,7 @@ class CapaModule(XModule): (self.location.url(), msg)) return LoncapaProblem( problem_text, self.location.html_id(), - instance_state, seed=seed, system=self.system) + self._instance_state, seed=seed, system=self.system) else: # add extra info and raise raise Exception(msg), None, sys.exc_info()[2] From 0fe2abbbdb00049125da2bd3bacafd8af6fad829 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Tue, 7 Aug 2012 18:39:19 -0400 Subject: [PATCH 11/15] Added docstring. --- common/lib/xmodule/xmodule/capa_module.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 3563e23611..4fefe237a5 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -139,6 +139,13 @@ class CapaModule(XModule): @lazyproperty def lcp(self): + """ + Loading the problem can be fairly expensive, so we do this lazily. + + This can throw errors. Always be sure to use try/catch before + displaying modules to a user. + """ + if self.rerandomize == 'never': seed = 1 elif self.rerandomize == "per_student" and hasattr(self.system, 'id'): From 696804da133c28c907afaae91b1ab1ab966b5611 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Wed, 8 Aug 2012 10:28:40 -0400 Subject: [PATCH 12/15] Changes from pull request comments. --- common/lib/xmodule/xmodule/x_module.py | 2 +- lms/djangoapps/courseware/grades.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 7bc8a72731..06b9fa0c26 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -306,7 +306,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): module_class = XModule # Attributes for inpsection of the descriptor - stores_state = False # Indicates wether the xmodule state should be + stores_state = False # Indicates whether the xmodule state should be # stored in a database (independent of shared state) has_score = False # This indicates whether the xmodule is a problem-type. # It should respond to max_score() and grade(). It can be graded or ungraded diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 3be3f954f9..deeae979c9 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -185,7 +185,7 @@ def get_score(user, problem, student_module_cache): problem: an XModule cache: A StudentModuleCache """ - if not problem.descriptor.stores_state or not problem.descriptor.has_score: + if not (problem.descriptor.stores_state and problem.descriptor.has_score): # These are not problems, and do not have a score return (None, None) From e4329c312ddbafa8b0d06734afd1663b1a6b0926 Mon Sep 17 00:00:00 2001 From: Bridger Maxwell Date: Wed, 8 Aug 2012 16:03:28 -0400 Subject: [PATCH 13/15] Fixed log of error. Xmodules don't have ids, so I log the location instead. --- lms/djangoapps/courseware/grades.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index deeae979c9..b0220670eb 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -91,7 +91,7 @@ def grade(student, request, course, student_module_cache=None): if graded_total.possible > 0: format_scores.append(graded_total) else: - log.exception("Unable to grade a section with a total possible score of zero. " + str(section_descriptor.id)) + log.exception("Unable to grade a section with a total possible score of zero. " + str(section_descriptor.location)) totaled_scores[section_format] = format_scores From 9d87710f59a7bb299efd312ef115f3f174747965 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 9 Aug 2012 10:31:48 -0400 Subject: [PATCH 14/15] Revert "Lazily loading capa problems (for faster grading)." We don't want lazyness, and will save the proper fix (moving work into descriptor load for later) --- common/lib/xmodule/xmodule/capa_module.py | 33 +++++++---------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 4fefe237a5..eb083e97db 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -15,7 +15,6 @@ from capa.capa_problem import LoncapaProblem from capa.responsetypes import StudentInputError from capa.util import convert_files_to_filenames from progress import Progress -from xmodule.util.decorators import lazyproperty from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from xmodule.exceptions import NotFoundError @@ -119,11 +118,9 @@ class CapaModule(XModule): if self.show_answer == "": self.show_answer = "closed" - + if instance_state is not None: instance_state = json.loads(instance_state) - self._instance_state = instance_state # We will need this later to lazily load lcp - if instance_state is not None and 'attempts' in instance_state: self.attempts = instance_state['attempts'] @@ -135,27 +132,18 @@ class CapaModule(XModule): else: self.weight = None - - - @lazyproperty - def lcp(self): - """ - Loading the problem can be fairly expensive, so we do this lazily. - - This can throw errors. Always be sure to use try/catch before - displaying modules to a user. - """ - if self.rerandomize == 'never': seed = 1 elif self.rerandomize == "per_student" and hasattr(self.system, 'id'): seed = system.id else: seed = None - + try: - return LoncapaProblem(self.definition['data'], self.location.html_id(), - self._instance_state, seed=seed, system=self.system) + # TODO (vshnayder): move as much as possible of this work and error + # checking to descriptor load time + self.lcp = LoncapaProblem(self.definition['data'], self.location.html_id(), + instance_state, seed=seed, system=self.system) except Exception as err: msg = 'cannot create LoncapaProblem {loc}: {err}'.format( loc=self.location.url(), err=err) @@ -172,13 +160,12 @@ class CapaModule(XModule): problem_text = ('' 'Problem %s has an error:%s' % (self.location.url(), msg)) - return LoncapaProblem( + self.lcp = LoncapaProblem( problem_text, self.location.html_id(), - self._instance_state, seed=seed, system=self.system) + instance_state, seed=seed, system=self.system) else: # add extra info and raise raise Exception(msg), None, sys.exc_info()[2] - @property def rerandomize(self): @@ -441,7 +428,7 @@ class CapaModule(XModule): event_info = dict() event_info['state'] = self.lcp.get_state() event_info['problem_id'] = self.location.url() - + answers = self.make_dict_of_responses(get) event_info['answers'] = convert_files_to_filenames(answers) @@ -577,7 +564,7 @@ class CapaDescriptor(RawDescriptor): """ module_class = CapaModule - + stores_state = True has_score = True From 9867dceca5c5720408663a0a49de73daa243e43b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 9 Aug 2012 10:47:08 -0400 Subject: [PATCH 15/15] Load templates at descriptor load * instead of being lazy --- common/lib/xmodule/xmodule/template_module.py | 51 ++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/common/lib/xmodule/xmodule/template_module.py b/common/lib/xmodule/xmodule/template_module.py index 075ebba4ca..a13d0fa095 100644 --- a/common/lib/xmodule/xmodule/template_module.py +++ b/common/lib/xmodule/xmodule/template_module.py @@ -2,7 +2,6 @@ from xmodule.x_module import XModule from xmodule.raw_module import RawDescriptor from lxml import etree from mako.template import Template -from xmodule.util.decorators import lazyproperty class CustomTagModule(XModule): @@ -31,33 +30,37 @@ class CustomTagModule(XModule): instance_state=None, shared_state=None, **kwargs): XModule.__init__(self, system, location, definition, descriptor, instance_state, shared_state, **kwargs) - - xmltree = etree.fromstring(self.definition['data']) - if 'impl' in xmltree.attrib: - self._template_name = xmltree.attrib['impl'] - else: - # VS[compat] backwards compatibility with old nested customtag structure - child_impl = xmltree.find('impl') - if child_impl is not None: - self._template_name = child_impl.text - else: - # TODO (vshnayder): better exception type - raise Exception("Could not find impl attribute in customtag {0}" - .format(location)) - - self._params = dict(xmltree.items()) - - - @lazyproperty - def html(self): - with self.system.filestore.open( - 'custom_tags/{name}'.format(name=self._template_name)) as template: - return Template(template.read()).render(**self._params) - + self.html = definition['html'] def get_html(self): return self.html class CustomTagDescriptor(RawDescriptor): + """ Descriptor for custom tags. Loads the template when created.""" module_class = CustomTagModule + + @classmethod + def definition_from_xml(cls, xml_object, system): + definition = RawDescriptor.definition_from_xml(xml_object, system) + + # Render the template and save it. + xmltree = etree.fromstring(definition['data']) + if 'impl' in xmltree.attrib: + template_name = xmltree.attrib['impl'] + else: + # VS[compat] backwards compatibility with old nested customtag structure + child_impl = xmltree.find('impl') + if child_impl is not None: + template_name = child_impl.text + else: + # TODO (vshnayder): better exception type + raise Exception("Could not find impl attribute in customtag {0}" + .format(location)) + + params = dict(xmltree.items()) + with system.resources_fs.open('custom_tags/{name}' + .format(name=template_name)) as template: + definition['html'] = Template(template.read()).render(**params) + + return definition