From 05b410606b5b5c336d8a9cee47f2c09f99aed3d6 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 17 Oct 2023 13:07:36 -0400 Subject: [PATCH 1/4] feat: first iteration of migration script for blockstore to openedx-learning models --- cms/envs/common.py | 15 ++ lms/envs/common.py | 17 +- .../djangoapps/content_libraries/admin.py | 6 +- .../core/djangoapps/content_libraries/api.py | 38 +++- .../commands/migrate_lib_to_learning_core.py | 202 ++++++++++++++++++ .../djangoapps/content_libraries/models.py | 14 ++ .../djangoapps/content_libraries/views.py | 3 + .../xblock/runtime/blockstore_runtime.py | 80 ++++++- openedx/core/lib/blockstore_api/methods.py | 1 + 9 files changed, 366 insertions(+), 10 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 179eede100b1..ea6204d11ed8 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1811,9 +1811,24 @@ # alternative swagger generator for CMS API 'drf_spectacular', + 'openedx_events', + + # Learning Core Apps + "openedx_learning.core.components.apps.ComponentsConfig", + "openedx_learning.core.contents.apps.ContentsConfig", + "openedx_learning.core.publishing.apps.PublishingConfig", + + # Learning Contrib Apps + # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] +OPENEDX_LEARNING = { + # Custom file storage, though this is better done through Django's + # STORAGES setting in Django >= 4.2 + "STORAGE": None, +} + ################# EDX MARKETING SITE ################################## diff --git a/lms/envs/common.py b/lms/envs/common.py index 0bad10728565..39b65cbaec47 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -40,7 +40,6 @@ # and throws spurious errors. Therefore, we disable invalid-name checking. # pylint: disable=invalid-name - import importlib.util import sys import os @@ -3316,9 +3315,25 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # Notifications 'openedx.core.djangoapps.notifications', + + 'openedx_events', + + # Learning Core Apps + "openedx_learning.core.components.apps.ComponentsConfig", + "openedx_learning.core.contents.apps.ContentsConfig", + "openedx_learning.core.publishing.apps.PublishingConfig", + + # Learning Contrib Apps + # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] +OPENEDX_LEARNING = { + # Custom file storage, though this is better done through Django's + # STORAGES setting in Django >= 4.2 + "STORAGE": None, +} + ######################### CSRF ######################################### # Forwards-compatibility with Django 1.7 diff --git a/openedx/core/djangoapps/content_libraries/admin.py b/openedx/core/djangoapps/content_libraries/admin.py index 559d2471cee3..e715841187e6 100644 --- a/openedx/core/djangoapps/content_libraries/admin.py +++ b/openedx/core/djangoapps/content_libraries/admin.py @@ -2,7 +2,7 @@ Admin site for content libraries """ from django.contrib import admin -from .models import ContentLibrary, ContentLibraryPermission +from .models import ContentLibrary, ContentLibraryLearningPackage, ContentLibraryPermission class ContentLibraryPermissionInline(admin.TabularInline): @@ -40,3 +40,7 @@ def get_readonly_fields(self, request, obj=None): return ["library_key", "org", "slug", "bundle_uuid"] else: return ["library_key", ] + +@admin.register(ContentLibraryLearningPackage) +class ContentLibraryLearningPackageAdmin(admin.ModelAdmin): + pass diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 737673b32e3f..fb2c2af3d074 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -733,7 +733,13 @@ def get_library_block(usage_key): ) -def get_library_block_olx(usage_key): +from openedx_learning.core.components.api import get_component_version_content +from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent +from openedx_learning.core.contents.models import RawContent, TextContent +from openedx_learning.core.publishing.models import Draft, LearningPackage +from django.db.models import Q + +def get_library_block_olx(usage_key: LibraryUsageLocatorV2): """ Get the OLX source of the given XBlock. """ @@ -747,6 +753,36 @@ def get_library_block_olx(usage_key): return xml_str + #xml_str = get_component_version_content( + # str(usage_key.lib_key), + # # xblock.v1:problem@3e8c88ea8ec545b4b3c7066c89527d5e_cvo3nwt344t7ytqts + # f"xblock.v1:{usage_key.definition_key}", + # 1, + # "definition.xml", + #) +# cv_rc = ComponentVersionRawContent.objects.select_related( +# "raw_content", +# "raw_content__text_content" +# "component_version", +# "component_version__component", +# "component_version__component__learning_package", +# ) + # Inefficient but simple approach first + learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) + component = Component.objects.get( + learning_package=learning_package, + namespace='xblock.v1', + type=usage_key.block_type, + local_key=usage_key.block_id, + ) + component_version = component.versioning.draft + text_content = component_version.raw_contents.get(key="definition.xml").text_content + + print("I really executed! Not a hallucination!") + + return text_content.text + + def set_library_block_olx(usage_key, new_olx_str): """ Replace the OLX source of the given XBlock. diff --git a/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py new file mode 100644 index 000000000000..6f9a3f8516b4 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py @@ -0,0 +1,202 @@ +""" +Command to import Blockstore-backed v2 Libraries to Learning Core data models. +""" +from datetime import datetime, timezone +from xml.etree import ElementTree as ET +import logging + +from django.conf import settings +from django.db import transaction +from django.core.management import BaseCommand, CommandError + +from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx.core.djangoapps.content_libraries import api as lib_api +from openedx.core.djangoapps.content_libraries import models as lib_models +from openedx.core.djangoapps.content_libraries import constants as lib_constants +from openedx.core.lib.blockstore_api import ( + get_bundle, + get_bundle_file_metadata, + get_bundle_file_data, + get_bundle_files_dict, +) +from openedx_learning.core.publishing import api as publishing_api +from openedx_learning.core.components import api as components_api +from openedx_learning.core.contents import api as contents_api + +# These imports should all be removed at some point. + +from openedx_learning.core.publishing.models import Draft + + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + Create a LearningPackage and initialize with contents from Library + """ + + def add_arguments(self, parser): + """ + Add arguments to the argument parser. + """ + parser.add_argument( + 'library-key', + type=LibraryLocatorV2.from_string, + help=('Content Library Key to import content from.'), + ) + + def handle(self, *args, **options): + # Search for the library. + lib_key = options['library-key'] + lib_data = lib_api.get_library(lib_key) + lib = lib_models.ContentLibrary.objects.get_by_key(lib_key) + + COMPONENT_NAMESPACE = 'xblock.v1' + + with transaction.atomic(): + # This is a migration script and we're assuming there's no important + # state attached to the LearningPackage yet. That makes it safe to + # just wipe out everything and recreate it. + if hasattr(lib, 'contents'): + lp = lib.contents.learning_package + log.info(f"Deleting existing LearningPackage {lp.key} ({lp.uuid})") + lib.contents.delete() + lp.delete() + + # Initialize a new LearningPackage + learning_package = publishing_api.create_learning_package( + key=lib_key, + title=lib_data.title, + ) + log.info(f"Created LearningPackage {learning_package.key} ({learning_package.uuid})") + lib_models.ContentLibraryLearningPackage.objects.create( + content_library=lib, + learning_package=learning_package, + ) + + # We only really need to get the most recent version in Studio draft + # for now. + bundle = get_bundle(lib.bundle_uuid) + published_files = get_bundle_files_dict(lib.bundle_uuid) + + now = datetime.now(timezone.utc) + + # First get the published version into openedx-learning models. This + # creates ComponentVersions and puts them as the current Draft. + published_metadata_dict = {} + published_component_pks = {} + published_definition_files = { + file_path: metadata + for file_path, metadata in published_files.items() + if file_path.endswith('/definition.xml') + } + for file_path, metadata in published_definition_files.items(): + block_type, block_id, _def_xml = file_path.split('/') + published_metadata_dict[file_path] = metadata + xml_bytes = get_bundle_file_data(bundle.uuid, file_path) + display_name = extract_display_name(xml_bytes, file_path) + + component, component_version = components_api.create_component_and_version( + learning_package_id=learning_package.id, + namespace=COMPONENT_NAMESPACE, + type=block_type, + local_key=block_id, + title=display_name, + created=now, + created_by=None, + ) + published_component_pks[file_path] = component.pk + text_content, _created = contents_api.get_or_create_text_content_from_bytes( + learning_package_id=learning_package.id, + data_bytes=xml_bytes, + mime_type=f"application/vnd.openedx.xblock.v1.{block_type}+xml", + created=now, + ) + components_api.add_content_to_component_version( + component_version, + raw_content_id=text_content.pk, + key="definition.xml", + learner_downloadable=False + ) + # Publish all the Draft versions we created. + publishing_api.publish_all_drafts( + learning_package_id=learning_package.id, + message="Initial import from Blockstore", + published_at=now, + ) + + # Now grab the draft versions from blockstore, and copy those... + draft_files = get_bundle_files_dict(lib.bundle_uuid, use_draft=lib_constants.DRAFT_NAME) + draft_definition_files = { + file_path: metadata + for file_path, metadata in draft_files.items() + if file_path.endswith("definition.xml") + } + for file_path, draft_metadata in draft_definition_files.items(): + published_metadata = published_metadata_dict.get(file_path) + if draft_metadata.modified: + block_type, block_id, def_xml = file_path.split('/') + xml_bytes = get_bundle_file_data(bundle.uuid, file_path, use_draft=lib_constants.DRAFT_NAME) + display_name = extract_display_name(xml_bytes, file_path) + + # If this is newly created in the draft... + if published_metadata is None: + component = components_api.create_component( + learning_package_id=learning_package.id, + namespace=COMPONENT_NAMESPACE, + type=block_type, + local_key=block_id, + created=now, + created_by=None, + ) + component_pk = component.pk + version_num = 1 + # Otherwise, it's been modified... + else: + component_pk = published_component_pks[file_path] + version_num = 2 + + component_version = components_api.create_component_version( + component_pk=component_pk, + version_num=version_num, + title=display_name, + created=now, + created_by=None, + ) + text_content, _created = contents_api.get_or_create_text_content_from_bytes( + learning_package_id=learning_package.id, + data_bytes=xml_bytes, + mime_type=f"application/vnd.openedx.xblock.v1.{block_type}+xml", + created=now, + ) + components_api.add_content_to_component_version( + component_version, + raw_content_id=text_content.pk, + key="definition.xml", + learner_downloadable=False + ) + + # Now remove stuff that was present in the published set but was + # deleted in the draft. + deleted_definition_files = set(published_definition_files) - set(draft_definition_files) + for deleted_definition_file in deleted_definition_files: + log.info(f"Deleting {deleted_definition_file} from draft") + component_pk = published_component_pks[deleted_definition_file] + draft = Draft.objects.get(entity_id=component_pk) + draft.version = None + draft.save() + + +def extract_display_name(xml_bytes, file_path): + # Do some basic parsing of the content to see if it's even well + # constructed enough to add (or whether we should skip/error on it). + try: + xml_str = xml_bytes.decode('utf-8') + block_root = ET.fromstring(xml_str) + display_name = block_root.attrib.get("display_name", "") + except ET.ParseError as err: + log.error(f"Parse error for {file_path}: {err}") + display_name = "" + + return display_name diff --git a/openedx/core/djangoapps/content_libraries/models.py b/openedx/core/djangoapps/content_libraries/models.py index 7bf8792699f9..7970c9097d7f 100644 --- a/openedx/core/djangoapps/content_libraries/models.py +++ b/openedx/core/djangoapps/content_libraries/models.py @@ -56,6 +56,7 @@ LIBRARY_TYPES, COMPLEX, LICENSE_OPTIONS, ALL_RIGHTS_RESERVED, ) +from openedx_learning.core.publishing.models import LearningPackage from organizations.models import Organization # lint-amnesty, pylint: disable=wrong-import-order from .apps import ContentLibrariesConfig @@ -527,3 +528,16 @@ def update_score(self, weighted_earned, weighted_possible, timestamp): def __str__(self): return str(self.usage_key) + + +class ContentLibraryLearningPackage(models.Model): + content_library = models.OneToOneField( + ContentLibrary, + primary_key=True, + on_delete=models.CASCADE, + related_name='contents', + ) + learning_package = models.ForeignKey( + LearningPackage, + on_delete=models.RESTRICT + ) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index c83a9c4fcb56..76221a878de5 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -680,6 +680,9 @@ def get(self, request, usage_key_str): """ key = LibraryUsageLocatorV2.from_string(usage_key_str) api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + + print("You called me!") + xml_str = api.get_library_block_olx(key) return Response(LibraryXBlockOlxSerializer({"olx": xml_str}).data) diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index 33ea8514b632..e96a45084212 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -25,6 +25,15 @@ log = logging.getLogger(__name__) + +from openedx_learning.core.components.api import get_component_version_content +from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent +from openedx_learning.core.contents.models import RawContent, TextContent +from openedx_learning.core.publishing.models import Draft, LearningPackage +from django.db.models import Q + + + class BlockstoreXBlockRuntime(XBlockRuntime): """ A runtime designed to work with Blockstore, reading and writing @@ -33,13 +42,8 @@ class BlockstoreXBlockRuntime(XBlockRuntime): def parse_xml_file(self, fileobj, id_generator=None): raise NotImplementedError("Use parse_olx_file() instead") - def get_block(self, usage_id, for_parent=None): - """ - Create an XBlock instance in this runtime. - - Args: - usage_key(OpaqueKey): identifier used to find the XBlock class and data. - """ + def get_block_blockstore(self, usage_key, for_parent=None): + usage_id = usage_key def_id = self.id_reader.get_definition_id(usage_id) if def_id is None: raise ValueError(f"Definition not found for usage {usage_id}") @@ -81,6 +85,68 @@ def get_block(self, usage_id, for_parent=None): block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access return block + def get_block_learning_core(self, usage_key, for_parent=None): + # We should use the lookup table here instead of taking the lib_key directly. + learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) + + # We can do this more efficiently in a single query later, but for now + # just get it the easy way. + component = Component.objects.get( + learning_package=learning_package, + namespace='xblock.v1', + type=usage_key.block_type, + local_key=usage_key.block_id, + ) + component_version = component.versioning.draft + raw_content = component_version.raw_contents.get( + componentversionrawcontent__key="definition.xml" + ) + + xml_str = raw_content.text_content.text + xml_node = etree.fromstring(xml_str) + block_type = usage_id.block_type + + # At this point, how much value do we really get out of explicitly + # differentiating definitions from usages? + def_id = self.id_reader.get_definition_id(usage_id) + keys = ScopeIds(self.user_id, block_type, def_id, usage_id) + + if xml_node.get("url_name", None): + log.warning("XBlock at %s should not specify an old-style url_name attribute.", def_id.olx_path) + block_class = self.mixologist.mix(self.load_block_type(block_type)) + if hasattr(block_class, 'parse_xml_new_runtime'): + # This is a (former) XModule with messy XML parsing code; let its parse_xml() method continue to work + # as it currently does in the old runtime, but let this parse_xml_new_runtime() method parse the XML in + # a simpler way that's free of tech debt, if defined. + # In particular, XmlMixin doesn't play well with this new runtime, so this is mostly about + # bypassing that mixin's code. + # When a former XModule no longer needs to support the old runtime, its parse_xml_new_runtime method + # should be removed and its parse_xml() method should be simplified to just call the super().parse_xml() + # 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) + # 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 + self.system.authored_data_store.cache_fields(block) + # There is no way to set the parent via parse_xml, so do what + # HierarchyMixin would do: + if for_parent is not None: + block._parent_block = for_parent # pylint: disable=protected-access + block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access + return block + + + + def get_block(self, usage_id, for_parent=None): + """ + Create an XBlock instance in this runtime. + + Args: + usage_key(OpaqueKey): identifier used to find the XBlock class and data. + """ + return self.get_block_blockstore(usage_id, for_parent=for_parent) + def add_node_as_child(self, block, node, id_generator=None): """ This runtime API should normally be used via diff --git a/openedx/core/lib/blockstore_api/methods.py b/openedx/core/lib/blockstore_api/methods.py index 7d7c65decdc1..561d1dc5ecea 100644 --- a/openedx/core/lib/blockstore_api/methods.py +++ b/openedx/core/lib/blockstore_api/methods.py @@ -363,6 +363,7 @@ def get_bundle_files_dict(bundle_uuid, use_draft=None): BundleFileData or DraftFileData tuples. """ bundle = get_bundle(bundle_uuid) + if use_draft and use_draft in bundle.drafts: # pylint: disable=unsupported-membership-test draft_uuid = bundle.drafts[use_draft] # pylint: disable=unsubscriptable-object return get_draft(draft_uuid).files From 0967df902b89d997e7f96c6af35ec9707e0909b2 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 24 Oct 2023 11:54:36 -0400 Subject: [PATCH 2/4] fixup!: add migrations, clean up import code, add comments --- cms/envs/common.py | 5 +- lms/envs/common.py | 5 +- .../commands/migrate_lib_to_learning_core.py | 92 +++++++++++++------ .../0010_contentlibrarylearningpackage.py | 22 +++++ .../djangoapps/content_libraries/models.py | 16 +++- 5 files changed, 101 insertions(+), 39 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py diff --git a/cms/envs/common.py b/cms/envs/common.py index ea6204d11ed8..e7a47b3ce6b0 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1814,13 +1814,10 @@ 'openedx_events', - # Learning Core Apps + # Learning Core Apps, used by v2 content libraries (content_libraries app) "openedx_learning.core.components.apps.ComponentsConfig", "openedx_learning.core.contents.apps.ContentsConfig", "openedx_learning.core.publishing.apps.PublishingConfig", - - # Learning Contrib Apps - # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] OPENEDX_LEARNING = { diff --git a/lms/envs/common.py b/lms/envs/common.py index 39b65cbaec47..d64de1992531 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3319,13 +3319,10 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'openedx_events', - # Learning Core Apps + # Learning Core Apps, used by v2 content libraries (content_libraries app) "openedx_learning.core.components.apps.ComponentsConfig", "openedx_learning.core.contents.apps.ContentsConfig", "openedx_learning.core.publishing.apps.PublishingConfig", - - # Learning Contrib Apps - # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] OPENEDX_LEARNING = { diff --git a/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py index 6f9a3f8516b4..925d4f473bd0 100644 --- a/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py +++ b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py @@ -1,13 +1,15 @@ """ Command to import Blockstore-backed v2 Libraries to Learning Core data models. + +This will hopefully be very short-lived code. """ from datetime import datetime, timezone from xml.etree import ElementTree as ET import logging -from django.conf import settings from django.db import transaction from django.core.management import BaseCommand, CommandError +from django.core.exceptions import ObjectDoesNotExist from opaque_keys.edx.locator import LibraryLocatorV2 from openedx.core.djangoapps.content_libraries import api as lib_api @@ -15,7 +17,6 @@ from openedx.core.djangoapps.content_libraries import constants as lib_constants from openedx.core.lib.blockstore_api import ( get_bundle, - get_bundle_file_metadata, get_bundle_file_data, get_bundle_files_dict, ) @@ -23,17 +24,21 @@ from openedx_learning.core.components import api as components_api from openedx_learning.core.contents import api as contents_api -# These imports should all be removed at some point. - -from openedx_learning.core.publishing.models import Draft - log = logging.getLogger(__name__) class Command(BaseCommand): """ - Create a LearningPackage and initialize with contents from Library + Create a new LearningPackage and initialize with contents from Library. + + If you run this and specify a Library that already has a LearningPackage + (using -f), this command will delete that LearningPackage and create a new + one to associate with the Libary. It does not modify the existing one. + + All the work is done in a transaction, so errors partway through the + process shouldn't cause state inconsistency in the database. A partly- + imported course *can* cause data to end up in Django Storages. """ def add_arguments(self, parser): @@ -45,20 +50,43 @@ def add_arguments(self, parser): type=LibraryLocatorV2.from_string, help=('Content Library Key to import content from.'), ) + parser.add_argument( + '-f', + '--force', + action='store_true', + default=False, + ) def handle(self, *args, **options): + """ + Does the work of parsing content from Blockstore and writing it into + openedx-learning core models (publishing, components, contents). + """ # Search for the library. - lib_key = options['library-key'] - lib_data = lib_api.get_library(lib_key) - lib = lib_models.ContentLibrary.objects.get_by_key(lib_key) + try: + lib_key = options['library-key'] + lib_data = lib_api.get_library(lib_key) + lib = lib_models.ContentLibrary.objects.get_by_key(lib_key) + except ObjectDoesNotExist: + raise CommandError(f"Library not found: {lib_key}") COMPONENT_NAMESPACE = 'xblock.v1' + learning_package_already_exists = ( + hasattr(lib, 'contents') and + lib.contents.learning_package is not None + ) + + if learning_package_already_exists and not options['force']: + raise CommandError( + f"Learning Package already exists for {lib_key} (use -f to overwrite)" + ) + with transaction.atomic(): # This is a migration script and we're assuming there's no important # state attached to the LearningPackage yet. That makes it safe to # just wipe out everything and recreate it. - if hasattr(lib, 'contents'): + if learning_package_already_exists: lp = lib.contents.learning_package log.info(f"Deleting existing LearningPackage {lp.key} ({lp.uuid})") lib.contents.delete() @@ -74,22 +102,23 @@ def handle(self, *args, **options): content_library=lib, learning_package=learning_package, ) - - # We only really need to get the most recent version in Studio draft - # for now. + + # We don't need the full history stored in Blockstore, just the most + # recently published version and the most recent draft. bundle = get_bundle(lib.bundle_uuid) published_files = get_bundle_files_dict(lib.bundle_uuid) now = datetime.now(timezone.utc) - - # First get the published version into openedx-learning models. This - # creates ComponentVersions and puts them as the current Draft. + + # First get the published version into openedx-learning models. On + # the openedx-learning side, we'll create them as Drafts and then + # publish at the end. published_metadata_dict = {} published_component_pks = {} published_definition_files = { file_path: metadata for file_path, metadata in published_files.items() - if file_path.endswith('/definition.xml') + if file_path.endswith('/definition.xml') # This is the OLX } for file_path, metadata in published_definition_files.items(): block_type, block_id, _def_xml = file_path.split('/') @@ -126,7 +155,7 @@ def handle(self, *args, **options): published_at=now, ) - # Now grab the draft versions from blockstore, and copy those... + # Now grab the draft version from blockstore, and copy those... draft_files = get_bundle_files_dict(lib.bundle_uuid, use_draft=lib_constants.DRAFT_NAME) draft_definition_files = { file_path: metadata @@ -136,11 +165,12 @@ def handle(self, *args, **options): for file_path, draft_metadata in draft_definition_files.items(): published_metadata = published_metadata_dict.get(file_path) if draft_metadata.modified: - block_type, block_id, def_xml = file_path.split('/') + block_type, block_id, _def_xml = file_path.split('/') xml_bytes = get_bundle_file_data(bundle.uuid, file_path, use_draft=lib_constants.DRAFT_NAME) display_name = extract_display_name(xml_bytes, file_path) - # If this is newly created in the draft... + # If this is newly created in the draft, we have to create a + # whole new Component... if published_metadata is None: component = components_api.create_component( learning_package_id=learning_package.id, @@ -151,8 +181,8 @@ def handle(self, *args, **options): created_by=None, ) component_pk = component.pk - version_num = 1 - # Otherwise, it's been modified... + version_num = 1 + # Otherwise, it's just been modified... else: component_pk = published_component_pks[file_path] version_num = 2 @@ -176,21 +206,23 @@ def handle(self, *args, **options): key="definition.xml", learner_downloadable=False ) - + # Now remove stuff that was present in the published set but was # deleted in the draft. deleted_definition_files = set(published_definition_files) - set(draft_definition_files) for deleted_definition_file in deleted_definition_files: log.info(f"Deleting {deleted_definition_file} from draft") component_pk = published_component_pks[deleted_definition_file] - draft = Draft.objects.get(entity_id=component_pk) - draft.version = None - draft.save() + publishing_api.set_draft_version(component_pk, None) def extract_display_name(xml_bytes, file_path): - # Do some basic parsing of the content to see if it's even well - # constructed enough to add (or whether we should skip/error on it). + """ + Parse the display_name out of the XML. + + This will return an empty string if no display_name is specified, or if + there is a parsing error. + """ try: xml_str = xml_bytes.decode('utf-8') block_root = ET.fromstring(xml_str) @@ -198,5 +230,5 @@ def extract_display_name(xml_bytes, file_path): except ET.ParseError as err: log.error(f"Parse error for {file_path}: {err}") display_name = "" - + return display_name diff --git a/openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py b/openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py new file mode 100644 index 000000000000..0549e09b2497 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py @@ -0,0 +1,22 @@ +# Generated by Django 3.2.22 on 2023-10-24 15:44 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_publishing', '0002_alter_fk_on_delete'), + ('content_libraries', '0009_alter_contentlibrary_authorized_lti_configs'), + ] + + operations = [ + migrations.CreateModel( + name='ContentLibraryLearningPackage', + fields=[ + ('content_library', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, related_name='contents', serialize=False, to='content_libraries.contentlibrary')), + ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.learningpackage')), + ], + ), + ] diff --git a/openedx/core/djangoapps/content_libraries/models.py b/openedx/core/djangoapps/content_libraries/models.py index 7970c9097d7f..e64aa16c19ae 100644 --- a/openedx/core/djangoapps/content_libraries/models.py +++ b/openedx/core/djangoapps/content_libraries/models.py @@ -531,6 +531,20 @@ def __str__(self): class ContentLibraryLearningPackage(models.Model): + """ + Associates ContentLibrary with a LearningPackage. + + This essentially maps the abstract, catalog concept of a Content Library + from the actual bucket of content that is stored in that library. + + If the referenced ContentLibrary is deleted, this association is also + deleted, but the LearningPackage being referenced is _not_ deleted. + + This association actively prevents the referenced LearningPackage from being + deleted, i.e. you cannot just remove the content that is backing a + ContentLibrary by mistake. You have to explicitly disassociate it from the + ContentLibrary first. + """ content_library = models.OneToOneField( ContentLibrary, primary_key=True, @@ -539,5 +553,5 @@ class ContentLibraryLearningPackage(models.Model): ) learning_package = models.ForeignKey( LearningPackage, - on_delete=models.RESTRICT + on_delete=models.RESTRICT, ) From 31f008a5bbe9ed4c22c17c7f6af6724a523eadaa Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 24 Oct 2023 15:24:20 -0400 Subject: [PATCH 3/4] fixup!: moving XBlock runtime related commits to a different PR --- .../core/djangoapps/content_libraries/api.py | 38 +-------- .../xblock/runtime/blockstore_runtime.py | 80 ++----------------- openedx/core/lib/blockstore_api/methods.py | 1 - 3 files changed, 8 insertions(+), 111 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index fb2c2af3d074..737673b32e3f 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -733,13 +733,7 @@ def get_library_block(usage_key): ) -from openedx_learning.core.components.api import get_component_version_content -from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent -from openedx_learning.core.contents.models import RawContent, TextContent -from openedx_learning.core.publishing.models import Draft, LearningPackage -from django.db.models import Q - -def get_library_block_olx(usage_key: LibraryUsageLocatorV2): +def get_library_block_olx(usage_key): """ Get the OLX source of the given XBlock. """ @@ -753,36 +747,6 @@ def get_library_block_olx(usage_key: LibraryUsageLocatorV2): return xml_str - #xml_str = get_component_version_content( - # str(usage_key.lib_key), - # # xblock.v1:problem@3e8c88ea8ec545b4b3c7066c89527d5e_cvo3nwt344t7ytqts - # f"xblock.v1:{usage_key.definition_key}", - # 1, - # "definition.xml", - #) -# cv_rc = ComponentVersionRawContent.objects.select_related( -# "raw_content", -# "raw_content__text_content" -# "component_version", -# "component_version__component", -# "component_version__component__learning_package", -# ) - # Inefficient but simple approach first - learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) - component = Component.objects.get( - learning_package=learning_package, - namespace='xblock.v1', - type=usage_key.block_type, - local_key=usage_key.block_id, - ) - component_version = component.versioning.draft - text_content = component_version.raw_contents.get(key="definition.xml").text_content - - print("I really executed! Not a hallucination!") - - return text_content.text - - def set_library_block_olx(usage_key, new_olx_str): """ Replace the OLX source of the given XBlock. diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index e96a45084212..33ea8514b632 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -25,15 +25,6 @@ log = logging.getLogger(__name__) - -from openedx_learning.core.components.api import get_component_version_content -from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent -from openedx_learning.core.contents.models import RawContent, TextContent -from openedx_learning.core.publishing.models import Draft, LearningPackage -from django.db.models import Q - - - class BlockstoreXBlockRuntime(XBlockRuntime): """ A runtime designed to work with Blockstore, reading and writing @@ -42,8 +33,13 @@ class BlockstoreXBlockRuntime(XBlockRuntime): def parse_xml_file(self, fileobj, id_generator=None): raise NotImplementedError("Use parse_olx_file() instead") - def get_block_blockstore(self, usage_key, for_parent=None): - usage_id = usage_key + def get_block(self, usage_id, for_parent=None): + """ + Create an XBlock instance in this runtime. + + Args: + usage_key(OpaqueKey): identifier used to find the XBlock class and data. + """ def_id = self.id_reader.get_definition_id(usage_id) if def_id is None: raise ValueError(f"Definition not found for usage {usage_id}") @@ -85,68 +81,6 @@ def get_block_blockstore(self, usage_key, for_parent=None): block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access return block - def get_block_learning_core(self, usage_key, for_parent=None): - # We should use the lookup table here instead of taking the lib_key directly. - learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) - - # We can do this more efficiently in a single query later, but for now - # just get it the easy way. - component = Component.objects.get( - learning_package=learning_package, - namespace='xblock.v1', - type=usage_key.block_type, - local_key=usage_key.block_id, - ) - component_version = component.versioning.draft - raw_content = component_version.raw_contents.get( - componentversionrawcontent__key="definition.xml" - ) - - xml_str = raw_content.text_content.text - xml_node = etree.fromstring(xml_str) - block_type = usage_id.block_type - - # At this point, how much value do we really get out of explicitly - # differentiating definitions from usages? - def_id = self.id_reader.get_definition_id(usage_id) - keys = ScopeIds(self.user_id, block_type, def_id, usage_id) - - if xml_node.get("url_name", None): - log.warning("XBlock at %s should not specify an old-style url_name attribute.", def_id.olx_path) - block_class = self.mixologist.mix(self.load_block_type(block_type)) - if hasattr(block_class, 'parse_xml_new_runtime'): - # This is a (former) XModule with messy XML parsing code; let its parse_xml() method continue to work - # as it currently does in the old runtime, but let this parse_xml_new_runtime() method parse the XML in - # a simpler way that's free of tech debt, if defined. - # In particular, XmlMixin doesn't play well with this new runtime, so this is mostly about - # bypassing that mixin's code. - # When a former XModule no longer needs to support the old runtime, its parse_xml_new_runtime method - # should be removed and its parse_xml() method should be simplified to just call the super().parse_xml() - # 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) - # 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 - self.system.authored_data_store.cache_fields(block) - # There is no way to set the parent via parse_xml, so do what - # HierarchyMixin would do: - if for_parent is not None: - block._parent_block = for_parent # pylint: disable=protected-access - block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access - return block - - - - def get_block(self, usage_id, for_parent=None): - """ - Create an XBlock instance in this runtime. - - Args: - usage_key(OpaqueKey): identifier used to find the XBlock class and data. - """ - return self.get_block_blockstore(usage_id, for_parent=for_parent) - def add_node_as_child(self, block, node, id_generator=None): """ This runtime API should normally be used via diff --git a/openedx/core/lib/blockstore_api/methods.py b/openedx/core/lib/blockstore_api/methods.py index 561d1dc5ecea..7d7c65decdc1 100644 --- a/openedx/core/lib/blockstore_api/methods.py +++ b/openedx/core/lib/blockstore_api/methods.py @@ -363,7 +363,6 @@ def get_bundle_files_dict(bundle_uuid, use_draft=None): BundleFileData or DraftFileData tuples. """ bundle = get_bundle(bundle_uuid) - if use_draft and use_draft in bundle.drafts: # pylint: disable=unsupported-membership-test draft_uuid = bundle.drafts[use_draft] # pylint: disable=unsubscriptable-object return get_draft(draft_uuid).files From 828c8220ec8a18d7111c875a07fb0bb546fe6adc Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 24 Oct 2023 15:25:10 -0400 Subject: [PATCH 4/4] fixup!: one more view I needed to undo changes on. --- openedx/core/djangoapps/content_libraries/views.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 76221a878de5..c83a9c4fcb56 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -680,9 +680,6 @@ def get(self, request, usage_key_str): """ key = LibraryUsageLocatorV2.from_string(usage_key_str) api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) - - print("You called me!") - xml_str = api.get_library_block_olx(key) return Response(LibraryXBlockOlxSerializer({"olx": xml_str}).data)