From f2c339fefd425b80a1c7b16c459aef3c00119c4d Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 29 May 2025 17:06:22 -0500 Subject: [PATCH 01/14] test: Test for publish section/subsection --- .../tests/test_downstream_sync_integration.py | 4 +- .../content_libraries/api/containers.py | 2 +- .../content_libraries/api/libraries.py | 2 +- .../content_libraries/tests/base.py | 8 +- .../tests/test_containers.py | 269 ++++++++++++++---- 5 files changed, 226 insertions(+), 59 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py index b7890f821dcc..0e8d3dc3c758 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py @@ -363,7 +363,7 @@ def test_unit_sync(self): 'single select...' ) self._add_container_children(self.upstream_unit["id"], [upstream_problem3["id"]]) - self._remove_container_components(self.upstream_unit["id"], [self.upstream_problem2["id"]]) + self._remove_container_children(self.upstream_unit["id"], [self.upstream_problem2["id"]]) self._commit_library_changes(self.library["id"]) # publish everything status = self._get_sync_status(downstream_unit["locator"]) @@ -415,7 +415,7 @@ def test_unit_sync(self): """) # Now, reorder components - self._patch_container_components(self.upstream_unit["id"], [ + self._patch_container_children(self.upstream_unit["id"], [ upstream_problem3["id"], self.upstream_problem1["id"], self.upstream_html1["id"], diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 8e2e486377da..6580975b9696 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -121,7 +121,7 @@ def from_container(cls, library_key, container: Container, associated_collection container=container, ) container_type = ContainerType(container_key.container_type) - published_by = "" + published_by = None if last_publish_log and last_publish_log.published_by: published_by = last_publish_log.published_by.username diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index b6f4d82aeccb..487b01b381a4 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -194,7 +194,7 @@ class PublishableItem(LibraryItem): published_display_name: str | None last_published: datetime | None = None # The username of the user who last published this. - published_by: str = "" + published_by: str | None = "" last_draft_created: datetime | None = None # The username of the user who created the last draft. last_draft_created_by: str = "" diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 0036c208b0c7..02e9cd251e4f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -419,13 +419,13 @@ def _add_container_children( expect_response ) - def _remove_container_components( + def _remove_container_children( self, container_key: ContainerKey | str, children_ids: list[str], expect_response=200, ): - """ Remove container components""" + """ Remove container children""" return self._api( 'delete', URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key), @@ -433,13 +433,13 @@ def _remove_container_components( expect_response ) - def _patch_container_components( + def _patch_container_children( self, container_key: ContainerKey | str, children_ids: list[str], expect_response=200, ): - """ Update container components""" + """ Update container children""" return self._api( 'patch', URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key), diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 26196d8394d8..d9c54289031f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -155,7 +155,7 @@ def test_container_crud(self, container_type, slug, display_name) -> None: "container_type": container_type, "display_name": display_name, "last_published": None, - "published_by": "", + "published_by": None, "last_draft_created": "2024-09-08T07:06:05Z", "last_draft_created_by": 'Bob', 'has_unpublished_changes': True, @@ -371,7 +371,7 @@ def test_container_remove_children(self, container_name, items_to_remove, expect data = self._get_container_children(container["id"]) assert len(data) == 4 # Remove items. - self._remove_container_components( + self._remove_container_children( container["id"], children_ids=[item_to_remove_1["id"], item_to_remove_2["id"]] ) @@ -392,7 +392,7 @@ def test_unit_replace_children(self) -> None: assert data[3]['id'] == self.html_block_2['id'] # Reorder the components - self._patch_container_components( + self._patch_container_children( self.unit_with_components["id"], children_ids=[ self.problem_block["id"], @@ -411,7 +411,7 @@ def test_unit_replace_children(self) -> None: # Replace with new components new_problem_block = self._add_block_to_library(self.lib["id"], "problem", "New_Problem", can_stand_alone=False) new_html_block = self._add_block_to_library(self.lib["id"], "html", "New_Html", can_stand_alone=False) - self._patch_container_components( + self._patch_container_children( self.unit_with_components["id"], children_ids=[new_problem_block["id"], new_html_block["id"]], ) @@ -432,7 +432,7 @@ def test_subsection_replace_children(self) -> None: assert data[3]['id'] == self.unit_3['id'] # Reorder the units - self._patch_container_components( + self._patch_container_children( self.subsection_with_units["id"], children_ids=[ self.unit_2["id"], @@ -451,7 +451,7 @@ def test_subsection_replace_children(self) -> None: # Replace with new units new_unit_1 = self._create_container(self.lib["id"], "unit", display_name="New Unit 1", slug=None) new_unit_2 = self._create_container(self.lib["id"], "unit", display_name="New Unit 2", slug=None) - self._patch_container_components( + self._patch_container_children( self.subsection_with_units["id"], children_ids=[new_unit_1["id"], new_unit_2["id"]], ) @@ -472,7 +472,7 @@ def test_section_replace_children(self) -> None: assert data[3]['id'] == self.subsection_3['id'] # Reorder the subsections - self._patch_container_components( + self._patch_container_children( self.section_with_subsections["id"], children_ids=[ self.subsection_2["id"], @@ -501,7 +501,7 @@ def test_section_replace_children(self) -> None: display_name="New Subsection 2", slug=None, ) - self._patch_container_components( + self._patch_container_children( self.section_with_subsections["id"], children_ids=[new_subsection_1["id"], new_subsection_2["id"]], ) @@ -532,7 +532,7 @@ def test_restore_containers(self, container_type) -> None: "container_type": container_type, "display_name": container["display_name"], "last_published": None, - "published_by": "", + "published_by": None, "last_draft_created": "2024-09-08T07:06:05Z", "last_draft_created_by": 'Bob', 'has_unpublished_changes': True, @@ -543,7 +543,7 @@ def test_restore_containers(self, container_type) -> None: self.assertDictContainsEntries(new_container_data, expected_data) - def test_container_collections(self) -> None: + def test_unit_collections(self) -> None: # Create a collection col1 = api.create_library_collection( self.lib_key, @@ -566,9 +566,30 @@ def test_container_collections(self) -> None: # Verify the collections assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] - def test_publish_container(self) -> None: # pylint: disable=too-many-statements + def _verify_publish_state( + self, + container_id, + expected_childen_len, + expected_has_unpublished_changes, + expected_published_by, + expected_children_ids, + ) -> None: """ - Test that we can publish the changes to a specific container + Verify the publish state of a container + """ + container = self._get_container(container_id) + assert container["has_unpublished_changes"] == expected_has_unpublished_changes + container_children = self._get_container_children(container_id) + assert len(container_children) == expected_childen_len + + for index in range(expected_childen_len): + assert container_children[index]["id"] == expected_children_ids[index] + assert container_children[index]["has_unpublished_changes"] == expected_has_unpublished_changes + assert container_children[index]["published_by"] == expected_published_by + + def test_publish_unit(self) -> None: + """ + Test that we can publish the changes to a specific unit """ html_block_3 = self._add_block_to_library(self.lib["id"], "html", "Html3") self._add_container_children( @@ -580,54 +601,200 @@ def test_publish_container(self) -> None: # pylint: disable=too-many-statements ) # At first everything is unpublished: - c1_before = self._get_container(self.unit_with_components["id"]) - assert c1_before["has_unpublished_changes"] - c1_components_before = self._get_container_children(self.unit_with_components["id"]) - assert len(c1_components_before) == 4 - assert c1_components_before[0]["id"] == self.problem_block["id"] - assert c1_components_before[0]["has_unpublished_changes"] - assert c1_components_before[0]["published_by"] is None - assert c1_components_before[1]["id"] == self.html_block["id"] - assert c1_components_before[1]["has_unpublished_changes"] - assert c1_components_before[1]["published_by"] is None - assert c1_components_before[2]["id"] == self.problem_block_2["id"] - assert c1_components_before[2]["has_unpublished_changes"] - assert c1_components_before[2]["published_by"] is None - assert c1_components_before[3]["id"] == self.html_block_2["id"] - assert c1_components_before[3]["has_unpublished_changes"] - assert c1_components_before[3]["published_by"] is None - c2_before = self._get_container(self.unit["id"]) - assert c2_before["has_unpublished_changes"] - - # Now publish only Container 1 + self._verify_publish_state( + container_id=self.unit_with_components["id"], + expected_childen_len=4, + expected_has_unpublished_changes=True, + expected_published_by=None, + expected_children_ids=[ + self.problem_block["id"], + self.html_block["id"], + self.problem_block_2["id"], + self.html_block_2["id"], + ], + ) + self._verify_publish_state( + container_id=self.unit["id"], + expected_childen_len=2, + expected_has_unpublished_changes=True, + expected_published_by=None, + expected_children_ids=[ + self.html_block["id"], + html_block_3["id"], + ], + ) + + # Now publish only unit 1 self._publish_container(self.unit_with_components["id"]) # Now it is published: - c1_after = self._get_container(self.unit_with_components["id"]) - assert c1_after["has_unpublished_changes"] is False - c1_components_after = self._get_container_children(self.unit_with_components["id"]) - assert len(c1_components_after) == 4 - assert c1_components_after[0]["id"] == self.problem_block["id"] - assert c1_components_after[0]["has_unpublished_changes"] is False - assert c1_components_after[0]["published_by"] == self.user.username - assert c1_components_after[1]["id"] == self.html_block["id"] - assert c1_components_after[1]["has_unpublished_changes"] is False - assert c1_components_after[1]["published_by"] == self.user.username - assert c1_components_after[2]["id"] == self.problem_block_2["id"] - assert c1_components_after[2]["has_unpublished_changes"] is False - assert c1_components_after[2]["published_by"] == self.user.username - assert c1_components_after[3]["id"] == self.html_block_2["id"] - assert c1_components_after[3]["has_unpublished_changes"] is False - assert c1_components_after[3]["published_by"] == self.user.username - - # and container 2 is still unpublished, except for the shared HTML block that is also in container 1: + self._verify_publish_state( + container_id=self.unit_with_components["id"], + expected_childen_len=4, + expected_has_unpublished_changes=False, + expected_published_by=self.user.username, + expected_children_ids=[ + self.problem_block["id"], + self.html_block["id"], + self.problem_block_2["id"], + self.html_block_2["id"], + ], + ) + + # and unit 2 is still unpublished, except for the shared HTML block that is also in unit 1: c2_after = self._get_container(self.unit["id"]) assert c2_after["has_unpublished_changes"] c2_components_after = self._get_container_children(self.unit["id"]) assert len(c2_components_after) == 2 assert c2_components_after[0]["id"] == self.html_block["id"] - assert c2_components_after[0]["has_unpublished_changes"] is False # published since it's also in container 1 + assert c2_components_after[0]["has_unpublished_changes"] is False # published since it's also in unit 1 assert c2_components_after[0]["published_by"] == self.user.username assert c2_components_after[1]["id"] == html_block_3["id"] assert c2_components_after[1]["has_unpublished_changes"] # unaffected assert c2_components_after[1]["published_by"] is None + + def test_publish_subsection(self) -> None: + """ + Test that we can publish the changes to a specific subsection + """ + unit_4 = self._create_container(self.lib["id"], "unit", display_name="Test Unit 4", slug=None) + + # Add units on subsection + self._add_container_children( + self.subsection["id"], + children_ids=[ + self.unit["id"], + unit_4["id"], + ] + ) + # Removing the unit with components because the components (children of children) are not published. + # If the unit is kept, the subsection continues to have changes even after it is published. + self._remove_container_children( + self.subsection_with_units["id"], + children_ids=[ + self.unit_with_components["id"], + ] + ) + + # At first everything is unpublished: + self._verify_publish_state( + container_id=self.subsection_with_units["id"], + expected_childen_len=3, + expected_has_unpublished_changes=True, + expected_published_by=None, + expected_children_ids=[ + self.unit["id"], + self.unit_2["id"], + self.unit_3["id"], + ], + ) + self._verify_publish_state( + container_id=self.subsection["id"], + expected_childen_len=2, + expected_has_unpublished_changes=True, + expected_published_by=None, + expected_children_ids=[ + self.unit["id"], + unit_4["id"], + ], + ) + + # Now publish only subsection 1 + self._publish_container(self.subsection_with_units["id"]) + + # Now it is published: + self._verify_publish_state( + container_id=self.subsection_with_units["id"], + expected_childen_len=3, + expected_has_unpublished_changes=False, + expected_published_by=self.user.username, + expected_children_ids=[ + self.unit["id"], + self.unit_2["id"], + self.unit_3["id"], + ], + ) + + # and subsection 2 is still unpublished, except for the shared unit that is also in subsection 1: + c2_after = self._get_container(self.subsection["id"]) + assert c2_after["has_unpublished_changes"] + c2_units_after = self._get_container_children(self.subsection["id"]) + assert len(c2_units_after) == 2 + assert c2_units_after[0]["id"] == self.unit["id"] + assert c2_units_after[0]["has_unpublished_changes"] is False # published since it's also in subsection 1 + assert c2_units_after[0]["published_by"] == self.user.username + assert c2_units_after[1]["id"] == unit_4["id"] + assert c2_units_after[1]["has_unpublished_changes"] # unaffected + assert c2_units_after[1]["published_by"] is None + + def test_publish_section(self) -> None: + """ + Test that we can publish the changes to a specific section + """ + subsection_4 = self._create_container(self.lib["id"], "subsection", display_name="Test Subsection 4", slug=None) + self._add_container_children( + self.section["id"], + children_ids=[ + self.subsection["id"], + subsection_4["id"], + ] + ) + # Removing the subsection with units because the units (children of children) are not published. + # If the subsection is kept, the section continues to have changes even after it is published. + self._remove_container_children( + self.section_with_subsections["id"], + children_ids=[ + self.subsection_with_units["id"], + ] + ) + + # At first everything is unpublished: + self._verify_publish_state( + container_id=self.section_with_subsections["id"], + expected_childen_len=3, + expected_has_unpublished_changes=True, + expected_published_by=None, + expected_children_ids=[ + self.subsection["id"], + self.subsection_2["id"], + self.subsection_3["id"], + ], + ) + self._verify_publish_state( + container_id=self.section["id"], + expected_childen_len=2, + expected_has_unpublished_changes=True, + expected_published_by=None, + expected_children_ids=[ + self.subsection["id"], + subsection_4["id"], + ], + ) + + # Now publish only section 1 + self._publish_container(self.section_with_subsections["id"]) + + # Now it is published: + self._verify_publish_state( + container_id=self.section_with_subsections["id"], + expected_childen_len=3, + expected_has_unpublished_changes=False, + expected_published_by=self.user.username, + expected_children_ids=[ + self.subsection["id"], + self.subsection_2["id"], + self.subsection_3["id"], + ], + ) + + # and section 2 is still unpublished, except for the shared subsection that is also in section 1: + c2_after = self._get_container(self.section["id"]) + assert c2_after["has_unpublished_changes"] + c2_units_after = self._get_container_children(self.section["id"]) + assert len(c2_units_after) == 2 + assert c2_units_after[0]["id"] == self.subsection["id"] + assert c2_units_after[0]["has_unpublished_changes"] is False # published since it's also in section 1 + assert c2_units_after[0]["published_by"] == self.user.username + assert c2_units_after[1]["id"] == subsection_4["id"] + assert c2_units_after[1]["has_unpublished_changes"] # unaffected + assert c2_units_after[1]["published_by"] is None From b8071aafdaf79e23e6d42a5499f11722b2e3d6c4 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 18 Jun 2025 01:36:22 +0930 Subject: [PATCH 02/14] test: published_by is now None for unpublished containers --- .../content_libraries/tests/test_course_to_library.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_course_to_library.py b/openedx/core/djangoapps/content_libraries/tests/test_course_to_library.py index 5c1bd5774403..5b77791e9377 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_course_to_library.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_course_to_library.py @@ -52,7 +52,7 @@ def test_library_paste_unit_from_course(self): "created": paste_data["created"], "modified": paste_data["modified"], "last_published": None, - "published_by": "", + "published_by": None, "has_unpublished_changes": True, "collections": [], "can_stand_alone": True, From 7bf61d39aa91a865e145fed47824a8cbb0fc6e41 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 18 Jun 2025 02:16:50 +0930 Subject: [PATCH 03/14] test: adds TODO comments to the tests in anticipation of publishing container children --- .../djangoapps/content_libraries/tests/test_containers.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index d9c54289031f..1f7c4aa086d8 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -667,6 +667,9 @@ def test_publish_subsection(self) -> None: unit_4["id"], ] ) + + # TODO -- remove this when containers publish their children: + # https://github.com/openedx/openedx-learning/pull/307 # Removing the unit with components because the components (children of children) are not published. # If the unit is kept, the subsection continues to have changes even after it is published. self._remove_container_children( @@ -675,6 +678,7 @@ def test_publish_subsection(self) -> None: self.unit_with_components["id"], ] ) + # /TODO # At first everything is unpublished: self._verify_publish_state( @@ -739,6 +743,9 @@ def test_publish_section(self) -> None: subsection_4["id"], ] ) + + # TODO -- remove this when containers publish their children: + # https://github.com/openedx/openedx-learning/pull/307 # Removing the subsection with units because the units (children of children) are not published. # If the subsection is kept, the section continues to have changes even after it is published. self._remove_container_children( @@ -747,6 +754,7 @@ def test_publish_section(self) -> None: self.subsection_with_units["id"], ] ) + # /TODO # At first everything is unpublished: self._verify_publish_state( From c19a64c90c8caabf542fdbe722db890200bdc711 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 24 Jun 2025 05:44:57 +0930 Subject: [PATCH 04/14] feat: adds api to retrieve library block/container hierarchy --- .../content_libraries/api/containers.py | 189 +++++++++++++++++- .../content_libraries/rest_api/blocks.py | 63 ++++-- .../content_libraries/rest_api/containers.py | 25 +++ .../content_libraries/rest_api/serializers.py | 12 ++ .../content_libraries/tests/base.py | 12 ++ .../tests/test_containers.py | 160 ++++++++++++--- .../core/djangoapps/content_libraries/urls.py | 4 + 7 files changed, 414 insertions(+), 51 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index dd613cc4b28c..e4f48e8c0e0f 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -3,12 +3,13 @@ """ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field as dataclass_field from datetime import datetime, timezone from enum import Enum import logging from uuid import uuid4 +from django.db.models import QuerySet from django.utils.text import slugify from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_events.content_authoring.data import ( @@ -26,6 +27,7 @@ from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Container, ContainerVersion, Component from openedx.core.djangoapps.content_libraries.api.collections import library_collection_locator +from openedx.core.djangoapps.content_libraries.api.block_metadata import LibraryXBlockMetadata from openedx.core.djangoapps.xblock.api import get_component_from_usage_key @@ -40,6 +42,7 @@ # Models "ContainerMetadata", "ContainerType", + "LibraryObjectHierarchy", # API methods "get_container", "create_container", @@ -52,6 +55,7 @@ "update_container_children", "get_containers_contains_item", "publish_container_changes", + "get_library_object_hierarchy", ] log = logging.getLogger(__name__) @@ -610,3 +614,186 @@ def publish_container_changes(container_key: LibraryContainerLocator, user_id: i # Update the search index (and anything else) for the affected container + blocks # This is mostly synchronous but may complete some work asynchronously if there are a lot of changes. tasks.wait_for_post_publish_events(publish_log, library_key) + + +@dataclass(frozen=True, kw_only=True) +class LibraryObjectHierarchy: + """ + Describes the full ancestry and descendents of a given library object. + """ + sections: list[ContainerMetadata] = dataclass_field(default_factory=list) + subsections: list[ContainerMetadata] = dataclass_field(default_factory=list) + units: list[ContainerMetadata] = dataclass_field(default_factory=list) + components: list[LibraryXBlockMetadata] = dataclass_field(default_factory=list) + object_key: LibraryUsageLocatorV2 | LibraryContainerLocator + + def append( + self, + level: str, + *items: Component | Container | LibraryXBlockMetadata | ContainerMetadata, + ) -> None: + """ + Appends the metadata for the given items to the given level of the hierarchy. + """ + for item in items: + # Convert item to metadata if needed + if level == "components": + if isinstance(item, Component): + metadata = LibraryXBlockMetadata.from_component( + self.object_key.context_key, + item, + ) + else: + assert isinstance(item, LibraryXBlockMetadata) + metadata = item + + self.components.append(metadata) + continue + + if isinstance(item, Container): + metadata = ContainerMetadata.from_container( + self.object_key.context_key, + item, + ) + else: + assert isinstance(item, ContainerMetadata) + metadata = item + + if level == 'units': + self.units.append(metadata) + elif level == 'subsections': + self.subsections.append(metadata) + elif level == 'sections': + self.sections.append(metadata) + else: + raise TypeError(f"Invalid level: {level}") + + @staticmethod + def parent_level(level: str | None) -> str | None: + """ + Returns the name of the parent field above the given level, + or None if level is already the top level. + """ + match level: + case "components": + return "units" + case "units": + return "subsections" + case "subsections": + return "sections" + case _: + return None + + @staticmethod + def child_level(level: str | None) -> str | None: + """ + Returns the name of the child field below the given level, + or None if level is already the lowest level. + """ + match level: + case "sections": + return "subsections" + case "subsections": + return "units" + case "units": + return "components" + case _: + return None + + @classmethod + def create_from_library_object_key( + cls, + object_key: LibraryUsageLocatorV2 | LibraryContainerLocator, + ): + """ + Returns a LibraryObjectHierarchy populated from the library object represented by the given object_key. + """ + root_items: list[Component] | list[Container] + root_level: str + + if isinstance(object_key, LibraryUsageLocatorV2): + root_items = [get_component_from_usage_key(object_key)] + root_level = "components" + + elif isinstance(object_key, LibraryContainerLocator): + root_items = [_get_container_from_key(object_key)] + root_level = f"{object_key.container_type}s" + + else: + raise TypeError(f"Unexpected '{object_key}': must be LibraryUsageLocatorv2 or LibraryContainerLocator") + + # Fill in root level of hierarchy + hierarchy = cls(object_key=object_key) + items = root_items + hierarchy.append(root_level, *items) + level: str | None = root_level + + # Fill in hierarchy up through parents + while level := hierarchy.parent_level(level): + items = list(_get_containers_with_entities(items).all()) + hierarchy.append(level, *items) + + # Fill in hierarchy down from root_level. + if root_level != 'components': # Components have no children + level = root_level + children = getattr(hierarchy, level) + while level := hierarchy.child_level(level): + children = _get_containers_children(children) + hierarchy.append(level, *children) + + return hierarchy + + +def _get_containers_with_entities( + entities: list[Container] | list[Component], + *, + ignore_pinned=False, +) -> QuerySet[Container]: + """ + Find all draft containers that directly contain the given entities. + + Args: + entities: iterable list or queryset of PublishableEntities. + ignore_pinned: if true, ignore any pinned references to the entity. + """ + qs = Container.objects.none() + for entity in entities: + qs = qs.union(authoring_api.get_containers_with_entity( + entity.publishable_entity.pk, + ignore_pinned=ignore_pinned, + )) + return qs + + +def _get_containers_children( + containers: list[ContainerMetadata], + *, + published=False, +) -> list[LibraryXBlockMetadata | ContainerMetadata]: + """ + Find all components or containers directly contained by the given containers. + + Args: + containers: iterable list or queryset of Containers of the same type. + published: `True` if we want the published version of the children, or + `False` for the draft version. + """ + children: list[LibraryXBlockMetadata | ContainerMetadata] = [] + for container in containers: + children.extend( + get_container_children( + container.container_key, + published=published, + ) + ) + + return children + + +def get_library_object_hierarchy( + object_key: LibraryUsageLocatorV2 | LibraryContainerLocator, +) -> LibraryObjectHierarchy: + """ + Returns the full ancestry and descendents of the library object with the given object_key. + """ + return LibraryObjectHierarchy.create_from_library_object_key(object_key) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index bc314099893c..706998d307c8 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -18,14 +18,7 @@ from rest_framework.views import APIView from openedx.core.djangoapps.content_libraries import api, permissions -from openedx.core.djangoapps.content_libraries.rest_api.serializers import ( - ContentLibraryItemCollectionsUpdateSerializer, - LibraryXBlockCreationSerializer, - LibraryXBlockMetadataSerializer, - LibraryXBlockOlxSerializer, - LibraryXBlockStaticFileSerializer, - LibraryXBlockStaticFilesSerializer, -) +from openedx.core.djangoapps.content_libraries.rest_api import serializers from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.lib.api.view_utils import view_auth_classes from openedx.core.types.http import RestRequest @@ -40,7 +33,7 @@ class LibraryBlocksView(GenericAPIView): """ Views to work with XBlocks in a specific content library. """ - serializer_class = LibraryXBlockMetadataSerializer + serializer_class = serializers.LibraryXBlockMetadataSerializer @apidocs.schema( parameters=[ @@ -74,13 +67,13 @@ def get(self, request, lib_key_str): api.LibraryXBlockMetadata.from_component(key, component) for component in self.paginate_queryset(components) ] - serializer = LibraryXBlockMetadataSerializer(paginated_xblock_metadata, many=True) + serializer = self.serializer_class(paginated_xblock_metadata, many=True) return self.get_paginated_response(serializer.data) @convert_exceptions @swagger_auto_schema( - request_body=LibraryXBlockCreationSerializer, - responses={200: LibraryXBlockMetadataSerializer} + request_body=serializers.LibraryXBlockCreationSerializer, + responses={200: serializers.LibraryXBlockMetadataSerializer} ) def post(self, request, lib_key_str): """ @@ -88,7 +81,7 @@ def post(self, request, lib_key_str): """ library_key = LibraryLocatorV2.from_string(lib_key_str) api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) - serializer = LibraryXBlockCreationSerializer(data=request.data) + serializer = serializers.LibraryXBlockCreationSerializer(data=request.data) serializer.is_valid(raise_exception=True) # Create a new regular top-level block: @@ -99,7 +92,7 @@ def post(self, request, lib_key_str): detail={'block_type': str(err)}, ) - return Response(LibraryXBlockMetadataSerializer(result).data) + return Response(self.serializer_class(result).data) @method_decorator(non_atomic_requests, name="dispatch") @@ -108,6 +101,8 @@ class LibraryBlockView(APIView): """ Views to work with an existing XBlock in a content library. """ + serializer_class = serializers.LibraryXBlockMetadataSerializer + @convert_exceptions def get(self, request, usage_key_str): """ @@ -123,7 +118,7 @@ def get(self, request, usage_key_str): api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) result = api.get_library_block(key, include_collections=True) - return Response(LibraryXBlockMetadataSerializer(result).data) + return Response(self.serializer_class(result).data) @convert_exceptions def delete(self, request, usage_key_str): # pylint: disable=unused-argument @@ -150,6 +145,8 @@ class LibraryBlockAssetListView(APIView): """ Views to list an existing XBlock's static asset files """ + serializer_class = serializers.LibraryXBlockStaticFilesSerializer + @convert_exceptions def get(self, request, usage_key_str): """ @@ -158,7 +155,7 @@ 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) files = api.get_library_block_static_asset_files(key) - return Response(LibraryXBlockStaticFilesSerializer({"files": files}).data) + return Response(self.serializer_class({"files": files}).data) @method_decorator(non_atomic_requests, name="dispatch") @@ -168,6 +165,7 @@ class LibraryBlockAssetView(APIView): Views to work with an existing XBlock's static asset files """ parser_classes = (MultiPartParser, ) + serializer_class = serializers.LibraryXBlockStaticFileSerializer @convert_exceptions def get(self, request, usage_key_str, file_path): @@ -179,7 +177,7 @@ def get(self, request, usage_key_str, file_path): files = api.get_library_block_static_asset_files(key) for f in files: if f.path == file_path: - return Response(LibraryXBlockStaticFileSerializer(f).data) + return Response(self.serializer_class(f).data) raise NotFound @convert_exceptions @@ -206,7 +204,7 @@ def put(self, request, usage_key_str, file_path): result = api.add_library_block_static_asset_file(usage_key, file_path, file_content, request.user) except ValueError: raise ValidationError("Invalid file path") # lint-amnesty, pylint: disable=raise-missing-from - return Response(LibraryXBlockStaticFileSerializer(result).data) + return Response(self.serializer_class(result).data) @convert_exceptions def delete(self, request, usage_key_str, file_path): @@ -265,7 +263,7 @@ def patch(self, request: RestRequest, usage_key_str) -> Response: request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - serializer = ContentLibraryItemCollectionsUpdateSerializer(data=request.data) + serializer = serializers.ContentLibraryItemCollectionsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) component = api.get_component_from_usage_key(key) @@ -309,6 +307,8 @@ class LibraryBlockOlxView(APIView): """ Views to work with an existing XBlock's OLX """ + serializer_class = serializers.LibraryXBlockOlxSerializer + @convert_exceptions def get(self, request, usage_key_str): """ @@ -320,7 +320,7 @@ 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) xml_str = xblock_api.get_block_draft_olx(key) - return Response(LibraryXBlockOlxSerializer({"olx": xml_str}).data) + return Response(self.serializer_class({"olx": xml_str}).data) @convert_exceptions def post(self, request, usage_key_str): @@ -332,14 +332,14 @@ def post(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_EDIT_THIS_CONTENT_LIBRARY) - serializer = LibraryXBlockOlxSerializer(data=request.data) + serializer = self.serializer_class(data=request.data) 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 except ValueError as err: raise ValidationError(detail=str(err)) # lint-amnesty, pylint: disable=raise-missing-from - return Response(LibraryXBlockOlxSerializer({"olx": new_olx_str, "version_num": version_num}).data) + return Response(self.serializer_class({"olx": new_olx_str, "version_num": version_num}).data) @view_auth_classes() @@ -358,6 +358,25 @@ def post(self, request, usage_key_str) -> Response: return Response(None, status=status.HTTP_204_NO_CONTENT) +@method_decorator(non_atomic_requests, name="dispatch") +@view_auth_classes() +class LibraryBlockHierarchy(GenericAPIView): + """ + View to return the full hierarchy of containers that contain a library block. + """ + serializer_class = serializers.LibraryObjectHierarchySerializer + + @convert_exceptions + def get(self, request, usage_key_str) -> Response: + """ + Fetches and returns the full container hierarchy for the given library block. + """ + key = LibraryUsageLocatorV2.from_string(usage_key_str) + api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + hierarchy = api.get_library_object_hierarchy(key) + return Response(self.serializer_class(hierarchy).data) + + def get_component_version_asset(request, component_version_uuid, asset_path): """ Serves static assets associated with particular Component versions. diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 36118346daa6..af1e4633f472 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -347,3 +347,28 @@ def post(self, request: RestRequest, container_key: LibraryContainerLocator) -> # If we need to in the future, we could return a list of all the child containers/components that were # auto-published as a result. return Response({}) + + +@method_decorator(non_atomic_requests, name="dispatch") +@view_auth_classes() +class LibraryContainerHierarchy(GenericAPIView): + """ + View to return the full hierarchy of containers that contain and are contained by a library container. + """ + serializer_class = serializers.LibraryObjectHierarchySerializer + + @convert_exceptions + @swagger_auto_schema( + responses={200: serializers.LibraryObjectHierarchySerializer} + ) + def get(self, request: RestRequest, container_key: LibraryContainerLocator) -> Response: + """ + Fetches and returns the full container hierarchy for the given library block. + """ + api.require_permission_for_library_key( + container_key.lib_key, + request.user, + permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, + ) + hierarchy = api.get_library_object_hierarchy(container_key) + return Response(self.serializer_class(hierarchy).data) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index 6734babd00cc..a719a7a8f982 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -386,3 +386,15 @@ class ContentLibraryItemCollectionsUpdateSerializer(serializers.Serializer): """ collection_keys = serializers.ListField(child=serializers.CharField(), allow_empty=True) + + +class LibraryObjectHierarchySerializer(serializers.Serializer): + """ + Serializer which represents the full hierarchy of containers and components that contain and are contained by a + given library container or library block. + """ + sections = serializers.ListField(child=LibraryContainerMetadataSerializer(), allow_empty=True) + subsections = serializers.ListField(child=LibraryContainerMetadataSerializer(), allow_empty=True) + units = serializers.ListField(child=LibraryContainerMetadataSerializer(), allow_empty=True) + components = serializers.ListField(child=LibraryXBlockMetadataSerializer(), allow_empty=True) + object_key = OpaqueKeySerializer() diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 02e9cd251e4f..b394d8ee3cca 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -37,8 +37,10 @@ URL_LIB_BLOCK_OLX = URL_LIB_BLOCK + 'olx/' # Get or set the OLX of the specified XBlock URL_LIB_BLOCK_ASSETS = URL_LIB_BLOCK + 'assets/' # List the static asset files of the specified XBlock URL_LIB_BLOCK_ASSET_FILE = URL_LIB_BLOCK + 'assets/{file_name}' # Get, delete, or upload a specific static asset file +URL_LIB_BLOCK_HIERARCHY = URL_LIB_BLOCK + 'hierarchy/' # Get a library block's full hierarchy URL_LIB_CONTAINER = URL_PREFIX + 'containers/{container_key}/' # Get a container in this library URL_LIB_CONTAINER_CHILDREN = URL_LIB_CONTAINER + 'children/' # Get, add or delete a component in this container +URL_LIB_CONTAINER_HIERARCHY = URL_LIB_CONTAINER + 'hierarchy/' # Get a container's full hierarchy URL_LIB_CONTAINER_RESTORE = URL_LIB_CONTAINER + 'restore/' # Restore a deleted container URL_LIB_CONTAINER_COLLECTIONS = URL_LIB_CONTAINER + 'collections/' # Handle associated collections URL_LIB_CONTAINER_PUBLISH = URL_LIB_CONTAINER + 'publish/' # Publish changes to the specified container + children @@ -465,6 +467,16 @@ def _publish_container(self, container_key: ContainerKey | str, expect_response= """ Publish all changes in the specified container + children """ return self._api('post', URL_LIB_CONTAINER_PUBLISH.format(container_key=container_key), None, expect_response) + def _get_block_hierarchy(self, block_key, expect_response=200): + """ Returns the hierarchy of containers that contain the given block """ + url = URL_LIB_BLOCK_HIERARCHY.format(block_key=block_key) + return self._api('get', url, None, expect_response) + + def _get_container_hierarchy(self, container_key, expect_response=200): + """ Returns the hierarchy of containers that contain and are contained by the given container """ + url = URL_LIB_CONTAINER_HIERARCHY.format(container_key=container_key) + return self._api('get', url, None, expect_response) + def _create_collection( self, lib_key: LibraryLocatorV2 | str, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 1f7c4aa086d8..df9afd029853 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -38,6 +38,7 @@ class ContainersTestCase(ContentLibrariesRestApiTest): def setUp(self) -> None: super().setUp() self.create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) + self.modified_date = datetime(2024, 10, 9, 8, 7, 6, tzinfo=timezone.utc) self.lib = self._create_library( slug="containers", title="Container Test Library", @@ -99,36 +100,45 @@ def setUp(self) -> None: 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") - # Add components to `unit_with_components` - self._add_container_children( - self.unit_with_components["id"], - children_ids=[ - self.problem_block["id"], - self.html_block["id"], - self.problem_block_2["id"], - self.html_block_2["id"], - ], - ) - # Add units to `subsection_with_units` - self._add_container_children( - self.subsection_with_units["id"], - children_ids=[ - self.unit["id"], + with freeze_time(self.modified_date): + # Add components to `unit_with_components` + self._add_container_children( self.unit_with_components["id"], - self.unit_2["id"], - self.unit_3["id"], - ], - ) - # Add subsections to `section_with_subsections` - self._add_container_children( - self.section_with_subsections["id"], - children_ids=[ - self.subsection["id"], + children_ids=[ + self.problem_block["id"], + self.html_block["id"], + self.problem_block_2["id"], + self.html_block_2["id"], + ], + ) + # Refetch to update modified dates + self.unit_with_components = self._get_container(self.unit_with_components["id"]) + + # Add units to `subsection_with_units` + self._add_container_children( self.subsection_with_units["id"], - self.subsection_2["id"], - self.subsection_3["id"], - ], - ) + children_ids=[ + self.unit["id"], + self.unit_with_components["id"], + self.unit_2["id"], + self.unit_3["id"], + ], + ) + # Refetch to update modified dates + self.subsection_with_units = self._get_container(self.subsection_with_units["id"]) + + # Add subsections to `section_with_subsections` + self._add_container_children( + self.section_with_subsections["id"], + children_ids=[ + self.subsection["id"], + self.subsection_with_units["id"], + self.subsection_2["id"], + self.subsection_3["id"], + ], + ) + # Refetch to update modified dates + self.section_with_subsections = self._get_container(self.section_with_subsections["id"]) @ddt.data( ("unit", "u1", "Test Unit"), @@ -566,6 +576,100 @@ def test_unit_collections(self) -> None: # Verify the collections assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] + def test_section_hierarchy(self): + hierarchy = self._get_container_hierarchy(self.section_with_subsections["id"]) + assert hierarchy["object_key"] == self.section_with_subsections["id"] + assert hierarchy["components"] == [ + self.problem_block, + self.html_block, + self.problem_block_2, + self.html_block_2, + ] + assert hierarchy["units"] == [ + self.unit, + self.unit_with_components, + self.unit_2, + self.unit_3, + ] + assert hierarchy["subsections"] == [ + self.subsection, + self.subsection_with_units, + self.subsection_2, + self.subsection_3, + ] + assert hierarchy["sections"] == [ + self.section_with_subsections, + ] + + def test_subsection_hierarchy(self): + hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) + assert hierarchy["object_key"] == self.subsection_with_units["id"] + assert hierarchy["components"] == [ + self.problem_block, + self.html_block, + self.problem_block_2, + self.html_block_2, + ] + assert hierarchy["units"] == [ + self.unit, + self.unit_with_components, + self.unit_2, + self.unit_3, + ] + assert hierarchy["subsections"] == [ + self.subsection_with_units, + ] + assert hierarchy["sections"] == [ + self.section_with_subsections, + ] + + def test_units_hierarchy(self): + hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) + assert hierarchy["object_key"] == self.unit_with_components["id"] + assert hierarchy["components"] == [ + self.problem_block, + self.html_block, + self.problem_block_2, + self.html_block_2, + ] + assert hierarchy["units"] == [ + self.unit_with_components, + ] + assert hierarchy["subsections"] == [ + self.subsection_with_units, + ] + assert hierarchy["sections"] == [ + self.section_with_subsections, + ] + + def test_container_hierarchy_not_found(self): + self._get_container_hierarchy( + "lct:CL-TEST:containers:section:does-not-exist", + expect_response=404, + ) + + def test_block_hierarchy(self): + hierarchy = self._get_block_hierarchy(self.problem_block["id"]) + assert hierarchy["object_key"] == self.problem_block["id"] + assert hierarchy["components"] == [ + self.problem_block, + ] + assert hierarchy["units"] == [ + self.unit_with_components, + ] + assert hierarchy["subsections"] == [ + self.subsection_with_units, + ] + assert hierarchy["sections"] == [ + self.section_with_subsections, + ] + + def test_block_hierarchy_not_found(self): + self._get_block_hierarchy( + "lb:CL-TEST:problem:does-not-exist", + expect_response=404, + ) + def _verify_publish_state( self, container_id, diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 2b9cd59af7e9..ad4899762f30 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -64,6 +64,8 @@ path('restore/', blocks.LibraryBlockRestore.as_view()), # Update collections for a given component path('collections/', blocks.LibraryBlockCollectionsView.as_view(), name='update-collections'), + # Get the full hierarchy that the block belongs to + path('hierarchy/', blocks.LibraryBlockHierarchy.as_view()), # Get the LTI URL of a specific XBlock path('lti/', blocks.LibraryBlockLtiUrlView.as_view(), name='lti-url'), # Get the OLX source code of the specified block: @@ -80,6 +82,8 @@ path('', containers.LibraryContainerView.as_view()), # update components under container path('children/', containers.LibraryContainerChildrenView.as_view()), + # Get the full hierarchy that the container belongs to + path('hierarchy/', containers.LibraryContainerHierarchy.as_view()), # Restore a soft-deleted container path('restore/', containers.LibraryContainerRestore.as_view()), # Update collections for a given container From b9020f37607f9563c701ed83829ebf3652192971 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 25 Jun 2025 06:04:32 +0930 Subject: [PATCH 05/14] test: adds query counts for hierarchy API tests These are really high, but highlight the need for future optimizations. --- .../content_libraries/tests/test_containers.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index df9afd029853..b44708ea37eb 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -577,7 +577,8 @@ def test_unit_collections(self) -> None: assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] def test_section_hierarchy(self): - hierarchy = self._get_container_hierarchy(self.section_with_subsections["id"]) + with self.assertNumQueries(272): + hierarchy = self._get_container_hierarchy(self.section_with_subsections["id"]) assert hierarchy["object_key"] == self.section_with_subsections["id"] assert hierarchy["components"] == [ self.problem_block, @@ -602,7 +603,8 @@ def test_section_hierarchy(self): ] def test_subsection_hierarchy(self): - hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) + with self.assertNumQueries(179): + hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) assert hierarchy["object_key"] == self.subsection_with_units["id"] assert hierarchy["components"] == [ self.problem_block, @@ -624,7 +626,8 @@ def test_subsection_hierarchy(self): ] def test_units_hierarchy(self): - hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) + with self.assertNumQueries(89): + hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) assert hierarchy["object_key"] == self.unit_with_components["id"] assert hierarchy["components"] == [ self.problem_block, @@ -649,7 +652,8 @@ def test_container_hierarchy_not_found(self): ) def test_block_hierarchy(self): - hierarchy = self._get_block_hierarchy(self.problem_block["id"]) + with self.assertNumQueries(32): + hierarchy = self._get_block_hierarchy(self.problem_block["id"]) assert hierarchy["object_key"] == self.problem_block["id"] assert hierarchy["components"] == [ self.problem_block, From 6fd8dca388f6a9115f33be556def7ecdc5b0de78 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 27 Jun 2025 05:16:25 +0930 Subject: [PATCH 06/14] perf: reduce hierarchy API query counts --- .../djangoapps/content_libraries/tests/test_containers.py | 8 ++++---- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 3 ++- requirements/edx/testing.txt | 2 +- 6 files changed, 10 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index b6fd548738fc..9c5201c3df74 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -603,7 +603,7 @@ def test_unit_collections(self) -> None: assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] def test_section_hierarchy(self): - with self.assertNumQueries(281): + with self.assertNumQueries(225): hierarchy = self._get_container_hierarchy(self.section_with_subsections["id"]) assert hierarchy["object_key"] == self.section_with_subsections["id"] assert hierarchy["components"] == [ @@ -629,7 +629,7 @@ def test_section_hierarchy(self): ] def test_subsection_hierarchy(self): - with self.assertNumQueries(185): + with self.assertNumQueries(147): hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) assert hierarchy["object_key"] == self.subsection_with_units["id"] assert hierarchy["components"] == [ @@ -652,7 +652,7 @@ def test_subsection_hierarchy(self): ] def test_units_hierarchy(self): - with self.assertNumQueries(92): + with self.assertNumQueries(72): hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) assert hierarchy["object_key"] == self.unit_with_components["id"] assert hierarchy["components"] == [ @@ -678,7 +678,7 @@ def test_container_hierarchy_not_found(self): ) def test_block_hierarchy(self): - with self.assertNumQueries(35): + with self.assertNumQueries(29): hierarchy = self._get_block_hierarchy(self.problem_block["id"]) assert hierarchy["object_key"] == self.problem_block["id"] assert hierarchy["components"] == [ diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index ce3567fcebdf..26bfdc208a5e 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -840,7 +840,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.0 # via -r requirements/edx/kernel.in -openedx-learning==0.26.0 +openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 66201eb8771f..ac267999a1bb 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1400,7 +1400,7 @@ openedx-forum==0.3.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-learning==0.26.0 +openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 675592a5360a..5bc5c684652c 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1005,7 +1005,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.0 # via -r requirements/edx/base.txt -openedx-learning==0.26.0 +openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index fcc502755c57..b9d83fa6a7d1 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -120,7 +120,8 @@ openedx-django-require openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) openedx-forum # Open edX forum v2 application -openedx-learning # Open edX Learning core (experimental) +# TODO -- https://github.com/openedx/openedx-learning/pull/333 +git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head#egg=openedx-learning openedx-mongodbproxy openedx-django-wiki path diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 0bbbe3f30180..9984baafe3f4 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1064,7 +1064,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.0 # via -r requirements/edx/base.txt -openedx-learning==0.26.0 +openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 6fedb3b67b73b5e5c2cc40629fc1016a4c764c56 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 2 Jul 2025 22:22:52 +0930 Subject: [PATCH 07/14] perf: cut query counts in half Required a refactor of the approach to avoid using the Metadata classes. --- .../content_libraries/api/containers.py | 266 +++++++++++------- .../content_libraries/rest_api/blocks.py | 2 +- .../content_libraries/rest_api/containers.py | 4 +- .../content_libraries/rest_api/serializers.py | 19 +- .../content_libraries/tests/base.py | 11 + .../tests/test_containers.py | 76 ++--- 6 files changed, 228 insertions(+), 150 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 059a4c1c0619..67eeb9f4fbc3 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -1,4 +1,4 @@ -"""D +""" API for containers (Sections, Subsections, Units) in Content Libraries """ from __future__ import annotations @@ -25,7 +25,7 @@ LIBRARY_CONTAINER_UPDATED, ) from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import Container, ContainerVersion, Component +from openedx_learning.api.authoring_models import Container, ContainerVersion, Component, PublishableEntity from openedx.core.djangoapps.content_libraries.api.collections import library_collection_locator from openedx.core.djangoapps.content_libraries.api.block_metadata import LibraryXBlockMetadata from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts @@ -34,7 +34,7 @@ from ..models import ContentLibrary from .exceptions import ContentLibraryContainerNotFound -from .libraries import PublishableItem +from .libraries import PublishableItem, library_component_usage_key from .block_metadata import LibraryXBlockMetadata from .. import tasks @@ -43,7 +43,7 @@ # Models "ContainerMetadata", "ContainerType", - "LibraryObjectHierarchy", + "ContainerHierarchy", # API methods "get_container", "create_container", @@ -629,88 +629,138 @@ def publish_container_changes(container_key: LibraryContainerLocator, user_id: i @dataclass(frozen=True, kw_only=True) -class LibraryObjectHierarchy: +class ContainerHierarchyMember: + """ + Represents an individual member of ContainerHierarchy which is ready to be serialized. + """ + id: LibraryContainerLocator | LibraryUsageLocatorV2 + display_name: str + has_unpublished_changes: bool + component: Component | None + container: Container | None + + @classmethod + def create( + cls, + library_key: LibraryLocatorV2, + entity: Container | Component, + ) -> ContainerHierarchyMember: + """ + Creates a ContainerHierarchyMember. + + Arguments: + * library_key: required for generating a usage/locator key for the given entitity. + * entity: the Container or Component + """ + if isinstance(entity, Component): + return ContainerHierarchyMember( + id=library_component_usage_key(library_key, entity), + display_name=entity.versioning.draft.title, + has_unpublished_changes=entity.versioning.has_unpublished_changes, + component=entity, + container=None, + ) + assert isinstance(entity, Container) + return ContainerHierarchyMember( + id=library_container_locator( + library_key, + container=entity, + ), + display_name=entity.versioning.draft.title, + has_unpublished_changes=authoring_api.contains_unpublished_changes(entity.pk), + container=entity, + component=None, + ) + + @property + def entity(self) -> PublishableEntity: + """ + Returns the PublishableEntity associated with this member. + + Raises AssertError if there isn't a Component or Container set. + """ + entity = self.component or self.container + assert entity + return entity.publishable_entity + + +@dataclass(frozen=True, kw_only=True) +class ContainerHierarchy: """ Describes the full ancestry and descendents of a given library object. """ - sections: list[ContainerMetadata] = dataclass_field(default_factory=list) - subsections: list[ContainerMetadata] = dataclass_field(default_factory=list) - units: list[ContainerMetadata] = dataclass_field(default_factory=list) - components: list[LibraryXBlockMetadata] = dataclass_field(default_factory=list) + sections: list[ContainerHierarchyMember] = dataclass_field(default_factory=list) + subsections: list[ContainerHierarchyMember] = dataclass_field(default_factory=list) + units: list[ContainerHierarchyMember] = dataclass_field(default_factory=list) + components: list[ContainerHierarchyMember] = dataclass_field(default_factory=list) object_key: LibraryUsageLocatorV2 | LibraryContainerLocator + class Level(Enum): + """ + Enumeratable levels contained by the ContainerHierarchy. + """ + none = 0 + components = 1 + units = 2 + subsections = 3 + sections = 4 + + def __bool__(self) -> bool: + """ + Level.none is False + All others are True. + """ + return self != ContainerHierarchy.Level.none + + @property + def parent(self) -> ContainerHierarchy.Level: + """ + Returns the parent level above the given level, + or Level.none if this is already the top level. + """ + if not self: + return self + try: + return ContainerHierarchy.Level(self.value + 1) + except ValueError: + return ContainerHierarchy.Level.none + + @property + def child(self) -> ContainerHierarchy.Level: + """ + Returns the name of the child field below the given level, + or None if level is already the lowest level. + """ + if not self: + return self + try: + return ContainerHierarchy.Level(self.value - 1) + except ValueError: + return ContainerHierarchy.Level.none + def append( self, - level: str, - *items: Component | Container | LibraryXBlockMetadata | ContainerMetadata, - ) -> None: + level: Level, + *items: Component | Container, + ) -> list[ContainerHierarchyMember]: """ Appends the metadata for the given items to the given level of the hierarchy. + Returns the resulting list. + + Arguments: + * level: a valid Level (not Level.none) + * ...list of Components or Containers to add to this level. """ + assert level for item in items: - # Convert item to metadata if needed - if level == "components": - if isinstance(item, Component): - metadata = LibraryXBlockMetadata.from_component( - self.object_key.context_key, - item, - ) - else: - assert isinstance(item, LibraryXBlockMetadata) - metadata = item - - self.components.append(metadata) - continue - - if isinstance(item, Container): - metadata = ContainerMetadata.from_container( + getattr(self, level.name).append( + ContainerHierarchyMember.create( self.object_key.context_key, item, ) - else: - assert isinstance(item, ContainerMetadata) - metadata = item - - if level == 'units': - self.units.append(metadata) - elif level == 'subsections': - self.subsections.append(metadata) - elif level == 'sections': - self.sections.append(metadata) - else: - raise TypeError(f"Invalid level: {level}") - - @staticmethod - def parent_level(level: str | None) -> str | None: - """ - Returns the name of the parent field above the given level, - or None if level is already the top level. - """ - match level: - case "components": - return "units" - case "units": - return "subsections" - case "subsections": - return "sections" - case _: - return None - - @staticmethod - def child_level(level: str | None) -> str | None: - """ - Returns the name of the child field below the given level, - or None if level is already the lowest level. - """ - match level: - case "sections": - return "subsections" - case "subsections": - return "units" - case "units": - return "components" - case _: - return None + ) + + return getattr(self, level.name) @classmethod def create_from_library_object_key( @@ -718,46 +768,47 @@ def create_from_library_object_key( object_key: LibraryUsageLocatorV2 | LibraryContainerLocator, ): """ - Returns a LibraryObjectHierarchy populated from the library object represented by the given object_key. + Returns a ContainerHierarchy populated from the library object represented by the given object_key. """ root_items: list[Component] | list[Container] - root_level: str + root_level: ContainerHierarchy.Level if isinstance(object_key, LibraryUsageLocatorV2): root_items = [get_component_from_usage_key(object_key)] - root_level = "components" + root_level = ContainerHierarchy.Level.components elif isinstance(object_key, LibraryContainerLocator): root_items = [_get_container_from_key(object_key)] - root_level = f"{object_key.container_type}s" + root_level = ContainerHierarchy.Level[f"{object_key.container_type}s"] - else: + if not root_level: raise TypeError(f"Unexpected '{object_key}': must be LibraryUsageLocatorv2 or LibraryContainerLocator") # Fill in root level of hierarchy hierarchy = cls(object_key=object_key) - items = root_items - hierarchy.append(root_level, *items) - level: str | None = root_level + root_members = hierarchy.append(root_level, *root_items) # Fill in hierarchy up through parents - while level := hierarchy.parent_level(level): - items = list(_get_containers_with_entities(items).all()) - hierarchy.append(level, *items) + level = root_level + members = root_members + while level := level.parent: + items = list(_get_containers_with_entities(members).all()) + members = hierarchy.append(level, *items) # Fill in hierarchy down from root_level. - if root_level != 'components': # Components have no children + if root_level != cls.Level.components: # Components have no children level = root_level - children = getattr(hierarchy, level) - while level := hierarchy.child_level(level): - children = _get_containers_children(children) - hierarchy.append(level, *children) + members = root_members + while level := level.child: + children = _get_containers_children(level, members) + members = hierarchy.append(level, *children) return hierarchy +# TODO -- can these methods be moved inside ContainerHierarchy? def _get_containers_with_entities( - entities: list[Container] | list[Component], + members: list[ContainerHierarchyMember], *, ignore_pinned=False, ) -> QuerySet[Container]: @@ -769,43 +820,50 @@ def _get_containers_with_entities( ignore_pinned: if true, ignore any pinned references to the entity. """ qs = Container.objects.none() - for entity in entities: + for member in members: qs = qs.union(authoring_api.get_containers_with_entity( - entity.publishable_entity.pk, + member.entity.pk, ignore_pinned=ignore_pinned, )) return qs def _get_containers_children( - containers: list[ContainerMetadata], + level: ContainerHierarchy.Level, + members: list[ContainerHierarchyMember], *, published=False, -) -> list[LibraryXBlockMetadata | ContainerMetadata]: +) -> list[Component | Container]: """ - Find all components or containers directly contained by the given containers. + Find all components or containers directly contained by the given hierarchy members. Args: containers: iterable list or queryset of Containers of the same type. published: `True` if we want the published version of the children, or `False` for the draft version. """ - children: list[LibraryXBlockMetadata | ContainerMetadata] = [] - for container in containers: - children.extend( - get_container_children( - container.container_key, - published=published, - ) - ) + children: list[Component | Container] = [] + for member in members: + container = member.container + assert container + for entry in authoring_api.get_entities_in_container( + container, + published=published, + ): + match level: + case ContainerHierarchy.Level.components: + children.append(entry.entity_version.componentversion.component) + case _: + children.append(entry.entity_version.containerversion.container) return children +# /TODO -- can these methods be moved inside ContainerHierarchy? def get_library_object_hierarchy( object_key: LibraryUsageLocatorV2 | LibraryContainerLocator, -) -> LibraryObjectHierarchy: +) -> ContainerHierarchy: """ Returns the full ancestry and descendents of the library object with the given object_key. """ - return LibraryObjectHierarchy.create_from_library_object_key(object_key) + return ContainerHierarchy.create_from_library_object_key(object_key) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index 706998d307c8..b93b48e5ad86 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -364,7 +364,7 @@ class LibraryBlockHierarchy(GenericAPIView): """ View to return the full hierarchy of containers that contain a library block. """ - serializer_class = serializers.LibraryObjectHierarchySerializer + serializer_class = serializers.ContainerHierarchySerializer @convert_exceptions def get(self, request, usage_key_str) -> Response: diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 11fea123251b..c7acadf569cc 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -358,11 +358,11 @@ class LibraryContainerHierarchy(GenericAPIView): """ View to return the full hierarchy of containers that contain and are contained by a library container. """ - serializer_class = serializers.LibraryObjectHierarchySerializer + serializer_class = serializers.ContainerHierarchySerializer @convert_exceptions @swagger_auto_schema( - responses={200: serializers.LibraryObjectHierarchySerializer} + responses={200: serializers.ContainerHierarchySerializer} ) def get(self, request: RestRequest, container_key: LibraryContainerLocator) -> Response: """ diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index a719a7a8f982..d120c1b65c9f 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -388,13 +388,22 @@ class ContentLibraryItemCollectionsUpdateSerializer(serializers.Serializer): collection_keys = serializers.ListField(child=serializers.CharField(), allow_empty=True) -class LibraryObjectHierarchySerializer(serializers.Serializer): +class ContainerHierarchyMemberSerializer(serializers.Serializer): + """ + Serializer for the members of a hierarchy, which can be either Components or Containers. + """ + id = OpaqueKeySerializer() + display_name = serializers.CharField() + has_unpublished_changes = serializers.BooleanField() + + +class ContainerHierarchySerializer(serializers.Serializer): """ Serializer which represents the full hierarchy of containers and components that contain and are contained by a given library container or library block. """ - sections = serializers.ListField(child=LibraryContainerMetadataSerializer(), allow_empty=True) - subsections = serializers.ListField(child=LibraryContainerMetadataSerializer(), allow_empty=True) - units = serializers.ListField(child=LibraryContainerMetadataSerializer(), allow_empty=True) - components = serializers.ListField(child=LibraryXBlockMetadataSerializer(), allow_empty=True) + sections = serializers.ListField(child=ContainerHierarchyMemberSerializer(), allow_empty=True) + subsections = serializers.ListField(child=ContainerHierarchyMemberSerializer(), allow_empty=True) + units = serializers.ListField(child=ContainerHierarchyMemberSerializer(), allow_empty=True) + components = serializers.ListField(child=ContainerHierarchyMemberSerializer(), allow_empty=True) object_key = OpaqueKeySerializer() diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index b394d8ee3cca..1897c3efb801 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -467,6 +467,17 @@ def _publish_container(self, container_key: ContainerKey | str, expect_response= """ Publish all changes in the specified container + children """ return self._api('post', URL_LIB_CONTAINER_PUBLISH.format(container_key=container_key), None, expect_response) + @staticmethod + def _hierarchy_member(obj) -> dict: + """ + Returns the subset of metadata fields used by the container hierarchy. + """ + return { + "id": obj["id"], + "display_name": obj["display_name"], + "has_unpublished_changes": obj["has_unpublished_changes"], + } + def _get_block_hierarchy(self, block_key, expect_response=200): """ Returns the hierarchy of containers that contain the given block """ url = URL_LIB_BLOCK_HIERARCHY.format(block_key=block_key) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 9c5201c3df74..30c599ba994d 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -603,72 +603,72 @@ def test_unit_collections(self) -> None: assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] def test_section_hierarchy(self): - with self.assertNumQueries(225): + with self.assertNumQueries(117): hierarchy = self._get_container_hierarchy(self.section_with_subsections["id"]) assert hierarchy["object_key"] == self.section_with_subsections["id"] assert hierarchy["components"] == [ - self.problem_block, - self.html_block, - self.problem_block_2, - self.html_block_2, + self._hierarchy_member(self.problem_block), + self._hierarchy_member(self.html_block), + self._hierarchy_member(self.problem_block_2), + self._hierarchy_member(self.html_block_2), ] assert hierarchy["units"] == [ - self.unit, - self.unit_with_components, - self.unit_2, - self.unit_3, + self._hierarchy_member(self.unit), + self._hierarchy_member(self.unit_with_components), + self._hierarchy_member(self.unit_2), + self._hierarchy_member(self.unit_3), ] assert hierarchy["subsections"] == [ - self.subsection, - self.subsection_with_units, - self.subsection_2, - self.subsection_3, + self._hierarchy_member(self.subsection), + self._hierarchy_member(self.subsection_with_units), + self._hierarchy_member(self.subsection_2), + self._hierarchy_member(self.subsection_3), ] assert hierarchy["sections"] == [ - self.section_with_subsections, + self._hierarchy_member(self.section_with_subsections), ] def test_subsection_hierarchy(self): - with self.assertNumQueries(147): + with self.assertNumQueries(81): hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) assert hierarchy["object_key"] == self.subsection_with_units["id"] assert hierarchy["components"] == [ - self.problem_block, - self.html_block, - self.problem_block_2, - self.html_block_2, + self._hierarchy_member(self.problem_block), + self._hierarchy_member(self.html_block), + self._hierarchy_member(self.problem_block_2), + self._hierarchy_member(self.html_block_2), ] assert hierarchy["units"] == [ - self.unit, - self.unit_with_components, - self.unit_2, - self.unit_3, + self._hierarchy_member(self.unit), + self._hierarchy_member(self.unit_with_components), + self._hierarchy_member(self.unit_2), + self._hierarchy_member(self.unit_3), ] assert hierarchy["subsections"] == [ - self.subsection_with_units, + self._hierarchy_member(self.subsection_with_units), ] assert hierarchy["sections"] == [ - self.section_with_subsections, + self._hierarchy_member(self.section_with_subsections), ] def test_units_hierarchy(self): - with self.assertNumQueries(72): + with self.assertNumQueries(48): hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) assert hierarchy["object_key"] == self.unit_with_components["id"] assert hierarchy["components"] == [ - self.problem_block, - self.html_block, - self.problem_block_2, - self.html_block_2, + self._hierarchy_member(self.problem_block), + self._hierarchy_member(self.html_block), + self._hierarchy_member(self.problem_block_2), + self._hierarchy_member(self.html_block_2), ] assert hierarchy["units"] == [ - self.unit_with_components, + self._hierarchy_member(self.unit_with_components), ] assert hierarchy["subsections"] == [ - self.subsection_with_units, + self._hierarchy_member(self.subsection_with_units), ] assert hierarchy["sections"] == [ - self.section_with_subsections, + self._hierarchy_member(self.section_with_subsections), ] def test_container_hierarchy_not_found(self): @@ -678,20 +678,20 @@ def test_container_hierarchy_not_found(self): ) def test_block_hierarchy(self): - with self.assertNumQueries(29): + with self.assertNumQueries(21): hierarchy = self._get_block_hierarchy(self.problem_block["id"]) assert hierarchy["object_key"] == self.problem_block["id"] assert hierarchy["components"] == [ - self.problem_block, + self._hierarchy_member(self.problem_block), ] assert hierarchy["units"] == [ - self.unit_with_components, + self._hierarchy_member(self.unit_with_components), ] assert hierarchy["subsections"] == [ - self.subsection_with_units, + self._hierarchy_member(self.subsection_with_units), ] assert hierarchy["sections"] == [ - self.section_with_subsections, + self._hierarchy_member(self.section_with_subsections), ] def test_block_hierarchy_not_found(self): From 6c83d5ec39438286de865f648c5852fac89137af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Jul 2025 15:08:35 -0300 Subject: [PATCH 08/14] chore: trigger ci From 27d8418f5d69729262cee6079dd575d396636f4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Jul 2025 18:54:20 -0300 Subject: [PATCH 09/14] chore: update openedx-learning constraint --- requirements/constraints.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index bef19ed18bc3..e818e69e2d31 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -70,7 +70,7 @@ numpy<2.0.0 # Date: 2023-09-18 # pinning this version to avoid updates while the library is being developed # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35269 -openedx-learning==0.27.0 +openedx-learning==0.27.1 # Date: 2023-11-29 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. From df46ff6b1680e96d82ef97cbdab43aa50e3fe613 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 14 Aug 2025 17:14:25 -0300 Subject: [PATCH 10/14] chore: compile requirements --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 3 +-- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 326859e20085..65d8530fdc4d 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -849,7 +849,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.2 # via -r requirements/edx/kernel.in -openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head +openedx-learning==0.27.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 9a66c129668e..ce6061213449 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1407,7 +1407,7 @@ openedx-forum==0.3.2 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head +openedx-learning==0.27.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 8353f18f57b1..b362e383993a 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1025,7 +1025,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.2 # via -r requirements/edx/base.txt -openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head +openedx-learning==0.27.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index baf1e8aa5039..e1be13957206 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -120,8 +120,7 @@ openedx-django-require openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) openedx-forum # Open edX forum v2 application -# TODO -- https://github.com/openedx/openedx-learning/pull/333 -git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head#egg=openedx-learning +openedx-learning openedx-django-wiki path piexif # Exif image metadata manipulation, used in the profile_images app diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 3c220441181f..f065c3bf2e37 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1071,7 +1071,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.2 # via -r requirements/edx/base.txt -openedx-learning @ git+https://github.com/openedx/openedx-learning.git@refs/pull/333/head +openedx-learning==0.27.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From ef4fd077300afde31837a2dad38eb5496e222fe6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 14 Aug 2025 18:39:58 -0300 Subject: [PATCH 11/14] test: updating query count --- .../djangoapps/content_libraries/tests/test_containers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index da048a694bce..8b7b0c527381 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -604,7 +604,7 @@ def test_unit_collections(self) -> None: assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] def test_section_hierarchy(self): - with self.assertNumQueries(117): + with self.assertNumQueries(133): hierarchy = self._get_container_hierarchy(self.section_with_subsections["id"]) assert hierarchy["object_key"] == self.section_with_subsections["id"] assert hierarchy["components"] == [ @@ -630,7 +630,7 @@ def test_section_hierarchy(self): ] def test_subsection_hierarchy(self): - with self.assertNumQueries(81): + with self.assertNumQueries(93): hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) assert hierarchy["object_key"] == self.subsection_with_units["id"] assert hierarchy["components"] == [ @@ -653,7 +653,7 @@ def test_subsection_hierarchy(self): ] def test_units_hierarchy(self): - with self.assertNumQueries(48): + with self.assertNumQueries(56): hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) assert hierarchy["object_key"] == self.unit_with_components["id"] assert hierarchy["components"] == [ From e29c69e65a72a45c953dcc3992225d813170d18f Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Fri, 15 Aug 2025 12:41:06 -0500 Subject: [PATCH 12/14] style: Add missing comment in kernel.in --- requirements/edx/kernel.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index e1be13957206..f92b110a0685 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -120,7 +120,7 @@ openedx-django-require openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) openedx-forum # Open edX forum v2 application -openedx-learning +openedx-learning # Open edX Learning core (experimental) openedx-django-wiki path piexif # Exif image metadata manipulation, used in the profile_images app From 3e969afd1f22898174e9fad2e0b157c3cbe64c04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Aug 2025 21:13:07 -0300 Subject: [PATCH 13/14] fix: get_container_from_key param and comments --- .../djangoapps/content_libraries/api/container_metadata.py | 7 +++++-- .../core/djangoapps/content_libraries/api/containers.py | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index e5c9c89b20a9..c3c775c3624a 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -381,7 +381,7 @@ def library_container_locator( ) -def get_container_from_key(container_key: LibraryContainerLocator, isDeleted=False) -> Container: +def get_container_from_key(container_key: LibraryContainerLocator, include_deleted=False) -> Container: """ Internal method to fetch the Container object from its LibraryContainerLocator @@ -395,6 +395,9 @@ def get_container_from_key(container_key: LibraryContainerLocator, isDeleted=Fal learning_package.id, key=container_key.container_id, ) - if container and (isDeleted or container.versioning.draft): + # We only return the container if it exists and either: + # 1. the container has a draft version (which means it is not soft-deleted) OR + # 2. the container was soft-deleted but the `include_deleted` flag is set to True + if container and (include_deleted or container.versioning.draft): return container raise ContentLibraryContainerNotFound diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index d0dd078eb36f..57ce93cca722 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -334,7 +334,7 @@ def restore_container(container_key: LibraryContainerLocator) -> None: Restore the specified library container. """ library_key = container_key.lib_key - container = get_container_from_key(container_key, isDeleted=True) + container = get_container_from_key(container_key, include_deleted=True) affected_collections = authoring_api.get_entity_collections( container.publishable_entity.learning_package_id, From 1934a7da75b710a1f04eef519ef77fae321498b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Aug 2025 21:22:17 -0300 Subject: [PATCH 14/14] docs: mark api as UNSTABLE and add comment about get_library_object_hierarchy implementation --- .../api/container_metadata.py | 4 +++ .../content_libraries/api/containers.py | 32 +++++++++++-------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index c3c775c3624a..65779fafb42b 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -129,6 +129,10 @@ def from_container(cls, library_key, container: Container, associated_collection class ContainerHierarchy: """ Describes the full ancestry and descendents of a given library object. + + TODO: We intend to replace this implementation with a more efficient one that makes fewer + database queries in the future. More details being discussed in + https://github.com/openedx/edx-platform/pull/36813#issuecomment-3136631767 """ sections: list[ContainerHierarchyMember] = dataclass_field(default_factory=list) subsections: list[ContainerHierarchyMember] = dataclass_field(default_factory=list) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 57ce93cca722..28f05a05723a 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -45,7 +45,8 @@ from openedx.core.djangoapps.content_staging.api import UserClipboardData -# The public API is only the following symbols: +# 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured +# out our approach to dynamic content (randomized, A/B tests, etc.) __all__ = [ "get_container", "create_container", @@ -71,7 +72,7 @@ def get_container( include_collections=False, ) -> ContainerMetadata: """ - Get a container (a Section, Subsection, or Unit). + [ 🛑 UNSTABLE ] Get a container (a Section, Subsection, or Unit). """ container = get_container_from_key(container_key) if include_collections: @@ -99,7 +100,7 @@ def create_container( created: datetime | None = None, ) -> ContainerMetadata: """ - Create a container (a Section, Subsection, or Unit) in the specified content library. + [ 🛑 UNSTABLE ] Create a container (a Section, Subsection, or Unit) in the specified content library. It will initially be empty. """ @@ -168,7 +169,7 @@ def update_container( user_id: int | None, ) -> ContainerMetadata: """ - Update a container (a Section, Subsection, or Unit) title. + [ 🛑 UNSTABLE ] Update a container (a Section, Subsection, or Unit) title. """ container = get_container_from_key(container_key) library_key = container_key.lib_key @@ -256,7 +257,7 @@ def delete_container( container_key: LibraryContainerLocator, ) -> None: """ - Delete a container (a Section, Subsection, or Unit) (soft delete). + [ 🛑 UNSTABLE ] Delete a container (a Section, Subsection, or Unit) (soft delete). No-op if container doesn't exist or has already been soft-deleted. """ @@ -331,7 +332,7 @@ def delete_container( def restore_container(container_key: LibraryContainerLocator) -> None: """ - Restore the specified library container. + [ 🛑 UNSTABLE ] Restore the specified library container. """ library_key = container_key.lib_key container = get_container_from_key(container_key, include_deleted=True) @@ -421,7 +422,7 @@ def get_container_children( published=False, ) -> list[LibraryXBlockMetadata | ContainerMetadata]: """ - Get the entities contained in the given container + [ 🛑 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) """ container = get_container_from_key(container_key) @@ -459,7 +460,7 @@ def get_container_children_count( published=False, ) -> int: """ - Get the count of entities contained in the given container (e.g. the components/xblocks in a unit) + [ 🛑 UNSTABLE ] Get the count of entities contained in the given container (e.g. the components/xblocks in a unit) """ container = get_container_from_key(container_key) return authoring_api.get_container_children_count(container, published=published) @@ -472,7 +473,7 @@ def update_container_children( entities_action: authoring_api.ChildrenEntitiesAction = authoring_api.ChildrenEntitiesAction.REPLACE, ): """ - Adds children components or containers to given container. + [ 🛑 UNSTABLE ] Adds children components or containers to given container. """ library_key = container_key.lib_key container_type = ContainerType(container_key.container_type) @@ -555,8 +556,7 @@ def get_containers_contains_item( key: LibraryUsageLocatorV2 | LibraryContainerLocator ) -> list[ContainerMetadata]: """ - Get containers that contains the item, - that can be a component or another container. + [ 🛑 UNSTABLE ] Get containers that contains the item, that can be a component or another container. """ item: Component | Container @@ -577,7 +577,7 @@ def get_containers_contains_item( def publish_container_changes(container_key: LibraryContainerLocator, user_id: int | None) -> None: """ - Publish all unpublished changes in a container and all its child + [ 🛑 UNSTABLE ] Publish all unpublished changes in a container and all its child containers/blocks. """ container = get_container_from_key(container_key) @@ -600,7 +600,7 @@ def publish_container_changes(container_key: LibraryContainerLocator, user_id: i def copy_container(container_key: LibraryContainerLocator, user_id: int) -> UserClipboardData: """ - Copy a container (a Section, Subsection, or Unit) to the content staging. + [ 🛑 UNSTABLE ] Copy a container (a Section, Subsection, or Unit) to the content staging. """ container_metadata = get_container(container_key) container_serializer = ContainerSerializer(container_metadata) @@ -625,6 +625,10 @@ def get_library_object_hierarchy( object_key: LibraryUsageLocatorV2 | LibraryContainerLocator, ) -> ContainerHierarchy: """ - Returns the full ancestry and descendents of the library object with the given object_key. + [ 🛑 UNSTABLE ] Returns the full ancestry and descendents of the library object with the given object_key. + + TODO: We intend to replace this implementation with a more efficient one that makes fewer + database queries in the future. More details being discussed in + https://github.com/openedx/edx-platform/pull/36813#issuecomment-3136631767 """ return ContainerHierarchy.create_from_library_object_key(object_key)