From ed30ce27ee07790769d8bd2100ea5b3276706b8e Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 14 Aug 2024 07:36:15 +0300 Subject: [PATCH 01/39] 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 3d64ea4aef5c65b494b0e785ceb71ff87e4e4122 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 22 Aug 2024 08:33:45 +0300 Subject: [PATCH 02/39] 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 931f688049ae30d9ac00a9c9d2c53fea0ec05e97 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 23 Aug 2024 10:00:56 +0300 Subject: [PATCH 03/39] 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 b2b38cb940c8885fb0fd7e619b062742a9f69938 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 23 Aug 2024 13:46:23 +0300 Subject: [PATCH 04/39] 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 a16b39848d185c3651fed34e6639c9bc7b3e5cd3 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 26 Aug 2024 07:42:19 +0300 Subject: [PATCH 05/39] 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 1c612fab6741..ad5659628dee 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 e6fd96e3be28..88fad695c43f 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 c3dcafee9343..b6e3bcbeab50 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 925fe517c858..30200d52ea41 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 be39d0349c6e9c11443fa207792d4e12de504715 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 26 Aug 2024 08:43:40 +0300 Subject: [PATCH 06/39] 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 22fa79153fc7eccf0e4a0724e0c0d0aba3e7e83a Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 29 Aug 2024 08:49:27 +0300 Subject: [PATCH 07/39] 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 0bf0f78a33587c0b7d81762abbaf16782097b01b Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 30 Aug 2024 09:08:11 +0300 Subject: [PATCH 08/39] refactor: Relocate convert_exceptions, remove utils --- .../collections/rest_api/v1/views.py | 2 +- .../djangoapps/content_libraries/utils.py | 44 ------------------- .../djangoapps/content_libraries/views.py | 34 +++++++++++++- 3 files changed, 34 insertions(+), 46 deletions(-) delete mode 100644 openedx/core/djangoapps/content_libraries/utils.py 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 7484b0893cff..5a51470077ae 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,7 +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.views import convert_exceptions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, ContentLibraryCollectionCreateOrUpdateSerializer, diff --git a/openedx/core/djangoapps/content_libraries/utils.py b/openedx/core/djangoapps/content_libraries/utils.py deleted file mode 100644 index bdfb1337d7bf..000000000000 --- a/openedx/core/djangoapps/content_libraries/utils.py +++ /dev/null @@ -1,44 +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.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 11e5c9d23b2e..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 @@ -83,6 +84,7 @@ from pylti1p3.exception import LtiException, OIDCException import edx_api_doc_tools as apidocs +from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from organizations.api import ensure_organization from organizations.exceptions import InvalidOrganizationException @@ -119,13 +121,43 @@ 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 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 + + class LibraryApiPaginationDocs: """ API docs for query params related to paginating ContentLibraryMetadata objects. From 45cf88652a1991fc58b6917d87ddcab036c0f203 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 30 Aug 2024 09:10:32 +0300 Subject: [PATCH 09/39] refactor: Remove Collection handlers skeleton code --- .../core/djangoapps/content_libraries/apps.py | 1 - .../content_libraries/collections/handlers.py | 57 ------------------- 2 files changed, 58 deletions(-) delete mode 100644 openedx/core/djangoapps/content_libraries/collections/handlers.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/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") From 366a7e9412a97034d0e473ee67f324cbcb97377c Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 30 Aug 2024 09:20:01 +0300 Subject: [PATCH 10/39] refactor: Move Collections views/tests --- .../core/djangoapps/content_libraries/collections/__init__.py | 0 .../content_libraries/collections/rest_api/__init__.py | 0 .../content_libraries/collections/rest_api/v1/__init__.py | 0 .../collections/rest_api/v1/tests/__init__.py | 0 .../tests/test_views.py => tests/test_views_collections.py} | 0 openedx/core/djangoapps/content_libraries/urls.py | 4 ++-- .../rest_api/v1/views.py => views_collections.py} | 0 7 files changed, 2 insertions(+), 2 deletions(-) delete mode 100644 openedx/core/djangoapps/content_libraries/collections/__init__.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 rename openedx/core/djangoapps/content_libraries/{collections/rest_api/v1/tests/test_views.py => tests/test_views_collections.py} (100%) rename openedx/core/djangoapps/content_libraries/{collections/rest_api/v1/views.py => views_collections.py} (100%) 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/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/tests/test_views_collections.py similarity index 100% rename from openedx/core/djangoapps/content_libraries/collections/rest_api/v1/tests/test_views.py rename to openedx/core/djangoapps/content_libraries/tests/test_views_collections.py diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index a0c5a9f8cd61..9455f0de5e61 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -7,7 +7,7 @@ from rest_framework import routers from . import views -from .collections.rest_api.v1 import views as collection_views +from . import views_collections # Django application name. @@ -21,7 +21,7 @@ library_collections_router = routers.DefaultRouter() library_collections_router.register( - r'collections', collection_views.LibraryCollectionsView, basename="library-collections" + r'collections', views_collections.LibraryCollectionsView, basename="library-collections" ) # These URLs are only used in Studio. The LMS already provides all the diff --git a/openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py b/openedx/core/djangoapps/content_libraries/views_collections.py similarity index 100% rename from openedx/core/djangoapps/content_libraries/collections/rest_api/v1/views.py rename to openedx/core/djangoapps/content_libraries/views_collections.py From 504aa4ff251948c0a9c6f30855ea00daf1467c9b Mon Sep 17 00:00:00 2001 From: Jillian Date: Tue, 3 Sep 2024 11:03:31 +0930 Subject: [PATCH 11/39] feat: Add REST endpoints to update Components in a Collections (temp) (#674) * chore: uses openedx-learning==0.11.3 * feat: add/remove components to/from a collection Sends a CONTENT_OBJECT_TAGS_CHANGED for each component added/removed. * docs: Add warning about unstable REST APIs * refactor: use oel_collections.get_collections as oel_collections.get_learning_package_collections has been removed. * test: fixes flaky collection search test * refactor: simplify the REST API params and validation --- openedx/core/djangoapps/content/search/api.py | 76 +++++----- .../content/search/tests/test_api.py | 16 +-- .../core/djangoapps/content_libraries/api.py | 75 +++++++++- .../content_libraries/serializers.py | 33 +++++ .../content_libraries/tests/test_api.py | 130 +++++++++++++++++ .../tests/test_views_collections.py | 133 ++++++++++++++++++ .../djangoapps/content_libraries/views.py | 3 + .../content_libraries/views_collections.py | 80 ++++++----- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 13 files changed, 467 insertions(+), 89 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.") 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): diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 9a4ccbd6456e..e3d4adc606d5 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, PublishableEntity from organizations.models import Organization from xblock.core import XBlock from xblock.exceptions import XBlockNotFoundError @@ -111,6 +116,8 @@ ContentLibraryNotFound = ContentLibrary.DoesNotExist +ContentLibraryCollectionNotFound = Collection.DoesNotExist + class ContentLibraryBlockNotFound(XBlockNotFoundError): """ XBlock not found in the content library """ @@ -1062,6 +1069,68 @@ def revert_changes(library_key): ) +def update_collection_components( + collection: Collection, + usage_keys: list[UsageKeyV2], + created_by: int | None = None, + remove=False, +) -> Collection: + """ + 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: + * 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. + + Returns the updated Collection. + """ + # 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)) + + try: + component = authoring_api.get_component_by_key( + collection.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 + entities_qset = PublishableEntity.objects.filter( + key__in=component_keys, + ) + + if remove: + collection = authoring_api.remove_from_collection( + collection.pk, + entities_qset, + ) + else: + collection = authoring_api.add_to_collection( + collection.pk, + entities_qset, + created_by=created_by, + ) + + # Emit a CONTENT_OBJECT_TAGS_CHANGED event for each of the objects added/removed + for usage_key in usage_keys: + CONTENT_OBJECT_TAGS_CHANGED.send_event( + content_object=ContentObjectData(object_id=usage_key), + ) + + return collection + + # 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/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 7c49d4af3c2b..6de602c63b36 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 ContentLibraryCollectionComponentsUpdateSerializer(serializers.Serializer): + """ + Serializer for adding/removing Components to/from a Collection. + """ + + usage_keys = serializers.ListField(child=UsageKeyV2Serializer(), allow_empty=False) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 87ae180d291e..f3226876131e 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,127 @@ 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, + ] + + @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() + + # 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): + 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 len(self.col1.entities.all()) == 2 + + self.col1 = api.update_collection_components( + collection=self.col1, + usage_keys=[ + UsageKey.from_string(self.lib1_html_block["id"]), + ], + remove=True, + ) + 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( + collection=self.col1, + 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_collection_components_from_wrong_library(self): + with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: + api.update_collection_components( + collection=self.col2, + 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) 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/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 5a51470077ae..0cfa51817312 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -4,8 +4,7 @@ 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 from rest_framework.status import HTTP_405_METHOD_NOT_ALLOWED @@ -22,6 +21,7 @@ from openedx.core.djangoapps.content_libraries.views import convert_exceptions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, + ContentLibraryCollectionComponentsUpdateSerializer, ContentLibraryCollectionCreateOrUpdateSerializer, ) @@ -36,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_learning_package_collections( + 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 @@ -53,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) @@ -77,18 +75,16 @@ 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 ) - 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) @@ -97,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 ) @@ -133,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 ) @@ -175,3 +164,28 @@ 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): + """ + Adds (PATCH) or removes (DELETE) Components to/from a Collection. + + Collection and Components must all be part of the given library/learning package. + """ + collection = self._verify_and_fetch_library_collection( + lib_key_str, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) + + 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/requirements/constraints.txt b/requirements/constraints.txt index 4a2f87e652b8..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==0.11.2 +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 ad5659628dee..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==0.11.2 +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 88fad695c43f..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==0.11.2 +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 b6e3bcbeab50..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==0.11.2 +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 30200d52ea41..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==0.11.2 +openedx-learning==0.11.3 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From a2da207f601c8f436f4b70bba8bf236e04b67a0a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 3 Sep 2024 14:18:11 +0930 Subject: [PATCH 12/39] refactor: pull view functionality into api For the Collection "create" and "update" views, we call an authoring_api method and emit an event on success. This change simplifies the view code by moving these call to new content_libraries.api methods. --- .../core/djangoapps/content_libraries/api.py | 95 ++++++++++++++++++- .../content_libraries/tests/test_api.py | 27 +++--- .../content_libraries/views_collections.py | 73 +++++++------- 3 files changed, 139 insertions(+), 56 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index e3d4adc606d5..73da2a9c3300 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -80,6 +80,7 @@ ContentLibraryData, ContentObjectData, LibraryBlockData, + LibraryCollectionData, ) from openedx_events.content_authoring.signals import ( CONTENT_OBJECT_TAGS_CHANGED, @@ -89,6 +90,8 @@ LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, ) from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Collection, Component, MediaType, LearningPackage, PublishableEntity @@ -1069,11 +1072,77 @@ def revert_changes(library_key): ) -def update_collection_components( - collection: Collection, +def create_library_collection( + library_key: LibraryLocatorV2, + title: str, + description: str = "", + created_by: int | None = None, + content_library: ContentLibrary | None = None, +) -> Collection: + """ + Creates a Collection in the given ContentLibrary, + and emits a LIBRARY_COLLECTION_CREATED event. + + If you've already fetched a ContentLibrary for the given library_key, pass it in here to avoid refetching. + """ + if not content_library: + content_library = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] + assert content_library + assert content_library.learning_package_id + assert content_library.library_key == library_key + + collection = authoring_api.create_collection( + learning_package_id=content_library.learning_package_id, + title=title, + description=description, + created_by=created_by, + ) + + # Emit event for library collection created + LIBRARY_COLLECTION_CREATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_id=collection.id, + ) + ) + + return collection + + +def update_library_collection( + library_key: LibraryLocatorV2, + collection_id: int, + title: str | None = None, + description: str | None = None, +) -> Collection: + """ + Creates a Collection in the given ContentLibrary, + and emits a LIBRARY_COLLECTION_CREATED event. + """ + collection = authoring_api.update_collection( + collection_id=collection_id, + title=title, + description=description, + ) + + # Emit event for library collection updated + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_id=collection.id + ) + ) + + return collection + + +def update_library_collection_components( + library_key: LibraryLocatorV2, + collection_id: int, usage_keys: list[UsageKeyV2], created_by: int | None = None, remove=False, + content_library: ContentLibrary | None = None, ) -> Collection: """ Associates the Collection with Components for the given UsageKeys. @@ -1081,12 +1150,20 @@ def update_collection_components( By default the Components are added to the Collection. If remove=True, the Components are removed from the Collection. + If you've already fetched the ContentLibrary, pass it in to avoid refetching. + 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. Returns the updated Collection. """ + if not content_library: + content_library = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] + assert content_library + assert content_library.learning_package_id + assert content_library.library_key == library_key + # Fetch the Component.key values for the provided UsageKeys. component_keys = [] for usage_key in usage_keys: @@ -1095,7 +1172,7 @@ def update_collection_components( try: component = authoring_api.get_component_by_key( - collection.learning_package_id, + content_library.learning_package_id, namespace=block_type.block_family, type_name=usage_key.block_type, local_key=usage_key.block_id, @@ -1112,16 +1189,24 @@ def update_collection_components( if remove: collection = authoring_api.remove_from_collection( - collection.pk, + collection_id, entities_qset, ) else: collection = authoring_api.add_to_collection( - collection.pk, + collection_id, entities_qset, created_by=created_by, ) + # Emit event for library collection updated + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_id=collection_id + ) + ) + # Emit a CONTENT_OBJECT_TAGS_CHANGED event for each of the objects added/removed for usage_key in usage_keys: CONTENT_OBJECT_TAGS_CHANGED.send_event( diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index f3226876131e..93c8d4b3ad4e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -6,6 +6,7 @@ import hashlib from unittest import mock +from django.core.exceptions import ValidationError from django.test import TestCase from opaque_keys.edx.keys import ( @@ -304,11 +305,12 @@ def setUp(self): self.lib1.library_key, "html", "html1", ) - def test_update_collection_components(self): + def test_update_library_collection_components(self): assert not list(self.col1.entities.all()) - self.col1 = api.update_collection_components( - collection=self.col1, + self.col1 = api.update_library_collection_components( + self.lib1.library_key, + collection_id=self.col1.id, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), @@ -316,8 +318,9 @@ def test_update_collection_components(self): ) assert len(self.col1.entities.all()) == 2 - self.col1 = api.update_collection_components( - collection=self.col1, + self.col1 = api.update_library_collection_components( + self.lib1.library_key, + collection_id=self.col1.id, usage_keys=[ UsageKey.from_string(self.lib1_html_block["id"]), ], @@ -325,14 +328,15 @@ def test_update_collection_components(self): ) assert len(self.col1.entities.all()) == 1 - def test_update_collection_components_event(self): + def test_update_library_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( - collection=self.col1, + api.update_library_collection_components( + self.lib1.library_key, + collection_id=self.col1.id, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), @@ -362,9 +366,10 @@ def test_update_collection_components_event(self): ) def test_update_collection_components_from_wrong_library(self): - with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: - api.update_collection_components( - collection=self.col2, + with self.assertRaises(ValidationError) as exc: + api.update_library_collection_components( + self.lib1.library_key, + collection_id=self.col2.id, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), diff --git a/openedx/core/djangoapps/content_libraries/views_collections.py b/openedx/core/djangoapps/content_libraries/views_collections.py index 0cfa51817312..13746d4a8865 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -11,13 +11,8 @@ 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.models import ContentLibrary from openedx.core.djangoapps.content_libraries.views import convert_exceptions from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, @@ -36,13 +31,18 @@ class LibraryCollectionsView(ModelViewSet): serializer_class = ContentLibraryCollectionSerializer - def _verify_and_fetch_library_collection(self, lib_key_str, collection_id, user, permission) -> Collection | None: + def _verify_and_fetch_library_collection( + self, + library_key, + collection_id, + user, + permission, + ) -> tuple[ContentLibrary, Collection]: """ 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: @@ -51,7 +51,7 @@ def _verify_and_fetch_library_collection(self, lib_key_str, collection_id, user, ).filter(id=collection_id).first() if not collection: raise api.ContentLibraryCollectionNotFound - return collection + return library_obj, collection @convert_exceptions def retrieve(self, request, *args, **kwargs): @@ -59,12 +59,13 @@ def retrieve(self, request, *args, **kwargs): Retrieve the Content Library Collection """ 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 view this collection by checking if # user has permission to view the Content Library it belongs to - collection = self._verify_and_fetch_library_collection( - lib_key_str, pk, 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 ) serializer = self.get_serializer(collection) @@ -104,22 +105,15 @@ def create(self, request, *args, **kwargs): 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"] + collection = api.create_library_collection( + library_key=library_key, + content_library=content_library, + title=create_serializer.validated_data["title"], + description=create_serializer.validated_data["description"], + created_by=request.user.id, ) 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 @@ -133,24 +127,20 @@ def partial_update(self, request, *args, **kwargs): # 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( - lib_key_str, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( - collection, data=request.data, partial=True + 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 - ) + updated_collection = api.update_library_collection( + library_key=library_key, + collection_id=pk, + **update_serializer.validated_data ) + serializer = self.get_serializer(updated_collection) return Response(serializer.data) @@ -173,16 +163,19 @@ def update_components(self, request, lib_key_str, pk): Collection and Components must all be part of the given library/learning package. """ - collection = self._verify_and_fetch_library_collection( - lib_key_str, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + library_key = LibraryLocatorV2.from_string(lib_key_str) + library_obj, _ = self._verify_and_fetch_library_collection( + library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) serializer = ContentLibraryCollectionComponentsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) usage_keys = serializer.validated_data["usage_keys"] - api.update_collection_components( - collection, + api.update_library_collection_components( + library_key=library_key, + content_library=library_obj, + collection_id=pk, usage_keys=usage_keys, created_by=self.request.user.id, remove=(request.method == "DELETE"), From aa3c1e3643b5c5e867bb6f8b5bdc2afe087f6986 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 4 Sep 2024 11:46:38 +0930 Subject: [PATCH 13/39] temp: use temporary openedx-learning branch --- 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 224344d12a36..718063ea9513 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.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key # 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 9638248a1b04..19b806042f88 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.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index d0f3c226c1c3..fd8c18a40e35 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.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 81e8d6839437..20c1cf3da3ec 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.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index dc021dbd37e4..85a3b687ac75 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.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 6ae83baa47e91bde4eef52a85d524c60e95184fe Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 4 Sep 2024 12:03:57 +0930 Subject: [PATCH 14/39] refactor: use collection.key as ID in search records --- openedx/core/djangoapps/content/search/documents.py | 2 +- openedx/core/djangoapps/content/search/tests/test_api.py | 3 ++- .../djangoapps/content/search/tests/test_documents.py | 9 +++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index a45b37ab2ad2..be5428a0a9c2 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -288,7 +288,7 @@ def searchable_doc_for_collection(collection) -> dict: found using faceted search. """ doc = { - Fields.id: collection.id, + Fields.id: collection.key, Fields.type: DocType.collection, Fields.display_name: collection.title, Fields.description: collection.description, diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 23840997b87b..1b4f9b6abe7f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -180,12 +180,13 @@ def setUp(self): with freeze_time(created_date): self.collection = authoring_api.create_collection( learning_package_id=self.learning_package.id, + key="MYCOL", title="my_collection", created_by=None, description="my collection description" ) self.collection_dict = { - 'id': self.collection.id, + 'id': 'MYCOL', 'type': 'collection', 'display_name': 'my_collection', 'description': 'my collection description', diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 6140411705bb..998be96d0023 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -215,19 +215,20 @@ def test_collection_with_no_library(self): ) collection = authoring_api.create_collection( learning_package_id=learning_package.id, + key="MYCOL", title="my_collection", created_by=None, description="my collection description" ) doc = searchable_doc_for_collection(collection) assert doc == { - "id": collection.id, + "id": "MYCOL", "type": "collection", - "display_name": collection.title, - "description": collection.description, + "display_name": "my_collection", + "description": "my collection description", "context_key": learning_package.key, "access_id": self.toy_course_access_id, - "breadcrumbs": [{"display_name": learning_package.title}], + "breadcrumbs": [{"display_name": "some learning_package"}], "created": created_date.timestamp(), "modified": created_date.timestamp(), } From abb1eb18eb8bf2c79d7a36929180bf3b52719cf7 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 4 Sep 2024 12:10:18 +0930 Subject: [PATCH 15/39] refactor: use Collection.key as identifier in content_libraries and refactors collection views to use DRF conventions. --- .../core/djangoapps/content_libraries/api.py | 45 +++-- .../content_libraries/serializers.py | 12 +- .../content_libraries/tests/test_api.py | 23 +-- .../tests/test_views_collections.py | 65 +++---- .../content_libraries/views_collections.py | 160 +++++++++--------- 5 files changed, 170 insertions(+), 135 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 73da2a9c3300..2253514ee0f9 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1074,9 +1074,12 @@ def revert_changes(library_key): def create_library_collection( library_key: LibraryLocatorV2, + collection_key: str, title: str, + *, description: str = "", created_by: int | None = None, + # As an optimization, callers may pass in a pre-fetched ContentLibrary instance content_library: ContentLibrary | None = None, ) -> Collection: """ @@ -1093,6 +1096,7 @@ def create_library_collection( collection = authoring_api.create_collection( learning_package_id=content_library.learning_package_id, + key=collection_key, title=title, description=description, created_by=created_by, @@ -1102,7 +1106,7 @@ def create_library_collection( LIBRARY_COLLECTION_CREATED.send_event( library_collection=LibraryCollectionData( library_key=library_key, - collection_id=collection.id, + collection_key=collection.key, ) ) @@ -1111,25 +1115,38 @@ def create_library_collection( def update_library_collection( library_key: LibraryLocatorV2, - collection_id: int, + collection_key: str, + *, title: str | None = None, description: str | None = None, + # As an optimization, callers may pass in a pre-fetched ContentLibrary instance + content_library: ContentLibrary | None = None, ) -> Collection: """ Creates a Collection in the given ContentLibrary, and emits a LIBRARY_COLLECTION_CREATED event. """ - collection = authoring_api.update_collection( - collection_id=collection_id, - title=title, - description=description, - ) + if not content_library: + content_library = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] + assert content_library + assert content_library.learning_package_id + assert content_library.library_key == library_key + + try: + collection = authoring_api.update_collection( + learning_package_id=content_library.learning_package_id, + key=collection_key, + title=title, + description=description, + ) + except Collection.DoesNotExist as exc: + raise ContentLibraryCollectionNotFound from exc # Emit event for library collection updated LIBRARY_COLLECTION_UPDATED.send_event( library_collection=LibraryCollectionData( library_key=library_key, - collection_id=collection.id + collection_key=collection.key, ) ) @@ -1138,10 +1155,12 @@ def update_library_collection( def update_library_collection_components( library_key: LibraryLocatorV2, - collection_id: int, + collection_key: str, + *, usage_keys: list[UsageKeyV2], created_by: int | None = None, remove=False, + # As an optimization, callers may pass in a pre-fetched ContentLibrary instance content_library: ContentLibrary | None = None, ) -> Collection: """ @@ -1189,12 +1208,14 @@ def update_library_collection_components( if remove: collection = authoring_api.remove_from_collection( - collection_id, + content_library.learning_package_id, + collection_key, entities_qset, ) else: collection = authoring_api.add_to_collection( - collection_id, + content_library.learning_package_id, + collection_key, entities_qset, created_by=created_by, ) @@ -1203,7 +1224,7 @@ def update_library_collection_components( LIBRARY_COLLECTION_UPDATED.send_event( library_collection=LibraryCollectionData( library_key=library_key, - collection_id=collection_id + collection_key=collection.key, ) ) diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 6de602c63b36..12a11e96e13c 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -262,15 +262,23 @@ class Meta: fields = '__all__' -class ContentLibraryCollectionCreateOrUpdateSerializer(serializers.Serializer): +class ContentLibraryCollectionUpdateSerializer(serializers.Serializer): """ - Serializer for add/update a Collection in a Content Library + Serializer for updating a Collection in a Content Library """ title = serializers.CharField() description = serializers.CharField() +class ContentLibraryCollectionCreateSerializer(ContentLibraryCollectionUpdateSerializer): + """ + Serializer for adding a Collection in a Content Library + """ + + key = serializers.CharField() + + class UsageKeyV2Serializer(serializers.Serializer): """ Serializes a UsageKeyV2. diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 93c8d4b3ad4e..f026dfb4c06e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -6,7 +6,6 @@ import hashlib from unittest import mock -from django.core.exceptions import ValidationError from django.test import TestCase from opaque_keys.edx.keys import ( @@ -284,14 +283,16 @@ def setUp(self): 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, + self.col1 = api.create_library_collection( + self.lib1.library_key, + collection_key="COL1", 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, + self.col2 = api.create_library_collection( + self.lib2.library_key, + collection_key="COL2", title="Collection 2", description="Description for Collection 2", created_by=self.user.id, @@ -310,7 +311,7 @@ def test_update_library_collection_components(self): self.col1 = api.update_library_collection_components( self.lib1.library_key, - collection_id=self.col1.id, + self.col1.key, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), @@ -320,7 +321,7 @@ def test_update_library_collection_components(self): self.col1 = api.update_library_collection_components( self.lib1.library_key, - collection_id=self.col1.id, + self.col1.key, usage_keys=[ UsageKey.from_string(self.lib1_html_block["id"]), ], @@ -336,7 +337,7 @@ def test_update_library_collection_components_event(self): CONTENT_OBJECT_TAGS_CHANGED.connect(event_receiver) api.update_library_collection_components( self.lib1.library_key, - collection_id=self.col1.id, + self.col1.key, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), @@ -366,10 +367,10 @@ def test_update_library_collection_components_event(self): ) def test_update_collection_components_from_wrong_library(self): - with self.assertRaises(ValidationError) as exc: + with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: api.update_library_collection_components( - self.lib1.library_key, - collection_id=self.col2.id, + self.lib2.library_key, + self.col2.key, usage_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), 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 a03c7dd65484..a4ef40613df7 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -6,17 +6,17 @@ 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 import api 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 = URL_LIB_COLLECTIONS + '{collection_key}/' URL_LIB_COLLECTION_COMPONENTS = URL_LIB_COLLECTION + 'components/' @@ -37,21 +37,24 @@ def setUp(self): 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, + self.col1 = api.create_library_collection( + self.lib1.library_key, + "COL1", 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, + self.col2 = api.create_library_collection( + self.lib1.library_key, + "COL2", 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, + self.col3 = api.create_library_collection( + self.lib2.library_key, + "COL3", title="Collection 3", created_by=self.user.id, description="Description for Collection 3", @@ -76,7 +79,7 @@ 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) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) ) # Check that correct Content Library Collection data retrieved @@ -91,7 +94,7 @@ def test_get_library_collection(self): 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) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) ) assert resp.status_code == 403 @@ -101,21 +104,21 @@ def test_get_invalid_library_collection(self): """ # 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) + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key=self.col3.key) ) 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) + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key='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) + URL_LIB_COLLECTION.format(lib_key=123, collection_key='123') ) assert resp.status_code == 404 @@ -127,12 +130,12 @@ def test_list_library_collections(self): # Check that the correct collections are listed assert resp.status_code == 200 - assert len(resp.data) == 2 + assert len(resp.data["results"]) == 2 expected_collections = [ - {"title": "Collection 1", "description": "Description for Collection 1"}, - {"title": "Collection 2", "description": "Description for Collection 2"}, + {"key": "COL1", "title": "Collection 1", "description": "Description for Collection 1"}, + {"key": "COL2", "title": "Collection 2", "description": "Description for Collection 2"}, ] - for collection, expected in zip(resp.data, expected_collections): + for collection, expected in zip(resp.data["results"], expected_collections): self.assertDictContainsEntries(collection, expected) # Check that a random user without permissions cannot access Content Library Collections @@ -159,6 +162,7 @@ def test_create_library_collection(self): Test creating a Content Library Collection """ post_data = { + "key": "COL4", "title": "Collection 4", "description": "Description for Collection 4", } @@ -179,6 +183,7 @@ def test_create_library_collection(self): with self.as_user(reader): post_data = { + "key": "COL5", "title": "Collection 5", "description": "Description for Collection 5", } @@ -226,7 +231,7 @@ def test_update_library_collection(self): "title": "Collection 3 Updated", } resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_id=self.col3.id), + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key), patch_data, format="json" ) @@ -248,7 +253,7 @@ def test_update_library_collection(self): "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), + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key), patch_data, format="json" ) @@ -264,7 +269,7 @@ def test_update_invalid_library_collection(self): } # 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), + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key=self.col3.key), patch_data, format="json" ) @@ -273,7 +278,7 @@ def test_update_invalid_library_collection(self): # 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), + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key='123'), patch_data, format="json" ) @@ -282,7 +287,7 @@ def test_update_invalid_library_collection(self): # 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), + URL_LIB_COLLECTION.format(lib_key=123, collection_key=self.col3.key), patch_data, format="json" ) @@ -295,7 +300,7 @@ def test_delete_library_collection(self): 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) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) ) assert resp.status_code == 405 @@ -308,7 +313,7 @@ def test_get_components(self): resp = self.client.get( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_id=self.col1.id, + collection_key=self.col1.key, ), ) assert resp.status_code == 405 @@ -321,7 +326,7 @@ def test_update_components(self): resp = self.client.patch( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_id=self.col1.id, + collection_key=self.col1.key, ), data={ "usage_keys": [ @@ -337,7 +342,7 @@ def test_update_components(self): resp = self.client.delete( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_id=self.col1.id, + collection_key=self.col1.key, ), data={ "usage_keys": [ @@ -356,7 +361,7 @@ def test_update_components_wrong_collection(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib2.library_key, - collection_id=self.col1.id, + collection_key=self.col1.key, ), data={ "usage_keys": [ @@ -374,7 +379,7 @@ def test_update_components_missing_data(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib2.library_key, - collection_id=self.col3.id, + collection_key=self.col3.key, ), ) assert resp.status_code == 400 @@ -390,7 +395,7 @@ def test_update_components_from_another_library(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib2.library_key, - collection_id=self.col3.id, + collection_key=self.col3.key, ), data={ "usage_keys": [ @@ -411,7 +416,7 @@ def test_update_components_permissions(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_id=self.col1.id, + collection_key=self.col1.key, ), ) assert resp.status_code == 403 diff --git a/openedx/core/djangoapps/content_libraries/views_collections.py b/openedx/core/djangoapps/content_libraries/views_collections.py index 13746d4a8865..482b6e2bb9f1 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -4,12 +4,16 @@ from __future__ import annotations +from django.db.models import QuerySet + 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 from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx_learning.api import authoring as authoring_api +from openedx_learning.api.authoring_models import Collection from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.models import ContentLibrary @@ -17,12 +21,10 @@ from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, ContentLibraryCollectionComponentsUpdateSerializer, - ContentLibraryCollectionCreateOrUpdateSerializer, + ContentLibraryCollectionCreateSerializer, + ContentLibraryCollectionUpdateSerializer, ) -from openedx_learning.api.authoring_models import Collection -from openedx_learning.api import authoring as authoring_api - class LibraryCollectionsView(ModelViewSet): """ @@ -30,84 +32,90 @@ class LibraryCollectionsView(ModelViewSet): """ serializer_class = ContentLibraryCollectionSerializer + lookup_field = 'key' - def _verify_and_fetch_library_collection( - self, - library_key, - collection_id, - user, - permission, - ) -> tuple[ContentLibrary, Collection]: + def __init__(self, *args, **kwargs) -> None: """ - 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. + Caches the ContentLibrary for the duration of the request. """ - 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 library_obj, collection + super().__init__(*args, **kwargs) + self._content_library: ContentLibrary | None = None - @convert_exceptions - def retrieve(self, request, *args, **kwargs): + def get_content_library(self) -> ContentLibrary: """ - Retrieve the Content Library Collection + Returns the requested ContentLibrary object, if access allows. """ - lib_key_str = kwargs.pop("lib_key_str") + if self._content_library: + return self._content_library + + lib_key_str = self.kwargs["lib_key_str"] library_key = LibraryLocatorV2.from_string(lib_key_str) - pk = kwargs.pop("pk") + permission = ( + permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + if self.request.method in ['OPTIONS', 'GET'] + else permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) - # 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 + self._content_library = api.require_permission_for_library_key( + library_key, + self.request.user, + permission, ) + return self._content_library - serializer = self.get_serializer(collection) - return Response(serializer.data) + def get_queryset(self) -> QuerySet[Collection]: + """ + Returns a queryset for the requested Collections, if access allows. - @convert_exceptions - def list(self, request, *args, **kwargs): + This method may raise exceptions; these are handled by the @convert_exceptions wrapper on the views. """ - List Collections that belong to Content Library + content_library = self.get_content_library() + assert content_library.learning_package_id + return authoring_api.get_collections(content_library.learning_package_id) + + def get_object(self) -> Collection: """ - lib_key_str = kwargs.pop("lib_key_str") - library_key = LibraryLocatorV2.from_string(lib_key_str) + Returns the requested Collections, if access allows. - # Check if user has permissions to view collections by checking if user - # has permission to view the Content Library they belong to - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY - ) + This method may raise exceptions; these are handled by the @convert_exceptions wrapper on the views. + """ + collection = super().get_object() + content_library = self.get_content_library() - collections = authoring_api.get_collections(content_library.learning_package.id) - serializer = self.get_serializer(collections, many=True) - return Response(serializer.data) + # Ensure the ContentLibrary and Collection share the same learning package + if collection.learning_package_id != content_library.learning_package_id: + raise api.ContentLibraryCollectionNotFound + return collection @convert_exceptions - def create(self, request, *args, **kwargs): + def retrieve(self, request, *args, **kwargs) -> Response: """ - Create a Collection that belongs to a Content Library + Retrieve the Content Library Collection """ - lib_key_str = kwargs.pop("lib_key_str") - library_key = LibraryLocatorV2.from_string(lib_key_str) + # View declared so we can wrap it in @convert_exceptions + return super().retrieve(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 - content_library = api.require_permission_for_library_key( - library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) + @convert_exceptions + def list(self, request, *args, **kwargs) -> Response: + """ + List Collections that belong to Content Library + """ + # View declared so we can wrap it in @convert_exceptions + return super().list(request, *args, **kwargs) - create_serializer = ContentLibraryCollectionCreateOrUpdateSerializer(data=request.data) + @convert_exceptions + def create(self, request, *args, **kwargs) -> Response: + """ + Create a Collection that belongs to a Content Library + """ + content_library = self.get_content_library() + create_serializer = ContentLibraryCollectionCreateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) + collection = api.create_library_collection( - library_key=library_key, + library_key=content_library.library_key, content_library=content_library, + collection_key=create_serializer.validated_data["key"], title=create_serializer.validated_data["title"], description=create_serializer.validated_data["description"], created_by=request.user.id, @@ -117,27 +125,21 @@ def create(self, request, *args, **kwargs): return Response(serializer.data) @convert_exceptions - def partial_update(self, request, *args, **kwargs): + def partial_update(self, request, *args, **kwargs) -> Response: """ Update a Collection that belongs to a Content Library """ - lib_key_str = kwargs.pop('lib_key_str') - library_key = LibraryLocatorV2.from_string(lib_key_str) - pk = kwargs.pop('pk') + content_library = self.get_content_library() + collection_key = kwargs["key"] - # Check if user has permissions to update a collection in the Content Library - # by checking if user has permission to edit the Content Library - self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) - - update_serializer = ContentLibraryCollectionCreateOrUpdateSerializer( + update_serializer = ContentLibraryCollectionUpdateSerializer( data=request.data, partial=True ) update_serializer.is_valid(raise_exception=True) updated_collection = api.update_library_collection( - library_key=library_key, - collection_id=pk, + library_key=content_library.library_key, + collection_key=collection_key, + content_library=content_library, **update_serializer.validated_data ) serializer = self.get_serializer(updated_collection) @@ -145,7 +147,7 @@ def partial_update(self, request, *args, **kwargs): return Response(serializer.data) @convert_exceptions - def destroy(self, request, *args, **kwargs): + def destroy(self, request, *args, **kwargs) -> Response: """ Deletes a Collection that belongs to a Content Library @@ -157,25 +159,23 @@ 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): + def update_components(self, request, *args, **kwargs) -> Response: """ 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, _ = self._verify_and_fetch_library_collection( - library_key, pk, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY - ) + content_library = self.get_content_library() + collection_key = kwargs["key"] serializer = ContentLibraryCollectionComponentsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) usage_keys = serializer.validated_data["usage_keys"] api.update_library_collection_components( - library_key=library_key, - content_library=library_obj, - collection_id=pk, + library_key=content_library.library_key, + content_library=content_library, + collection_key=collection_key, usage_keys=usage_keys, created_by=self.request.user.id, remove=(request.method == "DELETE"), From 0eb52cfab69cae7cb0a3a9c4dc0dc9b3ad9fe708 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 4 Sep 2024 12:16:53 +0930 Subject: [PATCH 16/39] test: adds tests for events raised by collections api --- .../content_libraries/tests/test_api.py | 105 +++++++++++++++++- 1 file changed, 99 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index f026dfb4c06e..b7b646acac56 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -13,10 +13,16 @@ 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.content_authoring.data import ( + ContentObjectData, + LibraryCollectionData, +) +from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_TAGS_CHANGED, + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, +) from openedx_events.tests.utils import OpenEdxEventsTestMixin -from openedx_learning.api import authoring as authoring_api from .. import api from ..models import ContentLibrary @@ -257,6 +263,8 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTe """ ENABLED_OPENEDX_EVENTS = [ CONTENT_OBJECT_TAGS_CHANGED.event_type, + LIBRARY_COLLECTION_CREATED.event_type, + LIBRARY_COLLECTION_UPDATED.event_type, ] @classmethod @@ -306,6 +314,78 @@ def setUp(self): self.lib1.library_key, "html", "html1", ) + def test_create_library_collection(self): + event_receiver = mock.Mock() + LIBRARY_COLLECTION_CREATED.connect(event_receiver) + + collection = api.create_library_collection( + self.lib2.library_key, + collection_key="COL4", + title="Collection 4", + description="Description for Collection 4", + created_by=self.user.id, + ) + assert collection.key == "COL4" + assert collection.title == "Collection 4" + assert collection.description == "Description for Collection 4" + assert collection.created_by == self.user + + assert event_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_CREATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib2.library_key, + collection_key="COL4", + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + + def test_create_library_collection_invalid_library(self): + library_key = LibraryLocatorV2.from_string("lib:INVALID:test-lib-does-not-exist") + with self.assertRaises(api.ContentLibraryNotFound) as exc: + api.create_library_collection( + library_key, + collection_key="COL4", + title="Collection 3", + ) + + def test_update_library_collection(self): + event_receiver = mock.Mock() + LIBRARY_COLLECTION_UPDATED.connect(event_receiver) + + self.col1 = api.update_library_collection( + self.lib1.library_key, + self.col1.key, + title="New title for Collection 1", + ) + assert self.col1.key == "COL1" + assert self.col1.title == "New title for Collection 1" + assert self.col1.description == "Description for Collection 1" + assert self.col1.created_by == self.user + + assert event_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib1.library_key, + collection_key="COL1", + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + + def test_update_library_collection_wrong_library(self): + with self.assertRaises(api.ContentLibraryCollectionNotFound) as exc: + api.update_library_collection( + self.lib1.library_key, + self.col2.key, + ) + def test_update_library_collection_components(self): assert not list(self.col1.entities.all()) @@ -335,6 +415,8 @@ def test_update_library_collection_components_event(self): """ event_receiver = mock.Mock() CONTENT_OBJECT_TAGS_CHANGED.connect(event_receiver) + LIBRARY_COLLECTION_UPDATED.connect(event_receiver) + api.update_library_collection_components( self.lib1.library_key, self.col1.key, @@ -344,7 +426,18 @@ def test_update_library_collection_components_event(self): ], ) - assert event_receiver.call_count == 2 + assert event_receiver.call_count == 3 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib1.library_key, + collection_key="COL1", + ), + }, + event_receiver.call_args_list[0].kwargs, + ) self.assertDictContainsSubset( { "signal": CONTENT_OBJECT_TAGS_CHANGED, @@ -353,7 +446,7 @@ def test_update_library_collection_components_event(self): object_id=UsageKey.from_string(self.lib1_problem_block["id"]), ), }, - event_receiver.call_args_list[0].kwargs, + event_receiver.call_args_list[1].kwargs, ) self.assertDictContainsSubset( { @@ -363,7 +456,7 @@ def test_update_library_collection_components_event(self): object_id=UsageKey.from_string(self.lib1_html_block["id"]), ), }, - event_receiver.call_args_list[1].kwargs, + event_receiver.call_args_list[2].kwargs, ) def test_update_collection_components_from_wrong_library(self): From 6f397675e65a68c36817134340a4e938c2a0c59b Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 4 Sep 2024 12:48:18 +0930 Subject: [PATCH 17/39] temp: use temporary openedx-events branch --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 19b806042f88..9097af24cc38 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 @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/kernel.in # edx-event-bus-kafka diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index fd8c18a40e35..4e48379cfa5e 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 @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 20c1cf3da3ec..30f35ebaba8a 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 @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/base.txt # edx-event-bus-kafka diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 5154dc293efa..5f9417fd0326 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -119,7 +119,7 @@ openedx-calc # Library supporting mathematical calculatio openedx-django-require # 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-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals 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 85a3b687ac75..cb0a41683736 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 @ git+https://github.com/open-craft/openedx-events.git@8101eb46d15717e7d5390af0901b8314a2c7da25 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/base.txt # edx-event-bus-kafka From bcc5cbbff0996e67ee02be6a3c89719550978a53 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Aug 2024 20:11:22 +0930 Subject: [PATCH 18/39] fix: pass a ContentKey to the search doc handlers They expect a UsageKey object, not the string object_id. --- openedx/core/djangoapps/content/search/handlers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index ba0e8c1a1680..835810879780 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -152,9 +152,9 @@ def content_object_tags_changed_handler(**kwargs) -> None: try: # Check if valid if course or library block - get_content_key_from_string(content_object_tags.object_id) + content_key = get_content_key_from_string(str(content_object_tags.object_id)) except ValueError: log.error("Received invalid content object id") return - upsert_block_tags_index_docs(content_object_tags.object_id) + upsert_block_tags_index_docs(content_key) From 0e32451b35ff2128676045188b68975bc48d9602 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Aug 2024 20:15:01 +0930 Subject: [PATCH 19/39] feat: add tags.collections to content object search index and ensures this new field is searchable and filterable. Serializes the object's collections to a list of collection.key values. --- openedx/core/djangoapps/content/search/api.py | 2 + .../djangoapps/content/search/documents.py | 35 ++++-- .../content/search/tests/test_api.py | 46 +++++++- .../content/search/tests/test_documents.py | 105 +++++++++++++++++- 4 files changed, 173 insertions(+), 15 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 76bb5eb3f4a4..00b908e26cc4 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -317,6 +317,7 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.context_key, Fields.org, Fields.tags, + Fields.tags + "." + Fields.tags_collections, Fields.tags + "." + Fields.tags_taxonomy, Fields.tags + "." + Fields.tags_level0, Fields.tags + "." + Fields.tags_level1, @@ -339,6 +340,7 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: # are searchable only if at least one document in the index has a value. If we didn't list them here and, # say, there were no tags.level3 tags in the index, the client would get an error if trying to search for # these sub-fields: "Attribute `tags.level3` is not searchable." + Fields.tags + "." + Fields.tags_collections, Fields.tags + "." + Fields.tags_taxonomy, Fields.tags + "." + Fields.tags_level0, Fields.tags + "." + Fields.tags_level1, diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index be5428a0a9c2..c92776e12a4b 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -7,7 +7,9 @@ from hashlib import blake2b from django.utils.text import slugify +from django.core.exceptions import ObjectDoesNotExist from opaque_keys.edx.keys import LearningContextKey, UsageKey +from openedx_learning.api import authoring as authoring_api from openedx.core.djangoapps.content.search.models import SearchAccess from openedx.core.djangoapps.content_libraries import api as lib_api @@ -52,6 +54,7 @@ class Fields: tags_level1 = "level1" tags_level2 = "level2" tags_level3 = "level3" + tags_collections = "collections" # subfield of tags, i.e. tags.collections # The "content" field is a dictionary of arbitrary data, depending on the block_type. # It comes from each XBlock's index_dictionary() method (if present) plus some processing. # Text (html) blocks have an "html_content" key in here, capa has "capa_content" and "problem_types", and so on. @@ -164,10 +167,11 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: See the comments above on "Field.tags" for an explanation of the format. - e.g. for something tagged "Difficulty: Hard" and "Location: Vancouver" this - would return: + e.g. for something tagged "Difficulty: Hard" and "Location: Vancouver", + and in Collections 3 and 4, this would return: { "tags": { + "collections": [3, 4], "taxonomy": ["Location", "Difficulty"], "level0": ["Location > North America", "Difficulty > Hard"], "level1": ["Location > North America > Canada"], @@ -182,21 +186,16 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: strings in a particular format that the frontend knows how to render to support hierarchical refinement by tag. """ + result = {} + # Note that we could improve performance for indexing many components from the same library/course, # if we used get_all_object_tags() to load all the tags for the library in a single query rather than loading the # tags for each component separately. all_tags = tagging_api.get_object_tags(str(object_id)).all() - if not all_tags: - # Clear out tags in the index when unselecting all tags for the block, otherwise - # it would remain the last value if a cleared Fields.tags field is not included - return {Fields.tags: {}} - result = { - Fields.tags_taxonomy: [], - Fields.tags_level0: [], - # ... other levels added as needed - } for obj_tag in all_tags: # Add the taxonomy name: + if Fields.tags_taxonomy not in result: + result[Fields.tags_taxonomy] = [] if obj_tag.taxonomy.name not in result[Fields.tags_taxonomy]: result[Fields.tags_taxonomy].append(obj_tag.taxonomy.name) # Taxonomy name plus each level of tags, in a list: # e.g. ["Location", "North America", "Canada", "Vancouver"] @@ -220,6 +219,20 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: if len(parts) == level + 2: break # We have all the levels for this tag now (e.g. parts=["Difficulty", "Hard"] -> need level0 only) + # Gather the collections associated with this object + collections = [] + try: + component = lib_api.get_component_from_usage_key(object_id) + collections = authoring_api.get_entity_collections( + component.learning_package_id, + component.key, + ).values_list("key", flat=True) + except ObjectDoesNotExist: + log.warning(f"No component found for {object_id}") + + if collections: + result[Fields.tags_collections] = list(collections) + return {Fields.tags: result} diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 1b4f9b6abe7f..68a734c9e8aa 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -175,7 +175,7 @@ def setUp(self): tagging_api.add_tag_to_taxonomy(self.taxonomyB, "three") tagging_api.add_tag_to_taxonomy(self.taxonomyB, "four") - # Create a collection: + # Create collections: self.learning_package = authoring_api.get_learning_package_by_key(self.library.key) with freeze_time(created_date): self.collection = authoring_api.create_collection( @@ -379,11 +379,37 @@ def test_index_library_block_tags(self, mock_meilisearch): """ Test indexing an Library Block with tags. """ + collection1 = authoring_api.create_collection( + learning_package_id=self.library.learning_package.id, + key="COL1", + title="Collection 1", + created_by=None, + description="First Collection", + ) + + collection2 = authoring_api.create_collection( + learning_package_id=self.library.learning_package.id, + key="COL2", + title="Collection 2", + created_by=None, + description="Second Collection", + ) # Tag XBlock (these internally call `upsert_block_tags_index_docs`) tagging_api.tag_object(str(self.problem1.usage_key), self.taxonomyA, ["one", "two"]) tagging_api.tag_object(str(self.problem1.usage_key), self.taxonomyB, ["three", "four"]) + # Add Problem1 to both Collections (these internally call `upsert_block_tags_index_docs`) + # (adding in reverse order to test sorting of collection tag)) + for collection in (collection2, collection1): + library_api.update_library_collection_components( + self.library.key, + collection_key=collection.key, + usage_keys=[ + self.problem1.usage_key, + ], + ) + # Build expected docs with tags at each stage doc_problem_with_tags1 = { "id": self.doc_problem1["id"], @@ -399,11 +425,29 @@ def test_index_library_block_tags(self, mock_meilisearch): 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] } } + doc_problem_with_collection2 = { + "id": self.doc_problem1["id"], + "tags": { + 'collections': [collection2.key], + 'taxonomy': ['A', 'B'], + 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] + } + } + doc_problem_with_collection1 = { + "id": self.doc_problem1["id"], + "tags": { + 'collections': [collection1.key, collection2.key], + 'taxonomy': ['A', 'B'], + 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] + } + } mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ call([doc_problem_with_tags1]), call([doc_problem_with_tags2]), + call([doc_problem_with_collection2]), + call([doc_problem_with_collection1]), ], any_order=True, ) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 998be96d0023..c8442f406460 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -8,6 +8,7 @@ from openedx_learning.api import authoring as authoring_api from openedx.core.djangoapps.content_tagging import api as tagging_api +from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangolib.testing.utils import skip_unless_cms from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -15,12 +16,18 @@ try: # This import errors in the lms because content.search is not an installed app there. - from ..documents import searchable_doc_for_course_block, searchable_doc_tags, searchable_doc_for_collection + from ..documents import ( + searchable_doc_for_course_block, + searchable_doc_tags, + searchable_doc_for_collection, + searchable_doc_for_library_block, + ) from ..models import SearchAccess except RuntimeError: searchable_doc_for_course_block = lambda x: x searchable_doc_tags = lambda x: x searchable_doc_for_collection = lambda x: x + searchable_doc_for_library_block = lambda x: x SearchAccess = {} @@ -38,12 +45,13 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): def setUpClass(cls): super().setUpClass() cls.store = modulestore() + cls.org = Organization.objects.create(name="edX", short_name="edX") cls.toy_course = ToyCourseFactory.create() # See xmodule/modulestore/tests/sample_courses.py cls.toy_course_key = cls.toy_course.id # Get references to some blocks in the toy course cls.html_block_key = cls.toy_course_key.make_usage_key("html", "toyjumpto") - # Create a problem in library + # Create a problem in course cls.problem_block = BlockFactory.create( category="problem", parent_location=cls.toy_course_key.make_usage_key("vertical", "vertical_test"), @@ -51,8 +59,38 @@ def setUpClass(cls): data="What is a test?", ) + # Create a library and collection with a block + created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) + with freeze_time(created_date): + cls.library = library_api.create_library( + org=cls.org, + slug="2012_Fall", + title="some content_library", + description="some description", + ) + cls.collection = library_api.create_library_collection( + cls.library.key, + collection_key="TOY_COLLECTION", + title="Toy Collection", + created_by=None, + description="my toy collection description" + ) + cls.library_block = library_api.create_library_block( + cls.library.key, + "html", + "text2", + ) + + # Add the problem block to the collection + library_api.update_library_collection_components( + cls.library.key, + collection_key="TOY_COLLECTION", + usage_keys=[ + cls.library_block.usage_key, + ] + ) + # Create a couple taxonomies and some tags - cls.org = Organization.objects.create(name="edX", short_name="edX") cls.difficulty_tags = tagging_api.create_taxonomy(name="Difficulty", orgs=[cls.org], allow_multiple=False) tagging_api.add_tag_to_taxonomy(cls.difficulty_tags, tag="Easy") tagging_api.add_tag_to_taxonomy(cls.difficulty_tags, tag="Normal") @@ -69,6 +107,7 @@ def setUpClass(cls): tagging_api.tag_object(str(cls.problem_block.usage_key), cls.difficulty_tags, tags=["Easy"]) tagging_api.tag_object(str(cls.html_block_key), cls.subject_tags, tags=["Chinese", "Jump Links"]) tagging_api.tag_object(str(cls.html_block_key), cls.difficulty_tags, tags=["Normal"]) + tagging_api.tag_object(str(cls.library_block.usage_key), cls.difficulty_tags, tags=["Normal"]) @property def toy_course_access_id(self): @@ -80,6 +119,16 @@ def toy_course_access_id(self): """ return SearchAccess.objects.get(context_key=self.toy_course_key).id + @property + def library_access_id(self): + """ + Returns the SearchAccess.id created for the library. + + This SearchAccess object is created when documents are added to the search index, so this method must be called + after this step, or risk a DoesNotExist error. + """ + return SearchAccess.objects.get(context_key=self.library.key).id + def test_problem_block(self): """ Test how a problem block gets represented in the search index @@ -205,6 +254,56 @@ def test_video_block_untagged(self): # This video has no tags. } + def test_html_library_block(self): + """ + Test how a library block gets represented in the search index + """ + doc = {} + doc.update(searchable_doc_for_library_block(self.library_block)) + doc.update(searchable_doc_tags(self.library_block.usage_key)) + assert doc == { + "id": "lbedx2012_fallhtmltext2-4bb47d67", + "type": "library_block", + "block_type": "html", + "usage_key": "lb:edX:2012_Fall:html:text2", + "block_id": "text2", + "context_key": "lib:edX:2012_Fall", + "org": "edX", + "access_id": self.library_access_id, + "display_name": "Text", + "breadcrumbs": [ + { + "display_name": "some content_library", + }, + ], + "last_published": None, + "created": 1680674828.0, + "modified": 1680674828.0, + "content": { + "html_content": "", + }, + "tags": { + "collections": ["TOY_COLLECTION"], + "taxonomy": ["Difficulty"], + "level0": ["Difficulty > Normal"], + }, + } + + def test_collection_with_library(self): + doc = searchable_doc_for_collection(self.collection) + assert doc == { + "id": "TOY_COLLECTION", + "type": "collection", + "org": "edX", + "display_name": "Toy Collection", + "description": "my toy collection description", + "context_key": "lib:edX:2012_Fall", + "access_id": self.library_access_id, + "breadcrumbs": [{"display_name": "some content_library"}], + "created": 1680674828.0, + "modified": 1680674828.0, + } + def test_collection_with_no_library(self): created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) with freeze_time(created_date): From 3c6617e3d39a0c3f809302c6fdbe9075ab3e74ef Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 5 Sep 2024 12:20:47 +0930 Subject: [PATCH 20/39] temp: use openedx-events temporary branch which adds CONTENT_OBJECT_ASSOCIATIONS_CHANGED --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 9097af24cc38..4830f3522cb3 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations # via # -r requirements/edx/kernel.in # edx-event-bus-kafka diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 4e48379cfa5e..e2ec6f46b81d 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 30f35ebaba8a..e1de91706bdc 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations # via # -r requirements/edx/base.txt # edx-event-bus-kafka diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 5f9417fd0326..782bb0fe73e1 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -119,7 +119,7 @@ openedx-calc # Library supporting mathematical calculatio openedx-django-require # 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@yusuf-musleh/content-library-collections-signals +openedx-events @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations 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 cb0a41683736..654d2fa2361d 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations # via # -r requirements/edx/base.txt # edx-event-bus-kafka From 0c29552bb3b591181f8d2e3f986ad9e0df1899be Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 5 Sep 2024 12:21:13 +0930 Subject: [PATCH 21/39] feat: emit CONTENT_OBJECT_ASSOCIATIONS_CHANGED whenever a content object's tags or collections have changed, and handle that event in content/search. The deprecated CONTENT_OBJECT_TAGS_CHANGED event is still emitted when tags change; to be removed after Sumac. --- .../djangoapps/content/search/handlers.py | 28 +++++++++++-------- .../core/djangoapps/content_libraries/api.py | 13 +++++---- .../content_libraries/tests/test_api.py | 20 +++++++------ .../core/djangoapps/content_tagging/api.py | 20 ++++++++----- .../content_tagging/rest_api/v1/views.py | 19 +++++++++++-- 5 files changed, 65 insertions(+), 35 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 835810879780..7b008b7a454e 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -6,7 +6,14 @@ from django.db.models.signals import post_delete from django.dispatch import receiver -from openedx_events.content_authoring.data import ContentLibraryData, ContentObjectData, LibraryBlockData, XBlockData +from openedx_events.content_authoring.data import ( + ContentLibraryData, + ContentObjectChangedData, + LibraryBlockData, + XBlockData, +) +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, @@ -16,9 +23,8 @@ XBLOCK_CREATED, XBLOCK_DELETED, XBLOCK_UPDATED, - CONTENT_OBJECT_TAGS_CHANGED, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, ) -from openedx.core.djangoapps.content_tagging.utils import get_content_key_from_string from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.search.models import SearchAccess @@ -139,22 +145,22 @@ def content_library_updated_handler(**kwargs) -> None: update_content_library_index_docs.delay(str(content_library_data.library_key)) -@receiver(CONTENT_OBJECT_TAGS_CHANGED) +@receiver(CONTENT_OBJECT_ASSOCIATIONS_CHANGED) @only_if_meilisearch_enabled -def content_object_tags_changed_handler(**kwargs) -> None: +def content_object_associations_changed_handler(**kwargs) -> None: """ - Update the tags data in the index for the Content Object + Update the collections/tags data in the index for the Content Object """ - content_object_tags = kwargs.get("content_object", None) - if not content_object_tags or not isinstance(content_object_tags, ContentObjectData): + content_object = kwargs.get("content_object", None) + if not content_object or not isinstance(content_object, ContentObjectChangedData): log.error("Received null or incorrect data for event") return try: # Check if valid if course or library block - content_key = get_content_key_from_string(str(content_object_tags.object_id)) - except ValueError: + usage_key = UsageKey.from_string(str(content_object.object_id)) + except InvalidKeyError: log.error("Received invalid content object id") return - upsert_block_tags_index_docs(content_key) + upsert_block_tags_index_docs(usage_key) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 2253514ee0f9..649aaa5b3287 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -78,12 +78,12 @@ from opaque_keys import InvalidKeyError from openedx_events.content_authoring.data import ( ContentLibraryData, - ContentObjectData, + ContentObjectChangedData, LibraryBlockData, LibraryCollectionData, ) from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_TAGS_CHANGED, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, CONTENT_LIBRARY_CREATED, CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, @@ -1228,10 +1228,13 @@ def update_library_collection_components( ) ) - # Emit a CONTENT_OBJECT_TAGS_CHANGED event for each of the objects added/removed + # Emit a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for each of the objects added/removed for usage_key in usage_keys: - CONTENT_OBJECT_TAGS_CHANGED.send_event( - content_object=ContentObjectData(object_id=usage_key), + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=str(usage_key), + changes=["collections"], + ), ) return collection diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index b7b646acac56..46015a50d750 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -14,11 +14,11 @@ ) from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_events.content_authoring.data import ( - ContentObjectData, + ContentObjectChangedData, LibraryCollectionData, ) from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_TAGS_CHANGED, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_COLLECTION_CREATED, LIBRARY_COLLECTION_UPDATED, ) @@ -262,7 +262,7 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTe Same guidelines as ContentLibrariesTestCase. """ ENABLED_OPENEDX_EVENTS = [ - CONTENT_OBJECT_TAGS_CHANGED.event_type, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.event_type, LIBRARY_COLLECTION_CREATED.event_type, LIBRARY_COLLECTION_UPDATED.event_type, ] @@ -411,10 +411,10 @@ def test_update_library_collection_components(self): def test_update_library_collection_components_event(self): """ - Check that a CONTENT_OBJECT_TAGS_CHANGED event is raised for each added/removed component. + Check that a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event is raised for each added/removed component. """ event_receiver = mock.Mock() - CONTENT_OBJECT_TAGS_CHANGED.connect(event_receiver) + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) LIBRARY_COLLECTION_UPDATED.connect(event_receiver) api.update_library_collection_components( @@ -440,20 +440,22 @@ def test_update_library_collection_components_event(self): ) self.assertDictContainsSubset( { - "signal": CONTENT_OBJECT_TAGS_CHANGED, + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "sender": None, - "content_object": ContentObjectData( + "content_object": ContentObjectChangedData( object_id=UsageKey.from_string(self.lib1_problem_block["id"]), + changes=["collections"], ), }, event_receiver.call_args_list[1].kwargs, ) self.assertDictContainsSubset( { - "signal": CONTENT_OBJECT_TAGS_CHANGED, + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "sender": None, - "content_object": ContentObjectData( + "content_object": ContentObjectChangedData( object_id=UsageKey.from_string(self.lib1_html_block["id"]), + changes=["collections"], ), }, event_receiver.call_args_list[2].kwargs, diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 47b157c1a34e..80de81958abc 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -18,8 +18,8 @@ from openedx_tagging.core.tagging.models.utils import TAGS_CSV_SEPARATOR from organizations.models import Organization from .helpers.objecttag_export_helpers import build_object_tree_with_objecttags, iterate_with_level -from openedx_events.content_authoring.data import ContentObjectData -from openedx_events.content_authoring.signals import CONTENT_OBJECT_TAGS_CHANGED +from openedx_events.content_authoring.data import ContentObjectChangedData +from openedx_events.content_authoring.signals import CONTENT_OBJECT_ASSOCIATIONS_CHANGED from .models import TaxonomyOrg from .types import ContentKey, TagValuesByObjectIdDict, TagValuesByTaxonomyIdDict, TaxonomyDict @@ -301,9 +301,12 @@ def set_exported_object_tags( create_invalid=True, taxonomy_export_id=str(taxonomy_export_id), ) - CONTENT_OBJECT_TAGS_CHANGED.send_event( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( time=now(), - content_object=ContentObjectData(object_id=content_key_str) + content_object=ContentObjectChangedData( + object_id=content_key_str, + changes=["tags"], + ) ) @@ -378,7 +381,7 @@ def tag_object( Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags, if the taxonomy can be used by the given object_id. - This is a wrapper around oel_tagging.tag_object that adds emitting the `CONTENT_OBJECT_TAGS_CHANGED` event + This is a wrapper around oel_tagging.tag_object that adds emitting the `CONTENT_OBJECT_ASSOCIATIONS_CHANGED` event when tagging an object. tags: A list of the values of the tags from this taxonomy to apply. @@ -399,9 +402,12 @@ def tag_object( taxonomy=taxonomy, tags=tags, ) - CONTENT_OBJECT_TAGS_CHANGED.send_event( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( time=now(), - content_object=ContentObjectData(object_id=object_id) + content_object=ContentObjectChangedData( + object_id=object_id, + changes=["tags"], + ) ) # Expose the oel_tagging APIs diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index 71b210b9e561..3fc99736bae9 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -13,8 +13,11 @@ from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView -from openedx_events.content_authoring.data import ContentObjectData -from openedx_events.content_authoring.signals import CONTENT_OBJECT_TAGS_CHANGED +from openedx_events.content_authoring.data import ContentObjectData, ContentObjectChangedData +from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + CONTENT_OBJECT_TAGS_CHANGED, +) from ...auth import has_view_object_tags_access from ...api import ( @@ -149,14 +152,24 @@ class ObjectTagOrgView(ObjectTagView): def update(self, request, *args, **kwargs) -> Response: """ - Extend the update method to fire CONTENT_OBJECT_TAGS_CHANGED event + Extend the update method to fire CONTENT_OBJECT_ASSOCIATIONS_CHANGED event """ response = super().update(request, *args, **kwargs) if response.status_code == 200: object_id = kwargs.get('object_id') + + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=object_id, + changes=["tags"], + ) + ) + + # Emit a (deprecated) CONTENT_OBJECT_TAGS_CHANGED event too CONTENT_OBJECT_TAGS_CHANGED.send_event( content_object=ContentObjectData(object_id=object_id) ) + return response From e20ee245915e0afbc7ae7fecd955cd06eec732da Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 6 Sep 2024 13:56:29 +0930 Subject: [PATCH 22/39] temp: use WIP openedx-events branch --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 4830f3522cb3..9097af24cc38 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 @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/kernel.in # edx-event-bus-kafka diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index e2ec6f46b81d..4e48379cfa5e 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 @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index e1de91706bdc..30f35ebaba8a 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 @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/base.txt # edx-event-bus-kafka diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 782bb0fe73e1..5f9417fd0326 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -119,7 +119,7 @@ openedx-calc # Library supporting mathematical calculatio openedx-django-require # 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@jill/content-object-associations +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals 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 654d2fa2361d..cb0a41683736 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 @ git+https://github.com/open-craft/openedx-events.git@jill/content-object-associations +openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals # via # -r requirements/edx/base.txt # edx-event-bus-kafka From 9619341dbcdef79de7f8bbbd83c106d062df7b58 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 6 Sep 2024 13:57:03 +0930 Subject: [PATCH 23/39] test: fix test failing after changes to openedx-learning --- openedx/core/djangoapps/content_libraries/tests/test_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 46015a50d750..b02e71b002a3 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -443,7 +443,7 @@ def test_update_library_collection_components_event(self): "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "sender": None, "content_object": ContentObjectChangedData( - object_id=UsageKey.from_string(self.lib1_problem_block["id"]), + object_id=self.lib1_problem_block["id"], changes=["collections"], ), }, @@ -454,7 +454,7 @@ def test_update_library_collection_components_event(self): "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "sender": None, "content_object": ContentObjectChangedData( - object_id=UsageKey.from_string(self.lib1_html_block["id"]), + object_id=self.lib1_html_block["id"], changes=["collections"], ), }, From 1764e87b5a37010c1962e02c2e5d026bd47523d0 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 6 Sep 2024 14:04:20 +0930 Subject: [PATCH 24/39] fix: emit both CONTENT_OBJECT_ASSOCIATIONS_CHANGED and CONTENT_OBJECT_TAGS_CHANGED in content_tagging app, while CONTENT_OBJECT_TAGS_CHANGED is being deprecated. --- .../core/djangoapps/content_tagging/api.py | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 80de81958abc..b63c70c5f0b8 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -18,8 +18,11 @@ from openedx_tagging.core.tagging.models.utils import TAGS_CSV_SEPARATOR from organizations.models import Organization from .helpers.objecttag_export_helpers import build_object_tree_with_objecttags, iterate_with_level -from openedx_events.content_authoring.data import ContentObjectChangedData -from openedx_events.content_authoring.signals import CONTENT_OBJECT_ASSOCIATIONS_CHANGED +from openedx_events.content_authoring.data import ContentObjectData, ContentObjectChangedData +from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + CONTENT_OBJECT_TAGS_CHANGED, +) from .models import TaxonomyOrg from .types import ContentKey, TagValuesByObjectIdDict, TagValuesByTaxonomyIdDict, TaxonomyDict @@ -301,6 +304,7 @@ def set_exported_object_tags( create_invalid=True, taxonomy_export_id=str(taxonomy_export_id), ) + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( time=now(), content_object=ContentObjectChangedData( @@ -309,6 +313,12 @@ def set_exported_object_tags( ) ) + # Emit a (deprecated) CONTENT_OBJECT_TAGS_CHANGED event too + CONTENT_OBJECT_TAGS_CHANGED.send_event( + time=now(), + content_object=ContentObjectData(object_id=content_key_str) + ) + def import_course_tags_from_csv(csv_path, course_id) -> None: """ @@ -410,6 +420,12 @@ def tag_object( ) ) + # Emit a (deprecated) CONTENT_OBJECT_TAGS_CHANGED event too + CONTENT_OBJECT_TAGS_CHANGED.send_event( + time=now(), + content_object=ContentObjectData(object_id=object_id) + ) + # Expose the oel_tagging APIs add_tag_to_taxonomy = oel_tagging.add_tag_to_taxonomy From ae7926f0a66f9b01ae15361eaf7017dd79f607b1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 6 Sep 2024 14:08:18 +0930 Subject: [PATCH 25/39] docs: minor comment update Collection.key is stored here, not ID --- openedx/core/djangoapps/content/search/documents.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index c92776e12a4b..27383260115e 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -171,7 +171,7 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: and in Collections 3 and 4, this would return: { "tags": { - "collections": [3, 4], + "collections": ["Col_key1", "Col_key2"], "taxonomy": ["Location", "Difficulty"], "level0": ["Location > North America", "Difficulty > Hard"], "level1": ["Location > North America > Canada"], From 007f80a2a70cb9eafe72e91d0de7749688c61a58 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 6 Sep 2024 14:45:17 +0930 Subject: [PATCH 26/39] fix: catch IntegrityError in create_library_collection and re-raise as api.LibraryCollectionAlreadyExists --- .../core/djangoapps/content_libraries/api.py | 21 ++++++++++++------- .../tests/test_views_collections.py | 20 ++++++++++++++---- .../djangoapps/content_libraries/views.py | 3 +++ 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 2253514ee0f9..00110143456a 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -130,6 +130,10 @@ class LibraryAlreadyExists(KeyError): """ A library with the specified slug already exists """ +class LibraryCollectionAlreadyExists(IntegrityError): + """ A Collection with that key already exists in the library """ + + class LibraryBlockAlreadyExists(KeyError): """ An XBlock with that ID already exists in the library """ @@ -1094,13 +1098,16 @@ def create_library_collection( assert content_library.learning_package_id assert content_library.library_key == library_key - collection = authoring_api.create_collection( - learning_package_id=content_library.learning_package_id, - key=collection_key, - title=title, - description=description, - created_by=created_by, - ) + try: + collection = authoring_api.create_collection( + learning_package_id=content_library.learning_package_id, + key=collection_key, + title=title, + description=description, + created_by=created_by, + ) + except IntegrityError as err: + raise LibraryCollectionAlreadyExists from err # Emit event for library collection created LIBRARY_COLLECTION_CREATED.send_event( 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 a4ef40613df7..ad027f2412bc 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -198,7 +198,7 @@ def test_create_invalid_library_collection(self): Test creating an invalid Content Library Collection """ post_data_missing_title = { - "description": "Description for Collection 4", + "key": "COL_KEY", } resp = self.client.post( URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_title, format="json" @@ -206,19 +206,31 @@ def test_create_invalid_library_collection(self): assert resp.status_code == 400 - post_data_missing_desc = { + post_data_missing_key = { "title": "Collection 4", } resp = self.client.post( - URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_desc, format="json" + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data_missing_key, format="json" ) assert resp.status_code == 400 + # Create collection with an existing collection.key; it should fail + post_data_existing_key = { + "key": self.col1.key, + "title": "Collection 4", + } + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), + post_data_existing_key, + 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}, + {**post_data_missing_title, **post_data_missing_key}, format="json" ) assert resp.status_code == 404 diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index cc1a58634d4f..835a5de1f1f2 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -149,6 +149,9 @@ def wrapped_fn(*args, **kwargs): except api.ContentLibraryCollectionNotFound: log.exception("Collection not found in content library") raise NotFound # lint-amnesty, pylint: disable=raise-missing-from + except api.LibraryCollectionAlreadyExists as exc: + log.exception(str(exc)) + raise ValidationError(str(exc)) # 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 436822e4fd05118b5de1c85e90d02b809c7d3f1c Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 10 Sep 2024 13:06:18 +0930 Subject: [PATCH 27/39] docs: replaces CONTENT_OBJECT_TAGS_CHANGED with CONTENT_OBJECT_ASSOCIATIONS_CHANGED --- docs/hooks/events.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/hooks/events.rst b/docs/hooks/events.rst index 2bceee5050ea..2ee070b8a276 100644 --- a/docs/hooks/events.rst +++ b/docs/hooks/events.rst @@ -244,10 +244,6 @@ Content Authoring Events - org.openedx.content_authoring.content_library.deleted.v1 - 2023-07-20 - * - `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 @@ -259,3 +255,7 @@ Content Authoring Events * - `LIBRARY_COLLECTION_DELETED `_ - org.openedx.content_authoring.content.library.collection.deleted.v1 - 2024-08-23 + + * - `CONTENT_OBJECT_ASSOCIATIONS_CHANGED `_ + - org.openedx.content_authoring.content.object.associations.changed.v1 + - 2024-09-06 From d517d801067409de9f0d35b596ce0d208c40ba55 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 10 Sep 2024 13:06:58 +0930 Subject: [PATCH 28/39] refactor: store collections outside of tags index --- openedx/core/djangoapps/content/search/api.py | 15 +++- .../djangoapps/content/search/documents.py | 53 +++++++++++--- .../djangoapps/content/search/handlers.py | 7 +- .../content/search/tests/test_api.py | 72 ++++++++++--------- .../content/search/tests/test_documents.py | 2 +- 5 files changed, 102 insertions(+), 47 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 00b908e26cc4..d601abde8e6d 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -34,6 +34,7 @@ searchable_doc_for_course_block, searchable_doc_for_collection, searchable_doc_for_library_block, + searchable_doc_collections, searchable_doc_tags, ) @@ -317,12 +318,12 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.context_key, Fields.org, Fields.tags, - Fields.tags + "." + Fields.tags_collections, Fields.tags + "." + Fields.tags_taxonomy, Fields.tags + "." + Fields.tags_level0, Fields.tags + "." + Fields.tags_level1, Fields.tags + "." + Fields.tags_level2, Fields.tags + "." + Fields.tags_level3, + Fields.collections, Fields.type, Fields.access_id, Fields.last_published, @@ -336,11 +337,11 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.content, Fields.tags, Fields.description, + Fields.collections, # If we don't list the following sub-fields _explicitly_, they're only sometimes searchable - that is, they # are searchable only if at least one document in the index has a value. If we didn't list them here and, # say, there were no tags.level3 tags in the index, the client would get an error if trying to search for # these sub-fields: "Attribute `tags.level3` is not searchable." - Fields.tags + "." + Fields.tags_collections, Fields.tags + "." + Fields.tags_taxonomy, Fields.tags + "." + Fields.tags_level0, Fields.tags + "." + Fields.tags_level1, @@ -377,6 +378,7 @@ def index_library(lib_key: str) -> list: doc = {} doc.update(searchable_doc_for_library_block(metadata)) doc.update(searchable_doc_tags(metadata.usage_key)) + doc.update(searchable_doc_collections(metadata.usage_key)) docs.append(doc) except Exception as err: # pylint: disable=broad-except status_cb(f"Error indexing library component {component}: {err}") @@ -573,6 +575,15 @@ def upsert_block_tags_index_docs(usage_key: UsageKey): _update_index_docs([doc]) +def upsert_block_collections_index_docs(usage_key: UsageKey): + """ + Updates the collections data in documents for the given Course/Library block + """ + doc = {Fields.id: meili_id_from_opaque_key(usage_key)} + doc.update(searchable_doc_collections(usage_key)) + _update_index_docs([doc]) + + def _get_user_orgs(request: Request) -> list[str]: """ Get the org.short_names for the organizations that the requesting user has OrgStaffRole or OrgInstructorRole. diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 27383260115e..51eca8816430 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -54,7 +54,8 @@ class Fields: tags_level1 = "level1" tags_level2 = "level2" tags_level3 = "level3" - tags_collections = "collections" # subfield of tags, i.e. tags.collections + # List of collection.key strings this object belongs to. + collections = "collections" # The "content" field is a dictionary of arbitrary data, depending on the block_type. # It comes from each XBlock's index_dictionary() method (if present) plus some processing. # Text (html) blocks have an "html_content" key in here, capa has "capa_content" and "problem_types", and so on. @@ -167,11 +168,10 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: See the comments above on "Field.tags" for an explanation of the format. - e.g. for something tagged "Difficulty: Hard" and "Location: Vancouver", - and in Collections 3 and 4, this would return: + e.g. for something tagged "Difficulty: Hard" and "Location: Vancouver" this + would return: { "tags": { - "collections": ["Col_key1", "Col_key2"], "taxonomy": ["Location", "Difficulty"], "level0": ["Location > North America", "Difficulty > Hard"], "level1": ["Location > North America > Canada"], @@ -186,16 +186,21 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: strings in a particular format that the frontend knows how to render to support hierarchical refinement by tag. """ - result = {} - # Note that we could improve performance for indexing many components from the same library/course, # if we used get_all_object_tags() to load all the tags for the library in a single query rather than loading the # tags for each component separately. all_tags = tagging_api.get_object_tags(str(object_id)).all() + if not all_tags: + # Clear out tags in the index when unselecting all tags for the block, otherwise + # it would remain the last value if a cleared Fields.tags field is not included + return {Fields.tags: {}} + result = { + Fields.tags_taxonomy: [], + Fields.tags_level0: [], + # ... other levels added as needed + } for obj_tag in all_tags: # Add the taxonomy name: - if Fields.tags_taxonomy not in result: - result[Fields.tags_taxonomy] = [] if obj_tag.taxonomy.name not in result[Fields.tags_taxonomy]: result[Fields.tags_taxonomy].append(obj_tag.taxonomy.name) # Taxonomy name plus each level of tags, in a list: # e.g. ["Location", "North America", "Canada", "Vancouver"] @@ -219,7 +224,22 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: if len(parts) == level + 2: break # We have all the levels for this tag now (e.g. parts=["Difficulty", "Hard"] -> need level0 only) + return {Fields.tags: result} + + +def _collections_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: + """ + Given an XBlock, course, library, etc., get the collections for its index doc. + + e.g. for something in Collections "COL_A" and "COL_B", this would return: + { + "collections": ["COL_A", "COL_B"], + } + + Returns an empty dict if the object is not in any collections. + """ # Gather the collections associated with this object + result = {} collections = [] try: component = lib_api.get_component_from_usage_key(object_id) @@ -231,9 +251,9 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: log.warning(f"No component found for {object_id}") if collections: - result[Fields.tags_collections] = list(collections) + result[Fields.collections] = list(collections) - return {Fields.tags: result} + return result def searchable_doc_for_library_block(xblock_metadata: lib_api.LibraryXBlockMetadata) -> dict: @@ -278,6 +298,19 @@ def searchable_doc_tags(usage_key: UsageKey) -> dict: return doc +def searchable_doc_collections(usage_key: UsageKey) -> dict: + """ + Generate a dictionary document suitable for ingestion into a search engine + like Meilisearch or Elasticsearch, with the collections data for the given content object. + """ + doc = { + Fields.id: meili_id_from_opaque_key(usage_key), + } + doc.update(_collections_for_content_object(usage_key)) + + return doc + + def searchable_doc_for_course_block(block) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 7b008b7a454e..bcd7120d79bc 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -163,4 +163,9 @@ def content_object_associations_changed_handler(**kwargs) -> None: log.error("Received invalid content object id") return - upsert_block_tags_index_docs(usage_key) + # This event's changes may contain both "tags" and "collections", but this will happen rarely, if ever. + # So we allow a potential double "upsert" here. + if "tags" in content_object.changes: + upsert_block_tags_index_docs(usage_key) + elif "collections" in content_object.changes: + upsert_block_collections_index_docs(usage_key) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 68a734c9e8aa..f8f13d8b9a6f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -175,7 +175,7 @@ def setUp(self): tagging_api.add_tag_to_taxonomy(self.taxonomyB, "three") tagging_api.add_tag_to_taxonomy(self.taxonomyB, "four") - # Create collections: + # 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( @@ -379,6 +379,40 @@ def test_index_library_block_tags(self, mock_meilisearch): """ Test indexing an Library Block with tags. """ + + # Tag XBlock (these internally call `upsert_block_tags_index_docs`) + tagging_api.tag_object(str(self.problem1.usage_key), self.taxonomyA, ["one", "two"]) + tagging_api.tag_object(str(self.problem1.usage_key), self.taxonomyB, ["three", "four"]) + + # Build expected docs with tags at each stage + doc_problem_with_tags1 = { + "id": self.doc_problem1["id"], + "tags": { + 'taxonomy': ['A'], + 'level0': ['A > one', 'A > two'] + } + } + doc_problem_with_tags2 = { + "id": self.doc_problem1["id"], + "tags": { + 'taxonomy': ['A', 'B'], + 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] + } + } + + mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( + [ + call([doc_problem_with_tags1]), + call([doc_problem_with_tags2]), + ], + any_order=True, + ) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_index_library_block_collections(self, mock_meilisearch): + """ + Test indexing an Library Block that is in two collections. + """ collection1 = authoring_api.create_collection( learning_package_id=self.library.learning_package.id, key="COL1", @@ -395,11 +429,7 @@ def test_index_library_block_tags(self, mock_meilisearch): description="Second Collection", ) - # Tag XBlock (these internally call `upsert_block_tags_index_docs`) - tagging_api.tag_object(str(self.problem1.usage_key), self.taxonomyA, ["one", "two"]) - tagging_api.tag_object(str(self.problem1.usage_key), self.taxonomyB, ["three", "four"]) - - # Add Problem1 to both Collections (these internally call `upsert_block_tags_index_docs`) + # Add Problem1 to both Collections (these internally call `upsert_block_collections_index_docs`) # (adding in reverse order to test sorting of collection tag)) for collection in (collection2, collection1): library_api.update_library_collection_components( @@ -410,42 +440,18 @@ def test_index_library_block_tags(self, mock_meilisearch): ], ) - # Build expected docs with tags at each stage - doc_problem_with_tags1 = { - "id": self.doc_problem1["id"], - "tags": { - 'taxonomy': ['A'], - 'level0': ['A > one', 'A > two'] - } - } - doc_problem_with_tags2 = { - "id": self.doc_problem1["id"], - "tags": { - 'taxonomy': ['A', 'B'], - 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] - } - } + # Build expected docs with collections at each stage doc_problem_with_collection2 = { "id": self.doc_problem1["id"], - "tags": { - 'collections': [collection2.key], - 'taxonomy': ['A', 'B'], - 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] - } + "collections": [collection2.key], } doc_problem_with_collection1 = { "id": self.doc_problem1["id"], - "tags": { - 'collections': [collection1.key, collection2.key], - 'taxonomy': ['A', 'B'], - 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] - } + "collections": [collection1.key, collection2.key], } mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ - call([doc_problem_with_tags1]), - call([doc_problem_with_tags2]), call([doc_problem_with_collection2]), call([doc_problem_with_collection1]), ], diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index c8442f406460..62ae6d66593a 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -282,8 +282,8 @@ def test_html_library_block(self): "content": { "html_content": "", }, + "collections": ["TOY_COLLECTION"], "tags": { - "collections": ["TOY_COLLECTION"], "taxonomy": ["Difficulty"], "level0": ["Difficulty > Normal"], }, From d312ff3e6aa1f02785f2cd2dd68d54df02359ad1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 10 Sep 2024 13:09:40 +0930 Subject: [PATCH 29/39] chore: updates openedx-events==9.14.0 and fixes event types documented in the hooks. --- docs/hooks/events.rst | 12 ++++++------ 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 +- 6 files changed, 11 insertions(+), 13 deletions(-) diff --git a/docs/hooks/events.rst b/docs/hooks/events.rst index 2bceee5050ea..2a7561df24e1 100644 --- a/docs/hooks/events.rst +++ b/docs/hooks/events.rst @@ -233,15 +233,15 @@ Content Authoring Events - 2023-07-20 * - `LIBRARY_BLOCK_CREATED `_ - - org.openedx.content_authoring.content_library.created.v1 + - org.openedx.content_authoring.library_block.created.v1 - 2023-07-20 * - `LIBRARY_BLOCK_UPDATED `_ - - org.openedx.content_authoring.content_library.updated.v1 + - org.openedx.content_authoring.library_block.updated.v1 - 2023-07-20 * - `LIBRARY_BLOCK_DELETED `_ - - org.openedx.content_authoring.content_library.deleted.v1 + - org.openedx.content_authoring.library_block.deleted.v1 - 2023-07-20 * - `CONTENT_OBJECT_TAGS_CHANGED `_ @@ -249,13 +249,13 @@ Content Authoring Events - 2024-03-31 * - `LIBRARY_COLLECTION_CREATED `_ - - org.openedx.content_authoring.content.library.collection.created.v1 + - org.openedx.content_authoring.content_library.collection.created.v1 - 2024-08-23 * - `LIBRARY_COLLECTION_UPDATED `_ - - org.openedx.content_authoring.content.library.collection.updated.v1 + - org.openedx.content_authoring.content_library.collection.updated.v1 - 2024-08-23 * - `LIBRARY_COLLECTION_DELETED `_ - - org.openedx.content_authoring.content.library.collection.deleted.v1 + - org.openedx.content_authoring.content_library.collection.deleted.v1 - 2024-08-23 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 4f4675a40fd7..d85039139ad0 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events==9.14.0 # via # -r requirements/edx/kernel.in # edx-enterprise diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 2f952b16867b..8f401727ded1 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events==9.14.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 5a95515a1670..9a3e3ea80367 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events==9.14.0 # via # -r requirements/edx/base.txt # edx-enterprise diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 5f9417fd0326..a5b510742ac7 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -117,9 +117,7 @@ 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) -# TODO: Remove this once new version released -openedx-events @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events # Open edX Events from Hooks Extension Framework (OEP-50) 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 af9dcf957a90..40d1f7e8dcb9 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 @ git+https://github.com/open-craft/openedx-events.git@yusuf-musleh/content-library-collections-signals +openedx-events==9.14.0 # via # -r requirements/edx/base.txt # edx-enterprise From 66b2aa89478b59040c207345fb37cf77815d779d Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 10 Sep 2024 13:34:02 +0930 Subject: [PATCH 30/39] fix: missing import --- openedx/core/djangoapps/content/search/handlers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index a6126501ef2c..0c2054ad39a8 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -29,13 +29,13 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.search.models import SearchAccess -from .api import only_if_meilisearch_enabled, upsert_block_tags_index_docs +from .api import only_if_meilisearch_enabled, upsert_block_collections_index_docs, upsert_block_tags_index_docs from .tasks import ( delete_library_block_index_doc, delete_xblock_index_doc, update_content_library_index_docs, upsert_library_block_index_doc, - upsert_xblock_index_doc + upsert_xblock_index_doc, ) log = logging.getLogger(__name__) From d5aeff8ad40638c694cafe10144e73f7fae2db92 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 11 Sep 2024 10:27:39 +0930 Subject: [PATCH 31/39] chore: updates openedx-learning==0.11.4 --- 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 42b1679ffc0c..b5d838156124 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/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # 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 abc0d6779c84..40d64855cb52 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -824,7 +824,7 @@ openedx-filters==1.9.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 05d1fc065c2b..a18ddb26b8b9 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1373,7 +1373,7 @@ openedx-filters==1.9.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index deabf7e3cf88..00fc580dedc4 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -983,7 +983,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 3cd689280103..966bd772a876 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1034,7 +1034,7 @@ openedx-filters==1.9.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning @ git+https://github.com/open-craft/openedx-learning/@jill/collection-key +openedx-learning==0.11.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From e6b469d8f1d687ac9cb7b08df6333fbea216633f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Tue, 10 Sep 2024 20:15:25 -0500 Subject: [PATCH 32/39] refactor: Update collections crud rest api (#683) * Update description as optional in ContentLibraryCollectionUpdateSerializer * Create collection Rest API to auto-generate key --- .../content_libraries/serializers.py | 10 +----- .../tests/test_views_collections.py | 29 ++++++++++++++-- .../content_libraries/views_collections.py | 34 +++++++++++++------ 3 files changed, 51 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 12a11e96e13c..2062f96d93ae 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -268,15 +268,7 @@ class ContentLibraryCollectionUpdateSerializer(serializers.Serializer): """ title = serializers.CharField() - description = serializers.CharField() - - -class ContentLibraryCollectionCreateSerializer(ContentLibraryCollectionUpdateSerializer): - """ - Serializer for adding a Collection in a Content Library - """ - - key = serializers.CharField() + description = serializers.CharField(allow_blank=True) class UsageKeyV2Serializer(serializers.Serializer): 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 ad027f2412bc..bc600759b5b3 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -162,16 +162,15 @@ def test_create_library_collection(self): Test creating a Content Library Collection """ post_data = { - "key": "COL4", "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 + post_data["key"] = 'collection-4' self.assertDictContainsEntries(resp.data, post_data) created_collection = Collection.objects.get(id=resp.data["id"]) @@ -183,7 +182,6 @@ def test_create_library_collection(self): with self.as_user(reader): post_data = { - "key": "COL5", "title": "Collection 5", "description": "Description for Collection 5", } @@ -193,6 +191,31 @@ def test_create_library_collection(self): assert resp.status_code == 403 + def test_create_collection_same_key(self): + """ + Test collection creation with same key + """ + post_data = { + "title": "Same Collection", + "description": "Description for Collection 4", + } + self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + + for i in range(100): + resp = self.client.post( + URL_LIB_COLLECTIONS.format(lib_key=self.lib1.library_key), post_data, format="json" + ) + expected_data = { + "key": f"same-collection-{i + 1}", + "title": "Same Collection", + "description": "Description for Collection 4", + } + + assert resp.status_code == 200 + self.assertDictContainsEntries(resp.data, expected_data) + def test_create_invalid_library_collection(self): """ Test creating an invalid Content Library Collection diff --git a/openedx/core/djangoapps/content_libraries/views_collections.py b/openedx/core/djangoapps/content_libraries/views_collections.py index 482b6e2bb9f1..2f40a1788628 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -5,6 +5,8 @@ from __future__ import annotations from django.db.models import QuerySet +from django.utils.text import slugify +from django.db import transaction from rest_framework.decorators import action from rest_framework.response import Response @@ -21,7 +23,6 @@ from openedx.core.djangoapps.content_libraries.serializers import ( ContentLibraryCollectionSerializer, ContentLibraryCollectionComponentsUpdateSerializer, - ContentLibraryCollectionCreateSerializer, ContentLibraryCollectionUpdateSerializer, ) @@ -109,17 +110,30 @@ def create(self, request, *args, **kwargs) -> Response: Create a Collection that belongs to a Content Library """ content_library = self.get_content_library() - create_serializer = ContentLibraryCollectionCreateSerializer(data=request.data) + create_serializer = ContentLibraryCollectionUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) - collection = api.create_library_collection( - library_key=content_library.library_key, - content_library=content_library, - collection_key=create_serializer.validated_data["key"], - title=create_serializer.validated_data["title"], - description=create_serializer.validated_data["description"], - created_by=request.user.id, - ) + title = create_serializer.validated_data['title'] + key = slugify(title) + + attempt = 0 + collection = None + while not collection: + modified_key = key if attempt == 0 else key + '-' + str(attempt) + try: + # Add transaction here to avoid TransactionManagementError on retry + with transaction.atomic(): + collection = api.create_library_collection( + library_key=content_library.library_key, + content_library=content_library, + collection_key=modified_key, + title=title, + description=create_serializer.validated_data["description"], + created_by=request.user.id, + ) + except api.LibraryCollectionAlreadyExists: + attempt += 1 + serializer = self.get_serializer(collection) return Response(serializer.data) From bd448743c974447520650949c3f973a05836bf5b Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 11 Sep 2024 12:16:34 +0930 Subject: [PATCH 33/39] test: fixed failing test after refactor --- openedx/core/djangoapps/content/search/tests/test_documents.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 62ae6d66593a..d972c9ac4956 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -19,6 +19,7 @@ from ..documents import ( searchable_doc_for_course_block, searchable_doc_tags, + searchable_doc_collections, searchable_doc_for_collection, searchable_doc_for_library_block, ) @@ -261,6 +262,7 @@ def test_html_library_block(self): doc = {} doc.update(searchable_doc_for_library_block(self.library_block)) doc.update(searchable_doc_tags(self.library_block.usage_key)) + doc.update(searchable_doc_collections(self.library_block.usage_key)) assert doc == { "id": "lbedx2012_fallhtmltext2-4bb47d67", "type": "library_block", From bfd548bffa623fe8bb2ca76afdfddf0dd6d1ab04 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 11 Sep 2024 14:14:37 +0930 Subject: [PATCH 34/39] feat: index collections when created or updated --- openedx/core/djangoapps/content/search/api.py | 16 +++ .../djangoapps/content/search/handlers.py | 26 +++- .../core/djangoapps/content/search/tasks.py | 13 ++ .../content/search/tests/test_api.py | 129 +++++++++++++----- 4 files changed, 147 insertions(+), 37 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index d601abde8e6d..24f6a012478a 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -553,6 +553,22 @@ def upsert_library_block_index_doc(usage_key: UsageKey) -> None: _update_index_docs(docs) +def upsert_library_collection_index_doc(library_key: LibraryLocatorV2, collection_key: str) -> None: + """ + Creates or updates the document for the given Library Collection in the search index + """ + content_library = lib_api.ContentLibrary.objects.get_by_key(library_key) + collection = authoring_api.get_collection( + learning_package_id=content_library.learning_package_id, + collection_key=collection_key, + ) + docs = [ + searchable_doc_for_collection(collection) + ] + + _update_index_docs(docs) + + def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: """ Creates or updates the documents for the given Content Library in the search index diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 0c2054ad39a8..47583baad091 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -6,20 +6,23 @@ from django.db.models.signals import post_delete from django.dispatch import receiver +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey from openedx_events.content_authoring.data import ( ContentLibraryData, ContentObjectChangedData, LibraryBlockData, + LibraryCollectionData, XBlockData, ) -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import UsageKey from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, XBLOCK_CREATED, XBLOCK_DELETED, XBLOCK_UPDATED, @@ -34,6 +37,7 @@ delete_library_block_index_doc, delete_xblock_index_doc, update_content_library_index_docs, + update_library_collection_index_doc, upsert_library_block_index_doc, upsert_xblock_index_doc, ) @@ -151,6 +155,24 @@ def content_library_updated_handler(**kwargs) -> None: update_content_library_index_docs.apply(args=[str(content_library_data.library_key)]) +@receiver(LIBRARY_COLLECTION_CREATED) +@receiver(LIBRARY_COLLECTION_UPDATED) +@only_if_meilisearch_enabled +def library_collection_updated_handler(**kwargs) -> None: + """ + Create or update the index for the content library collection + """ + library_collection = kwargs.get("library_collection", None) + if not library_collection or not isinstance(library_collection, LibraryCollectionData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + update_library_collection_index_doc.delay( + str(library_collection.library_key), + library_collection.collection_key, + ) + + @receiver(CONTENT_OBJECT_ASSOCIATIONS_CHANGED) @only_if_meilisearch_enabled def content_object_associations_changed_handler(**kwargs) -> None: diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index dfd603776981..d9dad834db29 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -84,3 +84,16 @@ def update_content_library_index_docs(library_key_str: str) -> None: # Delete all documents in this library that were not published by above function # as this task is also triggered on discard event. api.delete_all_draft_docs_for_library(library_key) + + +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) +@set_code_owner_attribute +def update_library_collection_index_doc(library_key_str: str, collection_key: str) -> None: + """ + Celery task to update the content index documents for a library collection + """ + library_key = LibraryLocatorV2.from_string(library_key_str) + + log.info("Updating content index documents for collection %s in library%s", collection_key, library_key) + + api.upsert_library_collection_index_doc(library_key, collection_key) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index f8f13d8b9a6f..d075681798bc 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -186,16 +186,16 @@ def setUp(self): description="my collection description" ) self.collection_dict = { - 'id': 'MYCOL', - 'type': 'collection', - 'display_name': 'my_collection', - 'description': 'my collection description', - 'context_key': 'lib:org1:lib', - 'org': 'org1', - 'created': created_date.timestamp(), - 'modified': created_date.timestamp(), + "id": "MYCOL", + "type": "collection", + "display_name": "my_collection", + "description": "my collection description", + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": created_date.timestamp(), "access_id": lib_access.id, - 'breadcrumbs': [{'display_name': 'Library'}] + "breadcrumbs": [{"display_name": "Library"}], } @override_settings(MEILISEARCH_ENABLED=False) @@ -409,38 +409,93 @@ def test_index_library_block_tags(self, mock_meilisearch): ) @override_settings(MEILISEARCH_ENABLED=True) - def test_index_library_block_collections(self, mock_meilisearch): + def test_index_library_block_and_collections(self, mock_meilisearch): """ - Test indexing an Library Block that is in two collections. + Test indexing an Library Block and the Collections it's in. """ - collection1 = authoring_api.create_collection( - learning_package_id=self.library.learning_package.id, - key="COL1", - title="Collection 1", - created_by=None, - description="First Collection", - ) - - collection2 = authoring_api.create_collection( - learning_package_id=self.library.learning_package.id, - key="COL2", - title="Collection 2", - created_by=None, - description="Second Collection", - ) + # Create collections (these internally call `upsert_library_collection_index_doc`) + created_date = datetime(2023, 5, 6, 7, 8, 9, tzinfo=timezone.utc) + with freeze_time(created_date): + collection1 = library_api.create_library_collection( + self.library.key, + collection_key="COL1", + title="Collection 1", + created_by=None, + description="First Collection", + ) - # Add Problem1 to both Collections (these internally call `upsert_block_collections_index_docs`) - # (adding in reverse order to test sorting of collection tag)) - for collection in (collection2, collection1): - library_api.update_library_collection_components( + collection2 = library_api.create_library_collection( self.library.key, - collection_key=collection.key, - usage_keys=[ - self.problem1.usage_key, - ], + collection_key="COL2", + title="Collection 2", + created_by=None, + description="Second Collection", ) - # Build expected docs with collections at each stage + # Add Problem1 to both Collections (these internally call `upsert_block_collections_index_docs` and + # `upsert_library_collection_index_doc`) + # (adding in reverse order to test sorting of collection tag) + updated_date = datetime(2023, 6, 7, 8, 9, 10, tzinfo=timezone.utc) + with freeze_time(updated_date): + for collection in (collection2, collection1): + library_api.update_library_collection_components( + self.library.key, + collection_key=collection.key, + usage_keys=[ + self.problem1.usage_key, + ], + ) + + # Build expected docs at each stage + lib_access, _ = SearchAccess.objects.get_or_create(context_key=self.library.key) + doc_collection1_created = { + "id": "COL1", + "type": "collection", + "display_name": "Collection 1", + "description": "First Collection", + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": created_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } + doc_collection2_created = { + "id": "COL2", + "type": "collection", + "display_name": "Collection 2", + "description": "Second Collection", + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": created_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } + doc_collection2_updated = { + "id": "COL2", + "type": "collection", + "display_name": "Collection 2", + "description": "Second Collection", + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": updated_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } + doc_collection1_updated = { + "id": "COL1", + "type": "collection", + "display_name": "Collection 1", + "description": "First Collection", + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": updated_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } doc_problem_with_collection2 = { "id": self.doc_problem1["id"], "collections": [collection2.key], @@ -452,6 +507,10 @@ def test_index_library_block_collections(self, mock_meilisearch): mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ + call([doc_collection1_created]), + call([doc_collection2_created]), + call([doc_collection2_updated]), + call([doc_collection1_updated]), call([doc_problem_with_collection2]), call([doc_problem_with_collection1]), ], From 0037530d32408d9a417665285b061fdd83439afc Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 11 Sep 2024 14:15:31 +0930 Subject: [PATCH 35/39] feat: adds num_children to collection index --- openedx/core/djangoapps/content/search/documents.py | 5 +++++ openedx/core/djangoapps/content/search/tests/test_api.py | 5 +++++ .../core/djangoapps/content/search/tests/test_documents.py | 2 ++ 3 files changed, 12 insertions(+) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 51eca8816430..e45c9fb3f634 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -61,6 +61,10 @@ class Fields: # Text (html) blocks have an "html_content" key in here, capa has "capa_content" and "problem_types", and so on. content = "content" + # Collections use this field to communicate how many entities/components they contain. + # Structural XBlocks may use this one day to indicate how many child blocks they ocntain. + num_children = "num_children" + # Note: new fields or values can be added at any time, but if they need to be indexed for filtering or keyword # search, the index configuration will need to be changed, which is only done as part of the 'reindex_studio' # command (changing those settings on an large active index is not recommended). @@ -344,6 +348,7 @@ def searchable_doc_for_collection(collection) -> dict: # If related contentlibrary is found, it will override this value below. # Mostly contentlibrary.library_key == learning_package.key Fields.context_key: collection.learning_package.key, + Fields.num_children: collection.entities.count(), } # Just in case learning_package is not related to a library try: diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index d075681798bc..ec9975724753 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -190,6 +190,7 @@ def setUp(self): "type": "collection", "display_name": "my_collection", "description": "my collection description", + "num_children": 0, "context_key": "lib:org1:lib", "org": "org1", "created": created_date.timestamp(), @@ -453,6 +454,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "type": "collection", "display_name": "Collection 1", "description": "First Collection", + "num_children": 0, "context_key": "lib:org1:lib", "org": "org1", "created": created_date.timestamp(), @@ -465,6 +467,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "type": "collection", "display_name": "Collection 2", "description": "Second Collection", + "num_children": 0, "context_key": "lib:org1:lib", "org": "org1", "created": created_date.timestamp(), @@ -477,6 +480,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "type": "collection", "display_name": "Collection 2", "description": "Second Collection", + "num_children": 1, "context_key": "lib:org1:lib", "org": "org1", "created": created_date.timestamp(), @@ -489,6 +493,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "type": "collection", "display_name": "Collection 1", "description": "First Collection", + "num_children": 1, "context_key": "lib:org1:lib", "org": "org1", "created": created_date.timestamp(), diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index d972c9ac4956..90ac9c1a70bb 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -299,6 +299,7 @@ def test_collection_with_library(self): "org": "edX", "display_name": "Toy Collection", "description": "my toy collection description", + "num_children": 1, "context_key": "lib:edX:2012_Fall", "access_id": self.library_access_id, "breadcrumbs": [{"display_name": "some content_library"}], @@ -327,6 +328,7 @@ def test_collection_with_no_library(self): "type": "collection", "display_name": "my_collection", "description": "my collection description", + "num_children": 0, "context_key": learning_package.key, "access_id": self.toy_course_access_id, "breadcrumbs": [{"display_name": "some learning_package"}], From 5bdcc9eeabaac38fba151a943a614cc5b01ce104 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 12 Sep 2024 14:08:53 +0930 Subject: [PATCH 36/39] fix: store collection.key in the document block_id field --- openedx/core/djangoapps/content/search/documents.py | 9 +++++++-- openedx/core/djangoapps/content/search/tests/test_api.py | 7 ++++++- .../djangoapps/content/search/tests/test_documents.py | 2 ++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index ab73123d7572..6a6c920a71db 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -28,7 +28,11 @@ class Fields: id = "id" usage_key = "usage_key" type = "type" # DocType.course_block or DocType.library_block (see below) - block_id = "block_id" # The block_id part of the usage key. Sometimes human-readable, sometimes a random hex ID + # The block_id part of the usage key for course or library blocks. + # If it's a collection, the collection.key is stored here. + # Sometimes human-readable, sometimes a random hex ID + # Is only unique within the given context_key. + block_id = "block_id" display_name = "display_name" description = "description" modified = "modified" @@ -54,7 +58,7 @@ class Fields: tags_level1 = "level1" tags_level2 = "level2" tags_level3 = "level3" - # List of collection.key strings this object belongs to. + # List of collection.block_id strings this object belongs to. collections = "collections" # The "content" field is a dictionary of arbitrary data, depending on the block_type. # It comes from each XBlock's index_dictionary() method (if present) plus some processing. @@ -339,6 +343,7 @@ def searchable_doc_for_collection(collection) -> dict: """ doc = { Fields.id: collection.id, + Fields.block_id: collection.key, Fields.type: DocType.collection, Fields.display_name: collection.title, Fields.description: collection.description, diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index e4a004635103..3105ad8c007f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -186,7 +186,8 @@ def setUp(self): description="my collection description" ) self.collection_dict = { - 'id': self.collection.id, + "id": self.collection.id, + "block_id": self.collection.key, "type": "collection", "display_name": "my_collection", "description": "my collection description", @@ -451,6 +452,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): lib_access, _ = SearchAccess.objects.get_or_create(context_key=self.library.key) doc_collection1_created = { "id": collection1.id, + "block_id": collection1.key, "type": "collection", "display_name": "Collection 1", "description": "First Collection", @@ -464,6 +466,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): } doc_collection2_created = { "id": collection2.id, + "block_id": collection2.key, "type": "collection", "display_name": "Collection 2", "description": "Second Collection", @@ -477,6 +480,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): } doc_collection2_updated = { "id": collection2.id, + "block_id": collection2.key, "type": "collection", "display_name": "Collection 2", "description": "Second Collection", @@ -490,6 +494,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch): } doc_collection1_updated = { "id": collection1.id, + "block_id": collection1.key, "type": "collection", "display_name": "Collection 1", "description": "First Collection", diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 990ec4db9dcb..47a89b2068a8 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -295,6 +295,7 @@ def test_collection_with_library(self): doc = searchable_doc_for_collection(self.collection) assert doc == { "id": self.collection.id, + "block_id": self.collection.key, "type": "collection", "org": "edX", "display_name": "Toy Collection", @@ -325,6 +326,7 @@ def test_collection_with_no_library(self): doc = searchable_doc_for_collection(collection) assert doc == { "id": collection.id, + "block_id": collection.key, "type": "collection", "display_name": "my_collection", "description": "my collection description", From a81ea9afd20a27918f376983f65cb7ee1c32667c Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 12 Sep 2024 14:52:25 +0930 Subject: [PATCH 37/39] fix: store library block collections as a dict in search index so we can index on the collection key or title. --- openedx/core/djangoapps/content/search/api.py | 6 +++- .../djangoapps/content/search/documents.py | 36 ++++++++++++++----- .../content/search/tests/test_api.py | 23 +++++++++--- .../content/search/tests/test_documents.py | 5 ++- 4 files changed, 55 insertions(+), 15 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index d601abde8e6d..dda4bea63ac9 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -324,6 +324,8 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.tags + "." + Fields.tags_level2, Fields.tags + "." + Fields.tags_level3, Fields.collections, + Fields.collections + "." + Fields.collections_display_name, + Fields.collections + "." + Fields.collections_key, Fields.type, Fields.access_id, Fields.last_published, @@ -335,8 +337,8 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.display_name, Fields.block_id, Fields.content, - Fields.tags, Fields.description, + Fields.tags, Fields.collections, # If we don't list the following sub-fields _explicitly_, they're only sometimes searchable - that is, they # are searchable only if at least one document in the index has a value. If we didn't list them here and, @@ -347,6 +349,8 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.tags + "." + Fields.tags_level1, Fields.tags + "." + Fields.tags_level2, Fields.tags + "." + Fields.tags_level3, + Fields.collections + "." + Fields.collections_display_name, + Fields.collections + "." + Fields.collections_key, ]) # Mark which attributes can be used for sorting search results: client.index(temp_index_name).update_sortable_attributes([ diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 1d16c6908034..b5cd8dabddd3 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -54,8 +54,12 @@ class Fields: tags_level1 = "level1" tags_level2 = "level2" tags_level3 = "level3" - # List of collection.key strings this object belongs to. + # Collections (dictionary) that this object belongs to. + # Similarly to tags above, we collect the collection.titles and collection.keys into hierarchical facets. collections = "collections" + collections_display_name = "display_name" + collections_key = "key" + # The "content" field is a dictionary of arbitrary data, depending on the block_type. # It comes from each XBlock's index_dictionary() method (if present) plus some processing. # Text (html) blocks have an "html_content" key in here, capa has "capa_content" and "problem_types", and so on. @@ -233,25 +237,41 @@ def _collections_for_content_object(object_id: UsageKey | LearningContextKey) -> e.g. for something in Collections "COL_A" and "COL_B", this would return: { - "collections": ["COL_A", "COL_B"], + "collections": { + "display_name": ["Collection A", "Collection B"], + "key": ["COL_A", "COL_B"], + } + } + + If the object is in no collections, returns: + { + "collections": {}, } - Returns an empty dict if the object is not in any collections. """ # Gather the collections associated with this object - result = {} - collections = [] + collections = None try: component = lib_api.get_component_from_usage_key(object_id) collections = authoring_api.get_entity_collections( component.learning_package_id, component.key, - ).values_list("key", flat=True) + ) except ObjectDoesNotExist: log.warning(f"No component found for {object_id}") - if collections: - result[Fields.collections] = list(collections) + if not collections: + return {Fields.collections: {}} + + result = { + Fields.collections: { + Fields.collections_display_name: [], + Fields.collections_key: [], + } + } + for collection in collections: + result[Fields.collections][Fields.collections_display_name].append(collection.title) + result[Fields.collections][Fields.collections_key].append(collection.key) return result diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 5d54ddb0cf28..0e5de4da91fe 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -215,10 +215,13 @@ def test_reindex_meilisearch(self, mock_meilisearch): doc_vertical["tags"] = {} doc_problem1 = copy.deepcopy(self.doc_problem1) doc_problem1["tags"] = {} + doc_problem1["collections"] = {} doc_problem2 = copy.deepcopy(self.doc_problem2) doc_problem2["tags"] = {} + doc_problem2["collections"] = {} api.rebuild_index() + assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 3 mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls( [ call([doc_sequential, doc_vertical]), @@ -254,6 +257,7 @@ def test_reindex_meilisearch_library_block_error(self, mock_meilisearch): doc_vertical["tags"] = {} doc_problem2 = copy.deepcopy(self.doc_problem2) doc_problem2["tags"] = {} + doc_problem2["collections"] = {} orig_from_component = library_api.LibraryXBlockMetadata.from_component @@ -346,6 +350,7 @@ def test_index_xblock_tags(self, mock_meilisearch): } } + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 2 mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ call([doc_sequential_with_tags1]), @@ -400,6 +405,7 @@ def test_index_library_block_tags(self, mock_meilisearch): } } + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 2 mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ call([doc_problem_with_tags1]), @@ -441,19 +447,26 @@ def test_index_library_block_collections(self, mock_meilisearch): ) # Build expected docs with collections at each stage - doc_problem_with_collection2 = { + doc_problem_with_collection1 = { "id": self.doc_problem1["id"], - "collections": [collection2.key], + "collections": { + "display_name": ["Collection 2"], + "key": ["COL2"], + }, } - doc_problem_with_collection1 = { + doc_problem_with_collection2 = { "id": self.doc_problem1["id"], - "collections": [collection1.key, collection2.key], + "collections": { + "display_name": ["Collection 1", "Collection 2"], + "key": ["COL1", "COL2"], + }, } + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 2 mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ - call([doc_problem_with_collection2]), call([doc_problem_with_collection1]), + call([doc_problem_with_collection2]), ], any_order=True, ) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index c2b12d609054..97dcb5e79066 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -284,7 +284,10 @@ def test_html_library_block(self): "content": { "html_content": "", }, - "collections": ["TOY_COLLECTION"], + "collections": { + "key": ["TOY_COLLECTION"], + "display_name": ["Toy Collection"], + }, "tags": { "taxonomy": ["Difficulty"], "level0": ["Difficulty > Normal"], From 360ec35720bca6f62ab73fa5e6ace383d524ee10 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 12 Sep 2024 15:27:24 +0930 Subject: [PATCH 38/39] fix: reindex tags + collections if no changes specified --- openedx/core/djangoapps/content/search/handlers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 0c2054ad39a8..35f3ad68ee21 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -171,7 +171,7 @@ def content_object_associations_changed_handler(**kwargs) -> None: # This event's changes may contain both "tags" and "collections", but this will happen rarely, if ever. # So we allow a potential double "upsert" here. - if "tags" in content_object.changes: + if not content_object.changes or "tags" in content_object.changes: upsert_block_tags_index_docs(usage_key) - elif "collections" in content_object.changes: + if not content_object.changes or "collections" in content_object.changes: upsert_block_collections_index_docs(usage_key) From 2c6c8cf9d98ff28a60c2f928115544e6906e296b Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 12 Sep 2024 15:39:13 +0930 Subject: [PATCH 39/39] fix: apply library collection changes synchronously to update the index before the frontend re-fetches --- openedx/core/djangoapps/content/search/handlers.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index b41c0e5d7851..6a341c92ed2b 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -167,10 +167,13 @@ def library_collection_updated_handler(**kwargs) -> None: log.error("Received null or incorrect data for event") return - update_library_collection_index_doc.delay( + # Update collection index synchronously to make sure that search index is updated before + # the frontend invalidates/refetches index. + # See content_library_updated_handler for more details. + update_library_collection_index_doc.apply(args=[ str(library_collection.library_key), library_collection.collection_key, - ) + ]) @receiver(CONTENT_OBJECT_ASSOCIATIONS_CHANGED)