From 9a913882ec621ab3d5131b8b9665bba8d12b51fe Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 21:47:12 +0930 Subject: [PATCH 1/3] feat: uses InheritanceManager for Taxonomy * Adds dependency on django-model-utils (which is already a dependency of edx-platform) * Updates api.get_taxonomies and adds api.get_taxonomy, which return the correct subclass of Taxonomy * Adds a simple SystemTaxonomy base class to verify ^ * Updates tests to ensure correct classes are used, and no extra queries are required for this functionality --- openedx_tagging/core/tagging/api.py | 11 ++- .../tagging/migrations/0002_systemtaxonomy.py | 30 ++++++++ openedx_tagging/core/tagging/models.py | 59 ++++++++++++++- requirements/base.in | 2 + requirements/base.txt | 3 + requirements/dev.txt | 3 + requirements/doc.txt | 3 + requirements/quality.txt | 3 + requirements/test.txt | 3 + .../openedx_tagging/core/tagging/test_api.py | 22 +++--- .../core/tagging/test_models.py | 71 +++++++++++-------- .../core/tagging/test_rules.py | 15 ++-- 12 files changed, 175 insertions(+), 50 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0002_systemtaxonomy.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 6b9bf2a04..7bd869bd0 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 List, Type +from typing import List, Type, Union from django.db.models import QuerySet from django.utils.translation import gettext_lazy as _ @@ -45,12 +45,19 @@ def get_taxonomies(enabled=True) -> QuerySet: If you want the disabled taxonomies, pass enabled=False. If you want all taxonomies (both enabled and disabled), pass enabled=None. """ - queryset = Taxonomy.objects.order_by("name", "id") + queryset = Taxonomy.objects.order_by("name", "id").select_subclasses() if enabled is None: return queryset.all() return queryset.filter(enabled=enabled) +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).select_subclasses().first() + + def get_tags(taxonomy: Taxonomy) -> List[Tag]: """ Returns a list of predefined tags for the given taxonomy. diff --git a/openedx_tagging/core/tagging/migrations/0002_systemtaxonomy.py b/openedx_tagging/core/tagging/migrations/0002_systemtaxonomy.py new file mode 100644 index 000000000..da4338b41 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0002_systemtaxonomy.py @@ -0,0 +1,30 @@ +# Generated by Django 3.2.19 on 2023-06-28 10:19 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0001_initial"), + ] + + operations = [ + migrations.CreateModel( + name="SystemTaxonomy", + fields=[ + ( + "taxonomy_ptr", + models.OneToOneField( + auto_created=True, + on_delete=django.db.models.deletion.CASCADE, + parent_link=True, + primary_key=True, + serialize=False, + to="oel_tagging.taxonomy", + ), + ), + ], + bases=("oel_tagging.taxonomy",), + ), + ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index c90f8a34a..a802fdfc0 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -3,10 +3,10 @@ from django.db import models from django.utils.translation import gettext_lazy as _ +from model_utils.managers import InheritanceManager 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 @@ -93,11 +93,19 @@ def get_lineage(self) -> Lineage: return lineage +class TaxonomyManager(InheritanceManager): + """ + Base Taxonomy class uses InheritanceManager to help instantiate subclasses during queries. + """ + + class Taxonomy(models.Model): """ Represents a namespace and rules for a group of tags which can be applied to a particular Open edX object. """ + objects = TaxonomyManager() + id = models.BigAutoField(primary_key=True) name = case_insensitive_char_field( null=False, @@ -140,6 +148,18 @@ class Taxonomy(models.Model): class Meta: verbose_name_plural = "Taxonomies" + def __repr__(self): + """ + Developer-facing representation of a Taxonomy. + """ + return str(self) + + def __str__(self): + """ + User-facing string representation of a Taxonomy. + """ + return f"<{self.__class__.__name__}> {self.name}" + @property def system_defined(self) -> bool: """ @@ -351,6 +371,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.object_id} ({self.object_type}): {self.name}={self.value}" + @property def name(self) -> str: """ @@ -402,7 +434,16 @@ def is_valid(self) -> bool: A valid ObjectTag must be linked to a Taxonomy, and be a valid tag in that taxonomy. """ - return self.taxonomy_id and self.taxonomy.validate_object_tag(self) + if self.taxonomy_id: + # self.taxonomy doesn't return the correct subclass, so we must fetch it + # FIXME: + # * cache to prevent re-fetching the taxonomy + # * discourage people from using self.taxonomy + taxonomy = ( + Taxonomy.objects.filter(id=self.taxonomy_id).select_subclasses().first() + ) + return taxonomy.validate_object_tag(self) + return False def get_lineage(self) -> Lineage: """ @@ -456,3 +497,17 @@ def resync(self) -> bool: changed = True return changed + + +class SystemTaxonomy(Taxonomy): + """ + System-defined taxonomies are not editable by normal users; they're defined by fixtures/migrations, and may have + dynamically-determined Tags and ObjectTags. + """ + + @property + def system_defined(self) -> bool: + """ + This is a system-defined taxonomy. + """ + return True diff --git a/requirements/base.in b/requirements/base.in index d17e06397..a9e1bfc7b 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -6,3 +6,5 @@ Django<5.0 # Web application framework djangorestframework<4.0 # REST API rules<4.0 # Django extension for rules-based authorization checks + +django-model-utils # InheritanceManager for openedx-tagging models diff --git a/requirements/base.txt b/requirements/base.txt index 8c0f00b63..a5dc5b291 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -10,7 +10,10 @@ django==3.2.19 # via # -c requirements/constraints.txt # -r requirements/base.in + # django-model-utils # djangorestframework +django-model-utils==4.3.1 + # via -r requirements/base.in djangorestframework==3.14.0 # via -r requirements/base.in pytz==2023.3 diff --git a/requirements/dev.txt b/requirements/dev.txt index add9cf1de..ccc5c194f 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -78,10 +78,13 @@ django==3.2.19 # -c requirements/constraints.txt # -r requirements/quality.txt # django-debug-toolbar + # django-model-utils # djangorestframework # edx-i18n-tools django-debug-toolbar==4.1.0 # via -r requirements/dev.in +django-model-utils==4.3.1 + # via -r requirements/quality.txt djangorestframework==3.14.0 # via -r requirements/quality.txt docutils==0.20.1 diff --git a/requirements/doc.txt b/requirements/doc.txt index d9a4c3c2a..fc8778e62 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -41,8 +41,11 @@ django==3.2.19 # via # -c requirements/constraints.txt # -r requirements/test.txt + # django-model-utils # djangorestframework # sphinxcontrib-django +django-model-utils==4.3.1 + # via -r requirements/test.txt djangorestframework==3.14.0 # via -r requirements/test.txt doc8==1.1.1 diff --git a/requirements/quality.txt b/requirements/quality.txt index c580bd7e1..e78a0b511 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -47,7 +47,10 @@ django==3.2.19 # via # -c requirements/constraints.txt # -r requirements/test.txt + # django-model-utils # djangorestframework +django-model-utils==4.3.1 + # via -r requirements/test.txt djangorestframework==3.14.0 # via -r requirements/test.txt docutils==0.20.1 diff --git a/requirements/test.txt b/requirements/test.txt index e64d8190b..7e52fbbfe 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -23,7 +23,10 @@ ddt==1.6.0 # via # -c requirements/constraints.txt # -r requirements/base.txt + # django-model-utils # djangorestframework +django-model-utils==4.3.1 + # via -r requirements/base.txt djangorestframework==3.14.0 # via -r requirements/base.txt exceptiongroup==1.1.1 diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 7a418e687..2ba402ac3 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -31,14 +31,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]) == " Enabled" + assert str(enabled[1]) == " Life on Earth" + assert str(enabled[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..44709febc 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 ObjectTag, SystemTaxonomy, Tag, Taxonomy def get_tag(value): @@ -65,6 +65,8 @@ def setUp(self): get_tag("Porifera"), ] + self.system_taxonomy = SystemTaxonomy.objects.create(name="System Languages") + def setup_tag_depths(self): """ Annotate our tags with depth so we can compare them. @@ -85,10 +87,16 @@ 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) == " Life on Earth" + assert ( + str(self.system_taxonomy) + == repr(self.system_taxonomy) + == " System Languages" + ) + assert str(self.bacteria) == repr(self.bacteria) == "Tag (1) Bacteria" @ddt.data( # Root tags just return their own value @@ -136,6 +144,19 @@ 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_representations(self): + assert ( + str(self.object_tag) + == repr(self.object_tag) + == "object:id:1 (life): Life on Earth=Bacteria" + ) def test_object_tag_name(self): # ObjectTag's name defaults to its taxonomy's name @@ -159,50 +180,38 @@ def test_object_tag_name(self): 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, - ) - assert object_tag.value == self.tag.value + assert self.object_tag.value == self.tag.value # Even if we overwrite the value, it still uses the tag's value - object_tag.value = "Another tag" - assert object_tag.value == self.tag.value - object_tag.save() - assert object_tag.value == self.tag.value + self.object_tag.value = "Another tag" + assert self.object_tag.value == self.tag.value + self.object_tag.save() + assert self.object_tag.value == self.tag.value # But if the tag is deleted, then the object_tag's value reverts to our cached value self.tag.delete() - object_tag.refresh_from_db() - assert object_tag.value == "Another tag" + self.object_tag.refresh_from_db() + assert self.object_tag.value == "Another tag" 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, - ) - assert object_tag.get_lineage() == self.tag.get_lineage() + assert self.object_tag.get_lineage() == self.tag.get_lineage() # Even if we overwrite the value, it still uses the tag's lineage - object_tag.value = "Another tag" - assert object_tag.get_lineage() == self.tag.get_lineage() - object_tag.save() - assert object_tag.get_lineage() == self.tag.get_lineage() + self.object_tag.value = "Another tag" + assert self.object_tag.get_lineage() == self.tag.get_lineage() + self.object_tag.save() + assert self.object_tag.get_lineage() == self.tag.get_lineage() # But if the tag is deleted, then the object_tag's lineage reverts to our cached value self.tag.delete() - object_tag.refresh_from_db() - assert object_tag.get_lineage() == ["Another tag"] + self.object_tag.refresh_from_db() + assert self.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", + object_type="life", ) assert not object_tag.is_valid @@ -215,6 +224,7 @@ def test_object_tag_is_valid(self): # or, we can have no tag, and a free-text taxonomy object_tag.tag = None self.taxonomy.allow_free_text = True + self.taxonomy.save() assert object_tag.is_valid def test_validate_object_tag_invalid(self): @@ -281,6 +291,7 @@ def test_tag_object(self): def test_tag_object_free_text(self): self.taxonomy.allow_free_text = True + self.taxonomy.save() object_tags = self.taxonomy.tag_object( ["Eukaryota Xenomorph"], "biology101", diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index 2c2b4e748..6a50feec4 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -3,9 +3,9 @@ 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 +from openedx_tagging.core.tagging.api import get_taxonomy +from openedx_tagging.core.tagging.models import ObjectTag, Tag from .test_models import TestTagTaxonomyMixin @@ -50,12 +50,13 @@ def setUp(self): ) def test_add_change_taxonomy(self, perm): """Taxonomy administrators can create or modify any Taxonomy""" + taxonomy = get_taxonomy(self.taxonomy.id) assert self.superuser.has_perm(perm) - assert self.superuser.has_perm(perm, self.taxonomy) + assert self.superuser.has_perm(perm, taxonomy) assert self.staff.has_perm(perm) - assert self.staff.has_perm(perm, self.taxonomy) + assert self.staff.has_perm(perm, taxonomy) assert not self.learner.has_perm(perm) - assert not self.learner.has_perm(perm, self.taxonomy) + assert not self.learner.has_perm(perm, taxonomy) @ddt.data( "oel_tagging.add_taxonomy", @@ -64,9 +65,7 @@ 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 + system_taxonomy = get_taxonomy(self.system_taxonomy.id) 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) From 1a6e3ce81651459b72376cd790165f78108a8e72 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 22:17:02 +0930 Subject: [PATCH 2/3] 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 799ad2a92d5f5e0542b318c6d1ef2785fdfda598 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 4 Jul 2023 13:36:17 +0930 Subject: [PATCH 3/3] refactor: renames ObjectTag.taxonomy to ObjectTag._taxonomy so we can ensure that the ObjectTag.taxonomy returned is of the correct subclass. --- openedx_tagging/core/tagging/api.py | 4 +- .../migrations/0003_objecttag__taxonomy.py | 28 ++++++++ openedx_tagging/core/tagging/models.py | 65 ++++++++++++++----- .../core/tagging/test_models.py | 27 +++++++- .../core/tagging/test_rules.py | 4 +- 5 files changed, 105 insertions(+), 23 deletions(-) create mode 100644 openedx_tagging/core/tagging/migrations/0003_objecttag__taxonomy.py diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 7bd869bd0..59438f183 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -94,8 +94,8 @@ def get_object_tags( 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. """ - tags = ObjectTag.objects.filter( - taxonomy=taxonomy, object_id=object_id, object_type=object_type + tags = taxonomy.objecttag_set.filter( + 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)] diff --git a/openedx_tagging/core/tagging/migrations/0003_objecttag__taxonomy.py b/openedx_tagging/core/tagging/migrations/0003_objecttag__taxonomy.py new file mode 100644 index 000000000..1dbc3b762 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0003_objecttag__taxonomy.py @@ -0,0 +1,28 @@ +# Generated by Django 3.2.19 on 2023-07-04 02:20 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("oel_tagging", "0002_systemtaxonomy"), + ] + + operations = [ + migrations.RemoveIndex( + model_name="objecttag", + name="oel_tagging_taxonom_3668ec_idx", + ), + migrations.RenameField( + model_name="objecttag", + old_name="taxonomy", + new_name="_taxonomy", + ), + migrations.AddIndex( + model_name="objecttag", + index=models.Index( + fields=["_taxonomy", "_value"], name="oel_tagging__taxono_af4c8a_idx" + ), + ), + ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index a802fdfc0..acee7fb3f 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -224,7 +224,7 @@ def validate_object_tag( """ # Must be linked to this taxonomy if check_taxonomy and ( - not object_tag.taxonomy_id or object_tag.taxonomy_id != self.id + not object_tag.taxonomy or object_tag.taxonomy.id != self.id ): return False @@ -261,8 +261,8 @@ def tag_object( current_tags = { tag.tag_ref: tag - for tag in ObjectTag.objects.filter( - taxonomy=self, object_id=object_id, object_type=object_type + for tag in self.objecttag_set.filter( + object_id=object_id, object_type=object_type ) } updated_tags = [] @@ -330,7 +330,7 @@ class ObjectTag(models.Model): max_length=255, help_text=_("Type of object being tagged"), ) - taxonomy = models.ForeignKey( + _taxonomy = models.ForeignKey( Taxonomy, null=True, default=None, @@ -368,9 +368,16 @@ class ObjectTag(models.Model): class Meta: indexes = [ - models.Index(fields=["taxonomy", "_value"]), + models.Index(fields=["_taxonomy", "_value"]), ] + def __init__(self, *args, **kwargs): + """ + Initializes the cached taxonomy instance. + """ + super().__init__(*args, **kwargs) + self._cached_taxonomy = None + def __repr__(self): """ Developer-facing representation of an ObjectTag. @@ -383,6 +390,35 @@ def __str__(self): """ return f"{self.object_id} ({self.object_type}): {self.name}={self.value}" + @property + def taxonomy(self) -> str: + """ + Returns this tag's taxonomy object, instantiated as the correct Taxonomy subclass. + + This instance is cached so that subsequent calls to self.taxonomy don't re-fetch the value. + + Returns None if taxonomy_id is not set. + """ + if not self._taxonomy_id: + self._cached_taxonomy = None + + elif not self._cached_taxonomy: + self._cached_taxonomy = ( + Taxonomy.objects.filter(id=self._taxonomy_id) + .select_subclasses() + .first() + ) + + return self._cached_taxonomy + + @taxonomy.setter + def taxonomy(self, taxonomy: Taxonomy): + """ + Stores to the _taxonomy field. + """ + self._taxonomy = taxonomy + self._cached_taxonomy = taxonomy + @property def name(self) -> str: """ @@ -391,7 +427,7 @@ def name(self) -> str: If taxonomy is set, then returns its name. Otherwise, returns the cached _name field. """ - return self.taxonomy.name if self.taxonomy_id else self._name + return self.taxonomy.name if self._taxonomy_id else self._name @name.setter def name(self, name: str): @@ -434,15 +470,8 @@ def is_valid(self) -> bool: A valid ObjectTag must be linked to a Taxonomy, and be a valid tag in that taxonomy. """ - if self.taxonomy_id: - # self.taxonomy doesn't return the correct subclass, so we must fetch it - # FIXME: - # * cache to prevent re-fetching the taxonomy - # * discourage people from using self.taxonomy - taxonomy = ( - Taxonomy.objects.filter(id=self.taxonomy_id).select_subclasses().first() - ) - return taxonomy.validate_object_tag(self) + if self._taxonomy_id: + return self.taxonomy.validate_object_tag(self) return False def get_lineage(self) -> Lineage: @@ -468,8 +497,10 @@ def resync(self) -> bool: changed = False # Locate a taxonomy matching _name - if not self.taxonomy_id: - for taxonomy in Taxonomy.objects.filter(name=self.name, enabled=True): + if not self._taxonomy_id: + for taxonomy in Taxonomy.objects.filter( + name=self.name, enabled=True + ).select_subclasses(): # Make sure this taxonomy will accept object tags like this. self.taxonomy = taxonomy if taxonomy.validate_object_tag(self, check_tag=False): diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index 44709febc..39dd013b1 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -158,6 +158,30 @@ def test_representations(self): == "object:id:1 (life): Life on Earth=Bacteria" ) + def test_change_taxonomy(self): + assert self.object_tag.taxonomy == self.taxonomy + assert self.object_tag.taxonomy.id == self.taxonomy.id + + # Check that taxonomy can be cleared + self.object_tag.taxonomy = None + self.object_tag.save() + self.object_tag.refresh_from_db() + assert self.object_tag.taxonomy is None + assert ( + # pylint: disable=protected-access + self.object_tag._taxonomy_id + is None + ) + + # Check that taxonomy can be changed + self.object_tag.taxonomy = self.system_taxonomy + assert self.object_tag.taxonomy == self.system_taxonomy + assert ( + # pylint: disable=protected-access + self.object_tag._taxonomy_id + == self.system_taxonomy.id + ) + def test_object_tag_name(self): # ObjectTag's name defaults to its taxonomy's name object_tag = ObjectTag.objects.create( @@ -274,8 +298,7 @@ def test_tag_object(self): ) # Ensure the expected number of tags exist in the database - assert ObjectTag.objects.filter( - taxonomy=self.taxonomy, + assert self.taxonomy.objecttag_set.filter( object_id="biology101", object_type="course", ).count() == len(tag_list) diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index 6a50feec4..f2873ff2a 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -184,8 +184,8 @@ def test_add_change_object_tag(self, perm): ) def test_object_tag_disabled_taxonomy(self, perm): """Taxonomy administrators cannot create/edit an ObjectTag with a disabled Taxonomy""" - self.taxonomy.enabled = False - self.taxonomy.save() + self.object_tag.taxonomy.enabled = False + self.object_tag.taxonomy.save() assert self.superuser.has_perm(perm, self.object_tag) assert not self.staff.has_perm(perm, self.object_tag) assert not self.learner.has_perm(perm, self.object_tag)