From 438ac3b1a4fdd78329d810a09bd8aa900e5bb760 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 12 Jun 2013 11:56:12 -0400 Subject: [PATCH 1/2] Move video xmodule tests to xmodule directory --- .../lib/xmodule/xmodule}/tests/test_video_xml.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {lms/djangoapps/courseware => common/lib/xmodule/xmodule}/tests/test_video_xml.py (100%) diff --git a/lms/djangoapps/courseware/tests/test_video_xml.py b/common/lib/xmodule/xmodule/tests/test_video_xml.py similarity index 100% rename from lms/djangoapps/courseware/tests/test_video_xml.py rename to common/lib/xmodule/xmodule/tests/test_video_xml.py From 82606a062c83e9782b9325ad203282b1414df5ba Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 12 Jun 2013 16:48:49 -0400 Subject: [PATCH 2/2] Make XModuleDescriptor use __init__ from XBlock Fixes JIRA LMS-203 --- CHANGELOG.rst | 4 + common/lib/xmodule/xmodule/capa_module.py | 7 +- .../xmodule/combined_open_ended_module.py | 15 ++-- common/lib/xmodule/xmodule/error_module.py | 4 +- common/lib/xmodule/xmodule/mako_module.py | 12 +-- .../lib/xmodule/xmodule/modulestore/mongo.py | 29 +++++-- .../xmodule/modulestore/tests/test_mongo.py | 79 ++++++++++++++++- common/lib/xmodule/xmodule/modulestore/xml.py | 2 +- .../combined_open_ended_modulev1.py | 13 +-- .../xmodule/xmodule/peer_grading_module.py | 12 +-- .../xmodule/tests/test_annotatable_module.py | 4 +- .../xmodule/xmodule/tests/test_capa_module.py | 4 +- .../xmodule/tests/test_combined_open_ended.py | 15 ++-- .../xmodule/xmodule/tests/test_conditional.py | 11 ++- .../xmodule/xmodule/tests/test_html_module.py | 7 +- .../lib/xmodule/xmodule/tests/test_logic.py | 6 +- .../xmodule/xmodule/tests/test_mako_module.py | 22 +++++ .../xmodule/xmodule/tests/test_progress.py | 2 +- .../xmodule/xmodule/tests/test_video_xml.py | 4 +- .../xmodule/xmodule/tests/test_xml_module.py | 4 +- common/lib/xmodule/xmodule/x_module.py | 87 ++++++++++--------- common/lib/xmodule/xmodule/xml_module.py | 4 +- lms/djangoapps/courseware/tests/__init__.py | 7 +- .../courseware/tests/test_video_mongo.py | 4 +- lms/djangoapps/open_ended_grading/tests.py | 8 +- 25 files changed, 243 insertions(+), 123 deletions(-) create mode 100644 common/lib/xmodule/xmodule/tests/test_mako_module.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 84e1bb474b..01a132a9b5 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ captions. CMS: Allow editors to delete uploaded files/assets +XModules: `XModuleDescriptor.__init__` and `XModule.__init__` dropped the +`location` parameter (and added it as a field), and renamed `system` to `runtime`, +to accord more closely to `XBlock.__init__` + LMS: Some errors handling Non-ASCII data in XML courses have been fixed. LMS: Add page-load tracking using segment-io (if SEGMENT_IO_LMS_KEY and diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index ca10d4d2a0..fee80a34ff 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -117,6 +117,8 @@ class CapaModule(CapaFields, XModule): ''' An XModule implementing LonCapa format problems, implemented by way of capa.capa_problem.LoncapaProblem + + CapaModule.__init__ takes the same arguments as xmodule.x_module:XModule.__init__ ''' icon_class = 'problem' @@ -131,8 +133,9 @@ class CapaModule(CapaFields, XModule): js_module_name = "Problem" css = {'scss': [resource_string(__name__, 'css/capa/display.scss')]} - def __init__(self, system, location, descriptor, model_data): - XModule.__init__(self, system, location, descriptor, model_data) + def __init__(self, *args, **kwargs): + """ Accepts the same arguments as xmodule.x_module:XModule.__init__ """ + XModule.__init__(self, *args, **kwargs) due_date = self.due diff --git a/common/lib/xmodule/xmodule/combined_open_ended_module.py b/common/lib/xmodule/xmodule/combined_open_ended_module.py index dc753a64b8..ac95567946 100644 --- a/common/lib/xmodule/xmodule/combined_open_ended_module.py +++ b/common/lib/xmodule/xmodule/combined_open_ended_module.py @@ -116,6 +116,8 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule): incorporates multiple children (tasks): openendedmodule selfassessmentmodule + + CombinedOpenEndedModule.__init__ takes the same arguments as xmodule.x_module:XModule.__init__ """ STATE_VERSION = 1 @@ -139,8 +141,7 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule): css = {'scss': [resource_string(__name__, 'css/combinedopenended/display.scss')]} - def __init__(self, system, location, descriptor, model_data): - XModule.__init__(self, system, location, descriptor, model_data) + def __init__(self, *args, **kwargs): """ Definition file should have one or many task blocks, a rubric block, and a prompt block: @@ -175,9 +176,9 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule): """ + XModule.__init__(self, *args, **kwargs) - self.system = system - self.system.set('location', location) + self.system.set('location', self.location) if self.task_states is None: self.task_states = [] @@ -189,13 +190,11 @@ class CombinedOpenEndedModule(CombinedOpenEndedFields, XModule): attributes = self.student_attributes + self.settings_attributes - static_data = { - 'rewrite_content_links': self.rewrite_content_links, - } + static_data = {} instance_state = {k: getattr(self, k) for k in attributes} self.child_descriptor = version_tuple.descriptor(self.system) self.child_definition = version_tuple.descriptor.definition_from_xml(etree.fromstring(self.data), self.system) - self.child_module = version_tuple.module(self.system, location, self.child_definition, self.child_descriptor, + self.child_module = version_tuple.module(self.system, self.location, self.child_definition, self.child_descriptor, instance_state=instance_state, static_data=static_data, attributes=attributes) self.save_instance_data() diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index e998ceb58e..a37081c447 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/common/lib/xmodule/xmodule/error_module.py @@ -94,11 +94,11 @@ class ErrorDescriptor(ErrorFields, JSONEditingDescriptor): model_data = { 'error_msg': str(error_msg), 'contents': contents, - 'display_name': 'Error: ' + location.name + 'display_name': 'Error: ' + location.name, + 'location': location, } return cls( system, - location, model_data, ) diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index 8abb1d7777..526fc1a0eb 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -18,14 +18,16 @@ class MakoModuleDescriptor(XModuleDescriptor): Expects the descriptor to have the `mako_template` attribute set with the name of the template to render, and it will pass the descriptor as the `module` parameter to that template + + MakoModuleDescriptor.__init__ takes the same arguments as xmodule.x_module:XModuleDescriptor.__init__ """ - def __init__(self, system, location, model_data): - if getattr(system, 'render_template', None) is None: - raise TypeError('{system} must have a render_template function' + def __init__(self, *args, **kwargs): + super(MakoModuleDescriptor, self).__init__(*args, **kwargs) + if getattr(self.runtime, 'render_template', None) is None: + raise TypeError('{runtime} must have a render_template function' ' in order to use a MakoDescriptor'.format( - system=system)) - super(MakoModuleDescriptor, self).__init__(system, location, model_data) + runtime=self.runtime)) def get_context(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 87a995cd06..422abbdd73 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -37,15 +37,23 @@ def get_course_id_no_run(location): return "/".join([location.org, location.course]) +class InvalidWriteError(Exception): + """ + Raised to indicate that writing to a particular key + in the KeyValueStore is disabled + """ + + class MongoKeyValueStore(KeyValueStore): """ A KeyValueStore that maps keyed data access to one of the 3 data areas known to the MongoModuleStore (data, children, and metadata) """ - def __init__(self, data, children, metadata): + def __init__(self, data, children, metadata, location): self._data = data self._children = children self._metadata = metadata + self._location = location def get(self, key): if key.scope == Scope.children: @@ -55,7 +63,9 @@ class MongoKeyValueStore(KeyValueStore): elif key.scope == Scope.settings: return self._metadata[key.field_name] elif key.scope == Scope.content: - if key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'location': + return self._location + elif key.field_name == 'data' and not isinstance(self._data, dict): return self._data else: return self._data[key.field_name] @@ -68,7 +78,9 @@ class MongoKeyValueStore(KeyValueStore): elif key.scope == Scope.settings: self._metadata[key.field_name] = value elif key.scope == Scope.content: - if key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'location': + self._location = value + elif key.field_name == 'data' and not isinstance(self._data, dict): self._data = value else: self._data[key.field_name] = value @@ -82,7 +94,9 @@ class MongoKeyValueStore(KeyValueStore): if key.field_name in self._metadata: del self._metadata[key.field_name] elif key.scope == Scope.content: - if key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'location': + self._location = Location(None) + elif key.field_name == 'data' and not isinstance(self._data, dict): self._data = None else: del self._data[key.field_name] @@ -95,7 +109,9 @@ class MongoKeyValueStore(KeyValueStore): elif key.scope == Scope.settings: return key.field_name in self._metadata elif key.scope == Scope.content: - if key.field_name == 'data' and not isinstance(self._data, dict): + if key.field_name == 'location': + return True + elif key.field_name == 'data' and not isinstance(self._data, dict): return True else: return key.field_name in self._data @@ -171,10 +187,11 @@ class CachingDescriptorSystem(MakoDescriptorSystem): definition.get('data', {}), definition.get('children', []), metadata, + location, ) model_data = DbModel(kvs, class_, None, MongoUsage(self.course_id, location)) - module = class_(self, location, model_data) + module = class_(self, model_data) if self.cached_metadata is not None: # parent container pointers don't differentiate between draft and non-draft # so when we do the lookup, we should do so with a non-draft location diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 8a479fbf9e..07e6124537 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -1,11 +1,14 @@ import pymongo from mock import Mock -from nose.tools import assert_equals, assert_raises, assert_not_equals, with_setup, assert_false +from nose.tools import assert_equals, assert_raises, assert_not_equals, assert_false from pprint import pprint +from xblock.core import Scope +from xblock.runtime import KeyValueStore, InvalidScopeError + from xmodule.modulestore import Location -from xmodule.modulestore.mongo import MongoModuleStore +from xmodule.modulestore.mongo import MongoModuleStore, MongoKeyValueStore from xmodule.modulestore.xml_importer import import_from_xml from xmodule.templates import update_templates @@ -114,3 +117,75 @@ class TestMongoModuleStore(object): course.location.org == 'edx' and course.location.course == 'templates', '{0} is a template course'.format(course) ) + +class TestMongoKeyValueStore(object): + + def setUp(self): + self.data = {'foo': 'foo_value'} + self.location = Location('i4x://org/course/category/name@version') + self.children = ['i4x://org/course/child/a', 'i4x://org/course/child/b'] + self.metadata = {'meta': 'meta_val'} + self.kvs = MongoKeyValueStore(self.data, self.children, self.metadata, self.location) + + def _check_read(self, key, expected_value): + assert_equals(expected_value, self.kvs.get(key)) + assert self.kvs.has(key) + + def test_read(self): + assert_equals(self.data['foo'], self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'foo'))) + assert_equals(self.location, self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'location'))) + assert_equals(self.children, self.kvs.get(KeyValueStore.Key(Scope.children, None, None, 'children'))) + assert_equals(self.metadata['meta'], self.kvs.get(KeyValueStore.Key(Scope.settings, None, None, 'meta'))) + assert_equals(None, self.kvs.get(KeyValueStore.Key(Scope.parent, None, None, 'parent'))) + + def test_read_invalid_scope(self): + for scope in (Scope.preferences, Scope.user_info, Scope.user_state): + key = KeyValueStore.Key(scope, None, None, 'foo') + with assert_raises(InvalidScopeError): + self.kvs.get(key) + assert_false(self.kvs.has(key)) + + def test_read_non_dict_data(self): + self.kvs._data = 'xml_data' + assert_equals('xml_data', self.kvs.get(KeyValueStore.Key(Scope.content, None, None, 'data'))) + + def _check_write(self, key, value): + self.kvs.set(key, value) + assert_equals(value, self.kvs.get(key)) + + def test_write(self): + yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'foo'), 'new_data') + yield (self._check_write, KeyValueStore.Key(Scope.content, None, None, 'location'), Location('i4x://org/course/category/name@new_version')) + yield (self._check_write, KeyValueStore.Key(Scope.children, None, None, 'children'), []) + yield (self._check_write, KeyValueStore.Key(Scope.settings, None, None, 'meta'), 'new_settings') + + def test_write_non_dict_data(self): + self.kvs._data = 'xml_data' + self._check_write(KeyValueStore.Key(Scope.content, None, None, 'data'), 'new_data') + + def test_write_invalid_scope(self): + for scope in (Scope.preferences, Scope.user_info, Scope.user_state, Scope.parent): + with assert_raises(InvalidScopeError): + self.kvs.set(KeyValueStore.Key(scope, None, None, 'foo'), 'new_value') + + def _check_delete_default(self, key, default_value): + self.kvs.delete(key) + assert_equals(default_value, self.kvs.get(key)) + assert self.kvs.has(key) + + def _check_delete_key_error(self, key): + self.kvs.delete(key) + with assert_raises(KeyError): + self.kvs.get(key) + assert_false(self.kvs.has(key)) + + def test_delete(self): + yield (self._check_delete_key_error, KeyValueStore.Key(Scope.content, None, None, 'foo')) + yield (self._check_delete_default, KeyValueStore.Key(Scope.content, None, None, 'location'), Location(None)) + yield (self._check_delete_default, KeyValueStore.Key(Scope.children, None, None, 'children'), []) + yield (self._check_delete_key_error, KeyValueStore.Key(Scope.settings, None, None, 'meta')) + + def test_delete_invalid_scope(self): + for scope in (Scope.preferences, Scope.user_info, Scope.user_state, Scope.parent): + with assert_raises(InvalidScopeError): + self.kvs.delete(KeyValueStore.Key(scope, None, None, 'foo')) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 6ab6843216..a704fc2ae8 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -463,7 +463,7 @@ class XMLModuleStore(ModuleStoreBase): # tabs are referenced in policy.json through a 'slug' which is just the filename without the .html suffix slug = os.path.splitext(os.path.basename(filepath))[0] loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, category, slug) - module = HtmlDescriptor(system, loc, {'data': html}) + module = HtmlDescriptor(system, {'data': html, 'location': loc}) # VS[compat]: # Hack because we need to pull in the 'display_name' for static tabs (because we need to edit them) # from the course policy diff --git a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py index b48c70c8d5..45e73442d0 100644 --- a/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py +++ b/common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py @@ -117,7 +117,6 @@ class CombinedOpenEndedV1Module(): self.instance_state = instance_state self.display_name = instance_state.get('display_name', "Open Ended") - self.rewrite_content_links = static_data.get('rewrite_content_links', "") #We need to set the location here so the child modules can use it system.set('location', location) @@ -354,17 +353,7 @@ class CombinedOpenEndedV1Module(): Output: Child task HTML """ self.update_task_states() - html = self.current_task.get_html(self.system) - return_html = html - try: - #Without try except block, get this error: - # File "/home/vik/mitx_all/mitx/common/lib/xmodule/xmodule/x_module.py", line 263, in rewrite_content_links - # if link.startswith(XASSET_SRCREF_PREFIX): - # Placing try except so that if the error is fixed, this code will start working again. - return_html = rewrite_links(html, self.rewrite_content_links) - except Exception: - pass - return return_html + return self.current_task.get_html(self.system) def get_current_attributes(self, task_number): """ diff --git a/common/lib/xmodule/xmodule/peer_grading_module.py b/common/lib/xmodule/xmodule/peer_grading_module.py index 979bc95cb0..a13fef8e40 100644 --- a/common/lib/xmodule/xmodule/peer_grading_module.py +++ b/common/lib/xmodule/xmodule/peer_grading_module.py @@ -62,6 +62,9 @@ class PeerGradingFields(object): class PeerGradingModule(PeerGradingFields, XModule): + """ + PeerGradingModule.__init__ takes the same arguments as xmodule.x_module:XModule.__init__ + """ _VERSION = 1 js = {'coffee': [resource_string(__name__, 'js/src/peergrading/peer_grading.coffee'), @@ -73,12 +76,11 @@ class PeerGradingModule(PeerGradingFields, XModule): css = {'scss': [resource_string(__name__, 'css/combinedopenended/display.scss')]} - def __init__(self, system, location, descriptor, model_data): - XModule.__init__(self, system, location, descriptor, model_data) + def __init__(self, *args, **kwargs): + super(PeerGradingModule, self).__init__(*args, **kwargs) - # We need to set the location here so the child modules can use it - system.set('location', location) - self.system = system + #We need to set the location here so the child modules can use it + self.runtime.set('location', self.location) if (self.system.open_ended_grading_interface): self.peer_gs = PeerGradingService(self.system.open_ended_grading_interface, self.system) else: diff --git a/common/lib/xmodule/xmodule/tests/test_annotatable_module.py b/common/lib/xmodule/xmodule/tests/test_annotatable_module.py index 43eae8e43e..268f743421 100644 --- a/common/lib/xmodule/xmodule/tests/test_annotatable_module.py +++ b/common/lib/xmodule/xmodule/tests/test_annotatable_module.py @@ -29,10 +29,10 @@ class AnnotatableModuleTestCase(unittest.TestCase): ''' descriptor = Mock() - module_data = {'data': sample_xml} + module_data = {'data': sample_xml, 'location': location} def setUp(self): - self.annotatable = AnnotatableModule(test_system(), self.location, self.descriptor, self.module_data) + self.annotatable = AnnotatableModule(test_system(), self.descriptor, self.module_data) def test_annotation_data_attr(self): el = etree.fromstring('test') diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 3153250527..7cba4a76b3 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -86,7 +86,7 @@ class CapaFactory(object): """ location = Location(["i4x", "edX", "capa_test", "problem", "SampleProblem{0}".format(CapaFactory.next_num())]) - model_data = {'data': CapaFactory.sample_problem_xml} + model_data = {'data': CapaFactory.sample_problem_xml, 'location': location} if graceperiod is not None: model_data['graceperiod'] = graceperiod @@ -113,7 +113,7 @@ class CapaFactory(object): system = test_system() system.render_template = Mock(return_value="
Test Template HTML
") - module = CapaModule(system, location, descriptor, model_data) + module = CapaModule(system, descriptor, model_data) if correct: # TODO: probably better to actually set the internal state properly, but... diff --git a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py index 409347882f..7dbaf6fe3d 100644 --- a/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py +++ b/common/lib/xmodule/xmodule/tests/test_combined_open_ended.py @@ -175,7 +175,6 @@ class OpenEndedModuleTest(unittest.TestCase): 'max_score': max_score, 'display_name': 'Name', 'accept_file_upload': False, - 'rewrite_content_links': "", 'close_date': None, 's3_interface': test_util_open_ended.S3_INTERFACE, 'open_ended_grading_interface': test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE, @@ -332,7 +331,6 @@ class CombinedOpenEndedModuleTest(unittest.TestCase): 'max_score': max_score, 'display_name': 'Name', 'accept_file_upload': False, - 'rewrite_content_links': "", 'close_date': "", 's3_interface': test_util_open_ended.S3_INTERFACE, 'open_ended_grading_interface': test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE, @@ -370,10 +368,15 @@ class CombinedOpenEndedModuleTest(unittest.TestCase): full_definition = definition_template.format(prompt=prompt, rubric=rubric, task1=task_xml1, task2=task_xml2) descriptor = Mock(data=full_definition) test_system = test_system() - combinedoe_container = CombinedOpenEndedModule(test_system, - location, - descriptor, - model_data={'data': full_definition, 'weight': '1'}) + combinedoe_container = CombinedOpenEndedModule( + test_system, + descriptor, + model_data={ + 'data': full_definition, + 'weight': '1', + 'location': location + } + ) def setUp(self): # TODO: this constructor call is definitely wrong, but neither branch diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index 320b94efb7..e88bf0c588 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -60,9 +60,9 @@ class ConditionalFactory(object): source_location = Location(["i4x", "edX", "conditional_test", "problem", "SampleProblem"]) if source_is_error_module: # Make an error descriptor and module - source_descriptor = NonStaffErrorDescriptor.from_xml('some random xml data', + source_descriptor = NonStaffErrorDescriptor.from_xml('some random xml data', system, - org=source_location.org, + org=source_location.org, course=source_location.course, error_msg='random error message') source_module = source_descriptor.xmodule(system) @@ -87,8 +87,8 @@ class ConditionalFactory(object): # construct conditional module: cond_location = Location(["i4x", "edX", "conditional_test", "conditional", "SampleConditional"]) - model_data = {'data': ''} - cond_module = ConditionalModule(system, cond_location, cond_descriptor, model_data) + model_data = {'data': '', 'location': cond_location} + cond_module = ConditionalModule(system, cond_descriptor, model_data) # return dict: return {'cond_module': cond_module, @@ -106,7 +106,7 @@ class ConditionalModuleBasicTest(unittest.TestCase): self.test_system = test_system() def test_icon_class(self): - '''verify that get_icon_class works independent of condition satisfaction''' + '''verify that get_icon_class works independent of condition satisfaction''' modules = ConditionalFactory.create(self.test_system) for attempted in ["false", "true"]: for icon_class in [ 'other', 'problem', 'video']: @@ -186,7 +186,6 @@ class ConditionalModuleXmlTest(unittest.TestCase): if isinstance(descriptor, Location): location = descriptor descriptor = self.modulestore.get_instance(course.id, location, depth=None) - location = descriptor.location return descriptor.xmodule(self.test_system) # edx - HarvardX diff --git a/common/lib/xmodule/xmodule/tests/test_html_module.py b/common/lib/xmodule/xmodule/tests/test_html_module.py index e56e9babe7..ea6b358f3b 100644 --- a/common/lib/xmodule/xmodule/tests/test_html_module.py +++ b/common/lib/xmodule/xmodule/tests/test_html_module.py @@ -8,14 +8,13 @@ from xmodule.modulestore import Location from . import test_system class HtmlModuleSubstitutionTestCase(unittest.TestCase): - location = Location(["i4x", "edX", "toy", "html", "simple_html"]) descriptor = Mock() def test_substitution_works(self): sample_xml = '''%%USER_ID%%''' module_data = {'data': sample_xml} module_system = test_system() - module = HtmlModule(module_system, self.location, self.descriptor, module_data) + module = HtmlModule(module_system, self.descriptor, module_data) self.assertEqual(module.get_html(), str(module_system.anonymous_student_id)) @@ -26,7 +25,7 @@ class HtmlModuleSubstitutionTestCase(unittest.TestCase): ''' module_data = {'data': sample_xml} - module = HtmlModule(test_system(), self.location, self.descriptor, module_data) + module = HtmlModule(test_system(), self.descriptor, module_data) self.assertEqual(module.get_html(), sample_xml) @@ -35,6 +34,6 @@ class HtmlModuleSubstitutionTestCase(unittest.TestCase): module_data = {'data': sample_xml} module_system = test_system() module_system.anonymous_student_id = None - module = HtmlModule(module_system, self.location, self.descriptor, module_data) + module = HtmlModule(module_system, self.descriptor, module_data) self.assertEqual(module.get_html(), sample_xml) diff --git a/common/lib/xmodule/xmodule/tests/test_logic.py b/common/lib/xmodule/xmodule/tests/test_logic.py index e60af63921..ff17f88dfc 100644 --- a/common/lib/xmodule/xmodule/tests/test_logic.py +++ b/common/lib/xmodule/xmodule/tests/test_logic.py @@ -30,13 +30,13 @@ class LogicTest(unittest.TestCase): pass self.system = None - self.location = None self.descriptor = EmptyClass() self.xmodule_class = self.descriptor_class.module_class self.xmodule = self.xmodule_class( - self.system, self.location, - self.descriptor, self.raw_model_data + self.system, + self.descriptor, + self.raw_model_data ) def ajax_request(self, dispatch, get): diff --git a/common/lib/xmodule/xmodule/tests/test_mako_module.py b/common/lib/xmodule/xmodule/tests/test_mako_module.py new file mode 100644 index 0000000000..7ba023bda7 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/test_mako_module.py @@ -0,0 +1,22 @@ +""" Test mako_module.py """ + +from unittest import TestCase +from mock import Mock + +from xmodule.mako_module import MakoModuleDescriptor + + +class MakoModuleTest(TestCase): + """ Test MakoModuleDescriptor """ + + def test_render_template_check(self): + mock_system = Mock() + mock_system.render_template = None + + with self.assertRaises(TypeError): + MakoModuleDescriptor(mock_system, {}) + + del mock_system.render_template + + with self.assertRaises(TypeError): + MakoModuleDescriptor(mock_system, {}) diff --git a/common/lib/xmodule/xmodule/tests/test_progress.py b/common/lib/xmodule/xmodule/tests/test_progress.py index 4bb663ad85..97ec00b13e 100644 --- a/common/lib/xmodule/xmodule/tests/test_progress.py +++ b/common/lib/xmodule/xmodule/tests/test_progress.py @@ -134,6 +134,6 @@ class ModuleProgressTest(unittest.TestCase): ''' def test_xmodule_default(self): '''Make sure default get_progress exists, returns None''' - xm = x_module.XModule(test_system(), 'a://b/c/d/e', None, {}) + xm = x_module.XModule(test_system(), None, {'location': 'a://b/c/d/e'}) p = xm.get_progress() self.assertEqual(p, None) diff --git a/common/lib/xmodule/xmodule/tests/test_video_xml.py b/common/lib/xmodule/xmodule/tests/test_video_xml.py index c199a0aee1..fae1580323 100644 --- a/common/lib/xmodule/xmodule/tests/test_video_xml.py +++ b/common/lib/xmodule/xmodule/tests/test_video_xml.py @@ -47,13 +47,13 @@ class VideoFactory(object): """Method return Video Xmodule instance.""" location = Location(["i4x", "edX", "video", "default", "SampleProblem1"]) - model_data = {'data': VideoFactory.sample_problem_xml_youtube} + model_data = {'data': VideoFactory.sample_problem_xml_youtube, 'location': location} descriptor = Mock(weight="1") system = test_system() system.render_template = lambda template, context: context - module = VideoModule(system, location, descriptor, model_data) + module = VideoModule(system, descriptor, model_data) return module diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index eb715b44bc..46410def8e 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py @@ -142,7 +142,7 @@ class EditableMetadataFieldsTest(unittest.TestCase): def get_xml_editable_fields(self, model_data): system = test_system() system.render_template = Mock(return_value="
Test Template HTML
") - return XmlDescriptor(system=system, location=None, model_data=model_data).editable_metadata_fields + return XmlDescriptor(runtime=system, model_data=model_data).editable_metadata_fields def get_descriptor(self, model_data): class TestModuleDescriptor(TestFields, XmlDescriptor): @@ -154,7 +154,7 @@ class EditableMetadataFieldsTest(unittest.TestCase): system = test_system() system.render_template = Mock(return_value="
Test Template HTML
") - return TestModuleDescriptor(system=system, location=None, model_data=model_data) + return TestModuleDescriptor(runtime=system, model_data=model_data) def assert_field_values(self, editable_fields, name, field, explicitly_set, inheritable, value, default_value, type='Generic', options=[]): diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index a1813a795a..3edc22df43 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -10,7 +10,7 @@ from pkg_resources import resource_listdir, resource_string, resource_isdir from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError -from xblock.core import XBlock, Scope, String, Integer, Float +from xblock.core import XBlock, Scope, String, Integer, Float, ModelType log = logging.getLogger(__name__) @@ -19,6 +19,23 @@ def dummy_track(event_type, event): pass +class LocationField(ModelType): + """ + XBlock field for storing Location values + """ + def from_json(self, value): + """ + Parse the json value as a Location + """ + return Location(value) + + def to_json(self, value): + """ + Store the Location as a url string in json + """ + return value.url() + + class HTMLSnippet(object): """ A base class defining an interface for an object that is able to present an @@ -87,6 +104,16 @@ class XModuleFields(object): default=None ) + # Please note that in order to be compatible with XBlocks more generally, + # the LMS and CMS shouldn't be using this field. It's only for internal + # consumption by the XModules themselves + location = LocationField( + display_name="Location", + help="This is the location id for the XModule.", + scope=Scope.content, + default=Location(None), + ) + class XModule(XModuleFields, HTMLSnippet, XBlock): ''' Implements a generic learning module. @@ -106,24 +133,20 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): icon_class = 'other' - def __init__(self, system, location, descriptor, model_data): + def __init__(self, runtime, descriptor, model_data): ''' Construct a new xmodule - system: A ModuleSystem allowing access to external resources - - location: Something Location-like that identifies this xmodule + runtime: An XBlock runtime allowing access to external resources descriptor: the XModuleDescriptor that this module is an instance of. - TODO (vshnayder): remove the definition parameter and location--they - can come from the descriptor. model_data: A dictionary-like object that maps field names to values for those fields. ''' + super(XModule, self).__init__(runtime, model_data) self._model_data = model_data - self.system = system - self.location = Location(location) + self.system = runtime self.descriptor = descriptor self.url_name = self.location.name self.category = self.location.category @@ -254,19 +277,6 @@ class XModule(XModuleFields, HTMLSnippet, XBlock): get is a dictionary-like object ''' return "" - # cdodge: added to support dynamic substitutions of - # links for courseware assets (e.g. images). is passed through from lxml.html parser - def rewrite_content_links(self, link): - # see if we start with our format, e.g. 'xasset:' - if link.startswith(XASSET_SRCREF_PREFIX): - # yes, then parse out the name - name = link[len(XASSET_SRCREF_PREFIX):] - loc = Location(self.location) - # resolve the reference to our internal 'filepath' which - link = StaticContent.compute_location_filename(loc.org, loc.course, name) - - return link - def policy_key(location): """ @@ -340,13 +350,12 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): template_dir_name = "default" # Class level variable + + # True if this descriptor always requires recalculation of grades, for + # example if the score can change via an extrnal service, not just when the + # student interacts with the module on the page. A specific example is + # FoldIt, which posts grade-changing updates through a separate API. always_recalculate_grades = False - """ - Return whether this descriptor always requires recalculation of grades, for - example if the score can change via an extrnal service, not just when the - student interacts with the module on the page. A specific example is - FoldIt, which posts grade-changing updates through a separate API. - """ # VS[compat]. Backwards compatibility code that can go away after # importing 2012 courses. @@ -357,10 +366,7 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): } # ============================= STRUCTURAL MANIPULATION =================== - def __init__(self, - system, - location, - model_data): + def __init__(self, *args, **kwargs): """ Construct a new XModuleDescriptor. The only required arguments are the system, used for interaction with external resources, and the @@ -371,19 +377,17 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): This allows for maximal flexibility to add to the interface while preserving backwards compatibility. - system: A DescriptorSystem for interacting with external resources - - location: Something Location-like that identifies this xmodule + runtime: A DescriptorSystem for interacting with external resources model_data: A dictionary-like object that maps field names to values for those fields. + + XModuleDescriptor.__init__ takes the same arguments as xblock.core:XBlock.__init__ """ - self.system = system - self.location = Location(location) + super(XModuleDescriptor, self).__init__(*args, **kwargs) + self.system = self.runtime self.url_name = self.location.name self.category = self.location.category - self._model_data = model_data - self._child_instances = None @property @@ -441,7 +445,6 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): """ return self.module_class( system, - self.location, self, system.xblock_model_data(self), ) @@ -510,7 +513,9 @@ class XModuleDescriptor(XModuleFields, HTMLSnippet, ResourceTemplates, XBlock): else: model_data['data'] = definition['data'] - return cls(system=system, location=json_data['location'], model_data=model_data) + model_data['location'] = json_data['location'] + + return cls(system, model_data) @classmethod def _translate(cls, key): diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 9b5de9d7e7..e1a0e0cf08 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -254,7 +254,7 @@ class XmlDescriptor(XModuleDescriptor): definition, children = cls.definition_from_xml(definition_xml, system) if definition_metadata: definition['definition_metadata'] = definition_metadata - definition['filename'] = [ filepath, filename ] + definition['filename'] = [ filepath, filename ] return definition, children @@ -352,10 +352,10 @@ class XmlDescriptor(XModuleDescriptor): for key, value in metadata.items(): if key not in set(f.name for f in cls.fields + cls.lms.fields): model_data['xml_attributes'][key] = value + model_data['location'] = location return cls( system, - location, model_data, ) diff --git a/lms/djangoapps/courseware/tests/__init__.py b/lms/djangoapps/courseware/tests/__init__.py index dc61239c36..cc53bf735a 100644 --- a/lms/djangoapps/courseware/tests/__init__.py +++ b/lms/djangoapps/courseware/tests/__init__.py @@ -77,14 +77,15 @@ class BaseTestXmodule(ModuleStoreTestCase): data=self.DATA ) - location = self.item_descriptor.location system = test_system() system.render_template = lambda template, context: context + model_data = {'location': self.item_descriptor.location} + model_data.update(self.MODEL_DATA) self.item_module = self.item_descriptor.module_class( - system, location, self.item_descriptor, self.MODEL_DATA + system, self.item_descriptor, model_data ) - self.item_url = Location(location).url() + self.item_url = Location(self.item_module.location).url() # login all users for acces to Xmodule self.clients = {user.username: Client() for user in self.users} diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index c041ccc151..a0fdecc77a 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -15,8 +15,8 @@ class TestVideo(BaseTestXmodule): user.username: self.clients[user.username].post( self.get_url('whatever'), {}, - HTTP_X_REQUESTED_WITH='XMLHttpRequest') - for user in self.users + HTTP_X_REQUESTED_WITH='XMLHttpRequest' + ) for user in self.users } self.assertEqual( diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 13d780df12..3b6c992881 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -160,7 +160,7 @@ class TestPeerGradingService(LoginEnrollmentTestCase): self.course_id = "edX/toy/2012_Fall" self.toy = modulestore().get_course(self.course_id) location = "i4x://edX/toy/peergrading/init" - model_data = {'data': ""} + model_data = {'data': "", 'location': location} self.mock_service = peer_grading_service.MockPeerGradingService() self.system = ModuleSystem( ajax_url=location, @@ -172,9 +172,9 @@ class TestPeerGradingService(LoginEnrollmentTestCase): s3_interface=test_util_open_ended.S3_INTERFACE, open_ended_grading_interface=test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE ) - self.descriptor = peer_grading_module.PeerGradingDescriptor(self.system, location, model_data) - model_data = {} - self.peer_module = peer_grading_module.PeerGradingModule(self.system, location, self.descriptor, model_data) + self.descriptor = peer_grading_module.PeerGradingDescriptor(self.system, model_data) + model_data = {'location': location} + self.peer_module = peer_grading_module.PeerGradingModule(self.system, self.descriptor, model_data) self.peer_module.peer_gs = self.mock_service self.logout()