From f79bfc515df010f08b85f8e0ad615e1922d68f80 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Tue, 19 Mar 2024 14:52:36 -0400 Subject: [PATCH] feat!: collapse extraneous XBlock mixins (#718) Various extraneous classes have been removed from the XBlock API, simplifying its implementation and making debugging of XBlock instances easier. We believe that most, if not all, XBlock API users will be unaffected by this change. Some improvements have also been made to the reference documentation. See CHANGELOG.rst for details. Closes: https://github.com/openedx/XBlock/issues/714 --- CHANGELOG.rst | 55 +- docs/conf.py | 1 + xblock/__init__.py | 27 +- xblock/core.py | 859 ++++++++++++++++-- xblock/django/request.py | 50 - xblock/exceptions.py | 4 +- xblock/fields.py | 13 +- xblock/internal.py | 40 - xblock/mixins.py | 625 ------------- xblock/runtime.py | 6 +- xblock/test/test_core.py | 62 +- ...st_mixins.py => test_core_capabilities.py} | 111 ++- xblock/test/test_internal.py | 60 +- xblock/test/test_parsing.py | 4 +- 14 files changed, 929 insertions(+), 988 deletions(-) delete mode 100644 xblock/mixins.py rename xblock/test/{test_mixins.py => test_core_capabilities.py} (82%) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 162b95886..c91ccab58 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,11 +2,62 @@ Change history for XBlock ========================= -These are notable changes in XBlock. - Unreleased ---------- + +3.0.0 - 2024-03-18 +------------------ + +Various extraneous classes have been removed from the XBlock API, simplifying its implementation +and making debugging of XBlock instances easier. We believe that most, if not all, XBlock API users +will be unaffected by this change. Some improvements have also been made to the reference documentation. + +Specific changes: + +* **Removed:** + + * ``xblock.XBlockMixin`` (still available as ``xblock.core.XBlockMixin``) + * ``xblock.core.SharedBlockBase`` (replaced with ``xblock.core.Blocklike``) + * ``xblock.internal.Nameable`` + * ``xblock.internal.NamedAttributesMetaclass`` + * ``xblock.django.request.HeadersDict`` + * ``xblock.fields.XBlockMixin`` (still available as ``xblock.core.XBlockMixin``) + * ``xblock.mixins.RuntimeServicesMixin`` + * ``xblock.mixins.ScopedStorageMixin`` + * ``xblock.mixins.IndexInfoMixin`` + * ``xblock.mixins.XmlSerializationMixin`` + * ``xblock.mixins.HandlersMixin`` + * ``xblock.mixins.ChildrenModelMetaclass`` + * ``xblock.mixins.HierarchyMixin`` + * ``xblock.mixins.ViewsMixin`` + +* **Added:** + + * ``xblock.core.Blocklike``, the new common ancestor of ``XBlock`` and ``XBlockAside``, and ``XBlockMixin``, + replacing ``xblock.core.SharedBlockBase``. + + * New attributes on ``xblock.core.XBlockAside``, each behaving the same as their ``XBlock`` counterpart: + + * ``usage_key`` + * ``context_key`` + * ``index_dictionary`` + + * Various new attributes on ``xblock.core.XBlockMixin``, encompassing the functionality of these former classes: + + * ``xblock.mixins.IndexInfoMixin`` + * ``xblock.mixins.XmlSerializationMixin`` + * ``xblock.mixins.HandlersMixin`` + +* **Docs** + + * Various docstrings have been improved, some of which are published in the docs. + * XBlockAside will now be represented in the API docs, right below XBlock on the "XBlock API" page. + * XBlockMixin has been removed from the docs. + It was only ever documented under the "Fields API" page (which didn't make any sense), + and it was barely even documented there. We considered adding it back to the "XBlock API" page, + but as noted in the class's new docstring, we do not want to encourage any new use of XBlockMixin. + 2.0.0 - 2024-02-26 ------------------ 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/xblock/__init__.py b/xblock/__init__.py index 82fc398f6..ec599798f 100644 --- a/xblock/__init__.py +++ b/xblock/__init__.py @@ -2,29 +2,4 @@ XBlock Courseware Components """ -# For backwards compatibility, provide the XBlockMixin in xblock.fields -# without causing a circular import -import codecs -import os -import warnings - -import xblock.core -import xblock.fields - - -class XBlockMixin(xblock.core.XBlockMixin): - """ - A wrapper around xblock.core.XBlockMixin that provides backwards compatibility for the old location. - - Deprecated. - """ - def __init__(self, *args, **kwargs): - warnings.warn("Please use xblock.core.XBlockMixin", DeprecationWarning, stacklevel=2) - super().__init__(*args, **kwargs) - - -# For backwards compatibility, provide the XBlockMixin in xblock.fields -# without causing a circular import -xblock.fields.XBlockMixin = XBlockMixin - -__version__ = '2.0.0' +__version__ = '3.0.0' diff --git a/xblock/core.py b/xblock/core.py index 2c93195ac..fcea0f597 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -1,58 +1,96 @@ """ -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__ = ['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 XBlocks and XBlockAsides, providing these common capabilities: + + - services + - fields + - OLX + - handlers + - resources + (see XBlock and XBlockAside classes for details) + """ resources_dir = '' public_dir = 'public' i18n_js_namespace = None @@ -60,24 +98,24 @@ class SharedBlockBase(Plugin): @classmethod def get_resources_dir(cls): """ - Gets the resource directory for this XBlock. + Gets the resource directory for this XBlock-like class. """ return cls.resources_dir @classmethod def get_public_dir(cls): """ - Gets the public directory for this XBlock. + Gets the public directory for this XBlock-like class. """ 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 XBlock-like class. Returns: - str: The JavaScript namespace for this XBlock. + str: The JavaScript namespace for this XBlock-like class. None: If this doesn't have JavaScript translations configured. """ return cls.i18n_js_namespace @@ -90,13 +128,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 XBlock-like 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-XBlock-like basis. XBlock-like 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 +159,529 @@ 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 an XBlock-like 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 XBlock-like 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. + + XBlock-like classes 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 {} + + @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 parse_xml(cls, node, runtime, keys): + """ + 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. + """ + 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) + + # 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 + + @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("%s does not contain field %s", type(block), name) + + def __init__(self, scope_ids, field_data=None, *, runtime, **kwargs): + """ + Arguments: + + scope_ids (:class:`.ScopeIds`): Identifiers needed to resolve + scopes. + + field_data (:class:`.FieldData`): Interface used by XBlock-likes' + fields to access their data from wherever it is persisted. + DEPRECATED--supply a field-data Runtime service instead. + + runtime (:class:`.Runtime`): Use it to access the environment. + It is available in XBlock code as ``self.runtime``. + + """ + 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(): + 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 XBlock-like, 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-like 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]) + + 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 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) -# -- Base Block -class XBlock(XmlSerializationMixin, HierarchyMixin, ScopedStorageMixin, RuntimeServicesMixin, HandlersMixin, - IndexInfoMixin, ViewsMixin, SharedBlockBase): - """Base class for XBlocks. - 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. +# All Blocklike objects use the field-data service. +Blocklike.needs('field-data')(Blocklike) - Don't provide the ``__init__`` method when deriving from this class. +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. + """ + + +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. Derive from this class to create new type of XBlock. + + Subclasses of XBlocks can: + + - Name one or more **views**, i.e. methods which render the block to HTML. + - Access the **parents** of their instances. + - Access and manage the **children** of their instances. + - Request **services** from the runtime, for their instances to use. + - Define scoped **fields**, which instances will use to store content, settings, and data. + - Define how instances are serialized to and deserialized from **OLX** (Open Learning XML). + - Mark methods as **handlers** for AJAX requests. + - Be installed into a platform as an entry-point **plugin**. + + Note: 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 +696,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 +720,62 @@ 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): """ - Construct a new XBlock. - - This class should only be instantiated by runtimes. + 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. + """ + 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) + + # 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 __init__( + self, + runtime, + field_data=None, + scope_ids=UNSET, + *args, # pylint: disable=keyword-arg-before-vararg + **kwargs + ): + """ Arguments: runtime (:class:`.Runtime`): Use it to access the environment. @@ -202,34 +791,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 + 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 - @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. - - 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 +834,114 @@ 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) + ] + + 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. -class XBlockAside(XmlSerializationMixin, ScopedStorageMixin, RuntimeServicesMixin, HandlersMixin, SharedBlockBase): + 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 for XBlock-like objects that are rendered alongside :class:`.XBlock` views. + + Subclasses of XBlockAside can: + + - Specify which XBlock views they are to be **injected** into. + - Request **services** from the runtime, for their instances to use. + - Define scoped **fields**, which instances will use to store content, settings, and data. + - Define how instances are serialized to and deserialized from **OLX** (Open Learning XML). + - Mark methods as **handlers** for AJAX requests. + - Be installed into a platform as an entry-point **plugin**. """ entry_point = "xblock_asides.v1" @@ -339,22 +1016,4 @@ def needs_serialization(self): If all of the aside's data is empty or a default value, then the aside shouldn't 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) + return any(field.is_set_on(self) for field in self.fields.values()) diff --git a/xblock/django/request.py b/xblock/django/request.py index 9e17e6721..01829a7a8 100644 --- a/xblock/django/request.py +++ b/xblock/django/request.py @@ -1,5 +1,4 @@ """Helpers for WebOb requests and responses.""" -from collections.abc import MutableMapping from itertools import chain, repeat from lazy import lazy @@ -20,55 +19,6 @@ def webob_to_django_response(webob_response): return django_response -class HeaderDict(MutableMapping): - """ - Provide a dictionary view of the HTTP headers in a - Django request.META dictionary that translates the - keys into actually HTTP header names - """ - UNPREFIXED_HEADERS = ('CONTENT_TYPE', 'CONTENT_LENGTH') - - def __init__(self, meta): - super().__init__() - self._meta = meta - - def _meta_name(self, name): - """ - Translate HTTP header names to the format used by Django request objects. - - See https://docs.djangoproject.com/en/1.4/ref/request-response/#django.http.HttpRequest.META - """ - name = name.upper().replace('-', '_') - if name not in self.UNPREFIXED_HEADERS: - name = 'HTTP_' + name - return name - - def _un_meta_name(self, name): - """ - Reverse of _meta_name - """ - if name.startswith('HTTP_'): - name = name[5:] - return name.replace('_', '-').title() - - def __getitem__(self, name): - return self._meta[self._meta_name(name)] - - def __setitem__(self, name, value): - self._meta[self._meta_name(name)] = value - - def __delitem__(self, name): - del self._meta[self._meta_name(name)] - - def __iter__(self): - for key in self._meta: - if key in self.UNPREFIXED_HEADERS or key.startswith('HTTP_'): - yield self._un_meta_name(key) - - def __len__(self): - return len(list(self)) - - def querydict_to_multidict(query_dict, wrap=None): """ Returns a new `webob.MultiDict` from a `django.http.QueryDict`. diff --git a/xblock/exceptions.py b/xblock/exceptions.py index 09742328b..7f50b7e4e 100644 --- a/xblock/exceptions.py +++ b/xblock/exceptions.py @@ -131,7 +131,9 @@ def get_response(self, **kwargs): class DisallowedFileError(Exception): - """Raised by :meth:`.XBlock.open_local_resource` if the requested file is not allowed.""" + """ + Raised by :meth:`.XBlock.open_local_resource` or :meth:`.XBlockAside.open_local_resource`. + """ class FieldDataDeprecationWarning(DeprecationWarning): diff --git a/xblock/fields.py b/xblock/fields.py index 8f5eecc8a..018944c21 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -20,14 +20,12 @@ import pytz import yaml -from xblock.internal import Nameable # __all__ controls what classes end up in the docs, and in what order. __all__ = [ 'BlockScope', 'UserScope', 'Scope', 'ScopeIds', 'Field', 'Boolean', 'Dict', 'Float', 'Integer', 'List', 'Set', 'String', 'XMLString', - 'XBlockMixin', ] @@ -263,7 +261,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 +310,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 +339,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 bdb473455..000000000 --- a/xblock/mixins.py +++ /dev/null @@ -1,625 +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): - """ - 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. - - """ - 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) - - # 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/runtime.py b/xblock/runtime.py index 5ca1cabcc..7f9f12385 100644 --- a/xblock/runtime.py +++ b/xblock/runtime.py @@ -541,9 +541,9 @@ def __init__( default_class (class): The default class to use if a class can't be found for a particular `block_type` when loading an :class:`.XBlock`. - select: A function to select from one or more :class:`.XBlock` subtypes found - when calling :meth:`.XBlock.load_class` to resolve a `block_type`. - This is the same `select` as used by :meth:`.Plugin.load_class`. + select: A function to select from one or more XBlock-like subtypes found + when calling :meth:`.XBlock.load_class` or :meth:`.XBlockAside.load_class` + to resolve a `block_type`. """ self.id_reader = id_reader diff --git a/xblock/test/test_core.py b/xblock/test/test_core.py index ea35a6d9f..c6a6fed10 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, @@ -24,9 +24,8 @@ DisallowedFileError, FieldDataDeprecationWarning, ) -from xblock.fields import Dict, Float, Integer, List, Set, Field, Scope, ScopeIds +from xblock.fields import Dict, Float, Integer, List, Set, Field, Scope, ScopeIds, String 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,8 +434,8 @@ class FieldTester(XBlock): def test_object_identity(): # Check that values that are modified are what is returned - class FieldTester(ScopedStorageMixin): - """Toy class for ModelMetaclass and field access testing""" + class FieldTester(Blocklike): + """Toy class for field access testing""" field_a = List(scope=Scope.settings) def mock_default(block, name): @@ -475,8 +474,8 @@ 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): - """Toy class for ModelMetaclass and field access testing""" + class FieldTester(Blocklike): + """Toy class for field access testing""" field_a = List(scope=Scope.settings) field_data = MagicMock(spec=FieldData) @@ -777,17 +776,20 @@ def test_handle_shortcut(): def test_services_decorators(): - # A default XBlock has requested no services - xblock = XBlock(None, None, None) - assert not XBlock._services_requested - assert not xblock._services_requested + + class NoServicesBlock(XBlock): + """XBlock requesting no services""" + + no_services_block = NoServicesBlock(None, None, None) + assert not NoServicesBlock._services_requested + assert not no_services_block._services_requested @XBlock.needs("n") @XBlock.wants("w") class ServiceUsingBlock(XBlock): """XBlock using some services.""" - service_using_block = ServiceUsingBlock(None, scope_ids=None) + service_using_block = ServiceUsingBlock(None, scope_ids=Mock()) assert ServiceUsingBlock._services_requested == { 'n': 'need', 'w': 'want' } @@ -807,11 +809,12 @@ class ServiceUsingBlock(XBlock): class SubServiceUsingBlock(ServiceUsingBlock): """Does this class properly inherit services from ServiceUsingBlock?""" - sub_service_using_block = SubServiceUsingBlock(None, scope_ids=None) + sub_service_using_block = SubServiceUsingBlock(None, scope_ids=Mock()) assert sub_service_using_block.service_declaration("n1") == "need" assert sub_service_using_block.service_declaration("w1") == "want" assert sub_service_using_block.service_declaration("n2") == "need" assert sub_service_using_block.service_declaration("w2") == "want" + assert sub_service_using_block.service_declaration("field-data") == "need" assert sub_service_using_block.service_declaration("xx") is None @@ -972,7 +975,7 @@ def stub_resource_stream(self, module, name): "public/\N{SNOWMAN}.js", ) def test_open_good_local_resource(self, uri): - loadable = self.LoadableXBlock(None, scope_ids=None) + loadable = self.LoadableXBlock(None, scope_ids=Mock()) with patch('pkg_resources.resource_stream', self.stub_resource_stream): assert loadable.open_local_resource(uri) == "!" + uri + "!" assert loadable.open_local_resource(uri.encode('utf-8')) == "!" + uri + "!" @@ -986,7 +989,7 @@ def test_open_good_local_resource(self, uri): "public/\N{SNOWMAN}.js".encode(), ) def test_open_good_local_resource_binary(self, uri): - loadable = self.LoadableXBlock(None, scope_ids=None) + loadable = self.LoadableXBlock(None, scope_ids=Mock()) with patch('pkg_resources.resource_stream', self.stub_resource_stream): assert loadable.open_local_resource(uri) == "!" + uri.decode('utf-8') + "!" @@ -1000,7 +1003,7 @@ def test_open_good_local_resource_binary(self, uri): "static/\N{SNOWMAN}.js", ) def test_open_bad_local_resource(self, uri): - loadable = self.LoadableXBlock(None, scope_ids=None) + loadable = self.LoadableXBlock(None, scope_ids=Mock()) with patch('pkg_resources.resource_stream', self.stub_resource_stream): msg_pattern = ".*: %s" % re.escape(repr(uri)) with pytest.raises(DisallowedFileError, match=msg_pattern): @@ -1016,7 +1019,7 @@ def test_open_bad_local_resource(self, uri): "static/\N{SNOWMAN}.js".encode(), ) def test_open_bad_local_resource_binary(self, uri): - loadable = self.LoadableXBlock(None, scope_ids=None) + loadable = self.LoadableXBlock(None, scope_ids=Mock()) with patch('pkg_resources.resource_stream', self.stub_resource_stream): msg = ".*: %s" % re.escape(repr(uri.decode('utf-8'))) with pytest.raises(DisallowedFileError, match=msg): @@ -1038,7 +1041,7 @@ def test_open_bad_local_resource_binary(self, uri): "static/\N{SNOWMAN}.js", ) def test_open_local_resource_with_no_resources_dir(self, uri): - unloadable = self.UnloadableXBlock(None, scope_ids=None) + unloadable = self.UnloadableXBlock(None, scope_ids=Mock()) with patch('pkg_resources.resource_stream', self.stub_resource_stream): msg = "not configured to serve local resources" @@ -1078,6 +1081,12 @@ class TestXBlock(XBlock): Class to test default Xblock provides a dictionary """ + class TestXBlockWithDisplayName(XBlock): + """ + Class to test default Xblock provides a dictionary + """ + display_name = String(default="Test Block with Display Name", scope=Scope.settings) + class TestIndexedXBlock(XBlock): """ Class to test when an Xblock provides a dictionary @@ -1089,7 +1098,7 @@ def index_dictionary(self): "text_block": "Here is some text that was indexed", } - def test_default_index_view(self): + def test_default_index_view_with_no_display_name(self): test_runtime = TestRuntime(services={'field-data': DictFieldData({})}) test_xblock = self.TestXBlock(test_runtime, scope_ids=Mock(spec=ScopeIds)) @@ -1097,6 +1106,17 @@ def test_default_index_view(self): self.assertFalse(index_info) self.assertTrue(isinstance(index_info, dict)) + def test_default_index_view_with_display_name(self): + test_runtime = TestRuntime(services={'field-data': DictFieldData({})}) + test_xblock = self.TestXBlockWithDisplayName(test_runtime, scope_ids=Mock(spec=ScopeIds)) + test_xblock.display_name = "My Display Name" + + index_info = test_xblock.index_dictionary() + assert index_info == { + "content": {"display_name": "My Display Name"}, + "content_type": "Test Block with Display Name", + } + def test_override_index_view(self): test_runtime = TestRuntime(services={'field-data': DictFieldData({})}) test_xblock = self.TestIndexedXBlock(test_runtime, scope_ids=Mock(spec=ScopeIds)) diff --git a/xblock/test/test_mixins.py b/xblock/test/test_core_capabilities.py similarity index 82% rename from xblock/test/test_mixins.py rename to xblock/test/test_core_capabilities.py index d04c34399..0919a35ee 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""" + """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(None, None, 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,7 @@ def has_support(self, view, functionality): """ return functionality == "a_functionality" - test_xblock = HasSupportOverrideTester() + test_xblock = HasSupportOverrideTester(None, None, None) for view_name, functionality, expected_result in ( ("functionality_supported_view", "a_functionality", True), @@ -230,8 +226,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 """ @@ -309,7 +308,7 @@ def test_no_fields_set_add_xml_to_node(self): node = etree.Element(self.test_xblock_tag) # Precondition check: no fields are set. - for field_name in self.test_xblock.fields.keys(): # pylint: disable=no-member + for field_name in self.test_xblock.fields.keys(): self.assertFalse(self.test_xblock.fields[field_name].is_set_on(self.test_xblock)) self.test_xblock.add_xml_to_node(node) 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__')) diff --git a/xblock/test/test_parsing.py b/xblock/test/test_parsing.py index db2447f03..7000baad4 100644 --- a/xblock/test/test_parsing.py +++ b/xblock/test/test_parsing.py @@ -344,7 +344,7 @@ def test_dict_and_list_export_format(self): def test_unknown_field_as_attribute_raises_warning(self, parameter_name): with mock.patch('logging.warning') as patched_warn: block = self.parse_xml_to_block(f"") - patched_warn.assert_called_once_with("XBlock %s does not contain field %s", type(block), parameter_name) + patched_warn.assert_called_once_with("%s does not contain field %s", type(block), parameter_name) @XBlock.register_temp_plugin(LeafWithOption) @ddt.data( @@ -361,7 +361,7 @@ def test_unknown_field_as_node_raises_warning(self, parameter_name): """) % (get_namespace_attrs(), parameter_name, parameter_name) with mock.patch('logging.warning') as patched_warn: block = self.parse_xml_to_block(xml) - patched_warn.assert_called_once_with("XBlock %s does not contain field %s", type(block), parameter_name) + patched_warn.assert_called_once_with("%s does not contain field %s", type(block), parameter_name) class TestRoundTrip(XmlTest, unittest.TestCase):