diff --git a/docs/decisions/0007-tagging-app.rst b/docs/decisions/0007-tagging-app.rst index ed536c79..0346d93f 100644 --- a/docs/decisions/0007-tagging-app.rst +++ b/docs/decisions/0007-tagging-app.rst @@ -19,7 +19,7 @@ Taxonomy The ``openedx_tagging`` module defines ``openedx_tagging.core.models.Taxonomy``, whose data and functionality are self-contained to the ``openedx_tagging`` app. However in Studio, we need to be able to limit access to some Taxonomy by organization, using the same "course creator" access which limits course creation for an organization to a defined set of users. -So in edx-platform, we will create the ``openedx.features.content_tagging`` app, to contain the models and logic for linking Organization owners to Taxonomies. There's no need to subclass Taxonomy here; we can enforce access using the ``content_tagging.api``. +So in edx-platform, we will create the ``openedx.features.content_tagging`` app, to contain the models and logic for linking Organization owners to Taxonomies. Here, we can subclass ``Taxonomy`` as needed, preferably using proxy models. The APIs are responsible for ensuring that any ``Taxonomy`` instances are cast to the appropriate subclass. ObjectTag ~~~~~~~~~ @@ -27,7 +27,7 @@ ObjectTag Similarly, the ``openedx_tagging`` module defined ``openedx_tagging.core.models.ObjectTag``, also self-contained to the ``openedx_tagging`` app. -But to tag content in the LMS/Studio, we need to enforce ``object_id`` as a CourseKey or UsageKey type. So to do this, we subclass ``ObjectTag``, and use the ``openedx.features.tagging.registry`` to register the subclass, so that it will be picked up when this tagging API creates or resyncs object tags. +But to tag content in the LMS/Studio, we need to enforce ``object_id`` as a CourseKey or UsageKey type. So to do this, we subclass ``ObjectTag``, and use this class when creating object tags for the content taxonomies. Once the ``object_id`` is set, it is not editable, and so this key validation need not happen again. Rejected Alternatives --------------------- @@ -38,14 +38,3 @@ Embed in edx-platform Embedding the logic in edx-platform would provide the content tagging logic specifically required for the MVP. However, we plan to extend tagging to other object types (e.g. People) and contexts (e.g. Marketing), and so a generic, standalone library is preferable in the log run. - - -Subclass Taxonomy -~~~~~~~~~~~~~~~~~ - -Another approach considered was to encapsulate the complexity of ObjectTag validation in the Taxonomy class, and so use subclasses of Taxonomy to validate different types of ObjectTags, and use `multi-table inheritance`_ and django polymorphism when pulling the taxonomies from the database. - -However, because we will have a number of different types of Taxonomies, this proved non-performant. - - --.. _multi-table inheritance: https://docs.djangoproject.com/en/3.2/topics/db/models/#multi-table-inheritance diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 8b593a27..57d4c64e 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -25,11 +25,12 @@ def create_taxonomy( required=False, allow_multiple=False, allow_free_text=False, + taxonomy_class: Type = None, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. """ - taxonomy = Taxonomy.objects.create( + taxonomy = Taxonomy( name=name, description=description, enabled=enabled, @@ -37,19 +38,27 @@ def create_taxonomy( allow_multiple=allow_multiple, allow_free_text=allow_free_text, ) - return taxonomy + if taxonomy_class: + taxonomy.taxonomy_class = taxonomy_class + taxonomy.save() + return taxonomy.cast() def get_taxonomy(id: int) -> Union[Taxonomy, None]: """ - Returns a Taxonomy of the appropriate subclass which has the given ID. + Returns a Taxonomy cast to the appropriate subclass which has the given ID. """ - return Taxonomy.objects.filter(id=id).first() + taxonomy = Taxonomy.objects.filter(id=id).first() + return taxonomy.cast() if taxonomy else None def get_taxonomies(enabled=True) -> QuerySet: """ Returns a queryset containing the enabled taxonomies, 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 you want the disabled taxonomies, pass enabled=False. If you want all taxonomies (both enabled and disabled), pass enabled=None. """ @@ -65,37 +74,11 @@ def get_tags(taxonomy: Taxonomy) -> List[Tag]: Note that if the taxonomy allows free-text tags, then the returned list will be empty. """ - return taxonomy.get_tags() - - -def cast_object_tag(object_tag: ObjectTag) -> ObjectTag: - """ - Casts/copies the given object tag data into the ObjectTag subclass most appropriate for this tag. - """ - return object_tag.cast_object_tag() - - -def resync_object_tags(object_tags: QuerySet = None) -> int: - """ - Reconciles ObjectTag entries with any changes made to their associated taxonomies and tags. - - By default, we iterate over all ObjectTags. Pass a filtered ObjectTags queryset to limit which tags are resynced. - """ - if not object_tags: - object_tags = ObjectTag.objects.select_related("tag", "taxonomy") - - num_changed = 0 - for tag in object_tags: - object_tag = cast_object_tag(tag) - changed = object_tag.resync() - if changed: - object_tag.save() - num_changed += 1 - return num_changed + return taxonomy.cast().get_tags() def get_object_tags( - object_id: str, object_type: str = None, taxonomy: Taxonomy = None, valid_only=True + object_id: str, taxonomy: Taxonomy = None, valid_only=True ) -> Iterator[ObjectTag]: """ Generates a list of object tags for a given object. @@ -112,13 +95,10 @@ def get_object_tags( .select_related("tag", "taxonomy") .order_by("id") ) - if object_type: - tags = tags.filter(object_type=object_type) if taxonomy: tags = tags.filter(taxonomy=taxonomy) - for tag in tags: - object_tag = cast_object_tag(tag) + for object_tag in tags: if not valid_only or object_tag.is_valid(): yield object_tag @@ -127,8 +107,6 @@ def tag_object( taxonomy: Taxonomy, tags: List, object_id: str, - object_type: str, - object_tag_class: Type = None, ) -> List[ObjectTag]: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags. @@ -139,58 +117,4 @@ def tag_object( Raised ValueError if the proposed tags are invalid for this taxonomy. Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. """ - - if not taxonomy.allow_multiple and len(tags) > 1: - raise ValueError(_(f"Taxonomy ({taxonomy.id}) only allows one tag per object.")) - - if taxonomy.required and len(tags) == 0: - raise ValueError( - _(f"Taxonomy ({taxonomy.id}) requires at least one tag per object.") - ) - - current_tags = { - tag.tag_ref: tag - for tag in ObjectTag.objects.filter( - taxonomy=taxonomy, object_id=object_id, object_type=object_type - ) - } - updated_tags = [] - for tag_ref in tags: - if tag_ref in current_tags: - object_tag = cast_object_tag(current_tags.pop(tag_ref)) - else: - try: - tag = taxonomy.tag_set.get( - id=tag_ref, - ) - value = tag.value - except (ValueError, Tag.DoesNotExist): - # This might be ok, e.g. if taxonomy.allow_free_text. - # We'll validate below before saving. - tag = None - value = tag_ref - - object_tag = ObjectTag( - taxonomy=taxonomy, - object_id=object_id, - object_type=object_type, - tag=tag, - value=value, - name=taxonomy.name, - ) - if object_tag_class: - object_tag.object_tag_class = object_tag_class - object_tag = cast_object_tag(object_tag) - - object_tag.resync() - updated_tags.append(object_tag) - - # Save all updated tags at once to avoid partial updates - for object_tag in updated_tags: - object_tag.save() - - # ...and delete any omitted existing tags - for old_tag in current_tags.values(): - old_tag.delete() - - return updated_tags + return taxonomy.cast().tag_object(tags, object_id) diff --git a/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py b/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py new file mode 100644 index 00000000..c6216a74 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py @@ -0,0 +1,79 @@ +# Generated by Django 3.2.19 on 2023-07-18 05:54 + +import django.db.models.deletion +from django.db import migrations, models + +import openedx_learning.lib.fields + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="taxonomy", + name="system_defined", + field=models.BooleanField( + default=False, + editable=False, + help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", + ), + ), + migrations.AlterField( + model_name="tag", + name="parent", + field=models.ForeignKey( + default=None, + help_text="Tag that lives one level up from the current tag, forming a hierarchy.", + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="children", + to="oel_tagging.tag", + ), + ), + migrations.AlterField( + model_name="tag", + name="taxonomy", + field=models.ForeignKey( + default=None, + help_text="Namespace and rules for using a given set of tags.", + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="oel_tagging.taxonomy", + ), + ), + migrations.AddField( + model_name="taxonomy", + name="visible_to_authors", + field=models.BooleanField( + default=True, + editable=False, + help_text="Indicates whether this taxonomy should be visible to object authors.", + ), + ), + migrations.RemoveField( + model_name="objecttag", + name="object_type", + ), + migrations.AddField( + model_name="taxonomy", + name="_taxonomy_class", + field=models.CharField( + help_text="Taxonomy subclass used to instantiate this instance.Must be a fully-qualified module and class name.", + max_length=255, + null=True, + ), + ), + migrations.AlterField( + model_name="objecttag", + name="object_id", + field=openedx_learning.lib.fields.MultiCollationCharField( + db_collations={"mysql": "utf8mb4_unicode_ci", "sqlite": "NOCASE"}, + editable=False, + help_text="Identifier for the object being tagged", + max_length=255, + ), + ), + ] diff --git a/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py b/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py deleted file mode 100644 index 6f1beb92..00000000 --- a/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py +++ /dev/null @@ -1,20 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-05 04:52 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0001_initial"), - ] - - operations = [ - migrations.AddField( - model_name="taxonomy", - name="system_defined", - field=models.BooleanField( - default=False, - help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", - ), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py b/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py deleted file mode 100644 index faaaa66d..00000000 --- a/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py +++ /dev/null @@ -1,41 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-05 05:07 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0002_taxonomy_system_defined"), - ] - - operations = [ - migrations.CreateModel( - name="OpenObjectTag", - fields=[], - options={ - "proxy": True, - "indexes": [], - "constraints": [], - }, - bases=("oel_tagging.objecttag",), - ), - migrations.AddField( - model_name="taxonomy", - name="_object_tag_class", - field=models.CharField( - help_text="Overrides the default ObjectTag subclass associated with this taxonomy.Must be a fully-qualified module and class name.", - max_length=255, - null=True, - ), - ), - migrations.CreateModel( - name="ClosedObjectTag", - fields=[], - options={ - "proxy": True, - "indexes": [], - "constraints": [], - }, - bases=("oel_tagging.openobjecttag",), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py b/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py deleted file mode 100644 index ebb6f74b..00000000 --- a/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py +++ /dev/null @@ -1,36 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-06 03:56 - -import django.db.models.deletion -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0003_objecttag_proxies"), - ] - - operations = [ - migrations.AlterField( - model_name="tag", - name="parent", - field=models.ForeignKey( - default=None, - help_text="Tag that lives one level up from the current tag, forming a hierarchy.", - null=True, - on_delete=django.db.models.deletion.CASCADE, - related_name="children", - to="oel_tagging.tag", - ), - ), - migrations.AlterField( - model_name="tag", - name="taxonomy", - field=models.ForeignKey( - default=None, - help_text="Namespace and rules for using a given set of tags.", - null=True, - on_delete=django.db.models.deletion.CASCADE, - to="oel_tagging.taxonomy", - ), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py b/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py deleted file mode 100644 index 4b6210ec..00000000 --- a/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py +++ /dev/null @@ -1,30 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-10 06:40 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0004_tag_cascade_delete"), - ] - - operations = [ - migrations.AddField( - model_name="taxonomy", - name="visible_to_authors", - field=models.BooleanField( - default=True, - editable=False, - help_text="Indicates whether this taxonomy should be visible to object authors.", - ), - ), - migrations.AlterField( - model_name="taxonomy", - name="system_defined", - field=models.BooleanField( - default=False, - editable=False, - help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", - ), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py b/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py deleted file mode 100644 index 48832c1a..00000000 --- a/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py +++ /dev/null @@ -1,31 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-17 07:00 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0005_auto_20230710_0140"), - ] - - operations = [ - migrations.DeleteModel( - name="ClosedObjectTag", - ), - migrations.DeleteModel( - name="OpenObjectTag", - ), - migrations.RemoveField( - model_name="taxonomy", - name="_object_tag_class", - ), - migrations.AddField( - model_name="objecttag", - name="_object_tag_class", - field=models.CharField( - help_text="Overrides the ObjectTag subclass used to instantiate this ObjectTag.Must be a fully-qualified module and class name.", - max_length=255, - null=True, - ), - ), - ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index ded22db8..9d7b42e4 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -1,5 +1,6 @@ """ Tagging app data models """ -from typing import List, Type +import logging +from typing import List, Type, Union from django.db import models from django.utils.module_loading import import_string @@ -7,6 +8,9 @@ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field +log = logging.getLogger(__name__) + + # Maximum depth allowed for a hierarchical taxonomy's tree of tags. TAXONOMY_MAX_DEPTH = 3 @@ -151,6 +155,14 @@ class Taxonomy(models.Model): "Indicates whether this taxonomy should be visible to object authors." ), ) + _taxonomy_class = models.CharField( + null=True, + max_length=255, + help_text=_( + "Taxonomy subclass used to instantiate this instance." + "Must be a fully-qualified module and class name.", + ), + ) class Meta: verbose_name_plural = "Taxonomies" @@ -167,6 +179,71 @@ def __str__(self): """ return f"<{self.__class__.__name__}> ({self.id}) {self.name}" + @property + def taxonomy_class(self) -> Type: + """ + Returns the Taxonomy subclass associated with this instance, or None if none supplied. + + May raise ImportError if a custom taxonomy_class cannot be imported. + """ + if self._taxonomy_class: + return import_string(self._taxonomy_class) + return None + + @taxonomy_class.setter + def taxonomy_class(self, taxonomy_class: Union[Type, None]): + """ + Assigns the given taxonomy_class's module path.class to the field. + + Raises ValueError if the given `taxonomy_class` is a built-in class; it should be a Taxonomy-like class. + """ + if taxonomy_class: + if taxonomy_class.__module__ == "builtins": + raise ValueError( + f"Unable to assign taxonomy_class for {self}: {taxonomy_class} must be a class like Taxonomy" + ) + + # ref: https://stackoverflow.com/a/2020083 + self._taxonomy_class = ".".join( + [taxonomy_class.__module__, taxonomy_class.__qualname__] + ) + else: + self._taxonomy_class = None + + def cast(self): + """ + Returns the current Taxonomy instance cast into its taxonomy_class. + + If no taxonomy_class is set, then just returns self. + """ + try: + TaxonomyClass = self.taxonomy_class + if TaxonomyClass and not isinstance(self, TaxonomyClass): + return TaxonomyClass().copy(self) + except ImportError: + # Log error and continue + log.exception( + f"Unable to import taxonomy_class for {self}: {self._taxonomy_class}" + ) + + return self + + def copy(self, taxonomy: "Taxonomy") -> "Taxonomy": + """ + Copy the fields from the given Taxonomy into the current instance. + """ + self.id = taxonomy.id + self.name = taxonomy.name + self.description = taxonomy.description + self.enabled = taxonomy.enabled + self.required = taxonomy.required + self.allow_multiple = taxonomy.allow_multiple + self.allow_free_text = taxonomy.allow_free_text + self.system_defined = taxonomy.system_defined + self.visible_to_authors = taxonomy.visible_to_authors + self._taxonomy_class = taxonomy._taxonomy_class + return self + def get_tags(self) -> List[Tag]: """ Returns a list of all Tags in the current taxonomy, from the root(s) down to TAXONOMY_MAX_DEPTH tags, in tree order. @@ -201,6 +278,142 @@ def get_tags(self) -> List[Tag]: break return tags + def tag_object( + self, + tags: List, + object_id: str, + ) -> List["ObjectTag"]: + """ + Replaces the existing ObjectTag entries for the current taxonomy + object_id with the given list of tags. + If self.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. + Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. + """ + + if not self.allow_multiple and len(tags) > 1: + raise ValueError(_(f"Taxonomy ({self.id}) only allows one tag per object.")) + + if self.required and len(tags) == 0: + raise ValueError( + _(f"Taxonomy ({self.id}) requires at least one tag per object.") + ) + + current_tags = { + tag.tag_ref: tag + for tag in ObjectTag.objects.filter( + taxonomy=self, + object_id=object_id, + ) + } + updated_tags = [] + for tag_ref in tags: + if tag_ref in current_tags: + object_tag = current_tags.pop(tag_ref) + else: + object_tag = ObjectTag( + taxonomy=self, + object_id=object_id, + ) + + try: + object_tag.tag = self.tag_set.get( + id=tag_ref, + ) + except (ValueError, Tag.DoesNotExist): + # This might be ok, e.g. if self.allow_free_text. + # We'll validate below before saving. + object_tag.value = tag_ref + + # Resync the tag's name/value with its taxonomy/tag + object_tag.name = self.name + if object_tag.tag_id: + object_tag.value = object_tag.tag.value + + # Check that it's valid + if not self.validate_object_tag(object_tag): + raise ValueError( + _(f"Invalid object tag for taxonomy ({self.id}): {tag_ref}") + ) + updated_tags.append(object_tag) + + # Save all updated tags at once to avoid partial updates + for object_tag in updated_tags: + object_tag.save() + + # ...and delete any omitted existing tags + for old_tag in current_tags.values(): + old_tag.delete() + + return updated_tags + + def validate_object_tag( + self, + object_tag: "ObjectTag", + check_taxonomy=True, + check_tag=True, + check_object=True, + ) -> bool: + """ + Returns True if the given object tag is valid for the current Taxonomy. + + Subclasses should override the internal _validate* methods to perform their own validation checks, e.g. against + dynamically generated tag lists. + + If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. + If `check_tag` is False, then we skip validating the object tag's tag reference. + If `check_object` is False, then we skip validating the object ID/type. + """ + if check_taxonomy and not self._check_taxonomy(object_tag): + return False + + if check_tag and not self._check_tag(object_tag): + return False + + if check_object and not self._check_object(object_tag): + return False + + return True + + def _check_taxonomy( + self, + object_tag: "ObjectTag", + ) -> bool: + """ + Returns True if the given object tag is valid for the current Taxonomy. + + Subclasses can override this method to perform their own taxonomy validation checks. + """ + # Must be linked to this taxonomy + return object_tag.taxonomy_id and object_tag.taxonomy_id == self.id + + def _check_tag( + self, + object_tag: "ObjectTag", + ) -> bool: + """ + Returns True if the given object tag's value is valid for the current Taxonomy. + + Subclasses can override this method to perform their own taxonomy validation checks. + """ + # Open taxonomies only need a value. + if self.allow_free_text: + return bool(object_tag.value) + + # Closed taxonomies need an associated tag in this taxonomy + return object_tag.tag_id and object_tag.tag.taxonomy_id == self.id + + def _check_object( + self, + object_tag: "ObjectTag", + ) -> bool: + """ + Returns True if the given object tag's object is valid for the current Taxonomy. + + Subclasses can override this method to perform their own taxonomy validation checks. + """ + return bool(object_tag.object_id) + class ObjectTag(models.Model): """ @@ -223,12 +436,9 @@ class ObjectTag(models.Model): id = models.BigAutoField(primary_key=True) object_id = case_insensitive_char_field( max_length=255, + editable=False, help_text=_("Identifier for the object being tagged"), ) - object_type = case_insensitive_char_field( - max_length=255, - help_text=_("Type of object being tagged"), - ) taxonomy = models.ForeignKey( Taxonomy, null=True, @@ -264,14 +474,6 @@ class ObjectTag(models.Model): " If the tag field is set, then tag.value takes precedence over this field." ), ) - _object_tag_class = models.CharField( - null=True, - max_length=255, - help_text=_( - "Overrides the ObjectTag subclass used to instantiate this ObjectTag." - "Must be a fully-qualified module and class name.", - ), - ) class Meta: indexes = [ @@ -288,7 +490,7 @@ def __str__(self): """ User-facing string representation of an ObjectTag. """ - return f"<{self.__class__.__name__}> {self.object_id} ({self.object_type}): {self.name}={self.value}" + return f"<{self.__class__.__name__}> {self.object_id}: {self.name}={self.value}" @property def name(self) -> str: @@ -334,59 +536,23 @@ def tag_ref(self) -> str: """ return self.tag.id if self.tag_id else self._value - @property - def object_tag_class(self) -> Type: + @classmethod + def cast(cls, object_tag: "ObjectTag") -> "ObjectTag": """ - Returns the ObjectTag subclass associated with this ObjectTag, or None if none supplied. - - May raise ImportError if a custom object_tag_class cannot be imported. + Returns a cls instance with the same properties as the given ObjectTag. """ - if self._object_tag_class: - return import_string(self._object_tag_class) - return None - - @object_tag_class.setter - def object_tag_class(self, object_tag_class: Type): - """ - Assigns the given object_tag_class's module path.class to the field. - - Raises ValueError if the given `object_tag_class` is a built-in class; it should be an ObjectTag-like class. - """ - if object_tag_class.__module__ == "builtins": - raise ValueError( - f"object_tag_class {object_tag_class} must be class like ObjectTag" - ) - - # ref: https://stackoverflow.com/a/2020083 - self._object_tag_class = ".".join( - [object_tag_class.__module__, object_tag_class.__qualname__] - ) - - def cast_object_tag(self): - """ - Returns the current object tag cast into its object_tag_class. - """ - try: - ObjectTagClass = self.object_tag_class - if ObjectTagClass: - return ObjectTagClass().copy(self) - except ImportError: - # Log error and continue - log.exception(f"Unable to import custom object_tag_class for {taxonomy}") - - return self + return cls().copy(object_tag) def copy(self, object_tag: "ObjectTag") -> "ObjectTag": """ - Copy the fields from the given ObjectTag into the current instance. + Copy the fields from the given Taxonomy into the current instance. """ self.id = object_tag.id - self.object_id = object_tag.object_id - self.object_type = object_tag.object_type - self.taxonomy = object_tag.taxonomy self.tag = object_tag.tag - self._name = object_tag._name + self.taxonomy = object_tag.taxonomy + self.object_id = object_tag.object_id self._value = object_tag._value + self._name = object_tag._name return self def get_lineage(self) -> Lineage: @@ -398,116 +564,8 @@ def get_lineage(self) -> Lineage: """ return self.tag.get_lineage() if self.tag_id else [self._value] - def _check_taxonomy(self): - """ - Returns True if this ObjectTag is linked to a taxonomy. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - # Must be linked to a free-text taxonomy - return self.taxonomy_id - - def _check_tag(self): - """ - Returns True if this ObjectTag has a valid tag value. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - return self.taxonomy_id and ( - # Open taxonomies only need a value. - (self.taxonomy.allow_free_text and bool(self._value)) - or - # Closed taxonomies need an associated tag in their taxonomy. - ( - not self.taxonomy.allow_free_text - and bool(self.tag_id) - and self.taxonomy_id == self.tag.taxonomy_id - ) - ) - - def _check_object(self): - """ - Returns True if this ObjectTag has a valid object. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - return self.object_id and self.object_type - - def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: - """ - Returns True if this ObjectTag is valid. - - If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. - If `check_tag` is False, then we skip validating the object tag's tag reference. - If `check_object` is False, then we skip validating the object ID/type. - """ - if check_taxonomy and not self._check_taxonomy(): - return False - - if check_tag and not self._check_tag(): - return False - - if check_object and not self._check_object(): - return False - - return True - - def resync(self) -> bool: + def is_valid(self) -> bool: """ - Reconciles the stored ObjectTag properties with any changes made to its associated taxonomy or tag. - - This method is useful to propagate changes to a Taxonomy name or Tag value. - - It's also useful for a set of ObjectTags are imported from an external source prior to when a Taxonomy exists to - validate or store its available Tags. - - Subclasses may override this method to perform any additional syncing for the particular type of object tag. - - Returns True if anything was changed, False otherwise. + Returns True if this ObjectTag is valid for its associated taxonomy. """ - changed = False - - # Locate an enabled taxonomy matching _name, and maybe a tag matching _value - if not self.taxonomy_id: - # Use the linked tag's taxonomy if there is one. - if self.tag_id: - self.taxonomy_id = self.tag.taxonomy_id - changed = True - else: - for taxonomy in Taxonomy.objects.filter( - name=self.name, enabled=True - ).order_by("allow_free_text", "id"): - # Closed taxonomies require a tag matching _value, - # and we'd rather match a closed taxonomy than an open one. - # So see if there's a matching tag available in this taxonomy. - tag = taxonomy.tag_set.filter(value=self.value).first() - - # Make sure this taxonomy will accept object tags like this. - object_tag = self.cast_object_tag() - object_tag.taxonomy = taxonomy - object_tag.tag = tag - if object_tag.is_valid(): - self.taxonomy = taxonomy - self.tag = tag - changed = True - break - # If not, try the next one - - # Sync the stored _name with the taxonomy.name - if self.taxonomy_id and self._name != self.taxonomy.name: - self.name = self.taxonomy.name - changed = True - - # Closed taxonomies require a tag matching _value - if self.taxonomy and not self.taxonomy.allow_free_text and not self.tag_id: - tag = self.taxonomy.tag_set.filter(value=self.value).first() - if tag: - self.tag = tag - changed = True - - # Sync the stored _value with the tag.name - elif self.tag and self._value != self.tag.value: - self.value = self.tag.value - changed = True - - return changed + return self.taxonomy_id and self.taxonomy.validate_object_tag(self) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 94ab96ac..5d4ec87a 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -3,7 +3,7 @@ from django.test.testcases import TestCase import openedx_tagging.core.tagging.api as tagging_api -from openedx_tagging.core.tagging.models import ObjectTag, Tag +from openedx_tagging.core.tagging.models import ObjectTag from .test_models import TestTagTaxonomyMixin @@ -28,6 +28,14 @@ def test_create_taxonomy(self): assert not taxonomy.system_defined assert taxonomy.visible_to_authors + def test_bad_taxonomy_class(self): + with self.assertRaises(ValueError) as exc: + tagging_api.create_taxonomy( + name="Bad class", + taxonomy_class=str, + ) + assert " must be a class like Taxonomy" in str(exc.exception) + def test_get_taxonomy(self): tax1 = tagging_api.get_taxonomy(1) assert tax1 == self.taxonomy @@ -71,105 +79,6 @@ def check_object_tag(self, object_tag, taxonomy, tag, name, value): assert object_tag.name == name assert object_tag.value == value - def test_resync_object_tags(self): - missing_links = ObjectTag.objects.create( - object_id="abc", - object_type="alpha", - _name=self.taxonomy.name, - _value=self.mammalia.value, - ) - changed_links = ObjectTag.objects.create( - object_id="def", - object_type="alpha", - taxonomy=self.taxonomy, - tag=self.mammalia, - ) - changed_links.name = "Life" - changed_links.value = "Animals" - changed_links.save() - no_changes = ObjectTag.objects.create( - object_id="ghi", - object_type="beta", - taxonomy=self.taxonomy, - tag=self.mammalia, - ) - no_changes.name = self.taxonomy.name - no_changes.value = self.mammalia.value - no_changes.save() - - changed = tagging_api.resync_object_tags() - assert changed == 2 - for object_tag in (missing_links, changed_links, no_changes): - self.check_object_tag( - object_tag, self.taxonomy, self.mammalia, "Life on Earth", "Mammalia" - ) - - # Once all tags are resynced, they stay that way - changed = tagging_api.resync_object_tags() - assert changed == 0 - - # ObjectTag value preserved even if linked tag is deleted - self.mammalia.delete() - for object_tag in (missing_links, changed_links, no_changes): - self.check_object_tag( - object_tag, self.taxonomy, None, "Life on Earth", "Mammalia" - ) - # Recreating the tag to test resyncing works - new_mammalia = Tag.objects.create( - value="Mammalia", - taxonomy=self.taxonomy, - ) - changed = tagging_api.resync_object_tags() - assert changed == 3 - for object_tag in (missing_links, changed_links, no_changes): - self.check_object_tag( - object_tag, self.taxonomy, new_mammalia, "Life on Earth", "Mammalia" - ) - - # ObjectTag name preserved even if linked taxonomy and its tags are deleted - self.taxonomy.delete() - for object_tag in (missing_links, changed_links, no_changes): - self.check_object_tag(object_tag, None, None, "Life on Earth", "Mammalia") - - # Resyncing the tags for code coverage - changed = tagging_api.resync_object_tags() - assert changed == 0 - - # Recreate the taxonomy and resync some tags - first_taxonomy = tagging_api.create_taxonomy( - "Life on Earth", allow_free_text=True - ) - second_taxonomy = tagging_api.create_taxonomy("Life on Earth") - new_tag = Tag.objects.create( - value="Mammalia", - taxonomy=second_taxonomy, - ) - - # Ensure the resync prefers the closed taxonomy with the matching tag - changed = tagging_api.resync_object_tags( - ObjectTag.objects.filter(object_type="alpha") - ) - assert changed == 2 - - for object_tag in (missing_links, changed_links): - self.check_object_tag( - object_tag, second_taxonomy, new_tag, "Life on Earth", "Mammalia" - ) - - # Ensure the omitted tag was not updated - self.check_object_tag(no_changes, None, None, "Life on Earth", "Mammalia") - - # Update that one too, to demonstrate the free-text tags are ok - no_changes.value = "Anamelia" - no_changes.save() - changed = tagging_api.resync_object_tags( - ObjectTag.objects.filter(object_type="beta") - ) - assert changed == 1 - self.check_object_tag( - no_changes, first_taxonomy, None, "Life on Earth", "Anamelia" - ) - def test_tag_object(self): self.taxonomy.allow_multiple = True self.taxonomy.save() @@ -195,7 +104,6 @@ def test_tag_object(self): self.taxonomy, tag_list, "biology101", - "course", ) # Ensure the expected number of tags exist in the database @@ -204,7 +112,6 @@ def test_tag_object(self): tagging_api.get_object_tags( taxonomy=self.taxonomy, object_id="biology101", - object_type="course", ) ) == object_tags @@ -217,7 +124,6 @@ def test_tag_object(self): assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" - assert object_tag.object_type == "course" def test_tag_object_free_text(self): self.taxonomy.allow_free_text = True @@ -226,7 +132,6 @@ def test_tag_object_free_text(self): self.taxonomy, ["Eukaryota Xenomorph"], "biology101", - "course", ) assert len(object_tags) == 1 object_tag = object_tags[0] @@ -236,7 +141,6 @@ def test_tag_object_free_text(self): assert object_tag.tag_ref == "Eukaryota Xenomorph" assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] assert object_tag.object_id == "biology101" - assert object_tag.object_type == "course" def test_tag_object_no_multiple(self): with self.assertRaises(ValueError) as exc: @@ -244,7 +148,6 @@ def test_tag_object_no_multiple(self): self.taxonomy, ["A", "B"], "biology101", - "course", ) assert "only allows one tag per object" in str(exc.exception) @@ -256,32 +159,31 @@ def test_tag_object_required(self): self.taxonomy, [], "biology101", - "course", ) assert "requires at least one tag per object" in str(exc.exception) def test_tag_object_invalid_tag(self): - object_tag = tagging_api.tag_object( - self.taxonomy, - ["Eukaryota Xenomorph"], - "biology101", - "course", - )[0] - assert type(object_tag) == ObjectTag # pylint: disable=unidiomatic-typecheck + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + self.taxonomy, + ["Eukaryota Xenomorph"], + "biology101", + ) + assert "Invalid object tag for taxonomy (1): Eukaryota Xenomorph" in str( + exc.exception + ) def test_get_object_tags(self): # Alpha tag has no taxonomy - alpha = ObjectTag(object_id="abc", object_type="alpha") + alpha = ObjectTag(object_id="abc") alpha.name = self.taxonomy.name alpha.value = self.mammalia.value alpha.save() # Beta tag has a closed taxonomy beta = ObjectTag.objects.create( object_id="abc", - object_type="beta", taxonomy=self.taxonomy, ) - beta = tagging_api.cast_object_tag(beta) # Fetch all the tags for a given object ID assert list( @@ -312,17 +214,6 @@ def test_get_object_tags(self): beta, ] - # Fetch all the tags for alpha-type objects - assert list( - tagging_api.get_object_tags( - object_id="abc", - object_type="alpha", - valid_only=False, - ) - ) == [ - alpha, - ] - # Fetch all the tags for a given object ID + taxonomy assert list( tagging_api.get_object_tags( @@ -333,32 +224,3 @@ def test_get_object_tags(self): ) == [ beta, ] - - def test_bad_object_tag_class(self): - with self.assertRaises(ValueError) as exc: - tagging_api.tag_object( - taxonomy=self.taxonomy, - tags=[self.bacteria.id], - object_id="anthropology101", - object_type="course", - object_tag_class=str, - ) - assert "object_tag_class must be class like ObjectTag" in str( - exc.exception - ) - - def test_cast_object_tag(self): - # Create a valid ObjectTag in a closed taxonomy - assert not self.taxonomy.allow_free_text - object_tag = ObjectTag.objects.create( - object_id="object:id:1", - object_type="life", - taxonomy=self.taxonomy, - tag=self.bacteria, - ) - object_tag = tagging_api.cast_object_tag(object_tag) - assert ( - str(object_tag) - == repr(object_tag) - == " object:id:1 (life): Life on Earth=Bacteria" - ) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 5f088c91..6d5c9703 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -78,6 +78,39 @@ def setup_tag_depths(self): tag.depth = 2 +class TestTaxonomySubclassA(Taxonomy): + """ + Model A for testing the taxonomy subclass casting. + """ + + class Meta: + managed = False + proxy = True + app_label = "oel_tagging" + + +class TestTaxonomySubclassB(TestTaxonomySubclassA): + """ + Model B for testing the taxonomy subclass casting. + """ + + class Meta: + managed = False + proxy = True + app_label = "oel_tagging" + + +class TestObjectTagSubclass(ObjectTag): + """ + Model for testing the ObjectTag copy. + """ + + class Meta: + managed = False + proxy = True + app_label = "oel_tagging" + + @ddt.ddt class TestModelTagTaxonomy(TestTagTaxonomyMixin, TestCase): """ @@ -99,6 +132,45 @@ def test_representations(self): ) assert str(self.bacteria) == repr(self.bacteria) == " (1) Bacteria" + def test_taxonomy_cast(self): + for subclass in ( + TestTaxonomySubclassA, + # Ensure that casting to a sub-subclass works as expected + TestTaxonomySubclassB, + # and that we can un-set the subclass + None, + ): + self.taxonomy.taxonomy_class = subclass + cast_taxonomy = self.taxonomy.cast() + if subclass: + expected_class = subclass.__name__ + else: + expected_class = "Taxonomy" + assert self.taxonomy == cast_taxonomy + assert ( + str(cast_taxonomy) + == repr(cast_taxonomy) + == f"<{expected_class}> (1) Life on Earth" + ) + + def test_taxonomy_cast_import_error(self): + taxonomy = Taxonomy.objects.create( + name="Invalid cast", _taxonomy_class="not.a.class" + ) + # Error is logged, but ignored. + cast_taxonomy = taxonomy.cast() + assert cast_taxonomy == taxonomy + assert ( + str(cast_taxonomy) + == repr(cast_taxonomy) + == f" ({taxonomy.id}) Invalid cast" + ) + + def test_taxonomy_cast_bad_value(self): + with self.assertRaises(ValueError) as exc: + self.taxonomy.taxonomy_class = str + assert " must be a class like Taxonomy" in str(exc.exception) + @ddt.data( # Root tags just return their own value ("bacteria", ["Bacteria"]), @@ -147,11 +219,25 @@ def setUp(self): self.tag = self.bacteria self.object_tag = ObjectTag.objects.create( object_id="object:id:1", - object_type="life", taxonomy=self.taxonomy, tag=self.tag, ) + def test_representations(self): + assert ( + str(self.object_tag) + == repr(self.object_tag) + == " object:id:1: Life on Earth=Bacteria" + ) + + def test_cast(self): + copy_tag = TestObjectTagSubclass.cast(self.object_tag) + assert ( + str(copy_tag) + == repr(copy_tag) + == " object:id:1: Life on Earth=Bacteria" + ) + def test_object_tag_name(self): # ObjectTag's name defaults to its taxonomy's name assert self.object_tag.name == self.taxonomy.name @@ -171,7 +257,6 @@ def test_object_tag_value(self): # ObjectTag's value defaults to its tag's value object_tag = ObjectTag.objects.create( object_id="object:id", - object_type="any_old_object", taxonomy=self.taxonomy, tag=self.tag, ) @@ -192,7 +277,6 @@ def test_object_tag_lineage(self): # ObjectTag's value defaults to its tag's lineage object_tag = ObjectTag.objects.create( object_id="object:id", - object_type="any_old_object", taxonomy=self.taxonomy, tag=self.tag, ) @@ -215,46 +299,21 @@ def test_object_tag_is_valid(self): allow_free_text=True, ) - # ObjectTags in a free-text taxonomy are valid with a value object_tag = ObjectTag( taxonomy=self.taxonomy, ) - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=True, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) - object_tag.object_id = "object:id" - object_tag.object_type = "life" - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) + # ObjectTag will only be valid for its taxonomy + assert not open_taxonomy.validate_object_tag(object_tag) + + # ObjectTags in a free-text taxonomy are valid with a value + assert not object_tag.is_valid() object_tag.value = "Any text we want" object_tag.taxonomy = open_taxonomy + assert not object_tag.is_valid() + object_tag.object_id = "object:id" assert object_tag.is_valid() # ObjectTags in a closed taxonomy require a tag in that taxonomy - object_tag = ObjectTag( - taxonomy=open_taxonomy, - ) - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=True, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) - object_tag.object_id = "object:id" - object_tag.object_type = "life" - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) object_tag.taxonomy = self.taxonomy object_tag.tag = Tag.objects.create( taxonomy=self.system_taxonomy, diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index b965bedc..577c594f 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -37,7 +37,6 @@ def setUp(self): taxonomy=self.taxonomy, tag=self.bacteria, ) - self.object_tag.resync() self.object_tag.save() # Taxonomy