refactor: allows Taxonomy to be subclassed

* adds ContentTaxonomy and ContentTag proxy models
* updates api to create/return ContentTaxonomy and ContentTag models
* other updates to align with changes to openedx/openedx-learning#62
* fixes linting & test coverage

Author: pomegranited

openedx/features/content_tagging/api.py

@@ -1,13 +1,13 @@
 """
 Content Tagging APIs
 """
-from typing import Generator, List, Type
+from typing import Iterator, List, Type
 
 import openedx_tagging.core.tagging.api as oel_tagging
-from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy
+from openedx_tagging.core.tagging.models import Taxonomy
 from organizations.models import Organization
 
-from .models import BlockObjectTag, CourseObjectTag, TaxonomyOrg
+from .models import ContentTag, ContentTaxonomy, TaxonomyOrg
 
 
 def create_taxonomy(
@@ -18,10 +18,13 @@ 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.
+
     If `org_owners` is empty/None, then the returned taxonomy is enabled for all organizations.
     """
     taxonomy = oel_tagging.create_taxonomy(
@@ -31,7 +34,8 @@ def create_taxonomy(
         required=required,
         allow_multiple=allow_multiple,
         allow_free_text=allow_free_text,
-    )
+        taxonomy_class=taxonomy_class,
+    ).cast()
     if org_owners:
         set_taxonomy_org_owners(taxonomy, org_owners)
     return taxonomy
@@ -62,7 +66,7 @@ def set_taxonomy_org_owners(
 
 def get_taxonomies_for_org(
     org_owner: Organization = None, enabled=True
-) -> Generator[Taxonomy, None, None]:
+) -> Iterator[Taxonomy]:
     """
     Generates a list of the enabled Taxonomies owned by the given org, sorted by name.
 
@@ -73,42 +77,59 @@ def get_taxonomies_for_org(
     for taxonomy in taxonomies.all():
         org_short_name = org_owner.short_name if org_owner else None
         if TaxonomyOrg.is_owner(taxonomy, org_short_name):
-            yield taxonomy
+            yield taxonomy.cast()
+
+
+def get_object_tags(
+    object_id: str, taxonomy: Taxonomy = None, valid_only=True
+) -> Iterator[ContentTag]:
+    """
+    Generates a list of content tags for a given object.
+
+    Pass taxonomy to limit the returned object_tags to a specific taxonomy.
+
+    Pass valid_only=False when displaying tags to content authors, so they can see invalid tags too.
+    Invalid tags will (probably) be hidden from learners.
+    """
+    for object_tag in oel_tagging.get_object_tags(
+        object_id=object_id,
+        taxonomy=taxonomy,
+        valid_only=valid_only,
+    ):
+        yield ContentTag.cast(object_tag)
 
 
 def tag_object(
-    taxonomy: Taxonomy, tags: List, object_id: str, object_type: str, object_tag_class: Type=None,
-) -> List[ObjectTag]:
+    taxonomy: Taxonomy,
+    tags: List,
+    object_id: str,
+) -> List[ContentTag]:
     """
-    Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags.
+    Replace the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags.
 
     If taxonomy.allows_free_text, then the list should be a list of tag values.
     Otherwise, it should be a list of existing Tag IDs.
 
-    Raised ValueError if the proposed tags are invalid for this taxonomy.
+    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.
-
-    Sets the object_tag_class based on the given ``object_type``.
     """
-    if not object_tag_class:
-        if object_type == 'block':
-            object_tag_class = BlockObjectTag
-        elif object_type == 'course':
-            object_tag_class = CourseObjectTag
-    return oel_tagging.tag_object(
+    # Check that this will create valid ContentTags
+    content_tag = ContentTag(object_id=object_id)
+    if not content_tag.object_key:
+        raise ValueError(f"Invalid ContentTag.object_id: {object_id}")
+    content_tags = []
+    for object_tag in oel_tagging.tag_object(
         taxonomy=taxonomy,
         tags=tags,
         object_id=object_id,
-        object_type=object_type,
-        object_tag_class=object_tag_class,
-    )
+    ):
+        content_tags.append(ContentTag.cast(object_tag))
+    return content_tags
 
 
 # Expose the oel_tagging APIs
 
 get_taxonomy = oel_tagging.get_taxonomy
 get_taxonomies = oel_tagging.get_taxonomies
 get_tags = oel_tagging.get_tags
-cast_object_tag = oel_tagging.cast_object_tag
 resync_object_tags = oel_tagging.resync_object_tags
-get_object_tags = oel_tagging.get_object_tags

openedx/features/content_tagging/migrations/0001_initial.py

@@ -1,4 +1,4 @@
-# Generated by Django 3.2.20 on 2023-07-17 07:42
+# Generated by Django 3.2.20 on 2023-07-18 03:06
 
 from django.db import migrations, models
 import django.db.models.deletion
@@ -9,12 +9,12 @@ class Migration(migrations.Migration):
 
     dependencies = [
         ("oel_tagging", "__latest__"),
-        ("organizations", "0003_historicalorganizationcourse"),
+        ("organizations", "__latest__"),
     ]
 
     operations = [
         migrations.CreateModel(
-            name="BlockObjectTag",
+            name="ContentTag",
             fields=[],
             options={
                 "proxy": True,
@@ -23,6 +23,16 @@ class Migration(migrations.Migration):
             },
             bases=("oel_tagging.objecttag",),
         ),
+        migrations.CreateModel(
+            name="ContentTaxonomy",
+            fields=[],
+            options={
+                "proxy": True,
+                "indexes": [],
+                "constraints": [],
+            },
+            bases=("oel_tagging.taxonomy",),
+        ),
         migrations.CreateModel(
             name="TaxonomyOrg",
             fields=[
@@ -57,16 +67,6 @@ class Migration(migrations.Migration):
                 ),
             ],
         ),
-        migrations.CreateModel(
-            name="CourseObjectTag",
-            fields=[],
-            options={
-                "proxy": True,
-                "indexes": [],
-                "constraints": [],
-            },
-            bases=("content_tagging.blockobjecttag",),
-        ),
         migrations.AddIndex(
             model_name="taxonomyorg",
             index=models.Index(

openedx/features/content_tagging/models.py

@@ -3,15 +3,19 @@
 """
 from django.db import models
 from django.utils.translation import gettext as _
-from opaque_keys import InvalidKeyError
+from opaque_keys import InvalidKeyError, OpaqueKey
 from opaque_keys.edx.keys import CourseKey, UsageKey
+from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2
 from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy
 from organizations.models import Organization
 
 
 class TaxonomyOrg(models.Model):
     """
     Represents the many-to-many relationship between Taxonomies and Organizations.
+
+    We keep this as a separate class from ContentTaxonomy so that class can remain a proxy for Taxonomy, keeping the
+    data models and usage simple.
     """
 
     class RelType(models.TextChoices):
@@ -60,80 +64,58 @@ def is_owner(cls, taxonomy: Taxonomy, org_short_name: str) -> bool:
         return org_owners.exists()
 
 
-class OrgObjectTagMixin:
+class ContentTag(ObjectTag):
     """
-    Mixin for ObjectTag that checks the TaxonomyOrg owner relationship.
+    ObjectTag that requires an opaque key as an object ID.
     """
 
-    @property
-    def org_short_name(self):
-        """
-        Subclasses should override this to return the object tag's org short name.
-        """
-        raise NotImplementedError
-
-    def _check_taxonomy(self):
-        """
-        Returns True if this ObjectTag's taxonomy is owned by the org.
-        """
-        if not super()._check_taxonomy():
-            return False
-
-        if not TaxonomyOrg.is_owner(self.taxonomy, self.org_short_name):
-            return False
-
-        return True
-
-
-class BlockObjectTag(ObjectTag):
-    """
-    ObjectTag which requires object_id to be a valid UsageKey.
-    """
-
-    OBJECT_KEY_CLASS = UsageKey
-
     class Meta:
         proxy = True
 
+    OPAQUE_KEY_TYPES = [
+        UsageKey,
+        CourseKey,
+        LibraryLocatorV2,
+        LibraryUsageLocatorV2,
+    ]
+
     @property
-    def object_key(self):
+    def object_key(self) -> OpaqueKey:
         """
         Returns the object ID parsed as an Opaque Key, or None if invalid.
         """
         if self.object_id:
-            try:
-                return self.OBJECT_KEY_CLASS.from_string(str(self.object_id))
-            except InvalidKeyError:
-                pass
+            for OpaqueKeyClass in (UsageKey, CourseKey):
+                try:
+                    return OpaqueKeyClass.from_string(str(self.object_id))
+                except InvalidKeyError:
+                    pass
         return None
 
-    @property
-    def org_short_name(self):
-        """
-        Returns the org short name, if one can be parsed from the object ID.
-        """
-        object_key = self.object_key
-        if object_key:
-            return object_key.org
-        return None
-
-    def _check_object(self):
-        """
-        Returns True if this ObjectTag has a valid object_key.
-        """
-        # TODO: do check object_type? Do we still need it?
-        if not self.object_key:
-            return False
-
-        return super()._check_object()
-
 
-class CourseObjectTag(BlockObjectTag):
+class ContentTaxonomy(Taxonomy):
     """
-    ObjectTag which requires object_id to be a valid CourseKey.
+    Taxonomy that accepts ContentTags,
+    and ensures a valid TaxonomyOrg owner relationship with the content object.
     """
 
-    OBJECT_KEY_CLASS = CourseKey
-
     class Meta:
         proxy = True
+
+    def _check_object(self, object_tag: ObjectTag) -> bool:
+        """
+        Returns True if this ObjectTag has a valid object_id.
+        """
+        content_tag = ContentTag.cast(object_tag)
+        return super()._check_object(content_tag) and content_tag.object_key
+
+    def _check_taxonomy(self, object_tag: ObjectTag) -> bool:
+        """
+        Returns True if this taxonomy is owned by the tag's org.
+        """
+        object_key = ContentTag.cast(object_tag).object_key
+        return (
+            super()._check_taxonomy(object_tag)
+            and object_key
+            and TaxonomyOrg.is_owner(self, object_key.org)
+        )