Merge pull request #6209 from edx/plat/fix-pure-system

Fix PureSystem so that runtime.publish() works for a pure XBlock.

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

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):

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'<p>This is a secret</p>'
         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({

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.

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]

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."""

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()

lms/djangoapps/courseware/tests/test_module_render.py

@@ -39,7 +39,7 @@ from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory, check_mongo_calls
-from xmodule.x_module import XModuleDescriptor, STUDENT_VIEW
+from xmodule.x_module import XModuleDescriptor, XModule, STUDENT_VIEW
 
 TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT
 
@@ -51,6 +51,20 @@ class PureXBlock(XBlock):
     pass
 
 
+class EmptyXModule(XModule):  # pylint: disable=abstract-method
+    """
+    Empty XModule for testing with no dependencies.
+    """
+    pass
+
+
+class EmptyXModuleDescriptor(XModuleDescriptor):  # pylint: disable=abstract-method
+    """
+    Empty XModule for testing with no dependencies.
+    """
+    module_class = EmptyXModule
+
+
 @ddt.ddt
 @override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE)
 class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase):
@@ -1105,3 +1119,42 @@ class TestRebindModule(TestSubmittingProblems):
         module = self.get_module_for_user(self.anon_user)
         module.system.rebind_noauth_module_to_user(module, self.anon_user)
         self.assertFalse(psycho_handler.called)
+
+
+@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):
+        """
+        Set up the course and user context
+        """
+        super(TestEventPublishing, self).setUp()
+
+        self.mock_user = UserFactory()
+        self.mock_user.id = 1
+        self.request_factory = RequestFactory()
+
+    @ddt.data('xblock', 'xmodule')
+    @XBlock.register_temp_plugin(PureXBlock, identifier='xblock')
+    @XBlock.register_temp_plugin(EmptyXModuleDescriptor, identifier='xmodule')
+    @patch.object(render, 'make_track_function')
+    def test_event_publishing(self, block_type, mock_track_function):
+        request = self.request_factory.get('')
+        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)  # pylint: disable=no-member
+        block = render.get_module(self.mock_user, request, descriptor.location, field_data_cache)
+
+        event_type = 'event_type'
+        event = {'event': 'data'}
+
+        block.runtime.publish(block, event_type, event)
+
+        mock_track_function.assert_called_once_with(request)
+
+        mock_track_function.return_value.assert_called_once_with(event_type, event)