From d5850c812cd7e46acc701ace791e23f69c9fc74c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Tue, 19 Nov 2024 15:45:34 -0500 Subject: [PATCH] fix: keep library collection card component count in sync (#35734) Fixed component counter synchronization in these cases: * When deleting a component inside a collection. * With the library published, when adding a new component in a collection and reverting library changes. * With the library published, when deleting a component inside a collection and reverting library changes. Also adds a published > num_counts field in collections in the search index. --- .../djangoapps/content/search/documents.py | 15 +- .../content/search/tests/test_api.py | 15 ++ .../content/search/tests/test_documents.py | 32 ++++ .../core/djangoapps/content_libraries/api.py | 60 ++++++- .../content_libraries/tests/test_api.py | 152 ++++++++++++++++++ requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 10 files changed, 276 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 7e547ca4d889..0dd02683ceea 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -80,6 +80,7 @@ class Fields: published = "published" published_display_name = "display_name" published_description = "description" + published_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' @@ -488,6 +489,15 @@ def searchable_doc_for_collection( if collection: assert collection.key == collection_key + draft_num_children = authoring_api.filter_publishable_entities( + collection.entities, + has_draft=True, + ).count() + published_num_children = authoring_api.filter_publishable_entities( + collection.entities, + has_published=True, + ).count() + doc.update({ Fields.context_key: str(library_key), Fields.org: str(library_key.org), @@ -498,7 +508,10 @@ def searchable_doc_for_collection( Fields.description: collection.description, Fields.created: collection.created.timestamp(), Fields.modified: collection.modified.timestamp(), - Fields.num_children: collection.entities.count(), + Fields.num_children: draft_num_children, + Fields.published: { + Fields.published_num_children: published_num_children, + }, Fields.access_id: _meili_access_id_from_context_key(library_key), Fields.breadcrumbs: [{"display_name": collection.learning_package.title}], }) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 990226f343cf..0aa762fd187f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -198,6 +198,9 @@ def setUp(self): "created": created_date.timestamp(), "modified": created_date.timestamp(), "access_id": lib_access.id, + "published": { + "num_children": 0 + }, "breadcrumbs": [{"display_name": "Library"}], } @@ -472,6 +475,9 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "created": created_date.timestamp(), "modified": created_date.timestamp(), "access_id": lib_access.id, + "published": { + "num_children": 0 + }, "breadcrumbs": [{"display_name": "Library"}], } doc_collection2_created = { @@ -487,6 +493,9 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "created": created_date.timestamp(), "modified": created_date.timestamp(), "access_id": lib_access.id, + "published": { + "num_children": 0 + }, "breadcrumbs": [{"display_name": "Library"}], } doc_collection2_updated = { @@ -502,6 +511,9 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "created": created_date.timestamp(), "modified": updated_date.timestamp(), "access_id": lib_access.id, + "published": { + "num_children": 0 + }, "breadcrumbs": [{"display_name": "Library"}], } doc_collection1_updated = { @@ -517,6 +529,9 @@ def test_index_library_block_and_collections(self, mock_meilisearch): "created": created_date.timestamp(), "modified": updated_date.timestamp(), "access_id": lib_access.id, + "published": { + "num_children": 0 + }, "breadcrumbs": [{"display_name": "Library"}], } 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 e0a604c24feb..603cc8d92f5e 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -443,5 +443,37 @@ def test_collection_with_library(self): 'tags': { 'taxonomy': ['Difficulty'], 'level0': ['Difficulty > Normal'] + }, + "published": { + "num_children": 0 + } + } + + def test_collection_with_published_library(self): + library_api.publish_changes(self.library.key) + + doc = searchable_doc_for_collection(self.library.key, self.collection.key) + doc.update(searchable_doc_tags_for_collection(self.library.key, self.collection.key)) + + assert doc == { + "id": "lib-collectionedx2012_falltoy_collection-d1d907a4", + "block_id": self.collection.key, + "usage_key": self.collection_usage_key, + "type": "collection", + "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"}], + "created": 1680674828.0, + "modified": 1680674828.0, + 'tags': { + 'taxonomy': ['Difficulty'], + 'level0': ['Difficulty > Normal'] + }, + "published": { + "num_children": 1 } } diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 3a4b9535bbba..85cd2f2c06e8 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -83,6 +83,7 @@ ContentLibraryData, LibraryBlockData, LibraryCollectionData, + ContentObjectChangedData, ) from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_CREATED, @@ -92,6 +93,7 @@ LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, LIBRARY_COLLECTION_UPDATED, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, ) from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import ( @@ -684,7 +686,11 @@ def _get_library_component_tags_count(library_key) -> dict: return get_object_tag_counts(library_key_pattern, count_implicit=True) -def get_library_components(library_key, text_search=None, block_types=None) -> QuerySet[Component]: +def get_library_components( + library_key, + text_search=None, + block_types=None, +) -> QuerySet[Component]: """ Get the library components and filter. @@ -700,6 +706,7 @@ def get_library_components(library_key, text_search=None, block_types=None) -> Q type_names=block_types, draft_title=text_search, ) + return components @@ -1093,15 +1100,31 @@ def delete_library_block(usage_key, remove_from_parent=True): Delete the specified block from this library (soft delete). """ component = get_component_from_usage_key(usage_key) + library_key = usage_key.context_key + affected_collections = authoring_api.get_entity_collections(component.learning_package_id, component.key) + authoring_api.soft_delete_draft(component.pk) LIBRARY_BLOCK_DELETED.send_event( library_block=LibraryBlockData( - library_key=usage_key.context_key, + library_key=library_key, usage_key=usage_key ) ) + # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger + # collection indexing asynchronously. + # + # To delete the component on collections + for collection in affected_collections: + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_key=collection.key, + background=True, + ) + ) + def get_library_block_static_asset_files(usage_key) -> list[LibraryXBlockStaticFile]: """ @@ -1318,6 +1341,39 @@ def revert_changes(library_key): ) ) + # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger + # collection indexing asynchronously. + # + # This is to update component counts in all library collections, + # because there may be components that have been discarded in the revert. + for collection in authoring_api.get_collections(learning_package.id): + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + library_key=library_key, + collection_key=collection.key, + background=True, + ) + ) + + # Reindex components that are in collections + # + # Use case: When a component that was within a collection has been deleted + # and the changes are reverted, the component should appear in the + # collection again. + components_in_collections = authoring_api.get_components( + learning_package.id, draft=True, namespace='xblock.v1', + ).filter(publishable_entity__collections__isnull=False) + + for component in components_in_collections: + usage_key = library_component_usage_key(library_key, component) + + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=str(usage_key), + changes=["collections"], + ), + ) + def create_library_collection( library_key: LibraryLocatorV2, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index c526e7b1a1f3..7be3e592ba9d 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -561,3 +561,155 @@ def test_set_library_component_collections(self): }, collection_update_event_receiver.call_args_list[1].kwargs, ) + + def test_delete_library_block(self): + api.update_library_collection_components( + self.lib1.library_key, + self.col1.key, + usage_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + UsageKey.from_string(self.lib1_html_block["id"]), + ], + ) + + event_receiver = mock.Mock() + LIBRARY_COLLECTION_UPDATED.connect(event_receiver) + + api.delete_library_block(UsageKey.from_string(self.lib1_problem_block["id"])) + + assert event_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib1.library_key, + collection_key=self.col1.key, + background=True, + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + + def test_add_component_and_revert(self): + # Add component and publish + api.update_library_collection_components( + self.lib1.library_key, + self.col1.key, + usage_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + ], + ) + api.publish_changes(self.lib1.library_key) + + # Add component and revert + api.update_library_collection_components( + self.lib1.library_key, + self.col1.key, + usage_keys=[ + UsageKey.from_string(self.lib1_html_block["id"]), + ], + ) + + 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) + + api.revert_changes(self.lib1.library_key) + + assert collection_update_event_receiver.call_count == 1 + assert event_receiver.call_count == 2 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib1.library_key, + collection_key=self.col1.key, + background=True, + ), + }, + collection_update_event_receiver.call_args_list[0].kwargs, + ) + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "sender": None, + "content_object": ContentObjectChangedData( + object_id=str(self.lib1_problem_block["id"]), + changes=["collections"], + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "sender": None, + "content_object": ContentObjectChangedData( + object_id=str(self.lib1_html_block["id"]), + changes=["collections"], + ), + }, + event_receiver.call_args_list[1].kwargs, + ) + + def test_delete_component_and_revert(self): + # Add components and publish + api.update_library_collection_components( + self.lib1.library_key, + self.col1.key, + usage_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + UsageKey.from_string(self.lib1_html_block["id"]) + ], + ) + api.publish_changes(self.lib1.library_key) + + # Delete component and revert + api.delete_library_block(UsageKey.from_string(self.lib1_problem_block["id"])) + + 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) + + api.revert_changes(self.lib1.library_key) + + assert collection_update_event_receiver.call_count == 1 + assert event_receiver.call_count == 2 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + self.lib1.library_key, + collection_key=self.col1.key, + background=True, + ), + }, + collection_update_event_receiver.call_args_list[0].kwargs, + ) + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "sender": None, + "content_object": ContentObjectChangedData( + object_id=str(self.lib1_problem_block["id"]), + changes=["collections"], + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "sender": None, + "content_object": ContentObjectChangedData( + object_id=str(self.lib1_html_block["id"]), + changes=["collections"], + ), + }, + event_receiver.call_args_list[1].kwargs, + ) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 5a0939e571d1..21dc4cb0f882 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -135,7 +135,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.17.0 +openedx-learning==0.18.0 # 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 bdbc671e0958..20d2b6a7e0a2 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -827,7 +827,7 @@ openedx-filters==1.11.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-learning==0.17.0 +openedx-learning==0.18.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 26917829a5ed..fb806a43ce06 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1386,7 +1386,7 @@ openedx-filters==1.11.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-learning==0.17.0 +openedx-learning==0.18.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 6b8457a73bbf..86bd2ce9c42f 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -997,7 +997,7 @@ openedx-filters==1.11.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.17.0 +openedx-learning==0.18.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 9317adc968f7..d1be959f4ba7 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1042,7 +1042,7 @@ openedx-filters==1.11.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-learning==0.17.0 +openedx-learning==0.18.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt