diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index bd392562fe..b0f9d4d239 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -609,6 +609,10 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes Called from Studio view. """ + user_service = self.runtime.service(self, 'user') + if user_service is None: + return Response() + user_partition = self.get_selected_partition() changed = False diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 3fba95b2ab..6b6b6ddbd4 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -92,16 +92,36 @@ def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')): where `my_render_func` is a function of the form my_render_func(template, context). """ - user = Mock(is_staff=False) + user = Mock(name='get_test_system.user', is_staff=False) + + descriptor_system = get_test_descriptor_system(), + + def get_module(descriptor): + """Mocks module_system get_module function""" + # pylint: disable=protected-access + + # Unlike XBlock Runtimes or DescriptorSystems, + # each XModule is provided with a new ModuleSystem. + # Construct one for the new XModule. + module_system = get_test_system() + + # Descriptors can all share a single DescriptorSystem. + # So, bind to the same one as the current descriptor. + module_system.descriptor_runtime = descriptor.runtime._descriptor_system + + descriptor.bind_for_student(module_system, descriptor._field_data) + + return descriptor + return TestModuleSystem( static_url='/static', - track_function=Mock(), - get_module=Mock(), + track_function=Mock(name='get_test_system.track_function'), + get_module=get_module, render_template=mock_render_template, replace_urls=str, user=user, get_real_user=lambda(__): user, - filestore=Mock(), + filestore=Mock(name='get_test_system.filestore'), debug=True, hostname="edx.org", xqueue={ @@ -109,16 +129,16 @@ def get_test_system(course_id=SlashSeparatedCourseKey('org', 'course', 'run')): 'callback_url': '/', 'default_queuename': 'testqueue', 'waittime': 10, - 'construct_callback': Mock(side_effect="/"), + 'construct_callback': Mock(name='get_test_system.xqueue.construct_callback', side_effect="/"), }, node_path=os.environ.get("NODE_PATH", "/usr/local/lib/node_modules"), anonymous_student_id='student', open_ended_grading_interface=open_ended_grading_interface, course_id=course_id, error_descriptor_class=ErrorDescriptor, - get_user_role=Mock(is_staff=False), - descriptor_runtime=get_test_descriptor_system(), - user_location=Mock(), + get_user_role=Mock(name='get_test_system.get_user_role', is_staff=False), + user_location=Mock(name='get_test_system.user_location'), + descriptor_runtime=descriptor_system, ) @@ -128,15 +148,17 @@ def get_test_descriptor_system(): """ field_data = DictFieldData({}) - return MakoDescriptorSystem( - load_item=Mock(), - resources_fs=Mock(), - error_tracker=Mock(), + descriptor_system = MakoDescriptorSystem( + load_item=Mock(name='get_test_descriptor_system.load_item'), + resources_fs=Mock(name='get_test_descriptor_system.resources_fs'), + error_tracker=Mock(name='get_test_descriptor_system.error_tracker'), render_template=mock_render_template, mixins=(InheritanceMixin, XModuleMixin), field_data=field_data, services={'field-data': field_data}, ) + descriptor_system.get_asides = lambda block: [] + return descriptor_system def mock_render_template(*args, **kwargs): diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index 373388a7ac..effdf2784a 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/common/lib/xmodule/xmodule/tests/test_conditional.py @@ -64,14 +64,14 @@ class ConditionalFactory(object): error_msg='random error message' ) else: - source_descriptor = Mock() + source_descriptor = Mock(name='source_descriptor') source_descriptor.location = source_location source_descriptor.runtime = descriptor_system source_descriptor.render = lambda view, context=None: descriptor_system.render(source_descriptor, view, context) # construct other descriptors: - child_descriptor = Mock() + child_descriptor = Mock(name='child_descriptor') child_descriptor._xmodule.student_view.return_value.content = u'

This is a secret

