From 8409965a984daf95ec702c0feb6c90688f8c8994 Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 28 Oct 2026 22:27:59 +0000 Subject: [PATCH 1/8] feat: leave archived tags out of Studio taxonomy tag counts Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr --- .../rest_api/v1/tests/test_views.py | 36 +++++++++++++++++++ .../content_tagging/rest_api/v1/views.py | 4 +-- 2 files changed, 38 insertions(+), 2 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 91494a696b7e..29c0fe9be96e 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 @@ -615,6 +615,42 @@ def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): assert taxonomy["can_delete_taxonomy"] == user.is_staff # not the metadata about the taxonomy. assert taxonomy["can_tag_object"] + def _get_tags_count(self, taxonomy: Taxonomy, query_params: dict) -> int: + """ + Returns the "tags_count" that the list endpoint reports for the given taxonomy. + """ + self.client.force_authenticate(user=self.staff) + response = self.client.get(TAXONOMY_ORG_LIST_URL, {**query_params, "page_size": 20}, format="json") + assert response.status_code == status.HTTP_200_OK + return next(t for t in response.data["results"] if t["id"] == taxonomy.id)["tags_count"] + + @ddt.data( + ("tA1", {}), + ("tA1", {"org": "orgA"}), + ("ot1", {"unassigned": "true"}), + ) + @ddt.unpack + def test_list_taxonomy_tags_count_excludes_archived(self, taxonomy_attr: str, query_params: dict) -> None: + """ + Archived tags are hidden from the taxonomy's tag list, so they must not be counted either. + """ + taxonomy = getattr(self, taxonomy_attr) + for value in ("one", "two", "three"): + Tag.objects.create(taxonomy=taxonomy, value=value) + Tag.objects.filter(taxonomy=taxonomy, value="three").update(archived=True) + + assert self._get_tags_count(taxonomy, query_params) == 2 + + def test_list_taxonomy_tags_count_all_archived_is_zero(self) -> None: + """ + A taxonomy whose tags are all archived reports no tags. + """ + for value in ("one", "two", "three"): + Tag.objects.create(taxonomy=self.tA1, value=value) + Tag.objects.filter(taxonomy=self.tA1).update(archived=True) + + assert self._get_tags_count(self.tA1, {}) == 0 + def test_list_taxonomy_type_mixed(self) -> None: """ The list endpoint reports "taxonomy_type" per row for a mixed set of taxonomies: 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 b49109fc1d3c..8722e5d3aae3 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -7,7 +7,7 @@ from typing import TYPE_CHECKING from django.core import exceptions -from django.db.models import Count +from django.db.models import Count, Q from django.http import StreamingHttpResponse from openedx_authz import api as authz_api from openedx_authz.constants.permissions import COURSES_VIEW_COURSE @@ -104,7 +104,7 @@ def get_queryset(self): queryset = queryset.prefetch_related("taxonomyorg_set__org") # Annotate with tags_count to avoid selecting all the tags - queryset = queryset.annotate(tags_count=Count("tag", distinct=True)) + queryset = queryset.annotate(tags_count=Count("tag", filter=Q(tag__archived=False), distinct=True)) # Select the competency taxonomy relation (if any) so is_competency_taxonomy() # costs no extra query per row when serializing taxonomy_type. From cf6025e984d11d8ee4896a1bf28719771b127e3e Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 28 Oct 2026 22:42:21 +0000 Subject: [PATCH 2/8] feat: leave archived associations out of tag export, duplicate, and copy Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr --- .../contentstore/views/tests/test_block.py | 8 +- .../views/tests/test_clipboard_paste.py | 14 ++- .../core/djangoapps/content_tagging/api.py | 3 +- .../content_tagging/tests/test_api.py | 103 +++++++++++++++++- 4 files changed, 121 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 525e0edc26fc..9bcd927c4660 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -28,6 +28,7 @@ from openedx_events.content_authoring.data import DuplicatedXBlockData from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED from openedx_events.testing import OpenEdxEventsTestMixin +from openedx_tagging.models import ObjectTag from pytz import UTC from web_fragments.fragment import Fragment from webob import Response @@ -1541,6 +1542,9 @@ def test_duplicate_tags(self): # Tag the child block tagging_api.tag_object(str(source_block.location), taxonomyA, ["two"],) + # Archive one of the chapter's associations; it must not be duplicated. + ObjectTag.objects.filter(object_id=str(source_chapter.location), _value="four").update(archived=True) + # Duplicate the chapter (and its children) dupe_location = duplicate_block( parent_usage_key=source_course.location, @@ -1555,7 +1559,6 @@ def test_duplicate_tags(self): expected_chapter_tags = [ f' {str(dupe_chapter.location)}: A=one', f' {str(dupe_chapter.location)}: A=two', - f' {str(dupe_chapter.location)}: B=four', f' {str(dupe_chapter.location)}: B=three', ] dupe_chapter_tags = [str(object_tag) for object_tag in tagging_api.get_object_tags(str(dupe_chapter.location))] @@ -1566,6 +1569,9 @@ def test_duplicate_tags(self): dupe_block_tags = [str(object_tag) for object_tag in tagging_api.get_object_tags(str(dupe_block.location))] assert dupe_block_tags == expected_block_tags + # The archived association is left in place on the source. + assert ObjectTag.objects.get(object_id=str(source_chapter.location), _value="four").archived is True + @ddt.ddt class TestMoveItem(ItemTest): diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 4a604dae7c2d..ac058e6499fb 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -13,7 +13,7 @@ XBLOCK_UPDATED, ) from openedx_events.testing import OpenEdxEventsTestMixin -from openedx_tagging.models import Tag +from openedx_tagging.models import ObjectTag, Tag from organizations.models import Organization from rest_framework.test import APIClient @@ -284,8 +284,15 @@ def test_copy_and_paste_unit_with_tags(self): dest_chapter = BlockFactory.create(parent=dest_course, category='chapter', display_name='Section') dest_sequential = BlockFactory.create(parent=dest_chapter, category='sequential', display_name='Subsection') - # Copy the unit + # Archive one of the unit's associations, to test that it isn't pasted. unit_key = source_block_keys['vertical'] + ObjectTag.objects.filter( + object_id=str(unit_key), + taxonomy__export_id='ALL_ORGS', + _value='tag_2', + ).update(archived=True) + + # Copy the unit copy_response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(unit_key)}, format="json") assert copy_response.status_code == 200 @@ -302,9 +309,8 @@ def test_copy_and_paste_unit_with_tags(self): # Only tags from the taxonomy that is associated with the dest org should be copied tags = list(tagging_api.get_object_tags(str(dest_unit_key))) - assert len(tags) == 2 + assert len(tags) == 1 assert str(tags[0]) == f' {dest_unit_key}: test_taxonomy=tag_1' - assert str(tags[1]) == f' {dest_unit_key}: test_taxonomy=tag_2' # Ensure that the pasted child blocks were tagged too. dest_tags, _ = tagging_api.get_all_object_tags(dest_course.id) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index ecfd04a91d86..2148df06109b 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -155,6 +155,7 @@ def get_all_object_tags( Includes any tags applied to the course/library as a whole. Returns a tuple with a dictionary of grouped object tag values for all blocks and a dictionary of taxonomies. + Archived associations, and those whose tag or taxonomy is archived, are left out. If `prefetch_orgs` is set, then the returned ObjectTag taxonomies will have their TaxonomyOrgs prefetched, which makes checking permissions faster. @@ -176,7 +177,7 @@ def get_all_object_tags( if block_id_prefix: object_id_clause |= Q(object_id__startswith=block_id_prefix) - all_object_tags = ObjectTag.objects.filter( + all_object_tags = ObjectTag.objects.unarchived().filter( Q(tag__isnull=False, tag__taxonomy__isnull=False), object_id_clause, ).select_related("tag__taxonomy").order_by("object_id") diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index ea97706a9d1d..cd08f5909b5f 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,4 +1,5 @@ """Tests for the Tagging models""" +import copy import io import os import tempfile @@ -9,7 +10,7 @@ from fs.osfs import OSFS from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 -from openedx_tagging.models import ObjectTag +from openedx_tagging.models import ObjectTag, Tag, Taxonomy from organizations.models import Organization from .. import api @@ -306,6 +307,14 @@ def test_get_course_object_tags_with_add_tags(self): self.taxonomy_2.id: [tag.value for tag in vertical2_tags], } + # The order of values within a taxonomy is not guaranteed, and this test is about no tags being lost. + for tags_by_taxonomy in ( + object_tags["block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1"], + self.expected_course_objecttags["block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical1"], + ): + for values in tags_by_taxonomy.values(): + values.sort() + assert object_tags == self.expected_course_objecttags assert taxonomies == { self.taxonomy_1.id: self.taxonomy_1, @@ -327,6 +336,65 @@ def test_get_library_object_tags(self): self.taxonomy_2.id: self.taxonomy_2, } + def test_get_course_object_tags_excludes_archived_association(self): + """ + An archived association is left out, and the other associations on the same block are kept. + """ + sequential_id = "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" + ObjectTag.objects.filter(object_id=sequential_id, _value="Tag 1.1").update(archived=True) + + object_tags, taxonomies = api.get_all_object_tags( + CourseKey.from_string("course-v1:orgA+test_course+test_run") + ) + + expected = copy.deepcopy(self.expected_course_objecttags) + expected[sequential_id][self.taxonomy_1.id] = ["Tag 1.2"] + assert object_tags == expected + assert taxonomies == { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } + + def test_get_course_object_tags_excludes_archived_tag(self): + """ + Associations to an archived tag are left out. + """ + Tag.objects.filter(pk=self.tag_1_2.pk).update(archived=True) + + object_tags, taxonomies = api.get_all_object_tags( + CourseKey.from_string("course-v1:orgA+test_course+test_run") + ) + + expected = copy.deepcopy(self.expected_course_objecttags) + sequential_id = "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" + expected[sequential_id][self.taxonomy_1.id] = ["Tag 1.1"] + assert object_tags == expected + assert taxonomies == { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } + + def test_get_course_object_tags_excludes_archived_taxonomy(self): + """ + Associations under an archived taxonomy are left out, and the taxonomy is not returned. + """ + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + + object_tags, taxonomies = api.get_all_object_tags( + CourseKey.from_string("course-v1:orgA+test_course+test_run") + ) + + # The vertical and html blocks only had taxonomy_2 tags, so they have no entry at all. + assert object_tags == { + "course-v1:orgA+test_course+test_run": self.expected_course_objecttags[ + "course-v1:orgA+test_course+test_run" + ], + "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential": { + self.taxonomy_1.id: ["Tag 1.1", "Tag 1.2"], + }, + } + assert taxonomies == {self.taxonomy_1.id: self.taxonomy_1} + def _test_copy_object_tags(self, src_key, dst_key, expected_tags): """ Test copying object tags to a new object. @@ -453,6 +521,39 @@ def test_generate_csv_rows(self) -> None: assert csv_content == self.expected_csv + def test_generate_csv_rows_excludes_archived(self) -> None: + sequential_id = "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" + ObjectTag.objects.filter(object_id=sequential_id, _value="Tag 1.1").update(archived=True) + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + + buffer = io.StringIO() + list(api.generate_csv_rows(str(self.course.id), buffer)) + buffer.seek(0) + csv_content = buffer.getvalue() + + assert csv_content == ( + '"Name","Type","ID","1-taxonomy-1"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1"\r\n' + '" test sequential","sequential","test_sequential","Tag 1.2"\r\n' + '" test vertical1","vertical","test_vertical1",""\r\n' + '" test vertical2","vertical","test_vertical2",""\r\n' + '" test html","html","test_html",""\r\n' + '" untagged sequential","sequential","untagged_sequential",""\r\n' + '" untagged vertical","vertical","untagged_vertical",""\r\n' + ) + + def test_generate_csv_rows_keeps_disabled_taxonomy(self) -> None: + # Disabled taxonomies are not archived, so their tags are still exported. + self.taxonomy_1.enabled = False + self.taxonomy_1.save() + + buffer = io.StringIO() + list(api.generate_csv_rows(str(self.course.id), buffer)) + buffer.seek(0) + csv_content = buffer.getvalue() + + assert csv_content == self.expected_csv + def test_export_tags_in_csv_file(self) -> None: file_dir_name = tempfile.mkdtemp() file_dir = OSFS(file_dir_name) From 40823b0cdf21e09e9766142c666ac52bada1b2f1 Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 28 Oct 2026 22:57:59 +0000 Subject: [PATCH 3/8] feat: keep hidden associations and skip archived taxonomies on course tag import Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr --- .../core/djangoapps/content_tagging/api.py | 20 +++-- .../content_tagging/tests/test_api.py | 82 +++++++++++++++++++ 2 files changed, 97 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 2148df06109b..0d2fba5c3395 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -5,6 +5,7 @@ import csv import io +import logging from itertools import groupby from typing import Iterator # noqa: UP035 @@ -25,6 +26,8 @@ from .types import ContentKey, TagValuesByObjectIdDict, TagValuesByTaxonomyIdDict, TaxonomyDict from .utils import check_taxonomy_context_key_org, get_content_key_from_string, get_context_key_from_key +log = logging.getLogger(__name__) + class InvalidOrgException(ValidationError): """ @@ -296,20 +299,27 @@ def set_exported_object_tags( """ content_key_str = str(content_key) - # Clear all tags related with the content. - oel_tagging.delete_object_tags(content_key_str) + # Clear the associations the author can see. Archived ones, and those under an archived tag or taxonomy, + # are left alone; the queryset delete archives any locked visible row instead of deleting it. + ObjectTag.objects.filter(object_id=content_key_str).unarchived().delete() for taxonomy_export_id, tags_values in exported_tags.items(): if not tags_values: continue - taxonomy = oel_tagging.get_taxonomy_by_export_id(str(taxonomy_export_id)) + export_id = str(taxonomy_export_id) + # The filtered lookup returns None for an archived and a missing taxonomy alike; tell them apart here. + taxonomy = Taxonomy.objects.filter(export_id=export_id).first() + if taxonomy is not None and taxonomy.archived: + # No placeholder: ObjectTag.resync() would re-link it to the archived taxonomy. + log.info("Skipping tags %s for %s: taxonomy %s is archived", tags_values, content_key_str, export_id) + continue oel_tagging.tag_object( object_id=content_key_str, - taxonomy=taxonomy, + taxonomy=taxonomy, # None for a missing taxonomy, which creates placeholders. tags=tags_values, create_invalid=True, - taxonomy_export_id=str(taxonomy_export_id), + taxonomy_export_id=export_id, ) # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index cd08f5909b5f..74ff3e0eda76 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -3,6 +3,7 @@ import io import os import tempfile +from unittest import mock import ddt import pytest @@ -499,6 +500,7 @@ def test_tag_container_invalid_org(self) -> None: api.tag_object(object_id=str(unit_key_org_b), taxonomy=self.taxonomy_3, tags=[tag]) +@ddt.ddt class TestExportImportTags(TaggedCourseMixin): """ Tests for export/import functions @@ -631,3 +633,83 @@ def test_import_tags_invalid_taxonomy(self) -> None: assert object_tag.value == 'Tag 1.11' assert object_tag.taxonomy is None assert object_tag.export_id == '1-taxonomy-1-1' + + @ddt.data("association", "tag", "taxonomy") + def test_import_keeps_hidden_association(self, hidden_by: str) -> None: + course_id = str(self.course.id) + api.tag_object(object_id=course_id, taxonomy=self.taxonomy_2, tags=["Tag 2.1"]) + hidden_pk = ObjectTag.objects.get(object_id=course_id, _value="Tag 2.1").pk + if hidden_by == "association": + ObjectTag.objects.filter(pk=hidden_pk).update(archived=True) + elif hidden_by == "tag": + Tag.objects.filter(pk=self.tag_2_1.pk).update(archived=True) + else: + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + + csv_path = self._create_csv_file( + '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.2",""\r\n' + ) + api.import_course_tags_from_csv(csv_path, self.course.id) + + visible_tags = list(api.get_object_tags(self.course.id)) + assert [t.value for t in visible_tags] == ["Tag 1.2"] + assert ObjectTag.objects.filter(object_id=course_id, _value="Tag 2.1").count() == 1 + assert ObjectTag.objects.get(pk=hidden_pk) # The same row survives. + assert hidden_pk not in [t.pk for t in visible_tags] # It is still hidden. + + def test_import_clears_placeholder_and_deleted_reference_rows(self) -> None: + course_id = str(self.course.id) + # A placeholder for a taxonomy that does not exist, and a row whose tag was deleted. + ObjectTag.objects.create(object_id=course_id, taxonomy=None, tag=None, _value="X", _export_id="gone-taxonomy") + ObjectTag.objects.create(object_id=course_id, taxonomy=self.taxonomy_1, tag=None, _value="Tag 1.11") + + csv_path = self._create_csv_file( + '"Name","Type","ID","1-taxonomy-1"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1"\r\n' + ) + api.import_course_tags_from_csv(csv_path, self.course.id) + + assert not ObjectTag.objects.filter(object_id=course_id, _export_id="gone-taxonomy").exists() + assert not ObjectTag.objects.filter(object_id=course_id, _value="Tag 1.11").exists() + assert [t.value for t in api.get_object_tags(self.course.id)] == ["Tag 1.1"] + assert ObjectTag.objects.filter(object_id=course_id).count() == 1 + + def test_import_archives_locked_visible_association(self) -> None: + course_id = str(self.course.id) + locked_pk = ObjectTag.objects.get(object_id=course_id, _value="Tag 1.1").pk + ObjectTag.objects.filter(pk=locked_pk).update(deletion_locked=True) + + csv_path = self._create_csv_file( + '"Name","Type","ID","1-taxonomy-1"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.2"\r\n' + ) + api.import_course_tags_from_csv(csv_path, self.course.id) + + assert ObjectTag.objects.get(pk=locked_pk).archived + assert [t.value for t in api.get_object_tags(self.course.id)] == ["Tag 1.2"] + + def test_import_skips_archived_taxonomy(self) -> None: + course_id = str(self.course.id) + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + + csv_path = self._create_csv_file( + '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1","Tag 2.1"\r\n' + ) + with ( + mock.patch( + "openedx.core.djangoapps.content_tagging.api.CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event" + ) as send_event, + self.assertLogs("openedx.core.djangoapps.content_tagging.api", level="INFO") as logs, + ): + api.import_course_tags_from_csv(csv_path, self.course.id) + + # These come before the log check so that a missing log does not hide the state they describe. + assert [t.value for t in api.get_object_tags(self.course.id)] == ["Tag 1.1"] + assert not ObjectTag.objects.filter(object_id=course_id, _export_id="2-taxonomy-2").exists() + assert not ObjectTag.objects.filter(object_id=course_id, taxonomy=self.taxonomy_2).exists() + # The skipped taxonomy sends no event, so only taxonomy_1 does. + assert send_event.call_count == 1 + + assert "2-taxonomy-2" in logs.output[0] From 05673976da7a11c91b592b6236990dfada81921c Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 28 Oct 2026 23:01:18 +0000 Subject: [PATCH 4/8] docs: say which associations the tag import clears and why archived taxonomies are skipped Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr --- openedx/core/djangoapps/content_tagging/api.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 0d2fba5c3395..405bd60ec181 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -299,8 +299,8 @@ def set_exported_object_tags( """ content_key_str = str(content_key) - # Clear the associations the author can see. Archived ones, and those under an archived tag or taxonomy, - # are left alone; the queryset delete archives any locked visible row instead of deleting it. + # Clear the associations that archiving does not hide. An archived association, or one under an archived tag + # or taxonomy, is left alone. The queryset delete archives a deletion-locked row instead of deleting it. ObjectTag.objects.filter(object_id=content_key_str).unarchived().delete() for taxonomy_export_id, tags_values in exported_tags.items(): @@ -308,15 +308,17 @@ def set_exported_object_tags( continue export_id = str(taxonomy_export_id) - # The filtered lookup returns None for an archived and a missing taxonomy alike; tell them apart here. + # get_taxonomy_by_export_id() returns None for an archived taxonomy and a missing one alike. The two are + # handled differently below, so look the taxonomy up through the unfiltered manager to tell them apart. taxonomy = Taxonomy.objects.filter(export_id=export_id).first() if taxonomy is not None and taxonomy.archived: - # No placeholder: ObjectTag.resync() would re-link it to the archived taxonomy. + # A placeholder would be wrong here, because ObjectTag.resync() looks taxonomies up unfiltered and + # would re-link the placeholder to the archived taxonomy. log.info("Skipping tags %s for %s: taxonomy %s is archived", tags_values, content_key_str, export_id) continue oel_tagging.tag_object( object_id=content_key_str, - taxonomy=taxonomy, # None for a missing taxonomy, which creates placeholders. + taxonomy=taxonomy, # None for a missing taxonomy, so tag_object() creates placeholders. tags=tags_values, create_invalid=True, taxonomy_export_id=export_id, From 7036431ace590b222a4cf999af520c8370b64d3a Mon Sep 17 00:00:00 2001 From: tbain Date: Mon, 2 Nov 2026 19:42:45 +0000 Subject: [PATCH 5/8] test: pin same-taxonomy re-import, all-archived export, and library block cases Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr --- .../content_tagging/rest_api/v1/views.py | 2 +- .../content_tagging/tests/test_api.py | 78 ++++++++++++++++--- 2 files changed, 68 insertions(+), 12 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 8722e5d3aae3..fcf54c14dfa4 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -103,7 +103,7 @@ def get_queryset(self): # Prefetch taxonomyorgs so we can check permissions queryset = queryset.prefetch_related("taxonomyorg_set__org") - # Annotate with tags_count to avoid selecting all the tags + # Annotate with tags_count to avoid selecting all the tags. The count leaves archived tags out. queryset = queryset.annotate(tags_count=Count("tag", filter=Q(tag__archived=False), distinct=True)) # Select the competency taxonomy relation (if any) so is_competency_taxonomy() diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 74ff3e0eda76..24a51c57843f 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,5 +1,6 @@ """Tests for the Tagging models""" import copy +import csv import io import os import tempfile @@ -7,6 +8,7 @@ import ddt import pytest +from django.db.models import Q from django.test.testcases import TestCase from fs.osfs import OSFS from opaque_keys.edx.keys import CourseKey, UsageKey @@ -337,6 +339,25 @@ def test_get_library_object_tags(self): self.taxonomy_2.id: self.taxonomy_2, } + def test_get_library_object_tags_excludes_archived_association(self): + """ + An archived association is left out of a library's tags, and a block with no other tags is dropped. + """ + problem_id = f"lb:orgA:lib_{self.block_suffix}:problem:problem1_{self.block_suffix}" + ObjectTag.objects.filter(object_id=problem_id, _value="Tag 1.1").update(archived=True) + + object_tags, taxonomies = api.get_all_object_tags( + LibraryLocatorV2.from_string(f"lib:orgA:lib_{self.block_suffix}") + ) + + expected = copy.deepcopy(self.expected_library_objecttags) + del expected[problem_id] + assert object_tags == expected + assert taxonomies == { + self.taxonomy_1.id: self.taxonomy_1, + self.taxonomy_2.id: self.taxonomy_2, + } + def test_get_course_object_tags_excludes_archived_association(self): """ An archived association is left out, and the other associations on the same block are kept. @@ -544,6 +565,23 @@ def test_generate_csv_rows_excludes_archived(self) -> None: '" untagged vertical","vertical","untagged_vertical",""\r\n' ) + def test_generate_csv_rows_all_archived(self) -> None: + # The same prefix get_all_object_tags() uses to find the course's blocks. + block_id_prefix = str(self.course.id).replace("course-v1:", "block-v1:", 1) + ObjectTag.objects.filter( + Q(object_id=str(self.course.id)) | Q(object_id__startswith=block_id_prefix) + ).update(archived=True) + + buffer = io.StringIO() + list(api.generate_csv_rows(str(self.course.id), buffer)) + buffer.seek(0) + csv_rows = list(csv.reader(buffer)) + + # Nothing is left to export, so no taxonomy columns remain, but every block still has a row. + expected_rows = [row[:3] for row in csv.reader(io.StringIO(self.expected_csv))] + assert expected_rows[0] == ["Name", "Type", "ID"] + assert csv_rows == expected_rows + def test_generate_csv_rows_keeps_disabled_taxonomy(self) -> None: # Disabled taxonomies are not archived, so their tags are still exported. self.taxonomy_1.enabled = False @@ -634,11 +672,10 @@ def test_import_tags_invalid_taxonomy(self) -> None: assert object_tag.taxonomy is None assert object_tag.export_id == '1-taxonomy-1-1' - @ddt.data("association", "tag", "taxonomy") - def test_import_keeps_hidden_association(self, hidden_by: str) -> None: - course_id = str(self.course.id) - api.tag_object(object_id=course_id, taxonomy=self.taxonomy_2, tags=["Tag 2.1"]) - hidden_pk = ObjectTag.objects.get(object_id=course_id, _value="Tag 2.1").pk + def _hide_association(self, hidden_by: str, hidden_pk: int) -> None: + """ + Hides the given association by archiving the association, its tag, or its taxonomy. + """ if hidden_by == "association": ObjectTag.objects.filter(pk=hidden_pk).update(archived=True) elif hidden_by == "tag": @@ -646,16 +683,35 @@ def test_import_keeps_hidden_association(self, hidden_by: str) -> None: else: Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) - csv_path = self._create_csv_file( - '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' - '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.2",""\r\n' - ) + @ddt.data( + ("association", False), + ("tag", False), + ("taxonomy", False), + # An archived taxonomy is skipped before tag_object() runs, so it has no same-taxonomy case. + ("association", True), + ("tag", True), + ) + @ddt.unpack + def test_import_keeps_hidden_association(self, hidden_by: str, visible_in_same_taxonomy: bool) -> None: + course_id = str(self.course.id) + api.tag_object(object_id=course_id, taxonomy=self.taxonomy_2, tags=["Tag 2.1"]) + hidden_pk = ObjectTag.objects.get(object_id=course_id, _value="Tag 2.1").pk + self._hide_association(hidden_by, hidden_pk) + + if visible_in_same_taxonomy: + # tag_object() runs on the taxonomy that holds the hidden row, and must leave that row alone. + row = '"Test Course","course","course-v1:orgA+test_course+test_run","","Tag 2.2"\r\n' + expected_visible = ["Tag 2.2"] + else: + row = '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.2",""\r\n' + expected_visible = ["Tag 1.2"] + csv_path = self._create_csv_file('"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' + row) api.import_course_tags_from_csv(csv_path, self.course.id) visible_tags = list(api.get_object_tags(self.course.id)) - assert [t.value for t in visible_tags] == ["Tag 1.2"] + assert [t.value for t in visible_tags] == expected_visible assert ObjectTag.objects.filter(object_id=course_id, _value="Tag 2.1").count() == 1 - assert ObjectTag.objects.get(pk=hidden_pk) # The same row survives. + assert ObjectTag.objects.filter(pk=hidden_pk).exists() # The same row survives. assert hidden_pk not in [t.pk for t in visible_tags] # It is still hidden. def test_import_clears_placeholder_and_deleted_reference_rows(self) -> None: From 2469f55841cddd44a4525f92a0d30bc40556e58f Mon Sep 17 00:00:00 2001 From: tbain Date: Mon, 2 Nov 2026 21:27:39 +0000 Subject: [PATCH 6/8] test: pin library container copy leaving archived tags out; state the tag import contract Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr --- .../content_libraries/tests/test_api.py | 41 +++++++++++++++++++ .../core/djangoapps/content_tagging/api.py | 13 +++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index d2c333fce120..7f64cba09554 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -30,11 +30,14 @@ ) from openedx_events.testing import EventsIsolationMixin from openedx_events.tooling import OpenEdxPublicSignal +from openedx_tagging.models import ObjectTag from user_tasks.models import UserTaskStatus from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_tagging import api as tagging_api from .. import api +from ..api.serializers import ContainerSerializer from ..models import ContentLibrary from .base import ContentLibrariesRestApiTest @@ -780,6 +783,44 @@ def test_copy_and_paste_container_same_library(self) -> None: child_history = content_api.get_entity_draft_history(subsections[0].container_id) assert len(child_history) == 0 + def test_copy_container_leaves_archived_tags_out(self) -> None: + """ + Copying a container leaves out archived tag associations, on the container and on its children. + + Pasting into a library does not apply tags, so this checks what the serializer and the clipboard carry. + """ + taxonomy = tagging_api.create_taxonomy('Archived Tags Taxonomy') + tagging_api.set_taxonomy_orgs(taxonomy, all_orgs=True) + for value in ("one", "two", "three", "four"): + tagging_api.add_tag_to_taxonomy(taxonomy, value) + + section_id = str(self.section1.container_key) + child_id = str(self.subsection1.container_key) + tagging_api.tag_object(section_id, taxonomy, ["one", "two"]) + tagging_api.tag_object(child_id, taxonomy, ["three", "four"]) + # Archive one association on the copied container and one on its child + ObjectTag.objects.filter(object_id=section_id, _value="two").update(archived=True) + ObjectTag.objects.filter(object_id=child_id, _value="four").update(archived=True) + + serializer = ContainerSerializer(api.get_container(self.section1.container_key)) + + # The serializer also holds an empty entry for each untagged child block, so check the tagged ids by name. + assert serializer.tags[section_id] == {taxonomy.id: ["one"]} + assert serializer.tags[child_id] == {taxonomy.id: ["three"]} + assert all(not tags for object_id, tags in serializer.tags.items() if object_id not in (section_id, child_id)) + # The clipboard receives the same tags + clipboard = api.copy_container(self.section1.container_key, self.user.id) + clipboard_values = { + object_id: sorted(value for values in by_taxonomy.values() for value in values) + for object_id, by_taxonomy in clipboard.content.tags.items() + } + assert clipboard_values[section_id] == ["one"] + assert clipboard_values[child_id] == ["three"] + assert all(not v for k, v in clipboard_values.items() if k not in (section_id, child_id)) + # Copying does not delete the archived associations + assert ObjectTag.objects.filter(object_id=section_id, _value="two", archived=True).exists() + assert ObjectTag.objects.filter(object_id=child_id, _value="four", archived=True).exists() + def test_copy_and_paste_container_another_library(self) -> None: """ Test copying and then pasting a container (into a different library) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 405bd60ec181..2748dbd32d18 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -295,7 +295,18 @@ def set_exported_object_tags( exported_tags: TagValuesByTaxonomyIdDict, ) -> None: """ - Sets the tags for the given exported content object. + Replaces the tags of the given content object with the tags from an export, keeping what archiving hides. + + ``exported_tags`` maps a taxonomy export id to the list of tag values for that taxonomy. + + This clears only the associations that ``ObjectTag.objects.unarchived()`` returns. An archived association, and an + association whose tag or taxonomy is archived, is kept. A deletion-locked association is archived instead of + deleted. To clear every association of the object, archiving the deletion-locked ones, call + ``delete_object_tags`` instead. + + For a live or disabled taxonomy, the object is tagged with the given values. An archived taxonomy is skipped and + logged, with no placeholder association and no event. A taxonomy that does not exist on this instance gets + placeholder associations, which a later import of that taxonomy can link to it. """ content_key_str = str(content_key) From b3539393171aea1d79de5f6a4d866175555caaac Mon Sep 17 00:00:00 2001 From: tbain Date: Mon, 9 Nov 2026 21:58:55 +0000 Subject: [PATCH 7/8] docs: shorten the comments and docstring in set_exported_object_tags Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01VjwKFzQ6Tfh5HJ5xzgmGKr --- .../core/djangoapps/content_tagging/api.py | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 2748dbd32d18..4d98352e229a 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -295,23 +295,16 @@ def set_exported_object_tags( exported_tags: TagValuesByTaxonomyIdDict, ) -> None: """ - Replaces the tags of the given content object with the tags from an export, keeping what archiving hides. + Replace the object's tags with the tags from an export, keeping what archiving hides. - ``exported_tags`` maps a taxonomy export id to the list of tag values for that taxonomy. - - This clears only the associations that ``ObjectTag.objects.unarchived()`` returns. An archived association, and an - association whose tag or taxonomy is archived, is kept. A deletion-locked association is archived instead of - deleted. To clear every association of the object, archiving the deletion-locked ones, call - ``delete_object_tags`` instead. - - For a live or disabled taxonomy, the object is tagged with the given values. An archived taxonomy is skipped and - logged, with no placeholder association and no event. A taxonomy that does not exist on this instance gets - placeholder associations, which a later import of that taxonomy can link to it. + ``exported_tags`` maps a taxonomy export id to that taxonomy's tag values. Only the associations that + ``ObjectTag.objects.unarchived()`` returns are cleared, and a deletion-locked one is archived instead of deleted. + Tags from an archived taxonomy are skipped and logged. Tags from a taxonomy that does not exist on this instance + become placeholder associations, as before. ``delete_object_tags`` clears everything. """ content_key_str = str(content_key) - # Clear the associations that archiving does not hide. An archived association, or one under an archived tag - # or taxonomy, is left alone. The queryset delete archives a deletion-locked row instead of deleting it. + # A queryset delete, so that a deletion-locked association is archived rather than deleted. ObjectTag.objects.filter(object_id=content_key_str).unarchived().delete() for taxonomy_export_id, tags_values in exported_tags.items(): @@ -319,17 +312,15 @@ def set_exported_object_tags( continue export_id = str(taxonomy_export_id) - # get_taxonomy_by_export_id() returns None for an archived taxonomy and a missing one alike. The two are - # handled differently below, so look the taxonomy up through the unfiltered manager to tell them apart. + # Unfiltered on purpose: get_taxonomy_by_export_id() hides archived taxonomies, which are handled below. taxonomy = Taxonomy.objects.filter(export_id=export_id).first() if taxonomy is not None and taxonomy.archived: - # A placeholder would be wrong here, because ObjectTag.resync() looks taxonomies up unfiltered and - # would re-link the placeholder to the archived taxonomy. + # No placeholder either: ObjectTag.resync() would re-link it to the archived taxonomy. log.info("Skipping tags %s for %s: taxonomy %s is archived", tags_values, content_key_str, export_id) continue oel_tagging.tag_object( object_id=content_key_str, - taxonomy=taxonomy, # None for a missing taxonomy, so tag_object() creates placeholders. + taxonomy=taxonomy, tags=tags_values, create_invalid=True, taxonomy_export_id=export_id, From 0b53bc5084fbeaf36e07edd55c4fafa3f56c04cc Mon Sep 17 00:00:00 2001 From: tbain Date: Wed, 11 Nov 2026 13:07:29 -0700 Subject: [PATCH 8/8] feat: addressing found issues during dev code review --- .../contentstore/views/tests/test_block.py | 9 +- .../views/tests/test_clipboard_paste.py | 65 ++++++- .../core/djangoapps/content_tagging/api.py | 34 ++-- .../rest_api/v1/tests/test_views.py | 25 +-- .../content_tagging/tests/test_api.py | 172 ++++++++++++------ 5 files changed, 219 insertions(+), 86 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 9bcd927c4660..4d6a3e661507 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -1542,6 +1542,10 @@ def test_duplicate_tags(self): # Tag the child block tagging_api.tag_object(str(source_block.location), taxonomyA, ["two"],) + # Disable a taxonomy; disabled is not archived, so its tags are still duplicated. + taxonomyB.enabled = False + taxonomyB.save() + # Archive one of the chapter's associations; it must not be duplicated. ObjectTag.objects.filter(object_id=str(source_chapter.location), _value="four").update(archived=True) @@ -1561,7 +1565,10 @@ def test_duplicate_tags(self): f' {str(dupe_chapter.location)}: A=two', f' {str(dupe_chapter.location)}: B=three', ] - dupe_chapter_tags = [str(object_tag) for object_tag in tagging_api.get_object_tags(str(dupe_chapter.location))] + # get_object_tags() hides disabled taxonomies, so read the rows directly. + dupe_chapter_tags = sorted(str(object_tag) for object_tag in ObjectTag.objects.filter( + object_id=str(dupe_chapter.location) + )) assert dupe_chapter_tags == expected_chapter_tags expected_block_tags = [ f' {str(dupe_block.location)}: A=two', diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index ac058e6499fb..8e0a870cbaf9 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -5,6 +5,7 @@ """ import ddt from opaque_keys.edx.keys import UsageKey +from openedx_content import models_api as content_models from openedx_content.api import signals as content_signals from openedx_events.content_authoring.signals import ( LIBRARY_BLOCK_DELETED, @@ -284,8 +285,21 @@ def test_copy_and_paste_unit_with_tags(self): dest_chapter = BlockFactory.create(parent=dest_course, category='chapter', display_name='Section') dest_sequential = BlockFactory.create(parent=dest_chapter, category='sequential', display_name='Subsection') - # Archive one of the unit's associations, to test that it isn't pasted. unit_key = source_block_keys['vertical'] + + # Disable a taxonomy; disabled is not archived, so its tags are still pasted. + taxonomy_disabled = tagging_api.create_taxonomy( + "test_taxonomy_disabled", + "Test Taxonomy Disabled", + export_id="DISABLED", + ) + tagging_api.set_taxonomy_orgs(taxonomy_disabled, all_orgs=True) + tagging_api.add_tag_to_taxonomy(taxonomy_disabled, "tag_1") + tagging_api.tag_object(object_id=str(unit_key), taxonomy=taxonomy_disabled, tags=["tag_1"]) + taxonomy_disabled.enabled = False + taxonomy_disabled.save() + + # Archive one of the unit's associations, to test that it isn't pasted. ObjectTag.objects.filter( object_id=str(unit_key), taxonomy__export_id='ALL_ORGS', @@ -312,6 +326,13 @@ def test_copy_and_paste_unit_with_tags(self): assert len(tags) == 1 assert str(tags[0]) == f' {dest_unit_key}: test_taxonomy=tag_1' + # get_object_tags() hides the disabled taxonomy, so read the rows directly. + disabled_values = ObjectTag.objects.filter( + object_id=str(dest_unit_key), + taxonomy__export_id="DISABLED", + ).values_list("_value", flat=True) + assert list(disabled_values) == ["tag_1"] + # Ensure that the pasted child blocks were tagged too. dest_tags, _ = tagging_api.get_all_object_tags(dest_course.id) dest_block_ids = { @@ -512,6 +533,48 @@ def test_paste_from_library_read_only_tags(self): assert object_tag.value in self.lib_block_tags assert not object_tag.is_copied + def test_paste_from_library_unit_leaves_archived_tags_out(self): + # Neither the pasted unit nor its child carries an archived tag. + # If we're working with Containers in test cases, we need this line: + self.addCleanup(content_models.Container.reset_cache) + taxonomy = tagging_api.get_taxonomy_by_export_id("ALL_ORGS") + unit = library_api.create_container( + library_key=self.library.key, + container_cls=content_models.Unit, + slug="unit-1", + title="Unit 1", + user_id=None, + ) + library_api.update_container_children(unit.container_key, [self.lib_block_key], None) + library_api.publish_changes(self.library.key) + unit_id = str(unit.container_key) + child_id = str(self.lib_block_key) + tagging_api.tag_object(unit_id, taxonomy, ["tag_2", "tag_3"]) + # The child already has the tags from setUp. + ObjectTag.objects.filter(object_id=unit_id, _value="tag_3").update(archived=True) + ObjectTag.objects.filter(object_id=child_id, _value="tag_5").update(archived=True) + + library_api.copy_container(unit.container_key, self.user.id) + with self.store.bulk_operations(self.course.id): + chapter = BlockFactory.create(parent=self.course, category='chapter', display_name='Section') + sequential = BlockFactory.create(parent=chapter, category='sequential', display_name='Subsection') + paste_response = self.client.post(XBLOCK_ENDPOINT, { + "parent_locator": str(sequential.location), + "staged_content": "clipboard", + }, format="json") + assert paste_response.status_code == 200 + + pasted_unit_key = UsageKey.from_string(paste_response.json()["locator"]) + pasted_children = self.store.get_item(pasted_unit_key).children + assert len(pasted_children) == 1 + + def stored_values(key): + # Unfiltered, so that a hidden copy would be found too. + return sorted(ObjectTag.objects.filter(object_id=str(key)).values_list("_value", flat=True)) + + assert stored_values(pasted_unit_key) == ["tag_2"] + assert stored_values(pasted_children[0]) == ["tag_1"] + def test_paste_from_library_copies_asset(self): """ Assets from a library component copied into a subdir of Files & Uploads. diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 4d98352e229a..727970edaa48 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -6,6 +6,7 @@ import csv import io import logging +from collections import Counter from itertools import groupby from typing import Iterator # noqa: UP035 @@ -183,7 +184,7 @@ def get_all_object_tags( all_object_tags = ObjectTag.objects.unarchived().filter( Q(tag__isnull=False, tag__taxonomy__isnull=False), object_id_clause, - ).select_related("tag__taxonomy").order_by("object_id") + ).select_related("tag__taxonomy").order_by("object_id", "id") if prefetch_orgs: all_object_tags = all_object_tags.prefetch_related("tag__taxonomy__taxonomyorg_set") @@ -293,19 +294,22 @@ def export_tags_in_csv_file(object_id, file_dir, file_name) -> None: def set_exported_object_tags( content_key: ContentKey, exported_tags: TagValuesByTaxonomyIdDict, -) -> None: +) -> dict[str, int]: """ - Replace the object's tags with the tags from an export, keeping what archiving hides. + Replace the object's tags with the tags from an export, keeping its hidden associations. + + A hidden association is one that is archived, or whose tag or taxonomy is archived. ``exported_tags`` maps a + taxonomy export id to that taxonomy's tag values. Every other association of the object is cleared first, and a + deletion-locked one is archived instead of deleted. Tags from an archived taxonomy are skipped. Tags from a + taxonomy that does not exist on this instance become placeholder associations. ``delete_object_tags`` also + removes the hidden associations, and it too archives a deletion-locked one. - ``exported_tags`` maps a taxonomy export id to that taxonomy's tag values. Only the associations that - ``ObjectTag.objects.unarchived()`` returns are cleared, and a deletion-locked one is archived instead of deleted. - Tags from an archived taxonomy are skipped and logged. Tags from a taxonomy that does not exist on this instance - become placeholder associations, as before. ``delete_object_tags`` clears everything. + Returns the number of skipped tag values for each archived taxonomy, keyed by export id. """ content_key_str = str(content_key) + skipped: dict[str, int] = {} - # A queryset delete, so that a deletion-locked association is archived rather than deleted. - ObjectTag.objects.filter(object_id=content_key_str).unarchived().delete() + oel_tagging.delete_object_tags(content_key_str, keep_hidden=True) for taxonomy_export_id, tags_values in exported_tags.items(): if not tags_values: @@ -316,7 +320,7 @@ def set_exported_object_tags( taxonomy = Taxonomy.objects.filter(export_id=export_id).first() if taxonomy is not None and taxonomy.archived: # No placeholder either: ObjectTag.resync() would re-link it to the archived taxonomy. - log.info("Skipping tags %s for %s: taxonomy %s is archived", tags_values, content_key_str, export_id) + skipped[export_id] = len(tags_values) continue oel_tagging.tag_object( object_id=content_key_str, @@ -344,6 +348,8 @@ def set_exported_object_tags( content_object=ContentObjectData(object_id=content_key_str) ) + return skipped + def import_course_tags_from_csv(csv_path, course_id) -> None: """ @@ -366,6 +372,7 @@ def get_exported_tags(block) -> TagValuesByTaxonomyIdDict: return result course_key = CourseKey.from_string(str(course_id)) + skipped_by_taxonomy: Counter[str] = Counter() for block in tags_in_blocks: exported_tags = get_exported_tags(block) @@ -376,10 +383,13 @@ def get_exported_tags(block) -> TagValuesByTaxonomyIdDict: raise ValueError(f"Invalid format of csv in: '{block}'.") if block_type == 'course': - set_exported_object_tags(course_key, exported_tags) + skipped_by_taxonomy.update(set_exported_object_tags(course_key, exported_tags)) else: block_key = course_key.make_usage_key(block_type, block_id) - set_exported_object_tags(block_key, exported_tags) + skipped_by_taxonomy.update(set_exported_object_tags(block_key, exported_tags)) + + for export_id, count in skipped_by_taxonomy.items(): + log.info("Course tag import for %s skipped %d tags from archived taxonomy %s", course_key, count, export_id) def copy_object_tags( 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 29c0fe9be96e..fda058ee1c17 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 @@ -625,31 +625,24 @@ def _get_tags_count(self, taxonomy: Taxonomy, query_params: dict) -> int: return next(t for t in response.data["results"] if t["id"] == taxonomy.id)["tags_count"] @ddt.data( - ("tA1", {}), - ("tA1", {"org": "orgA"}), - ("ot1", {"unassigned": "true"}), + ("tA1", {}, ("three",), 2), + ("tA1", {"org": "orgA"}, ("three",), 2), + ("ot1", {"unassigned": "true"}, ("three",), 2), + ("tA1", {}, ("one", "two", "three"), 0), ) @ddt.unpack - def test_list_taxonomy_tags_count_excludes_archived(self, taxonomy_attr: str, query_params: dict) -> None: + def test_list_taxonomy_tags_count_excludes_archived( + self, taxonomy_attr: str, query_params: dict, archived_values: tuple[str, ...], expected_count: int + ) -> None: """ Archived tags are hidden from the taxonomy's tag list, so they must not be counted either. """ taxonomy = getattr(self, taxonomy_attr) for value in ("one", "two", "three"): Tag.objects.create(taxonomy=taxonomy, value=value) - Tag.objects.filter(taxonomy=taxonomy, value="three").update(archived=True) - - assert self._get_tags_count(taxonomy, query_params) == 2 - - def test_list_taxonomy_tags_count_all_archived_is_zero(self) -> None: - """ - A taxonomy whose tags are all archived reports no tags. - """ - for value in ("one", "two", "three"): - Tag.objects.create(taxonomy=self.tA1, value=value) - Tag.objects.filter(taxonomy=self.tA1).update(archived=True) + Tag.objects.filter(taxonomy=taxonomy, value__in=archived_values).update(archived=True) - assert self._get_tags_count(self.tA1, {}) == 0 + assert self._get_tags_count(taxonomy, query_params) == expected_count def test_list_taxonomy_type_mixed(self) -> None: """ diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 24a51c57843f..069c0d5f57ee 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,6 +1,5 @@ """Tests for the Tagging models""" import copy -import csv import io import os import tempfile @@ -13,6 +12,8 @@ from fs.osfs import OSFS from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 +from openedx_tagging.import_export.api import import_tags +from openedx_tagging.import_export.parsers import ParserFormat from openedx_tagging.models import ObjectTag, Tag, Taxonomy from organizations.models import Organization @@ -249,6 +250,7 @@ def test_get_tags(self): assert result[0]["depth"] == 0 +@ddt.ddt class TestAPIObjectTags(TestGetAllObjectTagsMixin, TestCase): """ Tests object tag API functions. @@ -340,9 +342,6 @@ def test_get_library_object_tags(self): } def test_get_library_object_tags_excludes_archived_association(self): - """ - An archived association is left out of a library's tags, and a block with no other tags is dropped. - """ problem_id = f"lb:orgA:lib_{self.block_suffix}:problem:problem1_{self.block_suffix}" ObjectTag.objects.filter(object_id=problem_id, _value="Tag 1.1").update(archived=True) @@ -351,6 +350,7 @@ def test_get_library_object_tags_excludes_archived_association(self): ) expected = copy.deepcopy(self.expected_library_objecttags) + # The problem has no other tags, so it is dropped from the result. del expected[problem_id] assert object_tags == expected assert taxonomies == { @@ -358,38 +358,26 @@ def test_get_library_object_tags_excludes_archived_association(self): self.taxonomy_2.id: self.taxonomy_2, } - def test_get_course_object_tags_excludes_archived_association(self): - """ - An archived association is left out, and the other associations on the same block are kept. - """ + @ddt.data( + ("association", ["Tag 1.2"]), + ("tag", ["Tag 1.1"]), + ) + @ddt.unpack + def test_get_course_object_tags_excludes_archived_association_or_tag( + self, archived: str, expected_sequential_values: list[str] + ): sequential_id = "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" - ObjectTag.objects.filter(object_id=sequential_id, _value="Tag 1.1").update(archived=True) - - object_tags, taxonomies = api.get_all_object_tags( - CourseKey.from_string("course-v1:orgA+test_course+test_run") - ) - - expected = copy.deepcopy(self.expected_course_objecttags) - expected[sequential_id][self.taxonomy_1.id] = ["Tag 1.2"] - assert object_tags == expected - assert taxonomies == { - self.taxonomy_1.id: self.taxonomy_1, - self.taxonomy_2.id: self.taxonomy_2, - } - - def test_get_course_object_tags_excludes_archived_tag(self): - """ - Associations to an archived tag are left out. - """ - Tag.objects.filter(pk=self.tag_1_2.pk).update(archived=True) + if archived == "association": + ObjectTag.objects.filter(object_id=sequential_id, _value="Tag 1.1").update(archived=True) + else: + Tag.objects.filter(pk=self.tag_1_2.pk).update(archived=True) object_tags, taxonomies = api.get_all_object_tags( CourseKey.from_string("course-v1:orgA+test_course+test_run") ) expected = copy.deepcopy(self.expected_course_objecttags) - sequential_id = "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" - expected[sequential_id][self.taxonomy_1.id] = ["Tag 1.1"] + expected[sequential_id][self.taxonomy_1.id] = expected_sequential_values assert object_tags == expected assert taxonomies == { self.taxonomy_1.id: self.taxonomy_1, @@ -397,9 +385,6 @@ def test_get_course_object_tags_excludes_archived_tag(self): } def test_get_course_object_tags_excludes_archived_taxonomy(self): - """ - Associations under an archived taxonomy are left out, and the taxonomy is not returned. - """ Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) object_tags, taxonomies = api.get_all_object_tags( @@ -566,21 +551,24 @@ def test_generate_csv_rows_excludes_archived(self) -> None: ) def test_generate_csv_rows_all_archived(self) -> None: - # The same prefix get_all_object_tags() uses to find the course's blocks. - block_id_prefix = str(self.course.id).replace("course-v1:", "block-v1:", 1) - ObjectTag.objects.filter( - Q(object_id=str(self.course.id)) | Q(object_id__startswith=block_id_prefix) - ).update(archived=True) + ObjectTag.objects.update(archived=True) buffer = io.StringIO() list(api.generate_csv_rows(str(self.course.id), buffer)) buffer.seek(0) - csv_rows = list(csv.reader(buffer)) + csv_content = buffer.getvalue() # Nothing is left to export, so no taxonomy columns remain, but every block still has a row. - expected_rows = [row[:3] for row in csv.reader(io.StringIO(self.expected_csv))] - assert expected_rows[0] == ["Name", "Type", "ID"] - assert csv_rows == expected_rows + assert csv_content == ( + '"Name","Type","ID"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run"\r\n' + '" test sequential","sequential","test_sequential"\r\n' + '" test vertical1","vertical","test_vertical1"\r\n' + '" test vertical2","vertical","test_vertical2"\r\n' + '" test html","html","test_html"\r\n' + '" untagged sequential","sequential","untagged_sequential"\r\n' + '" untagged vertical","vertical","untagged_vertical"\r\n' + ) def test_generate_csv_rows_keeps_disabled_taxonomy(self) -> None: # Disabled taxonomies are not archived, so their tags are still exported. @@ -672,6 +660,36 @@ def test_import_tags_invalid_taxonomy(self) -> None: assert object_tag.taxonomy is None assert object_tag.export_id == '1-taxonomy-1-1' + def _course_object_tags(self, course_key: CourseKey | str): + """ + Returns every association, hidden or not, of the course and its blocks. + """ + block_id_prefix = str(course_key).replace("course-v1:", "block-v1:", 1) + return ObjectTag.objects.filter(Q(object_id=str(course_key)) | Q(object_id__startswith=block_id_prefix)) + + def test_export_then_import_into_new_course_carries_only_visible_tags(self) -> None: + sequential_id = "block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential" + ObjectTag.objects.filter(object_id=sequential_id, _value="Tag 1.1").update(archived=True) + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + buffer = io.StringIO() + list(api.generate_csv_rows(str(self.course.id), buffer)) + csv_path = self._create_csv_file(buffer.getvalue()) + + # The import only builds usage keys from the CSV, so the second course needs no blocks of its own. + second_course_key = CourseKey.from_string("course-v1:orgA+test_course+second_run") + api.import_course_tags_from_csv(csv_path, second_course_key) + + second_course_tags = self._course_object_tags(second_course_key) + assert sorted(second_course_tags.values_list("object_id", "taxonomy_id", "_value")) == sorted([ + ("course-v1:orgA+test_course+second_run", self.taxonomy_1.id, "Tag 1.1"), + ( + "block-v1:orgA+test_course+second_run+type@sequential+block@test_sequential", + self.taxonomy_1.id, + "Tag 1.2", + ), + ]) + assert not second_course_tags.filter(Q(taxonomy=self.taxonomy_2) | Q(_export_id="2-taxonomy-2")).exists() + def _hide_association(self, hidden_by: str, hidden_pk: int) -> None: """ Hides the given association by archiving the association, its tag, or its taxonomy. @@ -679,9 +697,9 @@ def _hide_association(self, hidden_by: str, hidden_pk: int) -> None: if hidden_by == "association": ObjectTag.objects.filter(pk=hidden_pk).update(archived=True) elif hidden_by == "tag": - Tag.objects.filter(pk=self.tag_2_1.pk).update(archived=True) + Tag.objects.filter(objecttag__pk=hidden_pk).update(archived=True) else: - Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + Taxonomy.objects.filter(objecttag__pk=hidden_pk).update(archived=True) @ddt.data( ("association", False), @@ -748,24 +766,66 @@ def test_import_archives_locked_visible_association(self) -> None: def test_import_skips_archived_taxonomy(self) -> None: course_id = str(self.course.id) Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + csv_path = self._create_csv_file( + '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1","Tag 2.1"\r\n' + ) + + api.import_course_tags_from_csv(csv_path, self.course.id) + + assert [t.value for t in api.get_object_tags(course_id)] == ["Tag 1.1"] + # No placeholder either: resyncing it would link it to the archived taxonomy. + assert not ObjectTag.objects.filter(object_id=course_id, taxonomy=self.taxonomy_2).exists() + assert not ObjectTag.objects.filter(object_id=course_id, _export_id="2-taxonomy-2").exists() + def test_import_skipping_archived_taxonomy_sends_no_event_for_it(self) -> None: + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) csv_path = self._create_csv_file( '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' '"Test Course","course","course-v1:orgA+test_course+test_run","Tag 1.1","Tag 2.1"\r\n' ) - with ( - mock.patch( - "openedx.core.djangoapps.content_tagging.api.CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event" - ) as send_event, - self.assertLogs("openedx.core.djangoapps.content_tagging.api", level="INFO") as logs, - ): + + with mock.patch( + "openedx.core.djangoapps.content_tagging.api.CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event" + ) as send_event: + api.import_course_tags_from_csv(csv_path, self.course.id) + + send_event.assert_called_once() + assert send_event.call_args.kwargs["content_object"].object_id == str(self.course.id) + + def test_import_logs_skipped_archived_taxonomy_once(self) -> None: + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + csv_path = self._create_csv_file( + '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' + '"Test Course","course","course-v1:orgA+test_course+test_run","","Tag 2.1;Tag 2.2"\r\n' + '" test sequential","sequential","test_sequential","","Tag 2.1"\r\n' + ) + + with self.assertLogs("openedx.core.djangoapps.content_tagging.api", level="INFO") as logs: api.import_course_tags_from_csv(csv_path, self.course.id) - # These come before the log check so that a missing log does not hide the state they describe. - assert [t.value for t in api.get_object_tags(self.course.id)] == ["Tag 1.1"] - assert not ObjectTag.objects.filter(object_id=course_id, _export_id="2-taxonomy-2").exists() - assert not ObjectTag.objects.filter(object_id=course_id, taxonomy=self.taxonomy_2).exists() - # The skipped taxonomy sends no event, so only taxonomy_1 does. - assert send_event.call_count == 1 + messages = [record.getMessage() for record in logs.records if "2-taxonomy-2" in record.getMessage()] + assert len(messages) == 1 + # Two values on the course row and one on the block row. + assert "skipped 3 tags" in messages[0] + + def test_taxonomy_tag_import_does_not_restore_skipped_archived_taxonomy_tags(self) -> None: + new_course_key = CourseKey.from_string("course-v1:orgA+test_course+second_run") + Taxonomy.objects.filter(pk=self.taxonomy_2.pk).update(archived=True) + csv_path = self._create_csv_file( + '"Name","Type","ID","1-taxonomy-1","2-taxonomy-2"\r\n' + '"Test Course","course","course-v1:orgA+test_course+second_run","Tag 1.1","Tag 2.1"\r\n' + '" test sequential","sequential","test_sequential","Tag 1.2","Tag 2.2"\r\n' + ) + api.import_course_tags_from_csv(csv_path, new_course_key) + + # The same calls the taxonomy tag-file import endpoint makes. + import_success, _task, _plan = import_tags( + self.taxonomy_1, io.BytesIO(b"id,value\r\ntag_1_3,Tag 1.3\r\n"), ParserFormat.CSV + ) + assert import_success + api.resync_object_tags() - assert "2-taxonomy-2" in logs.output[0] + new_course_tags = self._course_object_tags(new_course_key) + assert new_course_tags.count() == 2 + assert not new_course_tags.filter(Q(taxonomy=self.taxonomy_2) | Q(_export_id="2-taxonomy-2")).exists()