diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 2f4fd1831992..d918fcafe6ae 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -29,6 +29,7 @@ from xmodule.modulestore.django import modulestore from .documents import ( + DocType, Fields, meili_id_from_opaque_key, searchable_doc_for_course_block, @@ -256,6 +257,28 @@ def _update_index_docs(docs) -> None: _wait_for_meili_tasks(tasks) +def _delete_index_docs(docs) -> None: + """ + Helper function that deletes the given documents from the search index + + If there is a rebuild in progress, the document will also be removed from the new index. + """ + if not docs: + return + + client = _get_meilisearch_client() + current_rebuild_index_name = _get_running_rebuild_index_name() + + tasks = [] + if current_rebuild_index_name: + # If there is a rebuild in progress, the document will also be removed from the new index. + tasks.append(client.index(current_rebuild_index_name).delete_documents(docs)) + + tasks.append(client.index(STUDIO_INDEX_NAME).delete_documents(docs)) + + _wait_for_meili_tasks(tasks) + + def only_if_meilisearch_enabled(f): """ Only call `f` if meilisearch is enabled @@ -563,13 +586,19 @@ def upsert_library_block_index_doc(usage_key: UsageKey) -> None: 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 + Creates, updates, or deletes the document for the given Library Collection in the search index. + + If the Collection is not found or disabled (i.e. soft-deleted), then delete it from the search index. """ - docs = [ - searchable_doc_for_collection(library_key, collection_key) - ] + doc = searchable_doc_for_collection(library_key, collection_key) - _update_index_docs(docs) + # If the doc is a "collection", then it's valid for insert/update. + if doc.get(Fields.type, "") == DocType.collection: + _update_index_docs([doc]) + + # Otherwise, the collection wasn't found or it's been disabled/soft-deleted, so we delete it from the index. + else: + _delete_index_docs([doc]) def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 2d9ddb5b84b5..a235b9054b10 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -407,7 +407,9 @@ def searchable_doc_for_collection( # Collection not found, so we can only return the base doc pass - if collection: + # Disabled collections should be removed from the search index, + # so we intentionally don't populate the rest of the info here. + if collection and collection.enabled: assert collection.key == collection_key doc.update({ diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index f50dead8474a..085387d336b1 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -23,6 +23,7 @@ LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_DELETED, LIBRARY_COLLECTION_UPDATED, XBLOCK_CREATED, XBLOCK_DELETED, @@ -166,6 +167,7 @@ def content_library_updated_handler(**kwargs) -> None: @receiver(LIBRARY_COLLECTION_CREATED) +@receiver(LIBRARY_COLLECTION_DELETED) @receiver(LIBRARY_COLLECTION_UPDATED) @only_if_meilisearch_enabled def library_collection_updated_handler(**kwargs) -> None: diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 0ac75f4f45b6..d7d5bc8a8628 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -615,3 +615,96 @@ def test_index_tags_in_collections(self, mock_meilisearch): ], any_order=True, ) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_delete_collection(self, mock_meilisearch): + """ + Test soft-deleting, restoring, and hard-deleting a collection. + """ + # Add a component to the collection + updated_date = datetime(2023, 6, 7, 8, 9, 10, tzinfo=timezone.utc) + with freeze_time(updated_date): + library_api.update_library_collection_components( + self.library.key, + collection_key=self.collection.key, + usage_keys=[ + self.problem1.usage_key, + ], + ) + + doc_collection = copy.deepcopy(self.collection_dict) + doc_collection["num_children"] = 1 + doc_collection["modified"] = updated_date.timestamp() + doc_problem_with_collection = { + "id": self.doc_problem1["id"], + "collections": { + "display_name": [self.collection.title], + "key": [self.collection.key], + }, + } + 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_collection]), + call([doc_problem_with_collection]), + ], + any_order=True, + ) + mock_meilisearch.return_value.index.reset_mock() + + # Soft-delete the collection + authoring_api.delete_collection( + self.collection.learning_package_id, + self.collection.key, + ) + + doc_collection = { + "id": self.collection_dict["id"], + } + doc_problem_without_collection = { + "id": self.doc_problem1["id"], + "collections": {}, + } + + mock_meilisearch.return_value.index.return_value.delete_documents.assert_called_once_with([doc_collection]) + ## TODO: update the components in a soft-deleted collection + # mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([ + # doc_problem_without_collection + # ]) + mock_meilisearch.return_value.index.reset_mock() + + # Restore the collection + restored_date = datetime(2023, 8, 9, 10, 11, 12, tzinfo=timezone.utc) + with freeze_time(restored_date): + authoring_api.restore_collection( + self.collection.learning_package_id, + self.collection.key, + ) + + doc_collection = copy.deepcopy(self.collection_dict) + doc_collection["num_children"] = 1 + doc_collection["modified"] = restored_date.timestamp() + + mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([doc_collection]) + ## TODO: update the components in a restored collection + # mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([ + # doc_problem_with_collection + # ]) + mock_meilisearch.return_value.index.reset_mock() + + # Hard-delete the collection + authoring_api.delete_collection( + self.collection.learning_package_id, + self.collection.key, + hard_delete=True, + ) + + doc_collection = { + "id": self.collection_dict["id"], + } + + mock_meilisearch.return_value.index.return_value.delete_documents.assert_called_once_with([doc_collection]) + ## TODO: update the components in a deleted collection? + # mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([ + # doc_problem_without_collection + # ]) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index d831523a394b..1efdcb410902 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -5,7 +5,7 @@ import logging from django.conf import settings -from django.db.models.signals import post_save +from django.db.models.signals import post_save, post_delete from django.dispatch import receiver from opaque_keys import InvalidKeyError # lint-amnesty, pylint: disable=wrong-import-order @@ -15,6 +15,7 @@ ) from openedx_events.content_authoring.signals import ( LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_DELETED, LIBRARY_COLLECTION_UPDATED, ) from openedx_learning.api.authoring_models import Collection @@ -93,3 +94,22 @@ def library_collection_saved(sender, instance, created, **kwargs): collection_key=instance.key, ) ) + + +@receiver(post_delete, sender=Collection, dispatch_uid="library_collection_deleted") +def library_collection_deleted(sender, instance, **kwargs): + """ + Raises LIBRARY_COLLECTION_DELETED for the deleted Collection. + """ + try: + library = ContentLibrary.objects.get(learning_package_id=instance.learning_package_id) + except ContentLibrary.DoesNotExist: + log.error("{instance} is not associated with a content library.") + return + + LIBRARY_COLLECTION_DELETED.send_event( + library_collection=LibraryCollectionData( + library_key=library.library_key, + collection_key=instance.key, + ) + ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index b02e71b002a3..84805451f6bc 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -20,9 +20,11 @@ from openedx_events.content_authoring.signals import ( CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_DELETED, 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 @@ -264,6 +266,7 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTe ENABLED_OPENEDX_EVENTS = [ CONTENT_OBJECT_ASSOCIATIONS_CHANGED.event_type, LIBRARY_COLLECTION_CREATED.event_type, + LIBRARY_COLLECTION_DELETED.event_type, LIBRARY_COLLECTION_UPDATED.event_type, ] @@ -386,6 +389,29 @@ def test_update_library_collection_wrong_library(self): self.col2.key, ) + def test_delete_library_collection(self): + event_receiver = mock.Mock() + LIBRARY_COLLECTION_DELETED.connect(event_receiver) + + authoring_api.delete_collection( + self.lib1.learning_package_id, + self.col1.key, + hard_delete=True, + ) + + assert event_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_DELETED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib1.library_key, + collection_key="COL1", + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + def test_update_library_collection_components(self): assert not list(self.col1.entities.all())