' child_descriptor.student_view = child_descriptor._xmodule.student_view child_descriptor.displayable_items.return_value = [child_descriptor] @@ -85,6 +85,8 @@ class ConditionalFactory(object): source_location: source_descriptor }.get + system.descriptor_runtime = descriptor_system + # construct conditional module: cond_location = Location("edX", "conditional_test", "test_run", "conditional", "SampleConditional", None) field_data = DictFieldData({ diff --git a/common/lib/xmodule/xmodule/tests/test_split_test_module.py b/common/lib/xmodule/xmodule/tests/test_split_test_module.py index 36755a0308..700145c8d6 100644 --- a/common/lib/xmodule/xmodule/tests/test_split_test_module.py +++ b/common/lib/xmodule/xmodule/tests/test_split_test_module.py @@ -47,15 +47,7 @@ class SplitTestModuleTest(XModuleXmlImportTest, PartitionTestCase): self.course_sequence = self.course.get_children()[0] self.module_system = get_test_system() - def get_module(descriptor): - """Mocks module_system get_module function""" - module_system = get_test_system() - module_system.get_module = get_module - descriptor.bind_for_student(module_system, descriptor._field_data) # pylint: disable=protected-access - return descriptor - - self.module_system.get_module = get_module - self.module_system.descriptor_system = self.course.runtime + self.module_system.descriptor_runtime = self.course.runtime._descriptor_system # pylint: disable=protected-access self.course.runtime.export_fs = MemoryFS() self.partitions_service = StaticPartitionService( @@ -97,14 +89,12 @@ class SplitTestModuleLMSTest(SplitTestModuleTest): self.module_system.render(self.split_test_module, STUDENT_VIEW).content ) - @ddt.data((0,), (1,)) - @ddt.unpack + @ddt.data(0, 1) def test_child_missing_tag_value(self, _user_tag): # If user_tag has a missing value, we should still get back a valid child url self.assertIn(self.split_test_module.child_descriptor.url_name, ['split_test_cond0', 'split_test_cond1']) - @ddt.data((100,), (200,), (300,), (400,), (500,), (600,), (700,), (800,), (900,), (1000,)) - @ddt.unpack + @ddt.data(100, 200, 300, 400, 500, 600, 700, 800, 900, 1000) def test_child_persist_new_tag_value_when_tag_missing(self, _user_tag): # If a user_tag has a missing value, a group should be saved/persisted for that user. # So, we check that we get the same url_name when we call on the url_name twice. diff --git a/common/lib/xmodule/xmodule/tests/test_vertical.py b/common/lib/xmodule/xmodule/tests/test_vertical.py index e951c13fb7..e515ed2c03 100644 --- a/common/lib/xmodule/xmodule/tests/test_vertical.py +++ b/common/lib/xmodule/xmodule/tests/test_vertical.py @@ -27,15 +27,7 @@ class BaseVerticalModuleTest(XModuleXmlImportTest): course_seq = self.course.get_children()[0] self.module_system = get_test_system() - def get_module(descriptor): - """Mocks module_system get_module function""" - module_system = get_test_system() - module_system.get_module = get_module - descriptor.bind_for_student(module_system, descriptor._field_data) # pylint: disable=protected-access - return descriptor - - self.module_system.get_module = get_module - self.module_system.descriptor_system = self.course.runtime + self.module_system.descriptor_runtime = self.course.runtime._descriptor_system # pylint: disable=protected-access self.course.runtime.export_fs = MemoryFS() self.vertical = course_seq.get_children()[0] diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index a4943fc83d..88f87f80d3 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -286,15 +286,7 @@ class XModuleMixin(XBlockMixin): @property def runtime(self): - # Handle XModule backwards compatibility. If this is a pure - # XBlock, and it has an xmodule_runtime defined, then we're in - # an XModule context, not an XModuleDescriptor context, - # so we should use the xmodule_runtime (ModuleSystem) as the runtime. - if (not isinstance(self, (XModule, XModuleDescriptor)) and - self.xmodule_runtime is not None): - return PureSystem(self.xmodule_runtime, self._runtime) - else: - return self._runtime + return CombinedSystem(self.xmodule_runtime, self._runtime) @runtime.setter def runtime(self, value): @@ -424,6 +416,9 @@ class XModuleMixin(XBlockMixin): for child_loc in self.children: try: child = self.runtime.get_block(child_loc) + if child is None: + continue + child.runtime.export_fs = self.runtime.export_fs except ItemNotFoundError: log.warning(u'Unable to load item {loc}, skipping'.format(loc=child_loc)) @@ -1273,39 +1268,22 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # p result['default_value'] = field.to_json(field.default) return result - def render(self, block, view_name, context=None): - if view_name in PREVIEW_VIEWS: - assert block.xmodule_runtime is not None - if isinstance(block, (XModule, XModuleDescriptor)): - to_render = block._xmodule - else: - to_render = block - return block.xmodule_runtime.render(to_render, view_name, context) - else: - return super(DescriptorSystem, self).render(block, view_name, context) - def handler_url(self, block, handler_name, suffix='', query='', thirdparty=False): - if block.xmodule_runtime is not None: - return block.xmodule_runtime.handler_url(block, handler_name, suffix, query, thirdparty) - else: - # Currently, Modulestore is responsible for instantiating DescriptorSystems - # This means that LMS/CMS don't have a way to define a subclass of DescriptorSystem - # that implements the correct handler url. So, for now, instead, we will reference a - # global function that the application can override. - return descriptor_global_handler_url(block, handler_name, suffix, query, thirdparty) + # Currently, Modulestore is responsible for instantiating DescriptorSystems + # This means that LMS/CMS don't have a way to define a subclass of DescriptorSystem + # that implements the correct handler url. So, for now, instead, we will reference a + # global function that the application can override. + return descriptor_global_handler_url(block, handler_name, suffix, query, thirdparty) def local_resource_url(self, block, uri): """ See :meth:`xblock.runtime.Runtime:local_resource_url` for documentation. """ - if block.xmodule_runtime is not None: - return block.xmodule_runtime.local_resource_url(block, uri) - else: - # Currently, Modulestore is responsible for instantiating DescriptorSystems - # This means that LMS/CMS don't have a way to define a subclass of DescriptorSystem - # that implements the correct local_resource_url. So, for now, instead, we will reference a - # global function that the application can override. - return descriptor_global_local_resource_url(block, uri) + # Currently, Modulestore is responsible for instantiating DescriptorSystems + # This means that LMS/CMS don't have a way to define a subclass of DescriptorSystem + # that implements the correct local_resource_url. So, for now, instead, we will reference a + # global function that the application can override. + return descriptor_global_local_resource_url(block, uri) def applicable_aside_types(self, block): """ @@ -1323,18 +1301,15 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # p """ raise NotImplementedError("edX Platform doesn't currently implement XBlock resource urls") - def publish(self, block, event_type, event): - """ - See :meth:`xblock.runtime.Runtime:publish` for documentation. - """ - if block.xmodule_runtime is not None: - return block.xmodule_runtime.publish(block, event_type, event) - def add_block_as_child_node(self, block, node): child = etree.SubElement(node, "unknown") child.set('url_name', block.url_name) block.add_xml_to_node(child) + def publish(self, block, event_type, event): + # A stub publish method that doesn't emit any events from XModuleDescriptors. + pass + class XMLParsingSystem(DescriptorSystem): def __init__(self, process_xml, **kwargs): @@ -1585,9 +1560,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # pylin """provide uniform access to attributes (like etree)""" self.__dict__[attr] = val - def __repr__(self): - return repr(self.__dict__) - def __str__(self): return str(self.__dict__) @@ -1609,11 +1581,15 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # pylin pass -class PureSystem(ModuleSystem, DescriptorSystem): +class CombinedSystem(object): """ - A subclass of both ModuleSystem and DescriptorSystem to provide pure (non-XModule) XBlocks - a single Runtime interface for both the ModuleSystem and DescriptorSystem, when available. + This class is a shim to allow both pure XBlocks and XModuleDescriptors + that have been bound as XModules to access both the attributes of ModuleSystem + and of DescriptorSystem as a single runtime. """ + + __slots__ = ('_module_system', '_descriptor_system') + # This system doesn't override a number of methods that are provided by ModuleSystem and DescriptorSystem, # namely handler_url, local_resource_url, query, and resource_url. # @@ -1621,12 +1597,74 @@ class PureSystem(ModuleSystem, DescriptorSystem): # # pylint: disable=abstract-method def __init__(self, module_system, descriptor_system): - # N.B. This doesn't call super(PureSystem, self).__init__, because it is only inheriting from - # ModuleSystem and DescriptorSystem to pass isinstance checks. - # pylint: disable=super-init-not-called + # These attributes are set directly to __dict__ below to avoid a recursion in getattr/setattr. self._module_system = module_system self._descriptor_system = descriptor_system + def _get_student_block(self, block): + """ + If block is an XModuleDescriptor that has been bound to a student, return + the corresponding XModule, instead of the XModuleDescriptor. + + Otherwise, return block. + """ + if isinstance(block, XModuleDescriptor) and block.xmodule_runtime: + return block._xmodule # pylint: disable=protected-access + else: + return block + + def render(self, block, view_name, context=None): + """ + Render a block by invoking its view. + + Finds the view named `view_name` on `block`. The default view will be + used if a specific view hasn't be registered. If there is no default + view, an exception will be raised. + + The view is invoked, passing it `context`. The value returned by the + view is returned, with possible modifications by the runtime to + integrate it into a larger whole. + + """ + if view_name in PREVIEW_VIEWS: + block = self._get_student_block(block) + + return self.__getattr__('render')(block, view_name, context) + + def service(self, block, service_name): + """Return a service, or None. + + Services are objects implementing arbitrary other interfaces. They are + requested by agreed-upon names, see [XXX TODO] for a list of possible + services. The object returned depends on the service requested. + + XBlocks must announce their intention to request services with the + `XBlock.needs` or `XBlock.wants` decorators. Use `needs` if you assume + that the service is available, or `wants` if your code is flexible and + can accept a None from this method. + + Runtimes can override this method if they have different techniques for + finding and delivering services. + + Arguments: + block (an XBlock): this block's class will be examined for service + decorators. + service_name (string): the name of the service requested. + + Returns: + An object implementing the requested service, or None. + + """ + service = None + + if self._module_system: + service = self._module_system.service(block, service_name) + + if service is None: + service = self._descriptor_system.service(block, service_name) + + return service + def __getattr__(self, name): """ If the ModuleSystem doesn't have an attribute, try returning the same attribute from the @@ -1638,6 +1676,30 @@ class PureSystem(ModuleSystem, DescriptorSystem): except AttributeError: return getattr(self._descriptor_system, name) + def __setattr__(self, name, value): + """ + If the ModuleSystem is set, set the attr on it. + Always set the attr on the DescriptorSystem. + """ + if name in self.__slots__: + return super(CombinedSystem, self).__setattr__(name, value) + + if self._module_system: + setattr(self._module_system, name, value) + setattr(self._descriptor_system, name, value) + + def __delattr__(self, name): + """ + If the ModuleSystem is set, delete the attribute from it. + Always delete the attribute from the DescriptorSystem. + """ + if self._module_system: + delattr(self._module_system, name) + delattr(self._descriptor_system, name) + + def __repr__(self): + return "CombinedSystem({!r}, {!r})".format(self._module_system, self._descriptor_system) + class DoNothingCache(object): """A duck-compatible object to use in ModuleSystem when there's no cache.""" diff --git a/lms/djangoapps/courseware/tests/__init__.py b/lms/djangoapps/courseware/tests/__init__.py index ede67b9778..b38f408e2f 100644 --- a/lms/djangoapps/courseware/tests/__init__.py +++ b/lms/djangoapps/courseware/tests/__init__.py @@ -57,17 +57,7 @@ class BaseTestXmodule(ModuleStoreTestCase): """ Generate a new ModuleSystem that is minimally set up for testing """ - runtime = get_test_system(course_id=self.course.id) - - # When asked for a module out of a descriptor, just create a new xmodule runtime, - # and inject it into the descriptor - def get_module(descr): - descr.xmodule_runtime = self.new_module_runtime() - return descr - - runtime.get_module = get_module - - return runtime + return get_test_system(course_id=self.course.id) def new_descriptor_runtime(self): runtime = get_test_descriptor_system() diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index cf4b8a6235..7e17441142 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -51,11 +51,17 @@ class PureXBlock(XBlock): pass -class EmptyXModule(XModule): +class EmptyXModule(XModule): # pylint: disable=abstract-method + """ + Empty XModule for testing with no dependencies. + """ pass -class EmptyXModuleDescriptor(XModuleDescriptor): +class EmptyXModuleDescriptor(XModuleDescriptor): # pylint: disable=abstract-method + """ + Empty XModule for testing with no dependencies. + """ module_class = EmptyXModule @@ -1118,6 +1124,9 @@ class TestRebindModule(TestSubmittingProblems): @ddt.ddt @override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE) class TestEventPublishing(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Tests of event publishing for both XModules and XBlocks. + """ def setUp(self): """ @@ -1138,7 +1147,7 @@ class TestEventPublishing(ModuleStoreTestCase, LoginEnrollmentTestCase): request.user = self.mock_user course = CourseFactory() descriptor = ItemFactory(category=block_type, parent=course) - field_data_cache = FieldDataCache([course, descriptor], course.id, self.mock_user) + field_data_cache = FieldDataCache([course, descriptor], course.id, self.mock_user) # pylint: disable=no-member block = render.get_module(self.mock_user, request, descriptor.location, field_data_cache) event_type = 'event_type' @@ -1148,8 +1157,4 @@ class TestEventPublishing(ModuleStoreTestCase, LoginEnrollmentTestCase): mock_track_function.assert_called_once_with(request) - if block_type == 'xblock': - self.assertFalse(mock_track_function.return_value.called) - else: - mock_track_function.return_value.assert_called_once_with(event_type, event) - + mock_track_function.return_value.assert_called_once_with(event_type, event)