From 702ea031c324f701815b3a778573f8d7a2e755fe Mon Sep 17 00:00:00 2001 From: katrinan029 <71999631+katrinan029@users.noreply.github.com> Date: Thu, 12 Sep 2024 20:58:42 +0000 Subject: [PATCH 1/8] feat: Upgrade Python dependency edx-enterprise version bump Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` --- 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 8c95f7fcc2..c67cd72953 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -26,7 +26,7 @@ celery>=5.2.2,<6.0.0 # The team that owns this package will manually bump this package rather than having it pulled in automatically. # This is to allow them to better control its deployment and to do it in a process that works better # for them. -edx-enterprise==4.25.10 +edx-enterprise==4.25.11 # Stay on LTS version, remove once this is added to common constraint Django<5.0 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 9c85f60fc3..7254e00a8f 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -467,7 +467,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.10 +edx-enterprise==4.25.11 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 15e078ae0d..2e9c422391 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -741,7 +741,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.10 +edx-enterprise==4.25.11 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index defc09a23d..ab2af959df 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -547,7 +547,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.10 +edx-enterprise==4.25.11 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 49be07ecec..a660795c24 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -571,7 +571,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.25.10 +edx-enterprise==4.25.11 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From c41fe8991832de7e987368a533d4b0e9b91fefce Mon Sep 17 00:00:00 2001 From: Jillian Date: Fri, 13 Sep 2024 22:50:54 +0930 Subject: [PATCH 2/8] Store content object collections in search index [FC-0062] (#35469) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: Add Library Collections REST endpoints * test: Add tests for Collections REST APIs * chore: Add missing __init__ files * docs: Add warning about unstable REST APIs * feat: emit CONTENT_OBJECT_ASSOCIATIONS_CHANGED whenever a content object's tags or collections have changed, and handle that event in content/search. The deprecated CONTENT_OBJECT_TAGS_CHANGED event is still emitted when tags change; to be removed after Sumac. * docs: replaces CONTENT_OBJECT_TAGS_CHANGED with CONTENT_OBJECT_ASSOCIATIONS_CHANGED * chore: updates openedx-events==9.14.0 * chore: updates openedx-learning==0.11.4 Co-authored-by: Yusuf Musleh Co-authored-by: Chris Chávez Co-authored-by: Rômulo Penido --- docs/hooks/events.rst | 8 +- openedx/core/djangoapps/content/search/api.py | 35 +++- .../djangoapps/content/search/documents.py | 78 ++++++++- .../djangoapps/content/search/handlers.py | 62 ++++++-- .../core/djangoapps/content/search/tasks.py | 13 ++ .../content/search/tests/test_api.py | 150 ++++++++++++++++-- .../content/search/tests/test_documents.py | 114 ++++++++++++- .../core/djangoapps/content_libraries/api.py | 13 +- .../content_libraries/tests/test_api.py | 24 +-- .../core/djangoapps/content_tagging/api.py | 28 +++- .../content_tagging/rest_api/v1/views.py | 19 ++- 11 files changed, 491 insertions(+), 53 deletions(-) diff --git a/docs/hooks/events.rst b/docs/hooks/events.rst index 2a7561df24..bccb98e56a 100644 --- a/docs/hooks/events.rst +++ b/docs/hooks/events.rst @@ -244,10 +244,6 @@ Content Authoring Events - org.openedx.content_authoring.library_block.deleted.v1 - 2023-07-20 - * - `CONTENT_OBJECT_TAGS_CHANGED `_ - - org.openedx.content_authoring.content.object.tags.changed.v1 - - 2024-03-31 - * - `LIBRARY_COLLECTION_CREATED `_ - org.openedx.content_authoring.content_library.collection.created.v1 - 2024-08-23 @@ -259,3 +255,7 @@ Content Authoring Events * - `LIBRARY_COLLECTION_DELETED `_ - org.openedx.content_authoring.content_library.collection.deleted.v1 - 2024-08-23 + + * - `CONTENT_OBJECT_ASSOCIATIONS_CHANGED `_ + - org.openedx.content_authoring.content.object.associations.changed.v1 + - 2024-09-06 diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 76bb5eb3f4..4a775a710d 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -34,6 +34,7 @@ from .documents import ( searchable_doc_for_course_block, searchable_doc_for_collection, searchable_doc_for_library_block, + searchable_doc_collections, searchable_doc_tags, ) @@ -322,6 +323,9 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.tags + "." + Fields.tags_level1, Fields.tags + "." + Fields.tags_level2, Fields.tags + "." + Fields.tags_level3, + Fields.collections, + Fields.collections + "." + Fields.collections_display_name, + Fields.collections + "." + Fields.collections_key, Fields.type, Fields.access_id, Fields.last_published, @@ -333,8 +337,9 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.display_name, Fields.block_id, Fields.content, - Fields.tags, Fields.description, + Fields.tags, + Fields.collections, # If we don't list the following sub-fields _explicitly_, they're only sometimes searchable - that is, they # are searchable only if at least one document in the index has a value. If we didn't list them here and, # say, there were no tags.level3 tags in the index, the client would get an error if trying to search for @@ -344,6 +349,8 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.tags + "." + Fields.tags_level1, Fields.tags + "." + Fields.tags_level2, Fields.tags + "." + Fields.tags_level3, + Fields.collections + "." + Fields.collections_display_name, + Fields.collections + "." + Fields.collections_key, ]) # Mark which attributes can be used for sorting search results: client.index(temp_index_name).update_sortable_attributes([ @@ -375,6 +382,7 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: doc = {} doc.update(searchable_doc_for_library_block(metadata)) doc.update(searchable_doc_tags(metadata.usage_key)) + doc.update(searchable_doc_collections(metadata.usage_key)) docs.append(doc) except Exception as err: # pylint: disable=broad-except status_cb(f"Error indexing library component {component}: {err}") @@ -549,6 +557,22 @@ def upsert_library_block_index_doc(usage_key: UsageKey) -> None: _update_index_docs(docs) +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 + """ + content_library = lib_api.ContentLibrary.objects.get_by_key(library_key) + collection = authoring_api.get_collection( + learning_package_id=content_library.learning_package_id, + collection_key=collection_key, + ) + docs = [ + searchable_doc_for_collection(collection) + ] + + _update_index_docs(docs) + + def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: """ Creates or updates the documents for the given Content Library in the search index @@ -571,6 +595,15 @@ def upsert_block_tags_index_docs(usage_key: UsageKey): _update_index_docs([doc]) +def upsert_block_collections_index_docs(usage_key: UsageKey): + """ + Updates the collections data in documents for the given Course/Library block + """ + doc = {Fields.id: meili_id_from_opaque_key(usage_key)} + doc.update(searchable_doc_collections(usage_key)) + _update_index_docs([doc]) + + def _get_user_orgs(request: Request) -> list[str]: """ Get the org.short_names for the organizations that the requesting user has OrgStaffRole or OrgInstructorRole. diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index a45b37ab2a..6f19b610fe 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -7,7 +7,9 @@ import logging from hashlib import blake2b from django.utils.text import slugify +from django.core.exceptions import ObjectDoesNotExist from opaque_keys.edx.keys import LearningContextKey, UsageKey +from openedx_learning.api import authoring as authoring_api from openedx.core.djangoapps.content.search.models import SearchAccess from openedx.core.djangoapps.content_libraries import api as lib_api @@ -26,7 +28,11 @@ class Fields: id = "id" usage_key = "usage_key" type = "type" # DocType.course_block or DocType.library_block (see below) - block_id = "block_id" # The block_id part of the usage key. Sometimes human-readable, sometimes a random hex ID + # The block_id part of the usage key for course or library blocks. + # If it's a collection, the collection.key is stored here. + # Sometimes human-readable, sometimes a random hex ID + # Is only unique within the given context_key. + block_id = "block_id" display_name = "display_name" description = "description" modified = "modified" @@ -52,11 +58,21 @@ class Fields: tags_level1 = "level1" tags_level2 = "level2" tags_level3 = "level3" + # Collections (dictionary) that this object belongs to. + # Similarly to tags above, we collect the collection.titles and collection.keys into hierarchical facets. + collections = "collections" + collections_display_name = "display_name" + collections_key = "key" + # The "content" field is a dictionary of arbitrary data, depending on the block_type. # It comes from each XBlock's index_dictionary() method (if present) plus some processing. # Text (html) blocks have an "html_content" key in here, capa has "capa_content" and "problem_types", and so on. content = "content" + # Collections use this field to communicate how many entities/components they contain. + # Structural XBlocks may use this one day to indicate how many child blocks they ocntain. + 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' # command (changing those settings on an large active index is not recommended). @@ -223,6 +239,51 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: return {Fields.tags: result} +def _collections_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: + """ + Given an XBlock, course, library, etc., get the collections for its index doc. + + e.g. for something in Collections "COL_A" and "COL_B", this would return: + { + "collections": { + "display_name": ["Collection A", "Collection B"], + "key": ["COL_A", "COL_B"], + } + } + + If the object is in no collections, returns: + { + "collections": {}, + } + + """ + # Gather the collections associated with this object + collections = None + try: + component = lib_api.get_component_from_usage_key(object_id) + collections = authoring_api.get_entity_collections( + component.learning_package_id, + component.key, + ) + except ObjectDoesNotExist: + log.warning(f"No component found for {object_id}") + + if not collections: + return {Fields.collections: {}} + + 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) + + return result + + def searchable_doc_for_library_block(xblock_metadata: lib_api.LibraryXBlockMetadata) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine @@ -265,6 +326,19 @@ def searchable_doc_tags(usage_key: UsageKey) -> dict: return doc +def searchable_doc_collections(usage_key: UsageKey) -> dict: + """ + Generate a dictionary document suitable for ingestion into a search engine + like Meilisearch or Elasticsearch, with the collections data for the given content object. + """ + doc = { + Fields.id: meili_id_from_opaque_key(usage_key), + } + doc.update(_collections_for_content_object(usage_key)) + + return doc + + def searchable_doc_for_course_block(block) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine @@ -289,6 +363,7 @@ def searchable_doc_for_collection(collection) -> dict: """ doc = { Fields.id: collection.id, + Fields.block_id: collection.key, Fields.type: DocType.collection, Fields.display_name: collection.title, Fields.description: collection.description, @@ -298,6 +373,7 @@ def searchable_doc_for_collection(collection) -> dict: # If related contentlibrary is found, it will override this value below. # Mostly contentlibrary.library_key == learning_package.key Fields.context_key: collection.learning_package.key, + Fields.num_children: collection.entities.count(), } # Just in case learning_package is not related to a library try: diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 94ac02ea16..6a341c92ed 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -6,30 +6,40 @@ import logging from django.db.models.signals import post_delete from django.dispatch import receiver -from openedx_events.content_authoring.data import ContentLibraryData, ContentObjectData, LibraryBlockData, XBlockData +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey +from openedx_events.content_authoring.data import ( + ContentLibraryData, + ContentObjectChangedData, + LibraryBlockData, + LibraryCollectionData, + XBlockData, +) from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, + LIBRARY_COLLECTION_CREATED, + LIBRARY_COLLECTION_UPDATED, XBLOCK_CREATED, XBLOCK_DELETED, XBLOCK_UPDATED, - CONTENT_OBJECT_TAGS_CHANGED, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, ) -from openedx.core.djangoapps.content_tagging.utils import get_content_key_from_string from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.search.models import SearchAccess -from .api import only_if_meilisearch_enabled, upsert_block_tags_index_docs +from .api import only_if_meilisearch_enabled, upsert_block_collections_index_docs, upsert_block_tags_index_docs from .tasks import ( delete_library_block_index_doc, delete_xblock_index_doc, update_content_library_index_docs, + update_library_collection_index_doc, upsert_library_block_index_doc, - upsert_xblock_index_doc + upsert_xblock_index_doc, ) log = logging.getLogger(__name__) @@ -145,22 +155,48 @@ def content_library_updated_handler(**kwargs) -> None: update_content_library_index_docs.apply(args=[str(content_library_data.library_key)]) -@receiver(CONTENT_OBJECT_TAGS_CHANGED) +@receiver(LIBRARY_COLLECTION_CREATED) +@receiver(LIBRARY_COLLECTION_UPDATED) @only_if_meilisearch_enabled -def content_object_tags_changed_handler(**kwargs) -> None: +def library_collection_updated_handler(**kwargs) -> None: """ - Update the tags data in the index for the Content Object + Create or update the index for the content library collection """ - content_object_tags = kwargs.get("content_object", None) - if not content_object_tags or not isinstance(content_object_tags, ContentObjectData): + library_collection = kwargs.get("library_collection", None) + if not library_collection or not isinstance(library_collection, LibraryCollectionData): # pragma: no cover + 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, + ]) + + +@receiver(CONTENT_OBJECT_ASSOCIATIONS_CHANGED) +@only_if_meilisearch_enabled +def content_object_associations_changed_handler(**kwargs) -> None: + """ + Update the collections/tags data in the index for the Content Object + """ + content_object = kwargs.get("content_object", None) + if not content_object or not isinstance(content_object, ContentObjectChangedData): log.error("Received null or incorrect data for event") return try: # Check if valid if course or library block - get_content_key_from_string(content_object_tags.object_id) - except ValueError: + usage_key = UsageKey.from_string(str(content_object.object_id)) + except InvalidKeyError: log.error("Received invalid content object id") return - upsert_block_tags_index_docs(content_object_tags.object_id) + # This event's changes may contain both "tags" and "collections", but this will happen rarely, if ever. + # So we allow a potential double "upsert" here. + if not content_object.changes or "tags" in content_object.changes: + upsert_block_tags_index_docs(usage_key) + if not content_object.changes or "collections" in content_object.changes: + upsert_block_collections_index_docs(usage_key) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index dfd6037769..d9dad834db 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -84,3 +84,16 @@ def update_content_library_index_docs(library_key_str: str) -> None: # Delete all documents in this library that were not published by above function # as this task is also triggered on discard event. api.delete_all_draft_docs_for_library(library_key) + + +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) +@set_code_owner_attribute +def update_library_collection_index_doc(library_key_str: str, collection_key: str) -> None: + """ + Celery task to update the content index documents for a library collection + """ + library_key = LibraryLocatorV2.from_string(library_key_str) + + log.info("Updating content index documents for collection %s in library%s", collection_key, library_key) + + api.upsert_library_collection_index_doc(library_key, 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 d6b58674f5..023265f4d0 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -186,16 +186,18 @@ class TestSearchApi(ModuleStoreTestCase): description="my collection description" ) self.collection_dict = { - 'id': self.collection.id, - 'type': 'collection', - 'display_name': 'my_collection', - 'description': 'my collection description', - 'context_key': 'lib:org1:lib', - 'org': 'org1', - 'created': created_date.timestamp(), - 'modified': created_date.timestamp(), + "id": self.collection.id, + "block_id": self.collection.key, + "type": "collection", + "display_name": "my_collection", + "description": "my collection description", + "num_children": 0, + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": created_date.timestamp(), "access_id": lib_access.id, - 'breadcrumbs': [{'display_name': 'Library'}] + "breadcrumbs": [{"display_name": "Library"}], } @override_settings(MEILISEARCH_ENABLED=False) @@ -215,10 +217,13 @@ class TestSearchApi(ModuleStoreTestCase): doc_vertical["tags"] = {} doc_problem1 = copy.deepcopy(self.doc_problem1) doc_problem1["tags"] = {} + doc_problem1["collections"] = {} doc_problem2 = copy.deepcopy(self.doc_problem2) doc_problem2["tags"] = {} + doc_problem2["collections"] = {} api.rebuild_index() + assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 3 mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls( [ call([doc_sequential, doc_vertical]), @@ -254,6 +259,7 @@ class TestSearchApi(ModuleStoreTestCase): doc_vertical["tags"] = {} doc_problem2 = copy.deepcopy(self.doc_problem2) doc_problem2["tags"] = {} + doc_problem2["collections"] = {} orig_from_component = library_api.LibraryXBlockMetadata.from_component @@ -346,6 +352,7 @@ class TestSearchApi(ModuleStoreTestCase): } } + 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_sequential_with_tags1]), @@ -400,6 +407,7 @@ class TestSearchApi(ModuleStoreTestCase): } } + 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_problem_with_tags1]), @@ -408,6 +416,130 @@ class TestSearchApi(ModuleStoreTestCase): any_order=True, ) + @override_settings(MEILISEARCH_ENABLED=True) + def test_index_library_block_and_collections(self, mock_meilisearch): + """ + Test indexing an Library Block and the Collections it's in. + """ + # Create collections (these internally call `upsert_library_collection_index_doc`) + created_date = datetime(2023, 5, 6, 7, 8, 9, tzinfo=timezone.utc) + with freeze_time(created_date): + collection1 = library_api.create_library_collection( + self.library.key, + collection_key="COL1", + title="Collection 1", + created_by=None, + description="First Collection", + ) + + collection2 = library_api.create_library_collection( + self.library.key, + collection_key="COL2", + title="Collection 2", + created_by=None, + description="Second Collection", + ) + + # Add Problem1 to both Collections (these internally call `upsert_block_collections_index_docs` and + # `upsert_library_collection_index_doc`) + # (adding in reverse order to test sorting of collection tag) + updated_date = datetime(2023, 6, 7, 8, 9, 10, tzinfo=timezone.utc) + with freeze_time(updated_date): + for collection in (collection2, collection1): + library_api.update_library_collection_components( + self.library.key, + collection_key=collection.key, + usage_keys=[ + self.problem1.usage_key, + ], + ) + + # Build expected docs at each stage + lib_access, _ = SearchAccess.objects.get_or_create(context_key=self.library.key) + doc_collection1_created = { + "id": collection1.id, + "block_id": collection1.key, + "type": "collection", + "display_name": "Collection 1", + "description": "First Collection", + "num_children": 0, + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": created_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } + doc_collection2_created = { + "id": collection2.id, + "block_id": collection2.key, + "type": "collection", + "display_name": "Collection 2", + "description": "Second Collection", + "num_children": 0, + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": created_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } + doc_collection2_updated = { + "id": collection2.id, + "block_id": collection2.key, + "type": "collection", + "display_name": "Collection 2", + "description": "Second Collection", + "num_children": 1, + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": updated_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } + doc_collection1_updated = { + "id": collection1.id, + "block_id": collection1.key, + "type": "collection", + "display_name": "Collection 1", + "description": "First Collection", + "num_children": 1, + "context_key": "lib:org1:lib", + "org": "org1", + "created": created_date.timestamp(), + "modified": updated_date.timestamp(), + "access_id": lib_access.id, + "breadcrumbs": [{"display_name": "Library"}], + } + doc_problem_with_collection1 = { + "id": self.doc_problem1["id"], + "collections": { + "display_name": ["Collection 2"], + "key": ["COL2"], + }, + } + doc_problem_with_collection2 = { + "id": self.doc_problem1["id"], + "collections": { + "display_name": ["Collection 1", "Collection 2"], + "key": ["COL1", "COL2"], + }, + } + + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 6 + mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( + [ + call([doc_collection1_created]), + call([doc_collection2_created]), + call([doc_collection2_updated]), + call([doc_collection1_updated]), + call([doc_problem_with_collection1]), + call([doc_problem_with_collection2]), + ], + any_order=True, + ) + @override_settings(MEILISEARCH_ENABLED=True) def test_delete_index_library_block(self, mock_meilisearch): """ diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 1f10fc3c98..7ff330c0b4 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -8,6 +8,7 @@ from freezegun import freeze_time from openedx_learning.api import authoring as authoring_api from openedx.core.djangoapps.content_tagging import api as tagging_api +from openedx.core.djangoapps.content_libraries import api as library_api from openedx.core.djangolib.testing.utils import skip_unless_cms from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -15,12 +16,19 @@ from xmodule.modulestore.tests.factories import BlockFactory, ToyCourseFactory try: # This import errors in the lms because content.search is not an installed app there. - from ..documents import searchable_doc_for_course_block, searchable_doc_tags, searchable_doc_for_collection + from ..documents import ( + searchable_doc_for_course_block, + searchable_doc_tags, + searchable_doc_collections, + searchable_doc_for_collection, + searchable_doc_for_library_block, + ) from ..models import SearchAccess except RuntimeError: searchable_doc_for_course_block = lambda x: x searchable_doc_tags = lambda x: x searchable_doc_for_collection = lambda x: x + searchable_doc_for_library_block = lambda x: x SearchAccess = {} @@ -38,12 +46,13 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): def setUpClass(cls): super().setUpClass() cls.store = modulestore() + cls.org = Organization.objects.create(name="edX", short_name="edX") cls.toy_course = ToyCourseFactory.create() # See xmodule/modulestore/tests/sample_courses.py cls.toy_course_key = cls.toy_course.id # Get references to some blocks in the toy course cls.html_block_key = cls.toy_course_key.make_usage_key("html", "toyjumpto") - # Create a problem in library + # Create a problem in course cls.problem_block = BlockFactory.create( category="problem", parent_location=cls.toy_course_key.make_usage_key("vertical", "vertical_test"), @@ -51,8 +60,38 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): data="What is a test?", ) + # Create a library and collection with a block + created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) + with freeze_time(created_date): + cls.library = library_api.create_library( + org=cls.org, + slug="2012_Fall", + title="some content_library", + description="some description", + ) + cls.collection = library_api.create_library_collection( + cls.library.key, + collection_key="TOY_COLLECTION", + title="Toy Collection", + created_by=None, + description="my toy collection description" + ) + cls.library_block = library_api.create_library_block( + cls.library.key, + "html", + "text2", + ) + + # Add the problem block to the collection + library_api.update_library_collection_components( + cls.library.key, + collection_key="TOY_COLLECTION", + usage_keys=[ + cls.library_block.usage_key, + ] + ) + # Create a couple taxonomies and some tags - cls.org = Organization.objects.create(name="edX", short_name="edX") cls.difficulty_tags = tagging_api.create_taxonomy(name="Difficulty", orgs=[cls.org], allow_multiple=False) tagging_api.add_tag_to_taxonomy(cls.difficulty_tags, tag="Easy") tagging_api.add_tag_to_taxonomy(cls.difficulty_tags, tag="Normal") @@ -69,6 +108,7 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): tagging_api.tag_object(str(cls.problem_block.usage_key), cls.difficulty_tags, tags=["Easy"]) tagging_api.tag_object(str(cls.html_block_key), cls.subject_tags, tags=["Chinese", "Jump Links"]) tagging_api.tag_object(str(cls.html_block_key), cls.difficulty_tags, tags=["Normal"]) + tagging_api.tag_object(str(cls.library_block.usage_key), cls.difficulty_tags, tags=["Normal"]) @property def toy_course_access_id(self): @@ -80,6 +120,16 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): """ return SearchAccess.objects.get(context_key=self.toy_course_key).id + @property + def library_access_id(self): + """ + Returns the SearchAccess.id created for the library. + + This SearchAccess object is created when documents are added to the search index, so this method must be called + after this step, or risk a DoesNotExist error. + """ + return SearchAccess.objects.get(context_key=self.library.key).id + def test_problem_block(self): """ Test how a problem block gets represented in the search index @@ -205,6 +255,62 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): # This video has no tags. } + def test_html_library_block(self): + """ + Test how a library block gets represented in the search index + """ + doc = {} + doc.update(searchable_doc_for_library_block(self.library_block)) + doc.update(searchable_doc_tags(self.library_block.usage_key)) + doc.update(searchable_doc_collections(self.library_block.usage_key)) + assert doc == { + "id": "lbedx2012_fallhtmltext2-4bb47d67", + "type": "library_block", + "block_type": "html", + "usage_key": "lb:edX:2012_Fall:html:text2", + "block_id": "text2", + "context_key": "lib:edX:2012_Fall", + "org": "edX", + "access_id": self.library_access_id, + "display_name": "Text", + "breadcrumbs": [ + { + "display_name": "some content_library", + }, + ], + "last_published": None, + "created": 1680674828.0, + "modified": 1680674828.0, + "content": { + "html_content": "", + }, + "collections": { + "key": ["TOY_COLLECTION"], + "display_name": ["Toy Collection"], + }, + "tags": { + "taxonomy": ["Difficulty"], + "level0": ["Difficulty > Normal"], + }, + } + + def test_collection_with_library(self): + doc = searchable_doc_for_collection(self.collection) + assert doc == { + "id": self.collection.id, + "block_id": self.collection.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, + } + def test_collection_with_no_library(self): created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) with freeze_time(created_date): @@ -223,9 +329,11 @@ class StudioDocumentsTest(SharedModuleStoreTestCase): doc = searchable_doc_for_collection(collection) assert doc == { "id": collection.id, + "block_id": collection.key, "type": "collection", "display_name": "my_collection", "description": "my collection description", + "num_children": 0, "context_key": learning_package.key, "access_id": self.toy_course_access_id, "breadcrumbs": [{"display_name": "some learning_package"}], diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 0011014345..c19c9bf880 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -78,12 +78,12 @@ from opaque_keys.edx.locator import ( from opaque_keys import InvalidKeyError from openedx_events.content_authoring.data import ( ContentLibraryData, - ContentObjectData, + ContentObjectChangedData, LibraryBlockData, LibraryCollectionData, ) from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_TAGS_CHANGED, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, CONTENT_LIBRARY_CREATED, CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, @@ -1235,10 +1235,13 @@ def update_library_collection_components( ) ) - # Emit a CONTENT_OBJECT_TAGS_CHANGED event for each of the objects added/removed + # Emit a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for each of the objects added/removed for usage_key in usage_keys: - CONTENT_OBJECT_TAGS_CHANGED.send_event( - content_object=ContentObjectData(object_id=usage_key), + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=str(usage_key), + changes=["collections"], + ), ) return collection diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index b7b646acac..b02e71b002 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -14,11 +14,11 @@ from opaque_keys.edx.keys import ( ) from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_events.content_authoring.data import ( - ContentObjectData, + ContentObjectChangedData, LibraryCollectionData, ) from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_TAGS_CHANGED, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_COLLECTION_CREATED, LIBRARY_COLLECTION_UPDATED, ) @@ -262,7 +262,7 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTe Same guidelines as ContentLibrariesTestCase. """ ENABLED_OPENEDX_EVENTS = [ - CONTENT_OBJECT_TAGS_CHANGED.event_type, + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.event_type, LIBRARY_COLLECTION_CREATED.event_type, LIBRARY_COLLECTION_UPDATED.event_type, ] @@ -411,10 +411,10 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTe def test_update_library_collection_components_event(self): """ - Check that a CONTENT_OBJECT_TAGS_CHANGED event is raised for each added/removed component. + Check that a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event is raised for each added/removed component. """ event_receiver = mock.Mock() - CONTENT_OBJECT_TAGS_CHANGED.connect(event_receiver) + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) LIBRARY_COLLECTION_UPDATED.connect(event_receiver) api.update_library_collection_components( @@ -440,20 +440,22 @@ class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, OpenEdxEventsTe ) self.assertDictContainsSubset( { - "signal": CONTENT_OBJECT_TAGS_CHANGED, + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "sender": None, - "content_object": ContentObjectData( - object_id=UsageKey.from_string(self.lib1_problem_block["id"]), + "content_object": ContentObjectChangedData( + object_id=self.lib1_problem_block["id"], + changes=["collections"], ), }, event_receiver.call_args_list[1].kwargs, ) self.assertDictContainsSubset( { - "signal": CONTENT_OBJECT_TAGS_CHANGED, + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "sender": None, - "content_object": ContentObjectData( - object_id=UsageKey.from_string(self.lib1_html_block["id"]), + "content_object": ContentObjectChangedData( + object_id=self.lib1_html_block["id"], + changes=["collections"], ), }, event_receiver.call_args_list[2].kwargs, diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 47b157c1a3..b63c70c5f0 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -18,8 +18,11 @@ from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from openedx_tagging.core.tagging.models.utils import TAGS_CSV_SEPARATOR from organizations.models import Organization from .helpers.objecttag_export_helpers import build_object_tree_with_objecttags, iterate_with_level -from openedx_events.content_authoring.data import ContentObjectData -from openedx_events.content_authoring.signals import CONTENT_OBJECT_TAGS_CHANGED +from openedx_events.content_authoring.data import ContentObjectData, ContentObjectChangedData +from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + CONTENT_OBJECT_TAGS_CHANGED, +) from .models import TaxonomyOrg from .types import ContentKey, TagValuesByObjectIdDict, TagValuesByTaxonomyIdDict, TaxonomyDict @@ -301,6 +304,16 @@ def set_exported_object_tags( create_invalid=True, taxonomy_export_id=str(taxonomy_export_id), ) + + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + time=now(), + content_object=ContentObjectChangedData( + object_id=content_key_str, + changes=["tags"], + ) + ) + + # Emit a (deprecated) CONTENT_OBJECT_TAGS_CHANGED event too CONTENT_OBJECT_TAGS_CHANGED.send_event( time=now(), content_object=ContentObjectData(object_id=content_key_str) @@ -378,7 +391,7 @@ def tag_object( Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags, if the taxonomy can be used by the given object_id. - This is a wrapper around oel_tagging.tag_object that adds emitting the `CONTENT_OBJECT_TAGS_CHANGED` event + This is a wrapper around oel_tagging.tag_object that adds emitting the `CONTENT_OBJECT_ASSOCIATIONS_CHANGED` event when tagging an object. tags: A list of the values of the tags from this taxonomy to apply. @@ -399,6 +412,15 @@ def tag_object( taxonomy=taxonomy, tags=tags, ) + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + time=now(), + content_object=ContentObjectChangedData( + object_id=object_id, + changes=["tags"], + ) + ) + + # Emit a (deprecated) CONTENT_OBJECT_TAGS_CHANGED event too CONTENT_OBJECT_TAGS_CHANGED.send_event( time=now(), content_object=ContentObjectData(object_id=object_id) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index 71b210b9e5..3fc99736ba 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -13,8 +13,11 @@ from rest_framework.exceptions import PermissionDenied, ValidationError from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView -from openedx_events.content_authoring.data import ContentObjectData -from openedx_events.content_authoring.signals import CONTENT_OBJECT_TAGS_CHANGED +from openedx_events.content_authoring.data import ContentObjectData, ContentObjectChangedData +from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + CONTENT_OBJECT_TAGS_CHANGED, +) from ...auth import has_view_object_tags_access from ...api import ( @@ -149,14 +152,24 @@ class ObjectTagOrgView(ObjectTagView): def update(self, request, *args, **kwargs) -> Response: """ - Extend the update method to fire CONTENT_OBJECT_TAGS_CHANGED event + Extend the update method to fire CONTENT_OBJECT_ASSOCIATIONS_CHANGED event """ response = super().update(request, *args, **kwargs) if response.status_code == 200: object_id = kwargs.get('object_id') + + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=object_id, + changes=["tags"], + ) + ) + + # Emit a (deprecated) CONTENT_OBJECT_TAGS_CHANGED event too CONTENT_OBJECT_TAGS_CHANGED.send_event( content_object=ContentObjectData(object_id=object_id) ) + return response From dd59dc634a56a57449a8254d2b8bce294bae280a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Fri, 13 Sep 2024 09:19:25 -0500 Subject: [PATCH 3/8] Tag in Collections [FC-0062] (#35383) * feat: Allow tag in collections * chore: Bump version of opaque-keys --- .../core/djangoapps/content_tagging/api.py | 4 +- .../rest_api/v1/tests/test_views.py | 91 ++++++++++++++++++- .../content_tagging/tests/test_api.py | 19 +++- .../core/djangoapps/content_tagging/types.py | 4 +- .../core/djangoapps/content_tagging/utils.py | 16 +++- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/paver.txt | 2 +- requirements/edx/testing.txt | 2 +- 10 files changed, 130 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index b63c70c5f0..f015770e5d 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -12,7 +12,7 @@ from opaque_keys.edx.keys import UsageKey import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import Exists, OuterRef, Q, QuerySet from django.utils.timezone import now -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, LibraryCollectionKey from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from openedx_tagging.core.tagging.models.utils import TAGS_CSV_SEPARATOR @@ -230,7 +230,7 @@ def generate_csv_rows(object_id, buffer) -> Iterator[str]: """ content_key = get_content_key_from_string(object_id) - if isinstance(content_key, UsageKey): + if isinstance(content_key, (UsageKey, LibraryCollectionKey)): raise ValueError("The object_id must be a CourseKey or a LibraryLocatorV2.") all_object_tags, taxonomies = get_all_object_tags(content_key) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index f64fba5c33..e386ee2342 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -13,7 +13,7 @@ from urllib.parse import parse_qs, urlparse import ddt from django.contrib.auth import get_user_model from django.core.files.uploadedfile import SimpleUploadedFile -from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryCollectionLocator from openedx_tagging.core.tagging.models import Tag, Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomySerializer @@ -111,6 +111,9 @@ class TestTaxonomyObjectsMixin: ) self.libraryA = str(self.content_libraryA.key) + def _setUp_collection(self): + self.collection_key = str(LibraryCollectionLocator(self.content_libraryA.key, 'test-collection')) + def _setUp_users(self): """ Create users for testing @@ -284,6 +287,7 @@ class TestTaxonomyObjectsMixin: self._setUp_library() self._setUp_users() self._setUp_taxonomies() + self._setUp_collection() # Clear the rules cache in between test runs to keep query counts consistent. rules_cache.clear() @@ -1653,6 +1657,87 @@ class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): response = self._call_put_request(self.libraryA, taxonomy.pk, ["invalid"]) assert response.status_code == status.HTTP_400_BAD_REQUEST + @ddt.data( + # staffA and staff are staff in collection and can tag using enabled taxonomies + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("user", "tA1", [], status.HTTP_403_FORBIDDEN), + ("staffA", "tA1", [], status.HTTP_200_OK), + ("staff", "tA1", [], status.HTTP_200_OK), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("staffA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("staffA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("staff", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ) + @ddt.unpack + def test_tag_collection(self, user_attr, taxonomy_attr, tag_values, expected_status): + """ + Tests that only staff and org level users can tag collections + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + response = self._call_put_request(self.collection_key, taxonomy.pk, tag_values) + + assert response.status_code == expected_status + if status.is_success(expected_status): + tags_by_taxonomy = response.data[str(self.collection_key)]["taxonomies"] + if tag_values: + response_taxonomy = tags_by_taxonomy[0] + assert response_taxonomy["name"] == taxonomy.name + response_tags = response_taxonomy["tags"] + assert [t["value"] for t in response_tags] == tag_values + else: + assert tags_by_taxonomy == [] # No tags are set from any taxonomy + + # Check that re-fetching the tags returns what we set + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.collection_key) + new_response = self.client.get(url, format="json") + assert status.is_success(new_response.status_code) + assert new_response.data == response.data + + @ddt.data( + "staffA", + "staff", + ) + def test_tag_collection_disabled_taxonomy(self, user_attr): + """ + Nobody can use disabled taxonomies to tag objects + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + disabled_taxonomy = self.tA2 + assert disabled_taxonomy.enabled is False + + response = self._call_put_request(self.collection_key, disabled_taxonomy.pk, ["Tag 1"]) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + @ddt.data( + ("staffA", "tA1"), + ("staff", "tA1"), + ("staffA", "multiple_taxonomy"), + ("staff", "multiple_taxonomy"), + ) + @ddt.unpack + def test_tag_collection_invalid(self, user_attr, taxonomy_attr): + """ + Tests that nobody can add invalid tags to a collection using a closed taxonomy + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + response = self._call_put_request(self.collection_key, taxonomy.pk, ["invalid"]) + assert response.status_code == status.HTTP_400_BAD_REQUEST + @ddt.data( ("superuser", status.HTTP_200_OK), ("staff", status.HTTP_403_FORBIDDEN), @@ -1768,10 +1853,14 @@ class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): @ddt.data( ('staff', 'courseA', 8), ('staff', 'libraryA', 8), + ('staff', 'collection_key', 8), ("content_creatorA", 'courseA', 11, False), ("content_creatorA", 'libraryA', 11, False), + ("content_creatorA", 'collection_key', 11, False), ("library_staffA", 'libraryA', 11, False), # Library users can only view objecttags, not change them? + ("library_staffA", 'collection_key', 11, False), ("library_userA", 'libraryA', 11, False), + ("library_userA", 'collection_key', 11, False), ("instructorA", 'courseA', 11), ("course_instructorA", 'courseA', 11), ("course_staffA", 'courseA', 11), diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 1bc80b7372..b693f7ee0f 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -5,7 +5,7 @@ import tempfile import ddt from django.test.testcases import TestCase from fs.osfs import OSFS -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey, LibraryCollectionKey from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import ObjectTag from organizations.models import Organization @@ -380,6 +380,23 @@ class TestAPIObjectTags(TestGetAllObjectTagsMixin, TestCase): with self.assertNumQueries(31): # TODO why so high? self._test_copy_object_tags(src_key, dst_key, expected_tags) + def test_tag_collection(self): + collection_key = LibraryCollectionKey.from_string("lib-collection:orgA:libX:1") + + api.tag_object( + object_id=str(collection_key), + taxonomy=self.taxonomy_3, + tags=["Tag 3.1"], + ) + + with self.assertNumQueries(1): + object_tags, taxonomies = api.get_all_object_tags(collection_key) + + assert object_tags == {'lib-collection:orgA:libX:1': {3: ['Tag 3.1']}} + assert taxonomies == { + self.taxonomy_3.id: self.taxonomy_3, + } + class TestExportImportTags(TaggedCourseMixin): """ diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index 64fa0d58f0..9ffb090d61 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -5,11 +5,11 @@ from __future__ import annotations from typing import Dict, List, Union -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey, LibraryCollectionKey from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import Taxonomy -ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey] +ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey, LibraryCollectionKey] ContextKey = Union[LibraryLocatorV2, CourseKey] TagValuesByTaxonomyIdDict = Dict[int, List[str]] diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py index 8cc9c9e7f7..39dd925c1a 100644 --- a/openedx/core/djangoapps/content_tagging/utils.py +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -5,7 +5,7 @@ from __future__ import annotations from edx_django_utils.cache import RequestCache from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey, LibraryCollectionKey from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import Taxonomy from organizations.models import Organization @@ -26,8 +26,14 @@ def get_content_key_from_string(key_str: str) -> ContentKey: except InvalidKeyError: try: return UsageKey.from_string(key_str) - except InvalidKeyError as usage_key_error: - raise ValueError("object_id must be a CourseKey, LibraryLocatorV2 or a UsageKey") from usage_key_error + except InvalidKeyError: + try: + return LibraryCollectionKey.from_string(key_str) + except InvalidKeyError as usage_key_error: + raise ValueError( + "object_id must be one of the following " + "keys: CourseKey, LibraryLocatorV2, UsageKey or LibCollectionKey" + ) from usage_key_error def get_context_key_from_key(content_key: ContentKey) -> ContextKey: @@ -38,6 +44,10 @@ def get_context_key_from_key(content_key: ContentKey) -> ContextKey: if isinstance(content_key, (CourseKey, LibraryLocatorV2)): return content_key + # If the content key is a LibraryCollectionKey, return the LibraryLocatorV2 + if isinstance(content_key, LibraryCollectionKey): + return content_key.library_key + # If the content key is a UsageKey, return the context key context_key = content_key.context_key diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 9c85f60fc3..8127bc6180 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -484,7 +484,7 @@ edx-milestones==0.6.0 # via -r requirements/edx/kernel.in edx-name-affirmation==2.4.0 # via -r requirements/edx/kernel.in -edx-opaque-keys[django]==2.10.0 +edx-opaque-keys[django]==2.11.0 # via # -r requirements/edx/kernel.in # -r requirements/edx/paver.txt diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 15e078ae0d..d49e358ea3 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -770,7 +770,7 @@ edx-name-affirmation==2.4.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -edx-opaque-keys[django]==2.10.0 +edx-opaque-keys[django]==2.11.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index defc09a23d..8e06e72e60 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -564,7 +564,7 @@ edx-milestones==0.6.0 # via -r requirements/edx/base.txt edx-name-affirmation==2.4.0 # via -r requirements/edx/base.txt -edx-opaque-keys[django]==2.10.0 +edx-opaque-keys[django]==2.11.0 # via # -r requirements/edx/base.txt # edx-bulk-grades diff --git a/requirements/edx/paver.txt b/requirements/edx/paver.txt index faa0085f16..d86acae05f 100644 --- a/requirements/edx/paver.txt +++ b/requirements/edx/paver.txt @@ -12,7 +12,7 @@ charset-normalizer==2.0.12 # requests dnspython==2.6.1 # via pymongo -edx-opaque-keys==2.10.0 +edx-opaque-keys==2.11.0 # via -r requirements/edx/paver.in idna==3.7 # via requests diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 49be07ecec..548c0c636a 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -590,7 +590,7 @@ edx-milestones==0.6.0 # via -r requirements/edx/base.txt edx-name-affirmation==2.4.0 # via -r requirements/edx/base.txt -edx-opaque-keys[django]==2.10.0 +edx-opaque-keys[django]==2.11.0 # via # -r requirements/edx/base.txt # edx-bulk-grades From e9559bb2565ca239472428f3f3dfb56201344f67 Mon Sep 17 00:00:00 2001 From: Isaac Lee <124631592+ilee2u@users.noreply.github.com> Date: Fri, 13 Sep 2024 12:03:33 -0400 Subject: [PATCH 4/8] temp: pin edx-name-affirmation (#35481) * temp: pin edx-name-affirmation - doing this to make changes to both this repo and name-affirmation without breaking either * fix: compiled requirements * fix: compile requirements again --- requirements/constraints.txt | 4 ++++ requirements/edx/base.txt | 4 +++- requirements/edx/development.txt | 1 + requirements/edx/doc.txt | 4 +++- requirements/edx/testing.txt | 4 +++- 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index c67cd72953..a87d412921 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -142,3 +142,7 @@ django-storages<1.14.4 # We are pinning this until after all the smaller migrations get handled and then we can migrate this all at once. # Ticket to unpin: https://github.com/edx/edx-arch-experiments/issues/760 social-auth-app-django<=5.4.1 + +# Temporary pin as to prevent a new version of edx-name-affirmation from being merged before we modify it to work +# properly along with work in this PR: https://github.com/openedx/edx-platform/pull/35468 +edx-name-affirmation==2.4.0 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index b0b41835da..7f9f822de9 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -483,7 +483,9 @@ edx-i18n-tools==1.5.0 edx-milestones==0.6.0 # via -r requirements/edx/kernel.in edx-name-affirmation==2.4.0 - # via -r requirements/edx/kernel.in + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/kernel.in edx-opaque-keys[django]==2.11.0 # via # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 13c9380ab4..0979f70d50 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -768,6 +768,7 @@ edx-milestones==0.6.0 # -r requirements/edx/testing.txt edx-name-affirmation==2.4.0 # via + # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt edx-opaque-keys[django]==2.11.0 diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 25f81a659e..8b2302ebe3 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -563,7 +563,9 @@ edx-i18n-tools==1.5.0 edx-milestones==0.6.0 # via -r requirements/edx/base.txt edx-name-affirmation==2.4.0 - # via -r requirements/edx/base.txt + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/base.txt edx-opaque-keys[django]==2.11.0 # via # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 29784c29e5..231fe76188 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -589,7 +589,9 @@ edx-lint==5.3.7 edx-milestones==0.6.0 # via -r requirements/edx/base.txt edx-name-affirmation==2.4.0 - # via -r requirements/edx/base.txt + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/base.txt edx-opaque-keys[django]==2.11.0 # via # -r requirements/edx/base.txt From 4e5a3b2db96c70a5cacb287ce754b20bfe3fc45b Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 13 Sep 2024 16:35:25 +0000 Subject: [PATCH 5/8] feat: Upgrade Python dependency openedx-events (#35472) Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: ilee2u <124631592+ilee2u@users.noreply.github.com> From bb6ac5a898d344da78de1bd0acd4dea31308eb89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Sep 2024 15:45:33 -0300 Subject: [PATCH 6/8] feat: new unstable URL to render a v2 library xblock in an iframe (#35473) --- .../content_libraries/xblock_iframe.html | 7 ++- lms/templates/xblock_v2/xblock_iframe.html | 1 + .../core/djangoapps/xblock/rest_api/urls.py | 5 ++ .../core/djangoapps/xblock/rest_api/views.py | 50 ++++++++++++++++++- 4 files changed, 60 insertions(+), 3 deletions(-) create mode 120000 lms/templates/xblock_v2/xblock_iframe.html diff --git a/cms/templates/content_libraries/xblock_iframe.html b/cms/templates/content_libraries/xblock_iframe.html index e8eb4c96ea..b6e455f785 100644 --- a/cms/templates/content_libraries/xblock_iframe.html +++ b/cms/templates/content_libraries/xblock_iframe.html @@ -6,7 +6,12 @@ - + {% if is_development %} + + + {% else %} + + {% endif %} diff --git a/lms/templates/xblock_v2/xblock_iframe.html b/lms/templates/xblock_v2/xblock_iframe.html new file mode 120000 index 0000000000..7264c25346 --- /dev/null +++ b/lms/templates/xblock_v2/xblock_iframe.html @@ -0,0 +1 @@ +../../../cms/templates/content_libraries/xblock_iframe.html \ No newline at end of file diff --git a/openedx/core/djangoapps/xblock/rest_api/urls.py b/openedx/core/djangoapps/xblock/rest_api/urls.py index 034d490aa3..dec2fc562f 100644 --- a/openedx/core/djangoapps/xblock/rest_api/urls.py +++ b/openedx/core/djangoapps/xblock/rest_api/urls.py @@ -29,4 +29,9 @@ urlpatterns = [ ), ])), ])), + path('xblocks/v2//', include([ + # render one of this XBlock's views (e.g. student_view) for embedding in an iframe + # NOTE: this endpoint is **unstable** and subject to changes after Sumac + re_path(r'^embed/(?P[\w\-]+)/$', views.embed_block_view), + ])), ] diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 9972e7463b..7934e24bd2 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -1,12 +1,16 @@ """ Views that implement a RESTful API for interacting with XBlocks. """ +import itertools +import json from common.djangoapps.util.json_request import JsonResponse from corsheaders.signals import check_request_enabled +from django.conf import settings from django.contrib.auth import get_user_model from django.db.transaction import atomic from django.http import Http404 +from django.shortcuts import render from django.utils.translation import gettext as _ from django.views.decorators.clickjacking import xframe_options_exempt from django.views.decorators.csrf import csrf_exempt @@ -21,6 +25,7 @@ from xblock.fields import Scope from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey +import openedx.core.djangoapps.site_configuration.helpers as configuration_helpers from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl from openedx.core.lib.api.view_utils import view_auth_classes from ..api import ( @@ -87,6 +92,47 @@ def render_block_view(request, usage_key_str, view_name): return Response(response_data) +@api_view(['GET']) +@view_auth_classes(is_authenticated=False) +@permission_classes((permissions.AllowAny, )) # Permissions are handled at a lower level, by the learning context +@xframe_options_exempt +def embed_block_view(request, usage_key_str, view_name): + """ + Render the given XBlock in an