From 4c203a4a8d845361719580e3a48e5d6e5493594f Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 28 Sep 2023 18:06:14 -0700 Subject: [PATCH 01/13] feat: Remove object_tag_class from Taxonomy --- openedx_tagging/core/tagging/api.py | 112 +++++++++++++-- openedx_tagging/core/tagging/models/base.py | 149 +------------------- 2 files changed, 100 insertions(+), 161 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index ca2cb1f1..411c30d3 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -14,7 +14,8 @@ from typing import Iterator -from django.db.models import QuerySet +from django.db import transaction +from django.db.models import QuerySet, F from django.utils.translation import gettext_lazy as _ from .models import ObjectTag, Tag, Taxonomy @@ -139,21 +140,16 @@ def resync_object_tags(object_tags: QuerySet | None = None) -> int: def get_object_tags( - object_id: str, taxonomy_id: str | None = None + object_id: str, + ObjectTagClass: type[ObjectTag] = ObjectTag ) -> QuerySet[ObjectTag]: """ Returns a Queryset of object tags for a given object. Pass taxonomy to limit the returned object_tags to a specific taxonomy. """ - ObjectTagClass = ObjectTag - extra_filters = {} - if taxonomy_id is not None: - taxonomy = Taxonomy.objects.get(pk=taxonomy_id) - ObjectTagClass = taxonomy.object_tag_class - extra_filters["taxonomy_id"] = taxonomy_id tags = ( - ObjectTagClass.objects.filter(object_id=object_id, **extra_filters) + ObjectTagClass.objects.filter(object_id=object_id) .select_related("tag", "taxonomy") .order_by("id") ) @@ -164,9 +160,7 @@ def delete_object_tags(object_id: str): """ Delete all ObjectTag entries for a given object. """ - tags = ObjectTag.objects.filter( - object_id=object_id, - ) + tags = ObjectTag.objects.filter(object_id=object_id) tags.delete() @@ -175,6 +169,7 @@ def tag_object( taxonomy: Taxonomy, tags: list[str], object_id: str, + ObjectTagClass: type[ObjectTag] = ObjectTag, ) -> list[ObjectTag]: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags. @@ -185,9 +180,77 @@ 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. """ - return taxonomy.cast().tag_object(tags, object_id) + def _check_new_tag_count(new_tag_count: int) -> None: + """ + Checks if the new count of tags for the object is equal or less than 100 + """ + # Exclude self.id to avoid counting the tags that are going to be updated + current_count = ObjectTag.objects.filter(object_id=object_id).exclude(taxonomy_id=taxonomy.id).count() + + if current_count + new_tag_count > 100: + raise ValueError( + _(f"Cannot add more than 100 tags to ({object_id}).") + ) + + if not isinstance(tags, list): + raise ValueError(_(f"Tags must be a list, not {type(tags).__name__}.")) + + taxonomy = taxonomy.cast() # Make sure we're using the right subclass. This is a no-op if we are already. + tags = list(dict.fromkeys(tags)) # Remove duplicates preserving order + + _check_new_tag_count(len(tags)) + + if not taxonomy.allow_multiple and len(tags) > 1: + raise ValueError(_(f"Taxonomy ({taxonomy.name}) 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 = list( + ObjectTagClass.objects.filter(taxonomy=taxonomy, object_id=object_id) + ) + updated_tags = [] + if taxonomy.allow_free_text: + for tag_value in tags: + object_tag_index = next((i for (i, t) in enumerate(current_tags) if t._value == tag_value), -1) + if object_tag_index >= 0: + # This tag is already applied. + object_tag = current_tags.pop(object_tag_index) + else: + object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, _value=tag_value) + updated_tags.append(object_tag) + else: + # Handle closed taxonomies: + for tag_ref in tags: + tag = taxonomy.tag_set.get(pk=tag_ref) # TODO: this should be taxonomy.tag_for_value(tag_value) + object_tag_index = next((i for (i, t) in enumerate(current_tags) if t.tag_id == tag.id), -1) + if object_tag_index >= 0: + # This tag is already applied. + object_tag = current_tags.pop(object_tag_index) + if object_tag._value != tag.value: + # The ObjectTag's cached '_value' is out of sync with the Tag, so update it: + object_tag._value = tag.value + updated_tags.append(object_tag) + else: + # We are newly applying this tag: + object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, tag=tag, _value=tag.value) + updated_tags.append(object_tag) + + # Save all updated tags at once to avoid partial updates + with transaction.atomic(): + for object_tag in updated_tags: + object_tag.save() + # ...and delete any omitted existing tags + for old_tag in current_tags: + old_tag.delete() + + return updated_tags + +# TODO: return tags from closed taxonomies as well as the count of how many times each is used. def autocomplete_tags( taxonomy: Taxonomy, search: str, @@ -228,4 +291,25 @@ def autocomplete_tags( "using get_tags() and filtering them on the frontend." ) ) - return taxonomy.cast().autocomplete_tags(search, object_id) + # Fetch tags that the object already has to exclude them from the result + excluded_tags: list[str] = [] + if object_id: + excluded_tags = list( + taxonomy.objecttag_set.filter(object_id=object_id).values_list( + "_value", flat=True + ) + ) + return ( + # Fetch object tags from this taxonomy whose value contains the search + taxonomy.objecttag_set.filter(_value__icontains=search) + # omit any tags whose values match the tags on the given object + .exclude(_value__in=excluded_tags) + # alphabetical ordering + .order_by("_value") + # Alias the `_value` field to `value` to make it nicer for users + .annotate(value=F("_value")) + # obtain tag values + .values("value", "tag_id") + # remove repeats + .distinct() + ) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 58a7c950..3a224d6d 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -187,15 +187,6 @@ def __str__(self): ) return f"<{self.__class__.__name__}> ({self.id}) {self.name}" - @property - def object_tag_class(self) -> type[ObjectTag]: - """ - Returns the ObjectTag subclass associated with this taxonomy, which is ObjectTag by default. - - Taxonomy subclasses may override this method to use different subclasses of ObjectTag. - """ - return ObjectTag - @property def taxonomy_class(self) -> type[Taxonomy] | None: """ @@ -350,6 +341,7 @@ def get_filtered_tags( return tag_set.order_by("value", "id") + # TODO: replace this with just validate_value() and value_for_external_id() def validate_object_tag( self, object_tag: "ObjectTag", @@ -419,144 +411,6 @@ def _check_object( """ return bool(object_tag.object_id) - def tag_object( - self, - tags: list[str], - 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 either a list of existing Tag Values or 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. - """ - - def _find_object_tag_index(tag_ref, object_tags) -> int: - """ - Search for Tag in the given list of ObjectTags by tag_ref or value, - returning its index or -1 if not found. - """ - return next( - ( - i - for i, object_tag in enumerate(object_tags) - if object_tag.tag_ref == tag_ref or object_tag.value == tag_ref - ), - -1, - ) - - def _check_new_tag_count(new_tag_count: int) -> None: - """ - Checks if the new count of tags for the object is equal or less than 100 - """ - # Exclude self.id to avoid counting the tags that are going to be updated - current_count = ObjectTag.objects.filter(object_id=object_id).exclude(taxonomy_id=self.id).count() - - if current_count + new_tag_count > 100: - raise ValueError( - _(f"Cannot add more than 100 tags to ({object_id}).") - ) - - if not isinstance(tags, list): - raise ValueError(_(f"Tags must be a list, not {type(tags).__name__}.")) - - tags = list(dict.fromkeys(tags)) # Remove duplicates preserving order - - _check_new_tag_count(len(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.") - ) - - ObjectTagClass = self.object_tag_class - current_tags = list( - ObjectTagClass.objects.filter( - taxonomy=self, - object_id=object_id, - ) - ) - updated_tags = [] - for tag_ref in tags: - object_tag_index = _find_object_tag_index(tag_ref, current_tags) - if object_tag_index >= 0: - object_tag = current_tags.pop(object_tag_index) - else: - object_tag = ObjectTagClass( - taxonomy=self, - object_id=object_id, - ) - - object_tag.tag_ref = tag_ref - object_tag.resync() - 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: - old_tag.delete() - - return updated_tags - - def autocomplete_tags( - self, - search: str, - object_id: str | None = None, - ) -> models.QuerySet: - """ - Provides auto-complete suggestions by matching the `search` string against existing - ObjectTags linked to the given taxonomy. A case-insensitive search is used in order - to return the highest number of relevant tags. - - If `object_id` is provided, then object tag values already linked to this object - are omitted from the returned suggestions. (ObjectTag values must be unique for a - given object + taxonomy, and so omitting these suggestions helps users avoid - duplication errors.). - - Returns a QuerySet of dictionaries containing distinct `value` (string) and `tag` - (numeric ID) values, sorted alphabetically by `value`. - - Subclasses can override this method to perform their own autocomplete process. - Subclass use cases: - * Large taxonomy associated with a model. It can be overridden to get - the suggestions directly from the model by doing own filtering. - * Taxonomy with a list of available tags: It can be overridden to only - search the suggestions on a list of available tags. - """ - # Fetch tags that the object already has to exclude them from the result - excluded_tags: list[str] = [] - if object_id: - excluded_tags = list( - self.objecttag_set.filter(object_id=object_id).values_list( - "_value", flat=True - ) - ) - return ( - # Fetch object tags from this taxonomy whose value contains the search - self.objecttag_set.filter(_value__icontains=search) - # omit any tags whose values match the tags on the given object - .exclude(_value__in=excluded_tags) - # alphabetical ordering - .order_by("_value") - # Alias the `_value` field to `value` to make it nicer for users - .annotate(value=models.F("_value")) - # obtain tag values - .values("value", "tag_id") - # remove repeats - .distinct() - ) - class ObjectTag(models.Model): """ @@ -683,6 +537,7 @@ def tag_ref(self) -> str: """ return self.tag.id if self.tag else self._value + # TODO: remove tag_ref, always reference tags by 'value' @tag_ref.setter def tag_ref(self, tag_ref: str): """ From 1e11da92a07e25aef3ac8dfb737dd660b2c06494 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 28 Sep 2023 18:33:53 -0700 Subject: [PATCH 02/13] feat: set tags by value, not tag_ref. Remove ObjectTag from Taxonomy. --- openedx_tagging/core/tagging/api.py | 22 +- .../core/tagging/models/__init__.py | 2 +- openedx_tagging/core/tagging/models/base.py | 134 +++------ .../core/tagging/models/system_defined.py | 283 ++++++++---------- .../core/tagging/rest_api/v1/serializers.py | 1 - 5 files changed, 193 insertions(+), 249 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 411c30d3..9cea8020 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -20,6 +20,9 @@ from .models import ObjectTag, Tag, Taxonomy +# Export this as part of the API +TagDoesNotExist = Tag.DoesNotExist + def create_taxonomy( name: str, @@ -165,6 +168,7 @@ def delete_object_tags(object_id: str): tags.delete() +# TODO: a function called "tag_object" should take "object_id" as its first parameter, not taxonomy def tag_object( taxonomy: Taxonomy, tags: list[str], @@ -172,13 +176,17 @@ def tag_object( ObjectTagClass: type[ObjectTag] = ObjectTag, ) -> list[ObjectTag]: """ - Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags. + Replaces the existing ObjectTag entries for the given taxonomy + object_id + with the given list of tags. + + tags: A list of the values of the tags from this taxonomy to apply. - 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. + ObjectTagClass: Optional. Use a proxy subclass of ObjectTag for additional + validation. (e.g. only allow tagging certain types of objects.) - 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. + 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. """ def _check_new_tag_count(new_tag_count: int) -> None: @@ -224,8 +232,8 @@ def _check_new_tag_count(new_tag_count: int) -> None: updated_tags.append(object_tag) else: # Handle closed taxonomies: - for tag_ref in tags: - tag = taxonomy.tag_set.get(pk=tag_ref) # TODO: this should be taxonomy.tag_for_value(tag_value) + for tag_value in tags: + tag = taxonomy.tag_for_value(tag_value) # Will raise Tag.DoesNotExist if the value is invalid. object_tag_index = next((i for (i, t) in enumerate(current_tags) if t.tag_id == tag.id), -1) if object_tag_index >= 0: # This tag is already applied. diff --git a/openedx_tagging/core/tagging/models/__init__.py b/openedx_tagging/core/tagging/models/__init__.py index 226d9991..d3d8ab1f 100644 --- a/openedx_tagging/core/tagging/models/__init__.py +++ b/openedx_tagging/core/tagging/models/__init__.py @@ -1,3 +1,3 @@ from .base import ObjectTag, Tag, Taxonomy from .import_export import TagImportTask, TagImportTaskState -from .system_defined import LanguageTaxonomy, ModelObjectTag, ModelSystemDefinedTaxonomy, UserSystemDefinedTaxonomy +from .system_defined import LanguageTaxonomy, ModelSystemDefinedTaxonomy, UserSystemDefinedTaxonomy diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 3a224d6d..045a715c 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -137,7 +137,7 @@ class Taxonomy(models.Model): ), ) allow_multiple = models.BooleanField( - default=False, + default=False, # TODO: This should be true, or perhaps remove this property altogether help_text=_( "Indicates that multiple tags from this taxonomy may be added to an object." ), @@ -243,6 +243,13 @@ def cast(self): ) return self + + def check_casted(self): + """ + Double-check that this taxonomy has been cast() to a subclass if needed. + """ + if self.cast() is not self: + raise TypeError("Taxonomy was used incorrectly - without .cast()") def copy(self, taxonomy: Taxonomy) -> Taxonomy: """ @@ -341,75 +348,54 @@ def get_filtered_tags( return tag_set.order_by("value", "id") - # TODO: replace this with just validate_value() and value_for_external_id() - def validate_object_tag( - self, - object_tag: "ObjectTag", - check_taxonomy=True, - check_tag=True, - check_object=True, - ) -> bool: + def validate_value(self, value: str) -> 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. + Check if 'value' is part of this Taxonomy. + A 'Tag' object may not exist for the value (e.g. if this is a free text + taxonomy, then any value is allowed but no Tags are created; if this is + a user taxonomy, Tag entries may only get created as needed.), but if + this returns True then the value conceptually exists in this taxonomy + and can be used to tag objects. """ - 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 + self.check_casted() + if self.allow_free_text: + return True + return self.tag_set.filter(value__iexact=value).exists() - def _check_taxonomy( - self, - object_tag: ObjectTag, - ) -> bool: + def tag_for_value(self, value: str) -> Tag: """ - Returns True if the given object tag is valid for the current Taxonomy. + Get the Tag object for the given value. + Some Taxonomies may auto-create the Tag at this point, e.g. a User + Taxonomy will create User Tags "just in time". - Subclasses can override this method to perform their own taxonomy validation checks. + Will raise Tag.DoesNotExist if the value is not valid for this taxonomy. """ - # Must be linked to this taxonomy - return ( - object_tag.taxonomy_id is not None - ) and object_tag.taxonomy_id == self.id + self.check_casted() + if self.allow_free_text: + raise ValueError("tag_for_value() doesn't work for free text taxonomies. They don't use Tag instances.") + return self.tag_set.get(value__iexact=value) - def _check_tag( - self, - object_tag: ObjectTag, - ) -> bool: + def validate_external_id(self, external_id: str) -> 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. + Check if 'external_id' is part of this Taxonomy. """ - # Open taxonomies only need a value. + self.check_casted() if self.allow_free_text: - return bool(object_tag.value) + return False # Free text taxonomies don't use 'external_id' on their tags + return self.tag_set.filter(external_id__iexact=external_id).exists() - # Closed taxonomies need an associated tag in this taxonomy - return (object_tag.tag is not None) and object_tag.tag.taxonomy_id == self.id - - def _check_object( - self, - object_tag: ObjectTag, - ) -> bool: + def tag_for_external_id(self, external_id: str) -> Tag: """ - Returns True if the given object tag's object is valid for the current Taxonomy. + Get the Tag object for the given external_id. + Some Taxonomies may auto-create the Tag at this point, e.g. a User + Taxonomy will create User Tags "just in time". - Subclasses can override this method to perform their own taxonomy validation checks. + Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. """ - return bool(object_tag.object_id) + self.check_casted() + if self.allow_free_text: + raise ValueError("tag_for_external_id() doesn't work for free text taxonomies.") + return self.tag_set.get(external_id__iexact=external_id) class ObjectTag(models.Model): @@ -527,42 +513,18 @@ def value(self, value: str): """ self._value = value - @property - def tag_ref(self) -> str: - """ - Returns this tag's reference string. - - If tag is set, then returns its id. - Otherwise, returns the cached _value field. - """ - return self.tag.id if self.tag else self._value - - # TODO: remove tag_ref, always reference tags by 'value' - @tag_ref.setter - def tag_ref(self, tag_ref: str): - """ - Sets the ObjectTag's Tag and/or value, depending on whether a valid Tag is found. - - Subclasses may override this method to dynamically create Tags. - """ - self.value = tag_ref - - if self.taxonomy: - try: - self.tag = self.taxonomy.tag_set.get(pk=tag_ref) - self.value = self.tag.value - except (ValueError, Tag.DoesNotExist): - # This might be ok, e.g. if our taxonomy.allow_free_text, so we just pass through here. - # We rely on the caller to validate before saving. - pass - def is_valid(self) -> bool: """ Returns True if this ObjectTag represents a valid taxonomy tag. A valid ObjectTag must be linked to a Taxonomy, and be a valid tag in that taxonomy. """ - return self.taxonomy.validate_object_tag(self) if self.taxonomy else False + if self.tag: + return True # If the Tag still exists, this is a valid value in the taxonomy. + elif self.taxonomy: + # If this is not a free text value and it's no longer linked to a Tag, it's invalid. + # Running self.resync() _might_ fix this though. + return self.taxonomy.allow_free_text def get_lineage(self) -> Lineage: """ diff --git a/openedx_tagging/core/tagging/models/system_defined.py b/openedx_tagging/core/tagging/models/system_defined.py index c78e9aa8..c431e1c0 100644 --- a/openedx_tagging/core/tagging/models/system_defined.py +++ b/openedx_tagging/core/tagging/models/system_defined.py @@ -4,15 +4,15 @@ from __future__ import annotations import logging -from typing import Any from django.conf import settings from django.contrib.auth import get_user_model +from django.core.exceptions import ObjectDoesNotExist from django.db import models -from openedx_tagging.core.tagging.models.base import ObjectTag, Tag +from openedx_tagging.core.tagging.models.base import Tag -from .base import ObjectTag, Tag, Taxonomy +from .base import Tag, Taxonomy log = logging.getLogger(__name__) @@ -34,113 +34,16 @@ def system_defined(self) -> bool: return True -class ModelObjectTag(ObjectTag): - """ - Model-based ObjectTag, abstract class. - - Used by ModelSystemDefinedTaxonomy to maintain dynamic Tags which are associated with a configured Model instance. - """ - - class Meta: - proxy = True - - def __init__(self, *args: Any, **kwargs: Any) -> None: - """ - Checks if the `tag_class_model` is correct - """ - assert issubclass(self.tag_class_model, models.Model) - super().__init__(*args, **kwargs) - - @property - def tag_class_model(self) -> type[models.Model]: - """ - Subclasses must implement this method to return the Django.model - class referenced by these object tags. - """ - raise NotImplementedError - - @property - def tag_class_value(self) -> str: - """ - Returns the name of the tag_class_model field to use as the Tag.value when creating Tags for this taxonomy. - - Subclasses may override this method to use different fields. - """ - return "pk" - - def get_instance(self) -> models.Model | None: - """ - Returns the instance of tag_class_model associated with this object tag, or None if not found. - """ - instance_id = self.tag.external_id if self.tag else None - if instance_id: - try: - return self.tag_class_model.objects.get(pk=instance_id) - except ValueError as e: - log.exception(f"{self}: {str(e)}") - except self.tag_class_model.DoesNotExist: - log.exception( - f"{self}: {self.tag_class_model.__name__} pk={instance_id} does not exist." - ) - - return None - - def _resync_tag(self) -> bool: - """ - Resync our tag's value with the value from the instance. - - If the instance associated with the tag no longer exists, we unset our tag, because it's no longer valid. - - Returns True if the given tag was changed, False otherwise. - """ - instance = self.get_instance() - if instance: - value = getattr(instance, self.tag_class_value) - self.value = value - if self.tag and self.tag.value != value: - self.tag.value = value - self.tag.save() - return True - else: - self.tag = None - - return False - - @property - def tag_ref(self) -> str: - return (self.tag.external_id or self.tag.id) if self.tag else self._value - - @tag_ref.setter - def tag_ref(self, tag_ref: str): - """ - Sets the ObjectTag's Tag and/or value, depending on whether a valid Tag is found, or can be created. - - Creates a Tag for the given tag_ref value, if one containing that external_id not already exist. - """ - self.value = tag_ref - - if self.taxonomy: - try: - self.tag = self.taxonomy.tag_set.get( - external_id=tag_ref, - ) - except (ValueError, Tag.DoesNotExist): - # Creates a new Tag for this instance - self.tag = Tag( - taxonomy=self.taxonomy, - external_id=tag_ref, - ) - - self._resync_tag() - - class ModelSystemDefinedTaxonomy(SystemDefinedTaxonomy): """ Model based system taxonomy abstract class. - This type of taxonomy has an associated Django model in ModelObjectTag.tag_class_model(). - They are designed to create Tags when required for new ObjectTags, to maintain - their status as "closed" taxonomies. + This type of taxonomy has an associated Django model in + ModelSystemDefinedTaxonomy.tag_class_model. + + They are designed to create Tags when required for new ObjectTags, to + maintain their status as "closed" taxonomies. + The Tags are representations of the instances of the associated model. Tag.external_id stores an identifier from the instance (`pk` as default) @@ -155,41 +58,99 @@ class ModelSystemDefinedTaxonomy(SystemDefinedTaxonomy): class Meta: proxy = True - def __init__(self, *args: Any, **kwargs: Any) -> None: + @property + def tag_class_model(self) -> type[models.Model]: """ - Checks if the `object_tag_class` is a subclass of ModelObjectTag. + Define what Django model this taxonomy is associated with """ - assert issubclass(self.object_tag_class, ModelObjectTag) - super().__init__(*args, **kwargs) + raise NotImplementedError @property - def object_tag_class(self) -> type[ModelObjectTag]: + def tag_class_value_field(self) -> str: """ - Returns the ModelObjectTag subclass associated with this taxonomy. + The name of the tag_class_model field to use as the Tag.value when creating Tags for this taxonomy. - Model Taxonomy subclasses must implement this to provide a ModelObjectTag subclass. + Subclasses may override this method to use different fields. """ raise NotImplementedError - def _check_instance(self, object_tag: ObjectTag) -> bool: + @property + def tag_class_key_field(self) -> str: """ - Returns True if the instance exists + The name of the tag_class_model field to use as the Tag.external_id when creating Tags for this taxonomy. - Subclasses can override this method to perform their own instance validation checks. + This must be an immutable ID. """ - object_tag = self.object_tag_class.cast(object_tag) - return bool(object_tag.get_instance()) + return "pk" - def _check_tag(self, object_tag: ObjectTag) -> bool: - """ - Returns True if the instance is valid - """ - return super()._check_tag(object_tag) and self._check_instance(object_tag) + def validate_value(self, value: str): + """ + Check if 'value' is part of this Taxonomy, based on the specified model. + """ + try: + self.tag_class_model.objects.get(**{f"{self.tag_class_value_field}__iexact": value}) + return True + except ObjectDoesNotExist: + return False + + def tag_for_value(self, value: str): + """ + Get the Tag object for the given value. + """ + try: + # First we look up the instance by value + instance = self.tag_class_model.objects.get(**{f"{self.tag_class_value_field}__iexact": value}) + except ObjectDoesNotExist: + raise Tag.DoesNotExist + # Use the canonical value from here on (possibly with different case from the value given as a parameter) + value = getattr(instance, self.tag_class_value_field) + # We assume the value may change but the external_id is immutable. + # So look up keys using external_id. There may be a key with the same external_id but an out of date value. + external_id = str(getattr(instance, self.tag_class_key_field)) + tag, _created = self.tag_set.get_or_create(external_id=external_id, defaults={"value": value}) + if tag.value != value: + # Update the Tag to reflect the new cached 'value' + tag.value = value + tag.save() + return tag + + def validate_external_id(self, external_id: str): + """ + Check if 'external_id' is part of this Taxonomy. + """ + try: + self.tag_class_model.objects.get(**{f"{self.tag_class_key_field}__iexact": external_id}) + return True + except ObjectDoesNotExist: + return False + + def tag_for_external_id(self, external_id: str): + """ + Get the Tag object for the given external_id. + Some Taxonomies may auto-create the Tag at this point, e.g. a User + Taxonomy will create User Tags "just in time". + + Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. + """ + try: + # First we look up the instance by value + instance = self.tag_class_model.objects.get(**{f"{self.tag_class_key_field}__iexact": external_id}) + except ObjectDoesNotExist: + raise Tag.DoesNotExist + value = getattr(instance, self.tag_class_value_field) + # Use the canonical external_id from here on (may differ in capitalization) + external_id = getattr(instance, self.tag_class_key_field) + tag, _created = self.tag_set.get_or_create(external_id=external_id, defaults={"value": value}) + if tag.value != value: + # Update the Tag to reflect the new cached 'value' + tag.value = value + tag.save() + return tag -class UserModelObjectTag(ModelObjectTag): +class UserSystemDefinedTaxonomy(ModelSystemDefinedTaxonomy): """ - ObjectTags for the UserSystemDefinedTaxonomy. + A Taxonomy that allows tagging objects using users. """ class Meta: @@ -198,12 +159,12 @@ class Meta: @property def tag_class_model(self) -> type[models.Model]: """ - Associate the user model + Define what Django model this taxonomy is associated with """ return get_user_model() @property - def tag_class_value(self) -> str: + def tag_class_value_field(self) -> str: """ Returns the name of the tag_class_model field to use as the Tag.value when creating Tags for this taxonomy. @@ -212,24 +173,6 @@ def tag_class_value(self) -> str: return "username" -class UserSystemDefinedTaxonomy(ModelSystemDefinedTaxonomy): - """ - User based system taxonomy class. - """ - - class Meta: - proxy = True - - @property - def object_tag_class(self): - """ - Returns the ObjectTag subclass associated with this taxonomy, which is ModelObjectTag by default. - - Model Taxonomy subclasses must implement this to provide a ModelObjectTag subclass. - """ - return UserModelObjectTag - - class LanguageTaxonomy(SystemDefinedTaxonomy): """ Language System-defined taxonomy @@ -288,17 +231,49 @@ def _get_available_languages(cls) -> set[str]: langs.add(django_lang[0].split("-")[0]) return langs - def _check_valid_language(self, object_tag: ObjectTag) -> bool: + def validate_value(self, value: str): """ - Returns True if the tag is on the available languages + Check if 'value' is part of this Taxonomy, based on the specified model. """ - available_langs = self._get_available_languages() - if not object_tag.tag: - raise AttributeError("Expected object_tag.tag to be set") - return object_tag.tag.external_id in available_langs + for _, lang_name in settings.LANGUAGES: + if lang_name == value: + return True + return False + + def tag_for_value(self, value: str): + """ + Get the Tag object for the given value. + """ + for lang_code, lang_name in settings.LANGUAGES: + if lang_name == value: + return self.tag_for_external_id(lang_code) + raise Tag.DoesNotExist - def _check_tag(self, object_tag: ObjectTag) -> bool: + def validate_external_id(self, external_id: str): """ - Returns True if the tag is on the available languages + Check if 'external_id' is part of this Taxonomy. + """ + lang_code = external_id.lower() + LANGUAGE_DICT = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) # setting may or may not exist. + return lang_code in LANGUAGE_DICT + + def tag_for_external_id(self, external_id: str): + """ + Get the Tag object for the given external_id. + Some Taxonomies may auto-create the Tag at this point, e.g. a User + Taxonomy will create User Tags "just in time". + + Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. """ - return super()._check_tag(object_tag) and self._check_valid_language(object_tag) + lang_code = external_id.lower() + LANGUAGE_DICT = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) # setting may or may not exist. + try: + lang_name = LANGUAGE_DICT[lang_code] + except KeyError: + raise Tag.DoesNotExist + tag, _created = self.tag_set.get_or_create(external_id=lang_code, defaults={"value": lang_name}) + if tag.value != lang_name: + # Update the Tag to reflect the new language name + tag.value = lang_name + tag.save() + return tag diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 756d4a79..22071054 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -53,7 +53,6 @@ class Meta: "name", "value", "taxonomy_id", - "tag_ref", "is_valid", ] From 9aeea89b3a7faf5d2f50d325e909872d1d216f95 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 28 Sep 2023 23:17:28 -0700 Subject: [PATCH 03/13] chore: updated tests for system defined taxonomies --- .../tagging/test_system_defined_models.py | 344 ++++++++++-------- 1 file changed, 185 insertions(+), 159 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_system_defined_models.py b/tests/openedx_tagging/core/tagging/test_system_defined_models.py index ff2d669b..3026a844 100644 --- a/tests/openedx_tagging/core/tagging/test_system_defined_models.py +++ b/tests/openedx_tagging/core/tagging/test_system_defined_models.py @@ -2,14 +2,16 @@ Test the tagging system-defined taxonomy models """ from __future__ import annotations +from datetime import datetime import ddt # type: ignore[import] -from django.contrib.auth import get_user_model -from django.db.utils import IntegrityError from django.test import TestCase, override_settings +import pytest +from openedx_learning.core.publishing.models import LearningPackage +from openedx_tagging.core.tagging import api +from openedx_tagging.core.tagging.models import Taxonomy from openedx_tagging.core.tagging.models.system_defined import ( - ModelObjectTag, ModelSystemDefinedTaxonomy, UserSystemDefinedTaxonomy, ) @@ -18,6 +20,7 @@ test_languages = [ ("en", "English"), + ("en-uk", "English (United Kingdom)"), ("az", "Azerbaijani"), ("id", "Indonesian"), ("qu", "Quechua"), @@ -46,29 +49,21 @@ class Meta: app_label = "oel_tagging" -class TestModelTag(ModelObjectTag): +class TestLPTaxonomy(ModelSystemDefinedTaxonomy): """ - Model used for testing + Model used for testing - points to LearningPackage instances """ - @property def tag_class_model(self): - return get_user_model() + return LearningPackage - class Meta: - proxy = True - managed = False - app_label = "oel_tagging" - - -class TestModelTaxonomy(ModelSystemDefinedTaxonomy): - """ - Model used for testing - """ + @property + def tag_class_value_field(self) -> str: + return "key" @property - def object_tag_class(self): - return TestModelTag + def tag_class_key_field(self) -> str: + return "uuid" class Meta: proxy = True @@ -82,122 +77,165 @@ class TestModelSystemDefinedTaxonomy(TestTagTaxonomyMixin, TestCase): Test for Model Model System defined taxonomy """ - @ddt.data( - (ModelSystemDefinedTaxonomy, NotImplementedError), - (ModelObjectTag, NotImplementedError), - (InvalidModelTaxonomy, AssertionError), - (UserSystemDefinedTaxonomy, None), - ) - @ddt.unpack - def test_implementation_error(self, taxonomy_cls, expected_exception): - if not expected_exception: - assert taxonomy_cls() - else: - with self.assertRaises(expected_exception): - taxonomy_cls() - - # FIXME: something is wrong with this test case. It's setting the string - # "tag_id" as the primary key (integer) of the Tag instance, and it mentions - # "parent validation" but there is nothing to do with parents here. - # - # @ddt.data( - # ("1", "tag_id", True), # Valid - # ("0", "tag_id", False), # Invalid user - # ("test_id", "tag_id", False), # Invalid user id - # ("1", None, False), # Testing parent validations - # ) - # @ddt.unpack - # def test_validations(self, tag_external_id: str, tag_id: str | None, expected: bool) -> None: - # tag = Tag( - # id=tag_id, - # taxonomy=self.user_taxonomy, - # value="_val", - # external_id=tag_external_id, - # ) - # object_tag = ObjectTag( - # object_id="id", - # tag=tag, - # ) - # - # assert self.user_taxonomy.validate_object_tag( - # object_tag=object_tag, - # check_object=False, - # check_taxonomy=False, - # check_tag=True, - # ) == expected - - def test_tag_object_invalid_user(self): - # Test user that doesn't exist - with self.assertRaises(ValueError): - self.user_taxonomy.tag_object(tags=[4], object_id="object_id") - - def _tag_object(self): - return self.user_taxonomy.tag_object( - tags=[self.user_1.id], object_id="object_id" + @staticmethod + def _create_learning_pkg(**kwargs) -> LearningPackage: + timestamp = datetime.now() + return LearningPackage.objects.create(**kwargs, created=timestamp, updated=timestamp) + + @classmethod + def setUpClass(cls): + super().setUpClass() + # Create two learning packages and a taxonomy that can tag any object using learning packages as tags: + cls.learning_pkg_1 = cls._create_learning_pkg(key="p1", title="Learning Package 1") + cls.learning_pkg_2 = cls._create_learning_pkg(key="p2", title="Learning Package 2") + cls.lp_taxonomy = TestLPTaxonomy.objects.create( + taxonomy_class=TestLPTaxonomy, + name="LearningPackage Taxonomy", + allow_multiple=True, + ) + # Also create an "Author" taxonomy that can tag any object using user IDs/usernames: + cls.author_taxonomy = UserSystemDefinedTaxonomy.objects.create( + taxonomy_class=UserSystemDefinedTaxonomy, + name="Authors", + allow_multiple=True, ) - def test_tag_object_tag_creation(self): - # Test creation of a new Tag with user taxonomy - assert self.user_taxonomy.tag_set.count() == 0 - updated_tags = self._tag_object() - assert self.user_taxonomy.tag_set.count() == 1 - assert len(updated_tags) == 1 - assert updated_tags[0].tag.external_id == str(self.user_1.id) - assert updated_tags[0].tag.value == self.user_1.get_username() - - # Test parent functions - taxonomy = TestModelTaxonomy( - name="Test", - description="Test", + def test_lp_taxonomy_validation(self): + """ + Test that the validation methods of the Learning Package Taxonomy are working + """ + # Create a new LearningPackage - we know no Tag instances will exist for it yet. + valid_lp = self._create_learning_pkg(key="valid-lp", title="New Learning Packacge") + # The taxonomy can validate tags by value which we've defined as they 'key' of the LearningPackage: + assert self.lp_taxonomy.validate_value(self.learning_pkg_2.key) is True + assert self.lp_taxonomy.validate_value(self.learning_pkg_2.key) is True + assert self.lp_taxonomy.validate_value(valid_lp.key) is True + assert self.lp_taxonomy.validate_value("foo") is False + # The taxonomy can also validate tags by external_id, which we've defined as the UUID of the LearningPackage: + assert self.lp_taxonomy.validate_external_id(self.learning_pkg_2.uuid) is True + assert self.lp_taxonomy.validate_external_id(self.learning_pkg_2.uuid) is True + assert self.lp_taxonomy.validate_external_id(valid_lp.uuid) is True + assert self.lp_taxonomy.validate_external_id("ba11225e-9ec9-4a50-87ea-3155c7c20466") is False + + def test_author_taxonomy_validation(self): + """ + Test the validation methods of the Author Taxonomy (Author = User) + """ + assert self.author_taxonomy.validate_value(self.user_1.username) is True + assert self.author_taxonomy.validate_value(self.user_2.username) is True + assert self.author_taxonomy.validate_value("not a user") is False + # And we can validate by ID if we want: + assert self.author_taxonomy.validate_external_id(str(self.user_1.id)) is True + assert self.author_taxonomy.validate_external_id(str(self.user_2.id)) is True + assert self.author_taxonomy.validate_external_id("8742590") is False + + @ddt.data( + "validate_value", "tag_for_value", "validate_external_id", "tag_for_external_id", + ) + def test_warns_uncasted(self, method): + """ + Test that if we use a taxonomy directly without cast(), we get warned. + """ + base_taxonomy = Taxonomy.objects.get(pk=self.lp_taxonomy.pk) + with pytest.raises(TypeError) as excinfo: + # e.g. base_taxonomy.validate_value("foo") + getattr(base_taxonomy, method)("foo") + assert "Taxonomy was used incorrectly - without .cast()" in str(excinfo.value) + + def test_simple_tag_object(self): + """ + Test applying tags to an object. + """ + object1_id, object2_id = "obj1", "obj2" + api.tag_object(self.lp_taxonomy, ["p1"], object1_id) + api.tag_object(self.lp_taxonomy, ["p1", "p2"], object2_id) + assert [t.value for t in api.get_object_tags(object1_id)] == ["p1"] + assert [t.value for t in api.get_object_tags(object2_id)] == ["p1", "p2"] + + def test_invalid_tag(self): + """ + Trying to apply an invalid tag raises TagDoesNotExist + """ + with pytest.raises(api.TagDoesNotExist): + api.tag_object(self.lp_taxonomy, ["nonexistent"], "obj1") + + def test_case_insensitive_values(self): + """ + For now, values are case insensitive. We may change that in the future. + """ + object1_id, object2_id = "obj1", "obj2" + api.tag_object(self.lp_taxonomy, ["P1"], object1_id) + api.tag_object(self.lp_taxonomy, ["p1", "P2"], object2_id) + assert [t.value for t in api.get_object_tags(object1_id)] == ["p1"] + assert [t.value for t in api.get_object_tags(object2_id)] == ["p1", "p2"] + + def test_multiple_taxonomies(self): + """ + Test using several different instances of a taxonomy to tag the same object + """ + reviewer_taxonomy = UserSystemDefinedTaxonomy.objects.create( + taxonomy_class=UserSystemDefinedTaxonomy, + name="Reviewer", + allow_multiple=True, ) - taxonomy.save() - assert taxonomy.tag_set.count() == 0 - updated_tags = taxonomy.tag_object(tags=[self.user_1.id], object_id="object_id") - assert taxonomy.tag_set.count() == 1 - assert taxonomy.tag_set.count() == 1 - assert len(updated_tags) == 1 - assert updated_tags[0].tag.external_id == str(self.user_1.id) - assert updated_tags[0].tag.value == str(self.user_1.id) - - def test_tag_object_existing_tag(self): - # Test add an existing Tag - self._tag_object() - assert self.user_taxonomy.tag_set.count() == 1 - with self.assertRaises(IntegrityError): - self._tag_object() + pr_1_id, pr_2_id = "pull_request_1", "pull_request_2" + + # Tag PR 1 as having "Author: user1, user2; Reviewer: user2" + api.tag_object(self.author_taxonomy, [self.user_1.username, self.user_2.username], pr_1_id) + api.tag_object(reviewer_taxonomy, [self.user_2.username], pr_1_id) + + # Tag PR 2 as having "Author: user2, reviewer: user1" + api.tag_object(self.author_taxonomy, [self.user_2.username], pr_2_id) + api.tag_object(reviewer_taxonomy, [self.user_1.username], pr_2_id) + + # Check the results: + assert [f"{t.taxonomy.name}:{t.value}" for t in api.get_object_tags(pr_1_id)] == [ + f"Authors:{self.user_1.username}", + f"Authors:{self.user_2.username}", + f"Reviewer:{self.user_2.username}", + ] + assert [f"{t.taxonomy.name}:{t.value}" for t in api.get_object_tags(pr_2_id)] == [ + f"Authors:{self.user_2.username}", + f"Reviewer:{self.user_1.username}", + ] def test_tag_object_resync(self): - self._tag_object() - - self.user_1.username = "new_username" + """ + If the value changes, we can use the new value to tag objects, and the + Tag will be updated automatically. + """ + # Tag two objects with "Author: user_1" + object1_id, object2_id, other_obj_id = "obj1", "obj2", "other" + api.tag_object(self.author_taxonomy, [self.user_1.username], object1_id) + api.tag_object(self.author_taxonomy, [self.user_1.username], object2_id) + initial_object_tags = api.get_object_tags(object1_id) + assert [t.value for t in initial_object_tags] == [self.user_1.username] + assert list(api.get_object_tags(other_obj_id)) == [] + # Change user_1's username: + new_username = "new_username" + self.user_1.username = new_username self.user_1.save() - updated_tags = self._tag_object() - assert self.user_taxonomy.tag_set.count() == 1 - assert len(updated_tags) == 1 - assert updated_tags[0].tag.external_id == str(self.user_1.id) - assert updated_tags[0].tag.value == self.user_1.get_username() + # Now we update the tags on just one of the objects: + api.tag_object(self.author_taxonomy, [new_username], object1_id) + assert [t.value for t in api.get_object_tags(object1_id)] == [new_username] + # But because this will have updated the shared Tag instance, object2 will also be updated as a side effect. + # This is good - all the objects throughout the system with this tag now show the new value. + assert [t.value for t in api.get_object_tags(object2_id)] == [new_username] + # And just to make sure there are no other random changes to other objects: + assert list(api.get_object_tags(other_obj_id)) == [] def test_tag_object_delete_user(self): + """ + Using a deleted model instance as a tag will raise TagDoesNotExist + """ + # Tag an object with "Author: user_1" + object_id = "obj123" + api.tag_object(self.author_taxonomy, [self.user_1.username], object_id) + assert [t.value for t in api.get_object_tags(object_id)] == [self.user_1.username] # Test after delete user - self._tag_object() - user_1_id = self.user_1.id self.user_1.delete() - with self.assertRaises(ValueError): - self.user_taxonomy.tag_object( - tags=[user_1_id], - object_id="object_id", - ) - - def test_tag_ref(self): - object_tag = TestModelTag() - object_tag.tag_ref = 1 - object_tag.save() - assert object_tag.tag is None - assert object_tag.value == 1 - - def test_get_instance(self): - object_tag = TestModelTag() - assert object_tag.get_instance() is None + with self.assertRaises(api.TagDoesNotExist): + api.tag_object(self.author_taxonomy, [self.user_1.username], object_id) @ddt.ddt @@ -207,37 +245,25 @@ class TestLanguageTaxonomy(TestTagTaxonomyMixin, TestCase): Test for Language taxonomy """ - # FIXME: something is wrong with this test case. It's setting the string - # "tag_id" as the primary key (integer) of the Tag instance, and it mentions - # "parent validation" but there is nothing to do with parents here. - # - # @ddt.data( - # ("en", "tag_id", True), # Valid - # ("es", "tag_id", False), # Not available lang - # ("en", None, False), # Test parent validations - # ) - # @ddt.unpack - # def test_validations(self, lang: str, tag_id: str | None, expected: bool): - # tag = Tag( - # id=tag_id, - # taxonomy=self.language_taxonomy, - # value="_val", - # external_id=lang, - # ) - # object_tag = ObjectTag( - # object_id="id", - # tag=tag, - # ) - # assert self.language_taxonomy.validate_object_tag( - # object_tag=object_tag, - # check_object=False, - # check_taxonomy=False, - # check_tag=True, - # ) == expected - - def test_get_tags(self): - tags = self.language_taxonomy.get_tags() - expected_langs = [lang[0] for lang in test_languages] - for tag in tags: - assert tag.external_id in expected_langs - assert tag.annotated_field == 0 + def test_validate_lang_ids(self): + """ + Whether or not languages are available as tags depends on the django settings + """ + assert self.language_taxonomy.validate_external_id("en") is True + assert self.language_taxonomy.tag_for_external_id("en").value == "English" + assert self.language_taxonomy.tag_for_external_id("en-uk").value == "English (United Kingdom)" + assert self.language_taxonomy.tag_for_external_id("id").value == "Indonesian" + + assert self.language_taxonomy.validate_external_id("xx") is False + with pytest.raises(api.TagDoesNotExist): + self.language_taxonomy.tag_for_external_id("xx") + + @override_settings(LANGUAGES=[("fr", "Français")]) + def test_minimal_languages(self): + """ + Whether or not languages are available as tags depends on the django settings + """ + assert self.language_taxonomy.validate_external_id("en") is False + with pytest.raises(api.TagDoesNotExist): + self.language_taxonomy.tag_for_external_id("en") + assert self.language_taxonomy.tag_for_external_id("fr").value == "Français" From bc30c50a19b3a2d7b4804bef882282371179aced Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 29 Sep 2023 11:15:00 -0700 Subject: [PATCH 04/13] feat: more cleanups and validation for ObjectTag --- openedx_tagging/core/tagging/api.py | 14 +- .../core/tagging/migrations/0010_cleanups.py | 31 +++ openedx_tagging/core/tagging/models/base.py | 51 +++-- .../core/tagging/rest_api/v1/serializers.py | 3 +- .../core/tagging/test_models.py | 185 ++++++++++-------- 5 files changed, 183 insertions(+), 101 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0010_cleanups.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 9cea8020..c3ffed4c 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -174,7 +174,7 @@ def tag_object( tags: list[str], object_id: str, ObjectTagClass: type[ObjectTag] = ObjectTag, -) -> list[ObjectTag]: +) -> None: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags. @@ -244,18 +244,18 @@ def _check_new_tag_count(new_tag_count: int) -> None: updated_tags.append(object_tag) else: # We are newly applying this tag: - object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, tag=tag, _value=tag.value) + object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, tag=tag) updated_tags.append(object_tag) # Save all updated tags at once to avoid partial updates with transaction.atomic(): - for object_tag in updated_tags: - object_tag.save() - # ...and delete any omitted existing tags + # delete any omitted existing tags. We do this first to reduce chances of UNIQUE constraint edge cases for old_tag in current_tags: old_tag.delete() - - return updated_tags + # add the new tags: + for object_tag in updated_tags: + object_tag.full_clean() # Run validation + object_tag.save() # TODO: return tags from closed taxonomies as well as the count of how many times each is used. diff --git a/openedx_tagging/core/tagging/migrations/0010_cleanups.py b/openedx_tagging/core/tagging/migrations/0010_cleanups.py new file mode 100644 index 00000000..fbd10271 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0010_cleanups.py @@ -0,0 +1,31 @@ +# Generated by Django 3.2.19 on 2023-09-29 16:59 + +from django.db import migrations, models +import django.db.models.expressions +import openedx_learning.lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0009_alter_objecttag_object_id'), + ] + + operations = [ + migrations.DeleteModel( + name='ModelObjectTag', + ), + migrations.DeleteModel( + name='UserModelObjectTag', + ), + migrations.AlterUniqueTogether( + name='objecttag', + unique_together={('object_id', 'taxonomy', 'tag_id'), ('object_id', 'taxonomy', '_value')}, + ), + # ObjectTag.Tag can be blank + migrations.AlterField( + model_name='objecttag', + name='tag', + field=models.ForeignKey(blank=True, default=None, help_text="Tag associated with this object tag. Provides the tag's 'value' if set.", null=True, on_delete=django.db.models.deletion.SET_NULL, to='oel_tagging.tag'), + ), + ] diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 045a715c..2b64fa76 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -6,6 +6,7 @@ import logging from typing import List +from django.core.exceptions import ValidationError from django.db import models from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ @@ -59,7 +60,7 @@ class Tag(models.Model): ) external_id = case_insensitive_char_field( max_length=255, - null=True, + null=True, # To allow multiple values with our UNIQUE constraint, we need to use NULL values here instead of "" blank=True, help_text=_( "Used to link an Open edX Tag with a tag in an externally-defined taxonomy." @@ -359,7 +360,7 @@ def validate_value(self, value: str) -> bool: """ self.check_casted() if self.allow_free_text: - return True + return value != "" and isinstance(value, str) return self.tag_set.filter(value__iexact=value).exists() def tag_for_value(self, value: str) -> Tag: @@ -435,7 +436,8 @@ class ObjectTag(models.Model): ) tag = models.ForeignKey( Tag, - null=True, + null=True, # NULL in the case of free text taxonomies or when the Tag gets deleted. + blank=True, default=None, on_delete=models.SET_NULL, help_text=_( @@ -443,7 +445,6 @@ class ObjectTag(models.Model): ), ) _name = case_insensitive_char_field( - null=False, max_length=255, help_text=_( "User-facing label used for this tag, stored in case taxonomy is (or becomes) null." @@ -451,7 +452,6 @@ class ObjectTag(models.Model): ), ) _value = case_insensitive_char_field( - null=False, max_length=500, help_text=_( "User-facing value used for this tag, stored in case tag is null, e.g if taxonomy is free text, or if it" @@ -465,7 +465,19 @@ class Meta: models.Index(fields=["taxonomy", "object_id"]), models.Index(fields=["taxonomy", "_value"]), ] - unique_together = ("taxonomy", "_value", "object_id") + unique_together = [ + ("object_id", "taxonomy", "tag_id"), + ("object_id", "taxonomy", "_value"), + ] + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + if not self.pk: # This is a new instance: + # Set _name and _value automatically on creation, if they weren't set: + if self.taxonomy and not self._name: + self._name = self.taxonomy.name + if not self._value and self.tag: + self._value = self.tag.value def __repr__(self): """ @@ -512,19 +524,26 @@ def value(self, value: str): Stores to the _value field. """ self._value = value - - def is_valid(self) -> bool: + + @property + def is_deleted(self) -> bool: """ - Returns True if this ObjectTag represents a valid taxonomy tag. - - A valid ObjectTag must be linked to a Taxonomy, and be a valid tag in that taxonomy. + Has this Tag been deleted from the Taxonomy? If so, we preserve this + ObjecTag in the DB but it shouldn't be shown to the user. """ + return self.taxonomy is None or (self.tag is None and not self.taxonomy.allow_free_text) + + def clean(self): if self.tag: - return True # If the Tag still exists, this is a valid value in the taxonomy. - elif self.taxonomy: - # If this is not a free text value and it's no longer linked to a Tag, it's invalid. - # Running self.resync() _might_ fix this though. - return self.taxonomy.allow_free_text + if self.tag.taxonomy_id != self.taxonomy_id: + raise ValidationError("ObjectTag's Taxonomy does not match Tag taxonomy") + elif self.tag.value != self._value: + raise ValidationError("ObjectTag's _value is out of sync with Tag.value") + else: + # Note: self.taxonomy and/or self.tag may be NULL which is OK, because it means the Tag/Taxonomy + # was deleted, but we still preserve this _value here in case the Taxonomy or Tag get re-created in future. + if self._value == "": + raise ValidationError("Invalid _value - empty string") def get_lineage(self) -> Lineage: """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 22071054..22878fc7 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -53,7 +53,8 @@ class Meta: "name", "value", "taxonomy_id", - "is_valid", + # If the Tag or Taxonomy has been deleted, this ObjectTag shouldn't be shown to users. + "is_deleted", ] diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index aa7bca0a..c4cc76c8 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -3,10 +3,13 @@ """ import ddt # type: ignore[import] from django.contrib.auth import get_user_model +from django.core.exceptions import ValidationError from django.db import transaction from django.db.utils import IntegrityError from django.test.testcases import TestCase +import pytest +from openedx_tagging.core.tagging import api from openedx_tagging.core.tagging.models import LanguageTaxonomy, ObjectTag, Tag, Taxonomy @@ -177,7 +180,7 @@ class Meta: @ddt.ddt -class TestModelTagTaxonomy(TestTagTaxonomyMixin, TestCase): +class TestTagTaxonomy(TestTagTaxonomyMixin, TestCase): """ Test the Tag and Taxonomy models' properties and methods. """ @@ -326,7 +329,7 @@ def test_unique_tags(self): ).save() -class TestModelObjectTag(TestTagTaxonomyMixin, TestCase): +class TestObjectTag(TestTagTaxonomyMixin, TestCase): """ Test the ObjectTag model and the related Taxonomy methods and fields. """ @@ -410,144 +413,127 @@ def test_object_tag_lineage(self): object_tag.refresh_from_db() assert object_tag.get_lineage() == ["Another tag"] - def test_tag_ref(self): - object_tag = ObjectTag() - object_tag.tag_ref = 1 - object_tag.save() - assert object_tag.tag is None - assert object_tag.value == 1 - - def test_object_tag_is_valid(self): + def test_validate_value_free_text(self): open_taxonomy = Taxonomy.objects.create( name="Freetext Life", allow_free_text=True, ) + # An empty string or other non-string is not valid in a free-text taxonomy + assert open_taxonomy.validate_value("") is False + assert open_taxonomy.validate_value(None) is False + assert open_taxonomy.validate_value(True) is False + # But any other string value is valid: + assert open_taxonomy.validate_value("Any text we want") is True + + def test_validate_value_closed(self): + """ + Test validate_value() in a closed taxonomy + """ + assert self.taxonomy.validate_value("Eukaryota") is True + assert self.taxonomy.validate_value("Foobarensia") is False + assert self.taxonomy.tag_for_value("Eukaryota").value == "Eukaryota" + with pytest.raises(api.TagDoesNotExist): + self.taxonomy.tag_for_value("Foobarensia") - object_tag = ObjectTag( - taxonomy=self.taxonomy, - ) - # 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() - + def test_clean(self): # ObjectTags in a closed taxonomy require a tag in that taxonomy - object_tag.taxonomy = self.taxonomy - object_tag.tag = Tag.objects.create( - taxonomy=self.system_taxonomy, + object_tag = ObjectTag(taxonomy=self.taxonomy, tag=Tag.objects.create( + taxonomy=self.system_taxonomy, # Different taxonomy value="PT", - ) - assert not object_tag.is_valid() + )) + with pytest.raises(ValidationError): + object_tag.full_clean() object_tag.tag = self.tag - assert object_tag.is_valid() + object_tag._value = self.tag.value + object_tag.full_clean() def test_tag_object(self): self.taxonomy.allow_multiple = True test_tags = [ [ - self.archaea.id, - self.eubacteria.id, - self.chordata.id, + self.archaea, + self.eubacteria, + self.chordata, ], [ - self.archaebacteria.id, - self.chordata.id, + self.archaebacteria, + self.chordata, ], [ - self.archaea.id, - self.archaebacteria.id, + self.archaea, + self.archaebacteria, ], ] # Tag and re-tag the object, checking that the expected tags are returned and deleted for tag_list in test_tags: - object_tags = self.taxonomy.tag_object( - tag_list, + api.tag_object( + self.taxonomy, + [t.value for t in tag_list], "biology101", ) - # Ensure the expected number of tags exist in the database - assert ObjectTag.objects.filter( + object_tags = ObjectTag.objects.filter( taxonomy=self.taxonomy, object_id="biology101", - ).count() == len(tag_list) + ) # And the expected number of tags were returned assert len(object_tags) == len(tag_list) for index, object_tag in enumerate(object_tags): - assert object_tag.tag_id == tag_list[index] - assert object_tag.is_valid + assert object_tag.tag_id == tag_list[index].id + assert object_tag._value == tag_list[index].value + object_tag.full_clean() # Should not raise any ValidationErrors assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" def test_tag_object_free_text(self): self.taxonomy.allow_free_text = True - object_tags = self.taxonomy.tag_object( + api.tag_object( + self.taxonomy, ["Eukaryota Xenomorph"], "biology101", ) + object_tags = api.get_object_tags("biology101") assert len(object_tags) == 1 object_tag = object_tags[0] - assert object_tag.is_valid + object_tag.full_clean() # Should not raise any ValidationErrors assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name - assert object_tag.tag_ref == "Eukaryota Xenomorph" + assert object_tag._value == "Eukaryota Xenomorph" assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] assert object_tag.object_id == "biology101" def test_tag_object_no_multiple(self): - with self.assertRaises(ValueError) as exc: - self.taxonomy.tag_object( - ["A", "B"], - "biology101", - ) - assert "only allows one tag per object" in str(exc.exception) + with pytest.raises(ValueError) as excinfo: + api.tag_object(self.taxonomy, ["A", "B"], "biology101") + assert "only allows one tag per object" in str(excinfo.value) def test_tag_object_required(self): self.taxonomy.required = True - with self.assertRaises(ValueError) as exc: - self.taxonomy.tag_object( - [], - "biology101", - ) - assert "requires at least one tag per object" in str(exc.exception) + with pytest.raises(ValueError) as excinfo: + api.tag_object(self.taxonomy, [], "biology101") + assert "requires at least one tag per object" in str(excinfo.value) def test_tag_object_invalid_tag(self): - with self.assertRaises(ValueError) as exc: - self.taxonomy.tag_object( - ["Eukaryota Xenomorph"], - "biology101", - ) - assert "Invalid object tag for taxonomy" in str(exc.exception) + with pytest.raises(api.TagDoesNotExist) as excinfo: + api.tag_object(self.taxonomy, ["Eukaryota Xenomorph"], "biology101") + assert "Tag matching query does not exist." in str(excinfo.value) def test_tag_case(self) -> None: """ Test that the object_id is case sensitive. """ # Tag with object_id with lower case - ObjectTag( - object_id="case:id:2", - taxonomy=self.taxonomy, - tag=self.domain_tags[0], - ).save() + api.tag_object(self.taxonomy, [self.domain_tags[0].value], object_id="case:id:2") # Tag with object_id with upper case should not trigger IntegrityError - ObjectTag( - object_id="CASE:id:2", - taxonomy=self.taxonomy, - tag=self.domain_tags[0], - ).save() + api.tag_object(self.taxonomy, [self.domain_tags[0].value], object_id="CASE:id:2") # Create another ObjectTag with lower case object_id should trigger IntegrityError with transaction.atomic(): - with self.assertRaises(IntegrityError): + with pytest.raises(IntegrityError): ObjectTag( object_id="case:id:2", taxonomy=self.taxonomy, @@ -556,9 +542,54 @@ def test_tag_case(self) -> None: # Create another ObjectTag with upper case object_id should trigger IntegrityError with transaction.atomic(): - with self.assertRaises(IntegrityError): + with pytest.raises(IntegrityError): ObjectTag( object_id="CASE:id:2", taxonomy=self.taxonomy, tag=self.domain_tags[0], ).save() + + def test_is_deleted(self): + self.taxonomy.allow_multiple=True + self.taxonomy.save() + open_taxonomy = Taxonomy.objects.create(name="Freetext Life", allow_free_text=True, allow_multiple=True) + + object_id = "obj1" + # Create some tags: + api.tag_object(self.taxonomy, [self.archaea.value, self.bacteria.value], object_id) # Regular tags + api.tag_object(open_taxonomy, ["foo", "bar", "tribble"], object_id) # Free text tags + + # At first, none of these will be deleted: + assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ + (self.archaea.value, False), + (self.bacteria.value, False), + ("foo", False), + ("bar", False), + ("tribble", False), + ] + initial_tags = api.get_object_tags(object_id) + assert len(initial_tags) == 5 + for t in initial_tags: + assert t.is_deleted is False + + # Delete "bacteria" from the taxonomy: + self.bacteria.delete() # TODO: add an API method for this + + assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. + ("foo", False), + ("bar", False), + ("tribble", False), + ] + + # Then delete the whole free text taxonomy + open_taxonomy.delete() + + assert [(t.value, t.is_deleted) for t in api.get_object_tags(object_id)] == [ + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. + ("foo", True), # <--- Deleted, but the value is preserved + ("bar", True), # <--- Deleted, but the value is preserved + ("tribble", True), # <--- Deleted, but the value is preserved + ] From 689cd26e1f13df4be0410409096b12401821bd03 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 29 Sep 2023 11:48:45 -0700 Subject: [PATCH 05/13] feat: Remove "re-sync" of deleted taxonomies, update test_api.py --- openedx_tagging/core/tagging/api.py | 6 +- openedx_tagging/core/tagging/models/base.py | 30 +- .../openedx_tagging/core/tagging/test_api.py | 376 +++++------------- .../core/tagging/test_models.py | 78 ---- 4 files changed, 108 insertions(+), 382 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index c3ffed4c..ac05b32a 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -144,15 +144,17 @@ def resync_object_tags(object_tags: QuerySet | None = None) -> int: def get_object_tags( object_id: str, + taxonomy_id: int | None = None, ObjectTagClass: type[ObjectTag] = ObjectTag ) -> QuerySet[ObjectTag]: """ Returns a Queryset of object tags for a given object. - Pass taxonomy to limit the returned object_tags to a specific taxonomy. + Pass taxonomy_id to limit the returned object_tags to a specific taxonomy. """ + filters = {"taxonomy_id": taxonomy_id} if taxonomy_id else {} tags = ( - ObjectTagClass.objects.filter(object_id=object_id) + ObjectTagClass.objects.filter(object_id=object_id, **filters) .select_related("tag", "taxonomy") .order_by("id") ) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 2b64fa76..6ec2e3cd 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -567,34 +567,8 @@ def resync(self) -> bool: """ 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: - 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"): - # Cast to the subclass to preserve custom validation - taxonomy = taxonomy.cast() - - # 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. - self.taxonomy = taxonomy - self.tag = tag - if taxonomy.validate_object_tag(self): - changed = True - break - # If not, undo those changes and try the next one - else: - self.taxonomy = None - self.tag = None + # We used to have code here that would try to find a new taxonomy if the current taxonomy has been deleted. + # But for now that's removed, as it risks things like linking a tag to the wrong org's taxonomy. # Sync the stored _name with the taxonomy.name if self.taxonomy and self._name != self.taxonomy.name: diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 422b15c1..81c236a0 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -2,11 +2,11 @@ Test the tagging APIs """ from __future__ import annotations - from typing import Any import ddt # type: ignore[import] from django.test import TestCase, override_settings +import pytest import openedx_tagging.core.tagging.api as tagging_api from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy @@ -174,226 +174,128 @@ def check_object_tag( assert object_tag.value == value def test_resync_object_tags(self) -> None: - missing_links = ObjectTag.objects.create( - object_id="abc", - _name=self.taxonomy.name, - _value=self.mammalia.value, - ) - changed_links = ObjectTag.objects.create( - object_id="def", - 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", - taxonomy=self.taxonomy, - tag=self.mammalia, - ) - no_changes.name = self.taxonomy.name - no_changes.value = self.mammalia.value - no_changes.save() + self.taxonomy.allow_multiple = True + self.taxonomy.save() + open_taxonomy = Taxonomy.objects.create(name="Freetext Life", allow_free_text=True, allow_multiple=True) + + object_id = "obj1" + # Create some tags: + tagging_api.tag_object(self.taxonomy, [self.archaea.value, self.bacteria.value], object_id) # Regular tags + tagging_api.tag_object(open_taxonomy, ["foo", "bar"], object_id) # Free text tags + + # At first, none of these will be deleted: + assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ + (self.archaea.value, False), + (self.bacteria.value, False), + ("foo", False), + ("bar", False), + ] - 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" - ) + # Delete "bacteria" from the taxonomy: + self.bacteria.delete() # TODO: add an API method for this - # Once all tags are resynced, they stay that way - changed = tagging_api.resync_object_tags() - assert changed == 0 + assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ + (self.archaea.value, False), + (self.bacteria.value, True), # <--- deleted! But the value is preserved. + ("foo", False), + ("bar", False), + ] - # Resync will use the tag's taxonomy if possible - changed_links.taxonomy = None - changed_links.save() - changed = tagging_api.resync_object_tags() - assert changed == 1 - for object_tag in (missing_links, changed_links, no_changes): - self.check_object_tag( - object_tag, self.taxonomy, self.mammalia, "Life on Earth", "Mammalia" - ) + # Re-syncing the tags at this point does nothing: + tagging_api.resync_object_tags() - # Resync will use the taxonomy's tags if possible - changed_links.tag = None - changed_links.value = "Xenomorph" - changed_links.save() - changed = tagging_api.resync_object_tags() - assert changed == 0 - changed_links.value = "Mammalia" - changed_links.save() - - # 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, - ) + # Now re-create the tag + self.bacteria.save() + + # Then re-sync the tags: 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" - ) + assert changed == 1 - # 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") + # Now the tag is not deleted: + assert [(t.value, t.is_deleted) for t in tagging_api.get_object_tags(object_id)] == [ + (self.archaea.value, False), + (self.bacteria.value, False), # <--- not deleted + ("foo", False), + ("bar", False), + ] - # Resyncing the tags for code coverage + # Re-syncing the tags now does nothing: 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_id__in=["abc", "def"]) - ) - 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(id=no_changes.id) - ) - assert changed == 1 - self.check_object_tag( - no_changes, first_taxonomy, None, "Life on Earth", "Anamelia" - ) - - def test_tag_object(self) -> None: + def test_tag_object(self): self.taxonomy.allow_multiple = True - self.taxonomy.save() + test_tags = [ [ - self.archaea.id, - self.eubacteria.id, - self.chordata.id, + self.archaea, + self.eubacteria, + self.chordata, ], [ - self.chordata.id, - self.archaebacteria.id, + self.archaebacteria, + self.chordata, ], [ - self.archaebacteria.id, - self.archaea.id, + self.archaea, + self.archaebacteria, ], ] # Tag and re-tag the object, checking that the expected tags are returned and deleted for tag_list in test_tags: - object_tags = tagging_api.tag_object( + tagging_api.tag_object( self.taxonomy, - tag_list, + [t.value for t in tag_list], "biology101", ) - # Ensure the expected number of tags exist in the database - assert ( - list( - tagging_api.get_object_tags( - taxonomy_id=self.taxonomy.pk, - object_id="biology101", - ) - ) - == object_tags + object_tags = ObjectTag.objects.filter( + taxonomy=self.taxonomy, + object_id="biology101", ) # And the expected number of tags were returned assert len(object_tags) == len(tag_list) for index, object_tag in enumerate(object_tags): - assert object_tag.tag_id == tag_list[index] - assert object_tag.is_valid() + assert object_tag.tag_id == tag_list[index].id + assert object_tag._value == tag_list[index].value + object_tag.full_clean() # Should not raise any ValidationErrors assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" - # Delete the tags - tagging_api.delete_object_tags("biology101") - - # Ensure the tags were deleted - assert ( - len( - list( - tagging_api.get_object_tags( - object_id="biology101", - ) - ) - ) - == 0 - ) - - def test_tag_object_free_text(self) -> None: + def test_tag_object_free_text(self): self.taxonomy.allow_free_text = True - self.taxonomy.save() - object_tags = tagging_api.tag_object( + tagging_api.tag_object( self.taxonomy, ["Eukaryota Xenomorph"], "biology101", ) + object_tags = tagging_api.get_object_tags("biology101") assert len(object_tags) == 1 object_tag = object_tags[0] - assert object_tag.is_valid() + object_tag.full_clean() # Should not raise any ValidationErrors assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name - assert object_tag.tag_ref == "Eukaryota Xenomorph" + assert object_tag._value == "Eukaryota Xenomorph" assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] assert object_tag.object_id == "biology101" - def test_tag_object_no_multiple(self) -> None: - with self.assertRaises(ValueError) as exc: - tagging_api.tag_object( - self.taxonomy, - ["A", "B"], - "biology101", - ) - assert "only allows one tag per object" in str(exc.exception) + def test_tag_object_no_multiple(self): + with pytest.raises(ValueError) as excinfo: + tagging_api.tag_object(self.taxonomy, ["A", "B"], "biology101") + assert "only allows one tag per object" in str(excinfo.value) - def test_tag_object_required(self) -> None: + def test_tag_object_required(self): self.taxonomy.required = True - self.taxonomy.save() - with self.assertRaises(ValueError) as exc: - tagging_api.tag_object( - self.taxonomy, - [], - "biology101", - ) - assert "requires at least one tag per object" in str(exc.exception) + with pytest.raises(ValueError) as excinfo: + tagging_api.tag_object(self.taxonomy, [], "biology101") + assert "requires at least one tag per object" in str(excinfo.value) - def test_tag_object_invalid_tag(self) -> None: - 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_tag_object_invalid_tag(self): + with pytest.raises(tagging_api.TagDoesNotExist) as excinfo: + tagging_api.tag_object(self.taxonomy, ["Eukaryota Xenomorph"], "biology101") + assert "Tag matching query does not exist." in str(excinfo.value) def test_tag_object_string(self) -> None: with self.assertRaises(ValueError) as exc: @@ -414,17 +316,18 @@ def test_tag_object_integer(self) -> None: assert "Tags must be a list, not int." in str(exc.exception) def test_tag_object_same_id(self) -> None: - # Tag the object with the same id twice + # Tag the object with the same tag twice tagging_api.tag_object( self.taxonomy, - [self.eubacteria.id], + [self.eubacteria.value], "biology101", ) - object_tags = tagging_api.tag_object( + tagging_api.tag_object( self.taxonomy, - [self.eubacteria.id], + [self.eubacteria.value], "biology101", ) + object_tags = tagging_api.get_object_tags("biology101") assert len(object_tags) == 1 assert str(object_tags[0]) == " biology101: Life on Earth=Eubacteria" @@ -432,66 +335,24 @@ def test_tag_object_same_value(self) -> None: # Tag the object with the same value twice tagging_api.tag_object( self.taxonomy, - ["Eubacteria"], + [self.eubacteria.value, self.eubacteria.value], "biology101", ) - object_tags = tagging_api.tag_object( - self.taxonomy, - ["Eubacteria"], - "biology101", - ) - + object_tags = tagging_api.get_object_tags("biology101") assert len(object_tags) == 1 assert str(object_tags[0]) == " biology101: Life on Earth=Eubacteria" - def test_tag_object_same_mixed(self) -> None: - # Tag the object with the same id/value twice - tagging_api.tag_object( - self.taxonomy, - [self.eubacteria.id], - "biology101", - ) - object_tags = tagging_api.tag_object( - self.taxonomy, - ["Eubacteria"], - "biology101", - ) - - assert len(object_tags) == 1 - assert str(object_tags[0]) == " biology101: Life on Earth=Eubacteria" - - def test_tag_object_same_id_multiple(self) -> None: - self.taxonomy.allow_multiple = True - self.taxonomy.save() - # Tag the object with the same value twice - object_tags = tagging_api.tag_object( - self.taxonomy, - [self.eubacteria.id, self.eubacteria.id], - "biology101", - ) - assert len(object_tags) == 1 - - def test_tag_object_same_value_multiple(self) -> None: - self.taxonomy.allow_multiple = True - self.taxonomy.save() - # Tag the object with the same value twice - object_tags = tagging_api.tag_object( - self.taxonomy, - ["Eubacteria", "Eubacteria"], - "biology101", - ) - assert len(object_tags) == 1 - def test_tag_object_same_value_multiple_free(self) -> None: self.taxonomy.allow_multiple = True self.taxonomy.allow_free_text = True self.taxonomy.save() # Tag the object with the same value twice - object_tags = tagging_api.tag_object( + tagging_api.tag_object( self.taxonomy, ["tag1", "tag1"], "biology101", ) + object_tags = tagging_api.get_object_tags("biology101") assert len(object_tags) == 1 def test_tag_object_case_id(self) -> None: @@ -500,13 +361,13 @@ def test_tag_object_case_id(self) -> None: """ tagging_api.tag_object( self.taxonomy, - [self.eubacteria.id], + [self.eubacteria.value], "biology101", ) tagging_api.tag_object( self.taxonomy, - [self.archaea.id], + [self.archaea.value], "BIOLOGY101", ) @@ -529,48 +390,33 @@ def test_tag_object_case_id(self) -> None: @override_settings(LANGUAGES=test_languages) def test_tag_object_language_taxonomy(self) -> None: tags_list = [ - [get_tag("Azerbaijani").id], - [get_tag("English").id], + ["Azerbaijani"], + ["English"], ] for tags in tags_list: - object_tags = tagging_api.tag_object( - self.language_taxonomy, - tags, - "biology101", - ) + tagging_api.tag_object(self.language_taxonomy, tags, "biology101") # Ensure the expected number of tags exist in the database - assert ( - list( - tagging_api.get_object_tags( - taxonomy_id=self.language_taxonomy.pk, - object_id="biology101", - ) - ) - == object_tags - ) + object_tags = tagging_api.get_object_tags("biology101") # And the expected number of tags were returned assert len(object_tags) == len(tags) for index, object_tag in enumerate(object_tags): - assert object_tag.tag_id == tags[index] - assert object_tag.is_valid() + object_tag.full_clean() # Check full model validation + assert object_tag.value == tags[index] + assert not object_tag.is_deleted assert object_tag.taxonomy == self.language_taxonomy assert object_tag.name == self.language_taxonomy.name assert object_tag.object_id == "biology101" @override_settings(LANGUAGES=test_languages) - def test_tag_object_language_taxonomy_ivalid(self) -> None: - tags = [get_tag("Spanish").id] - with self.assertRaises(ValueError) as exc: + def test_tag_object_language_taxonomy_invalid(self) -> None: + with self.assertRaises(tagging_api.TagDoesNotExist): tagging_api.tag_object( self.language_taxonomy, - tags, + ["Spanish"], "biology101", ) - assert "Invalid object tag for taxonomy (-1): -40" in str( - exc.exception - ) def test_tag_object_model_system_taxonomy(self) -> None: users = [ @@ -579,45 +425,27 @@ def test_tag_object_model_system_taxonomy(self) -> None: ] for user in users: - tags = [user.id] - object_tags = tagging_api.tag_object( - self.user_taxonomy, - tags, - "biology101", - ) + tags = [user.username] + tagging_api.tag_object(self.user_taxonomy, tags, "biology101") # Ensure the expected number of tags exist in the database - assert ( - list( - tagging_api.get_object_tags( - taxonomy_id=self.user_taxonomy.pk, - object_id="biology101", - ) - ) - == object_tags - ) + object_tags = tagging_api.get_object_tags("biology101") # And the expected number of tags were returned assert len(object_tags) == len(tags) for object_tag in object_tags: + object_tag.full_clean() # Check full model validation assert object_tag.tag assert object_tag.tag.external_id == str(user.id) assert object_tag.tag.value == user.username - assert object_tag.is_valid() + assert not object_tag.is_deleted assert object_tag.taxonomy == self.user_taxonomy assert object_tag.name == self.user_taxonomy.name assert object_tag.object_id == "biology101" def test_tag_object_model_system_taxonomy_invalid(self) -> None: tags = ["Invalid id"] - with self.assertRaises(ValueError) as exc: - tagging_api.tag_object( - self.user_taxonomy, - tags, - "biology101", - ) - assert "Invalid object tag for taxonomy (3): Invalid id" in str( - exc.exception - ) + with self.assertRaises(tagging_api.TagDoesNotExist): + tagging_api.tag_object(self.user_taxonomy, tags, "biology101") def test_tag_object_limit(self) -> None: """ diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index c4cc76c8..c7825190 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -447,80 +447,6 @@ def test_clean(self): object_tag._value = self.tag.value object_tag.full_clean() - def test_tag_object(self): - self.taxonomy.allow_multiple = True - - test_tags = [ - [ - self.archaea, - self.eubacteria, - self.chordata, - ], - [ - self.archaebacteria, - self.chordata, - ], - [ - self.archaea, - self.archaebacteria, - ], - ] - - # Tag and re-tag the object, checking that the expected tags are returned and deleted - for tag_list in test_tags: - api.tag_object( - self.taxonomy, - [t.value for t in tag_list], - "biology101", - ) - # Ensure the expected number of tags exist in the database - object_tags = ObjectTag.objects.filter( - taxonomy=self.taxonomy, - object_id="biology101", - ) - # And the expected number of tags were returned - assert len(object_tags) == len(tag_list) - for index, object_tag in enumerate(object_tags): - assert object_tag.tag_id == tag_list[index].id - assert object_tag._value == tag_list[index].value - object_tag.full_clean() # Should not raise any ValidationErrors - assert object_tag.taxonomy == self.taxonomy - assert object_tag.name == self.taxonomy.name - assert object_tag.object_id == "biology101" - - def test_tag_object_free_text(self): - self.taxonomy.allow_free_text = True - api.tag_object( - self.taxonomy, - ["Eukaryota Xenomorph"], - "biology101", - ) - object_tags = api.get_object_tags("biology101") - assert len(object_tags) == 1 - object_tag = object_tags[0] - object_tag.full_clean() # Should not raise any ValidationErrors - assert object_tag.taxonomy == self.taxonomy - assert object_tag.name == self.taxonomy.name - assert object_tag._value == "Eukaryota Xenomorph" - assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] - assert object_tag.object_id == "biology101" - - def test_tag_object_no_multiple(self): - with pytest.raises(ValueError) as excinfo: - api.tag_object(self.taxonomy, ["A", "B"], "biology101") - assert "only allows one tag per object" in str(excinfo.value) - - def test_tag_object_required(self): - self.taxonomy.required = True - with pytest.raises(ValueError) as excinfo: - api.tag_object(self.taxonomy, [], "biology101") - assert "requires at least one tag per object" in str(excinfo.value) - - def test_tag_object_invalid_tag(self): - with pytest.raises(api.TagDoesNotExist) as excinfo: - api.tag_object(self.taxonomy, ["Eukaryota Xenomorph"], "biology101") - assert "Tag matching query does not exist." in str(excinfo.value) - def test_tag_case(self) -> None: """ Test that the object_id is case sensitive. @@ -567,10 +493,6 @@ def test_is_deleted(self): ("bar", False), ("tribble", False), ] - initial_tags = api.get_object_tags(object_id) - assert len(initial_tags) == 5 - for t in initial_tags: - assert t.is_deleted is False # Delete "bacteria" from the taxonomy: self.bacteria.delete() # TODO: add an API method for this From 6480da3ba62eba33051e1f6cf4114eceab9faf7a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 29 Sep 2023 11:57:24 -0700 Subject: [PATCH 06/13] chore: get all tests passing --- openedx_tagging/core/tagging/rest_api/v1/views.py | 2 ++ tests/openedx_tagging/core/tagging/test_views.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 91208a0f..9dbb2cc0 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -318,6 +318,8 @@ def update(self, request, object_id, partial=False): tags = body.data.get("tags", []) try: tag_object(taxonomy, tags, object_id) + except Tag.DoesNotExist as e: + raise ValidationError(e) except ValueError as e: raise ValidationError(e) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index b7b12cd3..effd6460 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -533,7 +533,7 @@ def test_retrieve_object_tags_taxonomy_queryparam( if status.is_success(expected_status): assert len(response.data) == expected_count for object_tag in response.data: - assert object_tag.get("is_valid") is True + assert object_tag.get("is_deleted") is False assert object_tag.get("taxonomy_id") == self.enabled_taxonomy.pk @ddt.data( From 54c01a5e5ecbe1aeee94c8eb12d9c4ee361093ab Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 29 Sep 2023 12:02:56 -0700 Subject: [PATCH 07/13] chore: quality fixes --- openedx_tagging/core/tagging/api.py | 2 +- .../core/tagging/migrations/0010_cleanups.py | 3 ++- openedx_tagging/core/tagging/models/base.py | 4 +-- .../core/tagging/models/system_defined.py | 2 +- .../openedx_tagging/core/tagging/test_api.py | 9 ++++--- .../core/tagging/test_models.py | 6 ++--- .../tagging/test_system_defined_models.py | 27 ++++--------------- 7 files changed, 19 insertions(+), 34 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index ac05b32a..d711bed8 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -15,7 +15,7 @@ from typing import Iterator from django.db import transaction -from django.db.models import QuerySet, F +from django.db.models import F, QuerySet from django.utils.translation import gettext_lazy as _ from .models import ObjectTag, Tag, Taxonomy diff --git a/openedx_tagging/core/tagging/migrations/0010_cleanups.py b/openedx_tagging/core/tagging/migrations/0010_cleanups.py index fbd10271..35b74a2d 100644 --- a/openedx_tagging/core/tagging/migrations/0010_cleanups.py +++ b/openedx_tagging/core/tagging/migrations/0010_cleanups.py @@ -1,7 +1,8 @@ # Generated by Django 3.2.19 on 2023-09-29 16:59 -from django.db import migrations, models import django.db.models.expressions +from django.db import migrations, models + import openedx_learning.lib.fields diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 6ec2e3cd..f8246308 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -244,7 +244,7 @@ def cast(self): ) return self - + def check_casted(self): """ Double-check that this taxonomy has been cast() to a subclass if needed. @@ -524,7 +524,7 @@ def value(self, value: str): Stores to the _value field. """ self._value = value - + @property def is_deleted(self) -> bool: """ diff --git a/openedx_tagging/core/tagging/models/system_defined.py b/openedx_tagging/core/tagging/models/system_defined.py index c431e1c0..bcd90800 100644 --- a/openedx_tagging/core/tagging/models/system_defined.py +++ b/openedx_tagging/core/tagging/models/system_defined.py @@ -103,7 +103,7 @@ def tag_for_value(self, value: str): except ObjectDoesNotExist: raise Tag.DoesNotExist # Use the canonical value from here on (possibly with different case from the value given as a parameter) - value = getattr(instance, self.tag_class_value_field) + value = getattr(instance, self.tag_class_value_field) # We assume the value may change but the external_id is immutable. # So look up keys using external_id. There may be a key with the same external_id but an out of date value. external_id = str(getattr(instance, self.tag_class_key_field)) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 81c236a0..6b9aac17 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -2,11 +2,12 @@ Test the tagging APIs """ from __future__ import annotations + from typing import Any import ddt # type: ignore[import] -from django.test import TestCase, override_settings import pytest +from django.test import TestCase, override_settings import openedx_tagging.core.tagging.api as tagging_api from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy @@ -257,9 +258,9 @@ def test_tag_object(self): # And the expected number of tags were returned assert len(object_tags) == len(tag_list) for index, object_tag in enumerate(object_tags): - assert object_tag.tag_id == tag_list[index].id - assert object_tag._value == tag_list[index].value object_tag.full_clean() # Should not raise any ValidationErrors + assert object_tag.tag_id == tag_list[index].id + assert object_tag._value == tag_list[index].value # pylint: disable=protected-access assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" @@ -277,7 +278,7 @@ def test_tag_object_free_text(self): object_tag.full_clean() # Should not raise any ValidationErrors assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name - assert object_tag._value == "Eukaryota Xenomorph" + assert object_tag._value == "Eukaryota Xenomorph" # pylint: disable=protected-access assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] assert object_tag.object_id == "biology101" diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index c7825190..4ea699a8 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -2,12 +2,12 @@ Test the tagging base models """ import ddt # type: ignore[import] +import pytest from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.db import transaction from django.db.utils import IntegrityError from django.test.testcases import TestCase -import pytest from openedx_tagging.core.tagging import api from openedx_tagging.core.tagging.models import LanguageTaxonomy, ObjectTag, Tag, Taxonomy @@ -444,7 +444,7 @@ def test_clean(self): with pytest.raises(ValidationError): object_tag.full_clean() object_tag.tag = self.tag - object_tag._value = self.tag.value + object_tag._value = self.tag.value # pylint: disable=protected-access object_tag.full_clean() def test_tag_case(self) -> None: @@ -476,7 +476,7 @@ def test_tag_case(self) -> None: ).save() def test_is_deleted(self): - self.taxonomy.allow_multiple=True + self.taxonomy.allow_multiple = True self.taxonomy.save() open_taxonomy = Taxonomy.objects.create(name="Freetext Life", allow_free_text=True, allow_multiple=True) diff --git a/tests/openedx_tagging/core/tagging/test_system_defined_models.py b/tests/openedx_tagging/core/tagging/test_system_defined_models.py index 3026a844..7280d5c6 100644 --- a/tests/openedx_tagging/core/tagging/test_system_defined_models.py +++ b/tests/openedx_tagging/core/tagging/test_system_defined_models.py @@ -2,19 +2,17 @@ Test the tagging system-defined taxonomy models """ from __future__ import annotations + from datetime import datetime import ddt # type: ignore[import] -from django.test import TestCase, override_settings import pytest +from django.test import TestCase, override_settings from openedx_learning.core.publishing.models import LearningPackage from openedx_tagging.core.tagging import api from openedx_tagging.core.tagging.models import Taxonomy -from openedx_tagging.core.tagging.models.system_defined import ( - ModelSystemDefinedTaxonomy, - UserSystemDefinedTaxonomy, -) +from openedx_tagging.core.tagging.models.system_defined import ModelSystemDefinedTaxonomy, UserSystemDefinedTaxonomy from .test_models import TestTagTaxonomyMixin @@ -34,21 +32,6 @@ class EmptyTestClass: """ -class InvalidModelTaxonomy(ModelSystemDefinedTaxonomy): - """ - Model used for testing - """ - - @property - def object_tag_class(self): - return EmptyTestClass - - class Meta: - proxy = True - managed = False - app_label = "oel_tagging" - - class TestLPTaxonomy(ModelSystemDefinedTaxonomy): """ Model used for testing - points to LearningPackage instances @@ -210,7 +193,7 @@ def test_tag_object_resync(self): api.tag_object(self.author_taxonomy, [self.user_1.username], object2_id) initial_object_tags = api.get_object_tags(object1_id) assert [t.value for t in initial_object_tags] == [self.user_1.username] - assert list(api.get_object_tags(other_obj_id)) == [] + assert not list(api.get_object_tags(other_obj_id)) # Change user_1's username: new_username = "new_username" self.user_1.username = new_username @@ -222,7 +205,7 @@ def test_tag_object_resync(self): # This is good - all the objects throughout the system with this tag now show the new value. assert [t.value for t in api.get_object_tags(object2_id)] == [new_username] # And just to make sure there are no other random changes to other objects: - assert list(api.get_object_tags(other_obj_id)) == [] + assert not list(api.get_object_tags(other_obj_id)) def test_tag_object_delete_user(self): """ From 89a4ab8265beed65bfbf987dd524f49491a8f8a3 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 29 Sep 2023 12:55:23 -0700 Subject: [PATCH 08/13] fix: case-insensitive values on MySQL --- .../core/tagging/models/system_defined.py | 6 ++-- .../tagging/test_system_defined_models.py | 36 +++++++++++++++---- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/openedx_tagging/core/tagging/models/system_defined.py b/openedx_tagging/core/tagging/models/system_defined.py index bcd90800..0978ed62 100644 --- a/openedx_tagging/core/tagging/models/system_defined.py +++ b/openedx_tagging/core/tagging/models/system_defined.py @@ -98,7 +98,8 @@ def tag_for_value(self, value: str): Get the Tag object for the given value. """ try: - # First we look up the instance by value + # First we look up the instance by value. + # We specify 'iexact' but whether it's case sensitive or not on MySQL depends on the model's collation. instance = self.tag_class_model.objects.get(**{f"{self.tag_class_value_field}__iexact": value}) except ObjectDoesNotExist: raise Tag.DoesNotExist @@ -133,7 +134,8 @@ def tag_for_external_id(self, external_id: str): Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. """ try: - # First we look up the instance by value + # First we look up the instance by external_id + # We specify 'iexact' but whether it's case sensitive or not on MySQL depends on the model's collation. instance = self.tag_class_model.objects.get(**{f"{self.tag_class_key_field}__iexact": external_id}) except ObjectDoesNotExist: raise Tag.DoesNotExist diff --git a/tests/openedx_tagging/core/tagging/test_system_defined_models.py b/tests/openedx_tagging/core/tagging/test_system_defined_models.py index 7280d5c6..09f7b2bc 100644 --- a/tests/openedx_tagging/core/tagging/test_system_defined_models.py +++ b/tests/openedx_tagging/core/tagging/test_system_defined_models.py @@ -3,7 +3,7 @@ """ from __future__ import annotations -from datetime import datetime +from datetime import datetime, timezone import ddt # type: ignore[import] import pytest @@ -54,6 +54,24 @@ class Meta: app_label = "oel_tagging" +class CaseInsensitiveTitleLPTaxonomy(TestLPTaxonomy): + """ + Model that points to LearningPackage instances but uses 'title' as values + """ + @property + def tag_class_value_field(self) -> str: + # Title isn't unique, so wouldn't make a good 'value' in real usage, but title is case-insensitive so we use it + # here to test case insensitivity. (On MySQL, only columns with case-insensitive collation can be used with + # case-insensitive comparison operators. On SQLite you could just use the 'key' field for testing, and it works + # fine.) + return "title" + + class Meta: + proxy = True + managed = False + app_label = "oel_tagging" + + @ddt.ddt class TestModelSystemDefinedTaxonomy(TestTagTaxonomyMixin, TestCase): """ @@ -62,7 +80,7 @@ class TestModelSystemDefinedTaxonomy(TestTagTaxonomyMixin, TestCase): @staticmethod def _create_learning_pkg(**kwargs) -> LearningPackage: - timestamp = datetime.now() + timestamp = datetime.now(tz=timezone.utc) return LearningPackage.objects.create(**kwargs, created=timestamp, updated=timestamp) @classmethod @@ -147,10 +165,16 @@ def test_case_insensitive_values(self): For now, values are case insensitive. We may change that in the future. """ object1_id, object2_id = "obj1", "obj2" - api.tag_object(self.lp_taxonomy, ["P1"], object1_id) - api.tag_object(self.lp_taxonomy, ["p1", "P2"], object2_id) - assert [t.value for t in api.get_object_tags(object1_id)] == ["p1"] - assert [t.value for t in api.get_object_tags(object2_id)] == ["p1", "p2"] + taxonomy = CaseInsensitiveTitleLPTaxonomy.objects.create( + taxonomy_class=CaseInsensitiveTitleLPTaxonomy, + name="LearningPackage Title Taxonomy", + allow_multiple=True, + ) + api.tag_object(taxonomy, ["LEARNING PACKAGE 1"], object1_id) + api.tag_object(taxonomy, ["Learning Package 1", "LEARNING PACKAGE 2"], object2_id) + # But they always get normalized to the case used on the actual model: + assert [t.value for t in api.get_object_tags(object1_id)] == ["Learning Package 1"] + assert [t.value for t in api.get_object_tags(object2_id)] == ["Learning Package 1", "Learning Package 2"] def test_multiple_taxonomies(self): """ From ba53463a1994e76f5d4fb12b7c1337b463ab530c Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 29 Sep 2023 15:07:09 -0700 Subject: [PATCH 09/13] feat: minor cleanups --- openedx_tagging/core/tagging/api.py | 9 +++++---- .../core/tagging/fixtures/language_taxonomy.yaml | 1 + tests/openedx_tagging/core/tagging/test_api.py | 2 +- tests/openedx_tagging/core/tagging/test_models.py | 4 +--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index d711bed8..7619291e 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -145,7 +145,7 @@ def resync_object_tags(object_tags: QuerySet | None = None) -> int: def get_object_tags( object_id: str, taxonomy_id: int | None = None, - ObjectTagClass: type[ObjectTag] = ObjectTag + object_tag_class: type[ObjectTag] = ObjectTag ) -> QuerySet[ObjectTag]: """ Returns a Queryset of object tags for a given object. @@ -154,7 +154,7 @@ def get_object_tags( """ filters = {"taxonomy_id": taxonomy_id} if taxonomy_id else {} tags = ( - ObjectTagClass.objects.filter(object_id=object_id, **filters) + object_tag_class.objects.filter(object_id=object_id, **filters) .select_related("tag", "taxonomy") .order_by("id") ) @@ -175,7 +175,7 @@ def tag_object( taxonomy: Taxonomy, tags: list[str], object_id: str, - ObjectTagClass: type[ObjectTag] = ObjectTag, + object_tag_class: type[ObjectTag] = ObjectTag, ) -> None: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id @@ -183,7 +183,7 @@ def tag_object( tags: A list of the values of the tags from this taxonomy to apply. - ObjectTagClass: Optional. Use a proxy subclass of ObjectTag for additional + 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. @@ -206,6 +206,7 @@ def _check_new_tag_count(new_tag_count: int) -> None: if not isinstance(tags, list): raise ValueError(_(f"Tags must be a list, not {type(tags).__name__}.")) + ObjectTagClass = object_tag_class taxonomy = taxonomy.cast() # Make sure we're using the right subclass. This is a no-op if we are already. tags = list(dict.fromkeys(tags)) # Remove duplicates preserving order diff --git a/openedx_tagging/core/tagging/fixtures/language_taxonomy.yaml b/openedx_tagging/core/tagging/fixtures/language_taxonomy.yaml index 38355b99..b300d8ac 100644 --- a/openedx_tagging/core/tagging/fixtures/language_taxonomy.yaml +++ b/openedx_tagging/core/tagging/fixtures/language_taxonomy.yaml @@ -1296,3 +1296,4 @@ allow_multiple: false allow_free_text: false visible_to_authors: true + _taxonomy_class: openedx_tagging.core.tagging.models.system_defined.LanguageTaxonomy diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 6b9aac17..56ada673 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -82,7 +82,7 @@ def test_get_taxonomies(self) -> None: ] + self.dummy_taxonomies assert str(enabled[0]) == f" ({tax1.id}) Enabled" assert str(enabled[1]) == " (5) Import Taxonomy Test" - assert str(enabled[2]) == " (-1) Languages" + assert str(enabled[2]) == " (-1) Languages" assert str(enabled[3]) == " (1) Life on Earth" assert str(enabled[4]) == " (4) System defined taxonomy" diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 4ea699a8..6c1d852e 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -33,9 +33,7 @@ def setUp(self): self.system_taxonomy = Taxonomy.objects.get( name="System defined taxonomy" ) - self.language_taxonomy = Taxonomy.objects.get(name="Languages") - self.language_taxonomy.taxonomy_class = LanguageTaxonomy - self.language_taxonomy = self.language_taxonomy.cast() + self.language_taxonomy = LanguageTaxonomy.objects.get(name="Languages") self.user_taxonomy = Taxonomy.objects.get(name="User Authors").cast() self.archaea = get_tag("Archaea") self.archaebacteria = get_tag("Archaebacteria") From 3895b3af597f103c3885b3dbc3556c9a953310ae Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 29 Sep 2023 16:29:26 -0700 Subject: [PATCH 10/13] fix: fix flaky test --- tests/openedx_tagging/core/tagging/test_api.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 56ada673..182f79c2 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -234,12 +234,12 @@ def test_tag_object(self): self.chordata, ], [ - self.archaebacteria, self.chordata, + self.archaebacteria, ], [ - self.archaea, self.archaebacteria, + self.archaea, ], ] @@ -251,10 +251,7 @@ def test_tag_object(self): "biology101", ) # Ensure the expected number of tags exist in the database - object_tags = ObjectTag.objects.filter( - taxonomy=self.taxonomy, - object_id="biology101", - ) + object_tags = tagging_api.get_object_tags("biology101", taxonomy_id=self.taxonomy.id) # And the expected number of tags were returned assert len(object_tags) == len(tag_list) for index, object_tag in enumerate(object_tags): From 47ec0dc85004ca21039ba04353cd32475e28956a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 3 Oct 2023 14:11:25 -0700 Subject: [PATCH 11/13] chore: address lint issues & review comments --- openedx_tagging/core/tagging/api.py | 12 ++++----- .../core/tagging/import_export/api.py | 2 +- .../core/tagging/models/__init__.py | 3 +++ openedx_tagging/core/tagging/models/base.py | 19 ++++++++++---- .../core/tagging/models/system_defined.py | 26 ++++++++++--------- 5 files changed, 37 insertions(+), 25 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 7619291e..14595a1a 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -12,8 +12,6 @@ """ from __future__ import annotations -from typing import Iterator - from django.db import transaction from django.db.models import F, QuerySet from django.utils.translation import gettext_lazy as _ @@ -50,11 +48,11 @@ def create_taxonomy( return taxonomy.cast() -def get_taxonomy(id: int) -> Taxonomy | None: +def get_taxonomy(taxonomy_id: int) -> Taxonomy | None: """ Returns a Taxonomy cast to the appropriate subclass which has the given ID. """ - taxonomy = Taxonomy.objects.filter(id=id).first() + taxonomy = Taxonomy.objects.filter(pk=taxonomy_id).first() return taxonomy.cast() if taxonomy else None @@ -226,7 +224,7 @@ def _check_new_tag_count(new_tag_count: int) -> None: updated_tags = [] if taxonomy.allow_free_text: for tag_value in tags: - object_tag_index = next((i for (i, t) in enumerate(current_tags) if t._value == tag_value), -1) + object_tag_index = next((i for (i, t) in enumerate(current_tags) if t.value == tag_value), -1) if object_tag_index >= 0: # This tag is already applied. object_tag = current_tags.pop(object_tag_index) @@ -241,9 +239,9 @@ def _check_new_tag_count(new_tag_count: int) -> None: if object_tag_index >= 0: # This tag is already applied. object_tag = current_tags.pop(object_tag_index) - if object_tag._value != tag.value: + if object_tag._value != tag.value: # pylint: disable=protected-access # The ObjectTag's cached '_value' is out of sync with the Tag, so update it: - object_tag._value = tag.value + object_tag._value = tag.value # pylint: disable=protected-access updated_tags.append(object_tag) else: # We are newly applying this tag: diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 80f01ad6..77203bf5 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -183,7 +183,7 @@ def _import_export_validations(taxonomy: Taxonomy): if taxonomy.allow_free_text: raise NotImplementedError( _( - f"Import/export for free-form taxonomies will be implemented in the future." + "Import/export for free-form taxonomies will be implemented in the future." ) ) if taxonomy.system_defined: diff --git a/openedx_tagging/core/tagging/models/__init__.py b/openedx_tagging/core/tagging/models/__init__.py index d3d8ab1f..c6b31e05 100644 --- a/openedx_tagging/core/tagging/models/__init__.py +++ b/openedx_tagging/core/tagging/models/__init__.py @@ -1,3 +1,6 @@ +""" +Core models for Tagging +""" from .base import ObjectTag, Tag, Taxonomy from .import_export import TagImportTask, TagImportTaskState from .system_defined import LanguageTaxonomy, ModelSystemDefinedTaxonomy, UserSystemDefinedTaxonomy diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index f8246308..eb4678bd 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -264,7 +264,7 @@ def copy(self, taxonomy: Taxonomy) -> Taxonomy: self.allow_multiple = taxonomy.allow_multiple self.allow_free_text = taxonomy.allow_free_text self.visible_to_authors = taxonomy.visible_to_authors - self._taxonomy_class = taxonomy._taxonomy_class + self._taxonomy_class = taxonomy._taxonomy_class # pylint: disable=protected-access return self def get_tags( @@ -474,7 +474,7 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if not self.pk: # This is a new instance: # Set _name and _value automatically on creation, if they weren't set: - if self.taxonomy and not self._name: + if not self._name and self.taxonomy: self._name = self.taxonomy.name if not self._value and self.tag: self._value = self.tag.value @@ -534,16 +534,25 @@ def is_deleted(self) -> bool: return self.taxonomy is None or (self.tag is None and not self.taxonomy.allow_free_text) def clean(self): + """ + Validate this ObjectTag. + + Note: this doesn't happen automatically on save(); only when edited in + the django admin. So it's best practice to call obj_tag.full_clean() + before saving. + """ if self.tag: if self.tag.taxonomy_id != self.taxonomy_id: raise ValidationError("ObjectTag's Taxonomy does not match Tag taxonomy") - elif self.tag.value != self._value: + if self.tag.value != self._value: raise ValidationError("ObjectTag's _value is out of sync with Tag.value") else: # Note: self.taxonomy and/or self.tag may be NULL which is OK, because it means the Tag/Taxonomy # was deleted, but we still preserve this _value here in case the Taxonomy or Tag get re-created in future. if self._value == "": raise ValidationError("Invalid _value - empty string") + if self.taxonomy and self.taxonomy.name != self._name: + raise ValidationError("ObjectTag's _name is out of sync with Taxonomy.name") def get_lineage(self) -> Lineage: """ @@ -604,6 +613,6 @@ def copy(self, object_tag: ObjectTag) -> Self: self.tag = object_tag.tag self.taxonomy = object_tag.taxonomy self.object_id = object_tag.object_id - self._value = object_tag._value - self._name = object_tag._name + self._value = object_tag._value # pylint: disable=protected-access + self._name = object_tag._name # pylint: disable=protected-access return self diff --git a/openedx_tagging/core/tagging/models/system_defined.py b/openedx_tagging/core/tagging/models/system_defined.py index 0978ed62..23180f6e 100644 --- a/openedx_tagging/core/tagging/models/system_defined.py +++ b/openedx_tagging/core/tagging/models/system_defined.py @@ -101,8 +101,8 @@ def tag_for_value(self, value: str): # First we look up the instance by value. # We specify 'iexact' but whether it's case sensitive or not on MySQL depends on the model's collation. instance = self.tag_class_model.objects.get(**{f"{self.tag_class_value_field}__iexact": value}) - except ObjectDoesNotExist: - raise Tag.DoesNotExist + except ObjectDoesNotExist as exc: + raise Tag.DoesNotExist from exc # Use the canonical value from here on (possibly with different case from the value given as a parameter) value = getattr(instance, self.tag_class_value_field) # We assume the value may change but the external_id is immutable. @@ -137,8 +137,8 @@ def tag_for_external_id(self, external_id: str): # First we look up the instance by external_id # We specify 'iexact' but whether it's case sensitive or not on MySQL depends on the model's collation. instance = self.tag_class_model.objects.get(**{f"{self.tag_class_key_field}__iexact": external_id}) - except ObjectDoesNotExist: - raise Tag.DoesNotExist + except ObjectDoesNotExist as exc: + raise Tag.DoesNotExist from exc value = getattr(instance, self.tag_class_value_field) # Use the canonical external_id from here on (may differ in capitalization) external_id = getattr(instance, self.tag_class_key_field) @@ -223,14 +223,14 @@ def get_filtered_tags( search_in_all=search_in_all, ) + @classmethod def _get_available_languages(cls) -> set[str]: """ Get available languages from Django LANGUAGE. """ langs = set() for django_lang in settings.LANGUAGES: - # Split to get the language part - langs.add(django_lang[0].split("-")[0]) + langs.add(django_lang[0]) return langs def validate_value(self, value: str): @@ -256,8 +256,9 @@ def validate_external_id(self, external_id: str): Check if 'external_id' is part of this Taxonomy. """ lang_code = external_id.lower() - LANGUAGE_DICT = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) # setting may or may not exist. - return lang_code in LANGUAGE_DICT + # Get settings.LANGUAGES (a list of tuples) as a dict. In LMS/CMS this is already cached as LANGUAGE_DICT + languages_as_dict = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) + return lang_code in languages_as_dict def tag_for_external_id(self, external_id: str): """ @@ -268,11 +269,12 @@ def tag_for_external_id(self, external_id: str): Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. """ lang_code = external_id.lower() - LANGUAGE_DICT = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) # setting may or may not exist. + # Get settings.LANGUAGES (a list of tuples) as a dict. In LMS/CMS this is already cached as LANGUAGE_DICT + languages_as_dict = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) try: - lang_name = LANGUAGE_DICT[lang_code] - except KeyError: - raise Tag.DoesNotExist + lang_name = languages_as_dict[lang_code] + except KeyError as exc: + raise Tag.DoesNotExist from exc tag, _created = self.tag_set.get_or_create(external_id=lang_code, defaults={"value": lang_name}) if tag.value != lang_name: # Update the Tag to reflect the new language name From ecff923f0cf00ee92b89bf9cbb29035d34088321 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 3 Oct 2023 14:25:34 -0700 Subject: [PATCH 12/13] docs: Update system defined taxonomy creation ADR --- .../0012-system-taxonomy-creation.rst | 95 ++++++++++--------- 1 file changed, 51 insertions(+), 44 deletions(-) diff --git a/docs/decisions/0012-system-taxonomy-creation.rst b/docs/decisions/0012-system-taxonomy-creation.rst index 0036432b..c7bb44ff 100644 --- a/docs/decisions/0012-system-taxonomy-creation.rst +++ b/docs/decisions/0012-system-taxonomy-creation.rst @@ -4,9 +4,10 @@ Context -------- -System-defined taxonomies are taxonomies created by the system. Some of these are totally static (e.g Language) -and some depends on a core data model (e.g. Organizations). It is necessary to define how to create and validate -the System-defined taxonomies and their tags. +System-defined taxonomies are taxonomies created by the system. Some of these +depend on Django settings (e.g. Languages) and others depends on a core data +model (e.g. Organizations or Users). It is necessary to define how to create and +validate the System-defined taxonomies and their tags. Decision @@ -15,44 +16,50 @@ Decision System Tag lists and validation ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Each System-defined Taxonomy will have its own ``ObjectTag`` subclass which is used for tag validation (e.g. ``LanguageObjectTag``, ``OrganizationObjectTag``). -Each subclass can overwrite ``get_tags``; to configure the valid tags, and ``is_valid``; to check if a list of tags are valid. Both functions are implemented on the ``ObjectTag`` base class, but can be overwritten to handle special cases. - -We need to create an instance of each System-defined Taxonomy in a fixture. With their respective characteristics and subclasses. -The ``pk`` of these instances must be negative so as not to affect the auto-incremented ``pk`` of Taxonomies. - -Later, we need to create content-side ObjectTags that live on ``openedx.features.content_tagging`` for each content and taxonomy to be used (eg. ``CourseLanguageObjectTag``, ``CourseOrganizationObjectTag``). -This new class is used to configure the automatic content tagging. You can read the `document number 0013`_ to see this configuration. - -Tags creation -~~~~~~~~~~~~~~ - -We have two ways to handle Tags creation and validation for System-defined Taxonomies: - -**Hardcoded by fixtures/migrations** - -#. If the tags don't change over the time, you can create all on a fixture (e.g Languages). - The ``pk`` of these instances must be negative. -#. If the tags change over the time, you can create all on a migration. If you edit, delete, or add new tags, you should also do it in a migration. - -**Dynamic tags** - -Closed Taxonomies that depends on a core data model. Ex. AuthorTaxonomy with Users as Tags - -#. Tags are created on the fly when new ObjectTags are added. -#. Tag.external_id we store an identifier from the instance (eg. User.pk). -#. Tag.value we store a human readable representation of the instance (eg. User.username). -#. Resync the tags to re-fetch the value. - - -Rejected Options ------------------ - -Free-form tags -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -Open Taxonomy that depends on a core data model, but simplifies the creation of Tags by allowing free-form tags, - -Rejected because it has been seen that using dynamic tags provides more functionality and more advantages. - -.. _document number 0013: https://github.com/openedx/openedx-learning/blob/main/docs/decisions/0013-system-taxonomy-auto-tagging.rst +Each Taxonomy has two methods for validating tags: +#. ``validate_value`` +#. ``validate_external_id`` + +These functions will return ``True`` if a given tag is valid, based on its +external ID or value. Subclasses should override these as needed, to implement +different types of taxonomy behavior (e.g. based on a model or baed on Django +settings). + +For example ``validate_value("English")`` will return ``True`` for the language +taxonomy if the English language is enabled in the Django settings. Likewise, +``validate_external_id("en")`` would return true, but +``validate_external_id("zz")`` would be ``False`` because there is no such +language. Or, for a User taxonomy, ``validate_value("username")`` would return +``True`` if a user with that username exists, or ``validate_external_id(...)`` +could validate if a user with that ID exists (note that the ID must be converted +to a string). + +In all of these cases, a ``Tag`` instance may or may not exist in the database. +Before saving an ``ObjectTag`` which references a tag in these taxonomies, the +tagging API will use either ``Taxonomy.tag_for_value`` or +``Taxonomy.tag_for_external_id``. These methods are responsible for both +validating the tag (like ``validate_...``) but also auto-creating the ``Tag`` +instance in case it doesn't already exist. Subclasses should override these as +needed. + +In this way, the system-defined taxonomies are fully dynamic and can represent +tags based on Languages, Users, or Organizations that may exist in large numbers +or be constantly created. + +At present, there isn't a good way to *list* all of the [potential] tags that +exist in a system-defined Taxonomy. We may add an API for that in the future, +for example to list all of the available languages. However for other cases like +users it doesn't make sense to even try to list all of the available tags. So +for now, the assumption is that the UI will not even try to display a list of +available tags for system-defined taxonomies. After all, system-defined tags are +usually applied automatically, rather than a user manually selecting from a +list. If there is a need to show a list of tags to the user, use the API that +lists the actually applied tags - i.e. the values of the ``ObjectTag``s +currently applied to objects using the taxonomy. + +Tags hard-coded by fixtures/migrations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In the future there may be system-defined taxonomies that are not dynamics at +all, where the list of tags are defined by ``Tag`` instances created by a +fixture or migration. However, as of now we don't have a use case for that. From 20cb65ef4a26a67b46dc68eb8c9cbda0f968c8ca Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 3 Oct 2023 14:28:35 -0700 Subject: [PATCH 13/13] chore: version bump: 0.2.0 --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 576d6b42..574c53b5 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.1.8" +__version__ = "0.2.0"