From f58bb6759b751b3130e20927c71bc6a8812af47d Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 14 Aug 2024 07:36:15 +0300 Subject: [PATCH 01/26] feat: Add Library Collections REST endpoints --- .../core/djangoapps/content_libraries/api.py | 6 +- .../collections/rest_api/v1/views.py | 110 ++++++++++++++++++ .../content_libraries/serializers.py | 21 ++++ .../core/djangoapps/content_libraries/urls.py | 6 + 4 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 17bea80b3a96..42bc6d4879d2 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -86,7 +86,7 @@ LIBRARY_BLOCK_UPDATED, ) from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import Component, MediaType +from openedx_learning.api.authoring_models import Component, MediaType, LearningPackage from organizations.models import Organization from xblock.core import XBlock from xblock.exceptions import XBlockNotFoundError @@ -150,6 +150,7 @@ class ContentLibraryMetadata: Class that represents the metadata about a content library. """ key = attr.ib(type=LibraryLocatorV2) + learning_package = attr.ib(type=LearningPackage) title = attr.ib("") description = attr.ib("") num_blocks = attr.ib(0) @@ -323,6 +324,7 @@ def get_metadata(queryset, text_search=None): has_unpublished_changes=False, has_unpublished_deletes=False, license=lib.license, + learning_package=lib.learning_package, ) for lib in queryset ] @@ -408,6 +410,7 @@ def get_library(library_key): license=ref.license, created=learning_package.created, updated=learning_package.updated, + learning_package=learning_package ) @@ -479,6 +482,7 @@ def create_library( allow_public_learning=ref.allow_public_learning, allow_public_read=ref.allow_public_read, license=library_license, + learning_package=ref.learning_package ) diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py new file mode 100644 index 000000000000..9fefa29e6170 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -0,0 +1,110 @@ +""" +Collections API Views +""" + +from __future__ import annotations + +from django.http import Http404 + +# from rest_framework.generics import GenericAPIView +from rest_framework.response import Response +from rest_framework.viewsets import ModelViewSet +from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED + +from opaque_keys.edx.locator import LibraryLocatorV2 + +from openedx.core.djangoapps.content_libraries import api, permissions +from openedx.core.djangoapps.content_libraries.serializers import ( + ContentLibraryCollectionSerializer, + ContentLibraryCollectionCreateOrUpdateSerializer, +) + +from openedx_learning.api.authoring_models import Collection +from openedx_learning.api import authoring as authoring_api + + +class LibraryCollectionsView(ModelViewSet): + """ + Views to get, create and update Library Collections. + """ + + serializer_class = ContentLibraryCollectionSerializer + + def retrieve(self, request, lib_key_str, pk=None): + """ + Retrieve the Content Library Collection + """ + try: + collection = authoring_api.get_collection(pk) + except Collection.DoesNotExist as exc: + raise Http404 from exc + + # Check if user has permissions to view this collection by checking if + # user has permission to view the Content Library it belongs to + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + serializer = self.get_serializer(collection) + return Response(serializer.data) + + def list(self, request, lib_key_str): + """ + List Collections that belong to Content Library + """ + # Check if user has permissions to view collections by checking if user + # has permission to view the Content Library they belong to + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + content_library = api.get_library(library_key) + collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) + serializer = self.get_serializer(collections, many=True) + return Response(serializer.data) + + def create(self, request, lib_key_str): + """ + Create a Collection that belongs to a Content Library + """ + # Check if user has permissions to create a collection in the Content Library + # by checking if user has permission to edit the Content Library + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) + create_serializer.is_valid(raise_exception=True) + content_library = api.get_library(library_key) + collection = authoring_api.create_collection( + content_library.learning_package.id, + create_serializer.validated_data["title"], + request.user.id, + create_serializer.validated_data["description"] + ) + serializer = self.get_serializer(collection) + return Response(serializer.data) + + def partial_update(self, request, lib_key_str, pk=None): + """ + Update a Collection that belongs to a Content Library + """ + # Check if user has permissions to update a collection in the Content Library + # by checking if user has permission to edit the Content Library + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + + try: + collection = authoring_api.get_collection(pk) + except Collection.DoesNotExist as exc: + raise Http404 from exc + + update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( + collection, data=request.data, partial=True + ) + update_serializer.is_valid(raise_exception=True) + updated_collection = authoring_api.update_collection(pk, **update_serializer.validated_data) + serializer = self.get_serializer(updated_collection) + return Response(serializer.data) + + def destroy(self, request, lib_key_str, pk=None): + """ + Deletes a Collection that belongs to a Content Library + + Note: (currently not allowed) + """ + return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 497eda81475b..7c49d4af3c2b 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -5,6 +5,8 @@ from django.core.validators import validate_unicode_slug from rest_framework import serializers + +from openedx_learning.api.authoring_models import Collection from openedx.core.djangoapps.content_libraries.constants import ( LIBRARY_TYPES, COMPLEX, @@ -245,3 +247,22 @@ class ContentLibraryBlockImportTaskCreateSerializer(serializers.Serializer): """ course_key = CourseKeyField() + + +class ContentLibraryCollectionSerializer(serializers.ModelSerializer): + """ + Serializer for a Content Library Collection + """ + + class Meta: + model = Collection + fields = '__all__' + + +class ContentLibraryCollectionCreateOrUpdateSerializer(serializers.Serializer): + """ + Serializer for add/update a Collection in a Content Library + """ + + title = serializers.CharField() + description = serializers.CharField() diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 6e450df63522..5521ad05bf63 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -7,6 +7,7 @@ from rest_framework import routers from . import views +from .collections.rest_api.v1 import views as collection_views # Django application name. @@ -18,6 +19,9 @@ import_blocks_router = routers.DefaultRouter() import_blocks_router.register(r'tasks', views.LibraryImportTaskViewSet, basename='import-block-task') +library_collections_router = routers.DefaultRouter() +library_collections_router.register(r'collections', collection_views.LibraryCollectionsView, basename="library-collections") + # These URLs are only used in Studio. The LMS already provides all the # API endpoints needed to serve XBlocks from content libraries using the # standard XBlock REST API (see openedx.core.django_apps.xblock.rest_api.urls) @@ -45,6 +49,8 @@ path('import_blocks/', include(import_blocks_router.urls)), # Paste contents of clipboard into library path('paste_clipboard/', views.LibraryPasteClipboardView.as_view()), + # Library Collections + path('', include(library_collections_router.urls)), ])), path('blocks//', include([ # Get metadata about a specific XBlock in this library, or delete the block: From f4900071c3f9b0668390bc7bbaf361a4282aa5de Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 22 Aug 2024 08:33:45 +0300 Subject: [PATCH 02/26] test: Add tests for Collections REST APIs --- .../collections/rest_api/v1/tests/__init__.py | 0 .../rest_api/v1/tests/test_views.py | 184 ++++++++++++++++++ .../core/djangoapps/content_libraries/urls.py | 4 +- 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py new file mode 100644 index 000000000000..3f37ad3a1c08 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -0,0 +1,184 @@ +""" +Tests Library Collections REST API views +""" + +from __future__ import annotations + +from openedx_learning.api.authoring_models import Collection + +from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from openedx.core.djangoapps.content_libraries.models import ContentLibrary +from common.djangoapps.student.tests.factories import UserFactory + +URL_PREFIX = '/api/libraries/v2/{lib_key}/' +URL_LIB_COLLECTIONS = URL_PREFIX + 'collections/' +URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_id}/' + + +@skip_unless_cms # Content Library Collections REST API is only available in Studio +class ContentLibraryCollectionsViewsTest(ContentLibrariesRestApiTest): + """ + Tests for Content Library Collection REST API Views + """ + + def setUp(self): + super().setUp() + + # Create Content Libraries + self._create_library("test-lib-col-1", "Test Library 1") + self._create_library("test-lib-col-2", "Test Library 2") + self.lib1 = ContentLibrary.objects.get(slug="test-lib-col-1") + self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") + + print("self.lib1", self.lib1) + print("self.lib2", self.lib2) + + # Create Content Library Collections + self.col1 = Collection.objects.create( + learning_package_id=self.lib1.learning_package.id, + title="Collection 1", + description="Description for Collection 1", + created_by=self.user, + ) + self.col2 = Collection.objects.create( + learning_package_id=self.lib1.learning_package.id, + title="Collection 2", + description="Description for Collection 2", + created_by=self.user, + ) + self.col3 = Collection.objects.create( + learning_package_id=self.lib2.learning_package.id, + title="Collection 3", + description="Description for Collection 3", + created_by=self.user, + ) + + def test_get_library_collection(self): + """ + Test retrieving a Content Library Collection + """ + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) + ) + + # Check that correct Content Library Collection data retrieved + expected_collection = { + "title": "Collection 3", + "description": "Description for Collection 3", + } + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, expected_collection) + + # Check that a random user without permissions cannot access Content Library Collection + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) + ) + assert resp.status_code == 403 + + def test_list_library_collections(self): + """ + Test listing Content Library Collections + """ + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) + + # Check that the correct collections are listed + assert resp.status_code == 200 + assert len(resp.data) == 2 + expected_collections = [ + {"title": "Collection 1", "description": "Description for Collection 1"}, + {"title": "Collection 2", "description": "Description for Collection 2"}, + ] + for collection, expected in zip(resp.data, expected_collections): + self.assertDictContainsEntries(collection, expected) + + # Check that a random user without permissions cannot access Content Library Collections + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) + assert resp.status_code == 403 + + def test_create_library_collection(self): + """ + Test creating a Content Library Collection + """ + post_data = { + "title": "Collection 4", + "description": "Description for Collection 4", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + + # Check that the new Content Library Collection is returned in response and created in DB + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, post_data) + + created_collection = Collection.objects.get(id=resp.data["id"]) + self.assertIsNotNone(created_collection) + + # Check that user with read only access cannot create new Content Library Collection + reader = UserFactory.create(username="Reader", email="reader@example.com") + self._add_user_by_email(self.lib1.library_key, reader.email, access_level="read") + + with self.as_user(reader): + post_data = { + "title": "Collection 5", + "description": "Description for Collection 5", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + + assert resp.status_code == 403 + + def test_update_library_collection(self): + """ + Test updating a Content Library Collection + """ + patch_data = { + "title": "Collection 3 Updated", + } + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + patch_data, + format="json" + ) + + # Check that updated Content Library Collection is returned in response and updated in DB + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, patch_data) + + created_collection = Collection.objects.get(id=resp.data["id"]) + self.assertIsNotNone(created_collection) + self.assertEqual(created_collection.title, patch_data["title"]) + + # Check that user with read only access cannot update a Content Library Collection + reader = UserFactory.create(username="Reader", email="reader@example.com") + self._add_user_by_email(self.lib1.library_key, reader.email, access_level="read") + + with self.as_user(reader): + patch_data = { + "title": "Collection 3 should not update", + } + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + patch_data, + format="json" + ) + + assert resp.status_code == 403 + + def test_delete_library_collection(self): + """ + Test deleting a Content Library Collection + + Note: Currently not implemented and should return a 405 + """ + resp = self.client.delete( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) + ) + + assert resp.status_code == 405 diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 5521ad05bf63..a0c5a9f8cd61 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -20,7 +20,9 @@ import_blocks_router.register(r'tasks', views.LibraryImportTaskViewSet, basename='import-block-task') library_collections_router = routers.DefaultRouter() -library_collections_router.register(r'collections', collection_views.LibraryCollectionsView, basename="library-collections") +library_collections_router.register( + r'collections', collection_views.LibraryCollectionsView, basename="library-collections" +) # These URLs are only used in Studio. The LMS already provides all the # API endpoints needed to serve XBlocks from content libraries using the From 17933a1744cbf73bca64217bfe3e94bdd411b79d Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 23 Aug 2024 10:00:56 +0300 Subject: [PATCH 03/26] chore: Add missing __init__ files --- .../djangoapps/content_libraries/collections/rest_api/__init__.py | 0 .../content_libraries/collections/rest_api/v1/__init__.py | 0 2 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 From 743291c8b4de541b890ea519180f602d19900d4b Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 23 Aug 2024 13:46:23 +0300 Subject: [PATCH 04/26] feat: Verify collection belongs to library --- .../core/djangoapps/content_libraries/api.py | 6 +- .../rest_api/v1/tests/test_views.py | 78 ++++++++++++++++++- .../collections/rest_api/v1/views.py | 61 +++++++++++---- 3 files changed, 123 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 42bc6d4879d2..9a4ccbd6456e 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -331,7 +331,7 @@ def get_metadata(queryset, text_search=None): return libraries -def require_permission_for_library_key(library_key, user, permission): +def require_permission_for_library_key(library_key, user, permission) -> ContentLibrary: """ Given any of the content library permission strings defined in openedx.core.djangoapps.content_libraries.permissions, @@ -341,10 +341,12 @@ def require_permission_for_library_key(library_key, user, permission): Raises django.core.exceptions.PermissionDenied if the user doesn't have permission. """ - library_obj = ContentLibrary.objects.get_by_key(library_key) + library_obj = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] if not user.has_perm(permission, obj=library_obj): raise PermissionDenied + return library_obj + def get_library(library_key): """ diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py index 3f37ad3a1c08..dbaa5f4c1cf9 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -5,6 +5,7 @@ from __future__ import annotations from openedx_learning.api.authoring_models import Collection +from opaque_keys.edx.locator import LibraryLocatorV2 from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest @@ -31,9 +32,6 @@ def setUp(self): self.lib1 = ContentLibrary.objects.get(slug="test-lib-col-1") self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") - print("self.lib1", self.lib1) - print("self.lib2", self.lib2) - # Create Content Library Collections self.col1 = Collection.objects.create( learning_package_id=self.lib1.learning_package.id, @@ -78,6 +76,24 @@ def test_get_library_collection(self): ) assert resp.status_code == 403 + def test_get_invalid_library_collection(self): + """ + Test retrieving a an invalid Content Library Collection or one that does not exist + """ + # Fetch collection that belongs to a different library, it should fail + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=self.col3.id) + ) + + assert resp.status_code == 404 + + # Fetch collection with invalid ID provided, it should fail + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=123) + ) + + assert resp.status_code == 404 + def test_list_library_collections(self): """ Test listing Content Library Collections @@ -100,6 +116,15 @@ def test_list_library_collections(self): resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) assert resp.status_code == 403 + def test_list_invalid_library_collections(self): + """ + Test listing invalid Content Library Collections + """ + invalid_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=invalid_key)) + + assert resp.status_code == 404 + def test_create_library_collection(self): """ Test creating a Content Library Collection @@ -134,6 +159,28 @@ def test_create_library_collection(self): assert resp.status_code == 403 + def test_create_invalid_library_collection(self): + """ + Test creating an invalid Content Library Collection + """ + post_data_missing_title = { + "description": "Description for Collection 4", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_title, format="json" + ) + + assert resp.status_code == 400 + + post_data_missing_desc = { + "title": "Collection 4", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_desc, format="json" + ) + + assert resp.status_code == 400 + def test_update_library_collection(self): """ Test updating a Content Library Collection @@ -171,6 +218,31 @@ def test_update_library_collection(self): assert resp.status_code == 403 + def test_update_invalid_library_collection(self): + """ + Test updating an invalid Content Library Collection or one that does not exist + """ + patch_data = { + "title": "Collection 3 Updated", + } + # Update collection that belongs to a different library, it should fail + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=self.col3.id), + patch_data, + format="json" + ) + + assert resp.status_code == 404 + + # Update collection with invalid ID provided, it should fail + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=123), + patch_data, + format="json" + ) + + assert resp.status_code == 404 + def test_delete_library_collection(self): """ Test deleting a Content Library Collection diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 9fefa29e6170..2c4df17e6d08 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -6,7 +6,6 @@ from django.http import Http404 -# from rest_framework.generics import GenericAPIView from rest_framework.response import Response from rest_framework.viewsets import ModelViewSet from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED @@ -30,19 +29,37 @@ class LibraryCollectionsView(ModelViewSet): serializer_class = ContentLibraryCollectionSerializer + def _verify_and_fetch_library_collection(self, library_key, collection_id, user, permission) -> Collection | None: + """ + Verify that the collection belongs to the library and the user has the correct permissions + """ + try: + library_obj = api.require_permission_for_library_key(library_key, user, permission) + except api.ContentLibraryNotFound: + raise Http404 + + collection = None + if library_obj.learning_package_id: + collection = authoring_api.get_learning_package_collections( + library_obj.learning_package_id + ).filter(id=collection_id).first() + return collection + def retrieve(self, request, lib_key_str, pk=None): """ Retrieve the Content Library Collection """ - try: - collection = authoring_api.get_collection(pk) - except Collection.DoesNotExist as exc: - raise Http404 from exc + library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to view this collection by checking if # user has permission to view the Content Library it belongs to - library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + collection = self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + ) + + if not collection: + raise Http404 + serializer = self.get_serializer(collection) return Response(serializer.data) @@ -53,8 +70,13 @@ def list(self, request, lib_key_str): # Check if user has permissions to view collections by checking if user # has permission to view the Content Library they belong to library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) - content_library = api.get_library(library_key) + try: + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + ) + except api.ContentLibraryNotFound: + raise Http404 + collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) @@ -66,10 +88,15 @@ def create(self, request, lib_key_str): # Check if user has permissions to create a collection in the Content Library # by checking if user has permission to edit the Content Library library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + try: + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) + except api.ContentLibraryNotFound: + raise Http404 + create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) - content_library = api.get_library(library_key) collection = authoring_api.create_collection( content_library.learning_package.id, create_serializer.validated_data["title"], @@ -83,15 +110,15 @@ def partial_update(self, request, lib_key_str, pk=None): """ Update a Collection that belongs to a Content Library """ + library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to update a collection in the Content Library # by checking if user has permission to edit the Content Library - library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + collection = self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) - try: - collection = authoring_api.get_collection(pk) - except Collection.DoesNotExist as exc: - raise Http404 from exc + if not collection: + raise Http404 update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( collection, data=request.data, partial=True From 3da44d516361a14a7004292ee83a14504cb25412 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 26 Aug 2024 07:42:19 +0300 Subject: [PATCH 05/26] feat: Add events emitting for Collections --- docs/hooks/events.rst | 12 ++++ .../core/djangoapps/content_libraries/apps.py | 1 + .../content_libraries/collections/__init__.py | 0 .../content_libraries/collections/handlers.py | 57 +++++++++++++++++++ .../collections/rest_api/v1/views.py | 26 +++++++++ requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 4 +- requirements/edx/testing.txt | 2 +- 10 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/__init__.py create mode 100644 openedx/core/djangoapps/content_libraries/collections/handlers.py diff --git a/docs/hooks/events.rst b/docs/hooks/events.rst index 7f9584c9e8e1..2bceee5050ea 100644 --- a/docs/hooks/events.rst +++ b/docs/hooks/events.rst @@ -247,3 +247,15 @@ Content Authoring Events * - `CONTENT_OBJECT_TAGS_CHANGED `_ - org.openedx.content_authoring.content.object.tags.changed.v1 - 2024-03-31 + + * - `LIBRARY_COLLECTION_CREATED `_ + - org.openedx.content_authoring.content.library.collection.created.v1 + - 2024-08-23 + + * - `LIBRARY_COLLECTION_UPDATED `_ + - org.openedx.content_authoring.content.library.collection.updated.v1 + - 2024-08-23 + + * - `LIBRARY_COLLECTION_DELETED `_ + - org.openedx.content_authoring.content.library.collection.deleted.v1 + - 2024-08-23 diff --git a/openedx/core/djangoapps/content_libraries/apps.py b/openedx/core/djangoapps/content_libraries/apps.py index 52c3e5179721..aea920714ce0 100644 --- a/openedx/core/djangoapps/content_libraries/apps.py +++ b/openedx/core/djangoapps/content_libraries/apps.py @@ -37,3 +37,4 @@ def ready(self): Import signal handler's module to ensure they are registered. """ from . import signal_handlers # pylint: disable=unused-import + from .collections import handlers # pylint: disable=unused-import diff --git a/openedx/core/djangoapps/content_libraries/collections/__init__.py b/openedx/core/djangoapps/content_libraries/collections/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/content_libraries/collections/handlers.py b/openedx/core/djangoapps/content_libraries/collections/handlers.py new file mode 100644 index 000000000000..c2c67ad403e5 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/collections/handlers.py @@ -0,0 +1,57 @@ +""" +Signal handlers for Content Library Collections. +""" + +import logging + +from django.dispatch import receiver +from openedx_events.content_authoring.data import LibraryCollectionData +from openedx_events.content_authoring.signals import ( + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, + LIBRARY_COLLECTION_DELETED, +) + + +log = logging.getLogger(__name__) + + +@receiver(LIBRARY_COLLECTION_CREATED) +def library_collection_created_handler(**kwargs): + """ + Content Library Collection Created signal handler + """ + library_collection_data = kwargs.get("library_collection", None) + if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): + log.error("Received null or incorrect data for event") + return + + log.info("Received Collection Created Signal") + + # TODO: Implement handler logic + + +@receiver(LIBRARY_COLLECTION_UPDATED) +def library_collection_updated_handler(**kwargs): + """ + Content Library Collection Updated signal handler + """ + library_collection_data = kwargs.get("library_collection", None) + if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): + log.error("Received null or incorrect data for event") + return + + log.info("Received Collection Updated Signal") + + +@receiver(LIBRARY_COLLECTION_DELETED) +def library_collection_deleted_handler(**kwargs): + """ + Content Library Collection Deleted signal handler + """ + library_collection_data = kwargs.get("library_collection", None) + if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): + log.error("Received null or incorrect data for event") + return + + log.info("Received Collection Deleted Signal") diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 2c4df17e6d08..b20c00a48c96 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -12,6 +12,12 @@ from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx_events.content_authoring.data import LibraryCollectionData +from openedx_events.content_authoring.signals import ( + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, +) + from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, @@ -104,6 +110,15 @@ def create(self, request, lib_key_str): create_serializer.validated_data["description"] ) serializer = self.get_serializer(collection) + + # Emit event for library content collection creation + LIBRARY_COLLECTION_CREATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_id=collection.id + ) + ) + return Response(serializer.data) def partial_update(self, request, lib_key_str, pk=None): @@ -126,6 +141,15 @@ def partial_update(self, request, lib_key_str, pk=None): update_serializer.is_valid(raise_exception=True) updated_collection = authoring_api.update_collection(pk, **update_serializer.validated_data) serializer = self.get_serializer(updated_collection) + + # Emit event for library content collection updated + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_id=collection.id + ) + ) + return Response(serializer.data) def destroy(self, request, lib_key_str, pk=None): @@ -134,4 +158,6 @@ def destroy(self, request, lib_key_str, pk=None): Note: (currently not allowed) """ + # TODO: Implement the deletion logic and emit event signal + return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 27de7847f284..107ed6a79018 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -811,7 +811,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/kernel.in openedx-django-wiki==2.1.0 # via -r requirements/edx/kernel.in -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/kernel.in # edx-event-bus-kafka diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 0bdc8144ee71..cefdd9b330ce 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1358,7 +1358,7 @@ openedx-django-wiki==2.1.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 91b30d81df6d..91313a627e48 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -970,7 +970,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/base.txt # edx-event-bus-kafka diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index a5b510742ac7..5154dc293efa 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -117,7 +117,9 @@ olxcleaner openedx-atlas # CLI tool to manage translations openedx-calc # Library supporting mathematical calculations for Open edX openedx-django-require -openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) +# openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) +# TODO: Remove this once new version released +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) openedx-learning # Open edX Learning core (experimental) openedx-mongodbproxy diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 2092c9354834..04b85af47b80 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1021,7 +1021,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/base.txt # edx-event-bus-kafka From 59514cc13ff97e1b9c03b5b4acadd070a78d08c8 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 26 Aug 2024 08:43:40 +0300 Subject: [PATCH 06/26] chore: fix pylint errors --- .../collections/rest_api/v1/views.py | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index b20c00a48c96..0d89c4b8a2c7 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -41,8 +41,8 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, """ try: library_obj = api.require_permission_for_library_key(library_key, user, permission) - except api.ContentLibraryNotFound: - raise Http404 + except api.ContentLibraryNotFound as exc: + raise Http404 from exc collection = None if library_obj.learning_package_id: @@ -51,10 +51,15 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, ).filter(id=collection_id).first() return collection - def retrieve(self, request, lib_key_str, pk=None): + def retrieve(self, request, *args, **kwargs): """ Retrieve the Content Library Collection """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + + pk = kwargs.pop("pk", None) library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to view this collection by checking if @@ -69,10 +74,14 @@ def retrieve(self, request, lib_key_str, pk=None): serializer = self.get_serializer(collection) return Response(serializer.data) - def list(self, request, lib_key_str): + def list(self, request, *args, **kwargs): """ List Collections that belong to Content Library """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + # Check if user has permissions to view collections by checking if user # has permission to view the Content Library they belong to library_key = LibraryLocatorV2.from_string(lib_key_str) @@ -80,17 +89,21 @@ def list(self, request, lib_key_str): content_library = api.require_permission_for_library_key( library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) - except api.ContentLibraryNotFound: - raise Http404 + except api.ContentLibraryNotFound as exc: + raise Http404 from exc collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) - def create(self, request, lib_key_str): + def create(self, request, *args, **kwargs): """ Create a Collection that belongs to a Content Library """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + # Check if user has permissions to create a collection in the Content Library # by checking if user has permission to edit the Content Library library_key = LibraryLocatorV2.from_string(lib_key_str) @@ -98,8 +111,8 @@ def create(self, request, lib_key_str): content_library = api.require_permission_for_library_key( library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - except api.ContentLibraryNotFound: - raise Http404 + except api.ContentLibraryNotFound as exc: + raise Http404 from exc create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) @@ -121,10 +134,15 @@ def create(self, request, lib_key_str): return Response(serializer.data) - def partial_update(self, request, lib_key_str, pk=None): + def partial_update(self, request, *args, **kwargs): """ Update a Collection that belongs to a Content Library """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + + pk = kwargs.pop('pk', None) library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to update a collection in the Content Library # by checking if user has permission to edit the Content Library @@ -152,7 +170,7 @@ def partial_update(self, request, lib_key_str, pk=None): return Response(serializer.data) - def destroy(self, request, lib_key_str, pk=None): + def destroy(self, request, *args, **kwargs): """ Deletes a Collection that belongs to a Content Library From 250434fddb1cb8d5e205318d7cdb9c85e3725070 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 23 Aug 2024 19:50:05 +0930 Subject: [PATCH 07/26] chore: update openedx-learning --- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 74263b5f7141..0c088275fedb 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -93,7 +93,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.11.1 +openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. openai<=0.28.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 107ed6a79018..ac8d7e421701 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -823,7 +823,7 @@ openedx-filters==1.9.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-learning==0.11.1 +openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index cefdd9b330ce..97d51c13facc 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1372,7 +1372,7 @@ openedx-filters==1.9.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-learning==0.11.1 +openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 91313a627e48..2527d80833a6 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -982,7 +982,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.11.1 +openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 04b85af47b80..340909a950b4 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1033,7 +1033,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.11.1 +openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From f4521b507c756ea10af5d136b2f1ca767c9239e2 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Aug 2024 13:33:31 +0930 Subject: [PATCH 08/26] feat: add/remove components to/from a collection Python API: * Converts UsageKeyV2 object keys into component keys for use with the oel_collections api. * Sends a CONTENT_OBJECT_TAGS_CHANGED for each component added/removed. REST API: * Calls the python API * Receives a collection PK + a list of UsageKeys to add to the collection. --- .../core/djangoapps/content_libraries/api.py | 74 ++++++++++- .../rest_api/v1/tests/test_views.py | 120 ++++++++++++++++++ .../collections/rest_api/v1/views.py | 46 ++++++- .../content_libraries/serializers.py | 33 +++++ 4 files changed, 266 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 9a4ccbd6456e..3fc9a1e72d4a 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -69,15 +69,20 @@ from django.utils.translation import gettext as _ from edx_rest_api_client.client import OAuthAPIClient from lxml import etree -from opaque_keys.edx.keys import UsageKey, UsageKeyV2 +from opaque_keys.edx.keys import BlockTypeKey, UsageKey, UsageKeyV2 from opaque_keys.edx.locator import ( LibraryLocatorV2, LibraryUsageLocatorV2, LibraryLocator as LibraryLocatorV1 ) from opaque_keys import InvalidKeyError -from openedx_events.content_authoring.data import ContentLibraryData, LibraryBlockData +from openedx_events.content_authoring.data import ( + ContentLibraryData, + ContentObjectData, + LibraryBlockData, +) from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_TAGS_CHANGED, CONTENT_LIBRARY_CREATED, CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, @@ -86,7 +91,7 @@ LIBRARY_BLOCK_UPDATED, ) from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import Component, MediaType, LearningPackage +from openedx_learning.api.authoring_models import Collection, Component, MediaType, LearningPackage from organizations.models import Organization from xblock.core import XBlock from xblock.exceptions import XBlockNotFoundError @@ -1062,6 +1067,69 @@ def revert_changes(library_key): ) +def update_collection_contents( + library: ContentLibraryMetadata, + collection_pk: int, + usage_keys: list[UsageKeyV2], + remove=False, +) -> int: + """ + Associates the Collection with Components for the given UsageKeys. + + By default the Components are added to the Collection. + If remove=True, the Components are removed from the Collection. + + Raises: + * Collection.DoesNotExist if no Collection with the given pk is found in the given library. + * Component.DoesNotExist if any of the given usage_keys don't correspond to a Component in the + library/learning_package. + """ + learning_package = library.learning_package + collections_qset = authoring_api.get_learning_package_collections( + learning_package.id, + ).filter(pk=collection_pk) + + collection = collections_qset.first() + if not collection: + raise Collection.DoesNotExist() + + # Fetch the Component.key values for the provided UsageKeys. + component_keys = [] + for usage_key in usage_keys: + # Parse the block_family from the key to use as namespace. + block_type = BlockTypeKey.from_string(str(usage_key)) + + # Can raise Component.DoesNotExist + component = authoring_api.get_component_by_key( + learning_package.id, + namespace=block_type.block_family, + type_name=usage_key.block_type, + local_key=usage_key.block_id, + ) + component_keys.append(component.key) + + # Note: Component.key matches its PublishableEntity.key + contents_qset = learning_package.publishable_entities.filter( + key__in=component_keys, + ) + if not contents_qset.count(): + raise Component.DoesNotExist() + + if remove: + count = authoring_api.remove_from_collections(collections_qset, contents_qset) + else: + count = authoring_api.add_to_collections(collections_qset, contents_qset) + + # Emit a CONTENT_OBJECT_TAGS_CHANGED event for each of the objects added/removed + object_keys = contents_qset.values_list("key", flat=True) + for object_key in object_keys: + CONTENT_OBJECT_TAGS_CHANGED.send_event( + content_object=ContentObjectData(object_id=object_key), + ) + + return count + + # V1/V2 Compatibility Helpers # (Should be removed as part of # https://github.com/openedx/edx-platform/issues/32457) diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py index dbaa5f4c1cf9..798e83116b29 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -3,6 +3,7 @@ """ from __future__ import annotations +import ddt from openedx_learning.api.authoring_models import Collection from opaque_keys.edx.locator import LibraryLocatorV2 @@ -15,8 +16,10 @@ URL_PREFIX = '/api/libraries/v2/{lib_key}/' URL_LIB_COLLECTIONS = URL_PREFIX + 'collections/' URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_id}/' +URL_LIB_COLLECTION_CONTENTS = URL_LIB_COLLECTION + 'contents/' +@ddt.ddt @skip_unless_cms # Content Library Collections REST API is only available in Studio class ContentLibraryCollectionsViewsTest(ContentLibrariesRestApiTest): """ @@ -52,6 +55,20 @@ def setUp(self): created_by=self.user, ) + # Create some library blocks + self.lib1_problem_block = self._add_block_to_library( + self.lib1.library_key, "problem", "problem1", + ) + self.lib1_html_block = self._add_block_to_library( + self.lib1.library_key, "html", "html1", + ) + self.lib2_problem_block = self._add_block_to_library( + self.lib2.library_key, "problem", "problem2", + ) + self.lib2_html_block = self._add_block_to_library( + self.lib2.library_key, "html", "html2", + ) + def test_get_library_collection(self): """ Test retrieving a Content Library Collection @@ -254,3 +271,106 @@ def test_delete_library_collection(self): ) assert resp.status_code == 405 + + def test_get_components(self): + """ + Retrieving components is not supported by the REST API; + use Meilisearch instead. + """ + resp = self.client.get( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + ) + assert resp.status_code == 405 + + def test_update_components(self): + """ + Test adding and removing components from a collection. + """ + # Add two components to col1 + resp = self.client.patch( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + self.lib1_html_block["id"], + ] + } + ) + assert resp.status_code == 200 + assert resp.data == {"count": 2} + + # Remove one of the added components from col1 + resp = self.client.delete( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + ] + } + ) + assert resp.status_code == 200 + assert resp.data == {"count": 1} + + @ddt.data("patch", "delete") + def test_update_components_wrong_collection(self, method): + """ + Collection must belong to the requested library. + """ + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib2.library_key, collection_id=self.col1.id), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + ] + } + ) + assert resp.status_code == 404 + + @ddt.data("patch", "delete") + def test_update_components_missing_data(self, method): + """ + List of component keys must contain at least one item. + """ + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + ) + assert resp.status_code == 400 + assert resp.data == { + "usage_keys": ["This field is required."], + } + + @ddt.data("patch", "delete") + def test_update_components_from_another_library(self, method): + """ + Adding/removing components from another library raises a validation error. + """ + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + self.lib1_html_block["id"], + ] + } + ) + assert resp.status_code == 400 + assert resp.data == { + "usage_keys": "Component(s) not found in library", + } + + @ddt.data("patch", "delete") + def test_update_components_permissions(self, method): + """ + Check that a random user without permissions cannot update a Content Library Collection's components. + """ + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + ) + assert resp.status_code == 403 + + resp = self.client.patch( + URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + ) + assert resp.status_code == 403 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 0d89c4b8a2c7..c0a104101534 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -5,11 +5,16 @@ from __future__ import annotations from django.http import Http404 +from django.utils.translation import gettext as _ +from rest_framework.decorators import action +from rest_framework.exceptions import ValidationError from rest_framework.response import Response -from rest_framework.viewsets import ModelViewSet from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED +from rest_framework.viewsets import ModelViewSet +from openedx_learning.api.authoring_models import Collection, Component +from openedx_learning.api import authoring as authoring_api from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_events.content_authoring.data import LibraryCollectionData @@ -21,12 +26,10 @@ from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, + ContentLibraryCollectionContentsUpdateSerializer, ContentLibraryCollectionCreateOrUpdateSerializer, ) -from openedx_learning.api.authoring_models import Collection -from openedx_learning.api import authoring as authoring_api - class LibraryCollectionsView(ModelViewSet): """ @@ -179,3 +182,38 @@ def destroy(self, request, *args, **kwargs): # TODO: Implement the deletion logic and emit event signal return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) + + @action(detail=True, methods=['delete', 'patch'], url_path='contents', url_name='contents:update') + def update_contents(self, request, lib_key_str, pk=None): + """ + Adds (PATCH) or removes (DELETE) Components to/from a Collection. + + Collection and Components must all be part of the given library/learning package. + """ + library_key = LibraryLocatorV2.from_string(lib_key_str) + library_obj = api.require_permission_for_library_key( + library_key, + request.user, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + ) + + serializer = ContentLibraryCollectionContentsUpdateSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + + try: + count = api.update_collection_contents( + library_obj, + collection_pk=pk, + usage_keys=serializer.validated_data["usage_keys"], + remove=(request.method == "DELETE"), + ) + except Collection.DoesNotExist as exc: + raise Http404() from exc + + except Component.DoesNotExist as exc: + # Only allows adding/removing components that are in the collection's learning package. + raise ValidationError({ + "usage_keys": _("Component(s) not found in library"), + }) from exc + + return Response({'count': count}) diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 7c49d4af3c2b..c3eb2f056f9c 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -4,7 +4,10 @@ # pylint: disable=abstract-method from django.core.validators import validate_unicode_slug from rest_framework import serializers +from rest_framework.exceptions import ValidationError +from opaque_keys.edx.keys import UsageKeyV2 +from opaque_keys import InvalidKeyError from openedx_learning.api.authoring_models import Collection from openedx.core.djangoapps.content_libraries.constants import ( @@ -266,3 +269,33 @@ class ContentLibraryCollectionCreateOrUpdateSerializer(serializers.Serializer): title = serializers.CharField() description = serializers.CharField() + + +class UsageKeyV2Serializer(serializers.Serializer): + """ + Serializes a UsageKeyV2. + """ + def to_representation(self, value: UsageKeyV2) -> str: + """ + Returns the UsageKeyV2 value as a string. + """ + return str(value) + + def to_internal_value(self, value: str) -> UsageKeyV2: + """ + Returns a UsageKeyV2 from the string value. + + Raises ValidationError if invalid UsageKeyV2. + """ + try: + return UsageKeyV2.from_string(value) + except InvalidKeyError as err: + raise ValidationError from err + + +class ContentLibraryCollectionContentsUpdateSerializer(serializers.Serializer): + """ + Serializer for adding/removing Components to/from a Collection. + """ + + usage_keys = serializers.ListField(child=UsageKeyV2Serializer(), allow_empty=False) From 129bcf28eb2997649f5fcb3b6332fec5d2ea30a1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 29 Aug 2024 11:19:21 +0930 Subject: [PATCH 09/26] refactor: use "components" not "contents" Addresses PR reviews. * Collection contents -> Collection components * Add warning about unstable REST APIs * Wrap update_components view in convert_exceptions * Raise api exceptions in api.update_collection_components * Use usage_key in CONTENT_OBJECT_TAGS_CHANGED event --- .../core/djangoapps/content_libraries/api.py | 52 ++++++++++++------- .../rest_api/v1/tests/test_views.py | 51 +++++++++++------- .../collections/rest_api/v1/views.py | 39 ++++++-------- .../content_libraries/serializers.py | 2 +- .../djangoapps/content_libraries/views.py | 4 ++ 5 files changed, 86 insertions(+), 62 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 3fc9a1e72d4a..09c872ff7dee 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -116,6 +116,8 @@ ContentLibraryNotFound = ContentLibrary.DoesNotExist +ContentLibraryCollectionNotFound = Collection.DoesNotExist + class ContentLibraryBlockNotFound(XBlockNotFoundError): """ XBlock not found in the content library """ @@ -1067,10 +1069,11 @@ def revert_changes(library_key): ) -def update_collection_contents( +def update_collection_components( library: ContentLibraryMetadata, collection_pk: int, usage_keys: list[UsageKeyV2], + created_by: int | None = None, remove=False, ) -> int: """ @@ -1080,9 +1083,8 @@ def update_collection_contents( If remove=True, the Components are removed from the Collection. Raises: - * Collection.DoesNotExist if no Collection with the given pk is found in the given library. - * Component.DoesNotExist if any of the given usage_keys don't correspond to a Component in the - library/learning_package. + * ContentLibraryCollectionNotFound if no Collection with the given pk is found in the given library. + * ContentLibraryBlockNotFound if any of the given usage_keys don't match Components in the given library. """ learning_package = library.learning_package collections_qset = authoring_api.get_learning_package_collections( @@ -1091,7 +1093,7 @@ def update_collection_contents( collection = collections_qset.first() if not collection: - raise Collection.DoesNotExist() + raise ContentLibraryCollectionNotFound(collection_pk) # Fetch the Component.key values for the provided UsageKeys. component_keys = [] @@ -1099,32 +1101,42 @@ def update_collection_contents( # Parse the block_family from the key to use as namespace. block_type = BlockTypeKey.from_string(str(usage_key)) - # Can raise Component.DoesNotExist - component = authoring_api.get_component_by_key( - learning_package.id, - namespace=block_type.block_family, - type_name=usage_key.block_type, - local_key=usage_key.block_id, - ) + try: + component = authoring_api.get_component_by_key( + learning_package.id, + namespace=block_type.block_family, + type_name=usage_key.block_type, + local_key=usage_key.block_id, + ) + except Component.DoesNotExist as exc: + raise ContentLibraryBlockNotFound(usage_key) from exc + component_keys.append(component.key) # Note: Component.key matches its PublishableEntity.key - contents_qset = learning_package.publishable_entities.filter( + entities_qset = learning_package.publishable_entities.filter( key__in=component_keys, ) - if not contents_qset.count(): - raise Component.DoesNotExist() + if not entities_qset.count(): + usage_key = usage_keys[0] if usage_keys else None + raise ContentLibraryBlockNotFound(usage_key) if remove: - count = authoring_api.remove_from_collections(collections_qset, contents_qset) + count = authoring_api.remove_from_collections( + collections_qset, + entities_qset, + ) else: - count = authoring_api.add_to_collections(collections_qset, contents_qset) + count = authoring_api.add_to_collections( + collections_qset, + entities_qset, + created_by=created_by, + ) # Emit a CONTENT_OBJECT_TAGS_CHANGED event for each of the objects added/removed - object_keys = contents_qset.values_list("key", flat=True) - for object_key in object_keys: + for usage_key in usage_keys: CONTENT_OBJECT_TAGS_CHANGED.send_event( - content_object=ContentObjectData(object_id=object_key), + content_object=ContentObjectData(object_id=usage_key), ) return count diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py index 798e83116b29..4f6a9b442daf 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -16,7 +16,7 @@ URL_PREFIX = '/api/libraries/v2/{lib_key}/' URL_LIB_COLLECTIONS = URL_PREFIX + 'collections/' URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_id}/' -URL_LIB_COLLECTION_CONTENTS = URL_LIB_COLLECTION + 'contents/' +URL_LIB_COLLECTION_COMPONENTS = URL_LIB_COLLECTION + 'components/' @ddt.ddt @@ -278,7 +278,10 @@ def test_get_components(self): use Meilisearch instead. """ resp = self.client.get( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), ) assert resp.status_code == 405 @@ -288,7 +291,10 @@ def test_update_components(self): """ # Add two components to col1 resp = self.client.patch( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), data={ "usage_keys": [ self.lib1_problem_block["id"], @@ -301,7 +307,10 @@ def test_update_components(self): # Remove one of the added components from col1 resp = self.client.delete( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), data={ "usage_keys": [ self.lib1_problem_block["id"], @@ -317,7 +326,10 @@ def test_update_components_wrong_collection(self, method): Collection must belong to the requested library. """ resp = getattr(self.client, method)( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib2.library_key, collection_id=self.col1.id), + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib2.library_key, + collection_id=self.col1.id, + ), data={ "usage_keys": [ self.lib1_problem_block["id"], @@ -329,10 +341,13 @@ def test_update_components_wrong_collection(self, method): @ddt.data("patch", "delete") def test_update_components_missing_data(self, method): """ - List of component keys must contain at least one item. + List of usage keys must contain at least one item. """ resp = getattr(self.client, method)( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib2.library_key, + collection_id=self.col3.id, + ), ) assert resp.status_code == 400 assert resp.data == { @@ -342,10 +357,13 @@ def test_update_components_missing_data(self, method): @ddt.data("patch", "delete") def test_update_components_from_another_library(self, method): """ - Adding/removing components from another library raises a validation error. + Adding/removing components from another library raises a 404. """ resp = getattr(self.client, method)( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib2.library_key, + collection_id=self.col3.id, + ), data={ "usage_keys": [ self.lib1_problem_block["id"], @@ -353,10 +371,7 @@ def test_update_components_from_another_library(self, method): ] } ) - assert resp.status_code == 400 - assert resp.data == { - "usage_keys": "Component(s) not found in library", - } + assert resp.status_code == 404 @ddt.data("patch", "delete") def test_update_components_permissions(self, method): @@ -366,11 +381,9 @@ def test_update_components_permissions(self, method): random_user = UserFactory.create(username="Random", email="random@example.com") with self.as_user(random_user): resp = getattr(self.client, method)( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), - ) - assert resp.status_code == 403 - - resp = self.client.patch( - URL_LIB_COLLECTION_CONTENTS.format(lib_key=self.lib1.library_key, collection_id=self.col1.id), + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), ) assert resp.status_code == 403 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index c0a104101534..97b45772ea06 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -5,15 +5,13 @@ from __future__ import annotations from django.http import Http404 -from django.utils.translation import gettext as _ from rest_framework.decorators import action -from rest_framework.exceptions import ValidationError from rest_framework.response import Response from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED from rest_framework.viewsets import ModelViewSet -from openedx_learning.api.authoring_models import Collection, Component +from openedx_learning.api.authoring_models import Collection from openedx_learning.api import authoring as authoring_api from opaque_keys.edx.locator import LibraryLocatorV2 @@ -26,14 +24,18 @@ from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, - ContentLibraryCollectionContentsUpdateSerializer, + ContentLibraryCollectionComponentsUpdateSerializer, ContentLibraryCollectionCreateOrUpdateSerializer, ) +from openedx.core.djangoapps.content_libraries.views import convert_exceptions class LibraryCollectionsView(ModelViewSet): """ Views to get, create and update Library Collections. + + **Warning** These APIs are UNSTABLE, and may change once we implement the ability to store + units/sections/subsections in the library. """ serializer_class = ContentLibraryCollectionSerializer @@ -183,8 +185,9 @@ def destroy(self, request, *args, **kwargs): return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) - @action(detail=True, methods=['delete', 'patch'], url_path='contents', url_name='contents:update') - def update_contents(self, request, lib_key_str, pk=None): + @convert_exceptions + @action(detail=True, methods=['delete', 'patch'], url_path='components', url_name='components-update') + def update_components(self, request, lib_key_str, pk=None): """ Adds (PATCH) or removes (DELETE) Components to/from a Collection. @@ -197,23 +200,15 @@ def update_contents(self, request, lib_key_str, pk=None): permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, ) - serializer = ContentLibraryCollectionContentsUpdateSerializer(data=request.data) + serializer = ContentLibraryCollectionComponentsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) - try: - count = api.update_collection_contents( - library_obj, - collection_pk=pk, - usage_keys=serializer.validated_data["usage_keys"], - remove=(request.method == "DELETE"), - ) - except Collection.DoesNotExist as exc: - raise Http404() from exc - - except Component.DoesNotExist as exc: - # Only allows adding/removing components that are in the collection's learning package. - raise ValidationError({ - "usage_keys": _("Component(s) not found in library"), - }) from exc + count = api.update_collection_components( + library_obj, + collection_pk=pk, + usage_keys=serializer.validated_data["usage_keys"], + created_by=self.request.user.id, + remove=(request.method == "DELETE"), + ) return Response({'count': count}) diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index c3eb2f056f9c..6de602c63b36 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -293,7 +293,7 @@ def to_internal_value(self, value: str) -> UsageKeyV2: raise ValidationError from err -class ContentLibraryCollectionContentsUpdateSerializer(serializers.Serializer): +class ContentLibraryCollectionComponentsUpdateSerializer(serializers.Serializer): """ Serializer for adding/removing Components to/from a Collection. """ diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index bde8142d3fcc..820c30661c8b 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -142,6 +142,10 @@ def wrapped_fn(*args, **kwargs): except api.ContentLibraryBlockNotFound: log.exception("XBlock not found in content library") raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.ContentLibraryCollectionNotFound: + log.exception("Collection not found in content library") + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.LibraryBlockAlreadyExists as exc: log.exception(str(exc)) raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from From e8013ebd66197fd80ce24bdd5e4d89c76ae79cbe Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 29 Aug 2024 11:56:08 +0930 Subject: [PATCH 10/26] refactor: use oel_collections.get_collections as oel_collections.get_learning_package_collections has been removed. --- openedx/core/djangoapps/content_libraries/api.py | 4 +--- .../content_libraries/collections/rest_api/v1/views.py | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 09c872ff7dee..d07171c53253 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1087,9 +1087,7 @@ def update_collection_components( * ContentLibraryBlockNotFound if any of the given usage_keys don't match Components in the given library. """ learning_package = library.learning_package - collections_qset = authoring_api.get_learning_package_collections( - learning_package.id, - ).filter(pk=collection_pk) + collections_qset = authoring_api.get_collections(learning_package.id).filter(pk=collection_pk) collection = collections_qset.first() if not collection: diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 97b45772ea06..cd4de80593f7 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -51,7 +51,7 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, collection = None if library_obj.learning_package_id: - collection = authoring_api.get_learning_package_collections( + collection = authoring_api.get_collections( library_obj.learning_package_id ).filter(id=collection_id).first() return collection @@ -97,7 +97,7 @@ def list(self, request, *args, **kwargs): except api.ContentLibraryNotFound as exc: raise Http404 from exc - collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) + collections = authoring_api.get_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) From 94890dbd50caa6accc367ff1429ae57a6acd291b Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 29 Aug 2024 14:00:24 +0930 Subject: [PATCH 11/26] fix: pylint/type error --- openedx/core/djangoapps/content_libraries/api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index d07171c53253..79f76c36d5ed 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1086,6 +1086,9 @@ def update_collection_components( * ContentLibraryCollectionNotFound if no Collection with the given pk is found in the given library. * ContentLibraryBlockNotFound if any of the given usage_keys don't match Components in the given library. """ + if not usage_keys: + return 0 + learning_package = library.learning_package collections_qset = authoring_api.get_collections(learning_package.id).filter(pk=collection_pk) @@ -1115,9 +1118,6 @@ def update_collection_components( entities_qset = learning_package.publishable_entities.filter( key__in=component_keys, ) - if not entities_qset.count(): - usage_key = usage_keys[0] if usage_keys else None - raise ContentLibraryBlockNotFound(usage_key) if remove: count = authoring_api.remove_from_collections( From 6ded8a91deb1f99e5fddcda0d93e84170ecbb411 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 29 Aug 2024 14:00:52 +0930 Subject: [PATCH 12/26] test: adds tests for api.update_collection_components --- .../content_libraries/tests/test_api.py | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 87ae180d291e..4f89a9c833f3 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -13,8 +13,14 @@ UsageKey, ) from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx_events.content_authoring.data import ContentObjectData +from openedx_events.content_authoring.signals import CONTENT_OBJECT_TAGS_CHANGED +from openedx_events.tests.utils import OpenEdxEventsTestMixin +from openedx_learning.api import authoring as authoring_api from .. import api +from ..models import ContentLibrary +from .base import ContentLibrariesRestApiTest class EdxModulestoreImportClientTest(TestCase): @@ -241,3 +247,142 @@ def test_import_block_when_url_is_from_studio( block_olx ) mock_publish_changes.assert_not_called() + + +class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin): + """ + Tests for Content Library API collections methods. + + Same guidelines as ContentLibrariesTestCase. + """ + ENABLED_OPENEDX_EVENTS = [ + CONTENT_OBJECT_TAGS_CHANGED.event_type, + ] + + def setUp(self): + super().setUp() + + # Create Content Libraries + self._create_library("test-lib-col-1", "Test Library 1") + self._create_library("test-lib-col-2", "Test Library 2") + + # Fetch the created ContentLibrare objects so we can access their learning_package.id + self.lib1 = ContentLibrary.objects.get(slug="test-lib-col-1") + self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") + + # Create Content Library Collections + self.col1 = authoring_api.create_collection( + learning_package_id=self.lib1.learning_package.id, + title="Collection 1", + description="Description for Collection 1", + created_by=self.user.id, + ) + self.col2 = authoring_api.create_collection( + learning_package_id=self.lib2.learning_package.id, + title="Collection 2", + description="Description for Collection 2", + created_by=self.user.id, + ) + + # Create some library blocks in lib1 + self.lib1_problem_block = self._add_block_to_library( + self.lib1.library_key, "problem", "problem1", + ) + self.lib1_html_block = self._add_block_to_library( + self.lib1.library_key, "html", "html1", + ) + + def test_update_collection_components(self): + count = api.update_collection_components( + library=self.lib1, + collection_pk=self.col1.pk, + usage_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + UsageKey.from_string(self.lib1_html_block["id"]), + ], + ) + assert count == 2 + assert len(self.col1.entities.all()) == 2 + + count = api.update_collection_components( + library=self.lib1, + collection_pk=self.col1.pk, + usage_keys=[ + UsageKey.from_string(self.lib1_html_block["id"]), + ], + remove=True, + ) + assert count == 1 + self.col1.refresh_from_db() + assert len(self.col1.entities.all()) == 1 + + def test_update_collection_components_event(self): + """ + Check that a CONTENT_OBJECT_TAGS_CHANGED event is raised for each added/removed component. + """ + event_receiver = mock.Mock() + CONTENT_OBJECT_TAGS_CHANGED.connect(event_receiver) + api.update_collection_components( + library=self.lib1, + collection_pk=self.col1.pk, + usage_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + UsageKey.from_string(self.lib1_html_block["id"]), + ], + ) + + assert event_receiver.call_count == 2 + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_TAGS_CHANGED, + "sender": None, + "content_object": ContentObjectData( + object_id=UsageKey.from_string(self.lib1_problem_block["id"]), + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_TAGS_CHANGED, + "sender": None, + "content_object": ContentObjectData( + object_id=UsageKey.from_string(self.lib1_html_block["id"]), + ), + }, + event_receiver.call_args_list[1].kwargs, + ) + + def test_update_no_components(self): + """ + Calling update_collection_components with no usage_keys should not fail, just return 0. + """ + assert not api.update_collection_components( + library=self.lib1, + collection_pk=self.col1.pk, + usage_keys=[], + ) + + def test_update_collection_components_from_wrong_library(self): + with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: + api.update_collection_components( + library=self.lib2, + collection_pk=self.col2.pk, + usage_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + UsageKey.from_string(self.lib1_html_block["id"]), + ], + ) + assert self.lib1_problem_block["id"] in str(exc.exception) + + def test_update_collection_components_from_wrong_collection(self): + with self.assertRaises(api.ContentLibraryCollectionNotFound) as exc: + api.update_collection_components( + library=self.lib2, + collection_pk=self.col1.pk, + usage_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + UsageKey.from_string(self.lib1_html_block["id"]), + ], + ) + assert self.lib1_problem_block["id"] in str(exc.exception) From 0d8079c23f68a4ae0fc421e42f1bc93166ee546c Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 29 Aug 2024 14:32:15 +0930 Subject: [PATCH 13/26] test: fixes failing test --- .../djangoapps/content_libraries/tests/test_api.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 4f89a9c833f3..0c373f285055 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -259,6 +259,18 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTe CONTENT_OBJECT_TAGS_CHANGED.event_type, ] + @classmethod + def setUpClass(cls): + """ + Set up class method for the Test class. + + TODO: It's unclear why we need to call start_events_isolation ourselves rather than relying on + OpenEdxEventsTestMixin.setUpClass to handle it. It fails it we don't, and many other test cases do it, + so we're following a pattern here. But that pattern doesn't really make sense. + """ + super().setUpClass() + cls.start_events_isolation() + def setUp(self): super().setUp() From fe43b7204e60d96e691a39f78b02f33092778cf3 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 14 Aug 2024 07:36:15 +0300 Subject: [PATCH 14/26] feat: Add Library Collections REST endpoints --- .../core/djangoapps/content_libraries/api.py | 6 +- .../collections/rest_api/v1/views.py | 110 ++++++++++++++++++ .../content_libraries/serializers.py | 21 ++++ .../core/djangoapps/content_libraries/urls.py | 6 + 4 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 17bea80b3a96..42bc6d4879d2 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -86,7 +86,7 @@ LIBRARY_BLOCK_UPDATED, ) from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import Component, MediaType +from openedx_learning.api.authoring_models import Component, MediaType, LearningPackage from organizations.models import Organization from xblock.core import XBlock from xblock.exceptions import XBlockNotFoundError @@ -150,6 +150,7 @@ class ContentLibraryMetadata: Class that represents the metadata about a content library. """ key = attr.ib(type=LibraryLocatorV2) + learning_package = attr.ib(type=LearningPackage) title = attr.ib("") description = attr.ib("") num_blocks = attr.ib(0) @@ -323,6 +324,7 @@ def get_metadata(queryset, text_search=None): has_unpublished_changes=False, has_unpublished_deletes=False, license=lib.license, + learning_package=lib.learning_package, ) for lib in queryset ] @@ -408,6 +410,7 @@ def get_library(library_key): license=ref.license, created=learning_package.created, updated=learning_package.updated, + learning_package=learning_package ) @@ -479,6 +482,7 @@ def create_library( allow_public_learning=ref.allow_public_learning, allow_public_read=ref.allow_public_read, license=library_license, + learning_package=ref.learning_package ) diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py new file mode 100644 index 000000000000..9fefa29e6170 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -0,0 +1,110 @@ +""" +Collections API Views +""" + +from __future__ import annotations + +from django.http import Http404 + +# from rest_framework.generics import GenericAPIView +from rest_framework.response import Response +from rest_framework.viewsets import ModelViewSet +from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED + +from opaque_keys.edx.locator import LibraryLocatorV2 + +from openedx.core.djangoapps.content_libraries import api, permissions +from openedx.core.djangoapps.content_libraries.serializers import ( + ContentLibraryCollectionSerializer, + ContentLibraryCollectionCreateOrUpdateSerializer, +) + +from openedx_learning.api.authoring_models import Collection +from openedx_learning.api import authoring as authoring_api + + +class LibraryCollectionsView(ModelViewSet): + """ + Views to get, create and update Library Collections. + """ + + serializer_class = ContentLibraryCollectionSerializer + + def retrieve(self, request, lib_key_str, pk=None): + """ + Retrieve the Content Library Collection + """ + try: + collection = authoring_api.get_collection(pk) + except Collection.DoesNotExist as exc: + raise Http404 from exc + + # Check if user has permissions to view this collection by checking if + # user has permission to view the Content Library it belongs to + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + serializer = self.get_serializer(collection) + return Response(serializer.data) + + def list(self, request, lib_key_str): + """ + List Collections that belong to Content Library + """ + # Check if user has permissions to view collections by checking if user + # has permission to view the Content Library they belong to + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + content_library = api.get_library(library_key) + collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) + serializer = self.get_serializer(collections, many=True) + return Response(serializer.data) + + def create(self, request, lib_key_str): + """ + Create a Collection that belongs to a Content Library + """ + # Check if user has permissions to create a collection in the Content Library + # by checking if user has permission to edit the Content Library + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) + create_serializer.is_valid(raise_exception=True) + content_library = api.get_library(library_key) + collection = authoring_api.create_collection( + content_library.learning_package.id, + create_serializer.validated_data["title"], + request.user.id, + create_serializer.validated_data["description"] + ) + serializer = self.get_serializer(collection) + return Response(serializer.data) + + def partial_update(self, request, lib_key_str, pk=None): + """ + Update a Collection that belongs to a Content Library + """ + # Check if user has permissions to update a collection in the Content Library + # by checking if user has permission to edit the Content Library + library_key = LibraryLocatorV2.from_string(lib_key_str) + api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + + try: + collection = authoring_api.get_collection(pk) + except Collection.DoesNotExist as exc: + raise Http404 from exc + + update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( + collection, data=request.data, partial=True + ) + update_serializer.is_valid(raise_exception=True) + updated_collection = authoring_api.update_collection(pk, **update_serializer.validated_data) + serializer = self.get_serializer(updated_collection) + return Response(serializer.data) + + def destroy(self, request, lib_key_str, pk=None): + """ + Deletes a Collection that belongs to a Content Library + + Note: (currently not allowed) + """ + return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 497eda81475b..7c49d4af3c2b 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -5,6 +5,8 @@ from django.core.validators import validate_unicode_slug from rest_framework import serializers + +from openedx_learning.api.authoring_models import Collection from openedx.core.djangoapps.content_libraries.constants import ( LIBRARY_TYPES, COMPLEX, @@ -245,3 +247,22 @@ class ContentLibraryBlockImportTaskCreateSerializer(serializers.Serializer): """ course_key = CourseKeyField() + + +class ContentLibraryCollectionSerializer(serializers.ModelSerializer): + """ + Serializer for a Content Library Collection + """ + + class Meta: + model = Collection + fields = '__all__' + + +class ContentLibraryCollectionCreateOrUpdateSerializer(serializers.Serializer): + """ + Serializer for add/update a Collection in a Content Library + """ + + title = serializers.CharField() + description = serializers.CharField() diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 6e450df63522..5521ad05bf63 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -7,6 +7,7 @@ from rest_framework import routers from . import views +from .collections.rest_api.v1 import views as collection_views # Django application name. @@ -18,6 +19,9 @@ import_blocks_router = routers.DefaultRouter() import_blocks_router.register(r'tasks', views.LibraryImportTaskViewSet, basename='import-block-task') +library_collections_router = routers.DefaultRouter() +library_collections_router.register(r'collections', collection_views.LibraryCollectionsView, basename="library-collections") + # These URLs are only used in Studio. The LMS already provides all the # API endpoints needed to serve XBlocks from content libraries using the # standard XBlock REST API (see openedx.core.django_apps.xblock.rest_api.urls) @@ -45,6 +49,8 @@ path('import_blocks/', include(import_blocks_router.urls)), # Paste contents of clipboard into library path('paste_clipboard/', views.LibraryPasteClipboardView.as_view()), + # Library Collections + path('', include(library_collections_router.urls)), ])), path('blocks//', include([ # Get metadata about a specific XBlock in this library, or delete the block: From f5900be59a0cd39ad68685f80c7e53f78601eff5 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 22 Aug 2024 08:33:45 +0300 Subject: [PATCH 15/26] test: Add tests for Collections REST APIs --- .../collections/rest_api/v1/tests/__init__.py | 0 .../rest_api/v1/tests/test_views.py | 184 ++++++++++++++++++ .../core/djangoapps/content_libraries/urls.py | 4 +- 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py new file mode 100644 index 000000000000..3f37ad3a1c08 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -0,0 +1,184 @@ +""" +Tests Library Collections REST API views +""" + +from __future__ import annotations + +from openedx_learning.api.authoring_models import Collection + +from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from openedx.core.djangoapps.content_libraries.models import ContentLibrary +from common.djangoapps.student.tests.factories import UserFactory + +URL_PREFIX = '/api/libraries/v2/{lib_key}/' +URL_LIB_COLLECTIONS = URL_PREFIX + 'collections/' +URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_id}/' + + +@skip_unless_cms # Content Library Collections REST API is only available in Studio +class ContentLibraryCollectionsViewsTest(ContentLibrariesRestApiTest): + """ + Tests for Content Library Collection REST API Views + """ + + def setUp(self): + super().setUp() + + # Create Content Libraries + self._create_library("test-lib-col-1", "Test Library 1") + self._create_library("test-lib-col-2", "Test Library 2") + self.lib1 = ContentLibrary.objects.get(slug="test-lib-col-1") + self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") + + print("self.lib1", self.lib1) + print("self.lib2", self.lib2) + + # Create Content Library Collections + self.col1 = Collection.objects.create( + learning_package_id=self.lib1.learning_package.id, + title="Collection 1", + description="Description for Collection 1", + created_by=self.user, + ) + self.col2 = Collection.objects.create( + learning_package_id=self.lib1.learning_package.id, + title="Collection 2", + description="Description for Collection 2", + created_by=self.user, + ) + self.col3 = Collection.objects.create( + learning_package_id=self.lib2.learning_package.id, + title="Collection 3", + description="Description for Collection 3", + created_by=self.user, + ) + + def test_get_library_collection(self): + """ + Test retrieving a Content Library Collection + """ + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) + ) + + # Check that correct Content Library Collection data retrieved + expected_collection = { + "title": "Collection 3", + "description": "Description for Collection 3", + } + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, expected_collection) + + # Check that a random user without permissions cannot access Content Library Collection + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) + ) + assert resp.status_code == 403 + + def test_list_library_collections(self): + """ + Test listing Content Library Collections + """ + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) + + # Check that the correct collections are listed + assert resp.status_code == 200 + assert len(resp.data) == 2 + expected_collections = [ + {"title": "Collection 1", "description": "Description for Collection 1"}, + {"title": "Collection 2", "description": "Description for Collection 2"}, + ] + for collection, expected in zip(resp.data, expected_collections): + self.assertDictContainsEntries(collection, expected) + + # Check that a random user without permissions cannot access Content Library Collections + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) + assert resp.status_code == 403 + + def test_create_library_collection(self): + """ + Test creating a Content Library Collection + """ + post_data = { + "title": "Collection 4", + "description": "Description for Collection 4", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + + # Check that the new Content Library Collection is returned in response and created in DB + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, post_data) + + created_collection = Collection.objects.get(id=resp.data["id"]) + self.assertIsNotNone(created_collection) + + # Check that user with read only access cannot create new Content Library Collection + reader = UserFactory.create(username="Reader", email="reader@example.com") + self._add_user_by_email(self.lib1.library_key, reader.email, access_level="read") + + with self.as_user(reader): + post_data = { + "title": "Collection 5", + "description": "Description for Collection 5", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + + assert resp.status_code == 403 + + def test_update_library_collection(self): + """ + Test updating a Content Library Collection + """ + patch_data = { + "title": "Collection 3 Updated", + } + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + patch_data, + format="json" + ) + + # Check that updated Content Library Collection is returned in response and updated in DB + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, patch_data) + + created_collection = Collection.objects.get(id=resp.data["id"]) + self.assertIsNotNone(created_collection) + self.assertEqual(created_collection.title, patch_data["title"]) + + # Check that user with read only access cannot update a Content Library Collection + reader = UserFactory.create(username="Reader", email="reader@example.com") + self._add_user_by_email(self.lib1.library_key, reader.email, access_level="read") + + with self.as_user(reader): + patch_data = { + "title": "Collection 3 should not update", + } + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + patch_data, + format="json" + ) + + assert resp.status_code == 403 + + def test_delete_library_collection(self): + """ + Test deleting a Content Library Collection + + Note: Currently not implemented and should return a 405 + """ + resp = self.client.delete( + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) + ) + + assert resp.status_code == 405 diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 5521ad05bf63..a0c5a9f8cd61 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -20,7 +20,9 @@ import_blocks_router.register(r'tasks', views.LibraryImportTaskViewSet, basename='import-block-task') library_collections_router = routers.DefaultRouter() -library_collections_router.register(r'collections', collection_views.LibraryCollectionsView, basename="library-collections") +library_collections_router.register( + r'collections', collection_views.LibraryCollectionsView, basename="library-collections" +) # These URLs are only used in Studio. The LMS already provides all the # API endpoints needed to serve XBlocks from content libraries using the From 97739c6fe0a8151a38eba820b277628c69df3442 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 23 Aug 2024 10:00:56 +0300 Subject: [PATCH 16/26] chore: Add missing __init__ files --- .../djangoapps/content_libraries/collections/rest_api/__init__.py | 0 .../content_libraries/collections/rest_api/v1/__init__.py | 0 2 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py create mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 From 2996a2e64d145383d38cc72a01cfe17565f62b2f Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 23 Aug 2024 13:46:23 +0300 Subject: [PATCH 17/26] feat: Verify collection belongs to library --- .../core/djangoapps/content_libraries/api.py | 6 +- .../rest_api/v1/tests/test_views.py | 78 ++++++++++++++++++- .../collections/rest_api/v1/views.py | 61 +++++++++++---- 3 files changed, 123 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 42bc6d4879d2..9a4ccbd6456e 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -331,7 +331,7 @@ def get_metadata(queryset, text_search=None): return libraries -def require_permission_for_library_key(library_key, user, permission): +def require_permission_for_library_key(library_key, user, permission) -> ContentLibrary: """ Given any of the content library permission strings defined in openedx.core.djangoapps.content_libraries.permissions, @@ -341,10 +341,12 @@ def require_permission_for_library_key(library_key, user, permission): Raises django.core.exceptions.PermissionDenied if the user doesn't have permission. """ - library_obj = ContentLibrary.objects.get_by_key(library_key) + library_obj = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] if not user.has_perm(permission, obj=library_obj): raise PermissionDenied + return library_obj + def get_library(library_key): """ diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py index 3f37ad3a1c08..dbaa5f4c1cf9 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -5,6 +5,7 @@ from __future__ import annotations from openedx_learning.api.authoring_models import Collection +from opaque_keys.edx.locator import LibraryLocatorV2 from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest @@ -31,9 +32,6 @@ def setUp(self): self.lib1 = ContentLibrary.objects.get(slug="test-lib-col-1") self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") - print("self.lib1", self.lib1) - print("self.lib2", self.lib2) - # Create Content Library Collections self.col1 = Collection.objects.create( learning_package_id=self.lib1.learning_package.id, @@ -78,6 +76,24 @@ def test_get_library_collection(self): ) assert resp.status_code == 403 + def test_get_invalid_library_collection(self): + """ + Test retrieving a an invalid Content Library Collection or one that does not exist + """ + # Fetch collection that belongs to a different library, it should fail + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=self.col3.id) + ) + + assert resp.status_code == 404 + + # Fetch collection with invalid ID provided, it should fail + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=123) + ) + + assert resp.status_code == 404 + def test_list_library_collections(self): """ Test listing Content Library Collections @@ -100,6 +116,15 @@ def test_list_library_collections(self): resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) assert resp.status_code == 403 + def test_list_invalid_library_collections(self): + """ + Test listing invalid Content Library Collections + """ + invalid_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=invalid_key)) + + assert resp.status_code == 404 + def test_create_library_collection(self): """ Test creating a Content Library Collection @@ -134,6 +159,28 @@ def test_create_library_collection(self): assert resp.status_code == 403 + def test_create_invalid_library_collection(self): + """ + Test creating an invalid Content Library Collection + """ + post_data_missing_title = { + "description": "Description for Collection 4", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_title, format="json" + ) + + assert resp.status_code == 400 + + post_data_missing_desc = { + "title": "Collection 4", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_desc, format="json" + ) + + assert resp.status_code == 400 + def test_update_library_collection(self): """ Test updating a Content Library Collection @@ -171,6 +218,31 @@ def test_update_library_collection(self): assert resp.status_code == 403 + def test_update_invalid_library_collection(self): + """ + Test updating an invalid Content Library Collection or one that does not exist + """ + patch_data = { + "title": "Collection 3 Updated", + } + # Update collection that belongs to a different library, it should fail + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=self.col3.id), + patch_data, + format="json" + ) + + assert resp.status_code == 404 + + # Update collection with invalid ID provided, it should fail + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=123), + patch_data, + format="json" + ) + + assert resp.status_code == 404 + def test_delete_library_collection(self): """ Test deleting a Content Library Collection diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 9fefa29e6170..2c4df17e6d08 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -6,7 +6,6 @@ from django.http import Http404 -# from rest_framework.generics import GenericAPIView from rest_framework.response import Response from rest_framework.viewsets import ModelViewSet from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED @@ -30,19 +29,37 @@ class LibraryCollectionsView(ModelViewSet): serializer_class = ContentLibraryCollectionSerializer + def _verify_and_fetch_library_collection(self, library_key, collection_id, user, permission) -> Collection | None: + """ + Verify that the collection belongs to the library and the user has the correct permissions + """ + try: + library_obj = api.require_permission_for_library_key(library_key, user, permission) + except api.ContentLibraryNotFound: + raise Http404 + + collection = None + if library_obj.learning_package_id: + collection = authoring_api.get_learning_package_collections( + library_obj.learning_package_id + ).filter(id=collection_id).first() + return collection + def retrieve(self, request, lib_key_str, pk=None): """ Retrieve the Content Library Collection """ - try: - collection = authoring_api.get_collection(pk) - except Collection.DoesNotExist as exc: - raise Http404 from exc + library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to view this collection by checking if # user has permission to view the Content Library it belongs to - library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + collection = self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + ) + + if not collection: + raise Http404 + serializer = self.get_serializer(collection) return Response(serializer.data) @@ -53,8 +70,13 @@ def list(self, request, lib_key_str): # Check if user has permissions to view collections by checking if user # has permission to view the Content Library they belong to library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) - content_library = api.get_library(library_key) + try: + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + ) + except api.ContentLibraryNotFound: + raise Http404 + collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) @@ -66,10 +88,15 @@ def create(self, request, lib_key_str): # Check if user has permissions to create a collection in the Content Library # by checking if user has permission to edit the Content Library library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + try: + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) + except api.ContentLibraryNotFound: + raise Http404 + create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) - content_library = api.get_library(library_key) collection = authoring_api.create_collection( content_library.learning_package.id, create_serializer.validated_data["title"], @@ -83,15 +110,15 @@ def partial_update(self, request, lib_key_str, pk=None): """ Update a Collection that belongs to a Content Library """ + library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to update a collection in the Content Library # by checking if user has permission to edit the Content Library - library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + collection = self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) - try: - collection = authoring_api.get_collection(pk) - except Collection.DoesNotExist as exc: - raise Http404 from exc + if not collection: + raise Http404 update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( collection, data=request.data, partial=True From fbfd98323d14aef7561c2f0f69bb4b8e74e1e7dd Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 26 Aug 2024 07:42:19 +0300 Subject: [PATCH 18/26] feat: Add events emitting for Collections --- docs/hooks/events.rst | 12 ++++ .../core/djangoapps/content_libraries/apps.py | 1 + .../content_libraries/collections/__init__.py | 0 .../content_libraries/collections/handlers.py | 57 +++++++++++++++++++ .../collections/rest_api/v1/views.py | 26 +++++++++ requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 4 +- requirements/edx/testing.txt | 2 +- 10 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/collections/__init__.py create mode 100644 openedx/core/djangoapps/content_libraries/collections/handlers.py diff --git a/docs/hooks/events.rst b/docs/hooks/events.rst index 7f9584c9e8e1..2bceee5050ea 100644 --- a/docs/hooks/events.rst +++ b/docs/hooks/events.rst @@ -247,3 +247,15 @@ Content Authoring Events * - `CONTENT_OBJECT_TAGS_CHANGED `_ - org.openedx.content_authoring.content.object.tags.changed.v1 - 2024-03-31 + + * - `LIBRARY_COLLECTION_CREATED `_ + - org.openedx.content_authoring.content.library.collection.created.v1 + - 2024-08-23 + + * - `LIBRARY_COLLECTION_UPDATED `_ + - org.openedx.content_authoring.content.library.collection.updated.v1 + - 2024-08-23 + + * - `LIBRARY_COLLECTION_DELETED `_ + - org.openedx.content_authoring.content.library.collection.deleted.v1 + - 2024-08-23 diff --git a/openedx/core/djangoapps/content_libraries/apps.py b/openedx/core/djangoapps/content_libraries/apps.py index 52c3e5179721..aea920714ce0 100644 --- a/openedx/core/djangoapps/content_libraries/apps.py +++ b/openedx/core/djangoapps/content_libraries/apps.py @@ -37,3 +37,4 @@ def ready(self): Import signal handler's module to ensure they are registered. """ from . import signal_handlers # pylint: disable=unused-import + from .collections import handlers # pylint: disable=unused-import diff --git a/openedx/core/djangoapps/content_libraries/collections/__init__.py b/openedx/core/djangoapps/content_libraries/collections/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/core/djangoapps/content_libraries/collections/handlers.py b/openedx/core/djangoapps/content_libraries/collections/handlers.py new file mode 100644 index 000000000000..c2c67ad403e5 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/collections/handlers.py @@ -0,0 +1,57 @@ +""" +Signal handlers for Content Library Collections. +""" + +import logging + +from django.dispatch import receiver +from openedx_events.content_authoring.data import LibraryCollectionData +from openedx_events.content_authoring.signals import ( + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, + LIBRARY_COLLECTION_DELETED, +) + + +log = logging.getLogger(__name__) + + +@receiver(LIBRARY_COLLECTION_CREATED) +def library_collection_created_handler(**kwargs): + """ + Content Library Collection Created signal handler + """ + library_collection_data = kwargs.get("library_collection", None) + if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): + log.error("Received null or incorrect data for event") + return + + log.info("Received Collection Created Signal") + + # TODO: Implement handler logic + + +@receiver(LIBRARY_COLLECTION_UPDATED) +def library_collection_updated_handler(**kwargs): + """ + Content Library Collection Updated signal handler + """ + library_collection_data = kwargs.get("library_collection", None) + if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): + log.error("Received null or incorrect data for event") + return + + log.info("Received Collection Updated Signal") + + +@receiver(LIBRARY_COLLECTION_DELETED) +def library_collection_deleted_handler(**kwargs): + """ + Content Library Collection Deleted signal handler + """ + library_collection_data = kwargs.get("library_collection", None) + if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): + log.error("Received null or incorrect data for event") + return + + log.info("Received Collection Deleted Signal") diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 2c4df17e6d08..b20c00a48c96 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -12,6 +12,12 @@ from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx_events.content_authoring.data import LibraryCollectionData +from openedx_events.content_authoring.signals import ( + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, +) + from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, @@ -104,6 +110,15 @@ def create(self, request, lib_key_str): create_serializer.validated_data["description"] ) serializer = self.get_serializer(collection) + + # Emit event for library content collection creation + LIBRARY_COLLECTION_CREATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_id=collection.id + ) + ) + return Response(serializer.data) def partial_update(self, request, lib_key_str, pk=None): @@ -126,6 +141,15 @@ def partial_update(self, request, lib_key_str, pk=None): update_serializer.is_valid(raise_exception=True) updated_collection = authoring_api.update_collection(pk, **update_serializer.validated_data) serializer = self.get_serializer(updated_collection) + + # Emit event for library content collection updated + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_id=collection.id + ) + ) + return Response(serializer.data) def destroy(self, request, lib_key_str, pk=None): @@ -134,4 +158,6 @@ def destroy(self, request, lib_key_str, pk=None): Note: (currently not allowed) """ + # TODO: Implement the deletion logic and emit event signal + return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 640ccbe982e7..7779b553a1b8 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -811,7 +811,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/kernel.in openedx-django-wiki==2.1.0 # via -r requirements/edx/kernel.in -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/kernel.in # edx-event-bus-kafka diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 9ae45b7ff72c..c01a427dcd10 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1358,7 +1358,7 @@ openedx-django-wiki==2.1.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index d315955694e7..d718495039f7 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -970,7 +970,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/base.txt # edx-event-bus-kafka diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index a5b510742ac7..5154dc293efa 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -117,7 +117,9 @@ olxcleaner openedx-atlas # CLI tool to manage translations openedx-calc # Library supporting mathematical calculations for Open edX openedx-django-require -openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) +# openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) +# TODO: Remove this once new version released +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) openedx-learning # Open edX Learning core (experimental) openedx-mongodbproxy diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index fcb7db05a5ef..2e0d6ea2d61d 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1021,7 +1021,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==9.11.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 # via # -r requirements/edx/base.txt # edx-event-bus-kafka From cf9e90634c48f3aac39bda580501aff7850090d1 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 26 Aug 2024 08:43:40 +0300 Subject: [PATCH 19/26] chore: fix pylint errors --- .../collections/rest_api/v1/views.py | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index b20c00a48c96..0d89c4b8a2c7 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -41,8 +41,8 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, """ try: library_obj = api.require_permission_for_library_key(library_key, user, permission) - except api.ContentLibraryNotFound: - raise Http404 + except api.ContentLibraryNotFound as exc: + raise Http404 from exc collection = None if library_obj.learning_package_id: @@ -51,10 +51,15 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, ).filter(id=collection_id).first() return collection - def retrieve(self, request, lib_key_str, pk=None): + def retrieve(self, request, *args, **kwargs): """ Retrieve the Content Library Collection """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + + pk = kwargs.pop("pk", None) library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to view this collection by checking if @@ -69,10 +74,14 @@ def retrieve(self, request, lib_key_str, pk=None): serializer = self.get_serializer(collection) return Response(serializer.data) - def list(self, request, lib_key_str): + def list(self, request, *args, **kwargs): """ List Collections that belong to Content Library """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + # Check if user has permissions to view collections by checking if user # has permission to view the Content Library they belong to library_key = LibraryLocatorV2.from_string(lib_key_str) @@ -80,17 +89,21 @@ def list(self, request, lib_key_str): content_library = api.require_permission_for_library_key( library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) - except api.ContentLibraryNotFound: - raise Http404 + except api.ContentLibraryNotFound as exc: + raise Http404 from exc collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) - def create(self, request, lib_key_str): + def create(self, request, *args, **kwargs): """ Create a Collection that belongs to a Content Library """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + # Check if user has permissions to create a collection in the Content Library # by checking if user has permission to edit the Content Library library_key = LibraryLocatorV2.from_string(lib_key_str) @@ -98,8 +111,8 @@ def create(self, request, lib_key_str): content_library = api.require_permission_for_library_key( library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - except api.ContentLibraryNotFound: - raise Http404 + except api.ContentLibraryNotFound as exc: + raise Http404 from exc create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) @@ -121,10 +134,15 @@ def create(self, request, lib_key_str): return Response(serializer.data) - def partial_update(self, request, lib_key_str, pk=None): + def partial_update(self, request, *args, **kwargs): """ Update a Collection that belongs to a Content Library """ + lib_key_str = kwargs.pop('lib_key_str', None) + if not lib_key_str: + raise Http404 + + pk = kwargs.pop('pk', None) library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to update a collection in the Content Library # by checking if user has permission to edit the Content Library @@ -152,7 +170,7 @@ def partial_update(self, request, lib_key_str, pk=None): return Response(serializer.data) - def destroy(self, request, lib_key_str, pk=None): + def destroy(self, request, *args, **kwargs): """ Deletes a Collection that belongs to a Content Library From 5bf94226fcbb778a28d63bb11ae10c43061f2a8b Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 29 Aug 2024 08:49:27 +0300 Subject: [PATCH 20/26] refactor: Use convert_exceptions + update tests --- .../rest_api/v1/tests/test_views.py | 44 +++++++++++++++---- .../collections/rest_api/v1/views.py | 30 ++++++------- .../djangoapps/content_libraries/utils.py | 44 +++++++++++++++++++ .../djangoapps/content_libraries/views.py | 30 +------------ 4 files changed, 94 insertions(+), 54 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/utils.py diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py index dbaa5f4c1cf9..1b4ce1a1b2fa 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py @@ -5,6 +5,7 @@ from __future__ import annotations from openedx_learning.api.authoring_models import Collection +from openedx_learning.api.authoring import create_collection from opaque_keys.edx.locator import LibraryLocatorV2 from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -33,23 +34,24 @@ def setUp(self): self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") # Create Content Library Collections - self.col1 = Collection.objects.create( + self.col1 = create_collection( learning_package_id=self.lib1.learning_package.id, title="Collection 1", + created_by=self.user.id, description="Description for Collection 1", - created_by=self.user, ) - self.col2 = Collection.objects.create( + + self.col2 = create_collection( learning_package_id=self.lib1.learning_package.id, title="Collection 2", + created_by=self.user.id, description="Description for Collection 2", - created_by=self.user, ) - self.col3 = Collection.objects.create( + self.col3 = create_collection( learning_package_id=self.lib2.learning_package.id, title="Collection 3", + created_by=self.user.id, description="Description for Collection 3", - created_by=self.user, ) def test_get_library_collection(self): @@ -94,6 +96,12 @@ def test_get_invalid_library_collection(self): assert resp.status_code == 404 + # Fetch collection with invalid library_key provided, it should fail + resp = self.client.get( + URL_LIB_COLLECTION.format(lib_key=123, collection_id=123) + ) + assert resp.status_code == 404 + def test_list_library_collections(self): """ Test listing Content Library Collections @@ -120,11 +128,15 @@ def test_list_invalid_library_collections(self): """ Test listing invalid Content Library Collections """ - invalid_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") - resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=invalid_key)) + non_existing_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") + resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=non_existing_key)) assert resp.status_code == 404 + # List collections with invalid library_key provided, it should fail + resp = resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=123)) + assert resp.status_code == 404 + def test_create_library_collection(self): """ Test creating a Content Library Collection @@ -181,6 +193,14 @@ def test_create_invalid_library_collection(self): assert resp.status_code == 400 + # Create collection with invalid library_key provided, it should fail + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=123), + {**post_data_missing_title, **post_data_missing_desc}, + format="json" + ) + assert resp.status_code == 404 + def test_update_library_collection(self): """ Test updating a Content Library Collection @@ -243,6 +263,14 @@ def test_update_invalid_library_collection(self): assert resp.status_code == 404 + # Update collection with invalid library_key provided, it should fail + resp = self.client.patch( + URL_LIB_COLLECTION.format(lib_key=123, collection_id=self.col3.id), + patch_data, + format="json" + ) + assert resp.status_code == 404 + def test_delete_library_collection(self): """ Test deleting a Content Library Collection diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index 0d89c4b8a2c7..7484b0893cff 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -19,6 +19,7 @@ ) from openedx.core.djangoapps.content_libraries import api, permissions +from openedx.core.djangoapps.content_libraries.utils import convert_exceptions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, ContentLibraryCollectionCreateOrUpdateSerializer, @@ -39,11 +40,7 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, """ Verify that the collection belongs to the library and the user has the correct permissions """ - try: - library_obj = api.require_permission_for_library_key(library_key, user, permission) - except api.ContentLibraryNotFound as exc: - raise Http404 from exc - + library_obj = api.require_permission_for_library_key(library_key, user, permission) collection = None if library_obj.learning_package_id: collection = authoring_api.get_learning_package_collections( @@ -51,6 +48,7 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, ).filter(id=collection_id).first() return collection + @convert_exceptions def retrieve(self, request, *args, **kwargs): """ Retrieve the Content Library Collection @@ -74,6 +72,7 @@ def retrieve(self, request, *args, **kwargs): serializer = self.get_serializer(collection) return Response(serializer.data) + @convert_exceptions def list(self, request, *args, **kwargs): """ List Collections that belong to Content Library @@ -85,17 +84,15 @@ def list(self, request, *args, **kwargs): # Check if user has permissions to view collections by checking if user # has permission to view the Content Library they belong to library_key = LibraryLocatorV2.from_string(lib_key_str) - try: - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY - ) - except api.ContentLibraryNotFound as exc: - raise Http404 from exc + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + ) collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) + @convert_exceptions def create(self, request, *args, **kwargs): """ Create a Collection that belongs to a Content Library @@ -107,12 +104,9 @@ def create(self, request, *args, **kwargs): # Check if user has permissions to create a collection in the Content Library # by checking if user has permission to edit the Content Library library_key = LibraryLocatorV2.from_string(lib_key_str) - try: - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) - except api.ContentLibraryNotFound as exc: - raise Http404 from exc + content_library = api.require_permission_for_library_key( + library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) @@ -134,6 +128,7 @@ def create(self, request, *args, **kwargs): return Response(serializer.data) + @convert_exceptions def partial_update(self, request, *args, **kwargs): """ Update a Collection that belongs to a Content Library @@ -170,6 +165,7 @@ def partial_update(self, request, *args, **kwargs): return Response(serializer.data) + @convert_exceptions def destroy(self, request, *args, **kwargs): """ Deletes a Collection that belongs to a Content Library diff --git a/openedx/core/djangoapps/content_libraries/utils.py b/openedx/core/djangoapps/content_libraries/utils.py new file mode 100644 index 000000000000..bdfb1337d7bf --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/utils.py @@ -0,0 +1,44 @@ +""" Utils used for the content libraries. """ + +from functools import wraps +import logging + +from rest_framework.exceptions import NotFound, ValidationError + +from opaque_keys import InvalidKeyError + +from openedx.core.djangoapps.content_libraries import api + + +log = logging.getLogger(__name__) + + +def convert_exceptions(fn): + """ + Catch any Content Library API exceptions that occur and convert them to + DRF exceptions so DRF will return an appropriate HTTP response + """ + + @wraps(fn) + def wrapped_fn(*args, **kwargs): + try: + return fn(*args, **kwargs) + except InvalidKeyError as exc: + log.exception(str(exc)) + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.ContentLibraryNotFound: + log.exception("Content library not found") + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.ContentLibraryBlockNotFound: + log.exception("XBlock not found in content library") + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.LibraryBlockAlreadyExists as exc: + log.exception(str(exc)) + raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from + except api.InvalidNameError as exc: + log.exception(str(exc)) + raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from + except api.BlockLimitReachedError as exc: + log.exception(str(exc)) + raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from + return wrapped_fn diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index bde8142d3fcc..11e5c9d23b2e 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -63,7 +63,6 @@ https://github.com/openedx/edx-platform/pull/30456 """ -from functools import wraps import itertools import json import logging @@ -120,40 +119,13 @@ from openedx.core.djangoapps.xblock import api as xblock_api from .models import ContentLibrary, LtiGradedResource, LtiProfile +from .utils import convert_exceptions User = get_user_model() log = logging.getLogger(__name__) -def convert_exceptions(fn): - """ - Catch any Content Library API exceptions that occur and convert them to - DRF exceptions so DRF will return an appropriate HTTP response - """ - - @wraps(fn) - def wrapped_fn(*args, **kwargs): - try: - return fn(*args, **kwargs) - except api.ContentLibraryNotFound: - log.exception("Content library not found") - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - except api.ContentLibraryBlockNotFound: - log.exception("XBlock not found in content library") - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - except api.LibraryBlockAlreadyExists as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - except api.InvalidNameError as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - except api.BlockLimitReachedError as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - return wrapped_fn - - class LibraryApiPaginationDocs: """ API docs for query params related to paginating ContentLibraryMetadata objects. From 80348617a5ff605dfc2e663ea908c95fdc5ba05e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 29 Aug 2024 17:20:51 +0930 Subject: [PATCH 21/26] refactor: index collections within each library so we can use the updated oel_collections.get_collections method. --- openedx/core/djangoapps/content/search/api.py | 76 +++++++++---------- 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 9fb49b24b6d1..76bb5eb3f4a4 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -296,16 +296,12 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: status_cb("Counting courses...") num_courses = CourseOverview.objects.count() - # Get the list of collections - status_cb("Counting collections...") - num_collections = authoring_api.get_collections().count() - # Some counters so we can track our progress as indexing progresses: - num_contexts = num_courses + num_libraries + num_collections + num_contexts = num_courses + num_libraries num_contexts_done = 0 # How many courses/libraries we've indexed num_blocks_done = 0 # How many individual components/XBlocks we've indexed - status_cb(f"Found {num_courses} courses, {num_libraries} libraries and {num_collections} collections.") + status_cb(f"Found {num_courses} courses, {num_libraries} libraries.") with _using_temp_index(status_cb) as temp_index_name: ############## Configure the index ############## @@ -390,10 +386,43 @@ def index_library(lib_key: str) -> list: status_cb(f"Error indexing library {lib_key}: {err}") return docs + ############## Collections ############## + def index_collection_batch(batch, num_done) -> int: + docs = [] + for collection in batch: + try: + doc = searchable_doc_for_collection(collection) + # Uncomment below line once collections are tagged. + # doc.update(searchable_doc_tags(collection.id)) + docs.append(doc) + except Exception as err: # pylint: disable=broad-except + status_cb(f"Error indexing collection {collection}: {err}") + num_done += 1 + + if docs: + try: + # Add docs in batch of 100 at once (usually faster than adding one at a time): + _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) + except (TypeError, KeyError, MeilisearchError) as err: + status_cb(f"Error indexing collection batch {p}: {err}") + return num_done + for lib_key in lib_keys: - status_cb(f"{num_contexts_done + 1}/{num_contexts}. Now indexing library {lib_key}") + status_cb(f"{num_contexts_done + 1}/{num_contexts}. Now indexing blocks in library {lib_key}") lib_docs = index_library(lib_key) num_blocks_done += len(lib_docs) + + # To reduce memory usage on large instances, split up the Collections into pages of 100 collections: + library = lib_api.get_library(lib_key) + collections = authoring_api.get_collections(library.learning_package.id, enabled=True) + num_collections = collections.count() + num_collections_done = 0 + status_cb(f"{num_collections_done + 1}/{num_collections}. Now indexing collections in library {lib_key}") + paginator = Paginator(collections, 100) + for p in paginator.page_range: + num_collections_done = index_collection_batch(paginator.page(p).object_list, num_collections_done) + status_cb(f"{num_collections_done}/{num_collections} collections indexed for library {lib_key}") + num_contexts_done += 1 ############## Courses ############## @@ -430,39 +459,6 @@ def add_with_children(block): num_contexts_done += 1 num_blocks_done += len(course_docs) - ############## Collections ############## - status_cb("Indexing collections...") - - def index_collection_batch(batch, num_contexts_done) -> int: - docs = [] - for collection in batch: - status_cb( - f"{num_contexts_done + 1}/{num_contexts}. " - f"Now indexing collection {collection.title} ({collection.id})" - ) - try: - doc = searchable_doc_for_collection(collection) - # Uncomment below line once collections are tagged. - # doc.update(searchable_doc_tags(collection.id)) - docs.append(doc) - except Exception as err: # pylint: disable=broad-except - status_cb(f"Error indexing collection {collection}: {err}") - finally: - num_contexts_done += 1 - - if docs: - try: - # Add docs in batch of 100 at once (usually faster than adding one at a time): - _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) - except (TypeError, KeyError, MeilisearchError) as err: - status_cb(f"Error indexing collection batch {p}: {err}") - return num_contexts_done - - # To reduce memory usage on large instances, split up the Collections into pages of 100 collections: - paginator = Paginator(authoring_api.get_collections(enabled=True), 100) - for p in paginator.page_range: - num_contexts_done = index_collection_batch(paginator.page(p).object_list, num_contexts_done) - status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses, collections and libraries.") From 55338e1c2a5f24d532d9273ba5515eef979efbf4 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 30 Aug 2024 14:15:53 +0930 Subject: [PATCH 22/26] test: fix flaky test --- .../djangoapps/content/search/tests/test_api.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 549817700370..23840997b87b 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -177,8 +177,15 @@ def setUp(self): # Create a collection: self.learning_package = authoring_api.get_learning_package_by_key(self.library.key) + with freeze_time(created_date): + self.collection = authoring_api.create_collection( + learning_package_id=self.learning_package.id, + title="my_collection", + created_by=None, + description="my collection description" + ) self.collection_dict = { - 'id': 1, + 'id': self.collection.id, 'type': 'collection', 'display_name': 'my_collection', 'description': 'my collection description', @@ -189,13 +196,6 @@ def setUp(self): "access_id": lib_access.id, 'breadcrumbs': [{'display_name': 'Library'}] } - with freeze_time(created_date): - self.collection = authoring_api.create_collection( - learning_package_id=self.learning_package.id, - title="my_collection", - created_by=None, - description="my collection description" - ) @override_settings(MEILISEARCH_ENABLED=False) def test_reindex_meilisearch_disabled(self, mock_meilisearch): From 23f5d5b3e2725393bdef995e4e6c7621470de9e8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 30 Aug 2024 14:17:43 +0930 Subject: [PATCH 23/26] refactor: adapt to oel_collection.update_collection_components changes --- .../core/djangoapps/content_libraries/api.py | 33 ++++++-------- .../collections/rest_api/v1/views.py | 18 ++++---- .../content_libraries/tests/test_api.py | 43 ++++--------------- 3 files changed, 29 insertions(+), 65 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 79f76c36d5ed..e3d4adc606d5 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -91,7 +91,7 @@ LIBRARY_BLOCK_UPDATED, ) from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import Collection, Component, MediaType, LearningPackage +from openedx_learning.api.authoring_models import Collection, Component, MediaType, LearningPackage, PublishableEntity from organizations.models import Organization from xblock.core import XBlock from xblock.exceptions import XBlockNotFoundError @@ -1070,12 +1070,11 @@ def revert_changes(library_key): def update_collection_components( - library: ContentLibraryMetadata, - collection_pk: int, + collection: Collection, usage_keys: list[UsageKeyV2], created_by: int | None = None, remove=False, -) -> int: +) -> Collection: """ Associates the Collection with Components for the given UsageKeys. @@ -1085,17 +1084,9 @@ def update_collection_components( Raises: * ContentLibraryCollectionNotFound if no Collection with the given pk is found in the given library. * ContentLibraryBlockNotFound if any of the given usage_keys don't match Components in the given library. - """ - if not usage_keys: - return 0 - - learning_package = library.learning_package - collections_qset = authoring_api.get_collections(learning_package.id).filter(pk=collection_pk) - - collection = collections_qset.first() - if not collection: - raise ContentLibraryCollectionNotFound(collection_pk) + Returns the updated Collection. + """ # Fetch the Component.key values for the provided UsageKeys. component_keys = [] for usage_key in usage_keys: @@ -1104,7 +1095,7 @@ def update_collection_components( try: component = authoring_api.get_component_by_key( - learning_package.id, + collection.learning_package_id, namespace=block_type.block_family, type_name=usage_key.block_type, local_key=usage_key.block_id, @@ -1115,18 +1106,18 @@ def update_collection_components( component_keys.append(component.key) # Note: Component.key matches its PublishableEntity.key - entities_qset = learning_package.publishable_entities.filter( + entities_qset = PublishableEntity.objects.filter( key__in=component_keys, ) if remove: - count = authoring_api.remove_from_collections( - collections_qset, + collection = authoring_api.remove_from_collection( + collection.pk, entities_qset, ) else: - count = authoring_api.add_to_collections( - collections_qset, + collection = authoring_api.add_to_collection( + collection.pk, entities_qset, created_by=created_by, ) @@ -1137,7 +1128,7 @@ def update_collection_components( content_object=ContentObjectData(object_id=usage_key), ) - return count + return collection # V1/V2 Compatibility Helpers diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py index df2e2340155f..86456ca3ee3b 100644 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py @@ -188,21 +188,21 @@ def update_components(self, request, lib_key_str, pk=None): Collection and Components must all be part of the given library/learning package. """ library_key = LibraryLocatorV2.from_string(lib_key_str) - library_obj = api.require_permission_for_library_key( - library_key, - request.user, - permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + collection = self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) + if not collection: + raise Http404 serializer = ContentLibraryCollectionComponentsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) - count = api.update_collection_components( - library_obj, - collection_pk=pk, - usage_keys=serializer.validated_data["usage_keys"], + usage_keys = serializer.validated_data["usage_keys"] + api.update_collection_components( + collection, + usage_keys=usage_keys, created_by=self.request.user.id, remove=(request.method == "DELETE"), ) - return Response({'count': count}) + return Response({'count': len(usage_keys)}) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 0c373f285055..f3226876131e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -305,27 +305,24 @@ def setUp(self): ) def test_update_collection_components(self): - count = api.update_collection_components( - library=self.lib1, - collection_pk=self.col1.pk, + assert not list(self.col1.entities.all()) + + self.col1 = api.update_collection_components( + collection=self.col1, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), ], ) - assert count == 2 assert len(self.col1.entities.all()) == 2 - count = api.update_collection_components( - library=self.lib1, - collection_pk=self.col1.pk, + self.col1 = api.update_collection_components( + collection=self.col1, usage_keys=[ UsageKey.from_string(self.lib1_html_block["id"]), ], remove=True, ) - assert count == 1 - self.col1.refresh_from_db() assert len(self.col1.entities.all()) == 1 def test_update_collection_components_event(self): @@ -335,8 +332,7 @@ def test_update_collection_components_event(self): event_receiver = mock.Mock() CONTENT_OBJECT_TAGS_CHANGED.connect(event_receiver) api.update_collection_components( - library=self.lib1, - collection_pk=self.col1.pk, + collection=self.col1, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), @@ -365,33 +361,10 @@ def test_update_collection_components_event(self): event_receiver.call_args_list[1].kwargs, ) - def test_update_no_components(self): - """ - Calling update_collection_components with no usage_keys should not fail, just return 0. - """ - assert not api.update_collection_components( - library=self.lib1, - collection_pk=self.col1.pk, - usage_keys=[], - ) - def test_update_collection_components_from_wrong_library(self): with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: api.update_collection_components( - library=self.lib2, - collection_pk=self.col2.pk, - usage_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - UsageKey.from_string(self.lib1_html_block["id"]), - ], - ) - assert self.lib1_problem_block["id"] in str(exc.exception) - - def test_update_collection_components_from_wrong_collection(self): - with self.assertRaises(api.ContentLibraryCollectionNotFound) as exc: - api.update_collection_components( - library=self.lib2, - collection_pk=self.col1.pk, + collection=self.col2, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), From addde52ead87bd57e2fa05f85757d5c23962876d Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 2 Sep 2024 13:01:23 +0930 Subject: [PATCH 24/26] refactor: moved code around after merging base --- .../core/djangoapps/content_libraries/apps.py | 1 - .../content_libraries/collections/__init__.py | 0 .../content_libraries/collections/handlers.py | 57 --- .../collections/rest_api/__init__.py | 0 .../collections/rest_api/v1/__init__.py | 0 .../collections/rest_api/v1/tests/__init__.py | 0 .../rest_api/v1/tests/test_views.py | 421 ------------------ .../collections/rest_api/v1/views.py | 208 --------- .../tests/test_views_collections.py | 133 ++++++ .../djangoapps/content_libraries/utils.py | 48 -- .../djangoapps/content_libraries/views.py | 2 +- .../content_libraries/views_collections.py | 34 +- 12 files changed, 166 insertions(+), 738 deletions(-) delete mode 100644 openedx/core/djangoapps/content_libraries/collections/__init__.py delete mode 100644 openedx/core/djangoapps/content_libraries/collections/handlers.py delete mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py delete mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py delete mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py delete mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py delete mode 100644 openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py delete mode 100644 openedx/core/djangoapps/content_libraries/utils.py diff --git a/openedx/core/djangoapps/content_libraries/apps.py b/openedx/core/djangoapps/content_libraries/apps.py index aea920714ce0..52c3e5179721 100644 --- a/openedx/core/djangoapps/content_libraries/apps.py +++ b/openedx/core/djangoapps/content_libraries/apps.py @@ -37,4 +37,3 @@ def ready(self): Import signal handler's module to ensure they are registered. """ from . import signal_handlers # pylint: disable=unused-import - from .collections import handlers # pylint: disable=unused-import diff --git a/openedx/core/djangoapps/content_libraries/collections/__init__.py b/openedx/core/djangoapps/content_libraries/collections/__init__.py deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/openedx/core/djangoapps/content_libraries/collections/handlers.py b/openedx/core/djangoapps/content_libraries/collections/handlers.py deleted file mode 100644 index c2c67ad403e5..000000000000 --- a/openedx/core/djangoapps/content_libraries/collections/handlers.py +++ /dev/null @@ -1,57 +0,0 @@ -""" -Signal handlers for Content Library Collections. -""" - -import logging - -from django.dispatch import receiver -from openedx_events.content_authoring.data import LibraryCollectionData -from openedx_events.content_authoring.signals import ( - LIBRARY_COLLECTION_CREATED, - LIBRARY_COLLECTION_UPDATED, - LIBRARY_COLLECTION_DELETED, -) - - -log = logging.getLogger(__name__) - - -@receiver(LIBRARY_COLLECTION_CREATED) -def library_collection_created_handler(**kwargs): - """ - Content Library Collection Created signal handler - """ - library_collection_data = kwargs.get("library_collection", None) - if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): - log.error("Received null or incorrect data for event") - return - - log.info("Received Collection Created Signal") - - # TODO: Implement handler logic - - -@receiver(LIBRARY_COLLECTION_UPDATED) -def library_collection_updated_handler(**kwargs): - """ - Content Library Collection Updated signal handler - """ - library_collection_data = kwargs.get("library_collection", None) - if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): - log.error("Received null or incorrect data for event") - return - - log.info("Received Collection Updated Signal") - - -@receiver(LIBRARY_COLLECTION_DELETED) -def library_collection_deleted_handler(**kwargs): - """ - Content Library Collection Deleted signal handler - """ - library_collection_data = kwargs.get("library_collection", None) - if not library_collection_data or not isinstance(library_collection_data, LibraryCollectionData): - log.error("Received null or incorrect data for event") - return - - log.info("Received Collection Deleted Signal") diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/__init__.py deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/__init__.py deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/__init__.py deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py deleted file mode 100644 index b9321c136383..000000000000 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py +++ /dev/null @@ -1,421 +0,0 @@ -""" -Tests Library Collections REST API views -""" - -from __future__ import annotations -import ddt - -from openedx_learning.api.authoring_models import Collection -from openedx_learning.api.authoring import create_collection -from opaque_keys.edx.locator import LibraryLocatorV2 - -from openedx.core.djangolib.testing.utils import skip_unless_cms -from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest -from openedx.core.djangoapps.content_libraries.models import ContentLibrary -from common.djangoapps.student.tests.factories import UserFactory - -URL_PREFIX = '/api/libraries/v2/{lib_key}/' -URL_LIB_COLLECTIONS = URL_PREFIX + 'collections/' -URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_id}/' -URL_LIB_COLLECTION_COMPONENTS = URL_LIB_COLLECTION + 'components/' - - -@ddt.ddt -@skip_unless_cms # Content Library Collections REST API is only available in Studio -class ContentLibraryCollectionsViewsTest(ContentLibrariesRestApiTest): - """ - Tests for Content Library Collection REST API Views - """ - - def setUp(self): - super().setUp() - - # Create Content Libraries - self._create_library("test-lib-col-1", "Test Library 1") - self._create_library("test-lib-col-2", "Test Library 2") - self.lib1 = ContentLibrary.objects.get(slug="test-lib-col-1") - self.lib2 = ContentLibrary.objects.get(slug="test-lib-col-2") - - # Create Content Library Collections - self.col1 = create_collection( - learning_package_id=self.lib1.learning_package.id, - title="Collection 1", - created_by=self.user.id, - description="Description for Collection 1", - ) - self.col2 = create_collection( - learning_package_id=self.lib1.learning_package.id, - title="Collection 2", - created_by=self.user.id, - description="Description for Collection 2", - ) - self.col3 = create_collection( - learning_package_id=self.lib2.learning_package.id, - title="Collection 3", - created_by=self.user.id, - description="Description for Collection 3", - ) - - # Create some library blocks - self.lib1_problem_block = self._add_block_to_library( - self.lib1.library_key, "problem", "problem1", - ) - self.lib1_html_block = self._add_block_to_library( - self.lib1.library_key, "html", "html1", - ) - self.lib2_problem_block = self._add_block_to_library( - self.lib2.library_key, "problem", "problem2", - ) - self.lib2_html_block = self._add_block_to_library( - self.lib2.library_key, "html", "html2", - ) - - def test_get_library_collection(self): - """ - Test retrieving a Content Library Collection - """ - resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) - ) - - # Check that correct Content Library Collection data retrieved - expected_collection = { - "title": "Collection 3", - "description": "Description for Collection 3", - } - assert resp.status_code == 200 - self.assertDictContainsEntries(resp.data, expected_collection) - - # Check that a random user without permissions cannot access Content Library Collection - random_user = UserFactory.create(username="Random", email="random@example.com") - with self.as_user(random_user): - resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) - ) - assert resp.status_code == 403 - - def test_get_invalid_library_collection(self): - """ - Test retrieving a an invalid Content Library Collection or one that does not exist - """ - # Fetch collection that belongs to a different library, it should fail - resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=self.col3.id) - ) - - assert resp.status_code == 404 - - # Fetch collection with invalid ID provided, it should fail - resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=123) - ) - - assert resp.status_code == 404 - - # Fetch collection with invalid library_key provided, it should fail - resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=123, collection_id=123) - ) - assert resp.status_code == 404 - - def test_list_library_collections(self): - """ - Test listing Content Library Collections - """ - resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) - - # Check that the correct collections are listed - assert resp.status_code == 200 - assert len(resp.data) == 2 - expected_collections = [ - {"title": "Collection 1", "description": "Description for Collection 1"}, - {"title": "Collection 2", "description": "Description for Collection 2"}, - ] - for collection, expected in zip(resp.data, expected_collections): - self.assertDictContainsEntries(collection, expected) - - # Check that a random user without permissions cannot access Content Library Collections - random_user = UserFactory.create(username="Random", email="random@example.com") - with self.as_user(random_user): - resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key)) - assert resp.status_code == 403 - - def test_list_invalid_library_collections(self): - """ - Test listing invalid Content Library Collections - """ - invalid_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") - resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=invalid_key)) - - assert resp.status_code == 404 - - non_existing_key = LibraryLocatorV2.from_string("lib:DoesNotExist:NE1") - resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=non_existing_key)) - - assert resp.status_code == 404 - - # List collections with invalid library_key provided, it should fail - resp = resp = self.client.get(URL_LIB_COLLECTIONS.format(lib_key=123)) - assert resp.status_code == 404 - - def test_create_library_collection(self): - """ - Test creating a Content Library Collection - """ - post_data = { - "title": "Collection 4", - "description": "Description for Collection 4", - } - resp = self.client.post( - URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" - ) - - # Check that the new Content Library Collection is returned in response and created in DB - assert resp.status_code == 200 - self.assertDictContainsEntries(resp.data, post_data) - - created_collection = Collection.objects.get(id=resp.data["id"]) - self.assertIsNotNone(created_collection) - - # Check that user with read only access cannot create new Content Library Collection - reader = UserFactory.create(username="Reader", email="reader@example.com") - self._add_user_by_email(self.lib1.library_key, reader.email, access_level="read") - - with self.as_user(reader): - post_data = { - "title": "Collection 5", - "description": "Description for Collection 5", - } - resp = self.client.post( - URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" - ) - - assert resp.status_code == 403 - - def test_create_invalid_library_collection(self): - """ - Test creating an invalid Content Library Collection - """ - post_data_missing_title = { - "description": "Description for Collection 4", - } - resp = self.client.post( - URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_title, format="json" - ) - - assert resp.status_code == 400 - - post_data_missing_desc = { - "title": "Collection 4", - } - resp = self.client.post( - URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_desc, format="json" - ) - - assert resp.status_code == 400 - - # Create collection with invalid library_key provided, it should fail - resp = self.client.post( - URL_LIB_COLLECTIONS.format(lib_key=123), - {**post_data_missing_title, **post_data_missing_desc}, - format="json" - ) - assert resp.status_code == 404 - - def test_update_library_collection(self): - """ - Test updating a Content Library Collection - """ - patch_data = { - "title": "Collection 3 Updated", - } - resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), - patch_data, - format="json" - ) - - # Check that updated Content Library Collection is returned in response and updated in DB - assert resp.status_code == 200 - self.assertDictContainsEntries(resp.data, patch_data) - - created_collection = Collection.objects.get(id=resp.data["id"]) - self.assertIsNotNone(created_collection) - self.assertEqual(created_collection.title, patch_data["title"]) - - # Check that user with read only access cannot update a Content Library Collection - reader = UserFactory.create(username="Reader", email="reader@example.com") - self._add_user_by_email(self.lib1.library_key, reader.email, access_level="read") - - with self.as_user(reader): - patch_data = { - "title": "Collection 3 should not update", - } - resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), - patch_data, - format="json" - ) - - assert resp.status_code == 403 - - def test_update_invalid_library_collection(self): - """ - Test updating an invalid Content Library Collection or one that does not exist - """ - patch_data = { - "title": "Collection 3 Updated", - } - # Update collection that belongs to a different library, it should fail - resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=self.col3.id), - patch_data, - format="json" - ) - - assert resp.status_code == 404 - - # Update collection with invalid ID provided, it should fail - resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_id=123), - patch_data, - format="json" - ) - - assert resp.status_code == 404 - - # Update collection with invalid library_key provided, it should fail - resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=123, collection_id=self.col3.id), - patch_data, - format="json" - ) - assert resp.status_code == 404 - - def test_delete_library_collection(self): - """ - Test deleting a Content Library Collection - - Note: Currently not implemented and should return a 405 - """ - resp = self.client.delete( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id) - ) - - assert resp.status_code == 405 - - def test_get_components(self): - """ - Retrieving components is not supported by the REST API; - use Meilisearch instead. - """ - resp = self.client.get( - URL_LIB_COLLECTION_COMPONENTS.format( - lib_key=self.lib1.library_key, - collection_id=self.col1.id, - ), - ) - assert resp.status_code == 405 - - def test_update_components(self): - """ - Test adding and removing components from a collection. - """ - # Add two components to col1 - resp = self.client.patch( - URL_LIB_COLLECTION_COMPONENTS.format( - lib_key=self.lib1.library_key, - collection_id=self.col1.id, - ), - data={ - "usage_keys": [ - self.lib1_problem_block["id"], - self.lib1_html_block["id"], - ] - } - ) - assert resp.status_code == 200 - assert resp.data == {"count": 2} - - # Remove one of the added components from col1 - resp = self.client.delete( - URL_LIB_COLLECTION_COMPONENTS.format( - lib_key=self.lib1.library_key, - collection_id=self.col1.id, - ), - data={ - "usage_keys": [ - self.lib1_problem_block["id"], - ] - } - ) - assert resp.status_code == 200 - assert resp.data == {"count": 1} - - @ddt.data("patch", "delete") - def test_update_components_wrong_collection(self, method): - """ - Collection must belong to the requested library. - """ - resp = getattr(self.client, method)( - URL_LIB_COLLECTION_COMPONENTS.format( - lib_key=self.lib2.library_key, - collection_id=self.col1.id, - ), - data={ - "usage_keys": [ - self.lib1_problem_block["id"], - ] - } - ) - assert resp.status_code == 404 - - @ddt.data("patch", "delete") - def test_update_components_missing_data(self, method): - """ - List of usage keys must contain at least one item. - """ - resp = getattr(self.client, method)( - URL_LIB_COLLECTION_COMPONENTS.format( - lib_key=self.lib2.library_key, - collection_id=self.col3.id, - ), - ) - assert resp.status_code == 400 - assert resp.data == { - "usage_keys": ["This field is required."], - } - - @ddt.data("patch", "delete") - def test_update_components_from_another_library(self, method): - """ - Adding/removing components from another library raises a 404. - """ - resp = getattr(self.client, method)( - URL_LIB_COLLECTION_COMPONENTS.format( - lib_key=self.lib2.library_key, - collection_id=self.col3.id, - ), - data={ - "usage_keys": [ - self.lib1_problem_block["id"], - self.lib1_html_block["id"], - ] - } - ) - assert resp.status_code == 404 - - @ddt.data("patch", "delete") - def test_update_components_permissions(self, method): - """ - Check that a random user without permissions cannot update a Content Library Collection's components. - """ - random_user = UserFactory.create(username="Random", email="random@example.com") - with self.as_user(random_user): - resp = getattr(self.client, method)( - URL_LIB_COLLECTION_COMPONENTS.format( - lib_key=self.lib1.library_key, - collection_id=self.col1.id, - ), - ) - assert resp.status_code == 403 diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py deleted file mode 100644 index 86456ca3ee3b..000000000000 --- a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py +++ /dev/null @@ -1,208 +0,0 @@ -""" -Collections API Views -""" - -from __future__ import annotations - -from django.http import Http404 - -from rest_framework.decorators import action -from rest_framework.response import Response -from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED -from rest_framework.viewsets import ModelViewSet - -from openedx_learning.api.authoring_models import Collection -from openedx_learning.api import authoring as authoring_api -from opaque_keys.edx.locator import LibraryLocatorV2 -from openedx_events.content_authoring.data import LibraryCollectionData -from openedx_events.content_authoring.signals import ( - LIBRARY_COLLECTION_CREATED, - LIBRARY_COLLECTION_UPDATED, -) - -from openedx.core.djangoapps.content_libraries import api, permissions -from openedx.core.djangoapps.content_libraries.serializers import ( - ContentLibraryCollectionSerializer, - ContentLibraryCollectionComponentsUpdateSerializer, - ContentLibraryCollectionCreateOrUpdateSerializer, -) -from openedx.core.djangoapps.content_libraries.utils import convert_exceptions - - -class LibraryCollectionsView(ModelViewSet): - """ - Views to get, create and update Library Collections. - - **Warning** These APIs are UNSTABLE, and may change once we implement the ability to store - units/sections/subsections in the library. - """ - - serializer_class = ContentLibraryCollectionSerializer - - def _verify_and_fetch_library_collection(self, library_key, collection_id, user, permission) -> Collection | None: - """ - Verify that the collection belongs to the library and the user has the correct permissions - """ - library_obj = api.require_permission_for_library_key(library_key, user, permission) - collection = None - if library_obj.learning_package_id: - collection = authoring_api.get_collections( - library_obj.learning_package_id - ).filter(id=collection_id).first() - return collection - - @convert_exceptions - def retrieve(self, request, *args, **kwargs): - """ - Retrieve the Content Library Collection - """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 - - pk = kwargs.pop("pk", None) - library_key = LibraryLocatorV2.from_string(lib_key_str) - - # Check if user has permissions to view this collection by checking if - # user has permission to view the Content Library it belongs to - collection = self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY - ) - - if not collection: - raise Http404 - - serializer = self.get_serializer(collection) - return Response(serializer.data) - - @convert_exceptions - def list(self, request, *args, **kwargs): - """ - List Collections that belong to Content Library - """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 - - # Check if user has permissions to view collections by checking if user - # has permission to view the Content Library they belong to - library_key = LibraryLocatorV2.from_string(lib_key_str) - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY - ) - - collections = authoring_api.get_collections(content_library.learning_package.id) - serializer = self.get_serializer(collections, many=True) - return Response(serializer.data) - - @convert_exceptions - def create(self, request, *args, **kwargs): - """ - Create a Collection that belongs to a Content Library - """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 - - # Check if user has permissions to create a collection in the Content Library - # by checking if user has permission to edit the Content Library - library_key = LibraryLocatorV2.from_string(lib_key_str) - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) - - create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) - create_serializer.is_valid(raise_exception=True) - collection = authoring_api.create_collection( - content_library.learning_package.id, - create_serializer.validated_data["title"], - request.user.id, - create_serializer.validated_data["description"] - ) - serializer = self.get_serializer(collection) - - # Emit event for library content collection creation - LIBRARY_COLLECTION_CREATED.send_event( - library_collection=LibraryCollectionData( - library_key=library_key, - collection_id=collection.id - ) - ) - - return Response(serializer.data) - - @convert_exceptions - def partial_update(self, request, *args, **kwargs): - """ - Update a Collection that belongs to a Content Library - """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 - - pk = kwargs.pop('pk', None) - library_key = LibraryLocatorV2.from_string(lib_key_str) - # Check if user has permissions to update a collection in the Content Library - # by checking if user has permission to edit the Content Library - collection = self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) - - if not collection: - raise Http404 - - update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( - collection, data=request.data, partial=True - ) - update_serializer.is_valid(raise_exception=True) - updated_collection = authoring_api.update_collection(pk, **update_serializer.validated_data) - serializer = self.get_serializer(updated_collection) - - # Emit event for library content collection updated - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - library_key=library_key, - collection_id=collection.id - ) - ) - - return Response(serializer.data) - - @convert_exceptions - def destroy(self, request, *args, **kwargs): - """ - Deletes a Collection that belongs to a Content Library - - Note: (currently not allowed) - """ - # TODO: Implement the deletion logic and emit event signal - - return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) - - @convert_exceptions - @action(detail=True, methods=['delete', 'patch'], url_path='components', url_name='components-update') - def update_components(self, request, lib_key_str, pk=None): - """ - Adds (PATCH) or removes (DELETE) Components to/from a Collection. - - Collection and Components must all be part of the given library/learning package. - """ - library_key = LibraryLocatorV2.from_string(lib_key_str) - collection = self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) - if not collection: - raise Http404 - - serializer = ContentLibraryCollectionComponentsUpdateSerializer(data=request.data) - serializer.is_valid(raise_exception=True) - - usage_keys = serializer.validated_data["usage_keys"] - api.update_collection_components( - collection, - usage_keys=usage_keys, - created_by=self.request.user.id, - remove=(request.method == "DELETE"), - ) - - return Response({'count': len(usage_keys)}) 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 1b4ce1a1b2fa..a03c7dd65484 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -3,6 +3,7 @@ """ from __future__ import annotations +import ddt from openedx_learning.api.authoring_models import Collection from openedx_learning.api.authoring import create_collection @@ -16,8 +17,10 @@ URL_PREFIX = '/api/libraries/v2/{lib_key}/' URL_LIB_COLLECTIONS = URL_PREFIX + 'collections/' URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_id}/' +URL_LIB_COLLECTION_COMPONENTS = URL_LIB_COLLECTION + 'components/' +@ddt.ddt @skip_unless_cms # Content Library Collections REST API is only available in Studio class ContentLibraryCollectionsViewsTest(ContentLibrariesRestApiTest): """ @@ -54,6 +57,20 @@ def setUp(self): description="Description for Collection 3", ) + # Create some library blocks + self.lib1_problem_block = self._add_block_to_library( + self.lib1.library_key, "problem", "problem1", + ) + self.lib1_html_block = self._add_block_to_library( + self.lib1.library_key, "html", "html1", + ) + self.lib2_problem_block = self._add_block_to_library( + self.lib2.library_key, "problem", "problem2", + ) + self.lib2_html_block = self._add_block_to_library( + self.lib2.library_key, "html", "html2", + ) + def test_get_library_collection(self): """ Test retrieving a Content Library Collection @@ -282,3 +299,119 @@ def test_delete_library_collection(self): ) assert resp.status_code == 405 + + def test_get_components(self): + """ + Retrieving components is not supported by the REST API; + use Meilisearch instead. + """ + resp = self.client.get( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), + ) + assert resp.status_code == 405 + + def test_update_components(self): + """ + Test adding and removing components from a collection. + """ + # Add two components to col1 + resp = self.client.patch( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + self.lib1_html_block["id"], + ] + } + ) + assert resp.status_code == 200 + assert resp.data == {"count": 2} + + # Remove one of the added components from col1 + resp = self.client.delete( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + ] + } + ) + assert resp.status_code == 200 + assert resp.data == {"count": 1} + + @ddt.data("patch", "delete") + def test_update_components_wrong_collection(self, method): + """ + Collection must belong to the requested library. + """ + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib2.library_key, + collection_id=self.col1.id, + ), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + ] + } + ) + assert resp.status_code == 404 + + @ddt.data("patch", "delete") + def test_update_components_missing_data(self, method): + """ + List of usage keys must contain at least one item. + """ + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib2.library_key, + collection_id=self.col3.id, + ), + ) + assert resp.status_code == 400 + assert resp.data == { + "usage_keys": ["This field is required."], + } + + @ddt.data("patch", "delete") + def test_update_components_from_another_library(self, method): + """ + Adding/removing components from another library raises a 404. + """ + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib2.library_key, + collection_id=self.col3.id, + ), + data={ + "usage_keys": [ + self.lib1_problem_block["id"], + self.lib1_html_block["id"], + ] + } + ) + assert resp.status_code == 404 + + @ddt.data("patch", "delete") + def test_update_components_permissions(self, method): + """ + Check that a random user without permissions cannot update a Content Library Collection's components. + """ + random_user = UserFactory.create(username="Random", email="random@example.com") + with self.as_user(random_user): + resp = getattr(self.client, method)( + URL_LIB_COLLECTION_COMPONENTS.format( + lib_key=self.lib1.library_key, + collection_id=self.col1.id, + ), + ) + assert resp.status_code == 403 diff --git a/openedx/core/djangoapps/content_libraries/utils.py b/openedx/core/djangoapps/content_libraries/utils.py deleted file mode 100644 index da21d9057c92..000000000000 --- a/openedx/core/djangoapps/content_libraries/utils.py +++ /dev/null @@ -1,48 +0,0 @@ -""" Utils used for the content libraries. """ - -from functools import wraps -import logging - -from rest_framework.exceptions import NotFound, ValidationError - -from opaque_keys import InvalidKeyError - -from openedx.core.djangoapps.content_libraries import api - - -log = logging.getLogger(__name__) - - -def convert_exceptions(fn): - """ - Catch any Content Library API exceptions that occur and convert them to - DRF exceptions so DRF will return an appropriate HTTP response - """ - - @wraps(fn) - def wrapped_fn(*args, **kwargs): - try: - return fn(*args, **kwargs) - except InvalidKeyError as exc: - log.exception(str(exc)) - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - except api.ContentLibraryNotFound: - log.exception("Content library not found") - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - except api.ContentLibraryBlockNotFound: - log.exception("XBlock not found in content library") - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - except api.ContentLibraryCollectionNotFound: - log.exception("Collection not found in content library") - raise NotFound # lint-amnesty, pylint: disable=raise-missing-from - - except api.LibraryBlockAlreadyExists as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - except api.InvalidNameError as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - except api.BlockLimitReachedError as exc: - log.exception(str(exc)) - raise ValidationError(str(exc)) # lint-amnesty, pylint: disable=raise-missing-from - return wrapped_fn diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index bdb36d9f1aae..bb1c4903f478 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -63,6 +63,7 @@ https://github.com/openedx/edx-platform/pull/30456 """ +from functools import wraps import itertools import json import logging @@ -120,7 +121,6 @@ from openedx.core.djangoapps.xblock import api as xblock_api from .models import ContentLibrary, LtiGradedResource, LtiProfile -from .utils import convert_exceptions User = get_user_model() diff --git a/openedx/core/djangoapps/content_libraries/views_collections.py b/openedx/core/djangoapps/content_libraries/views_collections.py index 5a51470077ae..2317ee3764e1 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -6,6 +6,7 @@ from django.http import Http404 +from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.viewsets import ModelViewSet from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED @@ -22,6 +23,7 @@ from openedx.core.djangoapps.content_libraries.views import convert_exceptions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, + ContentLibraryCollectionComponentsUpdateSerializer, ContentLibraryCollectionCreateOrUpdateSerializer, ) @@ -43,7 +45,7 @@ def _verify_and_fetch_library_collection(self, library_key, collection_id, user, library_obj = api.require_permission_for_library_key(library_key, user, permission) collection = None if library_obj.learning_package_id: - collection = authoring_api.get_learning_package_collections( + collection = authoring_api.get_collections( library_obj.learning_package_id ).filter(id=collection_id).first() return collection @@ -88,7 +90,7 @@ def list(self, request, *args, **kwargs): library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) - collections = authoring_api.get_learning_package_collections(content_library.learning_package.id) + collections = authoring_api.get_collections(content_library.learning_package.id) serializer = self.get_serializer(collections, many=True) return Response(serializer.data) @@ -175,3 +177,31 @@ def destroy(self, request, *args, **kwargs): # TODO: Implement the deletion logic and emit event signal return Response(None, status=HTTP_405_METHOD_NOT_ALLOWED) + + @convert_exceptions + @action(detail=True, methods=['delete', 'patch'], url_path='components', url_name='components-update') + def update_components(self, request, lib_key_str, pk=None): + """ + Adds (PATCH) or removes (DELETE) Components to/from a Collection. + + Collection and Components must all be part of the given library/learning package. + """ + library_key = LibraryLocatorV2.from_string(lib_key_str) + collection = self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) + if not collection: + raise Http404 + + serializer = ContentLibraryCollectionComponentsUpdateSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + + usage_keys = serializer.validated_data["usage_keys"] + api.update_collection_components( + collection, + usage_keys=usage_keys, + created_by=self.request.user.id, + remove=(request.method == "DELETE"), + ) + + return Response({'count': len(usage_keys)}) From 51f5c1a7b399eabf5bfc485cf6daf57474bed202 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 3 Sep 2024 10:34:41 +0930 Subject: [PATCH 25/26] refactor: simplify the REST API params and validation --- .../djangoapps/content_libraries/views.py | 3 + .../content_libraries/views_collections.py | 56 +++++++------------ 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index bb1c4903f478..cc1a58634d4f 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -146,6 +146,9 @@ def wrapped_fn(*args, **kwargs): except api.ContentLibraryBlockNotFound: log.exception("XBlock not found in content library") raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.ContentLibraryCollectionNotFound: + log.exception("Collection not found in content library") + raise NotFound # lint-amnesty, pylint: disable=raise-missing-from except api.LibraryBlockAlreadyExists 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/views_collections.py b/openedx/core/djangoapps/content_libraries/views_collections.py index 2317ee3764e1..0cfa51817312 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -4,8 +4,6 @@ from __future__ import annotations -from django.http import Http404 - from rest_framework.decorators import action from rest_framework.response import Response from rest_framework.viewsets import ModelViewSet @@ -38,16 +36,21 @@ class LibraryCollectionsView(ModelViewSet): serializer_class = ContentLibraryCollectionSerializer - def _verify_and_fetch_library_collection(self, library_key, collection_id, user, permission) -> Collection | None: + def _verify_and_fetch_library_collection(self, lib_key_str, collection_id, user, permission) -> Collection | None: """ - Verify that the collection belongs to the library and the user has the correct permissions + Verify that the collection belongs to the library and the user has the correct permissions. + + This method may raise exceptions; these are handled by the @convert_exceptions wrapper on the views. """ + library_key = LibraryLocatorV2.from_string(lib_key_str) library_obj = api.require_permission_for_library_key(library_key, user, permission) collection = None if library_obj.learning_package_id: collection = authoring_api.get_collections( library_obj.learning_package_id ).filter(id=collection_id).first() + if not collection: + raise api.ContentLibraryCollectionNotFound return collection @convert_exceptions @@ -55,22 +58,15 @@ def retrieve(self, request, *args, **kwargs): """ Retrieve the Content Library Collection """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 - - pk = kwargs.pop("pk", None) - library_key = LibraryLocatorV2.from_string(lib_key_str) + lib_key_str = kwargs.pop("lib_key_str") + pk = kwargs.pop("pk") # Check if user has permissions to view this collection by checking if # user has permission to view the Content Library it belongs to collection = self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + lib_key_str, pk, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) - if not collection: - raise Http404 - serializer = self.get_serializer(collection) return Response(serializer.data) @@ -79,13 +75,11 @@ def list(self, request, *args, **kwargs): """ List Collections that belong to Content Library """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 + lib_key_str = kwargs.pop("lib_key_str") + library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to view collections by checking if user # has permission to view the Content Library they belong to - library_key = LibraryLocatorV2.from_string(lib_key_str) content_library = api.require_permission_for_library_key( library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) @@ -99,13 +93,11 @@ def create(self, request, *args, **kwargs): """ Create a Collection that belongs to a Content Library """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 + lib_key_str = kwargs.pop("lib_key_str") + library_key = LibraryLocatorV2.from_string(lib_key_str) # Check if user has permissions to create a collection in the Content Library # by checking if user has permission to edit the Content Library - library_key = LibraryLocatorV2.from_string(lib_key_str) content_library = api.require_permission_for_library_key( library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) @@ -135,21 +127,16 @@ def partial_update(self, request, *args, **kwargs): """ Update a Collection that belongs to a Content Library """ - lib_key_str = kwargs.pop('lib_key_str', None) - if not lib_key_str: - raise Http404 - - pk = kwargs.pop('pk', None) + lib_key_str = kwargs.pop('lib_key_str') library_key = LibraryLocatorV2.from_string(lib_key_str) + pk = kwargs.pop('pk') + # Check if user has permissions to update a collection in the Content Library # by checking if user has permission to edit the Content Library collection = self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + lib_key_str, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - if not collection: - raise Http404 - update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( collection, data=request.data, partial=True ) @@ -180,18 +167,15 @@ def destroy(self, request, *args, **kwargs): @convert_exceptions @action(detail=True, methods=['delete', 'patch'], url_path='components', url_name='components-update') - def update_components(self, request, lib_key_str, pk=None): + def update_components(self, request, lib_key_str, pk): """ Adds (PATCH) or removes (DELETE) Components to/from a Collection. Collection and Components must all be part of the given library/learning package. """ - library_key = LibraryLocatorV2.from_string(lib_key_str) collection = self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + lib_key_str, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - if not collection: - raise Http404 serializer = ContentLibraryCollectionComponentsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) From 7a8e4c6d33df0e238322713cbc9d3347a6e9dd3e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 3 Sep 2024 10:35:08 +0930 Subject: [PATCH 26/26] chore: uses openedx-learning==0.11.3 --- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 65b0cba58cfa..224344d12a36 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -93,7 +93,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components +openedx-learning==0.11.3 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. openai<=0.28.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 99676901838a..9638248a1b04 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -823,7 +823,7 @@ openedx-filters==1.9.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components +openedx-learning==0.11.3 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index b4bb1d2f7b26..d0f3c226c1c3 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1372,7 +1372,7 @@ openedx-filters==1.9.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components +openedx-learning==0.11.3 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index dde1d21fb4e5..81e8d6839437 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -982,7 +982,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components +openedx-learning==0.11.3 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index f8c4f02b71f3..dc021dbd37e4 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1033,7 +1033,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/openedx/openedx-learning/@jill/collection-components +openedx-learning==0.11.3 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt