Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: rename ModuleStore runtimes now that XModules are gone #35523

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cms/djangoapps/contentstore/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ def _import_xml_node_to_parent(
block_type = node.tag

# Modulestore's IdGenerator here is SplitMongoIdManager which is assigned
# by CachingDescriptorSystem Runtime and since we need our custom ImportIdGenerator
# by SplitModuleStoreRuntime and since we need our custom ImportIdGenerator
# here we are temporaraliy swtiching it.
original_id_generator = runtime.id_generator

Expand Down Expand Up @@ -357,7 +357,8 @@ def _import_xml_node_to_parent(
else:
# We have to handle the children ourselves, because there are lots of complex interactions between
# * the vanilla XBlock parse_xml() method, and its lack of API for "create and save a new XBlock"
# * the XmlMixin version of parse_xml() which only works with ImportSystem, not modulestore or the v2 runtime
# * the XmlMixin version of parse_xml() which only works with XMLImportingModuleStoreRuntime,
# not modulestore or the v2 runtime
# * the modulestore APIs for creating and saving a new XBlock, which work but don't support XML parsing.
# We can safely assume that if the XBLock class supports children, every child node will be the XML
# serialization of a child block, in order. For blocks that don't support children, their XML content/nodes
Expand Down
8 changes: 4 additions & 4 deletions cms/djangoapps/contentstore/views/tests/test_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -1845,7 +1845,7 @@ def setUp(self):

@XBlockAside.register_temp_plugin(AsideTest, "test_aside")
@patch(
"xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types",
"xmodule.modulestore.split_mongo.runtime.SplitModuleStoreRuntime.applicable_aside_types",
lambda self, block: ["test_aside"],
)
def test_duplicate_equality_with_asides(self):
Expand Down Expand Up @@ -2690,8 +2690,8 @@ def test_add_groups(self):
group_id_to_child = split_test.group_id_to_child.copy()
self.assertEqual(2, len(group_id_to_child))

# CachingDescriptorSystem is used in tests.
# CachingDescriptorSystem doesn't have user service, that's needed for
# SplitModuleStoreRuntime is used in tests.
# SplitModuleStoreRuntime doesn't have user service, that's needed for
# SplitTestBlock. So, in this line of code we add this service manually.
split_test.runtime._services["user"] = DjangoXBlockUserService( # pylint: disable=protected-access
self.user
Expand Down Expand Up @@ -4375,7 +4375,7 @@ def test_self_paced_item_visibility_state(self):


@patch(
"xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types",
"xmodule.modulestore.split_mongo.runtime.SplitModuleStoreRuntime.applicable_aside_types",
lambda self, block: ["test_aside"],
)
class TestUpdateFromSource(ModuleStoreTestCase):
Expand Down
4 changes: 2 additions & 2 deletions common/djangoapps/static_replace/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ReplaceURLService(Service):
A service for replacing static/course/jump-to-id URLs with absolute URLs in XBlocks.

Args:
block: (optional) An XBlock instance. Used when retrieving the service from the DescriptorSystem.
block: (optional) An XBlock instance. Used when retrieving the service from the ModuleStoreRuntime.
static_asset_path: (optional) Path for static assets, which overrides data_directory and course_id, if nonempty
static_paths_out: (optional) Array to collect tuples for each static URI found:
* the original unmodified static URI
Expand All @@ -39,7 +39,7 @@ def __init__(
self.jump_to_id_base_url = jump_to_id_base_url
self.lookup_asset_url = lookup_asset_url
# This is needed because the `Service` class initialization expects the XBlock passed as an `xblock` keyword
# argument, but the `service` method from the `DescriptorSystem` passes a `block`.
# argument, but the `service` method from the `ModuleStoreRuntime` passes a `block`.
self._xblock = self.xblock() or block

def replace_urls(self, text, static_replace_only=False):
Expand Down
7 changes: 3 additions & 4 deletions docs/decisions/0020-upstream-downstream.rst
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,7 @@ To support the Libraries Relaunch in Sumac:
Video blocks.

* We will define method(s) for syncing update on the XBlock runtime so that
they are available in the SplitModuleStore's XBlock Runtime
(CachingDescriptorSystem).
they are available in the SplitModuleStoreRuntime.

* Either in the initial implementation or in a later implementation, it may
make sense to declare abstract versions of the syncing method(s) higher up
Expand Down Expand Up @@ -350,10 +349,10 @@ change through development and code review.


###########################################################################
# xmodule/modulestore/split_mongo/caching_descriptor_system.py
# xmodule/modulestore/split_mongo/runtime.py
###########################################################################

class CachingDescriptorSystem(...):
class SplitModuleStoreRuntime(...):

def validate_upstream_key(self, usage_key: UsageKey | str) -> UsageKey:
"""
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class LmsModuleRenderError(Exception):
def make_track_function(request):
'''
Make a tracking function that logs what happened.
For use in DescriptorSystem.
For use in ModuleStoreRuntime.
'''
from common.djangoapps.track import views as track_views

Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/tests/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class BaseTestXmodule(ModuleStoreTestCase):

def new_module_runtime(self, runtime=None, **kwargs):
"""
Generate a new DescriptorSystem that is minimally set up for testing
Generate a new ModuleStoreRuntime that is minimally set up for testing
"""
if runtime:
return prepare_block_runtime(runtime, course_id=self.course.id, **kwargs)
Expand Down
17 changes: 10 additions & 7 deletions lms/djangoapps/courseware/tests/test_block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
from xmodule.modulestore.tests.test_asides import AsideTestType # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.services import RebindUserServiceError
from xmodule.video_block import VideoBlock # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.x_module import STUDENT_VIEW, DescriptorSystem # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.x_module import STUDENT_VIEW, ModuleStoreRuntime # lint-amnesty, pylint: disable=wrong-import-order
from common.djangoapps.course_modes.models import CourseMode # lint-amnesty, pylint: disable=reimported
from common.djangoapps.student.tests.factories import (
BetaTesterFactory,
Expand Down Expand Up @@ -461,8 +461,11 @@ def test_rebinding_same_user(self, block_type):
@override_settings(FIELD_OVERRIDE_PROVIDERS=(
'lms.djangoapps.courseware.student_field_overrides.IndividualStudentOverrideProvider',
))
@patch('xmodule.modulestore.xml.ImportSystem.applicable_aside_types', lambda self, block: ['test_aside'])
@patch('xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types',
@patch(
'xmodule.modulestore.xml.XMLImportingModuleStoreRuntime.applicable_aside_types',
lambda self, block: ['test_aside']
)
@patch('xmodule.modulestore.split_mongo.runtime.SplitModuleStoreRuntime.applicable_aside_types',
lambda self, block: ['test_aside'])
@XBlockAside.register_temp_plugin(AsideTestType, 'test_aside')
@ddt.data('regular', 'test_aside')
Expand Down Expand Up @@ -1920,7 +1923,7 @@ def _get_anonymous_id(self, course_id, xblock_class, should_get_deprecated_id: b
location=location,
static_asset_path=None,
_runtime=Mock(
spec=DescriptorSystem,
spec=ModuleStoreRuntime,
resources_fs=None,
mixologist=Mock(_mixins=(), name='mixologist'),
_services={},
Expand All @@ -1933,7 +1936,7 @@ def _get_anonymous_id(self, course_id, xblock_class, should_get_deprecated_id: b
fields={},
days_early_for_beta=None,
)
block.runtime = DescriptorSystem(None, None, None)
block.runtime = ModuleStoreRuntime(None, None, None)
# Use the xblock_class's bind_for_student method
block.bind_for_student = partial(xblock_class.bind_for_student, block)

Expand Down Expand Up @@ -2006,9 +2009,9 @@ def test_context_contains_display_name(self, mock_tracker):
assert problem_display_name == block_info['display_name']

@XBlockAside.register_temp_plugin(AsideTestType, 'test_aside')
@patch('xmodule.modulestore.mongo.base.CachingDescriptorSystem.applicable_aside_types',
@patch('xmodule.modulestore.mongo.base.OldModuleStoreRuntime.applicable_aside_types',
lambda self, block: ['test_aside'])
@patch('xmodule.x_module.DescriptorSystem.applicable_aside_types',
@patch('xmodule.x_module.ModuleStoreRuntime.applicable_aside_types',
lambda self, block: ['test_aside'])
def test_context_contains_aside_info(self, mock_tracker):
"""
Expand Down
12 changes: 6 additions & 6 deletions lms/djangoapps/courseware/tests/test_video_mongo.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE
# noinspection PyUnresolvedReferences
from xmodule.tests.helpers import override_descriptor_system # pylint: disable=unused-import
from xmodule.tests.test_import import DummySystem
from xmodule.tests.test_import import DummyModuleStoreRuntime
from xmodule.tests.test_video import VideoBlockTestBase
from xmodule.video_block import VideoBlock, bumper_utils, video_utils
from xmodule.video_block.transcripts_utils import Transcript, save_to_store, subs_filename
Expand Down Expand Up @@ -1971,7 +1971,7 @@ def test_import_val_data_internal(self):
Test that import val data internal works as expected.
"""
create_profile('mobile')
module_system = DummySystem(load_error_blocks=True)
module_system = DummyModuleStoreRuntime(load_error_blocks=True)

edx_video_id = 'test_edx_video_id'
sub_id = '0CzPOIIdUsA'
Expand Down Expand Up @@ -2074,7 +2074,7 @@ def test_import_no_video_id(self):
"""
xml_data = """<video><video_asset></video_asset></video>"""
xml_object = etree.fromstring(xml_data)
module_system = DummySystem(load_error_blocks=True)
module_system = DummyModuleStoreRuntime(load_error_blocks=True)

# Verify edx_video_id is empty before.
assert self.block.edx_video_id == ''
Expand Down Expand Up @@ -2110,7 +2110,7 @@ def test_import_val_transcript(self):
val_transcript_provider=val_transcript_provider
)
xml_object = etree.fromstring(xml_data)
module_system = DummySystem(load_error_blocks=True)
module_system = DummyModuleStoreRuntime(load_error_blocks=True)

# Create static directory in import file system and place transcript files inside it.
module_system.resources_fs.makedirs(EXPORT_IMPORT_STATIC_DIR, recreate=True)
Expand Down Expand Up @@ -2215,7 +2215,7 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_
edx_video_id = 'test_edx_video_id'
language_code = 'en'

module_system = DummySystem(load_error_blocks=True)
module_system = DummyModuleStoreRuntime(load_error_blocks=True)

# Create static directory in import file system and place transcript files inside it.
module_system.resources_fs.makedirs(EXPORT_IMPORT_STATIC_DIR, recreate=True)
Expand Down Expand Up @@ -2283,7 +2283,7 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_

def test_import_val_data_invalid(self):
create_profile('mobile')
module_system = DummySystem(load_error_blocks=True)
module_system = DummyModuleStoreRuntime(load_error_blocks=True)

# Negative file_size is invalid
xml_data = """
Expand Down
4 changes: 2 additions & 2 deletions lms/djangoapps/lms_xblock/test/test_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from opaque_keys.edx.locations import BlockUsageLocator, CourseLocator
from xblock.fields import ScopeIds

from xmodule.x_module import DescriptorSystem
from xmodule.x_module import ModuleStoreRuntime
from lms.djangoapps.lms_xblock.runtime import handler_url


Expand Down Expand Up @@ -51,7 +51,7 @@ class TestHandlerUrl(TestCase):
def setUp(self):
super().setUp()
self.block = BlockMock(name='block')
self.runtime = DescriptorSystem(
self.runtime = ModuleStoreRuntime(
load_item=Mock(name='get_test_descriptor_system.load_item'),
resources_fs=Mock(name='get_test_descriptor_system.resources_fs'),
error_tracker=Mock(name='get_test_descriptor_system.error_tracker')
Expand Down
4 changes: 2 additions & 2 deletions openedx/core/djangoapps/xblock/runtime/shims.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ def process_xml(self, xml):
"""
# We can't parse XML in a vacuum - we need to know the parent block and/or the
# OLX file that holds this XML in order to generate useful definition keys etc.
# The older ImportSystem runtime could do this because it stored the course_id
# The older XMLImportingModuleStoreRuntime runtime could do this because it stored the course_id
# as part of the runtime.
raise NotImplementedError("This newer runtime does not support process_xml()")

Expand Down Expand Up @@ -244,7 +244,7 @@ def xqueue(self):

def get_field_provenance(self, xblock, field):
"""
A Studio-specific method that was implemented on DescriptorSystem.
A Studio-specific method that was implemented on ModuleStoreRuntime.
Used by the problem block.

For the given xblock, return a dict for the field's current state:
Expand Down
5 changes: 4 additions & 1 deletion openedx/core/lib/tests/test_xblock_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,10 @@ def test_is_not_xblock_aside(self):
"""test if xblock is not aside"""
assert is_xblock_aside(self.block.scope_ids.usage_id) is False

@patch('xmodule.modulestore.xml.ImportSystem.applicable_aside_types', lambda self, block: ['test_aside'])
@patch(
'xmodule.modulestore.xml.XMLImportingModuleStoreRuntime.applicable_aside_types',
lambda self, block: ['test_aside'],
)
@XBlockAside.register_temp_plugin(AsideTestType, 'test_aside')
def test_get_aside(self):
"""test get aside success"""
Expand Down
2 changes: 1 addition & 1 deletion xmodule/capa/capa_problem.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class LoncapaSystem(object):
i18n: an object implementing the `gettext.Translations` interface so
that we can use `.ugettext` to localize strings.

See :class:`DescriptorSystem` for documentation of other attributes.
See :class:`ModuleStoreRuntime` for documentation of other attributes.

"""
def __init__(
Expand Down
2 changes: 1 addition & 1 deletion xmodule/error_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def _construct(cls, system, contents, error_msg, location, for_parent=None):
Build a new ErrorBlock using ``system``.

Arguments:
system (:class:`DescriptorSystem`): The :class:`DescriptorSystem` used
system (:class:`ModuleStoreRuntime`): The :class:`ModuleStoreRuntime` used
to construct the XBlock that had an error.
contents (unicode): An encoding of the content of the xblock that had an error.
error_msg (unicode): A message describing the error.
Expand Down
25 changes: 1 addition & 24 deletions xmodule/mako_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,7 @@

from web_fragments.fragment import Fragment

from .x_module import DescriptorSystem, shim_xmodule_js


class MakoDescriptorSystem(DescriptorSystem): # lint-amnesty, pylint: disable=abstract-method
"""
Descriptor system that renders mako templates.
"""
def __init__(self, render_template, **kwargs):
super().__init__(**kwargs)

self.render_template = render_template

# Add the MakoService to the runtime services.
# If it already exists, do not attempt to reinitialize it; otherwise, this could override the `namespace_prefix`
# of the `MakoService`, breaking template rendering in Studio.
#
# This is not needed by most XBlocks, because the MakoService is added to their runtimes.
# However, there are a few cases where the MakoService is not added to the XBlock's
# runtime. Specifically:
# * in the Instructor Dashboard bulk emails tab, when rendering the HtmlBlock for its WYSIWYG editor.
# * during testing, when fetching factory-created blocks.
if 'mako' not in self._services:
from common.djangoapps.edxmako.services import MakoService
self._services['mako'] = MakoService()
from .x_module import shim_xmodule_js


class MakoTemplateBlockBase:
Expand Down
Loading
Loading