From 0a6eb51166d2c035191d5b9ed5acffb3a913212e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 5 Oct 2023 09:38:24 -0700 Subject: [PATCH] feat: Simplify Content Tagging Models (#33378) * feat: Simplify tagging - remove Content*Taxonomy models * chore: bump version of openedx-learning * chore: fix lint errors --- .../core/djangoapps/content_tagging/api.py | 58 ++++++----- .../0002_system_defined_taxonomies.py | 6 +- .../migrations/0006_simplify_models.py | 22 +++++ .../content_tagging/models/__init__.py | 5 - .../djangoapps/content_tagging/models/base.py | 85 +++------------- .../content_tagging/models/system_defined.py | 27 ----- .../core/djangoapps/content_tagging/tasks.py | 64 +++++------- .../content_tagging/tests/test_api.py | 98 +++++++++---------- .../content_tagging/tests/test_models.py | 67 ------------- .../content_tagging/tests/test_rules.py | 32 ------ .../content_tagging/tests/test_tasks.py | 56 ++++------- .../core/djangoapps/content_tagging/types.py | 8 ++ requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 17 files changed, 173 insertions(+), 365 deletions(-) create mode 100644 openedx/core/djangoapps/content_tagging/migrations/0006_simplify_models.py delete mode 100644 openedx/core/djangoapps/content_tagging/models/system_defined.py delete mode 100644 openedx/core/djangoapps/content_tagging/tests/test_models.py create mode 100644 openedx/core/djangoapps/content_tagging/types.py diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index bc420b908489..4ff21dff3f3b 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -3,15 +3,15 @@ """ from __future__ import annotations -from typing import Iterator, List, Type +from typing import Iterator import openedx_tagging.core.tagging.api as oel_tagging -from django.db.models import QuerySet -from opaque_keys.edx.keys import CourseKey, UsageKey +from django.db.models import Q, QuerySet, Exists, OuterRef from openedx_tagging.core.tagging.models import Taxonomy from organizations.models import Organization -from .models import ContentObjectTag, ContentTaxonomy, TaxonomyOrg +from .models import ContentObjectTag, TaxonomyOrg +from .types import ContentKey def create_taxonomy( @@ -21,12 +21,9 @@ def create_taxonomy( required=False, allow_multiple=False, allow_free_text=False, - taxonomy_class: Type = ContentTaxonomy, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. - - If `taxonomy_class` not provided, then uses ContentTaxonomy. """ return oel_tagging.create_taxonomy( name=name, @@ -35,14 +32,13 @@ def create_taxonomy( required=required, allow_multiple=allow_multiple, allow_free_text=allow_free_text, - taxonomy_class=taxonomy_class, ) def set_taxonomy_orgs( taxonomy: Taxonomy, all_orgs=False, - orgs: List[Organization] = None, + orgs: list[Organization] = None, relationship: TaxonomyOrg.RelType = TaxonomyOrg.RelType.OWNER, ): """ @@ -81,7 +77,7 @@ def set_taxonomy_orgs( def get_taxonomies_for_org( enabled=True, - org_owner: Organization = None, + org_owner: Organization | None = None, ) -> QuerySet: """ Generates a list of the enabled Taxonomies available for the given org, sorted by name. @@ -94,32 +90,38 @@ def get_taxonomies_for_org( If you want the disabled Taxonomies, pass enabled=False. If you want all Taxonomies (both enabled and disabled), pass enabled=None. """ - taxonomies = oel_tagging.get_taxonomies(enabled=enabled) - return ContentTaxonomy.taxonomies_for_org( - org=org_owner, - queryset=taxonomies, + org_short_name = org_owner.short_name if org_owner else None + return oel_tagging.get_taxonomies(enabled=enabled).filter( + Exists( + TaxonomyOrg.get_relationships( + taxonomy=OuterRef("pk"), + rel_type=TaxonomyOrg.RelType.OWNER, + org_short_name=org_short_name, + ) + ) ) def get_content_tags( - object_id: str, taxonomy_id: str = None + object_key: ContentKey, + taxonomy_id: str | None = None, ) -> Iterator[ContentObjectTag]: """ Generates a list of content tags for a given object. Pass taxonomy to limit the returned object_tags to a specific taxonomy. """ - for object_tag in oel_tagging.get_object_tags( - object_id=object_id, + return oel_tagging.get_object_tags( + object_id=str(object_key), taxonomy_id=taxonomy_id, - ): - yield ContentObjectTag.cast(object_tag) + object_tag_class=ContentObjectTag, + ) def tag_content_object( + object_key: ContentKey, taxonomy: Taxonomy, tags: list, - object_id: CourseKey | UsageKey, ) -> list[ContentObjectTag]: """ This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or @@ -136,14 +138,18 @@ def tag_content_object( Raises ValueError if the proposed tags are invalid for this taxonomy. Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. """ - content_tags = [] - for object_tag in oel_tagging.tag_object( + if not taxonomy.system_defined: + # We require that this taxonomy is linked to the content object's "org" or linked to "all orgs" (None): + org_short_name = object_key.org + if not taxonomy.taxonomyorg_set.filter(Q(org__short_name=org_short_name) | Q(org=None)).exists(): + raise ValueError(f"The specified Taxonomy is not enabled for the content object's org ({org_short_name})") + oel_tagging.tag_object( taxonomy=taxonomy, tags=tags, - object_id=str(object_id), - ): - content_tags.append(ContentObjectTag.cast(object_tag)) - return content_tags + object_id=str(object_key), + object_tag_class=ContentObjectTag, + ) + return get_content_tags(str(object_key), taxonomy_id=taxonomy.id) # Expose the oel_tagging APIs diff --git a/openedx/core/djangoapps/content_tagging/migrations/0002_system_defined_taxonomies.py b/openedx/core/djangoapps/content_tagging/migrations/0002_system_defined_taxonomies.py index 78b0baa9496b..14a3e6ace128 100644 --- a/openedx/core/djangoapps/content_tagging/migrations/0002_system_defined_taxonomies.py +++ b/openedx/core/djangoapps/content_tagging/migrations/0002_system_defined_taxonomies.py @@ -21,7 +21,7 @@ class Migration(migrations.Migration): 'indexes': [], 'constraints': [], }, - bases=(openedx.core.djangoapps.content_tagging.models.base.ContentTaxonomyMixin, 'oel_tagging.usersystemdefinedtaxonomy'), + bases=('oel_tagging.usersystemdefinedtaxonomy', ), ), migrations.CreateModel( name='ContentLanguageTaxonomy', @@ -32,7 +32,7 @@ class Migration(migrations.Migration): 'indexes': [], 'constraints': [], }, - bases=(openedx.core.djangoapps.content_tagging.models.base.ContentTaxonomyMixin, 'oel_tagging.languagetaxonomy'), + bases=('oel_tagging.languagetaxonomy', ), ), migrations.CreateModel( name='ContentOrganizationTaxonomy', @@ -43,7 +43,7 @@ class Migration(migrations.Migration): 'indexes': [], 'constraints': [], }, - bases=(openedx.core.djangoapps.content_tagging.models.base.ContentTaxonomyMixin, 'oel_tagging.modelsystemdefinedtaxonomy'), + bases=('oel_tagging.modelsystemdefinedtaxonomy', ), ), migrations.CreateModel( name='OrganizationModelObjectTag', diff --git a/openedx/core/djangoapps/content_tagging/migrations/0006_simplify_models.py b/openedx/core/djangoapps/content_tagging/migrations/0006_simplify_models.py new file mode 100644 index 000000000000..7e8eb99ee71c --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/migrations/0006_simplify_models.py @@ -0,0 +1,22 @@ +# Generated by Django 3.2.21 on 2023-09-29 23:32 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_tagging', '0005_auto_20230830_1517'), + ] + + operations = [ + migrations.DeleteModel( + name='ContentAuthorTaxonomy', + ), + migrations.DeleteModel( + name='ContentLanguageTaxonomy', + ), + migrations.DeleteModel( + name='ContentTaxonomy', + ), + ] diff --git a/openedx/core/djangoapps/content_tagging/models/__init__.py b/openedx/core/djangoapps/content_tagging/models/__init__.py index 4606c5b85386..f330b8ae819b 100644 --- a/openedx/core/djangoapps/content_tagging/models/__init__.py +++ b/openedx/core/djangoapps/content_tagging/models/__init__.py @@ -4,9 +4,4 @@ from .base import ( TaxonomyOrg, ContentObjectTag, - ContentTaxonomy, -) -from .system_defined import ( - ContentLanguageTaxonomy, - ContentAuthorTaxonomy, ) diff --git a/openedx/core/djangoapps/content_tagging/models/base.py b/openedx/core/djangoapps/content_tagging/models/base.py index 38dcfb5b7572..77061e46118a 100644 --- a/openedx/core/djangoapps/content_tagging/models/base.py +++ b/openedx/core/djangoapps/content_tagging/models/base.py @@ -3,12 +3,12 @@ """ from __future__ import annotations +from django.core.exceptions import ValidationError from django.db import models -from django.db.models import Exists, OuterRef, Q, QuerySet +from django.db.models import Q, QuerySet from django.utils.translation import gettext as _ from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import LearningContextKey -from opaque_keys.edx.locator import BlockUsageLocator +from opaque_keys.edx.keys import LearningContextKey, UsageKey from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization @@ -91,7 +91,7 @@ class Meta: proxy = True @property - def object_key(self) -> BlockUsageLocator | LearningContextKey: + def object_key(self) -> UsageKey | LearningContextKey: """ Returns the object ID parsed as a UsageKey or LearningContextKey. Raises InvalidKeyError object_id cannot be parse into one of those key types. @@ -99,75 +99,14 @@ def object_key(self) -> BlockUsageLocator | LearningContextKey: Returns None if there's no object_id. """ try: - return LearningContextKey.from_string(str(self.object_id)) + return LearningContextKey.from_string(self.object_id) except InvalidKeyError: - return BlockUsageLocator.from_string(str(self.object_id)) + return UsageKey.from_string(self.object_id) - -class ContentTaxonomyMixin: - """ - Taxonomy which can only tag Content objects (e.g. XBlocks or Courses) via ContentObjectTag. - - Also ensures a valid TaxonomyOrg owner relationship with the content object. - """ - - @classmethod - def taxonomies_for_org( - cls, - queryset: QuerySet, - org: Organization | None = None, - ) -> QuerySet: - """ - Filters the given QuerySet to those ContentTaxonomies which are available for the given organization. - - If no `org` is provided, then only ContentTaxonomies available to all organizations are returned. - If `org` is provided, then ContentTaxonomies available to this organizations are also returned. - """ - org_short_name = org.short_name if org else None - return queryset.filter( - Exists( - TaxonomyOrg.get_relationships( - taxonomy=OuterRef("pk"), - rel_type=TaxonomyOrg.RelType.OWNER, - org_short_name=org_short_name, - ) - ) - ) - - def _check_object(self, object_tag: ObjectTag) -> bool: - """ - Returns True if this ObjectTag has a valid object_id. - """ - content_tag = ContentObjectTag.cast(object_tag) + def clean(self): + super().clean() + # Make sure that object_id is a valid key try: - content_tag.object_key - except InvalidKeyError: - return False - return super()._check_object(content_tag) - - def _check_taxonomy(self, object_tag: ObjectTag) -> bool: - """ - Returns True if this taxonomy is owned by the tag's org. - """ - content_tag = ContentObjectTag.cast(object_tag) - try: - object_key = content_tag.object_key - except InvalidKeyError: - return False - if not TaxonomyOrg.get_relationships( - taxonomy=self, - rel_type=TaxonomyOrg.RelType.OWNER, - org_short_name=object_key.org, - ).exists(): - return False - return super()._check_taxonomy(content_tag) - - -class ContentTaxonomy(ContentTaxonomyMixin, Taxonomy): - """ - Taxonomy that accepts ContentTags, - and ensures a valid TaxonomyOrg owner relationship with the content object. - """ - - class Meta: - proxy = True + self.object_key + except InvalidKeyError as err: + raise ValidationError("object_id is not a valid opaque key string.") from err diff --git a/openedx/core/djangoapps/content_tagging/models/system_defined.py b/openedx/core/djangoapps/content_tagging/models/system_defined.py deleted file mode 100644 index 9948625455e7..000000000000 --- a/openedx/core/djangoapps/content_tagging/models/system_defined.py +++ /dev/null @@ -1,27 +0,0 @@ -""" -System defined models -""" -from openedx_tagging.core.tagging.models import ( - UserSystemDefinedTaxonomy, - LanguageTaxonomy, -) - -from .base import ContentTaxonomyMixin - - -class ContentLanguageTaxonomy(ContentTaxonomyMixin, LanguageTaxonomy): - """ - Language system-defined taxonomy that accepts ContentTags - """ - - class Meta: - proxy = True - - -class ContentAuthorTaxonomy(ContentTaxonomyMixin, UserSystemDefinedTaxonomy): - """ - Author system-defined taxonomy that accepts ContentTags - """ - - class Meta: - proxy = True diff --git a/openedx/core/djangoapps/content_tagging/tasks.py b/openedx/core/djangoapps/content_tagging/tasks.py index 3ffa6c29f107..f28b6d156dcc 100644 --- a/openedx/core/djangoapps/content_tagging/tasks.py +++ b/openedx/core/djangoapps/content_tagging/tasks.py @@ -10,12 +10,13 @@ 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.keys import LearningContextKey, UsageKey from openedx_tagging.core.tagging.models import Taxonomy from xmodule.modulestore.django import modulestore from . import api +from .types import ContentKey LANGUAGE_TAXONOMY_ID = -1 @@ -23,53 +24,32 @@ User = get_user_model() -def _has_taxonomy(taxonomy: Taxonomy, content_object: CourseKey | UsageKey) -> bool: - """ - Return True if this Taxonomy have some Tag set in the content_object - """ - _exausted = object() - - content_tags = api.get_content_tags(object_id=str(content_object), taxonomy_id=taxonomy.id) - return next(content_tags, _exausted) is not _exausted - - -def _set_initial_language_tag(content_object: CourseKey | UsageKey, lang: str) -> None: +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) + lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID).cast() - if lang and not _has_taxonomy(lang_taxonomy, content_object): - tags = api.get_tags(lang_taxonomy) - is_language_configured = any(lang_code == lang for lang_code, _ in settings.LANGUAGES) is not None - if not is_language_configured: + if lang_code and not api.get_content_tags(object_key=content_key, taxonomy_id=lang_taxonomy.id).exists(): + 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, - settings.LANGUAGE_CODE, + lang_code, + default_lang_code, ) - lang = settings.LANGUAGE_CODE - - lang_tag = next((tag for tag in tags if tag.external_id == lang), None) - if lang_tag is None: - if not is_language_configured: - logging.error( - "Language tag not found for default language: %s. Skipping", lang - ) - return - - logging.warning( - "Language tag not found for language: %s. Using default language: %s", lang, settings.LANGUAGE_CODE - ) - lang_tag = next(tag for tag in tags if tag.external_id == settings.LANGUAGE_CODE) - - api.tag_content_object(lang_taxonomy, [lang_tag.id], content_object) + lang_tag = lang_taxonomy.tag_for_external_id(default_lang_code) + api.tag_content_object(content_key, lang_taxonomy, [lang_tag.value]) -def _delete_tags(content_object: CourseKey | UsageKey) -> None: +def _delete_tags(content_object: ContentKey) -> None: api.delete_object_tags(str(content_object)) @@ -84,14 +64,14 @@ def update_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: - course_key = CourseKey.from_string(course_key_str) + course_key = LearningContextKey.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 = course.language - _set_initial_language_tag(course_key, lang) + lang_code = course.language + _set_initial_language_tag(course_key, lang_code) return True except Exception as e: # pylint: disable=broad-except @@ -109,7 +89,7 @@ def delete_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: - course_key = CourseKey.from_string(course_key_str) + course_key = LearningContextKey.from_string(course_key_str) log.info("Deleting tags for Course with id: %s", course_key) @@ -140,11 +120,11 @@ def update_xblock_tags(usage_key_str: str) -> bool: course = modulestore().get_course(usage_key.course_key) if course is None: return True - lang = course.language + lang_code = course.language else: return True - _set_initial_language_tag(usage_key, lang) + _set_initial_language_tag(usage_key, lang_code) return True except Exception as e: # pylint: disable=broad-except diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 263ae761ef57..c0595dffb679 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -2,7 +2,7 @@ import ddt from django.test.testcases import TestCase from opaque_keys.edx.keys import CourseKey, UsageKey -from openedx_tagging.core.tagging.models import ObjectTag, Tag +from openedx_tagging.core.tagging.models import Tag from organizations.models import Organization from .. import api @@ -65,59 +65,42 @@ def setUp(self): ) # ObjectTags self.all_orgs_course_tag = api.tag_content_object( + object_key=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), taxonomy=self.taxonomy_all_orgs, - tags=[self.tag_all_orgs.id], - object_id=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), + tags=[self.tag_all_orgs.value], )[0] self.all_orgs_block_tag = api.tag_content_object( - taxonomy=self.taxonomy_all_orgs, - tags=[self.tag_all_orgs.id], - object_id=UsageKey.from_string( + object_key=UsageKey.from_string( "block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde" ), + taxonomy=self.taxonomy_all_orgs, + tags=[self.tag_all_orgs.value], )[0] self.both_orgs_course_tag = api.tag_content_object( + object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), taxonomy=self.taxonomy_both_orgs, - tags=[self.tag_both_orgs.id], - object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + tags=[self.tag_both_orgs.value], )[0] self.both_orgs_block_tag = api.tag_content_object( - taxonomy=self.taxonomy_both_orgs, - tags=[self.tag_both_orgs.id], - object_id=UsageKey.from_string( + object_key=UsageKey.from_string( "block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde" ), + taxonomy=self.taxonomy_both_orgs, + tags=[self.tag_both_orgs.value], )[0] self.one_org_block_tag = api.tag_content_object( - taxonomy=self.taxonomy_one_org, - tags=[self.tag_one_org.id], - object_id=UsageKey.from_string( + object_key=UsageKey.from_string( "block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde" ), + taxonomy=self.taxonomy_one_org, + tags=[self.tag_one_org.value], )[0] self.disabled_course_tag = api.tag_content_object( + object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), taxonomy=self.taxonomy_disabled, - tags=[self.tag_disabled.id], - object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + tags=[self.tag_disabled.value], )[0] - # Invalid object tags must be manually created - self.all_orgs_invalid_tag = ObjectTag.objects.create( - taxonomy=self.taxonomy_all_orgs, - tag=self.tag_all_orgs, - object_id="course-v1_OpenedX_DemoX_Demo_Course", - ) - self.one_org_invalid_org_tag = ObjectTag.objects.create( - taxonomy=self.taxonomy_one_org, - tag=self.tag_one_org, - object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", - ) - self.no_orgs_invalid_tag = ObjectTag.objects.create( - taxonomy=self.taxonomy_no_orgs, - tag=self.tag_no_orgs, - object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), - ) - @ddt.ddt class TestAPITaxonomy(TestTaxonomyMixin, TestCase): @@ -190,11 +173,11 @@ def test_get_content_tags_valid_for_org( ): taxonomy_id = getattr(self, taxonomy_attr).id object_tag = getattr(self, object_tag_attr) - with self.assertNumQueries(2): + with self.assertNumQueries(1): valid_tags = list( api.get_content_tags( + object_key=object_tag.object_key, taxonomy_id=taxonomy_id, - object_id=object_tag.object_id, ) ) assert len(valid_tags) == 1 @@ -204,38 +187,55 @@ def test_get_content_tags_valid_for_org( ("taxonomy_disabled", "disabled_course_tag"), ("taxonomy_all_orgs", "all_orgs_course_tag"), ("taxonomy_all_orgs", "all_orgs_block_tag"), - ("taxonomy_all_orgs", "all_orgs_invalid_tag"), ("taxonomy_both_orgs", "both_orgs_course_tag"), ("taxonomy_both_orgs", "both_orgs_block_tag"), ("taxonomy_one_org", "one_org_block_tag"), - ("taxonomy_one_org", "one_org_invalid_org_tag"), ) @ddt.unpack - def test_get_content_tags_include_invalid( + def test_get_content_tags( self, taxonomy_attr, object_tag_attr, ): taxonomy_id = getattr(self, taxonomy_attr).id object_tag = getattr(self, object_tag_attr) - with self.assertNumQueries(2): + with self.assertNumQueries(1): valid_tags = list( api.get_content_tags( + object_key=object_tag.object_key, taxonomy_id=taxonomy_id, - object_id=object_tag.object_id, ) ) assert len(valid_tags) == 1 assert valid_tags[0].id == object_tag.id - @ddt.data( - "all_orgs_invalid_tag", - "one_org_invalid_org_tag", - "no_orgs_invalid_tag", - ) - def test_object_tag_not_valid_check_object(self, tag_attr): - object_tag = getattr(self, tag_attr) - assert not object_tag.is_valid() - def test_get_tags(self): assert api.get_tags(self.taxonomy_all_orgs) == [self.tag_all_orgs] + + def test_cannot_tag_across_orgs(self): + """ + Ensure that I cannot apply tags from a taxonomy that's linked to another + org. + """ + # This taxonomy is only linked to the "OpenedX org", so it can't be used for "Axim" content. + taxonomy = self.taxonomy_one_org + tags = [self.tag_one_org.value] + with self.assertRaises(ValueError) as exc: + api.tag_content_object( + object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + taxonomy=taxonomy, + tags=tags, + ) + assert "The specified Taxonomy is not enabled for the content object's org (Ax)" in str(exc.exception) + # But this will work fine: + api.tag_content_object( + object_key=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), + taxonomy=taxonomy, + tags=tags, + ) + # As will this: + api.tag_content_object( + object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + taxonomy=self.taxonomy_both_orgs, + tags=[self.tag_both_orgs.value], + ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_models.py b/openedx/core/djangoapps/content_tagging/tests/test_models.py deleted file mode 100644 index 81c5da86413c..000000000000 --- a/openedx/core/djangoapps/content_tagging/tests/test_models.py +++ /dev/null @@ -1,67 +0,0 @@ -""" -Test for Content models -""" -import ddt -from django.test.testcases import TestCase - -from openedx_tagging.core.tagging.models import ( - ObjectTag, - Tag, -) -from openedx_tagging.core.tagging.api import create_taxonomy -from ..models import ( - ContentLanguageTaxonomy, - ContentAuthorTaxonomy, -) - - -@ddt.ddt -class TestSystemDefinedModels(TestCase): - """ - Test for System defined models - """ - - @ddt.data( - (ContentLanguageTaxonomy, "taxonomy"), # Invalid object key - (ContentLanguageTaxonomy, "tag"), # Invalid external_id, invalid language - (ContentLanguageTaxonomy, "object"), # Invalid object key - (ContentAuthorTaxonomy, "taxonomy"), # Invalid object key - (ContentAuthorTaxonomy, "tag"), # Invalid external_id, User don't exits - (ContentAuthorTaxonomy, "object"), # Invalid object key - ) - @ddt.unpack - def test_validations( - self, - taxonomy_cls, - check, - ): - """ - Test that the respective validations are being called - """ - taxonomy = create_taxonomy( - name='Test taxonomy', - taxonomy_class=taxonomy_cls, - ) - - tag = Tag( - value="value", - external_id="external_id", - taxonomy=taxonomy, - ) - tag.save() - - object_tag = ObjectTag( - object_id='object_id', - taxonomy=taxonomy, - tag=tag, - ) - - check_taxonomy = check == 'taxonomy' - check_object = check == 'object' - check_tag = check == 'tag' - assert not taxonomy.validate_object_tag( - object_tag=object_tag, - check_taxonomy=check_taxonomy, - check_object=check_object, - check_tag=check_tag, - ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 442fda5a71bd..013b82255840 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -3,7 +3,6 @@ import ddt from django.contrib.auth import get_user_model from django.test.testcases import TestCase, override_settings -from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from openedx_tagging.core.tagging.models import ( Tag, @@ -161,19 +160,6 @@ def setUp(self): object_id=str(self.course2), ) - self.all_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_all_orgs, - object_id="course-v1_OpenedX_DemoX_Demo_Course", - ) - self.one_org_invalid_org_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_one_org, - object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", - ) - self.no_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_no_orgs, - object_id=str(self.course1), - ) - self.all_org_perms = ( self.tax_all_course1, self.tax_all_course2, @@ -544,30 +530,12 @@ def test_change_object_tag_org1(self, perm, tag_attr): assert not self.user_org2.has_perm(perm, perm_item) assert not self.learner.has_perm(perm, perm_item) - @ddt.data( - - ("oel_tagging.add_object_tag", "one_org_invalid_org_tag_perm"), - ("oel_tagging.add_object_tag", "all_orgs_invalid_tag_perm"), - ("oel_tagging.change_object_tag", "one_org_invalid_org_tag_perm"), - ("oel_tagging.change_object_tag", "all_orgs_invalid_tag_perm"), - ("oel_tagging.delete_object_tag", "one_org_invalid_org_tag_perm"), - ("oel_tagging.delete_object_tag", "all_orgs_invalid_tag_perm"), - ) - @ddt.unpack - def test_change_object_tag_invalid_key(self, perm, tag_attr): - perm_item = getattr(self, tag_attr) - with self.assertRaises(InvalidKeyError): - assert self.staff.has_perm(perm, perm_item) - @ddt.data( "all_orgs_course_tag", "all_orgs_block_tag", "both_orgs_course_tag", "both_orgs_block_tag", "one_org_block_tag", - "all_orgs_invalid_tag", - "one_org_invalid_org_tag", - "no_orgs_invalid_tag", "disabled_course_tag", ) def test_view_object_tag(self, tag_attr): diff --git a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py index c1c45e944962..f3c964b836c7 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py @@ -8,7 +8,7 @@ from django.core.management import call_command from django.test import override_settings from edx_toggles.toggles.testutils import override_waffle_flag -from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy +from openedx_tagging.core.tagging.models import LanguageTaxonomy, Tag, Taxonomy from organizations.models import Organization from common.djangoapps.student.tests.factories import UserFactory @@ -16,8 +16,9 @@ from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase from .. import api -from ..models import ContentLanguageTaxonomy, TaxonomyOrg +from ..models import TaxonomyOrg from ..toggles import CONTENT_TAGGING_AUTO +from ..types import ContentKey LANGUAGE_TAXONOMY_ID = -1 @@ -31,13 +32,16 @@ class TestAutoTagging(ModuleStoreTestCase): MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - def _check_tag(self, object_id: str, taxonomy_id: int, value: str | None): + 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_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy_id=taxonomy_id).first() + object_tags = api.get_content_tags(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}" @@ -52,13 +56,9 @@ def setUpClass(cls): # Run fixtures to create the system defined tags call_command("loaddata", "--app=oel_tagging", "language_taxonomy.yaml") - # Configure language taxonomy - language_taxonomy = Taxonomy.objects.get(id=-1) - language_taxonomy.taxonomy_class = ContentLanguageTaxonomy - language_taxonomy.save() - # Enable Language taxonomy for all orgs - TaxonomyOrg.objects.create(id=-1, taxonomy=language_taxonomy, org=None) + language_taxonomy = LanguageTaxonomy.objects.get(id=LANGUAGE_TAXONOMY_ID) + TaxonomyOrg.objects.create(taxonomy=language_taxonomy, org=None) super().setUpClass() @@ -80,13 +80,13 @@ def test_create_course(self): "test_course", "test_run", self.user_id, - fields={"language": "pt"}, + fields={"language": "pl"}, ) # Check if the tags are created in the Course - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Polski") - @override_settings(LANGUAGE_CODE='pt') + @override_settings(LANGUAGE_CODE='pt-br') def test_create_course_invalid_language(self): # Create course course = self.store.create_course( @@ -98,9 +98,9 @@ def test_create_course_invalid_language(self): ) # Check if the tags are created in the Course is the system default - assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Português (Brasil)") - @override_settings(LANGUAGES=[('pt', 'Portuguese')], LANGUAGE_CODE='pt') + @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( @@ -114,22 +114,6 @@ def test_create_course_unsuported_language(self): # 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_language(self): - # Remove English tag - Tag.objects.filter(taxonomy_id=LANGUAGE_TAXONOMY_ID, value="English").delete() - # 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 @@ -153,19 +137,19 @@ def test_update_course(self): "test_course", "test_run", self.user_id, - fields={"language": "pt"}, + fields={"language": "pt-br"}, ) # Simulates user manually changing a tag lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) - api.tag_content_object(lang_taxonomy, ["Spanish"], course.id) + api.tag_content_object(course.id, lang_taxonomy, ["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, "Spanish") + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Español (España)") def test_create_delete_xblock(self): # Create course @@ -174,7 +158,7 @@ def test_create_delete_xblock(self): "test_course", "test_run", self.user_id, - fields={"language": "pt"}, + fields={"language": "pt-br"}, ) # Create XBlocks @@ -184,7 +168,7 @@ def test_create_delete_xblock(self): 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, "Portuguese") + 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) diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py new file mode 100644 index 000000000000..3a9f6ec54935 --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -0,0 +1,8 @@ +""" +Types used by content tagging API and implementation +""" +from typing import Union + +from opaque_keys.edx.keys import LearningContextKey, UsageKey + +ContentKey = Union[LearningContextKey, UsageKey] diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 7f0643c04595..bae9a5be22e1 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -121,7 +121,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.1.7 +openedx-learning==0.2.0 # lti-consumer-xblock 9.6.2 contains a breaking change that makes # existing custom parameter configurations unusable. diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 192f932843d8..e2e9014d326f 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -783,7 +783,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.1.7 +openedx-learning==0.2.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 07d3d9ae0996..2c95eabd1882 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1316,7 +1316,7 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning==0.1.7 +openedx-learning==0.2.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 70c15bc73886..b636f128f415 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -923,7 +923,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.7 +openedx-learning==0.2.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 235918f5553c..c8a9dc113106 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -990,7 +990,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.7 +openedx-learning==0.2.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt