From 52bc665a021a7c87925ad665b631a75f011d95a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 25 Jan 2024 16:41:41 -0300 Subject: [PATCH 01/50] feat: export tagged course as csv --- .../core/djangoapps/content_tagging/api.py | 159 +++++++++++++++++- .../rest_api/v1/serializers.py | 7 + .../rest_api/v1/tests/test_views.py | 66 +++++++- .../content_tagging/rest_api/v1/views.py | 62 ++++++- .../core/djangoapps/content_tagging/rules.py | 5 +- .../content_tagging/tests/test_api.py | 104 ++++++++++++ 6 files changed, 394 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 70aa7e2150da..f5e6754091f0 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -2,14 +2,26 @@ Content Tagging APIs """ from __future__ import annotations +from typing import TYPE_CHECKING + +import csv +from itertools import groupby +from io import StringIO import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import Q, QuerySet, Exists, OuterRef -from openedx_tagging.core.tagging.models import Taxonomy -from organizations.models import Organization +from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx_tagging.core.tagging.models import ObjectTag + +from xmodule.modulestore.django import modulestore from .models import ContentObjectTag, TaxonomyOrg -from .types import ContentKey + +if TYPE_CHECKING: + from openedx_tagging.core.tagging.models import Taxonomy + from xblock.runtime import Runtime + from organizations.models import Organization + from .types import ContentKey def create_taxonomy( @@ -141,6 +153,8 @@ def get_content_tags( ) +# FixMe: The following method (tag_content_object) is only used in tasks.py for auto-tagging. To tag object we are +# using oel_tagging.tag_object and checking permissions via rule overrides. def tag_content_object( object_key: ContentKey, taxonomy: Taxonomy, @@ -175,6 +189,145 @@ def tag_content_object( return get_content_tags(object_key, taxonomy_id=taxonomy.id) +def export_content_object_children_tags( + course_key_str: str, +) -> str: + """ + Generates a CSV file with the tags for all the children of a course. + """ + def _get_course_children_tags(course_key: CourseKey) -> tuple[dict[str, dict[int, list[str]]], dict[int, str]]: + """ + Returns a tuple with a dictionary of object tags for all blocks of a course, + grouping by the block id and taxonomy id; and a dictionary of taxonomy ids and names. + + I.e. + // result + { + // Block with id block-v1:edX+DemoX+Demo_Course+type@chapter+block@chapter + "block-v1:edX+DemoX+Demo_Course+type@chapter+block@chapter": { + // ObjectTags from Taxonomy with id 1 + "1": ( + "Tag1", + "Tag2", + ... + ), + // ObjectTags from Taxonomy with id 2 + "2": ( + "Tag3", + ... + ), + ... + }, + // Block with id block-v1:edX+DemoX+Demo_Course+type@sequential+block@sequential + "block-v1:edX+DemoX+Demo_Course+type@sequential+block@sequential": { + // ObjectTags from Taxonomy with id 1 + "1": ( + "Tag2", + ... + ), + ... + }, + } + + // taxonomies + { + "1": "Taxonomy A", + "2": "Taxonomy B", + ... + } + """ + block_id_prefix = str(course_key).replace("course-v1:", "block-v1:", 1) + block_tags_records = ObjectTag.objects.filter(object_id__startswith=block_id_prefix).all() + + result: dict[str, dict[int, list[str]]] = {} + taxonomies: dict[int, str] = {} + + for object_id, block_tags in groupby(block_tags_records, lambda x: x.object_id): + result[object_id] = {} + for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): + object_tag_list = list(taxonomy_tags) + result[object_id][taxonomy_id] = [ + # If the tag is not found (deleted or freeText), use the objecttag._name instead + objecttag.tag.value if objecttag.tag else objecttag.name + for objecttag in object_tag_list + ] + + if taxonomy_id not in taxonomies: + taxonomies[taxonomy_id] = object_tag_list[0].tag.taxonomy.name + + return result, taxonomies + + def _generate_csv( + header: dict[str, str], + blocks: list[tuple[int, UsageKey]], + tags: dict[str, dict[int, list[str]]], + taxonomies: dict[int, str], + runtime: Runtime, + ) -> str: + """ + Receives the blocks, tags and taxonomies and returns a CSV string + """ + + with StringIO() as csv_buffer: + csv_writer = csv.DictWriter(csv_buffer, fieldnames=header.keys()) + csv_writer.writerow(header) + + # Iterate over the blocks stack and write the block rows + while blocks: + level, block_id = blocks.pop() + # ToDo: fix block typing + block = runtime.get_block(block_id) + + block_data = { + "name": level * " " + block.display_name_with_default, + "type": block.category, + "id": block_id + } + + block_id_str = str(block_id) + + # Add the tags for each taxonomy + for taxonomy_id in taxonomies: + if block_id_str in tags and taxonomy_id in tags[block_id_str]: + block_data[f"taxonomy_{taxonomy_id}"] = ", ".join(tags[block_id_str][taxonomy_id]) + + csv_writer.writerow(block_data) + + # Add children to the stack + if block.has_children: + for child_id in block.children: + blocks.append((level + 1, child_id)) + + return csv_buffer.getvalue() + + store = modulestore() + course_key = CourseKey.from_string(course_key_str) + if not course_key.is_course: + raise ValueError(f"Invalid course key {course_key_str}") + + # ToDo: fix course typing + course = store.get_course(course_key) + if course is None: + raise ValueError(f"Course {course_key} not found") + + tags, taxonomies = _get_course_children_tags(course_key) + + blocks = [] + # Add children to the stack + if course.has_children: + for child_id in course.children: + blocks.append((0, child_id)) + + header = {"name": "Name", "type": "Type", "id": "ID"} + + # Prepare the header for the taxonomies + # We are using the taxonomy id as the field name to avoid collisions + for taxonomy_id, name in taxonomies.items(): + header[f"taxonomy_{taxonomy_id}"] = name + + return _generate_csv(header, blocks, tags, taxonomies, course.runtime) + + # Expose the oel_tagging APIs get_taxonomy = oel_tagging.get_taxonomy diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index 12433f8a381b..a71e24f3fde9 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -99,3 +99,10 @@ class Meta: model = TaxonomySerializer.Meta.model fields = TaxonomySerializer.Meta.fields + ["orgs", "all_orgs"] read_only_fields = ["orgs", "all_orgs"] + + +class ContentObjectChildrenTagsExportQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Serializer for the query params for the export objecttags GET view + """ + download = serializers.BooleanField(required=False, default=False) 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 20b1deb661b7..fbff9879d5e7 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 @@ -39,12 +39,15 @@ from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.lib import blockstore_api +from ....tests.test_api import TaggedCourseMixin + User = get_user_model() TAXONOMY_ORG_LIST_URL = "/api/content_tagging/v1/taxonomies/" TAXONOMY_ORG_DETAIL_URL = "/api/content_tagging/v1/taxonomies/{pk}/" TAXONOMY_ORG_UPDATE_ORG_URL = "/api/content_tagging/v1/taxonomies/{pk}/orgs/" OBJECT_TAG_UPDATE_URL = "/api/content_tagging/v1/object_tags/{object_id}/?taxonomy={taxonomy_id}" +OBJECT_TAGS_EXPORT_URL = "/api/content_tagging/v1/object_tags/{object_id}/export/" OBJECT_TAGS_URL = "/api/content_tagging/v1/object_tags/{object_id}/" TAXONOMY_TEMPLATE_URL = "/api/content_tagging/v1/taxonomies/import/{filename}" TAXONOMY_CREATE_IMPORT_URL = "/api/content_tagging/v1/taxonomies/import/" @@ -1624,6 +1627,63 @@ def test_object_tags_query_count(self): assert response.data[object_id]["taxonomies"][0]["tags"] == expected_tags +@skip_unless_cms +@ddt.ddt +class TestContentObjectChildrenExportView(TaggedCourseMixin, APITestCase): # type: ignore[misc] + """ + Tests exporting course children with tags + """ + def setUp(self): + super().setUp() + self.user = User.objects.create( + username="user", + email="user@example.com", + ) + self.staff = User.objects.create( + username="staff", + email="staff@example.com", + is_staff=True, + ) + + self.staffA = User.objects.create( + username="staffA", + email="userA@example.com", + ) + update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) + + @ddt.data( + "staff", + "staffA", + ) + def test_export_course(self, user_attr) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) + + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + response = self.client.get(url) + assert response.status_code == status.HTTP_200_OK + assert response.headers['Content-Type'] == 'text' + assert int(response.headers['Content-Length']) > 0 + assert response.content == self.expected_csv.encode("utf-8") + + def test_export_course_anoymous_unauthorized(self) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) + response = self.client.get(url) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_export_course_user_forbidden(self) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) + self.client.force_authenticate(user=self.user) + response = self.client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_export_course_invalid_id(self) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id="invalid") + self.client.force_authenticate(user=self.staff) + response = self.client.get(url) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @skip_unless_cms @ddt.ddt class TestDownloadTemplateView(APITestCase): @@ -1635,7 +1695,7 @@ class TestDownloadTemplateView(APITestCase): ("template.json", "application/json"), ) @ddt.unpack - def test_download(self, filename, content_type): + def test_download(self, filename, content_type) -> None: url = TAXONOMY_TEMPLATE_URL.format(filename=filename) response = self.client.get(url) assert response.status_code == status.HTTP_200_OK @@ -1643,12 +1703,12 @@ def test_download(self, filename, content_type): assert response.headers['Content-Disposition'] == f'attachment; filename="{filename}"' assert int(response.headers['Content-Length']) > 0 - def test_download_not_found(self): + def test_download_not_found(self) -> None: url = TAXONOMY_TEMPLATE_URL.format(filename="template.txt") response = self.client.get(url) assert response.status_code == status.HTTP_404_NOT_FOUND - def test_download_method_not_allowed(self): + def test_download_method_not_allowed(self) -> None: url = TAXONOMY_TEMPLATE_URL.format(filename="template.txt") response = self.client.post(url) assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED 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 151bc09f5d76..f54efa526376 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -1,7 +1,10 @@ """ Tagging Org API Views """ +from django.db.models.query import QuerySet +from django.http import HttpResponse from openedx_tagging.core.tagging import rules as oel_tagging_rules +from openedx_tagging.core.tagging.models import ObjectTag from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status from rest_framework.decorators import action @@ -11,6 +14,7 @@ from ...api import ( create_taxonomy, + export_content_object_children_tags, get_taxonomy, get_taxonomies, get_taxonomies_for_org, @@ -18,7 +22,12 @@ set_taxonomy_orgs, ) from ...rules import get_admin_orgs -from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer +from .serializers import ( + ContentObjectChildrenTagsExportQueryParamsSerializer, + TaxonomyOrgListQueryParamsSerializer, + TaxonomyOrgSerializer, + TaxonomyUpdateOrgBodySerializer, +) from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend @@ -130,8 +139,57 @@ 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. + This view extends the ObjectTagView to add Organization filters for the results and + new actions like: export. Refer to ObjectTagView docstring for usage details. """ filter_backends = [ObjectTagTaxonomyOrgFilterBackend] + + def get_queryset(self): + if self.action == "retrieve": + return super().get_queryset() + + # For other actions, return a dummy queryset only for permission checking + dummy_queryset = QuerySet(model=ObjectTag) + + return dummy_queryset + + @action(detail=True, url_path="export", methods=["get"]) + def export_children_object_tags(self, request: Request, **kwargs) -> HttpResponse: + """ + Export all the object tags for the given object_id children. + """ + object_id: str = kwargs.get('object_id', None) + + query_params = ContentObjectChildrenTagsExportQueryParamsSerializer( + data=request.query_params.dict() + ) + query_params.is_valid(raise_exception=True) + + # Check if the user has permission to view object tags for this object_id + try: + if not self.request.user.has_perm( + "oel_tagging.view_objecttag", + # The obj arg expects a model, but we are passing an object + oel_tagging_rules.ObjectTagPermissionItem(taxonomy=None, object_id=object_id), # type: ignore[arg-type] + ): + raise PermissionDenied( + "You do not have permission to view object tags for this object_id." + ) + except ValueError as e: + raise ValidationError from e + + if query_params.data.get("download"): + content_type = "text/csv" + else: + content_type = "text" + + tags = export_content_object_children_tags(object_id) + + if query_params.data.get("download"): + response = HttpResponse(tags.encode('utf-8'), content_type=content_type) + response["Content-Disposition"] = f'attachment; filename="{object_id}_tags.csv"' + return response + + return HttpResponse(tags, content_type=content_type) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index af6bdbeb9435..dae691c4e6a7 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -257,7 +257,10 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: raise ValueError("object_id must be from a block or a course") course_key = usage_key.course_key except InvalidKeyError: - course_key = CourseKey.from_string(object_id) + try: + course_key = CourseKey.from_string(object_id) + except InvalidKeyError as e: + raise ValueError("object_id must be from a block or a course") from e return has_studio_read_access(user, 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 9a297be968b1..832fa38d89fb 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -4,6 +4,10 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import Tag from organizations.models import Organization +from unittest.mock import patch + +from common.djangoapps.student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE from .. import api @@ -248,3 +252,103 @@ def test_cannot_tag_across_orgs(self): taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.value], ) + + +class TaggedCourseMixin(ModuleStoreTestCase): + """ + Mixin with a course structure and taxonomies + """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + super().setUp() + # Create user + self.user = UserFactory.create() + self.user_id = self.user.id + + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") + self.taxonomy_1 = api.create_taxonomy(name="Taxonomy 1") + api.set_taxonomy_orgs(self.taxonomy_1, all_orgs=True) + Tag.objects.create( + taxonomy=self.taxonomy_1, + value="Tag 1.1", + ) + Tag.objects.create( + taxonomy=self.taxonomy_1, + value="Tag 1.2", + ) + + self.taxonomy_2 = api.create_taxonomy(name="Taxonomy 2") + api.set_taxonomy_orgs(self.taxonomy_2, all_orgs=True) + + Tag.objects.create( + taxonomy=self.taxonomy_2, + value="Tag 2.1", + ) + Tag.objects.create( + taxonomy=self.taxonomy_2, + value="Tag 2.2", + ) + + self.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) + self.addCleanup(self.patcher.stop) + self.patcher.start() + + # Create course + self.course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + ) + + # Create XBlocks + sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical1") + vertical2 = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical2") + text = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") + + # Tag blocks + api.tag_content_object( + object_key=sequential.location, + taxonomy=self.taxonomy_1, + tags=['Tag 1.1', 'Tag 1.2'], + ) + api.tag_content_object( + object_key=sequential.location, + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + api.tag_content_object( + object_key=vertical.location, + taxonomy=self.taxonomy_2, + tags=['Tag 2.2'], + ) + api.tag_content_object( + object_key=text.location, + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + + self.expected_csv = ( + "Name,Type,ID,Taxonomy 1,Taxonomy 2\r\n" + 'test sequential,sequential,block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential,' + '"Tag 1.1, Tag 1.2",Tag 2.1\r\n' + ' test vertical2,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical2,,\r\n' + ' Text,html,block-v1:orgA+test_course+test_run+type@html+block@test_html,,Tag 2.1\r\n' + ' test vertical1,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1,' + ',Tag 2.2\r\n' + ) + + +class TestContetTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] + """ + Test exporting content objects + """ + + def test_export_tagged_course_children(self): + """ + Test if we can export a course with tagged children + """ + result = api.export_content_object_children_tags(str(self.course.id)) + assert result == self.expected_csv From f2759087deba4338799276c4950b68e02802ed42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 25 Jan 2024 16:58:25 -0300 Subject: [PATCH 02/50] docs: add comment --- openedx/core/djangoapps/content_tagging/api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index f5e6754091f0..87bbaf938c28 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -322,6 +322,8 @@ def _generate_csv( # Prepare the header for the taxonomies # We are using the taxonomy id as the field name to avoid collisions + # ToDo: Change name -> export_id after done: + # - https://github.com/openedx/modular-learning/issues/183 for taxonomy_id, name in taxonomies.items(): header[f"taxonomy_{taxonomy_id}"] = name From a870dfd7104b4d58b7cda1c8e61bdccfbbe8d0ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 26 Jan 2024 10:47:43 -0300 Subject: [PATCH 03/50] fix: add select_related to ObjectTag query --- openedx/core/djangoapps/content_tagging/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 87bbaf938c28..87a7086311a9 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -237,7 +237,8 @@ def _get_course_children_tags(course_key: CourseKey) -> tuple[dict[str, dict[int } """ block_id_prefix = str(course_key).replace("course-v1:", "block-v1:", 1) - block_tags_records = ObjectTag.objects.filter(object_id__startswith=block_id_prefix).all() + block_tags_records = ObjectTag.objects.filter(object_id__startswith=block_id_prefix) \ + .select_related("tag__taxonomy").all() result: dict[str, dict[int, list[str]]] = {} taxonomies: dict[int, str] = {} From 4dd027aec208f1eb9a44ce4da7c43373725087c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 26 Jan 2024 10:48:53 -0300 Subject: [PATCH 04/50] fix: always use objecttag.value --- openedx/core/djangoapps/content_tagging/api.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 87a7086311a9..907203b62225 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -248,8 +248,7 @@ def _get_course_children_tags(course_key: CourseKey) -> tuple[dict[str, dict[int for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): object_tag_list = list(taxonomy_tags) result[object_id][taxonomy_id] = [ - # If the tag is not found (deleted or freeText), use the objecttag._name instead - objecttag.tag.value if objecttag.tag else objecttag.name + objecttag.value for objecttag in object_tag_list ] From 5fb03aa21c227ee2262a29fc4329c99f3f43b6a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 26 Jan 2024 10:55:17 -0300 Subject: [PATCH 05/50] docs: change comment position --- openedx/core/djangoapps/content_tagging/api.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 907203b62225..0dc2ff012da3 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -253,6 +253,8 @@ def _get_course_children_tags(course_key: CourseKey) -> tuple[dict[str, dict[int ] if taxonomy_id not in taxonomies: + # ToDo: Change name -> export_id after done: + # - https://github.com/openedx/modular-learning/issues/183 taxonomies[taxonomy_id] = object_tag_list[0].tag.taxonomy.name return result, taxonomies @@ -321,9 +323,6 @@ def _generate_csv( header = {"name": "Name", "type": "Type", "id": "ID"} # Prepare the header for the taxonomies - # We are using the taxonomy id as the field name to avoid collisions - # ToDo: Change name -> export_id after done: - # - https://github.com/openedx/modular-learning/issues/183 for taxonomy_id, name in taxonomies.items(): header[f"taxonomy_{taxonomy_id}"] = name From 8f1238e56605ce3b19402b2b5c692ac472bc1f57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 26 Jan 2024 10:58:46 -0300 Subject: [PATCH 06/50] refactor: rename serializer to a more sane name --- .../djangoapps/content_tagging/rest_api/v1/serializers.py | 2 +- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index a71e24f3fde9..6e0fe5bbc4f2 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -101,7 +101,7 @@ class Meta: read_only_fields = ["orgs", "all_orgs"] -class ContentObjectChildrenTagsExportQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method +class ExportContentTagsQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer for the query params for the export objecttags GET view """ 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 f54efa526376..eabc39842f29 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -23,7 +23,7 @@ ) from ...rules import get_admin_orgs from .serializers import ( - ContentObjectChildrenTagsExportQueryParamsSerializer, + ExportContentTagsQueryParamsSerializer, TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer, @@ -162,7 +162,7 @@ def export_children_object_tags(self, request: Request, **kwargs) -> HttpRespons """ object_id: str = kwargs.get('object_id', None) - query_params = ContentObjectChildrenTagsExportQueryParamsSerializer( + query_params = ExportContentTagsQueryParamsSerializer( data=request.query_params.dict() ) query_params.is_valid(raise_exception=True) From bdedf93e96091f38f68e7fb35c199c3894e07ffc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 26 Jan 2024 11:13:51 -0300 Subject: [PATCH 07/50] refactor: remove download param --- .../rest_api/v1/serializers.py | 7 ------- .../rest_api/v1/tests/test_views.py | 2 +- .../content_tagging/rest_api/v1/views.py | 20 +++---------------- 3 files changed, 4 insertions(+), 25 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index 6e0fe5bbc4f2..12433f8a381b 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -99,10 +99,3 @@ class Meta: model = TaxonomySerializer.Meta.model fields = TaxonomySerializer.Meta.fields + ["orgs", "all_orgs"] read_only_fields = ["orgs", "all_orgs"] - - -class ExportContentTagsQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method - """ - Serializer for the query params for the export objecttags GET view - """ - download = serializers.BooleanField(required=False, default=False) 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 fbff9879d5e7..ec7c21ca0d5b 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 @@ -1662,7 +1662,7 @@ def test_export_course(self, user_attr) -> None: self.client.force_authenticate(user=user) response = self.client.get(url) assert response.status_code == status.HTTP_200_OK - assert response.headers['Content-Type'] == 'text' + assert response.headers['Content-Type'] == 'text/csv' assert int(response.headers['Content-Length']) > 0 assert response.content == self.expected_csv.encode("utf-8") 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 eabc39842f29..12cc776c025f 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -23,7 +23,6 @@ ) from ...rules import get_admin_orgs from .serializers import ( - ExportContentTagsQueryParamsSerializer, TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer, @@ -162,11 +161,6 @@ def export_children_object_tags(self, request: Request, **kwargs) -> HttpRespons """ object_id: str = kwargs.get('object_id', None) - query_params = ExportContentTagsQueryParamsSerializer( - data=request.query_params.dict() - ) - query_params.is_valid(raise_exception=True) - # Check if the user has permission to view object tags for this object_id try: if not self.request.user.has_perm( @@ -180,16 +174,8 @@ def export_children_object_tags(self, request: Request, **kwargs) -> HttpRespons except ValueError as e: raise ValidationError from e - if query_params.data.get("download"): - content_type = "text/csv" - else: - content_type = "text" - tags = export_content_object_children_tags(object_id) - if query_params.data.get("download"): - response = HttpResponse(tags.encode('utf-8'), content_type=content_type) - response["Content-Disposition"] = f'attachment; filename="{object_id}_tags.csv"' - return response - - return HttpResponse(tags, content_type=content_type) + response = HttpResponse(tags.encode('utf-8'), content_type="text/csv") + response["Content-Disposition"] = f'attachment; filename="{object_id}_tags.csv"' + return response From 04ca0724162afb12891984f26fcad4393297eae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 26 Jan 2024 11:46:57 -0300 Subject: [PATCH 08/50] refactor: create a new view to export objecttags --- .../rest_api/v1/tests/test_views.py | 4 ++-- .../content_tagging/rest_api/v1/urls.py | 4 ++++ .../content_tagging/rest_api/v1/views.py | 19 ++++++------------- 3 files changed, 12 insertions(+), 15 deletions(-) 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 ec7c21ca0d5b..27481576c25f 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 @@ -1666,10 +1666,10 @@ def test_export_course(self, user_attr) -> None: assert int(response.headers['Content-Length']) > 0 assert response.content == self.expected_csv.encode("utf-8") - def test_export_course_anoymous_unauthorized(self) -> None: + def test_export_course_anoymous_forbidden(self) -> None: url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) response = self.client.get(url) - assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response.status_code == status.HTTP_403_FORBIDDEN def test_export_course_user_forbidden(self) -> None: url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py index ad7fd8005c71..47cf07944c52 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py @@ -30,5 +30,9 @@ oel_tagging_views_import.TemplateView.as_view(), name="taxonomy-import-template", ), + path( + "object_tags//export/", + views.ObjectTagExportView.as_view(), + ), path('', include(router.urls)) ] 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 12cc776c025f..0a08426ceeec 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -11,6 +11,7 @@ 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 ...api import ( create_taxonomy, @@ -145,20 +146,12 @@ class ObjectTagOrgView(ObjectTagView): """ filter_backends = [ObjectTagTaxonomyOrgFilterBackend] - def get_queryset(self): - if self.action == "retrieve": - return super().get_queryset() - - # For other actions, return a dummy queryset only for permission checking - dummy_queryset = QuerySet(model=ObjectTag) - - return dummy_queryset - @action(detail=True, url_path="export", methods=["get"]) - def export_children_object_tags(self, request: Request, **kwargs) -> HttpResponse: - """ - Export all the object tags for the given object_id children. - """ +class ObjectTagExportView(APIView): + """" + Export a CSV with all children and tags for a given object_id. + """ + def get(self, request: Request, **kwargs) -> HttpResponse: object_id: str = kwargs.get('object_id', None) # Check if the user has permission to view object tags for this object_id From b01d6d46349ece52eddb9e6f69bdb435b4bff62c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 29 Jan 2024 12:21:44 -0300 Subject: [PATCH 09/50] refactor: change api and view structure --- .../core/djangoapps/content_tagging/api.py | 221 ++++++++---------- .../rest_api/v1/serializers.py | 8 +- .../rest_api/v1/tests/test_views.py | 35 ++- .../content_tagging/rest_api/v1/views.py | 111 ++++++++- .../content_tagging/tests/test_api.py | 148 ++++++++++-- .../core/djangoapps/content_tagging/types.py | 20 +- 6 files changed, 377 insertions(+), 166 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 0dc2ff012da3..af6797e2a098 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -2,26 +2,23 @@ Content Tagging APIs """ from __future__ import annotations -from typing import TYPE_CHECKING -import csv from itertools import groupby -from io import StringIO import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import Q, QuerySet, Exists, OuterRef from opaque_keys.edx.keys import CourseKey, UsageKey -from openedx_tagging.core.tagging.models import ObjectTag - +from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy +from organizations.models import Organization from xmodule.modulestore.django import modulestore from .models import ContentObjectTag, TaxonomyOrg - -if TYPE_CHECKING: - from openedx_tagging.core.tagging.models import Taxonomy - from xblock.runtime import Runtime - from organizations.models import Organization - from .types import ContentKey +from .types import ( + ContentKey, + ObjectTagByObjectIdDict, + TaggedContent, + TaxonomyDict, +) def create_taxonomy( @@ -140,18 +137,22 @@ def get_unassigned_taxonomies(enabled=True) -> QuerySet: def get_content_tags( object_key: ContentKey, taxonomy_id: int | None = None, -) -> QuerySet: +) -> QuerySet[ContentObjectTag]: """ Generates a list of content tags for a given object. Pass taxonomy to limit the returned object_tags to a specific taxonomy. """ - return oel_tagging.get_object_tags( + + tags = oel_tagging.get_object_tags( object_id=str(object_key), taxonomy_id=taxonomy_id, object_tag_class=ContentObjectTag, ) + # Add a generic type to get_object_tags to fix this + return tags # type: ignore + # FixMe: The following method (tag_content_object) is only used in tasks.py for auto-tagging. To tag object we are # using oel_tagging.tag_object and checking permissions via rule overrides. @@ -159,7 +160,7 @@ def tag_content_object( object_key: ContentKey, taxonomy: Taxonomy, tags: list, -) -> QuerySet: +) -> QuerySet[ContentObjectTag]: """ This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or course). @@ -189,144 +190,112 @@ def tag_content_object( return get_content_tags(object_key, taxonomy_id=taxonomy.id) -def export_content_object_children_tags( - course_key_str: str, -) -> str: +def get_content_tags_for_object( + content_key: ContentKey, + include_children: bool, +) -> tuple[TaggedContent, TaxonomyDict]: """ - Generates a CSV file with the tags for all the children of a course. + Returns the object with the tags associated with it. If include_children is True, then it will also include + the children of the object and their tags. """ - def _get_course_children_tags(course_key: CourseKey) -> tuple[dict[str, dict[int, list[str]]], dict[int, str]]: + + def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySet[ObjectTag]: """ - Returns a tuple with a dictionary of object tags for all blocks of a course, - grouping by the block id and taxonomy id; and a dictionary of taxonomy ids and names. - - I.e. - // result - { - // Block with id block-v1:edX+DemoX+Demo_Course+type@chapter+block@chapter - "block-v1:edX+DemoX+Demo_Course+type@chapter+block@chapter": { - // ObjectTags from Taxonomy with id 1 - "1": ( - "Tag1", - "Tag2", - ... - ), - // ObjectTags from Taxonomy with id 2 - "2": ( - "Tag3", - ... - ), - ... - }, - // Block with id block-v1:edX+DemoX+Demo_Course+type@sequential+block@sequential - "block-v1:edX+DemoX+Demo_Course+type@sequential+block@sequential": { - // ObjectTags from Taxonomy with id 1 - "1": ( - "Tag2", - ... - ), - ... - }, - } - - // taxonomies - { - "1": "Taxonomy A", - "2": "Taxonomy B", - ... - } + Return the tags for the object and its children using a single db query. """ - block_id_prefix = str(course_key).replace("course-v1:", "block-v1:", 1) - block_tags_records = ObjectTag.objects.filter(object_id__startswith=block_id_prefix) \ + content_key_str = str(content_key) + if not include_children: + return ObjectTag.objects.filter(object_id=content_key_str).select_related("tag__taxonomy").all() + + # 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. + # ToDo: Add support for other content types (like LibraryContent and LibraryBlock) + if isinstance(content_key, UsageKey): + course_key_str = str(content_key.course_key) + block_id_prefix = course_key_str.replace("course-v1:", "block-v1:", 1) + elif isinstance(content_key, CourseKey): + course_key_str = str(content_key) + block_id_prefix = str(content_key).replace("course-v1:", "block-v1:", 1) + else: + raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") + + return ObjectTag.objects.filter(Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str)) \ .select_related("tag__taxonomy").all() - result: dict[str, dict[int, list[str]]] = {} - taxonomies: dict[int, str] = {} + def _group_object_tags_by_objectid_taxonomy( + all_object_tags: list[ObjectTag] + ) -> tuple[ObjectTagByObjectIdDict, TaxonomyDict]: + """ + Returns a tuple with a dictionary of grouped object tags for all blocks and a dictionary of taxonomies. + """ - for object_id, block_tags in groupby(block_tags_records, lambda x: x.object_id): - result[object_id] = {} + groupedObjectTags: ObjectTagByObjectIdDict = {} + taxonomies: TaxonomyDict = {} + + for object_id, block_tags in groupby(all_object_tags, lambda x: x.object_id): + groupedObjectTags[object_id] = {} for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): - object_tag_list = list(taxonomy_tags) - result[object_id][taxonomy_id] = [ - objecttag.value - for objecttag in object_tag_list - ] + object_tags_list = list(taxonomy_tags) + groupedObjectTags[object_id][taxonomy_id] = object_tags_list if taxonomy_id not in taxonomies: # ToDo: Change name -> export_id after done: # - https://github.com/openedx/modular-learning/issues/183 - taxonomies[taxonomy_id] = object_tag_list[0].tag.taxonomy.name + taxonomies[taxonomy_id] = object_tags_list[0].tag.taxonomy - return result, taxonomies + return groupedObjectTags, taxonomies - def _generate_csv( - header: dict[str, str], - blocks: list[tuple[int, UsageKey]], - tags: dict[str, dict[int, list[str]]], - taxonomies: dict[int, str], - runtime: Runtime, - ) -> str: + def _get_object_with_tags( + content_key: ContentKey, + include_children: bool, + objectTagCache: ObjectTagByObjectIdDict, + store + ) -> TaggedContent: """ - Receives the blocks, tags and taxonomies and returns a CSV string + Returns the object with the tags associated with it. If include_children is True, then it will also include + the children of the object and their tags. """ + if isinstance(content_key, CourseKey): + xblock = store.get_course(content_key) + elif isinstance(content_key, UsageKey): + xblock = store.get_item(content_key) + else: + raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") + + tagged_xblock = TaggedContent( + xblock=xblock, + object_tags=objectTagCache.get(str(content_key), {}), + children=None, + ) - with StringIO() as csv_buffer: - csv_writer = csv.DictWriter(csv_buffer, fieldnames=header.keys()) - csv_writer.writerow(header) - - # Iterate over the blocks stack and write the block rows - while blocks: - level, block_id = blocks.pop() - # ToDo: fix block typing - block = runtime.get_block(block_id) - - block_data = { - "name": level * " " + block.display_name_with_default, - "type": block.category, - "id": block_id - } + if not include_children: + return tagged_xblock - block_id_str = str(block_id) + blocks = [tagged_xblock] - # Add the tags for each taxonomy - for taxonomy_id in taxonomies: - if block_id_str in tags and taxonomy_id in tags[block_id_str]: - block_data[f"taxonomy_{taxonomy_id}"] = ", ".join(tags[block_id_str][taxonomy_id]) + while blocks: + block = blocks.pop() + block.children = [] - csv_writer.writerow(block_data) + if block.xblock.has_children: + for child_id in block.xblock.children: + child = TaggedContent( + xblock=store.get_item(child_id), + object_tags=objectTagCache.get(str(child_id), {}), + children=None, + ) + block.children.append(child) - # Add children to the stack - if block.has_children: - for child_id in block.children: - blocks.append((level + 1, child_id)) + blocks.append(child) - return csv_buffer.getvalue() + return tagged_xblock store = modulestore() - course_key = CourseKey.from_string(course_key_str) - if not course_key.is_course: - raise ValueError(f"Invalid course key {course_key_str}") - - # ToDo: fix course typing - course = store.get_course(course_key) - if course is None: - raise ValueError(f"Course {course_key} not found") - - tags, taxonomies = _get_course_children_tags(course_key) - - blocks = [] - # Add children to the stack - if course.has_children: - for child_id in course.children: - blocks.append((0, child_id)) - - header = {"name": "Name", "type": "Type", "id": "ID"} - # Prepare the header for the taxonomies - for taxonomy_id, name in taxonomies.items(): - header[f"taxonomy_{taxonomy_id}"] = name + all_blocks_tag_records = list(_get_object_tags(content_key, include_children)) + objectTagCache, taxonomies = _group_object_tags_by_objectid_taxonomy(all_blocks_tag_records) - return _generate_csv(header, blocks, tags, taxonomies, course.runtime) + return _get_object_with_tags(content_key, include_children, objectTagCache, store), taxonomies # Expose the oel_tagging APIs diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index 12433f8a381b..f52135e37f7b 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -98,4 +98,10 @@ def get_all_orgs(self, obj) -> bool: class Meta: model = TaxonomySerializer.Meta.model fields = TaxonomySerializer.Meta.fields + ["orgs", "all_orgs"] - read_only_fields = ["orgs", "all_orgs"] + + +class ExportContentTagsQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Serializer for the query params for the export objecttags GET view + """ + include_children = serializers.BooleanField(required=False, default=True) 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 27481576c25f..e546d4b02971 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 @@ -4,8 +4,9 @@ from __future__ import annotations -from urllib.parse import parse_qs, urlparse import json +from io import BytesIO +from urllib.parse import parse_qs, urlparse from unittest.mock import MagicMock import abc @@ -1663,8 +1664,36 @@ def test_export_course(self, user_attr) -> None: response = self.client.get(url) assert response.status_code == status.HTTP_200_OK assert response.headers['Content-Type'] == 'text/csv' - assert int(response.headers['Content-Length']) > 0 - assert response.content == self.expected_csv.encode("utf-8") + + expected_csv = ( + "Name,Type,ID,Taxonomy 1,Taxonomy 2\r\n" + 'Test Course,course,course-v1:orgA+test_course+test_run,Tag 1.1,\r\n' + ' test sequential,sequential,block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential,' + '"Tag 1.1, Tag 1.2",Tag 2.1\r\n' + ' test vertical1,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1,' + ',Tag 2.2\r\n' + ' test vertical2,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical2,,\r\n' + ' Text,html,block-v1:orgA+test_course+test_run+type@html+block@test_html,,Tag 2.1\r\n' + ) + + zip_content = BytesIO(b"".join(response.streaming_content)).getvalue() # type: ignore[attr-defined] + assert zip_content == expected_csv.encode() + + def test_export_course_no_children(self) -> None: + url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) + + self.client.force_authenticate(user=self.staff) + response = self.client.get(url, {"include_children": False}) + assert response.status_code == status.HTTP_200_OK + assert response.headers['Content-Type'] == 'text/csv' + + expected_csv = ( + "Name,Type,ID,Taxonomy 1\r\n" + 'Test Course,course,course-v1:orgA+test_course+test_run,Tag 1.1\r\n' + ) + + zip_content = BytesIO(b"".join(response.streaming_content)).getvalue() # type: ignore[attr-defined] + assert zip_content == expected_csv.encode() def test_export_course_anoymous_forbidden(self) -> None: url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.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 0a08426ceeec..7d41f345cc24 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -1,10 +1,15 @@ """ Tagging Org API Views """ -from django.db.models.query import QuerySet -from django.http import HttpResponse +from __future__ import annotations + +import csv +from typing import Iterator + +from django.http import StreamingHttpResponse +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging import rules as oel_tagging_rules -from openedx_tagging.core.tagging.models import ObjectTag from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status from rest_framework.decorators import action @@ -15,7 +20,7 @@ from ...api import ( create_taxonomy, - export_content_object_children_tags, + get_content_tags_for_object, get_taxonomy, get_taxonomies, get_taxonomies_for_org, @@ -23,7 +28,9 @@ set_taxonomy_orgs, ) from ...rules import get_admin_orgs +from ...types import TaggedContent, TaxonomyDict from .serializers import ( + ExportContentTagsQueryParamsSerializer, TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer, @@ -149,11 +156,86 @@ class ObjectTagOrgView(ObjectTagView): class ObjectTagExportView(APIView): """" - Export a CSV with all children and tags for a given object_id. + View to export a CSV with all children and tags for a given object_id. """ - def get(self, request: Request, **kwargs) -> HttpResponse: + def get(self, request: Request, **kwargs) -> StreamingHttpResponse: + """ + Export a CSV with all children and tags for a given object_id. + """ + + def _content_generator( + tagged_content: TaggedContent, level: int = 0 + ) -> Iterator[tuple[TaggedContent, int]]: + """ + Generator that yields the tagged content and the level of the block + """ + yield tagged_content, level + if tagged_content.children: + for child in tagged_content.children: + yield from _content_generator(child, level + 1) + + class Echo(object): + """ + Class that implements just the write method of the file-like interface, + used for the streaming response. + """ + def write(self, value): + return value + + def _generate_csv_rows( + tagged_content: TaggedContent, + taxonomies: TaxonomyDict, + pseudo_buffer: Echo, + ) -> Iterator[str]: + """ + Receives the blocks, tags and taxonomies and returns a CSV string + """ + + header = {"name": "Name", "type": "Type", "id": "ID"} + + # Prepare the header for the taxonomies + for taxonomy_id, taxonomy in taxonomies.items(): + # ToDo: change to taxonomy.external_id after the external_id is implemented + header[f"taxonomy_{taxonomy_id}"] = taxonomy.name + + csv_writer = csv.DictWriter(pseudo_buffer, fieldnames=header.keys()) + yield csv_writer.writerow(header) + + # Iterate over the blocks and yield the rows + for item, level in _content_generator(tagged_content): + if item.xblock.category == "course": + block_id = item.xblock.id + else: + block_id = item.xblock.location + + block_data = { + "name": level * " " + item.xblock.display_name_with_default, + "type": item.xblock.category, + "id": str(block_id), + } + + # Add the tags for each taxonomy + for taxonomy_id in taxonomies: + if taxonomy_id in item.object_tags: + block_data[f"taxonomy_{taxonomy_id}"] = ", ".join([ + object_tag.value + for object_tag in item.object_tags[taxonomy_id] + ]) + + yield csv_writer.writerow(block_data) + object_id: str = kwargs.get('object_id', None) + content_key: UsageKey | CourseKey + + try: + content_key = UsageKey.from_string(object_id) + except InvalidKeyError: + try: + content_key = CourseKey.from_string(object_id) + except InvalidKeyError as e: + raise ValidationError("object_id is not a valid content key.") from e + # Check if the user has permission to view object tags for this object_id try: if not self.request.user.has_perm( @@ -167,8 +249,17 @@ def get(self, request: Request, **kwargs) -> HttpResponse: except ValueError as e: raise ValidationError from e - tags = export_content_object_children_tags(object_id) + query_params = ExportContentTagsQueryParamsSerializer( + data=request.query_params.dict() + ) + query_params.is_valid(raise_exception=True) - response = HttpResponse(tags.encode('utf-8'), content_type="text/csv") - response["Content-Disposition"] = f'attachment; filename="{object_id}_tags.csv"' - return response + include_children = query_params.validated_data.get("include_children") + + tagged_block, taxonomies = get_content_tags_for_object(content_key, include_children=include_children) + + return StreamingHttpResponse( + streaming_content=_generate_csv_rows(tagged_block, taxonomies, Echo()), + content_type="text/csv", + headers={'Content-Disposition': f'attachment; filename="{object_id}_tags.csv"'}, + ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 832fa38d89fb..28921afc61fc 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -10,6 +10,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE from .. import api +from ..types import TaggedContent class TestTaxonomyMixin: @@ -300,55 +301,152 @@ def setUp(self): "test_course", "test_run", self.user_id, + fields={'display_name': "Test Course"}, + ) + course_tags = api.tag_content_object( + object_key=self.course.id, + taxonomy=self.taxonomy_1, + tags=['Tag 1.1'], ) - # Create XBlocks - sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") - vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical1") - vertical2 = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical2") - text = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") + self.expected_tagged_xblock = TaggedContent( + xblock=self.course, + children=[], + object_tags={ + self.taxonomy_1.id: list(course_tags), + }, + ) + # Create XBlocks + self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") # Tag blocks - api.tag_content_object( - object_key=sequential.location, + sequential_tags1 = api.tag_content_object( + object_key=self.sequential.location, taxonomy=self.taxonomy_1, tags=['Tag 1.1', 'Tag 1.2'], ) - api.tag_content_object( - object_key=sequential.location, + sequential_tags2 = api.tag_content_object( + object_key=self.sequential.location, taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - api.tag_content_object( + tagged_sequential = TaggedContent( + xblock=self.store.get_item(self.sequential.location), + children=[], + object_tags={ + self.taxonomy_1.id: list(sequential_tags1), + self.taxonomy_2.id: list(sequential_tags2), + }, + ) + + assert self.expected_tagged_xblock.children is not None # type guard + self.expected_tagged_xblock.children.append(tagged_sequential) + + vertical = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical1") + vertical_tags = api.tag_content_object( object_key=vertical.location, taxonomy=self.taxonomy_2, tags=['Tag 2.2'], ) - api.tag_content_object( + tagged_vertical = TaggedContent( + xblock=self.store.get_item(vertical.location), + children=[], + object_tags={ + self.taxonomy_2.id: list(vertical_tags), + }, + ) + + assert tagged_sequential.children is not None # type guard + tagged_sequential.children.append(tagged_vertical) + + vertical2 = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical2") + tagged_vertical2 = TaggedContent( + xblock=self.store.get_item(vertical2.location), + children=[], + object_tags={}, + ) + assert tagged_sequential.children is not None # type guard + tagged_sequential.children.append(tagged_vertical2) + + text = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") + text_tags = api.tag_content_object( object_key=text.location, taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - - self.expected_csv = ( - "Name,Type,ID,Taxonomy 1,Taxonomy 2\r\n" - 'test sequential,sequential,block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential,' - '"Tag 1.1, Tag 1.2",Tag 2.1\r\n' - ' test vertical2,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical2,,\r\n' - ' Text,html,block-v1:orgA+test_course+test_run+type@html+block@test_html,,Tag 2.1\r\n' - ' test vertical1,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1,' - ',Tag 2.2\r\n' + tagged_text = TaggedContent( + xblock=self.store.get_item(text.location), + children=[], + object_tags={ + self.taxonomy_2.id: list(text_tags), + }, ) + assert tagged_vertical2.children is not None # type guard + tagged_vertical2.children.append(tagged_text) -class TestContetTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] + +@ddt.ddt +class TestContentTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] """ Test exporting content objects """ + def _compare_tagged_xblock(self, expected: TaggedContent, actual: TaggedContent): + """ + Compare two TaggedContent objects + """ + assert expected.xblock.location == actual.xblock.location + assert expected.object_tags == actual.object_tags + if expected.children is None: + assert actual.children is None + return - def test_export_tagged_course_children(self): + assert actual.children is not None + for i in range(len(expected.children)): + self._compare_tagged_xblock(expected.children[i], actual.children[i]) + + @ddt.data( + True, + False, + ) + def test_export_tagged_course(self, include_children: bool) -> None: + """ + Test if we can export a course + """ + tagged_xblock, taxonomies = api.get_content_tags_for_object(self.course.id, include_children=include_children) + if include_children: + expected_taxonomies = { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } + else: + self.expected_tagged_xblock.children = None + expected_taxonomies = { + self.taxonomy_1.id: self.taxonomy_1, + } + + self._compare_tagged_xblock(self.expected_tagged_xblock, tagged_xblock) + assert taxonomies == expected_taxonomies + + @ddt.data( + True, + False, + ) + def test_export_tagged_block(self, include_children: bool) -> None: """ - Test if we can export a course with tagged children + Test if we can export a course """ - result = api.export_content_object_children_tags(str(self.course.id)) - assert result == self.expected_csv + tagged_xblock, taxonomies = api.get_content_tags_for_object(self.course.id, include_children=include_children) + if include_children: + expected_taxonomies = { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } + else: + self.expected_tagged_xblock.children = None + expected_taxonomies = { + self.taxonomy_1.id: self.taxonomy_1, + } + + self._compare_tagged_xblock(self.expected_tagged_xblock, tagged_xblock) + assert taxonomies == expected_taxonomies diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index 3a9f6ec54935..7d2b44e77de5 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -1,8 +1,26 @@ """ Types used by content tagging API and implementation """ -from typing import Union +from __future__ import annotations +from typing import Union, Dict, List +from attrs import define from opaque_keys.edx.keys import LearningContextKey, UsageKey +from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy +from xblock.core import XBlock ContentKey = Union[LearningContextKey, UsageKey] + +ObjectTagByTaxonomyIdDict = Dict[int, List[ObjectTag]] +ObjectTagByObjectIdDict = Dict[str, ObjectTagByTaxonomyIdDict] +TaxonomyDict = Dict[int, Taxonomy] + + +@define +class TaggedContent: + """ + A tagged content, with its tags and children. + """ + xblock: XBlock # ToDo: Check correct type here + object_tags: ObjectTagByTaxonomyIdDict + children: list[TaggedContent] | None From 18a84258bf18f6602cd9d75b3ae7cb345b51833d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 29 Jan 2024 14:09:55 -0300 Subject: [PATCH 10/50] docs: remove old comment --- openedx/core/djangoapps/content_tagging/api.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index af6797e2a098..467d9ef438ec 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -239,8 +239,6 @@ def _group_object_tags_by_objectid_taxonomy( groupedObjectTags[object_id][taxonomy_id] = object_tags_list if taxonomy_id not in taxonomies: - # ToDo: Change name -> export_id after done: - # - https://github.com/openedx/modular-learning/issues/183 taxonomies[taxonomy_id] = object_tags_list[0].tag.taxonomy return groupedObjectTags, taxonomies From 38ae3533d41e0fbf776cca927de13721ec558bab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 14:05:22 -0300 Subject: [PATCH 11/50] docs: revert view docstring --- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 2 -- 1 file changed, 2 deletions(-) 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 7d41f345cc24..f09c77b3de6b 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -146,8 +146,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 and - new actions like: export. Refer to ObjectTagView docstring for usage details. """ From b65a6c8e1c0d852664c0c9bc7376b496cf202f1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 14:11:38 -0300 Subject: [PATCH 12/50] fix: remove include_children query param --- openedx/core/djangoapps/content_tagging/api.py | 2 +- .../content_tagging/rest_api/v1/serializers.py | 7 ------- .../rest_api/v1/tests/test_views.py | 16 ---------------- .../content_tagging/rest_api/v1/views.py | 10 +--------- 4 files changed, 2 insertions(+), 33 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 467d9ef438ec..77e796c9a0b2 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -192,7 +192,7 @@ def tag_content_object( def get_content_tags_for_object( content_key: ContentKey, - include_children: bool, + include_children: bool = True, ) -> tuple[TaggedContent, TaxonomyDict]: """ Returns the object with the tags associated with it. If include_children is True, then it will also include diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index f52135e37f7b..b7ce7d7fbb5a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -98,10 +98,3 @@ def get_all_orgs(self, obj) -> bool: class Meta: model = TaxonomySerializer.Meta.model fields = TaxonomySerializer.Meta.fields + ["orgs", "all_orgs"] - - -class ExportContentTagsQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method - """ - Serializer for the query params for the export objecttags GET view - """ - include_children = serializers.BooleanField(required=False, default=True) 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 e546d4b02971..7414b3d84b3a 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 @@ -1679,22 +1679,6 @@ def test_export_course(self, user_attr) -> None: zip_content = BytesIO(b"".join(response.streaming_content)).getvalue() # type: ignore[attr-defined] assert zip_content == expected_csv.encode() - def test_export_course_no_children(self) -> None: - url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) - - self.client.force_authenticate(user=self.staff) - response = self.client.get(url, {"include_children": False}) - assert response.status_code == status.HTTP_200_OK - assert response.headers['Content-Type'] == 'text/csv' - - expected_csv = ( - "Name,Type,ID,Taxonomy 1\r\n" - 'Test Course,course,course-v1:orgA+test_course+test_run,Tag 1.1\r\n' - ) - - zip_content = BytesIO(b"".join(response.streaming_content)).getvalue() # type: ignore[attr-defined] - assert zip_content == expected_csv.encode() - def test_export_course_anoymous_forbidden(self) -> None: url = OBJECT_TAGS_EXPORT_URL.format(object_id=str(self.course.id)) response = self.client.get(url) 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 f09c77b3de6b..8f7b4bf3601e 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -30,7 +30,6 @@ from ...rules import get_admin_orgs from ...types import TaggedContent, TaxonomyDict from .serializers import ( - ExportContentTagsQueryParamsSerializer, TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer, @@ -247,14 +246,7 @@ def _generate_csv_rows( except ValueError as e: raise ValidationError from e - query_params = ExportContentTagsQueryParamsSerializer( - data=request.query_params.dict() - ) - query_params.is_valid(raise_exception=True) - - include_children = query_params.validated_data.get("include_children") - - tagged_block, taxonomies = get_content_tags_for_object(content_key, include_children=include_children) + tagged_block, taxonomies = get_content_tags_for_object(content_key) return StreamingHttpResponse( streaming_content=_generate_csv_rows(tagged_block, taxonomies, Echo()), From f5ac3a6c34bc38e6f0239e6173b56dc4f32e5a87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 14:13:39 -0300 Subject: [PATCH 13/50] fix: removing .all() call from queryset --- openedx/core/djangoapps/content_tagging/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 77e796c9a0b2..5463072bc508 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -205,7 +205,7 @@ def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySe """ content_key_str = str(content_key) if not include_children: - return ObjectTag.objects.filter(object_id=content_key_str).select_related("tag__taxonomy").all() + return ObjectTag.objects.filter(object_id=content_key_str).select_related("tag__taxonomy") # 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. @@ -220,7 +220,7 @@ def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySe raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") return ObjectTag.objects.filter(Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str)) \ - .select_related("tag__taxonomy").all() + .select_related("tag__taxonomy") def _group_object_tags_by_objectid_taxonomy( all_object_tags: list[ObjectTag] From debf254e67ddfa0b59d0f519406e212a3e7d3f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 14:19:58 -0300 Subject: [PATCH 14/50] refactor: method rename --- openedx/core/djangoapps/content_tagging/api.py | 6 +++--- .../core/djangoapps/content_tagging/rest_api/v1/views.py | 4 ++-- openedx/core/djangoapps/content_tagging/tests/test_api.py | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 5463072bc508..b69534eff123 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -190,7 +190,7 @@ def tag_content_object( return get_content_tags(object_key, taxonomy_id=taxonomy.id) -def get_content_tags_for_object( +def get_object_tree_with_objecttags( content_key: ContentKey, include_children: bool = True, ) -> tuple[TaggedContent, TaxonomyDict]: @@ -243,7 +243,7 @@ def _group_object_tags_by_objectid_taxonomy( return groupedObjectTags, taxonomies - def _get_object_with_tags( + def _build_object_tree_with_objecttags( content_key: ContentKey, include_children: bool, objectTagCache: ObjectTagByObjectIdDict, @@ -293,7 +293,7 @@ def _get_object_with_tags( all_blocks_tag_records = list(_get_object_tags(content_key, include_children)) objectTagCache, taxonomies = _group_object_tags_by_objectid_taxonomy(all_blocks_tag_records) - return _get_object_with_tags(content_key, include_children, objectTagCache, store), taxonomies + return _build_object_tree_with_objecttags(content_key, include_children, objectTagCache, store), taxonomies # Expose the oel_tagging APIs 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 8f7b4bf3601e..bcc47071ebe3 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -20,7 +20,7 @@ from ...api import ( create_taxonomy, - get_content_tags_for_object, + get_object_tree_with_objecttags, get_taxonomy, get_taxonomies, get_taxonomies_for_org, @@ -246,7 +246,7 @@ def _generate_csv_rows( except ValueError as e: raise ValidationError from e - tagged_block, taxonomies = get_content_tags_for_object(content_key) + tagged_block, taxonomies = get_object_tree_with_objecttags(content_key) return StreamingHttpResponse( streaming_content=_generate_csv_rows(tagged_block, taxonomies, Echo()), diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 28921afc61fc..3aed1f5db42d 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -413,7 +413,7 @@ def test_export_tagged_course(self, include_children: bool) -> None: """ Test if we can export a course """ - tagged_xblock, taxonomies = api.get_content_tags_for_object(self.course.id, include_children=include_children) + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) if include_children: expected_taxonomies = { self.taxonomy_1.id: self.taxonomy_1, @@ -436,7 +436,7 @@ def test_export_tagged_block(self, include_children: bool) -> None: """ Test if we can export a course """ - tagged_xblock, taxonomies = api.get_content_tags_for_object(self.course.id, include_children=include_children) + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) if include_children: expected_taxonomies = { self.taxonomy_1.id: self.taxonomy_1, From 9726d6d23f9edae217a28ec7fab466d736960a4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 14:26:17 -0300 Subject: [PATCH 15/50] fix: filter deleted tags --- openedx/core/djangoapps/content_tagging/api.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index b69534eff123..589481bc516e 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -205,7 +205,11 @@ def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySe """ content_key_str = str(content_key) if not include_children: - return ObjectTag.objects.filter(object_id=content_key_str).select_related("tag__taxonomy") + return ObjectTag.objects.filter( + object_id=content_key_str, + tag__isnull=False, + tag__taxonomy__isnull=False, + ).select_related("tag__taxonomy") # 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. @@ -219,8 +223,10 @@ def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySe else: raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") - return ObjectTag.objects.filter(Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str)) \ - .select_related("tag__taxonomy") + return ObjectTag.objects.filter( + Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str), + Q(tag__isnull=False, tag__taxonomy__isnull=False), + ).select_related("tag__taxonomy") def _group_object_tags_by_objectid_taxonomy( all_object_tags: list[ObjectTag] From c8c12bbc00bc4bb1ee1a4448606d1efb9f799deb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 14:38:43 -0300 Subject: [PATCH 16/50] fix: pylint --- openedx/core/djangoapps/content_tagging/api.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 589481bc516e..3f7f6aaa2fbf 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -206,10 +206,10 @@ def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySe content_key_str = str(content_key) if not include_children: return ObjectTag.objects.filter( - object_id=content_key_str, - tag__isnull=False, - tag__taxonomy__isnull=False, - ).select_related("tag__taxonomy") + object_id=content_key_str, + tag__isnull=False, + tag__taxonomy__isnull=False, + ).select_related("tag__taxonomy") # 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. @@ -224,9 +224,9 @@ def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySe raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") return ObjectTag.objects.filter( - Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str), - Q(tag__isnull=False, tag__taxonomy__isnull=False), - ).select_related("tag__taxonomy") + Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str), + Q(tag__isnull=False, tag__taxonomy__isnull=False), + ).select_related("tag__taxonomy") def _group_object_tags_by_objectid_taxonomy( all_object_tags: list[ObjectTag] From 1f11b17e50e9ea94853c2d188fcbc10fe8d2f5bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 14:52:20 -0300 Subject: [PATCH 17/50] fix: quote string in csv export --- .../rest_api/v1/tests/test_views.py | 17 +++++++++-------- .../content_tagging/rest_api/v1/views.py | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) 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 7414b3d84b3a..a772dea8a0c0 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 @@ -1666,14 +1666,15 @@ def test_export_course(self, user_attr) -> None: assert response.headers['Content-Type'] == 'text/csv' expected_csv = ( - "Name,Type,ID,Taxonomy 1,Taxonomy 2\r\n" - 'Test Course,course,course-v1:orgA+test_course+test_run,Tag 1.1,\r\n' - ' test sequential,sequential,block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential,' - '"Tag 1.1, Tag 1.2",Tag 2.1\r\n' - ' test vertical1,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1,' - ',Tag 2.2\r\n' - ' test vertical2,vertical,block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical2,,\r\n' - ' Text,html,block-v1:orgA+test_course+test_run+type@html+block@test_html,,Tag 2.1\r\n' + '"Name","Type","ID","Taxonomy 1","Taxonomy 2"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1",""\r\n' + '" test sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@test_' + 'sequential","Tag 1.1, Tag 1.2","Tag 2.1"\r\n' + '" test vertical1","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + 'vertical1","","Tag 2.2"\r\n' + '" test vertical2","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + 'vertical2","",""\r\n' + '" Text","html","block-v1:orgA+test_course+test_run+type@html+block@test_html","","Tag 2.1"\r\n' ) zip_content = BytesIO(b"".join(response.streaming_content)).getvalue() # type: ignore[attr-defined] 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 bcc47071ebe3..2c9fe400653b 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -195,7 +195,7 @@ def _generate_csv_rows( # ToDo: change to taxonomy.external_id after the external_id is implemented header[f"taxonomy_{taxonomy_id}"] = taxonomy.name - csv_writer = csv.DictWriter(pseudo_buffer, fieldnames=header.keys()) + csv_writer = csv.DictWriter(pseudo_buffer, fieldnames=header.keys(), quoting=csv.QUOTE_NONNUMERIC) yield csv_writer.writerow(header) # Iterate over the blocks and yield the rows From 484c0424afb23d785cec0941f7d534379fb15bb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 15:08:36 -0300 Subject: [PATCH 18/50] test: add querycount --- .../core/djangoapps/content_tagging/tests/test_api.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 3aed1f5db42d..abcd12e4bfa6 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -413,7 +413,10 @@ def test_export_tagged_course(self, include_children: bool) -> None: """ Test if we can export a course """ - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) + # 2 from get_course() / get_item() + 1 from _get_object_tags() + with self.assertNumQueries(3): + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) + if include_children: expected_taxonomies = { self.taxonomy_1.id: self.taxonomy_1, @@ -436,7 +439,10 @@ def test_export_tagged_block(self, include_children: bool) -> None: """ Test if we can export a course """ - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) + # 2 from get_course() / get_item() + 1 from _get_object_tags() + with self.assertNumQueries(3): + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) + if include_children: expected_taxonomies = { self.taxonomy_1.id: self.taxonomy_1, From 6b6ba34df5c8c6be93262a504d09d38df93fd757 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 15:10:15 -0300 Subject: [PATCH 19/50] fix: pylint --- .../content_tagging/rest_api/v1/tests/test_views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 a772dea8a0c0..48f4e6dbccca 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 @@ -1668,11 +1668,11 @@ def test_export_course(self, user_attr) -> None: expected_csv = ( '"Name","Type","ID","Taxonomy 1","Taxonomy 2"\r\n' '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1",""\r\n' - '" test sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@test_' + '" test sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@test_' \ 'sequential","Tag 1.1, Tag 1.2","Tag 2.1"\r\n' - '" test vertical1","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + '" test vertical1","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' \ 'vertical1","","Tag 2.2"\r\n' - '" test vertical2","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + '" test vertical2","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' \ 'vertical2","",""\r\n' '" Text","html","block-v1:orgA+test_course+test_run+type@html+block@test_html","","Tag 2.1"\r\n' ) From 909645402f09952af1f44b2640c015f40fca9630 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 15:22:58 -0300 Subject: [PATCH 20/50] fix: pylint.. --- .../content_tagging/rest_api/v1/tests/test_views.py | 12 ++++++------ uwsgi.ini | 0 2 files changed, 6 insertions(+), 6 deletions(-) create mode 100755 uwsgi.ini 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 48f4e6dbccca..6566ec3d22cc 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 @@ -1668,12 +1668,12 @@ def test_export_course(self, user_attr) -> None: expected_csv = ( '"Name","Type","ID","Taxonomy 1","Taxonomy 2"\r\n' '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1",""\r\n' - '" test sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@test_' \ - 'sequential","Tag 1.1, Tag 1.2","Tag 2.1"\r\n' - '" test vertical1","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' \ - 'vertical1","","Tag 2.2"\r\n' - '" test vertical2","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' \ - 'vertical2","",""\r\n' + '" test sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@test_' + 'sequential","Tag 1.1, Tag 1.2","Tag 2.1"\r\n' + '" test vertical1","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + 'vertical1","","Tag 2.2"\r\n' + '" test vertical2","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' + 'vertical2","",""\r\n' '" Text","html","block-v1:orgA+test_course+test_run+type@html+block@test_html","","Tag 2.1"\r\n' ) diff --git a/uwsgi.ini b/uwsgi.ini new file mode 100755 index 000000000000..e69de29bb2d1 From a688689f5558c36c7b0a19c72518235ea3d75b7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 15:48:38 -0300 Subject: [PATCH 21/50] fix: pylint --- openedx/core/djangoapps/content_tagging/tests/test_api.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index abcd12e4bfa6..6fd76033194e 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -415,7 +415,9 @@ def test_export_tagged_course(self, include_children: bool) -> None: """ # 2 from get_course() / get_item() + 1 from _get_object_tags() with self.assertNumQueries(3): - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags( + self.course.id, include_children=include_children + ) if include_children: expected_taxonomies = { @@ -441,7 +443,9 @@ def test_export_tagged_block(self, include_children: bool) -> None: """ # 2 from get_course() / get_item() + 1 from _get_object_tags() with self.assertNumQueries(3): - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id, include_children=include_children) + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags( + self.course.id, include_children=include_children + ) if include_children: expected_taxonomies = { From e9335c822f8ff8ab03c09d6d5b1a7615185ff353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 18:26:06 -0300 Subject: [PATCH 22/50] revert: undo removed property --- .../core/djangoapps/content_tagging/rest_api/v1/serializers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index b7ce7d7fbb5a..12433f8a381b 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -98,3 +98,4 @@ def get_all_orgs(self, obj) -> bool: class Meta: model = TaxonomySerializer.Meta.model fields = TaxonomySerializer.Meta.fields + ["orgs", "all_orgs"] + read_only_fields = ["orgs", "all_orgs"] From ab1a69ed4cfb26cb927c82823f57b8a4c5b78d23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 19:24:19 -0300 Subject: [PATCH 23/50] style: fix camelCase --- openedx/core/djangoapps/content_tagging/api.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 3f7f6aaa2fbf..de0c4c89c5c3 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -235,19 +235,19 @@ def _group_object_tags_by_objectid_taxonomy( Returns a tuple with a dictionary of grouped object tags for all blocks and a dictionary of taxonomies. """ - groupedObjectTags: ObjectTagByObjectIdDict = {} + grouped_object_tags: ObjectTagByObjectIdDict = {} taxonomies: TaxonomyDict = {} for object_id, block_tags in groupby(all_object_tags, lambda x: x.object_id): - groupedObjectTags[object_id] = {} + grouped_object_tags[object_id] = {} for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): object_tags_list = list(taxonomy_tags) - groupedObjectTags[object_id][taxonomy_id] = object_tags_list + grouped_object_tags[object_id][taxonomy_id] = object_tags_list if taxonomy_id not in taxonomies: taxonomies[taxonomy_id] = object_tags_list[0].tag.taxonomy - return groupedObjectTags, taxonomies + return grouped_object_tags, taxonomies def _build_object_tree_with_objecttags( content_key: ContentKey, From db9116d906586417ee9fae9929d32551afcd79a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 19:57:34 -0300 Subject: [PATCH 24/50] refactor: remove xblock from TaggedContent and include_children param --- .../core/djangoapps/content_tagging/api.py | 72 ++++++------- .../content_tagging/rest_api/v1/views.py | 11 +- .../content_tagging/tests/test_api.py | 102 +++++++----------- .../core/djangoapps/content_tagging/types.py | 5 +- 4 files changed, 76 insertions(+), 114 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index de0c4c89c5c3..68c36f7a09e6 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -7,7 +7,7 @@ import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import Q, QuerySet, Exists, OuterRef -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import CourseKey, LearningContextKey from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization from xmodule.modulestore.django import modulestore @@ -191,33 +191,22 @@ def tag_content_object( def get_object_tree_with_objecttags( - content_key: ContentKey, - include_children: bool = True, + content_key: LearningContextKey, ) -> tuple[TaggedContent, TaxonomyDict]: """ - Returns the object with the tags associated with it. If include_children is True, then it will also include - the children of the object and their tags. + Returns the object with the tags associated with it. """ - def _get_object_tags(content_key: ContentKey, include_children: bool) -> QuerySet[ObjectTag]: + def _get_object_tags(content_key: LearningContextKey) -> QuerySet[ObjectTag]: """ Return the tags for the object and its children using a single db query. """ content_key_str = str(content_key) - if not include_children: - return ObjectTag.objects.filter( - object_id=content_key_str, - tag__isnull=False, - tag__taxonomy__isnull=False, - ).select_related("tag__taxonomy") # 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. # ToDo: Add support for other content types (like LibraryContent and LibraryBlock) - if isinstance(content_key, UsageKey): - course_key_str = str(content_key.course_key) - block_id_prefix = course_key_str.replace("course-v1:", "block-v1:", 1) - elif isinstance(content_key, CourseKey): + if isinstance(content_key, CourseKey): course_key_str = str(content_key) block_id_prefix = str(content_key).replace("course-v1:", "block-v1:", 1) else: @@ -250,56 +239,57 @@ def _group_object_tags_by_objectid_taxonomy( return grouped_object_tags, taxonomies def _build_object_tree_with_objecttags( - content_key: ContentKey, - include_children: bool, + content_key: LearningContextKey, objectTagCache: ObjectTagByObjectIdDict, store ) -> TaggedContent: """ - Returns the object with the tags associated with it. If include_children is True, then it will also include - the children of the object and their tags. + Returns the object with the tags associated with it. """ if isinstance(content_key, CourseKey): - xblock = store.get_course(content_key) - elif isinstance(content_key, UsageKey): - xblock = store.get_item(content_key) + course = store.get_course(content_key) else: raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") - tagged_xblock = TaggedContent( - xblock=xblock, + display_name = course.display_name_with_default + course_id = str(course.id) + + tagged_course = TaggedContent( + display_name=display_name, + block_id=course_id, + category=course.category, object_tags=objectTagCache.get(str(content_key), {}), children=None, ) - if not include_children: - return tagged_xblock - - blocks = [tagged_xblock] + blocks = [(tagged_course, course)] while blocks: - block = blocks.pop() - block.children = [] - - if block.xblock.has_children: - for child_id in block.xblock.children: - child = TaggedContent( - xblock=store.get_item(child_id), + tagged_block, xblock = 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=objectTagCache.get(str(child_id), {}), children=None, ) - block.children.append(child) + tagged_block.children.append(tagged_child) - blocks.append(child) + blocks.append((tagged_child, child_block)) - return tagged_xblock + return tagged_course store = modulestore() - all_blocks_tag_records = list(_get_object_tags(content_key, include_children)) + all_blocks_tag_records = list(_get_object_tags(content_key)) objectTagCache, taxonomies = _group_object_tags_by_objectid_taxonomy(all_blocks_tag_records) - return _build_object_tree_with_objecttags(content_key, include_children, objectTagCache, store), taxonomies + return _build_object_tree_with_objecttags(content_key, objectTagCache, store), taxonomies # Expose the oel_tagging APIs 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 2c9fe400653b..ccbc0b47ec70 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -200,15 +200,10 @@ def _generate_csv_rows( # Iterate over the blocks and yield the rows for item, level in _content_generator(tagged_content): - if item.xblock.category == "course": - block_id = item.xblock.id - else: - block_id = item.xblock.location - block_data = { - "name": level * " " + item.xblock.display_name_with_default, - "type": item.xblock.category, - "id": str(block_id), + "name": level * " " + item.display_name, + "type": item.category, + "id": item.block_id, } # Add the tags for each taxonomy diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 6fd76033194e..dba284ad3506 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -310,7 +310,9 @@ def setUp(self): ) self.expected_tagged_xblock = TaggedContent( - xblock=self.course, + display_name=self.course.display_name_with_default, + block_id=str(self.course.id), + category=self.course.category, children=[], object_tags={ self.taxonomy_1.id: list(course_tags), @@ -330,8 +332,11 @@ def setUp(self): taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) + xblock = self.store.get_item(self.sequential.location) tagged_sequential = TaggedContent( - xblock=self.store.get_item(self.sequential.location), + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, children=[], object_tags={ self.taxonomy_1.id: list(sequential_tags1), @@ -348,8 +353,11 @@ def setUp(self): taxonomy=self.taxonomy_2, tags=['Tag 2.2'], ) + xblock = self.store.get_item(vertical.location) tagged_vertical = TaggedContent( - xblock=self.store.get_item(vertical.location), + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, children=[], object_tags={ self.taxonomy_2.id: list(vertical_tags), @@ -360,25 +368,31 @@ def setUp(self): tagged_sequential.children.append(tagged_vertical) vertical2 = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical2") + xblock = self.store.get_item(vertical2.location) tagged_vertical2 = TaggedContent( - xblock=self.store.get_item(vertical2.location), + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, children=[], object_tags={}, ) assert tagged_sequential.children is not None # type guard tagged_sequential.children.append(tagged_vertical2) - text = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") - text_tags = api.tag_content_object( - object_key=text.location, + html = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") + html_tags = api.tag_content_object( + object_key=html.location, taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) + xblock = self.store.get_item(html.location) tagged_text = TaggedContent( - xblock=self.store.get_item(text.location), + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, children=[], object_tags={ - self.taxonomy_2.id: list(text_tags), + self.taxonomy_2.id: list(html_tags), }, ) @@ -391,72 +405,34 @@ class TestContentTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] """ Test exporting content objects """ - def _compare_tagged_xblock(self, expected: TaggedContent, actual: TaggedContent): - """ - Compare two TaggedContent objects - """ - assert expected.xblock.location == actual.xblock.location - assert expected.object_tags == actual.object_tags - if expected.children is None: - assert actual.children is None - return - - assert actual.children is not None - for i in range(len(expected.children)): - self._compare_tagged_xblock(expected.children[i], actual.children[i]) - - @ddt.data( - True, - False, - ) - def test_export_tagged_course(self, include_children: bool) -> None: + def test_export_tagged_course(self) -> None: """ Test if we can export a course """ # 2 from get_course() / get_item() + 1 from _get_object_tags() with self.assertNumQueries(3): - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags( - self.course.id, include_children=include_children - ) + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id) + + expected_taxonomies = { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } - if include_children: - expected_taxonomies = { - self.taxonomy_1.id: self.taxonomy_1, - self.taxonomy_2.id: self.taxonomy_2, - } - else: - self.expected_tagged_xblock.children = None - expected_taxonomies = { - self.taxonomy_1.id: self.taxonomy_1, - } - - self._compare_tagged_xblock(self.expected_tagged_xblock, tagged_xblock) + assert tagged_xblock == self.expected_tagged_xblock assert taxonomies == expected_taxonomies - @ddt.data( - True, - False, - ) - def test_export_tagged_block(self, include_children: bool) -> None: + def test_export_tagged_block(self) -> None: """ Test if we can export a course """ # 2 from get_course() / get_item() + 1 from _get_object_tags() with self.assertNumQueries(3): - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags( - self.course.id, include_children=include_children - ) + tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id) + + expected_taxonomies = { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } - if include_children: - expected_taxonomies = { - self.taxonomy_1.id: self.taxonomy_1, - self.taxonomy_2.id: self.taxonomy_2, - } - else: - self.expected_tagged_xblock.children = None - expected_taxonomies = { - self.taxonomy_1.id: self.taxonomy_1, - } - - self._compare_tagged_xblock(self.expected_tagged_xblock, tagged_xblock) + assert tagged_xblock == self.expected_tagged_xblock assert taxonomies == expected_taxonomies diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index 7d2b44e77de5..b4da0ec9bc57 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -7,7 +7,6 @@ from attrs import define from opaque_keys.edx.keys import LearningContextKey, UsageKey from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy -from xblock.core import XBlock ContentKey = Union[LearningContextKey, UsageKey] @@ -21,6 +20,8 @@ class TaggedContent: """ A tagged content, with its tags and children. """ - xblock: XBlock # ToDo: Check correct type here + display_name: str + block_id: str + category: str object_tags: ObjectTagByTaxonomyIdDict children: list[TaggedContent] | None From 9b3dee88fd87fb77c83ea805c2c8bc00b200415d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 20:03:03 -0300 Subject: [PATCH 25/50] fix: remove UsageKey --- .../djangoapps/content_tagging/rest_api/v1/views.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) 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 ccbc0b47ec70..b26b7d543ec6 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -218,15 +218,12 @@ def _generate_csv_rows( object_id: str = kwargs.get('object_id', None) - content_key: UsageKey | CourseKey + content_key: CourseKey try: - content_key = UsageKey.from_string(object_id) - except InvalidKeyError: - try: - content_key = CourseKey.from_string(object_id) - except InvalidKeyError as e: - raise ValidationError("object_id is not a valid content key.") from e + content_key = CourseKey.from_string(object_id) + except InvalidKeyError as e: + raise ValidationError("object_id is not a valid content key.") from e # Check if the user has permission to view object tags for this object_id try: From 821e2160cbc9b2c8fcb73fd5d9f87cc13da6f685 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 21:39:33 -0300 Subject: [PATCH 26/50] fix: removing unused import --- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b26b7d543ec6..d4b28a23c9c4 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -8,7 +8,7 @@ from django.http import StreamingHttpResponse from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import CourseKey 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 From 6207915641e3737026fe798aff3335bda7d007da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 22:08:46 -0300 Subject: [PATCH 27/50] refactor: cleaning code --- openedx/core/djangoapps/content_tagging/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 68c36f7a09e6..26e26c983f6b 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -207,8 +207,8 @@ def _get_object_tags(content_key: LearningContextKey) -> QuerySet[ObjectTag]: # (course) in a single db query. # ToDo: Add support for other content types (like LibraryContent and LibraryBlock) if isinstance(content_key, CourseKey): - course_key_str = str(content_key) - block_id_prefix = str(content_key).replace("course-v1:", "block-v1:", 1) + course_key_str = content_key_str + block_id_prefix = content_key_str.replace("course-v1:", "block-v1:", 1) else: raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") From 7a2874246077446d49d635c8643907948e65e63e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 31 Jan 2024 14:33:50 -0300 Subject: [PATCH 28/50] refactor: refactoring api, helper and view code --- .../core/djangoapps/content_tagging/api.py | 123 +++--------- .../rest_api/v1/objecttag_export_helpers.py | 88 ++++++++ .../v1/tests/test_objecttag_export_helpers.py | 189 ++++++++++++++++++ .../rest_api/v1/tests/test_views.py | 2 +- .../content_tagging/rest_api/v1/views.py | 31 +-- .../content_tagging/tests/test_api.py | 165 ++++----------- .../core/djangoapps/content_tagging/types.py | 16 +- 7 files changed, 351 insertions(+), 263 deletions(-) create mode 100644 openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py create mode 100644 openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 26e26c983f6b..b9bae8f5633a 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -6,19 +6,13 @@ from itertools import groupby import openedx_tagging.core.tagging.api as oel_tagging -from django.db.models import Q, QuerySet, Exists, OuterRef +from django.db.models import Exists, OuterRef, Q, QuerySet from opaque_keys.edx.keys import CourseKey, LearningContextKey from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization -from xmodule.modulestore.django import modulestore from .models import ContentObjectTag, TaxonomyOrg -from .types import ( - ContentKey, - ObjectTagByObjectIdDict, - TaggedContent, - TaxonomyDict, -) +from .types import ContentKey, ObjectTagByObjectIdDict, TaxonomyDict def create_taxonomy( @@ -190,106 +184,39 @@ def tag_content_object( return get_content_tags(object_key, taxonomy_id=taxonomy.id) -def get_object_tree_with_objecttags( +def get_all_object_tags( content_key: LearningContextKey, -) -> tuple[TaggedContent, TaxonomyDict]: +) -> tuple[ObjectTagByObjectIdDict, TaxonomyDict]: """ - Returns the object with the tags associated with it. + Returns a tuple with a dictionary of grouped object tags for all blocks and a dictionary of taxonomies. """ - - def _get_object_tags(content_key: LearningContextKey) -> QuerySet[ObjectTag]: - """ - Return the tags for the object and its children using a single db query. - """ - content_key_str = str(content_key) - + # ToDo: Add support for other content types (like LibraryContent and LibraryBlock) + 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. - # ToDo: Add support for other content types (like LibraryContent and LibraryBlock) - if isinstance(content_key, CourseKey): - course_key_str = content_key_str - block_id_prefix = content_key_str.replace("course-v1:", "block-v1:", 1) - else: - raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") - - return ObjectTag.objects.filter( - Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str), - Q(tag__isnull=False, tag__taxonomy__isnull=False), - ).select_related("tag__taxonomy") - - def _group_object_tags_by_objectid_taxonomy( - all_object_tags: list[ObjectTag] - ) -> tuple[ObjectTagByObjectIdDict, TaxonomyDict]: - """ - Returns a tuple with a dictionary of grouped object tags for all blocks and a dictionary of taxonomies. - """ - - grouped_object_tags: ObjectTagByObjectIdDict = {} - taxonomies: TaxonomyDict = {} - - for object_id, block_tags in groupby(all_object_tags, lambda x: x.object_id): - grouped_object_tags[object_id] = {} - for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): - object_tags_list = list(taxonomy_tags) - grouped_object_tags[object_id][taxonomy_id] = object_tags_list - - if taxonomy_id not in taxonomies: - taxonomies[taxonomy_id] = object_tags_list[0].tag.taxonomy - - return grouped_object_tags, taxonomies - - def _build_object_tree_with_objecttags( - content_key: LearningContextKey, - objectTagCache: ObjectTagByObjectIdDict, - store - ) -> TaggedContent: - """ - Returns the object with the tags associated with it. - """ - if isinstance(content_key, CourseKey): - course = store.get_course(content_key) - else: - raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") - - display_name = course.display_name_with_default - course_id = str(course.id) - - tagged_course = TaggedContent( - display_name=display_name, - block_id=course_id, - category=course.category, - object_tags=objectTagCache.get(str(content_key), {}), - children=None, - ) - - blocks = [(tagged_course, course)] - - while blocks: - tagged_block, xblock = 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=objectTagCache.get(str(child_id), {}), - children=None, - ) - tagged_block.children.append(tagged_child) + block_id_prefix = course_key_str.replace("course-v1:", "block-v1:", 1) + else: + raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") - blocks.append((tagged_child, child_block)) + all_object_tags = list(ObjectTag.objects.filter( + Q(object_id__startswith=block_id_prefix) | Q(object_id=course_key_str), + Q(tag__isnull=False, tag__taxonomy__isnull=False), + ).select_related("tag__taxonomy")) - return tagged_course + grouped_object_tags: ObjectTagByObjectIdDict = {} + taxonomies: TaxonomyDict = {} - store = modulestore() + for object_id, block_tags in groupby(all_object_tags, lambda x: x.object_id): + grouped_object_tags[object_id] = {} + for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): + object_tags_list = list(taxonomy_tags) + grouped_object_tags[object_id][taxonomy_id] = object_tags_list - all_blocks_tag_records = list(_get_object_tags(content_key)) - objectTagCache, taxonomies = _group_object_tags_by_objectid_taxonomy(all_blocks_tag_records) + if taxonomy_id not in taxonomies: + taxonomies[taxonomy_id] = object_tags_list[0].tag.taxonomy - return _build_object_tree_with_objecttags(content_key, objectTagCache, store), taxonomies + return grouped_object_tags, taxonomies # Expose the oel_tagging APIs 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 new file mode 100644 index 000000000000..4a447467206d --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/objecttag_export_helpers.py @@ -0,0 +1,88 @@ +""" +This module contains helper functions to build a object tree with object tags. +""" + +from __future__ import annotations + +from typing import Iterator + +from attrs import define +from opaque_keys.edx.keys import CourseKey, LearningContextKey + +from xmodule.modulestore.django import modulestore + +from ...types import ObjectTagByObjectIdDict, ObjectTagByTaxonomyIdDict + + +@define +class TaggedContent: + """ + A tagged content, with its tags and children. + """ + display_name: str + block_id: str + category: str + object_tags: ObjectTagByTaxonomyIdDict + children: list[TaggedContent] | None + + +def iterate_with_level( + tagged_content: TaggedContent, level: int = 0 +) -> Iterator[tuple[TaggedContent, int]]: + """ + Iterator that yields the tagged content and the level of the block + """ + yield tagged_content, level + if tagged_content.children: + for child in tagged_content.children: + yield from iterate_with_level(child, level + 1) + + +def build_object_tree_with_objecttags( + content_key: LearningContextKey, + objectTagCache: 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}") + else: + raise NotImplementedError(f"Invalid content_key: {type(content_key)} -> {content_key}") + + display_name = course.display_name_with_default + course_id = str(course.id) + + tagged_course = TaggedContent( + display_name=display_name, + block_id=course_id, + category=course.category, + object_tags=objectTagCache.get(str(content_key), {}), + children=None, + ) + + blocks = [(tagged_course, course)] + + while blocks: + tagged_block, xblock = 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=objectTagCache.get(str(child_id), {}), + children=None, + ) + tagged_block.children.append(tagged_child) + + blocks.append((tagged_child, child_block)) + + return tagged_course 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 new file mode 100644 index 000000000000..963b4d8201d2 --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_objecttag_export_helpers.py @@ -0,0 +1,189 @@ +""" +Test the objecttag_export_helpers module +""" +from unittest.mock import patch + +from openedx_tagging.core.tagging.models import Tag +from organizations.models import Organization + +from common.djangoapps.student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase + +from .... import api +from ..objecttag_export_helpers import TaggedContent, build_object_tree_with_objecttags, iterate_with_level + + +class TaggedCourseMixin(ModuleStoreTestCase): + """ + Mixin with a course structure and taxonomies + """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + super().setUp() + # Create user + self.user = UserFactory.create() + self.user_id = self.user.id + + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") + self.taxonomy_1 = api.create_taxonomy(name="Taxonomy 1") + api.set_taxonomy_orgs(self.taxonomy_1, all_orgs=True) + Tag.objects.create( + taxonomy=self.taxonomy_1, + value="Tag 1.1", + ) + Tag.objects.create( + taxonomy=self.taxonomy_1, + value="Tag 1.2", + ) + + self.taxonomy_2 = api.create_taxonomy(name="Taxonomy 2") + api.set_taxonomy_orgs(self.taxonomy_2, all_orgs=True) + + Tag.objects.create( + taxonomy=self.taxonomy_2, + value="Tag 2.1", + ) + Tag.objects.create( + taxonomy=self.taxonomy_2, + value="Tag 2.2", + ) + + self.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) + self.addCleanup(self.patcher.stop) + self.patcher.start() + + # Create course + self.course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={'display_name': "Test Course"}, + ) + course_tags = api.tag_content_object( + object_key=self.course.id, + taxonomy=self.taxonomy_1, + tags=['Tag 1.1'], + ) + + self.expected_tagged_xblock = TaggedContent( + display_name=self.course.display_name_with_default, + block_id=str(self.course.id), + category=self.course.category, + children=[], + object_tags={ + self.taxonomy_1.id: list(course_tags), + }, + ) + + # Create XBlocks + self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") + # Tag blocks + sequential_tags1 = api.tag_content_object( + object_key=self.sequential.location, + taxonomy=self.taxonomy_1, + tags=['Tag 1.1', 'Tag 1.2'], + ) + sequential_tags2 = api.tag_content_object( + object_key=self.sequential.location, + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + xblock = self.store.get_item(self.sequential.location) + tagged_sequential = TaggedContent( + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, + children=[], + object_tags={ + self.taxonomy_1.id: list(sequential_tags1), + self.taxonomy_2.id: list(sequential_tags2), + }, + ) + + assert self.expected_tagged_xblock.children is not None # type guard + self.expected_tagged_xblock.children.append(tagged_sequential) + + vertical = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical1") + vertical_tags = api.tag_content_object( + object_key=vertical.location, + taxonomy=self.taxonomy_2, + tags=['Tag 2.2'], + ) + xblock = self.store.get_item(vertical.location) + tagged_vertical = TaggedContent( + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, + children=[], + object_tags={ + self.taxonomy_2.id: list(vertical_tags), + }, + ) + + assert tagged_sequential.children is not None # type guard + tagged_sequential.children.append(tagged_vertical) + + vertical2 = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical2") + xblock = self.store.get_item(vertical2.location) + tagged_vertical2 = TaggedContent( + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, + children=[], + object_tags={}, + ) + assert tagged_sequential.children is not None # type guard + tagged_sequential.children.append(tagged_vertical2) + + html = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") + html_tags = api.tag_content_object( + object_key=html.location, + taxonomy=self.taxonomy_2, + tags=['Tag 2.1'], + ) + xblock = self.store.get_item(html.location) + tagged_text = TaggedContent( + display_name=xblock.display_name_with_default, + block_id=str(xblock.location), + category=xblock.category, + children=[], + object_tags={ + self.taxonomy_2.id: list(html_tags), + }, + ) + + assert tagged_vertical2.children is not None # type guard + tagged_vertical2.children.append(tagged_text) + + self.all_object_tags, _ = api.get_all_object_tags(self.course.id) + self.expected_tagged_content_list = [ + (self.expected_tagged_xblock, 0), + (tagged_sequential, 1), + (tagged_vertical, 2), + (tagged_vertical2, 2), + (tagged_text, 3), + ] + + +class TestContentTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] + """ + Test helper functions for exporting tagged content + """ + def test_build_object_tree(self) -> None: + """ + Test if we can export a course + """ + # 2 from get_course() + with self.assertNumQueries(2): + tagged_xblock = build_object_tree_with_objecttags(self.course.id, self.all_object_tags) + + assert tagged_xblock == self.expected_tagged_xblock + + def test_iterate_with_level(self) -> None: + """ + Test if we can iterate over the tagged content in the correct order + """ + tagged_content_list = list(iterate_with_level(self.expected_tagged_xblock)) + assert tagged_content_list == self.expected_tagged_content_list 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 6566ec3d22cc..740a4ed801e8 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 @@ -40,7 +40,7 @@ from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.lib import blockstore_api -from ....tests.test_api import TaggedCourseMixin +from .test_objecttag_export_helpers import TaggedCourseMixin User = get_user_model() 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 d4b28a23c9c4..7bebd805c50f 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -20,7 +20,7 @@ from ...api import ( create_taxonomy, - get_object_tree_with_objecttags, + get_all_object_tags, get_taxonomy, get_taxonomies, get_taxonomies_for_org, @@ -28,13 +28,10 @@ set_taxonomy_orgs, ) from ...rules import get_admin_orgs -from ...types import TaggedContent, TaxonomyDict -from .serializers import ( - TaxonomyOrgListQueryParamsSerializer, - TaxonomyOrgSerializer, - TaxonomyUpdateOrgBodySerializer, -) +from ...types import TaxonomyDict from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend +from .objecttag_export_helpers import TaggedContent, build_object_tree_with_objecttags, iterate_with_level +from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer class TaxonomyOrgView(TaxonomyView): @@ -160,17 +157,6 @@ def get(self, request: Request, **kwargs) -> StreamingHttpResponse: Export a CSV with all children and tags for a given object_id. """ - def _content_generator( - tagged_content: TaggedContent, level: int = 0 - ) -> Iterator[tuple[TaggedContent, int]]: - """ - Generator that yields the tagged content and the level of the block - """ - yield tagged_content, level - if tagged_content.children: - for child in tagged_content.children: - yield from _content_generator(child, level + 1) - class Echo(object): """ Class that implements just the write method of the file-like interface, @@ -199,7 +185,7 @@ def _generate_csv_rows( yield csv_writer.writerow(header) # Iterate over the blocks and yield the rows - for item, level in _content_generator(tagged_content): + for item, level in iterate_with_level(tagged_content): block_data = { "name": level * " " + item.display_name, "type": item.category, @@ -218,8 +204,6 @@ def _generate_csv_rows( object_id: str = kwargs.get('object_id', None) - content_key: CourseKey - try: content_key = CourseKey.from_string(object_id) except InvalidKeyError as e: @@ -238,10 +222,11 @@ def _generate_csv_rows( except ValueError as e: raise ValidationError from e - tagged_block, taxonomies = get_object_tree_with_objecttags(content_key) + all_object_tags, taxonomies = get_all_object_tags(content_key) + tagged_content = build_object_tree_with_objecttags(content_key, all_object_tags) return StreamingHttpResponse( - streaming_content=_generate_csv_rows(tagged_block, taxonomies, Echo()), + streaming_content=_generate_csv_rows(tagged_content, taxonomies, Echo()), content_type="text/csv", headers={'Content-Disposition': f'attachment; filename="{object_id}_tags.csv"'}, ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index dba284ad3506..143a225becad 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -4,13 +4,8 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import Tag from organizations.models import Organization -from unittest.mock import patch - -from common.djangoapps.student.tests.factories import UserFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE from .. import api -from ..types import TaggedContent class TestTaxonomyMixin: @@ -255,18 +250,13 @@ def test_cannot_tag_across_orgs(self): ) -class TaggedCourseMixin(ModuleStoreTestCase): +class TestGetAllObjectTags(TestCase): """ - Mixin with a course structure and taxonomies + Test the get_all_object_tags function """ - MODULESTORE = TEST_DATA_SPLIT_MODULESTORE def setUp(self): super().setUp() - # Create user - self.user = UserFactory.create() - self.user_id = self.user.id - self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") self.taxonomy_1 = api.create_taxonomy(name="Taxonomy 1") api.set_taxonomy_orgs(self.taxonomy_1, all_orgs=True) @@ -291,148 +281,69 @@ def setUp(self): value="Tag 2.2", ) - self.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) - self.addCleanup(self.patcher.stop) - self.patcher.start() - - # Create course - self.course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={'display_name': "Test Course"}, - ) course_tags = api.tag_content_object( - object_key=self.course.id, + object_key=CourseKey.from_string("course-v1:orgA+test_course+test_run"), taxonomy=self.taxonomy_1, tags=['Tag 1.1'], ) - self.expected_tagged_xblock = TaggedContent( - display_name=self.course.display_name_with_default, - block_id=str(self.course.id), - category=self.course.category, - children=[], - object_tags={ - self.taxonomy_1.id: list(course_tags), - }, - ) - - # Create XBlocks - self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") # Tag blocks - sequential_tags1 = api.tag_content_object( - object_key=self.sequential.location, + sequential_tags = api.tag_content_object( + object_key=UsageKey.from_string( + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" + ), taxonomy=self.taxonomy_1, tags=['Tag 1.1', 'Tag 1.2'], ) sequential_tags2 = api.tag_content_object( - object_key=self.sequential.location, + object_key=UsageKey.from_string( + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" + ), taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - xblock = self.store.get_item(self.sequential.location) - tagged_sequential = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, - children=[], - object_tags={ - self.taxonomy_1.id: list(sequential_tags1), - self.taxonomy_2.id: list(sequential_tags2), - }, - ) - - assert self.expected_tagged_xblock.children is not None # type guard - self.expected_tagged_xblock.children.append(tagged_sequential) - - vertical = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical1") - vertical_tags = api.tag_content_object( - object_key=vertical.location, + vertical1_tags = api.tag_content_object( + object_key=UsageKey.from_string( + "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1" + ), taxonomy=self.taxonomy_2, tags=['Tag 2.2'], ) - xblock = self.store.get_item(vertical.location) - tagged_vertical = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, - children=[], - object_tags={ - self.taxonomy_2.id: list(vertical_tags), - }, - ) - - assert tagged_sequential.children is not None # type guard - tagged_sequential.children.append(tagged_vertical) - - vertical2 = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical2") - xblock = self.store.get_item(vertical2.location) - tagged_vertical2 = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, - children=[], - object_tags={}, - ) - assert tagged_sequential.children is not None # type guard - tagged_sequential.children.append(tagged_vertical2) - - html = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") html_tags = api.tag_content_object( - object_key=html.location, + object_key=UsageKey.from_string( + "block-v1:orgA+test_course+test_run+type@html+block@test_html" + ), taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - xblock = self.store.get_item(html.location) - tagged_text = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, - children=[], - object_tags={ + + self.expected_objecttags = { + "course-v1:orgA+test_course+test_run": { + self.taxonomy_1.id: list(course_tags), + }, + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential": { + self.taxonomy_1.id: list(sequential_tags), + self.taxonomy_2.id: list(sequential_tags2), + }, + "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1": { + self.taxonomy_2.id: list(vertical1_tags), + }, + "block-v1:orgA+test_course+test_run+type@html+block@test_html": { self.taxonomy_2.id: list(html_tags), }, - ) - - assert tagged_vertical2.children is not None # type guard - tagged_vertical2.children.append(tagged_text) - - -@ddt.ddt -class TestContentTagChildrenExport(TaggedCourseMixin): # type: ignore[misc] - """ - Test exporting content objects - """ - def test_export_tagged_course(self) -> None: - """ - Test if we can export a course - """ - # 2 from get_course() / get_item() + 1 from _get_object_tags() - with self.assertNumQueries(3): - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id) - - expected_taxonomies = { - self.taxonomy_1.id: self.taxonomy_1, - self.taxonomy_2.id: self.taxonomy_2, } - assert tagged_xblock == self.expected_tagged_xblock - assert taxonomies == expected_taxonomies - - def test_export_tagged_block(self) -> None: + def test_get_all_object_tags(self): """ - Test if we can export a course + Test the get_all_object_tags function """ - # 2 from get_course() / get_item() + 1 from _get_object_tags() - with self.assertNumQueries(3): - tagged_xblock, taxonomies = api.get_object_tree_with_objecttags(self.course.id) + with self.assertNumQueries(1): + object_tags, taxonomies = api.get_all_object_tags( + CourseKey.from_string("course-v1:orgA+test_course+test_run") + ) - expected_taxonomies = { + assert object_tags == self.expected_objecttags + assert taxonomies == { self.taxonomy_1.id: self.taxonomy_1, self.taxonomy_2.id: self.taxonomy_2, } - - assert tagged_xblock == self.expected_tagged_xblock - assert taxonomies == expected_taxonomies diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index b4da0ec9bc57..aa9a2179d612 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -2,9 +2,9 @@ Types used by content tagging API and implementation """ from __future__ import annotations -from typing import Union, Dict, List -from attrs import define +from typing import Dict, List, Union + from opaque_keys.edx.keys import LearningContextKey, UsageKey from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy @@ -13,15 +13,3 @@ ObjectTagByTaxonomyIdDict = Dict[int, List[ObjectTag]] ObjectTagByObjectIdDict = Dict[str, ObjectTagByTaxonomyIdDict] TaxonomyDict = Dict[int, Taxonomy] - - -@define -class TaggedContent: - """ - A tagged content, with its tags and children. - """ - display_name: str - block_id: str - category: str - object_tags: ObjectTagByTaxonomyIdDict - children: list[TaggedContent] | None From fabb72918cf6c4f53787b6728275c7f440a45ab6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 31 Jan 2024 17:27:03 -0300 Subject: [PATCH 29/50] docs: add comment about ObjectTag query --- openedx/core/djangoapps/content_tagging/api.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index b9bae8f5633a..1cf529dcf0a3 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -199,6 +199,8 @@ def get_all_object_tags( 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(tag__isnull=False, tag__taxonomy__isnull=False), From 548d57c34104f54698d276c7d1d9191094f3877e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 31 Jan 2024 06:48:26 +1030 Subject: [PATCH 30/50] test: use CourseFactory and BlockFactory --- .../v1/tests/test_objecttag_export_helpers.py | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) 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 963b4d8201d2..c732837ba8f1 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 @@ -8,6 +8,7 @@ from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory from .... import api from ..objecttag_export_helpers import TaggedContent, build_object_tree_with_objecttags, iterate_with_level @@ -23,7 +24,6 @@ def setUp(self): super().setUp() # Create user self.user = UserFactory.create() - self.user_id = self.user.id self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") self.taxonomy_1 = api.create_taxonomy(name="Taxonomy 1") @@ -54,12 +54,11 @@ def setUp(self): self.patcher.start() # Create course - self.course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={'display_name': "Test Course"}, + self.course = CourseFactory.create( + org=self.orgA.short_name, + number="test_course", + run="test_run", + display_name="Test Course", ) course_tags = api.tag_content_object( object_key=self.course.id, @@ -78,7 +77,11 @@ def setUp(self): ) # Create XBlocks - self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") + self.sequential = BlockFactory.create( + parent=self.course, + category="sequential", + display_name="test sequential", + ) # Tag blocks sequential_tags1 = api.tag_content_object( object_key=self.sequential.location, @@ -105,7 +108,11 @@ def setUp(self): assert self.expected_tagged_xblock.children is not None # type guard self.expected_tagged_xblock.children.append(tagged_sequential) - vertical = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical1") + vertical = BlockFactory.create( + parent=self.sequential, + category="vertical", + display_name="test vertical1", + ) vertical_tags = api.tag_content_object( object_key=vertical.location, taxonomy=self.taxonomy_2, @@ -125,7 +132,11 @@ def setUp(self): assert tagged_sequential.children is not None # type guard tagged_sequential.children.append(tagged_vertical) - vertical2 = self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical2") + vertical2 = BlockFactory.create( + parent=self.sequential, + category="vertical", + display_name="test vertical2", + ) xblock = self.store.get_item(vertical2.location) tagged_vertical2 = TaggedContent( display_name=xblock.display_name_with_default, @@ -137,7 +148,11 @@ def setUp(self): assert tagged_sequential.children is not None # type guard tagged_sequential.children.append(tagged_vertical2) - html = self.store.create_child(self.user_id, vertical2.location, "html", "test_html") + html = BlockFactory.create( + parent=vertical2, + category="html", + display_name="test html", + ) html_tags = api.tag_content_object( object_key=html.location, taxonomy=self.taxonomy_2, From 30e06d0e2d890682f8737cb5b397ac724ba67835 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 31 Jan 2024 06:49:25 +1030 Subject: [PATCH 31/50] test: compare results to hardcoded strings instead of reproducing feature logic in the tests --- .../v1/tests/test_objecttag_export_helpers.py | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) 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 c732837ba8f1..7cf8b205e687 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 @@ -67,9 +67,9 @@ def setUp(self): ) self.expected_tagged_xblock = TaggedContent( - display_name=self.course.display_name_with_default, - block_id=str(self.course.id), - category=self.course.category, + display_name="Test Course", + block_id="course-v1:orgA+test_course+test_run", + category="course", children=[], object_tags={ self.taxonomy_1.id: list(course_tags), @@ -93,11 +93,10 @@ def setUp(self): taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - xblock = self.store.get_item(self.sequential.location) tagged_sequential = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, + display_name="test sequential", + block_id="block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + category="sequential", children=[], object_tags={ self.taxonomy_1.id: list(sequential_tags1), @@ -118,11 +117,10 @@ def setUp(self): taxonomy=self.taxonomy_2, tags=['Tag 2.2'], ) - xblock = self.store.get_item(vertical.location) tagged_vertical = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, + display_name="test vertical1", + block_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", + category="vertical", children=[], object_tags={ self.taxonomy_2.id: list(vertical_tags), @@ -137,11 +135,10 @@ def setUp(self): category="vertical", display_name="test vertical2", ) - xblock = self.store.get_item(vertical2.location) tagged_vertical2 = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, + display_name="test vertical2", + block_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical2", + category="vertical", children=[], object_tags={}, ) @@ -158,11 +155,10 @@ def setUp(self): taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - xblock = self.store.get_item(html.location) tagged_text = TaggedContent( - display_name=xblock.display_name_with_default, - block_id=str(xblock.location), - category=xblock.category, + display_name="test html", + block_id="block-v1:orgA+test_course+test_run+type@html+block@test_html", + category="html", children=[], object_tags={ self.taxonomy_2.id: list(html_tags), From 35a3d2b187fc583dccaf4f6f7185b07745409964 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 31 Jan 2024 06:50:44 +1030 Subject: [PATCH 32/50] test: Adds "deleted" object tags to ensure they are omitted from results --- .../v1/tests/test_objecttag_export_helpers.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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 7cf8b205e687..96962525ad48 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 @@ -11,6 +11,7 @@ from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory from .... import api +from ....models import ContentObjectTag from ..objecttag_export_helpers import TaggedContent, build_object_tree_with_objecttags, iterate_with_level @@ -168,6 +169,21 @@ def setUp(self): assert tagged_vertical2.children is not None # type guard tagged_vertical2.children.append(tagged_text) + # Create "deleted" object tags, which will be omitted from the results. + for object_id in ( + self.course.id, + self.sequential.location, + vertical.location, + html.location, + ): + ContentObjectTag.objects.create( + object_id=str(object_id), + taxonomy=None, + tag=None, + _value="deleted tag", + _name="deleted taxonomy", + ) + self.all_object_tags, _ = api.get_all_object_tags(self.course.id) self.expected_tagged_content_list = [ (self.expected_tagged_xblock, 0), From 1e13f54f019e2b3ac302f721e82174bb35f9bc3e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 31 Jan 2024 06:51:08 +1030 Subject: [PATCH 33/50] test: adds untagged blocks with children ensures that untagged elements and their children are included in results --- .../v1/tests/test_objecttag_export_helpers.py | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) 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 96962525ad48..b69db294ed55 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 @@ -108,6 +108,37 @@ def setUp(self): assert self.expected_tagged_xblock.children is not None # type guard self.expected_tagged_xblock.children.append(tagged_sequential) + # Untagged blocks + sequential2 = BlockFactory.create( + parent=self.course, + category="sequential", + display_name="untagged sequential", + ) + untagged_sequential = TaggedContent( + display_name="untagged sequential", + block_id="block-v1:orgA+test_course+test_run+type@sequential+block@untagged_sequential", + category="sequential", + children=[], + object_tags={}, + ) + assert self.expected_tagged_xblock.children is not None # type guard + self.expected_tagged_xblock.children.append(untagged_sequential) + BlockFactory.create( + parent=sequential2, + category="vertical", + display_name="untagged vertical", + ) + untagged_vertical = TaggedContent( + display_name="untagged vertical", + block_id="block-v1:orgA+test_course+test_run+type@vertical+block@untagged_vertical", + category="vertical", + children=[], + object_tags={}, + ) + assert untagged_sequential.children is not None # type guard + untagged_sequential.children.append(untagged_vertical) + # /Untagged blocks + vertical = BlockFactory.create( parent=self.sequential, category="vertical", @@ -191,6 +222,8 @@ def setUp(self): (tagged_vertical, 2), (tagged_vertical2, 2), (tagged_text, 3), + (untagged_sequential, 1), + (untagged_vertical, 2), ] From 35bc860cbec803dc916d1b90f86c5e9f123f1643 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 31 Jan 2024 15:35:06 +1030 Subject: [PATCH 34/50] test: refactors tests so shared data can be re-used --- .../v1/tests/test_objecttag_export_helpers.py | 88 ++----------------- .../rest_api/v1/tests/test_views.py | 15 ++-- .../content_tagging/tests/test_api.py | 47 +++++++--- 3 files changed, 49 insertions(+), 101 deletions(-) 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 b50eb0580b81..d52b48a3eaf7 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,19 +3,15 @@ """ from unittest.mock import patch -from openedx_tagging.core.tagging.models import Tag -from organizations.models import Organization - -from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory from .... import api -from ....models import ContentObjectTag +from ....tests.test_api import TestGetAllObjectTagsMixin from ..objecttag_export_helpers import TaggedContent, build_object_tree_with_objecttags, iterate_with_level -class TaggedCourseMixin(ModuleStoreTestCase): +class TaggedCourseMixin(TestGetAllObjectTagsMixin, ModuleStoreTestCase): # type: ignore[misc] """ Mixin with a course structure and taxonomies """ @@ -23,33 +19,8 @@ class TaggedCourseMixin(ModuleStoreTestCase): def setUp(self): super().setUp() - # Create user - self.user = UserFactory.create() - - self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") - self.taxonomy_1 = api.create_taxonomy(name="Taxonomy 1") - api.set_taxonomy_orgs(self.taxonomy_1, all_orgs=True) - Tag.objects.create( - taxonomy=self.taxonomy_1, - value="Tag 1.1", - ) - Tag.objects.create( - taxonomy=self.taxonomy_1, - value="Tag 1.2", - ) - - self.taxonomy_2 = api.create_taxonomy(name="Taxonomy 2") - api.set_taxonomy_orgs(self.taxonomy_2, all_orgs=True) - - Tag.objects.create( - taxonomy=self.taxonomy_2, - value="Tag 2.1", - ) - Tag.objects.create( - taxonomy=self.taxonomy_2, - value="Tag 2.2", - ) + # Patch modulestore self.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) self.addCleanup(self.patcher.stop) self.patcher.start() @@ -61,19 +32,13 @@ def setUp(self): run="test_run", display_name="Test Course", ) - course_tags = api.tag_content_object( - object_key=self.course.id, - taxonomy=self.taxonomy_1, - tags=['Tag 1.1'], - ) - self.expected_tagged_xblock = TaggedContent( display_name="Test Course", block_id="course-v1:orgA+test_course+test_run", category="course", children=[], object_tags={ - self.taxonomy_1.id: list(course_tags), + self.taxonomy_1.id: list(self.course_tags), }, ) @@ -84,24 +49,14 @@ def setUp(self): display_name="test sequential", ) # Tag blocks - sequential_tags1 = api.tag_content_object( - object_key=self.sequential.location, - taxonomy=self.taxonomy_1, - tags=['Tag 1.1', 'Tag 1.2'], - ) - sequential_tags2 = api.tag_content_object( - object_key=self.sequential.location, - taxonomy=self.taxonomy_2, - tags=['Tag 2.1'], - ) tagged_sequential = TaggedContent( display_name="test sequential", block_id="block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", category="sequential", children=[], object_tags={ - self.taxonomy_1.id: list(sequential_tags1), - self.taxonomy_2.id: list(sequential_tags2), + self.taxonomy_1.id: list(self.sequential_tags1), + self.taxonomy_2.id: list(self.sequential_tags2), }, ) @@ -144,21 +99,15 @@ def setUp(self): category="vertical", display_name="test vertical1", ) - vertical_tags = api.tag_content_object( - object_key=vertical.location, - taxonomy=self.taxonomy_2, - tags=['Tag 2.2'], - ) tagged_vertical = TaggedContent( display_name="test vertical1", block_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", category="vertical", children=[], object_tags={ - self.taxonomy_2.id: list(vertical_tags), + self.taxonomy_2.id: list(self.vertical1_tags), }, ) - assert tagged_sequential.children is not None # type guard tagged_sequential.children.append(tagged_vertical) @@ -182,39 +131,18 @@ def setUp(self): category="html", display_name="test html", ) - html_tags = api.tag_content_object( - object_key=html.location, - taxonomy=self.taxonomy_2, - tags=['Tag 2.1'], - ) tagged_text = TaggedContent( display_name="test html", block_id="block-v1:orgA+test_course+test_run+type@html+block@test_html", category="html", children=[], object_tags={ - self.taxonomy_2.id: list(html_tags), + self.taxonomy_2.id: list(self.html_tags), }, ) - assert untagged_vertical2.children is not None # type guard untagged_vertical2.children.append(tagged_text) - # Create "deleted" object tags, which will be omitted from the results. - for object_id in ( - self.course.id, - self.sequential.location, - vertical.location, - html.location, - ): - ContentObjectTag.objects.create( - object_id=str(object_id), - taxonomy=None, - tag=None, - _value="deleted tag", - _name="deleted taxonomy", - ) - self.all_object_tags, _ = api.get_all_object_tags(self.course.id) self.expected_tagged_content_list = [ (self.expected_tagged_xblock, 0), 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 0c151070eac5..6346adcebe88 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 @@ -4,14 +4,13 @@ from __future__ import annotations +import abc import json from io import BytesIO -from urllib.parse import parse_qs, urlparse from unittest.mock import MagicMock +from urllib.parse import parse_qs, urlparse -import abc import ddt -from common.djangoapps.student.tests.factories import UserFactory from django.contrib.auth import get_user_model from django.core.files.uploadedfile import SimpleUploadedFile from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator @@ -29,13 +28,10 @@ OrgContentCreatorRole, OrgInstructorRole, OrgLibraryUserRole, - OrgStaffRole, -) -from openedx.core.djangoapps.content_libraries.api import ( - AccessLevel, - create_library, - set_library_user_permissions, + OrgStaffRole ) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries.api import AccessLevel, create_library, set_library_user_permissions from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -1637,6 +1633,7 @@ class TestContentObjectChildrenExportView(TaggedCourseMixin, APITestCase): # ty """ def setUp(self): super().setUp() + self.user = UserFactory.create() self.staff = UserFactory.create( username="staff", email="staff@example.com", diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 143a225becad..76502163f2f4 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -6,6 +6,7 @@ from organizations.models import Organization from .. import api +from ..models import ContentObjectTag class TestTaxonomyMixin: @@ -250,13 +251,14 @@ def test_cannot_tag_across_orgs(self): ) -class TestGetAllObjectTags(TestCase): +class TestGetAllObjectTagsMixin: """ - Test the get_all_object_tags function + Set up data to test get_all_object_tags functions """ def setUp(self): super().setUp() + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") self.taxonomy_1 = api.create_taxonomy(name="Taxonomy 1") api.set_taxonomy_orgs(self.taxonomy_1, all_orgs=True) @@ -281,35 +283,35 @@ def setUp(self): value="Tag 2.2", ) - course_tags = api.tag_content_object( + self.course_tags = api.tag_content_object( object_key=CourseKey.from_string("course-v1:orgA+test_course+test_run"), taxonomy=self.taxonomy_1, tags=['Tag 1.1'], ) # Tag blocks - sequential_tags = api.tag_content_object( + self.sequential_tags1 = api.tag_content_object( object_key=UsageKey.from_string( "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" ), taxonomy=self.taxonomy_1, tags=['Tag 1.1', 'Tag 1.2'], ) - sequential_tags2 = api.tag_content_object( + self.sequential_tags2 = api.tag_content_object( object_key=UsageKey.from_string( "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" ), taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - vertical1_tags = api.tag_content_object( + self.vertical1_tags = api.tag_content_object( object_key=UsageKey.from_string( "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1" ), taxonomy=self.taxonomy_2, tags=['Tag 2.2'], ) - html_tags = api.tag_content_object( + self.html_tags = api.tag_content_object( object_key=UsageKey.from_string( "block-v1:orgA+test_course+test_run+type@html+block@test_html" ), @@ -317,22 +319,43 @@ def setUp(self): tags=['Tag 2.1'], ) + # Create "deleted" object tags, which will be omitted from the results. + for object_id in ( + "course-v1:orgA+test_course+test_run", + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", + "block-v1:orgA+test_course+test_run+type@html+block@test_html", + ): + ContentObjectTag.objects.create( + object_id=str(object_id), + taxonomy=None, + tag=None, + _value="deleted tag", + _name="deleted taxonomy", + ) + self.expected_objecttags = { "course-v1:orgA+test_course+test_run": { - self.taxonomy_1.id: list(course_tags), + self.taxonomy_1.id: list(self.course_tags), }, "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential": { - self.taxonomy_1.id: list(sequential_tags), - self.taxonomy_2.id: list(sequential_tags2), + self.taxonomy_1.id: list(self.sequential_tags1), + self.taxonomy_2.id: list(self.sequential_tags2), }, "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1": { - self.taxonomy_2.id: list(vertical1_tags), + self.taxonomy_2.id: list(self.vertical1_tags), }, "block-v1:orgA+test_course+test_run+type@html+block@test_html": { - self.taxonomy_2.id: list(html_tags), + self.taxonomy_2.id: list(self.html_tags), }, } + +class TestGetAllObjectTags(TestGetAllObjectTagsMixin, TestCase): + """ + Test get_all_object_tags api function + """ + def test_get_all_object_tags(self): """ Test the get_all_object_tags function From a1d41fda78f47aa51d856347c87cfc33ecd7634e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 12:12:33 -0300 Subject: [PATCH 35/50] test: fix variable name --- .../rest_api/v1/tests/test_objecttag_export_helpers.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 b69db294ed55..3e0bf4d9660f 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 @@ -167,7 +167,7 @@ def setUp(self): category="vertical", display_name="test vertical2", ) - tagged_vertical2 = TaggedContent( + untagged_vertical2 = TaggedContent( display_name="test vertical2", block_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical2", category="vertical", @@ -175,7 +175,7 @@ def setUp(self): object_tags={}, ) assert tagged_sequential.children is not None # type guard - tagged_sequential.children.append(tagged_vertical2) + tagged_sequential.children.append(untagged_vertical2) html = BlockFactory.create( parent=vertical2, @@ -197,8 +197,8 @@ def setUp(self): }, ) - assert tagged_vertical2.children is not None # type guard - tagged_vertical2.children.append(tagged_text) + assert untagged_vertical2.children is not None # type guard + untagged_vertical2.children.append(tagged_text) # Create "deleted" object tags, which will be omitted from the results. for object_id in ( @@ -220,7 +220,7 @@ def setUp(self): (self.expected_tagged_xblock, 0), (tagged_sequential, 1), (tagged_vertical, 2), - (tagged_vertical2, 2), + (untagged_vertical2, 2), (tagged_text, 3), (untagged_sequential, 1), (untagged_vertical, 2), From f07b841c32dd1825ff686337e009a073f3545a97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 12:12:42 -0300 Subject: [PATCH 36/50] fix: delete unwanted file --- uwsgi.ini | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100755 uwsgi.ini diff --git a/uwsgi.ini b/uwsgi.ini deleted file mode 100755 index e69de29bb2d1..000000000000 From 233135a777c40e02a5906549ded51c651304980a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 12:13:22 -0300 Subject: [PATCH 37/50] test: fix query count --- .../rest_api/v1/tests/test_objecttag_export_helpers.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 3e0bf4d9660f..b50eb0580b81 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 @@ -235,8 +235,7 @@ def test_build_object_tree(self) -> None: """ Test if we can export a course """ - # 2 from get_course() - with self.assertNumQueries(2): + with self.assertNumQueries(3): tagged_xblock = build_object_tree_with_objecttags(self.course.id, self.all_object_tags) assert tagged_xblock == self.expected_tagged_xblock From ac98812cbe2c559172e4f79475057bad752eb388 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 12:20:10 -0300 Subject: [PATCH 38/50] test: fix expected value with new tags --- .../content_tagging/rest_api/v1/tests/test_views.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 740a4ed801e8..8c6b8e1d0e1c 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 @@ -1674,7 +1674,11 @@ def test_export_course(self, user_attr) -> None: 'vertical1","","Tag 2.2"\r\n' '" test vertical2","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@test_' 'vertical2","",""\r\n' - '" Text","html","block-v1:orgA+test_course+test_run+type@html+block@test_html","","Tag 2.1"\r\n' + '" test html","html","block-v1:orgA+test_course+test_run+type@html+block@test_html","","Tag 2.1"\r\n' + '" untagged sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@untagged_' + 'sequential","",""\r\n' + '" untagged vertical","vertical","block-v1:orgA+test_course+test_run+type@vertical+block@untagged_' + 'vertical","",""\r\n' ) zip_content = BytesIO(b"".join(response.streaming_content)).getvalue() # type: ignore[attr-defined] From 309ce943808ea57b7c43db88020728629bf07264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 13:08:57 -0300 Subject: [PATCH 39/50] fix: use variables from outer function --- .../core/djangoapps/content_tagging/rest_api/v1/views.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) 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 7bebd805c50f..4e225d967cb3 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -165,11 +165,7 @@ class Echo(object): def write(self, value): return value - def _generate_csv_rows( - tagged_content: TaggedContent, - taxonomies: TaxonomyDict, - pseudo_buffer: Echo, - ) -> Iterator[str]: + def _generate_csv_rows() -> Iterator[str]: """ Receives the blocks, tags and taxonomies and returns a CSV string """ @@ -224,9 +220,10 @@ def _generate_csv_rows( 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( - streaming_content=_generate_csv_rows(tagged_content, taxonomies, Echo()), + streaming_content=_generate_csv_rows(), content_type="text/csv", headers={'Content-Disposition': f'attachment; filename="{object_id}_tags.csv"'}, ) From 32cdf9356801bdc9baa44f5f05c230e1f151bc77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 13:25:53 -0300 Subject: [PATCH 40/50] test: use UserFactory --- .../content_tagging/rest_api/v1/tests/test_views.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) 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 8c6b8e1d0e1c..0c151070eac5 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 @@ -11,6 +11,7 @@ import abc import ddt +from common.djangoapps.student.tests.factories import UserFactory from django.contrib.auth import get_user_model from django.core.files.uploadedfile import SimpleUploadedFile from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator @@ -1636,17 +1637,13 @@ class TestContentObjectChildrenExportView(TaggedCourseMixin, APITestCase): # ty """ def setUp(self): super().setUp() - self.user = User.objects.create( - username="user", - email="user@example.com", - ) - self.staff = User.objects.create( + self.staff = UserFactory.create( username="staff", email="staff@example.com", is_staff=True, ) - self.staffA = User.objects.create( + self.staffA = UserFactory.create( username="staffA", email="userA@example.com", ) From 4ed7570ab84f269399ffab82e9302799a53f7fa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 14:58:19 -0300 Subject: [PATCH 41/50] style: removed unused imports --- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 4e225d967cb3..6702c9d2191f 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -28,9 +28,8 @@ set_taxonomy_orgs, ) from ...rules import get_admin_orgs -from ...types import TaxonomyDict from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend -from .objecttag_export_helpers import TaggedContent, build_object_tree_with_objecttags, iterate_with_level +from .objecttag_export_helpers import build_object_tree_with_objecttags, iterate_with_level from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer From ee5bc3defa47cd14914ccd261968497343a1a29c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 16:08:32 -0300 Subject: [PATCH 42/50] chore: trigger CI From 6d4c23ae2e1a12b3ff9c99e3e4324cb7fcca390b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 2 Feb 2024 10:50:55 -0300 Subject: [PATCH 43/50] fix: disable default staff user from module store mixin --- .../rest_api/v1/tests/test_objecttag_export_helpers.py | 1 + 1 file changed, 1 insertion(+) 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 b50eb0580b81..90082a477427 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 @@ -20,6 +20,7 @@ class TaggedCourseMixin(ModuleStoreTestCase): Mixin with a course structure and taxonomies """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + CREATE_USER = False def setUp(self): super().setUp() From da3fdf92ea4d16151236c1be926225f50dfc4dbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 5 Feb 2024 18:49:38 -0300 Subject: [PATCH 44/50] style: fix case --- .../content_tagging/rest_api/v1/objecttag_export_helpers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 4a447467206d..14103642d8ec 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 @@ -40,7 +40,7 @@ def iterate_with_level( def build_object_tree_with_objecttags( content_key: LearningContextKey, - objectTagCache: ObjectTagByObjectIdDict, + object_tag_cache: ObjectTagByObjectIdDict, ) -> TaggedContent: """ Returns the object with the tags associated with it. @@ -61,7 +61,7 @@ def build_object_tree_with_objecttags( display_name=display_name, block_id=course_id, category=course.category, - object_tags=objectTagCache.get(str(content_key), {}), + object_tags=object_tag_cache.get(str(content_key), {}), children=None, ) @@ -78,7 +78,7 @@ def build_object_tree_with_objecttags( display_name=child_block.display_name_with_default, block_id=str(child_id), category=child_block.category, - object_tags=objectTagCache.get(str(child_id), {}), + object_tags=object_tag_cache.get(str(child_id), {}), children=None, ) tagged_block.children.append(tagged_child) From fd5a542afc6e3420e2861f6af94294f4b7774a2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 6 Feb 2024 18:02:34 -0300 Subject: [PATCH 45/50] docs: adds removed docstring --- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 1 + 1 file changed, 1 insertion(+) 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 6702c9d2191f..90ab1906a1f0 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -141,6 +141,7 @@ 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. """ From dfe43bed6978661e4c4871bbb176d5d5edb9d3f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 8 Feb 2024 19:42:32 -0300 Subject: [PATCH 46/50] fix: cleaning merged code --- .../content_tagging/tests/test_api.py | 76 +++++++------------ 1 file changed, 27 insertions(+), 49 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index ba0027ac8aa3..37d40ad61c24 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,11 +1,11 @@ """Tests for the Tagging models""" import ddt from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import Tag +from opaque_keys.edx.keys import CourseKey +from openedx_tagging.core.tagging.models import ObjectTag, Tag from organizations.models import Organization from .. import api -from ..models import ContentObjectTag class TestTaxonomyMixin: @@ -233,34 +233,6 @@ def test_get_tags(self): assert result[0]["parent_value"] is None assert result[0]["depth"] == 0 - def test_cannot_tag_across_orgs(self): - """ - Ensure that I cannot apply tags from a taxonomy that's linked to another - org. - """ - # This taxonomy is only linked to the "OpenedX org", so it can't be used for "Axim" content. - taxonomy = self.taxonomy_one_org - tags = [self.tag_one_org.value] - with self.assertRaises(ValueError) as exc: - api.tag_content_object( - object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), - taxonomy=taxonomy, - tags=tags, - ) - assert "The specified Taxonomy is not enabled for the content object's org (Ax)" in str(exc.exception) - # But this will work fine: - api.tag_content_object( - object_key=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), - taxonomy=taxonomy, - tags=tags, - ) - # As will this: - api.tag_content_object( - object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), - taxonomy=self.taxonomy_both_orgs, - tags=[self.tag_both_orgs.value], - ) - class TestGetAllObjectTagsMixin: """ @@ -294,41 +266,47 @@ def setUp(self): value="Tag 2.2", ) - self.course_tags = api.tag_content_object( - object_key=CourseKey.from_string("course-v1:orgA+test_course+test_run"), + api.tag_object( + object_id="course-v1:orgA+test_course+test_run", taxonomy=self.taxonomy_1, tags=['Tag 1.1'], ) + self.course_tags = api.get_object_tags("course-v1:orgA+test_course+test_run") # Tag blocks - self.sequential_tags1 = api.tag_content_object( - object_key=UsageKey.from_string( - "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" - ), + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", taxonomy=self.taxonomy_1, tags=['Tag 1.1', 'Tag 1.2'], ) - self.sequential_tags2 = api.tag_content_object( - object_key=UsageKey.from_string( - "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" - ), + self.sequential_tags1 = api.get_object_tags( + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + taxonomy_id=self.taxonomy_1.id, + + ) + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) - self.vertical1_tags = api.tag_content_object( - object_key=UsageKey.from_string( - "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1" - ), + self.sequential_tags2 = api.get_object_tags( + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential", + taxonomy_id=self.taxonomy_2.id, + ) + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", taxonomy=self.taxonomy_2, tags=['Tag 2.2'], ) - self.html_tags = api.tag_content_object( - object_key=UsageKey.from_string( - "block-v1:orgA+test_course+test_run+type@html+block@test_html" - ), + self.vertical1_tags = api.get_object_tags( + "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1" + ) + api.tag_object( + object_id="block-v1:orgA+test_course+test_run+type@html+block@test_html", taxonomy=self.taxonomy_2, tags=['Tag 2.1'], ) + self.html_tags = api.get_object_tags("block-v1:orgA+test_course+test_run+type@html+block@test_html") # Create "deleted" object tags, which will be omitted from the results. for object_id in ( @@ -337,7 +315,7 @@ def setUp(self): "block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1", "block-v1:orgA+test_course+test_run+type@html+block@test_html", ): - ContentObjectTag.objects.create( + ObjectTag.objects.create( object_id=str(object_id), taxonomy=None, tag=None, From 52452641a8a67ff9e459d788a45014db09f281da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 8 Feb 2024 19:46:49 -0300 Subject: [PATCH 47/50] style: run isort --- .../djangoapps/content_tagging/rest_api/v1/urls.py | 12 ++++-------- .../djangoapps/content_tagging/rest_api/v1/views.py | 4 ++-- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py index 47cf07944c52..7e0ab60e41b4 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py @@ -2,15 +2,11 @@ Taxonomies API v1 URLs. """ -from rest_framework.routers import DefaultRouter +from django.urls.conf import include, path +from openedx_tagging.core.tagging.rest_api.v1 import views as oel_tagging_views +from openedx_tagging.core.tagging.rest_api.v1 import views_import as oel_tagging_views_import from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagCountsView - -from django.urls.conf import path, include - -from openedx_tagging.core.tagging.rest_api.v1 import ( - views as oel_tagging_views, - views_import as oel_tagging_views_import, -) +from rest_framework.routers import DefaultRouter from . import views 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..719bac56f941 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -21,11 +21,11 @@ from ...api import ( create_taxonomy, get_all_object_tags, - get_taxonomy, get_taxonomies, get_taxonomies_for_org, + get_taxonomy, get_unassigned_taxonomies, - set_taxonomy_orgs, + set_taxonomy_orgs ) from ...rules import get_admin_orgs from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend From c82e9cb2360c851b16bab219d671d04ad17770c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 9 Feb 2024 19:25:34 -0300 Subject: [PATCH 48/50] refactor: use taxonomy.export_id in header --- .../djangoapps/content_tagging/rest_api/v1/tests/test_views.py | 2 +- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) 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 391cdd297459..9b87f35e0483 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 @@ -1818,7 +1818,7 @@ def test_export_course(self, user_attr) -> None: assert response.headers['Content-Type'] == 'text/csv' expected_csv = ( - '"Name","Type","ID","Taxonomy 1","Taxonomy 2"\r\n' + '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1",""\r\n' '" test sequential","sequential","block-v1:orgA+test_course+test_run+type@sequential+block@test_' 'sequential","Tag 1.1, Tag 1.2","Tag 2.1"\r\n' 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 719bac56f941..95608865f3a0 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -174,8 +174,7 @@ def _generate_csv_rows() -> Iterator[str]: # Prepare the header for the taxonomies for taxonomy_id, taxonomy in taxonomies.items(): - # ToDo: change to taxonomy.external_id after the external_id is implemented - header[f"taxonomy_{taxonomy_id}"] = taxonomy.name + header[f"taxonomy_{taxonomy_id}"] = taxonomy.export_id csv_writer = csv.DictWriter(pseudo_buffer, fieldnames=header.keys(), quoting=csv.QUOTE_NONNUMERIC) yield csv_writer.writerow(header) From 01b9b5ff2718e7305bc9b96adc4ea4bdf6e4fdde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 15 Feb 2024 10:15:12 -0300 Subject: [PATCH 49/50] refactor: change `object_id` to `context_id` Co-authored-by: Braden MacDonald --- .../core/djangoapps/content_tagging/rest_api/v1/urls.py | 2 +- .../core/djangoapps/content_tagging/rest_api/v1/views.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py index 7e0ab60e41b4..50fec093c1fb 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py @@ -27,7 +27,7 @@ name="taxonomy-import-template", ), path( - "object_tags//export/", + "object_tags//export/", views.ObjectTagExportView.as_view(), ), path('', include(router.urls)) 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 95608865f3a0..ca8343312bfd 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -150,11 +150,11 @@ class ObjectTagOrgView(ObjectTagView): class ObjectTagExportView(APIView): """" - View to export a CSV with all children and tags for a given object_id. + View to export a CSV with all children and tags for a given course/context. """ def get(self, request: Request, **kwargs) -> StreamingHttpResponse: """ - Export a CSV with all children and tags for a given object_id. + Export a CSV with all children and tags for a given course/context. """ class Echo(object): @@ -197,12 +197,12 @@ def _generate_csv_rows() -> Iterator[str]: yield csv_writer.writerow(block_data) - object_id: str = kwargs.get('object_id', None) + object_id: str = kwargs.get('context_id', None) try: content_key = CourseKey.from_string(object_id) except InvalidKeyError as e: - raise ValidationError("object_id is not a valid content key.") from e + raise ValidationError("context_id is not a valid course key.") from e # Check if the user has permission to view object tags for this object_id try: From 779cc985c2af05c7b0ccbf2f125f83d5fb5ec0c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 15 Feb 2024 11:05:25 -0300 Subject: [PATCH 50/50] chore: trigger CI