diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 0226308b..f0ed822c 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.7.0" +__version__ = "0.8.0" diff --git a/openedx_tagging/core/tagging/admin.py b/openedx_tagging/core/tagging/admin.py index 4016df6e..0c64f09c 100644 --- a/openedx_tagging/core/tagging/admin.py +++ b/openedx_tagging/core/tagging/admin.py @@ -34,7 +34,7 @@ class ObjectTagAdmin(admin.ModelAdmin): """ fields = ["object_id", "taxonomy", "tag", "_value"] autocomplete_fields = ["tag"] - list_display = ["object_id", "name", "value"] + list_display = ["object_id", "export_id", "value"] readonly_fields = ["object_id"] def has_add_permission(self, request): diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index ff9f0996..6d46334e 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -205,7 +205,7 @@ def get_object_tags( Value("\t"), output_field=models.CharField(), ))) - .annotate(taxonomy_name=Coalesce(F("taxonomy__name"), F("_name"))) + .annotate(taxonomy_name=Coalesce(F("taxonomy__name"), F("_export_id"))) # Sort first by taxonomy name, then by tag value in tree order: .order_by("taxonomy_name", "sort_key") ) @@ -274,11 +274,58 @@ def delete_object_tags(object_id: str): tags.delete() +def _check_new_tag_count( + new_tag_count: int, + taxonomy: Taxonomy | None, + object_id: str, + taxonomy_export_id: str | None = None, +) -> None: + """ + Checks if the new count of tags for the object is equal or less than 100 + """ + # Exclude to avoid counting the tags that are going to be updated + if taxonomy: + current_count = ObjectTag.objects.filter(object_id=object_id).exclude(taxonomy_id=taxonomy.id).count() + else: + current_count = ObjectTag.objects.filter(object_id=object_id).exclude(_export_id=taxonomy_export_id).count() + + if current_count + new_tag_count > 100: + raise ValueError( + _("Cannot add more than 100 tags to ({object_id}).").format(object_id=object_id) + ) + + +def _get_current_tags( + taxonomy: Taxonomy | None, + tags: list[str], + object_id: str, + object_tag_class: type[ObjectTag] = ObjectTag, + taxonomy_export_id: str | None = None, +) -> list[ObjectTag]: + """ + Returns the current object tags of the related object_id with taxonomy + """ + ObjectTagClass = object_tag_class + if taxonomy: + if not taxonomy.allow_multiple and len(tags) > 1: + raise ValueError(_("Taxonomy ({name}) only allows one tag per object.").format(name=taxonomy.name)) + current_tags = list( + ObjectTagClass.objects.filter(taxonomy=taxonomy, object_id=object_id) + ) + else: + current_tags = list( + ObjectTagClass.objects.filter(_export_id=taxonomy_export_id, object_id=object_id) + ) + return current_tags + + def tag_object( object_id: str, - taxonomy: Taxonomy, + taxonomy: Taxonomy | None, tags: list[str], object_tag_class: type[ObjectTag] = ObjectTag, + create_invalid: bool = False, + taxonomy_export_id: str | None = None, ) -> None: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id @@ -292,37 +339,34 @@ def tag_object( 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: - """ - 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( - _("Cannot add more than 100 tags to ({object_id}).").format(object_id=object_id) - ) + create_invalid: You can create invalid tags and avoid the previous behavior using. + taxonomy_export_id: You can create object tags without taxonomy using this param + and `taxonomy` as None. You need to use the taxonomy.export_id, so you can resync + this object tag if the taxonomy is created in the future. + """ if not isinstance(tags, list): raise ValueError(_("Tags must be a list, not {type}.").format(type=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 - _check_new_tag_count(len(tags)) - - if not taxonomy.allow_multiple and len(tags) > 1: - raise ValueError(_("Taxonomy ({name}) only allows one tag per object.").format(name=taxonomy.name)) - - current_tags = list( - ObjectTagClass.objects.filter(taxonomy=taxonomy, object_id=object_id) + if taxonomy: + taxonomy = taxonomy.cast() # Make sure we're using the right subclass. This is a no-op if we are already. + elif not taxonomy_export_id: + raise ValueError("`taxonomy_export_id` can't be None if `taxonomy` is None") + + _check_new_tag_count(len(tags), taxonomy, object_id, taxonomy_export_id) + current_tags = _get_current_tags( + taxonomy, + tags, + object_id, + object_tag_class, + taxonomy_export_id ) + updated_tags = [] - if taxonomy.allow_free_text: + if taxonomy and 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: @@ -334,19 +378,46 @@ def _check_new_tag_count(new_tag_count: int) -> None: else: # Handle closed taxonomies: 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. - object_tag = current_tags.pop(object_tag_index) - 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 # pylint: disable=protected-access + tag = None + # When export, sometimes, the value has a space at the beginning and end. + tag_value = tag_value.strip() + if taxonomy: + try: + tag = taxonomy.tag_for_value(tag_value) # Will raise Tag.DoesNotExist if the value is invalid. + except Tag.DoesNotExist as e: + if not create_invalid: + raise e + + if tag: + # Tag exists in the taxonomy + 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: # pylint: disable=protected-access + # The ObjectTag's cached '_value' is out of sync with the Tag, so update it: + object_tag._value = tag.value # pylint: disable=protected-access + updated_tags.append(object_tag) + else: + # We are newly applying this tag: + object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, tag=tag) updated_tags.append(object_tag) - else: - # We are newly applying this tag: - object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, tag=tag) + elif taxonomy: + # Tag doesn't exist in the taxonomy and `create_invalid` is True + object_tag = ObjectTagClass(taxonomy=taxonomy, object_id=object_id, _value=tag_value) updated_tags.append(object_tag) + else: + # Taxonomy is None (also tag doesn't exist) + if taxonomy_export_id: + # This will always be true, since it is verified at the beginning of the function. + # This condition is placed by the type checks. + object_tag = ObjectTagClass( + taxonomy=None, + object_id=object_id, + _value=tag_value, + _export_id=taxonomy_export_id + ) + updated_tags.append(object_tag) # Save all updated tags at once to avoid partial updates with transaction.atomic(): diff --git a/openedx_tagging/core/tagging/migrations/0016_object_tag_export_id.py b/openedx_tagging/core/tagging/migrations/0016_object_tag_export_id.py new file mode 100644 index 00000000..671cf1a3 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0016_object_tag_export_id.py @@ -0,0 +1,62 @@ +# Generated by Django 3.2.22 on 2024-03-22 19:47 + +import django.db.models.deletion +from django.db import migrations, models + +import openedx_learning.lib.fields + + +def migrate_export_id(apps, schema_editor): + ObjectTag = apps.get_model("oel_tagging", "ObjectTag") + for object_tag in ObjectTag.objects.all(): + if object_tag.taxonomy: + object_tag.export_id = object_tag.taxonomy.export_id + object_tag.save(update_fields=["_export_id"]) + + +def reverse_export_id(apps, schema_editor): + pass + + +def migrate_language_export_id(apps, schema_editor): + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + language_taxonomy = Taxonomy.objects.get(id=-1) + language_taxonomy.export_id = 'languages-v1' + language_taxonomy.save(update_fields=["export_id"]) + + +def reverse_language_export_id(apps, schema_editor): + """ + Return to old export_id + """ + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + language_taxonomy = Taxonomy.objects.get(id=-1) + language_taxonomy.export_id = '-1-languages' + language_taxonomy.save(update_fields=["export_id"]) + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0015_taxonomy_export_id'), + ] + + operations = [ + migrations.RenameField( + model_name='objecttag', + old_name='_name', + new_name='_export_id', + ), + migrations.RunPython(migrate_export_id, reverse_export_id), + migrations.AlterField( + model_name='objecttag', + name='taxonomy', + field=models.ForeignKey(blank=True, default=None, help_text="Taxonomy that this object tag belongs to. Used for validating the tag and provides the tag's 'name' if set.", null=True, on_delete=django.db.models.deletion.SET_NULL, to='oel_tagging.taxonomy'), + ), + migrations.AlterField( + model_name='objecttag', + name='_export_id', + field=openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_unicode_ci', 'sqlite': 'NOCASE'}, help_text='User-facing label used for this tag, stored in case taxonomy is (or becomes) null. If the taxonomy field is set, then taxonomy.export_id takes precedence over this field.', max_length=255), + ), + migrations.RunPython(migrate_language_export_id, reverse_language_export_id), + ] diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index 0235d7b3..3a377587 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -19,7 +19,7 @@ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field, case_sensitive_char_field from ..data import TagDataQuerySet -from .utils import ConcatNull +from .utils import RESERVED_TAG_CHARS, ConcatNull log = logging.getLogger(__name__) @@ -191,9 +191,11 @@ def clean(self): self.value = self.value.strip() if self.external_id: self.external_id = self.external_id.strip() - # Don't allow \t (tab) character at all, as we use it for lineage in database queries - if "\t" in self.value: - raise ValidationError("Tags in a taxonomy cannot contain a TAB character.") + + for reserved_char in RESERVED_TAG_CHARS: + if reserved_char in self.value: + raise ValidationError(f"Tags cannot contain a '{reserved_char}' character.") + if self.external_id and "\t" in self.external_id: raise ValidationError("Tag external ID cannot contain a TAB character.") @@ -775,6 +777,7 @@ class ObjectTag(models.Model): taxonomy = models.ForeignKey( Taxonomy, null=True, + blank=True, default=None, on_delete=models.SET_NULL, help_text=_( @@ -792,11 +795,11 @@ class ObjectTag(models.Model): "Tag associated with this object tag. Provides the tag's 'value' if set." ), ) - _name = case_insensitive_char_field( + _export_id = case_insensitive_char_field( max_length=255, help_text=_( "User-facing label used for this tag, stored in case taxonomy is (or becomes) null." - " If the taxonomy field is set, then taxonomy.name takes precedence over this field." + " If the taxonomy field is set, then taxonomy.export_id takes precedence over this field." ), ) _value = case_insensitive_char_field( @@ -821,9 +824,9 @@ class Meta: 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 not self._name and self.taxonomy: - self._name = self.taxonomy.name + # Set _export_id and _value automatically on creation, if they weren't set: + if not self._export_id and self.taxonomy: + self._export_id = self.taxonomy.export_id if not self._value and self.tag: self._value = self.tag.value @@ -837,24 +840,28 @@ def __str__(self): """ User-facing string representation of an ObjectTag. """ - return f"<{self.__class__.__name__}> {self.object_id}: {self.name}={self.value}" + if self.taxonomy: + name = self.taxonomy.name + else: + name = self.export_id + return f"<{self.__class__.__name__}> {self.object_id}: {name}={self.value}" @property - def name(self) -> str: + def export_id(self) -> str: """ Returns this tag's name/label. If taxonomy is set, then returns its name. - Otherwise, returns the cached _name field. + Otherwise, returns the cached _export_id field. """ - return self.taxonomy.name if self.taxonomy else self._name + return self.taxonomy.export_id if self.taxonomy else self._export_id - @name.setter - def name(self, name: str): + @export_id.setter + def export_id(self, export_id: str): """ - Stores to the _name field. + Stores to the _export_id field. """ - self._name = name + self._export_id = export_id @property def value(self) -> str: @@ -899,8 +906,11 @@ def clean(self): # 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") + for reserved_char in RESERVED_TAG_CHARS: + if reserved_char in self.value: + raise ValidationError(f"Invalid _value - '{reserved_char}' is not allowed") + if self.taxonomy and self.taxonomy.export_id != self._export_id: + raise ValidationError("ObjectTag's _export_id is out of sync with Taxonomy.export_id") if "," in self.object_id or "*" in self.object_id: # Some APIs may use these characters to allow wildcard matches or multiple matches in the future. raise ValidationError("Object ID contains invalid characters") @@ -930,11 +940,18 @@ def resync(self) -> bool: # 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: - self.name = self.taxonomy.name + # Sync the stored _export_id with the taxonomy.name + if self.taxonomy and self._export_id != self.taxonomy.export_id: + self.export_id = self.taxonomy.export_id changed = True + # Sync taxonomy with matching _export_id + if not self.taxonomy: + taxonomy = Taxonomy.objects.filter(export_id=self.export_id).first() + if taxonomy: + self.taxonomy = taxonomy + changed = True + # Closed taxonomies require a tag matching _value if self.taxonomy and not self.taxonomy.allow_free_text and not self.tag_id: tag = self.taxonomy.tag_set.filter(value=self.value).first() @@ -965,5 +982,5 @@ def copy(self, object_tag: ObjectTag) -> Self: self.taxonomy = object_tag.taxonomy self.object_id = object_tag.object_id self._value = object_tag._value # pylint: disable=protected-access - self._name = object_tag._name # pylint: disable=protected-access + self._export_id = object_tag._export_id # pylint: disable=protected-access return self diff --git a/openedx_tagging/core/tagging/models/utils.py b/openedx_tagging/core/tagging/models/utils.py index c2e2201e..86a5f128 100644 --- a/openedx_tagging/core/tagging/models/utils.py +++ b/openedx_tagging/core/tagging/models/utils.py @@ -4,6 +4,16 @@ from django.db.models import Aggregate, CharField from django.db.models.expressions import Func +RESERVED_TAG_CHARS = [ + '\t', # Used in the database to separate tag levels in the "lineage" field + # e.g. lineage="Earth\tNorth America\tMexico\tMexico City" + ' > ', # Used in the search index and Instantsearch frontend to separate tag levels + # e.g. tags_level3="Earth > North America > Mexico > Mexico City" + ';', # Used in CSV exports to separate multiple tags from the same taxonomy + # e.g. languages-v1: en;es;fr +] +TAGS_CSV_SEPARATOR = RESERVED_TAG_CHARS[2] + class ConcatNull(Func): # pylint: disable=abstract-method """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 64299853..354bb434 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -180,10 +180,11 @@ def to_representation(self, instance: list[ObjectTag]) -> dict: tax_entry = next((t for t in taxonomies if t["taxonomy_id"] == obj_tag.taxonomy_id), None) if tax_entry is None: tax_entry = { - "name": obj_tag.name, + "name": obj_tag.taxonomy.name if obj_tag.taxonomy else None, "taxonomy_id": obj_tag.taxonomy_id, "can_tag_object": self._can(can_tag_object_perm, obj_tag), - "tags": [] + "tags": [], + "export_id": obj_tag.export_id, } taxonomies.append(tax_entry) tax_entry["tags"].append(ObjectTagMinimalSerializer(obj_tag, context=self.context).data) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 20057d82..4d398f52 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -23,6 +23,7 @@ get_object_tags, get_taxonomies, get_taxonomy, + resync_object_tags, tag_object, update_tag_in_taxonomy, ) @@ -315,6 +316,8 @@ def create_import(self, request: Request, **_kwargs) -> Response: if import_success: serializer = self.get_serializer(taxonomy) + # We need to resync all object tags because, there may be tags that do not have a taxonomy. + resync_object_tags() return Response(serializer.data, status=status.HTTP_201_CREATED) else: taxonomy.delete() @@ -340,6 +343,8 @@ def update_import(self, request: Request, **_kwargs) -> Response: if import_success: serializer = self.get_serializer(taxonomy) + # We need to resync all object tags because, there may be tags that do not have a taxonomy. + resync_object_tags() return Response(serializer.data) else: return Response(task.log, status=status.HTTP_400_BAD_REQUEST) diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 5a4c64f0..32142c77 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -10,7 +10,7 @@ from django.test import TestCase, override_settings import openedx_tagging.core.tagging.api as tagging_api -from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy from .test_models import TestTagTaxonomyMixin, get_tag from .utils import pretty_format_tags @@ -329,6 +329,73 @@ def test_resync_object_tags(self) -> None: changed = tagging_api.resync_object_tags() assert changed == 0 + def test_resync_object_tags_without_tag(self) -> None: + # Create object tag with an invalid tag + tag_value = "Eukaryota Xenomorph" + tagging_api.tag_object( + object_id="biology101", + taxonomy=self.taxonomy, + tags=[tag_value], + create_invalid=True, + ) + + object_tags = tagging_api.get_object_tags("biology101") + assert len(object_tags) == 0 + + # Create tag + tag = Tag( + taxonomy=self.taxonomy, + value=tag_value, + ) + tag.save() + + # Resync object tags + changed = tagging_api.resync_object_tags() + assert changed == 1 + + object_tags = tagging_api.get_object_tags("biology101") + assert len(object_tags) == 1 + object_tag = object_tags[0] + assert object_tag.taxonomy == self.taxonomy + assert object_tag.tag == tag + + def test_resync_object_tags_without_taxonomy(self) -> None: + # Create object tag with an invalid taxonomy + tag_value = "Eukaryota Xenomorph" + tagging_api.tag_object( + object_id="biology101", + taxonomy=None, + tags=[tag_value], + create_invalid=True, + taxonomy_export_id='taxonomy_test' + ) + + object_tags = tagging_api.get_object_tags("biology101") + assert len(object_tags) == 0 + + # Create taxonomy + new_taxonomy = tagging_api.create_taxonomy( + 'Taxonomy_test', + export_id='taxonomy_test' + ) + new_taxonomy.save() + # Create tag + tag = Tag( + taxonomy=new_taxonomy, + value=tag_value, + ) + tag.save() + + # Resync object tags + changed = tagging_api.resync_object_tags() + assert changed == 1 + + object_tags = tagging_api.get_object_tags("biology101") + assert len(object_tags) == 1 + object_tag = object_tags[0] + assert object_tag.taxonomy == new_taxonomy + assert object_tag.tag == tag + def test_tag_object(self): self.taxonomy.allow_multiple = True @@ -364,7 +431,7 @@ def test_tag_object(self): 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.export_id == self.taxonomy.export_id assert object_tag.object_id == "biology101" def test_tag_object_free_text(self) -> None: @@ -381,7 +448,7 @@ def test_tag_object_free_text(self) -> None: object_tag = object_tags[0] object_tag.full_clean() # Should not raise any ValidationErrors assert object_tag.taxonomy == self.free_text_taxonomy - assert object_tag.name == self.free_text_taxonomy.name + assert object_tag.export_id == self.free_text_taxonomy.export_id assert object_tag._value == "Eukaryota Xenomorph" # pylint: disable=protected-access assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] assert object_tag.object_id == "biology101" @@ -396,6 +463,61 @@ def test_tag_object_invalid_tag(self): tagging_api.tag_object("biology101", self.taxonomy, ["Eukaryota Xenomorph"]) assert "Tag matching query does not exist." in str(excinfo.value) + def test_tag_object_create_without_tag(self): + tagging_api.tag_object( + object_id="biology101", + taxonomy=self.taxonomy, + tags=["Eukaryota Xenomorph"], + create_invalid=True, + ) + object_tags = tagging_api.get_object_tags( + "biology101", + include_deleted=True, + ) + 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.export_id == self.taxonomy.export_id + assert object_tag.tag is None + assert object_tag._value == "Eukaryota Xenomorph" # pylint: disable=protected-access + assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] + assert object_tag.object_id == "biology101" + + def test_tag_object_create_without_taxonomy(self): + tagging_api.tag_object( + object_id="biology101", + taxonomy=None, + tags=["Eukaryota Xenomorph"], + create_invalid=True, + taxonomy_export_id='test_taxonomy' + ) + object_tags = tagging_api.get_object_tags( + "biology101", + include_deleted=True, + ) + assert len(object_tags) == 1 + object_tag = object_tags[0] + object_tag.full_clean() # Should not raise any ValidationErrors + assert object_tag.taxonomy is None + assert object_tag.export_id == 'test_taxonomy' + assert object_tag.tag is None + assert object_tag._value == "Eukaryota Xenomorph" # pylint: disable=protected-access + assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] + assert object_tag.object_id == "biology101" + + def test_tag_object_taxonomy_export_error(self): + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + object_id="biology101", + taxonomy=None, + tags=["Eukaryota Xenomorph"], + create_invalid=True, + taxonomy_export_id=None + ) + assert exc.exception + assert "`taxonomy_export_id` can't be None if `taxonomy` is None" in str(exc.exception) + def test_tag_object_string(self) -> None: with self.assertRaises(ValueError) as exc: tagging_api.tag_object( @@ -505,7 +627,7 @@ def test_tag_object_language_taxonomy(self) -> None: 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.export_id == self.language_taxonomy.export_id assert object_tag.object_id == "biology101" @override_settings(LANGUAGES=test_languages) @@ -538,7 +660,7 @@ def test_tag_object_model_system_taxonomy(self) -> None: assert object_tag.tag.value == user.username assert not object_tag.is_deleted assert object_tag.taxonomy == self.user_taxonomy - assert object_tag.name == self.user_taxonomy.name + assert object_tag.export_id == self.user_taxonomy.export_id assert object_tag.object_id == "biology101" def test_tag_object_model_system_taxonomy_invalid(self) -> None: @@ -603,15 +725,15 @@ def test_get_object_tags_deleted_disabled(self) -> None: tagging_api.tag_object(object_id=obj_id, taxonomy=disabled_taxonomy, tags=["disabled tag"]) def get_object_tags(): - return [f"{ot.name}: {'>'.join(ot.get_lineage())}" for ot in tagging_api.get_object_tags(obj_id)] + return [f"{ot.export_id}: {'>'.join(ot.get_lineage())}" for ot in tagging_api.get_object_tags(obj_id)] # Before deleting/disabling: assert get_object_tags() == [ - "Disabled Taxonomy: disabled tag", - "Free Text: has a notochord", - "Languages: English", - "Life on Earth: Archaea>DPANN", - "Life on Earth: Eukaryota>Animalia>Chordata", + "7-disabled-taxonomy: disabled tag", + "6-free-text: has a notochord", + "languages-v1: English", + "life_on_earth: Archaea>DPANN", + "life_on_earth: Eukaryota>Animalia>Chordata" ] # Now delete and disable things: @@ -622,8 +744,8 @@ def get_object_tags(): # Now retrieve the tags again: assert get_object_tags() == [ - "Languages: English", - "Life on Earth: Eukaryota>Animalia>Chordata", + "languages-v1: English", + "life_on_earth: Eukaryota>Animalia>Chordata", ] @ddt.data( diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 76bfb312..f17f56ce 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -13,6 +13,7 @@ from openedx_tagging.core.tagging import api from openedx_tagging.core.tagging.models import LanguageTaxonomy, ObjectTag, Tag, Taxonomy +from openedx_tagging.core.tagging.models.utils import RESERVED_TAG_CHARS from .utils import pretty_format_tags @@ -119,7 +120,7 @@ def create_100_taxonomies(self): ObjectTag.objects.create( object_id="limit_tag_count", taxonomy=taxonomy, - _name=taxonomy.name, + _export_id=taxonomy.export_id, _value="Dummy Tag", ) dummy_taxonomies.append(taxonomy) @@ -257,16 +258,13 @@ def test_trailing_whitespace(self): t2 = api.add_tag_to_taxonomy(self.taxonomy, "\t value\n") assert t2.value == "value" - def test_no_tab(self): - """ - Test that tags cannot contain a TAB character, which we use as a field - separator in the database when computing lineage. - """ - with pytest.raises(ValidationError): - self.taxonomy.add_tag("has\ttab") - # And via the API: - with pytest.raises(ValidationError): - api.add_tag_to_taxonomy(self.taxonomy, "first\tsecond") + def test_reserved_chars(self): + for reserved_char in RESERVED_TAG_CHARS: + with pytest.raises(ValidationError): + self.taxonomy.add_tag(f"tag 1 {reserved_char} tag 2") + # And via the API: + with pytest.raises(ValidationError): + api.add_tag_to_taxonomy(self.taxonomy, f"tag 3 {reserved_char} tag 4") @ddt.data( ("test"), @@ -725,20 +723,20 @@ def test_cast(self): == " object:id:1: Life on Earth=Bacteria" ) - def test_object_tag_name(self): - # ObjectTag's name defaults to its taxonomy's name - assert self.object_tag.name == self.taxonomy.name + def test_object_tag_export_id(self): + # ObjectTag's export_id defaults to its taxonomy's export_id + assert self.object_tag.export_id == self.taxonomy.export_id - # Even if we overwrite the name, it still uses the taxonomy's name - self.object_tag.name = "Another tag" - assert self.object_tag.name == self.taxonomy.name + # Even if we overwrite the export_id, it still uses the taxonomy's export_id + self.object_tag.export_id = "another-taxonomy" + assert self.object_tag.export_id == self.taxonomy.export_id self.object_tag.save() - assert self.object_tag.name == self.taxonomy.name + assert self.object_tag.export_id == self.taxonomy.export_id - # But if the taxonomy is deleted, then the object_tag's name reverts to our cached name + # But if the taxonomy is deleted, then the object_tag's export_id reverts to our cached export_id self.taxonomy.delete() self.object_tag.refresh_from_db() - assert self.object_tag.name == "Another tag" + assert self.object_tag.export_id == "another-taxonomy" def test_object_tag_value(self): # ObjectTag's value defaults to its tag's value @@ -799,7 +797,7 @@ def test_validate_value_closed(self): with pytest.raises(api.TagDoesNotExist): self.taxonomy.tag_for_value("Foobarensia") - def test_clean(self): + def test_clean_tag_in_taxonomy(self): # ObjectTags in a closed taxonomy require a tag in that taxonomy object_tag = ObjectTag(taxonomy=self.taxonomy, tag=Tag.objects.create( taxonomy=self.system_taxonomy, # Different taxonomy @@ -811,6 +809,23 @@ def test_clean(self): object_tag._value = self.tag.value # pylint: disable=protected-access object_tag.full_clean() + def test_clean_invalid_value(self): + object_tag = ObjectTag(taxonomy=self.taxonomy, _value="") + with self.assertRaises(ValidationError) as exc: + object_tag.full_clean() + assert exc.exception + assert "Invalid _value - empty string" in str(exc.exception) + + for reserved_char in RESERVED_TAG_CHARS: + object_tag = ObjectTag(taxonomy=self.taxonomy, _value=f"tag 1 {reserved_char} tag 2") + with self.assertRaises(ValidationError) as exc: + object_tag.full_clean() + assert exc.exception + assert f"Invalid _value - '{reserved_char}' it's not allowed" in str(exc.exception) + + object_tag = ObjectTag(taxonomy=self.taxonomy, _value="tag 1") + object_tag.full_clean() + def test_tag_case(self) -> None: """ Test that the object_id is case sensitive. diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index a3664e27..682535b4 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -175,7 +175,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "can_change_taxonomy": False, "can_delete_taxonomy": False, "can_tag_object": False, - "export_id": "-1-languages", + "export_id": "languages-v1", }, { "id": taxonomy.id, @@ -298,7 +298,7 @@ def test_language_taxonomy(self): can_change_taxonomy=False, can_delete_taxonomy=False, can_tag_object=False, - export_id='-1-languages', + export_id='languages-v1', ) @ddt.data( @@ -774,7 +774,8 @@ def test_retrieve_object_tags(self, user_attr, expected_status): "lineage": ["Eukaryota", "Fungi"], "can_delete_objecttag": True, }, - ] + ], + "export_id": "life_on_earth" }, { "name": "User Authors", @@ -787,6 +788,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): "can_delete_objecttag": True, }, ], + "export_id": "user_authors" } ], }, @@ -937,6 +939,7 @@ def test_retrieve_object_tags_taxonomy_queryparam( "can_delete_objecttag": True, }, ], + "export_id": "user_authors", } ], },