From 8b9731dfa18a4b8b59f626e870f3866a9ba4f3e0 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 20 May 2025 20:40:36 -0500 Subject: [PATCH 01/13] refactor: Tests on test_containers.py to use setUp --- .../tests/test_containers.py | 248 +++++++++--------- 1 file changed, 120 insertions(+), 128 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 6c59c8c086e4..2d94cd4e4316 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -35,17 +35,42 @@ class ContainersTestCase(ContentLibrariesRestApiTest): break any tests, but backwards-incompatible API changes will. """ + def setUp(self): + super().setUp() + self.create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) + self.lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") + self.lib_key = LibraryLocatorV2.from_string(self.lib["id"]) + + # Create unit + with freeze_time(self.create_date): + self.unit = self._create_container(self.lib["id"], "unit", display_name="Alpha Bravo", slug=None) + self.unit_with_components = self._create_container(self.lib["id"], "unit", display_name="Alpha Charly", slug=None) + + # Create blocks + self.problem_block = self._add_block_to_library(self.lib["id"], "problem", "Problem1", can_stand_alone=False) + self.html_block = self._add_block_to_library(self.lib["id"], "html", "Html1", can_stand_alone=False) + self.problem_block_2 = self._add_block_to_library(self.lib["id"], "problem", "Problem2", can_stand_alone=False) + self.html_block_2 = self._add_block_to_library(self.lib["id"], "html", "Html2") + + # Add components to `unit_with_components` + self._add_container_components( + 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"], + ] + ) + def test_unit_crud(self): """ Test Create, Read, Update, and Delete of a Unit """ - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") - lib_key = LibraryLocatorV2.from_string(lib["id"]) - # Create a unit: create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) with freeze_time(create_date): - container_data = self._create_container(lib["id"], "unit", slug="u1", display_name="Test Unit") + container_data = self._create_container(self.lib["id"], "unit", slug="u1", display_name="Test Unit") expected_data = { "id": "lct:CL-TEST:containers:unit:u1", "container_type": "unit", @@ -88,20 +113,19 @@ def test_unit_permissions(self): """ Test that a regular user with read-only permissions on the library cannot create, update, or delete units. """ - lib = self._create_library(slug="containers2", title="Container Test Library 2", description="Unit permissions") - container_data = self._create_container(lib["id"], "unit", slug="u2", display_name="Test Unit") + container_data = self._create_container(self.lib["id"], "unit", slug="u2", display_name="Test Unit") random_user = UserFactory.create(username="Random", email="random@example.com") with self.as_user(random_user): - self._create_container(lib["id"], "unit", slug="u3", display_name="Test Unit", expect_response=403) + self._create_container(self.lib["id"], "unit", slug="u3", display_name="Test Unit", expect_response=403) self._get_container(container_data["id"], expect_response=403) self._update_container(container_data["id"], display_name="Unit ABC", expect_response=403) self._delete_container(container_data["id"], expect_response=403) # Granting read-only permissions on the library should only allow retrieval, nothing else. - self._add_user_by_email(lib["id"], random_user.email, access_level="read") + self._add_user_by_email(self.lib["id"], random_user.email, access_level="read") with self.as_user(random_user): - self._create_container(lib["id"], "unit", slug="u2", display_name="Test Unit", expect_response=403) + self._create_container(self.lib["id"], "unit", slug="u2", display_name="Test Unit", expect_response=403) self._get_container(container_data["id"], expect_response=200) self._update_container(container_data["id"], display_name="Unit ABC", expect_response=403) self._delete_container(container_data["id"], expect_response=403) @@ -111,45 +135,36 @@ def test_unit_gets_auto_slugs(self): Test that we can create units by specifying only a title, and they get unique slugs assigned automatically. """ - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") + unit_2 = self._create_container(self.lib["id"], "unit", display_name="Alpha Bravo", slug=None) - # Create two units, specifying their titles but not their slugs/keys: - container1_data = self._create_container(lib["id"], "unit", display_name="Alpha Bravo", slug=None) - container2_data = self._create_container(lib["id"], "unit", display_name="Alpha Bravo", slug=None) # Notice the container IDs below are slugified from the title: "alpha-bravo-NNNNN" - assert container1_data["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") - assert container2_data["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") - assert container1_data["id"] != container2_data["id"] + assert self.unit["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") + assert unit_2["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") + assert self.unit["id"] != unit_2["id"] def test_unit_add_children(self): """ Test that we can add and get unit children components """ - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") - lib_key = LibraryLocatorV2.from_string(lib["id"]) - # Create container and add some components - container_data = self._create_container(lib["id"], "unit", display_name="Alpha Bravo", slug=None) - problem_block = self._add_block_to_library(lib["id"], "problem", "Problem1", can_stand_alone=False) - html_block = self._add_block_to_library(lib["id"], "html", "Html1", can_stand_alone=False) self._add_container_components( - container_data["id"], - children_ids=[problem_block["id"], html_block["id"]] + self.unit["id"], + children_ids=[self.problem_block["id"], self.html_block["id"]] ) - data = self._get_container_components(container_data["id"]) + data = self._get_container_components(self.unit["id"]) assert len(data) == 2 - assert data[0]['id'] == problem_block['id'] + assert data[0]['id'] == self.problem_block['id'] assert not data[0]['can_stand_alone'] - assert data[1]['id'] == html_block['id'] + assert data[1]['id'] == self.html_block['id'] assert not data[1]['can_stand_alone'] - problem_block_2 = self._add_block_to_library(lib["id"], "problem", "Problem2", can_stand_alone=False) - html_block_2 = self._add_block_to_library(lib["id"], "html", "Html2") + problem_block_2 = self._add_block_to_library(self.lib["id"], "problem", "Problem_2", can_stand_alone=False) + html_block_2 = self._add_block_to_library(self.lib["id"], "html", "Html_2") # Add two more components self._add_container_components( - container_data["id"], + self.unit["id"], children_ids=[problem_block_2["id"], html_block_2["id"]] ) - data = self._get_container_components(container_data["id"]) + data = self._get_container_components(self.unit["id"]) # Verify total number of components to be 2 + 2 = 4 assert len(data) == 4 assert data[2]['id'] == problem_block_2['id'] @@ -161,75 +176,54 @@ def test_unit_remove_children(self): """ Test that we can remove unit children components """ - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") - lib_key = LibraryLocatorV2.from_string(lib["id"]) - - # Create container and add some components - container_data = self._create_container(lib["id"], "unit", display_name="Alpha Bravo", slug=None) - problem_block = self._add_block_to_library(lib["id"], "problem", "Problem1", can_stand_alone=False) - html_block = self._add_block_to_library(lib["id"], "html", "Html1", can_stand_alone=False) - problem_block_2 = self._add_block_to_library(lib["id"], "problem", "Problem2", can_stand_alone=False) - html_block_2 = self._add_block_to_library(lib["id"], "html", "Html2") - self._add_container_components( - container_data["id"], - children_ids=[problem_block["id"], html_block["id"], problem_block_2["id"], html_block_2["id"]] - ) - data = self._get_container_components(container_data["id"]) + data = self._get_container_components(self.unit_with_components["id"]) assert len(data) == 4 # Remove both problem blocks. self._remove_container_components( - container_data["id"], - children_ids=[problem_block_2["id"], problem_block["id"]] + self.unit_with_components["id"], + children_ids=[self.problem_block_2["id"], self.problem_block["id"]] ) - data = self._get_container_components(container_data["id"]) + data = self._get_container_components(self.unit_with_components["id"]) assert len(data) == 2 - assert data[0]['id'] == html_block['id'] - assert data[1]['id'] == html_block_2['id'] + assert data[0]['id'] == self.html_block['id'] + assert data[1]['id'] == self.html_block_2['id'] def test_unit_replace_children(self): """ Test that we can completely replace/reorder unit children components. """ - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") - lib_key = LibraryLocatorV2.from_string(lib["id"]) - - # Create container and add some components - container_data = self._create_container(lib["id"], "unit", display_name="Alpha Bravo", slug=None) - problem_block = self._add_block_to_library(lib["id"], "problem", "Problem1", can_stand_alone=False) - html_block = self._add_block_to_library(lib["id"], "html", "Html1", can_stand_alone=False) - problem_block_2 = self._add_block_to_library(lib["id"], "problem", "Problem2", can_stand_alone=False) - html_block_2 = self._add_block_to_library(lib["id"], "html", "Html2") - self._add_container_components( - container_data["id"], - children_ids=[problem_block["id"], html_block["id"], problem_block_2["id"], html_block_2["id"]] - ) - data = self._get_container_components(container_data["id"]) + data = self._get_container_components(self.unit_with_components["id"]) assert len(data) == 4 - assert data[0]['id'] == problem_block['id'] - assert data[1]['id'] == html_block['id'] - assert data[2]['id'] == problem_block_2['id'] - assert data[3]['id'] == html_block_2['id'] + assert data[0]['id'] == self.problem_block['id'] + assert data[1]['id'] == self.html_block['id'] + assert data[2]['id'] == self.problem_block_2['id'] + assert data[3]['id'] == self.html_block_2['id'] # Reorder the components self._patch_container_components( - container_data["id"], - children_ids=[problem_block["id"], problem_block_2["id"], html_block["id"], html_block_2["id"]] + self.unit_with_components["id"], + children_ids=[ + self.problem_block["id"], + self.problem_block_2["id"], + self.html_block["id"], + self.html_block_2["id"], + ] ) - data = self._get_container_components(container_data["id"]) + data = self._get_container_components(self.unit_with_components["id"]) assert len(data) == 4 - assert data[0]['id'] == problem_block['id'] - assert data[1]['id'] == problem_block_2['id'] - assert data[2]['id'] == html_block['id'] - assert data[3]['id'] == html_block_2['id'] + assert data[0]['id'] == self.problem_block['id'] + assert data[1]['id'] == self.problem_block_2['id'] + assert data[2]['id'] == self.html_block['id'] + assert data[3]['id'] == self.html_block_2['id'] # Replace with new components - new_problem_block = self._add_block_to_library(lib["id"], "problem", "New_Problem", can_stand_alone=False) - new_html_block = self._add_block_to_library(lib["id"], "html", "New_Html", can_stand_alone=False) + 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( - container_data["id"], + self.unit_with_components["id"], children_ids=[new_problem_block["id"], new_html_block["id"]], ) - data = self._get_container_components(container_data["id"]) + data = self._get_container_components(self.unit_with_components["id"]) assert len(data) == 2 assert data[0]['id'] == new_problem_block['id'] assert data[1]['id'] == new_html_block['id'] @@ -238,24 +232,16 @@ def test_restore_unit(self): """ Test restore a deleted unit. """ - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") - lib_key = LibraryLocatorV2.from_string(lib["id"]) - - # Create a unit: - create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) - with freeze_time(create_date): - container_data = self._create_container(lib["id"], "unit", slug="u1", display_name="Test Unit") - # Delete the unit - self._delete_container(container_data["id"]) + self._delete_container(self.unit["id"]) # Restore container - self._restore_container(container_data["id"]) - new_container_data = self._get_container(container_data["id"]) + self._restore_container(self.unit["id"]) + new_container_data = self._get_container(self.unit["id"]) expected_data = { - "id": "lct:CL-TEST:containers:unit:u1", + "id": self.unit["id"], "container_type": "unit", - "display_name": "Test Unit", + "display_name": "Alpha Bravo", "last_published": None, "published_by": "", "last_draft_created": "2024-09-08T07:06:05Z", @@ -266,17 +252,12 @@ def test_restore_unit(self): 'collections': [], } - def test_container_collections(self): - # Create a library - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") - lib_key = LibraryLocatorV2.from_string(lib["id"]) - - # Create a unit - container_data = self._create_container(lib["id"], "unit", display_name="Alpha Bravo", slug=None) + self.assertDictContainsEntries(new_container_data, expected_data) + def test_container_collections(self): # Create a collection col1 = api.create_library_collection( - lib_key, + self.lib_key, "COL1", title="Collection 1", created_by=self.user.id, @@ -284,14 +265,14 @@ def test_container_collections(self): ) result = self._patch_container_collections( - container_data["id"], + self.unit["id"], collection_keys=[col1.key], ) assert result['count'] == 1 # Fetch the unit - unit_as_read = self._get_container(container_data["id"]) + unit_as_read = self._get_container(self.unit["id"]) # Verify the collections assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] @@ -300,53 +281,64 @@ def test_publish_container(self): # pylint: disable=too-many-statements """ Test that we can publish the changes to a specific container """ - lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") - - # Create two containers and add some components - container1 = self._create_container(lib["id"], "unit", display_name="Alpha Unit", slug=None) - container2 = self._create_container(lib["id"], "unit", display_name="Bravo Unit", slug=None) - problem_block = self._add_block_to_library(lib["id"], "problem", "Problem1", can_stand_alone=False) - html_block = self._add_block_to_library(lib["id"], "html", "Html1", can_stand_alone=False) - html_block2 = self._add_block_to_library(lib["id"], "html", "Html2", can_stand_alone=False) - self._add_container_components(container1["id"], children_ids=[problem_block["id"], html_block["id"]]) - self._add_container_components(container2["id"], children_ids=[html_block["id"], html_block2["id"]]) + html_block_3 = self._add_block_to_library(self.lib["id"], "html", "Html3") + self._add_container_components( + self.unit["id"], + children_ids=[ + self.html_block["id"], + html_block_3["id"], + ] + ) + # At first everything is unpublished: - c1_before = self._get_container(container1["id"]) + c1_before = self._get_container(self.unit_with_components["id"]) assert c1_before["has_unpublished_changes"] - c1_components_before = self._get_container_components(container1["id"]) - assert len(c1_components_before) == 2 - assert c1_components_before[0]["id"] == problem_block["id"] + c1_components_before = self._get_container_components(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"] == html_block["id"] + 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 - c2_before = self._get_container(container2["id"]) + 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._publish_container(container1["id"]) + self._publish_container(self.unit_with_components["id"]) # Now it is published: - c1_after = self._get_container(container1["id"]) + c1_after = self._get_container(self.unit_with_components["id"]) assert c1_after["has_unpublished_changes"] is False - c1_components_after = self._get_container_components(container1["id"]) - assert len(c1_components_after) == 2 - assert c1_components_after[0]["id"] == problem_block["id"] + c1_components_after = self._get_container_components(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"] == html_block["id"] + 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: - c2_after = self._get_container(container2["id"]) + c2_after = self._get_container(self.unit["id"]) assert c2_after["has_unpublished_changes"] - c2_components_after = self._get_container_components(container2["id"]) + c2_components_after = self._get_container_components(self.unit["id"]) assert len(c2_components_after) == 2 - assert c2_components_after[0]["id"] == html_block["id"] + 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]["published_by"] == self.user.username - assert c2_components_after[1]["id"] == html_block2["id"] + 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 From 3f6c61611e05ea89154a504d3dced329b9853615 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 20 May 2025 21:00:14 -0500 Subject: [PATCH 02/13] feat: CRUD for section/subsections with tests --- .../content_libraries/api/containers.py | 83 ++++++++++++---- .../content_libraries/rest_api/containers.py | 2 +- .../tests/test_containers.py | 99 ++++++++++++++++++- 3 files changed, 165 insertions(+), 19 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 01974a441ed2..17019073b7f8 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -25,7 +25,7 @@ LIBRARY_CONTAINER_UPDATED, ) from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import Container +from openedx_learning.api.authoring_models import Container, ContainerVersion from openedx.core.djangoapps.content_libraries.api.collections import library_collection_locator from openedx.core.djangoapps.xblock.api import get_component_from_usage_key @@ -159,11 +159,15 @@ def library_container_locator( ) -> LibraryContainerLocator: """ Returns a LibraryContainerLocator for the given library + container. - - Currently only supports Unit-type containers; will support other container types in future. """ - assert container.unit is not None - container_type = ContainerType.Unit + if hasattr(container, 'unit'): + container_type = ContainerType.Unit + elif hasattr(container, 'subsection'): + container_type = ContainerType.Subsection + elif hasattr(container, 'section'): + container_type = ContainerType.Section + + assert container_type is not None return LibraryContainerLocator( library_key, @@ -225,7 +229,7 @@ def create_container( created: datetime | None = None, ) -> ContainerMetadata: """ - Create a container (e.g. a Unit) in the specified content library. + Create a container (a Section, Subsection, or Unit) in the specified content library. It will initially be empty. """ @@ -241,6 +245,13 @@ def create_container( container_type=container_type.value, container_id=slug, ) + + if not created: + created = datetime.now(tz=timezone.utc) + + container: Container + _initial_version: ContainerVersion + # Then try creating the actual container: match container_type: case ContainerType.Unit: @@ -248,7 +259,23 @@ def create_container( content_library.learning_package_id, key=slug, title=title, - created=created or datetime.now(tz=timezone.utc), + created=created, + created_by=user_id, + ) + case ContainerType.Subsection: + container, _initial_version = authoring_api.create_subsection_and_version( + content_library.learning_package_id, + key=slug, + title=title, + created=created, + created_by=user_id, + ) + case ContainerType.Section: + container, _initial_version = authoring_api.create_section_and_version( + content_library.learning_package_id, + key=slug, + title=title, + created=created, created_by=user_id, ) case _: @@ -269,18 +296,40 @@ def update_container( user_id: int | None, ) -> ContainerMetadata: """ - Update a container (e.g. a Unit) title. + Update a container (a Section, Subsection, or Unit) title. """ container = _get_container_from_key(container_key) library_key = container_key.lib_key + created = datetime.now(tz=timezone.utc) - assert container.unit - unit_version = authoring_api.create_next_unit_version( - container.unit, - title=display_name, - created=datetime.now(tz=timezone.utc), - created_by=user_id, - ) + container_type = ContainerType(container_key.container_type) + + version: ContainerVersion + + match container_type: + case ContainerType.Unit: + version = authoring_api.create_next_unit_version( + container.unit, + title=display_name, + created=created, + created_by=user_id, + ) + case ContainerType.Subsection: + version = authoring_api.create_next_subsection_version( + container.subsection, + title=display_name, + created=created, + created_by=user_id, + ) + case ContainerType.Section: + version = authoring_api.create_next_section_version( + container.section, + title=display_name, + created=created, + created_by=user_id, + ) + case _: + raise NotImplementedError(f"Library support for {container_type} is in progress") LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( @@ -288,14 +337,14 @@ def update_container( ) ) - return ContainerMetadata.from_container(library_key, unit_version.container) + return ContainerMetadata.from_container(library_key, version.container) def delete_container( container_key: LibraryContainerLocator, ) -> None: """ - Delete a container (e.g. a Unit) (soft delete). + Delete a container (a Section, Subsection, or Unit) (soft delete). No-op if container doesn't exist or has already been soft-deleted. """ diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 06baa6fc4254..b0c8a21d0448 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -64,7 +64,7 @@ def post(self, request, lib_key_str): @view_auth_classes() class LibraryContainerView(GenericAPIView): """ - View to retrieve or update data about a specific container (a section, subsection, or unit) + View to retrieve, delete or update data about a specific container (a section, subsection, or unit) """ serializer_class = serializers.LibraryContainerMetadataSerializer diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 2d94cd4e4316..03d1d6740869 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -44,7 +44,12 @@ def setUp(self): # Create unit with freeze_time(self.create_date): self.unit = self._create_container(self.lib["id"], "unit", display_name="Alpha Bravo", slug=None) - self.unit_with_components = self._create_container(self.lib["id"], "unit", display_name="Alpha Charly", slug=None) + self.unit_with_components = self._create_container( + self.lib["id"], + "unit", + display_name="Alpha Charly", + slug=None, + ) # Create blocks self.problem_block = self._add_block_to_library(self.lib["id"], "problem", "Problem1", can_stand_alone=False) @@ -109,6 +114,98 @@ def test_unit_crud(self): self._delete_container(container_data["id"]) self._get_container(container_data["id"], expect_response=404) + def test_subsection_crud(self): + # Create a subsection: + with freeze_time(self.create_date): + container_data = self._create_container( + self.lib["id"], + "subsection", + slug="subs1", + display_name="Test Subsection", + ) + expected_data = { + "id": "lct:CL-TEST:containers:subsection:subs1", + "container_type": "subsection", + "display_name": "Test Subsection", + "last_published": None, + "published_by": "", + "last_draft_created": "2024-09-08T07:06:05Z", + "last_draft_created_by": 'Bob', + 'has_unpublished_changes': True, + 'created': '2024-09-08T07:06:05Z', + 'modified': '2024-09-08T07:06:05Z', + 'collections': [], + } + + self.assertDictContainsEntries(container_data, expected_data) + + # Fetch the subsection: + subsection_as_read = self._get_container(container_data["id"]) + # make sure it contains the same data when we read it back: + self.assertDictContainsEntries(subsection_as_read, expected_data) + + # Update the subsection: + modified_date = datetime(2024, 10, 9, 8, 7, 6, tzinfo=timezone.utc) + with freeze_time(modified_date): + container_data = self._update_container( + "lct:CL-TEST:containers:subsection:subs1", + display_name="Subsection ABC", + ) + expected_data['last_draft_created'] = expected_data['modified'] = '2024-10-09T08:07:06Z' + expected_data['display_name'] = 'Subsection ABC' + self.assertDictContainsEntries(container_data, expected_data) + + # Re-fetch the subsection + subsection_as_re_read = self._get_container(container_data["id"]) + # make sure it contains the same data when we read it back: + self.assertDictContainsEntries(subsection_as_re_read, expected_data) + + # Delete the subsection + self._delete_container(container_data["id"]) + self._get_container(container_data["id"], expect_response=404) + + def test_section_crud(self): + # Create a section: + with freeze_time(self.create_date): + container_data = self._create_container(self.lib["id"], "section", slug="s1", display_name="Test Section") + expected_data = { + "id": "lct:CL-TEST:containers:section:s1", + "container_type": "section", + "display_name": "Test Section", + "last_published": None, + "published_by": "", + "last_draft_created": "2024-09-08T07:06:05Z", + "last_draft_created_by": 'Bob', + 'has_unpublished_changes': True, + 'created': '2024-09-08T07:06:05Z', + 'modified': '2024-09-08T07:06:05Z', + 'collections': [], + } + + self.assertDictContainsEntries(container_data, expected_data) + + # Fetch the section: + section_as_read = self._get_container(container_data["id"]) + # make sure it contains the same data when we read it back: + self.assertDictContainsEntries(section_as_read, expected_data) + + # Update the section: + modified_date = datetime(2024, 10, 9, 8, 7, 6, tzinfo=timezone.utc) + with freeze_time(modified_date): + container_data = self._update_container("lct:CL-TEST:containers:section:s1", display_name="Section ABC") + expected_data['last_draft_created'] = expected_data['modified'] = '2024-10-09T08:07:06Z' + expected_data['display_name'] = 'Section ABC' + self.assertDictContainsEntries(container_data, expected_data) + + # Re-fetch the section + section_as_re_read = self._get_container(container_data["id"]) + # make sure it contains the same data when we read it back: + self.assertDictContainsEntries(section_as_re_read, expected_data) + + # Delete the section + self._delete_container(container_data["id"]) + self._get_container(container_data["id"], expect_response=404) + def test_unit_permissions(self): """ Test that a regular user with read-only permissions on the library cannot create, update, or delete units. From c9300cf1af23b75dc8bb11d050573a97ff43e3a6 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 21 May 2025 20:36:52 -0500 Subject: [PATCH 03/13] test: test_containers updated * Created `test_subsection_permissions` * Created `test_section_permissions` * Rename `test_units_gets_auto_slugs` to `test_containers_gets_auto_slugs` and also test sections and subsections --- .../tests/test_containers.py | 68 +++++++++++++++++-- 1 file changed, 64 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 03d1d6740869..4188ed523f90 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -41,8 +41,9 @@ def setUp(self): self.lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") self.lib_key = LibraryLocatorV2.from_string(self.lib["id"]) - # Create unit + # Create containers with freeze_time(self.create_date): + # Unit self.unit = self._create_container(self.lib["id"], "unit", display_name="Alpha Bravo", slug=None) self.unit_with_components = self._create_container( self.lib["id"], @@ -50,6 +51,10 @@ def setUp(self): display_name="Alpha Charly", slug=None, ) + # Subsection + self.subsection = self._create_container(self.lib["id"], "subsection", display_name="Subsection Alpha", slug=None) + # Section + self.section = self._create_container(self.lib["id"], "section", display_name="Section Alpha", slug=None) # Create blocks self.problem_block = self._add_block_to_library(self.lib["id"], "problem", "Problem1", can_stand_alone=False) @@ -115,6 +120,9 @@ def test_unit_crud(self): self._get_container(container_data["id"], expect_response=404) def test_subsection_crud(self): + """ + Test Create, Read, Update, and Delete of a Subsection + """ # Create a subsection: with freeze_time(self.create_date): container_data = self._create_container( @@ -165,6 +173,9 @@ def test_subsection_crud(self): self._get_container(container_data["id"], expect_response=404) def test_section_crud(self): + """ + Test Create, Read, Update, and Delete of a Section + """ # Create a section: with freeze_time(self.create_date): container_data = self._create_container(self.lib["id"], "section", slug="s1", display_name="Test Section") @@ -227,18 +238,67 @@ def test_unit_permissions(self): self._update_container(container_data["id"], display_name="Unit ABC", expect_response=403) self._delete_container(container_data["id"], expect_response=403) - def test_unit_gets_auto_slugs(self): + def test_subsection_permissions(self): + """ + Test that a regular user with read-only permissions on the library cannot create, update, or delete subsection. """ - Test that we can create units by specifying only a title, and they get + container_data = self._create_container(self.lib["id"], "subsection", slug="subs2", display_name="Test Subsection") + + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + self._create_container(self.lib["id"], "unit", slug="subs3", display_name="Test Subsection", expect_response=403) + self._get_container(container_data["id"], expect_response=403) + self._update_container(container_data["id"], display_name="Subsection ABC", expect_response=403) + self._delete_container(container_data["id"], expect_response=403) + + # Granting read-only permissions on the library should only allow retrieval, nothing else. + self._add_user_by_email(self.lib["id"], random_user.email, access_level="read") + with self.as_user(random_user): + self._create_container(self.lib["id"], "unit", slug="subs2", display_name="Test Subsection", expect_response=403) + self._get_container(container_data["id"], expect_response=200) + self._update_container(container_data["id"], display_name="Subsection ABC", expect_response=403) + self._delete_container(container_data["id"], expect_response=403) + + def test_section_permissions(self): + container_data = self._create_container(self.lib["id"], "section", slug="s2", display_name="Test Section") + + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + self._create_container(self.lib["id"], "section", slug="s3", display_name="Test Section", expect_response=403) + self._get_container(container_data["id"], expect_response=403) + self._update_container(container_data["id"], display_name="Section ABC", expect_response=403) + self._delete_container(container_data["id"], expect_response=403) + + # Granting read-only permissions on the library should only allow retrieval, nothing else. + self._add_user_by_email(self.lib["id"], random_user.email, access_level="read") + with self.as_user(random_user): + self._create_container(self.lib["id"], "section", slug="s2", display_name="Test Section", expect_response=403) + self._get_container(container_data["id"], expect_response=200) + self._update_container(container_data["id"], display_name="Section ABC", expect_response=403) + self._delete_container(container_data["id"], expect_response=403) + + def test_containers_gets_auto_slugs(self): + """ + Test that we can create containers by specifying only a title, and they get unique slugs assigned automatically. """ unit_2 = self._create_container(self.lib["id"], "unit", display_name="Alpha Bravo", slug=None) + subsection_2 = self._create_container(self.lib["id"], "subsection", display_name="Subsection Alpha", slug=None) + section_2 = self._create_container(self.lib["id"], "section", display_name="Section Alpha", slug=None) - # Notice the container IDs below are slugified from the title: "alpha-bravo-NNNNN" assert self.unit["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") assert unit_2["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") assert self.unit["id"] != unit_2["id"] + assert self.subsection["id"].startswith("lct:CL-TEST:containers:subsection:subsection-alpha-") + assert subsection_2["id"].startswith("lct:CL-TEST:containers:subsection:subsection-alpha-") + assert self.subsection["id"] != subsection_2["id"] + + assert self.section["id"].startswith("lct:CL-TEST:containers:section:section-alpha-") + assert section_2["id"].startswith("lct:CL-TEST:containers:section:section-alpha-") + assert self.section["id"] != section_2["id"] + + def test_unit_add_children(self): """ Test that we can add and get unit children components From d921f53560477d48a6daa3429a8bb135b522ac24 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 21 May 2025 21:10:00 -0500 Subject: [PATCH 04/13] test: Updates and refactors in test_containers * feat: `test_restore_units` renamed to `test_restore_containers` to also test restore section and subsections * refactor: Created `test_container_crud` to test all container types using ddt. * refactor: Created `test_container_permissions` to test all container types using ddt. * refactor: Created `test_containers_gets_auto_slugs` to test all container types using ddt --- .../tests/test_containers.py | 254 +++++------------- 1 file changed, 69 insertions(+), 185 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 4188ed523f90..c7a9ecbeaf24 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -73,116 +73,30 @@ def setUp(self): ] ) - def test_unit_crud(self): + @ddt.data( + ("unit", "u1", "Test Unit"), + ("subsection", "subs1", "Test Subsection"), + ("section", "s1", "Test Section"), + ) + @ddt.unpack + def test_container_crud(self, container_type, slug, display_name): """ - Test Create, Read, Update, and Delete of a Unit + Test Create, Read, Update, and Delete of a Containers """ - # Create a unit: + # Create container: create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) with freeze_time(create_date): - container_data = self._create_container(self.lib["id"], "unit", slug="u1", display_name="Test Unit") - expected_data = { - "id": "lct:CL-TEST:containers:unit:u1", - "container_type": "unit", - "display_name": "Test Unit", - "last_published": None, - "published_by": "", - "last_draft_created": "2024-09-08T07:06:05Z", - "last_draft_created_by": 'Bob', - 'has_unpublished_changes': True, - 'created': '2024-09-08T07:06:05Z', - 'modified': '2024-09-08T07:06:05Z', - 'collections': [], - } - - self.assertDictContainsEntries(container_data, expected_data) - - # Fetch the unit: - unit_as_read = self._get_container(container_data["id"]) - # make sure it contains the same data when we read it back: - self.assertDictContainsEntries(unit_as_read, expected_data) - - # Update the unit: - modified_date = datetime(2024, 10, 9, 8, 7, 6, tzinfo=timezone.utc) - with freeze_time(modified_date): - container_data = self._update_container("lct:CL-TEST:containers:unit:u1", display_name="Unit ABC") - expected_data['last_draft_created'] = expected_data['modified'] = '2024-10-09T08:07:06Z' - expected_data['display_name'] = 'Unit ABC' - self.assertDictContainsEntries(container_data, expected_data) - - # Re-fetch the unit - unit_as_re_read = self._get_container(container_data["id"]) - # make sure it contains the same data when we read it back: - self.assertDictContainsEntries(unit_as_re_read, expected_data) - - # Delete the unit - self._delete_container(container_data["id"]) - self._get_container(container_data["id"], expect_response=404) - - def test_subsection_crud(self): - """ - Test Create, Read, Update, and Delete of a Subsection - """ - # Create a subsection: - with freeze_time(self.create_date): container_data = self._create_container( self.lib["id"], - "subsection", - slug="subs1", - display_name="Test Subsection", - ) - expected_data = { - "id": "lct:CL-TEST:containers:subsection:subs1", - "container_type": "subsection", - "display_name": "Test Subsection", - "last_published": None, - "published_by": "", - "last_draft_created": "2024-09-08T07:06:05Z", - "last_draft_created_by": 'Bob', - 'has_unpublished_changes': True, - 'created': '2024-09-08T07:06:05Z', - 'modified': '2024-09-08T07:06:05Z', - 'collections': [], - } - - self.assertDictContainsEntries(container_data, expected_data) - - # Fetch the subsection: - subsection_as_read = self._get_container(container_data["id"]) - # make sure it contains the same data when we read it back: - self.assertDictContainsEntries(subsection_as_read, expected_data) - - # Update the subsection: - modified_date = datetime(2024, 10, 9, 8, 7, 6, tzinfo=timezone.utc) - with freeze_time(modified_date): - container_data = self._update_container( - "lct:CL-TEST:containers:subsection:subs1", - display_name="Subsection ABC", - ) - expected_data['last_draft_created'] = expected_data['modified'] = '2024-10-09T08:07:06Z' - expected_data['display_name'] = 'Subsection ABC' - self.assertDictContainsEntries(container_data, expected_data) - - # Re-fetch the subsection - subsection_as_re_read = self._get_container(container_data["id"]) - # make sure it contains the same data when we read it back: - self.assertDictContainsEntries(subsection_as_re_read, expected_data) - - # Delete the subsection - self._delete_container(container_data["id"]) - self._get_container(container_data["id"], expect_response=404) - - def test_section_crud(self): - """ - Test Create, Read, Update, and Delete of a Section - """ - # Create a section: - with freeze_time(self.create_date): - container_data = self._create_container(self.lib["id"], "section", slug="s1", display_name="Test Section") + container_type, + slug=slug, + display_name=display_name + ) + id = f"lct:CL-TEST:containers:{container_type}:{slug}" expected_data = { - "id": "lct:CL-TEST:containers:section:s1", - "container_type": "section", - "display_name": "Test Section", + "id": id, + "container_type": container_type, + "display_name": display_name, "last_published": None, "published_by": "", "last_draft_created": "2024-09-08T07:06:05Z", @@ -195,109 +109,72 @@ def test_section_crud(self): self.assertDictContainsEntries(container_data, expected_data) - # Fetch the section: - section_as_read = self._get_container(container_data["id"]) + # Fetch the container: + container_as_read = self._get_container(container_data["id"]) # make sure it contains the same data when we read it back: - self.assertDictContainsEntries(section_as_read, expected_data) + self.assertDictContainsEntries(container_as_read, expected_data) - # Update the section: + # Update the container: modified_date = datetime(2024, 10, 9, 8, 7, 6, tzinfo=timezone.utc) with freeze_time(modified_date): - container_data = self._update_container("lct:CL-TEST:containers:section:s1", display_name="Section ABC") - expected_data['last_draft_created'] = expected_data['modified'] = '2024-10-09T08:07:06Z' - expected_data['display_name'] = 'Section ABC' + container_data = self._update_container(id, display_name=f"New Display Name for {container_type}") + expected_data["last_draft_created"] = expected_data["modified"] = "2024-10-09T08:07:06Z" + expected_data["display_name"] = f"New Display Name for {container_type}" self.assertDictContainsEntries(container_data, expected_data) - # Re-fetch the section - section_as_re_read = self._get_container(container_data["id"]) + # Re-fetch the container + container_as_re_read = self._get_container(container_data["id"]) # make sure it contains the same data when we read it back: - self.assertDictContainsEntries(section_as_re_read, expected_data) + self.assertDictContainsEntries(container_as_re_read, expected_data) - # Delete the section + # Delete the container self._delete_container(container_data["id"]) self._get_container(container_data["id"], expect_response=404) - def test_unit_permissions(self): + @ddt.data( + ("unit", "u2", "Test Unit"), + ("subsection", "subs2", "Test Subsection"), + ("section", "s2", "Test Section"), + ) + @ddt.unpack + def test_container_permissions(self, container_type, slug, display_name): """ - Test that a regular user with read-only permissions on the library cannot create, update, or delete units. + Test that a regular user with read-only permissions on the library cannot create, update, or delete containers. """ - container_data = self._create_container(self.lib["id"], "unit", slug="u2", display_name="Test Unit") + container_data = self._create_container(self.lib["id"], container_type, slug=slug, display_name=display_name) random_user = UserFactory.create(username="Random", email="random@example.com") with self.as_user(random_user): - self._create_container(self.lib["id"], "unit", slug="u3", display_name="Test Unit", expect_response=403) + self._create_container(self.lib["id"], container_type, slug="new_slug", display_name=display_name, expect_response=403) self._get_container(container_data["id"], expect_response=403) - self._update_container(container_data["id"], display_name="Unit ABC", expect_response=403) + self._update_container(container_data["id"], display_name="New Display Name", expect_response=403) self._delete_container(container_data["id"], expect_response=403) # Granting read-only permissions on the library should only allow retrieval, nothing else. self._add_user_by_email(self.lib["id"], random_user.email, access_level="read") with self.as_user(random_user): - self._create_container(self.lib["id"], "unit", slug="u2", display_name="Test Unit", expect_response=403) + self._create_container(self.lib["id"], container_type, slug=slug, display_name=display_name, expect_response=403) self._get_container(container_data["id"], expect_response=200) - self._update_container(container_data["id"], display_name="Unit ABC", expect_response=403) + self._update_container(container_data["id"], display_name="New Display Name", expect_response=403) self._delete_container(container_data["id"], expect_response=403) - def test_subsection_permissions(self): - """ - Test that a regular user with read-only permissions on the library cannot create, update, or delete subsection. - """ - container_data = self._create_container(self.lib["id"], "subsection", slug="subs2", display_name="Test Subsection") - - random_user = UserFactory.create(username="Random", email="random@example.com") - with self.as_user(random_user): - self._create_container(self.lib["id"], "unit", slug="subs3", display_name="Test Subsection", expect_response=403) - self._get_container(container_data["id"], expect_response=403) - self._update_container(container_data["id"], display_name="Subsection ABC", expect_response=403) - self._delete_container(container_data["id"], expect_response=403) - - # Granting read-only permissions on the library should only allow retrieval, nothing else. - self._add_user_by_email(self.lib["id"], random_user.email, access_level="read") - with self.as_user(random_user): - self._create_container(self.lib["id"], "unit", slug="subs2", display_name="Test Subsection", expect_response=403) - self._get_container(container_data["id"], expect_response=200) - self._update_container(container_data["id"], display_name="Subsection ABC", expect_response=403) - self._delete_container(container_data["id"], expect_response=403) - - def test_section_permissions(self): - container_data = self._create_container(self.lib["id"], "section", slug="s2", display_name="Test Section") - - random_user = UserFactory.create(username="Random", email="random@example.com") - with self.as_user(random_user): - self._create_container(self.lib["id"], "section", slug="s3", display_name="Test Section", expect_response=403) - self._get_container(container_data["id"], expect_response=403) - self._update_container(container_data["id"], display_name="Section ABC", expect_response=403) - self._delete_container(container_data["id"], expect_response=403) - - # Granting read-only permissions on the library should only allow retrieval, nothing else. - self._add_user_by_email(self.lib["id"], random_user.email, access_level="read") - with self.as_user(random_user): - self._create_container(self.lib["id"], "section", slug="s2", display_name="Test Section", expect_response=403) - self._get_container(container_data["id"], expect_response=200) - self._update_container(container_data["id"], display_name="Section ABC", expect_response=403) - self._delete_container(container_data["id"], expect_response=403) - - def test_containers_gets_auto_slugs(self): + @ddt.data( + ("unit", "Alpha Bravo", "lct:CL-TEST:containers:unit:alpha-bravo-"), + ("subsection", "Subsection Alpha", "lct:CL-TEST:containers:subsection:subsection-alpha-"), + ("section", "Section Alpha", "lct:CL-TEST:containers:section:section-alpha-"), + ) + @ddt.unpack + def test_containers_gets_auto_slugs(self, container_type, display_name, expected_id): """ Test that we can create containers by specifying only a title, and they get unique slugs assigned automatically. """ - unit_2 = self._create_container(self.lib["id"], "unit", display_name="Alpha Bravo", slug=None) - subsection_2 = self._create_container(self.lib["id"], "subsection", display_name="Subsection Alpha", slug=None) - section_2 = self._create_container(self.lib["id"], "section", display_name="Section Alpha", slug=None) - - assert self.unit["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") - assert unit_2["id"].startswith("lct:CL-TEST:containers:unit:alpha-bravo-") - assert self.unit["id"] != unit_2["id"] - - assert self.subsection["id"].startswith("lct:CL-TEST:containers:subsection:subsection-alpha-") - assert subsection_2["id"].startswith("lct:CL-TEST:containers:subsection:subsection-alpha-") - assert self.subsection["id"] != subsection_2["id"] - - assert self.section["id"].startswith("lct:CL-TEST:containers:section:section-alpha-") - assert section_2["id"].startswith("lct:CL-TEST:containers:section:section-alpha-") - assert self.section["id"] != section_2["id"] + container_1 = getattr(self, container_type) + container_2 = self._create_container(self.lib["id"], container_type, display_name=display_name, slug=None) + assert container_1["id"].startswith(expected_id) + assert container_2["id"].startswith(expected_id) + assert container_1["id"] != container_2["id"] def test_unit_add_children(self): """ @@ -385,20 +262,27 @@ def test_unit_replace_children(self): assert data[0]['id'] == new_problem_block['id'] assert data[1]['id'] == new_html_block['id'] - def test_restore_unit(self): + @ddt.data( + "unit", + "subsection", + "section", + ) + def test_restore_containers(self, container_type): """ - Test restore a deleted unit. + Test restore a deleted container. """ - # Delete the unit - self._delete_container(self.unit["id"]) + container = getattr(self, container_type) + + # Delete container + self._delete_container(container["id"]) # Restore container - self._restore_container(self.unit["id"]) - new_container_data = self._get_container(self.unit["id"]) + self._restore_container(container["id"]) + new_container_data = self._get_container(container["id"]) expected_data = { - "id": self.unit["id"], - "container_type": "unit", - "display_name": "Alpha Bravo", + "id": container["id"], + "container_type": container_type, + "display_name": container["display_name"], "last_published": None, "published_by": "", "last_draft_created": "2024-09-08T07:06:05Z", From e08f1b15e32f5a13d104288551864bd295f2fc61 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 22 May 2025 20:02:51 -0500 Subject: [PATCH 05/13] feat: CRUD support for section and subsection children --- .../content_libraries/api/containers.py | 87 ++++-- .../content_libraries/rest_api/containers.py | 18 +- .../content_libraries/rest_api/serializers.py | 16 +- .../content_libraries/tests/base.py | 18 +- .../tests/test_containers.py | 262 ++++++++++++++++-- 5 files changed, 330 insertions(+), 71 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 17019073b7f8..7ed8316fae5d 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -428,21 +428,37 @@ def get_container_children( published=False, ) -> list[LibraryXBlockMetadata | ContainerMetadata]: """ - Get the entities contained in the given container (e.g. the components/xblocks in a unit) + 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) - if container_key.container_type == ContainerType.Unit.value: - child_components = authoring_api.get_components_in_unit(container.unit, published=published) - return [LibraryXBlockMetadata.from_component( - container_key.lib_key, - entry.component - ) for entry in child_components] - else: - child_entities = authoring_api.get_entities_in_container(container, published=published) - return [ContainerMetadata.from_container( - container_key.lib_key, - entry.entity - ) for entry in child_entities] + container_type = ContainerType(container_key.container_type) + + match container_type: + case ContainerType.Unit: + child_components = authoring_api.get_components_in_unit(container.unit, published=published) + return [LibraryXBlockMetadata.from_component( + container_key.lib_key, + entry.component + ) for entry in child_components] + case ContainerType.Subsection: + child_units = authoring_api.get_units_in_subsection(container.subsection, published=published) + return [ContainerMetadata.from_container( + container_key.lib_key, + entry.unit + ) for entry in child_units] + case ContainerType.Section: + child_subsections = authoring_api.get_subsections_in_section(container.section, published=published) + return [ContainerMetadata.from_container( + container_key.lib_key, + entry.subsection, + ) for entry in child_subsections] + case _: + child_entities = authoring_api.get_entities_in_container(container, published=published) + return [ContainerMetadata.from_container( + container_key.lib_key, + entry.entity + ) for entry in child_entities] def get_container_children_count( @@ -466,15 +482,16 @@ def update_container_children( Adds children components or containers to given container. """ library_key = container_key.lib_key - container_type = container_key.container_type + container_type = ContainerType(container_key.container_type) container = _get_container_from_key(container_key) + created = datetime.now(tz=timezone.utc) match container_type: - case ContainerType.Unit.value: + case ContainerType.Unit: components = [get_component_from_usage_key(key) for key in children_ids] # type: ignore[arg-type] new_version = authoring_api.create_next_unit_version( container.unit, components=components, # type: ignore[arg-type] - created=datetime.now(tz=timezone.utc), + created=created, created_by=user_id, entities_action=entities_action, ) @@ -486,6 +503,44 @@ def update_container_children( changes=["units"], ), ) + case ContainerType.Subsection: + units = [] + for key in children_ids: + # Verify that all children are units + if not key.container_type == ContainerType.Unit.value: + raise ValueError( + f"Invalid children type: {key}. All Subsection children must be Units", + ) + units.append(_get_container_from_key(key).unit) + + new_version = authoring_api.create_next_subsection_version( + container.subsection, + units=units, # type: ignore[arg-type] + created=created, + created_by=user_id, + entities_action=entities_action, + ) + + # TODO add CONTENT_OBJECT_ASSOCIATIONS_CHANGED for subsections + case ContainerType.Section: + subsections = [] + for key in children_ids: + # Verify that all children are subsections + if not key.container_type == ContainerType.Subsection.value: + raise ValueError( + f"Invalid children type: {key}. All Section children must be Subsections", + ) + subsections.append(_get_container_from_key(key).subsection) + + new_version = authoring_api.create_next_section_version( + container.section, + subsections=subsections, # type: ignore[arg-type] + created=created, + created_by=user_id, + entities_action=entities_action, + ) + + # TODO add CONTENT_OBJECT_ASSOCIATIONS_CHANGED for sections case _: raise ValueError(f"Invalid container type: {container_type}") diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index b0c8a21d0448..36118346daa6 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -141,7 +141,7 @@ class LibraryContainerChildrenView(GenericAPIView): ) def get(self, request, container_key: LibraryContainerLocator): """ - Get children components of given container + Get children of given container Example: GET /api/libraries/v2/containers//children/ Result: @@ -205,10 +205,8 @@ def _update_component_children( request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, ) - serializer = serializers.ContentLibraryComponentKeysSerializer(data=request.data) + serializer = serializers.ContentLibraryItemContainerKeysSerializer(data=request.data) serializer.is_valid(raise_exception=True) - # Only components under units are supported for now. - assert container_key.container_type == api.ContainerType.Unit.value container = api.update_container_children( container_key, @@ -220,12 +218,12 @@ def _update_component_children( @convert_exceptions @swagger_auto_schema( - request_body=serializers.ContentLibraryComponentKeysSerializer, + request_body=serializers.ContentLibraryItemContainerKeysSerializer, responses={200: serializers.LibraryContainerMetadataSerializer} ) def post(self, request, container_key: LibraryContainerLocator): """ - Add components to unit + Add items to container Example: POST /api/libraries/v2/containers//children/ Request body: @@ -239,12 +237,12 @@ def post(self, request, container_key: LibraryContainerLocator): @convert_exceptions @swagger_auto_schema( - request_body=serializers.ContentLibraryComponentKeysSerializer, + request_body=serializers.ContentLibraryItemContainerKeysSerializer, responses={200: serializers.LibraryContainerMetadataSerializer} ) def delete(self, request, container_key: LibraryContainerLocator): """ - Remove components from unit + Remove items from container Example: DELETE /api/libraries/v2/containers//children/ Request body: @@ -258,12 +256,12 @@ def delete(self, request, container_key: LibraryContainerLocator): @convert_exceptions @swagger_auto_schema( - request_body=serializers.ContentLibraryComponentKeysSerializer, + request_body=serializers.ContentLibraryItemContainerKeysSerializer, responses={200: serializers.LibraryContainerMetadataSerializer} ) def patch(self, request, container_key: LibraryContainerLocator): """ - Replace components in unit, can be used to reorder components as well. + Replace items in container, can be used to reorder items as well. Example: PATCH /api/libraries/v2/containers//children/ Request body: diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index 38765f0b320f..6734babd00cc 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -339,14 +339,6 @@ def to_internal_value(self, value: str) -> LibraryUsageLocatorV2: raise ValidationError from err -class ContentLibraryComponentKeysSerializer(serializers.Serializer): - """ - Serializer for adding/removing Components to/from a Collection. - """ - - usage_keys = serializers.ListField(child=UsageKeyV2Serializer(), allow_empty=False) - - class OpaqueKeySerializer(serializers.BaseSerializer): """ Serializes a OpaqueKey with the correct class. @@ -372,6 +364,14 @@ def to_internal_value(self, value: str) -> OpaqueKey: raise ValidationError from err +class ContentLibraryItemContainerKeysSerializer(serializers.Serializer): + """ + Serializer for adding/removing items to/from a Container. + """ + + usage_keys = serializers.ListField(child=OpaqueKeySerializer(), allow_empty=False) + + class ContentLibraryItemKeysSerializer(serializers.Serializer): """ Serializer for adding/removing items to/from a Collection. diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index e5a9f5f12ec9..0036c208b0c7 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -38,7 +38,7 @@ 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_CONTAINER = URL_PREFIX + 'containers/{container_key}/' # Get a container in this library -URL_LIB_CONTAINER_COMPONENTS = URL_LIB_CONTAINER + 'children/' # Get, add or delete a component in this container +URL_LIB_CONTAINER_CHILDREN = URL_LIB_CONTAINER + 'children/' # Get, add or delete a component in this container 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 @@ -396,25 +396,25 @@ def _restore_container(self, container_key: ContainerKey | str, expect_response= """ Restore a deleted a container (unit etc.) """ return self._api('post', URL_LIB_CONTAINER_RESTORE.format(container_key=container_key), None, expect_response) - def _get_container_components(self, container_key: ContainerKey | str, expect_response=200): - """ Get container components""" + def _get_container_children(self, container_key: ContainerKey | str, expect_response=200): + """ Get container children""" return self._api( 'get', - URL_LIB_CONTAINER_COMPONENTS.format(container_key=container_key), + URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key), None, expect_response ) - def _add_container_components( + def _add_container_children( self, container_key: ContainerKey | str, children_ids: list[str], expect_response=200, ): - """ Add container components""" + """ Add container children""" return self._api( 'post', - URL_LIB_CONTAINER_COMPONENTS.format(container_key=container_key), + URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key), {'usage_keys': children_ids}, expect_response ) @@ -428,7 +428,7 @@ def _remove_container_components( """ Remove container components""" return self._api( 'delete', - URL_LIB_CONTAINER_COMPONENTS.format(container_key=container_key), + URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key), {'usage_keys': children_ids}, expect_response ) @@ -442,7 +442,7 @@ def _patch_container_components( """ Update container components""" return self._api( 'patch', - URL_LIB_CONTAINER_COMPONENTS.format(container_key=container_key), + URL_LIB_CONTAINER_CHILDREN.format(container_key=container_key), {'usage_keys': children_ids}, expect_response ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index c7a9ecbeaf24..0a284da18915 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -38,7 +38,11 @@ class ContainersTestCase(ContentLibrariesRestApiTest): def setUp(self): super().setUp() self.create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) - self.lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") + self.lib = self._create_library( + slug="containers", + title="Container Test Library", + description="Units and more", + ) self.lib_key = LibraryLocatorV2.from_string(self.lib["id"]) # Create containers @@ -51,10 +55,43 @@ def setUp(self): display_name="Alpha Charly", slug=None, ) + self.unit_2 = self._create_container(self.lib["id"], "unit", display_name="Test Unit 2", slug=None) + self.unit_3 = self._create_container(self.lib["id"], "unit", display_name="Test Unit 3", slug=None) + # Subsection - self.subsection = self._create_container(self.lib["id"], "subsection", display_name="Subsection Alpha", slug=None) + self.subsection = self._create_container( + self.lib["id"], + "subsection", + display_name="Subsection Alpha", + slug=None, + ) + self.subsection_with_units = self._create_container( + self.lib["id"], + "subsection", + display_name="Subsection with units", + slug=None, + ) + self.subsection_2 = self._create_container( + self.lib["id"], + "subsection", + display_name="Test Subsection 2", + slug=None, + ) + self.subsection_3 = self._create_container( + self.lib["id"], + "subsection", + display_name="Test Subsection 3", + slug=None, + ) + # Section self.section = self._create_container(self.lib["id"], "section", display_name="Section Alpha", slug=None) + self.section_with_subsections = self._create_container( + self.lib["id"], + "section", + display_name="Section with subsections", + slug=None, + ) # Create blocks self.problem_block = self._add_block_to_library(self.lib["id"], "problem", "Problem1", can_stand_alone=False) @@ -63,14 +100,34 @@ def setUp(self): self.html_block_2 = self._add_block_to_library(self.lib["id"], "html", "Html2") # Add components to `unit_with_components` - self._add_container_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"], + 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"], + self.subsection_with_units["id"], + self.subsection_2["id"], + self.subsection_3["id"], + ], ) @ddt.data( @@ -92,9 +149,9 @@ def test_container_crud(self, container_type, slug, display_name): slug=slug, display_name=display_name ) - id = f"lct:CL-TEST:containers:{container_type}:{slug}" + container_id = f"lct:CL-TEST:containers:{container_type}:{slug}" expected_data = { - "id": id, + "id": container_id, "container_type": container_type, "display_name": display_name, "last_published": None, @@ -117,7 +174,7 @@ def test_container_crud(self, container_type, slug, display_name): # Update the container: modified_date = datetime(2024, 10, 9, 8, 7, 6, tzinfo=timezone.utc) with freeze_time(modified_date): - container_data = self._update_container(id, display_name=f"New Display Name for {container_type}") + container_data = self._update_container(container_id, display_name=f"New Display Name for {container_type}") expected_data["last_draft_created"] = expected_data["modified"] = "2024-10-09T08:07:06Z" expected_data["display_name"] = f"New Display Name for {container_type}" self.assertDictContainsEntries(container_data, expected_data) @@ -180,12 +237,12 @@ def test_unit_add_children(self): """ Test that we can add and get unit children components """ - # Create container and add some components - self._add_container_components( + # Add some components + self._add_container_children( self.unit["id"], children_ids=[self.problem_block["id"], self.html_block["id"]] ) - data = self._get_container_components(self.unit["id"]) + data = self._get_container_children(self.unit["id"]) assert len(data) == 2 assert data[0]['id'] == self.problem_block['id'] assert not data[0]['can_stand_alone'] @@ -194,11 +251,11 @@ def test_unit_add_children(self): problem_block_2 = self._add_block_to_library(self.lib["id"], "problem", "Problem_2", can_stand_alone=False) html_block_2 = self._add_block_to_library(self.lib["id"], "html", "Html_2") # Add two more components - self._add_container_components( + self._add_container_children( self.unit["id"], children_ids=[problem_block_2["id"], html_block_2["id"]] ) - data = self._get_container_components(self.unit["id"]) + data = self._get_container_children(self.unit["id"]) # Verify total number of components to be 2 + 2 = 4 assert len(data) == 4 assert data[2]['id'] == problem_block_2['id'] @@ -206,27 +263,96 @@ def test_unit_add_children(self): assert data[3]['id'] == html_block_2['id'] assert data[3]['can_stand_alone'] - def test_unit_remove_children(self): + def test_subsection_add_children(self): + # Create units + child_unit_1 = self._create_container(self.lib["id"], "unit", display_name="Child unit 1", slug=None) + child_unit_2 = self._create_container(self.lib["id"], "unit", display_name="Child unit 2", slug=None) + + # Add the units to subsection + self._add_container_children( + self.subsection["id"], + children_ids=[child_unit_1["id"], child_unit_2["id"]] + ) + data = self._get_container_children(self.subsection["id"]) + assert len(data) == 2 + assert data[0]['id'] == child_unit_1['id'] + assert data[1]['id'] == child_unit_2['id'] + + child_unit_3 = self._create_container(self.lib["id"], "unit", display_name="Child unit 3", slug=None) + child_unit_4 = self._create_container(self.lib["id"], "unit", display_name="Child unit 4", slug=None) + + # Add two more units to subsection + self._add_container_children( + self.subsection["id"], + children_ids=[child_unit_3["id"], child_unit_4["id"]] + ) + data = self._get_container_children(self.subsection["id"]) + # Verify total number of units to be 2 + 2 = 4 + assert len(data) == 4 + assert data[2]['id'] == child_unit_3['id'] + assert data[3]['id'] == child_unit_4['id'] + + def test_section_add_children(self): + # Create Subsections + child_subsection_1 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 1", slug=None) + child_subsection_2 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 2", slug=None) + + # Add the subsections to section + self._add_container_children( + self.section["id"], + children_ids=[child_subsection_1["id"], child_subsection_2["id"]] + ) + data = self._get_container_children(self.section["id"]) + assert len(data) == 2 + assert data[0]['id'] == child_subsection_1['id'] + assert data[1]['id'] == child_subsection_2['id'] + + child_subsection_3 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 3", slug=None) + child_subsection_4 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 4", slug=None) + + # Add two more subsections to section + self._add_container_children( + self.section["id"], + children_ids=[child_subsection_3["id"], child_subsection_4["id"]] + ) + data = self._get_container_children(self.section["id"]) + # Verify total number of subsections to be 2 + 2 = 4 + assert len(data) == 4 + assert data[2]['id'] == child_subsection_3['id'] + assert data[3]['id'] == child_subsection_4['id'] + + @ddt.data( + ("unit_with_components", ["problem_block_2", "problem_block"], ["html_block", "html_block_2"]), + ("subsection_with_units", ["unit", "unit_with_components"], ["unit_2", "unit_3"]), + ("section_with_subsections", ["subsection", "subsection_with_units"], ["subsection_2", "subsection_3"]), + ) + @ddt.unpack + def test_container_remove_children(self, container_name, items_to_remove, expected_items): """ - Test that we can remove unit children components + Test that we can remove container children """ - data = self._get_container_components(self.unit_with_components["id"]) + container = getattr(self, container_name) + item_to_remove_1 = getattr(self, items_to_remove[0]) + item_to_remove_2 = getattr(self, items_to_remove[1]) + expected_item_1 = getattr(self, expected_items[0]) + expected_item_2 = getattr(self, expected_items[1]) + data = self._get_container_children(container["id"]) assert len(data) == 4 - # Remove both problem blocks. + # Remove items. self._remove_container_components( - self.unit_with_components["id"], - children_ids=[self.problem_block_2["id"], self.problem_block["id"]] + container["id"], + children_ids=[item_to_remove_1["id"], item_to_remove_2["id"]] ) - data = self._get_container_components(self.unit_with_components["id"]) + data = self._get_container_children(container["id"]) assert len(data) == 2 - assert data[0]['id'] == self.html_block['id'] - assert data[1]['id'] == self.html_block_2['id'] + assert data[0]['id'] == expected_item_1['id'] + assert data[1]['id'] == expected_item_2['id'] def test_unit_replace_children(self): """ Test that we can completely replace/reorder unit children components. """ - data = self._get_container_components(self.unit_with_components["id"]) + data = self._get_container_children(self.unit_with_components["id"]) assert len(data) == 4 assert data[0]['id'] == self.problem_block['id'] assert data[1]['id'] == self.html_block['id'] @@ -243,7 +369,7 @@ def test_unit_replace_children(self): self.html_block_2["id"], ] ) - data = self._get_container_components(self.unit_with_components["id"]) + data = self._get_container_children(self.unit_with_components["id"]) assert len(data) == 4 assert data[0]['id'] == self.problem_block['id'] assert data[1]['id'] == self.problem_block_2['id'] @@ -257,11 +383,91 @@ def test_unit_replace_children(self): self.unit_with_components["id"], children_ids=[new_problem_block["id"], new_html_block["id"]], ) - data = self._get_container_components(self.unit_with_components["id"]) + data = self._get_container_children(self.unit_with_components["id"]) assert len(data) == 2 assert data[0]['id'] == new_problem_block['id'] assert data[1]['id'] == new_html_block['id'] + def test_subsection_replace_children(self): + """ + Test that we can completely replace/reorder subsection children. + """ + data = self._get_container_children(self.subsection_with_units["id"]) + assert len(data) == 4 + assert data[0]['id'] == self.unit['id'] + assert data[1]['id'] == self.unit_with_components['id'] + assert data[2]['id'] == self.unit_2['id'] + assert data[3]['id'] == self.unit_3['id'] + + # Reorder the units + self._patch_container_components( + self.subsection_with_units["id"], + children_ids=[ + self.unit_2["id"], + self.unit["id"], + self.unit_3["id"], + self.unit_with_components["id"], + ] + ) + data = self._get_container_children(self.subsection_with_units["id"]) + assert len(data) == 4 + assert data[0]['id'] == self.unit_2['id'] + assert data[1]['id'] == self.unit['id'] + assert data[2]['id'] == self.unit_3['id'] + assert data[3]['id'] == self.unit_with_components['id'] + + # 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.subsection_with_units["id"], + children_ids=[new_unit_1["id"], new_unit_2["id"]], + ) + data = self._get_container_children(self.subsection_with_units["id"]) + assert len(data) == 2 + assert data[0]['id'] == new_unit_1['id'] + assert data[1]['id'] == new_unit_2['id'] + + def test_section_replace_children(self): + """ + Test that we can completely replace/reorder section children. + """ + data = self._get_container_children(self.section_with_subsections["id"]) + assert len(data) == 4 + assert data[0]['id'] == self.subsection['id'] + assert data[1]['id'] == self.subsection_with_units['id'] + assert data[2]['id'] == self.subsection_2['id'] + assert data[3]['id'] == self.subsection_3['id'] + + # Reorder the subsections + self._patch_container_components( + self.section_with_subsections["id"], + children_ids=[ + self.subsection_2["id"], + self.subsection["id"], + self.subsection_3["id"], + self.subsection_with_units["id"], + ] + ) + data = self._get_container_children(self.section_with_subsections["id"]) + assert len(data) == 4 + assert data[0]['id'] == self.subsection_2['id'] + assert data[1]['id'] == self.subsection['id'] + assert data[2]['id'] == self.subsection_3['id'] + assert data[3]['id'] == self.subsection_with_units['id'] + + # Replace with new subsections + new_subsection_1 = self._create_container(self.lib["id"], "subsection", display_name="New Subsection 1", slug=None) + new_subsection_2 = self._create_container(self.lib["id"], "subsection", display_name="New Subsection 2", slug=None) + self._patch_container_components( + self.section_with_subsections["id"], + children_ids=[new_subsection_1["id"], new_subsection_2["id"]], + ) + data = self._get_container_children(self.section_with_subsections["id"]) + assert len(data) == 2 + assert data[0]['id'] == new_subsection_1['id'] + assert data[1]['id'] == new_subsection_2['id'] + @ddt.data( "unit", "subsection", @@ -323,7 +529,7 @@ def test_publish_container(self): # pylint: disable=too-many-statements Test that we can publish the changes to a specific container """ html_block_3 = self._add_block_to_library(self.lib["id"], "html", "Html3") - self._add_container_components( + self._add_container_children( self.unit["id"], children_ids=[ self.html_block["id"], @@ -334,7 +540,7 @@ def test_publish_container(self): # 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_components(self.unit_with_components["id"]) + 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"] @@ -357,7 +563,7 @@ def test_publish_container(self): # pylint: disable=too-many-statements # 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_components(self.unit_with_components["id"]) + 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 @@ -375,7 +581,7 @@ def test_publish_container(self): # pylint: disable=too-many-statements # and container 2 is still unpublished, except for the shared HTML block that is also in container 1: c2_after = self._get_container(self.unit["id"]) assert c2_after["has_unpublished_changes"] - c2_components_after = self._get_container_components(self.unit["id"]) + 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 From 7c478d3bf9d69137c06cfc6ac3809d3e0b3ae117 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Fri, 23 May 2025 19:22:23 -0500 Subject: [PATCH 06/13] feat: Basic CRUD and Children CRUD for search index for section and subections --- .../djangoapps/content/search/documents.py | 28 +++- .../content/search/tests/test_api.py | 157 ++++++++++++++++-- .../content/search/tests/test_documents.py | 47 +++++- .../content_libraries/api/containers.py | 7 +- .../tests/test_containers.py | 64 +++++-- .../content_libraries/tests/test_events.py | 10 +- 6 files changed, 267 insertions(+), 46 deletions(-) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 36698da6a091..29b3673a0172 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -95,6 +95,9 @@ class Fields: published_content = "content" published_num_children = "num_children" + # List of children keys + child_usage_keys = "child_usage_keys" + # Note: new fields or values can be added at any time, but if they need to be indexed for filtering or keyword # search, the index configuration will need to be changed, which is only done as part of the 'reindex_studio' # command (changing those settings on an large active index is not recommended). @@ -656,16 +659,28 @@ def searchable_doc_for_container( elif container.has_unpublished_changes: publish_status = PublishStatus.modified + container_type = lib_api.ContainerType(container_key.container_type) + + def get_child_keys(children): + match container_type: + case lib_api.ContainerType.Unit: + return [ + str(child.usage_key) + for child in children + ] + case lib_api.ContainerType.Subsection | lib_api.ContainerType.Section: + return [ + str(child.container_key) + for child in children + ] + doc.update({ Fields.display_name: container.display_name, Fields.created: container.created.timestamp(), Fields.modified: container.modified.timestamp(), Fields.num_children: len(draft_children), Fields.content: { - "child_usage_keys": [ - str(child.usage_key) - for child in draft_children - ], + Fields.child_usage_keys: get_child_keys(draft_children) }, Fields.publish_status: publish_status, Fields.last_published: container.last_published.timestamp() if container.last_published else None, @@ -683,10 +698,7 @@ def searchable_doc_for_container( Fields.published_display_name: container.published_display_name, Fields.published_num_children: len(published_children), Fields.published_content: { - "child_usage_keys": [ - str(child.usage_key) - for child in published_children - ], + Fields.child_usage_keys: get_child_keys(published_children), }, } diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 2093b3dc8587..4718afb3e828 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -219,7 +219,7 @@ def setUp(self): "breadcrumbs": [{"display_name": "Library"}], } - # Create a unit: + # Create a container: with freeze_time(self.created_date): self.unit = library_api.create_container( library_key=self.library.key, @@ -229,6 +229,23 @@ def setUp(self): user_id=None, ) self.unit_key = "lct:org1:lib:unit:unit-1" + self.subsection = library_api.create_container( + self.library.key, + container_type=library_api.ContainerType.Subsection, + slug="subsection-1", + title="Subsection 1", + user_id=None, + ) + self.subsection_key = "lct:org1:lib:subsection:subsection-1" + self.section = library_api.create_container( + self.library.key, + container_type=library_api.ContainerType.Section, + slug="section-1", + title="Section 1", + user_id=None, + ) + self.section_key = "lct:org1:lib:section:section-1" + self.unit_dict = { "id": "lctorg1libunitunit-1-e4527f7c", "block_id": "unit-1", @@ -249,6 +266,46 @@ def setUp(self): "breadcrumbs": [{"display_name": "Library"}], # "published" is not set since we haven't published it yet } + self.subsection_dict = { + "id": "lctorg1libsubsectionsubsection-1-cf808309", + "block_id": "subsection-1", + "block_type": "subsection", + "usage_key": self.subsection_key, + "type": "library_container", + "display_name": "Subsection 1", + # description is not set for containers + "num_children": 0, + "content": {"child_usage_keys": []}, + "publish_status": "never", + "context_key": "lib:org1:lib", + "org": "org1", + "created": self.created_date.timestamp(), + "modified": self.created_date.timestamp(), + "last_published": None, + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + # "published" is not set since we haven't published it yet + } + self.section_dict = { + "id": "lctorg1libsectionsection-1-dc4791a4", + "block_id": "section-1", + "block_type": "section", + "usage_key": self.section_key, + "type": "library_container", + "display_name": "Section 1", + # description is not set for containers + "num_children": 0, + "content": {"child_usage_keys": []}, + "publish_status": "never", + "context_key": "lib:org1:lib", + "org": "org1", + "created": self.created_date.timestamp(), + "modified": self.created_date.timestamp(), + "last_published": None, + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + # "published" is not set since we haven't published it yet + } @override_settings(MEILISEARCH_ENABLED=False) def test_reindex_meilisearch_disabled(self, mock_meilisearch): @@ -278,6 +335,12 @@ def test_reindex_meilisearch(self, mock_meilisearch): doc_unit = copy.deepcopy(self.unit_dict) doc_unit["tags"] = {} doc_unit["collections"] = {'display_name': [], 'key': []} + doc_subsection = copy.deepcopy(self.subsection_dict) + doc_subsection["tags"] = {} + doc_subsection["collections"] = {'display_name': [], 'key': []} + doc_section = copy.deepcopy(self.section_dict) + doc_section["tags"] = {} + doc_section["collections"] = {'display_name': [], 'key': []} api.rebuild_index() assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 @@ -286,7 +349,7 @@ def test_reindex_meilisearch(self, mock_meilisearch): call([doc_sequential, doc_vertical]), call([doc_problem1, doc_problem2]), call([doc_collection]), - call([doc_unit]), + call([doc_unit, doc_subsection, doc_section]), ], any_order=True, ) @@ -312,6 +375,12 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch): doc_unit = copy.deepcopy(self.unit_dict) doc_unit["tags"] = {} doc_unit["collections"] = {"display_name": [], "key": []} + doc_subsection = copy.deepcopy(self.subsection_dict) + doc_subsection["tags"] = {} + doc_subsection["collections"] = {'display_name': [], 'key': []} + doc_section = copy.deepcopy(self.section_dict) + doc_section["tags"] = {} + doc_section["collections"] = {'display_name': [], 'key': []} api.rebuild_index(incremental=True) assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 @@ -320,7 +389,7 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch): call([doc_sequential, doc_vertical]), call([doc_problem1, doc_problem2]), call([doc_collection]), - call([doc_unit]), + call([doc_unit, doc_subsection, doc_section]), ], any_order=True, ) @@ -894,15 +963,23 @@ def test_delete_collection(self, mock_meilisearch): any_order=True, ) + @ddt.data( + "unit", + "subsection", + "section", + ) @override_settings(MEILISEARCH_ENABLED=True) - def test_delete_index_container(self, mock_meilisearch): + def test_delete_index_container(self, container_type, mock_meilisearch): """ Test delete a container index. """ - library_api.delete_container(self.unit.container_key) + container = getattr(self, container_type) + container_dict = getattr(self, f"{container_type}_dict") + + library_api.delete_container(container.container_key) mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( - self.unit_dict["id"], + container_dict["id"], ) @override_settings(MEILISEARCH_ENABLED=True) @@ -914,22 +991,30 @@ def test_index_library_container_metadata(self, mock_meilisearch): mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.unit_dict]) + @ddt.data( + ("unit", "lctorg1libunitunit-1-e4527f7c"), + ("subsection", "lctorg1libsubsectionsubsection-1-cf808309"), + ("section", "lctorg1libsectionsection-1-dc4791a4"), + ) + @ddt.unpack @override_settings(MEILISEARCH_ENABLED=True) - def test_index_tags_in_containers(self, mock_meilisearch): - # Tag collection - tagging_api.tag_object(self.unit_key, self.taxonomyA, ["one", "two"]) - tagging_api.tag_object(self.unit_key, self.taxonomyB, ["three", "four"]) + def test_index_tags_in_containers(self, container_type, container_id, mock_meilisearch): + container_key = getattr(self, f"{container_type}_key") + + # Tag container + tagging_api.tag_object(container_key, self.taxonomyA, ["one", "two"]) + tagging_api.tag_object(container_key, self.taxonomyB, ["three", "four"]) # Build expected docs with tags at each stage doc_unit_with_tags1 = { - "id": "lctorg1libunitunit-1-e4527f7c", + "id": container_id, "tags": { 'taxonomy': ['A'], 'level0': ['A > one', 'A > two'] } } doc_unit_with_tags2 = { - "id": "lctorg1libunitunit-1-e4527f7c", + "id": container_id, "tags": { 'taxonomy': ['A', 'B'], 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] @@ -975,3 +1060,51 @@ def test_block_in_units(self, mock_meilisearch): ], any_order=True, ) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_units_in_subsection(self, mock_meilisearch): + with freeze_time(self.created_date): + library_api.update_container_children( + LibraryContainerLocator.from_string(self.subsection_key), + [LibraryContainerLocator.from_string(self.unit_key)], + None, + ) + + # TODO verify subsections in units + + new_subsection_dict = { + **self.subsection_dict, + "num_children": 1, + 'content': {'child_usage_keys': [self.unit_key]} + } + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 1 + mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( + [ + call([new_subsection_dict]), + ], + any_order=True, + ) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_section_in_usbsections(self, mock_meilisearch): + with freeze_time(self.created_date): + library_api.update_container_children( + LibraryContainerLocator.from_string(self.section_key), + [LibraryContainerLocator.from_string(self.subsection_key)], + None, + ) + + # TODO verify section in subsections + + new_section_dict = { + **self.section_dict, + "num_children": 1, + 'content': {'child_usage_keys': [self.subsection_key]} + } + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 1 + mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( + [ + call([new_section_dict]), + ], + any_order=True, + ) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 74772c89c017..8d019ab31c53 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -1,6 +1,7 @@ """ Tests for the Studio content search documents (what gets stored in the index) """ +import ddt from dataclasses import replace from datetime import datetime, timezone @@ -42,6 +43,7 @@ @skip_unless_cms +@ddt.ddt class StudioDocumentsTest(SharedModuleStoreTestCase): """ Tests for the Studio content search documents (what gets stored in the @@ -100,6 +102,26 @@ def setUpClass(cls): cls.container_key = LibraryContainerLocator.from_string( "lct:edX:2012_Fall:unit:unit1", ) + cls.subsection = library_api.create_container( + cls.library.key, + container_type=library_api.ContainerType.Subsection, + slug="subsection1", + title="A Subsection in the Search Index", + user_id=None, + ) + cls.subsection_key = LibraryContainerLocator.from_string( + "lct:edX:2012_Fall:subsection:subsection1", + ) + cls.section = library_api.create_container( + cls.library.key, + container_type=library_api.ContainerType.Section, + slug="section1", + title="A Section in the Search Index", + user_id=None, + ) + cls.section_key = LibraryContainerLocator.from_string( + "lct:edX:2012_Fall:section:section1", + ) # Add the problem block to the collection library_api.update_library_collection_items( @@ -130,6 +152,8 @@ def setUpClass(cls): tagging_api.tag_object(str(cls.library_block.usage_key), cls.difficulty_tags, tags=["Normal"]) tagging_api.tag_object(str(cls.collection_key), cls.difficulty_tags, tags=["Normal"]) tagging_api.tag_object(str(cls.container_key), cls.difficulty_tags, tags=["Normal"]) + tagging_api.tag_object(str(cls.subsection_key), cls.difficulty_tags, tags=["Normal"]) + tagging_api.tag_object(str(cls.section_key), cls.difficulty_tags, tags=["Normal"]) @property def toy_course_access_id(self): @@ -514,21 +538,28 @@ def test_collection_with_published_library(self): } } - def test_draft_container(self): + @ddt.data( + ("container", "unit1", "unit", "edd13a0c"), + ("subsection", "subsection1", "subsection", "c6c172be"), + ("section", "section1", "section", "79ee8fa2"), + ) + @ddt.unpack + def test_draft_container_1(self, container_name, container_slug, container_type, doc_id): """ Test creating a search document for a draft-only container """ - doc = searchable_doc_for_container(self.container.container_key) - doc.update(searchable_doc_tags(self.container.container_key)) + container = getattr(self, container_name) + doc = searchable_doc_for_container(container.container_key) + doc.update(searchable_doc_tags(container.container_key)) assert doc == { - "id": "lctedx2012_fallunitunit1-edd13a0c", - "block_id": "unit1", - "block_type": "unit", - "usage_key": "lct:edX:2012_Fall:unit:unit1", + "id": f"lctedx2012_fall{container_type}{container_slug}-{doc_id}", + "block_id": container_slug, + "block_type": container_type, + "usage_key": f"lct:edX:2012_Fall:{container_type}:{container_slug}", "type": "library_container", "org": "edX", - "display_name": "A Unit in the Search Index", + "display_name": container.display_name, # description is not set for containers "num_children": 0, "content": { diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 7ed8316fae5d..1cb39bf9db8d 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -485,6 +485,7 @@ def update_container_children( container_type = ContainerType(container_key.container_type) container = _get_container_from_key(container_key) created = datetime.now(tz=timezone.utc) + new_version: ContainerVersion match container_type: case ContainerType.Unit: components = [get_component_from_usage_key(key) for key in children_ids] # type: ignore[arg-type] @@ -507,7 +508,8 @@ def update_container_children( units = [] for key in children_ids: # Verify that all children are units - if not key.container_type == ContainerType.Unit.value: + if not isinstance(key, LibraryContainerLocator) \ + or not key.container_type == ContainerType.Unit.value: raise ValueError( f"Invalid children type: {key}. All Subsection children must be Units", ) @@ -526,7 +528,8 @@ def update_container_children( subsections = [] for key in children_ids: # Verify that all children are subsections - if not key.container_type == ContainerType.Subsection.value: + if not isinstance(key, LibraryContainerLocator) \ + or not key.container_type == ContainerType.Subsection.value: raise ValueError( f"Invalid children type: {key}. All Section children must be Subsections", ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 0a284da18915..e0fea5c861d4 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -148,7 +148,7 @@ def test_container_crud(self, container_type, slug, display_name): container_type, slug=slug, display_name=display_name - ) + ) container_id = f"lct:CL-TEST:containers:{container_type}:{slug}" expected_data = { "id": container_id, @@ -202,7 +202,13 @@ def test_container_permissions(self, container_type, slug, display_name): random_user = UserFactory.create(username="Random", email="random@example.com") with self.as_user(random_user): - self._create_container(self.lib["id"], container_type, slug="new_slug", display_name=display_name, expect_response=403) + self._create_container( + self.lib["id"], + container_type, + slug="new_slug", + display_name=display_name, + expect_response=403, + ) self._get_container(container_data["id"], expect_response=403) self._update_container(container_data["id"], display_name="New Display Name", expect_response=403) self._delete_container(container_data["id"], expect_response=403) @@ -210,7 +216,13 @@ def test_container_permissions(self, container_type, slug, display_name): # Granting read-only permissions on the library should only allow retrieval, nothing else. self._add_user_by_email(self.lib["id"], random_user.email, access_level="read") with self.as_user(random_user): - self._create_container(self.lib["id"], container_type, slug=slug, display_name=display_name, expect_response=403) + self._create_container( + self.lib["id"], + container_type, + slug=slug, + display_name=display_name, + expect_response=403, + ) self._get_container(container_data["id"], expect_response=200) self._update_container(container_data["id"], display_name="New Display Name", expect_response=403) self._delete_container(container_data["id"], expect_response=403) @@ -289,13 +301,23 @@ def test_subsection_add_children(self): data = self._get_container_children(self.subsection["id"]) # Verify total number of units to be 2 + 2 = 4 assert len(data) == 4 - assert data[2]['id'] == child_unit_3['id'] + assert data[2]['id'] == child_unit_3['id'] assert data[3]['id'] == child_unit_4['id'] def test_section_add_children(self): # Create Subsections - child_subsection_1 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 1", slug=None) - child_subsection_2 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 2", slug=None) + child_subsection_1 = self._create_container( + self.lib["id"], + "subsection", + display_name="Child Subsection 1", + slug=None, + ) + child_subsection_2 = self._create_container( + self.lib["id"], + "subsection", + display_name="Child Subsection 2", + slug=None, + ) # Add the subsections to section self._add_container_children( @@ -307,8 +329,18 @@ def test_section_add_children(self): assert data[0]['id'] == child_subsection_1['id'] assert data[1]['id'] == child_subsection_2['id'] - child_subsection_3 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 3", slug=None) - child_subsection_4 = self._create_container(self.lib["id"], "subsection", display_name="Child Subsection 4", slug=None) + child_subsection_3 = self._create_container( + self.lib["id"], + "subsection", + display_name="Child Subsection 3", + slug=None, + ) + child_subsection_4 = self._create_container( + self.lib["id"], + "subsection", + display_name="Child Subsection 4", + slug=None, + ) # Add two more subsections to section self._add_container_children( @@ -318,7 +350,7 @@ def test_section_add_children(self): data = self._get_container_children(self.section["id"]) # Verify total number of subsections to be 2 + 2 = 4 assert len(data) == 4 - assert data[2]['id'] == child_subsection_3['id'] + assert data[2]['id'] == child_subsection_3['id'] assert data[3]['id'] == child_subsection_4['id'] @ddt.data( @@ -457,8 +489,18 @@ def test_section_replace_children(self): assert data[3]['id'] == self.subsection_with_units['id'] # Replace with new subsections - new_subsection_1 = self._create_container(self.lib["id"], "subsection", display_name="New Subsection 1", slug=None) - new_subsection_2 = self._create_container(self.lib["id"], "subsection", display_name="New Subsection 2", slug=None) + new_subsection_1 = self._create_container( + self.lib["id"], + "subsection", + display_name="New Subsection 1", + slug=None, + ) + new_subsection_2 = self._create_container( + self.lib["id"], + "subsection", + display_name="New Subsection 2", + slug=None, + ) self._patch_container_components( self.section_with_subsections["id"], children_ids=[new_subsection_1["id"], new_subsection_2["id"]], diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index e0e3e7392765..88d426d3ef06 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -308,8 +308,8 @@ def test_publish_all_lib_changes(self) -> None: problem_block = self._add_block_to_library(self.lib1_key, "problem", "Problem1", can_stand_alone=False) html_block = self._add_block_to_library(self.lib1_key, "html", "Html1", can_stand_alone=False) html_block2 = self._add_block_to_library(self.lib1_key, "html", "Html2", can_stand_alone=False) - self._add_container_components(container1["id"], children_ids=[problem_block["id"], html_block["id"]]) - self._add_container_components(container2["id"], children_ids=[html_block["id"], html_block2["id"]]) + self._add_container_children(container1["id"], children_ids=[problem_block["id"], html_block["id"]]) + self._add_container_children(container2["id"], children_ids=[html_block["id"], html_block2["id"]]) # Now publish only Container 2 (which will auto-publish both HTML blocks since they're children) self._publish_container(container2["id"]) @@ -351,7 +351,7 @@ def test_publish_child_block(self) -> None: # Create a container and a block container1 = self._create_container(self.lib1_key, "unit", display_name="Alpha Unit", slug=None) problem_block = self._add_block_to_library(self.lib1_key, "problem", "Problem1", can_stand_alone=False) - self._add_container_components(container1["id"], children_ids=[problem_block["id"]]) + self._add_container_children(container1["id"], children_ids=[problem_block["id"]]) # Publish all changes self._commit_library_changes(self.lib1_key) assert self._get_container(container1["id"])["has_unpublished_changes"] is False @@ -396,8 +396,8 @@ def test_publish_container(self) -> None: problem_block = self._add_block_to_library(self.lib1_key, "problem", "Problem1", can_stand_alone=False) html_block = self._add_block_to_library(self.lib1_key, "html", "Html1", can_stand_alone=False) html_block2 = self._add_block_to_library(self.lib1_key, "html", "Html2", can_stand_alone=False) - self._add_container_components(container1["id"], children_ids=[problem_block["id"], html_block["id"]]) - self._add_container_components(container2["id"], children_ids=[html_block["id"], html_block2["id"]]) + self._add_container_children(container1["id"], children_ids=[problem_block["id"], html_block["id"]]) + self._add_container_children(container2["id"], children_ids=[html_block["id"], html_block2["id"]]) # At first everything is unpublished: c1_before = self._get_container(container1["id"]) assert c1_before["has_unpublished_changes"] From 8ed07f3c2c6ddc5642d515e7c96a3e6202da1862 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 26 May 2025 17:09:44 -0500 Subject: [PATCH 07/13] style: Fix broken lint --- .../core/djangoapps/content/search/tests/test_api.py | 1 + .../djangoapps/content_libraries/api/containers.py | 12 ++++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 4718afb3e828..2a1aced68601 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -48,6 +48,7 @@ class TestSearchApi(ModuleStoreTestCase): MODULESTORE = TEST_DATA_SPLIT_MODULESTORE def setUp(self): + # pylint: disable=too-many-statements super().setUp() self.user = UserFactory.create() self.user_id = self.user.id diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 1cb39bf9db8d..e8dbdbaad9ee 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -508,8 +508,10 @@ def update_container_children( units = [] for key in children_ids: # Verify that all children are units - if not isinstance(key, LibraryContainerLocator) \ - or not key.container_type == ContainerType.Unit.value: + if ( + not isinstance(key, LibraryContainerLocator) + or not key.container_type == ContainerType.Unit.value + ): raise ValueError( f"Invalid children type: {key}. All Subsection children must be Units", ) @@ -528,8 +530,10 @@ def update_container_children( subsections = [] for key in children_ids: # Verify that all children are subsections - if not isinstance(key, LibraryContainerLocator) \ - or not key.container_type == ContainerType.Subsection.value: + if ( + not isinstance(key, LibraryContainerLocator) + or not key.container_type == ContainerType.Subsection.value + ): raise ValueError( f"Invalid children type: {key}. All Section children must be Subsections", ) From b269d46bd740c1bbd5ad39599db0dbe674beb93b Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 26 May 2025 17:45:04 -0500 Subject: [PATCH 08/13] style: fix tests --- .../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 8f45fdd8cc9d..5c1bd5774403 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 @@ -58,7 +58,7 @@ def test_library_paste_unit_from_course(self): "can_stand_alone": True, }) - children = self._get_container_components(paste_data["id"]) + children = self._get_container_children(paste_data["id"]) assert len(children) == 4 self.assertDictContainsEntries(children[0], {"display_name": "default", "block_type": "video"}) From 539f26c133a7aecc00087018d481a268abee0b2f Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 26 May 2025 18:32:23 -0500 Subject: [PATCH 09/13] test: Add/remove containers in collections --- .../tests/test_views_collections.py | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py index 283f5fbe97e5..ce9b0e880dee 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -74,6 +74,14 @@ def setUp(self): self.lib2_html_block = self._add_block_to_library( self.lib2.library_key, "html", "html2", ) + self.unit = self._create_container(self.lib1.library_key, "unit", display_name="Unit 1", slug=None) + self.subsection = self._create_container( + self.lib1.library_key, + "subsection", + display_name="Subsection 1", + slug=None, + ) + self.section = self._create_container(self.lib1.library_key, "section", display_name="Section 1", slug=None) def test_get_library_collection(self): """ @@ -407,6 +415,43 @@ def test_update_components(self): assert resp.status_code == 200 assert resp.data == {"count": 1} + def test_update_containers(self): + """ + Test adding and removing containers from a collection. + """ + # Add containers to col1 + resp = self.client.patch( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_key=self.col1.key, + ), + data={ + "usage_keys": [ + self.unit["id"], + self.subsection["id"], + self.section["id"], + ] + } + ) + assert resp.status_code == 200 + assert resp.data == {"count": 3} + + # Remove one of the added containers from col1 + resp = self.client.delete( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_key=self.col1.key, + ), + data={ + "usage_keys": [ + self.unit["id"], + self.subsection["id"], + ] + } + ) + assert resp.status_code == 200 + assert resp.data == {"count": 2} + @ddt.data("patch", "delete") def test_update_components_wrong_collection(self, method): """ From c1a0c08cf8951e6363662cb5f2f829df082ea370 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 27 May 2025 14:37:40 -0500 Subject: [PATCH 10/13] style: fix broken tests --- .../v2/views/tests/test_downstream_sync_integration.py | 4 ++-- .../core/djangoapps/content/search/tests/test_documents.py | 2 +- openedx/core/djangoapps/content_libraries/api/containers.py | 4 ++-- 3 files changed, 5 insertions(+), 5 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 1541a0e5e0ba..b7890f821dcc 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 @@ -44,7 +44,7 @@ def setUp(self): 'This is the HTML.' ) self.upstream_unit = self._create_container(lib_id, "unit", slug="u1", display_name="Unit 1 Title") - self._add_container_components(self.upstream_unit["id"], [ + self._add_container_children(self.upstream_unit["id"], [ self.upstream_html1["id"], self.upstream_problem1["id"], self.upstream_problem2["id"], @@ -362,7 +362,7 @@ def test_unit_sync(self): upstream_problem3["id"], 'single select...' ) - self._add_container_components(self.upstream_unit["id"], [upstream_problem3["id"]]) + self._add_container_children(self.upstream_unit["id"], [upstream_problem3["id"]]) self._remove_container_components(self.upstream_unit["id"], [self.upstream_problem2["id"]]) self._commit_library_changes(self.library["id"]) # publish everything diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 8d019ab31c53..0ff45fa53e9b 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -544,7 +544,7 @@ def test_collection_with_published_library(self): ("section", "section1", "section", "79ee8fa2"), ) @ddt.unpack - def test_draft_container_1(self, container_name, container_slug, container_type, doc_id): + def test_draft_container(self, container_name, container_slug, container_type, doc_id): """ Test creating a search document for a draft-only container """ diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index e8dbdbaad9ee..dc03611a1279 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -279,7 +279,7 @@ def create_container( created_by=user_id, ) case _: - raise NotImplementedError(f"Library support for {container_type} is in progress") + raise NotImplementedError(f"Library does not support {container_type} yet") LIBRARY_CONTAINER_CREATED.send_event( library_container=LibraryContainerData( @@ -329,7 +329,7 @@ def update_container( created_by=user_id, ) case _: - raise NotImplementedError(f"Library support for {container_type} is in progress") + raise NotImplementedError(f"Library does not support {container_type} yet") LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( From cbd5c84b826f3aeab9d902c0a8bd600d85ddd7d4 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 28 May 2025 15:00:42 -0500 Subject: [PATCH 11/13] refactor: Add type verification to tests * Add `-> None` to tests * Add `-> list[str]` to `get_child_keys` --- .../djangoapps/content/search/documents.py | 2 +- .../content/search/tests/test_api.py | 52 ++++---- .../content_libraries/api/blocks.py | 2 +- .../content_libraries/api/containers.py | 2 +- .../content_libraries/tests/test_api.py | 115 +++++++++--------- .../tests/test_containers.py | 28 ++--- 6 files changed, 102 insertions(+), 99 deletions(-) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 29b3673a0172..34a3c86b375e 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -661,7 +661,7 @@ def searchable_doc_for_container( container_type = lib_api.ContainerType(container_key.container_type) - def get_child_keys(children): + def get_child_keys(children) -> list[str]: match container_type: case lib_api.ContainerType.Unit: return [ diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 2a1aced68601..3b22ce389d0f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -47,7 +47,7 @@ class TestSearchApi(ModuleStoreTestCase): MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - def setUp(self): + def setUp(self) -> None: # pylint: disable=too-many-statements super().setUp() self.user = UserFactory.create() @@ -309,14 +309,14 @@ def setUp(self): } @override_settings(MEILISEARCH_ENABLED=False) - def test_reindex_meilisearch_disabled(self, mock_meilisearch): + def test_reindex_meilisearch_disabled(self, mock_meilisearch) -> None: with self.assertRaises(RuntimeError): api.rebuild_index() mock_meilisearch.return_value.swap_indexes.assert_not_called() @override_settings(MEILISEARCH_ENABLED=True) - def test_reindex_meilisearch(self, mock_meilisearch): + def test_reindex_meilisearch(self, mock_meilisearch) -> None: # Add tags field to doc, since reindex calls includes tags doc_sequential = copy.deepcopy(self.doc_sequential) @@ -356,7 +356,7 @@ def test_reindex_meilisearch(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_reindex_meilisearch_incremental(self, mock_meilisearch): + def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None: # Add tags field to doc, since reindex calls includes tags doc_sequential = copy.deepcopy(self.doc_sequential) @@ -413,7 +413,7 @@ def simulated_interruption(message): assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 8 @override_settings(MEILISEARCH_ENABLED=True) - def test_reset_meilisearch_index(self, mock_meilisearch): + def test_reset_meilisearch_index(self, mock_meilisearch) -> None: api.reset_index() mock_meilisearch.return_value.swap_indexes.assert_called_once() mock_meilisearch.return_value.create_index.assert_called_once() @@ -422,7 +422,7 @@ def test_reset_meilisearch_index(self, mock_meilisearch): mock_meilisearch.return_value.delete_index.call_count = 4 @override_settings(MEILISEARCH_ENABLED=True) - def test_init_meilisearch_index(self, mock_meilisearch): + def test_init_meilisearch_index(self, mock_meilisearch) -> None: # Test index already exists api.init_index() mock_meilisearch.return_value.swap_indexes.assert_not_called() @@ -453,7 +453,7 @@ def test_init_meilisearch_index(self, mock_meilisearch): "openedx.core.djangoapps.content.search.api.searchable_doc_for_collection", Mock(side_effect=Exception("Failed to generate document")), ) - def test_reindex_meilisearch_collection_error(self, mock_meilisearch): + def test_reindex_meilisearch_collection_error(self, mock_meilisearch) -> None: mock_logger = Mock() api.rebuild_index(mock_logger) @@ -469,7 +469,7 @@ def test_reindex_meilisearch_collection_error(self, mock_meilisearch): "openedx.core.djangoapps.content.search.api.searchable_doc_for_container", Mock(side_effect=Exception("Failed to generate document")), ) - def test_reindex_meilisearch_container_error(self, mock_meilisearch): + def test_reindex_meilisearch_container_error(self, mock_meilisearch) -> None: mock_logger = Mock() api.rebuild_index(mock_logger) @@ -481,7 +481,7 @@ def test_reindex_meilisearch_container_error(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_reindex_meilisearch_library_block_error(self, mock_meilisearch): + def test_reindex_meilisearch_library_block_error(self, mock_meilisearch) -> None: # Add tags field to doc, since reindex calls includes tags doc_sequential = copy.deepcopy(self.doc_sequential) @@ -539,7 +539,7 @@ def mocked_from_component(lib_key, component): False ) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_xblock_metadata(self, recursive, mock_meilisearch): + def test_index_xblock_metadata(self, recursive, mock_meilisearch) -> None: """ Test indexing an XBlock. """ @@ -553,13 +553,13 @@ def test_index_xblock_metadata(self, recursive, mock_meilisearch): mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with(expected_docs) @override_settings(MEILISEARCH_ENABLED=True) - def test_no_index_excluded_xblocks(self, mock_meilisearch): + def test_no_index_excluded_xblocks(self, mock_meilisearch) -> None: api.upsert_xblock_index_doc(UsageKey.from_string(self.course_block_key)) mock_meilisearch.return_value.index.return_value.update_document.assert_not_called() @override_settings(MEILISEARCH_ENABLED=True) - def test_index_xblock_tags(self, mock_meilisearch): + def test_index_xblock_tags(self, mock_meilisearch) -> None: """ Test indexing an XBlock with tags. """ @@ -593,7 +593,7 @@ def test_index_xblock_tags(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_delete_index_xblock(self, mock_meilisearch): + def test_delete_index_xblock(self, mock_meilisearch) -> None: """ Test deleting an XBlock doc from the index. """ @@ -604,7 +604,7 @@ def test_delete_index_xblock(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_library_block_metadata(self, mock_meilisearch): + def test_index_library_block_metadata(self, mock_meilisearch) -> None: """ Test indexing a Library Block. """ @@ -613,7 +613,7 @@ def test_index_library_block_metadata(self, mock_meilisearch): mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.doc_problem1]) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_library_block_tags(self, mock_meilisearch): + def test_index_library_block_tags(self, mock_meilisearch) -> None: """ Test indexing an Library Block with tags. """ @@ -648,7 +648,7 @@ def test_index_library_block_tags(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_library_block_and_collections(self, mock_meilisearch): + def test_index_library_block_and_collections(self, mock_meilisearch) -> None: """ Test indexing an Library Block and the Collections it's in. """ @@ -788,7 +788,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_delete_index_library_block(self, mock_meilisearch): + def test_delete_index_library_block(self, mock_meilisearch) -> None: """ Test deleting a Library Block doc from the index. """ @@ -799,7 +799,7 @@ def test_delete_index_library_block(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_content_library_metadata(self, mock_meilisearch): + def test_index_content_library_metadata(self, mock_meilisearch) -> None: """ Test indexing a whole content library. """ @@ -810,7 +810,7 @@ def test_index_content_library_metadata(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_tags_in_collections(self, mock_meilisearch): + def test_index_tags_in_collections(self, mock_meilisearch) -> None: # Tag collection tagging_api.tag_object(str(self.collection_key), self.taxonomyA, ["one", "two"]) tagging_api.tag_object(str(self.collection_key), self.taxonomyB, ["three", "four"]) @@ -841,7 +841,7 @@ def test_index_tags_in_collections(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_delete_collection(self, mock_meilisearch): + def test_delete_collection(self, mock_meilisearch) -> None: """ Test soft-deleting, restoring, and hard-deleting a collection. """ @@ -970,7 +970,7 @@ def test_delete_collection(self, mock_meilisearch): "section", ) @override_settings(MEILISEARCH_ENABLED=True) - def test_delete_index_container(self, container_type, mock_meilisearch): + def test_delete_index_container(self, container_type, mock_meilisearch) -> None: """ Test delete a container index. """ @@ -984,7 +984,7 @@ def test_delete_index_container(self, container_type, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_library_container_metadata(self, mock_meilisearch): + def test_index_library_container_metadata(self, mock_meilisearch) -> None: """ Test indexing a Library Container. """ @@ -999,7 +999,7 @@ def test_index_library_container_metadata(self, mock_meilisearch): ) @ddt.unpack @override_settings(MEILISEARCH_ENABLED=True) - def test_index_tags_in_containers(self, container_type, container_id, mock_meilisearch): + def test_index_tags_in_containers(self, container_type, container_id, mock_meilisearch) -> None: container_key = getattr(self, f"{container_type}_key") # Tag container @@ -1032,7 +1032,7 @@ def test_index_tags_in_containers(self, container_type, container_id, mock_meili ) @override_settings(MEILISEARCH_ENABLED=True) - def test_block_in_units(self, mock_meilisearch): + def test_block_in_units(self, mock_meilisearch) -> None: with freeze_time(self.created_date): library_api.update_container_children( LibraryContainerLocator.from_string(self.unit_key), @@ -1063,7 +1063,7 @@ def test_block_in_units(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_units_in_subsection(self, mock_meilisearch): + def test_units_in_subsection(self, mock_meilisearch) -> None: with freeze_time(self.created_date): library_api.update_container_children( LibraryContainerLocator.from_string(self.subsection_key), @@ -1087,7 +1087,7 @@ def test_units_in_subsection(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_section_in_usbsections(self, mock_meilisearch): + def test_section_in_usbsections(self, mock_meilisearch) -> None: with freeze_time(self.created_date): library_api.update_container_children( LibraryContainerLocator.from_string(self.section_key), diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index dd67095597a5..3f6867e72daa 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -493,7 +493,7 @@ def _import_staged_block_as_container( title=title, user_id=user.id, ) - new_child_keys: list[UsageKeyV2] = [] + new_child_keys: list[LibraryUsageLocatorV2] = [] for child_node in olx_node: try: child_metadata = _import_staged_block( diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index dc03611a1279..ea9864530ce0 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -474,7 +474,7 @@ def get_container_children_count( def update_container_children( container_key: LibraryContainerLocator, - children_ids: list[UsageKeyV2] | list[LibraryContainerLocator], + children_ids: list[LibraryUsageLocatorV2] | list[LibraryContainerLocator], user_id: int | None, entities_action: authoring_api.ChildrenEntitiesAction = authoring_api.ChildrenEntitiesAction.REPLACE, ): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 085f69d0a381..c965a4da3fa5 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -11,8 +11,9 @@ from opaque_keys.edx.keys import ( CourseKey, UsageKey, + UsageKeyV2, ) -from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_events.content_authoring.data import ( ContentObjectChangedData, LibraryCollectionData, @@ -265,7 +266,7 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest): Same guidelines as ContentLibrariesTestCase. """ - def setUp(self): + def setUp(self) -> None: super().setUp() # Create Content Libraries @@ -317,7 +318,7 @@ def setUp(self): self.lib2.library_key, "problem", "problem2", ) - def test_create_library_collection(self): + def test_create_library_collection(self) -> None: event_receiver = mock.Mock() LIBRARY_COLLECTION_CREATED.connect(event_receiver) @@ -348,7 +349,7 @@ def test_create_library_collection(self): event_receiver.call_args_list[0].kwargs, ) - def test_create_library_collection_invalid_library(self): + def test_create_library_collection_invalid_library(self) -> None: library_key = LibraryLocatorV2.from_string("lib:INVALID:test-lib-does-not-exist") with self.assertRaises(api.ContentLibraryNotFound) as exc: api.create_library_collection( @@ -357,7 +358,7 @@ def test_create_library_collection_invalid_library(self): title="Collection 3", ) - def test_update_library_collection(self): + def test_update_library_collection(self) -> None: event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(event_receiver) @@ -386,17 +387,18 @@ def test_update_library_collection(self): event_receiver.call_args_list[0].kwargs, ) - def test_update_library_collection_wrong_library(self): + def test_update_library_collection_wrong_library(self) -> None: with self.assertRaises(api.ContentLibraryCollectionNotFound) as exc: api.update_library_collection( self.lib1.library_key, self.col2.key, ) - def test_delete_library_collection(self): + def test_delete_library_collection(self) -> None: event_receiver = mock.Mock() LIBRARY_COLLECTION_DELETED.connect(event_receiver) + assert self.lib1.learning_package_id is not None authoring_api.delete_collection( self.lib1.learning_package_id, self.col1.key, @@ -418,15 +420,15 @@ def test_delete_library_collection(self): event_receiver.call_args_list[0].kwargs, ) - def test_update_library_collection_items(self): + def test_update_library_collection_items(self) -> None: assert not list(self.col1.entities.all()) self.col1 = api.update_library_collection_items( self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), LibraryContainerLocator.from_string(self.unit1["id"]), ], ) @@ -436,13 +438,13 @@ def test_update_library_collection_items(self): self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), ], remove=True, ) assert len(self.col1.entities.all()) == 2 - def test_update_library_collection_components_event(self): + def test_update_library_collection_components_event(self) -> None: """ Check that a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event is raised for each added/removed component. """ @@ -454,8 +456,8 @@ def test_update_library_collection_components_event(self): self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), LibraryContainerLocator.from_string(self.unit1["id"]), ], ) @@ -508,32 +510,33 @@ def test_update_library_collection_components_event(self): event_receiver.call_args_list[3].kwargs, ) - def test_update_collection_components_from_wrong_library(self): + def test_update_collection_components_from_wrong_library(self) -> None: with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: api.update_library_collection_items( self.lib2.library_key, self.col2.key, opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), LibraryContainerLocator.from_string(self.unit1["id"]), ], ) assert self.lib1_problem_block["id"] in str(exc.exception) - def test_set_library_component_collections(self): + def test_set_library_component_collections(self) -> None: event_receiver = mock.Mock() CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) assert not list(self.col2.entities.all()) - component = api.get_component_from_usage_key(UsageKey.from_string(self.lib2_problem_block["id"])) + component = api.get_component_from_usage_key(UsageKeyV2.from_string(self.lib2_problem_block["id"])) api.set_library_item_collections( library_key=self.lib2.library_key, entity_key=component.publishable_entity.key, collection_keys=[self.col2.key, self.col3.key], ) - + + assert self.lib2.learning_package_id is not None assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col2.key).entities.all()) == 1 assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col3.key).entities.all()) == 1 assert { @@ -559,20 +562,20 @@ def test_set_library_component_collections(self): ) } - def test_delete_library_block(self): + def test_delete_library_block(self) -> None: api.update_library_collection_items( self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), ], ) event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(event_receiver) - api.delete_library_block(UsageKey.from_string(self.lib1_problem_block["id"])) + api.delete_library_block(LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"])) assert event_receiver.call_count == 1 self.assertDictContainsSubset( @@ -590,13 +593,13 @@ def test_delete_library_block(self): event_receiver.call_args_list[0].kwargs, ) - def test_delete_library_container(self): + def test_delete_library_container(self) -> None: api.update_library_collection_items( self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), LibraryContainerLocator.from_string(self.unit1["id"]), ], ) @@ -622,20 +625,20 @@ def test_delete_library_container(self): event_receiver.call_args_list[0].kwargs, ) - def test_restore_library_block(self): + def test_restore_library_block(self) -> None: api.update_library_collection_items( self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), ], ) event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(event_receiver) - api.restore_library_block(UsageKey.from_string(self.lib1_problem_block["id"])) + api.restore_library_block(LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"])) assert event_receiver.call_count == 1 self.assertDictContainsSubset( @@ -653,7 +656,7 @@ def test_restore_library_block(self): event_receiver.call_args_list[0].kwargs, ) - def test_add_component_and_revert(self): + def test_add_component_and_revert(self) -> None: # Publish changes api.publish_changes(self.lib1.library_key) @@ -667,8 +670,8 @@ def test_add_component_and_revert(self): self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_html_block["id"]), - UsageKey.from_string(new_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), + LibraryUsageLocatorV2.from_string(new_problem_block["id"]), ], ) @@ -689,7 +692,7 @@ def test_add_component_and_revert(self): ), }.items() <= collection_update_event_receiver.call_args_list[0].kwargs.items() - def test_delete_component_and_revert(self): + def test_delete_component_and_revert(self) -> None: """ When a component is deleted and then the delete is reverted, signals will be emitted to update any containing collections. @@ -699,14 +702,14 @@ def test_delete_component_and_revert(self): self.lib1.library_key, self.col1.key, opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]) + LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]) ], ) api.publish_changes(self.lib1.library_key) # Delete component and revert - api.delete_library_block(UsageKey.from_string(self.lib1_problem_block["id"])) + api.delete_library_block(LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"])) collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) @@ -731,7 +734,7 @@ class ContentLibraryContainersTest(ContentLibrariesRestApiTest): Tests for Content Library API containers methods. """ - def setUp(self): + def setUp(self) -> None: super().setUp() # Create Content Libraries @@ -749,11 +752,11 @@ def setUp(self): self.problem_block = self._add_block_to_library( self.lib1.library_key, "problem", "problem1", ) - self.problem_block_usage_key = UsageKey.from_string(self.problem_block["id"]) + self.problem_block_usage_key = LibraryUsageLocatorV2.from_string(self.problem_block["id"]) self.html_block = self._add_block_to_library( self.lib1.library_key, "html", "html1", ) - self.html_block_usage_key = UsageKey.from_string(self.html_block["id"]) + self.html_block_usage_key = LibraryUsageLocatorV2.from_string(self.html_block["id"]) # Add content to units api.update_container_children( @@ -767,7 +770,7 @@ def setUp(self): None, ) - def test_get_containers_contains_component(self): + def test_get_containers_contains_component(self) -> None: problem_block_containers = api.get_containers_contains_component(self.problem_block_usage_key) html_block_containers = api.get_containers_contains_component(self.html_block_usage_key) @@ -778,7 +781,7 @@ def test_get_containers_contains_component(self): assert html_block_containers[0].container_key == self.unit1.container_key assert html_block_containers[1].container_key == self.unit2.container_key - def _validate_calls_of_html_block(self, event_mock): + def _validate_calls_of_html_block(self, event_mock) -> None: """ Validate that the `event_mock` has been called twice using the `LIBRARY_CONTAINER_UPDATED` signal. @@ -807,14 +810,14 @@ def _validate_calls_of_html_block(self, event_mock): event_mock.call_args_list[1].kwargs, ) - def test_call_container_update_signal_when_delete_component(self): + def test_call_container_update_signal_when_delete_component(self) -> None: container_update_event_receiver = mock.Mock() LIBRARY_CONTAINER_UPDATED.connect(container_update_event_receiver) api.delete_library_block(self.html_block_usage_key) self._validate_calls_of_html_block(container_update_event_receiver) - def test_call_container_update_signal_when_restore_component(self): + def test_call_container_update_signal_when_restore_component(self) -> None: api.delete_library_block(self.html_block_usage_key) container_update_event_receiver = mock.Mock() @@ -823,7 +826,7 @@ def test_call_container_update_signal_when_restore_component(self): self._validate_calls_of_html_block(container_update_event_receiver) - def test_call_container_update_signal_when_update_olx(self): + def test_call_container_update_signal_when_update_olx(self) -> None: block_olx = "Hello world!" container_update_event_receiver = mock.Mock() LIBRARY_CONTAINER_UPDATED.connect(container_update_event_receiver) @@ -831,7 +834,7 @@ def test_call_container_update_signal_when_update_olx(self): self._set_library_block_olx(self.html_block_usage_key, block_olx) self._validate_calls_of_html_block(container_update_event_receiver) - def test_call_container_update_signal_when_update_component(self): + def test_call_container_update_signal_when_update_component(self) -> None: block_olx = "Hello world!" container_update_event_receiver = mock.Mock() LIBRARY_CONTAINER_UPDATED.connect(container_update_event_receiver) @@ -839,13 +842,13 @@ def test_call_container_update_signal_when_update_component(self): self._set_library_block_fields(self.html_block_usage_key, {"data": block_olx, "metadata": {}}) self._validate_calls_of_html_block(container_update_event_receiver) - def test_call_object_changed_signal_when_remove_component(self): + def test_call_object_changed_signal_when_remove_component(self) -> None: html_block_1 = self._add_block_to_library( self.lib1.library_key, "html", "html3", ) api.update_container_children( self.unit2.container_key, - [UsageKey.from_string(html_block_1["id"])], + [LibraryUsageLocatorV2.from_string(html_block_1["id"])], None, entities_action=authoring_api.ChildrenEntitiesAction.APPEND, ) @@ -854,7 +857,7 @@ def test_call_object_changed_signal_when_remove_component(self): CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_reciver) api.update_container_children( self.unit2.container_key, - [UsageKey.from_string(html_block_1["id"])], + [LibraryUsageLocatorV2.from_string(html_block_1["id"])], None, entities_action=authoring_api.ChildrenEntitiesAction.REMOVE, ) @@ -872,7 +875,7 @@ def test_call_object_changed_signal_when_remove_component(self): event_reciver.call_args_list[0].kwargs, ) - def test_call_object_changed_signal_when_add_component(self): + def test_call_object_changed_signal_when_add_component(self) -> None: event_reciver = mock.Mock() CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_reciver) html_block_1 = self._add_block_to_library( @@ -885,8 +888,8 @@ def test_call_object_changed_signal_when_add_component(self): api.update_container_children( self.unit2.container_key, [ - UsageKey.from_string(html_block_1["id"]), - UsageKey.from_string(html_block_2["id"]) + LibraryUsageLocatorV2.from_string(html_block_1["id"]), + LibraryUsageLocatorV2.from_string(html_block_2["id"]) ], None, entities_action=authoring_api.ChildrenEntitiesAction.APPEND, @@ -916,19 +919,19 @@ def test_call_object_changed_signal_when_add_component(self): event_reciver.call_args_list[1].kwargs, ) - def test_delete_component_and_revert(self): + def test_delete_component_and_revert(self) -> None: """ When a component is deleted and then the delete is reverted, signals will be emitted to update any containing containers. """ # Add components and publish api.update_container_children(self.unit1.container_key, [ - UsageKey.from_string(self.problem_block["id"]), + LibraryUsageLocatorV2.from_string(self.problem_block["id"]), ], user_id=None) api.publish_changes(self.lib1.library_key) # Delete component and revert - api.delete_library_block(UsageKey.from_string(self.problem_block["id"])) + api.delete_library_block(LibraryUsageLocatorV2.from_string(self.problem_block["id"])) container_event_receiver = mock.Mock() LIBRARY_CONTAINER_UPDATED.connect(container_event_receiver) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index e0fea5c861d4..26196d8394d8 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -35,7 +35,7 @@ class ContainersTestCase(ContentLibrariesRestApiTest): break any tests, but backwards-incompatible API changes will. """ - def setUp(self): + def setUp(self) -> None: super().setUp() self.create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) self.lib = self._create_library( @@ -136,7 +136,7 @@ def setUp(self): ("section", "s1", "Test Section"), ) @ddt.unpack - def test_container_crud(self, container_type, slug, display_name): + def test_container_crud(self, container_type, slug, display_name) -> None: """ Test Create, Read, Update, and Delete of a Containers """ @@ -194,7 +194,7 @@ def test_container_crud(self, container_type, slug, display_name): ("section", "s2", "Test Section"), ) @ddt.unpack - def test_container_permissions(self, container_type, slug, display_name): + def test_container_permissions(self, container_type, slug, display_name) -> None: """ Test that a regular user with read-only permissions on the library cannot create, update, or delete containers. """ @@ -233,7 +233,7 @@ def test_container_permissions(self, container_type, slug, display_name): ("section", "Section Alpha", "lct:CL-TEST:containers:section:section-alpha-"), ) @ddt.unpack - def test_containers_gets_auto_slugs(self, container_type, display_name, expected_id): + def test_containers_gets_auto_slugs(self, container_type, display_name, expected_id) -> None: """ Test that we can create containers by specifying only a title, and they get unique slugs assigned automatically. @@ -245,7 +245,7 @@ def test_containers_gets_auto_slugs(self, container_type, display_name, expected assert container_2["id"].startswith(expected_id) assert container_1["id"] != container_2["id"] - def test_unit_add_children(self): + def test_unit_add_children(self) -> None: """ Test that we can add and get unit children components """ @@ -275,7 +275,7 @@ def test_unit_add_children(self): assert data[3]['id'] == html_block_2['id'] assert data[3]['can_stand_alone'] - def test_subsection_add_children(self): + def test_subsection_add_children(self) -> None: # Create units child_unit_1 = self._create_container(self.lib["id"], "unit", display_name="Child unit 1", slug=None) child_unit_2 = self._create_container(self.lib["id"], "unit", display_name="Child unit 2", slug=None) @@ -304,7 +304,7 @@ def test_subsection_add_children(self): assert data[2]['id'] == child_unit_3['id'] assert data[3]['id'] == child_unit_4['id'] - def test_section_add_children(self): + def test_section_add_children(self) -> None: # Create Subsections child_subsection_1 = self._create_container( self.lib["id"], @@ -359,7 +359,7 @@ def test_section_add_children(self): ("section_with_subsections", ["subsection", "subsection_with_units"], ["subsection_2", "subsection_3"]), ) @ddt.unpack - def test_container_remove_children(self, container_name, items_to_remove, expected_items): + def test_container_remove_children(self, container_name, items_to_remove, expected_items) -> None: """ Test that we can remove container children """ @@ -380,7 +380,7 @@ def test_container_remove_children(self, container_name, items_to_remove, expect assert data[0]['id'] == expected_item_1['id'] assert data[1]['id'] == expected_item_2['id'] - def test_unit_replace_children(self): + def test_unit_replace_children(self) -> None: """ Test that we can completely replace/reorder unit children components. """ @@ -420,7 +420,7 @@ def test_unit_replace_children(self): assert data[0]['id'] == new_problem_block['id'] assert data[1]['id'] == new_html_block['id'] - def test_subsection_replace_children(self): + def test_subsection_replace_children(self) -> None: """ Test that we can completely replace/reorder subsection children. """ @@ -460,7 +460,7 @@ def test_subsection_replace_children(self): assert data[0]['id'] == new_unit_1['id'] assert data[1]['id'] == new_unit_2['id'] - def test_section_replace_children(self): + def test_section_replace_children(self) -> None: """ Test that we can completely replace/reorder section children. """ @@ -515,7 +515,7 @@ def test_section_replace_children(self): "subsection", "section", ) - def test_restore_containers(self, container_type): + def test_restore_containers(self, container_type) -> None: """ Test restore a deleted container. """ @@ -543,7 +543,7 @@ def test_restore_containers(self, container_type): self.assertDictContainsEntries(new_container_data, expected_data) - def test_container_collections(self): + def test_container_collections(self) -> None: # Create a collection col1 = api.create_library_collection( self.lib_key, @@ -566,7 +566,7 @@ def test_container_collections(self): # Verify the collections assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] - def test_publish_container(self): # pylint: disable=too-many-statements + def test_publish_container(self) -> None: # pylint: disable=too-many-statements """ Test that we can publish the changes to a specific container """ From 02e70f04bad33e95bdcf1c4370b9638184d9d334 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 28 May 2025 15:12:18 -0500 Subject: [PATCH 12/13] refactor: Delete unnecesary checks in update_container_children --- .../content_libraries/api/containers.py | 27 ++----------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index ea9864530ce0..c5ddfb944ea0 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -10,7 +10,6 @@ from uuid import uuid4 from django.utils.text import slugify -from opaque_keys.edx.keys import UsageKeyV2 from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_events.content_authoring.data import ( ContentObjectChangedData, @@ -505,18 +504,7 @@ def update_container_children( ), ) case ContainerType.Subsection: - units = [] - for key in children_ids: - # Verify that all children are units - if ( - not isinstance(key, LibraryContainerLocator) - or not key.container_type == ContainerType.Unit.value - ): - raise ValueError( - f"Invalid children type: {key}. All Subsection children must be Units", - ) - units.append(_get_container_from_key(key).unit) - + units = [_get_container_from_key(key).unit for key in children_ids] new_version = authoring_api.create_next_subsection_version( container.subsection, units=units, # type: ignore[arg-type] @@ -527,18 +515,7 @@ def update_container_children( # TODO add CONTENT_OBJECT_ASSOCIATIONS_CHANGED for subsections case ContainerType.Section: - subsections = [] - for key in children_ids: - # Verify that all children are subsections - if ( - not isinstance(key, LibraryContainerLocator) - or not key.container_type == ContainerType.Subsection.value - ): - raise ValueError( - f"Invalid children type: {key}. All Section children must be Subsections", - ) - subsections.append(_get_container_from_key(key).subsection) - + subsections = [_get_container_from_key(key).subsection for key in children_ids] new_version = authoring_api.create_next_section_version( container.section, subsections=subsections, # type: ignore[arg-type] From b55ccc0eaa9a01a6b9bbf3358edaea8c9d73b053 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 28 May 2025 15:25:25 -0500 Subject: [PATCH 13/13] style: Fix broken lint --- openedx/core/djangoapps/content_libraries/api/containers.py | 4 ++-- openedx/core/djangoapps/content_libraries/tests/test_api.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index c5ddfb944ea0..8e2e486377da 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -504,7 +504,7 @@ def update_container_children( ), ) case ContainerType.Subsection: - units = [_get_container_from_key(key).unit for key in children_ids] + units = [_get_container_from_key(key).unit for key in children_ids] # type: ignore[arg-type] new_version = authoring_api.create_next_subsection_version( container.subsection, units=units, # type: ignore[arg-type] @@ -515,7 +515,7 @@ def update_container_children( # TODO add CONTENT_OBJECT_ASSOCIATIONS_CHANGED for subsections case ContainerType.Section: - subsections = [_get_container_from_key(key).subsection for key in children_ids] + subsections = [_get_container_from_key(key).subsection for key in children_ids] # type: ignore[arg-type] new_version = authoring_api.create_next_section_version( container.section, subsections=subsections, # type: ignore[arg-type] diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index c965a4da3fa5..9517e5d0d2f6 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -535,7 +535,7 @@ def test_set_library_component_collections(self) -> None: entity_key=component.publishable_entity.key, collection_keys=[self.col2.key, self.col3.key], ) - + assert self.lib2.learning_package_id is not None assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col2.key).entities.all()) == 1 assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col3.key).entities.all()) == 1