diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index c65c8af0c59c..46df05b442b1 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -881,6 +881,7 @@ def _migrate_container( user_id=context.created_by, ) if container_exists and context.should_skip_strategy: + assert container.draft_version_num is not None # We know it exists, this is just for mypy return PublishableEntityVersion.objects.get( entity_id=container.container_id, version_num=container.draft_version_num, diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 4623b814d0a0..5a6685d7365d 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -12,7 +12,7 @@ from opaque_keys.edx.keys import ContainerKey, LearningContextKey, OpaqueKey, UsageKey from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator from openedx_content import api as content_api -from openedx_content.models_api import Collection, Section, Subsection, Unit +from openedx_content.models_api import Collection from rest_framework.exceptions import NotFound from openedx.core.djangoapps.content.search.models import SearchAccess @@ -627,42 +627,21 @@ def searchable_doc_for_container( log.error(f"Container {container_key} not found") return doc - draft_children = lib_api.get_container_children( - container_key, - published=False, - ) + draft_children = lib_api.get_container_children_list(container_key, published=False) publish_status = PublishStatus.published if container.last_published is None: publish_status = PublishStatus.never elif container.has_unpublished_changes: publish_status = PublishStatus.modified - container_type_code = container_key.container_type - - def get_child_keys(children) -> list[str]: - match container_type_code: - case Unit.type_code: - return [ - str(child.usage_key) - for child in children - ] - case Subsection.type_code | Section.type_code: - return [ - str(child.container_key) - for child in children - ] - - def get_child_names(children) -> list[str]: - return [child.display_name for child in children] - doc.update({ Fields.display_name: container.display_name, Fields.created: container.created.timestamp(), Fields.modified: container.modified.timestamp(), Fields.num_children: len(draft_children), Fields.content: { - Fields.child_usage_keys: get_child_keys(draft_children), - Fields.child_display_names: get_child_names(draft_children), + Fields.child_usage_keys: [str(child.key) for child in draft_children], + Fields.child_display_names: [child.display_name for child in draft_children], }, Fields.publish_status: publish_status, Fields.last_published: container.last_published.timestamp() if container.last_published else None, @@ -672,16 +651,13 @@ def get_child_names(children) -> list[str]: doc[Fields.breadcrumbs] = [{"display_name": library.title}] if container.published_version_num is not None: - published_children = lib_api.get_container_children( - container_key, - published=True, - ) + published_children = lib_api.get_container_children_list(container_key, published=True) doc[Fields.published] = { Fields.published_display_name: container.published_display_name, Fields.published_num_children: len(published_children), Fields.published_content: { - Fields.child_usage_keys: get_child_keys(published_children), - Fields.child_display_names: get_child_names(published_children), + Fields.child_usage_keys: [str(child.key) for child in published_children], + Fields.child_display_names: [child.display_name for child in published_children], }, } diff --git a/openedx/core/djangoapps/content_libraries/api/block_metadata.py b/openedx/core/djangoapps/content_libraries/api/block_metadata.py index cb76aee22ed1..1d8fe64b5330 100644 --- a/openedx/core/djangoapps/content_libraries/api/block_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/block_metadata.py @@ -46,9 +46,20 @@ class LibraryXBlockMetadata(PublishableItem): usage_key: LibraryUsageLocatorV2 @classmethod - def from_component(cls, library_key, component, associated_collections=None): + def from_component( + cls, + library_key: LibraryLocatorV2, + component, + associated_collections=None, + use_published=False, + ) -> LibraryXBlockMetadata: """ Construct a LibraryXBlockMetadata from a Component object. + + Requires that the draft version of the component exists, unless you + specify use_published=True, in which case it requires that the published + version exists. The 'display_name' and 'modified' fields will depend on + which version you request. """ # Import content_tagging.api here to avoid circular imports from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts @@ -61,17 +72,17 @@ def from_component(cls, library_key, component, associated_collections=None): draft = component.versioning.draft published = component.versioning.published last_draft_created = draft.created if draft else None - last_draft_created_by = draft.publishable_entity_version.created_by if draft else None + last_draft_created_by = draft.publishable_entity_version.created_by if draft else "" usage_key = library_component_usage_key(library_key, component) tags = get_object_tag_counts(str(usage_key), count_implicit=True) return cls( usage_key=usage_key, - display_name=draft.title, + display_name=published.title if use_published else draft.title, created=component.created, created_by=component.created_by.username if component.created_by else None, - modified=draft.created, - draft_version_num=draft.version_num, + modified=published.created if use_published else draft.created, + draft_version_num=draft.version_num if draft else None, published_version_num=published.version_num if published else None, published_display_name=published.title if published else None, last_published=None if last_publish_log is None else last_publish_log.published_at, diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index e23322d3b29b..a111b91a2cab 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -417,6 +417,7 @@ def set_library_block_olx( usage_key: LibraryUsageLocatorV2, new_olx_str: str, paths_to_media: dict | None = None, + created_by: int | None = None, ) -> ComponentVersion: """ Replace the OLX source of the given XBlock. @@ -488,6 +489,7 @@ def set_library_block_olx( 'block.xml': new_olx_media.pk, }, created=now, + created_by=created_by, ) return new_component_version diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index 98a5024ac674..d7ad7e7f6ad4 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -74,9 +74,14 @@ class ContainerMetadata(PublishableItem): container_id: Container.ID @classmethod - def from_container(cls, library_key, container: Container, associated_collections=None): + def from_container(cls, library_key, container: Container, associated_collections=None, use_published=False): """ Construct a ContainerMetadata object from a Container object. + + Requires that the draft version of the container exists, unless you + specify use_published=True, in which case it requires that the published + version exists. The 'display_name' and 'modified' fields will depend on + which version you request. """ last_publish_log = container.versioning.last_publish_log container_key = library_container_locator( @@ -100,10 +105,10 @@ def from_container(cls, library_key, container: Container, associated_collection container_key=container_key, container_type_code=container_key.container_type, container_id=container.id, - display_name=draft.title, + display_name=published.title if use_published else draft.title, created=container.created, - modified=draft.created, - draft_version_num=draft.version_num, + modified=published.created if use_published else draft.created, + draft_version_num=draft.version_num if draft else None, published_version_num=published.version_num if published else None, published_display_name=published.title if published else None, last_published=None if last_publish_log is None else last_publish_log.published_at, diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 9bb54c5daba3..8952576cb163 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -7,6 +7,7 @@ import logging import operator import typing +from dataclasses import dataclass from datetime import datetime, timezone from functools import cache from uuid import UUID, uuid4 @@ -28,6 +29,7 @@ LibraryXBlockMetadata, direct_published_entity_from_record, get_entity_item_type, + library_component_usage_key, make_contributor, resolve_change_action, ) @@ -48,9 +50,11 @@ # 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured # out our approach to dynamic content (randomized, A/B tests, etc.) __all__ = [ + "ContainerChildMetadata", "get_container", "create_container", "get_container_children", + "get_container_children_list", "get_container_children_count", "update_container", "delete_container", @@ -70,6 +74,15 @@ log = logging.getLogger(__name__) +@dataclass(frozen=True) +class ContainerChildMetadata: + """ + Class that represents limited metadata about a child entity of a container + """ + display_name: str + key: LibraryUsageLocatorV2 | LibraryContainerLocator + + def get_container( container_key: LibraryContainerLocator, *, @@ -202,8 +215,8 @@ def get_container_children( published=False, ) -> list[LibraryXBlockMetadata | ContainerMetadata]: """ - [ 🛑 UNSTABLE ] Get the entities contained in the given container - (e.g. the components/xblocks in a unit, units in a subsection, subsections in a section) + [ 🛑 UNSTABLE ] Get the entities contained in the given container (e.g. the + components in a unit, units in a subsection, subsections in a section). """ container = get_container_from_key(container_key) @@ -211,10 +224,40 @@ def get_container_children( result: list[LibraryXBlockMetadata | ContainerMetadata] = [] for entry in child_entities: if hasattr(entry.entity, "component"): # the child is a Component - result.append(LibraryXBlockMetadata.from_component(container_key.lib_key, entry.entity.component)) + result.append(LibraryXBlockMetadata.from_component( + container_key.lib_key, entry.entity.component, use_published=published, + )) + else: + assert isinstance(entry.entity.container, Container) + result.append(ContainerMetadata.from_container( + container_key.lib_key, entry.entity.container, use_published=published, + )) + return result + + +def get_container_children_list( + container_key: LibraryContainerLocator, *, + published: bool, +) -> list[ContainerChildMetadata]: + """ + [ 🛑 UNSTABLE ] Get the entities contained in the given container (e.g. the + components/xblocks in a unit, units in a subsection, subsections in a section) + + Returns a list of ``ContainerChildMetadata`` objects (which give only each + child's display name and opaque key, though the opaque key also includes + information on what "type" of component/container it is). + """ + container = get_container_from_key(container_key) + result: list[ContainerChildMetadata] = [] + for entry in content_api.get_entities_in_container(container, published=published): + key: LibraryUsageLocatorV2 | LibraryContainerLocator + if hasattr(entry.entity, "component"): # the child is a Component + key = library_component_usage_key(container_key.lib_key, entry.entity.component) else: assert isinstance(entry.entity.container, Container) - result.append(ContainerMetadata.from_container(container_key.lib_key, entry.entity.container)) + key = library_container_locator(container_key.lib_key, entry.entity.container) + display_name = entry.entity_version.title + result.append(ContainerChildMetadata(display_name=display_name, key=key)) return result @@ -254,7 +297,7 @@ def update_container_children( def get_containers_contains_item(key: LibraryUsageLocatorV2 | LibraryContainerLocator) -> list[ContainerMetadata]: """ - [ 🛑 UNSTABLE ] Get containers that contains the item, that can be a component or another container. + [ 🛑 UNSTABLE ] Get list of draft containers that contain the item. Item can be a component or another container. """ entity = get_entity_from_key(key) containers = content_api.get_containers_with_entity(entity.id).select_related("container_type") diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index d00c32a9e1d1..305d396d4619 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -200,7 +200,7 @@ class PublishableItem(LibraryItem): Common fields for anything that can be found in a content library that has draft/publish support. """ - draft_version_num: int + draft_version_num: int | None published_version_num: int | None = None published_display_name: str | None last_published: datetime | None = None diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index ba39662077d2..73c0f574ca50 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -433,7 +433,7 @@ def post(self, request, usage_key_str): serializer.is_valid(raise_exception=True) new_olx_str = serializer.validated_data["olx"] try: - version_num = api.set_library_block_olx(key, new_olx_str).version_num + version_num = api.set_library_block_olx(key, new_olx_str, created_by=request.user.id).version_num except ValueError as err: raise ValidationError(detail=str(err)) # pylint: disable=raise-missing-from # noqa: B904 return Response(self.serializer_class({"olx": new_olx_str, "version_num": version_num}).data) diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 4137ee36d045..8e5a9bb264ff 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -489,11 +489,11 @@ def _restore_container(self, container_key: ContainerKey | str, expect_response= """ Restore a deleted a container (unit etc.) """ return self._api('post', URL_LIB_CONTAINER_RESTORE.format(container_key=container_key), None, expect_response) - def _get_container_children(self, container_key: ContainerKey | str, expect_response=200): + def _get_container_children(self, container_key: ContainerKey | str, published=False, expect_response=200): """ Get container children""" return self._api( 'get', - URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key), + URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key) + ("?published=true" if published else ""), None, expect_response ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 55be983d18dd..3900234f1db5 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -1,12 +1,13 @@ """ Tests for openedx_content-based Content Libraries """ +import copy import textwrap from datetime import UTC, datetime import ddt from freezegun import freeze_time -from opaque_keys.edx.locator import LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api @@ -103,10 +104,19 @@ def setUp(self) -> None: ) # Create blocks - self.problem_block = self._add_block_to_library(self.lib["id"], "problem", "Problem1", can_stand_alone=False) - self.html_block = self._add_block_to_library(self.lib["id"], "html", "Html1", can_stand_alone=False) - self.problem_block_2 = self._add_block_to_library(self.lib["id"], "problem", "Problem2", can_stand_alone=False) - self.html_block_2 = self._add_block_to_library(self.lib["id"], "html", "Html2") + with freeze_time(self.create_date): + self.problem_block = self._add_block_to_library( + self.lib["id"], "problem", "Problem1", can_stand_alone=False + ) + self.html_block = self._add_block_to_library( + self.lib["id"], "html", "Html1", can_stand_alone=False + ) + self.problem_block_2 = self._add_block_to_library( + self.lib["id"], "problem", "Problem2", can_stand_alone=False + ) + self.html_block_2 = self._add_block_to_library( + self.lib["id"], "html", "Html2" + ) with freeze_time(self.modified_date): # Add components to `unit_with_components` @@ -528,6 +538,194 @@ def test_section_replace_children(self) -> None: assert data[0]['id'] == new_subsection_1['id'] assert data[1]['id'] == new_subsection_2['id'] + def test_published_child_components(self) -> None: + """ + Test that we can get the published version of a unit. + """ + unit_key_str = self.unit_with_components["id"] + unit_key = LibraryContainerLocator.from_string(unit_key_str) + # Publish "unit with components" + self._publish_container(unit_key_str) + # Now both its draft and published versions contain 4 identical components: + assert api.get_container_children_count(unit_key, published=False) == 4 + assert api.get_container_children_count(unit_key, published=True) == 4 + + original_children = self._get_container_children(unit_key_str) + draft_edited_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=UTC) + # Modify the first component: + first_child_key_str = original_children[0]["id"] + with freeze_time(draft_edited_date): + self._set_library_block_olx(first_child_key_str, "") + # And delete the last one: + last_child_key_str = original_children[3]["id"] + with freeze_time(draft_edited_date): + self._delete_library_block(last_child_key_str) + + # Now the counts should be different - the draft has only three children: + assert api.get_container_children_count(unit_key, published=False) == 3 + assert api.get_container_children_count(unit_key, published=True) == 4 + + # Check the published version + expected_published = copy.deepcopy(original_children) + # The first child was modified since publish: + expected_published[0]["has_unpublished_changes"] = True + # The last child was modified (deleted) since publish: + expected_published[3]["has_unpublished_changes"] = True + expected_published[3]["last_draft_created"] = None + expected_published[3]["last_draft_created_by"] = "" + + assert self._get_container_children(unit_key_str, published=True) == expected_published + + # Check the draft version: + expected_draft = copy.deepcopy(original_children) + # The first child was modified since publish: + expected_draft[0]["display_name"] = "DRAFT problem" + expected_draft[0]["has_unpublished_changes"] = True + # The last child was modified (deleted) since publish: + del expected_draft[3] + + assert self._get_container_children(unit_key_str, published=False) == expected_draft + + def test_published_child_containers(self) -> None: + """ + Test that we can get the published version of a subsection. + """ + subsection_key_str = self.subsection_with_units["id"] + subsection_key = LibraryContainerLocator.from_string(subsection_key_str) + # Publish "subsection with units" + self._publish_container(subsection_key_str) + # Now both its draft and published versions contain 4 identical units: + assert api.get_container_children_count(subsection_key, published=False) == 4 + assert api.get_container_children_count(subsection_key, published=True) == 4 + + original_children = self._get_container_children(subsection_key_str) + draft_edited_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=UTC) + # Modify the first child unit: + first_child_key_str = original_children[0]["id"] + with freeze_time(draft_edited_date): + self._update_container(first_child_key_str, display_name="DRAFT unit") + # And delete the last one: + last_child_key_str = original_children[3]["id"] + with freeze_time(draft_edited_date): + self._delete_container(last_child_key_str) + + # Now the counts should be different - the draft has only three children: + assert api.get_container_children_count(subsection_key, published=False) == 3 + assert api.get_container_children_count(subsection_key, published=True) == 4 + + # Check the published version + expected_published = copy.deepcopy(original_children) + # The first child was modified since publish: + expected_published[0]["has_unpublished_changes"] = True + # The last child was modified (deleted) since publish: + expected_published[3]["has_unpublished_changes"] = True + expected_published[3]["last_draft_created"] = None + expected_published[3]["last_draft_created_by"] = "" # last draft was deleted. + + assert self._get_container_children(subsection_key_str, published=True)[0] == expected_published[0] + assert self._get_container_children(subsection_key_str, published=True)[3] == expected_published[3] + assert self._get_container_children(subsection_key_str, published=True) == expected_published + + # Check the draft version: + expected_draft = copy.deepcopy(original_children) + # The first child was modified since publish: + expected_draft[0]["display_name"] = "DRAFT unit" + expected_draft[0]["has_unpublished_changes"] = True + # The last child was modified (deleted) since publish: + del expected_draft[3] + + assert self._get_container_children(subsection_key_str, published=False) == expected_draft + + def test_get_container_children_queries(self): + """ + Test how many queries are used to retrieve the children of a container + """ + empty_unit_key = LibraryContainerLocator.from_string(self.unit["id"]) + unit_with_children_key = LibraryContainerLocator.from_string(self.unit_with_components["id"]) + EMPTY_QUERIES = 6 + PER_CHILD_QUERIES = 10 # There's room to optimize here. + with self.assertNumQueries(EMPTY_QUERIES): + result = api.get_container_children(empty_unit_key) + assert len(result) == 0 + with self.assertNumQueries(6): + num_children = api.get_container_children_count(unit_with_children_key) + with self.assertNumQueries(EMPTY_QUERIES + PER_CHILD_QUERIES * num_children): + result = api.get_container_children(unit_with_children_key) + assert len(result) == num_children + + def test_get_container_children_list_components(self) -> None: + """ + Test that we can use get_container_children_list() to get the draft and + published children of any container, simply and performantly. + """ + # Publish "unit with components" + unit_key_str = self.unit_with_components["id"] + unit_key = LibraryContainerLocator.from_string(unit_key_str) + self._publish_container(unit_key_str) + + original_list = api.get_container_children_list(unit_key, published=True) + assert original_list == [ + api.ContainerChildMetadata( + display_name='Blank Problem', + key=LibraryUsageLocatorV2.from_string('lb:CL-TEST:containers:problem:Problem1'), + ), + api.ContainerChildMetadata( + display_name='Text', + key=LibraryUsageLocatorV2.from_string('lb:CL-TEST:containers:html:Html1'), + ), + api.ContainerChildMetadata( + display_name='Blank Problem', + key=LibraryUsageLocatorV2.from_string('lb:CL-TEST:containers:problem:Problem2'), + ), + api.ContainerChildMetadata( + display_name='Text', + key=LibraryUsageLocatorV2.from_string('lb:CL-TEST:containers:html:Html2'), + ), + ] + + # Modify the first component: + first_child_key = original_list[0].key + self._set_library_block_olx(str(first_child_key), "") + # And delete the last one: + last_child_key = original_list[3].key + self._delete_library_block(str(last_child_key)) + + # Now the counts should be different - the draft has only three children: + assert api.get_container_children_list(unit_key, published=True) == original_list + assert api.get_container_children_list(unit_key, published=False) == [ + api.ContainerChildMetadata( + display_name='DRAFT problem', # new name + key=LibraryUsageLocatorV2.from_string('lb:CL-TEST:containers:problem:Problem1'), + ), + api.ContainerChildMetadata( + display_name='Text', + key=LibraryUsageLocatorV2.from_string('lb:CL-TEST:containers:html:Html1'), + ), + api.ContainerChildMetadata( + display_name='Blank Problem', + key=LibraryUsageLocatorV2.from_string('lb:CL-TEST:containers:problem:Problem2'), + ), + # last component was deleted. + ] + + + def test_get_container_children_list_queries(self): + """ + Test how many queries are used to retrieve the children of a container + """ + empty_unit_key = LibraryContainerLocator.from_string(self.unit["id"]) + unit_with_children_key = LibraryContainerLocator.from_string(self.unit_with_components["id"]) + EMPTY_QUERIES = 6 + PER_CHILD_QUERIES = 3 # There's room to optimize here - remove the componenttype lookups + with self.assertNumQueries(EMPTY_QUERIES): + result = api.get_container_children_list(empty_unit_key, published=False) + assert len(result) == 0 + with self.assertNumQueries(6): + num_children = api.get_container_children_count(unit_with_children_key) + with self.assertNumQueries(EMPTY_QUERIES + PER_CHILD_QUERIES * num_children): + result = api.get_container_children_list(unit_with_children_key, published=False) + assert len(result) == num_children + @ddt.data( "unit", "subsection",