diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 4f62469220a5..8ebb15561d91 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 +from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization @@ -130,24 +131,28 @@ def get_unassigned_taxonomies(enabled=True) -> QuerySet: def get_all_object_tags( - content_key: LearningContextKey, + content_key: LibraryLocatorV2 | CourseKey, ) -> tuple[ObjectTagByObjectIdDict, TaxonomyDict]: """ + Get all the object tags applied to components in the given course/library. + + Includes any tags applied to the course/library as a whole. 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), + 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..e99017f432e8 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 @@ -4,11 +4,15 @@ from __future__ import annotations -from typing import Iterator +from typing import Any, Callable, Iterator, Union 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 +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 from ...types import ObjectTagByObjectIdDict, ObjectTagByTaxonomyIdDict @@ -38,51 +42,137 @@ def iterate_with_level( yield from iterate_with_level(child, level + 1) -def build_object_tree_with_objecttags( - content_key: LearningContextKey, - object_tag_cache: ObjectTagByObjectIdDict, -) -> TaggedContent: +def _get_course_tagged_object_and_children( + course_key: CourseKey, object_tag_cache: ObjectTagByObjectIdDict +) -> tuple[TaggedContent, list[XBlock]]: """ - Returns the object with the tags associated with it. + Returns a TaggedContent with course metadata with its tags, and its children. """ 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}") - else: - raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") + course = store.get_course(course_key) + if course is None: + raise ValueError(f"Course not found: {course_key}") - display_name = course.display_name_with_default - course_id = str(course.id) + course_id = str(course_key) tagged_course = TaggedContent( - display_name=display_name, + display_name=course.display_name_with_default, block_id=course_id, category=course.category, - object_tags=object_tag_cache.get(str(content_key), {}), + object_tags=object_tag_cache.get(course_id, {}), + children=None, + ) + + return tagged_course, course.children if course.has_children else [] + + +def _get_library_tagged_object_and_children( + library_key: LibraryLocatorV2, object_tag_cache: ObjectTagByObjectIdDict +) -> tuple[TaggedContent, list[LibraryXBlockMetadata]]: + """ + Returns a TaggedContent with library metadata with its tags, and its children. + """ + library = library_api.get_library(library_key) + if library is None: + raise ValueError(f"Library not found: {library_key}") + + 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, + ) + + library_components = library_api.get_library_components(library_key) + children = [ + LibraryXBlockMetadata.from_component(library_key, component) + for component in library_components + ] + + return tagged_library, children + + +def _get_xblock_tagged_object_and_children( + usage_key: UsageKey, object_tag_cache: ObjectTagByObjectIdDict +) -> tuple[TaggedContent, list[XBlock]]: + """ + Returns a TaggedContent with xblock metadata with its tags, and its children. + """ + store = modulestore() + 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, ) - blocks = [(tagged_course, course)] + 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]: + """ + Returns a TaggedContent with library content block metadata and its tags, + and 'None' as children. + """ + 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: LibraryLocatorV2 | CourseKey, + object_tag_cache: ObjectTagByObjectIdDict, +) -> TaggedContent: + """ + Returns the object with the tags associated with it. + """ + get_tagged_children: Union[ + # _get_course_tagged_object_and_children type + Callable[[LibraryXBlockMetadata, dict[str, dict[int, list[Any]]]], tuple[TaggedContent, None]], + # _get_library_block_tagged_object type + Callable[[UsageKey, dict[str, dict[int, list[Any]]]], tuple[TaggedContent, list[Any]]] + ] + if isinstance(content_key, CourseKey): + tagged_content, children = _get_course_tagged_object_and_children( + content_key, object_tag_cache + ) + get_tagged_children = _get_xblock_tagged_object_and_children + elif isinstance(content_key, LibraryLocatorV2): + tagged_content, children = _get_library_tagged_object_and_children( + content_key, object_tag_cache + ) + get_tagged_children = _get_library_block_tagged_object + else: + raise ValueError(f"Invalid content_key: {type(content_key)} -> {content_key}") + + blocks: list[tuple[TaggedContent, list | None]] = [(tagged_content, children)] while blocks: - tagged_block, xblock = blocks.pop() + tagged_block, 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, - ) - tagged_block.children.append(tagged_child) - - blocks.append((tagged_child, child_block)) - - return tagged_course + if not block_children: + continue + + for child in block_children: + tagged_child, child_children = get_tagged_children(child, object_tag_cache) + tagged_block.children.append(tagged_child) + blocks.append((tagged_child, child_children)) + + 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..61132a69a227 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,7 @@ """ from unittest.mock import patch +from openedx.core.djangoapps.content_libraries import api as library_api from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory @@ -33,7 +34,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 +63,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 +79,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 +129,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 +144,126 @@ 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 + self.library = library_api.create_library( + self.orgA, + f"lib_{self.block_suffix}", + "Test Library", + ) + self.expected_library_tagged_xblock = TaggedContent( + display_name="Test Library", + block_id=f"lib:orgA:lib_{self.block_suffix}", + 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_suffix}", + ) + tagged_problem = TaggedContent( + display_name="Blank Problem", + block_id=f"lb:orgA:lib_{self.block_suffix}:problem:problem1_{self.block_suffix}", + 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_suffix}", + ) + untagged_problem = TaggedContent( + display_name="Blank Problem", + block_id=f"lb:orgA:lib_{self.block_suffix}:problem:problem2_{self.block_suffix}", + category="problem", + children=[], + object_tags={}, + ) + + library_api.create_library_block( + self.library.key, + "html", + f"html_{self.block_suffix}", + ) + tagged_library_html = TaggedContent( + display_name="Text", + block_id=f"lb:orgA:lib_{self.block_suffix}:html:html_{self.block_suffix}", + 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 + # The children are sorted by add order + self.expected_library_tagged_xblock.children.append(tagged_problem) + self.expected_library_tagged_xblock.children.append(untagged_problem) + self.expected_library_tagged_xblock.children.append(tagged_library_html) + + 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_problem, 1), + (untagged_problem, 1), + (tagged_library_html, 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(8): + 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 cea834b38d04..d9bf18cd88e0 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -8,8 +8,7 @@ from django.db.models import Count from django.http import StreamingHttpResponse -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import UsageKey from openedx_tagging.core.tagging import rules as oel_tagging_rules from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status @@ -29,6 +28,7 @@ set_taxonomy_orgs ) from ...rules import get_admin_orgs +from ...utils import get_content_key_from_string from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend from .objecttag_export_helpers import build_object_tree_with_objecttags, iterate_with_level from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer @@ -202,12 +202,12 @@ def _generate_csv_rows() -> Iterator[str]: object_id: str = kwargs.get('context_id', None) try: - content_key = CourseKey.from_string(object_id) - except InvalidKeyError as e: - raise ValidationError("context_id is not a valid course key.") from e + content_key = get_content_key_from_string(object_id) - # Check if the user has permission to view object tags for this object_id - try: + if isinstance(content_key, UsageKey): + raise ValidationError("The object_id must be a CourseKey or a LibraryLocatorV2.") + + # Check if the user has permission to view object tags for this object_id if not self.request.user.has_perm( "oel_tagging.view_objecttag", # The obj arg expects a model, but we are passing an object @@ -216,11 +216,13 @@ def _generate_csv_rows() -> Iterator[str]: raise PermissionDenied( "You do not have permission to view object tags for this object_id." ) + + all_object_tags, taxonomies = get_all_object_tags(content_key) + tagged_content = build_object_tree_with_objecttags(content_key, all_object_tags) + except ValueError as e: raise ValidationError from e - all_object_tags, taxonomies = get_all_object_tags(content_key) - tagged_content = build_object_tree_with_objecttags(content_key, all_object_tags) pseudo_buffer = Echo() return StreamingHttpResponse( diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 229b8d62bc6a..31e6802393c0 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,7 +1,11 @@ """Tests for the Tagging models""" +import time + import ddt from django.test.testcases import TestCase + from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.core.tagging.models import ObjectTag, Tag from organizations.models import Organization @@ -323,7 +327,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), }, @@ -339,22 +343,103 @@ def setUp(self): }, } + # Library tags and library contents need a unique block_id that is persisted along test runs + self.block_suffix = str(round(time.time() * 1000)) + + api.tag_object( + object_id=f"lib:orgA:lib_{self.block_suffix}", + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + self.library_tags = api.get_object_tags(f"lib:orgA:lib_{self.block_suffix}") + + api.tag_object( + object_id=f"lb:orgA:lib_{self.block_suffix}:problem:problem1_{self.block_suffix}", + taxonomy=self.taxonomy_1, + tags=['Tag 1.1'], + ) + self.problem1_tags = api.get_object_tags( + f"lb:orgA:lib_{self.block_suffix}:problem:problem1_{self.block_suffix}" + ) + + api.tag_object( + object_id=f"lb:orgA:lib_{self.block_suffix}:html:html_{self.block_suffix}", + taxonomy=self.taxonomy_1, + tags=['Tag 1.2'], + ) + self.library_html_tags1 = api.get_object_tags( + object_id=f"lb:orgA:lib_{self.block_suffix}:html:html_{self.block_suffix}", + taxonomy_id=self.taxonomy_1.id, + ) + + api.tag_object( + object_id=f"lb:orgA:lib_{self.block_suffix}:html:html_{self.block_suffix}", + taxonomy=self.taxonomy_2, + tags=['Tag 2.2'], + ) + self.library_html_tags2 = api.get_object_tags( + object_id=f"lb:orgA:lib_{self.block_suffix}:html:html_{self.block_suffix}", + taxonomy_id=self.taxonomy_2.id, + ) + + # Create "deleted" object tags, which will be omitted from the results. + for object_id in ( + f"lib:orgA:lib_{self.block_suffix}", + f"lb:orgA:lib_{self.block_suffix}:problem:problem1_{self.block_suffix}", + f"lb:orgA:lib_{self.block_suffix}:html:html_{self.block_suffix}", + ): + ObjectTag.objects.create( + object_id=object_id, + taxonomy=None, + tag=None, + _value="deleted tag", + _name="deleted taxonomy", + ) + + self.expected_library_objecttags = { + f"lib:orgA:lib_{self.block_suffix}": { + self.taxonomy_2.id: list(self.library_tags), + }, + f"lb:orgA:lib_{self.block_suffix}:problem:problem1_{self.block_suffix}": { + self.taxonomy_1.id: list(self.problem1_tags), + }, + f"lb:orgA:lib_{self.block_suffix}:html:html_{self.block_suffix}": { + 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_suffix}") + ) + + assert object_tags == self.expected_library_objecttags assert taxonomies == { self.taxonomy_1.id: self.taxonomy_1, self.taxonomy_2.id: self.taxonomy_2,