From 9c06865c3c8591df979f7fa722729b6b851b79bb Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 12 Feb 2024 11:42:41 -0500 Subject: [PATCH] feat!: collapse extraneous XBlock mixins WIP Closes: https://github.com/openedx/XBlock/issues/714 --- docs/conf.py | 1 + docs/xblock.rst | 4 +- xblock/core.py | 808 +++++++++++++++--- xblock/fields.py | 12 +- xblock/internal.py | 40 - xblock/mixins.py | 629 -------------- xblock/test/test_core.py | 11 +- ...st_mixins.py => test_core_capabilities.py} | 112 +-- xblock/test/test_internal.py | 60 +- 9 files changed, 786 insertions(+), 891 deletions(-) delete mode 100644 xblock/mixins.py rename xblock/test/{test_mixins.py => test_core_capabilities.py} (83%) diff --git a/docs/conf.py b/docs/conf.py index db3f46c03..fabefb96e 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -128,6 +128,7 @@ ('py:class', 'aside'), ('py:class', 'aside_fn'), ('py:class', 'webob.Request'), + ('py:class', 'webob.Response'), ] suppress_warnings = [ diff --git a/docs/xblock.rst b/docs/xblock.rst index adad415cf..2d796f028 100644 --- a/docs/xblock.rst +++ b/docs/xblock.rst @@ -6,8 +6,10 @@ XBlock API .. autoclass:: xblock.core.XBlock :members: - :inherited-members: .. autoclass:: xblock.core.XBlockAside :members: + +.. autoclass:: xblock.core.Blocklike + :members: :inherited-members: diff --git a/xblock/core.py b/xblock/core.py index 2c93195ac..42244af87 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -1,58 +1,94 @@ """ -Core classes for the XBlock family. - -This code is in the Runtime layer, because it is authored once by edX -and used by all runtimes. - +Base classes for all XBlock-like objects. Used by all XBlock Runtimes. """ -from collections import defaultdict +import copy +import functools import inspect +import json +import logging import os import warnings +from collections import OrderedDict, defaultdict import pkg_resources - -import xblock.exceptions -from xblock.exceptions import DisallowedFileError -from xblock.fields import String, List, Scope -from xblock.internal import class_lazy -import xblock.mixins -from xblock.mixins import ( - ScopedStorageMixin, - HierarchyMixin, - RuntimeServicesMixin, - HandlersMixin, - XmlSerializationMixin, - IndexInfoMixin, - ViewsMixin, +from lxml import etree +from webob import Response + +from xblock.exceptions import ( + DisallowedFileError, + FieldDataDeprecationWarning, + JsonHandlerError, + KeyValueMultiSaveError, + XBlockSaveError, ) +from xblock.fields import Field, List, Reference, ReferenceList, Scope, String +from xblock.internal import class_lazy from xblock.plugin import Plugin from xblock.validation import Validation -# exposing XML_NAMESPACES as a member of core, in order to avoid importing mixins where -# XML_NAMESPACES are needed (e.g. runtime.py). -XML_NAMESPACES = xblock.mixins.XML_NAMESPACES +# OrderedDict is used so that namespace attributes are put in predictable order +# This allows for simple string equality assertions in tests and have no other effects +XML_NAMESPACES = OrderedDict([ + ("option", "http://code.edx.org/xblock/option"), + ("block", "http://code.edx.org/xblock/block"), +]) # __all__ controls what classes end up in the docs. -__all__ = ['XBlock'] +__all__ = ['Blocklike', 'XBlock', 'XBlockAside'] + UNSET = object() -class XBlockMixin(ScopedStorageMixin): +class _AutoNamedFieldsMetaclass(type): """ - Base class for XBlock Mixin classes. + Builds classes such that their Field attributes know their own names. - XBlockMixin classes can add new fields and new properties to all XBlocks - created by a particular runtime. + This allows XBlock API users to define fields without name redundancy, e.g. like this: - """ + class MyBlock(XBlock): + my_field = Field(...) + rather than this: -class SharedBlockBase(Plugin): + class MyBlock(XBlock): + my_field = Field(name="my_field", ...) """ - Behaviors and attrs which all XBlock like things should share + def __new__(mcs, name, bases, attrs): + """ + Ensure __name__ is set on all Field attributes, both on the new class and on its bases. + """ + def needs_name(obj): + """ + Is this object a Field that hasn't had its name assigned yet? + """ + return isinstance(obj, Field) and not obj.__name__ + + # Iterate over the attrs before they're bound to the class + # so that we don't accidentally trigger any __get__ methods + for attr_name, attr in attrs.items(): + if needs_name(attr): + attr.__name__ = attr_name + + # Iterate over all of the base classes, so that we can add + # names to any mixins that don't include this metaclass, but that + # do include Fields + for base in bases: + for attr_name, attr in inspect.getmembers(base, predicate=needs_name): + attr.__name__ = attr_name + + return super().__new__(mcs, name, bases, attrs) + + +class Blocklike(metaclass=_AutoNamedFieldsMetaclass): """ + Shared base for :class:`.XBlock` and :class:`.XBlockAside`. Provides capabilities: + - **Services**: Need, want, and use services from the Runtime. + - **Fields**: Define named fields with scoped storage. + - **OLX**: Serialize to and deserialize from Open Learning XML (OLX). + - **Handlers**: Mark methods that respond to AJAX requests as "handlers". + - **Resources**: Access files on the local system. + """ resources_dir = '' public_dir = 'public' i18n_js_namespace = None @@ -60,24 +96,24 @@ class SharedBlockBase(Plugin): @classmethod def get_resources_dir(cls): """ - Gets the resource directory for this XBlock. + Gets the resource directory for this Blocklike. """ return cls.resources_dir @classmethod def get_public_dir(cls): """ - Gets the public directory for this XBlock. + Gets the public directory for this Blocklike. """ return cls.public_dir @classmethod def get_i18n_js_namespace(cls): """ - Gets the JavaScript translations namespace for this XBlock. + Gets the JavaScript translations namespace for this Blocklike. Returns: - str: The JavaScript namespace for this XBlock. + str: The JavaScript namespace for this Blocklike. None: If this doesn't have JavaScript translations configured. """ return cls.i18n_js_namespace @@ -90,13 +126,13 @@ def open_local_resource(cls, uri): The container calls this method when it receives a request for a resource on a URL which was generated by Runtime.local_resource_url(). It will pass the URI from the original call to local_resource_url() - back to this method. The XBlock must parse this URI and return an open + back to this method. The Blocklike must parse this URI and return an open file-like object for the resource. For security reasons, the default implementation will return only a very restricted set of file types, which must be located in a folder that defaults to "public". The location used for public resources can - be changed on a per-XBlock basis. XBlock authors who want to override + be changed on a per-Blocklike basis. Blocklike authors who want to override this behavior will need to take care to ensure that the method only serves legitimate public resources. At the least, the URI should be matched against a whitelist regex to ensure that you do not serve an @@ -121,23 +157,484 @@ def open_local_resource(cls, uri): return pkg_resources.resource_stream(cls.__module__, os.path.join(cls.resources_dir, uri)) + @classmethod + def json_handler(cls, func): + """ + Wrap a handler to consume and produce JSON. + + Rather than a Request object, the method will now be passed the + JSON-decoded body of the request. The request should be a POST request + in order to use this method. Any data returned by the function + will be JSON-encoded and returned as the response. + + The wrapped function can raise JsonHandlerError to return an error + response with a non-200 status code. + + This decorator will return a 405 HTTP status code if the method is not + POST. + This decorator will return a 400 status code if the body contains + invalid JSON. + """ + @cls.handler + @functools.wraps(func) + def wrapper(self, request, suffix=''): + """The wrapper function `json_handler` returns.""" + if request.method != "POST": + return JsonHandlerError(405, "Method must be POST").get_response(allow=["POST"]) + try: + request_json = json.loads(request.body.decode('utf-8')) + except ValueError: + return JsonHandlerError(400, "Invalid JSON").get_response() + try: + response = func(self, request_json, suffix) + except JsonHandlerError as err: + return err.get_response() + if isinstance(response, Response): + return response + else: + return Response(json.dumps(response), content_type='application/json', charset='utf8') + return wrapper + + @classmethod + def handler(cls, func): + """ + A decorator to indicate a function is usable as a handler. + + The wrapped function must return a :class:`webob.Response` object. + """ + func._is_xblock_handler = True # pylint: disable=protected-access + return func + + @classmethod + def needs(cls, *service_names): + """ + A class decorator to indicate that a Blocklike class needs particular services. + """ + def _decorator(cls_): + for service_name in service_names: + cls_._services_requested[service_name] = "need" # pylint: disable=protected-access + return cls_ + return _decorator + + @classmethod + def wants(cls, *service_names): + """ + A class decorator to indicate that a Blocklike class wants particular services. + """ + def _decorator(cls_): + for service_name in service_names: + cls_._services_requested[service_name] = "want" # pylint: disable=protected-access + return cls_ + return _decorator + + @classmethod + def service_declaration(cls, service_name): + """ + Find and return a service declaration. + + Blocklikes declare their service requirements with `@XBlock{Aside}.needs` and + `@XBlock{Aside}.wants` decorators. These store information on the class. + This function finds those declarations for a block. + + Arguments: + service_name (str): the name of the service requested. + + Returns: + One of "need", "want", or None. + """ + return cls._combined_services.get(service_name) # pylint: disable=no-member + + @class_lazy + def _services_requested(cls): # pylint: disable=no-self-argument + """ + A per-class dictionary to store the services requested by a particular XBlock. + """ + return {"field-data": "needs"} # All Blocklikes needs field data + + @class_lazy + def _combined_services(cls): # pylint: disable=no-self-argument + """ + A dictionary that collects all _services_requested by all ancestors of this XBlock class. + """ + # The class declares what services it desires. To deal with subclasses, + # especially mixins, properly, we have to walk up the inheritance + # hierarchy, and combine all the declared services into one dictionary. + combined = {} + for parent in reversed(cls.mro()): # pylint: disable=no-member + combined.update(getattr(parent, "_services_requested", {})) + return combined + + @class_lazy + def fields(cls): # pylint: disable=no-self-argument + """ + A dictionary mapping the attribute name to the Field object for all Field attributes of the class. + """ + fields = {} + # Loop through all of the baseclasses of cls, in + # the order that methods are resolved (Method Resolution Order / mro) + # and find all of their defined fields. + # + # Only save the first such defined field (as expected for method resolution) + + bases = cls.mro() # pylint: disable=no-member + local = bases.pop(0) + + # First, descend the MRO from the top down, updating the 'fields' dictionary + # so that the dictionary always has the most specific version of fields in it + for base in reversed(bases): + fields.update(getattr(base, 'fields', {})) + + # For this class, loop through all attributes not named 'fields', + # find those of type Field, and save them to the 'fields' dict + for attr_name, attr_value in inspect.getmembers(local, lambda attr: isinstance(attr, Field)): + fields[attr_name] = attr_value + + return fields + + @classmethod + def _set_field_if_present(cls, block, name, value, attrs): + """ + Sets the field block.name, if block have such a field. + """ + if name in block.fields: + value = (block.fields[name]).from_string(value) + if "none" in attrs and attrs["none"] == "true": + setattr(block, name, None) + else: + setattr(block, name, value) + else: + logging.warning("Blocklike %s does not contain field %s", type(block), name) + + def __init__(self, runtime, scope_ids, field_data=None, **kwargs): + """ + Arguments: + + runtime (:class:`.Runtime`): Use it to access the environment. + It is available in XBlock code as ``self.runtime``. + + scope_ids (:class:`.ScopeIds`): Identifiers needed to resolve + scopes. + + field_data (:class:`.FieldData`): Interface used by Blocklike's + fields to access their data from wherever it is persisted. + DEPRECATED--supply a field-data Runtime service instead. + """ + self.runtime = runtime + + # This is used to store a directly passed field data + # for backwards compatibility + if field_data: + warnings.warn( + "Setting _field_data via the constructor is deprecated, please use a Runtime service", + FieldDataDeprecationWarning, + stacklevel=2 + ) + # Storing _field_data instead of _deprecated_per_instance_field_data allows subclasses to + # continue to override this behavior (for instance, the way that edx-platform's XModule does + # in order to proxy to XBlock). + self._field_data = field_data + else: + self._deprecated_per_instance_field_data = None # pylint: disable=invalid-name + + self._field_data_cache = {} + self._dirty_fields = {} + self.scope_ids = scope_ids + + super().__init__(**kwargs) + + def __repr__(self): + attrs = [] + for field in self.fields.values(): # pylint: disable=no-member + try: + value = getattr(self, field.name) + except Exception: # pylint: disable=broad-except + # Ensure we return a string, even if unanticipated exceptions. + attrs.append(f" {field.name}=???") + else: + if isinstance(value, bytes): + value = value.decode('utf-8', errors='escape') + if isinstance(value, str): + value = value.strip() + if len(value) > 40: + value = value[:37] + "..." + attrs.append(f" {field.name}={value!r}") + return "<{} @{:04X}{}>".format( + self.__class__.__name__, + id(self) % 0xFFFF, + ','.join(attrs) + ) + + @property + def usage_key(self): + """ + A key identifying this particular usage of the Blocklike, unique across all learning contexts in the system. + + Equivalent to to `.scope_ids.usage_id`. + """ + return self.scope_ids.usage_id + + @property + def context_key(self): + """ + A key identifying the learning context (course, library, etc.) that contains this Blocklike usage. + + Equivalent to `.scope_ids.usage_id.context_key`. + + Returns: + * `LearningContextKey`, if `.scope_ids.usage_id` is a `UsageKey` instance. + * `None`, otherwise. + + After https://github.com/openedx/XBlock/issues/708 is complete, we can assume that + `.scope_ids.usage_id` is always a `UsageKey`, and that this method will + always return a `LearningContextKey`. + """ + return getattr(self.scope_ids.usage_id, "context_key", None) + + def index_dictionary(self): + """ + Return a dict containing information that could be used to feed a search index. + + Values may be numeric, string, or dict. + """ + display_name = getattr(self, "display_name", None) + + # Getting self.display_name.default wouldn't work as self.display_name is actually + # a str after the class instance is created. So, we can only access the default value + # of display_name field by accessing class variable of same name + content_type = getattr( + getattr(self.__class__, "display_name", None), "default", None + ) + + _index_dictionary = {} + + if display_name is not None: + _index_dictionary.update({ + "content": { + "display_name": display_name + } + }) + + if content_type is not None: + _index_dictionary.update({ + "content_type": content_type + }) + + return _index_dictionary + + def handle(self, handler_name, request, suffix=''): + """ + Handle `request` with this block's runtime. + """ + return self.runtime.handle(self, handler_name, request, suffix) + + @property + def _field_data(self): + """ + Return the FieldData for this XBlock (either as passed in the constructor + or from retrieving the 'field-data' service). + """ + if self._deprecated_per_instance_field_data: + return self._deprecated_per_instance_field_data + else: + return self.runtime.service(self, 'field-data') + + @_field_data.setter + def _field_data(self, field_data): + """ + Set _field_data. Deprecated. + """ + warnings.warn("Setting _field_data is deprecated", FieldDataDeprecationWarning, stacklevel=2) + self._deprecated_per_instance_field_data = field_data + + def save(self): + """ + Save all dirty fields attached to this XBlock. + """ + if not self._dirty_fields: + # nop if _dirty_fields attribute is empty + return + + fields_to_save = self._get_fields_to_save() + if fields_to_save: + self.force_save_fields(fields_to_save) + self.runtime.save_block(self) + + def force_save_fields(self, field_names): + """ + Save all fields that are specified in `field_names`, even if they are not dirty. + """ + fields = [ + self.fields[field_name] # pylint: disable=unsubscriptable-object + for field_name in field_names + ] + fields_to_save_json = {} + for field in fields: + fields_to_save_json[field.name] = field.to_json(self._field_data_cache[field.name]) -# -- Base Block -class XBlock(XmlSerializationMixin, HierarchyMixin, ScopedStorageMixin, RuntimeServicesMixin, HandlersMixin, - IndexInfoMixin, ViewsMixin, SharedBlockBase): - """Base class for XBlocks. + try: + # Throws KeyValueMultiSaveError if things go wrong + self._field_data.set_many(self, fields_to_save_json) + except KeyValueMultiSaveError as save_error: + saved_fields = [field for field in fields + if field.name in save_error.saved_field_names] + for field in saved_fields: + # should only find one corresponding field + fields.remove(field) + # if the field was dirty, delete from dirty fields + self._reset_dirty_field(field) + msg = f'Error saving fields {save_error.saved_field_names}' + raise XBlockSaveError(saved_fields, fields, msg) # pylint: disable= raise-missing-from - Derive from this class to create a new kind of XBlock. There are no - required methods, but you will probably need at least one view. + # Remove all dirty fields, since the save was successful + for field in fields: + self._reset_dirty_field(field) - Don't provide the ``__init__`` method when deriving from this class. + def _get_fields_to_save(self): + """ + Get an xblock's dirty fields. + """ + # If the field value isn't the same as the baseline we recorded + # when it was read, then save it + # pylint: disable=protected-access + return [field.name for field in self._dirty_fields if field._is_dirty(self)] + def _clear_dirty_fields(self): + """ + Remove all dirty fields from an XBlock. + """ + self._dirty_fields.clear() + + def _reset_dirty_field(self, field): + """ + Resets dirty field value with the value from the field data cache. + """ + if field in self._dirty_fields: + self._dirty_fields[field] = copy.deepcopy( + self._field_data_cache[field.name] + ) + + def add_xml_to_node(self, node): + """ + For exporting, set data on `node` from ourselves. + """ + # pylint: disable=E1101 + # Set node.tag based on our class name. + node.tag = self.xml_element_name() + node.set('xblock-family', self.entry_point) + + # Set node attributes based on our fields. + for field_name, field in list(self.fields.items()): + if field_name in ('children', 'parent', 'content'): + continue + if field.is_set_on(self) or field.force_export: + self._add_field(node, field_name, field) + + # A content field becomes text content. + text = self.xml_text_content() + if text is not None: + node.text = text + + def xml_element_name(self): + """ + What XML element name should be used for this block? + """ + return self.scope_ids.block_type + + def xml_text_content(self): + """ + What is the text content for this block's XML node? + """ + if 'content' in self.fields and self.content: # pylint: disable=unsupported-membership-test + return self.content + else: + return None + + def _add_field(self, node, field_name, field): + """ + Add xml representation of field to node. + + Depending on settings, it either stores the value of field + as an xml attribute or creates a separate child node. + """ + value = field.to_string(field.read_from(self)) + text_value = "" if value is None else value + + # Is the field type supposed to serialize the fact that the value is None to XML? + save_none_as_xml_attr = field.none_to_xml and value is None + field_attrs = {"none": "true"} if save_none_as_xml_attr else {} + + if save_none_as_xml_attr or field.xml_node: + # Field will be output to XML as an separate element. + tag = etree.QName(XML_NAMESPACES["option"], field_name) + elem = etree.SubElement(node, tag, field_attrs) + if field.xml_node: + # Only set the value if forced via xml_node; + # in all other cases, the value is None. + # Avoids an unnecessary XML end tag. + elem.text = text_value + else: + # Field will be output to XML as an attribute on the node. + node.set(field_name, text_value) + + +class XBlockMixin(Blocklike): + """ + Base class for custom XBlock mixins. + + To provide custom attributes to all XBlock instances in a Runtime, extend this class and + supply it to the Runtime's `mixins` parameter. + + (...or don't do that, because it's confusing for other developers, and linters/type-checkers + won't understand that the mixins are part of XBlock instances, so your code will be full + of ignore statements :) + """ + + +class _HasChildrenMetaclass(_AutoNamedFieldsMetaclass): + """ + Adds a ``children`` XBlock ReferenceList field to classes where ``has_children == True``. + """ + def __new__(mcs, name, bases, attrs): + if (attrs.get('has_children', False) or any(getattr(base, 'has_children', False) for base in bases)): + attrs['children'] = ReferenceList( + help='The ids of the children of this XBlock', + scope=Scope.children) + else: + attrs['has_children'] = False + + return super().__new__(mcs, name, bases, attrs) + + +class XBlock(Plugin, Blocklike, metaclass=_HasChildrenMetaclass): """ + Base class for XBlocks. Capabilities include: + + - All those of :class:`.Blocklike`. + - **Views**: Mark methods that render HTML as "views". + - **Hierarchy**: Access this block's parent; manage and access its children. + - **Plugging-in:**: Plug the class into a platform using the 'xblock.v1' entrypoint. + - **Tagging**: Add simple metadata strings to the class, to be interpreted by the Runtime. + + Derive from this class to create a new kind of XBlock. There are no + required methods, but in order for the XBlock type to be useful, you will + probably want to provide at least one view. + + Don't override the ``__init__`` method when deriving from this class. + """ + entry_point = 'xblock.v1' name = String(help="Short name for the block", scope=Scope.settings) tags = List(help="Tags for this block", scope=Scope.settings) + parent = Reference(help='The id of the parent of this XBlock', default=None, scope=Scope.parent) + + # These are dynamically managed by the awful hackery of _HasChildrenMetaclass. + # We just declare their types here to make static analyzers happy. + # Note that children is only defined iff has_children is defined and True. + has_children: bool + children: ReferenceList + @class_lazy def _class_tags(cls): # pylint: disable=no-self-argument """ @@ -152,7 +649,9 @@ def _class_tags(cls): # pylint: disable=no-self-argument @staticmethod def tag(tags): - """Returns a function that adds the words in `tags` as class tags to this class.""" + """ + Returns a function that adds the words in `tags` as class tags to this class. + """ def dec(cls): """Add the words in `tags` as class tags to this class.""" # Add in this class's tags @@ -174,19 +673,67 @@ def load_tagged_classes(cls, tag, fail_silently=True): (e.g. on startup or first page load), and in what contexts. Hence, the flag. """ - # Allow this method to access the `_class_tags` - # pylint: disable=W0212 for name, class_ in cls.load_classes(fail_silently): - if tag in class_._class_tags: + if tag in class_._class_tags: # pylint: disable=protected-access yield name, class_ - # pylint: disable=keyword-arg-before-vararg - def __init__(self, runtime, field_data=None, scope_ids=UNSET, *args, **kwargs): + @classmethod + def parse_xml(cls, node, runtime, keys, id_generator): + """ + Use `node` to construct a new block. + + Arguments: + node (:class:`~xml.etree.ElementTree.Element`): The xml node to parse into an xblock. + + runtime (:class:`.Runtime`): The runtime to use while parsing. + + keys (:class:`.ScopeIds`): The keys identifying where this block + will store its data. + + id_generator (:class:`.IdGenerator`): An object that will allow the + runtime to generate correct definition and usage ids for + children of this block. + """ - Construct a new XBlock. + block = runtime.construct_xblock_from_class(cls, keys) + + # The base implementation: child nodes become child blocks. + # Or fields, if they belong to the right namespace. + for child in node: + if child.tag is etree.Comment: + continue + qname = etree.QName(child) + tag = qname.localname + namespace = qname.namespace + + if namespace == XML_NAMESPACES["option"]: + cls._set_field_if_present(block, tag, child.text, child.attrib) + else: + block.runtime.add_node_as_child(block, child, id_generator) + + # Attributes become fields. + for name, value in list(node.items()): # lxml has no iteritems + cls._set_field_if_present(block, name, value, {}) - This class should only be instantiated by runtimes. + # Text content becomes "content", if such a field exists. + if "content" in block.fields and block.fields["content"].scope == Scope.content: + text = node.text + if text: + text = text.strip() + if text: + block.content = text + return block + + def __init__( + self, + runtime, + field_data=None, + *args, # pylint: disable=keyword-arg-before-vararg + scope_ids=UNSET, + **kwargs + ): + """ Arguments: runtime (:class:`.Runtime`): Use it to access the environment. @@ -202,34 +749,18 @@ def __init__(self, runtime, field_data=None, scope_ids=UNSET, *args, **kwargs): if scope_ids is UNSET: raise TypeError('scope_ids are required') - # Provide backwards compatibility for external access through _field_data - super().__init__(runtime=runtime, scope_ids=scope_ids, field_data=field_data, *args, **kwargs) + # A cache of the parent block, retrieved from .parent + self._parent_block = None + self._parent_block_id = None + self._child_cache = {} - @property - def usage_key(self): - """ - A key identifying this particular usage of the XBlock, unique across all learning contexts in the system. - - Equivalent to to `.scope_ids.usage_id`. - """ - return self.scope_ids.usage_id - - @property - def context_key(self): - """ - A key identifying the learning context (course, library, etc.) that contains this XBlock usage. - - Equivalent to `.scope_ids.usage_id.context_key`. - - Returns: - * `LearningContextKey`, if `.scope_ids.usage_id` is a `UsageKey` instance. - * `None`, otherwise. + for_parent = kwargs.pop('for_parent', None) + if for_parent is not None: + self._parent_block = for_parent + self._parent_block_id = for_parent.scope_ids.usage_id - After https://github.com/openedx/XBlock/issues/708 is complete, we can assume that - `.scope_ids.usage_id` is always a `UsageKey`, and that this method will - always return a `LearningContextKey`. - """ - return getattr(self.scope_ids.usage_id, "context_key", None) + # Provide backwards compatibility for external access through _field_data + super().__init__(runtime=runtime, scope_ids=scope_ids, field_data=field_data, *args, **kwargs) def render(self, view, context=None): """Render `view` with this block's runtime and the supplied `context`""" @@ -261,10 +792,109 @@ def add_xml_to_node(self, node): # Add children for each of our children. self.add_children_to_node(node) + def get_parent(self): + """Return the parent block of this block, or None if there isn't one.""" + if not self.has_cached_parent: + if self.parent is not None: + self._parent_block = self.runtime.get_block(self.parent) + else: + self._parent_block = None + self._parent_block_id = self.parent + return self._parent_block + + @property + def has_cached_parent(self): + """Return whether this block has a cached parent block.""" + return self.parent is not None and self._parent_block_id == self.parent + + def get_child(self, usage_id): + """Return the child identified by ``usage_id``.""" + if usage_id in self._child_cache: + return self._child_cache[usage_id] + + child_block = self.runtime.get_block(usage_id, for_parent=self) + self._child_cache[usage_id] = child_block + return child_block + + def get_children(self, usage_id_filter=None): + """ + Return instantiated XBlocks for each of this blocks ``children``. + """ + if not self.has_children: + return [] + + return [ + self.get_child(usage_id) + for usage_id in self.children + if usage_id_filter is None or usage_id_filter(usage_id) + ] -class XBlockAside(XmlSerializationMixin, ScopedStorageMixin, RuntimeServicesMixin, HandlersMixin, SharedBlockBase): + def clear_child_cache(self): + """ + Reset the cache of children stored on this XBlock. + """ + self._child_cache.clear() + + def add_children_to_node(self, node): + """ + Add children to etree.Element `node`. + """ + if self.has_children: + for child_id in self.children: + child = self.runtime.get_block(child_id) + self.runtime.add_block_as_child_node(child, node) + + @classmethod + def supports(cls, *functionalities): + """ + A view decorator to indicate that an xBlock view has support for the + given functionalities. + + Arguments: + functionalities: String identifiers for the functionalities of the view. + For example: "multi_device". + """ + def _decorator(view): + """ + Internal decorator that updates the given view's list of supported + functionalities. + """ + # pylint: disable=protected-access + if not hasattr(view, "_supports"): + view._supports = set() + for functionality in functionalities: + view._supports.add(functionality) + return view + return _decorator + + def has_support(self, view, functionality): + """ + Returns whether the given view has support for the given functionality. + + An XBlock view declares support for a functionality with the + @XBlock.supports decorator. The decorator stores information on the view. + + Note: We implement this as an instance method to allow xBlocks to + override it, if necessary. + + Arguments: + view (object): The view of the xBlock. + functionality (str): A functionality of the view. + For example: "multi_device". + + Returns: + True or False + """ + return hasattr(view, "_supports") and functionality in view._supports # pylint: disable=protected-access + + +class XBlockAside(Plugin, Blocklike): """ - This mixin allows Xblock-like class to declare that it provides aside functionality. + Base class :class:`.Blocklike` objects that get injected into :class:`.XBlock` views. Capabilities include: + + - All those of :class:`.Blocklike`. + - **Injection:** Pick which views of which :class:`.XBlock` instances that this should be rendered into. + - **Plugging-in:** Plug the class into a platform using the 'xblock_asides.v1' entrypoint. """ entry_point = "xblock_asides.v1" @@ -340,21 +970,3 @@ def needs_serialization(self): be serialized as XML at all. """ return any(field.is_set_on(self) for field in self.fields.values()) # pylint: disable=no-member - - -class KeyValueMultiSaveError(xblock.exceptions.KeyValueMultiSaveError): - """ - Backwards compatibility class wrapper around :class:`.KeyValueMultiSaveError`. - """ - def __init__(self, *args, **kwargs): - warnings.warn("Please use xblock.exceptions.KeyValueMultiSaveError", DeprecationWarning, stacklevel=2) - super().__init__(*args, **kwargs) - - -class XBlockSaveError(xblock.exceptions.XBlockSaveError): - """ - Backwards compatibility class wrapper around :class:`.XBlockSaveError`. - """ - def __init__(self, *args, **kwargs): - warnings.warn("Please use xblock.exceptions.XBlockSaveError", DeprecationWarning, stacklevel=2) - super().__init__(*args, **kwargs) diff --git a/xblock/fields.py b/xblock/fields.py index 8f5eecc8a..8a8e95a36 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -20,7 +20,6 @@ import pytz import yaml -from xblock.internal import Nameable # __all__ controls what classes end up in the docs, and in what order. __all__ = [ @@ -263,7 +262,7 @@ class ScopeIds(namedtuple('ScopeIds', 'user_id block_type def_id usage_id')): NO_GENERATED_DEFAULTS = ('parent', 'children') -class Field(Nameable): +class Field: """ A field class that can be used as a class attribute to define what data the class will want to refer to. @@ -312,6 +311,8 @@ class will want to refer to. # Indicates if a field's None value should be sent to the XML representation. none_to_xml = False + __name__ = None + # We're OK redefining built-in `help` def __init__(self, help=None, default=UNSET, scope=Scope.content, # pylint:disable=redefined-builtin display_name=None, values=None, enforce_type=False, @@ -339,6 +340,13 @@ def default(self): else: return self._default + @staticmethod + def needs_name(field): + """ + Returns whether the given ) is yet to be named. + """ + return not field.__name__ + @property def name(self): """Returns the name of this field.""" diff --git a/xblock/internal.py b/xblock/internal.py index 12632aff0..087eb905c 100644 --- a/xblock/internal.py +++ b/xblock/internal.py @@ -2,7 +2,6 @@ Internal machinery used to make building XBlock family base classes easier. """ import functools -import inspect class LazyClassProperty: @@ -28,42 +27,3 @@ def __get__(self, instance, owner): class_lazy = LazyClassProperty # pylint: disable=invalid-name - - -class NamedAttributesMetaclass(type): - """ - A metaclass which adds the __name__ attribute to all Nameable attributes - which are attributes of the instantiated class, or of its baseclasses. - """ - def __new__(mcs, name, bases, attrs): - # Iterate over the attrs before they're bound to the class - # so that we don't accidentally trigger any __get__ methods - for attr_name, attr in attrs.items(): - if Nameable.needs_name(attr): - attr.__name__ = attr_name - - # Iterate over all of the base classes, so that we can add - # names to any mixins that don't include this metaclass, but that - # do include Nameable attributes - for base in bases: - for attr_name, attr in inspect.getmembers(base, Nameable.needs_name): - attr.__name__ = attr_name - - return super().__new__(mcs, name, bases, attrs) - - -class Nameable: - """ - A base class for class attributes which, when used in concert with - :class:`.NamedAttributesMetaclass`, will be assigned a `__name__` - attribute based on what class attribute they are bound to. - """ - __name__ = None - - @staticmethod - def needs_name(obj): - """ - Return True if `obj` is a :class:`.Nameable` object that - hasn't yet been assigned a name. - """ - return isinstance(obj, Nameable) and obj.__name__ is None diff --git a/xblock/mixins.py b/xblock/mixins.py deleted file mode 100644 index e921caf80..000000000 --- a/xblock/mixins.py +++ /dev/null @@ -1,629 +0,0 @@ -""" -This module defines all of the Mixins that provide components of XBlock-family -functionality, such as ScopeStorage, RuntimeServices, and Handlers. -""" -from collections import OrderedDict -import copy -import functools -import inspect -import logging -import warnings -import json - -from lxml import etree -from webob import Response - -from xblock.exceptions import JsonHandlerError, KeyValueMultiSaveError, XBlockSaveError, FieldDataDeprecationWarning -from xblock.fields import Field, Reference, Scope, ReferenceList -from xblock.internal import class_lazy, NamedAttributesMetaclass - - -# OrderedDict is used so that namespace attributes are put in predictable order -# This allows for simple string equality assertions in tests and have no other effects -XML_NAMESPACES = OrderedDict([ - ("option", "http://code.edx.org/xblock/option"), - ("block", "http://code.edx.org/xblock/block"), -]) - - -class HandlersMixin: - """ - A mixin provides all of the machinery needed for working with XBlock-style handlers. - """ - - @classmethod - def json_handler(cls, func): - """ - Wrap a handler to consume and produce JSON. - - Rather than a Request object, the method will now be passed the - JSON-decoded body of the request. The request should be a POST request - in order to use this method. Any data returned by the function - will be JSON-encoded and returned as the response. - - The wrapped function can raise JsonHandlerError to return an error - response with a non-200 status code. - - This decorator will return a 405 HTTP status code if the method is not - POST. - This decorator will return a 400 status code if the body contains - invalid JSON. - """ - @cls.handler - @functools.wraps(func) - def wrapper(self, request, suffix=''): - """The wrapper function `json_handler` returns.""" - if request.method != "POST": - return JsonHandlerError(405, "Method must be POST").get_response(allow=["POST"]) - try: - request_json = json.loads(request.body.decode('utf-8')) - except ValueError: - return JsonHandlerError(400, "Invalid JSON").get_response() - try: - response = func(self, request_json, suffix) - except JsonHandlerError as err: - return err.get_response() - if isinstance(response, Response): - return response - else: - return Response(json.dumps(response), content_type='application/json', charset='utf8') - return wrapper - - @classmethod - def handler(cls, func): - """ - A decorator to indicate a function is usable as a handler. - - The wrapped function must return a `webob.Response` object. - """ - func._is_xblock_handler = True # pylint: disable=protected-access - return func - - def handle(self, handler_name, request, suffix=''): - """Handle `request` with this block's runtime.""" - return self.runtime.handle(self, handler_name, request, suffix) - - -class RuntimeServicesMixin: - """ - This mixin provides all of the machinery needed for an XBlock-style object - to declare dependencies on particular runtime services. - """ - - @class_lazy - def _services_requested(cls): # pylint: disable=no-self-argument - """A per-class dictionary to store the services requested by a particular XBlock.""" - return {} - - @class_lazy - def _combined_services(cls): # pylint: disable=no-self-argument - """ - A dictionary that collects all _services_requested by all ancestors of this XBlock class. - """ - # The class declares what services it desires. To deal with subclasses, - # especially mixins, properly, we have to walk up the inheritance - # hierarchy, and combine all the declared services into one dictionary. - combined = {} - for parent in reversed(cls.mro()): - combined.update(getattr(parent, "_services_requested", {})) - return combined - - def __init__(self, runtime, **kwargs): - """ - Arguments: - - runtime (:class:`.Runtime`): Use it to access the environment. - It is available in XBlock code as ``self.runtime``. - """ - self.runtime = runtime - super().__init__(**kwargs) - - @classmethod - def needs(cls, *service_names): - """A class decorator to indicate that an XBlock class needs particular services.""" - def _decorator(cls_): - for service_name in service_names: - cls_._services_requested[service_name] = "need" # pylint: disable=protected-access - return cls_ - return _decorator - - @classmethod - def wants(cls, *service_names): - """A class decorator to indicate that an XBlock class wants particular services.""" - def _decorator(cls_): - for service_name in service_names: - cls_._services_requested[service_name] = "want" # pylint: disable=protected-access - return cls_ - return _decorator - - @classmethod - def service_declaration(cls, service_name): - """ - Find and return a service declaration. - - XBlocks declare their service requirements with `@XBlock.needs` and - `@XBlock.wants` decorators. These store information on the class. - This function finds those declarations for a block. - - Arguments: - service_name (str): the name of the service requested. - - Returns: - One of "need", "want", or None. - """ - return cls._combined_services.get(service_name) # pylint: disable=no-member - - -@RuntimeServicesMixin.needs('field-data') -class ScopedStorageMixin(RuntimeServicesMixin, metaclass=NamedAttributesMetaclass): - """ - This mixin provides scope for Fields and the associated Scoped storage. - """ - - @class_lazy - def fields(cls): # pylint: disable=no-self-argument - """ - A dictionary mapping the attribute name to the Field object for all - Field attributes of the class. - """ - fields = {} - # Loop through all of the baseclasses of cls, in - # the order that methods are resolved (Method Resolution Order / mro) - # and find all of their defined fields. - # - # Only save the first such defined field (as expected for method resolution) - - bases = cls.mro() - local = bases.pop(0) - - # First, descend the MRO from the top down, updating the 'fields' dictionary - # so that the dictionary always has the most specific version of fields in it - for base in reversed(bases): - fields.update(getattr(base, 'fields', {})) - - # For this class, loop through all attributes not named 'fields', - # find those of type Field, and save them to the 'fields' dict - for attr_name, attr_value in inspect.getmembers(local, lambda attr: isinstance(attr, Field)): - fields[attr_name] = attr_value - - return fields - - def __init__(self, scope_ids, field_data=None, **kwargs): - """ - Arguments: - field_data (:class:`.FieldData`): Interface used by the XBlock - fields to access their data from wherever it is persisted. - - scope_ids (:class:`.ScopeIds`): Identifiers needed to resolve - scopes. - """ - # This is used to store a directly passed field data - # for backwards compatibility - if field_data: - warnings.warn( - "Setting _field_data via the constructor is deprecated, please use a Runtime service", - FieldDataDeprecationWarning, - stacklevel=2 - ) - # Storing _field_data instead of _deprecated_per_instance_field_data allows subclasses to - # continue to override this behavior (for instance, the way that edx-platform's XModule does - # in order to proxy to XBlock). - self._field_data = field_data - else: - self._deprecated_per_instance_field_data = None # pylint: disable=invalid-name - - self._field_data_cache = {} - self._dirty_fields = {} - self.scope_ids = scope_ids - - super().__init__(**kwargs) - - @property - def _field_data(self): - """ - Return the FieldData for this XBlock (either as passed in the constructor - or from retrieving the 'field-data' service). - """ - if self._deprecated_per_instance_field_data: - return self._deprecated_per_instance_field_data - else: - return self.runtime.service(self, 'field-data') - - @_field_data.setter - def _field_data(self, field_data): - """ - Set _field_data. - - Deprecated. - """ - warnings.warn("Setting _field_data is deprecated", FieldDataDeprecationWarning, stacklevel=2) - self._deprecated_per_instance_field_data = field_data - - def save(self): - """Save all dirty fields attached to this XBlock.""" - if not self._dirty_fields: - # nop if _dirty_fields attribute is empty - return - - fields_to_save = self._get_fields_to_save() - if fields_to_save: - self.force_save_fields(fields_to_save) - self.runtime.save_block(self) - - def force_save_fields(self, field_names): - """ - Save all fields that are specified in `field_names`, even - if they are not dirty. - """ - fields = [self.fields[field_name] for field_name in field_names] - fields_to_save_json = {} - for field in fields: - fields_to_save_json[field.name] = field.to_json(self._field_data_cache[field.name]) - - try: - # Throws KeyValueMultiSaveError if things go wrong - self._field_data.set_many(self, fields_to_save_json) - except KeyValueMultiSaveError as save_error: - saved_fields = [field for field in fields - if field.name in save_error.saved_field_names] - for field in saved_fields: - # should only find one corresponding field - fields.remove(field) - # if the field was dirty, delete from dirty fields - self._reset_dirty_field(field) - msg = f'Error saving fields {save_error.saved_field_names}' - raise XBlockSaveError(saved_fields, fields, msg) # pylint: disable= raise-missing-from - - # Remove all dirty fields, since the save was successful - for field in fields: - self._reset_dirty_field(field) - - def _get_fields_to_save(self): - """ - Get an xblock's dirty fields. - """ - # If the field value isn't the same as the baseline we recorded - # when it was read, then save it - # pylint: disable=protected-access - return [field.name for field in self._dirty_fields if field._is_dirty(self)] - - def _clear_dirty_fields(self): - """ - Remove all dirty fields from an XBlock. - """ - self._dirty_fields.clear() - - def _reset_dirty_field(self, field): - """ - Resets dirty field value with the value from the field data cache. - """ - if field in self._dirty_fields: - self._dirty_fields[field] = copy.deepcopy( - self._field_data_cache[field.name] - ) - - def __repr__(self): - # `ScopedStorageMixin` obtains the `fields` attribute from the `ModelMetaclass`. - # Since this is not understood by static analysis, silence this error. - # pylint: disable=E1101 - attrs = [] - for field in self.fields.values(): - try: - value = getattr(self, field.name) - except Exception: # pylint: disable=broad-except - # Ensure we return a string, even if unanticipated exceptions. - attrs.append(f" {field.name}=???") - else: - if isinstance(value, bytes): - value = value.decode('utf-8', errors='escape') - if isinstance(value, str): - value = value.strip() - if len(value) > 40: - value = value[:37] + "..." - attrs.append(f" {field.name}={value!r}") - return "<{} @{:04X}{}>".format( - self.__class__.__name__, - id(self) % 0xFFFF, - ','.join(attrs) - ) - - -class ChildrenModelMetaclass(ScopedStorageMixin.__class__): - """ - A metaclass that transforms the attribute `has_children = True` into a List - field with a children scope. - - """ - def __new__(mcs, name, bases, attrs): - if (attrs.get('has_children', False) or any(getattr(base, 'has_children', False) for base in bases)): - attrs['children'] = ReferenceList( - help='The ids of the children of this XBlock', - scope=Scope.children) - else: - attrs['has_children'] = False - - return super().__new__(mcs, name, bases, attrs) - - -class HierarchyMixin(ScopedStorageMixin, metaclass=ChildrenModelMetaclass): - """ - This adds Fields for parents and children. - """ - - parent = Reference(help='The id of the parent of this XBlock', default=None, scope=Scope.parent) - - def __init__(self, **kwargs): - # A cache of the parent block, retrieved from .parent - self._parent_block = None - self._parent_block_id = None - self._child_cache = {} - - for_parent = kwargs.pop('for_parent', None) - - if for_parent is not None: - self._parent_block = for_parent - self._parent_block_id = for_parent.scope_ids.usage_id - - super().__init__(**kwargs) - - def get_parent(self): - """Return the parent block of this block, or None if there isn't one.""" - if not self.has_cached_parent: - if self.parent is not None: - self._parent_block = self.runtime.get_block(self.parent) - else: - self._parent_block = None - self._parent_block_id = self.parent - return self._parent_block - - @property - def has_cached_parent(self): - """Return whether this block has a cached parent block.""" - return self.parent is not None and self._parent_block_id == self.parent - - def get_child(self, usage_id): - """Return the child identified by ``usage_id``.""" - if usage_id in self._child_cache: - return self._child_cache[usage_id] - - child_block = self.runtime.get_block(usage_id, for_parent=self) - self._child_cache[usage_id] = child_block - return child_block - - def get_children(self, usage_id_filter=None): - """ - Return instantiated XBlocks for each of this blocks ``children``. - """ - if not self.has_children: - return [] - - return [ - self.get_child(usage_id) - for usage_id in self.children - if usage_id_filter is None or usage_id_filter(usage_id) - ] - - def clear_child_cache(self): - """ - Reset the cache of children stored on this XBlock. - """ - self._child_cache.clear() - - def add_children_to_node(self, node): - """ - Add children to etree.Element `node`. - """ - if self.has_children: - for child_id in self.children: - child = self.runtime.get_block(child_id) - self.runtime.add_block_as_child_node(child, node) - - -class XmlSerializationMixin(ScopedStorageMixin): - """ - A mixin that provides XML serialization and deserialization on top of ScopedStorage. - """ - - @classmethod - def parse_xml(cls, node, runtime, keys, id_generator): - """ - Use `node` to construct a new block. - - Arguments: - node (:class:`~xml.etree.ElementTree.Element`): The xml node to parse into an xblock. - - runtime (:class:`.Runtime`): The runtime to use while parsing. - - keys (:class:`.ScopeIds`): The keys identifying where this block - will store its data. - - id_generator (:class:`.IdGenerator`): An object that will allow the - runtime to generate correct definition and usage ids for - children of this block. - - """ - block = runtime.construct_xblock_from_class(cls, keys) - - # The base implementation: child nodes become child blocks. - # Or fields, if they belong to the right namespace. - for child in node: - if child.tag is etree.Comment: - continue - qname = etree.QName(child) - tag = qname.localname - namespace = qname.namespace - - if namespace == XML_NAMESPACES["option"]: - cls._set_field_if_present(block, tag, child.text, child.attrib) - else: - block.runtime.add_node_as_child(block, child, id_generator) - - # Attributes become fields. - for name, value in list(node.items()): # lxml has no iteritems - cls._set_field_if_present(block, name, value, {}) - - # Text content becomes "content", if such a field exists. - if "content" in block.fields and block.fields["content"].scope == Scope.content: - text = node.text - if text: - text = text.strip() - if text: - block.content = text - - return block - - def add_xml_to_node(self, node): - """ - For exporting, set data on `node` from ourselves. - """ - # pylint: disable=E1101 - # Set node.tag based on our class name. - node.tag = self.xml_element_name() - node.set('xblock-family', self.entry_point) - - # Set node attributes based on our fields. - for field_name, field in list(self.fields.items()): - if field_name in ('children', 'parent', 'content'): - continue - if field.is_set_on(self) or field.force_export: - self._add_field(node, field_name, field) - - # A content field becomes text content. - text = self.xml_text_content() - if text is not None: - node.text = text - - def xml_element_name(self): - """What XML element name should be used for this block?""" - return self.scope_ids.block_type - - def xml_text_content(self): - """What is the text content for this block's XML node?""" - if 'content' in self.fields and self.content: - return self.content - else: - return None - - @classmethod - def _set_field_if_present(cls, block, name, value, attrs): - """Sets the field block.name, if block have such a field.""" - if name in block.fields: - value = (block.fields[name]).from_string(value) - if "none" in attrs and attrs["none"] == "true": - setattr(block, name, None) - else: - setattr(block, name, value) - else: - logging.warning("XBlock %s does not contain field %s", type(block), name) - - def _add_field(self, node, field_name, field): - """ - Add xml representation of field to node. - - Depending on settings, it either stores the value of field - as an xml attribute or creates a separate child node. - """ - value = field.to_string(field.read_from(self)) - text_value = "" if value is None else value - - # Is the field type supposed to serialize the fact that the value is None to XML? - save_none_as_xml_attr = field.none_to_xml and value is None - field_attrs = {"none": "true"} if save_none_as_xml_attr else {} - - if save_none_as_xml_attr or field.xml_node: - # Field will be output to XML as an separate element. - tag = etree.QName(XML_NAMESPACES["option"], field_name) - elem = etree.SubElement(node, tag, field_attrs) - if field.xml_node: - # Only set the value if forced via xml_node; - # in all other cases, the value is None. - # Avoids an unnecessary XML end tag. - elem.text = text_value - else: - # Field will be output to XML as an attribute on the node. - node.set(field_name, text_value) - - -class IndexInfoMixin: - """ - This mixin provides interface for classes that wish to provide index - information which might be used within a search index - """ - - def index_dictionary(self): - """ - return key/value fields to feed an index within in a Python dict object - values may be numeric / string or dict - """ - display_name = getattr(self, "display_name", None) - - # Getting self.display_name.default wouldn't work as self.display_name is actually - # a str after the class instance is created. So, we can only access the default value - # of display_name field by accessing class variable of same name - content_type = getattr( - getattr(self.__class__, "display_name", None), "default", None - ) - - _index_dictionary = {} - - if display_name is not None: - _index_dictionary.update({ - "content": { - "display_name": display_name - } - }) - - if content_type is not None: - _index_dictionary.update({ - "content_type": content_type - }) - - return _index_dictionary - - -class ViewsMixin: - """ - This mixin provides decorators that can be used on xBlock view methods. - """ - @classmethod - def supports(cls, *functionalities): - """ - A view decorator to indicate that an xBlock view has support for the - given functionalities. - - Arguments: - functionalities: String identifiers for the functionalities of the view. - For example: "multi_device". - """ - def _decorator(view): - """ - Internal decorator that updates the given view's list of supported - functionalities. - """ - # pylint: disable=protected-access - if not hasattr(view, "_supports"): - view._supports = set() - for functionality in functionalities: - view._supports.add(functionality) - return view - return _decorator - - def has_support(self, view, functionality): - """ - Returns whether the given view has support for the given functionality. - - An XBlock view declares support for a functionality with the - @XBlock.supports decorator. The decorator stores information on the view. - - Note: We implement this as an instance method to allow xBlocks to - override it, if necessary. - - Arguments: - view (object): The view of the xBlock. - functionality (str): A functionality of the view. - For example: "multi_device". - - Returns: - True or False - """ - return hasattr(view, "_supports") and functionality in view._supports # pylint: disable=protected-access diff --git a/xblock/test/test_core.py b/xblock/test/test_core.py index 7f7cd4700..cf681858e 100644 --- a/xblock/test/test_core.py +++ b/xblock/test/test_core.py @@ -16,7 +16,7 @@ from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryLocatorV2 from webob import Response -from xblock.core import XBlock +from xblock.core import Blocklike, XBlock from xblock.exceptions import ( XBlockSaveError, KeyValueMultiSaveError, @@ -26,7 +26,6 @@ ) from xblock.fields import Dict, Float, Integer, List, Set, Field, Scope, ScopeIds from xblock.field_data import FieldData, DictFieldData -from xblock.mixins import ScopedStorageMixin from xblock.runtime import Runtime from xblock.test.tools import ( @@ -386,8 +385,8 @@ def to_json(self, value): """Convert a datetime object to a string""" return value.strftime("%m/%d/%Y") - class FieldTester(ScopedStorageMixin): - """Toy class for ModelMetaclass and field access testing""" + class FieldTester(Blocklike): + """Toy class for field access testing""" field_a = Date(scope=Scope.settings) field_b = Date(scope=Scope.content, default=datetime(2013, 4, 1)) @@ -435,7 +434,7 @@ class FieldTester(XBlock): def test_object_identity(): # Check that values that are modified are what is returned - class FieldTester(ScopedStorageMixin): + class FieldTester(Blocklike): """Toy class for ModelMetaclass and field access testing""" field_a = List(scope=Scope.settings) @@ -475,7 +474,7 @@ def mock_default(block, name): def test_caching_is_per_instance(): # Test that values cached for one instance do not appear on another - class FieldTester(ScopedStorageMixin): + class FieldTester(Blocklike): """Toy class for ModelMetaclass and field access testing""" field_a = List(scope=Scope.settings) diff --git a/xblock/test/test_mixins.py b/xblock/test/test_core_capabilities.py similarity index 83% rename from xblock/test/test_mixins.py rename to xblock/test/test_core_capabilities.py index d04c34399..2c4ac443c 100644 --- a/xblock/test/test_mixins.py +++ b/xblock/test/test_core_capabilities.py @@ -1,5 +1,12 @@ """ -Tests of the XBlock-family functionality mixins +Tests for various capabilities of Blocklikes. + +This is separate from test_core because these capabilities were originally +provided by a varity of mixins to XBlock and XBlockAside. Those mixins were +mostly collapsed into Blocklike because they were hard to reason about and +unfriendly to type annotation. However, it led to pretty good organization +for testing various block capabilities, so we've left the tests separated +like this. """ from datetime import datetime from unittest import TestCase @@ -9,10 +16,9 @@ import pytz from lxml import etree -from xblock.core import XBlock, XBlockAside +from xblock.core import Blocklike, XBlock, XBlockAside, XML_NAMESPACES from xblock.fields import List, Scope, Integer, String, ScopeIds, UNIQUE_ID, DateTime from xblock.field_data import DictFieldData -from xblock.mixins import ScopedStorageMixin, HierarchyMixin, IndexInfoMixin, ViewsMixin, XML_NAMESPACES from xblock.runtime import Runtime from xblock.test.tools import TestRuntime @@ -30,40 +36,40 @@ def assertNotHasAttr(self, obj, attr): self.assertFalse(hasattr(obj, attr), f"{obj!r} has attribute {attr!r}") -class TestScopedStorageMixin(AttrAssertionMixin, TestCase): - "Tests of the ScopedStorageMixin." +class TestScopedStorage(AttrAssertionMixin, TestCase): + """Tests of the scoped storage capaility of Blocklikes.""" - class ScopedStorageMixinTester(ScopedStorageMixin): - """Toy class for ScopedStorageMixin testing""" + # pylint doesn't understand that @class_lazy fields is a dict. + # pylint: disable=unsubscriptable-object + + class ScopedStorageTester(Blocklike): + """Toy class for scoped storage testing""" field_a = Integer(scope=Scope.settings) field_b = Integer(scope=Scope.content) - class ChildClass(ScopedStorageMixinTester): - """Toy class for ModelMetaclass testing""" + class ChildClass(ScopedStorageTester): + """Toy class for scoped storage testing""" class FieldsMixin: """Toy mixin for field testing""" field_c = Integer(scope=Scope.settings) - class MixinChildClass(FieldsMixin, ScopedStorageMixinTester): - """Toy class for ScopedStorageMixin testing with mixed-in fields""" + class MixinChildClass(FieldsMixin, ScopedStorageTester): + """Toy class for scoped storage testing with mixed-in fields""" class MixinGrandchildClass(MixinChildClass): - """Toy class for ScopedStorageMixin testing with inherited mixed-in fields""" + """Toy class for scoped storage testing with inherited mixed-in fields""" - def test_scoped_storage_mixin(self): + def test_scoped_storage(self): - # `ModelMetaclassTester` and `ChildClass` both obtain the `fields` attribute - # from the `ModelMetaclass`. Since this is not understood by static analysis, - # silence this error for the duration of this test. - self.assertIsNot(self.ScopedStorageMixinTester.fields, self.ChildClass.fields) + self.assertIsNot(self.ScopedStorageTester.fields, self.ChildClass.fields) - self.assertHasAttr(self.ScopedStorageMixinTester, 'field_a') - self.assertHasAttr(self.ScopedStorageMixinTester, 'field_b') + self.assertHasAttr(self.ScopedStorageTester, 'field_a') + self.assertHasAttr(self.ScopedStorageTester, 'field_b') - self.assertIs(self.ScopedStorageMixinTester.field_a, self.ScopedStorageMixinTester.fields['field_a']) - self.assertIs(self.ScopedStorageMixinTester.field_b, self.ScopedStorageMixinTester.fields['field_b']) + self.assertIs(self.ScopedStorageTester.field_a, self.ScopedStorageTester.fields['field_a']) + self.assertIs(self.ScopedStorageTester.field_b, self.ScopedStorageTester.fields['field_b']) self.assertHasAttr(self.ChildClass, 'field_a') self.assertHasAttr(self.ChildClass, 'field_b') @@ -74,10 +80,6 @@ def test_scoped_storage_mixin(self): def test_with_mixins(self): # Testing model metaclass with mixins - # `MixinChildClass` and `MixinGrandchildClass` both obtain the `fields` attribute - # from the `ScopedStorageMixin`. Since this is not understood by static analysis, - # silence this error for the duration of this test. - self.assertHasAttr(self.MixinChildClass, 'field_a') self.assertHasAttr(self.MixinChildClass, 'field_c') self.assertIs(self.MixinChildClass.field_a, self.MixinChildClass.fields['field_a']) @@ -90,7 +92,7 @@ def test_with_mixins(self): def test_save_calls_runtime_save_block(self): """ - Make sure that when block.save() is called, ScopedStorageMixin will let + Make sure that when block.save() is called, it lets the runtime know that the entire block should be saved. """ class SaveTestBlock(XBlock): @@ -106,25 +108,20 @@ class SaveTestBlock(XBlock): runtime.save_block.assert_called_once_with(block) -class TestHierarchyMixin(AttrAssertionMixin, TestCase): - "Tests of the HierarchyMixin." +class TestHierarchy(AttrAssertionMixin, TestCase): + """Tests of the hierarchy capability of XBlocks.""" - class HasChildren(HierarchyMixin): - """Toy class for ChildrenModelMetaclass testing""" + class HasChildren(XBlock): + """Toy class for hierarchy testing""" has_children = True - class WithoutChildren(HierarchyMixin): - """Toy class for ChildrenModelMetaclass testing""" + class WithoutChildren(XBlock): + """Toy class for hierarchy testing""" - class InheritedChildren(HasChildren): - """Toy class for ChildrenModelMetaclass testing""" + class InheritedChildren(XBlock): + """Toy class for hierarchy testing""" def test_children_metaclass(self): - # `HasChildren` and `WithoutChildren` both obtain the `children` attribute and - # the `has_children` method from the `ChildrenModelMetaclass`. Since this is not - # understood by static analysis, silence this error for the duration of this test. - # pylint: disable=E1101 - self.assertTrue(self.HasChildren.has_children) self.assertFalse(self.WithoutChildren.has_children) self.assertTrue(self.InheritedChildren.has_children) @@ -139,40 +136,39 @@ def test_children_metaclass(self): self.assertEqual(Scope.children, self.InheritedChildren.children.scope) -class TestIndexInfoMixin(AttrAssertionMixin): - """ - Tests for Index - """ - class IndexInfoMixinTester(IndexInfoMixin): - """Test class for index mixin""" +class TestIndexInfo(AttrAssertionMixin): + """Tests for search index information.""" + + class IndexInfoMixinTester(Blocklike): + """Test class for index info capability of Blocklikes""" def test_index_info(self): self.assertHasAttr(self.IndexInfoMixinTester, 'index_dictionary') - with_index_info = self.IndexInfoMixinTester().index_dictionary() + with_index_info = self.IndexInfoMixinTester(runtime=None, scope_ids=None).index_dictionary() self.assertFalse(with_index_info) self.assertTrue(isinstance(with_index_info, dict)) -class TestViewsMixin(TestCase): +class TestViews(TestCase): """ - Tests for ViewsMixin + Tests for the views capability of XBlocks. """ def test_supports_view_decorator(self): """ Tests the @supports decorator for xBlock view methods """ - class SupportsDecoratorTester(ViewsMixin): + class SupportsDecoratorTester(XBlock): """ Test class for @supports decorator """ - @ViewsMixin.supports("a_functionality") + @XBlock.supports("a_functionality") def functionality_supported_view(self): """ A view that supports a functionality """ # pragma: no cover - @ViewsMixin.supports("functionality1", "functionality2") + @XBlock.supports("functionality1", "functionality2") def multi_featured_view(self): """ A view that supports multiple functionalities @@ -185,7 +181,7 @@ def an_unsupported_view(self): """ # pragma: no cover - test_xblock = SupportsDecoratorTester() + test_xblock = SupportsDecoratorTester(runtime=None) for view_name, functionality, expected_result in ( ("functionality_supported_view", "a_functionality", True), @@ -207,7 +203,7 @@ def test_has_support_override(self): """ Tests overriding has_support """ - class HasSupportOverrideTester(ViewsMixin): + class HasSupportOverrideTester(XBlock): """ Test class for overriding has_support """ @@ -217,7 +213,8 @@ def has_support(self, view, functionality): """ return functionality == "a_functionality" - test_xblock = HasSupportOverrideTester() + test_xblock = HasSupportOverrideTester(runtime=None) + for view_name, functionality, expected_result in ( ("functionality_supported_view", "a_functionality", True), @@ -230,8 +227,11 @@ def has_support(self, view, functionality): @ddt.ddt -class TestXmlSerializationMixin(TestCase): - """ Tests for XmlSerialization Mixin """ +class TestXmlSerialization(TestCase): + """ Tests for XML (de)serialization capability of Blocklikes """ + + # pylint doesn't understand that @class_lazy fields is a dict. + # pylint: disable=unsubscriptable-object class TestXBlock(XBlock): """ XBlock for XML export test """ diff --git a/xblock/test/test_internal.py b/xblock/test/test_internal.py index 24de5e3c7..79be61ba6 100644 --- a/xblock/test/test_internal.py +++ b/xblock/test/test_internal.py @@ -1,7 +1,7 @@ """Tests of the xblock.internal module.""" from unittest import TestCase -from xblock.internal import class_lazy, NamedAttributesMetaclass, Nameable +from xblock.internal import class_lazy class TestLazyClassProperty(TestCase): @@ -22,61 +22,3 @@ def test_isolation(self): self.assertEqual({}, self.Base.isolated_dict) self.assertEqual({}, self.Derived.isolated_dict) self.assertIsNot(self.Base.isolated_dict, self.Derived.isolated_dict) - - -class TestDescriptor(Nameable): - """Descriptor that returns itself for introspection in tests.""" - def __get__(self, instance, owner): - return self - - -class TestGetSetDescriptor(Nameable): - """Descriptor that returns itself for introspection in tests.""" - def __get__(self, instance, owner): - return self - - def __set__(self, instance, value): - pass - - -class NamingTester(metaclass=NamedAttributesMetaclass): - """Class with several descriptors that should get names.""" - - test_descriptor = TestDescriptor() - test_getset_descriptor = TestGetSetDescriptor() - test_nonnameable = object() - - def meth(self): - "An empty method." - - @property - def prop(self): - "An empty property." - - -class InheritedNamingTester(NamingTester): - """Class with several inherited descriptors that should get names.""" - inherited = TestDescriptor() - - -class TestNamedDescriptorsMetaclass(TestCase): - "Tests of the NamedDescriptorsMetaclass." - - def test_named_descriptor(self): - self.assertEqual('test_descriptor', NamingTester.test_descriptor.__name__) - - def test_named_getset_descriptor(self): - self.assertEqual('test_getset_descriptor', NamingTester.test_getset_descriptor.__name__) - - def test_inherited_naming(self): - self.assertEqual('test_descriptor', InheritedNamingTester.test_descriptor.__name__) - self.assertEqual('inherited', InheritedNamingTester.inherited.__name__) - - def test_unnamed_attribute(self): - self.assertFalse(hasattr(NamingTester.test_nonnameable, '__name__')) - - def test_method(self): - self.assertEqual('meth', NamingTester.meth.__name__) - - def test_prop(self): - self.assertFalse(hasattr(NamingTester.prop, '__name__'))