From 13ea45968081e438d24bccff2eba4f4ad233d837 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 8 Jul 2026 11:53:52 -0700 Subject: [PATCH] feat!: remove system-defined taxonomyies, replacing w/ read_only flag Co-Authored-By: Claude --- src/openedx_tagging/api.py | 33 +-- src/openedx_tagging/import_export/api.py | 7 +- ...021_remove_system_defined_add_read_only.py | 89 ++++++ src/openedx_tagging/models/__init__.py | 1 - src/openedx_tagging/models/base.py | 136 +-------- src/openedx_tagging/models/system_defined.py | 254 ---------------- src/openedx_tagging/rest_api/paginators.py | 40 ++- src/openedx_tagging/rest_api/utils.py | 7 +- .../rest_api/v1/serializers.py | 9 +- src/openedx_tagging/rest_api/v1/views.py | 1 - src/openedx_tagging/rules.py | 24 +- tests/openedx_tagging/fixtures/tagging.yaml | 18 +- .../openedx_tagging/import_export/test_api.py | 13 +- tests/openedx_tagging/test_api.py | 176 +++-------- tests/openedx_tagging/test_models.py | 96 ++---- tests/openedx_tagging/test_rules.py | 20 +- .../test_system_defined_models.py | 280 ------------------ tests/openedx_tagging/test_views.py | 200 ++++++------- 18 files changed, 334 insertions(+), 1070 deletions(-) create mode 100644 src/openedx_tagging/migrations/0021_remove_system_defined_add_read_only.py delete mode 100644 src/openedx_tagging/models/system_defined.py delete mode 100644 tests/openedx_tagging/test_system_defined_models.py diff --git a/src/openedx_tagging/api.py b/src/openedx_tagging/api.py index 9fb725978..c0aded1e4 100644 --- a/src/openedx_tagging/api.py +++ b/src/openedx_tagging/api.py @@ -37,7 +37,7 @@ def create_taxonomy( # pylint: disable=too-many-positional-arguments enabled=True, allow_multiple=True, allow_free_text=False, - taxonomy_class: type[Taxonomy] | None = None, + read_only=False, export_id: str | None = None, ) -> Taxonomy: """ @@ -52,30 +52,27 @@ def create_taxonomy( # pylint: disable=too-many-positional-arguments enabled=enabled, allow_multiple=allow_multiple, allow_free_text=allow_free_text, + read_only=read_only, export_id=export_id, ) - if taxonomy_class: - taxonomy.taxonomy_class = taxonomy_class taxonomy.full_clean() taxonomy.save() - return taxonomy.cast() + return taxonomy def get_taxonomy(taxonomy_id: int) -> Taxonomy | None: """ - Returns a Taxonomy cast to the appropriate subclass which has the given ID. + Returns the Taxonomy which has the given ID, or None if not found. """ - taxonomy = Taxonomy.objects.filter(pk=taxonomy_id).first() - return taxonomy.cast() if taxonomy else None + return Taxonomy.objects.filter(pk=taxonomy_id).first() def get_taxonomy_by_export_id(taxonomy_export_id: str) -> Taxonomy | None: """ - Returns a Taxonomy cast to the appropriate subclass which has the given export ID. + Returns the Taxonomy which has the given export ID, or None if not found. """ - taxonomy = Taxonomy.objects.filter(export_id=taxonomy_export_id).first() - return taxonomy.cast() if taxonomy else None + return Taxonomy.objects.filter(export_id=taxonomy_export_id).first() def get_taxonomies(enabled=True) -> QuerySet[Taxonomy]: @@ -83,7 +80,6 @@ def get_taxonomies(enabled=True) -> QuerySet[Taxonomy]: 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. @@ -101,7 +97,7 @@ def get_tags(taxonomy: Taxonomy) -> TagDataQuerySet: Note that if the taxonomy is dynamic or free-text, only tags that have already been applied to some object will be returned. """ - return taxonomy.cast().get_filtered_tags() + return taxonomy.get_filtered_tags() def get_root_tags(taxonomy: Taxonomy) -> TagDataQuerySet: @@ -110,7 +106,7 @@ def get_root_tags(taxonomy: Taxonomy) -> TagDataQuerySet: Note that if the taxonomy allows free-text tags, then the returned list will be empty. """ - return taxonomy.cast().get_filtered_tags(depth=1) + return taxonomy.get_filtered_tags(depth=1) def search_tags( @@ -135,7 +131,7 @@ def search_tags( "_value", flat=True ) ) - qs = taxonomy.cast().get_filtered_tags( + qs = taxonomy.get_filtered_tags( search_term=search_term, excluded_values=excluded_values, ) @@ -151,7 +147,7 @@ def get_children_tags( Note that if the taxonomy allows free-text tags, then the returned list will be empty. """ - return taxonomy.cast().get_filtered_tags(parent_tag_value=parent_tag_value, depth=1) + return taxonomy.get_filtered_tags(parent_tag_value=parent_tag_value, depth=1) def resync_object_tags(object_tags: QuerySet | None = None) -> int: @@ -354,9 +350,7 @@ def tag_object( # pylint: disable=too-many-positional-arguments ObjectTagClass = object_tag_class tags = list(dict.fromkeys(tags)) # Remove duplicates preserving order - if taxonomy: - taxonomy = taxonomy.cast() # Make sure we're using the right subclass. This is a no-op if we are already. - elif not taxonomy_export_id: + if not taxonomy and not taxonomy_export_id: raise ValueError("`taxonomy_export_id` can't be None if `taxonomy` is None") _check_new_tag_count(len(tags), taxonomy, object_id, taxonomy_export_id) @@ -444,7 +438,6 @@ def add_tag_to_taxonomy( Taxonomy, an exception is raised, otherwise the newly created Tag is returned """ - taxonomy = taxonomy.cast() new_tag = taxonomy.add_tag(tag, parent_tag_value, external_id) # Resync all related ObjectTags after creating new Tag to @@ -463,7 +456,6 @@ def update_tag_in_taxonomy(taxonomy: Taxonomy, tag: str, new_value: str): Currently only supports updating the Tag value. """ - taxonomy = taxonomy.cast() updated_tag = taxonomy.update_tag(tag, new_value) # Resync all related ObjectTags to update to the new Tag value @@ -483,7 +475,6 @@ def delete_tags_from_taxonomy( the `with_subtags` is not set to `True` it will fail, otherwise the sub-tags will be deleted as well. """ - taxonomy = taxonomy.cast() taxonomy.delete_tags(tags, with_subtags) diff --git a/src/openedx_tagging/import_export/api.py b/src/openedx_tagging/import_export/api.py index 591e3487b..c9ac1434b 100644 --- a/src/openedx_tagging/import_export/api.py +++ b/src/openedx_tagging/import_export/api.py @@ -216,17 +216,16 @@ def _import_validations(taxonomy: Taxonomy): """ Validates if the taxonomy is allowed to import tags """ - taxonomy = taxonomy.cast() if taxonomy.allow_free_text: raise ValueError( _( - "Invalid taxonomy ({id}): You cannot import a free-form taxonomy." + "Invalid taxonomy ({id}): You cannot import to a free-text taxonomy." ).format(id=taxonomy.id) ) - if taxonomy.system_defined: + if taxonomy.read_only: raise ValueError( _( - "Invalid taxonomy ({id}): You cannot import a system-defined taxonomy." + "Invalid taxonomy ({id}): You cannot import to a read-only taxonomy." ).format(id=taxonomy.id) ) diff --git a/src/openedx_tagging/migrations/0021_remove_system_defined_add_read_only.py b/src/openedx_tagging/migrations/0021_remove_system_defined_add_read_only.py new file mode 100644 index 000000000..44802e373 --- /dev/null +++ b/src/openedx_tagging/migrations/0021_remove_system_defined_add_read_only.py @@ -0,0 +1,89 @@ +""" +Remove the "system-defined" taxonomy machinery (Taxonomy subclasses and the +``_taxonomy_class`` casting column) in favour of a simple ``read_only`` flag. + +See https://github.com/openedx/openedx-core/issues/634 for the rationale. + +This migration: + +* Adds the ``read_only`` boolean field to Taxonomy. +* Marks any taxonomy that used to be "system-defined" (i.e. had a + ``_taxonomy_class`` set) as ``read_only=True``, so that its tags remain + immutable as they were before. +* Handles the auto-created "Languages" taxonomy (``id=-1``), which was created + by ``0012_language_taxonomy`` and is no longer supported: if it has been used + (any related object tags exist) it is converted into a regular, editable + taxonomy; otherwise it is deleted. +* Removes the now-unused ``_taxonomy_class`` column and the proxy models. +""" + +from django.db import migrations, models + +# The Languages taxonomy was auto-created with this fixed id by 0012_language_taxonomy. +LANGUAGE_TAXONOMY_ID = -1 + + +def forwards(apps, schema_editor): + """ + Migrate system-defined taxonomies to the new read_only flag, and either + convert or drop the auto-created Languages taxonomy. + """ + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + ObjectTag = apps.get_model("oel_tagging", "ObjectTag") + + language_taxonomy = Taxonomy.objects.filter(id=LANGUAGE_TAXONOMY_ID).first() + if language_taxonomy: + if ObjectTag.objects.filter(taxonomy_id=LANGUAGE_TAXONOMY_ID).exists(): + # It's in use, so convert it into a regular, editable taxonomy, + # keeping whatever language Tags have already been created. + language_taxonomy._taxonomy_class = None + language_taxonomy.read_only = False + language_taxonomy.save() + else: + # Unused, so remove it (and its tags) entirely. + language_taxonomy.delete() + + # Any remaining taxonomy that was backed by a subclass was "system-defined", + # meaning its tags could not be modified. Preserve that by marking it read-only. + Taxonomy.objects.exclude(_taxonomy_class__isnull=True).exclude(_taxonomy_class="").update(read_only=True) + + +def backwards(apps, schema_editor): + """ + Nothing to undo here: the deleted Languages taxonomy and the subclass + information cannot be restored, because the subclasses no longer exist. + The ``read_only`` column is dropped by the reversal of the AddField + operation. + """ + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0020_tag_depth_and_lineage'), + ] + + operations = [ + migrations.DeleteModel( + name='LanguageTaxonomy', + ), + migrations.DeleteModel( + name='ModelSystemDefinedTaxonomy', + ), + migrations.DeleteModel( + name='SystemDefinedTaxonomy', + ), + migrations.DeleteModel( + name='UserSystemDefinedTaxonomy', + ), + migrations.AddField( + model_name='taxonomy', + name='read_only', + field=models.BooleanField(default=False, help_text='Indicates that the tags in this taxonomy are maintained by the system or an external integration; taxonomy admins will not be permitted to add, edit, or delete its tags.'), + ), + migrations.RunPython(forwards, backwards), + migrations.RemoveField( + model_name='taxonomy', + name='_taxonomy_class', + ), + ] diff --git a/src/openedx_tagging/models/__init__.py b/src/openedx_tagging/models/__init__.py index c6b31e052..d39459942 100644 --- a/src/openedx_tagging/models/__init__.py +++ b/src/openedx_tagging/models/__init__.py @@ -3,4 +3,3 @@ """ from .base import ObjectTag, Tag, Taxonomy from .import_export import TagImportTask, TagImportTaskState -from .system_defined import LanguageTaxonomy, ModelSystemDefinedTaxonomy, UserSystemDefinedTaxonomy diff --git a/src/openedx_tagging/models/base.py b/src/openedx_tagging/models/base.py index bf9c64c27..f2cc41b4f 100644 --- a/src/openedx_tagging/models/base.py +++ b/src/openedx_tagging/models/base.py @@ -4,7 +4,6 @@ from __future__ import annotations -import logging import re from typing import List, Self, cast @@ -13,7 +12,6 @@ from django.db.models import F, Value from django.db.models.functions import Concat, Length, Replace, Substr from django.utils.functional import cached_property -from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ from openedx_django_lib.fields import MultiCollationTextField, case_insensitive_char_field, case_sensitive_char_field @@ -21,8 +19,6 @@ from ..data import TagDataQuerySet from .utils import RESERVED_TAG_CHARS -log = logging.getLogger(__name__) - # Maximum depth of tags that can be created. Internally, the system has no depth limits, but for reasonable performance # guarantees we enforce this depth. Note: depth is zero-indexed so "5" means 6 levels of depth are allowed. TAXONOMY_MAX_DEPTH = 5 @@ -295,14 +291,12 @@ class Taxonomy(models.Model): ), unique=True, ) - _taxonomy_class = models.CharField( - null=True, - max_length=255, + read_only = models.BooleanField( + default=False, 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." + "Indicates that the tags in this taxonomy are maintained by the system or an external integration;" + " taxonomy admins will not be permitted to add, edit, or delete its tags." ), - blank=True, ) class Meta: @@ -318,55 +312,8 @@ def __str__(self): """ User-facing string representation of a Taxonomy. """ - try: - if self._taxonomy_class: - return f"<{self.taxonomy_class.__name__}> ({self.id}) {self.name}" - except ImportError: - # Log error and continue - log.exception( - f"Unable to import taxonomy_class for {self.id}: {self._taxonomy_class}" - ) return f"<{self.__class__.__name__}> ({self.id}) {self.name}" - @property - def taxonomy_class(self) -> type[Taxonomy] | None: - """ - 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: type[Taxonomy] | None): - """ - Assigns the given taxonomy_class's module path.class to the field. - - Must be a subclass of Taxonomy, or raises a ValueError. - """ - if taxonomy_class: - if not issubclass(taxonomy_class, Taxonomy): - raise ValueError( - f"Unable to assign taxonomy_class for {self}: {taxonomy_class} must be a subclass of Taxonomy" - ) - - # ref: https://stackoverflow.com/a/2020083 - self._taxonomy_class = ".".join( - [taxonomy_class.__module__, taxonomy_class.__qualname__] - ) - else: - self._taxonomy_class = None - - @property - def system_defined(self) -> bool: - """ - Indicates that tags and metadata for this taxonomy are maintained by the system; - taxonomy admins will not be permitted to modify them. - """ - return False - def clean(self): super().clean() @@ -375,52 +322,6 @@ def clean(self): "The export_id should only contain alphanumeric characters or '_' '-' '.'" ) - def cast(self): - """ - Returns the current Taxonomy instance cast into its taxonomy_class. - - If no taxonomy_class is set, or if we're unable to import it, 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 check_casted(self): - """ - Double-check that this taxonomy has been cast() to a subclass if needed. - """ - if self.cast() is not self: - raise TypeError("Taxonomy was used incorrectly - without .cast()") - - def copy(self, taxonomy: Taxonomy) -> Taxonomy: - """ - 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.allow_multiple = taxonomy.allow_multiple - self.allow_free_text = taxonomy.allow_free_text - self.visible_to_authors = taxonomy.visible_to_authors - self.export_id = taxonomy.export_id - self._taxonomy_class = taxonomy._taxonomy_class # pylint: disable=protected-access - - # Copy Django's internal prefetch_related cache to reduce queries required on the casted taxonomy. - if hasattr(taxonomy, '_prefetched_objects_cache'): - # pylint: disable=protected-access,attribute-defined-outside-init - self._prefetched_objects_cache: dict = taxonomy._prefetched_objects_cache - - return self - def get_filtered_tags( self, depth: int | None = None, @@ -611,16 +512,14 @@ def add_tag( exists in the Taxonomy, an exception is raised, otherwise the newly created Tag is returned """ - self.check_casted() - if self.allow_free_text: raise ValueError( "add_tag() doesn't work for free text taxonomies. They don't use Tag instances." ) - if self.system_defined: + if self.read_only: raise ValueError( - "add_tag() doesn't work for system defined taxonomies. They cannot be modified." + "add_tag() doesn't work for read-only taxonomies. They cannot be modified." ) if self.tag_set.filter(value__iexact=tag_value).exists(): @@ -644,16 +543,14 @@ def update_tag(self, tag: str, new_value: str) -> Tag: Update an existing Tag in Taxonomy and return it. Currently only supports updating the Tag's value. """ - self.check_casted() - if self.allow_free_text: raise ValueError( "update_tag() doesn't work for free text taxonomies. They don't use Tag instances." ) - if self.system_defined: + if self.read_only: raise ValueError( - "update_tag() doesn't work for system defined taxonomies. They cannot be modified." + "update_tag() doesn't work for read-only taxonomies. They cannot be modified." ) # Update Tag instance with new value, raises Tag.DoesNotExist if @@ -669,16 +566,14 @@ def delete_tags(self, tags: List[str], with_subtags: bool = False): the `with_subtags` is not set to `True` it will fail, otherwise the sub-tags will be deleted as well. """ - self.check_casted() - if self.allow_free_text: raise ValueError( "delete_tags() doesn't work for free text taxonomies. They don't use Tag instances." ) - if self.system_defined: + if self.read_only: raise ValueError( - "delete_tags() doesn't work for system defined taxonomies. They cannot be modified." + "delete_tags() doesn't work for read-only taxonomies. They cannot be modified." ) tags_to_delete = self.tag_set.filter(value__in=tags) @@ -704,12 +599,10 @@ def validate_value(self, value: str) -> bool: """ Check if 'value' is part of this Taxonomy. A 'Tag' object may not exist for the value (e.g. if this is a free text - taxonomy, then any value is allowed but no Tags are created; if this is - a user taxonomy, Tag entries may only get created as needed.), but if + taxonomy, then any value is allowed but no Tags are created), but if this returns True then the value conceptually exists in this taxonomy and can be used to tag objects. """ - self.check_casted() if self.allow_free_text: return value != "" and isinstance(value, str) return self.tag_set.filter(value__iexact=value).exists() @@ -717,12 +610,9 @@ def validate_value(self, value: str) -> bool: def tag_for_value(self, value: str, select_related: list[str] | None = None) -> Tag: """ Get the Tag object for the given value. - Some Taxonomies may auto-create the Tag at this point, e.g. a User - Taxonomy will create User Tags "just in time". Will raise Tag.DoesNotExist if the value is not valid for this taxonomy. """ - self.check_casted() if self.allow_free_text: raise ValueError("tag_for_value() doesn't work for free text taxonomies. They don't use Tag instances.") if select_related is not None: @@ -735,7 +625,6 @@ def validate_external_id(self, external_id: str) -> bool: """ Check if 'external_id' is part of this Taxonomy. """ - self.check_casted() if self.allow_free_text: return False # Free text taxonomies don't use 'external_id' on their tags return self.tag_set.filter(external_id__iexact=external_id).exists() @@ -743,12 +632,9 @@ def validate_external_id(self, external_id: str) -> bool: def tag_for_external_id(self, external_id: str) -> Tag: """ Get the Tag object for the given external_id. - Some Taxonomies may auto-create the Tag at this point, e.g. a User - Taxonomy will create User Tags "just in time". Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. """ - self.check_casted() if self.allow_free_text: raise ValueError("tag_for_external_id() doesn't work for free text taxonomies.") return self.tag_set.get(external_id__iexact=external_id) diff --git a/src/openedx_tagging/models/system_defined.py b/src/openedx_tagging/models/system_defined.py deleted file mode 100644 index e709fcae1..000000000 --- a/src/openedx_tagging/models/system_defined.py +++ /dev/null @@ -1,254 +0,0 @@ -""" -Tagging app system-defined taxonomies data models -""" -from __future__ import annotations - -import logging -from typing import override - -from django.conf import settings -from django.contrib.auth import get_user_model -from django.core.exceptions import ObjectDoesNotExist -from django.db import models - -from openedx_tagging.models.base import Tag - -from .base import Tag, Taxonomy - -log = logging.getLogger(__name__) - - -class SystemDefinedTaxonomy(Taxonomy): - """ - Simple subclass of Taxonomy which requires the system_defined flag to be set. - """ - - class Meta: - proxy = True - - @property - def system_defined(self) -> bool: - """ - Indicates that tags and metadata for this taxonomy are maintained by the system; - taxonomy admins will not be permitted to modify them. - """ - return True - - -class ModelSystemDefinedTaxonomy(SystemDefinedTaxonomy): - """ - Model based system taxonomy abstract class. - - This type of taxonomy has an associated Django model in - ModelSystemDefinedTaxonomy.tag_class_model. - - They are designed to create Tags when required for new ObjectTags, to - maintain their status as "closed" taxonomies. - - The Tags are representations of the instances of the associated model. - - Tag.external_id stores an identifier from the instance (`pk` as default) - and Tag.value stores a human readable representation of the instance - (e.g. `username`). - The subclasses can override this behavior, to choose the right field. - - When an ObjectTag is created with an existing Tag, - the Tag is re-synchronized with its instance. - """ - - class Meta: - proxy = True - - @property - def tag_class_model(self) -> type[models.Model]: - """ - Define what Django model this taxonomy is associated with - """ - raise NotImplementedError - - @property - def tag_class_value_field(self) -> str: - """ - The name of the tag_class_model field to use as the Tag.value when creating Tags for this taxonomy. - - Subclasses may override this method to use different fields. - """ - raise NotImplementedError - - @property - def tag_class_key_field(self) -> str: - """ - The name of the tag_class_model field to use as the Tag.external_id when creating Tags for this taxonomy. - - This must be an immutable ID. - """ - return "pk" - - def validate_value(self, value: str): - """ - Check if 'value' is part of this Taxonomy, based on the specified model. - """ - try: - # See https://github.com/typeddjango/django-stubs/issues/1684 for why we need to ignore this. - self.tag_class_model.objects.get( # type: ignore[attr-defined] - **{f"{self.tag_class_value_field}__iexact": value} - ) - return True - except ObjectDoesNotExist: - return False - - @override - def tag_for_value(self, value: str, select_related: list[str] | None = None) -> Tag: - """ - Get the Tag object for the given value. - """ - try: - # First we look up the instance by value. - # We specify 'iexact' but whether it's case sensitive or not on MySQL depends on the model's collation. - # See https://github.com/typeddjango/django-stubs/issues/1684 for why we need to ignore this. - instance = self.tag_class_model.objects.get( # type: ignore[attr-defined] - **{f"{self.tag_class_value_field}__iexact": value} - ) - except ObjectDoesNotExist as exc: - raise Tag.DoesNotExist from exc - # Use the canonical value from here on (possibly with different case from the value given as a parameter) - value = getattr(instance, self.tag_class_value_field) - # We assume the value may change but the external_id is immutable. - # So look up keys using external_id. There may be a key with the same external_id but an out of date value. - external_id = str(getattr(instance, self.tag_class_key_field)) - tag_set = self.tag_set.all() - if select_related is not None: - tag_set = tag_set.select_related(*select_related) # type: ignore[assignment] - tag, _created = self.tag_set.get_or_create(external_id=external_id, defaults={"value": value}) - if tag.value != value: - # Update the Tag to reflect the new cached 'value' - tag.value = value - tag.save() - return tag - - def validate_external_id(self, external_id: str): - """ - Check if 'external_id' is part of this Taxonomy. - """ - try: - # See https://github.com/typeddjango/django-stubs/issues/1684 for why we need to ignore this. - self.tag_class_model.objects.get( # type: ignore[attr-defined] - **{f"{self.tag_class_key_field}__iexact": external_id} - ) - return True - except ObjectDoesNotExist: - return False - - def tag_for_external_id(self, external_id: str): - """ - Get the Tag object for the given external_id. - Some Taxonomies may auto-create the Tag at this point, e.g. a User - Taxonomy will create User Tags "just in time". - - Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. - """ - try: - # First we look up the instance by external_id - # We specify 'iexact' but whether it's case sensitive or not on MySQL depends on the model's collation. - # See https://github.com/typeddjango/django-stubs/issues/1684 for why we need to ignore this. - instance = self.tag_class_model.objects.get( # type: ignore[attr-defined] - **{f"{self.tag_class_key_field}__iexact": external_id} - ) - except ObjectDoesNotExist as exc: - raise Tag.DoesNotExist from exc - value = getattr(instance, self.tag_class_value_field) - # Use the canonical external_id from here on (may differ in capitalization) - external_id = getattr(instance, self.tag_class_key_field) - tag, _created = self.tag_set.get_or_create(external_id=external_id, defaults={"value": value}) - if tag.value != value: - # Update the Tag to reflect the new cached 'value' - tag.value = value - tag.save() - return tag - - -class UserSystemDefinedTaxonomy(ModelSystemDefinedTaxonomy): - """ - A Taxonomy that allows tagging objects using users. - """ - - class Meta: - proxy = True - - @property - def tag_class_model(self) -> type[models.Model]: - """ - Define what Django model this taxonomy is associated with - """ - return get_user_model() - - @property - def tag_class_value_field(self) -> str: - """ - Returns the name of the tag_class_model field to use as the Tag.value when creating Tags for this taxonomy. - - Subclasses may override this method to use different fields. - """ - return "username" - - -class LanguageTaxonomy(SystemDefinedTaxonomy): - """ - Language System-defined taxonomy - - The tags are filtered and validated taking into account the - languages available in Django LANGUAGES settings var - """ - - class Meta: - proxy = True - - def validate_value(self, value: str): - """ - Check if 'value' is part of this Taxonomy, based on the specified model. - """ - for _, lang_name in settings.LANGUAGES: - if lang_name == value: - return True - return False - - @override - def tag_for_value(self, value: str, select_related: list[str] | None = None) -> Tag: - """ - Get the Tag object for the given value. - """ - for lang_code, lang_name in settings.LANGUAGES: - if lang_name == value: - return self.tag_for_external_id(lang_code) - raise Tag.DoesNotExist - - def validate_external_id(self, external_id: str): - """ - Check if 'external_id' is part of this Taxonomy. - """ - lang_code = external_id.lower() - # Get settings.LANGUAGES (a list of tuples) as a dict. In LMS/CMS this is already cached as LANGUAGE_DICT - languages_as_dict = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) - return lang_code in languages_as_dict - - def tag_for_external_id(self, external_id: str): - """ - Get the Tag object for the given external_id. - Some Taxonomies may auto-create the Tag at this point, e.g. a User - Taxonomy will create User Tags "just in time". - - Will raise Tag.DoesNotExist if the tag is not valid for this taxonomy. - """ - lang_code = external_id.lower() - # Get settings.LANGUAGES (a list of tuples) as a dict. In LMS/CMS this is already cached as LANGUAGE_DICT - languages_as_dict = getattr(settings, "LANGUAGE_DICT", dict(settings.LANGUAGES)) - try: - lang_name = languages_as_dict[lang_code] - except KeyError as exc: - raise Tag.DoesNotExist from exc - tag, _created = self.tag_set.get_or_create(external_id=lang_code, defaults={"value": lang_name}) - if tag.value != lang_name: - # Update the Tag to reflect the new language name - tag.value = lang_name - tag.save() - return tag diff --git a/src/openedx_tagging/rest_api/paginators.py b/src/openedx_tagging/rest_api/paginators.py index c1f16c3b8..bad63269a 100644 --- a/src/openedx_tagging/rest_api/paginators.py +++ b/src/openedx_tagging/rest_api/paginators.py @@ -21,6 +21,8 @@ class CanAddPermissionMixin(UserPermissionsHelper): # pylint: disable=abstract- The value of the field indicates whether request user may create new instances of the current model. """ + view = None + @property def _request(self) -> Request: """ @@ -28,6 +30,13 @@ def _request(self) -> Request: """ return self.request # type: ignore[attr-defined] + def paginate_queryset(self, queryset, request, view=None): + """ + Keeps a reference to the view, so `get_can_add` can use it to check object-level permissions. + """ + self.view = view + return super().paginate_queryset(queryset, request, view=view) + def get_paginated_response(self, data) -> Response: """ Injects the user's model-level permissions into the paginated response. @@ -53,14 +62,11 @@ def _model(self) -> Type: return Taxonomy -class TagsPagination(CanAddPermissionMixin, DefaultPagination): +class TagPermissionsMixin(CanAddPermissionMixin): # pylint: disable=abstract-method """ - Custom pagination configuration for taxonomies - with a large number of tags. Used on the get tags API view. + Checks "add_tag" permission using a Tag bound to the current taxonomy, so that taxonomies with + read_only=True correctly report can_add_tag=False. """ - page_size = 10 - max_page_size = 300 - @property def _model(self) -> Type: """ @@ -68,8 +74,21 @@ def _model(self) -> Type: """ return Tag + def get_can_add(self, _instance=None) -> bool | None: + taxonomy = self.view.get_taxonomy() if self.view else None + return super().get_can_add(Tag(taxonomy=taxonomy) if taxonomy else None) + -class DisabledTagsPagination(CanAddPermissionMixin, DefaultPagination): +class TagsPagination(TagPermissionsMixin, DefaultPagination): + """ + Custom pagination configuration for taxonomies + with a large number of tags. Used on the get tags API view. + """ + page_size = 10 + max_page_size = 300 + + +class DisabledTagsPagination(TagPermissionsMixin, DefaultPagination): """ Custom pagination configuration for taxonomies with a small number of tags. Used on the get tags API view @@ -80,10 +99,3 @@ class DisabledTagsPagination(CanAddPermissionMixin, DefaultPagination): """ page_size = MAX_FULL_DEPTH_THRESHOLD max_page_size = MAX_FULL_DEPTH_THRESHOLD + 1 - - @property - def _model(self) -> Type: - """ - Returns the model that is being paginated. - """ - return Tag diff --git a/src/openedx_tagging/rest_api/utils.py b/src/openedx_tagging/rest_api/utils.py index ed68de50f..a3c9f1bbf 100644 --- a/src/openedx_tagging/rest_api/utils.py +++ b/src/openedx_tagging/rest_api/utils.py @@ -78,14 +78,15 @@ def _can(self, perm_name: str, instance=None) -> Optional[bool]: assert request and request.user return request.user.has_perm(perm_name, instance) - def get_can_add(self, _instance=None) -> Optional[bool]: + def get_can_add(self, instance=None) -> Optional[bool]: """ Returns True if the current user is allowed to add new instances. - Note: we omit the actual instance from the permissions check; most tagging models prefer this. + `instance` is optional: most tagging models don't need one to check "add" permissions, but some + (e.g. Tag, whose "add" permission depends on its taxonomy's read_only flag) do. """ perm_name = self._get_permission_name('add') - return self._can(perm_name) + return self._can(perm_name, instance) def get_can_view(self, instance) -> Optional[bool]: """ diff --git a/src/openedx_tagging/rest_api/v1/serializers.py b/src/openedx_tagging/rest_api/v1/serializers.py index 21f889168..fcab256a3 100644 --- a/src/openedx_tagging/rest_api/v1/serializers.py +++ b/src/openedx_tagging/rest_api/v1/serializers.py @@ -84,7 +84,7 @@ class Meta: "enabled", "allow_multiple", "allow_free_text", - "system_defined", + "read_only", "visible_to_authors", "tags_count", "can_change_taxonomy", @@ -93,13 +93,6 @@ class Meta: "export_id", ] - def to_representation(self, instance): - """ - Cast the taxonomy before serialize - """ - instance = instance.cast() - return super().to_representation(instance) - def get_tags_count(self, instance): """ Return the "tags_count" annotation if present. diff --git a/src/openedx_tagging/rest_api/v1/views.py b/src/openedx_tagging/rest_api/v1/views.py index 6192797c1..45890c16f 100644 --- a/src/openedx_tagging/rest_api/v1/views.py +++ b/src/openedx_tagging/rest_api/v1/views.py @@ -470,7 +470,6 @@ def get_queryset(self) -> models.QuerySet: taxonomy = query_params.validated_data.get("taxonomy", None) taxonomy_id = None if taxonomy: - taxonomy = taxonomy.cast() taxonomy_id = taxonomy.id if object_id.endswith("*") or "," in object_id: diff --git a/src/openedx_tagging/rules.py b/src/openedx_tagging/rules.py index a78c07398..cbb23e074 100644 --- a/src/openedx_tagging/rules.py +++ b/src/openedx_tagging/rules.py @@ -37,17 +37,18 @@ def can_view_taxonomy(user: UserType, taxonomy: Taxonomy | None = None) -> bool: Anyone can view an enabled taxonomy or list all taxonomies, but only taxonomy admins can view a disabled taxonomy. """ - return not taxonomy or taxonomy.cast().enabled or is_taxonomy_admin(user) + return not taxonomy or taxonomy.enabled or is_taxonomy_admin(user) @rules.predicate -def can_change_taxonomy(user: UserType, taxonomy: Taxonomy | None = None) -> bool: +def can_change_taxonomy(user: UserType, taxonomy: Taxonomy | None = None) -> bool: # pylint: disable=unused-argument """ - Even taxonomy admins cannot change system taxonomies. + Taxonomy admins can create, modify, and delete any taxonomy. + + This includes toggling a taxonomy's read_only flag: the flag only prevents + editing the taxonomy's *tags* (see can_change_tag), not the taxonomy itself. """ - return is_taxonomy_admin(user) and ( - not taxonomy or bool(taxonomy and not taxonomy.cast().system_defined) - ) + return is_taxonomy_admin(user) @rules.predicate @@ -55,7 +56,7 @@ def can_view_tag(user: UserType, tag: Tag | None = None) -> bool: """ User can view tags for any taxonomy they can view. """ - taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None + taxonomy = tag.taxonomy if (tag and tag.taxonomy) else None return user.has_perm( "oel_tagging.view_taxonomy", taxonomy, @@ -65,9 +66,12 @@ def can_view_tag(user: UserType, tag: Tag | None = None) -> bool: @rules.predicate def can_change_tag(user: UserType, tag: Tag | None = None) -> bool: """ - Users can change tags for any taxonomy they can modify. + Users can change tags for any taxonomy they can modify, except read-only + taxonomies, whose tags are maintained by the system and cannot be changed. """ - taxonomy = tag.taxonomy.cast() if (tag and tag.taxonomy) else None + taxonomy = tag.taxonomy if (tag and tag.taxonomy) else None + if taxonomy and taxonomy.read_only: + return False return user.has_perm( "oel_tagging.change_taxonomy", taxonomy, @@ -85,7 +89,7 @@ def can_view_object_tag_taxonomy(user: UserType, taxonomy: Taxonomy) -> bool: if not taxonomy: return True - return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) + return taxonomy.enabled and can_view_taxonomy(user, taxonomy) @rules.predicate diff --git a/tests/openedx_tagging/fixtures/tagging.yaml b/tests/openedx_tagging/fixtures/tagging.yaml index 83f1fba83..9c458aeca 100644 --- a/tests/openedx_tagging/fixtures/tagging.yaml +++ b/tests/openedx_tagging/fixtures/tagging.yaml @@ -289,26 +289,16 @@ allow_multiple: false allow_free_text: false export_id: life_on_earth -- model: oel_tagging.taxonomy - pk: 3 - fields: - name: User Authors - description: Allows tags for any User on the instance. - enabled: true - allow_multiple: false - allow_free_text: false - export_id: user_authors - _taxonomy_class: openedx_tagging.models.system_defined.UserSystemDefinedTaxonomy - model: oel_tagging.taxonomy pk: 4 fields: - name: System defined taxonomy - description: Generic System defined taxonomy + name: Read Only Taxonomy + description: Generic read-only taxonomy enabled: true allow_multiple: true allow_free_text: false - export_id: system_defined_taxonomy - _taxonomy_class: openedx_tagging.models.system_defined.SystemDefinedTaxonomy + export_id: read_only_taxonomy + read_only: true - model: oel_tagging.taxonomy pk: 5 fields: diff --git a/tests/openedx_tagging/import_export/test_api.py b/tests/openedx_tagging/import_export/test_api.py index d9b1c541a..bdb04a86a 100644 --- a/tests/openedx_tagging/import_export/test_api.py +++ b/tests/openedx_tagging/import_export/test_api.py @@ -8,7 +8,7 @@ import openedx_tagging.import_export.api as import_export_api from openedx_tagging.import_export import ParserFormat -from openedx_tagging.models import LanguageTaxonomy, Tag, TagImportTask, TagImportTaskState, Taxonomy +from openedx_tagging.models import Tag, TagImportTask, TagImportTaskState, Taxonomy from .mixins import TestImportExportMixin @@ -45,11 +45,10 @@ def setUp(self) -> None: name="Open taxonomy", allow_free_text=True ) - self.system_taxonomy = Taxonomy( - name="System taxonomy", + self.read_only_taxonomy = Taxonomy( + name="Read-only taxonomy", + read_only=True, ) - self.system_taxonomy.taxonomy_class = LanguageTaxonomy - self.system_taxonomy = self.system_taxonomy.cast() return super().setUp() def test_check_status(self) -> None: @@ -81,10 +80,10 @@ def test_import_export_validations(self) -> None: self.parser_format, ) - # Check that import is invalid with system taxonomy + # Check that import is invalid with read-only taxonomy with self.assertRaises(ValueError): import_export_api.import_tags( - self.system_taxonomy, + self.read_only_taxonomy, self.file, self.parser_format, ) diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index 74789677d..1acad84ea 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -9,7 +9,7 @@ import ddt # type: ignore[import] import pytest from django.core.exceptions import ValidationError -from django.test import TestCase, override_settings +from django.test import TestCase import openedx_tagging.api as tagging_api from openedx_tagging.models import ObjectTag, Tag, Taxonomy @@ -17,22 +17,6 @@ from .test_models import TestTagTaxonomyMixin, get_tag from .utils import pretty_format_tags -test_languages = [ - ("az", "Azerbaijani"), - ("en", "English"), - ("id", "Indonesian"), - ("ga", "Irish"), - ("pl", "Polish"), - ("qu", "Quechua"), - ("zu", "Zulu"), -] -# Languages that contains 'ish' -filtered_test_languages = [ - ("en", "English"), - ("ga", "Irish"), - ("pl", "Polish"), -] - tag_values_for_autocomplete_test = [ 'Archaea', 'Archaebacteria', @@ -63,9 +47,13 @@ def test_create_taxonomy(self) -> None: # Note: we must specify '-> None' to op taxonomy = tagging_api.create_taxonomy(**params) for param, value in params.items(): assert getattr(taxonomy, param) == value - assert not taxonomy.system_defined + assert not taxonomy.read_only assert taxonomy.visible_to_authors + def test_create_taxonomy_read_only(self) -> None: + taxonomy = tagging_api.create_taxonomy(name="Read Only", read_only=True) + assert taxonomy.read_only + def test_create_taxonomy_without_export_id(self) -> None: params: dict[str, Any] = { "name": "Taxonomy Data: test 3", @@ -74,15 +62,7 @@ def test_create_taxonomy_without_export_id(self) -> None: "allow_free_text": True, } taxonomy = tagging_api.create_taxonomy(**params) - assert taxonomy.export_id == "7-taxonomy-data-test-3" - - def test_bad_taxonomy_class(self) -> None: - with self.assertRaises(ValueError) as exc: - tagging_api.create_taxonomy( - name="Bad class", - taxonomy_class=str, # type: ignore[arg-type] - ) - assert " must be a subclass of Taxonomy" in str(exc.exception) + assert taxonomy.export_id == "5-taxonomy-data-test-3" def test_get_taxonomy(self) -> None: tax1 = tagging_api.get_taxonomy(1) @@ -107,17 +87,14 @@ def test_get_taxonomies(self) -> None: tax1, self.free_text_taxonomy, tax3, - self.language_taxonomy, self.taxonomy, self.system_taxonomy, - self.user_taxonomy, ] assert str(enabled[0]) == f" ({tax1.id}) Enabled" assert str(enabled[1]) == f" ({self.free_text_taxonomy.id}) Free Text" assert str(enabled[2]) == " (5) Import Taxonomy Test" - assert str(enabled[3]) == " (-1) Languages" - assert str(enabled[4]) == " (1) Life on Earth" - assert str(enabled[5]) == " (4) System defined taxonomy" + assert str(enabled[3]) == " (1) Life on Earth" + assert str(enabled[4]) == " (4) Read Only Taxonomy" with self.assertNumQueries(1): disabled = list(tagging_api.get_taxonomies(enabled=False)) @@ -131,10 +108,8 @@ def test_get_taxonomies(self) -> None: tax1, self.free_text_taxonomy, tax3, - self.language_taxonomy, self.taxonomy, self.system_taxonomy, - self.user_taxonomy, ] def test_get_tags(self) -> None: @@ -162,7 +137,6 @@ def test_get_tags(self) -> None: " Protista (children: 0)", ] - @override_settings(LANGUAGES=test_languages) def test_get_tags_system(self) -> None: assert pretty_format_tags(tagging_api.get_tags(self.system_taxonomy), parent=False) == [ "System Tag 1 (children: 0)", @@ -179,7 +153,6 @@ def test_get_root_tags(self): 'Eukaryota (children: 5)', ] - @override_settings(LANGUAGES=test_languages) def test_get_root_tags_system(self): result = tagging_api.get_root_tags(self.system_taxonomy) assert pretty_format_tags(result, parent=False) == [ @@ -189,28 +162,6 @@ def test_get_root_tags_system(self): 'System Tag 4 (children: 0)', ] - @override_settings(LANGUAGES=test_languages) - def test_get_root_language_tags(self): - """ - For the language taxonomy, listing and searching tags will only show - tags that have been used at least once. - """ - before_langs = [ - tag["external_id"] for tag in - tagging_api.get_root_tags(self.language_taxonomy) - ] - assert before_langs == ["en"] - # Use a few more tags: - for _lang_code, lang_value in test_languages: - tagging_api.tag_object(object_id="foo", taxonomy=self.language_taxonomy, tags=[lang_value]) - # now a search will return matching tags: - after_langs = [ - tag["external_id"] for tag in - tagging_api.get_root_tags(self.language_taxonomy) - ] - expected_langs = [lang_code for lang_code, _ in test_languages] - assert after_langs == expected_langs - def test_search_tags(self) -> None: result = tagging_api.search_tags(self.taxonomy, search_term='eU') assert pretty_format_tags(result, parent=False) == [ @@ -221,28 +172,6 @@ def test_search_tags(self) -> None: 'Eukaryota (children: 0)', ] - @override_settings(LANGUAGES=test_languages) - def test_search_language_tags(self): - """ - For the language taxonomy, listing and searching tags will only show - tags that have been used at least once. - """ - before_langs = [ - tag["external_id"] for tag in - tagging_api.search_tags(self.language_taxonomy, search_term='IsH') - ] - assert before_langs == ["en"] - # Use a few more tags: - for _lang_code, lang_value in test_languages: - tagging_api.tag_object(object_id="foo", taxonomy=self.language_taxonomy, tags=[lang_value]) - # now a search will return matching tags: - after_langs = [ - tag["external_id"] for tag in - tagging_api.search_tags(self.language_taxonomy, search_term='IsH') - ] - expected_langs = [lang_code for lang_code, _ in filtered_test_languages] - assert after_langs == expected_langs - def test_get_children_tags(self) -> None: """ Test getting the children of a particular tag in a closed taxonomy. @@ -611,15 +540,14 @@ def test_tag_object_case_id(self) -> None: assert len(object_tags_upper) == 1 assert object_tags_upper[0].tag_id == self.archaea.id - @override_settings(LANGUAGES=test_languages) - def test_tag_object_language_taxonomy(self) -> None: + def test_tag_object_read_only_taxonomy(self) -> None: tags_list = [ - ["Azerbaijani"], - ["English"], + ["System Tag 1"], + ["System Tag 2"], ] for tags in tags_list: - tagging_api.tag_object("biology101", self.language_taxonomy, tags) + tagging_api.tag_object("biology101", self.system_taxonomy, tags) # Ensure the expected number of tags exist in the database object_tags = tagging_api.get_object_tags("biology101") @@ -629,48 +557,18 @@ def test_tag_object_language_taxonomy(self) -> None: object_tag.full_clean() # Check full model validation assert object_tag.value == tags[index] assert not object_tag.is_deleted - assert object_tag.taxonomy == self.language_taxonomy - assert object_tag.export_id == self.language_taxonomy.export_id + assert object_tag.taxonomy == self.system_taxonomy + assert object_tag.export_id == self.system_taxonomy.export_id assert object_tag.object_id == "biology101" - @override_settings(LANGUAGES=test_languages) - def test_tag_object_language_taxonomy_invalid(self) -> None: + def test_tag_object_read_only_taxonomy_invalid(self) -> None: with self.assertRaises(tagging_api.TagDoesNotExist): tagging_api.tag_object( "biology101", - self.language_taxonomy, - ["Spanish"], + self.system_taxonomy, + ["Not a real tag"], ) - def test_tag_object_model_system_taxonomy(self) -> None: - users = [ - self.user_1, - self.user_2, - ] - - for user in users: - tags = [user.username] - tagging_api.tag_object("biology101", self.user_taxonomy, tags) - - # Ensure the expected number of tags exist in the database - object_tags = tagging_api.get_object_tags("biology101") - # And the expected number of tags were returned - assert len(object_tags) == len(tags) - for object_tag in object_tags: - object_tag.full_clean() # Check full model validation - assert object_tag.tag - assert object_tag.tag.external_id == str(user.id) - assert object_tag.tag.value == user.username - assert not object_tag.is_deleted - assert object_tag.taxonomy == self.user_taxonomy - assert object_tag.export_id == self.user_taxonomy.export_id - assert object_tag.object_id == "biology101" - - def test_tag_object_model_system_taxonomy_invalid(self) -> None: - tags = ["Invalid id"] - with self.assertRaises(tagging_api.TagDoesNotExist): - tagging_api.tag_object("biology101", self.user_taxonomy, tags) - def test_tag_object_limit(self) -> None: """ Test that the tagging limit is enforced. @@ -723,7 +621,7 @@ def test_get_object_tags_deleted_disabled(self) -> None: self.taxonomy.save() disabled_taxonomy = tagging_api.create_taxonomy("Disabled Taxonomy", allow_free_text=True) tagging_api.tag_object(object_id=obj_id, taxonomy=self.taxonomy, tags=["DPANN", "Chordata"]) - tagging_api.tag_object(object_id=obj_id, taxonomy=self.language_taxonomy, tags=["English"]) + tagging_api.tag_object(object_id=obj_id, taxonomy=self.system_taxonomy, tags=["System Tag 1"]) tagging_api.tag_object(object_id=obj_id, taxonomy=self.free_text_taxonomy, tags=["has a notochord"]) tagging_api.tag_object(object_id=obj_id, taxonomy=disabled_taxonomy, tags=["disabled tag"]) @@ -732,11 +630,11 @@ def get_object_tags(): # Before deleting/disabling: assert get_object_tags() == [ - "7-disabled-taxonomy: disabled tag", - "6-free-text: has a notochord", - "languages-v1: English", + "5-disabled-taxonomy: disabled tag", + "4-free-text: has a notochord", "life_on_earth: Archaea>DPANN", - "life_on_earth: Eukaryota>Animalia>Chordata" + "life_on_earth: Eukaryota>Animalia>Chordata", + "read_only_taxonomy: System Tag 1", ] # Now delete and disable things: @@ -748,8 +646,8 @@ def get_object_tags(): # Now retrieve the tags again: assert get_object_tags() == [ - "languages-v1: English", "life_on_earth: Eukaryota>Animalia>Chordata", + "read_only_taxonomy: System Tag 1", ] @ddt.data( @@ -895,7 +793,7 @@ def test_get_object_tag_counts_deleted_disabled(self) -> None: obj2 = "object_id2" # Give each object 2 tags: tagging_api.tag_object(object_id=obj1, taxonomy=self.taxonomy, tags=["DPANN"]) - tagging_api.tag_object(object_id=obj1, taxonomy=self.language_taxonomy, tags=["English"]) + tagging_api.tag_object(object_id=obj1, taxonomy=self.system_taxonomy, tags=["System Tag 1"]) tagging_api.tag_object(object_id=obj2, taxonomy=self.taxonomy, tags=["Chordata"]) tagging_api.tag_object(object_id=obj2, taxonomy=self.free_text_taxonomy, tags=["has a notochord"]) @@ -909,7 +807,7 @@ def test_get_object_tag_counts_deleted_disabled(self) -> None: self.free_text_taxonomy.save() assert tagging_api.get_object_tag_counts("object_*") == {obj1: 1, obj2: 1} # Also check the result with count_implicit: - # "English" has no implicit tags but "Chordata" has two, so we expect these totals: + # "System Tag 1" has no implicit tags but "Chordata" has two, so we expect these totals: assert tagging_api.get_object_tag_counts("object_*", count_implicit=True) == {obj1: 1, obj2: 3} # But, by the way, if we re-enable the taxonomy and restore the tag, the counts return: @@ -924,14 +822,14 @@ def test_copy_tags(self) -> None: obj2 = "object_id2" tags_list = [ - { - "value": "English", - "taxonomy": self.language_taxonomy, - }, { "value": "DPANN", "taxonomy": self.taxonomy, }, + { + "value": "System Tag 1", + "taxonomy": self.system_taxonomy, + }, ] for tag_object in tags_list: @@ -954,7 +852,7 @@ def test_copy_tags_with_non_copied(self) -> None: obj1 = "object_id1" obj2 = "object_id2" - tagging_api.tag_object(object_id=obj1, taxonomy=self.language_taxonomy, tags=["English"]) + tagging_api.tag_object(object_id=obj1, taxonomy=self.system_taxonomy, tags=["System Tag 1"]) tagging_api.tag_object(object_id=obj2, taxonomy=self.taxonomy, tags=["Chordata"]) tagging_api.tag_object(object_id=obj2, taxonomy=self.free_text_taxonomy, tags=["has a notochord"]) @@ -969,16 +867,16 @@ def test_copy_tags_with_non_copied(self) -> None: "taxonomy": self.free_text_taxonomy, "copied": False, }, - { - "value": "English", - "taxonomy": self.language_taxonomy, - "copied": True, - }, { "value": "Chordata", "taxonomy": self.taxonomy, "copied": False, }, + { + "value": "System Tag 1", + "taxonomy": self.system_taxonomy, + "copied": True, + }, ] assert len(object_tags) == 3 for index, object_tag in enumerate(object_tags): @@ -1057,7 +955,7 @@ def test_unmark_copied_tags(self) -> None: obj2 = "object_id2" # Put 2 tags on obj1 - tagging_api.tag_object(object_id=obj1, taxonomy=self.language_taxonomy, tags=["English"]) + tagging_api.tag_object(object_id=obj1, taxonomy=self.taxonomy, tags=["DPANN"]) tagging_api.tag_object(object_id=obj1, taxonomy=self.system_taxonomy, tags=["System Tag 1"]) # Copy tags from obj1 to obj2 diff --git a/tests/openedx_tagging/test_models.py b/tests/openedx_tagging/test_models.py index 0590513b0..0799f7e6d 100644 --- a/tests/openedx_tagging/test_models.py +++ b/tests/openedx_tagging/test_models.py @@ -16,7 +16,7 @@ from django.test.testcases import TestCase from openedx_tagging import api -from openedx_tagging.models import LanguageTaxonomy, ObjectTag, Tag, Taxonomy +from openedx_tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.models.utils import RESERVED_TAG_CHARS from openedx_tagging.signal_handlers import _is_explicit_tag_delete from openedx_tagging.tasks import ( @@ -45,9 +45,7 @@ def setUp(self): super().setUp() # Core pre-defined taxonomies for testing: self.taxonomy = Taxonomy.objects.get(name="Life on Earth") - self.system_taxonomy = Taxonomy.objects.get(name="System defined taxonomy") - self.language_taxonomy = LanguageTaxonomy.objects.get(name="Languages") - self.user_taxonomy = Taxonomy.objects.get(name="User Authors").cast() + self.system_taxonomy = Taxonomy.objects.get(name="Read Only Taxonomy") self.free_text_taxonomy = api.create_taxonomy(name="Free Text", allow_free_text=True) self.import_taxonomy = Taxonomy.objects.get(name="Import Taxonomy Test") @@ -60,7 +58,6 @@ def setUp(self): self.mammalia = get_tag("Mammalia") self.animalia = get_tag("Animalia") self.system_taxonomy_tag = get_tag("System Tag 1") - self.english_tag = self.language_taxonomy.tag_for_external_id("en") self.user_1 = get_user_model()( id=1, username="test_user_1", @@ -137,28 +134,6 @@ def create_100_taxonomies(self): return dummy_taxonomies -class TaxonomyTestSubclassA(Taxonomy): - """ - Model A for testing the taxonomy subclass casting. - """ - - class Meta: - managed = False - proxy = True - app_label = "oel_tagging" - - -class TaxonomyTestSubclassB(TaxonomyTestSubclassA): - """ - Model B for testing the taxonomy subclass casting. - """ - - class Meta: - managed = False - proxy = True - app_label = "oel_tagging" - - class ObjectTagTestSubclass(ObjectTag): """ Model for testing the ObjectTag copy. @@ -176,60 +151,37 @@ class TestTagTaxonomy(TestTagTaxonomyMixin, TestCase): Test the Tag and Taxonomy models' properties and methods. """ - def test_system_defined(self): - assert not self.taxonomy.system_defined - assert self.system_taxonomy.cast().system_defined + def test_read_only(self): + assert not self.taxonomy.read_only + assert self.system_taxonomy.read_only + + def test_read_only_taxonomy_tags_immutable(self): + """ + The tags of a read-only taxonomy cannot be added, edited, or deleted. + """ + with pytest.raises(ValueError) as add_exc: + self.system_taxonomy.add_tag("New Tag") + assert "read-only" in str(add_exc.value) + + with pytest.raises(ValueError) as update_exc: + self.system_taxonomy.update_tag("System Tag 1", "Renamed") + assert "read-only" in str(update_exc.value) + + with pytest.raises(ValueError) as delete_exc: + self.system_taxonomy.delete_tags(["System Tag 1"]) + assert "read-only" in str(delete_exc.value) def test_representations(self): assert ( str(self.taxonomy) == repr(self.taxonomy) == " (1) Life on Earth" ) assert ( - str(self.language_taxonomy) - == repr(self.language_taxonomy) - == " (-1) Languages" + str(self.system_taxonomy) + == repr(self.system_taxonomy) + == " (4) Read Only Taxonomy" ) assert str(self.bacteria) == repr(self.bacteria) == " (1) Bacteria" - def test_taxonomy_cast(self): - for subclass in ( - TaxonomyTestSubclassA, - # Ensure that casting to a sub-subclass works as expected - TaxonomyTestSubclassB, - # 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", export_id='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 subclass of Taxonomy" in str(exc.exception) - def test_unique_tags(self): # Creating new tag Tag( diff --git a/tests/openedx_tagging/test_rules.py b/tests/openedx_tagging/test_rules.py index 7e6cc7e02..20703449d 100644 --- a/tests/openedx_tagging/test_rules.py +++ b/tests/openedx_tagging/test_rules.py @@ -76,14 +76,28 @@ def test_add_change_taxonomy(self, perm): "oel_tagging.change_taxonomy", "oel_tagging.delete_taxonomy", ) - def test_system_taxonomy(self, perm): + def test_read_only_taxonomy(self, perm): """ - Taxonomy administrators cannot edit system taxonomies + Taxonomy administrators can still modify or delete a read-only taxonomy + itself (e.g. to toggle its read_only flag); only its tags are protected. """ assert self.superuser.has_perm(perm, self.system_taxonomy) - assert not self.staff.has_perm(perm, self.system_taxonomy) + assert self.staff.has_perm(perm, self.system_taxonomy) assert not self.learner.has_perm(perm, self.system_taxonomy) + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + "oel_tagging.delete_tag", + ) + def test_read_only_taxonomy_tags(self, perm): + """ + Even taxonomy administrators cannot modify the tags of a read-only taxonomy. + """ + assert self.superuser.has_perm(perm, self.system_taxonomy_tag) + assert not self.staff.has_perm(perm, self.system_taxonomy_tag) + assert not self.learner.has_perm(perm, self.system_taxonomy_tag) + @ddt.data( True, False, diff --git a/tests/openedx_tagging/test_system_defined_models.py b/tests/openedx_tagging/test_system_defined_models.py deleted file mode 100644 index 5fd8565c9..000000000 --- a/tests/openedx_tagging/test_system_defined_models.py +++ /dev/null @@ -1,280 +0,0 @@ -""" -Test the tagging system-defined taxonomy models -""" -from __future__ import annotations - -from datetime import datetime, timezone - -import ddt # type: ignore[import] -import pytest -from django.test import TestCase, override_settings - -from openedx_content.applets.publishing.models import LearningPackage -from openedx_tagging import api -from openedx_tagging.models import Taxonomy -from openedx_tagging.models.system_defined import ModelSystemDefinedTaxonomy, UserSystemDefinedTaxonomy - -from .test_models import TestTagTaxonomyMixin - -test_languages = [ - ("en", "English"), - ("en-uk", "English (United Kingdom)"), - ("az", "Azerbaijani"), - ("id", "Indonesian"), - ("qu", "Quechua"), - ("zu", "Zulu"), -] - - -class EmptyTestClass: - """ - Empty class used for testing - """ - - -class LPTaxonomyTest(ModelSystemDefinedTaxonomy): - """ - Model used for testing - points to LearningPackage instances - """ - @property - def tag_class_model(self): - return LearningPackage - - @property - def tag_class_value_field(self) -> str: - return "package_ref" - - @property - def tag_class_key_field(self) -> str: - return "uuid" - - class Meta: - proxy = True - managed = False - app_label = "oel_tagging" - - -class CaseInsensitiveTitleLPTaxonomy(LPTaxonomyTest): - """ - Model that points to LearningPackage instances but uses 'title' as values - """ - @property - def tag_class_value_field(self) -> str: - # Title isn't unique, so wouldn't make a good 'value' in real usage, but title is case-insensitive so we use it - # here to test case insensitivity. (On MySQL, only columns with case-insensitive collation can be used with - # case-insensitive comparison operators. On SQLite you could just use the 'key' field for testing, and it works - # fine.) - return "title" - - class Meta: - proxy = True - managed = False - app_label = "oel_tagging" - - -@ddt.ddt -class TestModelSystemDefinedTaxonomy(TestTagTaxonomyMixin, TestCase): - """ - Test for Model Model System defined taxonomy - """ - - @staticmethod - def _create_learning_pkg(**kwargs) -> LearningPackage: - timestamp = datetime.now(tz=timezone.utc) - return LearningPackage.objects.create(**kwargs, created=timestamp, updated=timestamp) - - @classmethod - def setUpClass(cls): - super().setUpClass() - # Create two learning packages and a taxonomy that can tag any object using learning packages as tags: - cls.learning_pkg_1 = cls._create_learning_pkg(package_ref="p1", title="Learning Package 1") - cls.learning_pkg_2 = cls._create_learning_pkg(package_ref="p2", title="Learning Package 2") - cls.lp_taxonomy = LPTaxonomyTest.objects.create( - taxonomy_class=LPTaxonomyTest, - name="LearningPackage Taxonomy", - allow_multiple=True, - export_id="learning_package_taxonomy", - ) - # Also create an "Author" taxonomy that can tag any object using user IDs/usernames: - cls.author_taxonomy = UserSystemDefinedTaxonomy.objects.create( - taxonomy_class=UserSystemDefinedTaxonomy, - name="Authors", - allow_multiple=True, - export_id="authors", - ) - - def test_lp_taxonomy_validation(self): - """ - Test that the validation methods of the Learning Package Taxonomy are working - """ - # Create a new LearningPackage - we know no Tag instances will exist for it yet. - valid_lp = self._create_learning_pkg(package_ref="valid-lp", title="New Learning Packacge") - # The taxonomy can validate tags by value which we've defined as they 'key' of the LearningPackage: - assert self.lp_taxonomy.validate_value(self.learning_pkg_2.package_ref) is True - assert self.lp_taxonomy.validate_value(self.learning_pkg_2.package_ref) is True - assert self.lp_taxonomy.validate_value(valid_lp.package_ref) is True - assert self.lp_taxonomy.validate_value("foo") is False - # The taxonomy can also validate tags by external_id, which we've defined as the UUID of the LearningPackage: - assert self.lp_taxonomy.validate_external_id(self.learning_pkg_2.uuid) is True - assert self.lp_taxonomy.validate_external_id(self.learning_pkg_2.uuid) is True - assert self.lp_taxonomy.validate_external_id(valid_lp.uuid) is True - assert self.lp_taxonomy.validate_external_id("ba11225e-9ec9-4a50-87ea-3155c7c20466") is False - - def test_author_taxonomy_validation(self): - """ - Test the validation methods of the Author Taxonomy (Author = User) - """ - assert self.author_taxonomy.validate_value(self.user_1.username) is True - assert self.author_taxonomy.validate_value(self.user_2.username) is True - assert self.author_taxonomy.validate_value("not a user") is False - # And we can validate by ID if we want: - assert self.author_taxonomy.validate_external_id(str(self.user_1.id)) is True - assert self.author_taxonomy.validate_external_id(str(self.user_2.id)) is True - assert self.author_taxonomy.validate_external_id("8742590") is False - - @ddt.data( - "validate_value", "tag_for_value", "validate_external_id", "tag_for_external_id", - ) - def test_warns_uncasted(self, method): - """ - Test that if we use a taxonomy directly without cast(), we get warned. - """ - base_taxonomy = Taxonomy.objects.get(pk=self.lp_taxonomy.pk) - with pytest.raises(TypeError) as excinfo: - # e.g. base_taxonomy.validate_value("foo") - getattr(base_taxonomy, method)("foo") - assert "Taxonomy was used incorrectly - without .cast()" in str(excinfo.value) - - def test_simple_tag_object(self): - """ - Test applying tags to an object. - """ - object1_id, object2_id = "obj1", "obj2" - api.tag_object(object1_id, self.lp_taxonomy, ["p1"]) - api.tag_object(object2_id, self.lp_taxonomy, ["p1", "p2"]) - assert [t.value for t in api.get_object_tags(object1_id)] == ["p1"] - assert [t.value for t in api.get_object_tags(object2_id)] == ["p1", "p2"] - - def test_invalid_tag(self): - """ - Trying to apply an invalid tag raises TagDoesNotExist - """ - with pytest.raises(api.TagDoesNotExist): - api.tag_object("obj1", self.lp_taxonomy, ["nonexistent"]) - - def test_case_insensitive_values(self): - """ - For now, values are case insensitive. We may change that in the future. - """ - object1_id, object2_id = "obj1", "obj2" - taxonomy = CaseInsensitiveTitleLPTaxonomy.objects.create( - taxonomy_class=CaseInsensitiveTitleLPTaxonomy, - name="LearningPackage Title Taxonomy", - allow_multiple=True, - export_id="learning_package_title_taxonomy", - ) - api.tag_object(object1_id, taxonomy, ["LEARNING PACKAGE 1"]) - api.tag_object(object2_id, taxonomy, ["Learning Package 1", "LEARNING PACKAGE 2"]) - # But they always get normalized to the case used on the actual model: - assert [t.value for t in api.get_object_tags(object1_id)] == ["Learning Package 1"] - assert [t.value for t in api.get_object_tags(object2_id)] == ["Learning Package 1", "Learning Package 2"] - - def test_multiple_taxonomies(self): - """ - Test using several different instances of a taxonomy to tag the same object - """ - reviewer_taxonomy = UserSystemDefinedTaxonomy.objects.create( - taxonomy_class=UserSystemDefinedTaxonomy, - name="Reviewer", - allow_multiple=True, - export_id="reviewer", - ) - pr_1_id, pr_2_id = "pull_request_1", "pull_request_2" - - # Tag PR 1 as having "Author: user1, user2; Reviewer: user2" - api.tag_object(pr_1_id, self.author_taxonomy, [self.user_1.username, self.user_2.username]) - api.tag_object(pr_1_id, reviewer_taxonomy, [self.user_2.username]) - - # Tag PR 2 as having "Author: user2, reviewer: user1" - api.tag_object(pr_2_id, self.author_taxonomy, [self.user_2.username]) - api.tag_object(pr_2_id, reviewer_taxonomy, [self.user_1.username]) - - # Check the results: - assert [f"{t.taxonomy.name}:{t.value}" for t in api.get_object_tags(pr_1_id)] == [ - f"Authors:{self.user_1.username}", - f"Authors:{self.user_2.username}", - f"Reviewer:{self.user_2.username}", - ] - assert [f"{t.taxonomy.name}:{t.value}" for t in api.get_object_tags(pr_2_id)] == [ - f"Authors:{self.user_2.username}", - f"Reviewer:{self.user_1.username}", - ] - - def test_tag_object_resync(self): - """ - If the value changes, we can use the new value to tag objects, and the - Tag will be updated automatically. - """ - # Tag two objects with "Author: user_1" - object1_id, object2_id, other_obj_id = "obj1", "obj2", "other" - api.tag_object(object1_id, self.author_taxonomy, [self.user_1.username]) - api.tag_object(object2_id, self.author_taxonomy, [self.user_1.username]) - initial_object_tags = api.get_object_tags(object1_id) - assert [t.value for t in initial_object_tags] == [self.user_1.username] - assert not list(api.get_object_tags(other_obj_id)) - # Change user_1's username: - new_username = "new_username" - self.user_1.username = new_username - self.user_1.save() - # Now we update the tags on just one of the objects: - api.tag_object(object1_id, self.author_taxonomy, [new_username]) - assert [t.value for t in api.get_object_tags(object1_id)] == [new_username] - # But because this will have updated the shared Tag instance, object2 will also be updated as a side effect. - # This is good - all the objects throughout the system with this tag now show the new value. - assert [t.value for t in api.get_object_tags(object2_id)] == [new_username] - # And just to make sure there are no other random changes to other objects: - assert not list(api.get_object_tags(other_obj_id)) - - def test_tag_object_delete_user(self): - """ - Using a deleted model instance as a tag will raise TagDoesNotExist - """ - # Tag an object with "Author: user_1" - object_id = "obj123" - api.tag_object(object_id, self.author_taxonomy, [self.user_1.username]) - assert [t.value for t in api.get_object_tags(object_id)] == [self.user_1.username] - # Test after delete user - self.user_1.delete() - with self.assertRaises(api.TagDoesNotExist): - api.tag_object(object_id, self.author_taxonomy, [self.user_1.username]) - - -@ddt.ddt -@override_settings(LANGUAGES=test_languages) -class TestLanguageTaxonomy(TestTagTaxonomyMixin, TestCase): - """ - Test for Language taxonomy - """ - - def test_validate_lang_ids(self): - """ - Whether or not languages are available as tags depends on the django settings - """ - assert self.language_taxonomy.validate_external_id("en") is True - assert self.language_taxonomy.tag_for_external_id("en").value == "English" - assert self.language_taxonomy.tag_for_external_id("en-uk").value == "English (United Kingdom)" - assert self.language_taxonomy.tag_for_external_id("id").value == "Indonesian" - - assert self.language_taxonomy.validate_external_id("xx") is False - with pytest.raises(api.TagDoesNotExist): - self.language_taxonomy.tag_for_external_id("xx") - - @override_settings(LANGUAGES=[("fr", "Français")]) - def test_minimal_languages(self): - """ - Whether or not languages are available as tags depends on the django settings - """ - assert self.language_taxonomy.validate_external_id("en") is False - with pytest.raises(api.TagDoesNotExist): - self.language_taxonomy.tag_for_external_id("en") - assert self.language_taxonomy.tag_for_external_id("fr").value == "Français" diff --git a/tests/openedx_tagging/test_views.py b/tests/openedx_tagging/test_views.py index 9851ce0d3..e196c11a5 100644 --- a/tests/openedx_tagging/test_views.py +++ b/tests/openedx_tagging/test_views.py @@ -20,7 +20,6 @@ from openedx_tagging.import_export import api as import_export_api from openedx_tagging.import_export.parsers import ParserFormat from openedx_tagging.models import ObjectTag, Tag, Taxonomy -from openedx_tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.rest_api.paginators import TagsPagination from openedx_tagging.rules import can_change_object_tag_objectid, can_view_object_tag_objectid @@ -42,8 +41,6 @@ OBJECT_TAG_COUNTS_URL = "/tagging/rest_api/v1/object_tag_counts/{object_id_pattern}/" OBJECT_TAGS_UPDATE_URL = "/tagging/rest_api/v1/object_tags/{object_id}/" -LANGUAGE_TAXONOMY_ID = -1 - def check_taxonomy( data, @@ -54,7 +51,7 @@ def check_taxonomy( enabled=True, allow_multiple=True, allow_free_text=False, - system_defined=False, + read_only=False, visible_to_authors=True, can_change_taxonomy=None, can_delete_taxonomy=None, @@ -70,7 +67,7 @@ def check_taxonomy( assert data["enabled"] == enabled assert data["allow_multiple"] == allow_multiple assert data["allow_free_text"] == allow_free_text - assert data["system_defined"] == system_defined + assert data["read_only"] == read_only assert data["visible_to_authors"] == visible_to_authors assert data["can_change_taxonomy"] == can_change_taxonomy assert data["can_delete_taxonomy"] == can_delete_taxonomy @@ -105,14 +102,14 @@ class TestTaxonomyViewSet(TestTaxonomyViewMixin): """ @ddt.data( - (None, status.HTTP_200_OK, 4), - (1, status.HTTP_200_OK, 3), + (None, status.HTTP_200_OK, 3), + (1, status.HTTP_200_OK, 2), (0, status.HTTP_200_OK, 1), - (True, status.HTTP_200_OK, 3), + (True, status.HTTP_200_OK, 2), (False, status.HTTP_200_OK, 1), - ("True", status.HTTP_200_OK, 3), + ("True", status.HTTP_200_OK, 2), ("False", status.HTTP_200_OK, 1), - ("1", status.HTTP_200_OK, 3), + ("1", status.HTTP_200_OK, 2), ("0", status.HTTP_200_OK, 1), (2, status.HTTP_400_BAD_REQUEST, None), ("invalid", status.HTTP_400_BAD_REQUEST, None), @@ -133,7 +130,6 @@ def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected assert response.status_code == expected_status # If we were able to list the taxonomies, check that we got the expected number back - # We take into account the Language Taxonomy that is created by the system in a migration if status.is_success(expected_status): assert len(response.data["results"]) == expected_count @@ -164,22 +160,6 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c # Check response if status.is_success(expected_status): assert response.data["results"] == [ - { - "id": -1, - "name": "Languages", - "description": "Languages that are enabled on this system.", - "enabled": True, - "allow_multiple": False, - "allow_free_text": False, - "system_defined": True, - "visible_to_authors": True, - "tags_count": 0, - # System taxonomy cannot be modified - "can_change_taxonomy": False, - "can_delete_taxonomy": False, - "can_tag_object": False, - "export_id": "languages-v1", - }, { "id": taxonomy.id, "name": "Taxonomy enabled 1", @@ -187,7 +167,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "enabled": True, "allow_multiple": True, "allow_free_text": False, - "system_defined": False, + "read_only": False, "visible_to_authors": True, "tags_count": tags_count, # Enabled taxonomy can be modified by taxonomy admins @@ -196,7 +176,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c # can_tag_object is False because we default to not allowing users to tag arbitrary objects. # But specific uses of this code (like content_tagging) will override this perm for their use cases. "can_tag_object": False, - "export_id": "2-taxonomy-enabled-1", + "export_id": "1-taxonomy-enabled-1", }, ] assert response.data.get("can_add_taxonomy") == is_admin @@ -216,7 +196,7 @@ def test_list_taxonomy_pagination(self) -> None: assert response.status_code == status.HTTP_200_OK - self.assertEqual(set(t["name"] for t in response.data["results"]), set(("T2", "T3"))) + self.assertEqual(set(t["name"] for t in response.data["results"]), set(("T3", "T4"))) parsed_url = urlparse(response.data["next"]) next_page = parse_qs(parsed_url.query).get("page", [""])[0] @@ -228,11 +208,6 @@ def test_list_taxonomy_pagination(self) -> None: ) @ddt.unpack def test_list_taxonomy_empty(self, user_attr, expected_can_add) -> None: - # Delete the language taxonomy so we can get an empty list - language_taxonomy = api.get_taxonomy(LANGUAGE_TAXONOMY_ID) - if language_taxonomy: - language_taxonomy.delete() - url = TAXONOMY_LIST_URL user = getattr(self, user_attr) self.client.force_authenticate(user=user) @@ -260,12 +235,12 @@ def test_list_taxonomy_query_count(self): url = TAXONOMY_LIST_URL self.client.force_authenticate(user=self.user) - with self.assertNumQueries(3): + with self.assertNumQueries(2): response = self.client.get(url) assert response.status_code == 200 assert not response.data["can_add_taxonomy"] - assert len(response.data["results"]) == 3 + assert len(response.data["results"]) == 2 for taxonomy in response.data["results"]: assert not taxonomy["can_change_taxonomy"] assert not taxonomy["can_delete_taxonomy"] @@ -282,28 +257,6 @@ def test_list_invalid_page(self) -> None: assert response.status_code == status.HTTP_404_NOT_FOUND - def test_language_taxonomy(self): - """ - Test the "Language" taxonomy that's included. - """ - self.client.force_authenticate(user=self.user) - response = self.client.get(TAXONOMY_LIST_URL) - assert response.status_code == status.HTTP_200_OK - taxonomy_list = response.data["results"] - assert len(taxonomy_list) == 1 - check_taxonomy( - taxonomy_list[0], - taxonomy_id=LANGUAGE_TAXONOMY_ID, - name="Languages", - description="Languages that are enabled on this system.", - allow_multiple=False, # We may change this in the future to allow multiple language tags - system_defined=True, - can_change_taxonomy=False, - can_delete_taxonomy=False, - can_tag_object=False, - export_id='languages-v1', - ) - @ddt.data( (None, {"enabled": True}, status.HTTP_401_UNAUTHORIZED), (None, {"enabled": False}, status.HTTP_401_UNAUTHORIZED), @@ -338,12 +291,14 @@ def test_detail_taxonomy( expected_data["can_tag_object"] = False check_taxonomy(response.data, taxonomy_id=taxonomy.pk, **expected_data) # type: ignore[arg-type] - def test_detail_system_taxonomy(self): - url = TAXONOMY_DETAIL_URL.format(pk=LANGUAGE_TAXONOMY_ID) + def test_detail_read_only_taxonomy(self): + taxonomy = api.create_taxonomy(name="Read Only", read_only=True) + url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) self.client.force_authenticate(user=self.user) response = self.client.get(url) assert response.status_code == status.HTTP_200_OK + assert response.data["read_only"] is True def test_detail_taxonomy_404(self) -> None: url = TAXONOMY_DETAIL_URL.format(pk=123123) @@ -413,7 +368,7 @@ def test_create_without_export_id(self): check_taxonomy( response.data, taxonomy_id=response.data["id"], - export_id="2-taxonomy-data-3", + export_id="1-taxonomy-data-3", **create_data, ) @@ -429,17 +384,17 @@ def test_create_taxonomy_error(self, create_data: dict[str, str]): response = self.client.post(url, create_data, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST - @ddt.data({"name": "System defined taxonomy", "system_defined": True}) - def test_create_taxonomy_system_defined(self, create_data): + @ddt.data({"name": "Read only taxonomy", "read_only": True}) + def test_create_taxonomy_read_only(self, create_data): """ - Cannont create a taxonomy with system_defined=true + Taxonomy admins can create a read-only taxonomy through the API. """ url = TAXONOMY_LIST_URL self.client.force_authenticate(user=self.staff) response = self.client.post(url, create_data, format="json") assert response.status_code == status.HTTP_201_CREATED - assert not response.data["system_defined"] + assert response.data["read_only"] is True @ddt.data( (None, status.HTTP_401_UNAUTHORIZED), @@ -476,28 +431,27 @@ def test_update_taxonomy(self, user_attr, expected_status): "can_change_taxonomy": True, "can_delete_taxonomy": True, "can_tag_object": False, - "export_id": "2-test-update-taxonomy", + "export_id": "1-test-update-taxonomy", }, ) - @ddt.data( - (False, status.HTTP_200_OK), - (True, status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_update_taxonomy_system_defined(self, system_defined, expected_status): + @ddt.data(True, False) + def test_update_taxonomy_read_only(self, read_only): """ - Test that we can't update system_defined field + Admins can update a taxonomy regardless of its read_only status, and can + toggle the read_only flag itself. """ taxonomy = api.create_taxonomy( - name="test system taxonomy", - taxonomy_class=SystemDefinedTaxonomy if system_defined else None, + name="test read only taxonomy", + read_only=read_only, ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) self.client.force_authenticate(user=self.staff) - response = self.client.put(url, {"name": "new name"}, format="json") - assert response.status_code == expected_status + response = self.client.put(url, {"name": "new name", "read_only": not read_only}, format="json") + assert response.status_code == status.HTTP_200_OK + assert response.data["name"] == "new name" + assert response.data["read_only"] == (not read_only) def test_update_taxonomy_404(self): url = TAXONOMY_DETAIL_URL.format(pk=123123) @@ -536,28 +490,26 @@ def test_patch_taxonomy(self, user_attr, expected_status): "can_change_taxonomy": True, "can_delete_taxonomy": True, "can_tag_object": False, - "export_id": '2-test-patch-taxonomy', + "export_id": '1-test-patch-taxonomy', }, ) - @ddt.data( - (False, status.HTTP_200_OK), - (True, status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_patch_taxonomy_system_defined(self, system_defined, expected_status): + @ddt.data(True, False) + def test_patch_taxonomy_read_only(self, read_only): """ - Test that we can't patch system_defined field + Admins can patch a taxonomy regardless of its read_only status, including + toggling the read_only flag itself. """ taxonomy = api.create_taxonomy( - name="test system taxonomy", - taxonomy_class=SystemDefinedTaxonomy if system_defined else None, + name="test read only taxonomy", + read_only=read_only, ) url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) self.client.force_authenticate(user=self.staff) - response = self.client.patch(url, {"name": "New name"}, format="json") - assert response.status_code == expected_status + response = self.client.patch(url, {"read_only": not read_only}, format="json") + assert response.status_code == status.HTTP_200_OK + assert response.data["read_only"] == (not read_only) def test_patch_taxonomy_404(self): url = TAXONOMY_DETAIL_URL.format(pk=123123) @@ -743,7 +695,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status, object_id): # Apply the object tags that we're about to retrieve: api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"]) - api.tag_object(object_id=object_id, taxonomy=self.user_taxonomy, tags=[self.user_1.username]) + api.tag_object(object_id=object_id, taxonomy=self.system_taxonomy, tags=["System Tag 1"]) url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) @@ -782,17 +734,17 @@ def test_retrieve_object_tags(self, user_attr, expected_status, object_id): "export_id": "life_on_earth" }, { - "name": "User Authors", - "taxonomy_id": 3, + "name": "Read Only Taxonomy", + "taxonomy_id": 4, "can_tag_object": True, "tags": [ { - "value": "test_user_1", - "lineage": ["test_user_1"], + "value": "System Tag 1", + "lineage": ["System Tag 1"], "can_delete_objecttag": False, }, ], - "export_id": "user_authors" + "export_id": "read_only_taxonomy" } ], }, @@ -915,7 +867,7 @@ def test_retrieve_object_tags_taxonomy_queryparam( # Apply the object tags that we're about to retrieve: api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"]) - api.tag_object(object_id=object_id, taxonomy=self.user_taxonomy, tags=[self.user_1.username]) + api.tag_object(object_id=object_id, taxonomy=self.system_taxonomy, tags=["System Tag 1"]) url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) @@ -923,7 +875,7 @@ def test_retrieve_object_tags_taxonomy_queryparam( user = getattr(self, user_attr) self.client.force_authenticate(user=user) - response = self.client.get(url, {"taxonomy": self.user_taxonomy.pk}) + response = self.client.get(url, {"taxonomy": self.system_taxonomy.pk}) assert response.status_code == expected_status if status.is_success(expected_status): assert response.data == { @@ -933,17 +885,17 @@ def test_retrieve_object_tags_taxonomy_queryparam( "taxonomies": [ # The "Life on Earth" tags are excluded here... { - "name": "User Authors", - "taxonomy_id": 3, + "name": "Read Only Taxonomy", + "taxonomy_id": 4, "can_tag_object": True, "tags": [ { - "value": "test_user_1", - "lineage": ["test_user_1"], + "value": "System Tag 1", + "lineage": ["System Tag 1"], "can_delete_objecttag": False, }, ], - "export_id": "user_authors", + "export_id": "read_only_taxonomy", } ], }, @@ -1012,9 +964,9 @@ def test_object_tags_remaining_http_methods( @ddt.data( # Users and staff can add tags - (None, "language_taxonomy", {}, ["Portuguese"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"), - ("user_1", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK, "abc"), - ("staff", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK, "abc.xyz"), + (None, "system_taxonomy", {}, ["System Tag 1"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"), + ("user_1", "system_taxonomy", {}, ["System Tag 1"], status.HTTP_200_OK, "abc"), + ("staff", "system_taxonomy", {}, ["System Tag 1"], status.HTTP_200_OK, "abc.xyz"), # user_1s and staff can clear add tags (None, "taxonomy", {}, ["Fungi"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"), ("user_1", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK, "abc.xyz"), @@ -1038,9 +990,9 @@ def test_object_tags_remaining_http_methods( ("user_1", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN, "abc.xyz"), ("staff", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN, "abc"), # Can't add invalid/nonexistent tags using a closed taxonomy - (None, "language_taxonomy", {}, ["Invalid"], status.HTTP_401_UNAUTHORIZED, "abc"), - ("user_1", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc.xyz"), - ("staff", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc"), + (None, "system_taxonomy", {}, ["Invalid"], status.HTTP_401_UNAUTHORIZED, "abc"), + ("user_1", "system_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc.xyz"), + ("staff", "system_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc"), ("staff", "taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc.xyz"), ) @ddt.unpack @@ -1402,6 +1354,26 @@ def test_not_authorized_user(self): assert response.status_code == status.HTTP_403_FORBIDDEN + def test_read_only_taxonomy(self) -> None: + """ + Test the permissions results of a read only taxonomy + """ + ro_taxonomy = api.create_taxonomy(name="Read only taxonomy") + api.add_tag_to_taxonomy(ro_taxonomy, "tag1") + ro_taxonomy.read_only = True + ro_taxonomy.save() + url = TAXONOMY_TAGS_URL.format(pk=ro_taxonomy.pk) + + self.client.force_authenticate(user=self.staff) + response = self.client.get(url) + + assert response.status_code == status.HTTP_200_OK + assert not response.data["can_add_tag"] + assert len(response.data["results"]) == 1 + for tag in response.data["results"]: + assert not tag["can_change_tag"] + assert not tag["can_delete_tag"] + def test_small_taxonomy_root(self): """ Test explicitly requesting only the root tags of a small taxonomy. @@ -2023,7 +1995,7 @@ def test_create_tag_in_free_text_taxonomy(self): assert response.status_code == status.HTTP_400_BAD_REQUEST - def test_create_tag_in_system_defined_taxonomy(self): + def test_create_tag_in_read_only_taxonomy(self): self.client.force_authenticate(user=self.staff) new_tag_value = "New Tag" @@ -2031,8 +2003,8 @@ def test_create_tag_in_system_defined_taxonomy(self): "tag": new_tag_value } - # Setting taxonomy to be system defined - self.small_taxonomy.taxonomy_class = SystemDefinedTaxonomy + # Setting taxonomy to be read-only + self.small_taxonomy.read_only = True self.small_taxonomy.save() response = self.client.post( @@ -3512,7 +3484,7 @@ def test_import_free_text(self, file_format) -> None: ) assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.data == f"Invalid taxonomy ({self.taxonomy.id}): You cannot import a free-form taxonomy." + assert response.data == f"Invalid taxonomy ({self.taxonomy.id}): You cannot import to a free-text taxonomy." # Check if the taxonomy was no tags, since it is free text url = TAXONOMY_TAGS_URL.format(pk=self.taxonomy.id) @@ -3547,7 +3519,7 @@ def test_import_plan_free_text(self, file_format) -> None: ) assert response.status_code == status.HTTP_400_BAD_REQUEST - expected_message = f"Invalid taxonomy ({self.taxonomy.id}): You cannot import a free-form taxonomy." + expected_message = f"Invalid taxonomy ({self.taxonomy.id}): You cannot import to a free-text taxonomy." assert response.data["error"] == expected_message # Check if the taxonomy was no tags, since it is free text