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 + ]