diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 1cf529dcf0a3..fdac1c8f1696 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -7,7 +7,8 @@ import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import Exists, OuterRef, Q, QuerySet -from opaque_keys.edx.keys import CourseKey, LearningContextKey +from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization @@ -172,9 +173,16 @@ def tag_content_object( """ if not taxonomy.system_defined: # We require that this taxonomy is linked to the content object's "org" or linked to "all orgs" (None): - org_short_name = object_key.org # type: ignore + if isinstance(object_key, CourseKey) or isinstance(object_key, LibraryLocatorV2): + org_short_name = object_key.org + elif isinstance(object_key, UsageKey): + org_short_name = object_key.context_key.org # type: ignore + else: + raise ValueError(f"Invalid object_key: {type(object_key)} -> {object_key}") + if not taxonomy.taxonomyorg_set.filter(Q(org__short_name=org_short_name) | Q(org=None)).exists(): raise ValueError(f"The specified Taxonomy is not enabled for the content object's org ({org_short_name})") + oel_tagging.tag_object( taxonomy=taxonomy, tags=tags, @@ -185,24 +193,25 @@ def tag_content_object( def get_all_object_tags( - content_key: LearningContextKey, + content_key: LibraryLocatorV2 | CourseKey, ) -> tuple[ObjectTagByObjectIdDict, TaxonomyDict]: """ Returns a tuple with a dictionary of grouped object tags for all blocks and a dictionary of taxonomies. """ - # ToDo: Add support for other content types (like LibraryContent and LibraryBlock) + context_key_str = str(content_key) + # We use a block_id_prefix (i.e. the modified course id) to get the tags for the children of the Content + # (course/library) in a single db query. if isinstance(content_key, CourseKey): - course_key_str = str(content_key) - # We use a block_id_prefix (i.e. the modified course id) to get the tags for the children of the Content - # (course) in a single db query. - block_id_prefix = course_key_str.replace("course-v1:", "block-v1:", 1) + block_id_prefix = context_key_str.replace("course-v1:", "block-v1:", 1) + elif isinstance(content_key, LibraryLocatorV2): + block_id_prefix = context_key_str.replace("lib:", "lb:", 1) else: raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") # There is no API method in oel_tagging.api that does this yet, # so for now we have to build the ORM query directly. - all_object_tags = list(ObjectTag.objects.filter( - Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str), + all_object_tags = list(ContentObjectTag.objects.filter( + Q(object_id__startswith=block_id_prefix) | Q(object_id=content_key), Q(tag__isnull=False, tag__taxonomy__isnull=False), ).select_related("tag__taxonomy")) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py index 14103642d8ec..48cc93c06693 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py @@ -7,9 +7,13 @@ from typing import Iterator from attrs import define -from opaque_keys.edx.keys import CourseKey, LearningContextKey +from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2 +from xblock.core import XBlock -from xmodule.modulestore.django import modulestore +import openedx.core.djangoapps.content_libraries.api as library_api +from openedx.core.djangoapps.content_libraries.api import LibraryXBlockMetadata +from xmodule.modulestore.django import ModuleStore, modulestore from ...types import ObjectTagByObjectIdDict, ObjectTagByTaxonomyIdDict @@ -38,51 +42,127 @@ def iterate_with_level( yield from iterate_with_level(child, level + 1) +def get_course_tagged_object_and_children( + course_key: CourseKey, object_tag_cache: ObjectTagByObjectIdDict +) -> tuple[TaggedContent, list[XBlock]]: + store = modulestore() + + course = store.get_course(course_key) + assert course is not None + course_id = str(course_key) + + tagged_course = TaggedContent( + display_name=course.display_name_with_default, + block_id=course_id, + category=course.category, + object_tags=object_tag_cache.get(course_id, {}), + children=None, + ) + if course.has_children: + children = course.children + else: + children = [] + + return tagged_course, children + + +def get_library_tagged_object_and_children( + library_key: LibraryLocatorV2, object_tag_cache: ObjectTagByObjectIdDict +) -> tuple[TaggedContent, list[LibraryXBlockMetadata]]: + library = library_api.get_library(library_key) + assert library is not None + + library_id = str(library_key) + + tagged_library = TaggedContent( + display_name=library.title, + block_id=library_id, + category='library', + object_tags=object_tag_cache.get(library_id, {}), + children=None, + ) + + children = library_api.get_library_blocks(library_key) + + return tagged_library, children + + +def get_xblock_tagged_object_and_children( + usage_key: UsageKey, object_tag_cache: ObjectTagByObjectIdDict, store +) -> tuple[TaggedContent, list[XBlock]]: + block = store.get_item(usage_key) + block_id = str(usage_key) + tagged_block = TaggedContent( + display_name=block.display_name_with_default, + block_id=block_id, + category=block.category, + object_tags=object_tag_cache.get(block_id, {}), + children=None, + ) + + return tagged_block, block.children if block.has_children else [] + + +def get_library_block_tagged_object( + library_block: LibraryXBlockMetadata, object_tag_cache: ObjectTagByObjectIdDict +) -> tuple[TaggedContent, None]: + block_id = str(library_block.usage_key) + tagged_library_block = TaggedContent( + display_name=library_block.display_name, + block_id=block_id, + category=library_block.usage_key.block_type, + object_tags=object_tag_cache.get(block_id, {}), + children=None, + ) + + return tagged_library_block, None + + def build_object_tree_with_objecttags( - content_key: LearningContextKey, + content_key: LibraryLocatorV2 | CourseKey, object_tag_cache: ObjectTagByObjectIdDict, ) -> TaggedContent: """ Returns the object with the tags associated with it. """ - store = modulestore() - if isinstance(content_key, CourseKey): - course = store.get_course(content_key) - if course is None: - raise ValueError(f"Course not found: {content_key}") + tagged_content, children = get_course_tagged_object_and_children( + content_key, object_tag_cache + ) + elif isinstance(content_key, LibraryLocatorV2): + tagged_content, children = get_library_tagged_object_and_children( + content_key, object_tag_cache + ) else: raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") - display_name = course.display_name_with_default - course_id = str(course.id) + blocks: list[tuple[TaggedContent, list | None]] = [(tagged_content, children)] - tagged_course = TaggedContent( - display_name=display_name, - block_id=course_id, - category=course.category, - object_tags=object_tag_cache.get(str(content_key), {}), - children=None, - ) - - blocks = [(tagged_course, course)] + store = modulestore() while blocks: - tagged_block, xblock = blocks.pop() + tagged_block, children = blocks.pop() tagged_block.children = [] - if xblock.has_children: - for child_id in xblock.children: - child_block = store.get_item(child_id) - tagged_child = TaggedContent( - display_name=child_block.display_name_with_default, - block_id=str(child_id), - category=child_block.category, - object_tags=object_tag_cache.get(str(child_id), {}), - children=None, + if not children: + continue + + for child in children: + child_children: list | None + + if isinstance(child, UsageKey): + tagged_child, child_children = get_xblock_tagged_object_and_children( + child, object_tag_cache, store ) - tagged_block.children.append(tagged_child) + elif isinstance(child, LibraryXBlockMetadata): + tagged_child, child_children = get_library_block_tagged_object( + child, object_tag_cache + ) + else: + raise NotImplementedError(f"Invalid child: {type(child)} -> {child}") + + tagged_block.children.append(tagged_child) - blocks.append((tagged_child, child_block)) + blocks.append((tagged_child, child_children)) - return tagged_course + return tagged_content diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py index 28d75f0fdfb6..e1deccd035a1 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py @@ -3,6 +3,8 @@ """ from unittest.mock import patch +from openedx.core.djangoapps.content_libraries import api as library_api +from openedx.core.lib import blockstore_api from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory @@ -33,7 +35,8 @@ def setUp(self): run="test_run", display_name="Test Course", ) - self.expected_tagged_xblock = TaggedContent( + + self.expected_course_tagged_xblock = TaggedContent( display_name="Test Course", block_id="course-v1:orgA+test_course+test_run", category="course", @@ -61,8 +64,8 @@ def setUp(self): }, ) - assert self.expected_tagged_xblock.children is not None # type guard - self.expected_tagged_xblock.children.append(tagged_sequential) + assert self.expected_course_tagged_xblock.children is not None # type guard + self.expected_course_tagged_xblock.children.append(tagged_sequential) # Untagged blocks sequential2 = BlockFactory.create( @@ -77,8 +80,8 @@ def setUp(self): children=[], object_tags={}, ) - assert self.expected_tagged_xblock.children is not None # type guard - self.expected_tagged_xblock.children.append(untagged_sequential) + assert self.expected_course_tagged_xblock.children is not None # type guard + self.expected_course_tagged_xblock.children.append(untagged_sequential) BlockFactory.create( parent=sequential2, category="vertical", @@ -127,12 +130,12 @@ def setUp(self): assert tagged_sequential.children is not None # type guard tagged_sequential.children.append(untagged_vertical2) - html = BlockFactory.create( + BlockFactory.create( parent=vertical2, category="html", display_name="test html", ) - tagged_text = TaggedContent( + tagged_html = TaggedContent( display_name="test html", block_id="block-v1:orgA+test_course+test_run+type@html+block@test_html", category="html", @@ -142,36 +145,129 @@ def setUp(self): }, ) assert untagged_vertical2.children is not None # type guard - untagged_vertical2.children.append(tagged_text) + untagged_vertical2.children.append(tagged_html) - self.all_object_tags, _ = api.get_all_object_tags(self.course.id) - self.expected_tagged_content_list = [ - (self.expected_tagged_xblock, 0), + self.all_course_object_tags, _ = api.get_all_object_tags(self.course.id) + self.expected_course_tagged_content_list = [ + (self.expected_course_tagged_xblock, 0), (tagged_sequential, 1), (tagged_vertical, 2), (untagged_vertical2, 2), - (tagged_text, 3), + (tagged_html, 3), (untagged_sequential, 1), (untagged_vertical, 2), ] + # Create a library + collection = blockstore_api.create_collection("Content Library Test Collection") + + self.library = library_api.create_library( + collection.uuid, + self.orgA, + f"lib_{self.block_sufix}", + "Test Library", + ) + self.expected_library_tagged_xblock = TaggedContent( + display_name="Test Library", + block_id=f"lib:orgA:lib_{self.block_sufix}", + category="library", + children=[], + object_tags={ + self.taxonomy_2.id: list(self.library_tags), + }, + ) + + library_api.create_library_block( + self.library.key, + "problem", + f"problem1_{self.block_sufix}", + ) + tagged_problem = TaggedContent( + display_name="Blank Problem", + block_id=f"lb:orgA:lib_{self.block_sufix}:problem:problem1_{self.block_sufix}", + category="problem", + children=[], + object_tags={ + self.taxonomy_1.id: list(self.problem1_tags), + }, + ) + + library_api.create_library_block( + self.library.key, + "problem", + f"problem2_{self.block_sufix}", + ) + untagged_problem = TaggedContent( + display_name="Blank Problem", + block_id=f"lb:orgA:lib_{self.block_sufix}:problem:problem2_{self.block_sufix}", + category="problem", + children=[], + object_tags={}, + ) + + library_api.create_library_block( + self.library.key, + "html", + f"html_{self.block_sufix}", + ) + tagged_library_html = TaggedContent( + display_name="Text", + block_id=f"lb:orgA:lib_{self.block_sufix}:html:html_{self.block_sufix}", + category="html", + children=[], + object_tags={ + self.taxonomy_1.id: list(self.library_html_tags1), + self.taxonomy_2.id: list(self.library_html_tags2), + }, + ) + + assert self.expected_library_tagged_xblock.children is not None # type guard + # ToDo: Check the order of the children + self.expected_library_tagged_xblock.children.append(tagged_library_html) + self.expected_library_tagged_xblock.children.append(tagged_problem) + self.expected_library_tagged_xblock.children.append(untagged_problem) + + self.all_library_object_tags, _ = api.get_all_object_tags(self.library.key) + self.expected_library_tagged_content_list = [ + (self.expected_library_tagged_xblock, 0), + (tagged_library_html, 1), + (tagged_problem, 1), + (untagged_problem, 1), + ] + class TestContentTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] """ Test helper functions for exporting tagged content """ - def test_build_object_tree(self) -> None: + def test_build_course_object_tree(self) -> None: """ Test if we can export a course """ with self.assertNumQueries(3): - tagged_xblock = build_object_tree_with_objecttags(self.course.id, self.all_object_tags) + tagged_course = build_object_tree_with_objecttags(self.course.id, self.all_course_object_tags) + + assert tagged_course == self.expected_course_tagged_xblock - assert tagged_xblock == self.expected_tagged_xblock + def test_build_library_object_tree(self) -> None: + """ + Test if we can export a library + """ + with self.assertNumQueries(12): + tagged_library = build_object_tree_with_objecttags(self.library.key, self.all_library_object_tags) + + assert tagged_library == self.expected_library_tagged_xblock + + def test_course_iterate_with_level(self) -> None: + """ + Test if we can iterate over the tagged course in the correct order + """ + tagged_content_list = list(iterate_with_level(self.expected_course_tagged_xblock)) + assert tagged_content_list == self.expected_course_tagged_content_list - def test_iterate_with_level(self) -> None: + def test_library_iterate_with_level(self) -> None: """ - Test if we can iterate over the tagged content in the correct order + Test if we can iterate over the tagged library in the correct order """ - tagged_content_list = list(iterate_with_level(self.expected_tagged_xblock)) - assert tagged_content_list == self.expected_tagged_content_list + tagged_content_list = list(iterate_with_level(self.expected_library_tagged_xblock)) + assert tagged_content_list == self.expected_library_tagged_content_list 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 90ab1906a1f0..6702c9d2191f 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -141,7 +141,6 @@ def orgs(self, request, **_kwargs) -> Response: class ObjectTagOrgView(ObjectTagView): """ View to create and retrieve ObjectTags for a provided Object ID (object_id). - This view extends the ObjectTagView to add Organization filters for the results. Refer to ObjectTagView docstring for usage details. """ diff --git a/openedx/core/djangoapps/content_tagging/tasks.py b/openedx/core/djangoapps/content_tagging/tasks.py index c566a020cbcb..d200c837c035 100644 --- a/openedx/core/djangoapps/content_tagging/tasks.py +++ b/openedx/core/djangoapps/content_tagging/tasks.py @@ -10,7 +10,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from edx_django_utils.monitoring import set_code_owner_attribute -from opaque_keys.edx.keys import LearningContextKey, UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryUsageLocatorV2 from openedx_tagging.core.tagging.models import Taxonomy @@ -65,7 +65,7 @@ def update_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: - course_key = LearningContextKey.from_string(course_key_str) + course_key = CourseKey.from_string(course_key_str) log.info("Updating tags for Course with id: %s", course_key) @@ -90,7 +90,7 @@ def delete_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: - course_key = LearningContextKey.from_string(course_key_str) + course_key = CourseKey.from_string(course_key_str) log.info("Deleting tags for Course with id: %s", course_key) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 76502163f2f4..d685a321d35e 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,7 +1,10 @@ """Tests for the Tagging models""" +import time + import ddt from django.test.testcases import TestCase from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import Tag from organizations.models import Organization @@ -334,7 +337,7 @@ def setUp(self): _name="deleted taxonomy", ) - self.expected_objecttags = { + self.expected_course_objecttags = { "course-v1:orgA+test_course+test_run": { self.taxonomy_1.id: list(self.course_tags), }, @@ -350,22 +353,91 @@ def setUp(self): }, } + # Library tags and library contents need a unique block_id that is persisted along test runs + self.block_sufix = str(round(time.time() * 1000)) + + self.library_tags = api.tag_content_object( + object_key=LibraryLocatorV2.from_string(f"lib:orgA:lib_{self.block_sufix}"), + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + + self.problem1_tags = api.tag_content_object( + object_key=UsageKey.from_string(f"lb:orgA:lib_{self.block_sufix}:problem:problem1_{self.block_sufix}"), + taxonomy=self.taxonomy_1, + tags=['Tag 1.1'], + ) + + self.library_html_tags1 = api.tag_content_object( + object_key=UsageKey.from_string(f"lb:orgA:lib_{self.block_sufix}:html:html_{self.block_sufix}"), + taxonomy=self.taxonomy_1, + tags=['Tag 1.2'], + ) + + self.library_html_tags2 = api.tag_content_object( + object_key=UsageKey.from_string(f"lb:orgA:lib_{self.block_sufix}:html:html_{self.block_sufix}"), + taxonomy=self.taxonomy_2, + tags=['Tag 2.2'], + ) + + # Create "deleted" object tags, which will be omitted from the results. + for object_id in ( + f"lib:orgA:lib_{self.block_sufix}", + f"lb:orgA:lib_{self.block_sufix}:problem:problem1_{self.block_sufix}", + f"lb:orgA:lib_{self.block_sufix}:html:html_{self.block_sufix}", + ): + ContentObjectTag.objects.create( + object_id=str(object_id), + taxonomy=None, + tag=None, + _value="deleted tag", + _name="deleted taxonomy", + ) + + self.expected_library_objecttags = { + f"lib:orgA:lib_{self.block_sufix}": { + self.taxonomy_2.id: list(self.library_tags), + }, + f"lb:orgA:lib_{self.block_sufix}:problem:problem1_{self.block_sufix}": { + self.taxonomy_1.id: list(self.problem1_tags), + }, + f"lb:orgA:lib_{self.block_sufix}:html:html_{self.block_sufix}": { + self.taxonomy_1.id: list(self.library_html_tags1), + self.taxonomy_2.id: list(self.library_html_tags2), + }, + } + class TestGetAllObjectTags(TestGetAllObjectTagsMixin, TestCase): """ Test get_all_object_tags api function """ - def test_get_all_object_tags(self): + def test_get_course_object_tags(self): """ - Test the get_all_object_tags function + Test the get_all_object_tags function using a course """ with self.assertNumQueries(1): object_tags, taxonomies = api.get_all_object_tags( CourseKey.from_string("course-v1:orgA+test_course+test_run") ) - assert object_tags == self.expected_objecttags + assert object_tags == self.expected_course_objecttags + assert taxonomies == { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } + + def test_get_library_object_tags(self): + """ + Test the get_all_object_tags function using a library + """ + with self.assertNumQueries(1): + object_tags, taxonomies = api.get_all_object_tags( + LibraryLocatorV2.from_string(f"lib:orgA:lib_{self.block_sufix}") + ) + + assert object_tags == self.expected_library_objecttags assert taxonomies == { self.taxonomy_1.id: self.taxonomy_1, self.taxonomy_2.id: self.taxonomy_2, diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index aa9a2179d612..3c186df187f2 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -5,10 +5,11 @@ from typing import Dict, List, Union -from opaque_keys.edx.keys import LearningContextKey, UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy -ContentKey = Union[LearningContextKey, UsageKey] +ContentKey = Union[CourseKey, LibraryLocatorV2, UsageKey] ObjectTagByTaxonomyIdDict = Dict[int, List[ObjectTag]] ObjectTagByObjectIdDict = Dict[str, ObjectTagByTaxonomyIdDict]