fix: Keep sync component count in collection card [FC-0062] #35734
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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]: | ||
| """ | ||
| 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=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): | ||
|
Contributor
There was a problem hiding this comment. I'm not going to block this PR over this, but I don't think re-indexing every Collection in the Library is an acceptable long term solution. A Library might have many, many Collections. One of the deep problems with Modulestore has been the tendency for apps to have to reprocess the entire course because they don't have granular data on whether the pieces that are relevant to them have changed or not. I don't want that pattern to get started here. The
Contributor
Author
There was a problem hiding this comment. I agree with this, could this be done in a follow-up task? @bradenmacdonald @pomegranited
Contributor
There was a problem hiding this comment. Yes, as long as the bug is fixed now and the fix is backported to Sumac, we can address those refactors and improvements in a follow-up task. Let's do that as soon as sumac.0 is cut. Doing it now will complicate other backports like this one, especially if any change to openedx-learning are required. But I don't want to postpone it any later than that, or we won't get to it. Ideally you can start on the PRs for it now, even if we don't merge until after Sumac.0 is released. |
||
| 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, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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): | ||
|
Contributor
There was a problem hiding this comment. Thank you for testing this case too @ChrisChV ! |
||
| # 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, | ||
| ) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[off-topic] Out of scope, but also wanted to note that
revert_changesmight not be the best name for this function. It's described as "Discard Changes" in the UI, and it means specifically to reset the draft version to be the published version for all content. The word "revert" might be more appropriate to use when you have something that was already published, and you want to undo that publish. That's definitely a more product question though.