From 26c624edd2913f24eb03e86547bec0006b006d9a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 15 Jul 2026 12:01:25 -0700 Subject: [PATCH 1/3] feat: remove system-defined taxonomies and content auto tagging --- cms/djangoapps/contentstore/helpers.py | 1 + .../views/tests/test_clipboard_paste.py | 5 +- .../content/search/tests/test_handlers.py | 4 - .../core/djangoapps/content_tagging/api.py | 55 ++- .../djangoapps/content_tagging/handlers.py | 110 ++---- .../rest_api/v1/tests/test_views.py | 193 +++++----- .../core/djangoapps/content_tagging/rules.py | 32 +- .../core/djangoapps/content_tagging/tasks.py | 204 ----------- .../content_tagging/tests/test_api.py | 29 +- .../content_tagging/tests/test_handlers.py | 122 +++++++ .../tests/test_objecttag_export_helpers.py | 6 - .../content_tagging/tests/test_rules.py | 45 +-- .../content_tagging/tests/test_tasks.py | 345 ------------------ .../djangoapps/content_tagging/toggles.py | 14 - .../core/djangoapps/content_tagging/types.py | 12 +- .../tests/test_mixed_modulestore.py | 20 +- 16 files changed, 347 insertions(+), 850 deletions(-) delete mode 100644 openedx/core/djangoapps/content_tagging/tasks.py create mode 100644 openedx/core/djangoapps/content_tagging/tests/test_handlers.py delete mode 100644 openedx/core/djangoapps/content_tagging/tests/test_tasks.py diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 93d71ce811bb..dee3956f0bb3 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -644,6 +644,7 @@ def _import_xml_node_to_parent( content_tagging_api.set_all_object_tags( content_key=new_xblock.location, object_tags=object_tags, + ignore_invalid_orgs=True, ) return new_xblock diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 8bed7e6d1031..4a604dae7c2d 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -256,16 +256,19 @@ def _setup_tagged_content(self, course_key) -> dict: tags=["tag_1", "tag_2"], ) - # Tag our blocks using a taxonomy that isn't enabled for any orgs -- these tags won't be pasted + # Tag our blocks using a taxonomy that isn't enabled for any orgs -- these tags won't be pasted. taxonomy_no_org = tagging_api.create_taxonomy("test_taxonomy_no_org", "Test Taxonomy No Org") Tag.objects.create(taxonomy=taxonomy_no_org, value="tag_1") Tag.objects.create(taxonomy=taxonomy_no_org, value="tag_2") + # We have to temporarily make it apply to "all orgs", then we can apply the tags then change back to "no orgs": + tagging_api.set_taxonomy_orgs(taxonomy_no_org, all_orgs=True) for object_key in all_block_keys.values(): tagging_api.tag_object( object_id=str(object_key), taxonomy=taxonomy_no_org, tags=["tag_1", "tag_2"], ) + tagging_api.set_taxonomy_orgs(taxonomy_no_org, all_orgs=False, orgs=[]) return all_block_keys diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index 6ee28a658ed3..e2215c7f3c56 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -43,10 +43,6 @@ def setUp(self): self.orgA = OrganizationFactory.create(short_name="orgA") - self.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) - self.addCleanup(self.patcher.stop) - self.patcher.start() - api.clear_meilisearch_client() # Clear the Meilisearch client to avoid leaking state from other tests def test_create_delete_xblock(self, meilisearch_client): diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index ed77d9516ee5..ecfd04a91d86 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -9,6 +9,7 @@ from typing import Iterator # noqa: UP035 import openedx_tagging.api as oel_tagging +from django.core.exceptions import ValidationError from django.db.models import Exists, OuterRef, Q, QuerySet from django.utils.timezone import now from opaque_keys.edx.keys import CollectionKey, ContainerKey, CourseKey, UsageKey @@ -25,6 +26,16 @@ from .utils import check_taxonomy_context_key_org, get_content_key_from_string, get_context_key_from_key +class InvalidOrgException(ValidationError): + """ + Exception when attempting to use a taxonomy that's not valid for the given + org's content. + """ + + def __init__(self, message: str): + """Initialize with the detailed error message""" + super().__init__(message) + def create_taxonomy( name: str, description: str | None = None, @@ -71,9 +82,6 @@ def set_taxonomy_orgs( If not `all_orgs`, the taxonomy is associated with each org in the `orgs` list. If that list is empty, the taxonomy is not associated with any orgs. """ - if taxonomy.system_defined: - raise ValueError("Cannot set orgs for a system-defined taxonomy") - TaxonomyOrg.objects.filter( taxonomy=taxonomy, rel_type=relationship, @@ -103,7 +111,6 @@ def get_taxonomies_for_org( Generates a list of the enabled Taxonomies available for the given org, sorted by name. We return a QuerySet here for ease of use with Django Rest Framework and other query-based use cases. - So be sure to use `Taxonomy.cast()` to cast these instances to the appropriate subclass before use. If no `org` is provided, then only Taxonomies which are available for _all_ Organizations are returned. @@ -200,22 +207,27 @@ def get_all_object_tags( def set_all_object_tags( content_key: ContentKey, object_tags: TagValuesByTaxonomyIdDict, + ignore_invalid_orgs=False, ) -> None: """ Sets the tags for the given content object. + + If any of the taxonomies are not configured for use by the org that owns + the content, this will raise ``InvalidOrgException``, unless you also pass + ``ignore_invalid_orgs=True``. """ for taxonomy_id, tags_values in object_tags.items(): - taxonomy = oel_tagging.get_taxonomy(taxonomy_id) - if not taxonomy: continue - tag_object( - object_id=str(content_key), - taxonomy=taxonomy, - tags=tags_values, - ) + try: + tag_object(object_id=str(content_key), taxonomy=taxonomy, tags=tags_values) + except InvalidOrgException: + if ignore_invalid_orgs: + pass + else: + raise def generate_csv_rows(object_id, buffer) -> Iterator[str]: @@ -372,18 +384,20 @@ def copy_object_tags( for taxonomy_id, taxonomy in taxonomies.items(): tags = source_object_tags.get(taxonomy_id, []) - tag_object( - object_id=str(dest_content_key), - taxonomy=taxonomy, - tags=tags, - ) + try: + tag_object( + object_id=str(dest_content_key), + taxonomy=taxonomy, + tags=tags, + ) + except InvalidOrgException: + pass def tag_object( object_id: str, taxonomy: Taxonomy, tags: list[str], - object_tag_class: type[ObjectTag] = ObjectTag, ) -> None: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id @@ -394,9 +408,6 @@ def tag_object( tags: A list of the values of the tags from this taxonomy to apply. - object_tag_class: Optional. Use a proxy subclass of ObjectTag for additional - validation. (e.g. only allow tagging certain types of objects.) - Raised Tag.DoesNotExist if the proposed tags are invalid for this taxonomy. Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. @@ -427,6 +438,10 @@ def tag_object( time=now(), content_object=ContentObjectData(object_id=object_id) ) + else: + raise InvalidOrgException( + f'Taxonomy "{taxonomy.name}" is not configured for use with organization "{context_key.org}".' + ) # Expose the oel_tagging APIs diff --git a/openedx/core/djangoapps/content_tagging/handlers.py b/openedx/core/djangoapps/content_tagging/handlers.py index f271f6d73cd9..86f9fd8dc177 100644 --- a/openedx/core/djangoapps/content_tagging/handlers.py +++ b/openedx/core/djangoapps/content_tagging/handlers.py @@ -4,112 +4,41 @@ import logging -import crum from django.dispatch import receiver -from openedx_events.content_authoring.data import CourseData, DuplicatedXBlockData, LibraryBlockData, XBlockData +from openedx_events.content_authoring.data import DuplicatedXBlockData, LibraryBlockData, XBlockData from openedx_events.content_authoring.signals import ( - COURSE_CREATED, - LIBRARY_BLOCK_CREATED, - LIBRARY_BLOCK_UPDATED, - XBLOCK_CREATED, + LIBRARY_BLOCK_DELETED, XBLOCK_DELETED, XBLOCK_DUPLICATED, - XBLOCK_UPDATED, ) -from .api import copy_object_tags -from .tasks import ( - delete_course_tags, - delete_xblock_tags, - update_course_tags, - update_library_block_tags, - update_xblock_tags, -) -from .toggles import CONTENT_TAGGING_AUTO +from . import api +from .types import ContentKey log = logging.getLogger(__name__) - -@receiver(COURSE_CREATED) -def auto_tag_course(**kwargs): - """ - Automatically tag course based on their metadata - """ - course_data = kwargs.get("course", None) - if not course_data or not isinstance(course_data, CourseData): - log.error("Received null or incorrect data for event") - return - - if not CONTENT_TAGGING_AUTO.is_enabled(course_data.course_key): - return - - update_course_tags.delay(str(course_data.course_key)) - - -@receiver(XBLOCK_CREATED) -@receiver(XBLOCK_UPDATED) -def auto_tag_xblock(**kwargs): - """ - Automatically tag XBlock based on their metadata - """ - xblock_info = kwargs.get("xblock_info", None) - if not xblock_info or not isinstance(xblock_info, XBlockData): - log.error("Received null or incorrect data for event") - return - - if not CONTENT_TAGGING_AUTO.is_enabled(xblock_info.usage_key.course_key): - return - - if xblock_info.block_type == 'course_info': - # We want to add tags only to the course id, not with its XBlock - return - - if xblock_info.block_type == "course": - # Course update is handled by XBlock of course type - update_course_tags.delay(str(xblock_info.usage_key.course_key)) - return - - update_xblock_tags.delay(str(xblock_info.usage_key)) +def _delete_tags(content_object: ContentKey) -> None: + """Delete all tags associated with the given XBlock/course/etc.""" + log.info("Deleting tags for %s", content_object) + # This is super fast; no need to do it from a celery task. + api.delete_object_tags(str(content_object)) @receiver(XBLOCK_DELETED) def delete_tag_xblock(**kwargs): """ - Automatically delete XBlock auto tags. + Delete an XBlock's tags when the block itself is deleted. """ xblock_info = kwargs.get("xblock_info", None) if not xblock_info or not isinstance(xblock_info, XBlockData): log.error("Received null or incorrect data for event") return - if not CONTENT_TAGGING_AUTO.is_enabled(xblock_info.usage_key.course_key): - return - if xblock_info.block_type == "course": # Course deletion is handled by XBlock of course type - delete_course_tags.delay(str(xblock_info.usage_key.course_key)) - - delete_xblock_tags.delay(str(xblock_info.usage_key)) - + _delete_tags(xblock_info.usage_key.course_key) -@receiver(LIBRARY_BLOCK_CREATED) -@receiver(LIBRARY_BLOCK_UPDATED) -def auto_tag_library_block(**kwargs): - """ - Automatically tag Library Blocks based on metadata - """ - if not CONTENT_TAGGING_AUTO.is_enabled(): - return - - library_block_data = kwargs.get("library_block", None) - if not library_block_data or not isinstance(library_block_data, LibraryBlockData): - log.error("Received null or incorrect data for event") - return - - current_request = crum.get_current_request() - update_library_block_tags.delay( - str(library_block_data.usage_key), current_request.LANGUAGE_CODE - ) + _delete_tags(xblock_info.usage_key) @receiver(XBLOCK_DUPLICATED) @@ -122,7 +51,20 @@ def duplicate_tags(**kwargs): log.error("Received null or incorrect data for event") return - copy_object_tags( + api.copy_object_tags( xblock_data.source_usage_key, xblock_data.usage_key, ) + + +@receiver(LIBRARY_BLOCK_DELETED) +def library_block_deleted(**kwargs) -> None: + """ + Delete an XBlock's tags when the block itself is deleted. + """ + library_block_data = kwargs.get("library_block", None) + if not library_block_data or not isinstance(library_block_data, LibraryBlockData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + _delete_tags(library_block_data.usage_key) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 0422845cc346..f9fddc26630e 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 @@ -19,7 +19,6 @@ from openedx_authz.constants import permissions as authz_permissions from openedx_authz.constants.roles import COURSE_AUDITOR, COURSE_EDITOR, COURSE_STAFF from openedx_tagging.models import Tag, Taxonomy -from openedx_tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.rest_api.v1.serializers import TaxonomySerializer from organizations.models import Organization from rest_framework import status @@ -68,7 +67,7 @@ def check_taxonomy( enabled=True, allow_multiple=True, allow_free_text=False, - system_defined=False, + read_only=False, visible_to_authors=True, export_id=None, **_ @@ -82,7 +81,7 @@ def check_taxonomy( assert data["enabled"] == enabled assert data["allow_multiple"] == allow_multiple assert data["allow_free_text"] == allow_free_text - assert data["system_defined"] == system_defined + assert data["read_only"] == read_only assert data["visible_to_authors"] == visible_to_authors assert data["export_id"] == export_id @@ -203,20 +202,22 @@ def _setUp_taxonomies(self): self.ot1 = tagging_api.create_taxonomy(name="ot1", enabled=True) self.ot2 = tagging_api.create_taxonomy(name="ot2", enabled=False) - # System defined taxonomy - self.st1 = tagging_api.create_taxonomy(name="st1", enabled=True) - self.st1.taxonomy_class = SystemDefinedTaxonomy - self.st1.save() + # Read-only taxonomy + self.ro1 = tagging_api.create_taxonomy(name="ro1", enabled=True) + self.ro1.add_tag("read only tag") + self.ro1.read_only = True + self.ro1.save() TaxonomyOrg.objects.create( - taxonomy=self.st1, + taxonomy=self.ro1, rel_type=TaxonomyOrg.RelType.OWNER, org=None, ) - self.st2 = tagging_api.create_taxonomy(name="st2", enabled=False) - self.st2.taxonomy_class = SystemDefinedTaxonomy - self.st2.save() + self.ro2 = tagging_api.create_taxonomy(name="ro2", enabled=False) + self.ro2.add_tag("read only tag in ro2") + self.ro2.read_only = True + self.ro2.save() TaxonomyOrg.objects.create( - taxonomy=self.st2, + taxonomy=self.ro2, rel_type=TaxonomyOrg.RelType.OWNER, ) @@ -346,7 +347,7 @@ def test_list_taxonomy_staff(self) -> None: Tests that staff users see all taxonomies """ # page_size=10, and so "tBA1" and "tBA2" appear on the second page - expected_taxonomies = ["ot1", "ot2", "st1", "st2", "t1", "t2", "tA1", "tA2", "tB1", "tB2"] + expected_taxonomies = ["ot1", "ot2", "ro1", "ro2", "t1", "t2", "tA1", "tA2", "tB1", "tB2"] self._test_list_taxonomy( user_attr="staff", expected_taxonomies=expected_taxonomies, @@ -365,7 +366,7 @@ def test_list_taxonomy_orgA(self, user_attr: str) -> None: """ Tests that non staff users from orgA can see only enabled taxonomies from orgA and global taxonomies """ - expected_taxonomies = ["st1", "t1", "tA1", "tBA1"] + expected_taxonomies = ["ro1", "t1", "tA1", "tBA1"] self._test_list_taxonomy( user_attr=user_attr, enabled_parameter=True, @@ -373,8 +374,8 @@ def test_list_taxonomy_orgA(self, user_attr: str) -> None: ) @ddt.data( - (True, ["ot1", "st1", "t1", "tA1", "tB1", "tBA1"]), - (False, ["ot2", "st2", "t2", "tA2", "tB2", "tBA2"]), + (True, ["ot1", "ro1", "t1", "tA1", "tB1", "tBA1"]), + (False, ["ot2", "ro2", "t2", "tA2", "tB2", "tBA2"]), ) @ddt.unpack def test_list_taxonomy_enabled_filter(self, enabled_parameter: bool, expected_taxonomies: list[str]) -> None: @@ -388,11 +389,11 @@ def test_list_taxonomy_enabled_filter(self, enabled_parameter: bool, expected_ta ) @ddt.data( - ("orgA", ["st1", "st2", "t1", "t2", "tA1", "tA2", "tBA1", "tBA2"]), - ("orgB", ["st1", "st2", "t1", "t2", "tB1", "tB2", "tBA1", "tBA2"]), - ("orgX", ["st1", "st2", "t1", "t2"]), + ("orgA", ["ro1", "ro2", "t1", "t2", "tA1", "tA2", "tBA1", "tBA2"]), + ("orgB", ["ro1", "ro2", "t1", "t2", "tB1", "tB2", "tBA1", "tBA2"]), + ("orgX", ["ro1", "ro2", "t1", "t2"]), # Non-existent orgs are ignored - ("invalidOrg", ["st1", "st2", "t1", "t2"]), + ("invalidOrg", ["ro1", "ro2", "t1", "t2"]), ) @ddt.unpack def test_list_taxonomy_org_filter(self, org_parameter: str, expected_taxonomies: list[str]) -> None: @@ -433,7 +434,7 @@ def test_list_unassigned_and_org_filter_invalid(self) -> None: @ddt.data( ("user", (), None), ("staffA", ["tA2", "tBA1", "tBA2"], None), - ("staff", ["st2", "t1", "t2"], "3"), + ("staff", ["ro2", "t1", "t2"], "3"), ) @ddt.unpack def test_list_taxonomy_pagination( @@ -520,13 +521,13 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: assert response.data["orgs"] == [self.orgA.short_name] @ddt.data( - ('staff', 11), - ("content_creatorA", 23), - ("library_staffA", 23), - ("library_userA", 23), - ("instructorA", 23), - ("course_instructorA", 23), - ("course_staffA", 23), + ('staff', 10), + ("content_creatorA", 22), + ("library_staffA", 22), + ("library_userA", 22), + ("instructorA", 22), + ("course_instructorA", 22), + ("course_staffA", 22), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -543,14 +544,9 @@ def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): assert response.data["can_add_taxonomy"] == user.is_staff assert len(response.data["results"]) == 4 for taxonomy in response.data["results"]: - if taxonomy["system_defined"]: - assert not taxonomy["can_change_taxonomy"] - assert not taxonomy["can_delete_taxonomy"] - assert taxonomy["can_tag_object"] - else: - assert taxonomy["can_change_taxonomy"] == user.is_staff - assert taxonomy["can_delete_taxonomy"] == user.is_staff - assert taxonomy["can_tag_object"] + assert taxonomy["can_change_taxonomy"] == user.is_staff # Note: taxonomy.read_only only affects its tags, + assert taxonomy["can_delete_taxonomy"] == user.is_staff # not the metadata about the taxonomy. + assert taxonomy["can_tag_object"] @ddt.ddt @@ -670,7 +666,7 @@ def test_detail_taxonomy_org_admin_see_disabled(self, taxonomy_attr: str) -> Non ) @ddt.data( - "st2", + "ro2", "t2", ) def test_detail_taxonomy_org_admin_dont_see_disabled_global(self, taxonomy_attr: str) -> None: @@ -810,8 +806,8 @@ def test_detail_taxonomy_dont_see_other_org(self, user_attr: str) -> None: @ddt.data( "ot1", "ot2", - "st1", - "st2", + "ro1", + "ro2", "t1", "t2", "tA1", @@ -869,7 +865,7 @@ def _test_api_call(self, **kwargs) -> None: check_taxonomy( response.data, taxonomy.pk, - **(TaxonomySerializer(taxonomy.cast(), context=context)).data, + **(TaxonomySerializer(taxonomy, context=context)).data, ) @@ -919,8 +915,8 @@ def _test_api_call(self, **_kwargs) -> None: @ddt.data( "ot1", "ot2", - "st1", - "st2", + "ro1", + "ro2", "t1", "t2", "tA1", @@ -1004,34 +1000,19 @@ def test_org_staff_cant_edit_other_org_taxonomies(self, taxonomy_attr: str) -> N "tB2", "tBA1", "tBA2", - + "ro1", # Even read-only taxonomies can be edited; just not their tags + "ro2", ) def test_staff_can_edit_almost_all_taxonomies(self, taxonomy_attr: str) -> None: """ - Tests that staff can edit all but system defined taxonomies + Tests that staff can edit all taxonomies """ self._test_api_call( user_attr="staff", taxonomy_attr=taxonomy_attr, # Check both status: 200 for update and 204 for delete expected_status=[status.HTTP_200_OK, status.HTTP_204_NO_CONTENT], - reason="Staff should be able to edit all but system defined taxonomies", - ) - - @ddt.data( - "st1", - "st2", - ) - def test_staff_cant_edit_system_defined_taxonomies(self, taxonomy_attr: str) -> None: - """ - Tests that staff can't edit system defined taxonomies - """ - self._test_api_call( - user_attr="staff", - taxonomy_attr=taxonomy_attr, - # Check both status: 200 for update and 204 for delete - expected_status=[status.HTTP_403_FORBIDDEN], - reason="Staff shouldn't be able to edit system defined ", + reason="Staff should be able to edit all taxonomies", ) @@ -1072,6 +1053,7 @@ def _test_api_call(self, **kwargs) -> None: "description": taxonomy.description, "enabled": taxonomy.enabled, "export_id": taxonomy.export_id, + "read_only": taxonomy.read_only, }, ) @@ -1113,6 +1095,7 @@ def _test_api_call(self, **kwargs) -> None: "description": taxonomy.description, "enabled": taxonomy.enabled, "export_id": taxonomy.export_id, + "read_only": taxonomy.read_only, }, ) @@ -1229,22 +1212,6 @@ def test_update_org_invalid_inputs(self, all_orgs: bool, orgs: list[str], reason response = self.client.get(url) assert response.data["orgs"] == [self.orgA.short_name] - def test_update_org_system_defined(self) -> None: - """ - Tests that is not possible to change the orgs associated with a system defined taxonomy - """ - url = TAXONOMY_ORG_UPDATE_ORG_URL.format(pk=self.st1.pk) - self.client.force_authenticate(user=self.staff) - - response = self.client.put(url, {"orgs": [self.orgA.short_name]}, format="json") - assert response.status_code in [status.HTTP_403_FORBIDDEN, status.HTTP_400_BAD_REQUEST] - - # Check that the orgs didn't change - url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) - response = self.client.get(url) - assert response.data["orgs"] == [] - assert response.data["all_orgs"] - @ddt.data( "staffA", "content_creatorA", @@ -1981,18 +1948,18 @@ def test_get_copied_tags(self): @ddt.data( ('staff', 'courseA', 10), - ('staff', 'libraryA', 17), - ('staff', 'collection_key', 17), - ("content_creatorA", 'courseA', 20, False), - ("content_creatorA", 'libraryA', 23, False), - ("content_creatorA", 'collection_key', 23, False), - ("library_staffA", 'libraryA', 23, False), # Library users can only view objecttags, not change them? - ("library_staffA", 'collection_key', 23, False), - ("library_userA", 'libraryA', 23, False), - ("library_userA", 'collection_key', 23, False), - ("instructorA", 'courseA', 20), - ("course_instructorA", 'courseA', 20), - ("course_staffA", 'courseA', 20), + ('staff', 'libraryA', 13), + ('staff', 'collection_key', 13), + ("content_creatorA", 'courseA', 14, False), + ("content_creatorA", 'libraryA', 17, False), + ("content_creatorA", 'collection_key', 17, False), + ("library_staffA", 'libraryA', 17, False), # Library users can only view objecttags, not change them? + ("library_staffA", 'collection_key', 17, False), + ("library_userA", 'libraryA', 17, False), + ("library_userA", 'collection_key', 17, False), + ("instructorA", 'courseA', 14), + ("course_instructorA", 'courseA', 14), + ("course_staffA", 'courseA', 14), ) @ddt.unpack def test_object_tags_query_count( @@ -2024,6 +1991,11 @@ def test_object_tags_query_count( url = OBJECT_TAGS_URL.format(object_id=object_id) user = getattr(self, user_attr) self.client.force_authenticate(user=user) + # Pre-GET the endpoint to populate the in-memory caches of the current site and waffle flags. + # Without this, the number of queries can sometimes be different depending on the order in which these tests get + # run by the test runner, causing the test to occasionally be flaky. + self.client.get(url) + # Now run the test and count the queries: with self.assertNumQueries(expected_queries): response = self.client.get(url) @@ -2827,7 +2799,7 @@ def test_import_free_text(self, file_format) -> None: ) assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.data == f"Invalid taxonomy ({self.taxonomy.id}): You cannot import a free-form taxonomy." + assert response.data == f"Invalid taxonomy ({self.taxonomy.id}): You cannot import to a free-text taxonomy." # Check if the taxonomy has no tags, since it is free text url = TAXONOMY_TAGS_URL.format(pk=self.taxonomy.id) @@ -2879,12 +2851,12 @@ class TestTaxonomyTagsViewSet(TestTaxonomyObjectsMixin, APITestCase): """ @ddt.data( ('staff', 11), - ("content_creatorA", 13), - ("library_staffA", 13), - ("library_userA", 13), - ("instructorA", 13), - ("course_instructorA", 13), - ("course_staffA", 13), + ("content_creatorA", 11), + ("library_staffA", 11), + ("library_userA", 11), + ("instructorA", 11), + ("course_instructorA", 11), + ("course_staffA", 11), ) @ddt.unpack def test_taxonomy_tags_query_count(self, user_attr: str, expected_queries: int): @@ -2895,6 +2867,12 @@ def test_taxonomy_tags_query_count(self, user_attr: str, expected_queries: int): user = getattr(self, user_attr) self.client.force_authenticate(user=user) + + # Pre-GET the endpoint to populate the in-memory caches of the current site and waffle flags. + # Without this, the number of queries can sometimes be different depending on the order in which these tests get + # run by the test runner, causing the test to occasionally be flaky. + self.client.get(url) + # Now test the number of queries: with self.assertNumQueries(expected_queries): response = self.client.get(url) @@ -2904,3 +2882,26 @@ def test_taxonomy_tags_query_count(self, user_attr: str, expected_queries: int): for taxonomy in response.data["results"]: assert taxonomy["can_change_tag"] == user.is_staff assert taxonomy["can_delete_tag"] == user.is_staff + + @ddt.data( + ('staff', ), + ("content_creatorA", ), + ("library_staffA", ), + ) + @ddt.unpack + def test_taxonomy_tags_read_only(self, user_attr: str): + """ + Check taxonomy tag editing permissions with a read-only taxonomy + """ + url = f"{TAXONOMY_TAGS_URL}".format(pk=self.ro1.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 not response.data["can_add_tag"] + assert len(response.data["results"]) == 1 + for taxonomy in response.data["results"]: + assert not taxonomy["can_change_tag"] + assert not taxonomy["can_delete_tag"] diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 3b70076189ef..858168b6a622 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -157,8 +157,6 @@ def can_view_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: if not taxonomy: return True - taxonomy = taxonomy.cast() - # Taxonomy admins can view any taxonomy if oel_tagging.is_taxonomy_admin(user): return True @@ -186,7 +184,7 @@ def can_change_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: Returns True if the given user can edit the given taxonomy. System definied taxonomies cannot be edited - Taxonomy admins can edit any non system defined taxonomies + Taxonomy admins can edit any taxonomies (but can't change tags if the taxonomy is read-only). Only taxonomy admins can edit all org taxonomies Org-level staff can edit any taxonomy that is associated with one of their orgs. """ @@ -194,13 +192,7 @@ def can_change_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: if not taxonomy: return True - taxonomy = taxonomy.cast() - - # System definied taxonomies cannot be edited - if taxonomy.system_defined: - return False - - # Taxonomy admins can edit any non system defined taxonomies + # Taxonomy admins can edit any taxonomies if oel_tagging.is_taxonomy_admin(user): return True @@ -264,7 +256,7 @@ def can_view_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) """ # Note: in the REST API, where we're dealing with multiple taxonomies at once, permissions # are also enforced by ObjectTagTaxonomyOrgFilterBackend. - return not taxonomy or (taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy)) + return not taxonomy or (taxonomy.enabled and can_view_taxonomy(user, taxonomy)) @rules.predicate @@ -356,18 +348,18 @@ def can_change_object_tag( @rules.predicate def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) -> bool: """ - Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies - (these don't have predefined tags). + Even taxonomy admins cannot add tags to read-only or free-text taxonomies. """ taxonomy = tag.taxonomy if tag else None if taxonomy: - taxonomy = taxonomy.cast() - return oel_tagging.is_taxonomy_admin(user) and ( - not tag - or not taxonomy - or (bool(taxonomy) and not taxonomy.allow_free_text and not taxonomy.system_defined) - ) - + if taxonomy.read_only: + return False # Cannot edit tags in read-only taxonomies. + if taxonomy.allow_free_text: + return False # Cannot edit tags in free-text taxonomies. + # FIXME: properly block adding new tags in this case. (when tag=None, we don't have access to 'taxonomy', so the + # permissions check needs to pass a dummy tag object.) + # Also, for superusers, this permissions check is bypassed: https://github.com/openedx/openedx-core/issues/635 + return oel_tagging.is_taxonomy_admin(user) # Taxonomy rules.set_perm("oel_tagging.add_taxonomy", can_create_taxonomy) diff --git a/openedx/core/djangoapps/content_tagging/tasks.py b/openedx/core/djangoapps/content_tagging/tasks.py deleted file mode 100644 index 8f67f737c2d3..000000000000 --- a/openedx/core/djangoapps/content_tagging/tasks.py +++ /dev/null @@ -1,204 +0,0 @@ -""" -Defines asynchronous celery task for auto-tagging content -""" -from __future__ import annotations - -import logging - -from celery import shared_task -from celery_utils.logged_task import LoggedTask -from django.conf import settings -from django.contrib.auth import get_user_model -from edx_django_utils.monitoring import set_code_owner_attribute -from opaque_keys.edx.keys import CourseKey, UsageKey -from opaque_keys.edx.locator import LibraryUsageLocatorV2 -from openedx_tagging.models import Taxonomy - -from xmodule.modulestore.django import modulestore - -from . import api -from .types import ContentKey - -LANGUAGE_TAXONOMY_ID = -1 - -log = logging.getLogger(__name__) -User = get_user_model() - - -def _set_initial_language_tag(content_key: ContentKey, lang_code: str) -> None: - """ - Create a tag for the language taxonomy in the content_object if it doesn't exist. - - lang_code is the two-letter language code, optionally with country suffix. - - If the language is not configured in the plataform or the language tag doesn't exist, - use the default language of the platform. - """ - lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID).cast() - - if lang_code and not api.get_object_tags(object_id=str(content_key), taxonomy_id=lang_taxonomy.id): - try: - lang_tag = lang_taxonomy.tag_for_external_id(lang_code) - except api.oel_tagging.TagDoesNotExist: - default_lang_code = settings.LANGUAGE_CODE - logging.warning( - "Language not configured in the plataform: %s. Using default language: %s", - lang_code, - default_lang_code, - ) - lang_tag = lang_taxonomy.tag_for_external_id(default_lang_code) - api.tag_object( - object_id=str(content_key), - taxonomy=lang_taxonomy, - tags=[lang_tag.value], - ) - - -def _delete_tags(content_object: ContentKey) -> None: - api.delete_object_tags(str(content_object)) - - -@shared_task(base=LoggedTask) -@set_code_owner_attribute -def update_course_tags(course_key_str: str) -> bool: - """ - Updates the automatically-managed tags for a course - (whenever a course is created or updated) - - Params: - course_key_str (str): identifier of the Course - """ - try: - course_key = CourseKey.from_string(course_key_str) - - log.info("Updating tags for Course with id: %s", course_key) - - course = modulestore().get_course(course_key) - if course: - lang_code = course.language - _set_initial_language_tag(course_key, lang_code) - - return True - except Exception as e: # pylint: disable=broad-except - log.error("Error updating tags for Course with id: %s. %s", course_key, e) - return False - - -@shared_task(base=LoggedTask) -@set_code_owner_attribute -def delete_course_tags(course_key_str: str) -> bool: - """ - Delete the tags for a Course (when the course itself has been deleted). - - Params: - course_key_str (str): identifier of the Course - """ - try: - course_key = CourseKey.from_string(course_key_str) - - log.info("Deleting tags for Course with id: %s", course_key) - - _delete_tags(course_key) - - return True - except Exception as e: # pylint: disable=broad-except - log.error("Error deleting tags for Course with id: %s. %s", course_key, e) - return False - - -@shared_task(base=LoggedTask) -@set_code_owner_attribute -def update_xblock_tags(usage_key_str: str) -> bool: - """ - Updates the automatically-managed tags for a XBlock - (whenever an XBlock is created/updated). - - Params: - usage_key_str (str): identifier of the XBlock - """ - try: - usage_key = UsageKey.from_string(usage_key_str) - - log.info("Updating tags for XBlock with id: %s", usage_key) - - if usage_key.course_key.is_course: - course = modulestore().get_course(usage_key.course_key) - if course is None: - return True - lang_code = course.language - else: - return True - - _set_initial_language_tag(usage_key, lang_code) - - return True - except Exception as e: # pylint: disable=broad-except - log.error("Error updating tags for XBlock with id: %s. %s", usage_key, e) - return False - - -@shared_task(base=LoggedTask) -@set_code_owner_attribute -def delete_xblock_tags(usage_key_str: str) -> bool: - """ - Delete the tags for a XBlock (when the XBlock itself is deleted). - - Params: - usage_key_str (str): identifier of the XBlock - """ - try: - usage_key = UsageKey.from_string(usage_key_str) - - log.info("Deleting tags for XBlock with id: %s", usage_key) - - _delete_tags(usage_key) - - return True - except Exception as e: # pylint: disable=broad-except - log.error("Error deleting tags for XBlock with id: %s. %s", usage_key, e) - return False - - -@shared_task(base=LoggedTask) -@set_code_owner_attribute -def update_library_block_tags(usage_key_str: str, language_code: str) -> bool: - """ - Updates the automatically-managed tags for a content library block - whenever it is created/updated - - Params: - usage_key_str (str): identifier of the Library Block - langauge_code (str): the preferred language code of the user - """ - try: - usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) - - log.info("Updating tags for Library Block with id: %s", usage_key) - - _set_initial_language_tag(usage_key, language_code) - return True - except Exception as e: # pylint: disable=broad-except - log.error("Error updating tags for XBlock with id: %s. %s", usage_key, e) - return False - - -@shared_task(base=LoggedTask) -@set_code_owner_attribute -def delete_library_block_tags(usage_key_str: str) -> bool: - """ - Delete the tags for a Library Block (when the Library Block itself is deleted). - - Params: - usage_key_str (str): identifier of the Library Block - """ - try: - usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) - - log.info("Deleting tags for Library Block with id: %s", usage_key) - - _delete_tags(usage_key) - - return True - except Exception as e: # pylint: disable=broad-except - log.error("Error deleting tags for Library Block with id: %s. %s", usage_key, e) - return False diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index a3d965cef1c7..ea97706a9d1d 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -4,6 +4,7 @@ import tempfile import ddt +import pytest from django.test.testcases import TestCase from fs.osfs import OSFS from opaque_keys.edx.keys import CourseKey, UsageKey @@ -133,7 +134,7 @@ class TestAPITaxonomy(TestTaxonomyMixin, TestCase): def test_get_taxonomies_enabled_subclasses(self): with self.assertNumQueries(1): - taxonomies = list(taxonomy.cast() for taxonomy in api.get_taxonomies()) + taxonomies = list(api.get_taxonomies()) assert taxonomies == [ self.taxonomy_all_orgs, self.taxonomy_no_orgs, @@ -172,10 +173,7 @@ def test_get_taxonomies_enabled_subclasses(self): def test_get_taxonomies_for_org(self, org_attr, enabled, expected): org_owner = getattr(self, org_attr).short_name if org_attr else None taxonomies = list( - taxonomy.cast() - for taxonomy in api.get_taxonomies_for_org( - org_short_name=org_owner, enabled=enabled - ) + api.get_taxonomies_for_org(org_short_name=org_owner, enabled=enabled) ) assert taxonomies == [ getattr(self, taxonomy_attr) for taxonomy_attr in expected @@ -381,7 +379,7 @@ def test_copy_cross_org_tags(self): with self.assertNumQueries(31): # TODO why so high? self._test_copy_object_tags(src_key, dst_key, expected_tags) - def test_tag_collection(self): + def test_tag_collection(self) -> None: collection_key = LibraryCollectionLocator.from_string("lib-collection:orgA:libX:1") api.tag_object( @@ -398,7 +396,7 @@ def test_tag_collection(self): self.taxonomy_3.id: self.taxonomy_3, } - def test_tag_container(self): + def test_tag_container(self) -> None: unit_key = LibraryContainerLocator.from_string('lct:orgA:libX:unit:unit1') api.tag_object( @@ -415,6 +413,23 @@ def test_tag_container(self): self.taxonomy_3.id: self.taxonomy_3, } + def test_tag_container_invalid_org(self) -> None: + """ + Test what happens when we try to use a taxonomy that's not enabled for + the content's organization + """ + unit_key_org_a = LibraryContainerLocator.from_string('lct:orgA:libX:unit:unit1') + unit_key_org_b = LibraryContainerLocator.from_string('lct:orgB:libX:unit:unit1') + tag = "Tag 3.1" + + api.tag_object(object_id=str(unit_key_org_a), taxonomy=self.taxonomy_3, tags=[tag]) + + with pytest.raises( + api.InvalidOrgException, + match='Taxonomy "Taxonomy 3" is not configured for use with organization "orgB".', + ): + api.tag_object(object_id=str(unit_key_org_b), taxonomy=self.taxonomy_3, tags=[tag]) + class TestExportImportTags(TaggedCourseMixin): """ diff --git a/openedx/core/djangoapps/content_tagging/tests/test_handlers.py b/openedx/core/djangoapps/content_tagging/tests/test_handlers.py new file mode 100644 index 000000000000..6e5c6d23b4b5 --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/tests/test_handlers.py @@ -0,0 +1,122 @@ +""" +Test for auto-tagging content +""" +from __future__ import annotations + +from django.test import LiveServerTestCase +from organizations.models import Organization + +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries.api import ( + create_library, + create_library_block, + delete_library_block, + restore_library_block, +) +from openedx.core.djangolib.testing.utils import skip_unless_cms +from xmodule.modulestore.tests.django_utils import ( + TEST_DATA_SPLIT_MODULESTORE, + ImmediateOnCommitMixin, + ModuleStoreTestCase, +) + +from .. import api +from ..types import ContentKey + + +@skip_unless_cms # Automatically deleting tags when an object is deleted only applies to the CMS +class TestAutoTagHandlers( # type: ignore[misc] + ImmediateOnCommitMixin, + ModuleStoreTestCase, + LiveServerTestCase +): + """ + Test if the Course and XBlock tags are automatically created + """ + + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def _check_tag(self, object_key: ContentKey | str, taxonomy_id: int, value: str | None) -> bool: + """ + Check if the ObjectTag exists for the given object_id and taxonomy_id + + If value is None, check if the ObjectTag does not exists + """ + object_tags = list(api.get_object_tags(str(object_key), taxonomy_id=taxonomy_id)) + object_tag = object_tags[0] if len(object_tags) == 1 else None + if len(object_tags) > 1: + raise ValueError("Found too many object tags") + if value is None: + assert not object_tag, f"Expected no tag for taxonomy_id={taxonomy_id}, " \ + f"but one found with value={object_tag.value}" + else: + assert object_tag, f"Tag for taxonomy_id={taxonomy_id} with value={value} with expected, but none found" + assert object_tag.value == value, f"Tag value mismatch {object_tag.value} != {value}" + + return True + + def setUp(self) -> None: + 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") + + # Create a taxonomy and tag + self.taxonomy = api.create_taxonomy("Test Taxonomy") + self.tag = api.add_tag_to_taxonomy(self.taxonomy, "new tag for testing") + api.set_taxonomy_orgs(self.taxonomy, all_orgs=True) + + def test_create_delete_xblock(self) -> None: + # Create course + 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, course.location, "sequential", "test_sequential") + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + + usage_key_str = str(vertical.location) + + # Apply a tag to the XBlock + api.tag_object(usage_key_str, self.taxonomy, [self.tag.value]) + assert self._check_tag(usage_key_str, self.taxonomy.id, self.tag.value) + + # Delete the XBlock + self.store.delete_item(vertical.location, self.user_id) + + # Check if the tags are deleted + assert self._check_tag(usage_key_str, self.taxonomy.id, None) + + def test_create_delete_restore_library_block(self) -> None: + # Create library + library = create_library( + org=self.orgA, + slug="lib_a", + title="Library Org A", + description="This is a library from Org A", + ) + + library_block = create_library_block(library.key, "problem", "Problem1") + usage_key_str = str(library_block.usage_key) + + # Apply a tag to the XBlock + api.tag_object(usage_key_str, self.taxonomy, [self.tag.value]) + assert self._check_tag(usage_key_str, self.taxonomy.id, self.tag.value) + + # Soft delete the XBlock + delete_library_block(library_block.usage_key) + + # Check that the tags are not deleted + assert self._check_tag(usage_key_str, self.taxonomy.id, self.tag.value) + + # Restore the XBlock + restore_library_block(library_block.usage_key) + + # Check if the tags are still present for the Library Block + assert self._check_tag(usage_key_str, self.taxonomy.id, self.tag.value) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py b/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py index f5525f530628..a169e73880c4 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_objecttag_export_helpers.py @@ -2,7 +2,6 @@ Test the objecttag_export_helpers module """ import time -from unittest.mock import patch from openedx_tagging.models import ObjectTag from organizations.models import Organization @@ -204,11 +203,6 @@ class TaggedCourseMixin(TestGetAllObjectTagsMixin, ModuleStoreTestCase): # type def setUp(self): super().setUp() - # Patch modulestore - 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 = CourseFactory.create( org=self.orgA.short_name, diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 4432310147c7..02a20dd5e515 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -7,7 +7,7 @@ from django.test import TestCase from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocatorV2 from openedx_authz.constants import permissions as authz_permissions -from openedx_tagging.models import Tag, UserSystemDefinedTaxonomy +from openedx_tagging.models import Tag from openedx_tagging.rules import ObjectTagPermissionItem from common.djangoapps.student.auth import add_users, update_org_role @@ -264,25 +264,6 @@ def test_change_taxonomy_all_no_org(self, perm, taxonomy_attr): assert not self.user_org2.has_perm(perm, taxonomy) assert not self.learner.has_perm(perm, taxonomy) - @ddt.data( - "oel_tagging.change_taxonomy", - "oel_tagging.delete_taxonomy", - ) - def test_system_taxonomy(self, perm): - """ - Test that even taxonomy administrators cannot edit/delete system taxonomies. - """ - system_taxonomy = api.create_taxonomy( - name="System Languages", - ) - system_taxonomy.taxonomy_class = UserSystemDefinedTaxonomy - system_taxonomy = system_taxonomy.cast() - assert self.superuser.has_perm(perm, system_taxonomy) - assert not self.staff.has_perm(perm, system_taxonomy) - assert not self.user_both_orgs.has_perm(perm, system_taxonomy) - assert not self.user_org2.has_perm(perm, system_taxonomy) - assert not self.learner.has_perm(perm, system_taxonomy) - def test_view_taxonomy_no_orgs(self): """ Test that only Staff & Superuser can view taxonomies with no orgs. @@ -428,28 +409,26 @@ def test_change_tag(self, perm, tag_attr): assert not self.learner.has_perm(perm, tag) @ddt.data( + "oel_tagging.add_tag", "oel_tagging.change_tag", "oel_tagging.delete_tag", ) def test_system_taxonomy_tag(self, perm): """ - Test that even taxonomy administrators cannot edit/delete tags on system taxonomies. + Test that even taxonomy administrators cannot add/edit/delete tags on read-only taxonomies. """ - system_taxonomy = api.create_taxonomy( + ro_taxonomy = api.create_taxonomy( name="System Languages", ) - system_taxonomy.taxonomy_class = UserSystemDefinedTaxonomy - system_taxonomy = system_taxonomy.cast() - tag_system_taxonomy = Tag.objects.create( - taxonomy=system_taxonomy, - value="en", - ) + ro_taxonomy.read_only = True + ro_taxonomy.save() + dummy_tag = Tag(taxonomy=ro_taxonomy, value="en") # Not saved - assert self.superuser.has_perm(perm, tag_system_taxonomy) - assert not self.staff.has_perm(perm, tag_system_taxonomy) - assert not self.user_both_orgs.has_perm(perm, tag_system_taxonomy) - assert not self.user_org2.has_perm(perm, tag_system_taxonomy) - assert not self.learner.has_perm(perm, tag_system_taxonomy) + assert self.superuser.has_perm(perm, dummy_tag) + assert not self.staff.has_perm(perm, dummy_tag) + assert not self.user_both_orgs.has_perm(perm, dummy_tag) + assert not self.user_org2.has_perm(perm, dummy_tag) + assert not self.learner.has_perm(perm, dummy_tag) @ddt.data( "oel_tagging.change_tag", diff --git a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py deleted file mode 100644 index f36f5c475e1b..000000000000 --- a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py +++ /dev/null @@ -1,345 +0,0 @@ -""" -Test for auto-tagging content -""" -from __future__ import annotations - -from unittest.mock import patch - -from django.http import HttpRequest -from django.test import LiveServerTestCase, override_settings -from edx_toggles.toggles.testutils import override_waffle_flag -from openedx_tagging.models import ObjectTag, Tag, Taxonomy -from organizations.models import Organization - -from common.djangoapps.student.tests.factories import UserFactory -from openedx.core.djangoapps.content_libraries.api import ( - create_library, - create_library_block, - delete_library_block, - restore_library_block, -) -from openedx.core.djangolib.testing.utils import skip_unless_cms -from xmodule.modulestore.tests.django_utils import ( - TEST_DATA_SPLIT_MODULESTORE, - ImmediateOnCommitMixin, - ModuleStoreTestCase, -) - -from .. import api -from ..models.base import TaxonomyOrg -from ..toggles import CONTENT_TAGGING_AUTO -from ..types import ContentKey - -LANGUAGE_TAXONOMY_ID = -1 - - -class LanguageTaxonomyTestMixin: - """ - Mixin for test cases that expect the Language System Taxonomy to exist. - """ - - def setUp(self): - """ - When pytest runs, it creates the database by inspecting models, not by - running migrations. So data created by our migrations is not present. - In particular, the Language Taxonomy is not present. So this mixin will - create the taxonomy, simulating the effect of the following migrations: - 1. openedx_tagging.migrations.0012_language_taxonomy - 2. content_tagging.migrations.0007_system_defined_org_2 - 3. openedx_tagging.migrations.0015_taxonomy_export_id - """ - super().setUp() - Taxonomy.objects.get_or_create(id=-1, defaults={ - "name": "Languages", - "description": "Languages that are enabled on this system.", - "enabled": True, - "allow_multiple": False, - "allow_free_text": False, - "visible_to_authors": True, - "export_id": "-1_languages", - "_taxonomy_class": "openedx_tagging.models.system_defined.LanguageTaxonomy", - }) - TaxonomyOrg.objects.get_or_create(taxonomy_id=-1, defaults={"org": None}) - - -@skip_unless_cms # Auto-tagging is only available in the CMS -@override_waffle_flag(CONTENT_TAGGING_AUTO, active=True) -class TestAutoTagging( # type: ignore[misc] - LanguageTaxonomyTestMixin, - ImmediateOnCommitMixin, - ModuleStoreTestCase, - LiveServerTestCase -): - """ - Test if the Course and XBlock tags are automatically created - """ - - MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - - def _check_tag(self, object_key: ContentKey, taxonomy_id: int, value: str | None): - """ - Check if the ObjectTag exists for the given object_id and taxonomy_id - - If value is None, check if the ObjectTag does not exists - """ - object_tags = list(api.get_object_tags(str(object_key), taxonomy_id=taxonomy_id)) - object_tag = object_tags[0] if len(object_tags) == 1 else None - if len(object_tags) > 1: - raise ValueError("Found too many object tags") - if value is None: - assert not object_tag, f"Expected no tag for taxonomy_id={taxonomy_id}, " \ - f"but one found with value={object_tag.value}" - else: - assert object_tag, f"Tag for taxonomy_id={taxonomy_id} with value={value} with expected, but none found" - assert object_tag.value == value, f"Tag value mismatch {object_tag.value} != {value}" - - return True - - 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.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) - self.addCleanup(self.patcher.stop) - self.patcher.start() - - def test_create_course(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "pl"}, - ) - - # Check if the tags are created in the Course - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Polski") - - def test_only_tag_course_id(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "pl"}, - ) - object_id = str(course.id).replace('course-v1:', '') - - # Check that only one object tag is created for the course - tags = ObjectTag.objects.filter(object_id__contains=object_id) - assert len(tags) == 1 - assert tags[0].value == "Polski" - assert tags[0].object_id == str(course.id) - - @override_settings(LANGUAGE_CODE='pt-br') - def test_create_course_invalid_language(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "11"}, - ) - - # Check if the tags are created in the Course is the system default - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Português (Brasil)") - - @override_settings(LANGUAGES=[('pt', 'Portuguese')], LANGUAGE_DICT={'pt': 'Portuguese'}, LANGUAGE_CODE='pt') - def test_create_course_unsuported_language(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "en"}, - ) - - # Check if the tags are created in the Course is the system default - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") - - @override_settings(LANGUAGE_CODE='pt') - def test_create_course_no_tag_default_language(self): - # Remove Portuguese tag - Tag.objects.filter(taxonomy_id=LANGUAGE_TAXONOMY_ID, value="Portuguese").delete() - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "11"}, - ) - - # No tags created - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) - - def test_update_course(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "pt-br"}, - ) - - # Simulates user manually changing a tag - lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) - api.tag_object( - object_id=str(course.id), - taxonomy=lang_taxonomy, - tags=["Español (España)"] - ) - - # Update course language - course.language = "en" - self.store.update_item(course, self.user_id) - - # Does not automatically update the tag - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Español (España)") - - def test_create_delete_xblock(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "pt-br"}, - ) - - # Create XBlocks - sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") - vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") - - usage_key_str = str(vertical.location) - - # Check if the tags are created in the XBlock - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, "Português (Brasil)") - - # Delete the XBlock - self.store.delete_item(vertical.location, self.user_id) - - # Check if the tags are deleted - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) - - @override_waffle_flag(CONTENT_TAGGING_AUTO, active=False) - def test_waffle_disabled_create_update_course(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "pt"}, - ) - - # No tags created - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) - - # Update course language - course.language = "en" - self.store.update_item(course, self.user_id) - - # No tags created - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) - - @override_waffle_flag(CONTENT_TAGGING_AUTO, active=False) - def test_waffle_disabled_create_delete_xblock(self): - # Create course - course = self.store.create_course( - self.orgA.short_name, - "test_course", - "test_run", - self.user_id, - fields={"language": "pt"}, - ) - - # Create XBlocks - sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") - vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") - - usage_key_str = str(vertical.location) - - # No tags created - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) - - # Delete the XBlock - self.store.delete_item(vertical.location, self.user_id) - - # Still no tags - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) - - def test_create_delete_restore_library_block(self): - # Create library - library = create_library( - org=self.orgA, - slug="lib_a", - title="Library Org A", - description="This is a library from Org A", - ) - - fake_request = HttpRequest() - fake_request.LANGUAGE_CODE = "pt-br" - # Note: self.captureOnCommitCallbacks(execute=True) is required for the library events to fire here. - with patch('crum.get_current_request', return_value=fake_request), self.captureOnCommitCallbacks(execute=True): - # Create Library Block - library_block = create_library_block(library.key, "problem", "Problem1") - - usage_key_str = str(library_block.usage_key) - - # Check if the tags are created in the Library Block with the user's preferred language - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, 'Português (Brasil)') - - # Soft delete the XBlock - delete_library_block(library_block.usage_key) - - # Check that the tags are not deleted - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, 'Português (Brasil)') - - # Restore the XBlock - restore_library_block(library_block.usage_key) - - # Check if the tags are still present in the Library Block with the user's preferred language - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, 'Português (Brasil)') - - @override_waffle_flag(CONTENT_TAGGING_AUTO, active=False) - def test_waffle_disabled_create_delete_library_block(self): - # Create library - library = create_library( - org=self.orgA, - slug="lib_a2", - title="Library Org A 2", - description="This is a library from Org A 2", - ) - - fake_request = HttpRequest() - fake_request.LANGUAGE_CODE = "pt-br" - with patch('crum.get_current_request', return_value=fake_request): - # Create Library Block - library_block = create_library_block(library.key, "problem", "Problem2") - - usage_key_str = str(library_block.usage_key) - - # No tags created - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) - - # Delete the XBlock - delete_library_block(library_block.usage_key) - - # Still no tags - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) - - # Restore the XBlock - with patch('crum.get_current_request', return_value=fake_request): - restore_library_block(library_block.usage_key) - - # Still no tags - assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) diff --git a/openedx/core/djangoapps/content_tagging/toggles.py b/openedx/core/djangoapps/content_tagging/toggles.py index a6b0a4e19afa..69e98d3f3e26 100644 --- a/openedx/core/djangoapps/content_tagging/toggles.py +++ b/openedx/core/djangoapps/content_tagging/toggles.py @@ -4,20 +4,6 @@ from edx_toggles.toggles import WaffleFlag -from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag - -# .. toggle_name: content_tagging.auto -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Setting this enables automatic tagging of content -# .. toggle_type: feature_flag -# .. toggle_category: admin -# .. toggle_use_cases: open_edx -# .. toggle_creation_date: 2023-08-30 -# .. toggle_tickets: https://github.com/openedx/modular-learning/issues/79 -CONTENT_TAGGING_AUTO = CourseWaffleFlag('content_tagging.auto', __name__) - - # .. toggle_name: content_tagging.disabled # .. toggle_implementation: WaffleFlag # .. toggle_default: False diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index 2c4aac1690cb..72cbea44d1e7 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -3,15 +3,13 @@ """ from __future__ import annotations -from typing import Dict, List, Union # noqa: UP035 - from opaque_keys.edx.keys import CollectionKey, ContainerKey, CourseKey, UsageKey from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_tagging.models import Taxonomy -ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey, CollectionKey, ContainerKey] # noqa: UP007 -ContextKey = Union[LibraryLocatorV2, CourseKey] # noqa: UP007 +type ContentKey = LibraryLocatorV2 | CourseKey | UsageKey | CollectionKey | ContainerKey +type ContextKey = LibraryLocatorV2 | CourseKey -TagValuesByTaxonomyIdDict = Dict[int, List[str]] # noqa: UP006 -TagValuesByObjectIdDict = Dict[str, TagValuesByTaxonomyIdDict] # noqa: UP006 -TaxonomyDict = Dict[int, Taxonomy] # noqa: UP006 +type TagValuesByTaxonomyIdDict = dict[int, list[str]] +type TagValuesByObjectIdDict = dict[str, TagValuesByTaxonomyIdDict] +type TaxonomyDict = dict[int, Taxonomy] diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 7de75de0237e..b0c468b248bb 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -577,11 +577,10 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp assert len(items_in_tree) == expected_items_in_tree # split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, - # check CONTENT_TAGGING_AUTO CourseWaffleFlag + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.split, 4, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.split, 3, 2, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1110,10 +1109,11 @@ def test_has_changes_missing_child(self, default_ms, default_branch): # Split # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, - # check CONTENT_TAGGING_AUTO CourseWaffleFlag + # delete tags (+ BEGIN + COMMIT), check LTI consumer config (+ BEGIN + COMMIT), check LTI passport + # (+ BEGIN + COMMIT) # Find: active_versions, 2 structures (published & draft), definition (unnecessary) # Sends: updated draft and published structures and active_versions - @ddt.data((ModuleStoreEnum.Type.split, 11, 2, 3)) + @ddt.data((ModuleStoreEnum.Type.split, 13, 2, 3)) @ddt.unpack def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1133,10 +1133,11 @@ def test_delete_item(self, default_ms, num_mysql, max_find, max_send): # Split: # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, - # check CONTENT_TAGGING_AUTO CourseWaffleFlag + # delete tags (+ BEGIN + COMMIT), check LTI consumer config (+ BEGIN + COMMIT), check LTI passport + # (+ BEGIN + COMMIT) # find: draft and published structures, definition (unnecessary) # sends: update published (why?), draft, and active_versions - @ddt.data((ModuleStoreEnum.Type.split, 11, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.split, 13, 3, 3)) @ddt.unpack def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ @@ -1183,10 +1184,11 @@ def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send # Split: # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, - # check CONTENT_TAGGING_AUTO CourseWaffleFlag + # delete tags (+ BEGIN + COMMIT), check LTI consumer config (+ BEGIN + COMMIT), check LTI passport + # (+ BEGIN + COMMIT) # find: structure (cached) # send: update structure and active_versions - @ddt.data((ModuleStoreEnum.Type.split, 11, 1, 2)) + @ddt.data((ModuleStoreEnum.Type.split, 13, 1, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ From 52e3dc3f4966aebdbda51b0569fd980c183680c5 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 15 Jul 2026 17:28:45 -0700 Subject: [PATCH 2/3] temp: use WIP version of openedx-core --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 05f7630f3a51..155ca6c39592 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -840,7 +840,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/kernel.in # xblocks-contrib -openedx-core==1.1.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/remove-system-defined # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 0dda45866702..674a2a5fd685 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1383,7 +1383,7 @@ openedx-calc==5.0.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # xblocks-contrib -openedx-core==1.1.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/remove-system-defined # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index b06f157fb76f..90c3f488f7d1 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1018,7 +1018,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==1.1.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/remove-system-defined # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index a66a29307700..f3cb84ed0ca0 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1058,7 +1058,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==1.1.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/remove-system-defined # via # -c requirements/constraints.txt # -r requirements/edx/base.txt From 7b4c37c2cd274894d9ef6c8637cbe03ba7140b59 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 16 Jul 2026 15:58:10 -0700 Subject: [PATCH 3/3] chore: update PII annotation safelist for new openedx-core version --- .annotation_safe_list.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 4c93795458ff..66248b6ea4f1 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -380,8 +380,6 @@ openedx_content.ContainerType: ".. no_pii:": "No PII" openedx_content.ContainerVersion: ".. no_pii:": "No PII" -openedx_content.Draft: - ".. no_pii:": "No PII" openedx_content.EntityList: ".. no_pii:": "No PII" openedx_content.EntityListRow: @@ -398,10 +396,6 @@ openedx_content.PublishLogRecord: ".. no_pii:": "No PII" openedx_content.PublishableEntity: ".. no_pii:": "No PII" -openedx_content.PublishableEntityVersion: - ".. no_pii:": "No PII" -openedx_content.PublishableEntityVersionDependency: - ".. no_pii:": "No PII" openedx_content.Published: ".. no_pii:": "No PII" openedx_content.Section: