diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index b562913baadc..5b24b6540b10 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -16,24 +16,33 @@ from openedx_events.content_authoring.data import LibraryContainerData from openedx_events.content_authoring.signals import ( LIBRARY_CONTAINER_CREATED, + LIBRARY_CONTAINER_DELETED, + LIBRARY_CONTAINER_UPDATED, ) from openedx_learning.api import authoring as authoring_api -from openedx_learning.api import authoring_models +from openedx_learning.api.authoring_models import Container from ..models import ContentLibrary from .libraries import PublishableItem + # The public API is only the following symbols: __all__ = [ + "ContentLibraryContainerNotFound", "ContainerMetadata", "ContainerType", "get_container", "create_container", "get_container_children", "library_container_locator", + "update_container", + "delete_container", ] +ContentLibraryContainerNotFound = Container.DoesNotExist + + class ContainerType(Enum): Unit = "unit" @@ -47,7 +56,7 @@ class ContainerMetadata(PublishableItem): container_type: ContainerType @classmethod - def from_container(cls, library_key, container: authoring_models.Container, associated_collections=None): + def from_container(cls, library_key, container: Container, associated_collections=None): """ Construct a ContainerMetadata object from a Container object. """ @@ -89,7 +98,7 @@ def from_container(cls, library_key, container: authoring_models.Container, asso def library_container_locator( library_key: LibraryLocatorV2, - container: authoring_models.Container, + container: Container, ) -> LibraryContainerLocator: """ Returns a LibraryContainerLocator for the given library + container. @@ -106,9 +115,11 @@ def library_container_locator( ) -def get_container(container_key: LibraryContainerLocator) -> ContainerMetadata: +def _get_container(container_key: LibraryContainerLocator) -> Container: """ - Get a container (a Section, Subsection, or Unit). + Internal method to fetch the Container object from its LibraryContainerLocator + + Raises ContentLibraryContainerNotFound if no container found, or if the container has been soft deleted. """ assert isinstance(container_key, LibraryContainerLocator) content_library = ContentLibrary.objects.get_by_key(container_key.library_key) @@ -118,6 +129,16 @@ def get_container(container_key: LibraryContainerLocator) -> ContainerMetadata: learning_package.id, key=container_key.container_id, ) + if container and container.versioning.draft: + return container + raise ContentLibraryContainerNotFound + + +def get_container(container_key: LibraryContainerLocator) -> ContainerMetadata: + """ + Get a container (a Section, Subsection, or Unit). + """ + container = _get_container(container_key) container_meta = ContainerMetadata.from_container(container_key.library_key, container) assert container_meta.container_type.value == container_key.container_type return container_meta @@ -170,6 +191,60 @@ def create_container( return ContainerMetadata.from_container(library_key, container) +def update_container( + container_key: LibraryContainerLocator, + display_name: str, + user_id: int | None, +) -> ContainerMetadata: + """ + Update a container (e.g. a Unit) title. + """ + container = _get_container(container_key) + library_key = container_key.library_key + + assert container.unit + unit_version = authoring_api.create_next_unit_version( + container.unit, + title=display_name, + created=datetime.now(), + created_by=user_id, + ) + + LIBRARY_CONTAINER_UPDATED.send_event( + library_container=LibraryContainerData( + library_key=library_key, + container_key=str(container_key), + ) + ) + + return ContainerMetadata.from_container(library_key, unit_version.container) + + +def delete_container( + container_key: LibraryContainerLocator, +) -> None: + """ + Delete a container (e.g. a Unit) (soft delete). + + No-op if container doesn't exist or has already been soft-deleted. + """ + try: + container = _get_container(container_key) + except ContentLibraryContainerNotFound: + return + + authoring_api.soft_delete_draft(container.pk) + + LIBRARY_CONTAINER_DELETED.send_event( + library_container=LibraryContainerData( + library_key=container_key.library_key, + container_key=str(container_key), + ) + ) + + # TODO: trigger a LIBRARY_COLLECTION_UPDATED for each collection the container was in + + def get_container_children( container_key: LibraryContainerLocator, published=False, diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index fd89ceb0d3fc..13d41921e860 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -97,7 +97,6 @@ from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import ( Collection, - Container, Component, ComponentVersion, MediaType, @@ -128,7 +127,6 @@ # Exceptions - maybe move them to a new file? "ContentLibraryNotFound", "ContentLibraryCollectionNotFound", - "ContentLibraryContainerNotFound", "ContentLibraryBlockNotFound", "LibraryAlreadyExists", "LibraryCollectionAlreadyExists", @@ -181,8 +179,6 @@ ContentLibraryCollectionNotFound = Collection.DoesNotExist -ContentLibraryContainerNotFound = Container.DoesNotExist - class ContentLibraryBlockNotFound(XBlockNotFoundError): """ XBlock not found in the content library """ diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index fd96315391cd..ad23a51d55b3 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -14,6 +14,7 @@ from opaque_keys.edx.locator import LibraryLocatorV2, LibraryContainerLocator from rest_framework.generics import GenericAPIView from rest_framework.response import Response +from rest_framework.status import HTTP_204_NO_CONTENT from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.lib.api.view_utils import view_auth_classes @@ -62,7 +63,7 @@ def post(self, request, lib_key_str): @view_auth_classes() class LibraryContainerView(GenericAPIView): """ - View to get data about a specific container (a section, subsection, or unit) + View to retrieve or update data about a specific container (a section, subsection, or unit) """ serializer_class = serializers.LibraryContainerMetadataSerializer @@ -81,3 +82,45 @@ def get(self, request, container_key: LibraryContainerLocator): ) container = api.get_container(container_key) return Response(serializers.LibraryContainerMetadataSerializer(container).data) + + @convert_exceptions + @swagger_auto_schema( + request_body=serializers.LibraryContainerUpdateSerializer, + responses={200: serializers.LibraryContainerMetadataSerializer} + ) + def patch(self, request, container_key: LibraryContainerLocator): + """ + Update a Container. + """ + api.require_permission_for_library_key( + container_key.library_key, + request.user, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + ) + serializer = serializers.LibraryContainerUpdateSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + + container = api.update_container( + container_key, + display_name=serializer.validated_data['display_name'], + user_id=request.user.id, + ) + + return Response(serializers.LibraryContainerMetadataSerializer(container).data) + + @convert_exceptions + def delete(self, request, container_key: LibraryContainerLocator): + """ + Delete a Container (soft delete). + """ + api.require_permission_for_library_key( + container_key.library_key, + request.user, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + ) + + api.delete_container( + container_key, + ) + + return Response({}, status=HTTP_204_NO_CONTENT) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index cac8fddd79ad..67b8ec00e56f 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -270,6 +270,13 @@ def to_internal_value(self, data): return result +class LibraryContainerUpdateSerializer(serializers.Serializer): + """ + Serializer for updating metadata for Containers like Sections, Subsections, Units + """ + display_name = serializers.CharField() + + class ContentLibraryBlockImportTaskSerializer(serializers.ModelSerializer): """ Serializer for a Content Library block import task. diff --git a/openedx/core/djangoapps/content_libraries/rest_api/utils.py b/openedx/core/djangoapps/content_libraries/rest_api/utils.py index 53b5dd0b4059..99825fbe75f8 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/utils.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/utils.py @@ -34,6 +34,9 @@ def wrapped_fn(*args, **kwargs): except api.ContentLibraryCollectionNotFound: log.exception("Collection not found in content library") raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.ContentLibraryContainerNotFound: + log.exception("Container not found in content library") + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from except api.LibraryCollectionAlreadyExists as exc: log.exception(str(exc)) raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index e4b7b92e088f..d8030afae7f0 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -363,3 +363,12 @@ def _create_container(self, lib_key, container_type, slug: str | None, display_n def _get_container(self, container_key: str, expect_response=200): """ Get a container (unit etc.) """ return self._api('get', URL_LIB_CONTAINER.format(container_key=container_key), None, expect_response) + + def _update_container(self, container_key: str, display_name: str, expect_response=200): + """ Update a container (unit etc.) """ + data = {"display_name": display_name} + return self._api('patch', URL_LIB_CONTAINER.format(container_key=container_key), data, expect_response) + + def _delete_container(self, container_key: str, expect_response=204): + """ Delete a container (unit etc.) """ + return self._api('delete', URL_LIB_CONTAINER.format(container_key=container_key), None, expect_response) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 39479daffb3a..23a519899a85 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -9,9 +9,14 @@ from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_events.content_authoring.data import LibraryContainerData -from openedx_events.content_authoring.signals import LIBRARY_CONTAINER_CREATED +from openedx_events.content_authoring.signals import ( + LIBRARY_CONTAINER_CREATED, + LIBRARY_CONTAINER_DELETED, + LIBRARY_CONTAINER_UPDATED, +) from openedx_events.tests.utils import OpenEdxEventsTestMixin +from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -38,6 +43,8 @@ class ContainersTestCase(OpenEdxEventsTestMixin, ContentLibrariesRestApiTest): """ ENABLED_OPENEDX_EVENTS = [ LIBRARY_CONTAINER_CREATED.event_type, + LIBRARY_CONTAINER_DELETED.event_type, + LIBRARY_CONTAINER_UPDATED.event_type, ] def test_unit_crud(self): @@ -50,6 +57,12 @@ def test_unit_crud(self): create_receiver = mock.Mock() LIBRARY_CONTAINER_CREATED.connect(create_receiver) + update_receiver = mock.Mock() + LIBRARY_CONTAINER_UPDATED.connect(update_receiver) + + delete_receiver = mock.Mock() + LIBRARY_CONTAINER_DELETED.connect(delete_receiver) + # Create a unit: create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) with freeze_time(create_date): @@ -87,7 +100,69 @@ def test_unit_crud(self): # make sure it contains the same data when we read it back: self.assertDictContainsEntries(unit_as_read, expected_data) - # TODO: test that a regular user with read-only permissions on the library cannot create units + # 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) + + assert update_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_CONTAINER_UPDATED, + "sender": None, + "library_container": LibraryContainerData( + lib_key, + container_key="lct:CL-TEST:containers:unit:u1", + ), + }, + update_receiver.call_args_list[0].kwargs, + ) + + # Re-fetch the unit + unit_as_re_read = self._get_container(container_data["container_key"]) + # 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["container_key"]) + self._get_container(container_data["container_key"], expect_response=404) + assert delete_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_CONTAINER_DELETED, + "sender": None, + "library_container": LibraryContainerData( + lib_key, + container_key="lct:CL-TEST:containers:unit:u1", + ), + }, + delete_receiver.call_args_list[0].kwargs, + ) + + 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") + + 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._get_container(container_data["container_key"], expect_response=403) + self._update_container(container_data["container_key"], display_name="Unit ABC", expect_response=403) + self._delete_container(container_data["container_key"], 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") + with self.as_user(random_user): + self._create_container(lib["id"], "unit", slug="u2", display_name="Test Unit", expect_response=403) + self._get_container(container_data["container_key"], expect_response=200) + self._update_container(container_data["container_key"], display_name="Unit ABC", expect_response=403) + self._delete_container(container_data["container_key"], expect_response=403) def test_unit_gets_auto_slugs(self): """ diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index c3e7b8130057..1656a8589616 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -78,7 +78,7 @@ ])), # Containers are Sections, Subsections, and Units path('containers//', include([ - # Get metadata about a specific container in this library, or delete the container: + # Get metadata about a specific container in this library, update or delete the container: path('', containers.LibraryContainerView.as_view()), # Update collections for a given container # path('collections/', views.LibraryContainerCollectionsView.as_view(), name='update-collections-ct'),