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.

Sign up for GitHub

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

upgrade xblock==2.0 #34398

Merged
merged 8 commits into from
Apr 8, 2024
+92 −99
Merged

upgrade xblock==2.0 #34398

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3faa773
feat: add support for xblock 2
irtazaakram Apr 1, 2024
834384c
fix: runtim id_generator issue
irtazaakram Apr 1, 2024
acc86dd
fix: review changes
irtazaakram Apr 2, 2024
79a4327
Merge branch 'master' into xblock2
irtazaakram Apr 3, 2024
682a80a
fix: review changes
irtazaakram Apr 3, 2024
c810a1b
Merge branch 'master' into xblock2
irtazaakram Apr 4, 2024
49f5a4f
fix: pylint and unit test
irtazaakram Apr 4, 2024
d9458ce
fix: remove print statements
irtazaakram Apr 4, 2024
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
1 change: 1 addition & 0 deletions .github/workflows/unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ jobs:
if: (github.repository == 'edx/edx-platform-private') || (github.repository == 'openedx/edx-platform' && (startsWith(github.base_ref, 'open-release') == false))
runs-on: [ edx-platform-runner ]
strategy:
fail-fast: false
irtazaakram marked this conversation as resolved.
Show resolved Hide resolved
matrix:
python-version:
- "3.8"
Expand Down
10 changes: 5 additions & 5 deletions cms/djangoapps/contentstore/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,9 @@ def _import_xml_node_to_parent(
block_type = node.tag

# Generate the new ID:
id_generator = ImportIdGenerator(parent_key.context_key)
def_id = id_generator.create_definition(block_type, slug_hint)
usage_id = id_generator.create_usage(def_id)
runtime.id_generator = ImportIdGenerator(parent_key.context_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we saving this as an attribute of the runtime? I don't think we want to update the runtime id_generator in perpetuity just use a different one here at import time right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this specific case runtime was using SplitMongoIdManager instead of ImportIdGenerator.

Details: Unit test failure : Raw Logs

In general id_generator should be assigned by runtime but for every case where a custom id_generator was required we have to pass it through attribute. i.e. runtime.id_generator

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhhh, that is interesting. I guess it makes sense--the runtime here is the general LMS/CMS runtime (CachingDescriptorSystem), so by default, its id_generator is not going to match ImportSystem's id_generator. Thanks for pointing that out @irtazaakram .

Setting it here is a little awkward, but I don't know of a better solution currently.

In order to contain the impact of this, do you think it would be reasonable to set the original id_generator to a temporary value, and then restore it? If you agree, could you make that change, and add a comment explaining why we need to do it?

original_id_generator = runtime.id_generator
runtime.id_generator = ImportIdGenerator(...)
...
runtime.id_generator = original_id_generator

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. 682a80a

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, ty 👍🏻

def_id = runtime.id_generator.create_definition(block_type, slug_hint)
usage_id = runtime.id_generator.create_usage(def_id)
keys = ScopeIds(None, block_type, def_id, usage_id)
# parse_xml is a really messy API. We pass both 'keys' and 'id_generator' and, depending on the XBlock, either
# one may be used to determine the new XBlock's usage key, and the other will be ignored. e.g. video ignores
Expand All @@ -344,7 +344,7 @@ def _import_xml_node_to_parent(

if not xblock_class.has_children:
# No children to worry about. The XML may contain child nodes, but they're not XBlocks.
temp_xblock = xblock_class.parse_xml(node, runtime, keys, id_generator)
temp_xblock = xblock_class.parse_xml(node, runtime, keys)
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"
Expand All @@ -354,7 +354,7 @@ def _import_xml_node_to_parent(
# serialization of a child block, in order. For blocks that don't support children, their XML content/nodes
# could be anything (e.g. HTML, capa)
node_without_children = etree.Element(node.tag, **node.attrib)
temp_xblock = xblock_class.parse_xml(node_without_children, runtime, keys, id_generator)
temp_xblock = xblock_class.parse_xml(node_without_children, runtime, keys)
child_nodes = list(node)
if xblock_class.has_children and temp_xblock.children:
raise NotImplementedError("We don't yet support pasting XBlocks with children")
Expand Down
14 changes: 6 additions & 8 deletions lms/djangoapps/courseware/tests/test_video_mongo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2027,7 +2027,8 @@ def test_import_val_data_internal(self):
xml_object = etree.fromstring(xml_data)
id_generator = Mock()
id_generator.target_course_id = "test_course_id"
video = self.block.parse_xml(xml_object, module_system, None, id_generator)
module_system.id_generator = id_generator
video = self.block.parse_xml(xml_object, module_system, None)

assert video.edx_video_id == 'test_edx_video_id'
video_data = get_video_info(video.edx_video_id)
Expand Down Expand Up @@ -2075,12 +2076,11 @@ 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)
id_generator = Mock()

# Verify edx_video_id is empty before.
assert self.block.edx_video_id == ''

video = self.block.parse_xml(xml_object, module_system, None, id_generator)
video = self.block.parse_xml(xml_object, module_system, None)

# Verify edx_video_id is populated after the import.
assert video.edx_video_id != ''
Expand Down Expand Up @@ -2112,7 +2112,6 @@ def test_import_val_transcript(self):
)
xml_object = etree.fromstring(xml_data)
module_system = DummySystem(load_error_blocks=True)
id_generator = Mock()

# 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 All @@ -2128,7 +2127,7 @@ def test_import_val_transcript(self):
# Verify edx_video_id is empty before.
assert self.block.edx_video_id == ''

video = self.block.parse_xml(xml_object, module_system, None, id_generator)
video = self.block.parse_xml(xml_object, module_system, None)

# Verify edx_video_id is populated after the import.
assert video.edx_video_id != ''
Expand Down Expand Up @@ -2218,7 +2217,6 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_
language_code = 'en'

module_system = DummySystem(load_error_blocks=True)
id_generator = Mock()

# 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 @@ -2270,7 +2268,7 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_
# Verify edx_video_id is empty before import.
assert self.block.edx_video_id == ''

video = self.block.parse_xml(xml_object, module_system, None, id_generator)
video = self.block.parse_xml(xml_object, module_system, None)

# Verify edx_video_id is not empty after import.
assert video.edx_video_id != ''
Expand Down Expand Up @@ -2298,7 +2296,7 @@ def test_import_val_data_invalid(self):
"""
xml_object = etree.fromstring(xml_data)
with pytest.raises(ValCannotCreateError):
VideoBlock.parse_xml(xml_object, module_system, None, id_generator=Mock())
VideoBlock.parse_xml(xml_object, module_system, None)
with pytest.raises(ValVideoNotFoundError):
get_video_info("test_edx_video_id")

Expand Down
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def get_block(self, usage_key, for_parent=None):
# plus some minor additional lines of code as needed.
block = block_class.parse_xml_new_runtime(xml_node, runtime=self, keys=keys)
else:
block = block_class.parse_xml(xml_node, runtime=self, keys=keys, id_generator=None)
block = block_class.parse_xml(xml_node, runtime=self, keys=keys)

# Update field data with parsed values. We can't call .save() because it will call save_block(), below.
block.force_save_fields(block._get_fields_to_save()) # pylint: disable=protected-access
Expand Down
4 changes: 2 additions & 2 deletions openedx/core/djangoapps/xblock/runtime/runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,11 +217,11 @@ def applicable_aside_types(self, block: XBlock):
""" Disable XBlock asides in this runtime """
return []

def parse_xml_file(self, fileobj, id_generator=None):
def parse_xml_file(self, fileobj):
# Deny access to the inherited method
raise NotImplementedError("XML Serialization is only supported with BlockstoreXBlockRuntime")

def add_node_as_child(self, block, node, id_generator=None):
def add_node_as_child(self, block, node):
"""
Called by XBlock.parse_xml to treat a child node as a child block.
"""
Expand Down
8 changes: 4 additions & 4 deletions requirements/edx/base.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#
-e git+https://github.com/anupdhabarde/edx-proctoring-proctortrack.git@31c6c9923a51c903ae83760ecbbac191363aa2a2#egg=edx_proctoring_proctortrack
# via -r requirements/edx/github.in
acid-xblock==0.2.1
acid-xblock==0.3.0
# via -r requirements/edx/kernel.in
aiohttp==3.9.3
# via
Expand Down Expand Up @@ -528,7 +528,7 @@ edx-rest-api-client==5.6.1
# edx-proctoring
edx-search==3.8.2
# via -r requirements/edx/kernel.in
edx-sga==0.23.1
edx-sga==0.24.1
# via -r requirements/edx/bundled.in
edx-submissions==3.6.0
# via
Expand Down Expand Up @@ -804,7 +804,7 @@ optimizely-sdk==4.1.1
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/bundled.in
ora2==6.5.1
ora2==6.6.1
# via -r requirements/edx/bundled.in
packaging==23.2
# via
Expand Down Expand Up @@ -1222,7 +1222,7 @@ webob==1.8.7
# xblock
wrapt==1.16.0
# via -r requirements/edx/paver.txt
xblock[django]==1.10.0
xblock[django]==2.0.0
# via
# -r requirements/edx/kernel.in
# acid-xblock
Expand Down
8 changes: 4 additions & 4 deletions requirements/edx/development.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ accessible-pygments==0.0.4
# via
# -r requirements/edx/doc.txt
# pydata-sphinx-theme
acid-xblock==0.2.1
acid-xblock==0.3.0
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
Expand Down Expand Up @@ -825,7 +825,7 @@ edx-search==3.8.2
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
edx-sga==0.23.1
edx-sga==0.24.1
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
Expand Down Expand Up @@ -1333,7 +1333,7 @@ optimizely-sdk==4.1.1
# -c requirements/edx/../constraints.txt
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
ora2==6.5.1
ora2==6.6.1
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
Expand Down Expand Up @@ -2185,7 +2185,7 @@ wrapt==1.16.0
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
# astroid
xblock[django]==1.10.0
xblock[django]==2.0.0
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
Expand Down
8 changes: 4 additions & 4 deletions requirements/edx/doc.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
# via -r requirements/edx/base.txt
accessible-pygments==0.0.4
# via pydata-sphinx-theme
acid-xblock==0.2.1
acid-xblock==0.3.0
# via -r requirements/edx/base.txt
aiohttp==3.9.3
# via
Expand Down Expand Up @@ -607,7 +607,7 @@ edx-rest-api-client==5.6.1
# edx-proctoring
edx-search==3.8.2
# via -r requirements/edx/base.txt
edx-sga==0.23.1
edx-sga==0.24.1
# via -r requirements/edx/base.txt
edx-submissions==3.6.0
# via
Expand Down Expand Up @@ -942,7 +942,7 @@ optimizely-sdk==4.1.1
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
ora2==6.5.1
ora2==6.6.1
# via -r requirements/edx/base.txt
packaging==23.2
# via
Expand Down Expand Up @@ -1488,7 +1488,7 @@ webob==1.8.7
# xblock
wrapt==1.16.0
# via -r requirements/edx/base.txt
xblock[django]==1.10.0
xblock[django]==2.0.0
# via
# -r requirements/edx/base.txt
# acid-xblock
Expand Down
8 changes: 4 additions & 4 deletions requirements/edx/testing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#
-e git+https://github.com/anupdhabarde/edx-proctoring-proctortrack.git@31c6c9923a51c903ae83760ecbbac191363aa2a2#egg=edx_proctoring_proctortrack
# via -r requirements/edx/base.txt
acid-xblock==0.2.1
acid-xblock==0.3.0
# via -r requirements/edx/base.txt
aiohttp==3.9.3
# via
Expand Down Expand Up @@ -633,7 +633,7 @@ edx-rest-api-client==5.6.1
# edx-proctoring
edx-search==3.8.2
# via -r requirements/edx/base.txt
edx-sga==0.23.1
edx-sga==0.24.1
# via -r requirements/edx/base.txt
edx-submissions==3.6.0
# via
Expand Down Expand Up @@ -997,7 +997,7 @@ optimizely-sdk==4.1.1
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
ora2==6.5.1
ora2==6.6.1
# via -r requirements/edx/base.txt
packaging==23.2
# via
Expand Down Expand Up @@ -1604,7 +1604,7 @@ wrapt==1.16.0
# via
# -r requirements/edx/base.txt
# astroid
xblock[django]==1.10.0
xblock[django]==2.0.0
# via
# -r requirements/edx/base.txt
# acid-xblock
Expand Down
4 changes: 2 additions & 2 deletions xmodule/course_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,8 +1166,8 @@ def read_grading_policy(cls, paths, system):
return policy_str

@classmethod
def parse_xml(cls, node, runtime, keys, id_generator):
instance = super().parse_xml(node, runtime, keys, id_generator)
def parse_xml(cls, node, runtime, keys):
instance = super().parse_xml(node, runtime, keys)

policy_dir = None
url_name = node.get('url_name')
Expand Down
4 changes: 2 additions & 2 deletions xmodule/discussion_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ def student_view_data(self):
return {'topic_id': self.discussion_id}

@classmethod
def parse_xml(cls, node, runtime, keys, id_generator):
def parse_xml(cls, node, runtime, keys):
"""
Parses OLX into XBlock.

Expand All @@ -246,7 +246,7 @@ def parse_xml(cls, node, runtime, keys, id_generator):
XBlock.parse_xml. Otherwise this method parses file in "discussion" folder (known as definition_xml), applies
policy.json and updates fields accordingly.
"""
block = super().parse_xml(node, runtime, keys, id_generator)
block = super().parse_xml(node, runtime, keys)

cls._apply_metadata_and_policy(block, node, runtime)

Expand Down
4 changes: 2 additions & 2 deletions xmodule/error_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,13 @@ def from_xml(cls, xml_data, system, id_generator, # pylint: disable=arguments-d
return cls._construct(system, xml_data, error_msg, location=id_generator.create_definition('error'))

@classmethod
def parse_xml(cls, node, runtime, keys, id_generator): # lint-amnesty, pylint: disable=unused-argument
def parse_xml(cls, node, runtime, keys): # lint-amnesty, pylint: disable=unused-argument
"""
Interpret the parsed XML in `node`, creating an XModuleDescriptor.
"""
# It'd be great to not reserialize and deserialize the xml
xml = etree.tostring(node).decode('utf-8')
block = cls.from_xml(xml, runtime, id_generator)
block = cls.from_xml(xml, runtime, runtime.id_generator)
return block

def export_to_xml(self, resource_fs):
Expand Down
2 changes: 1 addition & 1 deletion xmodule/modulestore/xml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ def construct_xblock_from_class(self, cls, scope_ids, field_data=None, *args, **

# id_generator is ignored, because each ImportSystem is already local to
# a course, and has it's own id_generator already in place
def add_node_as_child(self, block, node, id_generator): # lint-amnesty, pylint: disable=signature-differs
def add_node_as_child(self, block, node): # lint-amnesty, pylint: disable=signature-differs
child_block = self.process_xml(etree.tostring(node))
block.children.append(child_block.scope_ids.usage_id)

Expand Down
2 changes: 1 addition & 1 deletion xmodule/raw_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def parse_xml_new_runtime(cls, node, runtime, keys):
try:
block = super().parse_xml_new_runtime(node, runtime, keys)
except AttributeError:
block = super().parse_xml(node, runtime, keys, id_generator=None)
block = super().parse_xml(node, runtime, keys)
block.data = data_field_value
return block

Expand Down
2 changes: 1 addition & 1 deletion xmodule/template_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ class TranslateCustomTagBlock( # pylint: disable=abstract-method
resources_dir = None

@classmethod
def parse_xml(cls, node, runtime, _keys, _id_generator):
def parse_xml(cls, node, runtime, _keys):
"""
Transforms the xml_data from <$custom_tag attr="" attr=""/> to
<customtag attr="" attr="" impl="$custom_tag"/>
Expand Down
6 changes: 3 additions & 3 deletions xmodule/tests/test_discussion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def setUp(self):
self.runtime_mock = mock.Mock(spec=Runtime)
self.runtime_mock.construct_xblock_from_class = mock.Mock(side_effect=self._construct_xblock_mock)
self.runtime_mock.get_policy = mock.Mock(return_value={})
self.id_gen_mock = mock.Mock()
self.id_generator = mock.Mock()
irtazaakram marked this conversation as resolved.
Show resolved Hide resolved

def _construct_xblock_mock(self, cls, keys): # pylint: disable=unused-argument
"""
Expand Down Expand Up @@ -102,7 +102,7 @@ def test_xblock_export_format(self, id_pair, category_pair, target_pair, patched

patched_load_definition_xml.side_effect = Exception("Irrelevant")

block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys, self.id_gen_mock)
block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys)
try:
assert block.discussion_id == id_pair.value
assert block.discussion_category == category_pair.value
Expand Down Expand Up @@ -134,7 +134,7 @@ def test_legacy_export_format(self, id_pair, category_pair, target_pair, patched

patched_load_definition_xml.return_value = (definition_node, "irrelevant")

block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys, self.id_gen_mock)
block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys)
try:
assert block.discussion_id == id_pair.value
assert block.discussion_category == category_pair.value
Expand Down
4 changes: 2 additions & 2 deletions xmodule/tests/test_library_content.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ def test_xml_export_import_cycle(self):

# Now import it.
olx_element = etree.fromstring(exported_olx)
imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None, self.id_generator)
imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None)

self._verify_xblock_properties(imported_lc_block)

Expand All @@ -219,7 +219,7 @@ def test_xml_import_with_comments(self):

# Import the olx.
olx_element = etree.fromstring(olx_with_comments)
imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None, self.id_generator)
imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None)

self._verify_xblock_properties(imported_lc_block)

Expand Down
3 changes: 2 additions & 1 deletion xmodule/tests/test_poll.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ def test_poll_export_with_unescaped_characters_xml(self):
'''
node = etree.fromstring(sample_poll_xml)

output = PollBlock.parse_xml(node, module_system, self.scope_ids, id_generator)
module_system.id_generator = id_generator
output = PollBlock.parse_xml(node, module_system, self.scope_ids)
# Update the answer with invalid character.
invalid_characters_poll_answer = output.answers[0]
# Invalid less-than character.
Expand Down
Loading
Loading