diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2b6a3b0f4..4822212c7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,16 @@ Change history for XBlock Unreleased ---------- +6.0.0 - 2026-01-20 +------------------ + +* Raise an exception when scope IDs are missing or are not the expected types. In + particular, definition IDs must be DefinitionKey instances and usage IDs must be + UsageKey instances. This has been effectively true within edx-platform (the lone + production client of the XBlock library) for a long time, but explictly + enforcing it will now allow us to add strong type annotations to XBlock in an + upcoming release. + 5.3.0 - 2025-12-19 ------------------ diff --git a/xblock/__init__.py b/xblock/__init__.py index 779a96e05..8f666bd02 100644 --- a/xblock/__init__.py +++ b/xblock/__init__.py @@ -2,4 +2,4 @@ XBlock Courseware Components """ -__version__ = '5.3.0' +__version__ = '6.0.0' diff --git a/xblock/core.py b/xblock/core.py index 7ff362475..171aabcfe 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -20,7 +20,7 @@ KeyValueMultiSaveError, XBlockSaveError, ) -from xblock.fields import Field, List, Reference, ReferenceList, Scope, String +from xblock.fields import Field, List, Reference, ReferenceList, Scope, String, ScopeIds from xblock.internal import class_lazy from xblock.plugin import Plugin from xblock.validation import Validation @@ -393,6 +393,9 @@ def __init__(self, scope_ids, field_data=None, *, runtime, **kwargs): self._field_data_cache = {} self._dirty_fields = {} + if not isinstance(scope_ids, ScopeIds): + raise TypeError(f"got {scope_ids=}; should be a ScopeIds instance") + scope_ids.validate_types() self.scope_ids = scope_ids super().__init__(**kwargs) @@ -780,9 +783,8 @@ def __init__( self, runtime, field_data=None, - scope_ids=UNSET, - *args, # pylint: disable=keyword-arg-before-vararg - **kwargs + scope_ids=None, + **kwargs, ): """ Arguments: @@ -797,9 +799,6 @@ def __init__( scope_ids (:class:`.ScopeIds`): Identifiers needed to resolve scopes. """ - if scope_ids is UNSET: - raise TypeError('scope_ids are required') - # A cache of the parent block, retrieved from .parent self._parent_block = None self._parent_block_id = None @@ -811,7 +810,7 @@ def __init__( self._parent_block_id = for_parent.scope_ids.usage_id # Provide backwards compatibility for external access through _field_data - super().__init__(runtime=runtime, scope_ids=scope_ids, field_data=field_data, *args, **kwargs) + super().__init__(runtime=runtime, scope_ids=scope_ids, field_data=field_data, **kwargs) def render(self, view, context=None): """Render `view` with this block's runtime and the supplied `context`""" diff --git a/xblock/fields.py b/xblock/fields.py index 6ad3ca1bb..951794d3d 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -3,8 +3,9 @@ **scopes** to associate each field with particular sets of blocks and users. The hosting runtime application decides what actual storage mechanism to use for each scope. - """ +from __future__ import annotations + from collections import namedtuple import copy import datetime @@ -17,6 +18,8 @@ import traceback import warnings +from opaque_keys.edx.keys import UsageKey, DefinitionKey + import dateutil.parser from lxml import etree import pytz @@ -250,6 +253,27 @@ class ScopeIds(namedtuple('ScopeIds', 'user_id block_type def_id usage_id')): """ __slots__ = () + def validate_types(self): + """ + Raise an AssertionError if any of the ids are an unexpected type. + + Originally, these fields were all freely-typed; but in practice, + edx-platform's XBlock runtime would fail if the ids did not match the + types below. In order to make the XBlock library reflect the + edx-platform reality and improve type-safety, we've decided to actually + enforce the types here, per: + https://github.com/openedx/XBlock/issues/708 + """ + if self.user_id is not None: + if not isinstance(self.user_id, (int, str)): + raise TypeError(f"got {self.user_id=}; should be an int, str, or None") + if not isinstance(self.block_type, str): + raise TypeError(f"got {self.block_type=}; should be a str") + if not isinstance(self.def_id, DefinitionKey): + raise TypeError(f"got {self.def_id=}; should be a DefinitionKey") + if not isinstance(self.usage_id, UsageKey): + raise TypeError(f"got {self.usage_id=}; should be a UsageKey") + # Define special reference that can be used as a field's default in field # definition to signal that the field should default to a unique string value diff --git a/xblock/runtime.py b/xblock/runtime.py index 8aa822dda..deed19327 100644 --- a/xblock/runtime.py +++ b/xblock/runtime.py @@ -1,6 +1,8 @@ """ Machinery to make the common case easy when building new runtimes """ +from __future__ import annotations + from abc import ABCMeta, abstractmethod from collections import namedtuple import functools @@ -12,6 +14,7 @@ import logging import re import threading +import typing as t import warnings from lxml import etree @@ -19,6 +22,8 @@ from web_fragments.fragment import Fragment +from opaque_keys.edx.keys import UsageKey, DefinitionKey, LearningContextKey, CourseKey +from opaque_keys.edx.asides import AsideDefinitionKeyV2, AsideUsageKeyV2 from xblock.core import XBlock, XBlockAside, XML_NAMESPACES from xblock.fields import Field, BlockScope, Scope, ScopeIds, UserScope from xblock.field_data import FieldData @@ -357,78 +362,148 @@ def create_definition(self, block_type, slug=None): raise NotImplementedError() +class _InMemoryDefinitionKey(DefinitionKey): + """ + A simple definition key: md::. NOT part of the public XBlock API. + """ + CANONICAL_NAMESPACE = 'md' # "(In-)Memory Definition" + KEY_FIELDS = ("block_type", "definition_id") + block_type: str + definition_id: str + __slots__ = KEY_FIELDS + CHECKED_INIT = False + + def __init__(self, block_type: str, definition_id: str): + super().__init__(block_type=block_type, definition_id=definition_id) + + def _to_string(self) -> str: + return f"{self.block_type}:{self.definition_id}" + + @classmethod + def _from_string(cls, serialized: str): + try: + block_type, definition_id = serialized.split(":") + except ValueError as exc: + raise ValueError(f"invalid {cls.__name__}: {serialized}") from exc + return _InMemoryDefinitionKey(block_type, definition_id) + + +class _InMemoryUsageKey(UsageKey): + """ + A simple usage key: mu::. NOT part of the public XBlock API. + """ + CANONICAL_NAMESPACE = 'mb' # "(In-)Memory Block" + KEY_FIELDS = ('block_type', 'usage_id') + block_type: str + usage_id: str + __slots__ = KEY_FIELDS + CHECKED_INIT = False + + def __init__(self, block_type: str, usage_id: str): + super().__init__(block_type=block_type, usage_id=usage_id) + + def _to_string(self) -> str: + return f"{self.block_type}:{self.usage_id}" + + @classmethod + def _from_string(cls, serialized: str): + try: + block_type, usage_id = serialized.split(":") + except ValueError as exc: + raise ValueError(f"invalid {cls.__name__}: {serialized}") from exc + return _InMemoryDefinitionKey(block_type, usage_id) + + @property + def block_id(self) -> str: + return self.definition_id + + @property + def context_key(self) -> LearningContextKey: + """ + Raise an error because these blocks exist outside a LearningContext. + """ + raise TypeError("Usages managed by MemoryIdManager do not have a LearningContext") + + @property + def definition_key(self) -> DefinitionKey: + """ + Raise an error because the InMemoryIdManager must be used to access the definition key. + """ + raise TypeError( + "Usages managed by MemoryIdManager do not know their definition keys. Use get_definition_id instead." + ) + + course_key = context_key # the UsageKey class demands this for backcompat. + + def map_into_course(self, course_key: CourseKey) -> t.Self: + return course_key.make_usage_key(self.block_type, self.block_id) + + class MemoryIdManager(IdReader, IdGenerator): """A simple dict-based implementation of IdReader and IdGenerator.""" - ASIDE_USAGE_ID = namedtuple('MemoryAsideUsageId', 'usage_id aside_type') - ASIDE_DEFINITION_ID = namedtuple('MemoryAsideDefinitionId', 'definition_id aside_type') - def __init__(self): - self._ids = itertools.count() - self._usages = {} - self._definitions = {} + self._ids: t.Iterator[int] = itertools.count() + self._usages: dict[DefinitionKey, _InMemoryUsageKey] = {} - def _next_id(self, prefix): + def _next_id(self, prefix) -> str: """Generate a new id.""" return f"{prefix}_{next(self._ids)}" - def clear(self): + def clear(self) -> None: """Remove all entries.""" self._usages.clear() - self._definitions.clear() - def create_aside(self, definition_id, usage_id, aside_type): + def create_aside( + self, definition_id: DefinitionKey, usage_id: UsageKey, aside_type: str + ) -> t.tuple[AsideDefinitionKeyV2, AsideUsageKeyV2]: """Create the aside.""" return ( - self.ASIDE_DEFINITION_ID(definition_id, aside_type), - self.ASIDE_USAGE_ID(usage_id, aside_type), + AsideDefinitionKeyV2(definition_id, aside_type), + AsideUsageKeyV2(usage_id, aside_type) ) - def get_usage_id_from_aside(self, aside_id): + def get_usage_id_from_aside(self, aside_id: AsideUsageKeyV2) -> UsageKey: """Extract the usage_id from the aside's usage_id.""" - return aside_id.usage_id + return aside_id.usage_key - def get_definition_id_from_aside(self, aside_id): + def get_definition_id_from_aside(self, aside_id: AsideDefinitionKeyV2) -> DefinitionKey: """Extract the original xblock's definition_id from an aside's definition_id.""" - return aside_id.definition_id + return aside_id.definition_key - def create_usage(self, def_id): + def create_usage(self, def_id: DefinitionKey) -> _InMemoryUsageKey: """Make a usage, storing its definition id.""" - usage_id = self._next_id("u") - self._usages[usage_id] = def_id - return usage_id + if not isinstance(def_id, _InMemoryDefinitionKey): + raise TypeError( + f"got def_id of type {type(def_id)}, expected def_id of type {_InMemoryDefinitionKey.__name__}" + ) + usage_key = _InMemoryUsageKey(def_id, self._next_id("u")) + self._usages[usage_key] = def_id + return usage_key - def get_definition_id(self, usage_id): + def get_definition_id(self, usage_id: UsageKey) -> _InMemoryDefinitionKey: """Get a definition_id by its usage id.""" try: return self._usages[usage_id] except KeyError: raise NoSuchUsage(repr(usage_id)) # pylint: disable= raise-missing-from - def create_definition(self, block_type, slug=None): - """Make a definition, storing its block type.""" + def create_definition(self, block_type: str, slug: str | None = None) -> _InMemoryDefinitionKey: + """Make a definition, including its block type in its key.""" prefix = "d" if slug: prefix += "_" + slug - def_id = self._next_id(prefix) - self._definitions[def_id] = block_type - return def_id + return _InMemoryDefinitionKey(block_type, self._next_id(prefix)) - def get_block_type(self, def_id): + def get_block_type(self, def_id: DefinitionKey) -> str: """Get a block_type by its definition id.""" - try: - return self._definitions[def_id] - except KeyError: - try: - return def_id.aside_type - except AttributeError: - raise NoSuchDefinition(repr(def_id)) # pylint: disable= raise-missing-from + return def_id.block_type - def get_aside_type_from_definition(self, aside_id): + def get_aside_type_from_definition(self, aside_id: AsideDefinitionKeyV2) -> str: """Get an aside's type from its definition id.""" return aside_id.aside_type - def get_aside_type_from_usage(self, aside_id): + def get_aside_type_from_usage(self, aside_id: AsideUsageKeyV2) -> str: """Get an aside's type from its usage id.""" return aside_id.aside_type diff --git a/xblock/test/django/test_field_translation.py b/xblock/test/django/test_field_translation.py index 54d3e317c..fb5ffbeb9 100644 --- a/xblock/test/django/test_field_translation.py +++ b/xblock/test/django/test_field_translation.py @@ -15,7 +15,7 @@ DictKeyValueStore, KvsFieldData, ) -from xblock.test.tools import TestRuntime +from xblock.test.tools import TestRuntime, TestKey class TestXBlockStringFieldDefaultTranslation(TestCase): @@ -45,7 +45,8 @@ class XBlockTest(XBlock): # Change language to 'de'. user_language = 'de' with translation.override(user_language): - tester = runtime.construct_xblock_from_class(XBlockTest, ScopeIds('s0', 'XBlockTest', 'd0', 'u0')) + test_key = TestKey("XBlockTest", "k0") + tester = runtime.construct_xblock_from_class(XBlockTest, ScopeIds('s0', 'XBlockTest', test_key, test_key)) # Assert instantiated XBlock str_field value is not yet evaluated. assert 'django.utils.functional.' in str(type(tester.str_field)) diff --git a/xblock/test/test_completable.py b/xblock/test/test_completable.py index 09db80a3e..caa224d6c 100644 --- a/xblock/test/test_completable.py +++ b/xblock/test/test_completable.py @@ -15,6 +15,7 @@ from xblock.fields import ScopeIds from xblock.runtime import Runtime from xblock.completable import CompletableXBlockMixin, XBlockCompletionMode +from xblock.test.tools import TestKey @ddt.ddt @@ -77,7 +78,8 @@ def _make_block(self, runtime=None, block_type=None): """ block_type = block_type if block_type else self.TestBuddyXBlock runtime = runtime if runtime else mock.Mock(spec=Runtime) - scope_ids = ScopeIds("user_id", "test_buddy", "def_id", "usage_id") + test_key = TestKey("test_buddy", "test_id") + scope_ids = ScopeIds("user_id", "test_buddy", test_key, test_key) return block_type(runtime=runtime, scope_ids=scope_ids) def test_has_custom_completion_property(self): diff --git a/xblock/test/test_core.py b/xblock/test/test_core.py index 8a888a210..89f957354 100644 --- a/xblock/test/test_core.py +++ b/xblock/test/test_core.py @@ -13,6 +13,7 @@ import ddt import pytest +from opaque_keys.edx.keys import DefinitionKey from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryLocatorV2 from webob import Response @@ -45,7 +46,7 @@ class FieldTester(XBlock): field_data = DictFieldData({'field_a': 5, 'float_a': 6.1, 'field_x': 15}) - field_tester = FieldTester(TestRuntime(services={'field-data': field_data}), scope_ids=Mock()) + field_tester = FieldTester(TestRuntime(services={'field-data': field_data}), scope_ids=Mock(spec=ScopeIds)) # Verify that the fields have been set assert field_tester.field_a == 5 assert field_tester.field_b == 10 @@ -153,7 +154,10 @@ class FieldTester(XBlock): field_c = Set(scope=Scope.content, default=[4, 5, 6]) field_d = Set(scope=Scope.settings) - field_tester = FieldTester(MagicMock(), DictFieldData({'field_a': [200], 'field_b': [11, 12, 13]}), Mock()) + field_tester = FieldTester( + MagicMock(), + DictFieldData({'field_a': [200], 'field_b': [11, 12, 13]}), Mock(spec=ScopeIds), + ) # Check initial values have been set properly assert {200} == field_tester.field_a @@ -279,7 +283,7 @@ class FieldTester(XBlock): field_tester = FieldTester( TestRuntime(services={'field-data': field_data}), None, - Mock() + Mock(spec=ScopeIds), ) # Check initial values have been set properly @@ -524,7 +528,7 @@ class FieldTester(XBlock): field_tester = FieldTester( TestRuntime(services={'field-data': field_data}), None, - Mock(), + Mock(spec=ScopeIds), ) assert field_tester.field == 4 @@ -534,26 +538,26 @@ class FieldTester(XBlock): def test_class_tags(): - xblock = XBlock(None, None, None) + xblock = XBlock(None, None, Mock(ScopeIds)) assert xblock._class_tags == set() # pylint: disable=comparison-with-callable class Sub1Block(XBlock): """Toy XBlock""" - sub1block = Sub1Block(None, None, None) + sub1block = Sub1Block(None, None, Mock(ScopeIds)) assert sub1block._class_tags == set() # pylint: disable=comparison-with-callable @XBlock.tag("cat dog") class Sub2Block(Sub1Block): """Toy XBlock""" - sub2block = Sub2Block(None, None, None) + sub2block = Sub2Block(None, None, Mock(ScopeIds)) assert sub2block._class_tags == {"cat", "dog"} # pylint: disable=comparison-with-callable class Sub3Block(Sub2Block): """Toy XBlock""" - sub3block = Sub3Block(None, None, None) + sub3block = Sub3Block(None, None, Mock(ScopeIds)) assert sub3block._class_tags == {"cat", "dog"} # pylint: disable=comparison-with-callable @XBlock.tag("mixin") @@ -563,7 +567,7 @@ class MixinBlock(XBlock): class Sub4Block(MixinBlock, Sub3Block): """Toy XBlock""" - sub4block = Sub4Block(None, None, None) + sub4block = Sub4Block(None, None, Mock(ScopeIds)) assert sub4block._class_tags == { # pylint: disable=comparison-with-callable "cat", "dog", "mixin" } @@ -763,7 +767,7 @@ class MutableTester(XBlock): def test_handle_shortcut(): runtime = Mock(spec=['handle']) - scope_ids = Mock(spec=[]) + scope_ids = Mock(spec=ScopeIds) request = Mock(spec=[]) block = XBlock(runtime, None, scope_ids) @@ -780,7 +784,7 @@ def test_services_decorators(): class NoServicesBlock(XBlock): """XBlock requesting no services""" - no_services_block = NoServicesBlock(None, None, None) + no_services_block = NoServicesBlock(None, None, Mock(ScopeIds)) assert not NoServicesBlock._services_requested assert not no_services_block._services_requested @@ -789,7 +793,7 @@ class NoServicesBlock(XBlock): class ServiceUsingBlock(XBlock): """XBlock using some services.""" - service_using_block = ServiceUsingBlock(None, scope_ids=Mock()) + service_using_block = ServiceUsingBlock(None, scope_ids=Mock(ScopeIds)) assert ServiceUsingBlock._services_requested == { 'n': 'need', 'w': 'want' } @@ -809,7 +813,7 @@ class ServiceUsingBlock(XBlock): class SubServiceUsingBlock(ServiceUsingBlock): """Does this class properly inherit services from ServiceUsingBlock?""" - sub_service_using_block = SubServiceUsingBlock(None, scope_ids=Mock()) + sub_service_using_block = SubServiceUsingBlock(None, scope_ids=Mock(spec=ScopeIds)) 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" @@ -974,7 +978,7 @@ def stub_open_resource(self, uri): "public/\N{SNOWMAN}.js", ) def test_open_good_local_resource(self, uri): - loadable = self.LoadableXBlock(None, scope_ids=Mock()) + loadable = self.LoadableXBlock(None, scope_ids=Mock(spec=ScopeIds)) with patch('xblock.core.Blocklike._open_resource', self.stub_open_resource): assert loadable.open_local_resource(uri) == "!" + uri + "!" assert loadable.open_local_resource(uri.encode('utf-8')) == "!" + uri + "!" @@ -988,7 +992,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=Mock()) + loadable = self.LoadableXBlock(None, scope_ids=Mock(spec=ScopeIds)) with patch('xblock.core.Blocklike._open_resource', self.stub_open_resource): assert loadable.open_local_resource(uri) == "!" + uri.decode('utf-8') + "!" @@ -1002,7 +1006,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=Mock()) + loadable = self.LoadableXBlock(None, scope_ids=Mock(spec=ScopeIds)) with patch('xblock.core.Blocklike._open_resource', self.stub_open_resource): msg_pattern = ".*: %s" % re.escape(repr(uri)) with pytest.raises(DisallowedFileError, match=msg_pattern): @@ -1018,7 +1022,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=Mock()) + loadable = self.LoadableXBlock(None, scope_ids=Mock(spec=ScopeIds)) with patch('xblock.core.Blocklike._open_resource', self.stub_open_resource): msg = ".*: %s" % re.escape(repr(uri.decode('utf-8'))) with pytest.raises(DisallowedFileError, match=msg): @@ -1040,7 +1044,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=Mock()) + unloadable = self.UnloadableXBlock(None, scope_ids=Mock(spec=ScopeIds)) with patch('xblock.core.Blocklike._open_resource', self.stub_open_resource): msg = "not configured to serve local resources" @@ -1141,25 +1145,9 @@ def test_key_properties(self): scope_ids = ScopeIds( user_id="myUser", block_type="myType", - def_id="myDefId", + def_id=Mock(spec=DefinitionKey), usage_id=self.library_block_key, ) block = XBlock(Mock(spec=Runtime), scope_ids=scope_ids) self.assertEqual(block.usage_key, self.library_block_key) self.assertEqual(block.context_key, self.library_key) - - def test_key_properties_when_usage_is_not_an_opaque_key(self): - """ - Tests a legacy scenario that we believe only happens in xblock-sdk at this point. - - Remove this test as part of https://github.com/openedx/XBlock/issues/708. - """ - scope_ids = ScopeIds( - user_id="myUser", - block_type="myType", - def_id="myDefId", - usage_id="myWeirdOldUsageId", - ) - block = XBlock(Mock(spec=Runtime), scope_ids=scope_ids) - self.assertEqual(block.usage_key, "myWeirdOldUsageId") - self.assertIsNone(block.context_key) diff --git a/xblock/test/test_core_capabilities.py b/xblock/test/test_core_capabilities.py index 0919a35ee..22d82f714 100644 --- a/xblock/test/test_core_capabilities.py +++ b/xblock/test/test_core_capabilities.py @@ -20,7 +20,7 @@ from xblock.fields import List, Scope, Integer, String, ScopeIds, UNIQUE_ID, DateTime from xblock.field_data import DictFieldData from xblock.runtime import Runtime -from xblock.test.tools import TestRuntime +from xblock.test.tools import TestRuntime, TestKey class AttrAssertionMixin(TestCase): @@ -144,7 +144,9 @@ class IndexInfoMixinTester(Blocklike): def test_index_info(self): self.assertHasAttr(self.IndexInfoMixinTester, 'index_dictionary') - with_index_info = self.IndexInfoMixinTester(runtime=None, scope_ids=None).index_dictionary() + with_index_info = self.IndexInfoMixinTester( + runtime=None, scope_ids=mock.Mock(spec=ScopeIds) + ).index_dictionary() self.assertFalse(with_index_info) self.assertTrue(isinstance(with_index_info, dict)) @@ -181,7 +183,7 @@ def an_unsupported_view(self): """ # pragma: no cover - test_xblock = SupportsDecoratorTester(None, None, None) + test_xblock = SupportsDecoratorTester(None, None, mock.Mock(spec=ScopeIds)) for view_name, functionality, expected_result in ( ("functionality_supported_view", "a_functionality", True), @@ -213,7 +215,7 @@ def has_support(self, view, functionality): """ return functionality == "a_functionality" - test_xblock = HasSupportOverrideTester(None, None, None) + test_xblock = HasSupportOverrideTester(None, None, mock.Mock(spec=ScopeIds)) for view_name, functionality, expected_result in ( ("functionality_supported_view", "a_functionality", True), @@ -273,7 +275,8 @@ def _make_block(self, block_type=None): """ Creates a test block """ block_type = block_type if block_type else self.TestXBlock runtime_mock = mock.Mock(spec=Runtime) - scope_ids = ScopeIds("user_id", block_type.etree_node_tag, "def_id", "usage_id") + test_key = TestKey(block_type.etree_node_tag, "usage_id") + scope_ids = ScopeIds("user_id", test_key.block_type, test_key, test_key) return block_type(runtime=runtime_mock, field_data=DictFieldData({}), scope_ids=scope_ids) def _assert_node_attributes(self, node, expected_attributes, entry_point=None): diff --git a/xblock/test/test_field_data.py b/xblock/test/test_field_data.py index eaf12281e..861a6d871 100644 --- a/xblock/test/test_field_data.py +++ b/xblock/test/test_field_data.py @@ -6,7 +6,7 @@ from xblock.core import XBlock from xblock.exceptions import InvalidScopeError -from xblock.fields import Scope, String +from xblock.fields import Scope, String, ScopeIds from xblock.field_data import SplitFieldData, ReadOnlyFieldData from xblock.test.tools import TestRuntime @@ -48,7 +48,7 @@ def setup_method(self): self.runtime = TestRuntime(services={'field-data': self.split}) self.block = TestingBlock( runtime=self.runtime, - scope_ids=Mock(), + scope_ids=Mock(spec=ScopeIds), ) # pylint: enable=attribute-defined-outside-init @@ -105,7 +105,7 @@ def setup_method(self): self.runtime = TestRuntime(services={'field-data': self.read_only}) self.block = TestingBlock( runtime=self.runtime, - scope_ids=Mock(), + scope_ids=Mock(spec=ScopeIds), ) # pylint: enable=attribute-defined-outside-init diff --git a/xblock/test/test_fields.py b/xblock/test/test_fields.py index 1941e7410..11c3216d3 100644 --- a/xblock/test/test_fields.py +++ b/xblock/test/test_fields.py @@ -22,7 +22,7 @@ ScopeIds, Sentinel, UNIQUE_ID, scope_key, Date, Timedelta, RelativeTime, ScoreField, ListScoreField ) from xblock.scorable import Score -from xblock.test.tools import TestRuntime +from xblock.test.tools import TestRuntime, TestKey class FieldTest(unittest.TestCase): @@ -717,25 +717,36 @@ class TestBlock(XBlock): pref_lst = List(scope=Scope.preferences, name='') user_info_lst = List(scope=Scope.user_info, name='') - sids = ScopeIds(user_id="_bob", - block_type="b.12#ob", - def_id="..", - usage_id="..") - + usage_key = TestKey("b.12#ob", "..") + sids = ScopeIds(user_id="_bob", block_type=usage_key.block_type, def_id=usage_key, usage_id=usage_key) field_data = DictFieldData({}) runtime = TestRuntime(Mock(), services={'field-data': field_data}) block = TestBlock(runtime, None, sids) - # Format: usage or block ID/field_name/user_id - for item, correct_key in [[TestBlock.field_x, "__..../field__x/NONE.NONE"], - [TestBlock.user_info_lst, "NONE.NONE/user__info__lst/____bob"], - [TestBlock.pref_lst, "b..12_35_ob/pref__lst/____bob"], - [TestBlock.user_lst, "__..../user__lst/____bob"], - [TestBlock.uss_lst, "__..../uss__lst/NONE.NONE"], - [TestBlock.settings_lst, "__..../settings__lst/NONE.NONE"]]: - key = scope_key(item, block) - assert key == correct_key + # Format: // + # Note that our is a TestKey, which is formatted as: + # tk:$ <- original + # tk: $ + # tk-__36_ <- encoded + # And our particular is: + # b.12#ob <- original + # b. 12# ob + # b..12_35_ob <- encoded + # And our particular is: + # .. <- original + # . . + # .... <- encoded + # And so the full formatted TestKey instance is: + # tk:b.12#ob$.. <- original + # tk: b. 12# ob$ . . + # tk-_b..12_35_ob_36_.... <- encoded + assert scope_key(TestBlock.field_x, block) == "tk-_b..12_35_ob_36_..../field__x/NONE.NONE" + assert scope_key(TestBlock.user_info_lst, block) == "NONE.NONE/user__info__lst/____bob" + assert scope_key(TestBlock.pref_lst, block) == "b..12_35_ob/pref__lst/____bob" + assert scope_key(TestBlock.user_lst, block) == "tk-_b..12_35_ob_36_..../user__lst/____bob" + assert scope_key(TestBlock.uss_lst, block) == "tk-_b..12_35_ob_36_..../uss__lst/NONE.NONE" + assert scope_key(TestBlock.settings_lst, block) == "tk-_b..12_35_ob_36_..../settings__lst/NONE.NONE" def test_field_display_name(): @@ -763,10 +774,13 @@ class TestBlock(XBlock): field_a = String(default=UNIQUE_ID, scope=Scope.settings) field_b = String(default=UNIQUE_ID, scope=Scope.user_state) - sids = ScopeIds(user_id="bob", - block_type="bobs-type", - def_id="definition-id", - usage_id="usage-id") + usage_key = TestKey("testtype", "..") + sids = ScopeIds( + user_id="_bob", + block_type=usage_key.block_type, + def_id=usage_key, + usage_id=usage_key, + ) runtime = TestRuntime(services={'field-data': DictFieldData({})}) block = TestBlock(runtime, DictFieldData({}), sids) @@ -785,7 +799,7 @@ class TestBlock(XBlock): assert unique_b != block.field_b # Change the usage id. Unique ID default for both fields should change. runtime = TestRuntime(services={'field-data': DictFieldData({})}) - block = TestBlock(runtime, DictFieldData({}), sids._replace(usage_id='usage-2')) + block = TestBlock(runtime, DictFieldData({}), sids._replace(usage_id=TestKey("testtype", "usage-2"))) assert unique_a != block.field_a assert unique_b != block.field_b @@ -828,7 +842,7 @@ class FieldTester(XBlock): not_timezone_aware = dt.datetime(2015, 1, 1) timezone_aware = dt.datetime(2015, 1, 1, tzinfo=pytz.UTC) runtime = TestRuntime(services={'field-data': DictFieldData({})}) - field_tester = FieldTester(runtime, scope_ids=Mock(spec=ScopeIds)) + field_tester = FieldTester(runtime, scope_ids=Mock(ScopeIds)) field_tester.incomparable = not_timezone_aware field_tester.incomparable = timezone_aware assert field_tester.incomparable == timezone_aware diff --git a/xblock/test/test_runtime.py b/xblock/test/test_runtime.py index 4e58d813f..45736983d 100644 --- a/xblock/test/test_runtime.py +++ b/xblock/test/test_runtime.py @@ -29,7 +29,7 @@ ) from xblock.field_data import DictFieldData, FieldData -from xblock.test.tools import unabc, WarningTestMixin, TestRuntime +from xblock.test.tools import unabc, WarningTestMixin, TestRuntime, TestKey class TestMixin: @@ -112,7 +112,11 @@ def test_db_model_keys(): key_store = DictKeyValueStore() field_data = KvsFieldData(key_store) runtime = TestRuntime(Mock(), mixins=[TestMixin], services={'field-data': field_data}) - tester = runtime.construct_xblock_from_class(TestXBlock, ScopeIds('s0', 'TestXBlock', 'd0', 'u0')) + test_def_key = TestKey("TestXBlock", "d0") + test_usage_key = TestKey("TestXBlock", "u0") + tester = runtime.construct_xblock_from_class( + TestXBlock, ScopeIds('s0', 'TestXBlock', test_def_key, test_usage_key) + ) assert not field_data.has(tester, 'not a field') @@ -136,36 +140,36 @@ def get_key_value(scope, user_id, block_scope_id, field_name): return key_store.db_dict[new_key] # Examine each value in the database and ensure that keys were constructed correctly - assert get_key_value(Scope.content, None, 'd0', 'content') == 'new content' - assert get_key_value(Scope.settings, None, 'u0', 'settings') == 'new settings' - assert get_key_value(Scope.user_state, 's0', 'u0', 'user_state') == 'new user_state' + assert get_key_value(Scope.content, None, test_def_key, 'content') == 'new content' + assert get_key_value(Scope.settings, None, test_usage_key, 'settings') == 'new settings' + assert get_key_value(Scope.user_state, 's0', test_usage_key, 'user_state') == 'new user_state' assert get_key_value(Scope.preferences, 's0', 'TestXBlock', 'preferences') == 'new preferences' assert get_key_value(Scope.user_info, 's0', None, 'user_info') == 'new user_info' assert get_key_value(Scope(UserScope.NONE, BlockScope.TYPE), None, 'TestXBlock', 'by_type') == 'new by_type' assert get_key_value(Scope(UserScope.NONE, BlockScope.ALL), None, None, 'for_all') == 'new for_all' - assert get_key_value(Scope(UserScope.ONE, BlockScope.DEFINITION), 's0', 'd0', 'user_def') == 'new user_def' + assert get_key_value(Scope(UserScope.ONE, BlockScope.DEFINITION), 's0', test_def_key, 'user_def') == 'new user_def' assert get_key_value(Scope(UserScope.ALL, BlockScope.ALL), None, None, 'agg_global') == 'new agg_global' assert get_key_value(Scope(UserScope.ALL, BlockScope.TYPE), None, 'TestXBlock', 'agg_type') == 'new agg_type' - assert get_key_value(Scope(UserScope.ALL, BlockScope.DEFINITION), None, 'd0', 'agg_def') == 'new agg_def' - assert get_key_value(Scope.user_state_summary, None, 'u0', 'agg_usage') == 'new agg_usage' - assert get_key_value(Scope.content, None, 'd0', 'mixin_content') == 'new mixin_content' - assert get_key_value(Scope.settings, None, 'u0', 'mixin_settings') == 'new mixin_settings' - assert get_key_value(Scope.user_state, 's0', 'u0', 'mixin_user_state') == 'new mixin_user_state' + assert get_key_value(Scope(UserScope.ALL, BlockScope.DEFINITION), None, test_def_key, 'agg_def') == 'new agg_def' + assert get_key_value(Scope.user_state_summary, None, test_usage_key, 'agg_usage') == 'new agg_usage' + assert get_key_value(Scope.content, None, test_def_key, 'mixin_content') == 'new mixin_content' + assert get_key_value(Scope.settings, None, test_usage_key, 'mixin_settings') == 'new mixin_settings' + assert get_key_value(Scope.user_state, 's0', test_usage_key, 'mixin_user_state') == 'new mixin_user_state' assert get_key_value(Scope.preferences, 's0', 'TestXBlock', 'mixin_preferences') == 'new mixin_preferences' assert get_key_value(Scope.user_info, 's0', None, 'mixin_user_info') == 'new mixin_user_info' assert get_key_value(Scope(UserScope.NONE, BlockScope.TYPE), None, 'TestXBlock', 'mixin_by_type') == \ 'new mixin_by_type' assert get_key_value(Scope(UserScope.NONE, BlockScope.ALL), None, None, 'mixin_for_all') == \ 'new mixin_for_all' - assert get_key_value(Scope(UserScope.ONE, BlockScope.DEFINITION), 's0', 'd0', 'mixin_user_def') == \ + assert get_key_value(Scope(UserScope.ONE, BlockScope.DEFINITION), 's0', test_def_key, 'mixin_user_def') == \ 'new mixin_user_def' assert get_key_value(Scope(UserScope.ALL, BlockScope.ALL), None, None, 'mixin_agg_global') == \ 'new mixin_agg_global' assert get_key_value(Scope(UserScope.ALL, BlockScope.TYPE), None, 'TestXBlock', 'mixin_agg_type') == \ 'new mixin_agg_type' - assert get_key_value(Scope(UserScope.ALL, BlockScope.DEFINITION), None, 'd0', 'mixin_agg_def') == \ + assert get_key_value(Scope(UserScope.ALL, BlockScope.DEFINITION), None, test_def_key, 'mixin_agg_def') == \ 'new mixin_agg_def' - assert get_key_value(Scope.user_state_summary, None, 'u0', 'mixin_agg_usage') == 'new mixin_agg_usage' + assert get_key_value(Scope.user_state_summary, None, test_usage_key, 'mixin_agg_usage') == 'new mixin_agg_usage' @unabc("{} shouldn't be used in tests") @@ -338,7 +342,7 @@ def test_mixin_field_access(): }) runtime = TestRuntime(Mock(), mixins=[TestSimpleMixin], services={'field-data': field_data}) - field_tester = runtime.construct_xblock_from_class(FieldTester, Mock()) + field_tester = runtime.construct_xblock_from_class(FieldTester, Mock(spec=ScopeIds)) assert field_tester.field_a == 5 assert field_tester.field_b == 10 @@ -559,7 +563,7 @@ def test_ugettext_calls(): Test ugettext calls in xblock. """ runtime = TestRuntime() - block = XBlockWithServices(runtime, scope_ids=Mock(spec=[])) + block = XBlockWithServices(runtime, scope_ids=Mock(spec=ScopeIds)) assert block.ugettext('test') == 'test' assert isinstance(block.ugettext('test'), str) @@ -567,7 +571,7 @@ def test_ugettext_calls(): runtime = TestRuntime(services={ 'i18n': None }) - block = XBlockWithServices(runtime, scope_ids=Mock(spec=[])) + block = XBlockWithServices(runtime, scope_ids=Mock(spec=ScopeIds)) with pytest.raises(NoSuchServiceError): block.ugettext('test') diff --git a/xblock/test/tools.py b/xblock/test/tools.py index 98a6e2a6e..8a8c25262 100644 --- a/xblock/test/tools.py +++ b/xblock/test/tools.py @@ -1,13 +1,72 @@ """ Tools for testing XBlocks """ +import typing as t + +import warnings from contextlib import contextmanager from functools import partial -import warnings +from opaque_keys.edx.keys import CourseKey, DefinitionKey, UsageKey, LearningContextKey from xblock.runtime import Runtime, MemoryIdManager +class TestKey(UsageKey, DefinitionKey): + """ + A simple way to identify block usages and definitions using just type and ID slug. + + This key can serve as both an identifer for block Usages and block Definitions. + (This aligns the new Learning-Core-based XBlockRuntimes in openedx-platform, which + don't differentiate between usage and definition--UsageKeyV2s are used for both). + + This class is exclusively for XBlock framework test code! + + Serialization --> tk:$ + """ + CANONICAL_NAMESPACE = 'tk' # "Test Key" + KEY_FIELDS = ('block_type', 'block_id') + block_type: str + block_id: str + __slots__ = KEY_FIELDS + CHECKED_INIT = False + + def __init__(self, block_type: str, block_id: str): + super().__init__(block_type=block_type, block_id=block_id) + + def _to_string(self) -> str: + return f"{self.block_type}${self.block_id}" + + @classmethod + def _from_string(cls, serialized: str): + return cls(*serialized.split("$")) + + @property + def definition_key(self) -> DefinitionKey: + return self + + @property + def context_key(self) -> LearningContextKey: + """ + Raise an error because core XBlock code should be oblivious to LearningContexts. + + Within the actual openedx-platform runtimes, every Usage belongs to a LearningContext. + Within the XBlock framework, though, we are oblivious to the idea of LearningContexts. We just deal + with opaque UsageKeys instead. It's a nice simplifying assumption to have. + So, rather than return fake context key here, let's fail the unit test. + Future devs: if you really need to add LearningContext awareness in the XBlock framework, + you could define a TestContextKey class and return a static instance of it from this method. + """ + raise TypeError( + "Cannot access the Context of a TestKey " + "(are you sure you need to call .context_key in order to test XBlock code?)" + ) + + course_key = context_key # the UsageKey class demands this for backcompat. + + def map_into_course(self, course_key: CourseKey) -> t.Self: + raise ValueError("Cannot use this key type in the context of courses") + + def blocks_are_equivalent(block1, block2): """Compare two blocks for equivalence. """ diff --git a/xblock/test/toy_runtime.py b/xblock/test/toy_runtime.py index 342cdb998..d33a3bbd0 100644 --- a/xblock/test/toy_runtime.py +++ b/xblock/test/toy_runtime.py @@ -52,7 +52,7 @@ def _actual_key(self, key): key_list.append(key.scope.block.attr_name) if key.block_scope_id is not None: - key_list.append(key.block_scope_id) + key_list.append(str(key.block_scope_id)) if key.user_id: key_list.append(key.user_id) return ".".join(key_list) diff --git a/xblock/test/utils/test_settings.py b/xblock/test/utils/test_settings.py index f5593a553..2985bc6ee 100644 --- a/xblock/test/utils/test_settings.py +++ b/xblock/test/utils/test_settings.py @@ -8,6 +8,7 @@ import ddt from xblock.core import XBlock +from xblock.fields import ScopeIds from xblock.utils.settings import XBlockWithSettingsMixin, ThemableXBlockMixin @@ -48,7 +49,7 @@ def setUp(self): @ddt.data(None, 1, "2", [3, 4], {5: '6'}) def test_no_settings_service_return_default(self, default_value): - xblock = DummyXBlockWithSettings(self.runtime, scope_ids=Mock()) + xblock = DummyXBlockWithSettings(self.runtime, scope_ids=Mock(spec=ScopeIds)) self.runtime.service.return_value = None self.assertEqual(xblock.get_xblock_settings(default=default_value), default_value) @@ -59,7 +60,7 @@ def test_no_settings_service_return_default(self, default_value): )) @ddt.unpack def test_invokes_get_settings_bucket_and_returns_result(self, block, settings_service_return_value, default): - xblock = block(self.runtime, scope_ids=Mock()) + xblock = block(self.runtime, scope_ids=Mock(spec=ScopeIds)) self.settings_service.get_settings_bucket = Mock(return_value=settings_service_return_value) self.assertEqual(xblock.get_xblock_settings(default=default), settings_service_return_value) @@ -78,13 +79,13 @@ def setUp(self): @ddt.data(DummyXBlockWithSettings, OtherXBlockWithSettings) def test_theme_uses_default_theme_if_settings_service_is_not_available(self, xblock_class): - xblock = xblock_class(self.runtime_mock, scope_ids=Mock()) + xblock = xblock_class(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) self.runtime_mock.service = Mock(return_value=None) self.assertEqual(xblock.get_theme(), xblock_class.default_theme_config) @ddt.data(DummyXBlockWithSettings, OtherXBlockWithSettings) def test_theme_uses_default_theme_if_no_theme_is_set(self, xblock_class): - xblock = xblock_class(self.runtime_mock, scope_ids=Mock()) + xblock = xblock_class(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) self.service_mock.get_settings_bucket = Mock(return_value=None) self.assertEqual(xblock.get_theme(), xblock_class.default_theme_config) self.service_mock.get_settings_bucket.assert_called_once_with(xblock, default={}) @@ -95,7 +96,7 @@ def test_theme_uses_default_theme_if_no_theme_is_set(self, xblock_class): )) @ddt.unpack def test_theme_raises_if_theme_object_is_not_iterable(self, xblock_class, theme_config): - xblock = xblock_class(self.runtime_mock, scope_ids=Mock()) + xblock = xblock_class(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) self.service_mock.get_settings_bucket = Mock(return_value=theme_config) with self.assertRaises(TypeError): xblock.get_theme() @@ -107,7 +108,7 @@ def test_theme_raises_if_theme_object_is_not_iterable(self, xblock_class, theme_ )) @ddt.unpack def test_theme_uses_default_theme_if_no_mentoring_theme_is_set_up(self, xblock_class, theme_config): - xblock = xblock_class(self.runtime_mock, scope_ids=Mock()) + xblock = xblock_class(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) self.service_mock.get_settings_bucket = Mock(return_value=theme_config) self.assertEqual(xblock.get_theme(), xblock_class.default_theme_config) self.service_mock.get_settings_bucket.assert_called_once_with(xblock, default={}) @@ -118,13 +119,13 @@ def test_theme_uses_default_theme_if_no_mentoring_theme_is_set_up(self, xblock_c )) @ddt.unpack def test_theme_correctly_returns_configured_theme(self, xblock_class, theme_config): - xblock = xblock_class(self.runtime_mock, scope_ids=Mock()) + xblock = xblock_class(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) self.service_mock.get_settings_bucket = Mock(return_value={xblock_class.theme_key: theme_config}) self.assertEqual(xblock.get_theme(), theme_config) @ddt.data(DummyXBlockWithSettings, OtherXBlockWithSettings) def test_theme_files_are_loaded_from_correct_package(self, xblock_class): - xblock = xblock_class(self.runtime_mock, scope_ids=Mock()) + xblock = xblock_class(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) fragment = MagicMock() package_name = 'some_package' theme_config = {xblock_class.theme_key: {'package': package_name, 'locations': ['lms.css']}} @@ -141,7 +142,7 @@ def test_theme_files_are_loaded_from_correct_package(self, xblock_class): ) @ddt.unpack def test_theme_files_are_added_to_fragment(self, package_name, locations): - xblock = DummyXBlockWithSettings(self.runtime_mock, scope_ids=Mock()) + xblock = DummyXBlockWithSettings(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) fragment = MagicMock() theme_config = {DummyXBlockWithSettings.theme_key: {'package': package_name, 'locations': locations}} self.service_mock.get_settings_bucket = Mock(return_value=theme_config) @@ -154,7 +155,7 @@ def test_theme_files_are_added_to_fragment(self, package_name, locations): @ddt.data(None, {}, {'locations': ['red.css']}) def test_invalid_default_theme_config(self, theme_config): - xblock = DummyXBlockWithSettings(self.runtime_mock, scope_ids=Mock()) + xblock = DummyXBlockWithSettings(self.runtime_mock, scope_ids=Mock(spec=ScopeIds)) xblock.default_theme_config = theme_config self.service_mock.get_settings_bucket = Mock(return_value={}) fragment = MagicMock()