From bf17dbbd86c9c12dfc691864c89068ccb69e66f0 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 3 Oct 2024 20:50:08 +0530 Subject: [PATCH 01/16] feat: add & remove collections to component --- .../core/djangoapps/content_libraries/api.py | 52 +++++++++++++++++++ .../content_libraries/serializers.py | 8 +++ .../content_libraries/signal_handlers.py | 9 ++-- .../core/djangoapps/content_libraries/urls.py | 2 + .../djangoapps/content_libraries/views.py | 43 +++++++++++++++ 5 files changed, 110 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index b9f3779af539..61afdbe09cd2 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1235,6 +1235,58 @@ def update_library_collection_components( return collection +def update_library_component_collections( + library_key: LibraryLocatorV2, + component: Component, + *, + collection_keys: list[str], + 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: + """ + This api has opposite then functionality from `update_library_collection_components`. + It Associates the component with collections for the given collection keys. + + By default the Collections are added to the Component. + If remove=True, the Collections are removed from the Component. + + If you've already fetched the ContentLibrary, pass it in to avoid refetching. + + Raises: + * ContentLibraryCollectionNotFound if any of the given collection_keys don't match Collections in the given library. + + Returns the updated Component. + """ + 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 + + # Note: Component.key matches its PublishableEntity.key + collection_qs = authoring_api.get_collections(content_library.learning_package_id).filter( + key__in=collection_keys + ) + + if remove: + component = authoring_api.remove_collections( + content_library.learning_package_id, + component, + collection_qs, + ) + else: + component = authoring_api.add_collections( + content_library.learning_package_id, + component, + collection_qs, + created_by=created_by, + ) + + return component + + def get_library_collection_usage_key( library_key: LibraryLocatorV2, collection_key: str, diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 51ba55cd6b48..f9563ff3603c 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -305,3 +305,11 @@ class ContentLibraryCollectionComponentsUpdateSerializer(serializers.Serializer) """ usage_keys = serializers.ListField(child=UsageKeyV2Serializer(), allow_empty=False) + + +class ContentLibraryComponentCollectionsUpdateSerializer(serializers.Serializer): + """ + Serializer for adding/removing Collections to/from a Component. + """ + + collection_keys = serializers.ListField(child=serializers.CharField(), allow_empty=False) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index fedee045a9f6..18e102bf686d 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -21,7 +21,7 @@ LIBRARY_COLLECTION_UPDATED, ) from openedx_learning.api.authoring import get_collection_components, get_component, get_components -from openedx_learning.api.authoring_models import Collection, CollectionPublishableEntity, Component +from openedx_learning.api.authoring_models import Collection, CollectionPublishableEntity, Component, PublishableEntity from lms.djangoapps.grades.api import signals as grades_signals @@ -177,9 +177,6 @@ def library_collection_entities_changed(sender, instance, action, pk_set, **kwar """ Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components added/removed/cleared from a collection. """ - if not isinstance(instance, Collection): - return - if action not in ["post_add", "post_remove", "post_clear"]: return @@ -191,6 +188,10 @@ def library_collection_entities_changed(sender, instance, action, pk_set, **kwar log.error("{instance} is not associated with a content library.") return + if isinstance(instance, PublishableEntity): + _library_collection_component_changed(instance.component, library.library_key) + return + if pk_set: components = get_collection_components( instance.learning_package_id, diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 9455f0de5e61..e77c1b34d277 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -57,6 +57,8 @@ path('blocks//', include([ # Get metadata about a specific XBlock in this library, or delete the block: path('', views.LibraryBlockView.as_view()), + # Update collections for a given component + path('collections/', views.LibraryBlockCollectionsView.as_view(), name='update-collections'), # Get the LTI URL of a specific XBlock path('lti/', views.LibraryBlockLtiUrlView.as_view(), name='lti-url'), # Get the OLX source code of the specified block: diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 3712af6e597f..abf00d4ae93b 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -90,6 +90,7 @@ from organizations.exceptions import InvalidOrganizationException from organizations.models import Organization from rest_framework import status +from rest_framework.decorators import action from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError from rest_framework.generics import GenericAPIView from rest_framework.parsers import MultiPartParser @@ -106,6 +107,7 @@ ContentLibraryPermissionLevelSerializer, ContentLibraryPermissionSerializer, ContentLibraryUpdateSerializer, + ContentLibraryComponentCollectionsUpdateSerializer, LibraryXBlockCreationSerializer, LibraryXBlockMetadataSerializer, LibraryXBlockTypeSerializer, @@ -640,6 +642,47 @@ def delete(self, request, usage_key_str): # pylint: disable=unused-argument return Response({}) +@method_decorator(non_atomic_requests, name="dispatch") +@view_auth_classes() +class LibraryBlockCollectionsView(APIView): + def _handle_request(self, request, usage_key_str) -> Response: + key = LibraryUsageLocatorV2.from_string(usage_key_str) + content_library = api.require_permission_for_library_key(key.lib_key, request.user, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + component = api.get_component_from_usage_key(key) + serializer = ContentLibraryComponentCollectionsUpdateSerializer(data=request.data) + serializer.is_valid(raise_exception=True) + + collection_keys = serializer.validated_data['collection_keys'] + api.update_library_component_collections( + library_key=key.lib_key, + component=component, + collection_keys=collection_keys, + created_by=self.request.user.id, + remove=(request.method == "DELETE"), + content_library=content_library, + ) + + return Response({'count': len(collection_keys)}) + + @convert_exceptions + def patch(self, request, usage_key_str) -> Response: + """ + Adds Collections to a Component. + + Collection and Components must all be part of the given library/learning package. + """ + return self._handle_request(request, usage_key_str) + + @convert_exceptions + def delete(self, request, usage_key_str) -> Response: + """ + Removes Collections from a Component. + + Collection and Components must all be part of the given library/learning package. + """ + return self._handle_request(request, usage_key_str) + @method_decorator(non_atomic_requests, name="dispatch") @view_auth_classes() class LibraryBlockLtiUrlView(APIView): From e5d3b69b65b5ae6db24d9baf93202005b8b98563 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 4 Oct 2024 11:43:52 +0530 Subject: [PATCH 02/16] refactor: set collections in a component --- .../core/djangoapps/content_libraries/api.py | 25 ++++++------------ .../djangoapps/content_libraries/views.py | 26 +++++-------------- 2 files changed, 15 insertions(+), 36 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 61afdbe09cd2..8992f0fc7844 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1241,16 +1241,14 @@ def update_library_component_collections( *, collection_keys: list[str], 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: """ - This api has opposite then functionality from `update_library_collection_components`. It Associates the component with collections for the given collection keys. - By default the Collections are added to the Component. - If remove=True, the Collections are removed from the Component. + Only collections in queryset are associated with component, all previous component-collections + associations are removed. If you've already fetched the ContentLibrary, pass it in to avoid refetching. @@ -1270,19 +1268,12 @@ def update_library_component_collections( key__in=collection_keys ) - if remove: - component = authoring_api.remove_collections( - content_library.learning_package_id, - component, - collection_qs, - ) - else: - component = authoring_api.add_collections( - content_library.learning_package_id, - component, - collection_qs, - created_by=created_by, - ) + component = authoring_api.set_collections( + content_library.learning_package_id, + component, + collection_qs, + created_by=created_by, + ) return component diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index abf00d4ae93b..8ebd8fb2507a 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -645,7 +645,13 @@ def delete(self, request, usage_key_str): # pylint: disable=unused-argument @method_decorator(non_atomic_requests, name="dispatch") @view_auth_classes() class LibraryBlockCollectionsView(APIView): - def _handle_request(self, request, usage_key_str) -> Response: + @convert_exceptions + def patch(self, request, usage_key_str) -> Response: + """ + Adds Collections to a Component. + + Collection and Components must all be part of the given library/learning package. + """ key = LibraryUsageLocatorV2.from_string(usage_key_str) content_library = api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) @@ -659,29 +665,11 @@ def _handle_request(self, request, usage_key_str) -> Response: component=component, collection_keys=collection_keys, created_by=self.request.user.id, - remove=(request.method == "DELETE"), content_library=content_library, ) return Response({'count': len(collection_keys)}) - @convert_exceptions - def patch(self, request, usage_key_str) -> Response: - """ - Adds Collections to a Component. - - Collection and Components must all be part of the given library/learning package. - """ - return self._handle_request(request, usage_key_str) - - @convert_exceptions - def delete(self, request, usage_key_str) -> Response: - """ - Removes Collections from a Component. - - Collection and Components must all be part of the given library/learning package. - """ - return self._handle_request(request, usage_key_str) @method_decorator(non_atomic_requests, name="dispatch") @view_auth_classes() From 40a63e413f3497baae4cdcd4290e4f7d39cee8ed Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 4 Oct 2024 20:11:22 +0530 Subject: [PATCH 03/16] refactor: async collections indexing --- openedx/core/djangoapps/content_libraries/api.py | 6 +++++- openedx/core/djangoapps/content_libraries/serializers.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 8992f0fc7844..3ff26003f80d 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1268,13 +1268,17 @@ def update_library_component_collections( key__in=collection_keys ) - component = authoring_api.set_collections( + affected_collections = authoring_api.set_collections( content_library.learning_package_id, component, collection_qs, created_by=created_by, ) + from ..content.search.tasks import update_library_collection_index_doc + for collection in affected_collections: + update_library_collection_index_doc.delay(str(library_key), collection.key) + return component diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index f9563ff3603c..6765656e69d5 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -312,4 +312,4 @@ class ContentLibraryComponentCollectionsUpdateSerializer(serializers.Serializer) Serializer for adding/removing Collections to/from a Component. """ - collection_keys = serializers.ListField(child=serializers.CharField(), allow_empty=False) + collection_keys = serializers.ListField(child=serializers.CharField(), allow_empty=True) From d18d5ba5b196c5cac84a02814066e1106ad00a61 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 4 Oct 2024 20:13:06 +0530 Subject: [PATCH 04/16] fix: lint issues --- openedx/core/djangoapps/content_libraries/views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 8ebd8fb2507a..f909b682c038 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -90,7 +90,6 @@ from organizations.exceptions import InvalidOrganizationException from organizations.models import Organization from rest_framework import status -from rest_framework.decorators import action from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError from rest_framework.generics import GenericAPIView from rest_framework.parsers import MultiPartParser @@ -645,6 +644,9 @@ def delete(self, request, usage_key_str): # pylint: disable=unused-argument @method_decorator(non_atomic_requests, name="dispatch") @view_auth_classes() class LibraryBlockCollectionsView(APIView): + """ + View to set collections for a component. + """ @convert_exceptions def patch(self, request, usage_key_str) -> Response: """ From ac59d53567c4dafe71af589363662f643e04a697 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 7 Oct 2024 11:32:01 +0530 Subject: [PATCH 05/16] test: test set collections for component --- .../core/djangoapps/content_libraries/api.py | 2 +- .../content_libraries/tests/test_api.py | 46 +++++++++++++++++++ .../djangoapps/content_libraries/views.py | 2 +- 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 3ff26003f80d..8494e0cad336 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1235,7 +1235,7 @@ def update_library_collection_components( return collection -def update_library_component_collections( +def set_library_component_collections( library_key: LibraryLocatorV2, component: Component, *, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 8041c508dc31..9ab60a2c9d26 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -308,6 +308,13 @@ def setUp(self): description="Description for Collection 2", created_by=self.user.id, ) + self.col3 = api.create_library_collection( + self.lib2.library_key, + collection_key="COL3", + title="Collection 3", + description="Description for Collection 3", + created_by=self.user.id, + ) # Create some library blocks in lib1 self.lib1_problem_block = self._add_block_to_library( @@ -316,6 +323,10 @@ def setUp(self): self.lib1_html_block = self._add_block_to_library( self.lib1.library_key, "html", "html1", ) + # Create some library blocks in lib2 + self.lib2_problem_block = self._add_block_to_library( + self.lib2.library_key, "problem", "problem2", + ) def test_create_library_collection(self): event_receiver = mock.Mock() @@ -498,3 +509,38 @@ def test_update_collection_components_from_wrong_library(self): ], ) assert self.lib1_problem_block["id"] in str(exc.exception) + + @mock.patch('openedx.core.djangoapps.content.search.api.upsert_library_collection_index_doc') + def test_set_library_component_collections(self, mock_update_collection_index_doc): + event_receiver = mock.Mock() + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) + assert not list(self.col2.entities.all()) + component = api.get_component_from_usage_key(UsageKey.from_string(self.lib2_problem_block["id"])) + + api.set_library_component_collections( + self.lib2.library_key, + component, + collection_keys=[self.col2.key, self.col3.key], + ) + + assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col2.key).entities.all()) == 1 + assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col3.key).entities.all()) == 1 + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "sender": None, + "content_object": ContentObjectChangedData( + object_id=self.lib2_problem_block["id"], + changes=["collections"], + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + self.assertListEqual( + list(mock_update_collection_index_doc.call_args_list[0][0]), + [self.lib2.library_key, self.col2.key] + ) + self.assertListEqual( + list(mock_update_collection_index_doc.call_args_list[1][0]), + [self.lib2.library_key, self.col3.key] + ) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index f909b682c038..a3aa2407f331 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -662,7 +662,7 @@ def patch(self, request, usage_key_str) -> Response: serializer.is_valid(raise_exception=True) collection_keys = serializer.validated_data['collection_keys'] - api.update_library_component_collections( + api.set_library_component_collections( library_key=key.lib_key, component=component, collection_keys=collection_keys, From fa8ad9e52b4288b8e24076fea67293597a40c76a Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 7 Oct 2024 11:47:00 +0530 Subject: [PATCH 06/16] chore: fix docs --- openedx/core/djangoapps/content_libraries/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index a3aa2407f331..4d0dfe17fda1 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -650,7 +650,7 @@ class LibraryBlockCollectionsView(APIView): @convert_exceptions def patch(self, request, usage_key_str) -> Response: """ - Adds Collections to a Component. + Sets Collections for a Component. Collection and Components must all be part of the given library/learning package. """ From 0b7a7821bac5811b900c9243f05ccb5732d48ab0 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 7 Oct 2024 11:58:21 +0530 Subject: [PATCH 07/16] chore: temporarily update openedx-learning package --- 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 dd727a4b1801..6535ebc5ab2a 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -140,7 +140,7 @@ optimizely-sdk<5.0 # Date: 2023-09-18 # pinning this version to avoid updates while the library is being developed # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35269 -openedx-learning==0.13.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api # Date: 2023-11-29 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 55793bd0c5af..a63d720263fa 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -825,7 +825,7 @@ openedx-filters==1.10.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-learning==0.13.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 1bdd3736516b..de28130d84c6 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1374,7 +1374,7 @@ openedx-filters==1.10.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-learning==0.13.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 6f12d8046759..a4e2a83074aa 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -984,7 +984,7 @@ openedx-filters==1.10.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.13.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index d3b14a6f4373..feedb4279a53 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1035,7 +1035,7 @@ openedx-filters==1.10.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.13.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@navin/component-collection-api # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From e007398237f1db27517dc1a1686bd928645760d6 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 7 Oct 2024 12:02:47 +0530 Subject: [PATCH 08/16] fix: lint issues --- openedx/core/djangoapps/content_libraries/api.py | 2 +- openedx/core/djangoapps/content_libraries/views.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 8494e0cad336..11b720bbb970 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1243,7 +1243,7 @@ def set_library_component_collections( created_by: int | None = None, # As an optimization, callers may pass in a pre-fetched ContentLibrary instance content_library: ContentLibrary | None = None, -) -> Collection: +) -> Component: """ It Associates the component with collections for the given collection keys. diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 4d0dfe17fda1..c9051d155208 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -655,8 +655,11 @@ def patch(self, request, usage_key_str) -> Response: Collection and Components must all be part of the given library/learning package. """ key = LibraryUsageLocatorV2.from_string(usage_key_str) - content_library = api.require_permission_for_library_key(key.lib_key, request.user, - permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + content_library = api.require_permission_for_library_key( + key.lib_key, + request.user, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + ) component = api.get_component_from_usage_key(key) serializer = ContentLibraryComponentCollectionsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) From 0a438d0adfb7d9071cd947b3f5b1f56e2a9bdf21 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 8 Oct 2024 20:31:32 +0530 Subject: [PATCH 09/16] chore: add usage_key to filterable attribute --- openedx/core/djangoapps/content/search/api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index b5ed1bde78e1..17338f20ab83 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -320,6 +320,7 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.block_id, Fields.block_type, Fields.context_key, + Fields.usage_key, Fields.org, Fields.tags, Fields.tags + "." + Fields.tags_taxonomy, From 4c8ab8aeb6d06db07692dfd70c1b5b69f0ec1236 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 9 Oct 2024 16:18:48 +0530 Subject: [PATCH 10/16] feat: add collections to component api --- .../core/djangoapps/content_libraries/api.py | 22 +++++++++++++++++-- .../content_libraries/serializers.py | 10 +++++++++ .../djangoapps/content_libraries/views.py | 2 +- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 11b720bbb970..e5f43375866e 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -204,6 +204,15 @@ class ContentLibraryPermissionEntry: access_level = attr.ib(AccessLevel.NO_ACCESS) +@attr.s +class CollectionMetadata: + """ + Class to represent collection metadata in a content library. + """ + key = attr.ib(type=str) + title = attr.ib(type=str) + + @attr.s class LibraryXBlockMetadata: """ @@ -219,9 +228,10 @@ class LibraryXBlockMetadata: published_by = attr.ib("") has_unpublished_changes = attr.ib(False) created = attr.ib(default=None, type=datetime) + collections = attr.ib(type=list[CollectionMetadata], factory=list) @classmethod - def from_component(cls, library_key, component): + def from_component(cls, library_key, component, collections=None): """ Construct a LibraryXBlockMetadata from a Component object. """ @@ -248,6 +258,7 @@ def from_component(cls, library_key, component): last_draft_created=last_draft_created, last_draft_created_by=last_draft_created_by, has_unpublished_changes=component.versioning.has_unpublished_changes, + collections=collections or [], ) @@ -690,7 +701,7 @@ def get_library_components(library_key, text_search=None, block_types=None) -> Q return components -def get_library_block(usage_key) -> LibraryXBlockMetadata: +def get_library_block(usage_key, include_collections=False) -> LibraryXBlockMetadata: """ Get metadata about (the draft version of) one specific XBlock in a library. @@ -713,9 +724,16 @@ def get_library_block(usage_key) -> LibraryXBlockMetadata: if not draft_version: raise ContentLibraryBlockNotFound(usage_key) + collections = [] + if include_collections: + collections = authoring_api.get_entity_collections( + component.learning_package_id, + component.key, + ).values('key', 'title') xblock_metadata = LibraryXBlockMetadata.from_component( library_key=usage_key.context_key, component=component, + collections=collections, ) return xblock_metadata diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 6765656e69d5..8e9e5fc2a749 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -134,6 +134,14 @@ class ContentLibraryFilterSerializer(BaseFilterSerializer): type = serializers.ChoiceField(choices=LIBRARY_TYPES, default=None, required=False) +class CollectionMetadataSerializer(serializers.Serializer): + """ + Serializer for CollectionMetadata + """ + key = serializers.CharField() + title = serializers.CharField() + + class LibraryXBlockMetadataSerializer(serializers.Serializer): """ Serializer for LibraryXBlockMetadata @@ -161,6 +169,8 @@ class LibraryXBlockMetadataSerializer(serializers.Serializer): slug = serializers.CharField(write_only=True) tags_count = serializers.IntegerField(read_only=True) + collections = CollectionMetadataSerializer(many=True, required=False) + class LibraryXBlockTypeSerializer(serializers.Serializer): """ diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index c9051d155208..6e50559f38f6 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -618,7 +618,7 @@ def get(self, request, usage_key_str): """ key = LibraryUsageLocatorV2.from_string(usage_key_str) api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) - result = api.get_library_block(key) + result = api.get_library_block(key, include_collections=True) return Response(LibraryXBlockMetadataSerializer(result).data) From 86b8cdd68d0980680b753adf1b76e78e20c7d8a3 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Wed, 9 Oct 2024 16:29:29 +0530 Subject: [PATCH 11/16] fix: lint issues --- openedx/core/djangoapps/content_libraries/api.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index e5f43375866e..07d07222234f 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -231,7 +231,7 @@ class LibraryXBlockMetadata: collections = attr.ib(type=list[CollectionMetadata], factory=list) @classmethod - def from_component(cls, library_key, component, collections=None): + def from_component(cls, library_key, component, associated_collections=None): """ Construct a LibraryXBlockMetadata from a Component object. """ @@ -258,7 +258,7 @@ def from_component(cls, library_key, component, collections=None): last_draft_created=last_draft_created, last_draft_created_by=last_draft_created_by, has_unpublished_changes=component.versioning.has_unpublished_changes, - collections=collections or [], + collections=associated_collections or [], ) @@ -724,16 +724,17 @@ def get_library_block(usage_key, include_collections=False) -> LibraryXBlockMeta if not draft_version: raise ContentLibraryBlockNotFound(usage_key) - collections = [] if include_collections: - collections = authoring_api.get_entity_collections( + associated_collections = authoring_api.get_entity_collections( component.learning_package_id, component.key, ).values('key', 'title') + else: + associated_collections = None xblock_metadata = LibraryXBlockMetadata.from_component( library_key=usage_key.context_key, component=component, - collections=collections, + associated_collections=associated_collections, ) return xblock_metadata From cd9eec97e1d28929e8e9ceb6e8be6de83ab9dc60 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 10 Oct 2024 11:50:26 +0530 Subject: [PATCH 12/16] fix: remove collection-entity through model post_delete signal handler The indexing update is already handled by m2m_changed handler. --- .../djangoapps/content_libraries/signal_handlers.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 18e102bf686d..1d723bea8b23 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -162,16 +162,6 @@ def library_collection_entity_saved(sender, instance, created, **kwargs): _library_collection_component_changed(component) -@receiver(post_delete, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entity_deleted") -def library_collection_entity_deleted(sender, instance, **kwargs): - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components removed from a collection. - """ - # Component.pk matches its entity.pk - component = get_component(instance.entity_id) - _library_collection_component_changed(component) - - @receiver(m2m_changed, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entities_changed") def library_collection_entities_changed(sender, instance, action, pk_set, **kwargs): """ From 97ad3ba9db006880f4b309e772abf0dae9e28e45 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 10 Oct 2024 12:53:27 +0530 Subject: [PATCH 13/16] fix: use lazy field to control collection update task --- .../djangoapps/content/search/handlers.py | 20 +++++++---- .../core/djangoapps/content_libraries/api.py | 13 ++++++-- .../content_libraries/tests/test_api.py | 33 ++++++++++++++----- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 085387d336b1..46b015367a0d 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -179,13 +179,19 @@ def library_collection_updated_handler(**kwargs) -> None: log.error("Received null or incorrect data for event") return - # 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, - ]) + if library_collection.lazy: + update_library_collection_index_doc.delay( + str(library_collection.library_key), + library_collection.collection_key, + ) + else: + # 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) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 07d07222234f..1ba00ac7122c 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -80,6 +80,7 @@ from openedx_events.content_authoring.data import ( ContentLibraryData, LibraryBlockData, + LibraryCollectionData, ) from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_CREATED, @@ -88,6 +89,7 @@ LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, + LIBRARY_COLLECTION_UPDATED, ) from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Collection, Component, MediaType, LearningPackage, PublishableEntity @@ -1294,9 +1296,16 @@ def set_library_component_collections( created_by=created_by, ) - from ..content.search.tasks import update_library_collection_index_doc + # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set lazy=True to trigger + # collection indexing asynchronously. for collection in affected_collections: - update_library_collection_index_doc.delay(str(library_key), collection.key) + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_key=collection.key, + lazy=True, + ) + ) return component diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 9ab60a2c9d26..4e49bff8c9f1 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -510,10 +510,11 @@ def test_update_collection_components_from_wrong_library(self): ) assert self.lib1_problem_block["id"] in str(exc.exception) - @mock.patch('openedx.core.djangoapps.content.search.api.upsert_library_collection_index_doc') - def test_set_library_component_collections(self, mock_update_collection_index_doc): + def test_set_library_component_collections(self): event_receiver = mock.Mock() CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) + collection_update_event_receiver = mock.Mock() + LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) assert not list(self.col2.entities.all()) component = api.get_component_from_usage_key(UsageKey.from_string(self.lib2_problem_block["id"])) @@ -536,11 +537,27 @@ def test_set_library_component_collections(self, mock_update_collection_index_do }, event_receiver.call_args_list[0].kwargs, ) - self.assertListEqual( - list(mock_update_collection_index_doc.call_args_list[0][0]), - [self.lib2.library_key, self.col2.key] + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib2.library_key, + collection_key=self.col2.key, + lazy=True, + ), + }, + collection_update_event_receiver.call_args_list[0].kwargs, ) - self.assertListEqual( - list(mock_update_collection_index_doc.call_args_list[1][0]), - [self.lib2.library_key, self.col3.key] + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib2.library_key, + collection_key=self.col3.key, + lazy=True, + ), + }, + collection_update_event_receiver.call_args_list[1].kwargs, ) From baa10fc39a5a6673683985ad947a9882dd46a38b Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 10 Oct 2024 12:53:44 +0530 Subject: [PATCH 14/16] chore: temporarily point openedx-events to dev branch --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index a63d720263fa..5eea20268e5c 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.14.1 +openedx-events @ git+https://github.com/open-craft/openedx-events@navin/update-library-collection-data # via # -r requirements/edx/kernel.in # edx-enterprise diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index de28130d84c6..a40c6d87dbd4 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.14.1 +openedx-events @ git+https://github.com/open-craft/openedx-events@navin/update-library-collection-data # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index a4e2a83074aa..e4db7f5e4314 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.14.1 +openedx-events @ git+https://github.com/open-craft/openedx-events@navin/update-library-collection-data # via # -r requirements/edx/base.txt # edx-enterprise diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index feedb4279a53..4f88fade88b5 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.14.1 +openedx-events @ git+https://github.com/open-craft/openedx-events@navin/update-library-collection-data # via # -r requirements/edx/base.txt # edx-enterprise From e553ec0fdcaca73a7dd5f60cb4ce2350a217b41e Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 10 Oct 2024 16:15:03 +0530 Subject: [PATCH 15/16] fix: use get_components in m2m_changed signal handler Using `get_collection_components` does not work in case of post_remove as the component is already removed from collection and we don't really need additional filtering as we already have pk_set from the signal. --- .../content_libraries/signal_handlers.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 1d723bea8b23..49d40b27d689 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -20,7 +20,7 @@ LIBRARY_COLLECTION_DELETED, LIBRARY_COLLECTION_UPDATED, ) -from openedx_learning.api.authoring import get_collection_components, get_component, get_components +from openedx_learning.api.authoring import get_component, get_components from openedx_learning.api.authoring_models import Collection, CollectionPublishableEntity, Component, PublishableEntity from lms.djangoapps.grades.api import signals as grades_signals @@ -182,18 +182,12 @@ def library_collection_entities_changed(sender, instance, action, pk_set, **kwar _library_collection_component_changed(instance.component, library.library_key) return + # When action=="post_clear", pk_set==None + # Since the collection instance now has an empty entities set, + # we don't know which ones were removed, so we need to update associations for all library components. + components = get_components(instance.learning_package_id) if pk_set: - components = get_collection_components( - instance.learning_package_id, - instance.key, - ).filter(pk__in=pk_set) - else: - # When action=="post_clear", pk_set==None - # Since the collection instance now has an empty entities set, - # we don't know which ones were removed, so we need to update associations for all library components. - components = get_components( - instance.learning_package_id, - ) + components = components.filter(pk__in=pk_set) for component in components.all(): _library_collection_component_changed(component, library.library_key) From 758334f3083c5ec3b26479f4765f3b49328bd4c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 10 Oct 2024 11:25:16 -0300 Subject: [PATCH 16/16] fix: circuvent meilisearch bug --- .../core/djangoapps/content/search/documents.py | 15 ++++++++------- .../djangoapps/content/search/tests/test_api.py | 8 ++++---- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index eabeab9654ca..f520cd14e40f 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -267,6 +267,13 @@ def _collections_for_content_object(object_id: UsageKey | LearningContextKey) -> } """ + result = { + Fields.collections: { + Fields.collections_display_name: [], + Fields.collections_key: [], + } + } + # Gather the collections associated with this object collections = None try: @@ -279,14 +286,8 @@ def _collections_for_content_object(object_id: UsageKey | LearningContextKey) -> log.warning(f"No component found for {object_id}") if not collections: - return {Fields.collections: {}} + return result - 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) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 4c6227af309f..c89d84490e96 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -219,10 +219,10 @@ def test_reindex_meilisearch(self, mock_meilisearch): doc_vertical["tags"] = {} doc_problem1 = copy.deepcopy(self.doc_problem1) doc_problem1["tags"] = {} - doc_problem1["collections"] = {} + doc_problem1["collections"] = {'display_name': [], 'key': []} doc_problem2 = copy.deepcopy(self.doc_problem2) doc_problem2["tags"] = {} - doc_problem2["collections"] = {} + doc_problem2["collections"] = {'display_name': [], 'key': []} doc_collection = copy.deepcopy(self.collection_dict) doc_collection["tags"] = {} @@ -263,7 +263,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"] = {} + doc_problem2["collections"] = {'display_name': [], 'key': []} orig_from_component = library_api.LibraryXBlockMetadata.from_component @@ -662,7 +662,7 @@ def test_delete_collection(self, mock_meilisearch): doc_problem_without_collection = { "id": self.doc_problem1["id"], - "collections": {}, + "collections": {'display_name': [], 'key': []}, } # Should delete the collection document