From 6f8556ea9084f89b7f75d9df8268e482c7dae977 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 22:17:02 +0930 Subject: [PATCH 01/14] style: adds type annotations to rules --- openedx_tagging/core/tagging/rules.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index 7ef984b86..72b9fa6b4 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -1,6 +1,12 @@ """Django rules-based permissions for tagging""" import rules +from django.contrib.auth import get_user_model + +from .models import ObjectTag, Tag, Taxonomy + +User = get_user_model() + # Global staff are taxonomy admins. # (Superusers can already do anything) @@ -8,7 +14,7 @@ @rules.predicate -def can_view_taxonomy(user, taxonomy=None): +def can_view_taxonomy(user: User, taxonomy: Taxonomy = None) -> bool: """ Anyone can view an enabled taxonomy, but only taxonomy admins can view a disabled taxonomy. @@ -17,7 +23,7 @@ def can_view_taxonomy(user, taxonomy=None): @rules.predicate -def can_change_taxonomy(user, taxonomy=None): +def can_change_taxonomy(user: User, taxonomy: Taxonomy = None) -> bool: """ Even taxonomy admins cannot change system taxonomies. """ @@ -27,7 +33,7 @@ def can_change_taxonomy(user, taxonomy=None): @rules.predicate -def can_change_taxonomy_tag(user, tag=None): +def can_change_taxonomy_tag(user: User, tag: Tag = None) -> bool: """ Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies (these don't have predefined tags). @@ -44,7 +50,7 @@ def can_change_taxonomy_tag(user, tag=None): @rules.predicate -def can_change_object_tag(user, object_tag=None): +def can_change_object_tag(user: User, object_tag: ObjectTag = None) -> bool: """ Taxonomy admins can create or modify object tags on enabled taxonomies. """ From 35da04b5ef099e2c489e881ae00b1ca0ed2a5bf1 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 5 Jul 2023 14:23:52 +0930 Subject: [PATCH 02/14] feat: adds Taxonomy.system_defined field so that system taxonomies can store this field and ensure no one edits them. --- openedx_tagging/core/tagging/api.py | 2 ++ .../0002_taxonomy_system_defined.py | 20 +++++++++++++++ openedx_tagging/core/tagging/models.py | 25 ++++++++++++------- .../core/fixtures/tagging.yaml | 11 ++++++++ .../openedx_tagging/core/tagging/test_api.py | 23 +++++++++++------ .../core/tagging/test_models.py | 13 ++++++++-- .../core/tagging/test_rules.py | 10 +++----- 7 files changed, 78 insertions(+), 26 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 6b9bf2a04..89e0fb8c2 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -25,6 +25,7 @@ def create_taxonomy( required=False, allow_multiple=False, allow_free_text=False, + system_defined=False, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. @@ -36,6 +37,7 @@ def create_taxonomy( required=required, allow_multiple=allow_multiple, allow_free_text=allow_free_text, + system_defined=system_defined, ) diff --git a/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py b/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py new file mode 100644 index 000000000..6f1beb92e --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py @@ -0,0 +1,20 @@ +# Generated by Django 3.2.19 on 2023-07-05 04:52 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="taxonomy", + name="system_defined", + field=models.BooleanField( + default=False, + help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", + ), + ), + ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index c90f8a34a..640dc788b 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -6,7 +6,6 @@ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field - # Maximum depth allowed for a hierarchical taxonomy's tree of tags. TAXONOMY_MAX_DEPTH = 3 @@ -73,7 +72,7 @@ def __str__(self): """ User-facing string representation of a Tag. """ - return f"Tag ({self.id}) {self.value}" + return f"<{self.__class__.__name__}> ({self.id}) {self.value}" def get_lineage(self) -> Lineage: """ @@ -136,20 +135,28 @@ class Taxonomy(models.Model): "Indicates that tags in this taxonomy need not be predefined; authors may enter their own tag values." ), ) + system_defined = models.BooleanField( + default=False, + help_text=_( + "Indicates that tags and metadata for this taxonomy are maintained by the system;" + " taxonomy admins will not be permitted to modify them.", + ), + ) class Meta: verbose_name_plural = "Taxonomies" - @property - def system_defined(self) -> bool: + def __repr__(self): """ - Base taxonomies are user-defined, not system-defined. - - System-defined taxonomies cannot be edited by ordinary users. + Developer-facing representation of a Taxonomy. + """ + return str(self) - Subclasses should override this property as required. + def __str__(self): + """ + User-facing string representation of a Taxonomy. """ - return False + return f"<{self.__class__.__name__}> ({self.id}) {self.name}" def get_tags(self) -> List[Tag]: """ diff --git a/tests/openedx_tagging/core/fixtures/tagging.yaml b/tests/openedx_tagging/core/fixtures/tagging.yaml index 50b9459bb..a56ece81a 100644 --- a/tests/openedx_tagging/core/fixtures/tagging.yaml +++ b/tests/openedx_tagging/core/fixtures/tagging.yaml @@ -154,3 +154,14 @@ required: false allow_multiple: false allow_free_text: false + system_defined: false +- model: oel_tagging.taxonomy + pk: 2 + fields: + name: System Languages + description: Allows tags for any language configured for use on the instance. + enabled: true + required: false + allow_multiple: false + allow_free_text: false + system_defined: true diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 7a418e687..58f1ed4ba 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -23,6 +23,7 @@ def test_create_taxonomy(self): "required": True, "allow_multiple": True, "allow_free_text": True, + "system_defined": True, } taxonomy = tagging_api.create_taxonomy(**params) for param, value in params.items(): @@ -31,14 +32,20 @@ def test_create_taxonomy(self): def test_get_taxonomies(self): tax1 = tagging_api.create_taxonomy("Enabled") tax2 = tagging_api.create_taxonomy("Disabled", enabled=False) - enabled = tagging_api.get_taxonomies() - assert list(enabled) == [tax1, self.taxonomy] - - disabled = tagging_api.get_taxonomies(enabled=False) - assert list(disabled) == [tax2] - - both = tagging_api.get_taxonomies(enabled=None) - assert list(both) == [tax2, tax1, self.taxonomy] + with self.assertNumQueries(1): + enabled = list(tagging_api.get_taxonomies()) + assert enabled == [tax1, self.taxonomy, self.system_taxonomy] + assert str(enabled[0]) == " (3) Enabled" + assert str(enabled[1]) == " (1) Life on Earth" + assert str(enabled[2]) == " (2) System Languages" + + with self.assertNumQueries(1): + disabled = list(tagging_api.get_taxonomies(enabled=False)) + assert disabled == [tax2] + + with self.assertNumQueries(1): + both = list(tagging_api.get_taxonomies(enabled=None)) + assert both == [tax2, tax1, self.taxonomy, self.system_taxonomy] def test_get_tags(self): self.setup_tag_depths() diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index d74c041db..098a4ee01 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -23,6 +23,7 @@ class TestTagTaxonomyMixin: def setUp(self): super().setUp() self.taxonomy = Taxonomy.objects.get(name="Life on Earth") + self.system_taxonomy = Taxonomy.objects.get(name="System Languages") self.archaea = get_tag("Archaea") self.archaebacteria = get_tag("Archaebacteria") self.bacteria = get_tag("Bacteria") @@ -85,10 +86,18 @@ class TestModelTagTaxonomy(TestTagTaxonomyMixin, TestCase): def test_system_defined(self): assert not self.taxonomy.system_defined + assert self.system_taxonomy.system_defined def test_representations(self): - assert str(self.bacteria) == "Tag (1) Bacteria" - assert repr(self.bacteria) == "Tag (1) Bacteria" + assert ( + str(self.taxonomy) == repr(self.taxonomy) == " (1) Life on Earth" + ) + assert ( + str(self.system_taxonomy) + == repr(self.system_taxonomy) + == " (2) System Languages" + ) + assert str(self.bacteria) == repr(self.bacteria) == " (1) Bacteria" @ddt.data( # Root tags just return their own value diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index 2c2b4e748..ecd453ebb 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -3,7 +3,6 @@ import ddt from django.contrib.auth import get_user_model from django.test.testcases import TestCase -from mock import Mock from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy @@ -64,12 +63,9 @@ def test_add_change_taxonomy(self, perm): ) def test_system_taxonomy(self, perm): """Taxonomy administrators cannot edit system taxonomies""" - # TODO: use SystemTaxonomy when available - system_taxonomy = Mock(spec=Taxonomy) - system_taxonomy.system_defined.return_value = True - assert self.superuser.has_perm(perm, system_taxonomy) - assert not self.staff.has_perm(perm, system_taxonomy) - assert not self.learner.has_perm(perm, system_taxonomy) + assert self.superuser.has_perm(perm, self.system_taxonomy) + assert not self.staff.has_perm(perm, self.system_taxonomy) + assert not self.learner.has_perm(perm, self.system_taxonomy) @ddt.data( True, From b94143f11652fde2b07a9a907436bc91f85c82b5 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 4 Jul 2023 16:43:44 +0930 Subject: [PATCH 03/14] refactor: moves validation to ObjectTag and subclasses allowing ObjectTags to be subclassed to validate themselves with different taxonomies. --- openedx_tagging/core/tagging/api.py | 64 +++-- .../migrations/0003_objecttag_proxies.py | 41 ++++ openedx_tagging/core/tagging/models.py | 220 ++++++++++++++---- .../openedx_tagging/core/tagging/test_api.py | 122 ++++++++-- .../core/tagging/test_models.py | 133 +++++++---- .../core/tagging/test_rules.py | 5 +- 6 files changed, 456 insertions(+), 129 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 89e0fb8c2..30bc39153 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -10,27 +10,28 @@ Please look at the models.py file for more information about the kinds of data are stored in this app. """ -from typing import List, Type +from typing import Generator, List, Type from django.db.models import QuerySet from django.utils.translation import gettext_lazy as _ -from .models import ObjectTag, Tag, Taxonomy +from .models import ClosedObjectTag, ObjectTag, OpenObjectTag, Tag, Taxonomy def create_taxonomy( - name, - description=None, + name: str, + description: str = None, enabled=True, required=False, allow_multiple=False, allow_free_text=False, system_defined=False, + object_tag_class: Type = None, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. """ - return Taxonomy.objects.create( + taxonomy = Taxonomy( name=name, description=description, enabled=enabled, @@ -39,6 +40,10 @@ def create_taxonomy( allow_free_text=allow_free_text, system_defined=system_defined, ) + if object_tag_class: + taxonomy.object_tag_class = object_tag_class + taxonomy.save() + return taxonomy def get_taxonomies(enabled=True) -> QuerySet: @@ -62,6 +67,22 @@ def get_tags(taxonomy: Taxonomy) -> List[Tag]: return taxonomy.get_tags() +def cast_object_tag( + object_tag: ObjectTag, default_class=OpenObjectTag +) -> OpenObjectTag: + """ + Casts/copies the given object tag data into the ObjectTag subclass appropriate for this tag. + + E.g. if the tag's taxonomy has a custom ObjectTag class it prefers, use that. + Recommends using OpenObjectTag by default, because that is the least restrictive type. + """ + ObjectTagClass = default_class + if object_tag.taxonomy_id: + ObjectTagClass = object_tag.taxonomy.object_tag_class + new_object_tag = ObjectTagClass().copy(object_tag) + return new_object_tag + + def resync_object_tags(object_tags: QuerySet = None) -> int: """ Reconciles ObjectTag entries with any changes made to their associated taxonomies and tags. @@ -69,7 +90,7 @@ def resync_object_tags(object_tags: QuerySet = None) -> int: By default, we iterate over all ObjectTags. Pass a filtered ObjectTags queryset to limit which tags are resynced. """ if not object_tags: - object_tags = ObjectTag.objects.all() + object_tags = ObjectTag.objects.select_related("tag", "taxonomy") num_changed = 0 for object_tag in object_tags: @@ -81,18 +102,33 @@ def resync_object_tags(object_tags: QuerySet = None) -> int: def get_object_tags( - taxonomy: Taxonomy, object_id: str, object_type: str, valid_only=True -) -> List[ObjectTag]: + object_id: str, object_type: str = None, taxonomy: Taxonomy = None, valid_only=True +) -> Generator[ObjectTag, None, None]: """ - Returns a list of tags for a given taxonomy + content. + Generates a list of object tags for a given object. + + Pass taxonomy to limit the returned object_tags to a specific taxonomy. Pass valid_only=False when displaying tags to content authors, so they can see invalid tags too. - Invalid tags will likely be hidden from learners. + Invalid tags will (probably) be hidden from learners. """ - tags = ObjectTag.objects.filter( - taxonomy=taxonomy, object_id=object_id, object_type=object_type - ).order_by("id") - return [tag for tag in tags if not valid_only or taxonomy.validate_object_tag(tag)] + tags = ( + ObjectTag.objects.filter( + object_id=object_id, + ) + .select_related("tag", "taxonomy") + .order_by("id") + ) + if object_type: + tags = tags.filter(object_type=object_type) + if taxonomy: + tags = tags.filter(taxonomy=taxonomy) + + for tag in tags: + # We can only validate tags with taxonomies, because we need the object_tag_class + object_tag = cast_object_tag(tag) + if not valid_only or object_tag.is_valid(): + yield object_tag def tag_object( diff --git a/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py b/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py new file mode 100644 index 000000000..faaaa66dc --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py @@ -0,0 +1,41 @@ +# Generated by Django 3.2.19 on 2023-07-05 05:07 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0002_taxonomy_system_defined"), + ] + + operations = [ + migrations.CreateModel( + name="OpenObjectTag", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("oel_tagging.objecttag",), + ), + migrations.AddField( + model_name="taxonomy", + name="_object_tag_class", + field=models.CharField( + help_text="Overrides the default ObjectTag subclass associated with this taxonomy.Must be a fully-qualified module and class name.", + max_length=255, + null=True, + ), + ), + migrations.CreateModel( + name="ClosedObjectTag", + fields=[], + options={ + "proxy": True, + "indexes": [], + "constraints": [], + }, + bases=("oel_tagging.openobjecttag",), + ), + ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index 640dc788b..202c7776b 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -2,6 +2,7 @@ from typing import List, Type from django.db import models +from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field @@ -94,7 +95,7 @@ def get_lineage(self) -> Lineage: class Taxonomy(models.Model): """ - Represents a namespace and rules for a group of tags which can be applied to a particular Open edX object. + Represents a namespace and rules for a group of tags. """ id = models.BigAutoField(primary_key=True) @@ -142,6 +143,14 @@ class Taxonomy(models.Model): " taxonomy admins will not be permitted to modify them.", ), ) + _object_tag_class = models.CharField( + null=True, + max_length=255, + help_text=_( + "Overrides the default ObjectTag subclass associated with this taxonomy." + "Must be a fully-qualified module and class name.", + ), + ) class Meta: verbose_name_plural = "Taxonomies" @@ -158,6 +167,39 @@ def __str__(self): """ return f"<{self.__class__.__name__}> ({self.id}) {self.name}" + @property + def object_tag_class(self) -> Type: + """ + Returns the ObjectTag subclass associated with this taxonomy. + + May raise ImportError if a custom object_tag_class cannot be imported. + """ + if self._object_tag_class: + ObjectTagClass = import_string(self._object_tag_class) + elif self.allow_free_text: + ObjectTagClass = OpenObjectTag + else: + ObjectTagClass = ClosedObjectTag + + return ObjectTagClass + + @object_tag_class.setter + def object_tag_class(self, object_tag_class: Type): + """ + Assigns the given object_tag_class's module path.class to the field. + + Raises ValueError if the given `object_tag_class` is a built-in class; it should be an ObjectTag-like class. + """ + if object_tag_class.__module__ == "builtins": + raise ValueError( + f"object_tag_class {object_tag_class} must be class like ObjectTag" + ) + + # ref: https://stackoverflow.com/a/2020083 + self._object_tag_class = ".".join( + [object_tag_class.__module__, object_tag_class.__qualname__] + ) + def get_tags(self) -> List[Tag]: """ Returns a list of all Tags in the current taxonomy, from the root(s) down to TAXONOMY_MAX_DEPTH tags, in tree order. @@ -192,39 +234,6 @@ def get_tags(self) -> List[Tag]: break return tags - def validate_object_tag( - self, - object_tag: "ObjectTag", - check_taxonomy=True, - check_tag=True, - check_object=True, - ) -> bool: - """ - Returns True if the given object tag is valid for the current Taxonomy. - - Subclasses can override this method 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. - """ - # Must be linked to this taxonomy - if check_taxonomy and ( - not object_tag.taxonomy_id or object_tag.taxonomy_id != self.id - ): - return False - - # Must be linked to a Tag unless its a free-text taxonomy - if check_tag and (not self.allow_free_text and not object_tag.tag_id): - return False - - # Must have a valid object id/type: - if check_object and (not object_tag.object_id or not object_tag.object_type): - return False - - return True - def tag_object( self, tags: List, object_id: str, object_type: str ) -> List["ObjectTag"]: @@ -246,6 +255,7 @@ def tag_object( _(f"Taxonomy ({self.id}) requires at least one tag per object.") ) + ObjectTagClass = self.object_tag_class current_tags = { tag.tag_ref: tag for tag in ObjectTag.objects.filter( @@ -255,9 +265,9 @@ def tag_object( updated_tags = [] for tag_ref in tags: if tag_ref in current_tags: - object_tag = current_tags.pop(tag_ref) + object_tag = ObjectTagClass().copy(current_tags.pop(tag_ref)) else: - object_tag = ObjectTag( + object_tag = ObjectTagClass( taxonomy=self, object_id=object_id, object_type=object_type, @@ -273,7 +283,7 @@ def tag_object( object_tag.value = tag_ref object_tag.resync() - if not self.validate_object_tag(object_tag): + if not object_tag.is_valid(): raise ValueError( _(f"Invalid object tag for taxonomy ({self.id}): {tag_ref}") ) @@ -358,6 +368,18 @@ class Meta: models.Index(fields=["taxonomy", "_value"]), ] + def __repr__(self): + """ + Developer-facing representation of an ObjectTag. + """ + return str(self) + + def __str__(self): + """ + User-facing string representation of an ObjectTag. + """ + return f"<{self.__class__.__name__}> {self.object_id} ({self.object_type}): {self.name}={self.value}" + @property def name(self) -> str: """ @@ -402,14 +424,18 @@ def tag_ref(self) -> str: """ return self.tag.id if self.tag_id else self._value - @property - def is_valid(self) -> bool: + def copy(self, object_tag: "ObjectTag") -> "ObjectTag": """ - 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. + Copy the fields from the given ObjectTag into the current instance. """ - return self.taxonomy_id and self.taxonomy.validate_object_tag(self) + self.id = object_tag.id + self.object_id = object_tag.object_id + self.object_type = object_tag.object_type + self.taxonomy_id = object_tag.taxonomy_id + self.tag_id = object_tag.tag_id + self._name = object_tag.name + self._value = object_tag.value + return self def get_lineage(self) -> Lineage: """ @@ -420,6 +446,19 @@ def get_lineage(self) -> Lineage: """ return self.tag.get_lineage() if self.tag_id else [self._value] + def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: + """ + Always returns False -- use subclasses to find out whether this ObjectTag is valid for the linked taxonomy and/or tag. + + Subclasses must override this method to perform the proper validation checks, e.g. closed vs open taxonomies, + dynamically generated tag lists or object definitions. + + 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. + """ + return False + def resync(self) -> bool: """ Reconciles the stored ObjectTag properties with any changes made to its associated taxonomy or tag. @@ -429,6 +468,8 @@ def resync(self) -> bool: It's also useful for a set of ObjectTags are imported from an external source prior to when a Taxonomy exists to validate or store its available Tags. + Subclasses may override this method to perform any additional syncing for the particular type of object tag. + Returns True if anything was changed, False otherwise. """ changed = False @@ -437,21 +478,23 @@ def resync(self) -> bool: if not self.taxonomy_id: for taxonomy in Taxonomy.objects.filter(name=self.name, enabled=True): # Make sure this taxonomy will accept object tags like this. - self.taxonomy = taxonomy - if taxonomy.validate_object_tag(self, check_tag=False): + object_tag = taxonomy.object_tag_class().copy(self) + object_tag.taxonomy = taxonomy + if object_tag.is_valid( + check_taxonomy=True, check_tag=False, check_object=False + ): + self.taxonomy = taxonomy changed = True break # If not, try the next one - else: - self.taxonomy = None # Sync the stored _name with the taxonomy.name - elif self._name != self.taxonomy.name: + if self.taxonomy_id and self._name != self.taxonomy.name: self.name = self.taxonomy.name changed = True - # Locate a tag matching _value - if self.taxonomy and not self.tag_id and not self.taxonomy.allow_free_text: + # Closed taxonomies require a tag matching _value + if self.taxonomy and not self.taxonomy.allow_free_text and not self.tag_id: tag = self.taxonomy.tag_set.filter(value=self.value).first() if tag: self.tag = tag @@ -463,3 +506,80 @@ def resync(self) -> bool: changed = True return changed + + +class OpenObjectTag(ObjectTag): + """ + Free-text object tag. + + Only needs a free-text taxonomy and a value to be valid. + """ + + class Meta: + proxy = True + + def _check_object(self): + """ + Returns True if this ObjectTag has a valid object. + + Subclasses should override this method to perform any additional validation for the particular type of object tag. + """ + # Must have a valid object id/type: + return self.object_id and self.object_type + + def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: + """ + Returns True if this ObjectTag is valid for use with a free-text taxonomy. + + Subclasses should override this method to perform any additional validation for the particular type of object tag. + + 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. + """ + # Must be linked to a free-text taxonomy + if check_taxonomy and ( + not self.taxonomy_id or not self.taxonomy.allow_free_text + ): + return False + + # Open taxonomies don't need an associated tag, but we need a value. + if check_tag and not self.value: + return False + + if check_object and not self._check_object(): + return False + + return True + + +class ClosedObjectTag(OpenObjectTag): + """ + Object tags linked to a closed taxonomy, where the available tag value options are known. + """ + + class Meta: + proxy = True + + def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: + """ + Returns True if this ObjectTag is valid for use with a closed taxonomy. + + Subclasses should override this method to perform any additional validation for the particular type of object tag. + + 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. + """ + # Must be linked to a closed taxonomy + if check_taxonomy and (not self.taxonomy_id or self.taxonomy.allow_free_text): + return False + + # Closed taxonomies require a Tag + if check_tag and not self.tag_id: + return False + + if check_object and not self._check_object(): + return False + + return True diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 58f1ed4ba..7827e7978 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -1,6 +1,6 @@ """ Test the tagging APIs """ -from unittest.mock import patch +from unittest.mock import Mock, patch from django.test.testcases import TestCase @@ -29,6 +29,16 @@ def test_create_taxonomy(self): for param, value in params.items(): assert getattr(taxonomy, param) == value + def test_create_taxonomy_bad_object_tag_class(self): + with self.assertRaises(ValueError) as exc: + tagging_api.create_taxonomy( + name="invalid", + object_tag_class=str, + ) + assert "object_tag_class must be class like ObjectTag" in str( + exc.exception + ) + def test_get_taxonomies(self): tax1 = tagging_api.create_taxonomy("Enabled") tax2 = tagging_api.create_taxonomy("Disabled", enabled=False) @@ -42,6 +52,7 @@ def test_get_taxonomies(self): with self.assertNumQueries(1): disabled = list(tagging_api.get_taxonomies(enabled=False)) assert disabled == [tax2] + assert str(disabled[0]) == " (4) Disabled" with self.assertNumQueries(1): both = list(tagging_api.get_taxonomies(enabled=None)) @@ -66,11 +77,13 @@ def check_object_tag(self, object_tag, taxonomy, tag, name, value): assert object_tag.value == value def test_resync_object_tags(self): - missing_links = ObjectTag(object_id="abc", object_type="alpha") - missing_links.name = self.taxonomy.name - missing_links.value = self.mammalia.value - missing_links.save() - changed_links = ObjectTag( + missing_links = ObjectTag.objects.create( + object_id="abc", + object_type="alpha", + _name=self.taxonomy.name, + _value=self.mammalia.value, + ) + changed_links = ObjectTag.objects.create( object_id="def", object_type="alpha", taxonomy=self.taxonomy, @@ -79,8 +92,7 @@ def test_resync_object_tags(self): changed_links.name = "Life" changed_links.value = "Animals" changed_links.save() - - no_changes = ObjectTag( + no_changes = ObjectTag.objects.create( object_id="ghi", object_type="beta", taxonomy=self.taxonomy, @@ -118,21 +130,30 @@ def test_resync_object_tags(self): assert changed == 0 # Recreate the taxonomy and resync some tags - first_taxonomy = tagging_api.create_taxonomy("Life on Earth") + 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, ) + # Patch so that open taxonomy object tags won't validate, + # demonstrating that the resync logic will move on to the next taxonomy. with patch( - "openedx_tagging.core.tagging.models.Taxonomy.validate_object_tag", - side_effect=[False, True, False, True], - ): + "openedx_tagging.core.tagging.models.OpenObjectTag", + ) as mock_open_object_tag: + mock_open_object_tag.return_value = Mock() + mock_open_object_tag().copy.return_value = Mock() + mock_open_object_tag().copy().is_valid.return_value = False + changed = tagging_api.resync_object_tags( ObjectTag.objects.filter(object_type="alpha") ) assert changed == 2 + mock_open_object_tag().copy().is_valid.assert_called() + for object_tag in (missing_links, changed_links): self.check_object_tag( object_tag, second_taxonomy, new_tag, "Life on Earth", "Mammalia" @@ -179,10 +200,12 @@ def test_tag_object(self): # Ensure the expected number of tags exist in the database assert ( - tagging_api.get_object_tags( - taxonomy=self.taxonomy, - object_id="biology101", - object_type="course", + list( + tagging_api.get_object_tags( + taxonomy=self.taxonomy, + object_id="biology101", + object_type="course", + ) ) == object_tags ) @@ -190,8 +213,73 @@ def test_tag_object(self): 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.is_valid() assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" assert object_tag.object_type == "course" + + def test_get_object_tags(self): + # Alpha tag has no taxonomy + alpha = ObjectTag(object_id="abc", object_type="alpha") + alpha.name = self.taxonomy.name + alpha.value = self.mammalia.value + alpha.save() + # Beta tag has a closed taxonomy + beta = ObjectTag.objects.create( + object_id="abc", + object_type="beta", + taxonomy=self.taxonomy, + ) + beta = tagging_api.cast_object_tag(beta) + + # Fetch all the tags for a given object ID + assert list( + tagging_api.get_object_tags( + object_id="abc", + valid_only=False, + ) + ) == [ + alpha, + beta, + ] + + # No valid tags for this object yet.. + assert not list( + tagging_api.get_object_tags( + object_id="abc", + valid_only=True, + ) + ) + beta.tag = self.mammalia + beta.save() + assert list( + tagging_api.get_object_tags( + object_id="abc", + valid_only=True, + ) + ) == [ + beta, + ] + + # Fetch all the tags for alpha-type objects + assert list( + tagging_api.get_object_tags( + object_id="abc", + object_type="alpha", + valid_only=False, + ) + ) == [ + alpha, + ] + + # Fetch all the tags for a given object ID + taxonomy + assert list( + tagging_api.get_object_tags( + object_id="abc", + taxonomy=self.taxonomy, + valid_only=False, + ) + ) == [ + beta, + ] diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 098a4ee01..94d301958 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -3,7 +3,7 @@ import ddt from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy +from openedx_tagging.core.tagging.models import ClosedObjectTag, ObjectTag, OpenObjectTag, Tag, Taxonomy def get_tag(value): @@ -145,26 +145,58 @@ class TestModelObjectTag(TestTagTaxonomyMixin, TestCase): def setUp(self): super().setUp() self.tag = self.bacteria + self.object_tag = ObjectTag.objects.create( + object_id="object:id:1", + object_type="life", + taxonomy=self.taxonomy, + tag=self.tag, + ) + + def test_object_tag_class(self): + # This object tag is a ClosedObjectTag + object_tag = self.object_tag.taxonomy.object_tag_class().copy(self.object_tag) + assert ( + str(object_tag) + == repr(object_tag) + == " object:id:1 (life): Life on Earth=Bacteria" + ) + + # Check that changing the taxonomy to an open taxonomy changes the object tag class + open_taxonomy = Taxonomy.objects.create( + name="Freetext Life", + allow_free_text=True, + ) + self.object_tag.taxonomy = open_taxonomy + object_tag = self.object_tag.taxonomy.object_tag_class().copy(self.object_tag) + assert ( + str(object_tag) + == repr(object_tag) + == " object:id:1 (life): Freetext Life=Bacteria" + ) + + # Check that explicitly changing the object_tag_class also works + self.object_tag.taxonomy.object_tag_class = ObjectTag + object_tag = self.object_tag.taxonomy.object_tag_class().copy(self.object_tag) + assert ( + str(object_tag) + == repr(object_tag) + == " object:id:1 (life): Freetext Life=Bacteria" + ) def test_object_tag_name(self): # ObjectTag's name defaults to its taxonomy's name - object_tag = ObjectTag.objects.create( - object_id="object:id", - object_type="any_old_object", - taxonomy=self.taxonomy, - ) - assert object_tag.name == self.taxonomy.name + assert self.object_tag.name == self.taxonomy.name # Even if we overwrite the name, it still uses the taxonomy's name - object_tag.name = "Another tag" - assert object_tag.name == self.taxonomy.name - object_tag.save() - assert object_tag.name == self.taxonomy.name + self.object_tag.name = "Another tag" + assert self.object_tag.name == self.taxonomy.name + self.object_tag.save() + assert self.object_tag.name == self.taxonomy.name # But if the taxonomy is deleted, then the object_tag's name reverts to our cached name self.taxonomy.delete() - object_tag.refresh_from_db() - assert object_tag.name == "Another tag" + self.object_tag.refresh_from_db() + assert self.object_tag.name == "Another tag" def test_object_tag_value(self): # ObjectTag's value defaults to its tag's value @@ -209,41 +241,52 @@ def test_object_tag_lineage(self): assert object_tag.get_lineage() == ["Another tag"] def test_object_tag_is_valid(self): - object_tag = ObjectTag( - object_id="object:id", - object_type="any_old_object", - ) - assert not object_tag.is_valid - - object_tag.taxonomy = self.taxonomy - assert not object_tag.is_valid - - object_tag.tag = self.tag - assert object_tag.is_valid - - # or, we can have no tag, and a free-text taxonomy - object_tag.tag = None - self.taxonomy.allow_free_text = True - assert object_tag.is_valid + # ObjectTags are never valid + object_tag = ObjectTag() + assert not object_tag.is_valid() - def test_validate_object_tag_invalid(self): - taxonomy = Taxonomy.objects.create( - name="Another taxonomy", + open_taxonomy = Taxonomy.objects.create( + name="Freetext Life", + allow_free_text=True, ) - object_tag = ObjectTag( + + # OpenObjectTags are valid with a free-text taxonomy and a value + object_tag = OpenObjectTag( taxonomy=self.taxonomy, ) - assert not taxonomy.validate_object_tag(object_tag) - - object_tag.taxonomy = taxonomy - assert not taxonomy.validate_object_tag(object_tag) - - taxonomy.allow_free_text = True - assert not taxonomy.validate_object_tag(object_tag) - + assert not object_tag.is_valid( + check_taxonomy=True, check_tag=False, check_object=False + ) + assert not object_tag.is_valid( + check_taxonomy=False, check_tag=True, check_object=False + ) + assert not object_tag.is_valid( + check_taxonomy=False, check_tag=False, check_object=True + ) object_tag.object_id = "object:id" - object_tag.object_type = "object:type" - assert taxonomy.validate_object_tag(object_tag) + object_tag.object_type = "life" + object_tag._value = "Any text we want" + object_tag.taxonomy = open_taxonomy + assert object_tag.is_valid() + + # ClosedObjectTags require a closed taxonomy and a tag + object_tag = ClosedObjectTag( + taxonomy=open_taxonomy, + ) + assert not object_tag.is_valid( + check_taxonomy=True, check_tag=False, check_object=False + ) + assert not object_tag.is_valid( + check_taxonomy=False, check_tag=True, check_object=False + ) + assert not object_tag.is_valid( + check_taxonomy=False, check_tag=False, check_object=True + ) + object_tag.object_id = "object:id" + object_tag.object_type = "life" + object_tag.taxonomy = self.taxonomy + object_tag.tag = self.tag + assert object_tag.is_valid() def test_tag_object(self): self.taxonomy.allow_multiple = True @@ -282,7 +325,7 @@ def test_tag_object(self): 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.is_valid() assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" @@ -297,7 +340,7 @@ def test_tag_object_free_text(self): ) assert len(object_tags) == 1 object_tag = object_tags[0] - assert object_tag.is_valid + assert object_tag.is_valid() assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.tag_ref == "Eukaryota Xenomorph" diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index ecd453ebb..463ff540f 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy +from openedx_tagging.core.tagging.models import ClosedObjectTag, ObjectTag, Tag, Taxonomy from .test_models import TestTagTaxonomyMixin @@ -33,8 +33,7 @@ def setUp(self): username="learner", email="learner@example.com", ) - - self.object_tag = ObjectTag( + self.object_tag = ClosedObjectTag.objects.create( taxonomy=self.taxonomy, tag=self.bacteria, ) From 3993ad5062e7ba3bd0d1403294d699e07f1a9dfd Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 6 Jul 2023 08:33:31 +0930 Subject: [PATCH 04/14] feat: adds api.get_taxonomy for completeness. --- openedx_tagging/core/tagging/api.py | 8 +++++++- tests/openedx_tagging/core/tagging/test_api.py | 10 ++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 30bc39153..3c0609301 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -10,7 +10,7 @@ Please look at the models.py file for more information about the kinds of data are stored in this app. """ -from typing import Generator, List, Type +from typing import Generator, List, Type, Union from django.db.models import QuerySet from django.utils.translation import gettext_lazy as _ @@ -46,6 +46,12 @@ def create_taxonomy( return taxonomy +def get_taxonomy(id: int) -> Union[Taxonomy, None]: + """ + Returns a Taxonomy of the appropriate subclass which has the given ID. + """ + return Taxonomy.objects.filter(id=id).first() + def get_taxonomies(enabled=True) -> QuerySet: """ Returns a queryset containing the enabled taxonomies, sorted by name. diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 7827e7978..9e5ebd676 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -39,20 +39,26 @@ def test_create_taxonomy_bad_object_tag_class(self): exc.exception ) + def test_get_taxonomy(self): + tax1 = tagging_api.get_taxonomy(1) + assert tax1 == self.taxonomy + no_tax = tagging_api.get_taxonomy(10) + assert no_tax is None + def test_get_taxonomies(self): tax1 = tagging_api.create_taxonomy("Enabled") tax2 = tagging_api.create_taxonomy("Disabled", enabled=False) with self.assertNumQueries(1): enabled = list(tagging_api.get_taxonomies()) assert enabled == [tax1, self.taxonomy, self.system_taxonomy] - assert str(enabled[0]) == " (3) Enabled" + assert str(enabled[0]) == f" ({tax1.id}) Enabled" assert str(enabled[1]) == " (1) Life on Earth" assert str(enabled[2]) == " (2) System Languages" with self.assertNumQueries(1): disabled = list(tagging_api.get_taxonomies(enabled=False)) assert disabled == [tax2] - assert str(disabled[0]) == " (4) Disabled" + assert str(disabled[0]) == f" ({tax2.id}) Disabled" with self.assertNumQueries(1): both = list(tagging_api.get_taxonomies(enabled=None)) From 926a8d354821f9d74308fbb8a7c61a770969651f Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 6 Jul 2023 12:59:22 +0930 Subject: [PATCH 05/14] feat: adds object tag registry so that the various object tag classes can register themselves as candidates for casting/resyncing ObjectTags. * Updates tests to ensure full coverage for this change * Updates Tag model to cascade delete if the Taxonomy or parent Tag is deleted. Fixes an oversight in the data model. --- openedx_tagging/core/tagging/api.py | 84 +++++++++-- .../migrations/0004_tag_cascade_delete.py | 36 +++++ openedx_tagging/core/tagging/models.py | 142 ++++++++---------- openedx_tagging/core/tagging/registry.py | 58 +++++++ .../openedx_tagging/core/tagging/test_api.py | 130 +++++++++++++--- .../core/tagging/test_models.py | 141 +++-------------- .../core/tagging/test_rules.py | 2 +- 7 files changed, 361 insertions(+), 232 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py create mode 100644 openedx_tagging/core/tagging/registry.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 3c0609301..5c9261236 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -16,6 +16,7 @@ from django.utils.translation import gettext_lazy as _ from .models import ClosedObjectTag, ObjectTag, OpenObjectTag, Tag, Taxonomy +from .registry import get_object_tag_class def create_taxonomy( @@ -52,6 +53,7 @@ def get_taxonomy(id: int) -> Union[Taxonomy, None]: """ return Taxonomy.objects.filter(id=id).first() + def get_taxonomies(enabled=True) -> QuerySet: """ Returns a queryset containing the enabled taxonomies, sorted by name. @@ -73,18 +75,21 @@ def get_tags(taxonomy: Taxonomy) -> List[Tag]: return taxonomy.get_tags() -def cast_object_tag( - object_tag: ObjectTag, default_class=OpenObjectTag -) -> OpenObjectTag: +def cast_object_tag(object_tag: ObjectTag) -> ObjectTag: """ - Casts/copies the given object tag data into the ObjectTag subclass appropriate for this tag. + Casts/copies the given object tag data into the ObjectTag subclass most appropriate for this tag. E.g. if the tag's taxonomy has a custom ObjectTag class it prefers, use that. Recommends using OpenObjectTag by default, because that is the least restrictive type. """ - ObjectTagClass = default_class - if object_tag.taxonomy_id: - ObjectTagClass = object_tag.taxonomy.object_tag_class + ObjectTagClass = get_object_tag_class( + taxonomy=object_tag.taxonomy, + object_type=object_tag.object_type, + object_id=object_tag.object_id, + tag=object_tag.tag, + value=object_tag.value, + name=object_tag.name, + ) new_object_tag = ObjectTagClass().copy(object_tag) return new_object_tag @@ -99,7 +104,8 @@ def resync_object_tags(object_tags: QuerySet = None) -> int: object_tags = ObjectTag.objects.select_related("tag", "taxonomy") num_changed = 0 - for object_tag in object_tags: + for tag in object_tags: + object_tag = cast_object_tag(tag) changed = object_tag.resync() if changed: object_tag.save() @@ -150,4 +156,64 @@ def tag_object( Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. """ - return taxonomy.tag_object(tags, object_id, object_type) + if not taxonomy.allow_multiple and len(tags) > 1: + raise ValueError(_(f"Taxonomy ({taxonomy.id}) only allows one tag per object.")) + + if taxonomy.required and len(tags) == 0: + raise ValueError( + _(f"Taxonomy ({taxonomy.id}) requires at least one tag per object.") + ) + + current_tags = { + tag.tag_ref: tag + for tag in ObjectTag.objects.filter( + taxonomy=taxonomy, object_id=object_id, object_type=object_type + ) + } + updated_tags = [] + for tag_ref in tags: + if tag_ref in current_tags: + object_tag = cast_object_tag(current_tags.pop(tag_ref)) + else: + try: + tag = taxonomy.tag_set.get( + id=tag_ref, + ) + value = tag.value + except (ValueError, Tag.DoesNotExist): + # This might be ok, e.g. if taxonomy.allow_free_text. + # We'll validate below before saving. + tag = None + value = tag_ref + + ObjectTagClass = get_object_tag_class( + taxonomy=taxonomy, + object_id=object_id, + object_type=object_type, + tag=tag, + value=value, + name=taxonomy.name, + ) + object_tag = ObjectTagClass() + object_tag.taxonomy = taxonomy + object_tag.tag = tag + object_tag.object_id = object_id + object_tag.object_type = object_type + object_tag.value = value + + object_tag.resync() + if not object_tag.is_valid(): + raise ValueError( + _(f"Invalid object tag for taxonomy ({taxonomy.id}): {object_tag}") + ) + updated_tags.append(object_tag) + + # Save all updated tags at once to avoid partial updates + for object_tag in updated_tags: + object_tag.save() + + # ...and delete any omitted existing tags + for old_tag in current_tags.values(): + old_tag.delete() + + return updated_tags diff --git a/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py b/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py new file mode 100644 index 000000000..ebb6f74b2 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py @@ -0,0 +1,36 @@ +# Generated by Django 3.2.19 on 2023-07-06 03:56 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0003_objecttag_proxies"), + ] + + operations = [ + migrations.AlterField( + model_name="tag", + name="parent", + field=models.ForeignKey( + default=None, + help_text="Tag that lives one level up from the current tag, forming a hierarchy.", + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="children", + to="oel_tagging.tag", + ), + ), + migrations.AlterField( + model_name="tag", + name="taxonomy", + field=models.ForeignKey( + default=None, + help_text="Namespace and rules for using a given set of tags.", + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="oel_tagging.taxonomy", + ), + ), + ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index 202c7776b..c040a28aa 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -1,4 +1,5 @@ """ Tagging app data models """ + from typing import List, Type from django.db import models @@ -7,6 +8,8 @@ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field +from .registry import get_object_tag_class, register_object_tag_class + # Maximum depth allowed for a hierarchical taxonomy's tree of tags. TAXONOMY_MAX_DEPTH = 3 @@ -29,14 +32,14 @@ class Tag(models.Model): "Taxonomy", null=True, default=None, - on_delete=models.SET_NULL, + on_delete=models.CASCADE, help_text=_("Namespace and rules for using a given set of tags."), ) parent = models.ForeignKey( "self", null=True, default=None, - on_delete=models.SET_NULL, + on_delete=models.CASCADE, related_name="children", help_text=_( "Tag that lives one level up from the current tag, forming a hierarchy." @@ -170,18 +173,13 @@ def __str__(self): @property def object_tag_class(self) -> Type: """ - Returns the ObjectTag subclass associated with this taxonomy. + Returns the ObjectTag subclass associated with this taxonomy, or None if none supplied. May raise ImportError if a custom object_tag_class cannot be imported. """ if self._object_tag_class: - ObjectTagClass = import_string(self._object_tag_class) - elif self.allow_free_text: - ObjectTagClass = OpenObjectTag - else: - ObjectTagClass = ClosedObjectTag - - return ObjectTagClass + return import_string(self._object_tag_class) + return None @object_tag_class.setter def object_tag_class(self, object_tag_class: Type): @@ -234,71 +232,6 @@ def get_tags(self) -> List[Tag]: break return tags - def tag_object( - self, tags: List, object_id: str, object_type: str - ) -> List["ObjectTag"]: - """ - Replaces the existing ObjectTag entries for the current taxonomy + object_id with the given list of tags. - - If self.allows_free_text, then the list should be a list of tag values. - Otherwise, it should be a list of existing Tag IDs. - - Raised ValueError if the proposed tags are invalid for this taxonomy. - Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. - """ - - if not self.allow_multiple and len(tags) > 1: - raise ValueError(_(f"Taxonomy ({self.id}) only allows one tag per object.")) - - if self.required and len(tags) == 0: - raise ValueError( - _(f"Taxonomy ({self.id}) requires at least one tag per object.") - ) - - ObjectTagClass = self.object_tag_class - current_tags = { - tag.tag_ref: tag - for tag in ObjectTag.objects.filter( - taxonomy=self, object_id=object_id, object_type=object_type - ) - } - updated_tags = [] - for tag_ref in tags: - if tag_ref in current_tags: - object_tag = ObjectTagClass().copy(current_tags.pop(tag_ref)) - else: - object_tag = ObjectTagClass( - taxonomy=self, - object_id=object_id, - object_type=object_type, - ) - - try: - object_tag.tag = self.tag_set.get( - id=tag_ref, - ) - except (ValueError, Tag.DoesNotExist): - # This might be ok, e.g. if self.allow_free_text. - # We'll validate below before saving. - object_tag.value = tag_ref - - object_tag.resync() - if not object_tag.is_valid(): - raise ValueError( - _(f"Invalid object tag for taxonomy ({self.id}): {tag_ref}") - ) - updated_tags.append(object_tag) - - # Save all updated tags at once to avoid partial updates - for object_tag in updated_tags: - object_tag.save() - - # ...and delete any omitted existing tags - for old_tag in current_tags.values(): - old_tag.delete() - - return updated_tags - class ObjectTag(models.Model): """ @@ -368,6 +301,13 @@ class Meta: models.Index(fields=["taxonomy", "_value"]), ] + @classmethod + def valid_for(cls, **kwargs) -> bool: + """ + This is always a valid ObjectTag class, so we register it first. + """ + return True + def __repr__(self): """ Developer-facing representation of an ObjectTag. @@ -433,8 +373,8 @@ def copy(self, object_tag: "ObjectTag") -> "ObjectTag": self.object_type = object_tag.object_type self.taxonomy_id = object_tag.taxonomy_id self.tag_id = object_tag.tag_id - self._name = object_tag.name - self._value = object_tag.value + self._name = object_tag._name + self._value = object_tag._value return self def get_lineage(self) -> Lineage: @@ -474,16 +414,33 @@ def resync(self) -> bool: """ changed = False - # Locate a taxonomy matching _name + # Locate an enabled taxonomy matching _name, and maybe a tag matching _value if not self.taxonomy_id: - for taxonomy in Taxonomy.objects.filter(name=self.name, enabled=True): + for taxonomy in Taxonomy.objects.filter( + name=self.name, enabled=True + ).order_by("allow_free_text", "id"): + # Closed taxonomies require a tag matching _value, + # and we'd rather match a closed taxonomy than an open one. + # So see if there's a matching tag available in this taxonomy. + tag = self.tag or taxonomy.tag_set.filter(value=self.value).first() + # Make sure this taxonomy will accept object tags like this. - object_tag = taxonomy.object_tag_class().copy(self) + ObjectTagClass = get_object_tag_class( + taxonomy=taxonomy, + tag=tag, + object_id=self.object_id, + object_type=self.object_type, + name=self.name, + value=self.value, + ) + object_tag = ObjectTagClass().copy(self) object_tag.taxonomy = taxonomy + object_tag.tag = tag if object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=False + check_taxonomy=True, check_tag=True, check_object=False ): self.taxonomy = taxonomy + self.tag = tag changed = True break # If not, try the next one @@ -518,6 +475,13 @@ class OpenObjectTag(ObjectTag): class Meta: proxy = True + @classmethod + def valid_for(cls, taxonomy: Taxonomy = None, **kwargs) -> bool: + """ + Returns True if the taxonomy allows free text tags. + """ + return taxonomy and taxonomy.allow_free_text + def _check_object(self): """ Returns True if this ObjectTag has a valid object. @@ -561,6 +525,13 @@ class ClosedObjectTag(OpenObjectTag): class Meta: proxy = True + @classmethod + def valid_for(cls, taxonomy: Taxonomy = None, tag: Tag = None, **kwargs) -> bool: + """ + Returns True if it's a closed taxonomy and there's a tag + """ + return tag and taxonomy and not taxonomy.allow_free_text + def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: """ Returns True if this ObjectTag is valid for use with a closed taxonomy. @@ -579,7 +550,16 @@ def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bo if check_tag and not self.tag_id: return False + if check_tag and check_taxonomy and (self.tag.taxonomy != self.taxonomy): + return False + if check_object and not self._check_object(): return False return True + + +# Register the object tag classes in reverse order for how we want them considered +register_object_tag_class(ObjectTag) +register_object_tag_class(OpenObjectTag) +register_object_tag_class(ClosedObjectTag) diff --git a/openedx_tagging/core/tagging/registry.py b/openedx_tagging/core/tagging/registry.py new file mode 100644 index 000000000..c968d6a18 --- /dev/null +++ b/openedx_tagging/core/tagging/registry.py @@ -0,0 +1,58 @@ +""" +Registry for object tag classes +""" +import logging +from typing import Type + +log = logging.getLogger(__name__) + +# Global registry +_OBJECT_TAG_CLASS_REGISTRY = [] + + +def register_object_tag_class(cls): + """ + Register a given class as a candidate object tag class. + + The class must have a `valid_for` class method. + """ + assert hasattr(cls, "valid_for") + _OBJECT_TAG_CLASS_REGISTRY.append(cls) + + +def get_object_tag_class( + taxonomy: "Taxonomy" = None, + object_id: str = None, + object_type: str = None, + tag: "Tag" = None, + value: str = None, + name: str = None, +) -> Type: + """ + Returns the most appropriate ObjectTag subclass for the given attributes. + """ + # Some taxonomies have custom object tag classes applied. + if taxonomy: + try: + ObjectTagClass = taxonomy.object_tag_class + if ObjectTagClass: + # Should we also verify ObjectTagClass.valid_for ? + return ObjectTagClass + except ImportError: + # Log error and continue + log.exception(f"Unable to import custom object_tag_class for {taxonomy}") + + # Return the most recently-registered, appropriate object tag class + for ObjectTagClass in reversed(_OBJECT_TAG_CLASS_REGISTRY): + if ObjectTagClass.valid_for( + taxonomy=taxonomy, + object_type=object_type, + object_id=object_id, + tag=tag, + name=name, + value=value, + ): + return ObjectTagClass + + # We should never reach here -- ObjectTag is always valid, and the last class checked. + return None # pragma: no cover diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 9e5ebd676..0355a517c 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -1,7 +1,5 @@ """ Test the tagging APIs """ -from unittest.mock import Mock, patch - from django.test.testcases import TestCase import openedx_tagging.core.tagging.api as tagging_api @@ -125,8 +123,19 @@ def test_resync_object_tags(self): self.check_object_tag( object_tag, self.taxonomy, None, "Life on Earth", "Mammalia" ) + # Recreating the tag to test resyncing works + new_mammalia = Tag.objects.create( + value="Mammalia", + taxonomy=self.taxonomy, + ) + changed = tagging_api.resync_object_tags() + assert changed == 3 + for object_tag in (missing_links, changed_links, no_changes): + self.check_object_tag( + object_tag, self.taxonomy, new_mammalia, "Life on Earth", "Mammalia" + ) - # ObjectTag name preserved even if linked taxonomy is deleted + # 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") @@ -145,20 +154,11 @@ def test_resync_object_tags(self): taxonomy=second_taxonomy, ) - # Patch so that open taxonomy object tags won't validate, - # demonstrating that the resync logic will move on to the next taxonomy. - with patch( - "openedx_tagging.core.tagging.models.OpenObjectTag", - ) as mock_open_object_tag: - mock_open_object_tag.return_value = Mock() - mock_open_object_tag().copy.return_value = Mock() - mock_open_object_tag().copy().is_valid.return_value = False - - changed = tagging_api.resync_object_tags( - ObjectTag.objects.filter(object_type="alpha") - ) - assert changed == 2 - mock_open_object_tag().copy().is_valid.assert_called() + # Ensure the resync prefers the closed taxonomy with the matching tag + changed = tagging_api.resync_object_tags( + ObjectTag.objects.filter(object_type="alpha") + ) + assert changed == 2 for object_tag in (missing_links, changed_links): self.check_object_tag( @@ -168,17 +168,20 @@ def test_resync_object_tags(self): # Ensure the omitted tag was not updated self.check_object_tag(no_changes, None, None, "Life on Earth", "Mammalia") - # Update that one too (without the patching) + # Update that one too, to demonstrate the free-text tags are ok + no_changes.value = "Anamelia" + no_changes.save() changed = tagging_api.resync_object_tags( ObjectTag.objects.filter(object_type="beta") ) assert changed == 1 self.check_object_tag( - no_changes, first_taxonomy, None, "Life on Earth", "Mammalia" + no_changes, first_taxonomy, None, "Life on Earth", "Anamelia" ) def test_tag_object(self): self.taxonomy.allow_multiple = True + self.taxonomy.save() test_tags = [ [ self.archaea.id, @@ -225,6 +228,57 @@ def test_tag_object(self): assert object_tag.object_id == "biology101" assert object_tag.object_type == "course" + def test_tag_object_free_text(self): + self.taxonomy.allow_free_text = True + self.taxonomy.save() + object_tags = tagging_api.tag_object( + self.taxonomy, + ["Eukaryota Xenomorph"], + "biology101", + "course", + ) + assert len(object_tags) == 1 + object_tag = object_tags[0] + assert object_tag.is_valid() + assert object_tag.taxonomy == self.taxonomy + assert object_tag.name == self.taxonomy.name + assert object_tag.tag_ref == "Eukaryota Xenomorph" + assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] + assert object_tag.object_id == "biology101" + assert object_tag.object_type == "course" + + def test_tag_object_no_multiple(self): + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + self.taxonomy, + ["A", "B"], + "biology101", + "course", + ) + assert "only allows one tag per object" in str(exc.exception) + + 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", + "course", + ) + assert "requires at least one tag per object" in str(exc.exception) + + def test_tag_object_invalid_tag(self): + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + self.taxonomy, + ["Eukaryota Xenomorph"], + "biology101", + "course", + ) + assert "Invalid object tag for taxonomy" in str(exc.exception) + def test_get_object_tags(self): # Alpha tag has no taxonomy alpha = ObjectTag(object_id="abc", object_type="alpha") @@ -289,3 +343,41 @@ def test_get_object_tags(self): ) == [ beta, ] + + def test_object_tag_class(self): + # Create a valid ClosedObjectTag + assert not self.taxonomy.allow_free_text + object_tag = ObjectTag.objects.create( + object_id="object:id:1", + object_type="life", + taxonomy=self.taxonomy, + tag=self.bacteria, + ) + object_tag = tagging_api.cast_object_tag(object_tag) + assert ( + str(object_tag) + == repr(object_tag) + == " object:id:1 (life): Life on Earth=Bacteria" + ) + + # Check that changing the taxonomy to an open taxonomy changes the object tag class + open_taxonomy = tagging_api.create_taxonomy( + name="Freetext Life", + allow_free_text=True, + ) + object_tag.taxonomy = open_taxonomy + object_tag = tagging_api.cast_object_tag(object_tag) + assert ( + str(object_tag) + == repr(object_tag) + == " object:id:1 (life): Freetext Life=Bacteria" + ) + + # Check that explicitly changing the object_tag_class also works + object_tag.taxonomy.object_tag_class = ObjectTag + object_tag = tagging_api.cast_object_tag(object_tag) + assert ( + str(object_tag) + == repr(object_tag) + == " object:id:1 (life): Freetext Life=Bacteria" + ) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 94d301958..d99777b62 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -4,6 +4,7 @@ from django.test.testcases import TestCase from openedx_tagging.core.tagging.models import ClosedObjectTag, ObjectTag, OpenObjectTag, Tag, Taxonomy +from openedx_tagging.core.tagging.registry import get_object_tag_class def get_tag(value): @@ -136,6 +137,14 @@ def test_get_tags_shallow_taxonomy(self): with self.assertNumQueries(2): assert taxonomy.get_tags() == tags + def test_get_object_tag_class_invalid(self): + taxonomy = Taxonomy.objects.create( + name="invalid", + _object_tag_class="not.a.valid.class", + ) + ObjectTagClass = get_object_tag_class(taxonomy) + assert ObjectTagClass == ObjectTag + class TestModelObjectTag(TestTagTaxonomyMixin, TestCase): """ @@ -152,37 +161,6 @@ def setUp(self): tag=self.tag, ) - def test_object_tag_class(self): - # This object tag is a ClosedObjectTag - object_tag = self.object_tag.taxonomy.object_tag_class().copy(self.object_tag) - assert ( - str(object_tag) - == repr(object_tag) - == " object:id:1 (life): Life on Earth=Bacteria" - ) - - # Check that changing the taxonomy to an open taxonomy changes the object tag class - open_taxonomy = Taxonomy.objects.create( - name="Freetext Life", - allow_free_text=True, - ) - self.object_tag.taxonomy = open_taxonomy - object_tag = self.object_tag.taxonomy.object_tag_class().copy(self.object_tag) - assert ( - str(object_tag) - == repr(object_tag) - == " object:id:1 (life): Freetext Life=Bacteria" - ) - - # Check that explicitly changing the object_tag_class also works - self.object_tag.taxonomy.object_tag_class = ObjectTag - object_tag = self.object_tag.taxonomy.object_tag_class().copy(self.object_tag) - assert ( - str(object_tag) - == repr(object_tag) - == " object:id:1 (life): Freetext Life=Bacteria" - ) - def test_object_tag_name(self): # ObjectTag's name defaults to its taxonomy's name assert self.object_tag.name == self.taxonomy.name @@ -265,11 +243,11 @@ def test_object_tag_is_valid(self): ) object_tag.object_id = "object:id" object_tag.object_type = "life" - object_tag._value = "Any text we want" + object_tag.value = "Any text we want" object_tag.taxonomy = open_taxonomy assert object_tag.is_valid() - # ClosedObjectTags require a closed taxonomy and a tag + # ClosedObjectTags require a closed taxonomy and a tag in that taxonomy object_tag = ClosedObjectTag( taxonomy=open_taxonomy, ) @@ -282,96 +260,15 @@ def test_object_tag_is_valid(self): assert not object_tag.is_valid( check_taxonomy=False, check_tag=False, check_object=True ) + object_tag.taxonomy = self.taxonomy + object_tag.tag = Tag.objects.create( + taxonomy=self.system_taxonomy, + value="PT", + ) + assert not object_tag.is_valid( + check_taxonomy=True, check_tag=True, check_object=False + ) object_tag.object_id = "object:id" object_tag.object_type = "life" - object_tag.taxonomy = self.taxonomy object_tag.tag = self.tag assert object_tag.is_valid() - - def test_tag_object(self): - self.taxonomy.allow_multiple = True - - test_tags = [ - [ - self.archaea.id, - self.eubacteria.id, - self.chordata.id, - ], - [ - self.archaebacteria.id, - self.chordata.id, - ], - [ - self.archaea.id, - self.archaebacteria.id, - ], - ] - - # 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, - "biology101", - "course", - ) - - # Ensure the expected number of tags exist in the database - assert ObjectTag.objects.filter( - taxonomy=self.taxonomy, - object_id="biology101", - object_type="course", - ).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.taxonomy == self.taxonomy - assert object_tag.name == self.taxonomy.name - assert object_tag.object_id == "biology101" - assert object_tag.object_type == "course" - - def test_tag_object_free_text(self): - self.taxonomy.allow_free_text = True - object_tags = self.taxonomy.tag_object( - ["Eukaryota Xenomorph"], - "biology101", - "course", - ) - assert len(object_tags) == 1 - object_tag = object_tags[0] - assert object_tag.is_valid() - assert object_tag.taxonomy == self.taxonomy - assert object_tag.name == self.taxonomy.name - assert object_tag.tag_ref == "Eukaryota Xenomorph" - assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] - assert object_tag.object_id == "biology101" - assert object_tag.object_type == "course" - - def test_tag_object_no_multiple(self): - with self.assertRaises(ValueError) as exc: - self.taxonomy.tag_object( - ["A", "B"], - "biology101", - "course", - ) - assert "only allows one tag per object" in str(exc.exception) - - def test_tag_object_required(self): - self.taxonomy.required = True - with self.assertRaises(ValueError) as exc: - self.taxonomy.tag_object( - [], - "biology101", - "course", - ) - assert "requires at least one tag per object" in str(exc.exception) - - def test_tag_object_invalid_tag(self): - with self.assertRaises(ValueError) as exc: - self.taxonomy.tag_object( - ["Eukaryota Xenomorph"], - "biology101", - "course", - ) - assert "Invalid object tag for taxonomy" in str(exc.exception) diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index 463ff540f..fcd708e64 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import ClosedObjectTag, ObjectTag, Tag, Taxonomy +from openedx_tagging.core.tagging.models import ClosedObjectTag, ObjectTag, Tag from .test_models import TestTagTaxonomyMixin From 1280bcf6ec9ea98af884befb17f5e46474af861c Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 6 Jul 2023 13:49:05 +0930 Subject: [PATCH 06/14] docs: updates decisions --- docs/decisions/0007-tagging-app.rst | 14 +++++++++++--- docs/decisions/0009-tagging-administrators.rst | 2 +- .../decisions/0012-system-taxonomy-creation.rst | 17 +++++++---------- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/docs/decisions/0007-tagging-app.rst b/docs/decisions/0007-tagging-app.rst index 7dbc45b0b..ed536c79e 100644 --- a/docs/decisions/0007-tagging-app.rst +++ b/docs/decisions/0007-tagging-app.rst @@ -19,7 +19,7 @@ Taxonomy The ``openedx_tagging`` module defines ``openedx_tagging.core.models.Taxonomy``, whose data and functionality are self-contained to the ``openedx_tagging`` app. However in Studio, we need to be able to limit access to some Taxonomy by organization, using the same "course creator" access which limits course creation for an organization to a defined set of users. -So in edx-platform, we will create the ``openedx.features.tagging`` app, to contain ``models.OrgTaxonomy``. OrgTaxonomy subclasses ``openedx_tagging.core.models.Taxonomy``, employing Django's `multi-table inheritance`_ feature, which allows the base Tag class to keep foreign keys to the Taxonomy, while allowing OrgTaxonomy to store foreign keys into Studio's Organization table. +So in edx-platform, we will create the ``openedx.features.content_tagging`` app, to contain the models and logic for linking Organization owners to Taxonomies. There's no need to subclass Taxonomy here; we can enforce access using the ``content_tagging.api``. ObjectTag ~~~~~~~~~ @@ -27,7 +27,7 @@ ObjectTag Similarly, the ``openedx_tagging`` module defined ``openedx_tagging.core.models.ObjectTag``, also self-contained to the ``openedx_tagging`` app. -But to tag content in the LMS/Studio, we create ``openedx.features.tagging.models.ContentTag``, which subclasses ``ObjectTag``, and can then reference functionality available in the platform code. +But to tag content in the LMS/Studio, we need to enforce ``object_id`` as a CourseKey or UsageKey type. So to do this, we subclass ``ObjectTag``, and use the ``openedx.features.tagging.registry`` to register the subclass, so that it will be picked up when this tagging API creates or resyncs object tags. Rejected Alternatives --------------------- @@ -40,4 +40,12 @@ Embedding the logic in edx-platform would provide the content tagging logic spec However, we plan to extend tagging to other object types (e.g. People) and contexts (e.g. Marketing), and so a generic, standalone library is preferable in the log run. -.. _multi-table inheritance: https://docs.djangoproject.com/en/3.2/topics/db/models/#multi-table-inheritance +Subclass Taxonomy +~~~~~~~~~~~~~~~~~ + +Another approach considered was to encapsulate the complexity of ObjectTag validation in the Taxonomy class, and so use subclasses of Taxonomy to validate different types of ObjectTags, and use `multi-table inheritance`_ and django polymorphism when pulling the taxonomies from the database. + +However, because we will have a number of different types of Taxonomies, this proved non-performant. + + +-.. _multi-table inheritance: https://docs.djangoproject.com/en/3.2/topics/db/models/#multi-table-inheritance diff --git a/docs/decisions/0009-tagging-administrators.rst b/docs/decisions/0009-tagging-administrators.rst index 33b0ab523..cd5284232 100644 --- a/docs/decisions/0009-tagging-administrators.rst +++ b/docs/decisions/0009-tagging-administrators.rst @@ -22,7 +22,7 @@ In the Studio context, a modified version of "course creator" access will be use Permission #1 requires no external access, so can be enforced by the ``openedx_tagging`` app. -But because permissions #2 + #3 require access to the edx-platform CMS model `CourseCreator`_, this access can only be enforced in Studio, and so will live under `cms.djangoapps.tagging` along with the ``ContentTag`` class. Tagging MVP must work for libraries v1, v2 and courses created in Studio, and so tying these permissions to Studio is reasonable for the MVP. +But because permissions #2 + #3 require access to the edx-platform CMS model `CourseCreator`_, this access can only be enforced in Studio, and so will live under ``cms.djangoapps.content_tagging`` along with the ``ContentTag`` class. Tagging MVP must work for libraries v1, v2 and courses created in Studio, and so tying these permissions to Studio is reasonable for the MVP. Per `OEP-9`_, ``openedx_tagging`` will allow applications to use the standard Django API to query permissions, for example: ``user.has_perm('openedx_tagging.edit_taxonomy', taxonomy)``, and the appropriate permissions will be applied in that application's context. diff --git a/docs/decisions/0012-system-taxonomy-creation.rst b/docs/decisions/0012-system-taxonomy-creation.rst index fe67b6e34..2ed17886f 100644 --- a/docs/decisions/0012-system-taxonomy-creation.rst +++ b/docs/decisions/0012-system-taxonomy-creation.rst @@ -4,7 +4,7 @@ Context -------- -System-defined taxonomies are closed taxonomies created by the system. Some of these are totally static (e.g Language) +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. @@ -12,17 +12,15 @@ the System-defined taxonomies and their tags. Decision --------- -System-defined Taxonomy creation -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +System Tag lists and validation +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Each System-defined Taxonomy has its own class, which is used for tag validation (e.g. ``LanguageSystemTaxonomy``, ``OrganizationSystemTaxonomy``). -Each can overwrite ``get_tags``; to configure the valid tags, and ``validate_object_tag``; to check if a list of tags are valid. -Both functions are implemented on the ``Taxonomy`` base class, but can be overwritten to handle special cases. +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. This instances will be used on different APIs. +We need to create an instance of each System-defined Taxonomy's ObjectTag in a fixture. This instances will be used on different APIs. -Later, we need to create a ``Content-side`` class that lives on ``openedx.features.tagging`` for each content and taxonomy to be used -(eg. ``CourseLanguageSystemTaxonomy``, ``CourseOrganizationSystemTaxonomy``). +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 @@ -54,5 +52,4 @@ And if it's a large list of objects (e.g. Users), then copying that list into th It is better to dynamically generate the list of available Tags, and/or dynamically validate a submitted object tag than to store the options in the database. - .. _document number 0013: https://github.com/openedx/openedx-learning/blob/main/docs/decisions/0013-system-taxonomy-auto-tagging.rst From 6051491e87c01dad7a778301d750c41a746d7a90 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 10 Jul 2023 16:13:48 +0930 Subject: [PATCH 07/14] fix: fixes related to system-defined taxonomies * system_defined cannot be set with api.create_taxonomy, and is not editable once set. * adds taxonomy.visible_to_authors, which can be set to False for fully automated tagging. --- openedx_tagging/core/tagging/api.py | 6 ++-- .../migrations/0005_auto_20230710_0140.py | 30 +++++++++++++++++++ openedx_tagging/core/tagging/models.py | 8 +++++ .../openedx_tagging/core/tagging/test_api.py | 4 ++- 4 files changed, 43 insertions(+), 5 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 5c9261236..7d0b39da5 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -10,7 +10,7 @@ Please look at the models.py file for more information about the kinds of data are stored in this app. """ -from typing import Generator, List, Type, Union +from typing import Iterator, List, Type, Union from django.db.models import QuerySet from django.utils.translation import gettext_lazy as _ @@ -26,7 +26,6 @@ def create_taxonomy( required=False, allow_multiple=False, allow_free_text=False, - system_defined=False, object_tag_class: Type = None, ) -> Taxonomy: """ @@ -39,7 +38,6 @@ def create_taxonomy( required=required, allow_multiple=allow_multiple, allow_free_text=allow_free_text, - system_defined=system_defined, ) if object_tag_class: taxonomy.object_tag_class = object_tag_class @@ -115,7 +113,7 @@ def resync_object_tags(object_tags: QuerySet = None) -> int: def get_object_tags( object_id: str, object_type: str = None, taxonomy: Taxonomy = None, valid_only=True -) -> Generator[ObjectTag, None, None]: +) -> Iterator[ObjectTag]: """ Generates a list of object tags for a given object. diff --git a/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py b/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py new file mode 100644 index 000000000..4b6210ec2 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py @@ -0,0 +1,30 @@ +# Generated by Django 3.2.19 on 2023-07-10 06:40 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0004_tag_cascade_delete"), + ] + + operations = [ + migrations.AddField( + model_name="taxonomy", + name="visible_to_authors", + field=models.BooleanField( + default=True, + editable=False, + help_text="Indicates whether this taxonomy should be visible to object authors.", + ), + ), + migrations.AlterField( + model_name="taxonomy", + name="system_defined", + field=models.BooleanField( + default=False, + editable=False, + help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", + ), + ), + ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index c040a28aa..7a8166c48 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -141,11 +141,19 @@ class Taxonomy(models.Model): ) system_defined = models.BooleanField( default=False, + editable=False, help_text=_( "Indicates that tags and metadata for this taxonomy are maintained by the system;" " taxonomy admins will not be permitted to modify them.", ), ) + visible_to_authors = models.BooleanField( + default=True, + editable=False, + help_text=_( + "Indicates whether this taxonomy should be visible to object authors." + ), + ) _object_tag_class = models.CharField( null=True, max_length=255, diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 0355a517c..5509baaa1 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -21,11 +21,13 @@ def test_create_taxonomy(self): "required": True, "allow_multiple": True, "allow_free_text": True, - "system_defined": True, } taxonomy = tagging_api.create_taxonomy(**params) for param, value in params.items(): assert getattr(taxonomy, param) == value + assert taxonomy.system_defined == False + assert taxonomy.visible_to_authors == True + assert taxonomy.object_tag_class is None def test_create_taxonomy_bad_object_tag_class(self): with self.assertRaises(ValueError) as exc: From c35cb6aad05b018e2d9e2e04f78fa620c630b53a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 10 Jul 2023 14:12:52 +0930 Subject: [PATCH 08/14] feat: adds "index" option to register_object_tag_class to allow subclasses to be registered wherever they want to be. --- openedx_tagging/core/tagging/models.py | 2 +- openedx_tagging/core/tagging/registry.py | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index 7a8166c48..eca3ecbad 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -567,7 +567,7 @@ def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bo return True -# Register the object tag classes in reverse order for how we want them considered +# Register the ObjectTag subclasses in reverse order of how we want them considered. register_object_tag_class(ObjectTag) register_object_tag_class(OpenObjectTag) register_object_tag_class(ClosedObjectTag) diff --git a/openedx_tagging/core/tagging/registry.py b/openedx_tagging/core/tagging/registry.py index c968d6a18..89ea9208d 100644 --- a/openedx_tagging/core/tagging/registry.py +++ b/openedx_tagging/core/tagging/registry.py @@ -10,14 +10,17 @@ _OBJECT_TAG_CLASS_REGISTRY = [] -def register_object_tag_class(cls): +def register_object_tag_class(cls, index=0): """ Register a given class as a candidate object tag class. + By default, inserts the given class at the beginning of the list, so that it will be considered before all + previously-registered classes. Adjust ``index`` to change where the class will be considered. + The class must have a `valid_for` class method. """ assert hasattr(cls, "valid_for") - _OBJECT_TAG_CLASS_REGISTRY.append(cls) + _OBJECT_TAG_CLASS_REGISTRY.insert(index, cls) def get_object_tag_class( @@ -42,8 +45,8 @@ def get_object_tag_class( # Log error and continue log.exception(f"Unable to import custom object_tag_class for {taxonomy}") - # Return the most recently-registered, appropriate object tag class - for ObjectTagClass in reversed(_OBJECT_TAG_CLASS_REGISTRY): + # Return the first appropriate object tag class + for ObjectTagClass in _OBJECT_TAG_CLASS_REGISTRY: if ObjectTagClass.valid_for( taxonomy=taxonomy, object_type=object_type, From a8566d366eb2e5639c9e4d5d53b56e1217d7ff56 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 10 Jul 2023 16:31:44 +0930 Subject: [PATCH 09/14] refactor: adds _check_taxonomy and _check_tag methods to the ObjectTag subclasses --- openedx_tagging/core/tagging/models.py | 64 ++++++++++++++++++-------- 1 file changed, 46 insertions(+), 18 deletions(-) diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index eca3ecbad..4a2fa0b41 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -379,8 +379,8 @@ def copy(self, object_tag: "ObjectTag") -> "ObjectTag": self.id = object_tag.id self.object_id = object_tag.object_id self.object_type = object_tag.object_type - self.taxonomy_id = object_tag.taxonomy_id - self.tag_id = object_tag.tag_id + self.taxonomy = object_tag.taxonomy + self.tag = object_tag.tag self._name = object_tag._name self._value = object_tag._value return self @@ -490,6 +490,24 @@ def valid_for(cls, taxonomy: Taxonomy = None, **kwargs) -> bool: """ return taxonomy and taxonomy.allow_free_text + def _check_taxonomy(self): + """ + Returns True if this ObjectTag has a valid taxonomy. + + Subclasses should override this method to perform any additional validation for the particular type of object tag. + """ + # Must be linked to a free-text taxonomy + return self.taxonomy_id and self.taxonomy.allow_free_text + + def _check_tag(self): + """ + Returns True if this ObjectTag has a valid tag value. + + Subclasses should override this method to perform any additional validation for the particular type of object tag. + """ + # Open taxonomies don't need an associated tag, but we need a value. + return bool(self._value) + def _check_object(self): """ Returns True if this ObjectTag has a valid object. @@ -509,14 +527,10 @@ def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bo 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. """ - # Must be linked to a free-text taxonomy - if check_taxonomy and ( - not self.taxonomy_id or not self.taxonomy.allow_free_text - ): + if check_taxonomy and not self._check_taxonomy(): return False - # Open taxonomies don't need an associated tag, but we need a value. - if check_tag and not self.value: + if check_tag and not self._check_tag(): return False if check_object and not self._check_object(): @@ -540,6 +554,24 @@ def valid_for(cls, taxonomy: Taxonomy = None, tag: Tag = None, **kwargs) -> bool """ return tag and taxonomy and not taxonomy.allow_free_text + def _check_taxonomy(self): + """ + Returns True if this ObjectTag has a valid taxonomy. + + Subclasses should override this method to perform any additional validation for the particular type of object tag. + """ + # Must be linked to a closed taxonomy + return self.taxonomy_id and not self.taxonomy.allow_free_text + + def _check_tag(self): + """ + Returns True if this ObjectTag has a valid tag value. + + Subclasses should override this method to perform any additional validation for the particular type of object tag. + """ + # Closed taxonomies require a Tag + return bool(self.tag_id) + def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: """ Returns True if this ObjectTag is valid for use with a closed taxonomy. @@ -550,18 +582,14 @@ def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bo 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. """ - # Must be linked to a closed taxonomy - if check_taxonomy and (not self.taxonomy_id or self.taxonomy.allow_free_text): - return False - - # Closed taxonomies require a Tag - if check_tag and not self.tag_id: - return False - - if check_tag and check_taxonomy and (self.tag.taxonomy != self.taxonomy): + if not super().is_valid( + check_taxonomy=check_taxonomy, + check_tag=check_tag, + check_object=check_object, + ): return False - if check_object and not self._check_object(): + if check_tag and check_taxonomy and (self.tag.taxonomy_id != self.taxonomy_id): return False return True From 7fe2841bc2b7990a0dee616c753ba4398c5456a6 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 12 Jul 2023 10:44:52 +0930 Subject: [PATCH 10/14] docs: fixes comment --- openedx_tagging/core/tagging/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 7d0b39da5..401e03c15 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -135,7 +135,7 @@ def get_object_tags( tags = tags.filter(taxonomy=taxonomy) for tag in tags: - # We can only validate tags with taxonomies, because we need the object_tag_class + # Cast so the appropriate ObjectTag class can handle validation object_tag = cast_object_tag(tag) if not valid_only or object_tag.is_valid(): yield object_tag From 98daf95f09d5af0b11aa874e47155a6d087a13be Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 12 Jul 2023 14:26:35 +0930 Subject: [PATCH 11/14] refactor: consolidates ObjectTag validation * uses the (overwritable) is_valid method instead of the (difficult to override) class method valid_for * moves cast_object_tag to the registry, since we had to cast ObjectTags to do ^ * removes ObjectTag base class from the registry, so we can still use cast_object_tag for validation --- openedx_tagging/core/tagging/api.py | 55 ++++----- openedx_tagging/core/tagging/models.py | 113 ++++++++---------- openedx_tagging/core/tagging/registry.py | 40 ++----- .../openedx_tagging/core/tagging/test_api.py | 22 ++-- .../core/tagging/test_models.py | 39 ++++-- 5 files changed, 122 insertions(+), 147 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 401e03c15..762372419 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -16,7 +16,7 @@ from django.utils.translation import gettext_lazy as _ from .models import ClosedObjectTag, ObjectTag, OpenObjectTag, Tag, Taxonomy -from .registry import get_object_tag_class +from .registry import cast_object_tag as _cast_object_tag def create_taxonomy( @@ -73,22 +73,20 @@ def get_tags(taxonomy: Taxonomy) -> List[Tag]: return taxonomy.get_tags() -def cast_object_tag(object_tag: ObjectTag) -> ObjectTag: +def cast_object_tag( + object_tag: ObjectTag, return_subclass=False +) -> Union[ObjectTag, None]: """ Casts/copies the given object tag data into the ObjectTag subclass most appropriate for this tag. - E.g. if the tag's taxonomy has a custom ObjectTag class it prefers, use that. - Recommends using OpenObjectTag by default, because that is the least restrictive type. + If ``return_subclass``, this method may return None if it doesn't find a valid subclass of ObjectTag for the + given object_tag. + + If not ``return_subclass``, then the base ObjectTag class may be returned. """ - ObjectTagClass = get_object_tag_class( - taxonomy=object_tag.taxonomy, - object_type=object_tag.object_type, - object_id=object_tag.object_id, - tag=object_tag.tag, - value=object_tag.value, - name=object_tag.name, - ) - new_object_tag = ObjectTagClass().copy(object_tag) + new_object_tag = _cast_object_tag(object_tag) + if not new_object_tag and not return_subclass: + new_object_tag = ObjectTag().copy(object_tag) return new_object_tag @@ -135,9 +133,8 @@ def get_object_tags( tags = tags.filter(taxonomy=taxonomy) for tag in tags: - # Cast so the appropriate ObjectTag class can handle validation - object_tag = cast_object_tag(tag) - if not valid_only or object_tag.is_valid(): + object_tag = cast_object_tag(tag, return_subclass=valid_only) + if object_tag: yield object_tag @@ -184,26 +181,18 @@ def tag_object( tag = None value = tag_ref - ObjectTagClass = get_object_tag_class( - taxonomy=taxonomy, - object_id=object_id, - object_type=object_type, - tag=tag, - value=value, - name=taxonomy.name, + object_tag = cast_object_tag( + ObjectTag( + taxonomy=taxonomy, + object_id=object_id, + object_type=object_type, + tag=tag, + value=value, + name=taxonomy.name, + ) ) - object_tag = ObjectTagClass() - object_tag.taxonomy = taxonomy - object_tag.tag = tag - object_tag.object_id = object_id - object_tag.object_type = object_type - object_tag.value = value object_tag.resync() - if not object_tag.is_valid(): - raise ValueError( - _(f"Invalid object tag for taxonomy ({taxonomy.id}): {object_tag}") - ) updated_tags.append(object_tag) # Save all updated tags at once to avoid partial updates diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index 4a2fa0b41..6471c033a 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -1,5 +1,4 @@ """ Tagging app data models """ - from typing import List, Type from django.db import models @@ -8,7 +7,7 @@ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field -from .registry import get_object_tag_class, register_object_tag_class +from .registry import cast_object_tag, register_object_tag_class # Maximum depth allowed for a hierarchical taxonomy's tree of tags. TAXONOMY_MAX_DEPTH = 3 @@ -309,13 +308,6 @@ class Meta: models.Index(fields=["taxonomy", "_value"]), ] - @classmethod - def valid_for(cls, **kwargs) -> bool: - """ - This is always a valid ObjectTag class, so we register it first. - """ - return True - def __repr__(self): """ Developer-facing representation of an ObjectTag. @@ -394,18 +386,48 @@ def get_lineage(self) -> Lineage: """ return self.tag.get_lineage() if self.tag_id else [self._value] - def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: + def _check_taxonomy(self): + """ + Always returns True. + + Subclasses should override this method to perform validation for the particular type of object tag. + """ + return True + + def _check_tag(self): + """ + Always returns True. + + Subclasses should override this method to perform validation for the particular type of object tag. + """ + return True + + def _check_object(self): + """ + Always returns True. + + Subclasses should override this method to perform validation for the particular type of object tag. """ - Always returns False -- use subclasses to find out whether this ObjectTag is valid for the linked taxonomy and/or tag. + return True - Subclasses must override this method to perform the proper validation checks, e.g. closed vs open taxonomies, - dynamically generated tag lists or object definitions. + def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: + """ + Returns True if this ObjectTag is valid. If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. If `check_tag` is False, then we skip validating the object tag's tag reference. If `check_object` is False, then we skip validating the object ID/type. """ - return False + if check_taxonomy and not self._check_taxonomy(): + return False + + if check_tag and not self._check_tag(): + return False + + if check_object and not self._check_object(): + return False + + return True def resync(self) -> bool: """ @@ -433,20 +455,17 @@ def resync(self) -> bool: tag = self.tag or taxonomy.tag_set.filter(value=self.value).first() # Make sure this taxonomy will accept object tags like this. - ObjectTagClass = get_object_tag_class( - taxonomy=taxonomy, - tag=tag, - object_id=self.object_id, - object_type=self.object_type, - name=self.name, - value=self.value, + test_object_tag = cast_object_tag( + ObjectTag( + taxonomy=taxonomy, + tag=tag, + object_id=self.object_id, + object_type=self.object_type, + _name=self.name, + _value=self.value, + ) ) - object_tag = ObjectTagClass().copy(self) - object_tag.taxonomy = taxonomy - object_tag.tag = tag - if object_tag.is_valid( - check_taxonomy=True, check_tag=True, check_object=False - ): + if test_object_tag: self.taxonomy = taxonomy self.tag = tag changed = True @@ -483,13 +502,6 @@ class OpenObjectTag(ObjectTag): class Meta: proxy = True - @classmethod - def valid_for(cls, taxonomy: Taxonomy = None, **kwargs) -> bool: - """ - Returns True if the taxonomy allows free text tags. - """ - return taxonomy and taxonomy.allow_free_text - def _check_taxonomy(self): """ Returns True if this ObjectTag has a valid taxonomy. @@ -517,27 +529,6 @@ def _check_object(self): # Must have a valid object id/type: return self.object_id and self.object_type - def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: - """ - Returns True if this ObjectTag is valid for use with a free-text taxonomy. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - - If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. - If `check_tag` is False, then we skip validating the object tag's tag reference. - If `check_object` is False, then we skip validating the object ID/type. - """ - if check_taxonomy and not self._check_taxonomy(): - return False - - if check_tag and not self._check_tag(): - return False - - if check_object and not self._check_object(): - return False - - return True - class ClosedObjectTag(OpenObjectTag): """ @@ -547,16 +538,9 @@ class ClosedObjectTag(OpenObjectTag): class Meta: proxy = True - @classmethod - def valid_for(cls, taxonomy: Taxonomy = None, tag: Tag = None, **kwargs) -> bool: - """ - Returns True if it's a closed taxonomy and there's a tag - """ - return tag and taxonomy and not taxonomy.allow_free_text - def _check_taxonomy(self): """ - Returns True if this ObjectTag has a valid taxonomy. + Returns True if this ObjectTag is linked to a closed taxonomy. Subclasses should override this method to perform any additional validation for the particular type of object tag. """ @@ -565,7 +549,7 @@ def _check_taxonomy(self): def _check_tag(self): """ - Returns True if this ObjectTag has a valid tag value. + Returns True if this ObjectTag has a valid tag. Subclasses should override this method to perform any additional validation for the particular type of object tag. """ @@ -596,6 +580,5 @@ def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bo # Register the ObjectTag subclasses in reverse order of how we want them considered. -register_object_tag_class(ObjectTag) register_object_tag_class(OpenObjectTag) register_object_tag_class(ClosedObjectTag) diff --git a/openedx_tagging/core/tagging/registry.py b/openedx_tagging/core/tagging/registry.py index 89ea9208d..11dd0ff25 100644 --- a/openedx_tagging/core/tagging/registry.py +++ b/openedx_tagging/core/tagging/registry.py @@ -2,7 +2,7 @@ Registry for object tag classes """ import logging -from typing import Type +from typing import Iterator, Type log = logging.getLogger(__name__) @@ -16,46 +16,32 @@ def register_object_tag_class(cls, index=0): By default, inserts the given class at the beginning of the list, so that it will be considered before all previously-registered classes. Adjust ``index`` to change where the class will be considered. - - The class must have a `valid_for` class method. """ - assert hasattr(cls, "valid_for") _OBJECT_TAG_CLASS_REGISTRY.insert(index, cls) -def get_object_tag_class( - taxonomy: "Taxonomy" = None, - object_id: str = None, - object_type: str = None, - tag: "Tag" = None, - value: str = None, - name: str = None, -) -> Type: +def cast_object_tag(object_tag: "ObjectTag") -> "ObjectTag": """ Returns the most appropriate ObjectTag subclass for the given attributes. + + If no ObjectTag subclasses are found, then returns None. """ # Some taxonomies have custom object tag classes applied. - if taxonomy: + if object_tag.taxonomy_id: + taxonomy = object_tag.taxonomy try: ObjectTagClass = taxonomy.object_tag_class if ObjectTagClass: - # Should we also verify ObjectTagClass.valid_for ? - return ObjectTagClass + return ObjectTagClass().copy(object_tag) except ImportError: # Log error and continue log.exception(f"Unable to import custom object_tag_class for {taxonomy}") # Return the first appropriate object tag class for ObjectTagClass in _OBJECT_TAG_CLASS_REGISTRY: - if ObjectTagClass.valid_for( - taxonomy=taxonomy, - object_type=object_type, - object_id=object_id, - tag=tag, - name=name, - value=value, - ): - return ObjectTagClass - - # We should never reach here -- ObjectTag is always valid, and the last class checked. - return None # pragma: no cover + cast_object_tag = ObjectTagClass().copy(object_tag) + + if cast_object_tag.is_valid(): + return cast_object_tag + + return None diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 5509baaa1..5794e5a90 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -25,8 +25,8 @@ def test_create_taxonomy(self): taxonomy = tagging_api.create_taxonomy(**params) for param, value in params.items(): assert getattr(taxonomy, param) == value - assert taxonomy.system_defined == False - assert taxonomy.visible_to_authors == True + assert not taxonomy.system_defined + assert taxonomy.visible_to_authors assert taxonomy.object_tag_class is None def test_create_taxonomy_bad_object_tag_class(self): @@ -272,14 +272,13 @@ def test_tag_object_required(self): assert "requires at least one tag per object" in str(exc.exception) def test_tag_object_invalid_tag(self): - with self.assertRaises(ValueError) as exc: - tagging_api.tag_object( - self.taxonomy, - ["Eukaryota Xenomorph"], - "biology101", - "course", - ) - assert "Invalid object tag for taxonomy" in str(exc.exception) + object_tag = tagging_api.tag_object( + self.taxonomy, + ["Eukaryota Xenomorph"], + "biology101", + "course", + )[0] + assert type(object_tag) == ObjectTag # pylint: disable=unidiomatic-typecheck def test_get_object_tags(self): # Alpha tag has no taxonomy @@ -346,7 +345,7 @@ def test_get_object_tags(self): beta, ] - def test_object_tag_class(self): + def test_cast_object_tag(self): # Create a valid ClosedObjectTag assert not self.taxonomy.allow_free_text object_tag = ObjectTag.objects.create( @@ -368,6 +367,7 @@ def test_object_tag_class(self): allow_free_text=True, ) object_tag.taxonomy = open_taxonomy + object_tag.value = "Bacterium" object_tag = tagging_api.cast_object_tag(object_tag) assert ( str(object_tag) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index d99777b62..e5774f757 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -3,8 +3,14 @@ import ddt from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import ClosedObjectTag, ObjectTag, OpenObjectTag, Tag, Taxonomy -from openedx_tagging.core.tagging.registry import get_object_tag_class +from openedx_tagging.core.tagging.models import ( + ClosedObjectTag, + ObjectTag, + OpenObjectTag, + Tag, + Taxonomy, + cast_object_tag, +) def get_tag(value): @@ -142,8 +148,9 @@ def test_get_object_tag_class_invalid(self): name="invalid", _object_tag_class="not.a.valid.class", ) - ObjectTagClass = get_object_tag_class(taxonomy) - assert ObjectTagClass == ObjectTag + # cast_object_tag will fall back to ObjectTag if invalid. + object_tag = cast_object_tag(ObjectTag(taxonomy=taxonomy)) + assert object_tag is None class TestModelObjectTag(TestTagTaxonomyMixin, TestCase): @@ -219,9 +226,9 @@ def test_object_tag_lineage(self): assert object_tag.get_lineage() == ["Another tag"] def test_object_tag_is_valid(self): - # ObjectTags are never valid + # ObjectTags are always valid object_tag = ObjectTag() - assert not object_tag.is_valid() + assert object_tag.is_valid() open_taxonomy = Taxonomy.objects.create( name="Freetext Life", @@ -260,15 +267,25 @@ def test_object_tag_is_valid(self): assert not object_tag.is_valid( check_taxonomy=False, check_tag=False, check_object=True ) + object_tag.object_id = "object:id" + object_tag.object_type = "life" + assert object_tag.is_valid( + check_taxonomy=False, check_tag=False, check_object=True + ) object_tag.taxonomy = self.taxonomy object_tag.tag = Tag.objects.create( taxonomy=self.system_taxonomy, value="PT", ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=True, check_object=False - ) - object_tag.object_id = "object:id" - object_tag.object_type = "life" + assert not object_tag.is_valid() object_tag.tag = self.tag + assert object_tag.is_valid( + check_taxonomy=False, check_tag=False, check_object=True + ) + assert object_tag.is_valid( + check_taxonomy=False, check_tag=True, check_object=True + ) + assert object_tag.is_valid( + check_taxonomy=True, check_tag=False, check_object=True + ) assert object_tag.is_valid() From bbbb4cc6b9a477f397d1ca8e4618cb7278126d1a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 17 Jul 2023 18:05:09 +0930 Subject: [PATCH 12/14] refactor: moves object_tag_class to the ObjectTag class and removes the Closed vs Open ObjectTag subclasses, and registry. Tests are passing, but coverage isn't at 100% --- openedx_tagging/core/tagging/api.py | 52 ++-- .../migrations/0006_auto_20230717_0200.py | 31 +++ openedx_tagging/core/tagging/models.py | 257 +++++++----------- openedx_tagging/core/tagging/registry.py | 47 ---- .../openedx_tagging/core/tagging/test_api.py | 51 +--- .../core/tagging/test_models.py | 56 +--- .../core/tagging/test_rules.py | 4 +- 7 files changed, 177 insertions(+), 321 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py delete mode 100644 openedx_tagging/core/tagging/registry.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 762372419..8b593a27d 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -15,8 +15,7 @@ from django.db.models import QuerySet from django.utils.translation import gettext_lazy as _ -from .models import ClosedObjectTag, ObjectTag, OpenObjectTag, Tag, Taxonomy -from .registry import cast_object_tag as _cast_object_tag +from .models import ObjectTag, Tag, Taxonomy def create_taxonomy( @@ -26,12 +25,11 @@ def create_taxonomy( required=False, allow_multiple=False, allow_free_text=False, - object_tag_class: Type = None, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. """ - taxonomy = Taxonomy( + taxonomy = Taxonomy.objects.create( name=name, description=description, enabled=enabled, @@ -39,9 +37,6 @@ def create_taxonomy( allow_multiple=allow_multiple, allow_free_text=allow_free_text, ) - if object_tag_class: - taxonomy.object_tag_class = object_tag_class - taxonomy.save() return taxonomy @@ -73,21 +68,11 @@ def get_tags(taxonomy: Taxonomy) -> List[Tag]: return taxonomy.get_tags() -def cast_object_tag( - object_tag: ObjectTag, return_subclass=False -) -> Union[ObjectTag, None]: +def cast_object_tag(object_tag: ObjectTag) -> ObjectTag: """ Casts/copies the given object tag data into the ObjectTag subclass most appropriate for this tag. - - If ``return_subclass``, this method may return None if it doesn't find a valid subclass of ObjectTag for the - given object_tag. - - If not ``return_subclass``, then the base ObjectTag class may be returned. """ - new_object_tag = _cast_object_tag(object_tag) - if not new_object_tag and not return_subclass: - new_object_tag = ObjectTag().copy(object_tag) - return new_object_tag + return object_tag.cast_object_tag() def resync_object_tags(object_tags: QuerySet = None) -> int: @@ -133,13 +118,17 @@ def get_object_tags( tags = tags.filter(taxonomy=taxonomy) for tag in tags: - object_tag = cast_object_tag(tag, return_subclass=valid_only) - if object_tag: + object_tag = cast_object_tag(tag) + if not valid_only or object_tag.is_valid(): yield object_tag def tag_object( - taxonomy: Taxonomy, tags: List, object_id: str, object_type: str + taxonomy: Taxonomy, + tags: List, + object_id: str, + object_type: str, + object_tag_class: Type = None, ) -> List[ObjectTag]: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags. @@ -181,16 +170,17 @@ def tag_object( tag = None value = tag_ref - object_tag = cast_object_tag( - ObjectTag( - taxonomy=taxonomy, - object_id=object_id, - object_type=object_type, - tag=tag, - value=value, - name=taxonomy.name, - ) + object_tag = ObjectTag( + taxonomy=taxonomy, + object_id=object_id, + object_type=object_type, + tag=tag, + value=value, + name=taxonomy.name, ) + if object_tag_class: + object_tag.object_tag_class = object_tag_class + object_tag = cast_object_tag(object_tag) object_tag.resync() updated_tags.append(object_tag) diff --git a/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py b/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py new file mode 100644 index 000000000..48832c1a1 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py @@ -0,0 +1,31 @@ +# Generated by Django 3.2.19 on 2023-07-17 07:00 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0005_auto_20230710_0140"), + ] + + operations = [ + migrations.DeleteModel( + name="ClosedObjectTag", + ), + migrations.DeleteModel( + name="OpenObjectTag", + ), + migrations.RemoveField( + model_name="taxonomy", + name="_object_tag_class", + ), + migrations.AddField( + model_name="objecttag", + name="_object_tag_class", + field=models.CharField( + help_text="Overrides the ObjectTag subclass used to instantiate this ObjectTag.Must be a fully-qualified module and class name.", + max_length=255, + null=True, + ), + ), + ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index 6471c033a..ded22db82 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -7,8 +7,6 @@ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field -from .registry import cast_object_tag, register_object_tag_class - # Maximum depth allowed for a hierarchical taxonomy's tree of tags. TAXONOMY_MAX_DEPTH = 3 @@ -153,14 +151,6 @@ class Taxonomy(models.Model): "Indicates whether this taxonomy should be visible to object authors." ), ) - _object_tag_class = models.CharField( - null=True, - max_length=255, - help_text=_( - "Overrides the default ObjectTag subclass associated with this taxonomy." - "Must be a fully-qualified module and class name.", - ), - ) class Meta: verbose_name_plural = "Taxonomies" @@ -177,34 +167,6 @@ def __str__(self): """ return f"<{self.__class__.__name__}> ({self.id}) {self.name}" - @property - def object_tag_class(self) -> Type: - """ - Returns the ObjectTag subclass associated with this taxonomy, or None if none supplied. - - May raise ImportError if a custom object_tag_class cannot be imported. - """ - if self._object_tag_class: - return import_string(self._object_tag_class) - return None - - @object_tag_class.setter - def object_tag_class(self, object_tag_class: Type): - """ - Assigns the given object_tag_class's module path.class to the field. - - Raises ValueError if the given `object_tag_class` is a built-in class; it should be an ObjectTag-like class. - """ - if object_tag_class.__module__ == "builtins": - raise ValueError( - f"object_tag_class {object_tag_class} must be class like ObjectTag" - ) - - # ref: https://stackoverflow.com/a/2020083 - self._object_tag_class = ".".join( - [object_tag_class.__module__, object_tag_class.__qualname__] - ) - def get_tags(self) -> List[Tag]: """ Returns a list of all Tags in the current taxonomy, from the root(s) down to TAXONOMY_MAX_DEPTH tags, in tree order. @@ -302,6 +264,14 @@ class ObjectTag(models.Model): " If the tag field is set, then tag.value takes precedence over this field." ), ) + _object_tag_class = models.CharField( + null=True, + max_length=255, + help_text=_( + "Overrides the ObjectTag subclass used to instantiate this ObjectTag." + "Must be a fully-qualified module and class name.", + ), + ) class Meta: indexes = [ @@ -364,6 +334,48 @@ def tag_ref(self) -> str: """ return self.tag.id if self.tag_id else self._value + @property + def object_tag_class(self) -> Type: + """ + Returns the ObjectTag subclass associated with this ObjectTag, or None if none supplied. + + May raise ImportError if a custom object_tag_class cannot be imported. + """ + if self._object_tag_class: + return import_string(self._object_tag_class) + return None + + @object_tag_class.setter + def object_tag_class(self, object_tag_class: Type): + """ + Assigns the given object_tag_class's module path.class to the field. + + Raises ValueError if the given `object_tag_class` is a built-in class; it should be an ObjectTag-like class. + """ + if object_tag_class.__module__ == "builtins": + raise ValueError( + f"object_tag_class {object_tag_class} must be class like ObjectTag" + ) + + # ref: https://stackoverflow.com/a/2020083 + self._object_tag_class = ".".join( + [object_tag_class.__module__, object_tag_class.__qualname__] + ) + + def cast_object_tag(self): + """ + Returns the current object tag cast into its object_tag_class. + """ + try: + ObjectTagClass = self.object_tag_class + if ObjectTagClass: + return ObjectTagClass().copy(self) + except ImportError: + # Log error and continue + log.exception(f"Unable to import custom object_tag_class for {taxonomy}") + + return self + def copy(self, object_tag: "ObjectTag") -> "ObjectTag": """ Copy the fields from the given ObjectTag into the current instance. @@ -388,27 +400,38 @@ def get_lineage(self) -> Lineage: def _check_taxonomy(self): """ - Always returns True. + Returns True if this ObjectTag is linked to a taxonomy. - Subclasses should override this method to perform validation for the particular type of object tag. + Subclasses should override this method to perform any additional validation for the particular type of object tag. """ - return True + # Must be linked to a free-text taxonomy + return self.taxonomy_id def _check_tag(self): """ - Always returns True. + Returns True if this ObjectTag has a valid tag value. - Subclasses should override this method to perform validation for the particular type of object tag. + Subclasses should override this method to perform any additional validation for the particular type of object tag. """ - return True + return self.taxonomy_id and ( + # Open taxonomies only need a value. + (self.taxonomy.allow_free_text and bool(self._value)) + or + # Closed taxonomies need an associated tag in their taxonomy. + ( + not self.taxonomy.allow_free_text + and bool(self.tag_id) + and self.taxonomy_id == self.tag.taxonomy_id + ) + ) def _check_object(self): """ - Always returns True. + Returns True if this ObjectTag has a valid object. - Subclasses should override this method to perform validation for the particular type of object tag. + Subclasses should override this method to perform any additional validation for the particular type of object tag. """ - return True + return self.object_id and self.object_type def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: """ @@ -446,31 +469,29 @@ def resync(self) -> bool: # Locate an enabled taxonomy matching _name, and maybe a tag matching _value if not self.taxonomy_id: - for taxonomy in Taxonomy.objects.filter( - name=self.name, enabled=True - ).order_by("allow_free_text", "id"): - # Closed taxonomies require a tag matching _value, - # and we'd rather match a closed taxonomy than an open one. - # So see if there's a matching tag available in this taxonomy. - tag = self.tag or taxonomy.tag_set.filter(value=self.value).first() - - # Make sure this taxonomy will accept object tags like this. - test_object_tag = cast_object_tag( - ObjectTag( - taxonomy=taxonomy, - tag=tag, - object_id=self.object_id, - object_type=self.object_type, - _name=self.name, - _value=self.value, - ) - ) - if test_object_tag: - self.taxonomy = taxonomy - self.tag = tag - changed = True - break - # If not, try the next one + # Use the linked tag's taxonomy if there is one. + if self.tag_id: + self.taxonomy_id = self.tag.taxonomy_id + changed = True + else: + for taxonomy in Taxonomy.objects.filter( + name=self.name, enabled=True + ).order_by("allow_free_text", "id"): + # Closed taxonomies require a tag matching _value, + # and we'd rather match a closed taxonomy than an open one. + # So see if there's a matching tag available in this taxonomy. + tag = taxonomy.tag_set.filter(value=self.value).first() + + # Make sure this taxonomy will accept object tags like this. + object_tag = self.cast_object_tag() + object_tag.taxonomy = taxonomy + object_tag.tag = tag + if object_tag.is_valid(): + self.taxonomy = taxonomy + self.tag = tag + changed = True + break + # If not, try the next one # Sync the stored _name with the taxonomy.name if self.taxonomy_id and self._name != self.taxonomy.name: @@ -490,95 +511,3 @@ def resync(self) -> bool: changed = True return changed - - -class OpenObjectTag(ObjectTag): - """ - Free-text object tag. - - Only needs a free-text taxonomy and a value to be valid. - """ - - class Meta: - proxy = True - - def _check_taxonomy(self): - """ - Returns True if this ObjectTag has a valid taxonomy. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - # Must be linked to a free-text taxonomy - return self.taxonomy_id and self.taxonomy.allow_free_text - - def _check_tag(self): - """ - Returns True if this ObjectTag has a valid tag value. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - # Open taxonomies don't need an associated tag, but we need a value. - return bool(self._value) - - def _check_object(self): - """ - Returns True if this ObjectTag has a valid object. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - # Must have a valid object id/type: - return self.object_id and self.object_type - - -class ClosedObjectTag(OpenObjectTag): - """ - Object tags linked to a closed taxonomy, where the available tag value options are known. - """ - - class Meta: - proxy = True - - def _check_taxonomy(self): - """ - Returns True if this ObjectTag is linked to a closed taxonomy. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - # Must be linked to a closed taxonomy - return self.taxonomy_id and not self.taxonomy.allow_free_text - - def _check_tag(self): - """ - Returns True if this ObjectTag has a valid tag. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - # Closed taxonomies require a Tag - return bool(self.tag_id) - - def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: - """ - Returns True if this ObjectTag is valid for use with a closed taxonomy. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - - If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. - If `check_tag` is False, then we skip validating the object tag's tag reference. - If `check_object` is False, then we skip validating the object ID/type. - """ - if not super().is_valid( - check_taxonomy=check_taxonomy, - check_tag=check_tag, - check_object=check_object, - ): - return False - - if check_tag and check_taxonomy and (self.tag.taxonomy_id != self.taxonomy_id): - return False - - return True - - -# Register the ObjectTag subclasses in reverse order of how we want them considered. -register_object_tag_class(OpenObjectTag) -register_object_tag_class(ClosedObjectTag) diff --git a/openedx_tagging/core/tagging/registry.py b/openedx_tagging/core/tagging/registry.py deleted file mode 100644 index 11dd0ff25..000000000 --- a/openedx_tagging/core/tagging/registry.py +++ /dev/null @@ -1,47 +0,0 @@ -""" -Registry for object tag classes -""" -import logging -from typing import Iterator, Type - -log = logging.getLogger(__name__) - -# Global registry -_OBJECT_TAG_CLASS_REGISTRY = [] - - -def register_object_tag_class(cls, index=0): - """ - Register a given class as a candidate object tag class. - - By default, inserts the given class at the beginning of the list, so that it will be considered before all - previously-registered classes. Adjust ``index`` to change where the class will be considered. - """ - _OBJECT_TAG_CLASS_REGISTRY.insert(index, cls) - - -def cast_object_tag(object_tag: "ObjectTag") -> "ObjectTag": - """ - Returns the most appropriate ObjectTag subclass for the given attributes. - - If no ObjectTag subclasses are found, then returns None. - """ - # Some taxonomies have custom object tag classes applied. - if object_tag.taxonomy_id: - taxonomy = object_tag.taxonomy - try: - ObjectTagClass = taxonomy.object_tag_class - if ObjectTagClass: - return ObjectTagClass().copy(object_tag) - except ImportError: - # Log error and continue - log.exception(f"Unable to import custom object_tag_class for {taxonomy}") - - # Return the first appropriate object tag class - for ObjectTagClass in _OBJECT_TAG_CLASS_REGISTRY: - cast_object_tag = ObjectTagClass().copy(object_tag) - - if cast_object_tag.is_valid(): - return cast_object_tag - - return None diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 5794e5a90..94ab96ac7 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -27,17 +27,6 @@ def test_create_taxonomy(self): assert getattr(taxonomy, param) == value assert not taxonomy.system_defined assert taxonomy.visible_to_authors - assert taxonomy.object_tag_class is None - - def test_create_taxonomy_bad_object_tag_class(self): - with self.assertRaises(ValueError) as exc: - tagging_api.create_taxonomy( - name="invalid", - object_tag_class=str, - ) - assert "object_tag_class must be class like ObjectTag" in str( - exc.exception - ) def test_get_taxonomy(self): tax1 = tagging_api.get_taxonomy(1) @@ -345,8 +334,21 @@ def test_get_object_tags(self): beta, ] + def test_bad_object_tag_class(self): + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + taxonomy=self.taxonomy, + tags=[self.bacteria.id], + object_id="anthropology101", + object_type="course", + object_tag_class=str, + ) + assert "object_tag_class must be class like ObjectTag" in str( + exc.exception + ) + def test_cast_object_tag(self): - # Create a valid ClosedObjectTag + # Create a valid ObjectTag in a closed taxonomy assert not self.taxonomy.allow_free_text object_tag = ObjectTag.objects.create( object_id="object:id:1", @@ -358,28 +360,5 @@ def test_cast_object_tag(self): assert ( str(object_tag) == repr(object_tag) - == " object:id:1 (life): Life on Earth=Bacteria" - ) - - # Check that changing the taxonomy to an open taxonomy changes the object tag class - open_taxonomy = tagging_api.create_taxonomy( - name="Freetext Life", - allow_free_text=True, - ) - object_tag.taxonomy = open_taxonomy - object_tag.value = "Bacterium" - object_tag = tagging_api.cast_object_tag(object_tag) - assert ( - str(object_tag) - == repr(object_tag) - == " object:id:1 (life): Freetext Life=Bacteria" - ) - - # Check that explicitly changing the object_tag_class also works - object_tag.taxonomy.object_tag_class = ObjectTag - object_tag = tagging_api.cast_object_tag(object_tag) - assert ( - str(object_tag) - == repr(object_tag) - == " object:id:1 (life): Freetext Life=Bacteria" + == " object:id:1 (life): Life on Earth=Bacteria" ) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index e5774f757..5f088c917 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -3,14 +3,7 @@ import ddt from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import ( - ClosedObjectTag, - ObjectTag, - OpenObjectTag, - Tag, - Taxonomy, - cast_object_tag, -) +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy def get_tag(value): @@ -143,15 +136,6 @@ def test_get_tags_shallow_taxonomy(self): with self.assertNumQueries(2): assert taxonomy.get_tags() == tags - def test_get_object_tag_class_invalid(self): - taxonomy = Taxonomy.objects.create( - name="invalid", - _object_tag_class="not.a.valid.class", - ) - # cast_object_tag will fall back to ObjectTag if invalid. - object_tag = cast_object_tag(ObjectTag(taxonomy=taxonomy)) - assert object_tag is None - class TestModelObjectTag(TestTagTaxonomyMixin, TestCase): """ @@ -226,51 +210,50 @@ def test_object_tag_lineage(self): assert object_tag.get_lineage() == ["Another tag"] def test_object_tag_is_valid(self): - # ObjectTags are always valid - object_tag = ObjectTag() - assert object_tag.is_valid() - open_taxonomy = Taxonomy.objects.create( name="Freetext Life", allow_free_text=True, ) - # OpenObjectTags are valid with a free-text taxonomy and a value - object_tag = OpenObjectTag( + # ObjectTags in a free-text taxonomy are valid with a value + object_tag = ObjectTag( taxonomy=self.taxonomy, ) - assert not object_tag.is_valid( + assert object_tag.is_valid( check_taxonomy=True, check_tag=False, check_object=False ) assert not object_tag.is_valid( - check_taxonomy=False, check_tag=True, check_object=False + check_taxonomy=True, check_tag=True, check_object=False ) assert not object_tag.is_valid( - check_taxonomy=False, check_tag=False, check_object=True + check_taxonomy=True, check_tag=False, check_object=True ) object_tag.object_id = "object:id" object_tag.object_type = "life" + assert object_tag.is_valid( + check_taxonomy=True, check_tag=False, check_object=True + ) object_tag.value = "Any text we want" object_tag.taxonomy = open_taxonomy assert object_tag.is_valid() - # ClosedObjectTags require a closed taxonomy and a tag in that taxonomy - object_tag = ClosedObjectTag( + # ObjectTags in a closed taxonomy require a tag in that taxonomy + object_tag = ObjectTag( taxonomy=open_taxonomy, ) - assert not object_tag.is_valid( + assert object_tag.is_valid( check_taxonomy=True, check_tag=False, check_object=False ) assert not object_tag.is_valid( - check_taxonomy=False, check_tag=True, check_object=False + check_taxonomy=True, check_tag=True, check_object=False ) assert not object_tag.is_valid( - check_taxonomy=False, check_tag=False, check_object=True + check_taxonomy=True, check_tag=False, check_object=True ) object_tag.object_id = "object:id" object_tag.object_type = "life" assert object_tag.is_valid( - check_taxonomy=False, check_tag=False, check_object=True + check_taxonomy=True, check_tag=False, check_object=True ) object_tag.taxonomy = self.taxonomy object_tag.tag = Tag.objects.create( @@ -279,13 +262,4 @@ def test_object_tag_is_valid(self): ) assert not object_tag.is_valid() object_tag.tag = self.tag - assert object_tag.is_valid( - check_taxonomy=False, check_tag=False, check_object=True - ) - assert object_tag.is_valid( - check_taxonomy=False, check_tag=True, check_object=True - ) - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) assert object_tag.is_valid() diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index fcd708e64..b965bedc7 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -4,7 +4,7 @@ from django.contrib.auth import get_user_model from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import ClosedObjectTag, ObjectTag, Tag +from openedx_tagging.core.tagging.models import ObjectTag, Tag from .test_models import TestTagTaxonomyMixin @@ -33,7 +33,7 @@ def setUp(self): username="learner", email="learner@example.com", ) - self.object_tag = ClosedObjectTag.objects.create( + self.object_tag = ObjectTag.objects.create( taxonomy=self.taxonomy, tag=self.bacteria, ) From 0807949abcb6e6d363ab0f5470d8c5dd9e097762 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 18 Jul 2023 11:47:18 +0930 Subject: [PATCH 13/14] refactor: allows Taxonomy to be subclassed * adds optional taxonomy_class property+field to Taxonomy * adds Taxonomy cast() method to use this class * oel_tagging.api uses Taxonomy.cast() whenever practical * moves ObjectTag validation back to Taxonomy * removes ObjectTag.resync() logic -- we don't need it yet. * removes ObjectTag.object_type field -- we're not using it for anything. * squashes migrations from previous commits * reduces diff --- docs/decisions/0007-tagging-app.rst | 15 +- openedx_tagging/core/tagging/api.py | 94 +---- .../migrations/0002_auto_20230718_2026.py | 79 ++++ .../0002_taxonomy_system_defined.py | 20 - .../migrations/0003_objecttag_proxies.py | 41 -- .../migrations/0004_tag_cascade_delete.py | 36 -- .../migrations/0005_auto_20230710_0140.py | 30 -- .../migrations/0006_auto_20230717_0200.py | 31 -- openedx_tagging/core/tagging/models.py | 372 ++++++++++++------ .../openedx_tagging/core/tagging/test_api.py | 101 ++--- .../core/tagging/test_models.py | 209 ++++++++-- .../core/tagging/test_rules.py | 1 - 12 files changed, 556 insertions(+), 473 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py delete mode 100644 openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py delete mode 100644 openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py delete mode 100644 openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py delete mode 100644 openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py delete mode 100644 openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py diff --git a/docs/decisions/0007-tagging-app.rst b/docs/decisions/0007-tagging-app.rst index ed536c79e..0346d93f3 100644 --- a/docs/decisions/0007-tagging-app.rst +++ b/docs/decisions/0007-tagging-app.rst @@ -19,7 +19,7 @@ Taxonomy The ``openedx_tagging`` module defines ``openedx_tagging.core.models.Taxonomy``, whose data and functionality are self-contained to the ``openedx_tagging`` app. However in Studio, we need to be able to limit access to some Taxonomy by organization, using the same "course creator" access which limits course creation for an organization to a defined set of users. -So in edx-platform, we will create the ``openedx.features.content_tagging`` app, to contain the models and logic for linking Organization owners to Taxonomies. There's no need to subclass Taxonomy here; we can enforce access using the ``content_tagging.api``. +So in edx-platform, we will create the ``openedx.features.content_tagging`` app, to contain the models and logic for linking Organization owners to Taxonomies. Here, we can subclass ``Taxonomy`` as needed, preferably using proxy models. The APIs are responsible for ensuring that any ``Taxonomy`` instances are cast to the appropriate subclass. ObjectTag ~~~~~~~~~ @@ -27,7 +27,7 @@ ObjectTag Similarly, the ``openedx_tagging`` module defined ``openedx_tagging.core.models.ObjectTag``, also self-contained to the ``openedx_tagging`` app. -But to tag content in the LMS/Studio, we need to enforce ``object_id`` as a CourseKey or UsageKey type. So to do this, we subclass ``ObjectTag``, and use the ``openedx.features.tagging.registry`` to register the subclass, so that it will be picked up when this tagging API creates or resyncs object tags. +But to tag content in the LMS/Studio, we need to enforce ``object_id`` as a CourseKey or UsageKey type. So to do this, we subclass ``ObjectTag``, and use this class when creating object tags for the content taxonomies. Once the ``object_id`` is set, it is not editable, and so this key validation need not happen again. Rejected Alternatives --------------------- @@ -38,14 +38,3 @@ Embed in edx-platform Embedding the logic in edx-platform would provide the content tagging logic specifically required for the MVP. However, we plan to extend tagging to other object types (e.g. People) and contexts (e.g. Marketing), and so a generic, standalone library is preferable in the log run. - - -Subclass Taxonomy -~~~~~~~~~~~~~~~~~ - -Another approach considered was to encapsulate the complexity of ObjectTag validation in the Taxonomy class, and so use subclasses of Taxonomy to validate different types of ObjectTags, and use `multi-table inheritance`_ and django polymorphism when pulling the taxonomies from the database. - -However, because we will have a number of different types of Taxonomies, this proved non-performant. - - --.. _multi-table inheritance: https://docs.djangoproject.com/en/3.2/topics/db/models/#multi-table-inheritance diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 8b593a27d..5ca23c485 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -25,11 +25,12 @@ def create_taxonomy( required=False, allow_multiple=False, allow_free_text=False, + taxonomy_class: Type = None, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. """ - taxonomy = Taxonomy.objects.create( + taxonomy = Taxonomy( name=name, description=description, enabled=enabled, @@ -37,19 +38,27 @@ def create_taxonomy( allow_multiple=allow_multiple, allow_free_text=allow_free_text, ) - return taxonomy + if taxonomy_class: + taxonomy.taxonomy_class = taxonomy_class + taxonomy.save() + return taxonomy.cast() def get_taxonomy(id: int) -> Union[Taxonomy, None]: """ - Returns a Taxonomy of the appropriate subclass which has the given ID. + Returns a Taxonomy cast to the appropriate subclass which has the given ID. """ - return Taxonomy.objects.filter(id=id).first() + taxonomy = Taxonomy.objects.filter(id=id).first() + return taxonomy.cast() if taxonomy else None def get_taxonomies(enabled=True) -> QuerySet: """ Returns a queryset containing the enabled taxonomies, sorted by name. + + We return a QuerySet here for ease of use with Django Rest Framework and other query-based use cases. + So be sure to use `Taxonomy.cast()` to cast these instances to the appropriate subclass before use. + If you want the disabled taxonomies, pass enabled=False. If you want all taxonomies (both enabled and disabled), pass enabled=None. """ @@ -65,14 +74,7 @@ def get_tags(taxonomy: Taxonomy) -> List[Tag]: Note that if the taxonomy allows free-text tags, then the returned list will be empty. """ - return taxonomy.get_tags() - - -def cast_object_tag(object_tag: ObjectTag) -> ObjectTag: - """ - Casts/copies the given object tag data into the ObjectTag subclass most appropriate for this tag. - """ - return object_tag.cast_object_tag() + return taxonomy.cast().get_tags() def resync_object_tags(object_tags: QuerySet = None) -> int: @@ -85,8 +87,7 @@ def resync_object_tags(object_tags: QuerySet = None) -> int: object_tags = ObjectTag.objects.select_related("tag", "taxonomy") num_changed = 0 - for tag in object_tags: - object_tag = cast_object_tag(tag) + for object_tag in object_tags: changed = object_tag.resync() if changed: object_tag.save() @@ -95,7 +96,7 @@ def resync_object_tags(object_tags: QuerySet = None) -> int: def get_object_tags( - object_id: str, object_type: str = None, taxonomy: Taxonomy = None, valid_only=True + object_id: str, taxonomy: Taxonomy = None, valid_only=True ) -> Iterator[ObjectTag]: """ Generates a list of object tags for a given object. @@ -112,13 +113,10 @@ def get_object_tags( .select_related("tag", "taxonomy") .order_by("id") ) - if object_type: - tags = tags.filter(object_type=object_type) if taxonomy: tags = tags.filter(taxonomy=taxonomy) - for tag in tags: - object_tag = cast_object_tag(tag) + for object_tag in tags: if not valid_only or object_tag.is_valid(): yield object_tag @@ -127,8 +125,6 @@ def tag_object( taxonomy: Taxonomy, tags: List, object_id: str, - object_type: str, - object_tag_class: Type = None, ) -> List[ObjectTag]: """ Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags. @@ -139,58 +135,4 @@ def tag_object( Raised ValueError if the proposed tags are invalid for this taxonomy. Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. """ - - if not taxonomy.allow_multiple and len(tags) > 1: - raise ValueError(_(f"Taxonomy ({taxonomy.id}) only allows one tag per object.")) - - if taxonomy.required and len(tags) == 0: - raise ValueError( - _(f"Taxonomy ({taxonomy.id}) requires at least one tag per object.") - ) - - current_tags = { - tag.tag_ref: tag - for tag in ObjectTag.objects.filter( - taxonomy=taxonomy, object_id=object_id, object_type=object_type - ) - } - updated_tags = [] - for tag_ref in tags: - if tag_ref in current_tags: - object_tag = cast_object_tag(current_tags.pop(tag_ref)) - else: - try: - tag = taxonomy.tag_set.get( - id=tag_ref, - ) - value = tag.value - except (ValueError, Tag.DoesNotExist): - # This might be ok, e.g. if taxonomy.allow_free_text. - # We'll validate below before saving. - tag = None - value = tag_ref - - object_tag = ObjectTag( - taxonomy=taxonomy, - object_id=object_id, - object_type=object_type, - tag=tag, - value=value, - name=taxonomy.name, - ) - if object_tag_class: - object_tag.object_tag_class = object_tag_class - object_tag = cast_object_tag(object_tag) - - object_tag.resync() - updated_tags.append(object_tag) - - # Save all updated tags at once to avoid partial updates - for object_tag in updated_tags: - object_tag.save() - - # ...and delete any omitted existing tags - for old_tag in current_tags.values(): - old_tag.delete() - - return updated_tags + return taxonomy.cast().tag_object(tags, object_id) diff --git a/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py b/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py new file mode 100644 index 000000000..c6216a74b --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py @@ -0,0 +1,79 @@ +# Generated by Django 3.2.19 on 2023-07-18 05:54 + +import django.db.models.deletion +from django.db import migrations, models + +import openedx_learning.lib.fields + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="taxonomy", + name="system_defined", + field=models.BooleanField( + default=False, + editable=False, + help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", + ), + ), + migrations.AlterField( + model_name="tag", + name="parent", + field=models.ForeignKey( + default=None, + help_text="Tag that lives one level up from the current tag, forming a hierarchy.", + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="children", + to="oel_tagging.tag", + ), + ), + migrations.AlterField( + model_name="tag", + name="taxonomy", + field=models.ForeignKey( + default=None, + help_text="Namespace and rules for using a given set of tags.", + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="oel_tagging.taxonomy", + ), + ), + migrations.AddField( + model_name="taxonomy", + name="visible_to_authors", + field=models.BooleanField( + default=True, + editable=False, + help_text="Indicates whether this taxonomy should be visible to object authors.", + ), + ), + migrations.RemoveField( + model_name="objecttag", + name="object_type", + ), + migrations.AddField( + model_name="taxonomy", + name="_taxonomy_class", + field=models.CharField( + help_text="Taxonomy subclass used to instantiate this instance.Must be a fully-qualified module and class name.", + max_length=255, + null=True, + ), + ), + migrations.AlterField( + model_name="objecttag", + name="object_id", + field=openedx_learning.lib.fields.MultiCollationCharField( + db_collations={"mysql": "utf8mb4_unicode_ci", "sqlite": "NOCASE"}, + editable=False, + help_text="Identifier for the object being tagged", + max_length=255, + ), + ), + ] diff --git a/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py b/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py deleted file mode 100644 index 6f1beb92e..000000000 --- a/openedx_tagging/core/tagging/migrations/0002_taxonomy_system_defined.py +++ /dev/null @@ -1,20 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-05 04:52 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0001_initial"), - ] - - operations = [ - migrations.AddField( - model_name="taxonomy", - name="system_defined", - field=models.BooleanField( - default=False, - help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", - ), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py b/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py deleted file mode 100644 index faaaa66dc..000000000 --- a/openedx_tagging/core/tagging/migrations/0003_objecttag_proxies.py +++ /dev/null @@ -1,41 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-05 05:07 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0002_taxonomy_system_defined"), - ] - - operations = [ - migrations.CreateModel( - name="OpenObjectTag", - fields=[], - options={ - "proxy": True, - "indexes": [], - "constraints": [], - }, - bases=("oel_tagging.objecttag",), - ), - migrations.AddField( - model_name="taxonomy", - name="_object_tag_class", - field=models.CharField( - help_text="Overrides the default ObjectTag subclass associated with this taxonomy.Must be a fully-qualified module and class name.", - max_length=255, - null=True, - ), - ), - migrations.CreateModel( - name="ClosedObjectTag", - fields=[], - options={ - "proxy": True, - "indexes": [], - "constraints": [], - }, - bases=("oel_tagging.openobjecttag",), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py b/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py deleted file mode 100644 index ebb6f74b2..000000000 --- a/openedx_tagging/core/tagging/migrations/0004_tag_cascade_delete.py +++ /dev/null @@ -1,36 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-06 03:56 - -import django.db.models.deletion -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0003_objecttag_proxies"), - ] - - operations = [ - migrations.AlterField( - model_name="tag", - name="parent", - field=models.ForeignKey( - default=None, - help_text="Tag that lives one level up from the current tag, forming a hierarchy.", - null=True, - on_delete=django.db.models.deletion.CASCADE, - related_name="children", - to="oel_tagging.tag", - ), - ), - migrations.AlterField( - model_name="tag", - name="taxonomy", - field=models.ForeignKey( - default=None, - help_text="Namespace and rules for using a given set of tags.", - null=True, - on_delete=django.db.models.deletion.CASCADE, - to="oel_tagging.taxonomy", - ), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py b/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py deleted file mode 100644 index 4b6210ec2..000000000 --- a/openedx_tagging/core/tagging/migrations/0005_auto_20230710_0140.py +++ /dev/null @@ -1,30 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-10 06:40 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0004_tag_cascade_delete"), - ] - - operations = [ - migrations.AddField( - model_name="taxonomy", - name="visible_to_authors", - field=models.BooleanField( - default=True, - editable=False, - help_text="Indicates whether this taxonomy should be visible to object authors.", - ), - ), - migrations.AlterField( - model_name="taxonomy", - name="system_defined", - field=models.BooleanField( - default=False, - editable=False, - help_text="Indicates that tags and metadata for this taxonomy are maintained by the system; taxonomy admins will not be permitted to modify them.", - ), - ), - ] diff --git a/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py b/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py deleted file mode 100644 index 48832c1a1..000000000 --- a/openedx_tagging/core/tagging/migrations/0006_auto_20230717_0200.py +++ /dev/null @@ -1,31 +0,0 @@ -# Generated by Django 3.2.19 on 2023-07-17 07:00 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("oel_tagging", "0005_auto_20230710_0140"), - ] - - operations = [ - migrations.DeleteModel( - name="ClosedObjectTag", - ), - migrations.DeleteModel( - name="OpenObjectTag", - ), - migrations.RemoveField( - model_name="taxonomy", - name="_object_tag_class", - ), - migrations.AddField( - model_name="objecttag", - name="_object_tag_class", - field=models.CharField( - help_text="Overrides the ObjectTag subclass used to instantiate this ObjectTag.Must be a fully-qualified module and class name.", - max_length=255, - null=True, - ), - ), - ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index ded22db82..f9f1f9302 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -1,5 +1,6 @@ """ Tagging app data models """ -from typing import List, Type +import logging +from typing import List, Type, Union from django.db import models from django.utils.module_loading import import_string @@ -7,6 +8,9 @@ from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field +log = logging.getLogger(__name__) + + # Maximum depth allowed for a hierarchical taxonomy's tree of tags. TAXONOMY_MAX_DEPTH = 3 @@ -151,6 +155,14 @@ class Taxonomy(models.Model): "Indicates whether this taxonomy should be visible to object authors." ), ) + _taxonomy_class = models.CharField( + null=True, + max_length=255, + help_text=_( + "Taxonomy subclass used to instantiate this instance." + "Must be a fully-qualified module and class name.", + ), + ) class Meta: verbose_name_plural = "Taxonomies" @@ -167,6 +179,71 @@ def __str__(self): """ return f"<{self.__class__.__name__}> ({self.id}) {self.name}" + @property + def taxonomy_class(self) -> Type: + """ + Returns the Taxonomy subclass associated with this instance, or None if none supplied. + + May raise ImportError if a custom taxonomy_class cannot be imported. + """ + if self._taxonomy_class: + return import_string(self._taxonomy_class) + return None + + @taxonomy_class.setter + def taxonomy_class(self, taxonomy_class: Union[Type, None]): + """ + Assigns the given taxonomy_class's module path.class to the field. + + Raises ValueError if the given `taxonomy_class` is a built-in class; it should be a Taxonomy-like class. + """ + if taxonomy_class: + if taxonomy_class.__module__ == "builtins": + raise ValueError( + f"Unable to assign taxonomy_class for {self}: {taxonomy_class} must be a class like Taxonomy" + ) + + # ref: https://stackoverflow.com/a/2020083 + self._taxonomy_class = ".".join( + [taxonomy_class.__module__, taxonomy_class.__qualname__] + ) + else: + self._taxonomy_class = None + + def cast(self): + """ + Returns the current Taxonomy instance cast into its taxonomy_class. + + If no taxonomy_class is set, then just returns self. + """ + try: + TaxonomyClass = self.taxonomy_class + if TaxonomyClass and not isinstance(self, TaxonomyClass): + return TaxonomyClass().copy(self) + except ImportError: + # Log error and continue + log.exception( + f"Unable to import taxonomy_class for {self}: {self._taxonomy_class}" + ) + + return self + + def copy(self, taxonomy: "Taxonomy") -> "Taxonomy": + """ + Copy the fields from the given Taxonomy into the current instance. + """ + self.id = taxonomy.id + self.name = taxonomy.name + self.description = taxonomy.description + self.enabled = taxonomy.enabled + self.required = taxonomy.required + self.allow_multiple = taxonomy.allow_multiple + self.allow_free_text = taxonomy.allow_free_text + self.system_defined = taxonomy.system_defined + self.visible_to_authors = taxonomy.visible_to_authors + self._taxonomy_class = taxonomy._taxonomy_class + return self + def get_tags(self) -> List[Tag]: """ Returns a list of all Tags in the current taxonomy, from the root(s) down to TAXONOMY_MAX_DEPTH tags, in tree order. @@ -201,6 +278,137 @@ def get_tags(self) -> List[Tag]: break return tags + def validate_object_tag( + self, + object_tag: "ObjectTag", + check_taxonomy=True, + check_tag=True, + check_object=True, + ) -> bool: + """ + Returns True if the given object tag is valid for the current Taxonomy. + + Subclasses should override the internal _validate* methods to perform their own validation checks, e.g. against + dynamically generated tag lists. + + If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. + If `check_tag` is False, then we skip validating the object tag's tag reference. + If `check_object` is False, then we skip validating the object ID/type. + """ + if check_taxonomy and not self._check_taxonomy(object_tag): + return False + + if check_tag and not self._check_tag(object_tag): + return False + + if check_object and not self._check_object(object_tag): + return False + + return True + + def _check_taxonomy( + self, + object_tag: "ObjectTag", + ) -> bool: + """ + Returns True if the given object tag is valid for the current Taxonomy. + + Subclasses can override this method to perform their own taxonomy validation checks. + """ + # Must be linked to this taxonomy + return object_tag.taxonomy_id and object_tag.taxonomy_id == self.id + + def _check_tag( + self, + object_tag: "ObjectTag", + ) -> bool: + """ + Returns True if the given object tag's value is valid for the current Taxonomy. + + Subclasses can override this method to perform their own taxonomy validation checks. + """ + # Open taxonomies only need a value. + if self.allow_free_text: + return bool(object_tag.value) + + # Closed taxonomies need an associated tag in this taxonomy + return object_tag.tag_id and object_tag.tag.taxonomy_id == self.id + + def _check_object( + self, + object_tag: "ObjectTag", + ) -> bool: + """ + Returns True if the given object tag's object is valid for the current Taxonomy. + + Subclasses can override this method to perform their own taxonomy validation checks. + """ + return bool(object_tag.object_id) + + def tag_object( + self, + tags: List, + object_id: str, + ) -> List["ObjectTag"]: + """ + Replaces the existing ObjectTag entries for the current taxonomy + object_id with the given list of tags. + If self.allows_free_text, then the list should be a list of tag values. + Otherwise, it should be a list of existing Tag IDs. + Raised ValueError if the proposed tags are invalid for this taxonomy. + Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. + """ + + if not self.allow_multiple and len(tags) > 1: + raise ValueError(_(f"Taxonomy ({self.id}) only allows one tag per object.")) + + if self.required and len(tags) == 0: + raise ValueError( + _(f"Taxonomy ({self.id}) requires at least one tag per object.") + ) + + current_tags = { + tag.tag_ref: tag + for tag in ObjectTag.objects.filter( + taxonomy=self, + object_id=object_id, + ) + } + updated_tags = [] + for tag_ref in tags: + if tag_ref in current_tags: + object_tag = current_tags.pop(tag_ref) + else: + object_tag = ObjectTag( + taxonomy=self, + object_id=object_id, + ) + + try: + object_tag.tag = self.tag_set.get( + id=tag_ref, + ) + except (ValueError, Tag.DoesNotExist): + # This might be ok, e.g. if self.allow_free_text. + # We'll validate below before saving. + object_tag.value = tag_ref + + 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.values(): + old_tag.delete() + + return updated_tags + class ObjectTag(models.Model): """ @@ -223,12 +431,9 @@ class ObjectTag(models.Model): id = models.BigAutoField(primary_key=True) object_id = case_insensitive_char_field( max_length=255, + editable=False, help_text=_("Identifier for the object being tagged"), ) - object_type = case_insensitive_char_field( - max_length=255, - help_text=_("Type of object being tagged"), - ) taxonomy = models.ForeignKey( Taxonomy, null=True, @@ -264,14 +469,6 @@ class ObjectTag(models.Model): " If the tag field is set, then tag.value takes precedence over this field." ), ) - _object_tag_class = models.CharField( - null=True, - max_length=255, - help_text=_( - "Overrides the ObjectTag subclass used to instantiate this ObjectTag." - "Must be a fully-qualified module and class name.", - ), - ) class Meta: indexes = [ @@ -288,7 +485,7 @@ def __str__(self): """ User-facing string representation of an ObjectTag. """ - return f"<{self.__class__.__name__}> {self.object_id} ({self.object_type}): {self.name}={self.value}" + return f"<{self.__class__.__name__}> {self.object_id}: {self.name}={self.value}" @property def name(self) -> str: @@ -334,60 +531,13 @@ def tag_ref(self) -> str: """ return self.tag.id if self.tag_id else self._value - @property - def object_tag_class(self) -> Type: - """ - Returns the ObjectTag subclass associated with this ObjectTag, or None if none supplied. - - May raise ImportError if a custom object_tag_class cannot be imported. - """ - if self._object_tag_class: - return import_string(self._object_tag_class) - return None - - @object_tag_class.setter - def object_tag_class(self, object_tag_class: Type): - """ - Assigns the given object_tag_class's module path.class to the field. - - Raises ValueError if the given `object_tag_class` is a built-in class; it should be an ObjectTag-like class. - """ - if object_tag_class.__module__ == "builtins": - raise ValueError( - f"object_tag_class {object_tag_class} must be class like ObjectTag" - ) - - # ref: https://stackoverflow.com/a/2020083 - self._object_tag_class = ".".join( - [object_tag_class.__module__, object_tag_class.__qualname__] - ) - - def cast_object_tag(self): - """ - Returns the current object tag cast into its object_tag_class. + def is_valid(self) -> bool: """ - try: - ObjectTagClass = self.object_tag_class - if ObjectTagClass: - return ObjectTagClass().copy(self) - except ImportError: - # Log error and continue - log.exception(f"Unable to import custom object_tag_class for {taxonomy}") - - return self + Returns True if this ObjectTag represents a valid taxonomy tag. - def copy(self, object_tag: "ObjectTag") -> "ObjectTag": - """ - Copy the fields from the given ObjectTag into the current instance. + A valid ObjectTag must be linked to a Taxonomy, and be a valid tag in that taxonomy. """ - self.id = object_tag.id - self.object_id = object_tag.object_id - self.object_type = object_tag.object_type - self.taxonomy = object_tag.taxonomy - self.tag = object_tag.tag - self._name = object_tag._name - self._value = object_tag._value - return self + return self.taxonomy_id and self.taxonomy.validate_object_tag(self) def get_lineage(self) -> Lineage: """ @@ -398,60 +548,6 @@ def get_lineage(self) -> Lineage: """ return self.tag.get_lineage() if self.tag_id else [self._value] - def _check_taxonomy(self): - """ - Returns True if this ObjectTag is linked to a taxonomy. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - # Must be linked to a free-text taxonomy - return self.taxonomy_id - - def _check_tag(self): - """ - Returns True if this ObjectTag has a valid tag value. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - return self.taxonomy_id and ( - # Open taxonomies only need a value. - (self.taxonomy.allow_free_text and bool(self._value)) - or - # Closed taxonomies need an associated tag in their taxonomy. - ( - not self.taxonomy.allow_free_text - and bool(self.tag_id) - and self.taxonomy_id == self.tag.taxonomy_id - ) - ) - - def _check_object(self): - """ - Returns True if this ObjectTag has a valid object. - - Subclasses should override this method to perform any additional validation for the particular type of object tag. - """ - return self.object_id and self.object_type - - def is_valid(self, check_taxonomy=True, check_tag=True, check_object=True) -> bool: - """ - Returns True if this ObjectTag is valid. - - If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. - If `check_tag` is False, then we skip validating the object tag's tag reference. - If `check_object` is False, then we skip validating the object ID/type. - """ - if check_taxonomy and not self._check_taxonomy(): - return False - - if check_tag and not self._check_tag(): - return False - - if check_object and not self._check_object(): - return False - - return True - def resync(self) -> bool: """ Reconciles the stored ObjectTag properties with any changes made to its associated taxonomy or tag. @@ -461,8 +557,6 @@ def resync(self) -> bool: It's also useful for a set of ObjectTags are imported from an external source prior to when a Taxonomy exists to validate or store its available Tags. - Subclasses may override this method to perform any additional syncing for the particular type of object tag. - Returns True if anything was changed, False otherwise. """ changed = False @@ -477,21 +571,24 @@ def resync(self) -> bool: 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. - object_tag = self.cast_object_tag() - object_tag.taxonomy = taxonomy - object_tag.tag = tag - if object_tag.is_valid(): - self.taxonomy = taxonomy - self.tag = tag + self.taxonomy = taxonomy + self.tag = tag + if taxonomy.validate_object_tag(self): changed = True break - # If not, try the next one + # If not, undo those changes and try the next one + else: + self.taxonomy = None + self.tag = None # Sync the stored _name with the taxonomy.name if self.taxonomy_id and self._name != self.taxonomy.name: @@ -511,3 +608,22 @@ def resync(self) -> bool: changed = True return changed + + @classmethod + def cast(cls, object_tag: "ObjectTag") -> "ObjectTag": + """ + Returns a cls instance with the same properties as the given ObjectTag. + """ + return cls().copy(object_tag) + + def copy(self, object_tag: "ObjectTag") -> "ObjectTag": + """ + Copy the fields from the given Taxonomy into the current instance. + """ + self.id = object_tag.id + 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 + return self diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 94ab96ac7..86639d445 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -28,6 +28,14 @@ def test_create_taxonomy(self): assert not taxonomy.system_defined assert taxonomy.visible_to_authors + def test_bad_taxonomy_class(self): + with self.assertRaises(ValueError) as exc: + tagging_api.create_taxonomy( + name="Bad class", + taxonomy_class=str, + ) + assert " must be a class like Taxonomy" in str(exc.exception) + def test_get_taxonomy(self): tax1 = tagging_api.get_taxonomy(1) assert tax1 == self.taxonomy @@ -74,13 +82,11 @@ def check_object_tag(self, object_tag, taxonomy, tag, name, value): def test_resync_object_tags(self): missing_links = ObjectTag.objects.create( object_id="abc", - object_type="alpha", _name=self.taxonomy.name, _value=self.mammalia.value, ) changed_links = ObjectTag.objects.create( object_id="def", - object_type="alpha", taxonomy=self.taxonomy, tag=self.mammalia, ) @@ -89,7 +95,6 @@ def test_resync_object_tags(self): changed_links.save() no_changes = ObjectTag.objects.create( object_id="ghi", - object_type="beta", taxonomy=self.taxonomy, tag=self.mammalia, ) @@ -108,6 +113,25 @@ def test_resync_object_tags(self): changed = tagging_api.resync_object_tags() assert changed == 0 + # 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" + ) + + # 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): @@ -147,7 +171,7 @@ def test_resync_object_tags(self): # Ensure the resync prefers the closed taxonomy with the matching tag changed = tagging_api.resync_object_tags( - ObjectTag.objects.filter(object_type="alpha") + ObjectTag.objects.filter(object_id__in=["abc", "def"]) ) assert changed == 2 @@ -163,7 +187,7 @@ def test_resync_object_tags(self): no_changes.value = "Anamelia" no_changes.save() changed = tagging_api.resync_object_tags( - ObjectTag.objects.filter(object_type="beta") + ObjectTag.objects.filter(id=no_changes.id) ) assert changed == 1 self.check_object_tag( @@ -195,7 +219,6 @@ def test_tag_object(self): self.taxonomy, tag_list, "biology101", - "course", ) # Ensure the expected number of tags exist in the database @@ -204,7 +227,6 @@ def test_tag_object(self): tagging_api.get_object_tags( taxonomy=self.taxonomy, object_id="biology101", - object_type="course", ) ) == object_tags @@ -217,7 +239,6 @@ def test_tag_object(self): assert object_tag.taxonomy == self.taxonomy assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" - assert object_tag.object_type == "course" def test_tag_object_free_text(self): self.taxonomy.allow_free_text = True @@ -226,7 +247,6 @@ def test_tag_object_free_text(self): self.taxonomy, ["Eukaryota Xenomorph"], "biology101", - "course", ) assert len(object_tags) == 1 object_tag = object_tags[0] @@ -236,7 +256,6 @@ def test_tag_object_free_text(self): assert object_tag.tag_ref == "Eukaryota Xenomorph" assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] assert object_tag.object_id == "biology101" - assert object_tag.object_type == "course" def test_tag_object_no_multiple(self): with self.assertRaises(ValueError) as exc: @@ -244,7 +263,6 @@ def test_tag_object_no_multiple(self): self.taxonomy, ["A", "B"], "biology101", - "course", ) assert "only allows one tag per object" in str(exc.exception) @@ -256,32 +274,31 @@ def test_tag_object_required(self): self.taxonomy, [], "biology101", - "course", ) assert "requires at least one tag per object" in str(exc.exception) def test_tag_object_invalid_tag(self): - object_tag = tagging_api.tag_object( - self.taxonomy, - ["Eukaryota Xenomorph"], - "biology101", - "course", - )[0] - assert type(object_tag) == ObjectTag # pylint: disable=unidiomatic-typecheck + with self.assertRaises(ValueError) as exc: + tagging_api.tag_object( + self.taxonomy, + ["Eukaryota Xenomorph"], + "biology101", + ) + assert "Invalid object tag for taxonomy (1): Eukaryota Xenomorph" in str( + exc.exception + ) def test_get_object_tags(self): # Alpha tag has no taxonomy - alpha = ObjectTag(object_id="abc", object_type="alpha") + alpha = ObjectTag(object_id="abc") alpha.name = self.taxonomy.name alpha.value = self.mammalia.value alpha.save() # Beta tag has a closed taxonomy beta = ObjectTag.objects.create( object_id="abc", - object_type="beta", taxonomy=self.taxonomy, ) - beta = tagging_api.cast_object_tag(beta) # Fetch all the tags for a given object ID assert list( @@ -312,17 +329,6 @@ def test_get_object_tags(self): beta, ] - # Fetch all the tags for alpha-type objects - assert list( - tagging_api.get_object_tags( - object_id="abc", - object_type="alpha", - valid_only=False, - ) - ) == [ - alpha, - ] - # Fetch all the tags for a given object ID + taxonomy assert list( tagging_api.get_object_tags( @@ -333,32 +339,3 @@ def test_get_object_tags(self): ) == [ beta, ] - - def test_bad_object_tag_class(self): - with self.assertRaises(ValueError) as exc: - tagging_api.tag_object( - taxonomy=self.taxonomy, - tags=[self.bacteria.id], - object_id="anthropology101", - object_type="course", - object_tag_class=str, - ) - assert "object_tag_class must be class like ObjectTag" in str( - exc.exception - ) - - def test_cast_object_tag(self): - # Create a valid ObjectTag in a closed taxonomy - assert not self.taxonomy.allow_free_text - object_tag = ObjectTag.objects.create( - object_id="object:id:1", - object_type="life", - taxonomy=self.taxonomy, - tag=self.bacteria, - ) - object_tag = tagging_api.cast_object_tag(object_tag) - assert ( - str(object_tag) - == repr(object_tag) - == " object:id:1 (life): Life on Earth=Bacteria" - ) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 5f088c917..bc7ce2140 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -78,6 +78,39 @@ def setup_tag_depths(self): tag.depth = 2 +class TestTaxonomySubclassA(Taxonomy): + """ + Model A for testing the taxonomy subclass casting. + """ + + class Meta: + managed = False + proxy = True + app_label = "oel_tagging" + + +class TestTaxonomySubclassB(TestTaxonomySubclassA): + """ + Model B for testing the taxonomy subclass casting. + """ + + class Meta: + managed = False + proxy = True + app_label = "oel_tagging" + + +class TestObjectTagSubclass(ObjectTag): + """ + Model for testing the ObjectTag copy. + """ + + class Meta: + managed = False + proxy = True + app_label = "oel_tagging" + + @ddt.ddt class TestModelTagTaxonomy(TestTagTaxonomyMixin, TestCase): """ @@ -99,6 +132,45 @@ def test_representations(self): ) assert str(self.bacteria) == repr(self.bacteria) == " (1) Bacteria" + def test_taxonomy_cast(self): + for subclass in ( + TestTaxonomySubclassA, + # Ensure that casting to a sub-subclass works as expected + TestTaxonomySubclassB, + # and that we can un-set the subclass + None, + ): + self.taxonomy.taxonomy_class = subclass + cast_taxonomy = self.taxonomy.cast() + if subclass: + expected_class = subclass.__name__ + else: + expected_class = "Taxonomy" + assert self.taxonomy == cast_taxonomy + assert ( + str(cast_taxonomy) + == repr(cast_taxonomy) + == f"<{expected_class}> (1) Life on Earth" + ) + + def test_taxonomy_cast_import_error(self): + taxonomy = Taxonomy.objects.create( + name="Invalid cast", _taxonomy_class="not.a.class" + ) + # Error is logged, but ignored. + cast_taxonomy = taxonomy.cast() + assert cast_taxonomy == taxonomy + assert ( + str(cast_taxonomy) + == repr(cast_taxonomy) + == f" ({taxonomy.id}) Invalid cast" + ) + + def test_taxonomy_cast_bad_value(self): + with self.assertRaises(ValueError) as exc: + self.taxonomy.taxonomy_class = str + assert " must be a class like Taxonomy" in str(exc.exception) + @ddt.data( # Root tags just return their own value ("bacteria", ["Bacteria"]), @@ -147,11 +219,25 @@ def setUp(self): self.tag = self.bacteria self.object_tag = ObjectTag.objects.create( object_id="object:id:1", - object_type="life", taxonomy=self.taxonomy, tag=self.tag, ) + def test_representations(self): + assert ( + str(self.object_tag) + == repr(self.object_tag) + == " object:id:1: Life on Earth=Bacteria" + ) + + def test_cast(self): + copy_tag = TestObjectTagSubclass.cast(self.object_tag) + assert ( + str(copy_tag) + == repr(copy_tag) + == " object:id:1: Life on Earth=Bacteria" + ) + def test_object_tag_name(self): # ObjectTag's name defaults to its taxonomy's name assert self.object_tag.name == self.taxonomy.name @@ -171,7 +257,6 @@ def test_object_tag_value(self): # ObjectTag's value defaults to its tag's value object_tag = ObjectTag.objects.create( object_id="object:id", - object_type="any_old_object", taxonomy=self.taxonomy, tag=self.tag, ) @@ -192,7 +277,6 @@ def test_object_tag_lineage(self): # ObjectTag's value defaults to its tag's lineage object_tag = ObjectTag.objects.create( object_id="object:id", - object_type="any_old_object", taxonomy=self.taxonomy, tag=self.tag, ) @@ -215,46 +299,21 @@ def test_object_tag_is_valid(self): allow_free_text=True, ) - # ObjectTags in a free-text taxonomy are valid with a value object_tag = ObjectTag( taxonomy=self.taxonomy, ) - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=True, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) - object_tag.object_id = "object:id" - object_tag.object_type = "life" - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) + # ObjectTag will only be valid for its taxonomy + assert not open_taxonomy.validate_object_tag(object_tag) + + # ObjectTags in a free-text taxonomy are valid with a value + assert not object_tag.is_valid() object_tag.value = "Any text we want" object_tag.taxonomy = open_taxonomy + assert not object_tag.is_valid() + object_tag.object_id = "object:id" assert object_tag.is_valid() # ObjectTags in a closed taxonomy require a tag in that taxonomy - object_tag = ObjectTag( - taxonomy=open_taxonomy, - ) - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=True, check_object=False - ) - assert not object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) - object_tag.object_id = "object:id" - object_tag.object_type = "life" - assert object_tag.is_valid( - check_taxonomy=True, check_tag=False, check_object=True - ) object_tag.taxonomy = self.taxonomy object_tag.tag = Tag.objects.create( taxonomy=self.system_taxonomy, @@ -263,3 +322,83 @@ def test_object_tag_is_valid(self): assert not object_tag.is_valid() object_tag.tag = self.tag assert object_tag.is_valid() + + def test_tag_object(self): + self.taxonomy.allow_multiple = True + + test_tags = [ + [ + self.archaea.id, + self.eubacteria.id, + self.chordata.id, + ], + [ + self.archaebacteria.id, + self.chordata.id, + ], + [ + self.archaea.id, + self.archaebacteria.id, + ], + ] + + # 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, + "biology101", + ) + + # Ensure the expected number of tags exist in the database + assert 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.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( + ["Eukaryota Xenomorph"], + "biology101", + ) + assert len(object_tags) == 1 + object_tag = object_tags[0] + assert object_tag.is_valid + assert object_tag.taxonomy == self.taxonomy + assert object_tag.name == self.taxonomy.name + assert object_tag.tag_ref == "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) + + 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) + + 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) diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index b965bedc7..577c594f3 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -37,7 +37,6 @@ def setUp(self): taxonomy=self.taxonomy, tag=self.bacteria, ) - self.object_tag.resync() self.object_tag.save() # Taxonomy From 1094d24c5f99a375144b13171c88efec27a9244b Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 19 Jul 2023 11:02:21 +0930 Subject: [PATCH 14/14] fix: Taxonomy._taxonomy_class must be a subclass of Taxonomy also updates some docs as per PR review. --- docs/decisions/0007-tagging-app.rst | 2 +- .../tagging/migrations/0002_auto_20230718_2026.py | 2 +- openedx_tagging/core/tagging/models.py | 14 +++++++------- tests/openedx_tagging/core/tagging/test_api.py | 2 +- tests/openedx_tagging/core/tagging/test_models.py | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/decisions/0007-tagging-app.rst b/docs/decisions/0007-tagging-app.rst index 0346d93f3..9993a49bb 100644 --- a/docs/decisions/0007-tagging-app.rst +++ b/docs/decisions/0007-tagging-app.rst @@ -27,7 +27,7 @@ ObjectTag Similarly, the ``openedx_tagging`` module defined ``openedx_tagging.core.models.ObjectTag``, also self-contained to the ``openedx_tagging`` app. -But to tag content in the LMS/Studio, we need to enforce ``object_id`` as a CourseKey or UsageKey type. So to do this, we subclass ``ObjectTag``, and use this class when creating object tags for the content taxonomies. Once the ``object_id`` is set, it is not editable, and so this key validation need not happen again. +But to tag content in the LMS/Studio, we need to enforce ``object_id`` as a CourseKey or UsageKey type. So to do this, we subclass ``ObjectTag``, and use this class when creating content object tags. Once the ``object_id`` is set, it is not editable, and so this key validation need not happen again. Rejected Alternatives --------------------- diff --git a/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py b/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py index c6216a74b..d0d14c938 100644 --- a/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py +++ b/openedx_tagging/core/tagging/migrations/0002_auto_20230718_2026.py @@ -61,7 +61,7 @@ class Migration(migrations.Migration): model_name="taxonomy", name="_taxonomy_class", field=models.CharField( - help_text="Taxonomy subclass used to instantiate this instance.Must be a fully-qualified module and class name.", + help_text="Taxonomy subclass used to instantiate this instance; must be a fully-qualified module and class name. If the module/class cannot be imported, an error is logged and the base Taxonomy class is used instead.", max_length=255, null=True, ), diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index f9f1f9302..3c8222bcd 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -159,8 +159,8 @@ class Taxonomy(models.Model): null=True, max_length=255, help_text=_( - "Taxonomy subclass used to instantiate this instance." - "Must be a fully-qualified module and class name.", + "Taxonomy subclass used to instantiate this instance; must be a fully-qualified module and class name." + " If the module/class cannot be imported, an error is logged and the base Taxonomy class is used instead." ), ) @@ -195,12 +195,12 @@ def taxonomy_class(self, taxonomy_class: Union[Type, None]): """ Assigns the given taxonomy_class's module path.class to the field. - Raises ValueError if the given `taxonomy_class` is a built-in class; it should be a Taxonomy-like class. + Must be a subclass of Taxonomy, or raises a ValueError. """ if taxonomy_class: - if taxonomy_class.__module__ == "builtins": + if not issubclass(taxonomy_class, Taxonomy): raise ValueError( - f"Unable to assign taxonomy_class for {self}: {taxonomy_class} must be a class like Taxonomy" + f"Unable to assign taxonomy_class for {self}: {taxonomy_class} must be a subclass of Taxonomy" ) # ref: https://stackoverflow.com/a/2020083 @@ -214,7 +214,7 @@ def cast(self): """ Returns the current Taxonomy instance cast into its taxonomy_class. - If no taxonomy_class is set, then just returns self. + If no taxonomy_class is set, or if we're unable to import it, then just returns self. """ try: TaxonomyClass = self.taxonomy_class @@ -618,7 +618,7 @@ def cast(cls, object_tag: "ObjectTag") -> "ObjectTag": def copy(self, object_tag: "ObjectTag") -> "ObjectTag": """ - Copy the fields from the given Taxonomy into the current instance. + Copy the fields from the given ObjectTag into the current instance. """ self.id = object_tag.id self.tag = object_tag.tag diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 86639d445..798b4826b 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -34,7 +34,7 @@ def test_bad_taxonomy_class(self): name="Bad class", taxonomy_class=str, ) - assert " must be a class like Taxonomy" in str(exc.exception) + assert " must be a subclass of Taxonomy" in str(exc.exception) def test_get_taxonomy(self): tax1 = tagging_api.get_taxonomy(1) diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index bc7ce2140..65af41eaa 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -169,7 +169,7 @@ def test_taxonomy_cast_import_error(self): def test_taxonomy_cast_bad_value(self): with self.assertRaises(ValueError) as exc: self.taxonomy.taxonomy_class = str - assert " must be a class like Taxonomy" in str(exc.exception) + assert " must be a subclass of Taxonomy" in str(exc.exception) @ddt.data( # Root tags just return their own value @@ -368,7 +368,7 @@ def test_tag_object_free_text(self): object_tags = self.taxonomy.tag_object( ["Eukaryota Xenomorph"], "biology101", - ) + ) assert len(object_tags) == 1 object_tag = object_tags[0] assert object_tag.is_valid