Skip to content

Commit

Permalink
fix: keep library collection card component count in sync (#35734)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ChrisChV
ChrisChV authored Nov 19, 2024
1 parent 49330a2 commit d5850c8
Show file tree
Hide file tree
Showing 10 changed files with 276 additions and 8 deletions.
15 changes: 14 additions & 1 deletion openedx/core/djangoapps/content/search/documents.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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),
Expand All @@ -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}],
})
Expand Down
15 changes: 15 additions & 0 deletions openedx/core/djangoapps/content/search/tests/test_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"}],
}

Expand Down Expand Up @@ -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 = {
Expand All @@ -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 = {
Expand All @@ -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 = {
Expand All @@ -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 = {
Expand Down
32 changes: 32 additions & 0 deletions openedx/core/djangoapps/content/search/tests/test_documents.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
60 changes: 58 additions & 2 deletions openedx/core/djangoapps/content_libraries/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
ContentLibraryData,
LibraryBlockData,
LibraryCollectionData,
ContentObjectChangedData,
)
from openedx_events.content_authoring.signals import (
CONTENT_LIBRARY_CREATED,
Expand All @@ -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 (
Expand Down Expand Up @@ -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.
Expand All @@ -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


Expand Down Expand Up @@ -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]:
"""
Expand Down Expand Up @@ -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,
Expand Down
152 changes: 152 additions & 0 deletions openedx/core/djangoapps/content_libraries/tests/test_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
2 changes: 1 addition & 1 deletion requirements/constraints.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading

0 comments on commit d5850c8

Please sign in to comment.